Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make HttpClientGitHubConnector the default for Java 11+ #1314

Merged
merged 2 commits into from
Nov 21, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
19 changes: 16 additions & 3 deletions src/main/java/org/kohsuke/github/GitHubClient.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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;
Expand All @@ -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();
Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}

/**
Expand Down
15 changes: 0 additions & 15 deletions src/test/java/org/kohsuke/github/GHWorkflowRunTest.java
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand Down
14 changes: 13 additions & 1 deletion src/test/java/org/kohsuke/github/RequesterRetryTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {

Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down