From 1bb1ca3717765cf2c8dcfb7bfbb087e73149e922 Mon Sep 17 00:00:00 2001 From: Scott Mitchell Date: Thu, 19 Sep 2024 11:17:03 -0400 Subject: [PATCH] Allow null value for query parameter setter methods Motivation: 041f3dc9ce3fd0e423263a88e0df8cab7fda14ec 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. --- .../api/AbstractDelegatingHttpRequest.java | 4 +- .../api/BlockingStreamingHttpRequest.java | 4 +- .../DefaultBlockingStreamingHttpRequest.java | 4 +- .../http/api/DefaultHttpRequest.java | 4 +- .../http/api/DefaultHttpRequestMetaData.java | 23 +++++---- .../http/api/DefaultStreamingHttpRequest.java | 4 +- .../io/servicetalk/http/api/HttpQuery.java | 22 ++++----- .../io/servicetalk/http/api/HttpRequest.java | 4 +- .../http/api/HttpRequestMetaData.java | 8 ++-- .../http/api/StreamingHttpRequest.java | 4 +- .../api/AbstractHttpRequestMetaDataTest.java | 47 +++++++++++++++++++ .../http/api/InvalidMetadataValuesTest.java | 14 ------ 12 files changed, 89 insertions(+), 53 deletions(-) diff --git a/servicetalk-http-api/src/main/java/io/servicetalk/http/api/AbstractDelegatingHttpRequest.java b/servicetalk-http-api/src/main/java/io/servicetalk/http/api/AbstractDelegatingHttpRequest.java index 86fc5c7022..ac6b81552b 100644 --- a/servicetalk-http-api/src/main/java/io/servicetalk/http/api/AbstractDelegatingHttpRequest.java +++ b/servicetalk-http-api/src/main/java/io/servicetalk/http/api/AbstractDelegatingHttpRequest.java @@ -63,7 +63,7 @@ public Set 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 index 4434000f93..720b1fc40e 100644 --- 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 BlockingStreamingHttpRequest transform(TrailersTransformer values); @@ -282,7 +282,7 @@ default BlockingStreamingHttpRequest transform(TrailersTransformer values); diff --git a/servicetalk-http-api/src/main/java/io/servicetalk/http/api/DefaultBlockingStreamingHttpRequest.java b/servicetalk-http-api/src/main/java/io/servicetalk/http/api/DefaultBlockingStreamingHttpRequest.java index c71293a4e2..ab6f337ca7 100644 --- a/servicetalk-http-api/src/main/java/io/servicetalk/http/api/DefaultBlockingStreamingHttpRequest.java +++ b/servicetalk-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 index 8e8384244c..03d290d583 100644 --- 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 index 9bfb9f2204..76318da46d 100644 --- 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> params) { Iterator 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 index eb51865b4c..a5cde96ee2 100644 --- 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 index 3b0cc046a5..7086cf9cb8 100644 --- 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 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 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 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 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 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 { 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 index bc96c9eea3..99f952b0b2 100644 --- 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 HttpRequest payloadBody(T pojo, HttpSerializer 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 values); @@ -128,7 +128,7 @@ default HttpRequest payloadBody(T pojo, HttpSerializer 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 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 index cb2d574d03..73cff576c0 100644 --- 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 fragment 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 index 23ee24c9a2..9c668630f9 100644 --- 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 @@ StreamingHttpRequest transform(TrailersTransformer 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 values); @@ -270,7 +270,7 @@ StreamingHttpRequest transform(TrailersTransformer 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 values); diff --git a/servicetalk-http-api/src/test/java/io/servicetalk/http/api/AbstractHttpRequestMetaDataTest.java b/servicetalk-http-api/src/test/java/io/servicetalk/http/api/AbstractHttpRequestMetaDataTest.java index 9f44c4f999..1fcb47ec1e 100644 --- a/servicetalk-http-api/src/test/java/io/servicetalk/http/api/AbstractHttpRequestMetaDataTest.java +++ b/servicetalk-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 index 2e2856e9ef..ba1b4c6461 100644 --- 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) {