From 041f3dc9ce3fd0e423263a88e0df8cab7fda14ec Mon Sep 17 00:00:00 2001 From: Michael Nitschinger Date: Wed, 19 Apr 2023 21:42:57 +0200 Subject: [PATCH] Allow null values with FormUrlEncodedHttp[De]Serializer (#2554) Motivation: At the moment the behavior around null (and to an extent empty) values is a little bit unflexible. ServiceTalk makes the decision to ignore null values on encoding and converting them to the same value as an empty string on decoding. Since there is no spec which says that null values are disallowed, it is better if ServiceTalk is flexible and lets the user decide if they want to ignore values (which they can do by checking and then just not encoding them in the first place). Modifications: This changeset modifies behavior as follows. On the encoding side: - null values are now allowed and encoded as "just" the key, instead of not being encoded at all. (Distinguished without an "=" compared to an empty string) On the decoding side: - Null values are now not turned into an empty string, but if just the key is present turned into "null" - and if the "=" is present, an empty string is decoded. Note: this also implicitly changes the resulting HttpQuery behavior when calling lazyParseQueryString on HttpRequestMetaData, since it uses the same functionality underneath. Since the "get" method is marked as `Nullable` already, there is no illegal behavior observed. --- .../api/FormUrlEncodedHttpSerializer.java | 44 ++++++++---- .../io/servicetalk/http/api/UriUtils.java | 4 +- .../api/AbstractHttpRequestMetaDataTest.java | 15 ++-- .../FormUrlEncodedHttpDeserializerTest.java | 67 +++++++++-------- .../api/FormUrlEncodedHttpSerializerTest.java | 71 +++++++++++++++++-- 5 files changed, 144 insertions(+), 57 deletions(-) diff --git a/servicetalk-http-api/src/main/java/io/servicetalk/http/api/FormUrlEncodedHttpSerializer.java b/servicetalk-http-api/src/main/java/io/servicetalk/http/api/FormUrlEncodedHttpSerializer.java index 3e58cae5b9..50623283f9 100644 --- a/servicetalk-http-api/src/main/java/io/servicetalk/http/api/FormUrlEncodedHttpSerializer.java +++ b/servicetalk-http-api/src/main/java/io/servicetalk/http/api/FormUrlEncodedHttpSerializer.java @@ -27,6 +27,7 @@ import java.io.UnsupportedEncodingException; import java.net.URLEncoder; import java.nio.charset.Charset; +import java.nio.charset.StandardCharsets; import java.util.List; import java.util.Map; import java.util.concurrent.TimeUnit; @@ -46,12 +47,17 @@ final class FormUrlEncodedHttpSerializer implements HttpSerializer headers.set(CONTENT_TYPE, APPLICATION_X_WWW_FORM_URLENCODED_UTF_8)); + private static final byte EQUALS_BYTE = '='; + private static final byte AMPERSAND_BYTE = '&'; + private final Charset charset; + private final boolean isOptimizedCharset; private final Consumer addContentType; FormUrlEncodedHttpSerializer(final Charset charset, final Consumer addContentType) { this.charset = charset; this.addContentType = addContentType; + this.isOptimizedCharset = UTF_8.equals(charset) || StandardCharsets.US_ASCII.equals(charset); } @SuppressWarnings("ConstantConditions") @@ -168,34 +174,48 @@ private Buffer serialize(@Nullable final Map> parameters, f } final Buffer buffer = allocator.newBuffer(); - // Null values may be omitted - // https://tools.ietf.org/html/rfc1866#section-8.2 + // Null values may be omitted, but for more flexibility we choose to encode them if they are provided by the + // caller. See https://tools.ietf.org/html/rfc1866#section-8.2 parameters.forEach((key, values) -> { if (key == null || key.isEmpty()) { throw new SerializationException("Null or empty keys are not supported " + "for x-www-form-urlencoded params"); } - if (values == null) { + if (values == null || values.isEmpty()) { + // Received a key with no values, so just encode the key and return. + writeKey(buffer, isContinuation, key); return; } values.forEach(value -> { - if (value == null) { - return; - } - - if (buffer.writerIndex() != 0 || isContinuation) { - buffer.writeBytes("&".getBytes(charset)); + writeKey(buffer, isContinuation, key); + if (value != null) { + if (isOptimizedCharset) { + buffer.writeByte(EQUALS_BYTE); + } else { + buffer.writeBytes("=".getBytes(charset)); + } + if (!value.isEmpty()) { + buffer.writeBytes(urlEncode(value).getBytes(charset)); + } } - buffer.writeBytes(urlEncode(key).getBytes(charset)); - buffer.writeBytes("=".getBytes(charset)); - buffer.writeBytes(urlEncode(value).getBytes(charset)); }); }); return buffer; } + private void writeKey(final Buffer buffer, boolean isContinuation, String key) { + if (buffer.writerIndex() != 0 || isContinuation) { + if (isOptimizedCharset) { + buffer.writeByte(AMPERSAND_BYTE); + } else { + buffer.writeBytes("&".getBytes(charset)); + } + } + buffer.writeBytes(urlEncode(key).getBytes(charset)); + } + private String urlEncode(final String value) { try { return URLEncoder.encode(value, charset.name()); diff --git a/servicetalk-http-api/src/main/java/io/servicetalk/http/api/UriUtils.java b/servicetalk-http-api/src/main/java/io/servicetalk/http/api/UriUtils.java index 7d49d0dfc9..8a2d0b3744 100644 --- a/servicetalk-http-api/src/main/java/io/servicetalk/http/api/UriUtils.java +++ b/servicetalk-http-api/src/main/java/io/servicetalk/http/api/UriUtils.java @@ -252,7 +252,9 @@ private static void addQueryParam(final String s, final int nameStart, int value final String name; if (valueStart <= nameStart) { name = decoder.apply(s.substring(nameStart, valueEnd), charset); - value = ""; + // If no value is present, it should be represented as null (and not an empty string) so we can distinguish + // the cases of "key1&..." and "key1=&..." (note the presence of the equals sign) + value = null; } else { name = decoder.apply(s.substring(nameStart, valueStart - 1), charset); value = decoder.apply(s.substring(valueStart, valueEnd), charset); 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 9c7b6854f1..95fc3e316f 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 @@ -25,6 +25,7 @@ import java.util.Map.Entry; import java.util.Spliterator; import java.util.stream.StreamSupport; +import javax.annotation.Nullable; import static io.servicetalk.http.api.HttpHeaderNames.AUTHORIZATION; import static io.servicetalk.http.api.HttpHeaderNames.HOST; @@ -654,17 +655,17 @@ void testOneEmptyQueryParam() { createFixture("/foo?bar"); assertEquals("/foo?bar", fixture.requestTarget()); assertEquals("bar", fixture.rawQuery()); - assertEquals("", fixture.queryParameter("bar")); + assertNull(fixture.queryParameter("bar")); assertNull(fixture.queryParameter("nothing")); assertEquals(singletonList("bar"), iteratorAsList(fixture.queryParametersKeys().iterator())); Iterator> itr = fixture.queryParameters().iterator(); - assertNext(itr, "bar", ""); + assertNext(itr, "bar", null); assertFalse(itr.hasNext()); List> entries = iteratorAsList(fixture.queryParameters().iterator()); assertThat(entries, hasSize(1)); - assertEntry(entries.get(0), "bar", ""); + assertEntry(entries.get(0), "bar", null); } @Test @@ -861,17 +862,17 @@ private static List iteratorAsList(final Iterator iterator) { .collect(toList()); } - private static void assertNext(Iterator> itr, String key, String value) { + private static void assertNext(Iterator> itr, String key, @Nullable String value) { assertTrue(itr.hasNext()); assertEntry(itr.next(), key, value); } - private static void assertEntry(Entry next, String key, String value) { + private static void assertEntry(Entry next, String key, @Nullable String value) { assertEquals(key, next.getKey()); assertEquals(value, next.getValue()); } - private static String queryValue(String v) { - return v.isEmpty() ? "" : "=" + v; + private static String queryValue(@Nullable String v) { + return v == null ? "" : "=" + v; } } diff --git a/servicetalk-http-api/src/test/java/io/servicetalk/http/api/FormUrlEncodedHttpDeserializerTest.java b/servicetalk-http-api/src/test/java/io/servicetalk/http/api/FormUrlEncodedHttpDeserializerTest.java index 4e5abb616b..e8612c5fd6 100644 --- a/servicetalk-http-api/src/test/java/io/servicetalk/http/api/FormUrlEncodedHttpDeserializerTest.java +++ b/servicetalk-http-api/src/test/java/io/servicetalk/http/api/FormUrlEncodedHttpDeserializerTest.java @@ -21,6 +21,8 @@ import io.servicetalk.serialization.api.SerializationException; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import java.util.List; import java.util.Map; @@ -33,22 +35,26 @@ import static java.util.Collections.singletonList; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.emptyString; +import static org.hamcrest.Matchers.hasSize; +import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.not; +import static org.hamcrest.Matchers.nullValue; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; class FormUrlEncodedHttpDeserializerTest { + private static final FormUrlEncodedHttpDeserializer deserializer = FormUrlEncodedHttpDeserializer.UTF8; + + private static final HttpHeaders FE_HEADERS = DefaultHttpHeadersFactory.INSTANCE.newHeaders() + .set(CONTENT_TYPE, "application/x-www-form-urlencoded; charset=UTF-8"); + @Test void formParametersAreDeserialized() { - final FormUrlEncodedHttpDeserializer deserializer = FormUrlEncodedHttpDeserializer.UTF8; - - final HttpHeaders headers = DefaultHttpHeadersFactory.INSTANCE.newHeaders(); - headers.set(CONTENT_TYPE, "application/x-www-form-urlencoded; charset=UTF-8"); final String formParameters = "escape%26this%3D=and%26this%25¶m2=bar+¶m2=foo%20&emptyParam="; - - final Map> deserialized = deserializer.deserialize(headers, toBuffer(formParameters)); + final Map> deserialized = deserializer.deserialize(FE_HEADERS, toBuffer(formParameters)); assertEquals(singletonList("and&this%"), deserialized.get("escape&this="), "Unexpected parameter value."); @@ -63,23 +69,15 @@ void formParametersAreDeserialized() { @Test void deserializeEmptyBuffer() { - final FormUrlEncodedHttpDeserializer deserializer = FormUrlEncodedHttpDeserializer.UTF8; - - final HttpHeaders headers = DefaultHttpHeadersFactory.INSTANCE.newHeaders(); - headers.set(CONTENT_TYPE, "application/x-www-form-urlencoded; charset=UTF-8"); - - final Map> deserialized = deserializer.deserialize(headers, EMPTY_BUFFER); - + final Map> deserialized = deserializer.deserialize(FE_HEADERS, EMPTY_BUFFER); assertEquals(0, deserialized.size(), "Unexpected parameter count"); } @Test void invalidContentTypeThrows() { - final FormUrlEncodedHttpDeserializer deserializer = FormUrlEncodedHttpDeserializer.UTF8; - - final HttpHeaders headers = DefaultHttpHeadersFactory.INSTANCE.newHeaders(); final String invalidContentType = "invalid/content/type"; - headers.set(CONTENT_TYPE, invalidContentType); + final HttpHeaders headers = DefaultHttpHeadersFactory.INSTANCE.newHeaders() + .set(CONTENT_TYPE, invalidContentType); SerializationException e = assertThrows(SerializationException.class, () -> deserializer.deserialize(headers, EMPTY_BUFFER)); @@ -88,13 +86,11 @@ void invalidContentTypeThrows() { @Test void invalidContentTypeThrowsAndMasksAdditionalHeadersValues() { - final FormUrlEncodedHttpDeserializer deserializer = FormUrlEncodedHttpDeserializer.UTF8; - - final HttpHeaders headers = DefaultHttpHeadersFactory.INSTANCE.newHeaders(); final String invalidContentType = "invalid/content/type"; final String someHost = "some/host"; - headers.set(CONTENT_TYPE, invalidContentType); - headers.set(HttpHeaderNames.HOST, someHost); + final HttpHeaders headers = DefaultHttpHeadersFactory.INSTANCE.newHeaders() + .set(CONTENT_TYPE, invalidContentType) + .set(HttpHeaderNames.HOST, someHost); SerializationException e = assertThrows(SerializationException.class, () -> deserializer.deserialize(headers, EMPTY_BUFFER)); @@ -105,8 +101,6 @@ void invalidContentTypeThrowsAndMasksAdditionalHeadersValues() { @Test void missingContentTypeThrows() { - final FormUrlEncodedHttpDeserializer deserializer = FormUrlEncodedHttpDeserializer.UTF8; - final HttpHeaders headers = DefaultHttpHeadersFactory.INSTANCE.newHeaders(); assertThrows(SerializationException.class, () -> deserializer.deserialize(headers, EMPTY_BUFFER)); @@ -114,10 +108,6 @@ void missingContentTypeThrows() { @Test void iterableCloseIsPropagated() throws Exception { - final FormUrlEncodedHttpDeserializer deserializer = FormUrlEncodedHttpDeserializer.UTF8; - final HttpHeaders headers = DefaultHttpHeadersFactory.INSTANCE.newHeaders(); - headers.set(CONTENT_TYPE, "application/x-www-form-urlencoded; charset=UTF-8"); - final AtomicBoolean isClosed = new AtomicBoolean(false); final BlockingIterable formParametersIterable = () -> new BlockingIterator() { @@ -148,13 +138,30 @@ public boolean hasNext() { }; final BlockingIterable>> deserialized = deserializer - .deserialize(headers, formParametersIterable); + .deserialize(FE_HEADERS, formParametersIterable); deserialized.iterator().close(); assertTrue(isClosed.get(), "BlockingIterable was not closed."); } - private Buffer toBuffer(final String value) { + @ParameterizedTest(name = "{displayName} [{index}] paramIsNull={0}") + @ValueSource(booleans = { true, false }) + void deserializesNullOrEmptyValues(boolean paramIsNull) { + final String separator = paramIsNull ? "" : "="; + final String formParameters = String.format("key1%s&key2%s&key2%s", separator, separator, separator); + + final Map> deserialized = deserializer.deserialize(FE_HEADERS, toBuffer(formParameters)); + assertThat(deserialized.size(), is(2)); + + assertThat(deserialized.get("key1"), hasSize(1)); + assertThat(deserialized.get("key2"), hasSize(2)); + + assertThat(deserialized.get("key1").get(0), paramIsNull ? nullValue() : emptyString()); + assertThat(deserialized.get("key2").get(0), paramIsNull ? nullValue() : emptyString()); + assertThat(deserialized.get("key2").get(1), paramIsNull ? nullValue() : emptyString()); + } + + private static Buffer toBuffer(final String value) { return DEFAULT_ALLOCATOR.fromUtf8(value); } } diff --git a/servicetalk-http-api/src/test/java/io/servicetalk/http/api/FormUrlEncodedHttpSerializerTest.java b/servicetalk-http-api/src/test/java/io/servicetalk/http/api/FormUrlEncodedHttpSerializerTest.java index bbe9b01b9a..ffd06ac022 100644 --- a/servicetalk-http-api/src/test/java/io/servicetalk/http/api/FormUrlEncodedHttpSerializerTest.java +++ b/servicetalk-http-api/src/test/java/io/servicetalk/http/api/FormUrlEncodedHttpSerializerTest.java @@ -25,6 +25,9 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import java.nio.charset.StandardCharsets; +import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -36,7 +39,6 @@ import static io.servicetalk.http.api.HttpHeaderNames.CONTENT_TYPE; import static java.nio.charset.StandardCharsets.UTF_8; import static java.util.Arrays.asList; -import static java.util.Collections.EMPTY_MAP; import static java.util.Collections.emptyList; import static java.util.Collections.emptyMap; import static java.util.Collections.singletonList; @@ -62,6 +64,7 @@ class FormUrlEncodedHttpSerializerTest { MAP_B.put("key4", singletonList("val4")); } + private static final Map> EMPTY_MAP = Collections.emptyMap(); private static final Map> MAP_NULL_KEY = new HashMap<>(); static { MAP_NULL_KEY.put(null, singletonList("val1")); @@ -109,7 +112,8 @@ void serializeStreamingMultipleParts() throws Exception { Publisher.from(MAP_A, MAP_B), DEFAULT_ALLOCATOR); String queryStr = queryStringFromPublisher(serialized); - assertEquals("key1=val1&key2=val2&key3=val3&key4=val4", queryStr, "Unexpected serialized content."); + assertEquals("key1=val1&key2=val2&key5&key6&key3=val3&key4=val4&key7", + queryStr, "Unexpected serialized content."); } @Test @@ -141,7 +145,8 @@ void serializeStreamingMultiplePartsWithMixOfEmptyAndNotEmptyMaps() throws Excep String queryStr = queryStringFromPublisher(SERIALIZER.serialize(headers, Publisher.from(MAP_A, EMPTY_MAP, EMPTY_MAP, MAP_B), DEFAULT_ALLOCATOR)); - assertEquals("key1=val1&key2=val2&key3=val3&key4=val4", queryStr, "Unexpected serialized content."); + assertEquals("key1=val1&key2=val2&key5&key6&key3=val3&key4=val4&key7", + queryStr, "Unexpected serialized content."); } @Test @@ -150,10 +155,12 @@ void serializeStreamingMultiplePartsWithMixOfEmptyAndNotEmptyMapsAndResubscribe( Publisher.from(MAP_A, EMPTY_MAP, EMPTY_MAP, MAP_B), DEFAULT_ALLOCATOR); String queryStr = queryStringFromPublisher(pub); - assertEquals("key1=val1&key2=val2&key3=val3&key4=val4", queryStr, "Unexpected serialized content."); + assertEquals("key1=val1&key2=val2&key5&key6&key3=val3&key4=val4&key7", + queryStr, "Unexpected serialized content."); queryStr = queryStringFromPublisher(pub); - assertEquals("key1=val1&key2=val2&key3=val3&key4=val4", queryStr, "Unexpected serialized content."); + assertEquals("key1=val1&key2=val2&key5&key6&key3=val3&key4=val4&key7", + queryStr, "Unexpected serialized content."); } @Test @@ -161,7 +168,8 @@ void serializeBlockingItMultipleParts() { String queryStr = queryStringFromBlockingIterable(SERIALIZER.serialize(headers, BlockingIterables.from(asList(MAP_A, MAP_B)), DEFAULT_ALLOCATOR)); - assertEquals("key1=val1&key2=val2&key3=val3&key4=val4", queryStr, "Unexpected serialized content."); + assertEquals("key1=val1&key2=val2&key5&key6&key3=val3&key4=val4&key7", + queryStr, "Unexpected serialized content."); } @Test @@ -193,7 +201,8 @@ void serializeBlockingItMultiplePartsWithMixOfEmptyAndNotEmptyMaps() { String queryStr = queryStringFromBlockingIterable(SERIALIZER.serialize(headers, BlockingIterables.from(asList(MAP_A, EMPTY_MAP, EMPTY_MAP, MAP_B)), DEFAULT_ALLOCATOR)); - assertEquals("key1=val1&key2=val2&key3=val3&key4=val4", queryStr, "Unexpected serialized content."); + assertEquals("key1=val1&key2=val2&key5&key6&key3=val3&key4=val4&key7", + queryStr, "Unexpected serialized content."); } @Test @@ -237,6 +246,54 @@ public boolean hasNext() { "application/x-www-form-urlencoded; charset=UTF-8"), "Unexpected content type."); } + @Test + void serializesKeysWithoutValue() { + final Map> formParameters = new HashMap<>(); + formParameters.put("key1", null); + formParameters.put("key2", Collections.singletonList(null)); + formParameters.put("key3", Arrays.asList(null, null)); + + final Buffer serialized = SERIALIZER.serialize(headers, formParameters, DEFAULT_ALLOCATOR); + assertEquals("key1&key2&key3&key3", serialized.toString(UTF_8), "Unexpected serialized content."); + } + + @Test + void serializesKeysWithEmptyValue() { + final Map> formParameters = new HashMap<>(); + formParameters.put("key1", Collections.singletonList("")); + formParameters.put("key2", Arrays.asList("", "")); + final Buffer serialized = SERIALIZER.serialize(headers, formParameters, DEFAULT_ALLOCATOR); + + assertEquals("key1=&key2=&key2=", serialized.toString(UTF_8), "Unexpected serialized content."); + } + + @Test + void serializesMixOfNullEmptyAndActualValue() { + final Map> formParameters = new HashMap<>(); + formParameters.put("key1", null); + formParameters.put("key2", Collections.singletonList("")); + formParameters.put("key3", Collections.singletonList(null)); + formParameters.put("key4", Arrays.asList("a")); + formParameters.put("key5", Arrays.asList(null, null)); + formParameters.put("key6", Arrays.asList("", "")); + final Buffer serialized = SERIALIZER.serialize(headers, formParameters, DEFAULT_ALLOCATOR); + + assertEquals("key1&key2=&key5&key5&key6=&key6=&key3&key4=a", serialized.toString(UTF_8), + "Unexpected serialized content."); + } + + @Test + void serializesWithNonOptimizedCharset() { + final Map> formParameters = new HashMap<>(); + formParameters.put("key1", Collections.singletonList("a")); + formParameters.put("key2", Collections.singletonList("b")); + + final Buffer serialized = new FormUrlEncodedHttpSerializer(StandardCharsets.ISO_8859_1, headers -> { }) + .serialize(headers, formParameters, DEFAULT_ALLOCATOR); + assertEquals("key1=a&key2=b", serialized.toString(StandardCharsets.ISO_8859_1), + "Unexpected serialized content."); + } + private String queryStringFromPublisher(Publisher serialized) throws Exception { return serialized.collect(StringBuilder::new, (builder, buffer) -> { builder.append(buffer.toString(UTF_8));