Rework specification with a focus on backwards compatibility #175
Conversation
|
Thanks for the careful read @ghmcadams! I've applied a number of your suggestions and closed those discussions; others are left open to gather more feedback 👍 |
|
Pulling this out of a resolved conversation:
The term "GraphQL server" should never appear in either document. The GraphQL spec talks in terms of "GraphQL service" in general, and this spec talks directly in terms of "server" (defined as a "GraphQL over HTTP Specification compliant HTTP server"). Thanks for spotting my typo, I have changed "GraphQL server" to "server" in this place - I don't seem to have made this mistake elsewhere. I see what you mean though - does the concept of "server" encompass the GraphQL part or not? My intent is that it "wraps" the GraphQL part and almost sees GraphQL as a "black box" (except as noted by specific algorithm references), but finding a way of stating this does seem desirable. Let me know if you have specific thoughts how to address this. |
I agree. To address this, this specification could include these additional statements: Introduction
This specification details how libraries that wish to publish and consume GraphQL services over HTTP should do so in order to maximize interoperability between client libraries, tools, and server implementations. The goal of this specification is not to override or replace the GraphQL specification. It seeks only to augment it for the purpose of serving GraphQL services over HTTP. If ever there is conflict between the two, the GraphQL specification should be used. Overview:: In this document, the term {GraphQL service} refers to an application service as used by the GraphQL specification.
The role of a server is to provide clients access to one or more GraphQL services over HTTP. A server is not a GraphQL service. It is a GraphQL service host.
The role of a client is to issue requests to a server over HTTP in order to interact with a GraphQL service. |
|
Excellent suggestions @ghmcadams; I've incorporated wording similar to that into the document 👍 |
|
The topic of request/response versus client/server is one I would like to discuss in a working group. Let's organise one in the next month or two. |
|
Great work @benjie. This spec looks awesome. |
| A request for execution should contain the following request parameters: | ||
| A {GraphQL-over-HTTP request} is formed of the following parameters: | ||
|
|
||
| - {query} - A Document containing GraphQL Operations and Fragments to execute. |
There was a problem hiding this comment.
This also should be optional to allow for persisted queries, but I can do a separate PR on this outlining the use case. I originally added the {extensions} for this so the persisted query id can now go into that map.
There was a problem hiding this comment.
I don't think that a server that only allows persisted operations is a truly GraphQL-over-HTTP compliant server. The literal intent of an operation allowlist is to prevent interoperability, and that's okay!
There was a problem hiding this comment.
The Apollo implementation of automatic persisted queries works by leaving out query in the particular requests that are attempting to execute a persisted operation via hash (specified in extensions), but not in those requests that aren't trying to execute a persisted operation via hash (including those which are effectively "registering" a persisted query by sending both the query and the hash — this is what makes our implementation "automatic"). So I wouldn't describe this as "a server that only allows persisted operations". Note that Apollo APQs are not an allowlist: they are simply a way of optionally abbreviating the request. (One goal specifically is to make requests short enough to fit comfortably in the URL for a GET request.)
If we want to try to make the Apollo APQ protocol not a violation of this spec, we could make query optional and have some sort of note stating that when query is left out, the server must have some sort of extensions-based mechanism for deriving the Document.
Does the spec actually explain anywhere what to do if the query is not provided? I suppose for GETs this falls into the "Server URLs which enable GraphQL requests MAY also be used for other purposes" thing to some degree.
There was a problem hiding this comment.
The Apollo implementation of automatic persisted queries works by leaving out query in the particular requests [...] If we want to try to make the Apollo APQ protocol not a violation of this spec, we could make query optional and have some sort of note stating that when query is left out, the server must have some sort of extensions-based mechanism for deriving the Document.
I think it's okay for Automatic Persisted Queries to not be an official GraphQL request - the point of the specification is to define a common set of rules that means a compliant client from any vendor can speak to a compliant server from any vendor. APQ is not part of that flow, it's a specific behavior that the client/server can negotiate as an extension, separate from this specification.
Does the spec actually explain anywhere what to do if the query is not provided? I suppose for GETs this falls into the "Server URLs which enable GraphQL requests MAY also be used for other purposes" thing to some degree.
"A {GraphQL-over-HTTP request} is formed of the following parameters" implies that missing "query" means that it's not a "GraphQL over HTTP request"; then we have "When a server receives a {GraphQL-over-HTTP request}, it must return a well‐formed response." but it's not a GraphQL over HTTP request, so it's up to the server how to handle it, either by classifying it as "other purposes" (in this case serving GraphQL but in a way not 100% compatible with the GraphQL-over-HTTP spec), or it can throw an error.
Maybe we could add a non-normative note to make this clearer? Or indicate that there may be more ways to talk GraphQL with a webserver than strictly defined in this spec?
There was a problem hiding this comment.
I think it's okay for Automatic Persisted Queries to not be an official GraphQL request - the point of the specification is to define a common set of rules that means a compliant client from any vendor can speak to a compliant server from any vendor. APQ is not part of that flow, it's a specific behavior that the client/server can negotiate as an extension, separate from this specification.
Yep, that's fair. I just wanted to clarify that with our "automatic" PQ implementation, you don't have "a server that only allows persisted operations".
Re the second point, no strong feelings here (I think the spec is fine as given).
| you should note the schema and "initial value" are not included in the | ||
| GraphQL-over-HTTP request, they are handled by the server based on the URL used. | ||
|
|
||
| Note: Be aware that `query` is a misleading parameter name as its value is a |
There was a problem hiding this comment.
Should we not introduce a new term in this case document and support query as deprecated. Also, this discussion should go in a separate PR. But something we should think about.
There was a problem hiding this comment.
I was seriously tempted to make it so that application/graphql+json uses the term "document" (or maybe "source") whereas application/json uses "query", but decided that this would probably be ill advised. So yeah, definitely a follow-up discussion for that 👍
|
This is exciting! Going to try to find time this week to look at it from an Apollo Server perspective. |
|
This already looks great @benjie ... will give it a third read tomorrow :) Will also have a look at the impact for Hot Chocolate. |
|
A decent number of additional edits thanks to @spawnia and @michaelstaib, the most critical of which are:
|
|
I've spent the last few minutes restructuring this spec to be in terms of client and server, but there are many shared responsibilities in terms of request and response so I think this would be a larger piece of work. It may be worthwhile, but I suggest that we attempt to land these current changes first before restructuring further. I did at least find a bug whilst doing it, so I've patched that 👍 |
|
@benjie I suggest merging this. Any further requests can be made by submitting a new PR. |
|
@spawnia @michaelstaib How do you feel about merging this? Are there others still active from the old HTTP WG? |
|
@benjie As for your question of active members: Activity has been low for quite a while. I think it's safe to merge. Especially given that the spec is still in stage 0 (preliminary), which states that things can change rapidly. Any objections can be addressed with a new PR. |
|
Unless we get a few more approvals, let's give it two weeks and then merge on or after 28th June in line with the "preliminary" policy 👍 |
As discussed in graphql/graphql-over-http#175 (comment) - @benjie Every variant of the MIT license does it this way, see https://fedoraproject.org/wiki/Licensing:MIT for reference.
|
I have not yet had time to do a full review of this change yet from an Apollo Server perspective. (This month is very busy for me personally and work-wise and June 28th will be a challenge.) I'd like to specifically focus on the MIME type issue, though. My position is that I am in favor of the proposal with respect to response content-types, but I don't find the changes for request content-types to be compelling, for several reasons. First, I just fundamentally am skeptical of the concept of a single MIME type that means "either a JSON object with Second, my understanding is that the issue we're trying to solve with the new MIME types is to allow clients to tell if a response actually came from a GraphQL server or from some sort of intermediate processor. That makes sense as a goal to me, and I see how a specific response content-type helps here. But I don't see why having a specific request content-type is at all relevant to the issue we're trying to solve. For responses, the benefits seem to outweigh the costs to me, but the benefits seem way lower for requests. Third, for responses there's a great opt-in mechanism via Fourth, requiring support for this new content-type in requests makes implementing a spec-compliant server more complex. Specifically, many GraphQL servers are implemented as middleware that can be layered into a bigger web framework stack. Web frameworks typically have a standard mechanism for parsing incoming requests as JSON which properly solves issues like setting a configurable maximum size to parse, using a standard error handling mechanism, etc. It is not unreasonable to want to build your GraphQL server as a middleware that assumes the incoming body has already been parsed. In Apollo Server we've historically hardcoded the use of the npm (As an interesting note, graphql-helix explicitly claims to be compliant with this spec in its README, but turns out that's only true if you happen to notice buried in the CHANGELOG for version 1.11.0 a note suggesting that you configure your server to parse the content-type. No example in its repository does this correctly. I say this not to bash graphql-helix, but just to note that this great project written by folks sophisticated enough to know about this spec and even to know about the content-type incorrectly believe their project to be spec-compliant, due to exactly this issue.) To me, the fourth issue is a pretty big cost to the idea of adding a new request content-type to GraphQL-over-HTTP, and given the lack of a clear problem solved by this request content-type, makes me very unexcited about this aspect of the proposal. I would advise a simpler approach where we define |
|
Solid argument; I accept. Whilst making the changes, I also spotted some other issues, for example we said that Changes are, basically:
|
|
Assuming no more changes (other than typos), merge date pushed back to 1st July. |
Co-authored-by: Benedikt Franke <[email protected]>
There was a problem hiding this comment.
If we're interested in making the spec help implementers avoid CSRF vulnerabilities, here are a few thoughts. For those not familiar with CSRF, we recently wrote docs on CORS and CSRF for the Apollo Server docs that I think are helpful.
| type (as indicated by the `Content-Type` header). | ||
|
|
||
| If the client does not supply a `Content-Type` header with a POST request, the | ||
| server SHOULD reject the request using the appropriate `4xx` status code. |
There was a problem hiding this comment.
I'd consider making this a MUST for CSRF-protection reasons. A server that allows a POST with no custom headers (and which uses cookies for authentication) is vulnerable to trivial CSRF mutations. That is, I can go to any website on the internet and it can send a mutation to the GraphQL server which will contain my cookies, no matter what the server's CORS policy is. (The website may not be able to read the result of the mutation, but the damage will be done.) As soon as mutations require a custom content-type like application/json, this concern goes away because such requests get OPTIONS preflights.
| server SHOULD reject the request using the appropriate `4xx` status code. | ||
|
|
||
| A server MAY support POST requests encoded with and/or accepting other media | ||
| types. |
There was a problem hiding this comment.
A non-normative note suggesting that servers which support POST requests with content-type application/x-www-form-urlencoded, multipart/form-data, or text/plain need to take extra care to avoid CSRF may be useful.
| conform with the long-established semantics of safe methods within HTTP. | ||
| executed, the server MUST respond with error status code `405` (Method Not | ||
| Allowed) and halt execution. This restriction is necessary to conform with the | ||
| long-established semantics of safe methods within HTTP. |
There was a problem hiding this comment.
Would it be interesting to include a further non-normative note here saying something like:
Additionally, restricting mutations to POSTs which MUST contain a Content-Type header ensures that mutations cannot be sent as part of cross-site request forgery (CSRF) attacks, as long as the Content-Type header is not allowed to be one of the three values which can be in a CORS-safelisted Content-Type request header (application/x-www-form-urlencoded, multipart/form-data, or text/plain).
(that MUST assumes my other suggestion is taken, which perhaps it won't be)
|
Good notes @glasser; I think we should add these in a follow-up security focussed PR 👍 |
There was a problem hiding this comment.
I think Apollo Server is relatively close to obeying the MUSTs in this document. The main way it doesn't is by allowing query-less operations for APQ support.
(Additionally, Apollo Server also supports a "batched" protocol not described here where the body can be an array of requests rather than a single request. This protocol has its drawbacks — eg, ideally responses could be streamed via multi-part instead of returned as one big JSON array. The main reason people like this protocol is that it allows request setup (such as context value calculation) to occur once for all operations in the batch (sharing resources like DataLoaders across operations) rather than separately. However, supporting this additional syntax doesn't necessarily put AS out of spec, I think.)
| A request for execution should contain the following request parameters: | ||
| A {GraphQL-over-HTTP request} is formed of the following parameters: | ||
|
|
||
| - {query} - A Document containing GraphQL Operations and Fragments to execute. |
There was a problem hiding this comment.
The Apollo implementation of automatic persisted queries works by leaving out query in the particular requests that are attempting to execute a persisted operation via hash (specified in extensions), but not in those requests that aren't trying to execute a persisted operation via hash (including those which are effectively "registering" a persisted query by sending both the query and the hash — this is what makes our implementation "automatic"). So I wouldn't describe this as "a server that only allows persisted operations". Note that Apollo APQs are not an allowlist: they are simply a way of optionally abbreviating the request. (One goal specifically is to make requests short enough to fit comfortably in the URL for a GET request.)
If we want to try to make the Apollo APQ protocol not a violation of this spec, we could make query optional and have some sort of note stating that when query is left out, the server must have some sort of extensions-based mechanism for deriving the Document.
Does the spec actually explain anywhere what to do if the query is not provided? I suppose for GETs this falls into the "Server URLs which enable GraphQL requests MAY also be used for other purposes" thing to some degree.
| which accept the `application/graphql+json` media type (as indicated by the | ||
| `Accept` header). | ||
|
|
||
| Before 1st January 2025 (`2025-01-01T00:00:00Z`), if the client does not supply |
There was a problem hiding this comment.
Is the implication here that spec-compatible servers should literally contain a conditional checking the current time (until we're in 2025 and that conditional can be removed)? Or I guess because this is merely a SHOULD it is OK for a server author to just decide to transition their code from one default to the other at a point that feels reasonable?
There was a problem hiding this comment.
- Should it contain a conditional check on the time? Yes, ideally.
- Can it just decide to transition the code from one default to another at a point that feels reasonable? Yes, knowing that it could cause issues for legacy clients.
Note: client's SHOULD include Accept and SHOULD accept application/graphql+json, so the only clients affected by this will be clients who a) do not issue Accept headers, and b) actually bother to read the Content-Type of the response and explicitly do not support application/graphql+json. I wonder what the size of that Venn diagram overlap is - perhaps you have data on that at Apollo?
I wonder if we should make it so that clients MUST include an Accept header from the watershed?
There was a problem hiding this comment.
I wonder if we should make it so that clients MUST include an Accept header from the watershed?
This seems overbearing. I run GraphQL over curl all the time (a context where I don't really care about the content-type or whether it's OK to have a 400). I wouldn't want my server to reject all curl -XPOST -H 'content-type: application/json' -d '{"query":"..."}'. (Although really recent curl does have --json which sets accept... but the point remains.)
There was a problem hiding this comment.
Yeah; curl et al is the main reason that I left this as a SHOULD in the first place; but it's also why I figure if you don't supply Accept you probably don't care about what the server says the Content-Type is 😆
There was a problem hiding this comment.
Should it contain a conditional check on the time? Yes, ideally.
I see this differently. I don't see the purpose of a watershed as to set a precise date at which functionality should be turned on. The way I see it, a watershed is to set a date at which a server without such functionality is no longer compliant. During the time between now and the date of the watershed, new releases of a server should gradually start to support the new functionality described in the spec. Ideally (and totally at the discretion of the implementor), a server would be backward compatible.
There was a problem hiding this comment.
I broadly agree, but David has pointed out the point where there is a sudden change in the recommended behaviour - the interpretation of a request without an Accept header should be application/json before and application/graphql+json after. That said, since it's only a SHOULD it doesn't matter too much and you don't need a time check.
|
|
||
| In case the client can not access the schema at all, the server SHOULD respond | ||
| with the appropriate `4xx` status code. | ||
| If the response uses a non-`200` status code and the media type of the response |
There was a problem hiding this comment.
For what it's worth, the Apollo implementation of persisted queries does violate this MUST NOT: persisted query-related errors are sent (currently) as application/json with 400s and are parsed by the client.
| If | ||
| [CoerceVariableValues()](<https://spec.graphql.org/draft/#CoerceVariableValues()>) | ||
| raises a {GraphQL request error}, the server SHOULD NOT execute the request and | ||
| SHOULD return a status code of `400` (Bad Request). |
There was a problem hiding this comment.
One note: while this is a good idea which I support (and which Apollo Server tries to implement), it's challenging to implement this today in a non-hacky way with graphql-js. I thought there was already an issue for this but I couldn't find one so I filed graphql/graphql-js#3679
There was a problem hiding this comment.
Yeah; I believe the trick right now is that if it doesn't have a data property (note that's different from it being present but null) then you can assume it's a CoerceVariableValues() or similar request error as opposed to a field error. I think you can actually just straight up use graphql(...) and use this heuristic to adhere to the GraphQL-over-HTTP spec, you don't need to call out to the separate methods since all the request errors end up in HTTP 400 anyway. But yeah, it'd be beneficial to have it separate for sure 👍
There was a problem hiding this comment.
Ah true, I believe we do map lack of data to 400 but we also want a more
precise error code inline for the different error types and that's what we
can't do without better support from graphql-js.
TL;DR: Rather than reading this PR as a diff, please just read the finished content. Sorry ❤️
First, I'm sorry. I had to back out of involvement in the GraphQL-over-HTTP working group a couple years ago due to severe lack of time and I'm only just getting my head back above water now. I'm keen for us to start making progress again, but it seems we have quite a number of sticking points, not least the contention over
application/graphql+json(which, if I recall correctly, was my suggestion in the first place).Originally this specification was intended to codify existing practices such that the majority of existing servers and clients were already compatible. It turned out that HTTP status codes were a big sticking point, and this lead us to introduce the
application/graphql+jsonmedia type. We marked this media type as required from the get-go.In this revised proposal, I instead introduce a "watershed" of 1st Jan 2025 whereupon support from
application/graphql+jsonchanges from "RECOMMENDED" to "REQUIRED".I have also tightened up the language, referenced the relevant RFCs and spec locations, and in many places concretely changed the behavior that was previously specified. I've also restructured the specification significantly.
This is not a simple editorial pull request, nor one made up of incremental changes that can be easily separated into separate smaller PRs; this is pretty much a re-write of the specification, and for that I am very sorry. I have spent a decent number of hours going through everything with a fine-toothed comb, but this is still just a first draft and I expect there to be changes in follow-up PRs even after (if) this is merged.