Query parameter values not URL decoded when using a RequestStreamHandler implementation #1089
Conversation
…ter names in HTTP API requests
There was a problem hiding this comment.
Thanks for the very detailed PR. I added a couple of minor comments.
Also something that wasn't clear to me is: if the problem is with API GW V2, then why are there changes related to ALB too? Is there a separate bug with ALB encoding as well? Or is it related to the same issue?
| } | ||
|
|
||
| // decode all keys and values in map |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
😱 ... fixed, everything is spaces now
| List<String> values = new ArrayList<>(Arrays.asList(getQueryParamValues(request.getMultiValueQueryStringParameters(), s, config.isQueryStringCaseSensitive()))); | ||
| // List<String> values = getQueryParamValuesAsList(request.getMultiValueQueryStringParameters(), s, config.isQueryStringCaseSensitive()); |
There was a problem hiding this comment.
I imagine this comment shouldn't be here
There was a problem hiding this comment.
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.
As I began fixing the unit tests and adding tests for query param encoding/decoding, it revealed that the handling of encoded query params had issues in other areas as well. So this bug did expand into fixing query param encoding across all of the integration types. |
jpfennelly
force-pushed
the
976-query-params-not-urldecoded
branch
from
feff940
to
a202873
Compare
| queryStringParams = decodedQs; | ||
| } else { | ||
| // If it's case insensitive, we check the entire map on every parameter | ||
| queryStringParams = decodedQs.entrySet().stream().collect( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
deki
changed the title
|
Thank you very much for your contribution, @jpfennelly. Will be part of the 2.1.0 release. |
Issue #, if available: 976
Description of changes:
AwsProxyRequestBuilder, used by unit tests, to be immutable when calling thealb()method. Previously, a call to thealb()method would encode query parameters, modifying the state of the builder. Subsequent usage of the builder for non-ALB unit tests would use the now-encoded query parameters. This wasn't an issue in current current tests because no tests were testing query param decoding. Added unit tests for the builder class itself.generateParameterMap()method that takes a flag to determine if it should decode query parameters so that functionality for ALB and non-ALB requests can be supportedBy submitting this pull request