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

Include response body in 'unexpected status' error message. #29

Closed
cjwagner opened this issue Sep 17, 2018 · 3 comments
Closed

Include response body in 'unexpected status' error message. #29

cjwagner opened this issue Sep 17, 2018 · 3 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done.

Comments

@cjwagner
Copy link

The response body would be very useful for debugging error messages for unexpected statuses. Many 4xx and 5xx responses include useful explanations in their body text.
In particular the https://github.com/kubernetes/test-infra repo uses the https://github.com/shurcooL/githubv4 library to interact with GitHub's v4 API and we are seeing lots of 502 and 403 errors that should include explanations in the response bodies.
ref: kubernetes/test-infra#7466

The error is generated here:

graphql/graphql.go

Lines 72 to 74 in 3658993

if resp.StatusCode != http.StatusOK {
return fmt.Errorf("unexpected status: %v", resp.Status)
}

Adding a best effort ReadAll of the response body and appending it to the error text would make this error message much more useful.

I'd be happy to PR this change if this seems reasonable.

@dmitshur
Copy link
Member

dmitshur commented Sep 17, 2018

Thanks for the issue report @cjwagner, and for providing the details.

This issue is already on my radar, as PR #24 was sent to address a similar need. See discussion there for some background and #24 (comment) for latest status. The details you're provided in the issue should help find a viable resolution.

I'd prefer to think about this and discuss it in the issue first. Once we have agreement on a resolution plan, implementing the fix will be the easier part.

dmitshur added a commit that referenced this issue Sep 24, 2018
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
Copy link
Member

Ok, I think adding a ReadAll on the response body and including it in the error text is a reasonable first step to make, for now. Let's do that.

we are seeing lots of 502 and 403 errors that should include explanations in the response bodies.

I'm not sure whether to use %s or %q to format the response body. @cjwagner, would you be able to share sample response bodies during those errors, if you have them available?

A bigger step might be to create a struct error type with a ResponseBody []byte field in it, but I'd rather defer that until it's really needed.

I've sent PR #30, we can discuss the details there.

@cjwagner
Copy link
Author

Here is an example: Something went wrong while executing your query. This may be the result of a timeout, or it could be a GitHub bug. Please include 'EA06:8FDC:2D9BD3C:56E05DD:599AFE05' when reporting this issue.

The solution shouldn't be specific to the format or contents of the response body though since this is just an example from GitHub. The %q format token is more appropriate than %s or "%s" here as it will clearly indicate the start and end of the contents by escaping any "s.

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed thinking labels Sep 25, 2018
grihabor pushed a commit to grihabor/graphql that referenced this issue Jul 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Development

No branches or pull requests

2 participants