Fix request request path to always include leading slash /

palantir · Dec 15, 2024 · 647f6c8 · 647f6c8
1 parent 31f619d
commit 647f6c8
Show file tree

Hide file tree

Showing 2 changed files with 96 additions and 3 deletions.
diff --git a/...e-hc5-client/src/main/java/com/palantir/dialogue/hc5/ApacheHttpClientBlockingChannel.java b/...e-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/...5-client/src/test/java/com/palantir/dialogue/hc5/ApacheHttpClientBlockingChannelTest.java b/...5-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<String, String> 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%[email protected], 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<String, String> 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")