Skip to content
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 our terms of service and privacy 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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

demitsuri
Copy link

Subsystem
The changes only affect URLBuilder internals and some other internal APIs. No public API was affected.

Motivation
It fixes https://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.

@e5l e5l requested a review from bjhham July 11, 2024 09:32
@demitsuri
Copy link
Author

@bjhham Please take a look.

Copy link
Contributor

@bjhham bjhham left a 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:

  1. The order of the parameters coming out is not the same as it is going in.
  2. It further complicates an already-complicated set of types.
  3. It creates problems for any code interacting with Url.parameters where it's expecting a complete set.

I'd propose removing the isDecodable function and instead introducing a new line of functions like String.tryDecode that instead returned a sealed type UrlString { Decoded, Encoded }, then the Parameters and ParametersBuilder types can just use these types internally instead of decoding everything then encoding again later.

I can help with updating the Parameter 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.

@demitsuri
Copy link
Author

@bjhham Thank you for your review. I'll try to update the code according to your proposal shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants