Fix fetch group processing handling of dependencies #2622

pcmanus · 2023-06-09T13:19:24Z

The code that process FetchGroup in dependency order has a bug that, for some types of dependencies, can finish with some group not handled, even though they could and should have been.

In practice, this shows itself by failing the post-processing assertion that double-check that no groups are left unhandled.

The error message when this triggered looks like Root groups X should have no remaining groups unhandled (...) (where X is a dump of the fetch groups involved and so this can be large and hard to parse, but basically the assertion error starts with "Root groups").

changeset-bot · 2023-06-09T13:19:27Z

🦋 Changeset detected

Latest commit: 847e95a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages

Name	Type
@apollo/query-planner	Patch
@apollo/gateway	Patch
@apollo/federation-internals	Patch
@apollo/composition	Patch
@apollo/query-graphs	Patch
@apollo/subgraph	Patch
apollo-federation-integration-testsuite	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

netlify · 2023-06-09T13:19:27Z

👷 Deploy request for apollo-federation-docs pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	`847e95a`

codesandbox-ci · 2023-06-09T13:21:02Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

sachindshinde · 2023-06-10T23:28:01Z

query-planner-js/src/buildPlan.ts

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


Bit confused here, is there a reason this

unhandled.push(...r2);

isn't

for (const [g, edges] of r2) { if (edges.length == 0) { readyToHandle.push(g); } else { unhandled.push([g, edges]) } }

instead?

It would seem that if processGroup() returned an unhandled where some of its groups had no edges, then those groups are "ready to handle". Or at least with the current code, since processGroups() loops over multiple/n groups, the last group's zero-edge unhandleds won't be considered "ready to handle", while the previous n-1 groups' will.

You're right this was fishy here. The intend though was that UnhandledGroups would never contain group whose "edges" are empty (both methods that modify it make sure to remove groups as soon as their edges are all empty). But the "bug" was indeed that processGroup could "generate" an UnhandledGroups with "zero-edges" groups and that's where the code was confused. I've addressed in the small refactor I did (it now does genuinely avoid created zero-edge entries in UnhandledGroups, well, at least unless I brain-farted again obviously).

sachindshinde · 2023-06-10T23:48:01Z

query-planner-js/src/buildPlan.ts

-      const [canHandle, newRemaining] = this.mergeRemainings(remainingNext, unhandled);
-      remainingNext = newRemaining;
-      nextGroups = canHandle.concat(next);
+      remainingNext = this.mergeRemainings(remainingNext, unhandled, next);


I'm confused about the purpose of this call to mergeRemainings().

My understanding is:

processGroups() is passed remainingNext.

Along the way, processGroups() repeatedly merges its remainingNext with the unhandleds it receives from processGroup() (moving a group to next whenever it gets down to zero edges). It doesn't modify the original remainingNext, and makes copies instead.

processGroups() returns the new remainingNext along with next.

Then on this line, we merge the old remainingNext with the new remainingNext.

It seems like this merge might add back groups to remainingNext that were transferred over to next in processGroups(), which feels like a bug.

You're right, this was unnecessary (and probably dangerous).

The overall code around this was admittedly a bit confusing, so I did a small refactor to hopefully clear things up a bit. It should be somewhat equivalent to what we have before (save the real bug you rightfully pointed in the previous comment) but I think encapsulate things a tad better/is more explicit. It's probably not perfect, I think this processing code could maybe be cleaned up som more, but I'll leave that for later (started a slightly bigger refactor and messed it up, so figured I should leave that separate from this bug-fixing).

sachindshinde · 2023-06-11T00:14:00Z

query-planner-js/src/buildPlan.ts

+    readyToHandle: FetchGroup[],
+  ): UnhandledGroups {
+    const unhandled: UnhandledGroups = [];
+    for (const current of currentUnhandled) {


minor nit: To be consistent with mergeRemainings()/make code more clear, could you change current to e.g. [g, edges]? (edges is more clear than current[1])

The code that process `FetchGroup` in dependency order has a bug that, for some kind of dependencies, can finish with some group not handled, even though they could and should have been. In practice, this shows itself by failing the post-processing assertion that double-check that no groups are left unhandled.

sachindshinde

Thanks for making the changes! There's some comments below, but nothing really blocking this PR from merging.

sachindshinde · 2023-06-14T01:14:40Z

query-planner-js/src/buildPlan.ts

-    const groupIsOnlyParentOfAllChildren = children.every(g => g.parents().length === 1);
-    if (groupIsOnlyParentOfAllChildren) {
+    const state = ProcessingState.forChildrenOfProcessedGroup(group, children);
+    if (state.next.length > 0) {


Note that there's a logic change here.

Before this change, this if block executes only if all children have exactly one parent. After this change, this if block executes if at least one child has exactly one parent.

This might be fine/intentional, given that you've also swapped out children down below for state.next. Though in that case, you should update the comment below to say e.g. ready children instead of just children.

Yes, sorry for not pointing out that change myself. That was intentional: the old code was more restrictive than made sense and I think I made that initially out of convenience, but it wasn't hard to "fix" that as I refactored, so did it. I did updated the comment though, good point.

sachindshinde · 2023-06-14T01:35:26Z

query-planner-js/src/buildPlan.ts

@@ -2473,86 +2582,55 @@ class FetchDependencyGraph {
        deferredGroups: allDeferredGroups,
      } = this.processRootMainGroups({


Note that because processRootMainGroups() doesn't take the old state/return a new state, presumably when it eventually calls updateForProcessedGroups() for the state it internally creates, it won't alter the separate state in the upstream processGroups() call. This was the case before this PR too though, so it's probably fine to punt that to another PR if you'd like. (It might also be the case this was intentional.)

You know what, you're right. I hesitating changing it and for some reason didn't do it, but there is probably case where that could be a genuine issue, so took the liberty to fix this quickly (it's cleaner if anything anyway).

pcmanus requested a review from a team as a code owner June 9, 2023 13:19

pcmanus force-pushed the fix-fetch-group-processing branch from 87292ae to 4b4e0a4 Compare June 9, 2023 13:20

pcmanus force-pushed the fix-fetch-group-processing branch from 4b4e0a4 to 359f410 Compare June 9, 2023 14:30

sachindshinde requested changes Jun 11, 2023

View reviewed changes

pcmanus force-pushed the fix-fetch-group-processing branch from f975062 to ba2af08 Compare June 13, 2023 08:17

pcmanus requested a review from sachindshinde June 13, 2023 08:17

pcmanus added 2 commits June 13, 2023 17:06

Adds reproduction test

8df64b9

pcmanus force-pushed the fix-fetch-group-processing branch from ba2af08 to 3c5719c Compare June 13, 2023 15:07

sachindshinde approved these changes Jun 14, 2023

View reviewed changes

pcmanus force-pushed the fix-fetch-group-processing branch 3 times, most recently from 7848ca0 to e24b631 Compare June 14, 2023 07:10

Address review feedback through slight refactor

847e95a

pcmanus force-pushed the fix-fetch-group-processing branch from e24b631 to 847e95a Compare June 14, 2023 07:14

pcmanus merged commit 1293034 into apollographql:main Jun 14, 2023

github-actions bot mentioned this pull request Jun 14, 2023

release: on branch main #2625

Merged

o0Ignition0o mentioned this pull request Jun 20, 2023

prep release: v1.21.0 apollographql/router#3280

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix fetch group processing handling of dependencies #2622

Fix fetch group processing handling of dependencies #2622

pcmanus commented Jun 9, 2023

changeset-bot bot commented Jun 9, 2023 •

edited

Loading

netlify bot commented Jun 9, 2023 •

edited

Loading

codesandbox-ci bot commented Jun 9, 2023 •

edited

Loading

sachindshinde Jun 10, 2023 •

edited

Loading

pcmanus Jun 13, 2023

sachindshinde Jun 10, 2023

pcmanus Jun 13, 2023

sachindshinde Jun 11, 2023

sachindshinde left a comment

sachindshinde Jun 14, 2023

pcmanus Jun 14, 2023

sachindshinde Jun 14, 2023

pcmanus Jun 14, 2023

		@@ -2473,86 +2582,55 @@ class FetchDependencyGraph {
		deferredGroups: allDeferredGroups,
		} = this.processRootMainGroups({

Fix fetch group processing handling of dependencies #2622

Fix fetch group processing handling of dependencies #2622

Conversation

pcmanus commented Jun 9, 2023

changeset-bot bot commented Jun 9, 2023 • edited Loading

🦋 Changeset detected

netlify bot commented Jun 9, 2023 • edited Loading

👷 Deploy request for apollo-federation-docs pending review.

codesandbox-ci bot commented Jun 9, 2023 • edited Loading

sachindshinde Jun 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sachindshinde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

changeset-bot bot commented Jun 9, 2023 •

edited

Loading

netlify bot commented Jun 9, 2023 •

edited

Loading

codesandbox-ci bot commented Jun 9, 2023 •

edited

Loading

sachindshinde Jun 10, 2023 •

edited

Loading