Broke up UrlConnectionHttpClient into multiple classes. Hopefully this is an improvement? #3054
Conversation
…s is an improvement?
|
SonarCloud Quality Gate failed. |
| /** | ||
| * A logger used by classes in the URL connection HTTP client package. | ||
| */ | ||
| @SdkInternalApi | ||
| public class UrlConnectionLogger { | ||
| public static final Logger LOG = Logger.loggerFor(UrlConnectionHttpClient.class); |
There was a problem hiding this comment.
Is this needed? Is there any harm in exposing the underlying class name that's logging, even if it's an internal class? It somewhat unifies logs from a user perspective but it also makes things a little bit more difficult to trace.
| * An implementation of {@link HttpConnection} that delegates calls to a {@link HttpURLConnection}. | ||
| */ | ||
| @SdkInternalApi | ||
| public class DefaultHttpConnection implements HttpConnection { |
There was a problem hiding this comment.
Is this a HttpURLConnectionToHttpConnectionAdapter, or just HttpURLConnectionAdapter?
| /** | ||
| * Invoke {@link HttpURLConnection#getHeaderField(String)} | ||
| */ | ||
| String getResponseHeader(String header); |
There was a problem hiding this comment.
Should this return List<String>? Esp. since getResponseHeaders() uses a list.
| /** | ||
| * Invoke {@link HttpURLConnection#getInputStream()} | ||
| */ | ||
| InputStream getResponseStream(); | ||
|
|
||
| /** | ||
| * Invoke {@link HttpURLConnection#getErrorStream()} | ||
| */ | ||
| InputStream getResponseErrorStream(); |
There was a problem hiding this comment.
Do we feel it offers value to distinguish between these two streams? If not, could our own interface just normalize to getResponseStream()?
| * An abstract HTTP connection. Implemented with a {@link HttpURLConnection} in {@link DefaultHttpConnection}. | ||
| */ | ||
| @SdkInternalApi | ||
| public interface HttpConnection { |
There was a problem hiding this comment.
Any value in extending Abortable/AutoCloseable?
| HttpConnection connection = new DefaultHttpConnection(httpUrlConnection); | ||
| connection = new ResponseCodeBugHttpConnection(connection); | ||
| connection = new Expect100BugHttpConnection(connection, executeRequest.httpRequest()); | ||
| this.connection = connection; |
There was a problem hiding this comment.
Very elegant usage of wrapping!
|
Closing as it got stale. |
…de602d052 Pull request: release <- staging/7c012b4d-fe04-4805-bcb0-b56de602d052
No description provided.