Merge pull request #1314 from bitwiseman/task/java11default

Make HttpClientGitHubConnector the default for Java 11+
hub4j · Nov 21, 2021 · abf5a6d · abf5a6d
2 parents 65d6de2 + 6c21554
commit abf5a6d
Show file tree

Hide file tree

Showing 7 changed files with 49 additions and 28 deletions.
diff --git a/pom.xml b/pom.xml
@@ -825,6 +825,19 @@
                   <argLine>@{jacoco.surefire.argLine} ${surefire.argLine} -Dtest.github.connector=httpclient</argLine>
                 </configuration>
               </execution>
+              <execution>
+                <id>java11-urlconnection-test</id>
+                <phase>integration-test</phase>
+                <goals>
+                  <goal>test</goal>
+                </goals>
+                <configuration>
+                  <classesDirectory>${project.basedir}/target/github-api-${project.version}.jar</classesDirectory>
+                  <useSystemClassLoader>false</useSystemClassLoader>
+                  <excludesFile>src/test/resources/slow-or-flaky-tests.txt</excludesFile>
+                  <argLine>@{jacoco.surefire.argLine} ${surefire.argLine} -Dtest.github.connector=urlconnection</argLine>
+                </configuration>
+              </execution>
             </executions>
           </plugin>
         </plugins>

diff --git a/src/main/java/org/kohsuke/github/GitHubClient.java b/src/main/java/org/kohsuke/github/GitHubClient.java
@@ -25,8 +25,7 @@
 
 import static com.fasterxml.jackson.annotation.JsonAutoDetect.Visibility.ANY;
 import static com.fasterxml.jackson.annotation.JsonAutoDetect.Visibility.NONE;
-import static java.net.HttpURLConnection.HTTP_BAD_REQUEST;
-import static java.net.HttpURLConnection.HTTP_UNAUTHORIZED;
+import static java.net.HttpURLConnection.*;
 import static java.util.logging.Level.*;
 import static org.apache.commons.lang3.StringUtils.defaultString;
 
@@ -392,7 +391,7 @@ public <T> GitHubResponse<T> sendRequest(GitHubRequest request, @CheckForNull Bo
                     connectorRequest = e.connectorRequest;
                 }
             } catch (SocketException | SocketTimeoutException | SSLHandshakeException e) {
-                // These transient errors the
+                // These transient errors thrown by HttpURLConnection
                 if (retries > 0) {
                     logRetryConnectionError(e, request.url(), retries);
                     continue;
@@ -413,6 +412,7 @@ private void detectKnownErrors(GitHubConnectorResponse connectorResponse,
             boolean detectStatusCodeError) throws IOException {
         detectOTPRequired(connectorResponse);
         detectInvalidCached404Response(connectorResponse, request);
+        detectRedirect(connectorResponse);
         if (rateLimitHandler.isError(connectorResponse)) {
             rateLimitHandler.onError(connectorResponse);
             throw new RetryRequestException();
@@ -425,6 +425,19 @@ private void detectKnownErrors(GitHubConnectorResponse connectorResponse,
         }
     }
 
+    private void detectRedirect(GitHubConnectorResponse connectorResponse) throws IOException {
+        if (connectorResponse.statusCode() == HTTP_MOVED_PERM || connectorResponse.statusCode() == HTTP_MOVED_TEMP) {
+            // GitHubClient depends on GitHubConnector implementations to follow any redirects automatically
+            // If this is not done and a redirect is requested, throw in order to maintain security and consistency
+            throw new HttpException(
+                    "GitHubConnnector did not automatically follow redirect.\n"
+                            + "Change your http client configuration to automatically follow redirects as appropriate.",
+                    connectorResponse.statusCode(),
+                    "Redirect",
+                    connectorResponse.request().url().toString());
+        }
+    }
+
     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

diff --git a/src/main/java/org/kohsuke/github/internal/DefaultGitHubConnector.java b/src/main/java/org/kohsuke/github/internal/DefaultGitHubConnector.java
@@ -50,11 +50,11 @@ static GitHubConnector create(String defaultConnectorProperty) {
         } else if (defaultConnectorProperty.equalsIgnoreCase("httpclient")) {
             return new HttpClientGitHubConnector();
         } else if (defaultConnectorProperty.equalsIgnoreCase("default")) {
-            // try {
-            // return new HttpClientGitHubConnector();
-            // } catch (UnsupportedOperationException | LinkageError e) {
-            return new GitHubConnectorHttpConnectorAdapter(HttpConnector.DEFAULT);
-            // }
+            try {
+                return new HttpClientGitHubConnector();
+            } catch (UnsupportedOperationException | LinkageError e) {
+                return new GitHubConnectorHttpConnectorAdapter(HttpConnector.DEFAULT);
+            }
         } else {
             throw new IllegalStateException(
                     "Property 'test.github.connector' must reference a valid built-in connector - okhttp, okhttpconnector, urlconnection, or default.");

diff --git a/src/main/java11/org/kohsuke/github/extras/HttpClientGitHubConnector.java b/src/main/java11/org/kohsuke/github/extras/HttpClientGitHubConnector.java
@@ -28,10 +28,10 @@ public class HttpClientGitHubConnector implements GitHubConnector {
     private final HttpClient client;
 
     /**
-     * Instantiates a new HttpClientGitHubConnector with a defaut HttpClient.
+     * Instantiates a new HttpClientGitHubConnector with a default HttpClient.
      */
     public HttpClientGitHubConnector() {
-        this(HttpClient.newHttpClient());
+        this(HttpClient.newBuilder().followRedirects(HttpClient.Redirect.NORMAL).build());
     }
 
     /**

diff --git a/src/test/java/org/kohsuke/github/GHWorkflowRunTest.java b/src/test/java/org/kohsuke/github/GHWorkflowRunTest.java
@@ -1,15 +1,12 @@
 package org.kohsuke.github;
 
 import org.awaitility.Awaitility;
-import org.junit.Assume;
 import org.junit.Before;
 import org.junit.Test;
 import org.kohsuke.github.GHWorkflowJob.Step;
 import org.kohsuke.github.GHWorkflowRun.Conclusion;
 import org.kohsuke.github.GHWorkflowRun.Status;
-import org.kohsuke.github.extras.HttpClientGitHubConnector;
 import org.kohsuke.github.function.InputStreamFunction;
-import org.kohsuke.github.internal.DefaultGitHubConnector;
 
 import java.io.IOException;
 import java.time.Duration;
@@ -201,10 +198,6 @@ public void testSearchOnBranch() throws IOException {
 
     @Test
     public void testLogs() throws IOException {
-        // This test fails for HttpClientGitHubConnector.
-        // Not sure why, but it is better to move forward and come back to it later
-        Assume.assumeFalse(DefaultGitHubConnector.create() instanceof HttpClientGitHubConnector);
-
         GHWorkflow workflow = repo.getWorkflow(FAST_WORKFLOW_PATH);
 
         long latestPreexistingWorkflowRunId = getLatestPreexistingWorkflowRunId();
@@ -243,10 +236,6 @@ public void testLogs() throws IOException {
     @SuppressWarnings("resource")
     @Test
     public void testArtifacts() throws IOException {
-        // This test fails for HttpClientGitHubConnector.
-        // Not sure why, but it is better to move forward and come back to it later
-        Assume.assumeFalse(DefaultGitHubConnector.create() instanceof HttpClientGitHubConnector);
-
         GHWorkflow workflow = repo.getWorkflow(ARTIFACTS_WORKFLOW_PATH);
 
         long latestPreexistingWorkflowRunId = getLatestPreexistingWorkflowRunId();
@@ -327,10 +316,6 @@ public void testArtifacts() throws IOException {
 
     @Test
     public void testJobs() throws IOException {
-        // This test fails for HttpClientGitHubConnector.
-        // Not sure why, but it is better to move forward and come back to it later
-        Assume.assumeFalse(DefaultGitHubConnector.create() instanceof HttpClientGitHubConnector);
-
         GHWorkflow workflow = repo.getWorkflow(MULTI_JOBS_WORKFLOW_PATH);
 
         long latestPreexistingWorkflowRunId = getLatestPreexistingWorkflowRunId();

diff --git a/src/test/java/org/kohsuke/github/RequesterRetryTest.java b/src/test/java/org/kohsuke/github/RequesterRetryTest.java
@@ -4,11 +4,14 @@
 import com.github.tomakehurst.wiremock.stubbing.Scenario;
 import okhttp3.ConnectionPool;
 import okhttp3.OkHttpClient;
+import org.junit.Assume;
 import org.junit.Before;
 import org.junit.Ignore;
 import org.junit.Test;
+import org.kohsuke.github.extras.HttpClientGitHubConnector;
 import org.kohsuke.github.extras.ImpatientHttpConnector;
 import org.kohsuke.github.extras.okhttp3.OkHttpGitHubConnector;
+import org.kohsuke.github.internal.DefaultGitHubConnector;
 import org.mockito.Mockito;
 
 import java.io.ByteArrayOutputStream;
@@ -78,7 +81,7 @@ public void resetTestCapturedLog() throws IOException {
         attachLogCapturer();
     }
 
-    @Ignore("Used okhttp3 and this to verify connection closing. To variable for CI system.")
+    @Ignore("Used okhttp3 and this to verify connection closing. Too flaky for CI system.")
     @Test
     public void testGitHubIsApiUrlValid() throws Exception {
 
@@ -109,6 +112,9 @@ public void testGitHubIsApiUrlValid() throws Exception {
     // Issue #539
     @Test
     public void testSocketConnectionAndRetry() throws Exception {
+        // Only implemented for HttpURLConnection connectors
+        Assume.assumeThat(DefaultGitHubConnector.create(), not(instanceOf(HttpClientGitHubConnector.class)));
+
         // CONNECTION_RESET_BY_PEER errors result in two requests each
         // to get this failure for "3" tries we have to do 6 queries.
         this.mockGitHub.apiServer()
@@ -136,6 +142,9 @@ public void testSocketConnectionAndRetry() throws Exception {
     // Issue #539
     @Test
     public void testSocketConnectionAndRetry_StatusCode() throws Exception {
+        // Only implemented for HttpURLConnection connectors
+        Assume.assumeThat(DefaultGitHubConnector.create(), not(instanceOf(HttpClientGitHubConnector.class)));
+
         // CONNECTION_RESET_BY_PEER errors result in two requests each
         // to get this failure for "3" tries we have to do 6 queries.
         this.mockGitHub.apiServer()
@@ -164,6 +173,9 @@ public void testSocketConnectionAndRetry_StatusCode() throws Exception {
 
     @Test
     public void testSocketConnectionAndRetry_Success() throws Exception {
+        // Only implemented for HttpURLConnection connectors
+        Assume.assumeThat(DefaultGitHubConnector.create(), not(instanceOf(HttpClientGitHubConnector.class)));
+
         // CONNECTION_RESET_BY_PEER errors result in two requests each
         // to get this failure for "3" tries we have to do 6 queries.
         // If there are only 5 errors we succeed.

diff --git a/src/test/java/org/kohsuke/github/internal/DefaultGitHubConnectorTest.java b/src/test/java/org/kohsuke/github/internal/DefaultGitHubConnectorTest.java
@@ -37,8 +37,6 @@ public void testCreate() throws Exception {
 
         connector = DefaultGitHubConnector.create("default");
 
-        // Current implementation never uses httpclient for default.
-        usingHttpClient = false;
         if (usingHttpClient) {
             assertThat(connector, instanceOf(HttpClientGitHubConnector.class));
         } else {