#720: Do not double-escape URL query strings in HTTP API v2 functions #721
Conversation
|
Thank you for the PR! We've tested and are planning to accept it, and may combine with another fix we have in progress internally since they're introducing changes to existing behavior. |
|
@ashovlin Do you have any update on the progress for accepting this PR, or has that other fix mentioned caused this PR to no longer be relevant? |
|
Approved the PR. It makes sense to get the behavior to running ASP.NET Core through Kestrel. Because this is a breaking change we will need to do a major version bump to 6.0.0. |
There was a problem hiding this comment.
Minor, but might be nice to update the comment above CreateQueryStringParametersFromHttpApiV2 to clarify that the raw query string now matches the intended encoding and we're not encoding again.
(I know this sat a long time, so if not quick to revisit I imagine we can also clean it up when adding the commit for the version bump)
|
Thanks! I've updated the code comment, and also merged with master to aid merging this PR. |
|
Thanks! Updated docs look good. |
|
PR released as part of version 6.0.0 |
Issue #, if available: #720
Description of changes: Remove code to escape query string, since it is already escaped in HTTP API v2. My guess is that this was implemented because v1 events needed to build a query string from unescaped values.
This may change the behaviour of issues like #237 (though that one must've been filed for API v1). However, + being replaced by space is done by ASP.NET Core MVC and not something that should be attempted to be suppressed by the lambda framework.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.