Skip to content
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

Normalize trailing slash on Uri values #1261

Closed
jimmycuadra opened this issue Jul 14, 2017 · 2 comments
Closed

Normalize trailing slash on Uri values #1261

jimmycuadra opened this issue Jul 14, 2017 · 2 comments

Comments

@jimmycuadra
Copy link

While working on rust-etcd I ran into a problem with hyper::Uri where it did not normalize trailing slashes as I expected. I'm not sure if this is intended behavior or not, but it definitely tripped me up and I spent some time determining it was the cause of my bug, so I thought I'd bring it up.

Here is the behavior I'm seeing:

extern crate hyper;

use hyper::Uri;

fn main() {
    let no_path: Uri = "http://example.com".parse().unwrap();
    let root_path: Uri = "http://example.com/".parse().unwrap();

    println!("{}", no_path);
    println!("{}", no_path.path());

    println!("{}", root_path);
    println!("{}", root_path.path());
}

Output:

http://example.com
/
http://example.com/
/

I have an internal method that looks roughly like this:

fn build_url(uri: &Uri, path: &str) -> String {
    format!("{}v2/keys{}", uri, path)
}

If the string the Uri was built from ended in a slash (e.g. "https://example.com/"), this would work as expected. If it didn't (e.g. "https://example.com"), the output of my build_url function would come out as "https://example.comv2/keys/whatever", which is wrong.

I can work around this now that I know it's happening, but I'm wondering if it'd be appropriate for hyper::Uri to do some normalization here. If the current behavior is expected, it's probably worth talking about it in the documentation, as I doubt I'm the last person who will be surprised by this.

Probably also worth mentioning that I'm only doing this string manipulation because of continued pain with #1102—if I could simply do uri.set_path(path) that would be much better. I do understand why it was decided to use a separate Uri type rather than using the url crate, but as a user, having to deal with both types is unpleasant.

@algermissen
Copy link

algermissen commented Sep 20, 2017

A URI can have an empty path segment per spec. In general care must be taken not to confuse URIs and URI-References and the issues involved with resolving a URI reference against a context URI. In addition are the rules for HTTP request URIs. Then also, URIs and proxy requests can be unintuitive but exceptional cases be very useful (eg. doing a proxy requests to a non HTTP URI)

Looking at the current implementation briefly it looks not quite right but seems to focus on HTTP Requests instead of full URI spectrum.

In general, I think the best advice is to stick to the RFCs very strictly and let them guide the architecture of the various pieces (structs). Scala's Akka-HTTP did a good job, AFAIK and it certainly will be helpful to run the design by an expert briefly. I'll try to get one of them interested.

Sorry, I have never myself implemented an HTTP lib, just know that non-strict implementations cause a lot of pain down the line if you want to do real HTTP.

Akka-HTTP has always struck me as one of the truly better implementations around URI and HTTP specs. Eg:

http://doc.akka.io/docs/akka-http/current/scala/http/common/uri-model.html

Here is the explicit support for path-emptyness: http://doc.akka.io/japi/akka/2.4.4/akka/http/scaladsl/model/Uri.Path.html#isEmpty--

Jan

@seanmonstar
Copy link
Member

This is fixed in a9413d7, will be release soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants