Merge branch 'sharp/fix-migration-issues' into 'master'

Fix various migration issues.

See merge request veloren/veloren!1268

server/src/migrations/2020-08-06-212413_fix_various_problems/down.sql

@@ -0,0 +1 @@
+-- This file should undo anything in `up.sql`

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('""');

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::*;
 
+    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(&establish_connection(db_dir))?;
+        .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::<i64>(&establish_connection(db_dir))?;
+        .load::<i64>(&establish_connection(db_dir)?)?;
 
     match character_count.first() {
         Some(count) => {
@@ -511,7 +519,8 @@ impl CharacterUpdater {
 fn batch_update(updates: impl Iterator<Item = (i32, CharacterUpdateData)>, db_dir: &str) {
     let connection = establish_connection(db_dir);
 
-    if let Err(e) = connection.transaction::<_, diesel::result::Error, _>(|| {
+    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(
@@ -525,11 +534,13 @@ fn batch_update(updates: impl Iterator<Item = (i32, CharacterUpdateData)>, db_di
             );
 
             Ok(())
+        })
     }) {
         error!(?e, "Error during stats batch update transaction");
     }
 }
 
+/// NOTE: Only call while a transaction is held!
 fn update(
     character_id: i32,
     stats: &StatsUpdate,

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<SqliteConnection> {
     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 \
+            "Failed adding PRAGMA statements while establishing sqlite connection, including \
-             result in a higher likelihood of locking errors"
+             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 {