From ea03a7514ef462d8295ef39dd15a883745198c61 Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Tue, 22 Jul 2025 23:22:08 +0300 Subject: [PATCH 1/3] avoid re-visiting deferred fragment spreads This PR changes the fragment spread visiting logic to avoid re-visiting fragment spreads whenever possible. Previously, fragment spreads that have been visited in a non-deferred capacity were not revisited, but fragment spreads that had been visited in a deferred capacity could be revisited. This PR updates the logic so that fragment spreads that had been visited in a deferred capacity are not revisited in a deferred capacity. See https://github.com/graphql/graphql-spec/pull/1045#issuecomment-3085263996 --- src/execution/__tests__/collectFields-test.ts | 91 +++++++++++++++++++ src/execution/collectFields.ts | 54 ++++++----- 2 files changed, 120 insertions(+), 25 deletions(-) create mode 100644 src/execution/__tests__/collectFields-test.ts diff --git a/src/execution/__tests__/collectFields-test.ts b/src/execution/__tests__/collectFields-test.ts new file mode 100644 index 0000000000..43d75e156b --- /dev/null +++ b/src/execution/__tests__/collectFields-test.ts @@ -0,0 +1,91 @@ +import { expect } from 'chai'; +import { describe, it } from 'mocha'; + +import { invariant } from '../../jsutils/invariant.js'; + +import { parse } from '../../language/parser.js'; + +import { buildSchema } from '../../utilities/buildASTSchema.js'; + +import { collectFields } from '../collectFields.js'; +import { validateExecutionArgs } from '../execute.js'; + +const schema = buildSchema(` + type Query { + field: String + } +`); + +function collectRootFields(query: string) { + const validatedExecutionArgs = validateExecutionArgs({ + schema, + document: parse(query), + }); + + invariant('operation' in validatedExecutionArgs); + + const { operation, fragments, variableValues } = validatedExecutionArgs; + + const queryType = schema.getQueryType(); + + invariant(queryType != null); + + return collectFields( + schema, + fragments, + variableValues, + queryType, + operation.selectionSet, + false, + ); +} + +describe('collectFields', () => { + describe('overlapping fragment spreads', () => { + it('should not collect a deferred spread after a non-deferred spread has been collected', () => { + const { newDeferUsages } = collectRootFields(` + query { + ...FragmentName + ...FragmentName @defer + } + fragment FragmentName on Query { + field + } + `); + + expect(newDeferUsages).to.have.lengthOf(0); + }); + + it('should not collect a deferred spread after a deferred spread has been collected', () => { + const { newDeferUsages } = collectRootFields(` + query { + ...FragmentName @defer + ...FragmentName @defer + } + fragment FragmentName on Query { + field + } + `); + + expect(newDeferUsages).to.have.lengthOf(1); + }); + + it('should collect a non-deferred spread after a deferred spread has been collected', () => { + const { groupedFieldSet } = collectRootFields(` + query { + ...FragmentName @defer + ...FragmentName + } + fragment FragmentName on Query { + field + } + `); + + const fieldDetailsList = groupedFieldSet.get('field'); + + invariant(fieldDetailsList != null); + + expect(fieldDetailsList).to.have.lengthOf(2); + }); + }); +}); diff --git a/src/execution/collectFields.ts b/src/execution/collectFields.ts index 26367387f3..fd0910f2fe 100644 --- a/src/execution/collectFields.ts +++ b/src/execution/collectFields.ts @@ -67,7 +67,7 @@ interface CollectFieldsContext { fragments: ObjMap; variableValues: VariableValues; runtimeType: GraphQLObjectType; - visitedFragmentNames: Set; + visitedFragmentNames: Map; hideSuggestions: boolean; forbiddenDirectiveInstances: Array; forbidSkipAndInclude: boolean; @@ -103,7 +103,7 @@ export function collectFields( fragments, variableValues, runtimeType, - visitedFragmentNames: new Set(), + visitedFragmentNames: new Map(), hideSuggestions, forbiddenDirectiveInstances: [], forbidSkipAndInclude, @@ -144,7 +144,7 @@ export function collectSubfields( fragments, variableValues, runtimeType: returnType, - visitedFragmentNames: new Set(), + visitedFragmentNames: new Map(), hideSuggestions, forbiddenDirectiveInstances: [], forbidSkipAndInclude: false, @@ -258,7 +258,6 @@ function collectFieldsImpl( const fragName = selection.name.value; if ( - visitedFragmentNames.has(fragName) || !shouldIncludeNode( context, selection, @@ -284,6 +283,24 @@ function collectFieldsImpl( deferUsage, ); + const visitedStatus = visitedFragmentNames.get(fragName); + + let maybeNewDeferUsage: DeferUsage | undefined; + if (!newDeferUsage) { + if (visitedStatus === false) { + continue; + } + visitedFragmentNames.set(fragName, false); + maybeNewDeferUsage = deferUsage; + } else { + if (visitedStatus !== undefined) { + continue; + } + visitedFragmentNames.set(fragName, true); + newDeferUsages.push(newDeferUsage); + maybeNewDeferUsage = newDeferUsage; + } + const fragmentVariableSignatures = fragment.variableSignatures; let newFragmentVariableValues: FragmentVariableValues | undefined; if (fragmentVariableSignatures) { @@ -296,27 +313,14 @@ function collectFieldsImpl( ); } - if (!newDeferUsage) { - visitedFragmentNames.add(fragName); - collectFieldsImpl( - context, - fragment.definition.selectionSet, - groupedFieldSet, - newDeferUsages, - deferUsage, - newFragmentVariableValues, - ); - } else { - newDeferUsages.push(newDeferUsage); - collectFieldsImpl( - context, - fragment.definition.selectionSet, - groupedFieldSet, - newDeferUsages, - newDeferUsage, - newFragmentVariableValues, - ); - } + collectFieldsImpl( + context, + fragment.definition.selectionSet, + groupedFieldSet, + newDeferUsages, + maybeNewDeferUsage, + newFragmentVariableValues, + ); break; } } From 092549b9570850b4c2bd059aab1b16c08b93249e Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Tue, 22 Jul 2025 23:28:50 +0300 Subject: [PATCH 2/3] add additional test case --- src/execution/__tests__/collectFields-test.ts | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/execution/__tests__/collectFields-test.ts b/src/execution/__tests__/collectFields-test.ts index 43d75e156b..245d8424fd 100644 --- a/src/execution/__tests__/collectFields-test.ts +++ b/src/execution/__tests__/collectFields-test.ts @@ -87,5 +87,23 @@ describe('collectFields', () => { expect(fieldDetailsList).to.have.lengthOf(2); }); + + it('should not collect a non-deferred spread after a non-deferred spread has been collected', () => { + const { groupedFieldSet } = collectRootFields(` + query { + ...FragmentName + ...FragmentName + } + fragment FragmentName on Query { + field + } + `); + + const fieldDetailsList = groupedFieldSet.get('field'); + + invariant(fieldDetailsList != null); + + expect(fieldDetailsList).to.have.lengthOf(1); + }); }); }); From 7fc5050c430ee4c61941a191f1609f46b267e561 Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Thu, 24 Jul 2025 20:13:12 +0300 Subject: [PATCH 3/3] add comments --- src/execution/collectFields.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/execution/collectFields.ts b/src/execution/collectFields.ts index fd0910f2fe..2c166296f4 100644 --- a/src/execution/collectFields.ts +++ b/src/execution/collectFields.ts @@ -283,17 +283,21 @@ function collectFieldsImpl( deferUsage, ); - const visitedStatus = visitedFragmentNames.get(fragName); + const visitedAsDeferred = visitedFragmentNames.get(fragName); let maybeNewDeferUsage: DeferUsage | undefined; if (!newDeferUsage) { - if (visitedStatus === false) { + // 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 (visitedStatus !== undefined) { + // If this spread is deferred, it can be skipped if it has already been visited. + if (visitedAsDeferred !== undefined) { continue; } visitedFragmentNames.set(fragName, true);