-
Notifications
You must be signed in to change notification settings - Fork 48
Upstream minurl::Url type for url parsing
#467
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
base: master
Are you sure you want to change the base?
Conversation
fd71a06 to
0f95668
Compare
fbe008a to
d56d850
Compare
|
@tcharding Hmm, to have the |
d56d850 to
13bf05d
Compare
TheBlueMatt
left a comment
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.
nice!
|
Mind adding a test that this fixes #468? |
c35d3ea to
f792086
Compare
24e1c26 to
eb3e3b1
Compare
Move the minurl URL parsing code to `bitreq/src/url.rs` and expose it as `bitreq::Url` and `bitreq::UrlParseError`. Co-Authored-By: HAL 9000 Signed-off-by: Elias Rohrer <dev@tnull.de>
eb3e3b1 to
668eb49
Compare
|
Now clippy should also be happy. |
|
I pulled down What if we bump the MSRV of |
Hmm, I think at last Summit the consensus was to only do that some time post-April when Ubuntu LTS 2604 is released. I think until then it would be preferable to keep 1.75.0 and just pin-back that one dependency. |
|
No worries, lets do it. The process is easy enough theoretically but sometimes annoying to do. Just import |
00d2cf1 to
925228f
Compare
Migrate the property-based tests from minurl to bitreq: - `url_properties.rs`: Tests that URL parsing properties hold - `url_parity.rs`: Tests parity with the `url` crate - `common/mod.rs`: Shared test utilities and strategies Also add the required dev-dependencies (`proptest` and `url`). Co-Authored-By: HAL 9000 Signed-off-by: Elias Rohrer <dev@tnull.de>
This commit replaces the internal `HttpUrl` type with the newly-migrated `Url` type. The change includes: - Add `pub(crate)` helper methods to `Url`: `is_https()`, `path_and_query()`, `append_query_params()`, `preserve_fragment_from()`, `write_base_url_to()`, and `write_resource_to()` - Extract field-parsing logic into `parse_inner` method for use by mutation methods, making them simpler and more robust - Update `ParsedRequest` to use `Url` instead of `HttpUrl` - Update `ConnectionParams` to use `u16` for port instead of `Port` enum - Simplify `http_url.rs` to only contain percent encoding functions - Update `Response::new()` to accept url as `String` instead of `HttpUrl` - Simplify WASM `build_full_url()` to use `Url::as_str()` Co-Authored-By: HAL 9000 Signed-off-by: Elias Rohrer <dev@tnull.de>
Add `append_query_param` method to `Url` that handles percent encoding internally, rather than having external code use the encoding functions directly. This encapsulates the encoding logic within the `Url` type. This ensures that existing percent-encoded query parameters in URLs are not double-encoded when new parameters are added, fixing issue rust-bitcoin#468. Co-Authored-By: HAL 9000 Signed-off-by: Elias Rohrer <dev@tnull.de>
925228f to
4fa9e80
Compare
Alright, now went ahead and did this, both for This should be good to go then. |
| tiny_http = "0.12" | ||
| tokio = { version = "1.0", default-features = false, features = ["macros", "rt-multi-thread", "time"] } | ||
| proptest = { version = "1", default-features = false, features = ["std"] } | ||
| url = { version = "2.4" } |
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.
Should we just hard-fix this version with an = to the latest that meets our MSRV?
| /// Parses a URL string and returns a `Url` instance. | ||
| /// | ||
| /// Validates that the input contains only valid non-control ASCII characters. | ||
| pub fn parse(url_str: &str) -> Result<Self, ParseError> { |
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.
It really does kinda seem like this fn could use a fuzzer. AFAIU proptest isn't a replacement for gritty logic like this that has lots of potentially-panic-y indexing.
Fixes #468.
The
urlcrate has a notoriously large dependency tree which is why we want to avoid it as far as possible. However, we found us then re-implementing several aspects of URL parsing in different places. To this end, I recently (mostly vibe-)coded the 0-dependencyminurlcrate (https://github.com/tnull/minurl) which is meant as a drop-in replacement for the popularurlcrate.To this end, we kept the
UrlAPI completely compatible, and even added parity tests ensuring both APIs return exactly the same output given the same input.While I'm generally fine maintaining this as a separate crate, it makes a lot of sense to have this live as part of
bitreqand hence have it available everywhere in the ecosystem. Here I propose to upstream theminurl::Urltype (and ofc the corresponding test code). This also allows us to replace the ~half-donehttp_url::HttpUrltype.