From 29fadd909e5a8bace9737b12b7d44e1b06b620fb Mon Sep 17 00:00:00 2001 From: John Vandenberg Date: Wed, 1 Nov 2023 10:31:02 +0800 Subject: [PATCH 1/2] Force stable ordering of migration operations --- butane_core/src/migrations/adb.rs | 59 +++++++++++-- butane_core/tests/migration.rs | 139 ++++++++++++++++++++++++++++++ 2 files changed, 190 insertions(+), 8 deletions(-) create mode 100644 butane_core/tests/migration.rs diff --git a/butane_core/src/migrations/adb.rs b/butane_core/src/migrations/adb.rs index 2a5ffae2..1c01eee3 100644 --- a/butane_core/src/migrations/adb.rs +++ b/butane_core/src/migrations/adb.rs @@ -248,7 +248,7 @@ impl ADB { } /// Abstract representation of a database table schema. -#[derive(Clone, Debug, Deserialize, Serialize)] +#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] pub struct ATable { pub name: String, pub columns: Vec, @@ -388,7 +388,7 @@ impl AColumn { } /// Individual operation use to apply a migration. -#[derive(Clone, Debug, Deserialize, Serialize)] +#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] pub enum Operation { //future improvement: support column renames AddTable(ATable), @@ -404,17 +404,28 @@ pub fn diff(old: &ADB, new: &ADB) -> Vec { let mut ops: Vec = Vec::new(); let new_names: HashSet<&String> = new.tables.keys().collect(); let old_names: HashSet<&String> = old.tables.keys().collect(); - let new_tables = new_names.difference(&old_names); + + // Add new tables + let mut new_tables: Vec<&String> = new_names.difference(&old_names).copied().collect(); + new_tables.sort(); for added in new_tables { let added: &str = added.as_ref(); ops.push(Operation::AddTable( new.tables.get(added).expect("no table").clone(), )); } - for removed in old_names.difference(&new_names) { + + // Remove tables + let mut removed_tables: Vec<&String> = old_names.difference(&new_names).copied().collect(); + removed_tables.sort(); + for removed in removed_tables { ops.push(Operation::RemoveTable((*removed).to_string())); } - for table in new_names.intersection(&old_names) { + + // Change existing tables + let mut existing_tables: Vec<&String> = new_names.intersection(&old_names).copied().collect(); + existing_tables.sort(); + for table in existing_tables { let table: &str = table.as_ref(); ops.append(&mut diff_table( old.tables.get(table).expect("no table"), @@ -432,7 +443,10 @@ fn diff_table(old: &ATable, new: &ATable) -> Vec { let mut ops: Vec = Vec::new(); let new_names: HashSet<&String> = new.columns.iter().map(|c| &c.name).collect(); let old_names: HashSet<&String> = old.columns.iter().map(|c| &c.name).collect(); - let added_names = new_names.difference(&old_names); + + // Add columns + let mut added_names: Vec<&String> = new_names.difference(&old_names).copied().collect(); + added_names.sort(); for added in added_names { let added: &str = added.as_ref(); ops.push(Operation::AddColumn( @@ -440,13 +454,21 @@ fn diff_table(old: &ATable, new: &ATable) -> Vec { col_by_name(&new.columns, added).unwrap().clone(), )); } - for removed in old_names.difference(&new_names) { + + // Remove columns + let mut removed_names: Vec<&String> = old_names.difference(&new_names).copied().collect(); + removed_names.sort(); + for removed in removed_names { ops.push(Operation::RemoveColumn( old.name.clone(), (*removed).to_string(), )); } - for colname in new_names.intersection(&old_names) { + + // Change columns + let mut existing_names: Vec<&String> = new_names.intersection(&old_names).copied().collect(); + existing_names.sort(); + for colname in existing_names { let colname: &str = colname.as_ref(); let col = col_by_name(&new.columns, colname).unwrap(); let old_col = col_by_name(&old.columns, colname).unwrap(); @@ -461,3 +483,24 @@ fn diff_table(old: &ATable, new: &ATable) -> Vec { } ops } + +#[cfg(test)] +mod tests { + use std::collections::HashSet; + + #[test] + /// Demostration that .difference() results are not stable. + fn show_unstable_migration_operation_order() { + let old = ["a", "b"]; + let new = ["d", "b", "c"]; + let new_names: HashSet<&str> = new.iter().copied().collect(); + let old_names: HashSet<&str> = old.iter().copied().collect(); + let added_tables = new_names.difference(&old_names); + let mut added_tables: Vec<&str> = added_tables.copied().collect(); + + // Remove this line, and running this multiple times will result in a mix of passes and failures. + added_tables.sort(); + + assert_eq!(added_tables, vec!["c", "d"]); + } +} diff --git a/butane_core/tests/migration.rs b/butane_core/tests/migration.rs new file mode 100644 index 00000000..c2b4654a --- /dev/null +++ b/butane_core/tests/migration.rs @@ -0,0 +1,139 @@ +use butane_core::migrations::adb::*; +use butane_core::SqlType; + +#[test] +fn empty_diff() { + let old = ADB::default(); + let new = ADB::default(); + let ops = diff(&old, &new); + assert_eq!(ops, vec![]); +} + +#[test] +fn add_table() { + let old = ADB::default(); + let mut new = ADB::default(); + let mut table = ATable::new("a".to_owned()); + let column = AColumn::new_simple( + "a".to_owned(), + DeferredSqlType::KnownId(TypeIdentifier::Ty(SqlType::Text)), + ); + table.add_column(column); + new.replace_table(table.clone()); + + let ops = diff(&old, &new); + + assert_eq!(ops, vec![Operation::AddTable(table)]); +} + +#[test] +fn remove_table() { + let mut old = ADB::default(); + let new = ADB::default(); + let mut table = ATable::new("a".to_owned()); + let column = AColumn::new_simple( + "a".to_owned(), + DeferredSqlType::KnownId(TypeIdentifier::Ty(SqlType::Text)), + ); + table.add_column(column); + old.replace_table(table.clone()); + + let ops = diff(&old, &new); + + let expected_op = Operation::RemoveTable("a".to_owned()); + assert_eq!(ops, vec![expected_op]); +} + +#[test] +fn stable_table_alpha_order() { + let old = ADB::default(); + let mut new = ADB::default(); + + // Insert tables out of order + let mut table_b = ATable::new("b".to_owned()); + let column_b = AColumn::new_simple( + "b".to_owned(), + DeferredSqlType::KnownId(TypeIdentifier::Ty(SqlType::Text)), + ); + table_b.add_column(column_b); + new.replace_table(table_b.clone()); + + let mut table_a = ATable::new("a".to_owned()); + let column_a = AColumn::new_simple( + "a".to_owned(), + DeferredSqlType::KnownId(TypeIdentifier::Ty(SqlType::Text)), + ); + table_a.add_column(column_a); + new.replace_table(table_a.clone()); + + let ops = diff(&old, &new); + + assert_eq!( + ops, + vec![Operation::AddTable(table_a), Operation::AddTable(table_b)] + ); +} + +#[test] +fn stable_new_table_column_insert_order() { + let old = ADB::default(); + let mut new = ADB::default(); + let mut table = ATable::new("a".to_owned()); + + // Insert columns out of order + let column_b = AColumn::new_simple( + "b".to_owned(), + DeferredSqlType::KnownId(TypeIdentifier::Ty(SqlType::Text)), + ); + table.add_column(column_b.clone()); + + let column_a = AColumn::new_simple( + "a".to_owned(), + DeferredSqlType::KnownId(TypeIdentifier::Ty(SqlType::Text)), + ); + table.add_column(column_a.clone()); + + // Add updated table to new + new.replace_table(table.clone()); + + let ops = diff(&old, &new); + + // Columns remain in insertion order + assert_eq!(ops, vec![Operation::AddTable(table)]); +} + +#[test] +fn stable_add_column_alpha_order() { + let mut old = ADB::default(); + let mut new = ADB::default(); + let mut table = ATable::new("a".to_owned()); + + // Add empty table to old + old.replace_table(table.clone()); + + // Insert columns out of order + let column_b = AColumn::new_simple( + "b".to_owned(), + DeferredSqlType::KnownId(TypeIdentifier::Ty(SqlType::Text)), + ); + table.add_column(column_b.clone()); + + let column_a = AColumn::new_simple( + "a".to_owned(), + DeferredSqlType::KnownId(TypeIdentifier::Ty(SqlType::Text)), + ); + table.add_column(column_a.clone()); + + // Add updated table to new + new.replace_table(table.clone()); + + let ops = diff(&old, &new); + + assert_eq!( + ops, + vec![ + Operation::AddColumn("a".to_owned(), column_a), + Operation::AddColumn("a".to_owned(), column_b) + ] + ); +} From 01274a550581ca1726d6baea5ef272f0e6274ff9 Mon Sep 17 00:00:00 2001 From: John Vandenberg Date: Wed, 1 Nov 2023 11:30:13 +0800 Subject: [PATCH 2/2] Use BTreeSet for auto-sorting --- butane_core/src/migrations/adb.rs | 53 +++++++------------------------ butane_core/tests/migration.rs | 2 +- 2 files changed, 12 insertions(+), 43 deletions(-) diff --git a/butane_core/src/migrations/adb.rs b/butane_core/src/migrations/adb.rs index 1c01eee3..8e2c5612 100644 --- a/butane_core/src/migrations/adb.rs +++ b/butane_core/src/migrations/adb.rs @@ -5,7 +5,7 @@ use crate::{Error, Result, SqlType, SqlVal}; use serde::{de::Deserializer, de::Visitor, ser::Serializer, Deserialize, Serialize}; use std::cmp::Ordering; -use std::collections::{HashMap, HashSet}; +use std::collections::{BTreeSet, HashMap}; /// Identifier for a type as used in a database column. Supports both /// [`SqlType`] and identifiers known only by name. @@ -402,12 +402,11 @@ pub enum Operation { /// Determine the operations necessary to move the database schema from `old` to `new`. pub fn diff(old: &ADB, new: &ADB) -> Vec { let mut ops: Vec = Vec::new(); - let new_names: HashSet<&String> = new.tables.keys().collect(); - let old_names: HashSet<&String> = old.tables.keys().collect(); + let new_names: BTreeSet<&String> = new.tables.keys().collect(); + let old_names: BTreeSet<&String> = old.tables.keys().collect(); // Add new tables - let mut new_tables: Vec<&String> = new_names.difference(&old_names).copied().collect(); - new_tables.sort(); + let new_tables = new_names.difference(&old_names); for added in new_tables { let added: &str = added.as_ref(); ops.push(Operation::AddTable( @@ -416,16 +415,12 @@ pub fn diff(old: &ADB, new: &ADB) -> Vec { } // Remove tables - let mut removed_tables: Vec<&String> = old_names.difference(&new_names).copied().collect(); - removed_tables.sort(); - for removed in removed_tables { + for removed in old_names.difference(&new_names) { ops.push(Operation::RemoveTable((*removed).to_string())); } // Change existing tables - let mut existing_tables: Vec<&String> = new_names.intersection(&old_names).copied().collect(); - existing_tables.sort(); - for table in existing_tables { + for table in new_names.intersection(&old_names) { let table: &str = table.as_ref(); ops.append(&mut diff_table( old.tables.get(table).expect("no table"), @@ -441,12 +436,11 @@ fn col_by_name<'a>(columns: &'a [AColumn], name: &str) -> Option<&'a AColumn> { fn diff_table(old: &ATable, new: &ATable) -> Vec { let mut ops: Vec = Vec::new(); - let new_names: HashSet<&String> = new.columns.iter().map(|c| &c.name).collect(); - let old_names: HashSet<&String> = old.columns.iter().map(|c| &c.name).collect(); + let new_names: BTreeSet<&String> = new.columns.iter().map(|c| &c.name).collect(); + let old_names: BTreeSet<&String> = old.columns.iter().map(|c| &c.name).collect(); // Add columns - let mut added_names: Vec<&String> = new_names.difference(&old_names).copied().collect(); - added_names.sort(); + let added_names = new_names.difference(&old_names); for added in added_names { let added: &str = added.as_ref(); ops.push(Operation::AddColumn( @@ -456,9 +450,7 @@ fn diff_table(old: &ATable, new: &ATable) -> Vec { } // Remove columns - let mut removed_names: Vec<&String> = old_names.difference(&new_names).copied().collect(); - removed_names.sort(); - for removed in removed_names { + for removed in old_names.difference(&new_names) { ops.push(Operation::RemoveColumn( old.name.clone(), (*removed).to_string(), @@ -466,9 +458,7 @@ fn diff_table(old: &ATable, new: &ATable) -> Vec { } // Change columns - let mut existing_names: Vec<&String> = new_names.intersection(&old_names).copied().collect(); - existing_names.sort(); - for colname in existing_names { + for colname in new_names.intersection(&old_names) { let colname: &str = colname.as_ref(); let col = col_by_name(&new.columns, colname).unwrap(); let old_col = col_by_name(&old.columns, colname).unwrap(); @@ -483,24 +473,3 @@ fn diff_table(old: &ATable, new: &ATable) -> Vec { } ops } - -#[cfg(test)] -mod tests { - use std::collections::HashSet; - - #[test] - /// Demostration that .difference() results are not stable. - fn show_unstable_migration_operation_order() { - let old = ["a", "b"]; - let new = ["d", "b", "c"]; - let new_names: HashSet<&str> = new.iter().copied().collect(); - let old_names: HashSet<&str> = old.iter().copied().collect(); - let added_tables = new_names.difference(&old_names); - let mut added_tables: Vec<&str> = added_tables.copied().collect(); - - // Remove this line, and running this multiple times will result in a mix of passes and failures. - added_tables.sort(); - - assert_eq!(added_tables, vec!["c", "d"]); - } -} diff --git a/butane_core/tests/migration.rs b/butane_core/tests/migration.rs index c2b4654a..d5a28453 100644 --- a/butane_core/tests/migration.rs +++ b/butane_core/tests/migration.rs @@ -133,7 +133,7 @@ fn stable_add_column_alpha_order() { ops, vec![ Operation::AddColumn("a".to_owned(), column_a), - Operation::AddColumn("a".to_owned(), column_b) + Operation::AddColumn("a".to_owned(), column_b), ] ); }