Skip to content

Begin refactoring error handling/fallback logic #1920

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Nov 26, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/background_jobs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}

Expand Down Expand Up @@ -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)
Expand Down
12 changes: 6 additions & 6 deletions src/bin/enqueue-job.rs
Original file line number Diff line number Diff line change
@@ -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() {
Expand All @@ -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))
}
}

Expand Down
10 changes: 5 additions & 5 deletions src/bin/monitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::*;

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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, ..
Expand Down
4 changes: 2 additions & 2 deletions src/bin/on_call/mod.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use cargo_registry::util::{internal, CargoResult};
use cargo_registry::util::{internal, AppResult};

use reqwest::{header, StatusCode as Status};

Expand All @@ -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")?;

Expand Down
10 changes: 5 additions & 5 deletions src/boot/categories.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

use crate::{
db,
util::errors::{internal, CargoResult, ChainError},
util::errors::{internal, AppResult, ChainError},
};

use diesel::prelude::*;
Expand Down Expand Up @@ -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",
Expand All @@ -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 {
Expand Down Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion src/controllers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
6 changes: 3 additions & 3 deletions src/controllers/category.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)?;
Expand Down Expand Up @@ -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))
Expand Down
10 changes: 5 additions & 5 deletions src/controllers/crate_owner_invitation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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;

Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
4 changes: 2 additions & 2 deletions src/controllers/helpers.rs
Original file line number Diff line number Diff line change
@@ -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,
Expand Down
10 changes: 5 additions & 5 deletions src/controllers/helpers/pagination.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
)));
Expand All @@ -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;

Expand All @@ -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,
)));
Expand All @@ -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)?,
Expand Down
4 changes: 2 additions & 2 deletions src/controllers/keyword.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()?;
Expand Down Expand Up @@ -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()?;

Expand Down
2 changes: 1 addition & 1 deletion src/controllers/krate/downloads.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
8 changes: 4 additions & 4 deletions src/controllers/krate/follow.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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)?;
Expand All @@ -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()?;
Expand Down
Loading