Ensure startup errors are redacted even the first time (#5064)

This is a regression in #4981. If the server start process is begun implicitly
by the execution of an operation (ensureStarted inside graphQLServerOptions) and
startup throws, the log-and-redact logic wasn't being invoked.

Note that this case doesn't usually happen in practice, because:
- If you're using `apollo-server`, startup is begun in `listen()` before you can
  serve requests
- If you're using a serverless framework integration, startup is begun in the
  constructor
- If you're using a non-serverless framework integration, the function you call
  to connect it to your framework begins startup with `ensureStarting()`

So mostly this just affects the case that you're running `executeOperation`
without calling `start()` or `listen()`, or maybe you have your own custom
framework integration that doesn't call `ensureStarting()`. But it's still worth
missing.

Add some tests of this behavior and fix some TypeScript issues in the test file.

CHANGELOG.md

@@ -13,6 +13,8 @@ The version headers in this history reflect the versions of Apollo Server itself
 
 - `apollo-server-core`: Add optional argument to `ApolloServer.executeOperation` allowing the caller to manually specify an argument to the `config` function analogous to that provided by integration packages. [PR #4166](https://github.com/apollographql/apollo-server/pull/4166) [Issue #2886](https://github.com/apollographql/apollo-server/issues/2886)
 
+- `apollo-server-core`: Fix a regression in v2.22.0 where startup errors could be thrown as part of the GraphQL response instead of redacted in one edge case. [PR #5064](https://github.com/apollographql/apollo-server/pull/5064)
+
 ## v2.22.0
 
 - Improve startup error handling by ensuring that your server has loaded its schema and executed its `serverWillStart` handlers successfully before starting an HTTP server. If you're using the `apollo-server` package, no code changes are necessary. If you're using an integration such as `apollo-server-express` that is not a "serverless framework", you can insert [`await server.start()`](https://www.apollographql.com/docs/apollo-server/api/apollo-server/#start) between `server = new ApolloServer()` and `server.applyMiddleware`. (If you don't call `server.start()` yourself, your server will still work, but the previous behavior of starting a web server that may fail to load its schema still applies.) The serverless framework integrations (Lambda, Azure Functions, and Cloud Functions) do not support this functionality. While the protected method `willStart` still exists for backwards compatibility, you should replace calls to it with `start` or the new protected method `ensureStarting`. [PR #4981](https://github.com/apollographql/apollo-server/pull/4981)

packages/apollo-server-core/src/ApolloServer.ts

@@ -623,7 +623,12 @@ export class ApolloServerBase {
       switch (this.state.phase) {
         case 'initialized with gateway':
         case 'initialized with schema':
-          await this._start();
+          try {
+            await this._start();
+          } catch {
+            // Any thrown error should transition us to 'failed to start', and
+            // we'll handle that on the next iteration of the while loop.
+          }
           // continue the while loop
           break;
         case 'starting':
@@ -632,9 +637,8 @@ export class ApolloServerBase {
           // continue the while loop
           break;
         case 'failed to start':
-          //  First
-          // we log the error that prevented startup (which means it will get logged
-          // once for every GraphQL operation).
+          // First we log the error that prevented startup (which means it will
+          // get logged once for every GraphQL operation).
           this.logStartupError(this.state.error);
           // Now make the operation itself fail.
           // We intentionally do not re-throw actual startup error as it may contain

packages/apollo-server-core/src/__tests__/ApolloServerBase.test.ts

@@ -2,6 +2,8 @@ import { ApolloServerBase } from '../ApolloServer';
 import { buildServiceDefinition } from '@apollographql/apollo-tools';
 import gql from 'graphql-tag';
 import { Logger } from 'apollo-server-types';
+import { ApolloServerPlugin } from 'apollo-server-plugin-base';
+import type { GraphQLSchema } from 'graphql';
 
 const typeDefs = gql`
   type Query {
@@ -36,7 +38,6 @@ describe('ApolloServerBase construction', () => {
   });
 
   it('succeeds when passed a graphVariant in construction', () => {
-    let serverBase;
     expect(() =>
       new ApolloServerBase({
         typeDefs,
@@ -84,7 +85,7 @@ describe('ApolloServerBase construction', () => {
   it('throws when a GraphQLSchema is not provided to the schema configuration option', () => {
     expect(() => {
       new ApolloServerBase({
-        schema: {},
+        schema: {} as GraphQLSchema,
       });
     }).toThrowErrorMatchingInlineSnapshot(
       `"Expected {} to be a GraphQL schema."`,
@@ -100,6 +101,58 @@ describe('ApolloServerBase construction', () => {
   });
 });
 
+describe('ApolloServerBase start', () => {
+  const failToStartPlugin: ApolloServerPlugin = {
+    async serverWillStart() {
+      throw Error('nope');
+    },
+  };
+  const redactedMessage =
+    'This data graph is missing a valid configuration. More details may be available in the server logs.';
+
+  it('start throws on startup error', async () => {
+    const server = new ApolloServerBase({
+      typeDefs,
+      resolvers,
+      plugins: [failToStartPlugin],
+    });
+    await expect(server.start()).rejects.toThrow('nope');
+  });
+
+  it('execute throws redacted message on implicit startup error', async () => {
+    const error = jest.fn();
+    const logger: Logger = {
+      debug: jest.fn(),
+      info: jest.fn(),
+      warn: jest.fn(),
+      error,
+    };
+
+    const server = new ApolloServerBase({
+      typeDefs,
+      resolvers,
+      plugins: [failToStartPlugin],
+      logger,
+    });
+    // Run the operation twice (the first will kick off the start process). We
+    // want to see the same error thrown and log message for the "kick it off"
+    // call as the subsequent call.
+    await expect(
+      server.executeOperation({ query: '{__typename}' }),
+    ).rejects.toThrow(redactedMessage);
+    await expect(
+      server.executeOperation({ query: '{__typename}' }),
+    ).rejects.toThrow(redactedMessage);
+    expect(error).toHaveBeenCalledTimes(2);
+    expect(error.mock.calls[0][0]).toMatch(
+      /Apollo Server was started implicitly.*nope/,
+    );
+    expect(error.mock.calls[1][0]).toMatch(
+      /Apollo Server was started implicitly.*nope/,
+    );
+  });
+});
+
 describe('ApolloServerBase executeOperation', () => {
   it('returns error information without details by default', async () => {
     const server = new ApolloServerBase({