- Notifications
You must be signed in to change notification settings - Fork1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
KTOR-7190: Add support for encoded non-text queries#4110
base:main
Are you sure you want to change the base?
KTOR-7190: Add support for encoded non-text queries#4110
Conversation
@bjhham Please take a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
So there's a few things I don't like about this approach of using two maps for the parameters:
- The order of the parameters coming out is not the same as it is going in.
- It further complicates an already-complicated set of types.
- It creates problems for any code interacting with
Url.parameterswhere it's expecting a complete set.
I'd propose removing theisDecodable function and instead introducing a new line of functions likeString.tryDecode that instead returned a sealed typeUrlString { Decoded, Encoded }, then theParameters andParametersBuilder types can just use these types internally instead of decoding everything then encoding again later.
I can help with updating theParameter types if you'd like; I've been working on a pretty large refactor of our URL types to try to resolve some of our problems with default requests etc.
@bjhham Thank you for your review. I'll try to update the code according to your proposal shortly. |
Subsystem
The changes only affect URLBuilder internals and some other internal APIs. No public API was affected.
Motivation
It fixeshttps://youtrack.jetbrains.com/issue/KTOR-7190/URLBuilder.encodedParameters-cant-handle-encoded-non-text-query-data.
Solution
Former solution assumed that any query parameter is able to be decoded as an UTF-8 string. And any encoded query parameter was decoded during the building an URL and after that encoded back to create a textual representation of an URL. Unfortunately it crashes an app if it tries to add something like bittorrent info_hash parameter (also see#1351 (comment)) because there are a lot of byte sequences which can't be decoded as an UTF-8 string.
My solution reverses the data flow. I added one special ParametersBuilder for undecodable query parameters where they are stored in encoded form. And URLBuilder.encodedParameters became just a wrapper which stores decodable parameters in URLBuilder.parameters and undecodable ones in the private URLBuilder.rawEncodedParameters.