Fix ApolloError bug and GraphQLError spec compliance

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