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

Response body for non-well-formed GraphQL-over-HTTP requests #293

Closed
atrigent opened this issue Jun 19, 2024 · 6 comments
Closed

Response body for non-well-formed GraphQL-over-HTTP requests #293

atrigent opened this issue Jun 19, 2024 · 6 comments

Comments

@atrigent
Copy link

atrigent commented Jun 19, 2024

Unless I missed it, this specification doesn't specify the contents of the response body when a non-well-formed GraphQL-over-HTTP request is received, only specifying the response status codes to use. I can't even find anything in the GitHub issues about it. It seems to take the opinion that it is not possible to create a well-formed response in this case. In practice implementers will all decide on different ways to respond, which seems counter to the intent of creating this spec in the first place. Since the new application/graphql-response+json media type is not fully defined anywhere, the response body is seemingly not even required to be valid json, despite the name seemingly implying that, making it very unclear how these responses should be handled. In fact I think it can be argued that this spec is even MORE unclear than not having any spec at all. In the "legacy" world, where application/json was used as the response media type, I could see implementers making the decision themselves to ensure that all responses are at least valid JSON. However, now that this spec exists, I could imagine some implementers reading it and interpreting it to mean that these responses should not have any body at all, which is really unnecessarily awkward to handle on the client side and provides no clarity on what error occurred.

Can anyone explain why this spec doesn't define this behavior? Was it considered but rejected? Was it never considered at all? If so, why? I'm really having a hard time understanding why you wouldn't use the chance you had to define everything you possibly could to provide much needed clarity to implementers of both clients and servers.

@atrigent atrigent changed the title Response body for non well-formed GraphQL-over-HTTP requests Response body for non-well-formed GraphQL-over-HTTP requests Jun 19, 2024
@benjie
Copy link
Member

benjie commented Jun 19, 2024

this specification doesn't specify the contents of the response body when a non-well-formed GraphQL-over-HTTP request is received

Indeed, that is by design. If the request is not a well formed GraphQL over HTTP request then the server may respond to it however it wants. This is leveraged by #264 for example: since a persisted operation request is not a well-formed GraphQL-over-HTTP request, we may apply our own (additional) logic to the request. It can also be leveraged to return e.g. a GraphiQL interface when a plain HTTP GET request is made.

Roughly speaking, if the request doesn't "smell like" a GraphQL request, then this spec doesn't cover it (even if it's a request made to your /graphql endpoint), leaving you free to reply however you deem appropriate.

It seems to take the opinion that it is not possible to create a well-formed response in this case.

No, it simply places no requirements on such as response since it is outside of the scope of the spec.

In practice implementers will all decide on different ways to respond, which seems counter to the intent of creating this spec in the first place.

It is not; this spec only covers well-formed GraphQL-over-HTTP requests. Note that the GraphQL document itself does not have to be valid, but the query property must be present and it must be of type string (see spec for further requirements).

Since the new application/graphql-response+json media type is not fully defined anywhere, the response body is seemingly not even required to be valid json, despite the name seemingly implying that, making it very unclear how these responses should be handled.

This media type is defined under Media Types to be the media type for a GraphQL response, but it could indeed do with further definition.

Note that you can reply to a request that is not a well-formed GraphQL-over-HTTP request with application/graphql-response+json; this must still be a JSON representation of the GraphQL response as specified by the linked GraphQL response text, but this allows you to accept GraphQL queries in other non-standard formats and still respond in a standard way - for example, #264 leverages this to effectively "upgrade" a persisted operation request to well-forked GraphQL-over-HTTP request.

@benjie
Copy link
Member

benjie commented Jun 19, 2024

Oh, I missed one thing:

the response body is seemingly not even required to be valid json, despite the name seemingly implying that

This is covered by RFC6839: https://datatracker.ietf.org/doc/html/rfc6839#section-3.1

[RFC4627] defines the "application/json" media type. The suffix
"+json" MAY be used with any media type whose representation follows
that established for "application/json".

@jasonkuhrt
Copy link

This media type is defined under Media Types to be the media type for a GraphQL response, but it could indeed do with further definition.

When I navigate to https://graphql.github.io/graphql-over-http/draft/#sec-Media-Types I see:

For details of the shapes of these JSON payloads, please see Request and Response.

I would have expected (I think?) this to take me to the link you shared @benjie, https://spec.graphql.org/draft/#sec-Response. However, currently it goes to https://graphql.github.io/graphql-over-http/draft/#sec-Response.

Because while I understand your point @benjie when reading the document it is actually never possible AFAICT to follow hyperlinks to a schema for the response body json. That creates doubt and confusion etc. Make sense?

@benjie
Copy link
Member

benjie commented Oct 25, 2024

I would have expected (I think?) this to take me to the link you shared @benjie, spec.graphql.org/draft#sec-Response. However, currently it goes to graphql.github.io/graphql-over-http/draft#sec-Response.

These links point to the correct location.

Because while I understand your point @benjie when reading the document it is actually never possible AFAICT to follow hyperlinks to a schema for the response body json. That creates doubt and confusion etc. Make sense?

The request parameters are described in the Request section here: https://graphql.github.io/graphql-over-http/draft/#sec-Request-Parameters

And the JSON encoding of them is specified here: https://graphql.github.io/graphql-over-http/draft/#sec-JSON-Encoding

The response section here: https://graphql.github.io/graphql-over-http/draft/#sec-Body

Details:

The body of the server’s response MUST follow the requirements for a GraphQL response, encoded directly in the chosen media type.

And links... to the wrong place. Odd. We're using _GraphQL response_ in the source:

a well‐formed _GraphQL response_. The server's response describes the result of

which should link to the spec proper due to:

"https://spec.graphql.org/draft/": {
"graphql-service": "#sec-Overview",
"graphql-schema": "#sec-Schema",
"graphql-request": "#request",
"graphql-response": "#sec-Response",

Hmmm...

@benjie
Copy link
Member

benjie commented Oct 25, 2024

Turns out I was looking at the wrong paragraph, and it was an explicit link to the wrong place. Fixed in #316

@jasonkuhrt
Copy link

Thanks @benjie!

@benjie benjie closed this as completed Oct 25, 2024
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