diff --git a/server/src/migrations/2020-08-06-212413_fix_various_problems/down.sql b/server/src/migrations/2020-08-06-212413_fix_various_problems/down.sql new file mode 100644 index 0000000000..291a97c5ce --- /dev/null +++ b/server/src/migrations/2020-08-06-212413_fix_various_problems/down.sql @@ -0,0 +1 @@ +-- This file should undo anything in `up.sql` \ No newline at end of file diff --git a/server/src/migrations/2020-08-06-212413_fix_various_problems/up.sql b/server/src/migrations/2020-08-06-212413_fix_various_problems/up.sql new file mode 100644 index 0000000000..0c6269e8e5 --- /dev/null +++ b/server/src/migrations/2020-08-06-212413_fix_various_problems/up.sql @@ -0,0 +1,17 @@ +-- Delete invalid data caused by inconsistent enforcement of foreign key constraints. +DELETE FROM stats +WHERE NOT EXISTS (SELECT 1 FROM character WHERE character.id = stats.character_id); + +DELETE FROM body +WHERE NOT EXISTS (SELECT 1 FROM character WHERE character.id = body.character_id); + +DELETE FROM inventory +WHERE NOT EXISTS (SELECT 1 FROM character WHERE character.id = inventory.character_id); + +DELETE FROM loadout +WHERE NOT EXISTS (SELECT 1 FROM character WHERE character.id = loadout.character_id); + +-- Fix up incorrect skill data. +UPDATE stats +SET skills = json('{"skill_groups":[],"skills":[]}') +WHERE skills = json('""'); diff --git a/server/src/persistence/character.rs b/server/src/persistence/character.rs index 1fe6f8f657..0b14c7c44c 100644 --- a/server/src/persistence/character.rs +++ b/server/src/persistence/character.rs @@ -234,7 +234,7 @@ impl Drop for CharacterLoader { /// After first logging in, and after a character is selected, we fetch this /// data for the purpose of inserting their persisted data for the entity. fn load_character_data(player_uuid: &str, character_id: i32, db_dir: &str) -> CharacterDataResult { - let connection = establish_connection(db_dir); + let connection = establish_connection(db_dir)?; let result = schema::character::dsl::character .filter(schema::character::id.eq(character_id)) @@ -281,7 +281,7 @@ fn load_character_list(player_uuid: &str, db_dir: &str) -> CharacterListResult { .inner_join(schema::body::table) .inner_join(schema::stats::table) .inner_join(schema::loadout::table) - .load::<(Character, Body, Stats, Loadout)>(&establish_connection(db_dir)); + .load::<(Character, Body, Stats, Loadout)>(&establish_connection(db_dir)?); match result { Ok(data) => Ok(data @@ -322,7 +322,7 @@ fn create_character( ) -> CharacterListResult { check_character_limit(uuid, db_dir)?; - let connection = establish_connection(db_dir); + let connection = establish_connection(db_dir)?; connection.transaction::<_, diesel::result::Error, _>(|| { use schema::{body, character, character::dsl::*, inventory, loadout, stats}; @@ -413,12 +413,20 @@ fn create_character( fn delete_character(uuid: &str, character_id: i32, db_dir: &str) -> CharacterListResult { use schema::character::dsl::*; - diesel::delete( - character - .filter(id.eq(character_id)) - .filter(player_uuid.eq(uuid)), - ) - .execute(&establish_connection(db_dir))?; + let connection = establish_connection(db_dir)?; + + if let Err(e) = connection.transaction::<_, diesel::result::Error, _>(|| { + diesel::delete( + character + .filter(id.eq(character_id)) + .filter(player_uuid.eq(uuid)), + ) + .execute(&connection)?; + + Ok(()) + }) { + error!(?e, "Error during stats batch update transaction"); + } load_character_list(uuid, db_dir) } @@ -432,7 +440,7 @@ fn check_character_limit(uuid: &str, db_dir: &str) -> Result<(), Error> { let character_count = character .select(count_star()) .filter(player_uuid.eq(uuid)) - .load::(&establish_connection(db_dir))?; + .load::(&establish_connection(db_dir)?)?; match character_count.first() { Some(count) => { @@ -511,25 +519,28 @@ impl CharacterUpdater { fn batch_update(updates: impl Iterator, db_dir: &str) { let connection = establish_connection(db_dir); - if let Err(e) = connection.transaction::<_, diesel::result::Error, _>(|| { - updates.for_each( - |(character_id, (stats_update, inventory_update, loadout_update))| { - update( - character_id, - &stats_update, - &inventory_update, - &loadout_update, - &connection, - ) - }, - ); + if let Err(e) = connection.and_then(|connection| { + connection.transaction::<_, diesel::result::Error, _>(|| { + updates.for_each( + |(character_id, (stats_update, inventory_update, loadout_update))| { + update( + character_id, + &stats_update, + &inventory_update, + &loadout_update, + &connection, + ) + }, + ); - Ok(()) + Ok(()) + }) }) { error!(?e, "Error during stats batch update transaction"); } } +/// NOTE: Only call while a transaction is held! fn update( character_id: i32, stats: &StatsUpdate, diff --git a/server/src/persistence/mod.rs b/server/src/persistence/mod.rs index 441d741c63..8047372312 100644 --- a/server/src/persistence/mod.rs +++ b/server/src/persistence/mod.rs @@ -31,10 +31,16 @@ embed_migrations!(); pub fn run_migrations(db_dir: &str) -> Result<(), diesel_migrations::RunMigrationsError> { let db_dir = &apply_saves_dir_override(db_dir); let _ = fs::create_dir(format!("{}/", db_dir)); - embedded_migrations::run_with_output(&establish_connection(db_dir), &mut std::io::stdout()) + embedded_migrations::run_with_output( + &establish_connection(db_dir).expect( + "If we cannot execute migrations, we should not be allowed to launch the server, so \ + we don't populate it with bad data.", + ), + &mut std::io::stdout(), + ) } -fn establish_connection(db_dir: &str) -> SqliteConnection { +fn establish_connection(db_dir: &str) -> QueryResult { let db_dir = &apply_saves_dir_override(db_dir); let database_url = format!("{}/db.sqlite", db_dir); @@ -45,18 +51,21 @@ fn establish_connection(db_dir: &str) -> SqliteConnection { // Set a busy timeout (in ms): https://sqlite.org/c3ref/busy_timeout.html if let Err(e) = connection.batch_execute( " + PRAGMA foreign_keys = ON; PRAGMA journal_mode = WAL; PRAGMA busy_timeout = 250; ", ) { warn!( ?e, - "Failed adding PRAGMA statements while establishing sqlite connection, this will \ - result in a higher likelihood of locking errors" + "Failed adding PRAGMA statements while establishing sqlite connection, including \ + enabling foreign key constraints. We will not allow connecting to the server under \ + these conditions." ); + return Err(e); } - connection + Ok(connection) } fn apply_saves_dir_override(db_dir: &str) -> String {