fixes

1. use throwIfAborted 2. use correct path when aborting for a fieldPath within executeFields 3. polish tests to throw a proper Error as the reason
yaacovCR · Oct 23, 2024 · f05fc11 · f05fc11
1 parent ae108b2
commit f05fc11
Show file tree

Hide file tree

Showing 2 changed files with 39 additions and 31 deletions.
diff --git a/src/execution/__tests__/abort-signal-test.ts b/src/execution/__tests__/abort-signal-test.ts
@@ -61,7 +61,7 @@ const schema = buildSchema(`
 `);
 
 describe('Execute: Cancellation', () => {
-  it('should stop the execution when aborted', async () => {
+  it('should stop the execution when aborted during object field completion', async () => {
     const abortController = new AbortController();
     const document = parse(`
       query {
@@ -89,7 +89,7 @@ describe('Execute: Cancellation', () => {
       },
     });
 
-    abortController.abort('Aborted');
+    abortController.abort(new Error('Aborted'));
 
     const result = await resultPromise;
 
@@ -100,8 +100,8 @@ describe('Execute: Cancellation', () => {
       errors: [
         {
           message: 'Aborted',
-          path: ['todo'],
-          locations: [{ line: 3, column: 9 }],
+          path: ['todo', 'id'],
+          locations: [{ line: 4, column: 11 }],
         },
       ],
     });
@@ -135,7 +135,7 @@ describe('Execute: Cancellation', () => {
       },
     });
 
-    abortController.abort('Aborted');
+    abortController.abort(new Error('Aborted'));
 
     const result = await resultPromise;
 
@@ -149,8 +149,8 @@ describe('Execute: Cancellation', () => {
       errors: [
         {
           message: 'Aborted',
-          path: ['todo', 'author'],
-          locations: [{ line: 5, column: 11 }],
+          path: ['todo', 'author', 'id'],
+          locations: [{ line: 6, column: 13 }],
         },
       ],
     });
@@ -165,9 +165,7 @@ describe('Execute: Cancellation', () => {
           ... on Todo @defer {
             text
             author {
-              ... on Author @defer {
-                id
-              }
+              id
             }
           }
         }
@@ -189,7 +187,7 @@ describe('Execute: Cancellation', () => {
       abortSignal: abortController.signal,
     });
 
-    abortController.abort('Aborted');
+    abortController.abort(new Error('Aborted'));
 
     const result = await resultPromise;
 
@@ -200,8 +198,8 @@ describe('Execute: Cancellation', () => {
       errors: [
         {
           message: 'Aborted',
-          path: ['todo'],
-          locations: [{ line: 3, column: 9 }],
+          path: ['todo', 'id'],
+          locations: [{ line: 4, column: 11 }],
         },
       ],
     });
@@ -216,9 +214,7 @@ describe('Execute: Cancellation', () => {
           ... on Todo @defer {
             text
             author {
-              ... on Author @defer {
-                id
-              }
+              id
             }
           }
         }
@@ -232,9 +228,8 @@ describe('Execute: Cancellation', () => {
           Promise.resolve({
             id: '1',
             text: 'hello world',
-            /* c8 ignore next 2 */
-            author: async () =>
-              Promise.resolve(() => expect.fail('Should not be called')),
+            /* c8 ignore next */
+            author: () => expect.fail('Should not be called'),
           }),
       },
       abortController.signal,
@@ -244,7 +239,7 @@ describe('Execute: Cancellation', () => {
     await resolveOnNextTick();
     await resolveOnNextTick();
 
-    abortController.abort('Aborted');
+    abortController.abort(new Error('Aborted'));
 
     const result = await resultPromise;
 
@@ -264,6 +259,8 @@ describe('Execute: Cancellation', () => {
             errors: [
               {
                 message: 'Aborted',
+                path: ['todo', 'text'],
+                locations: [{ line: 6, column: 13 }],
               },
             ],
             id: '0',
@@ -294,7 +291,7 @@ describe('Execute: Cancellation', () => {
       },
     });
 
-    abortController.abort('Aborted');
+    abortController.abort(new Error('Aborted'));
 
     const result = await resultPromise;
 
@@ -325,7 +322,9 @@ describe('Execute: Cancellation', () => {
         }
       }
     `);
-    abortController.abort('Aborted');
+
+    abortController.abort(new Error('Aborted'));
+
     const result = await execute({
       document,
       schema,

diff --git a/src/execution/execute.ts b/src/execution/execute.ts
@@ -517,8 +517,10 @@ export function validateExecutionArgs(
     abortSignal,
   } = args;
 
-  if (abortSignal?.aborted) {
-    return [new GraphQLError(abortSignal.reason)];
+  try {
+    abortSignal?.throwIfAborted();
+  } catch (rawError: unknown) {
+    return [locatedError(rawError, undefined)];
   }
 
   // If the schema used for execution is invalid, throw an error.
@@ -665,10 +667,11 @@ function executeFieldsSerially(
     groupedFieldSet,
     (graphqlWrappedResult, [responseName, fieldDetailsList]) => {
       const fieldPath = addPath(path, responseName, parentType.name);
-      const abortSignal = exeContext.validatedExecutionArgs.abortSignal;
-      if (abortSignal?.aborted) {
+      try {
+        exeContext.validatedExecutionArgs.abortSignal?.throwIfAborted();
+      } catch (rawError) {
         handleFieldError(
-          new Error(abortSignal.reason),
+          rawError,
           exeContext,
           parentType,
           fieldDetailsList,
@@ -728,12 +731,18 @@ function executeFields(
 
   try {
     for (const [responseName, fieldDetailsList] of groupedFieldSet) {
-      const abortSignal = exeContext.validatedExecutionArgs.abortSignal;
-      if (abortSignal?.aborted) {
-        throw new GraphQLError(abortSignal.reason);
+      const fieldPath = addPath(path, responseName, parentType.name);
+
+      try {
+        exeContext.validatedExecutionArgs.abortSignal?.throwIfAborted();
+      } catch (rawError) {
+        throw locatedError(
+          rawError,
+          toNodes(fieldDetailsList),
+          pathToArray(fieldPath),
+        );
       }
 
-      const fieldPath = addPath(path, responseName, parentType.name);
       const result = executeField(
         exeContext,
         parentType,