Auto merge of #2032 - jtgeibel:error-refactor-step3, r=jtgeibel

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

This PR may be easier to review commit by commit.  Overall this series continues the previous refactoring work in #1920 and #1929.  In particular, a concrete `util::Error` error type is introduced.  The code is updated to follow a style where:

* The new concrete type `util::Error` is great for code that is not part of the request/response lifecycle.  Binaries have been converted to `fn main() -> Result<(), Error>` where applicable.  This avoids pulling in the unnecessary infrastructure to convert errors into a user facing JSON responses (relative to `AppError`).
  * There are now 13 results across 6 files for `, Error>`.  Usage occurs in `src/bin/`, `src/boot/`, and `src/uploaders.rs`.
* `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.
  * There are now 49 results across 16 files for `QueryResult<`.  Most usage is in `src/models/`, with some usage in `src/publish_rate_limit.rs`, `src/tasks/`, and `src/tests/`.
* `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 (85 of 102 results below).
  * There are now 102 results across 36 files for `AppResult<`.  Most usage is in `src/controllers/` and `src/models/`, with additional usage in `src/middleware/`, `src/{email,github,publish_rate_limit,router,uploaders}.rs`, and `src/{tests,util}/`.

Finally, a `middleware::Result` type is introduced for use in middleware and router code.

The behavior of `dyn AppError` and `chain_error` is a remaining area for improvement.  There may be cases where we want to wrap an `internal` error in a user facing error (like `cargo_err` or `server_err`) and still have the internal error make it up to the logging middleware.  However, I don't see an easy way to propagate this up the middleware stack (without encoding it in an internal response header that we later remove, :sweat:).

r? @smarnach

Author: bors

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