From 0c21cda58c0ac5b8a4bd9dfe7ffdce50eca0c30f Mon Sep 17 00:00:00 2001 From: Munia Balayil Date: Tue, 13 Apr 2021 11:12:31 +0530 Subject: [PATCH 1/3] Update error handling as per Go 1.13 guidelines --- README.md | 2 +- github/github.go | 74 ++++++++++++++++++++++++++++++++++++++++++- github/github_test.go | 12 +++---- 3 files changed, 80 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index df43395d488..68f8de8c33f 100644 --- a/README.md +++ b/README.md @@ -9,7 +9,7 @@ go-github is a Go client library for accessing the [GitHub API v3][]. -Currently, **go-github requires Go version 1.9 or greater**. go-github tracks +Currently, **go-github requires Go version 1.13 or greater**. go-github tracks [Go's version support policy][support-policy]. We do our best not to break older versions of Go if we don't have to, but due to tooling constraints, we don't always test older versions. diff --git a/github/github.go b/github/github.go index 4489dab2ab2..ea1f856a865 100644 --- a/github/github.go +++ b/github/github.go @@ -132,6 +132,8 @@ const ( mediaTypeContentAttachmentsPreview = "application/vnd.github.corsair-preview+json" ) +var errNonNilContext = errors.New("context must be non-nil") + // A Client manages communication with the GitHub API. type Client struct { clientMu sync.Mutex // clientMu protects the client during calls that modify the CheckRedirect func. @@ -530,7 +532,7 @@ func parseRate(r *http.Response) Rate { // canceled or times out, ctx.Err() will be returned. func (c *Client) BareDo(ctx context.Context, req *http.Request) (*Response, error) { if ctx == nil { - return nil, errors.New("context must be non-nil") + return nil, errNonNilContext } req = withContext(ctx, req) @@ -681,6 +683,36 @@ func (r *ErrorResponse) Error() string { r.Response.StatusCode, r.Message, r.Errors) } +func (r *ErrorResponse) Is(target error) bool { + v, ok := target.(*ErrorResponse) + if !ok { + return false + } + if (r.Message != v.Message) || (r.DocumentationURL != v.DocumentationURL) { + return false + } + if r.Response != nil { + if r.Response.StatusCode != v.Response.StatusCode { + return false + } + } + if len(r.Errors) != len(v.Errors) { + return false + } + for idx := range r.Errors { + if r.Errors[idx] != v.Errors[idx] { + return false + } + } + if r.Block != nil { + if r.Block.Reason != v.Block.Reason || *(r.Block).CreatedAt != + *(v.Block).CreatedAt { + return false + } + } + return true +} + // TwoFactorAuthError occurs when using HTTP Basic Authentication for a user // that has two-factor authentication enabled. The request can be reattempted // by providing a one-time password in the request. @@ -702,6 +734,22 @@ func (r *RateLimitError) Error() string { r.Response.StatusCode, r.Message, formatRateReset(time.Until(r.Rate.Reset.Time))) } +func (r *RateLimitError) Is(target error) bool { + v, ok := target.(*RateLimitError) + if !ok { + return false + } + if r.Rate != v.Rate || r.Message != v.Message { + return false + } + if r.Response != nil { + if r.Response.StatusCode != v.Response.StatusCode { + return false + } + } + return true +} + // AcceptedError occurs when GitHub returns 202 Accepted response with an // empty body, which means a job was scheduled on the GitHub side to process // the information needed and cache it. @@ -717,6 +765,14 @@ func (*AcceptedError) Error() string { return "job scheduled on GitHub side; try again later" } +func (ae *AcceptedError) Is(target error) bool { + v, ok := target.(*AcceptedError) + if !ok { + return false + } + return bytes.Compare(ae.Raw, v.Raw) == 0 +} + // AbuseRateLimitError occurs when GitHub returns 403 Forbidden response with the // "documentation_url" field value equal to "https://docs.github.com/en/free-pro-team@latest/rest/reference/#abuse-rate-limits". type AbuseRateLimitError struct { @@ -735,6 +791,22 @@ func (r *AbuseRateLimitError) Error() string { r.Response.StatusCode, r.Message) } +func (r *AbuseRateLimitError) Is(target error) bool { + v, ok := target.(*AbuseRateLimitError) + if !ok { + return false + } + if r.Message != v.Message || r.RetryAfter != v.RetryAfter { + return false + } + if r.Response != nil { + if r.Response.StatusCode != v.Response.StatusCode { + return false + } + } + return true +} + // sanitizeURL redacts the client_secret parameter from the URL which may be // exposed to the user. func sanitizeURL(uri *url.URL) *url.URL { diff --git a/github/github_test.go b/github/github_test.go index db50b465555..3d87907e9ba 100644 --- a/github/github_test.go +++ b/github/github_test.go @@ -705,7 +705,7 @@ func TestDo_nilContext(t *testing.T) { req, _ := client.NewRequest("GET", ".", nil) _, err := client.Do(nil, req, nil) - if !reflect.DeepEqual(err, errors.New("context must be non-nil")) { + if !errors.Is(err, errNonNilContext) { t.Errorf("Expected context must be non-nil error") } } @@ -1097,7 +1097,7 @@ func TestCheckResponse(t *testing.T) { CreatedAt: &Timestamp{time.Date(2016, time.March, 17, 15, 39, 46, 0, time.UTC)}, }, } - if !reflect.DeepEqual(err, want) { + if !errors.Is(err, want) { t.Errorf("Error = %#v, want %#v", err, want) } } @@ -1125,7 +1125,7 @@ func TestCheckResponse_RateLimit(t *testing.T) { Response: res, Message: "m", } - if !reflect.DeepEqual(err, want) { + if !errors.Is(err, want) { t.Errorf("Error = %#v, want %#v", err, want) } } @@ -1147,7 +1147,7 @@ func TestCheckResponse_AbuseRateLimit(t *testing.T) { Response: res, Message: "m", } - if !reflect.DeepEqual(err, want) { + if !errors.Is(err, want) { t.Errorf("Error = %#v, want %#v", err, want) } } @@ -1168,7 +1168,7 @@ func TestCheckResponse_noBody(t *testing.T) { want := &ErrorResponse{ Response: res, } - if !reflect.DeepEqual(err, want) { + if !errors.Is(err, want) { t.Errorf("Error = %#v, want %#v", err, want) } } @@ -1191,7 +1191,7 @@ func TestCheckResponse_unexpectedErrorStructure(t *testing.T) { Message: "m", Errors: []Error{{Message: "error 1"}}, } - if !reflect.DeepEqual(err, want) { + if !errors.Is(err, want) { t.Errorf("Error = %#v, want %#v", err, want) } data, err2 := ioutil.ReadAll(err.Response.Body) From e8f55e28f2b0259386f1b6ce463d3fde7e3775ba Mon Sep 17 00:00:00 2001 From: Munia Balayil Date: Wed, 14 Apr 2021 08:41:49 +0530 Subject: [PATCH 2/3] Restructure code and add tests --- github/github.go | 75 ++++++---- github/github_test.go | 334 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 382 insertions(+), 27 deletions(-) diff --git a/github/github.go b/github/github.go index ea1f856a865..efde8f8dbab 100644 --- a/github/github.go +++ b/github/github.go @@ -655,6 +655,20 @@ func (c *Client) checkRateLimitBeforeDo(req *http.Request, rateLimitCategory rat return nil } +// compareHttpResponse returns whether two http.Response objects are equal or not. +// Currently, only StatusCode is checked. This function is used when implementing the +// Is(error) bool interface for the custom error types in this package. +func compareHttpResponse(r1, r2 *http.Response) bool { + if r1 == nil && r2 == nil { + return true + } + + if r1 != nil && r2 != nil { + return r1.StatusCode == r2.StatusCode + } + return false +} + /* An ErrorResponse reports one or more errors caused by an API request. @@ -683,19 +697,19 @@ func (r *ErrorResponse) Error() string { r.Response.StatusCode, r.Message, r.Errors) } +// Is returns whether the provided error equals this error. func (r *ErrorResponse) Is(target error) bool { v, ok := target.(*ErrorResponse) if !ok { return false } - if (r.Message != v.Message) || (r.DocumentationURL != v.DocumentationURL) { + + if r.Message != v.Message || (r.DocumentationURL != v.DocumentationURL) || + !compareHttpResponse(r.Response, v.Response) { return false } - if r.Response != nil { - if r.Response.StatusCode != v.Response.StatusCode { - return false - } - } + + // Compare Errors. if len(r.Errors) != len(v.Errors) { return false } @@ -704,12 +718,26 @@ func (r *ErrorResponse) Is(target error) bool { return false } } - if r.Block != nil { - if r.Block.Reason != v.Block.Reason || *(r.Block).CreatedAt != - *(v.Block).CreatedAt { + + // Compare Block. + if (r.Block != nil && v.Block == nil) || (r.Block == nil && v.Block != nil) { + return false + } + if r.Block != nil && v.Block != nil { + if r.Block.Reason != v.Block.Reason { + return false + } + if (r.Block.CreatedAt != nil && v.Block.CreatedAt == nil) || (r.Block.CreatedAt == + nil && v.Block.CreatedAt != nil) { return false } + if r.Block.CreatedAt != nil && v.Block.CreatedAt != nil { + if *(r.Block.CreatedAt) != *(v.Block.CreatedAt) { + return false + } + } } + return true } @@ -734,20 +762,16 @@ func (r *RateLimitError) Error() string { r.Response.StatusCode, r.Message, formatRateReset(time.Until(r.Rate.Reset.Time))) } +// Is returns whether the provided error equals this error. func (r *RateLimitError) Is(target error) bool { v, ok := target.(*RateLimitError) if !ok { return false } - if r.Rate != v.Rate || r.Message != v.Message { - return false - } - if r.Response != nil { - if r.Response.StatusCode != v.Response.StatusCode { - return false - } - } - return true + + return r.Rate == v.Rate && + r.Message == v.Message && + compareHttpResponse(r.Response, v.Response) } // AcceptedError occurs when GitHub returns 202 Accepted response with an @@ -765,6 +789,7 @@ func (*AcceptedError) Error() string { return "job scheduled on GitHub side; try again later" } +// Is returns whether the provided error equals this error. func (ae *AcceptedError) Is(target error) bool { v, ok := target.(*AcceptedError) if !ok { @@ -791,20 +816,16 @@ func (r *AbuseRateLimitError) Error() string { r.Response.StatusCode, r.Message) } +// Is returns whether the provided error equals this error. func (r *AbuseRateLimitError) Is(target error) bool { v, ok := target.(*AbuseRateLimitError) if !ok { return false } - if r.Message != v.Message || r.RetryAfter != v.RetryAfter { - return false - } - if r.Response != nil { - if r.Response.StatusCode != v.Response.StatusCode { - return false - } - } - return true + + return r.Message == v.Message && + r.RetryAfter == v.RetryAfter && + compareHttpResponse(r.Response, v.Response) } // sanitizeURL redacts the client_secret parameter from the URL which may be diff --git a/github/github_test.go b/github/github_test.go index 3d87907e9ba..7f9d01051ac 100644 --- a/github/github_test.go +++ b/github/github_test.go @@ -1152,6 +1152,340 @@ func TestCheckResponse_AbuseRateLimit(t *testing.T) { } } +func TestCompareHttpResponse(t *testing.T) { + testcases := map[string]struct { + h1 *http.Response + h2 *http.Response + expected bool + }{ + "both are nil": { + expected: true, + }, + "both are non nil - same StatusCode": { + expected: true, + h1: &http.Response{StatusCode: 200}, + h2: &http.Response{StatusCode: 200}, + }, + "both are non nil - different StatusCode": { + expected: false, + h1: &http.Response{StatusCode: 200}, + h2: &http.Response{StatusCode: 404}, + }, + "one is nil, other is not": { + expected: false, + h2: &http.Response{}, + }, + } + + for name, tc := range testcases { + t.Run(name, func(t *testing.T) { + v := compareHttpResponse(tc.h1, tc.h2) + if tc.expected != v { + t.Errorf("Expected %t, got %t for (%#v, %#v)", tc.expected, v, tc.h1, tc.h2) + } + }) + } +} + +func TestErrorResponse_Is(t *testing.T) { + err := ErrorResponse{ + Response: &http.Response{}, + Message: "m", + Errors: []Error{{Resource: "r", Field: "f", Code: "c"}}, + Block: &struct { + Reason string `json:"reason,omitempty"` + CreatedAt *Timestamp `json:"created_at,omitempty"` + }{ + Reason: "r", + CreatedAt: &Timestamp{time.Date(2016, time.March, 17, 15, 39, 46, 0, time.UTC)}, + }, + DocumentationURL: "https://github.com", + } + testcases := map[string]struct { + wantSame bool + otherError error + }{ + "errors are same": { + wantSame: true, + otherError: &ErrorResponse{ + Response: &http.Response{}, + Errors: []Error{{Resource: "r", Field: "f", Code: "c"}}, + Message: "m", + Block: &struct { + Reason string `json:"reason,omitempty"` + CreatedAt *Timestamp `json:"created_at,omitempty"` + }{ + Reason: "r", + CreatedAt: &Timestamp{time.Date(2016, time.March, 17, 15, 39, 46, 0, time.UTC)}, + }, + DocumentationURL: "https://github.com", + }, + }, + "errors have different values - Message": { + wantSame: false, + otherError: &ErrorResponse{ + Response: &http.Response{}, + Errors: []Error{{Resource: "r", Field: "f", Code: "c"}}, + Message: "m1", + Block: &struct { + Reason string `json:"reason,omitempty"` + CreatedAt *Timestamp `json:"created_at,omitempty"` + }{ + Reason: "r", + CreatedAt: &Timestamp{time.Date(2016, time.March, 17, 15, 39, 46, 0, time.UTC)}, + }, + DocumentationURL: "https://github.com", + }, + }, + "errors have different values - DocumentationURL": { + wantSame: false, + otherError: &ErrorResponse{ + Response: &http.Response{}, + Errors: []Error{{Resource: "r", Field: "f", Code: "c"}}, + Message: "m", + Block: &struct { + Reason string `json:"reason,omitempty"` + CreatedAt *Timestamp `json:"created_at,omitempty"` + }{ + Reason: "r", + CreatedAt: &Timestamp{time.Date(2016, time.March, 17, 15, 39, 46, 0, time.UTC)}, + }, + DocumentationURL: "https://google.com", + }, + }, + "errors have different values - Response is nil": { + wantSame: false, + otherError: &ErrorResponse{ + Errors: []Error{{Resource: "r", Field: "f", Code: "c"}}, + Message: "m", + Block: &struct { + Reason string `json:"reason,omitempty"` + CreatedAt *Timestamp `json:"created_at,omitempty"` + }{ + Reason: "r", + CreatedAt: &Timestamp{time.Date(2016, time.March, 17, 15, 39, 46, 0, time.UTC)}, + }, + DocumentationURL: "https://github.com", + }, + }, + "errors have different values - Errors": { + wantSame: false, + otherError: &ErrorResponse{ + Errors: []Error{{Resource: "r1", Field: "f1", Code: "c1"}}, + Message: "m", + Block: &struct { + Reason string `json:"reason,omitempty"` + CreatedAt *Timestamp `json:"created_at,omitempty"` + }{ + Reason: "r", + CreatedAt: &Timestamp{time.Date(2016, time.March, 17, 15, 39, 46, 0, time.UTC)}, + }, + DocumentationURL: "https://github.com", + }, + }, + "errors have different values - Block": { + wantSame: false, + otherError: &ErrorResponse{ + Errors: []Error{{Resource: "r", Field: "f", Code: "c"}}, + Message: "m", + DocumentationURL: "https://github.com", + }, + }, + "errors have different types": { + wantSame: false, + otherError: errors.New("Github"), + }, + } + + for name, tc := range testcases { + t.Run(name, func(t *testing.T) { + if tc.wantSame != err.Is(tc.otherError) { + t.Errorf("Error = %#v, want %#v", err, tc.otherError) + } + }) + } +} + +func TestRateLimitError_Is(t *testing.T) { + err := &RateLimitError{ + Response: &http.Response{}, + Message: "Github", + } + testcases := map[string]struct { + wantSame bool + err *RateLimitError + otherError error + }{ + "errors are same": { + wantSame: true, + err: err, + otherError: &RateLimitError{ + Response: &http.Response{}, + Message: "Github", + }, + }, + "errors are same - Response is nil": { + wantSame: true, + err: &RateLimitError{ + Message: "Github", + }, + otherError: &RateLimitError{ + Message: "Github", + }, + }, + "errors have different values - Rate": { + wantSame: false, + err: err, + otherError: &RateLimitError{ + Rate: Rate{Limit: 10}, + Response: &http.Response{}, + Message: "Gitlab", + }, + }, + "errors have different values - Response is nil": { + wantSame: false, + err: err, + otherError: &RateLimitError{ + Message: "Github", + }, + }, + "errors have different values - StatusCode": { + wantSame: false, + err: err, + otherError: &RateLimitError{ + Response: &http.Response{StatusCode: 200}, + Message: "Github", + }, + }, + "errors have different types": { + wantSame: false, + err: err, + otherError: errors.New("Github"), + }, + } + + for name, tc := range testcases { + t.Run(name, func(t *testing.T) { + if tc.wantSame != tc.err.Is(tc.otherError) { + t.Errorf("Error = %#v, want %#v", tc.err, tc.otherError) + } + }) + } +} + +func TestAbuseRateLimitError_Is(t *testing.T) { + t1 := 1 * time.Second + t2 := 2 * time.Second + err := &AbuseRateLimitError{ + Response: &http.Response{}, + Message: "Github", + RetryAfter: &t1, + } + testcases := map[string]struct { + wantSame bool + err *AbuseRateLimitError + otherError error + }{ + "errors are same": { + wantSame: true, + err: err, + otherError: &AbuseRateLimitError{ + Response: &http.Response{}, + Message: "Github", + RetryAfter: &t1, + }, + }, + "errors are same - Response is nil": { + wantSame: true, + err: &AbuseRateLimitError{ + Message: "Github", + RetryAfter: &t1, + }, + otherError: &AbuseRateLimitError{ + Message: "Github", + RetryAfter: &t1, + }, + }, + "errors have different values - Message": { + wantSame: false, + err: err, + otherError: &AbuseRateLimitError{ + Response: &http.Response{}, + Message: "Gitlab", + RetryAfter: nil, + }, + }, + "errors have different values - RetryAfter": { + wantSame: false, + err: err, + otherError: &AbuseRateLimitError{ + Response: &http.Response{}, + Message: "Github", + RetryAfter: &t2, + }, + }, + "errors have different values - Response is nil": { + wantSame: false, + err: err, + otherError: &AbuseRateLimitError{ + Message: "Github", + RetryAfter: &t1, + }, + }, + "errors have different values - StatusCode": { + wantSame: false, + err: err, + otherError: &AbuseRateLimitError{ + Response: &http.Response{StatusCode: 200}, + Message: "Github", + RetryAfter: &t1, + }, + }, + "errors have different types": { + wantSame: false, + err: err, + otherError: errors.New("Github"), + }, + } + + for name, tc := range testcases { + t.Run(name, func(t *testing.T) { + if tc.wantSame != tc.err.Is(tc.otherError) { + t.Errorf("Error = %#v, want %#v", tc.err, tc.otherError) + } + }) + } +} + +func TestAcceptedError_Is(t *testing.T) { + err := &AcceptedError{Raw: []byte("Github")} + testcases := map[string]struct { + wantSame bool + otherError error + }{ + "errors are same": { + wantSame: true, + otherError: &AcceptedError{Raw: []byte("Github")}, + }, + "errors have different values": { + wantSame: false, + otherError: &AcceptedError{Raw: []byte("Gitlab")}, + }, + "errors have different types": { + wantSame: false, + otherError: errors.New("Github"), + }, + } + + for name, tc := range testcases { + t.Run(name, func(t *testing.T) { + if tc.wantSame != err.Is(tc.otherError) { + t.Errorf("Error = %#v, want %#v", err, tc.otherError) + } + }) + } +} + // ensure that we properly handle API errors that do not contain a response body func TestCheckResponse_noBody(t *testing.T) { res := &http.Response{ From f4057c72daad7d62ebe429133e2b2aee01702fdb Mon Sep 17 00:00:00 2001 From: Munia Balayil Date: Wed, 14 Apr 2021 09:37:31 +0530 Subject: [PATCH 3/3] Add more test cases to TestErrorResponse_Is --- github/github_test.go | 74 ++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 70 insertions(+), 4 deletions(-) diff --git a/github/github_test.go b/github/github_test.go index 7f9d01051ac..858b2043da8 100644 --- a/github/github_test.go +++ b/github/github_test.go @@ -1188,7 +1188,7 @@ func TestCompareHttpResponse(t *testing.T) { } func TestErrorResponse_Is(t *testing.T) { - err := ErrorResponse{ + err := &ErrorResponse{ Response: &http.Response{}, Message: "m", Errors: []Error{{Resource: "r", Field: "f", Code: "c"}}, @@ -1271,8 +1271,25 @@ func TestErrorResponse_Is(t *testing.T) { "errors have different values - Errors": { wantSame: false, otherError: &ErrorResponse{ - Errors: []Error{{Resource: "r1", Field: "f1", Code: "c1"}}, - Message: "m", + Response: &http.Response{}, + Errors: []Error{{Resource: "r1", Field: "f1", Code: "c1"}}, + Message: "m", + Block: &struct { + Reason string `json:"reason,omitempty"` + CreatedAt *Timestamp `json:"created_at,omitempty"` + }{ + Reason: "r", + CreatedAt: &Timestamp{time.Date(2016, time.March, 17, 15, 39, 46, 0, time.UTC)}, + }, + DocumentationURL: "https://github.com", + }, + }, + "errors have different values - Errors have different length": { + wantSame: false, + otherError: &ErrorResponse{ + Response: &http.Response{}, + Errors: []Error{}, + Message: "m", Block: &struct { Reason string `json:"reason,omitempty"` CreatedAt *Timestamp `json:"created_at,omitempty"` @@ -1283,14 +1300,63 @@ func TestErrorResponse_Is(t *testing.T) { DocumentationURL: "https://github.com", }, }, - "errors have different values - Block": { + "errors have different values - Block - one is nil, other is not": { wantSame: false, otherError: &ErrorResponse{ + Response: &http.Response{}, Errors: []Error{{Resource: "r", Field: "f", Code: "c"}}, Message: "m", DocumentationURL: "https://github.com", }, }, + "errors have different values - Block - different Reason": { + wantSame: false, + otherError: &ErrorResponse{ + Response: &http.Response{}, + Errors: []Error{{Resource: "r", Field: "f", Code: "c"}}, + Message: "m", + Block: &struct { + Reason string `json:"reason,omitempty"` + CreatedAt *Timestamp `json:"created_at,omitempty"` + }{ + Reason: "r1", + CreatedAt: &Timestamp{time.Date(2016, time.March, 17, 15, 39, 46, 0, time.UTC)}, + }, + DocumentationURL: "https://github.com", + }, + }, + "errors have different values - Block - different CreatedAt #1": { + wantSame: false, + otherError: &ErrorResponse{ + Response: &http.Response{}, + Errors: []Error{{Resource: "r", Field: "f", Code: "c"}}, + Message: "m", + Block: &struct { + Reason string `json:"reason,omitempty"` + CreatedAt *Timestamp `json:"created_at,omitempty"` + }{ + Reason: "r", + CreatedAt: nil, + }, + DocumentationURL: "https://github.com", + }, + }, + "errors have different values - Block - different CreatedAt #2": { + wantSame: false, + otherError: &ErrorResponse{ + Response: &http.Response{}, + Errors: []Error{{Resource: "r", Field: "f", Code: "c"}}, + Message: "m", + Block: &struct { + Reason string `json:"reason,omitempty"` + CreatedAt *Timestamp `json:"created_at,omitempty"` + }{ + Reason: "r", + CreatedAt: &Timestamp{time.Date(2017, time.March, 17, 15, 39, 46, 0, time.UTC)}, + }, + DocumentationURL: "https://github.com", + }, + }, "errors have different types": { wantSame: false, otherError: errors.New("Github"),