graphql icon indicating copy to clipboard operation
graphql copied to clipboard

Handle response status codes

Open kivutar opened this issue 6 years ago • 5 comments

I think there should be something like

if res.StatusCode != http.StatusOK {

after

res, err := c.httpClient.Do(r)

In case the server returns a 400 or a 500, the current code returns:

2018/01/17 17:44:37 << {"errors":[{}]}
graphql:

kivutar avatar Jan 17 '18 10:01 kivutar

Will there be a situation where we get an invalid HTTP status code, and a descriptive error in the JSON body? I suppose we might need to handle a few different possibilities:

  • non-200 response code with no error in body (like 404, the GraphQL endpoint wasn't found)
  • non-200 response code with a specific error in the body (like 401 and errors:[{message:"Not authorized"}]
  • 200 repsonse code with an error in the response body

matryer avatar Mar 07 '18 08:03 matryer

@kivutar Would you be able to you submit a failing test for what you expect here?

matryer avatar May 22 '18 10:05 matryer

Sorry matryer, it's been a while since the last time I used the library, and now I'm doing completely unrelated things.

I don't know HTTP enough to reply about the status codes. But most of the graphql servers I have been working with were using 200 in 99% of the cases, when displaying errors or normal payload. And 500 or 404 when the server is buggy or not well configured.

kivutar avatar May 22 '18 13:05 kivutar

I think letting the client handle the Errors object in the response is probably the best way to go since you can still receive a partial response of data. The main issue here is that if a non-200 status code is returned, there's still an attempt to marshal data and that throws an error that's not clear "invalid character 'N' looking for beginning of value" as an example.

From what I've read it seems reasonable to expect a 200 status code always and if we don't then there's an error from the server. In those cases, don't attempt to unmarshal and return an informative error with the status code in it.

erutherford avatar Aug 31 '18 20:08 erutherford

I went ahead and submitted a PR @matryer I hope that was ok. I'm also open to any other approaches you'd like to suggest, but it seemed a like a clean and simple way to maintain current functionality, but provide a more informative error if the client isn't able to parse JSON in the response.

erutherford avatar Aug 31 '18 22:08 erutherford