From f25a5cb8a427fb53065501b93afb480960fbd456 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Tue, 2 Aug 2022 06:29:54 -0700 Subject: [PATCH] Backport #2028 and #2029 (#2031) * gateway: remove dependency on apollo-server-caching (#2029) The point of apollo-server-caching is to provide an abstraction over multiple cache backends. Gateway is not using that abstraction; it's just using one particular implementation, which is a wrapper around an old version of lru-cache. As part of https://github.com/apollographql/apollo-server/issues/6057 and https://github.com/apollographql/apollo-server/issues/6719 we want to remove dependencies on Apollo Server from Apollo Gateway. Technically we don't really need to remove this dependency (since apollo-server-caching doesn't depend on anything else in AS) but apollo-server-caching won't be updated any more (in fact, even in AS3 it has already been replaced by `@apollo/utils.keyvaluecache`), so let's do it. While we're at it, we make a few other improvements: - Ever since #440, the queryPlanStore field is always set, so we can remove some conditionals around it. - Instead of using the old lru-cache@6 wrapped by the apollo-server-caching package, we use the newer lru-cache@7 (which improves the algorithm internally and changes the names of methods a bit). - The get and set methods on InMemoryLRUCache were only async because they implement the abstract KeyValueCache interface: the implementations didn't actually do anything async. So we no longer need to await them or include a giant comment about how we're not awaiting them. * gateway RemoteGraphQLDataSource: throw GraphQLError, not ApolloError (#2028) This is part of https://github.com/apollographql/apollo-server/issues/6057 (which is itself part of https://github.com/apollographql/apollo-server/issues/6719). We are trying to break the dependency of Gateway on Server so that (among other things) it is easier to have a single version of Gateway that works with both the current AS3 and the upcoming AS4. In AS4, we are removing the ApolloError class and its subclasses. Instead, we will just use GraphQLError directly. See: https://www.apollographql.com/docs/apollo-server/v4/migration#apolloerror https://www.apollographql.com/docs/apollo-server/v4/migration#built-in-error-classes https://github.com/apollographql/apollo-server/pull/6355 https://github.com/apollographql/apollo-server/pull/6705 This commit changes RemoteGraphQLDataSource to throw GraphQLError instead of ApolloError. The `code` extension will still be the same. (The `name` field of the thrown Error will no longer be eg `AuthenticationError`, though; this does not affect the error as serialized in GraphQL.) This is technically slightly backwards-incompatible (eg, the method errorFromResponse is public and now returns GraphQLError instead of the tighter ApolloError) but this doesn't seem likely to affect many users. We can adjust based on feedback if necessary. * Adjust #2028 for graphql@15.8 compatibility --- gateway-js/CHANGELOG.md | 3 +- gateway-js/package.json | 3 +- .../datasources/RemoteGraphQLDataSource.ts | 42 ++++++++++--------- .../__tests__/RemoteGraphQLDataSource.test.ts | 23 ++++------ gateway-js/src/index.ts | 39 +++++------------ package-lock.json | 37 ++++------------ 6 files changed, 54 insertions(+), 93 deletions(-) diff --git a/gateway-js/CHANGELOG.md b/gateway-js/CHANGELOG.md index b6aec0737..a4a3037eb 100644 --- a/gateway-js/CHANGELOG.md +++ b/gateway-js/CHANGELOG.md @@ -5,7 +5,8 @@ This CHANGELOG pertains only to Apollo Federation packages in the `0.x` range. T ## vNEXT > The changes noted within this `vNEXT` section have not been released yet. New PRs and commits which introduce changes should include an entry in this `vNEXT` section as part of their development. When a release is being prepared, a new header will be (manually) -- Nothing yet! Stay tuned. +- The method `RemoteGraphQLDataSource.errorFromResponse` now returns a `GraphQLError` (as defined by `graphql`) rather than an `ApolloError` (as defined by `apollo-server-errors`). [PR #2028](https://github.com/apollographql/federation/pull/2028) + - __BREAKING__: If you call `RemoteGraphQLDataSource.errorFromResponse` manually and expect its return value to be a particular subclass of `GraphQLError`, or if you expect the error received by `didEncounterError` to be a particular subclass of `GraphQLError`, then this change may affect you. We recommend checking `error.extensions.code` instead. ## v0.51.0 diff --git a/gateway-js/package.json b/gateway-js/package.json index acecdf76b..e9ce8a272 100644 --- a/gateway-js/package.json +++ b/gateway-js/package.json @@ -35,12 +35,11 @@ "@opentelemetry/api": "^1.0.1", "@types/node-fetch": "2.6.2", "apollo-reporting-protobuf": "^0.8.0 || ^3.0.0", - "apollo-server-caching": "^0.7.0 || ^3.0.0", "apollo-server-core": "^2.23.0 || ^3.0.0", - "apollo-server-errors": "^2.5.0 || ^3.0.0", "apollo-server-types": "^0.9.0 || ^3.0.0", "async-retry": "^1.3.3", "loglevel": "^1.6.1", + "lru-cache": "^7.13.1", "make-fetch-happen": "^10.1.2", "pretty-format": "^28.0.0" }, diff --git a/gateway-js/src/datasources/RemoteGraphQLDataSource.ts b/gateway-js/src/datasources/RemoteGraphQLDataSource.ts index f7e696db6..33f824419 100644 --- a/gateway-js/src/datasources/RemoteGraphQLDataSource.ts +++ b/gateway-js/src/datasources/RemoteGraphQLDataSource.ts @@ -7,11 +7,6 @@ import { CacheScope, CachePolicy, } from 'apollo-server-types'; -import { - ApolloError, - AuthenticationError, - ForbiddenError, -} from 'apollo-server-errors'; import { isObject } from '../utilities/predicates'; import { GraphQLDataSource, GraphQLDataSourceProcessOptions, GraphQLDataSourceRequestKind } from './types'; import { createHash } from '@apollo/utils.createhash'; @@ -19,6 +14,7 @@ import { parseCacheControlHeader } from './parseCacheControlHeader'; import fetcher from 'make-fetch-happen'; import { Headers as NodeFetchHeaders, Request as NodeFetchRequest } from 'node-fetch'; import { Fetcher, FetcherRequestInit, FetcherResponse } from '@apollo/utils.fetcher'; +import { GraphQLError, GraphQLErrorExtensions } from 'graphql'; export class RemoteGraphQLDataSource< TContext extends Record = Record, @@ -303,28 +299,36 @@ export class RemoteGraphQLDataSource< } public async errorFromResponse(response: FetcherResponse) { - const message = `${response.status}: ${response.statusText}`; - - let error: ApolloError; - if (response.status === 401) { - error = new AuthenticationError(message); - } else if (response.status === 403) { - error = new ForbiddenError(message); - } else { - error = new ApolloError(message); - } - const body = await this.parseBody(response); - Object.assign(error.extensions, { + const extensions: GraphQLErrorExtensions = { response: { url: response.url, status: response.status, statusText: response.statusText, body, }, - }); + }; + + if (response.status === 401) { + extensions.code = 'UNAUTHENTICATED'; + } else if (response.status === 403) { + extensions.code = 'FORBIDDEN'; + } - return error; + // Note: gateway 0.x still supports graphql-js v15.8, which does + // not have the options-based GraphQLError constructor. Note that + // the constructor used here is dropped in graphql-js v17, so this + // will have to be adjusted if we try to make gateway 0.x support + // graphql-js v17. + return new GraphQLError( + `${response.status}: ${response.statusText}`, + null, + null, + null, + null, + null, + extensions, + ); } } diff --git a/gateway-js/src/datasources/__tests__/RemoteGraphQLDataSource.test.ts b/gateway-js/src/datasources/__tests__/RemoteGraphQLDataSource.test.ts index 0f0a82b24..c60c0345f 100644 --- a/gateway-js/src/datasources/__tests__/RemoteGraphQLDataSource.test.ts +++ b/gateway-js/src/datasources/__tests__/RemoteGraphQLDataSource.test.ts @@ -1,15 +1,10 @@ -import { - ApolloError, - AuthenticationError, - ForbiddenError, -} from 'apollo-server-errors'; - import { RemoteGraphQLDataSource } from '../RemoteGraphQLDataSource'; import { Response, Headers } from 'node-fetch'; import { GraphQLRequestContext } from 'apollo-server-types'; import { GraphQLDataSourceRequestKind } from '../types'; import { nockBeforeEach, nockAfterEach } from '../../__tests__/nockAssertions'; import nock from 'nock'; +import { GraphQLError } from 'graphql'; beforeEach(nockBeforeEach); afterEach(nockAfterEach); @@ -461,7 +456,7 @@ describe('didEncounterError', () => { context, }); - await expect(result).rejects.toThrow(AuthenticationError); + await expect(result).rejects.toThrow(GraphQLError); expect(context).toMatchObject({ timingData: [{ time: 1616446845234 }], }); @@ -469,7 +464,7 @@ describe('didEncounterError', () => { }); describe('error handling', () => { - it('throws an AuthenticationError when the response status is 401', async () => { + it('throws error with code UNAUTHENTICATED when the response status is 401', async () => { const DataSource = new RemoteGraphQLDataSource({ url: 'https://api.example.com/foo', }); @@ -480,7 +475,7 @@ describe('error handling', () => { ...defaultProcessOptions, request: { query: '{ me { name } }' }, }); - await expect(result).rejects.toThrow(AuthenticationError); + await expect(result).rejects.toThrow(GraphQLError); await expect(result).rejects.toMatchObject({ extensions: { code: 'UNAUTHENTICATED', @@ -492,7 +487,7 @@ describe('error handling', () => { }); }); - it('throws a ForbiddenError when the response status is 403', async () => { + it('throws an error with code FORBIDDEN when the response status is 403', async () => { const DataSource = new RemoteGraphQLDataSource({ url: 'https://api.example.com/foo', }); @@ -503,7 +498,7 @@ describe('error handling', () => { ...defaultProcessOptions, request: { query: '{ me { name } }' }, }); - await expect(result).rejects.toThrow(ForbiddenError); + await expect(result).rejects.toThrow(GraphQLError); await expect(result).rejects.toMatchObject({ extensions: { code: 'FORBIDDEN', @@ -515,7 +510,7 @@ describe('error handling', () => { }); }); - it('throws an ApolloError when the response status is 500', async () => { + it('throws a GraphQLError when the response status is 500', async () => { const DataSource = new RemoteGraphQLDataSource({ url: 'https://api.example.com/foo', }); @@ -526,7 +521,7 @@ describe('error handling', () => { ...defaultProcessOptions, request: { query: '{ me { name } }' }, }); - await expect(result).rejects.toThrow(ApolloError); + await expect(result).rejects.toThrow(GraphQLError); await expect(result).rejects.toMatchObject({ extensions: { response: { @@ -560,7 +555,7 @@ describe('error handling', () => { ...defaultProcessOptions, request: { query: '{ me { name } }' }, }); - await expect(result).rejects.toThrow(ApolloError); + await expect(result).rejects.toThrow(GraphQLError); await expect(result).rejects.toMatchObject({ extensions: { response: { diff --git a/gateway-js/src/index.ts b/gateway-js/src/index.ts index 0a40c518c..28cc58a74 100644 --- a/gateway-js/src/index.ts +++ b/gateway-js/src/index.ts @@ -5,7 +5,7 @@ import { GraphQLRequestContextExecutionDidStart, } from 'apollo-server-types'; import type { Logger } from '@apollo/utils.logger'; -import { InMemoryLRUCache } from 'apollo-server-caching'; +import LRUCache from 'lru-cache'; import { isObjectType, isIntrospectionType, @@ -128,7 +128,7 @@ export class ApolloGateway implements GraphQLService { private serviceMap: DataSourceMap = Object.create(null); private config: GatewayConfig; private logger: Logger; - private queryPlanStore: InMemoryLRUCache; + private queryPlanStore: LRUCache; private apolloConfig?: ApolloConfigFromAS3; private onSchemaChangeListeners = new Set<(schema: GraphQLSchema) => void>(); private onSchemaLoadOrUpdateListeners = new Set< @@ -210,14 +210,14 @@ export class ApolloGateway implements GraphQLService { } private initQueryPlanStore(approximateQueryPlanStoreMiB?: number) { - return new InMemoryLRUCache({ + return new LRUCache({ // Create ~about~ a 30MiB InMemoryLRUCache. This is less than precise // since the technique to calculate the size of a DocumentNode is // only using JSON.stringify on the DocumentNode (and thus doesn't account // for unicode characters, etc.), but it should do a reasonable job at // providing a caching document store for most operations. maxSize: Math.pow(2, 20) * (approximateQueryPlanStoreMiB || 30), - sizeCalculator: approximateObjectSize, + sizeCalculation: approximateObjectSize, }); } @@ -607,7 +607,7 @@ export class ApolloGateway implements GraphQLService { // Once we remove the deprecated onSchemaChange() method, we can remove this. legacyDontNotifyOnSchemaChangeListeners: boolean = false, ): void { - if (this.queryPlanStore) this.queryPlanStore.flush(); + this.queryPlanStore.clear(); this.schema = toAPISchema(coreSchema); this.queryPlanner = new QueryPlanner(coreSchema); @@ -843,10 +843,7 @@ export class ApolloGateway implements GraphQLService { return { errors: validationErrors }; } - let queryPlan: QueryPlan | undefined; - if (this.queryPlanStore) { - queryPlan = await this.queryPlanStore.get(queryPlanStoreKey); - } + let queryPlan = this.queryPlanStore.get(queryPlanStoreKey); if (!queryPlan) { queryPlan = tracer.startActiveSpan( @@ -868,25 +865,11 @@ export class ApolloGateway implements GraphQLService { }, ); - if (this.queryPlanStore) { - // The underlying cache store behind the `documentStore` returns a - // `Promise` which is resolved (or rejected), eventually, based on the - // success or failure (respectively) of the cache save attempt. While - // it's certainly possible to `await` this `Promise`, we don't care about - // whether or not it's successful at this point. We'll instead proceed - // to serve the rest of the request and just hope that this works out. - // If it doesn't work, the next request will have another opportunity to - // try again. Errors will surface as warnings, as appropriate. - // - // While it shouldn't normally be necessary to wrap this `Promise` in a - // `Promise.resolve` invocation, it seems that the underlying cache store - // is returning a non-native `Promise` (e.g. Bluebird, etc.). - Promise.resolve( - this.queryPlanStore.set(queryPlanStoreKey, queryPlan), - ).catch((err) => - this.logger.warn( - 'Could not store queryPlan' + ((err && err.message) || err), - ), + try { + this.queryPlanStore.set(queryPlanStoreKey, queryPlan); + } catch (err) { + this.logger.warn( + 'Could not store queryPlan' + ((err && err.message) || err), ); } } diff --git a/package-lock.json b/package-lock.json index c6803092a..587238f14 100644 --- a/package-lock.json +++ b/package-lock.json @@ -102,12 +102,11 @@ "@opentelemetry/api": "^1.0.1", "@types/node-fetch": "2.6.2", "apollo-reporting-protobuf": "^0.8.0 || ^3.0.0", - "apollo-server-caching": "^0.7.0 || ^3.0.0", "apollo-server-core": "^2.23.0 || ^3.0.0", - "apollo-server-errors": "^2.5.0 || ^3.0.0", "apollo-server-types": "^0.9.0 || ^3.0.0", "async-retry": "^1.3.3", "loglevel": "^1.6.1", + "lru-cache": "^7.13.1", "make-fetch-happen": "^10.1.2", "pretty-format": "^28.0.0" }, @@ -119,9 +118,9 @@ } }, "gateway-js/node_modules/lru-cache": { - "version": "7.8.1", - "resolved": "https://registry.npmjs.org/lru-cache/-/lru-cache-7.8.1.tgz", - "integrity": "sha512-E1v547OCgJvbvevfjgK9sNKIVXO96NnsTsFPBlg4ZxjhsJSODoH9lk8Bm0OxvHNm6Vm5Yqkl/1fErDxhYL8Skg==", + "version": "7.13.1", + "resolved": "https://registry.npmjs.org/lru-cache/-/lru-cache-7.13.1.tgz", + "integrity": "sha512-CHqbAq7NFlW3RSnoWXLJBxCWaZVBrfa9UEHId2M3AW8iEBurbqduNexEUCGc3SHc6iCYXNJCDi903LajSVAEPQ==", "engines": { "node": ">=12" } @@ -5442,17 +5441,6 @@ "graphql": "^15.3.0 || ^16.0.0" } }, - "node_modules/apollo-server-caching": { - "version": "3.3.0", - "resolved": "https://registry.npmjs.org/apollo-server-caching/-/apollo-server-caching-3.3.0.tgz", - "integrity": "sha512-Wgcb0ArjZ5DjQ7ID+tvxUcZ7Yxdbk5l1MxZL8D8gkyjooOkhPNzjRVQ7ubPoXqO54PrOMOTm1ejVhsF+AfIirQ==", - "dependencies": { - "lru-cache": "^6.0.0" - }, - "engines": { - "node": ">=12.0" - } - }, "node_modules/apollo-server-core": { "version": "3.9.0", "resolved": "https://registry.npmjs.org/apollo-server-core/-/apollo-server-core-3.9.0.tgz", @@ -20733,20 +20721,19 @@ "@opentelemetry/api": "^1.0.1", "@types/node-fetch": "2.6.2", "apollo-reporting-protobuf": "^0.8.0 || ^3.0.0", - "apollo-server-caching": "^0.7.0 || ^3.0.0", "apollo-server-core": "^2.23.0 || ^3.0.0", - "apollo-server-errors": "^2.5.0 || ^3.0.0", "apollo-server-types": "^0.9.0 || ^3.0.0", "async-retry": "^1.3.3", "loglevel": "^1.6.1", + "lru-cache": "^7.13.1", "make-fetch-happen": "^10.1.2", "pretty-format": "^28.0.0" }, "dependencies": { "lru-cache": { - "version": "7.8.1", - "resolved": "https://registry.npmjs.org/lru-cache/-/lru-cache-7.8.1.tgz", - "integrity": "sha512-E1v547OCgJvbvevfjgK9sNKIVXO96NnsTsFPBlg4ZxjhsJSODoH9lk8Bm0OxvHNm6Vm5Yqkl/1fErDxhYL8Skg==" + "version": "7.13.1", + "resolved": "https://registry.npmjs.org/lru-cache/-/lru-cache-7.13.1.tgz", + "integrity": "sha512-CHqbAq7NFlW3RSnoWXLJBxCWaZVBrfa9UEHId2M3AW8iEBurbqduNexEUCGc3SHc6iCYXNJCDi903LajSVAEPQ==" }, "make-fetch-happen": { "version": "10.1.2", @@ -25008,14 +24995,6 @@ "express": "^4.17.1" } }, - "apollo-server-caching": { - "version": "3.3.0", - "resolved": "https://registry.npmjs.org/apollo-server-caching/-/apollo-server-caching-3.3.0.tgz", - "integrity": "sha512-Wgcb0ArjZ5DjQ7ID+tvxUcZ7Yxdbk5l1MxZL8D8gkyjooOkhPNzjRVQ7ubPoXqO54PrOMOTm1ejVhsF+AfIirQ==", - "requires": { - "lru-cache": "^6.0.0" - } - }, "apollo-server-core": { "version": "3.9.0", "resolved": "https://registry.npmjs.org/apollo-server-core/-/apollo-server-core-3.9.0.tgz",