From 3a787766fba8d41bb638d9cecdf4b5fc0d3e2b8d Mon Sep 17 00:00:00 2001 From: owjs3901 Date: Wed, 28 Jan 2026 12:26:47 +0900 Subject: [PATCH 1/2] Modify enum column with default value --- .../changepack_log_u7HHzEcBwNmzL3bTYO3aG.json | 1 + crates/vespertide-planner/src/diff.rs | 348 +++++++++++++++++- .../src/sql/modify_column_default.rs | 35 ++ 3 files changed, 383 insertions(+), 1 deletion(-) create mode 100644 .changepacks/changepack_log_u7HHzEcBwNmzL3bTYO3aG.json diff --git a/.changepacks/changepack_log_u7HHzEcBwNmzL3bTYO3aG.json b/.changepacks/changepack_log_u7HHzEcBwNmzL3bTYO3aG.json new file mode 100644 index 0000000..c938052 --- /dev/null +++ b/.changepacks/changepack_log_u7HHzEcBwNmzL3bTYO3aG.json @@ -0,0 +1 @@ +{"changes":{"crates/vespertide-exporter/Cargo.toml":"Patch","crates/vespertide-loader/Cargo.toml":"Patch","crates/vespertide-macro/Cargo.toml":"Patch","crates/vespertide-config/Cargo.toml":"Patch","crates/vespertide/Cargo.toml":"Patch","crates/vespertide-core/Cargo.toml":"Patch","crates/vespertide-naming/Cargo.toml":"Patch","crates/vespertide-query/Cargo.toml":"Patch","crates/vespertide-planner/Cargo.toml":"Patch","crates/vespertide-cli/Cargo.toml":"Patch"},"note":"Modify enum with default value issue","date":"2026-01-28T03:24:56.611461200Z"} \ No newline at end of file diff --git a/crates/vespertide-planner/src/diff.rs b/crates/vespertide-planner/src/diff.rs index 527ddfe..8309ff1 100644 --- a/crates/vespertide-planner/src/diff.rs +++ b/crates/vespertide-planner/src/diff.rs @@ -1,6 +1,9 @@ use std::collections::{BTreeMap, BTreeSet, HashSet, VecDeque}; -use vespertide_core::{MigrationAction, MigrationPlan, TableConstraint, TableDef}; +use vespertide_core::{ + ColumnType, ComplexColumnType, EnumValues, MigrationAction, MigrationPlan, TableConstraint, + TableDef, +}; use crate::error::PlannerError; @@ -291,6 +294,111 @@ fn sort_create_before_add_constraint(actions: &mut [MigrationAction]) { actions.sort_by(|a, b| compare_actions_for_create_order(a, b, &created_tables)); } +/// Get the set of string enum values that were removed (present in `from` but not in `to`). +/// Returns None if either type is not a string enum. +fn get_removed_string_enum_values(from_type: &ColumnType, to_type: &ColumnType) -> Option> { + match (from_type, to_type) { + ( + ColumnType::Complex(ComplexColumnType::Enum { + values: EnumValues::String(from_values), + .. + }), + ColumnType::Complex(ComplexColumnType::Enum { + values: EnumValues::String(to_values), + .. + }), + ) => { + let to_set: HashSet<&str> = to_values.iter().map(|s| s.as_str()).collect(); + let removed: Vec = from_values + .iter() + .filter(|v| !to_set.contains(v.as_str())) + .cloned() + .collect(); + if removed.is_empty() { + None + } else { + Some(removed) + } + } + _ => None, + } +} + +/// Extract the unquoted value from a SQL default string. +/// For enum defaults like `'active'`, returns `active`. +/// For values without quotes, returns as-is. +fn extract_unquoted_default(default_sql: &str) -> &str { + let trimmed = default_sql.trim(); + if trimmed.starts_with('\'') && trimmed.ends_with('\'') && trimmed.len() >= 2 { + &trimmed[1..trimmed.len() - 1] + } else { + trimmed + } +} + +/// Sort ModifyColumnType and ModifyColumnDefault actions when they affect the same +/// column AND involve enum value removal where the old default is the removed value. +/// +/// When an enum value is being removed and the current default is that value, +/// the default must be changed BEFORE the type is modified (to remove the enum value). +/// Otherwise, the database will reject the ALTER TYPE because the default still +/// references a value that would be removed. +fn sort_enum_default_dependencies( + actions: &mut [MigrationAction], + from_map: &BTreeMap<&str, &TableDef>, +) { + // Find indices of ModifyColumnType and ModifyColumnDefault actions + // Group by (table, column) + let mut type_changes: BTreeMap<(&str, &str), (usize, &ColumnType)> = BTreeMap::new(); + let mut default_changes: BTreeMap<(&str, &str), usize> = BTreeMap::new(); + + for (i, action) in actions.iter().enumerate() { + match action { + MigrationAction::ModifyColumnType { + table, + column, + new_type, + } => { + type_changes.insert((table.as_str(), column.as_str()), (i, new_type)); + } + MigrationAction::ModifyColumnDefault { table, column, .. } => { + default_changes.insert((table.as_str(), column.as_str()), i); + } + _ => {} + } + } + + // Find pairs that need reordering + let mut swaps: Vec<(usize, usize)> = Vec::new(); + + for ((table, column), (type_idx, new_type)) in &type_changes { + if let Some(&default_idx) = default_changes.get(&(*table, *column)) + && let Some(from_table) = from_map.get(table) + && let Some(from_column) = from_table.columns.iter().find(|c| c.name == *column) + && let Some(removed_values) = + get_removed_string_enum_values(&from_column.r#type, new_type) + && let Some(ref old_default) = from_column.default + { + // Both ModifyColumnType and ModifyColumnDefault exist for same column + // Check if old default is one of the removed enum values + let old_default_sql = old_default.to_sql(); + let old_default_unquoted = extract_unquoted_default(&old_default_sql); + + if removed_values.iter().any(|v| v == old_default_unquoted) + && *type_idx < default_idx + { + // Old default is being removed - must change default BEFORE type + swaps.push((*type_idx, default_idx)); + } + } + } + + // Apply swaps + for (i, j) in swaps { + actions.swap(i, j); + } +} + /// 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. @@ -498,6 +606,10 @@ pub fn diff_schemas(from: &[TableDef], to: &[TableDef]) -> Result, default: &str) -> ColumnDef { + ColumnDef { + name: name.to_string(), + r#type: ColumnType::Complex(ComplexColumnType::Enum { + name: enum_name.to_string(), + values: EnumValues::String(values.into_iter().map(String::from).collect()), + }), + nullable: false, + default: Some(default.into()), + comment: None, + primary_key: None, + unique: None, + index: None, + foreign_key: None, + } + } + + #[test] + fn enum_add_value_with_new_default() { + // Case 1: Add new enum value and change default to that new value + // Expected order: ModifyColumnType FIRST (add value), then ModifyColumnDefault + let from = vec![table( + "orders", + vec![col_enum_with_default( + "status", + "order_status", + vec!["pending", "shipped"], + "'pending'", + )], + vec![], + )]; + + let to = vec![table( + "orders", + vec![col_enum_with_default( + "status", + "order_status", + vec!["pending", "shipped", "delivered"], + "'delivered'", // new default uses newly added value + )], + vec![], + )]; + + let plan = diff_schemas(&from, &to).unwrap(); + + // Should generate both actions + assert_eq!(plan.actions.len(), 2, "Expected 2 actions, got: {:?}", plan.actions); + + // ModifyColumnType should come FIRST (to add the new enum value) + assert!( + matches!(&plan.actions[0], MigrationAction::ModifyColumnType { table, column, .. } + if table == "orders" && column == "status"), + "First action should be ModifyColumnType, got: {:?}", plan.actions[0] + ); + + // ModifyColumnDefault should come SECOND + assert!( + matches!(&plan.actions[1], MigrationAction::ModifyColumnDefault { table, column, .. } + if table == "orders" && column == "status"), + "Second action should be ModifyColumnDefault, got: {:?}", plan.actions[1] + ); + } + + #[test] + fn enum_remove_value_that_was_default() { + // Case 2: Remove enum value that was the default + // Expected order: ModifyColumnDefault FIRST (change away from removed value), + // then ModifyColumnType (remove the value) + let from = vec![table( + "orders", + vec![col_enum_with_default( + "status", + "order_status", + vec!["pending", "shipped", "cancelled"], + "'cancelled'", // default is 'cancelled' which will be removed + )], + vec![], + )]; + + let to = vec![table( + "orders", + vec![col_enum_with_default( + "status", + "order_status", + vec!["pending", "shipped"], // 'cancelled' removed + "'pending'", // default changed to existing value + )], + vec![], + )]; + + let plan = diff_schemas(&from, &to).unwrap(); + + // Should generate both actions + assert_eq!(plan.actions.len(), 2, "Expected 2 actions, got: {:?}", plan.actions); + + // ModifyColumnDefault should come FIRST (change default before removing enum value) + assert!( + matches!(&plan.actions[0], MigrationAction::ModifyColumnDefault { table, column, .. } + if table == "orders" && column == "status"), + "First action should be ModifyColumnDefault, got: {:?}", plan.actions[0] + ); + + // ModifyColumnType should come SECOND (now safe to remove enum value) + assert!( + matches!(&plan.actions[1], MigrationAction::ModifyColumnType { table, column, .. } + if table == "orders" && column == "status"), + "Second action should be ModifyColumnType, got: {:?}", plan.actions[1] + ); + } + + #[test] + fn enum_remove_value_default_unchanged() { + // Remove enum value, but default was NOT that value (no reordering needed) + let from = vec![table( + "orders", + vec![col_enum_with_default( + "status", + "order_status", + vec!["pending", "shipped", "cancelled"], + "'pending'", // default is 'pending', not the removed 'cancelled' + )], + vec![], + )]; + + let to = vec![table( + "orders", + vec![col_enum_with_default( + "status", + "order_status", + vec!["pending", "shipped"], // 'cancelled' removed + "'pending'", // default unchanged + )], + vec![], + )]; + + let plan = diff_schemas(&from, &to).unwrap(); + + // Should generate only ModifyColumnType (default unchanged) + assert_eq!(plan.actions.len(), 1, "Expected 1 action, got: {:?}", plan.actions); + assert!( + matches!(&plan.actions[0], MigrationAction::ModifyColumnType { table, column, .. } + if table == "orders" && column == "status"), + "Action should be ModifyColumnType, got: {:?}", plan.actions[0] + ); + } + + #[test] + fn enum_remove_value_with_default_change_to_remaining() { + // Remove multiple enum values, old default was one of them, new default is a remaining value + let from = vec![table( + "orders", + vec![col_enum_with_default( + "status", + "order_status", + vec!["draft", "pending", "shipped", "delivered", "cancelled"], + "'cancelled'", + )], + vec![], + )]; + + let to = vec![table( + "orders", + vec![col_enum_with_default( + "status", + "order_status", + vec!["pending", "shipped", "delivered"], // removed 'draft' and 'cancelled' + "'pending'", + )], + vec![], + )]; + + let plan = diff_schemas(&from, &to).unwrap(); + + assert_eq!(plan.actions.len(), 2, "Expected 2 actions, got: {:?}", plan.actions); + + // ModifyColumnDefault MUST come first because old default 'cancelled' is being removed + assert!( + matches!(&plan.actions[0], MigrationAction::ModifyColumnDefault { .. }), + "First action should be ModifyColumnDefault, got: {:?}", plan.actions[0] + ); + assert!( + matches!(&plan.actions[1], MigrationAction::ModifyColumnType { .. }), + "Second action should be ModifyColumnType, got: {:?}", plan.actions[1] + ); + } + + #[test] + fn enum_remove_value_with_unquoted_default() { + // Test coverage for extract_unquoted_default else branch (line 335) + // When default value doesn't have quotes, it should still be compared correctly + let from = vec![table( + "orders", + vec![col_enum_with_default( + "status", + "order_status", + vec!["pending", "shipped", "cancelled"], + "cancelled", // unquoted default (no single quotes) + )], + vec![], + )]; + + let to = vec![table( + "orders", + vec![col_enum_with_default( + "status", + "order_status", + vec!["pending", "shipped"], // 'cancelled' removed + "pending", // unquoted default + )], + vec![], + )]; + + let plan = diff_schemas(&from, &to).unwrap(); + + // Should generate both actions + assert_eq!(plan.actions.len(), 2, "Expected 2 actions, got: {:?}", plan.actions); + + // ModifyColumnDefault should come FIRST because unquoted 'cancelled' matches removed value + assert!( + matches!(&plan.actions[0], MigrationAction::ModifyColumnDefault { .. }), + "First action should be ModifyColumnDefault, got: {:?}", plan.actions[0] + ); + assert!( + matches!(&plan.actions[1], MigrationAction::ModifyColumnType { .. }), + "Second action should be ModifyColumnType, got: {:?}", plan.actions[1] + ); + } + } + // Tests for inline column constraints normalization mod inline_constraints { use super::*; diff --git a/crates/vespertide-query/src/sql/modify_column_default.rs b/crates/vespertide-query/src/sql/modify_column_default.rs index 91c497d..df85237 100644 --- a/crates/vespertide-query/src/sql/modify_column_default.rs +++ b/crates/vespertide-query/src/sql/modify_column_default.rs @@ -288,6 +288,41 @@ mod tests { assert!(err_msg.contains("Column 'email' not found")); } + /// Test Postgres default change when column is not in schema + /// This covers the fallback path where column_type is None + #[test] + fn test_postgres_column_not_in_schema_uses_default_as_is() { + let schema = vec![table_def( + "users", + vec![col( + "id", + ColumnType::Simple(SimpleColumnType::Integer), + false, + )], + // Note: "status" column is NOT in the schema + vec![], + )]; + + // Postgres doesn't error when column isn't found - it just uses the default as-is + let result = build_modify_column_default( + &DatabaseBackend::Postgres, + "users", + "status", // column not in schema + Some("'active'"), + &schema, + ); + assert!(result.is_ok()); + let queries = result.unwrap(); + let sql = queries + .iter() + .map(|q| q.build(DatabaseBackend::Postgres)) + .collect::>() + .join("\n"); + + // Should still generate valid SQL, using the default value as-is + assert!(sql.contains("ALTER TABLE \"users\" ALTER COLUMN \"status\" SET DEFAULT 'active'")); + } + /// Test with index - should recreate index after table rebuild (SQLite) #[rstest] #[case::postgres_with_index(DatabaseBackend::Postgres)] From 5960f059add0b03334a478103624acef1196b1cb Mon Sep 17 00:00:00 2001 From: owjs3901 Date: Wed, 28 Jan 2026 13:57:39 +0900 Subject: [PATCH 2/2] Fix lint --- crates/vespertide-planner/src/diff.rs | 94 ++++++++++++++++++++------- 1 file changed, 70 insertions(+), 24 deletions(-) diff --git a/crates/vespertide-planner/src/diff.rs b/crates/vespertide-planner/src/diff.rs index 8309ff1..352e1a6 100644 --- a/crates/vespertide-planner/src/diff.rs +++ b/crates/vespertide-planner/src/diff.rs @@ -296,7 +296,10 @@ fn sort_create_before_add_constraint(actions: &mut [MigrationAction]) { /// Get the set of string enum values that were removed (present in `from` but not in `to`). /// Returns None if either type is not a string enum. -fn get_removed_string_enum_values(from_type: &ColumnType, to_type: &ColumnType) -> Option> { +fn get_removed_string_enum_values( + from_type: &ColumnType, + to_type: &ColumnType, +) -> Option> { match (from_type, to_type) { ( ColumnType::Complex(ComplexColumnType::Enum { @@ -384,9 +387,7 @@ fn sort_enum_default_dependencies( let old_default_sql = old_default.to_sql(); let old_default_unquoted = extract_unquoted_default(&old_default_sql); - if removed_values.iter().any(|v| v == old_default_unquoted) - && *type_idx < default_idx - { + if removed_values.iter().any(|v| v == old_default_unquoted) && *type_idx < default_idx { // Old default is being removed - must change default BEFORE type swaps.push((*type_idx, default_idx)); } @@ -900,7 +901,12 @@ mod tests { use super::*; use vespertide_core::{ComplexColumnType, EnumValues}; - fn col_enum_with_default(name: &str, enum_name: &str, values: Vec<&str>, default: &str) -> ColumnDef { + fn col_enum_with_default( + name: &str, + enum_name: &str, + values: Vec<&str>, + default: &str, + ) -> ColumnDef { ColumnDef { name: name.to_string(), r#type: ColumnType::Complex(ComplexColumnType::Enum { @@ -946,20 +952,27 @@ mod tests { let plan = diff_schemas(&from, &to).unwrap(); // Should generate both actions - assert_eq!(plan.actions.len(), 2, "Expected 2 actions, got: {:?}", plan.actions); + assert_eq!( + plan.actions.len(), + 2, + "Expected 2 actions, got: {:?}", + plan.actions + ); // ModifyColumnType should come FIRST (to add the new enum value) assert!( matches!(&plan.actions[0], MigrationAction::ModifyColumnType { table, column, .. } if table == "orders" && column == "status"), - "First action should be ModifyColumnType, got: {:?}", plan.actions[0] + "First action should be ModifyColumnType, got: {:?}", + plan.actions[0] ); // ModifyColumnDefault should come SECOND assert!( matches!(&plan.actions[1], MigrationAction::ModifyColumnDefault { table, column, .. } if table == "orders" && column == "status"), - "Second action should be ModifyColumnDefault, got: {:?}", plan.actions[1] + "Second action should be ModifyColumnDefault, got: {:?}", + plan.actions[1] ); } @@ -985,7 +998,7 @@ mod tests { "status", "order_status", vec!["pending", "shipped"], // 'cancelled' removed - "'pending'", // default changed to existing value + "'pending'", // default changed to existing value )], vec![], )]; @@ -993,20 +1006,27 @@ mod tests { let plan = diff_schemas(&from, &to).unwrap(); // Should generate both actions - assert_eq!(plan.actions.len(), 2, "Expected 2 actions, got: {:?}", plan.actions); + assert_eq!( + plan.actions.len(), + 2, + "Expected 2 actions, got: {:?}", + plan.actions + ); // ModifyColumnDefault should come FIRST (change default before removing enum value) assert!( matches!(&plan.actions[0], MigrationAction::ModifyColumnDefault { table, column, .. } if table == "orders" && column == "status"), - "First action should be ModifyColumnDefault, got: {:?}", plan.actions[0] + "First action should be ModifyColumnDefault, got: {:?}", + plan.actions[0] ); // ModifyColumnType should come SECOND (now safe to remove enum value) assert!( matches!(&plan.actions[1], MigrationAction::ModifyColumnType { table, column, .. } if table == "orders" && column == "status"), - "Second action should be ModifyColumnType, got: {:?}", plan.actions[1] + "Second action should be ModifyColumnType, got: {:?}", + plan.actions[1] ); } @@ -1030,7 +1050,7 @@ mod tests { "status", "order_status", vec!["pending", "shipped"], // 'cancelled' removed - "'pending'", // default unchanged + "'pending'", // default unchanged )], vec![], )]; @@ -1038,11 +1058,17 @@ mod tests { let plan = diff_schemas(&from, &to).unwrap(); // Should generate only ModifyColumnType (default unchanged) - assert_eq!(plan.actions.len(), 1, "Expected 1 action, got: {:?}", plan.actions); + assert_eq!( + plan.actions.len(), + 1, + "Expected 1 action, got: {:?}", + plan.actions + ); assert!( matches!(&plan.actions[0], MigrationAction::ModifyColumnType { table, column, .. } if table == "orders" && column == "status"), - "Action should be ModifyColumnType, got: {:?}", plan.actions[0] + "Action should be ModifyColumnType, got: {:?}", + plan.actions[0] ); } @@ -1073,16 +1099,26 @@ mod tests { let plan = diff_schemas(&from, &to).unwrap(); - assert_eq!(plan.actions.len(), 2, "Expected 2 actions, got: {:?}", plan.actions); + assert_eq!( + plan.actions.len(), + 2, + "Expected 2 actions, got: {:?}", + plan.actions + ); // ModifyColumnDefault MUST come first because old default 'cancelled' is being removed assert!( - matches!(&plan.actions[0], MigrationAction::ModifyColumnDefault { .. }), - "First action should be ModifyColumnDefault, got: {:?}", plan.actions[0] + matches!( + &plan.actions[0], + MigrationAction::ModifyColumnDefault { .. } + ), + "First action should be ModifyColumnDefault, got: {:?}", + plan.actions[0] ); assert!( matches!(&plan.actions[1], MigrationAction::ModifyColumnType { .. }), - "Second action should be ModifyColumnType, got: {:?}", plan.actions[1] + "Second action should be ModifyColumnType, got: {:?}", + plan.actions[1] ); } @@ -1107,7 +1143,7 @@ mod tests { "status", "order_status", vec!["pending", "shipped"], // 'cancelled' removed - "pending", // unquoted default + "pending", // unquoted default )], vec![], )]; @@ -1115,16 +1151,26 @@ mod tests { let plan = diff_schemas(&from, &to).unwrap(); // Should generate both actions - assert_eq!(plan.actions.len(), 2, "Expected 2 actions, got: {:?}", plan.actions); + assert_eq!( + plan.actions.len(), + 2, + "Expected 2 actions, got: {:?}", + plan.actions + ); // ModifyColumnDefault should come FIRST because unquoted 'cancelled' matches removed value assert!( - matches!(&plan.actions[0], MigrationAction::ModifyColumnDefault { .. }), - "First action should be ModifyColumnDefault, got: {:?}", plan.actions[0] + matches!( + &plan.actions[0], + MigrationAction::ModifyColumnDefault { .. } + ), + "First action should be ModifyColumnDefault, got: {:?}", + plan.actions[0] ); assert!( matches!(&plan.actions[1], MigrationAction::ModifyColumnType { .. }), - "Second action should be ModifyColumnType, got: {:?}", plan.actions[1] + "Second action should be ModifyColumnType, got: {:?}", + plan.actions[1] ); } }