From 3c3620ba78b7c4f5d4c52cefcc90a4eb81c9ec8c Mon Sep 17 00:00:00 2001 From: Evans Hauser Date: Thu, 6 Jun 2019 16:55:53 -0700 Subject: [PATCH 01/36] op-reg: add hooks for plugin lifecycle This adds hooks into the lifecycle of the operation registry * onManifestUpdate(newManifest, oldManifest) * onUnregisteredOperation(operation) * onForbiddenOperation(operation) --- .../src/ApolloServerPluginOperationRegistry.ts | 16 +++++++++++++++- .../src/agent.ts | 9 +++++++-- 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts b/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts index 413e35746b3..62f627bd124 100644 --- a/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts +++ b/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts @@ -11,7 +11,7 @@ import { defaultOperationRegistrySignature, } from 'apollo-graphql'; import { ForbiddenError, ApolloError } from 'apollo-server-errors'; -import Agent from './agent'; +import Agent, { OperationManifest } from './agent'; import { GraphQLSchema } from 'graphql/type'; import { InMemoryLRUCache } from 'apollo-server-caching'; import loglevel from 'loglevel'; @@ -28,6 +28,12 @@ interface Options { | ForbidUnregisteredOperationsPredicate; dryRun?: boolean; schemaTag?: string; + onUnregisteredOperation?: (requestContext: GraphQLRequestContext) => void; + onForbiddenOperation?: (requestContext: GraphQLRequestContext) => void; + onManifestUpdate?: ( + newManifest: OperationManifest, + oldManifest: OperationManifest, + ) => void; } export default function plugin(options: Options = Object.create(null)) { @@ -99,6 +105,7 @@ for observability purposes, but all operations will be permitted.`, engine, store, logger, + onManifestUpdate: options.onManifestUpdate, }); await agent.start(); @@ -154,6 +161,10 @@ for observability purposes, but all operations will be permitted.`, ); requestContext.metrics.registeredOperation = true; return; + } else { + if (options.onUnregisteredOperation) { + options.onUnregisteredOperation(requestContext); + } } // If the `forbidUnregisteredOperations` option is set explicitly to @@ -218,6 +229,9 @@ for observability purposes, but all operations will be permitted.`, } if (shouldForbidOperation) { + if (options.onForbiddenOperation) { + options.onForbiddenOperation(requestContext); + } if (!options.dryRun) { logger.debug( `${logHash}: Execution denied because 'forbidUnregisteredOperations' was enabled for this request and the operation was not found in the local operation registry.`, diff --git a/packages/apollo-server-plugin-operation-registry/src/agent.ts b/packages/apollo-server-plugin-operation-registry/src/agent.ts index b27c526149e..861a14391be 100644 --- a/packages/apollo-server-plugin-operation-registry/src/agent.ts +++ b/packages/apollo-server-plugin-operation-registry/src/agent.ts @@ -11,6 +11,7 @@ import loglevel from 'loglevel'; import { Response } from 'node-fetch'; import { InMemoryLRUCache } from 'apollo-server-caching'; +import { GraphQLRequestContext } from 'apollo-server-plugin-base'; import { fetchIfNoneMatch } from './fetchIfNoneMatch'; const DEFAULT_POLL_SECONDS: number = 30; @@ -23,14 +24,18 @@ export interface AgentOptions { engine: any; store: InMemoryLRUCache; schemaTag: string; + onManifestUpdate?: ( + newManifest: OperationManifest, + oldManifest: OperationManifest, + ) => void; } -interface Operation { +export interface Operation { signature: string; document: string; } -interface OperationManifest { +export interface OperationManifest { version: number; operations: Array; } From b31efd3ca99f204e033301880a59466bbed367d9 Mon Sep 17 00:00:00 2001 From: Evans Hauser Date: Tue, 25 Jun 2019 15:04:54 -0700 Subject: [PATCH 02/36] call willManifestUpdate once on failure and updates --- .../ApolloServerPluginOperationRegistry.ts | 11 +++-- .../src/agent.ts | 47 ++++++++++++++----- 2 files changed, 42 insertions(+), 16 deletions(-) diff --git a/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts b/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts index 62f627bd124..30417a5af05 100644 --- a/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts +++ b/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts @@ -16,6 +16,7 @@ import { GraphQLSchema } from 'graphql/type'; import { InMemoryLRUCache } from 'apollo-server-caching'; import loglevel from 'loglevel'; import loglevelDebug from 'loglevel-debug'; +import { PromiseOrValue } from 'graphql/jsutils/PromiseOrValue'; type ForbidUnregisteredOperationsPredicate = ( requestContext: GraphQLRequestContext, @@ -30,10 +31,10 @@ interface Options { schemaTag?: string; onUnregisteredOperation?: (requestContext: GraphQLRequestContext) => void; onForbiddenOperation?: (requestContext: GraphQLRequestContext) => void; - onManifestUpdate?: ( - newManifest: OperationManifest, - oldManifest: OperationManifest, - ) => void; + willUpdateManifest?: ( + newManifest?: OperationManifest, + oldManifest?: OperationManifest, + ) => PromiseOrValue; } export default function plugin(options: Options = Object.create(null)) { @@ -105,7 +106,7 @@ for observability purposes, but all operations will be permitted.`, engine, store, logger, - onManifestUpdate: options.onManifestUpdate, + willUpdateManifest: options.willUpdateManifest, }); await agent.start(); diff --git a/packages/apollo-server-plugin-operation-registry/src/agent.ts b/packages/apollo-server-plugin-operation-registry/src/agent.ts index 861a14391be..4c9e3a880dc 100644 --- a/packages/apollo-server-plugin-operation-registry/src/agent.ts +++ b/packages/apollo-server-plugin-operation-registry/src/agent.ts @@ -11,8 +11,8 @@ import loglevel from 'loglevel'; import { Response } from 'node-fetch'; import { InMemoryLRUCache } from 'apollo-server-caching'; -import { GraphQLRequestContext } from 'apollo-server-plugin-base'; import { fetchIfNoneMatch } from './fetchIfNoneMatch'; +import { PromiseOrValue } from 'graphql/jsutils/PromiseOrValue'; const DEFAULT_POLL_SECONDS: number = 30; const SYNC_WARN_TIME_SECONDS: number = 60; @@ -24,10 +24,10 @@ export interface AgentOptions { engine: any; store: InMemoryLRUCache; schemaTag: string; - onManifestUpdate?: ( - newManifest: OperationManifest, - oldManifest: OperationManifest, - ) => void; + willUpdateManifest?: ( + newManifest?: OperationManifest, + oldManifest?: OperationManifest, + ) => PromiseOrValue; } export interface Operation { @@ -56,6 +56,7 @@ export default class Agent { public _timesChecked: number = 0; private lastOperationSignatures: SignatureStore = new Set(); + private lastManifest?: OperationManifest; private readonly options: AgentOptions = Object.create(null); constructor(options: AgentOptions) { @@ -106,8 +107,14 @@ export default class Agent { try { await pulse(); } catch (err) { + // Update the manifest to trigger `onManifestUpdate(newManifest: undefined, oldManifest: undefined)` + this.updateManifest(); console.error( - 'The operation manifest could not be fetched. Retries will continue, but requests will be forbidden until the manifest is fetched.', + `The operation manifest could not be fetched. Retries will continue.${ + this.lastManifest + ? '' + : ' Requests will be forbidden until the manifest is fetched.' + }`, err.message || err, ); } @@ -310,19 +317,37 @@ export default class Agent { }); } - public async updateManifest(manifest: OperationManifest) { + public async updateManifest(manifest?: OperationManifest) { if ( - !manifest || - manifest.version !== 2 || - !Array.isArray(manifest.operations) + manifest && + (manifest.version !== 2 || !Array.isArray(manifest.operations)) ) { throw new Error('Invalid manifest format.'); } + const returnedManifest = + (await (this.options.willUpdateManifest && + this.options.willUpdateManifest(manifest, this.lastManifest))) || + manifest; + + if (returnedManifest && !Array.isArray(returnedManifest.operations)) { + throw new Error( + "Invalid manifest format. Manifest's operations must be an array", + ); + } + + if (returnedManifest) { + this.updateOperationStore(returnedManifest.operations); + } + + this.lastManifest = manifest; + } + + private updateOperationStore(operations: OperationManifest['operations']) { const incomingOperations: Map = new Map(); const replacementSignatures: SignatureStore = new Set(); - for (const { signature, document } of manifest.operations) { + for (const { signature, document } of operations) { incomingOperations.set(signature, document); // Keep track of each operation in this manifest so we can store it // for comparison after the next fetch. From 51306db31a9889e52b8ab83ce6d275c27e7333b9 Mon Sep 17 00:00:00 2001 From: Evans Hauser Date: Tue, 25 Jun 2019 15:09:24 -0700 Subject: [PATCH 03/36] layout test structure --- .../src/__tests__/agent.test.ts | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/packages/apollo-server-plugin-operation-registry/src/__tests__/agent.test.ts b/packages/apollo-server-plugin-operation-registry/src/__tests__/agent.test.ts index d1af491a8d1..f51757bf8a9 100644 --- a/packages/apollo-server-plugin-operation-registry/src/__tests__/agent.test.ts +++ b/packages/apollo-server-plugin-operation-registry/src/__tests__/agent.test.ts @@ -539,6 +539,28 @@ describe('Agent', () => { }); }); }); + + describe('manifest lifecycle', () => { + describe('willUpdateManifest', () => { + describe("called with undefined arguments when can't fetch first time", () => {}); + describe('called with manifest retrieved from gcs', () => {}); + describe('uses returned manifest', () => {}); + describe('not called again when manifest remains same', () => {}); + describe('called with previous manifest when updated', () => {}); + }); + }); + }); + + describe('operation lifecycle hooks', () => { + describe('onUnregisterOperation', () => { + describe('called when unregistered operation received', () => {}); + describe('not called when registered operation received', () => {}); + }); + describe('onForbiddenOperation', () => { + describe('called when unregistered operation received and forbidden', () => {}); + describe('not called when unregistered operation received and unforbidden', () => {}); + describe('not called when registered operation received', () => {}); + }); }); }); From 7dd15040eefc6b24cb4355a39a00205b0f0b3593 Mon Sep 17 00:00:00 2001 From: Evans Hauser Date: Tue, 2 Jul 2019 09:35:13 -0700 Subject: [PATCH 04/36] add changelog --- packages/apollo-server-plugin-operation-registry/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/apollo-server-plugin-operation-registry/CHANGELOG.md b/packages/apollo-server-plugin-operation-registry/CHANGELOG.md index 33603713d7a..f738f4d0998 100644 --- a/packages/apollo-server-plugin-operation-registry/CHANGELOG.md +++ b/packages/apollo-server-plugin-operation-registry/CHANGELOG.md @@ -2,6 +2,7 @@ ### vNEXT (Currently `alpha` tag) +- Add lifecycle hooks: `willUpdateManifest`, `onUnregisteredOperation`, and `onForbiddenOperation`. [PR #158](https://github.com/apollographql/apollo-platform-commercial/pull/158) - Prevent the polling timer from keeping the event loop active [PR #223](https://github.com/apollographql/apollo-platform-commercial/pull/223) - Update error message for operations that are not in the operation registry [PR #170](https://github.com/apollographql/apollo-platform-commercial/pull/170) From b154b5606418e2c459dacaf4ae44c51c45092704 Mon Sep 17 00:00:00 2001 From: Evans Hauser Date: Tue, 2 Jul 2019 17:53:02 -0700 Subject: [PATCH 05/36] add willUpdateManfest tests --- .../src/__tests__/agent.test.ts | 425 +++++++++++++----- 1 file changed, 325 insertions(+), 100 deletions(-) diff --git a/packages/apollo-server-plugin-operation-registry/src/__tests__/agent.test.ts b/packages/apollo-server-plugin-operation-registry/src/__tests__/agent.test.ts index f51757bf8a9..b6534655905 100644 --- a/packages/apollo-server-plugin-operation-registry/src/__tests__/agent.test.ts +++ b/packages/apollo-server-plugin-operation-registry/src/__tests__/agent.test.ts @@ -7,7 +7,7 @@ import { } from '../common'; import { resolve as urlResolve } from 'url'; import { createHash } from 'crypto'; -import { AgentOptions } from '../agent'; +import { AgentOptions, OperationManifest } from '../agent'; const fakeBaseUrl = 'https://myfakehost/'; @@ -84,7 +84,7 @@ describe('Agent', () => { }); }); - describe('fetches', () => { + describe('with manifest', () => { let originalEnvApolloOpManifestBaseUrl: string | undefined; let originalEnvOverrideStorageSecretBaseUrl: string | undefined; let Agent: typeof import('../agent').default; @@ -123,6 +123,51 @@ describe('Agent', () => { ).replace(new RegExp(`^${urlOperationManifestBase}`), ''); }); + // Each nock is good for exactly one request! + function nockLegacyGoodManifest( + operations: ManifestRecord[] = [ + sampleManifestRecords.a, + sampleManifestRecords.b, + sampleManifestRecords.c, + ], + ) { + return nockBase() + .get(genericLegacyOperationManifestUrl) + .reply(200, { + version: 2, + operations, + }); + } + + function nockGoodManifestsUnderStorageSecret( + operations: ManifestRecord[] = [ + sampleManifestRecords.a, + sampleManifestRecords.b, + sampleManifestRecords.c, + ], + ) { + return nockBase() + .get(genericStorageSecretOperationManifestUrl) + .reply(200, { + version: 2, + operations, + }); + } + + function nockStorageSecret( + status = 200, + body: any = JSON.stringify(genericStorageSecret), + ) { + return nockBase() + .get( + getStorageSecretUrl(genericServiceID, genericApiKeyHash).replace( + new RegExp(`^${urlStorageSecretBase}`), + '', + ), + ) + .reply(status, body); + } + afterAll(() => { // Put the environment overrides back how they were. if (originalEnvApolloOpManifestBaseUrl) { @@ -148,6 +193,42 @@ describe('Agent', () => { jest.resetModules(); }); + const forCleanup: { + store?: InMemoryLRUCache; + agent?: import('../agent').default; + }[] = []; + + function createAgent({ ...args }: Partial = {}) { + const options = { ...defaultAgentOptions, ...args }; + + // We never actually let the Agent construct its own default store + // since we need to pluck the store out to instrument it with spies. + const store = options.store; + const agent = new Agent(options); + + // Save all agents and stores we've created so we can properly + // stop them and clean them up. + forCleanup.push({ agent, store }); + return agent; + } + + afterEach(() => { + if (!nock.isDone()) { + throw new Error( + `Not all nock interceptors were used! Pending mocks: ${nock.pendingMocks()}`, + ); + } + + let toCleanup; + // Loop through the `forCleanup` constant and empty it out by popping + // individual elements off the end and running the appropriate cleanup. + while ((toCleanup = forCleanup.pop())) { + if (toCleanup.agent) { + toCleanup.agent.stop(); + } + } + }); + it('correctly prepared the test environment', () => { expect(getLegacyOperationManifestUrl('abc123', 'def456')).toStrictEqual( urlResolve(fakeBaseUrl, '/abc123/def456.v2.json'), @@ -155,87 +236,6 @@ describe('Agent', () => { }); describe('manifest checking and store populating', () => { - const forCleanup: { - store?: InMemoryLRUCache; - agent?: import('../agent').default; - }[] = []; - - function createAgent({ ...args } = {}) { - const options = { ...defaultAgentOptions, ...args }; - - // We never actually let the Agent construct its own default store - // since we need to pluck the store out to instrument it with spies. - const store = options.store; - const agent = new Agent(options); - - // Save all agents and stores we've created so we can properly - // stop them and clean them up. - forCleanup.push({ agent, store }); - return agent; - } - - afterEach(() => { - if (!nock.isDone()) { - throw new Error( - `Not all nock interceptors were used! Pending mocks: ${nock.pendingMocks()}`, - ); - } - - let toCleanup; - // Loop through the `forCleanup` constant and empty it out by popping - // individual elements off the end and running the appropriate cleanup. - while ((toCleanup = forCleanup.pop())) { - if (toCleanup.agent) { - toCleanup.agent.stop(); - } - } - }); - - // Each nock is good for exactly one request! - function nockLegacyGoodManifest( - operations: ManifestRecord[] = [ - sampleManifestRecords.a, - sampleManifestRecords.b, - sampleManifestRecords.c, - ], - ) { - return nockBase() - .get(genericLegacyOperationManifestUrl) - .reply(200, { - version: 2, - operations, - }); - } - - function nockGoodManifestsUnderStorageSecret( - operations: ManifestRecord[] = [ - sampleManifestRecords.a, - sampleManifestRecords.b, - sampleManifestRecords.c, - ], - ) { - return nockBase() - .get(genericStorageSecretOperationManifestUrl) - .reply(200, { - version: 2, - operations, - }); - } - - function nockStorageSecret( - status = 200, - body: any = JSON.stringify(genericStorageSecret), - ) { - return nockBase() - .get( - getStorageSecretUrl(genericServiceID, genericApiKeyHash).replace( - new RegExp(`^${urlStorageSecretBase}`), - '', - ), - ) - .reply(status, body); - } - function expectStoreSpyOperationEach( spy: jest.SpyInstance, letters: string[], @@ -541,25 +541,250 @@ describe('Agent', () => { }); describe('manifest lifecycle', () => { + function generateWillUpdateManifest( + operations?: OperationManifest['operations'], + ) { + return jest.fn( + ( + newManifest?: OperationManifest, + oldManifest?: OperationManifest, + ) => { + const manifest = newManifest || oldManifest; + return ( + (operations && { version: 2, operations }) || + manifest || { version: 2, operations: [] } + ); + }, + ); + } + describe('willUpdateManifest', () => { - describe("called with undefined arguments when can't fetch first time", () => {}); - describe('called with manifest retrieved from gcs', () => {}); - describe('uses returned manifest', () => {}); - describe('not called again when manifest remains same', () => {}); - describe('called with previous manifest when updated', () => {}); - }); - }); - }); + it("receives undefined arguments when can't fetch first time", async () => { + nockStorageSecret(404); + nockBase() + .get(genericLegacyOperationManifestUrl) + .reply(500); + const mock = generateWillUpdateManifest(); + const store = defaultStore(); + const storeSetSpy = jest.spyOn(store, 'set'); + const storeDeleteSpy = jest.spyOn(store, 'delete'); - describe('operation lifecycle hooks', () => { - describe('onUnregisterOperation', () => { - describe('called when unregistered operation received', () => {}); - describe('not called when registered operation received', () => {}); - }); - describe('onForbiddenOperation', () => { - describe('called when unregistered operation received and forbidden', () => {}); - describe('not called when unregistered operation received and unforbidden', () => {}); - describe('not called when registered operation received', () => {}); + const agent = createAgent({ + willUpdateManifest: mock, + store, + }); + await agent.start(); + expect(mock).toHaveBeenCalledTimes(1); + expect(mock).toHaveBeenCalledWith(undefined, undefined); + + // no additions, no deletions. + expect(storeSetSpy).toBeCalledTimes(0); + expect(storeDeleteSpy).toBeCalledTimes(0); + }); + it('receives manifest retrieved from gcs', async () => { + nockStorageSecret(); + nockGoodManifestsUnderStorageSecret(); + const mock = generateWillUpdateManifest(); + const store = defaultStore(); + const storeSetSpy = jest.spyOn(store, 'set'); + const storeDeleteSpy = jest.spyOn(store, 'delete'); + + const agent = createAgent({ + willUpdateManifest: mock, + store, + }); + await agent.start(); + expect(mock).toHaveBeenCalledTimes(1); + expect(mock).toHaveBeenCalledWith( + { + version: 2, + operations: [ + sampleManifestRecords.a, + sampleManifestRecords.b, + sampleManifestRecords.c, + ], + }, + undefined, + ); + + // Three additions, no deletions. + expect(storeSetSpy).toBeCalledTimes(3); + expect(storeDeleteSpy).toBeCalledTimes(0); + }); + it('uses returned manifest to enforce safelist', async () => { + nockStorageSecret(); + nockGoodManifestsUnderStorageSecret(); + const mock = generateWillUpdateManifest([sampleManifestRecords.a]); + const store = defaultStore(); + const storeSetSpy = jest.spyOn(store, 'set'); + const storeDeleteSpy = jest.spyOn(store, 'delete'); + + const agent = createAgent({ + willUpdateManifest: mock, + store, + }); + await agent.start(); + expect(mock).toHaveBeenCalledTimes(1); + expect(mock).toHaveBeenCalledWith( + { + version: 2, + operations: [ + sampleManifestRecords.a, + sampleManifestRecords.b, + sampleManifestRecords.c, + ], + }, + undefined, + ); + expect(mock).toReturnWith({ + version: 2, + operations: [sampleManifestRecords.a], + }); + + // One additions, no deletions. + expect(storeSetSpy).toBeCalledTimes(1); + expect(storeDeleteSpy).toBeCalledTimes(0); + }); + it('is not called again when manifest remains same', async () => { + nockStorageSecret(); + nockGoodManifestsUnderStorageSecret(); + + const mock = generateWillUpdateManifest(); + const store = defaultStore(); + const storeSetSpy = jest.spyOn(store, 'set'); + const storeDeleteSpy = jest.spyOn(store, 'delete'); + + const agent = createAgent({ + willUpdateManifest: mock, + store, + }); + jest.useFakeTimers(); + await agent.start(); + expect(mock).toHaveBeenCalledTimes(1); + expect(mock).toHaveBeenCalledWith( + { + version: 2, + operations: [ + sampleManifestRecords.a, + sampleManifestRecords.b, + sampleManifestRecords.c, + ], + }, + undefined, + ); + // Three additions, no deletions. + expect(storeSetSpy).toBeCalledTimes(3); + expect(storeDeleteSpy).toBeCalledTimes(0); + + // If it's one millisecond short of our next poll interval, nothing + // should have changed yet. + jest.advanceTimersByTime(pollSeconds * 1000 - 1); + + // Still only one check. + expect(agent._timesChecked).toBe(1); + + // Now, we'll expect another request to go out, so we'll nock it. + nockStorageSecret(); + nockBase() + .get(genericStorageSecretOperationManifestUrl) + .reply(304); + + // If we move forward the last remaining millisecond, we should trigger + // and end up with a successful sync. + jest.advanceTimersByTime(1); + + // While that timer will fire, it will do work async, and we need to + // wait on that work itself. + await agent.requestPending(); + + // Now the times checked should have gone up. + expect(agent._timesChecked).toBe(2); + + // the agent should not have been called again + expect(mock).toHaveBeenCalledTimes(1); + expect(mock).toHaveBeenCalledWith( + { + version: 2, + operations: [ + sampleManifestRecords.a, + sampleManifestRecords.b, + sampleManifestRecords.c, + ], + }, + undefined, + ); + // Three additions, no deletions. + expect(storeSetSpy).toBeCalledTimes(3); + expect(storeDeleteSpy).toBeCalledTimes(0); + }); + + it('receives previous manifest when updated', async () => { + nockStorageSecret(); + nockGoodManifestsUnderStorageSecret(); + + const mock = generateWillUpdateManifest(); + const store = defaultStore(); + const storeSetSpy = jest.spyOn(store, 'set'); + const storeDeleteSpy = jest.spyOn(store, 'delete'); + + const agent = createAgent({ + willUpdateManifest: mock, + store, + }); + jest.useFakeTimers(); + await agent.start(); + expect(mock).toHaveBeenCalledTimes(1); + expect(mock).toHaveBeenCalledWith( + { + version: 2, + operations: [ + sampleManifestRecords.a, + sampleManifestRecords.b, + sampleManifestRecords.c, + ], + }, + undefined, + ); + // Three additions, no deletions. + expect(storeSetSpy).toBeCalledTimes(3); + expect(storeDeleteSpy).toBeCalledTimes(0); + + // If it's one millisecond short of our next poll interval, nothing + // should have changed yet. + jest.advanceTimersByTime(pollSeconds * 1000 - 1); + + // Still only one check. + expect(agent._timesChecked).toBe(1); + + // Now, we'll expect another request to go out, so we'll nock it. + nockStorageSecret(); + nockGoodManifestsUnderStorageSecret([sampleManifestRecords.a]); + + // If we move forward the last remaining millisecond, we should trigger + // and end up with a successful sync. + jest.advanceTimersByTime(1); + + // While that timer will fire, it will do work async, and we need to + // wait on that work itself. + await agent.requestPending(); + + expect(mock).toHaveBeenCalledTimes(2); + expect(mock).toHaveBeenCalledWith( + { version: 2, operations: [sampleManifestRecords.a] }, + { + version: 2, + operations: [ + sampleManifestRecords.a, + sampleManifestRecords.b, + sampleManifestRecords.c, + ], + }, + ); + // Three additions, two deletions. + expect(storeSetSpy).toBeCalledTimes(3); + expect(storeDeleteSpy).toBeCalledTimes(2); + }); + }); }); }); }); From 7aa8c03a77b1b50a19452c80450436ea13a19291 Mon Sep 17 00:00:00 2001 From: Evans Hauser Date: Wed, 3 Jul 2019 11:16:19 -0700 Subject: [PATCH 06/36] Add tests for on(Forbidden|Unregistered)Operation --- ...polloServerPluginOperationRegistry.test.ts | 252 ++++++++++++++++++ 1 file changed, 252 insertions(+) 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 8b65a036b24..751c9089f26 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 @@ -1,4 +1,36 @@ import plugin from '../ApolloServerPluginOperationRegistry'; +import { ApolloServerBase, ForbiddenError } from 'apollo-server-core'; +import { + defaultOperationRegistrySignature, + operationHash, +} from 'apollo-graphql'; +import gql from 'graphql-tag'; +import { print } from 'graphql'; + +const typeDefs = gql` + type Query { + hello: String + } +`; + +const query = gql` + query HelloFam { + hello + } +`; + +const normalizedQueryDocument = defaultOperationRegistrySignature( + query, + 'HelloFam', +); +const queryHash = operationHash(normalizedQueryDocument); + +// In order to expose will start and +class ApolloServerMock extends ApolloServerBase { + public async willStart() { + return super.willStart(); + } +} describe('Operation registry plugin', () => { it('will instantiate when not called with options', () => { @@ -8,4 +40,224 @@ describe('Operation registry plugin', () => { it('will instantiate with debug enabled', () => { expect(plugin({ debug: true })()).toHaveProperty('serverWillStart'); }); + + // These tests depend on the behavior of willUpdateManifest to update the + // operation safelist + describe('operation lifecycle hooks', () => { + describe('onUnregisterOperation', () => { + it('is called when unregistered operation received', async () => { + const mock = jest.fn(requestContext => { + expect(requestContext).toMatchObject({ + request: { + operationName: 'HelloFam', + }, + }); + }); + const server = new ApolloServerMock({ + typeDefs, + mockEntireSchema: true, + engine: { + apiKey: 'server:not-a-service:not-an-apikey', + }, + plugins: [ + plugin({ + willUpdateManifest: () => { + return { + version: 2, + operations: [], + }; + }, + onUnregisteredOperation: mock, + })(), + ], + }); + await server.willStart(); + const result = await server.executeOperation({ + query: print(query), + operationName: 'HelloFam', + }); + expect(result.data).toBeDefined(); + expect(result.errors).not.toBeDefined(); + expect(result.data && result.data.hello).toBeDefined(); + expect(mock).toHaveBeenCalledTimes(1); + await server.stop(); + }); + + it('is not called when registered operation received', async () => { + const mock = jest.fn(); + const server = new ApolloServerMock({ + typeDefs, + mockEntireSchema: true, + engine: { + apiKey: 'server:not-a-service:not-an-apikey', + }, + plugins: [ + plugin({ + willUpdateManifest: () => { + return { + version: 2, + operations: [ + { + document: normalizedQueryDocument, + signature: queryHash, + }, + ], + }; + }, + onUnregisteredOperation: mock, + })(), + ], + }); + await server.willStart(); + const result = await server.executeOperation({ + query: print(query), + operationName: 'HelloFam', + }); + expect(result.data).toBeDefined(); + expect(result.errors).not.toBeDefined(); + expect(result.data && result.data.hello).toBeDefined(); + expect(mock).toHaveBeenCalledTimes(0); + await server.stop(); + }); + }); + + describe('onForbiddenOperation', () => { + it('is called when unregistered operation received and forbidden', async () => { + const mock = jest.fn(requestContext => { + expect(requestContext).toMatchObject({ + request: { + operationName: 'HelloFam', + }, + }); + }); + const forbidUnregisteredOperations = jest.fn(requestContext => { + expect(requestContext).toMatchObject({ + request: { + operationName: 'HelloFam', + }, + }); + return true; + }); + + const server = new ApolloServerMock({ + typeDefs, + mockEntireSchema: true, + engine: { + apiKey: 'server:not-a-service:not-an-apikey', + }, + plugins: [ + plugin({ + willUpdateManifest: () => { + return { + version: 2, + operations: [], + }; + }, + forbidUnregisteredOperations, + onForbiddenOperation: mock, + })(), + ], + }); + await server.willStart(); + const result = await server.executeOperation({ + query: print(query), + operationName: 'HelloFam', + }); + expect(result.data).not.toBeDefined(); + expect(result.errors).toBeDefined(); + expect(result.errors).toHaveLength(1); + expect(result.errors && result.errors[0].message).toContain( + 'forbidden', + ); + expect(mock).toHaveBeenCalledTimes(1); + expect(forbidUnregisteredOperations).toHaveBeenCalledTimes(1); + await server.stop(); + }); + + it('is not called when unregistered operation received and unforbidden', async () => { + const mock = jest.fn(requestContext => { + expect(requestContext).toMatchObject({ + request: { + operationName: 'HelloFam', + }, + }); + }); + const forbidUnregisteredOperations = jest.fn(requestContext => { + expect(requestContext).toMatchObject({ + request: { + operationName: 'HelloFam', + }, + }); + return false; + }); + const server = new ApolloServerMock({ + typeDefs, + mockEntireSchema: true, + engine: { + apiKey: 'server:not-a-service:not-an-apikey', + }, + plugins: [ + plugin({ + willUpdateManifest: () => { + return { + version: 2, + operations: [], + }; + }, + forbidUnregisteredOperations, + onForbiddenOperation: mock, + })(), + ], + }); + await server.willStart(); + const result = await server.executeOperation({ + query: print(query), + operationName: 'HelloFam', + }); + expect(result.data).toBeDefined(); + expect(result.errors).not.toBeDefined(); + expect(result.data && result.data.hello).toBeDefined(); + expect(mock).toHaveBeenCalledTimes(0); + expect(forbidUnregisteredOperations).toHaveBeenCalledTimes(1); + await server.stop(); + }); + + it('is not called when registered operation received', async () => { + const mock = jest.fn(); + const server = new ApolloServerMock({ + typeDefs, + mockEntireSchema: true, + engine: { + apiKey: 'server:not-a-service:not-an-apikey', + }, + plugins: [ + plugin({ + willUpdateManifest: () => { + return { + version: 2, + operations: [ + { + document: normalizedQueryDocument, + signature: queryHash, + }, + ], + }; + }, + onForbiddenOperation: mock, + })(), + ], + }); + await server.willStart(); + const result = await server.executeOperation({ + query: print(query), + operationName: 'HelloFam', + }); + expect(result.data).toBeDefined(); + expect(result.errors).not.toBeDefined(); + expect(result.data && result.data.hello).toBeDefined(); + expect(mock).toHaveBeenCalledTimes(0); + await server.stop(); + }); + }); + }); }); From 6ffafbaf5d4bbb5f6e138a9a20490a2bdd29004a Mon Sep 17 00:00:00 2001 From: Evans Hauser Date: Wed, 24 Jul 2019 16:39:31 -0700 Subject: [PATCH 07/36] surround observability hooks in promise --- .../src/ApolloServerPluginOperationRegistry.ts | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts b/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts index 30417a5af05..8972a2fa582 100644 --- a/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts +++ b/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts @@ -164,7 +164,11 @@ for observability purposes, but all operations will be permitted.`, return; } else { if (options.onUnregisteredOperation) { - options.onUnregisteredOperation(requestContext); + new Promise(() => { + if (options.onUnregisteredOperation) { + options.onUnregisteredOperation(requestContext); + } + }); } } @@ -231,7 +235,11 @@ for observability purposes, but all operations will be permitted.`, if (shouldForbidOperation) { if (options.onForbiddenOperation) { - options.onForbiddenOperation(requestContext); + new Promise(() => { + if (options.onForbiddenOperation) { + options.onForbiddenOperation(requestContext); + } + }); } if (!options.dryRun) { logger.debug( From 87334b876254c6b6869a52af0ddb048b599edf2e Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Thu, 17 Oct 2019 14:45:33 +0300 Subject: [PATCH 08/36] Remove memory leak from guaranteed unresolved/unrejected Promise. I question whether or not we want to actually guarantee async execution of this method and not give the opportunity to the user to actually make it block if they'd like to, but we certainly should ensure that Promises should not be created on every request and never resolve or reject, which seems like a prime avenue for a DoS via memory leak. Of course, these should be garbage collected eventually, but we can avoid creating them that way. --- .../ApolloServerPluginOperationRegistry.ts | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts b/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts index 8972a2fa582..9eb7c3b3502 100644 --- a/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts +++ b/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts @@ -163,11 +163,11 @@ for observability purposes, but all operations will be permitted.`, requestContext.metrics.registeredOperation = true; return; } else { - if (options.onUnregisteredOperation) { - new Promise(() => { - if (options.onUnregisteredOperation) { - options.onUnregisteredOperation(requestContext); - } + // If defined, this method should not block, whether async or not. + if (typeof options.onUnregisteredOperation === 'function') { + const onUnregisteredOperation = options.onUnregisteredOperation; + Promise.resolve().then(() => { + onUnregisteredOperation(requestContext); }); } } @@ -234,11 +234,11 @@ for observability purposes, but all operations will be permitted.`, } if (shouldForbidOperation) { - if (options.onForbiddenOperation) { - new Promise(() => { - if (options.onForbiddenOperation) { - options.onForbiddenOperation(requestContext); - } + // If defined, this method should not block, whether async or not. + if (typeof options.onForbiddenOperation === 'function') { + const onForbiddenOperation = options.onForbiddenOperation; + Promise.resolve().then(() => { + onForbiddenOperation(requestContext); }); } if (!options.dryRun) { From 327d1183908dc19647f1c675434fa1b6b2950b12 Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Fri, 22 Nov 2019 18:57:16 +0200 Subject: [PATCH 09/36] op-reg: Rename internal usage of signature and hash to match public-facing manifest. --- .../ApolloServerPluginOperationRegistry.ts | 75 +++++++++++-------- .../src/common.ts | 2 +- 2 files changed, 43 insertions(+), 34 deletions(-) diff --git a/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts b/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts index fcaf8144d35..249d6e63635 100644 --- a/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts +++ b/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts @@ -1,5 +1,5 @@ import * as assert from 'assert'; -import { pluginName, getStoreKey, hashForLogging } from './common'; +import { pluginName, getStoreKey, signatureForLogging } from './common'; import { ApolloServerPlugin, GraphQLServiceContext, @@ -7,8 +7,15 @@ import { GraphQLRequestContext, } from 'apollo-server-plugin-base'; import { - operationHash, - defaultOperationRegistrySignature, + /** + * We alias these to different names entirely since the user-facing values + * which are present in their manifest (signature and document) are probably + * the most important concepts to rally around right now, in terms of + * approachability to the implementor. A future version of the + * `apollo-graphql` package should rename them to make this more clear. + */ + operationHash as operationSignature, + defaultOperationRegistrySignature as defaultOperationRegistryNormalization, } from 'apollo-graphql'; import { ForbiddenError, ApolloError } from 'apollo-server-errors'; import Agent, { OperationManifest } from './agent'; @@ -122,43 +129,45 @@ for observability purposes, but all operations will be permitted.`, throw new Error('Unable to access store.'); } - const hash = operationHash( - defaultOperationRegistrySignature( - document, - - // XXX The `operationName` is set from the AST, not from the - // request `operationName`. If `operationName` is `null`, - // then the operation is anonymous. However, it's not possible - // to register anonymous operations from the `apollo` CLI. - // We could fail early, however, we still want to abide by the - // desires of `forbidUnregisteredOperations`, so we'll allow - // this hash be generated anyway. The hash cannot be in the - // manifest, so this would be okay and allow this code to remain - // less conditional-y, eventually forbidding the operation when - // the hash is not found and `forbidUnregisteredOperations` is on. - requestContext.operationName || '', - ), + const normalizedDocument = defaultOperationRegistryNormalization( + document, + + // XXX The `operationName` is set from the AST, not from the + // request `operationName`. If `operationName` is `null`, + // then the operation is anonymous. However, it's not possible + // to register anonymous operations from the `apollo` CLI. + // We could fail early, however, we still want to abide by the + // desires of `forbidUnregisteredOperations`, so we'll allow + // this signature to be generated anyway. It could not be in the + // manifest, so this would be okay and allow this code to remain + // less conditional-y, eventually forbidding the operation when + // the signature is absent and `forbidUnregisteredOperations` is on. + requestContext.operationName || '', ); - if (!hash) { + const signature = operationSignature(normalizedDocument); + + if (!signature) { throw new ApolloError('No document.'); } - // The hashes are quite long and it seems we can get by with a substr. - const logHash = hashForLogging(hash); + // The signatures are quite long so we truncate to a prefix of it. + const logSignature = signatureForLogging(signature); - logger.debug(`${logHash}: Looking up operation in local registry.`); + logger.debug( + `${logSignature}: Looking up operation in local registry.`, + ); // Try to fetch the operation from the store of operations we're // currently aware of, which has been populated by the operation // registry. - const storeFetch = await store.get(getStoreKey(hash)); + const storeFetch = await store.get(getStoreKey(signature)); // If we have a hit, we'll return immediately, signaling that we're // not intending to block this request. if (storeFetch) { logger.debug( - `${logHash}: Permitting operation found in local registry.`, + `${logSignature}: Permitting operation found in local registry.`, ); requestContext.metrics.registeredOperation = true; return; @@ -190,7 +199,7 @@ for observability purposes, but all operations will be permitted.`, if (typeof options.forbidUnregisteredOperations === 'function') { logger.debug( - `${logHash}: Calling 'forbidUnregisteredOperations' predicate function with requestContext...`, + `${logSignature}: Calling 'forbidUnregisteredOperations' predicate function with requestContext...`, ); try { @@ -199,7 +208,7 @@ for observability purposes, but all operations will be permitted.`, ); logger.debug( - `${logHash}: The 'forbidUnregisteredOperations' predicate function returned ${predicateResult}`, + `${logSignature}: The 'forbidUnregisteredOperations' predicate function returned ${predicateResult}`, ); // If we've received a boolean back from the predicate function, @@ -211,7 +220,7 @@ for observability purposes, but all operations will be permitted.`, shouldForbidOperation = predicateResult; } else { logger.warn( - `${logHash} Predicate function did not return a boolean response. Got ${predicateResult}`, + `${logSignature} Predicate function did not return a boolean response. Got ${predicateResult}`, ); } } catch (err) { @@ -219,7 +228,7 @@ for observability purposes, but all operations will be permitted.`, // predicate function, we should assume that the implementor // had a security-wise intention and remain in enforcement mode. logger.error( - `${logHash}: An error occurred within the 'forbidUnregisteredOperations' predicate function: ${err}`, + `${logSignature}: An error occurred within the 'forbidUnregisteredOperations' predicate function: ${err}`, ); } } @@ -228,7 +237,7 @@ for observability purposes, but all operations will be permitted.`, // should be forbidden, we report it within metrics as forbidden, even though we may be running in dryRun mode. if (shouldForbidOperation && options.forbidUnregisteredOperations) { logger.debug( - `${logHash} Reporting operation as forbidden to Apollo trace warehouse.`, + `${logSignature} Reporting operation as forbidden to Apollo trace warehouse.`, ); requestContext.metrics.forbiddenOperation = true; @@ -244,13 +253,13 @@ for observability purposes, but all operations will be permitted.`, if (shouldForbidOperation) { if (!options.dryRun) { logger.debug( - `${logHash}: Execution denied because 'forbidUnregisteredOperations' was enabled for this request and the operation was not found in the local operation registry.`, + `${logSignature}: Execution denied because 'forbidUnregisteredOperations' was enabled for this request and the operation was not found in the local operation registry.`, ); const error = new ForbiddenError( 'Execution forbidden: Operation not found in operation registry', ); Object.assign(error.extensions, { - operationHash: hash, + operationSignature: signature, exception: { message: `Please register your operation with \`npx apollo client:push --tag="${schemaTag}"\`. See https://www.apollographql.com/docs/platform/operation-registry/ for more details.`, }, @@ -258,7 +267,7 @@ for observability purposes, but all operations will be permitted.`, throw error; } else { logger.debug( - `${dryRunPrefix} ${logHash}: Operation ${requestContext.operationName} would have been forbidden.`, + `${dryRunPrefix} ${logSignature}: Operation ${requestContext.operationName} would have been forbidden.`, ); } } diff --git a/packages/apollo-server-plugin-operation-registry/src/common.ts b/packages/apollo-server-plugin-operation-registry/src/common.ts index 4ba0bdac27a..a9dda2383ef 100644 --- a/packages/apollo-server-plugin-operation-registry/src/common.ts +++ b/packages/apollo-server-plugin-operation-registry/src/common.ts @@ -58,7 +58,7 @@ export function getLegacyOperationManifestUrl( ); } -export function hashForLogging(hash: string): string { +export function signatureForLogging(hash: string): string { if (typeof hash !== 'string') { return ''; } From e32434411cefa70cecc53c377ef928f5255a3445 Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Fri, 22 Nov 2019 18:59:35 +0200 Subject: [PATCH 10/36] Adjust tests to support testing multiple arguments and adjust assertions. I'm not actually certain that the `expect` assertions inside of the `jest.jn` calls were actually being exercised before, but in an effort to support testing multiple arguments, I'll switch to using the Jest matchers which are meant for asserting calls to methods (i.e. `toHaveBeenCalledWith`). --- ...polloServerPluginOperationRegistry.test.ts | 78 ++++++++----------- 1 file changed, 32 insertions(+), 46 deletions(-) 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 751c9089f26..00ea3d26682 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 @@ -46,13 +46,7 @@ describe('Operation registry plugin', () => { describe('operation lifecycle hooks', () => { describe('onUnregisterOperation', () => { it('is called when unregistered operation received', async () => { - const mock = jest.fn(requestContext => { - expect(requestContext).toMatchObject({ - request: { - operationName: 'HelloFam', - }, - }); - }); + const onUnregisteredOperation = jest.fn(); const server = new ApolloServerMock({ typeDefs, mockEntireSchema: true, @@ -67,7 +61,7 @@ describe('Operation registry plugin', () => { operations: [], }; }, - onUnregisteredOperation: mock, + onUnregisteredOperation, })(), ], }); @@ -79,12 +73,19 @@ describe('Operation registry plugin', () => { expect(result.data).toBeDefined(); expect(result.errors).not.toBeDefined(); expect(result.data && result.data.hello).toBeDefined(); - expect(mock).toHaveBeenCalledTimes(1); + expect(onUnregisteredOperation).toHaveBeenCalledTimes(1); + expect(onUnregisteredOperation).toHaveBeenCalledWith( + expect.objectContaining({ + request: expect.objectContaining({ + operationName: 'HelloFam', + }), + }), + ); await server.stop(); }); it('is not called when registered operation received', async () => { - const mock = jest.fn(); + const onUnregisteredOperation = jest.fn(); const server = new ApolloServerMock({ typeDefs, mockEntireSchema: true, @@ -104,7 +105,7 @@ describe('Operation registry plugin', () => { ], }; }, - onUnregisteredOperation: mock, + onUnregisteredOperation, })(), ], }); @@ -116,28 +117,17 @@ describe('Operation registry plugin', () => { expect(result.data).toBeDefined(); expect(result.errors).not.toBeDefined(); expect(result.data && result.data.hello).toBeDefined(); - expect(mock).toHaveBeenCalledTimes(0); + expect(onUnregisteredOperation).toHaveBeenCalledTimes(0); await server.stop(); }); }); describe('onForbiddenOperation', () => { it('is called when unregistered operation received and forbidden', async () => { - const mock = jest.fn(requestContext => { - expect(requestContext).toMatchObject({ - request: { - operationName: 'HelloFam', - }, - }); - }); - const forbidUnregisteredOperations = jest.fn(requestContext => { - expect(requestContext).toMatchObject({ - request: { - operationName: 'HelloFam', - }, - }); - return true; - }); + const onForbiddenOperation = jest.fn(); + + // Returning true from this predicate enables the enforcement. + const forbidUnregisteredOperations = jest.fn(() => true); const server = new ApolloServerMock({ typeDefs, @@ -154,7 +144,7 @@ describe('Operation registry plugin', () => { }; }, forbidUnregisteredOperations, - onForbiddenOperation: mock, + onForbiddenOperation, })(), ], }); @@ -169,27 +159,23 @@ describe('Operation registry plugin', () => { expect(result.errors && result.errors[0].message).toContain( 'forbidden', ); - expect(mock).toHaveBeenCalledTimes(1); + expect(onForbiddenOperation).toHaveBeenCalledTimes(1); + expect(onForbiddenOperation).toHaveBeenCalledWith( + expect.objectContaining({ + request: expect.objectContaining({ + operationName: 'HelloFam', + }), + }), + ); expect(forbidUnregisteredOperations).toHaveBeenCalledTimes(1); await server.stop(); }); it('is not called when unregistered operation received and unforbidden', async () => { - const mock = jest.fn(requestContext => { - expect(requestContext).toMatchObject({ - request: { - operationName: 'HelloFam', - }, - }); - }); - const forbidUnregisteredOperations = jest.fn(requestContext => { - expect(requestContext).toMatchObject({ - request: { - operationName: 'HelloFam', - }, - }); - return false; - }); + const onForbiddenOperation = jest.fn(); + + // Returning true from this predicate enables the enforcement. + const forbidUnregisteredOperations = jest.fn(() => false); const server = new ApolloServerMock({ typeDefs, mockEntireSchema: true, @@ -205,7 +191,7 @@ describe('Operation registry plugin', () => { }; }, forbidUnregisteredOperations, - onForbiddenOperation: mock, + onForbiddenOperation, })(), ], }); @@ -217,7 +203,7 @@ describe('Operation registry plugin', () => { expect(result.data).toBeDefined(); expect(result.errors).not.toBeDefined(); expect(result.data && result.data.hello).toBeDefined(); - expect(mock).toHaveBeenCalledTimes(0); + expect(onForbiddenOperation).toHaveBeenCalledTimes(0); expect(forbidUnregisteredOperations).toHaveBeenCalledTimes(1); await server.stop(); }); From 24d964f9c539bd2a7259d437d4c26442f40ce6e8 Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Fri, 22 Nov 2019 19:01:00 +0200 Subject: [PATCH 11/36] Switch from using `graphql`'s `PromiseOrValue` to our `ValueOrPromise`. I don't believe this was intentionally done, but more than likely auto-complete. --- packages/apollo-server-plugin-operation-registry/src/agent.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/apollo-server-plugin-operation-registry/src/agent.ts b/packages/apollo-server-plugin-operation-registry/src/agent.ts index d00c36a1576..f607bae9ecf 100644 --- a/packages/apollo-server-plugin-operation-registry/src/agent.ts +++ b/packages/apollo-server-plugin-operation-registry/src/agent.ts @@ -12,7 +12,7 @@ import loglevel from 'loglevel'; import { Response } from 'node-fetch'; import { InMemoryLRUCache } from 'apollo-server-caching'; import { fetchIfNoneMatch } from './fetchIfNoneMatch'; -import { PromiseOrValue } from 'graphql/jsutils/PromiseOrValue'; +import { ValueOrPromise } from "apollo-server-plugin-base"; const DEFAULT_POLL_SECONDS: number = 30; const SYNC_WARN_TIME_SECONDS: number = 60; @@ -27,7 +27,7 @@ export interface AgentOptions { willUpdateManifest?: ( newManifest?: OperationManifest, oldManifest?: OperationManifest, - ) => PromiseOrValue; + ) => ValueOrPromise; } export interface Operation { From af7e3d1b3fa61368dd1e6dee17ea1401ef15a630 Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Fri, 22 Nov 2019 19:02:08 +0200 Subject: [PATCH 12/36] Add 2nd argument to `on(Forbidden|Unregistered)Operation` with op details. This ensures that the `signature` that was calculated and the `normalizedDocument` that the `signature` is based on is available in the new observability hooks, so they can be compared against the same value in the published operation manifest. --- .../ApolloServerPluginOperationRegistry.ts | 25 ++++++++++++++++--- ...polloServerPluginOperationRegistry.test.ts | 14 +++++++++++ 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts b/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts index 249d6e63635..27b5fd574a3 100644 --- a/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts +++ b/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts @@ -29,6 +29,11 @@ type ForbidUnregisteredOperationsPredicate = ( requestContext: GraphQLRequestContext, ) => boolean; +interface OperationRegistryRequestContext { + signature: string; + normalizedDocument: string; +} + interface Options { debug?: boolean; forbidUnregisteredOperations?: @@ -36,8 +41,14 @@ interface Options { | ForbidUnregisteredOperationsPredicate; dryRun?: boolean; schemaTag?: string; - onUnregisteredOperation?: (requestContext: GraphQLRequestContext) => void; - onForbiddenOperation?: (requestContext: GraphQLRequestContext) => void; + onUnregisteredOperation?: ( + requestContext: GraphQLRequestContext, + operationRegistryRequestContext: OperationRegistryRequestContext, + ) => void; + onForbiddenOperation?: ( + requestContext: GraphQLRequestContext, + operationRegistryRequestContext: OperationRegistryRequestContext, + ) => void; willUpdateManifest?: ( newManifest?: OperationManifest, oldManifest?: OperationManifest, @@ -176,7 +187,10 @@ for observability purposes, but all operations will be permitted.`, if (typeof options.onUnregisteredOperation === 'function') { const onUnregisteredOperation = options.onUnregisteredOperation; Promise.resolve().then(() => { - onUnregisteredOperation(requestContext); + onUnregisteredOperation(requestContext, { + signature, + normalizedDocument, + }); }); } } @@ -245,7 +259,10 @@ for observability purposes, but all operations will be permitted.`, if (typeof options.onForbiddenOperation === 'function') { const onForbiddenOperation = options.onForbiddenOperation; Promise.resolve().then(() => { - onForbiddenOperation(requestContext); + onForbiddenOperation(requestContext, { + signature, + normalizedDocument, + }); }); } } 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 00ea3d26682..1a9bd67cf3d 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 @@ -75,11 +75,18 @@ describe('Operation registry plugin', () => { expect(result.data && result.data.hello).toBeDefined(); expect(onUnregisteredOperation).toHaveBeenCalledTimes(1); expect(onUnregisteredOperation).toHaveBeenCalledWith( + // First argument: request pipeline context. expect.objectContaining({ request: expect.objectContaining({ operationName: 'HelloFam', }), }), + + // Second argument: operation registry context. + expect.objectContaining({ + signature: expect.stringMatching(/^[a-f0-9]+$/), + normalizedDocument: expect.stringMatching(/^query HelloFam/) + }), ); await server.stop(); }); @@ -161,11 +168,18 @@ describe('Operation registry plugin', () => { ); expect(onForbiddenOperation).toHaveBeenCalledTimes(1); expect(onForbiddenOperation).toHaveBeenCalledWith( + // First argument: request pipeline context. expect.objectContaining({ request: expect.objectContaining({ operationName: 'HelloFam', }), }), + + // Second argument: operation registry context. + expect.objectContaining({ + signature: expect.stringMatching(/^[a-f0-9]+$/), + normalizedDocument: expect.stringMatching(/^query HelloFam/) + }), ); expect(forbidUnregisteredOperations).toHaveBeenCalledTimes(1); await server.stop(); From c6abd0585e17a1bd9cca0bfa052e39744d3a7eaf Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Fri, 22 Nov 2019 19:04:40 +0200 Subject: [PATCH 13/36] Rename `document` to specify its source and avoid overlap/overloading. --- .../src/ApolloServerPluginOperationRegistry.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts b/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts index 27b5fd574a3..9bb2692e694 100644 --- a/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts +++ b/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts @@ -133,7 +133,7 @@ for observability purposes, but all operations will be permitted.`, requestDidStart(): GraphQLRequestListener { return { async didResolveOperation(requestContext) { - const document = requestContext.document; + const documentFromRequestContext = requestContext.document; // This shouldn't happen under normal operation since `store` will be // set in `serverWillStart` and `requestDidStart` (this) comes after. if (!store) { @@ -141,7 +141,7 @@ for observability purposes, but all operations will be permitted.`, } const normalizedDocument = defaultOperationRegistryNormalization( - document, + documentFromRequestContext, // XXX The `operationName` is set from the AST, not from the // request `operationName`. If `operationName` is `null`, From 3a47e55d4412646d439ca608703a50b4820d99a0 Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Sat, 23 Nov 2019 14:38:03 +0200 Subject: [PATCH 14/36] Rename `hash to `signature` in the type arg for `signatureForLogging`. Follows-up previous work. --- .../apollo-server-plugin-operation-registry/src/common.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/apollo-server-plugin-operation-registry/src/common.ts b/packages/apollo-server-plugin-operation-registry/src/common.ts index a9dda2383ef..371834ad399 100644 --- a/packages/apollo-server-plugin-operation-registry/src/common.ts +++ b/packages/apollo-server-plugin-operation-registry/src/common.ts @@ -58,9 +58,9 @@ export function getLegacyOperationManifestUrl( ); } -export function signatureForLogging(hash: string): string { - if (typeof hash !== 'string') { +export function signatureForLogging(signature: string): string { + if (typeof signature !== 'string') { return ''; } - return hash.substring(0, 8); + return signature.substring(0, 8); } From feee7e3109c43a1a2559dcd4ce19d783f673c475 Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Sat, 23 Nov 2019 15:15:01 +0200 Subject: [PATCH 15/36] Change addl. use of `graphql-js` `PromiseOrValue` to our own `ValueOrPromise`. Same spirit as referenced commit. Ref: 1fe29a58f59e31b16b3be8e5fd80dcf8d2c06e43 --- .../src/ApolloServerPluginOperationRegistry.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts b/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts index 9bb2692e694..416ecdcd49f 100644 --- a/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts +++ b/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts @@ -23,7 +23,7 @@ import { GraphQLSchema } from 'graphql/type'; import { InMemoryLRUCache } from 'apollo-server-caching'; import loglevel from 'loglevel'; import loglevelDebug from 'loglevel-debug'; -import { PromiseOrValue } from 'graphql/jsutils/PromiseOrValue'; +import { ValueOrPromise } from 'apollo-server-plugin-base'; type ForbidUnregisteredOperationsPredicate = ( requestContext: GraphQLRequestContext, @@ -52,7 +52,7 @@ interface Options { willUpdateManifest?: ( newManifest?: OperationManifest, oldManifest?: OperationManifest, - ) => PromiseOrValue; + ) => ValueOrPromise; } export default function plugin(options: Options = Object.create(null)) { From 504bd89cc47bfece4ab5c1f7ce1a24f1c6b86e54 Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Sat, 23 Nov 2019 15:17:37 +0200 Subject: [PATCH 16/36] Update incorrect method name in comment. This was still using the old name of `onManifestUpdate`! Ref: https://github.com/apollographql/apollo-platform-commercial/pull/246/files#r346821528 --- packages/apollo-server-plugin-operation-registry/src/agent.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/apollo-server-plugin-operation-registry/src/agent.ts b/packages/apollo-server-plugin-operation-registry/src/agent.ts index f607bae9ecf..0c444276a49 100644 --- a/packages/apollo-server-plugin-operation-registry/src/agent.ts +++ b/packages/apollo-server-plugin-operation-registry/src/agent.ts @@ -107,7 +107,7 @@ export default class Agent { try { await pulse(); } catch (err) { - // Update the manifest to trigger `onManifestUpdate(newManifest: undefined, oldManifest: undefined)` + // Update the manifest to trigger `willUpdateManifest(newManifest: undefined, oldManifest: undefined)` this.updateManifest(); console.error( `The operation manifest could not be fetched. Retries will continue.${ From 1f8dd3bd7cbc2d7db7ae60098269db55b5e9c055 Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Sat, 23 Nov 2019 15:20:15 +0200 Subject: [PATCH 17/36] Await the `this.updateManifest` method to ensure it completes. --- packages/apollo-server-plugin-operation-registry/src/agent.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/apollo-server-plugin-operation-registry/src/agent.ts b/packages/apollo-server-plugin-operation-registry/src/agent.ts index 0c444276a49..e4e91851b05 100644 --- a/packages/apollo-server-plugin-operation-registry/src/agent.ts +++ b/packages/apollo-server-plugin-operation-registry/src/agent.ts @@ -108,7 +108,7 @@ export default class Agent { await pulse(); } catch (err) { // Update the manifest to trigger `willUpdateManifest(newManifest: undefined, oldManifest: undefined)` - this.updateManifest(); + await this.updateManifest(); console.error( `The operation manifest could not be fetched. Retries will continue.${ this.lastManifest From 6c15cb26ca4069813a6fca08f0508bf9db06c227 Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Sat, 23 Nov 2019 15:35:55 +0200 Subject: [PATCH 18/36] Remove unused `import` `ForbiddenError`. --- .../src/__tests__/ApolloServerPluginOperationRegistry.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 1a9bd67cf3d..3868091b504 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 @@ -1,5 +1,5 @@ import plugin from '../ApolloServerPluginOperationRegistry'; -import { ApolloServerBase, ForbiddenError } from 'apollo-server-core'; +import { ApolloServerBase } from 'apollo-server-core'; import { defaultOperationRegistrySignature, operationHash, From b1ab19c86b3d1c1dc318d575ed59ca36852cf465 Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Sat, 23 Nov 2019 15:41:02 +0200 Subject: [PATCH 19/36] Alias names for internal use which matches external terminology. Same as df28d8578767cf6bdcf885eba9a02a03154c4f95, except for within the tests. Essentially, let's use common terminology with the publicly facing manifest, just to have common terminology. We should rename the exports from the `apollo-graphql` package eventually. --- .../ApolloServerPluginOperationRegistry.test.ts | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) 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 3868091b504..97c60e4cc40 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 @@ -1,8 +1,15 @@ import plugin from '../ApolloServerPluginOperationRegistry'; import { ApolloServerBase } from 'apollo-server-core'; import { - defaultOperationRegistrySignature, - operationHash, + /** + * We alias these to different names entirely since the user-facing values + * which are present in their manifest (signature and document) are probably + * the most important concepts to rally around right now, in terms of + * approachability to the implementor. A future version of the + * `apollo-graphql` package should rename them to make this more clear. + */ + defaultOperationRegistrySignature as defaultOperationRegistryNormalization, + operationHash as operationSignature, } from 'apollo-graphql'; import gql from 'graphql-tag'; import { print } from 'graphql'; @@ -19,11 +26,11 @@ const query = gql` } `; -const normalizedQueryDocument = defaultOperationRegistrySignature( +const normalizedQueryDocument = defaultOperationRegistryNormalization( query, 'HelloFam', ); -const queryHash = operationHash(normalizedQueryDocument); +const queryHash = operationSignature(normalizedQueryDocument); // In order to expose will start and class ApolloServerMock extends ApolloServerBase { From 1639d1e4796f50ae1f95fec90ed111016491ebca Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Mon, 25 Nov 2019 20:30:45 +0200 Subject: [PATCH 20/36] Unreference the polling timer; Same as landed in #223. While we don't have a currently exposed ability to shut-down the server automatically using `.stop` when it receives `process.on('exit')` or similar, we can `unref` the timer to ensure that it doesn't keep the event loop active. This is more of a pronounced problem in tests, but we can address it eventually with proper shutdown hooks provided by Apollo Server itself. --- .../apollo-server-plugin-operation-registry/src/agent.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packages/apollo-server-plugin-operation-registry/src/agent.ts b/packages/apollo-server-plugin-operation-registry/src/agent.ts index e4e91851b05..0b12e0d5751 100644 --- a/packages/apollo-server-plugin-operation-registry/src/agent.ts +++ b/packages/apollo-server-plugin-operation-registry/src/agent.ts @@ -129,6 +129,11 @@ export default class Agent { // These errors will be logged, but not crash the server. pulse().catch(err => console.error(err.message || err)); }, this.pollSeconds() * 1000); + + // Prevent the Node.js event loop from remaining active (and preventing, + // e.g. process shutdown) by calling `unref` on the `Timeout`. For more + // information, see https://nodejs.org/api/timers.html#timers_timeout_unref. + this.timer.unref(); } public stop() { From 511668dd35de00ad795868279d74bdec8cdfffe8 Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Mon, 25 Nov 2019 20:30:50 +0200 Subject: [PATCH 21/36] Partially merge conditional branches of overlapping concern. NB: Suggesting side-by-side review. As I pointed out in https://github.com/apollographql/apollo-platform-commercial/pull/246#discussion_r346820147. The existing logic here was branching, re-converging and then branching again for the same `shouldForbidOperation` symbol's truthiness. This creates a fork in logic which seems to be for semantic reasoning, but is not. It also made a merge conflict resolution for this branch particularly unfriendly. Anyhow, it's unnecessary to have this separation and this code should read more linearly without it. --- .../ApolloServerPluginOperationRegistry.ts | 32 +++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts b/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts index 416ecdcd49f..819e28a5fc7 100644 --- a/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts +++ b/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts @@ -249,25 +249,25 @@ for observability purposes, but all operations will be permitted.`, // If the user explicitly set forbidUnregisteredOperations to either `true` or a function, and the operation // should be forbidden, we report it within metrics as forbidden, even though we may be running in dryRun mode. - if (shouldForbidOperation && options.forbidUnregisteredOperations) { - logger.debug( - `${logSignature} Reporting operation as forbidden to Apollo trace warehouse.`, - ); - requestContext.metrics.forbiddenOperation = true; - - // If defined, this method should not block, whether async or not. - if (typeof options.onForbiddenOperation === 'function') { - const onForbiddenOperation = options.onForbiddenOperation; - Promise.resolve().then(() => { - onForbiddenOperation(requestContext, { - signature, - normalizedDocument, + if (shouldForbidOperation) { + if (options.forbidUnregisteredOperations) { + logger.debug( + `${logSignature} Reporting operation as forbidden to Apollo trace warehouse.`, + ); + requestContext.metrics.forbiddenOperation = true; + + // If defined, this method should not block, whether async or not. + if (typeof options.onForbiddenOperation === 'function') { + const onForbiddenOperation = options.onForbiddenOperation; + Promise.resolve().then(() => { + onForbiddenOperation(requestContext, { + signature, + normalizedDocument, + }); }); - }); + } } - } - if (shouldForbidOperation) { if (!options.dryRun) { logger.debug( `${logSignature}: Execution denied because 'forbidUnregisteredOperations' was enabled for this request and the operation was not found in the local operation registry.`, From 39ac58658cbb951ff9693298712043bdf86d8af2 Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Mon, 25 Nov 2019 20:30:53 +0200 Subject: [PATCH 22/36] Return early if `shouldForbidOperation` is set. This avoids a very tall, indented block of `if (shouldForbidOperation)` which previously represented the remaining logic in this function with a block which returns early if the opposite is true (i.e. `if (!shouldForbidOperation) return;`). This makes the code easier to reason about, to me. --- .../ApolloServerPluginOperationRegistry.ts | 84 +++++++++++-------- 1 file changed, 47 insertions(+), 37 deletions(-) diff --git a/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts b/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts index 819e28a5fc7..bc8dcd0b7aa 100644 --- a/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts +++ b/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts @@ -247,47 +247,57 @@ for observability purposes, but all operations will be permitted.`, } } - // If the user explicitly set forbidUnregisteredOperations to either `true` or a function, and the operation - // should be forbidden, we report it within metrics as forbidden, even though we may be running in dryRun mode. - if (shouldForbidOperation) { - if (options.forbidUnregisteredOperations) { - logger.debug( - `${logSignature} Reporting operation as forbidden to Apollo trace warehouse.`, - ); - requestContext.metrics.forbiddenOperation = true; - - // If defined, this method should not block, whether async or not. - if (typeof options.onForbiddenOperation === 'function') { - const onForbiddenOperation = options.onForbiddenOperation; - Promise.resolve().then(() => { - onForbiddenOperation(requestContext, { - signature, - normalizedDocument, - }); - }); - } - } + // Whether we're in dryRun mode or not, the decision as to whether + // or not we'll be forbidding execution has already been decided. + // Therefore, we'll return early and avoid nesting this entire + // remaining 30+ line block in a `if (shouldForbidOperation)` fork. + if (!shouldForbidOperation) { + return; + } - if (!options.dryRun) { - logger.debug( - `${logSignature}: Execution denied because 'forbidUnregisteredOperations' was enabled for this request and the operation was not found in the local operation registry.`, - ); - const error = new ForbiddenError( - 'Execution forbidden: Operation not found in operation registry', - ); - Object.assign(error.extensions, { - operationSignature: signature, - exception: { - message: `Please register your operation with \`npx apollo client:push --tag="${schemaTag}"\`. See https://www.apollographql.com/docs/platform/operation-registry/ for more details.`, - }, + // If the user explicitly set `forbidUnregisteredOperations` to either + // `true` or a (predicate) function which returns `true` we'll + // report it within metrics as forbidden, even though we may be + // running in `dryRun` mode. This allows the user to incrementally + // go through their code-base and ensure that they've reached + // an "inbox zero" - so to speak - of operations needing registration. + if (options.forbidUnregisteredOperations) { + logger.debug( + `${logSignature} Reporting operation as forbidden to Apollo trace warehouse.`, + ); + requestContext.metrics.forbiddenOperation = true; + + // If defined, this method should not block, whether async or not. + if (typeof options.onForbiddenOperation === 'function') { + const onForbiddenOperation = options.onForbiddenOperation; + Promise.resolve().then(() => { + onForbiddenOperation(requestContext, { + signature, + normalizedDocument, + }); }); - throw error; - } else { - logger.debug( - `${dryRunPrefix} ${logSignature}: Operation ${requestContext.operationName} would have been forbidden.`, - ); } } + + if (!options.dryRun) { + logger.debug( + `${logSignature}: Execution denied because 'forbidUnregisteredOperations' was enabled for this request and the operation was not found in the local operation registry.`, + ); + const error = new ForbiddenError( + 'Execution forbidden: Operation not found in operation registry', + ); + Object.assign(error.extensions, { + operationSignature: signature, + exception: { + message: `Please register your operation with \`npx apollo client:push --tag="${schemaTag}"\`. See https://www.apollographql.com/docs/platform/operation-registry/ for more details.`, + }, + }); + throw error; + } else { + logger.debug( + `${dryRunPrefix} ${logSignature}: Operation ${requestContext.operationName} would have been forbidden.`, + ); + } }, }; }, From 7125c72e52d084f24c033ddca254611db543dd0c Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Mon, 25 Nov 2019 20:30:57 +0200 Subject: [PATCH 23/36] Reduce height of tall conditionals by returning early. At this stage, we're in a mode of enforcement only. Dry-run is ruled out and there is no chance of a predicate decision changing. Therefore, some slight tweaks in the ordering of operations here should make the code easier to reason about since it avoids the need to remember how many forks of logic we're inside. To summarize this change which Git will make difficult to read: This change moves the `logger.debug` for `dryRun` mode above the actual enforcement (i.e. non-`dryRun`!) section and have it `return`, rather than having a tall `if (!options.dryRun)` with many lines of logic when there are no remaining forks of logic in the function after it that need to be considered. --- .../ApolloServerPluginOperationRegistry.ts | 31 ++++++++++--------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts b/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts index bc8dcd0b7aa..ccb032d73e6 100644 --- a/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts +++ b/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts @@ -279,25 +279,26 @@ for observability purposes, but all operations will be permitted.`, } } - if (!options.dryRun) { - logger.debug( - `${logSignature}: Execution denied because 'forbidUnregisteredOperations' was enabled for this request and the operation was not found in the local operation registry.`, - ); - const error = new ForbiddenError( - 'Execution forbidden: Operation not found in operation registry', - ); - Object.assign(error.extensions, { - operationSignature: signature, - exception: { - message: `Please register your operation with \`npx apollo client:push --tag="${schemaTag}"\`. See https://www.apollographql.com/docs/platform/operation-registry/ for more details.`, - }, - }); - throw error; - } else { + if (options.dryRun) { logger.debug( `${dryRunPrefix} ${logSignature}: Operation ${requestContext.operationName} would have been forbidden.`, ); + return; } + + logger.debug( + `${logSignature}: Execution denied because 'forbidUnregisteredOperations' was enabled for this request and the operation was not found in the local operation registry.`, + ); + const error = new ForbiddenError( + 'Execution forbidden: Operation not found in operation registry', + ); + Object.assign(error.extensions, { + operationSignature: signature, + exception: { + message: `Please register your operation with \`npx apollo client:push --tag="${schemaTag}"\`. See https://www.apollographql.com/docs/platform/operation-registry/ for more details.`, + }, + }); + throw error; }, }; }, From f550e04a2482bae1d9a7c41ef9d722450f439545 Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Mon, 25 Nov 2019 20:31:01 +0200 Subject: [PATCH 24/36] Use function name of mocked function for variable holding its mock. Rather than using `mock`, this makes it more clear what mocked function is being used. It also avoids the need to map it in the object that we're passing to the plugin's constructor options since the shorthand works. Also matches the pattern used elsewhere in the file. --- .../__tests__/ApolloServerPluginOperationRegistry.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 97c60e4cc40..be8fbfae535 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 @@ -230,7 +230,7 @@ describe('Operation registry plugin', () => { }); it('is not called when registered operation received', async () => { - const mock = jest.fn(); + const onForbiddenOperation = jest.fn(); const server = new ApolloServerMock({ typeDefs, mockEntireSchema: true, @@ -250,7 +250,7 @@ describe('Operation registry plugin', () => { ], }; }, - onForbiddenOperation: mock, + onForbiddenOperation, })(), ], }); @@ -262,7 +262,7 @@ describe('Operation registry plugin', () => { expect(result.data).toBeDefined(); expect(result.errors).not.toBeDefined(); expect(result.data && result.data.hello).toBeDefined(); - expect(mock).toHaveBeenCalledTimes(0); + expect(onForbiddenOperation).toHaveBeenCalledTimes(0); await server.stop(); }); }); From 0ef8dfc9682702c715618f94b3587483d5870230 Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Mon, 25 Nov 2019 20:31:04 +0200 Subject: [PATCH 25/36] Export the `OperationRegistryRequestContext` type for consumers. Follows-up on fbb36b7a3b3a8004f5057e542e2a0e9bf596b35a. --- .../src/ApolloServerPluginOperationRegistry.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts b/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts index ccb032d73e6..695bb55136f 100644 --- a/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts +++ b/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts @@ -29,7 +29,7 @@ type ForbidUnregisteredOperationsPredicate = ( requestContext: GraphQLRequestContext, ) => boolean; -interface OperationRegistryRequestContext { +export interface OperationRegistryRequestContext { signature: string; normalizedDocument: string; } From c25716cb46ba31359d92138b98d811da9c81a9db Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Mon, 25 Nov 2019 20:31:08 +0200 Subject: [PATCH 26/36] Use the typings from `Options` to resolve `any`-typed callbacks in tests. The `onUnregisteredOptions` and `onForbiddenOperation` hooks weren't properly typed in the mocked tests, which was allowing them to have the incorrect type arguments. This wouldn't be a problem with in-lined definitions of these options - since that would be inferred from the parent object literal's definition, but for tests where we assign the function to a variable first, it is problematic. --- .../src/ApolloServerPluginOperationRegistry.ts | 2 +- .../__tests__/ApolloServerPluginOperationRegistry.test.ts | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts b/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts index 695bb55136f..0bb1ee27be8 100644 --- a/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts +++ b/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts @@ -34,7 +34,7 @@ export interface OperationRegistryRequestContext { normalizedDocument: string; } -interface Options { +export interface Options { debug?: boolean; forbidUnregisteredOperations?: | boolean 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 be8fbfae535..8dbcd62a138 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 @@ -1,4 +1,4 @@ -import plugin from '../ApolloServerPluginOperationRegistry'; +import plugin, { Options } from '../ApolloServerPluginOperationRegistry'; import { ApolloServerBase } from 'apollo-server-core'; import { /** @@ -53,7 +53,7 @@ describe('Operation registry plugin', () => { describe('operation lifecycle hooks', () => { describe('onUnregisterOperation', () => { it('is called when unregistered operation received', async () => { - const onUnregisteredOperation = jest.fn(); + const onUnregisteredOperation: Options['onUnregisteredOperation'] = jest.fn(); const server = new ApolloServerMock({ typeDefs, mockEntireSchema: true, @@ -99,7 +99,7 @@ describe('Operation registry plugin', () => { }); it('is not called when registered operation received', async () => { - const onUnregisteredOperation = jest.fn(); + const onUnregisteredOperation: Options['onUnregisteredOperation'] = jest.fn(); const server = new ApolloServerMock({ typeDefs, mockEntireSchema: true, From 9af20082024a5ab4702a220d22d6b0ebab72c9b4 Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Mon, 25 Nov 2019 20:31:12 +0200 Subject: [PATCH 27/36] Well beyond the rule of threes: set the re-used `apiKey` to a constant. Even more so because I just had to change a typo in its pattern which used `server:` rather than the `service:` prefix. :smile: --- .../ApolloServerPluginOperationRegistry.test.ts | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) 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 8dbcd62a138..5fb875a2156 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 @@ -51,6 +51,9 @@ describe('Operation registry plugin', () => { // These tests depend on the behavior of willUpdateManifest to update the // operation safelist describe('operation lifecycle hooks', () => { + const graphId = 'test-service'; + const apiKey = `service:${graphId}:not-an-api-key`; + describe('onUnregisterOperation', () => { it('is called when unregistered operation received', async () => { const onUnregisteredOperation: Options['onUnregisteredOperation'] = jest.fn(); @@ -58,7 +61,7 @@ describe('Operation registry plugin', () => { typeDefs, mockEntireSchema: true, engine: { - apiKey: 'server:not-a-service:not-an-apikey', + apiKey, }, plugins: [ plugin({ @@ -104,7 +107,7 @@ describe('Operation registry plugin', () => { typeDefs, mockEntireSchema: true, engine: { - apiKey: 'server:not-a-service:not-an-apikey', + apiKey, }, plugins: [ plugin({ @@ -147,7 +150,7 @@ describe('Operation registry plugin', () => { typeDefs, mockEntireSchema: true, engine: { - apiKey: 'server:not-a-service:not-an-apikey', + apiKey, }, plugins: [ plugin({ @@ -201,7 +204,7 @@ describe('Operation registry plugin', () => { typeDefs, mockEntireSchema: true, engine: { - apiKey: 'server:not-a-service:not-an-apikey', + apiKey, }, plugins: [ plugin({ @@ -235,7 +238,7 @@ describe('Operation registry plugin', () => { typeDefs, mockEntireSchema: true, engine: { - apiKey: 'server:not-a-service:not-an-apikey', + apiKey, }, plugins: [ plugin({ From 4246aec5acbb6f218081074c11df80ec55916a7c Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Mon, 25 Nov 2019 20:31:16 +0200 Subject: [PATCH 28/36] To allow tests to shutdown quickly, use `sendReportsImmediately`. Since the operation registry is now using the `engine` options from the `ApolloServer` constructor in order to get its API key, we are effectively instantiating Engine Reporting and its agent. Since we're running in a test environment, those transmissions are fake, but without setting this option, they are still trying to be batch transmitted at a preset interval. In the Jest test environment, this just causes the tests to not finish as quickly as Jest expects them to, and it spews warnings about open file handles. This should make that slightly better, though it would be ideal if we never actually instantiated engine agent for reporting purposes (though we want it for `apiKeyHash` receiving purposes. --- .../__tests__/ApolloServerPluginOperationRegistry.test.ts | 5 +++++ 1 file changed, 5 insertions(+) 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 5fb875a2156..de3596d8454 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 @@ -62,6 +62,7 @@ describe('Operation registry plugin', () => { mockEntireSchema: true, engine: { apiKey, + sendReportsImmediately: true, }, plugins: [ plugin({ @@ -108,6 +109,7 @@ describe('Operation registry plugin', () => { mockEntireSchema: true, engine: { apiKey, + sendReportsImmediately: true, }, plugins: [ plugin({ @@ -151,6 +153,7 @@ describe('Operation registry plugin', () => { mockEntireSchema: true, engine: { apiKey, + sendReportsImmediately: true, }, plugins: [ plugin({ @@ -205,6 +208,7 @@ describe('Operation registry plugin', () => { mockEntireSchema: true, engine: { apiKey, + sendReportsImmediately: true, }, plugins: [ plugin({ @@ -239,6 +243,7 @@ describe('Operation registry plugin', () => { mockEntireSchema: true, engine: { apiKey, + sendReportsImmediately: true, }, plugins: [ plugin({ From a445a3574601a2bbadc41102de9e44829dce8249 Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Mon, 25 Nov 2019 20:31:20 +0200 Subject: [PATCH 29/36] De-compose manifest mocking functions into a test helper for (soon!) re-use. In a follow-up commit, we'll introduce the manifest fetching to the `ApolloServerPluginOperationRegistry.test.ts` file so it can mock the states of manifests for testing. This allows that file to exercise tests related to observability functions like `onUnregisteredOperation` and `onForbiddenOperation` in way that doesn't rely on mixing concerns from `willUpdateManifest`. Avoiding `willUpdateManifest` is particularly important since we're not completely in agreement as to how that method should work as the current implementation would expose our internal manifest format to the implementing user in a less-than-ideal way. It's possible that some other internal test methods could be introduced that allow us to control the state of the internal manifest dynamically (but in a controlled fashion; e.g. `operationAllow('query Name { field }')`), but until we build that, it seems reasonable to test different manifest conditions in the same way across our different test files for this plugin (i.e. `agent.test.ts` and `ApolloServerPluginOperationRegistry.test.ts`). --- .../src/__tests__/agent.test.ts | 302 ++++++++++-------- .../src/__tests__/helpers.test-helpers.ts | 119 +++++++ .../src/common.ts | 2 + 3 files changed, 282 insertions(+), 141 deletions(-) create mode 100644 packages/apollo-server-plugin-operation-registry/src/__tests__/helpers.test-helpers.ts diff --git a/packages/apollo-server-plugin-operation-registry/src/__tests__/agent.test.ts b/packages/apollo-server-plugin-operation-registry/src/__tests__/agent.test.ts index b6534655905..c0a56b90217 100644 --- a/packages/apollo-server-plugin-operation-registry/src/__tests__/agent.test.ts +++ b/packages/apollo-server-plugin-operation-registry/src/__tests__/agent.test.ts @@ -4,32 +4,26 @@ import { envOverrideOperationManifest, envOverrideStorageSecretBaseUrl, getStoreKey, + fakeTestBaseUrl, } from '../common'; import { resolve as urlResolve } from 'url'; -import { createHash } from 'crypto'; import { AgentOptions, OperationManifest } from '../agent'; - -const fakeBaseUrl = 'https://myfakehost/'; - -const defaultStore = () => new InMemoryLRUCache(); - -const genericSchemaHash = 'abc123'; -const genericStorageSecret = 'someStorageSecret'; -const genericServiceID = 'test-service'; -const genericApiKeyHash = 'someapikeyhash123'; -const pollSeconds = 60; -const genericLegacyOperationManifestUrl = pathForServiceAndSchema( +import { + defaultAgentOptions, + nockBase, + genericLegacyOperationManifestUrl, + genericStorageSecret, + genericApiKeyHash, genericServiceID, + defaultStore, + nockStorageSecretOperationManifest, + nockStorageSecret, + nockGoodManifestsUnderStorageSecret, + nockLegacyGoodManifest, genericSchemaHash, -); - -const defaultAgentOptions: AgentOptions = { - schemaHash: genericSchemaHash, - engine: { serviceID: genericServiceID, apiKeyHash: genericApiKeyHash }, - store: defaultStore(), - pollSeconds, - schemaTag: 'current', -}; + hashedServiceId, + defaultTestAgentPollSeconds, +} from './helpers.test-helpers'; interface ManifestRecord { signature: string; @@ -90,8 +84,6 @@ describe('Agent', () => { let Agent: typeof import('../agent').default; let getOperationManifestUrl: typeof import('../common').getOperationManifestUrl; let getLegacyOperationManifestUrl: typeof import('../common').getLegacyOperationManifestUrl; - let getStorageSecretUrl: typeof import('../common').getStorageSecretUrl; - let urlStorageSecretBase: string; let urlOperationManifestBase: string; let genericStorageSecretOperationManifestUrl: string; @@ -99,11 +91,11 @@ describe('Agent', () => { // Override the tests URL with the one we want to mock/nock/test. originalEnvApolloOpManifestBaseUrl = process.env[envOverrideOperationManifest]; - process.env[envOverrideOperationManifest] = fakeBaseUrl; + process.env[envOverrideOperationManifest] = fakeTestBaseUrl; originalEnvOverrideStorageSecretBaseUrl = process.env[envOverrideStorageSecretBaseUrl]; - process.env[envOverrideStorageSecretBaseUrl] = fakeBaseUrl; + process.env[envOverrideStorageSecretBaseUrl] = fakeTestBaseUrl; // Reset the modules so they're ready to be imported. jest.resetModules(); @@ -113,8 +105,6 @@ describe('Agent', () => { getOperationManifestUrl = require('../common').getOperationManifestUrl; getLegacyOperationManifestUrl = require('../common') .getLegacyOperationManifestUrl; - getStorageSecretUrl = require('../common').getStorageSecretUrl; - urlStorageSecretBase = require('../common').urlStorageSecretBase; urlOperationManifestBase = require('../common').urlOperationManifestBase; genericStorageSecretOperationManifestUrl = getOperationManifestUrl( @@ -123,51 +113,6 @@ describe('Agent', () => { ).replace(new RegExp(`^${urlOperationManifestBase}`), ''); }); - // Each nock is good for exactly one request! - function nockLegacyGoodManifest( - operations: ManifestRecord[] = [ - sampleManifestRecords.a, - sampleManifestRecords.b, - sampleManifestRecords.c, - ], - ) { - return nockBase() - .get(genericLegacyOperationManifestUrl) - .reply(200, { - version: 2, - operations, - }); - } - - function nockGoodManifestsUnderStorageSecret( - operations: ManifestRecord[] = [ - sampleManifestRecords.a, - sampleManifestRecords.b, - sampleManifestRecords.c, - ], - ) { - return nockBase() - .get(genericStorageSecretOperationManifestUrl) - .reply(200, { - version: 2, - operations, - }); - } - - function nockStorageSecret( - status = 200, - body: any = JSON.stringify(genericStorageSecret), - ) { - return nockBase() - .get( - getStorageSecretUrl(genericServiceID, genericApiKeyHash).replace( - new RegExp(`^${urlStorageSecretBase}`), - '', - ), - ) - .reply(status, body); - } - afterAll(() => { // Put the environment overrides back how they were. if (originalEnvApolloOpManifestBaseUrl) { @@ -231,7 +176,7 @@ describe('Agent', () => { it('correctly prepared the test environment', () => { expect(getLegacyOperationManifestUrl('abc123', 'def456')).toStrictEqual( - urlResolve(fakeBaseUrl, '/abc123/def456.v2.json'), + urlResolve(fakeTestBaseUrl, '/abc123/def456.v2.json'), ); }); @@ -259,11 +204,17 @@ describe('Agent', () => { } it('logs debug updates to the manifest on startup', async () => { - nockStorageSecret(); - nockBase() - .get(genericStorageSecretOperationManifestUrl) - .reply(404); - nockLegacyGoodManifest(); + nockStorageSecret(genericServiceID, genericApiKeyHash); + nockStorageSecretOperationManifest( + genericServiceID, + genericStorageSecret, + 404, + ); + nockLegacyGoodManifest([ + sampleManifestRecords.a, + sampleManifestRecords.b, + sampleManifestRecords.c, + ]); const relevantLogs: any = []; const logger = { debug: jest.fn().mockImplementation((...args: any[]) => { @@ -289,14 +240,14 @@ describe('Agent', () => { expect(relevantLogs[0][0]).toBe( `Checking for manifest changes at ${urlResolve( - fakeBaseUrl, + fakeTestBaseUrl, getOperationManifestUrl(genericServiceID, genericStorageSecret), )}`, ); expect(relevantLogs[1][0]).toBe( `Checking for manifest changes at ${urlResolve( - fakeBaseUrl, + fakeTestBaseUrl, getLegacyOperationManifestUrl( hashedServiceId(genericServiceID), genericSchemaHash, @@ -321,8 +272,16 @@ describe('Agent', () => { }); it('populates the manifest store after starting', async () => { - nockStorageSecret(); - nockGoodManifestsUnderStorageSecret(); + nockStorageSecret(genericServiceID, genericApiKeyHash); + nockGoodManifestsUnderStorageSecret( + genericServiceID, + genericStorageSecret, + [ + sampleManifestRecords.a, + sampleManifestRecords.b, + sampleManifestRecords.c, + ], + ); const store = defaultStore(); const storeSetSpy = jest.spyOn(store, 'set'); await createAgent({ store }).start(); @@ -336,8 +295,16 @@ describe('Agent', () => { }); it('starts polling successfully', async () => { - nockStorageSecret(); - nockGoodManifestsUnderStorageSecret(); + nockStorageSecret(genericServiceID, genericApiKeyHash); + nockGoodManifestsUnderStorageSecret( + genericServiceID, + genericStorageSecret, + [ + sampleManifestRecords.a, + sampleManifestRecords.b, + sampleManifestRecords.c, + ], + ); const store = defaultStore(); const storeSetSpy = jest.spyOn(store, 'set'); const storeDeleteSpy = jest.spyOn(store, 'delete'); @@ -354,16 +321,18 @@ describe('Agent', () => { // If it's one millisecond short of our next poll interval, nothing // should have changed yet. - jest.advanceTimersByTime(pollSeconds * 1000 - 1); + jest.advanceTimersByTime(defaultTestAgentPollSeconds * 1000 - 1); // Still only one check. expect(agent._timesChecked).toBe(1); // Now, we'll expect another request to go out, so we'll nock it. - nockStorageSecret(); - nockBase() - .get(genericStorageSecretOperationManifestUrl) - .reply(304); + nockStorageSecret(genericServiceID, genericApiKeyHash); + nockStorageSecretOperationManifest( + genericServiceID, + genericStorageSecret, + 304, + ); // If we move forward the last remaining millisecond, we should trigger // and end up with a successful sync. @@ -383,10 +352,12 @@ describe('Agent', () => { }); it('continues polling even after initial failure', async () => { - nockStorageSecret(); - nockBase() - .get(genericStorageSecretOperationManifestUrl) - .reply(500); + nockStorageSecret(genericServiceID, genericApiKeyHash); + nockStorageSecretOperationManifest( + genericServiceID, + genericStorageSecret, + 500, + ); const store = defaultStore(); const storeSetSpy = jest.spyOn(store, 'set'); const storeDeleteSpy = jest.spyOn(store, 'delete'); @@ -402,15 +373,23 @@ describe('Agent', () => { // If it's one millisecond short of our next poll interval, nothing // should have changed yet. - jest.advanceTimersByTime(pollSeconds * 1000 - 1); + jest.advanceTimersByTime(defaultTestAgentPollSeconds * 1000 - 1); // Still only one check. expect(agent._timesChecked).toBe(1); expect(storeSetSpy).toBeCalledTimes(0); // Now, we'll expect another GOOD request to fulfill, so we'll nock it. - nockStorageSecret(); - nockGoodManifestsUnderStorageSecret(); + nockStorageSecret(genericServiceID, genericApiKeyHash); + nockGoodManifestsUnderStorageSecret( + genericServiceID, + genericStorageSecret, + [ + sampleManifestRecords.a, + sampleManifestRecords.b, + sampleManifestRecords.c, + ], + ); // If we move forward the last remaining millisecond, we should trigger // and end up with a successful sync. @@ -435,19 +414,28 @@ describe('Agent', () => { const agent = createAgent({ store }); expect(storeSetSpy).toBeCalledTimes(0); - nockStorageSecret(); - nockGoodManifestsUnderStorageSecret(); // Starting with ABC. + nockStorageSecret(genericServiceID, genericApiKeyHash); + nockGoodManifestsUnderStorageSecret( + genericServiceID, + genericStorageSecret, + [ + sampleManifestRecords.a, + sampleManifestRecords.b, + sampleManifestRecords.c, + ], + ); // Starting with ABC. await agent.checkForUpdate(); expect(agent._timesChecked).toBe(1); expect(storeSetSpy).toBeCalledTimes(3); expect(storeDeleteSpy).toBeCalledTimes(0); await expectStoreHasOperationEach(store, ['a', 'b', 'c']); - nockStorageSecret(); - nockGoodManifestsUnderStorageSecret([ - sampleManifestRecords.a, - sampleManifestRecords.b, - ]); // Just AB in this manifest. + nockStorageSecret(genericServiceID, genericApiKeyHash) + nockGoodManifestsUnderStorageSecret( + genericServiceID, + genericStorageSecret, + [sampleManifestRecords.a, sampleManifestRecords.b], + ); // Just AB in this manifest. await agent.checkForUpdate(); expect(agent._timesChecked).toBe(2); expect(storeSetSpy).toBeCalledTimes(3); // no new sets. @@ -457,8 +445,12 @@ describe('Agent', () => { store.get(getStoreKey(sampleManifestRecords.c.signature)), ).resolves.toBeUndefined(); - nockStorageSecret(); - nockGoodManifestsUnderStorageSecret([sampleManifestRecords.a]); // Just A in this manifest. + nockStorageSecret(genericServiceID, genericApiKeyHash) + nockGoodManifestsUnderStorageSecret( + genericServiceID, + genericStorageSecret, + [sampleManifestRecords.a] + ); // Just A in this manifest. await agent.checkForUpdate(); expect(agent._timesChecked).toBe(3); expect(storeSetSpy).toBeCalledTimes(3); // no new sets. @@ -471,8 +463,12 @@ describe('Agent', () => { describe('when fetching the storage secret fails', () => { it('will fetch the manifest using the legacy url', async () => { - nockStorageSecret(404); - nockLegacyGoodManifest(); + nockStorageSecret(genericServiceID, genericApiKeyHash, 404); + nockLegacyGoodManifest([ + sampleManifestRecords.a, + sampleManifestRecords.b, + sampleManifestRecords.c, + ]); const store = defaultStore(); const storeSetSpy = jest.spyOn(store, 'set'); @@ -491,11 +487,13 @@ describe('Agent', () => { describe('when fetching the manifest using the storage secret fails', () => { it('will fallback to fetching the manifest using the legacy url', async () => { - nockStorageSecret(); - nockBase() - .get(genericStorageSecretOperationManifestUrl) - .reply(404); - nockLegacyGoodManifest(); + nockStorageSecret(genericServiceID, genericApiKeyHash); + nockStorageSecretOperationManifest(genericServiceID, genericStorageSecret, 404) + nockLegacyGoodManifest([ + sampleManifestRecords.a, + sampleManifestRecords.b, + sampleManifestRecords.c, + ]); const store = defaultStore(); const storeSetSpy = jest.spyOn(store, 'set'); @@ -523,7 +521,7 @@ describe('Agent', () => { ); it('fetches manifests for the corresponding schema tag', async () => { - nockStorageSecret(); + nockStorageSecret(genericServiceID, genericApiKeyHash); const agent = createAgent({ schemaTag }); const nockedManifest = nockBase() .get( @@ -560,7 +558,7 @@ describe('Agent', () => { describe('willUpdateManifest', () => { it("receives undefined arguments when can't fetch first time", async () => { - nockStorageSecret(404); + nockStorageSecret(genericServiceID, genericApiKeyHash, 404); nockBase() .get(genericLegacyOperationManifestUrl) .reply(500); @@ -582,8 +580,16 @@ describe('Agent', () => { expect(storeDeleteSpy).toBeCalledTimes(0); }); it('receives manifest retrieved from gcs', async () => { - nockStorageSecret(); - nockGoodManifestsUnderStorageSecret(); + nockStorageSecret(genericServiceID, genericApiKeyHash); + nockGoodManifestsUnderStorageSecret( + genericServiceID, + genericStorageSecret, + [ + sampleManifestRecords.a, + sampleManifestRecords.b, + sampleManifestRecords.c, + ] + ); const mock = generateWillUpdateManifest(); const store = defaultStore(); const storeSetSpy = jest.spyOn(store, 'set'); @@ -612,8 +618,16 @@ describe('Agent', () => { expect(storeDeleteSpy).toBeCalledTimes(0); }); it('uses returned manifest to enforce safelist', async () => { - nockStorageSecret(); - nockGoodManifestsUnderStorageSecret(); + nockStorageSecret(genericServiceID, genericApiKeyHash); + nockGoodManifestsUnderStorageSecret( + genericServiceID, + genericStorageSecret, + [ + sampleManifestRecords.a, + sampleManifestRecords.b, + sampleManifestRecords.c, + ] + ); const mock = generateWillUpdateManifest([sampleManifestRecords.a]); const store = defaultStore(); const storeSetSpy = jest.spyOn(store, 'set'); @@ -646,8 +660,16 @@ describe('Agent', () => { expect(storeDeleteSpy).toBeCalledTimes(0); }); it('is not called again when manifest remains same', async () => { - nockStorageSecret(); - nockGoodManifestsUnderStorageSecret(); + nockStorageSecret(genericServiceID, genericApiKeyHash); + nockGoodManifestsUnderStorageSecret( + genericServiceID, + genericStorageSecret, + [ + sampleManifestRecords.a, + sampleManifestRecords.b, + sampleManifestRecords.c, + ] + ); const mock = generateWillUpdateManifest(); const store = defaultStore(); @@ -678,13 +700,13 @@ describe('Agent', () => { // If it's one millisecond short of our next poll interval, nothing // should have changed yet. - jest.advanceTimersByTime(pollSeconds * 1000 - 1); + jest.advanceTimersByTime(defaultTestAgentPollSeconds * 1000 - 1); // Still only one check. expect(agent._timesChecked).toBe(1); // Now, we'll expect another request to go out, so we'll nock it. - nockStorageSecret(); + nockStorageSecret(genericServiceID, genericApiKeyHash); nockBase() .get(genericStorageSecretOperationManifestUrl) .reply(304); @@ -719,8 +741,16 @@ describe('Agent', () => { }); it('receives previous manifest when updated', async () => { - nockStorageSecret(); - nockGoodManifestsUnderStorageSecret(); + nockStorageSecret(genericServiceID, genericApiKeyHash); + nockGoodManifestsUnderStorageSecret( + genericServiceID, + genericStorageSecret, + [ + sampleManifestRecords.a, + sampleManifestRecords.b, + sampleManifestRecords.c, + ] + ); const mock = generateWillUpdateManifest(); const store = defaultStore(); @@ -751,14 +781,18 @@ describe('Agent', () => { // If it's one millisecond short of our next poll interval, nothing // should have changed yet. - jest.advanceTimersByTime(pollSeconds * 1000 - 1); + jest.advanceTimersByTime(defaultTestAgentPollSeconds * 1000 - 1); // Still only one check. expect(agent._timesChecked).toBe(1); // Now, we'll expect another request to go out, so we'll nock it. - nockStorageSecret(); - nockGoodManifestsUnderStorageSecret([sampleManifestRecords.a]); + nockStorageSecret(genericServiceID, genericApiKeyHash); + nockGoodManifestsUnderStorageSecret( + genericServiceID, + genericStorageSecret, + [sampleManifestRecords.a], + ); // If we move forward the last remaining millisecond, we should trigger // and end up with a successful sync. @@ -788,17 +822,3 @@ describe('Agent', () => { }); }); }); - -function nockBase() { - return nock(fakeBaseUrl); -} - -function hashedServiceId(serviceID: string) { - return createHash('sha512') - .update(serviceID) - .digest('hex'); -} - -function pathForServiceAndSchema(serviceID: string, schemaHash: string) { - return `/${hashedServiceId(serviceID)}/${schemaHash}.v2.json`; -} diff --git a/packages/apollo-server-plugin-operation-registry/src/__tests__/helpers.test-helpers.ts b/packages/apollo-server-plugin-operation-registry/src/__tests__/helpers.test-helpers.ts new file mode 100644 index 00000000000..e75d1a8a70e --- /dev/null +++ b/packages/apollo-server-plugin-operation-registry/src/__tests__/helpers.test-helpers.ts @@ -0,0 +1,119 @@ +import { createHash } from 'crypto'; +import { AgentOptions } from "../agent"; +import { + getStorageSecretUrl, + urlStorageSecretBase, + getOperationManifestUrl, + urlOperationManifestBase, + fakeTestBaseUrl, +} from '../common'; +import nock from 'nock'; +import { InMemoryLRUCache } from "apollo-server-caching"; +import { Operation, OperationManifest } from "../ApolloServerPluginOperationRegistry"; + +export const defaultStore = () => new InMemoryLRUCache(); + +export const genericSchemaHash = 'abc123'; +export const genericStorageSecret = 'someStorageSecret'; +export const genericServiceID = 'test-service'; +export const genericApiKeyHash = 'someapikeyhash123'; +export const defaultTestAgentPollSeconds = 60; +export const genericLegacyOperationManifestUrl = pathForServiceAndSchema( + genericServiceID, + genericSchemaHash, +); + +export const defaultAgentOptions: AgentOptions = { + schemaHash: genericSchemaHash, + engine: { serviceID: genericServiceID, apiKeyHash: genericApiKeyHash }, + store: defaultStore(), + pollSeconds: defaultTestAgentPollSeconds, + schemaTag: 'current', +}; + +// Each nock is good for exactly one request! +export function nockLegacyGoodManifest(operations: Operation[]): nock.Scope { + return nockBase() + .get(genericLegacyOperationManifestUrl) + .reply(200, { + version: 2, + operations, + }); +} + +export function nockGoodManifestsUnderStorageSecret( + graphId: string, + storageSecret: string, + operations: Operation[], +): nock.Scope { + return nockStorageSecretOperationManifest(graphId, storageSecret, 200, { + version: 2, + operations, + }); +} + +export function getStorageSecretPath( + graphId: string, + apiKeyHash: string, +) { + return getStorageSecretUrl(graphId, apiKeyHash).replace( + new RegExp(`^${urlStorageSecretBase}`), + '', + ); +} + +export function nockStorageSecret( + graphId: string, + apiKeyHash: string, + status = 200, + body: string = JSON.stringify(genericStorageSecret), +) { + // Strip off the host for testing purposes with `nock`. + return nockBase() + .get(getStorageSecretPath(graphId, apiKeyHash)) + .reply(status, body); +} + +export function getOperationManifestPath( + graphId: string, + storageSecret: string, +): string { + // Strip off the host for testing purposes with `nock`. + return getOperationManifestUrl( + graphId, + storageSecret, + ).replace(new RegExp(`^${urlOperationManifestBase}`), ''); +} + +export function nockStorageSecretOperationManifest( + graphId: string, + storageSecret: string, + status = 200, + body?: OperationManifest, +) { + return nockBase() + .get(getOperationManifestPath(graphId, storageSecret)) + .reply(status, body); +} + + +export function nockBase() { + return nock(fakeTestBaseUrl); +} + +export function hashApiKey(apiKey: string): string { + return createHash('sha512') + .update(apiKey) + .digest('hex'); +} + +export function hashedServiceId(serviceID: string): string { + return createHash('sha512') + .update(serviceID) + .digest('hex'); +} + +function pathForServiceAndSchema(serviceID: string, schemaHash: string): string { + return `/${hashedServiceId(serviceID)}/${schemaHash}.v2.json`; +} + diff --git a/packages/apollo-server-plugin-operation-registry/src/common.ts b/packages/apollo-server-plugin-operation-registry/src/common.ts index 371834ad399..436c2b1001b 100644 --- a/packages/apollo-server-plugin-operation-registry/src/common.ts +++ b/packages/apollo-server-plugin-operation-registry/src/common.ts @@ -6,6 +6,8 @@ export const envOverrideOperationManifest = export const envOverrideStorageSecretBaseUrl = 'APOLLO_STORAGE_SECRET_BASE_URL'; +export const fakeTestBaseUrl = 'https://myfakehost/'; + // Generate and cache our desired operation manifest URL. export const urlOperationManifestBase: string = ((): string => { const desiredUrl = From 93cc00ba4debf7991fd0a58fea60db1fa254112e Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Mon, 25 Nov 2019 20:31:24 +0200 Subject: [PATCH 30/36] Move Operation and OperationManifest to plugin file. --- .../src/ApolloServerPluginOperationRegistry.ts | 12 +++++++++++- .../src/__tests__/agent.test.ts | 15 +++++++-------- .../src/agent.ts | 11 +---------- 3 files changed, 19 insertions(+), 19 deletions(-) diff --git a/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts b/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts index 0bb1ee27be8..df1caa71a20 100644 --- a/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts +++ b/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts @@ -18,7 +18,7 @@ import { defaultOperationRegistrySignature as defaultOperationRegistryNormalization, } from 'apollo-graphql'; import { ForbiddenError, ApolloError } from 'apollo-server-errors'; -import Agent, { OperationManifest } from './agent'; +import Agent from './agent'; import { GraphQLSchema } from 'graphql/type'; import { InMemoryLRUCache } from 'apollo-server-caching'; import loglevel from 'loglevel'; @@ -34,6 +34,16 @@ export interface OperationRegistryRequestContext { normalizedDocument: string; } +export interface Operation { + signature: string; + document: string; +} + +export interface OperationManifest { + version: number; + operations: Array; +} + export interface Options { debug?: boolean; forbidUnregisteredOperations?: diff --git a/packages/apollo-server-plugin-operation-registry/src/__tests__/agent.test.ts b/packages/apollo-server-plugin-operation-registry/src/__tests__/agent.test.ts index c0a56b90217..9dc1a085070 100644 --- a/packages/apollo-server-plugin-operation-registry/src/__tests__/agent.test.ts +++ b/packages/apollo-server-plugin-operation-registry/src/__tests__/agent.test.ts @@ -7,7 +7,7 @@ import { fakeTestBaseUrl, } from '../common'; import { resolve as urlResolve } from 'url'; -import { AgentOptions, OperationManifest } from '../agent'; +import { AgentOptions } from '../agent'; import { defaultAgentOptions, nockBase, @@ -24,15 +24,14 @@ import { hashedServiceId, defaultTestAgentPollSeconds, } from './helpers.test-helpers'; - -interface ManifestRecord { - signature: string; - document: string; -} +import { + Operation, + OperationManifest, +} from "../ApolloServerPluginOperationRegistry"; // These get a bit verbose within the tests below, so we use this as a // sample store to pick and grab from. -const sampleManifestRecords: Record = { +const sampleManifestRecords: Record = { a: { signature: 'ba4573fca2e1491fd54b9f398490531ad6327b00610f2c1216dc8c9c4cfd2993', @@ -51,7 +50,7 @@ const sampleManifestRecords: Record = { }, }; -const manifest = (...operations: ManifestRecord[]) => ({ +const manifest = (...operations: Operation[]) => ({ version: 2, operations, }); diff --git a/packages/apollo-server-plugin-operation-registry/src/agent.ts b/packages/apollo-server-plugin-operation-registry/src/agent.ts index 0b12e0d5751..6f12d8edfc3 100644 --- a/packages/apollo-server-plugin-operation-registry/src/agent.ts +++ b/packages/apollo-server-plugin-operation-registry/src/agent.ts @@ -13,6 +13,7 @@ import { Response } from 'node-fetch'; import { InMemoryLRUCache } from 'apollo-server-caching'; import { fetchIfNoneMatch } from './fetchIfNoneMatch'; import { ValueOrPromise } from "apollo-server-plugin-base"; +import { OperationManifest } from "./ApolloServerPluginOperationRegistry"; const DEFAULT_POLL_SECONDS: number = 30; const SYNC_WARN_TIME_SECONDS: number = 60; @@ -30,16 +31,6 @@ export interface AgentOptions { ) => ValueOrPromise; } -export interface Operation { - signature: string; - document: string; -} - -export interface OperationManifest { - version: number; - operations: Array; -} - type SignatureStore = Set; const callToAction = `Ensure this server's schema has been published with 'apollo service:push' and that operations have been registered with 'apollo client:push'.`; From 8ee8e6c081db6c530612e8cd18bd0bcaadf9f821 Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Mon, 25 Nov 2019 20:31:32 +0200 Subject: [PATCH 31/36] De-globalize variables in test file. These variables are only used by a portion of the tests, so it seems to make sense to scope them accordingly. I had originally started introducing a block of tests which needed to declare their own `typeDefs` and was thwarted by these globals. I'm no longer sure my follow-up commit absolutely necessitates this change, but it seems best to couple tests with their variables. --- ...polloServerPluginOperationRegistry.test.ts | 51 ++++++++++--------- 1 file changed, 26 insertions(+), 25 deletions(-) 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 de3596d8454..7ca6ba220c8 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 @@ -13,31 +13,7 @@ import { } from 'apollo-graphql'; import gql from 'graphql-tag'; import { print } from 'graphql'; - -const typeDefs = gql` - type Query { - hello: String - } -`; - -const query = gql` - query HelloFam { - hello - } -`; - -const normalizedQueryDocument = defaultOperationRegistryNormalization( - query, - 'HelloFam', -); -const queryHash = operationSignature(normalizedQueryDocument); - -// In order to expose will start and -class ApolloServerMock extends ApolloServerBase { - public async willStart() { - return super.willStart(); - } -} +import { hashApiKey } from "./helpers.test-helpers"; describe('Operation registry plugin', () => { it('will instantiate when not called with options', () => { @@ -53,6 +29,31 @@ describe('Operation registry plugin', () => { describe('operation lifecycle hooks', () => { const graphId = 'test-service'; const apiKey = `service:${graphId}:not-an-api-key`; + const hashedApiKey = hashApiKey(apiKey); + const typeDefs = gql` + type Query { + hello: String + } + `; + + const query = gql` + query HelloFam { + hello + } + `; + + const normalizedQueryDocument = defaultOperationRegistryNormalization( + query, + 'HelloFam', + ); + 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 () => { From bd182421c621bbd6ba29acfaaf518bb1f77e7353 Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Mon, 25 Nov 2019 20:31:39 +0200 Subject: [PATCH 32/36] Use a more manageable method for overriding our production manifest host. The initial implementation of the host mocking, which I personally was responsible for creating, has outgrown its usefulness, particularly as a single known manifest location has grown into a multi-fetch water-fall approach for locating a server's corresponding manifest. What was once a seemingly reasonable approach for temporarily overriding the manifest host - which invoked a `jest.resetModules()` call after overriding a sole `process.env` variable (which is captured inside a memoized function to avoid excessive expensive calls to `process.env`), has now evolved to an elaborate dance of avoiding normal `import` statements. While this approach still worked, it was difficult to debug since the dependency graph for this application has also increased in complexity. This should avoid the need to continue this dance, by merely leveraging `process.env.NODE_ENV === 'test'` - a variable set automatically by Jest and (most? all?) other test runners. --- .../src/__tests__/agent.test.ts | 95 +++---------------- .../src/__tests__/common.test.ts | 2 +- .../src/common.ts | 8 +- 3 files changed, 22 insertions(+), 83 deletions(-) diff --git a/packages/apollo-server-plugin-operation-registry/src/__tests__/agent.test.ts b/packages/apollo-server-plugin-operation-registry/src/__tests__/agent.test.ts index 9dc1a085070..8f28256133b 100644 --- a/packages/apollo-server-plugin-operation-registry/src/__tests__/agent.test.ts +++ b/packages/apollo-server-plugin-operation-registry/src/__tests__/agent.test.ts @@ -1,33 +1,25 @@ import nock from 'nock'; import { InMemoryLRUCache } from 'apollo-server-caching'; -import { - envOverrideOperationManifest, - envOverrideStorageSecretBaseUrl, - getStoreKey, - fakeTestBaseUrl, -} from '../common'; import { resolve as urlResolve } from 'url'; -import { AgentOptions } from '../agent'; import { defaultAgentOptions, - nockBase, - genericLegacyOperationManifestUrl, - genericStorageSecret, - genericApiKeyHash, genericServiceID, - defaultStore, - nockStorageSecretOperationManifest, + genericStorageSecret, nockStorageSecret, - nockGoodManifestsUnderStorageSecret, + nockBase, nockLegacyGoodManifest, genericSchemaHash, hashedServiceId, + nockGoodManifestsUnderStorageSecret, + defaultStore, defaultTestAgentPollSeconds, + nockStorageSecretOperationManifest, + genericApiKeyHash, + genericLegacyOperationManifestUrl, } from './helpers.test-helpers'; -import { - Operation, - OperationManifest, -} from "../ApolloServerPluginOperationRegistry"; +import Agent, { AgentOptions } from "../agent"; +import { Operation, OperationManifest } from "../ApolloServerPluginOperationRegistry"; +import { fakeTestBaseUrl, getLegacyOperationManifestUrl, getStoreKey, getOperationManifestUrl, urlOperationManifestBase } from "../common"; // These get a bit verbose within the tests below, so we use this as a // sample store to pick and grab from. @@ -78,65 +70,6 @@ describe('Agent', () => { }); describe('with manifest', () => { - let originalEnvApolloOpManifestBaseUrl: string | undefined; - let originalEnvOverrideStorageSecretBaseUrl: string | undefined; - let Agent: typeof import('../agent').default; - let getOperationManifestUrl: typeof import('../common').getOperationManifestUrl; - let getLegacyOperationManifestUrl: typeof import('../common').getLegacyOperationManifestUrl; - let urlOperationManifestBase: string; - let genericStorageSecretOperationManifestUrl: string; - - beforeAll(() => { - // Override the tests URL with the one we want to mock/nock/test. - originalEnvApolloOpManifestBaseUrl = - process.env[envOverrideOperationManifest]; - process.env[envOverrideOperationManifest] = fakeTestBaseUrl; - - originalEnvOverrideStorageSecretBaseUrl = - process.env[envOverrideStorageSecretBaseUrl]; - process.env[envOverrideStorageSecretBaseUrl] = fakeTestBaseUrl; - - // Reset the modules so they're ready to be imported. - jest.resetModules(); - - // Store these on the local scope after we've reset the modules. - Agent = require('../agent').default; - getOperationManifestUrl = require('../common').getOperationManifestUrl; - getLegacyOperationManifestUrl = require('../common') - .getLegacyOperationManifestUrl; - urlOperationManifestBase = require('../common').urlOperationManifestBase; - - genericStorageSecretOperationManifestUrl = getOperationManifestUrl( - genericServiceID, - genericStorageSecret, - ).replace(new RegExp(`^${urlOperationManifestBase}`), ''); - }); - - afterAll(() => { - // Put the environment overrides back how they were. - if (originalEnvApolloOpManifestBaseUrl) { - process.env[ - envOverrideOperationManifest - ] = originalEnvApolloOpManifestBaseUrl; - - originalEnvApolloOpManifestBaseUrl = undefined; - } else { - delete process.env[envOverrideOperationManifest]; - } - if (originalEnvOverrideStorageSecretBaseUrl) { - process.env[ - envOverrideStorageSecretBaseUrl - ] = originalEnvOverrideStorageSecretBaseUrl; - - originalEnvOverrideStorageSecretBaseUrl = undefined; - } else { - delete process.env[envOverrideStorageSecretBaseUrl]; - } - - // Reset modules again. - jest.resetModules(); - }); - const forCleanup: { store?: InMemoryLRUCache; agent?: import('../agent').default; @@ -706,9 +639,11 @@ describe('Agent', () => { // Now, we'll expect another request to go out, so we'll nock it. nockStorageSecret(genericServiceID, genericApiKeyHash); - nockBase() - .get(genericStorageSecretOperationManifestUrl) - .reply(304); + nockStorageSecretOperationManifest( + genericServiceID, + genericStorageSecret, + 304, + ); // If we move forward the last remaining millisecond, we should trigger // and end up with a successful sync. diff --git a/packages/apollo-server-plugin-operation-registry/src/__tests__/common.test.ts b/packages/apollo-server-plugin-operation-registry/src/__tests__/common.test.ts index 72312883ccd..e1b77154327 100644 --- a/packages/apollo-server-plugin-operation-registry/src/__tests__/common.test.ts +++ b/packages/apollo-server-plugin-operation-registry/src/__tests__/common.test.ts @@ -9,7 +9,7 @@ describe('common', () => { expect( common.getLegacyOperationManifestUrl('aServiceId', 'aSchemaHash'), ).toMatchInlineSnapshot( - `"https://storage.googleapis.com/engine-op-manifest-storage-prod/aServiceId/aSchemaHash.v2.json"`, + `"https://myfakehost/aServiceId/aSchemaHash.v2.json"`, ); }); }); diff --git a/packages/apollo-server-plugin-operation-registry/src/common.ts b/packages/apollo-server-plugin-operation-registry/src/common.ts index 436c2b1001b..febb52f58e7 100644 --- a/packages/apollo-server-plugin-operation-registry/src/common.ts +++ b/packages/apollo-server-plugin-operation-registry/src/common.ts @@ -12,7 +12,9 @@ export const fakeTestBaseUrl = 'https://myfakehost/'; export const urlOperationManifestBase: string = ((): string => { const desiredUrl = process.env[envOverrideOperationManifest] || - 'https://storage.googleapis.com/engine-op-manifest-storage-prod/'; + process.env['NODE_ENV'] === 'test' + ? fakeTestBaseUrl + : 'https://storage.googleapis.com/engine-op-manifest-storage-prod/'; // Make sure it has NO trailing slash. return desiredUrl.replace(/\/$/, ''); @@ -22,7 +24,9 @@ export const urlOperationManifestBase: string = ((): string => { export const urlStorageSecretBase: string = ((): string => { const desiredUrl = process.env[envOverrideStorageSecretBaseUrl] || - 'https://storage.googleapis.com/engine-partial-schema-prod/'; + process.env['NODE_ENV'] === 'test' + ? fakeTestBaseUrl + : 'https://storage.googleapis.com/engine-partial-schema-prod/'; // Make sure it has NO trailing slash. return desiredUrl.replace(/\/$/, ''); From c86a01266e0d2a808d5de06d55b9c25fd3defda6 Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Mon, 25 Nov 2019 20:31:43 +0200 Subject: [PATCH 33/36] Rejigger tests to not use `willUpdateManifest`. As allured to in 4c70889bb2dde0ba4f884297a7396f95d60adc2d, this changes the tests to not leverage `willUpdateManifest`, which is a slightly contested implementation that we're not certain will stay around. Instead, this leverages the same mechanism that `agent.test.ts` uses which was enabled by the changes in the aforementioned commit, without mixing concerns. And as a recap, here's more words from my other commit message which probably belonged here anyway: It's possible that some other internal test methods could be introduced that allow us to control the state of the internal manifest dynamically (but in a controlled fashion; e.g. `operationAllow('query Name { field }')`), but until we build that, it seems reasonable to test different manifest conditions in the same way across our different test files for this plugin (i.e. `agent.test.ts` and `ApolloServerPluginOperationRegistry.test.ts`). --- ...polloServerPluginOperationRegistry.test.ts | 88 ++++++++++--------- 1 file changed, 46 insertions(+), 42 deletions(-) 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 7ca6ba220c8..842663349a2 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 @@ -13,7 +13,12 @@ import { } from 'apollo-graphql'; import gql from 'graphql-tag'; import { print } from 'graphql'; -import { hashApiKey } from "./helpers.test-helpers"; +import { + hashApiKey, + nockStorageSecret, + nockGoodManifestsUnderStorageSecret, + genericStorageSecret, +} from './helpers.test-helpers'; describe('Operation registry plugin', () => { it('will instantiate when not called with options', () => { @@ -58,6 +63,12 @@ describe('Operation registry plugin', () => { describe('onUnregisterOperation', () => { it('is called when unregistered operation received', async () => { const onUnregisteredOperation: Options['onUnregisteredOperation'] = jest.fn(); + nockStorageSecret(graphId, hashedApiKey); + nockGoodManifestsUnderStorageSecret( + graphId, + genericStorageSecret, + [ /* Intentionally empty! */ ], + ); const server = new ApolloServerMock({ typeDefs, mockEntireSchema: true, @@ -67,12 +78,6 @@ describe('Operation registry plugin', () => { }, plugins: [ plugin({ - willUpdateManifest: () => { - return { - version: 2, - operations: [], - }; - }, onUnregisteredOperation, })(), ], @@ -105,6 +110,17 @@ describe('Operation registry plugin', () => { it('is not called when registered operation received', async () => { const onUnregisteredOperation: Options['onUnregisteredOperation'] = jest.fn(); + nockStorageSecret(graphId, hashedApiKey); + nockGoodManifestsUnderStorageSecret( + graphId, + genericStorageSecret, + [ + { + document: normalizedQueryDocument, + signature: queryHash, + }, + ], + ); const server = new ApolloServerMock({ typeDefs, mockEntireSchema: true, @@ -114,17 +130,6 @@ describe('Operation registry plugin', () => { }, plugins: [ plugin({ - willUpdateManifest: () => { - return { - version: 2, - operations: [ - { - document: normalizedQueryDocument, - signature: queryHash, - }, - ], - }; - }, onUnregisteredOperation, })(), ], @@ -148,7 +153,12 @@ describe('Operation registry plugin', () => { // Returning true from this predicate enables the enforcement. const forbidUnregisteredOperations = jest.fn(() => true); - + nockStorageSecret(graphId, hashedApiKey); + nockGoodManifestsUnderStorageSecret( + graphId, + genericStorageSecret, + [ /* Intentionally empty! */ ], + ); const server = new ApolloServerMock({ typeDefs, mockEntireSchema: true, @@ -158,12 +168,6 @@ describe('Operation registry plugin', () => { }, plugins: [ plugin({ - willUpdateManifest: () => { - return { - version: 2, - operations: [], - }; - }, forbidUnregisteredOperations, onForbiddenOperation, })(), @@ -204,6 +208,12 @@ describe('Operation registry plugin', () => { // Returning true from this predicate enables the enforcement. const forbidUnregisteredOperations = jest.fn(() => false); + nockStorageSecret(graphId, hashedApiKey); + nockGoodManifestsUnderStorageSecret( + graphId, + genericStorageSecret, + [ /* Intentionally empty! */ ], + ); const server = new ApolloServerMock({ typeDefs, mockEntireSchema: true, @@ -213,12 +223,6 @@ describe('Operation registry plugin', () => { }, plugins: [ plugin({ - willUpdateManifest: () => { - return { - version: 2, - operations: [], - }; - }, forbidUnregisteredOperations, onForbiddenOperation, })(), @@ -239,6 +243,17 @@ describe('Operation registry plugin', () => { it('is not called when registered operation received', async () => { const onForbiddenOperation = jest.fn(); + nockStorageSecret(graphId, hashedApiKey); + nockGoodManifestsUnderStorageSecret( + graphId, + genericStorageSecret, + [ + { + document: normalizedQueryDocument, + signature: queryHash, + }, + ], + ); const server = new ApolloServerMock({ typeDefs, mockEntireSchema: true, @@ -248,17 +263,6 @@ describe('Operation registry plugin', () => { }, plugins: [ plugin({ - willUpdateManifest: () => { - return { - version: 2, - operations: [ - { - document: normalizedQueryDocument, - signature: queryHash, - }, - ], - }; - }, onForbiddenOperation, })(), ], From d3808c04b03377801838965b2c7635320b113d37 Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Mon, 25 Nov 2019 20:31:55 +0200 Subject: [PATCH 34/36] Remove newly introduced `willUpdateManifest` method, for now. Removes the `willUpdateManifest` method which was originally introduced in https://github.com/apollographql/apollo-platform-commercial/pull/158, until we can come to a better agreement as to how it should work. While the idea of this method was to give user's control over what to do in the case of an unavailable manifest, the implementation exposes the exact format of the manifest to the user, preventing us from iterating on it. It also leaves the exercise of generating and updating the manifest to the user, which honestly, if documented using this approach would potentially introduce many additional complexities to deployment processes and severely undermine the value of the feature. Some things which we may want to introduce first are: - The ability to fail-safe or fail-secure on server startup when the manifest is not available. - The ability to fail-safe or fail-secure after a specified interval of manifest unavailability. For example, when the manifest cannot be fetched for a specific time, switch to a particular mode of operation. - A CDN for our manifests to reduce the likelihood of the manifest being unavailable. - Since this extends to other artifacts which are hosted in our cloud, potentially, a more blanket recommendation that consumers setup an on-premise artifact cache and potentially use a Varnish (or other) proxy as an in-between store for artifacts that are hosted in our cloud. As it stands right now though, there is a lack of agreement on a best practice here that doesn't heavily rely on being locked into our manifest format. Further, as far as I'm aware, nobody is actually implementing this method and no customer (other than "us") has specifically asked for this ability (even if we _think_ it might be what they need). --- .../CHANGELOG.md | 4 +- .../ApolloServerPluginOperationRegistry.ts | 6 - .../src/__tests__/agent.test.ts | 285 ------------------ .../src/agent.ts | 42 +-- 4 files changed, 8 insertions(+), 329 deletions(-) diff --git a/packages/apollo-server-plugin-operation-registry/CHANGELOG.md b/packages/apollo-server-plugin-operation-registry/CHANGELOG.md index f738f4d0998..13b86df661f 100644 --- a/packages/apollo-server-plugin-operation-registry/CHANGELOG.md +++ b/packages/apollo-server-plugin-operation-registry/CHANGELOG.md @@ -2,9 +2,9 @@ ### vNEXT (Currently `alpha` tag) -- Add lifecycle hooks: `willUpdateManifest`, `onUnregisteredOperation`, and `onForbiddenOperation`. [PR #158](https://github.com/apollographql/apollo-platform-commercial/pull/158) +- Add lifecycle hooks: `onUnregisteredOperation`, and `onForbiddenOperation`. [PR #158](https://github.com/apollographql/apollo-platform-commercial/pull/158) [PR #251](https://github.com/apollographql/apollo-platform-commercial/pull/251) [PR #TODO](https://github.com/apollographql/apollo-platform-commercial/pull/TODO) - Prevent the polling timer from keeping the event loop active [PR #223](https://github.com/apollographql/apollo-platform-commercial/pull/223) -- Update error message for operations that are not in the operation registry [PR #170](https://github.com/apollographql/apollo-platform-commercial/pull/170) +- Update error message for operations that are not in the operation registry. [PR #170](https://github.com/apollographql/apollo-platform-commercial/pull/170) ### 0.2.2 diff --git a/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts b/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts index c5f4f5182af..31073baa97d 100644 --- a/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts +++ b/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts @@ -23,7 +23,6 @@ import { GraphQLSchema } from 'graphql/type'; import { InMemoryLRUCache } from 'apollo-server-caching'; import loglevel from 'loglevel'; import loglevelDebug from 'loglevel-debug'; -import { ValueOrPromise } from 'apollo-server-plugin-base'; type ForbidUnregisteredOperationsPredicate = ( requestContext: GraphQLRequestContext, @@ -59,10 +58,6 @@ export interface Options { requestContext: GraphQLRequestContext, operationRegistryRequestContext: OperationRegistryRequestContext, ) => void; - willUpdateManifest?: ( - newManifest?: OperationManifest, - oldManifest?: OperationManifest, - ) => ValueOrPromise; } export default function plugin(options: Options = Object.create(null)) { @@ -134,7 +129,6 @@ for observability purposes, but all operations will be permitted.`, engine, store, logger, - willUpdateManifest: options.willUpdateManifest, }); await agent.start(); diff --git a/packages/apollo-server-plugin-operation-registry/src/__tests__/agent.test.ts b/packages/apollo-server-plugin-operation-registry/src/__tests__/agent.test.ts index 8f28256133b..26e5e3b44eb 100644 --- a/packages/apollo-server-plugin-operation-registry/src/__tests__/agent.test.ts +++ b/packages/apollo-server-plugin-operation-registry/src/__tests__/agent.test.ts @@ -469,290 +469,5 @@ describe('Agent', () => { }); }); }); - - describe('manifest lifecycle', () => { - function generateWillUpdateManifest( - operations?: OperationManifest['operations'], - ) { - return jest.fn( - ( - newManifest?: OperationManifest, - oldManifest?: OperationManifest, - ) => { - const manifest = newManifest || oldManifest; - return ( - (operations && { version: 2, operations }) || - manifest || { version: 2, operations: [] } - ); - }, - ); - } - - describe('willUpdateManifest', () => { - it("receives undefined arguments when can't fetch first time", async () => { - nockStorageSecret(genericServiceID, genericApiKeyHash, 404); - nockBase() - .get(genericLegacyOperationManifestUrl) - .reply(500); - const mock = generateWillUpdateManifest(); - const store = defaultStore(); - const storeSetSpy = jest.spyOn(store, 'set'); - const storeDeleteSpy = jest.spyOn(store, 'delete'); - - const agent = createAgent({ - willUpdateManifest: mock, - store, - }); - await agent.start(); - expect(mock).toHaveBeenCalledTimes(1); - expect(mock).toHaveBeenCalledWith(undefined, undefined); - - // no additions, no deletions. - expect(storeSetSpy).toBeCalledTimes(0); - expect(storeDeleteSpy).toBeCalledTimes(0); - }); - it('receives manifest retrieved from gcs', async () => { - nockStorageSecret(genericServiceID, genericApiKeyHash); - nockGoodManifestsUnderStorageSecret( - genericServiceID, - genericStorageSecret, - [ - sampleManifestRecords.a, - sampleManifestRecords.b, - sampleManifestRecords.c, - ] - ); - const mock = generateWillUpdateManifest(); - const store = defaultStore(); - const storeSetSpy = jest.spyOn(store, 'set'); - const storeDeleteSpy = jest.spyOn(store, 'delete'); - - const agent = createAgent({ - willUpdateManifest: mock, - store, - }); - await agent.start(); - expect(mock).toHaveBeenCalledTimes(1); - expect(mock).toHaveBeenCalledWith( - { - version: 2, - operations: [ - sampleManifestRecords.a, - sampleManifestRecords.b, - sampleManifestRecords.c, - ], - }, - undefined, - ); - - // Three additions, no deletions. - expect(storeSetSpy).toBeCalledTimes(3); - expect(storeDeleteSpy).toBeCalledTimes(0); - }); - it('uses returned manifest to enforce safelist', async () => { - nockStorageSecret(genericServiceID, genericApiKeyHash); - nockGoodManifestsUnderStorageSecret( - genericServiceID, - genericStorageSecret, - [ - sampleManifestRecords.a, - sampleManifestRecords.b, - sampleManifestRecords.c, - ] - ); - const mock = generateWillUpdateManifest([sampleManifestRecords.a]); - const store = defaultStore(); - const storeSetSpy = jest.spyOn(store, 'set'); - const storeDeleteSpy = jest.spyOn(store, 'delete'); - - const agent = createAgent({ - willUpdateManifest: mock, - store, - }); - await agent.start(); - expect(mock).toHaveBeenCalledTimes(1); - expect(mock).toHaveBeenCalledWith( - { - version: 2, - operations: [ - sampleManifestRecords.a, - sampleManifestRecords.b, - sampleManifestRecords.c, - ], - }, - undefined, - ); - expect(mock).toReturnWith({ - version: 2, - operations: [sampleManifestRecords.a], - }); - - // One additions, no deletions. - expect(storeSetSpy).toBeCalledTimes(1); - expect(storeDeleteSpy).toBeCalledTimes(0); - }); - it('is not called again when manifest remains same', async () => { - nockStorageSecret(genericServiceID, genericApiKeyHash); - nockGoodManifestsUnderStorageSecret( - genericServiceID, - genericStorageSecret, - [ - sampleManifestRecords.a, - sampleManifestRecords.b, - sampleManifestRecords.c, - ] - ); - - const mock = generateWillUpdateManifest(); - const store = defaultStore(); - const storeSetSpy = jest.spyOn(store, 'set'); - const storeDeleteSpy = jest.spyOn(store, 'delete'); - - const agent = createAgent({ - willUpdateManifest: mock, - store, - }); - jest.useFakeTimers(); - await agent.start(); - expect(mock).toHaveBeenCalledTimes(1); - expect(mock).toHaveBeenCalledWith( - { - version: 2, - operations: [ - sampleManifestRecords.a, - sampleManifestRecords.b, - sampleManifestRecords.c, - ], - }, - undefined, - ); - // Three additions, no deletions. - expect(storeSetSpy).toBeCalledTimes(3); - expect(storeDeleteSpy).toBeCalledTimes(0); - - // If it's one millisecond short of our next poll interval, nothing - // should have changed yet. - jest.advanceTimersByTime(defaultTestAgentPollSeconds * 1000 - 1); - - // Still only one check. - expect(agent._timesChecked).toBe(1); - - // Now, we'll expect another request to go out, so we'll nock it. - nockStorageSecret(genericServiceID, genericApiKeyHash); - nockStorageSecretOperationManifest( - genericServiceID, - genericStorageSecret, - 304, - ); - - // If we move forward the last remaining millisecond, we should trigger - // and end up with a successful sync. - jest.advanceTimersByTime(1); - - // While that timer will fire, it will do work async, and we need to - // wait on that work itself. - await agent.requestPending(); - - // Now the times checked should have gone up. - expect(agent._timesChecked).toBe(2); - - // the agent should not have been called again - expect(mock).toHaveBeenCalledTimes(1); - expect(mock).toHaveBeenCalledWith( - { - version: 2, - operations: [ - sampleManifestRecords.a, - sampleManifestRecords.b, - sampleManifestRecords.c, - ], - }, - undefined, - ); - // Three additions, no deletions. - expect(storeSetSpy).toBeCalledTimes(3); - expect(storeDeleteSpy).toBeCalledTimes(0); - }); - - it('receives previous manifest when updated', async () => { - nockStorageSecret(genericServiceID, genericApiKeyHash); - nockGoodManifestsUnderStorageSecret( - genericServiceID, - genericStorageSecret, - [ - sampleManifestRecords.a, - sampleManifestRecords.b, - sampleManifestRecords.c, - ] - ); - - const mock = generateWillUpdateManifest(); - const store = defaultStore(); - const storeSetSpy = jest.spyOn(store, 'set'); - const storeDeleteSpy = jest.spyOn(store, 'delete'); - - const agent = createAgent({ - willUpdateManifest: mock, - store, - }); - jest.useFakeTimers(); - await agent.start(); - expect(mock).toHaveBeenCalledTimes(1); - expect(mock).toHaveBeenCalledWith( - { - version: 2, - operations: [ - sampleManifestRecords.a, - sampleManifestRecords.b, - sampleManifestRecords.c, - ], - }, - undefined, - ); - // Three additions, no deletions. - expect(storeSetSpy).toBeCalledTimes(3); - expect(storeDeleteSpy).toBeCalledTimes(0); - - // If it's one millisecond short of our next poll interval, nothing - // should have changed yet. - jest.advanceTimersByTime(defaultTestAgentPollSeconds * 1000 - 1); - - // Still only one check. - expect(agent._timesChecked).toBe(1); - - // Now, we'll expect another request to go out, so we'll nock it. - nockStorageSecret(genericServiceID, genericApiKeyHash); - nockGoodManifestsUnderStorageSecret( - genericServiceID, - genericStorageSecret, - [sampleManifestRecords.a], - ); - - // If we move forward the last remaining millisecond, we should trigger - // and end up with a successful sync. - jest.advanceTimersByTime(1); - - // While that timer will fire, it will do work async, and we need to - // wait on that work itself. - await agent.requestPending(); - - expect(mock).toHaveBeenCalledTimes(2); - expect(mock).toHaveBeenCalledWith( - { version: 2, operations: [sampleManifestRecords.a] }, - { - version: 2, - operations: [ - sampleManifestRecords.a, - sampleManifestRecords.b, - sampleManifestRecords.c, - ], - }, - ); - // Three additions, two deletions. - expect(storeSetSpy).toBeCalledTimes(3); - expect(storeDeleteSpy).toBeCalledTimes(2); - }); - }); - }); }); }); diff --git a/packages/apollo-server-plugin-operation-registry/src/agent.ts b/packages/apollo-server-plugin-operation-registry/src/agent.ts index 6f12d8edfc3..93cc3a8d26e 100644 --- a/packages/apollo-server-plugin-operation-registry/src/agent.ts +++ b/packages/apollo-server-plugin-operation-registry/src/agent.ts @@ -12,7 +12,6 @@ import loglevel from 'loglevel'; import { Response } from 'node-fetch'; import { InMemoryLRUCache } from 'apollo-server-caching'; import { fetchIfNoneMatch } from './fetchIfNoneMatch'; -import { ValueOrPromise } from "apollo-server-plugin-base"; import { OperationManifest } from "./ApolloServerPluginOperationRegistry"; const DEFAULT_POLL_SECONDS: number = 30; @@ -25,10 +24,6 @@ export interface AgentOptions { engine: any; store: InMemoryLRUCache; schemaTag: string; - willUpdateManifest?: ( - newManifest?: OperationManifest, - oldManifest?: OperationManifest, - ) => ValueOrPromise; } type SignatureStore = Set; @@ -47,7 +42,6 @@ export default class Agent { public _timesChecked: number = 0; private lastOperationSignatures: SignatureStore = new Set(); - private lastManifest?: OperationManifest; private readonly options: AgentOptions = Object.create(null); constructor(options: AgentOptions) { @@ -98,14 +92,8 @@ export default class Agent { try { await pulse(); } catch (err) { - // Update the manifest to trigger `willUpdateManifest(newManifest: undefined, oldManifest: undefined)` - await this.updateManifest(); console.error( - `The operation manifest could not be fetched. Retries will continue.${ - this.lastManifest - ? '' - : ' Requests will be forbidden until the manifest is fetched.' - }`, + 'The operation manifest could not be fetched. Retries will continue, but requests will be forbidden until the manifest is fetched.', err.message || err, ); } @@ -308,37 +296,19 @@ export default class Agent { }); } - public async updateManifest(manifest?: OperationManifest) { + public async updateManifest(manifest: OperationManifest) { if ( - manifest && - (manifest.version !== 2 || !Array.isArray(manifest.operations)) + !manifest || + manifest.version !== 2 || + !Array.isArray(manifest.operations) ) { throw new Error('Invalid manifest format.'); } - const returnedManifest = - (await (this.options.willUpdateManifest && - this.options.willUpdateManifest(manifest, this.lastManifest))) || - manifest; - - if (returnedManifest && !Array.isArray(returnedManifest.operations)) { - throw new Error( - "Invalid manifest format. Manifest's operations must be an array", - ); - } - - if (returnedManifest) { - this.updateOperationStore(returnedManifest.operations); - } - - this.lastManifest = manifest; - } - - private updateOperationStore(operations: OperationManifest['operations']) { const incomingOperations: Map = new Map(); const replacementSignatures: SignatureStore = new Set(); - for (const { signature, document } of operations) { + for (const { signature, document } of manifest.operations) { incomingOperations.set(signature, document); // Keep track of each operation in this manifest so we can store it // for comparison after the next fetch. From 8d4bb5f4f79bcfa6be6e5bdbc9ab52bc8dc92850 Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Mon, 25 Nov 2019 22:01:50 +0200 Subject: [PATCH 35/36] Release - apollo-server-plugin-operation-registry@0.3.0-alpha.0 --- packages/apollo-server-plugin-operation-registry/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/apollo-server-plugin-operation-registry/package.json b/packages/apollo-server-plugin-operation-registry/package.json index 464667c8783..1255943a8f1 100644 --- a/packages/apollo-server-plugin-operation-registry/package.json +++ b/packages/apollo-server-plugin-operation-registry/package.json @@ -1,6 +1,6 @@ { "name": "apollo-server-plugin-operation-registry", - "version": "0.2.3-alpha.0", + "version": "0.3.0-alpha.0", "description": "Apollo Server operation registry", "main": "dist/index.js", "types": "dist/index.d.ts", From 78ca31a376f48ce19efbfc664e19a53744c24fee Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Mon, 25 Nov 2019 22:12:40 +0200 Subject: [PATCH 36/36] Update `fakeTesetBaseUrl` to a more recognizable hostname. Just in case it leaks somewhere and someone wants to know what it is quickly! --- .../src/__tests__/common.test.ts | 2 +- packages/apollo-server-plugin-operation-registry/src/common.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/apollo-server-plugin-operation-registry/src/__tests__/common.test.ts b/packages/apollo-server-plugin-operation-registry/src/__tests__/common.test.ts index e1b77154327..6b5bb2c22f0 100644 --- a/packages/apollo-server-plugin-operation-registry/src/__tests__/common.test.ts +++ b/packages/apollo-server-plugin-operation-registry/src/__tests__/common.test.ts @@ -9,7 +9,7 @@ describe('common', () => { expect( common.getLegacyOperationManifestUrl('aServiceId', 'aSchemaHash'), ).toMatchInlineSnapshot( - `"https://myfakehost/aServiceId/aSchemaHash.v2.json"`, + `"https://fake-host-for-apollo-op-reg-tests/aServiceId/aSchemaHash.v2.json"`, ); }); }); diff --git a/packages/apollo-server-plugin-operation-registry/src/common.ts b/packages/apollo-server-plugin-operation-registry/src/common.ts index febb52f58e7..d0a46f7c3eb 100644 --- a/packages/apollo-server-plugin-operation-registry/src/common.ts +++ b/packages/apollo-server-plugin-operation-registry/src/common.ts @@ -6,7 +6,7 @@ export const envOverrideOperationManifest = export const envOverrideStorageSecretBaseUrl = 'APOLLO_STORAGE_SECRET_BASE_URL'; -export const fakeTestBaseUrl = 'https://myfakehost/'; +export const fakeTestBaseUrl = 'https://fake-host-for-apollo-op-reg-tests/'; // Generate and cache our desired operation manifest URL. export const urlOperationManifestBase: string = ((): string => {