diff --git a/src/background_jobs.rs b/src/background_jobs.rs index 45b50d8d05b..e4c01d1d9a3 100644 --- a/src/background_jobs.rs +++ b/src/background_jobs.rs @@ -1,21 +1,22 @@ use std::panic::AssertUnwindSafe; use std::sync::{Arc, Mutex, MutexGuard, PoisonError}; + +use diesel::r2d2::PoolError; use swirl::PerformError; use crate::db::{DieselPool, DieselPooledConn}; use crate::git::Repository; use crate::uploaders::Uploader; -use crate::util::errors::{AppErrToStdErr, AppResult}; impl<'a> swirl::db::BorrowedConnection<'a> for DieselPool { type Connection = DieselPooledConn<'a>; } impl swirl::db::DieselPool for DieselPool { - type Error = AppErrToStdErr; + type Error = PoolError; fn get(&self) -> Result, Self::Error> { - self.get().map_err(AppErrToStdErr) + self.get() } } @@ -56,13 +57,11 @@ impl Environment { } } - pub fn connection(&self) -> Result, PerformError> { - self.connection_pool - .get() - .map_err(|e| AppErrToStdErr(e).into()) + pub fn connection(&self) -> Result, PoolError> { + self.connection_pool.get() } - pub fn lock_index(&self) -> AppResult> { + pub fn lock_index(&self) -> Result, PerformError> { let repo = self.index.lock().unwrap_or_else(PoisonError::into_inner); repo.reset_head()?; Ok(repo) diff --git a/src/bin/enqueue-job.rs b/src/bin/enqueue-job.rs index 568efc72811..876b9d1c5a9 100644 --- a/src/bin/enqueue-job.rs +++ b/src/bin/enqueue-job.rs @@ -1,8 +1,9 @@ -use cargo_registry::util::{cargo_err, AppError, AppResult}; -use cargo_registry::{db, env, tasks}; -use diesel::PgConnection; +#![deny(clippy::all)] -fn main() -> AppResult<()> { +use cargo_registry::{db, env, tasks, util::Error}; +use swirl::Job; + +fn main() -> Result<(), Error> { let conn = db::connect_now()?; let mut args = std::env::args().skip(1); @@ -10,24 +11,14 @@ fn main() -> AppResult<()> { println!("Enqueueing background job: {}", job); match &*job { - "update_downloads" => tasks::update_downloads().enqueue(&conn), + "update_downloads" => Ok(tasks::update_downloads().enqueue(&conn)?), "dump_db" => { let database_url = args.next().unwrap_or_else(|| env("DATABASE_URL")); let target_name = args .next() .unwrap_or_else(|| String::from("db-dump.tar.gz")); - tasks::dump_db(database_url, target_name).enqueue(&conn) + Ok(tasks::dump_db(database_url, target_name).enqueue(&conn)?) } - other => Err(cargo_err(&format!("Unrecognized job type `{}`", other))), + other => Err(Error::from(format!("Unrecognized job type `{}`", other))), } } - -/// Helper to map the `PerformError` returned by `swirl::Job::enqueue()` to a -/// `AppError`. Can be removed once `map_err()` isn't needed any more. -trait Enqueue: swirl::Job { - fn enqueue(self, conn: &PgConnection) -> AppResult<()> { - ::enqueue(self, conn).map_err(|e| AppError::from_std_error(e)) - } -} - -impl Enqueue for J {} diff --git a/src/bin/monitor.rs b/src/bin/monitor.rs index cfc5d6231d4..431c9084570 100644 --- a/src/bin/monitor.rs +++ b/src/bin/monitor.rs @@ -8,10 +8,10 @@ mod on_call; -use cargo_registry::{db, schema::*, util::AppResult}; +use cargo_registry::{db, schema::*, util::Error}; use diesel::prelude::*; -fn main() -> AppResult<()> { +fn main() -> Result<(), Error> { let conn = db::connect_now()?; check_stalled_background_jobs(&conn)?; @@ -19,7 +19,7 @@ fn main() -> AppResult<()> { Ok(()) } -fn check_stalled_background_jobs(conn: &PgConnection) -> AppResult<()> { +fn check_stalled_background_jobs(conn: &PgConnection) -> Result<(), Error> { use cargo_registry::schema::background_jobs::dsl::*; use diesel::dsl::*; @@ -55,7 +55,7 @@ fn check_stalled_background_jobs(conn: &PgConnection) -> AppResult<()> { Ok(()) } -fn check_spam_attack(conn: &PgConnection) -> AppResult<()> { +fn check_spam_attack(conn: &PgConnection) -> Result<(), Error> { use cargo_registry::models::krate::canon_crate_name; use diesel::dsl::*; use diesel::sql_types::Bool; @@ -116,7 +116,7 @@ fn check_spam_attack(conn: &PgConnection) -> AppResult<()> { Ok(()) } -fn log_and_trigger_event(event: on_call::Event) -> AppResult<()> { +fn log_and_trigger_event(event: on_call::Event) -> Result<(), Error> { match event { on_call::Event::Trigger { ref description, .. diff --git a/src/bin/on_call/mod.rs b/src/bin/on_call/mod.rs index 991dfe03bba..7e31f783f76 100644 --- a/src/bin/on_call/mod.rs +++ b/src/bin/on_call/mod.rs @@ -1,4 +1,4 @@ -use cargo_registry::util::{internal, AppResult}; +use cargo_registry::util::Error; use reqwest::{header, StatusCode as Status}; @@ -25,7 +25,7 @@ impl Event { /// /// If the variant is `Trigger`, this will page whoever is on call /// (potentially waking them up at 3 AM). - pub fn send(self) -> AppResult<()> { + pub fn send(self) -> Result<(), Error> { let api_token = dotenv::var("PAGERDUTY_API_TOKEN")?; let service_key = dotenv::var("PAGERDUTY_INTEGRATION_KEY")?; @@ -43,14 +43,15 @@ impl Event { s if s.is_success() => Ok(()), Status::BAD_REQUEST => { let error = response.json::()?; - Err(internal(&format_args!("pagerduty error: {:?}", error))) + Err(format!("pagerduty error: {:?}", error)) } - Status::FORBIDDEN => Err(internal("rate limited by pagerduty")), - n => Err(internal(&format_args!( + Status::FORBIDDEN => Err("rate limited by pagerduty".to_string()), + n => Err(format!( "Got a non 200 response code from pagerduty: {} with {:?}", n, response - ))), + )), } + .map_err(Into::into) } } diff --git a/src/bin/test-pagerduty.rs b/src/bin/test-pagerduty.rs index 88fda8f4fd8..de7f0d9ce1d 100644 --- a/src/bin/test-pagerduty.rs +++ b/src/bin/test-pagerduty.rs @@ -11,7 +11,9 @@ mod on_call; use std::env::args; -fn main() { +use cargo_registry::util::Error; + +fn main() -> Result<(), Error> { let args = args().collect::>(); let event_type = &*args[1]; @@ -32,5 +34,5 @@ fn main() { }, _ => panic!("Event type must be trigger, acknowledge, or resolve"), }; - event.send().unwrap() + event.send() } diff --git a/src/boot/categories.rs b/src/boot/categories.rs index 861707594f0..40014ac344a 100644 --- a/src/boot/categories.rs +++ b/src/boot/categories.rs @@ -1,10 +1,7 @@ // Sync available crate categories from `src/categories.toml`. // Runs when the server is started. -use crate::{ - db, - util::errors::{internal, AppResult, ChainError}, -}; +use crate::{db, util::Error}; use diesel::prelude::*; @@ -37,9 +34,12 @@ impl Category { } } -fn required_string_from_toml<'a>(toml: &'a toml::value::Table, key: &str) -> AppResult<&'a str> { - toml.get(key).and_then(toml::Value::as_str).chain_error(|| { - internal(&format_args!( +fn required_string_from_toml<'a>( + toml: &'a toml::value::Table, + key: &str, +) -> Result<&'a str, Error> { + toml.get(key).and_then(toml::Value::as_str).ok_or_else(|| { + Error::from(format!( "Expected category TOML attribute '{}' to be a String", key )) @@ -53,13 +53,13 @@ fn optional_string_from_toml<'a>(toml: &'a toml::value::Table, key: &str) -> &'a fn categories_from_toml( categories: &toml::value::Table, parent: Option<&Category>, -) -> AppResult> { +) -> Result, Error> { let mut result = vec![]; for (slug, details) in categories { let details = details .as_table() - .chain_error(|| internal(&format_args!("category {} was not a TOML table", slug)))?; + .ok_or_else(|| Error::from(format!("category {} was not a TOML table", slug)))?; let category = Category::from_parent( slug, @@ -69,12 +69,9 @@ fn categories_from_toml( ); if let Some(categories) = details.get("categories") { - let categories = categories.as_table().chain_error(|| { - internal(&format_args!( - "child categories of {} were not a table", - slug - )) - })?; + let categories = categories + .as_table() + .ok_or_else(|| format!("child categories of {} were not a table", slug))?; result.extend(categories_from_toml(categories, Some(&category))?); } @@ -85,12 +82,12 @@ fn categories_from_toml( Ok(result) } -pub fn sync(toml_str: &str) -> AppResult<()> { +pub fn sync(toml_str: &str) -> Result<(), Error> { let conn = db::connect_now().unwrap(); sync_with_connection(toml_str, &conn) } -pub fn sync_with_connection(toml_str: &str, conn: &PgConnection) -> AppResult<()> { +pub fn sync_with_connection(toml_str: &str, conn: &PgConnection) -> Result<(), Error> { use crate::schema::categories::dsl::*; use diesel::dsl::all; use diesel::pg::upsert::excluded; diff --git a/src/controllers.rs b/src/controllers.rs index eca318ce86f..afa66152e3d 100644 --- a/src/controllers.rs +++ b/src/controllers.rs @@ -1,6 +1,6 @@ mod cargo_prelude { pub use super::prelude::*; - pub use crate::util::cargo_err; + pub use crate::util::errors::cargo_err; } mod frontend_prelude { @@ -16,7 +16,7 @@ mod prelude { pub use conduit_router::RequestParams; pub use crate::db::RequestTransaction; - pub use crate::util::{cargo_err, AppResult}; // TODO: Remove cargo_err from here + pub use crate::util::errors::{cargo_err, AppError, AppResult, ChainError}; // TODO: Remove cargo_err from here pub use crate::middleware::app::RequestApp; pub use crate::middleware::current_user::RequestUser; diff --git a/src/controllers/helpers.rs b/src/controllers/helpers.rs index 056fc944daf..4f7053eaf08 100644 --- a/src/controllers/helpers.rs +++ b/src/controllers/helpers.rs @@ -1,4 +1,4 @@ -use crate::util::{json_response, AppResult}; +use crate::util::{errors::AppResult, json_response}; use conduit::Response; pub(crate) mod pagination; diff --git a/src/controllers/helpers/pagination.rs b/src/controllers/helpers/pagination.rs index 8454d40c25a..9fd7a4a7a92 100644 --- a/src/controllers/helpers/pagination.rs +++ b/src/controllers/helpers/pagination.rs @@ -1,5 +1,5 @@ use crate::models::helpers::with_count::*; -use crate::util::errors::*; +use crate::util::errors::{cargo_err, AppResult}; use diesel::pg::Pg; use diesel::prelude::*; use diesel::query_builder::*; diff --git a/src/controllers/krate/publish.rs b/src/controllers/krate/publish.rs index b3d93a9f672..2f686cb762f 100644 --- a/src/controllers/krate/publish.rs +++ b/src/controllers/krate/publish.rs @@ -13,8 +13,7 @@ use crate::models::{ }; use crate::render; -use crate::util::{read_fill, read_le_u32}; -use crate::util::{AppError, ChainError, Maximums}; +use crate::util::{read_fill, read_le_u32, Maximums}; use crate::views::{EncodableCrateUpload, GoodCrate, PublishWarnings}; /// Handles the `PUT /crates/new` route. diff --git a/src/controllers/token.rs b/src/controllers/token.rs index 67fce78c340..dcda68e0d6a 100644 --- a/src/controllers/token.rs +++ b/src/controllers/token.rs @@ -1,9 +1,9 @@ -use super::prelude::*; +use super::frontend_prelude::*; use crate::middleware::current_user::AuthenticationSource; use crate::models::ApiToken; use crate::schema::api_tokens; -use crate::util::{bad_request, read_fill, ChainError}; +use crate::util::read_fill; use crate::views::EncodableApiTokenWithToken; use serde_json as json; diff --git a/src/controllers/user/me.rs b/src/controllers/user/me.rs index f0bc1ad4d40..8fbb84086a0 100644 --- a/src/controllers/user/me.rs +++ b/src/controllers/user/me.rs @@ -4,7 +4,6 @@ use crate::controllers::frontend_prelude::*; use crate::controllers::helpers::*; use crate::email; -use crate::util::errors::{AppError, ChainError}; use crate::models::{ CrateOwner, Email, Follow, NewEmail, OwnerKind, User, Version, VersionOwnerAction, diff --git a/src/controllers/user/session.rs b/src/controllers/user/session.rs index 0316f1c9fd7..9527f5c4e7c 100644 --- a/src/controllers/user/session.rs +++ b/src/controllers/user/session.rs @@ -7,7 +7,7 @@ use oauth2::{prelude::*, AuthorizationCode, TokenResponse}; use crate::models::{NewUser, User}; use crate::schema::users; -use crate::util::errors::{AppError, ChainError, ReadOnlyMode}; +use crate::util::errors::ReadOnlyMode; /// Handles the `GET /api/private/session/begin` route. /// diff --git a/src/controllers/version/yank.rs b/src/controllers/version/yank.rs index 9262678eaaa..0d6c8e44721 100644 --- a/src/controllers/version/yank.rs +++ b/src/controllers/version/yank.rs @@ -5,8 +5,8 @@ use swirl::Job; use super::version_and_crate; use crate::controllers::cargo_prelude::*; use crate::git; -use crate::models::{insert_version_owner_action, Rights, VersionAction}; -use crate::util::AppError; +use crate::models::Rights; +use crate::models::{insert_version_owner_action, VersionAction}; /// Handles the `DELETE /crates/:crate_id/:version/yank` route. /// This does not delete a crate version, it makes the crate diff --git a/src/db.rs b/src/db.rs index 7ece465f3f0..7fb56a81737 100644 --- a/src/db.rs +++ b/src/db.rs @@ -7,7 +7,6 @@ use std::sync::Arc; use url::Url; use crate::middleware::app::RequestApp; -use crate::util::AppResult; use crate::Env; #[allow(missing_debug_implementations)] @@ -18,7 +17,7 @@ pub enum DieselPool { } impl DieselPool { - pub fn get(&self) -> AppResult> { + pub fn get(&self) -> Result, r2d2::PoolError> { match self { DieselPool::Pool(pool) => Ok(DieselPooledConn::Pool(pool.get()?)), DieselPool::Test(conn) => Ok(DieselPooledConn::Test(conn.lock())), @@ -89,12 +88,12 @@ pub trait RequestTransaction { /// /// The connection will live for the lifetime of the request. // FIXME: This description does not match the implementation below. - fn db_conn(&self) -> AppResult>; + fn db_conn(&self) -> Result, r2d2::PoolError>; } impl RequestTransaction for T { - fn db_conn(&self) -> AppResult> { - self.app().diesel_database.get().map_err(Into::into) + fn db_conn(&self) -> Result, r2d2::PoolError> { + self.app().diesel_database.get() } } diff --git a/src/email.rs b/src/email.rs index d4f6af7f605..ab688100256 100644 --- a/src/email.rs +++ b/src/email.rs @@ -1,6 +1,6 @@ use std::path::Path; -use crate::util::{bad_request, AppResult}; +use crate::util::errors::{server_error, AppResult}; use failure::Fail; use lettre::file::FileTransport; @@ -118,12 +118,12 @@ fn send_email(recipient: &str, subject: &str, body: &str) -> AppResult<()> { .transport(); let result = transport.send(email); - result.map_err(|_| bad_request("Error in sending email"))?; + result.map_err(|_| server_error("Error in sending email"))?; } None => { let mut sender = FileTransport::new(Path::new("/tmp")); let result = sender.send(email); - result.map_err(|_| bad_request("Email file could not be generated"))?; + result.map_err(|_| server_error("Email file could not be generated"))?; } } diff --git a/src/git.rs b/src/git.rs index e5bd05da724..d2d289b1cbd 100644 --- a/src/git.rs +++ b/src/git.rs @@ -3,14 +3,13 @@ use std::collections::HashMap; use std::fs::{self, OpenOptions}; use std::path::{Path, PathBuf}; -use swirl::errors::PerformError; +use swirl::PerformError; use tempdir::TempDir; use url::Url; use crate::background_jobs::Environment; use crate::models::{DependencyKind, Version}; use crate::schema::versions; -use crate::util::errors::{std_error_no_send, AppResult}; static DEFAULT_GIT_SSH_USERNAME: &str = "git"; @@ -148,7 +147,7 @@ pub struct Repository { } impl Repository { - pub fn open(repository_config: &RepositoryConfig) -> AppResult { + pub fn open(repository_config: &RepositoryConfig) -> Result { let checkout_path = TempDir::new("git")?; let repository = git2::build::RepoBuilder::new() @@ -224,7 +223,7 @@ impl Repository { ref_status } - pub fn reset_head(&self) -> AppResult<()> { + pub fn reset_head(&self) -> Result<(), PerformError> { let mut origin = self.repository.find_remote("origin")?; origin.fetch( &["refs/heads/*:refs/heads/*"], @@ -252,7 +251,7 @@ impl Repository { pub fn add_crate(env: &Environment, krate: Crate) -> Result<(), PerformError> { use std::io::prelude::*; - let repo = env.lock_index().map_err(std_error_no_send)?; + let repo = env.lock_index()?; let dst = repo.index_file(&krate.name); // Add the crate to its relevant file @@ -280,7 +279,7 @@ pub fn yank( ) -> Result<(), PerformError> { use diesel::prelude::*; - let repo = env.lock_index().map_err(std_error_no_send)?; + let repo = env.lock_index()?; let dst = repo.index_file(&krate); let conn = env.connection()?; diff --git a/src/github.rs b/src/github.rs index 86bc321c611..35e7a7e6574 100644 --- a/src/github.rs +++ b/src/github.rs @@ -8,7 +8,7 @@ use serde::de::DeserializeOwned; use std::str; use crate::app::App; -use crate::util::{cargo_err, errors::NotFound, internal, AppError, AppResult}; +use crate::util::errors::{cargo_err, internal, AppError, AppResult, NotFound}; /// Does all the nonsense for sending a GET to Github. Doesn't handle parsing /// because custom error-code handling may be desirable. Use diff --git a/src/middleware.rs b/src/middleware.rs index db840654670..d9fb144974d 100644 --- a/src/middleware.rs +++ b/src/middleware.rs @@ -1,17 +1,22 @@ mod prelude { pub use conduit::{Handler, Request, Response}; pub use conduit_middleware::{AroundMiddleware, Middleware}; - pub use std::error::Error; + + use std::error::Error; + pub type BoxError = Box; + pub type Result = std::result::Result; } -pub use self::app::AppMiddleware; -pub use self::current_user::CurrentUser; -pub use self::debug::*; -pub use self::ember_index_rewrite::EmberIndexRewrite; -pub use self::head::Head; +pub use prelude::Result; + +use self::app::AppMiddleware; +use self::current_user::CurrentUser; +use self::debug::*; +use self::ember_index_rewrite::EmberIndexRewrite; +use self::head::Head; use self::log_connection_pool_status::LogConnectionPoolStatus; -pub use self::security_headers::SecurityHeaders; -pub use self::static_or_continue::StaticOrContinue; +use self::security_headers::SecurityHeaders; +use self::static_or_continue::StaticOrContinue; pub mod app; mod block_traffic; diff --git a/src/middleware/app.rs b/src/middleware/app.rs index 3f60b7f2791..cf18cc56f8e 100644 --- a/src/middleware/app.rs +++ b/src/middleware/app.rs @@ -17,16 +17,12 @@ impl AppMiddleware { } impl Middleware for AppMiddleware { - fn before(&self, req: &mut dyn Request) -> Result<(), Box> { + fn before(&self, req: &mut dyn Request) -> Result<()> { req.mut_extensions().insert(Arc::clone(&self.app)); Ok(()) } - fn after( - &self, - req: &mut dyn Request, - res: Result>, - ) -> Result> { + fn after(&self, req: &mut dyn Request, res: Result) -> Result { req.mut_extensions().pop::>().unwrap(); res } diff --git a/src/middleware/block_traffic.rs b/src/middleware/block_traffic.rs index 690799dae67..770509c1f7c 100644 --- a/src/middleware/block_traffic.rs +++ b/src/middleware/block_traffic.rs @@ -39,7 +39,7 @@ impl AroundMiddleware for BlockTraffic { } impl Handler for BlockTraffic { - fn call(&self, req: &mut dyn Request) -> Result> { + fn call(&self, req: &mut dyn Request) -> Result { let has_blocked_value = req .headers() .find(&self.header_name) diff --git a/src/middleware/current_user.rs b/src/middleware/current_user.rs index f5e29e3efb6..af82852baf3 100644 --- a/src/middleware/current_user.rs +++ b/src/middleware/current_user.rs @@ -4,7 +4,7 @@ use conduit_cookie::RequestSession; use diesel::prelude::*; use crate::db::RequestTransaction; -use crate::util::errors::{std_error, AppResult, ChainError, Unauthorized}; +use crate::util::errors::{AppResult, ChainError, Unauthorized}; use crate::models::ApiToken; use crate::models::User; @@ -20,7 +20,7 @@ pub enum AuthenticationSource { } impl Middleware for CurrentUser { - fn before(&self, req: &mut dyn Request) -> Result<(), Box> { + fn before(&self, req: &mut dyn Request) -> Result<()> { // Check if the request has a session cookie with a `user_id` property inside let id = { req.session() @@ -28,7 +28,7 @@ impl Middleware for CurrentUser { .and_then(|s| s.parse::().ok()) }; - let conn = req.db_conn().map_err(std_error)?; + let conn = req.db_conn().map_err(|e| Box::new(e) as BoxError)?; if let Some(id) = id { // If it did, look for a user in the database with the given `user_id` @@ -56,7 +56,7 @@ impl Middleware for CurrentUser { }) }) .optional() - .map_err(|e| Box::new(e) as Box)? + .map_err(|e| Box::new(e) as BoxError)? } else { None }; diff --git a/src/middleware/debug.rs b/src/middleware/debug.rs index 819efc1d858..a99432fde2b 100644 --- a/src/middleware/debug.rs +++ b/src/middleware/debug.rs @@ -6,15 +6,11 @@ use super::prelude::*; pub struct Debug; impl Middleware for Debug { - fn before(&self, req: &mut dyn Request) -> Result<(), Box> { + fn before(&self, req: &mut dyn Request) -> Result<()> { DebugRequest.before(req) } - fn after( - &self, - _req: &mut dyn Request, - res: Result>, - ) -> Result> { + fn after(&self, _req: &mut dyn Request, res: Result) -> Result { res.map(|res| { println!(" <- {:?}", res.status); for (k, v) in &res.headers { @@ -29,7 +25,7 @@ impl Middleware for Debug { pub struct DebugRequest; impl Middleware for DebugRequest { - fn before(&self, req: &mut dyn Request) -> Result<(), Box> { + fn before(&self, req: &mut dyn Request) -> Result<()> { println!(" version: {}", req.http_version()); println!(" method: {:?}", req.method()); println!(" scheme: {:?}", req.scheme()); diff --git a/src/middleware/ember_index_rewrite.rs b/src/middleware/ember_index_rewrite.rs index 0b6117ce81d..db7868d6328 100644 --- a/src/middleware/ember_index_rewrite.rs +++ b/src/middleware/ember_index_rewrite.rs @@ -24,7 +24,7 @@ impl AroundMiddleware for EmberIndexRewrite { } impl Handler for EmberIndexRewrite { - fn call(&self, req: &mut dyn Request) -> Result> { + fn call(&self, req: &mut dyn Request) -> Result { // If the client is requesting html, then we've only got one page so // rewrite the request. let wants_html = req diff --git a/src/middleware/ensure_well_formed_500.rs b/src/middleware/ensure_well_formed_500.rs index 5b3b4771d85..87b94345dbe 100644 --- a/src/middleware/ensure_well_formed_500.rs +++ b/src/middleware/ensure_well_formed_500.rs @@ -10,11 +10,7 @@ use std::collections::HashMap; pub struct EnsureWellFormed500; impl Middleware for EnsureWellFormed500 { - fn after( - &self, - _: &mut dyn Request, - res: Result>, - ) -> Result> { + fn after(&self, _: &mut dyn Request, res: Result) -> Result { res.or_else(|_| { let body = "Internal Server Error"; let mut headers = HashMap::new(); diff --git a/src/middleware/head.rs b/src/middleware/head.rs index 9b9d1aa46c8..063f52239f3 100644 --- a/src/middleware/head.rs +++ b/src/middleware/head.rs @@ -20,7 +20,7 @@ impl AroundMiddleware for Head { } impl Handler for Head { - fn call(&self, req: &mut dyn Request) -> Result> { + fn call(&self, req: &mut dyn Request) -> Result { if req.method() == Method::Head { let mut req = RequestProxy::rewrite_method(req, Method::Get); self.handler diff --git a/src/middleware/log_connection_pool_status.rs b/src/middleware/log_connection_pool_status.rs index 9952fe17917..aab25309064 100644 --- a/src/middleware/log_connection_pool_status.rs +++ b/src/middleware/log_connection_pool_status.rs @@ -28,7 +28,7 @@ impl LogConnectionPoolStatus { } impl Middleware for LogConnectionPoolStatus { - fn before(&self, _: &mut dyn Request) -> Result<(), Box> { + fn before(&self, _: &mut dyn Request) -> Result<()> { let mut last_log_time = self .last_log_time .lock() @@ -45,11 +45,7 @@ impl Middleware for LogConnectionPoolStatus { Ok(()) } - fn after( - &self, - _: &mut dyn Request, - res: Result>, - ) -> Result> { + fn after(&self, _: &mut dyn Request, res: Result) -> Result { self.in_flight_requests.fetch_sub(1, Ordering::SeqCst); res } diff --git a/src/middleware/log_request.rs b/src/middleware/log_request.rs index b6097554fe5..f01f8935c6f 100644 --- a/src/middleware/log_request.rs +++ b/src/middleware/log_request.rs @@ -20,7 +20,7 @@ impl AroundMiddleware for LogRequests { } impl Handler for LogRequests { - fn call(&self, req: &mut dyn Request) -> Result> { + fn call(&self, req: &mut dyn Request) -> Result { let request_start = Instant::now(); let res = self.handler.as_ref().unwrap().call(req); let (level, response_code) = match res { diff --git a/src/middleware/require_user_agent.rs b/src/middleware/require_user_agent.rs index 8a24067e28c..cec098001f2 100644 --- a/src/middleware/require_user_agent.rs +++ b/src/middleware/require_user_agent.rs @@ -20,7 +20,7 @@ impl AroundMiddleware for RequireUserAgent { } impl Handler for RequireUserAgent { - fn call(&self, req: &mut dyn Request) -> Result> { + fn call(&self, req: &mut dyn Request) -> Result { let has_user_agent = request_header(req, "User-Agent") != ""; let is_download = req.path().ends_with("download"); if !has_user_agent && !is_download { diff --git a/src/middleware/security_headers.rs b/src/middleware/security_headers.rs index 98a60c94abb..1641c3bbce5 100644 --- a/src/middleware/security_headers.rs +++ b/src/middleware/security_headers.rs @@ -57,11 +57,7 @@ impl SecurityHeaders { } impl Middleware for SecurityHeaders { - fn after( - &self, - _: &mut dyn Request, - mut res: Result>, - ) -> Result> { + fn after(&self, _: &mut dyn Request, mut res: Result) -> Result { if let Ok(ref mut response) = res { response.headers.extend(self.headers.clone()); } diff --git a/src/middleware/static_or_continue.rs b/src/middleware/static_or_continue.rs index 2407538f424..478b7f6af85 100644 --- a/src/middleware/static_or_continue.rs +++ b/src/middleware/static_or_continue.rs @@ -27,7 +27,7 @@ impl AroundMiddleware for StaticOrContinue { } impl Handler for StaticOrContinue { - fn call(&self, req: &mut dyn Request) -> Result> { + fn call(&self, req: &mut dyn Request) -> Result { match self.static_handler.call(req) { Ok(ref resp) if resp.status.0 == 404 => {} ret => return ret, diff --git a/src/models/dependency.rs b/src/models/dependency.rs index bab9d921bff..dfb2257d1f5 100644 --- a/src/models/dependency.rs +++ b/src/models/dependency.rs @@ -1,7 +1,7 @@ use diesel::prelude::*; use crate::git; -use crate::util::{cargo_err, AppResult}; +use crate::util::errors::{cargo_err, AppResult}; use crate::models::{Crate, Version}; use crate::schema::*; diff --git a/src/models/krate.rs b/src/models/krate.rs index 46d035a4dae..8e3b6a36277 100644 --- a/src/models/krate.rs +++ b/src/models/krate.rs @@ -13,7 +13,7 @@ use crate::models::{ Badge, Category, CrateOwner, CrateOwnerInvitation, Keyword, NewCrateOwnerInvitation, Owner, OwnerKind, ReverseDependency, User, Version, }; -use crate::util::{cargo_err, AppResult}; +use crate::util::errors::{cargo_err, AppResult}; use crate::views::{EncodableCrate, EncodableCrateLinks}; use crate::models::helpers::with_count::*; @@ -387,7 +387,7 @@ impl Crate { /// Return both the newest (most recently updated) and /// highest version (in semver order) for the current crate. - pub fn top_versions(&self, conn: &PgConnection) -> AppResult { + pub fn top_versions(&self, conn: &PgConnection) -> QueryResult { use crate::schema::versions::dsl::*; Ok(Version::top( @@ -397,7 +397,7 @@ impl Crate { )) } - pub fn owners(&self, conn: &PgConnection) -> AppResult> { + pub fn owners(&self, conn: &PgConnection) -> QueryResult> { let users = CrateOwner::by_owner_kind(OwnerKind::User) .filter(crate_owners::crate_id.eq(self.id)) .inner_join(users::table) diff --git a/src/models/owner.rs b/src/models/owner.rs index 2d6a2ac3412..88d6ec0fb6a 100644 --- a/src/models/owner.rs +++ b/src/models/owner.rs @@ -3,7 +3,7 @@ use diesel::prelude::*; use crate::app::App; use crate::github; -use crate::util::{cargo_err, AppResult}; +use crate::util::errors::{cargo_err, AppResult}; use crate::models::{Crate, Team, User}; use crate::schema::{crate_owners, users}; diff --git a/src/models/team.rs b/src/models/team.rs index 0baf95cf504..994d14f94c4 100644 --- a/src/models/team.rs +++ b/src/models/team.rs @@ -2,7 +2,7 @@ use diesel::prelude::*; use crate::app::App; use crate::github::{github_api, team_url}; -use crate::util::{cargo_err, errors::NotFound, AppResult}; +use crate::util::errors::{cargo_err, AppResult, NotFound}; use oauth2::{prelude::*, AccessToken}; @@ -178,7 +178,7 @@ impl Team { team_with_gh_id_contains_user(app, self.github_id, user) } - pub fn owning(krate: &Crate, conn: &PgConnection) -> AppResult> { + pub fn owning(krate: &Crate, conn: &PgConnection) -> QueryResult> { let base_query = CrateOwner::belonging_to(krate).filter(crate_owners::deleted.eq(false)); let teams = base_query .inner_join(teams::table) diff --git a/src/models/user.rs b/src/models/user.rs index c3be5158335..d3f1dd3e381 100644 --- a/src/models/user.rs +++ b/src/models/user.rs @@ -2,7 +2,7 @@ use diesel::prelude::*; use std::borrow::Cow; use crate::app::App; -use crate::util::AppResult; +use crate::util::errors::AppResult; use crate::models::{ApiToken, Crate, CrateOwner, Email, NewEmail, Owner, OwnerKind, Rights}; use crate::schema::{crate_owners, emails, users}; @@ -116,7 +116,7 @@ impl User { Self::find(conn, api_token.user_id) } - pub fn owning(krate: &Crate, conn: &PgConnection) -> AppResult> { + pub fn owning(krate: &Crate, conn: &PgConnection) -> QueryResult> { let users = CrateOwner::by_owner_kind(OwnerKind::User) .inner_join(users::table) .select(users::all_columns) @@ -157,7 +157,7 @@ impl User { /// Queries the database for the verified emails /// belonging to a given user - pub fn verified_email(&self, conn: &PgConnection) -> AppResult> { + pub fn verified_email(&self, conn: &PgConnection) -> QueryResult> { Ok(Email::belonging_to(self) .select(emails::email) .filter(emails::verified.eq(true)) diff --git a/src/models/version.rs b/src/models/version.rs index 54345f746ff..5e0b7e22da1 100644 --- a/src/models/version.rs +++ b/src/models/version.rs @@ -3,7 +3,7 @@ use std::collections::HashMap; use chrono::NaiveDateTime; use diesel::prelude::*; -use crate::util::{cargo_err, AppResult}; +use crate::util::errors::{cargo_err, AppResult}; use crate::models::{Crate, Dependency, User, VersionOwnerAction}; use crate::schema::*; diff --git a/src/publish_rate_limit.rs b/src/publish_rate_limit.rs index c2912e55545..da825bfc805 100644 --- a/src/publish_rate_limit.rs +++ b/src/publish_rate_limit.rs @@ -55,7 +55,7 @@ impl PublishRateLimit { uploader: i32, now: NaiveDateTime, conn: &PgConnection, - ) -> AppResult { + ) -> QueryResult { use self::publish_limit_buckets::dsl::*; use diesel::sql_types::{Double, Interval, Text, Timestamp}; @@ -93,7 +93,6 @@ impl PublishRateLimit { .eq(last_refill + self.refill_rate().into_sql::() * tokens_to_add), )) .get_result(conn) - .map_err(Into::into) } fn refill_rate(&self) -> PgInterval { @@ -108,7 +107,7 @@ mod tests { use crate::test_util::*; #[test] - fn take_token_with_no_bucket_creates_new_one() -> AppResult<()> { + fn take_token_with_no_bucket_creates_new_one() -> QueryResult<()> { let conn = pg_connection(); let now = now(); @@ -139,7 +138,7 @@ mod tests { } #[test] - fn take_token_with_existing_bucket_modifies_existing_bucket() -> AppResult<()> { + fn take_token_with_existing_bucket_modifies_existing_bucket() -> QueryResult<()> { let conn = pg_connection(); let now = now(); @@ -159,7 +158,7 @@ mod tests { } #[test] - fn take_token_after_delay_refills() -> AppResult<()> { + fn take_token_after_delay_refills() -> QueryResult<()> { let conn = pg_connection(); let now = now(); @@ -180,12 +179,13 @@ mod tests { } #[test] - fn refill_subsecond_rate() -> AppResult<()> { + fn refill_subsecond_rate() -> QueryResult<()> { let conn = pg_connection(); // Subsecond rates have floating point rounding issues, so use a known // timestamp that rounds fine let now = - NaiveDateTime::parse_from_str("2019-03-19T21:11:24.620401", "%Y-%m-%dT%H:%M:%S%.f")?; + NaiveDateTime::parse_from_str("2019-03-19T21:11:24.620401", "%Y-%m-%dT%H:%M:%S%.f") + .unwrap(); let rate = PublishRateLimit { rate: Duration::from_millis(100), @@ -204,7 +204,7 @@ mod tests { } #[test] - fn last_refill_always_advanced_by_multiple_of_rate() -> AppResult<()> { + fn last_refill_always_advanced_by_multiple_of_rate() -> QueryResult<()> { let conn = pg_connection(); let now = now(); @@ -225,7 +225,7 @@ mod tests { } #[test] - fn zero_tokens_returned_when_user_has_no_tokens_left() -> AppResult<()> { + fn zero_tokens_returned_when_user_has_no_tokens_left() -> QueryResult<()> { let conn = pg_connection(); let now = now(); @@ -248,7 +248,7 @@ mod tests { } #[test] - fn a_user_with_no_tokens_gets_a_token_after_exactly_rate() -> AppResult<()> { + fn a_user_with_no_tokens_gets_a_token_after_exactly_rate() -> QueryResult<()> { let conn = pg_connection(); let now = now(); @@ -270,7 +270,7 @@ mod tests { } #[test] - fn tokens_never_refill_past_burst() -> AppResult<()> { + fn tokens_never_refill_past_burst() -> QueryResult<()> { let conn = pg_connection(); let now = now(); @@ -292,7 +292,7 @@ mod tests { } #[test] - fn override_is_used_instead_of_global_burst_if_present() -> AppResult<()> { + fn override_is_used_instead_of_global_burst_if_present() -> QueryResult<()> { let conn = pg_connection(); let now = now(); @@ -318,7 +318,7 @@ mod tests { Ok(()) } - fn new_user(conn: &PgConnection, gh_login: &str) -> AppResult { + fn new_user(conn: &PgConnection, gh_login: &str) -> QueryResult { use crate::models::NewUser; let user = NewUser { @@ -329,7 +329,11 @@ mod tests { Ok(user.id) } - fn new_user_bucket(conn: &PgConnection, tokens: i32, now: NaiveDateTime) -> AppResult { + fn new_user_bucket( + conn: &PgConnection, + tokens: i32, + now: NaiveDateTime, + ) -> QueryResult { diesel::insert_into(publish_limit_buckets::table) .values(Bucket { user_id: new_user(conn, "new_user")?, @@ -337,7 +341,6 @@ mod tests { last_refill: now, }) .get_result(conn) - .map_err(Into::into) } /// Strips ns precision from `Utc::now`. PostgreSQL only has microsecond diff --git a/src/render.rs b/src/render.rs index 203a589c5d2..bf24647bf15 100644 --- a/src/render.rs +++ b/src/render.rs @@ -4,7 +4,7 @@ use ammonia::{Builder, UrlRelative, UrlRelativeEvaluate}; use htmlescape::encode_minimal; use std::borrow::Cow; use std::path::Path; -use swirl::errors::PerformError; +use swirl::PerformError; use url::Url; use crate::background_jobs::Environment; @@ -229,7 +229,6 @@ pub fn render_and_upload_readme( base_url: Option, ) -> Result<(), PerformError> { use crate::schema::*; - use crate::util::errors::std_error_no_send; use diesel::prelude::*; let rendered = readme_to_html(&text, &file_name, base_url.as_ref().map(String::as_str)); @@ -243,8 +242,7 @@ pub fn render_and_upload_readme( .select((crates::name, versions::num)) .first::<(String, String)>(&*conn)?; env.uploader - .upload_readme(env.http_client(), &crate_name, &vers, rendered) - .map_err(std_error_no_send)?; + .upload_readme(env.http_client(), &crate_name, &vers, rendered)?; Ok(()) }) } diff --git a/src/router.rs b/src/router.rs index a4b01ef9e02..bde374a7e89 100644 --- a/src/router.rs +++ b/src/router.rs @@ -1,4 +1,3 @@ -use std::error::Error; use std::sync::Arc; use conduit::{Handler, Request, Response}; @@ -7,7 +6,7 @@ use conduit_router::{RequestParams, RouteBuilder}; use crate::controllers::*; use crate::util::errors::{std_error, AppError, AppResult, NotFound}; use crate::util::RequestProxy; -use crate::{App, Env}; +use crate::{middleware, App, Env}; pub fn build_router(app: &App) -> R404 { let mut api_router = RouteBuilder::new(); @@ -136,7 +135,7 @@ pub fn build_router(app: &App) -> R404 { struct C(pub fn(&mut dyn Request) -> AppResult); impl Handler for C { - fn call(&self, req: &mut dyn Request) -> Result> { + fn call(&self, req: &mut dyn Request) -> middleware::Result { let C(f) = *self; match f(req) { Ok(resp) => Ok(resp), @@ -151,7 +150,7 @@ impl Handler for C { struct R(pub Arc); impl Handler for R { - fn call(&self, req: &mut dyn Request) -> Result> { + fn call(&self, req: &mut dyn Request) -> middleware::Result { let path = req.params()["path"].to_string(); let R(ref sub_router) = *self; sub_router.call(&mut RequestProxy::rewrite_path(req, &path)) @@ -163,7 +162,7 @@ impl Handler for R { pub struct R404(pub RouteBuilder); impl Handler for R404 { - fn call(&self, req: &mut dyn Request) -> Result> { + fn call(&self, req: &mut dyn Request) -> middleware::Result { let R404(ref router) = *self; match router.recognize(&req.method(), req.path()) { Ok(m) => { @@ -178,7 +177,7 @@ impl Handler for R404 { #[cfg(test)] mod tests { use super::*; - use crate::util::errors::{bad_request, cargo_err, internal, NotFound, Unauthorized}; + use crate::util::errors::{bad_request, cargo_err, internal, AppError, NotFound, Unauthorized}; use conduit_test::MockRequest; use diesel::result::Error as DieselError; diff --git a/src/tasks/dump_db.rs b/src/tasks/dump_db.rs index 41d0e2a11d1..2536c3be44a 100644 --- a/src/tasks/dump_db.rs +++ b/src/tasks/dump_db.rs @@ -3,7 +3,7 @@ use std::{ path::{Path, PathBuf}, }; -use crate::{background_jobs::Environment, uploaders::Uploader, util::errors::std_error_no_send}; +use crate::{background_jobs::Environment, uploaders::Uploader}; use reqwest::header; use swirl::PerformError; @@ -156,16 +156,14 @@ impl DumpTarball { let tarfile = File::open(&self.tarball_path)?; let content_length = tarfile.metadata()?.len(); // TODO Figure out the correct content type. - uploader - .upload( - &client, - target_name, - tarfile, - content_length, - "application/gzip", - header::HeaderMap::new(), - ) - .map_err(std_error_no_send)?; + uploader.upload( + &client, + target_name, + tarfile, + content_length, + "application/gzip", + header::HeaderMap::new(), + )?; Ok(content_length) } } diff --git a/src/tests/all.rs b/src/tests/all.rs index c5dfc7a8c85..27f34b09910 100644 --- a/src/tests/all.rs +++ b/src/tests/all.rs @@ -13,7 +13,6 @@ use crate::util::{Bad, RequestHelper, TestApp}; use cargo_registry::{ models::{Crate, CrateOwner, Dependency, NewCategory, NewTeam, NewUser, Team, User, Version}, schema::crate_owners, - util::AppResult, views::{ EncodableCategory, EncodableCategoryWithSubcategories, EncodableCrate, EncodableKeyword, EncodableOwner, EncodableVersion, GoodCrate, @@ -229,7 +228,7 @@ fn new_team(login: &str) -> NewTeam<'_> { } } -fn add_team_to_crate(t: &Team, krate: &Crate, u: &User, conn: &PgConnection) -> AppResult<()> { +fn add_team_to_crate(t: &Team, krate: &Crate, u: &User, conn: &PgConnection) -> QueryResult<()> { let crate_owner = CrateOwner { crate_id: krate.id, owner_id: t.id, diff --git a/src/tests/builders.rs b/src/tests/builders.rs index 9f7d25d0260..56f223ee8f1 100644 --- a/src/tests/builders.rs +++ b/src/tests/builders.rs @@ -3,7 +3,7 @@ use cargo_registry::{ models::{Crate, Keyword, NewCrate, NewVersion, Version}, schema::{crates, dependencies, version_downloads, versions}, - util::AppResult, + util::errors::AppResult, views::krate_publish as u, }; use std::{collections::HashMap, io::Read}; diff --git a/src/uploaders.rs b/src/uploaders.rs index 7db99da9efb..5194071a33d 100644 --- a/src/uploaders.rs +++ b/src/uploaders.rs @@ -4,8 +4,8 @@ use openssl::error::ErrorStack; use openssl::hash::{Hasher, MessageDigest}; use reqwest::header; -use crate::util::LimitErrorReader; -use crate::util::{cargo_err, internal, AppResult, ChainError, Maximums}; +use crate::util::errors::{cargo_err, internal, AppResult, ChainError}; +use crate::util::{Error, LimitErrorReader, Maximums}; use std::env; use std::fs::{self, File}; @@ -102,19 +102,17 @@ impl Uploader { content_length: u64, content_type: &str, extra_headers: header::HeaderMap, - ) -> AppResult> { + ) -> Result, Error> { match *self { Uploader::S3 { ref bucket, .. } => { - bucket - .put( - client, - path, - content, - content_length, - content_type, - extra_headers, - ) - .map_err(|e| internal(&format_args!("failed to upload to S3: {}", e)))?; + bucket.put( + client, + path, + content, + content_length, + content_type, + extra_headers, + )?; Ok(Some(String::from(path))) } Uploader::Local => { @@ -156,7 +154,8 @@ impl Uploader { content_length, "application/x-tar", extra_headers, - )?; + ) + .map_err(|e| internal(&format_args!("failed to upload crate: {}", e)))?; Ok(checksum) } @@ -166,7 +165,7 @@ impl Uploader { crate_name: &str, vers: &str, readme: String, - ) -> AppResult<()> { + ) -> Result<(), Error> { let path = Uploader::readme_path(crate_name, vers); let content_length = readme.len() as u64; let content = Cursor::new(readme); diff --git a/src/util.rs b/src/util.rs index 8dac77a5a69..5bdbdb74d7b 100644 --- a/src/util.rs +++ b/src/util.rs @@ -5,8 +5,7 @@ use std::io::Cursor; use conduit::Response; use serde::Serialize; -pub use self::errors::ChainError; -pub use self::errors::{bad_request, cargo_err, internal, AppError, AppResult}; +pub use self::errors::concrete::Error; pub use self::io_util::{read_fill, read_le_u32, LimitErrorReader}; pub use self::request_helpers::*; pub use self::request_proxy::RequestProxy; diff --git a/src/util/errors.rs b/src/util/errors.rs index 5e9da0c9318..0ef93ce4893 100644 --- a/src/util/errors.rs +++ b/src/util/errors.rs @@ -1,3 +1,17 @@ +//! This module implements several error types and traits. The suggested usage in returned results +//! is as follows: +//! +//! * The concrete `util::concrete::Error` type (re-exported as `util::Error`) is great for code +//! that is not part of the request/response lifecycle. It avoids pulling in the unnecessary +//! infrastructure to convert errors into a user facing JSON responses (relative to `AppError`). +//! * `diesel::QueryResult` - There is a lot of code that only deals with query errors. If only +//! one type of error is possible in a function, using that specific error is preferable to the +//! more general `util::Error`. This is especially common in model code. +//! * `util::errors::AppResult` - Some failures should be converted into user facing JSON +//! responses. This error type is more dynamic and is box allocated. Low-level errors are +//! typically not converted to user facing errors and most usage is within the models, +//! controllers, and middleware layers. + use std::any::{Any, TypeId}; use std::error::Error; use std::fmt; @@ -8,6 +22,7 @@ use diesel::result::Error as DieselError; use crate::util::json_response; +pub(super) mod concrete; mod http; /// Returns an error with status 200 and the provided description as JSON @@ -24,18 +39,32 @@ pub fn cargo_err(error: &S) -> Box { // (see ), but Ember requires // non-200 response codes for its stores to work properly. +/// Return an error with status 400 and the provided description as JSON +pub fn bad_request(error: &S) -> Box { + Box::new(http::BadRequest(error.to_string())) +} + /// 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, +struct StringError<'a> { + detail: &'a str, } #[derive(Serialize)] -struct Bad { - errors: Vec, +struct Bad<'a> { + errors: Vec>, +} + +/// Generates a response with the provided status and description as JSON +fn json_error(detail: &str, status: (u32, &'static str)) -> Response { + let mut response = json_response(&Bad { + errors: vec![StringError { detail }], + }); + response.status = status; + response } // ============================================================================= @@ -167,7 +196,7 @@ impl From for Box { } } // ============================================================================= -// Concrete errors +// Internal error for use with `chain_error` #[derive(Debug)] struct InternalAppError { @@ -187,18 +216,14 @@ impl AppError for InternalAppError { } } +// TODO: The remaining can probably move under `http` + #[derive(Debug, Clone, Copy)] pub struct NotFound; impl AppError for NotFound { fn response(&self) -> Option { - let mut response = json_response(&Bad { - errors: vec![StringError { - detail: "Not Found".to_string(), - }], - }); - response.status = (404, "Not Found"); - Some(response) + Some(json_error("Not Found", (404, "Not Found"))) } } @@ -213,13 +238,8 @@ pub struct Unauthorized; impl AppError for Unauthorized { fn response(&self) -> Option { - let mut response = json_response(&Bad { - errors: vec![StringError { - detail: "must be logged in to perform that action".to_string(), - }], - }); - response.status = (403, "Forbidden"); - Some(response) + let detail = "must be logged in to perform that action"; + Some(json_error(detail, (403, "Forbidden"))) } } @@ -229,46 +249,14 @@ impl fmt::Display for Unauthorized { } } -#[derive(Debug)] -struct BadRequest(String); - -impl AppError for BadRequest { - fn response(&self) -> Option { - let mut response = json_response(&Bad { - errors: vec![StringError { - detail: self.0.clone(), - }], - }); - response.status = (400, "Bad Request"); - Some(response) - } -} - -impl fmt::Display for BadRequest { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - self.0.fmt(f) - } -} - pub fn internal(error: &S) -> Box { Box::new(InternalAppError { description: error.to_string(), }) } -/// 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 -/// non-200 response codes for its stores to work properly. -/// -/// Since this is going back to the UI these errors are treated the same as -/// `cargo_err` errors, other than the HTTP status code. -pub fn bad_request(error: &S) -> Box { - Box::new(BadRequest(error.to_string())) -} - #[derive(Debug)] -pub struct AppErrToStdErr(pub Box); +struct AppErrToStdErr(pub Box); impl Error for AppErrToStdErr {} @@ -282,24 +270,14 @@ pub(crate) fn std_error(e: Box) -> Box { Box::new(AppErrToStdErr(e)) } -pub(crate) fn std_error_no_send(e: Box) -> Box { - Box::new(AppErrToStdErr(e)) -} - #[derive(Debug, Clone, Copy)] pub struct ReadOnlyMode; impl AppError for ReadOnlyMode { fn response(&self) -> Option { - let mut response = json_response(&Bad { - errors: vec![StringError { - detail: "Crates.io is currently in read-only mode for maintenance. \ - Please try again later." - .to_string(), - }], - }); - response.status = (503, "Service Unavailable"); - Some(response) + let detail = "Crates.io is currently in read-only mode for maintenance. \ + Please try again later."; + Some(json_error(detail, (503, "Service Unavailable"))) } } @@ -319,17 +297,13 @@ impl AppError for TooManyRequests { const HTTP_DATE_FORMAT: &str = "%a, %d %b %Y %H:%M:%S GMT"; let retry_after = self.retry_after.format(HTTP_DATE_FORMAT); - let mut response = json_response(&Bad { - errors: vec![StringError { - detail: format!( - "You have published too many crates in a \ - short period of time. Please try again after {} or email \ - help@crates.io to have your limit increased.", - retry_after - ), - }], - }); - response.status = (429, "TOO MANY REQUESTS"); + let detail = format!( + "You have published too many crates in a \ + short period of time. Please try again after {} or email \ + help@crates.io to have your limit increased.", + retry_after + ); + let mut response = json_error(&detail, (429, "TOO MANY REQUESTS")); response .headers .insert("Retry-After".into(), vec![retry_after.to_string()]); diff --git a/src/util/errors/concrete.rs b/src/util/errors/concrete.rs new file mode 100644 index 00000000000..54491f00a39 --- /dev/null +++ b/src/util/errors/concrete.rs @@ -0,0 +1,81 @@ +use std::{error, fmt, io}; + +#[derive(Debug)] +pub enum Error { + DbConnect(diesel::result::ConnectionError), + DbQuery(diesel::result::Error), + DotEnv(dotenv::Error), + Internal(String), + Io(io::Error), + JobEnqueue(swirl::EnqueueError), + Openssl(openssl::error::ErrorStack), + Reqwest(reqwest::Error), +} + +impl error::Error for Error {} + +impl fmt::Display for Error { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Error::DbConnect(inner) => inner.fmt(f), + Error::DbQuery(inner) => inner.fmt(f), + Error::DotEnv(inner) => inner.fmt(f), + Error::Internal(inner) => inner.fmt(f), + Error::Io(inner) => inner.fmt(f), + Error::JobEnqueue(inner) => inner.fmt(f), + Error::Openssl(inner) => inner.fmt(f), + Error::Reqwest(inner) => inner.fmt(f), + } + } +} + +impl From for Error { + fn from(err: diesel::result::ConnectionError) -> Self { + Error::DbConnect(err) + } +} + +impl From for Error { + fn from(err: diesel::result::Error) -> Self { + Error::DbQuery(err) + } +} + +impl From for Error { + fn from(err: dotenv::Error) -> Self { + Error::DotEnv(err) + } +} + +impl From for Error { + fn from(err: String) -> Self { + Error::Internal(err) + } +} + +impl From for Error { + fn from(err: io::Error) -> Self { + Error::Io(err) + } +} + +impl From for Error { + fn from(err: swirl::EnqueueError) -> Self { + Error::JobEnqueue(err) + } +} + +impl From for Error { + fn from(err: s3::Error) -> Self { + match err { + s3::Error::Openssl(e) => Error::Openssl(e), + s3::Error::Reqwest(e) => Error::Reqwest(e), + } + } +} + +impl From for Error { + fn from(err: reqwest::Error) -> Self { + Error::Reqwest(err) + } +} diff --git a/src/util/errors/http.rs b/src/util/errors/http.rs index 2ee59fe15cc..b7c7063ea57 100644 --- a/src/util/errors/http.rs +++ b/src/util/errors/http.rs @@ -2,21 +2,18 @@ use std::fmt; use conduit::Response; -use super::{AppError, Bad, StringError}; -use crate::util::json_response; +use super::{json_error, AppError}; #[derive(Debug)] pub(super) struct Ok(pub(super) String); #[derive(Debug)] +pub(super) struct BadRequest(pub(super) String); +#[derive(Debug)] pub(super) struct ServerError(pub(super) String); impl AppError for Ok { fn response(&self) -> Option { - Some(json_response(&Bad { - errors: vec![StringError { - detail: self.0.clone(), - }], - })) + Some(json_error(&self.0, (200, "OK"))) } } @@ -26,15 +23,21 @@ impl fmt::Display for Ok { } } +impl AppError for BadRequest { + fn response(&self) -> Option { + Some(json_error(&self.0, (400, "Bad Request"))) + } +} + +impl fmt::Display for BadRequest { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + self.0.fmt(f) + } +} + impl AppError for ServerError { 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) + Some(json_error(&self.0, (500, "Internal Server Error"))) } }