From 71a94a7cfcfb97280a98332e2f05bc39bb8100d0 Mon Sep 17 00:00:00 2001 From: Rob Richard Date: Wed, 22 Jul 2026 10:28:18 -0400 Subject: [PATCH 1/2] Skip deferred fragments in collectFields only when same label is used --- src/execution/__tests__/collectFields-test.ts | 21 +++++ src/execution/collectFields.ts | 57 +++++++++---- .../incremental/__tests__/defer-test.ts | 84 +++++++++++++++++++ 3 files changed, 146 insertions(+), 16 deletions(-) diff --git a/src/execution/__tests__/collectFields-test.ts b/src/execution/__tests__/collectFields-test.ts index 9c9f282f0c..fc1a54fc70 100644 --- a/src/execution/__tests__/collectFields-test.ts +++ b/src/execution/__tests__/collectFields-test.ts @@ -106,5 +106,26 @@ describe('collectFields', () => { expect(fieldDetailsList).to.have.lengthOf(1); }); + + it('prevents infinite loop when deferred fragment is used with same label', () => { + const { newDeferUsages } = collectRootFields(` + query { + ...FragmentOne @defer(label: "Foo") + } + fragment FragmentOne on Query { + ...FragmentTwo @defer(label: "Bar") + field + } + fragment FragmentTwo on Query { + ...FragmentThree @defer(label: "Baz") + } + fragment FragmentThree on Query { + ...FragmentOne @defer(label: "Qux") + field + } + `); + + expect(newDeferUsages).to.have.lengthOf(4); + }); }); }); diff --git a/src/execution/collectFields.ts b/src/execution/collectFields.ts index 98546f032e..85167c26bf 100644 --- a/src/execution/collectFields.ts +++ b/src/execution/collectFields.ts @@ -69,12 +69,19 @@ export interface FragmentDetails { variableSignatures?: ObjMap | undefined; } +interface VisitedFragmentNames { + // Fragments that have been visited without @defer. + immediateFragments: Set; + // Map of fragment name to a set of labels that have been visited with @defer. + deferredFragments: Map>; +} + interface CollectFieldsContext { schema: GraphQLSchema; fragments: ObjMap; variableValues: VariableValues; runtimeType: GraphQLObjectType; - visitedFragmentNames: Map; + visitedFragmentNames: VisitedFragmentNames; hideSuggestions: boolean; forbiddenDirectiveInstances: Array; forbidSkipAndInclude: boolean; @@ -110,7 +117,10 @@ export function collectFields( fragments, variableValues, runtimeType, - visitedFragmentNames: new Map(), + visitedFragmentNames: { + immediateFragments: new Set(), + deferredFragments: new Map(), + }, hideSuggestions, forbiddenDirectiveInstances: [], forbidSkipAndInclude, @@ -151,7 +161,10 @@ export function collectSubfields( fragments, variableValues, runtimeType: returnType, - visitedFragmentNames: new Map(), + visitedFragmentNames: { + immediateFragments: new Set(), + deferredFragments: new Map(), + }, hideSuggestions, forbiddenDirectiveInstances: [], forbidSkipAndInclude: false, @@ -291,26 +304,38 @@ function collectFieldsImpl( deferUsage, ); - const visitedAsDeferred = visitedFragmentNames.get(fragName); + const visitedAsImmediate = + visitedFragmentNames.immediateFragments.has(fragName); + + if (visitedAsImmediate) { + // Even if the fragment is deferred, we don't need to + // collect it again if it has already been visited without @defer. + continue; + } let maybeNewDeferUsage: DeferUsage | undefined; - if (!newDeferUsage) { - // If this spread is not deferred, it may be skipped when already visited - // as a non-deferred spread. If it was previously visited as a deferred spread, - // it must be revisited. - if (visitedAsDeferred === false) { - continue; + + if (newDeferUsage) { + let visitedAsDeferredByLabelSet = + visitedFragmentNames.deferredFragments.get(fragName); + if (!visitedAsDeferredByLabelSet) { + visitedAsDeferredByLabelSet = new Set(); + visitedFragmentNames.deferredFragments.set( + fragName, + visitedAsDeferredByLabelSet, + ); } - visitedFragmentNames.set(fragName, false); - maybeNewDeferUsage = deferUsage; - } else { - // If this spread is deferred, it can be skipped if it has already been visited. - if (visitedAsDeferred !== undefined) { + if (visitedAsDeferredByLabelSet.has(newDeferUsage.label ?? null)) { + // If the fragment has already been visited as deferred by the same + // label, skip it. continue; } - visitedFragmentNames.set(fragName, true); newDeferUsages.push(newDeferUsage); maybeNewDeferUsage = newDeferUsage; + visitedAsDeferredByLabelSet.add(newDeferUsage.label ?? null); + } else { + maybeNewDeferUsage = deferUsage; + visitedFragmentNames.immediateFragments.add(fragName); } const fragmentVariableSignatures = fragment.variableSignatures; diff --git a/src/execution/incremental/__tests__/defer-test.ts b/src/execution/incremental/__tests__/defer-test.ts index 4b9676398c..9de18924ca 100644 --- a/src/execution/incremental/__tests__/defer-test.ts +++ b/src/execution/incremental/__tests__/defer-test.ts @@ -673,6 +673,90 @@ describe('Execute: defer directive', () => { }); }); + it('Can defer same fragment with different labels', async () => { + const document = parse(` + query HeroNameQuery { + hero { + ...TopFragment @defer(label: "DeferTop1") + ...TopFragment @defer(label: "DeferTop2") + } + } + fragment TopFragment on Hero { + name + } + `); + const result = await complete(document); + expectJSON(result).toDeepEqual([ + { + data: { hero: {} }, + pending: [ + { id: '0', path: ['hero'], label: 'DeferTop1' }, + { id: '1', path: ['hero'], label: 'DeferTop2' }, + ], + hasNext: true, + }, + { + hasNext: false, + incremental: [{ id: '0', data: { name: 'Luke' } }], + completed: [{ id: '0' }, { id: '1' }], + }, + ]); + }); + + it('Can skip deferred fragment if same label is used', async () => { + const document = parse(` + query HeroNameQuery { + hero { + ...TopFragment @defer(label: "DeferTop") + ...TopFragment @defer(label: "DeferTop") + } + } + fragment TopFragment on Hero { + name + } + `); + const result = await complete(document); + expectJSON(result).toDeepEqual([ + { + data: { hero: {} }, + pending: [{ id: '0', path: ['hero'], label: 'DeferTop' }], + hasNext: true, + }, + { + hasNext: false, + incremental: [{ id: '0', data: { name: 'Luke' } }], + completed: [{ id: '0' }], + }, + ]); + }); + + it('Can skip deferred fragment if no label is used', async () => { + const document = parse(` + query HeroNameQuery { + hero { + ...TopFragment @defer + ...TopFragment @defer + } + } + fragment TopFragment on Hero { + name + } + `); + const result = await complete(document); + expectJSON(result).toDeepEqual([ + { + data: { hero: {} }, + pending: [{ id: '0', path: ['hero'] }], + hasNext: true, + }, + { + hasNext: false, + incremental: [{ id: '0', data: { name: 'Luke' } }], + completed: [{ id: '0' }], + }, + ]); + }); + it('Can defer an inline fragment', async () => { const document = parse(` query HeroNameQuery { From e22bf04d89a1b7d79abcf9bf38488b2e3b9e15c3 Mon Sep 17 00:00:00 2001 From: Rob Richard Date: Tue, 18 Aug 2026 17:10:22 -0400 Subject: [PATCH 2/2] Skip deferred fragments in collectFields only when enclosed in the same defer directive --- src/execution/__tests__/collectFields-test.ts | 31 ++++--- src/execution/collectFields.ts | 80 +++++++------------ .../incremental/__tests__/defer-test.ts | 58 +++++++------- .../__tests__/legacy-defer-test.ts | 11 +-- 4 files changed, 80 insertions(+), 100 deletions(-) diff --git a/src/execution/__tests__/collectFields-test.ts b/src/execution/__tests__/collectFields-test.ts index fc1a54fc70..4d1874142a 100644 --- a/src/execution/__tests__/collectFields-test.ts +++ b/src/execution/__tests__/collectFields-test.ts @@ -57,24 +57,28 @@ describe('collectFields', () => { expect(newDeferUsages).to.have.lengthOf(0); }); - it('should not collect a deferred spread after a deferred spread has been collected', () => { - const { newDeferUsages } = collectRootFields(` + it('should collect a non-deferred spread after a deferred spread has been collected', () => { + const { groupedFieldSet } = collectRootFields(` query { ...FragmentName @defer - ...FragmentName @defer + ...FragmentName } fragment FragmentName on Query { field } `); - expect(newDeferUsages).to.have.lengthOf(1); + const fieldDetailsList = groupedFieldSet.get('field'); + + invariant(fieldDetailsList != null); + + expect(fieldDetailsList).to.have.lengthOf(2); }); - it('should collect a non-deferred spread after a deferred spread has been collected', () => { + it('should not collect a non-deferred spread after a non-deferred spread has been collected', () => { const { groupedFieldSet } = collectRootFields(` query { - ...FragmentName @defer + ...FragmentName ...FragmentName } fragment FragmentName on Query { @@ -86,20 +90,23 @@ describe('collectFields', () => { invariant(fieldDetailsList != null); - expect(fieldDetailsList).to.have.lengthOf(2); + expect(fieldDetailsList).to.have.lengthOf(1); }); - it('should not collect a non-deferred spread after a non-deferred spread has been collected', () => { - const { groupedFieldSet } = collectRootFields(` + it('should not collect a spread after a spread has been collected in the same defer context', () => { + const { newDeferUsages, groupedFieldSet } = collectRootFields(` query { - ...FragmentName - ...FragmentName + ... @defer { + ...FragmentName + ...FragmentName + } } fragment FragmentName on Query { field } `); + expect(newDeferUsages).to.have.lengthOf(1); const fieldDetailsList = groupedFieldSet.get('field'); invariant(fieldDetailsList != null); @@ -107,7 +114,7 @@ describe('collectFields', () => { expect(fieldDetailsList).to.have.lengthOf(1); }); - it('prevents infinite loop when deferred fragment is used with same label', () => { + it('prevents infinite loops from circular deferred fragment spreads', () => { const { newDeferUsages } = collectRootFields(` query { ...FragmentOne @defer(label: "Foo") diff --git a/src/execution/collectFields.ts b/src/execution/collectFields.ts index 85167c26bf..8a1fb1de46 100644 --- a/src/execution/collectFields.ts +++ b/src/execution/collectFields.ts @@ -25,14 +25,11 @@ import { typeFromAST } from '../utilities/typeFromAST.ts'; import type { GraphQLVariableSignature } from './getVariableSignature.ts'; import type { VariableValues } from './values.ts'; -import { - getArgumentValues, - getDirectiveValues, - getFragmentVariableValues, -} from './values.ts'; +import { getArgumentValues, getFragmentVariableValues } from './values.ts'; /** @internal */ export interface DeferUsage { + directiveNode: DirectiveNode; label: string | undefined; parentDeferUsage: DeferUsage | undefined; } @@ -69,12 +66,7 @@ export interface FragmentDetails { variableSignatures?: ObjMap | undefined; } -interface VisitedFragmentNames { - // Fragments that have been visited without @defer. - immediateFragments: Set; - // Map of fragment name to a set of labels that have been visited with @defer. - deferredFragments: Map>; -} +type VisitedFragmentNames = Map>; interface CollectFieldsContext { schema: GraphQLSchema; @@ -117,10 +109,7 @@ export function collectFields( fragments, variableValues, runtimeType, - visitedFragmentNames: { - immediateFragments: new Set(), - deferredFragments: new Map(), - }, + visitedFragmentNames: new Map(), hideSuggestions, forbiddenDirectiveInstances: [], forbidSkipAndInclude, @@ -161,10 +150,7 @@ export function collectSubfields( fragments, variableValues, runtimeType: returnType, - visitedFragmentNames: { - immediateFragments: new Set(), - deferredFragments: new Map(), - }, + visitedFragmentNames: new Map(), hideSuggestions, forbiddenDirectiveInstances: [], forbidSkipAndInclude: false, @@ -304,40 +290,27 @@ function collectFieldsImpl( deferUsage, ); - const visitedAsImmediate = - visitedFragmentNames.immediateFragments.has(fragName); + const maybeNewDeferUsage = newDeferUsage ?? deferUsage; + let visitedDeferSet = visitedFragmentNames.get(fragName); + if (!visitedDeferSet) { + visitedDeferSet = new Set(); + visitedFragmentNames.set(fragName, visitedDeferSet); + } - if (visitedAsImmediate) { - // Even if the fragment is deferred, we don't need to - // collect it again if it has already been visited without @defer. + if ( + visitedDeferSet.has(null) || + (maybeNewDeferUsage && + visitedDeferSet.has(maybeNewDeferUsage.directiveNode)) + ) { continue; } - let maybeNewDeferUsage: DeferUsage | undefined; - if (newDeferUsage) { - let visitedAsDeferredByLabelSet = - visitedFragmentNames.deferredFragments.get(fragName); - if (!visitedAsDeferredByLabelSet) { - visitedAsDeferredByLabelSet = new Set(); - visitedFragmentNames.deferredFragments.set( - fragName, - visitedAsDeferredByLabelSet, - ); - } - if (visitedAsDeferredByLabelSet.has(newDeferUsage.label ?? null)) { - // If the fragment has already been visited as deferred by the same - // label, skip it. - continue; - } newDeferUsages.push(newDeferUsage); - maybeNewDeferUsage = newDeferUsage; - visitedAsDeferredByLabelSet.add(newDeferUsage.label ?? null); - } else { - maybeNewDeferUsage = deferUsage; - visitedFragmentNames.immediateFragments.add(fragName); } + visitedDeferSet.add(maybeNewDeferUsage?.directiveNode ?? null); + const fragmentVariableSignatures = fragment.variableSignatures; let newFragmentVariableValues: FragmentVariableValues | undefined; if (fragmentVariableSignatures) { @@ -377,23 +350,28 @@ function getDeferUsage( node: FragmentSpreadNode | InlineFragmentNode, parentDeferUsage: DeferUsage | undefined, ): DeferUsage | undefined { - const defer = getDirectiveValues( - GraphQLDeferDirective, - node, - variableValues, - fragmentVariableValues, + const directiveNode = node.directives?.find( + (directive) => directive.name.value === GraphQLDeferDirective.name, ); - if (!defer) { + if (!directiveNode) { return; } + const defer = getArgumentValues( + GraphQLDeferDirective, + directiveNode, + variableValues, + fragmentVariableValues, + ); + if (defer.if === false) { return; } return { label: typeof defer.label === 'string' ? defer.label : undefined, + directiveNode, parentDeferUsage, }; } diff --git a/src/execution/incremental/__tests__/defer-test.ts b/src/execution/incremental/__tests__/defer-test.ts index 9de18924ca..4d373bf070 100644 --- a/src/execution/incremental/__tests__/defer-test.ts +++ b/src/execution/incremental/__tests__/defer-test.ts @@ -703,34 +703,7 @@ describe('Execute: defer directive', () => { ]); }); - it('Can skip deferred fragment if same label is used', async () => { - const document = parse(` - query HeroNameQuery { - hero { - ...TopFragment @defer(label: "DeferTop") - ...TopFragment @defer(label: "DeferTop") - } - } - fragment TopFragment on Hero { - name - } - `); - const result = await complete(document); - expectJSON(result).toDeepEqual([ - { - data: { hero: {} }, - pending: [{ id: '0', path: ['hero'], label: 'DeferTop' }], - hasNext: true, - }, - { - hasNext: false, - incremental: [{ id: '0', data: { name: 'Luke' } }], - completed: [{ id: '0' }], - }, - ]); - }); - - it('Can skip deferred fragment if no label is used', async () => { + it('Can defer same fragment with unlabeled sibling defers', async () => { const document = parse(` query HeroNameQuery { hero { @@ -746,13 +719,16 @@ describe('Execute: defer directive', () => { expectJSON(result).toDeepEqual([ { data: { hero: {} }, - pending: [{ id: '0', path: ['hero'] }], + pending: [ + { id: '0', path: ['hero'] }, + { id: '1', path: ['hero'] }, + ], hasNext: true, }, { hasNext: false, incremental: [{ id: '0', data: { name: 'Luke' } }], - completed: [{ id: '0' }], + completed: [{ id: '0' }, { id: '1' }], }, ]); }); @@ -1030,6 +1006,28 @@ describe('Execute: defer directive', () => { ]); }); + it('Does not skip non-deferred fragments when a fragment is deferred by parent defer', async () => { + const document = parse(` + query HeroNameQuery { + hero { + ... @defer(label: "DeferID") { + ...F + } + ...F + } + } + fragment F on Hero { + name + } + `); + const result = await complete(document); + expectJSON(result).toDeepEqual({ + data: { + hero: { name: 'Luke' }, + }, + }); + }); + it('Separately emits nested defer fragments with varying subfields of same priorities but different level of defers', async () => { const document = parse(` query HeroNameQuery { diff --git a/src/execution/legacyIncremental/__tests__/legacy-defer-test.ts b/src/execution/legacyIncremental/__tests__/legacy-defer-test.ts index 3a8872aef4..bd254b5e4d 100644 --- a/src/execution/legacyIncremental/__tests__/legacy-defer-test.ts +++ b/src/execution/legacyIncremental/__tests__/legacy-defer-test.ts @@ -1185,7 +1185,7 @@ describe('Execute: defer directive (legacy)', () => { }); }); - it('Skips duplicate nested defers on the same object', async () => { + it('Duplicates nested defers on the same object', async () => { const document = parse(` query { hero { @@ -1194,12 +1194,6 @@ describe('Execute: defer directive (legacy)', () => { ...FriendFrag ... @defer { ...FriendFrag - ... @defer { - ...FriendFrag - ... @defer { - ...FriendFrag - } - } } } } @@ -1223,6 +1217,9 @@ describe('Execute: defer directive (legacy)', () => { { data: { id: '2', name: 'Han' }, path: ['hero', 'friends', 0] }, { data: { id: '3', name: 'Leia' }, path: ['hero', 'friends', 1] }, { data: { id: '4', name: 'C-3PO' }, path: ['hero', 'friends', 2] }, + { data: { id: '2', name: 'Han' }, path: ['hero', 'friends', 0] }, + { data: { id: '3', name: 'Leia' }, path: ['hero', 'friends', 1] }, + { data: { id: '4', name: 'C-3PO' }, path: ['hero', 'friends', 2] }, ], hasNext: false, },