fix: __typename when formatting response in some @interfaceObject…

… cases

The introduction of `@interfaceObject` in federation 2.3 has for
consequence that a subgraph (that uses an `@interfaceObject`) may return
in its response a `__typename` that corresponds to an interface type in
the supergraph (but which, locally to that subgraph, is an object type).

The idea is that if that `__typename` is requested, then the query
planner will ensure that another follow-up fetch will override that
value so it maps to a proper object type.

However, in some cases, that `__typename` is not queried, nor is there
another reason to resolve the actual underlying implementation type of
the underlying object (meaning that only fields of the interface are
queried), and in that case the `__typename` may still point to the
interface type when `format_response` is called. Unfortunately, that
method currently nullify the whole object in such case, which is not the
behaviour we want and was not caught in #2489.

.changesets/fix_tidy_cherries_fly.md

@@ -0,0 +1,5 @@
+### Some response objects gets incorrectly set to `null` in some cases introduced by `@interfaceObject`
+
+The federation 2.3 `@interfaceObject` feature imply that an interface type in the supergraph may be locally handled as an object type by some specific subgraphs. Such subgraph may thus return objects whose `__typename` is the interface type in their response. In some cases, those `__typename` were leading the router to unexpectedly nullify the underlying objects and this was not caught in the initial integration of federation 2.3.
+
+By [@pcmanus](https://github.com/pcmanus) in https://github.com/apollographql/router/pull/2530

apollo-router/src/services/supergraph_service.rs

@@ -1678,6 +1678,145 @@ mod tests {
         );
     }
 
+    #[tokio::test]
+    async fn interface_object_response_processing() {
+        let schema = r#"
+          schema
+            @link(url: "https://specs.apollo.dev/link/v1.0")
+            @link(url: "https://specs.apollo.dev/join/v0.3", for: EXECUTION)
+          {
+            query: Query
+          }
+
+          directive @join__enumValue(graph: join__Graph!) repeatable on ENUM_VALUE
+          directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION
+          directive @join__graph(name: String!, url: String!) on ENUM_VALUE
+          directive @join__implements(graph: join__Graph!, interface: String!) repeatable on OBJECT | INTERFACE
+          directive @join__type(graph: join__Graph!, key: join__FieldSet, extension: Boolean! = false, resolvable: Boolean! = true, isInterfaceObject: Boolean! = false) repeatable on OBJECT | INTERFACE | UNION | ENUM | INPUT_OBJECT | SCALAR
+          directive @join__unionMember(graph: join__Graph!, member: String!) repeatable on UNION
+          directive @link(url: String, as: String, for: link__Purpose, import: [link__Import]) repeatable on SCHEMA
+
+          type Book implements Product
+            @join__implements(graph: PRODUCTS, interface: "Product")
+            @join__type(graph: PRODUCTS, key: "id")
+          {
+            id: ID!
+            description: String
+            price: Float
+            pages: Int
+            reviews: [Review!]! @join__field
+          }
+
+          scalar join__FieldSet
+
+          enum join__Graph {
+            PRODUCTS @join__graph(name: "products", url: "products")
+            REVIEWS @join__graph(name: "reviews", url: "reviews")
+          }
+
+          scalar link__Import
+
+          enum link__Purpose {
+            SECURITY
+            EXECUTION
+          }
+
+          type Movie implements Product
+            @join__implements(graph: PRODUCTS, interface: "Product")
+            @join__type(graph: PRODUCTS, key: "id")
+          {
+            id: ID!
+            description: String
+            price: Float
+            duration: Int
+            reviews: [Review!]! @join__field
+          }
+
+          interface Product
+            @join__type(graph: PRODUCTS, key: "id")
+            @join__type(graph: REVIEWS, key: "id", isInterfaceObject: true)
+          {
+            id: ID!
+            description: String @join__field(graph: PRODUCTS)
+            price: Float @join__field(graph: PRODUCTS)
+            reviews: [Review!]! @join__field(graph: REVIEWS)
+          }
+
+          type Query
+            @join__type(graph: PRODUCTS)
+            @join__type(graph: REVIEWS)
+          {
+            products: [Product!]! @join__field(graph: PRODUCTS)
+            allReviewedProducts: [Product!]! @join__field(graph: REVIEWS)
+            bestRatedProducts(limit: Int): [Product!]! @join__field(graph: REVIEWS)
+          }
+
+          type Review
+            @join__type(graph: REVIEWS)
+          {
+            author: String
+            text: String
+            rating: Int
+          }
+        "#;
+
+        let query = r#"
+          {
+            allReviewedProducts {
+              id
+              price
+            }
+          }
+        "#;
+
+        let subgraphs = MockedSubgraphs([
+            ("products", MockSubgraph::builder()
+                .with_json(
+                    serde_json::json! {{
+                        "query": "query($representations:[_Any!]!){_entities(representations:$representations){...on Product{price}}}",
+                        "variables": {"representations":[{"__typename":"Product","id":"1"},{"__typename":"Product","id":"2"}]}
+                    }},
+                    serde_json::json! {{
+                        "data": {"_entities":[{"price":12.99},{"price":14.99}]}
+                    }},
+                )
+                .build()),
+            ("reviews", MockSubgraph::builder()
+                .with_json(
+                    serde_json::json! {{
+                        "query": "{allReviewedProducts{__typename id}}"
+                    }},
+                    serde_json::json! {{
+                        "data": {"allReviewedProducts":[{"__typename":"Product","id":"1"},{"__typename":"Product","id":"2"}]}
+                    }},
+                )
+                .build()),
+        ].into_iter().collect());
+
+        let service = TestHarness::builder()
+            .configuration_json(serde_json::json!({"include_subgraph_errors": { "all": true } }))
+            .unwrap()
+            .schema(schema)
+            .extra_plugin(subgraphs)
+            .build_supergraph()
+            .await
+            .unwrap();
+
+        let request = supergraph::Request::fake_builder()
+            .query(query)
+            .build()
+            .unwrap();
+
+        let mut stream = service.oneshot(request).await.unwrap();
+        let response = stream.next_response().await.unwrap();
+        println!("{response:?}");
+
+        assert_eq!(
+            serde_json::to_value(&response.data).unwrap(),
+            serde_json::json!({ "allReviewedProducts": [ {"id": "1", "price": 12.99}, {"id": "2", "price": 14.99} ]}),
+        );
+    }
+
     #[tokio::test]
     async fn only_query_interface_object_subgraph() {
         // This test has 2 subgraphs, one with an interface and another with that interface

apollo-router/src/spec/query.rs

@@ -540,7 +540,13 @@ impl Query {
                         if let Some(input_type) =
                             input_object.get(TYPENAME).and_then(|val| val.as_str())
                         {
-                            if !parameters.schema.object_types.contains_key(input_type) {
+                            // If there is a __typename, make sure the pointed type is a valid type of the schema. Otherwise, something is wrong, and in case we might
+                            // be inadvertently leaking some data for an @inacessible type or something, nullify the whole object. However, do note that due to `@interfaceObject`,
+                            // some subgraph can have returned a __typename that is the name of an interface in the supergraph, and this is fine (that is, we should not
+                            // return such a __typename to the user, but as long as it's not returned, having it in the internal data is ok and sometimes expected).
+                            if !parameters.schema.object_types.contains_key(input_type)
+                                && !parameters.schema.interfaces.contains_key(input_type)
+                            {
                                 parameters.nullified.push(path.clone());
                                 *output = Value::Null;
                                 return Ok(());
@@ -677,8 +683,12 @@ impl Query {
                         let output_value =
                             output.entry((*field_name).clone()).or_insert(Value::Null);
                         if name.as_str() == TYPENAME {
-                            if input_value.is_string() {
-                                *output_value = input_value.clone();
+                            if let Some(input_str) = input_value.as_str() {
+                                if parameters.schema.object_types.contains_key(input_str) {
+                                    *output_value = input_value.clone();
+                                } else {
+                                    return Err(InvalidValue);
+                                }
                             }
                         } else {
                             path.push(PathElement::Key(field_name.as_str().to_string()));

apollo-router/src/spec/query/tests.rs

@@ -1014,6 +1014,116 @@ fn reformat_response_query_with_root_typename() {
         .test();
 }
 
+#[test]
+fn reformat_response_interface_typename_not_queried() {
+    // With the introduction of @interfaceObject, a subgraph can send back a typename that
+    // correspond to an interface in the supergraph. As long as that typename is not requested,
+    // we want this to be fine and to prevent formatting of the response.
+    FormatTest::builder()
+        .schema(
+            "type Query {
+                i: I
+            }
+            interface I {
+                x: String
+            }
+            type A implements I {
+                x: String
+            }",
+        )
+        .query("{i{x}}")
+        .response(json! {{
+            "i": {
+                "__typename": "I",
+                "x": "foo",
+            }
+        }})
+        .expected(json! {{
+            "i": {
+                "x": "foo",
+            },
+        }})
+        .test();
+}
+
+#[test]
+fn reformat_response_interface_typename_queried() {
+    // As mentioned in the previous test, the introduction of @interfaceObject makes it possible
+    // for a subgraph to send back a typename that correspond to an interface in the supergraph.
+    // If that typename is queried, then the query planner will ensure that such typename is
+    // replaced (overriden to a proper object type of the supergraph by a followup fetch). But
+    // as that later fetch can fail, we can have to format a response where the typename is
+    // requested and is still set to the interface. We must not return that value (it's invalid
+    // graphQL) and instead nullify the response in that case.
+    FormatTest::builder()
+        .schema(
+            "type Query {
+                i: I
+            }
+            interface I {
+                x: String
+            }
+            type A implements I {
+                x: String
+            }",
+        )
+        .query("{i{__typename x}}")
+        .response(json! {{
+            "i": {
+                "__typename": "I",
+                "x": "foo",
+            }
+        }})
+        .expected(json! {{
+            "i": null,
+        }})
+        .test();
+}
+
+#[test]
+fn reformat_response_unknown_typename() {
+    // If in a response we get a typename for a completely unknown type name, then we should
+    // nullify the object as something is off, and in the worst case we could be inadvertently
+    // leaking some @inaccessible type (or the subgraph is simply drunk but nullifying is fine too
+    // then). This should happen whether the actual __typename is queried or not.
+    let schema = "
+      type Query {
+          i: I
+      }
+      interface I {
+          x: String
+      }
+      type A implements I {
+          x: String
+      }";
+
+    // Without __typename queried
+    FormatTest::builder()
+        .schema(schema)
+        .query("{i{x}}")
+        .response(json! {{
+            "i": {
+                "__typename": "X",
+                "x": "foo",
+            }
+        }})
+        .expected(json! {{ "i": null, }})
+        .test();
+
+    // With __typename queried
+    FormatTest::builder()
+        .schema(schema)
+        .query("{i{__typename x}}")
+        .response(json! {{
+            "i": {
+                "__typename": "X",
+                "x": "foo",
+            }
+        }})
+        .expected(json! {{ "i": null, }})
+        .test();
+}
+
 macro_rules! run_validation {
     ($schema:expr, $query:expr, $variables:expr $(,)?) => {{
         let variables = match $variables {