add new deferred fragments to the wrapped graphql result (#4358)

alternative solution to #4357 in which we add the new deferred fragments to the wrapped graphql result

After an execution group "fails," i.e. a null has bubbled up too far, it fails the entire associated deferred fragment. But if the query includes a shared execution group between the failing deferred fragment and a non-failing deferred fragment with some unique subfields, those subfields will still be added to the graph for the failed deferred fragment, which is not present.

This throws an error, because the code assumes that in the situation above, i.e. a shared execution group between two fragments and some unique subfields, both of the deferred fragments must already be in the graph. So a runtime-error is triggered because some of the invariants we rely on when adding deferred fragments are violated.

But even if we didn't have an error, this would be an erroneous situation, as we should never add back a node to the graph that has been failed!

After this change, new deferred fragments are explicitly returned as part of the GraphQLWrappedResult and are only added at a single point when building the graph such that this error can be avoided entirely.

Author: yaacovCR

src/execution/IncrementalGraph.ts

@@ -35,9 +35,22 @@ export class IncrementalGraph {
   }
 
   getNewRootNodes(
+    newDeferredFragmentRecords:
+      | ReadonlyArray<DeferredFragmentRecord>
+      | undefined,
     incrementalDataRecords: ReadonlyArray<IncrementalDataRecord>,
   ): ReadonlyArray<DeliveryGroup> {
     const initialResultChildren = new Set<DeliveryGroup>();
+
+    if (newDeferredFragmentRecords !== undefined) {
+      for (const deferredFragmentRecord of newDeferredFragmentRecords) {
+        this._addDeferredFragment(
+          deferredFragmentRecord,
+          initialResultChildren,
+        );
+      }
+    }
+
     this._addIncrementalDataRecords(
       incrementalDataRecords,
       undefined,
@@ -49,8 +62,17 @@ export class IncrementalGraph {
   addCompletedSuccessfulExecutionGroup(
     successfulExecutionGroup: SuccessfulExecutionGroup,
   ): void {
-    const { pendingExecutionGroup, incrementalDataRecords } =
-      successfulExecutionGroup;
+    const {
+      pendingExecutionGroup,
+      newDeferredFragmentRecords,
+      incrementalDataRecords,
+    } = successfulExecutionGroup;
+
+    if (newDeferredFragmentRecords !== undefined) {
+      for (const deferredFragmentRecord of newDeferredFragmentRecords) {
+        this._addDeferredFragment(deferredFragmentRecord, undefined);
+      }
+    }
 
     const deferredFragmentRecords =
       pendingExecutionGroup.deferredFragmentRecords;
@@ -135,11 +157,7 @@ export class IncrementalGraph {
   removeDeferredFragment(
     deferredFragmentRecord: DeferredFragmentRecord,
   ): boolean {
-    if (!this._rootNodes.has(deferredFragmentRecord)) {
-      return false;
-    }
-    this._rootNodes.delete(deferredFragmentRecord);
-    return true;
+    return this._rootNodes.delete(deferredFragmentRecord);
   }
 
   removeStream(streamRecord: StreamRecord): void {
@@ -154,10 +172,6 @@ export class IncrementalGraph {
     for (const incrementalDataRecord of incrementalDataRecords) {
       if (isPendingExecutionGroup(incrementalDataRecord)) {
         for (const deferredFragmentRecord of incrementalDataRecord.deferredFragmentRecords) {
-          this._addDeferredFragment(
-            deferredFragmentRecord,
-            initialResultChildren,
-          );
           deferredFragmentRecord.pendingExecutionGroups.add(
             incrementalDataRecord,
           );
@@ -170,7 +184,6 @@ export class IncrementalGraph {
         initialResultChildren.add(incrementalDataRecord);
       } else {
         for (const parent of parents) {
-          this._addDeferredFragment(parent, initialResultChildren);
           parent.children.add(incrementalDataRecord);
         }
       }
@@ -219,17 +232,13 @@ export class IncrementalGraph {
     deferredFragmentRecord: DeferredFragmentRecord,
     initialResultChildren: Set<DeliveryGroup> | undefined,
   ): void {
-    if (this._rootNodes.has(deferredFragmentRecord)) {
-      return;
-    }
     const parent = deferredFragmentRecord.parent;
     if (parent === undefined) {
       invariant(initialResultChildren !== undefined);
       initialResultChildren.add(deferredFragmentRecord);
       return;
     }
     parent.children.add(deferredFragmentRecord);
-    this._addDeferredFragment(parent, initialResultChildren);
   }
 
   private _onExecutionGroup(
@@ -251,6 +260,7 @@ export class IncrementalGraph {
   private async _onStreamItems(streamRecord: StreamRecord): Promise<void> {
     let items: Array<unknown> = [];
     let errors: Array<GraphQLError> = [];
+    let newDeferredFragmentRecords: Array<DeferredFragmentRecord> = [];
     let incrementalDataRecords: Array<IncrementalDataRecord> = [];
     const streamItemQueue = streamRecord.streamItemQueue;
     let streamItemRecord: StreamItemRecord | undefined;
@@ -268,10 +278,12 @@ export class IncrementalGraph {
               errors.length > 0 /* c8 ignore start */
                 ? { items, errors } /* c8 ignore stop */
                 : { items },
+            newDeferredFragmentRecords,
             incrementalDataRecords,
           });
           items = [];
           errors = [];
+          newDeferredFragmentRecords = [];
           incrementalDataRecords = [];
         }
         // eslint-disable-next-line no-await-in-loop
@@ -286,6 +298,7 @@ export class IncrementalGraph {
           this._enqueue({
             streamRecord,
             result: errors.length > 0 ? { items, errors } : { items },
+            newDeferredFragmentRecords,
             incrementalDataRecords,
           });
         }
@@ -303,6 +316,9 @@ export class IncrementalGraph {
       if (result.errors !== undefined) {
         errors.push(...result.errors);
       }
+      if (result.newDeferredFragmentRecords !== undefined) {
+        newDeferredFragmentRecords.push(...result.newDeferredFragmentRecords);
+      }
       if (result.incrementalDataRecords !== undefined) {
         incrementalDataRecords.push(...result.incrementalDataRecords);
       }

src/execution/IncrementalPublisher.ts

@@ -33,12 +33,14 @@ export function buildIncrementalResponse(
   context: IncrementalPublisherContext,
   result: ObjMap<unknown>,
   errors: ReadonlyArray<GraphQLError> | undefined,
+  newDeferredFragmentRecords: ReadonlyArray<DeferredFragmentRecord> | undefined,
   incrementalDataRecords: ReadonlyArray<IncrementalDataRecord>,
 ): ExperimentalIncrementalExecutionResults {
   const incrementalPublisher = new IncrementalPublisher(context);
   return incrementalPublisher.buildResponse(
     result,
     errors,
+    newDeferredFragmentRecords,
     incrementalDataRecords,
   );
 }
@@ -74,9 +76,13 @@ class IncrementalPublisher {
   buildResponse(
     data: ObjMap<unknown>,
     errors: ReadonlyArray<GraphQLError> | undefined,
+    newDeferredFragmentRecords:
+      | ReadonlyArray<DeferredFragmentRecord>
+      | undefined,
     incrementalDataRecords: ReadonlyArray<IncrementalDataRecord>,
   ): ExperimentalIncrementalExecutionResults {
     const newRootNodes = this._incrementalGraph.getNewRootNodes(
+      newDeferredFragmentRecords,
       incrementalDataRecords,
     );
 
@@ -231,18 +237,16 @@ class IncrementalPublisher {
     if (isFailedExecutionGroup(completedExecutionGroup)) {
       for (const deferredFragmentRecord of completedExecutionGroup
         .pendingExecutionGroup.deferredFragmentRecords) {
-        const id = deferredFragmentRecord.id;
         if (
-          !this._incrementalGraph.removeDeferredFragment(deferredFragmentRecord)
+          this._incrementalGraph.removeDeferredFragment(deferredFragmentRecord)
         ) {
-          // This can occur if multiple deferred grouped field sets error for a fragment.
-          continue;
+          const id = deferredFragmentRecord.id;
+          invariant(id !== undefined);
+          context.completed.push({
+            id,
+            errors: completedExecutionGroup.errors,
+          });
         }
-        invariant(id !== undefined);
-        context.completed.push({
-          id,
-          errors: completedExecutionGroup.errors,
-        });
       }
       return;
     }
@@ -319,9 +323,11 @@ class IncrementalPublisher {
 
       context.incremental.push(incrementalEntry);
 
-      const incrementalDataRecords = streamItemsResult.incrementalDataRecords;
+      const { newDeferredFragmentRecords, incrementalDataRecords } =
+        streamItemsResult;
       if (incrementalDataRecords !== undefined) {
         const newRootNodes = this._incrementalGraph.getNewRootNodes(
+          newDeferredFragmentRecords,
           incrementalDataRecords,
         );
         context.pending.push(...this._toPendingResults(newRootNodes));