feat: Add DisableRateLimitCheck option to client (#3607)

Author: stevehipwell

README.md

@@ -189,54 +189,71 @@ using the installation ID of the GitHub app and authenticate with the OAuth meth
 
 ### Rate Limiting ###
 
-GitHub imposes a rate limit on all API clients. Unauthenticated clients are
-limited to 60 requests per hour, while authenticated clients can make up to
-5,000 requests per hour. The Search API has a custom rate limit. Unauthenticated
-clients are limited to 10 requests per minute, while authenticated clients
-can make up to 30 requests per minute. To receive the higher rate limit when
-making calls that are not issued on behalf of a user,
-use `UnauthenticatedRateLimitedTransport`.
-
-The returned `Response.Rate` value contains the rate limit information
+GitHub imposes rate limits on all API clients. The [primary rate limit](https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api#about-primary-rate-limits)
+is the limit to the number of REST API requests that a client can make within a
+specific amount of time. This limit helps prevent abuse and denial-of-service
+attacks, and ensures that the API remains available for all users. Some
+endpoints, like the search endpoints, have more restrictive limits.
+Unauthenticated clients may request public data but have a low rate limit,
+while authenticated clients have rate limits based on the client
+identity.
+
+In addition to primary rate limits, GitHub enforces [secondary rate limits](https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api#about-secondary-rate-limits)
+in order to prevent abuse and keep the API available for all users.
+Secondary rate limits generally limit the number of concurrent requests that a
+client can make.
+
+The client returned `Response.Rate` value contains the rate limit information
 from the most recent API call. If a recent enough response isn't
-available, you can use `RateLimits` to fetch the most up-to-date rate
-limit data for the client.
+available, you can use the client `RateLimits` service to fetch the most
+up-to-date rate limit data for the client.
 
-To detect an API rate limit error, you can check if its type is `*github.RateLimitError`:
+To detect a primary API rate limit error, you can check if the error is a
+`RateLimitError`.
 
 ```go
 repos, _, err := client.Repositories.List(ctx, "", nil)
-if _, ok := err.(*github.RateLimitError); ok {
-	log.Println("hit rate limit")
+var rateErr *github.RateLimitError
+if errors.As(err, &rateError) {
+	log.Printf("hit primary rate limit, used %d of %d\n", rateErr.Rate.Used, rateErr.rate.Limit)
 }
 ```
 
-Learn more about GitHub rate limiting in
-["REST API endpoints for rate limits"](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`:
+To detect an API secondary rate limit error, you can check if the error is an
+`AbuseRateLimitError`.
 
 ```go
 repos, _, err := client.Repositories.List(ctx, "", nil)
-if _, ok := err.(*github.AbuseRateLimitError); ok {
-	log.Println("hit secondary rate limit")
+var rateErr *github.AbuseRateLimitError
+if errors.As(err, &rateErr) {
+	log.Printf("hit secondary rate limit, retry after %v\n", rateErr.RetryAfter)
 }
 ```
 
-Alternatively, you can block until the rate limit is reset by using the `context.WithValue` method:
+If you hit the primary rate limit, you can use the `SleepUntilPrimaryRateLimitResetWhenRateLimited`
+method to block until the rate limit is reset.
 
 ```go
 repos, _, err := client.Repositories.List(context.WithValue(ctx, github.SleepUntilPrimaryRateLimitResetWhenRateLimited, true), "", nil)
 ```
 
-You can use [gofri/go-github-ratelimit](https://github.com/gofri/go-github-ratelimit) to handle
-secondary rate limit sleep-and-retry for you, as well as primary rate limit abuse-prevention and callback triggering.
+If you need to make a request even if the rate limit has been hit you can use
+the `BypassRateLimitCheck` method to bypass the rate limit check and make the
+request anyway.
+
+```go
+repos, _, err := client.Repositories.List(context.WithValue(ctx, github.BypassRateLimitCheck, true), "", nil)
+```
+
+For more advanced use cases, you can use [gofri/go-github-ratelimit](https://github.com/gofri/go-github-ratelimit)
+which provides a middleware (`http.RoundTripper`) that handles both the primary
+rate limit and secondary rate limit for the GitHub API. In this case you can
+set the client `DisableRateLimitCheck` to `true` so the client doesn't track the rate limit usage.
 
-Learn more about GitHub secondary rate limiting in
-["About secondary rate limits"](https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api?apiVersion=2022-11-28#about-secondary-rate-limits).
+If the client is an [OAuth app](https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api#primary-rate-limit-for-oauth-apps)
+you can use the apps higher rate limit to request public data by using the
+`UnauthenticatedRateLimitedTransport` to make calls as the app instead of as
+the user.
 
 ### Accepted Status ###
 

github/github.go

@@ -171,6 +171,11 @@ type Client struct {
 	// User agent used when communicating with the GitHub API.
 	UserAgent string
 
+	// DisableRateLimitCheck stops the client checking for rate limits or tracking
+	// them. This is different to setting BypassRateLimitCheck in the context,
+	// as that still tracks the rate limits.
+	DisableRateLimitCheck bool
+
 	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.
@@ -850,21 +855,26 @@ func (c *Client) bareDo(ctx context.Context, caller *http.Client, req *http.Requ
 
 	req = withContext(ctx, req)
 
-	rateLimitCategory := GetRateLimitCategory(req.Method, req.URL.Path)
+	rateLimitCategory := CoreCategory
 
-	if bypass := ctx.Value(BypassRateLimitCheck); bypass == nil {
-		// If we've hit rate limit, don't make further requests before Reset time.
-		if err := c.checkRateLimitBeforeDo(req, rateLimitCategory); err != nil {
-			return &Response{
-				Response: err.Response,
-				Rate:     err.Rate,
-			}, err
-		}
-		// If we've hit a secondary rate limit, don't make further requests before Retry After.
-		if err := c.checkSecondaryRateLimitBeforeDo(req); err != nil {
-			return &Response{
-				Response: err.Response,
-			}, err
+	if !c.DisableRateLimitCheck {
+		rateLimitCategory = GetRateLimitCategory(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.
+			if err := c.checkRateLimitBeforeDo(req, rateLimitCategory); err != nil {
+				return &Response{
+					Response: err.Response,
+					Rate:     err.Rate,
+				}, err
+			}
+
+			// If we've hit a secondary rate limit, don't make further requests before Retry After.
+			if err := c.checkSecondaryRateLimitBeforeDo(req); err != nil {
+				return &Response{
+					Response: err.Response,
+				}, err
+			}
 		}
 	}
 
@@ -894,9 +904,10 @@ func (c *Client) bareDo(ctx context.Context, caller *http.Client, req *http.Requ
 		return response, err
 	}
 
-	// Don't update the rate limits if this was a cached response.
-	// X-From-Cache is set by https://github.com/bartventer/httpcache
-	if response.Header.Get("X-From-Cache") == "" {
+	// Don't update the rate limits if the client has rate limits disabled or if
+	// this was a cached response. The X-From-Cache is set by
+	// https://github.com/bartventer/httpcache if it's enabled.
+	if !c.DisableRateLimitCheck && response.Header.Get("X-From-Cache") == "" {
 		c.rateMu.Lock()
 		c.rateLimits[rateLimitCategory] = response.Rate
 		c.rateMu.Unlock()
@@ -1586,8 +1597,8 @@ that need to use a higher rate limit associated with your OAuth application.
 This will add the client id and secret as a base64-encoded string in the format
 ClientID:ClientSecret and apply it as an "Authorization": "Basic" header.
 
-See https://docs.github.com/rest/#unauthenticated-rate-limited-requests for
-more information.
+See https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api#primary-rate-limit-for-oauth-apps
+for more information.
 */
 type UnauthenticatedRateLimitedTransport struct {
 	// ClientID is the GitHub OAuth client ID of the current application, which

github/github_test.go

@@ -251,6 +251,9 @@ func testNewRequestAndDoFailureCategory(t *testing.T, methodName string, client
 	client.BaseURL.Path = "/api-v3/"
 	client.rateLimits[category].Reset.Time = time.Now().Add(10 * time.Minute)
 	resp, err = f()
+	if client.DisableRateLimitCheck {
+		return
+	}
 	if bypass := resp.Request.Context().Value(BypassRateLimitCheck); bypass != nil {
 		return
 	}
@@ -1912,6 +1915,79 @@ func TestDo_rateLimit_abuseRateLimitError_maxDuration(t *testing.T) {
 	}
 }
 
+// Make network call if client has disabled the rate limit check.
+func TestDo_rateLimit_disableRateLimitCheck(t *testing.T) {
+	t.Parallel()
+	client, mux, _ := setup(t)
+	client.DisableRateLimitCheck = true
+
+	reset := time.Now().UTC().Add(60 * time.Second)
+	client.rateLimits[CoreCategory] = Rate{Limit: 5000, Remaining: 0, Reset: Timestamp{reset}}
+	requestCount := 0
+	mux.HandleFunc("/", func(w http.ResponseWriter, _ *http.Request) {
+		requestCount++
+		w.Header().Set(headerRateLimit, "5000")
+		w.Header().Set(headerRateRemaining, "5000")
+		w.Header().Set(headerRateUsed, "0")
+		w.Header().Set(headerRateReset, fmt.Sprint(reset.Add(time.Hour).Unix()))
+		w.Header().Set(headerRateResource, "core")
+		w.Header().Set("Content-Type", "application/json; charset=utf-8")
+		w.WriteHeader(http.StatusOK)
+		fmt.Fprintln(w, `{}`)
+	})
+	req, _ := client.NewRequest("GET", ".", nil)
+	ctx := context.Background()
+	resp, err := client.Do(ctx, req, nil)
+	if err != nil {
+		t.Errorf("Do returned unexpected error: %v", err)
+	}
+	if got, want := resp.StatusCode, http.StatusOK; got != want {
+		t.Errorf("Response status code = %v, want %v", got, want)
+	}
+	if got, want := requestCount, 1; got != want {
+		t.Errorf("Expected 1 request, got %d", got)
+	}
+	if got, want := client.rateLimits[CoreCategory].Remaining, 0; got != want {
+		t.Errorf("Expected 0 requests remaining, got %d", got)
+	}
+}
+
+// Make network call if client has bypassed the rate limit check.
+func TestDo_rateLimit_bypassRateLimitCheck(t *testing.T) {
+	t.Parallel()
+	client, mux, _ := setup(t)
+
+	reset := time.Now().UTC().Add(60 * time.Second)
+	client.rateLimits[CoreCategory] = Rate{Limit: 5000, Remaining: 0, Reset: Timestamp{reset}}
+	requestCount := 0
+	mux.HandleFunc("/", func(w http.ResponseWriter, _ *http.Request) {
+		requestCount++
+		w.Header().Set(headerRateLimit, "5000")
+		w.Header().Set(headerRateRemaining, "5000")
+		w.Header().Set(headerRateUsed, "0")
+		w.Header().Set(headerRateReset, fmt.Sprint(reset.Add(time.Hour).Unix()))
+		w.Header().Set(headerRateResource, "core")
+		w.Header().Set("Content-Type", "application/json; charset=utf-8")
+		w.WriteHeader(http.StatusOK)
+		fmt.Fprintln(w, `{}`)
+	})
+	req, _ := client.NewRequest("GET", ".", nil)
+	ctx := context.Background()
+	resp, err := client.Do(context.WithValue(ctx, BypassRateLimitCheck, true), req, nil)
+	if err != nil {
+		t.Errorf("Do returned unexpected error: %v", err)
+	}
+	if got, want := resp.StatusCode, http.StatusOK; got != want {
+		t.Errorf("Response status code = %v, want %v", got, want)
+	}
+	if got, want := requestCount, 1; got != want {
+		t.Errorf("Expected 1 request, got %d", got)
+	}
+	if got, want := client.rateLimits[CoreCategory].Remaining, 5000; got != want {
+		t.Errorf("Expected 5000 requests remaining, got %d", got)
+	}
+}
+
 func TestDo_noContent(t *testing.T) {
 	t.Parallel()
 	client, mux, _ := setup(t)