Skip to content

Commit

Permalink
Error extensions improvements (#6759)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@glasser
glasser authored Aug 5, 2022
1 parent f87d67a commit 6ef6a09
Show file tree
Hide file tree
Showing 13 changed files with 372 additions and 236 deletions.
14 changes: 14 additions & 0 deletions .changeset/small-suits-compare.md
View file
Original file line number Diff line number Diff line change
@@ -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`.
109 changes: 101 additions & 8 deletions docs/source/migration.mdx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,10 +166,10 @@ The `startStandaloneServer` function accepts two arguments; the first is the ins
<td>

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.
</td>

</tr>

</tbody>
Expand Down Expand Up @@ -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<MyContext>({
typeDefs,
resolvers,
includeStackTracesInErrorResponses: true,
includeStacktraceInErrorResponses: true,
});
```

Expand All @@ -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.
Expand Down Expand Up @@ -1082,7 +1084,9 @@ expect(result.data?.hello).toBe('Hello world!'); // -> true

</MultiCodeBlock>

### `formatError` hook improvements
### Error formatting changes

#### `formatError` improvements

Apollo Server 3 supports the `formatError` hook, which has the following signature:

Expand All @@ -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' };
}

Expand All @@ -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:

<MultiCodeBlock>

```ts
function formatError(
formattedError: GraphQLFormattedError,
error: unknown,
) {
const originalError = unwrapResolverError(error);
const exception: Record<string, unknown> = {
...(typeof originalError === 'object' ? originalError : null),
};
delete exception.extensions;
if (formattedError.extensions?.stacktrace) {
exception.stacktrace = formattedError.extensions.stacktrace;
}
const extensions: Record<string, unknown> = {
...formattedError.extensions,
exception,
};
delete extensions.stacktrace;
return {
...formattedError,
extensions,
};
}
```

</MultiCodeBlock>

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.
Expand Down Expand Up @@ -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`.

Expand Down
120 changes: 8 additions & 112 deletions packages/integration-testsuite/src/apolloServerTests.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -1020,7 +916,7 @@ export function defineIntegrationTestSuiteApolloServerTests(
}),
...plugins,
],
includeStackTracesInErrorResponses: true,
includeStacktraceInErrorResponses: true,
stopOnTerminationSignals: false,
nodeEnv: '',
...constructorOptions,
Expand Down Expand Up @@ -1459,7 +1355,7 @@ export function defineIntegrationTestSuiteApolloServerTests(
},
],
formatError,
includeStackTracesInErrorResponses: true,
includeStacktraceInErrorResponses: true,
});
const apolloFetch = createApolloFetch({ uri });
const result = await apolloFetch({
Expand Down Expand Up @@ -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 [
Expand Down Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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 () => {
Expand All @@ -1850,7 +1746,7 @@ export function defineIntegrationTestSuiteApolloServerTests(
},
stopOnTerminationSignals: false,
nodeEnv: 'development',
includeStackTracesInErrorResponses: false,
includeStacktraceInErrorResponses: false,
});

const apolloFetch = createApolloFetch({ uri });
Expand Down
Loading

0 comments on commit 6ef6a09

Please sign in to comment.