Avoid reusing named fragments that are invalid for the subgraph (#2255)

In some subtle conditions (mostly related to interface merging), it is possible that a named fragment from the user query "matches" in a subgraph fetch, yet to have that fragment being invalid for the subgraph (see test included in this patch for an example). This patch ensures we don't reuse such fragments.
apollographql · Nov 18, 2022 · 867ac31 · 867ac31
1 parent b54ccc1
commit 867ac31
Show file tree

Hide file tree

Showing 4 changed files with 112 additions and 3 deletions.
diff --git a/gateway-js/CHANGELOG.md b/gateway-js/CHANGELOG.md
@@ -5,6 +5,7 @@ This CHANGELOG pertains only to Apollo Federation packages in the 2.x range. The
 ## vNext
 
 - Fix issue where QP was generating invalid plan missing some data [#361](https://github.com/apollographql/federation/issues/361).
+- Avoid reusing named fragments that are invalid for the subgraph [PR #2255](https://github.com/apollographql/federation/pull/2255).
 - Fix QP not always type-exploding interface when necessary [PR #2246](https://github.com/apollographql/federation/pull/2246).
 - Fix potential QP issue with shareable root fields [PR #2239](https://github.com/apollographql/federation/pull/2239).
 - Correctly reject field names starting with `__` [PR #2237](https://github.com/apollographql/federation/pull/2237).

diff --git a/internals-js/src/operations.ts b/internals-js/src/operations.ts
@@ -690,10 +690,35 @@ export class NamedFragmentDefinition extends DirectiveTargetElement<NamedFragmen
    * @param type - the type at which we're looking at applying the fragment
    */
   canApplyAtType(type: CompositeType): boolean {
-    return (
+    const applyAtType =
       sameType(this.typeCondition, type)
-      || (isAbstractType(this.typeCondition) && !isUnionType(type) && isDirectSubtype(this.typeCondition, type))
-    );
+      || (isAbstractType(this.typeCondition) && !isUnionType(type) && isDirectSubtype(this.typeCondition, type));
+    return applyAtType
+      && this.validForSchema(type.schema());
+  }
+
+  // Checks whether this named fragment can be applied to the provided schema, which might be different
+  // from the one the named fragment originate from.
+  private validForSchema(schema: Schema): boolean {
+    if (schema === this.schema()) {
+      return true;
+    }
+
+    const typeInSchema = schema.type(this.typeCondition.name);
+    if (!typeInSchema || !isCompositeType(typeInSchema)) {
+      return false;
+    }
+
+    // We try "rebasing" the selection into the provided schema and checks if that succeed.
+    try {
+      const rebasedSelection = new SelectionSet(typeInSchema);
+      rebasedSelection.mergeIn(this.selectionSet);
+      // If this succeed, it means the fragment could be applied to that schema and be valid.
+      return true;
+    } catch (e) {
+      // We don't really care what kind of error was triggered; only that it doesn't work.
+      return false;
+    }
   }
 
   toString(indent?: string): string {

diff --git a/query-planner-js/CHANGELOG.md b/query-planner-js/CHANGELOG.md
@@ -4,6 +4,7 @@ This CHANGELOG pertains only to Apollo Federation packages in the 2.x range. The
 
 ## vNext
 
+- Avoid reusing named fragments that are invalid for the subgraph [PR #2255](https://github.com/apollographql/federation/pull/2255).
 - Fix QP not always type-exploding interface when necessary [PR #2246](https://github.com/apollographql/federation/pull/2246).
 - Fix potential QP issue with shareable root fields [PR #2239](https://github.com/apollographql/federation/pull/2239).
 

diff --git a/query-planner-js/src/__tests__/buildPlan.test.ts b/query-planner-js/src/__tests__/buildPlan.test.ts
@@ -4052,6 +4052,88 @@ describe('Named fragments preservation', () => {
       }
     `);
   });
+
+  it('do not try to apply fragments that are not valid for the subgaph', () => {
+    // Slightly artificial example for simplicity, but this highlight the problem.
+    // In that example, the only queried subgraph is the first one (there is in fact
+    // no way to ever reach the 2nd one), so the plan should mostly simply forward
+    // the query to the 1st subgraph, but a subtlety is that the named fragment used
+    // in the query is *not* valid for Subgraph1, because it queries `b` on `I`, but
+    // there is no `I.b` in Subgraph1.
+    // So including the named fragment in the fetch would be erroneous: the subgraph
+    // server would reject it when validating the query, and we must make sure it
+    // is not reused.
+    const subgraph1 = {
+      name: 'Subgraph1',
+      typeDefs: gql`
+        type Query {
+          i1: I
+          i2: I
+        }
+
+        interface I {
+          a: Int
+        }
+
+        type T implements I {
+          a: Int
+          b: Int
+        }
+      `
+    }
+
+    const subgraph2 = {
+      name: 'Subgraph2',
+      typeDefs: gql`
+        interface I {
+          a: Int
+          b: Int
+        }
+      `
+    }
+
+    const [api, queryPlanner] = composeAndCreatePlanner(subgraph1, subgraph2);
+    const operation = operationFromDocument(api, gql`
+      query {
+        i1 {
+          ... on T {
+            ...Frag
+          }
+        }
+        i2 {
+          ... on T {
+            ...Frag
+          }
+        }
+      }
+
+      fragment Frag on I {
+        b
+      }
+    `);
+
+    const plan = queryPlanner.buildQueryPlan(operation);
+    expect(plan).toMatchInlineSnapshot(`
+      QueryPlan {
+        Fetch(service: "Subgraph1") {
+          {
+            i1 {
+              __typename
+              ... on T {
+                b
+              }
+            }
+            i2 {
+              __typename
+              ... on T {
+                b
+              }
+            }
+          }
+        },
+      }
+    `);
+  });
 });
 
 test('works with key chains', () => {