-
Notifications
You must be signed in to change notification settings - Fork 60
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
[Incremental Delivery Questions] How is the response protocol determined in different scenarios #167
Comments
While, I love the fact that an API can drop into "incremental delivery" at any point, I'd like to second this question and problem. I believe on the one hand, the On the other hand, the client never gets to say that it supports incremental delivery and which protocol for incremental delivery (since there are scenarios where other protocols may be supported, e.g. upgrading to WS, let's say) I believe a simple solution may be to introduce a special MIME code like I also like the idea on standardising on results when incremental delivery is required, but the client hasn't indicated support. Personally, re. GET (and other) HTTP methods. I think it's fine to make a recommendation, but I do not think that it must be disallowed or discouraged for multipart responses per se. However, this may depend on how most HTTP CDN caches behave and treat multipart responses, I suppose |
It looks like this hasn't been discussed in a while. As the incremental delivery spec gets closer to being fully integrated into the GraphQL spec (yay), we're taking a look at this. Especially as the recent improvements to graphql-over-http put a greater accent on (I think the error is not necessary in a case like |
Scenario 1 (client sends header Scenario 2 (clients sends Scenario 3 (clients sends header |
I don't think the response type to incremental delivery should be
So, I'd probably default to sending:
Then, independent of their individual rankings, the server could choose |
Are you proposing changing this from mixed to related? That's a pretty big change to something that has had some implementations out there for a year plus. The examples in the multipart/related all show multiple content types. I don't think we're going to find examples of a perfect multipart/* match for what we are doing. I don't think multipart is designed for latency reduction at all — in fact, it's largely designed about email. If we want to find an existing spec that matches what we want it's probably EventStream rather than multipart... I think multipart/mixed is "good enough" and the difference between it and "multipart/related" isn't large enough or more strongly applicable that it's worth making this change. |
🤷♂️ I've not done enough research to weigh an opinion right now, I'm only just starting to dig into the incremental delivery stuff. |
At the moment in Hot Chocolate we decode on the response content-type depending on wether we have a batch request or if the request contains defer and stream. The question is how this is supposed with the accept header. So if a client does not send us an accept multipart/mixed what do we do with a defer request ... fail the request? Tell the client that it must support this transport to handle the feature. I mean this could be a valid solution.
but the above statement should be dropped in this case. Also, reading into |
This would also be in line with the spec text ....
So the accept header can be used to say this is what I understand and no more. Falling back to json is wrong in my opinion since we cannot fallback to json if we talk about defer and stream. |
One other aspect here is that we would need to exit as early a possible, in the case of defer and stream. Meaning if we do not have a streamable response content type in the accept header we should not execute if defer is involved. |
Yeah; I think that's reasonable. It's in line with the spec's intent - you're saying that the response you have (a streaming response) is not compatible with any of the media types the client has specified. The spec says:
You're choosing option 2, which is fine. There are two other options: option 1 (just return it as a stream even though the client didn't explicitly state they support it), or rewrite the response (turn it from a stream to a single payload) so that it is acceptable. The spec currently states you should "respond with the default media type of application/json" but that text should be edited - I've submitted a PR: #227 |
I have a question regarding how the response protocol is determined for a request that could POTENTIALLY result in multiple payloads.
Scenario: client sends header
accept: multipart/mixed
with non-"Incremental Delivery" operationGiven we have an operation that does not use defer/stream at all:
Should the response then use the
application/(graphql+)json
response type or themultipart/mixed
response type?Scenario: clients sends
accept: multipart/mixed, application/(graphql+)json
with non-"Incremental Delivery" operationGiven we have an operation that does not use defer/stream at all:
Should the response then still use the
application/(graphql+)json
content-type or themultipart/mixed
response type?According to the current spec draft, the server should always please the order of the accept header listed content-types if possible. Would that still apply if the server is capable of serving both content-types (
accept: multipart/mixed
andapplication/(graphql+)json
)Scenario: clients sends header
accept: application/(graphql+)json
with "Incremental Delivery" operationQuote from current Spec Draft:
Should the server in this scenario still send back
multipart/mixed
or raise a 406 HTTP error?Should the fact that the client sent an operation with a defer or stream directive be a hint to the server that the client MUST be able to parse it? A client not supporting it, would probably not have requested it in the first place?
Should the server be responsible for merging the results and sending them as a single result to the client?
Should the server do the execution with the defer and stream executor functionality being disabled?
Scenario: clients sends the HTTP
GET
methodIs incremental delivery via
GET
allowed or intended?The text was updated successfully, but these errors were encountered: