Add a concrete util::Error type (step 3 of refactoring)

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<swirl::db::DieselPooledConn<'_, Self>, Self::Error> {
-        self.get().map_err(AppErrToStdErr)
+        self.get()
     }
 }
 
@@ -56,13 +57,11 @@ impl Environment {
         }
     }
 
-    pub fn connection(&self) -> Result<DieselPooledConn<'_>, PerformError> {
-        self.connection_pool
-            .get()
-            .map_err(|e| AppErrToStdErr(e).into())
+    pub fn connection(&self) -> Result<DieselPooledConn<'_>, PoolError> {
+        self.connection_pool.get()
     }
 
-    pub fn lock_index(&self) -> AppResult<MutexGuard<'_, Repository>> {
+    pub fn lock_index(&self) -> Result<MutexGuard<'_, Repository>, PerformError> {
         let repo = self.index.lock().unwrap_or_else(PoisonError::into_inner);
         repo.reset_head()?;
         Ok(repo)

src/bin/enqueue-job.rs

@@ -1,33 +1,24 @@
-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);
 
     let job = args.next().unwrap_or_default();
     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<()> {
-        <Self as swirl::Job>::enqueue(self, conn).map_err(|e| AppError::from_std_error(e))
-    }
-}
-
-impl<J: swirl::Job> Enqueue for J {}

src/bin/monitor.rs

@@ -8,18 +8,18 @@
 
 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)?;
     check_spam_attack(&conn)?;
     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, ..

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::<InvalidEvent>()?;
-                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)
     }
 }
 

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::<Vec<_>>();
 
     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()
 }

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<Vec<Category>> {
+) -> Result<Vec<Category>, 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;

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;

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;

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::*;

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.

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;

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,

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.
 ///

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

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<DieselPooledConn<'_>> {
+    pub fn get(&self) -> Result<DieselPooledConn<'_>, 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<DieselPooledConn<'_>>;
+    fn db_conn(&self) -> Result<DieselPooledConn<'_>, r2d2::PoolError>;
 }
 
 impl<T: Request + ?Sized> RequestTransaction for T {
-    fn db_conn(&self) -> AppResult<DieselPooledConn<'_>> {
-        self.app().diesel_database.get().map_err(Into::into)
+    fn db_conn(&self) -> Result<DieselPooledConn<'_>, r2d2::PoolError> {
+        self.app().diesel_database.get()
     }
 }
 

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"))?;
         }
     }