diff --git a/.changepacks/changepack_log_kZhbf9xXT865BdVdu7Dl6.json b/.changepacks/changepack_log_kZhbf9xXT865BdVdu7Dl6.json new file mode 100644 index 00000000..5c6f1b5a --- /dev/null +++ b/.changepacks/changepack_log_kZhbf9xXT865BdVdu7Dl6.json @@ -0,0 +1 @@ +{"changes":{"crates/vespertide-exporter/Cargo.toml":"Patch","crates/vespertide/Cargo.toml":"Patch","crates/vespertide-naming/Cargo.toml":"Patch","crates/vespertide-config/Cargo.toml":"Patch","crates/vespertide-macro/Cargo.toml":"Patch","crates/vespertide-planner/Cargo.toml":"Patch","crates/vespertide-query/Cargo.toml":"Patch","crates/vespertide-cli/Cargo.toml":"Patch","crates/vespertide-core/Cargo.toml":"Patch","crates/vespertide-loader/Cargo.toml":"Patch"},"note":"Fix index column issue","date":"2026-04-13T08:37:23.992645500Z"} \ No newline at end of file diff --git a/Cargo.lock b/Cargo.lock index e3227970..178ee482 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3713,7 +3713,7 @@ dependencies = [ [[package]] name = "vespertide" -version = "0.1.55" +version = "0.1.56" dependencies = [ "sea-orm", "tokio", @@ -3723,7 +3723,7 @@ dependencies = [ [[package]] name = "vespertide-cli" -version = "0.1.55" +version = "0.1.56" dependencies = [ "anyhow", "assert_cmd", @@ -3752,7 +3752,7 @@ dependencies = [ [[package]] name = "vespertide-config" -version = "0.1.55" +version = "0.1.56" dependencies = [ "clap", "schemars", @@ -3762,7 +3762,7 @@ dependencies = [ [[package]] name = "vespertide-core" -version = "0.1.55" +version = "0.1.56" dependencies = [ "rstest", "schemars", @@ -3774,7 +3774,7 @@ dependencies = [ [[package]] name = "vespertide-exporter" -version = "0.1.55" +version = "0.1.56" dependencies = [ "insta", "rstest", @@ -3786,7 +3786,7 @@ dependencies = [ [[package]] name = "vespertide-loader" -version = "0.1.55" +version = "0.1.56" dependencies = [ "anyhow", "rstest", @@ -3801,7 +3801,7 @@ dependencies = [ [[package]] name = "vespertide-macro" -version = "0.1.55" +version = "0.1.56" dependencies = [ "proc-macro2", "runtime-macros", @@ -3816,11 +3816,11 @@ dependencies = [ [[package]] name = "vespertide-naming" -version = "0.1.55" +version = "0.1.56" [[package]] name = "vespertide-planner" -version = "0.1.55" +version = "0.1.56" dependencies = [ "insta", "rstest", @@ -3831,7 +3831,7 @@ dependencies = [ [[package]] name = "vespertide-query" -version = "0.1.55" +version = "0.1.56" dependencies = [ "insta", "rstest", diff --git a/crates/vespertide-planner/src/diff.rs b/crates/vespertide-planner/src/diff.rs index 201623aa..0aa75f0f 100644 --- a/crates/vespertide-planner/src/diff.rs +++ b/crates/vespertide-planner/src/diff.rs @@ -561,13 +561,22 @@ pub fn diff_schemas(from: &[TableDef], to: &[TableDef]) -> Result, } +/// Extract the target table name from any migration action. +/// Returns `None` for `RawSql` (no table) and `RenameTable` (ambiguous). +fn action_target_table(action: &MigrationAction) -> Option<&str> { + match action { + MigrationAction::CreateTable { table, .. } + | MigrationAction::DeleteTable { table } + | MigrationAction::AddColumn { table, .. } + | MigrationAction::RenameColumn { table, .. } + | MigrationAction::DeleteColumn { table, .. } + | MigrationAction::ModifyColumnType { table, .. } + | MigrationAction::ModifyColumnNullable { table, .. } + | MigrationAction::ModifyColumnDefault { table, .. } + | MigrationAction::ModifyColumnComment { table, .. } + | MigrationAction::AddConstraint { table, .. } + | MigrationAction::RemoveConstraint { table, .. } => Some(table), + MigrationAction::RenameTable { .. } | MigrationAction::RawSql { .. } => None, + } +} + pub fn build_plan_queries( plan: &MigrationPlan, current_schema: &[TableDef], @@ -27,8 +46,13 @@ pub fn build_plan_queries( // but haven't been physically created as DB indexes yet. // Without this, a temp table rebuild would recreate these indexes prematurely, // causing "index already exists" errors when their AddConstraint actions run later. + // + // This applies to ANY action that may trigger a SQLite temp table rebuild + // (AddColumn with NOT NULL, ModifyColumn*, DeleteColumn, AddConstraint FK/PK/Check, + // RemoveConstraint), not just AddConstraint. + let action_table = action_target_table(action); let pending_constraints: Vec = - if let MigrationAction::AddConstraint { table, .. } = action { + if let Some(table) = action_table { plan.actions[i + 1..] .iter() .filter_map(|a| { @@ -765,4 +789,102 @@ mod tests { assert_snapshot!(sql); }); } + + // ── Two NOT NULL AddColumns with inline index + AddConstraint ──────── + + /// Regression test: when two NOT NULL columns with inline `index: true` + /// are added sequentially, the second AddColumn triggers a SQLite temp + /// table rebuild. At that point the evolving schema already contains the + /// first column's index (from normalization). Without pending constraint + /// awareness, the rebuild recreates that index, and the later + /// AddConstraint for the same index fails with "index already exists". + #[rstest] + #[case::postgres("postgres", DatabaseBackend::Postgres)] + #[case::mysql("mysql", DatabaseBackend::MySql)] + #[case::sqlite("sqlite", DatabaseBackend::Sqlite)] + fn test_two_not_null_add_columns_with_inline_index_no_duplicate( + #[case] label: &str, + #[case] backend: DatabaseBackend, + ) { + use vespertide_core::DefaultValue; + use vespertide_core::schema::str_or_bool::StrOrBoolOrArray; + + let schema = vec![TableDef { + name: "article".into(), + description: None, + columns: vec![ + col("id", ColumnType::Simple(SimpleColumnType::Integer)), + col("title", ColumnType::Simple(SimpleColumnType::Text)), + ], + constraints: vec![], + }]; + + let plan = MigrationPlan { + id: String::new(), + comment: None, + created_at: None, + version: 1, + actions: vec![ + // 1. Add NOT NULL column with inline index + MigrationAction::AddColumn { + table: "article".into(), + column: Box::new(ColumnDef { + name: "category_pinned".into(), + r#type: ColumnType::Simple(SimpleColumnType::Boolean), + nullable: false, + default: Some(DefaultValue::Bool(false)), + comment: None, + primary_key: None, + unique: None, + index: Some(StrOrBoolOrArray::Bool(true)), + foreign_key: None, + }), + fill_with: None, + }, + // 2. Add another NOT NULL column with inline index + MigrationAction::AddColumn { + table: "article".into(), + column: Box::new(ColumnDef { + name: "main_pinned".into(), + r#type: ColumnType::Simple(SimpleColumnType::Boolean), + nullable: false, + default: Some(DefaultValue::Bool(false)), + comment: None, + primary_key: None, + unique: None, + index: Some(StrOrBoolOrArray::Bool(true)), + foreign_key: None, + }), + fill_with: None, + }, + // 3. AddConstraint for main_pinned index + MigrationAction::AddConstraint { + table: "article".into(), + constraint: TableConstraint::Index { + name: None, + columns: vec!["main_pinned".into()], + }, + }, + // 4. AddConstraint for category_pinned index + MigrationAction::AddConstraint { + table: "article".into(), + constraint: TableConstraint::Index { + name: None, + columns: vec!["category_pinned".into()], + }, + }, + ], + }; + + let result = build_plan_queries(&plan, &schema).unwrap(); + + // Core invariant: no duplicate indexes across actions + assert_no_duplicate_indexes_per_action(&result); + assert_no_orphan_duplicate_indexes(&result); + + let sql = collect_all_sql(&result, backend); + with_settings!({ snapshot_suffix => format!("two_not_null_inline_index_{}", label) }, { + assert_snapshot!(sql); + }); + } } diff --git a/crates/vespertide-query/src/snapshots/vespertide_query__builder__tests__two_not_null_add_columns_with_inline_index_no_duplicate@two_not_null_inline_index_mysql.snap b/crates/vespertide-query/src/snapshots/vespertide_query__builder__tests__two_not_null_add_columns_with_inline_index_no_duplicate@two_not_null_inline_index_mysql.snap new file mode 100644 index 00000000..9630dccf --- /dev/null +++ b/crates/vespertide-query/src/snapshots/vespertide_query__builder__tests__two_not_null_add_columns_with_inline_index_no_duplicate@two_not_null_inline_index_mysql.snap @@ -0,0 +1,15 @@ +--- +source: crates/vespertide-query/src/builder.rs +expression: sql +--- +-- Action 0: AddColumn { table: "article", column: ColumnDef { name: "category_pinned", type: Simple(Boolean), nullable: false, default: Some(Bool(false)), comment: None, primary_key: None, unique: None, index: Some(Bool(true)), foreign_key: None }, fill_with: None } +ALTER TABLE `article` ADD COLUMN `category_pinned` bool NOT NULL DEFAULT false + +-- Action 1: AddColumn { table: "article", column: ColumnDef { name: "main_pinned", type: Simple(Boolean), nullable: false, default: Some(Bool(false)), comment: None, primary_key: None, unique: None, index: Some(Bool(true)), foreign_key: None }, fill_with: None } +ALTER TABLE `article` ADD COLUMN `main_pinned` bool NOT NULL DEFAULT false + +-- Action 2: AddConstraint { table: "article", constraint: Index { name: None, columns: ["main_pinned"] } } +CREATE INDEX `ix_article__main_pinned` ON `article` (`main_pinned`) + +-- Action 3: AddConstraint { table: "article", constraint: Index { name: None, columns: ["category_pinned"] } } +CREATE INDEX `ix_article__category_pinned` ON `article` (`category_pinned`) diff --git a/crates/vespertide-query/src/snapshots/vespertide_query__builder__tests__two_not_null_add_columns_with_inline_index_no_duplicate@two_not_null_inline_index_postgres.snap b/crates/vespertide-query/src/snapshots/vespertide_query__builder__tests__two_not_null_add_columns_with_inline_index_no_duplicate@two_not_null_inline_index_postgres.snap new file mode 100644 index 00000000..0411e68c --- /dev/null +++ b/crates/vespertide-query/src/snapshots/vespertide_query__builder__tests__two_not_null_add_columns_with_inline_index_no_duplicate@two_not_null_inline_index_postgres.snap @@ -0,0 +1,15 @@ +--- +source: crates/vespertide-query/src/builder.rs +expression: sql +--- +-- Action 0: AddColumn { table: "article", column: ColumnDef { name: "category_pinned", type: Simple(Boolean), nullable: false, default: Some(Bool(false)), comment: None, primary_key: None, unique: None, index: Some(Bool(true)), foreign_key: None }, fill_with: None } +ALTER TABLE "article" ADD COLUMN "category_pinned" bool NOT NULL DEFAULT false + +-- Action 1: AddColumn { table: "article", column: ColumnDef { name: "main_pinned", type: Simple(Boolean), nullable: false, default: Some(Bool(false)), comment: None, primary_key: None, unique: None, index: Some(Bool(true)), foreign_key: None }, fill_with: None } +ALTER TABLE "article" ADD COLUMN "main_pinned" bool NOT NULL DEFAULT false + +-- Action 2: AddConstraint { table: "article", constraint: Index { name: None, columns: ["main_pinned"] } } +CREATE INDEX "ix_article__main_pinned" ON "article" ("main_pinned") + +-- Action 3: AddConstraint { table: "article", constraint: Index { name: None, columns: ["category_pinned"] } } +CREATE INDEX "ix_article__category_pinned" ON "article" ("category_pinned") diff --git a/crates/vespertide-query/src/snapshots/vespertide_query__builder__tests__two_not_null_add_columns_with_inline_index_no_duplicate@two_not_null_inline_index_sqlite.snap b/crates/vespertide-query/src/snapshots/vespertide_query__builder__tests__two_not_null_add_columns_with_inline_index_no_duplicate@two_not_null_inline_index_sqlite.snap new file mode 100644 index 00000000..38472387 --- /dev/null +++ b/crates/vespertide-query/src/snapshots/vespertide_query__builder__tests__two_not_null_add_columns_with_inline_index_no_duplicate@two_not_null_inline_index_sqlite.snap @@ -0,0 +1,21 @@ +--- +source: crates/vespertide-query/src/builder.rs +expression: sql +--- +-- Action 0: AddColumn { table: "article", column: ColumnDef { name: "category_pinned", type: Simple(Boolean), nullable: false, default: Some(Bool(false)), comment: None, primary_key: None, unique: None, index: Some(Bool(true)), foreign_key: None }, fill_with: None } +CREATE TABLE "article_temp" ( "id" integer, "title" text, "category_pinned" boolean NOT NULL DEFAULT false ); +INSERT INTO "article_temp" ("id", "title", "category_pinned") SELECT "id", "title", false AS "category_pinned" FROM "article"; +DROP TABLE "article"; +ALTER TABLE "article_temp" RENAME TO "article" + +-- Action 1: AddColumn { table: "article", column: ColumnDef { name: "main_pinned", type: Simple(Boolean), nullable: false, default: Some(Bool(false)), comment: None, primary_key: None, unique: None, index: Some(Bool(true)), foreign_key: None }, fill_with: None } +CREATE TABLE "article_temp" ( "id" integer, "title" text, "category_pinned" boolean NOT NULL DEFAULT false, "main_pinned" boolean NOT NULL DEFAULT false ); +INSERT INTO "article_temp" ("id", "title", "category_pinned", "main_pinned") SELECT "id", "title", "category_pinned", false AS "main_pinned" FROM "article"; +DROP TABLE "article"; +ALTER TABLE "article_temp" RENAME TO "article" + +-- Action 2: AddConstraint { table: "article", constraint: Index { name: None, columns: ["main_pinned"] } } +CREATE INDEX "ix_article__main_pinned" ON "article" ("main_pinned") + +-- Action 3: AddConstraint { table: "article", constraint: Index { name: None, columns: ["category_pinned"] } } +CREATE INDEX "ix_article__category_pinned" ON "article" ("category_pinned") diff --git a/crates/vespertide-query/src/sql/add_column.rs b/crates/vespertide-query/src/sql/add_column.rs index 66553309..18925611 100644 --- a/crates/vespertide-query/src/sql/add_column.rs +++ b/crates/vespertide-query/src/sql/add_column.rs @@ -37,6 +37,7 @@ pub fn build_add_column( column: &ColumnDef, fill_with: Option<&str>, current_schema: &[TableDef], + pending_constraints: &[vespertide_core::TableConstraint], ) -> Result, QueryError> { // SQLite: NOT NULL additions or enum columns require table recreation // (enum columns need CHECK constraint which requires table recreation in SQLite) @@ -99,7 +100,9 @@ pub fn build_add_column( let rename_query = build_rename_table(&temp_table, table); // Recreate indexes (both regular and UNIQUE) - let index_queries = recreate_indexes_after_rebuild(table, &table_def.constraints, &[]); + // Skip pending constraints that will be created by future AddConstraint actions + let index_queries = + recreate_indexes_after_rebuild(table, &table_def.constraints, pending_constraints); let mut stmts = vec![create_query, insert_query, drop_query, rename_query]; stmts.extend(index_queries); @@ -253,7 +256,7 @@ mod tests { constraints: vec![], }]; let result = - build_add_column(&backend, "users", &column, fill_with, ¤t_schema).unwrap(); + build_add_column(&backend, "users", &column, fill_with, ¤t_schema, &[]).unwrap(); let sql = result[0].build(backend); for exp in expected { assert!( @@ -289,6 +292,7 @@ mod tests { &column, None, ¤t_schema, + &[], ); assert!(result.is_err()); let err_msg = result.unwrap_err().to_string(); @@ -330,6 +334,7 @@ mod tests { &column, None, ¤t_schema, + &[], ); assert!(result.is_ok()); let queries = result.unwrap(); @@ -377,6 +382,7 @@ mod tests { &column, None, ¤t_schema, + &[], ); assert!(result.is_ok()); let queries = result.unwrap(); @@ -429,6 +435,7 @@ mod tests { &column, None, ¤t_schema, + &[], ); assert!(result.is_ok()); let queries = result.unwrap(); @@ -481,7 +488,7 @@ mod tests { }], constraints: vec![], }]; - let result = build_add_column(&backend, "users", &column, None, ¤t_schema); + let result = build_add_column(&backend, "users", &column, None, ¤t_schema, &[]); assert!(result.is_ok()); let queries = result.unwrap(); let sql = queries @@ -538,7 +545,7 @@ mod tests { }], constraints: vec![], }]; - let result = build_add_column(&backend, "users", &column, None, ¤t_schema); + let result = build_add_column(&backend, "users", &column, None, ¤t_schema, &[]); assert!(result.is_ok()); let queries = result.unwrap(); let sql = queries @@ -587,7 +594,7 @@ mod tests { }], constraints: vec![], }]; - let result = build_add_column(&backend, "users", &column, None, ¤t_schema); + let result = build_add_column(&backend, "users", &column, None, ¤t_schema, &[]); assert!(result.is_ok()); let queries = result.unwrap(); let sql = queries @@ -642,7 +649,8 @@ mod tests { }], constraints: vec![], }]; - let result = build_add_column(&backend, "project", &column, None, ¤t_schema).unwrap(); + let result = + build_add_column(&backend, "project", &column, None, ¤t_schema, &[]).unwrap(); let sql = result .iter() .map(|q| q.build(backend)) @@ -708,7 +716,7 @@ mod tests { constraints: vec![], }]; // fill_with empty string should become '' - let result = build_add_column(&backend, "users", &column, Some(""), ¤t_schema); + let result = build_add_column(&backend, "users", &column, Some(""), ¤t_schema, &[]); assert!(result.is_ok()); let queries = result.unwrap(); let sql = queries diff --git a/crates/vespertide-query/src/sql/delete_column.rs b/crates/vespertide-query/src/sql/delete_column.rs index 29ff3ea0..dc2d341f 100644 --- a/crates/vespertide-query/src/sql/delete_column.rs +++ b/crates/vespertide-query/src/sql/delete_column.rs @@ -21,6 +21,7 @@ pub fn build_delete_column( column: &str, column_type: Option<&ColumnType>, current_schema: &[TableDef], + pending_constraints: &[vespertide_core::TableConstraint], ) -> Vec { let mut stmts = Vec::new(); @@ -35,7 +36,13 @@ pub fn build_delete_column( && let ColumnType::Complex(vespertide_core::ComplexColumnType::Enum { .. }) = &col_def.r#type { - return build_delete_column_sqlite_temp_table(table, column, table_def, column_type); + return build_delete_column_sqlite_temp_table( + table, + column, + table_def, + column_type, + pending_constraints, + ); } // Handle constraints referencing the deleted column @@ -51,6 +58,7 @@ pub fn build_delete_column( column, table_def, column_type, + pending_constraints, ); } continue; @@ -64,6 +72,7 @@ pub fn build_delete_column( column, table_def, column_type, + pending_constraints, ); } // Unique/Index: drop the index first, then drop column below @@ -123,6 +132,7 @@ fn build_delete_column_sqlite_temp_table( column: &str, table_def: &TableDef, column_type: Option<&ColumnType>, + pending_constraints: &[vespertide_core::TableConstraint], ) -> Vec { let mut stmts = Vec::new(); let temp_table = format!("{}_temp", table); @@ -183,7 +193,11 @@ fn build_delete_column_sqlite_temp_table( stmts.push(build_rename_table(&temp_table, table)); // 5. Recreate indexes (both regular and UNIQUE) that don't reference the deleted column - stmts.extend(recreate_indexes_after_rebuild(table, &new_constraints, &[])); + stmts.extend(recreate_indexes_after_rebuild( + table, + &new_constraints, + pending_constraints, + )); // If column type is an enum, drop the type after (PostgreSQL only, but include for completeness) if let Some(col_type) = column_type @@ -238,7 +252,7 @@ mod tests { #[case] backend: DatabaseBackend, #[case] expected: &[&str], ) { - let result = build_delete_column(&backend, "users", "email", None, &[]); + let result = build_delete_column(&backend, "users", "email", None, &[], &[]); let sql = result[0].build(backend); for exp in expected { assert!( @@ -268,6 +282,7 @@ mod tests { "status", Some(&enum_type), &[], + &[], ); // Should have 2 statements: ALTER TABLE and DROP TYPE @@ -293,6 +308,7 @@ mod tests { "name", Some(&text_type), &[], + &[], ); // Should only have 1 statement: ALTER TABLE @@ -315,8 +331,14 @@ mod tests { }], }]; - let result = - build_delete_column(&DatabaseBackend::Sqlite, "gift", "gift_code", None, &schema); + let result = build_delete_column( + &DatabaseBackend::Sqlite, + "gift", + "gift_code", + None, + &schema, + &[], + ); // Should have 2 statements: DROP INDEX then ALTER TABLE DROP COLUMN assert_eq!(result.len(), 2); @@ -357,7 +379,14 @@ mod tests { }], }]; - let result = build_delete_column(&DatabaseBackend::Sqlite, "users", "email", None, &schema); + let result = build_delete_column( + &DatabaseBackend::Sqlite, + "users", + "email", + None, + &schema, + &[], + ); // Should have 2 statements: DROP INDEX then ALTER TABLE DROP COLUMN assert_eq!(result.len(), 2); @@ -392,6 +421,7 @@ mod tests { "gift_code", None, &schema, + &[], ); // Should have only 1 statement: ALTER TABLE DROP COLUMN @@ -417,8 +447,14 @@ mod tests { }], }]; - let result = - build_delete_column(&DatabaseBackend::Sqlite, "gift", "gift_code", None, &schema); + let result = build_delete_column( + &DatabaseBackend::Sqlite, + "gift", + "gift_code", + None, + &schema, + &[], + ); assert_eq!(result.len(), 2); @@ -452,8 +488,14 @@ mod tests { }], }]; - let result = - build_delete_column(&DatabaseBackend::Sqlite, "gift", "sender_id", None, &schema); + let result = build_delete_column( + &DatabaseBackend::Sqlite, + "gift", + "sender_id", + None, + &schema, + &[], + ); // Should use temp table approach: // 1. CREATE TABLE gift_temp (without sender_id column) @@ -535,8 +577,14 @@ mod tests { ], }]; - let result = - build_delete_column(&DatabaseBackend::Sqlite, "gift", "sender_id", None, &schema); + let result = build_delete_column( + &DatabaseBackend::Sqlite, + "gift", + "sender_id", + None, + &schema, + &[], + ); let all_sql: Vec = result .iter() @@ -589,6 +637,7 @@ mod tests { "sender_id", None, &schema, + &[], ); // Should have only 1 statement: ALTER TABLE DROP COLUMN @@ -634,6 +683,7 @@ mod tests { "product_id", None, &schema, + &[], ); // Should use temp table approach @@ -677,8 +727,14 @@ mod tests { }]; // Delete nickname, which does NOT have the unique constraint - let result = - build_delete_column(&DatabaseBackend::Sqlite, "users", "nickname", None, &schema); + let result = build_delete_column( + &DatabaseBackend::Sqlite, + "users", + "nickname", + None, + &schema, + &[], + ); // Should only have 1 statement: ALTER TABLE DROP COLUMN (no DROP INDEX needed) assert_eq!( @@ -744,8 +800,14 @@ mod tests { ], }]; - let result = - build_delete_column(&DatabaseBackend::Sqlite, "orders", "user_id", None, &schema); + let result = build_delete_column( + &DatabaseBackend::Sqlite, + "orders", + "user_id", + None, + &schema, + &[], + ); let all_sql: Vec = result .iter() @@ -815,7 +877,7 @@ mod tests { }], }]; - let result = build_delete_column(&backend, "users", "email", None, &schema); + let result = build_delete_column(&backend, "users", "email", None, &schema, &[]); let sql = build_sql_snapshot(&result, backend); with_settings!({ snapshot_suffix => format!("delete_column_with_unique_{}", title) }, { @@ -848,7 +910,7 @@ mod tests { }], }]; - let result = build_delete_column(&backend, "posts", "created_at", None, &schema); + let result = build_delete_column(&backend, "posts", "created_at", None, &schema, &[]); let sql = build_sql_snapshot(&result, backend); with_settings!({ snapshot_suffix => format!("delete_column_with_index_{}", title) }, { @@ -882,7 +944,7 @@ mod tests { }], }]; - let result = build_delete_column(&backend, "orders", "user_id", None, &schema); + let result = build_delete_column(&backend, "orders", "user_id", None, &schema, &[]); let sql = build_sql_snapshot(&result, backend); with_settings!({ snapshot_suffix => format!("delete_column_with_fk_{}", title) }, { @@ -912,7 +974,7 @@ mod tests { }], }]; - let result = build_delete_column(&backend, "order_items", "product_id", None, &schema); + let result = build_delete_column(&backend, "order_items", "product_id", None, &schema, &[]); let sql = build_sql_snapshot(&result, backend); with_settings!({ snapshot_suffix => format!("delete_column_with_pk_{}", title) }, { @@ -957,7 +1019,7 @@ mod tests { ], }]; - let result = build_delete_column(&backend, "orders", "user_id", None, &schema); + let result = build_delete_column(&backend, "orders", "user_id", None, &schema, &[]); let sql = build_sql_snapshot(&result, backend); with_settings!({ snapshot_suffix => format!("delete_column_with_fk_and_index_{}", title) }, { @@ -1005,6 +1067,7 @@ mod tests { "user_id", Some(&enum_type), &schema, + &[], ); // Should use temp table approach (FK triggers it) + DROP TYPE at end @@ -1063,8 +1126,14 @@ mod tests { }]; // Delete amount column — CHECK references it, so temp table is needed - let result = - build_delete_column(&DatabaseBackend::Sqlite, "orders", "amount", None, &schema); + let result = build_delete_column( + &DatabaseBackend::Sqlite, + "orders", + "amount", + None, + &schema, + &[], + ); // Should use temp table approach (CREATE temp, INSERT, DROP, RENAME) assert!( @@ -1106,7 +1175,14 @@ mod tests { }]; // Delete "note" column — CHECK only references "amount", not "note" - let result = build_delete_column(&DatabaseBackend::Sqlite, "orders", "note", None, &schema); + let result = build_delete_column( + &DatabaseBackend::Sqlite, + "orders", + "note", + None, + &schema, + &[], + ); assert_eq!( result.len(), diff --git a/crates/vespertide-query/src/sql/mod.rs b/crates/vespertide-query/src/sql/mod.rs index ee2dbcf1..e7ae64f6 100644 --- a/crates/vespertide-query/src/sql/mod.rs +++ b/crates/vespertide-query/src/sql/mod.rs @@ -63,7 +63,14 @@ pub fn build_action_queries_with_pending( table, column, fill_with, - } => build_add_column(backend, table, column, fill_with.as_deref(), current_schema), + } => build_add_column( + backend, + table, + column, + fill_with.as_deref(), + current_schema, + pending_constraints, + ), MigrationAction::RenameColumn { table, from, to } => { Ok(vec![build_rename_column(table, from, to)]) @@ -82,6 +89,7 @@ pub fn build_action_queries_with_pending( column, column_type, current_schema, + pending_constraints, )) } @@ -97,6 +105,7 @@ pub fn build_action_queries_with_pending( new_type, fill_with.as_ref(), current_schema, + pending_constraints, ), MigrationAction::ModifyColumnNullable { @@ -113,6 +122,7 @@ pub fn build_action_queries_with_pending( fill_with.as_deref(), delete_null_rows.unwrap_or(false), current_schema, + pending_constraints, ), MigrationAction::ModifyColumnDefault { @@ -125,6 +135,7 @@ pub fn build_action_queries_with_pending( column, new_default.as_deref(), current_schema, + pending_constraints, ), MigrationAction::ModifyColumnComment { @@ -151,9 +162,13 @@ pub fn build_action_queries_with_pending( pending_constraints, ), - MigrationAction::RemoveConstraint { table, constraint } => { - build_remove_constraint(backend, table, constraint, current_schema) - } + MigrationAction::RemoveConstraint { table, constraint } => build_remove_constraint( + backend, + table, + constraint, + current_schema, + pending_constraints, + ), } } diff --git a/crates/vespertide-query/src/sql/modify_column_default.rs b/crates/vespertide-query/src/sql/modify_column_default.rs index 4e173fd2..f9e91dc2 100644 --- a/crates/vespertide-query/src/sql/modify_column_default.rs +++ b/crates/vespertide-query/src/sql/modify_column_default.rs @@ -17,6 +17,7 @@ pub fn build_modify_column_default( column: &str, new_default: Option<&str>, current_schema: &[TableDef], + pending_constraints: &[vespertide_core::TableConstraint], ) -> Result, QueryError> { let mut queries = Vec::new(); @@ -142,7 +143,7 @@ pub fn build_modify_column_default( queries.extend(recreate_indexes_after_rebuild( table, &table_def.constraints, - &[], + pending_constraints, )); } } @@ -204,7 +205,8 @@ mod tests { vec![], )]; - let result = build_modify_column_default(&backend, "users", "email", new_default, &schema); + let result = + build_modify_column_default(&backend, "users", "email", new_default, &schema, &[]); assert!(result.is_ok()); let queries = result.unwrap(); let sql = queries @@ -244,7 +246,7 @@ mod tests { } let result = - build_modify_column_default(&backend, "users", "email", Some("'default'"), &[]); + build_modify_column_default(&backend, "users", "email", Some("'default'"), &[], &[]); assert!(result.is_err()); let err_msg = result.unwrap_err().to_string(); assert!(err_msg.contains("Table 'users' not found")); @@ -272,8 +274,14 @@ mod tests { vec![], )]; - let result = - build_modify_column_default(&backend, "users", "email", Some("'default'"), &schema); + let result = build_modify_column_default( + &backend, + "users", + "email", + Some("'default'"), + &schema, + &[], + ); assert!(result.is_err()); let err_msg = result.unwrap_err().to_string(); assert!(err_msg.contains("Column 'email' not found")); @@ -301,6 +309,7 @@ mod tests { "status", // column not in schema Some("'active'"), &schema, + &[], ); assert!(result.is_ok()); let queries = result.unwrap(); @@ -338,6 +347,7 @@ mod tests { "email", Some("'default@example.com'"), &schema, + &[], ); assert!(result.is_ok()); let queries = result.unwrap(); @@ -391,6 +401,7 @@ mod tests { "email", Some("'new@example.com'"), &schema, + &[], ); assert!(result.is_ok()); let queries = result.unwrap(); @@ -434,7 +445,7 @@ mod tests { )]; let result = - build_modify_column_default(&backend, "products", "quantity", Some("0"), &schema); + build_modify_column_default(&backend, "products", "quantity", Some("0"), &schema, &[]); assert!(result.is_ok()); let queries = result.unwrap(); let sql = queries @@ -477,7 +488,7 @@ mod tests { )]; let result = - build_modify_column_default(&backend, "users", "is_active", Some("true"), &schema); + build_modify_column_default(&backend, "users", "is_active", Some("true"), &schema, &[]); assert!(result.is_ok()); let queries = result.unwrap(); let sql = queries @@ -531,6 +542,7 @@ mod tests { "created_at", Some(default_value), &schema, + &[], ); assert!(result.is_ok()); let queries = result.unwrap(); @@ -573,8 +585,12 @@ mod tests { )]; let result = build_modify_column_default( - &backend, "orders", "status", None, // Drop default + &backend, + "orders", + "status", + None, // Drop default &schema, + &[], ); assert!(result.is_ok()); let queries = result.unwrap(); diff --git a/crates/vespertide-query/src/sql/modify_column_nullable.rs b/crates/vespertide-query/src/sql/modify_column_nullable.rs index 6e5463cf..2c414584 100644 --- a/crates/vespertide-query/src/sql/modify_column_nullable.rs +++ b/crates/vespertide-query/src/sql/modify_column_nullable.rs @@ -12,6 +12,7 @@ use crate::error::QueryError; /// Build SQL for changing column nullability. /// For nullable -> non-nullable transitions, fill_with should be provided to update NULL values. +#[allow(clippy::too_many_arguments)] pub fn build_modify_column_nullable( backend: &DatabaseBackend, table: &str, @@ -20,6 +21,7 @@ pub fn build_modify_column_nullable( fill_with: Option<&str>, delete_null_rows: bool, current_schema: &[TableDef], + pending_constraints: &[vespertide_core::TableConstraint], ) -> Result, QueryError> { let mut queries = Vec::new(); @@ -144,7 +146,7 @@ pub fn build_modify_column_nullable( queries.extend(recreate_indexes_after_rebuild( table, &table_def.constraints, - &[], + pending_constraints, )); } } @@ -215,7 +217,14 @@ mod tests { )]; let result = build_modify_column_nullable( - &backend, "users", "email", nullable, fill_with, false, &schema, + &backend, + "users", + "email", + nullable, + fill_with, + false, + &schema, + &[], ); assert!(result.is_ok()); let queries = result.unwrap(); @@ -257,7 +266,7 @@ mod tests { } let result = - build_modify_column_nullable(&backend, "users", "email", false, None, false, &[]); + build_modify_column_nullable(&backend, "users", "email", false, None, false, &[], &[]); assert!(result.is_err()); let err_msg = result.unwrap_err().to_string(); assert!(err_msg.contains("Table 'users' not found")); @@ -285,8 +294,16 @@ mod tests { vec![], )]; - let result = - build_modify_column_nullable(&backend, "users", "email", false, None, false, &schema); + let result = build_modify_column_nullable( + &backend, + "users", + "email", + false, + None, + false, + &schema, + &[], + ); assert!(result.is_err()); let err_msg = result.unwrap_err().to_string(); assert!(err_msg.contains("Column 'email' not found")); @@ -310,8 +327,16 @@ mod tests { }], )]; - let result = - build_modify_column_nullable(&backend, "users", "email", false, None, false, &schema); + let result = build_modify_column_nullable( + &backend, + "users", + "email", + false, + None, + false, + &schema, + &[], + ); assert!(result.is_ok()); let queries = result.unwrap(); let sql = queries @@ -367,6 +392,7 @@ mod tests { Some("NOW()"), false, &schema, + &[], ); assert!(result.is_ok()); let queries = result.unwrap(); @@ -420,8 +446,16 @@ mod tests { vec![], )]; - let result = - build_modify_column_nullable(&backend, "users", "email", false, None, false, &schema); + let result = build_modify_column_nullable( + &backend, + "users", + "email", + false, + None, + false, + &schema, + &[], + ); assert!(result.is_ok()); let queries = result.unwrap(); let sql = queries @@ -468,8 +502,16 @@ mod tests { vec![], )]; - let result = - build_modify_column_nullable(&backend, "orders", "user_id", false, None, true, &schema); + let result = build_modify_column_nullable( + &backend, + "orders", + "user_id", + false, + None, + true, + &schema, + &[], + ); assert!(result.is_ok()); let queries = result.unwrap(); let sql = queries @@ -525,8 +567,16 @@ mod tests { vec![], )]; - let result = - build_modify_column_nullable(&backend, "orders", "user_id", true, None, true, &schema); + let result = build_modify_column_nullable( + &backend, + "orders", + "user_id", + true, + None, + true, + &schema, + &[], + ); assert!(result.is_ok()); let queries = result.unwrap(); let sql = queries diff --git a/crates/vespertide-query/src/sql/modify_column_type.rs b/crates/vespertide-query/src/sql/modify_column_type.rs index c0601c53..328047d1 100644 --- a/crates/vespertide-query/src/sql/modify_column_type.rs +++ b/crates/vespertide-query/src/sql/modify_column_type.rs @@ -39,6 +39,7 @@ pub fn build_modify_column_type( new_type: &ColumnType, fill_with: Option<&BTreeMap>, current_schema: &[TableDef], + pending_constraints: &[vespertide_core::TableConstraint], ) -> Result, QueryError> { // SQLite does not support direct column type modification, so use temporary table approach if *backend == DatabaseBackend::Sqlite { @@ -99,7 +100,8 @@ pub fn build_modify_column_type( let rename_query = build_rename_table(&temp_table, table); // 5. Recreate indexes (both regular and UNIQUE) - let index_queries = recreate_indexes_after_rebuild(table, &table_def.constraints, &[]); + let index_queries = + recreate_indexes_after_rebuild(table, &table_def.constraints, pending_constraints); let mut queries = Vec::new(); @@ -383,6 +385,7 @@ mod tests { &ColumnType::Complex(ComplexColumnType::Varchar { length: 50 }), None, ¤t_schema, + &[], ); // SQLite may return multiple queries @@ -417,6 +420,7 @@ mod tests { &ColumnType::Simple(SimpleColumnType::BigInt), None, &[], + &[], ); assert!(result.is_err()); assert!( @@ -452,6 +456,7 @@ mod tests { &ColumnType::Simple(SimpleColumnType::BigInt), None, ¤t_schema, + &[], ); assert!(result.is_err()); assert!( @@ -519,6 +524,7 @@ mod tests { &ColumnType::Simple(SimpleColumnType::BigInt), None, ¤t_schema, + &[], ) .unwrap(); @@ -599,6 +605,7 @@ mod tests { &ColumnType::Complex(ComplexColumnType::Varchar { length: 255 }), None, ¤t_schema, + &[], ) .unwrap(); @@ -811,6 +818,7 @@ mod tests { &new_type, None, ¤t_schema, + &[], ) .unwrap(); @@ -877,6 +885,7 @@ mod tests { &new_type, None, ¤t_schema, + &[], ) .unwrap(); @@ -934,6 +943,7 @@ mod tests { }), None, &[], // Empty schema - old_type will be None + &[], ); assert!(result.is_ok()); @@ -1020,6 +1030,7 @@ mod tests { &new_type, Some(&fill_with_map), ¤t_schema, + &[], ) .unwrap(); diff --git a/crates/vespertide-query/src/sql/remove_constraint.rs b/crates/vespertide-query/src/sql/remove_constraint.rs index 71ca8b5c..47cb2d67 100644 --- a/crates/vespertide-query/src/sql/remove_constraint.rs +++ b/crates/vespertide-query/src/sql/remove_constraint.rs @@ -13,6 +13,7 @@ pub fn build_remove_constraint( table: &str, constraint: &TableConstraint, current_schema: &[TableDef], + pending_constraints: &[TableConstraint], ) -> Result, QueryError> { match constraint { TableConstraint::PrimaryKey { .. } => { @@ -63,8 +64,11 @@ pub fn build_remove_constraint( let rename_query = build_rename_table(&temp_table, table); // 5. Recreate indexes (both regular and UNIQUE) - let index_queries = - recreate_indexes_after_rebuild(table, &table_def.constraints, &[]); + let index_queries = recreate_indexes_after_rebuild( + table, + &table_def.constraints, + pending_constraints, + ); let mut queries = vec![create_query, insert_query, drop_query, rename_query]; queries.extend(index_queries); @@ -153,7 +157,8 @@ pub fn build_remove_constraint( let rename_query = build_rename_table(&temp_table, table); // 5. Recreate indexes (both regular and UNIQUE, from new_constraints which has the unique removed) - let index_queries = recreate_indexes_after_rebuild(table, &new_constraints, &[]); + let index_queries = + recreate_indexes_after_rebuild(table, &new_constraints, pending_constraints); let mut queries = vec![create_query, insert_query, drop_query, rename_query]; queries.extend(index_queries); @@ -248,8 +253,11 @@ pub fn build_remove_constraint( let rename_query = build_rename_table(&temp_table, table); // 5. Recreate indexes (both regular and UNIQUE) - let index_queries = - recreate_indexes_after_rebuild(table, &table_def.constraints, &[]); + let index_queries = recreate_indexes_after_rebuild( + table, + &table_def.constraints, + pending_constraints, + ); let mut queries = vec![create_query, insert_query, drop_query, rename_query]; queries.extend(index_queries); @@ -335,8 +343,11 @@ pub fn build_remove_constraint( let rename_query = build_rename_table(&temp_table, table); // 5. Recreate indexes (both regular and UNIQUE) - let index_queries = - recreate_indexes_after_rebuild(table, &table_def.constraints, &[]); + let index_queries = recreate_indexes_after_rebuild( + table, + &table_def.constraints, + pending_constraints, + ); let mut queries = vec![create_query, insert_query, drop_query, rename_query]; queries.extend(index_queries); @@ -526,7 +537,7 @@ mod tests { }]; let result = - build_remove_constraint(&backend, "users", &constraint, ¤t_schema).unwrap(); + build_remove_constraint(&backend, "users", &constraint, ¤t_schema, &[]).unwrap(); let sql = result[0].build(backend); for exp in expected { assert!( @@ -554,6 +565,7 @@ mod tests { "nonexistent_table", &constraint, &[], // Empty schema + &[], ); assert!(result.is_err()); let err_msg = result.unwrap_err().to_string(); @@ -594,7 +606,7 @@ mod tests { }]; let result = - build_remove_constraint(&backend, "users", &constraint, ¤t_schema).unwrap(); + build_remove_constraint(&backend, "users", &constraint, ¤t_schema, &[]).unwrap(); let sql = result .iter() .map(|q| q.build(backend)) @@ -645,7 +657,7 @@ mod tests { }]; let result = - build_remove_constraint(&backend, "users", &constraint, ¤t_schema).unwrap(); + build_remove_constraint(&backend, "users", &constraint, ¤t_schema, &[]).unwrap(); let sql = result .iter() .map(|q| q.build(backend)) @@ -674,6 +686,7 @@ mod tests { "nonexistent_table", &constraint, &[], // Empty schema + &[], ); assert!(result.is_err()); let err_msg = result.unwrap_err().to_string(); @@ -721,7 +734,7 @@ mod tests { }]; let result = - build_remove_constraint(&backend, "users", &constraint, ¤t_schema).unwrap(); + build_remove_constraint(&backend, "users", &constraint, ¤t_schema, &[]).unwrap(); let sql = result .iter() .map(|q| q.build(backend)) @@ -785,7 +798,7 @@ mod tests { }]; let result = - build_remove_constraint(&backend, "users", &constraint, ¤t_schema).unwrap(); + build_remove_constraint(&backend, "users", &constraint, ¤t_schema, &[]).unwrap(); let sql = result .iter() .map(|q| q.build(backend)) @@ -851,7 +864,7 @@ mod tests { }]; let result = - build_remove_constraint(&backend, "users", &constraint, ¤t_schema).unwrap(); + build_remove_constraint(&backend, "users", &constraint, ¤t_schema, &[]).unwrap(); let sql = result .iter() .map(|q| q.build(backend)) @@ -884,6 +897,7 @@ mod tests { "nonexistent_table", &constraint, &[], // Empty schema + &[], ); assert!(result.is_err()); let err_msg = result.unwrap_err().to_string(); @@ -935,7 +949,7 @@ mod tests { }]; let result = - build_remove_constraint(&backend, "posts", &constraint, ¤t_schema).unwrap(); + build_remove_constraint(&backend, "posts", &constraint, ¤t_schema, &[]).unwrap(); let sql = result .iter() .map(|q| q.build(backend)) @@ -1003,7 +1017,7 @@ mod tests { }]; let result = - build_remove_constraint(&backend, "posts", &constraint, ¤t_schema).unwrap(); + build_remove_constraint(&backend, "posts", &constraint, ¤t_schema, &[]).unwrap(); let sql = result .iter() .map(|q| q.build(backend)) @@ -1071,7 +1085,7 @@ mod tests { }]; let result = - build_remove_constraint(&backend, "posts", &constraint, ¤t_schema).unwrap(); + build_remove_constraint(&backend, "posts", &constraint, ¤t_schema, &[]).unwrap(); let sql = result .iter() .map(|q| q.build(backend)) @@ -1100,6 +1114,7 @@ mod tests { "nonexistent_table", &constraint, &[], // Empty schema + &[], ); assert!(result.is_err()); let err_msg = result.unwrap_err().to_string(); @@ -1153,7 +1168,7 @@ mod tests { }]; let result = - build_remove_constraint(&backend, "users", &constraint, ¤t_schema).unwrap(); + build_remove_constraint(&backend, "users", &constraint, ¤t_schema, &[]).unwrap(); let sql = result .iter() .map(|q| q.build(backend)) @@ -1217,7 +1232,7 @@ mod tests { }]; let result = - build_remove_constraint(&backend, "users", &constraint, ¤t_schema).unwrap(); + build_remove_constraint(&backend, "users", &constraint, ¤t_schema, &[]).unwrap(); let sql = result .iter() .map(|q| q.build(backend)) @@ -1285,7 +1300,7 @@ mod tests { }]; let result = - build_remove_constraint(&backend, "users", &constraint, ¤t_schema).unwrap(); + build_remove_constraint(&backend, "users", &constraint, ¤t_schema, &[]).unwrap(); let sql = result .iter() .map(|q| q.build(backend)) @@ -1359,7 +1374,7 @@ mod tests { }]; let result = - build_remove_constraint(&backend, "posts", &constraint, ¤t_schema).unwrap(); + build_remove_constraint(&backend, "posts", &constraint, ¤t_schema, &[]).unwrap(); let sql = result .iter() .map(|q| q.build(backend)) @@ -1425,7 +1440,7 @@ mod tests { }]; let result = - build_remove_constraint(&backend, "users", &constraint, ¤t_schema).unwrap(); + build_remove_constraint(&backend, "users", &constraint, ¤t_schema, &[]).unwrap(); let sql = result .iter() .map(|q| q.build(backend)) @@ -1463,9 +1478,14 @@ mod tests { }], constraints: vec![constraint.clone()], }]; - let result = - build_remove_constraint(&DatabaseBackend::Postgres, "orders", &constraint, &schema) - .unwrap(); + let result = build_remove_constraint( + &DatabaseBackend::Postgres, + "orders", + &constraint, + &schema, + &[], + ) + .unwrap(); assert_eq!(result.len(), 1); let sql = result[0].build(DatabaseBackend::Postgres); assert!(sql.contains("ALTER TABLE \"orders\" DROP CONSTRAINT \"orders_pkey\"")); @@ -1495,7 +1515,7 @@ mod tests { constraints: vec![constraint.clone()], }]; let result = - build_remove_constraint(&DatabaseBackend::MySql, "orders", &constraint, &schema) + build_remove_constraint(&DatabaseBackend::MySql, "orders", &constraint, &schema, &[]) .unwrap(); assert_eq!(result.len(), 1); let sql = result[0].build(DatabaseBackend::MySql); @@ -1533,7 +1553,7 @@ mod tests { constraints: vec![], }]; - let result = build_remove_constraint(&backend, "users", &constraint, &schema); + let result = build_remove_constraint(&backend, "users", &constraint, &schema, &[]); assert!(result.is_ok()); let sql = result .unwrap()