4.x: Fix resolved URI query params (#8566)

The ClientRequestBase class did not properly resolve URIs when path- and
query parameters were mixed. More specifically, the query parameters
were stripped if the URI template was absolute.

Author: Kay Werndli

webclient/api/src/main/java/io/helidon/webclient/api/ClientRequestBase.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2023 Oracle and/or its affiliates.
+ * Copyright (c) 2023, 2024 Oracle and/or its affiliates.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -37,6 +37,7 @@
 import io.helidon.common.tls.Tls;
 import io.helidon.common.uri.UriEncoding;
 import io.helidon.common.uri.UriFragment;
+import io.helidon.common.uri.UriQuery;
 import io.helidon.http.ClientRequestHeaders;
 import io.helidon.http.Header;
 import io.helidon.http.HeaderNames;
@@ -426,10 +427,19 @@ protected MediaContext mediaContext() {
     protected ClientUri resolveUri(ClientUri toResolve) {
         if (uriTemplate != null) {
             String resolved = resolvePathParams(uriTemplate);
+            URI uri;
             if (skipUriEncoding) {
-                toResolve.resolve(URI.create(resolved));
+                uri = URI.create(resolved);
             } else {
-                toResolve.resolve(URI.create(UriEncoding.encodeUri(resolved)));
+                uri = URI.create(UriEncoding.encodeUri(resolved));
+            }
+            toResolve.resolve(uri);
+
+            if (uri.isAbsolute()) { // query parameters are cleared when resolving against absolute URIs
+                UriQuery query = clientUri.query();
+                if (!query.isEmpty()) {
+                    toResolve.writeableQuery().from(query);
+                }
             }
         }
         return toResolve;

webclient/api/src/test/java/io/helidon/webclient/api/ClientRequestBaseTest.java

@@ -0,0 +1,96 @@
+/*
+ * Copyright (c) 2023, 2024 Oracle and/or its affiliates.
+ *
+ * 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 io.helidon.webclient.api;
+
+import io.helidon.common.uri.UriPath;
+import io.helidon.http.Method;
+import org.junit.jupiter.api.Test;
+
+import java.util.Collections;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+class ClientRequestBaseTest {
+
+    /**
+     * Verify that query parameters are preserved when resolving URI templates (cf. issue #8566).
+     * Make sure to test both absolute and relative URIs as they are handled differently when resolving.
+     */
+    @Test
+    public void resolvedUriTest() {
+        ClientUri uri = new FakeClientRequest()
+                .uri("https://www.example.com/")
+                .queryParam("k", "v").resolvedUri();
+        assertThat(uri.authority(), is("www.example.com:443"));
+        assertThat(uri.host(), is("www.example.com"));
+        assertThat(uri.path(), is(UriPath.root()));
+        assertThat(uri.port(), is(443));
+        assertThat(uri.scheme(), is("https"));
+        assertThat(uri.query().get("k"), is("v"));
+
+        uri = new FakeClientRequest()
+                .uri("https://www.example.com/{path}")
+                .pathParam("path", "p")
+                .queryParam("k", "v").resolvedUri();
+        assertThat(uri.authority(), is("www.example.com:443"));
+        assertThat(uri.host(), is("www.example.com"));
+        assertThat(uri.path(), is(UriPath.create("/p")));
+        assertThat(uri.port(), is(443));
+        assertThat(uri.scheme(), is("https"));
+        assertThat(uri.query().get("k"), is("v"));
+
+        uri = new FakeClientRequest()
+                .uri("example/{path}")
+                .pathParam("path", "p")
+                .queryParam("k", "v").resolvedUri();
+        assertThat(uri.authority(), is("localhost:80"));
+        assertThat(uri.host(), is("localhost"));
+        assertThat(uri.path(), is(UriPath.create("/example/p")));
+        assertThat(uri.port(), is(80));
+        assertThat(uri.scheme(), is("http"));
+        assertThat(uri.query().get("k"), is("v"));
+
+        uri = new FakeClientRequest()
+                .uri("https://www.example.com/p?k={k}")
+                .pathParam("k", "v")
+                .queryParam("k", "v").resolvedUri();
+        assertThat(uri.authority(), is("www.example.com:443"));
+        assertThat(uri.host(), is("www.example.com"));
+        assertThat(uri.path(), is(UriPath.create("/p%3Fk=v")));
+        assertThat(uri.port(), is(443));
+        assertThat(uri.scheme(), is("https"));
+        assertThat(uri.query().get("k"), is("v"));
+    }
+
+
+    private static final class FakeClientRequest extends ClientRequestBase<FakeClientRequest, HttpClientResponse> {
+        private FakeClientRequest() {
+            super(WebClientConfig.create(), null, "fake", Method.GET, ClientUri.create(), Collections.emptyMap());
+        }
+
+        @Override
+        protected HttpClientResponse doSubmit(Object entity) {
+            return null;
+        }
+
+        @Override
+        protected HttpClientResponse doOutputStream(OutputStreamHandler outputStreamHandler) {
+            return null;
+        }
+    }
+}

webclient/api/src/test/java/io/helidon/webclient/api/ClientUriTest.java

@@ -117,4 +117,30 @@ void testResolveQuery() {
         assertThat(clientUri.query().get("filter"), is("a b c"));
         assertThat(clientUri.query().getRaw("filter"), is("a%20b%20c"));
     }
-}
+
+    /**
+     * Verifies that {@link ClientUri#resolve(ClientUri)} retains query parameters but {@link ClientUri#resolve(URI)}
+     * doesn't (which is the same behaviour as {@link URI#resolve(URI)}).
+     */
+    @Test
+    void testResolveQueryParameter() {
+        ClientUri helper = ClientUri.create(URI.create("http://localhost:8080/?k=v"));
+        URI uri = URI.create("https://www.example.com:80");
+        helper.resolve(uri);
+        assertThat(helper.authority(), is("www.example.com:80"));
+        assertThat(helper.host(), is("www.example.com"));
+        assertThat(helper.path(), is(UriPath.root()));
+        assertThat(helper.port(), is(80));
+        assertThat(helper.scheme(), is("https"));
+        assertThat("unexpected query parameter: 'k'", !helper.query().contains("k"));
+
+        helper = ClientUri.create(URI.create("http://localhost:8080/?k=v"));
+        helper.resolve(ClientUri.create(uri));
+        assertThat(helper.authority(), is("www.example.com:80"));
+        assertThat(helper.host(), is("www.example.com"));
+        assertThat(helper.path(), is(UriPath.root()));
+        assertThat(helper.port(), is(80));
+        assertThat(helper.scheme(), is("https"));
+        assertThat(helper.query().get("k"), is("v"));
+    }
+}