Spec edits for incremental delivery, Section 3 & 7 only

[robrichard]

Extracted from the full PR (#1110) and targeting an integration branch to aid in review.

Helpful reference material:

• Response format examples: New response format (June 2023) defer-stream-wg#69
• GraphQL Conf talk: https://www.youtube.com/watch?v=LEyDeNoobT0

[Keweiqu]

Thank you so much @robrichard for this amazingly written edits! What a great progress!! See my inline comments.

And let me know if you want me to raise the error handling as a separate PR or a separate topic in WG discussion.

[Keweiqu]

Nits: let's move this after the @deprecated section because these are not required. Also should we mention that implementations not implementing @defer and @stream when facing a request with @defer and @stream will ignore these directives? (this might be specified somewhere else already as a general rule for unknown directive)

[robrichard]

Moved and added a sentence with a link to the Directives Are Defined validation rule which will fail if defer or stream is used on a service that does not implement them.

[Keweiqu]

nits: let's not mention "initial response" because for the case of @defer nested inside @stream. It's not always guaranteed to be in response #0.

[robrichard]

Updated!

[Keweiqu]

do you want to add a comment about validation error if this directive is applied on a non-list type?

[robrichard]

Added a sentence mentioning the "Stream Directives Are Used On List Fields" rule. It's a broken link for now, but we have this rule defined in the full PR

[Keweiqu]

Love this section!!!

[Keweiqu]

"This entry must not be present for GraphQL operations that return a single response map"

Why? Seems like an unnecessary restriction.

[robrichard]

We discussed this some time ago and landed on this: graphql/defer-stream-wg#57 (comment)

I think the main reason was a fear that clients may start to depend on hasNext: false being present in a single payload response, and this would break if they switch to a server that does not support defer/stream

[Keweiqu]

Ohhhhh yeah now I remember!

[benjie]

I think the wording isn't quite right though. I think if the request uses @defer/@stream then it's okay to have hasNext: false but if the request did not use @defer or @stream then the result must not include hasNext. Does that differ from what we discussed before?

[Keweiqu]

there is some ambiguity with this statement. When I read this it seems like with a fragment X {a, b, c} which is deferred, the GraphQL service can incrementally deliver {a, b} and then deliver {c}

[robrichard]

I agree that it is ambiguous. I wrote this way because I was thinking that your example is technically possible. If a and b are in both a deferred and non-deferred fragment, a & b will be in the initial response and only c will be incrementally delivered. Or if a and b are in both deferred fragment X and deferred fragment Y, a & b might be delivered incrementally when fragment Y is complete, while c will be sent in a later payload.

Do you have a suggestion to clarify?

[Keweiqu]

what about index? Do you want me to raise this as a separate PR?

[robrichard]

When we switched to the latest response format we decided not to put index in the incremental object. It's not strictly necessary since list items must be delivered in order. We were thinking it could be added in a future spec update if we want to support out of order or sparse responses. If this is an issue for you can you start a new discussion topic in https://github.com/graphql/defer-stream-wg/discussions?

[robrichard]

@Keweiqu thanks for your review! I have addressed your comments and also moved the examples into a new appendix as discussed during the working group meeting.

[martinbonnin]

Can we clarify what is meant by implementation here? My current understanding is that an "implementation" is typically a library, such as graphql-js or graphql-java while a "service" is typically a server or a collection of servers, such as the GitHub or Shopify APIs.

If that's the case, the "not required" part could be confusing. If I'm writing the graphql-cobol library, can I decide to leave @defer out and still claim compatibility with the spec?

[robrichard]

@martinbonnin thanks for reviewing. I updated this to say services instead of implementations as I was referring to servers here

[martinbonnin]

Thanks for the clarification 🙏

Would it make sense to use language similar to @specifiedBy and @oneOf:

GraphQL implementations that support the type system definition language should provide the
@defer directive if representing custom scalar definitions.

This is the closest to feature discovery we have in GraphQL and using common language pattern would make the spec more palatable. Or is there something fundamentally different about @defer and @stream?

[robrichard]

I'm not sure this applies to @defer and @stream, because they are directives that apply to "executable locations", whereas @specifiedBy and @oneOf only apply to "type system location".

I think a GraphQL server could not support the "type system definition language" but still support @defer and @stream. Clients could determine if @defer and @stream are supported by using the introspection query:

query {
  __schema {
    directives {
      name
    }
  }
}

I think the inverse could also be true. A GraphQL Server that supports the type system definition language may have no desire to support @defer/@stream and that's ok since we want this feature to be strictly optional.

It's possible I'm not thinking about this correctly, let me know what you think.

[martinbonnin]

Apologies in advance, a bunch of nitpicking on the vocabulary/glossary is coming below.

a GraphQL server could not support the "type system definition language" but still support @defer

I don't think it makes sense for a GraphQL server to support the "type system definition language". A server reads request in and writes responses out. SDL is an implementation detail for the server.

But maybe not for its implementation (==lib?). A lib takes SDL in and generates a compliant service out of it. I always thought of implementation in the spec as being the "lib" but re-reading it, I'm not sure any more.

Regardless, if we use this service vs implementation distinction, I would rephrase like so:

A GraphQL implementation that does not support the "type system definition language"  can
still support @defer and @stream

Clients can determine if @defer and @stream are supported by using the introspection 
query against a service.

A GraphQL service may have no desire to support @defer/@stream and that's ok since we 
want this feature to be strictly optional. 

I think this is the same for @oneOf and @specifiedBy:

A GraphQL implementation that does not support the "type system definition language"  can
still support @oneOf and @specifiedBy

Clients can determine if @oneOf and @specifiedBy are supported by using the introspection 
query against a service.

A GraphQL service may have no desire to support @oneOf and @specifiedBy and that's ok since we 
want this feature to be strictly optional. 

That's why I'm tempted to put them in the same bucket of features even if @defer is obviously quite important for clients to be aware of.

[robrichard]

@martinbonnin I am definitely open to changing this, just want to talk it through to make sure I fully understand.

Is this statement true for @oneOf and @specifiedBy?

A GraphQL implementation that does not support the "type system definition language" can
still support @oneOf and @specifiedBy

Since @oneOf and @specifiedBy are directives that can only be applied to grammar that is part of the "type system definition language", how can an implementation support this directive without supporting the grammar that is required to place the directive? I agree that specifiedByURL introspection field may be supported but this is not the same as the directive.

The same is not true for @defer and @stream because these directives are applied to the execution grammar.

A GraphQL service may have no desire to support @oneOf and @specifiedBy and that's ok since we
want this feature to be strictly optional.

Again, I could be wrong about this but my understand for @specifiedBy is that ideally all GraphQL servers would support it, but that's not possible for servers written before they were added to the spec, so RFC 2119 should is used. Additionally, we further restrict it to "implementations that support the type system definition language" since @specifiedBy can only be placed in the type system grammar.

For @defer and @stream we don't want to use RFC 2119 should/recommended language since it is optional and we expect some servers will choose not to implement it for reasons other than being behind on the latest spec changes.

[martinbonnin]

my understand for @SpecifiedBy is that ideally all GraphQL servers would support it, but that's not possible for servers written before they were added to the spec, so RFC 2119 should is used.
[...]
For @defer and @stream we don't want to use RFC 2119 should/recommended language since it is optional and we expect some servers will choose not to implement it for reasons other than being behind on the latest spec changes.

Gotcha, thanks! So we've got:

1. RFC 2119 must for stuff that all services must support (@include for an example)
2. RFC 2119 should to allow older services to still be compatible but newer services should support (@specifiedBy for an example)
3. explicit language like here for stuff that is completely optional (@defer for an example)

Did I get that right?

If yes, I was missing the 2. vs 3. distinction, thanks for pointing that out 🙏

I'm still not 100% sold on it though. I kind of like that other directives all use the same sentence and feels a bit weird that @defer is not following the pattern (or maybe it's just my OCD...).

What about using RFC 2119 may to denote that it's "more" optional than @specifiedBy? Was that ever mentioned?

:: A _built-in directive_ is any directive defined within this specification.

GraphQL implementations should provide the `@skip` and `@include` directives.

GraphQL implementations that support the type system definition language must
provide the `@deprecated` directive if representing deprecated portions of the
schema.

GraphQL implementations that support the type system definition language should
provide the `@specifiedBy` directive if representing custom scalar definitions.

GraphQL implementations that support the type system definition language may
provide the `@defer` directive if they support returning response streams.

GraphQL implementations that support the type system definition language may
provide the `@stream` directive if they support incrementally delivered results.

I don't want to bikeshed this too much especially if it was discussed already. Is there any reading material and/or pointers I could catch up on?

A GraphQL implementation that does not support the "type system definition language" can
still support @OneOf and @SpecifiedBy

Since @OneOf and @SpecifiedBy are directives that can only be applied to grammar that is part of the "type system definition language", how can an implementation support this directive without supporting the grammar that is required to place the directive?

It can't but it's fine?

My reading of A GraphQL implementation that does not support the "type system definition language" is a code-first GraphQL implementation like graphql-kotlin for an example. It has nothing to do with supporting the grammar or anything like this. A code-first implementation can implement @specifiedBy.

Supporting the "type system definition language" is a feature of some GraphQL libs that has nothing to do with how the grammar is internally implemented.

I read all of these sentences as: "if a lib reads a SDL written by a user, then the lib must/should/may provide the built in directive definitions":

user SDL + built-in directives == final schema

The same is not true for @defer and @stream because these directives are applied to the execution grammar.

The way I see it the directive location (executable or type system) is irrelevant. These sentences are about the directive definitions that must/should/may be added to the resulting schema.

But again this is my very personal understanding of it and maybe PTSD from the full schemas discussion. Thanks for diving into this with me!

[robrichard]

@martinbonnin I agree that we should probably rewrite this to use RFC 2119 may or optional to clearly state that it's an optional feature instead of recommended.

I still don't think we should include that support the type system definition language for anything related to @defer or @stream because they are features that are unrelated to the SDL.

I think a GraphQL server that creates everything programmatically fits the definition of an implementation that does not support the type system definition. My interpretation is that the spec is not providing any sort of guidance for the @specifiedBy directive for this type of implementation since it is excluded by the phrase GraphQL implementations that support the type system definition language ....

But for @defer and @stream it is important that this type of implementation follow this guidance regardless of SDL support so we should not include the same phrase.

[martinbonnin]

I still don't think we should include that support the type system definition language for anything related to @defer or @stream because they are features that are unrelated to the SDL.

I see it now! Thanks for explaining 🙏

My interpretation is that the spec is not providing any sort of guidance for the @SpecifiedBy directive for this type of implementation since it is excluded by the phrase GraphQL implementations that support the type system definition language ...

Agreed here as well 👍 But I also think this may be a problem?

Do we have somewhere in the spec where we say what built-in directives are required in a valid full/final schema and which ones are not? I initially thought this was the place but since it excludes code-first implementations it can't reasonnably be it?

But for @defer and @stream it is important that this type of implementation follow this guidance regardless of SDL support so we should not include the same phrase.

Right 👍 I actually like the sentence as it is now because it applies to services. I think using service here is correct since SDL support is irrelevant for a service.

Finally, I'd say there should be a similar sentence for other directives? @include/@skip come to mind for an example because they are used in executable locations. Can a service ignore them? But I would probably do it for all directives for symmetry and consistency reasons.

[robrichard]

@include/@skip come to mind for an example because they are used in executable locations.

For @include/@skip, the spec does have the following sentence at the beginning of this section

GraphQL implementations should provide the `@skip` and `@include` directives.

Do we have somewhere in the spec where we say what built-in directives are required in a valid full/final schema and which ones are not? I initially thought this was the place but since it excludes code-first implementations it can't reasonnably be it?

I do think this is the correct place, and with the above sentence for @skip and @include we are covered for all SDL and non-SDL directives. But even @skip and @include are not required since the spec is using RFC 2119 should.

My interpretation is that the spec is not providing any sort of guidance for the @specifiedBy directive for this type of implementation since it is excluded by the phrase GraphQL implementations that support the type system definition language ...

Agreed here as well 👍 But I also think this may be a problem?

Perhaps! Maybe it's worth a WG discussion to clarify?

[benjie]

I did not have time to review Appendix C and my review of section 7 was a bit rushed, but hopefully this feedback is helpful 🤞

[benjie]

First sentence: let's rewrite this to use RFC2119 may/optional.

implemented -> provided: If you provide the @defer directive then it must adhere to these specifications - this is broader text than before (i.e. it doesn't matter or not whether you were attempting to implement the same functionality, if @defer exists then it must be this @defer).

Remove middle sentence: since if you provide a directive, it is made available in introspection (I think), and the only directives in introspection are the ones you provide (I think), this middle sentence is not required (I think).

Final sentence: this is a non-normative note to aid the reader.

Suggested change

	GraphQL services are not required to implement the `@defer` and `@stream`
	directives. If either or both of these directives are implemented, they must be
	implemented according to this specification. GraphQL services that do not
	support these directives must not make them available via introspection. The
	[Directives Are Defined](#sec-Directives-Are-Defined) validation rule will
	prevent GraphQL Operations containing the `@defer` or `@stream` directive from
	being executed by a GraphQL service that does not implement these directives.
	An implementation may provide the `@defer` and/or `@stream` directives. If
	either or both of these directives are provided, they must conform to the
	requirements defined in this specification.
	Note: The [Directives Are Defined](#sec-Directives-Are-Defined) validation rule
	ensures that GraphQL Operations containing the `@defer` or `@stream` directives
	cannot be executed by a GraphQL service that does not support them.

[benjie]

Suggested change

	The `@defer` directive may be provided for fragment spreads and inline fragments
	to inform the executor to delay the execution of the current fragment to
	indicate deprioritization of the current fragment. A query with `@defer`
	directive will cause the request to potentially return multiple responses, where
	deferred data is delivered in subsequent responses. `@include` and `@skip` take
	precedence over `@defer`.
	The `@defer` directive may be provided on a fragment spread or inline fragment
	to indicate that execution of the related selection set should be deferred. When
	a request includes the `@defer` directive, the response may consist of multiple
	payloads: the initial payload containing all non-deferred data, while subsequent
	payloads include deferred data.
	The `@include` and `@skip` directives take precedence over `@defer`.

[benjie]

This falls foul of the CoerceArgumentValues() bug that I've proposed we fix previously. Also, since this variable is nullable you'd expect to be able to set it to null, but doing so would be an error. Let's give it a default here so as to not introduce more problematic examples into the spec.

Suggested change

	query myQuery($shouldDefer: Boolean) {
	query myQuery($shouldDefer: Boolean! = true) {

[benjie]

Suggested change

	- `label: String` - May be used by GraphQL clients to identify the data from
	responses and associate it with the corresponding defer directive. If
	provided, the GraphQL service must add it to the corresponding pending object
	in the response. `label` must be unique label across all `@defer` and
	`@stream` directives in a document. `label` must not be provided as a
	variable.
	- `label: String` - An optional string literal (variables are disallowed) used
	by GraphQL clients to identify data from responses and associate it with the
	corresponding defer directive. If provided, the GraphQL service must include
	this label in the corresponding pending object within the response. The
	`label` argument must be unique across all `@defer` and `@stream` directives
	in the document.

[benjie]

Suggested change

	that are skipped via `@skip` or `@include` directives or temporarily skipped via
	`@defer`. This ordering is correctly produced when using the {CollectFields()}
	algorithm.
	that are skipped via `@skip` or `@include` directives or postponed via `@defer`
	or `@stream`. This ordering is correctly produced when using the
	{CollectFields()} algorithm.

[yaacovCR]

Small tweak: in this context, I think only defer is relevant as a cause for fields being postponed. (Stream may cause subfields to be postponed, but there would always be an empty list for the initial field.)

[benjie]

Presumably this needs a selection set?

Suggested change

	nodes @stream(label: "friendsStream", initialCount: 5, if: $shouldStream)
	nodes @stream(label: "friendsStream", initialCount: 5, if: $shouldStream) {
	name
	}

Also should this example come with a schema?

[benjie]

Interesting, I didn't realise we had applied this to initialCount - I thought it was either non-deferred (i.e. give you the whole list) or deferred with initialCount supplied.

I think we should revisit this discussion, it's quite different to the general "don't defer" and "don't stream" optimizations in my mind - specifically if you specify initialCount: 2 I'd argue that at least 2 results should be supplied, or the entire thing should not be deferred (e.g. if there are fewer than 2 results). Skipping @defer / @include result in the client getting more data up front, but ignoring initialCount allows for the client to get less data up front, and that's a problem to my mind.

[benjie]

I think the wording isn't quite right though. I think if the request uses @defer/@stream then it's okay to have hasNext: false but if the request did not use @defer or @stream then the result must not include hasNext. Does that differ from what we discussed before?

[benjie]

Should we extract this and line 136/137 into a separate PR to merge sooner?

[benjie]

We don't use the word "bubble" in the spec, please use "propagate"

[benjie]

EDITED: My previous comment was incorrect, it's been edited here. Please see the edit history to make sense of the next couple comments.

---

One comment on this is that it quite often talks about multiple "responses", but I don't think that's really accurate. Subscriptions are the closest match, and they have a "response stream" which is a stream of responses; but the incremental payloads don't conform to response. I've put "payloads" in my edits, but I think the terminology we use here should be explored.

---

I noticed in section 7 you've referred to "result" as well. I think a "incremental result stream" consisting of multiple "payloads" of which the first is a "GraphQL response" payload and the remainder are "incremental result" payloads makes sense.

[martinbonnin]

One comment on this is that it quite often talks about multiple "responses", but I don't think that's really accurate.

Great point about terminology. I'm taking this opportunity to promote the Glossary. Whatever we decide we should add there.

Subscriptions are the closest match, and they have a single response which is a "response stream" which has _yields "events"

The spec can allow both interpretations I think?

A GraphQL service generates a response from a request via execution.

But then later on, in the Subscribe algorithm:

Return responseStream.

But then in the response format:

A response to a GraphQL request must be a map.

Can a response stream be a map? 🤔 Oh well...

[benjie]

Can a response stream be a map? 🤔 Oh well...

Now I come to think of it, subscriptions returns a stream of responses - each event on the stream causes an entire GraphQL response ({data, errors} - which is a map) to be produced; whereas defer/stream only produce one "response" payload, but then that's followed by incremental result payloads which are explicitly not conformant to a GraphQL response.

[benjie]

@martinbonnin I've edited quite heavily this comment above - WDYT?

[martinbonnin]

@benjie I agree that response is mostly associated with {data, errors} today and we should keep this as much as possible. Looks like we should extend it to account for {data, errors, pending, hasNext} with this proposal.

Re: payload, it's used pervasively in the Susbscription world where it's used either as generic data to be interpreted according to type (see graphql-ws) or as execution result (==response) (see graphql-sse) so agree we should keep payload as something holding data without mandating that this data has a specific shape.

I like incremental result but that introduces "result" in the mix. What's a non-incremental "result"? a response? If yes, then why not going with incremental response? Because an incremental response is not a response...etc.. naming is hard!