feat(federation): add @context support
#6310
Merged
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
…ent non-GraphQL-compliant JSON (#6288)
Co-authored-by: TylerBloom <tylerbloom2222@gmail.com>
#6223) Co-authored-by: Chris Lenfest <clenfest@apollographql.com>
Co-authored-by: Tyler Bloom <tylerbloom2222@gmail.com>
Co-authored-by: Tyler Bloom <tylerbloom2222@gmail.com>
In order to support `@context` during fetch dependency graph processing, this PR refactors `handle_requires()` into two functions: `handle_conditions_tree()` and `create_post_requires_node()` (the first of which gets reused by `@context` logic).
… and miscellaneous bugfixes (#6276) This PR: 1. Updates `compute_nodes_for_op_path_element()` to handle `@context`/`@fromContext` on fields, using the refactored `handleRequires()` functions created during #6263. 2. Updates `to_plan_node()` to add context variables and usages to the query plan fetch nodes. 3. Fixes a `todo()` in `resolve_condition_plan()` where the `context_map` should be `None`. 4. Changes various typings introduced during the `@context`/`@fromContext` work to be more specific, usually replacing `String` with `Name` or `FetchDataPathElement`. 5. Introduces `FetchDataPathElement::Parent` for specifically representing the "goto parent" element in fetch paths (`".."` is not a valid GraphQL name, and we shouldn't be using it in `FetchDataPathElement::Key`). - I did not change this behavior in router's query plan execution, which still currently uses a key with name `".."` (they're also using `String`s instead of `Name`s for keys, so this is a little less bad). 6. Switches `HashSet` usages to `IndexSet`, as `HashSet`/`HashMap` in the past have shown non-determinism during tests. (We were also seeing non-determinism during debugging.) 7. Switches `context_rewrites` in `FetchNode` to use `Vec<Arc<FetchDataRewrite>>` instead of `Vec<FetchDataRewrite>`, as this fixes a router compilation issue I was having with `convert.rs`. - This is mainly meant to quickly unblock, if folks want to de-`Arc` more I'm fine with us updating `convert.rs` later. 8. Switches `context_inputs` in `FetchDependencyGraphNode` to use `Vec<FetchDataKeyRenamer>` instead of `Vec<FetchDataRewrite>`. - We currently do not use value setters at all for `@context`/`@fromContext`, and allowing the extra variability for that field doesn't make sense (it's similar to the issue we have today with fragment spreads in selection sets, but we have an easy solution here). If we end up using value setters one day, I'm fine with changing it then. - If folks want to use `FetchDataRewrite` instead of `FetchDataKeyRenamer` in `FetchNode` because it simplifies `FetchNode` or router code (or if we want to effectively reserve the ability to use value setters there in the future), I'm more okay with the change to `FetchNode`. 9. Fixes various Rust-only bugs: - `FetchDependencyGraphNode::copy_inputs()` wasn't eliminating duplicate context renamers. - `FetchDependencyGraphNode::contains()` was using `==` instead of `<` for comparing used contexts. - Used contexts in `FetchDependencyGraphNode` should reference the full type instead of just the type position. 10. Fixes a bug present in both JS and Rust (and will need backporting to JS): - When a post-`@context` node doesn't need to be created, we weren't creating parent-child relationships with the condition nodes.
Co-authored-by: TylerBloom <tylerbloom2222@gmail.com>
…king whether we can rebase (#6289)
…oups` (#6302) Added `extra_conditions` argument to to `ConditionResolverCache` which was causing error in a snapshot test.
The native QP now supports it 🎉
@context support
lrlna
reviewed
Nov 21, 2024
There was a problem hiding this comment.
@goto-bus-stop let's run some perf tests and pay attention to these two things in:
- rebase
- edge conditions in compute_nodes_for_op_path_element
lrlna
reviewed
Nov 21, 2024
lrlna
approved these changes
Nov 21, 2024
Merged
IvanGoncharov
added a commit
that referenced
this pull request
Nov 30, 2024
`dev` wasn't merge in #6205 before it was squashed and committed. Because of that `dev` had some changes in tests that didn't change hash produced by old query hash agorithm but resulted in a hash change once #6205 was merged. Example: #6310 Added test changes related to @context and #6205 added a special case to handle @context directives
6 tasks
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.
Brings Rust query planner to parity with JS.