From f0e121763364793086f5c24fc537a0f649acab0c Mon Sep 17 00:00:00 2001 From: Piotr Pawluk Date: Mon, 20 Nov 2023 14:29:31 +0100 Subject: [PATCH 1/4] fix: update changed documentation URL --- github_ratelimit/detect.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/github_ratelimit/detect.go b/github_ratelimit/detect.go index 19a253a..5fc321a 100644 --- a/github_ratelimit/detect.go +++ b/github_ratelimit/detect.go @@ -15,13 +15,13 @@ type SecondaryRateLimitBody struct { const ( SecondaryRateLimitMessage = `You have exceeded a secondary rate limit` - SecondaryRateLimitDocumentationPath = `/rest/overview/resources-in-the-rest-api#secondary-rate-limits` + SecondaryRateLimitDocumentationPath = `/rest/overview/rate-limits-for-the-rest-api#about-secondary-rate-limits` ) // IsSecondaryRateLimit checks whether the response is a legitimate secondary rate limit. // It checks the prefix of the message and the suffix of the documentation URL in the response body in case // the message or documentation URL is modified in the future. -// https://docs.github.com/en/rest/overview/resources-in-the-rest-api#secondary-rate-limits +// https://docs.github.com/en/rest/overview/rate-limits-for-the-rest-api#about-secondary-rate-limits func (s SecondaryRateLimitBody) IsSecondaryRateLimit() bool { return strings.HasPrefix(s.Message, SecondaryRateLimitMessage) && strings.HasSuffix(s.DocumentURL, SecondaryRateLimitDocumentationPath) } From 438c8522595952ce8823df10de65ebde01242cf0 Mon Sep 17 00:00:00 2001 From: Piotr Pawluk Date: Mon, 20 Nov 2023 14:35:07 +0100 Subject: [PATCH 2/4] fix: require only one part of response to detect rate limit To safeguard for the modification of the message or documentation URL, only one part of response (prefix OR suffix) should be used to detect a rate limit - instead of AND. --- github_ratelimit/detect.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/github_ratelimit/detect.go b/github_ratelimit/detect.go index 5fc321a..e9fa1e7 100644 --- a/github_ratelimit/detect.go +++ b/github_ratelimit/detect.go @@ -23,7 +23,7 @@ const ( // the message or documentation URL is modified in the future. // https://docs.github.com/en/rest/overview/rate-limits-for-the-rest-api#about-secondary-rate-limits func (s SecondaryRateLimitBody) IsSecondaryRateLimit() bool { - return strings.HasPrefix(s.Message, SecondaryRateLimitMessage) && strings.HasSuffix(s.DocumentURL, SecondaryRateLimitDocumentationPath) + return strings.HasPrefix(s.Message, SecondaryRateLimitMessage) || strings.HasSuffix(s.DocumentURL, SecondaryRateLimitDocumentationPath) } // isSecondaryRateLimit checks whether the response is a legitimate secondary rate limit. From 01b70bdcdf9a423ad158b44fcb613e096a2bba52 Mon Sep 17 00:00:00 2001 From: Piotr Pawluk Date: Mon, 20 Nov 2023 17:39:47 +0100 Subject: [PATCH 3/4] test: add test covering new documentation URL --- .../ratelimit_injecter.go | 34 ++++---- .../github_ratelimit_test/ratelimit_test.go | 86 ++++++++----------- 2 files changed, 52 insertions(+), 68 deletions(-) diff --git a/github_ratelimit/github_ratelimit_test/ratelimit_injecter.go b/github_ratelimit/github_ratelimit_test/ratelimit_injecter.go index ec1c0df..8abf444 100644 --- a/github_ratelimit/github_ratelimit_test/ratelimit_injecter.go +++ b/github_ratelimit/github_ratelimit_test/ratelimit_injecter.go @@ -18,18 +18,22 @@ const ( ) const ( - SecondaryRateLimitMessage = `You have exceeded a secondary rate limit. Please wait a few minutes before you try again.` - SecondaryRateLimitDocumentationURL = `https://docs.github.com/rest/overview/resources-in-the-rest-api#secondary-rate-limits` - SecondaryRateLimitAlternateDocumentationURL = `https://docs.github.com/free-pro-team@latest/rest/overview/resources-in-the-rest-api#secondary-rate-limits` + SecondaryRateLimitMessage = `You have exceeded a secondary rate limit. Please wait a few minutes before you try again.` ) +var SecondaryRateLimitDocumentationURLs = []string{ + `https://docs.github.com/rest/overview/resources-in-the-rest-api#secondary-rate-limits`, + `https://docs.github.com/free-pro-team@latest/rest/overview/resources-in-the-rest-api#secondary-rate-limits`, + `https://docs.github.com/en/free-pro-team@latest/rest/overview/rate-limits-for-the-rest-api#about-secondary-rate-limits`, +} + type SecondaryRateLimitInjecterOptions struct { - Every time.Duration - Sleep time.Duration - InvalidBody bool - UseXRateLimit bool - UsePrimaryRateLimit bool - UseAlternateDocumentationURL bool + Every time.Duration + Sleep time.Duration + InvalidBody bool + UseXRateLimit bool + UsePrimaryRateLimit bool + DocumentationURL string } func NewRateLimitInjecter(base http.RoundTripper, options *SecondaryRateLimitInjecterOptions) (http.RoundTripper, error) { @@ -109,16 +113,14 @@ func (r *SecondaryRateLimitInjecter) NextSleepStart() time.Time { return r.blockUntil.Add(r.options.Every) } -func getSecondaryRateLimitBody(useAlternatateDocumentationURL bool) (io.ReadCloser, error) { - documentURL := SecondaryRateLimitDocumentationURL - - if useAlternatateDocumentationURL { - documentURL = SecondaryRateLimitAlternateDocumentationURL +func getSecondaryRateLimitBody(documentationURL string) (io.ReadCloser, error) { + if len(documentationURL) == 0 { + documentationURL = SecondaryRateLimitDocumentationURLs[0] } body := github_ratelimit.SecondaryRateLimitBody{ Message: SecondaryRateLimitMessage, - DocumentURL: documentURL, + DocumentURL: documentationURL, } bodyBytes, err := json.Marshal(body) if err != nil { @@ -132,7 +134,7 @@ func (t *SecondaryRateLimitInjecter) inject(resp *http.Response) (*http.Response if t.options.UsePrimaryRateLimit { return t.toPrimaryRateLimitResponse(resp), nil } else { - body, err := getSecondaryRateLimitBody(t.options.UseAlternateDocumentationURL) + body, err := getSecondaryRateLimitBody(t.options.DocumentationURL) if err != nil { return nil, err } diff --git a/github_ratelimit/github_ratelimit_test/ratelimit_test.go b/github_ratelimit/github_ratelimit_test/ratelimit_test.go index 291fbd4..0ab0b26 100644 --- a/github_ratelimit/github_ratelimit_test/ratelimit_test.go +++ b/github_ratelimit/github_ratelimit_test/ratelimit_test.go @@ -108,58 +108,40 @@ func TestSecondaryRateLimitBody(t *testing.T) { const every = 1 * time.Second const sleep = 1 * time.Second - slept := false - callback := func(*github_ratelimit.CallbackContext) { - slept = true - } - - // test documentation URL - i := setupInjecterWithOptions(t, SecondaryRateLimitInjecterOptions{ - Every: every, - Sleep: sleep, - UseAlternateDocumentationURL: false, - }) - c, err := github_ratelimit.NewRateLimitWaiterClient(i, github_ratelimit.WithLimitDetectedCallback(callback)) - if err != nil { - t.Fatal(err) - } - - // initialize injecter timing - _, _ = c.Get("/") - waitForNextSleep(i) - - // attempt during rate limit - _, err = c.Get("/") - if err != nil { - t.Fatal(err) - } - if !slept { - t.Fatal(slept) - } - - // test alternate documentation URL - slept = false - i = setupInjecterWithOptions(t, SecondaryRateLimitInjecterOptions{ - Every: every, - Sleep: sleep, - UseAlternateDocumentationURL: true, - }) - c, err = github_ratelimit.NewRateLimitWaiterClient(i, github_ratelimit.WithLimitDetectedCallback(callback)) - if err != nil { - t.Fatal(err) - } - - // initialize injecter timing - _, _ = c.Get("/") - waitForNextSleep(i) - - // attempt during rate limit - _, err = c.Get("/") - if err != nil { - t.Fatal(err) - } - if !slept { - t.Fatal(slept) + for i, docURL := range SecondaryRateLimitDocumentationURLs { + docURL := docURL + t.Run(fmt.Sprintf("docURL_%d", i), func(t *testing.T) { + t.Parallel() + + slept := false + callback := func(*github_ratelimit.CallbackContext) { + slept = true + } + + // test documentation URL + i := setupInjecterWithOptions(t, SecondaryRateLimitInjecterOptions{ + Every: every, + Sleep: sleep, + DocumentationURL: docURL, + }) + c, err := github_ratelimit.NewRateLimitWaiterClient(i, github_ratelimit.WithLimitDetectedCallback(callback)) + if err != nil { + t.Fatal(err) + } + + // initialize injecter timing + _, _ = c.Get("/") + waitForNextSleep(i) + + // attempt during rate limit + _, err = c.Get("/") + if err != nil { + t.Fatal(err) + } + if !slept { + t.Fatal(slept) + } + }) } } From bcdd4752c7ab6bad2d6008d9a49475d3e21e4bda Mon Sep 17 00:00:00 2001 From: Piotr Pawluk Date: Mon, 11 Dec 2023 11:07:54 +0100 Subject: [PATCH 4/4] fix: check only for the suffix of the documentation URL path --- github_ratelimit/detect.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/github_ratelimit/detect.go b/github_ratelimit/detect.go index e9fa1e7..8809d3a 100644 --- a/github_ratelimit/detect.go +++ b/github_ratelimit/detect.go @@ -14,8 +14,8 @@ type SecondaryRateLimitBody struct { } const ( - SecondaryRateLimitMessage = `You have exceeded a secondary rate limit` - SecondaryRateLimitDocumentationPath = `/rest/overview/rate-limits-for-the-rest-api#about-secondary-rate-limits` + SecondaryRateLimitMessage = `You have exceeded a secondary rate limit` + SecondaryRateLimitDocumentationPathSuffix = `secondary-rate-limits` ) // IsSecondaryRateLimit checks whether the response is a legitimate secondary rate limit. @@ -23,7 +23,7 @@ const ( // the message or documentation URL is modified in the future. // https://docs.github.com/en/rest/overview/rate-limits-for-the-rest-api#about-secondary-rate-limits func (s SecondaryRateLimitBody) IsSecondaryRateLimit() bool { - return strings.HasPrefix(s.Message, SecondaryRateLimitMessage) || strings.HasSuffix(s.DocumentURL, SecondaryRateLimitDocumentationPath) + return strings.HasPrefix(s.Message, SecondaryRateLimitMessage) || strings.HasSuffix(s.DocumentURL, SecondaryRateLimitDocumentationPathSuffix) } // isSecondaryRateLimit checks whether the response is a legitimate secondary rate limit.