Skip to content

Commit

Permalink
Allow null values with FormUrlEncodedHttp[De]Serializer (apple#2554)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@daschl
daschl authored Apr 19, 2023
1 parent 1ea310a commit 041f3dc
Show file tree
Hide file tree
Showing 5 changed files with 144 additions and 57 deletions.
44 changes: 32 additions & 12 deletions servicetalk-http-api/src/main/java/io/servicetalk/http/api/FormUrlEncodedHttpSerializer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -46,12 +47,17 @@ final class FormUrlEncodedHttpSerializer implements HttpSerializer<Map<String, L
static final FormUrlEncodedHttpSerializer UTF8 = new FormUrlEncodedHttpSerializer(UTF_8,
headers -> 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<HttpHeaders> addContentType;

FormUrlEncodedHttpSerializer(final Charset charset, final Consumer<HttpHeaders> addContentType) {
this.charset = charset;
this.addContentType = addContentType;
this.isOptimizedCharset = UTF_8.equals(charset) || StandardCharsets.US_ASCII.equals(charset);
}

@SuppressWarnings("ConstantConditions")
Expand Down Expand Up @@ -168,34 +174,48 @@ private Buffer serialize(@Nullable final Map<String, List<String>> 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());
Expand Down
4 changes: 3 additions & 1 deletion servicetalk-http-api/src/main/java/io/servicetalk/http/api/UriUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
15 changes: 8 additions & 7 deletions ...etalk-http-api/src/test/java/io/servicetalk/http/api/AbstractHttpRequestMetaDataTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<Entry<String, String>> itr = fixture.queryParameters().iterator();
assertNext(itr, "bar", "");
assertNext(itr, "bar", null);
assertFalse(itr.hasNext());

List<Entry<String, String>> entries = iteratorAsList(fixture.queryParameters().iterator());
assertThat(entries, hasSize(1));
assertEntry(entries.get(0), "bar", "");
assertEntry(entries.get(0), "bar", null);
}

@Test
Expand Down Expand Up @@ -861,17 +862,17 @@ private static <T> List<T> iteratorAsList(final Iterator<T> iterator) {
.collect(toList());
}

private static void assertNext(Iterator<Entry<String, String>> itr, String key, String value) {
private static void assertNext(Iterator<Entry<String, String>> itr, String key, @Nullable String value) {
assertTrue(itr.hasNext());
assertEntry(itr.next(), key, value);
}

private static void assertEntry(Entry<String, String> next, String key, String value) {
private static void assertEntry(Entry<String, String> 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;
}
}
67 changes: 37 additions & 30 deletions ...lk-http-api/src/test/java/io/servicetalk/http/api/FormUrlEncodedHttpDeserializerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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&param2=bar+&param2=foo%20&emptyParam=";

final Map<String, List<String>> deserialized = deserializer.deserialize(headers, toBuffer(formParameters));
final Map<String, List<String>> deserialized = deserializer.deserialize(FE_HEADERS, toBuffer(formParameters));

assertEquals(singletonList("and&this%"),
deserialized.get("escape&this="), "Unexpected parameter value.");
Expand All @@ -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<String, List<String>> deserialized = deserializer.deserialize(headers, EMPTY_BUFFER);

final Map<String, List<String>> 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));
Expand All @@ -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));
Expand All @@ -105,19 +101,13 @@ void invalidContentTypeThrowsAndMasksAdditionalHeadersValues() {

@Test
void missingContentTypeThrows() {
final FormUrlEncodedHttpDeserializer deserializer = FormUrlEncodedHttpDeserializer.UTF8;

final HttpHeaders headers = DefaultHttpHeadersFactory.INSTANCE.newHeaders();
assertThrows(SerializationException.class,
() -> deserializer.deserialize(headers, EMPTY_BUFFER));
}

@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<Buffer> formParametersIterable = () -> new BlockingIterator<Buffer>() {
Expand Down Expand Up @@ -148,13 +138,30 @@ public boolean hasNext() {
};

final BlockingIterable<Map<String, List<String>>> 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<String, List<String>> 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);
}
}
Loading

0 comments on commit 041f3dc

Please sign in to comment.