Avoid modifying query graph edge conditions for @requires #1783
Conversation
👷 Deploy request for apollo-federation-docs pending review.Visit the deploys page to approve it
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
| @@ -542,6 +542,7 @@ export class SelectionSet { | |||
| private readonly _selections = new MultiMap<string, Selection>(); | |||
There was a problem hiding this comment.
I was thinking about potentially doing Freezable as a mixin class so that you could have only one implementation, but I think that may get a little confusing. Maybe add an interface instead?
There was a problem hiding this comment.
I added an abstract class to save duplication when possible.
| expect(() => s.mergeIn(s2)).toThrowError('Cannot add to frozen selection: { t { a } }'); | ||
| }); | ||
|
|
||
| it('it does not clone non-frozen selections when adding to another one', () => { |
There was a problem hiding this comment.
Should you have a test that checks that updateForAddingTo will clone if necessary or is that covered by one of these cases?
There was a problem hiding this comment.
The test using mergeIn indirectly test it, as it's the cloning in this method that end up ensuring SelectionSet::add and SelectionSet::mergeIn do a clone. In fact, updateForAddingTo is not really meant to be use externally, it's only public because there is no simple way to define a method in Selection that only SelectionSet can use (well, I can I can make it private and cheat the type-system with prototype, but didn't wanted).
That said, I'll admit I didn't overdo testing here, and I could probably add at least a few test to exercise fragment selections, and why not direct test for both add and updateForAddingTo.
| add(selection: Selection): Selection { | ||
| // It's a but to try to add to a frozen selection set | ||
| assert(!this.isFrozen(), () => `Cannot add to frozen selection: ${this}`); |
There was a problem hiding this comment.
We don't need to add this assertion to other methods? i.e addPath, etc?
There was a problem hiding this comment.
The code make sure all modifications are done by this method, so that one should be sufficient in practice.
| // edge freely). Which means we need to clone any existing conditions (so we can modify them), and need | ||
| // to re-freeze the result afterwards. | ||
| this._conditions = this._conditions | ||
| ? this._conditions.clone() |
There was a problem hiding this comment.
Is this clone() necessary? Aren't we already cloning in the constructor?
There was a problem hiding this comment.
We're also freezing in the constructor, and here we're trying to mutate the conditions to add new ones. So that clone is essentially about "unfreezing" the selections (but in theory, we could add an unfreeze method that just flip back the _isFrozen boolean, and that would be enough for that one specific case; but an unfreeze method makes the frozen story a bit dodgy and I don't think cloning here matters performance wise so ...).
|
Took the liberty to rebase because there was some conflict in |
The problem this fixes is that `SelectionSet` is mutable, but query graphs needs to be immutable post-construction since they are reused between query plans, and the edge of those use `SelectionSet` for the conditions. As a result, it was a bit too easy to inadvertently modify an edge conditions without meaning to, which is exactly what was happens in the handling of requires. Note that we could manually fix the exact issue in the requires-handling code by manually cloning the edge conditions we weren't properly cloning, but this commit take a slightly more systemic approach that attempts to make it harder for such issue to popup in another place later. That is, this adds the possibility for "freezing" a `SelectionSet`, which makes it effectively immutable from that point on. This should allow to find unexpected modification to edge conditions much sooner during testing. But this also allow to automatically clone frozen selection sets when they would otherwise be embeded/aliased in other non-frozen (and thus mutable) sets, which is actually the issue from apollographql#1750. Fixes apollographql#1750
The problem this fixes is that
SelectionSetis mutable, but querygraphs needs to be immutable post-construction since they are reused
between query plans, and the edge of those use
SelectionSetfor theconditions. As a result, it was a bit too easy to inadvertently
modify an edge conditions without meaning to, which is exactly what
was happens in the handling of requires.
Note that we could manually fix the exact issue in the requires-handling
code by manually cloning the edge conditions we weren't properly
cloning, but this commit take a slightly more systemic approach
that attempts to make it harder for such issue to popup in another
place later.
That is, this adds the possibility for "freezing" a
SelectionSet,which makes it effectively immutable from that point on. This should
allow to find unexpected modification to edge conditions much sooner
during testing. But this also allow to automatically clone frozen
selection sets when they would otherwise be embeded/aliased in other
non-frozen (and thus mutable) sets, which is actually the issue
from #1750.
Fixes #1750