From 7fe1465cf35c2efe37ac5c73fac2b7ea04173318 Mon Sep 17 00:00:00 2001
From: Dariusz Kuc <9501705+dariuszkuc@users.noreply.github.com>
Date: Thu, 10 Oct 2024 11:16:21 -0500
Subject: [PATCH] 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 --->
---
 .changeset/quiet-rings-fly.md                 |   7 +
 .../src/__tests__/buildPlan.test.ts           | 125 ++++++++++++++++++
 query-planner-js/src/buildPlan.ts             |   6 +-
 3 files changed, 137 insertions(+), 1 deletion(-)
 create mode 100644 .changeset/quiet-rings-fly.md

diff --git a/.changeset/quiet-rings-fly.md b/.changeset/quiet-rings-fly.md
new file mode 100644
index 000000000..b4d0fc53f
--- /dev/null
+++ b/.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.
diff --git a/query-planner-js/src/__tests__/buildPlan.test.ts b/query-planner-js/src/__tests__/buildPlan.test.ts
index 5283ffe3f..d0e5d3864 100644
--- a/query-planner-js/src/__tests__/buildPlan.test.ts
+++ b/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
+                    }
+                  }
+                }
+              }
+            },
+          },
+        },
+      }
+    `);
+});
diff --git a/query-planner-js/src/buildPlan.ts b/query-planner-js/src/buildPlan.ts
index bc550aba3..dce90edad 100644
--- a/query-planner-js/src/buildPlan.ts
+++ b/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);
     }