Skip to content

Sibling errors should not be added after propagation #1184

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: clarify-one-error-per-result-position
Choose a base branch
from

Conversation

benjie
Copy link
Member

@benjie benjie commented Jul 10, 2025

This PR is built on top of:


GraphQL.js output is not (currently) stable after an operation terminates: more errors may be added to the result after the promise has resolved!

Reproduction with `graphql` module `test.mts`
import type { ExecutionResult } from "graphql";
import {
  graphql,
  GraphQLInt,
  GraphQLNonNull,
  GraphQLObjectType,
  GraphQLSchema,
} from "graphql";

const sleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms));

const Test = new GraphQLObjectType({
  name: "Test",
  fields: {
    a: {
      type: GraphQLInt,
      async resolve() {
        await sleep(0);
        throw new Error(`a`);
      },
    },
    b: {
      type: new GraphQLNonNull(GraphQLInt),
      async resolve() {
        await sleep(10);
        throw new Error(`b`);
      },
    },
    c: {
      type: GraphQLInt,
      async resolve() {
        await sleep(20);
        throw new Error(`c`);
      },
    },
  },
});

const Query = new GraphQLObjectType({
  name: "Query",
  fields: {
    test: {
      type: Test,
      resolve() {
        return {};
      },
    },
  },
});
const schema = new GraphQLSchema({
  query: Query,
});

const result = await graphql({
  schema,
  source: `{ test { a b c } }`,
});

console.log("Result:");
console.log();
console.log(JSON.stringify(result, null, 2));
await sleep(100);
console.log();
console.log("Exact same object 100ms later:");
console.log();
console.log(JSON.stringify(result, null, 2));
$ node test.mts 
Result:

{
  "errors": [
    { "message": "a", "path": ["test", "a"] },
    { "message": "b", "path": ["test", "b"] }
  ],
  "data": { "test": null }
}

Exact same object 100ms later:

{
  "errors": [
    { "message": "a", "path": ["test", "a"] },
    { "message": "b", "path": ["test", "b"] },
    { "message": "c", "path": ["test", "c"] }
  ],
  "data": { "test": null }
}

(I've formatted this output for brevity)

The reason for this: though we note in the spec that you may cancel sibling execution positions, we don't do that in GraphQL.js; and furthermore, we even process errors from the result and add them to the errors list!

This is particularly problematic for client-side "throw on error". Given this schema:

type Query {
  test: Test
}
type Test {
  a: Int  # Throws immediately
  b: Int! # Throws after 10ms
  c: Int  # Throws after 20ms
}

And the same spec-valid result as above:

{
  "errors": [
    { "message": "a", "path": ["test", "a"] },
    { "message": "b", "path": ["test", "b"] },
    { "message": "c", "path": ["test", "c"] }
  ],
  "data": { "test": null }
}

Technically the Test.b field is the field that caused data.test to be null - it's non-nullable, so it triggered error propagation - but without looking at the schema we can't determine this.

Solution: recommend that servers don't keep adding to errors after error propagation has occurred. This would mean:

  1. GraphQL.js won't keep adding to errors after the operation has "completed"
  2. We can throw the last error received that relates to the associated field, and trust that for an implementation following the recommendations it's going to be the one either from the field itself or from the field that triggered error propagation to this level.

@benjie benjie added the 💭 Strawman (RFC 0) RFC Stage 0 (See CONTRIBUTING.md) label Jul 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💭 Strawman (RFC 0) RFC Stage 0 (See CONTRIBUTING.md)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant