@defer errors

[alessbell]

Hello!

Given the following query:

query {
  hero {
    name
    heroFriends {
      id
      name
      ... @defer {
        homeWorld
      }
    }
  }
}

And the initial payload:

{
  "errors": [
    {
      "message": "Name for character with ID 1002 could not be fetched.",
      "locations": [{ "line": 6, "column": 7 }],
      "path": ["hero", "heroFriends", 1, "name"]
    }
  ],
  "data": {
    "hero": {
      "name": "R2-D2",
      "heroFriends": [
        {
          "id": "1000",
          "name": "Luke Skywalker"
        },
        {
          "id": "1002",
          "name": null
        },
        {
          "id": "1003",
          "name": "Leia Organa"
        }
      ]
    }
  },
  "hasNext": true
}

and the subsequent payload:

{
  "incremental": [
    {
      "path": ["hero", "heroFriends", 0],
      "errors": [
        {
          "message": "homeWorld for character with ID 1000 could not be fetched.",
          "locations": [{ "line": 6, "column": 7 }],
          "path": ["hero", "heroFriends", 1, "homeWorld"]
        }
      ],
      "data": {
        "homeWorld": null,
      }
    },
    {
      "path": ["hero", "heroFriends", 1],
      "data": {
        "homeWorld": "Corellia",
      }
    },
    {
      "path": ["hero", "heroFriends", 2],
      "data": {
        "homeWorld": "Alderaan",
      }
    }
  ],
  "hasNext": false
}

can we expect that there will never be top-level errors if incremental (and, thus, possibly errors within the list of incremental items) is present?

[glasser]

@IvanGoncharov and I had a lovely lengthy discussion about many things defer/stream yesterday and this was one of the things we discussed in detail.

My strong opinion, and I think @IvanGoncharov eventually agreed, is that there should not be top-level errors here.

The point of the incremental field, as discussed, is largely a performance concern: if a server is capable of generating a bunch of payloads simultaneously, it's helpful to allow clients to process them "atomically" rather than, say, re-rendering after each of them.

There are lots of ways we could have implemented this. eg, we could have made this a transport-layer concern and said that in the HTTP multipart/mixed spec, the body of each sub-message is a series of newline-separated JSON objects and servers can put more than one line in a submessage if multiple payloads are available simultaneously. We chose to put it more explicitly inside the content of the protocol itself. If nothing else, this is a bit more succinct as we don't have to include redundant hasNext: true on the N-1 obviously not final paylods in the array.

But the "batch" itself isn't really a semantically meaningful concept with its own identity, and so I don't think there should be a semantic difference between a "batch level error" and a "individual payload error". Payloads are already capable of representing errors both with and without paths. Making clients have to consider three kinds of errors (field errors with paths, errors inside payloads, and errors at the batch level) just makes it harder to write a correct client.

Note that as of a few hours ago this is represented in the reference implementation where the type for subsequent execution results is:

export interface SubsequentIncrementalExecutionResult<
  TData = ObjMap<unknown>,
  TExtensions = ObjMap<unknown>,
> {
  hasNext: boolean;
  incremental?: ReadonlyArray<IncrementalResult<TData, TExtensions>>;
  extensions?: TExtensions;
}

(Yes, extensions is available both at the batch level and the payload level, but the details of extensions are implementation-defined anyway.)

(For what it's worth, I think it would be simpler if we got rid of the distinction between initial and subsequent results and if execution is producing a stream of maps at all, all maps had the same structure where all information appears inside incremental even on the initial result, rather than today's spec where the initial result may have both top-level data/errors and incremental. But I think the current spec is good enough and I'd like this to be less of a moving target as more and more projects start actually shipping support so I'm not pushing for making this change.)

[glasser]

And independently of whether this is a "good idea" or not, I'm pretty confident it's what the current spec PR says:

  - Let {subsequentResponse} be an unordered map with an entry {incremental}
    initialized to an empty list.
  - Let {records} be the items in {subsequentPayloads} with a completed
    {dataExecution}.
  - For each {record} in {records}:
    - Remove {record} from {subsequentPayloads}.
    - If {isCompletedIterator} on {record} is {true}:
      - Continue to the next record in {records}.
    - Let {payload} be the completed result returned by {dataExecution}.
    - Append {payload} to the {incremental} entry on {subsequentResponse}.
  - If {subsequentPayloads} is empty:
    - Add an entry to {subsequentResponse} named `hasNext` with the value
      {false}.
  - Otherwise, if {subsequentPayloads} is not empty:
    - Add an entry to {subsequentResponse} named `hasNext` with the value
      {true}.
  - Yield {subsequentResponse}

errors never gets added to subsequentResponse here.