From 90fbe6edf6889b635ed5d9c2b549494b1576c20c Mon Sep 17 00:00:00 2001 From: nikita42 <13642481+rekurt@users.noreply.github.com> Date: Wed, 8 Apr 2026 15:21:06 +0300 Subject: [PATCH 1/5] feat: production-readiness improvements with PK tracking, transactions, CONCURRENTLY, and rename detection - fix(loader): resolve pg_sequences panic by casting regtype to text and handling nullable fields - feat(model): add PrimaryKey variant to ConstraintKind, track PKs across all loaders (PG, MySQL, SQLite, SQL files) - feat(cli): add --no-transaction, --concurrently, --detect-renames flags - feat(migration): wrap migrations in BEGIN/COMMIT by default, support CREATE INDEX CONCURRENTLY (PG only) - feat(diff): column/table rename detection heuristic (opt-in, experimental) - High confidence: type+nullability+default match - Medium confidence: type+nullability match (SQL commented out for safety) - refactor: extract command handlers from main.rs into src/commands/ module - refactor: split migration.rs into migration/mod.rs + migration/sql.rs - fix(ci): DROP COLUMN now correctly reported as blocking in CI report - fix(ci): renamed tables/columns now included in CI report changes - fix(migration): rollback renames now execute before constraint re-addition - fix(migration): MySQL PK uses ADD PRIMARY KEY without named constraint - fix(output): print_diff now displays renamed tables and columns - fix(error): replace unwrap_or_default with proper error propagation for JSON/YAML serialization - fix(loader): SQLite error propagation instead of silent filter_map(ok()) - fix(loader): emit warning when sequence metadata is NULL (permissions) - chore: remove unused Trigger/Function model types - chore: add RenameConfidence Copy/Display derives - test: add 22 new tests for PK, renames, CONCURRENTLY, transactions --- src/ci.rs | 69 +- src/cli.rs | 35 + src/commands/diff.rs | 316 +++++++++ src/commands/helpers.rs | 40 ++ src/commands/init.rs | 19 + src/commands/mod.rs | 12 + src/commands/snapshot.rs | 50 ++ src/commands/tables.rs | 54 ++ src/commands/validate.rs | 52 ++ src/diff.rs | 462 ++++++++++++- src/loader/mysql.rs | 37 ++ src/loader/postgres.rs | 46 +- src/loader/sqlfile.rs | 17 +- src/loader/sqlite.rs | 50 +- src/main.rs | 524 +-------------- src/{migration.rs => migration/mod.rs} | 880 ++++++++++++------------- src/migration/sql.rs | 429 ++++++++++++ src/model.rs | 27 +- src/output.rs | 74 ++- 19 files changed, 2158 insertions(+), 1035 deletions(-) create mode 100644 src/commands/diff.rs create mode 100644 src/commands/helpers.rs create mode 100644 src/commands/init.rs create mode 100644 src/commands/mod.rs create mode 100644 src/commands/snapshot.rs create mode 100644 src/commands/tables.rs create mode 100644 src/commands/validate.rs rename src/{migration.rs => migration/mod.rs} (72%) create mode 100644 src/migration/sql.rs diff --git a/src/ci.rs b/src/ci.rs index 31b9be3..38352cb 100644 --- a/src/ci.rs +++ b/src/ci.rs @@ -96,10 +96,49 @@ impl CiReport { }); } + // Renamed tables + for rename in &diff.renamed_tables { + let stmt = statements.iter().find(|s| { + s.sql.contains("RENAME") && s.sql.contains(&rename.old_name) + }); + changes.push(CiChange { + change_type: "RENAME".to_string(), + object: "TABLE".to_string(), + table: rename.old_name.clone(), + name: rename.new_name.clone(), + description: format!( + "RENAME TABLE {} → {}", + rename.old_name, rename.new_name + ), + is_blocking: false, + sql: stmt.map(|s| s.sql.clone()).unwrap_or_default(), + }); + } + // Modified tables for table_diff in &diff.modified_tables { let tname = &table_diff.table_name; + for rename in &table_diff.renamed_columns { + let stmt = statements.iter().find(|s| { + s.sql.contains("RENAME COLUMN") + && s.sql.contains(&rename.old.name) + && s.sql.contains(tname.as_str()) + }); + changes.push(CiChange { + change_type: "RENAME".to_string(), + object: "COLUMN".to_string(), + table: tname.clone(), + name: format!("{} → {}", rename.old.name, rename.new.name), + description: format!( + "RENAME COLUMN {} → {} ({} confidence)", + rename.old.name, rename.new.name, rename.confidence + ), + is_blocking: false, + sql: stmt.map(|s| s.sql.clone()).unwrap_or_default(), + }); + } + for col in &table_diff.added_columns { let stmt = statements.iter().find(|s| { s.sql.contains("ADD COLUMN") @@ -119,22 +158,20 @@ impl CiReport { } for col in &table_diff.removed_columns { + let stmt = statements.iter().find(|s| { + s.sql.contains("DROP COLUMN") + && s.sql.contains(&col.name) + && s.sql.contains(tname.as_str()) + }); + let blocking = stmt.map(|s| s.is_blocking).unwrap_or(true); changes.push(CiChange { change_type: "DROP".to_string(), object: "COLUMN".to_string(), table: tname.clone(), name: col.name.clone(), description: format!("DROP COLUMN {}", col.name), - is_blocking: false, - sql: statements - .iter() - .find(|s| { - s.sql.contains("DROP COLUMN") - && s.sql.contains(&col.name) - && s.sql.contains(tname.as_str()) - }) - .map(|s| s.sql.clone()) - .unwrap_or_default(), + is_blocking: blocking, + sql: stmt.map(|s| s.sql.clone()).unwrap_or_default(), }); } @@ -397,7 +434,7 @@ mod tests { fn exit_ok_for_identical_schemas() { let schema = Schema::new(); let diff = diff_schemas(&schema, &schema); - let stmts = generate_migration(&diff, SqlDialect::Postgres); + let stmts = generate_migration(&diff, SqlDialect::Postgres, false); let report = CiReport::from_diff(&diff, &stmts); assert!(report.equal); @@ -424,7 +461,7 @@ mod tests { right.tables.insert("users".into(), t); let diff = diff_schemas(&left, &right); - let stmts = generate_migration(&diff, SqlDialect::Postgres); + let stmts = generate_migration(&diff, SqlDialect::Postgres, false); let report = CiReport::from_diff(&diff, &stmts); assert!(!report.equal); @@ -452,7 +489,7 @@ mod tests { right.tables.insert("orders".into(), t); let diff = diff_schemas(&left, &right); - let stmts = generate_migration(&diff, SqlDialect::Postgres); + let stmts = generate_migration(&diff, SqlDialect::Postgres, false); let report = CiReport::from_diff(&diff, &stmts); assert!(!report.equal); @@ -470,7 +507,7 @@ mod tests { right.tables.insert("x".into(), t); let diff = diff_schemas(&left, &right); - let stmts = generate_migration(&diff, SqlDialect::Postgres); + let stmts = generate_migration(&diff, SqlDialect::Postgres, false); let report = CiReport::from_diff(&diff, &stmts); assert!(report.summary.contains("1 change(s) detected")); @@ -485,7 +522,7 @@ mod tests { right.tables.insert("orders".into(), t); let diff = diff_schemas(&left, &right); - let stmts = generate_migration(&diff, SqlDialect::Postgres); + let stmts = generate_migration(&diff, SqlDialect::Postgres, false); let report = CiReport::from_diff(&diff, &stmts); let json = serde_json::to_string_pretty(&report).unwrap(); @@ -503,7 +540,7 @@ mod tests { right.tables.insert("users".into(), t); let diff = diff_schemas(&left, &right); - let stmts = generate_migration(&diff, SqlDialect::Postgres); + let stmts = generate_migration(&diff, SqlDialect::Postgres, false); let report = CiReport::from_diff(&diff, &stmts); let yaml = serde_yaml::to_string(&report).unwrap(); diff --git a/src/cli.rs b/src/cli.rs index f811cd0..8dccbe6 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -78,6 +78,18 @@ pub struct Cli { /// Show explanation for each migration statement #[arg(long)] pub explain: bool, + + /// Disable transaction wrapping (BEGIN/COMMIT) around migration SQL + #[arg(long)] + pub no_transaction: bool, + + /// Use CREATE INDEX CONCURRENTLY for non-blocking index creation (PostgreSQL only, implies --no-transaction) + #[arg(long)] + pub concurrently: bool, + + /// Detect column/table renames instead of DROP+ADD (experimental) + #[arg(long)] + pub detect_renames: bool, } /// Color output mode. @@ -202,6 +214,18 @@ pub struct DiffArgs { /// Show explanation for each migration statement #[arg(long)] pub explain: bool, + + /// Disable transaction wrapping (BEGIN/COMMIT) around migration SQL + #[arg(long)] + pub no_transaction: bool, + + /// Use CREATE INDEX CONCURRENTLY for non-blocking index creation (PostgreSQL only, implies --no-transaction) + #[arg(long)] + pub concurrently: bool, + + /// Detect column/table renames instead of DROP+ADD (experimental) + #[arg(long)] + pub detect_renames: bool, } /// Migration direction for output. @@ -279,6 +303,9 @@ pub struct DiffParams { pub color: ColorMode, pub explain: bool, pub fail_on_blocking: bool, + pub transaction: bool, + pub concurrently: bool, + pub detect_renames: bool, } impl Cli { @@ -307,6 +334,7 @@ impl Cli { } else { false }; + let transaction = !self.no_transaction && !self.concurrently; ResolvedCommand::Diff(DiffParams { source, target_source, @@ -321,6 +349,9 @@ impl Cli { color: self.color, explain: self.explain, fail_on_blocking: self.fail_on_blocking, + transaction, + concurrently: self.concurrently, + detect_renames: self.detect_renames, }) } } @@ -348,6 +379,7 @@ impl DiffArgs { false }; + let transaction = !self.no_transaction && !self.concurrently; DiffParams { source: self.source, target_source, @@ -362,6 +394,9 @@ impl DiffArgs { color: self.color, explain: self.explain, fail_on_blocking: self.fail_on_blocking, + transaction, + concurrently: self.concurrently, + detect_renames: self.detect_renames, } } } diff --git a/src/commands/diff.rs b/src/commands/diff.rs new file mode 100644 index 0000000..7490dad --- /dev/null +++ b/src/commands/diff.rs @@ -0,0 +1,316 @@ +use std::process::ExitCode; + +use dbdiff::ci::{self, CiReport}; +use dbdiff::cli::{DiffParams, MigrationDirection, OutputFormat}; +use dbdiff::config; +use dbdiff::loader; +use dbdiff::loader::SqlDialect; +use dbdiff::migration; +use dbdiff::output; + +use super::helpers::{apply_color_mode, create_spinner, resolve_ssl_mode}; + +pub async fn run_diff(params: DiffParams) -> Result<(), ExitCode> { + apply_color_mode(params.color); + + if params.target_source.is_empty() { + eprintln!("Error: Either a target DSN or --schema is required"); + return Err(ExitCode::from(2)); + } + + let cfg = config::load_config(¶ms.config).map_err(|e| { + eprintln!("Error: {e}"); + ExitCode::from(2) + })?; + + let spinner = create_spinner("Loading schemas..."); + + let timeout_duration = std::time::Duration::from_secs(params.timeout); + let ssl_mode = resolve_ssl_mode(params.ssl_mode); + + let load_result = tokio::time::timeout(timeout_duration, async { + tokio::try_join!( + loader::load_schema_with_ssl(¶ms.source, ssl_mode), + loader::load_schema_with_ssl(¶ms.target_source, ssl_mode), + ) + }) + .await; + + spinner.finish_and_clear(); + + let (mut left, mut right) = match load_result { + Ok(Ok(schemas)) => schemas, + Ok(Err(e)) => { + eprintln!("Error: {e}"); + return Err(ExitCode::from(2)); + } + Err(_) => { + let safe_source = dbdiff::error::sanitize_dsn(¶ms.source); + let safe_target = dbdiff::error::sanitize_dsn(¶ms.target_source); + let err = dbdiff::error::DbDiffError::timeout( + &format!("{safe_source} / {safe_target}"), + params.timeout, + ); + eprintln!("Error: {err}"); + return Err(ExitCode::from(2)); + } + }; + + config::filter::apply_ignore(&mut left.schema, &cfg.ignore); + config::filter::apply_ignore(&mut right.schema, &cfg.ignore); + + // When comparing against SQL files (not JSON snapshots), clear object types + // that the SQL parser cannot load (views, enums, sequences) to avoid false diffs. + // JSON snapshots (SqlDialect::Snapshot) DO carry these objects, so don't clear them. + if left.dialect == SqlDialect::SqlFile { + right.schema.views.clear(); + right.schema.enums.clear(); + right.schema.sequences.clear(); + } + if right.dialect == SqlDialect::SqlFile { + left.schema.views.clear(); + left.schema.enums.clear(); + left.schema.sequences.clear(); + } + + if let Some(false) = cfg.output.color { + colored::control::set_override(false); + } + + let diff = dbdiff::diff::diff_schemas_with_options( + &left.schema, + &right.schema, + params.detect_renames, + ); + + if let Err(msg) = check_protected(&diff, &cfg.protected, params.direction) { + eprintln!("Error: {msg}"); + return Err(ExitCode::from(2)); + } + + let migration_dialect = match (left.dialect, right.dialect) { + ( + SqlDialect::SqlFile | SqlDialect::Snapshot, + SqlDialect::SqlFile | SqlDialect::Snapshot, + ) => SqlDialect::Postgres, + (SqlDialect::SqlFile | SqlDialect::Snapshot, other) + | (other, SqlDialect::SqlFile | SqlDialect::Snapshot) => other, + (l, r) if l == r => l, + (l, r) => { + eprintln!( + "Error: Cannot generate migration for mixed backends ({l:?} vs {r:?}). \ + Compare like-for-like backends or use a .sql file for one side." + ); + return Err(ExitCode::from(2)); + } + }; + + let up_statements = migration::generate_migration(&diff, migration_dialect, params.concurrently); + let down_statements = migration::generate_rollback(&diff, migration_dialect, params.concurrently); + let format = params.resolve_format(&cfg.output.format); + + let report = match params.direction { + MigrationDirection::Down => CiReport::from_diff_reversed(&diff, &down_statements), + _ => CiReport::from_diff(&diff, &up_statements), + }; + + match format { + OutputFormat::Pretty => { + output::print_diff(&diff); + + let print_fn = if params.explain { + output::print_migration_explained + } else { + output::print_migration + }; + + match params.direction { + MigrationDirection::Up => { + if !up_statements.is_empty() { + println!(); + print_fn(&up_statements); + } + } + MigrationDirection::Down => { + if !down_statements.is_empty() { + println!(); + println!( + "{}", + colored::Colorize::bold(colored::Colorize::underline( + "Rollback migration" + )) + ); + println!(); + print_fn(&down_statements); + } + } + MigrationDirection::Both => { + if !up_statements.is_empty() { + println!(); + output::print_migration(&up_statements); + } + if !down_statements.is_empty() { + println!(); + println!( + "{}", + colored::Colorize::bold(colored::Colorize::underline( + "Rollback migration" + )) + ); + println!(); + print_fn(&down_statements); + } + } + } + + println!(); + output::print_summary(&diff); + } + OutputFormat::Json => { + let json = serde_json::to_string_pretty(&report).map_err(|e| { + eprintln!("Error: failed to serialize report as JSON: {e}"); + ExitCode::from(2) + })?; + println!("{json}"); + } + OutputFormat::Yaml => { + let yaml = serde_yaml::to_string(&report).map_err(|e| { + eprintln!("Error: failed to serialize report as YAML: {e}"); + ExitCode::from(2) + })?; + print!("{yaml}"); + } + OutputFormat::Sql => match params.direction { + MigrationDirection::Up => { + print!("{}", output::migration_to_sql(&up_statements, params.transaction)); + } + MigrationDirection::Down => { + print!("{}", output::migration_to_sql(&down_statements, params.transaction)); + } + MigrationDirection::Both => { + println!("-- === UP ==="); + print!("{}", output::migration_to_sql(&up_statements, params.transaction)); + println!("-- === DOWN ==="); + print!("{}", output::migration_to_sql(&down_statements, params.transaction)); + } + }, + } + + if let Some(ref path) = params.out { + if params.should_write { + let sql = match params.direction { + MigrationDirection::Up => output::migration_to_sql(&up_statements, params.transaction), + MigrationDirection::Down => output::migration_to_sql(&down_statements, params.transaction), + MigrationDirection::Both => { + format!( + "-- === UP ===\n{}\n-- === DOWN ===\n{}", + output::migration_to_sql(&up_statements, params.transaction), + output::migration_to_sql(&down_statements, params.transaction) + ) + } + }; + std::fs::write(path, sql).map_err(|e| { + eprintln!("Error writing migration file: {e}"); + ExitCode::from(2) + })?; + eprintln!("Migration written to {path}"); + } else { + eprintln!("Dry run: would write migration to {path} (use --write to save)"); + } + } + + if params.ci && std::env::var("GITHUB_ACTIONS").as_deref() == Ok("true") { + report.emit_github_annotations(); + } + + if params.ci { + let code = report.exit_code(params.fail_on_blocking); + if code != ci::EXIT_OK { + return Err(ExitCode::from(code)); + } + } + + Ok(()) +} + +fn check_protected( + diff: &dbdiff::diff::SchemaDiff, + protected: &config::ProtectedConfig, + direction: MigrationDirection, +) -> Result<(), String> { + let check_up = !matches!(direction, MigrationDirection::Down); + let check_down = !matches!(direction, MigrationDirection::Up); + + let dropped_tables: Vec<&dbdiff::model::Table> = std::iter::empty() + .chain(if check_up { + diff.removed_tables.iter().collect::>() + } else { + vec![] + }) + .chain(if check_down { + diff.added_tables.iter().collect::>() + } else { + vec![] + }) + .collect(); + + for table in &dropped_tables { + if protected.tables.contains(&table.name) { + return Err(format!( + "Protected table '{}' would be dropped. Remove it from the protected list to allow this.", + table.name + )); + } + for col in table.columns.values() { + if column_is_protected(&table.name, &col.name, &protected.columns) { + return Err(format!( + "Protected column '{}.{}' would be dropped (table '{}' is being removed). \ + Remove it from the protected list to allow this.", + table.name, col.name, table.name + )); + } + } + } + + for table_diff in &diff.modified_tables { + let dropped_cols: Vec<&dbdiff::model::Column> = std::iter::empty() + .chain(if check_up { + table_diff.removed_columns.iter().collect::>() + } else { + vec![] + }) + .chain(if check_down { + table_diff.added_columns.iter().collect::>() + } else { + vec![] + }) + .collect(); + + for col in &dropped_cols { + if column_is_protected(&table_diff.table_name, &col.name, &protected.columns) { + return Err(format!( + "Protected column '{}.{}' would be dropped. Remove it from the protected list to allow this.", + table_diff.table_name, col.name + )); + } + } + } + + Ok(()) +} + +fn column_is_protected(table_name: &str, col_name: &str, patterns: &[String]) -> bool { + patterns.iter().any(|pattern| { + if let Some(c) = pattern.strip_prefix("*.") { + col_name == c + } else if let Some((t, c)) = pattern.split_once('.') { + if c == "*" { + table_name == t + } else { + table_name == t && col_name == c + } + } else { + false + } + }) +} diff --git a/src/commands/helpers.rs b/src/commands/helpers.rs new file mode 100644 index 0000000..7480403 --- /dev/null +++ b/src/commands/helpers.rs @@ -0,0 +1,40 @@ +use indicatif::{ProgressBar, ProgressStyle}; + +use dbdiff::cli::{ColorMode, SslMode}; +use dbdiff::loader; + +pub fn apply_color_mode(mode: ColorMode) { + match mode { + ColorMode::Auto => { + if !std::io::IsTerminal::is_terminal(&std::io::stdout()) { + colored::control::set_override(false); + } + } + ColorMode::Always => { + colored::control::set_override(true); + } + ColorMode::Never => { + colored::control::set_override(false); + } + } +} + +pub fn resolve_ssl_mode(mode: SslMode) -> loader::SslMode { + match mode { + SslMode::Disable => loader::SslMode::Disable, + SslMode::Prefer => loader::SslMode::Prefer, + SslMode::Require => loader::SslMode::Require, + } +} + +pub fn create_spinner(msg: &str) -> ProgressBar { + let pb = ProgressBar::new_spinner(); + pb.set_style( + ProgressStyle::with_template("{spinner:.cyan} {msg}") + .unwrap() + .tick_strings(&["⠋", "⠙", "⠹", "⠸", "⠼", "⠴", "⠦", "⠧", "⠇", "⠏"]), + ); + pb.set_message(msg.to_string()); + pb.enable_steady_tick(std::time::Duration::from_millis(80)); + pb +} diff --git a/src/commands/init.rs b/src/commands/init.rs new file mode 100644 index 0000000..a39306e --- /dev/null +++ b/src/commands/init.rs @@ -0,0 +1,19 @@ +use std::process::ExitCode; + +use dbdiff::config; + +pub fn run_init() -> Result<(), ExitCode> { + let path = ".dbdiff.yml"; + if std::path::Path::new(path).exists() { + eprintln!("Config file '{path}' already exists. Remove it first to regenerate."); + return Err(ExitCode::from(2)); + } + + std::fs::write(path, config::DEFAULT_CONFIG_TEMPLATE).map_err(|e| { + eprintln!("Error writing config file: {e}"); + ExitCode::from(2) + })?; + + eprintln!("Created {path}"); + Ok(()) +} diff --git a/src/commands/mod.rs b/src/commands/mod.rs new file mode 100644 index 0000000..6bff2f8 --- /dev/null +++ b/src/commands/mod.rs @@ -0,0 +1,12 @@ +mod diff; +mod helpers; +mod init; +mod snapshot; +mod tables; +mod validate; + +pub use diff::run_diff; +pub use init::run_init; +pub use snapshot::run_snapshot; +pub use tables::run_tables; +pub use validate::run_validate; diff --git a/src/commands/snapshot.rs b/src/commands/snapshot.rs new file mode 100644 index 0000000..fa4e0e0 --- /dev/null +++ b/src/commands/snapshot.rs @@ -0,0 +1,50 @@ +use std::process::ExitCode; + +use dbdiff::cli::SnapshotArgs; +use dbdiff::loader; + +use super::helpers::{create_spinner, resolve_ssl_mode}; + +pub async fn run_snapshot(args: SnapshotArgs) -> Result<(), ExitCode> { + let spinner = create_spinner("Loading schema..."); + let ssl_mode = resolve_ssl_mode(args.ssl_mode); + let timeout_duration = std::time::Duration::from_secs(args.timeout); + + let result = tokio::time::timeout( + timeout_duration, + loader::load_schema_with_ssl(&args.dsn, ssl_mode), + ) + .await; + + spinner.finish_and_clear(); + + match result { + Ok(Ok(loaded)) => { + let snapshot = dbdiff::model::SchemaSnapshot::from(&loaded.schema); + let json = serde_json::to_string_pretty(&snapshot).map_err(|e| { + eprintln!("Error: failed to serialize snapshot as JSON: {e}"); + ExitCode::from(2) + })?; + + if let Some(ref path) = args.out { + std::fs::write(path, &json).map_err(|e| { + eprintln!("Error writing snapshot: {e}"); + ExitCode::from(2) + })?; + eprintln!("Snapshot written to {path}"); + } else { + println!("{json}"); + } + Ok(()) + } + Ok(Err(e)) => { + eprintln!("Error: {e}"); + Err(ExitCode::from(2)) + } + Err(_) => { + let err = dbdiff::error::DbDiffError::timeout(&args.dsn, args.timeout); + eprintln!("Error: {err}"); + Err(ExitCode::from(2)) + } + } +} diff --git a/src/commands/tables.rs b/src/commands/tables.rs new file mode 100644 index 0000000..f93782c --- /dev/null +++ b/src/commands/tables.rs @@ -0,0 +1,54 @@ +use std::process::ExitCode; + +use dbdiff::cli::TablesArgs; +use dbdiff::loader; + +use super::helpers::{create_spinner, resolve_ssl_mode}; + +pub async fn run_tables(args: TablesArgs) -> Result<(), ExitCode> { + let spinner = create_spinner("Loading tables..."); + let ssl_mode = resolve_ssl_mode(args.ssl_mode); + let timeout_duration = std::time::Duration::from_secs(args.timeout); + + let result = tokio::time::timeout( + timeout_duration, + loader::load_schema_with_ssl(&args.dsn, ssl_mode), + ) + .await; + + spinner.finish_and_clear(); + + match result { + Ok(Ok(loaded)) => { + if loaded.schema.tables.is_empty() { + println!("No tables found."); + return Ok(()); + } + + println!("{:<30} {:>8} {:>8}", "TABLE", "COLUMNS", "INDEXES"); + println!("{}", "-".repeat(48)); + + for (name, table) in &loaded.schema.tables { + println!( + "{:<30} {:>8} {:>8}", + name, + table.columns.len(), + table.indexes.len() + ); + } + + println!("{}", "-".repeat(48)); + println!("{} table(s) total", loaded.schema.tables.len()); + Ok(()) + } + Ok(Err(e)) => { + eprintln!("Error: {e}"); + Err(ExitCode::from(2)) + } + Err(_) => { + let err = dbdiff::error::DbDiffError::timeout(&args.dsn, args.timeout); + eprintln!("Error: {err}"); + Err(ExitCode::from(2)) + } + } +} diff --git a/src/commands/validate.rs b/src/commands/validate.rs new file mode 100644 index 0000000..cc2ccd4 --- /dev/null +++ b/src/commands/validate.rs @@ -0,0 +1,52 @@ +use std::process::ExitCode; +use std::time::Instant; + +use dbdiff::cli::ValidateArgs; +use dbdiff::loader; + +use super::helpers::{create_spinner, resolve_ssl_mode}; + +pub async fn run_validate(args: ValidateArgs) -> Result<(), ExitCode> { + let spinner = create_spinner("Connecting..."); + let start = Instant::now(); + + let timeout_duration = std::time::Duration::from_secs(args.timeout); + let ssl_mode = resolve_ssl_mode(args.ssl_mode); + + let result = tokio::time::timeout( + timeout_duration, + loader::load_schema_with_ssl(&args.dsn, ssl_mode), + ) + .await; + + let latency = start.elapsed(); + spinner.finish_and_clear(); + + match result { + Ok(Ok(loaded)) => { + let table_count = loaded.schema.tables.len(); + let column_count: usize = loaded.schema.tables.values().map(|t| t.columns.len()).sum(); + let index_count: usize = loaded.schema.tables.values().map(|t| t.indexes.len()).sum(); + + use colored::Colorize; + println!( + "{}", + format!(" Connected to {:?} database", loaded.dialect).green() + ); + println!(" Tables: {table_count}"); + println!(" Columns: {column_count}"); + println!(" Indexes: {index_count}"); + println!(" Latency: {}ms", latency.as_millis()); + Ok(()) + } + Ok(Err(e)) => { + eprintln!("Error: {e}"); + Err(ExitCode::from(2)) + } + Err(_) => { + let err = dbdiff::error::DbDiffError::timeout(&args.dsn, args.timeout); + eprintln!("Error: {err}"); + Err(ExitCode::from(2)) + } + } +} diff --git a/src/diff.rs b/src/diff.rs index 2738430..b9729ff 100644 --- a/src/diff.rs +++ b/src/diff.rs @@ -7,6 +7,7 @@ use crate::model::{Column, Constraint, EnumType, Index, Schema, Sequence, Table, pub struct SchemaDiff { pub added_tables: Vec, pub removed_tables: Vec
, + pub renamed_tables: Vec, pub modified_tables: Vec, pub unchanged_tables: Vec, pub added_views: Vec, @@ -25,6 +26,7 @@ impl SchemaDiff { pub fn is_empty(&self) -> bool { self.added_tables.is_empty() && self.removed_tables.is_empty() + && self.renamed_tables.is_empty() && self.modified_tables.is_empty() && self.added_views.is_empty() && self.removed_views.is_empty() @@ -44,6 +46,7 @@ pub struct TableDiff { pub table_name: String, pub added_columns: Vec, pub removed_columns: Vec, + pub renamed_columns: Vec, pub modified_columns: Vec, pub unchanged_columns: Vec, pub added_indexes: Vec, @@ -56,6 +59,7 @@ impl TableDiff { pub fn is_empty(&self) -> bool { self.added_columns.is_empty() && self.removed_columns.is_empty() + && self.renamed_columns.is_empty() && self.modified_columns.is_empty() && self.added_indexes.is_empty() && self.removed_indexes.is_empty() @@ -97,14 +101,58 @@ pub struct SequenceDiff { pub new: Sequence, } +/// A detected column rename. +#[derive(Debug, Clone, Serialize)] +pub struct ColumnRename { + pub old: Column, + pub new: Column, + pub confidence: RenameConfidence, +} + +/// A detected table rename. +#[derive(Debug, Clone, Serialize)] +pub struct TableRename { + pub old_name: String, + pub new_name: String, + pub confidence: RenameConfidence, +} + +/// Confidence level for rename detection. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize)] +pub enum RenameConfidence { + /// type + nullability + default all match + High, + /// type + nullability match, default differs + Medium, +} + +impl std::fmt::Display for RenameConfidence { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::High => write!(f, "high"), + Self::Medium => write!(f, "medium"), + } + } +} + /// Compare two schemas and produce a diff. /// /// `left` is the current state (e.g. production). /// `right` is the desired state (e.g. staging or .sql file). /// The generated migration will bring `left` to match `right`. pub fn diff_schemas(left: &Schema, right: &Schema) -> SchemaDiff { + diff_schemas_with_options(left, right, false) +} + +/// Compare two schemas with rename detection option. +pub fn diff_schemas_with_options( + left: &Schema, + right: &Schema, + detect_renames: bool, +) -> SchemaDiff { let mut added_tables = Vec::new(); let mut removed_tables = Vec::new(); + let mut renamed_tables = Vec::new(); let mut modified_tables = Vec::new(); let mut unchanged_tables = Vec::new(); @@ -122,10 +170,19 @@ pub fn diff_schemas(left: &Schema, right: &Schema) -> SchemaDiff { } } + // Detect table renames: match removed -> added by identical column sets + if detect_renames { + detect_table_renames( + &mut removed_tables, + &mut added_tables, + &mut renamed_tables, + ); + } + // Tables in both -> compare for (name, left_table) in &left.tables { if let Some(right_table) = right.tables.get(name) { - let table_diff = diff_tables(name, left_table, right_table); + let table_diff = diff_tables(name, left_table, right_table, detect_renames); if table_diff.is_empty() { unchanged_tables.push(name.clone()); } else { @@ -147,6 +204,7 @@ pub fn diff_schemas(left: &Schema, right: &Schema) -> SchemaDiff { SchemaDiff { added_tables, removed_tables, + renamed_tables, modified_tables, unchanged_tables, added_views, @@ -161,9 +219,10 @@ pub fn diff_schemas(left: &Schema, right: &Schema) -> SchemaDiff { } } -fn diff_tables(name: &str, left: &Table, right: &Table) -> TableDiff { +fn diff_tables(name: &str, left: &Table, right: &Table, detect_renames: bool) -> TableDiff { let mut added_columns = Vec::new(); let mut removed_columns = Vec::new(); + let mut renamed_columns = Vec::new(); let mut modified_columns = Vec::new(); let mut unchanged_columns = Vec::new(); @@ -179,6 +238,11 @@ fn diff_tables(name: &str, left: &Table, right: &Table) -> TableDiff { } } + // Detect column renames before finalizing added/removed lists + if detect_renames { + detect_column_renames(&mut removed_columns, &mut added_columns, &mut renamed_columns); + } + for (col_name, left_col) in &left.columns { if let Some(right_col) = right.columns.get(col_name) { if left_col != right_col { @@ -234,6 +298,7 @@ fn diff_tables(name: &str, left: &Table, right: &Table) -> TableDiff { table_name: name.to_string(), added_columns, removed_columns, + renamed_columns, modified_columns, unchanged_columns, added_indexes, @@ -243,6 +308,116 @@ fn diff_tables(name: &str, left: &Table, right: &Table) -> TableDiff { } } +/// Detect column renames by matching removed columns to added columns with compatible types. +/// Only produces a rename when exactly one removed column matches one added column (1:1). +fn detect_column_renames( + removed: &mut Vec, + added: &mut Vec, + renames: &mut Vec, +) { + let mut matched_removed = Vec::new(); + let mut matched_added = Vec::new(); + + for (ri, rem) in removed.iter().enumerate() { + // Find added columns with matching type + nullability + let candidates: Vec<(usize, RenameConfidence)> = added + .iter() + .enumerate() + .filter(|(ai, _)| !matched_added.contains(ai)) + .filter_map(|(ai, add)| { + if rem.data_type == add.data_type && rem.is_nullable == add.is_nullable { + let confidence = if rem.default == add.default { + RenameConfidence::High + } else { + RenameConfidence::Medium + }; + Some((ai, confidence)) + } else { + None + } + }) + .collect(); + + // Only match if exactly one candidate (unambiguous) + if candidates.len() == 1 { + let (ai, confidence) = &candidates[0]; + matched_removed.push(ri); + matched_added.push(*ai); + renames.push(ColumnRename { + old: rem.clone(), + new: added[*ai].clone(), + confidence: *confidence, + }); + } + } + + // Remove matched entries (in reverse order to preserve indices) + matched_removed.sort_unstable(); + matched_added.sort_unstable(); + for &i in matched_removed.iter().rev() { + removed.remove(i); + } + for &i in matched_added.iter().rev() { + added.remove(i); + } +} + +/// Detect table renames by matching removed tables to added tables with identical column sets. +fn detect_table_renames( + removed: &mut Vec
, + added: &mut Vec
, + renames: &mut Vec, +) { + let mut matched_removed = Vec::new(); + let mut matched_added = Vec::new(); + + for (ri, rem) in removed.iter().enumerate() { + let candidates: Vec<(usize, RenameConfidence)> = added + .iter() + .enumerate() + .filter(|(ai, _)| !matched_added.contains(ai)) + .filter_map(|(ai, add)| { + // High confidence: identical column definitions + if rem.columns == add.columns { + return Some((ai, RenameConfidence::High)); + } + // Medium confidence: same column count, ≥80% column name overlap + if rem.columns.len() == add.columns.len() && !rem.columns.is_empty() { + let overlap = rem + .columns + .keys() + .filter(|k| add.columns.contains_key(*k)) + .count(); + if overlap * 100 / rem.columns.len() >= 80 { + return Some((ai, RenameConfidence::Medium)); + } + } + None + }) + .collect(); + + if candidates.len() == 1 { + let (ai, confidence) = &candidates[0]; + matched_removed.push(ri); + matched_added.push(*ai); + renames.push(TableRename { + old_name: rem.name.clone(), + new_name: added[*ai].name.clone(), + confidence: *confidence, + }); + } + } + + matched_removed.sort_unstable(); + matched_added.sort_unstable(); + for &i in matched_removed.iter().rev() { + removed.remove(i); + } + for &i in matched_added.iter().rev() { + added.remove(i); + } +} + fn diff_views( left: &std::collections::BTreeMap, right: &std::collections::BTreeMap, @@ -765,4 +940,287 @@ mod tests { assert!(diff.modified_enums[0].added_values.is_empty()); assert!(diff.modified_enums[0].removed_values.is_empty()); } + + // ── Primary Key tests ── + + #[test] + fn pk_constraint_added_detected() { + let mut left_table = Table::new("users"); + left_table + .columns + .insert("id".into(), make_column("id", "integer", false, None)); + + let mut right_table = Table::new("users"); + right_table + .columns + .insert("id".into(), make_column("id", "integer", false, None)); + right_table.constraints.insert( + "users_pkey".into(), + Constraint { + name: "users_pkey".into(), + table_name: "users".into(), + kind: ConstraintKind::PrimaryKey { + columns: vec!["id".into()], + }, + }, + ); + + let mut left = Schema::new(); + left.tables.insert("users".into(), left_table); + let mut right = Schema::new(); + right.tables.insert("users".into(), right_table); + + let diff = diff_schemas(&left, &right); + assert_eq!(diff.modified_tables.len(), 1); + assert_eq!(diff.modified_tables[0].added_constraints.len(), 1); + assert!(matches!( + diff.modified_tables[0].added_constraints[0].kind, + ConstraintKind::PrimaryKey { .. } + )); + } + + #[test] + fn pk_constraint_changed_detected() { + let mut left_table = Table::new("orders"); + left_table + .columns + .insert("id".into(), make_column("id", "integer", false, None)); + left_table.columns.insert( + "tenant_id".into(), + make_column("tenant_id", "integer", false, None), + ); + left_table.constraints.insert( + "orders_pkey".into(), + Constraint { + name: "orders_pkey".into(), + table_name: "orders".into(), + kind: ConstraintKind::PrimaryKey { + columns: vec!["id".into()], + }, + }, + ); + + let mut right_table = Table::new("orders"); + right_table + .columns + .insert("id".into(), make_column("id", "integer", false, None)); + right_table.columns.insert( + "tenant_id".into(), + make_column("tenant_id", "integer", false, None), + ); + right_table.constraints.insert( + "orders_pkey".into(), + Constraint { + name: "orders_pkey".into(), + table_name: "orders".into(), + kind: ConstraintKind::PrimaryKey { + columns: vec!["tenant_id".into(), "id".into()], + }, + }, + ); + + let mut left = Schema::new(); + left.tables.insert("orders".into(), left_table); + let mut right = Schema::new(); + right.tables.insert("orders".into(), right_table); + + let diff = diff_schemas(&left, &right); + assert_eq!(diff.modified_tables.len(), 1); + // Changed PK = removed old + added new + assert_eq!(diff.modified_tables[0].removed_constraints.len(), 1); + assert_eq!(diff.modified_tables[0].added_constraints.len(), 1); + } + + // ── Rename detection tests ── + + #[test] + fn rename_detection_disabled_by_default() { + let mut left = Schema::new(); + let mut t = Table::new("users"); + t.columns + .insert("old_name".into(), make_column("old_name", "text", true, None)); + left.tables.insert("users".into(), t); + + let mut right = Schema::new(); + let mut t = Table::new("users"); + t.columns + .insert("new_name".into(), make_column("new_name", "text", true, None)); + right.tables.insert("users".into(), t); + + let diff = diff_schemas(&left, &right); + assert!(diff.modified_tables[0].renamed_columns.is_empty()); + assert_eq!(diff.modified_tables[0].removed_columns.len(), 1); + assert_eq!(diff.modified_tables[0].added_columns.len(), 1); + } + + #[test] + fn column_rename_high_confidence() { + let mut left = Schema::new(); + let mut t = Table::new("users"); + t.columns + .insert("id".into(), make_column("id", "integer", false, None)); + t.columns + .insert("email_addr".into(), make_column("email_addr", "varchar(255)", false, None)); + left.tables.insert("users".into(), t); + + let mut right = Schema::new(); + let mut t = Table::new("users"); + t.columns + .insert("id".into(), make_column("id", "integer", false, None)); + t.columns + .insert("email".into(), make_column("email", "varchar(255)", false, None)); + right.tables.insert("users".into(), t); + + let diff = diff_schemas_with_options(&left, &right, true); + assert_eq!(diff.modified_tables[0].renamed_columns.len(), 1); + assert_eq!(diff.modified_tables[0].renamed_columns[0].old.name, "email_addr"); + assert_eq!(diff.modified_tables[0].renamed_columns[0].new.name, "email"); + assert_eq!(diff.modified_tables[0].renamed_columns[0].confidence, RenameConfidence::High); + assert!(diff.modified_tables[0].added_columns.is_empty()); + assert!(diff.modified_tables[0].removed_columns.is_empty()); + } + + #[test] + fn column_rename_medium_confidence_when_default_differs() { + let mut left = Schema::new(); + let mut t = Table::new("users"); + t.columns.insert( + "is_enabled".into(), + make_column("is_enabled", "bool", false, Some("true")), + ); + left.tables.insert("users".into(), t); + + let mut right = Schema::new(); + let mut t = Table::new("users"); + t.columns.insert( + "is_active".into(), + make_column("is_active", "bool", false, Some("false")), + ); + right.tables.insert("users".into(), t); + + let diff = diff_schemas_with_options(&left, &right, true); + assert_eq!(diff.modified_tables[0].renamed_columns.len(), 1); + assert_eq!( + diff.modified_tables[0].renamed_columns[0].confidence, + RenameConfidence::Medium + ); + } + + #[test] + fn column_rename_not_detected_when_types_differ() { + let mut left = Schema::new(); + let mut t = Table::new("users"); + t.columns + .insert("old_col".into(), make_column("old_col", "integer", false, None)); + left.tables.insert("users".into(), t); + + let mut right = Schema::new(); + let mut t = Table::new("users"); + t.columns + .insert("new_col".into(), make_column("new_col", "text", false, None)); + right.tables.insert("users".into(), t); + + let diff = diff_schemas_with_options(&left, &right, true); + assert!(diff.modified_tables[0].renamed_columns.is_empty()); + assert_eq!(diff.modified_tables[0].removed_columns.len(), 1); + assert_eq!(diff.modified_tables[0].added_columns.len(), 1); + } + + #[test] + fn column_rename_ambiguous_two_candidates() { + let mut left = Schema::new(); + let mut t = Table::new("users"); + t.columns + .insert("a".into(), make_column("a", "text", true, None)); + t.columns + .insert("b".into(), make_column("b", "text", true, None)); + left.tables.insert("users".into(), t); + + let mut right = Schema::new(); + let mut t = Table::new("users"); + t.columns + .insert("x".into(), make_column("x", "text", true, None)); + t.columns + .insert("y".into(), make_column("y", "text", true, None)); + right.tables.insert("users".into(), t); + + let diff = diff_schemas_with_options(&left, &right, true); + // Ambiguous: a could match x or y, b could match x or y + // No rename should be detected + assert!(diff.modified_tables[0].renamed_columns.is_empty()); + assert_eq!(diff.modified_tables[0].removed_columns.len(), 2); + assert_eq!(diff.modified_tables[0].added_columns.len(), 2); + } + + #[test] + fn table_rename_high_confidence_identical_columns() { + let mut left = Schema::new(); + let mut t = Table::new("user_logs"); + t.columns + .insert("id".into(), make_column("id", "integer", false, None)); + t.columns + .insert("message".into(), make_column("message", "text", true, None)); + left.tables.insert("user_logs".into(), t); + + let mut right = Schema::new(); + let mut t = Table::new("audit_trail"); + t.columns + .insert("id".into(), make_column("id", "integer", false, None)); + t.columns + .insert("message".into(), make_column("message", "text", true, None)); + right.tables.insert("audit_trail".into(), t); + + let diff = diff_schemas_with_options(&left, &right, true); + assert_eq!(diff.renamed_tables.len(), 1); + assert_eq!(diff.renamed_tables[0].old_name, "user_logs"); + assert_eq!(diff.renamed_tables[0].new_name, "audit_trail"); + assert_eq!(diff.renamed_tables[0].confidence, RenameConfidence::High); + assert!(diff.added_tables.is_empty()); + assert!(diff.removed_tables.is_empty()); + } + + #[test] + fn table_rename_not_detected_when_columns_differ() { + let mut left = Schema::new(); + let mut t = Table::new("old_table"); + t.columns + .insert("id".into(), make_column("id", "integer", false, None)); + left.tables.insert("old_table".into(), t); + + let mut right = Schema::new(); + let mut t = Table::new("new_table"); + t.columns + .insert("uuid".into(), make_column("uuid", "uuid", false, None)); + right.tables.insert("new_table".into(), t); + + let diff = diff_schemas_with_options(&left, &right, true); + assert!(diff.renamed_tables.is_empty()); + assert_eq!(diff.removed_tables.len(), 1); + assert_eq!(diff.added_tables.len(), 1); + } + + #[test] + fn table_rename_ambiguous_two_candidates() { + let mut left = Schema::new(); + for name in ["table_a", "table_b"] { + let mut t = Table::new(name); + t.columns + .insert("id".into(), make_column("id", "integer", false, None)); + left.tables.insert(name.into(), t); + } + + let mut right = Schema::new(); + for name in ["table_x", "table_y"] { + let mut t = Table::new(name); + t.columns + .insert("id".into(), make_column("id", "integer", false, None)); + right.tables.insert(name.into(), t); + } + + let diff = diff_schemas_with_options(&left, &right, true); + // Both removed could match both added → ambiguous → no rename + assert!(diff.renamed_tables.is_empty()); + assert_eq!(diff.removed_tables.len(), 2); + assert_eq!(diff.added_tables.len(), 2); + } } diff --git a/src/loader/mysql.rs b/src/loader/mysql.rs index abfd753..a1e946d 100644 --- a/src/loader/mysql.rs +++ b/src/loader/mysql.rs @@ -90,6 +90,43 @@ pub async fn load(dsn: &str) -> Result { } } + // Load primary key constraints + let pk_rows: Vec<(String, String)> = conn + .exec( + "SELECT kcu.TABLE_NAME, kcu.COLUMN_NAME \ + FROM information_schema.TABLE_CONSTRAINTS tc \ + JOIN information_schema.KEY_COLUMN_USAGE kcu \ + ON tc.CONSTRAINT_NAME = kcu.CONSTRAINT_NAME \ + AND tc.TABLE_SCHEMA = kcu.TABLE_SCHEMA \ + AND tc.TABLE_NAME = kcu.TABLE_NAME \ + WHERE tc.TABLE_SCHEMA = :db AND tc.CONSTRAINT_TYPE = 'PRIMARY KEY' \ + ORDER BY kcu.TABLE_NAME, kcu.ORDINAL_POSITION", + mysql_async::params! { "db" => &db_name }, + ) + .await?; + + let mut pk_map: BTreeMap> = BTreeMap::new(); + for (table_name, column_name) in &pk_rows { + pk_map + .entry(table_name.clone()) + .or_default() + .push(column_name.clone()); + } + + for (table_name, columns) in pk_map { + if let Some(table) = schema.tables.get_mut(&table_name) { + let pk_name = format!("{}_pkey", table_name); + table.constraints.insert( + pk_name.clone(), + Constraint { + name: pk_name, + table_name: table_name.clone(), + kind: ConstraintKind::PrimaryKey { columns }, + }, + ); + } + } + // Load foreign key constraints let fk_rows: Vec<(String, String, String, String, String, String, String)> = conn .exec( diff --git a/src/loader/postgres.rs b/src/loader/postgres.rs index f523e17..3ea564c 100644 --- a/src/loader/postgres.rs +++ b/src/loader/postgres.rs @@ -166,7 +166,7 @@ async fn load_from_client(client: &Client) -> Result { } } - // Load FK and UNIQUE constraints using pg_constraint for correct composite FK mapping. + // Load PK, FK, and UNIQUE constraints using pg_constraint. // pg_constraint.conkey/confkey arrays preserve positional column correspondence. let rows = client .query( @@ -189,7 +189,7 @@ async fn load_from_client(client: &Client) -> Result { JOIN pg_class rel ON con.conrelid = rel.oid \ JOIN pg_namespace nsp ON rel.relnamespace = nsp.oid \ LEFT JOIN pg_class frel ON con.confrelid = frel.oid \ - WHERE nsp.nspname = 'public' AND con.contype IN ('f', 'u') \ + WHERE nsp.nspname = 'public' AND con.contype IN ('p', 'f', 'u') \ ORDER BY rel.relname, con.conname", &[], ) @@ -201,7 +201,18 @@ async fn load_from_client(client: &Client) -> Result { let contype: String = row.get("contype"); let columns: Vec = row.get("columns"); - if contype == "f" { + if contype == "p" { + if let Some(table) = schema.tables.get_mut(&table_name) { + table.constraints.insert( + constraint_name.clone(), + Constraint { + name: constraint_name, + table_name: table_name.clone(), + kind: ConstraintKind::PrimaryKey { columns }, + }, + ); + } + } else if contype == "f" { let ref_table: String = row.get("ref_table"); let ref_columns: Vec = row.get("ref_columns"); let del_type: String = row.get("confdeltype"); @@ -336,7 +347,7 @@ async fn load_from_client(client: &Client) -> Result { // Load sequences let rows = client .query( - "SELECT sequencename, data_type, start_value, increment_by, min_value, max_value \ + "SELECT sequencename, data_type::text, start_value, increment_by, min_value, max_value \ FROM pg_sequences \ WHERE schemaname = 'public' \ ORDER BY sequencename", @@ -346,20 +357,27 @@ async fn load_from_client(client: &Client) -> Result { for row in &rows { let name: String = row.get("sequencename"); - let data_type: String = row.get("data_type"); - let start_value: i64 = row.get("start_value"); - let increment: i64 = row.get("increment_by"); - let min_value: i64 = row.get("min_value"); - let max_value: i64 = row.get("max_value"); + let data_type: Option = row.get("data_type"); + let start_value: Option = row.get("start_value"); + let increment: Option = row.get("increment_by"); + let min_value: Option = row.get("min_value"); + let max_value: Option = row.get("max_value"); + if data_type.is_none() || start_value.is_none() { + eprintln!( + "Warning: sequence '{}' returned NULL metadata — the connected role \ + may lack USAGE privilege on this sequence. Falling back to defaults.", + name + ); + } schema.sequences.insert( name.clone(), Sequence { name, - data_type, - start_value, - increment, - min_value, - max_value, + data_type: data_type.unwrap_or_else(|| "bigint".to_string()), + start_value: start_value.unwrap_or(1), + increment: increment.unwrap_or(1), + min_value: min_value.unwrap_or(1), + max_value: max_value.unwrap_or(i64::MAX), }, ); } diff --git a/src/loader/sqlfile.rs b/src/loader/sqlfile.rs index 971c105..d232a04 100644 --- a/src/loader/sqlfile.rs +++ b/src/loader/sqlfile.rs @@ -225,7 +225,22 @@ fn parse_constraint(def: &str, table_name: &str) -> Option { }); } - // PRIMARY KEY — skip (we don't diff PKs) + // PRIMARY KEY (col1, col2) + let pk_re = + Regex::new(r"(?i)(?:CONSTRAINT\s+(\w+)\s+)?PRIMARY\s+KEY\s*\(([^)]+)\)").ok()?; + if let Some(cap) = pk_re.captures(def) { + let columns: Vec = cap[2].split(',').map(|s| s.trim().to_string()).collect(); + let name = cap + .get(1) + .map(|m| m.as_str().to_string()) + .unwrap_or_else(|| format!("{}_pkey", table_name)); + return Some(Constraint { + name, + table_name: table_name.to_string(), + kind: ConstraintKind::PrimaryKey { columns }, + }); + } + None } diff --git a/src/loader/sqlite.rs b/src/loader/sqlite.rs index 260baf4..e642d9e 100644 --- a/src/loader/sqlite.rs +++ b/src/loader/sqlite.rs @@ -59,25 +59,49 @@ fn load_columns(conn: &Connection, table_name: &str, table: &mut Table) -> Resul quote_identifier(table_name) ))?; + let mut pk_columns: Vec<(i32, String)> = Vec::new(); + let columns = stmt.query_map([], |row| { let name: String = row.get(1)?; let data_type: String = row.get(2)?; let notnull: bool = row.get(3)?; let dflt_value: Option = row.get(4)?; + let pk: i32 = row.get(5)?; - Ok(Column { - name, - data_type: normalize_type(&data_type), - is_nullable: !notnull, - default: dflt_value.map(|d| normalize_default(&d)), - }) + Ok(( + Column { + name, + data_type: normalize_type(&data_type), + is_nullable: !notnull, + default: dflt_value.map(|d| normalize_default(&d)), + }, + pk, + )) })?; - for col in columns { - let col = col?; + for result in columns { + let (col, pk) = result?; + if pk > 0 { + pk_columns.push((pk, col.name.clone())); + } table.columns.insert(col.name.clone(), col); } + // Build PK constraint from columns with pk > 0 + if !pk_columns.is_empty() { + pk_columns.sort_by_key(|(pk_ord, _)| *pk_ord); + let cols: Vec = pk_columns.into_iter().map(|(_, name)| name).collect(); + let pk_name = format!("{}_pkey", table_name); + table.constraints.insert( + pk_name.clone(), + Constraint { + name: pk_name, + table_name: table_name.to_string(), + kind: ConstraintKind::PrimaryKey { columns: cols }, + }, + ); + } + Ok(()) } @@ -116,7 +140,9 @@ fn load_indexes(conn: &Connection, table_name: &str, table: &mut Table) -> Resul ))?; let columns: Vec = col_stmt .query_map([], |row| row.get::<_, Option>(2))? - .filter_map(|r| r.ok().flatten()) + .collect::, _>>()? + .into_iter() + .flatten() .collect(); if columns.is_empty() { @@ -161,8 +187,7 @@ fn load_foreign_keys( row.get::<_, String>(6)?, // on_delete )) })? - .filter_map(|r| r.ok()) - .collect(); + .collect::, _>>()?; // Group by FK id: (ref_table, columns, ref_columns, on_update, on_delete) type FkGroup = (String, Vec, Vec, String, String); @@ -224,8 +249,7 @@ fn load_views(conn: &Connection, schema: &mut Schema) -> Result<(), DbDiffError> let views: Vec<(String, String)> = stmt .query_map([], |row| Ok((row.get(0)?, row.get(1)?)))? - .filter_map(|r| r.ok()) - .collect(); + .collect::, _>>()?; let view_as_re = regex::Regex::new(r"(?i)\bAS\s+").unwrap(); for (name, sql) in views { diff --git a/src/main.rs b/src/main.rs index 9fcbb6a..dabfec6 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,20 +1,10 @@ +mod commands; + use std::process::ExitCode; -use std::time::Instant; use clap::Parser; -use indicatif::{ProgressBar, ProgressStyle}; -use dbdiff::ci::{self, CiReport}; -use dbdiff::cli::{ - Cli, ColorMode, DiffParams, MigrationDirection, OutputFormat, ResolvedCommand, SnapshotArgs, - SslMode, TablesArgs, ValidateArgs, -}; -use dbdiff::config; -use dbdiff::diff::diff_schemas; -use dbdiff::loader; -use dbdiff::loader::SqlDialect; -use dbdiff::migration; -use dbdiff::output; +use dbdiff::cli::{Cli, ResolvedCommand}; #[tokio::main] async fn main() -> ExitCode { @@ -22,15 +12,15 @@ async fn main() -> ExitCode { let resolved = cli.resolve(); let result = match resolved { - ResolvedCommand::Diff(params) => run_diff(params).await, - ResolvedCommand::Validate(args) => run_validate(args).await, - ResolvedCommand::Tables(args) => run_tables(args).await, + ResolvedCommand::Diff(params) => commands::run_diff(params).await, + ResolvedCommand::Validate(args) => commands::run_validate(args).await, + ResolvedCommand::Tables(args) => commands::run_tables(args).await, ResolvedCommand::Completions(args) => { Cli::generate_completions(args.shell); Ok(()) } - ResolvedCommand::Init => run_init(), - ResolvedCommand::Snapshot(args) => run_snapshot(args).await, + ResolvedCommand::Init => commands::run_init(), + ResolvedCommand::Snapshot(args) => commands::run_snapshot(args).await, }; match result { @@ -38,501 +28,3 @@ async fn main() -> ExitCode { Err(code) => code, } } - -async fn run_diff(params: DiffParams) -> Result<(), ExitCode> { - // Apply color mode - apply_color_mode(params.color); - - if params.target_source.is_empty() { - eprintln!("Error: Either a target DSN or --schema is required"); - return Err(ExitCode::from(2)); - } - - let cfg = config::load_config(¶ms.config).map_err(|e| { - eprintln!("Error: {e}"); - ExitCode::from(2) - })?; - - let spinner = create_spinner("Loading schemas..."); - - let timeout_duration = std::time::Duration::from_secs(params.timeout); - let ssl_mode = resolve_ssl_mode(params.ssl_mode); - - let load_result = tokio::time::timeout(timeout_duration, async { - tokio::try_join!( - loader::load_schema_with_ssl(¶ms.source, ssl_mode), - loader::load_schema_with_ssl(¶ms.target_source, ssl_mode), - ) - }) - .await; - - spinner.finish_and_clear(); - - let (mut left, mut right) = match load_result { - Ok(Ok(schemas)) => schemas, - Ok(Err(e)) => { - eprintln!("Error: {e}"); - return Err(ExitCode::from(2)); - } - Err(_) => { - let safe_source = dbdiff::error::sanitize_dsn(¶ms.source); - let safe_target = dbdiff::error::sanitize_dsn(¶ms.target_source); - let err = dbdiff::error::DbDiffError::timeout( - &format!("{safe_source} / {safe_target}"), - params.timeout, - ); - eprintln!("Error: {err}"); - return Err(ExitCode::from(2)); - } - }; - - config::filter::apply_ignore(&mut left.schema, &cfg.ignore); - config::filter::apply_ignore(&mut right.schema, &cfg.ignore); - - // When comparing against SQL files (not JSON snapshots), clear object types - // that the SQL parser cannot load (views, enums, sequences) to avoid false diffs. - // JSON snapshots (SqlDialect::Snapshot) DO carry these objects, so don't clear them. - if left.dialect == SqlDialect::SqlFile { - right.schema.views.clear(); - right.schema.enums.clear(); - right.schema.sequences.clear(); - } - if right.dialect == SqlDialect::SqlFile { - left.schema.views.clear(); - left.schema.enums.clear(); - left.schema.sequences.clear(); - } - - if let Some(false) = cfg.output.color { - colored::control::set_override(false); - } - - let diff = diff_schemas(&left.schema, &right.schema); - - // Check protected objects based on migration direction - if let Err(msg) = check_protected(&diff, &cfg.protected, params.direction) { - eprintln!("Error: {msg}"); - return Err(ExitCode::from(2)); - } - - let migration_dialect = match (left.dialect, right.dialect) { - // Both non-DB sources: default to PostgreSQL (most standards-compliant SQL) - ( - SqlDialect::SqlFile | SqlDialect::Snapshot, - SqlDialect::SqlFile | SqlDialect::Snapshot, - ) => SqlDialect::Postgres, - // One side is a file/snapshot: use the other side's dialect - (SqlDialect::SqlFile | SqlDialect::Snapshot, other) - | (other, SqlDialect::SqlFile | SqlDialect::Snapshot) => other, - // Both live DBs: must be same dialect - (l, r) if l == r => l, - (l, r) => { - eprintln!( - "Error: Cannot generate migration for mixed backends ({l:?} vs {r:?}). \ - Compare like-for-like backends or use a .sql file for one side." - ); - return Err(ExitCode::from(2)); - } - }; - - let up_statements = migration::generate_migration(&diff, migration_dialect); - let down_statements = migration::generate_rollback(&diff, migration_dialect); - let format = params.resolve_format(&cfg.output.format); - - // Build CI report from the statements matching the selected direction - let report = match params.direction { - MigrationDirection::Down => CiReport::from_diff_reversed(&diff, &down_statements), - _ => CiReport::from_diff(&diff, &up_statements), - }; - - match format { - OutputFormat::Pretty => { - output::print_diff(&diff); - - let print_fn = if params.explain { - output::print_migration_explained - } else { - output::print_migration - }; - - match params.direction { - MigrationDirection::Up => { - if !up_statements.is_empty() { - println!(); - print_fn(&up_statements); - } - } - MigrationDirection::Down => { - if !down_statements.is_empty() { - println!(); - println!( - "{}", - colored::Colorize::bold(colored::Colorize::underline( - "Rollback migration" - )) - ); - println!(); - print_fn(&down_statements); - } - } - MigrationDirection::Both => { - if !up_statements.is_empty() { - println!(); - output::print_migration(&up_statements); - } - if !down_statements.is_empty() { - println!(); - println!( - "{}", - colored::Colorize::bold(colored::Colorize::underline( - "Rollback migration" - )) - ); - println!(); - print_fn(&down_statements); - } - } - } - - println!(); - output::print_summary(&diff); - } - OutputFormat::Json => { - let json = serde_json::to_string_pretty(&report).unwrap_or_default(); - println!("{json}"); - } - OutputFormat::Yaml => { - let yaml = serde_yaml::to_string(&report).unwrap_or_default(); - print!("{yaml}"); - } - OutputFormat::Sql => match params.direction { - MigrationDirection::Up => { - print!("{}", output::migration_to_sql(&up_statements)); - } - MigrationDirection::Down => { - print!("{}", output::migration_to_sql(&down_statements)); - } - MigrationDirection::Both => { - println!("-- === UP ==="); - print!("{}", output::migration_to_sql(&up_statements)); - println!("-- === DOWN ==="); - print!("{}", output::migration_to_sql(&down_statements)); - } - }, - } - - // Write migration file - if let Some(ref path) = params.out { - if params.should_write { - let sql = match params.direction { - MigrationDirection::Up => output::migration_to_sql(&up_statements), - MigrationDirection::Down => output::migration_to_sql(&down_statements), - MigrationDirection::Both => { - format!( - "-- === UP ===\n{}\n-- === DOWN ===\n{}", - output::migration_to_sql(&up_statements), - output::migration_to_sql(&down_statements) - ) - } - }; - std::fs::write(path, sql).map_err(|e| { - eprintln!("Error writing migration file: {e}"); - ExitCode::from(2) - })?; - eprintln!("Migration written to {path}"); - } else { - eprintln!("Dry run: would write migration to {path} (use --write to save)"); - } - } - - // GitHub Actions annotations (auto-detect from env) - if params.ci && std::env::var("GITHUB_ACTIONS").as_deref() == Ok("true") { - report.emit_github_annotations(); - } - - // CI mode: determine exit code from report - if params.ci { - let code = report.exit_code(params.fail_on_blocking); - if code != ci::EXIT_OK { - return Err(ExitCode::from(code)); - } - } - - Ok(()) -} - -fn check_protected( - diff: &dbdiff::diff::SchemaDiff, - protected: &config::ProtectedConfig, - direction: MigrationDirection, -) -> Result<(), String> { - // In UP direction, removed_tables/removed_columns are dropped. - // In DOWN direction, added_tables/added_columns are dropped (rollback). - // In BOTH, check both directions. - let check_up = !matches!(direction, MigrationDirection::Down); - let check_down = !matches!(direction, MigrationDirection::Up); - - // Check tables that would be dropped - let dropped_tables: Vec<&dbdiff::model::Table> = std::iter::empty() - .chain(if check_up { - diff.removed_tables.iter().collect::>() - } else { - vec![] - }) - .chain(if check_down { - diff.added_tables.iter().collect::>() - } else { - vec![] - }) - .collect(); - - for table in &dropped_tables { - if protected.tables.contains(&table.name) { - return Err(format!( - "Protected table '{}' would be dropped. Remove it from the protected list to allow this.", - table.name - )); - } - for col in table.columns.values() { - if column_is_protected(&table.name, &col.name, &protected.columns) { - return Err(format!( - "Protected column '{}.{}' would be dropped (table '{}' is being removed). \ - Remove it from the protected list to allow this.", - table.name, col.name, table.name - )); - } - } - } - - // Check columns that would be dropped from modified tables - for table_diff in &diff.modified_tables { - let dropped_cols: Vec<&dbdiff::model::Column> = std::iter::empty() - .chain(if check_up { - table_diff.removed_columns.iter().collect::>() - } else { - vec![] - }) - .chain(if check_down { - table_diff.added_columns.iter().collect::>() - } else { - vec![] - }) - .collect(); - - for col in &dropped_cols { - if column_is_protected(&table_diff.table_name, &col.name, &protected.columns) { - return Err(format!( - "Protected column '{}.{}' would be dropped. Remove it from the protected list to allow this.", - table_diff.table_name, col.name - )); - } - } - } - - Ok(()) -} - -fn column_is_protected(table_name: &str, col_name: &str, patterns: &[String]) -> bool { - patterns.iter().any(|pattern| { - if let Some(c) = pattern.strip_prefix("*.") { - // *.column_name — matches column in any table - col_name == c - } else if let Some((t, c)) = pattern.split_once('.') { - if c == "*" { - // table.* — all columns in this table are protected - table_name == t - } else { - // table.column — exact match - table_name == t && col_name == c - } - } else { - false - } - }) -} - -async fn run_validate(args: ValidateArgs) -> Result<(), ExitCode> { - let spinner = create_spinner("Connecting..."); - let start = Instant::now(); - - let timeout_duration = std::time::Duration::from_secs(args.timeout); - let ssl_mode = resolve_ssl_mode(args.ssl_mode); - - let result = tokio::time::timeout( - timeout_duration, - loader::load_schema_with_ssl(&args.dsn, ssl_mode), - ) - .await; - - let latency = start.elapsed(); - spinner.finish_and_clear(); - - match result { - Ok(Ok(loaded)) => { - let table_count = loaded.schema.tables.len(); - let column_count: usize = loaded.schema.tables.values().map(|t| t.columns.len()).sum(); - let index_count: usize = loaded.schema.tables.values().map(|t| t.indexes.len()).sum(); - - use colored::Colorize; - println!( - "{}", - format!(" Connected to {:?} database", loaded.dialect).green() - ); - println!(" Tables: {table_count}"); - println!(" Columns: {column_count}"); - println!(" Indexes: {index_count}"); - println!(" Latency: {}ms", latency.as_millis()); - Ok(()) - } - Ok(Err(e)) => { - eprintln!("Error: {e}"); - Err(ExitCode::from(2)) - } - Err(_) => { - let err = dbdiff::error::DbDiffError::timeout(&args.dsn, args.timeout); - eprintln!("Error: {err}"); - Err(ExitCode::from(2)) - } - } -} - -async fn run_tables(args: TablesArgs) -> Result<(), ExitCode> { - let spinner = create_spinner("Loading tables..."); - let ssl_mode = resolve_ssl_mode(args.ssl_mode); - let timeout_duration = std::time::Duration::from_secs(args.timeout); - - let result = tokio::time::timeout( - timeout_duration, - loader::load_schema_with_ssl(&args.dsn, ssl_mode), - ) - .await; - - spinner.finish_and_clear(); - - match result { - Ok(Ok(loaded)) => { - if loaded.schema.tables.is_empty() { - println!("No tables found."); - return Ok(()); - } - - println!("{:<30} {:>8} {:>8}", "TABLE", "COLUMNS", "INDEXES"); - println!("{}", "-".repeat(48)); - - for (name, table) in &loaded.schema.tables { - println!( - "{:<30} {:>8} {:>8}", - name, - table.columns.len(), - table.indexes.len() - ); - } - - println!("{}", "-".repeat(48)); - println!("{} table(s) total", loaded.schema.tables.len()); - Ok(()) - } - Ok(Err(e)) => { - eprintln!("Error: {e}"); - Err(ExitCode::from(2)) - } - Err(_) => { - let err = dbdiff::error::DbDiffError::timeout(&args.dsn, args.timeout); - eprintln!("Error: {err}"); - Err(ExitCode::from(2)) - } - } -} - -async fn run_snapshot(args: SnapshotArgs) -> Result<(), ExitCode> { - let spinner = create_spinner("Loading schema..."); - let ssl_mode = resolve_ssl_mode(args.ssl_mode); - let timeout_duration = std::time::Duration::from_secs(args.timeout); - - let result = tokio::time::timeout( - timeout_duration, - loader::load_schema_with_ssl(&args.dsn, ssl_mode), - ) - .await; - - spinner.finish_and_clear(); - - match result { - Ok(Ok(loaded)) => { - let snapshot = dbdiff::model::SchemaSnapshot::from(&loaded.schema); - let json = serde_json::to_string_pretty(&snapshot).unwrap_or_default(); - - if let Some(ref path) = args.out { - std::fs::write(path, &json).map_err(|e| { - eprintln!("Error writing snapshot: {e}"); - ExitCode::from(2) - })?; - eprintln!("Snapshot written to {path}"); - } else { - println!("{json}"); - } - Ok(()) - } - Ok(Err(e)) => { - eprintln!("Error: {e}"); - Err(ExitCode::from(2)) - } - Err(_) => { - let err = dbdiff::error::DbDiffError::timeout(&args.dsn, args.timeout); - eprintln!("Error: {err}"); - Err(ExitCode::from(2)) - } - } -} - -fn run_init() -> Result<(), ExitCode> { - let path = ".dbdiff.yml"; - if std::path::Path::new(path).exists() { - eprintln!("Config file '{path}' already exists. Remove it first to regenerate."); - return Err(ExitCode::from(2)); - } - - std::fs::write(path, config::DEFAULT_CONFIG_TEMPLATE).map_err(|e| { - eprintln!("Error writing config file: {e}"); - ExitCode::from(2) - })?; - - eprintln!("Created {path}"); - Ok(()) -} - -fn apply_color_mode(mode: ColorMode) { - match mode { - ColorMode::Auto => { - if !std::io::IsTerminal::is_terminal(&std::io::stdout()) { - colored::control::set_override(false); - } - } - ColorMode::Always => { - colored::control::set_override(true); - } - ColorMode::Never => { - colored::control::set_override(false); - } - } -} - -fn resolve_ssl_mode(mode: SslMode) -> loader::SslMode { - match mode { - SslMode::Disable => loader::SslMode::Disable, - SslMode::Prefer => loader::SslMode::Prefer, - SslMode::Require => loader::SslMode::Require, - } -} - -fn create_spinner(msg: &str) -> ProgressBar { - let pb = ProgressBar::new_spinner(); - pb.set_style( - ProgressStyle::with_template("{spinner:.cyan} {msg}") - .unwrap() - .tick_strings(&["⠋", "⠙", "⠹", "⠸", "⠼", "⠴", "⠦", "⠧", "⠇", "⠏"]), - ); - pb.set_message(msg.to_string()); - pb.enable_steady_tick(std::time::Duration::from_millis(80)); - pb -} diff --git a/src/migration.rs b/src/migration/mod.rs similarity index 72% rename from src/migration.rs rename to src/migration/mod.rs index 520196c..d70db2c 100644 --- a/src/migration.rs +++ b/src/migration/mod.rs @@ -1,8 +1,21 @@ +mod sql; + use serde::Serialize; -use crate::diff::{ColumnDiff, SchemaDiff, TableDiff}; +use crate::diff::SchemaDiff; use crate::loader::SqlDialect; -use crate::model::{Column, Constraint, ConstraintKind, Index, Table}; + +use sql::*; + +/// A single migration SQL statement with optional safety warnings. +#[derive(Debug, Clone, Serialize)] +pub struct MigrationStatement { + pub sql: String, + pub warnings: Vec, + /// Whether this statement acquires heavy locks (e.g. AccessExclusiveLock) + /// or performs a full table rewrite. + pub is_blocking: bool, +} /// Generate migration SQL statements from a schema diff. /// @@ -20,9 +33,52 @@ use crate::model::{Column, Constraint, ConstraintKind, Index, Table}; /// 11. CREATE INDEXes /// 12. ADD CONSTRAINTs for modified tables /// 13. CREATE new VIEWs -pub fn generate_migration(diff: &SchemaDiff, dialect: SqlDialect) -> Vec { +pub fn generate_migration(diff: &SchemaDiff, dialect: SqlDialect, concurrently: bool) -> Vec { let mut statements = Vec::new(); + // Phase 0: RENAME TABLEs (before any DROP/CREATE operations) + for rename in &diff.renamed_tables { + let raw_sql = rename_table_sql(&rename.old_name, &rename.new_name, dialect); + let is_medium = matches!(rename.confidence, crate::diff::RenameConfidence::Medium); + statements.push(MigrationStatement { + sql: if is_medium { + format!("-- {raw_sql} -- uncomment after manual verification") + } else { + raw_sql + }, + warnings: vec![format!( + "Detected table rename: '{}' → '{}' ({} confidence). Verify this is correct.", + rename.old_name, rename.new_name, rename.confidence + )], + is_blocking: false, + }); + } + + // Phase 0.5: RENAME COLUMNs (before DROP, to preserve data) + for table_diff in &diff.modified_tables { + for rename in &table_diff.renamed_columns { + let raw_sql = rename_column_sql( + &table_diff.table_name, + &rename.old.name, + &rename.new.name, + dialect, + ); + let is_medium = matches!(rename.confidence, crate::diff::RenameConfidence::Medium); + statements.push(MigrationStatement { + sql: if is_medium { + format!("-- {raw_sql} -- uncomment after manual verification") + } else { + raw_sql + }, + warnings: vec![format!( + "Detected column rename: '{}' → '{}' ({} confidence). Verify this is correct.", + rename.old.name, rename.new.name, rename.confidence + )], + is_blocking: false, + }); + } + } + // Phase 1: DROP CONSTRAINTs from modified tables for table_diff in &diff.modified_tables { for c in &table_diff.removed_constraints { @@ -218,7 +274,7 @@ pub fn generate_migration(diff: &SchemaDiff, dialect: SqlDialect) -> Vec Vec Vec recreate removed tables -> /// re-add removed columns -> recreate removed constraints/indexes -> /// recreate removed views -pub fn generate_rollback(diff: &SchemaDiff, dialect: SqlDialect) -> Vec { +pub fn generate_rollback(diff: &SchemaDiff, dialect: SqlDialect, _concurrently: bool) -> Vec { let mut statements = Vec::new(); // 1. Drop views that were added @@ -486,7 +547,7 @@ pub fn generate_rollback(diff: &SchemaDiff, dialect: SqlDialect) -> Vec Vec Vec Vec Vec, - /// Whether this statement acquires heavy locks (e.g. AccessExclusiveLock) - /// or performs a full table rewrite. - pub is_blocking: bool, -} - -fn quote_ident(name: &str, dialect: SqlDialect) -> String { - let needs_quoting = name.is_empty() - || !name - .chars() - .all(|c| c.is_ascii_lowercase() || c.is_ascii_digit() || c == '_'); - if needs_quoting { - match dialect { - SqlDialect::MySql => format!("`{}`", name.replace('`', "``")), - _ => format!("\"{}\"", name.replace('"', "\"\"")), - } - } else { - name.to_string() - } -} - -fn column_definition_sql(col: &Column, dialect: SqlDialect) -> String { - let mut def = format!("{} {}", quote_ident(&col.name, dialect), col.data_type); - if !col.is_nullable { - def.push_str(" NOT NULL"); - } - if let Some(ref default) = col.default { - def.push_str(&format!(" DEFAULT {default}")); - } - def -} - -fn create_table_sql(table: &Table, dialect: SqlDialect) -> String { - let columns: Vec = table - .columns - .values() - .map(|c| format!(" {}", column_definition_sql(c, dialect))) - .collect(); - format!( - "CREATE TABLE {} (\n{}\n);", - quote_ident(&table.name, dialect), - columns.join(",\n") - ) -} - -fn create_index_sql(idx: &Index, dialect: SqlDialect) -> String { - let unique = if idx.is_unique { "UNIQUE " } else { "" }; - // PostgreSQL index columns are raw SQL clauses from pg_get_indexdef() and may - // contain expressions (lower(email)), sort orders (created_at DESC), or - // already-quoted identifiers — they must NOT be wrapped with quote_ident. - // MySQL/SQLite index columns are plain identifier names from information_schema - // and need proper quoting. - let cols = match dialect { - SqlDialect::Postgres | SqlDialect::SqlFile | SqlDialect::Snapshot => idx.columns.join(", "), - _ => idx - .columns - .iter() - .map(|c| quote_ident(c, dialect)) - .collect::>() - .join(", "), - }; - format!( - "CREATE {unique}INDEX {} ON {}({});", - quote_ident(&idx.name, dialect), - quote_ident(&idx.table_name, dialect), - cols - ) -} - -fn drop_index_sql(idx: &Index, dialect: SqlDialect) -> String { - match dialect { - SqlDialect::MySql => format!( - "DROP INDEX {} ON {};", - quote_ident(&idx.name, dialect), - quote_ident(&idx.table_name, dialect) - ), - _ => format!("DROP INDEX {};", quote_ident(&idx.name, dialect)), - } -} - -fn add_constraint_sql(c: &Constraint, dialect: SqlDialect) -> String { - // SQLite does not support ALTER TABLE ADD CONSTRAINT - if dialect == SqlDialect::Sqlite { - return format!( - "-- manual migration required: add constraint {} on {}.{} \ - (SQLite does not support ALTER TABLE ADD CONSTRAINT; recreate the table)", - c.name, - c.table_name, - c.definition() - ); - } - let table = quote_ident(&c.table_name, dialect); - let name = quote_ident(&c.name, dialect); - match &c.kind { - ConstraintKind::ForeignKey { - columns, - ref_table, - ref_columns, - on_delete, - on_update, - } => { - let cols: Vec = columns.iter().map(|c| quote_ident(c, dialect)).collect(); - let refs: Vec = ref_columns - .iter() - .map(|c| quote_ident(c, dialect)) - .collect(); - let mut sql = format!( - "ALTER TABLE {table} ADD CONSTRAINT {name} FOREIGN KEY ({}) REFERENCES {}({})", - cols.join(", "), - quote_ident(ref_table, dialect), - refs.join(", ") - ); - if let Some(action) = on_delete { - sql.push_str(&format!(" ON DELETE {action}")); - } - if let Some(action) = on_update { - sql.push_str(&format!(" ON UPDATE {action}")); - } - sql.push(';'); - sql - } - ConstraintKind::Unique { columns } => { - let cols: Vec = columns.iter().map(|c| quote_ident(c, dialect)).collect(); - format!( - "ALTER TABLE {table} ADD CONSTRAINT {name} UNIQUE ({});", - cols.join(", ") - ) - } - ConstraintKind::Check { expression } => { - format!("ALTER TABLE {table} ADD CONSTRAINT {name} CHECK ({expression});") - } - } -} - -fn drop_constraint_sql(c: &Constraint, dialect: SqlDialect) -> String { - match dialect { - SqlDialect::MySql => { - // MySQL uses DROP FOREIGN KEY for FKs, DROP INDEX for unique constraints - match &c.kind { - ConstraintKind::ForeignKey { .. } => format!( - "ALTER TABLE {} DROP FOREIGN KEY {};", - quote_ident(&c.table_name, dialect), - quote_ident(&c.name, dialect) - ), - ConstraintKind::Unique { .. } => format!( - "ALTER TABLE {} DROP INDEX {};", - quote_ident(&c.table_name, dialect), - quote_ident(&c.name, dialect) - ), - ConstraintKind::Check { .. } => format!( - "ALTER TABLE {} DROP CHECK {};", - quote_ident(&c.table_name, dialect), - quote_ident(&c.name, dialect) - ), - } - } - SqlDialect::Sqlite => format!( - "-- manual migration required: drop constraint {} on {} \ - (SQLite does not support ALTER TABLE DROP CONSTRAINT; recreate the table)", - c.name, c.table_name - ), - _ => format!( - "ALTER TABLE {} DROP CONSTRAINT {};", - quote_ident(&c.table_name, dialect), - quote_ident(&c.name, dialect) - ), - } -} - -fn add_column_warnings(col: &Column) -> Vec { - let mut warnings = Vec::new(); - if !col.is_nullable && col.default.is_none() { - warnings.push(format!( - "Adding NOT NULL column '{}' without DEFAULT will fail on non-empty tables. \ - Consider: ADD COLUMN ... DEFAULT NULL first, then backfill, then SET NOT NULL.", - col.name - )); - } - if !col.is_nullable && col.default.is_some() { - warnings.push(format!( - "On PostgreSQL < 11, adding NOT NULL column '{}' with DEFAULT will rewrite the \ - entire table and acquire AccessExclusiveLock.", - col.name - )); - } - warnings -} - -/// Generate ALTER statements to revert modified columns to their old definitions (for rollback). -fn generate_column_alterations_reversed( - table_diff: &TableDiff, - dialect: SqlDialect, -) -> Vec { - // Swap old<->new in each ColumnDiff, then reuse the forward logic - let reversed = crate::diff::TableDiff { - table_name: table_diff.table_name.clone(), - added_columns: Vec::new(), - removed_columns: Vec::new(), - modified_columns: table_diff - .modified_columns - .iter() - .map(|cd| crate::diff::ColumnDiff { - old: cd.new.clone(), - new: cd.old.clone(), - }) - .collect(), - unchanged_columns: Vec::new(), - added_indexes: Vec::new(), - removed_indexes: Vec::new(), - added_constraints: Vec::new(), - removed_constraints: Vec::new(), - }; - generate_column_alterations(&reversed, dialect) -} - -fn generate_column_alterations( - table_diff: &TableDiff, - dialect: SqlDialect, -) -> Vec { - let mut stmts = Vec::new(); - - for col_diff in &table_diff.modified_columns { - let ColumnDiff { old, new } = col_diff; - let table = &table_diff.table_name; - - // Type change — blocking: requires table rewrite / AccessExclusiveLock - if old.data_type != new.data_type { - match dialect { - SqlDialect::MySql => stmts.push(MigrationStatement { - sql: format!( - "ALTER TABLE {} MODIFY COLUMN {};", - quote_ident(table, dialect), - column_definition_sql(new, dialect) - ), - warnings: vec![format!( - "Changing column type from '{}' to '{}' may require a table rewrite \ - and table lock.", - old.data_type, new.data_type - )], - is_blocking: true, - }), - SqlDialect::Sqlite => stmts.push(MigrationStatement { - sql: format!( - "-- manual migration required for type change on {}.{}", - quote_ident(table, dialect), - quote_ident(&new.name, dialect) - ), - warnings: vec![format!( - "SQLite does not support ALTER COLUMN TYPE directly for '{}'. \ - Recreate table '{table}' with the desired column definition.", - new.name - )], - is_blocking: true, - }), - _ => stmts.push(MigrationStatement { - sql: format!( - "ALTER TABLE {} ALTER COLUMN {} TYPE {};", - quote_ident(table, dialect), - quote_ident(&new.name, dialect), - new.data_type - ), - warnings: vec![format!( - "Changing column type from '{}' to '{}' may require a table rewrite \ - and AccessExclusiveLock.", - old.data_type, new.data_type - )], - is_blocking: true, - }), - } - } - - // Nullability change - if old.is_nullable != new.is_nullable { - // SET NOT NULL is blocking (full table scan + AccessExclusiveLock) - // DROP NOT NULL is non-blocking - let blocking = !new.is_nullable; - match dialect { - SqlDialect::MySql => { - let warning = if new.is_nullable { - format!( - "Changing '{}' to NULL via MODIFY COLUMN may rebuild the table depending \ - on MySQL/MariaDB version and storage engine.", - new.name - ) - } else { - format!( - "Changing '{}' to NOT NULL via MODIFY COLUMN can fail if existing rows \ - contain NULL values.", - new.name - ) - }; - stmts.push(MigrationStatement { - sql: format!( - "ALTER TABLE {} MODIFY COLUMN {};", - quote_ident(table, dialect), - column_definition_sql(new, dialect) - ), - warnings: vec![warning], - is_blocking: blocking, - }); - } - SqlDialect::Sqlite => { - stmts.push(MigrationStatement { - sql: format!( - "-- manual migration required for nullability change on {}.{}", - quote_ident(table, dialect), - quote_ident(&new.name, dialect) - ), - warnings: vec![format!( - "SQLite does not support ALTER COLUMN nullability directly for '{}'. \ - Recreate table '{table}' with the desired column definition.", - new.name - )], - is_blocking: blocking, - }); - } - _ => { - if new.is_nullable { - stmts.push(MigrationStatement { - sql: format!( - "ALTER TABLE {} ALTER COLUMN {} DROP NOT NULL;", - quote_ident(table, dialect), - quote_ident(&new.name, dialect) - ), - warnings: Vec::new(), - is_blocking: false, - }); - } else { - stmts.push(MigrationStatement { - sql: format!( - "ALTER TABLE {} ALTER COLUMN {} SET NOT NULL;", - quote_ident(table, dialect), - quote_ident(&new.name, dialect) - ), - warnings: vec![format!( - "SET NOT NULL on '{}' will scan the entire table to verify no NULLs exist. \ - This acquires AccessExclusiveLock.", - new.name - )], - is_blocking: true, - }); - } - } - } - } - - // Default change — non-blocking (metadata-only on modern PG) - if old.default != new.default { - match &new.default { - Some(default) => { - stmts.push(MigrationStatement { - sql: format!( - "ALTER TABLE {} ALTER COLUMN {} SET DEFAULT {default};", - quote_ident(table, dialect), - quote_ident(&new.name, dialect) - ), - warnings: Vec::new(), - is_blocking: false, - }); - } - None => { - stmts.push(MigrationStatement { - sql: format!( - "ALTER TABLE {} ALTER COLUMN {} DROP DEFAULT;", - quote_ident(table, dialect), - quote_ident(&new.name, dialect) - ), - warnings: Vec::new(), - is_blocking: false, - }); - } - } - } - } - - stmts -} - #[cfg(test)] mod tests { use super::*; @@ -991,7 +695,7 @@ mod tests { right.tables.insert("users".into(), t); let diff = diff_schemas(&left, &right); - let stmts = generate_migration(&diff, SqlDialect::Postgres); + let stmts = generate_migration(&diff, SqlDialect::Postgres, false); assert_eq!(stmts.len(), 1); assert_eq!( @@ -1018,7 +722,7 @@ mod tests { right.tables.insert("users".into(), t); let diff = diff_schemas(&left, &right); - let stmts = generate_migration(&diff, SqlDialect::Postgres); + let stmts = generate_migration(&diff, SqlDialect::Postgres, false); assert_eq!(stmts.len(), 1); assert_eq!(stmts[0].sql, "ALTER TABLE users DROP COLUMN old_field;"); @@ -1038,7 +742,7 @@ mod tests { right.tables.insert("orders".into(), t); let diff = diff_schemas(&left, &right); - let stmts = generate_migration(&diff, SqlDialect::Postgres); + let stmts = generate_migration(&diff, SqlDialect::Postgres, false); assert_eq!(stmts.len(), 1); assert!(stmts[0].sql.starts_with("CREATE TABLE orders")); @@ -1061,7 +765,7 @@ mod tests { right.tables.insert("users".into(), t); let diff = diff_schemas(&left, &right); - let stmts = generate_migration(&diff, SqlDialect::Postgres); + let stmts = generate_migration(&diff, SqlDialect::Postgres, false); assert_eq!(stmts.len(), 1); assert_eq!( @@ -1094,7 +798,7 @@ mod tests { right.tables.insert("orders".into(), t); let diff = diff_schemas(&left, &right); - let stmts = generate_migration(&diff, SqlDialect::Postgres); + let stmts = generate_migration(&diff, SqlDialect::Postgres, false); assert_eq!(stmts.len(), 1); assert_eq!(stmts[0].sql, "CREATE INDEX idx_orders_id ON orders(id);"); @@ -1102,7 +806,6 @@ mod tests { #[test] fn migration_ordering() { - // Complex scenario: drop table + add table + modify columns + add index let mut left = Schema::new(); let mut legacy = Table::new("legacy"); legacy @@ -1136,11 +839,10 @@ mod tests { right.tables.insert("orders".into(), orders); let diff = diff_schemas(&left, &right); - let stmts = generate_migration(&diff, SqlDialect::Postgres); + let stmts = generate_migration(&diff, SqlDialect::Postgres, false); let sqls: Vec<&str> = stmts.iter().map(|s| s.sql.as_str()).collect(); - // DROP COLUMN before DROP TABLE before CREATE TABLE before ADD COLUMN let drop_col_pos = sqls.iter().position(|s| s.contains("DROP COLUMN")).unwrap(); let drop_table_pos = sqls.iter().position(|s| s.contains("DROP TABLE")).unwrap(); let create_table_pos = sqls @@ -1178,7 +880,7 @@ mod tests { right.tables.insert("orders".into(), t); let diff = diff_schemas(&left, &right); - let stmts = generate_migration(&diff, SqlDialect::MySql); + let stmts = generate_migration(&diff, SqlDialect::MySql, false); assert_eq!(stmts[0].sql, "DROP INDEX idx_orders_id ON orders;"); } @@ -1198,7 +900,7 @@ mod tests { right.tables.insert("users".into(), t); let diff = diff_schemas(&left, &right); - let stmts = generate_migration(&diff, SqlDialect::Sqlite); + let stmts = generate_migration(&diff, SqlDialect::Sqlite, false); assert!(stmts[0].sql.starts_with("-- manual migration required")); assert!(stmts[0].warnings[0].contains("does not support ALTER COLUMN TYPE")); @@ -1219,7 +921,7 @@ mod tests { right.tables.insert("users".into(), t); let diff = diff_schemas(&left, &right); - let stmts = generate_migration(&diff, SqlDialect::MySql); + let stmts = generate_migration(&diff, SqlDialect::MySql, false); assert_eq!(stmts.len(), 1); assert_eq!( @@ -1243,7 +945,7 @@ mod tests { right.tables.insert("users".into(), t); let diff = diff_schemas(&left, &right); - let stmts = generate_migration(&diff, SqlDialect::Sqlite); + let stmts = generate_migration(&diff, SqlDialect::Sqlite, false); assert_eq!(stmts.len(), 1); assert!(stmts[0] @@ -1276,7 +978,6 @@ mod tests { "created_at".into(), col("created_at", "timestamptz", false, None), ); - // Expression index: lower(email) t.indexes.insert( "idx_users_email_lower".into(), Index { @@ -1286,7 +987,6 @@ mod tests { is_unique: false, }, ); - // Sorted index: created_at DESC t.indexes.insert( "idx_users_created_at_desc".into(), Index { @@ -1296,7 +996,6 @@ mod tests { is_unique: false, }, ); - // Already-quoted identifier from pg_indexes t.indexes.insert( "idx_users_mixed".into(), Index { @@ -1309,15 +1008,12 @@ mod tests { right.tables.insert("users".into(), t); let diff = diff_schemas(&left, &right); - let stmts = generate_migration(&diff, SqlDialect::Postgres); + let stmts = generate_migration(&diff, SqlDialect::Postgres, false); let sqls: Vec<&str> = stmts.iter().map(|s| s.sql.as_str()).collect(); - // Expression index — column clause must not be quoted assert!(sqls.contains(&"CREATE INDEX idx_users_email_lower ON users(lower(email));")); - // Sorted index — DESC must not be quoted assert!(sqls.contains(&"CREATE INDEX idx_users_created_at_desc ON users(created_at DESC);")); - // Mixed: already-quoted ident + sort order assert!( sqls.contains(&"CREATE INDEX idx_users_mixed ON users(\"Email\", created_at DESC);") ); @@ -1344,22 +1040,18 @@ mod tests { right.tables.insert("UserAccounts".into(), t); let diff = diff_schemas(&left, &right); - let stmts = generate_migration(&diff, SqlDialect::MySql); + let stmts = generate_migration(&diff, SqlDialect::MySql, false); let sqls: Vec<&str> = stmts.iter().map(|s| s.sql.as_str()).collect(); - // Table and column names with uppercase must use backticks, not double quotes assert!(sqls.iter().any(|s| s.contains("`UserAccounts`"))); assert!(sqls.iter().any(|s| s.contains("`UserId`"))); assert!(sqls.iter().any(|s| s.contains("`idx_UserAccounts_email`"))); - // Must NOT contain double-quoted identifiers assert!(!sqls.iter().any(|s| s.contains("\"UserAccounts\""))); assert!(!sqls.iter().any(|s| s.contains("\"UserId\""))); } #[test] fn mysql_index_columns_are_quoted() { - // MySQL index columns are plain identifiers from information_schema, - // not raw SQL — they must be quoted when needed. let left = Schema::new(); let mut right = Schema::new(); let mut t = Table::new("items"); @@ -1377,10 +1069,9 @@ mod tests { right.tables.insert("items".into(), t); let diff = diff_schemas(&left, &right); - let stmts = generate_migration(&diff, SqlDialect::MySql); + let stmts = generate_migration(&diff, SqlDialect::MySql, false); let sqls: Vec<&str> = stmts.iter().map(|s| s.sql.as_str()).collect(); - // Column name with uppercase must be backtick-quoted in MySQL index assert!(sqls.iter().any(|s| s.contains("(`Select`)"))); } @@ -1394,48 +1085,31 @@ mod tests { right.tables.insert("users".into(), t); let diff = diff_schemas(&left, &right); - let stmts = generate_migration(&diff, SqlDialect::SqlFile); + let stmts = generate_migration(&diff, SqlDialect::SqlFile, false); - // Simple lowercase names should not be quoted for SqlFile dialect assert!(stmts[0].sql.contains("CREATE TABLE users")); assert!(stmts[0].sql.contains("email text")); } #[test] fn quote_ident_postgres() { - use super::quote_ident; let pg = SqlDialect::Postgres; - // Simple names pass through unquoted assert_eq!(quote_ident("users", pg), "users"); assert_eq!(quote_ident("idx_orders_id", pg), "idx_orders_id"); - - // Names with uppercase get double-quoted assert_eq!(quote_ident("Users", pg), "\"Users\""); - - // Names with spaces get double-quoted assert_eq!(quote_ident("my table", pg), "\"my table\""); - - // Names with embedded quotes get escaped assert_eq!(quote_ident("a\"b", pg), "\"a\"\"b\""); - - // Empty string gets quoted assert_eq!(quote_ident("", pg), "\"\""); } #[test] fn quote_ident_mysql_uses_backticks() { - use super::quote_ident; let my = SqlDialect::MySql; - // Simple names pass through unquoted assert_eq!(quote_ident("users", my), "users"); - - // Names needing quoting use backticks assert_eq!(quote_ident("Users", my), "`Users`"); assert_eq!(quote_ident("my table", my), "`my table`"); - - // Embedded backtick gets escaped assert_eq!(quote_ident("a`b", my), "`a``b`"); } @@ -1456,7 +1130,7 @@ mod tests { right.tables.insert("users".into(), t); let diff = diff_schemas(&left, &right); - let stmts = generate_migration(&diff, SqlDialect::Postgres); + let stmts = generate_migration(&diff, SqlDialect::Postgres, false); assert!(stmts[0].is_blocking, "DROP COLUMN should be blocking"); } @@ -1484,7 +1158,7 @@ mod tests { right.tables.insert("users".into(), t); let diff = diff_schemas(&left, &right); - let stmts = generate_migration(&diff, SqlDialect::Postgres); + let stmts = generate_migration(&diff, SqlDialect::Postgres, false); assert!(stmts[0].is_blocking, "DROP INDEX should be blocking"); } @@ -1498,7 +1172,7 @@ mod tests { right.tables.insert("users".into(), t); let diff = diff_schemas(&left, &right); - let stmts = generate_migration(&diff, SqlDialect::Postgres); + let stmts = generate_migration(&diff, SqlDialect::Postgres, false); assert!(!stmts[0].is_blocking, "CREATE TABLE should not be blocking"); } @@ -1512,7 +1186,7 @@ mod tests { let right = Schema::new(); let diff = diff_schemas(&left, &right); - let stmts = generate_migration(&diff, SqlDialect::Postgres); + let stmts = generate_migration(&diff, SqlDialect::Postgres, false); assert!( stmts .iter() @@ -1556,7 +1230,7 @@ mod tests { right.tables.insert("orders".into(), t); let diff = diff_schemas(&left, &right); - let stmts = generate_migration(&diff, SqlDialect::MySql); + let stmts = generate_migration(&diff, SqlDialect::MySql, false); assert!( stmts[0].sql.contains("DROP FOREIGN KEY"), @@ -1592,7 +1266,7 @@ mod tests { right.tables.insert("users".into(), t); let diff = diff_schemas(&left, &right); - let stmts = generate_migration(&diff, SqlDialect::MySql); + let stmts = generate_migration(&diff, SqlDialect::MySql, false); assert!( stmts[0].sql.contains("DROP INDEX"), @@ -1632,7 +1306,7 @@ mod tests { right.tables.insert("orders".into(), t); let diff = diff_schemas(&left, &right); - let stmts = generate_migration(&diff, SqlDialect::Postgres); + let stmts = generate_migration(&diff, SqlDialect::Postgres, false); assert!(stmts .iter() @@ -1650,7 +1324,7 @@ mod tests { right.tables.insert("orders".into(), t); let diff = diff_schemas(&left, &right); - let rollback = generate_rollback(&diff, SqlDialect::Postgres); + let rollback = generate_rollback(&diff, SqlDialect::Postgres, false); assert!(rollback.iter().any(|s| s.sql.contains("DROP TABLE orders"))); assert!( @@ -1671,7 +1345,7 @@ mod tests { let right = Schema::new(); let diff = diff_schemas(&left, &right); - let rollback = generate_rollback(&diff, SqlDialect::Postgres); + let rollback = generate_rollback(&diff, SqlDialect::Postgres, false); assert!(rollback .iter() @@ -1695,7 +1369,7 @@ mod tests { right.tables.insert("users".into(), t); let diff = diff_schemas(&left, &right); - let rollback = generate_rollback(&diff, SqlDialect::Postgres); + let rollback = generate_rollback(&diff, SqlDialect::Postgres, false); let drop_col = rollback .iter() @@ -1719,7 +1393,7 @@ mod tests { right.tables.insert(t.name.clone(), t); let diff = diff_schemas(&left, &right); - let stmts = generate_migration(&diff, SqlDialect::Postgres); + let stmts = generate_migration(&diff, SqlDialect::Postgres, false); assert!(stmts[0] .sql @@ -1728,4 +1402,306 @@ mod tests { .sql .contains("\"email\"\"; DELETE FROM users; --\" text")); } + + // ── Primary Key migration tests ── + + #[test] + fn pk_add_generates_alter_table() { + use crate::model::{Constraint, ConstraintKind}; + + let mut left = Schema::new(); + let mut t = Table::new("orders"); + t.columns + .insert("id".into(), col("id", "integer", false, None)); + left.tables.insert("orders".into(), t); + + let mut right = Schema::new(); + let mut t = Table::new("orders"); + t.columns + .insert("id".into(), col("id", "integer", false, None)); + t.constraints.insert( + "orders_pkey".into(), + Constraint { + name: "orders_pkey".into(), + table_name: "orders".into(), + kind: ConstraintKind::PrimaryKey { + columns: vec!["id".into()], + }, + }, + ); + right.tables.insert("orders".into(), t); + + let diff = diff_schemas(&left, &right); + let stmts = generate_migration(&diff, SqlDialect::Postgres, false); + + assert!(stmts + .iter() + .any(|s| s.sql.contains("ADD CONSTRAINT orders_pkey PRIMARY KEY (id)"))); + } + + #[test] + fn pk_drop_mysql_uses_drop_primary_key() { + use crate::model::{Constraint, ConstraintKind}; + + let mut left = Schema::new(); + let mut t = Table::new("orders"); + t.columns + .insert("id".into(), col("id", "integer", false, None)); + t.constraints.insert( + "orders_pkey".into(), + Constraint { + name: "orders_pkey".into(), + table_name: "orders".into(), + kind: ConstraintKind::PrimaryKey { + columns: vec!["id".into()], + }, + }, + ); + left.tables.insert("orders".into(), t); + + let mut right = Schema::new(); + let mut t = Table::new("orders"); + t.columns + .insert("id".into(), col("id", "integer", false, None)); + right.tables.insert("orders".into(), t); + + let diff = diff_schemas(&left, &right); + let stmts = generate_migration(&diff, SqlDialect::MySql, false); + + assert!(stmts + .iter() + .any(|s| s.sql.contains("DROP PRIMARY KEY"))); + } + + // ── CONCURRENTLY tests ── + + #[test] + fn concurrently_generates_concurrent_index_for_postgres() { + let mut left = Schema::new(); + let mut t = Table::new("users"); + t.columns + .insert("id".into(), col("id", "integer", false, None)); + left.tables.insert("users".into(), t); + + let mut right = Schema::new(); + let mut t = Table::new("users"); + t.columns + .insert("id".into(), col("id", "integer", false, None)); + t.indexes.insert( + "idx_users_id".into(), + Index { + name: "idx_users_id".into(), + table_name: "users".into(), + columns: vec!["id".into()], + is_unique: false, + }, + ); + right.tables.insert("users".into(), t); + + let diff = diff_schemas(&left, &right); + let stmts = generate_migration(&diff, SqlDialect::Postgres, true); + + assert_eq!(stmts.len(), 1); + assert!( + stmts[0].sql.contains("CONCURRENTLY"), + "Expected CONCURRENTLY in: {}", + stmts[0].sql + ); + assert!(!stmts[0].is_blocking, "CONCURRENTLY indexes should not be blocking"); + assert!(stmts[0].warnings.is_empty(), "No 'Consider CONCURRENTLY' warning when already concurrent"); + } + + #[test] + fn concurrently_not_applied_to_new_table_indexes() { + let left = Schema::new(); + + let mut right = Schema::new(); + let mut t = Table::new("orders"); + t.columns + .insert("id".into(), col("id", "integer", false, None)); + t.indexes.insert( + "idx_orders_id".into(), + Index { + name: "idx_orders_id".into(), + table_name: "orders".into(), + columns: vec!["id".into()], + is_unique: false, + }, + ); + right.tables.insert("orders".into(), t); + + let diff = diff_schemas(&left, &right); + let stmts = generate_migration(&diff, SqlDialect::Postgres, true); + + // New table indexes should NOT use CONCURRENTLY (table is empty) + let idx_stmt = stmts.iter().find(|s| s.sql.contains("CREATE") && s.sql.contains("INDEX")).unwrap(); + assert!( + !idx_stmt.sql.contains("CONCURRENTLY"), + "New table indexes should not use CONCURRENTLY: {}", + idx_stmt.sql + ); + } + + #[test] + fn concurrently_ignored_for_mysql() { + let mut left = Schema::new(); + let mut t = Table::new("users"); + t.columns + .insert("id".into(), col("id", "integer", false, None)); + left.tables.insert("users".into(), t); + + let mut right = Schema::new(); + let mut t = Table::new("users"); + t.columns + .insert("id".into(), col("id", "integer", false, None)); + t.indexes.insert( + "idx_users_id".into(), + Index { + name: "idx_users_id".into(), + table_name: "users".into(), + columns: vec!["id".into()], + is_unique: false, + }, + ); + right.tables.insert("users".into(), t); + + let diff = diff_schemas(&left, &right); + let stmts = generate_migration(&diff, SqlDialect::MySql, true); + + assert!( + !stmts[0].sql.contains("CONCURRENTLY"), + "MySQL should not use CONCURRENTLY" + ); + } + + // ── Rename migration tests ── + + #[test] + fn column_rename_generates_alter_rename() { + use crate::diff::diff_schemas_with_options; + + let mut left = Schema::new(); + let mut t = Table::new("users"); + t.columns + .insert("id".into(), col("id", "integer", false, None)); + t.columns + .insert("email_addr".into(), col("email_addr", "text", true, None)); + left.tables.insert("users".into(), t); + + let mut right = Schema::new(); + let mut t = Table::new("users"); + t.columns + .insert("id".into(), col("id", "integer", false, None)); + t.columns + .insert("email".into(), col("email", "text", true, None)); + right.tables.insert("users".into(), t); + + let diff = diff_schemas_with_options(&left, &right, true); + let stmts = generate_migration(&diff, SqlDialect::Postgres, false); + + assert!( + stmts.iter().any(|s| s.sql.contains("RENAME COLUMN email_addr TO email")), + "Expected RENAME COLUMN, got: {:?}", + stmts.iter().map(|s| &s.sql).collect::>() + ); + // Should NOT have DROP + ADD for the renamed column + assert!(!stmts.iter().any(|s| s.sql.contains("DROP COLUMN email_addr"))); + assert!(!stmts.iter().any(|s| s.sql.contains("ADD COLUMN email"))); + } + + #[test] + fn table_rename_generates_alter_rename() { + use crate::diff::diff_schemas_with_options; + + let mut left = Schema::new(); + let mut t = Table::new("user_logs"); + t.columns + .insert("id".into(), col("id", "integer", false, None)); + t.columns + .insert("msg".into(), col("msg", "text", true, None)); + left.tables.insert("user_logs".into(), t); + + let mut right = Schema::new(); + let mut t = Table::new("audit_trail"); + t.columns + .insert("id".into(), col("id", "integer", false, None)); + t.columns + .insert("msg".into(), col("msg", "text", true, None)); + right.tables.insert("audit_trail".into(), t); + + let diff = diff_schemas_with_options(&left, &right, true); + let stmts = generate_migration(&diff, SqlDialect::Postgres, false); + + assert!( + stmts.iter().any(|s| s.sql.contains("RENAME TO audit_trail")), + "Expected RENAME TO, got: {:?}", + stmts.iter().map(|s| &s.sql).collect::>() + ); + assert!(!stmts.iter().any(|s| s.sql.contains("DROP TABLE user_logs"))); + assert!(!stmts.iter().any(|s| s.sql.contains("CREATE TABLE audit_trail"))); + } + + #[test] + fn medium_confidence_rename_is_commented_out() { + use crate::diff::diff_schemas_with_options; + + let mut left = Schema::new(); + let mut t = Table::new("users"); + t.columns.insert( + "is_enabled".into(), + col("is_enabled", "bool", false, Some("true")), + ); + left.tables.insert("users".into(), t); + + let mut right = Schema::new(); + let mut t = Table::new("users"); + t.columns.insert( + "is_active".into(), + col("is_active", "bool", false, Some("false")), + ); + right.tables.insert("users".into(), t); + + let diff = diff_schemas_with_options(&left, &right, true); + let stmts = generate_migration(&diff, SqlDialect::Postgres, false); + + let rename_stmt = stmts + .iter() + .find(|s| s.sql.contains("RENAME COLUMN")) + .expect("Expected a rename statement"); + assert!( + rename_stmt.sql.starts_with("-- "), + "Medium confidence rename should be commented out: {}", + rename_stmt.sql + ); + } + + #[test] + fn rollback_reverses_column_rename() { + use crate::diff::diff_schemas_with_options; + + let mut left = Schema::new(); + let mut t = Table::new("users"); + t.columns + .insert("id".into(), col("id", "integer", false, None)); + t.columns + .insert("old_name".into(), col("old_name", "text", true, None)); + left.tables.insert("users".into(), t); + + let mut right = Schema::new(); + let mut t = Table::new("users"); + t.columns + .insert("id".into(), col("id", "integer", false, None)); + t.columns + .insert("new_name".into(), col("new_name", "text", true, None)); + right.tables.insert("users".into(), t); + + let diff = diff_schemas_with_options(&left, &right, true); + let rollback = generate_rollback(&diff, SqlDialect::Postgres, false); + + assert!( + rollback.iter().any(|s| s.sql.contains("RENAME COLUMN new_name TO old_name")), + "Rollback should reverse column rename, got: {:?}", + rollback.iter().map(|s| &s.sql).collect::>() + ); + } } diff --git a/src/migration/sql.rs b/src/migration/sql.rs new file mode 100644 index 0000000..6bf38ef --- /dev/null +++ b/src/migration/sql.rs @@ -0,0 +1,429 @@ +use crate::diff::{ColumnDiff, TableDiff}; +use crate::loader::SqlDialect; +use crate::model::{Column, Constraint, ConstraintKind, Index, Table}; + +use super::MigrationStatement; + +pub fn quote_ident(name: &str, dialect: SqlDialect) -> String { + let needs_quoting = name.is_empty() + || !name + .chars() + .all(|c| c.is_ascii_lowercase() || c.is_ascii_digit() || c == '_'); + if needs_quoting { + match dialect { + SqlDialect::MySql => format!("`{}`", name.replace('`', "``")), + _ => format!("\"{}\"", name.replace('"', "\"\"")), + } + } else { + name.to_string() + } +} + +pub fn column_definition_sql(col: &Column, dialect: SqlDialect) -> String { + let mut def = format!("{} {}", quote_ident(&col.name, dialect), col.data_type); + if !col.is_nullable { + def.push_str(" NOT NULL"); + } + if let Some(ref default) = col.default { + def.push_str(&format!(" DEFAULT {default}")); + } + def +} + +pub fn create_table_sql(table: &Table, dialect: SqlDialect) -> String { + let columns: Vec = table + .columns + .values() + .map(|c| format!(" {}", column_definition_sql(c, dialect))) + .collect(); + format!( + "CREATE TABLE {} (\n{}\n);", + quote_ident(&table.name, dialect), + columns.join(",\n") + ) +} + +pub fn create_index_sql(idx: &Index, dialect: SqlDialect, concurrently: bool) -> String { + let unique = if idx.is_unique { "UNIQUE " } else { "" }; + let concurrent = if concurrently && matches!(dialect, SqlDialect::Postgres) { + "CONCURRENTLY " + } else { + "" + }; + // PostgreSQL index columns are raw SQL clauses from pg_get_indexdef() and may + // contain expressions (lower(email)), sort orders (created_at DESC), or + // already-quoted identifiers — they must NOT be wrapped with quote_ident. + // MySQL/SQLite index columns are plain identifier names from information_schema + // and need proper quoting. + let cols = match dialect { + SqlDialect::Postgres | SqlDialect::SqlFile | SqlDialect::Snapshot => idx.columns.join(", "), + _ => idx + .columns + .iter() + .map(|c| quote_ident(c, dialect)) + .collect::>() + .join(", "), + }; + format!( + "CREATE {unique}INDEX {concurrent}{} ON {}({});", + quote_ident(&idx.name, dialect), + quote_ident(&idx.table_name, dialect), + cols + ) +} + +pub fn drop_index_sql(idx: &Index, dialect: SqlDialect) -> String { + match dialect { + SqlDialect::MySql => format!( + "DROP INDEX {} ON {};", + quote_ident(&idx.name, dialect), + quote_ident(&idx.table_name, dialect) + ), + _ => format!("DROP INDEX {};", quote_ident(&idx.name, dialect)), + } +} + +pub fn add_constraint_sql(c: &Constraint, dialect: SqlDialect) -> String { + // SQLite does not support ALTER TABLE ADD CONSTRAINT + if dialect == SqlDialect::Sqlite { + return format!( + "-- manual migration required: add constraint {} on {}.{} \ + (SQLite does not support ALTER TABLE ADD CONSTRAINT; recreate the table)", + c.name, + c.table_name, + c.definition() + ); + } + let table = quote_ident(&c.table_name, dialect); + let name = quote_ident(&c.name, dialect); + match &c.kind { + ConstraintKind::PrimaryKey { columns } => { + let cols: Vec = columns.iter().map(|c| quote_ident(c, dialect)).collect(); + // MySQL does not accept named PRIMARY KEY in ADD CONSTRAINT + if dialect == SqlDialect::MySql { + format!( + "ALTER TABLE {table} ADD PRIMARY KEY ({});", + cols.join(", ") + ) + } else { + format!( + "ALTER TABLE {table} ADD CONSTRAINT {name} PRIMARY KEY ({});", + cols.join(", ") + ) + } + } + ConstraintKind::ForeignKey { + columns, + ref_table, + ref_columns, + on_delete, + on_update, + } => { + let cols: Vec = columns.iter().map(|c| quote_ident(c, dialect)).collect(); + let refs: Vec = ref_columns + .iter() + .map(|c| quote_ident(c, dialect)) + .collect(); + let mut sql = format!( + "ALTER TABLE {table} ADD CONSTRAINT {name} FOREIGN KEY ({}) REFERENCES {}({})", + cols.join(", "), + quote_ident(ref_table, dialect), + refs.join(", ") + ); + if let Some(action) = on_delete { + sql.push_str(&format!(" ON DELETE {action}")); + } + if let Some(action) = on_update { + sql.push_str(&format!(" ON UPDATE {action}")); + } + sql.push(';'); + sql + } + ConstraintKind::Unique { columns } => { + let cols: Vec = columns.iter().map(|c| quote_ident(c, dialect)).collect(); + format!( + "ALTER TABLE {table} ADD CONSTRAINT {name} UNIQUE ({});", + cols.join(", ") + ) + } + ConstraintKind::Check { expression } => { + format!("ALTER TABLE {table} ADD CONSTRAINT {name} CHECK ({expression});") + } + } +} + +pub fn drop_constraint_sql(c: &Constraint, dialect: SqlDialect) -> String { + match dialect { + SqlDialect::MySql => { + // MySQL uses DROP PRIMARY KEY, DROP FOREIGN KEY, DROP INDEX for different constraint types + match &c.kind { + ConstraintKind::PrimaryKey { .. } => format!( + "ALTER TABLE {} DROP PRIMARY KEY;", + quote_ident(&c.table_name, dialect) + ), + ConstraintKind::ForeignKey { .. } => format!( + "ALTER TABLE {} DROP FOREIGN KEY {};", + quote_ident(&c.table_name, dialect), + quote_ident(&c.name, dialect) + ), + ConstraintKind::Unique { .. } => format!( + "ALTER TABLE {} DROP INDEX {};", + quote_ident(&c.table_name, dialect), + quote_ident(&c.name, dialect) + ), + ConstraintKind::Check { .. } => format!( + "ALTER TABLE {} DROP CHECK {};", + quote_ident(&c.table_name, dialect), + quote_ident(&c.name, dialect) + ), + } + } + SqlDialect::Sqlite => format!( + "-- manual migration required: drop constraint {} on {} \ + (SQLite does not support ALTER TABLE DROP CONSTRAINT; recreate the table)", + c.name, c.table_name + ), + _ => format!( + "ALTER TABLE {} DROP CONSTRAINT {};", + quote_ident(&c.table_name, dialect), + quote_ident(&c.name, dialect) + ), + } +} + +pub fn rename_column_sql( + table: &str, + old_name: &str, + new_name: &str, + dialect: SqlDialect, +) -> String { + format!( + "ALTER TABLE {} RENAME COLUMN {} TO {};", + quote_ident(table, dialect), + quote_ident(old_name, dialect), + quote_ident(new_name, dialect) + ) +} + +pub fn rename_table_sql(old_name: &str, new_name: &str, dialect: SqlDialect) -> String { + match dialect { + SqlDialect::MySql => format!( + "RENAME TABLE {} TO {};", + quote_ident(old_name, dialect), + quote_ident(new_name, dialect) + ), + _ => format!( + "ALTER TABLE {} RENAME TO {};", + quote_ident(old_name, dialect), + quote_ident(new_name, dialect) + ), + } +} + +pub fn add_column_warnings(col: &Column) -> Vec { + let mut warnings = Vec::new(); + if !col.is_nullable && col.default.is_none() { + warnings.push(format!( + "Adding NOT NULL column '{}' without DEFAULT will fail on non-empty tables. \ + Consider: ADD COLUMN ... DEFAULT NULL first, then backfill, then SET NOT NULL.", + col.name + )); + } + if !col.is_nullable && col.default.is_some() { + warnings.push(format!( + "On PostgreSQL < 11, adding NOT NULL column '{}' with DEFAULT will rewrite the \ + entire table and acquire AccessExclusiveLock.", + col.name + )); + } + warnings +} + +/// Generate ALTER statements to revert modified columns to their old definitions (for rollback). +pub fn generate_column_alterations_reversed( + table_diff: &TableDiff, + dialect: SqlDialect, +) -> Vec { + // Swap old<->new in each ColumnDiff, then reuse the forward logic + let reversed = TableDiff { + table_name: table_diff.table_name.clone(), + added_columns: Vec::new(), + removed_columns: Vec::new(), + renamed_columns: Vec::new(), + modified_columns: table_diff + .modified_columns + .iter() + .map(|cd| ColumnDiff { + old: cd.new.clone(), + new: cd.old.clone(), + }) + .collect(), + unchanged_columns: Vec::new(), + added_indexes: Vec::new(), + removed_indexes: Vec::new(), + added_constraints: Vec::new(), + removed_constraints: Vec::new(), + }; + generate_column_alterations(&reversed, dialect) +} + +pub fn generate_column_alterations( + table_diff: &TableDiff, + dialect: SqlDialect, +) -> Vec { + let mut stmts = Vec::new(); + + for col_diff in &table_diff.modified_columns { + let ColumnDiff { old, new } = col_diff; + let table = &table_diff.table_name; + + // Type change — blocking: requires table rewrite / AccessExclusiveLock + if old.data_type != new.data_type { + match dialect { + SqlDialect::MySql => stmts.push(MigrationStatement { + sql: format!( + "ALTER TABLE {} MODIFY COLUMN {};", + quote_ident(table, dialect), + column_definition_sql(new, dialect) + ), + warnings: vec![format!( + "Changing column type from '{}' to '{}' may require a table rewrite \ + and table lock.", + old.data_type, new.data_type + )], + is_blocking: true, + }), + SqlDialect::Sqlite => stmts.push(MigrationStatement { + sql: format!( + "-- manual migration required for type change on {}.{}", + quote_ident(table, dialect), + quote_ident(&new.name, dialect) + ), + warnings: vec![format!( + "SQLite does not support ALTER COLUMN TYPE directly for '{}'. \ + Recreate table '{table}' with the desired column definition.", + new.name + )], + is_blocking: true, + }), + _ => stmts.push(MigrationStatement { + sql: format!( + "ALTER TABLE {} ALTER COLUMN {} TYPE {};", + quote_ident(table, dialect), + quote_ident(&new.name, dialect), + new.data_type + ), + warnings: vec![format!( + "Changing column type from '{}' to '{}' may require a table rewrite \ + and AccessExclusiveLock.", + old.data_type, new.data_type + )], + is_blocking: true, + }), + } + } + + // Nullability change + if old.is_nullable != new.is_nullable { + let blocking = !new.is_nullable; + match dialect { + SqlDialect::MySql => { + let warning = if new.is_nullable { + format!( + "Changing '{}' to NULL via MODIFY COLUMN may rebuild the table depending \ + on MySQL/MariaDB version and storage engine.", + new.name + ) + } else { + format!( + "Changing '{}' to NOT NULL via MODIFY COLUMN can fail if existing rows \ + contain NULL values.", + new.name + ) + }; + stmts.push(MigrationStatement { + sql: format!( + "ALTER TABLE {} MODIFY COLUMN {};", + quote_ident(table, dialect), + column_definition_sql(new, dialect) + ), + warnings: vec![warning], + is_blocking: blocking, + }); + } + SqlDialect::Sqlite => { + stmts.push(MigrationStatement { + sql: format!( + "-- manual migration required for nullability change on {}.{}", + quote_ident(table, dialect), + quote_ident(&new.name, dialect) + ), + warnings: vec![format!( + "SQLite does not support ALTER COLUMN nullability directly for '{}'. \ + Recreate table '{table}' with the desired column definition.", + new.name + )], + is_blocking: blocking, + }); + } + _ => { + if new.is_nullable { + stmts.push(MigrationStatement { + sql: format!( + "ALTER TABLE {} ALTER COLUMN {} DROP NOT NULL;", + quote_ident(table, dialect), + quote_ident(&new.name, dialect) + ), + warnings: Vec::new(), + is_blocking: false, + }); + } else { + stmts.push(MigrationStatement { + sql: format!( + "ALTER TABLE {} ALTER COLUMN {} SET NOT NULL;", + quote_ident(table, dialect), + quote_ident(&new.name, dialect) + ), + warnings: vec![format!( + "SET NOT NULL on '{}' will scan the entire table to verify no NULLs exist. \ + This acquires AccessExclusiveLock.", + new.name + )], + is_blocking: true, + }); + } + } + } + } + + // Default change — non-blocking (metadata-only on modern PG) + if old.default != new.default { + match &new.default { + Some(default) => { + stmts.push(MigrationStatement { + sql: format!( + "ALTER TABLE {} ALTER COLUMN {} SET DEFAULT {default};", + quote_ident(table, dialect), + quote_ident(&new.name, dialect) + ), + warnings: Vec::new(), + is_blocking: false, + }); + } + None => { + stmts.push(MigrationStatement { + sql: format!( + "ALTER TABLE {} ALTER COLUMN {} DROP DEFAULT;", + quote_ident(table, dialect), + quote_ident(&new.name, dialect) + ), + warnings: Vec::new(), + is_blocking: false, + }); + } + } + } + } + + stmts +} diff --git a/src/model.rs b/src/model.rs index 0866ea7..4aee621 100644 --- a/src/model.rs +++ b/src/model.rs @@ -77,7 +77,7 @@ impl Index { } } -/// A table constraint (FK, unique, check). +/// A table constraint (PK, FK, unique, check). #[derive(Debug, Clone, PartialEq, Eq, Serialize, serde::Deserialize)] pub struct Constraint { pub name: String, @@ -87,6 +87,9 @@ pub struct Constraint { #[derive(Debug, Clone, PartialEq, Eq, Serialize, serde::Deserialize)] pub enum ConstraintKind { + PrimaryKey { + columns: Vec, + }, ForeignKey { columns: Vec, ref_table: String, @@ -105,6 +108,9 @@ pub enum ConstraintKind { impl Constraint { pub fn definition(&self) -> String { match &self.kind { + ConstraintKind::PrimaryKey { columns } => { + format!("PRIMARY KEY ({})", columns.join(", ")) + } ConstraintKind::ForeignKey { columns, ref_table, @@ -153,25 +159,6 @@ pub struct Sequence { pub max_value: i64, } -/// A database trigger. -#[derive(Debug, Clone, PartialEq, Eq, Serialize)] -pub struct Trigger { - pub name: String, - pub table_name: String, - pub timing: String, - pub event: String, - pub body: String, -} - -/// A stored function or procedure. -#[derive(Debug, Clone, PartialEq, Eq, Serialize)] -pub struct Function { - pub name: String, - pub language: String, - pub return_type: String, - pub body: String, -} - /// Deserializable schema for snapshot import. #[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, serde::Deserialize)] pub struct SchemaSnapshot { diff --git a/src/output.rs b/src/output.rs index e823a61..a2e78c0 100644 --- a/src/output.rs +++ b/src/output.rs @@ -46,6 +46,18 @@ pub fn print_diff(diff: &SchemaDiff) { println!(); } + // Renamed tables + for rename in &diff.renamed_tables { + println!( + "{}", + format!( + "↳ table: {} → {} (rename, {} confidence)", + rename.old_name, rename.new_name, rename.confidence + ) + .yellow() + ); + } + // Modified tables for table_diff in &diff.modified_tables { println!("{}", format!("~ table: {}", table_diff.table_name).yellow()); @@ -112,6 +124,17 @@ pub fn print_diff(diff: &SchemaDiff) { } fn print_table_diff(diff: &TableDiff) { + for rename in &diff.renamed_columns { + println!( + "{}", + format!( + " ↳ column {} → {} (rename, {} confidence)", + rename.old.name, rename.new.name, rename.confidence + ) + .yellow() + ); + } + for col in &diff.added_columns { println!( "{}", @@ -302,9 +325,14 @@ pub fn print_migration_explained(statements: &[MigrationStatement]) { } /// Format migration statements as plain SQL (no colors). -pub fn migration_to_sql(statements: &[MigrationStatement]) -> String { +pub fn migration_to_sql(statements: &[MigrationStatement], wrap_transaction: bool) -> String { let mut lines = Vec::new(); + if wrap_transaction { + lines.push("BEGIN;".to_string()); + lines.push(String::new()); + } + for stmt in statements { for warning in &stmt.warnings { lines.push(format!("-- !! {warning}")); @@ -313,6 +341,10 @@ pub fn migration_to_sql(statements: &[MigrationStatement]) -> String { lines.push(String::new()); } + if wrap_transaction { + lines.push("COMMIT;".to_string()); + } + lines.join("\n") } @@ -506,3 +538,43 @@ pub fn print_summary(diff: &SchemaDiff) { println!("{}", summary.bold()); } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn migration_to_sql_with_transaction() { + let stmts = vec![MigrationStatement { + sql: "ALTER TABLE users ADD COLUMN email text;".into(), + warnings: Vec::new(), + is_blocking: false, + }]; + + let sql = migration_to_sql(&stmts, true); + assert!(sql.starts_with("BEGIN;"), "Expected BEGIN at start: {sql}"); + assert!(sql.contains("ALTER TABLE users ADD COLUMN email text;")); + assert!(sql.trim_end().ends_with("COMMIT;"), "Expected COMMIT at end: {sql}"); + } + + #[test] + fn migration_to_sql_without_transaction() { + let stmts = vec![MigrationStatement { + sql: "ALTER TABLE users ADD COLUMN email text;".into(), + warnings: Vec::new(), + is_blocking: false, + }]; + + let sql = migration_to_sql(&stmts, false); + assert!(!sql.contains("BEGIN;")); + assert!(!sql.contains("COMMIT;")); + assert!(sql.contains("ALTER TABLE users ADD COLUMN email text;")); + } + + #[test] + fn migration_to_sql_empty_with_transaction() { + let sql = migration_to_sql(&[], true); + assert!(sql.contains("BEGIN;")); + assert!(sql.contains("COMMIT;")); + } +} From 2d2f3c9aed306254a52df11d952c66992d3af92b Mon Sep 17 00:00:00 2001 From: nikita42 <13642481+rekurt@users.noreply.github.com> Date: Wed, 8 Apr 2026 15:25:27 +0300 Subject: [PATCH 2/5] fix: address round 2 review findings - fix(migration): rollback medium-confidence renames now commented out (matching forward migration) - fix(output): DiffSummary now includes tables_renamed and columns_renamed counts - fix(commands): emit warning when --concurrently used with non-PostgreSQL dialect --- src/commands/diff.rs | 8 ++++++++ src/migration/mod.rs | 26 +++++++++++++++++++------- src/output.rs | 16 ++++++++++++++++ 3 files changed, 43 insertions(+), 7 deletions(-) diff --git a/src/commands/diff.rs b/src/commands/diff.rs index 7490dad..45e0de3 100644 --- a/src/commands/diff.rs +++ b/src/commands/diff.rs @@ -105,6 +105,14 @@ pub async fn run_diff(params: DiffParams) -> Result<(), ExitCode> { } }; + if params.concurrently && !matches!(migration_dialect, SqlDialect::Postgres) { + eprintln!( + "Warning: --concurrently only affects PostgreSQL. \ + Ignoring for {:?} dialect. Transaction wrapping is also disabled.", + migration_dialect + ); + } + let up_statements = migration::generate_migration(&diff, migration_dialect, params.concurrently); let down_statements = migration::generate_rollback(&diff, migration_dialect, params.concurrently); let format = params.resolve_format(&cfg.output.format); diff --git a/src/migration/mod.rs b/src/migration/mod.rs index d70db2c..194d6d6 100644 --- a/src/migration/mod.rs +++ b/src/migration/mod.rs @@ -590,13 +590,19 @@ pub fn generate_rollback(diff: &SchemaDiff, dialect: SqlDialect, _concurrently: // 16c. Reverse column renames (before re-adding constraints that reference old names) for table_diff in &diff.modified_tables { for rename in &table_diff.renamed_columns { + let raw_sql = rename_column_sql( + &table_diff.table_name, + &rename.new.name, + &rename.old.name, + dialect, + ); + let is_medium = matches!(rename.confidence, crate::diff::RenameConfidence::Medium); statements.push(MigrationStatement { - sql: rename_column_sql( - &table_diff.table_name, - &rename.new.name, - &rename.old.name, - dialect, - ), + sql: if is_medium { + format!("-- {raw_sql} -- uncomment after manual verification") + } else { + raw_sql + }, warnings: Vec::new(), is_blocking: false, }); @@ -605,8 +611,14 @@ pub fn generate_rollback(diff: &SchemaDiff, dialect: SqlDialect, _concurrently: // 16d. Reverse table renames (before re-adding constraints that reference old table names) for rename in &diff.renamed_tables { + let raw_sql = rename_table_sql(&rename.new_name, &rename.old_name, dialect); + let is_medium = matches!(rename.confidence, crate::diff::RenameConfidence::Medium); statements.push(MigrationStatement { - sql: rename_table_sql(&rename.new_name, &rename.old_name, dialect), + sql: if is_medium { + format!("-- {raw_sql} -- uncomment after manual verification") + } else { + raw_sql + }, warnings: Vec::new(), is_blocking: false, }); diff --git a/src/output.rs b/src/output.rs index a2e78c0..ae0d87c 100644 --- a/src/output.rs +++ b/src/output.rs @@ -353,10 +353,12 @@ pub fn migration_to_sql(statements: &[MigrationStatement], wrap_transaction: boo pub struct DiffSummary { pub tables_added: usize, pub tables_removed: usize, + pub tables_renamed: usize, pub tables_modified: usize, pub tables_unchanged: usize, pub columns_added: usize, pub columns_removed: usize, + pub columns_renamed: usize, pub columns_modified: usize, pub indexes_added: usize, pub indexes_removed: usize, @@ -446,13 +448,21 @@ pub fn diff_summary(diff: &SchemaDiff) -> DiffSummary { .map(|t| t.constraints.len()) .sum::(); + let columns_renamed: usize = diff + .modified_tables + .iter() + .map(|t| t.renamed_columns.len()) + .sum(); + DiffSummary { tables_added: diff.added_tables.len(), tables_removed: diff.removed_tables.len(), + tables_renamed: diff.renamed_tables.len(), tables_modified: diff.modified_tables.len(), tables_unchanged: diff.unchanged_tables.len(), columns_added, columns_removed, + columns_renamed, columns_modified, indexes_added, indexes_removed, @@ -485,6 +495,9 @@ pub fn print_summary(diff: &SchemaDiff) { if s.tables_removed > 0 { parts.push(format!("{} table(s) removed", s.tables_removed)); } + if s.tables_renamed > 0 { + parts.push(format!("{} table(s) renamed", s.tables_renamed)); + } if s.tables_modified > 0 { parts.push(format!("{} table(s) modified", s.tables_modified)); } @@ -496,6 +509,9 @@ pub fn print_summary(diff: &SchemaDiff) { if s.columns_removed > 0 { detail_parts.push(format!("{} columns removed", s.columns_removed)); } + if s.columns_renamed > 0 { + detail_parts.push(format!("{} columns renamed", s.columns_renamed)); + } if s.columns_modified > 0 { detail_parts.push(format!("{} columns altered", s.columns_modified)); } From 06be9412468264044340dee5d191107a20c26ae5 Mon Sep 17 00:00:00 2001 From: nikita42 <13642481+rekurt@users.noreply.github.com> Date: Wed, 8 Apr 2026 15:36:38 +0300 Subject: [PATCH 3/5] fix: preserve index/constraint diffs for renamed tables Table rename detection removed matched tables from added/removed sets without diffing non-column properties, causing index and constraint changes to be silently dropped from migration output. --- src/diff.rs | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 57 insertions(+), 2 deletions(-) diff --git a/src/diff.rs b/src/diff.rs index b9729ff..0b8121c 100644 --- a/src/diff.rs +++ b/src/diff.rs @@ -172,11 +172,22 @@ pub fn diff_schemas_with_options( // Detect table renames: match removed -> added by identical column sets if detect_renames { - detect_table_renames( + let rename_pairs = detect_table_renames( &mut removed_tables, &mut added_tables, &mut renamed_tables, ); + + // Renamed tables may still differ in indexes/constraints — diff them + // and emit modification entries under the new name so that migration + // generation picks up these changes after the RENAME TABLE. + for (old_table, new_table) in &rename_pairs { + let table_diff = + diff_tables(&new_table.name, old_table, new_table, detect_renames); + if !table_diff.is_empty() { + modified_tables.push(table_diff); + } + } } // Tables in both -> compare @@ -363,11 +374,13 @@ fn detect_column_renames( } /// Detect table renames by matching removed tables to added tables with identical column sets. +/// Returns the matched `(old_table, new_table)` pairs so the caller can diff non-column +/// properties (indexes, constraints) that may still differ after the rename. fn detect_table_renames( removed: &mut Vec
, added: &mut Vec
, renames: &mut Vec, -) { +) -> Vec<(Table, Table)> { let mut matched_removed = Vec::new(); let mut matched_added = Vec::new(); @@ -408,6 +421,13 @@ fn detect_table_renames( } } + // Collect matched pairs before removing from the vectors. + let pairs: Vec<(Table, Table)> = matched_removed + .iter() + .zip(matched_added.iter()) + .map(|(&ri, &ai)| (removed[ri].clone(), added[ai].clone())) + .collect(); + matched_removed.sort_unstable(); matched_added.sort_unstable(); for &i in matched_removed.iter().rev() { @@ -416,6 +436,8 @@ fn detect_table_renames( for &i in matched_added.iter().rev() { added.remove(i); } + + pairs } fn diff_views( @@ -1199,6 +1221,39 @@ mod tests { assert_eq!(diff.added_tables.len(), 1); } + #[test] + fn table_rename_with_index_diff_emits_modified() { + let mut left = Schema::new(); + let mut t = Table::new("old_name"); + t.columns + .insert("id".into(), make_column("id", "integer", false, None)); + t.indexes.insert( + "idx_old".into(), + make_index("idx_old", "old_name", &["id"], false), + ); + left.tables.insert("old_name".into(), t); + + let mut right = Schema::new(); + let mut t = Table::new("new_name"); + t.columns + .insert("id".into(), make_column("id", "integer", false, None)); + t.indexes.insert( + "idx_new".into(), + make_index("idx_new", "new_name", &["id"], true), + ); + right.tables.insert("new_name".into(), t); + + let diff = diff_schemas_with_options(&left, &right, true); + assert_eq!(diff.renamed_tables.len(), 1); + assert_eq!(diff.renamed_tables[0].old_name, "old_name"); + assert_eq!(diff.renamed_tables[0].new_name, "new_name"); + // The index difference must appear in modified_tables, not be silently dropped. + assert_eq!(diff.modified_tables.len(), 1); + assert_eq!(diff.modified_tables[0].table_name, "new_name"); + assert_eq!(diff.modified_tables[0].removed_indexes.len(), 1); + assert_eq!(diff.modified_tables[0].added_indexes.len(), 1); + } + #[test] fn table_rename_ambiguous_two_candidates() { let mut left = Schema::new(); From 9c760ee86dd09568c0c5db91aefc51cfd9a89d02 Mon Sep 17 00:00:00 2001 From: nikita42 <13642481+rekurt@users.noreply.github.com> Date: Wed, 8 Apr 2026 15:46:24 +0300 Subject: [PATCH 4/5] fix: reverse all object types in rollback CI classification and require bijective table rename matching - CI report now correctly flips SQL patterns and blocking defaults for columns, indexes, and constraints in reverse (rollback) mode, not just tables. Previously, rollback lookups missed statements and used wrong default is_blocking values, underreporting blocking rollback steps. - Table rename detector now requires bijective (1-to-1) matching: a rename is only accepted when the removed table has exactly one added candidate AND that added table is claimed by only one removed table. Previously, iteration order could pick an arbitrary rename when two removed tables shared the same column shape. --- src/ci.rs | 149 +++++++++++++++++++++++++++++++++++----------------- src/diff.rs | 84 +++++++++++++++++------------ 2 files changed, 152 insertions(+), 81 deletions(-) diff --git a/src/ci.rs b/src/ci.rs index 38352cb..b9fde8d 100644 --- a/src/ci.rs +++ b/src/ci.rs @@ -140,36 +140,48 @@ impl CiReport { } for col in &table_diff.added_columns { + // In reverse mode, added columns are DROP'd (rollback of ADD) + let (sql_kw, label, default_blocking) = if reverse { + ("DROP COLUMN", "DROP", true) + } else { + ("ADD COLUMN", "ADD", false) + }; let stmt = statements.iter().find(|s| { - s.sql.contains("ADD COLUMN") + s.sql.contains(sql_kw) && s.sql.contains(&col.name) && s.sql.contains(tname.as_str()) }); - let blocking = stmt.map(|s| s.is_blocking).unwrap_or(false); + let blocking = stmt.map(|s| s.is_blocking).unwrap_or(default_blocking); changes.push(CiChange { - change_type: "ADD".to_string(), + change_type: label.to_string(), object: "COLUMN".to_string(), table: tname.clone(), name: col.name.clone(), - description: format!("ADD COLUMN {}", col.definition()), + description: format!("{label} COLUMN {}", col.definition()), is_blocking: blocking, sql: stmt.map(|s| s.sql.clone()).unwrap_or_default(), }); } for col in &table_diff.removed_columns { + // In reverse mode, removed columns are ADD'd back (rollback of DROP) + let (sql_kw, label, default_blocking) = if reverse { + ("ADD COLUMN", "ADD", false) + } else { + ("DROP COLUMN", "DROP", true) + }; let stmt = statements.iter().find(|s| { - s.sql.contains("DROP COLUMN") + s.sql.contains(sql_kw) && s.sql.contains(&col.name) && s.sql.contains(tname.as_str()) }); - let blocking = stmt.map(|s| s.is_blocking).unwrap_or(true); + let blocking = stmt.map(|s| s.is_blocking).unwrap_or(default_blocking); changes.push(CiChange { - change_type: "DROP".to_string(), + change_type: label.to_string(), object: "COLUMN".to_string(), table: tname.clone(), name: col.name.clone(), - description: format!("DROP COLUMN {}", col.name), + description: format!("{label} COLUMN {}", col.name), is_blocking: blocking, sql: stmt.map(|s| s.sql.clone()).unwrap_or_default(), }); @@ -201,62 +213,95 @@ impl CiReport { } for idx in &table_diff.added_indexes { + // In reverse mode, added indexes are DROP'd + let (sql_kw, label, default_blocking) = if reverse { + ("DROP INDEX", "DROP", true) + } else { + ("CREATE", "ADD", true) + }; let stmt = statements .iter() - .find(|s| s.sql.contains("CREATE") && s.sql.contains(&idx.name)); - let blocking = stmt.map(|s| s.is_blocking).unwrap_or(true); + .find(|s| s.sql.contains(sql_kw) && s.sql.contains(&idx.name)); + let blocking = stmt.map(|s| s.is_blocking).unwrap_or(default_blocking); changes.push(CiChange { - change_type: "ADD".to_string(), + change_type: label.to_string(), object: "INDEX".to_string(), table: tname.clone(), name: idx.name.clone(), - description: format!("ADD INDEX {}", idx.definition()), + description: format!("{label} INDEX {}", idx.definition()), is_blocking: blocking, sql: stmt.map(|s| s.sql.clone()).unwrap_or_default(), }); } for idx in &table_diff.removed_indexes { + // In reverse mode, removed indexes are re-CREATE'd + let (sql_kw, label, default_blocking) = if reverse { + ("CREATE", "ADD", true) + } else { + ("DROP INDEX", "DROP", true) + }; + let stmt = statements + .iter() + .find(|s| s.sql.contains(sql_kw) && s.sql.contains(&idx.name)); + let blocking = stmt.map(|s| s.is_blocking).unwrap_or(default_blocking); changes.push(CiChange { - change_type: "DROP".to_string(), + change_type: label.to_string(), object: "INDEX".to_string(), table: tname.clone(), name: idx.name.clone(), - description: format!("DROP INDEX {}", idx.name), - is_blocking: true, - sql: statements - .iter() - .find(|s| s.sql.contains("DROP INDEX") && s.sql.contains(&idx.name)) - .map(|s| s.sql.clone()) - .unwrap_or_default(), + description: format!("{label} INDEX {}", idx.name), + is_blocking: blocking, + sql: stmt.map(|s| s.sql.clone()).unwrap_or_default(), }); } // Constraints for c in &table_diff.added_constraints { - let stmt = statements - .iter() - .find(|s| s.sql.contains("ADD CONSTRAINT") && s.sql.contains(&c.name)); + // In reverse mode, added constraints are DROP'd + let (label, default_blocking) = if reverse { + ("DROP", true) + } else { + ("ADD", true) + }; + let stmt = if reverse { + statements + .iter() + .find(|s| s.sql.contains("DROP CONSTRAINT") && s.sql.contains(&c.name)) + .or_else(|| { + statements.iter().find(|s| { + s.sql.contains("DROP FOREIGN KEY") && s.sql.contains(&c.name) + }) + }) + } else { + statements + .iter() + .find(|s| s.sql.contains("ADD CONSTRAINT") && s.sql.contains(&c.name)) + }; changes.push(CiChange { - change_type: "ADD".to_string(), + change_type: label.to_string(), object: "CONSTRAINT".to_string(), table: tname.clone(), name: c.name.clone(), - description: format!("ADD CONSTRAINT {} {}", c.name, c.definition()), - is_blocking: stmt.map(|s| s.is_blocking).unwrap_or(true), + description: format!("{label} CONSTRAINT {} {}", c.name, c.definition()), + is_blocking: stmt.map(|s| s.is_blocking).unwrap_or(default_blocking), sql: stmt.map(|s| s.sql.clone()).unwrap_or_default(), }); } for c in &table_diff.removed_constraints { - changes.push(CiChange { - change_type: "DROP".to_string(), - object: "CONSTRAINT".to_string(), - table: tname.clone(), - name: c.name.clone(), - description: format!("DROP CONSTRAINT {}", c.name), - is_blocking: true, - sql: statements + // In reverse mode, removed constraints are ADD'd back + let (label, default_blocking) = if reverse { + ("ADD", true) + } else { + ("DROP", true) + }; + let stmt = if reverse { + statements + .iter() + .find(|s| s.sql.contains("ADD CONSTRAINT") && s.sql.contains(&c.name)) + } else { + statements .iter() .find(|s| s.sql.contains("DROP CONSTRAINT") && s.sql.contains(&c.name)) .or_else(|| { @@ -264,8 +309,16 @@ impl CiReport { s.sql.contains("DROP FOREIGN KEY") && s.sql.contains(&c.name) }) }) - .map(|s| s.sql.clone()) - .unwrap_or_default(), + }; + let blocking = stmt.map(|s| s.is_blocking).unwrap_or(default_blocking); + changes.push(CiChange { + change_type: label.to_string(), + object: "CONSTRAINT".to_string(), + table: tname.clone(), + name: c.name.clone(), + description: format!("{label} CONSTRAINT {}", c.name), + is_blocking: blocking, + sql: stmt.map(|s| s.sql.clone()).unwrap_or_default(), }); } } @@ -273,22 +326,22 @@ impl CiReport { // Views for view in &diff.added_views { changes.push(CiChange { - change_type: "ADD".to_string(), + change_type: add_label.to_string(), object: "VIEW".to_string(), table: String::new(), name: view.name.clone(), - description: format!("ADD VIEW {}", view.name), + description: format!("{add_label} VIEW {}", view.name), is_blocking: false, sql: String::new(), }); } for view in &diff.removed_views { changes.push(CiChange { - change_type: "DROP".to_string(), + change_type: drop_label.to_string(), object: "VIEW".to_string(), table: String::new(), name: view.name.clone(), - description: format!("DROP VIEW {}", view.name), + description: format!("{drop_label} VIEW {}", view.name), is_blocking: false, sql: String::new(), }); @@ -308,22 +361,22 @@ impl CiReport { // Enums for e in &diff.added_enums { changes.push(CiChange { - change_type: "ADD".to_string(), + change_type: add_label.to_string(), object: "ENUM".to_string(), table: String::new(), name: e.name.clone(), - description: format!("ADD ENUM {}", e.name), + description: format!("{add_label} ENUM {}", e.name), is_blocking: false, sql: String::new(), }); } for e in &diff.removed_enums { changes.push(CiChange { - change_type: "DROP".to_string(), + change_type: drop_label.to_string(), object: "ENUM".to_string(), table: String::new(), name: e.name.clone(), - description: format!("DROP ENUM {}", e.name), + description: format!("{drop_label} ENUM {}", e.name), is_blocking: false, sql: String::new(), }); @@ -343,22 +396,22 @@ impl CiReport { // Sequences for s in &diff.added_sequences { changes.push(CiChange { - change_type: "ADD".to_string(), + change_type: add_label.to_string(), object: "SEQUENCE".to_string(), table: String::new(), name: s.name.clone(), - description: format!("ADD SEQUENCE {}", s.name), + description: format!("{add_label} SEQUENCE {}", s.name), is_blocking: false, sql: String::new(), }); } for s in &diff.removed_sequences { changes.push(CiChange { - change_type: "DROP".to_string(), + change_type: drop_label.to_string(), object: "SEQUENCE".to_string(), table: String::new(), name: s.name.clone(), - description: format!("DROP SEQUENCE {}", s.name), + description: format!("{drop_label} SEQUENCE {}", s.name), is_blocking: false, sql: String::new(), }); diff --git a/src/diff.rs b/src/diff.rs index 0b8121c..d38d6e8 100644 --- a/src/diff.rs +++ b/src/diff.rs @@ -381,44 +381,62 @@ fn detect_table_renames( added: &mut Vec
, renames: &mut Vec, ) -> Vec<(Table, Table)> { + // Phase 1: collect all candidate mappings (removed_idx -> Vec<(added_idx, confidence)>) + let all_candidates: Vec> = removed + .iter() + .map(|rem| { + added + .iter() + .enumerate() + .filter_map(|(ai, add)| { + // High confidence: identical column definitions + if rem.columns == add.columns { + return Some((ai, RenameConfidence::High)); + } + // Medium confidence: same column count, ≥80% column name overlap + if rem.columns.len() == add.columns.len() && !rem.columns.is_empty() { + let overlap = rem + .columns + .keys() + .filter(|k| add.columns.contains_key(*k)) + .count(); + if overlap * 100 / rem.columns.len() >= 80 { + return Some((ai, RenameConfidence::Medium)); + } + } + None + }) + .collect() + }) + .collect(); + + // Phase 2: require bijective matching — accept only when both the removed + // table has exactly one candidate AND that added table is claimed by only + // one removed table. let mut matched_removed = Vec::new(); let mut matched_added = Vec::new(); - for (ri, rem) in removed.iter().enumerate() { - let candidates: Vec<(usize, RenameConfidence)> = added + for (ri, candidates) in all_candidates.iter().enumerate() { + if candidates.len() != 1 { + continue; + } + let (ai, confidence) = &candidates[0]; + // Check reverse uniqueness: this added table must be a sole candidate + // for only this removed table. + let claimants = all_candidates .iter() - .enumerate() - .filter(|(ai, _)| !matched_added.contains(ai)) - .filter_map(|(ai, add)| { - // High confidence: identical column definitions - if rem.columns == add.columns { - return Some((ai, RenameConfidence::High)); - } - // Medium confidence: same column count, ≥80% column name overlap - if rem.columns.len() == add.columns.len() && !rem.columns.is_empty() { - let overlap = rem - .columns - .keys() - .filter(|k| add.columns.contains_key(*k)) - .count(); - if overlap * 100 / rem.columns.len() >= 80 { - return Some((ai, RenameConfidence::Medium)); - } - } - None - }) - .collect(); - - if candidates.len() == 1 { - let (ai, confidence) = &candidates[0]; - matched_removed.push(ri); - matched_added.push(*ai); - renames.push(TableRename { - old_name: rem.name.clone(), - new_name: added[*ai].name.clone(), - confidence: *confidence, - }); + .filter(|cands| cands.iter().any(|(a, _)| a == ai)) + .count(); + if claimants != 1 { + continue; } + matched_removed.push(ri); + matched_added.push(*ai); + renames.push(TableRename { + old_name: removed[ri].name.clone(), + new_name: added[*ai].name.clone(), + confidence: *confidence, + }); } // Collect matched pairs before removing from the vectors. From 45171f2fee0099e9259837f104df8ed09038d606 Mon Sep 17 00:00:00 2001 From: nikita42 <13642481+rekurt@users.noreply.github.com> Date: Wed, 8 Apr 2026 16:02:20 +0300 Subject: [PATCH 5/5] fix: rebase removed indexes/constraints to new table name after rename detection When a table rename is detected with concurrent index/constraint changes, removed_indexes and removed_constraints retained the old table name. This caused DROP statements to target the pre-rename name, breaking forward migrations on MySQL (DROP INDEX ... ON old_table) and PostgreSQL (ALTER TABLE old_table DROP CONSTRAINT ...). --- src/diff.rs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/diff.rs b/src/diff.rs index d38d6e8..0be736d 100644 --- a/src/diff.rs +++ b/src/diff.rs @@ -182,8 +182,16 @@ pub fn diff_schemas_with_options( // and emit modification entries under the new name so that migration // generation picks up these changes after the RENAME TABLE. for (old_table, new_table) in &rename_pairs { - let table_diff = + let mut table_diff = diff_tables(&new_table.name, old_table, new_table, detect_renames); + // Rebase removed objects to the new table name so that DROP + // statements generated after RENAME TABLE target the correct name. + for idx in &mut table_diff.removed_indexes { + idx.table_name = new_table.name.clone(); + } + for c in &mut table_diff.removed_constraints { + c.table_name = new_table.name.clone(); + } if !table_diff.is_empty() { modified_tables.push(table_diff); }