Fix "response position" definition; clarify sibling errors on propagation

[benjie]

:: A response position is a uniquely identifiable position in the response data produced during execution. It is either a direct entry in the {resultMap} of a {ExecuteSelectionSet()}, or it is a position in a (potentially nested) List value. Each response position is uniquely identifiable via a response path.

The intention of response position is that it includes things that were elided by error propagation; e.g. if you query { a { b } } and b is non-null and throws, the result will be {a: null} but the error will have path ["a", "b"] indicating the a.b response position even though that position does not actually exist in the response.

(Also: ExecuteSelectionSet() doesn't exist any more.)

Commit 1: clarify definition of response position to include these omitted values.

---

If a response position resolves to {null} because of an execution error which has already been added to the {"errors"} list in the execution result, the {"errors"} list must not be further affected. That is, only one error should be added to the errors list per response position.

But the response position mentioned at the beginning of this paragraph is different to the response position from which the error originated; therefore "only one error should be added to the errors list per response position" is kind of moot. Actually what we mean here is that the response path of the error (i.e. the "path" entry in the error object) should be unique.

Commit 2: clarify it's the "path" of the error that matters, not the position that re-raised it

---

The use of the term "response position" is confusing, because that position might not actually exist in the response if it was omitted due to error propagation.

Commit 3: rename response position to execution position throughout.

---

All of this together clarifies what happens when an error occurs to a nullable sibling of a non-nullable field that triggers error propagation.

[netlify[bot]]

✅ Deploy Preview for graphql-spec-draft ready!

Name	Link
🔨 Latest commit	4d6f01bc0d40d778f7e4c7241ad6956a655c5fa6
🔍 Latest deploy log	https://app.netlify.com/projects/graphql-spec-draft/deploys/686ff7c46b446a0008ff9be3
😎 Deploy Preview	https://deploy-preview-1183--graphql-spec-draft.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[ddebrunner]

Now if we have { a { b c } } and both b and c throw, the result is { a: null } and only one of the errors from b or c should be output in the final result.

I think this depends on if c is nullable or not (with b propagating the error to a).

If c also propagates, then errors will contain [a,b] or [a,c].

If c does not propagate, the errors will contain [a,b].

Which means then that not only is [a,c] (non-propagated) is not added to errors, it must also be removed from errors if it already exists?

Does this then apply to children of siblings? E.g. if a nullable (non-propagated) error was in a child of c, e.g. [a, c, d] must it be removed?

[benjie]

I think this depends on if c is nullable or not (with b propagating the error to a).

That's right, yes - exactly as you describe.

Which means then that not only is [a,c] (non-propagated) is not added to errors, it must also be removed from errors if it already exists?

That was how I understood it to work - my interpretation was that it was more of a functional approach where data/errors were "passed down" from nullable positions but non-null positions threw - thus sibling errors would be "overridden" by another field throwing. But when I checked this against the reference implementation I realised I was mistaken and the spec text is actually accurate to the behavior here:

https://github.com/graphql/graphql-js/blob/8dc295551f9a64276442ef850943794db6bcaa4a/src/execution/execute.ts#L597-L612

GraphQL.js reproduction

```ts import { graphql, GraphQLInt, GraphQLNonNull, GraphQLObjectType, GraphQLSchema, } from 'graphql';

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

const mkField = (counter: number) => ({ type: new GraphQLNonNull(GraphQLInt), async resolve() { console.log(Triggered ${counter}); await sleep(Math.random() * 10); console.log(Resolved ${counter}); throw new Error(E${counter}); }, });

const A = new GraphQLObjectType({ name: 'A', fields: { b: { type: new GraphQLNonNull(GraphQLInt), async resolve() { await sleep(10); throw new Error(b); }, }, c: { type: GraphQLInt, async resolve() { await sleep(0); throw new Error(c); }, }, }, });

const Query = new GraphQLObjectType({ name: 'Query', fields: { a: { type: A, resolve() { return {}; }, }, }, }); const schema = new GraphQLSchema({ query: Query, });

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

console.dir(result);

</details>

If `c` is nullable and throws first then the error will be added to the errors list. Later when `b` fails it is _also_ added to the errors list.

However if `c` is non-nullable and throws first, then `b` will be "cancelled" and its error will not be added to the list. (It's not actually cancelled, we just ignore the result. In future we'll use AbortSignal to make it possible to cancel.)

And if `b` (non-null) throws before `c` then `c` will not be added to the list whether it's nullable or not.

Interestingly if you implement GraphQL according to my previous interpretation it's still spec compliant because you as the consumer do not know whether b would run before or after c - the only place where this interpretation would break the spec would be non-nullable mutation fields:

<details>
<summary>GraphQL.js example of non-nullable second mutation field persisting error from first mutation</summary>

```ts
import {
  graphql,
  GraphQLInt,
  GraphQLNonNull,
  GraphQLObjectType,
  GraphQLSchema,
} from './npmDist/index.mjs';

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

const Query = new GraphQLObjectType({
  name: 'Query',
  fields: {
    a: {
      type: GraphQLInt,
    },
  },
});
const Mutation = new GraphQLObjectType({
  name: 'Mutation',
  fields: {
    a: {
      type: GraphQLInt,
      async resolve() {
        await sleep(10);
        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(0);
        throw new Error(`c`);
      },
    },
  },
});
const schema = new GraphQLSchema({
  query: Query,
  mutation: Mutation,
});

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

console.dir(result);

I'll update the edits to reflect this.

[benjie]

Plan from today's WG:

1. open a new PR that clarifies "response position" without renaming to "execution position" (which could be ambiguous w.r.t. visiting fragments?)
2. rebase this change on that PR, so this becomes just the one sentence change
3. see how hard this would be to implement in GraphQL.js and a few other implementations
4. if hard, reject
5. in graphql-toe, throw the last error that matches the path (just reverse the array?) or throw an aggregate error

[yaacovCR]

https://github.com/graphql/graphql-js/pull/4458 not sure if this is the implementation you want to test, I thought it’s the implementation for: https://github.com/graphql/graphql-spec/pull/1184