Skip to content

Commit

Permalink
Do not override concrete type names with interface names when merging…
Browse files Browse the repository at this point in the history 
… responses (#6250)

Co-authored-by: Iryna Shestak <[email protected]>
  • Loading branch information
@tninesling @lrlna
tninesling and lrlna authored Nov 15, 2024
1 parent d6135f6 commit e8d32d6
Show file tree
Hide file tree
Showing 4 changed files with 128 additions and 10 deletions.
5 changes: 5 additions & 0 deletions .changesets/fix_tninesling_typename_resolution.md
View file
Original file line number Diff line number Diff line change
@@ -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
113 changes: 113 additions & 0 deletions apollo-router/src/json_ext.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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!(
Expand Down
18 changes: 9 additions & 9 deletions apollo-router/src/query_planner/execution.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
}
Expand Down Expand Up @@ -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());
}
}
Expand Down Expand Up @@ -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()));
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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 {
Expand All @@ -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())
}
}
Expand Down Expand Up @@ -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)
}

Expand All @@ -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
Expand Down
2 changes: 1 addition & 1 deletion apollo-router/src/query_planner/selection.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}
Expand Down

0 comments on commit e8d32d6

Please sign in to comment.