Refactor Redirect API (#741)

Changed the redirect types to be from the `redirect` module:

- `reqwest::RedirectPolicy` is now `reqwest::redirect::Policy`
- `reqwest::RedirectAttempt` is now `reqwest::redirect::Attempt`
- `reqwest::RedirectAction` is now `reqwest::redirect::Action`

Changed behavior of default policy to no longer check for redirect loops
(loops should still be caught eventually by the maximum limit).

Removed the `too_many_redirects` and `loop_detected` methods from
`Action`.

Added `error` to `Action` that can be passed any error type.

Closes #717

src/async_impl/client.rs

@@ -29,7 +29,7 @@ use crate::connect::Connector;
 #[cfg(feature = "cookies")]
 use crate::cookie;
 use crate::into_url::{expect_uri, try_uri};
-use crate::redirect::{self, remove_sensitive_headers, RedirectPolicy};
+use crate::redirect::{self, remove_sensitive_headers};
 #[cfg(feature = "tls")]
 use crate::tls::TlsBackend;
 #[cfg(feature = "tls")]
@@ -70,7 +70,7 @@ struct Config {
     identity: Option<Identity>,
     proxies: Vec<Proxy>,
     auto_sys_proxy: bool,
-    redirect_policy: RedirectPolicy,
+    redirect_policy: redirect::Policy,
     referer: bool,
     timeout: Option<Duration>,
     #[cfg(feature = "tls")]
@@ -114,7 +114,7 @@ impl ClientBuilder {
                 max_idle_per_host: std::usize::MAX,
                 proxies: Vec::new(),
                 auto_sys_proxy: true,
-                redirect_policy: RedirectPolicy::default(),
+                redirect_policy: redirect::Policy::default(),
                 referer: true,
                 timeout: None,
                 #[cfg(feature = "tls")]
@@ -372,7 +372,7 @@ impl ClientBuilder {
     /// Set a `RedirectPolicy` for this client.
     ///
     /// Default will follow redirects up to a maximum of 10.
-    pub fn redirect(mut self, policy: RedirectPolicy) -> ClientBuilder {
+    pub fn redirect(mut self, policy: redirect::Policy) -> ClientBuilder {
         self.config.redirect_policy = policy;
         self
     }
@@ -915,7 +915,7 @@ struct ClientRef {
     gzip: bool,
     headers: HeaderMap,
     hyper: HyperClient,
-    redirect_policy: RedirectPolicy,
+    redirect_policy: redirect::Policy,
     referer: bool,
     request_timeout: Option<Duration>,
     proxies: Arc<Vec<Proxy>>,
@@ -1124,7 +1124,7 @@ impl Future for PendingRequest {
                         .check(res.status(), &loc, &self.urls);
 
                     match action {
-                        redirect::Action::Follow => {
+                        redirect::ActionKind::Follow => {
                             self.url = loc;
 
                             let mut headers =
@@ -1159,14 +1159,12 @@ impl Future for PendingRequest {
                             *self.as_mut().in_flight().get_mut() = self.client.hyper.request(req);
                             continue;
                         }
-                        redirect::Action::Stop => {
+                        redirect::ActionKind::Stop => {
                             debug!("redirect_policy disallowed redirection to '{}'", loc);
                         }
-                        redirect::Action::LoopDetected => {
+                        redirect::ActionKind::Error(err) => {
-                            return Poll::Ready(Err(crate::error::loop_detected(self.url.clone())));
+                            return Poll::Ready(Err(crate::error::redirect(
-                        }
+                                err,
-                        redirect::Action::TooManyRedirects => {
-                            return Poll::Ready(Err(crate::error::too_many_redirects(
                                 self.url.clone(),
                             )));
                         }

src/blocking/client.rs

@@ -13,7 +13,7 @@ use log::{error, trace};
 use super::request::{Request, RequestBuilder};
 use super::response::Response;
 use super::wait;
-use crate::{async_impl, header, IntoUrl, Method, Proxy, RedirectPolicy};
+use crate::{async_impl, header, IntoUrl, Method, Proxy, redirect};
 #[cfg(feature = "tls")]
 use crate::{Certificate, Identity};
 
@@ -176,10 +176,10 @@ impl ClientBuilder {
 
     // Redirect options
 
-    /// Set a `RedirectPolicy` for this client.
+    /// Set a `redirect::Policy` for this client.
     ///
     /// Default will follow redirects up to a maximum of 10.
-    pub fn redirect(self, policy: RedirectPolicy) -> ClientBuilder {
+    pub fn redirect(self, policy: redirect::Policy) -> ClientBuilder {
         self.with_inner(move |inner| inner.redirect(policy))
     }
 
@@ -541,7 +541,7 @@ impl Client {
     /// # Errors
     ///
     /// This method fails if there was an error while sending request,
-    /// redirect loop was detected or redirect limit was exhausted.
+    /// or redirect limit was exhausted.
     pub fn execute(&self, request: Request) -> crate::Result<Response> {
         self.inner.execute_request(request)
     }

src/error.rs

@@ -212,12 +212,8 @@ pub(crate) fn request<E: Into<BoxError>>(e: E) -> Error {
     Error::new(Kind::Request, Some(e))
 }
 
-pub(crate) fn loop_detected(url: Url) -> Error {
+pub(crate) fn redirect<E: Into<BoxError>>(e: E, url: Url) -> Error {
-    Error::new(Kind::Redirect, Some("infinite redirect loop detected")).with_url(url)
+    Error::new(Kind::Redirect, Some(e)).with_url(url)
-}
-
-pub(crate) fn too_many_redirects(url: Url) -> Error {
-    Error::new(Kind::Redirect, Some("too many redirects")).with_url(url)
 }
 
 pub(crate) fn status_code(url: Url, status: StatusCode) -> Error {

src/lib.rs

@@ -119,9 +119,9 @@
 //!
 //! ## Redirect Policies
 //!
-//! By default, a `Client` will automatically handle HTTP redirects, detecting
+//! By default, a `Client` will automatically handle HTTP redirects, having a
-//! loops, and having a maximum redirect chain of 10 hops. To customize this
+//! maximum redirect chain of 10 hops. To customize this behavior, a
-//! behavior, a [`RedirectPolicy`][redirect] can used with a `ClientBuilder`.
+//! [`redirect::Policy`][redirect] can be used with a `ClientBuilder`.
 //!
 //! ## Cookies
 //!
@@ -175,7 +175,7 @@
 //! [get]: ./fn.get.html
 //! [builder]: ./struct.RequestBuilder.html
 //! [serde]: http://serde.rs
-//! [redirect]: ./struct.RedirectPolicy.html
+//! [redirect]: crate::redirect
 //! [Proxy]: ./struct.Proxy.html
 //! [cargo-features]: https://doc.rust-lang.org/stable/cargo/reference/manifest.html#the-features-section
 
@@ -237,7 +237,6 @@ pub use self::into_url::IntoUrl;
 /// - native TLS backend cannot be initialized
 /// - supplied `Url` cannot be parsed
 /// - there was an error while sending request
-/// - redirect loop was detected
 /// - redirect limit was exhausted
 pub async fn get<T: IntoUrl>(url: T) -> crate::Result<Response> {
     Client::builder().build()?.get(url).send().await
@@ -279,7 +278,6 @@ if_hyper! {
         multipart, Body, Client, ClientBuilder, Request, RequestBuilder, Response, ResponseBuilderExt,
     };
     pub use self::proxy::Proxy;
-    pub use self::redirect::{RedirectAction, RedirectAttempt, RedirectPolicy};
     #[cfg(feature = "tls")]
     pub use self::tls::{Certificate, Identity};
 
@@ -293,17 +291,9 @@ if_hyper! {
     //#[cfg(feature = "trust-dns")]
     //mod dns;
     mod proxy;
-    mod redirect;
+    pub mod redirect;
     #[cfg(feature = "tls")]
     mod tls;
-
-    #[doc(hidden)]
-    #[deprecated(note = "types moved to top of crate")]
-    pub mod r#async {
-        pub use crate::async_impl::{
-            multipart, Body, Client, ClientBuilder, Request, RequestBuilder, Response,
-        };
-    }
 }
 
 if_wasm! {

src/redirect.rs

@@ -1,3 +1,10 @@
+//! Redirect Handling
+//!
+//! By default, a `Client` will automatically handle HTTP redirects, having a
+//! maximum redirect chain of 10 hops. To customize this behavior, a
+//! `redirect::Policy` can be used with a `ClientBuilder`.
+
+use std::error::Error as StdError;
 use std::fmt;
 
 use crate::header::{HeaderMap, AUTHORIZATION, COOKIE, PROXY_AUTHORIZATION, WWW_AUTHENTICATE};
@@ -14,14 +21,14 @@ use crate::Url;
 ///   the allowed maximum redirect hops in a chain.
 /// - `none` can be used to disable all redirect behavior.
 /// - `custom` can be used to create a customized policy.
-pub struct RedirectPolicy {
+pub struct Policy {
-    inner: Policy,
+    inner: PolicyKind,
 }
 
 /// A type that holds information on the next request and previous requests
 /// in redirect chain.
 #[derive(Debug)]
-pub struct RedirectAttempt<'a> {
+pub struct Attempt<'a> {
     status: StatusCode,
     next: &'a Url,
     previous: &'a [Url],
@@ -29,50 +36,50 @@ pub struct RedirectAttempt<'a> {
 
 /// An action to perform when a redirect status code is found.
 #[derive(Debug)]
-pub struct RedirectAction {
+pub struct Action {
-    inner: Action,
+    inner: ActionKind,
 }
 
-impl RedirectPolicy {
+impl Policy {
-    /// Create a RedirectPolicy with a maximum number of redirects.
+    /// Create a `Policy` with a maximum number of redirects.
     ///
     /// An `Error` will be returned if the max is reached.
-    pub fn limited(max: usize) -> RedirectPolicy {
+    pub fn limited(max: usize) -> Self {
-        RedirectPolicy {
+        Self {
-            inner: Policy::Limit(max),
+            inner: PolicyKind::Limit(max),
         }
     }
 
-    /// Create a RedirectPolicy that does not follow any redirect.
+    /// Create a `Policy` that does not follow any redirect.
-    pub fn none() -> RedirectPolicy {
+    pub fn none() -> Self {
-        RedirectPolicy {
+        Self {
-            inner: Policy::None,
+            inner: PolicyKind::None,
         }
     }
 
-    /// Create a custom RedirectPolicy using the passed function.
+    /// Create a custom `Policy` using the passed function.
     ///
     /// # Note
     ///
-    /// The default RedirectPolicy handles redirect loops and a maximum loop
+    /// The default `Policy` handles a maximum loop
     /// chain, but the custom variant does not do that for you automatically.
     /// The custom policy should have some way of handling those.
     ///
     /// Information on the next request and previous requests can be found
-    /// on the [`RedirectAttempt`] argument passed to the closure.
+    /// on the [`Attempt`] argument passed to the closure.
     ///
     /// Actions can be conveniently created from methods on the
-    /// [`RedirectAttempt`].
+    /// [`Attempt`].
     ///
     /// # Example
     ///
     /// ```rust
-    /// # use reqwest::{Error, RedirectPolicy};
+    /// # use reqwest::{Error, redirect};
     /// #
     /// # fn run() -> Result<(), Error> {
-    /// let custom = RedirectPolicy::custom(|attempt| {
+    /// let custom = redirect::Policy::custom(|attempt| {
     ///     if attempt.previous().len() > 5 {
-    ///         attempt.too_many_redirects()
+    ///         attempt.error("too many redirects")
     ///     } else if attempt.url().host_str() == Some("example.domain") {
     ///         // prevent redirects to 'example.domain'
     ///         attempt.stop()
@@ -87,54 +94,52 @@ impl RedirectPolicy {
     /// # }
     /// ```
     ///
-    /// [`RedirectAttempt`]: struct.RedirectAttempt.html
+    /// [`Attempt`]: struct.Attempt.html
-    pub fn custom<T>(policy: T) -> RedirectPolicy
+    pub fn custom<T>(policy: T) -> Self
     where
-        T: Fn(RedirectAttempt) -> RedirectAction + Send + Sync + 'static,
+        T: Fn(Attempt) -> Action + Send + Sync + 'static,
     {
-        RedirectPolicy {
+        Self {
-            inner: Policy::Custom(Box::new(policy)),
+            inner: PolicyKind::Custom(Box::new(policy)),
         }
     }
 
-    /// Apply this policy to a given [`RedirectAttempt`] to produce a [`RedirectAction`].
+    /// Apply this policy to a given [`Attempt`] to produce a [`Action`].
     ///
     /// # Note
     ///
-    /// This method can be used together with RedirectPolicy::custom()
+    /// This method can be used together with `Policy::custom()`
-    /// to construct one RedirectPolicy that wraps another.
+    /// to construct one `Policy` that wraps another.
     ///
     /// # Example
     ///
     /// ```rust
-    /// # use reqwest::{Error, RedirectPolicy};
+    /// # use reqwest::{Error, redirect};
     /// #
     /// # fn run() -> Result<(), Error> {
-    /// let custom = RedirectPolicy::custom(|attempt| {
+    /// let custom = redirect::Policy::custom(|attempt| {
     ///     eprintln!("{}, Location: {:?}", attempt.status(), attempt.url());
-    ///     RedirectPolicy::default().redirect(attempt)
+    ///     redirect::Policy::default().redirect(attempt)
     /// });
     /// # Ok(())
     /// # }
     /// ```
-    pub fn redirect(&self, attempt: RedirectAttempt) -> RedirectAction {
+    pub fn redirect(&self, attempt: Attempt) -> Action {
         match self.inner {
-            Policy::Custom(ref custom) => custom(attempt),
+            PolicyKind::Custom(ref custom) => custom(attempt),
-            Policy::Limit(max) => {
+            PolicyKind::Limit(max) => {
                 if attempt.previous.len() == max {
-                    attempt.too_many_redirects()
+                    attempt.error(TooManyRedirects)
-                } else if attempt.previous.contains(attempt.next) {
-                    attempt.loop_detected()
                 } else {
                     attempt.follow()
                 }
             }
-            Policy::None => attempt.stop(),
+            PolicyKind::None => attempt.stop(),
         }
     }
 
-    pub(crate) fn check(&self, status: StatusCode, next: &Url, previous: &[Url]) -> Action {
+    pub(crate) fn check(&self, status: StatusCode, next: &Url, previous: &[Url]) -> ActionKind {
-        self.redirect(RedirectAttempt {
+        self.redirect(Attempt {
             status,
             next,
             previous,
@@ -144,20 +149,20 @@ impl RedirectPolicy {
 
     pub(crate) fn is_default(&self) -> bool {
         match self.inner {
-            Policy::Limit(10) => true,
+            PolicyKind::Limit(10) => true,
             _ => false,
         }
     }
 }
 
-impl Default for RedirectPolicy {
+impl Default for Policy {
-    fn default() -> RedirectPolicy {
+    fn default() -> Policy {
         // Keep `is_default` in sync
-        RedirectPolicy::limited(10)
+        Policy::limited(10)
     }
 }
 
-impl<'a> RedirectAttempt<'a> {
+impl<'a> Attempt<'a> {
     /// Get the type of redirect.
     pub fn status(&self) -> StatusCode {
         self.status
@@ -173,70 +178,60 @@ impl<'a> RedirectAttempt<'a> {
         self.previous
     }
     /// Returns an action meaning reqwest should follow the next URL.
-    pub fn follow(self) -> RedirectAction {
+    pub fn follow(self) -> Action {
-        RedirectAction {
+        Action {
-            inner: Action::Follow,
+            inner: ActionKind::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 {
+    pub fn stop(self) -> Action {
-        RedirectAction {
+        Action {
-            inner: Action::Stop,
+            inner: ActionKind::Stop,
         }
     }
 
-    /// Returns an action meaning there was a loop of redirects found.
+    /// Returns an action failing the redirect with an error.
     ///
-    /// An `Error` will be returned for the result of the sent request.
+    /// The `Error` will be returned for the result of the sent request.
-    pub fn loop_detected(self) -> RedirectAction {
+    pub fn error<E: Into<Box<dyn StdError + Send + Sync>>>(self, error: E) -> Action {
-        RedirectAction {
+        Action {
-            inner: Action::LoopDetected,
+            inner: ActionKind::Error(error.into()),
-        }
-    }
-
-    /// 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 {
+enum PolicyKind {
-    Custom(Box<dyn Fn(RedirectAttempt) -> RedirectAction + Send + Sync + 'static>),
+    Custom(Box<dyn Fn(Attempt) -> Action + Send + Sync + 'static>),
     Limit(usize),
     None,
 }
 
-impl fmt::Debug for RedirectPolicy {
+impl fmt::Debug for Policy {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        f.debug_tuple("RedirectPolicy").field(&self.inner).finish()
+        f.debug_tuple("Policy").field(&self.inner).finish()
     }
 }
 
-impl fmt::Debug for Policy {
+impl fmt::Debug for PolicyKind {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match *self {
-            Policy::Custom(..) => f.pad("Custom"),
+            PolicyKind::Custom(..) => f.pad("Custom"),
-            Policy::Limit(max) => f.debug_tuple("Limit").field(&max).finish(),
+            PolicyKind::Limit(max) => f.debug_tuple("Limit").field(&max).finish(),
-            Policy::None => f.pad("None"),
+            PolicyKind::None => f.pad("None"),
         }
     }
 }
 
 // pub(crate)
 
-#[derive(Debug, PartialEq)]
+#[derive(Debug)]
-pub(crate) enum Action {
+pub(crate) enum ActionKind {
     Follow,
     Stop,
-    LoopDetected,
+    Error(Box<dyn StdError + Send + Sync>),
-    TooManyRedirects,
 }
 
 pub(crate) fn remove_sensitive_headers(headers: &mut HeaderMap, next: &Url, previous: &[Url]) {
@@ -253,48 +248,42 @@ pub(crate) fn remove_sensitive_headers(headers: &mut HeaderMap, next: &Url, prev
     }
 }
 
-/*
+#[derive(Debug)]
-This was the desired way of doing it, but ran in to inference issues when
+struct TooManyRedirects;
-using closures, since the arguments received are references (&Url and &[Url]),
-and the compiler could not infer the lifetimes of those references. That means
-people would need to annotate the closure's argument types, which is garbase.
 
-pub trait Redirect {
+impl fmt::Display for TooManyRedirects {
-    fn redirect(&self, next: &Url, previous: &[Url]) -> Result<bool>;
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-}
+        f.write_str("too many redirects")
-
-impl<F> Redirect for F
-where F: Fn(&Url, &[Url]) -> Result<bool> {
-    fn redirect(&self, next: &Url, previous: &[Url]) -> Result<bool> {
-        self(next, previous)
     }
 }
-*/
+
+impl StdError for TooManyRedirects {}
 
 #[test]
 fn test_redirect_policy_limit() {
-    let policy = RedirectPolicy::default();
+    let policy = Policy::default();
     let next = Url::parse("http://x.y/z").unwrap();
     let mut previous = (0..9)
         .map(|i| Url::parse(&format!("http://a.b/c/{}", i)).unwrap())
         .collect::<Vec<_>>();
 
-    assert_eq!(
+
-        policy.check(StatusCode::FOUND, &next, &previous),
+    match policy.check(StatusCode::FOUND, &next, &previous) {
-        Action::Follow
+        ActionKind::Follow => (),
-    );
+        other => panic!("unexpected {:?}", other),
+    }
 
     previous.push(Url::parse("http://a.b.d/e/33").unwrap());
 
-    assert_eq!(
+    match policy.check(StatusCode::FOUND, &next, &previous) {
-        policy.check(StatusCode::FOUND, &next, &previous),
+        ActionKind::Error(err) if err.is::<TooManyRedirects>() => (),
-        Action::TooManyRedirects
+        other => panic!("unexpected {:?}", other),
-    );
+    }
 }
 
 #[test]
 fn test_redirect_policy_custom() {
-    let policy = RedirectPolicy::custom(|attempt| {
+    let policy = Policy::custom(|attempt| {
         if attempt.url().host_str() == Some("foo") {
             attempt.stop()
         } else {
@@ -303,10 +292,16 @@ fn test_redirect_policy_custom() {
     });
 
     let next = Url::parse("http://bar/baz").unwrap();
-    assert_eq!(policy.check(StatusCode::FOUND, &next, &[]), Action::Follow);
+    match policy.check(StatusCode::FOUND, &next, &[]) {
+        ActionKind::Follow => (),
+        other => panic!("unexpected {:?}", other),
+    }
 
     let next = Url::parse("http://foo/baz").unwrap();
-    assert_eq!(policy.check(StatusCode::FOUND, &next, &[]), Action::Stop);
+     match policy.check(StatusCode::FOUND, &next, &[]) {
+        ActionKind::Stop => (),
+        other => panic!("unexpected {:?}", other),
+    }
 }
 
 #[test]

tests/cookie.rs

@@ -173,7 +173,7 @@ async fn cookie_store_expires() {
         }
     });
 
-    let client = reqwest::r#async::Client::builder()
+    let client = reqwest::Client::builder()
         .cookie_store(true)
         .build()
         .unwrap();
@@ -201,7 +201,7 @@ async fn cookie_store_path() {
         }
     });
 
-    let client = reqwest::r#async::Client::builder()
+    let client = reqwest::Client::builder()
         .cookie_store(true)
         .build()
         .unwrap();

tests/redirect.rs

@@ -240,7 +240,7 @@ async fn test_redirect_policy_can_stop_redirects_without_an_error() {
     let url = format!("http://{}/no-redirect", server.addr());
 
     let res = reqwest::Client::builder()
-        .redirect(reqwest::RedirectPolicy::none())
+        .redirect(reqwest::redirect::Policy::none())
         .build()
         .unwrap()
         .get(&url)