Clean up and code coverage

Author: bitwiseman

pom.xml

@@ -165,7 +165,7 @@
                       <exclude>org.kohsuke.github.GHCompare.User</exclude>
 
                       <!-- TODO: Some coverage, but more needed -->
-                      <exclude>org.kohsuke.github.GitHubResponseInfoHttpURLConnectionAdapter</exclude>
+                      <exclude>org.kohsuke.github.GitHubConnectorResponseHttpUrlConnectionAdapter</exclude>
                       <exclude>org.kohsuke.github.GHPullRequestReviewBuilder.DraftReviewComment</exclude>
                       <exclude>org.kohsuke.github.GHIssue.PullRequest</exclude>
                       <exclude>org.kohsuke.github.GHCommitSearchBuilder</exclude>

src/main/java/org/kohsuke/github/AbuseLimitHandler.java

@@ -6,6 +6,8 @@
 import java.io.InterruptedIOException;
 import java.net.HttpURLConnection;
 
+import javax.annotation.Nonnull;
+
 /**
  * Pluggable strategy to determine what to do when the API abuse limit is hit.
  *
@@ -34,12 +36,12 @@ public abstract class AbuseLimitHandler {
      *      with abuse rate limits</a>
      *
      */
-    void onError(GitHubConnectorResponse connectorResponse) throws IOException {
+    public void onError(@Nonnull GitHubConnectorResponse connectorResponse) throws IOException {
         GHIOException e = new HttpException("Abuse limit violation",
                 connectorResponse.statusCode(),
                 connectorResponse.header("Status"),
-                connectorResponse.url().toString()).withResponseHeaderFields(connectorResponse.allHeaders());
-        onError(e, new GitHubResponseInfoHttpURLConnectionAdapter(connectorResponse));
+                connectorResponse.request().url().toString()).withResponseHeaderFields(connectorResponse.allHeaders());
+        onError(e, new GitHubConnectorResponseHttpUrlConnectionAdapter(connectorResponse));
     }
 
     /**
@@ -63,7 +65,9 @@ void onError(GitHubConnectorResponse connectorResponse) throws IOException {
      *      with abuse rate limits</a>
      *
      */
-    public abstract void onError(IOException e, HttpURLConnection uc) throws IOException;
+    @Deprecated
+    public void onError(IOException e, HttpURLConnection uc) throws IOException {
+    }
 
     /**
      * Wait until the API abuse "wait time" is passed.

src/main/java/org/kohsuke/github/GitHub.java

@@ -473,7 +473,7 @@ public boolean isOffline() {
      *
      * @return the connector
      * @deprecated HttpConnector has been replaced by GitHubConnector which is generally not useful outside of this
-     *             library. If you are using
+     *             library. If you are using this method, file an issue describing your use case.
      */
     @Deprecated
     public HttpConnector getConnector() {

src/main/java/org/kohsuke/github/GitHubClient.java

@@ -6,6 +6,7 @@
 import org.kohsuke.github.authorization.AuthorizationProvider;
 import org.kohsuke.github.authorization.UserAuthorizationProvider;
 import org.kohsuke.github.connector.GitHubConnector;
+import org.kohsuke.github.connector.GitHubConnectorRequest;
 import org.kohsuke.github.connector.GitHubConnectorResponse;
 import org.kohsuke.github.function.BodyHandler;
 
@@ -368,24 +369,26 @@ public <T> GitHubResponse<T> sendRequest(@Nonnull GitHubRequest.Builder<?> build
     public <T> GitHubResponse<T> sendRequest(GitHubRequest request, @CheckForNull BodyHandler<T> handler)
             throws IOException {
         int retries = CONNECTION_ERROR_RETRIES;
-        request = prepareRequest(request);
+        GitHubConnectorRequest connectorRequest = prepareConnectorRequest(request);
+
         do {
             // if we fail to create a connection we do not retry and we do not wrap
 
             GitHubConnectorResponse connectorResponse = null;
             try {
                 try {
-                    logRequest(request);
+                    logRequest(connectorRequest);
                     rateLimitChecker.checkRateLimit(this, request.rateLimitTarget());
-                    connectorResponse = connector.send(request);
+                    connectorResponse = connector.send(connectorRequest);
                     noteRateLimit(request.rateLimitTarget(), connectorResponse);
                     detectOTPRequired(connectorResponse);
 
                     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();
+                        connectorRequest = prepareConnectorRequest(
+                                request.toBuilder().setHeader("Cache-Control", "no-cache").build());
                         continue;
                     }
                     if (!(isRateLimitResponse(connectorResponse) || isAbuseLimitResponse(connectorResponse))) {
@@ -397,7 +400,7 @@ public <T> GitHubResponse<T> sendRequest(GitHubRequest request, @CheckForNull Bo
                         continue;
                     }
 
-                    throw interpretApiError(e, request, connectorResponse);
+                    throw interpretApiError(e, connectorRequest, connectorResponse);
                 }
 
                 handleLimitingErrors(connectorResponse);
@@ -410,7 +413,7 @@ public <T> GitHubResponse<T> sendRequest(GitHubRequest request, @CheckForNull Bo
         throw new GHIOException("Ran out of retries for URL: " + request.url().toString());
     }
 
-    private GitHubRequest prepareRequest(GitHubRequest request) throws IOException {
+    private GitHubConnectorRequest prepareConnectorRequest(GitHubRequest request) throws IOException {
         GitHubRequest.Builder<?> builder = request.toBuilder();
         // if the authentication is needed but no credential is given, try it anyway (so that some calls
         // that do work with anonymous access in the reduced form should still work.)
@@ -442,7 +445,7 @@ private GitHubRequest prepareRequest(GitHubRequest request) throws IOException {
         return builder.build();
     }
 
-    private void logRequest(@Nonnull final GitHubRequest request) {
+    private void logRequest(@Nonnull final GitHubConnectorRequest request) {
         LOGGER.log(FINE,
                 () -> "GitHub API request [" + (getLogin() == null ? "anonymous" : getLogin()) + "]: "
                         + request.method() + " " + request.url().toString());
@@ -471,7 +474,7 @@ private static <T> GitHubResponse<T> createResponse(@Nonnull GitHubConnectorResp
             // workflow run cancellation - See https://docs.github.com/en/rest/reference/actions#cancel-a-workflow-run
 
             LOGGER.log(FINE,
-                    "Received HTTP_ACCEPTED(202) from " + connectorResponse.url().toString()
+                    "Received HTTP_ACCEPTED(202) from " + connectorResponse.request().url().toString()
                             + " . Please try again in 5 seconds.");
         } else if (handler != null) {
             body = handler.apply(connectorResponse);
@@ -483,7 +486,7 @@ private static <T> GitHubResponse<T> createResponse(@Nonnull GitHubConnectorResp
      * Handle API error by either throwing it or by returning normally to retry.
      */
     private static IOException interpretApiError(IOException e,
-            @Nonnull GitHubRequest request,
+            @Nonnull GitHubConnectorRequest connectorRequest,
             @CheckForNull GitHubConnectorResponse connectorResponse) throws IOException {
         // If we're already throwing a GHIOException, pass through
         if (e instanceof GHIOException) {
@@ -508,12 +511,12 @@ private static IOException interpretApiError(IOException e,
                 e = new GHFileNotFoundException(e.getMessage() + " " + errorMessage, e)
                         .withResponseHeaderFields(headers);
             } else if (statusCode >= 0) {
-                e = new HttpException(errorMessage, statusCode, message, request.url().toString(), e);
+                e = new HttpException(errorMessage, statusCode, message, connectorRequest.url().toString(), e);
             } else {
                 e = new GHIOException(errorMessage).withResponseHeaderFields(headers);
             }
         } else if (!(e instanceof FileNotFoundException)) {
-            e = new HttpException(statusCode, message, request.url().toString(), e);
+            e = new HttpException(statusCode, message, connectorRequest.url().toString(), e);
         }
         return e;
     }
@@ -548,7 +551,7 @@ private static boolean retryConnectionError(IOException e, URL url, int retries)
 
     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
+        // When the Requester detects a 404 response with an ETag (only happens when the server's 304
         // is bogus and would cause cache corruption), try the query again with new request header
         // that forces the server to not return 304 and return new data instead.
         //
@@ -561,7 +564,7 @@ private static boolean isInvalidCached404Response(GitHubConnectorResponse connec
                 && connectorResponse.header("ETag") != null
                 && !Objects.equals(connectorResponse.request().header("Cache-Control"), "no-cache")) {
             LOGGER.log(FINE,
-                    "Encountered GitHub invalid cached 404 from " + connectorResponse.url()
+                    "Encountered GitHub invalid cached 404 from " + connectorResponse.request().url()
                             + ". Retrying with \"Cache-Control\"=\"no-cache\"...");
             return true;
         }
@@ -735,7 +738,11 @@ static ObjectReader getMappingObjectReader(@CheckForNull GitHubConnectorResponse
 
         if (connectorResponse != null) {
             injected.put(GitHubConnectorResponse.class.getName(), connectorResponse);
-            injected.putAll(connectorResponse.request().injectedMappingValues());
+            GitHubConnectorRequest request = connectorResponse.request();
+            // This is cheating, but it is an acceptable cheat for now.
+            if (request instanceof GitHubRequest) {
+                injected.putAll(((GitHubRequest) connectorResponse.request()).injectedMappingValues());
+            }
         }
         return MAPPER.reader(new InjectableValues.Std(injected));
     }

src/main/java/org/kohsuke/github/GitHubResponseInfoHttpURLConnectionAdapter.java renamed to src/main/java/org/kohsuke/github/GitHubConnectorResponseHttpUrlConnectionAdapter.java

@@ -8,20 +8,19 @@
 import java.io.OutputStream;
 import java.net.HttpURLConnection;
 import java.net.ProtocolException;
-import java.net.URL;
 import java.nio.charset.StandardCharsets;
 import java.security.Permission;
 import java.util.*;
 
-class GitHubResponseInfoHttpURLConnectionAdapter extends HttpURLConnection {
+class GitHubConnectorResponseHttpUrlConnectionAdapter extends HttpURLConnection {
 
     private static final Comparator<String> nullableCaseInsensitiveComparator = Comparator
             .nullsFirst(String.CASE_INSENSITIVE_ORDER);
 
     private final GitHubConnectorResponse connectorResponse;
 
-    public GitHubResponseInfoHttpURLConnectionAdapter(GitHubConnectorResponse connectorResponse) {
-        super(connectorResponse.url());
+    public GitHubConnectorResponseHttpUrlConnectionAdapter(GitHubConnectorResponse connectorResponse) {
+        super(connectorResponse.request().url());
         this.connectorResponse = connectorResponse;
     }
 
@@ -121,11 +120,6 @@ public int getReadTimeout() {
         return super.getReadTimeout();
     }
 
-    @Override
-    public URL getURL() {
-        return connectorResponse.url();
-    }
-
     @Override
     public int getContentLength() {
         long l = getContentLengthLong();

src/main/java/org/kohsuke/github/GitHubResponse.java

@@ -4,14 +4,12 @@
 import com.fasterxml.jackson.databind.InjectableValues;
 import com.fasterxml.jackson.databind.JsonMappingException;
 import org.apache.commons.io.IOUtils;
-import org.kohsuke.github.connector.GitHubConnectorRequest;
 import org.kohsuke.github.connector.GitHubConnectorResponse;
 
 import java.io.IOException;
 import java.io.InputStreamReader;
 import java.lang.reflect.Array;
 import java.net.HttpURLConnection;
-import java.net.URL;
 import java.nio.charset.StandardCharsets;
 import java.util.*;
 import java.util.logging.Level;
@@ -35,9 +33,6 @@ class GitHubResponse<T> {
 
     private final int statusCode;
 
-    @Nonnull
-    private final GitHubConnectorRequest request;
-
     @Nonnull
     private final Map<String, List<String>> headers;
 
@@ -46,14 +41,12 @@ class GitHubResponse<T> {
 
     GitHubResponse(GitHubResponse<T> response, @CheckForNull T body) {
         this.statusCode = response.statusCode();
-        this.request = response.request();
         this.headers = response.headers;
         this.body = body;
     }
 
     GitHubResponse(GitHubConnectorResponse connectorResponse, @CheckForNull T body) {
         this.statusCode = connectorResponse.statusCode();
-        this.request = connectorResponse.request();
         this.headers = connectorResponse.allHeaders();
         this.body = body;
     }
@@ -138,26 +131,6 @@ static String getBodyAsString(GitHubConnectorResponse connectorResponse) throws
         return IOUtils.toString(r);
     }
 
-    /**
-     * The {@link URL} for this response.
-     *
-     * @return the {@link URL} for this response.
-     */
-    @Nonnull
-    public URL url() {
-        return request.url();
-    }
-
-    /**
-     * The {@link GitHubConnectorRequest} for this response.
-     *
-     * @return the {@link GitHubConnectorRequest} for this response.
-     */
-    @Nonnull
-    public GitHubConnectorRequest request() {
-        return request;
-    }
-
     /**
      * The status code for this response.
      *

src/main/java/org/kohsuke/github/RateLimitHandler.java

@@ -6,6 +6,8 @@
 import java.io.InterruptedIOException;
 import java.net.HttpURLConnection;
 
+import javax.annotation.Nonnull;
+
 /**
  * Pluggable strategy to determine what to do when the API rate limit is reached.
  *
@@ -30,12 +32,12 @@ public abstract class RateLimitHandler {
      *             the io exception
      * @see <a href="https://developer.github.com/v3/#rate-limiting">API documentation from GitHub</a>
      */
-    void onError(GitHubConnectorResponse connectorResponse) throws IOException {
+    public void onError(@Nonnull GitHubConnectorResponse connectorResponse) throws IOException {
         GHIOException e = new HttpException("Rate limit violation",
                 connectorResponse.statusCode(),
                 connectorResponse.header("Status"),
-                connectorResponse.url().toString()).withResponseHeaderFields(connectorResponse.allHeaders());
-        onError(e, new GitHubResponseInfoHttpURLConnectionAdapter(connectorResponse));
+                connectorResponse.request().url().toString()).withResponseHeaderFields(connectorResponse.allHeaders());
+        onError(e, new GitHubConnectorResponseHttpUrlConnectionAdapter(connectorResponse));
     }
 
     /**
@@ -55,7 +57,9 @@ void onError(GitHubConnectorResponse connectorResponse) throws IOException {
      *             the io exception
      * @see <a href="https://developer.github.com/v3/#rate-limiting">API documentation from GitHub</a>
      */
-    public abstract void onError(IOException e, HttpURLConnection uc) throws IOException;
+    @Deprecated
+    public void onError(IOException e, HttpURLConnection uc) throws IOException {
+    }
 
     /**
      * Block until the API rate limit is reset. Useful for long-running batch processing.

src/main/java/org/kohsuke/github/authorization/OrgAppInstallationAuthorizationProvider.java

@@ -19,7 +19,7 @@ public class OrgAppInstallationAuthorizationProvider extends GitHub.DependentAut
 
     private final String organizationName;
 
-    private String latestToken;
+    private String authorization;
 
     @Nonnull
     private Instant validUntil = Instant.MIN;
@@ -44,19 +44,20 @@ public OrgAppInstallationAuthorizationProvider(String organizationName,
     @Override
     public String getEncodedAuthorization() throws IOException {
         synchronized (this) {
-            if (latestToken == null || Instant.now().isAfter(this.validUntil)) {
-                refreshToken();
+            if (authorization == null || Instant.now().isAfter(this.validUntil)) {
+                String token = refreshToken();
+                authorization = String.format("token %s", token);
             }
-            return String.format("token %s", latestToken);
+            return authorization;
         }
     }
 
-    private void refreshToken() throws IOException {
+    private String refreshToken() throws IOException {
         GitHub gitHub = this.gitHub();
         GHAppInstallation installationByOrganization = gitHub.getApp()
                 .getInstallationByOrganization(this.organizationName);
         GHAppInstallationToken ghAppInstallationToken = installationByOrganization.createToken().create();
         this.validUntil = ghAppInstallationToken.getExpiresAt().toInstant().minus(Duration.ofMinutes(5));
-        this.latestToken = Objects.requireNonNull(ghAppInstallationToken.getToken());
+        return Objects.requireNonNull(ghAppInstallationToken.getToken());
     }
 }