From 9eb59dc9157ebd1520442112690872bed69baf29 Mon Sep 17 00:00:00 2001 From: Savanni D'Gerinel Date: Thu, 11 Dec 2025 14:38:22 -0500 Subject: [PATCH] Set up new_session with foreign key enforcement --- Cargo.lock | 1 + result-extended/src/lib.rs | 7 +- visions/visions-core/Cargo.toml | 1 + visions/visions-core/src/app.rs | 5 +- visions/visions-core/src/app_test.rs | 2 + visions/visions-core/src/database.rs | 177 +++++++++++----------- visions/visions-core/src/database_test.rs | 53 ++++--- visions/visions-core/src/lib.rs | 7 +- visions/visions-core/src/test_utils.rs | 121 +++++++++++++++ 9 files changed, 258 insertions(+), 116 deletions(-) create mode 100644 visions/visions-core/src/test_utils.rs diff --git a/Cargo.lock b/Cargo.lock index bec0a4c7..29cbabd7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4049,6 +4049,7 @@ dependencies = [ "cool_asserts", "fail", "futures", + "libsqlite3-sys", "result-extended", "rusqlite", "rusqlite_migration", diff --git a/result-extended/src/lib.rs b/result-extended/src/lib.rs index 5d4ba3d9..ee3921fd 100644 --- a/result-extended/src/lib.rs +++ b/result-extended/src/lib.rs @@ -106,10 +106,13 @@ impl ResultExt { } } - pub fn promote_fatal(result: Result) -> ResultExt { + pub fn promote_fatal(result: Result) -> ResultExt + where + ERR: Into, + { match result { Ok(val) => ResultExt::Ok(val), - Err(fatal) => ResultExt::Fatal(fatal), + Err(fatal) => ResultExt::Fatal(fatal.into()), } } } diff --git a/visions/visions-core/Cargo.toml b/visions/visions-core/Cargo.toml index 8487a7fd..dfb0a701 100644 --- a/visions/visions-core/Cargo.toml +++ b/visions/visions-core/Cargo.toml @@ -6,6 +6,7 @@ edition = "2024" [dependencies] fail = "0.5" futures = "0.3.31" +libsqlite3-sys = "0.35.0" result-extended = { path = "../../result-extended" } rusqlite = "0.37.0" rusqlite_migration = "2.3.0" diff --git a/visions/visions-core/src/app.rs b/visions/visions-core/src/app.rs index edfa6b30..2034a1f4 100644 --- a/visions/visions-core/src/app.rs +++ b/visions/visions-core/src/app.rs @@ -16,7 +16,7 @@ use tokio::sync::{RwLock, mpsc::UnboundedSender}; use visions_types::*; use crate::{ - database::{self, CharacterFilter, CreateSessionError, Database}, + database::{self, CharacterFilter, Database}, types::{Game, Scene, Tabletop, User}, }; @@ -153,6 +153,7 @@ impl App { } pub async fn create_session(&self, id: &UserId) -> ResultExt { + /* let state = self.inner.write().await; match state.database.new_session(id) { ResultExt::Ok(id) => ResultExt::Ok(id), @@ -169,6 +170,8 @@ impl App { } ResultExt::Fatal(err) => ResultExt::Fatal(err.into()), } + */ + todo!() } pub async fn delete_session(&self, session_id: &SessionId) { diff --git a/visions/visions-core/src/app_test.rs b/visions/visions-core/src/app_test.rs index 87168098..15f70182 100644 --- a/visions/visions-core/src/app_test.rs +++ b/visions/visions-core/src/app_test.rs @@ -430,9 +430,11 @@ impl TestSupport { // let db = polodb_core::Database::open_path(&path).unwrap(); let db = Database::new(path.to_path_buf()).expect(""); + /* for user in users() { db.save_user(user); } + */ // let user_collection = db.collection("users"); // user_collection.insert_many(users()).unwrap(); diff --git a/visions/visions-core/src/database.rs b/visions/visions-core/src/database.rs index cbbf99b7..bdfc0908 100644 --- a/visions/visions-core/src/database.rs +++ b/visions/visions-core/src/database.rs @@ -169,6 +169,21 @@ impl From for CannotOpen { } } +/// CannotCreate indicates that the database layer tried to create a piece of data but could not. +/// The database remains in a valid, consistent state. +#[derive(Debug, Error)] +pub enum CannotCreate { + #[error("Duplicate unique ID")] + DuplicateId, + + #[error("A dependency was missing: {0}")] + MissingDependency(String), + + #[error("Cannot open database: {0}")] + CannotOpen(#[from] CannotOpen), +} + +/* #[derive(Debug, Error)] pub enum CreateSessionError { #[error("Cannot find user")] @@ -180,6 +195,7 @@ pub enum CreateSessionError { #[error("Cannot write to database")] WriteError(#[from] rusqlite::Error), } +*/ #[derive(Debug, Error)] pub enum CannotSave { @@ -228,10 +244,8 @@ impl From for CannotFind { pub struct ConnectionLost; */ -const MIGRATIONS_LIST: &[M<'_>] = &[ - M::up("PRAGMA foreign_keys = ON"), - M::up( - "CREATE TABLE users (id TEXT PRIMARY KEY NOT NULL, +const MIGRATIONS_LIST: &[M<'_>] = &[M::up( + "CREATE TABLE users (id TEXT PRIMARY KEY NOT NULL, email TEXT NOT NULL, name TEXT NOT NULL, admin BOOLEAN NOT NULL, @@ -282,9 +296,9 @@ const MIGRATIONS_LIST: &[M<'_>] = &[ FOREIGN KEY(game) REFERENCES games(id), FOREIGN KEY(card) REFERENCES cards(id)); ", - ) - .down( - " +) +.down( + " DROP TABLE tabletop_cards; DROP TABLE tabletop; DROP TABLE characters; @@ -296,8 +310,7 @@ const MIGRATIONS_LIST: &[M<'_>] = &[ DROP TABLE sessions; DROP TABLE users; ", - ), -]; +)]; const MIGRATIONS: Migrations<'_> = Migrations::from_slice(MIGRATIONS_LIST); @@ -333,11 +346,21 @@ impl Database { )) }); - Connection::open(self.path.clone()).map_err(|err| CannotOpen(format!("{:?}", err))) + let connection = Connection::open(self.path.clone()) + .map_err(|err| CannotOpen(format!("{:?}", err))) + .expect(""); + { + connection + .pragma_update(None, "foreign_keys", "ON") + .expect(""); + } + Ok(connection) } /// Create a new session for this user, setting the expiration time to now + session_duration. - pub fn new_session(&self, user_id: &UserId) -> ResultExt { + /// + /// This could fail if the user ID doesn't exist, or if the database cannot be opened. + pub fn new_session(&self, user_id: &UserId) -> ResultExt { fail_point!("sqldatabase-new-session", |_| { Result::Err(Fatal::Unhandled( "FAILPOINT: sqldatabase-new-session".to_owned(), @@ -353,46 +376,31 @@ impl Database { .to_string(); let mut connection = return_error!(ResultExt::from(self.open()).map_err(|err| err.into())); - let tx = return_error!(ResultExt::promote_fatal( - connection.transaction().map_err(|err| err.into()) - )); + let tx = return_error!(ResultExt::promote_fatal(connection.transaction())); let result = { // The nesting is necessary in order to ensure that user_query goes out of scope and // drops the implicit reference against tx. - let mut user_query = return_error!(ResultExt::promote_fatal( - tx.prepare_cached("SELECT id FROM users WHERE id = ?1") - .map_err(|err| err.into()) - )); - - match user_query.query_one(rusqlite::params![user_id.as_str()], |_| Ok(())) { - Ok(_) => { - let res = tx.execute( - "INSERT INTO sessions (id, user_id, expiration) VALUES (?1, ?2, ?3)", - rusqlite::params![session_id.as_str(), user_id.as_str(), expiration], - ); - - match res { - Ok(_) => ResultExt::Ok(session_id), - Err(err) => ResultExt::Err(CreateSessionError::WriteError(err)), - } - } - Err(rusqlite::Error::QueryReturnedNoRows) => { - ResultExt::Err(CreateSessionError::CannotFindUser) - } - Err(err) => ResultExt::Fatal(err.into()), - } + let mut stmt = return_error!(ResultExt::promote_fatal(tx.prepare_cached( + "INSERT INTO sessions (id, user_id, expiration) VALUES (?1, ?2, ?3)" + ))); + stmt.insert(rusqlite::params![ + session_id.as_str(), + user_id.as_str(), + expiration + ]) }; match result { - ResultExt::Ok(_) => { - return_error!(ResultExt::promote_fatal( - tx.commit().map_err(|err| err.into()) - )) + Ok(_) => { + return_error!(ResultExt::promote_fatal(tx.commit())); + ResultExt::Ok(session_id) } - _ => {} + Err(err) if is_constraint_violation(&err) => ResultExt::Err( + CannotCreate::MissingDependency(format!("{}", user_id.as_str())), + ), + Err(err) => ResultExt::Fatal(Fatal::Unhandled(format!("{:?}", err))), } - result } /* @@ -416,32 +424,23 @@ impl Database { )?; Ok(session_id) } + */ - /// Retrieve the user ID given the session ID. Returns None if the user ID does not exist. - pub fn session_info(&self, session_id: &SessionId) -> Result, Fatal> { - let mut stmt = self - .connection - .prepare("SELECT user_id, expiration FROM sessions WHERE id = ?1")?; + /// Retrieve the user given the session ID. Returns None if the user ID does not exist. + pub fn session_info( + &self, + session_id: &SessionId, + ) -> ResultExt, CannotOpen, Fatal> { + let mut connection = return_error!(ResultExt::from(self.open())); + let tx = return_error!(ResultExt::promote_fatal(connection.transaction())); - let mut rows = stmt.query(rusqlite::params![session_id.as_str()])?; - - match rows.next()? { - Some(row) => { - let user_id: String = row.get(0)?; - let expiration: String = row.get(1)?; - let expiration: u64 = expiration - .parse::() - .map_err(|err| Fatal::MalformedData(format!("expiration date: {:?}", err)))?; - - let expiration_time = - std::time::UNIX_EPOCH + std::time::Duration::from_secs(expiration); - if SystemTime::now() > expiration_time { - return Ok(None); - } - - Ok(Some(UserId::from(user_id))) - } - None => Ok(None), + match user_by_session(&tx, session_id) { + Ok(Some(user)) => match User::try_from(user) { + Ok(user) => ResultExt::Ok(Some(user)), + Err(err) => ResultExt::Fatal(err), + }, + Ok(None) => ResultExt::Ok(None), + Err(err) => ResultExt::Fatal(err.into()), } } @@ -467,6 +466,7 @@ impl Database { } */ + /* /// Delete the specified session pub fn delete_session( &self, @@ -768,13 +768,9 @@ impl Database { fn user(&mut self, user_id: &UserId) -> ResultExt, CannotOpen, Fatal> { let mut connection = return_error!(ResultExt::from(self.open())); - let tx = return_error!(ResultExt::promote_fatal( - connection.transaction().map_err(|err| err.into()) - )); + let tx = return_error!(ResultExt::promote_fatal(connection.transaction())); - let row = return_error!(ResultExt::promote_fatal( - user_by_id(&tx, user_id).map_err(|err| err.into()) - )); + let row = return_error!(ResultExt::promote_fatal(user_by_id(&tx, user_id))); let user: Result, Fatal> = match row { Some(row) => User::try_from(row).map(Some), @@ -786,13 +782,9 @@ impl Database { pub fn user_by_email(&self, email: &str) -> ResultExt, CannotOpen, Fatal> { let mut connection = return_error!(ResultExt::from(self.open()).map_err(|err| err.into())); - let tx = return_error!(ResultExt::promote_fatal( - connection.transaction().map_err(|err| err.into()) - )); + let tx = return_error!(ResultExt::promote_fatal(connection.transaction())); - let row = return_error!(ResultExt::promote_fatal( - user_by_email(&tx, email).map_err(|err| err.into()) - )); + let row = return_error!(ResultExt::promote_fatal(user_by_email(&tx, email))); let user: Result, Fatal> = match row { Some(row) => User::try_from(row).map(Some), @@ -802,18 +794,15 @@ impl Database { ResultExt::promote_fatal(user) } + /* */ pub fn user_by_session( &self, session_id: &SessionId, ) -> ResultExt, CannotOpen, Fatal> { let mut connection = return_error!(ResultExt::from(self.open())); - let tx = return_error!(ResultExt::promote_fatal( - connection.transaction().map_err(|err| err.into()) - )); + let tx = return_error!(ResultExt::promote_fatal(connection.transaction())); - let row = return_error!(ResultExt::promote_fatal( - user_by_session(&tx, session_id).map_err(|err| err.into()) - )); + let row = return_error!(ResultExt::promote_fatal(user_by_session(&tx, session_id))); let user: Result, Fatal> = match row { Some(row) => User::try_from(row).map(Some), @@ -842,15 +831,11 @@ impl Database { pub fn save_user(&self, user: User) -> ResultExt<(), CannotSave, Fatal> { let mut connection = return_error!(ResultExt::from(self.open()).map_err(|err| err.into())); - let tx = return_error!(ResultExt::promote_fatal( - connection.transaction().map_err(|err| err.into()) - )); + let tx = return_error!(ResultExt::promote_fatal(connection.transaction())); match write_user(&tx, &user) { Ok(_) => { - return_error!(ResultExt::promote_fatal( - tx.commit().map_err(|err| err.into()) - )); + return_error!(ResultExt::promote_fatal(tx.commit())); ResultExt::Ok(()) } Err(err) => return ResultExt::Fatal(err.into()), @@ -1326,6 +1311,7 @@ impl DbConn { } */ +#[derive(Debug)] struct UserRow { id: String, email: String, @@ -1407,3 +1393,14 @@ fn game_from_row(row: &Row<'_>) -> Result { background, }) } + +fn is_constraint_violation(err: &rusqlite::Error) -> bool { + match err { + rusqlite::Error::SqliteFailure(libsqlite3_sys::Error { code, .. }, _) + if *code == libsqlite3_sys::ErrorCode::ConstraintViolation => + { + true + } + _ => false, + } +} diff --git a/visions/visions-core/src/database_test.rs b/visions/visions-core/src/database_test.rs index a7b7ebf0..e024fd1d 100644 --- a/visions/visions-core/src/database_test.rs +++ b/visions/visions-core/src/database_test.rs @@ -1,22 +1,8 @@ -use std::{path::PathBuf, time::Duration}; - use cool_asserts::assert_matches; use result_extended::ResultExt; use visions_types::*; -use crate::{ - database::{DbConn, SqlDatabase}, - types::{Game, Tabletop}, -}; - -fn create_database() -> SqlDatabase { - SqlDatabase::new::(None).expect("in-memory database should open") -} - -fn create_database_connection() -> DbConn { - DbConn::new::(None) - .expect("in-memory database failed to open with an unexpected error") -} +use crate::{app_test::VAKARIAN_ID, database::CannotCreate, test_utils::DbHandle}; /// SCENARIO: A session is created and associated with a user id /// GIVEN: The database connection has been created @@ -26,22 +12,46 @@ fn create_database_connection() -> DbConn { #[tokio::test] async fn create_session() { // GIVEN: The database connection has been created - let database = create_database_connection(); + let db_handle = DbHandle::new(); + let database = db_handle.open_init().await; // GIVEN: A session with a user ID exists - let user_id = UserId::default(); + let user_id = UserId::from(VAKARIAN_ID); let session_id = database - .new_session(user_id.clone()) - .await + .new_session(&user_id) .expect("session should be created"); // WHEN: I request information about the session // THEN: I get back the user ID - assert_matches!(database.session_info(session_id).await, ResultExt::Ok(Some(uid)) => { - assert_eq!(uid, user_id) + assert_matches!(database.session_info(&session_id), ResultExt::Ok(Some(user)) => { + assert_eq!(user.id, user_id) }) } +/// SCENARIO: The database will fail if the caller attempts to create a session for a user +/// who does not exist. +/// GIVEN: The database connection has been created +/// GIVEN: The specified user ID does not exist +/// WHEN: I try to create a session +/// THEN: I get back a MissingDependency error +#[tokio::test] +async fn cannot_create_session_for_missing_user() { + // GIVEN: The database connection has been created + let db_handle = DbHandle::new(); + let database = db_handle.open_init().await; + + // GIVEN: The specified user ID does not exist + let user_id = UserId::default(); + + // WHEN: I try to create a session + // THEN: I get back a MissingDependency error + assert_matches!( + database.new_session(&user_id), + ResultExt::Err(CannotCreate::MissingDependency(dep)) => assert_eq!(dep, user_id.as_str()) + ); +} + +/* /// SCENARIO: Requesting information about a session that does not exist returns None /// GIVEN: The database connection has been created /// WHEN: I request information about a random session ID @@ -396,3 +406,4 @@ async fn update_game() { assert_matches!(saved_game, Some(ref saved_game) => assert_eq!(*saved_game, updated_game)); assert_matches!(saved_game, Some(ref saved_game) => assert_ne!(*saved_game, game)); } +*/ diff --git a/visions/visions-core/src/lib.rs b/visions/visions-core/src/lib.rs index 74f1b66a..45e8a531 100644 --- a/visions/visions-core/src/lib.rs +++ b/visions/visions-core/src/lib.rs @@ -7,5 +7,8 @@ pub mod utils; #[cfg(test)] mod app_test; -// #[cfg(test)] -// mod database_test; +#[cfg(test)] +mod database_test; + +#[cfg(test)] +mod test_utils; diff --git a/visions/visions-core/src/test_utils.rs b/visions/visions-core/src/test_utils.rs new file mode 100644 index 00000000..5b1e5cf2 --- /dev/null +++ b/visions/visions-core/src/test_utils.rs @@ -0,0 +1,121 @@ +use tempfile::{NamedTempFile, TempPath}; +use visions_types::AccountStatus; + +use crate::{database::Database, types::User}; + +pub struct DbHandle { + path: TempPath, +} + +impl DbHandle { + pub fn new() -> Self { + let path = NamedTempFile::new().unwrap().into_temp_path(); + println!("path: {:?}", path); + Self { path } + } + + pub async fn open_init(&self) -> Database { + let db = self.open().await; + for user in users() { + db.save_user(user).expect("to populate the user"); + } + + // for game in games() { + // db.save_game(game).await.expect("to populate the game"); + // } + // for card in cards() { + // db.save_card(card) + // .await + // .expect("card initialization to succeed"); + // } + // for character in charsheets() { + // db.save_character(character) + // .await + // .expect("character initialization to succeed"); + // } + db + } + + pub async fn open(&self) -> Database { + let db = Database::new(self.path.to_path_buf()).expect("database opening should succeed"); + + db + } +} + +fn users() -> Vec { + vec![ + User { + id: "vakarian-id".into(), + email: "vakarian@nowhere.com".to_owned(), + name: "Garrus Vakarian".to_owned(), + admin: true, + status: AccountStatus::Ok, + password: "aoeu".to_owned(), + }, + User { + id: "shephard-id".into(), + email: "shephard@nowhere.com".to_owned(), + name: "Shephard".to_owned(), + admin: false, + status: AccountStatus::PasswordReset("2050-01-01 00:00:00".to_owned()), + password: "aoeu".to_owned(), + }, + User { + id: "tali-id".into(), + email: "tali@nowhere.com".to_owned(), + name: "Tali Zora vas Raya".to_owned(), + admin: false, + status: AccountStatus::Ok, + password: "aoeu".to_owned(), + }, + User { + id: "samara-id".into(), + email: "samara@nowhere.com".to_owned(), + name: "Samara".to_owned(), + admin: false, + status: AccountStatus::Ok, + password: "aoeu".to_owned(), + }, + User { + id: "jack-id".into(), + email: "jack@nowhere.com".to_owned(), + name: "Jack".to_owned(), + admin: false, + status: AccountStatus::Ok, + password: "aoeu".to_owned(), + }, + User { + id: "joker-id".into(), + email: "joker@nowhere.com".to_owned(), + name: "Jeff Moreau".to_owned(), + admin: false, + status: AccountStatus::Ok, + password: "aoeu".to_owned(), + }, + User { + id: "alenko-id".into(), + email: "alenko@nowhere.com".to_owned(), + name: "Kaiden Alenko".to_owned(), + admin: false, + status: AccountStatus::Ok, + password: "aoeu".to_owned(), + }, + User { + id: "tsoni-id".into(), + email: "tsoni@nowhere.com".to_owned(), + name: "Liara T'Soni".to_owned(), + admin: false, + status: AccountStatus::Ok, + password: "aoeu".to_owned(), + }, + User { + id: "grunt-id".into(), + email: "grunt@nowhere.com".to_owned(), + name: "Grunt".to_owned(), + admin: false, + status: AccountStatus::Locked, + password: "aoeu".to_owned(), + }, + ] +}