-
Notifications
You must be signed in to change notification settings - Fork 272
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ability for router to deal with query plans with contextual rewrites #5097
Conversation
…ariable. Use the first one we find
CI performance tests
|
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work on this and thanks for adding the extra tests and cleanup.
I have one comment about alpha federation release, which I don't think we've done before in the router, but apart from that I'm happy.
apollo-router/Cargo.toml
Outdated
@@ -188,7 +188,7 @@ regex = "1.10.3" | |||
reqwest.workspace = true | |||
|
|||
# note: this dependency should _always_ be pinned, prefix the version with an `=` | |||
router-bridge = "=0.5.21+v2.7.5" | |||
router-bridge = "=0.5.23+v2.8.0-alpha.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we happy releasing a router which is using a federation alpha release? Is the intent to update this before we release the next version of the router?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. This is just to let us bake in a bit more testing before we officially release 2.8.0, but if all goes well we can just promote it. We'll certainly release and bump the pin before we cut a 1.48.0 rc
…nt CI to run out of resources while running them.
…tes (#5097) Two main things that we're doing in this PR. 1. We've added a variable to FetchNode called `context_rewrites`. This is a vector of DataRewrite::KeyRenamer that are specifically taking data from their path (which will be relative and can traverse up the data path) and writes the data into an argument that is passed to the selection set. 2. There are two cases. In the most straightforward, the data that is passed to the selection set is the same for every entity. This case is pretty easy and doesn't require any special handling. In the second case, the value of the variable may be different per entity. If that is true, we need to use aliasing and duplication in our query in order to send it to subgraphs. Once graphql/composite-schemas-spec#25 is decided and has subgraph support, this query cloning will be able to go away. Co-authored-by: o0Ignition0o <[email protected]> Co-authored-by: Gary Pennington <[email protected]>
Two main things that we're doing in this PR.
context_rewrites
. This is a vector of DataRewrite::KeyRenamer that are specifically taking data from their path (which will be relative and can traverse up the data path) and writes the data into an argument that is passed to the selection set.Checklist
Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.
Exceptions
Note any exceptions here
Notes
Footnotes
It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. ↩
Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩
Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩