Access HTTP status codes

[pbostrom]

Is there any interest in providing access to the HTTP status code of a failed query? It would allow us to differentiate between graphql-level errors and HTTP-level errors (for example, a 403 when a user's session is expired). I would be willing to craft a PR if there is interest.

[oliyh]

Hi @pbostrom,

Yes, I can see why that would be helpful. One of my aims is to keep client code transport agnostic so one possible solution is that an unsuccessful status code should be pushed into the :errors key in the payload, i.e. payload looks like this:

{:errors [{:message "The HTTP call failed" 
           :status 403}]}

I think this might approximate what you would expect if you tried to make a query over the websocket that you were not permissioned to do. What do you think?

[pbostrom]

Hmm, let me think about it. Something I'm realizing about my particular use case is that we are retrofitting a GraphQL API to an existing REST API service. So another module handles authentication, and doesn't adhere to the GraphQL spec in the response. For example, in the unauthenticated case I mentioned, I get back a response like this:

{:errors {:details "Resource access requires authentication"}}

So :errors is a single map rather than a vector of maps.

I'm beginning to think maybe I should handle the quirks of my API in my own code rather than push additional error handling logic into the library.

[oliyh]

Have a read of https://github.com/walmartlabs/lacinia-pedestal/issues/50 - errors are inconsistent between HTTP and websocket anyway (the client will just have to cope with that).

Yours looks like a third way, so agree that it shouldn't be in this library, but if you wanted to do something with the status code like you originally suggested I'd still be open to that, where :message is a hardcoded string so at least you get an error in GraphQL terms.

[pbostrom]

I think the main thing I'm struggling with is that I can't predict what data I'll get back from the server, so I'll have to do some inspection of the response body here before I attempt to assoc in the status: https://github.com/oliyh/re-graph/blob/master/src/re_graph/internals.cljs#L11 Again, using my quirky API server as an example, I get nil back in the case of a 500 error; I get un-deserialized JSON back in the case of a 403 error (because the server did not correctly set the response header to application/json). So I'll have to check that the data is in the form of

{:errors [{:message ...}]}

before I attempt to assoc in the HTTP status code.

I suppose if it's nil I can construct and return the map myself. If the response does get deserialized into a Clojure map, but it's in the form I mentioned above:

{:errors {:details "Resource access requires authentication"}}

then should I attempt to coerce into

{:errors [{:message ... :status ...}]}

or just pass it along unmodified?

There may not be good answers to these questions by the way, I'm just thinking out loud.

[oliyh]

Hi,

I was thinking as a generic solution/first pass you could either:

1. Return the body from the server if it appears to be a proper GraphQL error
2. If it's not a GraphQL error then ignore the body and return a generic error message that looks like this:

   {:errors [{:message "The HTTP call failed" 
          :status 403}]}

Application developers could of course open the devtools and have a look at the actual response body, plus you could inspect the status code and decide what to do.

I wouldn't want to attempt to coerce a non-GraphQL message though as that would be very implementation specific.

[pbostrom]

After some research I noticed that the GraphQL spec says to not add additional top level entries to errors, but to use the extensions key. See: http://facebook.github.io/graphql/draft/#example-fce18 (scroll up a little bit to see the description). So the structure should look like:

{:errors [{:message "The HTTP call failed"
           :extensions {:status 403}}]}

The only remaining decision I have is whether to add the status to every error in the vector, or just the first one. I feel like every error map returned should have the extensions->status entry.

[oliyh]

Hi,

Yes, that sounds good then and a natural fit, thanks for researching. When would you have more than one error in the vector though in this scenario?

Cheers

[pbostrom]

I'm not sure, probably not in this particular scenario of 403/unauthenticated, but in the general case of handling any HTTP errors. For example, if I send a bad query to my GraphQL API like:

 {
   bad1(limit: 1) {
     name
     updated_at
   }
 }

I get back a 400 HTTP status with a body of:

{:errors [{:message "Cannot query field \"bad1\" on type \"RootQueryType\".",
           :locations [{:line 2, :column 0}]}
          {:message "Unknown argument \"limit\" on field \"bad1\" of type \"RootQueryType\".",
           :locations [{:line 2, :column 0}]}]}

I'm proposing that we would then insert (or update, if it already exists) the extensions map:

{:errors [{:message "Cannot query field \"bad1\" on type \"RootQueryType\".",
           :locations [{:line 2, :column 0}]
           :extensions {:status 400}}
          {:message "Unknown argument \"limit\" on field \"bad1\" of type \"RootQueryType\".",
           :locations [{:line 2, :column 0}]
           :extensions {:status 400}}]}

[fmnoise]

Good discussion, thanks guys, I'm also interested in that, let me know if I can help

[pbostrom]

PR created, @fmnoise feel free to take a look.

[oliyh]

Thanks @pbostrom I have merged your PR and done a deploy to clojars, 0.1.6-SNAPSHOT.