From ce43f80d8be3fef72267483aeba0a6927f150035 Mon Sep 17 00:00:00 2001 From: Sean McArthur Date: Mon, 16 Dec 2019 15:57:09 -0800 Subject: [PATCH] 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 | 22 ++--- src/blocking/client.rs | 8 +- src/error.rs | 8 +- src/lib.rs | 20 +--- src/redirect.rs | 203 +++++++++++++++++++-------------------- tests/cookie.rs | 4 +- tests/redirect.rs | 2 +- 7 files changed, 123 insertions(+), 144 deletions(-) diff --git a/src/async_impl/client.rs b/src/async_impl/client.rs index 5b03564..c305d23 100644 --- a/src/async_impl/client.rs +++ b/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, proxies: Vec, auto_sys_proxy: bool, - redirect_policy: RedirectPolicy, + redirect_policy: redirect::Policy, referer: bool, timeout: Option, #[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, proxies: Arc>, @@ -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 => { - return Poll::Ready(Err(crate::error::loop_detected(self.url.clone()))); - } - redirect::Action::TooManyRedirects => { - return Poll::Ready(Err(crate::error::too_many_redirects( + redirect::ActionKind::Error(err) => { + return Poll::Ready(Err(crate::error::redirect( + err, self.url.clone(), ))); } diff --git a/src/blocking/client.rs b/src/blocking/client.rs index 8c13d6a..1d80c8b 100644 --- a/src/blocking/client.rs +++ b/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 { self.inner.execute_request(request) } diff --git a/src/error.rs b/src/error.rs index a0ee866..03e792a 100644 --- a/src/error.rs +++ b/src/error.rs @@ -212,12 +212,8 @@ pub(crate) fn request>(e: E) -> Error { Error::new(Kind::Request, Some(e)) } -pub(crate) fn loop_detected(url: Url) -> Error { - Error::new(Kind::Redirect, Some("infinite redirect loop detected")).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 redirect>(e: E, url: Url) -> Error { + Error::new(Kind::Redirect, Some(e)).with_url(url) } pub(crate) fn status_code(url: Url, status: StatusCode) -> Error { diff --git a/src/lib.rs b/src/lib.rs index c43550b..43f2952 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -119,9 +119,9 @@ //! //! ## Redirect Policies //! -//! By default, a `Client` will automatically handle HTTP redirects, detecting -//! loops, and having a maximum redirect chain of 10 hops. To customize this -//! behavior, a [`RedirectPolicy`][redirect] can used with a `ClientBuilder`. +//! By default, a `Client` will automatically handle HTTP redirects, having a +//! maximum redirect chain of 10 hops. To customize this behavior, a +//! [`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(url: T) -> crate::Result { 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! { diff --git a/src/redirect.rs b/src/redirect.rs index b0f25bc..6d377e2 100644 --- a/src/redirect.rs +++ b/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 { - inner: Policy, +pub struct 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 { - inner: Action, +pub struct Action { + inner: ActionKind, } -impl RedirectPolicy { - /// Create a RedirectPolicy with a maximum number of redirects. +impl Policy { + /// 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 { - RedirectPolicy { - inner: Policy::Limit(max), + pub fn limited(max: usize) -> Self { + Self { + inner: PolicyKind::Limit(max), } } - /// Create a RedirectPolicy that does not follow any redirect. - pub fn none() -> RedirectPolicy { - RedirectPolicy { - inner: Policy::None, + /// Create a `Policy` that does not follow any redirect. + pub fn none() -> Self { + Self { + 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 - pub fn custom(policy: T) -> RedirectPolicy + /// [`Attempt`]: struct.Attempt.html + pub fn custom(policy: T) -> Self where - T: Fn(RedirectAttempt) -> RedirectAction + Send + Sync + 'static, + T: Fn(Attempt) -> Action + Send + Sync + 'static, { - RedirectPolicy { - inner: Policy::Custom(Box::new(policy)), + Self { + 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() - /// to construct one RedirectPolicy that wraps another. + /// This method can be used together with `Policy::custom()` + /// 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), - Policy::Limit(max) => { + PolicyKind::Custom(ref custom) => custom(attempt), + PolicyKind::Limit(max) => { if attempt.previous.len() == max { - attempt.too_many_redirects() - } else if attempt.previous.contains(attempt.next) { - attempt.loop_detected() + attempt.error(TooManyRedirects) } else { attempt.follow() } } - Policy::None => attempt.stop(), + PolicyKind::None => attempt.stop(), } } - pub(crate) fn check(&self, status: StatusCode, next: &Url, previous: &[Url]) -> Action { - self.redirect(RedirectAttempt { + pub(crate) fn check(&self, status: StatusCode, next: &Url, previous: &[Url]) -> ActionKind { + 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 { - fn default() -> RedirectPolicy { +impl Default for Policy { + 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 { - RedirectAction { - inner: Action::Follow, + pub fn follow(self) -> Action { + Action { + 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 { - RedirectAction { - inner: Action::Stop, + pub fn stop(self) -> Action { + Action { + 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. - 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, + /// The `Error` will be returned for the result of the sent request. + pub fn error>>(self, error: E) -> Action { + Action { + inner: ActionKind::Error(error.into()), } } } -enum Policy { - Custom(Box RedirectAction + Send + Sync + 'static>), +enum PolicyKind { + Custom(Box 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"), - Policy::Limit(max) => f.debug_tuple("Limit").field(&max).finish(), - Policy::None => f.pad("None"), + PolicyKind::Custom(..) => f.pad("Custom"), + PolicyKind::Limit(max) => f.debug_tuple("Limit").field(&max).finish(), + PolicyKind::None => f.pad("None"), } } } // pub(crate) -#[derive(Debug, PartialEq)] -pub(crate) enum Action { +#[derive(Debug)] +pub(crate) enum ActionKind { Follow, Stop, - LoopDetected, - TooManyRedirects, + Error(Box), } 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 } } -/* -This was the desired way of doing it, but ran in to inference issues when -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. +#[derive(Debug)] +struct TooManyRedirects; -pub trait Redirect { - fn redirect(&self, next: &Url, previous: &[Url]) -> Result; -} - -impl Redirect for F -where F: Fn(&Url, &[Url]) -> Result { - fn redirect(&self, next: &Url, previous: &[Url]) -> Result { - self(next, previous) +impl fmt::Display for TooManyRedirects { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str("too many redirects") } } -*/ + +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::>(); - assert_eq!( - policy.check(StatusCode::FOUND, &next, &previous), - Action::Follow - ); + + match policy.check(StatusCode::FOUND, &next, &previous) { + ActionKind::Follow => (), + other => panic!("unexpected {:?}", other), + } previous.push(Url::parse("http://a.b.d/e/33").unwrap()); - assert_eq!( - policy.check(StatusCode::FOUND, &next, &previous), - Action::TooManyRedirects - ); + match policy.check(StatusCode::FOUND, &next, &previous) { + ActionKind::Error(err) if err.is::() => (), + 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] diff --git a/tests/cookie.rs b/tests/cookie.rs index b163f1e..2684e32 100644 --- a/tests/cookie.rs +++ b/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(); diff --git a/tests/redirect.rs b/tests/redirect.rs index 66ba54b..e21b320 100644 --- a/tests/redirect.rs +++ b/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)