Improve error messages when before/after hooks throw (#6239)

Author: timleslie

.changeset/olive-maps-serve.md

@@ -0,0 +1,5 @@
+---
+'@keystone-next/keystone': patch
+---
+
+Added more details to before/after change/delete hook error messages.

packages/keystone/src/lib/core/graphql-errors.ts

@@ -7,6 +7,11 @@ export const validationFailureError = (messages: string[]) => {
   return new ApolloError(`You provided invalid data for this operation.\n${s}`);
 };
 
+export const extensionError = (extension: string, messages: string[]) => {
+  const s = messages.map(m => `  - ${m}`).join('\n');
+  return new ApolloError(`An error occured while running "${extension}".\n${s}`);
+};
+
 // FIXME: In an upcoming PR we will use these args to construct a better
 // error message, so leaving the, here for now. - TL
 // eslint-disable-next-line @typescript-eslint/no-unused-vars

packages/keystone/src/lib/core/mutations/hooks.ts

@@ -1,4 +1,4 @@
-import { promiseAllRejectWithAllErrors } from '../utils';
+import { extensionError } from '../graphql-errors';
 
 export async function runSideEffectOnlyHook<
   HookName extends string,
@@ -14,6 +14,7 @@ export async function runSideEffectOnlyHook<
     hooks: {
       [Key in HookName]?: (args: any) => Promise<void> | void;
     };
+    listKey: string;
   },
   Args extends Parameters<NonNullable<List['hooks'][HookName]>>[0]
 >(list: List, hookName: HookName, args: Args) {
@@ -30,14 +31,25 @@ export async function runSideEffectOnlyHook<
   }
 
   // Field hooks
-  await promiseAllRejectWithAllErrors(
-    Object.entries(list.fields).map(async ([fieldKey, field]) => {
-      if (shouldRunFieldLevelHook(fieldKey)) {
-        await field.hooks[hookName]?.({ fieldPath: fieldKey, ...args });
+  const fieldsErrors = [];
+  for (const [fieldPath, field] of Object.entries(list.fields)) {
+    if (shouldRunFieldLevelHook(fieldPath)) {
+      try {
+        // @ts-ignore
+        await field.hooks[hookName]?.({ fieldPath, ...args });
+      } catch (err) {
+        fieldsErrors.push(`${list.listKey}.${fieldPath}: ${err.message}`);
       }
-    })
-  );
+    }
+  }
+  if (fieldsErrors.length) {
+    throw extensionError(hookName, fieldsErrors);
+  }
 
   // List hooks
-  await list.hooks[hookName]?.(args);
+  try {
+    await list.hooks[hookName]?.(args);
+  } catch (err) {
+    throw extensionError(hookName, [`${list.listKey}: ${err.message}`]);
+  }
 }

tests/api-tests/hooks/hook-errors.test.ts

@@ -103,8 +103,8 @@ const runner = setupTestRunner({
 
         // Returns null and throws an error
         expect(data).toEqual({ createUser: null });
-        expectExtensionError(errors, [
-          { path: ['createUser'], message: `Simulated error: ${phase}Change` },
+        expectExtensionError(errors, `${phase}Change`, [
+          { path: ['createUser'], messages: [`User: Simulated error: ${phase}Change`] },
         ]);
 
         // Only the original user should exist for 'before', both exist for 'after'
@@ -130,8 +130,8 @@ const runner = setupTestRunner({
 
         // Returns null and throws an error
         expect(data).toEqual({ updateUser: null });
-        expectExtensionError(errors, [
-          { path: ['updateUser'], message: `Simulated error: ${phase}Change` },
+        expectExtensionError(errors, `${phase}Change`, [
+          { path: ['updateUser'], messages: [`User: Simulated error: ${phase}Change`] },
         ]);
 
         // User should have its original name for 'before', and the new name for 'after'.
@@ -160,8 +160,8 @@ const runner = setupTestRunner({
 
         // Returns null and throws an error
         expect(data).toEqual({ deleteUser: null });
-        expectExtensionError(errors, [
-          { path: ['deleteUser'], message: `Simulated error: ${phase}Delete` },
+        expectExtensionError(errors, `${phase}Delete`, [
+          { path: ['deleteUser'], messages: [`User: Simulated error: ${phase}Delete`] },
         ]);
 
         // Bad users should still be in the database for 'before', deleted for 'after'.
@@ -200,9 +200,9 @@ const runner = setupTestRunner({
           ],
         });
         // The invalid creates should have errors which point to the nulls in their path
-        expectExtensionError(errors, [
-          { path: ['createUsers', 1], message: `Simulated error: ${phase}Change` },
-          { path: ['createUsers', 3], message: `Simulated error: ${phase}Change` },
+        expectExtensionError(errors, `${phase}Change`, [
+          { path: ['createUsers', 1], messages: [`User: Simulated error: ${phase}Change`] },
+          { path: ['createUsers', 3], messages: [`User: Simulated error: ${phase}Change`] },
         ]);
 
         // Three users should exist in the database for 'before,' five for 'after'.
@@ -256,9 +256,9 @@ const runner = setupTestRunner({
           ],
         });
         // The invalid updates should have errors which point to the nulls in their path
-        expectExtensionError(errors, [
-          { path: ['updateUsers', 1], message: `Simulated error: ${phase}Change` },
-          { path: ['updateUsers', 3], message: `Simulated error: ${phase}Change` },
+        expectExtensionError(errors, `${phase}Change`, [
+          { path: ['updateUsers', 1], messages: [`User: Simulated error: ${phase}Change`] },
+          { path: ['updateUsers', 3], messages: [`User: Simulated error: ${phase}Change`] },
         ]);
 
         // All users should still exist in the database, un-changed for `before`, changed for `after`.
@@ -307,9 +307,9 @@ const runner = setupTestRunner({
           ],
         });
         // The invalid deletes should have errors which point to the nulls in their path
-        expectExtensionError(errors, [
-          { path: ['deleteUsers', 1], message: `Simulated error: ${phase}Delete` },
-          { path: ['deleteUsers', 3], message: `Simulated error: ${phase}Delete` },
+        expectExtensionError(errors, `${phase}Delete`, [
+          { path: ['deleteUsers', 1], messages: [`User: Simulated error: ${phase}Delete`] },
+          { path: ['deleteUsers', 3], messages: [`User: Simulated error: ${phase}Delete`] },
         ]);
 
         // Three users should still exist in the database for `before`, only 1 for `after`.
@@ -343,8 +343,14 @@ const runner = setupTestRunner({
             data: { title: `trigger ${phase}`, content: `trigger ${phase}` },
           },
         });
-        expectExtensionError(errors, [
-          { path: ['updatePost'], message: `Simulated error: title: ${phase}Change` },
+        expectExtensionError(errors, `${phase}Change`, [
+          {
+            path: ['updatePost'],
+            messages: [
+              `Post.title: Simulated error: title: ${phase}Change`,
+              `Post.content: Simulated error: content: ${phase}Change`,
+            ],
+          },
         ]);
         expect(data).toEqual({ updatePost: null });
 
@@ -371,8 +377,14 @@ const runner = setupTestRunner({
           query: `mutation ($id: ID!) { deletePost(where: { id: $id }) { id } }`,
           variables: { id: post.id },
         });
-        expectExtensionError(errors, [
-          { path: ['deletePost'], message: `Simulated error: title: ${phase}Delete` },
+        expectExtensionError(errors, `${phase}Delete`, [
+          {
+            path: ['deletePost'],
+            messages: [
+              `Post.title: Simulated error: title: ${phase}Delete`,
+              `Post.content: Simulated error: content: ${phase}Delete`,
+            ],
+          },
         ]);
         expect(data).toEqual({ deletePost: null });
 

tests/api-tests/utils.ts

@@ -85,12 +85,17 @@ export const expectValidationError = (
 
 export const expectExtensionError = (
   errors: readonly any[] | undefined,
-  args: { path: (string | number)[]; message: string }[]
+  extensionName: string,
+  args: { path: (string | number)[]; messages: string[] }[]
 ) => {
-  const unpackedErrors = (errors || []).map(({ locations, ...unpacked }) => ({
-    ...unpacked,
-  }));
-  expect(unpackedErrors).toEqual(args.map(({ path, message }) => ({ path, message })));
+  const unpackedErrors = unpackErrors(errors);
+  expect(unpackedErrors).toEqual(
+    args.map(({ path, messages }) => ({
+      extensions: { code: undefined },
+      path,
+      message: `An error occured while running "${extensionName}".\n${j(messages)}`,
+    }))
+  );
 };
 
 export const expectPrismaError = (