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 0648149ef..cb326c195 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 @@ -162,7 +162,7 @@ static ClassicHttpRequest createRequest(BaseUrl baseUrl, Endpoint endpoint, Requ endpoint.httpMethod().name()) .setScheme(target.getProtocol()) .setAuthority(parseAuthority(target)) - .setPath(target.getFile()); + .setPath(getPath(target)); // Fill headers request.headerParams().forEach(builder::addHeader); @@ -181,6 +181,15 @@ static ClassicHttpRequest createRequest(BaseUrl baseUrl, Endpoint endpoint, Requ return builder.build(); } + @VisibleForTesting + static String getPath(URL target) { + String path = target.getFile(); + if (path.startsWith("/")) { + return path; + } + return path.isEmpty() ? "/" : '/' + path; + } + @VisibleForTesting static URIAuthority parseAuthority(URL url) { try { 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 index 5439cc6b4..9205cea88 100644 --- 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 @@ -33,6 +33,7 @@ import java.util.Comparator; import java.util.Map; import org.apache.hc.core5.http.ClassicHttpRequest; +import org.apache.hc.core5.http.io.support.ClassicRequestBuilder; import org.apache.hc.core5.net.URIAuthority; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; @@ -116,9 +117,10 @@ public void renderPath(ListMultimap params, UrlBuilder url) { "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", + "http://localhost:59845/?REQUEST=GetCapabilities&SERVICE=WMS, localhost, 59845, ", + "http://localhost:59845?REQUEST=GetCapabilities&SERVICE=WMS, localhost, 59845, ", }) - void parseAuthority(String input, String expectedHost, int expectedPort, String expectedUserInfo) throws Exception { - URL url = new URL(input); + void parseAuthority(URL url, String expectedHost, int expectedPort, String expectedUserInfo) throws Exception { assertThat(ApacheHttpClientBlockingChannel.parseAuthority(url)) .isEqualTo(URIAuthority.create(url.toURI().getRawAuthority())) .isEqualTo(URIAuthority.create(url.getAuthority())) @@ -134,6 +136,88 @@ void parseAuthority(String input, String expectedHost, int expectedPort, String }); } + @ParameterizedTest + @ValueSource( + strings = { + "https://192.168.1.1", + "https://192.168.1.1/", + "https://localhost", + "https://localhost/", + "https://localhost/?REQUEST=GetCapabilities&SERVICE=WMS", + "https://localhost:12345", + "https://localhost:12345?REQUEST=GetCapabilities&SERVICE=WMS", + "https://localhost:12345/?REQUEST=GetCapabilities&SERVICE=WMS", + "https://www.example.local/path/to/foo/bar?baz=quux&hello=world", + }) + void getPathStartsWithSlash(URL url) { + assertThat(ApacheHttpClientBlockingChannel.getPath(url)) + .isNotNull() + .isNotEmpty() + .startsWith("/"); + } + + @ParameterizedTest + @CsvSource({ + "http://localhost:12345/?REQUEST=GetCapabilities&SERVICE=WMS , http://localhost:12345 , ", + "https://localhost:12345/?REQUEST=GetCapabilities&SERVICE=WMS , https://localhost:12345/ , ", + "https://localhost:12345/api?REQUEST=GetCapabilities&SERVICE=WMS , https://localhost:12345/api , ", + "https://localhost:12345/api?REQUEST=GetCapabilities&SERVICE=WMS , https://localhost:12345/api/ , ", + }) + void noPathQueryString(URL expectedUrl, String base) throws Exception { + Request wmsRequest = Request.builder() + .putQueryParams("REQUEST", "GetCapabilities") + .putQueryParams("SERVICE", "WMS") + .build(); + BaseUrl baseUrl = BaseUrl.of(new URL(base)); + Endpoint wmsEndpoint = new Endpoint() { + private final PathTemplate pathTemplate = PathTemplate.builder() + .variable("REQUEST") + .variable("SERVICE") + .build(); + + @Override + public HttpMethod httpMethod() { + return HttpMethod.GET; + } + + @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); + } + }; + URL target = baseUrl.render(wmsEndpoint, wmsRequest); + assertThat(ApacheHttpClientBlockingChannel.getPath(target)).isNotEmpty().startsWith("/"); + + ClassicHttpRequest expectedRequest = ClassicRequestBuilder.create( + wmsEndpoint.httpMethod().name()) + .setUri(target.toString()) + .build(); + + ClassicHttpRequest request = ApacheHttpClientBlockingChannel.createRequest(baseUrl, wmsEndpoint, wmsRequest); + assertThat(request.getMethod()).isEqualTo(wmsEndpoint.httpMethod().toString()); + assertThat(request.getUri()) + .isEqualTo(expectedUrl.toURI()) + .isEqualTo(expectedRequest.getUri()) + .asString() + .isEqualTo(expectedUrl.toString()); + assertThat(request.getPath()).isEqualTo(expectedRequest.getPath()); + } + @Test void testHostComparator() { assertThat("www.example.local")