Repository: algesten/ureq (rust)

Issue:
Improve doc of Config::max_redirects() in version 3.x
The current doc is:

> The max number of redirects to follow before giving up

I've three questions:

- What happens after the max redirects have been followed?
  - Will there be an error, which type of error?
  - Will the last HTTP response be returned?
- How can I access the final URL?
  This has been answered in https://github.com/algesten/ureq/issues/731#issuecomment-2563973764. But I would not have found it myself as I'm new to rust.
- What happens if I set this config to zero?
  - Will there be an error on redirect?
  - Will I just get a response with a redirect HTTP status code?

Thank you!

Pull request:
Max redirects do error
Close #915 

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 088cf3a..54d2fc4 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,7 @@
 # Unreleased
 
+  * Default to `TooManyRedirects` error (#916)
+  * Add `ConfigBuilder::max_redirects_will_error()` (#916)
   * Add new `SendBody::into_reader()` (#914)
   * Fix completely broken PEM parsing (#912)
   * Improve ergonomics for `AutoHeaderValue` (#896)
diff --git a/src/config.rs b/src/config.rs
index 18a5fa3..cec4c7e 100644
--- a/src/config.rs
+++ b/src/config.rs
@@ -143,6 +143,7 @@ pub struct Config {
     proxy: Option<Proxy>,
     no_delay: bool,
     max_redirects: u32,
+    max_redirects_will_error: bool,
     redirect_auth_headers: RedirectAuthHeaders,
     user_agent: AutoHeaderValue,
     accept: AutoHeaderValue,
@@ -187,6 +188,10 @@ impl Config {
 
         Some(proxy.uri())
     }
+
+    pub(crate) fn max_redirects_do_error(&self) -> bool {
+        self.max_redirects > 0 && self.max_redirects_will_error
+    }
 }
 
 impl Config {
@@ -238,13 +243,30 @@ impl Config {
         self.no_delay
     }
 
-    /// The max number of redirects to follow before giving up
+    /// The max number of redirects to follow before giving up.
+    ///
+    /// Whe max redirects are reached, the behavior is controlled by the
+    /// `max_redirects_will_error` setting. Set to `true` (which
+    /// is the default) the result is a `TooManyRedirects` error. Set
+    /// to `false`, the response is returned as is.
+    ///
+    /// If `max_redirects` is 0, no redirects are followed and the response
+    /// is always returned (never a `TooManyRedirects` error).
     ///
     /// Defaults to 10
     pub fn max_redirects(&self) -> u32 {
         self.max_redirects
     }
 
+    /// If we should error when max redirects are reached.
+    ///
+    /// This has no meaning if `max_redirects` is 0.
+    ///
+    /// Defaults to true
+    pub fn max_redirects_will_error(&self) -> bool {
+        self.max_redirects_will_error
+    }
+
     /// How to handle `Authorization` headers when following redirects
     ///
     /// * `Never` (the default) means the authorization header is never attached to a redirected call.
@@ -423,7 +445,15 @@ impl<Scope: private::ConfigScope> ConfigBuilder<Scope> {
         self
     }
 
-    /// The max number of redirects to follow before giving up
+    /// The max number of redirects to follow before giving up.
+    ///
+    /// Whe max redirects are reached, the behavior is controlled by the
+    /// `max_redirects_will_error` setting. Set to `true` (which
+    /// is the default) the result is a `TooManyRedirects` error. Set
+    /// to `false`, the response is returned as is.
+    ///
+    /// If `max_redirects` is 0, no redirects are followed and the response
+    /// is always returned (never a `TooManyRedirects` error).
     ///
     /// Defaults to 10
     pub fn max_redirects(mut self, v: u32) -> Self {
@@ -431,6 +461,16 @@ impl<Scope: private::ConfigScope> ConfigBuilder<Scope> {
         self
     }
 
+    /// If we should error when max redirects are reached.
+    ///
+    /// This has no meaning if `max_redirects` is 0.
+    ///
+    /// Defaults to true
+    pub fn max_redirects_will_error(mut self, v: bool) -> Self {
+        self.config().max_redirects_will_error = v;
+        self
+    }
+
     /// How to handle `Authorization` headers when following redirects
     ///
     /// * `Never` (the default) means the authorization header is never attached to a redirected call.
@@ -767,6 +807,7 @@ impl Default for Config {
             proxy: Proxy::try_from_env(),
             no_delay: true,
             max_redirects: 10,
+            max_redirects_will_error: true,
             redirect_auth_headers: RedirectAuthHeaders::Never,
             user_agent: AutoHeaderValue::default(),
             accept: AutoHeaderValue::default(),
diff --git a/src/error.rs b/src/error.rs
index a6c5d82..3675255 100644
--- a/src/error.rs
+++ b/src/error.rs
@@ -54,6 +54,16 @@ pub enum Error {
     /// A send body (Such as `&str`) is larger than the `content-length` header.
     BodyExceedsLimit(u64),
 
+    /// Too many redirects.
+    ///
+    /// The error can be turned off by setting
+    /// [`max_redirects_will_error()`](crate::config::ConfigBuilder::max_redirects_will_error)
+    /// to false. When turned off, the last response will be returned instead of causing
+    /// an error, even if it is a redirect.
+    ///
+    /// The number of redirects is limited to 10 by default.
+    TooManyRedirects,
+
     /// Some error with TLS.
     #[cfg(feature = "_tls")]
     Tls(&'static str),
@@ -190,6 +200,7 @@ impl fmt::Display for Error {
             Error::BodyExceedsLimit(v) => {
                 write!(f, "the response body is larger than request limit: {}", v)
             }
+            Error::TooManyRedirects => write!(f, "too many redirects"),
             #[cfg(feature = "_tls")]
             Error::Tls(v) => write!(f, "{}", v),
             #[cfg(feature = "_tls")]
diff --git a/src/lib.rs b/src/lib.rs
index 099dca1..f8feba1 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -753,6 +753,38 @@ pub(crate) mod test {
         assert_eq!(txt, "");
     }
 
+    #[test]
+    fn redirect_max_with_error() {
+        init_test_log();
+        let agent: Agent = Config::builder().max_redirects(3).build().into();
+        let res = agent
+            .get(
+                "http://httpbin.org/redirect-to?url=%2Fredirect-to%3F\
+                url%3D%2Fredirect-to%3Furl%3D%252Fredirect-to%253Furl%253D",
+            )
+            .call();
+        let err = res.unwrap_err();
+        assert_eq!(err.to_string(), "too many redirects");
+    }
+
+    #[test]
+    fn redirect_max_without_error() {
+        init_test_log();
+        let agent: Agent = Config::builder()
+            .max_redirects(3)
+            .max_redirects_will_error(false)
+            .build()
+            .into();
+        let res = agent
+            .get(
+                "http://httpbin.org/redirect-to?url=%2Fredirect-to%3F\
+                url%3D%2Fredirect-to%3Furl%3D%252Fredirect-to%253Furl%253D",
+            )
+            .call()
+            .unwrap();
+        assert_eq!(res.status(), 302);
+    }
+
     #[test]
     fn redirect_follow() {
         init_test_log();
diff --git a/src/run.rs b/src/run.rs
index bf9d29c..b561fd5 100644
--- a/src/run.rs
+++ b/src/run.rs
@@ -178,10 +178,16 @@ fn flow_run(
                 ..Default::default()
             };
 
-            if response.status().is_redirection() && redirect_count < config.max_redirects() {
-                let flow = handler.consume_redirect_body()?;
-
-                FlowResult::Redirect(flow, handler.timings)
+            if response.status().is_redirection() {
+                if redirect_count < config.max_redirects() {
+                    let flow = handler.consume_redirect_body()?;
+
+                    FlowResult::Redirect(flow, handler.timings)
+                } else if config.max_redirects_do_error() {
+                    return Err(Error::TooManyRedirects);
+                } else {
+                    FlowResult::Response(response, handler)
+                }
             } else {
                 FlowResult::Response(response, handler)
             }
@@ -189,10 +195,12 @@ fn flow_run(
         RecvResponseResult::Redirect(flow) => {
             cleanup(connection, flow.must_close_connection(), timings.now());
 
-            if redirect_count >= config.max_redirects() {
-                FlowResult::Response(response, BodyHandler::default())
-            } else {
+            if redirect_count < config.max_redirects() {
                 FlowResult::Redirect(flow, mem::take(timings))
+            } else if config.max_redirects_do_error() {
+                return Err(Error::TooManyRedirects);
+            } else {
+                FlowResult::Response(response, BodyHandler::default())
             }
         }
         RecvResponseResult::Cleanup(flow) => {


Test patch:
diff --git a/src/unversioned/transport/test.rs b/src/unversioned/transport/test.rs
index 039a3df..48c5df4 100644
--- a/src/unversioned/transport/test.rs
+++ b/src/unversioned/transport/test.rs
@@ -289,16 +289,23 @@ fn setup_default_handlers(handlers: &mut Vec<TestHandler>) {
     );
 
     maybe_add(
-        TestHandler::new("/redirect-to", |_uri, _req, w| {
+        TestHandler::new("/redirect-to", |uri, _req, w| {
+            let location = uri.query().unwrap();
+            assert!(location.starts_with("url="));
+            let location = &location[4..];
+            let location = percent_encoding::percent_decode_str(location)
+                .decode_utf8()
+                .unwrap();
             write!(
                 w,
                 "HTTP/1.1 302 FOUND\r\n\
-                Location: /get\r\n\
+                Location: {}\r\n\
                 Content-Length: 22\r\n\
                 Connection: close\r\n\
                 \r\n\
                 You've been redirected\
                 ",
+                location
             )
         }),
         handlers,