From d58c6a69d61852618f9ad81dfc1d287c8a574267 Mon Sep 17 00:00:00 2001 From: Suya1671 Date: Wed, 18 Jun 2025 15:20:31 +0200 Subject: [PATCH] feat(models): update models to use error-stack types This makes a bunch of errors more meaningful and also helps with typing --- Cargo.lock | 22 +++++++- Cargo.toml | 2 +- src/commands/member.rs | 18 +++---- src/commands/mod.rs | 7 +-- src/commands/trigger.rs | 3 +- src/interactions/mod.rs | 7 ++- src/models/alias.rs | 45 +++++++--------- src/models/member.rs | 115 +++++++++++++++++++++++++--------------- src/models/message.rs | 10 ++-- src/models/mod.rs | 6 +-- src/models/system.rs | 22 ++++++-- src/models/trigger.rs | 42 +++++++-------- 12 files changed, 177 insertions(+), 122 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 446299d..84170ea 100755 --- a/Cargo.lock +++ b/Cargo.lock @@ -574,6 +574,26 @@ dependencies = [ "serde", ] +[[package]] +name = "derive_more" +version = "2.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "093242cf7570c207c83073cf82f79706fe7b8317e98620a47d5be7c3d8497678" +dependencies = [ + "derive_more-impl", +] + +[[package]] +name = "derive_more-impl" +version = "2.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bda628edc44c4bb645fbe0f758797143e4e07926f7ebf4e9bdfbd3d2ce621df3" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.101", +] + [[package]] name = "digest" version = "0.10.7" @@ -2409,10 +2429,10 @@ version = "0.1.0" dependencies = [ "axum", "clap", + "derive_more", "displaydoc", "dotenvy 0.15.7 (git+https://github.com/allan2/dotenvy)", "error-stack", - "eyre", "http-body-util", "libsqlite3-sys", "menv", diff --git a/Cargo.toml b/Cargo.toml index 47b5183..14da013 100755 --- a/Cargo.toml +++ b/Cargo.toml @@ -16,7 +16,6 @@ error-stack = { version = "0.5.0", features = [ "serde", "spantrace", ] } -eyre = "0.6.12" http-body-util = "0.1.3" menv = "0.2.7" oauth2 = "5.0.0" @@ -42,6 +41,7 @@ dotenvy = { git = "https://github.com/allan2/dotenvy", features = ["macros"] } url = "2.5.4" serde_json = "1.0.140" tower-http = { version = "0.6.6", features = ["trace"] } +derive_more = { version = "2.0.1", features = ["from"] } [features] encrypt = ["libsqlite3-sys/bundled-sqlcipher"] diff --git a/src/commands/member.rs b/src/commands/member.rs index 3ab15b9..44c9a56 100755 --- a/src/commands/member.rs +++ b/src/commands/member.rs @@ -1,6 +1,6 @@ use std::sync::Arc; -use error_stack::{Result, ResultExt, report}; +use error_stack::{Result, ResultExt}; use slack_morphism::prelude::*; use tracing::{debug, info, trace}; @@ -125,7 +125,7 @@ impl Member { member::Id::new(member_id) .validate_by_system(system.id, &user_state.db) .await - .ok() + .change_context(CommandError::Sqlx)? }; debug!(target_member_id = ?new_active_member_id, "Changing active member"); @@ -143,13 +143,13 @@ impl Member { info!("Successfully switched to base account"); "Switched to base account".into() } - Err(ChangeActiveMemberError::MemberNotFound) => { - debug!("Requested member not found in system"); - "The member you gave doesn't exist!".into() - } - Err(ChangeActiveMemberError::Sqlx(err)) => { - return Err(report!(err).change_context(CommandError::Sqlx)); - } + Err(e) => match e.current_context() { + ChangeActiveMemberError::MemberNotFound => { + debug!("Requested member not found in system"); + "The member you gave doesn't exist!".into() + } + ChangeActiveMemberError::Sqlx => return Err(e.change_context(CommandError::Sqlx)), + }, }; Ok(SlackCommandEventResponse::new( diff --git a/src/commands/mod.rs b/src/commands/mod.rs index bf989c0..847d77e 100755 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -3,14 +3,15 @@ use std::sync::Arc; mod member; mod system; mod trigger; + use axum::{Extension, Json}; use clap::{Parser, error::ErrorKind}; use error_stack::ResultExt; -use member::Member; - use slack_morphism::prelude::*; -use system::System; use tracing::{Level, debug, error, trace}; + +use member::Member; +use system::System; use trigger::Trigger; use crate::fields; diff --git a/src/commands/trigger.rs b/src/commands/trigger.rs index a2013dd..b902f36 100755 --- a/src/commands/trigger.rs +++ b/src/commands/trigger.rs @@ -199,9 +199,10 @@ impl Trigger { let member_id = member::Id::new(member_id); // Validate the member belongs to the user's system - let Ok(member_id) = member_id + let Some(member_id) = member_id .validate_by_system(system_id, &user_state.db) .await + .change_context(CommandError::Sqlx)? else { debug!("Member not found"); return Ok(SlackCommandEventResponse::new( diff --git a/src/interactions/mod.rs b/src/interactions/mod.rs index 33fcbe5..94bd234 100755 --- a/src/interactions/mod.rs +++ b/src/interactions/mod.rs @@ -4,6 +4,7 @@ use std::error::Error; use std::sync::Arc; use axum::Extension; +use error_stack::ResultExt; use member::{create_member, edit_member}; use slack_morphism::prelude::*; use tracing::{debug, error}; @@ -114,7 +115,8 @@ async fn handle_modal_view( return Ok(()); }; - let Ok(trusted_member_id) = member_id.validate_by_user(&user_id, &user_state.db).await + let Some(trusted_member_id) = + member_id.validate_by_user(&user_id, &user_state.db).await? else { error!( id, @@ -137,7 +139,8 @@ async fn handle_modal_view( .map(models::member::Id::new) .expect("Failed to parse member id from external id"); - let Ok(trusted_member_id) = member_id.validate_by_user(&user_id, &user_state.db).await + let Some(trusted_member_id) = + member_id.validate_by_user(&user_id, &user_state.db).await? else { error!( id, diff --git a/src/models/alias.rs b/src/models/alias.rs index b25819a..35b3a38 100644 --- a/src/models/alias.rs +++ b/src/models/alias.rs @@ -1,9 +1,9 @@ use crate::id; use super::{Trustability, Trusted, Untrusted, member, system}; -use error_stack::ResultExt; +use error_stack::{Result, ResultExt}; use slack_morphism::prelude::*; -use sqlx::{SqlitePool, prelude::*}; +use sqlx::{SqlitePool, prelude::*, sqlite::SqliteQueryResult}; use tracing::{debug, warn}; id!( @@ -26,30 +26,24 @@ impl Id { self, system_id: system::Id, db: &SqlitePool, - ) -> Result, Self> { - let exists = sqlx::query!( - "SELECT EXISTS(SELECT 1 FROM aliases WHERE id = $1 AND system_id = $2) AS 'exists: bool'", + ) -> Result>, sqlx::Error> { + sqlx::query!( + "SELECT + id as 'id: Id' + FROM aliases + WHERE id = $1 AND system_id = $2", self.id, system_id.id ) - .fetch_one(db) + .fetch_optional(db) .await - .ok() - .is_some_and(|record| record.exists); - - if exists { - Ok(Id { - id: self.id, - trusted: std::marker::PhantomData, - }) - } else { - Err(self) - } + .map(|res| res.map(|res| res.id)) + .attach_printable("Failed to fetch alias id from database") } } impl Id { - pub async fn delete(self, db_pool: &SqlitePool) -> Result<(), sqlx::Error> { + pub async fn delete(self, db_pool: &SqlitePool) -> Result { sqlx::query!( r#" DELETE FROM aliases @@ -59,7 +53,7 @@ impl Id { ) .execute(db_pool) .await - .map(|_| ()) + .attach_printable("Failed to delete alias from database") } } @@ -99,6 +93,7 @@ impl Alias { ) .fetch_optional(db) .await + .attach_printable("Failed to fetch alias from database") } pub async fn fetch_by_system_id( @@ -122,12 +117,13 @@ impl Alias { ) .fetch_all(db) .await + .attach_printable("Failed to fetch aliases from database") } pub async fn fetch_by_member_id( db: &SqlitePool, member_id: member::Id, - ) -> error_stack::Result, Error> { + ) -> error_stack::Result, sqlx::Error> { sqlx::query_as!( Self, r#" @@ -144,7 +140,7 @@ impl Alias { ) .fetch_all(db) .await - .change_context(Error::Sqlx) + .attach_printable("Failed to fetch aliases from database") } } @@ -188,7 +184,7 @@ impl View { system_id: system::Id, member_id: member::Id, db_pool: &SqlitePool, - ) -> error_stack::Result, Error> { + ) -> Result, sqlx::Error> { debug!( "Adding alias for {} (Member ID {}) to database", system_id, member_id @@ -207,7 +203,6 @@ impl View { .fetch_one(db_pool) .await .attach_printable("Error adding alias to database") - .change_context(Error::Sqlx) .map(|row| Id { id: row.id, trusted: std::marker::PhantomData, @@ -219,7 +214,7 @@ impl View { &self, alias_id: Id, db: &SqlitePool, - ) -> error_stack::Result<(), Error> { + ) -> error_stack::Result { sqlx::query!( r#" UPDATE aliases @@ -232,8 +227,6 @@ impl View { .execute(db) .await .attach_printable("Error updating member alias in database") - .change_context(Error::Sqlx) - .map(|_| ()) } pub fn create_add_view(self, member_id: member::Id) -> SlackView { diff --git a/src/models/member.rs b/src/models/member.rs index db85464..b0e913f 100755 --- a/src/models/member.rs +++ b/src/models/member.rs @@ -1,4 +1,4 @@ -use error_stack::ResultExt; +use error_stack::{Result, ResultExt}; use slack_morphism::prelude::*; use sqlx::{SqlitePool, prelude::*, sqlite::SqliteQueryResult}; use tracing::{debug, warn}; @@ -7,14 +7,14 @@ use crate::id; use super::{ Trusted, Untrusted, system, - trigger::{self, Trigger, Type}, + trigger::{Trigger, Type}, user, }; #[derive(thiserror::Error, displaydoc::Display, Debug)] pub enum Error { /// Error while calling the database - Sqlx, + Database, /// A field was missing from the view MissingField(String), } @@ -39,67 +39,95 @@ impl Id { self, system_id: system::Id, db: &SqlitePool, - ) -> Result, Self> { - let exists = sqlx::query!( - "SELECT EXISTS(SELECT 1 FROM members WHERE id = $1 AND system_id = $2) AS 'exists: bool'", + ) -> Result>, sqlx::Error> { + sqlx::query!( + "SELECT + id as 'id: Id' + FROM members + WHERE id = $1 AND system_id = $2", self.id, system_id.id ) - .fetch_one(db) + .fetch_optional(db) .await - .ok() - .is_some_and(|record| record.exists); - - if exists { - Ok(Id { - id: self.id, - trusted: std::marker::PhantomData, - }) - } else { - Err(self) - } + .attach_printable("Failed to validate member by system") + .map(|res| res.map(|res| res.id)) } pub async fn validate_by_user( self, user_id: &user::Id, db: &SqlitePool, - ) -> Result, Self> { - let exists = sqlx::query!( - "SELECT EXISTS( - SELECT 1 + ) -> Result>, sqlx::Error> { + sqlx::query!( + " + SELECT + members.id as 'id: Id' FROM members JOIN systems ON members.system_id = systems.id WHERE members.id = $1 AND systems.owner_id = $2 - ) AS 'exists: bool'", + ", self.id, user_id ) - .fetch_one(db) + .fetch_optional(db) .await - .ok() - .is_some_and(|record| record.exists); + .attach_printable("Failed to validate member by user") + .map(|res| res.map(|res| res.id)) + } - if exists { - Ok(Id { - id: self.id, - trusted: std::marker::PhantomData, - }) - } else { - Err(self) - } + pub async fn fetch_by_alias( + alias: &str, + system_id: system::Id, + db: &SqlitePool, + ) -> Result>, sqlx::Error> { + sqlx::query!( + "SELECT + member_id AS 'id: Id' + FROM aliases + WHERE alias = $1 AND system_id = $2", + alias, + system_id + ) + .fetch_optional(db) + .await + .attach_printable("Failed to fetch member id by alias") + .map(|res| res.map(|res| res.id)) } } impl Id { - pub async fn fetch_triggers( - self, - db: &SqlitePool, - ) -> error_stack::Result, trigger::Error> { + pub async fn fetch_triggers(self, db: &SqlitePool) -> Result, sqlx::Error> { Trigger::fetch_by_member_id(db, self).await } } +#[derive(Debug, derive_more::From)] +/// An untrusted member reference from an external source +pub enum MemberRef { + Id(Id), + /// We were given a [`super::Alias`] + Alias(String), +} + +impl MemberRef { + pub async fn validate_by_system( + &self, + system_id: system::Id, + db: &SqlitePool, + ) -> Result>, sqlx::Error> { + match self { + MemberRef::Id(id) => id + .validate_by_system(system_id, db) + .await + .attach_printable("Failed to validate member reference via id and system"), + MemberRef::Alias(alias) => Id::fetch_by_alias(alias, system_id, db) + .await + .attach_printable("Failed to validate member reference via alias and system"), + } + } +} + // TO-DO: move SQL to rust struct #[derive(FromRow, Debug)] #[allow(dead_code)] @@ -149,6 +177,7 @@ impl Member { ) .fetch_optional(db) .await + .attach_printable("Failed to fetch member by id and trust by system") } /// Fetch a member by their id @@ -177,6 +206,7 @@ impl Member { ) .fetch_optional(db) .await + .attach_printable("Failed to fetch member by id") } } @@ -315,7 +345,7 @@ impl View { &self, system_id: system::Id, db: &SqlitePool, - ) -> error_stack::Result { + ) -> error_stack::Result { debug!("Adding member {} to database", self.display_name); sqlx::query!(" INSERT INTO members (full_name, display_name, profile_picture_url, title, pronouns, name_pronunciation, name_recording_url, system_id) @@ -334,7 +364,6 @@ impl View { .fetch_one(db) .await .attach_printable("Error adding member to database") - .change_context(Error::Sqlx) .map(|row| row.id) } @@ -345,7 +374,7 @@ impl View { &self, member_id: Id, db: &SqlitePool, - ) -> error_stack::Result, Error> { + ) -> error_stack::Result { sqlx::query!(" UPDATE members SET full_name = $1, display_name = $2, profile_picture_url = $3, title = $4, pronouns = $5, name_pronunciation = $6, name_recording_url = $7 @@ -361,15 +390,13 @@ impl View { member_id, ).execute(db).await .attach_printable("Error editing member in database") - .change_context(Error::Sqlx) - .map(Some) } } impl TryFrom for View { type Error = Error; - fn try_from(value: SlackViewState) -> Result { + fn try_from(value: SlackViewState) -> std::result::Result { let mut view = Self::default(); for (_id, values) in value.values { for (id, content) in values { diff --git a/src/models/message.rs b/src/models/message.rs index 62df896..8b6a3f9 100644 --- a/src/models/message.rs +++ b/src/models/message.rs @@ -1,8 +1,9 @@ use crate::id; use super::{Trusted, member}; +use error_stack::{Result, ResultExt}; use slack_morphism::SlackTs; -use sqlx::{SqlitePool, prelude::*}; +use sqlx::{SqlitePool, prelude::*, sqlite::SqliteQueryResult}; id!( /// You cannot create a message id, as it is internal generated-only. @@ -28,7 +29,7 @@ impl MessageLog { pub async fn delete_by_message_id( message_id: String, db: &SqlitePool, - ) -> Result<(), sqlx::Error> { + ) -> Result { sqlx::query!( r#" DELETE FROM message_logs @@ -38,7 +39,7 @@ impl MessageLog { ) .execute(db) .await - .map(|_| ()) + .attach_printable("Failed to delete message log") } /// Fetches a message log by the slack message ID. @@ -62,6 +63,7 @@ impl MessageLog { ) .fetch_optional(db) .await + .attach_printable("Failed to fetch message log") } /// Fetches all message logs by the member ID. @@ -86,6 +88,7 @@ impl MessageLog { ) .fetch_all(db) .await + .attach_printable("Failed to fetch message logs") } pub async fn insert( @@ -108,5 +111,6 @@ impl MessageLog { ) .fetch_one(db) .await + .attach_printable("Failed to insert message log") } } diff --git a/src/models/mod.rs b/src/models/mod.rs index 26f6ead..ea81931 100755 --- a/src/models/mod.rs +++ b/src/models/mod.rs @@ -33,7 +33,7 @@ macro_rules! id { ($(#[$attr:meta])* => $name:ident) => { #[derive(::sqlx::Type, Debug, PartialEq, Eq, Clone, Copy)] $(#[$attr])* - pub struct Id { + pub struct Id { pub id: i64, trusted: ::std::marker::PhantomData, } @@ -46,7 +46,7 @@ macro_rules! id { fn encode_by_ref( &self, buf: &mut ::ArgumentBuffer<'q>, - ) -> Result<::sqlx::encode::IsNull, ::sqlx::error::BoxDynError> { + ) -> ::std::result::Result<::sqlx::encode::IsNull, ::sqlx::error::BoxDynError> { >::encode_by_ref(&self.id, buf) } @@ -62,7 +62,7 @@ macro_rules! id { { fn decode( value: ::ValueRef<'q>, - ) -> Result { + ) -> ::std::result::Result { let id = >::decode(value)?; Ok(Id { id, diff --git a/src/models/system.rs b/src/models/system.rs index fd72481..b2a56c1 100755 --- a/src/models/system.rs +++ b/src/models/system.rs @@ -1,5 +1,5 @@ use crate::{ - id, + fields, id, models::member::{Member, TriggeredMember}, }; @@ -9,6 +9,7 @@ use super::{ trigger::Trigger, user, }; +use error_stack::{Result, ResultExt, bail}; use redact::Secret; use sqlx::{SqlitePool, prelude::*}; use tracing::debug; @@ -72,7 +73,7 @@ pub struct System { /// Error while changing the active member pub enum ChangeActiveMemberError { /// Error while calling the database - Sqlx(#[from] sqlx::Error), + Sqlx, /// The member is not part of the system MemberNotFound, } @@ -106,6 +107,7 @@ impl System { ) .fetch_optional(db) .await + .attach_printable("Error fetching system") } pub async fn active_member(&self, db: &SqlitePool) -> Result, sqlx::Error> { @@ -128,13 +130,19 @@ impl System { let mut new_active_member = None; if let Some(new_active_member_id) = new_active_member_id { - let Some(member) = Member::fetch_by_id(new_active_member_id, db).await? else { - return Err(ChangeActiveMemberError::MemberNotFound); + let Some(member) = Member::fetch_by_id(new_active_member_id, db) + .await + .change_context(ChangeActiveMemberError::Sqlx) + .attach_printable("Failed to fetch member")? + else { + bail!(ChangeActiveMemberError::MemberNotFound); }; new_active_member = Some(member); } + fields!(new_active_member = ?&new_active_member); + sqlx::query!( r#" UPDATE systems @@ -145,7 +153,9 @@ impl System { self.id ) .execute(db) - .await?; + .await + .change_context(ChangeActiveMemberError::Sqlx) + .attach_printable("Failed to update system active member")?; self.active_member_id = new_active_member_id; Ok(new_active_member) @@ -174,6 +184,7 @@ impl System { ) .fetch_all(db) .await + .attach_printable("Failed to fetch members") } pub async fn fetch_triggered_member( @@ -203,5 +214,6 @@ impl System { ) .fetch_optional(db) .await + .attach_printable("Failed to fetch triggered member") } } diff --git a/src/models/trigger.rs b/src/models/trigger.rs index be1e803..c27eab1 100755 --- a/src/models/trigger.rs +++ b/src/models/trigger.rs @@ -3,9 +3,9 @@ use std::str::FromStr; use crate::id; use super::{Trustability, Trusted, Untrusted, member, system}; -use error_stack::ResultExt; +use error_stack::{Result, ResultExt}; use slack_morphism::prelude::*; -use sqlx::{SqlitePool, prelude::*}; +use sqlx::{SqlitePool, prelude::*, sqlite::SqliteQueryResult}; use tracing::{debug, warn}; id!( @@ -28,30 +28,24 @@ impl Id { self, system_id: system::Id, db: &SqlitePool, - ) -> Result, Self> { - let exists = sqlx::query!( - "SELECT EXISTS(SELECT 1 FROM triggers WHERE id = $1 AND system_id = $2) AS 'exists: bool'", + ) -> Result, sqlx::Error> { + sqlx::query!( + "SELECT + id as 'id: Id' + FROM triggers + WHERE id = $1 AND system_id = $2", self.id, system_id.id ) .fetch_one(db) .await - .ok() - .is_some_and(|record| record.exists); - - if exists { - Ok(Id { - id: self.id, - trusted: std::marker::PhantomData, - }) - } else { - Err(self) - } + .map(|record| record.id) + .attach_printable("Error validating trigger") } } impl Id { - pub async fn delete(self, db_pool: &SqlitePool) -> Result<(), sqlx::Error> { + pub async fn delete(self, db_pool: &SqlitePool) -> Result { sqlx::query!( r#" DELETE FROM triggers @@ -61,7 +55,7 @@ impl Id { ) .execute(db_pool) .await - .map(|_| ()) + .attach_printable("Error deleting trigger") } } @@ -99,7 +93,7 @@ pub struct UnknownType(String); impl FromStr for Type { type Err = UnknownType; - fn from_str(s: &str) -> Result { + fn from_str(s: &str) -> std::result::Result { match s { "suffix" => Ok(Self::Suffix), "prefix" => Ok(Self::Prefix), @@ -140,6 +134,7 @@ impl Trigger { ) .fetch_optional(db) .await + .attach_printable("Error fetching trigger") } pub async fn fetch_by_system_id( @@ -164,12 +159,13 @@ impl Trigger { ) .fetch_all(db) .await + .attach_printable("Error fetching triggers") } pub async fn fetch_by_member_id( db: &SqlitePool, member_id: member::Id, - ) -> error_stack::Result, Error> { + ) -> error_stack::Result, sqlx::Error> { sqlx::query_as!( Trigger, r#" @@ -187,7 +183,7 @@ impl Trigger { ) .fetch_all(db) .await - .change_context(Error::Sqlx) + .attach_printable("Error fetching triggers") } } @@ -289,7 +285,7 @@ impl View { &self, trigger_id: Id, db: &SqlitePool, - ) -> error_stack::Result<(), Error> { + ) -> Result { sqlx::query!( r#" UPDATE triggers @@ -303,8 +299,6 @@ impl View { .execute(db) .await .attach_printable("Error updating trigger in database") - .change_context(Error::Sqlx) - .map(|_| ()) } pub fn create_add_view(self, member_id: member::Id) -> SlackView {