From 158a94aebed4fb4b9c686e142bf10828503ff189 Mon Sep 17 00:00:00 2001 From: owjs3901 Date: Tue, 27 Jan 2026 17:14:05 +0900 Subject: [PATCH 1/7] Sort mig action --- .../changepack_log_nA6oqqVeHgbDXY6Gn-wUD.json | 1 + Cargo.lock | 20 +-- crates/vespertide-planner/src/diff.rs | 137 ++++++++++++++++++ 3 files changed, 148 insertions(+), 10 deletions(-) create mode 100644 .changepacks/changepack_log_nA6oqqVeHgbDXY6Gn-wUD.json diff --git a/.changepacks/changepack_log_nA6oqqVeHgbDXY6Gn-wUD.json b/.changepacks/changepack_log_nA6oqqVeHgbDXY6Gn-wUD.json new file mode 100644 index 0000000..ed9836c --- /dev/null +++ b/.changepacks/changepack_log_nA6oqqVeHgbDXY6Gn-wUD.json @@ -0,0 +1 @@ +{"changes":{"crates/vespertide-cli/Cargo.toml":"Patch","crates/vespertide-planner/Cargo.toml":"Patch","crates/vespertide-query/Cargo.toml":"Patch","crates/vespertide-core/Cargo.toml":"Patch","crates/vespertide/Cargo.toml":"Patch","crates/vespertide-config/Cargo.toml":"Patch","crates/vespertide-exporter/Cargo.toml":"Patch","crates/vespertide-macro/Cargo.toml":"Patch","crates/vespertide-loader/Cargo.toml":"Patch","crates/vespertide-naming/Cargo.toml":"Patch"},"note":"Sort migration actions","date":"2026-01-27T08:13:52.424991800Z"} \ No newline at end of file diff --git a/Cargo.lock b/Cargo.lock index fba079f..6f7091e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2972,7 +2972,7 @@ checksum = "0b928f33d975fc6ad9f86c8f283853ad26bdd5b10b7f1542aa2fa15e2289105a" [[package]] name = "vespertide" -version = "0.1.36" +version = "0.1.37" dependencies = [ "vespertide-core", "vespertide-macro", @@ -2980,7 +2980,7 @@ dependencies = [ [[package]] name = "vespertide-cli" -version = "0.1.36" +version = "0.1.37" dependencies = [ "anyhow", "assert_cmd", @@ -3005,7 +3005,7 @@ dependencies = [ [[package]] name = "vespertide-config" -version = "0.1.36" +version = "0.1.37" dependencies = [ "clap", "schemars", @@ -3015,7 +3015,7 @@ dependencies = [ [[package]] name = "vespertide-core" -version = "0.1.36" +version = "0.1.37" dependencies = [ "rstest", "schemars", @@ -3027,7 +3027,7 @@ dependencies = [ [[package]] name = "vespertide-exporter" -version = "0.1.36" +version = "0.1.37" dependencies = [ "insta", "rstest", @@ -3038,7 +3038,7 @@ dependencies = [ [[package]] name = "vespertide-loader" -version = "0.1.36" +version = "0.1.37" dependencies = [ "anyhow", "rstest", @@ -3053,7 +3053,7 @@ dependencies = [ [[package]] name = "vespertide-macro" -version = "0.1.36" +version = "0.1.37" dependencies = [ "proc-macro2", "quote", @@ -3070,11 +3070,11 @@ dependencies = [ [[package]] name = "vespertide-naming" -version = "0.1.36" +version = "0.1.37" [[package]] name = "vespertide-planner" -version = "0.1.36" +version = "0.1.37" dependencies = [ "insta", "rstest", @@ -3085,7 +3085,7 @@ dependencies = [ [[package]] name = "vespertide-query" -version = "0.1.36" +version = "0.1.37" dependencies = [ "insta", "rstest", diff --git a/crates/vespertide-planner/src/diff.rs b/crates/vespertide-planner/src/diff.rs index c875636..9195f90 100644 --- a/crates/vespertide-planner/src/diff.rs +++ b/crates/vespertide-planner/src/diff.rs @@ -221,6 +221,70 @@ fn sort_delete_tables(actions: &mut [MigrationAction], all_tables: &BTreeMap<&st } } +/// Sort actions so that CreateTable actions come before AddConstraint actions +/// that reference those newly created tables via foreign keys. +fn sort_create_before_add_constraint(actions: &mut Vec) { + // Collect names of tables being created + let created_tables: BTreeSet = actions + .iter() + .filter_map(|a| { + if let MigrationAction::CreateTable { table, .. } = a { + Some(table.clone()) + } else { + None + } + }) + .collect(); + + if created_tables.is_empty() { + return; + } + + // Stable sort: CreateTable actions that are referenced by AddConstraint should come first + // We achieve this by partitioning: first all CreateTable, then everything else + actions.sort_by(|a, b| { + let a_is_create = matches!(a, MigrationAction::CreateTable { .. }); + let b_is_create = matches!(b, MigrationAction::CreateTable { .. }); + + // Check if action is AddConstraint with FK referencing a created table + let a_refs_created = if let MigrationAction::AddConstraint { + constraint: TableConstraint::ForeignKey { ref_table, .. }, + .. + } = a + { + created_tables.contains(ref_table) + } else { + false + }; + let b_refs_created = if let MigrationAction::AddConstraint { + constraint: TableConstraint::ForeignKey { ref_table, .. }, + .. + } = b + { + created_tables.contains(ref_table) + } else { + false + }; + + // Order: CreateTable first, then non-referencing actions, then referencing AddConstraint last + match (a_is_create, b_is_create, a_refs_created, b_refs_created) { + // Both CreateTable - maintain original order + (true, true, _, _) => std::cmp::Ordering::Equal, + // a is CreateTable, b is not - a comes first + (true, false, _, _) => std::cmp::Ordering::Less, + // a is not CreateTable, b is - b comes first + (false, true, _, _) => std::cmp::Ordering::Greater, + // Neither is CreateTable + // If a refs created table and b doesn't, a comes after + (false, false, true, false) => std::cmp::Ordering::Greater, + // If b refs created table and a doesn't, b comes after + (false, false, false, true) => std::cmp::Ordering::Less, + // Both ref or both don't ref - maintain original order + (false, false, _, _) => std::cmp::Ordering::Equal, + } + }); +} + /// Diff two schema snapshots into a migration plan. /// Schemas are normalized for comparison purposes, but the original (non-normalized) /// tables are used in migration actions to preserve inline constraint definitions. @@ -425,6 +489,9 @@ pub fn diff_schemas(from: &[TableDef], to: &[TableDef]) -> Result Date: Tue, 27 Jan 2026 17:16:50 +0900 Subject: [PATCH 2/7] Fix lint --- crates/vespertide-planner/src/diff.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/crates/vespertide-planner/src/diff.rs b/crates/vespertide-planner/src/diff.rs index 9195f90..fed54e6 100644 --- a/crates/vespertide-planner/src/diff.rs +++ b/crates/vespertide-planner/src/diff.rs @@ -1752,7 +1752,10 @@ mod tests { "notification", vec![ col("id", ColumnType::Simple(SimpleColumnType::Integer)), - col("broadcast_id", ColumnType::Simple(SimpleColumnType::Integer)), + col( + "broadcast_id", + ColumnType::Simple(SimpleColumnType::Integer), + ), ], vec![], ); @@ -1769,7 +1772,10 @@ mod tests { "notification", vec![ col("id", ColumnType::Simple(SimpleColumnType::Integer)), - col("broadcast_id", ColumnType::Simple(SimpleColumnType::Integer)), + col( + "broadcast_id", + ColumnType::Simple(SimpleColumnType::Integer), + ), ], vec![TableConstraint::ForeignKey { name: None, From 817e2fe10be1c1e4b761db3b95610c0b60801574 Mon Sep 17 00:00:00 2001 From: owjs3901 Date: Tue, 27 Jan 2026 17:19:15 +0900 Subject: [PATCH 3/7] Fix lint --- crates/vespertide-planner/src/diff.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/vespertide-planner/src/diff.rs b/crates/vespertide-planner/src/diff.rs index fed54e6..edcf3ca 100644 --- a/crates/vespertide-planner/src/diff.rs +++ b/crates/vespertide-planner/src/diff.rs @@ -223,7 +223,7 @@ fn sort_delete_tables(actions: &mut [MigrationAction], all_tables: &BTreeMap<&st /// Sort actions so that CreateTable actions come before AddConstraint actions /// that reference those newly created tables via foreign keys. -fn sort_create_before_add_constraint(actions: &mut Vec) { +fn sort_create_before_add_constraint(actions: &mut [MigrationAction]) { // Collect names of tables being created let created_tables: BTreeSet = actions .iter() From cf812aa648147bd46325bf5bb6a115ec6ecfda76 Mon Sep 17 00:00:00 2001 From: owjs3901 Date: Tue, 27 Jan 2026 17:28:00 +0900 Subject: [PATCH 4/7] Add testcase --- crates/vespertide-planner/src/diff.rs | 272 +++++++++++++++++++++++++- 1 file changed, 269 insertions(+), 3 deletions(-) diff --git a/crates/vespertide-planner/src/diff.rs b/crates/vespertide-planner/src/diff.rs index edcf3ca..66de6f7 100644 --- a/crates/vespertide-planner/src/diff.rs +++ b/crates/vespertide-planner/src/diff.rs @@ -1793,9 +1793,7 @@ mod tests { let plan = diff_schemas(&from_schema, &to_schema).unwrap(); // Find positions - let create_pos = plan.actions.iter().position(|a| { - matches!(a, MigrationAction::CreateTable { table, .. } if table == "notification_broadcast") - }); + let create_pos = plan.actions.iter().position(|a| matches!(a, MigrationAction::CreateTable { table, .. } if table == "notification_broadcast")); let add_constraint_pos = plan.actions.iter().position(|a| { matches!(a, MigrationAction::AddConstraint { constraint: TableConstraint::ForeignKey { ref_table, .. }, .. @@ -1818,6 +1816,274 @@ mod tests { ); } + /// Test sort_create_before_add_constraint with multiple action types + /// Covers lines 218, 221, 223, 225 in sort_create_before_add_constraint + #[test] + fn sort_create_before_add_constraint_all_branches() { + use super::*; + + // Scenario: Existing table gets modified (column change) AND gets FK to new table + // Plus another existing table gets a regular index added (not FK to new table) + // This creates: + // - ModifyColumnComment (doesn't ref created table) + // - AddConstraint FK (refs created table) + // - AddConstraint Index (doesn't ref created table) + // - CreateTable + + let users_from = table( + "users", + vec![ + col("id", ColumnType::Simple(SimpleColumnType::Integer)), + { + let mut c = col("name", ColumnType::Simple(SimpleColumnType::Text)); + c.comment = Some("Old comment".into()); + c + }, + col("role_id", ColumnType::Simple(SimpleColumnType::Integer)), + ], + vec![], + ); + + let posts_from = table( + "posts", + vec![ + col("id", ColumnType::Simple(SimpleColumnType::Integer)), + col("title", ColumnType::Simple(SimpleColumnType::Text)), + ], + vec![], + ); + + // New table: roles + let roles = table( + "roles", + vec![col("id", ColumnType::Simple(SimpleColumnType::Integer))], + vec![], + ); + + // Modified users: comment change + FK to new roles table + let users_to = table( + "users", + vec![ + col("id", ColumnType::Simple(SimpleColumnType::Integer)), + { + let mut c = col("name", ColumnType::Simple(SimpleColumnType::Text)); + c.comment = Some("New comment".into()); + c + }, + col("role_id", ColumnType::Simple(SimpleColumnType::Integer)), + ], + vec![TableConstraint::ForeignKey { + name: None, + columns: vec!["role_id".into()], + ref_table: "roles".into(), + ref_columns: vec!["id".into()], + on_delete: None, + on_update: None, + }], + ); + + // Modified posts: add index (not FK to new table) + let posts_to = table( + "posts", + vec![ + col("id", ColumnType::Simple(SimpleColumnType::Integer)), + col("title", ColumnType::Simple(SimpleColumnType::Text)), + ], + vec![TableConstraint::Index { + name: Some("idx_title".into()), + columns: vec!["title".into()], + }], + ); + + let from_schema = vec![users_from, posts_from]; + let to_schema = vec![users_to, posts_to, roles]; + + let plan = diff_schemas(&from_schema, &to_schema).unwrap(); + + // Verify CreateTable comes first + let create_pos = plan + .actions + .iter() + .position( + |a| matches!(a, MigrationAction::CreateTable { table, .. } if table == "roles"), + ) + .expect("Should have CreateTable for roles"); + + // ModifyColumnComment should come after CreateTable (line 218: non-create vs create) + let modify_pos = plan + .actions + .iter() + .position(|a| matches!(a, MigrationAction::ModifyColumnComment { .. })) + .expect("Should have ModifyColumnComment"); + + // AddConstraint Index (not FK to created) should come after CreateTable (line 218) + let add_index_pos = plan + .actions + .iter() + .position(|a| { + matches!( + a, + MigrationAction::AddConstraint { + constraint: TableConstraint::Index { .. }, + .. + } + ) + }) + .expect("Should have AddConstraint Index"); + + // AddConstraint FK to roles should come last (line 221: refs created, others don't) + let add_fk_pos = plan + .actions + .iter() + .position(|a| { + matches!( + a, + MigrationAction::AddConstraint { + constraint: TableConstraint::ForeignKey { ref_table, .. }, + .. + } if ref_table == "roles" + ) + }) + .expect("Should have AddConstraint FK to roles"); + + assert!( + create_pos < modify_pos, + "CreateTable must come before ModifyColumnComment" + ); + assert!( + create_pos < add_index_pos, + "CreateTable must come before AddConstraint Index" + ); + assert!( + create_pos < add_fk_pos, + "CreateTable must come before AddConstraint FK" + ); + // FK to created table should come after non-FK-to-created actions + assert!( + add_index_pos < add_fk_pos, + "AddConstraint Index (not referencing created) should come before AddConstraint FK (referencing created)" + ); + } + + /// Test that two AddConstraint FKs both referencing created tables maintain stable order + /// Covers line 225: both ref created tables + #[test] + fn sort_multiple_fks_to_created_tables() { + use super::*; + + // Two existing tables, each getting FK to a different new table + let users_from = table( + "users", + vec![ + col("id", ColumnType::Simple(SimpleColumnType::Integer)), + col("role_id", ColumnType::Simple(SimpleColumnType::Integer)), + ], + vec![], + ); + + let posts_from = table( + "posts", + vec![ + col("id", ColumnType::Simple(SimpleColumnType::Integer)), + col("category_id", ColumnType::Simple(SimpleColumnType::Integer)), + ], + vec![], + ); + + // Two new tables + let roles = table( + "roles", + vec![col("id", ColumnType::Simple(SimpleColumnType::Integer))], + vec![], + ); + let categories = table( + "categories", + vec![col("id", ColumnType::Simple(SimpleColumnType::Integer))], + vec![], + ); + + let users_to = table( + "users", + vec![ + col("id", ColumnType::Simple(SimpleColumnType::Integer)), + col("role_id", ColumnType::Simple(SimpleColumnType::Integer)), + ], + vec![TableConstraint::ForeignKey { + name: None, + columns: vec!["role_id".into()], + ref_table: "roles".into(), + ref_columns: vec!["id".into()], + on_delete: None, + on_update: None, + }], + ); + + let posts_to = table( + "posts", + vec![ + col("id", ColumnType::Simple(SimpleColumnType::Integer)), + col("category_id", ColumnType::Simple(SimpleColumnType::Integer)), + ], + vec![TableConstraint::ForeignKey { + name: None, + columns: vec!["category_id".into()], + ref_table: "categories".into(), + ref_columns: vec!["id".into()], + on_delete: None, + on_update: None, + }], + ); + + let from_schema = vec![users_from, posts_from]; + let to_schema = vec![users_to, posts_to, roles, categories]; + + let plan = diff_schemas(&from_schema, &to_schema).unwrap(); + + // Both CreateTable should come before both AddConstraint FK + let create_roles_pos = plan.actions.iter().position( + |a| matches!(a, MigrationAction::CreateTable { table, .. } if table == "roles"), + ); + let create_categories_pos = plan + .actions + .iter() + .position(|a| matches!(a, MigrationAction::CreateTable { table, .. } if table == "categories")); + let add_fk_roles_pos = plan.actions.iter().position(|a| { + matches!( + a, + MigrationAction::AddConstraint { + constraint: TableConstraint::ForeignKey { ref_table, .. }, + .. + } if ref_table == "roles" + ) + }); + let add_fk_categories_pos = plan.actions.iter().position(|a| { + matches!( + a, + MigrationAction::AddConstraint { + constraint: TableConstraint::ForeignKey { ref_table, .. }, + .. + } if ref_table == "categories" + ) + }); + + assert!(create_roles_pos.is_some()); + assert!(create_categories_pos.is_some()); + assert!(add_fk_roles_pos.is_some()); + assert!(add_fk_categories_pos.is_some()); + + // All CreateTable before all AddConstraint FK + let max_create = create_roles_pos + .unwrap() + .max(create_categories_pos.unwrap()); + let min_add_fk = add_fk_roles_pos + .unwrap() + .min(add_fk_categories_pos.unwrap()); + assert!( + max_create < min_add_fk, + "All CreateTable actions must come before all AddConstraint FK actions" + ); + } + /// Test that multiple FKs to the same table are deduplicated correctly #[test] fn create_tables_with_duplicate_fk_references() { From 36e246fc83ab0f9a66853cb945ad45b170d2e1f1 Mon Sep 17 00:00:00 2001 From: owjs3901 Date: Tue, 27 Jan 2026 18:10:11 +0900 Subject: [PATCH 5/7] Add testcase --- crates/vespertide-planner/src/diff.rs | 101 ++++++++++++++++++++++++++ 1 file changed, 101 insertions(+) diff --git a/crates/vespertide-planner/src/diff.rs b/crates/vespertide-planner/src/diff.rs index 66de6f7..bdd89e0 100644 --- a/crates/vespertide-planner/src/diff.rs +++ b/crates/vespertide-planner/src/diff.rs @@ -1232,6 +1232,107 @@ mod tests { } } + // Direct unit tests for sort_create_before_add_constraint + mod sort_create_before_add_constraint_tests { + use super::*; + use crate::diff::sort_create_before_add_constraint; + + /// Test line 276: (false, true, _, _) - a is NOT CreateTable, b IS CreateTable + #[test] + fn test_non_create_vs_create_ordering() { + // Put AddColumn BEFORE CreateTable - sort should move CreateTable first + let mut actions = vec![ + MigrationAction::AddColumn { + table: "users".into(), + column: Box::new(ColumnDef { + name: "name".into(), + r#type: ColumnType::Simple(SimpleColumnType::Text), + nullable: true, + default: None, + comment: None, + primary_key: None, + unique: None, + index: None, + foreign_key: None, + }), + fill_with: None, + }, + MigrationAction::CreateTable { + table: "roles".into(), + columns: vec![], + constraints: vec![], + }, + ]; + + sort_create_before_add_constraint(&mut actions); + + // CreateTable should now be first + assert!( + matches!(&actions[0], MigrationAction::CreateTable { table, .. } if table == "roles"), + "CreateTable should be moved to first position" + ); + assert!( + matches!(&actions[1], MigrationAction::AddColumn { .. }), + "AddColumn should be moved to second position" + ); + } + + /// Test line 281: (false, false, false, true) - neither is CreateTable, a doesn't ref, b refs + #[test] + fn test_non_ref_vs_ref_ordering() { + // Put AddConstraint FK (refs created) BEFORE AddColumn (doesn't ref) + // Sort should move the FK-referencing one AFTER the non-referencing one + let mut actions = vec![ + MigrationAction::AddConstraint { + table: "users".into(), + constraint: TableConstraint::ForeignKey { + name: None, + columns: vec!["role_id".into()], + ref_table: "roles".into(), // refs created table + ref_columns: vec!["id".into()], + on_delete: None, + on_update: None, + }, + }, + MigrationAction::AddColumn { + table: "posts".into(), + column: Box::new(ColumnDef { + name: "title".into(), + r#type: ColumnType::Simple(SimpleColumnType::Text), + nullable: true, + default: None, + comment: None, + primary_key: None, + unique: None, + index: None, + foreign_key: None, + }), + fill_with: None, + }, + MigrationAction::CreateTable { + table: "roles".into(), + columns: vec![], + constraints: vec![], + }, + ]; + + sort_create_before_add_constraint(&mut actions); + + // CreateTable should be first + assert!( + matches!(&actions[0], MigrationAction::CreateTable { .. }), + "CreateTable should be first" + ); + // AddColumn (non-ref) should come before AddConstraint FK (refs created) + let add_col_pos = actions.iter().position(|a| matches!(a, MigrationAction::AddColumn { .. })).unwrap(); + let add_fk_pos = actions.iter().position(|a| matches!(a, MigrationAction::AddConstraint { constraint: TableConstraint::ForeignKey { .. }, .. })).unwrap(); + assert!( + add_col_pos < add_fk_pos, + "AddColumn (non-ref) should come before AddConstraint FK (refs created)" + ); + } + } + // Tests for foreign key dependency ordering mod fk_ordering { use super::*; From 6a750a80d861e80149f5be36db1d64ba5b3d0840 Mon Sep 17 00:00:00 2001 From: owjs3901 Date: Tue, 27 Jan 2026 18:23:41 +0900 Subject: [PATCH 6/7] Fix lint --- crates/vespertide-planner/src/diff.rs | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/crates/vespertide-planner/src/diff.rs b/crates/vespertide-planner/src/diff.rs index bdd89e0..d7a53cd 100644 --- a/crates/vespertide-planner/src/diff.rs +++ b/crates/vespertide-planner/src/diff.rs @@ -1324,8 +1324,22 @@ mod tests { "CreateTable should be first" ); // AddColumn (non-ref) should come before AddConstraint FK (refs created) - let add_col_pos = actions.iter().position(|a| matches!(a, MigrationAction::AddColumn { .. })).unwrap(); - let add_fk_pos = actions.iter().position(|a| matches!(a, MigrationAction::AddConstraint { constraint: TableConstraint::ForeignKey { .. }, .. })).unwrap(); + let add_col_pos = actions + .iter() + .position(|a| matches!(a, MigrationAction::AddColumn { .. })) + .unwrap(); + let add_fk_pos = actions + .iter() + .position(|a| { + matches!( + a, + MigrationAction::AddConstraint { + constraint: TableConstraint::ForeignKey { .. }, + .. + } + ) + }) + .unwrap(); assert!( add_col_pos < add_fk_pos, "AddColumn (non-ref) should come before AddConstraint FK (refs created)" From 95f30f05f5bff30f837076b3f92fb1b6c9bf8020 Mon Sep 17 00:00:00 2001 From: owjs3901 Date: Tue, 27 Jan 2026 18:32:43 +0900 Subject: [PATCH 7/7] Add testcase --- crates/vespertide-planner/src/diff.rs | 362 ++++++++++++++++---------- 1 file changed, 219 insertions(+), 143 deletions(-) diff --git a/crates/vespertide-planner/src/diff.rs b/crates/vespertide-planner/src/diff.rs index d7a53cd..527ddfe 100644 --- a/crates/vespertide-planner/src/diff.rs +++ b/crates/vespertide-planner/src/diff.rs @@ -221,6 +221,54 @@ fn sort_delete_tables(actions: &mut [MigrationAction], all_tables: &BTreeMap<&st } } +/// Compare two migration actions for sorting. +/// Returns ordering where CreateTable comes first, then non-FK-ref actions, then FK-ref actions. +fn compare_actions_for_create_order( + a: &MigrationAction, + b: &MigrationAction, + created_tables: &BTreeSet, +) -> std::cmp::Ordering { + let a_is_create = matches!(a, MigrationAction::CreateTable { .. }); + let b_is_create = matches!(b, MigrationAction::CreateTable { .. }); + + // Check if action is AddConstraint with FK referencing a created table + let a_refs_created = if let MigrationAction::AddConstraint { + constraint: TableConstraint::ForeignKey { ref_table, .. }, + .. + } = a + { + created_tables.contains(ref_table) + } else { + false + }; + let b_refs_created = if let MigrationAction::AddConstraint { + constraint: TableConstraint::ForeignKey { ref_table, .. }, + .. + } = b + { + created_tables.contains(ref_table) + } else { + false + }; + + // Order: CreateTable first, then non-referencing actions, then referencing AddConstraint last + match (a_is_create, b_is_create, a_refs_created, b_refs_created) { + // Both CreateTable - maintain original order + (true, true, _, _) => std::cmp::Ordering::Equal, + // a is CreateTable, b is not - a comes first + (true, false, _, _) => std::cmp::Ordering::Less, + // a is not CreateTable, b is - b comes first + (false, true, _, _) => std::cmp::Ordering::Greater, + // Neither is CreateTable + // If a refs created table and b doesn't, a comes after + (false, false, true, false) => std::cmp::Ordering::Greater, + // If b refs created table and a doesn't, b comes after + (false, false, false, true) => std::cmp::Ordering::Less, + // Both ref or both don't ref - maintain original order + (false, false, _, _) => std::cmp::Ordering::Equal, + } +} + /// Sort actions so that CreateTable actions come before AddConstraint actions /// that reference those newly created tables via foreign keys. fn sort_create_before_add_constraint(actions: &mut [MigrationAction]) { @@ -240,49 +288,7 @@ fn sort_create_before_add_constraint(actions: &mut [MigrationAction]) { return; } - // Stable sort: CreateTable actions that are referenced by AddConstraint should come first - // We achieve this by partitioning: first all CreateTable, then everything else - actions.sort_by(|a, b| { - let a_is_create = matches!(a, MigrationAction::CreateTable { .. }); - let b_is_create = matches!(b, MigrationAction::CreateTable { .. }); - - // Check if action is AddConstraint with FK referencing a created table - let a_refs_created = if let MigrationAction::AddConstraint { - constraint: TableConstraint::ForeignKey { ref_table, .. }, - .. - } = a - { - created_tables.contains(ref_table) - } else { - false - }; - let b_refs_created = if let MigrationAction::AddConstraint { - constraint: TableConstraint::ForeignKey { ref_table, .. }, - .. - } = b - { - created_tables.contains(ref_table) - } else { - false - }; - - // Order: CreateTable first, then non-referencing actions, then referencing AddConstraint last - match (a_is_create, b_is_create, a_refs_created, b_refs_created) { - // Both CreateTable - maintain original order - (true, true, _, _) => std::cmp::Ordering::Equal, - // a is CreateTable, b is not - a comes first - (true, false, _, _) => std::cmp::Ordering::Less, - // a is not CreateTable, b is - b comes first - (false, true, _, _) => std::cmp::Ordering::Greater, - // Neither is CreateTable - // If a refs created table and b doesn't, a comes after - (false, false, true, false) => std::cmp::Ordering::Greater, - // If b refs created table and a doesn't, b comes after - (false, false, false, true) => std::cmp::Ordering::Less, - // Both ref or both don't ref - maintain original order - (false, false, _, _) => std::cmp::Ordering::Equal, - } - }); + actions.sort_by(|a, b| compare_actions_for_create_order(a, b, &created_tables)); } /// Diff two schema snapshots into a migration plan. @@ -1232,118 +1238,191 @@ mod tests { } } - // Direct unit tests for sort_create_before_add_constraint + // Direct unit tests for sort_create_before_add_constraint and compare_actions_for_create_order mod sort_create_before_add_constraint_tests { use super::*; - use crate::diff::sort_create_before_add_constraint; + use crate::diff::{compare_actions_for_create_order, sort_create_before_add_constraint}; + use std::cmp::Ordering; - /// Test line 276: (false, true, _, _) - a is NOT CreateTable, b IS CreateTable - #[test] - fn test_non_create_vs_create_ordering() { - // Put AddColumn BEFORE CreateTable - sort should move CreateTable first - let mut actions = vec![ - MigrationAction::AddColumn { - table: "users".into(), - column: Box::new(ColumnDef { - name: "name".into(), - r#type: ColumnType::Simple(SimpleColumnType::Text), - nullable: true, - default: None, - comment: None, - primary_key: None, - unique: None, - index: None, - foreign_key: None, - }), - fill_with: None, - }, - MigrationAction::CreateTable { - table: "roles".into(), - columns: vec![], - constraints: vec![], + fn make_add_column(table: &str, col: &str) -> MigrationAction { + MigrationAction::AddColumn { + table: table.into(), + column: Box::new(ColumnDef { + name: col.into(), + r#type: ColumnType::Simple(SimpleColumnType::Text), + nullable: true, + default: None, + comment: None, + primary_key: None, + unique: None, + index: None, + foreign_key: None, + }), + fill_with: None, + } + } + + fn make_create_table(name: &str) -> MigrationAction { + MigrationAction::CreateTable { + table: name.into(), + columns: vec![], + constraints: vec![], + } + } + + fn make_add_fk(table: &str, ref_table: &str) -> MigrationAction { + MigrationAction::AddConstraint { + table: table.into(), + constraint: TableConstraint::ForeignKey { + name: None, + columns: vec!["fk_col".into()], + ref_table: ref_table.into(), + ref_columns: vec!["id".into()], + on_delete: None, + on_update: None, }, - ]; + } + } - sort_create_before_add_constraint(&mut actions); + /// Test line 218: (false, true, _, _) - a is NOT CreateTable, b IS CreateTable + /// Direct test of comparison function + #[test] + fn test_compare_non_create_vs_create() { + let created_tables: BTreeSet = ["roles".to_string()].into_iter().collect(); - // CreateTable should now be first - assert!( - matches!(&actions[0], MigrationAction::CreateTable { table, .. } if table == "roles"), - "CreateTable should be moved to first position" + let add_col = make_add_column("users", "name"); + let create_table = make_create_table("roles"); + + // a=AddColumn (non-create), b=CreateTable (create) -> Greater (b comes first) + let result = compare_actions_for_create_order(&add_col, &create_table, &created_tables); + assert_eq!( + result, + Ordering::Greater, + "Non-CreateTable vs CreateTable should return Greater" ); - assert!( - matches!(&actions[1], MigrationAction::AddColumn { .. }), - "AddColumn should be moved to second position" + } + + /// Test line 216: (true, false, _, _) - a IS CreateTable, b is NOT CreateTable + #[test] + fn test_compare_create_vs_non_create() { + let created_tables: BTreeSet = ["roles".to_string()].into_iter().collect(); + + let create_table = make_create_table("roles"); + let add_col = make_add_column("users", "name"); + + // a=CreateTable (create), b=AddColumn (non-create) -> Less (a comes first) + let result = compare_actions_for_create_order(&create_table, &add_col, &created_tables); + assert_eq!( + result, + Ordering::Less, + "CreateTable vs Non-CreateTable should return Less" + ); + } + + /// Test line 214: (true, true, _, _) - both CreateTable + #[test] + fn test_compare_create_vs_create() { + let created_tables: BTreeSet = ["roles".to_string(), "categories".to_string()] + .into_iter() + .collect(); + + let create1 = make_create_table("roles"); + let create2 = make_create_table("categories"); + + // Both CreateTable -> Equal (maintain original order) + let result = compare_actions_for_create_order(&create1, &create2, &created_tables); + assert_eq!( + result, + Ordering::Equal, + "CreateTable vs CreateTable should return Equal" + ); + } + + /// Test line 221: (false, false, true, false) - neither CreateTable, a refs created, b doesn't + #[test] + fn test_compare_refs_vs_non_refs() { + let created_tables: BTreeSet = ["roles".to_string()].into_iter().collect(); + + let add_fk = make_add_fk("users", "roles"); // refs created + let add_col = make_add_column("posts", "title"); // doesn't ref + + // a refs created, b doesn't -> Greater (a comes after) + let result = compare_actions_for_create_order(&add_fk, &add_col, &created_tables); + assert_eq!( + result, + Ordering::Greater, + "FK-ref vs non-ref should return Greater" + ); + } + + /// Test line 223: (false, false, false, true) - neither CreateTable, a doesn't ref, b refs + #[test] + fn test_compare_non_refs_vs_refs() { + let created_tables: BTreeSet = ["roles".to_string()].into_iter().collect(); + + let add_col = make_add_column("posts", "title"); // doesn't ref + let add_fk = make_add_fk("users", "roles"); // refs created + + // a doesn't ref, b refs -> Less (b comes after, a comes first) + let result = compare_actions_for_create_order(&add_col, &add_fk, &created_tables); + assert_eq!( + result, + Ordering::Less, + "Non-ref vs FK-ref should return Less" + ); + } + + /// Test line 225: (false, false, _, _) - neither CreateTable, both don't ref + #[test] + fn test_compare_non_refs_vs_non_refs() { + let created_tables: BTreeSet = ["roles".to_string()].into_iter().collect(); + + let add_col1 = make_add_column("users", "name"); + let add_col2 = make_add_column("posts", "title"); + + // Both don't ref -> Equal + let result = compare_actions_for_create_order(&add_col1, &add_col2, &created_tables); + assert_eq!( + result, + Ordering::Equal, + "Non-ref vs non-ref should return Equal" + ); + } + + /// Test line 225: (false, false, _, _) - neither CreateTable, both ref created + #[test] + fn test_compare_refs_vs_refs() { + let created_tables: BTreeSet = ["roles".to_string(), "categories".to_string()] + .into_iter() + .collect(); + + let add_fk1 = make_add_fk("users", "roles"); + let add_fk2 = make_add_fk("posts", "categories"); + + // Both ref -> Equal + let result = compare_actions_for_create_order(&add_fk1, &add_fk2, &created_tables); + assert_eq!( + result, + Ordering::Equal, + "FK-ref vs FK-ref should return Equal" ); } - /// Test line 281: (false, false, false, true) - neither is CreateTable, a doesn't ref, b refs + /// Integration test: sort function works correctly #[test] - fn test_non_ref_vs_ref_ordering() { - // Put AddConstraint FK (refs created) BEFORE AddColumn (doesn't ref) - // Sort should move the FK-referencing one AFTER the non-referencing one + fn test_sort_integration() { let mut actions = vec![ - MigrationAction::AddConstraint { - table: "users".into(), - constraint: TableConstraint::ForeignKey { - name: None, - columns: vec!["role_id".into()], - ref_table: "roles".into(), // refs created table - ref_columns: vec!["id".into()], - on_delete: None, - on_update: None, - }, - }, - MigrationAction::AddColumn { - table: "posts".into(), - column: Box::new(ColumnDef { - name: "title".into(), - r#type: ColumnType::Simple(SimpleColumnType::Text), - nullable: true, - default: None, - comment: None, - primary_key: None, - unique: None, - index: None, - foreign_key: None, - }), - fill_with: None, - }, - MigrationAction::CreateTable { - table: "roles".into(), - columns: vec![], - constraints: vec![], - }, + make_add_column("t1", "c1"), + make_add_fk("users", "roles"), + make_create_table("roles"), ]; sort_create_before_add_constraint(&mut actions); - // CreateTable should be first - assert!( - matches!(&actions[0], MigrationAction::CreateTable { .. }), - "CreateTable should be first" - ); - // AddColumn (non-ref) should come before AddConstraint FK (refs created) - let add_col_pos = actions - .iter() - .position(|a| matches!(a, MigrationAction::AddColumn { .. })) - .unwrap(); - let add_fk_pos = actions - .iter() - .position(|a| { - matches!( - a, - MigrationAction::AddConstraint { - constraint: TableConstraint::ForeignKey { .. }, - .. - } - ) - }) - .unwrap(); - assert!( - add_col_pos < add_fk_pos, - "AddColumn (non-ref) should come before AddConstraint FK (refs created)" - ); + // CreateTable first, AddColumn second, AddConstraint FK last + assert!(matches!(&actions[0], MigrationAction::CreateTable { .. })); + assert!(matches!(&actions[1], MigrationAction::AddColumn { .. })); + assert!(matches!(&actions[2], MigrationAction::AddConstraint { .. })); } } @@ -2158,10 +2237,7 @@ mod tests { let create_roles_pos = plan.actions.iter().position( |a| matches!(a, MigrationAction::CreateTable { table, .. } if table == "roles"), ); - let create_categories_pos = plan - .actions - .iter() - .position(|a| matches!(a, MigrationAction::CreateTable { table, .. } if table == "categories")); + let create_categories_pos = plan.actions.iter().position(|a| matches!(a, MigrationAction::CreateTable { table, .. } if table == "categories")); let add_fk_roles_pos = plan.actions.iter().position(|a| { matches!( a,