diff --git a/.changepacks/changepack_log_kpCPfPCFVOmkGLiDqzIAL.json b/.changepacks/changepack_log_kpCPfPCFVOmkGLiDqzIAL.json new file mode 100644 index 0000000..9cd2aa1 --- /dev/null +++ b/.changepacks/changepack_log_kpCPfPCFVOmkGLiDqzIAL.json @@ -0,0 +1 @@ +{"changes":{"crates/vespertide/Cargo.toml":"Patch","crates/vespertide-macro/Cargo.toml":"Patch","crates/vespertide-core/Cargo.toml":"Patch","crates/vespertide-exporter/Cargo.toml":"Patch","crates/vespertide-loader/Cargo.toml":"Patch","crates/vespertide-naming/Cargo.toml":"Patch","crates/vespertide-config/Cargo.toml":"Patch","crates/vespertide-query/Cargo.toml":"Patch","crates/vespertide-planner/Cargo.toml":"Patch","crates/vespertide-cli/Cargo.toml":"Patch"},"note":"Fix duplicated foreign key removal when a column is dropped","date":"2026-01-19T05:58:18.303172700Z"} \ No newline at end of file diff --git a/crates/vespertide-core/src/schema/constraint.rs b/crates/vespertide-core/src/schema/constraint.rs index 13cdc43..5273e92 100644 --- a/crates/vespertide-core/src/schema/constraint.rs +++ b/crates/vespertide-core/src/schema/constraint.rs @@ -38,3 +38,17 @@ pub enum TableConstraint { columns: Vec, }, } + +impl TableConstraint { + /// Returns the columns referenced by this constraint. + /// For Check constraints, returns an empty slice (expression-based, not column-based). + pub fn columns(&self) -> &[ColumnName] { + match self { + TableConstraint::PrimaryKey { columns, .. } => columns, + TableConstraint::Unique { columns, .. } => columns, + TableConstraint::ForeignKey { columns, .. } => columns, + TableConstraint::Index { columns, .. } => columns, + TableConstraint::Check { .. } => &[], + } + } +} diff --git a/crates/vespertide-planner/src/diff.rs b/crates/vespertide-planner/src/diff.rs index 87ff24b..c875636 100644 --- a/crates/vespertide-planner/src/diff.rs +++ b/crates/vespertide-planner/src/diff.rs @@ -286,14 +286,18 @@ pub fn diff_schemas(from: &[TableDef], to: &[TableDef]) -> Result = from_cols + .keys() + .filter(|col| !to_cols.contains_key(*col)) + .copied() + .collect(); + + for col in &deleted_columns { + actions.push(MigrationAction::DeleteColumn { + table: name.to_string(), + column: col.to_string(), + }); } // Modified columns - type changes @@ -365,12 +369,25 @@ pub fn diff_schemas(from: &[TableDef], to: &[TableDef]) -> Result, ref_table: &str, ref_columns: Vec<&str>) -> TableConstraint { + TableConstraint::ForeignKey { + name: None, + columns: columns.into_iter().map(|s| s.to_string()).collect(), + ref_table: ref_table.to_string(), + ref_columns: ref_columns.into_iter().map(|s| s.to_string()).collect(), + on_delete: None, + on_update: None, + } + } + + #[test] + fn skip_remove_constraint_when_all_columns_deleted() { + // When a column with FK and index is deleted, the constraints should NOT + // generate separate RemoveConstraint actions (they are dropped with the column) + let from = vec![table( + "project", + vec![ + col("id", ColumnType::Simple(SimpleColumnType::Integer)), + col("template_id", ColumnType::Simple(SimpleColumnType::Integer)), + ], + vec![ + fk(vec!["template_id"], "book_template", vec!["id"]), + idx("ix_project__template_id", vec!["template_id"]), + ], + )]; + + let to = vec![table( + "project", + vec![col("id", ColumnType::Simple(SimpleColumnType::Integer))], + vec![], + )]; + + let plan = diff_schemas(&from, &to).unwrap(); + + // Should only have DeleteColumn, NO RemoveConstraint actions + assert_eq!(plan.actions.len(), 1); + assert!(matches!( + &plan.actions[0], + MigrationAction::DeleteColumn { table, column } + if table == "project" && column == "template_id" + )); + + // Explicitly verify no RemoveConstraint + let has_remove_constraint = plan + .actions + .iter() + .any(|a| matches!(a, MigrationAction::RemoveConstraint { .. })); + assert!( + !has_remove_constraint, + "Should NOT have RemoveConstraint when column is deleted" + ); + } + + #[test] + fn keep_remove_constraint_when_only_some_columns_deleted() { + // If a composite constraint has some columns remaining, RemoveConstraint is needed + let from = vec![table( + "orders", + vec![ + col("id", ColumnType::Simple(SimpleColumnType::Integer)), + col("user_id", ColumnType::Simple(SimpleColumnType::Integer)), + col("product_id", ColumnType::Simple(SimpleColumnType::Integer)), + ], + vec![idx( + "ix_orders__user_product", + vec!["user_id", "product_id"], + )], + )]; + + let to = vec![table( + "orders", + vec![ + col("id", ColumnType::Simple(SimpleColumnType::Integer)), + col("user_id", ColumnType::Simple(SimpleColumnType::Integer)), + // product_id is deleted, but user_id remains + ], + vec![], + )]; + + let plan = diff_schemas(&from, &to).unwrap(); + + // Should have both DeleteColumn AND RemoveConstraint + // (because user_id is still there, the composite index needs explicit removal) + let has_delete_column = plan.actions.iter().any(|a| { + matches!( + a, + MigrationAction::DeleteColumn { table, column } + if table == "orders" && column == "product_id" + ) + }); + assert!(has_delete_column, "Should have DeleteColumn for product_id"); + + let has_remove_constraint = plan.actions.iter().any(|a| { + matches!( + a, + MigrationAction::RemoveConstraint { table, .. } + if table == "orders" + ) + }); + assert!( + has_remove_constraint, + "Should have RemoveConstraint for composite index when only some columns deleted" + ); + } + + #[test] + fn skip_remove_constraint_when_all_composite_columns_deleted() { + // If ALL columns of a composite constraint are deleted, skip RemoveConstraint + let from = vec![table( + "orders", + vec![ + col("id", ColumnType::Simple(SimpleColumnType::Integer)), + col("user_id", ColumnType::Simple(SimpleColumnType::Integer)), + col("product_id", ColumnType::Simple(SimpleColumnType::Integer)), + ], + vec![idx( + "ix_orders__user_product", + vec!["user_id", "product_id"], + )], + )]; + + let to = vec![table( + "orders", + vec![col("id", ColumnType::Simple(SimpleColumnType::Integer))], + vec![], + )]; + + let plan = diff_schemas(&from, &to).unwrap(); + + // Should only have DeleteColumn actions, no RemoveConstraint + let delete_columns: Vec<_> = plan + .actions + .iter() + .filter(|a| matches!(a, MigrationAction::DeleteColumn { .. })) + .collect(); + assert_eq!( + delete_columns.len(), + 2, + "Should have 2 DeleteColumn actions" + ); + + let has_remove_constraint = plan + .actions + .iter() + .any(|a| matches!(a, MigrationAction::RemoveConstraint { .. })); + assert!( + !has_remove_constraint, + "Should NOT have RemoveConstraint when all composite columns deleted" + ); + } + + #[test] + fn keep_remove_constraint_when_no_columns_deleted() { + // Normal case: constraint removed but columns remain + let from = vec![table( + "users", + vec![ + col("id", ColumnType::Simple(SimpleColumnType::Integer)), + col("email", ColumnType::Simple(SimpleColumnType::Text)), + ], + vec![idx("ix_users__email", vec!["email"])], + )]; + + let to = vec![table( + "users", + vec![ + col("id", ColumnType::Simple(SimpleColumnType::Integer)), + col("email", ColumnType::Simple(SimpleColumnType::Text)), + ], + vec![], // Index removed but column remains + )]; + + let plan = diff_schemas(&from, &to).unwrap(); + + assert_eq!(plan.actions.len(), 1); + assert!(matches!( + &plan.actions[0], + MigrationAction::RemoveConstraint { table, .. } + if table == "users" + )); + } + } }