Repository: helidon-io/helidon (java)

Issue:
UriQueryWriteable.fromQueryString does not support multi-value parameters
## Environment Details
* Helidon Version: 4.0.10
* Helidon SE

----------

## Problem Description

When using a client to make a request with multi-value query params, only the last value is retained.

## Steps to reproduce

```java
@Test
void testMultiValueQueryParam() {
    try (Http1ClientResponse response = client.get("/greet")
            .queryParam("foo", "bar1")
            .queryParam("foo", "bar2")
            .request()) {

        assertThat(response.lastEndpointUri().query().all("foo"), is(List.of("bar1", "bar2")));
        // fails because we get ["bar2"]
    }
}
```

It boils down to `UriQueryWriteable.fromQueryString`:

```java
@Test
void testFromQueryWithMultiValueParameter() {
    UriQueryWriteable uriQuery = UriQueryWriteable.create();
    uriQuery.fromQueryString("foo=bar1&foo=bar2");
    assertThat(uriQuery.all("foo"), is(List.of("bar1", "bar2"))); 
    // fails because we get ["bar2"]
}
```



Pull request:
Fix multi-value query string parsing
Resolves #8886 

When parsing a query with multiple values, we incorrectly called `set` instead of `add`, so only the last value was used.

Also the query must be encoded when used for parsing - fixed javadoc, and usage.

Patch:
diff --git a/common/uri/src/main/java/io/helidon/common/uri/UriQueryWriteable.java b/common/uri/src/main/java/io/helidon/common/uri/UriQueryWriteable.java
index c831889631..12310b8749 100644
--- a/common/uri/src/main/java/io/helidon/common/uri/UriQueryWriteable.java
+++ b/common/uri/src/main/java/io/helidon/common/uri/UriQueryWriteable.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2022, 2023 Oracle and/or its affiliates.
+ * Copyright (c) 2022, 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.
@@ -85,8 +85,11 @@ public interface UriQueryWriteable extends UriQuery {
     UriQueryWriteable remove(String name, Consumer<List<String>> removedConsumer);
 
     /**
-     * Update from a query string (with decoded values).
-     * @param queryString decoded query string to update this instance
+     * Update from a query string (with <b>encoded</b> values).
+     * <p>
+     * This documentation (and behavior) has been changed, as we cannot create a proper query from {@code decoded} values,
+     *  as these may contain characters used to split the query.
+     * @param queryString encoded query string to update this instance
      */
     void fromQueryString(String queryString);
 
diff --git a/common/uri/src/main/java/io/helidon/common/uri/UriQueryWriteableImpl.java b/common/uri/src/main/java/io/helidon/common/uri/UriQueryWriteableImpl.java
index 2dc04a5167..b6c2ba45aa 100644
--- a/common/uri/src/main/java/io/helidon/common/uri/UriQueryWriteableImpl.java
+++ b/common/uri/src/main/java/io/helidon/common/uri/UriQueryWriteableImpl.java
@@ -292,11 +292,21 @@ final class UriQueryWriteableImpl implements UriQueryWriteable {
     private void addRaw(String next) {
         int eq = next.indexOf('=');
         if (eq == -1) {
-            set(next);
+            addRaw(next, "");
         } else {
             String name = next.substring(0, eq);
             String value = next.substring(eq + 1);
-            set(name, value);
+            addRaw(name, value);
         }
     }
+
+    private void addRaw(String encodedName, String encodedValue) {
+        String decodedName = UriEncoding.decodeUri(encodedName);
+        String decodedValue = UriEncoding.decodeUri(encodedValue);
+
+        rawQueryParams.computeIfAbsent(encodedName, it -> new ArrayList<>(1))
+                .add(encodedValue);
+        decodedQueryParams.computeIfAbsent(decodedName, it -> new ArrayList<>(1))
+                .add(decodedValue);
+    }
 }
diff --git a/webclient/api/src/main/java/io/helidon/webclient/api/ClientUri.java b/webclient/api/src/main/java/io/helidon/webclient/api/ClientUri.java
index 9903579584..c0f5403f52 100644
--- a/webclient/api/src/main/java/io/helidon/webclient/api/ClientUri.java
+++ b/webclient/api/src/main/java/io/helidon/webclient/api/ClientUri.java
@@ -18,7 +18,6 @@ package io.helidon.webclient.api;
 
 import java.net.URI;
 
-import io.helidon.common.uri.UriEncoding;
 import io.helidon.common.uri.UriFragment;
 import io.helidon.common.uri.UriInfo;
 import io.helidon.common.uri.UriPath;
@@ -194,11 +193,10 @@ public class ClientUri implements UriInfo {
 
         uriBuilder.path(resolvePath(uriBuilder.path().path(), uri.getPath()));
 
-        String queryString = uri.getQuery();
+        String queryString = uri.getRawQuery();
         if (queryString != null) {
             // class URI does not decode +'s, so we do it here
-            query.fromQueryString(queryString.indexOf('+') >= 0 ? UriEncoding.decodeUri(queryString)
-                    : queryString);
+            query.fromQueryString(queryString.replaceAll("\\+", "%20"));
         }
 
         if (uri.getRawFragment() != null) {


Test patch:
diff --git a/common/uri/src/test/java/io/helidon/common/uri/UriQueryTest.java b/common/uri/src/test/java/io/helidon/common/uri/UriQueryTest.java
index edb01dd62b..d9dc16fed2 100644
--- a/common/uri/src/test/java/io/helidon/common/uri/UriQueryTest.java
+++ b/common/uri/src/test/java/io/helidon/common/uri/UriQueryTest.java
@@ -57,7 +57,7 @@ class UriQueryTest {
         assertThat(uriQuery.get("h"), is("xc#e<"));
         assertThat(uriQuery.get("a"), is("b&c=d"));
     }
-    
+
     @Test
     void testEmptyQueryString() {
         UriQuery uriQuery = UriQuery.create("");
@@ -80,10 +80,23 @@ class UriQueryTest {
         UriQuery uriQuery = UriQuery.create(URI.create("http://foo/bar?a&b=c"));
         OptionalValue<String> optional = uriQuery.first("a");
         assertThat(optional.isEmpty(), is(true));
-        
+
         assertThat(uriQuery.all("a"), hasItems());
         assertThat(uriQuery.all("b"), hasItems("c"));
         assertThat(uriQuery.getRaw("a"), is(""));
     }
 
+    @Test
+    void testFromQueryString() {
+        UriQueryWriteable query = UriQueryWriteable.create();
+        query.fromQueryString("p1=v1&p2=v2&p3=%2F%2Fv3%2F%2F&p4=a%20b%20c");
+        assertThat(query.get("p1"), is("v1"));
+        assertThat(query.get("p2"), is("v2"));
+        assertThat(query.get("p3"), is("//v3//"));
+        // make sure the encoded value is correct
+        assertThat(query.getRaw("p3"), is("%2F%2Fv3%2F%2F"));
+        assertThat(query.get("p4"), is("a b c"));
+        assertThat(query.getRaw("p4"), is("a%20b%20c"));
+    }
+
 }
\ No newline at end of file
diff --git a/webclient/api/src/test/java/io/helidon/webclient/api/ClientUriTest.java b/webclient/api/src/test/java/io/helidon/webclient/api/ClientUriTest.java
index 56c29a00d7..a670bc4558 100644
--- a/webclient/api/src/test/java/io/helidon/webclient/api/ClientUriTest.java
+++ b/webclient/api/src/test/java/io/helidon/webclient/api/ClientUriTest.java
@@ -20,8 +20,10 @@ import java.net.URI;
 
 import io.helidon.common.uri.UriPath;
 import io.helidon.common.uri.UriQueryWriteable;
+
 import org.junit.jupiter.api.Test;
 
+import static org.hamcrest.CoreMatchers.hasItems;
 import static org.hamcrest.CoreMatchers.is;
 import static org.hamcrest.MatcherAssert.assertThat;
 
@@ -63,7 +65,12 @@ class ClientUriTest {
     void testQueryParams() {
         UriQueryWriteable query = UriQueryWriteable.create();
         query.fromQueryString("p1=v1&p2=v2&p3=%2F%2Fv3%2F%2F");
-        ClientUri helper = ClientUri.create(URI.create("http://localhost:8080/loom/quick?" + query.value()));
+        assertThat(query.get("p1"), is("v1"));
+        assertThat(query.get("p2"), is("v2"));
+        assertThat(query.get("p3"), is("//v3//"));
+        assertThat(query.getRaw("p3"), is("%2F%2Fv3%2F%2F"));
+
+        ClientUri helper = ClientUri.create(URI.create("http://localhost:8080/loom/quick?" + query.rawValue()));
 
         assertThat(helper.authority(), is("localhost:8080"));
         assertThat(helper.host(), is("localhost"));
@@ -76,6 +83,13 @@ class ClientUriTest {
         assertThat(helper.query().getRaw("p3"), is("%2F%2Fv3%2F%2F"));
     }
 
+    @Test
+    void testQueryMultipleValues() {
+        UriQueryWriteable query = UriQueryWriteable.create();
+        query.fromQueryString("p1=v1&p1=v2");
+        assertThat(query.all("p1"), hasItems("v1", "v2"));
+    }
+
     @Test
     void testResolvePath() {
         ClientUri helper = ClientUri.create(URI.create("http://localhost:8080/loom"));