Skip deferred fragments in collectFields only when same directive is used

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);
+    });
   });
 });

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<GraphQLVariableSignature> | undefined;
 }
 
+type VisitedFragmentNames = Map<string, Set<DirectiveNode | null>>;
+
 interface CollectFieldsContext {
   schema: GraphQLSchema;
   fragments: ObjMap<FragmentDetails>;
   variableValues: VariableValues;
   runtimeType: GraphQLObjectType;
-  visitedFragmentNames: Map<string, boolean>;
+  visitedFragmentNames: VisitedFragmentNames;
   hideSuggestions: boolean;
   forbiddenDirectiveInstances: Array<DirectiveNode>;
   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,
   };
 }

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 {

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,
       },