Skip to content

Commit

Permalink
Only one way of dealing with ApolloError extensions (#5294)
Browse files Browse the repository at this point in the history
In Apollo Server 2, we had two different ways to specify extensions to
`ApolloError`:

    new ApolloError(message, code, {ext1: value})
    new ApolloError(message, code, {extensions: {ext1: value}})

and two different ways to read those extensions back:

    error.extensions.ext1
    error.ext1

This also meant that the extension values showed up twice in the
user-exposed errors list.

In Apollo Server 3, we have just one way to do it: the first line of
each of those pairs. That is, we treat the third argument to the
ApolloError constructor as "the extensions", not "the extensions or
maybe something wrapping the extensions"; and we only put the extensions
on `error.extensions`, not directly on `error` as well. (Note that the
built in `code` and `exception` extensions already only lived on
`extensions` rather than showing up twice!)

If you try to use the old `{extensions: {...}}` method, the ApolloServer
constructor throws instead of creating an extension named `extensions`.

Fixes #3835.
  • Loading branch information
glasser authored Jun 9, 2021
1 parent 45e69e0 commit 36410aa
Show file tree
Hide file tree
Showing 4 changed files with 81 additions and 53 deletions.
23 changes: 13 additions & 10 deletions packages/apollo-server-core/src/__tests__/errors.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,12 @@ describe('Errors', () => {
});
it('allows code setting and additional properties', () => {
const code = 'CODE';
const key = 'key';
const key = 'value';
const error = new ApolloError(message, code, { key });
expect(error.message).toEqual(message);
expect(error.key).toEqual(key);
expect(error.key).toBeUndefined();
expect(error.extensions.code).toEqual(code);
expect(error.extensions.key).toEqual(key);
});
});

Expand All @@ -37,7 +38,7 @@ describe('Errors', () => {
| ((options?: Record<string, any>) => Record<string, any>);
const message = 'message';
const code = 'CODE';
const key = 'key';
const key = 'value';

const createFormattedError: CreateFormatError = (
options?: Record<string, any>,
Expand Down Expand Up @@ -66,7 +67,8 @@ describe('Errors', () => {
it('exposes a stacktrace in debug mode', () => {
const error = createFormattedError({ debug: true });
expect(error.message).toEqual(message);
expect(error.extensions.exception.key).toEqual(key);
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();
Expand All @@ -93,10 +95,9 @@ describe('Errors', () => {
it('exposes fields on error under exception field and provides code', () => {
const error = createFormattedError();
expect(error.message).toEqual(message);
expect(error.extensions.exception.key).toEqual(key);
expect(error.extensions.key).toEqual(key);
expect(error.extensions.exception).toBeUndefined();
expect(error.extensions.code).toEqual(code);
// stacktrace should exist under exception
expect(error.extensions.exception.stacktrace).toBeUndefined();
});
it('calls formatter after exposing the code and stacktrace', () => {
const error = new ApolloError(message, code, { key });
Expand All @@ -106,7 +107,8 @@ describe('Errors', () => {
debug: true,
});
expect(error.message).toEqual(message);
expect(error.key).toEqual(key);
expect(error.extensions.key).toEqual(key);
expect(error.key).toBeUndefined();
expect(error.extensions.code).toEqual(code);
expect(error instanceof ApolloError).toBe(true);
expect(formatter).toHaveBeenCalledTimes(1);
Expand Down Expand Up @@ -184,8 +186,9 @@ describe('Errors', () => {
),
])[0];

expect(formattedError.extensions.exception.field1).toEqual('property1');
expect(formattedError.extensions.exception.field2).toEqual('property2');
expect(formattedError.extensions.field1).toEqual('property1');
expect(formattedError.extensions.field2).toEqual('property2');
expect(formattedError.extensions.exception).toBeUndefined();
});
});
});
17 changes: 9 additions & 8 deletions packages/apollo-server-errors/src/__tests__/ApolloError.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,14 @@ describe('ApolloError', () => {
);
});

it('(back-compat) sets extensions correctly for users who use an extensions key in the third constructor argument', () => {
const error = new ApolloError('My original message', 'A_CODE', {
extensions: {
arbitrary: 'user_data',
},
});

expect(error.extensions.arbitrary).toEqual('user_data');
it('throws for users who use an extensions key in the third constructor argument', () => {
expect(
() =>
new ApolloError('My original message', 'A_CODE', {
extensions: {
arbitrary: 'user_data',
},
}),
).toThrow(/Pass extensions directly/);
});
});
59 changes: 24 additions & 35 deletions packages/apollo-server-errors/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@
import { ASTNode, GraphQLError, GraphQLFormattedError, Source, SourceLocation } from 'graphql';
import {
ASTNode,
GraphQLError,
GraphQLFormattedError,
Source,
SourceLocation,
} from 'graphql';

export class ApolloError extends Error implements GraphQLError {
public extensions: Record<string, any>;
Expand All @@ -19,39 +25,20 @@ export class ApolloError extends Error implements GraphQLError {
) {
super(message);

// This variable was previously named `properties`, which allowed users to set
// arbitrary properties on the ApolloError object. This use case is still supported,
// but deprecated in favor of using the ApolloError.extensions object instead.
// This change intends to comply with the GraphQL spec on errors. See:
// https://github.com/graphql/graphql-spec/blob/master/spec/Section%207%20--%20Response.md#response-format
//
// Going forward, users should use the ApolloError.extensions object for storing
// and reading arbitrary data on an error, as arbitrary properties on the ApolloError
// itself won't be supported in the future.
//
// XXX Filter 'message' and 'extensions' specifically so they don't overwrite the class property.
// We _could_ filter all of the class properties, but have chosen to only do
// so if it's an issue for other users. Please feel free to open an issue if you
// find yourself here with this exact problem.
if (extensions) {
Object.keys(extensions)
.filter(keyName => keyName !== 'message' && keyName !== 'extensions')
.forEach(key => {
this[key] = extensions[key];
});
}

// if no name provided, use the default. defineProperty ensures that it stays non-enumerable
if (!this.name) {
Object.defineProperty(this, 'name', { value: 'ApolloError' });
}

// Before the mentioned change to extensions, users could previously set the extensions
// object by providing it as a key on the third argument to the constructor.
// This step provides backwards compatibility for those hypothetical users.
const userProvidedExtensions = (extensions && extensions.extensions) || null;
if (extensions?.extensions) {
throw Error(
'Pass extensions directly as the third argument of the ApolloError constructor: `new ' +
'ApolloError(message, code, {myExt: value})`, not `new ApolloError(message, code, ' +
'{extensions: {myExt: value}})`',
);
}

this.extensions = { ...extensions, ...userProvidedExtensions, code };
this.extensions = { ...extensions, code };
}
}

Expand Down Expand Up @@ -164,8 +151,10 @@ export function fromGraphQLError(error: GraphQLError, options?: ErrorOptions) {
// copy the original error, while keeping all values non-enumerable, so they
// are not printed unless directly referenced
Object.defineProperty(copy, 'originalError', { value: {} });
Object.getOwnPropertyNames(error).forEach(key => {
Object.defineProperty(copy.originalError, key, { value: (error as any)[key] });
Object.getOwnPropertyNames(error).forEach((key) => {
Object.defineProperty(copy.originalError, key, {
value: (error as any)[key],
});
});

return copy;
Expand Down Expand Up @@ -224,8 +213,8 @@ export class PersistedQueryNotSupportedError extends ApolloError {
}

export class UserInputError extends ApolloError {
constructor(message: string, properties?: Record<string, any>) {
super(message, 'BAD_USER_INPUT', properties);
constructor(message: string, extensions?: Record<string, any>) {
super(message, 'BAD_USER_INPUT', extensions);

Object.defineProperty(this, 'name', { value: 'UserInputError' });
}
Expand All @@ -239,7 +228,7 @@ export function formatApolloErrors(
},
): Array<ApolloError> {
if (!options) {
return errors.map(error => enrichError(error));
return errors.map((error) => enrichError(error));
}
const { formatter, debug } = options;

Expand All @@ -263,7 +252,7 @@ export function formatApolloErrors(
// flattenedErrors.push(error);
// }

const enrichedErrors = errors.map(error => enrichError(error, debug));
const enrichedErrors = errors.map((error) => enrichError(error, debug));
const makePrintable = (error: GraphQLFormattedError) => {
if (error instanceof Error) {
// Error defines its `message` and other fields as non-enumerable, meaning JSON.stringigfy does not print them.
Expand All @@ -282,7 +271,7 @@ export function formatApolloErrors(
return enrichedErrors;
}

return enrichedErrors.map(error => {
return enrichedErrors.map((error) => {
try {
return makePrintable(formatter(error));
} catch (err) {
Expand Down
35 changes: 35 additions & 0 deletions packages/apollo-server-integration-testsuite/src/ApolloServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ import {
ApolloServerPluginUsageReportingOptions,
ApolloServerPluginLandingPageDisabled,
ApolloServerPluginLandingPageGraphQLPlayground,
ApolloError,
} from 'apollo-server-core';
import { Headers, fetch } from 'apollo-server-env';
import ApolloServerPluginResponseCache from 'apollo-server-plugin-response-cache';
Expand Down Expand Up @@ -1649,6 +1650,40 @@ export function testApolloServer<AS extends ApolloServerBase>(
expect(result.errors[0].extensions.code).toEqual('UNAUTHENTICATED');
expect(result.errors[0].extensions.exception).toBeUndefined();
});

it('shows ApolloError extensions in extensions (only!)', async () => {
const { url: uri } = await createApolloServer({
typeDefs: gql`
type Query {
fieldWhichWillError: String
}
`,
resolvers: {
Query: {
fieldWhichWillError: () => {
throw new ApolloError('Some message', 'SOME_CODE', {
ext1: 'myext',
});
},
},
},
stopOnTerminationSignals: false,
__testing_nodeEnv__: 'development',
});

const apolloFetch = createApolloFetch({ uri });

const result = await apolloFetch({ query: `{fieldWhichWillError}` });
expect(result.data).toEqual({ fieldWhichWillError: null });

expect(result.errors).toBeDefined();
expect(result.errors.length).toEqual(1);
expect(result.errors[0].message).toEqual('Some message');
expect(result.errors[0].extensions.code).toEqual('SOME_CODE');
expect(result.errors[0].extensions.ext1).toEqual('myext');
expect(result.errors[0].extensions.exception).toBeDefined();
expect(result.errors[0].extensions.exception.ext1).toBeUndefined();
});
});

describe('Persisted Queries', () => {
Expand Down

0 comments on commit 36410aa

Please sign in to comment.