Repository: algesten/ureq
Pull request #259 (2020-11-28 04:37:13)

De-redundantize Error kinds.
Change "Bad" to "Invalid" in error names, mimicking io::Error::ErrorKind.

Change InvalidProxyCreds to ProxyUnauthorized.

Change DnsFailed to just Dns (the fact that there was a failure is implicit
in the fact that this was an error).

jsha: Good point about io::Error having Invalid. I've changed to InvalidUrl. Are there any others that you think should have `Invalid`? E.g. InvalidStatus or InvalidHeader?
jyn514: 'InvalidStatus' seems to mean the status is itself invalid, like a status of 0 or something. I think 'ErrorStatus' or something would be more descriptive.
jsha: > 'InvalidStatus' seems to mean the status is itself invalid, like a status of 0 or something. I think 'ErrorStatus' or something would be more descriptive.

Actually, the ErrorKind `InvalidStatus` (or `Status` in this PR) reflects a failure to parse the status line - for instance a status of 0. For this PR, I'm going to revert it to `BadStatus` just so we don't have lots of conflicts with #266. Later, though, we should change this to merge BadStatus and BadHeader into just one ErrorKind, `InvalidHeader`. That avoids any potential confusion with `error::Error::Status(u16, Response)`.

Patch:
diff --git a/src/error.rs b/src/error.rs
index 8ae819d..ab3e85e 100644
--- a/src/error.rs
+++ b/src/error.rs
@@ -168,11 +168,11 @@ impl Error {
 #[derive(Debug, PartialEq, Clone, Copy)]
 pub enum ErrorKind {
     /// The url could not be understood.
-    BadUrl,
+    InvalidUrl,
     /// The url scheme could not be understood.
     UnknownScheme,
     /// DNS lookup failed.
-    DnsFailed,
+    Dns,
     /// Connection to server failed.
     ConnectionFailed,
     /// Too many redirects.
@@ -184,14 +184,12 @@ pub enum ErrorKind {
     /// Some unspecified `std::io::Error`.
     Io,
     /// Proxy information was not properly formatted
-    BadProxy,
-    /// Proxy credentials were not properly formatted
-    BadProxyCreds,
+    InvalidProxyUrl,
     /// Proxy could not connect
     ProxyConnect,
     /// Incorrect credentials for proxy
-    InvalidProxyCreds,
-    /// HTTP status code indicating an error (e.g. 4xx, 5xx).
+    ProxyUnauthorized,
+    /// HTTP status code indicating an error (e.g. 4xx, 5xx)
     /// Read the inner response body for details and to return
     /// the connection to the pool.
     HTTP,
@@ -228,18 +226,17 @@ impl From<Transport> for Error {
 impl fmt::Display for ErrorKind {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match self {
-            ErrorKind::BadUrl => write!(f, "Bad URL"),
+            ErrorKind::InvalidUrl => write!(f, "Bad URL"),
             ErrorKind::UnknownScheme => write!(f, "Unknown Scheme"),
-            ErrorKind::DnsFailed => write!(f, "Dns Failed"),
+            ErrorKind::Dns => write!(f, "Dns Failed"),
             ErrorKind::ConnectionFailed => write!(f, "Connection Failed"),
             ErrorKind::TooManyRedirects => write!(f, "Too Many Redirects"),
             ErrorKind::BadStatus => write!(f, "Bad Status"),
             ErrorKind::BadHeader => write!(f, "Bad Header"),
             ErrorKind::Io => write!(f, "Network Error"),
-            ErrorKind::BadProxy => write!(f, "Malformed proxy"),
-            ErrorKind::BadProxyCreds => write!(f, "Failed to parse proxy credentials"),
+            ErrorKind::InvalidProxyUrl => write!(f, "Malformed proxy"),
             ErrorKind::ProxyConnect => write!(f, "Proxy failed to connect"),
-            ErrorKind::InvalidProxyCreds => write!(f, "Provided proxy credentials are incorrect"),
+            ErrorKind::ProxyUnauthorized => write!(f, "Provided proxy credentials are incorrect"),
             ErrorKind::HTTP => write!(f, "HTTP status error"),
         }
     }
@@ -282,6 +279,6 @@ fn connection_closed() {
 fn error_is_send_and_sync() {
     fn takes_send(_: impl Send) {}
     fn takes_sync(_: impl Sync) {}
-    takes_send(crate::error::ErrorKind::BadUrl.new());
-    takes_sync(crate::error::ErrorKind::BadUrl.new());
+    takes_send(crate::error::ErrorKind::InvalidUrl.new());
+    takes_sync(crate::error::ErrorKind::InvalidUrl.new());
 }
diff --git a/src/lib.rs b/src/lib.rs
index bad23ea..8fc44ab 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -369,7 +369,7 @@ mod tests {
     #[cfg(feature = "tls")]
     fn connect_https_invalid_name() {
         let result = get("https://example.com{REQUEST_URI}/").call();
-        let e = ErrorKind::DnsFailed;
+        let e = ErrorKind::Dns;
         assert_eq!(result.unwrap_err().kind(), e);
     }
 }
diff --git a/src/proxy.rs b/src/proxy.rs
index 5f1bad3..0daea67 100644
--- a/src/proxy.rs
+++ b/src/proxy.rs
@@ -30,7 +30,7 @@ impl Proxy {
                     .into_iter();
 
                 if parts.len() != 2 {
-                    Err(ErrorKind::BadProxyCreds.new())
+                    Err(ErrorKind::InvalidProxyUrl.new())
                 } else {
                     Ok((
                         parts.next().map(String::from),
@@ -46,14 +46,14 @@ impl Proxy {
         match host {
             Some(host) => {
                 let mut parts = host.as_ref().split(':').collect::<Vec<&str>>().into_iter();
-                let host = parts.next().ok_or(ErrorKind::BadProxy.new())?;
+                let host = parts.next().ok_or(ErrorKind::InvalidProxyUrl.new())?;
                 let port = parts.next();
                 Ok((
                     String::from(host),
                     port.and_then(|port| port.parse::<u32>().ok()),
                 ))
             }
-            None => Err(ErrorKind::BadProxy.new()),
+            None => Err(ErrorKind::InvalidProxyUrl.new()),
         }
     }
 
@@ -84,7 +84,7 @@ impl Proxy {
                 Some("http") => Proto::HTTPConnect,
                 Some("socks") => Proto::SOCKS5,
                 Some("socks5") => Proto::SOCKS5,
-                _ => return Err(ErrorKind::BadProxy.new()),
+                _ => return Err(ErrorKind::InvalidProxyUrl.new()),
             }
         } else {
             Proto::HTTPConnect
@@ -92,7 +92,7 @@ impl Proxy {
 
         let remaining_parts = proxy_parts.next();
         if remaining_parts == None {
-            return Err(ErrorKind::BadProxy.new());
+            return Err(ErrorKind::InvalidProxyUrl.new());
         }
 
         let mut creds_server_port_parts = remaining_parts
@@ -159,12 +159,12 @@ Proxy-Connection: Keep-Alive\r\n\
         let status_code = top_line
             .split_whitespace()
             .nth(1)
-            .ok_or(ErrorKind::BadProxy.new())?;
+            .ok_or(ErrorKind::ProxyConnect.new())?;
 
         match status_code {
             "200" => Ok(()),
-            "401" | "407" => Err(ErrorKind::InvalidProxyCreds.new()),
-            _ => Err(ErrorKind::BadProxy.new()),
+            "401" | "407" => Err(ErrorKind::ProxyUnauthorized.new()),
+            _ => Err(ErrorKind::ProxyConnect.new()),
         }
     }
 }
diff --git a/src/request.rs b/src/request.rs
index 792632e..0394ad9 100644
--- a/src/request.rs
+++ b/src/request.rs
@@ -108,7 +108,7 @@ impl Request {
         let mut url: Url = match self.url.clone() {
             Urlish::Url(u) => u,
             Urlish::Str(s) => s.parse().map_err(|e: url::ParseError| {
-                ErrorKind::BadUrl
+                ErrorKind::InvalidUrl
                     .msg(&format!("failed to parse URL '{}'", self.url))
                     .src(e)
             })?,
diff --git a/src/response.rs b/src/response.rs
index 1906995..bb68527 100644
--- a/src/response.rs
+++ b/src/response.rs
@@ -94,11 +94,6 @@ impl Response {
         self.url.as_ref().map(|s| &s[..]).unwrap_or("")
     }
 
-    /// The entire status line like: `HTTP/1.1 200 OK`
-    pub fn status_line(&self) -> &str {
-        self.status_line.as_str()
-    }
-
     /// The http version: `HTTP/1.1`
     pub fn http_version(&self) -> &str {
         &self.status_line.as_str()[0..self.index.http_version]
diff --git a/src/stream.rs b/src/stream.rs
index 149771e..5850066 100644
--- a/src/stream.rs
+++ b/src/stream.rs
@@ -341,7 +341,7 @@ pub(crate) fn connect_https(unit: &Unit, hostname: &str) -> Result<Stream, Error
     let port = unit.url.port().unwrap_or(443);
 
     let sni = webpki::DNSNameRef::try_from_ascii_str(hostname)
-        .map_err(|err| ErrorKind::DnsFailed.new().src(err))?;
+        .map_err(|err| ErrorKind::Dns.new().src(err))?;
     let tls_conf: &Arc<rustls::ClientConfig> = unit
         .agent
         .config
@@ -375,10 +375,10 @@ pub(crate) fn connect_host(unit: &Unit, hostname: &str, port: u16) -> Result<Tcp
     let sock_addrs = unit
         .resolver()
         .resolve(&netloc)
-        .map_err(|e| ErrorKind::DnsFailed.new().src(e))?;
+        .map_err(|e| ErrorKind::Dns.new().src(e))?;
 
     if sock_addrs.is_empty() {
-        return Err(ErrorKind::DnsFailed.msg(&format!("No ip address for {}", hostname)));
+        return Err(ErrorKind::Dns.msg(&format!("No ip address for {}", hostname)));
     }
 
     let proto = if let Some(ref proxy) = proxy {
diff --git a/src/unit.rs b/src/unit.rs
index e434392..091bef7 100644
--- a/src/unit.rs
+++ b/src/unit.rs
@@ -172,7 +172,7 @@ pub(crate) fn connect(
     let host = unit
         .url
         .host_str()
-        .ok_or(ErrorKind::BadUrl.msg("no host in URL"))?;
+        .ok_or(ErrorKind::InvalidUrl.msg("no host in URL"))?;
     let url = &unit.url;
     let method = &unit.method;
     // open socket
@@ -241,7 +241,7 @@ pub(crate) fn connect(
         if let Some(location) = location {
             // join location header to current url in case it it relative
             let new_url = url.join(location).map_err(|e| {
-                ErrorKind::BadUrl
+                ErrorKind::InvalidUrl
                     .msg(&format!("Bad redirection: {}", location))
                     .src(e)
             })?;


Test patch:
diff --git a/src/test/redirect.rs b/src/test/redirect.rs
index d441db0..7a88a79 100644
--- a/src/test/redirect.rs
+++ b/src/test/redirect.rs
@@ -106,7 +106,7 @@ fn redirect_host() {
     let url = format!("http://localhost:{}/", srv.port);
     let result = crate::Agent::new().get(&url).call();
     assert!(
-        matches!(result, Err(ref e) if e.kind() == ErrorKind::DnsFailed),
+        matches!(result, Err(ref e) if e.kind() == ErrorKind::Dns),
         "expected Err(DnsFailed), got: {:?}",
         result
     );