From 7bcd0b335e447ec50b6bc9719a0837575d6516ba Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Wed, 8 Sep 2021 08:00:14 +0200 Subject: [PATCH 1/2] errors: Remove `ChainError` impl for `Option` For `Option` there is nothing to chain onto, so we might as well use the built-in `ok_or_else()` method to convert the `Option` to a `Result` --- src/controllers/krate/publish.rs | 2 +- src/controllers/krate/search.rs | 4 ++-- src/controllers/token.rs | 2 +- src/util/errors.rs | 16 +--------------- 4 files changed, 5 insertions(+), 19 deletions(-) diff --git a/src/controllers/krate/publish.rs b/src/controllers/krate/publish.rs index 81acef4d65a..9af0e61569b 100644 --- a/src/controllers/krate/publish.rs +++ b/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, diff --git a/src/controllers/krate/search.rs b/src/controllers/krate/search.rs index de966f658b3..d1af65110d2 100644 --- a/src/controllers/krate/search.rs +++ b/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::() ); diff --git a/src/controllers/token.rs b/src/controllers/token.rs index b845f633aaa..536edc5b27c 100644 --- a/src/controllers/token.rs +++ b/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))); diff --git a/src/util/errors.rs b/src/util/errors.rs index 3d2cf729869..7dc193ed50a 100644 --- a/src/util/errors.rs +++ b/src/util/errors.rs @@ -175,19 +175,6 @@ impl ChainError for Result { } } -impl ChainError for Option { - fn chain_error(self, callback: C) -> AppResult - where - E: AppError, - C: FnOnce() -> E, - { - match self { - Some(t) => Ok(t), - None => Err(Box::new(callback())), - } - } -} - impl AppError for ChainedError { fn response(&self) -> Option { self.error.response() @@ -274,8 +261,7 @@ pub(crate) fn std_error(e: Box) -> Box { #[test] fn chain_error_internal() { assert_eq!( - None::<()> - .chain_error(|| internal("inner")) + Err::<(), _>(internal("inner")) .chain_error(|| internal("middle")) .chain_error(|| internal("outer")) .unwrap_err() From 9e40742fa7caf69651fdfc0e04242bed02ab7870 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Wed, 8 Sep 2021 07:55:22 +0200 Subject: [PATCH 2/2] errors: Inline `chain_error()` method Using `map_err()` instead is just a few characters more, but makes it more clear how the original error is chained onto the new error --- src/controllers.rs | 2 +- src/controllers/user/me.rs | 2 +- src/controllers/user/other.rs | 3 +-- src/controllers/user/session.rs | 2 +- src/controllers/util.rs | 12 +++++------- src/router.rs | 8 +++----- src/uploaders.rs | 8 +++++--- src/util/errors.rs | 31 +++++++------------------------ 8 files changed, 24 insertions(+), 44 deletions(-) diff --git a/src/controllers.rs b/src/controllers.rs index d5b9c349eb3..4dae7b59055 100644 --- a/src/controllers.rs +++ b/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; diff --git a/src/controllers/user/me.rs b/src/controllers/user/me.rs index 7a1ef8b8bb8..8c72b6cbf80 100644 --- a/src/controllers/user/me.rs +++ b/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::() - .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(); diff --git a/src/controllers/user/other.rs b/src/controllers/user/other.rs index e80b5724b7c..72d51a449d3 100644 --- a/src/controllers/user/other.rs +++ b/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::() - .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) diff --git a/src/controllers/user/session.rs b/src/controllers/user/session.rs index d4f181976ce..93e1c50995f 100644 --- a/src/controllers/user/session.rs +++ b/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 diff --git a/src/controllers/util.rs b/src/controllers/util.rs index 90597864bd9..693733bb5db 100644 --- a/src/controllers/util.rs +++ b/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); + return Err(internal(&error_message).chain(forbidden())); } Ok(()) } @@ -76,7 +74,7 @@ fn authenticate_user(req: &dyn RequestExt) -> AppResult { 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 { })?; 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 { } // 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 { diff --git a/src/router.rs b/src/router.rs index f50411a1037..af67cec32b1 100644 --- a/src/router.rs +++ b/src/router.rs @@ -179,9 +179,7 @@ impl Handler for R { #[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::() - .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) diff --git a/src/uploaders.rs b/src/uploaders.rs index b705a70ca9f..8c728508a7f 100644 --- a/src/uploaders.rs +++ b/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/`. diff --git a/src/util/errors.rs b/src/util/errors.rs index 7dc193ed50a..255d20e96e9 100644 --- a/src/util/errors.rs +++ b/src/util/errors.rs @@ -152,29 +152,12 @@ pub type AppResult = Result>; // ============================================================================= // Chaining errors -pub trait ChainError { - fn chain_error(self, callback: F) -> AppResult - where - E: AppError, - F: FnOnce() -> E; -} - #[derive(Debug)] struct ChainedError { error: E, cause: Box, } -impl ChainError for Result { - fn chain_error(self, callback: C) -> AppResult - where - E2: AppError, - C: FnOnce() -> E2, - { - self.map_err(move |err| err.chain(callback())) - } -} - impl AppError for ChainedError { fn response(&self) -> Option { self.error.response() @@ -262,15 +245,15 @@ pub(crate) fn std_error(e: Box) -> Box { fn chain_error_internal() { assert_eq!( Err::<(), _>(internal("inner")) - .chain_error(|| internal("middle")) - .chain_error(|| internal("outer")) + .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" @@ -279,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" @@ -298,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 @@ -308,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