Skip to content

Commit

Permalink
Add error code to parse/validate errors passed to 'didEncounterErrors'
Browse files Browse the repository at this point in the history 
Implement fix proposed in apollographql#6558 but implement it on top of AS4
Also contain some cleanup and refactoring related to apollographql#6355
  • Loading branch information
@IvanGoncharov
IvanGoncharov committed Jun 14, 2022
1 parent e998f48 commit 2409356
Show file tree
Hide file tree
Showing 5 changed files with 83 additions and 65 deletions.
8 changes: 6 additions & 2 deletions packages/server/src/ApolloServer.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,11 @@ import Negotiator from 'negotiator';
import * as uuid from 'uuid';
import { newCachePolicy } from './cachePolicy';
import { determineApolloConfig } from './determineApolloConfig';
import { BadRequestError, ensureError, formatApolloErrors } from './errors';
import {
BadRequestError,
ensureError,
normalizeAndFormatErrors,
} from './errors';
import type {
ApolloServerPlugin,
BaseContext,
Expand Down Expand Up @@ -1081,7 +1085,7 @@ export class ApolloServer<TContext extends BaseContext = BaseContext> {
['content-type', 'application/json'],
]),
completeBody: prettyJSONStringify({
errors: formatApolloErrors([error], {
errors: normalizeAndFormatErrors([error], {
includeStackTracesInErrorResponses:
this.internals.includeStackTracesInErrorResponses,
formatError: this.internals.formatError,
Expand Down
20 changes: 10 additions & 10 deletions packages/server/src/__tests__/errors.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,10 @@ import {
UserInputError,
SyntaxError,
} from '..';
import { formatApolloErrors } from '../errors';
import { normalizeAndFormatErrors } from '../errors';

describe('Errors', () => {
describe('formatApolloErrors', () => {
describe('normalizeAndFormatErrors', () => {
type CreateFormatError =
| ((
options: Record<string, any>,
Expand All @@ -29,7 +29,7 @@ describe('Errors', () => {
const error = new GraphQLError(message, {
extensions: { code, key },
});
return formatApolloErrors(
return normalizeAndFormatErrors(
[
new GraphQLError(
error.message,
Expand All @@ -43,7 +43,7 @@ describe('Errors', () => {
options,
)[0];
} else {
return formatApolloErrors(errors, options);
return normalizeAndFormatErrors(errors, options);
}
};

Expand All @@ -61,7 +61,7 @@ describe('Errors', () => {
it('hides stacktrace by default', () => {
const thrown = new Error(message);
(thrown as any).key = key;
const error = formatApolloErrors([
const error = normalizeAndFormatErrors([
new GraphQLError(
thrown.message,
undefined,
Expand Down Expand Up @@ -89,7 +89,7 @@ describe('Errors', () => {
extensions: { code, key },
});
const formatError = jest.fn();
formatApolloErrors([error], {
normalizeAndFormatErrors([error], {
formatError,
includeStackTracesInErrorResponses: true,
});
Expand All @@ -104,7 +104,7 @@ describe('Errors', () => {
});
it('Formats native Errors in a JSON-compatible way', () => {
const error = new Error('Hello');
const [formattedError] = formatApolloErrors([error]);
const [formattedError] = normalizeAndFormatErrors([error]);
expect(JSON.parse(JSON.stringify(formattedError)).message).toBe('Hello');
});
});
Expand Down Expand Up @@ -140,14 +140,14 @@ describe('Errors', () => {
});
});
it('provides a syntax error', () => {
verifyError(new SyntaxError(message), {
verifyError(new SyntaxError(new GraphQLError(message)), {
code: 'GRAPHQL_PARSE_FAILED',
errorClass: SyntaxError,
name: 'SyntaxError',
});
});
it('provides a validation error', () => {
verifyError(new ValidationError(message), {
verifyError(new ValidationError(new GraphQLError(message)), {
code: 'GRAPHQL_VALIDATION_FAILED',
errorClass: ValidationError,
name: 'ValidationError',
Expand All @@ -166,7 +166,7 @@ describe('Errors', () => {
name: 'UserInputError',
});

const formattedError = formatApolloErrors([
const formattedError = normalizeAndFormatErrors([
new GraphQLError(
error.message,
undefined,
Expand Down
31 changes: 29 additions & 2 deletions packages/server/src/__tests__/runQuery.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -777,7 +777,7 @@ describe('request pipeline life-cycle hooks', () => {
},
];

it('called when an error occurs', async () => {
it('called when an parsing error occurs', async () => {
await runQuery(
{
schema,
Expand All @@ -788,7 +788,34 @@ describe('request pipeline life-cycle hooks', () => {

expect(didEncounterErrors).toBeCalledWith(
expect.objectContaining({
errors: expect.arrayContaining([expect.any(Error)]),
errors: expect.arrayContaining([
expect.objectContaining({
message: 'Syntax Error: Expected Name, found "}".',
extensions: { code: 'GRAPHQL_PARSE_FAILED' },
}),
]),
}),
);
});

it('called when a validation error occurs', async () => {
await runQuery(
{
schema,
plugins,
},
{ query: '{ testStringWithParseError }' },
);

expect(didEncounterErrors).toBeCalledWith(
expect.objectContaining({
errors: expect.arrayContaining([
expect.objectContaining({
message:
'Cannot query field "testStringWithParseError" on type "QueryType".',
extensions: { code: 'GRAPHQL_VALIDATION_FAILED' },
}),
]),
}),
);
});
Expand Down
57 changes: 25 additions & 32 deletions packages/server/src/errors.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,31 @@ declare module 'graphql' {
}

export class SyntaxError extends GraphQLError {
constructor(message: string) {
super(message, { extensions: { code: 'GRAPHQL_PARSE_FAILED' } });
constructor(graphqlError: GraphQLError) {
super(graphqlError.message, {
source: graphqlError.source,
positions: graphqlError.positions,
extensions: {
...graphqlError.extensions,
code: 'GRAPHQL_PARSE_FAILED',
},
originalError: graphqlError,
});

this.name = 'SyntaxError';
}
}

export class ValidationError extends GraphQLError {
constructor(message: string) {
super(message, { extensions: { code: 'GRAPHQL_VALIDATION_FAILED' } });
constructor(graphqlError: GraphQLError) {
super(graphqlError.message, {
nodes: graphqlError.nodes,
extensions: {
...graphqlError.extensions,
code: 'GRAPHQL_VALIDATION_FAILED',
},
originalError: graphqlError.originalError ?? graphqlError,
});

this.name = 'ValidationError';
}
Expand Down Expand Up @@ -102,46 +117,25 @@ export class BadRequestError extends GraphQLError {
}
}

// This function accepts any value that were thrown and convert it to GraphQLFormatterError.
// It also add default extensions.code and copy stack trace onto an extension if requested.
// This function should not throw.
export function formatApolloErrors(
export function normalizeAndFormatErrors(
errors: ReadonlyArray<unknown>,
options: {
formatError?: (
formattedError: GraphQLFormattedError,
error: unknown,
) => GraphQLFormattedError;
includeStackTracesInErrorResponses?: boolean;
errorCode?: string;
} = {},
): Array<GraphQLFormattedError> {
// Errors that occur in graphql-tools can contain an errors array that contains the errors thrown in a merged schema
// https://github.com/apollographql/graphql-tools/blob/3d53986ca/src/stitching/errors.ts#L104-L107
//
// They are are wrapped in an extra GraphQL error
// https://github.com/apollographql/graphql-tools/blob/3d53986ca/src/stitching/errors.ts#L109-L113
// which calls:
// https://github.com/graphql/graphql-js/blob/0a30b62964/src/error/locatedError.js#L18-L37
// Some processing for these nested errors could be done here:
//
// if (Array.isArray((error as any).errors)) {
// (error as any).errors.forEach(e => flattenedErrors.push(e));
// } else if (
// (error as any).originalError &&
// Array.isArray((error as any).originalError.errors)
// ) {
// (error as any).originalError.errors.forEach(e => flattenedErrors.push(e));
// } else {
// flattenedErrors.push(error);
// }

const { includeStackTracesInErrorResponses, errorCode } = options;

const formatError = options.formatError ?? ((error) => error);
return errors.map((error) => {
try {
return formatError(enrichError(error), error);
} catch (formattingError) {
if (includeStackTracesInErrorResponses) {
if (options.includeStackTracesInErrorResponses) {
// includeStackTracesInErrorResponses is used in development
// so it will be helpful to show errors thrown by formatError hooks in that mode
return enrichError(formattingError);
Expand All @@ -165,8 +159,7 @@ export function formatApolloErrors(

const extensions: GraphQLErrorExtensions = {
...graphqlError.extensions,
code:
graphqlError.extensions.code ?? errorCode ?? 'INTERNAL_SERVER_ERROR',
code: graphqlError.extensions.code ?? 'INTERNAL_SERVER_ERROR',
};

const { originalError } = graphqlError;
Expand All @@ -183,7 +176,7 @@ export function formatApolloErrors(
}
}

if (includeStackTracesInErrorResponses) {
if (options.includeStackTracesInErrorResponses) {
extensions.exception = {
...extensions.exception,
stacktrace: graphqlError.stack?.split('\n'),
Expand Down
32 changes: 13 additions & 19 deletions packages/server/src/requestPipeline.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,12 @@ import {
import {
PersistedQueryNotSupportedError,
PersistedQueryNotFoundError,
formatApolloErrors,
UserInputError,
BadRequestError,
ValidationError,
SyntaxError,
ensureError,
normalizeAndFormatErrors,
} from './errors';
import type {
GraphQLRequestContext,
Expand Down Expand Up @@ -111,7 +113,6 @@ export async function processGraphQLRequest<TContext extends BaseContext>(
if (!internals.persistedQueries) {
return await sendErrorResponse(
new PersistedQueryNotSupportedError(),
undefined,
// Not super clear why we need this to be uncached (makes sense for
// PersistedQueryNotFoundError, because there we're about to fill the
// cache and make the next copy of the same request succeed) but we've
Expand All @@ -133,7 +134,6 @@ export async function processGraphQLRequest<TContext extends BaseContext>(
} else {
return await sendErrorResponse(
new PersistedQueryNotFoundError(),
undefined,
getPersistedQueryErrorHttp(),
);
}
Expand Down Expand Up @@ -217,7 +217,11 @@ export async function processGraphQLRequest<TContext extends BaseContext>(
await parsingDidEnd(syntaxError as Error);
// XXX: This cast is pretty sketchy, as other error types can be thrown
// by parsingDidEnd!
return await sendErrorResponse(syntaxError, 'GRAPHQL_PARSE_FAILED');
return await sendErrorResponse(
syntaxError instanceof GraphQLError
? new SyntaxError(syntaxError)
: syntaxError,
);
}

const validationDidEnd = await invokeDidStartHook(
Expand All @@ -239,8 +243,7 @@ export async function processGraphQLRequest<TContext extends BaseContext>(
} else {
await validationDidEnd(validationErrors);
return await sendErrorResponse(
validationErrors,
'GRAPHQL_VALIDATION_FAILED',
validationErrors.map((error) => new ValidationError(error)),
);
}

Expand Down Expand Up @@ -294,7 +297,6 @@ export async function processGraphQLRequest<TContext extends BaseContext>(
new BadRequestError(
`GET requests only support query operations, not ${operation.operation} operations`,
),
undefined,
{ statusCode: 405, headers: new HeaderMap([['allow', 'POST']]) },
);
}
Expand All @@ -312,7 +314,6 @@ export async function processGraphQLRequest<TContext extends BaseContext>(
// by didResolveOperation!
return await sendErrorResponse(
err as GraphQLError,
undefined,
newHTTPGraphQLHead(500),
);
}
Expand Down Expand Up @@ -449,11 +450,7 @@ export async function processGraphQLRequest<TContext extends BaseContext>(
executionListeners.map((l) => l.executionDidEnd?.(executionError)),
);

return await sendErrorResponse(
executionError,
undefined,
newHTTPGraphQLHead(500),
);
return await sendErrorResponse(executionError, newHTTPGraphQLHead(500));
}
}

Expand Down Expand Up @@ -534,7 +531,6 @@ export async function processGraphQLRequest<TContext extends BaseContext>(
// Then, if the HTTP status code is not yet set, it sets it to 400.
async function sendErrorResponse(
errorOrErrors: ReadonlyArray<unknown> | unknown,
errorCode?: string,
http: HTTPGraphQLHead = newHTTPGraphQLHead(),
) {
// If a single error is passed, it should still be encapsulated in an array.
Expand All @@ -545,7 +541,7 @@ export async function processGraphQLRequest<TContext extends BaseContext>(
await didEncounterErrors(errors);

requestContext.response.result = {
errors: formatErrors(errors, errorCode),
errors: formatErrors(errors),
};

updateResponseHTTP(http);
Expand All @@ -558,11 +554,9 @@ export async function processGraphQLRequest<TContext extends BaseContext>(
}

function formatErrors(
errors: ReadonlyArray<GraphQLError>,
errorCode?: string,
errors: ReadonlyArray<unknown>,
): ReadonlyArray<GraphQLFormattedError> {
return formatApolloErrors(errors, {
errorCode,
return normalizeAndFormatErrors(errors, {
formatError: internals.formatError,
includeStackTracesInErrorResponses:
internals.includeStackTracesInErrorResponses,
Expand Down

0 comments on commit 2409356

Please sign in to comment.