diff --git a/.changesets/fix_igni_typename_fragment_interfaces.md b/.changesets/fix_igni_typename_fragment_interfaces.md new file mode 100644 index 0000000000..5ed53a9ee9 --- /dev/null +++ b/.changesets/fix_igni_typename_fragment_interfaces.md @@ -0,0 +1,5 @@ +### Deal with interfaces on fragment spreads when no __typename is queried ([Issue #2587](https://github.com/apollographql/router/issues/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. + +By [@o0Ignition0o](https://github.com/o0Ignition0o) and [@geal](https://github.com/geal) in https://github.com/apollographql/router/pull/3718 diff --git a/apollo-router/src/services/snapshots/apollo_router__services__supergraph_service__tests__no_typename_on_interface-2.snap b/apollo-router/src/services/snapshots/apollo_router__services__supergraph_service__tests__no_typename_on_interface-2.snap new file mode 100644 index 0000000000..00772267f1 --- /dev/null +++ b/apollo-router/src/services/snapshots/apollo_router__services__supergraph_service__tests__no_typename_on_interface-2.snap @@ -0,0 +1,13 @@ +--- +source: apollo-router/src/services/supergraph_service.rs +expression: with_typename +--- +{ + "data": { + "dog": { + "id": "8765", + "__typename": "Dog", + "name": "Spot" + } + } +} diff --git a/apollo-router/src/services/snapshots/apollo_router__services__supergraph_service__tests__no_typename_on_interface-3.snap b/apollo-router/src/services/snapshots/apollo_router__services__supergraph_service__tests__no_typename_on_interface-3.snap new file mode 100644 index 0000000000..f385b2bdd9 --- /dev/null +++ b/apollo-router/src/services/snapshots/apollo_router__services__supergraph_service__tests__no_typename_on_interface-3.snap @@ -0,0 +1,12 @@ +--- +source: apollo-router/src/services/supergraph_service.rs +expression: with_reversed_fragments +--- +{ + "data": { + "dog": { + "name": "Spot", + "id": "0000" + } + } +} diff --git a/apollo-router/src/services/snapshots/apollo_router__services__supergraph_service__tests__no_typename_on_interface.snap b/apollo-router/src/services/snapshots/apollo_router__services__supergraph_service__tests__no_typename_on_interface.snap new file mode 100644 index 0000000000..2a443b1b15 --- /dev/null +++ b/apollo-router/src/services/snapshots/apollo_router__services__supergraph_service__tests__no_typename_on_interface.snap @@ -0,0 +1,12 @@ +--- +source: apollo-router/src/services/supergraph_service.rs +expression: no_typename +--- +{ + "data": { + "dog": { + "id": "4321", + "name": "Spot" + } + } +} diff --git a/apollo-router/src/services/supergraph_service.rs b/apollo-router/src/services/supergraph_service.rs index 7471a059b9..9f14a693a0 100644 --- a/apollo-router/src/services/supergraph_service.rs +++ b/apollo-router/src/services/supergraph_service.rs @@ -2924,4 +2924,183 @@ 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":"8765","name":"Spot"}}}} + ).with_json( + serde_json::json!{{"query":"query dog__animal__0{dog{name id}}", "operationName": "dog__animal__0"}}, + serde_json::json!{{"data":{"dog":{"id":"0000","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(); + insta::assert_json_snapshot!(no_typename); + + 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.clone().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 + ); + insta::assert_json_snapshot!(with_typename); + + let request = supergraph::Request::fake_builder() + .context(defer_context()) + .query( + "query dog { + dog { + ...on Dog { + name + ...on Animal { + id + } + } + } + }", + ) + .build() + .unwrap(); + + let mut stream = service.oneshot(request).await.unwrap(); + + let with_reversed_fragments = stream.next_response().await.unwrap(); + assert_eq!( + with_reversed_fragments + .data + .clone() + .unwrap() + .get("dog") + .unwrap() + .get("name") + .unwrap(), + no_typename + .data + .clone() + .unwrap() + .get("dog") + .unwrap() + .get("name") + .unwrap(), + "{:?}\n{:?}", + with_reversed_fragments, + no_typename + ); + insta::assert_json_snapshot!(with_reversed_fragments); + } } diff --git a/apollo-router/src/spec/query.rs b/apollo-router/src/spec/query.rs index 10b9775310..6f6b5e02d3 100644 --- a/apollo-router/src/spec/query.rs +++ b/apollo-router/src/spec/query.rs @@ -680,18 +680,14 @@ 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() + // We have no typename, we apply the selection set if the known_type implements the type_condition .map(|k| parameters.schema.is_subtype(type_condition, k)) .unwrap_or_default() || known_type.as_deref() == Some(type_condition.as_str()) diff --git a/apollo-router/src/spec/query/tests.rs b/apollo-router/src/spec/query/tests.rs index 6da93dc0d4..e3493baf6d 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 + ); } } } @@ -498,7 +524,86 @@ fn reformat_response_data_best_effort() { "array": [ {}, null, - {}, + {} + ], + "other": null, + }, + } + }) + .test(); +} + +#[test] +// just like the test above, except the query is one the planner would generate. +fn reformat_response_data_best_effort_relevant_query() { + FormatTest::builder() + .schema( + "type Query { + get: Thing + } + type Thing { + foo: String + stuff: Baz + array: [Element] + other: Bar + } + + type Baz { + bar: String + baz: String + } + + type Bar { + bar: String + } + + union Element = Baz | Bar + ", + ) + .query("{get{foo stuff{bar baz}array{...on Baz{bar baz}}other{bar}}}") + // the planner generates this: + // {get{foo stuff{bar baz}array{__typename ...on Baz{bar baz}}other{bar}}} + .response(json! { + { + "get": { + "foo": "1", + "stuff": {"baz": "2"}, + "array": [ + { + "__typename": "Baz", + "baz": "3" + }, + "4", + { + "__typename": "Baz", + "baz": "5" + }, + ], + "other": "6", + }, + "should_be_removed": { + "aaa": 2 + }, + } + }) + .expected(json! { + { + "get": { + "foo": "1", + "stuff": { + "bar": null, + "baz": "2", + }, + "array": [ + { + "bar":null, + "baz":"3" + }, + null, + { + "bar": null, + "baz":"5" + } ], "other": null, }, diff --git a/apollo-router/src/spec/schema.rs b/apollo-router/src/spec/schema.rs index b34b180f37..892883339b 100644 --- a/apollo-router/src/spec/schema.rs +++ b/apollo-router/src/spec/schema.rs @@ -160,6 +160,13 @@ impl Schema { .unwrap_or(false) } + pub(crate) fn is_interface(&self, abstract_type: &str) -> bool { + self.type_system + .definitions + .interfaces + .contains_key(abstract_type) + } + /// Return an iterator over subgraphs that yields the subgraph name and its URL. pub(crate) fn subgraphs(&self) -> impl Iterator { self.subgraphs.iter() diff --git a/apollo-router/src/spec/selection.rs b/apollo-router/src/spec/selection.rs index 34728d419b..5252a7f5d9 100644 --- a/apollo-router/src/spec/selection.rs +++ b/apollo-router/src/spec/selection.rs @@ -151,18 +151,39 @@ impl Selection { .ok_or_else(|| SpecError::InvalidType(current_type.to_string()))?; let fragment_type = FieldType::new_named(type_condition.clone()); + let known_type = current_type.inner_type_name().map(|s| s.to_string()); + + // this is the type we pass when extracting the fragment's selections + // If the type condition is a union or interface and the current type implements it, then we want + // to keep the current type when extracting the fragment's selections, as it is more precise + // than the interface. + // If it is not, then we use the type condition + let relevant_type = if schema.is_interface(type_condition.as_str()) { + // Query validation should have already verified that current type implements that interface + debug_assert!( + schema.is_subtype( + type_condition.as_str(), + current_type.inner_type_name().unwrap_or("") + ) || + // if the current type and the type condition are both the same interface, it is still valid + type_condition.as_str() + == current_type.inner_type_name().unwrap_or("") + ); + current_type + } else { + &fragment_type + }; let selection_set = inline_fragment .selection_set() .selection() .iter() .filter_map(|selection| { - Selection::from_hir(selection, &fragment_type, schema, count, defer_stats) + Selection::from_hir(selection, relevant_type, schema, count, defer_stats) .transpose() }) .collect::>()?; - let known_type = current_type.inner_type_name().map(|s| s.to_string()); Some(Self::InlineFragment { type_condition, selection_set,