add go-github compatability test

.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 ./...

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.

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)

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
 }

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

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=

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
+}

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.