Go-github: Incorrect error-parsing on failure of creating review

[wataash]

Original error info from GitHub API (formatted for readability):

$ curl -H "Authorization: token XXXXXXXX" -X POST -d '{"commit_id": "ffffffffff"}' https://api.github.com/repos/wataash/sandbox/pulls/1/reviews
{
  "message": "Unprocessable Entity",
  "errors": [
    "Variable commitOID of type GitObjectID was provided invalid value"
  ],
  "documentation_url": "https://developer.github.com/v3/pulls/reviews/#create-a-pull-request-review"
}

indicates the commit id ffffffffff doesn't exist.

Same one via go-github:

ctx := context.Background()
ts := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: "XXXXXXXX"})
tc := oauth2.NewClient(ctx, ts)
client := github.NewClient(tc)
nonExistCommitID := "ffffffffff"
review := github.PullRequestReviewRequest{CommitID: &nonExistCommitID}
_, _, err := client.PullRequests.CreateReview(ctx, "wataash", "sandbox", 1, &review)
_, _ = fmt.Printf("%+v\n", err)

prints:

POST https://api.github.com/repos/wataash/sandbox/pulls/1/reviews: 422 Unprocessable Entity [{Resource: Field: Code: Message:}]

which lacks the error details but shows unrelated fields.

[wataash]

Most of the non-200 response formats seem to be undocumented, so I guess some other APIs also have this problem.

Thus making response body available as string would be helpful for go-github users when they faced incorrectly parsed errors.

I think performance degradation (as discussed in #1047) is negligible if the stringifying is done only on non-200.

[gmlewis]

Thank you for the side-by-side comparison, @wataash, and the reference to #1047 where we discussed returning the body for every response (which we rejected).

I'm not against returning the full body response for non-200 responses.

Thoughts, @gauntface, @willnorris, or anyone else?

[willnorris]

@wataash: some additional data can be fetched by casting the err to an ErrorResponse.

_, _, err := client.PullRequests.CreateReview(ctx, "wataash", "sandbox", 1, &review)
if er, ok := err.(github.ErrorResponse); ok {
    _, _ = fmt.Printf("%#v\n", er)
}

However, the errors array in this response is strings rather than structs, which go-github expects :-\

I think it seems reasonable to capture the response body for errors in a bufio.Reader or something and make it available in the resp.Response.Body. Because these error responses are often undocumented and inconsistent, there is stronger argument for paying the memory penalty discussed in #1047.

[gauntface]

If we could expose the response body in ErrorResponse that seems like we'd accept the memory penalty but enable a sane approach to debug with go-github.

[gmlewis]

OK, let's go ahead with this plan of populating the response body in the ErrorResponse.
Ideally we would only slurp the response body when generating the ErrorResponse, but let's see what implementation(s) the community comes up with.

This PR is open for a volunteer to take it on. I won't label this as good for beginner as this one is going to take a bit of thought and possibly experimentation to come up with a good solution.

Thank you, everyone, for your input, and thanks in advance to the person who takes on this PR.
We appreciate all contributions!

[vaibhavsingh97]

@gmlewis Can I take this up? I would love to fix the issue. I would love to do a little research and come back with a solution. Please redirect to me to the resources, I should look into before coming up with solutions

[gmlewis]

Ideally, this issue will be resolved by someone with experience writing Go clients.

For a starting point, the http portions of the Go standard library should be read and understood thoroughly:
https://godoc.org/net/http
https://godoc.org/net/http/httputil
https://godoc.org/net/http/httptest

The PR for this will need extensive testing (meaning writing unit tests) that cover a variety of cases.

I'm reluctant to assign this to anyone who needs me to direct them. This should be attempted by someone who already has an idea and plan for solving it... so I'm going to leave this open for now and contributors should feel free to post their PRs so that we as a community can discuss the pros and cons of their implementations.

[haya14busa]

The issue of inconsistent error structure is related with #540.