fix: ensure we are removing ALL useless groups (#3163)

When removing "useless" fetch nodes/groups we remove them in-place while
still iterating over the same list. This leads to potentially skipping
processing of some the children fetch nodes, as when we remove nodes we
left shift all remaining children but the iterator keeps the old
position unchanged effectively skipping next child.

<!-- FED-386 --->

Author: dariuszkuc

.changeset/quiet-rings-fly.md

@@ -0,0 +1,7 @@
+---
+"@apollo/query-planner": patch
+---
+
+Ensure all useless fetch groups are removed
+
+When removing "useless" fetch nodes/groups we remove them in-place while still iterating over the same list. This leads to potentially skipping processing of some the children fetch nodes, as when we remove nodes we left shift all remaining children but the iterator keeps the old position unchanged effectively skipping next child.

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

@@ -10124,3 +10124,128 @@ describe('@fromContext impacts on query planning', () => {
     ]);
   });
 });
+
+test('ensure we are removing all useless groups', () => {
+  const subgraph1 = {
+    name: 'Subgraph1',
+    typeDefs: gql`
+      type Query {
+        foo: Foo!
+      }
+
+      type Foo {
+        item: I
+      }
+
+      type I @interfaceObject @key(fields: "id") {
+        id: ID!
+      }
+    `,
+  };
+
+  const subgraph2 = {
+    name: 'Subgraph2',
+    typeDefs: gql`
+      interface I @key(fields: "id") {
+        id: ID!
+        name: String!
+      }
+
+      type A implements I @key(fields: "id") {
+        id: ID!
+        name: String!
+        x: X!
+      }
+
+      type X {
+        id: ID!
+      }
+
+      type B implements I @key(fields: "id") {
+        id: ID!
+        name: String!
+        y: Y!
+      }
+
+      type Y {
+        id: ID!
+      }
+    `,
+  };
+
+  const [api, queryPlanner] = composeAndCreatePlanner(subgraph1, subgraph2);
+  const operation = operationFromDocument(
+    api,
+    gql`
+      {
+        foo {
+          item {
+            __typename
+            id
+            ... on A {
+              __typename
+              id
+              x {
+                __typename
+                id
+              }
+            }
+            ... on B {
+              __typename
+              id
+              y {
+                __typename
+                id
+              }
+            }
+          }
+        }
+      }
+    `,
+  );
+
+  const plan = queryPlanner.buildQueryPlan(operation);
+  expect(plan).toMatchInlineSnapshot(`
+      QueryPlan {
+        Sequence {
+          Fetch(service: "Subgraph1") {
+            {
+              foo {
+                item {
+                  __typename
+                  id
+                }
+              }
+            }
+          },
+          Flatten(path: "foo.item") {
+            Fetch(service: "Subgraph2") {
+              {
+                ... on I {
+                  __typename
+                  id
+                }
+              } =>
+              {
+                ... on I {
+                  __typename
+                  ... on A {
+                    x {
+                      __typename
+                      id
+                    }
+                  }
+                  ... on B {
+                    y {
+                      __typename
+                      id
+                    }
+                  }
+                }
+              }
+            },
+          },
+        },
+      }
+    `);
+});

query-planner-js/src/buildPlan.ts

@@ -2612,8 +2612,12 @@ class FetchDependencyGraph {
   }
 
   private removeUselessGroups(group: FetchGroup) {
+    // Since we can potentially remove child in place, we need to
+    // explicitly copy the list of all children to ensure that we
+    // process ALL children
+    const children = group.children().concat();
     // Recursing first, this makes it a bit easier to reason about.
-    for (const child of group.children()) {
+    for (const child of children) {
       this.removeUselessGroups(child);
     }