Fix dataloader dispatch in @defer when multiple deferred fragments exist#4270
Open
timward60 wants to merge 2 commits intographql-java:masterfrom
Open
Fix dataloader dispatch in @defer when multiple deferred fragments exist#4270timward60 wants to merge 2 commits intographql-java:masterfrom
timward60 wants to merge 2 commits intographql-java:masterfrom
Conversation
Add two test cases to DeferWithDataLoaderTest: - Multiple separate @defer fragments with dataloader fields - Mixed deferred and non-deferred fields with dataloaders Both tests verify dispatch works correctly across all three dispatch strategy modes. Without the corresponding fix, the first test times out because dataloaders are never dispatched. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use mergedFields.size() (per-fragment field count) instead of deferredFields.size() (total across all fragments) when constructing AlternativeCallContext. The inflated count prevented dispatch strategies from ever triggering dispatch since each fragment only fetches its own fields. Fixes the bug introduced in graphql-java#3980. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Title
Fix dataloader dispatch in @defer when multiple deferred fragments exist (#4269)
Description
Problem
When a query contains multiple
@deferfragments at the same level, dataloaders invoked inside those deferred fragments are never dispatched. The deferred payloads hang until the request times out.This was introduced in #3980 ("make dataloader work inside defer blocks").
Root Cause
In
DeferredExecutionSupportImpl.createDeferredFragmentCall(), theAlternativeCallContextis constructed withdeferredFields.size()— the total field count across all@deferfragments — instead of the field count for the specific fragment:Each
DeferredFragmentCallonly invokes its own fields, so the fetch counter will only reach the count of fields in that fragment, never the inflated total. BothPerLevelDataLoaderDispatchStrategy.fieldFetched()andExhaustedDataLoaderDispatchStrategy.deferFieldFetched()compare against this inflated count to decide when to dispatch, so dispatch never triggers.Example: 2
@deferfragments with 1 field each →expectedFirstLevelFetchCount= 2, but each fragment only fetches 1 field → dispatch condition1 == 2is never satisfied → dataloaders hang.Fix
Pass
mergedFields.size()(the per-fragment field count) instead ofdeferredFields.size():private DeferredFragmentCall createDeferredFragmentCall(DeferredExecution deferredExecution) { int level = parameters.getPath().getLevel() + 1; - AlternativeCallContext alternativeCallContext = new AlternativeCallContext(level, deferredFields.size()); - - List<MergedField> mergedFields = deferredExecutionToFields.get(deferredExecution); + List<MergedField> mergedFields = deferredExecutionToFields.get(deferredExecution); + AlternativeCallContext alternativeCallContext = new AlternativeCallContext(level, mergedFields.size());This is safe for mixed deferred/non-deferred selection sets because the constructor already partitions fields — non-deferred fields are routed into
nonDeferredFieldNamesand never added todeferredExecutionToFields.Tests Added
Two new test cases in
DeferWithDataLoaderTest:multiple separate defer fragments with dataloader fields dispatch correctly— Two separate@deferblocks (departmentsandexpensiveDepartments) onshops, each backed by dataloaders. Without the fix, all 3 dispatch strategy variants time out at ~10s each. With the fix, they complete in ~1s total.mixed deferred and non-deferred fields with dataloaders dispatch correctly— Non-deferreddepartmentsalongside a single deferredexpensiveDepartments, confirming the fix correctly handles mixed selection sets.Both tests cover all three dispatch strategy modes: default (
PerLevel),ENABLE_DATA_LOADER_CHAINING, andENABLE_DATA_LOADER_EXHAUSTED_DISPATCHING.