Further clean up and coverage

This is why we test.
hub4j · Nov 9, 2021 · cd1faed · cd1faed
1 parent ac9c0ad
commit cd1faed
Show file tree

Hide file tree

Showing 17 changed files with 583 additions and 132 deletions.
diff --git a/pom.xml b/pom.xml
@@ -162,15 +162,13 @@
 
 
                       <!-- Deprecated -->
-                      <exclude>org.kohsuke.github.extras.okhttp3.OkHttpConnector</exclude>
-                      <exclude>org.kohsuke.github.extras.OkHttp3Connector</exclude>
+                      <exclude>org.kohsuke.github.extras.OkHttpConnector</exclude>
                       <exclude>org.kohsuke.github.extras.OkHttp3Connector</exclude>
                       <exclude>org.kohsuke.github.EnforcementLevel</exclude>
                       <exclude>org.kohsuke.github.GHPerson.1</exclude>
                       <exclude>org.kohsuke.github.GHCompare.User</exclude>
 
                       <!-- TODO: Some coverage, but more needed -->
-                      <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>
@@ -587,6 +585,17 @@
                   <argLine>@{jacoco.surefire.argLine} ${surefire.argLine} -Dtest.github.connector=okhttp</argLine>
                 </configuration>
               </execution>
+              <execution>
+                <id>okhttpconnector-test</id>
+                <phase>test</phase>
+                <goals>
+                  <goal>test</goal>
+                </goals>
+                <configuration>
+                  <excludesFile>src/test/resources/slow-or-flaky-tests.txt</excludesFile>
+                  <argLine>@{jacoco.surefire.argLine} ${surefire.argLine} -Dtest.github.connector=okhttpconnector</argLine>
+                </configuration>
+              </execution>
               <execution>
                 <id>slow-or-flaky-test</id>
                 <phase>test</phase>

diff --git a/src/main/java/org/kohsuke/github/AbuseLimitHandler.java b/src/main/java/org/kohsuke/github/AbuseLimitHandler.java
@@ -1,6 +1,7 @@
 package org.kohsuke.github;
 
 import org.kohsuke.github.connector.GitHubConnectorResponse;
+import org.kohsuke.github.internal.GitHubConnectorHttpConnectorAdapter;
 
 import java.io.IOException;
 import java.io.InterruptedIOException;
@@ -41,7 +42,14 @@ public void onError(@Nonnull GitHubConnectorResponse connectorResponse) throws I
                 connectorResponse.statusCode(),
                 connectorResponse.header("Status"),
                 connectorResponse.request().url().toString()).withResponseHeaderFields(connectorResponse.allHeaders());
-        onError(e, new GitHubConnectorResponseHttpUrlConnectionAdapter(connectorResponse));
+        HttpURLConnection connection;
+        if (connectorResponse instanceof GitHubConnectorHttpConnectorAdapter.HttpURLConnectionGitHubConnectorResponse) {
+            connection = ((GitHubConnectorHttpConnectorAdapter.HttpURLConnectionGitHubConnectorResponse) connectorResponse)
+                    .getConnection();
+        } else {
+            connection = new GitHubConnectorResponseHttpUrlConnectionAdapter(connectorResponse);
+        }
+        onError(e, connection);
     }
 
     /**

diff --git a/src/main/java/org/kohsuke/github/GHCompare.java b/src/main/java/org/kohsuke/github/GHCompare.java
@@ -3,7 +3,6 @@
 import com.fasterxml.jackson.annotation.JacksonInject;
 import com.infradna.tool.bridge_method_injector.WithBridgeMethods;
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
-import org.jetbrains.annotations.NotNull;
 
 import java.io.IOException;
 import java.net.URL;
@@ -174,7 +173,7 @@ public PagedIterable<Commit> listCommits() {
         } else {
             // if not using paginated commits, adapt the returned commits array
             return new PagedIterable<Commit>() {
-                @NotNull
+                @Nonnull
                 @Override
                 public PagedIterator<Commit> _iterator(int pageSize) {
                     return new PagedIterator<>(Collections.singleton(commits).iterator(), null);

diff --git a/src/main/java/org/kohsuke/github/GitHub.java b/src/main/java/org/kohsuke/github/GitHub.java
@@ -488,7 +488,7 @@ public HttpConnector getConnector() {
      * @deprecated HttpConnector should not be changed. If you find yourself needing to do this, file an issue.
      */
     @Deprecated
-    public void setConnector(HttpConnector connector) {
+    public void setConnector(@Nonnull HttpConnector connector) {
         client.setConnector(GitHubConnectorHttpConnectorAdapter.adapt(connector));
     }
 

diff --git a/src/main/java/org/kohsuke/github/GitHubBuilder.java b/src/main/java/org/kohsuke/github/GitHubBuilder.java
@@ -334,7 +334,7 @@ public GitHubBuilder withJwtToken(String jwtToken) {
      *            the connector
      * @return the git hub builder
      */
-    public GitHubBuilder withConnector(HttpConnector connector) {
+    public GitHubBuilder withConnector(@Nonnull HttpConnector connector) {
         return withConnector(GitHubConnectorHttpConnectorAdapter.adapt(connector));
     }
 

diff --git a/src/main/java/org/kohsuke/github/GitHubConnectorResponseHttpUrlConnectionAdapter.java b/src/main/java/org/kohsuke/github/GitHubConnectorResponseHttpUrlConnectionAdapter.java
@@ -280,7 +280,7 @@ public String getRequestProperty(String key) {
 
     @Override
     public Map<String, List<String>> getRequestProperties() {
-        return connectorResponse.request().allHeaders();
+        throw new IllegalStateException("Already connected");
     }
 
     @Override

diff --git a/src/main/java/org/kohsuke/github/RateLimitHandler.java b/src/main/java/org/kohsuke/github/RateLimitHandler.java
@@ -1,6 +1,7 @@
 package org.kohsuke.github;
 
 import org.kohsuke.github.connector.GitHubConnectorResponse;
+import org.kohsuke.github.internal.GitHubConnectorHttpConnectorAdapter;
 
 import java.io.IOException;
 import java.io.InterruptedIOException;
@@ -37,7 +38,14 @@ public void onError(@Nonnull GitHubConnectorResponse connectorResponse) throws I
                 connectorResponse.statusCode(),
                 connectorResponse.header("Status"),
                 connectorResponse.request().url().toString()).withResponseHeaderFields(connectorResponse.allHeaders());
-        onError(e, new GitHubConnectorResponseHttpUrlConnectionAdapter(connectorResponse));
+        HttpURLConnection connection;
+        if (connectorResponse instanceof GitHubConnectorHttpConnectorAdapter.HttpURLConnectionGitHubConnectorResponse) {
+            connection = ((GitHubConnectorHttpConnectorAdapter.HttpURLConnectionGitHubConnectorResponse) connectorResponse)
+                    .getConnection();
+        } else {
+            connection = new GitHubConnectorResponseHttpUrlConnectionAdapter(connectorResponse);
+        }
+        onError(e, connection);
     }
 
     /**

diff --git a/src/main/java/org/kohsuke/github/extras/okhttp3/OkHttpGitHubConnector.java b/src/main/java/org/kohsuke/github/extras/okhttp3/OkHttpGitHubConnector.java
@@ -7,9 +7,11 @@
 import org.kohsuke.github.connector.GitHubConnectorRequest;
 import org.kohsuke.github.connector.GitHubConnectorResponse;
 
+import java.io.ByteArrayInputStream;
 import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.io.InputStream;
+import java.nio.charset.StandardCharsets;
 import java.util.Arrays;
 import java.util.List;
 import java.util.Map;
@@ -112,6 +114,9 @@ private List<ConnectionSpec> TlsConnectionSpecs() {
      */
     static class OkHttpGitHubConnectorResponse extends GitHubConnectorResponse {
 
+        private boolean bodyBytesRead = false;
+        private byte[] bodyBytes = null;
+
         @Nonnull
         private final Response response;
 
@@ -135,9 +140,27 @@ public InputStream bodyStream() throws IOException {
                 }
             }
 
-            ResponseBody body = response.body();
-            InputStream bytes = body != null ? body.byteStream() : null;
-            return wrapStream(bytes);
+            readBodyBytes();
+            InputStream stream = bodyBytes == null ? null : new ByteArrayInputStream(bodyBytes);
+            return stream;
+        }
+
+        private void readBodyBytes() throws IOException {
+            synchronized (this) {
+                if (!bodyBytesRead) {
+                    try (ResponseBody body = response.body()) {
+                        if (body != null) {
+                            try (InputStream stream = wrapStream(body.byteStream())) {
+                                if (stream != null) {
+                                    bodyBytes = IOUtils.toByteArray(stream);
+                                }
+                            }
+                        }
+                    }
+                    bodyBytesRead = true;
+                }
+
+            }
         }
 
         /**
@@ -147,8 +170,8 @@ public String errorMessage() {
             String result = null;
             try {
                 if (!response.isSuccessful()) {
-                    ResponseBody body = response.body();
-                    result = body != null ? body.string() : null;
+                    readBodyBytes();
+                    result = bodyBytes == null ? null : new String(bodyBytes, StandardCharsets.UTF_8);
                 }
             } catch (Exception e) {
                 LOGGER.log(FINER, "Ignored exception get error message", e);

diff --git a/src/main/java/org/kohsuke/github/internal/DefaultGitHubConnector.java b/src/main/java/org/kohsuke/github/internal/DefaultGitHubConnector.java
@@ -3,6 +3,7 @@
 import okhttp3.OkHttpClient;
 import org.kohsuke.github.HttpConnector;
 import org.kohsuke.github.connector.GitHubConnector;
+import org.kohsuke.github.extras.okhttp3.OkHttpConnector;
 import org.kohsuke.github.extras.okhttp3.OkHttpGitHubConnector;
 
 /**
@@ -29,15 +30,21 @@ public class DefaultGitHubConnector {
      */
     public static GitHubConnector create() {
         String defaultConnectorProperty = System.getProperty("test.github.connector", "default");
+        return create(defaultConnectorProperty);
+    }
+
+    static GitHubConnector create(String defaultConnectorProperty) {
+
         if (defaultConnectorProperty.equalsIgnoreCase("okhttp")) {
             return new OkHttpGitHubConnector(new OkHttpClient.Builder().build());
-        } else if (defaultConnectorProperty.equalsIgnoreCase("httpconnector")) {
-            return new GitHubConnectorHttpConnectorAdapter(HttpConnector.DEFAULT);
-        } else if (defaultConnectorProperty.equalsIgnoreCase("default")) {
+        } else if (defaultConnectorProperty.equalsIgnoreCase("okhttpconnector")) {
+            return new GitHubConnectorHttpConnectorAdapter(new OkHttpConnector(new OkHttpClient.Builder().build()));
+        } else if (defaultConnectorProperty.equalsIgnoreCase("urlconnection")
+                || 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.");
+                    "Property 'test.github.connector' must reference a valid built-in connector - okhttp, okhttpconnector, urlconnection, or default.");
         }
     }
 }
diff --git a/src/main/java/org/kohsuke/github/internal/GitHubConnectorHttpConnectorAdapter.java b/src/main/java/org/kohsuke/github/internal/GitHubConnectorHttpConnectorAdapter.java
@@ -1,13 +1,14 @@
 package org.kohsuke.github.internal;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 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.ByteArrayInputStream;
+import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.io.InputStream;
 import java.lang.reflect.Field;
@@ -22,6 +23,8 @@
 
 import javax.annotation.Nonnull;
 
+import static java.net.HttpURLConnection.HTTP_BAD_REQUEST;
+import static java.net.HttpURLConnection.HTTP_NOT_FOUND;
 import static java.util.logging.Level.*;
 
 /**
@@ -31,6 +34,9 @@
  */
 public final class GitHubConnectorHttpConnectorAdapter implements GitHubConnector, HttpConnector {
 
+    /**
+     * Internal for testing.
+     */
     final HttpConnector httpConnector;
 
     /**
@@ -52,8 +58,8 @@ public GitHubConnectorHttpConnectorAdapter(HttpConnector httpConnector) {
      *            the HttpConnector to be adapted.
      * @return a GitHubConnector that calls into the provided HttpConnector.
      */
-    @NotNull
-    public static GitHubConnector adapt(HttpConnector connector) {
+    @Nonnull
+    public static GitHubConnector adapt(@Nonnull HttpConnector connector) {
         GitHubConnector gitHubConnector;
         if (connector == HttpConnector.DEFAULT) {
             gitHubConnector = GitHubConnector.DEFAULT;
@@ -152,8 +158,6 @@ private static void setRequestMethod(String method, HttpURLConnection connection
      * initially received and before the body is processed.
      *
      * Implementation specific to {@link HttpURLConnection}. For internal use only.
-     *
-     *
      */
     public final static class HttpURLConnectionGitHubConnectorResponse extends GitHubConnectorResponse {
 
@@ -177,14 +181,25 @@ public final static class HttpURLConnectionGitHubConnectorResponse extends GitHu
          * {@inheritDoc}
          */
         public InputStream bodyStream() throws IOException {
+            if (connection.getResponseCode() >= HTTP_BAD_REQUEST) {
+                if (connection.getResponseCode() == HTTP_NOT_FOUND) {
+                    throw new FileNotFoundException(request().url().toString());
+                } else {
+                    throw new HttpException(errorMessage(),
+                            connection.getResponseCode(),
+                            connection.getResponseMessage(),
+                            connection.getURL().toString());
+                }
+            }
+
             synchronized (this) {
                 if (!inputStreamRead) {
                     try (InputStream stream = wrapStream(connection.getInputStream())) {
                         if (stream != null) {
                             inputBytes = IOUtils.toByteArray(stream);
-                            inputStreamRead = true;
                         }
                     }
+                    inputStreamRead = true;
                 }
             }
 
@@ -202,9 +217,9 @@ public String errorMessage() {
                         try (InputStream stream = wrapStream(connection.getErrorStream())) {
                             if (stream != null) {
                                 errorString = new String(IOUtils.toByteArray(stream), StandardCharsets.UTF_8);
-                                errorStreamRead = true;
                             }
                         }
+                        errorStreamRead = true;
                     }
                 }
                 if (errorString != null) {
@@ -216,6 +231,13 @@ public String errorMessage() {
             return result;
         }
 
+        @SuppressFBWarnings(value = { "EI_EXPOSE_REP" },
+                justification = "Internal implementation class. Should not be used externally.")
+        @Nonnull
+        public HttpURLConnection getConnection() {
+            return connection;
+        }
+
         /**
          * Handles the "Content-Encoding" header.
          *