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));