From 7402a1b62fb7a08acf1f03149986771af0baf7f9 Mon Sep 17 00:00:00 2001 From: Ivan Goncharov Date: Tue, 17 Sep 2024 03:23:02 +0300 Subject: [PATCH 01/11] Fix __typename inside router response --- .../src/services/supergraph/tests.rs | 30 ++- apollo-router/src/spec/query.rs | 211 +++++++++--------- apollo-router/src/spec/selection.rs | 5 - .../tests/integration/subgraph_response.rs | 29 +++ apollo-router/tests/integration/typename.rs | 66 ++++++ 5 files changed, 229 insertions(+), 112 deletions(-) diff --git a/apollo-router/src/services/supergraph/tests.rs b/apollo-router/src/services/supergraph/tests.rs index 7a3d6765a1..afb2a534ba 100644 --- a/apollo-router/src/services/supergraph/tests.rs +++ b/apollo-router/src/services/supergraph/tests.rs @@ -1154,13 +1154,31 @@ 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 + } + "#; 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(); diff --git a/apollo-router/src/spec/query.rs b/apollo-router/src/spec/query.rs index 21f2914035..e47899e71e 100644 --- a/apollo-router/src/spec/query.rs +++ b/apollo-router/src/spec/query.rs @@ -43,6 +43,8 @@ use crate::spec::Selection; use crate::spec::SpecError; use crate::Configuration; +use super::Fragment; + pub(crate) mod change; pub(crate) mod subselections; pub(crate) mod transform; @@ -146,18 +148,6 @@ impl Query { errors: Vec::new(), nullified: Vec::new(), }; - // Detect if root __typename is asked in the original query (the qp doesn't put root __typename in subselections) - // cf https://github.com/apollographql/router/issues/1677 - let operation_kind_if_root_typename = - original_operation.and_then(|op| { - op.selection_set - .iter() - .any(|f| f.is_typename_field()) - .then(|| *op.kind()) - }); - if let Some(operation_kind) = operation_kind_if_root_typename { - output.insert(TYPENAME, operation_kind.default_type_name().into()); - } response.data = Some( match self.apply_root_selection_set( @@ -239,23 +229,23 @@ impl Query { } } Some(Value::Null) => { - // Detect if root __typename is asked in the original query (the qp doesn't put root __typename in subselections) - // cf https://github.com/apollographql/router/issues/1677 - let operation_kind_if_root_typename = original_operation.and_then(|op| { - op.selection_set - .iter() - .any(|f| f.is_typename_field()) - .then(|| *op.kind()) - }); - response.data = match operation_kind_if_root_typename { - Some(operation_kind) => { + if let Some(operation) = original_operation { + // Detect if root __typename is asked in the original query (the qp doesn't put root __typename in subselections) + // cf https://github.com/apollographql/router/issues/1677 + if self.has_only_typename_field(&operation.selection_set, &variables) { + let operation_type_name = schema + .root_operation(operation.kind.into()) + .map(|name| name.as_str()) + .unwrap_or(operation.kind.default_type_name()); + let mut output = Object::default(); - output.insert(TYPENAME, operation_kind.default_type_name().into()); - Some(output.into()) + output.insert(TYPENAME, operation_type_name.into()); + response.data = Some(output.into()); + return vec![]; } - None => Some(Value::default()), - }; + } + response.data = Some(Value::Null); return vec![]; } _ => { @@ -263,7 +253,7 @@ impl Query { } } - response.data = Some(Value::default()); + response.data = Some(Value::Null); vec![] } @@ -571,19 +561,13 @@ impl Query { .and_then(|s| apollo_compiler::ast::NamedType::new(s).ok()) .map(apollo_compiler::ast::Type::Named); - let current_type = if parameters - .schema - .get_interface(field_type.inner_named_type()) - .is_some() - || parameters - .schema - .get_union(field_type.inner_named_type()) - .is_some() - { - typename.as_ref().unwrap_or(field_type) - } else { - field_type - }; + let current_type = + match parameters.schema.types.get(field_type.inner_named_type()) { + Some(ExtendedType::Interface(..) | ExtendedType::Union(..)) => { + typename.as_ref().unwrap_or(field_type) + } + _ => field_type, + }; if self .apply_selection_set( @@ -640,21 +624,11 @@ impl Query { } if name.as_str() == TYPENAME { - let input_value = input - .get(field_name.as_str()) - .cloned() - .filter(|v| v.is_string()) - .unwrap_or_else(|| { - Value::String(ByteString::from( - current_type.inner_named_type().as_str().to_owned(), - )) - }); - if let Some(input_str) = input_value.as_str() { - if parameters.schema.get_object(input_str).is_some() { - output.insert((*field_name).clone(), input_value); - } else { - return Err(InvalidValue); - } + let typename = current_type.inner_named_type(); + if parameters.schema.get_object(typename).is_some() { + output.insert((*field_name).clone(), typename.as_str().into()); + } else { + return Err(InvalidValue); } continue; } @@ -751,11 +725,15 @@ impl Query { continue; } - if let Some(fragment) = self.fragments.get(name) { + if let Some(Fragment { + type_condition, + selection_set, + }) = self.fragments.get(name) + { let is_apply = current_type.inner_named_type().as_str() - == fragment.type_condition.as_str() + == type_condition.as_str() || parameters.schema.is_subtype( - &fragment.type_condition, + &type_condition, current_type.inner_named_type().as_str(), ); @@ -768,7 +746,7 @@ impl Query { } self.apply_selection_set( - &fragment.selection_set, + &selection_set, parameters, input, output, @@ -811,7 +789,12 @@ impl Query { let field_name = alias.as_ref().unwrap_or(name); let field_name_str = field_name.as_str(); - if let Some(input_value) = input.get_mut(field_name_str) { + + if name.as_str() == TYPENAME { + if !output.contains_key(field_name_str) { + output.insert(field_name.clone(), Value::String(root_type_name.into())); + } + } else if let Some(input_value) = input.get_mut(field_name_str) { // if there's already a value for that key in the output it means either: // - the value is a scalar and was moved into output using take(), replacing // the input value with Null @@ -837,10 +820,6 @@ impl Query { ); path.pop(); res? - } else if name.as_str() == TYPENAME { - if !output.contains_key(field_name_str) { - output.insert(field_name.clone(), Value::String(root_type_name.into())); - } } else if field_type.is_non_null() { parameters.errors.push(Error { message: format!( @@ -861,25 +840,27 @@ impl Query { include_skip, .. } => { - // top level objects will not provide a __typename field - if type_condition.as_str() != root_type_name { - return Err(InvalidValue); - } - if include_skip.should_skip(parameters.variables) { continue; } - self.apply_selection_set( - selection_set, - parameters, - input, - output, - path, - // FIXME: use `ast::Name` everywhere so fallible conversion isn’t needed - #[allow(clippy::unwrap_used)] - &FieldType::new_named(type_condition.try_into().unwrap()).0, - )?; + // check if the fragment matches the input type directly, and if not, check if the + // input type is a subtype of the fragment's type condition (interface, union) + let is_apply = (root_type_name == type_condition.as_str()) + || parameters + .schema + .is_subtype(&type_condition, root_type_name); + + if is_apply { + self.apply_root_selection_set( + root_type_name, + selection_set, + parameters, + input, + output, + path, + )?; + } } Selection::FragmentSpread { name, @@ -892,30 +873,28 @@ impl Query { continue; } - if let Some(fragment) = self.fragments.get(name) { - let is_apply = { - // check if the fragment matches the input type directly, and if not, check if the - // input type is a subtype of the fragment's type condition (interface, union) - root_type_name == fragment.type_condition.as_str() - || parameters - .schema - .is_subtype(&fragment.type_condition, root_type_name) - }; + if let Some(Fragment { + type_condition, + selection_set, + }) = self.fragments.get(name) + { + // check if the fragment matches the input type directly, and if not, check if the + // input type is a subtype of the fragment's type condition (interface, union) + let is_apply = (root_type_name == type_condition.as_str()) + || parameters + .schema + .is_subtype(&type_condition, root_type_name); - if !is_apply { - return Err(InvalidValue); + if is_apply { + self.apply_root_selection_set( + root_type_name, + &selection_set, + parameters, + input, + output, + path, + )?; } - - self.apply_selection_set( - &fragment.selection_set, - parameters, - input, - output, - path, - // FIXME: use `ast::Name` everywhere so fallible conversion isn’t needed - #[allow(clippy::unwrap_used)] - &FieldType::new_named(root_type_name.try_into().unwrap()).0, - )?; } else { // the fragment should have been already checked with the schema failfast_debug!("missing fragment named: {}", name); @@ -927,6 +906,36 @@ impl Query { Ok(()) } + fn has_only_typename_field(&self, selection_set: &Vec, variables: &Object) -> bool { + selection_set.iter().all(|s| match s { + Selection::Field { name, .. } => name.as_str() == TYPENAME, + Selection::InlineFragment { + selection_set, + include_skip, + defer, + .. + } => { + defer.eval(variables).unwrap_or(true) + || include_skip.should_skip(variables) + || self.has_only_typename_field(selection_set, variables) + } + Selection::FragmentSpread { + name, + include_skip, + defer, + .. + } => { + defer.eval(variables).unwrap_or(true) + || include_skip.should_skip(variables) + || if let Some(fragment) = self.fragments.get(name) { + self.has_only_typename_field(&fragment.selection_set, variables) + } else { + false + } + } + }) + } + /// Validate a [`Request`]'s variables against this [`Query`] using a provided [`Schema`]. #[tracing::instrument(skip_all, level = "trace")] pub(crate) fn validate_variables( diff --git a/apollo-router/src/spec/selection.rs b/apollo-router/src/spec/selection.rs index 0c0cd545b8..4b5a880946 100644 --- a/apollo-router/src/spec/selection.rs +++ b/apollo-router/src/spec/selection.rs @@ -11,7 +11,6 @@ use crate::spec::query::DeferStats; use crate::spec::FieldType; use crate::spec::Schema; use crate::spec::SpecError; -use crate::spec::TYPENAME; #[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] pub(crate) enum Selection { @@ -190,10 +189,6 @@ impl Selection { }) } - pub(crate) fn is_typename_field(&self) -> bool { - matches!(self, Selection::Field {name, ..} if name.as_str() == TYPENAME) - } - pub(crate) fn contains_error_path(&self, path: &[PathElement], fragments: &Fragments) -> bool { match (path.first(), self) { (None, _) => true, diff --git a/apollo-router/tests/integration/subgraph_response.rs b/apollo-router/tests/integration/subgraph_response.rs index 2dd8fc68d6..f2ea253511 100644 --- a/apollo-router/tests/integration/subgraph_response.rs +++ b/apollo-router/tests/integration/subgraph_response.rs @@ -9,6 +9,35 @@ include_subgraph_errors: all: true "#; +#[tokio::test(flavor = "multi_thread")] +async fn test_different_typename_on_query_root() -> Result<(), BoxError> { + let mut router = IntegrationTest::builder() + .config(CONFIG) + .responder(ResponseTemplate::new(200).set_body_json(json!({ + "data": null, + }))) + .build() + .await; + + router.start().await; + router.assert_started().await; + + async fn check_query(router: &IntegrationTest, query: &str) -> Result<(), BoxError> { + let (_trace_id, response) = router.execute_query(&json!({ "query": query })).await; + // assert_eq!(response.status(), 200); + assert_eq!( + serde_json::from_str::(&response.text().await?)?, + json!({ + "data": { "__typename": "Query" } + }) + ); + Ok(()) + } + + check_query(&router, "{ __typename }").await?; + check_query(&router, "{ ...{ __typename } }").await +} + #[tokio::test(flavor = "multi_thread")] async fn test_valid_error_locations() -> Result<(), BoxError> { let mut router = IntegrationTest::builder() diff --git a/apollo-router/tests/integration/typename.rs b/apollo-router/tests/integration/typename.rs index 4331333594..b0d1f181e6 100644 --- a/apollo-router/tests/integration/typename.rs +++ b/apollo-router/tests/integration/typename.rs @@ -71,6 +71,72 @@ async fn aliased() { "###); } +/* FIXME: should be fixed in query planner, failing with: + > value retrieval failed: empty query plan. This behavior is unexpected and we suggest opening an issue to apollographql/router with a reproduction. + +#[tokio::test] +async fn inside_inline_fragment() { + let request = Request::fake_builder() + .query("{ ... { __typename } }") + .build() + .unwrap(); + let response = make_request(request).await; + insta::assert_json_snapshot!(response, @r###" + { + "data": { + "n": "MyQuery" + } + } + "###); +} + +#[tokio::test] +async fn inside_fragment() { + let query = r#" + { ...SomeFragment } + + fragment SomeFragment on MyQuery { + __typename + } + "#; + let request = Request::fake_builder().query(query).build().unwrap(); + let response = make_request(request).await; + insta::assert_json_snapshot!(response, @r###" + { + "data": { + "n": "MyQuery" + } + } + "###); +} + +#[tokio::test] +async fn deeply_nested_inside_fragments() { + let query = r#" + { ...SomeFragment } + + fragment SomeFragment on MyQuery { + ... { + ...AnotherFragment + } + } + + fragment AnotherFragment on MyQuery { + __typename + } + "#; + let request = Request::fake_builder().query(query).build().unwrap(); + let response = make_request(request).await; + insta::assert_json_snapshot!(response, @r###" + { + "data": { + "n": "MyQuery" + } + } + "###); +} +*/ + #[tokio::test] async fn mutation() { let request = Request::fake_builder() From d7b4f9029dd93ba95b1f71c2c42fc408fe2ebb1c Mon Sep 17 00:00:00 2001 From: Ivan Goncharov Date: Tue, 17 Sep 2024 03:51:26 +0300 Subject: [PATCH 02/11] Add test for subgraph returning { data: null } --- apollo-router/src/spec/query.rs | 6 ++-- .../tests/integration/subgraph_response.rs | 28 +++++++------------ 2 files changed, 12 insertions(+), 22 deletions(-) diff --git a/apollo-router/src/spec/query.rs b/apollo-router/src/spec/query.rs index e47899e71e..38471fdbe7 100644 --- a/apollo-router/src/spec/query.rs +++ b/apollo-router/src/spec/query.rs @@ -927,11 +927,9 @@ impl Query { } => { defer.eval(variables).unwrap_or(true) || include_skip.should_skip(variables) - || if let Some(fragment) = self.fragments.get(name) { + || self.fragments.get(name).map(|fragment| { self.has_only_typename_field(&fragment.selection_set, variables) - } else { - false - } + }).unwrap_or(true) } }) } diff --git a/apollo-router/tests/integration/subgraph_response.rs b/apollo-router/tests/integration/subgraph_response.rs index f2ea253511..65b5b8c698 100644 --- a/apollo-router/tests/integration/subgraph_response.rs +++ b/apollo-router/tests/integration/subgraph_response.rs @@ -10,32 +10,24 @@ include_subgraph_errors: "#; #[tokio::test(flavor = "multi_thread")] -async fn test_different_typename_on_query_root() -> Result<(), BoxError> { +async fn test_subgraph_returning_data_null() -> Result<(), BoxError> { let mut router = IntegrationTest::builder() .config(CONFIG) - .responder(ResponseTemplate::new(200).set_body_json(json!({ - "data": null, - }))) + .responder(ResponseTemplate::new(200).set_body_json(json!({ "data": null }))) .build() .await; router.start().await; router.assert_started().await; - async fn check_query(router: &IntegrationTest, query: &str) -> Result<(), BoxError> { - let (_trace_id, response) = router.execute_query(&json!({ "query": query })).await; - // assert_eq!(response.status(), 200); - assert_eq!( - serde_json::from_str::(&response.text().await?)?, - json!({ - "data": { "__typename": "Query" } - }) - ); - Ok(()) - } - - check_query(&router, "{ __typename }").await?; - check_query(&router, "{ ...{ __typename } }").await + let query = "{ __typename topProducts { name } }"; + let (_trace_id, response) = router.execute_query(&json!({ "query": query })).await; + assert_eq!(response.status(), 200); + assert_eq!( + serde_json::from_str::(&response.text().await?)?, + json!({ "data": null }) + ); + Ok(()) } #[tokio::test(flavor = "multi_thread")] From 6de302e85907375184a887b0125fe5cf90f7d7c6 Mon Sep 17 00:00:00 2001 From: Ivan Goncharov Date: Tue, 17 Sep 2024 05:18:52 +0300 Subject: [PATCH 03/11] add test for subgraph returning different typename on query root --- apollo-router/src/spec/query.rs | 23 ++++++--- .../tests/integration/subgraph_response.rs | 51 +++++++++++++++++++ 2 files changed, 68 insertions(+), 6 deletions(-) diff --git a/apollo-router/src/spec/query.rs b/apollo-router/src/spec/query.rs index 38471fdbe7..416d3236b4 100644 --- a/apollo-router/src/spec/query.rs +++ b/apollo-router/src/spec/query.rs @@ -624,9 +624,16 @@ impl Query { } if name.as_str() == TYPENAME { - let typename = current_type.inner_named_type(); - if parameters.schema.get_object(typename).is_some() { - output.insert((*field_name).clone(), typename.as_str().into()); + let object_type = parameters + .schema + .get_object(current_type.inner_named_type()) + .or_else(|| { + let input_value = input.get(field_name.as_str())?.as_str()?; + parameters.schema.get_object(input_value) + }); + + if let Some(object_type) = object_type { + output.insert((*field_name).clone(), object_type.name.as_str().into()); } else { return Err(InvalidValue); } @@ -927,9 +934,13 @@ impl Query { } => { defer.eval(variables).unwrap_or(true) || include_skip.should_skip(variables) - || self.fragments.get(name).map(|fragment| { - self.has_only_typename_field(&fragment.selection_set, variables) - }).unwrap_or(true) + || self + .fragments + .get(name) + .map(|fragment| { + self.has_only_typename_field(&fragment.selection_set, variables) + }) + .unwrap_or(true) } }) } diff --git a/apollo-router/tests/integration/subgraph_response.rs b/apollo-router/tests/integration/subgraph_response.rs index 65b5b8c698..531553c586 100644 --- a/apollo-router/tests/integration/subgraph_response.rs +++ b/apollo-router/tests/integration/subgraph_response.rs @@ -30,6 +30,57 @@ async fn test_subgraph_returning_data_null() -> Result<(), BoxError> { Ok(()) } +#[tokio::test(flavor = "multi_thread")] +async fn test_subgraph_returning_different_typename_on_query_root() -> Result<(), BoxError> { + let mut router = IntegrationTest::builder() + .config(CONFIG) + .responder(ResponseTemplate::new(200).set_body_json(json!({ + "data": { + "topProducts": null, + "__typename": "SomeQueryRoot", + "aliased": "SomeQueryRoot", + "inside_fragment": "SomeQueryRoot", + "inside_inline_fragment": "SomeQueryRoot" + } + }))) + .build() + .await; + + router.start().await; + router.assert_started().await; + + let query = r#" + { + topProducts { name } + __typename + aliased: __typename + ...TypenameFragment + ... { + inside_inline_fragment: __typename + } + } + + fragment TypenameFragment on Query { + inside_fragment: __typename + } + "#; + let (_trace_id, response) = router.execute_query(&json!({ "query": query })).await; + assert_eq!(response.status(), 200); + assert_eq!( + serde_json::from_str::(&response.text().await?)?, + json!({ + "data": { + "topProducts": null, + "__typename": "Query", + "aliased": "Query", + "inside_fragment": "Query", + "inside_inline_fragment": "Query" + } + }) + ); + Ok(()) +} + #[tokio::test(flavor = "multi_thread")] async fn test_valid_error_locations() -> Result<(), BoxError> { let mut router = IntegrationTest::builder() From 4241a8cb2c541769b36112ef2913a1efc6d4be9d Mon Sep 17 00:00:00 2001 From: Ivan Goncharov Date: Tue, 17 Sep 2024 05:39:09 +0300 Subject: [PATCH 04/11] fix lint --- apollo-router/src/spec/query.rs | 21 +++++++------------ .../tests/integration/subgraph_response.rs | 16 +++++++------- 2 files changed, 16 insertions(+), 21 deletions(-) diff --git a/apollo-router/src/spec/query.rs b/apollo-router/src/spec/query.rs index 416d3236b4..7bf3de8063 100644 --- a/apollo-router/src/spec/query.rs +++ b/apollo-router/src/spec/query.rs @@ -21,6 +21,7 @@ use self::change::QueryHashVisitor; use self::subselections::BooleanValues; use self::subselections::SubSelectionKey; use self::subselections::SubSelectionValue; +use super::Fragment; use crate::error::FetchError; use crate::graphql::Error; use crate::graphql::Request; @@ -43,8 +44,6 @@ use crate::spec::Selection; use crate::spec::SpecError; use crate::Configuration; -use super::Fragment; - pub(crate) mod change; pub(crate) mod subselections; pub(crate) mod transform; @@ -628,7 +627,7 @@ impl Query { .schema .get_object(current_type.inner_named_type()) .or_else(|| { - let input_value = input.get(field_name.as_str())?.as_str()?; + let input_value = input.get(field_name.as_str())?.as_str()?; parameters.schema.get_object(input_value) }); @@ -740,7 +739,7 @@ impl Query { let is_apply = current_type.inner_named_type().as_str() == type_condition.as_str() || parameters.schema.is_subtype( - &type_condition, + type_condition, current_type.inner_named_type().as_str(), ); @@ -753,7 +752,7 @@ impl Query { } self.apply_selection_set( - &selection_set, + selection_set, parameters, input, output, @@ -854,9 +853,7 @@ impl Query { // check if the fragment matches the input type directly, and if not, check if the // input type is a subtype of the fragment's type condition (interface, union) let is_apply = (root_type_name == type_condition.as_str()) - || parameters - .schema - .is_subtype(&type_condition, root_type_name); + || parameters.schema.is_subtype(type_condition, root_type_name); if is_apply { self.apply_root_selection_set( @@ -888,14 +885,12 @@ impl Query { // check if the fragment matches the input type directly, and if not, check if the // input type is a subtype of the fragment's type condition (interface, union) let is_apply = (root_type_name == type_condition.as_str()) - || parameters - .schema - .is_subtype(&type_condition, root_type_name); + || parameters.schema.is_subtype(type_condition, root_type_name); if is_apply { self.apply_root_selection_set( root_type_name, - &selection_set, + selection_set, parameters, input, output, @@ -913,7 +908,7 @@ impl Query { Ok(()) } - fn has_only_typename_field(&self, selection_set: &Vec, variables: &Object) -> bool { + fn has_only_typename_field(&self, selection_set: &[Selection], variables: &Object) -> bool { selection_set.iter().all(|s| match s { Selection::Field { name, .. } => name.as_str() == TYPENAME, Selection::InlineFragment { diff --git a/apollo-router/tests/integration/subgraph_response.rs b/apollo-router/tests/integration/subgraph_response.rs index 531553c586..fabe2dcc5c 100644 --- a/apollo-router/tests/integration/subgraph_response.rs +++ b/apollo-router/tests/integration/subgraph_response.rs @@ -35,14 +35,14 @@ async fn test_subgraph_returning_different_typename_on_query_root() -> Result<() let mut router = IntegrationTest::builder() .config(CONFIG) .responder(ResponseTemplate::new(200).set_body_json(json!({ - "data": { - "topProducts": null, - "__typename": "SomeQueryRoot", - "aliased": "SomeQueryRoot", - "inside_fragment": "SomeQueryRoot", - "inside_inline_fragment": "SomeQueryRoot" - } - }))) + "data": { + "topProducts": null, + "__typename": "SomeQueryRoot", + "aliased": "SomeQueryRoot", + "inside_fragment": "SomeQueryRoot", + "inside_inline_fragment": "SomeQueryRoot" + } + }))) .build() .await; From cad52650f68ce260e7b1e829fae2ef8959d0a8ea Mon Sep 17 00:00:00 2001 From: Ivan Goncharov Date: Tue, 17 Sep 2024 09:51:40 +0300 Subject: [PATCH 05/11] correctly fix issue with __typename on primary part of defer --- apollo-router/src/query_planner/execution.rs | 2 + apollo-router/src/spec/query.rs | 48 -------------------- 2 files changed, 2 insertions(+), 48 deletions(-) diff --git a/apollo-router/src/query_planner/execution.rs b/apollo-router/src/query_planner/execution.rs index c6bd24e43e..e324d7d052 100644 --- a/apollo-router/src/query_planner/execution.rs +++ b/apollo-router/src/query_planner/execution.rs @@ -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())); } } .instrument(tracing::info_span!( diff --git a/apollo-router/src/spec/query.rs b/apollo-router/src/spec/query.rs index 7bf3de8063..60ce6a2c03 100644 --- a/apollo-router/src/spec/query.rs +++ b/apollo-router/src/spec/query.rs @@ -228,22 +228,6 @@ impl Query { } } Some(Value::Null) => { - if let Some(operation) = original_operation { - // Detect if root __typename is asked in the original query (the qp doesn't put root __typename in subselections) - // cf https://github.com/apollographql/router/issues/1677 - if self.has_only_typename_field(&operation.selection_set, &variables) { - let operation_type_name = schema - .root_operation(operation.kind.into()) - .map(|name| name.as_str()) - .unwrap_or(operation.kind.default_type_name()); - - let mut output = Object::default(); - output.insert(TYPENAME, operation_type_name.into()); - response.data = Some(output.into()); - return vec![]; - } - } - response.data = Some(Value::Null); return vec![]; } @@ -908,38 +892,6 @@ impl Query { Ok(()) } - fn has_only_typename_field(&self, selection_set: &[Selection], variables: &Object) -> bool { - selection_set.iter().all(|s| match s { - Selection::Field { name, .. } => name.as_str() == TYPENAME, - Selection::InlineFragment { - selection_set, - include_skip, - defer, - .. - } => { - defer.eval(variables).unwrap_or(true) - || include_skip.should_skip(variables) - || self.has_only_typename_field(selection_set, variables) - } - Selection::FragmentSpread { - name, - include_skip, - defer, - .. - } => { - defer.eval(variables).unwrap_or(true) - || include_skip.should_skip(variables) - || self - .fragments - .get(name) - .map(|fragment| { - self.has_only_typename_field(&fragment.selection_set, variables) - }) - .unwrap_or(true) - } - }) - } - /// Validate a [`Request`]'s variables against this [`Query`] using a provided [`Schema`]. #[tracing::instrument(skip_all, level = "trace")] pub(crate) fn validate_variables( From 0d61767f838747e5d2413da34e656fbd84208edc Mon Sep 17 00:00:00 2001 From: Ivan Goncharov Date: Tue, 17 Sep 2024 17:24:17 +0300 Subject: [PATCH 06/11] add changeset --- .changesets/fix_fix_typename.md | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 .changesets/fix_fix_typename.md diff --git a/.changesets/fix_fix_typename.md b/.changesets/fix_fix_typename.md new file mode 100644 index 0000000000..7dc3b24d1a --- /dev/null +++ b/.changesets/fix_fix_typename.md @@ -0,0 +1,22 @@ +### Fix variout 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 \ No newline at end of file From b411e5f896ac7519a85290aa6eeb604054b2b8b3 Mon Sep 17 00:00:00 2001 From: Ivan Goncharov Date: Thu, 19 Sep 2024 02:30:58 +0300 Subject: [PATCH 07/11] Add insta snapshot --- .../requires/include_skip.rs | 100 +++++++++--------- .../src/services/supergraph/tests.rs | 76 +++++++------ 2 files changed, 96 insertions(+), 80 deletions(-) diff --git a/apollo-federation/tests/query_plan/build_query_plan_tests/requires/include_skip.rs b/apollo-federation/tests/query_plan/build_query_plan_tests/requires/include_skip.rs index ebb98c6653..2c0d57f05e 100644 --- a/apollo-federation/tests/query_plan/build_query_plan_tests/requires/include_skip.rs +++ b/apollo-federation/tests/query_plan/build_query_plan_tests/requires/include_skip.rs @@ -178,64 +178,66 @@ fn it_handles_an_at_requires_where_multiple_conditional_are_involved() { } "#, @r###" - QueryPlan { - Include(if: $test1) { - Sequence { - Fetch(service: "Subgraph1") { + QueryPlan { + Include(if: $test1) { + Sequence { + Fetch(service: "Subgraph1") { + { + a { + __typename + idA + } + } + }, + Include(if: $test2) { + Sequence { + Flatten(path: "a") { + Fetch(service: "Subgraph2") { { - a { + ... on A { __typename idA } + } => + { + ... on A { + b { + __typename + idB + ... on B { + required + } + } + } } }, - Include(if: $test2) { - Sequence { - Flatten(path: "a") { - Fetch(service: "Subgraph2") { - { - ... on A { - __typename - idA - } - } => - { - ... on A { - b { - __typename - idB - required - } - } - } - }, - }, - Flatten(path: "a.b.@") { - Fetch(service: "Subgraph3") { - { - ... on B { - ... on B { - __typename - idB - required - } - } - } => - { - ... on B { - ... on B { - c - } - } - } - }, - }, + }, + Flatten(path: "a.b.@") { + Fetch(service: "Subgraph3") { + { + ... on B { + ... on B { + __typename + idB + required + } + } + } => + { + ... on B { + ... on B { + c + } + } } }, - } + }, }, - } - "### + }, + }, + }, + } + "### ); } diff --git a/apollo-router/src/services/supergraph/tests.rs b/apollo-router/src/services/supergraph/tests.rs index afb2a534ba..f72b36981c 100644 --- a/apollo-router/src/services/supergraph/tests.rs +++ b/apollo-router/src/services/supergraph/tests.rs @@ -1101,12 +1101,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"}] } @@ -1115,33 +1115,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()); @@ -1182,13 +1160,49 @@ async fn root_typename_with_defer_and_empty_first_response() { 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] From ddeed8d51df2c9c0edf19eb8fbb5530d31152295 Mon Sep 17 00:00:00 2001 From: Ivan Goncharov Date: Thu, 19 Sep 2024 02:36:14 +0300 Subject: [PATCH 08/11] switch to shorter json snapshot code All credits goes to @Geal --- .../tests/integration/subgraph_response.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/apollo-router/tests/integration/subgraph_response.rs b/apollo-router/tests/integration/subgraph_response.rs index fabe2dcc5c..52fc56fa27 100644 --- a/apollo-router/tests/integration/subgraph_response.rs +++ b/apollo-router/tests/integration/subgraph_response.rs @@ -24,7 +24,7 @@ async fn test_subgraph_returning_data_null() -> Result<(), BoxError> { let (_trace_id, response) = router.execute_query(&json!({ "query": query })).await; assert_eq!(response.status(), 200); assert_eq!( - serde_json::from_str::(&response.text().await?)?, + response.json::().await?, json!({ "data": null }) ); Ok(()) @@ -67,7 +67,7 @@ async fn test_subgraph_returning_different_typename_on_query_root() -> Result<() let (_trace_id, response) = router.execute_query(&json!({ "query": query })).await; assert_eq!(response.status(), 200); assert_eq!( - serde_json::from_str::(&response.text().await?)?, + response.json::().await?, json!({ "data": { "topProducts": null, @@ -107,7 +107,7 @@ async fn test_valid_error_locations() -> Result<(), BoxError> { .await; assert_eq!(response.status(), 200); assert_eq!( - serde_json::from_str::(&response.text().await?)?, + response.json::().await?, json!({ "data": { "topProducts": null }, "errors": [{ @@ -148,7 +148,7 @@ async fn test_empty_error_locations() -> Result<(), BoxError> { .await; assert_eq!(response.status(), 200); assert_eq!( - serde_json::from_str::(&response.text().await?)?, + response.json::().await?, json!({ "data": { "topProducts": null }, "errors": [{ @@ -185,7 +185,7 @@ async fn test_invalid_error_locations() -> Result<(), BoxError> { .await; assert_eq!(response.status(), 200); assert_eq!( - serde_json::from_str::(&response.text().await?)?, + response.json::().await?, json!({ "data": null, "errors": [{ @@ -227,7 +227,7 @@ async fn test_invalid_error_locations_with_single_negative_one_location() -> Res .await; assert_eq!(response.status(), 200); assert_eq!( - serde_json::from_str::(&response.text().await?)?, + response.json::().await?, json!({ "data": { "topProducts": null }, "errors": [{ @@ -268,7 +268,7 @@ async fn test_invalid_error_locations_contains_negative_one_location() -> Result .await; assert_eq!(response.status(), 200); assert_eq!( - serde_json::from_str::(&response.text().await?)?, + response.json::().await?, json!({ "data": { "topProducts": null }, "errors": [{ From f6377052cb8b7b73220f630c8901ea9912299850 Mon Sep 17 00:00:00 2001 From: Ivan Goncharov Date: Thu, 19 Sep 2024 15:19:59 +0300 Subject: [PATCH 09/11] fixed unintentional change in fed snapshot --- .../requires/include_skip.rs | 100 +++++++++--------- 1 file changed, 49 insertions(+), 51 deletions(-) diff --git a/apollo-federation/tests/query_plan/build_query_plan_tests/requires/include_skip.rs b/apollo-federation/tests/query_plan/build_query_plan_tests/requires/include_skip.rs index 2c0d57f05e..ebb98c6653 100644 --- a/apollo-federation/tests/query_plan/build_query_plan_tests/requires/include_skip.rs +++ b/apollo-federation/tests/query_plan/build_query_plan_tests/requires/include_skip.rs @@ -178,66 +178,64 @@ fn it_handles_an_at_requires_where_multiple_conditional_are_involved() { } "#, @r###" - QueryPlan { - Include(if: $test1) { - Sequence { - Fetch(service: "Subgraph1") { - { - a { - __typename - idA - } - } - }, - Include(if: $test2) { - Sequence { - Flatten(path: "a") { - Fetch(service: "Subgraph2") { + QueryPlan { + Include(if: $test1) { + Sequence { + Fetch(service: "Subgraph1") { { - ... on A { + a { __typename idA } - } => - { - ... on A { - b { - __typename - idB - ... on B { - required - } - } - } } }, - }, - Flatten(path: "a.b.@") { - Fetch(service: "Subgraph3") { - { - ... on B { - ... on B { - __typename - idB - required - } - } - } => - { - ... on B { - ... on B { - c - } - } + Include(if: $test2) { + Sequence { + Flatten(path: "a") { + Fetch(service: "Subgraph2") { + { + ... on A { + __typename + idA + } + } => + { + ... on A { + b { + __typename + idB + required + } + } + } + }, + }, + Flatten(path: "a.b.@") { + Fetch(service: "Subgraph3") { + { + ... on B { + ... on B { + __typename + idB + required + } + } + } => + { + ... on B { + ... on B { + c + } + } + } + }, + }, } }, - }, + } }, - }, - }, - }, - } - "### + } + "### ); } From 95cfb79785a73cb696470325625a281924491d95 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Tue, 8 Oct 2024 15:04:28 +0200 Subject: [PATCH 10/11] Typo fix Co-authored-by: Jesse Rosenberger --- .changesets/fix_fix_typename.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changesets/fix_fix_typename.md b/.changesets/fix_fix_typename.md index 7dc3b24d1a..56ee8aace7 100644 --- a/.changesets/fix_fix_typename.md +++ b/.changesets/fix_fix_typename.md @@ -1,4 +1,4 @@ -### Fix variout edge cases for `__typename` field ([PR #6009](https://github.com/apollographql/router/pull/6009)) +### 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. From 37c5b1edcbc350da421375734ff07ad2b95d164b Mon Sep 17 00:00:00 2001 From: Ivan Goncharov Date: Tue, 15 Oct 2024 19:34:34 +0300 Subject: [PATCH 11/11] address review feedback --- apollo-router/tests/integration/typename.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/apollo-router/tests/integration/typename.rs b/apollo-router/tests/integration/typename.rs index a99833f665..782e90adb6 100644 --- a/apollo-router/tests/integration/typename.rs +++ b/apollo-router/tests/integration/typename.rs @@ -106,10 +106,11 @@ async fn aliased() { "###); } -/* FIXME: should be fixed in query planner, failing with: - > value retrieval failed: empty query plan. This behavior is unexpected and we suggest opening an issue to apollographql/router with a reproduction. - +// FIXME: bellow test panic because of bug in query planner, failing with: +// "value retrieval failed: empty query plan. This behavior is unexpected and we suggest opening an issue to apollographql/router with a reproduction." +// See: https://github.com/apollographql/router/issues/6154 #[tokio::test] +#[should_panic] async fn inside_inline_fragment() { let request = Request::fake_builder() .query("{ ... { __typename } }") @@ -126,6 +127,7 @@ async fn inside_inline_fragment() { } #[tokio::test] +#[should_panic] // See above FIXME async fn inside_fragment() { let query = r#" { ...SomeFragment } @@ -146,6 +148,7 @@ async fn inside_fragment() { } #[tokio::test] +#[should_panic] // See above FIXME async fn deeply_nested_inside_fragments() { let query = r#" { ...SomeFragment } @@ -170,7 +173,6 @@ async fn deeply_nested_inside_fragments() { } "###); } -*/ #[tokio::test] async fn mutation() {