Fix assertion error on some multi-field @requires dependencies (#2575)

When handling a `@requires`, the code gathers what it expected to be the set
of "fetch groups" needed to fulfill the requirement, but in the rare cases,
the same group can be added multiple times and in that case it was not
de-duplicated. This duplication was unexpected by the rest of the code,
ultimately leading to triggering an assertion.

This would happen when a `@requires` was on multiple fields, and one of those
field was also a condition (say a key) for another of the required field.

Co-authored-by: Sylvain Lebresne <lebresene@gmail.com>

.changeset/shaggy-crews-repair.md

@@ -0,0 +1,11 @@
+---
+"@apollo/query-planner": patch
+---
+
+Fix potential assertion error during query planning in some multi-field `@requires` case. This error could be triggered
+when a field in a `@requires` depended on another field that was also part of that same requires (for instance, if a
+field has a `@requires(fields: "id otherField")` and that `id` is also a key necessary to reach the subgraph providing
+`otherField`).
+
+The assertion error thrown in that case contained the message `Root groups (...) should have no remaining groups unhandled (...)`
+

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

@@ -2716,6 +2716,108 @@ describe('@requires', () => {
       }
     `);
   });
+
+  it('require of multiple field, when one is also a key to reach another', () => {
+    // The specificity of this example is that we `T.v` requires 2 fields `req1`
+    // and `req2`, but `req1` is also a key to get `req2`. This dependency was
+    // confusing a previous version of the code (which, when gathering the
+    // "createdGroups" for `T.v` @requires, was using the group for `req1` twice
+    // separatly (instead of recognizing it was the same group), and this was
+    // confusing the rest of the code was wasn't expecting it.
+    const subgraph1 = {
+      name: 'A',
+      typeDefs: gql`
+        type Query {
+          t : T
+        }
+
+        type T @key(fields: "id1") @key(fields: "req1") {
+          id1: ID!
+          req1: Int
+        }
+      `
+    }
+
+    const subgraph2 = {
+      name: 'B',
+      typeDefs: gql`
+        type T @key(fields: "id1") {
+          id1: ID!
+          req1: Int @external
+          req2: Int @external
+          v: Int @requires(fields: "req1 req2")
+        }
+      `
+    }
+
+    const subgraph3 = {
+      name: 'C',
+      typeDefs: gql`
+        type T @key(fields: "req1") {
+          req1: Int
+          req2: Int
+        }
+      `
+    }
+
+    const [api, queryPlanner] = composeAndCreatePlanner(subgraph1, subgraph2, subgraph3);
+    const operation = operationFromDocument(api, gql`
+      {
+        t {
+          v
+        }
+      }
+    `);
+
+    const plan = queryPlanner.buildQueryPlan(operation);
+    expect(plan).toMatchInlineSnapshot(`
+      QueryPlan {
+        Sequence {
+          Fetch(service: "A") {
+            {
+              t {
+                __typename
+                id1
+                req1
+              }
+            }
+          },
+          Flatten(path: "t") {
+            Fetch(service: "C") {
+              {
+                ... on T {
+                  __typename
+                  req1
+                }
+              } =>
+              {
+                ... on T {
+                  req2
+                }
+              }
+            },
+          },
+          Flatten(path: "t") {
+            Fetch(service: "B") {
+              {
+                ... on T {
+                  __typename
+                  req1
+                  req2
+                  id1
+                }
+              } =>
+              {
+                ... on T {
+                  v
+                }
+              }
+            },
+          },
+        },
+      }
+    `);
+  });
 });
 
 describe('fetch operation names', () => {

query-planner-js/src/buildPlan.ts

@@ -3628,6 +3628,14 @@ function maybeSubstratPathPrefix(basePath: OperationPath, maybePrefix: Operation
   return undefined;
 }
 
+function updateCreatedGroups(createdGroups: FetchGroup[], ...newCreatedGroups: FetchGroup[]) {
+  for (const newGroup of newCreatedGroups) {
+    if (!createdGroups.includes(newGroup)) {
+      createdGroups.push(newGroup);
+    }
+  }
+}
+
 function computeGroupsForTree(
   dependencyGraph: FetchDependencyGraph,
   pathTree: OpPathTree<any>,
@@ -3649,7 +3657,7 @@ function computeGroupsForTree(
     context: initialContext,
     deferContext: initialDeferContext,
   }];
-  const createdGroups = [ ];
+  const createdGroups: FetchGroup[] = [ ];
   while (stack.length > 0) {
     const { tree, group, path, context, deferContext } = stack.pop()!;
     if (tree.localSelections) {
@@ -3675,7 +3683,7 @@ function computeGroupsForTree(
             assert(conditions, () => `Key edge ${edge} should have some conditions paths`);
             // First, we need to ensure we fetch the conditions from the current group.
             const conditionsGroups = computeGroupsForTree(dependencyGraph, conditions, group, path, deferContextForConditions(deferContext));
-            createdGroups.push(...conditionsGroups);
+            updateCreatedGroups(createdGroups, ...conditionsGroups);
             // Then we can "take the edge", creating a new group. That group depends
             // on the condition ones.
             const sourceType = edge.head.type as CompositeType; // We shouldn't have a key on a non-composite type
@@ -3700,7 +3708,7 @@ function computeGroupsForTree(
               conditionsGroups,
               deferRef: updatedDeferContext.activeDeferRef,
             });
-            createdGroups.push(newGroup);
+            updateCreatedGroups(createdGroups, newGroup);
             newGroup.addParents(conditionsGroups.map((conditionGroup) => {
               // If `conditionGroup` parent is `group`, that is the same as `newGroup` current parent, then we can infer the path of `newGroup` into
               // that condition `group` by looking at the paths of each to their common parent. But otherwise, we cannot have a proper
@@ -3846,7 +3854,7 @@ function computeGroupsForTree(
             );
             updated.group = requireResult.group;
             updated.path = requireResult.path;
-            createdGroups.push(...requireResult.createdGroups);
+            updateCreatedGroups(createdGroups, ...requireResult.createdGroups);
           }
 
           if (updatedOperation.kind === 'Field' && updatedOperation.name === typenameFieldName) {
@@ -4224,10 +4232,11 @@ function handleRequires(
       parent.group,
       postRequireGroup,
     );
+    updateCreatedGroups(unmergedGroups, postRequireGroup);
     return {
       group: postRequireGroup,
       path: path.forNewKeyFetch(createFetchInitialPath(dependencyGraph.supergraphSchema, entityType, context)),
-      createdGroups: unmergedGroups.concat(postRequireGroup),
+      createdGroups: unmergedGroups,
     };
   } else {
     // We're in the somewhat simpler case where a @require happens somewhere in the middle of a subgraph query (so, not
@@ -4258,10 +4267,11 @@ function handleRequires(
       group,
       newGroup,
     );
+    updateCreatedGroups(createdGroups, newGroup);
     return {
       group: newGroup,
       path: path.forNewKeyFetch(createFetchInitialPath(dependencyGraph.supergraphSchema, entityType, context)),
-      createdGroups: createdGroups.concat(newGroup),
+      createdGroups,
     };
   }
 }