From 15b1cb2e96d9ede9007d22f33b2f5a745f071dba Mon Sep 17 00:00:00 2001 From: David Glasser Date: Mon, 29 Aug 2022 16:28:38 -0700 Subject: [PATCH] New error extension `http` for setting status/headers in context, resolvers (#6857) You can now add a `Partial` as an `http` error extension, which provides an easier way of setting HTTP response headers or status codes. These are honored when thrown from resolvers and `context` functions (and are also used internally to implement bad request errors etc). Multiple resolver errors can have `http` extensions to set status code and distinct headers separately; we do not commit to the semantics of what happens if multiple errors set the status code or set the same header. Now that you have more control over `context` error handling, simplify the default behavior: if the error thrown from `context` is not a `GraphQLError` with an `http` extension with a `status`, the default status code is 500 rather than sometimes being 400 based on `extensions.code`. Only prepend `Context creation failed: ` to the error message if the error is not already a GraphQLError. This still keeps the helpful message for developers while allowing you to remove it if you don't want it to be visible in your app. Fixes #6140. Fixes #5636. Fixes #6840. Co-authored-by: Rose M Koron <32436232+rkoron007@users.noreply.github.com> --- .changeset/witty-squids-vanish.md | 6 + docs/source/data/errors.mdx | 54 ++++- docs/source/data/resolvers.mdx | 27 +++ docs/source/migration.mdx | 12 +- .../src/apolloServerTests.ts | 226 ++++++++++++++---- .../src/httpServerTests.ts | 99 ++++++++ packages/server/src/ApolloServer.ts | 59 ++--- packages/server/src/__tests__/errors.test.ts | 14 +- .../server/src/__tests__/runQuery.test.ts | 10 +- packages/server/src/errorNormalize.ts | 79 ++++-- packages/server/src/internalErrorClasses.ts | 40 +++- packages/server/src/requestPipeline.ts | 112 ++++----- packages/server/src/runHttpQuery.ts | 34 ++- 13 files changed, 577 insertions(+), 195 deletions(-) create mode 100644 .changeset/witty-squids-vanish.md diff --git a/.changeset/witty-squids-vanish.md b/.changeset/witty-squids-vanish.md new file mode 100644 index 00000000000..e63c3e45958 --- /dev/null +++ b/.changeset/witty-squids-vanish.md @@ -0,0 +1,6 @@ +--- +"@apollo/server-integration-testsuite": patch +"@apollo/server": patch +--- + +Errors thrown in resolvers and context functions can use `extensions.http` to affect the response status code and headers. The default behavior when a context function throws is now to always use status code 500 rather than comparing `extensions.code` to `INTERNAL_SERVER_ERROR`. diff --git a/docs/source/data/errors.mdx b/docs/source/data/errors.mdx index 9f7ff47c229..39c31acae51 100644 --- a/docs/source/data/errors.mdx +++ b/docs/source/data/errors.mdx @@ -604,13 +604,59 @@ In this case, the error above is reported to Apollo Studio as: The REDACTED doesn't have sufficient privileges. ``` -## Returning HTTP status codes +## Setting HTTP status code and headers GraphQL, by design, does not use the same conventions from REST to communicate via HTTP verbs and status codes. Client information should be contained in the schema or as part of the standard response `errors` field. We recommend using the included [Error Codes](#built-in-error-codes) or [Custom Errors](#custom-errors) for error consistency rather than directly modifying the HTTP response. -You can set custom fields on your HTTP response by using a [plugin](/apollo-server/integrations/plugins). Be aware that GraphQL client libraries may not treat all response status codes the same, and so it will be up to your team to decide what patterns to use. +Apollo Server uses different HTTP status codes in various situations: +- If Apollo Server hasn't correctly started up or is in the process of shutting down, it responds with a 500 status code. + - The former can happen if you use a serverless integration and it sends requests to an Apollo Server instance that had an error on startup. The latter happens if you aren't properly [draining your server](/apollo-server/api/plugin/drain-http-server/#using-the-plugin). +- If Apollo Server can't parse the request into a legal GraphQL document and validate it against your schema, it responds with a 400 status code. This can also happen with other request problems, such as if a client attempts to send a batched HTTP request when `allowBatchedHttpRequests` isn't enabled or if CSRF prevention blocks a request. +- If a request uses an invalid HTTP method (`GET` with a mutation, or any HTTP method other than `GET` or `POST`), then Apollo Server responds with a 405 status code. +- If your `context` function throws, Apollo Server responds with a 500 status code. +- If there is an unexpected error during the processing of the request (either a bug in Apollo Server or a plugin hook throws), Apollo Server responds with a 500 status code. +- Otherwise, Apollo Server returns a 200 status code. This is essentially the case where the server can execute the GraphQL operation, and execution completes successfully (though this can still include resolver-specific errors). -As an example, here is how you could set a custom response header and status code based on a GraphQL error: +There are three ways to change an HTTP status code or set custom response headers, you can: throw an error in a resolver, throw an error in your `context` function, or write a [plugin](/apollo-server/integrations/plugins). + +While Apollo Server does enable you to set HTTP status codes based on errors thrown by resolvers, best practices for GraphQL over HTTP encourage sending 200 whenever an operation executes. So, we don't recommend using this mechanism in resolvers, just in the `context` function or in a plugin hooking into an early stage of the request pipeline. + +Be aware that GraphQL client libraries might not treat all response status codes the same, so it will be up to your team to decide which patterns to use. + +To change the HTTP status code and response headers based on an error thrown in either a resolver or `context` function, throw a `GraphQLError` with an `http` extension, like so: + + + +```ts +import { GraphQLError } from 'graphql'; + +const resolvers = { + Query: { + someField() { + throw new GraphQLError('the error message', { + extensions: { + code: 'SOMETHING_BAD_HAPPENED', + http: { + status: 404, + headers: new Map([ + ['some-header', 'it was bad'], + ['another-header', 'seriously'], + ]), + }, + }, + }); + } + } +} +``` + + + +You don't need to include `status` unless you want to override the default status code (200 for a resolver or 500 for a `context` function). The optional `headers` field should provide a `Map` with lowercase header names. + +If your setup includes multiple resolvers which throw errors that set status codes or set the same header, Apollo Server might resolve this conflict in an arbitrary way (which could change in future versions). Instead, we recommend writing a plugin (as shown below). + +You can also set the HTTP status code and headers from a plugin. As an example, here is how you could set a custom response header and status code based on a GraphQL error: ```ts const setHttpPlugin = { @@ -618,7 +664,7 @@ const setHttpPlugin = { return { async willSendResponse({ response }) { response.http.headers.set('custom-header', 'hello'); - if (response?.result?.errors?.[0]?.message === 'teapot') { + if (response?.result?.errors?.[0]?.extensions?.code === 'TEAPOT') { response.http.status = 418; } }, diff --git a/docs/source/data/resolvers.mdx b/docs/source/data/resolvers.mdx index dcadf7bac40..242b20bf53d 100644 --- a/docs/source/data/resolvers.mdx +++ b/docs/source/data/resolvers.mdx @@ -450,6 +450,33 @@ context: async () => ({ } ``` +#### Throwing errors in `context` + +By default, if your `context` function throws an error, Apollo Server returns that error to the user in a JSON response with an HTTP status code of 500. If the thrown error is not a `GraphQLError`, the error's message will be prepended with `"Context creation failed: "`. + +You can control the HTTP status code of an error by throwing a [`GraphQLError` with an `http` extension](./errors/#setting-http-status-code-and-headers). For example: + +```ts +context: async ({ req }) => { + const user = await getUserFromReq(req); + if (!user) { + throw new GraphQLError('User is not authenticated', { + extensions: { + code: 'UNAUTHENTICATED', + http: { status: 401 }, + } + }); + } + + // If this throws a non-GraphQLError, it will be rendered with + // `code: "INTERNAL_SERVER_ERROR"` and HTTP status code 500, and + // with a message starting with "Context creation failed: ". + const db = await getDatabaseConnection(); + + return { user, db }; +}, +```` + ## Return values A resolver function's return value is treated differently by Apollo Server depending on its type: diff --git a/docs/source/migration.mdx b/docs/source/migration.mdx index a5ad5927b9a..5fec6253317 100644 --- a/docs/source/migration.mdx +++ b/docs/source/migration.mdx @@ -520,7 +520,7 @@ class ContextValue { public dataSources: { moviesAPI: MoviesAPI; }; - + constructor({ req, server }: { req: IncomingMessage; server: ApolloServer }) { this.token = getTokenFromRequest(req); const { cache } = server; @@ -1302,9 +1302,9 @@ Apollo Server 3.5.0 and newer included a TypeScript `declare module` declaration -### HTTP error handling changes +### Improvements to error handling outside of resolvers -Apollo Server 3 returns specific errors relating to GraphQL operations over HTTP/JSON as `text/plain` error messages. +Apollo Server 3 returns some errors relating to GraphQL operations over HTTP/JSON as `text/plain` error messages. Apollo Server 4 now returns all non-landing-page-related responses as `application/json` JSON responses. This means all single-error responses render like any other GraphQL error: @@ -1318,8 +1318,12 @@ Additionally, the [`formatError` hook](/apollo-server/data/errors/#for-client-re Apollo Server 4 also introduces new plugin hooks `startupDidFail`, `contextCreationDidFail`, `invalidRequestWasReceived`, and `unexpectedErrorProcessingRequest`, enabling plugins to observe errors in new settings. +In Apollo Server 3, if your `context` function throws, then the string `"Context creation failed: "` is *always* prepended to its message, and the error is rendered with HTTP status code 500 (if the error is a GraphQLError with `extensions.code` equal to `INTERNAL_SERVER_ERROR`) or 400. You cannot select a different HTTP status code or control HTTP response headers. + In Apollo Server 4, if either the `resolveOperation` or `execute` function throws an error, that error is rendered with the HTTP status code 500 (rather than 400). Note that the `execute` function commonly returns a non-empty list of errors rather than throwing an explicit error. +In Apollo Server 4, if your `context` function throws, the string `"Context creation failed: "` is only prepended to the message if the thrown error was not a `GraphQLError`. There is no special-casing of `extensions.code`; instead, you can use [`extensions.http`](./data/errors/#setting-http-status-code-and-headers) to set the HTTP status code or headers. If this extension is not provided, the status code defaults to 500 (not 400). + ### Warning for servers without draining @@ -1618,7 +1622,7 @@ new ApolloServer({ ## Renamed packages The following packages have been renamed in Apollo Server 4: - - `apollo-datasource-rest` is now [`@apollo/datasource-rest`](https://www.npmjs.com/package/@apollo/datasource-rest). + - `apollo-datasource-rest` is now [`@apollo/datasource-rest`](https://www.npmjs.com/package/@apollo/datasource-rest). - `apollo-server-plugin-response-cache` is now [`@apollo/server-plugin-response-cache`](https://www.npmjs.com/package/@apollo/server-plugin-response-cache). - `apollo-server-plugin-operation-registry` is now [`@apollo/server-plugin-operation-registry`](https://www.npmjs.com/package/@apollo/server-plugin-operation-registry). - `apollo-reporting-protobuf` (an internal implementation detail for the usage reporting plugin) is now [`@apollo/usage-reporting-protobuf`](https://www.npmjs.com/package/@apollo/usage-reporting-protobuf). diff --git a/packages/integration-testsuite/src/apolloServerTests.ts b/packages/integration-testsuite/src/apolloServerTests.ts index 69bbe728301..e62b2651b43 100644 --- a/packages/integration-testsuite/src/apolloServerTests.ts +++ b/packages/integration-testsuite/src/apolloServerTests.ts @@ -35,7 +35,7 @@ import type { BaseContext, ApolloServerPlugin, } from '@apollo/server'; -import fetch from 'node-fetch'; +import fetch, { type Headers } from 'node-fetch'; import resolvable, { Resolvable } from '@josephg/resolvable'; import type { AddressInfo } from 'net'; @@ -1653,58 +1653,192 @@ export function defineIntegrationTestSuiteApolloServerTests( expect(spy).toHaveBeenCalledTimes(1); }); - it('returns thrown context error as a valid graphql result', async () => { - const typeDefs = gql` - type Query { - hello: String - } - `; - const resolvers = { - Query: { - hello: () => { - throw Error('never get here'); + describe('context errors', () => { + async function run(errorToThrow: Error) { + const typeDefs = gql` + type Query { + hello: String + } + `; + const resolvers = { + Query: { + hello: () => { + throw Error('never get here'); + }, }, - }, - }; - const contextCreationDidFail = jest.fn<() => Promise>(); - const uri = await createServerAndGetUrl( - { - typeDefs, - resolvers, - stopOnTerminationSignals: false, - nodeEnv: '', - plugins: [{ contextCreationDidFail }], - }, - { - context: async () => { - throw new GraphQLError('valid result', { - extensions: { code: 'SOME_CODE' }, - }); + }; + const contextCreationDidFail = jest.fn<() => Promise>(); + const uri = await createServerAndGetUrl( + { + typeDefs, + resolvers, + stopOnTerminationSignals: false, + includeStacktraceInErrorResponses: false, + plugins: [{ contextCreationDidFail }], + formatError(formattedError) { + return { + ...formattedError, + extensions: { + ...formattedError.extensions, + formatErrorCalled: true, + }, + }; + }, }, - }, - ); + { + context: async () => { + throw errorToThrow; + }, + }, + ); - const apolloFetch = createApolloFetch({ uri }); + let status = 0; + let headers: Headers | undefined; + const apolloFetch = createApolloFetch({ uri }).useAfter( + (res, next) => { + status = res.response.status; + headers = res.response.headers; + next(); + }, + ); - const result = await apolloFetch({ query: '{hello}' }); - expect(result.errors.length).toEqual(1); - expect(result.data).toBeUndefined(); + const result = await apolloFetch({ query: '{hello}' }); - const e = result.errors[0]; - expect(e.message).toMatch('valid result'); - expect(e.extensions).toBeDefined(); - expect(e.extensions.code).toEqual('SOME_CODE'); - expect(e.extensions.stacktrace).toBeDefined(); + return { + result, + status, + specialHeader: headers!.get('special'), + contextCreationDidFailMockCalls: + contextCreationDidFail.mock.calls, + }; + } - expect(contextCreationDidFail.mock.calls).toMatchInlineSnapshot(` - Array [ - Array [ - Object { - "error": [GraphQLError: Context creation failed: valid result], + it('GraphQLErrors are formatted, defaulting to status 500', async () => { + expect( + await run( + new GraphQLError('valid result', { + extensions: { code: 'SOME_CODE' }, + }), + ), + ).toMatchInlineSnapshot(` + Object { + "contextCreationDidFailMockCalls": Array [ + Array [ + Object { + "error": [GraphQLError: valid result], + }, + ], + ], + "result": Object { + "errors": Array [ + Object { + "extensions": Object { + "code": "SOME_CODE", + "formatErrorCalled": true, + }, + "message": "valid result", + }, + ], }, - ], - ] - `); + "specialHeader": null, + "status": 500, + } + `); + }); + + it('GraphQLErrors are formatted, defaulting to INTERNAL_SERVER_ERROR', async () => { + expect(await run(new GraphQLError('some error'))) + .toMatchInlineSnapshot(` + Object { + "contextCreationDidFailMockCalls": Array [ + Array [ + Object { + "error": [GraphQLError: some error], + }, + ], + ], + "result": Object { + "errors": Array [ + Object { + "extensions": Object { + "code": "INTERNAL_SERVER_ERROR", + "formatErrorCalled": true, + }, + "message": "some error", + }, + ], + }, + "specialHeader": null, + "status": 500, + } + `); + }); + + it('GraphQLErrors are formatted, obeying http extension', async () => { + expect( + await run( + new GraphQLError('some error', { + extensions: { + http: { + status: 404, + headers: new Map([['special', 'hello']]), + }, + }, + }), + ), + ).toMatchInlineSnapshot(` + Object { + "contextCreationDidFailMockCalls": Array [ + Array [ + Object { + "error": [GraphQLError: some error], + }, + ], + ], + "result": Object { + "errors": Array [ + Object { + "extensions": Object { + "code": "INTERNAL_SERVER_ERROR", + "formatErrorCalled": true, + }, + "message": "some error", + }, + ], + }, + "specialHeader": "hello", + "status": 404, + } + `); + }); + + it('non-GraphQLErrors are formatted', async () => { + expect(await run(new Error('random error'))) + .toMatchInlineSnapshot(` + Object { + "contextCreationDidFailMockCalls": Array [ + Array [ + Object { + "error": [Error: random error], + }, + ], + ], + "result": Object { + "errors": Array [ + Object { + "extensions": Object { + "code": "INTERNAL_SERVER_ERROR", + "formatErrorCalled": true, + }, + "message": "Context creation failed: random error", + }, + ], + }, + "specialHeader": null, + "status": 500, + } + `); + }); }); }); diff --git a/packages/integration-testsuite/src/httpServerTests.ts b/packages/integration-testsuite/src/httpServerTests.ts index eaa6561c3b0..deb65324cce 100644 --- a/packages/integration-testsuite/src/httpServerTests.ts +++ b/packages/integration-testsuite/src/httpServerTests.ts @@ -187,6 +187,36 @@ const queryType = new GraphQLObjectType({ throw new MyGraphQLError('Secret error message'); }, }, + testGraphQLErrorWithHTTP1: { + type: GraphQLString, + resolve() { + throw new GraphQLError('error 1', { + extensions: { + http: { status: 402 }, + }, + }); + }, + }, + testGraphQLErrorWithHTTP2: { + type: GraphQLString, + resolve() { + throw new GraphQLError('error 2', { + extensions: { + http: { headers: new Map([['erroneous', 'indeed']]) }, + }, + }); + }, + }, + testGraphQLErrorWithHTTP3: { + type: GraphQLString, + resolve() { + throw new GraphQLError('error 3', { + extensions: { + http: { headers: new Map([['felonious', 'nah']]) }, + }, + }); + }, + }, }, }); @@ -1478,6 +1508,75 @@ export function defineIntegrationTestSuiteHttpServerTests( expect(unwrapResolverError(error)).toBeInstanceOf(MyGraphQLError); }); + it('GraphQLError HTTP extensions are respected and stripped', async () => { + const app = await createApp({ + schema, + }); + const res = await request(app).post('/').send({ + query: + 'query test{ testGraphQLErrorWithHTTP1 testGraphQLErrorWithHTTP2 testGraphQLErrorWithHTTP3 }', + }); + expect(res.status).toEqual(402); + expect(res.headers.erroneous).toBe('indeed'); + expect(res.headers.felonious).toBe('nah'); + expect(res.body).toMatchInlineSnapshot(` + Object { + "data": Object { + "testGraphQLErrorWithHTTP1": null, + "testGraphQLErrorWithHTTP2": null, + "testGraphQLErrorWithHTTP3": null, + }, + "errors": Array [ + Object { + "extensions": Object { + "code": "INTERNAL_SERVER_ERROR", + }, + "locations": Array [ + Object { + "column": 13, + "line": 1, + }, + ], + "message": "error 1", + "path": Array [ + "testGraphQLErrorWithHTTP1", + ], + }, + Object { + "extensions": Object { + "code": "INTERNAL_SERVER_ERROR", + }, + "locations": Array [ + Object { + "column": 39, + "line": 1, + }, + ], + "message": "error 2", + "path": Array [ + "testGraphQLErrorWithHTTP2", + ], + }, + Object { + "extensions": Object { + "code": "INTERNAL_SERVER_ERROR", + }, + "locations": Array [ + Object { + "column": 65, + "line": 1, + }, + ], + "message": "error 3", + "path": Array [ + "testGraphQLErrorWithHTTP3", + ], + }, + ], + } + `); + }); + it('formatError receives correct error for parse failure', async () => { const expected = '--blank--'; let gotCorrectCode = false; diff --git a/packages/server/src/ApolloServer.ts b/packages/server/src/ApolloServer.ts index 920f2410719..a959a242850 100644 --- a/packages/server/src/ApolloServer.ts +++ b/packages/server/src/ApolloServer.ts @@ -24,7 +24,11 @@ import Negotiator from 'negotiator'; import * as uuid from 'uuid'; import { newCachePolicy } from './cachePolicy.js'; import { determineApolloConfig } from './determineApolloConfig.js'; -import { ensureError, normalizeAndFormatErrors } from './errorNormalize.js'; +import { + ensureError, + ensureGraphQLError, + normalizeAndFormatErrors, +} from './errorNormalize.js'; import { ApolloServerErrorCode } from './errors/index.js'; import type { ApolloServerPlugin, @@ -40,7 +44,6 @@ import type { ApolloServerOptions, DocumentStore, PersistedQueryOptions, - HTTPGraphQLHead, ContextThunk, GraphQLRequestContext, } from './externalTypes/index.js'; @@ -52,7 +55,6 @@ import { } from './preventCsrf.js'; import { APQ_CACHE_PREFIX, processGraphQLRequest } from './requestPipeline.js'; import { - badMethodErrorMessage, cloneObject, HeaderMap, newHTTPGraphQLHead, @@ -976,16 +978,12 @@ export class ApolloServer { ); } - error.message = `Context creation failed: ${error.message}`; - // If we explicitly provide an error code that isn't - // INTERNAL_SERVER_ERROR, we'll treat it as a client error. - const status = - error instanceof GraphQLError && - error.extensions.code && - error.extensions.code !== ApolloServerErrorCode.INTERNAL_SERVER_ERROR - ? 400 - : 500; - return this.errorResponse(error, newHTTPGraphQLHead(status)); + // If some random function threw, add a helpful prefix when converting + // to GraphQLError. If it was already a GraphQLError, trust that the + // message was chosen thoughtfully and leave off the prefix. + return this.errorResponse( + ensureGraphQLError(error, 'Context creation failed: '), + ); } return await runPotentiallyBatchedHttpQuery( @@ -1012,38 +1010,29 @@ export class ApolloServer { `invalidRequestWasReceived hook threw: ${pluginError}`, ); } - return this.errorResponse( - maybeError, - // Quite hacky, but beats putting more stuff on GraphQLError - // subclasses, maybe? - maybeError.message === badMethodErrorMessage - ? { - status: 405, - headers: new HeaderMap([['allow', 'GET, POST']]), - } - : newHTTPGraphQLHead(400), - ); } return this.errorResponse(maybeError); } } - private errorResponse( - error: unknown, - httpGraphQLHead: HTTPGraphQLHead = newHTTPGraphQLHead(), - ): HTTPGraphQLResponse { + private errorResponse(error: unknown): HTTPGraphQLResponse { + const { formattedErrors, httpFromErrors } = normalizeAndFormatErrors( + [error], + { + includeStacktraceInErrorResponses: + this.internals.includeStacktraceInErrorResponses, + formatError: this.internals.formatError, + }, + ); + return { - status: httpGraphQLHead.status ?? 500, + status: httpFromErrors.status ?? 500, headers: new HeaderMap([ - ...httpGraphQLHead.headers, + ...httpFromErrors.headers, ['content-type', 'application/json'], ]), completeBody: prettyJSONStringify({ - errors: normalizeAndFormatErrors([error], { - includeStacktraceInErrorResponses: - this.internals.includeStacktraceInErrorResponses, - formatError: this.internals.formatError, - }), + errors: formattedErrors, }), bodyChunks: null, }; diff --git a/packages/server/src/__tests__/errors.test.ts b/packages/server/src/__tests__/errors.test.ts index 127368c4c71..3e5a3496bf1 100644 --- a/packages/server/src/__tests__/errors.test.ts +++ b/packages/server/src/__tests__/errors.test.ts @@ -20,7 +20,7 @@ describe('Errors', () => { }), ], { includeStacktraceInErrorResponses: true }, - ); + ).formattedErrors; expect(error.message).toEqual(message); expect(error.extensions?.key).toEqual(key); expect(error.extensions).not.toHaveProperty('exception'); // Removed in AS4 @@ -33,7 +33,7 @@ describe('Errors', () => { (thrown as any).key = key; const error = normalizeAndFormatErrors([ new GraphQLError(thrown.message, { originalError: thrown }), - ])[0]; + ]).formattedErrors[0]; expect(error.message).toEqual(message); expect(error.extensions?.code).toEqual('INTERNAL_SERVER_ERROR'); expect(error.extensions).not.toHaveProperty('exception'); // Removed in AS4 @@ -45,7 +45,7 @@ describe('Errors', () => { new GraphQLError(message, { extensions: { code, key }, }), - ])[0]; + ]).formattedErrors[0]; expect(error.message).toEqual(message); expect(error.extensions?.key).toEqual(key); expect(error.extensions).not.toHaveProperty('exception'); // Removed in AS4 @@ -71,7 +71,9 @@ describe('Errors', () => { }); it('Formats native Errors in a JSON-compatible way', () => { const error = new Error('Hello'); - const [formattedError] = normalizeAndFormatErrors([error]); + const [formattedError] = normalizeAndFormatErrors([ + error, + ]).formattedErrors; expect(JSON.parse(JSON.stringify(formattedError)).message).toBe('Hello'); }); @@ -106,7 +108,7 @@ describe('Errors', () => { const errors = normalizeAndFormatErrors([thrown], { formatError, includeStacktraceInErrorResponses: true, - }); + }).formattedErrors; expect(errors).toHaveLength(1); const [error] = errors; expect(error.extensions?.exception).toHaveProperty('stacktrace'); @@ -130,7 +132,7 @@ describe('Errors', () => { const errors = normalizeAndFormatErrors([thrown], { formatError, includeStacktraceInErrorResponses: false, - }); + }).formattedErrors; expect(errors).toHaveLength(1); const [error] = errors; expect(error).toMatchInlineSnapshot(` diff --git a/packages/server/src/__tests__/runQuery.test.ts b/packages/server/src/__tests__/runQuery.test.ts index d280d249f9f..df6b98dce64 100644 --- a/packages/server/src/__tests__/runQuery.test.ts +++ b/packages/server/src/__tests__/runQuery.test.ts @@ -830,7 +830,10 @@ describe('request pipeline life-cycle hooks', () => { errors: expect.arrayContaining([ expect.objectContaining({ message: 'Syntax Error: Expected Name, found "}".', - extensions: { code: 'GRAPHQL_PARSE_FAILED' }, + extensions: { + code: 'GRAPHQL_PARSE_FAILED', + http: { status: 400, headers: new Map() }, + }, }), ]), }), @@ -852,7 +855,10 @@ describe('request pipeline life-cycle hooks', () => { expect.objectContaining({ message: 'Cannot query field "testStringWithParseError" on type "QueryType".', - extensions: { code: 'GRAPHQL_VALIDATION_FAILED' }, + extensions: { + code: 'GRAPHQL_VALIDATION_FAILED', + http: { status: 400, headers: new Map() }, + }, }), ]), }), diff --git a/packages/server/src/errorNormalize.ts b/packages/server/src/errorNormalize.ts index a2dd25063e4..390afac3393 100644 --- a/packages/server/src/errorNormalize.ts +++ b/packages/server/src/errorNormalize.ts @@ -6,9 +6,20 @@ import { GraphQLFormattedError, } from 'graphql'; import { ApolloServerErrorCode } from './errors/index.js'; +import type { HTTPGraphQLHead } from './externalTypes/http.js'; +import { + HeaderMap, + mergeHTTPGraphQLHead, + newHTTPGraphQLHead, +} from './runHttpQuery.js'; // This function accepts any value that were thrown and convert it to GraphQLFormattedError. // It also add default extensions.code and copy stack trace onto an extension if requested. +// Additionally, it returns an `HTTPGraphQLHead` created from combining the values of any +// `HTTPGraphqlHead` objects found on `extensions.http` (the behavior when multiple errors +// set a status code or set the same header should be treated as undefined); these extensions +// are removed from the formatted error. +// // This function should not throw. export function normalizeAndFormatErrors( errors: ReadonlyArray, @@ -19,25 +30,33 @@ export function normalizeAndFormatErrors( ) => GraphQLFormattedError; includeStacktraceInErrorResponses?: boolean; } = {}, -): Array { +): { + formattedErrors: Array; + httpFromErrors: HTTPGraphQLHead; +} { const formatError = options.formatError ?? ((error) => error); - return errors.map((error) => { - try { - return formatError(enrichError(error), error); - } catch (formattingError) { - if (options.includeStacktraceInErrorResponses) { - // includeStacktraceInErrorResponses is used in development - // so it will be helpful to show errors thrown by formatError hooks in that mode - return enrichError(formattingError); - } else { - // obscure error - return { - message: 'Internal server error', - extensions: { code: ApolloServerErrorCode.INTERNAL_SERVER_ERROR }, - }; + const httpFromErrors = newHTTPGraphQLHead(); + + return { + httpFromErrors, + formattedErrors: errors.map((error) => { + try { + return formatError(enrichError(error), error); + } catch (formattingError) { + if (options.includeStacktraceInErrorResponses) { + // includeStacktraceInErrorResponses is used in development + // so it will be helpful to show errors thrown by formatError hooks in that mode + return enrichError(formattingError); + } else { + // obscure error + return { + message: 'Internal server error', + extensions: { code: ApolloServerErrorCode.INTERNAL_SERVER_ERROR }, + }; + } } - } - }); + }), + }; function enrichError(maybeError: unknown): GraphQLFormattedError { const graphqlError = ensureGraphQLError(maybeError); @@ -49,6 +68,14 @@ export function normalizeAndFormatErrors( ApolloServerErrorCode.INTERNAL_SERVER_ERROR, }; + if (isPartialHTTPGraphQLHead(extensions.http)) { + mergeHTTPGraphQLHead(httpFromErrors, { + headers: new HeaderMap(), + ...extensions.http, + }); + delete extensions.http; + } + if (options.includeStacktraceInErrorResponses) { // Note that if ensureGraphQLError created graphqlError from an // originalError, graphqlError.stack will be the same as @@ -67,10 +94,24 @@ export function ensureError(maybeError: unknown): Error { : new GraphQLError('Unexpected error value: ' + String(maybeError)); } -export function ensureGraphQLError(maybeError: unknown): GraphQLError { +export function ensureGraphQLError( + maybeError: unknown, + messagePrefixIfNotGraphQLError: string = '', +): GraphQLError { const error: Error = ensureError(maybeError); return error instanceof GraphQLError ? error - : new GraphQLError(error.message, { originalError: error }); + : new GraphQLError(messagePrefixIfNotGraphQLError + error.message, { + originalError: error, + }); +} + +function isPartialHTTPGraphQLHead(x: unknown): x is Partial { + return ( + !!x && + typeof x === 'object' && + (!('status' in x) || typeof (x as any).status === 'number') && + (!('headers' in x) || (x as any).headers instanceof Map) + ); } diff --git a/packages/server/src/internalErrorClasses.ts b/packages/server/src/internalErrorClasses.ts index 0562114ab5a..5f958fe50b0 100644 --- a/packages/server/src/internalErrorClasses.ts +++ b/packages/server/src/internalErrorClasses.ts @@ -1,5 +1,6 @@ import { GraphQLError, GraphQLErrorOptions } from 'graphql'; import { ApolloServerErrorCode } from './errors/index.js'; +import { HeaderMap, newHTTPGraphQLHead } from './runHttpQuery.js'; // These error classes are not part of Apollo Server's external API; the // ApolloServerErrorCode enum is (exported from `@apollo/server/errors`). @@ -23,7 +24,7 @@ export class SyntaxError extends GraphQLErrorWithCode { super(graphqlError.message, ApolloServerErrorCode.GRAPHQL_PARSE_FAILED, { source: graphqlError.source, positions: graphqlError.positions, - extensions: graphqlError.extensions, + extensions: { http: newHTTPGraphQLHead(400), ...graphqlError.extensions }, originalError: graphqlError, }); } @@ -36,18 +37,34 @@ export class ValidationError extends GraphQLErrorWithCode { ApolloServerErrorCode.GRAPHQL_VALIDATION_FAILED, { nodes: graphqlError.nodes, - extensions: graphqlError.extensions, + extensions: { + http: newHTTPGraphQLHead(400), + ...graphqlError.extensions, + }, originalError: graphqlError.originalError ?? graphqlError, }, ); } } +// Persisted query errors (especially "not found") need to be uncached, because +// hopefully we're about to fill in the APQ cache and the same request will +// succeed next time. We also want a 200 response to avoid any error handling +// that may mask the contents of an error response. (Otherwise, the default +// status code for a response with `errors` but no `data` (even null) is 400.) +const getPersistedQueryErrorHttp = () => ({ + status: 200, + headers: new HeaderMap([ + ['cache-control', 'private, no-cache, must-revalidate'], + ]), +}); + export class PersistedQueryNotFoundError extends GraphQLErrorWithCode { constructor() { super( 'PersistedQueryNotFound', ApolloServerErrorCode.PERSISTED_QUERY_NOT_FOUND, + { extensions: { http: getPersistedQueryErrorHttp() } }, ); } } @@ -57,6 +74,11 @@ export class PersistedQueryNotSupportedError extends GraphQLErrorWithCode { super( 'PersistedQueryNotSupported', ApolloServerErrorCode.PERSISTED_QUERY_NOT_SUPPORTED, + // 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 + // been doing it for years so :shrug: + { extensions: { http: getPersistedQueryErrorHttp() } }, ); } } @@ -79,14 +101,22 @@ export class OperationResolutionError extends GraphQLErrorWithCode { { nodes: graphqlError.nodes, originalError: graphqlError.originalError ?? graphqlError, - extensions: graphqlError.extensions, + extensions: { + http: newHTTPGraphQLHead(400), + ...graphqlError.extensions, + }, }, ); } } export class BadRequestError extends GraphQLErrorWithCode { - constructor(message: string) { - super(message, ApolloServerErrorCode.BAD_REQUEST); + constructor(message: string, options?: GraphQLErrorOptions) { + super(message, ApolloServerErrorCode.BAD_REQUEST, { + ...options, + // Default to 400 status code, but caller can override. (If caller just + // wants to override headers... well, they can't, sorry.) + extensions: { http: newHTTPGraphQLHead(400), ...options?.extensions }, + }); } } diff --git a/packages/server/src/requestPipeline.ts b/packages/server/src/requestPipeline.ts index 47c121217a2..4830de08471 100644 --- a/packages/server/src/requestPipeline.ts +++ b/packages/server/src/requestPipeline.ts @@ -3,7 +3,6 @@ import { specifiedRules, getOperationAST, GraphQLError, - GraphQLFormattedError, validate, parse, execute as graphqlExecute, @@ -41,7 +40,6 @@ import type { GraphQLRequestContextDidEncounterErrors, GraphQLRequestExecutionListener, BaseContext, - HTTPGraphQLHead, } from './externalTypes/index.js'; import { @@ -52,7 +50,11 @@ import { import { makeGatewayGraphQLRequestContext } from './utils/makeGatewayGraphQLRequestContext.js'; -import { HeaderMap, newHTTPGraphQLHead } from './runHttpQuery.js'; +import { + HeaderMap, + mergeHTTPGraphQLHead, + newHTTPGraphQLHead, +} from './runHttpQuery.js'; import type { ApolloServer, ApolloServerInternals, @@ -84,18 +86,6 @@ function isBadUserInputGraphQLError(error: GraphQLError): boolean { ); } -// Persisted query errors (especially "not found") need to be uncached, because -// hopefully we're about to fill in the APQ cache and the same request will -// succeed next time. We also want a 200 response to avoid any error handling -// that may mask the contents of an error response. (Otherwise, the default -// status code for a response with `errors` but no `data` (even null) is 400.) -const getPersistedQueryErrorHttp = () => ({ - status: 200, - headers: new HeaderMap([ - ['cache-control', 'private, no-cache, must-revalidate'], - ]), -}); - export async function processGraphQLRequest( schemaDerivedData: SchemaDerivedData, server: ApolloServer, @@ -121,17 +111,12 @@ export async function processGraphQLRequest( // It looks like we've received a persisted query. Check if we // support them. if (!internals.persistedQueries) { - return await sendErrorResponse( - [new PersistedQueryNotSupportedError()], - // 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 - // been doing it for years so :shrug: - getPersistedQueryErrorHttp(), - ); + return await sendErrorResponse([new PersistedQueryNotSupportedError()]); } else if (extensions.persistedQuery.version !== 1) { return await sendErrorResponse([ - new GraphQLError('Unsupported persisted query version'), + new GraphQLError('Unsupported persisted query version', { + extensions: { http: newHTTPGraphQLHead(400) }, + }), ]); } @@ -142,10 +127,7 @@ export async function processGraphQLRequest( if (query) { requestContext.metrics.persistedQueryHit = true; } else { - return await sendErrorResponse( - [new PersistedQueryNotFoundError()], - getPersistedQueryErrorHttp(), - ); + return await sendErrorResponse([new PersistedQueryNotFoundError()]); } } else { const computedQueryHash = computeQueryHash(query); @@ -156,7 +138,9 @@ export async function processGraphQLRequest( // an existing hash. if (queryHash !== computedQueryHash) { return await sendErrorResponse([ - new GraphQLError('provided sha does not match query'), + new GraphQLError('provided sha does not match query', { + extensions: { http: newHTTPGraphQLHead(400) }, + }), ]); } @@ -300,14 +284,16 @@ export async function processGraphQLRequest( operation?.operation && operation.operation !== 'query' ) { - return await sendErrorResponse( - [ - new BadRequestError( - `GET requests only support query operations, not ${operation.operation} operations`, - ), - ], - { status: 405, headers: new HeaderMap([['allow', 'POST']]) }, - ); + return await sendErrorResponse([ + new BadRequestError( + `GET requests only support query operations, not ${operation.operation} operations`, + { + extensions: { + http: { status: 405, headers: new HeaderMap([['allow', 'POST']]) }, + }, + }, + ), + ]); } try { @@ -319,10 +305,7 @@ export async function processGraphQLRequest( ), ); } catch (err: unknown) { - return await sendErrorResponse( - [ensureGraphQLError(err)], - newHTTPGraphQLHead(500), - ); + return await sendErrorResponse([ensureGraphQLError(err)]); } // Now that we've gone through the pre-execution phases of the request @@ -359,7 +342,7 @@ export async function processGraphQLRequest( ); if (responseFromPlugin !== null) { requestContext.response.result = responseFromPlugin.result; - updateResponseHTTP(responseFromPlugin.http); + mergeHTTPGraphQLHead(requestContext.response.http, responseFromPlugin.http); } else { const executionListeners = ( await Promise.all( @@ -414,7 +397,6 @@ export async function processGraphQLRequest( enablePluginsForSchemaResolvers(schemaDerivedData.schema); } - let statusIfExecuteThrows = 500; try { const result = await execute( requestContext as GraphQLRequestContextExecutionDidStart, @@ -429,7 +411,6 @@ export async function processGraphQLRequest( 'Unexpected error: Apollo Server did not resolve an operation but execute did not return errors', ); } - statusIfExecuteThrows = 400; throw new OperationResolutionError(result.errors[0]); } @@ -455,20 +436,22 @@ export async function processGraphQLRequest( await didEncounterErrors(resultErrors); } + const { formattedErrors, httpFromErrors } = resultErrors + ? formatErrors(resultErrors) + : { formattedErrors: undefined, httpFromErrors: newHTTPGraphQLHead() }; + requestContext.response.result = { ...result, - errors: resultErrors ? formatErrors(resultErrors) : undefined, + errors: formattedErrors, }; + mergeHTTPGraphQLHead(requestContext.response.http, httpFromErrors); } catch (executionMaybeError: unknown) { const executionError = ensureError(executionMaybeError); await Promise.all( executionListeners.map((l) => l.executionDidEnd?.(executionError)), ); - return await sendErrorResponse( - [ensureGraphQLError(executionError)], - newHTTPGraphQLHead(statusIfExecuteThrows), - ); + return await sendErrorResponse([ensureGraphQLError(executionError)]); } await Promise.all(executionListeners.map((l) => l.executionDidEnd?.())); @@ -503,17 +486,6 @@ export async function processGraphQLRequest( } } - function updateResponseHTTP(http: HTTPGraphQLHead) { - if (http.status) { - requestContext.response.http.status = http.status; - } - for (const [name, value] of http.headers) { - // TODO(AS4): this is overwriting rather than appending. However we should - // probably be able to eliminate this whole block if we refactor GraphQLResponse. - requestContext.response.http.headers.set(name, value); - } - } - async function invokeWillSendResponse() { await Promise.all( requestListeners.map((l) => @@ -544,23 +516,23 @@ export async function processGraphQLRequest( // some errors.) In this case "send" means "update requestContext.response and // invoke willSendResponse hooks". // - // If `http` is provided, it sets its status code and headers if given. + // If any errors have `extensions.http` set, it sets the response's status code + // and errors from them. // - // Then, if the HTTP status code is not yet set, it sets it to 400. - async function sendErrorResponse( - errors: ReadonlyArray, - http: HTTPGraphQLHead = newHTTPGraphQLHead(), - ) { + // Then, if the HTTP status code is not yet set, it sets it to 500. + async function sendErrorResponse(errors: ReadonlyArray) { await didEncounterErrors(errors); + const { formattedErrors, httpFromErrors } = formatErrors(errors); + requestContext.response.result = { - errors: formatErrors(errors), + errors: formattedErrors, }; - updateResponseHTTP(http); + mergeHTTPGraphQLHead(requestContext.response.http, httpFromErrors); if (!requestContext.response.http.status) { - requestContext.response.http.status = 400; + requestContext.response.http.status = 500; } await invokeWillSendResponse(); @@ -568,7 +540,7 @@ export async function processGraphQLRequest( function formatErrors( errors: ReadonlyArray, - ): ReadonlyArray { + ): ReturnType { return normalizeAndFormatErrors(errors, { formatError: internals.formatError, includeStacktraceInErrorResponses: diff --git a/packages/server/src/runHttpQuery.ts b/packages/server/src/runHttpQuery.ts index 2fe3afe19b1..8b826c157ae 100644 --- a/packages/server/src/runHttpQuery.ts +++ b/packages/server/src/runHttpQuery.ts @@ -120,9 +120,6 @@ function ensureQueryIsStringOrMissing(query: unknown) { } } -export const badMethodErrorMessage = - 'Apollo Server supports only GET/POST requests.'; - export async function runHttpQuery( server: ApolloServer, httpRequest: HTTPGraphQLRequest, @@ -191,7 +188,17 @@ export async function runHttpQuery( break; } default: - throw new BadRequestError(badMethodErrorMessage); + throw new BadRequestError( + 'Apollo Server supports only GET/POST requests.', + { + extensions: { + http: { + status: 405, + headers: new HeaderMap([['allow', 'GET, POST']]), + }, + }, + }, + ); } const graphQLResponse = await internalExecuteOperation({ @@ -240,3 +247,22 @@ export function newHTTPGraphQLHead(status?: number): HTTPGraphQLHead { headers: new HeaderMap(), }; } + +// Updates `target` with status code and headers from `source`. For now let's +// consider it undefined what happens if both have a status code set or both set +// the same header. +export function mergeHTTPGraphQLHead( + target: HTTPGraphQLHead, + source: HTTPGraphQLHead, +) { + if (source.status) { + target.status = source.status; + } + if (source.headers) { + for (const [name, value] of source.headers) { + // If source.headers contains non-lowercase header names, this will + // catch that case as long as target.headers is a HeaderMap. + target.headers.set(name, value); + } + } +}