From 33601ef703a640b57e5bd0bf7dbd6d7ffa7377bf Mon Sep 17 00:00:00 2001 From: Owen Jacobson Date: Tue, 11 Nov 2025 01:29:36 -0500 Subject: 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` (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. --- src/invite/app.rs | 107 ++++++++++++++++++++++++++++-------------------------- 1 file changed, 55 insertions(+), 52 deletions(-) (limited to 'src/invite/app.rs') 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 { - 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 { + 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 { + 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 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 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), } -- cgit v1.2.3