From 43879ffe2f564c89366e578c4993c94b78b3b93e Mon Sep 17 00:00:00 2001 From: David Glasser Date: Fri, 26 Feb 2021 15:20:32 -0800 Subject: [PATCH] Add async server.start() function Previously, server startup worked like this: - `new ApolloServer` - 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 don't await) the protected `willStart` function, 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: - ApolloServer represents its state explicitly with a new ServerState - `new ApolloServer` - If no gateway, initialize all the schema-derived state directly like before (though the state now lives inside ServerState) - If 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. The overall change to user experience: - If you're using `apollo-server`, startup errors will cause `listen` to reject; no code changes are necessary. - If you're using an integration you are encouraged to call `await server.start()` yourself immediately after the constructor, which will let you detect startup errors. - But if you don't do that, the server will call `start` itself eventually. When you try to execute your first GraphQL request, `start` will happen if it hasn't already. Also an integration call like `server.applyMiddleware` will initiate a background `start`. If startup fails, the startup error will be logged on *every* failed graphql request, not just the first time like happened before. - If you have your own ApolloServer subclass that calls the protected `willStart` method, it won't work before that method is gone. Consider whether you can eliminate that call by just calling `start`, or perhaps call `ensureStarting` instead. This is close enough to backwards-compatible to be appropriate for a v2 minor release. We are likely to make `start()` required in Apollo Server 3 (other than for `apollo-server`). Also: - Previously we used the deprecated `ApolloServer.schema` field to determine whether to install ApolloServerPluginInlineTrace, which we want to have active by default for federated schemas only. If you're using a gateway, this field isn't actually set at the time that ensurePluginInstantiation reads it. That's basically OK because we don't want to turn on the plugin automatically in the gateway, but in the interest of avoiding use of the deprecated field, I refactored it so that `ApolloServerPluginInlineTrace` is installed by default (ie, if you don't install your own version or install `ApolloServerPluginInlineTraceDisabled`) without checking the schema, and then (if it's installed automatically) it decides whether or not to be active by checking the schema at `serverWillStart` time. - Similarly, schema reporting now throws in its `serverWillStart` if the schema is federated, instead of in `ensurePluginInstantiation`. (This does mean that if you're not using the new `start()` or `apollo-server`, that failure won't make your app fail as fast as if the `ApolloServer` constructor threw.) - Fix some fastify tests that used a fixed listen port to not do that. - I am doing my best to never accidentally run `prettier` on whole files and instead to very carefully select specific blocks of the file to format them several times per minute. Apparently I screwed up once and ran it once on `packages/apollo-server-core/src/ApolloServer.ts`. The ratio of "prettier changes" to "actual changes" in that file is low enough that I'd rather just leave the changes in this PR rather than spending time carefully reverting them. (It's one of the files I work on the most and being able to keep it prettier-clean will be helpful anyway.) - Replace a hacky workaround for the lack of `start` in the op reg tests! - Replace a use of a `Barrier` class I added recently in tests with the `@josephg/resolvable` npm package, which does basically the same thing. Use that package in new tests and in the core state machine itself. - While running tests I found that some test files hung if run separately due to lack of cleanup. I ended up refactoring the cache tests to: - make who is responsible for calling cache.close more consistent - make the Redis client mocks self-contained mocks of the ioredis API instead of starting with an actual ioredis implementation and mocking out some internals - clean up Jest fake timers when a certain test is done I'm not super certain exactly which of these changes fixed the hangs but it does seem better this way. (Specifically I think the fake timer fix, which I did last, is what actually fixed it, but the other changes made it easier for me to reason about what was going on.) Can factor out into another PR if helpful. Fixes #4921. Fixes apollographql/federation#335. TODO: - [ ] Go through all docs and READMEs that have 'FIXME start' and add calls to start. This involves verifying that you can actually do top-level await in the contexts that matter. (eg if it turns out that you really can't call await before you assign a handler in Lambda, that's interesting and may require some other changes to this PR!) - [ ] Actually document start() in the apollo-server reference - [ ] Document start() in all the integrations references - [ ] CHANGELOG - [ ] consider whether removing the protected willStart function is OK --- docs/source/api/apollo-server.md | 1 + docs/source/data/subscriptions.mdx | 2 +- docs/source/deployment/azure-functions.md | 4 +- docs/source/deployment/lambda.md | 4 + docs/source/deployment/netlify.md | 3 +- docs/source/integrations/middleware.md | 1 + docs/source/security/terminating-ssl.md | 3 +- package-lock.json | 6 + package.json | 1 + .../apollo-server-azure-functions/README.md | 3 + .../src/ApolloServer.ts | 6 - .../src/__tests__/Memcached.test.ts | 3 + .../src/__mocks__/ioredis.ts | 71 +- .../src/__tests__/RedisCache.test.ts | 5 +- .../src/__tests__/testsuite.ts | 7 +- .../apollo-server-cloud-functions/README.md | 4 + .../src/ApolloServer.ts | 13 - .../src/ApolloServer.ts | 6 +- packages/apollo-server-core/package.json | 1 + .../apollo-server-core/src/ApolloServer.ts | 763 +++++++++++------- .../src/__tests__/ApolloServerBase.test.ts | 21 +- .../src/plugin/inlineTrace/index.ts | 32 + .../src/plugin/schemaIsFederated.ts | 33 + .../src/plugin/schemaReporting/index.ts | 12 + packages/apollo-server-express/README.md | 2 + .../apollo-server-express/src/ApolloServer.ts | 23 +- .../src/__tests__/ApolloServer.test.ts | 7 +- packages/apollo-server-fastify/README.md | 1 + .../apollo-server-fastify/src/ApolloServer.ts | 8 +- .../src/__tests__/ApolloServer.test.ts | 5 +- .../src/__tests__/datasource.test.ts | 11 +- packages/apollo-server-hapi/README.md | 1 + .../apollo-server-hapi/src/ApolloServer.ts | 5 +- .../src/__tests__/ApolloServer.test.ts | 5 +- .../src/ApolloServer.ts | 63 +- .../src/index.ts | 56 +- packages/apollo-server-koa/README.md | 1 + .../apollo-server-koa/src/ApolloServer.ts | 24 +- .../src/__tests__/ApolloServer.test.ts | 5 +- packages/apollo-server-lambda/README.md | 6 +- .../apollo-server-lambda/src/ApolloServer.ts | 13 - packages/apollo-server-micro/README.md | 4 + .../apollo-server-micro/src/ApolloServer.ts | 9 +- ...polloServerPluginOperationRegistry.test.ts | 28 +- .../apollo-server/src/__tests__/index.test.ts | 30 +- packages/apollo-server/src/index.ts | 10 + 46 files changed, 816 insertions(+), 506 deletions(-) create mode 100644 packages/apollo-server-core/src/plugin/schemaIsFederated.ts diff --git a/docs/source/api/apollo-server.md b/docs/source/api/apollo-server.md index 5ff97ee9885..021df8bf52d 100644 --- a/docs/source/api/apollo-server.md +++ b/docs/source/api/apollo-server.md @@ -701,6 +701,7 @@ const server = new ApolloServer({ typeDefs, resolvers, }); +// FIXME add start here const app = express(); diff --git a/docs/source/data/subscriptions.mdx b/docs/source/data/subscriptions.mdx index c7d5878872a..479a0fd354f 100644 --- a/docs/source/data/subscriptions.mdx +++ b/docs/source/data/subscriptions.mdx @@ -422,7 +422,7 @@ const express = require('express'); const PORT = 4000; const app = express(); const server = new ApolloServer({ typeDefs, resolvers }); - +// FIXME start server.applyMiddleware({app}) const httpServer = http.createServer(app); diff --git a/docs/source/deployment/azure-functions.md b/docs/source/deployment/azure-functions.md index 2c54a1e5294..2a791105d46 100644 --- a/docs/source/deployment/azure-functions.md +++ b/docs/source/deployment/azure-functions.md @@ -64,7 +64,7 @@ npm init -y npm install apollo-server-azure-functions graphql ``` -Copy the code below and paste at you **index.js** file. +Copy the code below and paste in your **index.js** file. ```javascript const { ApolloServer, gql } = require('apollo-server-azure-functions'); @@ -84,7 +84,7 @@ const resolvers = { }; const server = new ApolloServer({ typeDefs, resolvers }); - +// FIXME start exports.graphqlHandler = server.createHandler(); ``` diff --git a/docs/source/deployment/lambda.md b/docs/source/deployment/lambda.md index 96f73fe376e..599028e9d41 100644 --- a/docs/source/deployment/lambda.md +++ b/docs/source/deployment/lambda.md @@ -52,6 +52,7 @@ const resolvers = { }; const server = new ApolloServer({ typeDefs, resolvers }); +// FIXME start exports.graphqlHandler = server.createHandler(); ``` @@ -164,6 +165,7 @@ const server = new ApolloServer({ context, }), }); +// FIXME start exports.graphqlHandler = server.createHandler(); ``` @@ -190,6 +192,7 @@ const resolvers = { }; const server = new ApolloServer({ typeDefs, resolvers }); +// FIXME start exports.graphqlHandler = server.createHandler({ cors: { @@ -219,6 +222,7 @@ const resolvers = { }; const server = new ApolloServer({ typeDefs, resolvers }); +// FIXME start exports.graphqlHandler = server.createHandler({ cors: { diff --git a/docs/source/deployment/netlify.md b/docs/source/deployment/netlify.md index acc30166085..6dd8257d8ae 100644 --- a/docs/source/deployment/netlify.md +++ b/docs/source/deployment/netlify.md @@ -84,13 +84,14 @@ const server = new ApolloServer({ typeDefs, resolvers }); +// FIXME start exports.handler = server.createHandler(); ``` Now, make sure you've run `NODE_ENV=development npm run start:lambda`, and navigate to `localhost:9000/graphql` in your browser. You should see GraphQL Playground, where you can run queries against your API! -*Note - The GraphQL Playground will only run if your `NODE_ENV` is set to `development`. If you don't pass this, or your `NODE_ENV` is set to `production`, you will not see the GraphQL Playground.* +*Note - The GraphQL Playground will only run if your `NODE_ENV` is set to `development`. If you don't pass this, or your `NODE_ENV` is set to `production`, you will not see the GraphQL Playground.* ![Local GraphQL Server](../images/graphql.png) diff --git a/docs/source/integrations/middleware.md b/docs/source/integrations/middleware.md index 474fc098db6..d6c64dc9606 100644 --- a/docs/source/integrations/middleware.md +++ b/docs/source/integrations/middleware.md @@ -41,6 +41,7 @@ const server = new ApolloServer({ typeDefs, resolvers, }); +// FIXME start server.applyMiddleware({ app }); diff --git a/docs/source/security/terminating-ssl.md b/docs/source/security/terminating-ssl.md index 67925171932..a0f275d83d9 100644 --- a/docs/source/security/terminating-ssl.md +++ b/docs/source/security/terminating-ssl.md @@ -30,6 +30,7 @@ const environment = process.env.NODE_ENV || 'production' const config = configurations[environment] const apollo = new ApolloServer({ typeDefs, resolvers }) +// FIXME start const app = express() apollo.applyMiddleware({ app }) @@ -37,7 +38,7 @@ apollo.applyMiddleware({ app }) // Create the HTTPS or HTTP server, per configuration var server if (config.ssl) { - // Assumes certificates are in a .ssl folder off of the package root. Make sure + // Assumes certificates are in a .ssl folder off of the package root. Make sure // these files are secured. server = https.createServer( { diff --git a/package-lock.json b/package-lock.json index ef02b7b1c0a..cedf5981f9f 100644 --- a/package-lock.json +++ b/package-lock.json @@ -3135,6 +3135,11 @@ } } }, + "@josephg/resolvable": { + "version": "1.0.0", + "resolved": "https://registry.npmjs.org/@josephg/resolvable/-/resolvable-1.0.0.tgz", + "integrity": "sha512-OfTtjoqB2doov5aTJxkyAMK8dXoo7CjCUQSYUEtiY34jbWduOGV7+168tmCT8COMsUEd5DMSFg/0iAOPCBTNAQ==" + }, "@koa/cors": { "version": "2.2.3", "resolved": "https://registry.npmjs.org/@koa/cors/-/cors-2.2.3.tgz", @@ -5587,6 +5592,7 @@ "@apollographql/apollo-tools": "^0.4.3", "@apollographql/graphql-playground-html": "1.6.27", "@apollographql/graphql-upload-8-fork": "^8.1.3", + "@josephg/resolvable": "^1.0.0", "@types/ws": "^7.0.0", "apollo-cache-control": "file:packages/apollo-cache-control", "apollo-datasource": "file:packages/apollo-datasource", diff --git a/package.json b/package.json index 565cc14db4b..6df41cb2123 100644 --- a/package.json +++ b/package.json @@ -57,6 +57,7 @@ }, "devDependencies": { "@apollographql/graphql-upload-8-fork": "8.1.3", + "@josephg/resolvable": "^1.0.0", "@types/async-retry": "1.4.2", "@types/aws-lambda": "8.10.66", "@types/body-parser": "1.19.0", diff --git a/packages/apollo-server-azure-functions/README.md b/packages/apollo-server-azure-functions/README.md index 47906b08920..bba996c400d 100644 --- a/packages/apollo-server-azure-functions/README.md +++ b/packages/apollo-server-azure-functions/README.md @@ -31,6 +31,7 @@ const resolvers = { }; const server = new ApolloServer({ typeDefs, resolvers }); +// FIXME start module.exports = server.createHandler(); ``` @@ -86,6 +87,7 @@ const server = new ApolloServer({ typeDefs, resolvers, }); +// FIXME start module.exports = server.createHandler({ cors: { @@ -118,6 +120,7 @@ const server = new ApolloServer({ typeDefs, resolvers, }); +// FIXME start module.exports = server.createHandler({ cors: { diff --git a/packages/apollo-server-azure-functions/src/ApolloServer.ts b/packages/apollo-server-azure-functions/src/ApolloServer.ts index 14d663fce7b..984737479d9 100644 --- a/packages/apollo-server-azure-functions/src/ApolloServer.ts +++ b/packages/apollo-server-azure-functions/src/ApolloServer.ts @@ -36,11 +36,6 @@ 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(); - const corsHeaders: HttpResponse['headers'] = {}; if (cors) { @@ -144,7 +139,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..81841f54601 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/README.md b/packages/apollo-server-cloud-functions/README.md index 7178b1112d4..8fa14d1f902 100644 --- a/packages/apollo-server-cloud-functions/README.md +++ b/packages/apollo-server-cloud-functions/README.md @@ -37,6 +37,7 @@ const server = new ApolloServer({ playground: true, introspection: true, }); +// FIXME start exports.handler = server.createHandler(); ``` @@ -82,6 +83,7 @@ const server = new ApolloServer({ res, }), }); +// FIXME start exports.handler = server.createHandler(); ``` @@ -111,6 +113,7 @@ const server = new ApolloServer({ typeDefs, resolvers, }); +// FIXME start exports.handler = server.createHandler({ cors: { @@ -143,6 +146,7 @@ const server = new ApolloServer({ typeDefs, resolvers, }); +// FIXME start exports.handler = server.createHandler({ cors: { diff --git a/packages/apollo-server-cloud-functions/src/ApolloServer.ts b/packages/apollo-server-cloud-functions/src/ApolloServer.ts index b5b665f77de..c42f4a61211 100644 --- a/packages/apollo-server-cloud-functions/src/ApolloServer.ts +++ b/packages/apollo-server-cloud-functions/src/ApolloServer.ts @@ -34,11 +34,6 @@ 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(); - const corsHeaders = {} as Record; if (cors) { @@ -129,14 +124,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/package.json b/packages/apollo-server-core/package.json index 6a883179fd8..3253ec3a554 100644 --- a/packages/apollo-server-core/package.json +++ b/packages/apollo-server-core/package.json @@ -28,6 +28,7 @@ "@apollographql/apollo-tools": "^0.4.3", "@apollographql/graphql-playground-html": "1.6.27", "@apollographql/graphql-upload-8-fork": "^8.1.3", + "@josephg/resolvable": "^1.0.0", "@types/ws": "^7.0.0", "apollo-cache-control": "file:../apollo-cache-control", "apollo-datasource": "file:../apollo-datasource", diff --git a/packages/apollo-server-core/src/ApolloServer.ts b/packages/apollo-server-core/src/ApolloServer.ts index e47801eddec..3fe1679f55e 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,10 +19,8 @@ import { ValidationContext, FieldDefinitionNode, DocumentNode, - isObjectType, - isScalarType, - isSchema, } from 'graphql'; +import resolvable, { Resolvable } from '@josephg/resolvable'; import { GraphQLExtension } from 'graphql-extensions'; import { InMemoryLRUCache, @@ -43,10 +41,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 +50,7 @@ import { SubscriptionServerOptions, FileUploadOptions, PluginDefinition, + GraphQLService, } from './types'; import { gql } from './index'; @@ -75,13 +71,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 { @@ -115,20 +111,47 @@ function approximateObjectSize(obj: T): number { } 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: Resolvable } + | { + phase: 'invoking serverWillStart'; + barrier: Resolvable; + schemaDerivedData: SchemaDerivedData; + } + | { phase: 'failed to start'; error: Error } + | { + phase: 'started'; + schemaDerivedData: SchemaDerivedData; + } + | { phase: 'stopping'; barrier: Resolvable } + | { phase: 'stopped'; stopError: Error | null }; + +// 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,14 +167,13 @@ 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 toDisposeLast = 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) { @@ -195,7 +217,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, @@ -247,10 +269,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), @@ -325,21 +345,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); @@ -389,6 +394,52 @@ export class ApolloServerBase { }); }); } + + if (gateway) { + // ApolloServer has been initialized but we have not yet tried to load the + // schema from the gateway. That will wait until the user calls + // `server.start()`, or until `ensureStarting` or `ensureStarted` are + // called. (In the case of a serverless framework integration, + // `ensureStarting` is automatically called at the end of the + // constructor.) + this.state = { phase: 'initialized with gateway', gateway }; + + // The main thing that the Gateway does is replace execution with + // its own executor. It would be awkward if you always had to pass + // `gateway: gateway, executor: gateway` to this constructor, so + // we let specifying `gateway` be a shorthand for the above. + // (We won't actually invoke the executor until after we're successfully + // called `gateway.load`.) + this.requestOptions.executor = gateway.executor; + } else { + // We construct the schema synchronously so that we can fail fast if + // the schema can't be constructed, and so that installSubscriptionHandlers + // and the deprecated `.schema` field (neither of which have ever worked + // with the gateway) are available immediately after the constructor returns. + this.state = { + phase: 'initialized with schema', + schemaDerivedData: this.generateSchemaDerivedData( + this.constructSchema(), + ), + }; + // This field is deprecated; users who are interested in learning + // their server's schema should instead make a plugin with serverWillStart, + // or register onSchemaChange on their gateway. It is only ever + // set for non-gateway servers. + this.schema = this.state.schemaDerivedData.schema; + } + + // The main entry point (createHandler) to serverless frameworks generally + // needs to be called synchronously from the top level of your entry point, + // unlike (eg) applyMiddleware, so we can't expect you to `await + // server.start()` before calling it. So we kick off the start + // asynchronously from the constructor, and failures are logged and cause + // later requests to fail (in graphQLServerOptions). There's no way to make + // "the whole server fail" separately from making individual requests fail, + // but that's not entirely unreasonable for a "serverless" model. + if (this.serverlessFramework()) { + this.ensureStarting(); + } } // used by integrations to synchronize the path with subscriptions, some @@ -397,9 +448,249 @@ export class ApolloServerBase { this.graphqlPath = path; } - private initSchema(): GraphQLSchema | Promise { + // Awaiting a call to `start` ensures that a schema has been loaded and that + // all plugin `serverWillStart` hooks have been called. If either of these + // processes throw, `start` will (async) throw as well. + // + // If you're using the batteries-included `apollo-server` package, you don't + // need to call `start` yourself (in fact, it will throw if you do so); its + // `listen` method takes care of that for you (this is why the actual logic is + // in the `_start` helper). + // + // If instead you're using an integration package, you are highly encouraged + // to await a call to `start` immediately after creating your `ApolloServer`, + // before attaching it to your web framework and starting to accept requests. + // `start` should only be called once; if it throws and you'd like to retry, + // just create another `ApolloServer`. (Note that this paragraph does not + // apply to "serverless framework" integrations like Lambda.) + // + // For backwards compatibility with the pre-2.22 API, you are not required to + // call start() yourself (this may change in AS3). Most integration packages + // call the protected `ensureStarting` when you first interact with them, + // which kicks off a "background" call to `start` if you haven't called it + // yourself. Then `graphQLServerOptions` (which is called before processing) + // each incoming GraphQL request) calls `ensureStarted` which waits for + // `start` to successfully complete (possibly by calling it itself), and + // throws a redacted error if `start` was not successful. If `start` is + // invoked implicitly by either of these mechanisms, any error that it throws + // will be logged when they occur and then again on every subsequent + // `graphQLServerOptions` call (ie, every GraphQL request). Note that start + // failures are not recoverable without creating a new ApolloServer. You are + // highly encouraged to make these backwards-compatibility paths into no-ops + // by awaiting a call to `start` yourself. + // + // Serverless integrations like Lambda (which override `serverlessFramework()` + // to return true) do not support calling `start()`, because their lifecycle + // doesn't allow you to wait before assigning a handler or allowing the + // handler to be called. So they call `ensureStarting` at the end of the + // constructor, and don't really differentiate between startup failures and + // request failures. This is hopefully appropriate for a "serverless" + // framework. As above, startup failures result in returning a redacted error + // to the end user and logging the more detailed error. + public async start(): Promise { + if (this.serverlessFramework()) { + throw new Error( + 'When using an ApolloServer subclass from a serverless framework ' + + "package, you don't need to call start(); just call createHandler().", + ); + } + + return await this._start(); + } + + protected 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 = resolvable(); + this.state = { phase: 'starting', barrier }; + try { + const schemaDerivedData = + initialState.phase === 'initialized with schema' + ? initialState.schemaDerivedData + : this.generateSchemaDerivedData( + await this.startGatewayAndLoadSchema(initialState.gateway), + ); + + 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.resolve(); + } + } + + // Part of the backwards-compatibility behavior described above `start` + // to make ApolloServer work if you don't explicitly call `start`. This is + // called at the beginning of each GraphQL request by `graphQLServerOptions`. + // It calls `start` for you if it hasn't been called yet, and only returns + // successfully if some call to `start` succeeds. + 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; + // 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); + } + } + } + + // Part of the backwards-compatibility behavior described above `start` to + // make ApolloServer work if you don't explicitly call `start`. This is called + // by some of the integration frameworks when you interact with them (eg by + // calling applyMiddleware). It is also called from the end of the constructor + // for serverless framework integrations. + // + // It calls `start` for you if it hasn't been called yet, but doesn't wait for + // `start` to finish. The goal is that if you don't call `start` yourself the + // server should still do the rest of startup vaguely near when your server + // starts, not just when the first GraphQL request comes in. Without this + // call, startup wouldn't occur until `graphQLServerOptions` invokes + // `ensureStarted`. + 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. + // Any thrown error will get logged, and also will cause + // every call to graphQLServerOptions (ie, every GraphQL operation) + // to log it again and prevent the operation from running. + this._start().catch((e) => this.logStartupError(e)); + } + } + + // Given an error that occurred during Apollo Server startup, log it with a + // helpful message. Note that this is only used if `ensureStarting` or + // `graphQLServerOptions` had to initiate the startup process; if you call + // `start` yourself (or you're using `apollo-server` whose `listen()` does + // it for you) then you can handle the error however you'd like rather than + // this log occurring. (We don't suggest the use of `start()` for serverless + // frameworks because they don't support it.) + private logStartupError(err: Error) { + const prelude = this.serverlessFramework() + ? 'An error occurred during Apollo Server startup.' + : 'Apollo Server was started implicitly and an error occurred during startup. ' + + '(Consider calling `await server.start()` immediately after ' + + '`server = new ApolloServer()` so you can handle these errors directly before ' + + 'starting your web server.)'; + this.logger.error( + prelude + + ' All GraphQL requests will now fail. The startup error ' + + 'was: ' + + ((err && err.message) || err), + ); + } + + 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; + + 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, @@ -407,119 +698,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')); + 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', - ); - } - - const augmentedTypeDefs = Array.isArray(typeDefs) ? typeDefs : [typeDefs]; + return schema!; + } - // 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 }); + if (!isDirectiveDefined(augmentedTypeDefs, 'cacheControl')) { + augmentedTypeDefs.push( + gql` + enum CacheControlScope { + PUBLIC + PRIVATE } - } else { - if (resolvers && !resolvers.Upload) { - resolvers.Upload = GraphQLUpload; - } - } - // 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 { @@ -556,76 +800,57 @@ 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. + public async stop() { + // Calling stop more than once should have the same result as the first time. + if (this.state.phase === 'stopped') { + if (this.state.stopError) { + throw this.state.stopError; + } 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, - }; + // Two parallel calls to stop; just wait for the other one to finish and + // do whatever it did. + if (this.state.phase === 'stopping') { + await this.state.barrier; + // The cast here is because TS doesn't understand that this.state can + // change during the await + // (https://github.com/microsoft/TypeScript/issues/9998). + const state = this.state as ServerState; + if (state.phase !== 'stopped') { + throw Error(`Surprising post-stopping state ${state.phase}`); + } + if (state.stopError) { + throw state.stopError; + } + return; } - 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() { - // We run shutdown handlers in two phases because we don't want to turn - // off our signal listeners until we've done the important parts of shutdown - // like running serverWillStop handlers. (We can make this more generic later - // if it's helpful.) - await Promise.all([...this.toDispose].map(dispose => dispose())); - if (this.subscriptionServer) this.subscriptionServer.close(); - await Promise.all([...this.toDisposeLast].map(dispose => dispose())); + // Commit to stopping, actually stop, and update the phase. + this.state = { phase: 'stopping', barrier: resolvable() }; + try { + // We run shutdown handlers in two phases because we don't want to turn + // off our signal listeners until we've done the important parts of shutdown + // like running serverWillStop handlers. (We can make this more generic later + // if it's helpful.) + await Promise.all([...this.toDispose].map((dispose) => dispose())); + if (this.subscriptionServer) this.subscriptionServer.close(); + await Promise.all([...this.toDisposeLast].map((dispose) => dispose())); + } catch (stopError) { + this.state = { phase: 'stopped', stopError }; + return; + } + this.state = { phase: 'stopped', stopError: null }; } - 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( @@ -650,12 +875,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( { @@ -699,14 +942,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, ); } @@ -722,38 +965,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[] = []; @@ -769,7 +980,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. @@ -801,11 +1012,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(); } @@ -857,23 +1066,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(' '), ); } @@ -929,17 +1128,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; } @@ -956,8 +1153,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, }); } @@ -968,12 +1164,24 @@ 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) { + // This means that the user didn't call and await `server.start()` themselves + // (and they're not using `apollo-server` which does that for you). First + // we log the error that prevented startup (which means it will get logged + // once for every GraphQL operation). + this.logStartupError(err); + // Now make the operation itself fail. + // We intentionally do not re-throw actual startup error as it may contain + // implementation details and this error will propagate to the client. + 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 : {}; @@ -1026,7 +1234,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..2ca056f5a4d 100644 --- a/packages/apollo-server-core/src/plugin/inlineTrace/index.ts +++ b/packages/apollo-server-core/src/plugin/inlineTrace/index.ts @@ -2,6 +2,7 @@ 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 { /** @@ -11,6 +12,17 @@ export interface ApolloServerPluginInlineTraceOptions { * of the error by modifying it and returning the modified error. */ rewriteError?: ApolloServerPluginUsageReportingOptions['rewriteError']; + /** + * This option is for internal use by `apollo-server-core` only. + * + * By default we want to enable this plugin for federated schemas only, but we + * need to come up with our list of plugins before we have necessarily loaded + * the schema. So (unless the user installs this plugin or + * ApolloServerPluginInlineTraceDisabled themselves), `apollo-server-core` + * always installs this plugin and uses this option to make sure traces are + * only included if the schema appears to be federated. + */ + __onlyIfSchemaIsFederated?: boolean; } // This ftv1 plugin produces a base64'd Trace protobuf containing only the @@ -21,11 +33,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/README.md b/packages/apollo-server-express/README.md index dc8646f21ac..93f6727be08 100644 --- a/packages/apollo-server-express/README.md +++ b/packages/apollo-server-express/README.md @@ -29,6 +29,7 @@ const resolvers = { }; const server = new ApolloServer({ typeDefs, resolvers }); +// FIXME start const app = express(); server.applyMiddleware({ app }); @@ -69,6 +70,7 @@ const resolvers = { }; const server = new ApolloServer({ typeDefs, resolvers }); +// FIXME start const app = connect(); const path = '/graphql'; 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/README.md b/packages/apollo-server-fastify/README.md index ce4079df1b9..2846c9dec55 100644 --- a/packages/apollo-server-fastify/README.md +++ b/packages/apollo-server-fastify/README.md @@ -18,6 +18,7 @@ const server = new ApolloServer({ typeDefs, resolvers, }); +// FIXME start const app = require('fastify')(); 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/README.md b/packages/apollo-server-hapi/README.md index 33f4347767b..1b172c19cfd 100644 --- a/packages/apollo-server-hapi/README.md +++ b/packages/apollo-server-hapi/README.md @@ -18,6 +18,7 @@ const Hapi = require('hapi'); async function StartServer() { const server = new ApolloServer({ typeDefs, resolvers }); + // FIXME start const app = new Hapi.server({ port: 4000 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..706af759511 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,8 +439,13 @@ export function testApolloServer( }) ); expect(consoleErrorSpy).toHaveBeenCalledWith( - "This data graph is missing a valid configuration. " + - "load error which should be masked"); + 'Apollo Server was started implicitly and an error occurred during startup. ' + + '(Consider calling `await server.start()` immediately after ' + + '`server = new ApolloServer()` so you can handle these errors directly before ' + + 'starting your web server.) All GraphQL requests will now fail. The startup error ' + + 'was: ' + + 'load error which should be masked', + ); expect(executor).not.toHaveBeenCalled(); }); @@ -2673,7 +2694,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 +3300,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..75e226eed6d 100644 --- a/packages/apollo-server-integration-testsuite/src/index.ts +++ b/packages/apollo-server-integration-testsuite/src/index.ts @@ -17,6 +17,7 @@ import { } from 'graphql'; import request from 'supertest'; +import resolvable from '@josephg/resolvable'; import { GraphQLOptions, @@ -1119,15 +1120,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 = resolvable(); + const letPluginFinishBarrier = resolvable(); // We want this to create the app as fast as `createApp` will allow. // for integrations whose `applyMiddleware` currently returns a @@ -1138,35 +1142,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.resolve(); + await letPluginFinishBarrier; + 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 +1171,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; + expect(calls).toEqual(['zero']); + letPluginFinishBarrier.resolve(); // 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/README.md b/packages/apollo-server-koa/README.md index ec416cf77c2..54929c74c6d 100644 --- a/packages/apollo-server-koa/README.md +++ b/packages/apollo-server-koa/README.md @@ -29,6 +29,7 @@ const resolvers = { }; const server = new ApolloServer({ typeDefs, resolvers }); +// FIXME start const app = new Koa(); server.applyMiddleware({ app }); 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/README.md b/packages/apollo-server-lambda/README.md index 85871317ead..ccae3a54719 100644 --- a/packages/apollo-server-lambda/README.md +++ b/packages/apollo-server-lambda/README.md @@ -36,7 +36,7 @@ const resolvers = { const server = new ApolloServer({ typeDefs, resolvers, - + // By default, the GraphQL Playground interface and GraphQL introspection // is disabled in "production" (i.e. when `process.env.NODE_ENV` is `production`). // @@ -45,6 +45,7 @@ const server = new ApolloServer({ playground: true, introspection: true, }); +// FIXME start exports.handler = server.createHandler(); ``` @@ -135,6 +136,7 @@ const server = new ApolloServer({ context, }), }); +// FIXME start exports.handler = server.createHandler(); ``` @@ -164,6 +166,7 @@ const server = new ApolloServer({ typeDefs, resolvers, }); +// FIXME start exports.handler = server.createHandler({ cors: { @@ -196,6 +199,7 @@ const server = new ApolloServer({ typeDefs, resolvers, }); +// FIXME start exports.handler = server.createHandler({ cors: { diff --git a/packages/apollo-server-lambda/src/ApolloServer.ts b/packages/apollo-server-lambda/src/ApolloServer.ts index 1798da88b9b..d027eff2322 100644 --- a/packages/apollo-server-lambda/src/ApolloServer.ts +++ b/packages/apollo-server-lambda/src/ApolloServer.ts @@ -108,11 +108,6 @@ export class ApolloServer extends ApolloServerBase { 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(); - const corsHeaders = new Headers(); if (cors) { @@ -340,14 +335,6 @@ export class ApolloServer extends ApolloServerBase { { method: event.httpMethod, options: 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); }, query, diff --git a/packages/apollo-server-micro/README.md b/packages/apollo-server-micro/README.md index a4197b85db3..6acb2c7f25d 100644 --- a/packages/apollo-server-micro/README.md +++ b/packages/apollo-server-micro/README.md @@ -35,6 +35,7 @@ const resolvers = { }; const apolloServer = new ApolloServer({ typeDefs, resolvers }); +// FIXME start module.exports = apolloServer.createHandler(); ``` @@ -86,6 +87,7 @@ const resolvers = { }; const apolloServer = new ApolloServer({ typeDefs, resolvers }); +// FIXME start const handler = apolloServer.createHandler(); // highlight-line module.exports = cors((req, res) => req.method === 'OPTIONS' ? res.end() : handler(req, res)) // highlight-line ``` @@ -137,6 +139,7 @@ const resolvers = { }; const apolloServer = new ApolloServer({ typeDefs, resolvers }); +// FIXME start module.exports = apolloServer.createHandler({ path: '/data' }); // highlight-line ``` @@ -189,6 +192,7 @@ const resolvers = { }; const apolloServer = new ApolloServer({ typeDefs, resolvers }); +// FIXME start const graphqlPath = '/data'; const graphqlHandler = apolloServer.createHandler({ path: graphqlPath }); module.exports = router( 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/__tests__/index.test.ts b/packages/apollo-server/src/__tests__/index.test.ts index 94e28648936..8f4d9830c1f 100644 --- a/packages/apollo-server/src/__tests__/index.test.ts +++ b/packages/apollo-server/src/__tests__/index.test.ts @@ -1,6 +1,7 @@ import { createConnection } from 'net'; import request from 'request'; import { createApolloFetch } from 'apollo-fetch'; +import resolvable from '@josephg/resolvable'; import { gql, ApolloServer } from '../index'; @@ -17,19 +18,6 @@ const resolvers = { }, }; -class Barrier { - private resolvePromise!: () => void; - private promise = new Promise((r) => { - this.resolvePromise = r; - }); - async wait() { - await this.promise; - } - unblock() { - this.resolvePromise(); - } -} - describe('apollo-server', () => { describe('constructor', () => { it('accepts typeDefs and resolvers', () => { @@ -82,11 +70,11 @@ describe('apollo-server', () => { // Open a TCP connection to the server, and let it dangle idle // without starting a request. - const connectionBarrier = new Barrier(); + const connectionBarrier = resolvable(); createConnection({ host: 'localhost', port: port as number }, () => - connectionBarrier.unblock(), + connectionBarrier.resolve(), ); - await connectionBarrier.wait(); + await connectionBarrier; // Stop the server. Before, when this was just net.Server.close, this // would hang. Now that we use stoppable, the idle connection is immediately @@ -95,8 +83,8 @@ describe('apollo-server', () => { }); it('a connection with an active HTTP request', async () => { - const gotToHangBarrier = new Barrier(); - const hangBarrier = new Barrier(); + const gotToHangBarrier = resolvable(); + const hangBarrier = resolvable(); const server = new ApolloServer({ typeDefs, resolvers: { @@ -104,8 +92,8 @@ describe('apollo-server', () => { Query: { ...resolvers.Query, async hang() { - gotToHangBarrier.unblock(); - await hangBarrier.wait(); // never unblocks + gotToHangBarrier.resolve(); + await hangBarrier; // never unblocks }, }, }, @@ -119,7 +107,7 @@ describe('apollo-server', () => { // expected error that happens after the server is stopped.) const apolloFetch = createApolloFetch({ uri: url }); apolloFetch({ query: '{hang}' }).catch(() => {}); - await gotToHangBarrier.wait(); + await gotToHangBarrier; // Stop the server. Before, when this was just net.Server.close, this // would hang. Now that we use stoppable, the idle connection is immediately diff --git a/packages/apollo-server/src/index.ts b/packages/apollo-server/src/index.ts index cafe3e49df9..ebe40db0b0e 100644 --- a/packages/apollo-server/src/index.ts +++ b/packages/apollo-server/src/index.ts @@ -95,8 +95,18 @@ export class ApolloServer extends ApolloServerBase { ); } + public async start(): Promise { + throw new Error( + "When using the `apollo-server` package, you don't need to call start(); just call listen().", + ); + } + // Listen takes the same arguments as http.Server.listen. public async listen(...opts: Array): Promise { + // First start the server and throw if startup fails (eg, schema can't be loaded + // or a serverWillStart plugin throws). + 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();