-
Notifications
You must be signed in to change notification settings - Fork 273
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
Fix __typename inside router response #6009
Merged
Merged
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
7402a1b
Fix __typename inside router response
IvanGoncharov d7b4f90
Add test for subgraph returning { data: null }
IvanGoncharov 6de302e
add test for subgraph returning different typename on query root
IvanGoncharov 4241a8c
fix lint
IvanGoncharov cad5265
correctly fix issue with __typename on primary part of defer
IvanGoncharov c052392
Merge branch 'dev' into i1g/fix-typename
IvanGoncharov 0d61767
add changeset
IvanGoncharov 9115847
Merge branch 'dev' into i1g/fix-typename
IvanGoncharov ea7ded9
Merge branch 'dev' into i1g/fix-typename
IvanGoncharov b411e5f
Add insta snapshot
IvanGoncharov ddeed8d
switch to shorter json snapshot code
IvanGoncharov e871c93
Merge branch 'dev' into i1g/fix-typename
IvanGoncharov f637705
fixed unintentional change in fed snapshot
IvanGoncharov 95cfb79
Typo fix
SimonSapin f15535e
Merge branch 'dev' into i1g/fix-typename
SimonSapin b9d3f77
Merge branch 'dev' into i1g/fix-typename
IvanGoncharov 37c5b1e
address review feedback
IvanGoncharov 39a9fdc
Merge branch 'dev' into i1g/fix-typename
IvanGoncharov File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
### Fix various edge cases for `__typename` field ([PR #6009](https://github.com/apollographql/router/pull/6009)) | ||
|
||
The router now correctly handles the `__typename` field used on operation root types, even when the subgraph's root type has a name that differs from the supergraph's root type. | ||
|
||
For example, in query like this: | ||
```graphql | ||
{ | ||
...RootFragment | ||
} | ||
|
||
fragment RootFragment on Query { | ||
__typename | ||
me { | ||
name | ||
} | ||
} | ||
``` | ||
Even if the subgraph's root type returns a `__typename` that differs from `Query`, the router will still use `Query` as the value of the `__typename` field. | ||
|
||
This change also includes fixes for other edge cases related to the handling of `__typename` fields. For a detailed technical description of the edge cases that were fixed, please see [this description](https://github.com/apollographql/router/pull/6009#issue-2529717207). | ||
|
||
By [@IvanGoncharov](https://github.com/IvanGoncharov) in https://github.com/apollographql/router/pull/6009 |
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
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.
here is a test case from graphql-js:
https://github.com/graphql/graphql-js/blob/2b42a70191243d0ca0e0e4f1d580d6794718fbd5/src/execution/__tests__/defer-test.ts#L371-L384
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.
if the
data
field should ve an enpty object (unless it contained a non nullable field that was nullified), then I think this should be fixed here:router/apollo-router/src/query_planner/execution.rs
Line 80 in 3436380
to set it to an object instead of
Value::Null
by defaultThere 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.
You right, it should be correct.
I hesitated to do it since I'm less familiar with how QP executes and also I don't have capacity to test all possible scenarios in context of this PR.
So I choose to do it just inside this specific branch that I'm 100% sure is correct.
@Geal Do you think it safe to make this change for entire QP execution?
Or I can create issue for that I do it in separate PR?
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.
maybe in a separate PR, considering the timing
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.
I created a router issue for it.
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.
I agree that changing the linked line of
execution.rs
is a better fix, but it doesn’t have to be in this PR