From 875667469958be404ac222b94a45f2fd008bb1c0 Mon Sep 17 00:00:00 2001 From: Liam Newman Date: Sun, 7 Nov 2021 20:13:42 -0800 Subject: [PATCH] Introduce request and response interfaces for GitHubConnector --- pom.xml | 3 +- .../org/kohsuke/github/AbuseLimitHandler.java | 14 +- .../java/org/kohsuke/github/GHObject.java | 11 +- .../java/org/kohsuke/github/GHRateLimit.java | 9 +- src/main/java/org/kohsuke/github/GitHub.java | 5 + .../org/kohsuke/github/GitHubBuilder.java | 2 + .../java/org/kohsuke/github/GitHubClient.java | 155 +++++++------ .../org/kohsuke/github/GitHubConnector.java | 44 ---- .../kohsuke/github/GitHubPageIterator.java | 9 +- .../github/GitHubRateLimitChecker.java | 16 +- .../org/kohsuke/github/GitHubRequest.java | 13 +- .../org/kohsuke/github/GitHubResponse.java | 205 ++++-------------- ...bResponseInfoHttpURLConnectionAdapter.java | 43 ++-- .../org/kohsuke/github/HttpConnector.java | 2 + .../org/kohsuke/github/RateLimitHandler.java | 14 +- .../java/org/kohsuke/github/Requester.java | 20 +- .../github/connector/GitHubConnector.java | 29 +++ .../connector/GitHubConnectorRequest.java | 34 +++ .../connector/GitHubConnectorResponse.java | 120 ++++++++++ .../extras/okhttp3/OkHttpConnector.java | 13 +- .../kohsuke/github/function/BodyHandler.java | 15 ++ .../internal/DefaultGitHubConnector.java | 22 ++ .../GitHubConnectorHttpConnectorAdapter.java | 30 +-- .../org/kohsuke/github/GitHubStaticTest.java | 4 +- ...89e60064bfa43e374baeb10846f7ce82f40-4.json | 2 +- ...89e60064bfa43e374baeb10846f7ce82f40-4.json | 2 +- ...89e60064bfa43e374baeb10846f7ce82f40-5.json | 2 +- 27 files changed, 468 insertions(+), 370 deletions(-) delete mode 100644 src/main/java/org/kohsuke/github/GitHubConnector.java create mode 100644 src/main/java/org/kohsuke/github/connector/GitHubConnector.java create mode 100644 src/main/java/org/kohsuke/github/connector/GitHubConnectorRequest.java create mode 100644 src/main/java/org/kohsuke/github/connector/GitHubConnectorResponse.java create mode 100644 src/main/java/org/kohsuke/github/function/BodyHandler.java create mode 100644 src/main/java/org/kohsuke/github/internal/DefaultGitHubConnector.java rename src/main/java/org/kohsuke/github/{ => internal}/GitHubConnectorHttpConnectorAdapter.java (85%) diff --git a/pom.xml b/pom.xml index 181035062b..6ae3116492 100644 --- a/pom.xml +++ b/pom.xml @@ -266,7 +266,6 @@ - maven-surefire-plugin @@ -579,7 +578,7 @@ src/test/resources/slow-or-flaky-tests.txt - @{jacoco.surefire.argLine} ${surefire.argLine} -Dtest.github.useOkHttp + @{jacoco.surefire.argLine} ${surefire.argLine} -Dtest.github.connector=okhttp diff --git a/src/main/java/org/kohsuke/github/AbuseLimitHandler.java b/src/main/java/org/kohsuke/github/AbuseLimitHandler.java index ab2761b8c1..bd4c1c17f8 100644 --- a/src/main/java/org/kohsuke/github/AbuseLimitHandler.java +++ b/src/main/java/org/kohsuke/github/AbuseLimitHandler.java @@ -1,5 +1,7 @@ package org.kohsuke.github; +import org.kohsuke.github.connector.GitHubConnectorResponse; + import java.io.IOException; import java.io.InterruptedIOException; import java.net.HttpURLConnection; @@ -22,7 +24,7 @@ public abstract class AbuseLimitHandler { * an exception. If this method returns normally, another request will be attempted. For that to make sense, the * implementation needs to wait for some time. * - * @param responseInfo + * @param connectorResponse * Response information for this request. * @throws IOException * on failure @@ -32,12 +34,12 @@ public abstract class AbuseLimitHandler { * with abuse rate limits * */ - void onError(GitHubResponse.ResponseInfo responseInfo) throws IOException { + void onError(GitHubConnectorResponse connectorResponse) throws IOException { GHIOException e = new HttpException("Abuse limit violation", - responseInfo.statusCode(), - responseInfo.header("Status"), - responseInfo.url().toString()).withResponseHeaderFields(responseInfo.allHeaders()); - onError(e, new GitHubResponseInfoHttpURLConnectionAdapter(responseInfo)); + connectorResponse.statusCode(), + connectorResponse.header("Status"), + connectorResponse.url().toString()).withResponseHeaderFields(connectorResponse.allHeaders()); + onError(e, new GitHubResponseInfoHttpURLConnectionAdapter(connectorResponse)); } /** diff --git a/src/main/java/org/kohsuke/github/GHObject.java b/src/main/java/org/kohsuke/github/GHObject.java index 9bdbb3c758..7d89cd36db 100644 --- a/src/main/java/org/kohsuke/github/GHObject.java +++ b/src/main/java/org/kohsuke/github/GHObject.java @@ -5,6 +5,7 @@ import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import org.apache.commons.lang3.builder.ReflectionToStringBuilder; import org.apache.commons.lang3.builder.ToStringStyle; +import org.kohsuke.github.connector.GitHubConnectorResponse; import java.io.IOException; import java.lang.reflect.Field; @@ -39,13 +40,13 @@ public abstract class GHObject extends GitHubInteractiveObject { /** * Called by Jackson * - * @param responseInfo - * the {@link GitHubResponse.ResponseInfo} to get headers from. + * @param connectorResponse + * the {@link GitHubConnectorResponse} to get headers from. */ @JacksonInject - protected void setResponseHeaderFields(@CheckForNull GitHubResponse.ResponseInfo responseInfo) { - if (responseInfo != null) { - responseHeaderFields = responseInfo.allHeaders(); + protected void setResponseHeaderFields(@CheckForNull GitHubConnectorResponse connectorResponse) { + if (connectorResponse != null) { + responseHeaderFields = connectorResponse.allHeaders(); } } diff --git a/src/main/java/org/kohsuke/github/GHRateLimit.java b/src/main/java/org/kohsuke/github/GHRateLimit.java index 59e9f90d3b..0177e4a2b0 100644 --- a/src/main/java/org/kohsuke/github/GHRateLimit.java +++ b/src/main/java/org/kohsuke/github/GHRateLimit.java @@ -5,6 +5,7 @@ import com.fasterxml.jackson.annotation.JsonProperty; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import org.apache.commons.lang3.StringUtils; +import org.kohsuke.github.connector.GitHubConnectorResponse; import java.time.Duration; import java.time.ZonedDateTime; @@ -436,20 +437,20 @@ public Record(@JsonProperty(value = "limit", required = true) int limit, * the remaining * @param resetEpochSeconds * the reset epoch seconds - * @param responseInfo + * @param connectorResponse * the response info */ @JsonCreator Record(@JsonProperty(value = "limit", required = true) int limit, @JsonProperty(value = "remaining", required = true) int remaining, @JsonProperty(value = "reset", required = true) long resetEpochSeconds, - @JacksonInject @CheckForNull GitHubResponse.ResponseInfo responseInfo) { + @JacksonInject @CheckForNull GitHubConnectorResponse connectorResponse) { this.limit = limit; this.remaining = remaining; this.resetEpochSeconds = resetEpochSeconds; String updatedAt = null; - if (responseInfo != null) { - updatedAt = responseInfo.header("Date"); + if (connectorResponse != null) { + updatedAt = connectorResponse.header("Date"); } this.resetDate = calculateResetDate(updatedAt); } diff --git a/src/main/java/org/kohsuke/github/GitHub.java b/src/main/java/org/kohsuke/github/GitHub.java index 932895ebe6..bba6c8889d 100644 --- a/src/main/java/org/kohsuke/github/GitHub.java +++ b/src/main/java/org/kohsuke/github/GitHub.java @@ -30,6 +30,8 @@ import org.kohsuke.github.authorization.AuthorizationProvider; import org.kohsuke.github.authorization.ImmutableAuthorizationProvider; import org.kohsuke.github.authorization.UserAuthorizationProvider; +import org.kohsuke.github.connector.GitHubConnector; +import org.kohsuke.github.internal.GitHubConnectorHttpConnectorAdapter; import org.kohsuke.github.internal.Previews; import java.io.*; @@ -470,7 +472,10 @@ public boolean isOffline() { * Gets connector. * * @return the connector + * @deprecated HttpConnector has been replaced by GitHubConnector which is generally not useful outside of this + * library. If you are using */ + @Deprecated public HttpConnector getConnector() { return client.getConnector(); } diff --git a/src/main/java/org/kohsuke/github/GitHubBuilder.java b/src/main/java/org/kohsuke/github/GitHubBuilder.java index c9d4d20fcb..4d9135f19f 100644 --- a/src/main/java/org/kohsuke/github/GitHubBuilder.java +++ b/src/main/java/org/kohsuke/github/GitHubBuilder.java @@ -3,7 +3,9 @@ import org.apache.commons.io.IOUtils; import org.kohsuke.github.authorization.AuthorizationProvider; import org.kohsuke.github.authorization.ImmutableAuthorizationProvider; +import org.kohsuke.github.connector.GitHubConnector; import org.kohsuke.github.extras.ImpatientHttpConnector; +import org.kohsuke.github.internal.GitHubConnectorHttpConnectorAdapter; import java.io.File; import java.io.FileInputStream; diff --git a/src/main/java/org/kohsuke/github/GitHubClient.java b/src/main/java/org/kohsuke/github/GitHubClient.java index 073b9c1c8a..48ebed0af1 100644 --- a/src/main/java/org/kohsuke/github/GitHubClient.java +++ b/src/main/java/org/kohsuke/github/GitHubClient.java @@ -5,6 +5,9 @@ import org.apache.commons.io.IOUtils; import org.kohsuke.github.authorization.AuthorizationProvider; import org.kohsuke.github.authorization.UserAuthorizationProvider; +import org.kohsuke.github.connector.GitHubConnector; +import org.kohsuke.github.connector.GitHubConnectorResponse; +import org.kohsuke.github.function.BodyHandler; import java.io.*; import java.net.*; @@ -109,7 +112,7 @@ String getLogin() { private T fetch(Class type, String urlPath) throws IOException { GitHubRequest request = GitHubRequest.newBuilder().withApiUrl(getApiUrl()).withUrlPath(urlPath).build(); - return sendRequest(request, (responseInfo) -> GitHubResponse.parseBody(responseInfo, type)).body(); + return sendRequest(request, (connectorResponse) -> GitHubResponse.parseBody(connectorResponse, type)).body(); } /** @@ -137,7 +140,7 @@ public boolean isCredentialValid() { * @return {@code true} if this is an always offline "connection". */ public boolean isOffline() { - return getConnector() == GitHubConnector.OFFLINE; + return connector == GitHubConnector.OFFLINE; } /** @@ -145,8 +148,15 @@ public boolean isOffline() { * * @return the connector */ + @Deprecated public HttpConnector getConnector() { - return connector; + if (!(connector instanceof HttpConnector)) { + throw new UnsupportedOperationException("This GitHubConnector does not support HttpConnector.connect()."); + } + + LOGGER.warning( + "HttpConnector and getConnector() are deprecated. " + "Please file an issue describing your use case."); + return (HttpConnector) connector; } /** @@ -197,7 +207,7 @@ public GHRateLimit getRateLimit() throws IOException { } @CheckForNull - protected String getEncodedAuthorization() throws IOException { + String getEncodedAuthorization() throws IOException { return authorizationProvider.getEncodedAuthorization(); } @@ -211,7 +221,8 @@ GHRateLimit getRateLimit(@Nonnull RateLimitTarget rateLimitTarget) throws IOExce .withUrlPath("/rate_limit") .build(); result = this - .sendRequest(request, (responseInfo) -> GitHubResponse.parseBody(responseInfo, JsonRateLimit.class)) + .sendRequest(request, + (connectorResponse) -> GitHubResponse.parseBody(connectorResponse, JsonRateLimit.class)) .body().resources; } catch (FileNotFoundException e) { // For some versions of GitHub Enterprise, the rate_limit endpoint returns a 404. @@ -318,9 +329,8 @@ public String getApiUrl() { } /** - * Builds a {@link GitHubRequest}, sends the {@link GitHubRequest} to the server, and uses the - * {@link GitHubResponse.BodyHandler} to parse the response info and response body data into an instance of - * {@link T}. + * Builds a {@link GitHubRequest}, sends the {@link GitHubRequest} to the server, and uses the {@link BodyHandler} + * to parse the response info and response body data into an instance of {@link T}. * * @param builder * used to build the request that will be sent to the server. @@ -335,13 +345,13 @@ public String getApiUrl() { */ @Nonnull public GitHubResponse sendRequest(@Nonnull GitHubRequest.Builder builder, - @CheckForNull GitHubResponse.BodyHandler handler) throws IOException { + @CheckForNull BodyHandler handler) throws IOException { return sendRequest(builder.build(), handler); } /** - * Sends the {@link GitHubRequest} to the server, and uses the {@link GitHubResponse.BodyHandler} to parse the - * response info and response body data into an instance of {@link T}. + * Sends the {@link GitHubRequest} to the server, and uses the {@link BodyHandler} to parse the response info and + * response body data into an instance of {@link T}. * * @param request * the request that will be sent to the server. @@ -355,31 +365,31 @@ public GitHubResponse sendRequest(@Nonnull GitHubRequest.Builder build * if an I/O Exception occurs */ @Nonnull - public GitHubResponse sendRequest(GitHubRequest request, @CheckForNull GitHubResponse.BodyHandler handler) + public GitHubResponse sendRequest(GitHubRequest request, @CheckForNull BodyHandler handler) throws IOException { int retries = CONNECTION_ERROR_RETRIES; request = prepareRequest(request); do { // if we fail to create a connection we do not retry and we do not wrap - GitHubResponse.ResponseInfo responseInfo = null; + GitHubConnectorResponse connectorResponse = null; try { try { logRequest(request); - rateLimitChecker.checkRateLimit(this, request); - responseInfo = connector.send(request); - noteRateLimit(responseInfo); - detectOTPRequired(responseInfo); + rateLimitChecker.checkRateLimit(this, request.rateLimitTarget()); + connectorResponse = connector.send(request); + noteRateLimit(request.rateLimitTarget(), connectorResponse); + detectOTPRequired(connectorResponse); - if (isInvalidCached404Response(responseInfo)) { + if (isInvalidCached404Response(connectorResponse)) { // Setting "Cache-Control" to "no-cache" stops the cache from supplying // "If-Modified-Since" or "If-None-Match" values. // This makes GitHub give us current data (not incorrectly cached data) request = request.toBuilder().setHeader("Cache-Control", "no-cache").build(); continue; } - if (!(isRateLimitResponse(responseInfo) || isAbuseLimitResponse(responseInfo))) { - return createResponse(responseInfo, handler); + if (!(isRateLimitResponse(connectorResponse) || isAbuseLimitResponse(connectorResponse))) { + return createResponse(connectorResponse, handler); } } catch (IOException e) { // For transient errors, retry @@ -387,12 +397,12 @@ public GitHubResponse sendRequest(GitHubRequest request, @CheckForNull Gi continue; } - throw interpretApiError(e, request, responseInfo); + throw interpretApiError(e, request, connectorResponse); } - handleLimitingErrors(responseInfo); + handleLimitingErrors(connectorResponse); } finally { - IOUtils.closeQuietly(responseInfo); + IOUtils.closeQuietly(connectorResponse); } } while (--retries >= 0); @@ -415,7 +425,7 @@ private GitHubRequest prepareRequest(GitHubRequest request) throws IOException { } builder.setHeader("Accept-Encoding", "gzip"); - if (request.inBody()) { + if (request.hasBody()) { if (request.body() != null) { builder.contentType(defaultString(request.contentType(), "application/x-www-form-urlencoded")); } else { @@ -438,21 +448,21 @@ private void logRequest(@Nonnull final GitHubRequest request) { + request.method() + " " + request.url().toString()); } - private void handleLimitingErrors(@Nonnull GitHubResponse.ResponseInfo responseInfo) throws IOException { - if (isRateLimitResponse(responseInfo)) { - rateLimitHandler.onError(responseInfo); - } else if (isAbuseLimitResponse(responseInfo)) { - abuseLimitHandler.onError(responseInfo); + private void handleLimitingErrors(@Nonnull GitHubConnectorResponse connectorResponse) throws IOException { + if (isRateLimitResponse(connectorResponse)) { + rateLimitHandler.onError(connectorResponse); + } else if (isAbuseLimitResponse(connectorResponse)) { + abuseLimitHandler.onError(connectorResponse); } } @Nonnull - private static GitHubResponse createResponse(@Nonnull GitHubResponse.ResponseInfo responseInfo, - @CheckForNull GitHubResponse.BodyHandler handler) throws IOException { + private static GitHubResponse createResponse(@Nonnull GitHubConnectorResponse connectorResponse, + @CheckForNull BodyHandler handler) throws IOException { T body = null; - if (responseInfo.statusCode() == HttpURLConnection.HTTP_NOT_MODIFIED) { + if (connectorResponse.statusCode() == HttpURLConnection.HTTP_NOT_MODIFIED) { // special case handling for 304 unmodified, as the content will be "" - } else if (responseInfo.statusCode() == HttpURLConnection.HTTP_ACCEPTED) { + } else if (connectorResponse.statusCode() == HttpURLConnection.HTTP_ACCEPTED) { // Response code 202 means data is being generated or an action that can require some time is triggered. // This happens in specific cases: @@ -461,12 +471,12 @@ private static GitHubResponse createResponse(@Nonnull GitHubResponse.Resp // workflow run cancellation - See https://docs.github.com/en/rest/reference/actions#cancel-a-workflow-run LOGGER.log(FINE, - "Received HTTP_ACCEPTED(202) from " + responseInfo.url().toString() + "Received HTTP_ACCEPTED(202) from " + connectorResponse.url().toString() + " . Please try again in 5 seconds."); } else if (handler != null) { - body = handler.apply(responseInfo); + body = handler.apply(connectorResponse); } - return new GitHubResponse<>(responseInfo, body); + return new GitHubResponse<>(connectorResponse, body); } /** @@ -474,7 +484,7 @@ private static GitHubResponse createResponse(@Nonnull GitHubResponse.Resp */ private static IOException interpretApiError(IOException e, @Nonnull GitHubRequest request, - @CheckForNull GitHubResponse.ResponseInfo responseInfo) throws IOException { + @CheckForNull GitHubConnectorResponse connectorResponse) throws IOException { // If we're already throwing a GHIOException, pass through if (e instanceof GHIOException) { return e; @@ -485,11 +495,11 @@ private static IOException interpretApiError(IOException e, Map> headers = new HashMap<>(); String errorMessage = null; - if (responseInfo != null) { - statusCode = responseInfo.statusCode(); - message = responseInfo.header("Status"); - headers = responseInfo.allHeaders(); - errorMessage = responseInfo.errorMessage(); + if (connectorResponse != null) { + statusCode = connectorResponse.statusCode(); + message = connectorResponse.header("Status"); + headers = connectorResponse.allHeaders(); + errorMessage = connectorResponse.errorMessage(); } if (errorMessage != null) { @@ -508,14 +518,14 @@ private static IOException interpretApiError(IOException e, return e; } - protected static boolean isRateLimitResponse(@Nonnull GitHubResponse.ResponseInfo responseInfo) { - return responseInfo.statusCode() == HttpURLConnection.HTTP_FORBIDDEN - && "0".equals(responseInfo.header("X-RateLimit-Remaining")); + protected static boolean isRateLimitResponse(@Nonnull GitHubConnectorResponse connectorResponse) { + return connectorResponse.statusCode() == HttpURLConnection.HTTP_FORBIDDEN + && "0".equals(connectorResponse.header("X-RateLimit-Remaining")); } - protected static boolean isAbuseLimitResponse(@Nonnull GitHubResponse.ResponseInfo responseInfo) { - return responseInfo.statusCode() == HttpURLConnection.HTTP_FORBIDDEN - && responseInfo.header("Retry-After") != null; + protected static boolean isAbuseLimitResponse(@Nonnull GitHubConnectorResponse connectorResponse) { + return connectorResponse.statusCode() == HttpURLConnection.HTTP_FORBIDDEN + && connectorResponse.header("Retry-After") != null; } private static boolean retryConnectionError(IOException e, URL url, int retries) throws IOException { @@ -536,7 +546,7 @@ private static boolean retryConnectionError(IOException e, URL url, int retries) return false; } - private static boolean isInvalidCached404Response(GitHubResponse.ResponseInfo responseInfo) { + private static boolean isInvalidCached404Response(GitHubConnectorResponse connectorResponse) { // WORKAROUND FOR ISSUE #669: // When the Requester detects a 404 response with an ETag (only happpens when the server's 304 // is bogus and would cause cache corruption), try the query again with new request header @@ -547,44 +557,45 @@ private static boolean isInvalidCached404Response(GitHubResponse.ResponseInfo re // scenarios. If GitHub ever fixes their issue and/or begins providing accurate ETags to // their 404 responses, this will result in at worst two requests being made for each 404 // responses. However, only the second request will count against rate limit. - if (responseInfo.statusCode() == 404 && Objects.equals(responseInfo.request().method(), "GET") - && responseInfo.header("ETag") != null - && !Objects.equals(responseInfo.request().header("Cache-Control"), "no-cache")) { + if (connectorResponse.statusCode() == 404 && Objects.equals(connectorResponse.request().method(), "GET") + && connectorResponse.header("ETag") != null + && !Objects.equals(connectorResponse.request().header("Cache-Control"), "no-cache")) { LOGGER.log(FINE, - "Encountered GitHub invalid cached 404 from " + responseInfo.url() + "Encountered GitHub invalid cached 404 from " + connectorResponse.url() + ". Retrying with \"Cache-Control\"=\"no-cache\"..."); return true; } return false; } - private void noteRateLimit(@Nonnull GitHubResponse.ResponseInfo responseInfo) { + private void noteRateLimit(@Nonnull RateLimitTarget rateLimitTarget, + @Nonnull GitHubConnectorResponse connectorResponse) { try { - String limitString = Objects.requireNonNull(responseInfo.header("X-RateLimit-Limit"), + String limitString = Objects.requireNonNull(connectorResponse.header("X-RateLimit-Limit"), "Missing X-RateLimit-Limit"); - String remainingString = Objects.requireNonNull(responseInfo.header("X-RateLimit-Remaining"), + String remainingString = Objects.requireNonNull(connectorResponse.header("X-RateLimit-Remaining"), "Missing X-RateLimit-Remaining"); - String resetString = Objects.requireNonNull(responseInfo.header("X-RateLimit-Reset"), + String resetString = Objects.requireNonNull(connectorResponse.header("X-RateLimit-Reset"), "Missing X-RateLimit-Reset"); int limit, remaining; long reset; limit = Integer.parseInt(limitString); remaining = Integer.parseInt(remainingString); reset = Long.parseLong(resetString); - GHRateLimit.Record observed = new GHRateLimit.Record(limit, remaining, reset, responseInfo); - updateRateLimit(GHRateLimit.fromRecord(observed, responseInfo.request().rateLimitTarget())); + GHRateLimit.Record observed = new GHRateLimit.Record(limit, remaining, reset, connectorResponse); + updateRateLimit(GHRateLimit.fromRecord(observed, rateLimitTarget)); } catch (NumberFormatException | NullPointerException e) { LOGGER.log(FINEST, "Missing or malformed X-RateLimit header: ", e); } } - private static void detectOTPRequired(@Nonnull GitHubResponse.ResponseInfo responseInfo) throws GHIOException { + private static void detectOTPRequired(@Nonnull GitHubConnectorResponse connectorResponse) throws GHIOException { // 401 Unauthorized == bad creds or OTP request - if (responseInfo.statusCode() == HTTP_UNAUTHORIZED) { + if (connectorResponse.statusCode() == HTTP_UNAUTHORIZED) { // In the case of a user with 2fa enabled, a header with X-GitHub-OTP // will be returned indicating the user needs to respond with an otp - if (responseInfo.header("X-GitHub-OTP") != null) { - throw new GHOTPRequiredException().withResponseHeaderFields(responseInfo.allHeaders()); + if (connectorResponse.header("X-GitHub-OTP") != null) { + throw new GHOTPRequiredException().withResponseHeaderFields(connectorResponse.allHeaders()); } } } @@ -686,7 +697,7 @@ static ObjectWriter getMappingObjectWriter() { } /** - * Helper for {@link #getMappingObjectReader(GitHubResponse.ResponseInfo)} + * Helper for {@link #getMappingObjectReader(GitHubConnectorResponse)} * * @param root * the root GitHub object for this reader @@ -695,7 +706,7 @@ static ObjectWriter getMappingObjectWriter() { */ @Nonnull static ObjectReader getMappingObjectReader(@Nonnull GitHub root) { - ObjectReader reader = getMappingObjectReader((GitHubResponse.ResponseInfo) null); + ObjectReader reader = getMappingObjectReader((GitHubConnectorResponse) null); ((InjectableValues.Std) reader.getInjectableValues()).addValue(GitHub.class, root); return reader; } @@ -709,22 +720,22 @@ static ObjectReader getMappingObjectReader(@Nonnull GitHub root) { * Having one spot to create readers and having it take all injectable values is not a great long term solution but * it is sufficient for this first cut. * - * @param responseInfo - * the {@link GitHubResponse.ResponseInfo} to inject for this reader. + * @param connectorResponse + * the {@link GitHubConnectorResponse} to inject for this reader. * * @return an {@link ObjectReader} instance that can be further configured. */ @Nonnull - static ObjectReader getMappingObjectReader(@CheckForNull GitHubResponse.ResponseInfo responseInfo) { + static ObjectReader getMappingObjectReader(@CheckForNull GitHubConnectorResponse connectorResponse) { Map injected = new HashMap<>(); // Required or many things break - injected.put(GitHubResponse.ResponseInfo.class.getName(), null); + injected.put(GitHubConnectorResponse.class.getName(), null); injected.put(GitHub.class.getName(), null); - if (responseInfo != null) { - injected.put(GitHubResponse.ResponseInfo.class.getName(), responseInfo); - injected.putAll(responseInfo.request().injectedMappingValues()); + if (connectorResponse != null) { + injected.put(GitHubConnectorResponse.class.getName(), connectorResponse); + injected.putAll(connectorResponse.request().injectedMappingValues()); } return MAPPER.reader(new InjectableValues.Std(injected)); } diff --git a/src/main/java/org/kohsuke/github/GitHubConnector.java b/src/main/java/org/kohsuke/github/GitHubConnector.java deleted file mode 100644 index 9cd6519206..0000000000 --- a/src/main/java/org/kohsuke/github/GitHubConnector.java +++ /dev/null @@ -1,44 +0,0 @@ -package org.kohsuke.github; - -import okhttp3.OkHttpClient; -import org.kohsuke.github.extras.okhttp3.OkHttpConnector; - -import java.io.IOException; -import java.net.HttpURLConnection; -import java.net.URL; - -/** - * Pluggability for customizing HTTP request behaviors or using altogether different library. - * - *

- * For example, you can implement this to st custom timeouts. - * - * @author Kohsuke Kawaguchi - */ -@FunctionalInterface -public interface GitHubConnector extends HttpConnector { - /** - * Opens a connection to the given URL. - * - * @param url - * the url - * @return the http url connection - * @throws IOException - * the io exception - */ - @Deprecated - default HttpURLConnection connect(URL url) throws IOException { - throw new UnsupportedOperationException(); - } - - GitHubResponse.ResponseInfo send(GitHubRequest request) throws IOException; - - /** - * Default implementation that uses {@link URL#openConnection()}. - */ - GitHubConnector DEFAULT = (System.getProperty("test.github.useOkHttp", "false") != "false") - ? new OkHttpConnector(new OkHttpClient.Builder().build()) - : new GitHubConnectorHttpConnectorAdapter(HttpConnector.DEFAULT); - - GitHubConnector OFFLINE = new GitHubConnectorHttpConnectorAdapter(HttpConnector.OFFLINE); -} diff --git a/src/main/java/org/kohsuke/github/GitHubPageIterator.java b/src/main/java/org/kohsuke/github/GitHubPageIterator.java index 24975f4dff..476f5cb3cf 100644 --- a/src/main/java/org/kohsuke/github/GitHubPageIterator.java +++ b/src/main/java/org/kohsuke/github/GitHubPageIterator.java @@ -138,10 +138,10 @@ private void fetch() { URL url = nextRequest.url(); try { GitHubResponse nextResponse = client.sendRequest(nextRequest, - (responseInfo) -> GitHubResponse.parseBody(responseInfo, type)); + (connectorResponse) -> GitHubResponse.parseBody(connectorResponse, type)); assert nextResponse.body() != null; next = nextResponse.body(); - nextRequest = findNextURL(nextResponse); + nextRequest = findNextURL(nextRequest, nextResponse); if (nextRequest == null) { finalResponse = nextResponse; } @@ -155,7 +155,8 @@ private void fetch() { /** * Locate the next page from the pagination "Link" tag. */ - private GitHubRequest findNextURL(GitHubResponse nextResponse) throws MalformedURLException { + private GitHubRequest findNextURL(GitHubRequest nextRequest, GitHubResponse nextResponse) + throws MalformedURLException { GitHubRequest result = null; String link = nextResponse.header("Link"); if (link != null) { @@ -164,7 +165,7 @@ private GitHubRequest findNextURL(GitHubResponse nextResponse) throws Malform // found the next page. This should look something like // ; rel="next" int idx = token.indexOf('>'); - result = nextResponse.request().toBuilder().setRawUrlPath(token.substring(1, idx)).build(); + result = nextRequest.toBuilder().setRawUrlPath(token.substring(1, idx)).build(); break; } } diff --git a/src/main/java/org/kohsuke/github/GitHubRateLimitChecker.java b/src/main/java/org/kohsuke/github/GitHubRateLimitChecker.java index 5e5d998870..859baf0ce5 100644 --- a/src/main/java/org/kohsuke/github/GitHubRateLimitChecker.java +++ b/src/main/java/org/kohsuke/github/GitHubRateLimitChecker.java @@ -107,20 +107,20 @@ GitHubRateLimitChecker with(@Nonnull RateLimitChecker checker, @Nonnull RateLimi * * @param client * the {@link GitHubClient} to check - * @param request - * the {@link GitHubRequest} to check against + * @param rateLimitTarget + * the {@link RateLimitTarget} to check against * @throws IOException * if there is an I/O error */ - void checkRateLimit(GitHubClient client, GitHubRequest request) throws IOException { - RateLimitChecker guard = selectChecker(request.rateLimitTarget()); + void checkRateLimit(GitHubClient client, @Nonnull RateLimitTarget rateLimitTarget) throws IOException { + RateLimitChecker guard = selectChecker(rateLimitTarget); if (guard == RateLimitChecker.NONE) { return; } // For the first rate limit, accept the current limit if a valid one is already present. - GHRateLimit rateLimit = client.rateLimit(request.rateLimitTarget()); - GHRateLimit.Record rateLimitRecord = rateLimit.getRecord(request.rateLimitTarget()); + GHRateLimit rateLimit = client.rateLimit(rateLimitTarget); + GHRateLimit.Record rateLimitRecord = rateLimit.getRecord(rateLimitTarget); long waitCount = 0; try { while (guard.checkRateLimit(rateLimitRecord, waitCount)) { @@ -133,8 +133,8 @@ void checkRateLimit(GitHubClient client, GitHubRequest request) throws IOExcepti Thread.sleep(1000); // After the first wait, always request a new rate limit from the server. - rateLimit = client.getRateLimit(request.rateLimitTarget()); - rateLimitRecord = rateLimit.getRecord(request.rateLimitTarget()); + rateLimit = client.getRateLimit(rateLimitTarget); + rateLimitRecord = rateLimit.getRecord(rateLimitTarget); } } catch (InterruptedException e) { throw (IOException) new InterruptedIOException(e.getMessage()).initCause(e); diff --git a/src/main/java/org/kohsuke/github/GitHubRequest.java b/src/main/java/org/kohsuke/github/GitHubRequest.java index e7b17d7fbb..5430c1b0f9 100644 --- a/src/main/java/org/kohsuke/github/GitHubRequest.java +++ b/src/main/java/org/kohsuke/github/GitHubRequest.java @@ -4,6 +4,7 @@ import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import org.apache.commons.io.IOUtils; import org.apache.commons.lang3.StringUtils; +import org.kohsuke.github.connector.GitHubConnectorRequest; import org.kohsuke.github.internal.Previews; import java.io.ByteArrayInputStream; @@ -33,7 +34,7 @@ * not specified until late in the building process, so this is still untyped. *

*/ -public class GitHubRequest { +public class GitHubRequest implements GitHubConnectorRequest { private static final Comparator nullableCaseInsensitiveComparator = Comparator .nullsFirst(String.CASE_INSENSITIVE_ORDER); @@ -130,6 +131,7 @@ static String transformEnum(Enum en) { * * @return the request method. */ + @Override @Nonnull public String method() { return method; @@ -162,6 +164,7 @@ public List args() { * * @return the {@link Map} of headers */ + @Override @SuppressFBWarnings(value = { "EI_EXPOSE_REP" }, justification = "Unmodifiable Map of unmodifiable lists") @Nonnull public Map> allHeaders() { @@ -221,6 +224,7 @@ public String urlPath() { * * @return the content type. */ + @Override public String contentType() { return header("Content-type"); } @@ -230,6 +234,7 @@ public String contentType() { * * @return the {@link InputStream}. */ + @Override @CheckForNull public InputStream body() { return body != null ? new ByteArrayInputStream(body) : null; @@ -240,6 +245,7 @@ public InputStream body() { * * @return the request {@link URL} */ + @Override @Nonnull public URL url() { return url; @@ -250,7 +256,8 @@ public URL url() { * * @return true if the arguements should be sent in the body of the request. */ - public boolean inBody() { + @Override + public boolean hasBody() { return forceBody || !METHODS_WITHOUT_BODY.contains(method); } @@ -274,7 +281,7 @@ Builder toBuilder() { private String buildTailApiUrl() { String tailApiUrl = urlPath; - if (!inBody() && !args.isEmpty() && tailApiUrl.startsWith("/")) { + if (!hasBody() && !args.isEmpty() && tailApiUrl.startsWith("/")) { try { StringBuilder argString = new StringBuilder(); boolean questionMarkFound = tailApiUrl.indexOf('?') != -1; diff --git a/src/main/java/org/kohsuke/github/GitHubResponse.java b/src/main/java/org/kohsuke/github/GitHubResponse.java index c93596d679..126ee9e89e 100644 --- a/src/main/java/org/kohsuke/github/GitHubResponse.java +++ b/src/main/java/org/kohsuke/github/GitHubResponse.java @@ -4,11 +4,10 @@ import com.fasterxml.jackson.databind.InjectableValues; import com.fasterxml.jackson.databind.JsonMappingException; import org.apache.commons.io.IOUtils; -import org.kohsuke.github.function.FunctionThrows; +import org.kohsuke.github.connector.GitHubConnectorRequest; +import org.kohsuke.github.connector.GitHubConnectorResponse; -import java.io.Closeable; import java.io.IOException; -import java.io.InputStream; import java.io.InputStreamReader; import java.lang.reflect.Array; import java.net.HttpURLConnection; @@ -28,16 +27,16 @@ *

* * @param - * the type of the data parsed from the body of a {@link ResponseInfo}. + * the type of the data parsed from the body of a {@link GitHubConnectorResponse}. */ -public class GitHubResponse { +class GitHubResponse { private static final Logger LOGGER = Logger.getLogger(GitHubResponse.class.getName()); private final int statusCode; @Nonnull - private final GitHubRequest request; + private final GitHubConnectorRequest request; @Nonnull private final Map> headers; @@ -52,17 +51,17 @@ public class GitHubResponse { this.body = body; } - GitHubResponse(ResponseInfo responseInfo, @CheckForNull T body) { - this.statusCode = responseInfo.statusCode(); - this.request = responseInfo.request(); - this.headers = responseInfo.allHeaders(); + GitHubResponse(GitHubConnectorResponse connectorResponse, @CheckForNull T body) { + this.statusCode = connectorResponse.statusCode(); + this.request = connectorResponse.request(); + this.headers = connectorResponse.allHeaders(); this.body = body; } /** - * Parses a {@link ResponseInfo} body into a new instance of {@link T}. + * Parses a {@link GitHubConnectorResponse} body into a new instance of {@link T}. * - * @param responseInfo + * @param connectorResponse * response info to parse. * @param type * the type to be constructed. @@ -73,9 +72,9 @@ public class GitHubResponse { * if there is an I/O Exception. */ @CheckForNull - static T parseBody(ResponseInfo responseInfo, Class type) throws IOException { + static T parseBody(GitHubConnectorResponse connectorResponse, Class type) throws IOException { - if (responseInfo.statusCode() == HttpURLConnection.HTTP_NO_CONTENT) { + if (connectorResponse.statusCode() == HttpURLConnection.HTTP_NO_CONTENT) { if (type != null && type.isArray()) { // no content for array should be empty array return type.cast(Array.newInstance(type.getComponentType(), 0)); @@ -85,12 +84,12 @@ static T parseBody(ResponseInfo responseInfo, Class type) throws IOExcept } } - String data = responseInfo.getBodyAsString(); + String data = getBodyAsString(connectorResponse); try { InjectableValues.Std inject = new InjectableValues.Std(); - inject.addValue(ResponseInfo.class, responseInfo); + inject.addValue(GitHubConnectorResponse.class, connectorResponse); - return GitHubClient.getMappingObjectReader(responseInfo).forType(type).readValue(data); + return GitHubClient.getMappingObjectReader(connectorResponse).forType(type).readValue(data); } catch (JsonMappingException | JsonParseException e) { String message = "Failed to deserialize: " + data; LOGGER.log(Level.FINE, message); @@ -99,9 +98,9 @@ static T parseBody(ResponseInfo responseInfo, Class type) throws IOExcept } /** - * Parses a {@link ResponseInfo} body into a new instance of {@link T}. + * Parses a {@link GitHubConnectorResponse} body into a new instance of {@link T}. * - * @param responseInfo + * @param connectorResponse * response info to parse. * @param instance * the object to fill with data parsed from body @@ -112,11 +111,11 @@ static T parseBody(ResponseInfo responseInfo, Class type) throws IOExcept * if there is an I/O Exception. */ @CheckForNull - static T parseBody(ResponseInfo responseInfo, T instance) throws IOException { + static T parseBody(GitHubConnectorResponse connectorResponse, T instance) throws IOException { - String data = responseInfo.getBodyAsString(); + String data = getBodyAsString(connectorResponse); try { - return GitHubClient.getMappingObjectReader(responseInfo).withValueToUpdate(instance).readValue(data); + return GitHubClient.getMappingObjectReader(connectorResponse).withValueToUpdate(instance).readValue(data); } catch (JsonMappingException | JsonParseException e) { String message = "Failed to deserialize: " + data; LOGGER.log(Level.FINE, message); @@ -124,6 +123,21 @@ static T parseBody(ResponseInfo responseInfo, T instance) throws IOException } } + /** + * Gets the body of the response as a {@link String}. + * + * @return the body of the response as a {@link String}. + * @throws IOException + * if an I/O Exception occurs. + * @param connectorResponse + */ + @Nonnull + static String getBodyAsString(GitHubConnectorResponse connectorResponse) throws IOException { + InputStreamReader r = null; + r = new InputStreamReader(connectorResponse.bodyStream(), StandardCharsets.UTF_8); + return IOUtils.toString(r); + } + /** * The {@link URL} for this response. * @@ -135,12 +149,12 @@ public URL url() { } /** - * The {@link GitHubRequest} for this response. + * The {@link GitHubConnectorRequest} for this response. * - * @return the {@link GitHubRequest} for this response. + * @return the {@link GitHubConnectorRequest} for this response. */ @Nonnull - public GitHubRequest request() { + public GitHubConnectorRequest request() { return request; } @@ -189,145 +203,4 @@ public T body() { return body; } - /** - * Represents a supplier of results that can throw. - * - * @param - * the type of results supplied by this supplier - */ - @FunctionalInterface - interface BodyHandler extends FunctionThrows { - } - - /** - * Response information supplied to a {@link BodyHandler} when a response is received and before the body is - * processed. - * - * Instances of this class are closed once the response is done being processed. This means that contents of the - * body stream will not be readable after a call is completed. Status code, response headers, and request details - * will still be readable. - */ - public static abstract class ResponseInfo implements Closeable { - - private static final Comparator nullableCaseInsensitiveComparator = Comparator - .nullsFirst(String.CASE_INSENSITIVE_ORDER); - - private final int statusCode; - @Nonnull - private final GitHubRequest request; - @Nonnull - private final Map> headers; - - protected ResponseInfo(@Nonnull GitHubRequest request, - int statusCode, - @Nonnull Map> headers) { - this.request = request; - this.statusCode = statusCode; - - // Response header field names must be case-insensitive. - TreeMap> caseInsensitiveMap = new TreeMap<>(nullableCaseInsensitiveComparator); - caseInsensitiveMap.putAll(headers); - - this.headers = Collections.unmodifiableMap(caseInsensitiveMap); - } - - /** - * Gets the value of a header field for this response. - * - * @param name - * the name of the header field. - * @return the value of the header field, or {@code null} if the header isn't set. - */ - @CheckForNull - public String header(String name) { - String result = null; - if (headers.containsKey(name)) { - result = headers.get(name).get(0); - } - return result; - } - - /** - * Gets the value of a header field for this response. - * - * @param index - * the name of the header field. - * @return the value of the header field, or {@code null} if the header isn't set. - */ - @CheckForNull - String headerFieldKey(int index) { - List keys = new ArrayList<>(headers.keySet()); - return keys.get(index); - } - - /** - * The response body as an {@link InputStream}. - * - * @return the response body - * @throws IOException - * if an I/O Exception occurs. - */ - public abstract InputStream bodyStream() throws IOException; - - /** - * The error message for this response. - * - * @return if there is an error with some error string, that is returned. If not, {@code null}. - */ - public abstract String errorMessage(); - - /** - * The {@link URL} for this response. - * - * @return the {@link URL} for this response. - */ - @Nonnull - public URL url() { - return request.url(); - } - - /** - * Gets the {@link GitHubRequest} for this response. - * - * @return the {@link GitHubRequest} for this response. - */ - @Nonnull - public GitHubRequest request() { - return request; - } - - /** - * The status code for this response. - * - * @return the status code for this response. - */ - public int statusCode() { - return statusCode; - } - - /** - * The headers for this response. - * - * @return the headers for this response. - */ - @Nonnull - Map> allHeaders() { - return headers; - } - - /** - * Gets the body of the response as a {@link String}. - * - * @return the body of the response as a {@link String}. - * @throws IOException - * if an I/O Exception occurs. - */ - @Nonnull - String getBodyAsString() throws IOException { - InputStreamReader r = null; - r = new InputStreamReader(this.bodyStream(), StandardCharsets.UTF_8); - return IOUtils.toString(r); - } - } - } diff --git a/src/main/java/org/kohsuke/github/GitHubResponseInfoHttpURLConnectionAdapter.java b/src/main/java/org/kohsuke/github/GitHubResponseInfoHttpURLConnectionAdapter.java index 3c4e8d790f..b9d2a76591 100644 --- a/src/main/java/org/kohsuke/github/GitHubResponseInfoHttpURLConnectionAdapter.java +++ b/src/main/java/org/kohsuke/github/GitHubResponseInfoHttpURLConnectionAdapter.java @@ -1,5 +1,7 @@ package org.kohsuke.github; +import org.kohsuke.github.connector.GitHubConnectorResponse; + import java.io.ByteArrayInputStream; import java.io.IOException; import java.io.InputStream; @@ -16,16 +18,17 @@ class GitHubResponseInfoHttpURLConnectionAdapter extends HttpURLConnection { private static final Comparator nullableCaseInsensitiveComparator = Comparator .nullsFirst(String.CASE_INSENSITIVE_ORDER); - private final GitHubResponse.ResponseInfo responseInfo; + private final GitHubConnectorResponse connectorResponse; - GitHubResponseInfoHttpURLConnectionAdapter(GitHubResponse.ResponseInfo responseInfo) { - super(responseInfo.url()); - this.responseInfo = responseInfo; + public GitHubResponseInfoHttpURLConnectionAdapter(GitHubConnectorResponse connectorResponse) { + super(connectorResponse.url()); + this.connectorResponse = connectorResponse; } @Override public String getHeaderFieldKey(int n) { - return responseInfo.headerFieldKey(n); + List keys = new ArrayList<>(connectorResponse.allHeaders().keySet()); + return keys.get(n); } @Override @@ -45,7 +48,7 @@ public void setChunkedStreamingMode(int chunklen) { @Override public String getHeaderField(int n) { - return responseInfo.header(responseInfo.headerFieldKey(n)); + return connectorResponse.header(getHeaderFieldKey(n)); } @Override @@ -65,17 +68,17 @@ public void setRequestMethod(String method) throws ProtocolException { @Override public String getRequestMethod() { - return responseInfo.request().method(); + return connectorResponse.request().method(); } @Override public int getResponseCode() throws IOException { - return responseInfo.statusCode(); + return connectorResponse.statusCode(); } @Override public String getResponseMessage() throws IOException { - return responseInfo.header("Status"); + return connectorResponse.header("Status"); } @Override @@ -95,7 +98,7 @@ public Permission getPermission() throws IOException { @Override public InputStream getErrorStream() { - return new ByteArrayInputStream(responseInfo.errorMessage().getBytes(StandardCharsets.UTF_8)); + return new ByteArrayInputStream(connectorResponse.errorMessage().getBytes(StandardCharsets.UTF_8)); } @Override @@ -120,7 +123,7 @@ public int getReadTimeout() { @Override public URL getURL() { - return responseInfo.url(); + return connectorResponse.url(); } @Override @@ -138,12 +141,12 @@ public long getContentLengthLong() { @Override public String getContentType() { - return responseInfo.header("content-type"); + return connectorResponse.header("content-type"); } @Override public String getContentEncoding() { - return responseInfo.header("content-encoding"); + return connectorResponse.header("content-encoding"); } @Override @@ -163,12 +166,12 @@ public long getLastModified() { @Override public String getHeaderField(String name) { - return responseInfo.header(name); + return connectorResponse.header(name); } @Override public Map> getHeaderFields() { - return responseInfo.allHeaders(); + return connectorResponse.allHeaders(); } @Override @@ -203,7 +206,7 @@ public Object getContent() throws IOException { // // @Override // public InputStream getInputStream() throws IOException { - // return responseInfo.bodyStream(); + // return connectorResponse.bodyStream(); // } @Override @@ -213,7 +216,7 @@ public OutputStream getOutputStream() throws IOException { @Override public String toString() { - return responseInfo.toString(); + return connectorResponse.toString(); } @Override @@ -258,7 +261,7 @@ public void setIfModifiedSince(long ifmodifiedsince) { @Override public long getIfModifiedSince() { - String modifiedSince = responseInfo.request().header("If-Modified-Since"); + String modifiedSince = connectorResponse.request().header("If-Modified-Since"); if (modifiedSince != null) { return GitHubClient.parseDate(modifiedSince).getTime(); } else { @@ -283,12 +286,12 @@ public void addRequestProperty(String key, String value) { @Override public String getRequestProperty(String key) { - return responseInfo.request().header(key); + return connectorResponse.request().header(key); } @Override public Map> getRequestProperties() { - return responseInfo.request().allHeaders(); + return connectorResponse.request().allHeaders(); } @Override diff --git a/src/main/java/org/kohsuke/github/HttpConnector.java b/src/main/java/org/kohsuke/github/HttpConnector.java index 0cb92ff40e..91471ec7d9 100644 --- a/src/main/java/org/kohsuke/github/HttpConnector.java +++ b/src/main/java/org/kohsuke/github/HttpConnector.java @@ -12,9 +12,11 @@ *

* For example, you can implement this to st custom timeouts. * + * @deprecated Use {@link org.kohsuke.github.connector.GitHubConnector} instead. * @author Kohsuke Kawaguchi */ @FunctionalInterface +@Deprecated public interface HttpConnector { /** * Opens a connection to the given URL. diff --git a/src/main/java/org/kohsuke/github/RateLimitHandler.java b/src/main/java/org/kohsuke/github/RateLimitHandler.java index b8a5461de6..58a5f89f1e 100644 --- a/src/main/java/org/kohsuke/github/RateLimitHandler.java +++ b/src/main/java/org/kohsuke/github/RateLimitHandler.java @@ -1,5 +1,7 @@ package org.kohsuke.github; +import org.kohsuke.github.connector.GitHubConnectorResponse; + import java.io.IOException; import java.io.InterruptedIOException; import java.net.HttpURLConnection; @@ -21,19 +23,19 @@ public abstract class RateLimitHandler { * an exception. If this method returns normally, another request will be attempted. For that to make sense, the * implementation needs to wait for some time. * - * @param responseInfo + * @param connectorResponse * Response information for this request. * * @throws IOException * the io exception * @see API documentation from GitHub */ - void onError(GitHubResponse.ResponseInfo responseInfo) throws IOException { + void onError(GitHubConnectorResponse connectorResponse) throws IOException { GHIOException e = new HttpException("Rate limit violation", - responseInfo.statusCode(), - responseInfo.header("Status"), - responseInfo.url().toString()).withResponseHeaderFields(responseInfo.allHeaders()); - onError(e, new GitHubResponseInfoHttpURLConnectionAdapter(responseInfo)); + connectorResponse.statusCode(), + connectorResponse.header("Status"), + connectorResponse.url().toString()).withResponseHeaderFields(connectorResponse.allHeaders()); + onError(e, new GitHubResponseInfoHttpURLConnectionAdapter(connectorResponse)); } /** diff --git a/src/main/java/org/kohsuke/github/Requester.java b/src/main/java/org/kohsuke/github/Requester.java index 924f3b7318..2f3154ec09 100644 --- a/src/main/java/org/kohsuke/github/Requester.java +++ b/src/main/java/org/kohsuke/github/Requester.java @@ -25,6 +25,8 @@ import edu.umd.cs.findbugs.annotations.NonNull; import org.apache.commons.io.IOUtils; +import org.kohsuke.github.connector.GitHubConnectorResponse; +import org.kohsuke.github.function.BodyHandler; import org.kohsuke.github.function.InputStreamFunction; import java.io.ByteArrayInputStream; @@ -57,7 +59,7 @@ class Requester extends GitHubRequest.Builder { public void send() throws IOException { // Send expects there to be some body response, but doesn't care what it is. // If there isn't a body, this will throw. - client.sendRequest(this, (responseInfo) -> responseInfo.getBodyAsString()); + client.sendRequest(this, (connectorResponse) -> GitHubResponse.getBodyAsString(connectorResponse)); } /** @@ -72,7 +74,8 @@ public void send() throws IOException { * if the server returns 4xx/5xx responses. */ public T fetch(@Nonnull Class type) throws IOException { - return client.sendRequest(this, (responseInfo) -> GitHubResponse.parseBody(responseInfo, type)).body(); + return client.sendRequest(this, (connectorResponse) -> GitHubResponse.parseBody(connectorResponse, type)) + .body(); } /** @@ -87,7 +90,8 @@ public T fetch(@Nonnull Class type) throws IOException { * the io exception */ public T fetchInto(@Nonnull T existingInstance) throws IOException { - return client.sendRequest(this, (responseInfo) -> GitHubResponse.parseBody(responseInfo, existingInstance)) + return client + .sendRequest(this, (connectorResponse) -> GitHubResponse.parseBody(connectorResponse, existingInstance)) .body(); } @@ -111,17 +115,17 @@ public int fetchHttpStatusCode() throws IOException { * the io exception */ public T fetchStream(@Nonnull InputStreamFunction handler) throws IOException { - return client.sendRequest(this, (responseInfo) -> handler.apply(responseInfo.bodyStream())).body(); + return client.sendRequest(this, (connectorResponse) -> handler.apply(connectorResponse.bodyStream())).body(); } /** * Helper function to make it easy to pull streams. * * Copies an input stream to an in-memory input stream. The performance on this is not great but - * {@link GitHubResponse.ResponseInfo#bodyStream()} is closed at the end of every call to - * {@link GitHubClient#sendRequest(GitHubRequest, GitHubResponse.BodyHandler)}, so any reads to the original input - * stream must be completed before then. There are a number of deprecated methods that return {@link InputStream}. - * This method keeps all of them using the same code path. + * {@link GitHubConnectorResponse#bodyStream()} is closed at the end of every call to + * {@link GitHubClient#sendRequest(GitHubRequest, BodyHandler)}, so any reads to the original input stream must be + * completed before then. There are a number of deprecated methods that return {@link InputStream}. This method + * keeps all of them using the same code path. * * @param inputStream * the input stream to be copied diff --git a/src/main/java/org/kohsuke/github/connector/GitHubConnector.java b/src/main/java/org/kohsuke/github/connector/GitHubConnector.java new file mode 100644 index 0000000000..21558e985b --- /dev/null +++ b/src/main/java/org/kohsuke/github/connector/GitHubConnector.java @@ -0,0 +1,29 @@ +package org.kohsuke.github.connector; + +import org.kohsuke.github.internal.DefaultGitHubConnector; +import org.kohsuke.github.internal.GitHubConnectorHttpConnectorAdapter; + +import java.io.IOException; + +/** + * Pluggability for customizing HTTP request behaviors or using altogether different library. + * + * @author Liam Newman + */ +@FunctionalInterface +public interface GitHubConnector { + + GitHubConnectorResponse send(GitHubConnectorRequest request) throws IOException; + + /** + * Default implementation used when connector is not set by user. + */ + GitHubConnector DEFAULT = DefaultGitHubConnector.create(); + + /** + * Stub implementation that is always off-line. + */ + GitHubConnector OFFLINE = new GitHubConnectorHttpConnectorAdapter(url -> { + throw new IOException("Offline"); + }); +} diff --git a/src/main/java/org/kohsuke/github/connector/GitHubConnectorRequest.java b/src/main/java/org/kohsuke/github/connector/GitHubConnectorRequest.java new file mode 100644 index 0000000000..451987c0e3 --- /dev/null +++ b/src/main/java/org/kohsuke/github/connector/GitHubConnectorRequest.java @@ -0,0 +1,34 @@ +package org.kohsuke.github.connector; + +import java.io.InputStream; +import java.net.URL; +import java.util.List; +import java.util.Map; + +import javax.annotation.CheckForNull; +import javax.annotation.Nonnull; + +public interface GitHubConnectorRequest { + @Nonnull + String method(); + + @Nonnull + Map> allHeaders(); + + @CheckForNull + String header(String name); + + @CheckForNull + String contentType(); + + @CheckForNull + InputStream body(); + + @Nonnull + URL url(); + + boolean hasBody(); + + @Nonnull + Map injectedMappingValues(); +} diff --git a/src/main/java/org/kohsuke/github/connector/GitHubConnectorResponse.java b/src/main/java/org/kohsuke/github/connector/GitHubConnectorResponse.java new file mode 100644 index 0000000000..32cada1c8c --- /dev/null +++ b/src/main/java/org/kohsuke/github/connector/GitHubConnectorResponse.java @@ -0,0 +1,120 @@ +package org.kohsuke.github.connector; + +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; +import org.kohsuke.github.GitHubRequest; +import org.kohsuke.github.function.BodyHandler; + +import java.io.Closeable; +import java.io.IOException; +import java.io.InputStream; +import java.net.URL; +import java.util.*; + +import javax.annotation.CheckForNull; +import javax.annotation.Nonnull; + +/** + * Response information supplied to a {@link BodyHandler} when a response is received and before the body is processed. + *

+ * Instances of this class are closed once the response is done being processed. This means that contents of the body + * stream will not be readable after a call is completed. Status code, response headers, and request details will still + * be readable. + */ +public abstract class GitHubConnectorResponse implements Closeable { + + private static final Comparator nullableCaseInsensitiveComparator = Comparator + .nullsFirst(String.CASE_INSENSITIVE_ORDER); + + private final int statusCode; + @Nonnull + private final GitHubConnectorRequest request; + @Nonnull + private final Map> headers; + + protected GitHubConnectorResponse(@Nonnull GitHubConnectorRequest request, + int statusCode, + @Nonnull Map> headers) { + this.request = request; + this.statusCode = statusCode; + + // Response header field names must be case-insensitive. + TreeMap> caseInsensitiveMap = new TreeMap<>(nullableCaseInsensitiveComparator); + for (Map.Entry> entry : headers.entrySet()) { + caseInsensitiveMap.put(entry.getKey(), Collections.unmodifiableList(new ArrayList<>(entry.getValue()))); + } + this.headers = Collections.unmodifiableMap(caseInsensitiveMap); + } + + /** + * Gets the value of a header field for this response. + * + * @param name + * the name of the header field. + * @return the value of the header field, or {@code null} if the header isn't set. + */ + @CheckForNull + public String header(String name) { + String result = null; + if (headers.containsKey(name)) { + result = headers.get(name).get(0); + } + return result; + } + + /** + * The response body as an {@link InputStream}. + * + * @return the response body + * @throws IOException + * if an I/O Exception occurs. + */ + public abstract InputStream bodyStream() throws IOException; + + /** + * The error message for this response. + * + * @return if there is an error with some error string, that is returned. If not, {@code null}. + */ + public abstract String errorMessage(); + + /** + * The {@link URL} for this response. + * + * @return the {@link URL} for this response. + */ + @Nonnull + public URL url() { + return request.url(); + } + + /** + * Gets the {@link GitHubRequest} for this response. + * + * @return the {@link GitHubRequest} for this response. + */ + @Nonnull + public GitHubConnectorRequest request() { + return request; + } + + /** + * The status code for this response. + * + * @return the status code for this response. + */ + public int statusCode() { + return statusCode; + } + + /** + * The headers for this response. + * + * @return the headers for this response. + */ + @Nonnull + @SuppressFBWarnings(value = { "EI_EXPOSE_REP" }, justification = "Unmodifiable map of unmodifiable lists") + public Map> allHeaders() { + return headers; + } + +} diff --git a/src/main/java/org/kohsuke/github/extras/okhttp3/OkHttpConnector.java b/src/main/java/org/kohsuke/github/extras/okhttp3/OkHttpConnector.java index f415f832de..4c855eb16f 100644 --- a/src/main/java/org/kohsuke/github/extras/okhttp3/OkHttpConnector.java +++ b/src/main/java/org/kohsuke/github/extras/okhttp3/OkHttpConnector.java @@ -3,6 +3,9 @@ import okhttp3.*; import org.apache.commons.io.IOUtils; import org.kohsuke.github.*; +import org.kohsuke.github.connector.GitHubConnector; +import org.kohsuke.github.connector.GitHubConnectorRequest; +import org.kohsuke.github.connector.GitHubConnectorResponse; import java.io.FileNotFoundException; import java.io.IOException; @@ -67,7 +70,7 @@ public OkHttpConnector(OkHttpClient client, int cacheMaxAge) { } @Override - public GitHubResponse.ResponseInfo send(GitHubRequest request) throws IOException { + public GitHubConnectorResponse send(GitHubConnectorRequest request) throws IOException { Request.Builder builder = new Request.Builder().url(request.url()); if (maxAgeHeaderValue != null && request.header(HEADER_NAME) == null) { // By default OkHttp honors max-age, meaning it will use local cache @@ -87,14 +90,14 @@ public GitHubResponse.ResponseInfo send(GitHubRequest request) throws IOExceptio } RequestBody body = null; - if (request.inBody()) { + if (request.hasBody()) { body = RequestBody.create(IOUtils.toByteArray(request.body())); } builder.method(request.method(), body); Request okhttpRequest = builder.build(); Response okhttpResponse = client.newCall(okhttpRequest).execute(); - return new OkHttpResponseInfo(request, okhttpResponse); + return new OkHttpGitHubConnectorResponse(request, okhttpResponse); } /** Returns connection spec with TLS v1.2 in it */ @@ -107,12 +110,12 @@ private List TlsConnectionSpecs() { * * Implementation specific to {@link okhttp3.Response}. */ - static class OkHttpResponseInfo extends GitHubResponse.ResponseInfo { + static class OkHttpGitHubConnectorResponse extends GitHubConnectorResponse { @Nonnull private final Response response; - OkHttpResponseInfo(@Nonnull GitHubRequest request, @Nonnull Response response) { + OkHttpGitHubConnectorResponse(@Nonnull GitHubConnectorRequest request, @Nonnull Response response) { super(request, response.code(), response.headers().toMultimap()); this.response = response; } diff --git a/src/main/java/org/kohsuke/github/function/BodyHandler.java b/src/main/java/org/kohsuke/github/function/BodyHandler.java new file mode 100644 index 0000000000..0083dcd526 --- /dev/null +++ b/src/main/java/org/kohsuke/github/function/BodyHandler.java @@ -0,0 +1,15 @@ +package org.kohsuke.github.function; + +import org.kohsuke.github.connector.GitHubConnectorResponse; + +import java.io.IOException; + +/** + * Represents a supplier of results that can throw. + * + * @param + * the type of results supplied by this supplier + */ +@FunctionalInterface +public interface BodyHandler extends FunctionThrows { +} diff --git a/src/main/java/org/kohsuke/github/internal/DefaultGitHubConnector.java b/src/main/java/org/kohsuke/github/internal/DefaultGitHubConnector.java new file mode 100644 index 0000000000..c8220c7fec --- /dev/null +++ b/src/main/java/org/kohsuke/github/internal/DefaultGitHubConnector.java @@ -0,0 +1,22 @@ +package org.kohsuke.github.internal; + +import okhttp3.OkHttpClient; +import org.kohsuke.github.HttpConnector; +import org.kohsuke.github.connector.GitHubConnector; +import org.kohsuke.github.extras.okhttp3.OkHttpConnector; + +public class DefaultGitHubConnector { + public static GitHubConnector create() { + String defaultConnectorProperty = System.getProperty("test.github.connector", "default"); + if (defaultConnectorProperty.equalsIgnoreCase("okhttp")) { + return new OkHttpConnector(new OkHttpClient.Builder().build()); + } else if (defaultConnectorProperty.equalsIgnoreCase("httpconnector")) { + return new GitHubConnectorHttpConnectorAdapter(HttpConnector.DEFAULT); + } else if (defaultConnectorProperty.equalsIgnoreCase("default")) { + return new GitHubConnectorHttpConnectorAdapter(HttpConnector.DEFAULT); + } else { + throw new IllegalStateException( + "Property 'test.github.connector' must reference a valid built-in connector - okhttp, httpconnector, or default."); + } + } +} diff --git a/src/main/java/org/kohsuke/github/GitHubConnectorHttpConnectorAdapter.java b/src/main/java/org/kohsuke/github/internal/GitHubConnectorHttpConnectorAdapter.java similarity index 85% rename from src/main/java/org/kohsuke/github/GitHubConnectorHttpConnectorAdapter.java rename to src/main/java/org/kohsuke/github/internal/GitHubConnectorHttpConnectorAdapter.java index 6c4b586e4a..451da0a175 100644 --- a/src/main/java/org/kohsuke/github/GitHubConnectorHttpConnectorAdapter.java +++ b/src/main/java/org/kohsuke/github/internal/GitHubConnectorHttpConnectorAdapter.java @@ -1,7 +1,11 @@ -package org.kohsuke.github; +package org.kohsuke.github.internal; import org.apache.commons.io.IOUtils; import org.jetbrains.annotations.NotNull; +import org.kohsuke.github.*; +import org.kohsuke.github.connector.GitHubConnector; +import org.kohsuke.github.connector.GitHubConnectorRequest; +import org.kohsuke.github.connector.GitHubConnectorResponse; import java.io.IOException; import java.io.InputStream; @@ -30,7 +34,7 @@ * GitHubHttpUrlConnectionClient gets a new {@link HttpURLConnection} for each call to send. *

*/ -class GitHubConnectorHttpConnectorAdapter implements GitHubConnector { +public class GitHubConnectorHttpConnectorAdapter implements GitHubConnector, HttpConnector { final HttpConnector httpConnector; @@ -53,13 +57,13 @@ public static GitHubConnector adapt(HttpConnector connector) { return gitHubConnector; } - @Override + @Nonnull public HttpURLConnection connect(URL url) throws IOException { return this.httpConnector.connect(url); } @Nonnull - public GitHubResponse.ResponseInfo send(GitHubRequest request) throws IOException { + public GitHubConnectorResponse send(GitHubConnectorRequest request) throws IOException { HttpURLConnection connection; try { connection = setupConnection(httpConnector, request); @@ -73,11 +77,11 @@ public GitHubResponse.ResponseInfo send(GitHubRequest request) throws IOExceptio int statusCode = connection.getResponseCode(); Map> headers = connection.getHeaderFields(); - return new HttpURLConnectionResponseInfo(request, statusCode, headers, connection); + return new HttpURLConnectionGitHubConnectorResponse(request, statusCode, headers, connection); } @Nonnull - static HttpURLConnection setupConnection(@Nonnull HttpConnector connector, @Nonnull GitHubRequest request) + static HttpURLConnection setupConnection(@Nonnull HttpConnector connector, @Nonnull GitHubConnectorRequest request) throws IOException { HttpURLConnection connection = connector.connect(request.url()); setRequestMethod(request.method(), connection); @@ -89,14 +93,14 @@ static HttpURLConnection setupConnection(@Nonnull HttpConnector connector, @Nonn /** * Set up the request parameters or POST payload. */ - private static void buildRequest(GitHubRequest request, HttpURLConnection connection) throws IOException { + private static void buildRequest(GitHubConnectorRequest request, HttpURLConnection connection) throws IOException { for (Map.Entry> e : request.allHeaders().entrySet()) { List v = e.getValue(); if (v != null) connection.setRequestProperty(e.getKey(), String.join(", ", v)); } - if (request.inBody()) { + if (request.hasBody()) { connection.setDoOutput(true); IOUtils.copyLarge(request.body(), connection.getOutputStream()); } @@ -134,17 +138,17 @@ private static void setRequestMethod(String method, HttpURLConnection connection } /** - * Initial response information supplied to a {@link GitHubResponse.BodyHandler} when a response is initially - * received and before the body is processed. + * Initial response information supplied to a {@link org.kohsuke.github.function.BodyHandler} when a response is + * initially received and before the body is processed. * * Implementation specific to {@link HttpURLConnection}. */ - static class HttpURLConnectionResponseInfo extends GitHubResponse.ResponseInfo { + static class HttpURLConnectionGitHubConnectorResponse extends GitHubConnectorResponse { @Nonnull private final HttpURLConnection connection; - HttpURLConnectionResponseInfo(@Nonnull GitHubRequest request, + HttpURLConnectionGitHubConnectorResponse(@Nonnull GitHubConnectorRequest request, int statusCode, @Nonnull Map> headers, @Nonnull HttpURLConnection connection) { @@ -195,7 +199,7 @@ private InputStream wrapStream(InputStream stream) throws IOException { throw new UnsupportedOperationException("Unexpected Content-Encoding: " + encoding); } - private static final Logger LOGGER = Logger.getLogger(GitHubClient.class.getName()); + private static final Logger LOGGER = Logger.getLogger(GitHub.class.getName()); @Override public void close() throws IOException { diff --git a/src/test/java/org/kohsuke/github/GitHubStaticTest.java b/src/test/java/org/kohsuke/github/GitHubStaticTest.java index 0c57f1c0ce..37f67aa2ca 100644 --- a/src/test/java/org/kohsuke/github/GitHubStaticTest.java +++ b/src/test/java/org/kohsuke/github/GitHubStaticTest.java @@ -2,6 +2,8 @@ import org.junit.Assert; import org.junit.Test; +import org.kohsuke.github.connector.GitHubConnector; +import org.kohsuke.github.connector.GitHubConnectorResponse; import java.net.MalformedURLException; import java.net.URL; @@ -324,7 +326,7 @@ public void testMappingReaderWriter() throws Exception { assertThat(repoString, not(nullValue())); assertThat(repoString, containsString("testMappingReaderWriter")); - GHRepository readRepo = GitHubClient.getMappingObjectReader((GitHubResponse.ResponseInfo) null) + GHRepository readRepo = GitHubClient.getMappingObjectReader((GitHubConnectorResponse) null) .forType(GHRepository.class) .readValue(repoString); diff --git a/src/test/resources/org/kohsuke/github/GHRepositoryTest/wiremock/getCommitsBetweenOver250/__files/repos_hub4j-test-org_github-api_compare_4261c42949915816a9f246eb14c3dfd21a637bc294ff089e60064bfa43e374baeb10846f7ce82f40-4.json b/src/test/resources/org/kohsuke/github/GHRepositoryTest/wiremock/getCommitsBetweenOver250/__files/repos_hub4j-test-org_github-api_compare_4261c42949915816a9f246eb14c3dfd21a637bc294ff089e60064bfa43e374baeb10846f7ce82f40-4.json index d0313d237e..0f14fb7af1 100644 --- a/src/test/resources/org/kohsuke/github/GHRepositoryTest/wiremock/getCommitsBetweenOver250/__files/repos_hub4j-test-org_github-api_compare_4261c42949915816a9f246eb14c3dfd21a637bc294ff089e60064bfa43e374baeb10846f7ce82f40-4.json +++ b/src/test/resources/org/kohsuke/github/GHRepositoryTest/wiremock/getCommitsBetweenOver250/__files/repos_hub4j-test-org_github-api_compare_4261c42949915816a9f246eb14c3dfd21a637bc294ff089e60064bfa43e374baeb10846f7ce82f40-4.json @@ -21136,7 +21136,7 @@ "blob_url": "https://github.com/hub4j-test-org/github-api/blob/94ff089e60064bfa43e374baeb10846f7ce82f40/src/main/java/org/kohsuke/github/GitHubClient.java", "raw_url": "https://github.com/hub4j-test-org/github-api/raw/94ff089e60064bfa43e374baeb10846f7ce82f40/src/main/java/org/kohsuke/github/GitHubClient.java", "contents_url": "https://api.github.com/repos/hub4j-test-org/github-api/contents/src/main/java/org/kohsuke/github/GitHubClient.java?ref=94ff089e60064bfa43e374baeb10846f7ce82f40", - "patch": "@@ -14,6 +14,7 @@\n import java.time.format.DateTimeFormatter;\n import java.time.temporal.ChronoUnit;\n import java.util.*;\n+import java.util.concurrent.atomic.AtomicReference;\n import java.util.function.Consumer;\n import java.util.logging.Logger;\n \n@@ -52,10 +53,8 @@\n \n private HttpConnector connector;\n \n- private final Object rateLimitLock = new Object();\n-\n @Nonnull\n- private GHRateLimit rateLimit = GHRateLimit.DEFAULT;\n+ private final AtomicReference rateLimit = new AtomicReference<>(GHRateLimit.DEFAULT);\n \n private static final Logger LOGGER = Logger.getLogger(GitHubClient.class.getName());\n \n@@ -141,10 +140,9 @@ public boolean isCredentialValid() {\n getRateLimit();\n return true;\n } catch (IOException e) {\n- if (LOGGER.isLoggable(FINE))\n- LOGGER.log(FINE,\n- \"Exception validating credentials on \" + getApiUrl() + \" with login '\" + login + \"' \" + e,\n- e);\n+ LOGGER.log(FINE,\n+ \"Exception validating credentials on \" + getApiUrl() + \" with login '\" + login + \"' \" + e,\n+ e);\n return false;\n }\n }\n@@ -256,9 +254,7 @@ GHRateLimit getRateLimit(@Nonnull RateLimitTarget rateLimitTarget) throws IOExce\n @Nonnull\n @Deprecated\n GHRateLimit lastRateLimit() {\n- synchronized (rateLimitLock) {\n- return rateLimit;\n- }\n+ return rateLimit.get();\n }\n \n /**\n@@ -278,12 +274,19 @@ GHRateLimit lastRateLimit() {\n */\n @Nonnull\n GHRateLimit rateLimit(@Nonnull RateLimitTarget rateLimitTarget) throws IOException {\n- synchronized (rateLimitLock) {\n- if (rateLimit.getRecord(rateLimitTarget).isExpired()) {\n- getRateLimit(rateLimitTarget);\n+ GHRateLimit result = rateLimit.get();\n+ // Most of the time rate limit is not expired, so try to avoid locking.\n+ if (result.getRecord(rateLimitTarget).isExpired()) {\n+ // if the rate limit is expired, synchronize to ensure\n+ // only one call to getRateLimit() is made to refresh it.\n+ synchronized (this) {\n+ if (rateLimit.get().getRecord(rateLimitTarget).isExpired()) {\n+ getRateLimit(rateLimitTarget);\n+ }\n }\n- return rateLimit;\n+ result = rateLimit.get();\n }\n+ return result;\n }\n \n /**\n@@ -296,15 +299,9 @@ GHRateLimit rateLimit(@Nonnull RateLimitTarget rateLimitTarget) throws IOExcepti\n * {@link GHRateLimit.Record} constructed from the response header information\n */\n private GHRateLimit updateRateLimit(@Nonnull GHRateLimit observed) {\n- synchronized (rateLimitLock) {\n- observed = rateLimit.getMergedRateLimit(observed);\n-\n- if (rateLimit != observed) {\n- rateLimit = observed;\n- LOGGER.log(FINE, \"Rate limit now: {0}\", rateLimit);\n- }\n- return rateLimit;\n- }\n+ GHRateLimit result = rateLimit.accumulateAndGet(observed, (current, x) -> current.getMergedRateLimit(x));\n+ LOGGER.log(FINEST, \"Rate limit now: {0}\", rateLimit.get());\n+ return result;\n }\n \n /**\n@@ -384,11 +381,7 @@ public String getApiUrl() {\n GitHubResponse.ResponseInfo responseInfo = null;\n try {\n try {\n- if (LOGGER.isLoggable(FINE)) {\n- LOGGER.log(FINE,\n- \"GitHub API request [\" + (login == null ? \"anonymous\" : login) + \"]: \"\n- + request.method() + \" \" + request.url().toString());\n- }\n+ logRequest(request);\n rateLimitChecker.checkRateLimit(this, request);\n \n responseInfo = getResponseInfo(request);\n@@ -424,6 +417,12 @@ public String getApiUrl() {\n throw new GHIOException(\"Ran out of retries for URL: \" + request.url().toString());\n }\n \n+ private void logRequest(@Nonnull final GitHubRequest request) {\n+ LOGGER.log(FINE,\n+ () -> \"GitHub API request [\" + (login == null ? \"anonymous\" : login) + \"]: \" + request.method() + \" \"\n+ + request.url().toString());\n+ }\n+\n @Nonnull\n protected abstract GitHubResponse.ResponseInfo getResponseInfo(GitHubRequest request) throws IOException;\n \n@@ -437,20 +436,15 @@ public String getApiUrl() {\n // special case handling for 304 unmodified, as the content will be \"\"\n } else if (responseInfo.statusCode() == HttpURLConnection.HTTP_ACCEPTED) {\n \n- // Response code 202 means data is being generated.\n+ // Response code 202 means data is being generated or an action that can require some time is triggered.\n // This happens in specific cases:\n // statistics - See https://developer.github.com/v3/repos/statistics/#a-word-about-caching\n // fork creation - See https://developer.github.com/v3/repos/forks/#create-a-fork\n+ // workflow run cancellation - See https://docs.github.com/en/rest/reference/actions#cancel-a-workflow-run\n \n- if (responseInfo.url().toString().endsWith(\"/forks\")) {\n- LOGGER.log(INFO, \"The fork is being created. Please try again in 5 seconds.\");\n- } else if (responseInfo.url().toString().endsWith(\"/statistics\")) {\n- LOGGER.log(INFO, \"The statistics are being generated. Please try again in 5 seconds.\");\n- } else {\n- LOGGER.log(INFO,\n- \"Received 202 from \" + responseInfo.url().toString() + \" . Please try again in 5 seconds.\");\n- }\n- // Maybe throw an exception instead?\n+ LOGGER.log(FINE,\n+ \"Received HTTP_ACCEPTED(202) from \" + responseInfo.url().toString()\n+ + \" . Please try again in 5 seconds.\");\n } else if (handler != null) {\n body = handler.apply(responseInfo);\n }\n@@ -562,9 +556,7 @@ private void noteRateLimit(@Nonnull GitHubResponse.ResponseInfo responseInfo) {\n GHRateLimit.Record observed = new GHRateLimit.Record(limit, remaining, reset, responseInfo);\n updateRateLimit(GHRateLimit.fromRecord(observed, responseInfo.request().rateLimitTarget()));\n } catch (NumberFormatException | NullPointerException e) {\n- if (LOGGER.isLoggable(FINEST)) {\n- LOGGER.log(FINEST, \"Missing or malformed X-RateLimit header: \", e);\n- }\n+ LOGGER.log(FINEST, \"Missing or malformed X-RateLimit header: \", e);\n }\n }\n " + "patch": "@@ -14,6 +14,7 @@\n import java.time.format.DateTimeFormatter;\n import java.time.temporal.ChronoUnit;\n import java.util.*;\n+import java.util.concurrent.atomic.AtomicReference;\n import java.util.function.Consumer;\n import java.util.logging.Logger;\n \n@@ -52,10 +53,8 @@\n \n private HttpConnector connector;\n \n- private final Object rateLimitLock = new Object();\n-\n @Nonnull\n- private GHRateLimit rateLimit = GHRateLimit.DEFAULT;\n+ private final AtomicReference rateLimit = new AtomicReference<>(GHRateLimit.DEFAULT);\n \n private static final Logger LOGGER = Logger.getLogger(GitHubClient.class.getName());\n \n@@ -141,10 +140,9 @@ public boolean isCredentialValid() {\n getRateLimit();\n return true;\n } catch (IOException e) {\n- if (LOGGER.isLoggable(FINE))\n- LOGGER.log(FINE,\n- \"Exception validating credentials on \" + getApiUrl() + \" with login '\" + login + \"' \" + e,\n- e);\n+ LOGGER.log(FINE,\n+ \"Exception validating credentials on \" + getApiUrl() + \" with login '\" + login + \"' \" + e,\n+ e);\n return false;\n }\n }\n@@ -256,9 +254,7 @@ GHRateLimit getRateLimit(@Nonnull RateLimitTarget rateLimitTarget) throws IOExce\n @Nonnull\n @Deprecated\n GHRateLimit lastRateLimit() {\n- synchronized (rateLimitLock) {\n- return rateLimit;\n- }\n+ return rateLimit.get();\n }\n \n /**\n@@ -278,12 +274,19 @@ GHRateLimit lastRateLimit() {\n */\n @Nonnull\n GHRateLimit rateLimit(@Nonnull RateLimitTarget rateLimitTarget) throws IOException {\n- synchronized (rateLimitLock) {\n- if (rateLimit.getRecord(rateLimitTarget).isExpired()) {\n- getRateLimit(rateLimitTarget);\n+ GHRateLimit result = rateLimit.get();\n+ // Most of the time rate limit is not expired, so try to avoid locking.\n+ if (result.getRecord(rateLimitTarget).isExpired()) {\n+ // if the rate limit is expired, synchronize to ensure\n+ // only one call to getRateLimit() is made to refresh it.\n+ synchronized (this) {\n+ if (rateLimit.get().getRecord(rateLimitTarget).isExpired()) {\n+ getRateLimit(rateLimitTarget);\n+ }\n }\n- return rateLimit;\n+ result = rateLimit.get();\n }\n+ return result;\n }\n \n /**\n@@ -296,15 +299,9 @@ GHRateLimit rateLimit(@Nonnull RateLimitTarget rateLimitTarget) throws IOExcepti\n * {@link GHRateLimit.Record} constructed from the response header information\n */\n private GHRateLimit updateRateLimit(@Nonnull GHRateLimit observed) {\n- synchronized (rateLimitLock) {\n- observed = rateLimit.getMergedRateLimit(observed);\n-\n- if (rateLimit != observed) {\n- rateLimit = observed;\n- LOGGER.log(FINE, \"Rate limit now: {0}\", rateLimit);\n- }\n- return rateLimit;\n- }\n+ GHRateLimit result = rateLimit.accumulateAndGet(observed, (current, x) -> current.getMergedRateLimit(x));\n+ LOGGER.log(FINEST, \"Rate limit now: {0}\", rateLimit.get());\n+ return result;\n }\n \n /**\n@@ -384,11 +381,7 @@ public String getApiUrl() {\n GitHubResponse.ResponseInfo connectorResponse = null;\n try {\n try {\n- if (LOGGER.isLoggable(FINE)) {\n- LOGGER.log(FINE,\n- \"GitHub API request [\" + (login == null ? \"anonymous\" : login) + \"]: \"\n- + request.method() + \" \" + request.url().toString());\n- }\n+ logRequest(request);\n rateLimitChecker.checkRateLimit(this, request);\n \n connectorResponse = getResponseInfo(request);\n@@ -424,6 +417,12 @@ public String getApiUrl() {\n throw new GHIOException(\"Ran out of retries for URL: \" + request.url().toString());\n }\n \n+ private void logRequest(@Nonnull final GitHubRequest request) {\n+ LOGGER.log(FINE,\n+ () -> \"GitHub API request [\" + (login == null ? \"anonymous\" : login) + \"]: \" + request.method() + \" \"\n+ + request.url().toString());\n+ }\n+\n @Nonnull\n protected abstract GitHubResponse.ResponseInfo getResponseInfo(GitHubRequest request) throws IOException;\n \n@@ -437,20 +436,15 @@ public String getApiUrl() {\n // special case handling for 304 unmodified, as the content will be \"\"\n } else if (connectorResponse.statusCode() == HttpURLConnection.HTTP_ACCEPTED) {\n \n- // Response code 202 means data is being generated.\n+ // Response code 202 means data is being generated or an action that can require some time is triggered.\n // This happens in specific cases:\n // statistics - See https://developer.github.com/v3/repos/statistics/#a-word-about-caching\n // fork creation - See https://developer.github.com/v3/repos/forks/#create-a-fork\n+ // workflow run cancellation - See https://docs.github.com/en/rest/reference/actions#cancel-a-workflow-run\n \n- if (connectorResponse.url().toString().endsWith(\"/forks\")) {\n- LOGGER.log(INFO, \"The fork is being created. Please try again in 5 seconds.\");\n- } else if (connectorResponse.url().toString().endsWith(\"/statistics\")) {\n- LOGGER.log(INFO, \"The statistics are being generated. Please try again in 5 seconds.\");\n- } else {\n- LOGGER.log(INFO,\n- \"Received 202 from \" + connectorResponse.url().toString() + \" . Please try again in 5 seconds.\");\n- }\n- // Maybe throw an exception instead?\n+ LOGGER.log(FINE,\n+ \"Received HTTP_ACCEPTED(202) from \" + connectorResponse.url().toString()\n+ + \" . Please try again in 5 seconds.\");\n } else if (handler != null) {\n body = handler.apply(connectorResponse);\n }\n@@ -562,9 +556,7 @@ private void noteRateLimit(@Nonnull GitHubResponse.ResponseInfo connectorResponse) {\n GHRateLimit.Record observed = new GHRateLimit.Record(limit, remaining, reset, connectorResponse);\n updateRateLimit(GHRateLimit.fromRecord(observed, connectorResponse.request().rateLimitTarget()));\n } catch (NumberFormatException | NullPointerException e) {\n- if (LOGGER.isLoggable(FINEST)) {\n- LOGGER.log(FINEST, \"Missing or malformed X-RateLimit header: \", e);\n- }\n+ LOGGER.log(FINEST, \"Missing or malformed X-RateLimit header: \", e);\n }\n }\n " }, { "sha": "aea8b99b1f3301f65c8bc86c657bfd187db5c178", diff --git a/src/test/resources/org/kohsuke/github/GHRepositoryTest/wiremock/getCommitsBetweenPaged/__files/repos_hub4j-test-org_github-api_compare_4261c42949915816a9f246eb14c3dfd21a637bc294ff089e60064bfa43e374baeb10846f7ce82f40-4.json b/src/test/resources/org/kohsuke/github/GHRepositoryTest/wiremock/getCommitsBetweenPaged/__files/repos_hub4j-test-org_github-api_compare_4261c42949915816a9f246eb14c3dfd21a637bc294ff089e60064bfa43e374baeb10846f7ce82f40-4.json index a66e4891aa..7ff57c7ca2 100644 --- a/src/test/resources/org/kohsuke/github/GHRepositoryTest/wiremock/getCommitsBetweenPaged/__files/repos_hub4j-test-org_github-api_compare_4261c42949915816a9f246eb14c3dfd21a637bc294ff089e60064bfa43e374baeb10846f7ce82f40-4.json +++ b/src/test/resources/org/kohsuke/github/GHRepositoryTest/wiremock/getCommitsBetweenPaged/__files/repos_hub4j-test-org_github-api_compare_4261c42949915816a9f246eb14c3dfd21a637bc294ff089e60064bfa43e374baeb10846f7ce82f40-4.json @@ -1098,7 +1098,7 @@ "blob_url": "https://github.com/hub4j-test-org/github-api/blob/94ff089e60064bfa43e374baeb10846f7ce82f40/src/main/java/org/kohsuke/github/GitHubClient.java", "raw_url": "https://github.com/hub4j-test-org/github-api/raw/94ff089e60064bfa43e374baeb10846f7ce82f40/src/main/java/org/kohsuke/github/GitHubClient.java", "contents_url": "https://api.github.com/repos/hub4j-test-org/github-api/contents/src/main/java/org/kohsuke/github/GitHubClient.java?ref=94ff089e60064bfa43e374baeb10846f7ce82f40", - "patch": "@@ -14,6 +14,7 @@\n import java.time.format.DateTimeFormatter;\n import java.time.temporal.ChronoUnit;\n import java.util.*;\n+import java.util.concurrent.atomic.AtomicReference;\n import java.util.function.Consumer;\n import java.util.logging.Logger;\n \n@@ -52,10 +53,8 @@\n \n private HttpConnector connector;\n \n- private final Object rateLimitLock = new Object();\n-\n @Nonnull\n- private GHRateLimit rateLimit = GHRateLimit.DEFAULT;\n+ private final AtomicReference rateLimit = new AtomicReference<>(GHRateLimit.DEFAULT);\n \n private static final Logger LOGGER = Logger.getLogger(GitHubClient.class.getName());\n \n@@ -141,10 +140,9 @@ public boolean isCredentialValid() {\n getRateLimit();\n return true;\n } catch (IOException e) {\n- if (LOGGER.isLoggable(FINE))\n- LOGGER.log(FINE,\n- \"Exception validating credentials on \" + getApiUrl() + \" with login '\" + login + \"' \" + e,\n- e);\n+ LOGGER.log(FINE,\n+ \"Exception validating credentials on \" + getApiUrl() + \" with login '\" + login + \"' \" + e,\n+ e);\n return false;\n }\n }\n@@ -256,9 +254,7 @@ GHRateLimit getRateLimit(@Nonnull RateLimitTarget rateLimitTarget) throws IOExce\n @Nonnull\n @Deprecated\n GHRateLimit lastRateLimit() {\n- synchronized (rateLimitLock) {\n- return rateLimit;\n- }\n+ return rateLimit.get();\n }\n \n /**\n@@ -278,12 +274,19 @@ GHRateLimit lastRateLimit() {\n */\n @Nonnull\n GHRateLimit rateLimit(@Nonnull RateLimitTarget rateLimitTarget) throws IOException {\n- synchronized (rateLimitLock) {\n- if (rateLimit.getRecord(rateLimitTarget).isExpired()) {\n- getRateLimit(rateLimitTarget);\n+ GHRateLimit result = rateLimit.get();\n+ // Most of the time rate limit is not expired, so try to avoid locking.\n+ if (result.getRecord(rateLimitTarget).isExpired()) {\n+ // if the rate limit is expired, synchronize to ensure\n+ // only one call to getRateLimit() is made to refresh it.\n+ synchronized (this) {\n+ if (rateLimit.get().getRecord(rateLimitTarget).isExpired()) {\n+ getRateLimit(rateLimitTarget);\n+ }\n }\n- return rateLimit;\n+ result = rateLimit.get();\n }\n+ return result;\n }\n \n /**\n@@ -296,15 +299,9 @@ GHRateLimit rateLimit(@Nonnull RateLimitTarget rateLimitTarget) throws IOExcepti\n * {@link GHRateLimit.Record} constructed from the response header information\n */\n private GHRateLimit updateRateLimit(@Nonnull GHRateLimit observed) {\n- synchronized (rateLimitLock) {\n- observed = rateLimit.getMergedRateLimit(observed);\n-\n- if (rateLimit != observed) {\n- rateLimit = observed;\n- LOGGER.log(FINE, \"Rate limit now: {0}\", rateLimit);\n- }\n- return rateLimit;\n- }\n+ GHRateLimit result = rateLimit.accumulateAndGet(observed, (current, x) -> current.getMergedRateLimit(x));\n+ LOGGER.log(FINEST, \"Rate limit now: {0}\", rateLimit.get());\n+ return result;\n }\n \n /**\n@@ -384,11 +381,7 @@ public String getApiUrl() {\n GitHubResponse.ResponseInfo responseInfo = null;\n try {\n try {\n- if (LOGGER.isLoggable(FINE)) {\n- LOGGER.log(FINE,\n- \"GitHub API request [\" + (login == null ? \"anonymous\" : login) + \"]: \"\n- + request.method() + \" \" + request.url().toString());\n- }\n+ logRequest(request);\n rateLimitChecker.checkRateLimit(this, request);\n \n responseInfo = getResponseInfo(request);\n@@ -424,6 +417,12 @@ public String getApiUrl() {\n throw new GHIOException(\"Ran out of retries for URL: \" + request.url().toString());\n }\n \n+ private void logRequest(@Nonnull final GitHubRequest request) {\n+ LOGGER.log(FINE,\n+ () -> \"GitHub API request [\" + (login == null ? \"anonymous\" : login) + \"]: \" + request.method() + \" \"\n+ + request.url().toString());\n+ }\n+\n @Nonnull\n protected abstract GitHubResponse.ResponseInfo getResponseInfo(GitHubRequest request) throws IOException;\n \n@@ -437,20 +436,15 @@ public String getApiUrl() {\n // special case handling for 304 unmodified, as the content will be \"\"\n } else if (responseInfo.statusCode() == HttpURLConnection.HTTP_ACCEPTED) {\n \n- // Response code 202 means data is being generated.\n+ // Response code 202 means data is being generated or an action that can require some time is triggered.\n // This happens in specific cases:\n // statistics - See https://developer.github.com/v3/repos/statistics/#a-word-about-caching\n // fork creation - See https://developer.github.com/v3/repos/forks/#create-a-fork\n+ // workflow run cancellation - See https://docs.github.com/en/rest/reference/actions#cancel-a-workflow-run\n \n- if (responseInfo.url().toString().endsWith(\"/forks\")) {\n- LOGGER.log(INFO, \"The fork is being created. Please try again in 5 seconds.\");\n- } else if (responseInfo.url().toString().endsWith(\"/statistics\")) {\n- LOGGER.log(INFO, \"The statistics are being generated. Please try again in 5 seconds.\");\n- } else {\n- LOGGER.log(INFO,\n- \"Received 202 from \" + responseInfo.url().toString() + \" . Please try again in 5 seconds.\");\n- }\n- // Maybe throw an exception instead?\n+ LOGGER.log(FINE,\n+ \"Received HTTP_ACCEPTED(202) from \" + responseInfo.url().toString()\n+ + \" . Please try again in 5 seconds.\");\n } else if (handler != null) {\n body = handler.apply(responseInfo);\n }\n@@ -562,9 +556,7 @@ private void noteRateLimit(@Nonnull GitHubResponse.ResponseInfo responseInfo) {\n GHRateLimit.Record observed = new GHRateLimit.Record(limit, remaining, reset, responseInfo);\n updateRateLimit(GHRateLimit.fromRecord(observed, responseInfo.request().rateLimitTarget()));\n } catch (NumberFormatException | NullPointerException e) {\n- if (LOGGER.isLoggable(FINEST)) {\n- LOGGER.log(FINEST, \"Missing or malformed X-RateLimit header: \", e);\n- }\n+ LOGGER.log(FINEST, \"Missing or malformed X-RateLimit header: \", e);\n }\n }\n " + "patch": "@@ -14,6 +14,7 @@\n import java.time.format.DateTimeFormatter;\n import java.time.temporal.ChronoUnit;\n import java.util.*;\n+import java.util.concurrent.atomic.AtomicReference;\n import java.util.function.Consumer;\n import java.util.logging.Logger;\n \n@@ -52,10 +53,8 @@\n \n private HttpConnector connector;\n \n- private final Object rateLimitLock = new Object();\n-\n @Nonnull\n- private GHRateLimit rateLimit = GHRateLimit.DEFAULT;\n+ private final AtomicReference rateLimit = new AtomicReference<>(GHRateLimit.DEFAULT);\n \n private static final Logger LOGGER = Logger.getLogger(GitHubClient.class.getName());\n \n@@ -141,10 +140,9 @@ public boolean isCredentialValid() {\n getRateLimit();\n return true;\n } catch (IOException e) {\n- if (LOGGER.isLoggable(FINE))\n- LOGGER.log(FINE,\n- \"Exception validating credentials on \" + getApiUrl() + \" with login '\" + login + \"' \" + e,\n- e);\n+ LOGGER.log(FINE,\n+ \"Exception validating credentials on \" + getApiUrl() + \" with login '\" + login + \"' \" + e,\n+ e);\n return false;\n }\n }\n@@ -256,9 +254,7 @@ GHRateLimit getRateLimit(@Nonnull RateLimitTarget rateLimitTarget) throws IOExce\n @Nonnull\n @Deprecated\n GHRateLimit lastRateLimit() {\n- synchronized (rateLimitLock) {\n- return rateLimit;\n- }\n+ return rateLimit.get();\n }\n \n /**\n@@ -278,12 +274,19 @@ GHRateLimit lastRateLimit() {\n */\n @Nonnull\n GHRateLimit rateLimit(@Nonnull RateLimitTarget rateLimitTarget) throws IOException {\n- synchronized (rateLimitLock) {\n- if (rateLimit.getRecord(rateLimitTarget).isExpired()) {\n- getRateLimit(rateLimitTarget);\n+ GHRateLimit result = rateLimit.get();\n+ // Most of the time rate limit is not expired, so try to avoid locking.\n+ if (result.getRecord(rateLimitTarget).isExpired()) {\n+ // if the rate limit is expired, synchronize to ensure\n+ // only one call to getRateLimit() is made to refresh it.\n+ synchronized (this) {\n+ if (rateLimit.get().getRecord(rateLimitTarget).isExpired()) {\n+ getRateLimit(rateLimitTarget);\n+ }\n }\n- return rateLimit;\n+ result = rateLimit.get();\n }\n+ return result;\n }\n \n /**\n@@ -296,15 +299,9 @@ GHRateLimit rateLimit(@Nonnull RateLimitTarget rateLimitTarget) throws IOExcepti\n * {@link GHRateLimit.Record} constructed from the response header information\n */\n private GHRateLimit updateRateLimit(@Nonnull GHRateLimit observed) {\n- synchronized (rateLimitLock) {\n- observed = rateLimit.getMergedRateLimit(observed);\n-\n- if (rateLimit != observed) {\n- rateLimit = observed;\n- LOGGER.log(FINE, \"Rate limit now: {0}\", rateLimit);\n- }\n- return rateLimit;\n- }\n+ GHRateLimit result = rateLimit.accumulateAndGet(observed, (current, x) -> current.getMergedRateLimit(x));\n+ LOGGER.log(FINEST, \"Rate limit now: {0}\", rateLimit.get());\n+ return result;\n }\n \n /**\n@@ -384,11 +381,7 @@ public String getApiUrl() {\n GitHubResponse.ResponseInfo connectorResponse = null;\n try {\n try {\n- if (LOGGER.isLoggable(FINE)) {\n- LOGGER.log(FINE,\n- \"GitHub API request [\" + (login == null ? \"anonymous\" : login) + \"]: \"\n- + request.method() + \" \" + request.url().toString());\n- }\n+ logRequest(request);\n rateLimitChecker.checkRateLimit(this, request);\n \n connectorResponse = getResponseInfo(request);\n@@ -424,6 +417,12 @@ public String getApiUrl() {\n throw new GHIOException(\"Ran out of retries for URL: \" + request.url().toString());\n }\n \n+ private void logRequest(@Nonnull final GitHubRequest request) {\n+ LOGGER.log(FINE,\n+ () -> \"GitHub API request [\" + (login == null ? \"anonymous\" : login) + \"]: \" + request.method() + \" \"\n+ + request.url().toString());\n+ }\n+\n @Nonnull\n protected abstract GitHubResponse.ResponseInfo getResponseInfo(GitHubRequest request) throws IOException;\n \n@@ -437,20 +436,15 @@ public String getApiUrl() {\n // special case handling for 304 unmodified, as the content will be \"\"\n } else if (connectorResponse.statusCode() == HttpURLConnection.HTTP_ACCEPTED) {\n \n- // Response code 202 means data is being generated.\n+ // Response code 202 means data is being generated or an action that can require some time is triggered.\n // This happens in specific cases:\n // statistics - See https://developer.github.com/v3/repos/statistics/#a-word-about-caching\n // fork creation - See https://developer.github.com/v3/repos/forks/#create-a-fork\n+ // workflow run cancellation - See https://docs.github.com/en/rest/reference/actions#cancel-a-workflow-run\n \n- if (connectorResponse.url().toString().endsWith(\"/forks\")) {\n- LOGGER.log(INFO, \"The fork is being created. Please try again in 5 seconds.\");\n- } else if (connectorResponse.url().toString().endsWith(\"/statistics\")) {\n- LOGGER.log(INFO, \"The statistics are being generated. Please try again in 5 seconds.\");\n- } else {\n- LOGGER.log(INFO,\n- \"Received 202 from \" + connectorResponse.url().toString() + \" . Please try again in 5 seconds.\");\n- }\n- // Maybe throw an exception instead?\n+ LOGGER.log(FINE,\n+ \"Received HTTP_ACCEPTED(202) from \" + connectorResponse.url().toString()\n+ + \" . Please try again in 5 seconds.\");\n } else if (handler != null) {\n body = handler.apply(connectorResponse);\n }\n@@ -562,9 +556,7 @@ private void noteRateLimit(@Nonnull GitHubResponse.ResponseInfo connectorResponse) {\n GHRateLimit.Record observed = new GHRateLimit.Record(limit, remaining, reset, connectorResponse);\n updateRateLimit(GHRateLimit.fromRecord(observed, connectorResponse.request().rateLimitTarget()));\n } catch (NumberFormatException | NullPointerException e) {\n- if (LOGGER.isLoggable(FINEST)) {\n- LOGGER.log(FINEST, \"Missing or malformed X-RateLimit header: \", e);\n- }\n+ LOGGER.log(FINEST, \"Missing or malformed X-RateLimit header: \", e);\n }\n }\n " }, { "sha": "aea8b99b1f3301f65c8bc86c657bfd187db5c178", diff --git a/src/test/resources/org/kohsuke/github/GHRepositoryTest/wiremock/getCommitsBetweenPaged/__files/repos_hub4j-test-org_github-api_compare_4261c42949915816a9f246eb14c3dfd21a637bc294ff089e60064bfa43e374baeb10846f7ce82f40-5.json b/src/test/resources/org/kohsuke/github/GHRepositoryTest/wiremock/getCommitsBetweenPaged/__files/repos_hub4j-test-org_github-api_compare_4261c42949915816a9f246eb14c3dfd21a637bc294ff089e60064bfa43e374baeb10846f7ce82f40-5.json index 22c43a022f..3980686fae 100644 --- a/src/test/resources/org/kohsuke/github/GHRepositoryTest/wiremock/getCommitsBetweenPaged/__files/repos_hub4j-test-org_github-api_compare_4261c42949915816a9f246eb14c3dfd21a637bc294ff089e60064bfa43e374baeb10846f7ce82f40-5.json +++ b/src/test/resources/org/kohsuke/github/GHRepositoryTest/wiremock/getCommitsBetweenPaged/__files/repos_hub4j-test-org_github-api_compare_4261c42949915816a9f246eb14c3dfd21a637bc294ff089e60064bfa43e374baeb10846f7ce82f40-5.json @@ -9064,7 +9064,7 @@ "blob_url": "https://github.com/hub4j-test-org/github-api/blob/94ff089e60064bfa43e374baeb10846f7ce82f40/src/main/java/org/kohsuke/github/GitHubClient.java", "raw_url": "https://github.com/hub4j-test-org/github-api/raw/94ff089e60064bfa43e374baeb10846f7ce82f40/src/main/java/org/kohsuke/github/GitHubClient.java", "contents_url": "https://api.github.com/repos/hub4j-test-org/github-api/contents/src/main/java/org/kohsuke/github/GitHubClient.java?ref=94ff089e60064bfa43e374baeb10846f7ce82f40", - "patch": "@@ -14,6 +14,7 @@\n import java.time.format.DateTimeFormatter;\n import java.time.temporal.ChronoUnit;\n import java.util.*;\n+import java.util.concurrent.atomic.AtomicReference;\n import java.util.function.Consumer;\n import java.util.logging.Logger;\n \n@@ -52,10 +53,8 @@\n \n private HttpConnector connector;\n \n- private final Object rateLimitLock = new Object();\n-\n @Nonnull\n- private GHRateLimit rateLimit = GHRateLimit.DEFAULT;\n+ private final AtomicReference rateLimit = new AtomicReference<>(GHRateLimit.DEFAULT);\n \n private static final Logger LOGGER = Logger.getLogger(GitHubClient.class.getName());\n \n@@ -141,10 +140,9 @@ public boolean isCredentialValid() {\n getRateLimit();\n return true;\n } catch (IOException e) {\n- if (LOGGER.isLoggable(FINE))\n- LOGGER.log(FINE,\n- \"Exception validating credentials on \" + getApiUrl() + \" with login '\" + login + \"' \" + e,\n- e);\n+ LOGGER.log(FINE,\n+ \"Exception validating credentials on \" + getApiUrl() + \" with login '\" + login + \"' \" + e,\n+ e);\n return false;\n }\n }\n@@ -256,9 +254,7 @@ GHRateLimit getRateLimit(@Nonnull RateLimitTarget rateLimitTarget) throws IOExce\n @Nonnull\n @Deprecated\n GHRateLimit lastRateLimit() {\n- synchronized (rateLimitLock) {\n- return rateLimit;\n- }\n+ return rateLimit.get();\n }\n \n /**\n@@ -278,12 +274,19 @@ GHRateLimit lastRateLimit() {\n */\n @Nonnull\n GHRateLimit rateLimit(@Nonnull RateLimitTarget rateLimitTarget) throws IOException {\n- synchronized (rateLimitLock) {\n- if (rateLimit.getRecord(rateLimitTarget).isExpired()) {\n- getRateLimit(rateLimitTarget);\n+ GHRateLimit result = rateLimit.get();\n+ // Most of the time rate limit is not expired, so try to avoid locking.\n+ if (result.getRecord(rateLimitTarget).isExpired()) {\n+ // if the rate limit is expired, synchronize to ensure\n+ // only one call to getRateLimit() is made to refresh it.\n+ synchronized (this) {\n+ if (rateLimit.get().getRecord(rateLimitTarget).isExpired()) {\n+ getRateLimit(rateLimitTarget);\n+ }\n }\n- return rateLimit;\n+ result = rateLimit.get();\n }\n+ return result;\n }\n \n /**\n@@ -296,15 +299,9 @@ GHRateLimit rateLimit(@Nonnull RateLimitTarget rateLimitTarget) throws IOExcepti\n * {@link GHRateLimit.Record} constructed from the response header information\n */\n private GHRateLimit updateRateLimit(@Nonnull GHRateLimit observed) {\n- synchronized (rateLimitLock) {\n- observed = rateLimit.getMergedRateLimit(observed);\n-\n- if (rateLimit != observed) {\n- rateLimit = observed;\n- LOGGER.log(FINE, \"Rate limit now: {0}\", rateLimit);\n- }\n- return rateLimit;\n- }\n+ GHRateLimit result = rateLimit.accumulateAndGet(observed, (current, x) -> current.getMergedRateLimit(x));\n+ LOGGER.log(FINEST, \"Rate limit now: {0}\", rateLimit.get());\n+ return result;\n }\n \n /**\n@@ -384,11 +381,7 @@ public String getApiUrl() {\n GitHubResponse.ResponseInfo responseInfo = null;\n try {\n try {\n- if (LOGGER.isLoggable(FINE)) {\n- LOGGER.log(FINE,\n- \"GitHub API request [\" + (login == null ? \"anonymous\" : login) + \"]: \"\n- + request.method() + \" \" + request.url().toString());\n- }\n+ logRequest(request);\n rateLimitChecker.checkRateLimit(this, request);\n \n responseInfo = getResponseInfo(request);\n@@ -424,6 +417,12 @@ public String getApiUrl() {\n throw new GHIOException(\"Ran out of retries for URL: \" + request.url().toString());\n }\n \n+ private void logRequest(@Nonnull final GitHubRequest request) {\n+ LOGGER.log(FINE,\n+ () -> \"GitHub API request [\" + (login == null ? \"anonymous\" : login) + \"]: \" + request.method() + \" \"\n+ + request.url().toString());\n+ }\n+\n @Nonnull\n protected abstract GitHubResponse.ResponseInfo getResponseInfo(GitHubRequest request) throws IOException;\n \n@@ -437,20 +436,15 @@ public String getApiUrl() {\n // special case handling for 304 unmodified, as the content will be \"\"\n } else if (responseInfo.statusCode() == HttpURLConnection.HTTP_ACCEPTED) {\n \n- // Response code 202 means data is being generated.\n+ // Response code 202 means data is being generated or an action that can require some time is triggered.\n // This happens in specific cases:\n // statistics - See https://developer.github.com/v3/repos/statistics/#a-word-about-caching\n // fork creation - See https://developer.github.com/v3/repos/forks/#create-a-fork\n+ // workflow run cancellation - See https://docs.github.com/en/rest/reference/actions#cancel-a-workflow-run\n \n- if (responseInfo.url().toString().endsWith(\"/forks\")) {\n- LOGGER.log(INFO, \"The fork is being created. Please try again in 5 seconds.\");\n- } else if (responseInfo.url().toString().endsWith(\"/statistics\")) {\n- LOGGER.log(INFO, \"The statistics are being generated. Please try again in 5 seconds.\");\n- } else {\n- LOGGER.log(INFO,\n- \"Received 202 from \" + responseInfo.url().toString() + \" . Please try again in 5 seconds.\");\n- }\n- // Maybe throw an exception instead?\n+ LOGGER.log(FINE,\n+ \"Received HTTP_ACCEPTED(202) from \" + responseInfo.url().toString()\n+ + \" . Please try again in 5 seconds.\");\n } else if (handler != null) {\n body = handler.apply(responseInfo);\n }\n@@ -562,9 +556,7 @@ private void noteRateLimit(@Nonnull GitHubResponse.ResponseInfo responseInfo) {\n GHRateLimit.Record observed = new GHRateLimit.Record(limit, remaining, reset, responseInfo);\n updateRateLimit(GHRateLimit.fromRecord(observed, responseInfo.request().rateLimitTarget()));\n } catch (NumberFormatException | NullPointerException e) {\n- if (LOGGER.isLoggable(FINEST)) {\n- LOGGER.log(FINEST, \"Missing or malformed X-RateLimit header: \", e);\n- }\n+ LOGGER.log(FINEST, \"Missing or malformed X-RateLimit header: \", e);\n }\n }\n " + "patch": "@@ -14,6 +14,7 @@\n import java.time.format.DateTimeFormatter;\n import java.time.temporal.ChronoUnit;\n import java.util.*;\n+import java.util.concurrent.atomic.AtomicReference;\n import java.util.function.Consumer;\n import java.util.logging.Logger;\n \n@@ -52,10 +53,8 @@\n \n private HttpConnector connector;\n \n- private final Object rateLimitLock = new Object();\n-\n @Nonnull\n- private GHRateLimit rateLimit = GHRateLimit.DEFAULT;\n+ private final AtomicReference rateLimit = new AtomicReference<>(GHRateLimit.DEFAULT);\n \n private static final Logger LOGGER = Logger.getLogger(GitHubClient.class.getName());\n \n@@ -141,10 +140,9 @@ public boolean isCredentialValid() {\n getRateLimit();\n return true;\n } catch (IOException e) {\n- if (LOGGER.isLoggable(FINE))\n- LOGGER.log(FINE,\n- \"Exception validating credentials on \" + getApiUrl() + \" with login '\" + login + \"' \" + e,\n- e);\n+ LOGGER.log(FINE,\n+ \"Exception validating credentials on \" + getApiUrl() + \" with login '\" + login + \"' \" + e,\n+ e);\n return false;\n }\n }\n@@ -256,9 +254,7 @@ GHRateLimit getRateLimit(@Nonnull RateLimitTarget rateLimitTarget) throws IOExce\n @Nonnull\n @Deprecated\n GHRateLimit lastRateLimit() {\n- synchronized (rateLimitLock) {\n- return rateLimit;\n- }\n+ return rateLimit.get();\n }\n \n /**\n@@ -278,12 +274,19 @@ GHRateLimit lastRateLimit() {\n */\n @Nonnull\n GHRateLimit rateLimit(@Nonnull RateLimitTarget rateLimitTarget) throws IOException {\n- synchronized (rateLimitLock) {\n- if (rateLimit.getRecord(rateLimitTarget).isExpired()) {\n- getRateLimit(rateLimitTarget);\n+ GHRateLimit result = rateLimit.get();\n+ // Most of the time rate limit is not expired, so try to avoid locking.\n+ if (result.getRecord(rateLimitTarget).isExpired()) {\n+ // if the rate limit is expired, synchronize to ensure\n+ // only one call to getRateLimit() is made to refresh it.\n+ synchronized (this) {\n+ if (rateLimit.get().getRecord(rateLimitTarget).isExpired()) {\n+ getRateLimit(rateLimitTarget);\n+ }\n }\n- return rateLimit;\n+ result = rateLimit.get();\n }\n+ return result;\n }\n \n /**\n@@ -296,15 +299,9 @@ GHRateLimit rateLimit(@Nonnull RateLimitTarget rateLimitTarget) throws IOExcepti\n * {@link GHRateLimit.Record} constructed from the response header information\n */\n private GHRateLimit updateRateLimit(@Nonnull GHRateLimit observed) {\n- synchronized (rateLimitLock) {\n- observed = rateLimit.getMergedRateLimit(observed);\n-\n- if (rateLimit != observed) {\n- rateLimit = observed;\n- LOGGER.log(FINE, \"Rate limit now: {0}\", rateLimit);\n- }\n- return rateLimit;\n- }\n+ GHRateLimit result = rateLimit.accumulateAndGet(observed, (current, x) -> current.getMergedRateLimit(x));\n+ LOGGER.log(FINEST, \"Rate limit now: {0}\", rateLimit.get());\n+ return result;\n }\n \n /**\n@@ -384,11 +381,7 @@ public String getApiUrl() {\n GitHubResponse.ResponseInfo connectorResponse = null;\n try {\n try {\n- if (LOGGER.isLoggable(FINE)) {\n- LOGGER.log(FINE,\n- \"GitHub API request [\" + (login == null ? \"anonymous\" : login) + \"]: \"\n- + request.method() + \" \" + request.url().toString());\n- }\n+ logRequest(request);\n rateLimitChecker.checkRateLimit(this, request);\n \n connectorResponse = getResponseInfo(request);\n@@ -424,6 +417,12 @@ public String getApiUrl() {\n throw new GHIOException(\"Ran out of retries for URL: \" + request.url().toString());\n }\n \n+ private void logRequest(@Nonnull final GitHubRequest request) {\n+ LOGGER.log(FINE,\n+ () -> \"GitHub API request [\" + (login == null ? \"anonymous\" : login) + \"]: \" + request.method() + \" \"\n+ + request.url().toString());\n+ }\n+\n @Nonnull\n protected abstract GitHubResponse.ResponseInfo getResponseInfo(GitHubRequest request) throws IOException;\n \n@@ -437,20 +436,15 @@ public String getApiUrl() {\n // special case handling for 304 unmodified, as the content will be \"\"\n } else if (connectorResponse.statusCode() == HttpURLConnection.HTTP_ACCEPTED) {\n \n- // Response code 202 means data is being generated.\n+ // Response code 202 means data is being generated or an action that can require some time is triggered.\n // This happens in specific cases:\n // statistics - See https://developer.github.com/v3/repos/statistics/#a-word-about-caching\n // fork creation - See https://developer.github.com/v3/repos/forks/#create-a-fork\n+ // workflow run cancellation - See https://docs.github.com/en/rest/reference/actions#cancel-a-workflow-run\n \n- if (connectorResponse.url().toString().endsWith(\"/forks\")) {\n- LOGGER.log(INFO, \"The fork is being created. Please try again in 5 seconds.\");\n- } else if (connectorResponse.url().toString().endsWith(\"/statistics\")) {\n- LOGGER.log(INFO, \"The statistics are being generated. Please try again in 5 seconds.\");\n- } else {\n- LOGGER.log(INFO,\n- \"Received 202 from \" + connectorResponse.url().toString() + \" . Please try again in 5 seconds.\");\n- }\n- // Maybe throw an exception instead?\n+ LOGGER.log(FINE,\n+ \"Received HTTP_ACCEPTED(202) from \" + connectorResponse.url().toString()\n+ + \" . Please try again in 5 seconds.\");\n } else if (handler != null) {\n body = handler.apply(connectorResponse);\n }\n@@ -562,9 +556,7 @@ private void noteRateLimit(@Nonnull GitHubResponse.ResponseInfo connectorResponse) {\n GHRateLimit.Record observed = new GHRateLimit.Record(limit, remaining, reset, connectorResponse);\n updateRateLimit(GHRateLimit.fromRecord(observed, connectorResponse.request().rateLimitTarget()));\n } catch (NumberFormatException | NullPointerException e) {\n- if (LOGGER.isLoggable(FINEST)) {\n- LOGGER.log(FINEST, \"Missing or malformed X-RateLimit header: \", e);\n- }\n+ LOGGER.log(FINEST, \"Missing or malformed X-RateLimit header: \", e);\n }\n }\n " }, { "sha": "aea8b99b1f3301f65c8bc86c657bfd187db5c178",