Fix issue with @requires and conditional queries (@include and @skip)
#1835
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
👷 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. |
|
Added a 3rd commit to this PR as a 3rd issue around |
added 3 commits
May 17, 2022 11:16
A plans are built, the query planner has a notion of context
(`PathContext` in the), which currently is only use to remember which
`@include` or `@skip` are in effect at any point of the query planning.
That context wasn't always properly taken into account when handling
the inputs for a `@require`. Note that as for the current code,
the consequence was extremely minor due to the fact that the current
query plan format "loses" directives like `@include` and `@skip`
in the fetch inputs and in practice it means that the only consequence
of this was a fetch in the plan may have inputs looking like:
```
{
... on T {
__typename
id
}
... on T {
a
}
}
```
which is valid but is a bit confusing as to why both parts aren't
merged (and the underlying reason what that one of the branch had
a `@include` condition under hood while one didn't and so they
weren't merge).
In any case, while the consequence is very minor, the handling was
still somewhat incorrect. In particular, we should (imo) fix the
fact that `@include` and `@skip` are not in the query plan inputs,
and once we do the fix of this commit will become more important.
If a field has a `@require`, but that field is only conditionally requested in the query (through `@include` or `@skip`), then the requirement for this field should also ideally only be queried conditionally, but this wasn't always the case (due to a method that merges fetch-group ignoring some directives).
The code that recognize (and optimize accordindly) when a `@require` condition is taken _just after_ a key edge (which is the most common case in practice) was mistakenly assuming that the selection set "just after a key edge" was only starting by a single fragment. In practice, when multiple conditions (`@skip`/`@include`) are involved, all those conditions are preserved and may be layed out on multiple conditions. That is, after a key edge, the selection set may look like: ```graphql ... on EntityType @Skip(if: $c1) { ... on EntityType @include(if: $c2) { <rest of the selection> } } ``` In that case, the wrong part of the code was taken and this resulted in unoptimized query plans, ones having useless fetches. The commit fixes the condition, and ensure that now that the proper optimizations do apply, we streamline the created result sets by avoiding repeating conditions multiple times.
pcmanus
force-pushed
the
include-and-requires
branch
from
ff119e3
to
1428453
Compare
Merged
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.
Writing a few unit test around the interaction
@requireand@include/@skip, I noticed 2 issues/inefficiencies. See the commits for more details.