`PathAndQuery == PathAndQuery` doesn't handle empty paths properly
Nobody has claimed this yet.
- Dominant language
- Rust
- Stars
- 1.4k
- Forks
- 378
- Avg merge
- 1d 21h
- Merged PRs (30d)
- 5
Description
PathAndQuery::from_static("") tries to pretend to be PathAndQuery::from_static("/"), even going so far as to have the same Debug printing, returning the same string from .as_str(), and comparing as the same with PartialOrd. But they compare differently with PartialEq. Comparing two Uris that differ in this way works as Uri compares the path() and query() separately, but PathAndQuery == PathAndQuery just compares the raw data.
let uri1 = dbg!(Uri::from_static("http://example.com"));
let uri2 = dbg!(Uri::from_static("http://example.com/"));
// Test URI behavior
assert_eq!(uri1.to_string(), uri2.to_string()); // same Display
assert_eq!(format!("{uri1:?}"), format!("{uri2:?}")); // same Debug
assert_eq!(uri1, uri2); // equal
// Test PathAndQuery behavior
let path1 = dbg!(uri1.path_and_query()).unwrap();
let path2 = dbg!(uri2.path_and_query()).unwrap();
assert_eq!(path1.to_string(), path2.to_string()); // same Display
assert_eq!(format!("{path1:?}"), format!("{path2:?}")); // same Debug
assert!(path1 <= path2 && path1 >= path2); // PartialOrd says they're equal
assert_eq!(path1, path2); // FAILURE
Similarly if I have two non-empty PathAndQuerys where one is missing the leading slash then both PartialEq and PartialOrd say they're unequal despite still displaying them the same from Debug/Display and despite Uri considering them the same when comparing at that level (which is because Uri compares path() and query() instead of the underlying path_and_query). This can be tested by appending ?foo to the two URLs in the above example:
let uri1 = dbg!(Uri::from_static("http://example.com?foo"));
let uri2 = dbg!(Uri::from_static("http://example.com/?foo"));
// …
Also
The fact that PathAndQuery tries so hard to pretend that an empty path is the same as a slash is somewhat confusing in general and just seems ripe for having bugs like this going forward. It's also completely undocumented, and existing documentation even implies differently (e.g. Uri::path() says that it may return an empty string, though in practice it only does so for authority-only relative URIs).
There's also other oddities like I can construct a Uri from parts using PathAndQuery::from_static("foo") and this actually works, but the returned Uri then leaves the path as-is and serializes like http://example.comfoo. This is actually rather surprising given how much Uri and PathAndQuery try to pretend that it has a leading slash when it doesn't. Even if it was changed to disallow "foo" this would still fail on "*" as PathAndQuery explicitly supports that. Given that asterisk-form comes from request-target and that this is what Uri is documented as being used for, support for * should probably be handled at the Uri level instead of PathAndQuery (especially as it doesn't represent a path at all). Doing that would simplify the handling of PathAndQuery slightly and fix issues like how PathAndQuery::from_static("foo") prints as /foo but PathAndQuery::from_static("*foo") prints as just *foo.
In any case, PartialEq and PartialOrd should be fixed in these cases, but some thought really should be given to structuring this in a way that avoids sprinkling all this logic everywhere about edge cases and hoping that everything matches up correctly.
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with the PathAndQuery and Uri comparison and formatting behavior described in the issue, using the provided empty-path, slash, query, missing-leading-slash, and asterisk examples as regression cases. Determine how PartialEq and PartialOrd should agree with the displayed and Uri-level representations, and consider the suggested separation of asterisk-form handling. Done means the comparison inconsistencies are fixed and the relevant edge cases are covered by tests.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- networking
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100