Don't update the ratelimits if we got a response from a cache (#2273)

Jille · web-flow · commit 77b2d7586ddc · 2022-02-17T11:15:14.000-05:00
diff --git a/github/github.go b/github/github.go
@@ -655,9 +655,13 @@ func (c *Client) BareDo(ctx context.Context, req *http.Request) (*Response, erro
 
 	response := newResponse(resp)
 
-	c.rateMu.Lock()
-	c.rateLimits[rateLimitCategory] = response.Rate
-	c.rateMu.Unlock()
+	// Don't update the rate limits if this was a cached response.
+	// X-From-Cache is set by https://github.com/gregjones/httpcache
+	if response.Header.Get("X-From-Cache") == "" {
+		c.rateMu.Lock()
+		c.rateLimits[rateLimitCategory] = response.Rate
+		c.rateMu.Unlock()
+	}
 
 	err = CheckResponse(resp)
 	if err != nil {
diff --git a/github/github_test.go b/github/github_test.go
@@ -1116,6 +1116,49 @@ func TestDo_rateLimit_noNetworkCall(t *testing.T) {
 	}
 }
 
+// Ignore rate limit headers if the response was served from cache.
+func TestDo_rateLimit_ignoredFromCache(t *testing.T) {
+	client, mux, _, teardown := setup()
+	defer teardown()
+
+	reset := time.Now().UTC().Add(time.Minute).Round(time.Second) // Rate reset is a minute from now, with 1 second precision.
+
+	// By adding the X-From-Cache header we pretend this is served from a cache.
+	mux.HandleFunc("/first", func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("X-From-Cache", "1")
+		w.Header().Set(headerRateLimit, "60")
+		w.Header().Set(headerRateRemaining, "0")
+		w.Header().Set(headerRateReset, fmt.Sprint(reset.Unix()))
+		w.Header().Set("Content-Type", "application/json; charset=utf-8")
+		w.WriteHeader(http.StatusForbidden)
+		fmt.Fprintln(w, `{
+   "message": "API rate limit exceeded for xxx.xxx.xxx.xxx. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)",
+   "documentation_url": "https://docs.github.com/en/free-pro-team@latest/rest/overview/resources-in-the-rest-api#abuse-rate-limits"
+}`)
+	})
+
+	madeNetworkCall := false
+	mux.HandleFunc("/second", func(w http.ResponseWriter, r *http.Request) {
+		madeNetworkCall = true
+	})
+
+	// First request is made so afterwards we can check the returned rate limit headers were ignored.
+	req, _ := client.NewRequest("GET", "first", nil)
+	ctx := context.Background()
+	client.Do(ctx, req, nil)
+
+	// Second request should not by hindered by rate limits.
+	req, _ = client.NewRequest("GET", "second", nil)
+	_, err := client.Do(ctx, req, nil)
+
+	if err != nil {
+		t.Fatalf("Second request failed, even though the rate limits from the cache should've been ignored: %v", err)
+	}
+	if !madeNetworkCall {
+		t.Fatal("Network call was not made, even though the rate limits from the cache should've been ignored")
+	}
+}
+
 // Ensure *AbuseRateLimitError is returned when the response indicates that
 // the client has triggered an abuse detection mechanism.
 func TestDo_rateLimit_abuseRateLimitError(t *testing.T) {
