Possible missed json tag in github.ErrorResponse

[migueleliasweb]

Hi all,

I've been struck by an weird interaction between Github's API and go-github on migueleliasweb/go-github-mock#6 for a while now.

The issue seems to be that mocking errors from the API can be trickier than I first thought.

Recently, someone contributed with a pull request that mitigated the problem in migueleliasweb/go-github-mock#41 but I think the problem goes a bit deeper than that. Although the mitigation PR fixes the issue, I think the root cause should be fixed in this repo.

The root cause seems to be that github.ErrorResponse is missing a json tag for ommiting the Response attr when it's not set.

Here's why I think this is it:

Here's a not" found error" returned from the real Github's API, notice there's no Response attribute set.

{"message":"Not Found","documentation_url":"https://docs.github.com/rest/reference/teams#list-team-members"}

When the SDK receives this response, it first checks the status code and then goes and creates an error response that contains the mashalled information in the body.

See: https://github.com/google/go-github/blob/master/github/github.go#L1076

func CheckResponse(r *http.Response) error {
	if r.StatusCode == http.StatusAccepted {
		return &AcceptedError{}
	}
	if c := r.StatusCode; 200 <= c && c <= 299 {
		return nil
	}

	errorResponse := &ErrorResponse{Response: r}
	data, err := io.ReadAll(r.Body)
	if err == nil && data != nil {
		json.Unmarshal(data, errorResponse) // <- this is what causes the issue down the line
	}
        // [...] ommited for clarity
}

The problem here is that IF and ONLY IF the r.Body contains a nil Response atribute, the following function call will throw a panic: runtime error: invalid memory address or nil pointer dereference.

See: https://github.com/google/go-github/blob/master/github/github.go#L863

func (r *ErrorResponse) Error() string {
	return fmt.Sprintf("%v %v: %d %v %+v",
		r.Response.Request.Method, sanitizeURL(r.Response.Request.URL),
		r.Response.StatusCode, r.Message, r.Errors)
}

This happens becasue in the go-github-mock lib, I'm using the github.ErrorReponse to fake error responses from the API see. The problem is that, from Golang's perspective, there will always be a Response set inside the struct as there is no json tab to ommit that property if it's empty.

Basically, to make this work properly all we need is to add json:"omitempty" to the Response attr in github.ErrorResponse.

Let me know your thoughts 👍

[migueleliasweb]

I ve run a quick experiment adding that json tag using the master branch of go-github and running all the tests locally. They all pass.