From 6ef6a090cff26f5d98e9965cd839307931e12516 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Fri, 5 Aug 2022 14:29:14 -0700 Subject: [PATCH] Error extensions improvements (#6759) This PR does a few things: (a) Moves `error.extensions.exception.stacktrace` to `error.extensions.stacktrace`. (b) Removes the rest of `error.extensions.exception`. This contained enumerable properties of the original error, which could lead to surprising information leakage. (c) Documents that change and provides a `formatError` hook in the migration guide to maintain AS3 behavior. (d) Provide an `unwrapResolverError` function in `@apollo/server/errors` to help get back the original error in the resolver error case, which is a helpful part of the documented `formatError` recommendations (since the old error formatting put stuff from that unwrapped error on the `exception` extension). (e) Gets rid of the `declare module` which made `error.extensions.exception.code`/`error.extensions.exception.stacktrace` have a non-unknown value. Note that this declaration (added in 3.5.0) was actually inaccurate as `code` really goes directly on `extensions` rather than on `exception`. We could have instead preserved the declaration and adapted it to the new location of `stacktrace` and the correct location of `code`. - Pro: `declare module` is a bit scary and doesn't always merge well if you have more than one of them (eg, if you end up with both AS3 and AS4 in your TypeScript build: AS3 had a different `declare module` where `code` was nested under `exception`). - Con: End users who catch our errors can't get "correct" typing for `error.extensions.code` or `error.extensions.stacktrace`. - Pro: That's only "correct" if you assume all GraphQLErrors use extensions compatible with the definition; it might be better to export a helper function that takes a `GraphQLError` and returns the code/exception if it looks like it has the right shape. And nobody seems to have even noticed that the declaration has been wrong for almost a year, so how much value is it providing? (f) Renames the (new in v4) constructor option includeStackTracesInErrorResponses to includeStacktraceInErrorResponses, to match the extension name. (g) Removes a test around error handling a particular `yup` style of errors that is probably not relevant any more. This is to some degree part of #6719 because we're concerned about the effect of the `declare module` on the Gateway transition. --- .changeset/small-suits-compare.md | 14 ++ docs/source/migration.mdx | 109 +++++++++++- .../src/apolloServerTests.ts | 120 +------------ .../src/httpServerTests.ts | 104 ++++++++++- packages/server/src/ApolloServer.ts | 10 +- .../server/src/__tests__/ApolloServer.test.ts | 12 +- packages/server/src/__tests__/errors.test.ts | 162 +++++++++++------- .../server/src/__tests__/runQuery.test.ts | 6 +- packages/server/src/errorNormalize.ts | 42 ++--- packages/server/src/errors/index.ts | 18 ++ .../server/src/externalTypes/constructor.ts | 2 +- packages/server/src/externalTypes/graphql.ts | 3 +- packages/server/src/requestPipeline.ts | 6 +- 13 files changed, 372 insertions(+), 236 deletions(-) create mode 100644 .changeset/small-suits-compare.md diff --git a/.changeset/small-suits-compare.md b/.changeset/small-suits-compare.md new file mode 100644 index 00000000000..2974b1e6ef4 --- /dev/null +++ b/.changeset/small-suits-compare.md @@ -0,0 +1,14 @@ +--- +"@apollo/server-integration-testsuite": patch +"@apollo/server": patch +--- + +Refactor error formatting. + +Remove `error.extensions.exception`; you can add it back yourself with `formatError`. `error.extensions.exception.stacktrace` is now available on `error.extensions.stacktrace`. + +Provide `unwrapResolverError` function in `@apollo/server/errors`; useful for your `formatError` hook. + +No more TS `declare module` describing the `exception` extension (partially incorrectly). + +Rename the (new in v4) constructor option `includeStackTracesInErrorResponses` to `includeStacktraceInErrorResponses`. diff --git a/docs/source/migration.mdx b/docs/source/migration.mdx index 216ce1ca2a3..1dd298a7feb 100644 --- a/docs/source/migration.mdx +++ b/docs/source/migration.mdx @@ -166,10 +166,10 @@ The `startStandaloneServer` function accepts two arguments; the first is the ins An optional `listen` configuration option. The `listen` option accepts an object with the same properties as the [`net.Server.listen` options object](https://nodejs.org/api/net.html#serverlistenoptions-callback). - + For example, in Apollo Server 3, if you used `server.listen(4321)`, you'll now pass `listen: { port: 4321 }` to the `startStandaloneServer` function in Apollo Server 4. If you didn't pass any arguments to Apollo Server 3's `server.listen()` method; you don't need to specify this `listen` option. - + @@ -654,15 +654,15 @@ In Apollo Server 3, the `debug` constructor option (which defaults to `true` unl - Apollo Server 3 rarely sends messages at the `DEBUG` level, so this primarily affects plugins that use the provided `logger` to send `DEBUG` messages. - The `debug` flag is also available to plugins on `GraphQLRequestContext` to use as they wish. -Apollo Server 4 removes the `debug` constructor option. In its place is a new `includeStackTracesInErrorResponses` option which controls its namesake feature. Like `debug`, this option defaults to `true` unless the `NODE_ENV` environment variable is either `production` or `test`. +Apollo Server 4 removes the `debug` constructor option. In its place is a new `includeStacktraceInErrorResponses` option which controls its namesake feature. Like `debug`, this option defaults to `true` unless the `NODE_ENV` environment variable is either `production` or `test`. -If you use `debug` in Apollo Server 3, you can use `includeStackTracesInErrorResponses` with the same value in Apollo Server 4: +If you use `debug` in Apollo Server 3, you can use `includeStacktraceInErrorResponses` with the same value in Apollo Server 4: ```ts const apolloServerInstance = new ApolloServer({ typeDefs, resolvers, - includeStackTracesInErrorResponses: true, + includeStacktraceInErrorResponses: true, }); ``` @@ -679,6 +679,8 @@ const server = new ApolloServer({ }); ``` +(Note that the stack traces themselves have moved from `extensions.exception.stacktrace` to `extensions.stacktrace`.) + ### `formatResponse` hook Apollo Server 3 provides the `formatResponse` hook as a top-level constructor argument. The `formatResponse` hook is called after an operation successfully gets to the "execution" stage, enabling you to transform the structure of GraphQL response objects before sending them to a client. @@ -1082,7 +1084,9 @@ expect(result.data?.hello).toBe('Hello world!'); // -> true -### `formatError` hook improvements +### Error formatting changes + +#### `formatError` improvements Apollo Server 3 supports the `formatError` hook, which has the following signature: @@ -1100,12 +1104,17 @@ In Apollo Server 4, this becomes: Above, `formattedError` is the default JSON object sent in the response according to the [GraphQL specification](https://spec.graphql.org/draft/#sec-Errors), and `error` is the originally thrown error. If you need a field from the original error that isn't in `GraphQLFormattedError`, you can access that value from the `error` argument. +One caveat: if the error was thrown inside a resolver, `error` is not the error your resolver code threw; it is a `GraphQLError` wrapping it and adding helpful context such as the `path` in the operation to the resolver's field. If you want the exact error you threw, Apollo Server 4 provides the `unwrapResolverError` function in `@apollo/server/errors`, which removes this outer layer if you pass it a resolver error (and returns its argument otherwise). + So, you can format errors like this: ```ts +import { unwrapResolverError } from '@apollo/server/errors'; + +new ApolloServer({ formatError: (formattedError, error) => { // Don't give the specific errors to the client. - if (error instanceof CustomDBError) { + if (unwrapResolverError(error) instanceof CustomDBError) { return { message: 'Internal server error' }; } @@ -1122,8 +1131,92 @@ So, you can format errors like this: // be manipulated in other ways, as long as it's returned. return formattedError; }, + // ... +}); +``` + +#### `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 originalError = unwrapResolverError(error); + const exception: Record = { + ...(typeof originalError === 'object' ? originalError : 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. @@ -1390,7 +1483,7 @@ The `@apollo/utils.fetcher` package has a more precise name and only supports ar ### `@apollo/cache-control-types` -In Apollo Server 3, you could import the `CacheScope`, `CacheHint`, `CacheAnnotation`, `CachePolicy`, and `ResolveInfoCacheControl` types from your chosen Apollo Server package. +In Apollo Server 3, you could import the `CacheScope`, `CacheHint`, `CacheAnnotation`, `CachePolicy`, and `ResolveInfoCacheControl` types from your chosen Apollo Server package. In Apollo Server 4, the new `@apollo/cache-control-types` package exports the [`CacheScope`](#cachescope-type), `CacheHint`, `CacheAnnotation`, `CachePolicy`, and `ResolveInfoCacheControl` types. This enables libraries that work with multiple GraphQL servers (such as `@apollo/subgraph`) to refer to these types without depending on `@apollo/server`. diff --git a/packages/integration-testsuite/src/apolloServerTests.ts b/packages/integration-testsuite/src/apolloServerTests.ts index 88d6aacef6d..50699f8b761 100644 --- a/packages/integration-testsuite/src/apolloServerTests.ts +++ b/packages/integration-testsuite/src/apolloServerTests.ts @@ -775,110 +775,6 @@ export function defineIntegrationTestSuiteApolloServerTests( ); } }); - - it('works with errors similar to GraphQL errors, such as yup', async () => { - // https://npm.im/yup is a package that produces a particular type of - // error that we test compatibility with. This test was first brought - // with https://github.com/apollographql/apollo-server/pull/1288. We - // used to use the actual `yup` package to generate the error, but we - // don't need to actually bundle that dependency just to test - // compatibility with that particular error shape. To be honest, it's - // not clear from the original PR which attribute of this error need be - // mocked, but for the sake not not breaking anything, all of yup's - // error properties have been reproduced here. - const throwError = jest.fn(async () => { - // Intentionally `any` because this is a custom Error class with - // various custom properties (like `value` and `params`). - const yuppieError: any = new Error('email must be a valid email'); - yuppieError.name = 'ValidationError'; - - // Set `message` to enumerable, which `yup` does and `Error` doesn't. - Object.defineProperty(yuppieError, 'message', { - enumerable: true, - }); - - // Set other properties which `yup` sets. - yuppieError.path = 'email'; - yuppieError.type = undefined; - yuppieError.value = { email: 'invalid-email' }; - yuppieError.errors = ['email must be a valid email']; - yuppieError.inner = []; - yuppieError.params = { - path: 'email', - value: 'invalid-email', - originalValue: 'invalid-email', - label: undefined, - regex: /@/, - }; - - // This stack is fake, but roughly what `yup` generates! - yuppieError.stack = [ - 'ValidationError: email must be a valid email', - ' at createError (yup/lib/util/createValidation.js:64:35)', - ' at yup/lib/util/createValidation.js:113:108', - ' at process._tickCallback (internal/process/next_tick.js:68:7)', - ].join('\n'); - - throw yuppieError; - }); - - const formatError = jest.fn(() => { - return { - message: 'User Input Error', - extensions: { code: 'BAD_USER_INPUT' }, - }; - }); - - const uri = await createServerAndGetUrl({ - typeDefs: gql` - type Query { - fieldWhichWillError: String - } - `, - resolvers: { - Query: { - fieldWhichWillError: () => { - return throwError(); - }, - }, - }, - introspection: true, - includeStackTracesInErrorResponses: true, - formatError, - }); - - const apolloFetch = createApolloFetch({ uri }); - - const result = await apolloFetch({ - query: '{fieldWhichWillError}', - }); - - expect(throwError).toHaveBeenCalledTimes(1); - expect(formatError).toHaveBeenCalledTimes(1); - const formatErrorArgs: any = formatError.mock.calls[0]; - expect(formatErrorArgs[0]).toMatchObject({ - message: 'email must be a valid email', - path: ['fieldWhichWillError'], - 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); - - expect(result.data).toEqual({ fieldWhichWillError: null }); - expect(result.errors).toBeDefined(); - expect(result.errors[0].extensions.code).toEqual('BAD_USER_INPUT'); - expect(result.errors[0].message).toEqual('User Input Error'); - }); }); describe('lifecycle', () => { @@ -1020,7 +916,7 @@ export function defineIntegrationTestSuiteApolloServerTests( }), ...plugins, ], - includeStackTracesInErrorResponses: true, + includeStacktraceInErrorResponses: true, stopOnTerminationSignals: false, nodeEnv: '', ...constructorOptions, @@ -1459,7 +1355,7 @@ export function defineIntegrationTestSuiteApolloServerTests( }, ], formatError, - includeStackTracesInErrorResponses: true, + includeStacktraceInErrorResponses: true, }); const apolloFetch = createApolloFetch({ uri }); const result = await apolloFetch({ @@ -1691,7 +1587,7 @@ export function defineIntegrationTestSuiteApolloServerTests( expect(e.message).toMatch('valid result'); expect(e.extensions).toBeDefined(); expect(e.extensions.code).toEqual('SOME_CODE'); - expect(e.extensions.exception.stacktrace).toBeDefined(); + expect(e.extensions.stacktrace).toBeDefined(); expect(contextCreationDidFail.mock.calls).toMatchInlineSnapshot(` Array [ @@ -1765,7 +1661,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 +1692,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,8 +1724,8 @@ 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).toBeDefined(); - expect(result.errors[0].extensions.exception.stacktrace).toBeDefined(); + expect(result.errors[0].extensions).not.toHaveProperty('exception'); + expect(result.errors[0].extensions.stacktrace).toBeDefined(); }); it('shows error extensions in extensions (only!)', async () => { @@ -1850,7 +1746,7 @@ export function defineIntegrationTestSuiteApolloServerTests( }, stopOnTerminationSignals: false, nodeEnv: 'development', - includeStackTracesInErrorResponses: false, + includeStacktraceInErrorResponses: false, }); const apolloFetch = createApolloFetch({ uri }); diff --git a/packages/integration-testsuite/src/httpServerTests.ts b/packages/integration-testsuite/src/httpServerTests.ts index bafdd1c4ba7..fa0da3b06b7 100644 --- a/packages/integration-testsuite/src/httpServerTests.ts +++ b/packages/integration-testsuite/src/httpServerTests.ts @@ -47,6 +47,10 @@ import { } from '@jest/globals'; import type { Mock, SpyInstance } from 'jest-mock'; import { cacheControlFromInfo } from '@apollo/cache-control-types'; +import { + ApolloServerErrorCode, + unwrapResolverError, +} from '@apollo/server/errors'; const QueryRootType = new GraphQLObjectType({ name: 'QueryRoot', @@ -174,12 +178,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: { @@ -1401,23 +1414,96 @@ export function defineIntegrationTestSuiteHttpServerTests( }); }); - it('formatError receives error that passes instanceof checks', async () => { + it('formatError receives error that can be unwrapped to pass instanceof checks', async () => { const expected = '--blank--'; + let error: unknown; const app = await createApp({ schema, - formatError: (_, error) => { - expect(error instanceof Error).toBe(true); - expect(error instanceof GraphQLError).toBe(true); + formatError: (_, e) => { + error = e; 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(error).toBeInstanceOf(GraphQLError); + expect(error).toHaveProperty('path'); + expect(unwrapResolverError(error)).toBeInstanceOf(MyError); + }); + + it('formatError receives error that passes instanceof checks when GraphQLError', async () => { + const expected = '--blank--'; + let error: unknown; + const app = await createApp({ + schema, + formatError: (_, e) => { + error = e; + 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(error).toBeInstanceOf(GraphQLError); + // This is the locatedError so it's not our error. + expect(error).not.toBeInstanceOf(MyGraphQLError); + expect(error).toHaveProperty('path'); + expect(unwrapResolverError(error)).toBeInstanceOf(MyGraphQLError); + }); + + 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/ApolloServer.ts b/packages/server/src/ApolloServer.ts index d00718b4877..b8e279ff99f 100644 --- a/packages/server/src/ApolloServer.ts +++ b/packages/server/src/ApolloServer.ts @@ -150,7 +150,7 @@ export interface ApolloServerInternals { rootValue?: ((parsedQuery: DocumentNode) => any) | any; validationRules: Array<(context: ValidationContext) => any>; fieldResolver?: GraphQLFieldResolver; - includeStackTracesInErrorResponses: boolean; + includeStacktraceInErrorResponses: boolean; persistedQueries?: WithRequired; nodeEnv: string; allowBatchedHttpRequests: boolean; @@ -285,8 +285,8 @@ export class ApolloServer { ...(introspectionEnabled ? [] : [NoIntrospection]), ], fieldResolver: config.fieldResolver, - includeStackTracesInErrorResponses: - config.includeStackTracesInErrorResponses ?? + includeStacktraceInErrorResponses: + config.includeStacktraceInErrorResponses ?? (nodeEnv !== 'production' && nodeEnv !== 'test'), persistedQueries: config.persistedQueries === false @@ -1058,8 +1058,8 @@ export class ApolloServer { ]), completeBody: prettyJSONStringify({ errors: normalizeAndFormatErrors([error], { - includeStackTracesInErrorResponses: - this.internals.includeStackTracesInErrorResponses, + includeStacktraceInErrorResponses: + this.internals.includeStacktraceInErrorResponses, formatError: this.internals.formatError, }), }), diff --git a/packages/server/src/__tests__/ApolloServer.test.ts b/packages/server/src/__tests__/ApolloServer.test.ts index 9cceeb7adb3..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,7 +23,9 @@ const resolvers = { return 'world'; }, error() { - throw new Error('A test error'); + throw new GraphQLError('A test error', { + extensions: { someField: 'value' }, + }); }, contextFoo(_root: any, _args: any, context: any) { return context.foo; @@ -252,6 +254,7 @@ describe('ApolloServer executeOperation', () => { expect(result.errors).toHaveLength(1); expect(result.errors?.[0].extensions).toStrictEqual({ code: 'INTERNAL_SERVER_ERROR', + someField: 'value', }); await server.stop(); }); @@ -260,7 +263,7 @@ describe('ApolloServer executeOperation', () => { const server = new ApolloServer({ typeDefs, resolvers, - includeStackTracesInErrorResponses: true, + includeStacktraceInErrorResponses: true, }); await server.start(); @@ -271,7 +274,8 @@ describe('ApolloServer executeOperation', () => { expect(result.errors).toHaveLength(1); const extensions = result.errors?.[0].extensions; expect(extensions).toHaveProperty('code', 'INTERNAL_SERVER_ERROR'); - expect(extensions).toHaveProperty('exception.stacktrace'); + expect(extensions).toHaveProperty('stacktrace'); + 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 673e386719f..127368c4c71 100644 --- a/packages/server/src/__tests__/errors.test.ts +++ b/packages/server/src/__tests__/errors.test.ts @@ -1,81 +1,55 @@ -import { GraphQLError } from 'graphql'; +import { GraphQLError, GraphQLFormattedError } from 'graphql'; +import { unwrapResolverError } from '@apollo/server/errors'; 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 error = createFormattedError({ - includeStackTracesInErrorResponses: true, - }); + const thrown = new Error(message); + (thrown as any).key = key; + const [error] = normalizeAndFormatErrors( + [ + new GraphQLError(thrown.message, { + originalError: thrown, + extensions: { code, key }, + }), + ], + { includeStacktraceInErrorResponses: true }, + ); expect(error.message).toEqual(message); - expect(error.extensions.key).toEqual(key); - expect(error.extensions.exception.key).toBeUndefined(); - expect(error.extensions.code).toEqual(code); - // stacktrace should exist under exception - expect(error.extensions.exception.stacktrace).toBeDefined(); + expect(error.extensions?.key).toEqual(key); + expect(error.extensions).not.toHaveProperty('exception'); // Removed in AS4 + expect(error.extensions?.code).toEqual(code); + // stacktrace should exist + expect(error.extensions?.stacktrace).toBeDefined(); }); it('hides stacktrace by default', () => { const thrown = new Error(message); (thrown as any).key = key; const error = normalizeAndFormatErrors([ - new GraphQLError( - thrown.message, - undefined, - undefined, - undefined, - undefined, - thrown, - ), + new GraphQLError(thrown.message, { originalError: thrown }), ])[0]; expect(error.message).toEqual(message); expect(error.extensions?.code).toEqual('INTERNAL_SERVER_ERROR'); - expect(error.extensions?.exception).toHaveProperty('key', key); - // stacktrace should exist under exception - expect(error.extensions?.exception).not.toHaveProperty('stacktrace'); + expect(error.extensions).not.toHaveProperty('exception'); // Removed in AS4 + // stacktrace should not exist + expect(error.extensions).not.toHaveProperty('stacktrace'); }); - it('exposes fields on error under exception field and provides code', () => { - const error = createFormattedError(); + it('exposes extensions on error as extensions field and provides code', () => { + 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, { @@ -84,7 +58,7 @@ describe('Errors', () => { const formatError = jest.fn(); normalizeAndFormatErrors([error], { formatError, - includeStackTracesInErrorResponses: true, + includeStacktraceInErrorResponses: true, }); expect(formatError).toHaveBeenCalledTimes(1); @@ -100,5 +74,77 @@ 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 originalError = unwrapResolverError(error); + const exception: Record = { + ...(typeof originalError === 'object' ? originalError : 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/__tests__/runQuery.test.ts b/packages/server/src/__tests__/runQuery.test.ts index ee72ca098b1..d50df0d6f98 100644 --- a/packages/server/src/__tests__/runQuery.test.ts +++ b/packages/server/src/__tests__/runQuery.test.ts @@ -138,13 +138,13 @@ it('returns a syntax error if the query string contains one', async () => { // Maybe we used to log field errors and we want to make sure we don't do that? // Our Jest tests automatically fail if anything is logged to the console. it.each([true, false])( - 'does not call console.error if in an error occurs and includeStackTracesInErrorResponses is %s', - async (includeStackTracesInErrorResponses) => { + 'does not call console.error if in an error occurs and includeStacktraceInErrorResponses is %s', + async (includeStacktraceInErrorResponses) => { const query = `query { testError }`; const res = await runQuery( { schema, - includeStackTracesInErrorResponses, + includeStacktraceInErrorResponses, }, { query }, ); diff --git a/packages/server/src/errorNormalize.ts b/packages/server/src/errorNormalize.ts index aaeff4d0669..a2dd25063e4 100644 --- a/packages/server/src/errorNormalize.ts +++ b/packages/server/src/errorNormalize.ts @@ -7,16 +7,7 @@ import { } from 'graphql'; import { ApolloServerErrorCode } from './errors/index.js'; -declare module 'graphql' { - export interface GraphQLErrorExtensions { - code?: string; - exception?: { - stacktrace?: ReadonlyArray; - }; - } -} - -// This function accepts any value that were thrown and convert it to GraphQLFormatterError. +// This function accepts any value that were thrown and convert it to GraphQLFormattedError. // It also add default extensions.code and copy stack trace onto an extension if requested. // This function should not throw. export function normalizeAndFormatErrors( @@ -26,7 +17,7 @@ export function normalizeAndFormatErrors( formattedError: GraphQLFormattedError, error: unknown, ) => GraphQLFormattedError; - includeStackTracesInErrorResponses?: boolean; + includeStacktraceInErrorResponses?: boolean; } = {}, ): Array { const formatError = options.formatError ?? ((error) => error); @@ -34,8 +25,8 @@ export function normalizeAndFormatErrors( try { return formatError(enrichError(error), error); } catch (formattingError) { - if (options.includeStackTracesInErrorResponses) { - // includeStackTracesInErrorResponses is used in development + if (options.includeStacktraceInErrorResponses) { + // includeStacktraceInErrorResponses is used in development // so it will be helpful to show errors thrown by formatError hooks in that mode return enrichError(formattingError); } else { @@ -58,25 +49,12 @@ 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 = { - ...extensions.exception, - ...Object.fromEntries(originalErrorEnumerableEntries), - }; - } - } - - if (options.includeStackTracesInErrorResponses) { - extensions.exception = { - ...extensions.exception, - stacktrace: graphqlError.stack?.split('\n'), - }; + if (options.includeStacktraceInErrorResponses) { + // Note that if ensureGraphQLError created graphqlError from an + // originalError, graphqlError.stack will be the same as + // originalError.stack due to some special code in the GraphQLError + // constructor. + extensions.stacktrace = graphqlError.stack?.split('\n'); } return { ...graphqlError.toJSON(), extensions }; diff --git a/packages/server/src/errors/index.ts b/packages/server/src/errors/index.ts index d53a217d707..d816e826c2c 100644 --- a/packages/server/src/errors/index.ts +++ b/packages/server/src/errors/index.ts @@ -1,3 +1,5 @@ +import { GraphQLError } from 'graphql'; + export enum ApolloServerErrorCode { INTERNAL_SERVER_ERROR = 'INTERNAL_SERVER_ERROR', GRAPHQL_PARSE_FAILED = 'GRAPHQL_PARSE_FAILED', @@ -8,3 +10,19 @@ export enum ApolloServerErrorCode { OPERATION_RESOLUTION_FAILURE = 'OPERATION_RESOLUTION_FAILURE', BAD_REQUEST = 'BAD_REQUEST', } + +/** + * unwrapResolverError is a useful helper function for `formatError` hooks. + * Errors thrown in resolvers are wrapped by graphql-js in a GraphQLError that + * adds context such as the `path` to the field in the operation. If you'd like + * to look directly at the original error thrown in the resolver (with whatever + * data is on that error object, but without fields like `path`), you can use + * this function. Note that other GraphQLErrors that contain `originalError` + * (like parse errors) are not unwrapped by this function. + */ +export function unwrapResolverError(error: unknown): unknown { + if (error instanceof GraphQLError && error.path && error.originalError) { + return error.originalError; + } + return error; +} diff --git a/packages/server/src/externalTypes/constructor.ts b/packages/server/src/externalTypes/constructor.ts index 97e3e23607d..d31d078b662 100644 --- a/packages/server/src/externalTypes/constructor.ts +++ b/packages/server/src/externalTypes/constructor.ts @@ -98,7 +98,7 @@ interface ApolloServerOptionsBase { executor?: GraphQLExecutor; fieldResolver?: GraphQLFieldResolver; cache?: KeyValueCache; - includeStackTracesInErrorResponses?: boolean; + includeStacktraceInErrorResponses?: boolean; logger?: Logger; allowBatchedHttpRequests?: boolean; diff --git a/packages/server/src/externalTypes/graphql.ts b/packages/server/src/externalTypes/graphql.ts index abdcd10ebb5..bedd8b80308 100644 --- a/packages/server/src/externalTypes/graphql.ts +++ b/packages/server/src/externalTypes/graphql.ts @@ -69,7 +69,8 @@ export interface GraphQLRequestContext { * Unformatted errors which have occurred during the request. Note that these * are present earlier in the request pipeline and differ from **formatted** * errors which are the result of running the user-configurable `formatError` - * transformation function over specific errors. + * transformation function over specific errors; these can eventually be found + * in `response.result.errors`. */ readonly errors?: ReadonlyArray; diff --git a/packages/server/src/requestPipeline.ts b/packages/server/src/requestPipeline.ts index c1107ffe905..5ee1d80098d 100644 --- a/packages/server/src/requestPipeline.ts +++ b/packages/server/src/requestPipeline.ts @@ -559,12 +559,12 @@ export async function processGraphQLRequest( } function formatErrors( - errors: ReadonlyArray, + errors: ReadonlyArray, ): ReadonlyArray { return normalizeAndFormatErrors(errors, { formatError: internals.formatError, - includeStackTracesInErrorResponses: - internals.includeStackTracesInErrorResponses, + includeStacktraceInErrorResponses: + internals.includeStacktraceInErrorResponses, }); } }