machinebox / graphql

Simple low-level GraphQL HTTP client for Go
https://blog.machinebox.io/a-graphql-client-library-for-go-5bffd0455878
Apache License 2.0
933 stars 218 forks source link

Handle response status codes #8

Open kivutar opened 6 years ago

kivutar commented 6 years ago

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:
matryer commented 6 years ago

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:

matryer commented 6 years ago

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

kivutar commented 6 years ago

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.

erutherford commented 6 years ago

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 commented 6 years ago

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.