From 62e7d940de025f21e89c60404bce0dddac84ed6c Mon Sep 17 00:00:00 2001 From: Trevor Scheer Date: Thu, 24 Aug 2023 10:56:29 -0700 Subject: [PATCH] Fix trace placement for errors occurring in lists (#7699) Previously, when errors occurred while resolving a list item, the trace builder would fail to place the error at the correct path and just default to the root node with a warning message: > `Could not find node with path x.y.1, defaulting to put errors on root node.` This change places these errors at their correct paths and removes the log. --- .changeset/gentle-cycles-look.md | 11 +++ cspell-dict.txt | 1 + jest.config.base.js | 4 +- package-lock.json | 23 ++++++ package.json | 1 + .../inlineTrace/inlineTracePlugin.test.ts | 74 +++++++++++++++++++ .../server/src/plugin/inlineTrace/index.ts | 3 +- .../server/src/plugin/traceTreeBuilder.ts | 33 ++++++--- .../src/plugin/usageReporting/plugin.ts | 1 - 9 files changed, 136 insertions(+), 15 deletions(-) create mode 100644 .changeset/gentle-cycles-look.md create mode 100644 packages/server/src/__tests__/plugin/inlineTrace/inlineTracePlugin.test.ts diff --git a/.changeset/gentle-cycles-look.md b/.changeset/gentle-cycles-look.md new file mode 100644 index 00000000000..dbce17eb5a8 --- /dev/null +++ b/.changeset/gentle-cycles-look.md @@ -0,0 +1,11 @@ +--- +'@apollo/server': patch +--- + +Fix error path attachment for list items + +Previously, when errors occurred while resolving a list item, the trace builder would fail to place the error at the correct path and just default to the root node with a warning message: + +> `Could not find node with path x.y.1, defaulting to put errors on root node.` + +This change places these errors at their correct paths and removes the log. diff --git a/cspell-dict.txt b/cspell-dict.txt index a5455426440..89a1ae35f9b 100644 --- a/cspell-dict.txt +++ b/cspell-dict.txt @@ -189,6 +189,7 @@ tsconfig tsconfigs typecheck typeis +typenames typeof typesafe unawaited diff --git a/jest.config.base.js b/jest.config.base.js index cbff1f3e840..d30cf6a7b98 100644 --- a/jest.config.base.js +++ b/jest.config.base.js @@ -1,4 +1,5 @@ import { defaults } from 'jest-config'; +import { createRequire } from 'node:module'; export default { testEnvironment: 'node', @@ -22,6 +23,5 @@ export default { // Ignore '.js' at the end of imports; part of ESM support. '^(\\.{1,2}/.*)\\.js$': '$1', }, - // this can be removed with jest v29 - snapshotFormat: { escapeString: false, printBasicPrototype: false }, + prettierPath: createRequire(import.meta.url).resolve('prettier-2'), }; diff --git a/package-lock.json b/package-lock.json index 81a6a3d0616..30a3e564077 100644 --- a/package-lock.json +++ b/package-lock.json @@ -67,6 +67,7 @@ "nock": "13.3.2", "node-fetch": "2.6.12", "prettier": "3.0.1", + "prettier-2": "npm:prettier@^2", "qs-middleware": "1.0.3", "requisition": "1.7.0", "rollup": "3.28.0", @@ -11880,6 +11881,22 @@ "url": "https://github.com/prettier/prettier?sponsor=1" } }, + "node_modules/prettier-2": { + "name": "prettier", + "version": "2.8.8", + "resolved": "https://registry.npmjs.org/prettier/-/prettier-2.8.8.tgz", + "integrity": "sha512-tdN8qQGvNjw4CHbY+XXk0JgCXn9QiF21a55rBe5LJAU+kDyC4WQn4+awm2Xfk2lQMk5fKup9XgzTZtGkjBdP9Q==", + "dev": true, + "bin": { + "prettier": "bin-prettier.js" + }, + "engines": { + "node": ">=10.13.0" + }, + "funding": { + "url": "https://github.com/prettier/prettier?sponsor=1" + } + }, "node_modules/pretty-format": { "version": "29.6.2", "resolved": "https://registry.npmjs.org/pretty-format/-/pretty-format-29.6.2.tgz", @@ -23541,6 +23558,12 @@ "integrity": "sha512-fcOWSnnpCrovBsmFZIGIy9UqK2FaI7Hqax+DIO0A9UxeVoY4iweyaFjS5TavZN97Hfehph0nhsZnjlVKzEQSrQ==", "dev": true }, + "prettier-2": { + "version": "npm:prettier@2.8.8", + "resolved": "https://registry.npmjs.org/prettier/-/prettier-2.8.8.tgz", + "integrity": "sha512-tdN8qQGvNjw4CHbY+XXk0JgCXn9QiF21a55rBe5LJAU+kDyC4WQn4+awm2Xfk2lQMk5fKup9XgzTZtGkjBdP9Q==", + "dev": true + }, "pretty-format": { "version": "29.6.2", "resolved": "https://registry.npmjs.org/pretty-format/-/pretty-format-29.6.2.tgz", diff --git a/package.json b/package.json index bf3ad7545f4..dd63d385db7 100644 --- a/package.json +++ b/package.json @@ -95,6 +95,7 @@ "nock": "13.3.2", "node-fetch": "2.6.12", "prettier": "3.0.1", + "prettier-2": "npm:prettier@^2", "qs-middleware": "1.0.3", "requisition": "1.7.0", "rollup": "3.28.0", diff --git a/packages/server/src/__tests__/plugin/inlineTrace/inlineTracePlugin.test.ts b/packages/server/src/__tests__/plugin/inlineTrace/inlineTracePlugin.test.ts new file mode 100644 index 00000000000..902d8da8877 --- /dev/null +++ b/packages/server/src/__tests__/plugin/inlineTrace/inlineTracePlugin.test.ts @@ -0,0 +1,74 @@ +import { ApolloServer, HeaderMap } from '@apollo/server'; +import { gql } from 'graphql-tag'; +import { buildSubgraphSchema } from '@apollo/subgraph'; +import { describe, it, expect } from '@jest/globals'; +import assert from 'assert'; +import { Trace } from '@apollo/usage-reporting-protobuf'; +import { ApolloServerPluginInlineTrace } from '@apollo/server/plugin/inlineTrace'; + +describe('ApolloServerPluginInlineTrace', () => { + it('Adds errors within lists to the correct path rather than the root', async () => { + const server = new ApolloServer({ + schema: buildSubgraphSchema({ + typeDefs: gql` + type Query { + a: A! + } + type A { + brokenList: [String!]! + } + `, + resolvers: { + Query: { + a: () => ({}), + }, + A: { + brokenList() { + return ['hello world!', null]; + }, + }, + }, + }), + plugins: [ + // silence logs about the plugin being enabled + ApolloServerPluginInlineTrace(), + ], + }); + await server.start(); + const result = await server.executeHTTPGraphQLRequest({ + httpGraphQLRequest: { + body: { query: '{a{brokenList}}' }, + headers: new HeaderMap([ + ['content-type', 'application/json'], + ['apollo-federation-include-trace', 'ftv1'], + ]), + method: 'POST', + search: '', + }, + context: async () => ({}), + }); + assert(result.body.kind === 'complete'); + const trace = Trace.decode( + Buffer.from(JSON.parse(result.body.string).extensions?.ftv1, 'base64'), + ); + expect(trace.root?.error).toHaveLength(0); + // error is found at path a.brokenList.1 + expect(trace.root?.child?.[0].child?.[0].child?.[0].error) + .toMatchInlineSnapshot(` + [ + { + "json": "{"message":"","locations":[{"line":1,"column":4}],"path":["a","brokenList",1],"extensions":{"maskedBy":"ApolloServerPluginInlineTrace"}}", + "location": [ + { + "column": 4, + "line": 1, + }, + ], + "message": "", + }, + ] + `); + + await server.stop(); + }); +}); diff --git a/packages/server/src/plugin/inlineTrace/index.ts b/packages/server/src/plugin/inlineTrace/index.ts index 0c8ba0ac587..b150e1735c5 100644 --- a/packages/server/src/plugin/inlineTrace/index.ts +++ b/packages/server/src/plugin/inlineTrace/index.ts @@ -66,7 +66,7 @@ export function ApolloServerPluginInlineTrace( } } }, - async requestDidStart({ request: { http }, metrics, logger }) { + async requestDidStart({ request: { http }, metrics }) { if (!enabled) { return; } @@ -74,7 +74,6 @@ export function ApolloServerPluginInlineTrace( const treeBuilder = new TraceTreeBuilder({ maskedBy: 'ApolloServerPluginInlineTrace', sendErrors: options.includeErrors, - logger, }); // XXX Provide a mechanism to customize this logic. diff --git a/packages/server/src/plugin/traceTreeBuilder.ts b/packages/server/src/plugin/traceTreeBuilder.ts index fb658822af2..a2aa6eaa867 100644 --- a/packages/server/src/plugin/traceTreeBuilder.ts +++ b/packages/server/src/plugin/traceTreeBuilder.ts @@ -6,7 +6,6 @@ import { type ResponsePath, } from 'graphql'; import { Trace, google } from '@apollo/usage-reporting-protobuf'; -import type { Logger } from '@apollo/utils.logger'; import type { SendErrorsOptions } from './usageReporting'; import { UnreachableCaseError } from '../utils/UnreachableCaseError.js'; @@ -16,7 +15,6 @@ function internalError(message: string) { export class TraceTreeBuilder { private rootNode = new Trace.Node(); - private logger: Logger; public trace = new Trace({ root: this.rootNode, // By default, each trace counts as one operation for the sake of field @@ -39,10 +37,9 @@ export class TraceTreeBuilder { public constructor(options: { maskedBy: string; - logger: Logger; sendErrors?: SendErrorsOptions; }) { - const { logger, sendErrors, maskedBy } = options; + const { sendErrors, maskedBy } = options; if (!sendErrors || 'masked' in sendErrors) { this.transformError = () => new GraphQLError('', { @@ -55,7 +52,6 @@ export class TraceTreeBuilder { } else { throw new UnreachableCaseError(sendErrors); } - this.logger = logger; } public startTiming() { @@ -156,11 +152,11 @@ export class TraceTreeBuilder { if (specificNode) { node = specificNode; } else { - this.logger.warn( - `Could not find node with path ${path.join( - '.', - )}; defaulting to put errors on root node.`, - ); + const responsePath = responsePathFromArray(path, this.rootNode); + if (!responsePath) { + throw internalError('addProtobufError called with invalid path!'); + } + node = this.newNode(responsePath); } } @@ -280,6 +276,23 @@ function responsePathAsString(p?: ResponsePath): string { return res; } +function responsePathFromArray( + path: ReadonlyArray, + node: Trace.Node, +): ResponsePath | undefined { + let responsePath: ResponsePath | undefined; + let nodePtr: Trace.INode | undefined = node; + for (const key of path) { + nodePtr = nodePtr?.child?.find((child) => child.responseName === key); + responsePath = { + key, + prev: responsePath, + typename: nodePtr?.type ?? undefined, + }; + } + return responsePath; +} + function errorToProtobufError(error: GraphQLError): Trace.Error { return new Trace.Error({ message: error.message, diff --git a/packages/server/src/plugin/usageReporting/plugin.ts b/packages/server/src/plugin/usageReporting/plugin.ts index 84f20100529..2eb36a17c3f 100644 --- a/packages/server/src/plugin/usageReporting/plugin.ts +++ b/packages/server/src/plugin/usageReporting/plugin.ts @@ -411,7 +411,6 @@ export function ApolloServerPluginUsageReporting( const treeBuilder: TraceTreeBuilder = new TraceTreeBuilder({ maskedBy: 'ApolloServerPluginUsageReporting', sendErrors: options.sendErrors, - logger, }); treeBuilder.startTiming(); metrics.startHrTime = treeBuilder.startHrTime;