fix(federation): correct order of @skip and @include conditions on the same selection #6137
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
…n the same selection PR #6120 was the wrong solution. JS doesn't actually maintain the order of the conditions, like I thought. Instead, it always puts `@skip` first and `@include` second, the opposite of what Rust was doing. The queries I was testing with just happened to pass in #6120. This changes the implementation of `Conditions::from_directives` to pick the directives out manually instead of iterating. Technically, this does two iterations of the directive list...but, the code is a bit simpler, I think.
goto-bus-stop
requested review from
dariuszkuc,
sachindshinde,
SimonSapin,
lrlna,
TylerBloom and
duckki
as code owners
✅ Docs Preview ReadyNo new or changed pages found. |
|
@goto-bus-stop, please consider creating a changeset entry in |
duckki
requested changes
Oct 10, 2024
| "skip" => true, | ||
| _ => continue, | ||
|
|
||
| if let Some(skip) = directives.get("skip") { |
There was a problem hiding this comment.
It appears that the directives may have multiple skip/include directive applications. This will handle only one of each type.
There was a problem hiding this comment.
Per the spec @skip and @include are not repeatable so they cannot appear multiple times.
duckki
approved these changes
Oct 10, 2024
duckki
reviewed
Oct 11, 2024
| Ok(Self::from_variables(variables)) | ||
| } | ||
|
|
||
| // TODO(@goto-bus-stop): what exactly is the difference between this and `Self::merge`? |
There was a problem hiding this comment.
Indeed, merge makes more sense. It's unclear why update_with is so loose in the Boolean(false) cases.
There was a problem hiding this comment.
I played around a bit more with this code and learned what this actually does.
If you nested condition nodes, update_with() will remove from the inner node those conditions that were already matched by the outer node. So it's very different from merge() :)
I'll add a doc comment for that
|
CI performance tests
|
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.
PR #6120 was the wrong solution. JS doesn't actually maintain the order
of the conditions, like I thought. Instead, it always puts
@skipfirstand
@includesecond, the opposite of what Rust was doing.https://github.com/apollographql/federation/blob/cc4573471696ef78d04fa00c4cf8e5c50314ba9f/query-graphs-js/src/pathContext.ts#L13-L33
The queries I was testing with just happened to pass in #6120.
This changes the implementation of
Conditions::from_directivestopick the directives out manually instead of iterating. Technically, this
does two iterations of the directive list...but, the code is a bit
simpler, I think.
In the second commit I do a small refactor to replace the
is_negatedboolean by a
ConditionKindenum. I found it quite tricky toconstantly translate the skip/include concepts to negation or not
while reading the code, so I hope this simplifies it a bit.