Report more details about errors in do function

[jorgesece]

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

[dmitshur]

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]

Hi! For instance, when you execute a request with wrong graphql arguments it retrieves 400 response, and it is difficult to understand which is the problem. It could be either bad argument name, extra variables non-used or another query mistake. It makes hard/slow to implement queries. Graphiql returns real errors, so the developer can figure out which is his mistake faster.

Does it make sense?

Thanks, Jorge

2018-07-03 20:24 GMT+02:00 Dmitri Shuralyov notifications@github.com:

Hi, thanks for the PR. I'd like to understand it 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?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/shurcooL/graphql/pull/24#issuecomment-402250497, or mute the thread https://github.com/notifications/unsubscribe-auth/ABDM3r6RbsdQ09ljvUTxt9V_2N9PNJgXks5uC7b0gaJpZM4U2dpE .

--

- Jorge Sevilla Cedillo -

[robermorales]

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]

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]

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]

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]

Coming here from https://github.com/shurcooL/graphql/issues/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]

@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]

I commented on #30.