From 95f20ceca2b193ff95598591baf84c76a9ce6c55 Mon Sep 17 00:00:00 2001 From: o0Ignition0o Date: Fri, 1 Sep 2023 10:41:21 +0200 Subject: [PATCH] Deal with interfaces on fragment spreads when no __typename is queried Fix #2587 Operations would over rely on the presence of __typename to resolve selection sets on interface implementers. This changeset checks for the parent type in an InlineFragment, so we don't drop relevant selection set when applicable. --- .../src/services/supergraph_service.rs | 131 ++++++++++++++++++ apollo-router/src/spec/query.rs | 23 +-- apollo-router/src/spec/query/tests.rs | 42 +++++- 3 files changed, 182 insertions(+), 14 deletions(-) diff --git a/apollo-router/src/services/supergraph_service.rs b/apollo-router/src/services/supergraph_service.rs index 7471a059b9..d7bad236b5 100644 --- a/apollo-router/src/services/supergraph_service.rs +++ b/apollo-router/src/services/supergraph_service.rs @@ -2924,4 +2924,135 @@ mod tests { insta::assert_json_snapshot!(stream.next_response().await.unwrap()); } + + #[tokio::test] + async fn no_typename_on_interface() { + let subgraphs = MockedSubgraphs([ + ("animal", MockSubgraph::builder().with_json( + serde_json::json!{{"query":"query dog__animal__0{dog{id name}}", "operationName": "dog__animal__0"}}, + serde_json::json!{{"data":{"dog":{"id":"4321","name":"Spot"}}}} + ).with_json( + serde_json::json!{{"query":"query dog__animal__0{dog{__typename id name}}", "operationName": "dog__animal__0"}}, + serde_json::json!{{"data":{"dog":{"__typename":"Dog","id":"4321","name":"Spot"}}}} + ).build()), + ].into_iter().collect()); + + let service = TestHarness::builder() + .configuration_json(serde_json::json!({"include_subgraph_errors": { "all": true } })) + .unwrap() + .schema( + r#"schema + @core(feature: "https://specs.apollo.dev/core/v0.2"), + @core(feature: "https://specs.apollo.dev/join/v0.1", for: EXECUTION) + { + query: Query + } + directive @core(as: String, feature: String!, for: core__Purpose) repeatable on SCHEMA + directive @join__field(graph: join__Graph, provides: join__FieldSet, requires: join__FieldSet) on FIELD_DEFINITION + directive @join__graph(name: String!, url: String!) on ENUM_VALUE + directive @join__owner(graph: join__Graph!) on INTERFACE | OBJECT + directive @join__type(graph: join__Graph!, key: join__FieldSet) repeatable on INTERFACE | OBJECT + + interface Animal { + id: String! + } + + type Dog implements Animal { + id: String! + name: String! + } + + type Query { + animal: Animal! @join__field(graph: ANIMAL) + dog: Dog! @join__field(graph: ANIMAL) + } + + enum core__Purpose { + """ + `EXECUTION` features provide metadata necessary to for operation execution. + """ + EXECUTION + + """ + `SECURITY` features provide metadata necessary to securely resolve fields. + """ + SECURITY + } + + scalar join__FieldSet + + enum join__Graph { + ANIMAL @join__graph(name: "animal" url: "http://localhost:8080/query") + } + "#, + ) + .extra_plugin(subgraphs) + .build_supergraph() + .await + .unwrap(); + + let request = supergraph::Request::fake_builder() + .context(defer_context()) + .query( + "query dog { + dog { + ...on Animal { + id + ...on Dog { + name + } + } + } + }", + ) + .build() + .unwrap(); + + let mut stream = service.clone().oneshot(request).await.unwrap(); + + let no_typename = stream.next_response().await.unwrap(); + + let request = supergraph::Request::fake_builder() + .context(defer_context()) + .query( + "query dog { + dog { + ...on Animal { + id + __typename + ...on Dog { + name + } + } + } + }", + ) + .build() + .unwrap(); + + let mut stream = service.oneshot(request).await.unwrap(); + + let with_typename = stream.next_response().await.unwrap(); + assert_eq!( + with_typename + .data + .clone() + .unwrap() + .get("dog") + .unwrap() + .get("name") + .unwrap(), + no_typename + .data + .clone() + .unwrap() + .get("dog") + .unwrap() + .get("name") + .unwrap(), + "{:?}\n{:?}", + with_typename, + no_typename + ); + } } diff --git a/apollo-router/src/spec/query.rs b/apollo-router/src/spec/query.rs index 10b9775310..8d44c55175 100644 --- a/apollo-router/src/spec/query.rs +++ b/apollo-router/src/spec/query.rs @@ -680,21 +680,18 @@ impl Query { let is_apply = if let Some(input_type) = input.get(TYPENAME).and_then(|val| val.as_str()) { - // check if the fragment matches the input type directly, and if not, check if the + // Only 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) input_type == type_condition.as_str() || parameters.schema.is_subtype(type_condition, input_type) } else { - // known_type = true means that from the query's shape, we know - // we should get the right type here. But in the case we get a - // __typename field and it does not match, we should not apply - // that fragment - // If the type condition is an interface and the current known type implements it known_type - .as_ref() - .map(|k| parameters.schema.is_subtype(type_condition, k)) + .as_ref() + // We have no typename, we apply the selection set if the known_type implements the type_condition + .map(|k| is_subtype_or_same(parameters, type_condition, k)) .unwrap_or_default() - || known_type.as_deref() == Some(type_condition.as_str()) + // Or if the known_type implements the parent's type_condition because we're in an inline fragment. + || is_subtype_or_same(parameters, &parent_type.name(), type_condition) }; if is_apply { @@ -1072,6 +1069,14 @@ impl Query { } } +fn is_subtype_or_same( + parameters: &FormatParameters<'_>, + parent: &String, + maybe_child: &String, +) -> bool { + parent == maybe_child || parameters.schema.is_subtype(parent, maybe_child) +} + /// Intermediate structure for arguments passed through the entire formatting struct FormatParameters<'a> { variables: &'a Object, diff --git a/apollo-router/src/spec/query/tests.rs b/apollo-router/src/spec/query/tests.rs index 6da93dc0d4..32c08139ab 100644 --- a/apollo-router/src/spec/query/tests.rs +++ b/apollo-router/src/spec/query/tests.rs @@ -18,6 +18,26 @@ macro_rules! assert_eq_and_ordered { }; } +macro_rules! assert_eq_and_ordered_json { + ($a:expr, $b:expr $(,)?) => { + assert_eq!( + $a, + $b, + "assertion failed: objects are not the same:\ + \n left: `{}`\n right: `{}`", + serde_json::to_string(&$a).unwrap(), + serde_json::to_string(&$b).unwrap() + ); + assert!( + $a.eq_and_ordered(&$b), + "assertion failed: objects are not ordered the same:\ + \n left: `{}`\n right: `{}`", + serde_json::to_string(&$a).unwrap(), + serde_json::to_string(&$b).unwrap(), + ); + }; +} + #[derive(Default)] struct FormatTest { schema: Option<&'static str>, @@ -122,15 +142,21 @@ impl FormatTest { ); if let Some(e) = self.expected { - assert_eq_and_ordered!(response.data.as_ref().unwrap(), &e); + assert_eq_and_ordered_json!( + serde_json_bytes::to_value(response.data.as_ref()).unwrap(), + e + ); } if let Some(e) = self.expected_errors { - assert_eq_and_ordered!(serde_json_bytes::to_value(&response.errors).unwrap(), e); + assert_eq_and_ordered_json!(serde_json_bytes::to_value(&response.errors).unwrap(), e); } if let Some(e) = self.expected_extensions { - assert_eq_and_ordered!(serde_json_bytes::to_value(&response.extensions).unwrap(), e); + assert_eq_and_ordered_json!( + serde_json_bytes::to_value(&response.extensions).unwrap(), + e + ); } } } @@ -496,9 +522,15 @@ fn reformat_response_data_best_effort() { "baz": "2", }, "array": [ - {}, + { + "bar":null, + "baz":"3" + }, null, - {}, + { + "bar":"5", + "baz":null + } ], "other": null, },