- * Has a large limit and long duration so that it will doesn't expire too often. * * @since 1.100 */ public static class UnknownLimitRecord extends Record { - // One hour - private static final long unknownLimitResetSeconds = 60L * 60L; + /** + * This is set to a somewhat small duration, rather than a long one. + * + * {@link GitHubClient#rateLimit(String)} will not query for a new rate limit until the current Record expires. + * When {@link GitHubClient} is initialized it includes a default {@link GHRateLimit#Default()} which has + * expired rate limit records. This guarantees that the first time {@link GitHubClient#rateLimit(String)} is + * called TODO + * + * The + */ + static long unknownLimitResetSeconds = Duration.ofSeconds(30).toMillis() * 1000; static final int unknownLimit = 1000000; static final int unknownRemaining = 999999; - private UnknownLimitRecord() { - super(unknownLimit, unknownRemaining, System.currentTimeMillis() / 1000L + unknownLimitResetSeconds); + // The default UnknownLimitRecord is an expired record. + private static final UnknownLimitRecord DEFAULT = new UnknownLimitRecord(Long.MIN_VALUE); + + // The starting current UnknownLimitRecord is an expired record. + private static UnknownLimitRecord current = DEFAULT; + + /** + * Internal For testing only. + */ + UnknownLimitRecord() { + this(System.currentTimeMillis() / 1000L + unknownLimitResetSeconds); + } + + /** + * Not for use outside this class + * + * @param resetEpochSeconds + * the epoch second time when this record will expire. + */ + private UnknownLimitRecord(long resetEpochSeconds) { + super(unknownLimit, unknownRemaining, resetEpochSeconds); + } + + static synchronized Record current() { + if (current.isExpired()) { + current = new UnknownLimitRecord(); + } + return current; + } + + static synchronized void reset() { + current = DEFAULT; } } @@ -280,8 +364,6 @@ public class GHRateLimit { /** * The time at which the current rate limit window resets in UTC epoch seconds. - * - * This is the raw value returned by the server. */ private final long resetEpochSeconds; @@ -291,9 +373,11 @@ public class GHRateLimit { private final long createdAtEpochSeconds = System.currentTimeMillis() / 1000; /** - * The time at which the rate limit will reset. This value is calculated based on - * {@link #getResetEpochSeconds()} by calling {@link #calculateResetDate}. If the clock on the local machine not - * synchronized with the server clock, this time value will be adjusted to match the local machine's clock. + * The time at which the rate limit will reset. + * + * This value is calculated based on {@link #getResetEpochSeconds()} by calling + * {@link #calculateResetDate(String)}. If the clock on the local machine not synchronized with the server + * clock, this time value will be adjusted to match the local machine's clock. */ @Nonnull private final Date resetDate; @@ -341,6 +425,52 @@ public class GHRateLimit { this.resetDate = calculateResetDate(updatedAt); } + /** + * Determine if the current {@link Record} is outdated compared to another. Rate Limit dates are only accurate + * to the second, so we look at other information in the record as well. + * + * {@link Record}s with earlier {@link #getResetEpochSeconds()} are replaced by those with later. + * {@link Record}s with the same {@link #getResetEpochSeconds()} are replaced by those with less remaining + * count. + * + * {@link UnknownLimitRecord}s compare with each other like regular {@link Record}s. + * + * {@link Record}s are replaced by {@link UnknownLimitRecord}s only when the current {@link Record} is expired + * and the {@link UnknownLimitRecord} is not. Otherwise Regular {@link Record}s are not replaced by + * {@link UnknownLimitRecord}s. + * + * Expiration is only considered after other checks, meaning expired records may sometimes be replaced by other + * expired records. + * + * @param other + * the other {@link Record} + * @return the {@link Record} that is most current + */ + Record currentOrUpdated(@Nonnull Record other) { + // This set of checks avoids most calls to isExpired() + // Depends on UnknownLimitRecord.current() to prevent continuous updating of GHRateLimit rateLimit() + if (getResetEpochSeconds() > other.getResetEpochSeconds() + || (getResetEpochSeconds() == other.getResetEpochSeconds() + && getRemaining() <= other.getRemaining())) { + // If the current record has a later reset + // or the current record has the same reset and fewer or same requests remaining + // Then it is most recent + return this; + } else if (!(other instanceof UnknownLimitRecord)) { + // If the above is not the case that means other has a later reset + // or the same resent and fewer requests remaining. + // If the other record is not an unknown record, the the other is more recent + return other; + } else if (this.isExpired() && !other.isExpired()) { + // The other is an unknown record. + // If the current record has expired and the other hasn't, return the other. + return other; + } + + // If none of the above, the current record is most valid. + return this; + } + /** * Recalculates the {@link #resetDate} relative to the local machine clock. *
diff --git a/src/main/java/org/kohsuke/github/GitHub.java b/src/main/java/org/kohsuke/github/GitHub.java index 73a08df1b..bf9bc0c3f 100644 --- a/src/main/java/org/kohsuke/github/GitHub.java +++ b/src/main/java/org/kohsuke/github/GitHub.java @@ -373,12 +373,19 @@ public class GitHub { } /** - * Gets the current rate limit. + * Gets the current rate limit from the server. + * + * For some versions of GitHub Enterprise, the {@code /rate_limit} endpoint returns a {@code 404 Not Found}. In that + * case, if the most recent {@link GHRateLimit} will be returned. + * + * For most use cases it would be better to implement a {@link RateLimitChecker} and add it via + * {@link GitHubBuilder#withRateLimitChecker(RateLimitChecker)}. * * @return the rate limit * @throws IOException * the io exception */ + @Nonnull public GHRateLimit getRateLimit() throws IOException { return client.getRateLimit(); } @@ -388,8 +395,11 @@ public class GitHub { * GitHub Enterprise) or if no requests have been made. * * @return the most recently observed rate limit data or {@code null}. + * @deprecated implement a {@link RateLimitChecker} and add it via + * {@link GitHubBuilder#withRateLimitChecker(RateLimitChecker)}. */ - @CheckForNull + @Nonnull + @Deprecated public GHRateLimit lastRateLimit() { return client.lastRateLimit(); } @@ -400,10 +410,13 @@ public class GitHub { * @return the current rate limit data. * @throws IOException * if we couldn't get the current rate limit data. + * @deprecated implement a {@link RateLimitChecker} and add it via + * {@link GitHubBuilder#withRateLimitChecker(RateLimitChecker)}. */ @Nonnull + @Deprecated public GHRateLimit rateLimit() throws IOException { - return client.rateLimit(); + return client.rateLimit(""); } /** diff --git a/src/main/java/org/kohsuke/github/GitHubClient.java b/src/main/java/org/kohsuke/github/GitHubClient.java index a844362fe..0638b5748 100644 --- a/src/main/java/org/kohsuke/github/GitHubClient.java +++ b/src/main/java/org/kohsuke/github/GitHubClient.java @@ -45,7 +45,7 @@ import static java.util.logging.Level.*; * A GitHub API Client *
* A GitHubClient can be used to send requests and retrieve their responses. GitHubClient is thread-safe and can be used - * to send multiple requests. GitHubClient also track some GitHub API information such as {@link #rateLimit()}. + * to send multiple requests. GitHubClient also track some GitHub API information such as {@link GHRateLimit}. *
*/ abstract class GitHubClient { @@ -71,9 +71,10 @@ abstract class GitHubClient { private HttpConnector connector; - private final Object headerRateLimitLock = new Object(); - private GHRateLimit headerRateLimit = null; - private volatile GHRateLimit rateLimit = null; + private final Object rateLimitLock = new Object(); + + @Nonnull + private GHRateLimit rateLimit = GHRateLimit.Default(); private static final Logger LOGGER = Logger.getLogger(GitHubClient.class.getName()); @@ -211,9 +212,11 @@ abstract class GitHubClient { /** * Gets the current rate limit from the server. * - * For some versions of GitHub Enterprise, the {@code /rate_limit} endpoint returns a {@code 404 Not Found}. In - * that, if {@link #lastRateLimit()} is not {@code null} and is not expired, it will be returned. Otherwise, a - * placeholder {@link GHRateLimit} instance with {@link GHRateLimit.UnknownLimitRecord}s will be returned. + * For some versions of GitHub Enterprise, the {@code /rate_limit} endpoint returns a {@code 404 Not Found}. In that + * case, if the most recent {@link GHRateLimit} will be returned. + * + * For most use cases it would be better to implement a {@link RateLimitChecker} and add it via + * {@link GitHubBuilder#withRateLimitChecker(RateLimitChecker)}. * * @return the rate limit * @throws IOException @@ -221,59 +224,87 @@ abstract class GitHubClient { */ @Nonnull public GHRateLimit getRateLimit() throws IOException { + return getRateLimit("/rate_limit"); + } + + @Nonnull + GHRateLimit getRateLimit(@Nonnull String urlPath) throws IOException { GHRateLimit result; try { result = fetch(JsonRateLimit.class, "/rate_limit").resources; } catch (FileNotFoundException e) { // For some versions of GitHub Enterprise, the rate_limit endpoint returns a 404. + LOGGER.log(FINE, "/rate_limit returned 404 Not Found."); + // However some newer versions of GHE include rate limit header information - // Use that if available - result = lastRateLimit(); - if (result == null || result.isExpired()) { - // return a default rate limit - result = GHRateLimit.Unknown(); - } + // If the header info is missing and the endpoint returns 404, fill the rate limit + // with unknown + result = GHRateLimit.Unknown(urlPath); } - - return rateLimit = result; + return updateRateLimit(result); } /** - * 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. + * Returns the most recently observed rate limit data. * - * @return the most recently observed rate limit data or {@code null}. + * Generally, instead of calling this you should implement a {@link RateLimitChecker} or call + * + * @return the most recently observed rate limit data. This may include expired or + * {@link GHRateLimit.UnknownLimitRecord} entries. + * @deprecated implement a {@link RateLimitChecker} and add it via + * {@link GitHubBuilder#withRateLimitChecker(RateLimitChecker)}. */ - @CheckForNull - public GHRateLimit lastRateLimit() { - synchronized (headerRateLimitLock) { - return headerRateLimit; + @Nonnull + @Deprecated + GHRateLimit lastRateLimit() { + synchronized (rateLimitLock) { + return rateLimit; } } /** - * Gets the current rate limit while trying not to actually make any remote requests unless absolutely necessary. + * Gets the current rate limit for an endpoint while trying not to actually make any remote requests unless + * absolutely necessary. * - * If {@link #lastRateLimit()} is not {@code null} and is not expired, it will be returned. If the information - * returned from the last call to {@link #getRateLimit()} is not {@code null} and is not expired, then it will be - * returned. Otherwise, the result of a call to {@link #getRateLimit()} will be returned. + * If the {@link GHRateLimit.Record} for {@code urlPath} is not expired, it is returned. If the + * {@link GHRateLimit.Record} for {@code urlPath} is expired, {@link #getRateLimit()} will be called to get the + * current rate limit. * - * @return the current rate limit data. + * @param urlPath + * the path for the endpoint to get the rate limit for. + * + * @return the current rate limit data. {@link GHRateLimit.Record}s in this instance may be expired when returned. * @throws IOException * if there was an error getting current rate limit data. */ @Nonnull - public GHRateLimit rateLimit() throws IOException { - synchronized (headerRateLimitLock) { - if (headerRateLimit != null && !headerRateLimit.isExpired()) { - return headerRateLimit; + GHRateLimit rateLimit(@Nonnull String urlPath) throws IOException { + synchronized (rateLimitLock) { + if (rateLimit.getRecordForUrlPath(urlPath).isExpired()) { + getRateLimit(urlPath); } + return rateLimit; } - GHRateLimit result = this.rateLimit; - if (result == null || result.isExpired()) { - result = getRateLimit(); + } + + /** + * 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. Calls + * {@link GHRateLimit#getMergedRateLimit(GHRateLimit)} + * + * @param observed + * {@link GHRateLimit.Record} constructed from the response header information + */ + private GHRateLimit updateRateLimit(@Nonnull GHRateLimit observed) { + synchronized (rateLimitLock) { + observed = rateLimit.getMergedRateLimit(observed); + + if (rateLimit != observed) { + rateLimit = observed; + LOGGER.log(FINE, "Rate limit now: {0}", rateLimit); + } + return rateLimit; } - return result; } /** @@ -517,58 +548,25 @@ abstract class GitHubClient { } private void noteRateLimit(@Nonnull GitHubResponse.ResponseInfo responseInfo) { - if (responseInfo.request().urlPath().startsWith("/search")) { - // the search API uses a different rate limit - return; - } - - String limitString = responseInfo.headerField("X-RateLimit-Limit"); - if (StringUtils.isBlank(limitString)) { - // if we are missing a header, return fast - return; - } - String remainingString = responseInfo.headerField("X-RateLimit-Remaining"); - if (StringUtils.isBlank(remainingString)) { - // if we are missing a header, return fast - return; - } - String resetString = responseInfo.headerField("X-RateLimit-Reset"); - if (StringUtils.isBlank(resetString)) { - // if we are missing a header, return fast - return; - } - - int limit, remaining; - long reset; try { + String limitString = Objects.requireNonNull(responseInfo.headerField("X-RateLimit-Limit"), + "Missing X-RateLimit-Limit"); + String remainingString = Objects.requireNonNull(responseInfo.headerField("X-RateLimit-Remaining"), + "Missing X-RateLimit-Remaining"); + String resetString = Objects.requireNonNull(responseInfo.headerField("X-RateLimit-Reset"), + "Missing X-RateLimit-Reset"); + int limit, remaining; + long reset; limit = Integer.parseInt(limitString); - } catch (NumberFormatException e) { - if (LOGGER.isLoggable(FINEST)) { - LOGGER.log(FINEST, "Malformed X-RateLimit-Limit header value " + limitString, e); - } - return; - } - try { - remaining = Integer.parseInt(remainingString); - } catch (NumberFormatException e) { - if (LOGGER.isLoggable(FINEST)) { - LOGGER.log(FINEST, "Malformed X-RateLimit-Remaining header value " + remainingString, e); - } - return; - } - try { reset = Long.parseLong(resetString); - } catch (NumberFormatException e) { + GHRateLimit.Record observed = new GHRateLimit.Record(limit, remaining, reset, responseInfo); + updateRateLimit(GHRateLimit.fromHeaderRecord(observed, responseInfo.request().urlPath())); + } catch (NumberFormatException | NullPointerException e) { if (LOGGER.isLoggable(FINEST)) { - LOGGER.log(FINEST, "Malformed X-RateLimit-Reset header value " + resetString, e); + LOGGER.log(FINEST, "Missing or malformed X-RateLimit header: ", e); } - return; } - - GHRateLimit.Record observed = new GHRateLimit.Record(limit, remaining, reset, responseInfo); - - updateCoreRateLimit(observed); } private static void detectOTPRequired(@Nonnull GitHubResponse.ResponseInfo responseInfo) throws GHIOException { @@ -588,23 +586,6 @@ abstract class GitHubClient { "This operation requires a credential but none is given to the GitHub constructor"); } - /** - * 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. Calls - * {@link #shouldReplace(GHRateLimit.Record, GHRateLimit.Record)} - * - * @param observed - * {@link GHRateLimit.Record} constructed from the response header information - */ - private void updateCoreRateLimit(@Nonnull GHRateLimit.Record observed) { - synchronized (headerRateLimitLock) { - if (headerRateLimit == null || shouldReplace(observed, headerRateLimit.getCore())) { - headerRateLimit = GHRateLimit.fromHeaderRecord(observed); - LOGGER.log(FINE, "Rate limit now: {0}", headerRateLimit); - } - } - } - private static class GHApiInfo { private String rate_limit_url; @@ -654,37 +635,6 @@ abstract class GitHubClient { } } - /** - * Determine if one {@link GHRateLimit.Record} should replace another. 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()); - } - } - static URL parseURL(String s) { try { return s == null ? null : new URL(s); diff --git a/src/main/java/org/kohsuke/github/GitHubHttpUrlConnectionClient.java b/src/main/java/org/kohsuke/github/GitHubHttpUrlConnectionClient.java index 4e2ebf1d4..dcc56529a 100644 --- a/src/main/java/org/kohsuke/github/GitHubHttpUrlConnectionClient.java +++ b/src/main/java/org/kohsuke/github/GitHubHttpUrlConnectionClient.java @@ -24,7 +24,7 @@ import static org.apache.commons.lang3.StringUtils.defaultString; * A GitHub API Client for HttpUrlConnection ** A GitHubClient can be used to send requests and retrieve their responses. GitHubClient is thread-safe and can be used - * to send multiple requests. GitHubClient also track some GitHub API information such as {@link #rateLimit()}. + * to send multiple requests. GitHubClient also track some GitHub API information such as {@link GHRateLimit}. *
** GitHubHttpUrlConnectionClient gets a new {@link HttpURLConnection} for each call to send. diff --git a/src/main/java/org/kohsuke/github/GitHubRateLimitChecker.java b/src/main/java/org/kohsuke/github/GitHubRateLimitChecker.java index f819c52b7..679f7b6f6 100644 --- a/src/main/java/org/kohsuke/github/GitHubRateLimitChecker.java +++ b/src/main/java/org/kohsuke/github/GitHubRateLimitChecker.java @@ -3,6 +3,7 @@ package org.kohsuke.github; import java.io.IOException; import java.io.InterruptedIOException; import java.util.Objects; +import java.util.logging.Logger; import javax.annotation.Nonnull; @@ -39,6 +40,8 @@ class GitHubRateLimitChecker { @Nonnull private final RateLimitChecker integrationManifest; + private static final Logger LOGGER = Logger.getLogger(GitHubRateLimitChecker.class.getName()); + GitHubRateLimitChecker() { this(RateLimitChecker.NONE, RateLimitChecker.NONE, RateLimitChecker.NONE, RateLimitChecker.NONE); } @@ -48,13 +51,6 @@ class GitHubRateLimitChecker { @Nonnull RateLimitChecker graphql, @Nonnull RateLimitChecker integrationManifest) { this.core = Objects.requireNonNull(core); - - // for now only support rate limiting on core - // remove these asserts when that changes - assert search == RateLimitChecker.NONE; - assert graphql == RateLimitChecker.NONE; - assert integrationManifest == RateLimitChecker.NONE; - this.search = Objects.requireNonNull(search); this.graphql = Objects.requireNonNull(graphql); this.integrationManifest = Objects.requireNonNull(integrationManifest); @@ -98,7 +94,7 @@ class GitHubRateLimitChecker { } // For the first rate limit, accept the current limit if a valid one is already present. - GHRateLimit rateLimit = client.rateLimit(); + GHRateLimit rateLimit = client.rateLimit(request.urlPath()); GHRateLimit.Record rateLimitRecord = rateLimit.getRecordForUrlPath(request.urlPath()); long waitCount = 0; try { @@ -112,7 +108,7 @@ class GitHubRateLimitChecker { Thread.sleep(1000); // After the first wait, always request a new rate limit from the server. - rateLimit = client.getRateLimit(); + rateLimit = client.getRateLimit(request.urlPath()); rateLimitRecord = rateLimit.getRecordForUrlPath(request.urlPath()); } } catch (InterruptedException e) { diff --git a/src/test/java/org/kohsuke/github/AppTest.java b/src/test/java/org/kohsuke/github/AppTest.java index c6f447cfd..10ceec5b0 100755 --- a/src/test/java/org/kohsuke/github/AppTest.java +++ b/src/test/java/org/kohsuke/github/AppTest.java @@ -20,6 +20,7 @@ import java.util.Map.Entry; import java.util.regex.Pattern; import static org.hamcrest.CoreMatchers.*; +import static org.hamcrest.CoreMatchers.sameInstance; import static org.hamcrest.Matchers.hasProperty; import static org.hamcrest.Matchers.oneOf; @@ -108,7 +109,7 @@ public class AppTest extends AbstractGitHubWireMockTest { gitHub = getGitHubBuilder().withOAuthToken("bogus", "user") .withEndpoint(mockGitHub.apiServer().baseUrl()) .build(); - assertThat(gitHub.lastRateLimit(), nullValue()); + assertThat(gitHub.lastRateLimit(), equalTo(GHRateLimit.Default())); assertFalse(gitHub.isCredentialValid()); // For invalid credentials, we get a 401 but it includes anonymous rate limit headers assertThat(gitHub.lastRateLimit().getCore(), not(instanceOf(GHRateLimit.UnknownLimitRecord.class))); @@ -118,18 +119,22 @@ public class AppTest extends AbstractGitHubWireMockTest { @Test public void testCredentialValidEnterprise() throws IOException { // Simulated GHE: getRateLimit returns 404 - assertThat(gitHub.lastRateLimit(), nullValue()); + assertThat(gitHub.lastRateLimit(), equalTo(GHRateLimit.Default())); + assertThat(gitHub.lastRateLimit().getCore().isExpired(), is(true)); assertTrue(gitHub.isCredentialValid()); - // lastRateLimit stays null when 404 is encountered - assertThat(gitHub.lastRateLimit(), nullValue()); + + // lastRateLimitUpdates because 404 still includes header rate limit info + assertThat(gitHub.lastRateLimit(), notNullValue()); + assertThat(gitHub.lastRateLimit(), not(equalTo(GHRateLimit.Default()))); + assertThat(gitHub.lastRateLimit().getCore().isExpired(), is(false)); gitHub = getGitHubBuilder().withOAuthToken("bogus", "user") .withEndpoint(mockGitHub.apiServer().baseUrl()) .build(); - assertThat(gitHub.lastRateLimit(), nullValue()); + assertThat(gitHub.lastRateLimit(), equalTo(GHRateLimit.Default())); assertFalse(gitHub.isCredentialValid()); // Simulated GHE: For invalid credentials, we get a 401 that does not include ratelimit info - assertThat(gitHub.lastRateLimit(), nullValue()); + assertThat(gitHub.lastRateLimit(), equalTo(GHRateLimit.Default())); } @Test diff --git a/src/test/java/org/kohsuke/github/GHRateLimitTest.java b/src/test/java/org/kohsuke/github/GHRateLimitTest.java index 32b9ceba1..1cd5434b8 100644 --- a/src/test/java/org/kohsuke/github/GHRateLimitTest.java +++ b/src/test/java/org/kohsuke/github/GHRateLimitTest.java @@ -3,13 +3,14 @@ package org.kohsuke.github; import com.fasterxml.jackson.databind.exc.MismatchedInputException; import com.fasterxml.jackson.databind.exc.ValueInstantiationException; import com.github.tomakehurst.wiremock.core.WireMockConfiguration; -import org.hamcrest.CoreMatchers; import org.junit.Test; import java.io.IOException; import java.time.Duration; import java.util.Date; +import static org.hamcrest.CoreMatchers.equalTo; +import static org.hamcrest.CoreMatchers.sameInstance; import static org.hamcrest.Matchers.*; import static org.hamcrest.core.IsInstanceOf.instanceOf; @@ -51,13 +52,14 @@ public class GHRateLimitTest extends AbstractGitHubWireMockTest { public void testGitHubRateLimit() throws Exception { // Customized response that templates the date to keep things working snapshotNotAllowed(); + GHRateLimit.UnknownLimitRecord.reset(); assertThat(mockGitHub.getRequestCount(), equalTo(0)); // 4897 is just the what the limit was when the snapshot was taken previousLimit = GHRateLimit.fromHeaderRecord(new GHRateLimit.Record(5000, 4897, - (templating.testStartDate.getTime() + Duration.ofHours(1).toMillis()) / 1000L)); + (templating.testStartDate.getTime() + Duration.ofHours(1).toMillis()) / 1000L), ""); // ------------------------------------------------------------- // /user gets response with rate limit information @@ -76,8 +78,8 @@ public class GHRateLimitTest extends AbstractGitHubWireMockTest { // Give this a moment Thread.sleep(1500); - // ratelimit() uses headerRateLimit if available and headerRateLimit is not expired - assertThat(gitHub.rateLimit(), equalTo(headerRateLimit)); + // ratelimit() uses cached rate limit if available and not expired + assertThat(gitHub.rateLimit(), sameInstance(headerRateLimit)); assertThat(mockGitHub.getRequestCount(), equalTo(1)); @@ -88,8 +90,13 @@ public class GHRateLimitTest extends AbstractGitHubWireMockTest { rateLimit = gitHub.getRateLimit(); assertThat(mockGitHub.getRequestCount(), equalTo(2)); - // Because remaining and reset date are unchanged, the header should be unchanged as well - assertThat(gitHub.lastRateLimit(), sameInstance(headerRateLimit)); + // Because remaining and reset date are unchanged in core, the header should be unchanged as well + // But the overall instance has changed because of filling in of unknown data. + assertThat(gitHub.lastRateLimit(), not(sameInstance(headerRateLimit))); + // Identical Records should be preserved even when GHRateLimit is merged + assertThat(gitHub.lastRateLimit().getCore(), sameInstance(headerRateLimit.getCore())); + assertThat(gitHub.lastRateLimit().getSearch(), not(sameInstance(headerRateLimit.getSearch()))); + headerRateLimit = gitHub.lastRateLimit(); // rate limit request is free, remaining is unchanged verifyRateLimitValues(previousLimit, previousLimit.getRemaining()); @@ -141,9 +148,8 @@ public class GHRateLimitTest extends AbstractGitHubWireMockTest { verifyRateLimitValues(previousLimit, previousLimit.getRemaining(), true); previousLimit = rateLimit; - // When getRateLimit() succeeds, headerRateLimit updates as usual as well (if needed) - // These are separate instances, but should be equal - assertThat(gitHub.rateLimit(), not(sameInstance(rateLimit))); + // When getRateLimit() succeeds, cached rate limit updates as usual as well (if needed) + assertThat(gitHub.rateLimit(), sameInstance(rateLimit)); // Verify different record instances can be compared assertThat(gitHub.rateLimit().getCore(), equalTo(rateLimit.getCore())); @@ -194,6 +200,8 @@ public class GHRateLimitTest extends AbstractGitHubWireMockTest { public void testGitHubEnterpriseDoesNotHaveRateLimit() throws Exception { // Customized response that results in file not found the same as GitHub Enterprise snapshotNotAllowed(); + GHRateLimit.UnknownLimitRecord.reset(); + assertThat(mockGitHub.getRequestCount(), equalTo(0)); GHRateLimit rateLimit = null; @@ -203,13 +211,14 @@ public class GHRateLimitTest extends AbstractGitHubWireMockTest { Thread.sleep(1500); // ------------------------------------------------------------- - // Before any queries, rate limit starts as null but may be requested + // Before any queries, rate limit starts as default but may be requested gitHub = GitHub.connectToEnterprise(mockGitHub.apiServer().baseUrl(), "bogus", "bogus"); assertThat(mockGitHub.getRequestCount(), equalTo(0)); - assertThat(gitHub.lastRateLimit(), CoreMatchers.nullValue()); + assertThat(gitHub.lastRateLimit(), equalTo(GHRateLimit.Default())); rateLimit = gitHub.rateLimit(); + assertThat(gitHub.lastRateLimit(), not(equalTo(GHRateLimit.Default()))); assertThat(rateLimit.getCore(), instanceOf(GHRateLimit.UnknownLimitRecord.class)); assertThat(rateLimit.getLimit(), equalTo(GHRateLimit.UnknownLimitRecord.unknownLimit)); assertThat(rateLimit.getRemaining(), equalTo(GHRateLimit.UnknownLimitRecord.unknownRemaining)); @@ -218,8 +227,8 @@ public class GHRateLimitTest extends AbstractGitHubWireMockTest { assertThat(mockGitHub.getRequestCount(), equalTo(1)); - // last is still null, because it actually means lastHeaderRateLimit - assertThat(gitHub.lastRateLimit(), CoreMatchers.nullValue()); + // lastRateLimit the same as rateLimit + assertThat(gitHub.lastRateLimit(), sameInstance(rateLimit)); assertThat(mockGitHub.getRequestCount(), equalTo(1)); @@ -233,13 +242,14 @@ public class GHRateLimitTest extends AbstractGitHubWireMockTest { gitHub.getMyself(); assertThat(mockGitHub.getRequestCount(), equalTo(2)); - assertThat(gitHub.lastRateLimit(), CoreMatchers.nullValue()); + assertThat(gitHub.lastRateLimit(), equalTo(GHRateLimit.Default())); rateLimit = gitHub.rateLimit(); assertThat(rateLimit.getCore(), instanceOf(GHRateLimit.UnknownLimitRecord.class)); assertThat(rateLimit.getLimit(), equalTo(GHRateLimit.UnknownLimitRecord.unknownLimit)); assertThat(rateLimit.getRemaining(), equalTo(GHRateLimit.UnknownLimitRecord.unknownRemaining)); - assertThat(rateLimit.getResetDate().compareTo(lastReset), equalTo(1)); + // Same unknown instance is reused for a while + assertThat(rateLimit.getResetDate().compareTo(lastReset), equalTo(0)); lastReset = rateLimit.getResetDate(); assertThat(mockGitHub.getRequestCount(), equalTo(3)); @@ -258,13 +268,14 @@ public class GHRateLimitTest extends AbstractGitHubWireMockTest { assertThat(rateLimit.getCore(), instanceOf(GHRateLimit.UnknownLimitRecord.class)); assertThat(rateLimit.getLimit(), equalTo(GHRateLimit.UnknownLimitRecord.unknownLimit)); assertThat(rateLimit.getRemaining(), equalTo(GHRateLimit.UnknownLimitRecord.unknownRemaining)); - assertThat(rateLimit.getResetDate().compareTo(lastReset), equalTo(1)); + // When not expired, unknowns do not replace each other so last reset remains unchanged + assertThat(rateLimit.getResetDate().compareTo(lastReset), equalTo(0)); // Give this a moment Thread.sleep(1500); - // last is still null, because it actually means lastHeaderRateLimit - assertThat(gitHub.lastRateLimit(), CoreMatchers.nullValue()); + // lastRateLimit the same as rateLimit + assertThat(gitHub.lastRateLimit(), sameInstance(rateLimit)); // ratelimit() tries not to make additional requests, uses queried rate limit since header not available Thread.sleep(1500); @@ -286,6 +297,9 @@ public class GHRateLimitTest extends AbstractGitHubWireMockTest { assertThat(rateLimit.getResetDate().compareTo(lastReset), equalTo(-1)); lastReset = rateLimit.getResetDate(); + // When getting only header updates, the unknowns are also expired + assertThat(rateLimit.getSearch().isExpired(), is(true)); + GHRateLimit headerRateLimit = rateLimit; // Give this a moment @@ -325,9 +339,6 @@ public class GHRateLimitTest extends AbstractGitHubWireMockTest { assertThat(gitHub.rateLimit(), sameInstance(rateLimit)); assertThat(mockGitHub.getRequestCount(), equalTo(6)); - - // Wait for the header - Thread.sleep(1500); } @Test @@ -372,6 +383,7 @@ public class GHRateLimitTest extends AbstractGitHubWireMockTest { private void executeExpirationTest() throws Exception { // Customized response that templates the date to keep things working snapshotNotAllowed(); + GHRateLimit.UnknownLimitRecord.reset(); assertThat(mockGitHub.getRequestCount(), equalTo(0)); GHRateLimit rateLimit = null; @@ -413,15 +425,15 @@ public class GHRateLimitTest extends AbstractGitHubWireMockTest { assertThat("Header instance has expired", gitHub.lastRateLimit().isExpired(), is(true)); - assertThat("rateLimit() will ask server when header instance expires and it has not called getRateLimit() yet", + assertThat("rateLimit() will ask server when cached instance has expired", gitHub.rateLimit(), not(sameInstance(rateLimit))); - assertThat("lastRateLimit() (header instance) is populated as part of internal call to getRateLimit()", + assertThat("lastRateLimit() is populated as part of internal call to getRateLimit()", gitHub.lastRateLimit(), not(sameInstance(rateLimit))); - assertThat("After request, rateLimit() selects header instance since it has been refreshed", + assertThat("After request, rateLimit() selects cached since it has been refreshed", gitHub.rateLimit(), sameInstance(gitHub.lastRateLimit())); @@ -429,27 +441,27 @@ public class GHRateLimitTest extends AbstractGitHubWireMockTest { assertThat(mockGitHub.getRequestCount(), equalTo(2)); - // This time, rateLimit() should find an expired header record, but a valid returned record + // During the previous call we returned expired header info but valid returned record + // Merging means this has already been merged into a valid cached instance Thread.sleep(4000); rateLimit = gitHub.rateLimit(); // Using custom data to have a header instance that expires before the queried instance - assertThat( - "if header instance expires but queried instance is valid, ratelimit() uses it without asking server", + assertThat("if valid ratelimit() uses it without asking server", gitHub.rateLimit(), - not(sameInstance(gitHub.lastRateLimit()))); + sameInstance(gitHub.lastRateLimit())); assertThat("ratelimit() should almost never return a return a GHRateLimit that is already expired", gitHub.rateLimit().isExpired(), is(false)); - assertThat("Header instance hasn't been reloaded", gitHub.lastRateLimit(), sameInstance(headerRateLimit)); - assertThat("Header instance has expired", gitHub.lastRateLimit().isExpired(), is(true)); + assertThat("Cached instance hasn't been reloaded", gitHub.lastRateLimit(), sameInstance(headerRateLimit)); + assertThat("Cached instance has not expired", gitHub.lastRateLimit().isExpired(), is(false)); assertThat(mockGitHub.getRequestCount(), equalTo(2)); - // Finally they both expire and rateLimit() should find both expired and get a new record + // Finally the cached instance expires and rateLimit() should get a new record Thread.sleep(2000); headerRateLimit = gitHub.rateLimit(); diff --git a/src/test/java/org/kohsuke/github/GitHubStaticTest.java b/src/test/java/org/kohsuke/github/GitHubStaticTest.java index b2d6eec29..c52d834db 100644 --- a/src/test/java/org/kohsuke/github/GitHubStaticTest.java +++ b/src/test/java/org/kohsuke/github/GitHubStaticTest.java @@ -9,7 +9,7 @@ import java.util.Date; import java.util.TimeZone; import static org.hamcrest.CoreMatchers.*; -import static org.hamcrest.core.Is.is; +import static org.junit.Assert.fail; /** * Unit test for {@link GitHub} static helpers. @@ -65,68 +65,138 @@ public class GitHubStaticTest extends AbstractGitHubWireMockTest { @Test public void testGitHubRateLimitShouldReplaceRateLimit() throws Exception { - GHRateLimit.Record unknown0 = GHRateLimit.Unknown().getCore(); - GHRateLimit.Record unknown1 = GHRateLimit.Unknown().getCore(); + GHRateLimit.UnknownLimitRecord.unknownLimitResetSeconds = 5; + GHRateLimit.UnknownLimitRecord.reset(); - 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); + GHRateLimit.Record unknown0 = GHRateLimit.Unknown("").getCore(); Thread.sleep(2000); + // To reduce object creation: There is only one valid Unknown record at a time. + assertThat("Unknown current should should limit the creation of new unknown records", + unknown0, + sameInstance(GHRateLimit.Unknown("").getCore())); + + // For testing, we create an new unknown. + GHRateLimit.Record unknown1 = new GHRateLimit.UnknownLimitRecord(); + + assertThat("Valid unknown should not replace an existing one, regardless of created or reset time", + unknown1.currentOrUpdated(unknown0), + sameInstance(unknown1)); + assertThat("Valid unknown should not replace an existing one, regardless of created or reset time", + unknown0.currentOrUpdated(unknown1), + sameInstance(unknown0)); + + // Sleep to make different created time + Thread.sleep(2000); + + long epochSeconds = Instant.now().getEpochSecond(); + + GHRateLimit.Record record0 = new GHRateLimit.Record(10, 10, epochSeconds + 10L); + GHRateLimit.Record record1 = new GHRateLimit.Record(10, 9, epochSeconds + 10L); + GHRateLimit.Record record2 = new GHRateLimit.Record(10, 2, epochSeconds + 10L); + GHRateLimit.Record record3 = new GHRateLimit.Record(10, 10, epochSeconds + 20L); + GHRateLimit.Record record4 = new GHRateLimit.Record(10, 5, epochSeconds + 20L); + GHRateLimit.Record recordExpired0 = new GHRateLimit.Record(10, 10, epochSeconds - 1L); + GHRateLimit.Record recordExpired1 = new GHRateLimit.Record(10, 10, epochSeconds + 2L); + + // Sleep to make expired and different created time + Thread.sleep(3000); + 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(); + GHRateLimit.Record record00 = new GHRateLimit.Record(10, 10, epochSeconds + 10L); + + GHRateLimit.Record unknownExpired0 = unknown0; + GHRateLimit.Record unknownExpired1 = unknown1; + unknown0 = GHRateLimit.Unknown("").getCore(); // Rate-limit records maybe created and returned in different orders. - // We should update to the regular records over unknowns. + // We should update to the unexpired 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", GitHubClient.shouldReplace(unknown0, unknown1), is(false)); - assertThat("Equivalent unknown should not replace", GitHubClient.shouldReplace(unknown1, unknown0), is(false)); + assertThat("Expired unknowns should not replace another expired one, regardless of created or reset time", + unknownExpired0.currentOrUpdated(unknownExpired1), + sameInstance(unknownExpired0)); + assertThat("Expired unknowns should not replace another expired one, regardless of created or reset time", + unknownExpired1.currentOrUpdated(unknownExpired0), + sameInstance(unknownExpired1)); - assertThat("Later unknown should replace earlier", GitHubClient.shouldReplace(unknown2, unknown0), is(true)); - assertThat("Earlier unknown should not replace later", - GitHubClient.shouldReplace(unknown0, unknown2), - is(false)); + assertThat( + "Expired unknown should not be replaced by expired earlier normal record, regardless of created or reset time", + unknownExpired0.currentOrUpdated(recordExpired0), + sameInstance(unknownExpired0)); + assertThat( + "Expired normal record should not be replace an expired earlier unknown record, regardless of created or reset time", + recordExpired0.currentOrUpdated(unknownExpired0), + sameInstance(recordExpired0)); - assertThat("Worst record should replace later unknown", - GitHubClient.shouldReplace(recordWorst, unknown1), - is(true)); - assertThat("Unknown should not replace worst record", - GitHubClient.shouldReplace(unknown1, recordWorst), - is(false)); + assertThat( + "Expired unknown should be replaced by expired later normal record, regardless of created or reset time", + unknownExpired0.currentOrUpdated(recordExpired1), + sameInstance(recordExpired1)); + assertThat( + "Expired later normal record should not be replace an expired unknown record, regardless of created or reset time", + recordExpired1.currentOrUpdated(unknownExpired0), + sameInstance(recordExpired1)); - assertThat("Earlier record should replace later worst", - GitHubClient.shouldReplace(record0, recordWorst), - is(true)); + assertThat("Valid unknown should not be replaced by an expired unknown", + unknown0.currentOrUpdated(unknownExpired0), + sameInstance(unknown0)); + assertThat("Expired unknown should be replaced by valid unknown", + unknownExpired0.currentOrUpdated(unknown0), + sameInstance(unknown0)); + + assertThat("Valid unknown should replace an expired normal record", + recordExpired1.currentOrUpdated(unknown0), + sameInstance(unknown0)); + assertThat("Expired normal record should not replace a valid unknown record", + unknown0.currentOrUpdated(recordExpired1), + sameInstance(unknown0)); + + // In normal comparision, expiration doesn't matter + assertThat("Expired normal should not be replaced by an earlier expired one", + recordExpired1.currentOrUpdated(recordExpired0), + sameInstance(recordExpired1)); + assertThat("Expired normal should be replaced by a later expired one", + recordExpired0.currentOrUpdated(recordExpired1), + sameInstance(recordExpired1)); + + assertThat("Later worst record should be replaced by earlier record", + recordWorst.currentOrUpdated(record0), + sameInstance(record0)); assertThat("Later worst record should not replace earlier", - GitHubClient.shouldReplace(recordWorst, record0), - is(false)); + record0.currentOrUpdated(recordWorst), + sameInstance(record0)); - assertThat("Equivalent record should not replace", GitHubClient.shouldReplace(record0, record00), is(false)); - assertThat("Equivalent record should not replace", GitHubClient.shouldReplace(record00, record0), is(false)); + assertThat("Equivalent record should not replace other", + record00.currentOrUpdated(record0), + sameInstance(record00)); + assertThat("Equivalent record should not replace other", + record0.currentOrUpdated(record00), + sameInstance(record0)); - assertThat("Lower limit record should replace higher", GitHubClient.shouldReplace(record1, record0), is(true)); - assertThat("Lower limit record should replace higher", GitHubClient.shouldReplace(record2, record1), is(true)); + assertThat("Higher limit record should be replaced by lower", + record0.currentOrUpdated(record1), + sameInstance(record1)); + assertThat("Higher limit record should be replaced by lower", + record1.currentOrUpdated(record2), + sameInstance(record2)); - assertThat("Higher limit record should not replace lower", - GitHubClient.shouldReplace(record1, record2), - is(false)); + assertThat("Lower limit record should not be replaced higher", + record2.currentOrUpdated(record1), + sameInstance(record2)); - assertThat("Higher limit record with later reset should replace lower", - GitHubClient.shouldReplace(record3, record2), - is(true)); + assertThat("Lower limit record should be replaced by higher limit record with later reset", + record2.currentOrUpdated(record3), + sameInstance(record3)); - assertThat("Lower limit record with later reset should replace higher", - GitHubClient.shouldReplace(record4, record1), - is(true)); + assertThat("Higher limit record should be replaced by lower limit record with later reset", + record1.currentOrUpdated(record4), + sameInstance(record4)); - assertThat("Lower limit record with earlier reset should not replace higher", - GitHubClient.shouldReplace(record2, record4), - is(false)); + assertThat("Higher limit record should not be replaced by lower limit record with earlier reset", + record4.currentOrUpdated(record2), + sameInstance(record4)); } diff --git a/src/test/java/org/kohsuke/github/RateLimitCheckerTest.java b/src/test/java/org/kohsuke/github/RateLimitCheckerTest.java index 20848ca7d..ff81d89bc 100644 --- a/src/test/java/org/kohsuke/github/RateLimitCheckerTest.java +++ b/src/test/java/org/kohsuke/github/RateLimitCheckerTest.java @@ -32,6 +32,7 @@ public class RateLimitCheckerTest extends AbstractGitHubWireMockTest { public void testGitHubRateLimit() throws Exception { // Customized response that templates the date to keep things working snapshotNotAllowed(); + GHRateLimit.UnknownLimitRecord.reset(); assertThat(mockGitHub.getRequestCount(), equalTo(0)); @@ -45,7 +46,7 @@ public class RateLimitCheckerTest extends AbstractGitHubWireMockTest { .withEndpoint(mockGitHub.apiServer().baseUrl()) .build(); - assertThat(gitHub.lastRateLimit(), nullValue()); + assertThat(gitHub.lastRateLimit(), equalTo(GHRateLimit.Default())); // Checks the rate limit before getting myself gitHub.getMyself();