apollo-engine-reporting: tweaks to maskErrorDetails and rewriteError

CHANGELOG.md

@@ -6,6 +6,9 @@ The version headers in this history reflect the versions of Apollo Server itself
 
 > Upcoming!
 
+- `apollo-engine-reporting`: **BEHAVIOR CHANGE**: If the error returned from the `engine.rewriteError` hook has an `extensions` property, that property will be used instead of the original error's extensions. Document that changes to most other `GraphQLError` fields by `engine.rewriteError` are ignored. [PR #2932](https://github.com/apollographql/apollo-server/pull/2932)
+- `apollo-engine-reporting`: **BEHAVIOR CHANGE**: The `engine.maskErrorDetails` option, deprecated by `engine.rewriteError` in v2.5.0, now behaves a bit more like the new option: while all error messages will be redacted, they will still show up on the appropriate nodes in a trace. [PR #2932](https://github.com/apollographql/apollo-server/pull/2932)
+
 ### v2.6.7
 
 - `apollo-server-core`: Guard against undefined property access in `isDirectiveDefined` which resulted in "Cannot read property 'some' of undefined" error. [PR #2924](https://github.com/apollographql/apollo-server/pull/2924) [Issue #2921](https://github.com/apollographql/apollo-server/issues/2921)
@@ -80,7 +83,7 @@ The version headers in this history reflect the versions of Apollo Server itself
 #### New
 
 - New plugin package `apollo-server-plugin-response-cache` implementing a full query response cache based on `apollo-cache-control` hints. The implementation added a few hooks and context fields; see the PR for details. There is a slight change to `cacheControl` object: previously, `cacheControl.stripFormattedExtensions` defaulted to false if you did not provide a `cacheControl` option object, but defaulted to true if you provided (eg) `cacheControl: {defaultMaxAge: 10}`. Now `stripFormattedExtensions` defaults to false unless explicitly provided as `true`, or if you use the legacy boolean `cacheControl: true`. For more information, [read the documentation](https://www.apollographql.com/docs/apollo-server/features/caching).  [PR #2437](https://github.com/apollographql/apollo-server/pull/2437)
-- Add `rewriteError` option to `EngineReportingOptions` (i.e. the `engine` property of the `ApolloServer` constructor).  When defined as a `function`, it will receive an `err` property as its first argument which can be used to manipulate (e.g. redaction) an error prior to sending it to Apollo Engine by modifying, e.g., its `message` property.  The error can also be suppressed from reporting entirely by returning an explicit `null` value.  For more information, [read the documentation](https://www.apollographql.com/docs/apollo-server/features/errors#for-apollo-engine-reporting) and the [`EngineReportingOptions` API reference](https://www.apollographql.com/docs/apollo-server/api/apollo-server#enginereportingoptions). [PR #1639](https://github.com/apollographql/apollo-server/pull/1639)
+- Add `rewriteError` option to `EngineReportingOptions` (i.e. the `engine` property of the `ApolloServer` constructor).  When defined as a `function`, it will receive an `err` property as its first argument which can be used to manipulate (e.g. redaction) an error prior to sending it to Apollo Engine by modifying, e.g., its `message` property.  The error can also be suppressed from reporting entirely by returning an explicit `null` value.  For more information, [read the documentation](https://www.apollographql.com/docs/apollo-server/features/errors#for-apollo-engine-reporting) and the [`EngineReportingOptions` API reference](https://www.apollographql.com/docs/apollo-server/api/apollo-server#enginereportingoptions). `maskErrorDetails` is now deprecated. [PR #1639](https://github.com/apollographql/apollo-server/pull/1639)
 - `apollo-server-azure-functions`: Support `@azure/functions` to enable Apollo Server [Typescript development in Azure Functions](https://azure.microsoft.com/en-us/blog/improving-the-typescript-support-in-azure-functions/). [PR #2487](https://github.com/apollographql/apollo-server/pull/2487)
 - Allow `GraphQLRequestListener` callbacks in plugins to depend on `this`. [PR #2470](https://github.com/apollographql/apollo-server/pull/2470)
 - `apollo-server-testing`: Add `variables` and `operationName` to `Query` and `Mutation` types. [PR #2307](https://github.com/apollographql/apollo-server/pull/2307) [Issue #2172](https://github.com/apollographql/apollo-server/issue/2172)

docs/source/api/apollo-server.md

@@ -372,8 +372,14 @@ addMockFunctionsToSchema({
    purposes of Apollo Engine reporting.  The modified error (e.g. after changing
    the `err.message` property) should be returned or the function should return
    an explicit `null` to avoid reporting the error entirely.  It is not
-   permissable to return `undefined`.
-
+   permissable to return `undefined`. Note that most `GraphQLError` fields,
+   like `path`, will get copied from the original error to the new error: this
+   way, you can just `return new GraphQLError("message")` without having to
+   explicitly keep it associated with the same node. Specifically,  only
+   look at the `message` and `extensions` fields on the `GraphQLError` you return,
+   and if you don't explicitly specify `extensions` we use the original
+   error's `extensions`.
+
 *  `schemaTag`: String
 
    A human-readable name to tag this variant of a schema (i.e. staging, EU). Setting this value will cause metrics to be segmented in the Apollo Platform's UI. Additionally schema validation with a schema tag will only check metrics associate with the same string.

packages/apollo-engine-reporting/src/extension.ts

@@ -22,19 +22,6 @@ const clientNameHeaderKey = 'apollographql-client-name';
 const clientReferenceIdHeaderKey = 'apollographql-client-reference-id';
 const clientVersionHeaderKey = 'apollographql-client-version';
 
-// (DEPRECATE)
-// This special type is used internally to this module to implement the
-// `maskErrorDetails` (https://github.com/apollographql/apollo-server/pull/1615)
-// functionality in the exact form it was originally implemented — which didn't
-// have the result matching the interface provided by `GraphQLError` but instead
-// just had a `message` property set to `<masked>`.  Since `maskErrorDetails`
-// is now slated for deprecation (with its behavior superceded by the more
-// robust `rewriteError` functionality, this GraphQLErrorOrMaskedErrorObject`
-// should be removed when that deprecation is completed in a major release.
-type GraphQLErrorOrMaskedErrorObject =
-  | GraphQLError
-  | (Partial<GraphQLError> & Pick<GraphQLError, 'message'>);
-
 // EngineReportingExtension is the per-request GraphQLExtension which creates a
 // trace (in protobuf Trace format) for a single request. When the request is
 // done, it passes the Trace back to its associated EngineReportingAgent via the
@@ -275,27 +262,8 @@ export class EngineReportingExtension<TContext = any>
     });
   }
 
-  private rewriteError(
-    err: GraphQLError,
-  ): GraphQLErrorOrMaskedErrorObject | null {
-    // (DEPRECATE)
-    // This relatively basic representation of an error is an artifact
-    // introduced by https://github.com/apollographql/apollo-server/pull/1615.
-    // Interesting, the implementation of that feature didn't actually
-    // accomplish what the requestor had desired.  This functionality is now
-    // being superceded by the `rewriteError` function, which is a more dynamic
-    // implementation which multiple Engine users have been interested in.
-    // When this `maskErrorDetails` is officially deprecated, this
-    // `rewriteError` method can be changed to return `GraphQLError | null`,
-    // and as noted in its definition, `GraphQLErrorOrMaskedErrorObject` can be
-    // removed.
-    if (this.options.maskErrorDetails) {
-      return {
-        message: '<masked>',
-      };
-    }
-
-    if (typeof this.options.rewriteError === 'function') {
+  private rewriteError(err: GraphQLError): GraphQLError | null {
+    if (this.options.rewriteError) {
       // Before passing the error to the user-provided `rewriteError` function,
       // we'll make a shadow copy of the error so the user is free to change
       // the object as they see fit.
@@ -327,20 +295,26 @@ export class EngineReportingExtension<TContext = any>
         return err;
       }
 
+      // We only allow rewriteError to change the message and extensions of the
+      // error; we keep everything else the same. That way people don't have to
+      // do extra work to keep the error on the same trace node. We also keep
+      // extensions the same if it isn't explicitly changed (to, eg, {}). (Note
+      // that many of the fields of GraphQLError are not enumerable and won't
+      // show up in the trace (even in the json field) anyway.)
       return new GraphQLError(
         rewrittenError.message,
         err.nodes,
         err.source,
         err.positions,
         err.path,
         err.originalError,
-        err.extensions,
+        rewrittenError.extensions || err.extensions,
       );
     }
     return err;
   }
 
-  private addError(error: GraphQLErrorOrMaskedErrorObject): void {
+  private addError(error: GraphQLError): void {
     // By default, put errors on the root node.
     let node = this.nodes.get('');
     if (error.path) {

packages/apollo-server-core/src/ApolloServer.ts

@@ -335,6 +335,21 @@ export class ApolloServerBase {
     // or cacheControl.
     this.extensions = [];
 
+    // Normalize the legacy option maskErrorDetails.
+    if (engine && typeof engine === 'object') {
+      if (engine.maskErrorDetails && engine.rewriteError) {
+        throw new Error("Can't set both maskErrorDetails and rewriteError!");
+      } else if (
+        engine.rewriteError &&
+        typeof engine.rewriteError !== 'function'
+      ) {
+        throw new Error('rewriteError must be a function');
+      } else if (engine.maskErrorDetails) {
+        engine.rewriteError = () => new GraphQLError('<masked>');
+        delete engine.maskErrorDetails;
+      }
+    }
+
     // In an effort to avoid over-exposing the API key itself, extract the
     // service ID from the API key for plugins which only needs service ID.
     // The truthyness of this value can also be used in other forks of logic

packages/apollo-server-integration-testsuite/src/ApolloServer.ts

@@ -1099,10 +1099,12 @@ export function testApolloServer<AS extends ApolloServerBase>(
               const trace = Object.values(reports[0].tracesPerQuery)[0]
                 .trace[0];
 
-              expect(trace.root.error).toMatchObject([
+              expect(trace.root.child[0].error).toMatchObject([
                 {
-                  json: '{"message":"<masked>"}',
+                  json:
+                    '{"message":"<masked>","locations":[{"line":1,"column":2}],"path":["fieldWhichWillError"]}',
                   message: '<masked>',
+                  location: [{ line: 1, column: 2 }],
                 },
               ]);
             });