Surfacing HTTP Status Codes of Queries

[tim-steele]

Issue:
If a query returns a HTTP status code other than 200, express-graphql still returns 200, with the error placed in an array assigned to the errors property.

This can cause problems with caching. When error status codes are not set and a 200 is sent instead, the error or incomplete data will be cached.

Example:
You are hosting an online store. Someone views a product. The product details query returns an HTTP error, but the reviews query and recommendations query do not. That product's incomplete data is now cached due to the 200 status code, instead of surfacing the product details query's status code.

Proposed Solution:
I think the simplest solution would be creating a new option handleErrors, a function that receives the errors array and the response object on line 247 in src/index.js. If option was set, invoke it, otherwise do nothing. This will allow developers to access the errors and decide how they want to handle them or modify the response before being sent.

I wanted to gauge interest and feedback on this before I looked to submit a pull request. Please let me know if you would accept this PR and this proposed solution.

[lacker]

Where exactly is the incomplete data getting cached in your setup? It seems like in most cases you do not want to automatically cache successful responses, but also in most cases you could just send a POST to avoid any caching.

[tim-steele]

It is being cached at the Akamai layer.

We DO want to cache successful responses. We have had problems where one of the API responses returned an error, and the current implementation returns a status 200 with the error in the error property's array. This incomplete response is then cached. If we are able to surface our status codes, then we can opt not to cache the incomplete response.

[helfer]

I think the real issue is that there's not a neat overlap between HTTP error codes and errors in a GraphQL response. The response from the GraphQL server is indeed a 200, because it doesn't surface errors from the backend. Depending on the application it's perfectly fine to have that error and to cache it as well. If your goal with the status code is to prevent a CDN from caching it, I think it would be better to use the Cache-Control header.

Right now, I think express-graphql doesn't let you inspect the response before serializing and sending it, so you'd have to add another middleware and parse the response there, but if you're not overly attached to express-graphql, you can use apollo server, which is a simple GraphQL server just like express-graphql, but is more configurable and extensible. With apollo server you could use the formatResponse parameter to both format the response and set headers.

Caveat: apollo-server currently only allows POST requests which might defeat your purpose (even though they're technically cacheable). I'd be happy to accept a PR that adds support for GET though, since caching is pretty useful in some cases.

[tim-steele]

@helfer Unfortunately we are tied to express-graphql at this time. I added a PR that shows what I am referring to. Essentially, if you do not want to do anything with the errors, you can omit handleErrors as a function, and it will behave as normal. Otherwise, you can include handleErrors in the options and the results and response will be passed in so you have the freedom to modify the response as you see fit.

[tim-steele]

Also, added example to the PR.

[leebyron]

For reference I wrote up more thoughts on this here: #118 (comment)

The gist is that @helfer's post above is correct, HTTP error codes do not map to errors within a GraphQL result, instead they map to errors from running the query itself.

This is a consequence of GraphQL's fundamental difference from REST style APIs - because there is not a direct alignment between HTTP resources and data resources, HTTP error codes don't apply to individual fields fetched.

This discussion was really valuable though as it highlighted a bug! 200's were being returned for queries where data:null which according to the spec was an errored query, so #151 changed this to return 500 for that case.

[iki]

Actually, GraphQL specifies passing errors via errors field in the response and not using application transport protocol level, which may or may not be HTTP.

This concept can be simplified as always send 200 with graphql json response, or 500 if server blows up and is shared for example by facebook relay, graphcool, apollo-server, and graphene on server side, and expected for example by apollo-client on client-side.

Sending other error codes with graphql json response may result in graphql clients assuming server error and not trying to parse json in body, so the passed errors field is lost and error is replaced with generic http status error. This happens e.g. with apollo-client.

It would be nice to either unify on single concept, or make it configurable.

[StuAtGit]

@iki
Just a side note on this:

Actually, GraphQL specifies passing errors via errors field in the response and not using application transport protocol level, which may or may not be HTTP.

I followed the link, and it doesn't say that (at least I couldn't find anything close to that)

It says you put errors in the error field, but it does not say that you shouldn't surface them in HTTP (apologies for the double-negative!)

And it looks like the canonical graphql implementation may have switched to 422?
graphql/graphiql#88
More discussion here:
https://github.com/graphql/graphql-wg/blob/d3756c40e4f19931a680edd688b4b74feb146b0b/notes/2018-02-01.md

Note that even if the query pretty much fails horribly (fails to connect to service, timeouts, etc) it might still get 200/ok, depending on how gracefully your resolvers handle such things.

[tim-steele]

@StuAtGit we made modifications in our implementation to surface error codes. At large scale, it can become difficult with caching and error monitoring if everything is coming back 200 (or even 500). It is possible, you just have to do it manually.

[StuAtGit]

@tim-steele - now you're just making me miss using express-graphql instead of the apollo we're using now.... ;)
(which still seems to just be returning 200... on a failed outbound connection to a service).
I just ran into the error there, and started researching common practice and ran into this.

[allardhoeve]

@tim-steele: can you please link to the modifications? I'm very interested in the change.

[moekhalil]

I would probably stick with 200. I thought about using handleErrors and modifying it, but that actually would probably do more harm than good.

I am a very strong proponent of the right HTTP response code at the right time, but the thing with graphQL is I treat it as it's its own transport layer. Meaning I am using GraphQL INSTEAD of HTTP GET/POST/PUT/PATCH/DELETE. So if I am not using HTTP Methods, why on earth would I want HTTP Status Codes. I like "createPerson" better anyhow.

Another reason to stick to 200 is simple; graphQL allows me to execute parallel functions.

mutation modifyStudents(Student withdrawingStudent, Student newStudent) {
  deleteStudent(withdrawingStudent) {
    name
  } // yay, successfully deleted this dropout!
  createPerson(newStudent) {
    name
  } //  oops! This failed! New student gave me a throwaway email!
}

If one works and the other fails, are you going to return a 200 or 500? Or are you going to return a 207 but have a totally unrelated and invalid response body?

[tim-steele]

@allardhoeve - We are using Apollo, and we use formatError. It a nutshell, it is like this:

function formatError(error, res) {
  res.status(error.status);
  const formattedError = myFormatFunction(error);

  return formattedError;
}

@moekhalil agree the multiple responses make it difficult. It isn't a "simple" problem to solve. We generally default to 500 for server problem, 400 for client problem. Ultimately, when everything is a 200 it can create problems with caching layers and server monitoring. Also, for Apollo, if you return an error it doesn't cache it locally, and triggers the catch() on mutations vs. having to data check to validate responses. My $.02.