ApacheHttpClientBlockingChannel uses URL to build request

changelog/@unreleased/pr-2437.v2.yml

@@ -0,0 +1,5 @@
+type: improvement
+improvement:
+  description: ApacheHttpClientBlockingChannel uses URL to build request.
+  links:
+  - https://github.com/palantir/dialogue/pull/2437

dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/ApacheHttpClientBlockingChannel.java

@@ -15,6 +15,7 @@
  */
 package com.palantir.dialogue.hc5;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ListMultimap;
 import com.google.common.collect.MultimapBuilder;
@@ -33,7 +34,9 @@
 import com.palantir.logsafe.Preconditions;
 import com.palantir.logsafe.SafeArg;
 import com.palantir.logsafe.SafeLoggable;
+import com.palantir.logsafe.UnsafeArg;
 import com.palantir.logsafe.exceptions.SafeExceptions;
+import com.palantir.logsafe.exceptions.SafeIllegalArgumentException;
 import com.palantir.logsafe.exceptions.SafeRuntimeException;
 import com.palantir.logsafe.logger.SafeLogger;
 import com.palantir.logsafe.logger.SafeLoggerFactory;
@@ -45,6 +48,7 @@
 import java.io.InputStream;
 import java.io.OutputStream;
 import java.net.InetAddress;
+import java.net.URISyntaxException;
 import java.net.URL;
 import java.util.Arrays;
 import java.util.Collections;
@@ -62,11 +66,13 @@
 import org.apache.hc.client5.http.impl.classic.CloseableHttpResponse;
 import org.apache.hc.client5.http.protocol.HttpClientContext;
 import org.apache.hc.core5.function.Supplier;
+import org.apache.hc.core5.http.ClassicHttpRequest;
 import org.apache.hc.core5.http.Header;
 import org.apache.hc.core5.http.HttpEntity;
 import org.apache.hc.core5.http.NoHttpResponseException;
 import org.apache.hc.core5.http.io.support.ClassicRequestBuilder;
 import org.apache.hc.core5.http.message.BasicHeader;
+import org.apache.hc.core5.net.URIAuthority;
 
 final class ApacheHttpClientBlockingChannel implements BlockingChannel {
     private static final SafeLogger log = SafeLoggerFactory.get(ApacheHttpClientBlockingChannel.class);
@@ -98,31 +104,12 @@ final class ApacheHttpClientBlockingChannel implements BlockingChannel {
 
     @Override
     public Response execute(Endpoint endpoint, Request request) throws IOException {
-        // Create base request given the URL
-        URL target = baseUrl.render(endpoint, request);
-        ClassicRequestBuilder builder =
-                ClassicRequestBuilder.create(endpoint.httpMethod().name()).setUri(target.toString());
-
-        // Fill headers
-        request.headerParams().forEach(builder::addHeader);
-
-        if (request.body().isPresent()) {
-            Preconditions.checkArgument(
-                    endpoint.httpMethod() != HttpMethod.GET, "GET endpoints must not have a request body");
-            Preconditions.checkArgument(
-                    endpoint.httpMethod() != HttpMethod.HEAD, "HEAD endpoints must not have a request body");
-            Preconditions.checkArgument(
-                    endpoint.httpMethod() != HttpMethod.OPTIONS, "OPTIONS endpoints must not have a request body");
-            RequestBody body = request.body().get();
-            setBody(builder, body);
-        } else if (requiresEmptyBody(endpoint)) {
-            builder.setEntity(EmptyHttpEntity.INSTANCE);
-        }
         long startTime = System.nanoTime();
         try {
+            ClassicHttpRequest httpRequest = createRequest(baseUrl, endpoint, request);
             HttpClientContext context = HttpClientContext.create();
             resolvedHost.ifPresent(inetAddress -> DialogueRoutePlanner.set(context, inetAddress));
-            CloseableHttpResponse httpClientResponse = client.apacheClient().execute(builder.build(), context);
+            CloseableHttpResponse httpClientResponse = client.apacheClient().execute(httpRequest, context);
             // Defensively ensure that resources are closed if failures occur within this block,
             // for example HttpClientResponse allocation may throw an OutOfMemoryError.
             boolean close = true;
@@ -167,6 +154,47 @@ public Response execute(Endpoint endpoint, Request request) throws IOException {
         }
     }
 
+    @VisibleForTesting
+    static ClassicHttpRequest createRequest(BaseUrl baseUrl, Endpoint endpoint, Request request) {
+        // Create base request given the URL
+        URL target = baseUrl.render(endpoint, request);
+        ClassicRequestBuilder builder = ClassicRequestBuilder.create(
+                        endpoint.httpMethod().name())
+                .setScheme(target.getProtocol())
+                .setAuthority(parseAuthority(target))
+                .setPath(target.getFile());
+
+        // Fill headers
+        request.headerParams().forEach(builder::addHeader);
+
+        if (request.body().isPresent()) {
+            Preconditions.checkArgument(
+                    endpoint.httpMethod() != HttpMethod.GET, "GET endpoints must not have a request body");
+            Preconditions.checkArgument(
+                    endpoint.httpMethod() != HttpMethod.HEAD, "HEAD endpoints must not have a request body");
+            Preconditions.checkArgument(
+                    endpoint.httpMethod() != HttpMethod.OPTIONS, "OPTIONS endpoints must not have a request body");
+            setBody(builder, request.body().get());
+        } else if (requiresEmptyBody(endpoint)) {
+            builder.setEntity(EmptyHttpEntity.INSTANCE);
+        }
+        return builder.build();
+    }
+
+    @VisibleForTesting
+    static URIAuthority parseAuthority(URL url) {
+        try {
+            String host = url.getHost();
+            if (host != null && !host.startsWith("[")) {
+                // fast path for non-IPv6 hosts
+                return new URIAuthority(url.getUserInfo(), host, url.getPort());
+            }
+            return URIAuthority.create(url.getAuthority());
+        } catch (URISyntaxException e) {
+            throw new SafeIllegalArgumentException("Invalid URI authority", e, UnsafeArg.of("url", url));
+        }
+    }
+
     private Arg<?>[] failureDiagnosticArgs(Endpoint endpoint, Request request, long startTimeNanos) {
         long durationMillis = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - startTimeNanos);
         return new Arg<?>[] {
@@ -315,7 +343,7 @@ public int code() {
         public ListMultimap<String, String> headers() {
             if (headers == null) {
                 ListMultimap<String, String> tmpHeaders = MultimapBuilder.treeKeys(String.CASE_INSENSITIVE_ORDER)
-                        .arrayListValues()
+                        .arrayListValues(1)
                         .build();
                 Iterator<Header> headerIterator = response.headerIterator();
                 while (headerIterator.hasNext()) {

dialogue-apache-hc5-client/src/test/java/com/palantir/dialogue/hc5/ApacheHttpClientBlockingChannelTest.java

@@ -0,0 +1,182 @@
+/*
+ * (c) Copyright 2024 Palantir Technologies Inc. All rights reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.palantir.dialogue.hc5;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import com.google.common.collect.ListMultimap;
+import com.google.common.primitives.UnsignedBytes;
+import com.palantir.dialogue.Endpoint;
+import com.palantir.dialogue.HttpMethod;
+import com.palantir.dialogue.PathTemplate;
+import com.palantir.dialogue.Request;
+import com.palantir.dialogue.UrlBuilder;
+import com.palantir.dialogue.core.BaseUrl;
+import java.net.InetAddress;
+import java.net.URI;
+import java.net.URL;
+import java.net.UnknownHostException;
+import java.util.Comparator;
+import java.util.Map;
+import org.apache.hc.core5.http.ClassicHttpRequest;
+import org.apache.hc.core5.net.URIAuthority;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.CsvSource;
+import org.junit.jupiter.params.provider.ValueSource;
+
+class ApacheHttpClientBlockingChannelTest {
+
+    @ParameterizedTest
+    @ValueSource(strings = {"GET", "PUT", "POST"})
+    void createRequest(String method) throws Exception {
+
+        BaseUrl baseUrl = BaseUrl.of(new URL("https://www.example.local/foo/api"));
+        Endpoint endpoint = new Endpoint() {
+            private final PathTemplate pathTemplate = PathTemplate.builder()
+                    .fixed("fixed")
+                    .variable("endpoint")
+                    .variable("index")
+                    .build();
+
+            @Override
+            public HttpMethod httpMethod() {
+                return HttpMethod.valueOf(method);
+            }
+
+            @Override
+            public String serviceName() {
+                return "testService";
+            }
+
+            @Override
+            public String endpointName() {
+                return "testEndpoint";
+            }
+
+            @Override
+            public String version() {
+                return "1.2.3";
+            }
+
+            @Override
+            public void renderPath(ListMultimap<String, String> params, UrlBuilder url) {
+                pathTemplate.fill(params, url);
+            }
+        };
+
+        Request request = Request.builder()
+                .pathParams(Map.of("fixed", "bar", "endpoint", "baz", "index", "42"))
+                .putQueryParams("q", "1")
+                .putQueryParams("test", "true")
+                .build();
+
+        ClassicHttpRequest httpRequest = ApacheHttpClientBlockingChannel.createRequest(baseUrl, endpoint, request);
+        assertThat(httpRequest.getMethod()).isEqualTo(method);
+        assertThat(httpRequest.getPath())
+                .isEqualTo(httpRequest.getRequestUri())
+                .isEqualTo("/foo/api/fixed/baz/42?q=1&test=true");
+        assertThat(httpRequest.getUri())
+                .isEqualTo(URI.create("https://www.example.local/foo/api/fixed/baz/42?q=1&test=true"));
+    }
+
+    @ParameterizedTest
+    @CsvSource({
+        "http://example.local, example.local, -1,",
+        "https://example.local, example.local, -1,",
+        "https://localhost:1234, localhost, 1234,",
+        "https://127.0.0.1, 127.0.0.1, -1,",
+        "https://[0:0:0:0:0:ffff:c0a8:0102], 0:0:0:0:0:ffff:c0a8:0102, -1,",
+        "https://[0000:0000:0000:0000:0000:ffff:c0a8:0102], 0000:0000:0000:0000:0000:ffff:c0a8:0102, -1,",
+        "https://[::1], ::1, -1,",
+        "https://[::ffff:c0a8:102], ::ffff:c0a8:102, -1,",
+        "https://127.0.0.1:1234, 127.0.0.1, 1234,",
+        "https://[::1]:1234, ::1, 1234,",
+        "https://www.example.local, www.example.local, -1,",
+        "https://www.example.local:443, www.example.local, 443,",
+        "https://www.example.local/path/to/foo/bar, www.example.local, -1,",
+        "https://www.example.local/path/to/foo/bar?baz=quux&hello=world#hash-octothorpe, www.example.local, -1,",
+        "https://user@www.example.local:8443/path/to/foo/bar?baz=quux&hello=world#hash-octothorpe ,"
+                + " www.example.local, 8443, user",
+        "https://user@[::1]:8443/path/to/foo/bar?baz=quux&hello=world#hash-octothorpe , ::1, 8443, user",
+        "https://user@[0000:0000:0000:0000:0000:ffff:c0a8:0102]:8443/path/to/foo/bar?baz=quux&hello=world#an-octothorpe"
+                + " , 0000:0000:0000:0000:0000:ffff:c0a8:0102, 8443, user",
+        "https://user:slash%2Fslash@www.example.local, www.example.local, -1, user:slash%2Fslash",
+    })
+    void parseAuthority(String input, String expectedHost, int expectedPort, String expectedUserInfo) throws Exception {
+        URL url = new URL(input);
+        assertThat(ApacheHttpClientBlockingChannel.parseAuthority(url))
+                .isEqualTo(URIAuthority.create(url.toURI().getRawAuthority()))
+                .isEqualTo(URIAuthority.create(url.getAuthority()))
+                .satisfies(authority -> {
+                    assertThat(authority.getHostName())
+                            .usingComparator(hostComparator)
+                            .isEqualTo(expectedHost)
+                            .isEqualTo(url.getHost());
+                    assertThat(authority.getPort()).isEqualTo(expectedPort).isEqualTo(url.getPort());
+                    assertThat(authority.getUserInfo())
+                            .isEqualTo(expectedUserInfo)
+                            .isEqualTo(url.getUserInfo());
+                });
+    }
+
+    @Test
+    void testHostComparator() {
+        assertThat("www.example.local")
+                .usingComparator(hostComparator)
+                .isEqualTo("www.example.local")
+                .isNotEqualTo("www.example.com");
+        assertThat("127.0.0.1")
+                .usingComparator(hostComparator)
+                .isEqualTo("127.0.0.1")
+                .isNotEqualTo("127.0.0.2");
+        assertThat("::1")
+                .usingComparator(hostComparator)
+                .isEqualTo("::1")
+                .isEqualTo("[::1]")
+                .isEqualTo("[0000:0000:0000:0000:0000:0000:0000:0001]")
+                .isNotEqualTo("[::2]")
+                .isNotEqualTo("127.0.0.1");
+        assertThat("::ffff:c0a8:102")
+                .usingComparator(hostComparator)
+                .isEqualTo("::ffff:c0a8:102")
+                .isEqualTo("[0000:0000:0000:0000:0000:ffff:c0a8:0102]")
+                .isNotEqualTo("::ffff:c0a8:101")
+                .isNotEqualTo("[::ffff:c0a8:101]");
+    }
+
+    private static final Comparator<? super String> hostComparator = (host1, host2) -> {
+        if (host1.equals(host2)) {
+            return 0;
+        }
+        // treat IPv6 addresses with and without brackets as equivalent
+        InetAddress address1 = tryGetAddress(host1);
+        InetAddress address2 = tryGetAddress(host2);
+        if (address1 != null && address2 != null) {
+            return UnsignedBytes.lexicographicalComparator().compare(address1.getAddress(), address2.getAddress());
+        }
+        return host1.compareTo(host2);
+    };
+
+    private static InetAddress tryGetAddress(String host) {
+        try {
+            return InetAddress.getByName(host);
+        } catch (UnknownHostException e) {
+            return null;
+        }
+    }
+}