Do not override concrete type names with interface names when merging responses

.changesets/fix_tninesling_typename_resolution.md

@@ -0,0 +1,5 @@
+### Do not override concrete type names with interface names when merging responses ([PR #6250](https://github.com/apollographql/router/pull/6250))
+
+When using `@interfaceObject`, differing pieces of data can come back with either concrete types or interface types depending on the source. To make the response merging order-agnostic, check the schema to ensure concrete types are not overwritten with interfaces or less specific types.
+
+By [@tninesling](https://github.com/tninesling) in https://github.com/apollographql/router/pull/6250

apollo-router/src/json_ext.rs

@@ -72,6 +72,12 @@ pub(crate) trait ValueExt {
     #[track_caller]
     fn deep_merge(&mut self, other: Self);
 
+    /// Deep merge two JSON objects, overwriting values in `self` if it has the same key as `other`.
+    /// For GraphQL response objects, this uses schema information to avoid overwriting a concrete
+    /// `__typename` with an interface name.
+    #[track_caller]
+    fn type_aware_deep_merge(&mut self, other: Self, schema: &Schema);
+
     /// Returns `true` if the values are equal and the objects are ordered the same.
     ///
     /// **Note:** this is recursive.
@@ -192,6 +198,53 @@ impl ValueExt for Value {
         }
     }
 
+    fn type_aware_deep_merge(&mut self, other: Self, schema: &Schema) {
+        match (self, other) {
+            (Value::Object(a), Value::Object(b)) => {
+                for (key, value) in b.into_iter() {
+                    let k = key.clone();
+                    match a.entry(key) {
+                        Entry::Vacant(e) => {
+                            e.insert(value);
+                        }
+                        Entry::Occupied(e) => match (e.into_mut(), value) {
+                            (Value::String(type1), Value::String(type2))
+                                if k.as_str() == TYPENAME =>
+                            {
+                                // If type1 is a subtype of type2, we skip this overwrite to preserve the more specific `__typename`
+                                // in the response. Ideally, we could use `Schema::is_implementation`, but that looks to be buggy
+                                // and does not catch the problem we are trying to resolve.
+                                if !schema.is_subtype(type2.as_str(), type1.as_str()) {
+                                    *type1 = type2;
+                                }
+                            }
+                            (t, s) => t.type_aware_deep_merge(s, schema),
+                        },
+                    }
+                }
+            }
+            (Value::Array(a), Value::Array(mut b)) => {
+                for (b_value, a_value) in b.drain(..min(a.len(), b.len())).zip(a.iter_mut()) {
+                    a_value.type_aware_deep_merge(b_value, schema);
+                }
+
+                a.extend(b);
+            }
+            (_, Value::Null) => {}
+            (Value::Object(_), Value::Array(_)) => {
+                failfast_debug!("trying to replace an object with an array");
+            }
+            (Value::Array(_), Value::Object(_)) => {
+                failfast_debug!("trying to replace an array with an object");
+            }
+            (a, b) => {
+                if b != Value::Null {
+                    *a = b;
+                }
+            }
+        }
+    }
+
     #[cfg(test)]
     fn eq_and_ordered(&self, other: &Self) -> bool {
         match (self, other) {
@@ -1333,6 +1386,66 @@ mod tests {
         );
     }
 
+    #[test]
+    fn interface_typename_merging() {
+        let schema = Schema::parse(
+                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 @link(url: String, as: String, for: link__Purpose, import: [link__Import]) repeatable on SCHEMA
+            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__graph(name: String!, url: String!) on ENUM_VALUE
+
+            scalar link__Import
+            scalar join__FieldSet
+
+            enum link__Purpose {
+                SECURITY
+                EXECUTION
+            }
+
+            enum join__Graph {
+                TEST @join__graph(name: "test", url: "http://localhost:4001/graphql")
+            }
+
+            interface I {
+                s: String
+            }
+
+            type C implements I {
+                s: String
+            }
+
+            type Query {
+                i: I
+            }
+        "#,
+            &Default::default(),
+        )
+        .expect("valid schema");
+        let mut response1 = json!({
+            "__typename": "C"
+        });
+        let response2 = json!({
+            "__typename": "I",
+            "s": "data"
+        });
+
+        response1.type_aware_deep_merge(response2, &schema);
+
+        assert_eq!(
+            response1,
+            json!({
+                "__typename": "C",
+                "s": "data"
+            })
+        );
+    }
+
     #[test]
     fn test_is_subset_eq() {
         assert_is_subset!(

apollo-router/src/query_planner/execution.rs

@@ -139,7 +139,7 @@ impl PlanNode {
                                 )
                                 .in_current_span()
                                 .await;
-                            value.deep_merge(v);
+                            value.type_aware_deep_merge(v, parameters.schema);
                             errors.extend(err.into_iter());
                         }
                     }
@@ -167,7 +167,7 @@ impl PlanNode {
                             .collect();
 
                         while let Some((v, err)) = stream.next().in_current_span().await {
-                            value.deep_merge(v);
+                            value.type_aware_deep_merge(v, parameters.schema);
                             errors.extend(err.into_iter());
                         }
                     }
@@ -305,7 +305,7 @@ impl PlanNode {
                                     "otel.kind" = "INTERNAL"
                                 ))
                                 .await;
-                            value.deep_merge(v);
+                            value.type_aware_deep_merge(v, parameters.schema);
                             errors.extend(err.into_iter());
 
                             let _ = primary_sender.send((value.clone(), errors.clone()));
@@ -353,7 +353,7 @@ impl PlanNode {
                                         "otel.kind" = "INTERNAL"
                                     ))
                                     .await;
-                                value.deep_merge(v);
+                                value.type_aware_deep_merge(v, parameters.schema);
                                 errors.extend(err.into_iter());
                             } else if current_dir.is_empty() {
                                 // If the condition is on the root selection set and it's the only one
@@ -373,7 +373,7 @@ impl PlanNode {
                                     "otel.kind" = "INTERNAL"
                                 ))
                                 .await;
-                            value.deep_merge(v);
+                            value.type_aware_deep_merge(v, parameters.schema);
                             errors.extend(err.into_iter());
                         } else if current_dir.is_empty() {
                             // If the condition is on the root selection set and it's the only one
@@ -451,7 +451,7 @@ impl DeferredNode {
             if is_depends_empty {
                 let (primary_value, primary_errors) =
                     primary_receiver.recv().await.unwrap_or_default();
-                value.deep_merge(primary_value);
+                value.type_aware_deep_merge(primary_value, &sc);
                 errors.extend(primary_errors)
             } else {
                 while let Some((v, _remaining)) = stream.next().await {
@@ -460,7 +460,7 @@ impl DeferredNode {
                     // or because it is lagging, but here we only send one message so it
                     // will not happen
                     if let Some(Ok((deferred_value, err))) = v {
-                        value.deep_merge(deferred_value);
+                        value.type_aware_deep_merge(deferred_value, &sc);
                         errors.extend(err.into_iter())
                     }
                 }
@@ -499,7 +499,7 @@ impl DeferredNode {
                 if !is_depends_empty {
                     let (primary_value, primary_errors) =
                         primary_receiver.recv().await.unwrap_or_default();
-                    v.deep_merge(primary_value);
+                    v.type_aware_deep_merge(primary_value, &sc);
                     errors.extend(primary_errors)
                 }
 
@@ -524,7 +524,7 @@ impl DeferredNode {
             } else {
                 let (primary_value, primary_errors) =
                     primary_receiver.recv().await.unwrap_or_default();
-                value.deep_merge(primary_value);
+                value.type_aware_deep_merge(primary_value, &sc);
                 errors.extend(primary_errors);
 
                 if let Err(e) = tx

apollo-router/src/query_planner/selection.rs

@@ -175,7 +175,7 @@ pub(crate) fn execute_selection_set<'a>(
                                         e.insert(value);
                                     }
                                     Entry::Occupied(e) => {
-                                        e.into_mut().deep_merge(value);
+                                        e.into_mut().type_aware_deep_merge(value, schema);
                                     }
                                 }
                             }