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

Report more details about errors in do function #24

Open
wants to merge 2 commits into
base: master
from

Conversation

4 participants
@jorgesece

jorgesece commented Jun 25, 2018

Move status code control to the end of the 'do' function, so we can return more details about the error.

Report more details about errors in do function
Move status code control to the end of the 'do' function, so we can
return more details about the error.
graphql.go Outdated
return err
respBodyCopy, errRead := ioutil.ReadAll(resp.Body)
if errRead != nil {
fmt.Errorf("error decoding output: <an additional error ocurred while reading output: %s>", errRead)

This comment has been minimized.

@gopherci

gopherci bot Jun 25, 2018

staticcheck: Errorf is a pure function but its return value is ignored (SA4017)

graphql.go Outdated
if resp.StatusCode != http.StatusOK {
respBodyCopy, err := ioutil.ReadAll(resp.Body)
if err != nil {
fmt.Errorf("unexpected status: %v <an additional error ocurred while reading output: %s>", resp.Status, err)

This comment has been minimized.

@gopherci

gopherci bot Jun 25, 2018

staticcheck: Errorf is a pure function but its return value is ignored (SA4017)

@dmitshur

This comment has been minimized.

Member

dmitshur commented Jul 3, 2018

Hi, thanks for the PR. I'd like to understand this better, so we can implement a better solution.

Can you tell me under what conditions a non-200 response code is expected from a GraphQL server? How can I reproduce this?

@jorgesece

This comment has been minimized.

jorgesece commented Jul 9, 2018

@robermorales

This comment has been minimized.

robermorales commented Jul 13, 2018

Hey all,

A graphql server can return a 4xx error. As an HTTP server, if the error is found to be on the client side, it must return a 4xx error.

For instance, the postgraphile project has several 4xx status codes.

https://github.com/graphile/postgraphile/search?q=400&unscoped_q=400

@robermorales

This comment has been minimized.

robermorales commented Aug 15, 2018

Some news about merging this PR?

It will be so useful for us, since we used the library in production and this change can help us to debug errors.

Thanks in advance,

@dmitshur dmitshur added the thinking label Aug 19, 2018

@dmitshur

This comment has been minimized.

Member

dmitshur commented Aug 19, 2018

There are a few reasons I can't merge this PR yet:

  • It has correctness issues. Namely, the current code tries to do ioutil.ReadAll(resp.Body) after the resp.Body reader was already (partially or fully) consumed by json.NewDecoder(resp.Body).Decode(&out). It should probably be reading the resp.Body into a bytes.Buffer and then using that for JSON unmarshaling and error reporting.

  • It's not clear to me what status codes should be handled and how. The GraphQL spec doesn't document this. I think this is further support for #5, that the HTTP transport code should be factored out, making it easier for users of this library to customize the behavior (e.g., handle non-200 OK responses in a way that matches their specific target GraphQL server).

    Right now I'm hearing that 4xx codes can imply a valid GraphQL response, but it's hard to know whether that applies to all GraphQL servers or only specific ones. I want this library to be able to support any server that implements the GraphQL specification correctly.

I'll need to think more about how to best handle these issues. If you need custom behavior for your specific use case, I suggest using a modified version of this library until we come up with a satisfactory general solution.

@robermorales

This comment has been minimized.

robermorales commented Aug 28, 2018

Thanks, @dmitshur

We will continue using your wonderful library and perhaps we will maintain a light fork with the improved error reporting.

Let us know if you need with something. Thanks again.

@cjwagner

This comment has been minimized.

cjwagner commented Sep 18, 2018

Coming here from #29...
I think the do function should be doing a best effort attempt to return the response body as part of the error message for any error that occurs.

It's not clear to me what status codes should be handled and how. The GraphQL spec doesn't document this.

Currently only status code 200 is accepted and this PR doesn't change that so this PR isn't really related to which status codes should be handled.

I think this is further support for #5, that the HTTP transport code should be factored out, making it easier for users of this library to customize the behavior (e.g., handle non-200 OK responses in a way that matches their specific target GraphQL server).

Agreed, that sounds super useful, but I think the vast majority of users of the library would like the response body text from error responses to be exposed in error messages for error responses and it shouldn't be the case that most users need to write their own transport layer just to get a more descriptive error message.

Right now I'm hearing that 4xx codes can imply a valid GraphQL response, but it's hard to know whether that applies to all GraphQL servers or only specific ones. I want this library to be able to support any server that implements the GraphQL specification correctly.

As above, I think this is orthogonal to this PR since this PR doesn't modify which status codes should be considered errors, but rather includes the response body for whatever status codes we consider to be errors as part of the error message.

I definitely agree with the correctness problems, but those are easy to resolve once there is consensus on the direction of the PR.

dmitshur added a commit that referenced this pull request Sep 24, 2018

Include response body in non-200 OK error text.
When a GraphQL server returns a non-200 OK status code, it may be
very helpful to for caller to know what the response body was.
So, when such an error occurs, fetch the response body and
include it in the returned error text.

Fixes #29.
Updates #24.
@dmitshur

This comment has been minimized.

Member

dmitshur commented Sep 25, 2018

@jorgesece and @robermorales, I've merged PR #30, which should help. Please let me know if it works for your needs, or if doesn't completely cover them.

@robermorales

This comment has been minimized.

robermorales commented Sep 25, 2018

I commented on #30.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment