diff --git a/src/execution/__tests__/collectFields-test.ts b/src/execution/__tests__/collectFields-test.ts index 9c9f282f0c..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,25 +90,49 @@ 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); expect(fieldDetailsList).to.have.lengthOf(1); }); + + it('prevents infinite loops from circular deferred fragment spreads', () => { + 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..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,14 @@ export interface FragmentDetails { variableSignatures?: ObjMap | undefined; } +type VisitedFragmentNames = Map>; + interface CollectFieldsContext { schema: GraphQLSchema; fragments: ObjMap; variableValues: VariableValues; runtimeType: GraphQLObjectType; - visitedFragmentNames: Map; + visitedFragmentNames: VisitedFragmentNames; hideSuggestions: boolean; forbiddenDirectiveInstances: Array; forbidSkipAndInclude: boolean; @@ -291,28 +290,27 @@ function collectFieldsImpl( deferUsage, ); - const visitedAsDeferred = visitedFragmentNames.get(fragName); + const maybeNewDeferUsage = newDeferUsage ?? deferUsage; + let visitedDeferSet = visitedFragmentNames.get(fragName); + if (!visitedDeferSet) { + visitedDeferSet = new Set(); + visitedFragmentNames.set(fragName, visitedDeferSet); + } - 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; - } - 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) { - continue; - } - visitedFragmentNames.set(fragName, true); + if ( + visitedDeferSet.has(null) || + (maybeNewDeferUsage && + visitedDeferSet.has(maybeNewDeferUsage.directiveNode)) + ) { + continue; + } + + if (newDeferUsage) { newDeferUsages.push(newDeferUsage); - maybeNewDeferUsage = newDeferUsage; } + visitedDeferSet.add(maybeNewDeferUsage?.directiveNode ?? null); + const fragmentVariableSignatures = fragment.variableSignatures; let newFragmentVariableValues: FragmentVariableValues | undefined; if (fragmentVariableSignatures) { @@ -352,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 4b9676398c..4d373bf070 100644 --- a/src/execution/incremental/__tests__/defer-test.ts +++ b/src/execution/incremental/__tests__/defer-test.ts @@ -673,6 +673,66 @@ 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 defer same fragment with unlabeled sibling defers', 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'] }, + { id: '1', path: ['hero'] }, + ], + hasNext: true, + }, + { + hasNext: false, + incremental: [{ id: '0', data: { name: 'Luke' } }], + completed: [{ id: '0' }, { id: '1' }], + }, + ]); + }); + it('Can defer an inline fragment', async () => { const document = parse(` query HeroNameQuery { @@ -946,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, },