From f2241d75c800ab22415277d885df438d7656f7f3 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Fri, 26 Feb 2021 15:20:32 -0800 Subject: [PATCH] draft: async server.start() function Full description to come, but tl;dr: Previously server startup worked like this: - AS constructor runs - If no gateway, calculate schema and schema derived data immediately - If gateway, kick off gateway.load from the end of the constructor, and if it async-throws, log an error once and make the server kinda broken forever - At various spots in the framework integration code, call (but not await) protected willStart, which is an async function that first waits for the gateway to load the schema if necessary and then runs serverWillStart plugin functions; save the Promise returned by calling this. - At request time in the framework integration code, await that promise. And also if there's no schema fail with an error. Now server startup works like this: - There's an explicit state machine situation inside AS - AS constructor initializes state with schema directly if no gateway - If there is a gateway the constructor DOES NOT KICK OFF gateway.load - You can now call `await server.start()` yourself, which will first await gateway.load if necessary, and then await all serverWillStart calls - If you're using `apollo-server` rather than an integration, `server.listen()` will just transparently do this for you; explicit `start()` is just for integrations - The integration places that used to call willStart now call `server.ensureStarting()` instead which will kick off server.start in the background if you didn't (and log any errors thrown). - The places that used to await promiseWillStart no longer do so; generally right after that code we end up calling graphqlServerOptions - graphqlServerOptions now awaits `server.ensureStarted` which will start the server if necessary and throw if it threw. Overall changes: - If you're using `apollo-server`, startup errors will cause `listen` to reject, nothing else necessary. - If you're using an integration you are encouraged to call `await server.start()` yourself after the constructor, which will let you detect startup errors. - But if you don't do that, the server will still start up with similar properties. gateway.load won't start until the integration's `ensureStarting` or graphqlServerOptions' `ensuresStarted` though. Errors will be logged. - Also if you don't call `server.start()`, the full stack trace of any startup error will be logged on *every* failed graphql request instead of just a short message suggesting there's more in logs (but it's only at the beginning of your logs) Yes this is the tl;dr version that I jotted off at the end of my work day off the top of my head without going back and skimming through the PR for details :) Fixes #4921. Fixes apollographql/federation#335. --- .../src/ApolloServer.ts | 9 +- .../src/__tests__/Memcached.test.ts | 3 + .../src/__mocks__/ioredis.ts | 71 +- .../src/__tests__/RedisCache.test.ts | 5 +- .../src/__tests__/testsuite.ts | 7 +- .../src/ApolloServer.ts | 16 +- .../src/ApolloServer.ts | 6 +- .../apollo-server-core/src/ApolloServer.ts | 632 ++++++++++-------- .../src/__tests__/ApolloServerBase.test.ts | 21 +- .../src/plugin/inlineTrace/index.ts | 25 + .../src/plugin/schemaIsFederated.ts | 33 + .../src/plugin/schemaReporting/index.ts | 12 + .../apollo-server-express/src/ApolloServer.ts | 23 +- .../src/__tests__/ApolloServer.test.ts | 7 +- .../apollo-server-fastify/src/ApolloServer.ts | 8 +- .../src/__tests__/ApolloServer.test.ts | 5 +- .../src/__tests__/datasource.test.ts | 11 +- .../apollo-server-hapi/src/ApolloServer.ts | 5 +- .../src/__tests__/ApolloServer.test.ts | 5 +- .../src/ApolloServer.ts | 56 +- .../src/index.ts | 69 +- .../apollo-server-koa/src/ApolloServer.ts | 24 +- .../src/__tests__/ApolloServer.test.ts | 5 +- .../apollo-server-lambda/src/ApolloServer.ts | 16 +- .../apollo-server-micro/src/ApolloServer.ts | 9 +- ...polloServerPluginOperationRegistry.test.ts | 28 +- packages/apollo-server/src/index.ts | 3 + 27 files changed, 641 insertions(+), 473 deletions(-) create mode 100644 packages/apollo-server-core/src/plugin/schemaIsFederated.ts diff --git a/packages/apollo-server-azure-functions/src/ApolloServer.ts b/packages/apollo-server-azure-functions/src/ApolloServer.ts index c0ee3930c93..3c4a361ef4f 100644 --- a/packages/apollo-server-azure-functions/src/ApolloServer.ts +++ b/packages/apollo-server-azure-functions/src/ApolloServer.ts @@ -36,10 +36,10 @@ export class ApolloServer extends ApolloServerBase { } public createHandler({ cors }: CreateHandlerOptions = { cors: undefined }) { - // We will kick off the `willStart` event once for the server, and then - // await it before processing any requests by incorporating its `await` into - // the GraphQLServerOptions function which is called before each request. - const promiseWillStart = this.willStart(); + // In case the user didn't bother to call and await the `start` method, we + // kick it off in the background (with any errors getting logged + // and also rethrown from graphQLServerOptions during later requests). + this.ensureStarting(); const corsHeaders: HttpResponse['headers'] = {}; @@ -145,7 +145,6 @@ export class ApolloServer extends ApolloServerBase { ); }; graphqlAzureFunction(async () => { - await promiseWillStart; return this.createGraphQLServerOptions(req, context); })(context, req, callbackFilter); }; diff --git a/packages/apollo-server-cache-memcached/src/__tests__/Memcached.test.ts b/packages/apollo-server-cache-memcached/src/__tests__/Memcached.test.ts index d3d7d20b47e..e706b0207e5 100644 --- a/packages/apollo-server-cache-memcached/src/__tests__/Memcached.test.ts +++ b/packages/apollo-server-cache-memcached/src/__tests__/Memcached.test.ts @@ -9,6 +9,9 @@ import { describe('Memcached', () => { const cache = new MemcachedCache('localhost'); + afterAll(async () => { + await cache.close(); + }) testKeyValueCache_Basics(cache); testKeyValueCache_Expiration(cache); }); diff --git a/packages/apollo-server-cache-redis/src/__mocks__/ioredis.ts b/packages/apollo-server-cache-redis/src/__mocks__/ioredis.ts index 61a38a55f19..dd56966b0de 100644 --- a/packages/apollo-server-cache-redis/src/__mocks__/ioredis.ts +++ b/packages/apollo-server-cache-redis/src/__mocks__/ioredis.ts @@ -1,38 +1,53 @@ -const IORedis = jest.genMockFromModule('ioredis'); +class Redis { + private keyValue = {}; + private timeouts = new Set(); -const keyValue = {}; + async del(key: string) { + delete this.keyValue[key]; + return true; + } -const deleteKey = key => { - delete keyValue[key]; - return Promise.resolve(true); -}; + async get(key: string) { + if (this.keyValue[key]) { + return this.keyValue[key].value; + } + } -const getKey = key => { - if (keyValue[key]) { - return Promise.resolve(keyValue[key].value); + async mget(...keys: string[]) { + return keys.map((key) => { + if (this.keyValue[key]) { + return this.keyValue[key].value; + } + }); } - return Promise.resolve(undefined); -}; + async set(key, value, type, ttl) { + this.keyValue[key] = { + value, + ttl, + }; + if (ttl) { + const timeout = setTimeout(() => { + this.timeouts.delete(timeout); + delete this.keyValue[key]; + }, ttl * 1000); + this.timeouts.add(timeout); + } + return true; + } + + nodes() { + return []; + } -const mGetKey = (key, cb) => getKey(key).then(val => [val]); + async flushdb() {} -const setKey = (key, value, type, ttl) => { - keyValue[key] = { - value, - ttl, - }; - if (ttl) { - setTimeout(() => { - delete keyValue[key]; - }, ttl * 1000); + async quit() { + this.timeouts.forEach((t) => clearTimeout(t)); } - return Promise.resolve(true); -}; +} -IORedis.prototype.del.mockImplementation(deleteKey); -IORedis.prototype.get.mockImplementation(getKey); -IORedis.prototype.mget.mockImplementation(mGetKey); -IORedis.prototype.set.mockImplementation(setKey); +// Use the same mock as Redis.Cluster. +(Redis as any).Cluster = Redis; -export default IORedis; +export default Redis; diff --git a/packages/apollo-server-cache-redis/src/__tests__/RedisCache.test.ts b/packages/apollo-server-cache-redis/src/__tests__/RedisCache.test.ts index 9c0741d2744..f9afb7bac3d 100644 --- a/packages/apollo-server-cache-redis/src/__tests__/RedisCache.test.ts +++ b/packages/apollo-server-cache-redis/src/__tests__/RedisCache.test.ts @@ -7,7 +7,10 @@ import { } from '../../../apollo-server-caching/src/__tests__/testsuite'; describe('Redis', () => { - const cache = new RedisCache({ host: 'localhost' }); + const cache = new RedisCache(); + afterAll(async () => { + await cache.close(); + }) testKeyValueCache_Basics(cache); testKeyValueCache_Expiration(cache); }); diff --git a/packages/apollo-server-caching/src/__tests__/testsuite.ts b/packages/apollo-server-caching/src/__tests__/testsuite.ts index 722b6e36923..f095f3fc22a 100644 --- a/packages/apollo-server-caching/src/__tests__/testsuite.ts +++ b/packages/apollo-server-caching/src/__tests__/testsuite.ts @@ -36,8 +36,8 @@ export function testKeyValueCache_Expiration( }); afterAll(() => { + jest.useRealTimers(); unmockDate(); - keyValueCache.close && keyValueCache.close(); }); it('is able to expire keys based on ttl', async () => { @@ -70,6 +70,11 @@ export function testKeyValueCache_Expiration( export function testKeyValueCache(keyValueCache: TestableKeyValueCache) { describe('KeyValueCache Test Suite', () => { + afterAll(async () => { + if (keyValueCache.close) { + await keyValueCache.close(); + } + }) testKeyValueCache_Basics(keyValueCache); testKeyValueCache_Expiration(keyValueCache); }); diff --git a/packages/apollo-server-cloud-functions/src/ApolloServer.ts b/packages/apollo-server-cloud-functions/src/ApolloServer.ts index b5b665f77de..4ae494e8a78 100644 --- a/packages/apollo-server-cloud-functions/src/ApolloServer.ts +++ b/packages/apollo-server-cloud-functions/src/ApolloServer.ts @@ -34,10 +34,10 @@ export class ApolloServer extends ApolloServerBase { } public createHandler({ cors }: CreateHandlerOptions = { cors: undefined }) { - // We will kick off the `willStart` event once for the server, and then - // await it before processing any requests by incorporating its `await` into - // the GraphQLServerOptions function which is called before each request. - const promiseWillStart = this.willStart(); + // In case the user didn't bother to call and await the `start` method, we + // kick it off in the background (with any errors getting logged + // and also rethrown from graphQLServerOptions during later requests). + this.ensureStarting(); const corsHeaders = {} as Record; @@ -129,14 +129,6 @@ export class ApolloServer extends ApolloServerBase { } graphqlCloudFunction(async () => { - // In a world where this `createHandler` was async, we might avoid this - // but since we don't want to introduce a breaking change to this API - // (by switching it to `async`), we'll leverage the - // `GraphQLServerOptions`, which are dynamically built on each request, - // to `await` the `promiseWillStart` which we kicked off at the top of - // this method to ensure that it runs to completion (which is part of - // its contract) prior to processing the request. - await promiseWillStart; return this.createGraphQLServerOptions(req, res); })(req, res); }; diff --git a/packages/apollo-server-cloudflare/src/ApolloServer.ts b/packages/apollo-server-cloudflare/src/ApolloServer.ts index 052b234268e..bf8b1db36cb 100644 --- a/packages/apollo-server-cloudflare/src/ApolloServer.ts +++ b/packages/apollo-server-cloudflare/src/ApolloServer.ts @@ -14,7 +14,11 @@ export class ApolloServer extends ApolloServerBase { } public async listen() { - await this.willStart(); + // In case the user didn't bother to call and await the `start` method, we + // kick it off in the background (with any errors getting logged + // and also rethrown from graphQLServerOptions during later requests). + this.ensureStarting(); + addEventListener('fetch', (event: FetchEvent) => { event.respondWith( graphqlCloudflare(() => { diff --git a/packages/apollo-server-core/src/ApolloServer.ts b/packages/apollo-server-core/src/ApolloServer.ts index 49ae83deb6f..e4a3297cbea 100644 --- a/packages/apollo-server-core/src/ApolloServer.ts +++ b/packages/apollo-server-core/src/ApolloServer.ts @@ -3,8 +3,8 @@ import { addMockFunctionsToSchema, GraphQLParseOptions, } from 'graphql-tools'; -import { Server as NetServer } from 'net' -import { Server as TlsServer } from 'tls' +import { Server as NetServer } from 'net'; +import { Server as TlsServer } from 'tls'; import { Server as HttpServer } from 'http'; import { Http2Server, Http2SecureServer } from 'http2'; import { Server as HttpsServer } from 'https'; @@ -19,9 +19,6 @@ import { ValidationContext, FieldDefinitionNode, DocumentNode, - isObjectType, - isScalarType, - isSchema, } from 'graphql'; import { GraphQLExtension } from 'graphql-extensions'; import { @@ -43,10 +40,7 @@ import { import WebSocket from 'ws'; import { formatApolloErrors } from 'apollo-server-errors'; -import { - GraphQLServerOptions, - PersistedQueryOptions, -} from './graphqlOptions'; +import { GraphQLServerOptions, PersistedQueryOptions } from './graphqlOptions'; import { Config, @@ -55,6 +49,7 @@ import { SubscriptionServerOptions, FileUploadOptions, PluginDefinition, + GraphQLService, } from './types'; import { gql } from './index'; @@ -75,13 +70,13 @@ import { import { Headers } from 'apollo-server-env'; import { buildServiceDefinition } from '@apollographql/apollo-tools'; -import { plugin as pluginTracing } from "apollo-tracing"; -import { Logger, SchemaHash, ApolloConfig } from "apollo-server-types"; +import { plugin as pluginTracing } from 'apollo-tracing'; +import { Logger, SchemaHash, ApolloConfig } from 'apollo-server-types'; import { plugin as pluginCacheControl, CacheControlExtensionOptions, } from 'apollo-cache-control'; -import { cloneObject } from "./runHttpQuery"; +import { cloneObject } from './runHttpQuery'; import isNodeLike from './utils/isNodeLike'; import { determineApolloConfig } from './determineApolloConfig'; import { @@ -114,21 +109,61 @@ function approximateObjectSize(obj: T): number { return Buffer.byteLength(JSON.stringify(obj), 'utf8'); } +class Barrier { + private resolvePromise!: () => void; + private promise = new Promise((r) => { + this.resolvePromise = r; + }); + async wait() { + await this.promise; + } + unblock() { + this.resolvePromise(); + } +} + type SchemaDerivedData = { + schema: GraphQLSchema; + schemaHash: SchemaHash; + extensions: Array<() => GraphQLExtension>; // A store that, when enabled (default), will store the parsed and validated // versions of operations in-memory, allowing subsequent parses/validates // on the same operation to be executed immediately. documentStore?: InMemoryLRUCache; - schema: GraphQLSchema; - schemaHash: SchemaHash; - extensions: Array<() => GraphQLExtension>; }; +type ServerState = + | { phase: 'initialized with schema'; schemaDerivedData: SchemaDerivedData } + | { phase: 'initialized with gateway'; gateway: GraphQLService } + | { phase: 'starting'; barrier: Barrier } + | { + phase: 'invoking serverWillStart'; + barrier: Barrier; + schemaDerivedData: SchemaDerivedData; + } + | { phase: 'failed to start'; error: Error } + | { + phase: 'started'; + schemaDerivedData: SchemaDerivedData; + } + | { phase: 'stopping' } + | { phase: 'stopped' }; + +// Throw this in places that should be unreachable (because all other cases have +// been handled, reducing the type of the argument to `never`). TypeScript will +// complain if in fact there is a valid type for the argument. +class UnreachableCaseError extends Error { + constructor(val: never) { + super(`Unreachable case: ${val}`); + } +} export class ApolloServerBase { private logger: Logger; public subscriptionsPath?: string; public graphqlPath: string = '/graphql'; - public requestOptions: Partial> = Object.create(null); + public requestOptions: Partial> = Object.create( + null, + ); private context?: Context | ContextFunction; private apolloConfig: ApolloConfig; @@ -144,13 +179,12 @@ export class ApolloServerBase { protected playgroundOptions?: PlaygroundRenderPageOptions; private parseOptions: GraphQLParseOptions; - private schemaDerivedData: Promise; private config: Config; + private state: ServerState; /** @deprecated: This is undefined for servers operating as gateways, and will be removed in a future release **/ protected schema?: GraphQLSchema; private toDispose = new Set<() => Promise>(); - private experimental_approximateDocumentStoreMiB: - Config['experimental_approximateDocumentStoreMiB']; + private experimental_approximateDocumentStoreMiB: Config['experimental_approximateDocumentStoreMiB']; // The constructor should be universal across all environments. All environment specific behavior should be set by adding or overriding methods constructor(config: Config) { @@ -194,7 +228,7 @@ export class ApolloServerBase { this.logger = config.logger; } else { // If the user didn't provide their own logger, we'll initialize one. - const loglevelLogger = loglevel.getLogger("apollo-server"); + const loglevelLogger = loglevel.getLogger('apollo-server'); // We don't do much logging in Apollo Server right now. There's a notion // of a `debug` flag, which changes stack traces in some error messages, @@ -246,10 +280,8 @@ export class ApolloServerBase { } if (requestOptions.persistedQueries !== false) { - const { - cache: apqCache = requestOptions.cache!, - ...apqOtherOptions - } = requestOptions.persistedQueries || Object.create(null); + const { cache: apqCache = requestOptions.cache!, ...apqOtherOptions } = + requestOptions.persistedQueries || Object.create(null); requestOptions.persistedQueries = { cache: new PrefixingKeyValueCache(apqCache, APQ_CACHE_PREFIX), @@ -324,21 +356,6 @@ export class ApolloServerBase { this.playgroundOptions = createPlaygroundOptions(playground); - // TODO: This is a bit nasty because the subscription server needs this.schema synchronously, for reasons of backwards compatibility. - const _schema = this.initSchema(); - - if (isSchema(_schema)) { - const derivedData = this.generateSchemaDerivedData(_schema); - this.schema = derivedData.schema; - this.schemaDerivedData = Promise.resolve(derivedData); - } else if (typeof _schema.then === 'function') { - this.schemaDerivedData = _schema.then(schema => - this.generateSchemaDerivedData(schema), - ); - } else { - throw new Error("Unexpected error: Unable to resolve a valid GraphQLSchema. Please file an issue with a reproduction of this error, if possible."); - } - // Plugins will be instantiated if they aren't already, and this.plugins // is populated accordingly. this.ensurePluginInstantiation(plugins); @@ -369,6 +386,17 @@ export class ApolloServerBase { }); }); } + + if (gateway) { + this.state = { phase: 'initialized with gateway', gateway }; + } else { + this.state = { + phase: 'initialized with schema', + schemaDerivedData: this.generateSchemaDerivedData( + this.constructSchema(), + ), + }; + } } // used by integrations to synchronize the path with subscriptions, some @@ -377,9 +405,174 @@ export class ApolloServerBase { this.graphqlPath = path; } - private initSchema(): GraphQLSchema | Promise { + // The main goal of start is to assign to schemaDerivedData FIXME + // We must make sure that we manage the schemaDerivedData line before + // awaiting in the !gateway case, for installSubscriptionHandlers reasons + public async start(): Promise { + const initialState = this.state; + if ( + initialState.phase !== 'initialized with gateway' && + initialState.phase !== 'initialized with schema' + ) { + throw new Error( + `called start() with surprising state ${initialState.phase}`, + ); + } + const barrier = new Barrier(); + this.state = { phase: 'starting', barrier }; + try { + const schemaDerivedData = + initialState.phase === 'initialized with schema' + ? initialState.schemaDerivedData + : this.generateSchemaDerivedData( + await this.startGatewayAndLoadSchema(initialState.gateway), + ); + if (initialState.phase === 'initialized with schema') { + // This field is deprecated; users who are interested in learning + // their server's schema should make a plugin with serverWillStart, + // or register onSchemaChange on their gateway. It is only ever + // set for non-gateway servers. + this.schema = schemaDerivedData.schema; + } + + this.state = { + phase: 'invoking serverWillStart', + barrier, + schemaDerivedData, + }; + + const service: GraphQLServiceContext = { + logger: this.logger, + schema: schemaDerivedData.schema, + schemaHash: schemaDerivedData.schemaHash, + apollo: this.apolloConfig, + serverlessFramework: this.serverlessFramework(), + engine: { + serviceID: this.apolloConfig.graphId, + apiKeyHash: this.apolloConfig.keyHash, + }, + }; + + // The `persistedQueries` attribute on the GraphQLServiceContext was + // originally used by the operation registry, which shared the cache with + // it. This is no longer the case. However, while we are continuing to + // expand the support of the interface for `persistedQueries`, e.g. with + // additions like https://github.com/apollographql/apollo-server/pull/3623, + // we don't want to continually expand the API surface of what we expose + // to the plugin API. In this particular case, it certainly doesn't need + // to get the `ttl` default value which are intended for APQ only. + if (this.requestOptions.persistedQueries?.cache) { + service.persistedQueries = { + cache: this.requestOptions.persistedQueries.cache, + }; + } + + const serverListeners = ( + await Promise.all( + this.plugins.map( + (plugin) => + plugin.serverWillStart && plugin.serverWillStart(service), + ), + ) + ).filter( + (maybeServerListener): maybeServerListener is GraphQLServerListener => + typeof maybeServerListener === 'object' && + !!maybeServerListener.serverWillStop, + ); + this.toDispose.add(async () => { + await Promise.all( + serverListeners.map(({ serverWillStop }) => serverWillStop?.()), + ); + }); + + this.state = { phase: 'started', schemaDerivedData }; + } catch (error) { + this.state = { phase: 'failed to start', error }; + throw error; + } finally { + barrier.unblock(); + } + } + + // FIXME in AS3 this goes away + private async ensureStarted(): Promise { + while (true) { + switch (this.state.phase) { + case 'initialized with gateway': + case 'initialized with schema': + await this.start(); + // continue the while loop + break; + case 'starting': + case 'invoking serverWillStart': + await this.state.barrier.wait(); + // continue the while loop + break; + case 'failed to start': + throw this.state.error; + case 'started': + return this.state.schemaDerivedData; + case 'stopping': + throw new Error('Apollo Server is stopping'); + case 'stopped': + throw new Error('Apollo Server has stopped'); + default: + throw new UnreachableCaseError(this.state); + } + } + } + + // FIXME docs, remove in AS3 + protected ensureStarting() { + if ( + this.state.phase === 'initialized with gateway' || + this.state.phase === 'initialized with schema' + ) { + // Ah well. It would have been nice if the user had bothered + // to call and await `start()`; that way they'd be able to learn + // about any errors from it. Instead we'll kick it off here + // and just log errors. + this.start().catch((e) => console.error('FIXME', e)); + } + } + + private async startGatewayAndLoadSchema( + gateway: GraphQLService, + ): Promise { + // Store the unsubscribe handles, which are returned from + // `onSchemaChange`, for later disposal when the server stops + const unsubscriber = gateway.onSchemaChange((schema) => { + // If we're still happily running, update our schema-derived state. + if (this.state.phase === 'started') { + this.state.schemaDerivedData = this.generateSchemaDerivedData(schema); + } + }); + this.toDispose.add(async () => unsubscriber()); + + // For backwards compatibility with old versions of @apollo/gateway. + const engineConfig = + this.apolloConfig.keyHash && this.apolloConfig.graphId + ? { + apiKeyHash: this.apolloConfig.keyHash, + graphId: this.apolloConfig.graphId, + graphVariant: this.apolloConfig.graphVariant, + } + : undefined; + + // Set the executor whether the gateway 'load' call succeeds or not. + // FIXME rethink this or at least doc it better + this.requestOptions.executor = gateway.executor; + + const config = await gateway.load({ + apollo: this.apolloConfig, + engine: engineConfig, + }); + this.toDispose.add(async () => await gateway.stop?.()); + return config.schema; + } + + private constructSchema(): GraphQLSchema { const { - gateway, schema, modules, typeDefs, @@ -387,119 +580,72 @@ export class ApolloServerBase { schemaDirectives, parseOptions, } = this.config; - if (gateway) { - // Store the unsubscribe handles, which are returned from - // `onSchemaChange`, for later disposal when the server stops - const unsubscriber = gateway.onSchemaChange( - (schema) => - (this.schemaDerivedData = Promise.resolve( - this.generateSchemaDerivedData(schema), - )), - ); - this.toDispose.add(async () => unsubscriber()); - - // For backwards compatibility with old versions of @apollo/gateway. - const engineConfig = - this.apolloConfig.keyHash && this.apolloConfig.graphId - ? { - apiKeyHash: this.apolloConfig.keyHash, - graphId: this.apolloConfig.graphId, - graphVariant: this.apolloConfig.graphVariant, - } - : undefined; - - // Set the executor whether the gateway 'load' call succeeds or not. - // If the schema becomes available eventually (after a setInterval retry) - // this executor will still be necessary in order to be able to support - // a federated schema! - this.requestOptions.executor = gateway.executor; - - return gateway - .load({ apollo: this.apolloConfig, engine: engineConfig }) - .then((config) => { - this.toDispose.add(async () => await gateway.stop?.()); - return config.schema; - }) - .catch((err) => { - // We intentionally do not re-throw the exact error from the gateway - // configuration as it may contain implementation details and this - // error will propagate to the client. We will, however, log the error - // for observation in the logs. - const message = 'This data graph is missing a valid configuration.'; - this.logger.error(message + ' ' + ((err && err.message) || err)); - throw new Error( - message + ' More details may be available in the server logs.', - ); - }); + if (schema) { + return schema; } - let constructedSchema: GraphQLSchema; - if (schema) { - constructedSchema = schema; - } else if (modules) { + if (modules) { const { schema, errors } = buildServiceDefinition(modules); if (errors && errors.length > 0) { - throw new Error(errors.map(error => error.message).join('\n\n')); - } - constructedSchema = schema!; - } else { - if (!typeDefs) { - throw Error( - 'Apollo Server requires either an existing schema, modules or typeDefs', - ); + throw new Error(errors.map((error) => error.message).join('\n\n')); } + return schema!; + } - const augmentedTypeDefs = Array.isArray(typeDefs) ? typeDefs : [typeDefs]; - - // We augment the typeDefs with the @cacheControl directive and associated - // scope enum, so makeExecutableSchema won't fail SDL validation + if (!typeDefs) { + throw Error( + 'Apollo Server requires either an existing schema, modules or typeDefs', + ); + } - if (!isDirectiveDefined(augmentedTypeDefs, 'cacheControl')) { - augmentedTypeDefs.push( - gql` - enum CacheControlScope { - PUBLIC - PRIVATE - } + const augmentedTypeDefs = Array.isArray(typeDefs) ? typeDefs : [typeDefs]; - directive @cacheControl( - maxAge: Int - scope: CacheControlScope - ) on FIELD_DEFINITION | OBJECT | INTERFACE - `, - ); - } + // We augment the typeDefs with the @cacheControl directive and associated + // scope enum, so makeExecutableSchema won't fail SDL validation - if (this.uploadsConfig) { - const { GraphQLUpload } = require('@apollographql/graphql-upload-8-fork'); - if (Array.isArray(resolvers)) { - if (resolvers.every(resolver => !resolver.Upload)) { - resolvers.push({ Upload: GraphQLUpload }); - } - } else { - if (resolvers && !resolvers.Upload) { - resolvers.Upload = GraphQLUpload; + if (!isDirectiveDefined(augmentedTypeDefs, 'cacheControl')) { + augmentedTypeDefs.push( + gql` + enum CacheControlScope { + PUBLIC + PRIVATE } - } - // We augment the typeDefs with the Upload scalar, so typeDefs that - // don't include it won't fail - augmentedTypeDefs.push( - gql` - scalar Upload - `, - ); + directive @cacheControl( + maxAge: Int + scope: CacheControlScope + ) on FIELD_DEFINITION | OBJECT | INTERFACE + `, + ); + } + + if (this.uploadsConfig) { + const { GraphQLUpload } = require('@apollographql/graphql-upload-8-fork'); + if (Array.isArray(resolvers)) { + if (resolvers.every((resolver) => !resolver.Upload)) { + resolvers.push({ Upload: GraphQLUpload }); + } + } else { + if (resolvers && !resolvers.Upload) { + resolvers.Upload = GraphQLUpload; + } } - constructedSchema = makeExecutableSchema({ - typeDefs: augmentedTypeDefs, - schemaDirectives, - resolvers, - parseOptions, - }); + // We augment the typeDefs with the Upload scalar, so typeDefs that + // don't include it won't fail + augmentedTypeDefs.push( + gql` + scalar Upload + `, + ); } - return constructedSchema; + return makeExecutableSchema({ + typeDefs: augmentedTypeDefs, + schemaDirectives, + resolvers, + parseOptions, + }); } private generateSchemaDerivedData(schema: GraphQLSchema): SchemaDerivedData { @@ -536,71 +682,22 @@ export class ApolloServerBase { }; } - protected async willStart() { - try { - var { schema, schemaHash } = await this.schemaDerivedData; - } catch (err) { - // The `schemaDerivedData` can throw if the Promise it points to does not - // resolve with a `GraphQLSchema`. As errors from `willStart` are start-up - // errors, other Apollo middleware after us will not be called, including - // our health check, CORS, etc. - // - // Returning here allows the integration's other Apollo middleware to - // function properly in the event of a failure to obtain the data graph - // configuration from the gateway's `load` method during initialization. - return; - } - - const service: GraphQLServiceContext = { - logger: this.logger, - schema: schema, - schemaHash: schemaHash, - apollo: this.apolloConfig, - serverlessFramework: this.serverlessFramework(), - engine: { - serviceID: this.apolloConfig.graphId, - apiKeyHash: this.apolloConfig.keyHash, - }, - }; - - // The `persistedQueries` attribute on the GraphQLServiceContext was - // originally used by the operation registry, which shared the cache with - // it. This is no longer the case. However, while we are continuing to - // expand the support of the interface for `persistedQueries`, e.g. with - // additions like https://github.com/apollographql/apollo-server/pull/3623, - // we don't want to continually expand the API surface of what we expose - // to the plugin API. In this particular case, it certainly doesn't need - // to get the `ttl` default value which are intended for APQ only. - if (this.requestOptions.persistedQueries?.cache) { - service.persistedQueries = { - cache: this.requestOptions.persistedQueries.cache, - }; - } - - const serverListeners = ( - await Promise.all( - this.plugins.map( - (plugin) => plugin.serverWillStart && plugin.serverWillStart(service), - ), - ) - ).filter( - (maybeServerListener): maybeServerListener is GraphQLServerListener => - typeof maybeServerListener === 'object' && - !!maybeServerListener.serverWillStop, - ); - this.toDispose.add(async () => { - await Promise.all( - serverListeners.map(({ serverWillStop }) => serverWillStop?.()), - ); - }); - } - public async stop() { - await Promise.all([...this.toDispose].map(dispose => dispose())); + // FIXME read state to make this idempotenty + this.state = { phase: 'stopping' }; + await Promise.all([...this.toDispose].map((dispose) => dispose())); if (this.subscriptionServer) this.subscriptionServer.close(); + this.state = { phase: 'stopped' }; } - public installSubscriptionHandlers(server: HttpServer | HttpsServer | Http2Server | Http2SecureServer | WebSocket.Server) { + public installSubscriptionHandlers( + server: + | HttpServer + | HttpsServer + | Http2Server + | Http2SecureServer + | WebSocket.Server, + ) { if (!this.subscriptionServerOptions) { if (this.config.gateway) { throw Error( @@ -625,12 +722,30 @@ export class ApolloServerBase { path, } = this.subscriptionServerOptions; - // TODO: This shouldn't use this.schema, as it is deprecated in favor of the schemaDerivedData promise. - const schema = this.schema; - if (this.schema === undefined) - throw new Error( - 'Schema undefined during creation of subscription server.', - ); + let schema: GraphQLSchema; + switch (this.state.phase) { + case 'initialized with schema': + case 'invoking serverWillStart': + case 'started': + schema = this.state.schemaDerivedData.schema; + break; + case 'initialized with gateway': + // shouldn't happen: gateway doesn't support subs + case 'starting': + // shouldn't happen: there's no await between 'starting' and + // 'invoking serverWillStart' without gateway + case 'failed to start': + // only happens if you call 'start' yourself, in which case you really + // ought to see what happens before calling this function + case 'stopping': + case 'stopped': + // stopping is unlikely to happen during startup + throw new Error( + `Can't install subscription handlers when state is ${this.state.phase}`, + ); + default: + throw new UnreachableCaseError(this.state); + } this.subscriptionServer = SubscriptionServer.create( { @@ -674,14 +789,14 @@ export class ApolloServerBase { return { ...connection, context }; }, keepAlive, - validationRules: this.requestOptions.validationRules + validationRules: this.requestOptions.validationRules, }, server instanceof NetServer || server instanceof TlsServer ? { - server, - path, - } - : server + server, + path, + } + : server, ); } @@ -697,38 +812,6 @@ export class ApolloServerBase { return false; } - // Returns true if it appears that the schema was returned from - // @apollo/federation's buildFederatedSchema. This strategy avoids depending - // explicitly on @apollo/federation or relying on something that might not - // survive transformations like monkey-patching a boolean field onto the - // schema. - // - // This is used for two things: - // 1) Determining whether traces should be added to responses if requested - // with an HTTP header. If you want to include these traces even for - // non-federated schemas (when requested via header) you can use - // ApolloServerPluginInlineTrace yourself; if you want to never - // include these traces even for federated schemas you can use - // ApolloServerPluginInlineTraceDisabled. - // 2) Determining whether schema-reporting should be allowed; federated - // services shouldn't be reporting schemas, and we accordingly throw if - // it's attempted. - private schemaIsFederated(schema: GraphQLSchema): boolean { - const serviceType = schema.getType('_Service'); - if (!(serviceType && isObjectType(serviceType))) { - return false; - } - const sdlField = serviceType.getFields().sdl; - if (!sdlField) { - return false; - } - const sdlFieldType = sdlField.type; - if (!isScalarType(sdlFieldType)) { - return false; - } - return sdlFieldType.name == 'String'; - } - private ensurePluginInstantiation(plugins: PluginDefinition[] = []): void { const pluginsToInit: PluginDefinition[] = []; @@ -744,7 +827,7 @@ export class ApolloServerBase { // inline tracing plugin may be what you want, or just usage reporting if // the goal is to get traces to Apollo's servers.) if (this.config.tracing) { - pluginsToInit.push(pluginTracing()) + pluginsToInit.push(pluginTracing()); } // Enable cache control unless it was explicitly disabled. @@ -776,11 +859,9 @@ export class ApolloServerBase { pluginsToInit.push(pluginCacheControl(cacheControlOptions)); } - const federatedSchema = this.schema && this.schemaIsFederated(this.schema); - pluginsToInit.push(...plugins); - this.plugins = pluginsToInit.map(plugin => { + this.plugins = pluginsToInit.map((plugin) => { if (typeof plugin === 'function') { return plugin(); } @@ -832,23 +913,13 @@ export class ApolloServerBase { typeof engine === 'object' && (engine.reportSchema || engine.experimental_schemaReporting); if (alreadyHavePlugin || enabledViaEnvVar || enabledViaLegacyOption) { - if (federatedSchema) { - throw Error( - [ - 'Schema reporting is not yet compatible with federated services.', - "If you're interested in using schema reporting with federated", - 'services, please contact Apollo support. To set up managed federation, see', - 'https://go.apollo.dev/s/managed-federation' - ].join(' '), - ); - } if (this.config.gateway) { throw new Error( [ "Schema reporting is not yet compatible with the gateway. If you're", 'interested in using schema reporting with the gateway, please', 'contact Apollo support. To set up managed federation, see', - 'https://go.apollo.dev/s/managed-federation' + 'https://go.apollo.dev/s/managed-federation', ].join(' '), ); } @@ -904,17 +975,15 @@ export class ApolloServerBase { 'https://go.apollo.dev/s/migration-engine-plugins', ); } - } else if (federatedSchema && this.config.engine !== false) { - // If we have a federated schema, and we haven't explicitly disabled inline - // tracing via ApolloServerPluginInlineTraceDisabled or engine:false, - // we set up inline tracing. + } else if (this.config.engine !== false) { + // If we haven't explicitly disabled inline tracing via + // ApolloServerPluginInlineTraceDisabled or engine:false, + // we set up inline tracing in "only if federated" mode. // (This is slightly different than the pre-ApolloServerPluginInlineTrace where // we would also avoid doing this if an API key was configured and log a warning.) - this.logger.info( - 'Enabling inline tracing for this federated service. To disable, use ' + - 'ApolloServerPluginInlineTraceDisabled.', - ); - const options: ApolloServerPluginInlineTraceOptions = {}; + const options: ApolloServerPluginInlineTraceOptions = { + __onlyIfSchemaIsFederated: true, + }; if (typeof engine === 'object') { options.rewriteError = engine.rewriteError; } @@ -931,8 +1000,7 @@ export class ApolloServerBase { // for unicode characters, etc.), but it should do a reasonable job at // providing a caching document store for most operations. maxSize: - Math.pow(2, 20) * - (this.experimental_approximateDocumentStoreMiB || 30), + Math.pow(2, 20) * (this.experimental_approximateDocumentStoreMiB || 30), sizeCalculator: approximateObjectSize, }); } @@ -943,12 +1011,25 @@ export class ApolloServerBase { protected async graphQLServerOptions( integrationContextArgument?: Record, ): Promise { - const { - schema, - schemaHash, - documentStore, - extensions, - } = await this.schemaDerivedData; + let schemaDerivedData: SchemaDerivedData; + try { + schemaDerivedData = await this.ensureStarted(); + } catch (err) { + // We intentionally do not re-throw the exact error from the gateway + // configuration as it may contain implementation details and this error + // will propagate to the client. We will, however, log the error for + // observation in the logs. + // FIXME improve wording + this.logger.error( + 'Apollo Server failed to start correctly. Consider calling `await server.start()` immediately after `server = new ApolloServer()` to make this error stop your server from starting up. ' + + ((err && err.message) || err), + ); + throw new Error( + 'This data graph is missing a valid configuration. More details may be available in the server logs.', + ); + } + + const { schema, schemaHash, documentStore, extensions } = schemaDerivedData; let context: Context = this.context ? this.context : {}; @@ -1001,7 +1082,6 @@ export class ApolloServerBase { options.context = cloneObject(options.context); } - const requestCtx: GraphQLRequestContext = { logger: this.logger, schema: options.schema, diff --git a/packages/apollo-server-core/src/__tests__/ApolloServerBase.test.ts b/packages/apollo-server-core/src/__tests__/ApolloServerBase.test.ts index ecd00afd2d9..5de0353a247 100644 --- a/packages/apollo-server-core/src/__tests__/ApolloServerBase.test.ts +++ b/packages/apollo-server-core/src/__tests__/ApolloServerBase.test.ts @@ -37,16 +37,15 @@ describe('ApolloServerBase construction', () => { it('succeeds when passed a graphVariant in construction', () => { let serverBase; - expect( - () => - new ApolloServerBase({ - typeDefs, - resolvers, - engine: { - graphVariant: 'foo', - apiKey: 'not:real:key', - }, - }).stop() + expect(() => + new ApolloServerBase({ + typeDefs, + resolvers, + engine: { + graphVariant: 'foo', + apiKey: 'not:real:key', + }, + }).stop(), ).not.toThrow(); }); @@ -88,7 +87,7 @@ describe('ApolloServerBase construction', () => { schema: {}, }); }).toThrowErrorMatchingInlineSnapshot( - `"Unexpected error: Unable to resolve a valid GraphQLSchema. Please file an issue with a reproduction of this error, if possible."`, + `"Expected {} to be a GraphQL schema."`, ); }); diff --git a/packages/apollo-server-core/src/plugin/inlineTrace/index.ts b/packages/apollo-server-core/src/plugin/inlineTrace/index.ts index b15772e2c79..4448b3bfc6e 100644 --- a/packages/apollo-server-core/src/plugin/inlineTrace/index.ts +++ b/packages/apollo-server-core/src/plugin/inlineTrace/index.ts @@ -2,8 +2,13 @@ import { Trace } from 'apollo-reporting-protobuf'; import { TraceTreeBuilder } from '../traceTreeBuilder'; import type { ApolloServerPluginUsageReportingOptions } from '../usageReporting/options'; import type { InternalApolloServerPlugin } from '../internalPlugin'; +import { schemaIsFederated } from '../schemaIsFederated'; export interface ApolloServerPluginInlineTraceOptions { + /** + * FIXME doc or don't + */ + __onlyIfSchemaIsFederated?: boolean; /** * By default, all errors from this service get included in the trace. You * can specify a filter function to exclude specific errors from being @@ -21,11 +26,31 @@ export interface ApolloServerPluginInlineTraceOptions { export function ApolloServerPluginInlineTrace( options: ApolloServerPluginInlineTraceOptions = Object.create(null), ): InternalApolloServerPlugin { + let enabled: boolean | null = options.__onlyIfSchemaIsFederated ? null : true; return { __internal_plugin_id__() { return 'InlineTrace'; }, + serverWillStart({ schema, logger }) { + // Handle the case that the plugin was implicitly installed. We only want it + // to actually be active if the schema appears to be federated. If you don't + // like the log line, just install `ApolloServerPluginInlineTrace()` in + // `plugins` yourself. + if (enabled === null) { + enabled = schemaIsFederated(schema); + if (enabled) { + logger.info( + 'Enabling inline tracing for this federated service. To disable, use ' + + 'ApolloServerPluginInlineTraceDisabled.', + ); + } + } + }, requestDidStart({ request: { http } }) { + if (!enabled) { + return; + } + const treeBuilder = new TraceTreeBuilder({ rewriteError: options.rewriteError, }); diff --git a/packages/apollo-server-core/src/plugin/schemaIsFederated.ts b/packages/apollo-server-core/src/plugin/schemaIsFederated.ts new file mode 100644 index 00000000000..4d491720d2b --- /dev/null +++ b/packages/apollo-server-core/src/plugin/schemaIsFederated.ts @@ -0,0 +1,33 @@ +import { GraphQLSchema, isObjectType, isScalarType } from 'graphql'; + +// Returns true if it appears that the schema was returned from +// @apollo/federation's buildFederatedSchema. This strategy avoids depending +// explicitly on @apollo/federation or relying on something that might not +// survive transformations like monkey-patching a boolean field onto the +// schema. +// +// This is used for two things: +// 1) Determining whether traces should be added to responses if requested +// with an HTTP header. If you want to include these traces even for +// non-federated schemas (when requested via header) you can use +// ApolloServerPluginInlineTrace yourself; if you want to never +// include these traces even for federated schemas you can use +// ApolloServerPluginInlineTraceDisabled. +// 2) Determining whether schema-reporting should be allowed; federated +// services shouldn't be reporting schemas, and we accordingly throw if +// it's attempted. +export function schemaIsFederated(schema: GraphQLSchema): boolean { + const serviceType = schema.getType('_Service'); + if (!(serviceType && isObjectType(serviceType))) { + return false; + } + const sdlField = serviceType.getFields().sdl; + if (!sdlField) { + return false; + } + const sdlFieldType = sdlField.type; + if (!isScalarType(sdlFieldType)) { + return false; + } + return sdlFieldType.name == 'String'; +} diff --git a/packages/apollo-server-core/src/plugin/schemaReporting/index.ts b/packages/apollo-server-core/src/plugin/schemaReporting/index.ts index 8501e2484b9..4d7a4806ef1 100644 --- a/packages/apollo-server-core/src/plugin/schemaReporting/index.ts +++ b/packages/apollo-server-core/src/plugin/schemaReporting/index.ts @@ -4,6 +4,7 @@ import { v4 as uuidv4 } from 'uuid'; import { printSchema, validateSchema, buildSchema } from 'graphql'; import { SchemaReporter } from './schemaReporter'; import createSHA from '../../utils/createSHA'; +import { schemaIsFederated } from '../schemaIsFederated'; export interface ApolloServerPluginSchemaReportingOptions { /** @@ -97,6 +98,17 @@ export function ApolloServerPluginSchemaReporting( } } + if (schemaIsFederated(schema)) { + throw Error( + [ + 'Schema reporting is not yet compatible with federated services.', + "If you're interested in using schema reporting with federated", + 'services, please contact Apollo support. To set up managed federation, see', + 'https://go.apollo.dev/s/managed-federation', + ].join(' '), + ); + } + const executableSchema = overrideReportedSchema ?? printSchema(schema); const executableSchemaId = computeExecutableSchemaId(executableSchema); diff --git a/packages/apollo-server-express/src/ApolloServer.ts b/packages/apollo-server-express/src/ApolloServer.ts index 9fd6915ab56..8296e3e12b9 100644 --- a/packages/apollo-server-express/src/ApolloServer.ts +++ b/packages/apollo-server-express/src/ApolloServer.ts @@ -122,25 +122,12 @@ export class ApolloServer extends ApolloServerBase { }: GetMiddlewareOptions = {}): express.Router { if (!path) path = '/graphql'; - const router = express.Router(); + // In case the user didn't bother to call and await the `start` method, we + // kick it off in the background (with any errors getting logged + // and also rethrown from graphQLServerOptions during later requests). + this.ensureStarting(); - // Despite the fact that this `applyMiddleware` function is `async` in - // other integrations (e.g. Hapi), currently it is not for Express (@here). - // That should change in a future version, but that would be a breaking - // change right now (see comment above this method's declaration above). - // - // That said, we do need to await the `willStart` lifecycle event which - // can perform work prior to serving a request. Since Express doesn't - // natively support Promises yet, we'll do this via a middleware that - // calls `next` when the `willStart` finishes. We'll kick off the - // `willStart` right away, so hopefully it'll finish before the first - // request comes in, but we won't call `next` on this middleware until it - // does. (And we'll take care to surface any errors via the `.catch`-able.) - const promiseWillStart = this.willStart(); - - router.use(path, (_req, _res, next) => { - promiseWillStart.then(() => next()).catch(next); - }); + const router = express.Router(); if (!disableHealthCheck) { router.use('/.well-known/apollo/server-health', (req, res) => { diff --git a/packages/apollo-server-express/src/__tests__/ApolloServer.test.ts b/packages/apollo-server-express/src/__tests__/ApolloServer.test.ts index 5de96fc374a..bced5f2909a 100644 --- a/packages/apollo-server-express/src/__tests__/ApolloServer.test.ts +++ b/packages/apollo-server-express/src/__tests__/ApolloServer.test.ts @@ -36,8 +36,11 @@ describe('apollo-server-express', () => { let server; let httpServer; testApolloServer( - async options => { + async (options, suppressStartCall?: boolean) => { server = new ApolloServer(options); + if (!suppressStartCall) { + await server.start(); + } const app = express(); server.applyMiddleware({ app }); httpServer = await new Promise(resolve => { @@ -46,8 +49,8 @@ describe('apollo-server-express', () => { return createServerInfo(server, httpServer); }, async () => { - if (server) await server.stop(); if (httpServer && httpServer.listening) await httpServer.close(); + if (server) await server.stop(); }, ); }); diff --git a/packages/apollo-server-fastify/src/ApolloServer.ts b/packages/apollo-server-fastify/src/ApolloServer.ts index 42bc458b229..59e3293420a 100644 --- a/packages/apollo-server-fastify/src/ApolloServer.ts +++ b/packages/apollo-server-fastify/src/ApolloServer.ts @@ -85,13 +85,15 @@ export class ApolloServer extends ApolloServerBase { onHealthCheck, }: ServerRegistration = {}) { this.graphqlPath = path ? path : '/graphql'; - const promiseWillStart = this.willStart(); + + // In case the user didn't bother to call and await the `start` method, we + // kick it off in the background (with any errors getting logged + // and also rethrown from graphQLServerOptions during later requests). + this.ensureStarting(); return async ( app: FastifyInstance, ) => { - await promiseWillStart; - if (!disableHealthCheck) { app.get('/.well-known/apollo/server-health', async (req, res) => { // Response follows https://tools.ietf.org/html/draft-inadarei-api-health-check-01 diff --git a/packages/apollo-server-fastify/src/__tests__/ApolloServer.test.ts b/packages/apollo-server-fastify/src/__tests__/ApolloServer.test.ts index c38730cd47f..b7b2e892162 100644 --- a/packages/apollo-server-fastify/src/__tests__/ApolloServer.test.ts +++ b/packages/apollo-server-fastify/src/__tests__/ApolloServer.test.ts @@ -37,8 +37,11 @@ describe('apollo-server-fastify', () => { let app: FastifyInstance; testApolloServer( - async options => { + async (options, suppressStartCall?: boolean) => { server = new ApolloServer(options); + if (!suppressStartCall) { + await server.start(); + } app = fastify(); app.register(server.createHandler()); await app.listen(port); diff --git a/packages/apollo-server-fastify/src/__tests__/datasource.test.ts b/packages/apollo-server-fastify/src/__tests__/datasource.test.ts index 65b55c21160..97e3d811a81 100644 --- a/packages/apollo-server-fastify/src/__tests__/datasource.test.ts +++ b/packages/apollo-server-fastify/src/__tests__/datasource.test.ts @@ -60,7 +60,6 @@ restAPI.get('/str/:id', (req, res) => { }); describe('apollo-server-fastify', () => { - let restServer: FastifyInstance; let app: FastifyInstance; beforeAll(async () => { @@ -68,7 +67,7 @@ describe('apollo-server-fastify', () => { }); afterAll(async () => { - await new Promise(resolve => restServer.close(() => resolve())); + await new Promise(resolve => restAPI.close(() => resolve())); }); let server: ApolloServer; @@ -79,7 +78,7 @@ describe('apollo-server-fastify', () => { afterEach(async () => { await server.stop(); - await new Promise(resolve => app.close(() => resolve())); + await new Promise(resolve => app.close(() => resolve())); }); it('uses the cache', async () => { @@ -90,10 +89,11 @@ describe('apollo-server-fastify', () => { id: new IdAPI(), }), }); + await server.start(); app = fastify(); app.register(server.createHandler()); - await app.listen(6667); + await app.listen(0); const { url: uri } = createServerInfo(server, app.server); const apolloFetch = createApolloFetch({ uri }); @@ -118,10 +118,11 @@ describe('apollo-server-fastify', () => { id: new IdAPI(), }), }); + await server.start(); app = fastify(); app.register(server.createHandler()); - await app.listen(6668); + await app.listen(0); const { url: uri } = createServerInfo(server, app.server); const apolloFetch = createApolloFetch({ uri }); diff --git a/packages/apollo-server-hapi/src/ApolloServer.ts b/packages/apollo-server-hapi/src/ApolloServer.ts index 0575d89c149..cafd8fc7ed4 100644 --- a/packages/apollo-server-hapi/src/ApolloServer.ts +++ b/packages/apollo-server-hapi/src/ApolloServer.ts @@ -60,7 +60,10 @@ export class ApolloServer extends ApolloServerBase { disableHealthCheck, onHealthCheck, }: ServerRegistration) { - await this.willStart(); + // In case the user didn't bother to call and await the `start` method, we + // kick it off in the background (with any errors getting logged + // and also rethrown from graphQLServerOptions during later requests). + this.ensureStarting(); if (!path) path = '/graphql'; diff --git a/packages/apollo-server-hapi/src/__tests__/ApolloServer.test.ts b/packages/apollo-server-hapi/src/__tests__/ApolloServer.test.ts index 7699878bcbc..38fb11f40c6 100644 --- a/packages/apollo-server-hapi/src/__tests__/ApolloServer.test.ts +++ b/packages/apollo-server-hapi/src/__tests__/ApolloServer.test.ts @@ -27,9 +27,12 @@ const port = 0; const { Server } = require('hapi'); testApolloServer( - async options => { + async (options, suppressStartCall?: boolean) => { server = new ApolloServer(options); app = new Server({ host: 'localhost', port }); + if (!suppressStartCall) { + await server.start(); + } await server.applyMiddleware({ app }); await app.start(); const httpServer = app.listener; diff --git a/packages/apollo-server-integration-testsuite/src/ApolloServer.ts b/packages/apollo-server-integration-testsuite/src/ApolloServer.ts index 295b54e0b23..57e3e8ef814 100644 --- a/packages/apollo-server-integration-testsuite/src/ApolloServer.ts +++ b/packages/apollo-server-integration-testsuite/src/ApolloServer.ts @@ -45,6 +45,7 @@ import { ApolloServerPluginInlineTrace, ApolloServerPluginUsageReporting, ApolloServerPluginUsageReportingOptions, + GraphQLServiceConfig, } from 'apollo-server-core'; import { GraphQLExtension, GraphQLResponse } from 'graphql-extensions'; import { TracingFormat } from 'apollo-tracing'; @@ -123,28 +124,31 @@ const makeGatewayMock = ({ unsubscribeSpy?: () => void; executor?: GraphQLExecutor; } = {}) => { + let resolution: GraphQLServiceConfig | null = null; + let rejection: Error | null = null; const eventuallyAssigned = { - resolveLoad: null as ({ schema, executor }) => void, - rejectLoad: null as (err: Error) => void, + resolveLoad: (config: GraphQLServiceConfig) => { + resolution = config; + }, + rejectLoad: (err: Error) => { + rejection = err; + }, triggerSchemaChange: null as (newSchema) => void, }; - const mockedLoadResults = new Promise<{ - schema: GraphQLSchema; - executor: GraphQLExecutor; - }>((resolve, reject) => { - eventuallyAssigned.resolveLoad = ({ schema, executor }) => { - resolve({ schema, executor }); - }; - eventuallyAssigned.rejectLoad = (err: Error) => { - reject(err); - }; - }); const mockedGateway: GraphQLService = { executor, - load: options => { + load: async (options) => { optionsSpy(options); - return mockedLoadResults; + // Make sure it's async + await new Promise(res => setImmediate(res)); + if (rejection) { + throw rejection; + } + if (resolution) { + return resolution; + } + throw Error("Neither resolving nor rejecting?"); }, onSchemaChange: callback => { eventuallyAssigned.triggerSchemaChange = callback; @@ -165,7 +169,7 @@ export interface ServerInfo { } export interface CreateServerFunc { - (config: Config): Promise>; + (config: Config, suppressStartCall?: boolean): Promise>; } export interface StopServerFunc { @@ -387,6 +391,18 @@ export function testApolloServer( expect(executor).toHaveBeenCalled(); }); + it("rejected load promise is thrown by server.start", async () => { + const { gateway, triggers } = makeGatewayMock(); + + const loadError = new Error("load error which should be be thrown by start"); + triggers.rejectLoad(loadError); + + expect(createApolloServer({ + gateway, + subscriptions: false, + })).rejects.toThrowError(loadError); + }); + it("rejected load promise acts as an error boundary", async () => { const executor = jest.fn(); executor.mockResolvedValueOnce( @@ -407,7 +423,7 @@ export function testApolloServer( const { url: uri } = await createApolloServer({ gateway, subscriptions: false, - }); + }, true); const apolloFetch = createApolloFetch({ uri }); const result = await apolloFetch({ query: '{testString}' }); @@ -423,7 +439,7 @@ export function testApolloServer( }) ); expect(consoleErrorSpy).toHaveBeenCalledWith( - "This data graph is missing a valid configuration. " + + "Apollo Server failed to start correctly. Consider calling `await server.start()` immediately after `server = new ApolloServer()` to make this error stop your server from starting up. " + "load error which should be masked"); expect(executor).not.toHaveBeenCalled(); }); @@ -2673,7 +2689,7 @@ export function testApolloServer( }); it('reports a total duration that is longer than the duration of its resolvers', async () => { - const { url: uri } = await createApolloServer({ + const { url: uri, server } = await createApolloServer({ typeDefs: allTypeDefs, resolvers, }); @@ -3279,7 +3295,7 @@ export function testApolloServer( const { gateway, triggers } = makeGatewayMock({ optionsSpy }); triggers.resolveLoad({ schema, executor: () => {} }); - await createApolloServer({ + const { server } = await createApolloServer({ gateway, subscriptions: false, apollo: { key: 'service:tester:1234abc', graphVariant: 'staging' }, diff --git a/packages/apollo-server-integration-testsuite/src/index.ts b/packages/apollo-server-integration-testsuite/src/index.ts index e39cb84d036..c06108eb694 100644 --- a/packages/apollo-server-integration-testsuite/src/index.ts +++ b/packages/apollo-server-integration-testsuite/src/index.ts @@ -31,6 +31,20 @@ import { PersistedQueryNotFoundError } from "apollo-server-errors"; export * from './ApolloServer'; +// FIXME should find somewhere better to put this (just publish a little package?) +class Barrier { + private resolvePromise!: () => void; + private promise = new Promise((r) => { + this.resolvePromise = r; + }); + async wait() { + await this.promise; + } + unblock() { + this.resolvePromise(); + } +} + export const NODE_MAJOR_VERSION: number = parseInt( process.versions.node.split('.', 1)[0], 10, @@ -1119,15 +1133,18 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => { describe('request pipeline plugins', () => { describe('lifecycle hooks', () => { + // This tests the backwards-compatibility behavior that ensures + // that even if you don't call server.start() yourself, the first + // GraphQL request will result in starting the server before + // serving the first request it('calls serverWillStart before serving a request', async () => { - // We'll use this eventually-assigned function to programmatically - // resolve the `serverWillStart` event. - let resolveServerWillStart: Function; - - // We'll use this mocked function to determine the order in which + // We'll use this to determine the order in which // the events we're expecting to happen actually occur and validate // those expectations in various stages of this test. - const fn = jest.fn(); + const calls: string[] = []; + + const pluginStartedBarrier = new Barrier; + const letPluginFinishBarrier = new Barrier; // We want this to create the app as fast as `createApp` will allow. // for integrations whose `applyMiddleware` currently returns a @@ -1138,35 +1155,17 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => { schema, plugins: [ { - serverWillStart() { - fn('zero'); - return new Promise(resolve => { - resolveServerWillStart = () => { - fn('one'); - resolve(); - }; - }); + async serverWillStart() { + calls.push('zero'); + pluginStartedBarrier.unblock(); + await letPluginFinishBarrier.wait(); + calls.push('one'); }, }, ], }, }); - const delayUntil = async (check: () => boolean, expectedNumTicks) => { - if (check()) return expect(expectedNumTicks).toBe(0); - else expect(expectedNumTicks).not.toBe(0); - return new Promise(resolve => - process.nextTick(() => - delayUntil(check, expectedNumTicks - 1).then(resolve), - ), - ); - }; - - // Make sure that things were called in the expected order. - await delayUntil(() => fn.mock.calls.length === 1, 1); - expect(fn.mock.calls).toEqual([['zero']]); - resolveServerWillStart(); - // Account for the fact that `createApp` might return a Promise, // and might not, depending on the integration's implementation of // createApp. This is entirely to account for the fact that @@ -1185,18 +1184,22 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => { query: 'query test{ testString }', }) .then(res => { - fn('two'); + calls.push('two'); return res; }); - // Ensure the request has not gone through. - expect(fn.mock.calls).toEqual([['zero'], ['one']]); + // At this point calls might be [] or ['zero'] because the back-compat + // code kicks off start() asynchronously. We can safely wait on + // the plugin's serverWillStart to begin. + await pluginStartedBarrier.wait(); + expect(calls).toEqual(['zero']); + letPluginFinishBarrier.unblock(); // Now, wait for the request to finish. await res; // Finally, ensure that the order we expected was achieved. - expect(fn.mock.calls).toEqual([['zero'], ['one'], ['two']]); + expect(calls).toEqual(['zero', 'one', 'two']); }); }); }); diff --git a/packages/apollo-server-koa/src/ApolloServer.ts b/packages/apollo-server-koa/src/ApolloServer.ts index 2043e299f60..3608744b52c 100644 --- a/packages/apollo-server-koa/src/ApolloServer.ts +++ b/packages/apollo-server-koa/src/ApolloServer.ts @@ -101,26 +101,12 @@ export class ApolloServer extends ApolloServerBase { }: GetMiddlewareOptions = {}): Middleware { if (!path) path = '/graphql'; - // Despite the fact that this `applyMiddleware` function is `async` in - // other integrations (e.g. Hapi), currently it is not for Koa (@here). - // That should change in a future version, but that would be a breaking - // change right now (see comment above this method's declaration above). - // - // That said, we do need to await the `willStart` lifecycle event which - // can perform work prior to serving a request. While we could do this - // via awaiting in a Koa middleware, well kick off `willStart` right away, - // so hopefully it'll finish before the first request comes in. We won't - // call `next` until it's ready, which will effectively yield until that - // work has finished. Any errors will be surfaced to Koa through its own - // native Promise-catching facilities. - const promiseWillStart = this.willStart(); + // In case the user didn't bother to call and await the `start` method, we + // kick it off in the background (with any errors getting logged + // and also rethrown from graphQLServerOptions during later requests). + this.ensureStarting(); + const middlewares = []; - middlewares.push( - middlewareFromPath(path, async (_ctx: Koa.Context, next: Function) => { - await promiseWillStart; - return next(); - }), - ); if (!disableHealthCheck) { middlewares.push( diff --git a/packages/apollo-server-koa/src/__tests__/ApolloServer.test.ts b/packages/apollo-server-koa/src/__tests__/ApolloServer.test.ts index af8374b9e59..c5a4995e3d8 100644 --- a/packages/apollo-server-koa/src/__tests__/ApolloServer.test.ts +++ b/packages/apollo-server-koa/src/__tests__/ApolloServer.test.ts @@ -41,8 +41,11 @@ const resolvers = { let server: ApolloServer; let httpServer: http.Server; testApolloServer( - async options => { + async (options, suppressStartCall?: boolean) => { server = new ApolloServer(options); + if (!suppressStartCall) { + await server.start(); + } const app = new Koa(); server.applyMiddleware({ app }); httpServer = await new Promise(resolve => { diff --git a/packages/apollo-server-lambda/src/ApolloServer.ts b/packages/apollo-server-lambda/src/ApolloServer.ts index 69c26df9f5c..eba58a3f466 100644 --- a/packages/apollo-server-lambda/src/ApolloServer.ts +++ b/packages/apollo-server-lambda/src/ApolloServer.ts @@ -62,10 +62,10 @@ export class ApolloServer extends ApolloServerBase { } public createHandler({ cors, onHealthCheck }: CreateHandlerOptions = { cors: undefined, onHealthCheck: undefined }) { - // We will kick off the `willStart` event once for the server, and then - // await it before processing any requests by incorporating its `await` into - // the GraphQLServerOptions function which is called before each request. - const promiseWillStart = this.willStart(); + // In case the user didn't bother to call and await the `start` method, we + // kick it off in the background (with any errors getting logged + // and also rethrown from graphQLServerOptions during later requests). + this.ensureStarting(); const corsHeaders = new Headers(); @@ -274,14 +274,6 @@ export class ApolloServer extends ApolloServerBase { }; fileUploadHandler(() => graphqlLambda(async () => { - // In a world where this `createHandler` was async, we might avoid this - // but since we don't want to introduce a breaking change to this API - // (by switching it to `async`), we'll leverage the - // `GraphQLServerOptions`, which are dynamically built on each request, - // to `await` the `promiseWillStart` which we kicked off at the top of - // this method to ensure that it runs to completion (which is part of - // its contract) prior to processing the request. - await promiseWillStart; return this.createGraphQLServerOptions(event, context); })(event, context, callbackFilter)); }; diff --git a/packages/apollo-server-micro/src/ApolloServer.ts b/packages/apollo-server-micro/src/ApolloServer.ts index 6df712e063e..94b5d605309 100644 --- a/packages/apollo-server-micro/src/ApolloServer.ts +++ b/packages/apollo-server-micro/src/ApolloServer.ts @@ -33,15 +33,14 @@ export class ApolloServer extends ApolloServerBase { disableHealthCheck, onHealthCheck, }: ServerRegistration = {}) { - // We'll kick off the `willStart` right away, so hopefully it'll finish - // before the first request comes in. - const promiseWillStart = this.willStart(); + // In case the user didn't bother to call and await the `start` method, we + // kick it off in the background (with any errors getting logged + // and also rethrown from graphQLServerOptions during later requests). + this.ensureStarting(); return async (req, res) => { this.graphqlPath = path || '/graphql'; - await promiseWillStart; - if (typeof processFileUploads === 'function') { await this.handleFileUploads(req, res); } diff --git a/packages/apollo-server-plugin-operation-registry/src/__tests__/ApolloServerPluginOperationRegistry.test.ts b/packages/apollo-server-plugin-operation-registry/src/__tests__/ApolloServerPluginOperationRegistry.test.ts index c2c2451d730..3512f4a6be3 100644 --- a/packages/apollo-server-plugin-operation-registry/src/__tests__/ApolloServerPluginOperationRegistry.test.ts +++ b/packages/apollo-server-plugin-operation-registry/src/__tests__/ApolloServerPluginOperationRegistry.test.ts @@ -24,6 +24,7 @@ import { } from './helpers.test-helpers'; import { Headers } from 'apollo-server-env'; import { GraphQLRequest } from 'apollo-server-plugin-base'; +import { ApolloConfigInput } from 'apollo-server-types'; // While not ideal, today, Apollo Server has a very real expectation of an HTTP // request context. That will change in the future. While we can sometimes @@ -76,13 +77,6 @@ describe('Operation registry plugin', () => { ); const queryHash = operationSignature(normalizedQueryDocument); - // In order to expose will start and - class ApolloServerMock extends ApolloServerBase { - public async willStart() { - return super.willStart(); - } - } - describe('onUnregisterOperation', () => { it('is called when unregistered operation received', async () => { const onUnregisteredOperation: Options['onUnregisteredOperation'] = jest.fn(); @@ -92,7 +86,7 @@ describe('Operation registry plugin', () => { genericStorageSecret, [ /* Intentionally empty! */ ], ); - const server = new ApolloServerMock({ + const server = new ApolloServerBase({ typeDefs, mockEntireSchema: true, apollo, @@ -103,7 +97,7 @@ describe('Operation registry plugin', () => { })(), ], }); - await server.willStart(); + await server.start(); const result = await server.executeOperation({ ...mockHttpRequestContextForExecuteOperation, query: print(query), @@ -143,7 +137,7 @@ describe('Operation registry plugin', () => { }, ], ); - const server = new ApolloServerMock({ + const server = new ApolloServerBase({ typeDefs, mockEntireSchema: true, apollo, @@ -154,7 +148,7 @@ describe('Operation registry plugin', () => { })(), ], }); - await server.willStart(); + await server.start(); const result = await server.executeOperation({ ...mockHttpRequestContextForExecuteOperation, query: print(query), @@ -180,7 +174,7 @@ describe('Operation registry plugin', () => { genericStorageSecret, [ /* Intentionally empty! */ ], ); - const server = new ApolloServerMock({ + const server = new ApolloServerBase({ typeDefs, mockEntireSchema: true, apollo, @@ -192,7 +186,7 @@ describe('Operation registry plugin', () => { })(), ], }); - await server.willStart(); + await server.start(); const result = await server.executeOperation({ ...mockHttpRequestContextForExecuteOperation, query: print(query), @@ -234,7 +228,7 @@ describe('Operation registry plugin', () => { genericStorageSecret, [ /* Intentionally empty! */ ], ); - const server = new ApolloServerMock({ + const server = new ApolloServerBase({ typeDefs, mockEntireSchema: true, apollo, @@ -246,7 +240,7 @@ describe('Operation registry plugin', () => { })(), ], }); - await server.willStart(); + await server.start(); const result = await server.executeOperation({ ...mockHttpRequestContextForExecuteOperation, query: print(query), @@ -273,7 +267,7 @@ describe('Operation registry plugin', () => { }, ], ); - const server = new ApolloServerMock({ + const server = new ApolloServerBase({ typeDefs, mockEntireSchema: true, apollo, @@ -284,7 +278,7 @@ describe('Operation registry plugin', () => { })(), ], }); - await server.willStart(); + await server.start(); const result = await server.executeOperation({ ...mockHttpRequestContextForExecuteOperation, query: print(query), diff --git a/packages/apollo-server/src/index.ts b/packages/apollo-server/src/index.ts index cafe3e49df9..beedf5e3a20 100644 --- a/packages/apollo-server/src/index.ts +++ b/packages/apollo-server/src/index.ts @@ -97,6 +97,9 @@ export class ApolloServer extends ApolloServerBase { // Listen takes the same arguments as http.Server.listen. public async listen(...opts: Array): Promise { + // FIXME we should prevent you from calling start directly in this class + await this.start(); + // This class is the easy mode for people who don't create their own express // object, so we have to create it. const app = express();