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

[Incremental Delivery Questions] How is the response protocol determined in different scenarios #167

Open
n1ru4l opened this issue Nov 19, 2021 · 10 comments

Comments

@n1ru4l
Copy link
Contributor

n1ru4l commented Nov 19, 2021

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" operation

Given we have an operation that does not use defer/stream at all:

query { foo }

Should the response then use the application/(graphql+)json response type or the multipart/mixed response type?

Scenario: clients sends accept: multipart/mixed, application/(graphql+)json with non-"Incremental Delivery" operation

Given we have an operation that does not use defer/stream at all:

query { foo }

Should the response then still use the application/(graphql+)json content-type or the multipart/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 and application/(graphql+)json)

Scenario: clients sends header accept: application/(graphql+)json with "Incremental Delivery" operation

query {
  foo
  ... on Query @defer {
    bar
  }
}

Quote from current Spec Draft:

According to the HTTP 1.1 Accept specification, when a client does not include at least one supported content type in the Accept HTTP header, the server MAY choose to respond in one of several ways. The server MUST either:

Disregard the Accept header and respond with the default content type of application/graphql+json, specifying this in the Content-Type header; OR
Respond with a 406 Not Acceptable status code

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 method

Is incremental delivery via GET allowed or intended?

@kitten
Copy link

kitten commented Nov 19, 2021

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 multipart/mixed response protocol is optimal and not tied to the presence (or absence) of directives like @defer. This explicitly is good because for scenarios where it's desireable for a user to create "out-of-spec" incremental delivery modes, the API decides when to send it.

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 multipart/graphql+json, for instance, and to require clients to send Accept: application/graphql+json, multipart/graphql+json if possible (i.e. lists). This means that a client that only sends Accept: multipart/graphql+json would only accept a multipart response, while a client with Accept: application/graphql+json says that it does not support incremental delivery via a multipart response.

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

@glasser
Copy link
Contributor

glasser commented Aug 22, 2022

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 accept headers, I'd like to suggest that sending multipart/mixed incremental delivery responses require an accept header. ie, clients should send accept: multipart/mixed, application/graphql-response+json, application/json or something, and servers should not send multipart/mixed responses to clients that don't say that they can accept it. This will allow servers to provide better errors to folks sending @defer/@stream from old clients that don't support it, instead of the client getting some sort of SyntaxError: No number after minus sign in JSON error from trying to parse a boundary as JSON.

(I think the error is not necessary in a case like { ... @include(if: false) { ... @defer { x } } though – ie, it's OK if the error check happens as part of execution rather than as a new validation stage.)

@enisdenjo
Copy link
Member

The Accept request HTTP header indicates which content types, expressed as MIME types, the client is able to understand.

MDN about Accept header


Scenario 1 (client sends header accept: multipart/mixed with non-"Incremental Delivery" operation)
A: Response should use multipart/mixed content type.

Scenario 2 (clients sends accept: multipart/mixed, application/(graphql+)json with non-"Incremental Delivery" operation)
A: Since there is no "q" parameter for weighing, the server is free too choose as both offered content-types are of equivalent importance. I'd choose multipart/mixed simply because it appeared first.

Scenario 3 (clients sends header accept: application/(graphql+)json with "Incremental Delivery" operation)
A: RFC 9110 considers accept headers as "preferences" which is why both options in the spec are valid; however, IMHO responding with 406: Not Acceptable is a better move since the client might not expect/support content types not listed in the accept header.

@benjie
Copy link
Member

benjie commented Aug 30, 2022

I don't think the response type to incremental delivery should be multipart/mixed since it's not really mixed content; I think it's instead multipart/related:

used for compound documents (that is, messages in which the separate body parts are intended to work together to provide the full meaning of the message)
-- https://docs.microsoft.com/en-us/previous-versions/office/developer/exchange-server-2010/aa493937(v=exchg.140)#:~:text=used%20for%20compound%20documents%20(that%20is%2C%20messages%20in%20which%20the%20separate%20body%20parts%20are%20intended%20to%20work%20together%20to%20provide%20the%20full%20meaning%20of%20the%20message)

So, I'd probably default to sending:

Accept: application/graphql-response+json,
        multipart/related;type=application/graphql-response+json,
        application/json,
        multipart/related;type=application/json

Then, independent of their individual rankings, the server could choose application/graphql-response+json for regular requests and multipart/related;type=application/graphql-response+json for incremental delivery.

@glasser
Copy link
Contributor

glasser commented Aug 30, 2022

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.

@benjie
Copy link
Member

benjie commented Aug 30, 2022

🤷‍♂️ I've not done enough research to weigh an opinion right now, I'm only just starting to dig into the incremental delivery stuff.

@michaelstaib
Copy link
Member

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.

Disregard the Accept header and respond with the default media type of application/json, specifying this in the Content-Type header; OR

but the above statement should be dropped in this case.

Also, reading into text/event-stream, it could actually be a better alternative for defer.

@michaelstaib
Copy link
Member

This would also be in line with the spec text ....

The "Accept" header field can be used by user agents to specify
response media types that are acceptable. Accept header fields can
be used to indicate that the request is specifically limited to a
small set of desired types, as in the case of a request for an
in-line image.

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.

@michaelstaib
Copy link
Member

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.

@benjie
Copy link
Member

benjie commented Sep 6, 2022

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:

In alignment with the HTTP 1.1 Accept specification, when a client does not include at least one supported media type in the Accept HTTP header, the server MUST either:

  1. Disregard the Accept header and respond with the default media type of application/json, specifying this in the Content-Type header; OR
  2. Respond with a 406 Not Acceptable status code and stop processing the request.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants