From 97dbe7d15d8e5274321eb9262c22deeaf95bb7fe Mon Sep 17 00:00:00 2001 From: David Glasser Date: Thu, 25 Mar 2021 21:55:32 -0700 Subject: [PATCH] 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 | 5 ++ .../apollo-server-core/src/ApolloServer.ts | 12 ++-- .../src/__tests__/ApolloServerBase.test.ts | 57 ++++++++++++++++++- 3 files changed, 68 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 02920d72024..5eb817d5a02 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,11 @@ The version headers in this history reflect the versions of Apollo Server itself > The changes noted within this `vNEXT` section have not been released yet. New PRs and commits which introduce changes should include an entry in this `vNEXT` section as part of their development. With few exceptions, the format of the entry should follow convention (i.e., prefix with package name, use markdown `backtick formatting` for package names and code, suffix with a link to the change-set à la `[PR #YYY](https://link/pull/YYY)`, etc.). When a release is being prepared, a new header will be (manually) created below and the appropriate changes within that release will be moved into the new section. + +## v2.22.1 + +- `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) diff --git a/packages/apollo-server-core/src/ApolloServer.ts b/packages/apollo-server-core/src/ApolloServer.ts index cca871e342b..e30119d9725 100644 --- a/packages/apollo-server-core/src/ApolloServer.ts +++ b/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 diff --git a/packages/apollo-server-core/src/__tests__/ApolloServerBase.test.ts b/packages/apollo-server-core/src/__tests__/ApolloServerBase.test.ts index 5de0353a247..1ef355f4608 100644 --- a/packages/apollo-server-core/src/__tests__/ApolloServerBase.test.ts +++ b/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({