From 6364b062e75eed7689b5c40cbdd10b1a42965e8b Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Mon, 25 Nov 2019 20:31:55 +0200 Subject: [PATCH 1/2] 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 c685c5f7c43e7d6a86a79a84896472b280a5dbe8 Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Mon, 25 Nov 2019 22:01:50 +0200 Subject: [PATCH 2/2] 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",