generateQueryFragments generates invalid plans with missing fragment definitions (#2993)

This PR includes a failing test that show that `generateQueryFragments`
can generate invalid GraphQL queries. It seems related to the way the
hashCode is computed. Any selections of the same length and type are
somehow grouped together and an additional fragment definition is not
added.

Opening this in case somebody gets to the root cause fix while I
investigate further.

---------

Co-authored-by: Marc-Andre Giroux <[email protected]>

Author: xuorig

.changeset/stupid-lemons-rest.md

@@ -0,0 +1,26 @@
+---
+"@apollo/query-planner": patch
+---
+
+Fix issue with missing fragment definitions due to `generateQueryFragments`.
+
+An incorrect implementation detail in `generateQueryFragments` caused certain queries to be missing fragment definitions. Specifically, subsequent fragment "candidates" with the same type condition and the same length of selections as a previous fragment weren't correctly added to the list of fragments. An example of an affected query is:
+
+```graphql
+query {
+  t {
+    ... on A {
+      x
+      y
+    }
+  }
+  t2 {
+    ... on A {
+      y
+      z
+    }
+  }
+}
+```
+
+In this case, the second selection set would be converted to an inline fragment spread to subgraph fetches, but the fragment definition would be missing.

internals-js/src/operations.ts

@@ -1554,6 +1554,7 @@ export class SelectionSet {
         // compute and handle collisions as necessary.
         const mockHashCode = `on${selection.element.typeCondition}` + selection.selectionSet.selections().length;
         const equivalentSelectionSetCandidates = seenSelections.get(mockHashCode);
+
         if (equivalentSelectionSetCandidates) {
           // See if any candidates have an equivalent selection set, i.e. {x y} and {y x}.
           const match = equivalentSelectionSetCandidates.find(([candidateSet]) => candidateSet.equals(selection.selectionSet!));
@@ -1572,13 +1573,13 @@ export class SelectionSet {
           `_generated_${mockHashCode}_${equivalentSelectionSetCandidates?.length ?? 0}`,
           selection.element.typeCondition
         ).setSelectionSet(minimizedSelectionSet);
+        namedFragments.add(fragmentDefinition);
 
         // Create a new "hash code" bucket or add to the existing one.
-        if (!equivalentSelectionSetCandidates) {
-          seenSelections.set(mockHashCode, [[selection.selectionSet, fragmentDefinition]]);
-          namedFragments.add(fragmentDefinition);
-        } else {
+        if (equivalentSelectionSetCandidates) {
           equivalentSelectionSetCandidates.push([selection.selectionSet, fragmentDefinition]);
+        } else {
+            seenSelections.set(mockHashCode, [[selection.selectionSet, fragmentDefinition]]);
         }
 
         return new FragmentSpreadSelection(this.parentType, namedFragments, fragmentDefinition, []);

query-planner-js/src/__tests__/buildPlan.test.ts

@@ -5215,6 +5215,7 @@ describe('Fragment autogeneration', () => {
       type A {
         x: Int
         y: Int
+        z: Int
         t: T
       }
 
@@ -5425,6 +5426,60 @@ describe('Fragment autogeneration', () => {
       }
     `);
   });
+
+  it('fragments that share a hash but are not identical generate their own fragment definitions', () => {
+    const [api, queryPlanner] = composeAndCreatePlannerWithOptions([subgraph], {
+      generateQueryFragments: true,
+    });
+    const operation = operationFromDocument(
+      api,
+      gql`
+        query {
+          t {
+            ... on A {
+              x
+              y
+            }
+          }
+          t2 {
+            ... on A {
+              y
+              z
+            }
+          }
+        }
+      `,
+    );
+
+    const plan = queryPlanner.buildQueryPlan(operation);
+
+    expect(plan).toMatchInlineSnapshot(`
+      QueryPlan {
+        Fetch(service: "Subgraph1") {
+          {
+            t {
+              __typename
+              ..._generated_onA2_0
+            }
+            t2 {
+              __typename
+              ..._generated_onA2_1
+            }
+          }
+          
+          fragment _generated_onA2_0 on A {
+            x
+            y
+          }
+          
+          fragment _generated_onA2_1 on A {
+            y
+            z
+          }
+        },
+      }
+    `);
+  });
 });
 
 test('works with key chains', () => {