Merge pull request #1115 from hyperium/uri-fixes

Several Uri fixes

src/http/h1/parse.rs

@@ -344,7 +344,7 @@ mod tests {
         let (req, len) = parse::<http::ServerTransaction, _>(&mut raw).unwrap().unwrap();
         assert_eq!(len, expected_len);
         assert_eq!(req.subject.0, ::Method::Get);
-        assert_eq!(req.subject.1, "/echo".parse().unwrap());
+        assert_eq!(req.subject.1, "/echo");
         assert_eq!(req.version, ::HttpVersion::Http11);
         assert_eq!(req.headers.len(), 1);
         assert_eq!(req.headers.get_raw("Host").map(|raw| &raw[0]), Some(b"hyper.rs".as_ref()));

src/uri.rs

@@ -3,6 +3,7 @@ use std::fmt::{Display, self};
 use std::str::{self, FromStr};
 
 use http::ByteStr;
+use bytes::{BufMut, BytesMut};
 
 /// The Request-URI of a Request's StartLine.
 ///
@@ -32,8 +33,8 @@ pub struct Uri {
     source: ByteStr,
     scheme_end: Option<usize>,
     authority_end: Option<usize>,
-    query: Option<usize>,
+    query_start: Option<usize>,
-    fragment: Option<usize>,
+    fragment_start: Option<usize>,
 }
 
 impl Uri {
@@ -43,13 +44,7 @@ impl Uri {
             Err(UriError(ErrorKind::Empty))
         } else if s.as_bytes() == b"*" {
             // asterisk-form
-            Ok(Uri {
+            Ok(asterisk_form())
-                source: ByteStr::from_static("*"),
-                scheme_end: None,
-                authority_end: None,
-                query: None,
-                fragment: None,
-            })
         } else if s.as_bytes() == b"/" {
             // shortcut for '/'
             Ok(Uri::default())
@@ -61,13 +56,13 @@ impl Uri {
                 source: s,
                 scheme_end: None,
                 authority_end: None,
-                query: query,
+                query_start: query,
-                fragment: fragment,
+                fragment_start: fragment,
             })
         } else if s.contains("://") {
             // absolute-form
             let scheme = parse_scheme(&s);
-            let auth = parse_authority(&s);
+            let auth = Some(parse_authority(&s));
             let scheme_end = scheme.expect("just checked for ':' above");
             let auth_end = auth.expect("just checked for ://");
             if scheme_end + 3 == auth_end {
@@ -80,8 +75,8 @@ impl Uri {
                 source: s,
                 scheme_end: scheme,
                 authority_end: auth,
-                query: query,
+                query_start: query,
-                fragment: fragment,
+                fragment_start: fragment,
             })
         } else if (s.contains("/") || s.contains("?")) && !s.contains("://") {
             // last possibility is authority-form, above are illegal characters
@@ -93,19 +88,16 @@ impl Uri {
                 source: s,
                 scheme_end: None,
                 authority_end: Some(len),
-                query: None,
+                query_start: None,
-                fragment: None,
+                fragment_start: None,
             })
         }
     }
 
     /// Get the path of this `Uri`.
     pub fn path(&self) -> &str {
-        let index = self.authority_end.unwrap_or(self.scheme_end.unwrap_or(0));
+        let index = self.path_start();
-        let query_len = self.query.unwrap_or(0);
+        let end = self.path_end();
-        let fragment_len = self.fragment.unwrap_or(0);
-        let end = self.source.len() - if query_len > 0 { query_len + 1 } else { 0 } -
-            if fragment_len > 0 { fragment_len + 1 } else { 0 };
         if index >= end {
             if self.scheme().is_some() {
                 "/" // absolute-form MUST have path
@@ -117,6 +109,31 @@ impl Uri {
         }
     }
 
+    #[inline]
+    fn path_start(&self) -> usize {
+        self.authority_end.unwrap_or(self.scheme_end.unwrap_or(0))
+    }
+
+    #[inline]
+    fn path_end(&self) -> usize {
+        if let Some(query) = self.query_start {
+            query
+        } else if let Some(fragment) = self.fragment_start {
+            fragment
+        } else {
+            self.source.len()
+        }
+    }
+
+    #[inline]
+    fn origin_form_end(&self) -> usize {
+        if let Some(fragment) = self.fragment_start {
+            fragment
+        } else {
+            self.source.len()
+        }
+    }
+
     /// Get the scheme of this `Uri`.
     pub fn scheme(&self) -> Option<&str> {
         if let Some(end) = self.scheme_end {
@@ -156,23 +173,23 @@ impl Uri {
 
     /// Get the query string of this `Uri`, starting after the `?`.
     pub fn query(&self) -> Option<&str> {
-        let fragment_len = self.fragment.unwrap_or(0);
+        self.query_start.map(|start| {
-        let fragment_len = if fragment_len > 0 { fragment_len + 1 } else { 0 };
+            // +1 to remove '?'
-        if let Some(len) = self.query {
+            let start = start + 1;
-            Some(&self.source[self.source.len() - len - fragment_len..
+            if let Some(end) = self.fragment_start {
-                                       self.source.len() - fragment_len])
+                &self.source[start..end]
-        } else {
+            } else {
-            None
+                &self.source[start..]
-        }
+            }
+        })
     }
 
     #[cfg(test)]
     fn fragment(&self) -> Option<&str> {
-        if let Some(len) = self.fragment {
+        self.fragment_start.map(|start| {
-            Some(&self.source[self.source.len() - len..self.source.len()])
+            // +1 to remove the '#'
-        } else {
+           &self.source[start + 1..]
-            None
+        })
-        }
     }
 }
 
@@ -180,35 +197,31 @@ fn parse_scheme(s: &str) -> Option<usize> {
     s.find(':')
 }
 
-fn parse_authority(s: &str) -> Option<usize> {
+fn parse_authority(s: &str) -> usize {
-    let i = s.find("://").and_then(|p| Some(p + 3)).unwrap_or(0);
+    let i = s.find("://").map(|p| p + 3).unwrap_or(0);
-
+    s[i..].find('/')
-    Some(&s[i..].split("/")
+        .or_else(|| s[i..].find('?'))
-         .next()
+        .or_else(|| s[i..].find('#'))
-         .unwrap_or(s)
+        .map(|end| end + i)
-         .len() + i)
+        .unwrap_or(s.len())
 }
 
 fn parse_query(s: &str) -> Option<usize> {
-    match s.find('?') {
+    s.find('?').and_then(|i| {
-        Some(i) => {
+        if let Some(frag) = s.find('#') {
-            let frag_pos = s.find('#').unwrap_or(s.len());
+            if frag < i {
-
-            if frag_pos < i + 1 {
                 None
             } else {
-                Some(frag_pos - i - 1)
+                Some(i)
             }
-        },
+        } else {
-        None => None,
+            Some(i)
-    }
+        }
+    })
 }
 
 fn parse_fragment(s: &str) -> Option<usize> {
-    match s.find('#') {
+    s.find('#')
-        Some(i) => Some(s.len() - i - 1),
-        None => None,
-    }
 }
 
 impl FromStr for Uri {
@@ -227,6 +240,18 @@ impl PartialEq for Uri {
     }
 }
 
+impl<'a> PartialEq<&'a str> for Uri {
+    fn eq(&self, other: & &'a str) -> bool {
+        self.source.as_str() == *other
+    }
+}
+
+impl<'a> PartialEq<Uri> for &'a str{
+    fn eq(&self, other: &Uri) -> bool {
+        *self == other.source.as_str()
+    }
+}
+
 impl Eq for Uri {}
 
 impl AsRef<str> for Uri {
@@ -241,8 +266,8 @@ impl Default for Uri {
             source: ByteStr::from_static("/"),
             scheme_end: None,
             authority_end: None,
-            query: None,
+            query_start: None,
-            fragment: None,
+            fragment_start: None,
         }
     }
 }
@@ -269,27 +294,58 @@ pub fn scheme_and_authority(uri: &Uri) -> Option<Uri> {
             source: uri.source.slice_to(uri.authority_end.expect("scheme without authority")),
             scheme_end: uri.scheme_end,
             authority_end: uri.authority_end,
-            query: None,
+            query_start: None,
-            fragment: None,
+            fragment_start: None,
         })
     } else {
         None
     }
 }
 
-pub fn origin_form(uri: &Uri) -> Uri {
+#[inline]
-    let start = uri.authority_end.unwrap_or(uri.scheme_end.unwrap_or(0));
+fn asterisk_form() -> Uri {
-    let end = if let Some(f) = uri.fragment {
-        uri.source.len() - f - 1
-    } else {
-        uri.source.len()
-    };
     Uri {
-        source: uri.source.slice(start, end),
+        source: ByteStr::from_static("*"),
         scheme_end: None,
         authority_end: None,
-        query: uri.query,
+        query_start: None,
-        fragment: None,
+        fragment_start: None,
+    }
+}
+
+pub fn origin_form(uri: &Uri) -> Uri {
+    let range = Range(uri.path_start(), uri.origin_form_end());
+
+    let clone = if range.len() == 0 {
+        ByteStr::from_static("/")
+    } else if uri.source.as_bytes()[range.0] == b'*' {
+        return asterisk_form();
+    } else if uri.source.as_bytes()[range.0] != b'/' {
+        let mut new = BytesMut::with_capacity(range.1 - range.0 + 1);
+        new.put_u8(b'/');
+        new.put_slice(&uri.source.as_bytes()[range.0..range.1]);
+        // safety: the bytes are '/' + previous utf8 str
+        unsafe { ByteStr::from_utf8_unchecked(new.freeze()) }
+    } else if range.0 == 0 && range.1 == uri.source.len() {
+        uri.source.clone()
+    } else {
+        uri.source.slice(range.0, range.1)
+    };
+
+    Uri {
+        source: clone,
+        scheme_end: None,
+        authority_end: None,
+        query_start: uri.query_start,
+        fragment_start: None,
+    }
+}
+
+struct Range(usize, usize);
+
+impl Range {
+    fn len(&self) -> usize {
+        self.1 - self.0
     }
 }
 
@@ -329,6 +385,7 @@ macro_rules! test_parse {
         #[test]
         fn $test_name() {
             let uri = Uri::from_str($str).unwrap();
+            println!("{:?} = {:#?}", $str, uri);
             $(
             assert_eq!(uri.$method(), $value);
             )+
@@ -442,6 +499,30 @@ test_parse! {
     port = None,
 }
 
+test_parse! {
+    test_uri_parse_path_with_terminating_questionmark,
+    "http://127.0.0.1/path?",
+
+    scheme = Some("http"),
+    authority = Some("127.0.0.1"),
+    path = "/path",
+    query = Some(""),
+    fragment = None,
+    port = None,
+}
+
+test_parse! {
+    test_uri_parse_absolute_form_with_empty_path_and_nonempty_query,
+    "http://127.0.0.1?foo=bar",
+
+    scheme = Some("http"),
+    authority = Some("127.0.0.1"),
+    path = "/",
+    query = Some("foo=bar"),
+    fragment = None,
+    port = None,
+}
+
 #[test]
 fn test_uri_parse_error() {
     fn err(s: &str) {
@@ -452,6 +533,26 @@ fn test_uri_parse_error() {
     err("htt:p//host");
     err("hyper.rs/");
     err("hyper.rs?key=val");
+    err("?key=val");
     err("localhost/");
     err("localhost?key=val");
 }
+
+#[test]
+fn test_uri_to_origin_form() {
+    let cases = vec![
+        ("/", "/"),
+        ("/foo?bar", "/foo?bar"),
+        ("/foo?bar#nope", "/foo?bar"),
+        ("http://hyper.rs", "/"),
+        ("http://hyper.rs/", "/"),
+        ("http://hyper.rs/path", "/path"),
+        ("http://hyper.rs?query", "/?query"),
+        ("*", "*"),
+    ];
+
+    for case in cases {
+        let uri = Uri::from_str(case.0).unwrap();
+        assert_eq!(origin_form(&uri), case.1, "{:?}", case);
+    }
+}