Skip to content

Commit

Permalink
Ensure startup errors are redacted even the first time
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
glasser committed Mar 26, 2021
1 parent 318483b commit cde6656
Show file tree
Hide file tree
Showing 2 changed files with 67 additions and 7 deletions.
17 changes: 12 additions & 5 deletions packages/apollo-server-core/src/ApolloServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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':
Expand All @@ -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
Expand Down Expand Up @@ -1270,7 +1274,10 @@ export class ApolloServerBase {
* `{req: express.Request, res: express.Response }` object) and to keep it
* updated as you upgrade Apollo Server.
*/
public async executeOperation(request: GraphQLRequest, integrationContextArgument?: Record<string, any>) {
public async executeOperation(
request: GraphQLRequest,
integrationContextArgument?: Record<string, any>,
) {
const options = await this.graphQLServerOptions(integrationContextArgument);

if (typeof options.context === 'function') {
Expand Down
57 changes: 55 additions & 2 deletions packages/apollo-server-core/src/__tests__/ApolloServerBase.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -36,7 +38,6 @@ describe('ApolloServerBase construction', () => {
});

it('succeeds when passed a graphVariant in construction', () => {
let serverBase;
expect(() =>
new ApolloServerBase({
typeDefs,
Expand Down Expand Up @@ -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."`,
Expand All @@ -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({
Expand Down

0 comments on commit cde6656

Please sign in to comment.