From 056d1f0f623cd78c8c4d12ac8984bcc952aa17de Mon Sep 17 00:00:00 2001 From: Liam Newman Date: Tue, 9 Nov 2021 14:54:53 -0800 Subject: [PATCH] Clean up GitHubConnectorResponseHttpUrlConnectionAdapter --- .../org/kohsuke/github/AbuseLimitHandler.java | 10 +-- .../org/kohsuke/github/RateLimitHandler.java | 10 +-- .../connector/GitHubConnectorResponse.java | 14 ++++ ...ectorResponseHttpUrlConnectionAdapter.java | 73 +------------------ .../internal/DefaultGitHubConnector.java | 5 +- .../GitHubConnectorHttpConnectorAdapter.java | 4 +- .../kohsuke/github/AbuseLimitHandlerTest.java | 62 ++++++++++------ 7 files changed, 65 insertions(+), 113 deletions(-) rename src/main/java/org/kohsuke/github/{ => connector}/GitHubConnectorResponseHttpUrlConnectionAdapter.java (75%) diff --git a/src/main/java/org/kohsuke/github/AbuseLimitHandler.java b/src/main/java/org/kohsuke/github/AbuseLimitHandler.java index 41c3428852..dfbee7f1da 100644 --- a/src/main/java/org/kohsuke/github/AbuseLimitHandler.java +++ b/src/main/java/org/kohsuke/github/AbuseLimitHandler.java @@ -1,7 +1,6 @@ package org.kohsuke.github; import org.kohsuke.github.connector.GitHubConnectorResponse; -import org.kohsuke.github.internal.GitHubConnectorHttpConnectorAdapter; import java.io.IOException; import java.io.InterruptedIOException; @@ -42,14 +41,7 @@ public void onError(@Nonnull GitHubConnectorResponse connectorResponse) throws I connectorResponse.statusCode(), connectorResponse.header("Status"), connectorResponse.request().url().toString()).withResponseHeaderFields(connectorResponse.allHeaders()); - HttpURLConnection connection; - if (connectorResponse instanceof GitHubConnectorHttpConnectorAdapter.HttpURLConnectionGitHubConnectorResponse) { - connection = ((GitHubConnectorHttpConnectorAdapter.HttpURLConnectionGitHubConnectorResponse) connectorResponse) - .getConnection(); - } else { - connection = new GitHubConnectorResponseHttpUrlConnectionAdapter(connectorResponse); - } - onError(e, connection); + onError(e, connectorResponse.toHttpURLConnection()); } /** diff --git a/src/main/java/org/kohsuke/github/RateLimitHandler.java b/src/main/java/org/kohsuke/github/RateLimitHandler.java index d74071093f..ce7e7be6c6 100644 --- a/src/main/java/org/kohsuke/github/RateLimitHandler.java +++ b/src/main/java/org/kohsuke/github/RateLimitHandler.java @@ -1,7 +1,6 @@ package org.kohsuke.github; import org.kohsuke.github.connector.GitHubConnectorResponse; -import org.kohsuke.github.internal.GitHubConnectorHttpConnectorAdapter; import java.io.IOException; import java.io.InterruptedIOException; @@ -38,14 +37,7 @@ public void onError(@Nonnull GitHubConnectorResponse connectorResponse) throws I connectorResponse.statusCode(), connectorResponse.header("Status"), connectorResponse.request().url().toString()).withResponseHeaderFields(connectorResponse.allHeaders()); - HttpURLConnection connection; - if (connectorResponse instanceof GitHubConnectorHttpConnectorAdapter.HttpURLConnectionGitHubConnectorResponse) { - connection = ((GitHubConnectorHttpConnectorAdapter.HttpURLConnectionGitHubConnectorResponse) connectorResponse) - .getConnection(); - } else { - connection = new GitHubConnectorResponseHttpUrlConnectionAdapter(connectorResponse); - } - onError(e, connection); + onError(e, connectorResponse.toHttpURLConnection()); } /** diff --git a/src/main/java/org/kohsuke/github/connector/GitHubConnectorResponse.java b/src/main/java/org/kohsuke/github/connector/GitHubConnectorResponse.java index 4d4c134505..dedc8495fe 100644 --- a/src/main/java/org/kohsuke/github/connector/GitHubConnectorResponse.java +++ b/src/main/java/org/kohsuke/github/connector/GitHubConnectorResponse.java @@ -5,6 +5,7 @@ import java.io.Closeable; import java.io.IOException; import java.io.InputStream; +import java.net.HttpURLConnection; import java.util.*; import javax.annotation.CheckForNull; @@ -43,6 +44,19 @@ protected GitHubConnectorResponse(@Nonnull GitHubConnectorRequest request, this.headers = Collections.unmodifiableMap(caseInsensitiveMap); } + /** + * Get this response as a {@link HttpURLConnection}. + * + * @return an object that implements at least the response related methods of {@link HttpURLConnection}. + */ + @Deprecated + @Nonnull + public HttpURLConnection toHttpURLConnection() { + HttpURLConnection connection; + connection = new GitHubConnectorResponseHttpUrlConnectionAdapter(this); + return connection; + } + /** * Gets the value of a header field for this response. * diff --git a/src/main/java/org/kohsuke/github/GitHubConnectorResponseHttpUrlConnectionAdapter.java b/src/main/java/org/kohsuke/github/connector/GitHubConnectorResponseHttpUrlConnectionAdapter.java similarity index 75% rename from src/main/java/org/kohsuke/github/GitHubConnectorResponseHttpUrlConnectionAdapter.java rename to src/main/java/org/kohsuke/github/connector/GitHubConnectorResponseHttpUrlConnectionAdapter.java index b462168a20..1b4b0d6392 100644 --- a/src/main/java/org/kohsuke/github/GitHubConnectorResponseHttpUrlConnectionAdapter.java +++ b/src/main/java/org/kohsuke/github/connector/GitHubConnectorResponseHttpUrlConnectionAdapter.java @@ -1,26 +1,21 @@ -package org.kohsuke.github; - -import org.kohsuke.github.connector.GitHubConnectorResponse; +package org.kohsuke.github.connector; import java.io.ByteArrayInputStream; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; import java.net.HttpURLConnection; -import java.net.ProtocolException; import java.nio.charset.StandardCharsets; import java.security.Permission; import java.util.*; class GitHubConnectorResponseHttpUrlConnectionAdapter extends HttpURLConnection { - private static final Comparator nullableCaseInsensitiveComparator = Comparator - .nullsFirst(String.CASE_INSENSITIVE_ORDER); - private final GitHubConnectorResponse connectorResponse; public GitHubConnectorResponseHttpUrlConnectionAdapter(GitHubConnectorResponse connectorResponse) { super(connectorResponse.request().url()); + this.connected = true; this.connectorResponse = connectorResponse; } @@ -30,21 +25,6 @@ public String getHeaderFieldKey(int n) { return keys.get(n); } - @Override - public void setFixedLengthStreamingMode(int contentLength) { - throw new UnsupportedOperationException(); - } - - @Override - public void setFixedLengthStreamingMode(long contentLength) { - throw new UnsupportedOperationException(); - } - - @Override - public void setChunkedStreamingMode(int chunklen) { - throw new UnsupportedOperationException(); - } - @Override public String getHeaderField(int n) { return connectorResponse.header(getHeaderFieldKey(n)); @@ -60,11 +40,6 @@ public boolean getInstanceFollowRedirects() { throw new UnsupportedOperationException(); } - @Override - public void setRequestMethod(String method) throws ProtocolException { - throw new UnsupportedOperationException(); - } - @Override public String getRequestMethod() { return connectorResponse.request().method(); @@ -210,12 +185,7 @@ public OutputStream getOutputStream() throws IOException { @Override public String toString() { - return connectorResponse.toString(); - } - - @Override - public void setDoInput(boolean doinput) { - throw new UnsupportedOperationException(); + return this.getClass().getName() + ": " + connectorResponse.toString(); } @Override @@ -223,36 +193,16 @@ public boolean getDoInput() { throw new UnsupportedOperationException(); } - @Override - public void setDoOutput(boolean dooutput) { - throw new UnsupportedOperationException(); - } - @Override public boolean getDoOutput() { throw new UnsupportedOperationException(); } - @Override - public void setAllowUserInteraction(boolean allowuserinteraction) { - throw new UnsupportedOperationException(); - } - - @Override - public void setUseCaches(boolean usecaches) { - throw new UnsupportedOperationException(); - } - @Override public boolean getUseCaches() { throw new UnsupportedOperationException(); } - @Override - public void setIfModifiedSince(long ifmodifiedsince) { - throw new UnsupportedOperationException(); - } - @Override public long getIfModifiedSince() { return getHeaderFieldDate("If-Modified-Since", 0); @@ -263,26 +213,11 @@ public void setDefaultUseCaches(boolean defaultusecaches) { throw new UnsupportedOperationException(); } - @Override - public void setRequestProperty(String key, String value) { - throw new UnsupportedOperationException(); - } - - @Override - public void addRequestProperty(String key, String value) { - throw new UnsupportedOperationException(); - } - @Override public String getRequestProperty(String key) { return connectorResponse.request().header(key); } - @Override - public Map> getRequestProperties() { - throw new IllegalStateException("Already connected"); - } - @Override public boolean getAllowUserInteraction() { throw new UnsupportedOperationException(); @@ -305,6 +240,6 @@ public boolean usingProxy() { @Override public void connect() throws IOException { - throw new UnsupportedOperationException(); + // no op } } diff --git a/src/main/java/org/kohsuke/github/internal/DefaultGitHubConnector.java b/src/main/java/org/kohsuke/github/internal/DefaultGitHubConnector.java index 302014f9f1..c355c54494 100644 --- a/src/main/java/org/kohsuke/github/internal/DefaultGitHubConnector.java +++ b/src/main/java/org/kohsuke/github/internal/DefaultGitHubConnector.java @@ -11,7 +11,10 @@ * * Allow behavior to be changed for different version of Java, such as supporting Java 11 HttpClient. */ -public class DefaultGitHubConnector { +public final class DefaultGitHubConnector { + + private DefaultGitHubConnector() { + } /** * Creates a {@link GitHubConnector} that will be used as the default connector. diff --git a/src/main/java/org/kohsuke/github/internal/GitHubConnectorHttpConnectorAdapter.java b/src/main/java/org/kohsuke/github/internal/GitHubConnectorHttpConnectorAdapter.java index b6ca7e9da1..54c736afd1 100644 --- a/src/main/java/org/kohsuke/github/internal/GitHubConnectorHttpConnectorAdapter.java +++ b/src/main/java/org/kohsuke/github/internal/GitHubConnectorHttpConnectorAdapter.java @@ -234,7 +234,9 @@ public String errorMessage() { @SuppressFBWarnings(value = { "EI_EXPOSE_REP" }, justification = "Internal implementation class. Should not be used externally.") @Nonnull - public HttpURLConnection getConnection() { + @Override + @Deprecated + public HttpURLConnection toHttpURLConnection() { return connection; } diff --git a/src/test/java/org/kohsuke/github/AbuseLimitHandlerTest.java b/src/test/java/org/kohsuke/github/AbuseLimitHandlerTest.java index f3533b7d49..694a44faa6 100644 --- a/src/test/java/org/kohsuke/github/AbuseLimitHandlerTest.java +++ b/src/test/java/org/kohsuke/github/AbuseLimitHandlerTest.java @@ -9,6 +9,7 @@ import java.io.IOException; import java.io.InputStream; import java.net.HttpURLConnection; +import java.net.ProtocolException; import java.nio.charset.StandardCharsets; import java.util.Date; import java.util.Map; @@ -102,13 +103,25 @@ public void onError(IOException e, HttpURLConnection uc) throws IOException { Assert.assertThrows(IllegalStateException.class, () -> uc.getRequestProperties()); - // disconnect does nothing, never throws - uc.disconnect(); - uc.disconnect(); - - if (uc instanceof GitHubConnectorResponseHttpUrlConnectionAdapter) { - Assert.assertThrows(UnsupportedOperationException.class, () -> uc.connect()); + // Actions that are not allowed because connection already opened. + Assert.assertThrows(IllegalStateException.class, () -> uc.addRequestProperty("bogus", "item")); + + Assert.assertThrows(IllegalStateException.class, () -> uc.setAllowUserInteraction(true)); + Assert.assertThrows(IllegalStateException.class, () -> uc.setChunkedStreamingMode(1)); + Assert.assertThrows(IllegalStateException.class, () -> uc.setDoInput(true)); + Assert.assertThrows(IllegalStateException.class, () -> uc.setDoOutput(true)); + Assert.assertThrows(IllegalStateException.class, () -> uc.setFixedLengthStreamingMode(1)); + Assert.assertThrows(IllegalStateException.class, () -> uc.setFixedLengthStreamingMode(1L)); + Assert.assertThrows(IllegalStateException.class, () -> uc.setIfModifiedSince(1L)); + Assert.assertThrows(IllegalStateException.class, () -> uc.setRequestProperty("bogus", "thing")); + Assert.assertThrows(IllegalStateException.class, () -> uc.setUseCaches(true)); + + boolean isAdapter = false; + if (uc.toString().contains("GitHubConnectorResponseHttpUrlConnectionAdapter")) { + isAdapter = true; + } + if (isAdapter) { Assert.assertThrows(UnsupportedOperationException.class, () -> uc.getAllowUserInteraction()); Assert.assertThrows(UnsupportedOperationException.class, () -> uc.getConnectTimeout()); @@ -125,32 +138,33 @@ public void onError(IOException e, HttpURLConnection uc) throws IOException { Assert.assertThrows(UnsupportedOperationException.class, () -> uc.getUseCaches()); Assert.assertThrows(UnsupportedOperationException.class, () -> uc.usingProxy()); - Assert.assertThrows(UnsupportedOperationException.class, - () -> uc.addRequestProperty("bogus", "item")); - Assert.assertThrows(UnsupportedOperationException.class, - () -> uc.setAllowUserInteraction(true)); - Assert.assertThrows(UnsupportedOperationException.class, - () -> uc.setChunkedStreamingMode(1)); Assert.assertThrows(UnsupportedOperationException.class, () -> uc.setConnectTimeout(10)); Assert.assertThrows(UnsupportedOperationException.class, () -> uc.setDefaultUseCaches(true)); - Assert.assertThrows(UnsupportedOperationException.class, () -> uc.setDoInput(true)); - Assert.assertThrows(UnsupportedOperationException.class, () -> uc.setDoOutput(true)); - Assert.assertThrows(UnsupportedOperationException.class, - () -> uc.setFixedLengthStreamingMode(1)); - Assert.assertThrows(UnsupportedOperationException.class, - () -> uc.setFixedLengthStreamingMode(1L)); - Assert.assertThrows(UnsupportedOperationException.class, () -> uc.setIfModifiedSince(1L)); + Assert.assertThrows(UnsupportedOperationException.class, () -> uc.setInstanceFollowRedirects(true)); Assert.assertThrows(UnsupportedOperationException.class, () -> uc.setReadTimeout(10)); - Assert.assertThrows(UnsupportedOperationException.class, () -> uc.setRequestMethod("GET")); - Assert.assertThrows(UnsupportedOperationException.class, - () -> uc.setRequestProperty("bogus", "thing")); - Assert.assertThrows(UnsupportedOperationException.class, () -> uc.setUseCaches(true)); + Assert.assertThrows(ProtocolException.class, () -> uc.setRequestMethod("GET")); + } else { + uc.getDefaultUseCaches(); + assertThat(uc.getDoInput(), is(true)); + + // Depending on the underlying implementation, this may throw or not + // Assert.assertThrows(IllegalStateException.class, () -> uc.setRequestMethod("GET")); } - RateLimitHandler.FAIL.onError(e, uc); + // ignored + uc.connect(); + + // disconnect does nothing, never throws + uc.disconnect(); + uc.disconnect(); + + // ignored + uc.connect(); + + AbuseLimitHandler.FAIL.onError(e, uc); } }) .build();