Fix potential QP issue with shareable root fields #2239
Conversation
👷 Deploy request for apollo-federation-docs pending review.Visit the deploys page to approve it
|
|
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. |
| @@ -4285,3 +4370,55 @@ describe('__typename handling', () => { | |||
| `); | |||
| }); | |||
| }); | |||
|
|
|||
| describe('mutations', () => { | |||
There was a problem hiding this comment.
Note to reviewer: that test is not really directly related to this patch, but I notice that buildPlan.test.ts had no simple Mutation test. Mutation were still tested elsewhere (in the plan execution tests mostly, but also in plenty of older test files) but as I often use that one test file as a shortcut for "have I broken query planning", it's convenient to have at least a simple mutation test there.
| }; | ||
|
|
||
| function parallelCost(values: number[]): number { | ||
| return sum(values); | ||
| } | ||
|
|
||
| function sequenceCost(stages: number[]): number { | ||
| return stages.reduceRight((acc, stage, idx) => (acc + ((idx + 1) * pipeliningCost * stage)), 0); | ||
| return stages.reduceRight((acc, stage, idx) => (acc + (Math.max(1, idx * pipeliningCost) * stage)), 0); |
There was a problem hiding this comment.
Note to reviewer: this change is due to the fact that the code now more or less rely on the fact that calling processor.reduceSequence on a single element is essentially a no-op, and that was true for the reduceSequence that generates the actual plan, but for not the costing of the plan here. Doesn't really change the overall logic otherwise.
query-planner-js/src/buildPlan.ts
Outdated
| next: toHandleNext, | ||
| unhandled: remainingNext, | ||
| deferredGroups: allDeferredGroups, | ||
| }; | ||
| } | ||
|
|
||
| private mergeRemainings(r1: UnhandledGroups, r2: UnhandledGroups): [FetchGroup[], UnhandledGroups] { | ||
| if (r1.length === 0 && r2.length === 0) { |
There was a problem hiding this comment.
Is this code necessary? Seems like the code would return the same value if r1 and r2 are [] if this if block didn't exist.
There was a problem hiding this comment.
Yeah, that was me prematurely optimising for no reason. I've removed it, it's unnecessary, sorry about the noise.
pcmanus
force-pushed
the
shareable-root-bug
branch
from
61dad92
to
90c9d8c
Compare
Slightly complex to explain, but on some example, during query planning, the query planner had to consider plans whose dependencies where essentially:
but the "processing" code wasn't expecting that and that triggered an assert (the patch has a unit test that, if run against pre-patch
maintriggers that assertion; note that the assertion is thrown during query planning, but the final query plan of that test does not look like the one above). The patch fixes this issue.