From 7a077bbd7313615d4f924f12b065913ee94e1d58 Mon Sep 17 00:00:00 2001 From: Munia Balayil Date: Thu, 15 Apr 2021 02:39:14 +0530 Subject: [PATCH] Update error handling as per Go 1.13 guidelines (#1854) Fixes: #1846. --- README.md | 2 +- github/github.go | 95 +++++++++- github/github_test.go | 412 +++++++++++++++++++++++++++++++++++++++++- 3 files changed, 501 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index be95d20e059..7d865d89ddd 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..efde8f8dbab 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) @@ -653,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. @@ -681,6 +697,50 @@ 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) || + !compareHttpResponse(r.Response, v.Response) { + return false + } + + // Compare Errors. + if len(r.Errors) != len(v.Errors) { + return false + } + for idx := range r.Errors { + if r.Errors[idx] != v.Errors[idx] { + return false + } + } + + // 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 +} + // 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 +762,18 @@ 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 + } + + return r.Rate == v.Rate && + r.Message == v.Message && + compareHttpResponse(r.Response, v.Response) +} + // 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 +789,15 @@ 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 { + 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 +816,18 @@ 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 + } + + 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 // 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..858b2043da8 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,11 +1147,411 @@ 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) } } +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{ + 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"` + }{ + 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 - 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"), + }, + } + + 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{ @@ -1168,7 +1568,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 +1591,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)