Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upQueryStringBindable.unbind(): Do-do-do URLEncode for all queryString keys! #10370
Conversation
helllamer
changed the title
|
PR has been rewritten from scratch. |
helllamer
force-pushed the
suggestio:master
branch
7 times, most recently
from
8fea400
to
e2a1171
| @@ -309,8 +319,7 @@ object QueryStringBindable { | |||
|
|
|||
| // Use an option here in case users call index(null) in the routes -- see #818 | |||
| def unbind(key: String, value: String) = | |||
| URLEncoder.encode(Option(key).getOrElse(""), "utf-8") + "=" + URLEncoder | |||
| .encode(Option(value).getOrElse(""), "utf-8") | |||
| _urlEncode(key) + "=" + Option(value).fold("")(_urlEncode) | |||
ihostage
Jul 1, 2020
Member
Why Option dropped for key? 🤔 He is no longer needed?
Why Option dropped for key?
helllamer
Jul 1, 2020
Author
Possibly, it never needed.
Option(key).getOrElse() added year ago in #9403 (diff) during fixing URLEncode for values.
- In
Binders.scala there are no Option(key) for any other binders (Char, Long, Int, UUID, etc).
- There are zero tests for such behavior, where parsed queryString contains null keys with String-only values.
- Generated
Router.scala can't pass nulls as parameter names.
null as keys in scala Map[String,_] must always throw NPE. It indicates serious problem in somewhere outer logic.
Possibly, it never needed.
Option(key).getOrElse() added year ago in #9403 (diff) during fixing URLEncode for values.
- In
Binders.scalathere are noOption(key)for any other binders (Char, Long, Int, UUID, etc). - There are zero tests for such behavior, where parsed queryString contains null keys with String-only values.
- Generated
Router.scalacan't passnulls as parameter names. nullas keys in scalaMap[String,_]must alwaysthrow NPE. It indicates serious problem in somewhere outer logic.
helllamer
Jul 1, 2020
•
Author
@ihostage Этот Option(key) появился, т.к. код поддержки URLEncode просто скопипастили из value-кода в той же строке. Для value действительно нужен Option(), о чём в комменте там выше написано.
@ihostage Этот Option(key) появился, т.к. код поддержки URLEncode просто скопипастили из value-кода в той же строке. Для value действительно нужен Option(), о чём в комменте там выше написано.
ihostage
Jul 2, 2020
Member
Ok 👌
Ok
|
@helllamer Sorry, I'm not a maintainer and can't edit PR |
|
Sorry, I am too busy these days... Maybe @ignasi35 and/or @renatocaval can review? |
Added docs for safety implementing own custom QSBs. Tests for several other QSB implementations. Fixes #10369
Fixes #10369 - Let's DO
URLEncode.encode()for all queryString keys in QSB.unbind().Unify unbind() behaviour over all query-string binders.
Currently, different binders handle qs-keys differently (look at
res3):Special characters like
[,]and others MUST be Form-URL-encoded according to RFC.Added documentation about applying
URLEncodefor handy-implemented unbinders.Added tests for several common types of
QueryStringBindables.Pull Request Checklist
Helpful things
Fixes
Fixes #10369