Uri: inconsistent query param names (url-encode) when .renderString-ing #7127 #7241
Conversation
| test("Uri.renderString should Encode special chars in the query") { | ||
| val u = Uri(path = Uri.Path.Root).withQueryParam("foo", " !$&'()*+,;=:/?@~") | ||
| assertEquals(u.renderString, "/?foo=%20%21%24%26%27%28%29%2A%2B%2C%3B%3D%3A/?%40~") | ||
| } |
There was a problem hiding this comment.
Sorry, why did we remove this test? Is it asserting some wrong behavior?
There was a problem hiding this comment.
Yes, correct. It was good for the old QueryNoEncode characters, but for the new ones, this test case is no longer needed.
| @@ -24,7 +24,7 @@ import java.nio.charset.{Charset => JCharset} | |||
| /* Exists to work around circular dependencies */ | |||
| private[http4s] object UriCoding { | |||
| val Unreserved: CharPredicate = AlphaNum ++ "-_.~" | |||
| val QueryNoEncode: CharPredicate = Unreserved ++ "?/" | |||
| val QueryNoEncode: CharPredicate = Unreserved ++ "!$&'()*+,;=:/?@" | |||
There was a problem hiding this comment.
JFTR, the current implementation is the same as defined in Wikipedia 🤔
There was a problem hiding this comment.
Yes, I know it. I found several articles that says otherwise like in the one that I linked in the PR description. However, if @armanbilge says the same thing as you that we should stay with the one that Wikipedia "approves". Then I will redo my changes and will modify my solution. :)
There was a problem hiding this comment.
@danicheg Updated the PR. Probably the checks will fail on two test cases:
-
The first failing test is this one:
This should not be accepted as valid uri during parsing: https://en.wikipedia.org/wiki/Query_string#URL_encoding:~:text=The%20series%20of%20pairs%20is%20separated -
The second failing test is:
This should not be accepted anymore, because brackets should be encoded in the query parameters
What do you think, what should we do with these? Should i make them pass, or should I remove them?
|
I need to close this PR, because the branch bugged on my side. Opening a new one. |
|
New PR: #7243 |
This PR fixes #7127
The origin of the issue was that when we create a Uri object, the passed query value(s) doesn't get encoded. This causes the inconsistency that is mentioned in the Issue desciption.