Non-nullable field errors in defer/stream payloads

[robrichard]

Context

If a resolver returns null for a field on an output type that is declared NonNull, there should be an error. Additionally, the GraphQL spec states

If the field which experienced an error was declared as Non-Null, the null result will bubble up to the next nullable field.

This may not be possible when defer and stream are used because the next nullable field may have already been returned to the client.

What's implemented in graphql-js now: The data field on the payload will be returned as null, with the correct non-nullable error. This should be called out in the spec.

Example - @defer

query {
  hero {
    id
    ... @defer {
      nonNullErrorField # type is String! but resolves to `null`
    }
  }
}

Response

{
  "data": {
    "hero": {
      "id": "1"
    }
  },
  "hasNext": true
},
{
  "data": null,
  "path": ["hero"],
  "errors": [
    {
      "message": "Cannot return null for non-nullable field Hero.nonNullErrorField.",
      "locations": [
        {
          "line": 9,
          "column": 9
        }
      ],
      "path": ["hero", "nonNullErrorField"]
    }
  ],
  "hasNext": false
}

Example - @stream

query { 
  asyncIterableNonNullError @stream(initialCount: 1) { # type is [String!], second result resolves to null
    name
  }
}

Response

{
  "data": {
    "asyncIterableNonNullError": [
      {
        "name": "Luke"
      }
    ]
  },
  "hasNext": true
},
{
  "data": null,
  "path": ["asyncIterableNonNullError", 1],
  "errors": [
    {
      "message": "Cannot return null for non-nullable field Query.asyncIterableNonNullError.",
      "locations": [
        {
          "line": 3,
          "column": 9
        }
      ],
      "path": ["asyncIterableNonNullError", 1]
    }
  ],
  "hasNext": false
}

Decision

Discussed at the April 2020 WG meeting in the context of: #32
With the decision to wrap stream data in an array, it is no longer ambiguous to determine if an error would have bubbled above the defer/stream payload boundary.

[yaacovCR]

Just as straw man, only sensical alternative I can think of is to disallow stream a nonnullable list or to defer any subfields of a non nullable field, which I don’t think is necessary. Was this discussed in wg? As far as I can tell, this is last outstanding issue!

[josephsavona]

What's implemented now seems...fine? Ie, defer/stream conceptually act as nullable boundaries.

[robrichard]

I'm currently leaning towards

• defer - What's implemented now is ok because a payload from a @defer directive that bubbled up and returns "data": null doesn't break any non-nullable guarantees to the client. None of the fields in the fragment will be set to null, just a null result is returned on the payload.
• stream - I'm thinking of adding a validation rule to prevent stream on list types that have a non-null inner type to avoid any issues. Perhaps it can be addressed later with client-controlled nullability?

[robrichard]

@benjie pointed out that a validation rule preventing stream on list types that have a non-null inner type cannot be added. Nullable fields must be able to be changed to non-nullable fields without breaking existing queries. A previously valid query with @stream would become invalid if the inner type of the list is changed to non-null.

[leebyron]

For posterity from meeting:

My recommendation was to make sure "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", in this case based on @yaacovCR's recommendation to make sure @stream payloads are always arrays for normal results.

Also, adding "atIndex" makes it clear that this was a payload resulting from a @stream vs @defer.

This address the non-null error in a stream payload here, but warrants a top level discussion since this would be a significant change.

{
  "data": {
    "asyncIterableNonNullError": [
      {
        "name": "Luke"
      }
    ]
  },
  "hasNext": true
},
{
  "data": null,
  "path": ["asyncIterableNonNullError"],
  "atIndex": 1
  "errors": [
    {
      "message": "Cannot return null for non-nullable field Query.asyncIterableNonNullError.",
      "locations": [
        {
          "line": 3,
          "column": 9
        }
      ],
      "path": ["asyncIterableNonNullError", 1]
    }
  ],
  "hasNext": false
}

[robrichard]

Top level discussion posted here: #32

[robrichard]

Discussed at the Feb 2022 WG meeting

• A validation rule prohibiting stream was ruled out as noted above.
• There was general agreement that defer/stream should act as a null boundary.
• There needs to be a way to distinguish a payload that is null because the null bubbled up from a null error vs a valid null result field.
  • This is not a problem for defer payloads, the data field will always be an object for all valid defer payloads
  • @leebyron recommended changing stream payloads to return arrays in their data field. Discussion topic for this: Return arrays from stream payloads #32
  • Another alternative that was discussed was omitting the data field for null errors

[nodkz]

My 5 cents for @defer & @stream directives after watching GraphQL Working Group - February 3, 2022

Let's keep them KISS. I consider and assume that:

• defer like a Promise
• stream like an AsyncIterator

1. If defer works like a Promise

1.1. Can Promise return null? Yes, it can. So let's defer MAY return null too.
1.2. defer may return data, may NOT. So all returned fields by defer MUST be considered Nullable.
1.3. If defer works like a Promise may return any value. In such a case we can allow using defer for fields, not only for fragments.

2. If stream works like an AsyncIterator

2.1. Can AsyncIterator return null? Yes, it can. So let's stream MAY return null too.
2.2. stream CANNOT be applied for List with NonNull values [Item!]. In such a case, we enforce naive clients to check every received item in the List.
2.3. Should stream support batching? - As AsyncIterator, it shouldn't. Just return any result which was requested by the client. It looks like we try to invent something ... let's call it BatchingAsyncIterator.
2.4. If we consider stream like AsyncIterator then I don't think that it's a good idea to "Changing stream payloads to return arrays in their data field".

3. Batching

BatchingAsyncIterator looks like a YAGNI thing for most users, but anyway for these users, we may provide a @streamBatch directive with additional arguments

• batchSize: Int (maximum number of returned elements in the batch)
• batchTimeout: Int (maximum milliseconds wait period for gathering stream data).

[yaacovCR]

1. If defer works like a Promise

1.1. Can Promise return null? Yes, it can. So let's defer MAY return null too.
1.2. defer may return data, may NOT. So all returned fields by defer MUST be considered Nullable.

I'm not sure I follow the above. defer should try to return data for the requested fields, that data might be the null value, which should bubble up according to the normal rules. Why would all returned fields have to be considered nullable -- we actually have an easy way to distinguish between those cases:

Response without any non-nullable fields, where value of one field is null:

{
  data: {
    fieldA: "A"
    fieldB: null
  }
}

Response with non-nullable fields where at least one value is null:

{
  data: null
}

At first I thought you were talking about the types for responses when defer is used, which I agree currently have to be nullable, because defer means that the data might arrive later. But I guess I'm not 100% sure.

1.3. If defer works like a Promise may return any value. In such a case we can allow using defer for fields, not only for fragments.

I'm not sure what you mean here.... The discussion talking about using defer on fields is here: #4 (comment) -- and the main idea there seems to be about predictable types.

2. If stream works like an AsyncIterator

2.1. Can AsyncIterator return null? Yes, it can. So let's stream MAY return null too.
2.2. stream CANNOT be applied for List with NonNull values [Item!]. In such a case, we enforce naive clients to check every received item in the List.

I'm not sure what you mean here. We want to allow using stream on lists of non-null items. In particular, as discussed in the WG, this is because applying non-null as an output field is supposed to be a "non-breaking" change, i.e. it's not supposed to break any existing queries. (As opposed to removing non-null, which could break an existing query, and the reverse for input values).

And are you saying that it's bad to force clients to check every item on the list? Meta/facebook apparently suppresses list item nulls, whether stream is used or not. I don't think that's part of the spec, so I don't see how that can be supported in this proposal. I think we are stuck then forcing clients to check in some way, and the idea would be that we could either signify that a null bubbled up the entire list by using null instead of a single item array (if stream payloads return an array in their data field) or by leaving out the data property entirely (if stream payloads send the list item in the data field).

Please feel free to clarify!!! Do you not think we should support stream on lists of non-nullable items?

2.3. Should stream support batching? - As AsyncIterator, it shouldn't. Just return any result which was requested by the client. It looks like we try to invent something ... let's call it BatchingAsyncIterator.

I agree with you here on the logic, but just come to the opposite conclusion. If we want to equate stream with AsyncIterators, then we can't have more than one result. I just don't feel compelled to equate the two. Certainly, I think the default should be to stream items as single items, but I don't really see why clients shouldn't be able to request the items in chunks/batches. It seems like something that might be useful, especially for servers/transports that don't support batching entire payloads at that level.

2.4. If we consider stream like AsyncIterator then I don't think that it's a good idea to "Changing stream payloads to return arrays in their data field".

Same as above.

3. Batching

BatchingAsyncIterator looks like a YAGNI thing for most users, but anyway for these users, we may provide a @streamBatch directive with additional arguments

• batchSize: Int (maximum number of returned elements in the batch)
• batchTimeout: Int (maximum milliseconds wait period for gathering stream data).

I think the batchTimeout is a very interesting idea, will try to get that going in graphql-executor

Do we for sure need another directive, though -- even if we keep data as the list item rather than an array as to your preference, maybe we could say that for the same stream directive, when batchSize/batchTimeout options are used, data should in fact be an array.

[nodkz]

1. If defer works like a Promise

1.1. Can Promise return null? Yes, it can. So let's defer MAY return null too.
1.2. defer may return data, may NOT. So all returned fields by defer MUST be considered Nullable.

At first I thought you were talking about the types for responses when defer is used, which I agree currently have to be nullable, because defer means that the data might arrive later. But I guess I'm not 100% sure.

Exactly! When the client marks a set of fields as @defer they will be returned as null values in the initial response. So let's consider @defer like the client declared in its query that these fields are considered as non-required (nullable). In such a case the client takes responsibility for handling null values in their code. And no matter if these fields are NonNull in the schema or not. So any deferred field MUST is considered Nullable.

I'm not sure what you mean here.... The discussion talking about using defer on fields is here: #4 (comment) -- and the main idea there seems to be about predictable types.

I mean that I want to see @defer on fields too. And the main idea there seems to be about --- let's enforce client to use defer only with fragments because in our use cases and in our observation with Apollo Client we got such best practice. They may restrict it via eslint rules for their codebase, but not restrict the entire Specification.

Anyway thanks for pointing #4 (comment) I'll write there my thoughts. Let's discuss defer on fields there.

2. If stream works like an AsyncIterator

2.1. Can AsyncIterator return null? Yes, it can. So let's stream MAY return null too.
2.2. stream CANNOT be applied for List with NonNull values [Item!]. In such a case, we enforce naive clients to check every received item in the List.

I'm not sure what you mean here. We want to allow using stream on lists of non-null items. ... And are you saying that it's bad to force clients to check every item on the list? ... Do you not think we should support stream on lists of non-nullable items?

I think that if the client mark field as @stream then it MUST take responsibility for checking values for nullability in runtime. I suggest here the same behavior as with defer as I suggested above. If the field in schema is declared as [Item!]! then with @stream directive on it then the client MUST consider this field like [Item]! (array with nullable items).

It helps to set correct error boundaries and eliminate problems with bubbling NonNull errors to initial response. What is already received is correct and validated. Subsequent defer/stream chunks CANNOT invalidate the already received initial response. And it can be achieved if fields marked as @defer/@stream became Nullable by nature in runtime.

2.3. Should stream support batching? - As AsyncIterator, it shouldn't. Just return any result which was requested by the client. It looks like we try to invent something ... let's call it BatchingAsyncIterator.

I agree with you here on the logic, but just come to the opposite conclusion. If we want to equate stream with AsyncIterators, then we can't have more than one result. I just don't feel compelled to equate the two. Certainly, I think the default should be to stream items as single items ...

Honestly, I like the idea of batching streamed results. But I don't want to receive the @defer directive in several years while it's blocked by @stream discussions. That's why I tried to suggest extracting batching logic and its discussions to another proposal via a new directive like streamBatch and keep @stream as simple as possible. Because batching addition increased complexity for @stream directive, added new rounds of discussions and started blocking the most awaiting thing for me - the @defer directive.

[yaacovCR]

I guess you are saying that to some extent when using stream, the internal list items become practically nullable because they cannot go back in time and null the list in the initial response. But I think this is a bit confusing terminology — a client could indeed decide to null out the initial response in the store when it receives a later streamed null if the list is of non-nullable items. I think that’s beyond the scope of the spec as we are just describing the graphql response, not what to do with it, but I think it’s important for this reason not to jump to say that stream changes the nullable status of the list items…

[nodkz]

As an alternative solution for batched streams, I can suggest the following solution

• if some record has an error, then return it in a separate chunk
• if a record is valid then it is able to be added to the batch chunk with other valid records

In such a case, clients receive batched streams without errors and may add them blindly to the store. And if it's required decide what to do with errored items in dedicated chunks:

• invalidate response and start bubbling null checks up (I consider such behavior quite strange. Application starts displaying partial response and later wipes UI, after receiving additional chunks with errors).
• Or just ignore these errors, keep only valid items in the list. And maybe display somewhere in UI that some items had problems.

Anyway, for me, making streamed list items as nullable looks good.
Clients get a nullable list and are responsible to handle it correctly - ignoring it or displaying it in the UI list as an errored item. So stream by nature is a delayed response, moreover in streaming may happen any errors with data retrieving - AND making some NonNull guarantees for lists is absolutely unreal.

EITHER we await all data before streaming (what makes streaming worthless) OR we just make lists nullable by spec.

[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

[robrichard]

Spec PR is also updated to reflect this

[glasser]

@robrichard I'm not sure I see where in the spec PR we're explicit that defer/stream forms a null boundary. Should this be mentioned somewhere in "Handling Field Errors"?

[robrichard]

It was only mentioned in the response section, but I've now also added it to Handling Field Errors and the defer and stream execution functions