Introduce defaultHttp(s)Port for MultiAddressHttpClientBuilder (#2703)

Motivation
----------
At the moment, if no port number is explicitly provided, the
default HTTP and HTTPS ports are used (80 and 443). In some
environments it can be useful (i.e. if a client is always talking
to a high port number) to not have to specify it on every
request.

Modifications
-------------
This changeset introduces two new builder options to allow customizing
the defaultHttpPort as well as the defaultHttpsPort and applying it
if no explicit port is provided on the request itself.

Result
------
It is now possible to not specify the port number on every request
if they diverge from the default 80 or 443 port numbers.

servicetalk-http-api/src/main/java/io/servicetalk/http/api/DelegatingMultiAddressHttpClientBuilder.java

@@ -96,6 +96,18 @@ public MultiAddressHttpClientBuilder<U, R> followRedirects(final RedirectConfig
         return this;
     }
 
+    @Override
+    public MultiAddressHttpClientBuilder<U, R> defaultHttpPort(final int port) {
+        delegate = delegate.defaultHttpPort(port);
+        return this;
+    }
+
+    @Override
+    public MultiAddressHttpClientBuilder<U, R> defaultHttpsPort(final int port) {
+        delegate = delegate.defaultHttpsPort(port);
+        return this;
+    }
+
     @Override
     public HttpClient build() {
         return delegate.build();

servicetalk-http-api/src/main/java/io/servicetalk/http/api/MultiAddressHttpClientBuilder.java

@@ -147,4 +147,28 @@ default MultiAddressHttpClientBuilder<U, R> headersFactory(HttpHeadersFactory he
      * @see RedirectConfigBuilder
      */
     MultiAddressHttpClientBuilder<U, R> followRedirects(@Nullable RedirectConfig config);
+
+    /**
+     * Configures the default port for the HTTP scheme if not explicitly provided as part of the
+     * {@link HttpRequestMetaData#requestTarget()}.
+     *
+     * @param port the port that should be used if not explicitly provided for HTTP requests.
+     * @return {@code this}.
+     */
+    default MultiAddressHttpClientBuilder<U, R> defaultHttpPort(int port) { // FIXME: 0.43 - remove default impl
+        throw new UnsupportedOperationException("Setting defaultHttpPort is not yet supported by "
+                + getClass().getName());
+    }
+
+    /**
+     * Configures the default port for the HTTPS scheme if not explicitly provided as part of the
+     * {@link HttpRequestMetaData#requestTarget()}.
+     *
+     * @param port the port that should be used if not explicitly provided for HTTPS requests.
+     * @return {@code this}.
+     */
+    default MultiAddressHttpClientBuilder<U, R> defaultHttpsPort(int port) { // FIXME: 0.43 - remove default impl
+        throw new UnsupportedOperationException("Setting defaultHttpsPort is not yet supported by "
+                + getClass().getName());
+    }
 }

servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/DefaultMultiAddressUrlHttpClientBuilder.java

@@ -103,6 +103,8 @@ final class DefaultMultiAddressUrlHttpClientBuilder
     private HttpHeadersFactory headersFactory;
     @Nullable
     private RedirectConfig redirectConfig;
+    private int defaultHttpPort = HTTP.port();
+    private int defaultHttpsPort = HTTPS.port();
     @Nullable
     private SingleAddressInitializer<HostAndPort, InetSocketAddress> singleAddressInitializer;
 
@@ -118,7 +120,8 @@ public StreamingHttpClient buildStreaming() {
             final HttpExecutionContext executionContext = executionContextBuilder.build();
             final ClientFactory clientFactory =
                     new ClientFactory(builderFactory, executionContext, singleAddressInitializer);
-            final CachingKeyFactory keyFactory = closeables.prepend(new CachingKeyFactory());
+            final CachingKeyFactory keyFactory = closeables.prepend(
+                    new CachingKeyFactory(defaultHttpPort, defaultHttpsPort));
             final HttpHeadersFactory headersFactory = this.headersFactory;
             FilterableStreamingHttpClient urlClient = closeables.prepend(
                     new StreamingUrlHttpClient(executionContext, keyFactory, clientFactory,
@@ -144,6 +147,13 @@ public StreamingHttpClient buildStreaming() {
     private static final class CachingKeyFactory implements AsyncCloseable {
 
         private final ConcurrentMap<String, UrlKey> urlKeyCache = new ConcurrentHashMap<>();
+        private final int defaultHttpPort;
+        private final int defaultHttpsPort;
+
+        CachingKeyFactory(final int defaultHttpPort, final int defaultHttpsPort) {
+            this.defaultHttpPort = defaultHttpPort;
+            this.defaultHttpsPort = defaultHttpsPort;
+        }
 
         public UrlKey get(final HttpRequestMetaData metaData) throws MalformedURLException {
             final String host = metaData.host();
@@ -161,7 +171,7 @@ public UrlKey get(final HttpRequestMetaData metaData) throws MalformedURLExcepti
 
             final int parsedPort = metaData.port();
             final int port = parsedPort >= 0 ? parsedPort :
-                    (HTTPS_SCHEME.equalsIgnoreCase(scheme) ? HTTPS : HTTP).port();
+                    (HTTPS_SCHEME.equalsIgnoreCase(scheme) ? defaultHttpsPort : defaultHttpPort);
 
             metaData.requestTarget(absoluteToRelativeFormRequestTarget(metaData.requestTarget(), scheme, host));
 
@@ -453,4 +463,29 @@ public MultiAddressHttpClientBuilder<HostAndPort, InetSocketAddress> followRedir
         this.redirectConfig = config;
         return this;
     }
+
+    @Override
+    public MultiAddressHttpClientBuilder<HostAndPort, InetSocketAddress> defaultHttpPort(final int port) {
+        this.defaultHttpPort = verifyPortRange(port);
+        return this;
+    }
+
+    @Override
+    public MultiAddressHttpClientBuilder<HostAndPort, InetSocketAddress> defaultHttpsPort(final int port) {
+        this.defaultHttpsPort = verifyPortRange(port);
+        return this;
+    }
+
+    /**
+     * Verifies that the given port number is within the allowed range.
+     *
+     * @param port the port number to verify.
+     * @return the port number if in range.
+     */
+    private static int verifyPortRange(final int port) {
+        if (port < 1 || port > 0xFFFF) {
+            throw new IllegalArgumentException("Provided port number is out of range (between 1 and 65535): " + port);
+        }
+        return port;
+    }
 }

servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/MultiAddressUrlHttpClientTest.java

@@ -34,8 +34,10 @@
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.Test;
 
+import java.io.IOException;
 import java.net.InetSocketAddress;
 import java.net.MalformedURLException;
+import java.net.ServerSocket;
 import java.net.UnknownHostException;
 import java.util.Collection;
 import java.util.List;
@@ -80,6 +82,7 @@
 
 class MultiAddressUrlHttpClientTest {
 
+    private static final String ID = MultiAddressUrlHttpClientTest.class.getSimpleName();
     private static final String INVALID_HOSTNAME = "invalid.";
     private static final String X_REQUESTED_LOCATION = "X-Requested-Location";
     private static final String X_RECEIVED_REQUEST_TARGET = "X-Received-Request-Target";
@@ -97,8 +100,7 @@ class MultiAddressUrlHttpClientTest {
     static void beforeClass() throws Exception {
         afterClassCloseables = newCompositeCloseable();
 
-        client = afterClassCloseables.append(HttpClients.forMultiAddressUrl(
-                        MultiAddressUrlHttpClientTest.class.getSimpleName())
+        client = afterClassCloseables.append(HttpClients.forMultiAddressUrl(ID)
                 .followRedirects(new RedirectConfigBuilder().allowNonRelativeRedirects(true).build())
                 .initializer((scheme, address, builder) -> builder.serviceDiscoverer(sdThatSupportsInvalidHostname()))
                 .buildStreaming());
@@ -193,25 +195,25 @@ void requestWithAbsoluteFormRequestTargetWithHostHeader() throws Exception {
         StreamingHttpRequest request = client.get(format("http://%s/200?param=value#tag", hostHeader));
         // value in the HOST header should be ignored:
         request.headers().set(HOST, format("%s:%d", INVALID_HOSTNAME, 8080));
-        requestAndValidate(request, OK, "/200?param=value#tag");
+        requestAndValidate(client, request, OK, "/200?param=value#tag");
     }
 
     @Test
     void requestWithAbsoluteFormRequestTargetWithoutHostHeader() throws Exception {
         StreamingHttpRequest request = client.get(format("http://%s/200?param=value#tag", hostHeader));
-        requestAndValidate(request, OK, "/200?param=value#tag");
+        requestAndValidate(client, request, OK, "/200?param=value#tag");
     }
 
     @Test
     void requestWithAbsoluteFormRequestTargetWithoutPath() throws Exception {
         StreamingHttpRequest request = client.get(format("http://%s", hostHeader));
-        requestAndValidate(request, BAD_REQUEST, "/");
+        requestAndValidate(client, request, BAD_REQUEST, "/");
     }
 
     @Test
     void requestWithAbsoluteFormRequestTargetWithoutPathWithParams() throws Exception {
         StreamingHttpRequest request = client.get(format("http://%s?param=value", hostHeader));
-        requestAndValidate(request, BAD_REQUEST, "/?param=value");
+        requestAndValidate(client, request, BAD_REQUEST, "/?param=value");
     }
 
     @Test
@@ -255,14 +257,33 @@ void requestWithAsteriskFormRequestTargetWithHostHeader() {
     void requestWithRedirect() throws Exception {
         StreamingHttpRequest request = client.get(format("http://%s/301", hostHeader))
                 .setHeader(X_REQUESTED_LOCATION, format("http://%s/200", hostHeader));  // Location for redirect
-        requestAndValidate(request, OK, "/200");
+        requestAndValidate(client, request, OK, "/200");
     }
 
     @Test
     void requestWithRelativeRedirect() throws Exception {
         StreamingHttpRequest request = client.get(format("http://%s/301", hostHeader))
                 .setHeader(X_REQUESTED_LOCATION, "/200");  // Location for redirect
-        requestAndValidate(request, OK, "/200");
+        requestAndValidate(client, request, OK, "/200");
+    }
+
+    @Test
+    void requestWithCustomDefaultHttpPort() throws Exception {
+        try (StreamingHttpClient client = HttpClients
+                .forMultiAddressUrl(ID).defaultHttpPort(serverPort).buildStreaming()) {
+            StreamingHttpRequest request = client.get(format("http://%s/200", serverHost));
+            requestAndValidate(client, request, OK, "/200");
+        }
+    }
+
+    @Test
+    void requestWithExplicitPortTakesPrecedenceOverCustomDefaultHttpPort() throws Exception {
+        int illegalPort = availablePort();
+        try (StreamingHttpClient client = HttpClients
+                .forMultiAddressUrl(ID).defaultHttpPort(illegalPort).buildStreaming()) {
+            StreamingHttpRequest request = client.get(format("http://%s:%d/200", serverHost, serverPort));
+            requestAndValidate(client, request, OK, "/200");
+        }
     }
 
     @Test
@@ -278,22 +299,22 @@ void multipleRequestsToMultipleServers() throws Exception {
                     BAD_REQUEST, UNAUTHORIZED, FORBIDDEN,
                     INTERNAL_SERVER_ERROR, NOT_IMPLEMENTED, BAD_GATEWAY);
             for (HttpResponseStatus status : statuses) {
-                makeGetRequestAndValidate(hostHeader, status);
-                makeGetRequestAndValidate(hostHeader2, status);
-                makeGetRequestAndValidate(hostHeader3, status);
+                makeGetRequestAndValidate(client, hostHeader, status);
+                makeGetRequestAndValidate(client, hostHeader2, status);
+                makeGetRequestAndValidate(client, hostHeader3, status);
             }
         }
     }
 
-    private static void makeGetRequestAndValidate(final String hostHeader, final HttpResponseStatus status)
-            throws Exception {
+    private static void makeGetRequestAndValidate(final StreamingHttpClient client, final String hostHeader,
+                                                  final HttpResponseStatus status) throws Exception {
         StreamingHttpRequest request = client.get(format("http://%s/%d?param=value#tag", hostHeader, status.code()));
-        requestAndValidate(request, status, format("/%d?param=value#tag", status.code()));
+        requestAndValidate(client, request, status, format("/%d?param=value#tag", status.code()));
     }
 
-    private static void requestAndValidate(final StreamingHttpRequest request,
-                                           final HttpResponseStatus expectedStatus,
-                                           final String expectedRequestTarget) throws Exception {
+    private static void requestAndValidate(final StreamingHttpClient client, final StreamingHttpRequest request,
+                                           final HttpResponseStatus expectedStatus, final String expectedRequestTarget)
+            throws Exception {
         StreamingHttpResponse response = awaitIndefinitelyNonNull(client.request(request));
         assertThat(response.status(), is(expectedStatus));
         final CharSequence receivedRequestTarget = response.headers().get(X_RECEIVED_REQUEST_TARGET);
@@ -305,4 +326,11 @@ private static ServerContext startNewLocalServer(final StreamingHttpService http
                                                      final CompositeCloseable closeables) throws Exception {
         return closeables.append(HttpServers.forAddress(localAddress(0)).listenStreamingAndAwait(httpService));
     }
+
+    private static int availablePort() throws IOException {
+        ServerSocket s = new ServerSocket(0);
+        int port = s.getLocalPort();
+        s.close();
+        return port;
+    }
 }