checkpoint for error/http/context improvements

glasser · glasser · commit 9d6ae6e4a13e · 2022-08-26T13:08:16.000-07:00
diff --git a/packages/server/src/ApolloServer.ts b/packages/server/src/ApolloServer.ts
@@ -40,7 +40,6 @@ import type {
   ApolloServerOptions,
   DocumentStore,
   PersistedQueryOptions,
-  HTTPGraphQLHead,
   ContextThunk,
   GraphQLRequestContext,
 } from './externalTypes/index.js';
@@ -52,7 +51,6 @@ import {
 } from './preventCsrf.js';
 import { APQ_CACHE_PREFIX, processGraphQLRequest } from './requestPipeline.js';
 import {
-  badMethodErrorMessage,
   cloneObject,
   HeaderMap,
   newHTTPGraphQLHead,
@@ -976,16 +974,8 @@ export class ApolloServer<in out TContext extends BaseContext = BaseContext> {
           );
         }
 
-        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));
+        // FIXME doc that context errors always default to 500
+        return this.errorResponse(error);
       }
 
       return await runPotentiallyBatchedHttpQuery(
@@ -1012,38 +1002,30 @@ export class ApolloServer<in out TContext extends BaseContext = BaseContext> {
             `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);
       }
       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
@@ -3,6 +3,8 @@ import { unwrapResolverError } from '@apollo/server/errors';
 
 import { normalizeAndFormatErrors } from '../errorNormalize.js';
 
+// FIXME write some tests of throwing http errors in fields and context
+
 describe('Errors', () => {
   describe('normalizeAndFormatErrors', () => {
     const message = 'message';
@@ -20,7 +22,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 +35,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 +47,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 +73,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 +110,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 +134,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
@@ -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
@@ -6,9 +6,20 @@ import {
   GraphQLFormattedError,
 } from 'graphql';
 import { ApolloServerErrorCode } from './errors/index.js';
+import type { HTTPGraphQLHead } from './externalTypes/http.js';
+import {
+  isHTTPGraphQLHead,
+  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<unknown>,
@@ -19,25 +30,33 @@ export function normalizeAndFormatErrors(
     ) => GraphQLFormattedError;
     includeStacktraceInErrorResponses?: boolean;
   } = {},
-): Array<GraphQLFormattedError> {
+): {
+  formattedErrors: Array<GraphQLFormattedError>;
+  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,11 @@ export function normalizeAndFormatErrors(
         ApolloServerErrorCode.INTERNAL_SERVER_ERROR,
     };
 
+    if (isHTTPGraphQLHead(extensions.http)) {
+      mergeHTTPGraphQLHead(httpFromErrors, extensions.http);
+      delete extensions.http;
+    }
+
     if (options.includeStacktraceInErrorResponses) {
       // Note that if ensureGraphQLError created graphqlError from an
       // originalError, graphqlError.stack will be the same as
diff --git 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
diff --git a/packages/server/src/runHttpQuery.ts b/packages/server/src/runHttpQuery.ts
