Spec edits for incremental delivery, Response Section and Examples Appendix

[robrichard]

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

The response section is especially important, as it defines all of the terms which will be used in the execution algorithm

Helpful reference material:

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

[HenriVesala]

is this stuck in limbo? Or is there something that I could help on?

[benjie]

I don't know if it's just me, but I feel like the examples could use more narrative and explanation that this is just one way the query could turn out. Here's how I'd change the beginning of the first example:

[benjie]

Maybe change "completed result" to "incremental completion result" to tie it in with the "incremental" naming? (Also to be more specific.)

Suggested change

	deferred data and the first streamed list item. There is one _completed result_,
	deferred data and the first streamed list item. There is one _incremental completion result_,

[robrichard]

Updated now to incremental completion notice

[benjie]

I feel like the weirdness of this needs more calling out - name belongs to both id:0 and id:1, so we're kind of using id:0 as a shortcut (but id:1 would also work). We should make it clear to a client that it can't form HomeWorldFragment from solely the root data plus the id:0 stuff and NameAndHomeWorldFragment from solely the root data and the id:1 stuff - they must merge the trees before extracting the data.

[robrichard]

Added a paragraph here: https://github.com/graphql/graphql-spec/pull/1203/changes#diff-0237c26b29c484df8d43b13dc3a3658867cfd015d8ccea8ed93454c466dbacb3R200-R203

[benjie]

Splitting this across two paragraphs is a little weird, I feel like the list of keys up front should be exhaustive.

Suggested change

	The _initial incremental stream result_ must contain an entry with key {"data"},
	and may contain entries with keys {"errors"} and {"extensions"}. The value of
	these entries are defined in the same way as an _execution result_ as described
	in the "Data", "Errors", and "Extensions" sections below.
	The _initial incremental stream result_ must contain an entry with the key
	{"hasNext"}. The value of this entry must be {true} if there are any
	_incremental stream update results_ in the _incremental stream_. The value of
	this entry must be {false} if the initial incremental stream result is the last
	response of the incremental stream.
	The _initial incremental stream result_ must contain an entries with keys {"data"} and {"hasNext"}, and may contain entries with keys
	{"errors"}, {"pending"}, {"incremental"}, {"completed"}, and {"extensions"}.
	The value of {"data"}, {"errors"} and {"extensions"} are defined in the
	same way as an _execution result_ as described in the "Data", "Errors", and
	"Extensions" sections below.
	The value of the {"hasNext"} entry must be {false}
	if the initial incremental stream result is the last response of the incremental
	stream. Otherwise, {"hasNext"} must be {true}.

I'd like to see a non-normative note below this explaining that {"hasNext"} is required only for it to be an initial incremental stream result, and that omitting it is valid if it's the only result, just that it means it's an execution result instead (i.e. there's no "incremental" at all). In this case, the GraphQL response would not return a stream.

This is a terrible version of that note since I'm writing this already over-time on our call...

Note: Omitting {"hasNext"} would make the response a regular execution result, which is valid only when returned directly and not as part of a stream.

[robrichard]

This suggested edit changes pending from required to optional on the initial incremental stream result, I don't think we have a case for an incremental stream without a pending? I will keep it as required when I incorporate this change.

Do you still think we need a note about omitting hasNext? it's a bit more complicated as you also need to omit pending?

I will however add a note explaining the reasoning for allowing hasNext: false in the initial incremental stream result

[benjie]

I'd add a non-normative note detailing why this is allowed.

[benjie]

Same for "completed"

[robrichard]

Note added here: https://github.com/graphql/graphql-spec/pull/1203/changes#diff-98d0cd153b72b63c417ad4238e8cc0d3385691ccbde7f7674bc0d2a718b896ecR116-R126

[benjie]

If "hasNext" is true, pending must be present.
If "hasNext" is false, pending must not be present. < Not true, could be hasNext:false along with pending:[...],incremental:[...],completed:[...]

[robrichard]

Going to leave this as is since pending must be present in all cases for the initial incremental stream result

[benjie]

Should we add that {"hasNext": true} alone is invalid?

I propose that we do not do so. Not doing so allows {"hasNext: true} to be used as a keepalive.

[robrichard]

Agreed, do you think we should specifically mention this, outside of every other entry being defined as optional?

[benjie]

We could probably upstream this change.

[yaacovCR]

This is amazing work, and others have given already and I’m sure we’ll continue to give wonderful feedback.

My two cents is that it may be beneficial to refer to the pending/completed and possibly even the incremental entries as notifications rather than results. The result is the deferred/streamed data at that path that has an id. It’s an abstract concept in terms of the response format, to be assembled, perhaps as needed, by the client.

The response can strictly speaking never send a pending result, in fact that’s in some sense an internal contradiction, or, at best, a shorthand for what we really mean, a notification or description of a result that is pending/completed. The incremental entries are actually results/updates, although calling them as such may confuse them with the overarching defer/stream result that they help complete.

I offer this comment just as a suggestion, I could live with this somewhat confusing shorthand if others feel that it is the best compromise in terms of clarity.

[robrichard]

@yaacovCR @benjie The following terms have now been updated:

Pending Result => Incremental Pending Notice
Completed Result => Incremental Completion Notice