Deal with interfaces on fragment spreads when no __typename is queried (

#3718)

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.

.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

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"
+    }
+  }
+}

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"
+    }
+  }
+}

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"
+    }
+  }
+}

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

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())

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