From 407ef567928976447c5cb8b3b7af3a0795c87dfb Mon Sep 17 00:00:00 2001 From: David Glasser Date: Thu, 4 Aug 2022 15:00:23 -0700 Subject: [PATCH] Updates (see PR description) --- docs/source/migration.mdx | 87 +++++++++++- .../src/apolloServerTests.ts | 14 +- .../src/httpServerTests.ts | 91 +++++++++++- .../server/src/__tests__/ApolloServer.test.ts | 12 +- packages/server/src/__tests__/errors.test.ts | 132 ++++++++++++------ packages/server/src/errorNormalize.ts | 35 ++--- packages/server/src/requestPipeline.ts | 2 +- 7 files changed, 278 insertions(+), 95 deletions(-) diff --git a/docs/source/migration.mdx b/docs/source/migration.mdx index 374c6459e1b..3483caa8d1c 100644 --- a/docs/source/migration.mdx +++ b/docs/source/migration.mdx @@ -759,8 +759,6 @@ throw new ApolloError(message, 'YOUR_ERROR_CODE'); you should now pass your error code to the `extensions` option; see the above code snippet for an example. -In Apollo Server 3, the `code` and `stacktrace` fields on errors are found at `error.extensions.exception.code` and `error.extensions.exception.stacktrace`; `error.extensions.exception` also contains any enumerable properties on the originally thrown error. In Apollo Server 4, these fields are found directly at `error.extensions.code` and `error.extensions.stacktrace`; `error.extensions.exception` is reserved for the enumerable properties of the originally thrown error only. - ### Built-in error classes Apollo Server 3 exports several specific error classes. Apollo Server's code produces some of them (`SyntaxError`, `ValidationError`, and `UserInputError`). Others (`ForbiddenError` and `AuthenticationError`) are provided for users to use in their apps. All of these are subclasses of the `ApolloError` class. @@ -1028,7 +1026,9 @@ expect(result.data?.hello).toBe('Hello world!'); // -> true -### `formatError` hook improvements +### Error formatting changes + +#### `formatError` improvements Apollo Server 3 supports the `formatError` hook with the following signature: ``` @@ -1066,6 +1066,87 @@ So now you can format errors as such: }, ``` +#### `error.extensions.exception` is removed + +When Apollo Server 3 formats an error, it may add an extension called `exception`. This extension is an object with a field for every *enumerable* property of the originally thrown error. (This does not apply if the originally thrown error was already a `GraphQLError`.) In addition, [when in dev/debug mode](#debug), `exception` contains an array of strings called `stacktrace`. + +For example, if this code runs in a resolver: + +```js +const e = new Error("hello"); +e.extraProperty = "bye"; +throw e; +``` + +then (in debug mode) Apollo Server 3 will format the error like this: + +```js +{ + "errors": [ + { + "message": "hello", + "locations": [ + { + "line": 2, + "column": 3 + } + ], + "path": ["x"], + "extensions": { + "code": "INTERNAL_SERVER_ERROR", + "exception": { + "extraProperty": "bye", + "stacktrace": [ + "Error: hello", + " at Object.x (file:///private/tmp/as3-t/server.mjs:8:27)", + " at field.resolve (/private/tmp/as3-t/node_modules/apollo-server-core/dist/utils/schemaInstrumentation.js:56:26)", + // more lines elided + ] + } + } + } + ] +} +``` + +It was often hard to predict exactly which properties of which errors would be publicly exposed in this manner, which could lead to surprising information leaks. + +In Apollo Server 4, there is no `exception` extension. The `stacktrace` is provided directly on `extensions`. If you'd like to copy some or all properties from the original error onto the formatted error, you can do that with the `formatError` hook. + +If you'd like your errors to be formatted like they are in Apollo Server 3 (with the stack trace and the enumerable properties of the original error on the `exception` extension), you can provide this `formatError` implementation: + + + +```ts +function formatError( + formattedError: GraphQLFormattedError, + error: unknown, +) { + const exception: Record = { + ...(typeof error === 'object' ? error : null), + }; + delete exception.extensions; + if (formattedError.extensions?.stacktrace) { + exception.stacktrace = formattedError.extensions.stacktrace; + } + const extensions: Record = { + ...formattedError.extensions, + exception, + }; + delete extensions.stacktrace; + return { + ...formattedError, + extensions, + }; +} +``` + + + +Apollo Server 3.5.0 and newer included a TypeScript `declare module` declaration that teaches TypeScript that `error.extensions.exception.stacktrace` is an array of strings on *all* `GraphQLError` objects. Apollo Server 4 does not provide a replacement for this: that is, we do not tell TypeScript the type of `error.extensions.code` or `error.extensions.stacktrace`. (The Apollo Server 3 `declare module` declaration also incorrectly teaches TypeScript that `error.extensions.exception.code` is a string, which should have been `error.extensions.code` instead.) + + + ### HTTP error handling changes Apollo Server 3 returns specific errors relating to GraphQL operations over HTTP/JSON as `text/plain` error messages. diff --git a/packages/integration-testsuite/src/apolloServerTests.ts b/packages/integration-testsuite/src/apolloServerTests.ts index cd7fa2871b4..dfa083d36bd 100644 --- a/packages/integration-testsuite/src/apolloServerTests.ts +++ b/packages/integration-testsuite/src/apolloServerTests.ts @@ -862,14 +862,6 @@ export function defineIntegrationTestSuiteApolloServerTests( locations: [{ line: 1, column: 2 }], extensions: { code: 'INTERNAL_SERVER_ERROR', - exception: { - name: 'ValidationError', - message: 'email must be a valid email', - type: undefined, - value: { email: 'invalid-email' }, - errors: ['email must be a valid email'], - path: 'email', - }, }, }); expect(formatErrorArgs[1] instanceof Error).toBe(true); @@ -1765,7 +1757,7 @@ export function defineIntegrationTestSuiteApolloServerTests( expect(result.errors).toBeDefined(); expect(result.errors.length).toEqual(1); expect(result.errors[0].extensions.code).toEqual('SOME_CODE'); - expect(result.errors[0].extensions.exception).toBeUndefined(); + expect(result.errors[0].extensions).not.toHaveProperty('exception'); }); it('propagates error codes with null response in production', async () => { @@ -1796,7 +1788,7 @@ export function defineIntegrationTestSuiteApolloServerTests( expect(result.errors).toBeDefined(); expect(result.errors.length).toEqual(1); expect(result.errors[0].extensions.code).toEqual('SOME_CODE'); - expect(result.errors[0].extensions.exception).toBeUndefined(); + expect(result.errors[0].extensions).not.toHaveProperty('exception'); }); it('propagates error codes in dev mode', async () => { @@ -1828,7 +1820,7 @@ export function defineIntegrationTestSuiteApolloServerTests( expect(result.errors).toBeDefined(); expect(result.errors.length).toEqual(1); expect(result.errors[0].extensions.code).toEqual('SOME_CODE'); - expect(result.errors[0].extensions.exception).toBeUndefined(); + expect(result.errors[0].extensions).not.toHaveProperty('exception'); expect(result.errors[0].extensions.stacktrace).toBeDefined(); }); diff --git a/packages/integration-testsuite/src/httpServerTests.ts b/packages/integration-testsuite/src/httpServerTests.ts index bafdd1c4ba7..c26f39b8373 100644 --- a/packages/integration-testsuite/src/httpServerTests.ts +++ b/packages/integration-testsuite/src/httpServerTests.ts @@ -47,6 +47,7 @@ import { } from '@jest/globals'; import type { Mock, SpyInstance } from 'jest-mock'; import { cacheControlFromInfo } from '@apollo/cache-control-types'; +import { ApolloServerErrorCode } from '@apollo/server/errors'; const QueryRootType = new GraphQLObjectType({ name: 'QueryRoot', @@ -174,12 +175,21 @@ const queryType = new GraphQLObjectType({ testError: { type: GraphQLString, resolve() { - throw new Error('Secret error message'); + throw new MyError('Secret error message'); + }, + }, + testGraphQLError: { + type: GraphQLString, + resolve() { + throw new MyGraphQLError('Secret error message'); }, }, }, }); +class MyError extends Error {} +class MyGraphQLError extends GraphQLError {} + const mutationType = new GraphQLObjectType({ name: 'MutationType', fields: { @@ -1403,21 +1413,88 @@ export function defineIntegrationTestSuiteHttpServerTests( it('formatError receives error that passes instanceof checks', async () => { const expected = '--blank--'; + let gotMyError = false; const app = await createApp({ schema, formatError: (_, error) => { - expect(error instanceof Error).toBe(true); - expect(error instanceof GraphQLError).toBe(true); + gotMyError = error instanceof MyError; return { message: expected }; }, }); - const req = request(app).post('/').send({ + const res = await request(app).post('/').send({ query: 'query test{ testError }', }); - return req.then((res) => { - expect(res.status).toEqual(200); - expect(res.body.errors[0].message).toEqual(expected); + expect(res.status).toEqual(200); + expect(res.body).toMatchInlineSnapshot(` + Object { + "data": Object { + "testError": null, + }, + "errors": Array [ + Object { + "message": "--blank--", + }, + ], + } + `); + expect(gotMyError).toBe(true); + }); + + it('formatError receives error that passes instanceof checks when GraphQLError', async () => { + const expected = '--blank--'; + let gotMyGraphQLError = false; + const app = await createApp({ + schema, + formatError: (_, error) => { + gotMyGraphQLError = error instanceof MyGraphQLError; + return { message: expected }; + }, + }); + const res = await request(app).post('/').send({ + query: 'query test{ testGraphQLError }', }); + expect(res.status).toEqual(200); + expect(res.body).toMatchInlineSnapshot(` + Object { + "data": Object { + "testGraphQLError": null, + }, + "errors": Array [ + Object { + "message": "--blank--", + }, + ], + } + `); + expect(gotMyGraphQLError).toBe(true); + }); + + it('formatError receives correct error for parse failure', async () => { + const expected = '--blank--'; + let gotCorrectCode = false; + const app = await createApp({ + schema, + formatError: (_, error) => { + gotCorrectCode = + (error as any).extensions.code === + ApolloServerErrorCode.GRAPHQL_PARSE_FAILED; + return { message: expected }; + }, + }); + const res = await request(app).post('/').send({ + query: '}', + }); + expect(res.status).toEqual(400); + expect(res.body).toMatchInlineSnapshot(` + Object { + "errors": Array [ + Object { + "message": "--blank--", + }, + ], + } + `); + expect(gotCorrectCode).toBe(true); }); it('allows for custom error formatting to sanitize', async () => { diff --git a/packages/server/src/__tests__/ApolloServer.test.ts b/packages/server/src/__tests__/ApolloServer.test.ts index babbc699c8d..362023d6780 100644 --- a/packages/server/src/__tests__/ApolloServer.test.ts +++ b/packages/server/src/__tests__/ApolloServer.test.ts @@ -1,6 +1,6 @@ import { ApolloServer } from '..'; import type { ApolloServerOptions, GatewayInterface } from '..'; -import type { GraphQLSchema } from 'graphql'; +import { GraphQLError, GraphQLSchema } from 'graphql'; import type { ApolloServerPlugin, BaseContext } from '../externalTypes'; import { ApolloServerPluginCacheControlDisabled } from '../plugin/disabled/index.js'; import { ApolloServerPluginUsageReporting } from '../plugin/usageReporting/index.js'; @@ -23,9 +23,9 @@ const resolvers = { return 'world'; }, error() { - const myError = new Error('A test error'); - (myError as any).someField = 'value'; - throw myError; + throw new GraphQLError('A test error', { + extensions: { someField: 'value' }, + }); }, contextFoo(_root: any, _args: any, context: any) { return context.foo; @@ -254,7 +254,7 @@ describe('ApolloServer executeOperation', () => { expect(result.errors).toHaveLength(1); expect(result.errors?.[0].extensions).toStrictEqual({ code: 'INTERNAL_SERVER_ERROR', - exception: { someField: 'value' }, + someField: 'value', }); await server.stop(); }); @@ -275,7 +275,7 @@ describe('ApolloServer executeOperation', () => { const extensions = result.errors?.[0].extensions; expect(extensions).toHaveProperty('code', 'INTERNAL_SERVER_ERROR'); expect(extensions).toHaveProperty('stacktrace'); - expect(extensions).toHaveProperty('exception', { someField: 'value' }); + expect(extensions).toHaveProperty('someField', 'value'); await server.stop(); }); diff --git a/packages/server/src/__tests__/errors.test.ts b/packages/server/src/__tests__/errors.test.ts index da65b8c0ea7..113001feba9 100644 --- a/packages/server/src/__tests__/errors.test.ts +++ b/packages/server/src/__tests__/errors.test.ts @@ -1,55 +1,28 @@ -import { GraphQLError } from 'graphql'; +import { GraphQLError, GraphQLFormattedError } from 'graphql'; import { normalizeAndFormatErrors } from '../errorNormalize.js'; describe('Errors', () => { describe('normalizeAndFormatErrors', () => { - type CreateFormatError = - | (( - options: Record, - errors: Error[], - ) => Record[]) - | ((options?: Record) => Record); const message = 'message'; const code = 'CODE'; const key = 'value'; - const createFormattedError: CreateFormatError = ( - options?: Record, - errors?: Error[], - ) => { - if (errors === undefined) { - const error = new GraphQLError(message, { - extensions: { code, key }, - }); - return normalizeAndFormatErrors( - [ - new GraphQLError( - error.message, - undefined, - undefined, - undefined, - undefined, - error, - ), - ], - options, - )[0]; - } else { - return normalizeAndFormatErrors(errors, options); - } - }; - it('exposes a stacktrace in debug mode', () => { const thrown = new Error(message); (thrown as any).key = key; - const error = normalizeAndFormatErrors([ - new GraphQLError(thrown.message, { originalError: thrown }), + const [error] = normalizeAndFormatErrors( + [ + new GraphQLError(thrown.message, { + originalError: thrown, + extensions: { code, key }, + }), + ], { includeStacktraceInErrorResponses: true }, - ])[0]; + ); expect(error.message).toEqual(message); expect(error.extensions?.key).toEqual(key); - expect(error.extensions?.exception).toBeUndefined(); + expect(error.extensions).not.toHaveProperty('exception'); // Removed in AS4 expect(error.extensions?.code).toEqual(code); // stacktrace should exist expect(error.extensions?.stacktrace).toBeDefined(); @@ -62,16 +35,20 @@ describe('Errors', () => { ])[0]; expect(error.message).toEqual(message); expect(error.extensions?.code).toEqual('INTERNAL_SERVER_ERROR'); - expect(error.extensions?.exception).toHaveProperty('key', key); + expect(error.extensions).not.toHaveProperty('exception'); // Removed in AS4 // stacktrace should not exist expect(error.extensions).not.toHaveProperty('stacktrace'); }); it('exposes extensions on error as extensions field and provides code', () => { - const error = createFormattedError(); + const error = normalizeAndFormatErrors([ + new GraphQLError(message, { + extensions: { code, key }, + }), + ])[0]; expect(error.message).toEqual(message); - expect(error.extensions.key).toEqual(key); - expect(error.extensions.exception).toBeUndefined(); - expect(error.extensions.code).toEqual(code); + expect(error.extensions?.key).toEqual(key); + expect(error.extensions).not.toHaveProperty('exception'); // Removed in AS4 + expect(error.extensions?.code).toEqual(code); }); it('calls formatError after exposing the code and stacktrace', () => { const error = new GraphQLError(message, { @@ -96,5 +73,76 @@ describe('Errors', () => { const [formattedError] = normalizeAndFormatErrors([error]); expect(JSON.parse(JSON.stringify(formattedError)).message).toBe('Hello'); }); + + describe('formatError can be used to provide AS3-compatible extensions', () => { + function formatError( + formattedError: GraphQLFormattedError, + error: unknown, + ) { + const exception: Record = { + ...(typeof error === 'object' ? error : null), + }; + delete exception.extensions; + if (formattedError.extensions?.stacktrace) { + exception.stacktrace = formattedError.extensions.stacktrace; + } + const extensions: Record = { + ...formattedError.extensions, + exception, + }; + delete extensions.stacktrace; + + return { + ...formattedError, + extensions, + }; + } + + it('with stack trace', () => { + const thrown = new Error(message); + (thrown as any).key = key; + const errors = normalizeAndFormatErrors([thrown], { + formatError, + includeStacktraceInErrorResponses: true, + }); + expect(errors).toHaveLength(1); + const [error] = errors; + expect(error.extensions?.exception).toHaveProperty('stacktrace'); + delete (error as any).extensions.exception.stacktrace; + expect(error).toMatchInlineSnapshot(` + Object { + "extensions": Object { + "code": "INTERNAL_SERVER_ERROR", + "exception": Object { + "key": "value", + }, + }, + "message": "message", + } + `); + }); + + it('without stack trace', () => { + const thrown = new Error(message); + (thrown as any).key = key; + const errors = normalizeAndFormatErrors([thrown], { + formatError, + includeStacktraceInErrorResponses: false, + }); + expect(errors).toHaveLength(1); + const [error] = errors; + expect(error).toMatchInlineSnapshot(` + Object { + "extensions": Object { + "code": "INTERNAL_SERVER_ERROR", + "exception": Object { + "key": "value", + }, + }, + "message": "message", + } + `); + }); + }); }); }); diff --git a/packages/server/src/errorNormalize.ts b/packages/server/src/errorNormalize.ts index 9d85b62b9ab..2a5529addb0 100644 --- a/packages/server/src/errorNormalize.ts +++ b/packages/server/src/errorNormalize.ts @@ -23,7 +23,16 @@ export function normalizeAndFormatErrors( const formatError = options.formatError ?? ((error) => error); return errors.map((error) => { try { - return formatError(enrichError(error), error); + return formatError( + enrichError(error), + // If the error was thrown in a resolver, graphql-js will wrap it in a + // GraphQLError before it gets back to Apollo Server (to add the path + // etc to it). We'd like to give users their actual original error. (If + // there is no path, this error didn't come from a resolver.) + error instanceof GraphQLError && error.path && error.originalError + ? error.originalError + : error, + ); } catch (formattingError) { if (options.includeStacktraceInErrorResponses) { // includeStacktraceInErrorResponses is used in development @@ -39,16 +48,6 @@ export function normalizeAndFormatErrors( } }); - // You can't spread anything that isn't an object. If somebody - // puts a non-object on extensions.exception, we will just replace - // it with the empty object. - function ensureObject(x: unknown): object { - if (x && typeof x === 'object') { - return x; - } - return {}; - } - function enrichError(maybeError: unknown): GraphQLFormattedError { const graphqlError = ensureGraphQLError(maybeError); @@ -59,20 +58,6 @@ export function normalizeAndFormatErrors( ApolloServerErrorCode.INTERNAL_SERVER_ERROR, }; - const { originalError } = graphqlError; - if (originalError != null && !(originalError instanceof GraphQLError)) { - const originalErrorEnumerableEntries = Object.entries( - originalError, - ).filter(([key]) => key !== 'extensions'); - - if (originalErrorEnumerableEntries.length > 0) { - extensions.exception = { - ...ensureObject(extensions.exception), - ...Object.fromEntries(originalErrorEnumerableEntries), - }; - } - } - if (options.includeStacktraceInErrorResponses) { // Note that if ensureGraphQLError created graphqlError from an // originalError, graphqlError.stack will be the same as diff --git a/packages/server/src/requestPipeline.ts b/packages/server/src/requestPipeline.ts index ff3a8d53ede..5ee1d80098d 100644 --- a/packages/server/src/requestPipeline.ts +++ b/packages/server/src/requestPipeline.ts @@ -559,7 +559,7 @@ export async function processGraphQLRequest( } function formatErrors( - errors: ReadonlyArray, + errors: ReadonlyArray, ): ReadonlyArray { return normalizeAndFormatErrors(errors, { formatError: internals.formatError,