Clean up GitHubConnectorResponseHttpUrlConnectionAdapter

bitwiseman · Nov 9, 2021 · 056d1f0 · 056d1f0
1 parent 55a2b24
commit 056d1f0
Show file tree

Hide file tree

Showing 7 changed files with 65 additions and 113 deletions.
diff --git 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
@@ -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
@@ -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/...ctorResponseHttpUrlConnectionAdapter.java → ...ctorResponseHttpUrlConnectionAdapter.java b/...ctorResponseHttpUrlConnectionAdapter.java → ...ctorResponseHttpUrlConnectionAdapter.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<String> 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,49 +185,24 @@ 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
     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<String, List<String>> 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
@@ -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
@@ -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
@@ -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();