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

Handle response status codes #8

Open
kivutar opened this issue Jan 17, 2018 · 5 comments
Open

Handle response status codes #8

kivutar opened this issue Jan 17, 2018 · 5 comments

Comments

@kivutar
Copy link

kivutar commented Jan 17, 2018

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
Copy link
Contributor

matryer commented Mar 7, 2018

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
Copy link
Contributor

matryer commented May 22, 2018

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

@kivutar
Copy link
Author

kivutar commented May 22, 2018

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
Copy link
Contributor

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
Copy link
Contributor

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.

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

No branches or pull requests

3 participants