Yielding batches of payloads from defer/stream GraphQL requests

[robrichard]

(Continued from #38)

Context

This is not to discuss the payload format of GraphQL responses (e.g. the object containing the data, errors, path, label, extensions fields), but rather how we specify that these payloads are delivered to clients. What's decided here will affect both server implementations (e.g. return type of execute and graphql functions in graphql-js) and the GraphQL specification (we describe how payloads should be yielded to clients).

At the May 2022 WG meeting we discussed the benefits and tradeoffs of yielding individual payloads vs all multiple payloads.

It was determined that there are benefits to yielding lists of payloads. By yielding lists of payloads, clients can do one render pass for each list of payloads that are received. If we yield single payloads, clients would either render on receipt of each payload or have to insert a debouncing delay to see if additional payloads are being sent in quick succession. If each payload is sent individually, it could overwhelm clients that try to re-render UI on each payload when many are sent in rapid succession.

It's important to consider the common cases, it may be rare that multiple payloads are batched together. If 99% is going to be array of size 1, 99% of users are going to be wondering why that array is there.

The following options have been discussed:

Option A: A Single AsyncExecutionResult

• graphql-js return type:

PromiseOrValue<
  | ExecutionResult
  | AsyncGenerator<
      AsyncExecutionResult,
      void,
      void
    >
>

Option B: All Available AsyncExecutionResults

• graphql-js return type:

PromiseOrValue<
  | ExecutionResult
  | AsyncGenerator<
      ReadonlyArray<AsyncExecutionResult>,
      void,
      void
    >
>

• PromiseOrValue<ExecutionResult | AsyncGenerator<ReadonlyArray, void, void>`

Option C: Single payload or list of payloads

• graphql-js return type:

PromiseOrValue<
  | ExecutionResult
  | AsyncGenerator<
      AsyncExecutionResult | ReadonlyArray<AsyncExecutionResult>,
      void,
      void
    >
>

Option D: @defer/@stream payloads are wrapped in an object with an incremental array field. hasNext is hoisted to the wrapping object. The initial payload may optional also contain the "incremental".

Example:

{
  "incremental": [
    {
      "label": "filmsStream",
      "path": ["person", "films", 2],
      "data": [{ "title": "Return of the Jedi" }]
    },
    {
      "label": "filmsStream",
      "path": ["person", "films", 3],
      "data": [{ "title": "Some title for item #3" }]
    }
  ],
  "hasNext": false
}

References

• Outside GraphQL: https://medium.com/netscape/async-iterators-these-promises-are-killing-my-performance-4767df03d85b

Descision

Discussed in the July 2022 WG meeting and decided to move forward with Option D

[robrichard]

from @yaacovCR in #38

Implementation

See yaacovCR/graphql-executor#154 for a worked example

Suggested type signature for execute:

export function execute(
  args: ExecutionArgs,
): PromiseOrValue<
  | ExecutionResult
  | AsyncGenerator<ReadonlyArray<AsyncExecutionResult>, void, void>
> {
  ...
}

Suggested type signature for subscribe

Same as above.

Note that graphql-executor does not have a separate subscribe function, as subscription operations have been completely integrated into the execution code/algorithm, so the above signature already is the de facto signature within graphql-executor for the corresponding graphql-js subscribe function.

Note also that a choice was made to simplify the signature for subscriptions, as the signature could have theoretically been:

export function subscribe(
  args: ExecutionArgs,
): PromiseOrValue<
  | ExecutionResult
  | AsyncGenerator<ExecutionResult | ReadonlyArray<AsyncExecutionResult>, void, void>
> {
  ...
}

If a subscription operation does not use defer or stream, it should yield only single ExecutionResults rather than arrays. Even if it does include defer/stream, depending on initialCount values and/or other factors, the same operation may sometimes result in a single value or an array. For simplicity, the implementation wraps single values in arrays. The extra wrapping (and later unwrapping) (1) allows subscribe to have the same, simpler signature as query and mutation operations and (2) allows the caller to not have to check to see whether the generator yielded an array or not, as an array is always yielded.

[yaacovCR]

My very strong preference is for below Option E (as an extension/variation of option D), where defer/stream payloads are wrapped, and stream payloads are also combined: This is an easy pickup of the third type of batching discussed within #38:

For the following query from the test suite, modified only as to add labels:

query {
  nestedObject {
    ... DeferFragment @defer(label: "Defer")
  }
}

fragment DeferFragment on NestedObject {
  slowField
  asyncIterableList @stream(initialCount: 0, label: "Stream") {
    name
  }
}

{
  data: {
    nestedObject: {},
  },
  hasNext: true,
}

{
  incremental: [
    {
      data: {
        slowField: 'slow',
        asyncIterableList: [],
      },
      label: 'Defer',
      path: ['nestedObject'],
    },
    {
      data: [{ name: 'Luke' }, { name: 'Han' }, { name: 'Leia' }],
      path: ['nestedObject', 'asyncIterableList', 0],
      label: 'Stream',
      hasNext: false,
    },
  ],
  hasNext: false,
}

The idea is that the emitted payloads within the incremental array will all be unique by label/path.

Pros of option E (with 2 and 3 also true for option D):

1. Third type of batching is accomplished. In general, the client has to do a certain amount of payload processing, i.e. determining whether the payload is of type DEFER or of type STREAM (necessary to determine how to update the store: insert into object vs array) as well as reading of the path. With this type of batching, a smaller number of payloads have to be processed by the client. This can become very significant with long lists that are streamed with many results available concurrently. This is information that the server already has and can easily provide to the client, but it is discarded with the other options A-D. With option E, this information is preserved. Note that this type of batching is distinct from chunking (delaying stream payloads until a certain number of items has been completed), the second type of batching discussed in #38. The first type of batching is accomplished with all of these options.
2. With this nested object structure, there is space for specifying within an individual stream whether it has terminated (same as option D), raised previously by @IvanGoncharov
3. With this nested object structure, we can change the normative level of @defer and @stream from SHOULD to MUST (also raised quickly by @IvanGoncharov when he suggested option D). This was changed to SHOULD, because it was felt that clients might want to immediately send the data when available, ignoring defer/stream, if this would optimize delivery, i.e. prevent numerous payloads with use of defer/stream. We can now accomplish this simply using the batching structure above, without the ambiguity of leaving this unspecified.

[benjie]

Regarding point 3, are you indicating that the entire GraphQL result could come in one payload should the server determine there's no point in @stream/@defer for it? I like this, but would the original payload be inside the incremental or additionally at the root?

{
  // 👇 EITHER here: (Ivan says no)
  data: {
    nestedObject: {},
  },
  // 👆

  incremental: [
    // 👇 OR here: (Ivan says yes!)
    {
      data: {
        nestedObject: {},
      },
      path: [],
    },
    // 👆

    {
      data: {
        slowField: 'slow',
        asyncIterableList: [],
      },
      label: 'Defer',
      path: ['nestedObject'],
    },
    {
      data: [{ name: 'Luke' }, { name: 'Han' }, { name: 'Leia' }],
      path: ['nestedObject', 'asyncIterableList', 0],
      label: 'Stream',
      hasNext: false,
    },
  ],
  hasNext: false,
}

[IvanGoncharov]

@benjie In proposal (option D), it was exactly as in your example.
Idea is that we always have the same shape, so clients can do stuff like code generation and have the predictable shape of the response (save a bunch of ifs in client code).

[benjie]

@IvanGoncharov Sounds good! My example has the initial payload in two positions, which of the two was correct? (Look for the 👇 emojis)

[IvanGoncharov]

Inside incremental. So shapes inside data are always the same even if the "incremental" part is included in the first response.

[yaacovCR]

Also consider Option F:

{
  incremental: [
    {
      data: {
        slowField: 'slow',
        asyncIterableList: [{ name: 'Luke' }, { name: 'Han' }, { name: 'Leia' }],
      },
      label: 'Defer',
      path: ['nestedObject'],
    },
    {
      label: 'Stream',
      hasNext: false,
    },
  ],
  hasNext: false,
}

This relates to the third point above: the server/executor also "knows" that the Defer payload is the parent of the Stream payload and that it is being sent within the current batch; that information can be used to move the Stream payload data into the Defer payload. Note that the Stream payload could/should still be included to communicate that the Stream payload has terminated.

Note:

1. We could allow the Stream payload to drop in this particular instance because the final hasNext: false on the entire incremental dataset implies that all streams have terminated.
2. We could allow options D, E, and F to all be spec-conformant.

Consider also nested defer behavior:

    query HeroNameQuery {
      hero {
        id
        ...NameFragment @defer
      }
    }
    fragment NameFragment on Hero {
      name
      friends {
        ...NestedFragment @defer
      }
    }
    fragment NestedFragment on Friend {
      name
    }

For options D and E, the first payload is the same.

{
  data: {
    hero: { id: '1' },
  },
  hasNext: true,
}

For option E, the second payload could be:

{
  incremental: [
    {
      data: {
        name: 'Luke',
        friends: [{}, {}, {}],
      },
      path: ['hero'],
    },
    {
      data: { name: 'Han' },
      path: ['hero', 'friends', 0],
    },
    {
      data: { name: 'Leia' },
      path: ['hero', 'friends', 1],
    },
    {
      data: { name: 'C-3PO' },
      path: ['hero', 'friends', 2],
    },
  ],
  hasNext: false,
}

While for option F, it could be:

{
  incremental: [
    {
      data: {
        name: 'Luke',
        friends: [{ name: 'Han' }, { name: 'Leia' }, { name: 'C-3PO' }],
      },
      path: ['hero'],
    },
  ],
  hasNext: false,
}

I think E is better from a typings perspective. To avoid the verbosity, you could slightly rewrite the query, assuming you want the results to all come in at once:

    query HeroNameQuery {
      hero {
        id
        ...NameFragment @defer
      }
    }
    fragment NameFragment on Hero {
      name
      ...NestedFragment @defer
    }
    fragment NestedFragment on Friend {
      friends {
        name
      }
    }

We could also purposefully not specify between D, E, and F....

[benjie]

I favour option D; I can even see this being extended to subscriptions. It's common to have a subscription that's can sometimes kick out events at a high rate such that it can, when implemented naively, cause significant re-rendering issues on the client. Allowing the subscription to batch these payloads together into one payload to deliver to the client could be nice; but it raises these concerns:

1. clients would need to "opt in" to this, otherwise it would be a breaking change. Perhaps with a directive such as @batch(duration: 0.2) to batch events together over a 200ms interval.
2. is "incremental" the right name for this? Maybe something more generic like "batch", "payloads", or "results" would make sense?

(Point 2 is why I raise this comment; though I know it's bikeshedding!)

[yaacovCR]

Thanks for your response? Why D instead of E or F? Should we allow all three (current normative level suggests we sort of do)

[benjie]

Seems to me that D, E and F are all compatible-ish, so rather than arguing over specifics I figure that it makes sense to pick a general direction, and once that's decided we can argue over the more subtle aspects.

[robrichard]

If subsequent payloads are ready at the same time the initial payload is ready, what does option D look like for the initial payload? Something like this?

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

First 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"
        }
      ]
    }
  },
  "incremental": [
    {
      "path": ["hero", "heroFriends", 0],
      "data": {
        "homeWorld": "Tatooine",
      }
    },
    {
      "path": ["hero", "heroFriends", 1],
      "data": {
        "homeWorld": "Corellia",
      }
    }
  ],
  "hasNext": true
}

Second Payload

{
  "incremental": [
    {
      "path": ["hero", "heroFriends", 2],
      "data": {
        "homeWorld": "Alderaan",
      }
    }
  ],
  "hasNext": false
}

[benjie]

In this comment Ivan suggested that the initial data would go directly into incremental. I didn't have much of an opinion at that point, but having ruminated on it for a while, I think that the initial payload should still honour GraphQL's current contract which is that if there's no "data" key then it failed. So I think the way you propose here is preferable: initial payload in "data" and follow up ones in "incremental" 👍

[IvanGoncharov]

Also agree. Here is more context behind my comment that @benjie referencing:
I would say typical server will have some small timeout and will batch everything it can before timeout is hit. Because some stuff can be async but still fast (e.g. already in the cache) server needs the ability to return it inside the initial payload.

Previously we discussed an option where the server can effectively ignore @defer or @stream and just return data inlined in top-level "data".
I think it's bad and leads to a client being required to check if "deferred data is present" or if it should wait for it. Also it creates problems for codegen in statically typed languages where "data" is deserialized to "native types" since it requires deferred fields to be present but being optional at the same time (even if fields are non-null).

So with those arguments in mind, I propose to say that:
If the server exposes @stream/@defer through introspection it means SHOULD send those data separately but it MAY batch it inside "incremental" of the first payload.

[glasser]

I will say that from a typing perspective, it would be nice if all messages had the same structure rather than having the first message be different. But I do also like your point about being able to check data in the first message (though one could always just check incremental[0].data).

From a graphql-js perspective, perhaps a new API that supports incremental delivery could return something like this:

  {result: ExecutionResult} |
  {firstResult: InitialAsyncResult, subsequentResults: AsyncGenerator<SubsequentAsyncResult>}

[robrichard]

I updated option D in the description to

defer/stream payloads are wrapped in an object with an incremental array field. hasNext is hoisted to the wrapping object.

Discussion around a hasNext like field for detecting the end of individual streams can be discussed in #18

[yaacovCR]

It might be helpful for clients to have hard-baked into the response which are the non-incremental fields so that this data could be updated quicker than the stuff in the incremental section.

[yaacovCR]

An advantage of option B or C is that even by separating data from incremental in option D (and it’s cousins E and F), I think in general the execution result is an unordered map so there would be no way to guarantee the initial result returns prior to the incremental section.

[yaacovCR]

Actually, thinking this through further, I think might be reasonable that the initial result should always be sent on the wire as its own separate payload, even if incremental payloads available, as opposed to later payloads which should be batched in some way

[glasser]

Has there been a resolution on this discussion?

[robrichard]

This was discussed in the July 2022 WG meeting and the group decided to move forward with Option D.

• Spec pr is now updated to reflect this decision
• The agreed on format is implemented in graphql-js, but without batching currently (only one object included in incremental: []) (edit: now also batches results)

[yaacovCR]

Request for clarification:

From above Option D:

The initial payload may optional also contain the "incremental".

The algorithm within Section 6: Execution also includes the "incremental" entry when possible within the initial response:

• Initialize {initialIncremental} to an empty list.
• For each {record} in {initialRecords}:
  • 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 {initialIncremental}.
• If {initialIncremental} is not empty:
  • Add an entry to {initialResponse} named incremental containing the value
    {incremental}.
• Yield {initialResponse}.

However, the spec PR currently includes the following within "Section 7: Response" implying that the initial payload cannot contain the "incremental" entry:

When the response of the GraphQL operation is a response stream, the first value
will be the initial response. All subsequent values may contain an incremental
entry, containing a list of Defer or Stream payloads.

Of note, graphql-js reference implementation never includes the "incremental" entry within the initial response, as opposed to the algorithm above.

      if (exeContext.subsequentPayloads.size > 0) {
        return {
          initialResult: {
            ...initialResult,
            hasNext: true,
          },
          subsequentResults: yieldSubsequentPayloads(exeContext),
        };

[robrichard]

I think this warrants its own discussion #60