Move application/json responses to Appendix A#379
Conversation
martinbonnin
changed the title
application/json responses to Appendix A
|
For most of the last 10 years, there's been an unofficial GraphQL-over-HTTP spec which very very many custom servers implemented. There are major servers such as Apollo Server, Yoga, etc that many people depend upon and have been updated to conform with the latest GraphQL-over-HTTP spec, but serving GraphQL over HTTP is typically so simple that a GraphQL server can actually be implemented in very few lines of code, and very very many organizations have gone this route rather than adding a dependency:
This might look something like (scratched out of my head directly into GitHub editor, probably don't use this in production I guess?): import { graphql } from "graphql";
import express from "express";
import schema from "./schema";
export const app = express();
app.use(express.json());
app.post("/graphql", async (req, res) => {
if (typeof req.body !== "object" || req.body == null)
return res.status(400).json({ error: "Invalid GraphQL request" });
const { query, variables, operationName } = req.body;
if (typeof query !== "string")
return res.status(400).json({ error: "Missing query document" });
if (variables != null && (typeof variables !== "object" || Array.isArray(variables)))
return res.status(400).json({ error: "Invalid variables" });
if (operationName != null && typeof operationName !== "string")
return res.status(400).json({ error: "Invalid operation name" });
res.json(
await graphql({ schema, source: query, variableValues: variables, operationName }),
);
});This is all that's needed to serve GraphQL over HTTP, and has been fine for the vast majority of users. It is the opposite of complex: it's incredibly simple. The GraphQL-over-HTTP spec as it currently stands allows this kind of very simple server. HTTP status codes are here to serve a very specific audience: namely intermediary services on the server side of the network that do not understand GraphQL or do not want to parse the response body, but still want to know what's going on. This can be reverse proxies, caches, logging, anomaly detection, intrusion detection, etc. HTTP status codes do not exist to benefit GraphQL clients. Clients don't need them, they simply need to know:
They should not use HTTP status codes for any other purpose - they can determine everything else from the payload. GraphQL's new HTTP status codes are only there for the backend folks: backend developers, operations, caching, SRE, etc. If a GraphQL adopter does not need these, then it feels wrong to thrust upon them the requirement that they use a pre-built GraphQL-over-HTTP package that complies with all the status codes in the spec when they could solve it with the few lines of code above. This pattern has been demonstrated since the very introduction of GraphQL in 2015 (ref: https://github.com/graphql/express-graphql/blob/ef4cff8ebf6cda2280c086e5f88de4d04e9d90f2/src/index.js) and it's just as valid today as it has always been. Your proposal is to effectively force GraphQL adopters to use a more complex implementation, one that honors the various status codes, even if they won't benefit from it, because if they don't then over time the GraphQL clients they depend upon may no longer work with their custom servers since support is optional. In my opinion, this move forces additional complexity on GraphQL adopters. GraphQL adopters should be encouraged to use status codes, because it will likely benefit them in the long term. GraphQL server libraries and frameworks should be encouraged to support these status codes out of the box. GraphQL client libraries and frameworks should be encouraged to send requests that are compatible with status codes even if their servers don't yet support them because it enables the server team to enable this additional functionality (at the cost of a more complex server). But we should not mark all existing servers as GraphQL-over-HTTP non-compliant as a way of "forcing" them to adjust to what we currently feel is the best practice. If they don't need it, they should not have to adopt that complexity. |
|
IMO the golden path is a better way to "enforce" this. All "golden path" servers MUST support the new media type and status code. But the GraphQL-over-HTTP spec, like the GraphQL Spec itself, should remain flexible and widely compatible. |
|
@benjie can you share examples of projects that would be negatively affected by this move? Both Apollo server and Yoga have been supporting |
benjie
left a comment
benjie
left a comment
There was a problem hiding this comment.
For the reasons outlined in #379 (comment) I do not think we should make this change.
Any project that doesn't use an off-the-shelf GraphQL server and instead wrote their own GraphQL middleware will be impacted by this in the long run. I cannot share specific examples (I treat client engagements and private discussions with confidence) but I can tell you that I've seen quite a number of GraphQL adopters who write middleware like the above into their codebase rather than adding a dependency - who needs an extra dependency when it's so simple, that's just going to get you more dependabot alerts!
Indeed, I noted this above:
Essentially, this move requires the use of a GraphQL-over-HTTP capable library because it increases the complexity of supporting GraphQL-over-HTTP from a few lines of code to a significantly larger undertaking that must break out parse/validate and be more precise with status codes. If you don't need that, the complexity is unnecessary, and I think mandating it would be a net negative for GraphQL. This would be akin to requiring a GraphQL client rather than just allowing |
|
Thanks for the discussion. In general, I agree with what @benjie wrote but I would love to hear more and see these examples of the simple servers you are talking about - who are they? where do you see them? Also, I would love to also add to the discussion the benefits of @martinbonnin's change - what it helps with and for whom? Seems like we need to choose between people building custom implementations and people who use tools? |
|
My 2 cents is agreeing with this statement:
and
and
But still, I'd love to weigh the pros and cons. Why's having |
|
Thanks all for the comments. I will need a bit of time to process everything wanted to give a few comments while we're looking at this: First of, we live in a GraphQL bubble. We read specs all day long (or at least from time to time). But it's not the case for everyone, this issue is the most upvoted issue in this repo and is some indication that simplification would be welcome. Reading all of this, I believe there are 2 different considerations at play:
I'm mostly concerned about 2. 10 years from now, I don't want to receive my GraphQL response with Clarity and good defaults are what make this spec valuable. I think I'm down to rephrase this: as: Some parting thoughts:
I believe the past 10 years are proof that GraphQL observability is not that simple. Sure you can make a simple implementation but that might come back bit you down the road and then you come up on Twitter complaining about "200 OK". Whether this is a spec thing or golden path thing is an open question. It comes down to how much opinionated we want to be at the end of the day. |
I can't find great public examples, because it's very hard to search for and these are the kinds of thing that people generally just put together in their own apps which aren't typically open source. However, it's possible to find tutorials including copy-pasteable example handlers that don't use a server framework - I don't know how to enumerate the people who actually followed these tutorials though!
Our very own "serving over HTTP" page has walked people through the general concepts of doing this themselves since September 2015, so anyone who followed that in a language other than JS (where express-graphql/Apollo Server were recommended at the time) may well have rolled their own. I wouldn't know how to go about enumerating that either!
The majority are almost certainly the latter indeed; but I don't think that means we should exclude the former... I don't think we need to choose between them at all. The current version of the spec states:
and:
Together these require clients to support at least the new version and they should1 support the legacy version - as mentioned above, it doesn't really make much odds to them, they just need to do the "is 200 or application/graphql-response+json" check when receiving a response and include both media types in their Servers have the much harder time so they may choose which version they support. For maximal compatibility (i.e. if you're building a server library that others will use) they should support both, but they don't have to. This allows server implementers, the people who benefit (or not) from HTTP status codes, to make the decision as to whether they want status codes (in which case they'd probably use an off the shelf library that offers this by default) or whether they want simplicity/minimal bundle size (in which case they might just add a few lines of code themselves, or have their LLM do it). It allows us to serve both people rolling their own and people who use tools.
This still requires servers to be more complex than is required today.
If and when that happens, you as the server developer can switch out your 10 line JS function for a GraphQL server that implements the HTTP status codes, it should be very simple to achieve when you need it - add the dependency, switch out your middleware, nothing else needs to change.
And people can respond: "just use a decent GraphQL HTTP library, they handle this!" because that's what the spec will encourage of all HTTP libraries now. Hence the
Agree, and we have precedent for this. The spec should focus on compatibility and interoperability. The documentation should focus on best practices. The golden path should focus on guiding people to the pit of success (which would, indeed, require usage of the new media type). Footnotes
|
This was partially addressed by the removal of the watershed following the discussions; the spec is already simpler than it was when that issue was filed. |
* Removing Appendix A: My brain is fried and I won't have the time to process graphql/graphql-over-http#379 in time * Add aim for other GraphQL over HTTP items * Add Can I use.
* Removing Appendix A: My brain is fried and I won't have the time to process graphql/graphql-over-http#379 in time * Add aim for other GraphQL over HTTP items * Add Can I use.
|
@benjie been looking at this a bit over the weekend. What about changing your export const app = express();
app.use(express.json());
app.post("/graphql", async (req, res) => {
// change response content-type to application/graphql-response+json
res.set("Content-Type", `application/graphql-response+json; charset=utf-8`);
// Wrap errors in an array
if (typeof req.body !== "object" || req.body == null)
return res.status(400).json({ errors: [{ message: "Invalid GraphQL request" }] });
const { query, variables, operationName } = req.body;
if (typeof query !== "string")
return res.status(400).json({ errors: [{ message: "Missing query document" }] });
if (variables != null && (typeof variables !== "object" || Array.isArray(variables)))
return res.status(400).json({ errors: [{ message: "Invalid variables" }] });
if (operationName != null && typeof operationName !== "string")
return res.status(400).json({ errors: [{ message: "Invalid operation name" }] });
res.json(
await graphql({ schema, source: query, variableValues: variables, operationName }),
);
});That's one more line for setting the response content-type and a few extra chars for wrapping I believe it is also compliant with the current GraphQL over HTTP state? Sure by returning 400 always, it does not follow the recommendations but in terms of MUST, I think it clears the bar. All of that while paving a nice future where What am I missing? |
|
Isn't the snip by @benjie intended to demonstrate what is typically found in many GraphQL servers today, rather than what could be written today? For instance, the GraphQL.NET main library includes an extremely-simple server implementation within its sample, as it has always done, which just returns Even if this were changed, it is impossible to know how many users have simply copied the server implementation from the sample without utilizing the recommended implementation within the GraphQL.Server.All NuGet package (and updated it to v7+). Since the GraphQL package has been downloaded 74M times, and GraphQL.Server.Transports.AspNetCore only 19M times, stands to reason that many of the other 55M downloads have their own server implementation, probably using Further, here's a relatively new GraphQL server implementation that returns So, correct me if I'm wrong, but I think the point is that if |
|
@Shane32 It's all a tradeoff and I'd rather focus at the future than the past in this occasion. A clear, consistent media type with observable status codes will help the ecosystem a lot. I thought Benjie's point was that
But I can't really see the extra complexity. Are we talking about the extra line to set the response content-type and extra chars to wrap the errors in an array ? I'm down with that if it means we have a single media type for GraphQL responses. Edit: @Shane32 fun times, I just bumped into this PR of yours that is basically the same as this one minus the moving to appendix and was also approved by @enisdenjo |
|
@martinbonnin No, your example above is not compliant. But @Shane32 is right, my argument against this is not complexity1. My key concern is backwards compatibility and the innumerable APIs out there that use GraphQL servers derived from the recommended practices of the past. This WG was set up to standardize this existing pattern first and foremost, and to guide to a better future as a secondary objective. If we're changing the mission of the WG to "break the past to simplify the future" then that needs to go back to the primary working group and gain consensus on the new direction. Footnotes
|
Can you ellaborate? The current spec says:
My reading of this sentence is that the meaning of "appropriate" is left to the reader. So 400 is "appropriate" if I decide so for my implementation. Sure it's not great like using 405, etc... but it's what I decided made sense for my server. That would make the above implementation compliant. If that's not the intent of the current spec, I suggest we rephrase it.
For the record, current implementations can, and will continue to function. Clients are encouraged to support both media types for the time being, I'm certainly planning to do so for Apollo Kotlin. The upgrade story is quite good and the end state is clean. I'm down to go back to the drawing board to get clarity on this. I scheduled a meeting today if anyone wants to talk. If not we can discuss in the primary working group in a couple of week.
To answer to @Urigo's question, I believe the "implementer vs end user" distinction does not apply here. The HTTP headers and status code are very observable behaviour by everyone doing GraphQL. Every end userhas opened the network tools at least once to see the details of the HTTP request. A strong and clear path forward would help everyone there. |
…the server wording
Rephrased in #389, but the first issue is not related to your use of HTTP 400, it's that your example will return HTTP 200 for a GraphQL document that does not parse.
If we want to invalidate many existing custom servers out of the box then that's a concretely different purpose to that which this WG was set up for; that change in direction will need to be approved at the primary WG. Worth discussing at HTTP WG first to see if there's consensus. I've already stated my position a few times, I think breaking backwards compatibility would be a really bad move and bad look for GraphQL - it's one of our major selling points - instead, I think we should encourage people to do the right thing more gradually. I can't make today's WG because I have a school commitment, but I'll be at the next one on 30th. |
Right 👍 Updated app.post("/graphql", async (req, res) => {
res.set("Content-Type", `application/graphql-response+json; charset=utf-8`);
if (typeof req.body !== "object" || req.body == null)
return res.status(400).json({ errors: [{ message: "Invalid GraphQL request" }] });
const { query, variables, operationName } = req.body;
if (typeof query !== "string")
return res.status(400).json({ errors: [{ message: "Missing query document" }] });
if (variables != null && (typeof variables !== "object" || Array.isArray(variables)))
return res.status(400).json({ errors: [{ message: "Invalid variables" }] });
if (operationName != null && typeof operationName !== "string")
return res.status(400).json({ errors: [{ message: "Invalid operation name" }] });
const result = await graphql({ schema, source: query, variableValues: variables, operationName })
if (!('data' in result)) return res.status(400).json(result);
res.json(result);
});The diff compared to the export const app = express();
app.use(express.json());
app.post("/graphql", async (req, res) => {
+ res.set("Content-Type", `application/graphql-response+json; charset=utf-8`);
if (typeof req.body !== "object" || req.body == null)
- return res.status(400).json({ error: "Invalid GraphQL request" });
+ return res.status(400).json({ errors: [{ message: "Invalid GraphQL request" }] });
const { query, variables, operationName } = req.body;
if (typeof query !== "string")
- return res.status(400).json({ error: "Missing query document" });
+ return res.status(400).json({ errors: [{ message: "Missing query document" }] });
if (variables != null && (typeof variables !== "object" || Array.isArray(variables)))
- return res.status(400).json({ error: "Invalid variables" });
+ return res.status(400).json({ errors: [{ message: "Invalid variables" }] });
if (operationName != null && typeof operationName !== "string")
- return res.status(400).json({ error: "Invalid operation name" });
- res.json(
- await graphql({ schema, source: query, variableValues: variables, operationName }),
- );
+ return res.status(400).json({ errors: [{ message: "Invalid operation name" }] });
+ const result = await graphql({ schema, source: query, variableValues: variables, operationName })
+ if (!('data' in result)) return res.status(400).json(result);
+ res.json(result);
});I still believe it's very acceptable. If this is still non-compliant, let me know and I'll update it.
Right, this is the biggest question. My thinking is that omitting I don't see this as a big bang. I expect everything to keep working. If at some point down the road something breaks, we should examine the cost of fixing the errors, vs the cost of having to support both media types forever. Let's discuss this on Apr 30th. |
7 participants
Follow up from #370 and #327
Move the
application/jsonresponses to a separate appendix to keep the spec easy to parse and implement (see also #329).The main normative change is the requirement to support
application/graphql-response+json. In a nutshell:All the rest is moving legacy parts of the spec to an appendix for better readability.
Servers can (and probably should) support both media types for better compatibility.
This is our one opportunity at a clean slate, let's keep the entropy and LLM contexts low.