Skip to content
Original file line number Diff line number Diff line change
Expand Up @@ -483,10 +483,10 @@ private MultiValuedTreeMap<String, String> parseRawQueryString(String qs) {

String[] kv = value.split(QUERY_STRING_KEY_VALUE_SEPARATOR);
String key = URLDecoder.decode(kv[0], LambdaContainerHandler.getContainerConfig().getUriEncoding());
String val = kv.length == 2 ? kv[1] : "";
String val = kv.length == 2 ? AwsHttpServletRequest.decodeValueIfEncoded(kv[1]) : "";
qsMap.add(key, val);
} catch (UnsupportedEncodingException e) {
log.error("Unsupported encoding in query string key: " + SecurityUtils.crlf(value), e);
log.error("Unsupported encoding in query string key-value pair: " + SecurityUtils.crlf(value), e);
}
}
return qsMap;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -584,7 +584,7 @@ private void addPart(Map<String, List<Part>> params, String fieldName, Part newP

protected String[] getQueryParamValues(MultiValuedTreeMap<String, String> qs, String key, boolean isCaseSensitive) {
List<String> value = getQueryParamValuesAsList(qs, key, isCaseSensitive);
if (value == null){
if (value == null) {
return null;
}
return value.toArray(new String[0]);
Expand All @@ -607,39 +607,57 @@ protected List<String> getQueryParamValuesAsList(MultiValuedTreeMap<String, Stri
}

protected Map<String, String[]> generateParameterMap(MultiValuedTreeMap<String, String> qs, ContainerConfig config) {
return generateParameterMap(qs, config, false);
}

protected Map<String, String[]> generateParameterMap(MultiValuedTreeMap<String, String> qs, ContainerConfig config, boolean decodeQueryParams) {
Map<String, String[]> output;

Map<String, List<String>> formEncodedParams = getFormUrlEncodedParametersMap();

if (qs == null) {
// Just transform the List<String> values to String[]
output = formEncodedParams.entrySet().stream()
return formEncodedParams.entrySet().stream()
.collect(Collectors.toMap(Map.Entry::getKey, (e) -> e.getValue().toArray(new String[0])));
} else {
Map<String, List<String>> queryStringParams;
if (config.isQueryStringCaseSensitive()) {
queryStringParams = qs;
} else {
// If it's case insensitive, we check the entire map on every parameter
queryStringParams = qs.entrySet().stream().parallel().collect(
Collectors.toMap(
Map.Entry::getKey,
e -> getQueryParamValuesAsList(qs, e.getKey(), false)
));
}

// Merge formEncodedParams and queryStringParams Maps
output = Stream.of(formEncodedParams, queryStringParams).flatMap(m -> m.entrySet().stream())
.collect(
Collectors.toMap(
Map.Entry::getKey,
e -> e.getValue().toArray(new String[0]),
// If a parameter is in both Maps, we merge the list of values (and ultimately transform to String[])
(formParam, queryParam) -> Stream.of(formParam, queryParam).flatMap(Stream::of).toArray(String[]::new)
));

}

// decode all keys and values in map
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a fairly minor thing, but all this new code has a mix of spaces and tabs (first indentation is 4 spaces, then only tabs). Even though the project has some files with a mix of both, the vast majority of files are using spaces only. So try to keep this one with spaces only too. (The tests added are also using tabs, and hopefully you can change them to spaces)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😱 ... fixed, everything is spaces now

final MultiValuedTreeMap<String, String> decodedQs = new MultiValuedTreeMap<String, String>();
if (decodeQueryParams) {
for (Map.Entry<String, List<String>> entry : qs.entrySet()) {
String k = decodeValueIfEncoded(entry.getKey());
List<String> v = getQueryParamValuesAsList(qs, entry.getKey(), false).stream()
.map(AwsHttpServletRequest::decodeValueIfEncoded)
.collect(Collectors.toList());
// addAll in case map has 2 keys that are identical once decoded
decodedQs.addAll(k, v);
}
} else {
decodedQs.putAll(qs);
}

Map<String, List<String>> queryStringParams;
if (config.isQueryStringCaseSensitive()) {
queryStringParams = decodedQs;
} else {
// If it's case insensitive, we check the entire map on every parameter
queryStringParams = decodedQs.entrySet().stream().collect(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to remove this parallel here?

I'm not sure how much it actually helps, but this has been used since the first versions of this, so I'm curious if you had a specific reason to remove it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reintroduced it for now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parallel streams actually perform worse than a sequential stream for small stream data sets due to the work involved in splitting up the data set, farming them out to threads, then recombining the results. Here's an article: https://blogs.oracle.com/javamagazine/post/java-parallel-streams-performance-benchmark.

And because I come from the old days where we had to get every last drop out of available memory and CPU because there was no magical cloud scaling :) So it's an old habit to be searching for these types of efficiency gains. I also reworked some usage in getQueryParameters() where arrays and lists were being created and immediately disposed of.

It's not a big deal in the end, but it's a good one to know. Parallel is not always the silver bullet it seems to be.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for sharing - makes sense! If @valerena agrees, we can address that for 2.1.1, I just didn't want to delay the 2.1.0 release any longer.

Collectors.toMap(
Map.Entry::getKey,
e -> getQueryParamValuesAsList(decodedQs, e.getKey(), false)
));
}

// Merge formEncodedParams and queryStringParams Maps
output = Stream.of(formEncodedParams, queryStringParams).flatMap(m -> m.entrySet().stream())
.collect(
Collectors.toMap(
Map.Entry::getKey,
e -> e.getValue().toArray(new String[0]),
// If a parameter is in both Maps, we merge the list of values (and ultimately transform to String[])
(formParam, queryParam) -> Stream.of(formParam, queryParam).flatMap(Stream::of).toArray(String[]::new)
));

return output;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -325,8 +325,17 @@ public String getContentType() {

@Override
public String getParameter(String s) {

// decode key if ALB
if (request.getRequestSource() == RequestSource.ALB) {
s = decodeValueIfEncoded(s);
}

String queryStringParameter = getFirstQueryParamValue(request.getMultiValueQueryStringParameters(), s, config.isQueryStringCaseSensitive());
if (queryStringParameter != null) {
if (request.getRequestSource() == RequestSource.ALB) {
queryStringParameter = decodeValueIfEncoded(queryStringParameter);
}
return queryStringParameter;
}

Expand All @@ -345,15 +354,35 @@ public Enumeration<String> getParameterNames() {
if (request.getMultiValueQueryStringParameters() == null) {
return Collections.enumeration(formParameterNames);
}
return Collections.enumeration(Stream.concat(formParameterNames.stream(),
request.getMultiValueQueryStringParameters().keySet().stream()).collect(Collectors.toSet()));

Set<String> paramNames = request.getMultiValueQueryStringParameters().keySet();
if (request.getRequestSource() == RequestSource.ALB) {
paramNames = paramNames.stream().map(AwsProxyHttpServletRequest::decodeValueIfEncoded).collect(Collectors.toSet());
}

return Collections.enumeration(
Stream.concat(formParameterNames.stream(), paramNames.stream())
.collect(Collectors.toSet()));
}


@Override
@SuppressFBWarnings("PZLA_PREFER_ZERO_LENGTH_ARRAYS") // suppressing this as according to the specs we should be returning null here if we can't find params
public String[] getParameterValues(String s) {

// decode key if ALB
if (request.getRequestSource() == RequestSource.ALB) {
s = decodeValueIfEncoded(s);
}

// TODO lots of back and forth arrays and lists here, sort it out!
List<String> values = new ArrayList<>(Arrays.asList(getQueryParamValues(request.getMultiValueQueryStringParameters(), s, config.isQueryStringCaseSensitive())));
// List<String> values = getQueryParamValuesAsList(request.getMultiValueQueryStringParameters(), s, config.isQueryStringCaseSensitive());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagine this comment shouldn't be here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also had a todo around cleaning up the conversions between arrays and lists happening here and the getQueryParamValues() method. This should be a little better with some extra null safety now and the commented code has been removed.


// decode values if ALB
if (request.getRequestSource() == RequestSource.ALB) {
values = values.stream().map(AwsHttpServletRequest::decodeValueIfEncoded).collect(Collectors.toList());
}

values.addAll(Arrays.asList(getFormBodyParameterCaseInsensitive(s)));

Expand All @@ -367,7 +396,7 @@ public String[] getParameterValues(String s) {

@Override
public Map<String, String[]> getParameterMap() {
return generateParameterMap(request.getMultiValueQueryStringParameters(), config);
return generateParameterMap(request.getMultiValueQueryStringParameters(), config, request.getRequestSource() == RequestSource.ALB);
}


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
import java.util.Base64;
import java.util.List;
import java.util.Map;
import java.util.Arrays;


public class AwsHttpServletRequestTest {
Expand All @@ -38,10 +37,14 @@ public class AwsHttpServletRequestTest {
.queryString("one", "two").queryString("three", "four").build();
private static final AwsProxyRequest queryStringNullValue = new AwsProxyRequestBuilder("/test", "GET")
.queryString("one", "two").queryString("three", null).build();
private static final AwsProxyRequest queryStringEmptyValue = new AwsProxyRequestBuilder("/test", "GET")
.queryString("one", "two").queryString("three", "").build();
private static final AwsProxyRequest encodedQueryString = new AwsProxyRequestBuilder("/test", "GET")
.queryString("one", "two").queryString("json", "{\"name\":\"faisal\"}").build();
.queryString("one", "two").queryString("json value@1", "{\"name\":\"faisal\"}").build();
private static final AwsProxyRequest encodedQueryStringAlb = new AwsProxyRequestBuilder("/test", "GET")
.queryString("one", "two").queryString("json value@1", "{\"name\":\"faisal\"}").alb().build();
private static final AwsProxyRequest multipleParams = new AwsProxyRequestBuilder("/test", "GET")
.queryString("one", "two").queryString("one", "three").queryString("json", "{\"name\":\"faisal\"}").build();
.queryString("one", "two").queryString("one", "three").queryString("json value@1", "{\"name\":\"faisal\"}").build();
private static final AwsProxyRequest formEncodedAndQueryString = new AwsProxyRequestBuilder("/test", "POST")
.queryString("one", "two").queryString("one", "three")
.queryString("five", "six")
Expand Down Expand Up @@ -200,6 +203,20 @@ void queryString_generateQueryString_nullParameterIsEmpty() {
assertTrue(parsedString.endsWith("three="));
}

@Test
void queryString_generateQueryString_emptyParameterIsEmpty() {
AwsProxyHttpServletRequest request = new AwsProxyHttpServletRequest(queryStringEmptyValue, mockContext, null, config);
String parsedString = null;
try {
parsedString = request.generateQueryString(request.getAwsProxyRequest().getMultiValueQueryStringParameters(), true, config.getUriEncoding());
} catch (ServletException e) {
e.printStackTrace();
fail("Could not generate query string");
}

assertTrue(parsedString.endsWith("three="));
}

@Test
void queryStringWithEncodedParams_generateQueryString_validQuery() {
AwsProxyHttpServletRequest request = new AwsProxyHttpServletRequest(encodedQueryString, mockContext, null, config);
Expand All @@ -212,7 +229,23 @@ void queryStringWithEncodedParams_generateQueryString_validQuery() {
fail("Could not generate query string");
}
assertTrue(parsedString.contains("one=two"));
assertTrue(parsedString.contains("json=%7B%22name%22%3A%22faisal%22%7D"));
assertTrue(parsedString.contains("json+value%401=%7B%22name%22%3A%22faisal%22%7D"));
assertTrue(parsedString.contains("&") && parsedString.indexOf("&") > 0 && parsedString.indexOf("&") < parsedString.length());
}

@Test
void queryStringWithEncodedParams_alb_generateQueryString_validQuery() {
AwsProxyHttpServletRequest request = new AwsProxyHttpServletRequest(encodedQueryStringAlb, mockContext, null, config);

String parsedString = null;
try {
parsedString = request.generateQueryString(request.getAwsProxyRequest().getMultiValueQueryStringParameters(), false, config.getUriEncoding());
} catch (ServletException e) {
e.printStackTrace();
fail("Could not generate query string");
}
assertTrue(parsedString.contains("one=two"));
assertTrue(parsedString.contains("json+value%401=%7B%22name%22%3A%22faisal%22%7D"));
assertTrue(parsedString.contains("&") && parsedString.indexOf("&") > 0 && parsedString.indexOf("&") < parsedString.length());
}

Expand All @@ -229,7 +262,7 @@ void queryStringWithMultipleValues_generateQueryString_validQuery() {
}
assertTrue(parsedString.contains("one=two"));
assertTrue(parsedString.contains("one=three"));
assertTrue(parsedString.contains("json=%7B%22name%22%3A%22faisal%22%7D"));
assertTrue(parsedString.contains("json+value%401=%7B%22name%22%3A%22faisal%22%7D"));
assertTrue(parsedString.contains("&") && parsedString.indexOf("&") > 0 && parsedString.indexOf("&") < parsedString.length());
}

Expand Down Expand Up @@ -265,6 +298,22 @@ void parameterMap_generateParameterMap_nullParameter() {
assertTrue(paramMap.size() == 2);
}

@Test
void parameterMap_generateParameterMap_emptyParameter() {
AwsProxyHttpServletRequest request = new AwsProxyHttpServletRequest(queryStringEmptyValue, mockContext, null, config);
Map<String, String[]> paramMap = null;
try {
paramMap = request.generateParameterMap(request.getAwsProxyRequest().getMultiValueQueryStringParameters(), config);
} catch (Exception e) {
e.printStackTrace();
fail("Could not generate parameter map");
}

assertArrayEquals(new String[]{"two"}, paramMap.get("one"));
assertArrayEquals(new String[]{""}, paramMap.get("three"));
assertTrue(paramMap.size() == 2);
}

@Test
void parameterMapWithEncodedParams_generateParameterMap_validQuery() {
AwsProxyHttpServletRequest request = new AwsProxyHttpServletRequest(encodedQueryString, mockContext, null, config);
Expand All @@ -278,7 +327,24 @@ void parameterMapWithEncodedParams_generateParameterMap_validQuery() {
}

assertArrayEquals(new String[]{"two"}, paramMap.get("one"));
assertArrayEquals(new String[]{"{\"name\":\"faisal\"}"}, paramMap.get("json"));
assertArrayEquals(new String[]{"{\"name\":\"faisal\"}"}, paramMap.get("json value@1"));
assertTrue(paramMap.size() == 2);
}

@Test
void parameterMapWithEncodedParams_alb_generateParameterMap_validQuery() {
AwsProxyHttpServletRequest request = new AwsProxyHttpServletRequest(encodedQueryStringAlb, mockContext, null, config);

Map<String, String[]> paramMap = null;
try {
paramMap = request.generateParameterMap(request.getAwsProxyRequest().getMultiValueQueryStringParameters(), config, true);
} catch (Exception e) {
e.printStackTrace();
fail("Could not generate parameter map");
}

assertArrayEquals(new String[]{"two"}, paramMap.get("one"));
assertArrayEquals(new String[]{"{\"name\":\"faisal\"}"}, paramMap.get("json value@1"));
assertTrue(paramMap.size() == 2);
}

Expand All @@ -294,7 +360,7 @@ void parameterMapWithMultipleValues_generateParameterMap_validQuery() {
fail("Could not generate parameter map");
}
assertArrayEquals(new String[]{"two", "three"}, paramMap.get("one"));
assertArrayEquals(new String[]{"{\"name\":\"faisal\"}"}, paramMap.get("json"));
assertArrayEquals(new String[]{"{\"name\":\"faisal\"}"}, paramMap.get("json value@1"));
assertTrue(paramMap.size() == 2);
}

Expand All @@ -319,12 +385,32 @@ void parameterMap_generateParameterMap_formEncodedAndQueryString() {
}

@Test
void parameterMap_generateParameterMap_differentCasing() {
AwsProxyHttpServletRequest request = new AwsProxyHttpServletRequest(differentCasing, mockContext, null, config);
void parameterMap_generateParameterMap_differentCasing_caseSensitive() {
ContainerConfig caseSensitiveConfig = ContainerConfig.defaultConfig();
caseSensitiveConfig.setQueryStringCaseSensitive(true);
AwsProxyHttpServletRequest request = new AwsProxyHttpServletRequest(differentCasing, mockContext, null, caseSensitiveConfig);
Map<String, String[]> paramMap = null;
try {
paramMap = request.generateParameterMap(request.getAwsProxyRequest().getMultiValueQueryStringParameters(), caseSensitiveConfig);
} catch (Exception e) {
e.printStackTrace();
fail("Could not generate parameter map");
}
assertArrayEquals(new String[] {"two", "three"}, paramMap.get("one"));
assertArrayEquals(new String[] {"four"}, paramMap.get("ONE"));
assertTrue(paramMap.size() == 2);
}

@Test
void parameterMap_generateParameterMap_differentCasing_caseInsensitive() {
ContainerConfig caseInsensitiveConfig = ContainerConfig.defaultConfig();
caseInsensitiveConfig.setQueryStringCaseSensitive(false);

AwsProxyHttpServletRequest request = new AwsProxyHttpServletRequest(differentCasing, mockContext, null, caseInsensitiveConfig);

Map<String, String[]> paramMap = null;
try {
paramMap = request.generateParameterMap(request.getAwsProxyRequest().getMultiValueQueryStringParameters(), config);
paramMap = request.generateParameterMap(request.getAwsProxyRequest().getMultiValueQueryStringParameters(), caseInsensitiveConfig);
} catch (Exception e) {
e.printStackTrace();
fail("Could not generate parameter map");
Expand All @@ -346,6 +432,17 @@ void queryParamValues_getQueryParamValues() {
assertNull(result2);
}

@Test
void queryParamValues_getQueryParamValues_nullValue() {
AwsProxyHttpServletRequest request = new AwsProxyHttpServletRequest(new AwsProxyRequest(), mockContext, null);
MultiValuedTreeMap<String, String> map = new MultiValuedTreeMap<>();
map.add("test", null);
String[] result1 = request.getQueryParamValues(map, "test", true);
assertArrayEquals(new String[] {null}, result1);
String[] result2 = request.getQueryParamValues(map, "TEST", true);
assertNull(result2);
}

@Test
void queryParamValues_getQueryParamValues_caseInsensitive() {
AwsProxyHttpServletRequest request = new AwsProxyHttpServletRequest(new AwsProxyRequest(), mockContext, null);
Expand All @@ -358,4 +455,17 @@ void queryParamValues_getQueryParamValues_caseInsensitive() {
assertArrayEquals(new String[]{"test", "test2"}, result2);
}

@Test
void queryParamValues_getQueryParamValues_multipleCaseInsensitive() {
AwsProxyHttpServletRequest request = new AwsProxyHttpServletRequest(new AwsProxyRequest(), mockContext, null);

MultiValuedTreeMap<String, String> map = new MultiValuedTreeMap<>();
map.add("test", "test");
map.add("TEST", "test2");
String[] result1 = request.getQueryParamValues(map, "test", false);
assertArrayEquals(new String[]{"test2"}, result1);
String[] result2 = request.getQueryParamValues(map, "TEST", false);
assertArrayEquals(new String[]{"test2"}, result2);
}

}
Loading
Loading