From 6f2b69340e5fd6fcd1ca9dd92f41bd69e1646f53 Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Tue, 30 Apr 2019 13:19:01 +0300 Subject: [PATCH] feat(apollo-engine-reporting) Introduce `rewriteError` to munge errors for reporting. (#2618) * Add errorFilter option * update changelog * Rename errorFilter to filterErrors, return GraphQLError or null * Add back `maskErrorDetails` type to maintain support. ...though it's currently not functional as of this commit. * Introduce additional tests for `filterErrors` and `maskErrorDetails`. Even though `maskErrorDetails` is going away, it seems important to continue to test it along with the new functionality introduced by `filterErrors`. Note: These tests highlight some failures in the current `filterErrors` logic which need to be addressed in follow-up commits. * Finish up new `filterErrors` functionality, sans renaming to a new name. The intention is to rename the `filterErrors` function to `rewriteError` in a follow-up commit, but this maintains the previous name during the re-implementation phase. * Add a test that ensures that the `stack` is not transmitted. The new implementation of `rewriteError` (previously `formatErrors` and prior to that `maskErrorDetails`, do nothing to ensure that the `stack` property (i.e. `Error.prototype.stack`) is regenerated using the new combination of `${err.name}: ${err.message`, suffixed with the rest of `Error.captureStackTrace`. That's okay, but we should at least guard against that and make sure that no future code starts to add the `stack` property, without properly redacting it within `rewriteError` internals. That redaction is _slightly_ less than performant (string manipulation), so it didn't seem worth the effort, since we don't actually send it anywhere. * Rename new `filterErrors` function to a more accurate `rewriteError`. While the new `filterErrors` functionality did allow complete filtering of errors prior to reporting them to Apollo Engine, it also provides the ability to change the error entirely. This more thorough functionality deserves a name which more accurately defines its functionality. The use-defined `rewriteError` function (defined within the `engine` configuration on the `ApolloServer` constructor options) can still be used to "filter" an error from going to Engine by re-writing the error to `null` To accomplish this nullification of an error, an explicit `null` can be returned from the `rewriteError` function rather than the normal practice of returning a `GraphQLError` (in a presumably modified form). * Adjust the line lengths of some comments. Because prettier doesn't do it! * Add CHANGELOG.md for `rewriteError`. * Update deprecated to use rewriteError * Update rewriteError in docs - Deprecate `formatError` in docs in favor of `rewriteError` - lint-fix * Remove now-unnecessary guard which checks for `errors`. This is no longer necessary after the changes in @jbaxleyiii's https://github.com/apollographql/apollo-server/pull/2574 (and @martijnwalraven's https://github.com/apollographql/apollo-server/pull/2416), which changed the error reporting to be captured via the newly introduced `didEncounterErrors`, rather than in `willResolveField` where it used to happen. Yay! * Remove long-skipped test that was never close to working. I don't think this is providing much value at this point and it's been skipped for over a year. It appears that the test was intended to test `reportErrorFunction`, though the description claims that it's trying to test something else entirely. Regardless, the testing of traces is actually handled elsewhere now, so this is good to remove. * Fix test structure after merge. * Partially Revert "Update rewriteError in docs" This partially reverts commit 21b8b867ef2d28300733b8f7cab827b11aa4fbb1, which inadvertently changed `formatError`, which is a top-level function which can be used to adjust the response to the client, which is intrinsically different than the new `rewriteError` which modifies the shape of the error in the tracing data which is sent to the Apollo Platform. * Update the API section with more clarity. * Update docs for rewriteError. (#2585) * (docs) Add information on `rewriteError` to the `errors.md`. This adds additional clarity fro the new functionality provided in #1639. * Fix syntax error and use `String.prototype.startsWith` rather `St.pr.match`. Regular expressions are just not as approachable! * Update errors example. The previous example here wasn't a wonderful example since it was suggesting a pattern which is unnecessary when `NODE_ENV=production`, where stack traces are automatically hidden. * Update errors.md * Update errors.md * Update CHANGELOG.md Co-authored-by: GerA --- CHANGELOG.md | 1 + docs/source/api/apollo-server.md | 15 +- docs/source/features/errors.md | 169 ++++++- packages/apollo-engine-reporting/src/agent.ts | 10 +- .../apollo-engine-reporting/src/extension.ts | 134 +++++- .../src/__tests__/ApolloServer.test.ts | 22 - .../src/__tests__/ApolloServer.test.ts | 22 - .../src/ApolloServer.ts | 438 ++++++++++++++---- .../src/__tests__/ApolloServer.test.ts | 22 - 9 files changed, 627 insertions(+), 206 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fd1ea2fa29f..6d42b46c4b7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ - `apollo-server-express`: Fix Playground URL when Apollo Server is mounted inside of another Express app by utilizing `req.originalUrl`. [PR #2451](https://github.com/apollographql/apollo-server/pull/2451) - 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`. [PR #2437](https://github.com/apollographql/apollo-server/pull/2437) - Allow `GraphQLRequestListener` callbacks in plugins to depend on `this`. [PR #2470](https://github.com/apollographql/apollo-server/pull/2470) +- 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) - `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) - `apollo-datasource-rest`: Correctly allow a TTL value of `0` to represent "not-cacheable". [PR #2588](https://github.com/apollographql/apollo-server/pull/2588) - `apollo-datasource-rest`: Fix `Invalid argument` in IE11, when `this.headers` is `undefined`. [PR #2607](https://github.com/apollographql/apollo-server/pull/2607) diff --git a/docs/source/api/apollo-server.md b/docs/source/api/apollo-server.md index 2e9e667ede0..e475b925de4 100644 --- a/docs/source/api/apollo-server.md +++ b/docs/source/api/apollo-server.md @@ -364,13 +364,19 @@ addMockFunctionsToSchema({ 'sendReport()' on other signals if you'd like. Note that 'sendReport()' does not run synchronously so it cannot work usefully in an 'exit' handler. -* `maskErrorDetails`: boolean - - Set to true to remove error details from the traces sent to Apollo's servers. Defaults to false. +* `rewriteError`: (err: GraphQLError) => GraphQLError | null + + By default, all errors are reported to Apollo Engine. This function + can be used to exclude specific errors from being reported. This function + receives a copy of the `GraphQLError` and can manipulate it for the + 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`. * `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. + 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. * `generateClientInfo`: (GraphQLRequestContext) => ClientInfo **AS 2.2** @@ -392,4 +398,3 @@ addMockFunctionsToSchema({ > [WARNING] If you specify a `clientReferenceId`, Engine will treat the > `clientName` as a secondary lookup, so changing a `clientName` may result > in an unwanted experience. - diff --git a/docs/source/features/errors.md b/docs/source/features/errors.md index 74178759c73..12c57faca46 100644 --- a/docs/source/features/errors.md +++ b/docs/source/features/errors.md @@ -14,9 +14,9 @@ When an error occurs in Apollo server both inside and outside of resolvers, each The first step to improving the usability of a server is providing the error stack trace by default. The following example demonstrates the response returned from Apollo server with a resolver that throws a node [`SystemError`](https://nodejs.org/api/errors.html#errors_system_errors). ```js line=14-16 -const { - ApolloServer, - gql, +const { + ApolloServer, + gql, } = require('apollo-server'); const typeDefs = gql` @@ -45,10 +45,10 @@ The response will return: In addition to stacktraces, Apollo Server's exported errors specify a human-readable string in the `code` field of `extensions` that enables the client to perform corrective actions. In addition to improving the client experience, the `code` field allows the server to categorize errors. For example, an `AuthenticationError` sets the code to `UNAUTHENTICATED`, which enables the client to reauthenticate and would generally be ignored as a server anomaly. ```js line=4,15-17 -const { - ApolloServer, - gql, - AuthenticationError, +const { + ApolloServer, + gql, + AuthenticationError, } = require('apollo-server'); const typeDefs = gql` @@ -72,13 +72,13 @@ The response will return: ## Augmenting error details -When clients provide bad input, you may want to return additional information -like a localized message for each field or argument that was invalid. The +When clients provide bad input, you may want to return additional information +like a localized message for each field or argument that was invalid. The following example demonstrates how you can use `UserInputError` to augment your error messages with additional details. ```js line=15-21 -const { +const { ApolloServer, UserInputError, gql, @@ -114,23 +114,34 @@ application, you can use the base `ApolloError` class. ```js new ApolloError(message, code, additionalProperties); -``` +``` ## Masking and logging errors +### For the client response + The Apollo server constructor accepts a `formatError` function that is run on each error passed back to the client. This can be used to mask errors as well as for logging. -This example demonstrates masking (or suppressing the stacktrace): + +> Note that while this changes the error which is sent to the client, it +> doesn't change the error which is sent to Apollo Engine. See the +> `rewriteError` function in the _For Apollo Engine reporting_ section below +> if this behavior is desired. + +This example demonstrates throwing a different error when the error's message starts with `Database Error: `: ```js line=4-10 const server = new ApolloServer({ typeDefs, resolvers, - formatError: error => { - console.log(error); - return new Error('Internal server error'); - // Or, you can delete the exception information - // delete error.extensions.exception; - // return error; + formatError: (err) => { + // Don't give the specific errors to the client. + if (err.message.startsWith("Database Error: ")) { + return new Error('Internal server error'); + } + + // Otherwise return the original error. The error can also + // be manipulated in other ways, so long as it's returned. + return err; }, }); @@ -138,3 +149,125 @@ server.listen().then(({ url }) => { console.log(`🚀 Server ready at ${url}`); }); ``` + +### For Apollo Engine reporting + +With the Apollo Platform, it's possible to observe error rates within Apollo +Engine, rather than simply logging them to the console. While all errors are +sent to Apollo Engine by default, depending on the severity of the error, it +may be desirable to not send particular errors which may be caused by a +user's actions. Alternatively, it may be necessary to modify or redact errors +before transmitting them. + +To account for these needs, a `rewriteError` function can be provided within +the `engine` settings to Apollo Server. At a high-level, the function will +receive the error to be reported (i.e. a `GraphQLError` or an `ApolloError`) +as the first argument. The function should then return a modified form of +that error (e.g. after changing the `err.message` to remove potentially +sensitive information), or should return an explicit `null` in order to avoid +reporting the error entirely. + +The following sections give some examples of various use-cases for `rewriteError`. + +#### Avoid reporting lower-severity predefined errors. + +If an application is utilizing the predefined errors noted above (e.g. +`AuthenticationError`, `ForbiddenError`, `UserInputError`, etc.), these can +be used with `rewriteError`. + +For example, if the current server is `throw`ing the `AuthenticationError` +when a mis-typed password is supplied, an implementor can avoid reporting +this to Apollo Engine by defining `rewriteError` as follows: + +```js line=5-15 +const { ApolloServer, AuthenticationError } = require("apollo-server"); +const server = new ApolloServer({ + typeDefs, // (Not defined in this example) + resolvers, // " " " " + engine: { + rewriteError(err) { + // Return `null` to avoid reporting `AuthenticationError`s + if (err instanceof AuthenticationError) { + return null; + } + + // All other errors will be reported. + return err; + } + }, +}); +``` + +This example configuration would ensure that any `AuthenticationError` which +was thrown within a resolver would only be reported to the client, and never +sent to Apollo Engine. All other errors would be transmitted to Apollo Engine +normally. + +#### Avoid reporting an error based on other properties. + +When generating an error (e.g. `new ApolloError("Failure!")`), the `message` +is the most common property (i.e. `err.message`, which is `Failure!` in this +case). However, any number of properties can be attached to the error (e.g. +adding a `code` property). These properties can be checked when determining +whether an error should be reported to Apollo Engine using the `rewriteError` +function as follows: + +```js line=5-16 +const { ApolloServer } = require("apollo-server"); +const server = new ApolloServer({ + typeDefs, // (Not defined in this example) + resolvers, // " " " " + engine: { + rewriteError(err) { + // Using a more stable, known error property (e.g. `err.code`) would be + // more defensive, however checking the `message` might serve most needs! + if (err.message && err.message.startsWith("Known error message")) { + return null; + } + + // All other errors should still be reported! + return err; + } + }, +}); +``` + +This example configuration would ensure that any error which started with +`Known error message` was not transmitted to Apollo Engine, but all other +errors would continue to be sent. + +#### Redacting the error message. + +If it is necessary to change the error prior to reporting it to Apollo Engine +– for example, if there is personally identifiable information in the error +`message` — the `rewriteError` function can also help. + +Consider an example where the error contained a piece of information like +an API key (e.g. `throw new ApolloError("The x-api-key:12345 doesn't have +sufficient privileges.");`). + +While a best practice would suggest not including such information in the +error message itself, the `rewriteError` function could be used to make sure +it it's sent to Apollo Engine and potentially revealed outside its intended +scope: + +```js line=5-11 +const { ApolloServer } = require("apollo-server"); +const server = new ApolloServer({ + typeDefs, // (Not defined in this example) + resolvers, // " " " " + engine: { + rewriteError(err) { + // Make sure that a specific pattern is removed from all error messages. + err.message = err.message.replace(/x-api-key:[A-Z0-9-]+/, "REDACTED"); + return err; + } + }, +}); +``` + +In this case, the example error used above would be reported in Apollo Engine as: + +``` +The x-api-key:REDACTED doesn't have sufficient privileges. +``` diff --git a/packages/apollo-engine-reporting/src/agent.ts b/packages/apollo-engine-reporting/src/agent.ts index 61813fd272a..a9c230629c9 100644 --- a/packages/apollo-engine-reporting/src/agent.ts +++ b/packages/apollo-engine-reporting/src/agent.ts @@ -1,6 +1,6 @@ import os from 'os'; import { gzip } from 'zlib'; -import { DocumentNode } from 'graphql'; +import { DocumentNode, GraphQLError } from 'graphql'; import { FullTracesReport, ReportHeader, @@ -76,8 +76,14 @@ export interface EngineReportingOptions { handleSignals?: boolean; // Sends the trace report immediately. This options is useful for stateless environments sendReportsImmediately?: boolean; - // To remove the error message from traces, set this to true. Defaults to false + // (DEPRECATED; Use `rewriteError` instead) To remove the error message + // from traces, set this to true. Defaults to false. maskErrorDetails?: boolean; + // By default, all errors get reported to Engine servers. You can specify a + // a filter function to exclude specific errors from being reported by + // returning an explicit `null`, or you can mask certain details of the error + // by modifying it and returning the modified error. + rewriteError?: (err: GraphQLError) => GraphQLError | null; // A human readable name to tag this variant of a schema (i.e. staging, EU) schemaTag?: string; //Creates the client information for operation traces. diff --git a/packages/apollo-engine-reporting/src/extension.ts b/packages/apollo-engine-reporting/src/extension.ts index b8ccd37e364..4dd207d6a38 100644 --- a/packages/apollo-engine-reporting/src/extension.ts +++ b/packages/apollo-engine-reporting/src/extension.ts @@ -19,6 +19,19 @@ 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 ``. 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 & Pick); + // 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 @@ -46,7 +59,6 @@ export class EngineReportingExtension addTrace: (signature: string, operationName: string, trace: Trace) => void, ) { this.options = { - maskErrorDetails: false, ...options, }; this.addTrace = addTrace; @@ -276,31 +288,105 @@ export class EngineReportingExtension } public didEncounterErrors(errors: GraphQLError[]) { - if (errors) { - errors.forEach((error: GraphQLError) => { - // By default, put errors on the root node. - let node = this.nodes.get(''); - if (error.path) { - const specificNode = this.nodes.get(error.path.join('.')); - if (specificNode) { - node = specificNode; - } - } + errors.forEach(err => { + // In terms of reporting, errors can be re-written by the user by + // utilizing the `rewriteError` parameter. This allows changing + // the message or stack to remove potentially sensitive information. + // Returning `null` will result in the error not being reported at all. + const errorForReporting = this.rewriteError(err); - // Always send the trace errors, so that the UI acknowledges that there is an error. - const errorInfo = this.options.maskErrorDetails - ? { message: '' } - : { - message: error.message, - location: (error.locations || []).map( - ({ line, column }) => new Trace.Location({ line, column }), - ), - json: JSON.stringify(error), - }; - - node!.error!.push(new Trace.Error(errorInfo)); - }); + if (errorForReporting === null) { + return; + } + + this.addError(errorForReporting); + }); + } + + 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: '', + }; + } + + if (typeof this.options.rewriteError === 'function') { + // 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. + + // At this stage, this error is only for the purposes of reporting, but + // this is even more important since this is still a reference to the + // original error object and changing it would also change the error which + // is returned in the response to the client. + + // For the clone, we'll create a new object which utilizes the exact same + // prototype of the error being reported. + const clonedError = Object.assign( + Object.create(Object.getPrototypeOf(err)), + err, + ); + + const rewrittenError = this.options.rewriteError(clonedError); + + // Returning an explicit `null` means the user is requesting that, in + // terms of Engine reporting, the error be buried. + if (rewrittenError === null) { + return null; + } + + // We don't want users to be inadvertently not reporting errors, so if + // they haven't returned an explicit `GraphQLError` (or `null`, handled + // above), then we'll report the error as usual. + if (!(rewrittenError instanceof GraphQLError)) { + return err; + } + + return new GraphQLError( + rewrittenError.message, + err.nodes, + err.source, + err.positions, + err.path, + err.originalError, + err.extensions, + ); + } + return err; + } + + private addError(error: GraphQLErrorOrMaskedErrorObject): void { + // By default, put errors on the root node. + let node = this.nodes.get(''); + if (error.path) { + const specificNode = this.nodes.get(error.path.join('.')); + if (specificNode) { + node = specificNode; + } } + + node!.error!.push( + new Trace.Error({ + message: error.message, + location: (error.locations || []).map( + ({ line, column }) => new Trace.Location({ line, column }), + ), + json: JSON.stringify(error), + }), + ); } private newNode(path: ResponsePath): Trace.Node { diff --git a/packages/apollo-server-express/src/__tests__/ApolloServer.test.ts b/packages/apollo-server-express/src/__tests__/ApolloServer.test.ts index 57061edaddf..ac72f7ab437 100644 --- a/packages/apollo-server-express/src/__tests__/ApolloServer.test.ts +++ b/packages/apollo-server-express/src/__tests__/ApolloServer.test.ts @@ -890,28 +890,6 @@ describe('apollo-server-express', () => { expect(result.extensions).toBeDefined(); expect(result.extensions.tracing).toBeDefined(); }); - - xit('applies tracing extension with engine enabled', async () => { - const { url: uri } = await createServer({ - typeDefs, - resolvers, - tracing: true, - engine: { - apiKey: 'service:my-app:secret', - maxAttempts: 0, - endpointUrl: 'l', - reportErrorFunction: () => {}, - }, - }); - - const apolloFetch = createApolloFetch({ uri }); - const result = await apolloFetch({ - query: `{ books { title author } }`, - }); - expect(result.data).toEqual({ books }); - expect(result.extensions).toBeDefined(); - expect(result.extensions.tracing).toBeDefined(); - }); }); }); }); diff --git a/packages/apollo-server-fastify/src/__tests__/ApolloServer.test.ts b/packages/apollo-server-fastify/src/__tests__/ApolloServer.test.ts index 54fd7f58a9a..818f4d88316 100644 --- a/packages/apollo-server-fastify/src/__tests__/ApolloServer.test.ts +++ b/packages/apollo-server-fastify/src/__tests__/ApolloServer.test.ts @@ -809,28 +809,6 @@ describe('apollo-server-fastify', () => { expect(result.extensions).toBeDefined(); expect(result.extensions.tracing).toBeDefined(); }); - - xit('applies tracing extension with engine enabled', async () => { - const { url: uri } = await createServer({ - typeDefs, - resolvers, - tracing: true, - engine: { - apiKey: 'service:my-app:secret', - maxAttempts: 0, - endpointUrl: 'l', - reportErrorFunction: () => {}, - }, - }); - - const apolloFetch = createApolloFetch({ uri }); - const result = await apolloFetch({ - query: `{ books { title author } }`, - }); - expect(result.data).toEqual({ books }); - expect(result.extensions).toBeDefined(); - expect(result.extensions.tracing).toBeDefined(); - }); }); }); }); diff --git a/packages/apollo-server-integration-testsuite/src/ApolloServer.ts b/packages/apollo-server-integration-testsuite/src/ApolloServer.ts index c2b2e5d73a1..dfff861b47b 100644 --- a/packages/apollo-server-integration-testsuite/src/ApolloServer.ts +++ b/packages/apollo-server-integration-testsuite/src/ApolloServer.ts @@ -6,7 +6,7 @@ import express = require('express'); import bodyParser = require('body-parser'); import yup = require('yup'); -import { FullTracesReport, ITrace } from 'apollo-engine-reporting-protobuf'; +import { FullTracesReport } from 'apollo-engine-reporting-protobuf'; import { GraphQLSchema, @@ -487,7 +487,7 @@ export function testApolloServer( }); describe('lifecycle', () => { - describe('with Engine server', () => { + describe('for Apollo Engine', () => { let nodeEnv: string; let engineServer: EngineMockServer; @@ -586,126 +586,136 @@ export function testApolloServer( (engineServer.stop() || Promise.resolve()).then(done); }); - // TODO(jesse) This test block can be merged with another that will - // appear in a separate describe/it block after a branch merges - // that hasn't quite become a PR yet: abernix/finish-pr-1639. It's - // intentionally separate right now because of the guaranteed merge - // conflict. - it('validation > engine > extensions > formatError', async () => { - const throwError = jest.fn(() => { - throw new Error('nope'); - }); + describe('extensions', () => { + // While it's been broken down quite a bit, this test is still + // overloaded and is a prime candidate for de-composition! + it('calls formatError and other overloaded client identity tests', async () => { + const throwError = jest.fn(() => { + throw new Error('nope'); + }); - const validationRule = jest.fn(() => { - // formatError should be called after validation - expect(formatError).not.toBeCalled(); - // extension should be called after validation - expect(willSendResponseInExtension).not.toBeCalled(); - return true; - }); + const validationRule = jest.fn(() => { + // formatError should be called after validation + expect(formatError).not.toBeCalled(); + // extension should be called after validation + expect(willSendResponseInExtension).not.toBeCalled(); + return true; + }); - const willSendResponseInExtension = jest.fn(); + const willSendResponseInExtension = jest.fn(); - const formatError = jest.fn(error => { - try { - expect(error).toBeInstanceOf(Error); - // extension should be called before formatError - expect(willSendResponseInExtension).toHaveBeenCalledTimes(1); - // validationRules should be called before formatError - expect(validationRule).toHaveBeenCalledTimes(1); - } finally { - error.message = 'masked'; - return error; - } - }); + const formatError = jest.fn(error => { + try { + expect(error).toBeInstanceOf(Error); + // extension should be called before formatError + expect(willSendResponseInExtension).toHaveBeenCalledTimes(1); + // validationRules should be called before formatError + expect(validationRule).toHaveBeenCalledTimes(1); + } finally { + error.message = 'masked'; + return error; + } + }); - class Extension extends GraphQLExtension { - willSendResponse(o: { - graphqlResponse: GraphQLResponse; - context: TContext; - }) { - expect(o.graphqlResponse.errors.length).toEqual(1); - // formatError should be called before willSendResponse - expect(formatError).toHaveBeenCalledTimes(1); - // validationRule should be called before willSendResponse - expect(validationRule).toHaveBeenCalledTimes(1); - willSendResponseInExtension(); + class Extension extends GraphQLExtension { + willSendResponse(o: { + graphqlResponse: GraphQLResponse; + context: TContext; + }) { + expect(o.graphqlResponse.errors.length).toEqual(1); + // formatError should be called before willSendResponse + expect(formatError).toHaveBeenCalledTimes(1); + // validationRule should be called before willSendResponse + expect(validationRule).toHaveBeenCalledTimes(1); + willSendResponseInExtension(); + } } - } - const { url: uri } = await createApolloServer({ - typeDefs: gql` - type Query { - fieldWhichWillError: String - } - `, - resolvers: { - Query: { - fieldWhichWillError: () => { - throwError(); + const { url: uri } = await createApolloServer({ + typeDefs: gql` + type Query { + fieldWhichWillError: String + } + `, + resolvers: { + Query: { + fieldWhichWillError: () => { + throwError(); + }, }, }, - }, - validationRules: [validationRule], - extensions: [() => new Extension()], - engine: { - ...engineServer.engineOptions(), - apiKey: 'service:my-app:secret', - maxUncompressedReportSize: 1, - generateClientInfo: () => ({ - clientName: 'testing', - clientReferenceId: '1234', - clientVersion: 'v1.0.1', - }), - }, - formatError, - debug: true, - }); + validationRules: [validationRule], + extensions: [() => new Extension()], + engine: { + ...engineServer.engineOptions(), + apiKey: 'service:my-app:secret', + maxUncompressedReportSize: 1, + generateClientInfo: () => ({ + clientName: 'testing', + clientReferenceId: '1234', + clientVersion: 'v1.0.1', + }), + }, + formatError, + debug: true, + }); - const apolloFetch = createApolloFetch({ uri }); + const apolloFetch = createApolloFetch({ uri }); - const result = await apolloFetch({ - query: `{fieldWhichWillError}`, - }); - expect(result.data).toEqual({ - fieldWhichWillError: null, - }); - expect(result.errors).toBeDefined(); - expect(result.errors[0].message).toEqual('masked'); + const result = await apolloFetch({ + query: `{fieldWhichWillError}`, + }); + expect(result.data).toEqual({ + fieldWhichWillError: null, + }); + expect(result.errors).toBeDefined(); + expect(result.errors[0].message).toEqual('masked'); - expect(validationRule).toHaveBeenCalledTimes(1); - expect(throwError).toHaveBeenCalledTimes(1); - expect(formatError).toHaveBeenCalledTimes(1); - expect(willSendResponseInExtension).toHaveBeenCalledTimes(1); + expect(validationRule).toHaveBeenCalledTimes(1); + expect(throwError).toHaveBeenCalledTimes(1); + expect(formatError).toHaveBeenCalledTimes(1); + expect(willSendResponseInExtension).toHaveBeenCalledTimes(1); - const reports = await engineServer.promiseOfReports; + const reports = await engineServer.promiseOfReports; - expect(reports.length).toBe(1); + expect(reports.length).toBe(1); - const trace = Object.values(reports[0].tracesPerQuery)[0].trace[0]; + const trace = Object.values(reports[0].tracesPerQuery)[0].trace[0]; - expect(trace.clientReferenceId).toMatch(/1234/); - expect(trace.clientName).toMatch(/testing/); - expect(trace.clientVersion).toEqual('v1.0.1'); + expect(trace.clientReferenceId).toMatch(/1234/); + expect(trace.clientName).toMatch(/testing/); + expect(trace.clientVersion).toEqual('v1.0.1'); - expect(trace.root!.child![0].error![0].message).toMatch(/nope/); - expect(trace.root!.child![0].error![0].message).not.toMatch(/masked/); + expect(trace.root!.child![0].error![0].message).toMatch(/nope/); + expect(trace.root!.child![0].error![0].message).not.toMatch( + /masked/, + ); + }); }); - describe('validation > engine > traces', () => { + describe('traces', () => { + let throwError: jest.Mock; let apolloFetch: ApolloFetch; + beforeEach(async () => { + throwError = jest.fn(); + }); + const setupApolloServerAndFetchPair = async ( engineOptions: Partial> = {}, ) => { const { url: uri } = await createApolloServer({ typeDefs: gql` type Query { + fieldWhichWillError: String justAField: String } `, resolvers: { Query: { + fieldWhichWillError: () => { + throwError(); + }, justAField: () => 'a string', }, }, @@ -721,6 +731,42 @@ export function testApolloServer( apolloFetch = createApolloFetch({ uri }); }; + it('does not expose stack', async () => { + throwError.mockImplementationOnce(() => { + throw new Error('how do I stack up?'); + }); + + await setupApolloServerAndFetchPair(); + + const result = await apolloFetch({ + query: `{fieldWhichWillError}`, + }); + expect(result.data).toEqual({ + fieldWhichWillError: null, + }); + expect(result.errors).toBeDefined(); + + // The original error message should still be sent to the client. + expect(result.errors[0].message).toEqual('how do I stack up?'); + expect(throwError).toHaveBeenCalledTimes(1); + + const reports = await engineServer.promiseOfReports; + expect(reports.length).toBe(1); + const trace = Object.values(reports[0].tracesPerQuery)[0].trace[0]; + + // There should be no error at the root, our error is a child. + expect(trace.root.error).toStrictEqual([]); + + // There should only be one child. + expect(trace.root.child.length).toBe(1); + + // The error should not have the stack in it. + expect(trace.root.child[0].error[0]).not.toHaveProperty('stack'); + expect( + JSON.parse(trace.root.child[0].error[0].json), + ).not.toHaveProperty('stack'); + }); + it('sets the trace key to operationName when it is defined', async () => { await setupApolloServerAndFetchPair(); @@ -756,6 +802,216 @@ export function testApolloServer( expect(Object.keys(reports[0].tracesPerQuery)[0]).toMatch(/^# -\n/); }); + + describe('error munging', () => { + describe('rewriteError', () => { + it('new error', async () => { + throwError.mockImplementationOnce(() => { + throw new Error('rewriteError nope'); + }); + + await setupApolloServerAndFetchPair({ + rewriteError: () => + new GraphQLError('rewritten as a new error'), + }); + + const result = await apolloFetch({ + query: `{fieldWhichWillError}`, + }); + expect(result.data).toEqual({ + fieldWhichWillError: null, + }); + expect(result.errors).toBeDefined(); + + // The original error message should be sent to the client. + expect(result.errors[0].message).toEqual('rewriteError nope'); + expect(throwError).toHaveBeenCalledTimes(1); + + const reports = await engineServer.promiseOfReports; + expect(reports.length).toBe(1); + const trace = Object.values(reports[0].tracesPerQuery)[0] + .trace[0]; + // There should be no error at the root, our error is a child. + expect(trace.root.error).toStrictEqual([]); + + // There should only be one child. + expect(trace.root.child.length).toBe(1); + + // The child should maintain the path, but have its message + // rewritten. + expect(trace.root.child[0].error).toMatchObject([ + { + json: + '{"message":"rewritten as a new error","locations":[{"line":1,"column":2}],"path":["fieldWhichWillError"]}', + message: 'rewritten as a new error', + location: [{ column: 2, line: 1 }], + }, + ]); + }); + + it('modified error', async () => { + throwError.mockImplementationOnce(() => { + throw new Error('rewriteError mod nope'); + }); + + await setupApolloServerAndFetchPair({ + rewriteError: err => { + err.message = 'rewritten as a modified error'; + return err; + }, + }); + + const result = await apolloFetch({ + query: `{fieldWhichWillError}`, + }); + expect(result.data).toEqual({ + fieldWhichWillError: null, + }); + expect(result.errors).toBeDefined(); + expect(result.errors[0].message).toEqual( + 'rewriteError mod nope', + ); + expect(throwError).toHaveBeenCalledTimes(1); + + const reports = await engineServer.promiseOfReports; + expect(reports.length).toBe(1); + const trace = Object.values(reports[0].tracesPerQuery)[0] + .trace[0]; + // There should be no error at the root, our error is a child. + expect(trace.root.error).toStrictEqual([]); + + // There should only be one child. + expect(trace.root.child.length).toBe(1); + + // The child should maintain the path, but have its message + // rewritten. + expect(trace.root.child[0].error).toMatchObject([ + { + json: + '{"message":"rewritten as a modified error","locations":[{"line":1,"column":2}],"path":["fieldWhichWillError"]}', + message: 'rewritten as a modified error', + location: [{ column: 2, line: 1 }], + }, + ]); + }); + + it('nulled error', async () => { + throwError.mockImplementationOnce(() => { + throw new Error('rewriteError null nope'); + }); + + await setupApolloServerAndFetchPair({ + rewriteError: () => null, + }); + + const result = await apolloFetch({ + query: `{fieldWhichWillError}`, + }); + expect(result.data).toEqual({ + fieldWhichWillError: null, + }); + expect(result.errors).toBeDefined(); + expect(result.errors[0].message).toEqual( + 'rewriteError null nope', + ); + expect(throwError).toHaveBeenCalledTimes(1); + + const reports = await engineServer.promiseOfReports; + expect(reports.length).toBe(1); + const trace = Object.values(reports[0].tracesPerQuery)[0] + .trace[0]; + + // There should be no error at the root, our error is a child. + expect(trace.root.error).toStrictEqual([]); + + // There should only be one child. + expect(trace.root.child.length).toBe(1); + + // There should be no error in the trace for this property! + expect(trace.root.child[0].error).toStrictEqual([]); + }); + }); + + it('undefined error', async () => { + throwError.mockImplementationOnce(() => { + throw new Error('rewriteError undefined whoops'); + }); + + await setupApolloServerAndFetchPair({ + rewriteError: () => undefined, + }); + + const result = await apolloFetch({ + query: `{fieldWhichWillError}`, + }); + expect(result.data).toEqual({ + fieldWhichWillError: null, + }); + expect(result.errors).toBeDefined(); + expect(result.errors[0].message).toEqual( + 'rewriteError undefined whoops', + ); + expect(throwError).toHaveBeenCalledTimes(1); + + const reports = await engineServer.promiseOfReports; + expect(reports.length).toBe(1); + const trace = Object.values(reports[0].tracesPerQuery)[0] + .trace[0]; + + // There should be no error at the root, our error is a child. + expect(trace.root.error).toStrictEqual([]); + + // There should only be one child. + expect(trace.root.child.length).toBe(1); + + // The child should maintain the path, but have its message + // rewritten. + expect(trace.root.child[0].error).toMatchObject([ + { + json: + '{"message":"rewriteError undefined whoops","locations":[{"line":1,"column":2}],"path":["fieldWhichWillError"]}', + message: 'rewriteError undefined whoops', + location: [{ column: 2, line: 1 }], + }, + ]); + }); + + // This is deprecated, but we'll test it until it's removed in + // Apollo Server 3.x. + it('maskErrorDetails (legacy)', async () => { + throwError.mockImplementationOnce(() => { + throw new Error('maskErrorDetails nope'); + }); + + await setupApolloServerAndFetchPair({ + maskErrorDetails: true, + }); + + const result = await apolloFetch({ + query: `{fieldWhichWillError}`, + }); + + expect(result.data).toEqual({ + fieldWhichWillError: null, + }); + expect(result.errors).toBeDefined(); + expect(result.errors[0].message).toEqual('maskErrorDetails nope'); + + expect(throwError).toHaveBeenCalledTimes(1); + + const reports = await engineServer.promiseOfReports; + expect(reports.length).toBe(1); + const trace = Object.values(reports[0].tracesPerQuery)[0] + .trace[0]; + + expect(trace.root.error).toMatchObject([ + { + json: '{"message":""}', + message: '', + }, + ]); + }); + }); }); }); diff --git a/packages/apollo-server-koa/src/__tests__/ApolloServer.test.ts b/packages/apollo-server-koa/src/__tests__/ApolloServer.test.ts index 9318be58c96..c15bbc2125b 100644 --- a/packages/apollo-server-koa/src/__tests__/ApolloServer.test.ts +++ b/packages/apollo-server-koa/src/__tests__/ApolloServer.test.ts @@ -731,28 +731,6 @@ describe('apollo-server-koa', () => { expect(result.extensions).toBeDefined(); expect(result.extensions.tracing).toBeDefined(); }); - - xit('applies tracing extension with engine enabled', async () => { - const { url: uri } = await createServer({ - typeDefs, - resolvers, - tracing: true, - engine: { - apiKey: 'service:my-app:secret', - maxAttempts: 0, - endpointUrl: 'l', - reportErrorFunction: () => {}, - }, - }); - - const apolloFetch = createApolloFetch({ uri }); - const result = await apolloFetch({ - query: `{ books { title author } }`, - }); - expect(result.data).toEqual({ books }); - expect(result.extensions).toBeDefined(); - expect(result.extensions.tracing).toBeDefined(); - }); }); }); });