diff --git a/.changeset/gentle-plums-relax.md b/.changeset/gentle-plums-relax.md new file mode 100644 index 000000000..804caf918 --- /dev/null +++ b/.changeset/gentle-plums-relax.md @@ -0,0 +1,7 @@ +--- +"@apollo/query-planner": patch +--- + +Fix bug in the handling of dependencies of subgraph fetches. This bug was manifesting itself as an assertion error +thrown during query planning with a message of the form `Root groups X should have no remaining groups unhandled (...)`. + \ No newline at end of file diff --git a/query-planner-js/src/__tests__/buildPlan.test.ts b/query-planner-js/src/__tests__/buildPlan.test.ts index cf569f4c7..400d4a5ec 100644 --- a/query-planner-js/src/__tests__/buildPlan.test.ts +++ b/query-planner-js/src/__tests__/buildPlan.test.ts @@ -6386,7 +6386,7 @@ test('does not error on some complex fetch group dependencies', () => { v2: V # Note: this field is not queried, but matters to the reproduction this test exists - # for because it prevents some optimisations that would happen without it (namely, + # for because it prevents some optimizations that would happen without it (namely, # without it, the planner would notice that everything after type T is guaranteed # to be local to the subgraph). user: User diff --git a/query-planner-js/src/buildPlan.ts b/query-planner-js/src/buildPlan.ts index c0d413361..5a0a6452c 100644 --- a/query-planner-js/src/buildPlan.ts +++ b/query-planner-js/src/buildPlan.ts @@ -733,6 +733,7 @@ class QueryPlanningTraversal { .closedBranches .slice(0, idxFirstOfLengthOne) .map(b => b.map(opt => PathTree.createFromOpPaths(federatedQueryGraph, root, flattenClosedPath(opt)))); + const { best, cost} = generateAllPlansAndFindBest({ initial: { graph: initialDependencyGraph, tree: initialTree }, toAdd: otherTrees, @@ -795,6 +796,12 @@ class QueryPlanningTraversal { type UnhandledGroups = [FetchGroup, UnhandledParentRelations][]; type UnhandledParentRelations = ParentRelation[]; +function printUnhandled(u: UnhandledGroups): string { + return '[' + u.map(([g, relations]) => + `${g.index} (missing: [${relations.map((r) => r.group.index).join(', ')}])` + ).join(', ') + ']'; +} + /** * Used in `FetchDependencyGraph` to store, for a given group, information about one of its parent. * Namely, this structure stores: @@ -2514,10 +2521,9 @@ class FetchDependencyGraph { const { main, deferredGroups, unhandled } = this.processGroup(processor, group, handledConditions); processedNodes.push(main); allDeferredGroups.addAll(deferredGroups); - const [canHandle, newRemaining] = this.mergeRemainings(remainingNext, unhandled); - toHandleNext = toHandleNext.concat(canHandle); - remainingNext = newRemaining; + remainingNext = this.mergeRemainings(remainingNext, unhandled, toHandleNext); } + return { processed: processInParallel ? processor.reduceParallel(processedNodes) : processor.reduceSequence(processedNodes), next: toHandleNext, @@ -2526,19 +2532,18 @@ class FetchDependencyGraph { }; } - private mergeRemainings(r1: UnhandledGroups, r2: UnhandledGroups): [FetchGroup[], UnhandledGroups] { + private mergeRemainings(r1: UnhandledGroups, r2: UnhandledGroups, readyToHandle: FetchGroup[]): UnhandledGroups { const unhandled: UnhandledGroups = []; - const toHandle: FetchGroup[] = []; for (const [g, edges] of r1) { const newEdges = this.mergeRemaingsAndRemoveIfFound(g, edges, r2); if (newEdges.length == 0) { - toHandle.push(g); + readyToHandle.push(g); } else { unhandled.push([g, newEdges]) } } unhandled.push(...r2); - return [toHandle, unhandled]; + return unhandled; } private mergeRemaingsAndRemoveIfFound(group: FetchGroup, inEdges: UnhandledParentRelations, otherGroups: UnhandledGroups): UnhandledParentRelations { @@ -2586,9 +2591,9 @@ class FetchDependencyGraph { // After the root groups, handled on the first iteration, we can process everything in parallel. processInParallel = true; mainSequence.push(processed); - const [canHandle, newRemaining] = this.mergeRemainings(remainingNext, unhandled); - remainingNext = newRemaining; - nextGroups = canHandle.concat(next); + remainingNext = this.mergeRemainings(remainingNext, unhandled, next); + remainingNext = this.updateUnhandledOnProcessedGroups(nextGroups, remainingNext, next); + nextGroups = next; allDeferredGroups.addAll(deferredGroups); } return { @@ -2598,6 +2603,26 @@ class FetchDependencyGraph { }; } + private updateUnhandledOnProcessedGroups( + processed: readonly FetchGroup[], + currentUnhandled: UnhandledGroups, + readyToHandle: FetchGroup[], + ): UnhandledGroups { + const unhandled: UnhandledGroups = []; + for (const current of currentUnhandled) { + // Remove any of the processed groups from the unhandled edges of that group. + // And if there is no remaining edge, that group can be handled. + const newEdges = current[1].filter((edge) => !processed.includes(edge.group)); + if (newEdges.length === 0) { + readyToHandle.push(current[0]); + } else { + unhandled.push([current[0], newEdges]); + } + } + return unhandled; + } + + private processRootGroups({ processor, rootGroups, @@ -2622,7 +2647,10 @@ class FetchDependencyGraph { unhandled, deferredGroups, } = this.processRootMainGroups({ processor, rootsAreParallel, rootGroups, handledConditions }); - assert(unhandled.length == 0, () => `Root groups ${rootGroups} should have no remaining groups unhandled, but got ${unhandled}`); + assert( + unhandled.length == 0, + () => `Root groups:\n${rootGroups.map((g) => ` - ${g}`).join('\n')}\nshould have no remaining groups unhandled, but got: ${printUnhandled(unhandled)}` + ); const allDeferredGroups = new SetMultiMap(); if (otherDeferGroups) { allDeferredGroups.addAll(otherDeferGroups);