refactor(headers): errors for parse_header

Header::parse_header() returns now a hyper Result instead of an option
this will enable more precise Error messages in the future, currently
most failures are reported as ::Error::Header.

BREAKING CHANGE: parse_header returns Result instead of Option, related
code did also change

Author: pyfisch

benches/client.rs

@@ -58,8 +58,8 @@ impl hyper::header::Header for Foo {
     fn header_name() -> &'static str {
         "x-foo"
     }
-    fn parse_header(_: &[Vec<u8>]) -> Option<Foo> {
-        None
+    fn parse_header(_: &[Vec<u8>]) -> hyper::Result<Foo> {
+        Err(hyper::Error::Header)
     }
 }
 
@@ -104,4 +104,3 @@ fn bench_mock_hyper(b: &mut test::Bencher) {
             .read_to_string(&mut s).unwrap()
     });
 }
-

src/error.rs

@@ -2,6 +2,7 @@
 use std::error::Error as StdError;
 use std::fmt;
 use std::io::Error as IoError;
+use std::str::Utf8Error;
 
 use httparse;
 use openssl::ssl::error::SslError;
@@ -18,6 +19,7 @@ use self::Error::{
     Ssl,
     TooLarge,
     Http2,
+    Utf8
 };
 
 
@@ -45,6 +47,8 @@ pub enum Error {
     Ssl(SslError),
     /// An HTTP/2-specific error, coming from the `solicit` library.
     Http2(Http2Error),
+    /// Parsing a field as string failed
+    Utf8(Utf8Error),
 
     #[doc(hidden)]
     __Nonexhaustive(Void)
@@ -77,6 +81,7 @@ impl StdError for Error {
             Io(ref e) => e.description(),
             Ssl(ref e) => e.description(),
             Http2(ref e) => e.description(),
+            Utf8(ref e) => e.description(),
             Error::__Nonexhaustive(ref void) =>  match *void {}
         }
     }
@@ -113,6 +118,12 @@ impl From<SslError> for Error {
     }
 }
 
+impl From<Utf8Error> for Error {
+    fn from(err: Utf8Error) -> Error {
+        Utf8(err)
+    }
+}
+
 impl From<httparse::Error> for Error {
     fn from(err: httparse::Error) -> Error {
         match err {

src/header/common/accept_ranges.rs

@@ -52,8 +52,8 @@ pub enum RangeUnit {
 
 
 impl FromStr for RangeUnit {
-    type Err = ();
-    fn from_str(s: &str) -> Result<Self, ()> {
+    type Err = ::Error;
+    fn from_str(s: &str) -> ::Result<Self> {
         match s {
             "bytes" => Ok(RangeUnit::Bytes),
             "none" => Ok(RangeUnit::None),

src/header/common/access_control_allow_origin.rs

@@ -35,16 +35,14 @@ impl Header for AccessControlAllowOrigin {
         "Access-Control-Allow-Origin"
     }
 
-    fn parse_header(raw: &[Vec<u8>]) -> Option<AccessControlAllowOrigin> {
+    fn parse_header(raw: &[Vec<u8>]) -> ::Result<AccessControlAllowOrigin> {
         if raw.len() == 1 {
             match unsafe { &raw.get_unchecked(0)[..] } {
-                b"*" => Some(AccessControlAllowOrigin::Any),
-                b"null" => Some(AccessControlAllowOrigin::Null),
-                r => if let Ok(s) = str::from_utf8(r) {
-                    Url::parse(s).ok().map(AccessControlAllowOrigin::Value)
-                } else { None }
+                b"*" => Ok(AccessControlAllowOrigin::Any),
+                b"null" => Ok(AccessControlAllowOrigin::Null),
+                r => Ok(AccessControlAllowOrigin::Value(try!(Url::parse(try!(str::from_utf8(r))))))
             }
-        } else { None }
+        } else { Err(::Error::Header) }
     }
 }
 

src/header/common/authorization.rs

@@ -42,18 +42,25 @@ impl<S: Scheme + Any> Header for Authorization<S> where <S as FromStr>::Err: 'st
         "Authorization"
     }
 
-    fn parse_header(raw: &[Vec<u8>]) -> Option<Authorization<S>> {
-        if raw.len() == 1 {
-            match (from_utf8(unsafe { &raw.get_unchecked(0)[..] }), <S as Scheme>::scheme()) {
-                (Ok(header), Some(scheme))
-                    if header.starts_with(scheme) && header.len() > scheme.len() + 1 => {
-                    header[scheme.len() + 1..].parse::<S>().map(Authorization).ok()
-                },
-                (Ok(header), None) => header.parse::<S>().map(Authorization).ok(),
-                _ => None
+    fn parse_header(raw: &[Vec<u8>]) -> ::Result<Authorization<S>> {
+        if raw.len() != 1 {
+            return Err(::Error::Header);
+        }
+        let header = try!(from_utf8(unsafe { &raw.get_unchecked(0)[..] }));
+        return if let Some(scheme) = <S as Scheme>::scheme() {
+            if header.starts_with(scheme) && header.len() > scheme.len() + 1 {
+                match header[scheme.len() + 1..].parse::<S>().map(Authorization) {
+                    Ok(h) => Ok(h),
+                    Err(_) => Err(::Error::Header)
+                }
+            } else {
+                Err(::Error::Header)
             }
         } else {
-            None
+            match header.parse::<S>().map(Authorization) {
+                Ok(h) => Ok(h),
+                Err(_) => Err(::Error::Header)
+            }
         }
     }
 }
@@ -121,15 +128,15 @@ impl Scheme for Basic {
 }
 
 impl FromStr for Basic {
-    type Err = ();
-    fn from_str(s: &str) -> Result<Basic, ()> {
+    type Err = ::Error;
+    fn from_str(s: &str) -> ::Result<Basic> {
         match s.from_base64() {
             Ok(decoded) => match String::from_utf8(decoded) {
                 Ok(text) => {
                     let mut parts = &mut text.split(':');
                     let user = match parts.next() {
                         Some(part) => part.to_owned(),
-                        None => return Err(())
+                        None => return Err(::Error::Header)
                     };
                     let password = match parts.next() {
                         Some(part) => Some(part.to_owned()),
@@ -142,12 +149,12 @@ impl FromStr for Basic {
                 },
                 Err(e) => {
                     debug!("Basic::from_utf8 error={:?}", e);
-                    Err(())
+                    Err(::Error::Header)
                 }
             },
             Err(e) => {
                 debug!("Basic::from_base64 error={:?}", e);
-                Err(())
+                Err(::Error::Header)
             }
         }
     }

src/header/common/cache_control.rs

@@ -30,15 +30,15 @@ impl Header for CacheControl {
         "Cache-Control"
     }
 
-    fn parse_header(raw: &[Vec<u8>]) -> Option<CacheControl> {
+    fn parse_header(raw: &[Vec<u8>]) -> ::Result<CacheControl> {
         let directives = raw.iter()
-            .filter_map(|line| from_one_comma_delimited(&line[..]))
+            .filter_map(|line| from_one_comma_delimited(&line[..]).ok())
             .collect::<Vec<Vec<CacheDirective>>>()
             .concat();
         if !directives.is_empty() {
-            Some(CacheControl(directives))
+            Ok(CacheControl(directives))
         } else {
-            None
+            Err(::Error::Header)
         }
     }
 }
@@ -148,35 +148,35 @@ mod tests {
     #[test]
     fn test_parse_multiple_headers() {
         let cache = Header::parse_header(&[b"no-cache".to_vec(), b"private".to_vec()]);
-        assert_eq!(cache, Some(CacheControl(vec![CacheDirective::NoCache,
+        assert_eq!(cache.ok(), Some(CacheControl(vec![CacheDirective::NoCache,
                                                  CacheDirective::Private])))
     }
 
     #[test]
     fn test_parse_argument() {
         let cache = Header::parse_header(&[b"max-age=100, private".to_vec()]);
-        assert_eq!(cache, Some(CacheControl(vec![CacheDirective::MaxAge(100),
+        assert_eq!(cache.ok(), Some(CacheControl(vec![CacheDirective::MaxAge(100),
                                                  CacheDirective::Private])))
     }
 
     #[test]
     fn test_parse_quote_form() {
         let cache = Header::parse_header(&[b"max-age=\"200\"".to_vec()]);
-        assert_eq!(cache, Some(CacheControl(vec![CacheDirective::MaxAge(200)])))
+        assert_eq!(cache.ok(), Some(CacheControl(vec![CacheDirective::MaxAge(200)])))
     }
 
     #[test]
     fn test_parse_extension() {
         let cache = Header::parse_header(&[b"foo, bar=baz".to_vec()]);
-        assert_eq!(cache, Some(CacheControl(vec![
+        assert_eq!(cache.ok(), Some(CacheControl(vec![
             CacheDirective::Extension("foo".to_owned(), None),
             CacheDirective::Extension("bar".to_owned(), Some("baz".to_owned()))])))
     }
 
     #[test]
     fn test_parse_bad_syntax() {
-        let cache: Option<CacheControl> = Header::parse_header(&[b"foo=".to_vec()]);
-        assert_eq!(cache, None)
+        let cache: ::Result<CacheControl> = Header::parse_header(&[b"foo=".to_vec()]);
+        assert_eq!(cache.ok(), None)
     }
 }
 

src/header/common/cookie.rs

@@ -27,26 +27,23 @@ impl Header for Cookie {
         "Cookie"
     }
 
-    fn parse_header(raw: &[Vec<u8>]) -> Option<Cookie> {
+    fn parse_header(raw: &[Vec<u8>]) -> ::Result<Cookie> {
         let mut cookies = Vec::with_capacity(raw.len());
         for cookies_raw in raw.iter() {
-            match from_utf8(&cookies_raw[..]) {
-                Ok(cookies_str) => {
-                    for cookie_str in cookies_str.split(';') {
-                        match cookie_str.trim().parse() {
-                            Ok(cookie) => cookies.push(cookie),
-                            Err(_) => return None
-                        }
-                    }
-                },
-                Err(_) => return None
-            };
+            let cookies_str = try!(from_utf8(&cookies_raw[..]));
+            for cookie_str in cookies_str.split(';') {
+                if let Ok(cookie) = cookie_str.trim().parse() {
+                    cookies.push(cookie);
+                } else {
+                    return Err(::Error::Header);
+                }
+            }
         }
 
         if !cookies.is_empty() {
-            Some(Cookie(cookies))
+            Ok(Cookie(cookies))
         } else {
-            None
+            Err(::Error::Header)
         }
     }
 }
@@ -88,7 +85,7 @@ fn test_parse() {
     let h = Header::parse_header(&[b"foo=bar; baz=quux".to_vec()][..]);
     let c1 = CookiePair::new("foo".to_owned(), "bar".to_owned());
     let c2 = CookiePair::new("baz".to_owned(), "quux".to_owned());
-    assert_eq!(h, Some(Cookie(vec![c1, c2])));
+    assert_eq!(h.ok(), Some(Cookie(vec![c1, c2])));
 }
 
 #[test]

src/header/common/expect.rs

@@ -26,7 +26,7 @@ impl Header for Expect {
         "Expect"
     }
 
-    fn parse_header(raw: &[Vec<u8>]) -> Option<Expect> {
+    fn parse_header(raw: &[Vec<u8>]) -> ::Result<Expect> {
         if raw.len() == 1 {
             let text = unsafe {
                 // safe because:
@@ -38,12 +38,12 @@ impl Header for Expect {
                 str::from_utf8_unchecked(raw.get_unchecked(0))
             };
             if UniCase(text) == EXPECT_CONTINUE {
-                Some(Expect::Continue)
+                Ok(Expect::Continue)
             } else {
-                None
+                Err(::Error::Header)
             }
         } else {
-            None
+            Err(::Error::Header)
         }
     }
 }

src/header/common/host.rs

@@ -22,7 +22,7 @@ impl Header for Host {
         "Host"
     }
 
-    fn parse_header(raw: &[Vec<u8>]) -> Option<Host> {
+    fn parse_header(raw: &[Vec<u8>]) -> ::Result<Host> {
         from_one_raw_str(raw).and_then(|mut s: String| {
             // FIXME: use rust-url to parse this
             // https://github.com/servo/rust-url/issues/42
@@ -39,7 +39,7 @@ impl Header for Host {
                                 None
                             }
                         }
-                        None => return None // this is a bad ipv6 address...
+                        None => return Err(::Error::Header) // this is a bad ipv6 address...
                     }
                 } else {
                     slice.rfind(':')
@@ -56,7 +56,7 @@ impl Header for Host {
                 None => ()
             }
 
-            Some(Host {
+            Ok(Host {
                 hostname: s,
                 port: port
             })
@@ -82,14 +82,14 @@ mod tests {
     #[test]
     fn test_host() {
         let host = Header::parse_header([b"foo.com".to_vec()].as_ref());
-        assert_eq!(host, Some(Host {
+        assert_eq!(host.ok(), Some(Host {
             hostname: "foo.com".to_owned(),
             port: None
         }));
 
 
         let host = Header::parse_header([b"foo.com:8080".to_vec()].as_ref());
-        assert_eq!(host, Some(Host {
+        assert_eq!(host.ok(), Some(Host {
             hostname: "foo.com".to_owned(),
             port: Some(8080)
         }));

src/header/common/if_none_match.rs

@@ -45,18 +45,18 @@ mod tests {
 
     #[test]
     fn test_if_none_match() {
-        let mut if_none_match: Option<IfNoneMatch>;
+        let mut if_none_match: ::Result<IfNoneMatch>;
 
         if_none_match = Header::parse_header([b"*".to_vec()].as_ref());
-        assert_eq!(if_none_match, Some(IfNoneMatch::Any));
+        assert_eq!(if_none_match.ok(), Some(IfNoneMatch::Any));
 
         if_none_match = Header::parse_header([b"\"foobar\", W/\"weak-etag\"".to_vec()].as_ref());
         let mut entities: Vec<EntityTag> = Vec::new();
         let foobar_etag = EntityTag::new(false, "foobar".to_owned());
         let weak_etag = EntityTag::new(true, "weak-etag".to_owned());
         entities.push(foobar_etag);
         entities.push(weak_etag);
-        assert_eq!(if_none_match, Some(IfNoneMatch::Items(entities)));
+        assert_eq!(if_none_match.ok(), Some(IfNoneMatch::Items(entities)));
     }
 }