diff options
| author | Owen Jacobson <owen@grimoire.ca> | 2025-11-11 01:29:36 -0500 |
|---|---|---|
| committer | Owen Jacobson <owen@grimoire.ca> | 2025-11-25 20:49:03 -0500 |
| commit | 33601ef703a640b57e5bd0bf7dbd6d7ffa7377bf (patch) | |
| tree | 096b997d56959dd88d099f4f96a383daa4dbc39a | |
| parent | f9ecfa3c342d5932edd5f8d3e75e77482dca472d (diff) | |
Define a generic "Failed" case for app-level errors (and a few others).
We were previously exporting root causes from one layer of abstraction to the next. For example, anything that called into the database could cause an `sqlx::Error`, so anything that transitively called into that logic exported a `Database(sqlx::Error)` error variant of its own, using `From` to map errors from inner type to outer type.
This had a couple of side effects. First, it required each layer of error handling to carry with it a `From` implementation unwrapping and rewrapping root causes from the next layer down. This was particularly apparent in the event and boot endpoints, which had separate error cases unique to crypto key processing errors solely because they happened to involve handling events that contained those keys. There were others, including the pervasive `Database(sqlx::Error)` error variants.
Separately, none of the error variants introduced for this purpose were being used for anything other than printing to stderr. All the complexity of From impls and all the structure of the error types was being thrown away at top-level error handlers.
This change replaces most of those error types with a generic `Failed` error. A `Failed` carries with it two pieces of information: a (boxed) underlying error, of any boxable `Error` type, and text meant to explain the context and cause of an error. Code which acts on errors can treat `Failed` as a catch-all case, while individually handling errors that signify important cases. Errors can be moved into or out of the `Failed` case by refactoring, as needed.
The design of `Failed` is heavily motivated by [anyhow's `context` system][context] as a way for the programmer to capture immediate intention as an explanation for some underlying error. However, instead of accepting the full breadth of types that implement `Display`, a `Failed` can only carry strings as explanation. We don't need the generality at this time, and the implementation underlying it is pretty complex for what it does.
[context]: https://docs.rs/anyhow/latest/anyhow/struct.Error.html#method.context
This change also means that the full source chain for an error is now available to top-level error handlers, allowing more complete error messages. For example, starting `pilcrow` with an invalid network listen address produces
Failed to bind to www.google.com:64209
Caused by:
Can't assign requested address (os error 49)
instead of the previous
Error: Io(Os { code: 49, kind: AddrNotAvailable, message: "Can't assign requested address" })
which previously captured the same _cause_, but without the formatting (see previous commit) and without the _context_ (this commit). Similar improvements are available for many of the error scenarios Pilcrow is designed to give up on.
When deciding which errors to use `Failed` with, I've used the heuristic that if something can fail for more than one underlying reason, and if the caller will only ever need to be able to differentiate those reasons after substantial refactoring anyways, then the reasons should collase into `Failed`. If there's either only a single underlying failure reason possible, or only errors arising out of the function body possible, then I've left error handling alone.
In the process I've refactored most request-handler-level error mappings to explicitly map `Failed` to `Internal`, rather than having a catch-all mapping for all unhandled errors, to make it easier to remember to add request-level error representations when adding app-level error cases.
This also includes helper traits for `Error` and `Result`, to make constructing `Failed` (and errors that include `Failed` as an alternative) easier to do, and some constants for the recurring error messages related to transaction demarcation. I'm not completely happy with the repetitive nature of those error cases, but this is the best I've arrived at so far.
As errors are no longer expected to be convertible up the call stack, the `NotFound` and `Duplicate` helper traits for database errors had to change a bit. Those previously assumed that they would be used in the context of an error type implementing `From<sqlx::Error>` (or from another error type with similar characteristics), and that's not the case any more. The resulting idiom for converting a missing value into a domain error is `foo.await.optional().fail(MESSAGE)?.ok_or(DOMAIN ERROR)?`, which is rather clunky, but I've opted not to go further with it. The `Duplicate` helper is just plain gone, as it's not easily generalizable in this structure and using `match` is more tractable for me.
Finally, I've changed the convention for error messages from `all lowercase messages in whatever tense i feel like at the moment` to `Sentence-case messages in the past tense`, frequently starting with `Failed to` and a short summary of the task at hand. This, as above, makes error message capitalization between Pilcrow's own messages and messages coming from other libraries/the Rust stdlib much more coherent and less jarring to read.
31 files changed, 583 insertions, 514 deletions
diff --git a/src/boot/app.rs b/src/boot/app.rs index 1ca8adb..c2766cd 100644 --- a/src/boot/app.rs +++ b/src/boot/app.rs @@ -4,10 +4,10 @@ use sqlx::sqlite::SqlitePool; use super::Snapshot; use crate::{ conversation::{self, repo::Provider as _}, - db::NotFound, + db::{self, NotFound as _}, + error::failed::{Failed, ResultExt as _}, event::{Event, Sequence, repo::Provider as _}, message::{self, repo::Provider as _}, - name, user::{self, repo::Provider as _}, vapid::{self, repo::Provider as _}, }; @@ -21,16 +21,37 @@ impl Boot { Self { db } } - pub async fn snapshot(&self) -> Result<Snapshot, Error> { - let mut tx = self.db.begin().await?; - let resume_point = tx.sequence().current().await?; - - let users = tx.users().all(resume_point).await?; - let conversations = tx.conversations().all(resume_point).await?; - let messages = tx.messages().all(resume_point).await?; - let vapid = tx.vapid().current().await.optional()?; - - tx.commit().await?; + pub async fn snapshot(&self) -> Result<Snapshot, Failed> { + let mut tx = self.db.begin().await.fail(db::failed::BEGIN)?; + let resume_point = tx + .sequence() + .current() + .await + .fail("Failed to load resume point")?; + + let users = tx + .users() + .all(resume_point) + .await + .fail("Failed to load user events")?; + let conversations = tx + .conversations() + .all(resume_point) + .await + .fail("Failed to load conversation events")?; + let messages = tx + .messages() + .all(resume_point) + .await + .fail("Failed to load message events")?; + let vapid = tx + .vapid() + .current() + .await + .optional() + .fail("Failed to load VAPID key events")?; + + tx.commit().await.fail(db::failed::COMMIT)?; let user_events = users .iter() @@ -71,45 +92,3 @@ impl Boot { }) } } - -#[derive(Debug, thiserror::Error)] -#[error(transparent)] -pub enum Error { - Database(#[from] sqlx::Error), - Name(#[from] name::Error), - Ecdsa(#[from] p256::ecdsa::Error), - Pkcs8(#[from] p256::pkcs8::Error), - WebPush(#[from] web_push::WebPushError), -} - -impl From<user::repo::LoadError> for Error { - fn from(error: user::repo::LoadError) -> Self { - use user::repo::LoadError; - match error { - LoadError::Name(error) => error.into(), - LoadError::Database(error) => error.into(), - } - } -} - -impl From<conversation::repo::LoadError> for Error { - fn from(error: conversation::repo::LoadError) -> Self { - use conversation::repo::LoadError; - match error { - LoadError::Name(error) => error.into(), - LoadError::Database(error) => error.into(), - } - } -} - -impl From<vapid::repo::Error> for Error { - fn from(error: vapid::repo::Error) -> Self { - use vapid::repo::Error; - match error { - Error::Database(error) => error.into(), - Error::Ecdsa(error) => error.into(), - Error::Pkcs8(error) => error.into(), - Error::WebPush(error) => error.into(), - } - } -} @@ -3,7 +3,7 @@ //! This module supports running `pilcrow` as a freestanding program, via the //! [`Args`] struct. -use std::{future, io, str}; +use std::{future, str}; use axum::{ http::header, @@ -18,8 +18,10 @@ use web_push::{IsahcWebPushClient, WebPushClient}; pub use crate::exit::Exit; use crate::{ app::App, - clock, db, routes, - umask::{self, Umask}, + clock, db, + error::failed::{Failed, ResultExt as _}, + routes, + umask::Umask, }; /// Command-line entry point for running the `pilcrow` server. @@ -97,20 +99,24 @@ impl Args { /// of the failure. pub async fn run(self) -> Result<(), impl std::error::Error> { self.umask.set(); - let pool = self.pool().await?; + let pool = self.pool().await.fail("Failed to create database pool")?; - let webpush = IsahcWebPushClient::new()?; + let webpush = IsahcWebPushClient::new().fail("Failed to create web push publisher")?; let app = App::from(pool, webpush); match self.command { None => self.serve(app).await?, - Some(Command::RotateVapidKey) => app.vapid().rotate_key().await?, + Some(Command::RotateVapidKey) => app + .vapid() + .rotate_key() + .await + .fail("Failed to rotate VAPID key")?, } - Result::<_, Error>::Ok(()) + Result::<_, Failed>::Ok(()) } - async fn serve<P>(self, app: App<P>) -> Result<(), Error> + async fn serve<P>(self, app: App<P>) -> Result<(), Failed> where P: WebPushClient + Clone + Send + Sync + 'static, { @@ -119,24 +125,25 @@ impl Args { .route_layer(middleware::map_response(Self::server_info())) .with_state(app); - let listener = self.listener().await?; - let started_msg = started_msg(&listener)?; + let listen_addr = self.listen_addr(); + let listener = net::TcpListener::bind(&listen_addr).await.fail_with(|| { + format!( + "Failed to bind to {host}:{port}", + host = listen_addr.0, + port = listen_addr.1 + ) + })?; + let started_msg = started_msg(&listen_addr); let serve = axum::serve(listener, app); println!("{started_msg}"); - serve.await?; + serve.await.fail("Failed to serve application")?; Ok(()) } - async fn listener(&self) -> io::Result<net::TcpListener> { - let listen_addr = self.listen_addr(); - let listener = tokio::net::TcpListener::bind(listen_addr).await?; - Ok(listener) - } - - fn listen_addr(&self) -> impl net::ToSocketAddrs + '_ { - (self.address.as_str(), self.port) + fn listen_addr(&self) -> (String, u16) { + (self.address.clone(), self.port) } async fn pool(&self) -> Result<SqlitePool, db::Error> { @@ -155,17 +162,7 @@ impl Args { } } -fn started_msg(listener: &net::TcpListener) -> io::Result<String> { - let local_addr = listener.local_addr()?; - Ok(format!("listening on http://{local_addr}/")) -} - -#[derive(Debug, thiserror::Error)] -#[error(transparent)] -enum Error { - Io(#[from] io::Error), - Database(#[from] db::Error), - Sqlx(#[from] sqlx::Error), - Umask(#[from] umask::Error), - Webpush(#[from] web_push::WebPushError), +fn started_msg(listen_addr: &(String, u16)) -> String { + let (host, port) = listen_addr; + format!("Listening on http://{host}:{port}/") } diff --git a/src/conversation/app.rs b/src/conversation/app.rs index 2b62e77..73c655b 100644 --- a/src/conversation/app.rs +++ b/src/conversation/app.rs @@ -2,17 +2,14 @@ use chrono::TimeDelta; use itertools::Itertools; use sqlx::sqlite::SqlitePool; -use super::{ - Conversation, History, Id, history, - repo::{LoadError, Provider as _}, - validate, -}; +use super::{Conversation, History, Id, history, repo::Provider as _, validate}; use crate::{ clock::DateTime, - db::{Duplicate as _, NotFound as _}, + db::{self, NotFound as _}, + error::failed::{ErrorExt as _, Failed, ResultExt as _}, event::{Broadcaster, Sequence, repo::Provider as _}, message::repo::Provider as _, - name::{self, Name}, + name::Name, }; pub struct Conversations { @@ -34,8 +31,12 @@ impl Conversations { return Err(CreateError::InvalidName(name.clone())); } - let mut tx = self.db.begin().await?; - let created = tx.sequence().next(created_at).await?; + let mut tx = self.db.begin().await.fail(db::failed::BEGIN)?; + let created = tx + .sequence() + .next(created_at) + .await + .fail("Failed to find event sequence number")?; let conversation = History::begin(name, created); // This filter technically includes every event in the history, but it's easier to follow if @@ -45,9 +46,12 @@ impl Conversations { tx.conversations() .record_events(events.clone()) .await - .duplicate(|| CreateError::DuplicateName(name.clone()))?; + .map_err(|err| match err.as_database_error() { + Some(err) if err.is_unique_violation() => CreateError::DuplicateName(name.clone()), + _ => err.fail("Failed to store events"), + })?; - tx.commit().await?; + tx.commit().await.fail(db::failed::COMMIT)?; self.events.broadcast_from(events); @@ -56,17 +60,21 @@ impl Conversations { // This function is careless with respect to time, and gets you the // conversation as it exists in the specific moment when you call it. - pub async fn get(&self, conversation: &Id) -> Result<Conversation, Error> { - let to_not_found = || Error::NotFound(conversation.clone()); - let to_deleted = || Error::Deleted(conversation.clone()); + pub async fn get(&self, conversation: &Id) -> Result<Conversation, GetError> { + let to_not_found = || GetError::NotFound(conversation.clone()); + let to_deleted = || GetError::Deleted(conversation.clone()); + + let mut tx = self.db.begin().await.fail(db::failed::BEGIN)?; - let mut tx = self.db.begin().await?; let conversation = tx .conversations() .by_id(conversation) .await - .not_found(to_not_found)?; - tx.commit().await?; + .optional() + .fail("Failed to load conversation")? + .ok_or_else(to_not_found)?; + + tx.commit().await.fail(db::failed::COMMIT)?; conversation.as_snapshot().ok_or_else(to_deleted) } @@ -76,16 +84,28 @@ impl Conversations { conversation: &Id, deleted_at: &DateTime, ) -> Result<(), DeleteError> { - let mut tx = self.db.begin().await?; + let to_not_found = || DeleteError::NotFound(conversation.clone()); + + let mut tx = self.db.begin().await.fail(db::failed::BEGIN)?; let conversation = tx .conversations() .by_id(conversation) .await - .not_found(|| DeleteError::NotFound(conversation.clone()))?; + .optional() + .fail("Failed to load conversation")? + .ok_or_else(to_not_found)?; - let messages = tx.messages().live(&conversation).await?; - let deleted_at = tx.sequence().next(deleted_at).await?; + let messages = tx + .messages() + .live(&conversation) + .await + .fail("Failed to load messages")?; + let deleted_at = tx + .sequence() + .next(deleted_at) + .await + .fail("Failed to find event sequence number")?; let has_messages = messages .iter() @@ -100,9 +120,12 @@ impl Conversations { let events = conversation .events() .filter(Sequence::start_from(deleted_at)); - tx.conversations().record_events(events.clone()).await?; + tx.conversations() + .record_events(events.clone()) + .await + .fail("Failed to store events")?; - tx.commit().await?; + tx.commit().await.fail(db::failed::COMMIT)?; self.events.broadcast_from(events); @@ -114,23 +137,33 @@ impl Conversations { // expired until their messages expire. let expire_at = relative_to.to_owned() - TimeDelta::days(7); - let mut tx = self.db.begin().await?; - let expired = tx.conversations().expired(&expire_at).await?; + let mut tx = self.db.begin().await.fail(db::failed::BEGIN)?; + + let expired = tx + .conversations() + .expired(&expire_at) + .await + .fail("Failed to load expirable conversations")?; let mut events = Vec::with_capacity(expired.len()); for conversation in expired { - let deleted = tx.sequence().next(relative_to).await?; + let deleted = tx + .sequence() + .next(relative_to) + .await + .fail("Failed to find event sequence number")?; let conversation = conversation.delete(deleted)?; let conversation_events = conversation.events().filter(Sequence::start_from(deleted)); tx.conversations() .record_events(conversation_events.clone()) - .await?; + .await + .fail("Failed to store events")?; events.push(conversation_events); } - tx.commit().await?; + tx.commit().await.fail(db::failed::COMMIT)?; self.events .broadcast_from(events.into_iter().kmerge_by(Sequence::merge)); @@ -157,39 +190,17 @@ pub enum CreateError { #[error("invalid conversation name: {0}")] InvalidName(Name), #[error(transparent)] - Database(#[from] sqlx::Error), - #[error(transparent)] - Name(#[from] name::Error), -} - -impl From<LoadError> for CreateError { - fn from(error: LoadError) -> Self { - match error { - LoadError::Database(error) => error.into(), - LoadError::Name(error) => error.into(), - } - } + Failed(#[from] Failed), } #[derive(Debug, thiserror::Error)] -pub enum Error { +pub enum GetError { #[error("conversation {0} not found")] NotFound(Id), #[error("conversation {0} deleted")] Deleted(Id), #[error(transparent)] - Database(#[from] sqlx::Error), - #[error(transparent)] - Name(#[from] name::Error), -} - -impl From<LoadError> for Error { - fn from(error: LoadError) -> Self { - match error { - LoadError::Database(error) => error.into(), - LoadError::Name(error) => error.into(), - } - } + Failed(#[from] Failed), } #[derive(Debug, thiserror::Error)] @@ -201,51 +212,27 @@ pub enum DeleteError { #[error("conversation {0} not empty")] NotEmpty(Id), #[error(transparent)] - Database(#[from] sqlx::Error), - #[error(transparent)] - Name(#[from] name::Error), -} - -impl From<LoadError> for DeleteError { - fn from(error: LoadError) -> Self { - match error { - LoadError::Database(error) => error.into(), - LoadError::Name(error) => error.into(), - } - } + Failed(#[from] Failed), } impl From<history::DeleteError> for DeleteError { fn from(error: history::DeleteError) -> Self { - match error { - history::DeleteError::Deleted(conversation) => Self::Deleted(conversation.id().clone()), - } + let history::DeleteError::Deleted(conversation) = error; + Self::Deleted(conversation.id().clone()) } } #[derive(Debug, thiserror::Error)] pub enum ExpireError { - #[error("tried to expire already-deleted conversation: {0}")] + #[error("tried to expire already-deleted conversation {0}")] Deleted(Id), #[error(transparent)] - Database(#[from] sqlx::Error), - #[error(transparent)] - Name(#[from] name::Error), -} - -impl From<LoadError> for ExpireError { - fn from(error: LoadError) -> Self { - match error { - LoadError::Database(error) => error.into(), - LoadError::Name(error) => error.into(), - } - } + Failed(#[from] Failed), } impl From<history::DeleteError> for ExpireError { fn from(error: history::DeleteError) -> Self { - match error { - history::DeleteError::Deleted(conversation) => Self::Deleted(conversation.id().clone()), - } + let history::DeleteError::Deleted(conversation) = error; + Self::Deleted(conversation.id().clone()) } } diff --git a/src/conversation/handlers/create/mod.rs b/src/conversation/handlers/create/mod.rs index 2b7fa39..aa77411 100644 --- a/src/conversation/handlers/create/mod.rs +++ b/src/conversation/handlers/create/mod.rs @@ -57,9 +57,7 @@ impl IntoResponse for Error { app::CreateError::InvalidName(_) => { (StatusCode::BAD_REQUEST, error.to_string()).into_response() } - app::CreateError::Name(_) | app::CreateError::Database(_) => { - Internal::from(error).into_response() - } + app::CreateError::Failed(_) => Internal::from(error).into_response(), } } } diff --git a/src/conversation/handlers/delete/mod.rs b/src/conversation/handlers/delete/mod.rs index 231e433..2365303 100644 --- a/src/conversation/handlers/delete/mod.rs +++ b/src/conversation/handlers/delete/mod.rs @@ -50,9 +50,7 @@ impl IntoResponse for Error { app::DeleteError::NotEmpty(_) => { (StatusCode::CONFLICT, error.to_string()).into_response() } - app::DeleteError::Name(_) | app::DeleteError::Database(_) => { - Internal::from(error).into_response() - } + app::DeleteError::Failed(_) => Internal::from(error).into_response(), } } } diff --git a/src/conversation/handlers/send/mod.rs b/src/conversation/handlers/send/mod.rs index ff63652..979dd24 100644 --- a/src/conversation/handlers/send/mod.rs +++ b/src/conversation/handlers/send/mod.rs @@ -58,10 +58,9 @@ impl IntoResponse for Error { SendError::ConversationNotFound(_) | SendError::ConversationDeleted(_) => { NotFound(error).into_response() } - SendError::SenderNotFound(_) - | SendError::SenderDeleted(_) - | SendError::Name(_) - | SendError::Database(_) => Internal::from(error).into_response(), + SendError::SenderNotFound(_) | SendError::SenderDeleted(_) | SendError::Failed(_) => { + Internal::from(error).into_response() + } } } } diff --git a/src/db/failed.rs b/src/db/failed.rs new file mode 100644 index 0000000..f917ed1 --- /dev/null +++ b/src/db/failed.rs @@ -0,0 +1,2 @@ +pub const BEGIN: &str = "Failed to start transaction"; +pub const COMMIT: &str = "Failed to commit transaction"; diff --git a/src/db/mod.rs b/src/db/mod.rs index 9a74dcb..98ec618 100644 --- a/src/db/mod.rs +++ b/src/db/mod.rs @@ -1,9 +1,9 @@ mod backup; +pub mod failed; use std::str::FromStr; use sqlx::{ - error::{DatabaseError, ErrorKind}, migrate::MigrateDatabase as _, sqlite::{Sqlite, SqliteConnectOptions, SqlitePool, SqlitePoolOptions}, }; @@ -66,14 +66,6 @@ pub trait NotFound: Sized { type Ok; type Error; - fn not_found<E, F>(self, map: F) -> Result<Self::Ok, E> - where - E: From<Self::Error>, - F: FnOnce() -> E, - { - self.optional()?.ok_or_else(map) - } - fn optional(self) -> Result<Option<Self::Ok>, Self::Error>; } @@ -89,32 +81,3 @@ impl<T> NotFound for Result<T, sqlx::Error> { } } } - -pub trait Duplicate { - type Ok; - type Error; - - fn duplicate<E, F>(self, map: F) -> Result<Self::Ok, E> - where - E: From<Self::Error>, - F: FnOnce() -> E; -} - -impl<T> Duplicate for Result<T, sqlx::Error> { - type Ok = T; - type Error = sqlx::Error; - - fn duplicate<E, F>(self, map: F) -> Result<T, E> - where - E: From<sqlx::Error>, - F: FnOnce() -> E, - { - match self { - Ok(value) => Ok(value), - Err(error) => match error.as_database_error().map(DatabaseError::kind) { - Some(ErrorKind::UniqueViolation) => Err(map()), - _ => Err(error.into()), - }, - } - } -} diff --git a/src/error/failed.rs b/src/error/failed.rs new file mode 100644 index 0000000..4d55552 --- /dev/null +++ b/src/error/failed.rs @@ -0,0 +1,155 @@ +use std::{borrow::Cow, error::Error, fmt}; + +use super::BoxedError; + +// Don't bother allocating a String for each error if we have a static error message, which is +// a fairly common use case. +pub type Message = Cow<'static, str>; + +// Intended to represent a generic failure caused by some underlying cause, tagged with a +// context-appropriate message to help operators and developers diagnose and repair the issue. +// Unlike `Internal`, this error type has no default HTTP representation - it's not meant for that +// use case: the intended audience for a `Failed` and its underlying cause is the service operator, +// not the conversational users or client developers. +// +// The underlying cause will be used as the `Failed` error's `source()`, and the provided message +// will be used as its `Display` implementation. +// +// Ways to make a `Failed`: +// +// * Call `ResultExt::fail` or `ResultExt::fail_with` on a result to convert its error type (most +// preferred). +// * Call `ErrorExt::fail` to convert an existing error into `Failed`. +// * Call `Result::map_err` with `Failed::with_message` or `Failed::with_message_from` to convert +// its error type to `Failed`. +// * Call `Failed::new` with a message and an existing error (least preferred). +// +// Ways to use `Failed`: +// +// * As the error type of a `Result` return value, when all errors that can arise in a function +// are of interest to the operator only but there are multiple failure types to contend with. +// * As the error type of a `Result` return value, when there's only one error that can arise but +// you want to annotate that error with a contextual message, like what the function was doing at +// the time. +// * As a variant in am error enum, when _some_ errors are only of operator interest but other +// errors may need to be broken out for matching, or returned to clients. +// +// When not to use `Failed`: +// +// * When the caller needs to match on the specific error type. `Failed`, by design, assumes that +// nothing up the call stack will care about the structure of the error, and while you _can_ get +// at the underlying error to check what you ahve by going through the `source()` chain, it's +// tricky to do well. If the specific error is relevant to the caller, surface it through the +// error type, instead. +#[derive(Debug)] +pub struct Failed { + source: BoxedError, + message: Message, +} + +impl Failed { + pub fn new<M, E>(message: M, source: E) -> Self + where + M: Into<Message>, + E: Into<BoxedError>, + { + Self { + source: source.into(), + message: message.into(), + } + } +} + +// Adapters for use with `Result::map_err`. +impl Failed { + // The returned adaptor wraps an error with `Failed`, using the provided message. + pub fn with_message<M, E>(message: M) -> impl FnOnce(E) -> Self + where + M: Into<Message>, + E: Into<BoxedError>, + { + |source| Self::new(message, source) + } + + // The returned adaptor wraps an error with `Failed`, using a message from the provided message + // callback. + pub fn with_message_from<F, M, E>(message_fn: F) -> impl FnOnce(E) -> Self + where + F: FnOnce() -> M, + M: Into<Message>, + E: Into<BoxedError>, + { + |source| Self::new(message_fn(), source) + } +} + +impl Error for Failed { + fn source(&self) -> Option<&(dyn Error + 'static)> { + Some(self.source.as_ref()) + } +} + +impl fmt::Display for Failed { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + self.message.fmt(f) + } +} + +pub trait ErrorExt { + // Convenience for `Failed::new(message, err).into()`, when wrapping an existing error into + // `Failed`. + fn fail<M, T>(self, message: M) -> T + where + M: Into<Message>, + T: From<Failed>; +} + +impl<E> ErrorExt for E +where + E: Into<BoxedError>, +{ + fn fail<M, T>(self, message: M) -> T + where + M: Into<Message>, + T: From<Failed>, + { + Failed::new(message, self).into() + } +} + +pub trait ResultExt { + type Ok; + + // Convenience for `.map_err(Failed::with_message(message))`, when a result's error type can be + // collapsed into `Failed`. + fn fail<M>(self, message: M) -> Result<Self::Ok, Failed> + where + M: Into<Message>; + + fn fail_with<F, M>(self, message_fn: F) -> Result<Self::Ok, Failed> + where + F: FnOnce() -> M, + M: Into<Message>; +} + +impl<T, E> ResultExt for Result<T, E> +where + E: Into<BoxedError>, +{ + type Ok = T; + + fn fail<M>(self, message: M) -> Result<T, Failed> + where + M: Into<Message>, + { + self.map_err(Failed::with_message(message)) + } + + fn fail_with<F, M>(self, message_fn: F) -> Result<T, Failed> + where + F: FnOnce() -> M, + M: Into<Message>, + { + self.map_err(Failed::with_message_from(message_fn)) + } +} diff --git a/src/error/mod.rs b/src/error/mod.rs index 154f98f..606e344 100644 --- a/src/error/mod.rs +++ b/src/error/mod.rs @@ -6,6 +6,7 @@ use axum::{ }; pub mod chain; +pub mod failed; // I'm making an effort to avoid `anyhow` here, as that crate is _enormously_ // complex (though very usable). We don't need to be overly careful about diff --git a/src/event/app.rs b/src/event/app.rs index e422de9..386d749 100644 --- a/src/event/app.rs +++ b/src/event/app.rs @@ -8,9 +8,9 @@ use sqlx::sqlite::SqlitePool; use super::{Event, Sequence, Sequenced, broadcaster::Broadcaster}; use crate::{ conversation::{self, repo::Provider as _}, - db::NotFound, + db::{self, NotFound as _}, + error::failed::{Failed, ResultExt as _}, message::{self, repo::Provider as _}, - name, user::{self, repo::Provider as _}, vapid, vapid::repo::Provider as _, @@ -29,14 +29,18 @@ impl Events { pub async fn subscribe( &self, resume_at: Sequence, - ) -> Result<impl Stream<Item = Event> + std::fmt::Debug + use<>, Error> { + ) -> Result<impl Stream<Item = Event> + std::fmt::Debug + use<>, Failed> { // Subscribe before retrieving, to catch messages broadcast while we're // querying the DB. We'll prune out duplicates later. let live_messages = self.events.subscribe(); - let mut tx = self.db.begin().await?; + let mut tx = self.db.begin().await.fail(db::failed::BEGIN)?; - let users = tx.users().replay(resume_at).await?; + let users = tx + .users() + .replay(resume_at) + .await + .fail("Failed to load user events")?; let user_events = users .iter() .map(user::History::events) @@ -44,7 +48,11 @@ impl Events { .filter(Sequence::after(resume_at)) .map(Event::from); - let conversations = tx.conversations().replay(resume_at).await?; + let conversations = tx + .conversations() + .replay(resume_at) + .await + .fail("Failed to load conversation events")?; let conversation_events = conversations .iter() .map(conversation::History::events) @@ -52,7 +60,11 @@ impl Events { .filter(Sequence::after(resume_at)) .map(Event::from); - let messages = tx.messages().replay(resume_at).await?; + let messages = tx + .messages() + .replay(resume_at) + .await + .fail("Failed to load message events")?; let message_events = messages .iter() .map(message::History::events) @@ -60,7 +72,12 @@ impl Events { .filter(Sequence::after(resume_at)) .map(Event::from); - let vapid = tx.vapid().current().await.optional()?; + let vapid = tx + .vapid() + .current() + .await + .optional() + .fail("Failed to load VAPID key events")?; let vapid_events = vapid .iter() .flat_map(vapid::History::events) @@ -91,45 +108,3 @@ impl Events { move |event| future::ready(filter(event)) } } - -#[derive(Debug, thiserror::Error)] -#[error(transparent)] -pub enum Error { - Database(#[from] sqlx::Error), - Name(#[from] name::Error), - Ecdsa(#[from] p256::ecdsa::Error), - Pkcs8(#[from] p256::pkcs8::Error), - WebPush(#[from] web_push::WebPushError), -} - -impl From<user::repo::LoadError> for Error { - fn from(error: user::repo::LoadError) -> Self { - use user::repo::LoadError; - match error { - LoadError::Database(error) => error.into(), - LoadError::Name(error) => error.into(), - } - } -} - -impl From<conversation::repo::LoadError> for Error { - fn from(error: conversation::repo::LoadError) -> Self { - use conversation::repo::LoadError; - match error { - LoadError::Database(error) => error.into(), - LoadError::Name(error) => error.into(), - } - } -} - -impl From<vapid::repo::Error> for Error { - fn from(error: vapid::repo::Error) -> Self { - use vapid::repo::Error; - match error { - Error::Database(error) => error.into(), - Error::Ecdsa(error) => error.into(), - Error::Pkcs8(error) => error.into(), - Error::WebPush(error) => error.into(), - } - } -} diff --git a/src/event/handlers/stream/mod.rs b/src/event/handlers/stream/mod.rs index 8b89c31..dde4fae 100644 --- a/src/event/handlers/stream/mod.rs +++ b/src/event/handlers/stream/mod.rs @@ -10,8 +10,8 @@ use futures::stream::{Stream, StreamExt as _}; use crate::{ app::App, - error::{Internal, Unauthorized}, - event::{Event, Heartbeat::Heartbeat, Sequence, Sequenced as _, app, extract::LastEventId}, + error::{Internal, Unauthorized, failed::Failed}, + event::{Event, Heartbeat::Heartbeat, Sequence, Sequenced as _, extract::LastEventId}, token::{app::ValidateError, extract::Identity}, }; @@ -71,7 +71,7 @@ impl TryFrom<Event> for sse::Event { #[derive(Debug, thiserror::Error)] #[error(transparent)] pub enum Error { - Subscribe(#[from] app::Error), + Subscribe(#[from] Failed), Validate(#[from] ValidateError), } @@ -79,7 +79,9 @@ impl IntoResponse for Error { fn into_response(self) -> response::Response { match self { Self::Validate(ValidateError::InvalidToken) => Unauthorized.into_response(), - other => Internal::from(other).into_response(), + Self::Validate(ValidateError::Failed(_)) | Self::Subscribe(_) => { + Internal::from(self).into_response() + } } } } diff --git a/src/invite/app.rs b/src/invite/app.rs index 6f58d0a..54272bc 100644 --- a/src/invite/app.rs +++ b/src/invite/app.rs @@ -4,17 +4,14 @@ use sqlx::sqlite::SqlitePool; use super::{Id, Invite, Summary, repo::Provider as _}; use crate::{ clock::DateTime, - db::{Duplicate as _, NotFound as _}, + db::{self, NotFound as _}, + error::failed::{ErrorExt as _, Failed, ResultExt as _}, event::{Broadcaster, repo::Provider as _}, login::Login, - name::{self, Name}, + name::Name, password::Password, token::{Secret, Token, repo::Provider as _}, - user::{ - self, - create::{self, Create}, - repo::{LoadError, Provider as _}, - }, + user::{self, create, create::Create, repo::Provider as _}, }; pub struct Invites { @@ -27,20 +24,30 @@ impl Invites { Self { db, events } } - pub async fn issue(&self, issuer: &Login, issued_at: &DateTime) -> Result<Invite, Error> { - let issuer_not_found = || Error::IssuerNotFound(issuer.id.clone().into()); - let issuer_deleted = || Error::IssuerDeleted(issuer.id.clone().into()); + pub async fn issue(&self, issuer: &Login, issued_at: &DateTime) -> Result<Invite, IssueError> { + let issuer_not_found = || IssueError::IssuerNotFound(issuer.id.clone().into()); + let issuer_deleted = || IssueError::IssuerDeleted(issuer.id.clone().into()); - let mut tx = self.db.begin().await?; + let mut tx = self.db.begin().await.fail(db::failed::BEGIN)?; let issuer = tx .users() .by_login(issuer) .await - .not_found(issuer_not_found)?; - let now = tx.sequence().current().await?; + .optional() + .fail("Failed to load issuing user")? + .ok_or_else(issuer_not_found)?; + let now = tx + .sequence() + .current() + .await + .fail("Failed to find event sequence number")?; let issuer = issuer.as_of(now).ok_or_else(issuer_deleted)?; - let invite = tx.invites().create(&issuer, issued_at).await?; - tx.commit().await?; + let invite = tx + .invites() + .create(&issuer, issued_at) + .await + .fail("Failed to store new invitation")?; + tx.commit().await.fail(db::failed::COMMIT)?; Ok(invite) } @@ -60,33 +67,52 @@ impl Invites { password: &Password, accepted_at: &DateTime, ) -> Result<Secret, AcceptError> { + let to_not_found = || AcceptError::NotFound(invite.clone()); + let create = Create::begin(name, password, accepted_at); - let mut tx = self.db.begin().await?; + let mut tx = self.db.begin().await.fail(db::failed::BEGIN)?; let invite = tx .invites() .by_id(invite) .await - .not_found(|| AcceptError::NotFound(invite.clone()))?; + .optional() + .fail("Failed to load invitation")? + .ok_or_else(to_not_found)?; // Split the tx here so we don't block writes while we deal with the password, // and don't deal with the password until we're fairly confident we can accept // the invite. Final validation is in the next tx. - tx.commit().await?; + tx.commit().await.fail(db::failed::COMMIT)?; - let validated = create.validate()?; + let validated = create.validate().map_err(|err| match err { + create::Error::InvalidName(name) => AcceptError::InvalidName(name), + create::Error::Failed(_) => err.fail("Failed to validate invited user"), + })?; - let mut tx = self.db.begin().await?; + let mut tx = self.db.begin().await.fail(db::failed::BEGIN)?; // If the invite has been deleted or accepted in the interim, this step will // catch it. - tx.invites().accept(&invite).await?; - let stored = validated - .store(&mut tx) + tx.invites() + .accept(&invite) .await - .duplicate(|| AcceptError::DuplicateLogin(name.clone()))?; + .fail("Failed to remove accepted invitation")?; + let stored = + validated + .store(&mut tx) + .await + .map_err(|err| match err.as_database_error() { + Some(err) if err.is_unique_violation() => { + AcceptError::DuplicateLogin(name.clone()) + } + _ => err.fail("Failed to store invited user"), + })?; let login = stored.login(); let (token, secret) = Token::generate(login, accepted_at); - tx.tokens().create(&token, &secret).await?; - tx.commit().await?; + tx.tokens() + .create(&token, &secret) + .await + .fail("Failed to issue token for invited user")?; + tx.commit().await.fail(db::failed::COMMIT)?; stored.publish(&self.events); @@ -106,25 +132,13 @@ impl Invites { } #[derive(Debug, thiserror::Error)] -pub enum Error { +pub enum IssueError { #[error("issuing user {0} not found")] IssuerNotFound(user::Id), #[error("issuing user {0} deleted")] IssuerDeleted(user::Id), #[error(transparent)] - Database(#[from] sqlx::Error), - #[error(transparent)] - Name(#[from] name::Error), -} - -impl From<user::repo::LoadError> for Error { - fn from(error: LoadError) -> Self { - use user::repo::LoadError; - match error { - LoadError::Database(error) => error.into(), - LoadError::Name(error) => error.into(), - } - } + Failed(#[from] Failed), } #[derive(Debug, thiserror::Error)] @@ -136,16 +150,5 @@ pub enum AcceptError { #[error("name in use: {0}")] DuplicateLogin(Name), #[error(transparent)] - Database(#[from] sqlx::Error), - #[error(transparent)] - PasswordHash(#[from] password_hash::Error), -} - -impl From<create::Error> for AcceptError { - fn from(error: create::Error) -> Self { - match error { - create::Error::InvalidName(name) => Self::InvalidName(name), - create::Error::PasswordHash(error) => Self::PasswordHash(error), - } - } + Failed(#[from] Failed), } diff --git a/src/invite/handlers/accept/mod.rs b/src/invite/handlers/accept/mod.rs index 8bdaa51..34f1e94 100644 --- a/src/invite/handlers/accept/mod.rs +++ b/src/invite/handlers/accept/mod.rs @@ -53,7 +53,7 @@ impl IntoResponse for Error { app::AcceptError::DuplicateLogin(_) => { (StatusCode::CONFLICT, error.to_string()).into_response() } - other => Internal::from(other).into_response(), + app::AcceptError::Failed(_) => Internal::from(error).into_response(), } } } diff --git a/src/login/app.rs b/src/login/app.rs index 8cc8cd0..5c012c4 100644 --- a/src/login/app.rs +++ b/src/login/app.rs @@ -2,9 +2,11 @@ use sqlx::sqlite::SqlitePool; use crate::{ clock::DateTime, + db, db::NotFound as _, - login::{self, Login, repo::Provider as _}, - name::{self, Name}, + error::failed::{Failed, ResultExt as _}, + login::{Login, repo::Provider as _}, + name::Name, password::Password, push::repo::Provider as _, token::{Broadcaster, Event as TokenEvent, Secret, Token, repo::Provider as _}, @@ -26,24 +28,32 @@ impl Logins { candidate: &Password, login_at: &DateTime, ) -> Result<Secret, LoginError> { - let mut tx = self.db.begin().await?; + let mut tx = self.db.begin().await.fail(db::failed::BEGIN)?; let (login, password) = tx .logins() .by_name(name) .await - .not_found(|| LoginError::Rejected)?; + .optional() + .fail("Failed to load login")? + .ok_or_else(|| LoginError::Rejected)?; // Split the transaction here to avoid holding the tx open (potentially blocking // other writes) while we do the fairly expensive task of verifying the // password. It's okay if the token issuance transaction happens some notional // amount of time after retrieving the login, as inserting the token will fail // if the account is deleted during that time. - tx.commit().await?; + tx.commit().await.fail(db::failed::COMMIT)?; - if password.verify(candidate)? { - let mut tx = self.db.begin().await?; + if password + .verify(candidate) + .fail("Failed to verify password")? + { + let mut tx = self.db.begin().await.fail(db::failed::BEGIN)?; let (token, secret) = Token::generate(&login, login_at); - tx.tokens().create(&token, &secret).await?; - tx.commit().await?; + tx.tokens() + .create(&token, &secret) + .await + .fail("Failed to create token")?; + tx.commit().await.fail(db::failed::COMMIT)?; Ok(secret) } else { Err(LoginError::Rejected) @@ -57,30 +67,47 @@ impl Logins { to: &Password, changed_at: &DateTime, ) -> Result<Secret, LoginError> { - let mut tx = self.db.begin().await?; + let mut tx = self.db.begin().await.fail(db::failed::BEGIN)?; let (login, password) = tx .logins() .by_id(&login.id) .await - .not_found(|| LoginError::Rejected)?; + .optional() + .fail("Failed to load login")? + .ok_or_else(|| LoginError::Rejected)?; // Split the transaction here to avoid holding the tx open (potentially blocking // other writes) while we do the fairly expensive task of verifying the // password. It's okay if the token issuance transaction happens some notional // amount of time after retrieving the login, as inserting the token will fail // if the account is deleted during that time. - tx.commit().await?; + tx.commit().await.fail(db::failed::COMMIT)?; - if password.verify(from)? { - let to_hash = to.hash()?; + if password + .verify(from) + .fail("Failed to verify current password")? + { + let to_hash = to.hash().fail("Failed to digest new password")?; let (token, secret) = Token::generate(&login, changed_at); - let mut tx = self.db.begin().await?; - tx.logins().set_password(&login, &to_hash).await?; - - tx.push().unsubscribe_login(&login).await?; - let revoked = tx.tokens().revoke_all(&login).await?; - tx.tokens().create(&token, &secret).await?; - tx.commit().await?; + let mut tx = self.db.begin().await.fail(db::failed::BEGIN)?; + tx.logins() + .set_password(&login, &to_hash) + .await + .fail("Failed to store new password")?; + tx.push() + .unsubscribe_login(&login) + .await + .fail("Failed to remove push notification subscriptions")?; + let revoked = tx + .tokens() + .revoke_all(&login) + .await + .fail("Failed to revoke existing tokens")?; + tx.tokens() + .create(&token, &secret) + .await + .fail("Failed to create new token")?; + tx.commit().await.fail(db::failed::COMMIT)?; revoked .into_iter() @@ -99,19 +126,5 @@ pub enum LoginError { #[error("invalid login")] Rejected, #[error(transparent)] - Database(#[from] sqlx::Error), - #[error(transparent)] - Name(#[from] name::Error), - #[error(transparent)] - PasswordHash(#[from] password_hash::Error), -} - -impl From<login::repo::LoadError> for LoginError { - fn from(error: login::repo::LoadError) -> Self { - use login::repo::LoadError; - match error { - LoadError::Database(error) => error.into(), - LoadError::Name(error) => error.into(), - } - } + Failed(#[from] Failed), } diff --git a/src/login/handlers/login/mod.rs b/src/login/handlers/login/mod.rs index 2ce8a67..d42cff5 100644 --- a/src/login/handlers/login/mod.rs +++ b/src/login/handlers/login/mod.rs @@ -49,7 +49,7 @@ impl IntoResponse for Error { // not error::Unauthorized due to differing messaging (StatusCode::UNAUTHORIZED, "invalid name or password").into_response() } - other => Internal::from(other).into_response(), + app::LoginError::Failed(_) => Internal::from(error).into_response(), } } } diff --git a/src/login/handlers/logout/mod.rs b/src/login/handlers/logout/mod.rs index ce4cb1a..4bd7a89 100644 --- a/src/login/handlers/logout/mod.rs +++ b/src/login/handlers/logout/mod.rs @@ -42,9 +42,7 @@ impl IntoResponse for Error { let Self(error) = self; match error { app::ValidateError::InvalidToken => Unauthorized.into_response(), - app::ValidateError::Name(_) | app::ValidateError::Database(_) => { - Internal::from(error).into_response() - } + app::ValidateError::Failed(_) => Internal::from(error).into_response(), } } } diff --git a/src/login/handlers/password/mod.rs b/src/login/handlers/password/mod.rs index 8b82605..7eed24a 100644 --- a/src/login/handlers/password/mod.rs +++ b/src/login/handlers/password/mod.rs @@ -48,7 +48,7 @@ impl IntoResponse for Error { app::LoginError::Rejected => { (StatusCode::BAD_REQUEST, "invalid name or password").into_response() } - other => Internal::from(other).into_response(), + app::LoginError::Failed(_) => Internal::from(error).into_response(), } } } diff --git a/src/message/app.rs b/src/message/app.rs index cbcbff9..b82fa83 100644 --- a/src/message/app.rs +++ b/src/message/app.rs @@ -6,10 +6,11 @@ use super::{Body, History, Id, Message, history, repo::Provider as _}; use crate::{ clock::DateTime, conversation::{self, repo::Provider as _}, + db, db::NotFound as _, + error::failed::{Failed, ResultExt as _}, event::{Broadcaster, Sequence, repo::Provider as _}, login::Login, - name, user::{self, repo::Provider as _}, }; @@ -35,21 +36,29 @@ impl Messages { let sender_not_found = || SendError::SenderNotFound(sender.id.clone().into()); let sender_deleted = || SendError::SenderDeleted(sender.id.clone().into()); - let mut tx = self.db.begin().await?; + let mut tx = self.db.begin().await.fail(db::failed::BEGIN)?; let conversation = tx .conversations() .by_id(conversation) .await - .not_found(conversation_not_found)?; + .optional() + .fail("Failed to load conversation")? + .ok_or_else(conversation_not_found)?; let sender = tx .users() .by_login(sender) .await - .not_found(sender_not_found)?; + .optional() + .fail("Failed to load sending user")? + .ok_or_else(sender_not_found)?; // Ordering: don't bother allocating a sequence number before we know the channel might // exist. - let sent = tx.sequence().next(sent_at).await?; + let sent = tx + .sequence() + .next(sent_at) + .await + .fail("Failed to find event sequence number")?; let conversation = conversation.as_of(sent).ok_or_else(conversation_deleted)?; let sender = sender.as_of(sent).ok_or_else(sender_deleted)?; let message = History::begin(&conversation, &sender, body, sent); @@ -58,9 +67,12 @@ impl Messages { // the various event-manipulating app methods are consistent, and it's harmless to have an // always-satisfied filter. let events = message.events().filter(Sequence::start_from(sent)); - tx.messages().record_events(events.clone()).await?; + tx.messages() + .record_events(events.clone()) + .await + .fail("Failed to store events")?; - tx.commit().await?; + tx.commit().await.fail(db::failed::COMMIT)?; self.events.broadcast_from(events); @@ -76,20 +88,29 @@ impl Messages { let message_not_found = || DeleteError::MessageNotFound(message.clone()); let not_sender = || DeleteError::NotSender(deleted_by.id.clone().into()); - let mut tx = self.db.begin().await?; + let mut tx = self.db.begin().await.fail(db::failed::BEGIN)?; let message = tx .messages() .by_id(message) .await - .not_found(message_not_found)?; + .optional() + .fail("Failed to load message")? + .ok_or_else(message_not_found)?; if message.sender() == &deleted_by.id { - let deleted_at = tx.sequence().next(deleted_at).await?; + let deleted_at = tx + .sequence() + .next(deleted_at) + .await + .fail("Failed to find event sequence number")?; let message = message.delete(deleted_at)?; let events = message.events().filter(Sequence::start_from(deleted_at)); - tx.messages().record_events(events.clone()).await?; - tx.commit().await?; + tx.messages() + .record_events(events.clone()) + .await + .fail("Failed to store events")?; + tx.commit().await.fail(db::failed::COMMIT)?; self.events.broadcast_from(events); @@ -153,29 +174,7 @@ pub enum SendError { #[error("user {0} deleted")] SenderDeleted(user::Id), #[error(transparent)] - Database(#[from] sqlx::Error), - #[error(transparent)] - Name(#[from] name::Error), -} - -impl From<conversation::repo::LoadError> for SendError { - fn from(error: conversation::repo::LoadError) -> Self { - use conversation::repo::LoadError; - match error { - LoadError::Database(error) => error.into(), - LoadError::Name(error) => error.into(), - } - } -} - -impl From<user::repo::LoadError> for SendError { - fn from(error: user::repo::LoadError) -> Self { - use user::repo::LoadError; - match error { - LoadError::Database(error) => error.into(), - LoadError::Name(error) => error.into(), - } - } + Failed(#[from] Failed), } #[derive(Debug, thiserror::Error)] @@ -187,19 +186,7 @@ pub enum DeleteError { #[error("message {0} deleted")] Deleted(Id), #[error(transparent)] - Database(#[from] sqlx::Error), - #[error(transparent)] - Name(#[from] name::Error), -} - -impl From<user::repo::LoadError> for DeleteError { - fn from(error: user::repo::LoadError) -> Self { - use user::repo::LoadError; - match error { - LoadError::Database(error) => error.into(), - LoadError::Name(error) => error.into(), - } - } + Failed(#[from] Failed), } impl From<history::DeleteError> for DeleteError { diff --git a/src/message/handlers/delete/mod.rs b/src/message/handlers/delete/mod.rs index c09a752..c680db1 100644 --- a/src/message/handlers/delete/mod.rs +++ b/src/message/handlers/delete/mod.rs @@ -53,9 +53,7 @@ impl IntoResponse for Error { DeleteError::MessageNotFound(_) | DeleteError::Deleted(_) => { NotFound(error).into_response() } - DeleteError::Database(_) | DeleteError::Name(_) => { - Internal::from(error).into_response() - } + DeleteError::Failed(_) => Internal::from(error).into_response(), } } } diff --git a/src/push/app.rs b/src/push/app.rs index 56b9a02..5e57800 100644 --- a/src/push/app.rs +++ b/src/push/app.rs @@ -8,7 +8,14 @@ use web_push::{ }; use super::repo::Provider as _; -use crate::{login::Login, token::extract::Identity, vapid, vapid::repo::Provider as _}; +use crate::{ + db, + error::failed::{ErrorExt, Failed, ResultExt as _}, + login::Login, + token::extract::Identity, + vapid, + vapid::repo::Provider as _, +}; pub struct Push<P> { db: SqlitePool, @@ -26,9 +33,13 @@ impl<P> Push<P> { subscription: &SubscriptionInfo, vapid: &VerifyingKey, ) -> Result<(), SubscribeError> { - let mut tx = self.db.begin().await?; + let mut tx = self.db.begin().await.fail(db::failed::BEGIN)?; - let current = tx.vapid().current().await?; + let current = tx + .vapid() + .current() + .await + .fail("Failed to load current VAPID key")?; if vapid != ¤t.key { return Err(SubscribeError::StaleVapidKey(current.key)); } @@ -42,7 +53,8 @@ impl<P> Push<P> { let current = tx .push() .by_endpoint(&subscriber.login, &subscription.endpoint) - .await?; + .await + .fail("Failed to load existing subscriptions for endpoint")?; // If we already have a subscription for this endpoint, with _different_ // parameters, then this is a client error. They shouldn't reuse endpoint URLs, // per the various RFCs. @@ -55,12 +67,12 @@ impl<P> Push<P> { return Err(SubscribeError::Duplicate); } } else { - return Err(SubscribeError::Database(err)); + return Err(err.fail("Failed to create push subscription")); } } } - tx.commit().await?; + tx.commit().await.fail(db::failed::COMMIT)?; Ok(()) } @@ -138,10 +150,6 @@ where #[derive(Debug, thiserror::Error)] pub enum SubscribeError { - #[error(transparent)] - Database(#[from] sqlx::Error), - #[error(transparent)] - Vapid(#[from] vapid::repo::Error), #[error("subscription created with stale VAPID key")] StaleVapidKey(VerifyingKey), #[error("subscription already exists for endpoint")] @@ -149,6 +157,8 @@ pub enum SubscribeError { // and we want to limit its proliferation. The only intended recipient of this message is the // client, which already knows the endpoint anyways and doesn't need us to tell them. Duplicate, + #[error(transparent)] + Failed(#[from] Failed), } #[derive(Debug, thiserror::Error)] diff --git a/src/push/handlers/subscribe/mod.rs b/src/push/handlers/subscribe/mod.rs index a1a5899..6dcab06 100644 --- a/src/push/handlers/subscribe/mod.rs +++ b/src/push/handlers/subscribe/mod.rs @@ -81,7 +81,7 @@ impl IntoResponse for Error { app::SubscribeError::Duplicate => { (StatusCode::CONFLICT, err.to_string()).into_response() } - other => Internal::from(other).into_response(), + app::SubscribeError::Failed(_) => Internal::from(err).into_response(), } } } diff --git a/src/setup/app.rs b/src/setup/app.rs index 9539406..a0245f7 100644 --- a/src/setup/app.rs +++ b/src/setup/app.rs @@ -3,11 +3,13 @@ use sqlx::sqlite::SqlitePool; use super::repo::Provider as _; use crate::{ clock::DateTime, + db, + error::failed::{ErrorExt, Failed, ResultExt as _}, event::Broadcaster, name::Name, password::Password, token::{Secret, Token, repo::Provider as _}, - user::create::{self, Create}, + user::{create, create::Create}, }; #[derive(Clone)] @@ -29,18 +31,32 @@ impl Setup { ) -> Result<Secret, Error> { let create = Create::begin(name, password, created_at); - let validated = create.validate()?; + let validated = create.validate().map_err(|err| match err { + create::Error::InvalidName(name) => Error::InvalidName(name), + create::Error::Failed(_) => err.fail("Failed to validate new user"), + })?; - let mut tx = self.db.begin().await?; - if tx.setup().completed().await? { + let mut tx = self.db.begin().await.fail(db::failed::BEGIN)?; + if tx + .setup() + .completed() + .await + .fail("Failed to check for prior setup completion")? + { Err(Error::SetupCompleted) } else { - let stored = validated.store(&mut tx).await?; + let stored = validated + .store(&mut tx) + .await + .fail("Failed to create initial user")?; let login = stored.login(); let (token, secret) = Token::generate(login, created_at); - tx.tokens().create(&token, &secret).await?; - tx.commit().await?; + tx.tokens() + .create(&token, &secret) + .await + .fail("Failed to create token")?; + tx.commit().await.fail(db::failed::COMMIT)?; stored.publish(&self.events); @@ -64,16 +80,5 @@ pub enum Error { #[error("invalid user name: {0}")] InvalidName(Name), #[error(transparent)] - Database(#[from] sqlx::Error), - #[error(transparent)] - PasswordHash(#[from] password_hash::Error), -} - -impl From<create::Error> for Error { - fn from(error: create::Error) -> Self { - match error { - create::Error::InvalidName(name) => Self::InvalidName(name), - create::Error::PasswordHash(error) => Self::PasswordHash(error), - } - } + Failed(#[from] Failed), } diff --git a/src/setup/handlers/setup/mod.rs b/src/setup/handlers/setup/mod.rs index 2977da8..ce57e17 100644 --- a/src/setup/handlers/setup/mod.rs +++ b/src/setup/handlers/setup/mod.rs @@ -48,7 +48,7 @@ impl IntoResponse for Error { (StatusCode::BAD_REQUEST, error.to_string()).into_response() } app::Error::SetupCompleted => (StatusCode::CONFLICT, error.to_string()).into_response(), - other => Internal::from(other).into_response(), + app::Error::Failed(_) => Internal::from(error).into_response(), } } } diff --git a/src/token/app.rs b/src/token/app.rs index 4a08877..5916e53 100644 --- a/src/token/app.rs +++ b/src/token/app.rs @@ -6,11 +6,15 @@ use futures::{ use sqlx::sqlite::SqlitePool; use super::{ - Broadcaster, Event as TokenEvent, Secret, Token, - extract::Identity, - repo::{self, Provider as _}, + Broadcaster, Event as TokenEvent, Secret, Token, extract::Identity, repo::Provider as _, +}; +use crate::{ + clock::DateTime, + db, + db::NotFound as _, + error::failed::{Failed, ResultExt as _}, + push::repo::Provider as _, }; -use crate::{clock::DateTime, db::NotFound as _, name, push::repo::Provider as _}; pub struct Tokens { db: SqlitePool, @@ -27,13 +31,15 @@ impl Tokens { secret: &Secret, used_at: &DateTime, ) -> Result<Identity, ValidateError> { - let mut tx = self.db.begin().await?; + let mut tx = self.db.begin().await.fail(db::failed::BEGIN)?; let (token, login) = tx .tokens() .validate(secret, used_at) .await - .not_found(|| ValidateError::InvalidToken)?; - tx.commit().await?; + .optional() + .fail("Failed to load token")? + .ok_or(ValidateError::InvalidToken)?; + tx.commit().await.fail(db::failed::COMMIT)?; Ok(Identity { token, login }) } @@ -66,12 +72,14 @@ impl Tokens { // matter. Supervising a stream, on the other hand, will run for a // _long_ time; if we miss the race here, we'll never actually carry out the // supervision. - let mut tx = self.db.begin().await?; + let mut tx = self.db.begin().await.fail(db::failed::BEGIN)?; tx.tokens() .require(&token) .await - .not_found(|| ValidateError::InvalidToken)?; - tx.commit().await?; + .optional() + .fail("Failed to load token")? + .ok_or(ValidateError::InvalidToken)?; + tx.commit().await.fail(db::failed::COMMIT)?; // Then construct the guarded stream. First, project both streams into // `GuardedEvent`. @@ -111,10 +119,16 @@ impl Tokens { } pub async fn logout(&self, token: &Token) -> Result<(), ValidateError> { - let mut tx = self.db.begin().await?; - tx.push().unsubscribe_token(token).await?; - tx.tokens().revoke(token).await?; - tx.commit().await?; + let mut tx = self.db.begin().await.fail(db::failed::BEGIN)?; + tx.push() + .unsubscribe_token(token) + .await + .fail("Failed to remove push subscriptions")?; + tx.tokens() + .revoke(token) + .await + .fail("Failed to revoke token")?; + tx.commit().await.fail(db::failed::COMMIT)?; self.token_events .broadcast(TokenEvent::Revoked(token.id.clone())); @@ -128,18 +142,7 @@ pub enum ValidateError { #[error("invalid token")] InvalidToken, #[error(transparent)] - Database(#[from] sqlx::Error), - #[error(transparent)] - Name(#[from] name::Error), -} - -impl From<repo::LoadError> for ValidateError { - fn from(error: repo::LoadError) -> Self { - match error { - repo::LoadError::Database(error) => error.into(), - repo::LoadError::Name(error) => error.into(), - } - } + Failed(#[from] Failed), } #[derive(Debug)] diff --git a/src/token/extract/identity.rs b/src/token/extract/identity.rs index 5c004ef..59f1dfb 100644 --- a/src/token/extract/identity.rs +++ b/src/token/extract/identity.rs @@ -40,7 +40,7 @@ where .await .map_err(|err| match err { ValidateError::InvalidToken => LoginError::Unauthorized, - other => other.into(), + ValidateError::Failed(failed) => failed.into(), }) } } @@ -59,7 +59,7 @@ where match <Self as FromRequestParts<App>>::from_request_parts(parts, state).await { Ok(identity) => Ok(Some(identity)), Err(LoginError::Unauthorized) => Ok(None), - Err(other) => Err(other), + Err(LoginError::Failure(other)) => Err(LoginError::Failure(other)), } } } diff --git a/src/token/repo/mod.rs b/src/token/repo/mod.rs index 9df5bbb..50d9147 100644 --- a/src/token/repo/mod.rs +++ b/src/token/repo/mod.rs @@ -1,3 +1,3 @@ mod token; -pub use self::token::{LoadError, Provider}; +pub use self::token::Provider; diff --git a/src/ui/assets.rs b/src/ui/assets.rs index 0ca9593..ede1f7c 100644 --- a/src/ui/assets.rs +++ b/src/ui/assets.rs @@ -1,4 +1,4 @@ -use ::mime::{FromStrError, Mime}; +use ::mime::Mime; use axum::{ http::{StatusCode, header}, response::{IntoResponse, Response}, @@ -6,7 +6,10 @@ use axum::{ use rust_embed::EmbeddedFile; use super::{error::NotFound, mime}; -use crate::error::Internal; +use crate::error::{ + Internal, + failed::{Failed, ResultExt}, +}; #[derive(rust_embed::Embed)] #[folder = "$OUT_DIR/ui"] @@ -15,7 +18,7 @@ pub struct Assets; impl Assets { pub fn load(path: impl AsRef<str>) -> Result<Asset, Error> { let path = path.as_ref(); - let mime = mime::from_path(path)?; + let mime = mime::from_path(path).fail("Failed to determine MIME type from asset path")?; Self::get(path) .map(|file| Asset(mime, file)) @@ -49,14 +52,14 @@ pub enum Error { #[error("not found: {0}")] NotFound(String), #[error(transparent)] - Mime(#[from] FromStrError), + Failed(#[from] Failed), } impl IntoResponse for Error { fn into_response(self) -> Response { match self { Self::NotFound(_) => NotFound(self.to_string()).into_response(), - Self::Mime(_) => Internal::from(self).into_response(), + Self::Failed(_) => Internal::from(self).into_response(), } } } diff --git a/src/ui/handlers/conversation.rs b/src/ui/handlers/conversation.rs index 2ff090c..102efc6 100644 --- a/src/ui/handlers/conversation.rs +++ b/src/ui/handlers/conversation.rs @@ -37,11 +37,11 @@ pub enum Error { Internal(Internal), } -impl From<app::Error> for Error { - fn from(error: app::Error) -> Self { +impl From<app::GetError> for Error { + fn from(error: app::GetError) -> Self { match error { - app::Error::NotFound(_) | app::Error::Deleted(_) => Self::NotFound, - other => Self::Internal(other.into()), + app::GetError::NotFound(_) | app::GetError::Deleted(_) => Self::NotFound, + app::GetError::Failed(_) => Self::Internal(error.into()), } } } diff --git a/src/user/app.rs b/src/user/app.rs index 891e3b9..d993654 100644 --- a/src/user/app.rs +++ b/src/user/app.rs @@ -1,7 +1,15 @@ use sqlx::sqlite::SqlitePool; -use super::create::{self, Create}; -use crate::{clock::DateTime, event::Broadcaster, login::Login, name::Name, password::Password}; +use super::create::Create; +use crate::{ + clock::DateTime, + db, + error::failed::{Failed, ResultExt as _}, + event::Broadcaster, + login::Login, + name::Name, + password::Password, +}; pub struct Users { db: SqlitePool, @@ -18,13 +26,16 @@ impl Users { name: &Name, password: &Password, created_at: &DateTime, - ) -> Result<Login, CreateError> { + ) -> Result<Login, Failed> { let create = Create::begin(name, password, created_at); - let validated = create.validate()?; + let validated = create.validate().fail("Failed to validate new user")?; - let mut tx = self.db.begin().await?; - let stored = validated.store(&mut tx).await?; - tx.commit().await?; + let mut tx = self.db.begin().await.fail(db::failed::BEGIN)?; + let stored = validated + .store(&mut tx) + .await + .fail("Failed to store new user")?; + tx.commit().await.fail(db::failed::COMMIT)?; let login = stored.login().to_owned(); stored.publish(&self.events); @@ -32,22 +43,3 @@ impl Users { Ok(login) } } - -#[derive(Debug, thiserror::Error)] -pub enum CreateError { - #[error("invalid user name: {0}")] - InvalidName(Name), - #[error(transparent)] - PasswordHash(#[from] password_hash::Error), - #[error(transparent)] - Database(#[from] sqlx::Error), -} - -impl From<create::Error> for CreateError { - fn from(error: create::Error) -> Self { - match error { - create::Error::InvalidName(name) => Self::InvalidName(name), - create::Error::PasswordHash(error) => Self::PasswordHash(error), - } - } -} diff --git a/src/user/create.rs b/src/user/create.rs index d6656e5..cb52c5d 100644 --- a/src/user/create.rs +++ b/src/user/create.rs @@ -3,6 +3,7 @@ use sqlx::{Transaction, sqlite::Sqlite}; use super::{History, repo::Provider as _, validate}; use crate::{ clock::DateTime, + error::failed::{Failed, ResultExt as _}, event::{Broadcaster, Event, Sequence, repo::Provider as _}, login::{self, Login, repo::Provider as _}, name::Name, @@ -36,7 +37,7 @@ impl<'a> Create<'a> { return Err(Error::InvalidName(name.clone())); } - let password_hash = password.hash()?; + let password_hash = password.hash().fail("Failed to digest new password")?; Ok(Validated { name, @@ -112,5 +113,5 @@ pub enum Error { #[error("invalid user name: {0}")] InvalidName(Name), #[error(transparent)] - PasswordHash(#[from] password_hash::Error), + Failed(#[from] Failed), } |
