Return arrays from stream payloads

[robrichard]

Context

In the Feb 2022 WG meeting @leebyron & @yaacovCR proposed changing the stream payload response.

1. The data field on the payload would change to an array containing the streamed list element
2. A new field atIndex is added with the array index.
3. The path field would point to only the list location, not the index of the list item.

Initially we would only support one single element returned in the stream array, but we should consider how this payload can be utilized in a future spec amendment to support batches of results or unordered sets.

Example (containing both defer and stream for completeness):

query {
  person(id: "cGVvcGxlOjE=") {
    ...HomeWorldFragment @defer(label: "homeWorldDefer")
    name
    films @stream(initialCount: 2, label: "filmsStream") {
      title
    }
  }
}
fragment HomeWorldFragment on Person {
  homeworld {
    name
  }
}

Response Payloads

Payload 1

{
  "data": {
    "person": {
      "name": "Luke Skywalker",
      "films": [
        { "title": "A New Hope" },
        { "title": "The Empire Strikes Back" }
      ]
    }
  },
  "hasNext": true
}

Payload 2

{
  "label": "homeWorldDefer",
  "path": ["person"],
  "data": {
    "homeworld": {
      "name": "Tatooine"
    }
  },
  "hasNext": true
}

Payload 3

{
  "label": "filmsStream",
- "path": ["person", "films", 2],
+ "path": ["person", "films"],
+ "atIndex": 2,
- "data": {"title": "Return of the Jedi"},
+ "data": [{"title": "Return of the Jedi"}],
  "hasNext": false
}

The benefits of this change are:

• "data": null always means "error bubbled beyond all fields in this partial-payload" and can never mean "the data for this path was the value null". Valid defer payloads will always have an object set for data and now valid stream payloads will always have an array set for data. See discussion here: Non-nullable field errors in defer/stream payloads #23 (comment)
• This potentially makes it easier to add support for providing batches of results.
• atIndex is an additional data point clients can use to differentiate between stream payloads and defer payloads.

Decision

Discussed at the April 2020 WG meeting and decided to move forward with data enclosed in an array in stream payloads. Discussion around how the index is represented will be continued in #39

Consequences

• The data value for a defer payload is an instruction to "merge" at the specified path. Now the data value for a stream payload is an instruction to "concat" at the specified path, instead of providing the value at the specified path.

[robrichard]

@josephsavona Any thoughts on this from a Relay perspective?

[yaacovCR]

A well performing data layer makes a lot more sense than a non compliant executor —- but as graphql aims to provide type/null safety for non compliant data layers, this also suggests we cannot/should not assume nulls will be swallowed…

[josephsavona]

@yaacovCR no we shouldn’t assume all nulls will be swallowed, but optimizing for common cases does make sense. See my comment about how named fragment spreads achieve a compromise.

[yaacovCR]

I see the compromise but in practice I am still confused. We are trying to make it clear from response only without knowledge of schema whether a list item was null or whether a null bubbled up to the list because the list items are non nullable.

a client might want to know this because null list items would get integrated into the data store, while if the null bubbled up to the list itself, it wouldn’t. A client could check the schema to figure out which one happened, but everyone is seemingly agreeing that we want it to be obvious from the response.

the problem I have is that @josephsavona suggestion seems to imply that if a client would want to distinguish these cases, it would have to transform all of its streamed fields to use named fragments. Meaning, even though perhaps only some clients would want to distinguish these cases, those clients would presumably want to do so for all operations with streams.

End users creating queries/operations should not need to know anything about how their clients work, and so the burden of adding the error boundary with the named fragment should definitely be on the client and not the query author.

but some clients do not regularly do any query rewriting or manipulation. It would probably be easier just to check the schema and see if the field is nullable than to transform all queries. So this optional method, while theoretically possible seems like so great a burden it would rarely be used.

Perhaps it’s not necessary to distinguish between these cases, null list items and null bubbling up the list purely from within the stream payload response. But if it is worth it, named fragments does not seem like a path that would likely be used.

[josephsavona]

We are trying to make it clear from response only without knowledge of schema

Ah, yeah i think this is the disconnect. The power of GraphQL fundamentally stems from the use of a schema: clients that don't make use of the schema are limited in the capabilities they can provide, and the solutions we come up with in the spec to support them increase redundancy for everyone. I do think it should be possible for schema-ignoring clients to distinguish "value was null" from "value was nulled out" if that matters — but my pushback is whether that should be the default behavior. Named fragments would allow clients that actually care to distinguish.

[yaacovCR]

Fwiw the graphql-executor pr now supports both @josephsavona style batching of all available payloads as well as delaying stream payloads until an interval has passed or a certain chunk size has been reached.

It also supports parallel streaming, in which case an atIndices array describes the paths/indices of the corresponding returned results within the data array, instead of just a single atIndex value.

@robrichard @nodkz @josephsavona

[yaacovCR]

I think that field resolvers that return an async iterable should return an async iterable of iterables, ie an async iterable that returns chunks of the larger list.

[yaacovCR]

@robrichard any thoughts

[yaacovCR]

A class would require instanceof dynamic checks which we currently don’t really have, although we check for returned errors with instanceof, implementers can always throw errors without instanceof check

graphql executor works across realms, we want to use duck typing rather than nominal (if I am using terms correctly)

also, to get batching to work in resolvers, with a class, resolvers would depend on graphql js, not the hugest deal but previously resolvers could be dependency free functions

[yaacovCR]

Basically the allowed return types of the async iterable in my vision would correspond to the possible types of the data field in the stream payload when the field is streamed. If it’s not streamed, the executor would roll everything into one list.

[n1ru4l]

To me, the solution proposed by @IvanGoncharov in #32 (reply in thread) adds unnecessary complexity that is not really needed.

Having the same well-documented API (always yield an array) feels like a simpler approach than having an overloaded API (either single item or GraphQLListValueSlice(items)).

Furthermore, as TypeScript support within this library becomes better the rules could be enforced by stricter type-interference/checks.

[yaacovCR]

update: the graphql-executor WIP PR yaacovCR/graphql-js#154 now supports arbitrary chunkSizes based on the above approach.

[nodkz]

I consider @stream like AsyncIterator. If we want to provide BatchAsyncInterator why not create a new directive @streamBatch(batchSize: 100, batchTimeoutMs: 50, ...rest_stream_args).

Let's keep the data field in @stream with any type (scalar, object, array), but for @streamBatch it will be an array of any type.

PS. A little bit more thoughts about defer == Promise & stream == AsyncIterator: #23 (comment)

[yaacovCR]

@nodkz see the first and third benefits outlined by @robrichard above at #32 (comment) How do these seem to you, do you have in mind any alternatives?

[nodkz]

"data": null always means "error bubbled beyond all fields in this partial-payload" and can never mean "the data for this path was the value null". Valid defer payloads will always have an object set for data and now valid stream payloads will always have an array set for data. See discussion here: #23 (comment)

I consider stream like AsyncIterator. So how does it return an error? Just throw an Error without a data field.

atIndex is an additional data point clients can use to differentiate between stream payloads and defer payloads.

This is a good param. But in terms of AsyncIterator better to name it like idx or seqId.

[nodkz]

If a client wants to work with batching it might be

• streamBatch(maxBatchSize: 100, maxBatchTimeout: 50) - additional directive with specific logic
• or regular stream which works like AsyncIterator where batching logic is moved to the schema implementation side, something like:

query {
  filmStream @stream() {
      batch(maxSize: 100) {
        title
      }
  }
}

Payload 1 (Initial payload)

{
  "data": {
     "filmStream": []
  },
  "hasNext": true
}

Payload 2 (first 100 films)

{
  "path": ["filmStream"],
  "idx": 1,
  "data": {
      "batch": ["Film 1", "Film 2", ..., "Film 100"],
  },
  "hasNext": true
}

Payload 3 (second 100 films)

{
  "path": ["filmStream"],
  "idx": 2,
  "data": {
      "batch": ["Film 101", "Film 102", ..., "Film 200"],
  },
  "hasNext": true
}

Payload 4 (just 30 records)

{
  "path": ["filmStream"],
  "idx": 3,
  "data": {
      "batch": ["Film 201", "Film 202", ..., "Film 231"],
  },
  "hasNext": false
}

Payload 5 (Film 232 produces an error) 🛑

{
  "path": ["filmStream"],
  "idx": 4,
  "errors": [{...}],
  "hasNext": true
}

Payload 6 (continue from Film 233 + further 99 records)

{
  "path": ["filmStream"],
  "idx": 5,
  "data": {
      "batch": ["Film 233", "Film 234", ..., "Film 332"],
  },
  "hasNext": false
}

[yaacovCR]

I consider stream like AsyncIterator. So how does it return an error? Just throw an Error without a data field.

This was the initial suggestion in the working group. The hesitation stemmed -- I think -- from the fact that errors without data might mean different things depending on the context:

for an initial response = errors were encountered prior to execution
for defer payloads = would never happen
for stream payloads = would be a null bubbling up from the list item to the field.

Under the above proposal to return arrays, stream would be similar to defer and there would never be errors without data -- at least as I understand it.

Having said that, you may think that it would be better to just do overload the errors-only case rather than introduce the array, I could see how that in some respects might be simpler, but I tend to think that using a null versus an array is more straightforward.

[benjie]

I'm skeptical of the need for atIndex.

• Currently we require the payloads to be in order, so atIndex is redundant information that inflates the payload size
• Clients need to figure out what to do if the atIndex is a value different to that which they expect (i.e. not the length of the array they already have)
• We need to consider what would happen if the server gives an atIndex that's massive (e.g. 999999999) - does the client have to allocate a list that big just to set those entries? Seems like a possible security issue for a client to a malicious GraphQL server

One thing in its favour is it allows for us to do out-of-order delivery later, but I wonder if that's sufficiently likely and sufficiently valuable to justify these tradeoffs.

[yaacovCR]

I'm skeptical of the need for atIndex.

I believe @robrichard is in your corner, it can basically be appended t the end of the path

• Currently we require the payloads to be in order, so atIndex is redundant information that inflates the payload size
  [sic] One thing in its favour is it allows for us to do out-of-order delivery later, but I wonder if that's sufficiently likely and sufficiently valuable to justify these tradeoffs.

graphql-executor already supports batched and parallel ("out-of order") delivery -- I don't believe the initial release at graphql-js will support this, but it's not too difficult to foresee a future where this is useful and available.

• Clients need to figure out what to do if the atIndex is a value different to that which they expect (i.e. not the length of the array they already have)

Wouldn't that still be true if we put the index back in the path? The atIndex value is the same value that used to be the last entry in the path key.

• We need to consider what would happen if the server gives an atIndex that's massive (e.g. 999999999) - does the client have to allocate a list that big just to set those entries?

That would seem to be a problem just in general, even if the value currently proposed to be in atIndex was put back into the path.

Seems like a possible security issue for a client to a malicious GraphQL server

Do you mean from a malicious GraphQL server?

[robrichard]

Standalone index discussion opened: #39

[benjie]

Wouldn't that still be true if we put the index back in the path? The atIndex value is the same value that used to be the last entry in the path key.
That would seem to be a problem just in general, even if the value currently proposed to be in atIndex was put back into the path.

I don't believe it belongs in the path NOR atIndex. I think it's completely redundant and should be removed. But if it is kept, I would much rather see it as atIndex and not in path. It definitely feels wrong being in path to me.

Seems like a possible security issue for a client to a malicious GraphQL server

Do you mean from a malicious GraphQL server?

I meant:
Seems like a possible security issue for (a client to a malicious GraphQL server)

I think maybe you read:
(Seems like a possible security issue for a client) to a malicious GraphQL server

English is fun 😆

[benjie]

I've gone into more detail in my reasoning in the issue Rob linked above: #39 (comment)

[robrichard]

Discussed at the April 2020 WG meeting and decided to move forward with data enclosed in an array in stream payloads. Discussion around how the index is represented will be continued in #39

[robrichard]

The defer-stream branch on graphql-js is now updated to the behavior we agreed on in the WG meetings:
Stream payloads are now wrapped in [] and non-null errors will bubble up to "data": null

[glasser]

Note for posterity: while the decision to send the data in an array still holds, we are considering renaming the field from data to items in #42