From b961409831c273bf258c06c6cf01c04817c4c467 Mon Sep 17 00:00:00 2001 From: Justin Geibel Date: Mon, 25 Nov 2019 23:37:17 -0500 Subject: [PATCH 01/12] Remove some unused fields from ConcreteAppError --- src/util/errors.rs | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/src/util/errors.rs b/src/util/errors.rs index aca5bdef5d3..ec7b88052b6 100644 --- a/src/util/errors.rs +++ b/src/util/errors.rs @@ -198,17 +198,12 @@ impl From for Box { #[derive(Debug)] struct ConcreteAppError { description: String, - detail: Option, - cause: Option>, cargo_err: bool, } impl fmt::Display for ConcreteAppError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "{}", self.description)?; - if let Some(ref s) = self.detail { - write!(f, " ({})", s)?; - } Ok(()) } } @@ -218,7 +213,7 @@ impl AppError for ConcreteAppError { &self.description } fn cause(&self) -> Option<&dyn AppError> { - self.cause.as_ref().map(|c| &**c) + None } fn response(&self) -> Option { self.fallback_response() @@ -306,8 +301,6 @@ impl fmt::Display for BadRequest { pub fn internal(error: &S) -> Box { Box::new(ConcreteAppError { description: error.to_string(), - detail: None, - cause: None, cargo_err: false, }) } @@ -315,8 +308,6 @@ pub fn internal(error: &S) -> Box { pub fn cargo_err(error: &S) -> Box { Box::new(ConcreteAppError { description: error.to_string(), - detail: None, - cause: None, cargo_err: true, }) } From 5b0671d3cb46ad2de57df4652af59df9734c0440 Mon Sep 17 00:00:00 2001 From: Justin Geibel Date: Tue, 26 Nov 2019 19:47:55 -0500 Subject: [PATCH 02/12] Fix HTTP status codes for 2 non-cargo user endpoints --- src/controllers.rs | 12 +++++++++++- src/controllers/user/me.rs | 17 ++++++++--------- src/tests/user.rs | 6 +++--- src/util/errors.rs | 31 ++++++++++++++++++++++++------- src/util/errors/http.rs | 31 +++++++++++++++++++++++++++++++ 5 files changed, 77 insertions(+), 20 deletions(-) create mode 100644 src/util/errors/http.rs diff --git a/src/controllers.rs b/src/controllers.rs index d1437c8f69b..eca318ce86f 100644 --- a/src/controllers.rs +++ b/src/controllers.rs @@ -1,3 +1,13 @@ +mod cargo_prelude { + pub use super::prelude::*; + pub use crate::util::cargo_err; +} + +mod frontend_prelude { + pub use super::prelude::*; + pub use crate::util::errors::{bad_request, server_error}; +} + mod prelude { pub use super::helpers::ok_true; pub use diesel::prelude::*; @@ -6,7 +16,7 @@ mod prelude { pub use conduit_router::RequestParams; pub use crate::db::RequestTransaction; - pub use crate::util::{cargo_err, AppResult}; + pub use crate::util::{cargo_err, AppResult}; // TODO: Remove cargo_err from here pub use crate::middleware::app::RequestApp; pub use crate::middleware::current_user::RequestUser; diff --git a/src/controllers/user/me.rs b/src/controllers/user/me.rs index 65391eb9e45..f6f48681f7f 100644 --- a/src/controllers/user/me.rs +++ b/src/controllers/user/me.rs @@ -1,10 +1,9 @@ use std::collections::HashMap; -use crate::controllers::prelude::*; +use crate::controllers::frontend_prelude::*; use crate::controllers::helpers::*; use crate::email; -use crate::util::bad_request; use crate::util::errors::AppError; use crate::models::{CrateOwner, Email, Follow, NewEmail, OwnerKind, User, Version}; @@ -118,7 +117,7 @@ pub fn update_user(req: &mut dyn Request) -> AppResult { // need to check if current user matches user to be updated if &user.id.to_string() != name { - return Err(cargo_err("current user does not match requested user")); + return Err(bad_request("current user does not match requested user")); } #[derive(Deserialize)] @@ -132,17 +131,17 @@ pub fn update_user(req: &mut dyn Request) -> AppResult { } let user_update: UserUpdate = - serde_json::from_str(&body).map_err(|_| cargo_err("invalid json request"))?; + serde_json::from_str(&body).map_err(|_| bad_request("invalid json request"))?; if user_update.user.email.is_none() { - return Err(cargo_err("empty email rejected")); + return Err(bad_request("empty email rejected")); } let user_email = user_update.user.email.unwrap(); let user_email = user_email.trim(); if user_email == "" { - return Err(cargo_err("empty email rejected")); + return Err(bad_request("empty email rejected")); } conn.transaction::<_, Box, _>(|| { @@ -158,7 +157,7 @@ pub fn update_user(req: &mut dyn Request) -> AppResult { .set(&new_email) .returning(emails::token) .get_result::(&*conn) - .map_err(|_| cargo_err("Error in creating token"))?; + .map_err(|_| server_error("Error in creating token"))?; crate::email::send_user_confirm_email(user_email, &user.gh_login, &token); @@ -197,7 +196,7 @@ pub fn regenerate_token_and_send(req: &mut dyn Request) -> AppResult { // need to check if current user matches user to be updated if &user.id != name { - return Err(cargo_err("current user does not match requested user")); + return Err(bad_request("current user does not match requested user")); } conn.transaction(|| { @@ -207,7 +206,7 @@ pub fn regenerate_token_and_send(req: &mut dyn Request) -> AppResult { .map_err(|_| bad_request("Email could not be found"))?; email::try_send_user_confirm_email(&email.email, &user.gh_login, &email.token) - .map_err(|_| bad_request("Error in sending email")) + .map_err(|_| server_error("Error in sending email")) })?; ok_true() diff --git a/src/tests/user.rs b/src/tests/user.rs index fde2c94a39b..559ab3efc14 100644 --- a/src/tests/user.rs +++ b/src/tests/user.rs @@ -475,7 +475,7 @@ fn test_empty_email_not_added() { let json = user .update_email_more_control(model.id, Some("")) - .bad_with_status(200); + .bad_with_status(400); assert!( json.errors[0].detail.contains("empty email rejected"), "{:?}", @@ -484,7 +484,7 @@ fn test_empty_email_not_added() { let json = user .update_email_more_control(model.id, None) - .bad_with_status(200); + .bad_with_status(400); assert!( json.errors[0].detail.contains("empty email rejected"), @@ -510,7 +510,7 @@ fn test_other_users_cannot_change_my_email() { another_user_model.id, Some("pineapple@pineapples.pineapple"), ) - .bad_with_status(200); + .bad_with_status(400); assert!( json.errors[0] .detail diff --git a/src/util/errors.rs b/src/util/errors.rs index ec7b88052b6..c51b2028c4c 100644 --- a/src/util/errors.rs +++ b/src/util/errors.rs @@ -8,6 +8,30 @@ use diesel::result::Error as DieselError; use crate::util::json_response; +mod http; + +/// Returns an error with status 200 and the provided description as JSON +/// +/// This is for backwards compatibility with cargo endpoints. For all other +/// endpoints, use helpers like `bad_request` or `server_error` which set a +/// correct status code. +pub fn cargo_err(error: &S) -> Box { + Box::new(ConcreteAppError { + description: error.to_string(), + cargo_err: true, + }) +} + +// The following are intended to be used for errors being sent back to the Ember +// frontend, not to cargo as cargo does not handle non-200 response codes well +// (see ), but Ember requires +// non-200 response codes for its stores to work properly. + +/// Returns an error with status 500 and the provided description as JSON +pub fn server_error(error: &S) -> Box { + Box::new(http::ServerError(error.to_string())) +} + #[derive(Serialize)] struct StringError { detail: String, @@ -305,13 +329,6 @@ pub fn internal(error: &S) -> Box { }) } -pub fn cargo_err(error: &S) -> Box { - Box::new(ConcreteAppError { - description: error.to_string(), - cargo_err: true, - }) -} - /// This is intended to be used for errors being sent back to the Ember /// frontend, not to cargo as cargo does not handle non-200 response codes well /// (see ), but Ember requires diff --git a/src/util/errors/http.rs b/src/util/errors/http.rs new file mode 100644 index 00000000000..da2cfef95ca --- /dev/null +++ b/src/util/errors/http.rs @@ -0,0 +1,31 @@ +use std::fmt; + +use conduit::Response; + +use super::{AppError, Bad, StringError}; +use crate::util::json_response; + +#[derive(Debug)] +pub(super) struct ServerError(pub(super) String); + +impl AppError for ServerError { + fn description(&self) -> &str { + self.0.as_ref() + } + + fn response(&self) -> Option { + let mut response = json_response(&Bad { + errors: vec![StringError { + detail: self.0.clone(), + }], + }); + response.status = (500, "Internal Server Error"); + Some(response) + } +} + +impl fmt::Display for ServerError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + self.0.fmt(f) + } +} From 49a568e4290c48baecf3486daa4ae37414f4dbf1 Mon Sep 17 00:00:00 2001 From: Justin Geibel Date: Tue, 26 Nov 2019 19:53:46 -0500 Subject: [PATCH 03/12] Fix HTTP error status from `me/crate_owner_invitations` --- src/controllers/crate_owner_invitation.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/controllers/crate_owner_invitation.rs b/src/controllers/crate_owner_invitation.rs index 1cf8ded7ef4..655946d2351 100644 --- a/src/controllers/crate_owner_invitation.rs +++ b/src/controllers/crate_owner_invitation.rs @@ -1,4 +1,4 @@ -use super::prelude::*; +use super::frontend_prelude::*; use crate::models::{CrateOwner, CrateOwnerInvitation, OwnerKind}; use crate::schema::{crate_owner_invitations, crate_owners}; @@ -38,7 +38,7 @@ pub fn handle_invite(req: &mut dyn Request) -> AppResult { let conn = &*req.db_conn()?; let crate_invite: OwnerInvitation = - serde_json::from_str(&body).map_err(|_| cargo_err("invalid json request"))?; + serde_json::from_str(&body).map_err(|_| bad_request("invalid json request"))?; let crate_invite = crate_invite.crate_owner_invite; From e2c9213cfda22f3b5133b1a9bec86955062087d5 Mon Sep 17 00:00:00 2001 From: Justin Geibel Date: Tue, 26 Nov 2019 22:21:21 -0500 Subject: [PATCH 04/12] Fix HTTP error status for session endpoints --- src/controllers/user/session.rs | 10 ++++++---- src/tests/user.rs | 2 +- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/controllers/user/session.rs b/src/controllers/user/session.rs index 500d18048da..0316f1c9fd7 100644 --- a/src/controllers/user/session.rs +++ b/src/controllers/user/session.rs @@ -1,12 +1,13 @@ -use crate::controllers::prelude::*; +use crate::controllers::frontend_prelude::*; use crate::github; use conduit_cookie::RequestSession; +use failure::Fail; use oauth2::{prelude::*, AuthorizationCode, TokenResponse}; use crate::models::{NewUser, User}; use crate::schema::users; -use crate::util::errors::{AppError, ReadOnlyMode}; +use crate::util::errors::{AppError, ChainError, ReadOnlyMode}; /// Handles the `GET /api/private/session/begin` route. /// @@ -83,7 +84,7 @@ pub fn authorize(req: &mut dyn Request) -> AppResult { let session_state = req.session().remove(&"github_oauth_state".to_string()); let session_state = session_state.as_ref().map(|a| &a[..]); if Some(&state[..]) != session_state { - return Err(cargo_err("invalid state parameter")); + return Err(bad_request("invalid state parameter")); } } @@ -94,7 +95,8 @@ pub fn authorize(req: &mut dyn Request) -> AppResult { .app() .github .exchange_code(code) - .map_err(|s| cargo_err(&s))?; + .map_err(|e| e.compat()) + .chain_error(|| server_error("Error obtaining token"))?; let token = token.access_token(); let ghuser = github::github_api::(req.app(), "/user", token)?; let user = ghuser.save_to_database(&token.secret(), &*req.db_conn()?)?; diff --git a/src/tests/user.rs b/src/tests/user.rs index 559ab3efc14..756f6d2447f 100644 --- a/src/tests/user.rs +++ b/src/tests/user.rs @@ -115,7 +115,7 @@ fn access_token_needs_data() { let (_, anon) = TestApp::init().empty(); let json = anon .get::<()>("/api/private/session/authorize") - .bad_with_status(200); // Change endpoint to 400? + .bad_with_status(400); assert!(json.errors[0].detail.contains("invalid state")); } From fa4d87ba3c1087ee7f40fccc446364ad4812f4e5 Mon Sep 17 00:00:00 2001 From: Justin Geibel Date: Tue, 26 Nov 2019 22:56:25 -0500 Subject: [PATCH 05/12] Import either cargo_prelude or frontend_prelude in controlers This serves as a guard rail to help avoid using the wrong error response method. Two files still reference the generic prelude, becuase those files contain mixed endpoints and there is no reason to do a larger refactor at this time. --- src/controllers/krate/downloads.rs | 2 +- src/controllers/krate/follow.rs | 2 +- src/controllers/krate/metadata.rs | 2 +- src/controllers/krate/publish.rs | 2 +- src/controllers/krate/search.rs | 2 +- src/controllers/team.rs | 2 +- src/controllers/user/other.rs | 2 +- src/controllers/version/deprecated.rs | 2 +- src/controllers/version/metadata.rs | 2 +- src/controllers/version/yank.rs | 2 +- 10 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/controllers/krate/downloads.rs b/src/controllers/krate/downloads.rs index 155d7126746..7dc95fd22f7 100644 --- a/src/controllers/krate/downloads.rs +++ b/src/controllers/krate/downloads.rs @@ -5,7 +5,7 @@ use std::cmp; -use crate::controllers::prelude::*; +use crate::controllers::frontend_prelude::*; use crate::models::{Crate, CrateVersions, Version, VersionDownload}; use crate::schema::version_downloads; diff --git a/src/controllers/krate/follow.rs b/src/controllers/krate/follow.rs index d0e86df18a1..93bd2097aa2 100644 --- a/src/controllers/krate/follow.rs +++ b/src/controllers/krate/follow.rs @@ -2,7 +2,7 @@ use diesel::associations::Identifiable; -use crate::controllers::prelude::*; +use crate::controllers::frontend_prelude::*; use crate::db::DieselPooledConn; use crate::models::{Crate, Follow}; use crate::schema::*; diff --git a/src/controllers/krate/metadata.rs b/src/controllers/krate/metadata.rs index 9e02718f8c3..0390038d198 100644 --- a/src/controllers/krate/metadata.rs +++ b/src/controllers/krate/metadata.rs @@ -4,7 +4,7 @@ //! index or cached metadata which was extracted (client side) from the //! `Cargo.toml` file. -use crate::controllers::prelude::*; +use crate::controllers::frontend_prelude::*; use crate::models::{ Category, Crate, CrateCategory, CrateKeyword, CrateVersions, Keyword, RecentCrateDownloads, User, Version, diff --git a/src/controllers/krate/publish.rs b/src/controllers/krate/publish.rs index 527d3588bf7..09dce9934aa 100644 --- a/src/controllers/krate/publish.rs +++ b/src/controllers/krate/publish.rs @@ -4,7 +4,7 @@ use hex::ToHex; use std::sync::Arc; use swirl::Job; -use crate::controllers::prelude::*; +use crate::controllers::cargo_prelude::*; use crate::git; use crate::models::dependency; use crate::models::{ diff --git a/src/controllers/krate/search.rs b/src/controllers/krate/search.rs index 50aaa6c27ee..2ad799bd4ce 100644 --- a/src/controllers/krate/search.rs +++ b/src/controllers/krate/search.rs @@ -3,8 +3,8 @@ use diesel::dsl::*; use diesel_full_text_search::*; +use crate::controllers::cargo_prelude::*; use crate::controllers::helpers::Paginate; -use crate::controllers::prelude::*; use crate::models::{Crate, CrateBadge, CrateOwner, CrateVersions, OwnerKind, Version}; use crate::schema::*; use crate::views::EncodableCrate; diff --git a/src/controllers/team.rs b/src/controllers/team.rs index f62b9821e0e..a0a98b4a224 100644 --- a/src/controllers/team.rs +++ b/src/controllers/team.rs @@ -1,4 +1,4 @@ -use crate::controllers::prelude::*; +use crate::controllers::frontend_prelude::*; use crate::models::Team; use crate::schema::teams; diff --git a/src/controllers/user/other.rs b/src/controllers/user/other.rs index b2e9cb598d5..bb3a4ce225d 100644 --- a/src/controllers/user/other.rs +++ b/src/controllers/user/other.rs @@ -1,4 +1,4 @@ -use crate::controllers::prelude::*; +use crate::controllers::frontend_prelude::*; use crate::models::{OwnerKind, User}; use crate::schema::{crate_owners, crates, users}; diff --git a/src/controllers/version/deprecated.rs b/src/controllers/version/deprecated.rs index 9246a9b8a1f..e9667150765 100644 --- a/src/controllers/version/deprecated.rs +++ b/src/controllers/version/deprecated.rs @@ -5,7 +5,7 @@ //! period of time to ensure there are no external users of an endpoint before //! it is removed. -use crate::controllers::prelude::*; +use crate::controllers::frontend_prelude::*; use crate::models::{Crate, User, Version}; use crate::schema::*; diff --git a/src/controllers/version/metadata.rs b/src/controllers/version/metadata.rs index 9b985944a4f..0212bba5c8a 100644 --- a/src/controllers/version/metadata.rs +++ b/src/controllers/version/metadata.rs @@ -4,7 +4,7 @@ //! index or cached metadata which was extracted (client side) from the //! `Cargo.toml` file. -use crate::controllers::prelude::*; +use crate::controllers::frontend_prelude::*; use crate::schema::*; use crate::views::{EncodableDependency, EncodablePublicUser, EncodableVersion}; diff --git a/src/controllers/version/yank.rs b/src/controllers/version/yank.rs index e67d65f4666..9262678eaaa 100644 --- a/src/controllers/version/yank.rs +++ b/src/controllers/version/yank.rs @@ -3,7 +3,7 @@ use swirl::Job; use super::version_and_crate; -use crate::controllers::prelude::*; +use crate::controllers::cargo_prelude::*; use crate::git; use crate::models::{insert_version_owner_action, Rights, VersionAction}; use crate::util::AppError; From c84368aa4c7d052b602e50d0fab6b6f223f3837a Mon Sep 17 00:00:00 2001 From: Justin Geibel Date: Wed, 27 Nov 2019 19:06:52 -0500 Subject: [PATCH 06/12] Simplify implementation of `cargo_err` --- src/util/errors.rs | 13 ++++--------- src/util/errors/http.rs | 22 ++++++++++++++++++++++ 2 files changed, 26 insertions(+), 9 deletions(-) diff --git a/src/util/errors.rs b/src/util/errors.rs index c51b2028c4c..a8d5b9be9f1 100644 --- a/src/util/errors.rs +++ b/src/util/errors.rs @@ -16,10 +16,7 @@ mod http; /// endpoints, use helpers like `bad_request` or `server_error` which set a /// correct status code. pub fn cargo_err(error: &S) -> Box { - Box::new(ConcreteAppError { - description: error.to_string(), - cargo_err: true, - }) + Box::new(http::Ok(error.to_string())) } // The following are intended to be used for errors being sent back to the Ember @@ -51,6 +48,9 @@ pub trait AppError: Send + fmt::Display + fmt::Debug + 'static { } /// Generate an HTTP response for the error + /// + /// If none is returned, the error will bubble up the middleware stack + /// where it is eventually logged and turned into a status 500 response. fn response(&self) -> Option; /// Fallback logic for generating a cargo friendly response @@ -222,7 +222,6 @@ impl From for Box { #[derive(Debug)] struct ConcreteAppError { description: String, - cargo_err: bool, } impl fmt::Display for ConcreteAppError { @@ -242,9 +241,6 @@ impl AppError for ConcreteAppError { fn response(&self) -> Option { self.fallback_response() } - fn fallback_with_description_as_bad_200(&self) -> bool { - self.cargo_err - } } #[derive(Debug, Clone, Copy)] @@ -325,7 +321,6 @@ impl fmt::Display for BadRequest { pub fn internal(error: &S) -> Box { Box::new(ConcreteAppError { description: error.to_string(), - cargo_err: false, }) } diff --git a/src/util/errors/http.rs b/src/util/errors/http.rs index da2cfef95ca..92114fa9170 100644 --- a/src/util/errors/http.rs +++ b/src/util/errors/http.rs @@ -5,9 +5,31 @@ use conduit::Response; use super::{AppError, Bad, StringError}; use crate::util::json_response; +#[derive(Debug)] +pub(super) struct Ok(pub(super) String); #[derive(Debug)] pub(super) struct ServerError(pub(super) String); +impl AppError for Ok { + fn description(&self) -> &str { + self.0.as_ref() + } + + fn response(&self) -> Option { + Some(json_response(&Bad { + errors: vec![StringError { + detail: self.0.clone(), + }], + })) + } +} + +impl fmt::Display for Ok { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + self.0.fmt(f) + } +} + impl AppError for ServerError { fn description(&self) -> &str { self.0.as_ref() From f2009aad5ba88f6bd2f36ce00b17be7bd72b4f6f Mon Sep 17 00:00:00 2001 From: Justin Geibel Date: Wed, 27 Nov 2019 19:13:55 -0500 Subject: [PATCH 07/12] Drop description and fallback logic that is no longer used by `cargo_err` --- src/util/errors.rs | 77 +++-------------------------------------- src/util/errors/http.rs | 8 ----- 2 files changed, 4 insertions(+), 81 deletions(-) diff --git a/src/util/errors.rs b/src/util/errors.rs index a8d5b9be9f1..04326da55ab 100644 --- a/src/util/errors.rs +++ b/src/util/errors.rs @@ -42,7 +42,6 @@ struct Bad { // AppError trait pub trait AppError: Send + fmt::Display + fmt::Debug + 'static { - fn description(&self) -> &str; fn cause(&self) -> Option<&(dyn AppError)> { None } @@ -53,30 +52,6 @@ pub trait AppError: Send + fmt::Display + fmt::Debug + 'static { /// where it is eventually logged and turned into a status 500 response. fn response(&self) -> Option; - /// Fallback logic for generating a cargo friendly response - /// - /// This behavior is deprecated and no new calls or impls should be added. - fn fallback_response(&self) -> Option { - if self.fallback_with_description_as_bad_200() { - Some(json_response(&Bad { - errors: vec![StringError { - detail: self.description().to_string(), - }], - })) - } else { - self.cause().and_then(AppError::response) - } - } - - /// Determines if the `fallback_response` method should send the description as a status 200 - /// error to cargo, or send the cause response (if applicable). - /// - /// This is only to be used by the `fallback_response` method. If your error type impls - /// `response`, then there is no need to impl this method. - fn fallback_with_description_as_bad_200(&self) -> bool { - false - } - fn get_type_id(&self) -> TypeId { TypeId::of::() } @@ -105,15 +80,9 @@ impl dyn AppError { } impl AppError for Box { - fn description(&self) -> &str { - (**self).description() - } fn cause(&self) -> Option<&dyn AppError> { (**self).cause() } - fn fallback_with_description_as_bad_200(&self) -> bool { - (**self).fallback_with_description_as_bad_200() - } fn response(&self) -> Option { (**self).response() } @@ -179,18 +148,12 @@ impl ChainError for Option { } impl AppError for ChainedError { - fn description(&self) -> &str { - self.error.description() - } fn cause(&self) -> Option<&dyn AppError> { Some(&*self.cause) } fn response(&self) -> Option { self.error.response() } - fn fallback_with_description_as_bad_200(&self) -> bool { - self.error.fallback_with_description_as_bad_200() - } } impl fmt::Display for ChainedError { @@ -203,11 +166,8 @@ impl fmt::Display for ChainedError { // Error impls impl AppError for E { - fn description(&self) -> &str { - Error::description(self) - } fn response(&self) -> Option { - self.fallback_response() + None } } @@ -219,6 +179,7 @@ impl From for Box { // ============================================================================= // Concrete errors +// TODO: Rename to InternalAppError #[derive(Debug)] struct ConcreteAppError { description: String, @@ -232,14 +193,8 @@ impl fmt::Display for ConcreteAppError { } impl AppError for ConcreteAppError { - fn description(&self) -> &str { - &self.description - } - fn cause(&self) -> Option<&dyn AppError> { - None - } fn response(&self) -> Option { - self.fallback_response() + None } } @@ -247,10 +202,6 @@ impl AppError for ConcreteAppError { pub struct NotFound; impl AppError for NotFound { - fn description(&self) -> &str { - "not found" - } - fn response(&self) -> Option { let mut response = json_response(&Bad { errors: vec![StringError { @@ -272,10 +223,6 @@ impl fmt::Display for NotFound { pub struct Unauthorized; impl AppError for Unauthorized { - fn description(&self) -> &str { - "unauthorized" - } - fn response(&self) -> Option { let mut response = json_response(&Bad { errors: vec![StringError { @@ -297,10 +244,6 @@ impl fmt::Display for Unauthorized { struct BadRequest(String); impl AppError for BadRequest { - fn description(&self) -> &str { - self.0.as_ref() - } - fn response(&self) -> Option { let mut response = json_response(&Bad { errors: vec![StringError { @@ -338,11 +281,7 @@ pub fn bad_request(error: &S) -> Box { #[derive(Debug)] pub struct AppErrToStdErr(pub Box); -impl Error for AppErrToStdErr { - fn description(&self) -> &str { - self.0.description() - } -} +impl Error for AppErrToStdErr {} impl fmt::Display for AppErrToStdErr { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { @@ -370,10 +309,6 @@ pub(crate) fn std_error_no_send(e: Box) -> Box { pub struct ReadOnlyMode; impl AppError for ReadOnlyMode { - fn description(&self) -> &str { - "tried to write in read only mode" - } - fn response(&self) -> Option { let mut response = json_response(&Bad { errors: vec![StringError { @@ -399,10 +334,6 @@ pub struct TooManyRequests { } impl AppError for TooManyRequests { - fn description(&self) -> &str { - "too many requests" - } - fn response(&self) -> Option { const HTTP_DATE_FORMAT: &str = "%a, %d %b %Y %H:%M:%S GMT"; let retry_after = self.retry_after.format(HTTP_DATE_FORMAT); diff --git a/src/util/errors/http.rs b/src/util/errors/http.rs index 92114fa9170..2ee59fe15cc 100644 --- a/src/util/errors/http.rs +++ b/src/util/errors/http.rs @@ -11,10 +11,6 @@ pub(super) struct Ok(pub(super) String); pub(super) struct ServerError(pub(super) String); impl AppError for Ok { - fn description(&self) -> &str { - self.0.as_ref() - } - fn response(&self) -> Option { Some(json_response(&Bad { errors: vec![StringError { @@ -31,10 +27,6 @@ impl fmt::Display for Ok { } impl AppError for ServerError { - fn description(&self) -> &str { - self.0.as_ref() - } - fn response(&self) -> Option { let mut response = json_response(&Bad { errors: vec![StringError { From cb9e99242053835d570c1d97fb41307e7adcf362 Mon Sep 17 00:00:00 2001 From: Justin Geibel Date: Wed, 27 Nov 2019 19:17:26 -0500 Subject: [PATCH 08/12] Rename `ConcreteAppError` to `InternalAppError` --- src/util/errors.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/util/errors.rs b/src/util/errors.rs index 04326da55ab..a10fe21fad5 100644 --- a/src/util/errors.rs +++ b/src/util/errors.rs @@ -179,20 +179,19 @@ impl From for Box { // ============================================================================= // Concrete errors -// TODO: Rename to InternalAppError #[derive(Debug)] -struct ConcreteAppError { +struct InternalAppError { description: String, } -impl fmt::Display for ConcreteAppError { +impl fmt::Display for InternalAppError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "{}", self.description)?; Ok(()) } } -impl AppError for ConcreteAppError { +impl AppError for InternalAppError { fn response(&self) -> Option { None } @@ -262,7 +261,7 @@ impl fmt::Display for BadRequest { } pub fn internal(error: &S) -> Box { - Box::new(ConcreteAppError { + Box::new(InternalAppError { description: error.to_string(), }) } From 9d8a6bb7d41945fa3b38e40cf91b568a3e596ca1 Mon Sep 17 00:00:00 2001 From: Justin Geibel Date: Wed, 27 Nov 2019 20:49:09 -0500 Subject: [PATCH 09/12] Switch logging to use `to_string` on errors --- src/middleware/log_request.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/middleware/log_request.rs b/src/middleware/log_request.rs index ff1631adc5c..2b31527f199 100644 --- a/src/middleware/log_request.rs +++ b/src/middleware/log_request.rs @@ -53,7 +53,7 @@ impl Handler for LogRequests { ); if let Err(ref e) = res { - print!(" error=\"{}\"", e.description()); + print!(" error=\"{}\"", e.to_string()); } if response_time > 1000 { From a5a451e15fe1503d29becf768291fa3a76298f3c Mon Sep 17 00:00:00 2001 From: Justin Geibel Date: Wed, 27 Nov 2019 22:44:29 -0500 Subject: [PATCH 10/12] Remove duplicate error "caused by" logging The `impl fmt::Display for ChainedError` is sufficient. --- src/util/errors.rs | 20 +------------------- 1 file changed, 1 insertion(+), 19 deletions(-) diff --git a/src/util/errors.rs b/src/util/errors.rs index a10fe21fad5..e7e0e6bd4bc 100644 --- a/src/util/errors.rs +++ b/src/util/errors.rs @@ -42,10 +42,6 @@ struct Bad { // AppError trait pub trait AppError: Send + fmt::Display + fmt::Debug + 'static { - fn cause(&self) -> Option<&(dyn AppError)> { - None - } - /// Generate an HTTP response for the error /// /// If none is returned, the error will bubble up the middleware stack @@ -80,9 +76,6 @@ impl dyn AppError { } impl AppError for Box { - fn cause(&self) -> Option<&dyn AppError> { - (**self).cause() - } fn response(&self) -> Option { (**self).response() } @@ -148,9 +141,6 @@ impl ChainError for Option { } impl AppError for ChainedError { - fn cause(&self) -> Option<&dyn AppError> { - Some(&*self.cause) - } fn response(&self) -> Option { self.error.response() } @@ -284,15 +274,7 @@ impl Error for AppErrToStdErr {} impl fmt::Display for AppErrToStdErr { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "{}", self.0)?; - - let mut err = &*self.0; - while let Some(cause) = err.cause() { - err = cause; - write!(f, "\nCaused by: {}", err)?; - } - - Ok(()) + self.0.fmt(f) } } From e86de9b661304e70101060f7cc2851086332b9f3 Mon Sep 17 00:00:00 2001 From: Justin Geibel Date: Mon, 16 Dec 2019 22:07:16 -0500 Subject: [PATCH 11/12] Add test for `chain_error` used with `internal` --- src/util/errors.rs | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/util/errors.rs b/src/util/errors.rs index e7e0e6bd4bc..734b05461d3 100644 --- a/src/util/errors.rs +++ b/src/util/errors.rs @@ -342,3 +342,23 @@ impl fmt::Display for TooManyRequests { "Too many requests".fmt(f) } } + +#[test] +fn chain_error_internal() { + assert_eq!( + None::<()> + .chain_error(|| internal("inner")) + .chain_error(|| internal("middle")) + .chain_error(|| internal("outer")) + .unwrap_err() + .to_string(), + "outer caused by middle caused by inner" + ); + assert_eq!( + Err::<(), _>(internal("inner")) + .chain_error(|| internal("outer")) + .unwrap_err() + .to_string(), + "outer caused by inner" + ); +} From 2f1e697a55d6fadaa9e7950595c81fdf305a7274 Mon Sep 17 00:00:00 2001 From: Justin Geibel Date: Mon, 16 Dec 2019 22:44:36 -0500 Subject: [PATCH 12/12] Add a few more tests for `chain_err` --- src/util/errors.rs | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/src/util/errors.rs b/src/util/errors.rs index 734b05461d3..5e9da0c9318 100644 --- a/src/util/errors.rs +++ b/src/util/errors.rs @@ -361,4 +361,42 @@ fn chain_error_internal() { .to_string(), "outer caused by inner" ); + + // 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")) + .unwrap_err() + .to_string(), + "outer caused by inner" + ); + assert_eq!( + Err::<(), _>(Unauthorized) + .chain_error(|| internal("outer")) + .unwrap_err() + .to_string(), + "outer caused by must be logged in to perform that action" + ); +} + +#[test] +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")) + .unwrap_err() + .to_string(), + "outer caused by inner" // never logged + ); + + // The outer error is sent as a response to the client. + // 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")) + .unwrap_err() + .to_string(), + "outer caused by permission denied" // never logged + ); }