Updates based on review feedback

Created GHRateLimit.Record Add the four rate limit records to GHRateLimit Moved getLimt(), getRemaining(), and so on to point to core record for ease of use. Fixed update check for header to not replace existing with older when remaining count is lower. NOTE: Did not expose records other than core and did not resolve header update behavior to respect non-core records.
hub4j · Nov 11, 2019 · 03e9623 · 03e9623
1 parent 57ac6e9
commit 03e9623
Show file tree

Hide file tree

Showing 23 changed files with 443 additions and 474 deletions.
diff --git a/src/main/java/org/kohsuke/github/GHRateLimit.java b/src/main/java/org/kohsuke/github/GHRateLimit.java
diff --git a/src/main/java/org/kohsuke/github/GitHub.java b/src/main/java/org/kohsuke/github/GitHub.java
@@ -318,29 +318,59 @@ public void setConnector(HttpConnector connector) {
     public GHRateLimit getRateLimit() throws IOException {
         GHRateLimit rateLimit;
         try {
-            rateLimit = retrieve().to("/rate_limit", JsonRateLimit.class).rate;
+            rateLimit = retrieve().to("/rate_limit", JsonRateLimit.class).resources;
         } catch (FileNotFoundException e) {
-            // GitHub Enterprise doesn't have the rate limit, so in that case
-            // return some big number that's not too big.
-            // see issue #78
-            rateLimit = GHRateLimit.getPlaceholder();
+            // GitHub Enterprise doesn't have the rate limit
+            // return a default rate limit that
+            rateLimit = GHRateLimit.Unknown();
         }
 
         return this.rateLimit = rateLimit;
-
     }
 
-    /*package*/ void updateRateLimit(@Nonnull GHRateLimit observed) {
+    /**
+     * Update the Rate Limit with the latest info from response header.
+     * Due to multi-threading requests might complete out of order, we want to pick the one with the most recent info from the server.
+     *
+     * @param observed {@link GHRateLimit.Record} constructed from the response header information
+     */
+    void updateCoreRateLimit(@Nonnull GHRateLimit.Record observed) {
         synchronized (headerRateLimitLock) {
-            if (headerRateLimit == null
-                    || headerRateLimit.getRemaining() > observed.getRemaining()
-                    || headerRateLimit.getResetEpochSeconds() < observed.getResetEpochSeconds()) {
-                headerRateLimit = observed;
+            if (headerRateLimit == null || shouldReplace(observed, headerRateLimit.getCore())) {
+                headerRateLimit = GHRateLimit.fromHeaderRecord(observed);
                 LOGGER.log(FINE, "Rate limit now: {0}", headerRateLimit);
             }
         }
     }
 
+    /**
+     * Update the Rate Limit with the latest info from response header.
+     * Due to multi-threading requests might complete out of order, we want to pick the one with the most recent info from the server.
+     * Header date is only accurate to the second, so we look at the information in the record itself.
+     *
+     * {@link GHRateLimit.UnknownLimitRecord}s are always replaced by regular {@link GHRateLimit.Record}s.
+     * Regular {@link GHRateLimit.Record}s are never replaced by {@link GHRateLimit.UnknownLimitRecord}s.
+     * Candidates with resetEpochSeconds later than current record are more recent.
+     * Candidates with the same reset and a lower remaining count are more recent.
+     * Candidates with an earlier reset are older.
+     *
+     * @param candidate {@link GHRateLimit.Record} constructed from the response header information
+     * @param current the current {@link GHRateLimit.Record} record
+     */
+    static boolean shouldReplace(@Nonnull GHRateLimit.Record candidate, @Nonnull GHRateLimit.Record current) {
+        if (candidate instanceof GHRateLimit.UnknownLimitRecord && !(current instanceof GHRateLimit.UnknownLimitRecord)) {
+            // Unknown candidate never replaces a regular record
+            return false;
+        } else if (current instanceof GHRateLimit.UnknownLimitRecord && !(candidate instanceof GHRateLimit.UnknownLimitRecord)) {
+            // Any real record should replace an unknown Record.
+            return true;
+        } else {
+            // records of the same type compare to each other as normal.
+            return current.getResetEpochSeconds() < candidate.getResetEpochSeconds()
+                || (current.getResetEpochSeconds() == candidate.getResetEpochSeconds() && current.getRemaining() > candidate.getRemaining());
+        }
+    }
+
     /**
      * Returns the most recently observed rate limit data or {@code null} if either there is no rate limit
      * (for example GitHub Enterprise) or if no requests have been made.

diff --git a/src/main/java/org/kohsuke/github/JsonRateLimit.java b/src/main/java/org/kohsuke/github/JsonRateLimit.java
@@ -4,5 +4,7 @@
  * @author Kohsuke Kawaguchi
  */
 class JsonRateLimit {
-    GHRateLimit rate;
+
+    GHRateLimit resources;
+
 }
diff --git a/src/main/java/org/kohsuke/github/Requester.java b/src/main/java/org/kohsuke/github/Requester.java
@@ -399,9 +399,9 @@ private void noteRateLimit(String tailApiUrl) {
             return;
         }
 
-        GHRateLimit observed = new GHRateLimit(limit, remaining, reset, uc.getHeaderField("Date"));
+        GHRateLimit.Record observed = new GHRateLimit.Record(limit, remaining, reset, uc.getHeaderField("Date"));
 
-        root.updateRateLimit(observed);
+        root.updateCoreRateLimit(observed);
     }
 
     public String getResponseHeader(String header) {
@@ -710,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.recalculateResetDate(uc.getHeaderField("Date"));
+            ((JsonRateLimit)readValue).resources.getCore().recalculateResetDate(uc.getHeaderField("Date"));
         }
         return readValue;
     }

diff --git a/src/test/java/org/kohsuke/github/GHRateLimitTest.java b/src/test/java/org/kohsuke/github/GHRateLimitTest.java
@@ -166,8 +166,12 @@ public void testGitHubRateLimit() throws Exception {
         // These are separate instances, but should be equal
         assertThat(gitHub.rateLimit(), not(sameInstance(rateLimit)));
 
-        // Verify different intstances can be compared
-        assertThat(gitHub.rateLimit(), equalTo(rateLimit));
+        // Verify different record instances can be compared
+        assertThat(gitHub.rateLimit().getCore(), equalTo(rateLimit.getCore()));
+
+        // Verify different instances can be compared
+        // TODO: This is not work currently because the header rate limit has unknowns for records other than core.
+        // assertThat(gitHub.rateLimit().getCore(), equalTo(rateLimit.getCore()));
 
         assertThat(gitHub.rateLimit(), not(sameInstance(headerRateLimit)));
         assertThat(gitHub.rateLimit(), sameInstance(gitHub.lastRateLimit()));
@@ -195,11 +199,11 @@ public void testGitHubEnterpriseDoesNotHaveRateLimit() throws Exception {
         assertThat(gitHub.lastRateLimit(), CoreMatchers.nullValue());
 
         rateLimit = gitHub.rateLimit();
-        assertThat(rateLimit, notNullValue());
-        assertThat(rateLimit.limit, equalTo(1000000));
-        assertThat(rateLimit.getLimit(), equalTo(1000000));
-        assertThat(rateLimit.remaining, equalTo(1000000));
-        assertThat(rateLimit.getRemaining(), equalTo(1000000));
+        assertThat(rateLimit.getCore(), instanceOf(GHRateLimit.UnknownLimitRecord.class));
+        assertThat(rateLimit.limit, equalTo(GHRateLimit.UnknownLimitRecord.unknownLimit));
+        assertThat(rateLimit.getLimit(), equalTo(GHRateLimit.UnknownLimitRecord.unknownLimit));
+        assertThat(rateLimit.remaining, equalTo(GHRateLimit.UnknownLimitRecord.unknownRemaining));
+        assertThat(rateLimit.getRemaining(), equalTo(GHRateLimit.UnknownLimitRecord.unknownRemaining));
         assertThat(rateLimit.getResetDate().compareTo(lastReset), equalTo(1));
         lastReset = rateLimit.getResetDate();
 
@@ -222,11 +226,11 @@ public void testGitHubEnterpriseDoesNotHaveRateLimit() throws Exception {
         assertThat(gitHub.lastRateLimit(), CoreMatchers.nullValue());
 
         rateLimit = gitHub.rateLimit();
-        assertThat(rateLimit, notNullValue());
-        assertThat(rateLimit.limit, equalTo(1000000));
-        assertThat(rateLimit.getLimit(), equalTo(1000000));
-        assertThat(rateLimit.remaining, equalTo(1000000));
-        assertThat(rateLimit.getRemaining(), equalTo(1000000));
+        assertThat(rateLimit.getCore(), instanceOf(GHRateLimit.UnknownLimitRecord.class));
+        assertThat(rateLimit.limit, equalTo(GHRateLimit.UnknownLimitRecord.unknownLimit));
+        assertThat(rateLimit.getLimit(), equalTo(GHRateLimit.UnknownLimitRecord.unknownLimit));
+        assertThat(rateLimit.remaining, equalTo(GHRateLimit.UnknownLimitRecord.unknownRemaining));
+        assertThat(rateLimit.getRemaining(), equalTo(GHRateLimit.UnknownLimitRecord.unknownRemaining));
         assertThat(rateLimit.getResetDate().compareTo(lastReset), equalTo(1));
         lastReset = rateLimit.getResetDate();
 
@@ -239,11 +243,11 @@ public void testGitHubEnterpriseDoesNotHaveRateLimit() throws Exception {
         rateLimit = gitHub.getRateLimit();
         assertThat(mockGitHub.getRequestCount(), equalTo(4));
 
-        assertThat(rateLimit, notNullValue());
-        assertThat(rateLimit.limit, equalTo(1000000));
-        assertThat(rateLimit.getLimit(), equalTo(1000000));
-        assertThat(rateLimit.remaining, equalTo(1000000));
-        assertThat(rateLimit.getRemaining(), equalTo(1000000));
+        assertThat(rateLimit.getCore(), instanceOf(GHRateLimit.UnknownLimitRecord.class));
+        assertThat(rateLimit.limit, equalTo(GHRateLimit.UnknownLimitRecord.unknownLimit));
+        assertThat(rateLimit.getLimit(), equalTo(GHRateLimit.UnknownLimitRecord.unknownLimit));
+        assertThat(rateLimit.remaining, equalTo(GHRateLimit.UnknownLimitRecord.unknownRemaining));
+        assertThat(rateLimit.getRemaining(), equalTo(GHRateLimit.UnknownLimitRecord.unknownRemaining));
         assertThat(rateLimit.getResetDate().compareTo(lastReset), equalTo(1));
 
         // Give this a moment
@@ -290,11 +294,11 @@ public void testGitHubEnterpriseDoesNotHaveRateLimit() throws Exception {
         rateLimit = gitHub.getRateLimit();
         assertThat(mockGitHub.getRequestCount(), equalTo(6));
 
-        assertThat(rateLimit, notNullValue());
-        assertThat(rateLimit.limit, equalTo(1000000));
-        assertThat(rateLimit.getLimit(), equalTo(1000000));
-        assertThat(rateLimit.remaining, equalTo(1000000));
-        assertThat(rateLimit.getRemaining(), equalTo(1000000));
+        assertThat(rateLimit.getCore(), instanceOf(GHRateLimit.UnknownLimitRecord.class));
+        assertThat(rateLimit.limit, equalTo(GHRateLimit.UnknownLimitRecord.unknownLimit));
+        assertThat(rateLimit.getLimit(), equalTo(GHRateLimit.UnknownLimitRecord.unknownLimit));
+        assertThat(rateLimit.remaining, equalTo(GHRateLimit.UnknownLimitRecord.unknownRemaining));
+        assertThat(rateLimit.getRemaining(), equalTo(GHRateLimit.UnknownLimitRecord.unknownRemaining));
         assertThat(rateLimit.getResetDate().compareTo(lastReset), equalTo(1));
 
         // ratelimit() should prefer headerRateLimit when getRateLimit fails and headerRateLimit is not expired
@@ -307,12 +311,7 @@ public void testGitHubEnterpriseDoesNotHaveRateLimit() throws Exception {
     }
 
 
-    // These three tests should be have the same, showing server time adjustment working
-    @Test
-    public void testGitHubRateLimitExpiration() throws Exception {
-        executeExpirationTest();
-    }
-
+    // These tests should behave the same, showing server time adjustment working
     @Test
     public void testGitHubRateLimitExpirationServerFiveMinutesAhead() throws Exception {
         executeExpirationTest();

diff --git a/src/test/java/org/kohsuke/github/GitHubStaticTest.java b/src/test/java/org/kohsuke/github/GitHubStaticTest.java
@@ -1,5 +1,7 @@
 package org.kohsuke.github;
 
+import com.fasterxml.jackson.annotation.JsonTypeInfo;
+import org.eclipse.jgit.api.Git;
 import org.junit.Assert;
 import org.junit.Test;
 
@@ -11,6 +13,7 @@
 
 import static org.hamcrest.CoreMatchers.equalTo;
 import static org.hamcrest.CoreMatchers.not;
+import static org.hamcrest.core.Is.is;
 
 /**
  * Unit test for {@link GitHub} static helpers.
@@ -68,6 +71,65 @@ public void timeRoundTrip() throws Exception {
         } catch (IllegalStateException e) {
             assertThat(e.getMessage(), equalTo("Unable to parse the timestamp: " + instantBadFormat));
         }
+    }
+
+    @Test
+    public void testGitHubRateLimitShouldReplaceRateLimit() throws Exception {
+
+        GHRateLimit.Record unknown0 = GHRateLimit.Unknown().getCore();
+        GHRateLimit.Record unknown1 = GHRateLimit.Unknown().getCore();
+
+
+
+
+        GHRateLimit.Record record0 = new GHRateLimit.Record(10, 10, 10L);
+        GHRateLimit.Record record1 = new GHRateLimit.Record(10, 9, 10L);
+        GHRateLimit.Record record2 = new GHRateLimit.Record(10, 2, 10L);
+        GHRateLimit.Record record3 = new GHRateLimit.Record(10, 10, 20L);
+        GHRateLimit.Record record4 = new GHRateLimit.Record(10, 5, 20L);
+
+
+        Thread.sleep(2000);
+
+        GHRateLimit.Record recordWorst = new GHRateLimit.Record(Integer.MAX_VALUE, Integer.MAX_VALUE, Long.MIN_VALUE);
+        GHRateLimit.Record record00 = new GHRateLimit.Record(10, 10, 10L);
+        GHRateLimit.Record unknown2 = GHRateLimit.Unknown().getCore();
+
+        // Rate-limit records maybe created and returned in different orders.
+        // We should update to the regular records over unknowns.
+        // After that, we should update to the candidate if its limit is lower or its reset is later.
+
+        assertThat("Equivalent unknown should not replace", GitHub.shouldReplace(unknown0, unknown1), is(false));
+        assertThat("Equivalent unknown should not replace", GitHub.shouldReplace(unknown1, unknown0), is(false));
+
+        assertThat("Later unknown should replace earlier", GitHub.shouldReplace(unknown2, unknown0), is(true));
+        assertThat("Earlier unknown should not replace later", GitHub.shouldReplace(unknown0, unknown2), is(false));
+
+        assertThat("Worst record should replace later unknown", GitHub.shouldReplace(recordWorst, unknown1), is(true));
+        assertThat("Unknown should not replace worst record", GitHub.shouldReplace(unknown1, recordWorst), is(false));
+
+        assertThat("Earlier record should replace later worst", GitHub.shouldReplace(record0, recordWorst), is(true));
+        assertThat("Later worst record should not replace earlier", GitHub.shouldReplace(recordWorst, record0), is(false));
+
+        assertThat("Equivalent record should not replace", GitHub.shouldReplace(record0, record00), is(false));
+        assertThat("Equivalent record should not replace", GitHub.shouldReplace(record00, record0), is(false));
+
+        assertThat("Lower limit record should replace higher", GitHub.shouldReplace(record1, record0), is(true));
+        assertThat("Lower limit record should replace higher", GitHub.shouldReplace(record2, record1), is(true));
+
+        assertThat("Higher limit record should not replace lower", GitHub.shouldReplace(record1, record2), is(false));
+
+        assertThat("Higher limit record with later reset should  replace lower", GitHub.shouldReplace(record3, record2), is(true));
+
+        assertThat("Lower limit record with later reset should replace higher", GitHub.shouldReplace(record4, record1), is(true));
+
+        assertThat("Lower limit record with earlier reset should not replace higher", GitHub.shouldReplace(record2, record4), is(false));
+
+
+
+
+
+
     }
 
     static String formatDate(Date dt, String format) {

diff --git a/...wiremock/testGitHubRateLimit/__files/rate_limit-4e2fc33b-fb25-4dfc-9d56-34f9b4d707be.json b/...wiremock/testGitHubRateLimit/__files/rate_limit-4e2fc33b-fb25-4dfc-9d56-34f9b4d707be.json
@@ -3,27 +3,27 @@
     "core": {
       "limit": 5000,
       "remaining": 4896,
-      "reset": 1570478899
+      "reset": {{now offset='1 hours' format='unix'}}
     },
     "search": {
       "limit": 30,
       "remaining": 30,
-      "reset": 1570478180
+      "reset": {{now offset='1 hours' format='unix'}}
     },
     "graphql": {
       "limit": 5000,
       "remaining": 5000,
-      "reset": 1570481642
+      "reset": {{now offset='1 hours' format='unix'}}
     },
     "integration_manifest": {
       "limit": 5000,
       "remaining": 5000,
-      "reset": 1570481642
+      "reset": {{now offset='1 hours' format='unix'}}
     }
   },
   "rate": {
     "limit": 5000,
     "remaining": 4896,
-    "reset": {{now offset='1 hours' format='unix'}}
+    "reset": 1570478899
   }
 }
diff --git a/...wiremock/testGitHubRateLimit/__files/rate_limit-a1f82a96-500c-4462-ae7b-e0159afa8208.json b/...wiremock/testGitHubRateLimit/__files/rate_limit-a1f82a96-500c-4462-ae7b-e0159afa8208.json
@@ -3,27 +3,27 @@
     "core": {
       "limit": 5000,
       "remaining": 4897,
-      "reset": 1570478899
+      "reset": {{now offset='1 hours' format='unix'}}
     },
     "search": {
       "limit": 30,
       "remaining": 30,
-      "reset": 1570478180
+      "reset": {{now offset='1 hours' format='unix'}}
     },
     "graphql": {
       "limit": 5000,
       "remaining": 5000,
-      "reset": 1570481642
+      "reset": {{now offset='1 hours' format='unix'}}
     },
     "integration_manifest": {
       "limit": 5000,
       "remaining": 5000,
-      "reset": 1570481642
+      "reset": {{now offset='1 hours' format='unix'}}
     }
   },
   "rate": {
     "limit": 5000,
     "remaining": 4897,
-    "reset": {{now offset='1 hours' format='unix'}}
+    "reset": 1570478899
   }
 }
diff --git a/...wiremock/testGitHubRateLimit/__files/rate_limit-f22b1cea-3679-481d-8b95-459b2c47bf98.json b/...wiremock/testGitHubRateLimit/__files/rate_limit-f22b1cea-3679-481d-8b95-459b2c47bf98.json
@@ -3,27 +3,27 @@
     "core": {
       "limit": 5000,
       "remaining": 4897,
-      "reset": 1570478899
+      "reset": {{now offset='1 hours' format='unix'}}
     },
     "search": {
       "limit": 30,
       "remaining": 30,
-      "reset": 1570478180
+      "reset": {{now offset='1 hours' format='unix'}}
     },
     "graphql": {
       "limit": 5000,
       "remaining": 5000,
-      "reset": 1570481642
+      "reset": {{now offset='1 hours' format='unix'}}
     },
     "integration_manifest": {
       "limit": 5000,
       "remaining": 5000,
-      "reset": 1570481642
+      "reset": {{now offset='1 hours' format='unix'}}
     }
   },
   "rate": {
     "limit": 5000,
     "remaining": 4897,
-    "reset": {{now offset='1 hours' format='unix'}}
+    "reset": 1570478899
   }
 }