Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return 400 on unknown operation name (version 4) #6652

Merged
merged 9 commits into from
Jul 12, 2022
102 changes: 102 additions & 0 deletions packages/integration-testsuite/src/httpServerTests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,108 @@ export function defineIntegrationTestSuiteHttpServerTests(
});
});

it('returns an error on parse failure', async () => {
const app = await createApp();
const res = await request(app).post('/').send({
query: `{`,
});

expect(res.status).toEqual(400);
expect(res.body).toMatchInlineSnapshot(`
Object {
"errors": Array [
Object {
"extensions": Object {
"code": "GRAPHQL_PARSE_FAILED",
},
"locations": Array [
Object {
"column": 2,
"line": 1,
},
],
"message": "Syntax Error: Expected Name, found <EOF>.",
},
],
}
`);
});

it('returns an error on validation failure', async () => {
const app = await createApp();
const res = await request(app).post('/').send({
query: `{ hello }`,
});

expect(res.status).toEqual(400);
expect(res.body).toMatchInlineSnapshot(`
Object {
"errors": Array [
Object {
"extensions": Object {
"code": "GRAPHQL_VALIDATION_FAILED",
},
"locations": Array [
Object {
"column": 3,
"line": 1,
},
],
"message": "Cannot query field \\"hello\\" on type \\"QueryType\\".",
},
],
}
`);
});

it('unknown operation name returns 400 and OPERATION_RESOLUTION_FAILURE', async () => {
const app = await createApp();
const res = await request(app).post('/').send({
query: `query BadName { testString }`,
operationName: 'NotBadName',
});

expect(res.status).toEqual(400);
expect(res.body).toMatchInlineSnapshot(`
Object {
"errors": Array [
Object {
"extensions": Object {
"code": "OPERATION_RESOLUTION_FAILURE",
},
"message": "Unknown operation named \\"NotBadName\\".",
},
],
}
`);
});

it('multiple operations with no `operationName` specified returns 400 and OPERATION_RESOLUTION_FAILURE', async () => {
const app = await createApp();
const res = await request(app)
.post('/')
.send({
query: `
query One { testString }
query Two { testString }
`,
});

expect(res.status).toEqual(400);
expect(res.body).toMatchInlineSnapshot(`
Object {
"errors": Array [
Object {
"extensions": Object {
"code": "OPERATION_RESOLUTION_FAILURE",
},
"message": "Must provide operation name if query contains multiple operations.",
},
],
}
`);
});

it('throws an error if GET query is missing', async () => {
const app = await createApp();
const res = await request(app)
Expand Down
22 changes: 22 additions & 0 deletions packages/server/src/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,28 @@ export class UserInputError extends GraphQLError {
}
}

export class OperationResolutionError extends GraphQLError {
constructor(
message: string,
options?: {
nodes?: ReadonlyArray<ASTNode> | undefined;
originalError?: Error;
extensions?: Record<string, any>;
},
) {
super(message, {
nodes: options?.nodes,
originalError: options?.originalError,
extensions: {
...options?.extensions,
code: 'OPERATION_RESOLUTION_FAILURE',
},
});

this.name = 'OperationResolutionError';
}
}

// TODO(AS4): We added this in AS4. Is that a good idea? We should at least
// document it, and maybe consider using it for more of the errors in
// runHttpQuery instead of just returning text/plain errors.
Expand Down
25 changes: 24 additions & 1 deletion packages/server/src/requestPipeline.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import {
SyntaxError,
ensureError,
normalizeAndFormatErrors,
OperationResolutionError,
} from './errors.js';
import type {
GraphQLRequestContext,
Expand Down Expand Up @@ -412,6 +413,23 @@ export async function processGraphQLRequest<TContext extends BaseContext>(
requestContext as GraphQLRequestContextExecutionDidStart<TContext>,
);

// If we don't have an operation, there's no reason to go further. We know
// `result` will consist of one error (returned by `graphql-js`'s
// `buildExecutionContext`).
if (!requestContext.operation) {
if (!result.errors?.length) {
throw new Error(
'Unexpected error: Apollo Server did not resolve an operation but execute did not return errors',
);
}
const error = result.errors[0];
throw new OperationResolutionError(error.message, {
nodes: error.nodes,
originalError: error.originalError,
extensions: error.extensions,
});
}

// The first thing that execution does is coerce the request's variables
// to the types declared in the operation, which can lead to errors if
// they are of the wrong type. It also makes sure that all non-null
Expand Down Expand Up @@ -450,7 +468,12 @@ export async function processGraphQLRequest<TContext extends BaseContext>(
executionListeners.map((l) => l.executionDidEnd?.(executionError)),
);

return await sendErrorResponse(executionError, newHTTPGraphQLHead(500));
const errorStatusCode =
executionMaybeError instanceof OperationResolutionError ? 400 : 500;
return await sendErrorResponse(
executionError,
newHTTPGraphQLHead(errorStatusCode),
);
}
}

Expand Down