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); }