apollo-inline-trace - tracing should not fail if non-nullable field throw an error

[kroupacz]

Description

If non-nullable field in GQL schema throw an error then the tracing plugin is not able to find correct trace node by its path and throw an error here.
See the details in test called 'FAILING TEST - nonNullableFail - simple query - tracing plugin will throw "Could not find node with path testNestedField.failing" error' in this PR.

I am not sure what is the reason why the path is missing in the "ctx.nodes", but probably the reason is that:
https://spec.graphql.org/draft/#sel-GAPHRPTCAACEzBg6S

If the field which experienced an error was declared as Non-Null, the null result will bubble up to the next nullable field.

A possible solution could be:

• assign an error to the nearest possible trace node
• I'm also not sure if tracing should be able to break the entire GQL call... 🤔 and maybe the error should always be assigned to the "root node" if specificNode will not be found

// put errors on the root node by default
let node = ctx.rootNode;

if (Array.isArray(errToReport.path)) {
  const specificNode = getNearestNode(ctx.nodes,errToReport.path);
  if (specificNode) {
    node = specificNode;
  } else {
    throw new Error(`Could not find node with path ${errToReport.path.join('.')}`);
  }
}

... ... ...
...

/**
 * If something fails at "non-nullable" GQL field, the error path will not be in trace nodes
 * but the error should be assigned to the nearest possible trace node
 */
function getNearestNode(nodes: Map<string, Trace.Node>, path: (string | number)[]): Trace.Node | undefined {
  // iterates through "path" backwards
  for (let i = path.length; i > 0; i--) {
    const pathString = path.slice(0, i).join('.');
    const node = nodes.get(pathString);
    if (node) {
      return node;
    }
  }

  return;
}

Type of change

• Bug fix (non-breaking change which fixes an issue)

Test Environment:

• @graphql-yoga/plugin-apollo-inline-trace@3.8.0:
• NodeJS: node@20

[changeset-bot]

🦋 Changeset detected

Latest commit: e5bafe9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@graphql-yoga/plugin-apollo-usage-report	Patch
@graphql-yoga/plugin-apollo-inline-trace	Patch
@graphql-yoga/nestjs-federation	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[kroupacz]

Hello @EmrysMyrddin,
I just added commit with a possible fix that works without problems in my test and local environment.
May i ask you to check it and info if the fix is ok for you?
Then I will remove the test environment (tests) from packages/plugins/apollo-inline-trace/__tests__/apollo-inline-trace-failing-tests folder.

Thank you in advance.
Tomáš Kroupa

[EmrysMyrddin]

I think the fix is ok, it can be a perf penalty, but only on error paths, so it should be acceptable !

You want to remove the tests ? Perhaps we can keep a simplified version of it ? Because it looks like an edge case we want to keep track of in case of refactoring.

[kroupacz]

Interesting, I tried to write a similar test in this test suite, but I wasn't able to reproduce the same issue. The problem seems to only occur if the Yoga is used as a gateway. 🤔
@EmrysMyrddin or @ardatan, may i ask you to help me rewrite the last test called 'FAILING TEST - nonNullableFail - simple query - tracing plugin will throw "Could not find node with path testNestedField.failing" error to be more suitable for your test environment (so remove my custom implementation with @apollo/gateway, axios, ...) 🙏

[ardatan]

@kroupacz Usually we avoid dealing with real servers but use yoga.fetch for testing that mimics the real fetch as you can see in the existing tests.

[kroupacz]

Hello @EmrysMyrddin and @ardatan,
I just rewrote the tests to fit more into the testing environment of this repository.
But I am not sure what to do with following error which can be seen in CI.

    TypeError: Cannot read properties of undefined (reading 'QUERY')

      1 | /* eslint-disable import/no-extraneous-dependencies */
      2 | import { GraphQLSchema } from 'graphql';
    > 3 | import { IntrospectAndCompose, LocalGraphQLDataSource } from '@apollo/gateway';
        | ^

[kroupacz]

The above mentioned issue is caused by incompatibility with graphql@15.8.0. 😞 @apollo/federation-internals module used in @apollo/gateway requires graphql@16 or newer.

node_modules/.pnpm/@apollo+federation-internals@2.9.3_graphql@15.8.0/node_modules/@apollo/federation-internals/src/graphQLJSSchemaToAST.ts:26:50

Is there a way to do not run this test against graphql@15?

[kroupacz]

FYI @EmrysMyrddin , @ardatan
I solved the incompatibility with graphql@15 like this:

import { versionInfo } from 'graphql';

const describeIf = (condition: boolean) => (condition ? describe : describe.skip);

describeIf(versionInfo.major >= 16)('Inline Trace - Yoga gateway', () => {
  ... ...

I hope you will be ok with that... 🙂

[kroupacz]

Hello @EmrysMyrddin,
could we somehow finish this together?
Is there anything else that needs to be done on my side?
It should be the last piece which is blocking us from starting to use GraphQL Yoga as a gateway. 🙂
Thank you in advance.
Tomáš

[kroupacz]

Hello @EmrysMyrddin @ardatan,
I had to update the version of this package here as well, because it overrides the version for all other packages in this repository as well. In my case for the @graphql-yoga/plugin-apollo-inline-trace package.

[kroupacz]

FYI: @EmrysMyrddin @ardatan
I added one more commit with updated version of @envelop/on-resolve package.
Can I ask for a code review of this PR? 🙏
We're quite in a hurry for this fix because we can't use yoga without it.
Thank you in advance.
Tomáš