Allow null value for query parameter setter methods

Motivation: 041f3dc replace empty string for null during parsing. However the setter/accessor methods don't consistently allow null values. Modifications: - Allow null values from add & set queryParameter methods.
apple · Sep 19, 2024 · 1bb1ca3 · 1bb1ca3
1 parent 004983e
commit 1bb1ca3
Show file tree

Hide file tree

Showing 12 changed files with 89 additions and 53 deletions.
diff --git a/...icetalk-http-api/src/main/java/io/servicetalk/http/api/AbstractDelegatingHttpRequest.java b/...icetalk-http-api/src/main/java/io/servicetalk/http/api/AbstractDelegatingHttpRequest.java
@@ -63,7 +63,7 @@ public Set<String> queryParametersKeys() {
     }
 
     @Override
-    public boolean hasQueryParameter(final String key, final String value) {
+    public boolean hasQueryParameter(final String key, @Nullable final String value) {
         return original.hasQueryParameter(key, value);
     }
 
@@ -78,7 +78,7 @@ public boolean removeQueryParameters(final String key) {
     }
 
     @Override
-    public boolean removeQueryParameters(final String key, final String value) {
+    public boolean removeQueryParameters(final String key, @Nullable final String value) {
         return original.removeQueryParameters(key, value);
     }
 

diff --git a/servicetalk-http-api/src/main/java/io/servicetalk/http/api/BlockingStreamingHttpRequest.java b/servicetalk-http-api/src/main/java/io/servicetalk/http/api/BlockingStreamingHttpRequest.java
@@ -273,7 +273,7 @@ default <T> BlockingStreamingHttpRequest transform(TrailersTransformer<T, Buffer
     BlockingStreamingHttpRequest query(@Nullable String query);
 
     @Override
-    BlockingStreamingHttpRequest addQueryParameter(String key, String value);
+    BlockingStreamingHttpRequest addQueryParameter(String key, @Nullable String value);
 
     @Override
     BlockingStreamingHttpRequest addQueryParameters(String key, Iterable<String> values);
@@ -282,7 +282,7 @@ default <T> BlockingStreamingHttpRequest transform(TrailersTransformer<T, Buffer
     BlockingStreamingHttpRequest addQueryParameters(String key, String... values);
 
     @Override
-    BlockingStreamingHttpRequest setQueryParameter(String key, String value);
+    BlockingStreamingHttpRequest setQueryParameter(String key, @Nullable String value);
 
     @Override
     BlockingStreamingHttpRequest setQueryParameters(String key, Iterable<String> values);

diff --git a/...k-http-api/src/main/java/io/servicetalk/http/api/DefaultBlockingStreamingHttpRequest.java b/...k-http-api/src/main/java/io/servicetalk/http/api/DefaultBlockingStreamingHttpRequest.java
@@ -119,7 +119,7 @@ public BlockingStreamingHttpRequest query(@Nullable final String query) {
     }
 
     @Override
-    public BlockingStreamingHttpRequest addQueryParameter(String key, String value) {
+    public BlockingStreamingHttpRequest addQueryParameter(String key, @Nullable String value) {
         original.addQueryParameter(key, value);
         return this;
     }
@@ -137,7 +137,7 @@ public BlockingStreamingHttpRequest addQueryParameters(String key, String... val
     }
 
     @Override
-    public BlockingStreamingHttpRequest setQueryParameter(String key, String value) {
+    public BlockingStreamingHttpRequest setQueryParameter(String key, @Nullable String value) {
         original.setQueryParameter(key, value);
         return this;
     }

diff --git a/servicetalk-http-api/src/main/java/io/servicetalk/http/api/DefaultHttpRequest.java b/servicetalk-http-api/src/main/java/io/servicetalk/http/api/DefaultHttpRequest.java
@@ -110,7 +110,7 @@ public HttpRequest query(@Nullable final String query) {
     }
 
     @Override
-    public HttpRequest addQueryParameter(final String key, final String value) {
+    public HttpRequest addQueryParameter(final String key, @Nullable final String value) {
         original.addQueryParameter(key, value);
         return this;
     }
@@ -128,7 +128,7 @@ public HttpRequest addQueryParameters(final String key, final String... values)
     }
 
     @Override
-    public HttpRequest setQueryParameter(final String key, final String value) {
+    public HttpRequest setQueryParameter(final String key, @Nullable final String value) {
         original.setQueryParameter(key, value);
         return this;
     }

diff --git a/servicetalk-http-api/src/main/java/io/servicetalk/http/api/DefaultHttpRequestMetaData.java b/servicetalk-http-api/src/main/java/io/servicetalk/http/api/DefaultHttpRequestMetaData.java
@@ -312,7 +312,7 @@ public boolean hasQueryParameter(final String key) {
     }
 
     @Override
-    public boolean hasQueryParameter(final String key, final String value) {
+    public boolean hasQueryParameter(final String key, @Nullable final String value) {
         return lazyParseQueryString().contains(key, value);
     }
 
@@ -322,7 +322,7 @@ public int queryParametersSize() {
     }
 
     @Override
-    public HttpRequestMetaData addQueryParameter(final String key, final String value) {
+    public HttpRequestMetaData addQueryParameter(final String key, @Nullable final String value) {
         lazyParseQueryString().add(key, value);
         return this;
     }
@@ -340,7 +340,7 @@ public HttpRequestMetaData addQueryParameters(final String key, final String...
     }
 
     @Override
-    public HttpRequestMetaData setQueryParameter(final String key, final String value) {
+    public HttpRequestMetaData setQueryParameter(final String key, @Nullable final String value) {
         lazyParseQueryString().set(key, value);
         return this;
     }
@@ -363,7 +363,7 @@ public boolean removeQueryParameters(final String key) {
     }
 
     @Override
-    public boolean removeQueryParameters(final String key, final String value) {
+    public boolean removeQueryParameters(final String key, @Nullable final String value) {
         return lazyParseQueryString().remove(key, value);
     }
 
@@ -423,11 +423,16 @@ private void query(final Map<String, List<String>> params) {
                 Iterator<String> valuesItr = values.iterator();
                 if (valuesItr.hasNext()) {
                     String value = valuesItr.next();
-                    sb.append('=').append(encodeComponent(QUERY_VALUE, value, REQUEST_TARGET_CHARSET, true));
-                    while (valuesItr.hasNext()) {
-                        value = valuesItr.next();
-                        sb.append('&').append(encodedKey).append('=')
-                                .append(encodeComponent(QUERY_VALUE, value, REQUEST_TARGET_CHARSET, true));
+                    if (value != null) {
+                        sb.append('=').append(encodeComponent(QUERY_VALUE, value, REQUEST_TARGET_CHARSET, true));
+                        while (valuesItr.hasNext()) {
+                            value = valuesItr.next();
+                            sb.append('&').append(encodedKey);
+                            if (value != null) {
+                                sb.append('=')
+                                        .append(encodeComponent(QUERY_VALUE, value, REQUEST_TARGET_CHARSET, true));
+                            }
+                        }
                     }
                 }
             }

diff --git a/servicetalk-http-api/src/main/java/io/servicetalk/http/api/DefaultStreamingHttpRequest.java b/servicetalk-http-api/src/main/java/io/servicetalk/http/api/DefaultStreamingHttpRequest.java
@@ -121,7 +121,7 @@ public StreamingHttpRequest query(@Nullable final String query) {
     }
 
     @Override
-    public StreamingHttpRequest addQueryParameter(String key, String value) {
+    public StreamingHttpRequest addQueryParameter(String key, @Nullable String value) {
         super.addQueryParameter(key, value);
         return this;
     }
@@ -139,7 +139,7 @@ public StreamingHttpRequest addQueryParameters(String key, String... values) {
     }
 
     @Override
-    public StreamingHttpRequest setQueryParameter(String key, String value) {
+    public StreamingHttpRequest setQueryParameter(String key, @Nullable String value) {
         super.setQueryParameter(key, value);
         return this;
     }

diff --git a/servicetalk-http-api/src/main/java/io/servicetalk/http/api/HttpQuery.java b/servicetalk-http-api/src/main/java/io/servicetalk/http/api/HttpQuery.java
@@ -22,6 +22,7 @@
 import java.util.Map;
 import java.util.Map.Entry;
 import java.util.NoSuchElementException;
+import java.util.Objects;
 import java.util.Set;
 import java.util.Spliterator;
 import java.util.Spliterators;
@@ -91,8 +92,8 @@ public Set<String> keys() {
         return unmodifiableSet(params.keySet());
     }
 
-    public HttpQuery add(final String key, final String value) {
-        validateQueryParam(key, value);
+    public HttpQuery add(final String key, @Nullable final String value) {
+        validateQueryParam(key);
         getValues(key).add(value);
         markDirty();
         return this;
@@ -114,8 +115,8 @@ public HttpQuery add(final String key, final String... values) {
         return this;
     }
 
-    public HttpQuery set(final String key, final String value) {
-        validateQueryParam(key, value);
+    public HttpQuery set(final String key, @Nullable final String value) {
+        validateQueryParam(key);
         final ArrayList<String> list = new ArrayList<>(DEFAULT_LIST_SIZE);
         list.add(value);
         markDirty();
@@ -145,10 +146,10 @@ boolean contains(final String key) {
         return params.get(key) != null;
     }
 
-    public boolean contains(final String key, final String value) {
+    public boolean contains(final String key, @Nullable final String value) {
         final Iterator<String> values = valuesIterator(key);
         while (values.hasNext()) {
-            if (value.equals(values.next())) {
+            if (Objects.equals(value, values.next())) {
                 return true;
             }
         }
@@ -163,10 +164,10 @@ public boolean remove(final String key) {
         return false;
     }
 
-    public boolean remove(final String key, final String value) {
+    public boolean remove(final String key, @Nullable final String value) {
         final Iterator<String> values = valuesIterator(key);
         while (values.hasNext()) {
-            if (value.equals(values.next())) {
+            if (Objects.equals(value, values.next())) {
                 values.remove();
                 markDirty();
                 return true;
@@ -208,13 +209,10 @@ private List<String> getValues(final String key) {
         return params.computeIfAbsent(key, k -> new ArrayList<>(DEFAULT_LIST_SIZE));
     }
 
-    private void validateQueryParam(final String key, final String value) {
+    private static void validateQueryParam(@Nullable final String key) {
         if (key == null || key.isEmpty()) {
             throw new IllegalArgumentException("Null or empty query parameter names are not allowed.");
         }
-        if (value == null) {
-            throw new IllegalArgumentException("Null query parameter values are not allowed.");
-        }
     }
 
     private static final class ValuesIterator implements Iterator<String> {

diff --git a/servicetalk-http-api/src/main/java/io/servicetalk/http/api/HttpRequest.java b/servicetalk-http-api/src/main/java/io/servicetalk/http/api/HttpRequest.java
@@ -119,7 +119,7 @@ default <T> HttpRequest payloadBody(T pojo, HttpSerializer<T> serializer) {
     HttpRequest query(@Nullable String query);
 
     @Override
-    HttpRequest addQueryParameter(String key, String value);
+    HttpRequest addQueryParameter(String key, @Nullable String value);
 
     @Override
     HttpRequest addQueryParameters(String key, Iterable<String> values);
@@ -128,7 +128,7 @@ default <T> HttpRequest payloadBody(T pojo, HttpSerializer<T> serializer) {
     HttpRequest addQueryParameters(String key, String... values);
 
     @Override
-    HttpRequest setQueryParameter(String key, String value);
+    HttpRequest setQueryParameter(String key, @Nullable String value);
 
     @Override
     HttpRequest setQueryParameters(String key, Iterable<String> values);

diff --git a/servicetalk-http-api/src/main/java/io/servicetalk/http/api/HttpRequestMetaData.java b/servicetalk-http-api/src/main/java/io/servicetalk/http/api/HttpRequestMetaData.java
@@ -298,7 +298,7 @@ default boolean hasQueryParameter(final String key) {
      * @param value the query parameter value of the query parameter to find.
      * @return {@code true} if a {@code key}, {@code value} pair exists.
      */
-    boolean hasQueryParameter(String key, String value);
+    boolean hasQueryParameter(String key, @Nullable String value);
 
     /**
      * Returns the number of query parameters.
@@ -314,7 +314,7 @@ default boolean hasQueryParameter(final String key) {
      * @param value the query parameter value.
      * @return {@code this}.
      */
-    HttpRequestMetaData addQueryParameter(String key, String value);
+    HttpRequestMetaData addQueryParameter(String key, @Nullable String value);
 
     /**
      * Adds new query parameters with the specified {@code key} and {@code values}. This method is semantically
@@ -357,7 +357,7 @@ default boolean hasQueryParameter(final String key) {
      * @param value the query parameter value.
      * @return {@code this}.
      */
-    HttpRequestMetaData setQueryParameter(String key, String value);
+    HttpRequestMetaData setQueryParameter(String key, @Nullable String value);
 
     /**
      * Sets new query parameters with the specified {@code key} and {@code values}. This method is equivalent to:
@@ -406,7 +406,7 @@ default boolean hasQueryParameter(final String key) {
      * @param value the query parameter value.
      * @return {@code true} if at least one entry has been removed.
      */
-    boolean removeQueryParameters(String key, String value);
+    boolean removeQueryParameters(String key, @Nullable String value);
 
     /**
      * Returns the <a href="https://datatracker.ietf.org/doc/html/rfc3986#section-3.5">fragment</a> part of the request

diff --git a/servicetalk-http-api/src/main/java/io/servicetalk/http/api/StreamingHttpRequest.java b/servicetalk-http-api/src/main/java/io/servicetalk/http/api/StreamingHttpRequest.java
@@ -261,7 +261,7 @@ <T, S> StreamingHttpRequest transform(TrailersTransformer<T, S> trailersTransfor
     StreamingHttpRequest query(@Nullable String query);
 
     @Override
-    StreamingHttpRequest addQueryParameter(String key, String value);
+    StreamingHttpRequest addQueryParameter(String key, @Nullable String value);
 
     @Override
     StreamingHttpRequest addQueryParameters(String key, Iterable<String> values);
@@ -270,7 +270,7 @@ <T, S> StreamingHttpRequest transform(TrailersTransformer<T, S> trailersTransfor
     StreamingHttpRequest addQueryParameters(String key, String... values);
 
     @Override
-    StreamingHttpRequest setQueryParameter(String key, String value);
+    StreamingHttpRequest setQueryParameter(String key, @Nullable String value);
 
     @Override
     StreamingHttpRequest setQueryParameters(String key, Iterable<String> values);

diff --git a/...etalk-http-api/src/test/java/io/servicetalk/http/api/AbstractHttpRequestMetaDataTest.java b/...etalk-http-api/src/test/java/io/servicetalk/http/api/AbstractHttpRequestMetaDataTest.java
@@ -19,6 +19,7 @@
 
 import org.junit.jupiter.api.Test;
 
+import java.util.AbstractMap.SimpleEntry;
 import java.util.ArrayList;
 import java.util.Iterator;
 import java.util.List;
@@ -41,6 +42,8 @@
 import static java.util.Spliterators.spliteratorUnknownSize;
 import static java.util.stream.Collectors.toList;
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.contains;
+import static org.hamcrest.Matchers.emptyIterable;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.hasSize;
 import static org.hamcrest.Matchers.lessThan;
@@ -650,6 +653,50 @@ void testSetQueryAndReparse() {
         assertEquals(asList("new ", "new2"), iteratorAsList(fixture.queryParametersIterator("abc")));
     }
 
+    @Test
+    void testNullQueryParamValue() {
+        createFixture("/foo?bar");
+        assertEquals("/foo?bar", fixture.requestTarget());
+        assertEquals("bar", fixture.rawQuery());
+
+        // Test single add & remove.
+        fixture.addQueryParameters("baz", (String) null);
+        assertTrue(fixture.hasQueryParameter("baz"));
+        assertTrue(fixture.hasQueryParameter("baz", null));
+        assertThat(fixture.queryParameters("baz"), contains((String) null));
+
+        assertEquals("bar&baz", fixture.rawQuery());
+        assertEquals("bar&baz", fixture.query());
+        assertThat(fixture.queryParameters(), contains(new SimpleEntry<>("bar", null), new SimpleEntry<>("baz", null)));
+
+        assertTrue(fixture.removeQueryParameters("baz"));
+        assertFalse(fixture.hasQueryParameter("baz"));
+        assertThat(fixture.queryParameters("baz"), emptyIterable());
+
+        // Test that set will clear out existing values and replace with null
+        fixture.addQueryParameters("zzz", "one", null, "three");
+        assertThat(fixture.queryParameters("zzz"), contains("one", null, "three"));
+
+        assertEquals("bar&zzz=one&zzz&zzz=three", fixture.rawQuery());
+        assertEquals("bar&zzz=one&zzz&zzz=three", fixture.query());
+        assertThat(fixture.queryParameters(), contains(new SimpleEntry<>("bar", null),
+                new SimpleEntry<>("zzz", "one"), new SimpleEntry<>("zzz", null), new SimpleEntry<>("zzz", "three")));
+
+        fixture.setQueryParameters("zzz", (String) null);
+        assertNull(fixture.queryParameter("zzz"));
+        assertThat(fixture.queryParameters("zzz"), contains((String) null));
+        assertTrue(fixture.hasQueryParameter("zzz"));
+        assertEquals("bar&zzz", fixture.rawQuery());
+        assertEquals("bar&zzz", fixture.query());
+        assertThat(fixture.queryParameters(), contains(new SimpleEntry<>("bar", null), new SimpleEntry<>("zzz", null)));
+
+        assertTrue(fixture.removeQueryParameters("zzz", null));
+
+        // Reset the query parameter to test null vs empty string.
+        fixture.rawQuery("foo&foo=&foo=value");
+        assertThat(fixture.queryParameters("foo"), contains(null, "", "value"));
+    }
+
     @Test
     void testOneEmptyQueryParam() {
         createFixture("/foo?bar");

diff --git a/servicetalk-http-api/src/test/java/io/servicetalk/http/api/InvalidMetadataValuesTest.java b/servicetalk-http-api/src/test/java/io/servicetalk/http/api/InvalidMetadataValuesTest.java
@@ -107,13 +107,6 @@ void emptyQPNameToAdd(final HttpMetaData metaData, @SuppressWarnings("unused") S
         assertThrows(IllegalArgumentException.class, () -> requestMeta.addQueryParameter("", "foo"));
     }
 
-    @ParameterizedTest(name = "{displayName} [{index}]: source = {1}")
-    @MethodSource("data")
-    void nullQPValueToAdd(final HttpMetaData metaData, @SuppressWarnings("unused") String testName) {
-        HttpRequestMetaData requestMeta = assumeRequestMeta(metaData);
-        assertThrows(IllegalArgumentException.class, () -> requestMeta.addQueryParameter("foo", null));
-    }
-
     @ParameterizedTest(name = "{displayName} [{index}]: source = {1}")
     @MethodSource("data")
     void nullQPNameToSet(final HttpMetaData metaData, @SuppressWarnings("unused") String testName) {
@@ -128,13 +121,6 @@ void emptyQPNameToSet(final HttpMetaData metaData, @SuppressWarnings("unused") S
         assertThrows(IllegalArgumentException.class, () -> requestMeta.setQueryParameter("", "foo"));
     }
 
-    @ParameterizedTest(name = "{displayName} [{index}]: source = {1}")
-    @MethodSource("data")
-    void nullQPValueToSet(final HttpMetaData metaData, @SuppressWarnings("unused") String testName) {
-        HttpRequestMetaData requestMeta = assumeRequestMeta(metaData);
-        assertThrows(IllegalArgumentException.class, () -> requestMeta.setQueryParameter("foo", null));
-    }
-
     @ParameterizedTest(name = "{displayName} [{index}]: source = {1}")
     @MethodSource("data")
     void nullCookieName(final HttpMetaData metaData, @SuppressWarnings("unused") String testName) {