Auto merge of #3901 - Turbo87:chain-error, r=hi-rustin

Remove `ChainError` trait

The `chain_error()` function can be replaced by `ok_or_else()` (for `Option`) or `map_err()` (for `Result`), which makes the intent a little clearer, and removes one of the layers of indirection from our error handling, without being significantly more verbose.

Please note that this only removes the `ChainError` **trait**, but the `ChainedError` **struct** still exists.

Author: bors

src/controllers.rs

@@ -19,7 +19,7 @@ mod prelude {
 
     pub use crate::db::RequestTransaction;
     pub use crate::middleware::app::RequestApp;
-    pub use crate::util::errors::{cargo_err, AppError, AppResult, ChainError}; // TODO: Remove cargo_err from here
+    pub use crate::util::errors::{cargo_err, AppError, AppResult}; // TODO: Remove cargo_err from here
     pub use crate::util::{AppResponse, EndpointResult};
 
     use indexmap::IndexMap;

src/controllers/krate/publish.rs

@@ -130,7 +130,7 @@ pub fn publish(req: &mut dyn RequestExt) -> EndpointResult {
 
         let content_length = req
             .content_length()
-            .chain_error(|| cargo_err("missing header: Content-Length"))?;
+            .ok_or_else(|| cargo_err("missing header: Content-Length"))?;
 
         let maximums = Maximums::new(
             krate.max_upload_size,

src/controllers/krate/search.rs

@@ -10,7 +10,7 @@ use crate::models::{
     Crate, CrateBadge, CrateOwner, CrateVersions, OwnerKind, TopVersions, Version,
 };
 use crate::schema::*;
-use crate::util::errors::{bad_request, ChainError};
+use crate::util::errors::bad_request;
 use crate::views::EncodableCrate;
 
 use crate::controllers::helpers::pagination::{Page, Paginated, PaginationOptions};
@@ -153,7 +153,7 @@ pub fn search(req: &mut dyn RequestExt) -> EndpointResult {
             letter
                 .chars()
                 .next()
-                .chain_error(|| bad_request("letter value must contain 1 character"))?
+                .ok_or_else(|| bad_request("letter value must contain 1 character"))?
                 .to_lowercase()
                 .collect::<String>()
         );

src/controllers/token.rs

@@ -39,7 +39,7 @@ pub fn new(req: &mut dyn RequestExt) -> EndpointResult {
     let max_size = 2000;
     let length = req
         .content_length()
-        .chain_error(|| bad_request("missing header: Content-Length"))?;
+        .ok_or_else(|| bad_request("missing header: Content-Length"))?;
 
     if length > max_size {
         return Err(bad_request(&format!("max content length is: {}", max_size)));

src/controllers/user/me.rs

@@ -187,7 +187,7 @@ pub fn regenerate_token_and_send(req: &mut dyn RequestExt) -> EndpointResult {
 
     let param_user_id = req.params()["user_id"]
         .parse::<i32>()
-        .chain_error(|| bad_request("invalid user_id"))?;
+        .map_err(|err| err.chain(bad_request("invalid user_id")))?;
     let authenticated_user = req.authenticate()?;
     let conn = req.db_conn()?;
     let user = authenticated_user.user();

src/controllers/user/other.rs

@@ -2,7 +2,6 @@ use crate::controllers::frontend_prelude::*;
 
 use crate::models::{CrateOwner, OwnerKind, User};
 use crate::schema::{crate_owners, crates, users};
-use crate::util::errors::ChainError;
 use crate::views::EncodablePublicUser;
 
 /// Handles the `GET /users/:user_id` route.
@@ -25,7 +24,7 @@ pub fn stats(req: &mut dyn RequestExt) -> EndpointResult {
 
     let user_id = &req.params()["user_id"]
         .parse::<i32>()
-        .chain_error(|| bad_request("invalid user_id"))?;
+        .map_err(|err| err.chain(bad_request("invalid user_id")))?;
     let conn = req.db_conn()?;
 
     let data: i64 = CrateOwner::by_owner_kind(OwnerKind::User)

src/controllers/user/session.rs

@@ -90,7 +90,7 @@ pub fn authorize(req: &mut dyn RequestExt) -> EndpointResult {
         .github_oauth
         .exchange_code(code)
         .request(http_client)
-        .chain_error(|| server_error("Error obtaining token"))?;
+        .map_err(|err| err.chain(server_error("Error obtaining token")))?;
     let token = token.access_token();
 
     // Fetch the user info from GitHub using the access token we just got and create a user record

src/controllers/util.rs

@@ -6,8 +6,7 @@ use super::prelude::*;
 use crate::middleware::log_request;
 use crate::models::{ApiToken, User};
 use crate::util::errors::{
-    account_locked, forbidden, internal, AppError, AppResult, ChainError,
-    InsecurelyGeneratedTokenRevoked,
+    account_locked, forbidden, internal, AppError, AppResult, InsecurelyGeneratedTokenRevoked,
 };
 
 #[derive(Debug)]
@@ -62,8 +61,7 @@ fn verify_origin(req: &dyn RequestExt) -> AppResult<()> {
             "only same-origin requests can be authenticated. got {:?}",
             bad_origin
         );
-        return Err(internal(&error_message))
-            .chain_error(|| Box::new(forbidden()) as Box<dyn AppError>);
+        return Err(internal(&error_message).chain(forbidden()));
     }
     Ok(())
 }
@@ -76,7 +74,7 @@ fn authenticate_user(req: &dyn RequestExt) -> AppResult<AuthenticatedUser> {
 
     if let Some(id) = user_id_from_session {
         let user = User::find(&conn, id)
-            .chain_error(|| internal("user_id from cookie not found in database"))?;
+            .map_err(|err| err.chain(internal("user_id from cookie not found in database")))?;
 
         return Ok(AuthenticatedUser {
             user,
@@ -100,7 +98,7 @@ fn authenticate_user(req: &dyn RequestExt) -> AppResult<AuthenticatedUser> {
         })?;
 
         let user = User::find(&conn, token.user_id)
-            .chain_error(|| internal("user_id from token not found in database"))?;
+            .map_err(|err| err.chain(internal("user_id from token not found in database")))?;
 
         return Ok(AuthenticatedUser {
             user,
@@ -109,7 +107,7 @@ fn authenticate_user(req: &dyn RequestExt) -> AppResult<AuthenticatedUser> {
     }
 
     // Unable to authenticate the user
-    return Err(internal("no cookie session or auth header found")).chain_error(forbidden);
+    return Err(internal("no cookie session or auth header found").chain(forbidden()));
 }
 
 impl<'a> UserAuthenticationExt for dyn RequestExt + 'a {

src/router.rs

@@ -179,9 +179,7 @@ impl<H: Handler> Handler for R<H> {
 #[cfg(test)]
 mod tests {
     use super::*;
-    use crate::util::errors::{
-        bad_request, cargo_err, forbidden, internal, not_found, AppError, ChainError,
-    };
+    use crate::util::errors::{bad_request, cargo_err, forbidden, internal, not_found, AppError};
     use crate::util::EndpointResult;
 
     use conduit::StatusCode;
@@ -227,8 +225,8 @@ mod tests {
         let response = C(|_| {
             Err("-1"
                 .parse::<u8>()
-                .chain_error(|| internal("middle error"))
-                .chain_error(|| bad_request("outer user facing error"))
+                .map_err(|err| err.chain(internal("middle error")))
+                .map_err(|err| err.chain(bad_request("outer user facing error")))
                 .unwrap_err())
         })
         .call(&mut req)

src/uploaders.rs

@@ -4,7 +4,7 @@ use flate2::read::GzDecoder;
 use reqwest::{blocking::Client, header};
 use sha2::{Digest, Sha256};
 
-use crate::util::errors::{cargo_err, internal, AppResult, ChainError};
+use crate::util::errors::{cargo_err, internal, AppError, AppResult};
 use crate::util::{LimitErrorReader, Maximums};
 
 use std::env;
@@ -200,8 +200,10 @@ fn verify_tarball(
     let mut archive = tar::Archive::new(decoder);
     let prefix = format!("{}-{}", krate.name, vers);
     for entry in archive.entries()? {
-        let entry = entry.chain_error(|| {
-            cargo_err("uploaded tarball is malformed or too large when decompressed")
+        let entry = entry.map_err(|err| {
+            err.chain(cargo_err(
+                "uploaded tarball is malformed or too large when decompressed",
+            ))
         })?;
 
         // Verify that all entries actually start with `$name-$vers/`.

src/util/errors.rs

@@ -152,42 +152,12 @@ pub type AppResult<T> = Result<T, Box<dyn AppError>>;
 // =============================================================================
 // Chaining errors
 
-pub trait ChainError<T> {
-    fn chain_error<E, F>(self, callback: F) -> AppResult<T>
-    where
-        E: AppError,
-        F: FnOnce() -> E;
-}
-
 #[derive(Debug)]
 struct ChainedError<E> {
     error: E,
     cause: Box<dyn AppError>,
 }
 
-impl<T, E: AppError> ChainError<T> for Result<T, E> {
-    fn chain_error<E2, C>(self, callback: C) -> AppResult<T>
-    where
-        E2: AppError,
-        C: FnOnce() -> E2,
-    {
-        self.map_err(move |err| err.chain(callback()))
-    }
-}
-
-impl<T> ChainError<T> for Option<T> {
-    fn chain_error<E, C>(self, callback: C) -> AppResult<T>
-    where
-        E: AppError,
-        C: FnOnce() -> E,
-    {
-        match self {
-            Some(t) => Ok(t),
-            None => Err(Box::new(callback())),
-        }
-    }
-}
-
 impl<E: AppError> AppError for ChainedError<E> {
     fn response(&self) -> Option<AppResponse> {
         self.error.response()
@@ -274,17 +244,16 @@ pub(crate) fn std_error(e: Box<dyn AppError>) -> Box<dyn Error + Send> {
 #[test]
 fn chain_error_internal() {
     assert_eq!(
-        None::<()>
-            .chain_error(|| internal("inner"))
-            .chain_error(|| internal("middle"))
-            .chain_error(|| internal("outer"))
+        Err::<(), _>(internal("inner"))
+            .map_err(|err| err.chain(internal("middle")))
+            .map_err(|err| err.chain(internal("outer")))
             .unwrap_err()
             .to_string(),
         "outer caused by middle caused by inner"
     );
     assert_eq!(
         Err::<(), _>(internal("inner"))
-            .chain_error(|| internal("outer"))
+            .map_err(|err| err.chain(internal("outer")))
             .unwrap_err()
             .to_string(),
         "outer caused by inner"
@@ -293,14 +262,14 @@ fn chain_error_internal() {
     // Don't do this, the user will see a generic 500 error instead of the intended message
     assert_eq!(
         Err::<(), _>(cargo_err("inner"))
-            .chain_error(|| internal("outer"))
+            .map_err(|err| err.chain(internal("outer")))
             .unwrap_err()
             .to_string(),
         "outer caused by inner"
     );
     assert_eq!(
         Err::<(), _>(forbidden())
-            .chain_error(|| internal("outer"))
+            .map_err(|err| err.chain(internal("outer")))
             .unwrap_err()
             .to_string(),
         "outer caused by must be logged in to perform that action"
@@ -312,7 +281,7 @@ fn chain_error_user_facing() {
     // Do this rarely, the user will only see the outer error
     assert_eq!(
         Err::<(), _>(cargo_err("inner"))
-            .chain_error(|| cargo_err("outer"))
+            .map_err(|err| err.chain(cargo_err("outer")))
             .unwrap_err()
             .to_string(),
         "outer caused by inner" // never logged
@@ -322,7 +291,7 @@ fn chain_error_user_facing() {
     // The inner error never bubbles up to the logging middleware
     assert_eq!(
         Err::<(), _>(std::io::Error::from(std::io::ErrorKind::PermissionDenied))
-            .chain_error(|| cargo_err("outer"))
+            .map_err(|err| err.chain(cargo_err("outer")))
             .unwrap_err()
             .to_string(),
         "outer caused by permission denied" // never logged