diff --git a/changelog/@unreleased/pr-2437.v2.yml b/changelog/@unreleased/pr-2437.v2.yml new file mode 100644 index 000000000..44605d451 --- /dev/null +++ b/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 diff --git a/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/ApacheHttpClientBlockingChannel.java b/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/ApacheHttpClientBlockingChannel.java index 095a7ea1f..0648149ef 100644 --- a/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/ApacheHttpClientBlockingChannel.java +++ b/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 headers() { if (headers == null) { ListMultimap tmpHeaders = MultimapBuilder.treeKeys(String.CASE_INSENSITIVE_ORDER) - .arrayListValues() + .arrayListValues(1) .build(); Iterator
headerIterator = response.headerIterator(); while (headerIterator.hasNext()) { diff --git a/dialogue-apache-hc5-client/src/test/java/com/palantir/dialogue/hc5/ApacheHttpClientBlockingChannelTest.java b/dialogue-apache-hc5-client/src/test/java/com/palantir/dialogue/hc5/ApacheHttpClientBlockingChannelTest.java new file mode 100644 index 000000000..5439cc6b4 --- /dev/null +++ b/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 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 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; + } + } +}