avoid re-visiting deferred fragment spreads (#4462)

This PR changes the fragment spread visiting logic to avoid re-visiting fragment spreads in additional cases.

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: graphql/graphql-spec#1045 (comment)

Author: yaacovCR

src/execution/__tests__/collectFields-test.ts

@@ -0,0 +1,109 @@
+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);
+    });
+
+    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);
+    });
+  });
+});

src/execution/collectFields.ts

@@ -67,7 +67,7 @@ interface CollectFieldsContext {
   fragments: ObjMap<FragmentDetails>;
   variableValues: VariableValues;
   runtimeType: GraphQLObjectType;
-  visitedFragmentNames: Set<string>;
+  visitedFragmentNames: Map<string, boolean>;
   hideSuggestions: boolean;
   forbiddenDirectiveInstances: Array<DirectiveNode>;
   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,28 @@ function collectFieldsImpl(
           deferUsage,
         );
 
+        const visitedAsDeferred = visitedFragmentNames.get(fragName);
+
+        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);
+          newDeferUsages.push(newDeferUsage);
+          maybeNewDeferUsage = newDeferUsage;
+        }
+
         const fragmentVariableSignatures = fragment.variableSignatures;
         let newFragmentVariableValues: FragmentVariableValues | undefined;
         if (fragmentVariableSignatures) {
@@ -296,27 +317,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;
       }
     }