Open lilyball opened 2 years ago
For example, converting absolute URL to relative with this code:
let mut parts = url.into_parts();
parts.scheme = None;
parts.authority = None;
if parts.path_and_query.is_none() {
parts.path_and_query = Some(PathAndQuery::from_static("/"));
}
won’t work (and will silently produce an empty URI which I don’t think should be even possible, given that Uri::from_static("")
panics). Even checking that .path_and_query.as_str() != ""
won’t work, since .as_str()
converts ""
to "/"
.
The proper code is something like
if parts.path_and_query.filter(|x| x.as_str() != "/")).is_none() {
parts.path_and_query = Some(PathAndQuery::from_static("/"));
}
which is weird and unintuitive.
PathAndQuery::from_static("")
tries to pretend to bePathAndQuery::from_static("/")
, even going so far as to have the sameDebug
printing, returning the same string from.as_str()
, and comparing as the same withPartialOrd
. But they compare differently withPartialEq
. Comparing twoUri
s that differ in this way works asUri
compares thepath()
andquery()
separately, butPathAndQuery == PathAndQuery
just compares the raw data.Similarly if I have two non-empty
PathAndQuery
s where one is missing the leading slash then bothPartialEq
andPartialOrd
say they're unequal despite still displaying them the same fromDebug
/Display
and despiteUri
considering them the same when comparing at that level (which is becauseUri
comparespath()
andquery()
instead of the underlyingpath_and_query
). This can be tested by appending?foo
to the two URLs in the above example: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 usingPathAndQuery::from_static("foo")
and this actually works, but the returned Uri then leaves the path as-is and serializes likehttp://example.comfoo
. This is actually rather surprising given how muchUri
andPathAndQuery
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"*"
asPathAndQuery
explicitly supports that. Given that asterisk-form comes fromrequest-target
and that this is whatUri
is documented as being used for, support for*
should probably be handled at theUri
level instead ofPathAndQuery
(especially as it doesn't represent a path at all). Doing that would simplify the handling ofPathAndQuery
slightly and fix issues like howPathAndQuery::from_static("foo")
prints as/foo
butPathAndQuery::from_static("*foo")
prints as just*foo
.In any case,
PartialEq
andPartialOrd
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.