diff --git a/pom.xml b/pom.xml index aee98ccb75..0022ff4c2e 100644 --- a/pom.xml +++ b/pom.xml @@ -825,6 +825,19 @@ @{jacoco.surefire.argLine} ${surefire.argLine} -Dtest.github.connector=httpclient + + java11-urlconnection-test + integration-test + + test + + + ${project.basedir}/target/github-api-${project.version}.jar + false + src/test/resources/slow-or-flaky-tests.txt + @{jacoco.surefire.argLine} ${surefire.argLine} -Dtest.github.connector=urlconnection + + diff --git a/src/main/java/org/kohsuke/github/GitHubClient.java b/src/main/java/org/kohsuke/github/GitHubClient.java index fefd6cb0c3..a5db6d4ac7 100644 --- 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 GitHubResponse 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 index 1f7d488036..1b00b01596 100644 --- 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 index 1e2a0cfce0..e8eec47bfe 100644 --- 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 index d204f69b72..d7b5442716 100644 --- 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 index 9e1cc76164..523acff7b4 100644 --- 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 index 7469f64069..e7f66b1ff1 100644 --- 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 {