Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spec edits for @defer/@stream #742

Open
wants to merge 62 commits into
base: main
Choose a base branch
from

Conversation

robrichard
Copy link
Contributor

@robrichard robrichard commented Jul 2, 2020

Corresponding graphql-js pull request: graphql/graphql-js#2319

Please keep discussion limited to specifics on how the spec text is written. There is a dedicated discussion forum for questions, feedback, and comments on defer & stream: https://github.com/robrichard/defer-stream-wg/discussions

spec/Section 6 -- Execution.md Outdated Show resolved Hide resolved
spec/Section 6 -- Execution.md Outdated Show resolved Hide resolved
@@ -411,6 +411,7 @@ FieldsInSetCanMerge(set):
{set} including visiting fragments and inline fragments.
* Given each pair of members {fieldA} and {fieldB} in {fieldsForName}:
* {SameResponseShape(fieldA, fieldB)} must be true.
* {SameStreamDirective(fieldA, fieldB)} must be true.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SameStreamAndDeferDirective?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our proposal limits @defer to fragments and fragment spreads, so only @stream provides a new opportunity for overlapping fields to conflict.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@robrichard did you mind elaborating why this is the case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maraisr I added an explanation to the RFC document here: #774

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have concerns about this validation rule; namely I think it breaks "composability" of a GraphQL query. If you have a fragment somewhere that contains { friends { name } } and another fragment that contains { friends @stream { name } } then these two fragments are composible under current GraphQL rules but would not be composable once this rule is introduced.

I feel like these fragments could remain composable if we don't add this rule. Since "stream" is optional, in the above case GraphQL could determine that it has to evaluate the entire list up front anyway so it could just return the same set for both.

Similarly if you had { friends @stream(initialCount: 1) { name } } and { friends @stream(initialCount: 5) { name } } these could both be merged into a single { friends @stream(initialCount: 5) { name } } since (as @AndrewIngram points out in the WG chat) the initialCount: 5 would satisfy both.

(This consideration makes the no-stream effectively analogous to initialCount: ∞.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That said, I like the simplicity it introduces. So I'm definitely on the fence for this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We did originally do something like that, but received guidance from facebook recommending a validation rule

See discussion here: graphql/graphql-js#2319 (comment)

@yaacovCR
Copy link
Contributor

Following up on: graphql/graphql-js#2319 (comment)

I am still not quite sure on use case for label with stream if different labels are not allowed. probably just missing something, but if it could be elaborated upon with an example of a conflict, it would be most appreciated.

Thanks!

@robrichard
Copy link
Contributor Author

@yaacovCR There's a possibility for conflict when you combine defer and stream. I posted an example here: https://gist.github.com/robrichard/0a5e9fb7b0e615545198183c2b9c95a8

@yaacovCR
Copy link
Contributor

Well, defer needs a label in that example, but does stream?

@yaacovCR
Copy link
Contributor

Thanks so much for your quick reply!!!

@yaacovCR
Copy link
Contributor

My unsolicited two cents from the feedback so far based on the different ways fragments are processed and the implementing code is that the directive should be named patch instead of defer, as in theory, this is not a regular fragment of fields at all, but an indication to split off a new branch of execution from the existing operation, and would be better off with a new keyword in fact but for the need to preserve backwards compatibility.

@robrichard
Copy link
Contributor Author

@yaacovCR in this proposal, label is an optional argument to both @defer and @stream, so you will not be required to pass it unless your client library requires it. While it's possible that there aren't any conflicts that require a label on @stream, I don't think the proposal gains any clarity by forbidding it.

For @defer I think it is important that deferred fragments are regular fragments. It is only at the location the fragment is spread that we indicate that it should be deferred. I think it's an important use case that a single fragment can be deferred in one part of the query, and used normally in another part.

@yaacovCR
Copy link
Contributor

Wow, you have clarified everything! Thanks so much for your time.

I do still disagree as to the name of the defer directive. It seems the first conceptual point is that a new branch of execution is being created via a patch with a response that is possibly deferred. It is up to the server in fact to indicate whether it is indeed delayed at all. Perhaps defer means deferred to another payload, rather than another timepoint, but that is somewhat unclear.

It seems like a patch is a more appropriate, more general term, as defer more aptly describes how the payload is to be delivered, which might be better off in an argument to the proposed patch directive, with more granular options even than defer.

Other than that, again, this conversation has been tremendously clarifying for me, and very helpful to me at least in organizing some initial thoughts in terms of integrating schema stitching with these changes.

I very much appreciate your time and all the work that has gone into this spec change by @lilianammmatos, yourself and all others involved, apologies to all I am missing.

@yaacovCR
Copy link
Contributor

yaacovCR commented Jul 17, 2020

Although I suppose patch is not quite right for defer either as a patch might be a stream payload...

Maybe directive @branch(deliver="defer")

@IvanGoncharov IvanGoncharov added the 💡 Proposal (RFC 1) RFC Stage 1 (See CONTRIBUTING.md) label Aug 19, 2020
@IvanGoncharov

This comment has been minimized.

@robrichard

This comment has been minimized.

spec/Section 6 -- Execution.md Outdated Show resolved Hide resolved
spec/Section 6 -- Execution.md Outdated Show resolved Hide resolved
ResolveStreamRecord(streamRecord, variableValues, subsequentPayloads):
* Let {label}, {iterator}, {resolvedItems}, {index}, {path}, {fields},
{innerType} be the correspondent fields on the Stream Record structure.
* Remove the first entry from {resolvedItem}, let the entry be {item}. If
{resolvedItem} is empty, retrieve more items from {iterator}:
* Append {index} to {path}.
* Increment {index}.
* Let {payload} be the result of calling CompleteValue(innerType, fields, item, variableValues, subsequentPayloads, path)}.
* Add an entry to {payload} named `label` with the value {label}.
* Add an entry to {payload} named `path` with the value {path}.
* If {resolveItem} is not empty or {iterator} does not reach the end:
* Append {streamRecord} to {subsequentPayloads}.
* Return {payload}.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you might need to revisit this definition; I might be reading it wrong but it feels like you may have used resolvedItem in the place of resolvedItems in one place, and the "retrieve more items from {iterator}" isn't super clear, it refers to "If {resolvedItem} is not empty" under an item which already seems to state "If {resolvedItem} is empty", and there doesn't seem to be a point at which {resolvedItem} would change between the two. Also it uses resolveItem rather than resolvedItem which I think is a typo since it doesn't seem to be referred to elsewhere.

@benjie
Copy link
Member

benjie commented Oct 27, 2020

Sorry if this has been discussed elsewhere; but what if you don't know if the current payload is going to be the final payload? Is sending a payload with just {hasNext: false} (and the path/label/etc) with no items from the iterator acceptable? I couldn't quite figure this out from a quick scan of the algorithm.

Let me back this question up with a concrete example: price comparison websites.

Imagine we visit a price comparison website, PCW, and search for home insurance. PCW's back-end goes off to it's panel of 500 home insurance providers and asks them each for a quote with your details. Some of the providers offer a quote, others decline, and they take varying amounts of time to do so up to a maximum cut-off time of 2 minutes. Lets say 90 providers give a quote.

In GraphQL without @stream this might look like a 2 minute request that results in an array of these 90 quotes. Not a very good experience, since the user sees nothing for 2 minutes.

In an ideal @stream-enabled collection, you'd get the result from each provider as it comes through, allowing you to populate the page and show the prices coming in, giving the user feedback during the 2 minute wait. Once either all providers have responded or the 2 minute timeout elapses, the stream can be ended with no additional data.

In the currently proposed @stream, if I'm reading it right, I think it's saying you'd need to keep back one result such that once the 500 providers have finished responding you can return it with hasNext: false also in the same payload. This means that the result displayed to the user would always have to be one item behind unless you know exactly how many items you're expecting.

@robrichard
Copy link
Contributor Author

@benjie thanks for taking a look. It is our intention for the spec to support a final payload of {hasNext: false} for exactly this type of use case. I will update the spec to make sure this is reflected. We have this covered in test cases in graphql-js https://github.com/graphql/graphql-js/pull/2319/files#diff-a370dc23e95e90e2b04fe67a3a933e6eb09296c64b4eae7509cd3ae93edaf2aeR334

@benjie
Copy link
Member

benjie commented Oct 27, 2020

Awesome 🙌

robrichard and others added 27 commits Jan 15, 2023
* add comma

* remove unused parameter
* streamline stream execution

Currently, these spec changes introduce a new internal function named `ResolveFieldGenerator` that is suggested parallels `ResolveFieldValue`. This function is used  during field execution such that if the stream directive is specified, it is called instead of `ResolveFieldValue`.

The reference implementation, however, does not require any such function, simply utilizing the result of `ResolveFieldValue`.

With incremental delivery, collections completed by `CompleteValue` should be explicitly iterated using a well-defined iterator, such that the iterator can be passed to `ExecuteStreamField`. But this does not require a new internal function to be specified/exposed.

Moreover, introducing this function causes a mixing of concerns between the `ExecuteField` and `CompleteValue` algorithms; Currently, if stream is specified for a field, `ExecuteField` extracts the iterator and passes it to `CompleteValue`, while if stream is not specified, the `ExecuteField` passes the collection, i.e. the iterable, not the iterator. In the stream case, this shunts some of the logic checking the validity of resolution results into field execution. In fact, it exposes a specification "bug" => in the stream case, no checking is actually done that `ResolveFieldGenerator` returns an iterator!

This change removes `ResolveFieldGenerator` and with it some complexity, and brings it in line with the reference implementation.

The reference implementation contains some simplification of the algorithm for the synchronous iterator case (we don't have to preserve the iterator on the StreamRecord, because there will be no early close required and we don't have to set isCompletedIterator, beacuse we don't have to create a dummy payload for termination of the asynchronous stream), We could consider also removing these bits as well, as they are an implementation detail in terms of how our dispatcher is managing its iterators, but that should be left for another change.

* run prettier
* fix whitespace

* complete renaming of initialItems
* Add error handling for stream iterators

* also add iterator error handling within CompleteValue

* incorporate feedback
Co-authored-by: Simon Gellis <82392336+simongellis-attentive@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📄 Draft (RFC 2) RFC Stage 2 (See CONTRIBUTING.md)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet