Skip to content

Commit

Permalink
Merge branch 'master' into rename-hooks
Browse files Browse the repository at this point in the history
  • Loading branch information
@timleslie
timleslie authored Sep 30, 2021
2 parents b1c3d06 + 21c5d1a commit 272feed
Show file tree
Hide file tree
Showing 7 changed files with 173 additions and 44 deletions.
5 changes: 5 additions & 0 deletions .changeset/clever-eyes-invite.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@keystone-next/keystone': patch
---

Updated field-type resolver error handling to catch and group errors from all fields.
55 changes: 41 additions & 14 deletions packages/keystone/src/fields/types/file/tests/test-fixtures.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import fs from 'fs-extra';
import { Upload } from 'graphql-upload';
import mime from 'mime';
import { file } from '..';
import { expectResolverError } from '../../../../../../../tests/api-tests/utils';

const prepareFile = (_filePath: string) => {
const filePath = path.resolve(`${__dirname}/../test-files/${_filePath}`);
Expand Down Expand Up @@ -141,8 +142,16 @@ export const crudTests = (keystoneTestWrapper: any) => {
variables: { item: { secretFile: { ref: 'Invalid ref!' } } },
});
expect(data).toEqual({ createTest: null });
expect(errors).toHaveLength(1);
expect(errors![0].message).toEqual('Invalid file reference');
const message = `Invalid file reference`;
expectResolverError('dev', false, false, errors, [
{
path: ['createTest'],
messages: [`Test.secretFile: ${message}`],
debug: [
{ message, stacktrace: expect.stringMatching(new RegExp(`Error: ${message}\n`)) },
],
},
]);
})
);
test(
Expand All @@ -161,10 +170,16 @@ export const crudTests = (keystoneTestWrapper: any) => {
variables: { item: { secretFile: { ref: null } } },
});
expect(data).toEqual({ createTest: null });
expect(errors).toHaveLength(1);
expect(errors![0].message).toEqual(
'Input error: Either ref or upload must be passed to FileFieldInput'
);
const message = `Input error: Either ref or upload must be passed to FileFieldInput`;
expectResolverError('dev', false, false, errors, [
{
path: ['createTest'],
messages: [`Test.secretFile: ${message}`],
debug: [
{ message, stacktrace: expect.stringMatching(new RegExp(`Error: ${message}\n`)) },
],
},
]);
})
);
test(
Expand Down Expand Up @@ -193,10 +208,16 @@ export const crudTests = (keystoneTestWrapper: any) => {
},
});
expect(data).toEqual({ createTest: null });
expect(errors).toHaveLength(1);
expect(errors![0].message).toEqual(
'Input error: Only one of ref and upload can be passed to FileFieldInput'
);
const message = `Input error: Only one of ref and upload can be passed to FileFieldInput`;
expectResolverError('dev', false, false, errors, [
{
path: ['createTest'],
messages: [`Test.secretFile: ${message}`],
debug: [
{ message, stacktrace: expect.stringMatching(new RegExp(`Error: ${message}\n`)) },
],
},
]);
})
);
test(
Expand All @@ -217,10 +238,16 @@ export const crudTests = (keystoneTestWrapper: any) => {
},
});
expect(data).toEqual({ createTest: null });
expect(errors).toHaveLength(1);
expect(errors![0].message).toEqual(
'Input error: Only one of ref and upload can be passed to FileFieldInput'
);
const message = `Input error: Only one of ref and upload can be passed to FileFieldInput`;
expectResolverError('dev', false, false, errors, [
{
path: ['createTest'],
messages: [`Test.secretFile: ${message}`],
debug: [
{ message, stacktrace: expect.stringMatching(new RegExp(`Error: ${message}\n`)) },
],
},
]);
})
);
});
Expand Down
67 changes: 51 additions & 16 deletions packages/keystone/src/fields/types/image/tests/test-fixtures.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { Upload } from 'graphql-upload';
import mime from 'mime';
import { KeystoneContext } from '../../../../types';
import { image } from '..';
import { expectResolverError } from '../../../../../../../tests/api-tests/utils';

const prepareFile = (_filePath: string) => {
const filePath = path.resolve(`${__dirname}/../test-files/${_filePath}`);
Expand Down Expand Up @@ -113,8 +114,16 @@ export const crudTests = (keystoneTestWrapper: any) => {
variables: { item: { avatar: prepareFile('badfile.txt') } },
});
expect(data).toEqual({ createTest: null });
expect(errors).toHaveLength(1);
expect(errors![0].message).toEqual('File type not found');
const message = `File type not found`;
expectResolverError('dev', false, false, errors, [
{
path: ['createTest'],
messages: [`Test.avatar: ${message}`],
debug: [
{ message, stacktrace: expect.stringMatching(new RegExp(`Error: ${message}\n`)) },
],
},
]);
})
);
});
Expand Down Expand Up @@ -179,8 +188,16 @@ export const crudTests = (keystoneTestWrapper: any) => {
variables: { item: { avatar: { ref: 'Invalid ref!' } } },
});
expect(data).toEqual({ createTest: null });
expect(errors).toHaveLength(1);
expect(errors![0].message).toEqual('Invalid image reference');
const message = `Invalid image reference`;
expectResolverError('dev', false, false, errors, [
{
path: ['createTest'],
messages: [`Test.avatar: ${message}`],
debug: [
{ message, stacktrace: expect.stringMatching(new RegExp(`Error: ${message}\n`)) },
],
},
]);
})
);
test(
Expand All @@ -199,10 +216,16 @@ export const crudTests = (keystoneTestWrapper: any) => {
variables: { item: { avatar: { ref: null } } },
});
expect(data).toEqual({ createTest: null });
expect(errors).toHaveLength(1);
expect(errors![0].message).toEqual(
'Input error: Either ref or upload must be passed to ImageFieldInput'
);
const message = `Input error: Either ref or upload must be passed to ImageFieldInput`;
expectResolverError('dev', false, false, errors, [
{
path: ['createTest'],
messages: [`Test.avatar: ${message}`],
debug: [
{ message, stacktrace: expect.stringMatching(new RegExp(`Error: ${message}\n`)) },
],
},
]);
})
);
test(
Expand All @@ -229,10 +252,16 @@ export const crudTests = (keystoneTestWrapper: any) => {
},
});
expect(data).toEqual({ createTest: null });
expect(errors).toHaveLength(1);
expect(errors![0].message).toEqual(
'Input error: Only one of ref and upload can be passed to ImageFieldInput'
);
const message = `Input error: Only one of ref and upload can be passed to ImageFieldInput`;
expectResolverError('dev', false, false, errors, [
{
path: ['createTest'],
messages: [`Test.avatar: ${message}`],
debug: [
{ message, stacktrace: expect.stringMatching(new RegExp(`Error: ${message}\n`)) },
],
},
]);
})
);
test(
Expand All @@ -253,10 +282,16 @@ export const crudTests = (keystoneTestWrapper: any) => {
},
});
expect(data).toEqual({ createTest: null });
expect(errors).toHaveLength(1);
expect(errors![0].message).toEqual(
'Input error: Only one of ref and upload can be passed to ImageFieldInput'
);
const message = `Input error: Only one of ref and upload can be passed to ImageFieldInput`;
expectResolverError('dev', false, false, errors, [
{
path: ['createTest'],
messages: [`Test.avatar: ${message}`],
debug: [
{ message, stacktrace: expect.stringMatching(new RegExp(`Error: ${message}\n`)) },
],
},
]);
})
);
});
Expand Down
10 changes: 10 additions & 0 deletions packages/keystone/src/lib/core/graphql-errors.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,16 @@ export const extensionError = (extension: string, things: { error: Error; tag: s
);
};

export const resolverError = (things: { error: Error; tag: string }[]) => {
const s = things.map(t => ` - ${t.tag}: ${t.error.message}`).join('\n');
return new ApolloError(
`An error occured while resolving input fields.\n${s}`,
'INTERNAL_SERVER_ERROR',
// Make the original stack traces available.
{ debug: things.map(t => ({ stacktrace: t.error.stack, message: t.error.message })) }
);
};

export const accessReturnError = (things: { tag: string; returned: string }[]) => {
const s = things.map(t => ` - ${t.tag}: Returned: ${t.returned}. Expected: boolean.`).join('\n');
return new ApolloError(
Expand Down
14 changes: 11 additions & 3 deletions packages/keystone/src/lib/core/mutations/create-update.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
runWithPrisma,
} from '../utils';
import { InputFilter, resolveUniqueWhereInput, UniqueInputFilter } from '../where-inputs';
import { accessDeniedError, extensionError } from '../graphql-errors';
import { accessDeniedError, extensionError, resolverError } from '../graphql-errors';
import { getOperationAccess, getAccessFilters } from '../access-control';
import { checkFilterOrderAccess } from '../filter-order-access';
import {
Expand Down Expand Up @@ -254,18 +254,26 @@ async function getResolvedData(
}

// Apply non-relationship field type input resolvers
const resolverErrors: { error: Error; tag: string }[] = [];
resolvedData = Object.fromEntries(
await promiseAllRejectWithAllErrors(
await Promise.all(
Object.entries(list.fields).map(async ([fieldKey, field]) => {
const inputResolver = field.input?.[operation]?.resolve;
let input = resolvedData[fieldKey];
if (inputResolver && field.dbField.kind !== 'relation') {
input = await inputResolver(input, context, undefined);
try {
input = await inputResolver(input, context, undefined);
} catch (error: any) {
resolverErrors.push({ error, tag: `${list.listKey}.${fieldKey}` });
}
}
return [fieldKey, input] as const;
})
)
);
if (resolverErrors.length) {
throw resolverError(resolverErrors);
}

// Apply relationship field type input resolvers
resolvedData = Object.fromEntries(
Expand Down
27 changes: 16 additions & 11 deletions tests/api-tests/fields/crud.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { text } from '@keystone-next/keystone/fields';
import { KeystoneContext } from '@keystone-next/keystone/types';
import { setupTestRunner } from '@keystone-next/keystone/testing';
import { humanize } from '@keystone-next/keystone/src/lib/utils';
import { apiTestConfig, expectBadUserInput, expectValidationError } from '../utils';
import { apiTestConfig, expectResolverError, expectValidationError } from '../utils';

const testModules = globby.sync(`packages/**/src/**/test-fixtures.{js,ts}`, {
absolute: true,
Expand Down Expand Up @@ -200,16 +200,21 @@ testModules
} else {
expect(data).toEqual({ [updateMutationName]: null });
if (mod.neverNull) {
expectBadUserInput(
errors,
[
{
path: [updateMutationName],
message: `${mod.name} fields cannot be set to null`,
},
],
false
);
const message = `Input error: ${mod.name} fields cannot be set to null`;
expectResolverError('dev', false, false, errors, [
{
path: [updateMutationName],
messages: [`Test.${fieldName}: ${message}`],
debug: [
{
message,
stacktrace: expect.stringMatching(
new RegExp(`Error: ${message}\n`)
),
},
],
},
]);
} else {
expectValidationError(errors, [
{
Expand Down
39 changes: 39 additions & 0 deletions tests/api-tests/utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -213,3 +213,42 @@ export const expectFilterDenied = (
}))
);
};

export const expectResolverError = (
mode: 'dev' | 'production',
httpQuery: boolean,
_debug: boolean | undefined,
errors: readonly any[] | undefined,
args: { path: (string | number)[]; messages: string[]; debug: any[] }[]
) => {
const unpackedErrors = unpackErrors(errors);
expect(unpackedErrors).toEqual(
args.map(({ path, messages, debug }) => {
const message = `An error occured while resolving input fields.\n${j(messages)}`;
const stacktrace = message.split('\n');
stacktrace[0] = `Error: ${stacktrace[0]}`;

// We expect to see debug details if:
// - httpQuery is false
// - graphql.debug is true or
// - graphql.debug is undefined and mode !== production or
const expectDebug =
_debug === true || (_debug === undefined && mode !== 'production') || !httpQuery;
// We expect to see the Apollo exception under the same conditions, but only if
// httpQuery is also true.
const expectException = httpQuery && expectDebug;

return {
extensions: {
code: 'INTERNAL_SERVER_ERROR',
...(expectException
? { exception: { stacktrace: expect.arrayContaining(stacktrace) } }
: {}),
...(expectDebug ? { debug } : {}),
},
path,
message,
};
})
);
};

0 comments on commit 272feed

Please sign in to comment.