From 134b19b8811eb5ed12a4dc47ea4b61620723b2df Mon Sep 17 00:00:00 2001 From: Gal Ofri Date: Sat, 20 Jan 2024 08:27:35 +0200 Subject: [PATCH 1/2] add go-github compatability test --- .github/workflows/test.yaml | 2 +- README.md | 42 ++++--- github_ratelimit/callback.go | 6 +- github_ratelimit/config.go | 4 +- github_ratelimit/github_ratelimit_test/go.mod | 10 ++ github_ratelimit/github_ratelimit_test/go.sum | 9 ++ .../github_ratelimit_test/ratelimit_test.go | 105 +++++++++++++++--- github_ratelimit/ratelimit.go | 4 +- 8 files changed, 135 insertions(+), 47 deletions(-) create mode 100644 github_ratelimit/github_ratelimit_test/go.mod create mode 100644 github_ratelimit/github_ratelimit_test/go.sum diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index 50bc77f..edbab64 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -24,4 +24,4 @@ jobs: - name: Vet run: go vet -v ./... - name: Test - run: go test -v -count=1 -shuffle=on -timeout=30m -race ./... + run: cd github_ratelimit/github_ratelimit_test && go test -v -count=1 -shuffle=on -timeout=30m -race ./... diff --git a/README.md b/README.md index b28ce47..30136c6 100644 --- a/README.md +++ b/README.md @@ -6,51 +6,47 @@ Package `go-github-ratelimit` provides an http.RoundTripper implementation that The RoundTripper waits for the secondary rate limit to finish in a blocking mode and then issues/retries requests. `go-github-ratelimit` can be used with any HTTP client communicating with GitHub API. -It is meant to complement [go-github](https://github.com/google/go-github), but there is no association between this repository and the go-github repository or Google. +It is meant to complement [go-github](https://github.com/google/go-github), but there is no association between this repository and the go-github repository nor Google. ## Installation + ```go get github.com/gofri/go-github-ratelimit``` -## Usage Example (with go-github and [oauth2](golang.org/x/oauth2)) +## Usage Example (with [go-github](https://github.com/google/go-github)) + ```go -import "github.com/google/go-github/github" -import "golang.org/x/oauth2" +import "github.com/google/go-github/v58/github" import "github.com/gofri/go-github-ratelimit/github_ratelimit" func main() { - ctx := context.Background() - ts := oauth2.StaticTokenSource( - oauth2.Token{AccessToken: "Your Personal Access Token"}, - ) - tc := oauth2.NewClient(ctx, ts) - rateLimiter, err := github_ratelimit.NewRateLimitWaiterClient(tc.Transport) + rateLimiter, err := github_ratelimit.NewRateLimitWaiterClient(nil) if err != nil { panic(err) } - client := github.NewClient(rateLimiter) + client := github.NewClient(rateLimiter).WithAuthToken("your personal access token") // now use the client as you please } ``` ## Options -The RoundTripper accepts a set of options: -- User Context: provide a context.Context to pass to callbacks. -- Single Sleep Limit: limit the sleep time for a single rate limit. -- Total Sleep Limit: limit the accumulated sleep time for all rate limits. - -The RoundTripper accepts a set of optional callbacks: -- On Limit Detected: callback for when a rate limit that requires sleeping is detected. -- On Single Limit Exceeded: callback for when a rate limit that exceeds the single sleep limit is detected. -- On Total Limit Exceeded: callback for when a rate limit that exceeds the total sleep limit is detected. -note: to detect secondary rate limits and take a custom action without sleeping, use SingleSleepLimit=0 and OnSingleLimitExceeded(). +The RoundTripper accepts a set of options to configure its behavior and set callbacks. nil callbacks are treated as no-op. +The options are: + +- `WithLimitDetectedCallback(callback)`: the callback is triggered before a sleep. +- `WithSingleSleepLimit(duration, callback)`: limit the sleep duration for a single secondary rate limit & trigger a callback when the limit is exceeded. +- `WithTotalSleepLimit`: limit the accumulated sleep duration for all secondary rate limits & trigger a callback when the limit is exceeded. + +_Note_: to detect secondary rate limits without sleeping, use `WithSingleSleepLimit(0, your_callback_or_nil)`. ## Per-Request Options -Use WithOverrideConfig() to override the configuration for a specific request using a context. -Per-request overrides may be useful for special-cases of user requests, + +Use `WithOverrideConfig(opts...)` to override the configuration for a specific request (using the request context). +Per-request overrides may be useful for special cases of user requests, as well as fine-grained policy control (e.g., for a sophisticated pagination mechanism). ## License + This package is distributed under the MIT license found in the LICENSE file. Contribution and feedback is welcome. diff --git a/github_ratelimit/callback.go b/github_ratelimit/callback.go index 6767077..f069e45 100644 --- a/github_ratelimit/callback.go +++ b/github_ratelimit/callback.go @@ -16,18 +16,18 @@ type CallbackContext struct { } // OnLimitDetected is a callback to be called when a new rate limit is detected (before the sleep) -// The totalSleepTime includes the sleep time for the upcoming sleep +// The totalSleepTime includes the sleep duration for the upcoming sleep // Note: called while holding the lock. type OnLimitDetected func(*CallbackContext) // OnSingleLimitPassed is a callback to be called when a rate limit is exceeding the limit for a single sleep. -// The sleepUntil represents the end of sleep time if the limit was not exceeded. +// The sleepUntil represents the end of sleep duration if the limit was not exceeded. // The totalSleepTime does not include the sleep (that is not going to happen). // Note: called while holding the lock. type OnSingleLimitExceeded func(*CallbackContext) // OnTotalLimitExceeded is a callback to be called when a rate limit is exceeding the limit for the total sleep. -// The sleepUntil represents the end of sleep time if the limit was not exceeded. +// The sleepUntil represents the end of sleep duration if the limit was not exceeded. // The totalSleepTime does not include the sleep (that is not going to happen). // Note: called while holding the lock. type OnTotalLimitExceeded func(*CallbackContext) diff --git a/github_ratelimit/config.go b/github_ratelimit/config.go index d013077..14149a7 100644 --- a/github_ratelimit/config.go +++ b/github_ratelimit/config.go @@ -35,12 +35,12 @@ func (c *SecondaryRateLimitConfig) ApplyOptions(opts ...Option) { } } -// IsAboveSingleSleepLimit returns true if the single sleep time is above the limit. +// IsAboveSingleSleepLimit returns true if the single sleep duration is above the limit. func (c *SecondaryRateLimitConfig) IsAboveSingleSleepLimit(sleepTime time.Duration) bool { return c.singleSleepLimit != nil && sleepTime > *c.singleSleepLimit } -// IsAboveTotalSleepLimit returns true if the total sleep time is above the limit. +// IsAboveTotalSleepLimit returns true if the total sleep duration is above the limit. func (c *SecondaryRateLimitConfig) IsAboveTotalSleepLimit(sleepTime time.Duration, totalSleepTime time.Duration) bool { return c.totalSleepLimit != nil && totalSleepTime+sleepTime > *c.totalSleepLimit } diff --git a/github_ratelimit/github_ratelimit_test/go.mod b/github_ratelimit/github_ratelimit_test/go.mod new file mode 100644 index 0000000..4f4ea9f --- /dev/null +++ b/github_ratelimit/github_ratelimit_test/go.mod @@ -0,0 +1,10 @@ +module github.com/gofri/go-github-ratelimit/github-ratelimit-test + +go 1.19 + +require ( + github.com/gofri/go-github-ratelimit v1.1.0 + github.com/google/go-github/v58 v58.0.0 +) + +require github.com/google/go-querystring v1.1.0 // indirect diff --git a/github_ratelimit/github_ratelimit_test/go.sum b/github_ratelimit/github_ratelimit_test/go.sum new file mode 100644 index 0000000..32a7ea6 --- /dev/null +++ b/github_ratelimit/github_ratelimit_test/go.sum @@ -0,0 +1,9 @@ +github.com/gofri/go-github-ratelimit v1.1.0 h1:ijQ2bcv5pjZXNil5FiwglCg8wc9s8EgjTmNkqjw8nuk= +github.com/gofri/go-github-ratelimit v1.1.0/go.mod h1:OnCi5gV+hAG/LMR7llGhU7yHt44se9sYgKPnafoL7RY= +github.com/google/go-cmp v0.5.2/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= +github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= +github.com/google/go-github/v58 v58.0.0 h1:Una7GGERlF/37XfkPwpzYJe0Vp4dt2k1kCjlxwjIvzw= +github.com/google/go-github/v58 v58.0.0/go.mod h1:k4hxDKEfoWpSqFlc8LTpGd9fu2KrV1YAa6Hi6FmDNY4= +github.com/google/go-querystring v1.1.0 h1:AnCroh3fv4ZBgVIf1Iwtovgjaw/GiKJo8M8yD/fhyJ8= +github.com/google/go-querystring v1.1.0/go.mod h1:Kcdr2DB4koayq7X8pmAG4sNG59So17icRSOU623lUBU= +golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= diff --git a/github_ratelimit/github_ratelimit_test/ratelimit_test.go b/github_ratelimit/github_ratelimit_test/ratelimit_test.go index 5d9fae6..be8e95d 100644 --- a/github_ratelimit/github_ratelimit_test/ratelimit_test.go +++ b/github_ratelimit/github_ratelimit_test/ratelimit_test.go @@ -1,7 +1,9 @@ package github_ratelimit_test import ( + "bytes" "context" + "encoding/json" "fmt" "io" "log" @@ -14,6 +16,7 @@ import ( "time" "github.com/gofri/go-github-ratelimit/github_ratelimit" + "github.com/google/go-github/v58/github" ) type nopServer struct { @@ -31,16 +34,19 @@ func (n *nopServer) RoundTrip(r *http.Request) (*http.Response, error) { }, nil } -func setupSecondaryLimitInjecter(t *testing.T, every time.Duration, sleep time.Duration) http.RoundTripper { +func setupSecondaryLimitInjecter(t *testing.T, every time.Duration, sleep time.Duration, roundTrippger http.RoundTripper) http.RoundTripper { options := SecondaryRateLimitInjecterOptions{ Every: every, Sleep: sleep, } - return setupInjecterWithOptions(t, options) + return setupInjecterWithOptions(t, options, roundTrippger) } -func setupInjecterWithOptions(t *testing.T, options SecondaryRateLimitInjecterOptions) http.RoundTripper { - i, err := NewRateLimitInjecter(&nopServer{}, &options) +func setupInjecterWithOptions(t *testing.T, options SecondaryRateLimitInjecterOptions, roundTrippger http.RoundTripper) http.RoundTripper { + if roundTrippger == nil { + roundTrippger = &nopServer{} + } + i, err := NewRateLimitInjecter(roundTrippger, &options) if err != nil { t.Fatal(err) } @@ -66,7 +72,7 @@ func TestSecondaryRateLimit(t *testing.T) { time.Until(*context.SleepUntil).Seconds(), time.Now(), *context.SleepUntil) } - i := setupSecondaryLimitInjecter(t, every, sleep) + i := setupSecondaryLimitInjecter(t, every, sleep, nil) c, err := github_ratelimit.NewRateLimitWaiterClient(i, github_ratelimit.WithLimitDetectedCallback(print)) if err != nil { t.Fatal(err) @@ -126,7 +132,7 @@ func TestSecondaryRateLimitCombinations(t *testing.T) { Sleep: sleep, DocumentationURL: docURL, HttpStatusCode: statusCode, - }) + }, nil) c, err := github_ratelimit.NewRateLimitWaiterClient(i, github_ratelimit.WithLimitDetectedCallback(callback)) if err != nil { t.Fatal(err) @@ -164,7 +170,7 @@ func TestSingleSleepLimit(t *testing.T) { } // test sleep is short enough - i := setupSecondaryLimitInjecter(t, every, sleep) + i := setupSecondaryLimitInjecter(t, every, sleep, nil) c, err := github_ratelimit.NewRateLimitWaiterClient(i, github_ratelimit.WithLimitDetectedCallback(callback), github_ratelimit.WithSingleSleepLimit(5*time.Second, onLimitExceeded)) @@ -187,7 +193,7 @@ func TestSingleSleepLimit(t *testing.T) { // test sleep is too long slept = false - i = setupSecondaryLimitInjecter(t, every, sleep) + i = setupSecondaryLimitInjecter(t, every, sleep, nil) c, err = github_ratelimit.NewRateLimitWaiterClient(i, github_ratelimit.WithLimitDetectedCallback(callback), github_ratelimit.WithSingleSleepLimit(sleep/2, onLimitExceeded)) @@ -238,7 +244,7 @@ func TestTotalSleepLimit(t *testing.T) { } // test sleep is short enough - i := setupSecondaryLimitInjecter(t, every, sleep) + i := setupSecondaryLimitInjecter(t, every, sleep, nil) c, err := github_ratelimit.NewRateLimitWaiterClient(i, github_ratelimit.WithLimitDetectedCallback(callback), github_ratelimit.WithTotalSleepLimit(time.Second+time.Second/2, onLimitExceeded)) @@ -300,7 +306,7 @@ func TestXRateLimit(t *testing.T) { Every: every, Sleep: sleep, UseXRateLimit: true, - }) + }, nil) c, err := github_ratelimit.NewRateLimitWaiterClient(i, github_ratelimit.WithLimitDetectedCallback(callback)) if err != nil { t.Fatal(err) @@ -335,7 +341,7 @@ func TestPrimaryRateLimitIgnored(t *testing.T) { Every: every, Sleep: sleep, UsePrimaryRateLimit: true, - }) + }, nil) c, err := github_ratelimit.NewRateLimitWaiterClient(i, github_ratelimit.WithLimitDetectedCallback(callback)) if err != nil { t.Fatal(err) @@ -370,7 +376,7 @@ func TestHTTPForbiddenIgnored(t *testing.T) { Every: every, Sleep: sleep, InvalidBody: true, - }) + }, nil) c, err := github_ratelimit.NewRateLimitWaiterClient(i, github_ratelimit.WithLimitDetectedCallback(callback)) if err != nil { @@ -401,7 +407,7 @@ func TestCallbackContext(t *testing.T) { t.Parallel() const every = 1 * time.Second const sleep = 1 * time.Second - i := setupSecondaryLimitInjecter(t, every, sleep) + i := setupSecondaryLimitInjecter(t, every, sleep, nil) var roundTripper *github_ratelimit.SecondaryRateLimitWaiter = nil var requestNum atomic.Int64 @@ -419,7 +425,7 @@ func TestCallbackContext(t *testing.T) { t.Fatalf("unexpected sleep until time: %v < %v <= %v", min, got, max) } if got, want := *ctx.TotalSleepTime, sleep*time.Duration(requestsCycle); got != want { - t.Fatalf("unexpected total sleep time: %v != %v", got, want) + t.Fatalf("unexpected total sleep duration: %v != %v", got, want) } requestNum.Add(1) } @@ -478,7 +484,7 @@ func TestRequestConfigOverride(t *testing.T) { } // test sleep is short enough - i := setupSecondaryLimitInjecter(t, every, sleep) + i := setupSecondaryLimitInjecter(t, every, sleep, nil) c, err := github_ratelimit.NewRateLimitWaiterClient(i, github_ratelimit.WithLimitDetectedCallback(callback), github_ratelimit.WithSingleSleepLimit(5*time.Second, onLimitExceeded)) @@ -489,7 +495,7 @@ func TestRequestConfigOverride(t *testing.T) { // initialize injecter timing _, _ = c.Get("/") - // prepare an override - force sleep time to be 0, + // prepare an override - force sleep duration to be 0, // so that it will not sleep at all regardless of the original config. limit := github_ratelimit.WithSingleSleepLimit(0, onLimitExceeded) ctx := github_ratelimit.WithOverrideConfig(context.Background(), limit) @@ -538,3 +544,70 @@ func TestRequestConfigOverride(t *testing.T) { } } + +type orgLister struct { +} + +func (o *orgLister) GetOrgName() string { + return "org" +} + +func (o *orgLister) RoundTrip(r *http.Request) (*http.Response, error) { + org := github.Organization{ + Login: github.String(o.GetOrgName()), + } + + body, err := json.Marshal([]*github.Organization{&org}) + if err != nil { + return nil, err + } + + return &http.Response{ + Body: io.NopCloser(bytes.NewReader(body)), + Header: http.Header{}, + StatusCode: http.StatusOK, + }, nil +} + +// TestGoGithubClient is a test that uses the go-github client. +func TestGoGithubClientCompatability(t *testing.T) { + t.Parallel() + rand.Seed(time.Now().UnixNano()) + const every = 5 * time.Second + const sleep = 1 * time.Second + + print := func(context *github_ratelimit.CallbackContext) { + log.Printf("Secondary rate limit reached! Sleeping for %.2f seconds [%v --> %v]", + time.Until(*context.SleepUntil).Seconds(), time.Now(), *context.SleepUntil) + } + + orgLister := &orgLister{} + + i := setupSecondaryLimitInjecter(t, every, sleep, orgLister) + rateLimiter, err := github_ratelimit.NewRateLimitWaiterClient(i, github_ratelimit.WithLimitDetectedCallback(print)) + if err != nil { + t.Fatal(err) + } + + client := github.NewClient(rateLimiter) + orgs, resp, err := client.Organizations.List(context.Background(), "", nil) + if err != nil { + t.Fatalf("unexpected error response: %v", err) + } + + if resp.StatusCode != http.StatusOK { + t.Fatalf("unexpected status code: %v", resp.StatusCode) + } + + if len(orgs) != 1 { + t.Fatalf("unexpected number of orgs: %v", len(orgs)) + } + + if orgs[0].GetLogin() != orgLister.GetOrgName() { + t.Fatalf("unexpected org name: %v", orgs[0].GetLogin()) + } + + // TODO add tests for: + // - WithSingleSleepLimit(0, ...) => expect AbuseError + // - WithSingleSleepLimit(>0, ...) => expect sleeping +} diff --git a/github_ratelimit/ratelimit.go b/github_ratelimit/ratelimit.go index 8c8dd0a..de1a08c 100644 --- a/github_ratelimit/ratelimit.go +++ b/github_ratelimit/ratelimit.go @@ -174,7 +174,7 @@ func parseSecondaryLimitTime(resp *http.Response) *time.Time { return sleepUntil } - // XXX: per GitHub API docs, we should default to a 60 seconds sleep time in case the header is missing, + // XXX: per GitHub API docs, we should default to a 60 seconds sleep duration in case the header is missing, // with an exponential backoff mechanism. // we may want to implement this in the future (with configurable limits), // but let's avoid it while there are no known cases of missing headers. @@ -222,7 +222,7 @@ func httpHeaderIntValue(header http.Header, key string) (int64, bool) { return asInt, true } -// smoothSleepTime rounds up the sleep time to whole seconds. +// smoothSleepTime rounds up the sleep duration to whole seconds. // github only uses seconds to indicate the time to sleep, // but we sleep for less time because internal processing delay is taken into account. // round up the duration to get the original value. From c7f3c56284e65df466f54e471c3f3b604ff0aeb9 Mon Sep 17 00:00:00 2001 From: Gal Ofri Date: Sat, 20 Jan 2024 08:34:19 +0200 Subject: [PATCH 2/2] ci for testing --- .github/workflows/lint.yaml | 18 ++++++++++++++++-- .github/workflows/test.yaml | 6 +++++- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/.github/workflows/lint.yaml b/.github/workflows/lint.yaml index ffdf391..1a0bff3 100644 --- a/.github/workflows/lint.yaml +++ b/.github/workflows/lint.yaml @@ -20,5 +20,19 @@ jobs: - name: Lint uses: golangci/golangci-lint-action@08e2f20817b15149a52b5b3ebe7de50aff2ba8c5 # v3.4.0 with: - version: v1.50.1 - args: --timeout=3m \ No newline at end of file + version: v1.54 + args: --timeout=3m + + lint-test: + runs-on: ubuntu-latest + steps: + - uses: actions/setup-go@c4a742cab115ed795e34d4513e2cf7d472deb55f # v3 + with: + go-version: 1.19 + - uses: actions/checkout@93ea575cb5d8a053eaa0ac8fa3b40d7e05a33cc8 # v3 + - name: Lint + uses: golangci/golangci-lint-action@08e2f20817b15149a52b5b3ebe7de50aff2ba8c5 # v3.4.0 + with: + version: v1.54 + args: --timeout=3m + working-directory: github_ratelimit/github_ratelimit_test \ No newline at end of file diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index edbab64..12e6cf9 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -12,6 +12,8 @@ on: jobs: build_and_test: runs-on: ubuntu-latest + env: + TEST_DIR: github_ratelimit/github_ratelimit_test steps: - uses: actions/setup-go@c4a742cab115ed795e34d4513e2cf7d472deb55f # v3 with: @@ -23,5 +25,7 @@ jobs: run: go build -v ./... - name: Vet run: go vet -v ./... + - name: Vet-Test + run: cd "$TEST_DIR" && go vet -v ./... - name: Test - run: cd github_ratelimit/github_ratelimit_test && go test -v -count=1 -shuffle=on -timeout=30m -race ./... + run: cd "$TEST_DIR" && go test -v -count=1 -shuffle=on -timeout=30m -race ./...