Merge pull request #88 from seanmonstar/error-reform

redirect and error reform

src/client.rs

@@ -16,7 +16,7 @@ use serde_json;
 use serde_urlencoded;
 
 use ::body::{self, Body};
-use ::redirect::{RedirectPolicy, check_redirect};
+use ::redirect::{self, RedirectPolicy, check_redirect};
 use ::response::Response;
 
 static DEFAULT_USER_AGENT: &'static str = concat!(env!("CARGO_PKG_NAME"), "/", env!("CARGO_PKG_VERSION"));
@@ -36,7 +36,7 @@ pub struct Client {
 impl Client {
     /// Constructs a new `Client`.
     pub fn new() -> ::Result<Client> {
-        let mut client = try!(new_hyper_client());
+        let mut client = try_!(new_hyper_client());
         client.set_redirect_policy(::hyper::client::RedirectPolicy::FollowNone);
         Ok(Client {
             inner: Arc::new(ClientRef {
@@ -133,7 +133,7 @@ fn new_hyper_client() -> ::Result<::hyper::Client> {
         ::hyper::client::Pool::with_connector(
             Default::default(),
             ::hyper::net::HttpsConnector::new(
-                try!(NativeTlsClient::new()
+                try_!(NativeTlsClient::new()
                      .map_err(|e| ::hyper::Error::Ssl(Box::new(e)))))
         )
     ))
@@ -206,7 +206,7 @@ impl RequestBuilder {
     ///     .send();
     /// ```
     pub fn form<T: Serialize>(mut self, form: &T) -> RequestBuilder {
-        let body = serde_urlencoded::to_string(form).map_err(::Error::from);
+        let body = serde_urlencoded::to_string(form).map_err(::error::from);
         self.headers.set(ContentType::form_url_encoded());
         self.body = Some(body.map(|b| b.into()));
         self
@@ -250,10 +250,10 @@ impl RequestBuilder {
         }
         let client = self.client;
         let mut method = self.method;
-        let mut url = try!(self.url);
+        let mut url = try_!(self.url);
         let mut headers = self.headers;
         let mut body = match self.body {
-            Some(b) => Some(try!(b)),
+            Some(b) => Some(try_!(b)),
             None => None,
         };
 
@@ -271,7 +271,7 @@ impl RequestBuilder {
                     req = req.body(body);
                 }
 
-                try!(req.send())
+                try_!(req.send(), &url)
             };
 
             let should_redirect = match res.status {
@@ -312,12 +312,20 @@ impl RequestBuilder {
                     Ok(loc) => {
                         headers.set(Referer(url.to_string()));
                         urls.push(url);
-                        if check_redirect(&client.redirect_policy.lock().unwrap(), &loc, &urls)? {
+                        let action = check_redirect(&client.redirect_policy.lock().unwrap(), &loc, &urls);
-                            loc
+                        match action {
-                        } else {
+                            redirect::Action::Follow => loc,
-                            debug!("redirect_policy disallowed redirection to '{}'", loc);
+                            redirect::Action::Stop => {
+                                debug!("redirect_policy disallowed redirection to '{}'", loc);
 
-                            return Ok(::response::new(res, client.auto_ungzip.load(Ordering::Relaxed)));
+                                return Ok(::response::new(res, client.auto_ungzip.load(Ordering::Relaxed)));
+                            },
+                            redirect::Action::LoopDetected => {
+                                return Err(::error::loop_detected(res.url.clone()));
+                            },
+                            redirect::Action::TooManyRedirects => {
+                                return Err(::error::too_many_redirects(res.url.clone()));
+                            }
                         }
                     },
                     Err(e) => {

src/error.rs

@@ -1,87 +1,191 @@
 use std::error::Error as StdError;
 use std::fmt;
 
+use ::Url;
+
 /// The Errors that may occur when processing a `Request`.
 #[derive(Debug)]
-pub enum Error {
+pub struct Error {
-    /// An HTTP error from the `hyper` crate.
+    kind: Kind,
-    Http(::hyper::Error),
+    url: Option<Url>,
-    /// An error trying to serialize a value.
+}
-    ///
+
-    /// This may be serializing a value that is illegal in JSON or
+/// A `Result` alias where the `Err` case is `reqwest::Error`.
-    /// form-url-encoded bodies.
+pub type Result<T> = ::std::result::Result<T, Error>;
-    Serialize(Box<StdError + Send + Sync>),
+
-    /// A request tried to redirect too many times.
+impl Error {
-    TooManyRedirects,
+    /// Returns a possible URL related to this error.
-    /// An infinite redirect loop was detected.
+    #[inline]
-    RedirectLoop,
+    pub fn url(&self) -> Option<&Url> {
-    #[doc(hidden)]
+        self.url.as_ref()
-    __DontMatchMe,
+    }
+
+    /// Returns true if the error is related to HTTP.
+    #[inline]
+    pub fn is_http(&self) -> bool {
+        match self.kind {
+            Kind::Http(_) => true,
+            _ => false,
+        }
+    }
+
+    /// Returns true if the error is serialization related.
+    #[inline]
+    pub fn is_serialization(&self) -> bool {
+        match self.kind {
+            Kind::Json(_) |
+            Kind::UrlEncoded(_) => true,
+            _ => false,
+        }
+    }
+
+    /// Returns true if the error is from a `RedirectPolicy`.
+    #[inline]
+    pub fn is_redirect(&self) -> bool {
+        match self.kind {
+            Kind::TooManyRedirects |
+            Kind::RedirectLoop => true,
+            _ => false,
+        }
+    }
 }
 
 impl fmt::Display for Error {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        match *self {
+        if let Some(ref url) = self.url {
-            Error::Http(ref e) => fmt::Display::fmt(e, f),
+            try!(fmt::Display::fmt(url, f));
-            Error::Serialize(ref e) => fmt::Display::fmt(e, f),
+            try!(f.write_str(": "));
-            Error::TooManyRedirects => f.pad("Too many redirects"),
+        }
-            Error::RedirectLoop => f.pad("Infinite redirect loop"),
+        match self.kind {
-            Error::__DontMatchMe => unreachable!()
+            Kind::Http(ref e) => fmt::Display::fmt(e, f),
+            Kind::UrlEncoded(ref e) => fmt::Display::fmt(e, f),
+            Kind::Json(ref e) => fmt::Display::fmt(e, f),
+            Kind::TooManyRedirects => f.write_str("Too many redirects"),
+            Kind::RedirectLoop => f.write_str("Infinite redirect loop"),
         }
     }
 }
 
 impl StdError for Error {
     fn description(&self) -> &str {
-        match *self {
+        match self.kind {
-            Error::Http(ref e) => e.description(),
+            Kind::Http(ref e) => e.description(),
-            Error::Serialize(ref e) => e.description(),
+            Kind::UrlEncoded(ref e) => e.description(),
-            Error::TooManyRedirects => "Too many redirects",
+            Kind::Json(ref e) => e.description(),
-            Error::RedirectLoop => "Infinite redirect loop",
+            Kind::TooManyRedirects => "Too many redirects",
-            Error::__DontMatchMe => unreachable!()
+            Kind::RedirectLoop => "Infinite redirect loop",
         }
     }
 
     fn cause(&self) -> Option<&StdError> {
-        match *self {
+        match self.kind {
-            Error::Http(ref e) => Some(e),
+            Kind::Http(ref e) => Some(e),
-            Error::Serialize(ref e) => Some(&**e),
+            Kind::UrlEncoded(ref e) => Some(e),
-            Error::TooManyRedirects |
+            Kind::Json(ref e) => Some(e),
-            Error::RedirectLoop => None,
+            Kind::TooManyRedirects |
-            Error::__DontMatchMe => unreachable!()
+            Kind::RedirectLoop => None,
         }
     }
 }
 
-fn _assert_types() {
+// pub(crate)
-    fn _assert_send<T: Send>() {
+
-    }
+#[derive(Debug)]
-    _assert_send::<Error>();
+pub enum Kind {
+    Http(::hyper::Error),
+    UrlEncoded(::serde_urlencoded::ser::Error),
+    Json(::serde_json::Error),
+    TooManyRedirects,
+    RedirectLoop,
 }
 
-impl From<::hyper::Error> for Error {
+
-    fn from(err: ::hyper::Error) -> Error {
+impl From<::hyper::Error> for Kind {
-        Error::Http(err)
+    #[inline]
+    fn from(err: ::hyper::Error) -> Kind {
+        Kind::Http(err)
     }
 }
 
-impl From<::url::ParseError> for Error {
+impl From<::url::ParseError> for Kind {
-    fn from(err: ::url::ParseError) -> Error {
+    #[inline]
-        Error::Http(::hyper::Error::Uri(err))
+    fn from(err: ::url::ParseError) -> Kind {
+        Kind::Http(::hyper::Error::Uri(err))
     }
 }
 
-impl From<::serde_urlencoded::ser::Error> for Error {
+impl From<::serde_urlencoded::ser::Error> for Kind {
-    fn from(err: ::serde_urlencoded::ser::Error) -> Error {
+    #[inline]
-        Error::Serialize(Box::new(err))
+    fn from(err: ::serde_urlencoded::ser::Error) -> Kind {
+        Kind::UrlEncoded(err)
     }
 }
 
-impl From<::serde_json::Error> for Error {
+impl From<::serde_json::Error> for Kind {
-    fn from(err: ::serde_json::Error) -> Error {
+    #[inline]
-        Error::Serialize(Box::new(err))
+    fn from(err: ::serde_json::Error) -> Kind {
+        Kind::Json(err)
     }
 }
 
-/// A `Result` alias where the `Err` case is `reqwest::Error`.
+pub struct InternalFrom<T>(pub T, pub Option<Url>);
-pub type Result<T> = ::std::result::Result<T, Error>;
+
+impl From<InternalFrom<Error>> for Error {
+    #[inline]
+    fn from(other: InternalFrom<Error>) -> Error {
+        other.0
+    }
+}
+
+impl<T> From<InternalFrom<T>> for Error
+where T: Into<Kind> {
+    #[inline]
+    fn from(other: InternalFrom<T>) -> Error {
+         Error {
+            kind: other.0.into(),
+            url: other.1,
+        }
+    }
+}
+
+#[inline]
+pub fn from<T>(err: T) -> Error
+where T: Into<Kind> {
+    InternalFrom(err, None).into()
+}
+
+#[inline]
+pub fn loop_detected(url: Url) -> Error {
+    Error {
+        kind: Kind::RedirectLoop,
+        url: Some(url),
+    }
+}
+
+#[inline]
+pub fn too_many_redirects(url: Url) -> Error {
+    Error {
+        kind: Kind::TooManyRedirects,
+        url: Some(url),
+    }
+}
+
+#[macro_export]
+macro_rules! try_ {
+    ($e:expr) => (
+        match $e {
+            Ok(v) => v,
+            Err(err) => {
+                return Err(::Error::from(::error::InternalFrom(err, None)));
+            }
+        }
+    );
+    ($e:expr, $url:expr) => (
+        match $e {
+            Ok(v) => v,
+            Err(err) => {
+                return Err(::Error::from(::error::InternalFrom(err, Some($url.clone()))));
+            }
+        }
+    )
+}

src/lib.rs

@@ -124,9 +124,9 @@ pub use self::body::Body;
 pub use self::redirect::RedirectPolicy;
 pub use self::response::Response;
 
+#[macro_use] mod error;
 mod body;
 mod client;
-mod error;
 mod redirect;
 mod response;
 
@@ -161,4 +161,8 @@ fn _assert_impls() {
 
     assert_send::<RequestBuilder>();
     assert_send::<Response>();
+
+
+    assert_send::<Error>();
+    assert_sync::<Error>();
 }

src/redirect.rs

@@ -11,10 +11,22 @@ pub struct RedirectPolicy {
     inner: Policy,
 }
 
+#[derive(Debug)]
+pub struct RedirectAttempt<'a> {
+    next: &'a Url,
+    previous: &'a [Url],
+}
+
+/// An action to perform when a redirect status code is found.
+#[derive(Debug)]
+pub struct RedirectAction {
+    inner: Action,
+}
+
 impl RedirectPolicy {
     /// Create a RedirectPolicy with a maximum number of redirects.
     ///
-    /// A `Error::TooManyRedirects` will be returned if the max is reached.
+    /// An `Error` will be returned if the max is reached.
     pub fn limited(max: usize) -> RedirectPolicy {
         RedirectPolicy {
             inner: Policy::Limit(max),
@@ -36,45 +48,48 @@ impl RedirectPolicy {
     /// chain, but the custom variant does not do that for you automatically.
     /// The custom policy should have some way of handling those.
     ///
-    /// There are variants on `::Error` for both cases that can be used as
+    /// Information on the next request and previous requests can be found
-    /// return values.
+    /// on the `RedirectAttempt` argument passed to the closure.
+    ///
+    /// Actions can be conveniently created from methods on the
+    /// `RedirectAttempt`.
     ///
     /// # Example
     ///
     /// ```no_run
     /// # use reqwest::RedirectPolicy;
     /// # let mut client = reqwest::Client::new().unwrap();
-    /// client.redirect(RedirectPolicy::custom(|next, previous| {
+    /// client.redirect(RedirectPolicy::custom(|attempt| {
-    ///     if previous.len() > 5 {
+    ///     if attempt.previous().len() > 5 {
-    ///         Err(reqwest::Error::TooManyRedirects)
+    ///         attempt.too_many_redirects()
-    ///     } else if next.host_str() == Some("example.domain") {
+    ///     } else if attempt.url().host_str() == Some("example.domain") {
     ///         // prevent redirects to 'example.domain'
-    ///         Ok(false)
+    ///         attempt.stop()
     ///     } else {
-    ///         Ok(true)
+    ///         attempt.follow()
     ///     }
     /// }));
     /// ```
     pub fn custom<T>(policy: T) -> RedirectPolicy
-    where T: Fn(&Url, &[Url]) -> ::Result<bool> + Send + Sync + 'static {
+    where T: Fn(RedirectAttempt) -> RedirectAction + Send + Sync + 'static {
         RedirectPolicy {
             inner: Policy::Custom(Box::new(policy)),
         }
     }
 
-    fn redirect(&self, next: &Url, previous: &[Url]) -> ::Result<bool> {
+    fn redirect(&self, attempt: RedirectAttempt) -> RedirectAction {
         match self.inner {
-            Policy::Custom(ref custom) => custom(next, previous),
+            Policy::Custom(ref custom) => custom(attempt),
             Policy::Limit(max) => {
-                if previous.len() == max {
+                if attempt.previous.len() == max {
-                    Err(::Error::TooManyRedirects)
+                    attempt.too_many_redirects()
-                } else if previous.contains(next) {
+                } else if attempt.previous.contains(attempt.next) {
-                    Err(::Error::RedirectLoop)
+                    attempt.loop_detected()
                 } else {
-                    Ok(true)
+                    attempt.follow()
                 }
             },
-            Policy::None => Ok(false),
+            Policy::None => attempt.stop(),
         }
     }
 }
@@ -85,8 +100,53 @@ impl Default for RedirectPolicy {
     }
 }
 
+impl<'a> RedirectAttempt<'a> {
+    /// Get the next URL to redirect to.
+    pub fn url(&self) -> &Url {
+        self.next
+    }
+
+    /// Get the list of previous URLs that have already been requested in this chain.
+    pub fn previous(&self) -> &[Url] {
+        self.previous
+    }
+    /// Returns an action meaning reqwest should follow the next URL.
+    pub fn follow(self) -> RedirectAction {
+        RedirectAction {
+            inner: Action::Follow,
+        }
+    }
+
+    /// Returns an action meaning reqwest should not follow the next URL.
+    ///
+    /// The 30x response will be returned as the `Ok` result.
+    pub fn stop(self) -> RedirectAction {
+        RedirectAction {
+            inner: Action::Stop,
+        }
+    }
+
+    /// Returns an action meaning there was a loop of redirects found.
+    ///
+    /// An `Error` will be returned for the result of the sent request.
+    pub fn loop_detected(self) -> RedirectAction {
+        RedirectAction {
+            inner: Action::LoopDetected,
+        }
+    }
+
+    /// Returns an action meaning there was a loop of redirects found.
+    ///
+    /// An `Error` will be returned for the result of the sent request.
+    pub fn too_many_redirects(self) -> RedirectAction {
+        RedirectAction {
+            inner: Action::TooManyRedirects,
+        }
+    }
+}
+
 enum Policy {
-    Custom(Box<Fn(&Url, &[Url]) -> ::Result<bool> + Send + Sync + 'static>),
+    Custom(Box<Fn(RedirectAttempt) -> RedirectAction + Send + Sync + 'static>),
     Limit(usize),
     None,
 }
@@ -101,8 +161,22 @@ impl fmt::Debug for Policy {
     }
 }
 
-pub fn check_redirect(policy: &RedirectPolicy, next: &Url, previous: &[Url]) -> ::Result<bool> {
+// pub(crate)
-    policy.redirect(next, previous)
+
+#[derive(Debug, PartialEq)]
+pub enum Action {
+    Follow,
+    Stop,
+    LoopDetected,
+    TooManyRedirects,
+}
+
+#[inline]
+pub fn check_redirect(policy: &RedirectPolicy, next: &Url, previous: &[Url]) -> Action {
+    policy.redirect(RedirectAttempt {
+        next: next,
+        previous: previous,
+    }).inner
 }
 
 /*
@@ -132,32 +206,26 @@ fn test_redirect_policy_limit() {
         .collect::<Vec<_>>();
 
 
-    match policy.redirect(&next, &previous) {
+    assert_eq!(check_redirect(&policy, &next, &previous), Action::Follow);
-        Ok(true) => {},
-        other => panic!("expected Ok(true), got: {:?}", other)
-    }
 
     previous.push(Url::parse("http://a.b.d/e/33").unwrap());
 
-    match policy.redirect(&next, &previous) {
+    assert_eq!(check_redirect(&policy, &next, &previous), Action::TooManyRedirects);
-        Err(::Error::TooManyRedirects) => {},
-        other => panic!("expected TooManyRedirects, got: {:?}", other)
-    }
 }
 
 #[test]
 fn test_redirect_policy_custom() {
-    let policy = RedirectPolicy::custom(|next, _previous| {
+    let policy = RedirectPolicy::custom(|attempt| {
-        if next.host_str() == Some("foo") {
+        if attempt.url().host_str() == Some("foo") {
-            Ok(false)
+            attempt.stop()
         } else {
-            Ok(true)
+            attempt.follow()
         }
     });
 
     let next = Url::parse("http://bar/baz").unwrap();
-    assert_eq!(policy.redirect(&next, &[]).unwrap(), true);
+    assert_eq!(check_redirect(&policy, &next, &[]), Action::Follow);
 
     let next = Url::parse("http://foo/baz").unwrap();
-    assert_eq!(policy.redirect(&next, &[]).unwrap(), false);
+    assert_eq!(check_redirect(&policy, &next, &[]), Action::Stop);
 }

src/response.rs

@@ -89,7 +89,7 @@ impl Response {
     /// Try and deserialize the response body as JSON.
     #[inline]
     pub fn json<T: Deserialize>(&mut self) -> ::Result<T> {
-        serde_json::from_reader(self).map_err(::Error::from)
+        serde_json::from_reader(self).map_err(::error::from)
     }
 }
 

tests/client.rs

@@ -207,10 +207,7 @@ fn test_redirect_policy_can_return_errors() {
     };
 
     let err = reqwest::get(&format!("http://{}/loop", server.addr())).unwrap_err();
-    match err {
+    assert!(err.is_redirect());
-        reqwest::Error::RedirectLoop => (),
-        e => panic!("wrong error received: {:?}", e),
-    }
 }
 
 #[test]