Auto merge of #1920 - jtgeibel:rename-error-types-and-fns, r=smarnach

Begin refactoring error handling/fallback logic

This PR is best reviewed as individual commits.  The first commits rename some types and functions without changing anything.  The final commits remove some unused logic and lay some groundwork for future refactoring.

The overall intent of this PR is to more clearly document the existing behavior.  In particular, the top level error types no longer include `Cargo` in the name as not all error types are intended for cargo endpoints.

r? @smarnach

Author: bors

src/background_jobs.rs

@@ -5,17 +5,17 @@ use swirl::PerformError;
 use crate::db::{DieselPool, DieselPooledConn};
 use crate::git::Repository;
 use crate::uploaders::Uploader;
-use crate::util::errors::{CargoErrToStdErr, CargoResult};
+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 = CargoErrToStdErr;
+    type Error = AppErrToStdErr;
 
     fn get(&self) -> Result<swirl::db::DieselPooledConn<'_, Self>, Self::Error> {
-        self.get().map_err(CargoErrToStdErr)
+        self.get().map_err(AppErrToStdErr)
     }
 }
 
@@ -59,10 +59,10 @@ impl Environment {
     pub fn connection(&self) -> Result<DieselPooledConn<'_>, PerformError> {
         self.connection_pool
             .get()
-            .map_err(|e| CargoErrToStdErr(e).into())
+            .map_err(|e| AppErrToStdErr(e).into())
     }
 
-    pub fn lock_index(&self) -> CargoResult<MutexGuard<'_, Repository>> {
+    pub fn lock_index(&self) -> AppResult<MutexGuard<'_, Repository>> {
         let repo = self.index.lock().unwrap_or_else(PoisonError::into_inner);
         repo.reset_head()?;
         Ok(repo)

src/bin/enqueue-job.rs

@@ -1,8 +1,8 @@
-use cargo_registry::util::{human, CargoError, CargoResult};
+use cargo_registry::util::{cargo_err, AppError, AppResult};
 use cargo_registry::{db, env, tasks};
 use diesel::PgConnection;
 
-fn main() -> CargoResult<()> {
+fn main() -> AppResult<()> {
     let conn = db::connect_now()?;
     let mut args = std::env::args().skip(1);
     match &*args.next().unwrap_or_default() {
@@ -14,15 +14,15 @@ fn main() -> CargoResult<()> {
                 .unwrap_or_else(|| String::from("db-dump.tar.gz"));
             tasks::dump_db(database_url, target_name).enqueue(&conn)
         }
-        other => Err(human(&format!("Unrecognized job type `{}`", other))),
+        other => Err(cargo_err(&format!("Unrecognized job type `{}`", other))),
     }
 }
 
 /// Helper to map the `PerformError` returned by `swirl::Job::enqueue()` to a
-/// `CargoError`. Can be removed once `map_err()` isn't needed any more.
+/// `AppError`. Can be removed once `map_err()` isn't needed any more.
 trait Enqueue: swirl::Job {
-    fn enqueue(self, conn: &PgConnection) -> CargoResult<()> {
-        <Self as swirl::Job>::enqueue(self, conn).map_err(|e| CargoError::from_std_error(e))
+    fn enqueue(self, conn: &PgConnection) -> AppResult<()> {
+        <Self as swirl::Job>::enqueue(self, conn).map_err(|e| AppError::from_std_error(e))
     }
 }
 

src/bin/monitor.rs

@@ -8,18 +8,18 @@
 
 mod on_call;
 
-use cargo_registry::{db, schema::*, util::CargoResult};
+use cargo_registry::{db, schema::*, util::AppResult};
 use diesel::prelude::*;
 
-fn main() -> CargoResult<()> {
+fn main() -> AppResult<()> {
     let conn = db::connect_now()?;
 
     check_stalled_background_jobs(&conn)?;
     check_spam_attack(&conn)?;
     Ok(())
 }
 
-fn check_stalled_background_jobs(conn: &PgConnection) -> CargoResult<()> {
+fn check_stalled_background_jobs(conn: &PgConnection) -> AppResult<()> {
     use cargo_registry::schema::background_jobs::dsl::*;
     use diesel::dsl::*;
 
@@ -55,7 +55,7 @@ fn check_stalled_background_jobs(conn: &PgConnection) -> CargoResult<()> {
     Ok(())
 }
 
-fn check_spam_attack(conn: &PgConnection) -> CargoResult<()> {
+fn check_spam_attack(conn: &PgConnection) -> AppResult<()> {
     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) -> CargoResult<()> {
     Ok(())
 }
 
-fn log_and_trigger_event(event: on_call::Event) -> CargoResult<()> {
+fn log_and_trigger_event(event: on_call::Event) -> AppResult<()> {
     match event {
         on_call::Event::Trigger {
             ref description, ..

src/bin/on_call/mod.rs

@@ -1,4 +1,4 @@
-use cargo_registry::util::{internal, CargoResult};
+use cargo_registry::util::{internal, AppResult};
 
 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) -> CargoResult<()> {
+    pub fn send(self) -> AppResult<()> {
         let api_token = dotenv::var("PAGERDUTY_API_TOKEN")?;
         let service_key = dotenv::var("PAGERDUTY_INTEGRATION_KEY")?;
 

src/boot/categories.rs

@@ -3,7 +3,7 @@
 
 use crate::{
     db,
-    util::errors::{internal, CargoResult, ChainError},
+    util::errors::{internal, AppResult, ChainError},
 };
 
 use diesel::prelude::*;
@@ -37,7 +37,7 @@ impl Category {
     }
 }
 
-fn required_string_from_toml<'a>(toml: &'a toml::value::Table, key: &str) -> CargoResult<&'a str> {
+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!(
             "Expected category TOML attribute '{}' to be a String",
@@ -53,7 +53,7 @@ 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>,
-) -> CargoResult<Vec<Category>> {
+) -> AppResult<Vec<Category>> {
     let mut result = vec![];
 
     for (slug, details) in categories {
@@ -85,12 +85,12 @@ fn categories_from_toml(
     Ok(result)
 }
 
-pub fn sync(toml_str: &str) -> CargoResult<()> {
+pub fn sync(toml_str: &str) -> AppResult<()> {
     let conn = db::connect_now().unwrap();
     sync_with_connection(toml_str, &conn)
 }
 
-pub fn sync_with_connection(toml_str: &str, conn: &PgConnection) -> CargoResult<()> {
+pub fn sync_with_connection(toml_str: &str, conn: &PgConnection) -> AppResult<()> {
     use crate::schema::categories::dsl::*;
     use diesel::dsl::all;
     use diesel::pg::upsert::excluded;

src/controllers.rs

@@ -6,7 +6,7 @@ mod prelude {
     pub use conduit_router::RequestParams;
 
     pub use crate::db::RequestTransaction;
-    pub use crate::util::{human, CargoResult};
+    pub use crate::util::{cargo_err, AppResult};
 
     pub use crate::middleware::app::RequestApp;
     pub use crate::middleware::current_user::RequestUser;

src/controllers/category.rs

@@ -6,7 +6,7 @@ use crate::schema::categories;
 use crate::views::{EncodableCategory, EncodableCategoryWithSubcategories};
 
 /// Handles the `GET /categories` route.
-pub fn index(req: &mut dyn Request) -> CargoResult<Response> {
+pub fn index(req: &mut dyn Request) -> AppResult<Response> {
     let conn = req.db_conn()?;
     let query = req.query();
     // FIXME: There are 69 categories, 47 top level. This isn't going to
@@ -40,7 +40,7 @@ pub fn index(req: &mut dyn Request) -> CargoResult<Response> {
 }
 
 /// Handles the `GET /categories/:category_id` route.
-pub fn show(req: &mut dyn Request) -> CargoResult<Response> {
+pub fn show(req: &mut dyn Request) -> AppResult<Response> {
     let slug = &req.params()["category_id"];
     let conn = req.db_conn()?;
     let cat = Category::by_slug(slug).first::<Category>(&*conn)?;
@@ -77,7 +77,7 @@ pub fn show(req: &mut dyn Request) -> CargoResult<Response> {
 }
 
 /// Handles the `GET /category_slugs` route.
-pub fn slugs(req: &mut dyn Request) -> CargoResult<Response> {
+pub fn slugs(req: &mut dyn Request) -> AppResult<Response> {
     let conn = req.db_conn()?;
     let slugs = categories::table
         .select((categories::slug, categories::slug, categories::description))

src/controllers/crate_owner_invitation.rs

@@ -5,7 +5,7 @@ use crate::schema::{crate_owner_invitations, crate_owners};
 use crate::views::{EncodableCrateOwnerInvitation, InvitationResponse};
 
 /// Handles the `GET /me/crate_owner_invitations` route.
-pub fn list(req: &mut dyn Request) -> CargoResult<Response> {
+pub fn list(req: &mut dyn Request) -> AppResult<Response> {
     let conn = &*req.db_conn()?;
     let user_id = req.user()?.id;
 
@@ -31,14 +31,14 @@ struct OwnerInvitation {
 }
 
 /// Handles the `PUT /me/crate_owner_invitations/:crate_id` route.
-pub fn handle_invite(req: &mut dyn Request) -> CargoResult<Response> {
+pub fn handle_invite(req: &mut dyn Request) -> AppResult<Response> {
     let mut body = String::new();
     req.body().read_to_string(&mut body)?;
 
     let conn = &*req.db_conn()?;
 
     let crate_invite: OwnerInvitation =
-        serde_json::from_str(&body).map_err(|_| human("invalid json request"))?;
+        serde_json::from_str(&body).map_err(|_| cargo_err("invalid json request"))?;
 
     let crate_invite = crate_invite.crate_owner_invite;
 
@@ -53,7 +53,7 @@ fn accept_invite(
     req: &dyn Request,
     conn: &PgConnection,
     crate_invite: InvitationResponse,
-) -> CargoResult<Response> {
+) -> AppResult<Response> {
     use diesel::{delete, insert_into};
 
     let user_id = req.user()?.id;
@@ -92,7 +92,7 @@ fn decline_invite(
     req: &dyn Request,
     conn: &PgConnection,
     crate_invite: InvitationResponse,
-) -> CargoResult<Response> {
+) -> AppResult<Response> {
     use diesel::delete;
 
     let user_id = req.user()?.id;

src/controllers/helpers.rs

@@ -1,11 +1,11 @@
-use crate::util::{json_response, CargoResult};
+use crate::util::{json_response, AppResult};
 use conduit::Response;
 
 pub(crate) mod pagination;
 
 pub(crate) use self::pagination::Paginate;
 
-pub fn ok_true() -> CargoResult<Response> {
+pub fn ok_true() -> AppResult<Response> {
     #[derive(Serialize)]
     struct R {
         ok: bool,

src/controllers/helpers/pagination.rs

@@ -14,11 +14,11 @@ pub(crate) enum Page {
 }
 
 impl Page {
-    fn new(params: &IndexMap<String, String>) -> CargoResult<Self> {
+    fn new(params: &IndexMap<String, String>) -> AppResult<Self> {
         if let Some(s) = params.get("page") {
             let numeric_page = s.parse()?;
             if numeric_page < 1 {
-                return Err(human(&format_args!(
+                return Err(cargo_err(&format_args!(
                     "page indexing starts from 1, page {} is invalid",
                     numeric_page,
                 )));
@@ -38,7 +38,7 @@ pub(crate) struct PaginationOptions {
 }
 
 impl PaginationOptions {
-    pub(crate) fn new(params: &IndexMap<String, String>) -> CargoResult<Self> {
+    pub(crate) fn new(params: &IndexMap<String, String>) -> AppResult<Self> {
         const DEFAULT_PER_PAGE: u32 = 10;
         const MAX_PER_PAGE: u32 = 100;
 
@@ -48,7 +48,7 @@ impl PaginationOptions {
             .unwrap_or(Ok(DEFAULT_PER_PAGE))?;
 
         if per_page > MAX_PER_PAGE {
-            return Err(human(&format_args!(
+            return Err(cargo_err(&format_args!(
                 "cannot request more than {} items",
                 MAX_PER_PAGE,
             )));
@@ -70,7 +70,7 @@ impl PaginationOptions {
 }
 
 pub(crate) trait Paginate: Sized {
-    fn paginate(self, params: &IndexMap<String, String>) -> CargoResult<PaginatedQuery<Self>> {
+    fn paginate(self, params: &IndexMap<String, String>) -> AppResult<PaginatedQuery<Self>> {
         Ok(PaginatedQuery {
             query: self,
             options: PaginationOptions::new(params)?,

src/controllers/keyword.rs

@@ -5,7 +5,7 @@ use crate::models::Keyword;
 use crate::views::EncodableKeyword;
 
 /// Handles the `GET /keywords` route.
-pub fn index(req: &mut dyn Request) -> CargoResult<Response> {
+pub fn index(req: &mut dyn Request) -> AppResult<Response> {
     use crate::schema::keywords;
 
     let conn = req.db_conn()?;
@@ -41,7 +41,7 @@ pub fn index(req: &mut dyn Request) -> CargoResult<Response> {
 }
 
 /// Handles the `GET /keywords/:keyword_id` route.
-pub fn show(req: &mut dyn Request) -> CargoResult<Response> {
+pub fn show(req: &mut dyn Request) -> AppResult<Response> {
     let name = &req.params()["keyword_id"];
     let conn = req.db_conn()?;
 

src/controllers/krate/downloads.rs

@@ -14,7 +14,7 @@ use crate::views::EncodableVersionDownload;
 use crate::models::krate::to_char;
 
 /// Handles the `GET /crates/:crate_id/downloads` route.
-pub fn downloads(req: &mut dyn Request) -> CargoResult<Response> {
+pub fn downloads(req: &mut dyn Request) -> AppResult<Response> {
     use diesel::dsl::*;
     use diesel::sql_types::BigInt;
 

src/controllers/krate/follow.rs

@@ -7,7 +7,7 @@ use crate::db::DieselPooledConn;
 use crate::models::{Crate, Follow};
 use crate::schema::*;
 
-fn follow_target(req: &dyn Request, conn: &DieselPooledConn<'_>) -> CargoResult<Follow> {
+fn follow_target(req: &dyn Request, conn: &DieselPooledConn<'_>) -> AppResult<Follow> {
     let user = req.user()?;
     let crate_name = &req.params()["crate_id"];
     let crate_id = Crate::by_name(crate_name)
@@ -20,7 +20,7 @@ fn follow_target(req: &dyn Request, conn: &DieselPooledConn<'_>) -> CargoResult<
 }
 
 /// Handles the `PUT /crates/:crate_id/follow` route.
-pub fn follow(req: &mut dyn Request) -> CargoResult<Response> {
+pub fn follow(req: &mut dyn Request) -> AppResult<Response> {
     let conn = req.db_conn()?;
     let follow = follow_target(req, &conn)?;
     diesel::insert_into(follows::table)
@@ -32,7 +32,7 @@ pub fn follow(req: &mut dyn Request) -> CargoResult<Response> {
 }
 
 /// Handles the `DELETE /crates/:crate_id/follow` route.
-pub fn unfollow(req: &mut dyn Request) -> CargoResult<Response> {
+pub fn unfollow(req: &mut dyn Request) -> AppResult<Response> {
     let conn = req.db_conn()?;
     let follow = follow_target(req, &conn)?;
     diesel::delete(&follow).execute(&*conn)?;
@@ -41,7 +41,7 @@ pub fn unfollow(req: &mut dyn Request) -> CargoResult<Response> {
 }
 
 /// Handles the `GET /crates/:crate_id/following` route.
-pub fn following(req: &mut dyn Request) -> CargoResult<Response> {
+pub fn following(req: &mut dyn Request) -> AppResult<Response> {
     use diesel::dsl::exists;
 
     let conn = req.db_conn()?;