Fix ApolloError bug and GraphQLError spec compliance (#3408)

* 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.

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
 

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);

packages/apollo-server-errors/src/__tests__/ApolloError.test.ts

@@ -0,0 +1,49 @@
+import { ApolloError } from '..';
+
+describe('ApolloError', () => {
+  it("doesn't overwrite extensions when provided in the constructor", () => {
+    const error = new ApolloError('My message', 'A_CODE', {
+      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',
+    });
+  });
+
+  // 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 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');
+  });
+});

packages/apollo-server-errors/src/__tests__/tsconfig.json

@@ -0,0 +1,7 @@
+{
+  "extends": "../../../../tsconfig.test.base",
+  "include": ["**/*"],
+  "references": [
+    { "path": "../../" }
+  ]
+}

packages/apollo-server-errors/src/index.ts

@@ -15,24 +15,43 @@ export class ApolloError extends Error implements GraphQLError {
   constructor(
     message: string,
     code?: string,
-    properties?: Record<string, any>,
+    extensions?: Record<string, any>,
   ) {
     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 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
     if (!this.name) {
       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 };
   }
 }
 

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__/" },