Conversation
this function is an alternative to `Url::join` which addresses issues discussed in servo#333, mainly that `Url::join` is sensitive to trailing slashes is in the `Url`, and if the trailing slash is missing, may remove segments from the base url and replace them with segments from the joined `Url`. There are good reasons for `Url::join` to behave that way, because that is was is specified in the `Url` standard. However it's still inconvenient because it often leads to situations where, a service takes some base-url for some API as a config parameter, uses `Url::join` to append various routes to it and make requests, and if a trailing `/` is omitted in a config file, you don't figure it out until deploying and looking at logs and seeing nonsense requests failing. In many situations in web development these trailing `/` are not significant so this is easy to forget and can become just an annoying papercut. One suggestion in servo#333 was to add an alternative utility function that isn't sensitive to the trailing `/`'s in this way. This commit adds such a utility function with tests.
| // Remove any leading `/` from the path we are appending, this makes our code tolerate leading `/`'s | ||
| let path = path.as_ref(); | ||
| let path = path.strip_prefix('/').unwrap_or(path); | ||
| for segment in path.split('/') { |
There was a problem hiding this comment.
this splitting is based on similar code elsewhere in the library: https://docs.rs/url/2.5.0/src/url/lib.rs.html#1351
|
As a library user, I would absolutely love to see this merged! |
|
This PR has been out for a while and has gotten stale - is there something blocking this from being merged? |
|
Reviewer time, deciding if we want to support this, etc. In general I'm wary of adding more APIs here, but I do think this is a good idea overall. cc @valenting |
|
My 2 cents: As author explained, in the backend development this is the most common use case, to take a user-supplied base url and append a fixed api path to it. Since warning users about trailing slashes doesn't make sense (just makes their lifes harder) the most reliable solution is to create additional wrappers, like this one for example: https://docs.rs/famedly_rust_utils/latest/famedly_rust_utils/struct.BaseUrl.html. If this PR gets merged, in many areas it will be project/company-wide policy to use The wariness is completely understandable as this crate is the backbone of the huge chunk of rust OSS, but I think it's a small and uninvasive feature offering a solution to a long-discussed issue |
I'm happy to iterate on it if maintainers are interested, just let me know. Thanks! |
|
I'm supportive of this approach. One thing I'm not sure about is what should happen if you call appent_path("path/to/file?query#hash") |
| /// Fails if the Url is cannot-be-a-base. | ||
| #[allow(clippy::result_unit_err)] | ||
| #[inline] | ||
| pub fn append_path(&mut self, path: impl AsRef<str>) -> Result<(), ()> { |
There was a problem hiding this comment.
I would be nice if this method could take &self and return Result<Url, _> so that it's a drop-in replacement for join.
There was a problem hiding this comment.
Specifically I propose renaming this method to something like append_path_mut and adding another method with the signature fn append_path(&self, path: impl AsRef<str>) -> Result<Self, ()>, similar to join. Unfortunately the implementation requires cloning the url but that kinda comes with taking &self and returning Result<Self, _>.
| Err(()) | ||
| } | ||
|
|
||
| /// Append path segments to the path of a Url, escaping if necesary. |
There was a problem hiding this comment.
Typo: necesary should be necessary
| /// assert_eq!(my_url.as_str(), "http://www.example.com/api/v1/system/status"); | ||
| /// ``` | ||
| /// | ||
| /// Fails if the Url is cannot-be-a-base. |
There was a problem hiding this comment.
Add # Errors section (see clippy::missing_errors_doc)
# Errors
Fails if the Url is cannot-be-a-base.
This comment has been minimized.
This comment has been minimized.
|
Please do not post "me too" comments on this thread, they are pure noise and just serve to waste time. This is currently blocked on someone cleaning up the implementation and rebasing it. |
|
I rebased this branch, addressed the review comments and made a change I would like to use myself in #1116. |
8 participants
This
append_pathfunction is an alternative toUrl::joinwhich addresses issues discussed in #333, mainly thatUrl::joinis sensitive to trailing slashes is in theUrl, and if the trailing slash is missing, may remove segments from the base url and replace them with segments from the joinedUrl.There are good reasons for
Url::jointo behave that way, because that is was is specified in theUrlstandard. (mentioned here: #333 (comment))However it's still inconvenient because it often leads to situations where, a service takes some base-url for some API as a config parameter, uses
Url::jointo append various routes to it and make requests, and if a trailing/is omitted in the config, you don't figure it out until deploying and looking at logs and seeing nonsense requests failing. In many situations in web development these trailing/are not significant so this is easy to forget and can become just an annoying papercut.One suggestion in #333 was to add an alternative utility function that isn't sensitive to the trailing
/'s in this way. This commit adds such a utility function with tests.I've been copy-pasting this around in several projects, I figured I would PR it back and see if there was any interest since it was discussed in the github issue.