fix: handle directive conditions on fragments when building query gra…

…phs (#2875) This PR addresses issues with handling fragments when they specify directive conditions: * when exploding the types we were not propagating directive conditions * when processing fragment that specifies super type of an existing type and also specifies directive condition, we were incorrectly preserving the unnecessary type condition. This type condition was problematic as it could be referencing types from supergraph that were not available in the local schema. Instead, we now drop the redundant type condition and only preserve the directives (if specified). Related: * fixes #2862 * supersedes #2864 --------- Co-authored-by: Chris Lenfest <[email protected]>
apollographql · Dec 5, 2023 · 3f7392b · 3f7392b
1 parent 28d1709
commit 3f7392b
Show file tree

Hide file tree

Showing 3 changed files with 198 additions and 2 deletions.
diff --git a/.changeset/shiny-forks-happen.md b/.changeset/shiny-forks-happen.md
@@ -0,0 +1,9 @@
+---
+"@apollo/query-graphs": patch
+---
+
+fix: handle directive conditions on fragments when building query graphs
+
+This fix addresses issues with handling fragments when they specify directive conditions:
+* when exploding the types we were not propagating directive conditions
+* when processing fragment that specifies super type of an existing type and also specifies directive condition, we were incorrectly preserving the unnecessary type condition. This type condition was problematic as it could be referencing types from supergraph that were not available in the local schema. Instead, we now drop the redundant type condition and only preserve the directives (if specified).  
diff --git a/query-graphs-js/src/graphPath.ts b/query-graphs-js/src/graphPath.ts
@@ -2509,7 +2509,7 @@ function advanceWithOperation<V extends Vertex>(
         const optionsByImplems: OpGraphPath<V>[][][] = [];
         for (const tName of intersection) {
           debug.group(() => `Trying ${tName}`);
-          const castOp = new FragmentElement(currentType, tName);
+          const castOp = new FragmentElement(currentType, tName, operation.appliedDirectives);
           const implemOptions = advanceSimultaneousPathsWithOperation(
             supergraphSchema,
             new SimultaneousPathsWithLazyIndirectPaths([path], context, conditionResolver),
@@ -2548,8 +2548,13 @@ function advanceWithOperation<V extends Vertex>(
         const conditionType = supergraphSchema.type(typeName)!;
         if (isAbstractType(conditionType) && possibleRuntimeTypes(conditionType).some(t => t.name == currentType.name)) {
           debug.groupEnd(() => `${typeName} is a super-type of current type ${currentType}: no edge to take`);
+          // Operation type condition is applicable on the current type, so the types are already exploded but the
+          // condition can reference types from the supergraph that are not present in the local subgraph.
+          //
+          // If operation has applied directives we need to convert to inline fragment without type condition, otherwise
+          // we ignore the fragment altogether.
           const updatedPath = operation.appliedDirectives.length > 0
-            ? path.add(operation, null, noConditionsResolution, operation.deferDirectiveArgs())
+            ? path.add(operation.withUpdatedTypes(currentType, undefined), null, noConditionsResolution, operation.deferDirectiveArgs())
             : path;
           return { options: [[ updatedPath ]] };
         }

diff --git a/query-planner-js/src/__tests__/buildPlan.test.ts b/query-planner-js/src/__tests__/buildPlan.test.ts
@@ -7973,3 +7973,185 @@ describe('@requires references external field indirectly', () => {
     `);
   });
 });
+
+describe('handles fragments with directive conditions', () => {
+  test('fragment with intersecting parent type and directive condition', () => {
+    const subgraphA = {
+      typeDefs: gql`
+        directive @test on FRAGMENT_SPREAD
+        type Query {
+          i: I
+        }
+        interface I {
+          _id: ID
+        }
+        type T1 implements I @key(fields: "id") {
+          _id: ID
+          id: ID
+        }
+        type T2 implements I @key(fields: "id") {
+          _id: ID
+          id: ID
+        }
+      `,
+      name: 'A',
+    };
+
+    const subgraphB = {
+      typeDefs: gql`
+        directive @test on FRAGMENT_SPREAD
+        type Query {
+          i2s: [I2]
+        }
+        interface I2 {
+          id: ID
+          title: String
+        }
+        type T1 implements I2 @key(fields: "id") {
+          id: ID
+          title: String
+        }
+        type T2 implements I2 @key(fields: "id") {
+          id: ID
+          title: String
+        }
+      `,
+      name: 'B',
+    };
+
+    const [api, queryPlanner] = composeAndCreatePlanner(subgraphA, subgraphB);
+
+    const operation = operationFromDocument(
+      api,
+      gql`
+        query {
+          i {
+            _id
+            ... on I2 @test {
+              id
+            }
+          }
+        }
+      `,
+    );
+
+    const queryPlan = queryPlanner.buildQueryPlan(operation);
+    expect(queryPlan).toMatchInlineSnapshot(`
+      QueryPlan {
+        Fetch(service: "A") {
+          {
+            i {
+              __typename
+              _id
+              ... on T1 @test {
+                id
+              }
+              ... on T2 @test {
+                id
+              }
+            }
+          }
+        },
+      }
+    `);
+  });
+
+  test('nested fragment with interseting parent type and directive condition', () => {
+    const subgraphA = {
+      typeDefs: gql`
+        directive @test on FRAGMENT_SPREAD
+        type Query {
+          i: I
+        }
+        interface I {
+          _id: ID
+        }
+        type T1 implements I @key(fields: "id") {
+          _id: ID
+          id: ID
+        }
+        type T2 implements I @key(fields: "id") {
+          _id: ID
+          id: ID
+        }
+      `,
+      name: 'A',
+    };
+
+    const subgraphB = {
+      typeDefs: gql`
+        directive @test on FRAGMENT_SPREAD
+        type Query {
+          i2s: [I2]
+        }
+        interface I2 {
+          id: ID
+          title: String
+        }
+        type T1 implements I2 @key(fields: "id") {
+          id: ID
+          title: String
+        }
+        type T2 implements I2 @key(fields: "id") {
+          id: ID
+          title: String
+        }
+      `,
+      name: 'B',
+    };
+
+    const [api, queryPlanner] = composeAndCreatePlanner(subgraphA, subgraphB);
+
+    const operation = operationFromDocument(
+      api,
+      gql`
+        query {
+          i {
+            _id
+            ... on I2 {
+              ... on I2 @test {
+                id
+              }
+            }
+          }
+        }
+      `,
+    );
+
+    expect(operation.expandAllFragments().toString()).toMatchInlineSnapshot(`
+      "{
+        i {
+          _id
+          ... on I2 {
+            ... on I2 @test {
+              id
+            }
+          }
+        }
+      }"
+    `);
+    const queryPlan = queryPlanner.buildQueryPlan(operation);
+    expect(queryPlan).toMatchInlineSnapshot(`
+      QueryPlan {
+        Fetch(service: "A") {
+          {
+            i {
+              __typename
+              _id
+              ... on T1 {
+                ... @test {
+                  id
+                }
+              }
+              ... on T2 {
+                ... @test {
+                  id
+                }
+              }
+            }
+          }
+        },
+      }
+    `);
+  });
+});