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

Require support of application/graphql-response+json by servers #327

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Shane32
Copy link
Contributor

@Shane32 Shane32 commented Jan 30, 2025

As of 1/1/2025, the proposed watershed date has passed. As the GraphQL over HTTP specification has not been released yet, eliminating the watershed period entirely may leave clients without adequate time to adjust to some of the new behaviors mandated by the specification.

This PR aims to separate the date that the server is required to support application/graphql-response+json from the date that it is required to be considered the default when no Accept header is provided. This allows for clients to immediately take advantage of the benefits of the application/graphql-response+json media type for all compliant servers.

I'm suggesting that this support is required immediately. While the original concept was that the spec would be released quickly, representing the current state of GraphQL servers, this did not occur. However, the draft GraphQL-over-HTTP spec has been out since 2020, and has had no changes of substance (of any kind) since the addition of the application/graphql-response+json media type in Aug 2022. As such, the fact that it was not "released as a spec" makes little difference. Server implementations have had time to review and implement the new features of the draft specification, and various servers have done so.

Further, releasing the spec with this change does not break existing implementations. They simply are not officially compliant until they support the new content type. And finally, popular server implementations (assuming they have been watching this repository) have been expecting this change to take effect on 1/1/2025 and would expect to be non-compliant if they did not support the new content type.

So why make this change now? I believe the sooner that all servers support the new media type, the sooner that the entire GraphQL ecosystem will adopt it. Otherwise simple clients (e.g. handwritten with fetch) might not be able to use the new media type if servers are not supporting it, delaying general adoption.

This PR is complementary to pushing the watershed date further out as proposed within:

Copy link
Member

@enisdenjo enisdenjo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've put it well in the description, I think this makes sense.

@benjie
Copy link
Member

benjie commented Feb 13, 2025

This allows for clients to immediately take advantage of the benefits of the application/graphql-response+json media type for all compliant servers.

Clients can already use these benefits for responses that use it. For servers that don't support this content type, clients will still need to handle old-style responses. Fortunately, clients should be able to base decisions on the body of responses that return 2XX status and/or use application/graphql-response+json — and that is the case with or without the change proposed in this PR.

The main thing that this change accomplishes is it makes it reasonable for clients to drop support for servers that don't implement application/graphql-response+json, breaking compatibility with legacy servers. I think that would be a mistake, and could do great harm to the GraphQL ecosystem if not sufficiently well signposted. I think adding MUST support for application/graphql-response+json was probably the mistake on my part - that should either have been pushed a long way into the future, or simply removed. Actually... I think there might be a better solution.

the fact that it was not "released as a spec" makes little difference

It does make a difference. It's not an official spec, and as such only the people involved in this WG and their colleagues are likely to have implemented it - we've hardly told anyone else about it! I'm sure all the big servers may have done, but there's a lot of software out there that serves GraphQL in really simple ways. You can spin up an old-school GraphQL server in like 20 lines of code (parse request body, validate types of parameters, feed result into graphql() function, await and JSON stringify result, write back to wire with 200 status code and application/json content type); not everyone will be using one of the off-the-shelf GraphQL servers for such a minor need, and this has worked fine for 10 years.

Further, releasing the spec with this change does not break existing implementations.

No, but it does mean that clients don't need to support them any more, and thus a software update to the latest client may break existing applications, causing people to choose between running outdated software on the client or nagging their server teams to deploy servers with updated compliance - requests that may well be met with crickets in some environments.

I believe the sooner that all servers support the new media type, the sooner that the entire GraphQL ecosystem will adopt it.

Agreed, but making it mandatory and marking everyone who doesn't do it as non-compliant may not be the best way of achieving this goal... Let's use the carrot rather than the stick - talk to people about the benefits of using this content type, and give them time to migrate over.

Otherwise simple clients (e.g. handwritten with fetch) might not be able to use the new media type if servers are not supporting it, delaying general adoption.

What are you seeing that simple clients will get out of this change? If their requests are successful or partially successful they'll get 2xx same as they ever did; the only statuses that change are those that fail completely (e.g. due to being invalid) can no longer be 2xx status, and for those they can either check for a missing data property, or for errors presence, same as they always could.

The main value of the application/graphql-response+json media type is not for clients nor for servers, it's for integration with existing HTTP tooling such as logging, caches, intrusion detection, observability, etc - allowing these external systems to understand requests without having to parse their bodies.

@Shane32
Copy link
Contributor Author

Shane32 commented Feb 13, 2025

What are you seeing that simple clients will get out of this change?

I can only offer my own personal experience, which shapes my own viewpoint, and why I am advocating for the switch.

I started working with GraphQL around 6 years ago. I was tasked with writing a high-performance website, and chose to use GraphQL over simple REST APIs as it met all of these objectives:

  • Underlying database need only supply data for the requested fields, making use of indexes on large datasets
  • HTTP transport need only supply data for the requested fields, lowering bandwidth requirements
  • Complex queries (meaning, multiple inner joins) could be handled by the database directly, reducing latency between both the database and server, and more importantly, the server and client
  • Very easy to develop with, and new fields/features do not (necessarily) break existing clients

However, the Apollo client for React had multiple major issues and could not be used. First, it was very bulky, disproportionate to the rest of the application size. Second, it was very slow, on two fronts: application startup incurred significant delays by Apollo processing the response (CPU time), and it injected __typename into every query, increasing bandwidth requirements. And finally, it had problems with how it handled resetStore, breaking the login/logout portions of the site.

I have no doubt that by now Apollo or another library may work for me now, but at the time, I just needed and wrote some React wrappers for fetch, designing a lightweight GraphQL library with some basic caching features, which has worked well for me.

This brings me to the content type and response codes. I wanted my server to be able to easily tell whether the result was a good request or not. So I configured my servers to issue 4xx responses appropriately. I like being able to review the HTTP logs and determine which requests are failing (typically auth errors) and not. However, as many people have indicated, this can cause a conflict with HTTP relays and such returning 400 also.

As such, application/graphql-response+json seems like it would have been a better choice, had that been available. If my testing of GraphiQL had noticed that application/graphql-response+json was used, I would have modeled my client library after it. But at the time all the software used application/json. This is why I am pushing for the requirement now. If the major GraphQL libraries had used the new content type, I would have modeled my client library after them. If, on the other hand, everyone continues to use application/json, any developer writing their own implementation will just follow the pattern of everyone else.

As such, I recommend that application/graphql-response+json be required and application/json should in some way be discouraged while still being supported. I just believe it's better for the community as a whole.

So, this PR simply requires support of the new content type. It doesn't remove the requirement that application/json be supported. In fact, with the current watershed date, it does not change anything at all in the spec. But in general, favoring the new content type paves the way for new libraries to also do so. The only situation the watershed breaks (which this PR itself does not change) are legacy clients that both (1) don't send an accept header, and (2) reject an application/graphql-response+json content type -- and that's assuming that the server is upgraded without the client being upgraded.

@benjie
Copy link
Member

benjie commented Feb 13, 2025

What you describe explains exactly why this exists - so that you can see from the server logs what requests failed/succeeded - but it doesn't seem to answer the question as to how your client itself would benefit?

So, this PR simply requires support of the new content type. It doesn't remove the requirement that application/json be supported.

Indeed, but it does allow clients to stop supporting application/json, thereby dropping compatibility with existing (legacy) servers. That's what I think is a bad idea.

If we require all servers support the new content type, then taken at face value there's no reason for clients to support the legacy content type even though the code required to do so is minimal (i.e. to uncomment the commented part of this code: if (res.headers.contentType === 'application/graphql-response+json' /* || (res.statusCode >= 200 && res.statusCode < 300) */) { handleTheResult(res.json) } and to add application/json to their Accept header).

@Shane32
Copy link
Contributor Author

Shane32 commented Feb 13, 2025

But the reverse is just as true. Why support the new content type if all servers support the old one? Why add support for a content type that is not even required to be supported by the spec?

I follow your logic about the clients. But it seems we are fixing the wrong end of the problem. Why not simply require compliant clients to support both content types until 2030?

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

Successfully merging this pull request may close these issues.

4 participants