From aa96e04fe91b049923c524307168826b4014e13f Mon Sep 17 00:00:00 2001 From: Trevor Scheer Date: Mon, 14 Oct 2019 10:36:45 -0700 Subject: [PATCH 1/6] Add testing infrastructure for apollo-server-errors --- packages/apollo-server-errors/jest.config.js | 3 +++ tsconfig.test.json | 1 + 2 files changed, 4 insertions(+) create mode 100644 packages/apollo-server-errors/jest.config.js diff --git a/packages/apollo-server-errors/jest.config.js b/packages/apollo-server-errors/jest.config.js new file mode 100644 index 00000000000..a383fbc925f --- /dev/null +++ b/packages/apollo-server-errors/jest.config.js @@ -0,0 +1,3 @@ +const config = require('../../jest.config.base'); + +module.exports = Object.assign(Object.create(null), config); diff --git a/tsconfig.test.json b/tsconfig.test.json index 084abf89259..e4ef6484b8f 100644 --- a/tsconfig.test.json +++ b/tsconfig.test.json @@ -15,6 +15,7 @@ { "path": "./packages/apollo-server-caching/src/__tests__/" }, { "path": "./packages/apollo-server-cloud-functions/src/__tests__/" }, { "path": "./packages/apollo-server-core/src/__tests__/" }, + { "path": "./packages/apollo-server-errors/src/__tests__/" }, { "path": "./packages/apollo-server-express/src/__tests__/" }, { "path": "./packages/apollo-server-fastify/src/__tests__/" }, { "path": "./packages/apollo-server-hapi/src/__tests__/" }, From 0d878fd2cab6acfe189f0f91ae5f1ff42b81e18f Mon Sep 17 00:00:00 2001 From: Trevor Scheer Date: Mon, 14 Oct 2019 10:37:04 -0700 Subject: [PATCH 2/6] Initial exploration of ApolloError bug --- .../src/__tests__/ApolloError.test.ts | 24 +++++++++++++++++++ .../src/__tests__/tsconfig.json | 7 ++++++ 2 files changed, 31 insertions(+) create mode 100644 packages/apollo-server-errors/src/__tests__/ApolloError.test.ts create mode 100644 packages/apollo-server-errors/src/__tests__/tsconfig.json diff --git a/packages/apollo-server-errors/src/__tests__/ApolloError.test.ts b/packages/apollo-server-errors/src/__tests__/ApolloError.test.ts new file mode 100644 index 00000000000..7bf1beae967 --- /dev/null +++ b/packages/apollo-server-errors/src/__tests__/ApolloError.test.ts @@ -0,0 +1,24 @@ +import { ApolloError } from '..'; + +describe('ApolloError', () => { + it("doesn't overwrite extensions when provided in the constructor", () => { + const error = new ApolloError('My message', 'A_CODE', { + extensions: { arbitrary: 'user_data' }, + }); + + expect(error.extensions).toEqual({ + code: 'A_CODE', + arbitrary: 'user_data', + }); + + + }); + + it("additional message properties don't overwrite the message provided to the constructor", () => { + const error = new ApolloError('My original message', 'A_CODE', { + message: 'This message overwrites the original message', + }); + + expect(error.message).toEqual('My original message'); + }); +}); diff --git a/packages/apollo-server-errors/src/__tests__/tsconfig.json b/packages/apollo-server-errors/src/__tests__/tsconfig.json new file mode 100644 index 00000000000..428259da813 --- /dev/null +++ b/packages/apollo-server-errors/src/__tests__/tsconfig.json @@ -0,0 +1,7 @@ +{ + "extends": "../../../../tsconfig.test.base", + "include": ["**/*"], + "references": [ + { "path": "../../" } + ] +} From 0c7fbe4c919200f2b36ea32beb3c25bdfb96578e Mon Sep 17 00:00:00 2001 From: Trevor Scheer Date: Tue, 15 Oct 2019 11:52:32 -0700 Subject: [PATCH 3/6] Address ApolloError bug and non-spec compliance * No longer allow the 'properties' object to overwrite the error message provided to the ApolloError constructor * Move ApolloError towards a GraphQLError spec compliance by using extensions rather than arbitrary properties on the class itself. Previous behavior is still supported, however it's now deprecated in favor of extensions. For more context, please read the large comment block associated with this commit. --- .../src/__tests__/ApolloError.test.ts | 31 ++++++++++++++-- packages/apollo-server-errors/src/index.ts | 35 ++++++++++++++----- 2 files changed, 55 insertions(+), 11 deletions(-) diff --git a/packages/apollo-server-errors/src/__tests__/ApolloError.test.ts b/packages/apollo-server-errors/src/__tests__/ApolloError.test.ts index 7bf1beae967..5fdb2587be6 100644 --- a/packages/apollo-server-errors/src/__tests__/ApolloError.test.ts +++ b/packages/apollo-server-errors/src/__tests__/ApolloError.test.ts @@ -3,22 +3,47 @@ import { ApolloError } from '..'; describe('ApolloError', () => { it("doesn't overwrite extensions when provided in the constructor", () => { const error = new ApolloError('My message', 'A_CODE', { - extensions: { arbitrary: 'user_data' }, + arbitrary: 'user_data', }); expect(error.extensions).toEqual({ code: 'A_CODE', arbitrary: 'user_data', }); + }); + it("a code property doesn't overwrite the code provided to the constructor", () => { + const error = new ApolloError('My message', 'A_CODE', { + code: 'CANT_OVERWRITE', + }); + expect(error.extensions).toEqual({ + code: 'A_CODE', + }); }); - it("additional message properties don't overwrite the message provided to the constructor", () => { + // This is a byproduct of how we currently assign properties from the 3rd constructor + // argument onto properties of the class itself. This is expected, but deprecated behavior + // and as such this test should be deleted in the future when we make that breaking change. + it("a message property doesn't overwrite the message provided to the constructor", () => { const error = new ApolloError('My original message', 'A_CODE', { - message: 'This message overwrites the original message', + message: + "This message can't overwrite the original message, but it does end up in extensions", }); expect(error.message).toEqual('My original message'); + expect(error.extensions.message).toEqual( + "This message can't overwrite the original message, but it does end up in extensions", + ); + }); + + it('(back-compat) sets extensions correctly for users who use an extensions key in the third constructor argument', () => { + const error = new ApolloError('My original message', 'A_CODE', { + extensions: { + arbitrary: 'user_data', + }, + }); + + expect(error.extensions.arbitrary).toEqual('user_data'); }); }); diff --git a/packages/apollo-server-errors/src/index.ts b/packages/apollo-server-errors/src/index.ts index 8887d5fd4bf..22d6515bd35 100644 --- a/packages/apollo-server-errors/src/index.ts +++ b/packages/apollo-server-errors/src/index.ts @@ -15,14 +15,30 @@ export class ApolloError extends Error implements GraphQLError { constructor( message: string, code?: string, - properties?: Record, + extensions?: Record, ) { super(message); - if (properties) { - Object.keys(properties).forEach(key => { - this[key] = properties[key]; - }); + // This variable was previously named `properties`, which allowed users to set + // arbitrary properties on the ApolloError object. This use case is still supported, + // but deprecated in favor of using the ApolloError.extensions object instead. + // This change intends to comply with the GraphQL spec on errors. See: + // https://github.com/graphql/graphql-spec/blob/master/spec/Section%207%20--%20Response.md#response-format + // + // Going forward, users should use the ApolloError.extensions object for storing + // and reading arbitrary data on an error, as arbitrary properties on the ApolloError + // itself won't be supported in the future. + // + // XXX Filter 'message' and 'extensions' specifically so they don't't overwrite the class property. + // We _could_ filter all of the class properties, but have chosen to only do + // so if it's an issue for other users. Please feel free to open an issue if you + // find yourself here with this exact problem. + if (extensions) { + Object.keys(extensions) + .filter(keyName => keyName !== 'message' && keyName !== 'extensions') + .forEach(key => { + this[key] = extensions[key]; + }); } // if no name provided, use the default. defineProperty ensures that it stays non-enumerable @@ -30,9 +46,12 @@ export class ApolloError extends Error implements GraphQLError { Object.defineProperty(this, 'name', { value: 'ApolloError' }); } - // extensions are flattened to be included in the root of GraphQLError's, so - // don't add properties to extensions - this.extensions = { code }; + // Before the mentioned change to extensions, users could previously set the extensions + // object by providing it as a key on the third argument to the constructor. + // This step provides backwards compatibility for those hypothetical users. + const userProvidedExtensions = (extensions && extensions.extensions) || null; + + this.extensions = { ...extensions, ...userProvidedExtensions, code }; } } From 52b8b31e790f8bf95c701c852c2b0cca927a9d4f Mon Sep 17 00:00:00 2001 From: Trevor Scheer Date: Tue, 15 Oct 2019 12:00:56 -0700 Subject: [PATCH 4/6] Update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ba6504926ab..e4d4ba5b71b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ The version headers in this history reflect the versions of Apollo Server itself - `@apollo/gateway`, `@apollo/federation`, `apollo-engine-reporting`: Update `apollo-graphql` dependency to bring in [`apollo-tooling`'s #1551](https://github.com/apollographql/apollo-tooling/pull/1551) which resolve runtime errors when its source is minified. While this fixes a particular minification bug when Apollo Server packages are minified, we _do not_ recommend minification of server code in most cases. [PR #3387](https://github.com/apollographql/apollo-server/pull/3387) [Issue #3335](https://github.com/apollographql/apollo-server/issues/3335) - `apollo-server-koa`: Correctly declare dependency on `koa-compose`. [PR #3356](https://github.com/apollographql/apollo-server/pull/3356) - `apollo-server-core`: Preserve any `extensions` that have been placed on the response when pre-execution errors occur. [PR #3394](https://github.com/apollographql/apollo-server/pull/3394) +- `apollo-server-errors`: Fix ApolloError bug and GraphQLError spec compliance [#3408](https://github.com/apollographql/apollo-server/pull/3408) ### v2.9.3 From f4c0a59fdc50a5970e436ba29ec686e214c6dd74 Mon Sep 17 00:00:00 2001 From: Trevor Scheer Date: Tue, 15 Oct 2019 12:08:33 -0700 Subject: [PATCH 5/6] Update packages/apollo-server-errors/src/index.ts Co-Authored-By: Jesse Rosenberger --- packages/apollo-server-errors/src/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/apollo-server-errors/src/index.ts b/packages/apollo-server-errors/src/index.ts index 22d6515bd35..8f2fb3ecb4f 100644 --- a/packages/apollo-server-errors/src/index.ts +++ b/packages/apollo-server-errors/src/index.ts @@ -29,7 +29,7 @@ export class ApolloError extends Error implements GraphQLError { // and reading arbitrary data on an error, as arbitrary properties on the ApolloError // itself won't be supported in the future. // - // XXX Filter 'message' and 'extensions' specifically so they don't't overwrite the class property. + // XXX Filter 'message' and 'extensions' specifically so they don't overwrite the class property. // We _could_ filter all of the class properties, but have chosen to only do // so if it's an issue for other users. Please feel free to open an issue if you // find yourself here with this exact problem. From 9ce569a43bd5c984312b09a82ca28d7162b4c59d Mon Sep 17 00:00:00 2001 From: Trevor Scheer Date: Tue, 15 Oct 2019 12:08:47 -0700 Subject: [PATCH 6/6] Update CHANGELOG.md Co-Authored-By: Jesse Rosenberger --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e4d4ba5b71b..6094061c735 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ The version headers in this history reflect the versions of Apollo Server itself - `@apollo/gateway`, `@apollo/federation`, `apollo-engine-reporting`: Update `apollo-graphql` dependency to bring in [`apollo-tooling`'s #1551](https://github.com/apollographql/apollo-tooling/pull/1551) which resolve runtime errors when its source is minified. While this fixes a particular minification bug when Apollo Server packages are minified, we _do not_ recommend minification of server code in most cases. [PR #3387](https://github.com/apollographql/apollo-server/pull/3387) [Issue #3335](https://github.com/apollographql/apollo-server/issues/3335) - `apollo-server-koa`: Correctly declare dependency on `koa-compose`. [PR #3356](https://github.com/apollographql/apollo-server/pull/3356) - `apollo-server-core`: Preserve any `extensions` that have been placed on the response when pre-execution errors occur. [PR #3394](https://github.com/apollographql/apollo-server/pull/3394) -- `apollo-server-errors`: Fix ApolloError bug and GraphQLError spec compliance [#3408](https://github.com/apollographql/apollo-server/pull/3408) +- `apollo-server-errors`: Fix `ApolloError` bug and `GraphQLError` spec compliance [#3408](https://github.com/apollographql/apollo-server/pull/3408) ### v2.9.3