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.

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
+        );
+    }
 }

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,

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,
                 },