Various clean up and fixes for GHRateLimit

hub4j · Nov 7, 2019 · a6f2360 · a6f2360
1 parent 1ecad70
commit a6f2360
Show file tree

Hide file tree

Showing 43 changed files with 60 additions and 279 deletions.
diff --git a/src/main/java/org/kohsuke/github/GHRateLimit.java b/src/main/java/org/kohsuke/github/GHRateLimit.java
@@ -73,8 +73,8 @@ public class GHRateLimit {
      * The calculated time at which the rate limit will reset.
      * Only calculated if {@link #getResetDate} is called.
      */
-    @CheckForNull
-    private Date resetDate = null;
+    @Nonnull
+    private Date resetDate;
 
     /**
      * Gets a placeholder instance that can be used when we fail to get one from the server.
@@ -96,24 +96,26 @@ public GHRateLimit(@JsonProperty("limit") int limit,
         this(limit, remaining, resetEpochSeconds, null);
     }
 
+    @SuppressFBWarnings(value = "URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD",
+        justification = "Deprecated")
     public GHRateLimit(int limit, int remaining, long resetEpochSeconds, String updatedAt) {
         this.limitCount = limit;
         this.remainingCount = remaining;
         this.resetEpochSeconds = resetEpochSeconds;
-        setUpdatedAt(updatedAt);
+        this.resetDate = recalculateResetDate(updatedAt);
 
         // Deprecated fields
         this.remaining = remaining;
         this.limit = limit;
         this.reset = new Date(resetEpochSeconds);
-
     }
 
     /**
      *
      * @param updatedAt a string date in RFC 1123
+     * @return reset date based on the passed date
      */
-    void setUpdatedAt(String updatedAt) {
+    Date recalculateResetDate(String updatedAt) {
         long updatedAtEpochSeconds = createdAtEpochSeconds;
         if (!StringUtils.isBlank(updatedAt)) {
             try {
@@ -126,10 +128,10 @@ void setUpdatedAt(String updatedAt) {
             }
         }
 
-        long calculatedSecondsUntilReset = resetEpochSeconds - updatedAtEpochSeconds;
-
         // This may seem odd but it results in an accurate or slightly pessimistic reset date
-        resetDate = new Date((createdAtEpochSeconds + calculatedSecondsUntilReset) * 1000);
+        // based on system time rather than on the system being in sync with the server
+        long calculatedSecondsUntilReset = resetEpochSeconds - updatedAtEpochSeconds;
+        return resetDate = new Date((createdAtEpochSeconds + calculatedSecondsUntilReset) * 1000);
     }
 
     /**

diff --git a/src/main/java/org/kohsuke/github/Requester.java b/src/main/java/org/kohsuke/github/Requester.java
@@ -44,7 +44,15 @@
 import java.net.SocketTimeoutException;
 import java.net.URL;
 import java.net.URLEncoder;
-import java.util.*;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.Iterator;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.NoSuchElementException;
 import java.util.function.Consumer;
 import java.util.logging.Logger;
 import java.util.regex.Matcher;
@@ -702,7 +710,7 @@ private <T> T setResponseHeaders(T readValue) {
             setResponseHeaders((GHObject) readValue);
         } else if (readValue instanceof JsonRateLimit) {
             // if we're getting a GHRateLimit it needs the server date
-            ((JsonRateLimit)readValue).rate.setUpdatedAt(uc.getHeaderField("Date"));
+            ((JsonRateLimit)readValue).rate.recalculateResetDate(uc.getHeaderField("Date"));
         }
         return readValue;
     }

diff --git a/src/test/java/org/kohsuke/github/GitHubConnectionTest.java b/src/test/java/org/kohsuke/github/GitHubConnectionTest.java
@@ -6,8 +6,6 @@
 import java.lang.reflect.Field;
 import java.util.*;
 
-import static org.hamcrest.CoreMatchers.*;
-
 /**
  * Unit test for {@link GitHub}.
  */
@@ -97,233 +95,6 @@ public void testGithubBuilderWithAppInstallationToken() throws Exception{
         assertEquals("",github.login);
     }
 
-    @Test
-    public void testGitHubRateLimit() throws Exception {
-        assertThat(mockGitHub.getRequestCount(), equalTo(0));
-        GHRateLimit rateLimit = null;
-        GitHub hub = null;
-        Date lastReset = new Date(System.currentTimeMillis() / 1000L);
-        int lastRemaining = 5000;
-
-        // Give this a moment
-        Thread.sleep(1000);
-
-        // -------------------------------------------------------------
-        // /user gets response with rate limit information
-        hub = getGitHubBuilder()
-            .withEndpoint(mockGitHub.apiServer().baseUrl()).build();
-        hub.getMyself();
-
-        assertThat(mockGitHub.getRequestCount(), equalTo(1));
-
-        // Since we already had rate limit info these don't request again
-        rateLimit = hub.lastRateLimit();
-        assertThat(rateLimit, notNullValue());
-        assertThat(rateLimit.limit, equalTo(5000));
-        lastRemaining = rateLimit.remaining;
-        // Because we're gettting this from old mocked info, it will be an older date
-        //assertThat(rateLimit.getResetDate().compareTo(lastReset), equalTo(-1));
-        lastReset = rateLimit.getResetDate();
-
-        GHRateLimit headerRateLimit = rateLimit;
-
-        // Give this a moment
-        Thread.sleep(1000);
-
-        // ratelimit() uses headerRateLimit if available
-        assertThat(hub.rateLimit(), equalTo(headerRateLimit));
-
-        assertThat(mockGitHub.getRequestCount(), equalTo(1));
-
-        // Give this a moment
-        Thread.sleep(1000);
-
-        // Always requests new info
-        rateLimit = hub.getRateLimit();
-        assertThat(mockGitHub.getRequestCount(), equalTo(2));
-
-        assertThat(rateLimit, notNullValue());
-        assertThat(rateLimit.limit, equalTo(5000));
-        // rate limit request is free
-        assertThat(rateLimit.remaining, equalTo(lastRemaining));
-        assertThat(rateLimit.getResetDate().compareTo(lastReset), equalTo(0));
-
-        // Give this a moment
-        Thread.sleep(1000);
-
-        // Always requests new info
-        rateLimit = hub.getRateLimit();
-        assertThat(mockGitHub.getRequestCount(), equalTo(3));
-
-        assertThat(rateLimit, notNullValue());
-        assertThat(rateLimit.limit, equalTo(5000));
-        // rate limit request is free
-        assertThat(rateLimit.remaining, equalTo(lastRemaining));
-        assertThat(rateLimit.getResetDate().compareTo(lastReset), equalTo(0));
-
-
-        hub.getOrganization(GITHUB_API_TEST_ORG);
-        assertThat(mockGitHub.getRequestCount(), equalTo(4));
-
-
-        assertThat(hub.lastRateLimit(), not(equalTo(headerRateLimit)));
-        rateLimit = hub.lastRateLimit();
-        assertThat(rateLimit, notNullValue());
-        assertThat(rateLimit.limit, equalTo(5000));
-        // Org costs limit to query
-        assertThat(rateLimit.remaining, equalTo(lastRemaining - 1));
-        assertThat(rateLimit.getResetDate().compareTo(lastReset), equalTo(0));
-        lastReset = rateLimit.getResetDate();
-        headerRateLimit = rateLimit;
-
-        // ratelimit() should prefer headerRateLimit when it is most recent
-        assertThat(hub.rateLimit(), equalTo(headerRateLimit));
-
-        assertThat(mockGitHub.getRequestCount(), equalTo(4));
-
-        // Always requests new info
-        rateLimit = hub.getRateLimit();
-        assertThat(mockGitHub.getRequestCount(), equalTo(5));
-
-        assertThat(rateLimit, notNullValue());
-        assertThat(rateLimit.limit, equalTo(5000));
-        // Org costs limit to query
-        assertThat(rateLimit.remaining, equalTo(lastRemaining - 1));
-        assertThat(rateLimit.getResetDate().compareTo(lastReset), equalTo(0));
-
-        // ratelimit() should prefer headerRateLimit when getRateLimit() fails
-        // BUG: When getRateLimit() succeeds, it should reset the ratelimit() to the new value
-//        assertThat(hub.rateLimit(), equalTo(rateLimit));
-//        assertThat(hub.rateLimit(), not(equalTo(headerRateLimit)));
-        assertThat(hub.rateLimit(), equalTo(headerRateLimit));
-
-        assertThat(mockGitHub.getRequestCount(), equalTo(5));
-    }
-
-    @Test
-    public void testGitHubEnterpriseDoesNotHaveRateLimit() throws Exception {
-        // Customized response that results in file not found the same as GitHub Enterprise
-        snapshotNotAllowed();
-        assertThat(mockGitHub.getRequestCount(), equalTo(0));
-        GHRateLimit rateLimit = null;
-        GitHub hub = null;
-
-
-        Date lastReset = new Date(System.currentTimeMillis() / 1000L);
-
-        // Give this a moment
-        Thread.sleep(1000);
-
-        // -------------------------------------------------------------
-        // Before any queries, rate limit starts as null but may be requested
-        hub = GitHub.connectToEnterprise(mockGitHub.apiServer().baseUrl(), "bogus", "bogus");
-        assertThat(mockGitHub.getRequestCount(), equalTo(0));
-
-        assertThat(hub.lastRateLimit(), nullValue());
-
-        rateLimit = hub.rateLimit();
-        assertThat(rateLimit, notNullValue());
-        assertThat(rateLimit.limit, equalTo(1000000));
-        assertThat(rateLimit.remaining, equalTo(1000000));
-        assertThat(rateLimit.getResetDate().compareTo(lastReset), equalTo(1));
-        lastReset = rateLimit.getResetDate();
-
-        assertThat(mockGitHub.getRequestCount(), equalTo(1));
-
-        // last is still null, because it actually means lastHeaderRateLimit
-        assertThat(hub.lastRateLimit(), nullValue());
-
-        assertThat(mockGitHub.getRequestCount(), equalTo(1));
-
-        // Give this a moment
-        Thread.sleep(1000);
-
-        // -------------------------------------------------------------
-        // First call to /user gets response without rate limit information
-        hub = GitHub.connectToEnterprise(mockGitHub.apiServer().baseUrl(), "bogus", "bogus");
-        hub.getMyself();
-        assertThat(mockGitHub.getRequestCount(), equalTo(2));
-
-        assertThat(hub.lastRateLimit(), nullValue());
-
-        rateLimit = hub.rateLimit();
-        assertThat(rateLimit, notNullValue());
-        assertThat(rateLimit.limit, equalTo(1000000));
-        assertThat(rateLimit.remaining, equalTo(1000000));
-        assertThat(rateLimit.getResetDate().compareTo(lastReset), equalTo(1));
-        lastReset = rateLimit.getResetDate();
-
-        assertThat(mockGitHub.getRequestCount(), equalTo(3));
-
-        // Give this a moment
-        Thread.sleep(1000);
-
-        // Always requests new info
-        rateLimit = hub.getRateLimit();
-        assertThat(mockGitHub.getRequestCount(), equalTo(4));
-
-        assertThat(rateLimit, notNullValue());
-        assertThat(rateLimit.limit, equalTo(1000000));
-        assertThat(rateLimit.remaining, equalTo(1000000));
-        assertThat(rateLimit.getResetDate().compareTo(lastReset), equalTo(1));
-
-        // Give this a moment
-        Thread.sleep(1000);
-
-
-        // last is still null, because it actually means lastHeaderRateLimit
-        assertThat(hub.lastRateLimit(), nullValue());
-
-        // ratelimit() tries not to make additional requests, uses queried rate limit since header not available
-        Thread.sleep(1000);
-        assertThat(hub.rateLimit(), equalTo(rateLimit));
-
-        // -------------------------------------------------------------
-        // Second call to /user gets response with rate limit information
-        hub = GitHub.connectToEnterprise(mockGitHub.apiServer().baseUrl(), "bogus", "bogus");
-        hub.getMyself();
-        assertThat(mockGitHub.getRequestCount(), equalTo(5));
-
-        // Since we already had rate limit info these don't request again
-        rateLimit = hub.lastRateLimit();
-        assertThat(rateLimit, notNullValue());
-        assertThat(rateLimit.limit, equalTo(5000));
-        assertThat(rateLimit.remaining, equalTo(4978));
-        // Because we're gettting this from old mocked info, it will be an older date
-        assertThat(rateLimit.getResetDate().compareTo(lastReset), equalTo(-1));
-        lastReset = rateLimit.getResetDate();
-
-        GHRateLimit headerRateLimit = rateLimit;
-
-        // Give this a moment
-        Thread.sleep(1000);
-
-        // ratelimit() uses headerRateLimit if available
-        assertThat(hub.rateLimit(), equalTo(headerRateLimit));
-
-        assertThat(mockGitHub.getRequestCount(), equalTo(5));
-
-        // Give this a moment
-        Thread.sleep(1000);
-
-        // Always requests new info
-        rateLimit = hub.getRateLimit();
-        assertThat(mockGitHub.getRequestCount(), equalTo(6));
-
-        assertThat(rateLimit, notNullValue());
-        assertThat(rateLimit.limit, equalTo(1000000));
-        assertThat(rateLimit.remaining, equalTo(1000000));
-        assertThat(rateLimit.getResetDate().compareTo(lastReset), equalTo(1));
-
-        // Give this a moment
-        Thread.sleep(1000);
-
-        // ratelimit() should prefer headerRateLimit when getRateLimit fails
-        assertThat(hub.rateLimit(), equalTo(headerRateLimit));
-
-        assertThat(mockGitHub.getRequestCount(), equalTo(6));
-    }
-
     @Test
     public void testGitHubIsApiUrlValid() throws IOException {
         GitHub hub = GitHub.connectAnonymously();

diff --git a/.../OkHttpConnector_Cache_MaxAgeDefault_Zero/mappings/orgs_github-api-test-org-3-ec2931.json b/.../OkHttpConnector_Cache_MaxAgeDefault_Zero/mappings/orgs_github-api-test-org-3-ec2931.json
@@ -15,7 +15,7 @@
       "Status": "200 OK",
       "X-RateLimit-Limit": "5000",
       "X-RateLimit-Remaining": "4969",
-      "X-RateLimit-Reset": "1569875630",
+      "X-RateLimit-Reset": "{{now offset='1 hours' format='unix'}}",
       "Cache-Control": "private, max-age=60, s-maxage=60",
       "Vary": [
         "Accept, Authorization, Cookie, X-GitHub-OTP",

diff --git a/...rTest/wiremock/OkHttpConnector_Cache_MaxAgeDefault_Zero/mappings/rate_limit-2-36588a.json b/...rTest/wiremock/OkHttpConnector_Cache_MaxAgeDefault_Zero/mappings/rate_limit-2-36588a.json
@@ -15,7 +15,7 @@
       "Status": "200 OK",
       "X-RateLimit-Limit": "5000",
       "X-RateLimit-Remaining": "4970",
-      "X-RateLimit-Reset": "1569875630",
+      "X-RateLimit-Reset": "{{now offset='1 hours' format='unix'}}",
       "Cache-Control": "no-cache",
       "X-OAuth-Scopes": "gist, notifications, read:org, read:public_key, read:repo_hook, repo",
       "X-Accepted-OAuth-Scopes": "",

diff --git a/..._Cache_MaxAgeDefault_Zero/mappings/repos_github-api-test-org_github-api-10-10-5432b2.json b/..._Cache_MaxAgeDefault_Zero/mappings/repos_github-api-test-org_github-api-10-10-5432b2.json
@@ -15,7 +15,7 @@
       "Status": "200 OK",
       "X-RateLimit-Limit": "5000",
       "X-RateLimit-Remaining": "4966",
-      "X-RateLimit-Reset": "1569875630",
+      "X-RateLimit-Reset": "{{now offset='1 hours' format='unix'}}",
       "Cache-Control": "private, max-age=60, s-maxage=60",
       "Vary": [
         "Accept, Authorization, Cookie, X-GitHub-OTP",

diff --git a/..._Cache_MaxAgeDefault_Zero/mappings/repos_github-api-test-org_github-api-11-11-2e2a6e.json b/..._Cache_MaxAgeDefault_Zero/mappings/repos_github-api-test-org_github-api-11-11-2e2a6e.json
@@ -18,7 +18,7 @@
       "Status": "304 Not Modified",
       "X-RateLimit-Limit": "5000",
       "X-RateLimit-Remaining": "4966",
-      "X-RateLimit-Reset": "1569875630",
+      "X-RateLimit-Reset": "{{now offset='1 hours' format='unix'}}",
       "Cache-Control": "private, max-age=60, s-maxage=60",
       "Vary": [
         "Accept, Authorization, Cookie, X-GitHub-OTP",

diff --git a/..._Cache_MaxAgeDefault_Zero/mappings/repos_github-api-test-org_github-api-12-12-a9ae79.json b/..._Cache_MaxAgeDefault_Zero/mappings/repos_github-api-test-org_github-api-12-12-a9ae79.json
@@ -18,7 +18,7 @@
       "Status": "304 Not Modified",
       "X-RateLimit-Limit": "5000",
       "X-RateLimit-Remaining": "4966",
-      "X-RateLimit-Reset": "1569875630",
+      "X-RateLimit-Reset": "{{now offset='1 hours' format='unix'}}",
       "Cache-Control": "private, max-age=60, s-maxage=60",
       "Vary": [
         "Accept, Authorization, Cookie, X-GitHub-OTP",

diff --git a/..._Cache_MaxAgeDefault_Zero/mappings/repos_github-api-test-org_github-api-13-13-b2221a.json b/..._Cache_MaxAgeDefault_Zero/mappings/repos_github-api-test-org_github-api-13-13-b2221a.json
@@ -18,7 +18,7 @@
       "Status": "304 Not Modified",
       "X-RateLimit-Limit": "5000",
       "X-RateLimit-Remaining": "4966",
-      "X-RateLimit-Reset": "1569875630",
+      "X-RateLimit-Reset": "{{now offset='1 hours' format='unix'}}",
       "Cache-Control": "private, max-age=60, s-maxage=60",
       "Vary": [
         "Accept, Authorization, Cookie, X-GitHub-OTP",

diff --git a/..._Cache_MaxAgeDefault_Zero/mappings/repos_github-api-test-org_github-api-14-14-7e396e.json b/..._Cache_MaxAgeDefault_Zero/mappings/repos_github-api-test-org_github-api-14-14-7e396e.json
@@ -20,7 +20,7 @@
       "Status": "200 OK",
       "X-RateLimit-Limit": "5000",
       "X-RateLimit-Remaining": "4963",
-      "X-RateLimit-Reset": "1569875630",
+      "X-RateLimit-Reset": "{{now offset='1 hours' format='unix'}}",
       "Cache-Control": "private, max-age=60, s-maxage=60",
       "Vary": [
         "Accept, Authorization, Cookie, X-GitHub-OTP",

diff --git a/..._Cache_MaxAgeDefault_Zero/mappings/repos_github-api-test-org_github-api-15-15-bc8d8b.json b/..._Cache_MaxAgeDefault_Zero/mappings/repos_github-api-test-org_github-api-15-15-bc8d8b.json
@@ -18,7 +18,7 @@
       "Status": "304 Not Modified",
       "X-RateLimit-Limit": "5000",
       "X-RateLimit-Remaining": "4963",
-      "X-RateLimit-Reset": "1569875630",
+      "X-RateLimit-Reset": "{{now offset='1 hours' format='unix'}}",
       "Cache-Control": "private, max-age=60, s-maxage=60",
       "Vary": [
         "Accept, Authorization, Cookie, X-GitHub-OTP",