From 811ef6fcbd003796da69ac09db5674d5cbc498d7 Mon Sep 17 00:00:00 2001 From: Gal Ofri Date: Wed, 18 Jan 2023 22:33:07 +0200 Subject: [PATCH 1/9] Add issuing requests during secondary rate limits prevention and hook --- github/github.go | 48 +++++++++++++++++++++++++++++++++++++++++-- github/github_test.go | 21 +++++++++++++++++++ 2 files changed, 67 insertions(+), 2 deletions(-) diff --git a/github/github.go b/github/github.go index 629218165c7..583f85b8347 100644 --- a/github/github.go +++ b/github/github.go @@ -171,8 +171,9 @@ type Client struct { // User agent used when communicating with the GitHub API. UserAgent string - rateMu sync.Mutex - rateLimits [categories]Rate // Rate limits for the client as determined by the most recent API calls. + rateMu sync.Mutex + rateLimits [categories]Rate // Rate limits for the client as determined by the most recent API calls. + secondaryRateLimitReset time.Time // Secondary rate limit reset for the client as determined by the most recent API calls. common service // Reuse a single struct instead of allocating one for each service on the heap. @@ -712,6 +713,12 @@ func (c *Client) BareDo(ctx context.Context, req *http.Request) (*Response, erro Rate: err.Rate, }, err } + // If we've hit a secondary rate limit, don't make further requests before Retry After. + if err := c.checkSecondaryRateLimitBeforeDo(ctx, req); err != nil { + return &Response{ + Response: err.Response, + }, err + } } resp, err := c.client.Do(req) @@ -763,6 +770,14 @@ func (c *Client) BareDo(ctx context.Context, req *http.Request) (*Response, erro aerr.Raw = b err = aerr } + + // Update the secondary rate limit if we hit it. + rerr, ok := err.(*AbuseRateLimitError) + if ok && rerr.RetryAfter != nil { + c.rateMu.Lock() + c.secondaryRateLimitReset = time.Now().Add(*rerr.RetryAfter) + c.rateMu.Unlock() + } } return response, err } @@ -827,6 +842,35 @@ func (c *Client) checkRateLimitBeforeDo(req *http.Request, rateLimitCategory rat return nil } +// checkSecondaryRateLimitBeforeDo does not make any network calls, but uses existing knowledge from +// current client state in order to quickly check if *AbuseRateLimitError can be immediately returned +// from Client.Do, and if so, returns it so that Client.Do can skip making a network API call unnecessarily. +// Otherwise it returns nil, and Client.Do should proceed normally. +func (c *Client) checkSecondaryRateLimitBeforeDo(ctx context.Context, req *http.Request) *AbuseRateLimitError { + c.rateMu.Lock() + secondary := c.secondaryRateLimitReset + c.rateMu.Unlock() + if !secondary.IsZero() && time.Now().Before(secondary) { + // Create a fake response. + resp := &http.Response{ + Status: http.StatusText(http.StatusForbidden), + StatusCode: http.StatusForbidden, + Request: req, + Header: make(http.Header), + Body: io.NopCloser(strings.NewReader("")), + } + + retryAfter := time.Until(secondary) + return &AbuseRateLimitError{ + Response: resp, + Message: fmt.Sprintf("API secondary rate limit exceeded until %v, not making remote request.", secondary), + RetryAfter: &retryAfter, + } + } + + 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. diff --git a/github/github_test.go b/github/github_test.go index 50ecd5435bc..c1eb5a9d756 100644 --- a/github/github_test.go +++ b/github/github_test.go @@ -1407,6 +1407,27 @@ func TestDo_rateLimit_abuseRateLimitError_retryAfter(t *testing.T) { if got, want := *abuseRateLimitErr.RetryAfter, 123*time.Second; got != want { t.Errorf("abuseRateLimitErr RetryAfter = %v, want %v", got, want) } + + // expect prevention of a following request + _, err = client.Do(ctx, req, nil) + + if err == nil { + t.Error("Expected error to be returned.") + } + abuseRateLimitErr, ok = err.(*AbuseRateLimitError) + if !ok { + t.Fatalf("Expected a *AbuseRateLimitError error; got %#v.", err) + } + if abuseRateLimitErr.RetryAfter == nil { + t.Fatalf("abuseRateLimitErr RetryAfter is nil, expected not-nil") + } + // the saved duration might be a bit smaller than Retry-After because the duration is calculated from the expected end-of-cooldown time + if got, want := *abuseRateLimitErr.RetryAfter, 123*time.Second; want-got < 1 { + t.Errorf("abuseRateLimitErr RetryAfter = %v, want %v", got, want) + } + if got, wantSuffix := abuseRateLimitErr.Message, "not making remote request."; !strings.HasSuffix(got, wantSuffix) { + t.Errorf("Expected request to be prevented because of secondary rate limit, got: %v.", got) + } } func TestDo_noContent(t *testing.T) { From e0e8f4e7fd4ec27b08c69d88a80dbbd23ff50e73 Mon Sep 17 00:00:00 2001 From: Gal Ofri Date: Thu, 19 Jan 2023 16:00:29 +0200 Subject: [PATCH 2/9] Fix rate limit category detection --- github/code-scanning_test.go | 2 +- github/github.go | 29 ++++++++++-- github/github_test.go | 85 +++++++++++++++++++++++++++++++++++- 3 files changed, 110 insertions(+), 6 deletions(-) diff --git a/github/code-scanning_test.go b/github/code-scanning_test.go index 1ae05096d3f..7911fca6360 100644 --- a/github/code-scanning_test.go +++ b/github/code-scanning_test.go @@ -83,7 +83,7 @@ func TestCodeScanningService_UploadSarif(t *testing.T) { return err }) - testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) { + testNewRequestAndDoFailureCategory(t, methodName, client, codeScanningUploadCategory, func() (*Response, error) { _, resp, err := client.CodeScanning.UploadSarif(ctx, "o", "r", sarifAnalysis) return resp, err }) diff --git a/github/github.go b/github/github.go index 583f85b8347..4e3a1157f71 100644 --- a/github/github.go +++ b/github/github.go @@ -703,7 +703,7 @@ func (c *Client) BareDo(ctx context.Context, req *http.Request) (*Response, erro req = withContext(ctx, req) - rateLimitCategory := category(req.URL.Path) + rateLimitCategory := category(req.Method, req.URL.Path) if bypass := ctx.Value(bypassRateLimitCheck); bypass == nil { // If we've hit rate limit, don't make further requests before Reset time. @@ -1241,13 +1241,36 @@ const ( categories // An array of this length will be able to contain all rate limit categories. ) -// category returns the rate limit category of the endpoint, determined by Request.URL.Path. -func category(path string) rateLimitCategory { +// category returns the rate limit category of the endpoint, determined by HTTP method and Request.URL.Path. +func category(method string, path string) rateLimitCategory { switch { + // https://docs.github.com/en/rest/rate-limit#about-rate-limits default: + // NOTE: coreCategory is returned for actionsRunnerRegistrationCategory too, + // because no API found for this category. return coreCategory case strings.HasPrefix(path, "/search/"): return searchCategory + case path == "/graphql": + return graphqlCategory + case strings.HasPrefix(path, "/app-manifests/") && + strings.HasSuffix(path, "/conversions") && + method == http.MethodPost: + return integrationManifestCategory + + // https://docs.github.com/en/rest/migrations/source-imports#start-an-import + case strings.HasPrefix(path, "/repos/") && + strings.HasSuffix(path, "/import") && + method == http.MethodPut: + return sourceImportCategory + + // https://docs.github.com/en/rest/code-scanning#upload-an-analysis-as-sarif-data + case strings.HasSuffix(path, "/code-scanning/sarifs"): + return codeScanningUploadCategory + + // https://docs.github.com/en/enterprise-cloud@latest/rest/scim + case strings.HasPrefix(path, "/scim/"): + return scimCategory } } diff --git a/github/github_test.go b/github/github_test.go index c1eb5a9d756..4b9f31aa63e 100644 --- a/github/github_test.go +++ b/github/github_test.go @@ -185,6 +185,11 @@ func testBadOptions(t *testing.T, methodName string, f func() error) { // Method f should be a regular call that would normally succeed, but // should return an error when NewRequest or s.client.Do fails. func testNewRequestAndDoFailure(t *testing.T, methodName string, client *Client, f func() (*Response, error)) { + testNewRequestAndDoFailureCategory(t, methodName, client, coreCategory, f) +} + +// testNewRequestAndDoFailureCategory works Like testNewRequestAndDoFailure, but allows setting the category +func testNewRequestAndDoFailureCategory(t *testing.T, methodName string, client *Client, category rateLimitCategory, f func() (*Response, error)) { t.Helper() if methodName == "" { t.Error("testNewRequestAndDoFailure: must supply method methodName") @@ -200,7 +205,7 @@ func testNewRequestAndDoFailure(t *testing.T, methodName string, client *Client, } client.BaseURL.Path = "/api-v3/" - client.rateLimits[0].Reset.Time = time.Now().Add(10 * time.Minute) + client.rateLimits[category].Reset.Time = time.Now().Add(10 * time.Minute) resp, err = f() if bypass := resp.Request.Context().Value(bypassRateLimitCheck); bypass != nil { return @@ -1136,6 +1141,36 @@ func TestDo_rateLimit(t *testing.T) { } } +func TestDo_rateLimitCategory(t *testing.T) { + if c := category(http.MethodGet, "/"); c != coreCategory { + t.Errorf("expecting default category to be core, found %v", c) + } + if c := category(http.MethodGet, "/search/issues?q=rate"); c != searchCategory { + t.Errorf("expecting search category, found %v", c) + } + if c := category(http.MethodGet, "/graphql"); c != graphqlCategory { + t.Errorf("expecting graphQL category, found %v", c) + } + if c := category(http.MethodPost, "/app-manifests/code/conversions"); c != integrationManifestCategory { + t.Errorf("expecting integration manifest category, found %v", c) + } + if c := category(http.MethodGet, "/app-manifests/code/conversions"); c == integrationManifestCategory { + t.Errorf("expecting GET to not match integration manifest category, but it is") + } + if c := category(http.MethodPut, "/repos/google/go-github/import"); c != sourceImportCategory { + t.Errorf("expecting source import category, found %v", c) + } + if c := category(http.MethodGet, "/repos/google/go-github/import"); c == sourceImportCategory { + t.Errorf("expecting GET to not match source import category, but it is") + } + if c := category(http.MethodPost, "/repos/google/go-github/code-scanning/sarifs"); c != codeScanningUploadCategory { + t.Errorf("expecting code scanning upload category, found %v", c) + } + if c := category(http.MethodGet, "/scim/v2/organizations/ORG/Users"); c != scimCategory { + t.Errorf("expecting scim category, found %v", c) + } +} + // ensure rate limit is still parsed, even for error responses func TestDo_rateLimit_errorResponse(t *testing.T) { client, mux, _, teardown := setup() @@ -2136,9 +2171,15 @@ func TestRateLimits_overQuota(t *testing.T) { Reset: Timestamp{time.Now().Add(time.Hour).Local()}, } mux.HandleFunc("/rate_limit", func(w http.ResponseWriter, r *http.Request) { + // note: actionsRunnerRegistrationCategory is skipped because it is not implemented yet fmt.Fprint(w, `{"resources":{ "core": {"limit":2,"remaining":1,"reset":1372700873}, - "search": {"limit":3,"remaining":2,"reset":1372700874} + "search": {"limit":3,"remaining":2,"reset":1372700874}, + "graphql": {"limit":4,"remaining":3,"reset":1372700875}, + "integration_manifest": {"limit":5,"remaining":4,"reset":1372700876}, + "source_import": {"limit":6,"remaining":5,"reset":1372700877}, + "code_scanning_upload": {"limit":7,"remaining":6,"reset":1372700878}, + "scim": {"limit":9,"remaining":8,"reset":1372700880} }}`) }) @@ -2159,6 +2200,31 @@ func TestRateLimits_overQuota(t *testing.T) { Remaining: 2, Reset: Timestamp{time.Date(2013, time.July, 1, 17, 47, 54, 0, time.UTC).Local()}, }, + GraphQL: &Rate{ + Limit: 4, + Remaining: 3, + Reset: Timestamp{time.Date(2013, time.July, 1, 17, 47, 55, 0, time.UTC).Local()}, + }, + IntegrationManifest: &Rate{ + Limit: 5, + Remaining: 4, + Reset: Timestamp{time.Date(2013, time.July, 1, 17, 47, 56, 0, time.UTC).Local()}, + }, + SourceImport: &Rate{ + Limit: 6, + Remaining: 5, + Reset: Timestamp{time.Date(2013, time.July, 1, 17, 47, 57, 0, time.UTC).Local()}, + }, + CodeScanningUpload: &Rate{ + Limit: 7, + Remaining: 6, + Reset: Timestamp{time.Date(2013, time.July, 1, 17, 47, 58, 0, time.UTC).Local()}, + }, + SCIM: &Rate{ + Limit: 9, + Remaining: 8, + Reset: Timestamp{time.Date(2013, time.July, 1, 17, 48, 00, 0, time.UTC).Local()}, + }, } if !cmp.Equal(rate, want) { t.Errorf("RateLimits returned %+v, want %+v", rate, want) @@ -2170,6 +2236,21 @@ func TestRateLimits_overQuota(t *testing.T) { if got, want := client.rateLimits[searchCategory], *want.Search; got != want { t.Errorf("client.rateLimits[searchCategory] is %+v, want %+v", got, want) } + if got, want := client.rateLimits[graphqlCategory], *want.GraphQL; got != want { + t.Errorf("client.rateLimits[graphqlCategory] is %+v, want %+v", got, want) + } + if got, want := client.rateLimits[integrationManifestCategory], *want.IntegrationManifest; got != want { + t.Errorf("client.rateLimits[integrationManifestCategory] is %+v, want %+v", got, want) + } + if got, want := client.rateLimits[sourceImportCategory], *want.SourceImport; got != want { + t.Errorf("client.rateLimits[sourceImportCategory] is %+v, want %+v", got, want) + } + if got, want := client.rateLimits[codeScanningUploadCategory], *want.CodeScanningUpload; got != want { + t.Errorf("client.rateLimits[codeScanningUploadCategory] is %+v, want %+v", got, want) + } + if got, want := client.rateLimits[scimCategory], *want.SCIM; got != want { + t.Errorf("client.rateLimits[scimCategory] is %+v, want %+v", got, want) + } } func TestSetCredentialsAsHeaders(t *testing.T) { From 2b1619c42cedd90b46cdd2cea9514e31f51b4359 Mon Sep 17 00:00:00 2001 From: Gal Ofri Date: Sat, 28 Jan 2023 12:35:02 +0200 Subject: [PATCH 3/9] Add secondary rate limit example and guidance --- README.md | 18 ++++++++++++++++++ example/ratelimit/main.go | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+) create mode 100644 example/ratelimit/main.go diff --git a/README.md b/README.md index 0e76752c82b..461ae4d02da 100644 --- a/README.md +++ b/README.md @@ -186,6 +186,24 @@ if _, ok := err.(*github.RateLimitError); ok { Learn more about GitHub rate limiting at https://docs.github.com/en/rest/rate-limit . +In addition to these rate limits, GitHub imposes a secondary rate limit on all API clients. +This rate limit prevents clients from making too many concurrent requests. + +To detect an API secondary rate limit error, you can check if its type is `*github.AbuseRateLimitError`: + +```go +repos, _, err := client.Repositories.List(ctx, "", nil) +if _, ok := err.(*github.AbuseRateLimitError); ok { + log.Println("hit secondary rate limit") +} +``` + +You can use [go-github-ratelimit](https://github.com/gofri/go-github-ratelimit) to handle +secondary rate limit sleep-and-retry for you. + +Learn more about GitHub secondary rate limiting at +https://docs.github.com/en/rest/overview/resources-in-the-rest-api#secondary-rate-limits . + ### Accepted Status ### Some endpoints may return a 202 Accepted status code, meaning that the diff --git a/example/ratelimit/main.go b/example/ratelimit/main.go new file mode 100644 index 00000000000..b6da7b74cc1 --- /dev/null +++ b/example/ratelimit/main.go @@ -0,0 +1,39 @@ +// Copyright 2017 The go-github AUTHORS. All rights reserved. +// +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// The ratelimit command demonstrates using the github_ratelimit.SecondaryRateLimitWaiter. +// By using the waiter, the client automatically sleeps and retry requests +// when it hits secondary rate limits. +package main + +import ( + "context" + "fmt" + + "github.com/gofri/go-github-ratelimit/github_ratelimit" + "github.com/google/go-github/v49/github" +) + +func main() { + rateLimiter, err := github_ratelimit.NewRateLimitWaiterClient(nil) + if err != nil { + fmt.Printf("Error: %v\n", err) + return + } + + client := github.NewClient(rateLimiter) + + // arbitrary usage of the client + username := "some-username" + organizations, _, err := client.Organizations.List(context.Background(), username, nil) + if err != nil { + fmt.Printf("Error: %v\n", err) + return + } + + for i, organization := range organizations { + fmt.Printf("%v. %v\n", i+1, organization.GetLogin()) + } +} From 4e531d6be1bbfe8ae86af10c30ae52d1d7fb389a Mon Sep 17 00:00:00 2001 From: "Gal Ofri @ Legit Security" Date: Sat, 28 Jan 2023 16:04:34 +0200 Subject: [PATCH 4/9] Update example/ratelimit/main.go Co-authored-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com> --- example/ratelimit/main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/example/ratelimit/main.go b/example/ratelimit/main.go index b6da7b74cc1..2cafa884687 100644 --- a/example/ratelimit/main.go +++ b/example/ratelimit/main.go @@ -1,4 +1,4 @@ -// Copyright 2017 The go-github AUTHORS. All rights reserved. +// Copyright 2023 The go-github AUTHORS. All rights reserved. // // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. From 2164c942d05e03d291f261410f03a342ef723b8a Mon Sep 17 00:00:00 2001 From: Gal Ofri Date: Sat, 28 Jan 2023 16:05:42 +0200 Subject: [PATCH 5/9] pr fix --- github/github.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/github/github.go b/github/github.go index 4e3a1157f71..6b37f082cf6 100644 --- a/github/github.go +++ b/github/github.go @@ -1242,7 +1242,7 @@ const ( ) // category returns the rate limit category of the endpoint, determined by HTTP method and Request.URL.Path. -func category(method string, path string) rateLimitCategory { +func category(method, path string) rateLimitCategory { switch { // https://docs.github.com/en/rest/rate-limit#about-rate-limits default: From f947fc7cd2c97f7766ec37f78e92eb564088eae7 Mon Sep 17 00:00:00 2001 From: Gal Ofri Date: Sat, 28 Jan 2023 16:28:00 +0200 Subject: [PATCH 6/9] fix go mod --- example/go.mod | 1 + example/go.sum | 2 ++ example/ratelimit/main.go | 7 +++++-- 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/example/go.mod b/example/go.mod index 85db0c81f3b..e268c7ad626 100644 --- a/example/go.mod +++ b/example/go.mod @@ -4,6 +4,7 @@ go 1.17 require ( github.com/bradleyfalzon/ghinstallation/v2 v2.0.4 + github.com/gofri/go-github-ratelimit v1.0.1 github.com/google/go-github/v50 v50.0.0 golang.org/x/crypto v0.0.0-20210817164053-32db794688a5 golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be diff --git a/example/go.sum b/example/go.sum index 663dff29860..1043059ffa2 100644 --- a/example/go.sum +++ b/example/go.sum @@ -1,5 +1,7 @@ github.com/bradleyfalzon/ghinstallation/v2 v2.0.4 h1:tXKVfhE7FcSkhkv0UwkLvPDeZ4kz6OXd0PKPlFqf81M= github.com/bradleyfalzon/ghinstallation/v2 v2.0.4/go.mod h1:B40qPqJxWE0jDZgOR1JmaMy+4AY1eBP+IByOvqyAKp0= +github.com/gofri/go-github-ratelimit v1.0.1 h1:sgefSzxhnvwZ+wR9uZ4l9TnjgLuNiwipJVzJL4YLj9A= +github.com/gofri/go-github-ratelimit v1.0.1/go.mod h1:OnCi5gV+hAG/LMR7llGhU7yHt44se9sYgKPnafoL7RY= github.com/golang-jwt/jwt/v4 v4.0.0 h1:RAqyYixv1p7uEnocuy8P1nru5wprCh/MH2BIlW5z5/o= github.com/golang-jwt/jwt/v4 v4.0.0/go.mod h1:/xlHOz8bRuivTWchD4jCa+NbatV+wEUSzwAxVc6locg= github.com/golang/protobuf v1.3.1/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U= diff --git a/example/ratelimit/main.go b/example/ratelimit/main.go index 2cafa884687..228099faa9f 100644 --- a/example/ratelimit/main.go +++ b/example/ratelimit/main.go @@ -13,10 +13,14 @@ import ( "fmt" "github.com/gofri/go-github-ratelimit/github_ratelimit" - "github.com/google/go-github/v49/github" + "github.com/google/go-github/v50/github" ) func main() { + var username string + fmt.Print("Enter GitHub username: ") + fmt.Scanf("%s", &username) + rateLimiter, err := github_ratelimit.NewRateLimitWaiterClient(nil) if err != nil { fmt.Printf("Error: %v\n", err) @@ -26,7 +30,6 @@ func main() { client := github.NewClient(rateLimiter) // arbitrary usage of the client - username := "some-username" organizations, _, err := client.Organizations.List(context.Background(), username, nil) if err != nil { fmt.Printf("Error: %v\n", err) From 34795340d3bcdf015974d5dd5150d7510addb9a9 Mon Sep 17 00:00:00 2001 From: Gal Ofri Date: Sat, 28 Jan 2023 17:04:07 +0200 Subject: [PATCH 7/9] bugfix test --- github/github_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/github/github_test.go b/github/github_test.go index 4b9f31aa63e..3eb75bb4f06 100644 --- a/github/github_test.go +++ b/github/github_test.go @@ -1457,7 +1457,7 @@ func TestDo_rateLimit_abuseRateLimitError_retryAfter(t *testing.T) { t.Fatalf("abuseRateLimitErr RetryAfter is nil, expected not-nil") } // the saved duration might be a bit smaller than Retry-After because the duration is calculated from the expected end-of-cooldown time - if got, want := *abuseRateLimitErr.RetryAfter, 123*time.Second; want-got < 1 { + if got, want := *abuseRateLimitErr.RetryAfter, 123*time.Second; want-got > 1*time.Second { t.Errorf("abuseRateLimitErr RetryAfter = %v, want %v", got, want) } if got, wantSuffix := abuseRateLimitErr.Message, "not making remote request."; !strings.HasSuffix(got, wantSuffix) { From 151b0468b7a8768cd75f43745a6586b0fbd6af7c Mon Sep 17 00:00:00 2001 From: Gal Ofri Date: Sun, 29 Jan 2023 12:07:57 +0200 Subject: [PATCH 8/9] tests improvement (pr fix) --- github/github_test.go | 215 ++++++++++++++++++++++++++++-------------- 1 file changed, 144 insertions(+), 71 deletions(-) diff --git a/github/github_test.go b/github/github_test.go index 3eb75bb4f06..57fc3828304 100644 --- a/github/github_test.go +++ b/github/github_test.go @@ -1142,32 +1142,63 @@ func TestDo_rateLimit(t *testing.T) { } func TestDo_rateLimitCategory(t *testing.T) { - if c := category(http.MethodGet, "/"); c != coreCategory { - t.Errorf("expecting default category to be core, found %v", c) - } - if c := category(http.MethodGet, "/search/issues?q=rate"); c != searchCategory { - t.Errorf("expecting search category, found %v", c) - } - if c := category(http.MethodGet, "/graphql"); c != graphqlCategory { - t.Errorf("expecting graphQL category, found %v", c) - } - if c := category(http.MethodPost, "/app-manifests/code/conversions"); c != integrationManifestCategory { - t.Errorf("expecting integration manifest category, found %v", c) - } - if c := category(http.MethodGet, "/app-manifests/code/conversions"); c == integrationManifestCategory { - t.Errorf("expecting GET to not match integration manifest category, but it is") - } - if c := category(http.MethodPut, "/repos/google/go-github/import"); c != sourceImportCategory { - t.Errorf("expecting source import category, found %v", c) - } - if c := category(http.MethodGet, "/repos/google/go-github/import"); c == sourceImportCategory { - t.Errorf("expecting GET to not match source import category, but it is") - } - if c := category(http.MethodPost, "/repos/google/go-github/code-scanning/sarifs"); c != codeScanningUploadCategory { - t.Errorf("expecting code scanning upload category, found %v", c) + tests := []struct { + method string + url string + category rateLimitCategory + }{ + { + http.MethodGet, + "/", + coreCategory, + }, + { + http.MethodGet, + "/search/issues?q=rate", + searchCategory, + }, + { + http.MethodGet, + "/graphql", + graphqlCategory, + }, + { + http.MethodPost, + "/app-manifests/code/conversions", + integrationManifestCategory, + }, + { + http.MethodGet, + "/app-manifests/code/conversions", + coreCategory, // only POST requests are in the integration manifest category + }, + { + http.MethodPut, + "/repos/google/go-github/import", + sourceImportCategory, + }, + { + http.MethodGet, + "/repos/google/go-github/import", + coreCategory, // only PUT requests are in the source import category + }, + { + http.MethodPost, + "/repos/google/go-github/code-scanning/sarifs", + codeScanningUploadCategory, + }, + { + http.MethodGet, + "/scim/v2/organizations/ORG/Users", + scimCategory, + }, + // missing a check for actionsRunnerRegistrationCategory: API not found } - if c := category(http.MethodGet, "/scim/v2/organizations/ORG/Users"); c != scimCategory { - t.Errorf("expecting scim category, found %v", c) + + for _, tt := range tests { + if got, want := category(tt.method, tt.url), tt.category; got != want { + t.Errorf("expecting category %v, found %v", got, want) + } } } @@ -1444,9 +1475,7 @@ func TestDo_rateLimit_abuseRateLimitError_retryAfter(t *testing.T) { } // expect prevention of a following request - _, err = client.Do(ctx, req, nil) - - if err == nil { + if _, err = client.Do(ctx, req, nil); err == nil { t.Error("Expected error to be returned.") } abuseRateLimitErr, ok = err.(*AbuseRateLimitError) @@ -2121,30 +2150,48 @@ func TestRateLimits(t *testing.T) { if !cmp.Equal(rate, want) { t.Errorf("RateLimits returned %+v, want %+v", rate, want) } - - if got, want := client.rateLimits[coreCategory], *want.Core; got != want { - t.Errorf("client.rateLimits[coreCategory] is %+v, want %+v", got, want) - } - if got, want := client.rateLimits[searchCategory], *want.Search; got != want { - t.Errorf("client.rateLimits[searchCategory] is %+v, want %+v", got, want) - } - if got, want := client.rateLimits[graphqlCategory], *want.GraphQL; got != want { - t.Errorf("client.rateLimits[graphqlCategory] is %+v, want %+v", got, want) - } - if got, want := client.rateLimits[integrationManifestCategory], *want.IntegrationManifest; got != want { - t.Errorf("client.rateLimits[integrationManifestCategory] is %+v, want %+v", got, want) - } - if got, want := client.rateLimits[sourceImportCategory], *want.SourceImport; got != want { - t.Errorf("client.rateLimits[sourceImportCategory] is %+v, want %+v", got, want) - } - if got, want := client.rateLimits[codeScanningUploadCategory], *want.CodeScanningUpload; got != want { - t.Errorf("client.rateLimits[codeScanningUploadCategory] is %+v, want %+v", got, want) - } - if got, want := client.rateLimits[actionsRunnerRegistrationCategory], *want.ActionsRunnerRegistration; got != want { - t.Errorf("client.rateLimits[actionsRunnerRegistrationCategory] is %+v, want %+v", got, want) + tests := []struct { + category rateLimitCategory + rate *Rate + }{ + { + coreCategory, + want.Core, + }, + { + searchCategory, + want.Search, + }, + { + graphqlCategory, + want.GraphQL, + }, + { + integrationManifestCategory, + want.IntegrationManifest, + }, + { + sourceImportCategory, + want.SourceImport, + }, + { + codeScanningUploadCategory, + want.CodeScanningUpload, + }, + { + actionsRunnerRegistrationCategory, + want.ActionsRunnerRegistration, + }, + { + scimCategory, + want.SCIM, + }, } - if got, want := client.rateLimits[scimCategory], *want.SCIM; got != want { - t.Errorf("client.rateLimits[scimCategory] is %+v, want %+v", got, want) + + for _, tt := range tests { + if got, want := client.rateLimits[tt.category], *tt.rate; got != want { + t.Errorf("client.rateLimits[%v] is %+v, want %+v", tt.category, got, want) + } } } @@ -2171,7 +2218,6 @@ func TestRateLimits_overQuota(t *testing.T) { Reset: Timestamp{time.Now().Add(time.Hour).Local()}, } mux.HandleFunc("/rate_limit", func(w http.ResponseWriter, r *http.Request) { - // note: actionsRunnerRegistrationCategory is skipped because it is not implemented yet fmt.Fprint(w, `{"resources":{ "core": {"limit":2,"remaining":1,"reset":1372700873}, "search": {"limit":3,"remaining":2,"reset":1372700874}, @@ -2179,6 +2225,7 @@ func TestRateLimits_overQuota(t *testing.T) { "integration_manifest": {"limit":5,"remaining":4,"reset":1372700876}, "source_import": {"limit":6,"remaining":5,"reset":1372700877}, "code_scanning_upload": {"limit":7,"remaining":6,"reset":1372700878}, + "actions_runner_registration": {"limit":8,"remaining":7,"reset":1372700879}, "scim": {"limit":9,"remaining":8,"reset":1372700880} }}`) }) @@ -2220,6 +2267,11 @@ func TestRateLimits_overQuota(t *testing.T) { Remaining: 6, Reset: Timestamp{time.Date(2013, time.July, 1, 17, 47, 58, 0, time.UTC).Local()}, }, + ActionsRunnerRegistration: &Rate{ + Limit: 8, + Remaining: 7, + Reset: Timestamp{time.Date(2013, time.July, 1, 17, 47, 59, 0, time.UTC).Local()}, + }, SCIM: &Rate{ Limit: 9, Remaining: 8, @@ -2230,26 +2282,47 @@ func TestRateLimits_overQuota(t *testing.T) { t.Errorf("RateLimits returned %+v, want %+v", rate, want) } - if got, want := client.rateLimits[coreCategory], *want.Core; got != want { - t.Errorf("client.rateLimits[coreCategory] is %+v, want %+v", got, want) - } - if got, want := client.rateLimits[searchCategory], *want.Search; got != want { - t.Errorf("client.rateLimits[searchCategory] is %+v, want %+v", got, want) - } - if got, want := client.rateLimits[graphqlCategory], *want.GraphQL; got != want { - t.Errorf("client.rateLimits[graphqlCategory] is %+v, want %+v", got, want) - } - if got, want := client.rateLimits[integrationManifestCategory], *want.IntegrationManifest; got != want { - t.Errorf("client.rateLimits[integrationManifestCategory] is %+v, want %+v", got, want) - } - if got, want := client.rateLimits[sourceImportCategory], *want.SourceImport; got != want { - t.Errorf("client.rateLimits[sourceImportCategory] is %+v, want %+v", got, want) - } - if got, want := client.rateLimits[codeScanningUploadCategory], *want.CodeScanningUpload; got != want { - t.Errorf("client.rateLimits[codeScanningUploadCategory] is %+v, want %+v", got, want) + tests := []struct { + category rateLimitCategory + rate *Rate + }{ + { + coreCategory, + want.Core, + }, + { + searchCategory, + want.Search, + }, + { + graphqlCategory, + want.GraphQL, + }, + { + integrationManifestCategory, + want.IntegrationManifest, + }, + { + sourceImportCategory, + want.SourceImport, + }, + { + codeScanningUploadCategory, + want.CodeScanningUpload, + }, + { + actionsRunnerRegistrationCategory, + want.ActionsRunnerRegistration, + }, + { + scimCategory, + want.SCIM, + }, } - if got, want := client.rateLimits[scimCategory], *want.SCIM; got != want { - t.Errorf("client.rateLimits[scimCategory] is %+v, want %+v", got, want) + for _, tt := range tests { + if got, want := client.rateLimits[tt.category], *tt.rate; got != want { + t.Errorf("client.rateLimits[%v] is %+v, want %+v", tt.category, got, want) + } } } From 9d0b570803c3ca90c6699331c7736ff2ded0c851 Mon Sep 17 00:00:00 2001 From: Gal Ofri Date: Mon, 30 Jan 2023 13:32:45 +0200 Subject: [PATCH 9/9] named fields --- github/github_test.go | 118 +++++++++++++++++++++--------------------- 1 file changed, 59 insertions(+), 59 deletions(-) diff --git a/github/github_test.go b/github/github_test.go index 57fc3828304..7342e2dfd79 100644 --- a/github/github_test.go +++ b/github/github_test.go @@ -1148,49 +1148,49 @@ func TestDo_rateLimitCategory(t *testing.T) { category rateLimitCategory }{ { - http.MethodGet, - "/", - coreCategory, + method: http.MethodGet, + url: "/", + category: coreCategory, }, { - http.MethodGet, - "/search/issues?q=rate", - searchCategory, + method: http.MethodGet, + url: "/search/issues?q=rate", + category: searchCategory, }, { - http.MethodGet, - "/graphql", - graphqlCategory, + method: http.MethodGet, + url: "/graphql", + category: graphqlCategory, }, { - http.MethodPost, - "/app-manifests/code/conversions", - integrationManifestCategory, + method: http.MethodPost, + url: "/app-manifests/code/conversions", + category: integrationManifestCategory, }, { - http.MethodGet, - "/app-manifests/code/conversions", - coreCategory, // only POST requests are in the integration manifest category + method: http.MethodGet, + url: "/app-manifests/code/conversions", + category: coreCategory, // only POST requests are in the integration manifest category }, { - http.MethodPut, - "/repos/google/go-github/import", - sourceImportCategory, + method: http.MethodPut, + url: "/repos/google/go-github/import", + category: sourceImportCategory, }, { - http.MethodGet, - "/repos/google/go-github/import", - coreCategory, // only PUT requests are in the source import category + method: http.MethodGet, + url: "/repos/google/go-github/import", + category: coreCategory, // only PUT requests are in the source import category }, { - http.MethodPost, - "/repos/google/go-github/code-scanning/sarifs", - codeScanningUploadCategory, + method: http.MethodPost, + url: "/repos/google/go-github/code-scanning/sarifs", + category: codeScanningUploadCategory, }, { - http.MethodGet, - "/scim/v2/organizations/ORG/Users", - scimCategory, + method: http.MethodGet, + url: "/scim/v2/organizations/ORG/Users", + category: scimCategory, }, // missing a check for actionsRunnerRegistrationCategory: API not found } @@ -2155,36 +2155,36 @@ func TestRateLimits(t *testing.T) { rate *Rate }{ { - coreCategory, - want.Core, + category: coreCategory, + rate: want.Core, }, { - searchCategory, - want.Search, + category: searchCategory, + rate: want.Search, }, { - graphqlCategory, - want.GraphQL, + category: graphqlCategory, + rate: want.GraphQL, }, { - integrationManifestCategory, - want.IntegrationManifest, + category: integrationManifestCategory, + rate: want.IntegrationManifest, }, { - sourceImportCategory, - want.SourceImport, + category: sourceImportCategory, + rate: want.SourceImport, }, { - codeScanningUploadCategory, - want.CodeScanningUpload, + category: codeScanningUploadCategory, + rate: want.CodeScanningUpload, }, { - actionsRunnerRegistrationCategory, - want.ActionsRunnerRegistration, + category: actionsRunnerRegistrationCategory, + rate: want.ActionsRunnerRegistration, }, { - scimCategory, - want.SCIM, + category: scimCategory, + rate: want.SCIM, }, } @@ -2287,36 +2287,36 @@ func TestRateLimits_overQuota(t *testing.T) { rate *Rate }{ { - coreCategory, - want.Core, + category: coreCategory, + rate: want.Core, }, { - searchCategory, - want.Search, + category: searchCategory, + rate: want.Search, }, { - graphqlCategory, - want.GraphQL, + category: graphqlCategory, + rate: want.GraphQL, }, { - integrationManifestCategory, - want.IntegrationManifest, + category: integrationManifestCategory, + rate: want.IntegrationManifest, }, { - sourceImportCategory, - want.SourceImport, + category: sourceImportCategory, + rate: want.SourceImport, }, { - codeScanningUploadCategory, - want.CodeScanningUpload, + category: codeScanningUploadCategory, + rate: want.CodeScanningUpload, }, { - actionsRunnerRegistrationCategory, - want.ActionsRunnerRegistration, + category: actionsRunnerRegistrationCategory, + rate: want.ActionsRunnerRegistration, }, { - scimCategory, - want.SCIM, + category: scimCategory, + rate: want.SCIM, }, } for _, tt := range tests {