Prevent over-eager merging of fields which have differing directive applications #2713
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: 80c8d22 The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for apollo-federation-docs canceled.
|
|
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. |
clenfest
approved these changes
Aug 3, 2023
trevor-scheer
added a commit
that referenced
this pull request
Aug 4, 2023
…pplications (#2713) Fix over-eager merging of fields with different directive applications Previously, the following query would incorrectly combine the selection set of `hello`, with both fields ending up under the @Skip condition: ```graphql query Test($skipField: Boolean!) { hello @Skip(if: $skipField) { world } hello { goodbye } } ``` This change identifies those two selections on `hello` as unique while constructing our operation representation so they aren't merged at all, leaving it to the subgraph to handle the operation as-is.
pcmanus
added a commit
to pcmanus/federation
that referenced
this pull request
Aug 7, 2023
The previously committed [apollographql#2713](apollographql#2713) fixed an issue introduced by [apollographql#2387](apollographql#2387), ensuring that querying the same field with different directives applications was not merged, similar to what was/is done for fragments. But the exact behaviour slightly differs between fields and fragments when it comes to `@defer` in that for fragments, we never merge 2 similar fragments where both have `@defer`, which we do merge for fields. Or to put it more concretely, in the following query: ```graphq query Test($skipField: Boolean!) { x { ... on X @defer { a } ... on X @defer { b } } } ``` the 2 `... on X @defer` are not merged, resulting in 2 deferred sections that can run in parallel. But following [apollographql#2713](apollographql#2713), query: ```graphq query Test($skipField: Boolean!) { x @defer { a } x @defer { b } } ``` _will_ merge both `x @defer`, resulting in a single deferred section. This fix changes that later behaviour so that the 2 `x @defer` are not merged and result in 2 deferred sections, consistently with both 1) the case of fragments and 2) the behaviour prior to [apollographql#2387](apollographql#2387).
pcmanus
added a commit
to pcmanus/federation
that referenced
this pull request
Aug 7, 2023
The previously committed [apollographql#2713](apollographql#2713) fixed an issue introduced by [apollographql#2387](apollographql#2387), ensuring that querying the same field with different directives applications was not merged, similar to what was/is done for fragments. But the exact behaviour slightly differs between fields and fragments when it comes to `@defer` in that for fragments, we never merge 2 similar fragments where both have `@defer`, which we do merge for fields. Or to put it more concretely, in the following query: ```graphq query Test($skipField: Boolean!) { x { ... on X @defer { a } ... on X @defer { b } } } ``` the 2 `... on X @defer` are not merged, resulting in 2 deferred sections that can run in parallel. But following [apollographql#2713](apollographql#2713), query: ```graphq query Test($skipField: Boolean!) { x @defer { a } x @defer { b } } ``` _will_ merge both `x @defer`, resulting in a single deferred section. This fix changes that later behaviour so that the 2 `x @defer` are not merged and result in 2 deferred sections, consistently with both 1) the case of fragments and 2) the behaviour prior to [apollographql#2387](apollographql#2387).
pcmanus
added a commit
to pcmanus/federation
that referenced
this pull request
Aug 10, 2023
The previously committed [apollographql#2713](apollographql#2713) fixed an issue introduced by [apollographql#2387](apollographql#2387), ensuring that querying the same field with different directives applications was not merged, similar to what was/is done for fragments. But the exact behaviour slightly differs between fields and fragments when it comes to `@defer` in that for fragments, we never merge 2 similar fragments where both have `@defer`, which we do merge for fields. Or to put it more concretely, in the following query: ```graphq query Test($skipField: Boolean!) { x { ... on X @defer { a } ... on X @defer { b } } } ``` the 2 `... on X @defer` are not merged, resulting in 2 deferred sections that can run in parallel. But following [apollographql#2713](apollographql#2713), query: ```graphq query Test($skipField: Boolean!) { x @defer { a } x @defer { b } } ``` _will_ merge both `x @defer`, resulting in a single deferred section. This fix changes that later behaviour so that the 2 `x @defer` are not merged and result in 2 deferred sections, consistently with both 1) the case of fragments and 2) the behaviour prior to [apollographql#2387](apollographql#2387).
jeffjakub
pushed a commit
that referenced
this pull request
Aug 15, 2023
…ly (#2720) The previously committed [#2713](#2713) fixed an issue introduced by [#2387](#2387), ensuring that querying the same field with different directives applications was not merged, similar to what was/is done for fragments. But the exact behaviour slightly differs between fields and fragments when it comes to `@defer` in that for fragments, we never merge 2 similar fragments where both have `@defer`, which we do merge for fields. Or to put it more concretely, in the following query: ```graphq query Test($skipField: Boolean!) { x { ... on X @defer { a } ... on X @defer { b } } } ``` the 2 `... on X @defer` are not merged, resulting in 2 deferred sections that can run in parallel. But following [#2713](#2713), query: ```graphq query Test($skipField: Boolean!) { x @defer { a } x @defer { b } } ``` _will_ merge both `x @defer`, resulting in a single deferred section. This fix changes that later behaviour so that the 2 `x @defer` are not merged and result in 2 deferred sections, consistently with both 1) the case of fragments and 2) the behaviour prior to [#2387](#2387).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #2712
Directive applications are an interesting case where multiple usages of an unaliased field within the same selection set is permitted. See test cases for examples. I've confirmed that the resulting subgraph fetches after this change are still valid graphql operations (via a small test Apollo Server), so it makes sense to me that we "preserve" rather than merge in these cases and forward along a more complete version of the request to the subgraph.
The fix here is to have fields id themselves using a more complex key which includes their applied directives when we parse the operation, that way their selections aren't grouped into the same entry if the directive applications differ.