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.

Author: trevor-scheer

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

cspell-dict.txt

@@ -189,6 +189,7 @@ tsconfig
 tsconfigs
 typecheck
 typeis
+typenames
 typeof
 typesafe
 unawaited

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'),
 };

package-lock.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",

package.json

@@ -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":"<masked>","locations":[{"line":1,"column":4}],"path":["a","brokenList",1],"extensions":{"maskedBy":"ApolloServerPluginInlineTrace"}}",
+          "location": [
+            {
+              "column": 4,
+              "line": 1,
+            },
+          ],
+          "message": "<masked>",
+        },
+      ]
+    `);
+
+    await server.stop();
+  });
+});

packages/server/src/__tests__/plugin/inlineTrace/inlineTracePlugin.test.ts

@@ -66,15 +66,14 @@ export function ApolloServerPluginInlineTrace(
         }
       }
     },
-    async requestDidStart({ request: { http }, metrics, logger }) {
+    async requestDidStart({ request: { http }, metrics }) {
       if (!enabled) {
         return;
       }
 
       const treeBuilder = new TraceTreeBuilder({
         maskedBy: 'ApolloServerPluginInlineTrace',
         sendErrors: options.includeErrors,
-        logger,
       });
 
       // XXX Provide a mechanism to customize this logic.

packages/server/src/plugin/inlineTrace/index.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('<masked>', {
@@ -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<string | number>,
+  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,

packages/server/src/plugin/traceTreeBuilder.ts

@@ -411,7 +411,6 @@ export function ApolloServerPluginUsageReporting<TContext extends BaseContext>(
         const treeBuilder: TraceTreeBuilder = new TraceTreeBuilder({
           maskedBy: 'ApolloServerPluginUsageReporting',
           sendErrors: options.sendErrors,
-          logger,
         });
         treeBuilder.startTiming();
         metrics.startHrTime = treeBuilder.startHrTime;