feat(server): add path() and query() to Request

[ahmedcharles]

Fixes #896 and #897.

Also improve RequestUri tests.

[ahmedcharles]

Are there tests for Request that I should update? I didn't find any obvious ones. Alternatively, I can write unit tests for Request.

[seanmonstar]

I was imagine using something more primitive:

let end = s.find('?').unwrap_or(s.len());
&s[..end]

Is there a reason that this is incorrect?

[ahmedcharles]

I was going for consistency and doing it in a way that I don't have to read the RFC and implement correct parsing and interpretation of urls. Url::parse normalizes the urls, and therefore, it makes a copy. It would be weird if normalized paths and queries were returned for AbsoluteUri but not for AbsolutePath.

[seanmonstar]

Oh ok, good call. I'm wondering whether it's better to run the Url parser
when constructing the RequestUri, and thus be able to return a reference,
or do it on demand, as this patch does.

On Tue, Aug 23, 2016, 7:30 AM Ahmed Charles [email protected]
wrote:

In src/server/request.rs
#899 (comment):

 /// The target path of this Request.
 #[inline]

• pub fn path(&self) -> Option<&str> {

•    match *self.uri {
  

•        RequestUri::AbsolutePath(ref s) => Some(s),
  

•        RequestUri::AbsoluteUri(ref url) => Some(&url[::url::Position::BeforePath..]),
  

•        _ => None
  

• pub fn path(&self) -> Option<Cow> {

•    match self.uri {
  

•        RequestUri::AbsolutePath(ref s) => {
  

•            let url = Url::parse("http://example.com").ok().and_then(|u| u.join(s).ok());
  

I was going for consistency and doing it in a way that I don't have to
read the RFC and implement correct parsing and interpretation of urls.
Url::parse normalizes the urls, and therefore, it makes a copy. It would be
weird if normalized paths and queries were returned for AbsoluteUri but not
for AbsolutePath.

—
You are receiving this because you commented.

Reply to this email directly, view it on GitHub
https://github.com/hyperium/hyper/pull/899/files/e4ec7e0644b36b81407ec1899036c5c63b1dc2b9#r75877722,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADJF2Xxk2msmuotBxxEluLzk2527Onvks5qiwQMgaJpZM4JqgGd
.

[ahmedcharles]

I also found that appealing as an option. Though I didn't want to change the RequestUri enum. Also, I'm not sure what the appropriate design would be. It'd be nice if Url supported parsing partial urls, then it could ignore the domain.

Perhaps it would be best to implement a custom Url to return as part of AbsolutePath instead of just a String. That way we can return references to owned data while only parsing once, up front.

I'll take a look and see what I come up with.

[seanmonstar]

The spec claims this variant has "absolute-path [ "?" query ]". Maybe its worth adjusting the variant to AbsolutePath { path: String, query: Option<String> }?

[ahmedcharles]

Did you notice the test changes and do you know if they are actually correct or is the implementation buggy? It seems like some things that are accepted should be rejected.

[seanmonstar]

This one is surprising. I would expect it to be a parse error.

[ahmedcharles]

I implemented all of the suggested changes, though it doesn't fix one of the test cases, since I'm not entirely sure how to go about changing the request uri parser.

[seanmonstar]

travis logs say the code examples in the guide are now failing.

[ahmedcharles]

I guess I didn't run the doc tests. I'll do that. :)

[seanmonstar]

Excellent, thanks for sticking through to the end!

[ahmedcharles]

No problem. I really like that Rust has a tool for testing documentation, since that's something that's always out of date.