Skip to content
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 18 commits into from
Oct 16, 2024
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions .changesets/fix_fix_typename.md
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
2 changes: 2 additions & 0 deletions apollo-router/src/query_planner/execution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,8 @@ impl PlanNode {
let _ = primary_sender.send((value.clone(), errors.clone()));
} else {
let _ = primary_sender.send((value.clone(), errors.clone()));
// primary response should be an empty object
value.deep_merge(Value::Object(Default::default()));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

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:

&initial_value.unwrap_or_default(),

to set it to an object instead of Value::Null by default

Copy link
Member Author

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?

Copy link
Contributor

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

Copy link
Member Author

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.

Copy link
Contributor

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

}
}
.instrument(tracing::info_span!(
Expand Down
106 changes: 69 additions & 37 deletions apollo-router/src/services/supergraph/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1103,12 +1103,12 @@ async fn subscription_without_header() {
async fn root_typename_with_defer_and_empty_first_response() {
let subgraphs = MockedSubgraphs([
("user", MockSubgraph::builder().with_json(
serde_json::json!{{"query":"{currentUser{activeOrganization{__typename id}}}"}},
serde_json::json!{{"query":"{... on Query{currentUser{activeOrganization{__typename id}}}}"}},
serde_json::json!{{"data": {"currentUser": { "activeOrganization": { "__typename": "Organization", "id": "0" } }}}}
).build()),
("orga", MockSubgraph::builder().with_json(
serde_json::json!{{
"query":"query($representations:[_Any!]!){_entities(representations:$representations){...on Organization{suborga{__typename id}}}}",
"query":"query($representations:[_Any!]!){_entities(representations:$representations){...on Organization{suborga{id name}}}}",
"variables": {
"representations":[{"__typename": "Organization", "id":"0"}]
}
Expand All @@ -1117,33 +1117,11 @@ async fn root_typename_with_defer_and_empty_first_response() {
"data": {
"_entities": [{ "suborga": [
{ "__typename": "Organization", "id": "1"},
{ "__typename": "Organization", "id": "2"},
{ "__typename": "Organization", "id": "2", "name": "A"},
{ "__typename": "Organization", "id": "3"},
] }]
},
}}
)
.with_json(
serde_json::json!{{
"query":"query($representations:[_Any!]!){_entities(representations:$representations){...on Organization{name}}}",
"variables": {
"representations":[
{"__typename": "Organization", "id":"1"},
{"__typename": "Organization", "id":"2"},
{"__typename": "Organization", "id":"3"}

]
}
}},
serde_json::json!{{
"data": {
"_entities": [
{ "__typename": "Organization", "id": "1"},
{ "__typename": "Organization", "id": "2", "name": "A"},
{ "__typename": "Organization", "id": "3"},
]
}
}}
}}
).build())
].into_iter().collect());

Expand All @@ -1156,23 +1134,77 @@ async fn root_typename_with_defer_and_empty_first_response() {
.await
.unwrap();

let query = r#"
query {
...OnlyTypename
... @defer {
currentUser {
activeOrganization {
id
suborga {
id
name
}
}
}
}
}

fragment OnlyTypename on Query {
__typename
}
IvanGoncharov marked this conversation as resolved.
Show resolved Hide resolved
"#;
let request = supergraph::Request::fake_builder()
.context(defer_context())
.query(
"query { __typename ... @defer { currentUser { activeOrganization { id suborga { id name } } } } }",
)
.build()
.unwrap();
.context(defer_context())
.query(query)
.build()
.unwrap();

let mut stream = service.oneshot(request).await.unwrap();
let res = stream.next_response().await.unwrap();
assert_eq!(
res.data.as_ref().unwrap().get("__typename"),
Some(&serde_json_bytes::Value::String("Query".into()))
);

insta::assert_json_snapshot!(res, @r###"
{
"data": {
"__typename": "Query"
},
"hasNext": true
}
"###);

// Must have 2 chunks
let _ = stream.next_response().await.unwrap();
let res = stream.next_response().await.unwrap();
insta::assert_json_snapshot!(res, @r###"
{
"hasNext": false,
"incremental": [
{
"data": {
"currentUser": {
"activeOrganization": {
"id": "0",
"suborga": [
{
"id": "1",
"name": null
},
{
"id": "2",
"name": "A"
},
{
"id": "3",
"name": null
}
]
}
}
},
"path": []
}
]
}
"###);
}

#[tokio::test]
Expand Down
Loading
Loading