From 4c3b93d86dcdd6a8a61da539911605502971d262 Mon Sep 17 00:00:00 2001
From: Jacob Hoffman-Andrews <github@hoffman-andrews.com>
Date: Wed, 25 Nov 2020 20:15:46 -0800
Subject: [PATCH] Add Error::{kind, status, into_response}.

Also, remove Response::{ok, error, client_error, server_error,
redirect}. The idea is that you would access these through the
Error object instead.

I fetched all the reverse dependencies of ureq on crates.io and looked
for uses of the methods being removed. I found none.

I'm also considering removing the error_on_non_2xx method entirely. If
it's easy to get the underlying response for errors, it would be nice to
make that the single way to do things rather than support two separate
ways of handling HTTP errors.
---
 src/error.rs       | 46 +++++++++++++++++++++++++++++++++++++++++++---
 src/request.rs     |  2 +-
 src/response.rs    | 24 ------------------------
 src/test/simple.rs |  1 -
 src/unit.rs        |  2 +-
 5 files changed, 45 insertions(+), 30 deletions(-)

diff --git a/src/error.rs b/src/error.rs
index 5739c43..f5e43d8 100644
--- a/src/error.rs
+++ b/src/error.rs
@@ -13,7 +13,7 @@ pub struct Error {
     message: Option<String>,
     url: Option<Url>,
     source: Option<Box<dyn error::Error + Send + Sync + 'static>>,
-    response: Option<Box<Response>>,
+    response: Option<Response>,
 }
 
 impl Display for Error {
@@ -67,13 +67,53 @@ impl Error {
     }
 
     pub(crate) fn response(mut self, response: Response) -> Self {
-        self.response = Some(Box::new(response));
+        self.response = Some(response);
         self
     }
-    pub(crate) fn kind(&self) -> ErrorKind {
+
+    /// The type of this error.
+    ///
+    /// ```
+    /// # ureq::is_test(true);
+    /// let err = ureq::get("http://httpbin.org/status/500")
+    ///     .call().unwrap_err();
+    /// assert_eq!(err.kind(), ureq::ErrorKind::HTTP);
+    /// ```
+    pub fn kind(&self) -> ErrorKind {
         self.kind
     }
 
+    /// For Errors of type HTTP (i.e. those that result from an HTTP status code),
+    /// return the status code of the response. For all other Errors, return 0.
+    ///
+    /// ```
+    /// # ureq::is_test(true);
+    /// let err = ureq::get("http://httpbin.org/status/500")
+    ///     .call().unwrap_err();
+    /// assert_eq!(err.kind(), ureq::ErrorKind::HTTP);
+    /// assert_eq!(err.status(), 500);
+    /// ```
+    pub fn status(&self) -> u16 {
+        match &self.response {
+            Some(response) => response.status(),
+            None => 0,
+        }
+    }
+
+    /// For an Error of type HTTP (i.e. those that result from an HTTP status code),
+    /// turn the error into the underlying Response. For other errors, return None.
+    ///
+    /// ```
+    /// # ureq::is_test(true);
+    /// let err = ureq::get("http://httpbin.org/status/500")
+    ///     .call().unwrap_err();
+    /// assert_eq!(err.kind(), ureq::ErrorKind::HTTP);
+    /// assert_eq!(err.into_response().unwrap().status(), 500);
+    /// ```
+    pub fn into_response(self) -> Option<Response> {
+        self.response
+    }
+
     /// Return true iff the error was due to a connection closing.
     pub(crate) fn connection_closed(&self) -> bool {
         if self.kind() != ErrorKind::Io {
diff --git a/src/request.rs b/src/request.rs
index 18a882a..edd622b 100644
--- a/src/request.rs
+++ b/src/request.rs
@@ -121,7 +121,7 @@ impl Request {
         let response =
             unit::connect(unit, true, 0, reader, false).map_err(|e| e.url(url.clone()))?;
 
-        if response.error() && self.error_on_non_2xx {
+        if self.error_on_non_2xx && response.status() >= 400 {
             Err(ErrorKind::HTTP.new().url(url.clone()).response(response))
         } else {
             Ok(response)
diff --git a/src/response.rs b/src/response.rs
index e17bf32..1906995 100644
--- a/src/response.rs
+++ b/src/response.rs
@@ -145,30 +145,6 @@ impl Response {
             .collect()
     }
 
-    /// Whether the response status is: 200 <= status <= 299
-    pub fn ok(&self) -> bool {
-        self.status >= 200 && self.status <= 299
-    }
-
-    pub fn redirect(&self) -> bool {
-        self.status >= 300 && self.status <= 399
-    }
-
-    /// Whether the response status is: 400 <= status <= 499
-    pub fn client_error(&self) -> bool {
-        self.status >= 400 && self.status <= 499
-    }
-
-    /// Whether the response status is: 500 <= status <= 599
-    pub fn server_error(&self) -> bool {
-        self.status >= 500 && self.status <= 599
-    }
-
-    /// Whether the response status is: 400 <= status <= 599
-    pub fn error(&self) -> bool {
-        self.client_error() || self.server_error()
-    }
-
     /// The content type part of the "Content-Type" header without
     /// the charset.
     ///
diff --git a/src/test/simple.rs b/src/test/simple.rs
index 5a45e78..842bc37 100644
--- a/src/test/simple.rs
+++ b/src/test/simple.rs
@@ -175,7 +175,6 @@ pub fn no_status_text() {
         test::make_response(200, "", vec![], vec![])
     });
     let resp = get("test://host/no_status_text").call().unwrap();
-    assert!(resp.ok());
     assert_eq!(resp.status(), 200);
 }
 
diff --git a/src/unit.rs b/src/unit.rs
index 934b6db..720b422 100644
--- a/src/unit.rs
+++ b/src/unit.rs
@@ -231,7 +231,7 @@ pub(crate) fn connect(
     save_cookies(&unit, &resp);
 
     // handle redirects
-    if resp.redirect() && unit.agent.config.redirects > 0 {
+    if resp.status() >= 300 && resp.status() < 400 && unit.agent.config.redirects > 0 {
         if redirect_count == unit.agent.config.redirects {
             return Err(ErrorKind::TooManyRedirects.new());
         }