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

Clarification for Accept: */* #242

Open
tobias-tengler opened this issue Feb 18, 2023 · 8 comments
Open

Clarification for Accept: */* #242

tobias-tengler opened this issue Feb 18, 2023 · 8 comments

Comments

@tobias-tengler
Copy link

The spec is pretty clear on what to do if the Accept header is not specified or has a value of either application/graphql-response+json or application/json. It's not so clear to me though, which response Content-Type the server should choose in the case of Accept: */* (especially during the legacy watershed period). It would be great if the spec could clarify this part.

@tobias-tengler
Copy link
Author

According to the reference implementation */* is treated as application/json for now: https://github.com/graphql/graphql-http/blob/main/src/handler.ts#L689

@benjie
Copy link
Member

benjie commented Feb 18, 2023

It’s the servers choice; but I think defaulting too application/json prior to watershed and application/graphql-response+json after is reasonable. In fact you could go hybrid and use graphql-response on error even now so you can start using the HTTP codes!

@Shane32
Copy link
Contributor

Shane32 commented Feb 18, 2023

The HTTP protocol has a number of semantics for the Accept header, such as q-factor weighting, which are not covered by the GraphQL over HTTP protocol. A compliant server should fully support the HTTP specification. I would not think those specifications would need to be reiterated. It should be rather obvious that Accept: */* should act in the same manner as if Accept were not supplied, as the GraphQL over HTTP protocol defines the default content type during and after the legacy watershed period, and this matches Accept: */*. Perhaps the wording could be changed to say 'default content type' rather than 'when the Accept header is not specified', just to be more clear.

@tobias-tengler
Copy link
Author

Yes, that would be good @Shane32.

@benjie
Copy link
Member

benjie commented Feb 19, 2023

I actually disagree here. A client that sets no accept header expects the default. A client that says Accept: */* is explicitly telling the server: use whatever content type is most appropriate, I can handle it.

@tobias-tengler
Copy link
Author

tobias-tengler commented Feb 19, 2023

Yes, that's correct. In reality most web-based GraphQL clients will actually send Accept: */* though, if the Accept header wasn't specified explicitly by the developer. See the fetch spec (13.)
I don't have numbers to back this up, but I would assume that nearly 100% of GraphQL requests in browsers are done through the fetch API and of those at least 90% aren't specifying an explicit Accept header.

I think we're in a weird spot, since legacy clients might unknowingly tell the server they accept anything, while they probably don't in reality.

@Shane32
Copy link
Contributor

Shane32 commented Feb 19, 2023

The HTTP RFC 2616 states:

If no Accept header field is present, then it is assumed that the client accepts all media types.

In other words, a missing Accept header is equivalent to Accept: */*

@benjie
Copy link
Member

benjie commented Feb 19, 2023

Though that is indeed what the spec says (which aligns with what I suggest for users who explicitly set Accept: */*), I think the majority of GraphQL clients that don’t set Accept and won’t easily be updated to add an Accept header will be one-off shell scripts and other such ad-hoc uses which still expect application/json since that’s the current behaviour of most servers, so maintaining that until watershed makes sense to me.

But either way, it’s the server’s choice, I don’t think we need to dictate this.

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

No branches or pull requests

3 participants