Improve error messages for validation failures (#6218)

keystonejs · Aug 1, 2021 · c7e331d · c7e331d · vercel · Aug 1, 2021
1 parent 67807db
commit c7e331d
Show file tree

Hide file tree

Showing 9 changed files with 102 additions and 66 deletions.
diff --git a/.changeset/fluffy-coins-argue.md b/.changeset/fluffy-coins-argue.md
@@ -0,0 +1,6 @@
+---
+'@keystone-next/example-testing': patch
+'@keystone-next/keystone': patch
+---
+
+Added more details to validation failure error messages.
diff --git a/docs/pages/docs/guides/testing.mdx b/docs/pages/docs/guides/testing.mdx
@@ -94,7 +94,9 @@ runner(async ({ context }) => {
   expect(data.createPerson).toBe(null);
   expect(errors).toHaveLength(1);
   expect(errors[0].path).toEqual(['createPerson']);
-  expect(errors[0].message).toEqual('You attempted to perform an invalid mutation');
+  expect(errors[0].message).toEqual(
+    'You provided invalid data for this operation.\n  - Person.name: Required field "name" is null or undefined.'
+  );
 })
 ```
 

diff --git a/examples/testing/example.test.ts b/examples/testing/example.test.ts
@@ -57,7 +57,9 @@ describe('Example tests using test runner', () => {
       expect(data!.createPerson).toBe(null);
       expect(errors).toHaveLength(1);
       expect(errors![0].path).toEqual(['createPerson']);
-      expect(errors![0].message).toEqual('You attempted to perform an invalid mutation');
+      expect(errors![0].message).toEqual(
+        'You provided invalid data for this operation.\n  - Person.name: Required field "name" is null or undefined.'
+      );
     })
   );
 

diff --git a/packages/keystone/src/lib/core/graphql-errors.ts b/packages/keystone/src/lib/core/graphql-errors.ts
@@ -2,8 +2,10 @@ import { ApolloError } from 'apollo-server-errors';
 
 export const accessDeniedError = () => new ApolloError('You do not have access to this resource');
 
-export const validationFailureError = () =>
-  new ApolloError('You attempted to perform an invalid mutation');
+export const validationFailureError = (messages: string[]) => {
+  const s = messages.map(m => `  - ${m}`).join('\n');
+  return new ApolloError(`You provided invalid data for this operation.\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

diff --git a/packages/keystone/src/lib/core/mutations/validation.ts b/packages/keystone/src/lib/core/mutations/validation.ts
@@ -1,27 +1,19 @@
 import { validationFailureError } from '../graphql-errors';
 import { InitialisedList } from '../types-for-lists';
-import { promiseAllRejectWithAllErrors } from '../utils';
 
-type ValidationError = { msg: string; data: {}; internalData: {} };
+type AddValidationError = (msg: string) => void;
 
-type AddValidationError = (msg: string, data?: {}, internalData?: {}) => void;
-
-export async function validationHook(
-  listKey: string,
-  operation: 'create' | 'update' | 'delete',
-  originalInput: Record<string, string> | undefined,
-  validationHook: (addValidationError: AddValidationError) => void | Promise<void>
+async function validationHook(
+  _validationHook: (addValidationError: AddValidationError) => void | Promise<void>
 ) {
-  const errors: ValidationError[] = [];
+  const messages: string[] = [];
 
-  await validationHook((msg, data = {}, internalData = {}) => {
-    errors.push({ msg, data, internalData });
+  await _validationHook(msg => {
+    messages.push(msg);
   });
 
-  if (errors.length) {
-    // FIXME: We will incorporate the `msg` values, which are currently being lost
-    // into the error message in an upcoming change. -TL
-    throw validationFailureError();
+  if (messages.length) {
+    throw validationFailureError(messages);
   }
 }
 
@@ -35,10 +27,12 @@ export async function validateUpdateCreate({
   list: InitialisedList;
   hookArgs: Omit<UpdateCreateHookArgs, 'addValidationError'>;
 }) {
-  const { operation, resolvedData, originalInput } = hookArgs;
-  // Check isRequired
-  await validationHook(list.listKey, operation, originalInput, addValidationError => {
+  const { operation, resolvedData } = hookArgs;
+  await validationHook(async _addValidationError => {
+    // Check isRequired
     for (const [fieldKey, field] of Object.entries(list.fields)) {
+      const addValidationError = (msg: string) =>
+        _addValidationError(`${list.listKey}.${fieldKey}: ${msg}`);
       // yes, this is a massive hack, it's just to make image and file fields work well enough
       let val = resolvedData[fieldKey];
       if (field.dbField.kind === 'multi') {
@@ -53,27 +47,20 @@ export async function validateUpdateCreate({
         field.__legacy?.isRequired &&
         ((operation === 'create' && val == null) || (operation === 'update' && val === null))
       ) {
-        addValidationError(
-          `Required field "${fieldKey}" is null or undefined.`,
-          { resolvedData, operation, originalInput },
-          {}
-        );
+        addValidationError(`Required field "${fieldKey}" is null or undefined.`);
       }
     }
-  });
 
-  // Field validation hooks
-  await validationHook(list.listKey, operation, originalInput, async addValidationError => {
-    await promiseAllRejectWithAllErrors(
-      Object.entries(list.fields).map(async ([fieldPath, field]) => {
-        // @ts-ignore
-        await field.hooks.validateInput?.({ ...hookArgs, addValidationError, fieldPath });
-      })
-    );
-  });
+    // Field validation hooks
+    for (const [fieldPath, field] of Object.entries(list.fields)) {
+      const addValidationError = (msg: string) =>
+        _addValidationError(`${list.listKey}.${fieldPath}: ${msg}`);
+      // @ts-ignore
+      await field.hooks.validateInput?.({ ...hookArgs, addValidationError, fieldPath });
+    }
 
-  // List validation hooks
-  await validationHook(list.listKey, operation, originalInput, async addValidationError => {
+    // List validation hooks
+    const addValidationError = (msg: string) => _addValidationError(`${list.listKey}: ${msg}`);
     // @ts-ignore
     await list.hooks.validateInput?.({ ...hookArgs, addValidationError });
   });
@@ -87,17 +74,16 @@ export async function validateDelete({
   list: InitialisedList;
   hookArgs: Omit<DeleteHookArgs, 'addValidationError'>;
 }) {
-  // Field validation
-  await validationHook(list.listKey, 'delete', undefined, async addValidationError => {
-    await promiseAllRejectWithAllErrors(
-      Object.entries(list.fields).map(async ([fieldPath, field]) => {
-        await field.hooks.validateDelete?.({ ...hookArgs, addValidationError, fieldPath });
-      })
-    );
-  });
+  await validationHook(async _addValidationError => {
+    // Field validation
+    for (const [fieldPath, field] of Object.entries(list.fields)) {
+      const addValidationError = (msg: string) =>
+        _addValidationError(`${list.listKey}.${fieldPath}: ${msg}`);
+      await field.hooks.validateDelete?.({ ...hookArgs, addValidationError, fieldPath });
+    }
 
-  // List validation
-  await validationHook(list.listKey, 'delete', undefined, async addValidationError => {
+    // List validation
+    const addValidationError = (msg: string) => _addValidationError(`${list.listKey}: ${msg}`);
     await list.hooks.validateDelete?.({ ...hookArgs, addValidationError });
   });
 }
diff --git a/tests/api-tests/fields/required.test.ts b/tests/api-tests/fields/required.test.ts
@@ -65,7 +65,12 @@ testModules
                   }`,
             });
             expect(data).toEqual({ createTest: null });
-            expectValidationError(errors, [{ path: ['createTest'] }]);
+            expectValidationError(errors, [
+              {
+                path: ['createTest'],
+                messages: ['Test.testField: Required field "testField" is null or undefined.'],
+              },
+            ]);
           })
         );
 
@@ -85,7 +90,12 @@ testModules
                   }`,
             });
             expect(data).toEqual({ updateTest: null });
-            expectValidationError(errors, [{ path: ['updateTest'] }]);
+            expectValidationError(errors, [
+              {
+                path: ['updateTest'],
+                messages: ['Test.testField: Required field "testField" is null or undefined.'],
+              },
+            ]);
           })
         );
 

diff --git a/tests/api-tests/hooks/auth-hooks.test.skip.ts b/tests/api-tests/hooks/auth-hooks.test.skip.ts
@@ -246,7 +246,7 @@ describe('Auth Hooks', () => {
         expect(authenticateUserWithPassword).toBe(null);
         expect(errors).not.toBe(undefined);
         expect(errors).toHaveLength(1);
-        expect(errors[0].message).toEqual('You attempted to perform an invalid mutation');
+        expect(errors[0].message).toEqual('You provided invalid data for this operation.');
         expect(errors[0].path).toEqual(['authenticateUserWithPassword']);
       });
     })

diff --git a/tests/api-tests/hooks/validation.test.ts b/tests/api-tests/hooks/validation.test.ts
@@ -11,12 +11,12 @@ const runner = setupTestRunner({
         hooks: {
           validateInput: ({ resolvedData, addValidationError }) => {
             if (resolvedData.name === 'bad') {
-              addValidationError('Bad name');
+              addValidationError('This is not a valid name');
             }
           },
           validateDelete: ({ existingItem, addValidationError }) => {
             if (existingItem.name.startsWith('no delete')) {
-              addValidationError('Bad name');
+              addValidationError('Deleting this item would be bad');
             }
           },
         },
@@ -79,7 +79,9 @@ describe('List Hooks: #validateInput()', () => {
 
       // Returns null and throws an error
       expect(data).toEqual({ createUser: null });
-      expectValidationError(errors, [{ path: ['createUser'] }]);
+      expectValidationError(errors, [
+        { path: ['createUser'], messages: ['User: This is not a valid name'] },
+      ]);
 
       // Only the original user should exist
       const _users = await context.lists.User.findMany({ query: 'id name' });
@@ -102,7 +104,9 @@ describe('List Hooks: #validateInput()', () => {
 
       // Returns null and throws an error
       expect(data).toEqual({ updateUser: null });
-      expectValidationError(errors, [{ path: ['updateUser'] }]);
+      expectValidationError(errors, [
+        { path: ['updateUser'], messages: ['User: This is not a valid name'] },
+      ]);
 
       // User should have its original name
       const _users = await context.lists.User.findMany({ query: 'id name' });
@@ -126,7 +130,9 @@ describe('List Hooks: #validateInput()', () => {
 
       // Returns null and throws an error
       expect(data).toEqual({ deleteUser: null });
-      expectValidationError(errors, [{ path: ['deleteUser'] }]);
+      expectValidationError(errors, [
+        { path: ['deleteUser'], messages: ['User: Deleting this item would be bad'] },
+      ]);
 
       // Bad users should still be in the database.
       const _users = await context.lists.User.findMany({ query: 'id name' });
@@ -162,7 +168,10 @@ describe('List Hooks: #validateInput()', () => {
         ],
       });
       // The invalid creates should have errors which point to the nulls in their path
-      expectValidationError(errors, [{ path: ['createUsers', 1] }, { path: ['createUsers', 3] }]);
+      expectValidationError(errors, [
+        { path: ['createUsers', 1], messages: ['User: This is not a valid name'] },
+        { path: ['createUsers', 3], messages: ['User: This is not a valid name'] },
+      ]);
 
       // Three users should exist in the database
       const users = await context.lists.User.findMany({
@@ -211,7 +220,10 @@ describe('List Hooks: #validateInput()', () => {
         ],
       });
       // The invalid updates should have errors which point to the nulls in their path
-      expectValidationError(errors, [{ path: ['updateUsers', 1] }, { path: ['updateUsers', 3] }]);
+      expectValidationError(errors, [
+        { path: ['updateUsers', 1], messages: ['User: This is not a valid name'] },
+        { path: ['updateUsers', 3], messages: ['User: This is not a valid name'] },
+      ]);
 
       // All users should still exist in the database
       const _users = await context.lists.User.findMany({
@@ -261,7 +273,10 @@ describe('List Hooks: #validateInput()', () => {
         ],
       });
       // The invalid deletes should have errors which point to the nulls in their path
-      expectValidationError(errors, [{ path: ['deleteUsers', 1] }, { path: ['deleteUsers', 3] }]);
+      expectValidationError(errors, [
+        { path: ['deleteUsers', 1], messages: ['User: Deleting this item would be bad'] },
+        { path: ['deleteUsers', 3], messages: ['User: Deleting this item would be bad'] },
+      ]);
 
       // Three users should still exist in the database
       const _users = await context.lists.User.findMany({
@@ -283,7 +298,16 @@ describe('Field Hooks: #validateInput()', () => {
         query: `mutation ($id: ID! $data: PostUpdateInput!) { updatePost(where: { id: $id }, data: $data) { id } }`,
         variables: { id: post.id, data: {} },
       });
-      expectValidationError(errors, [{ path: ['updatePost'] }]);
+      expectValidationError(errors, [
+        {
+          path: ['updatePost'],
+          messages: [
+            'Post.neverValid: never change me',
+            'Post.doubleInvalid: first error',
+            'Post.doubleInvalid: second error',
+          ],
+        },
+      ]);
       expect(data).toEqual({ updatePost: null });
     })
   );
@@ -296,7 +320,9 @@ describe('Field Hooks: #validateInput()', () => {
         query: `mutation ($id: ID!) { deletePost(where: { id: $id }) { id } }`,
         variables: { id: post.id },
       });
-      expectValidationError(errors, [{ path: ['deletePost'] }]);
+      expectValidationError(errors, [
+        { path: ['deletePost'], messages: ['Post.noDelete: I am invincible!'] },
+      ]);
       expect(data).toEqual({ deletePost: null });
     })
   );

diff --git a/tests/api-tests/utils.ts b/tests/api-tests/utils.ts
@@ -22,6 +22,8 @@ const unpackErrors = (errors: readonly any[] | undefined) =>
     ...unpacked,
   }));
 
+const j = (messages: string[]) => messages.map(m => `  - ${m}`).join('\n');
+
 export const expectInternalServerError = (
   errors: readonly any[] | undefined,
   args: { path: any[]; message: string }[]
@@ -67,16 +69,16 @@ export const expectAccessDenied = (
 
 export const expectValidationError = (
   errors: readonly any[] | undefined,
-  args: { path: (string | number)[] }[]
+  args: { path: (string | number)[]; messages: string[] }[]
 ) => {
   const unpackedErrors = (errors || []).map(({ locations, ...unpacked }) => ({
     ...unpacked,
   }));
   expect(unpackedErrors).toEqual(
-    args.map(({ path }) => ({
+    args.map(({ path, messages }) => ({
       extensions: { code: undefined },
       path,
-      message: 'You attempted to perform an invalid mutation',
+      message: `You provided invalid data for this operation.\n${j(messages)}`,
     }))
   );
 };