Make "processing a response" non-normative and add a note for clients #304
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -511,19 +511,8 @@ execution regardless of validation errors. | |
|
|
||
| ## Status Codes | ||
|
|
||
| The status codes depends on the media type with which the GraphQL response will | ||
| be served: | ||
|
|
||
| ### application/json | ||
|
|
||
|
|
@@ -568,6 +557,11 @@ of `2xx` or `5xx` status codes when responding to invalid requests using the | |
| Note: URLs that enable GraphQL requests may enable other types of requests - see | ||
| the [URL](#url) section. | ||
|
|
||
| Note: When the response media type is `application/json`, clients may use a | ||
| `2xx` status code as an indication that the body contains a well-formed _GraphQL | ||
| response_. See [processing a response](#sec-Processing-a-response) for more | ||
| details. | ||
martinbonnin marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| #### Examples | ||
|
|
||
| The following examples provide guidance on how to deal with specific error cases | ||
|
|
@@ -671,6 +665,12 @@ pass validation, then the server SHOULD reply with `400` status code. | |
| If the client is not permitted to issue the GraphQL request then the server | ||
| SHOULD reply with `403`, `401` or similar appropriate status code. | ||
|
|
||
| Note: When the response media type is `application/graphql-response+json`, | ||
| clients can rely on the response being a well-formed _GraphQL response_ | ||
| regardless of the status code. The intermediary servers that do not understand | ||
| GraphQL may use the status code to get some information about the shape of the | ||
| _GraphQL response_. | ||
martinbonnin marked this conversation as resolved.
Show resolved
Hide resolved
benjie marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| #### Examples | ||
|
|
||
| The following examples provide guidance on how to deal with specific error cases | ||
|
|
@@ -738,10 +738,15 @@ Note: The GraphQL specification | |
| and refers to the situation wherein a _GraphQL field error_ occurs as a partial | ||
| response; it still indicates successful execution. | ||
|
|
||
| ## Processing a response | ||
|
|
||
| The source of a response may be an intermediary server, such as an API gateway, | ||
| proxy, or firewall, in certain environments. | ||
|
|
||
| Those intermediary servers may not understand GraphQL: in the case of an error, | ||
| they may return their own non-GraphQL `application/json` response with a | ||
| non-`2xx` status code. | ||
martinbonnin marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
|
There was a problem hiding this comment. The removal of this normative paragraph is, in my opinion, wrong. The client must not rely on it being a GraphQL response; if they want to treat it as a GraphQL response they must perform their own validations in order to do so (e.g. assert that it conforms to the correct shape/types), and must know that what they're doing is outside of the spec. There was a problem hiding this comment. @benjie , the way I see it, this is a server spec, not a client spec. A server has no control over whether or not a client processes the response correctly, incorrectly, or at all. I think that's why this should be a non-normative paragraph. And the modified language is, I feel, much easier to understand. Now we could write a client spec, defining what features a client must and must not have. For example, we might require the client send the request with a specific content type, or perhaps to support automatic fallback to a known content type if a server responds with an unsupported media type error. We might define the exact mechanism by which the response shape is verified for known response content types, etc. While it seems much of this is implicit based on the server requirements, the existing specification is not currently written as a client spec and does not contain any requirement written from the perspective of the client (apart from the little written in this paragraph). As such, I feel it is improper for it to have a normative note about processing a response in this spec. If this paragraph were to be kept as a normative paragraph, I feel it should at minimum be in a section labeled for client requirements. But even then, the existing paragraph says "MUST NOT rely on the body", it doesn't explain what TO do in that situation. Should it look for a certain shape? Should it ignore all data in the response? In other words, it's says "client MUST NOT count on it being well-formed, but it MIGHT be", which is not particularly helpful for a normative note. What do you think? There was a problem hiding this comment. Apologies for the delay here. +1 to @Shane32 comment. As a client developer relatively new to this spec but pretty familiar with GraphQL, it took me way longer than I'd like to admit to understand the nuances there. Having the language from the point of view of the client is easier to process in my opinion. There was a problem hiding this comment.
That does not seem accurate. This spec specifies:
It's a protocol spec that defines the protocol for communicating between a spec-compliant client and server. I believe @enisdenjo implemented both a client and a server based on this spec in
Indeed; but the client does; hence why we state:
It is the client's responsibility to handle this concern. The server may not even be the one supplying this response (it may have come from a gateway, proxy, etc), so of course the server cannot influence this - it is entirely the client's responsibility.
It has an entire chapter dedicated to how to form requests to the server. It also has loads of explicit conformance requirement statements for the client:
There are many more. There was a problem hiding this comment.
We don't currently label client and server requirements separately since both are intermingled; such as:
To do this in one place but not the rest of the spec would be a little strange. And doing it across the spec would make the spec a lot less readable, in my opinion.
Indeed; the client must not rely on the body. What it does with the body is up to it, but it cannot be trusted. Throwing an error is the most reliable way of handling it; but we're not going to require that (though we could RECOMMEND it, I suppose). It could also choose to run its own validations, but at that point it's up to the client to figure out what those should be - we've already told it that the response cannot be trusted, so if it feels it should push ahead anyway, that's its responsibility to figure out. There was a problem hiding this comment.
As a counter-example, HTTP details a
This effectively protects the client against the connection being terminated (as would be expected by Protocol generally have to somewhat trust the underlying protocol (which is why we have not detailed how HTTP works, and why HTTP doesn't detail how TCP works); however they also need to take into account how, even in a trusted network, they may fail - especially when the failure modes can cause ambiguity. Our failure mode here is that there's an intermediary that bails out the request early (permissions, failed cache, netsplit, timeout, etc etc etc) and returns a response using a common media type I see what you mean about it effectively being covered by other parts of the spec, but I'm not convinced that it doesn't bear repeating - what problem are we trying to solve by removing this or demoting it to a non-normative note? There was a problem hiding this comment.
I love this paragraph. I would love to see that kind of context added to the spec (permissions, failed cache, netsplit, timeout, etc...trust that if this happens it uses non-2xx, etc..)
When I read that as a client developer, "I MUST NOT trust something", I'm not sure what this concretely mean. If I get a
2 things (and one extra):
// This is very close to what a client would write
// (and also very close to how this PR is worded)
if (status == 2xx || content-type == "graphql-response+json") {
parseResponse()
}
// I find this harder to read
if (status != 2xx && content-type == "application/json") {
// What to do here? I don't know, it depends.
} else {
// This is actually the important branch
}
There was a problem hiding this comment.
I'll grant that the spec does contain a lot of client specifications, and written as such. However, I feel the requirement in question simply ** is ** non-normative by nature, and probably not within scope of this document. Let's compare to CORS issues. Is CORS part of the GraphQL-over-http protocol? No. Is it part of the bigger picture that a client/server should consider when designing a server? Yes. How do we deal with it? In #303 we add a non-normative note that mentions that if CORS issues are not considered, there may be security issues with the implementation. Why isn't it normative? CORS may not be relevant. CORS may be solved in another fashion. We don't require HTTPS either - for example, many microservices run on HTTP internally and the HTTPS layer is applied at the edge. Perhaps we could add a non-normative note saying that data over HTTP is insecure, although the readers probably already know that and have a plan to keep their data secure. By comparison, here we have an issue where an intermediary server might respond with failure. Could this be relevant? Yes. Is it always relevant? No. Perhaps it is known that no intermediary servers are in use. Perhaps the intermediary servers are configured to return 500 rather than 400 errors. Perhaps they return xml instead of json responses. Ultimately, I don't think this is part of the GraphQL-over-http protocol. A note is warranted as a reminder to implementers. Now if it's deemed important enough to add a normative note, then it should be worded as an optional requirement with specific behavior, such as:
This tells the client exactly what to do, but as an optional requirement. (just an example; it's worded poorly)
Completely agree.
Agree
Agree
Yes, I think this PR does a better job communicating with the reader the logic/rationale of the altered content type and how to use it. There was a problem hiding this comment.
I think "do not"'s are okay in specs - especially when you're telling someone to not do something that they typically would do unless told otherwise. But yes; we currently leave them to fend for themselves in this regard because there is no "correct" way to handle this, though there is an "incorrect" way, which is to blindly trust it.
I'm happy changing to wording along these lines - it aligns with what I said before: "throwing an error is the most reliable way of handling it; but we're not going to require that (though we could RECOMMEND it, I suppose)" - SHOULD and RECOMMEND have the same normative meaning. I still think that we should pair these together though: you MUST NOT blindly trust it, and we RECOMMEND that you (i.e. "you SHOULD" - same difference) raise an exception as if it were a network error.
I agree in general, but this is a conformance requirement specific to There was a problem hiding this comment. |
||
| For this reason, if the response uses a non-`2xx` status code and the media type | ||
| of the response body is `application/json` then the client MUST NOT rely on the | ||
| body to be a well-formed _GraphQL response_. | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, there's a tension between the Body first paragraph:
and that paragraph, which is why I removed it.
My current understanding is that this paragraph was meant for intermediary servers that may not understand GraphQL. So there are 2 kind of servers:
I'd say this spec should be about GraphQL servers and not Intermediary servers? Or if we want to specify the behaviour of intermediary server, we could do it in a separate section?
Or were there use cases when a GraphQL server is compliant and does not return a well formed GraphQL response?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first glance, it appears the paragraph was intended to address invalid requests or those that the server does not understand (which may be more than only GraphQL-over-HTTP requests). For example, if the request was not able to be parsed as JSON or had an unrecognized content type. This paragraph would enforce that a non-200 status code was used. Whereas if we read just above the 6.1 paragraph you referenced, paragraph 6 starts with:
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My gripe is specifically about that part:
In case of errors that completely prevent the generation of a well-formed _GraphQL response_. That contradicts 6.1:The body of the server’s response MUST follow the requirements for a GraphQL response. I understand 6.1 to be about the body. Then the status codes are refined in section 6.4 below.If the request was not able to be parsed as JSON or had an unrecognized content type, I think there are other places in the spec that address this already:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this paragraph contradicts anything, but I'm okay removing it as redundant / described elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The contradiction IMO is in the spec saying:
the body must always be a well formed GraphQL response. And then going on to say "In case of errors that prevent the generation of a well-formed GraphQL response", which I understand asif it isn't a well formed GraphQL response, then ...How can it not be a well formed GraphQL response if it must always be a well formed GraphQL response?
The only reason I see for the body to not be a well formed GraphQL response is if content negociation fails. But as soon as
application/jsonorgraphql-response+jsonis selected, my reading is that the body must always be a well formed GraphQL response?If content-negociation errors are what's at stake here (i.e. "the errors that prevent the generation of a well formed GraphQL response"), then this case is already handled by 6.1 so I'd remove the sentence. If there are other error cases, I'd love to explicit them cause it's not obvious what they could be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was poorly worded; perhaps:
Essentially it's saying when you encode a GraphQL response that it must adhere to the spec. The other paragraph (the one you've deleted) is saying if you cannot form a valid GraphQL response then you must not use a GraphQL response type.
This is also incorrect; more correct would be:
The
application/graphql-response+jsonis only for use with well-formed GraphQL responses, so if you can't form a well-formed GraphQL response you can't use it. I think I was probably thinking something like:try { return doTheGraphQLStuff() } catch (e) { return { errors: [{message: e.message}] }}where you could then use the graphql-response+json for these errors; but this is a well-formed GraphQL response so it contradicts the beginning of the sentence.This doesn't really refer to "intermediary" servers as much as it does the hosting environment. For example if you use
nginxandluato do your GraphQL, but theluaprocess crashes, then this requires that yournginxisn't configured to still use theapplication/graphql-response+jsonresponse type.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what my brain has a really hard time processing 😅 . Thinking in terms of logic:
proposition P:
content-type == application/graphql-response+jsonproposition Q:
isWellFormGraphQLResponse(body) == trueI think this is the same as
if P then Q?I understand that as
if !Q then !P?So the second sentence is the contrapositive of the first one and therefore strictly equivalent from a logical point of view?
If it brings the attention to a specific point I'm fine with it as a non-normative note but from in the spec text itself, I'd expect no redundancy. Or is there some nuance there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so.
Pas you have stated it is that you use a specific media type, but what's being stated is that you are encoding a GraphQL response; it could be in any supported media type.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright 👍 But I think my initial point that it's not clear whether the "non-valid GraphQL response" is part of the spec or not still holds.
I put the paragraph back in to keep that PR small and about non-normative client notes. Will open a follow up one for "non-valid GraphQL responses"