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

Specifically handle the cases where an operation couldn't be resolved (either
due to an incorrect operationName or a missing operationName in the
presence of two operations).

In these cases, we know that buildExecutionContext will return a single error
and we know this is a client error. We can end execution and respond with a 400
along with a useful error code and the error from graphql-js.

This previously worked in v3 but regressed during our implementation of v4.

packages/integration-testsuite/src/httpServerTests.ts

@@ -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)

packages/server/src/errors.ts

@@ -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.

packages/server/src/requestPipeline.ts

@@ -25,6 +25,7 @@ import {
  SyntaxError,
  ensureError,
  normalizeAndFormatErrors,
+ OperationResolutionError,
 } from './errors.js';
 import type {
  GraphQLRequestContext,
@@ -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
@@ -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),
+ );
  }
  }