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

Server can choose the media type on Accept fail #227

Merged
merged 3 commits into from
Oct 26, 2023

Conversation

benjie
Copy link
Member

@benjie benjie commented Sep 6, 2022

If the client issues Accept: application/graphql-response+json, application/json and the server does not support this for the request (e.g. the request uses @stream/@defer so needs a multipart response) then the server should be able to choose to respond with another suitable media type, for example multipart/mixed;type=application/graphql-response+json. Thanks to @michaelstaib for pointing out this oversight.

(Note: v1 doesn't explicitly specify @stream/@defer or subscription related concerns, but we should still be future proof where possible.)

Comment on lines 429 to 430
1. Disregard the `Accept` header and respond with the server's choice of media
type, indicating this in the `Content-Type` header; OR
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would a suggestion make sense here? (if you're not sure what to choose, use application/json)

Copy link
Member

Choose a reason for hiding this comment

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

The issue is that if you have a query that has deferred parts in it you have to respond with MultiPart/Mixed or an event stream.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think there’s enough context in the rest of the spec to infer the suggestion.

@maraisr
Copy link
Contributor

maraisr commented Sep 7, 2022

I thought we want the exact opposite? ~> graphql/defer-stream-wg#48

If the client cannot support a multipart response, and the accept header reflects this — then dont send it. For example using an older version of graphiql that doesn't support defer stream, but the server its targeting does.

@michaelstaib
Copy link
Member

michaelstaib commented Sep 7, 2022

@maraisr that is exactly how I feel :D I would like it if we made it strict, so that if you send in the accept header that is what the server abides by. This would give predictability. If the server cannot meet the requested accept header it would be great if it would fail.

But I think this is separate from this PR, this PR just refines already existing spec text and we can have a discussion on this in the next working group meeting or create another PR that discusses this.

@benjie
Copy link
Member Author

benjie commented Sep 7, 2022

Yeah, this is just meant to be a fix; the major change in behaviour needs more discussion 👍

@benjie
Copy link
Member Author

benjie commented Oct 3, 2023

The "Accept" header is specified such that:

If the header field is present in a request and none of the available representations for the response have a media type that is listed as acceptable, the origin server can either honor the header field by sending a 406 (Not Acceptable) response or disregard the header field by treating the response as if it is not subject to content negotiation.

In the text currently we extend this same wording effectively to our server implementors; since GraphQL-over-HTTP's purpose is to map more natively to HTTP semantics.

Having pondered this for a while now (!) I definitely see the value in honouring the Accept header and doing a 406 Not Acceptable; but this also severely limits the choices of servers - forcing them to reject the request otherwise they are not spec compliant. The client can (and should!) still check the Content-Type header on the response (since legacy or spec non-compliant servers may send the wrong Content-Type, as may intermediary servers such as proxies), so I don't see that allowing the server to accept the response and return another format of GraphQL is a dealbreaker.

I have a counter-proposal; we can say that the server SHOULD return a 406, but MAY ignore the header. This puts weight on the 406 approach, but does not forbid the "accept it anyway" approach.

What are your thoughts on this @michaelstaib and @maraisr?

@Shane32
Copy link
Contributor

Shane32 commented Oct 3, 2023

My thoughts:

  • There are multiple methods to stream responses, such as the graphql-ws protocol or multipart/mixed
  • I personally would expect that by default every GraphQL server responds in a basic (json) format, and does not prefer one streaming format over another -- although currently the spec states this:

If a client does not supply the Accept header then the server may respond with an error, or with any content type it chooses

  • Existing text would force a compliant server that receives a @defer or @stream request to return an application/json format. Seems that it should be allowed to return any format, such as would be the case if there were no Accept header.
  • HOWEVER, the written text only applies when the client specifically requests an unsupported format. As such, there should be no expectation that the client can interpret the response at all. (Otherwise it would have specified the format in the Accept header.)
  • In the sample at the top of this PR, it is quite unlikely that the client is going to be able to parse a multipart response. If it were able to do so, it would/should have been specified in the request. Responding with 406 would seem appropriate.

In conclusion, I agree with @benjie 's latest suggestion, that the spec should state that the server SHOULD return 406 (when the client requests an unsupported format), but MAY return any other format - both to mimic the HTTP spec and match the behavior when the Accept header is not specified.

@benjie
Copy link
Member Author

benjie commented Oct 3, 2023

I've updated the text to reflect this.

@benjie benjie force-pushed the server-chooses-on-accept-mismatch branch from d2167f3 to a2509bb Compare October 3, 2023 13:56
@benjie benjie merged commit ad817d8 into main Oct 26, 2023
5 checks passed
@benjie benjie deleted the server-chooses-on-accept-mismatch branch October 26, 2023 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants