Fix mutating path of URL without authority (idempotency, empty path segments) #996
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #996 +/- ##
=======================================
Coverage ? 80.52%
=======================================
Files ? 24
Lines ? 4329
Branches ? 0
=======================================
Hits ? 3486
Misses ? 843
Partials ? 0 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
If I'm not mistaken, these failing tests should also be made to pass as part of this test
rust-url/url/tests/expected_failures.txt
Lines 39 to 40 in da64903
That's about mutating the host, not the path, but I think the behaviour is closely related to this one.
|
@valenting, should I be worried about this failure? |
|
It seems the MSRV for libc changed Feel free to also increase ours to 1.63 |
theskim
force-pushed
the
main
branch
2 times, most recently
from
bc7d66c to
f8741dc
Compare
|
Need to add tests to check |
theskim
force-pushed
the
main
branch
6 times, most recently
from
e15a142 to
d14a9f4
Compare
theskim
force-pushed
the
main
branch
3 times, most recently
from
cff3eba to
dc26bfd
Compare
|
@valenting, thoughts on this patch? As you can see, now we actually check the position of host, ref, etc. |
There was a problem hiding this comment.
Sorry for the delayed review.
I think there are still some improvements and simplifications that can be made.
Have a look at main...valenting:rust-url:pr-996 - I added a few extra tests too.
| // 1. The host is null | ||
| // 2. The url's path length is greater than 1 | ||
| // 3. the first segment of the URL's path is an empty string | ||
| if !has_host && path.len() > 1 && path_empty { |
There was a problem hiding this comment.
I don't know if this works for a URL we've already parsed.
For example, this test case fails:
#[test]
fn test_valid_indices_after_set_path2() {
// Testing everything
let mut url = Url::parse("moz:/").unwrap();
assert_eq!(url.as_str(), "moz:/");
assert!(!url.cannot_be_a_base());
url.set_path("//p");
assert_eq!(url.as_str(), "moz:/.//p");
url.set_path("//d");
assert_eq!(url.as_str(), "moz:/.//d");
}I think the problem is that has_host is computed before we remove it again in line 1783
| assert_eq!(url.query(), Some("query")); | ||
| assert_eq!(url.fragment(), Some("frag")); | ||
| url.check_invariants().unwrap(); | ||
| } |
There was a problem hiding this comment.
The test I suggested in my prev commit still fails:
#[test]
fn test_set_path_again() {
// Testing everything
let mut url = Url::parse("moz:/").unwrap();
assert_eq!(url.as_str(), "moz:/");
assert!(!url.cannot_be_a_base());
url.set_path("//p");
assert_eq!(url.as_str(), "moz:/.//p");
url.set_path("//d");
assert_eq!(url.as_str(), "moz:/.//d");
}| // 1. The host is null | ||
| // 2. The url's path length is greater than 1 | ||
| // 3. the first segment of the URL's path is an empty string | ||
| if !has_host && segment.len() > 1 && path_empty { |
There was a problem hiding this comment.
| if !has_host && segment.len() > 1 && path_empty { | |
| if !has_host && self.path().starts_with("//") { |
I think the entire If can be simplified to this?
From Issue #984