From 6b592c8999fbfb0c881620fcbd0e150eb2c62340 Mon Sep 17 00:00:00 2001 From: Taylor Date: Sun, 19 Jul 2026 11:40:24 -0400 Subject: [PATCH 1/4] fix(migrate): create pass leaves existing tables to the reconciliation pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One auto-migrate pass that both adds columns to an existing table and declares a composite unique over them crashed at boot: the create pass fired the model's index DDL at the already-existing table before the reconciliation pass could add the referenced columns (Postgres: 'column does not exist'; SQLite silently built the index over a string constant via its double-quote fallback). The create pass now introspects live table names once and skips existing tables entirely — enforcing the documented contract that plain auto_migrate leaves existing tables untouched. Index reconciliation on existing tables already lives, correctly ordered, in migrate_updates. See ADR-0010; part of #324. --- src/introspect.rs | 27 ++++++++++++++++ src/schema.rs | 13 ++++++++ tests/test_auto_migrate.py | 64 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 104 insertions(+) diff --git a/src/introspect.rs b/src/introspect.rs index 0d081e1..49e5869 100644 --- a/src/introspect.rs +++ b/src/introspect.rs @@ -128,6 +128,33 @@ pub async fn live_table_columns( }) } +/// The set of live table names in the connected schema — one query, taken by +/// the create pass so it can leave every existing table to the reconciliation +/// pass (ADR-0010) instead of leaning on `IF NOT EXISTS` per statement. +pub async fn live_table_names( + engine: &EngineHandle, +) -> PyResult> { + let (sql, context) = match engine.backend() { + Dialect::Sqlite => ( + "SELECT name FROM sqlite_master WHERE type = 'table'", + "sqlite_master", + ), + Dialect::Postgres => ( + "SELECT table_name::text AS name FROM information_schema.tables \ + WHERE table_schema = current_schema()", + "information_schema.tables", + ), + }; + let rows = engine + .fetch_all_sql_unprepared(sql) + .await + .map_err(|e| introspection_error(context, "*", e))?; + Ok(rows + .iter() + .filter_map(|row| row_string(row, "name")) + .collect()) +} + async fn sqlite_table_columns(engine: &EngineHandle, table: &str) -> PyResult> { let sql = format!("PRAGMA table_info({})", quote_ident(table)); let rows = engine diff --git a/src/schema.rs b/src/schema.rs index 6af0e31..de9b620 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -79,9 +79,22 @@ pub async fn internal_create_tables(engine: Arc) -> PyResult<()> { let dialect = engine.backend(); + // ADR-0010: the create pass owns only missing tables. An existing table — + // whatever its shape — belongs to the reconciliation pass; firing even + // `IF NOT EXISTS` index DDL at it can reference columns only the + // reconcile pass will add (#324). + let existing_tables = crate::introspect::live_table_names(&engine).await?; + let model_refs: Vec<&ferro_schema_ir::SchemaModel> = modelset.payload.models.iter().collect(); for model in ferro_migrate::order_models_for_create(&model_refs) { + if existing_tables.contains(&model.table_name) { + crate::log_debug(format!( + "Ferro Engine: Table '{}' already exists — left to the reconciliation pass", + model.table_name + )); + continue; + } let emission = ferro_migrate::render_create_table(model, dialect).map_err(|err| { pyo3::exceptions::PyRuntimeError::new_err(format!( "CREATE TABLE emission failed for '{}': {}", diff --git a/tests/test_auto_migrate.py b/tests/test_auto_migrate.py index 0ad48c1..ccab386 100644 --- a/tests/test_auto_migrate.py +++ b/tests/test_auto_migrate.py @@ -887,6 +887,70 @@ class IdxSingleModel(Model): # noqa: F811 ) +@pytest.mark.asyncio +@pytest.mark.backend_matrix +async def test_migrate_updates_adds_columns_before_composite_unique_referencing_them( + db_url, db_backend, clean_registry +): + """Issue #324: one migrate_updates pass over an existing populated table + must add new columns before creating a composite unique that references + them. The create pass must leave the existing table entirely alone — + firing the unique's DDL there fails with "column does not exist" before + the reconciliation pass can add the columns.""" + from typing import ClassVar + + class ProvTxn(Model): + id: Annotated[int | None, FerroField(primary_key=True)] = None + account_id: int + + # Phase 1: the pre-upgrade release creates and populates the table. + await ferro.connect(db_url, auto_migrate=True) + async with ferro.engines.session(): + await ProvTxn.create(account_id=1) + ferro.reset_engine() + + from ferro import clear_registry + from ferro.registry import REGISTRY + + clear_registry() + REGISTRY.reset_for_test() + + # Phase 2: the upgrade declares two new columns and a composite unique + # spanning one existing and one new column — the single-deploy shape. + class ProvTxn(Model): # noqa: F811 — intentional redefinition + __ferro_composite_uniques__: ClassVar[tuple[tuple[str, ...], ...]] = ( + ("account_id", "provider_transaction_id"), + ) + id: Annotated[int | None, FerroField(primary_key=True)] = None + account_id: int + provider_transaction_id: str | None = None + pending: bool = False + + await ferro.connect(db_url, auto_migrate=True, migrate_updates=True) + async with ferro.engines.session(): + rows = await ProvTxn.all() + assert len(rows) == 1 + assert rows[0].provider_transaction_id is None + assert rows[0].pending is False + + names = _live_index_names(db_url, db_backend, "provtxn") + assert "uq_provtxn_account_id_provider_transaction_id" in names, ( + f"expected uq_provtxn_account_id_provider_transaction_id, got: {names}" + ) + + # The unique must span BOTH columns. (SQLite's double-quoted-string + # fallback can silently create an index over a string *constant* when + # the column doesn't exist yet — same account with two distinct + # provider ids must be allowed.) + await ProvTxn.create(account_id=2, provider_transaction_id="p1") + await ProvTxn.create(account_id=2, provider_transaction_id="p2") + + from ferro.exceptions import UniqueViolationError + + with pytest.raises(UniqueViolationError): + await ProvTxn.create(account_id=2, provider_transaction_id="p1") + + @pytest.mark.asyncio @pytest.mark.backend_matrix async def test_index_reconcile_noop_when_index_already_present( From fd82425919264634a94fd8d88c05246c445e1a2b Mon Sep 17 00:00:00 2001 From: Taylor Date: Sun, 19 Jul 2026 11:46:35 -0400 Subject: [PATCH 2/4] feat(migrate): introspect live FK constraints into the reconcile old IR MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The live-side IR previously hardcoded foreign_keys to empty: no FK introspection existed, so a declared FK change on an existing column was invisible to the diff. Introduce LiveForeignKey and per-backend readers — pg_constraint on Postgres (constraint name, columns, target, confdeltype mapped to the declared action vocabulary) and PRAGMA foreign_key_list on SQLite (which exposes no constraint names) — and populate the old IR's foreign_keys from them. Multi-column live FKs are user-owned by construction (ferro only emits single-column FKs) and are not surfaced. No diff behavior change yet; the FK diff step lands next. Part of #325. --- src/introspect.rs | 210 ++++++++++++++++++++++++++++++++++++++++++++++ src/migrate.rs | 57 +++++++++++-- 2 files changed, 261 insertions(+), 6 deletions(-) diff --git a/src/introspect.rs b/src/introspect.rs index 49e5869..0cdc3b4 100644 --- a/src/introspect.rs +++ b/src/introspect.rs @@ -57,6 +57,50 @@ pub(crate) fn is_ferro_index_name(name: &str) -> bool { name.starts_with("idx_") || name.starts_with("uq_") } +/// One live single-column foreign-key constraint, normalized across backends. +/// (Ferro only ever emits single-column FKs; multi-column live constraints are +/// user-owned by construction and never surfaced to reconciliation.) +/// +/// `Deserialize` exists for `_render_migration_sql_for_test`. +#[derive(Clone, Debug, serde::Deserialize)] +pub struct LiveForeignKey { + /// Constraint name. `None` on SQLite — `PRAGMA foreign_key_list` does not + /// expose constraint names (one more way SQLite FK constraints cannot be + /// reconciled in place, only warned about). + #[serde(default)] + pub name: Option, + /// Local column. + pub column: String, + /// Referenced table. + pub to_table: String, + /// Referenced column. + #[serde(default)] + pub to_column: String, + /// `ON DELETE` action in the declared-IR vocabulary: `CASCADE`, + /// `SET NULL`, `SET DEFAULT`, `RESTRICT`, `NO ACTION`. + pub on_delete: String, +} + +/// Ferro emits FK constraints as `fk___` +/// (`ferro_ddl_lowering::fk_name`). Reconciliation only ever touches names it +/// owns. +pub(crate) fn is_ferro_fk_name(name: &str) -> bool { + name.starts_with("fk_") +} + +/// Map `pg_constraint.confdeltype` to the declared-IR action vocabulary. +/// Returns `None` for codes Postgres does not produce. +pub(crate) fn fk_action_from_confdeltype(confdeltype: &str) -> Option<&'static str> { + match confdeltype { + "a" => Some("NO ACTION"), + "r" => Some("RESTRICT"), + "c" => Some("CASCADE"), + "n" => Some("SET NULL"), + "d" => Some("SET DEFAULT"), + _ => None, + } +} + /// One live SQLite index covering some column, with enough context to decide /// whether it can be dropped ahead of `ALTER TABLE ... DROP COLUMN`. #[derive(Clone, Debug)] @@ -155,6 +199,101 @@ pub async fn live_table_names( .collect()) } +/// Read the live single-column foreign-key constraints on `table`. +pub async fn live_table_foreign_keys( + engine: &EngineHandle, + table: &str, +) -> PyResult> { + match engine.backend() { + Dialect::Sqlite => sqlite_table_foreign_keys(engine, table).await, + Dialect::Postgres => postgres_table_foreign_keys(engine, table).await, + } +} + +async fn sqlite_table_foreign_keys( + engine: &EngineHandle, + table: &str, +) -> PyResult> { + let sql = format!("PRAGMA foreign_key_list({})", quote_ident(table)); + let rows = engine + .fetch_all_sql_unprepared(&sql) + .await + .map_err(|e| introspection_error("PRAGMA foreign_key_list", table, e))?; + + // Rows are (id, seq, table, from, to, on_update, on_delete, match); a + // multi-column FK repeats its `id` with seq > 0 — those are user-owned by + // construction and skipped whole. + let multi_column: std::collections::HashSet = rows + .iter() + .filter(|row| row_opt_i64(row, "seq").unwrap_or(0) > 0) + .filter_map(|row| row_opt_i64(row, "id")) + .collect(); + + Ok(rows + .iter() + .filter(|row| row_opt_i64(row, "seq").unwrap_or(0) == 0) + .filter(|row| row_opt_i64(row, "id").map_or(true, |id| !multi_column.contains(&id))) + .filter_map(|row| { + Some(LiveForeignKey { + name: None, + column: row_string(row, "from")?, + to_table: row_string(row, "table")?, + // `to` is NULL when the FK references the target's implicit PK. + to_column: row_string(row, "to").unwrap_or_default(), + on_delete: row_string(row, "on_delete") + .unwrap_or_else(|| "NO ACTION".to_string()) + .to_uppercase(), + }) + }) + .collect()) +} + +async fn postgres_table_foreign_keys( + engine: &EngineHandle, + table: &str, +) -> PyResult> { + let sql = r#" + SELECT + con.conname::text AS name, + src.attname::text AS column_name, + rel_f.relname::text AS to_table, + dst.attname::text AS to_column, + con.confdeltype::text AS on_delete, + array_length(con.conkey, 1)::bigint AS n_cols + FROM pg_constraint con + JOIN pg_class rel ON rel.oid = con.conrelid + JOIN pg_namespace nsp ON nsp.oid = rel.relnamespace + JOIN pg_class rel_f ON rel_f.oid = con.confrelid + LEFT JOIN pg_attribute src + ON src.attrelid = con.conrelid AND src.attnum = con.conkey[1] + LEFT JOIN pg_attribute dst + ON dst.attrelid = con.confrelid AND dst.attnum = con.confkey[1] + WHERE con.contype = 'f' + AND nsp.nspname = current_schema() + AND rel.relname = $1 + ORDER BY con.conname + "#; + + let rows = engine + .fetch_all_sql_unprepared_with_binds(sql, &[EngineBindValue::String(table.to_string())]) + .await + .map_err(|e| introspection_error("pg_constraint", table, e))?; + + Ok(rows + .iter() + .filter(|row| row_opt_i64(row, "n_cols") == Some(1)) + .filter_map(|row| { + Some(LiveForeignKey { + name: row_string(row, "name"), + column: row_string(row, "column_name")?, + to_table: row_string(row, "to_table")?, + to_column: row_string(row, "to_column").unwrap_or_default(), + on_delete: fk_action_from_confdeltype(&row_string(row, "on_delete")?)?.to_string(), + }) + }) + .collect()) +} + async fn sqlite_table_columns(engine: &EngineHandle, table: &str) -> PyResult> { let sql = format!("PRAGMA table_info({})", quote_ident(table)); let rows = engine @@ -375,6 +514,77 @@ mod tests { assert!(!is_ferro_index_name("my_custom_index")); } + #[test] + fn ferro_fk_names_are_recognized() { + assert!(is_ferro_fk_name("fk_account_connection_id_connection")); + assert!(!is_ferro_fk_name("account_connection_id_fkey")); + assert!(!is_ferro_fk_name("my_custom_fk")); + } + + #[test] + fn confdeltype_maps_to_declared_action_vocabulary() { + assert_eq!(fk_action_from_confdeltype("a"), Some("NO ACTION")); + assert_eq!(fk_action_from_confdeltype("r"), Some("RESTRICT")); + assert_eq!(fk_action_from_confdeltype("c"), Some("CASCADE")); + assert_eq!(fk_action_from_confdeltype("n"), Some("SET NULL")); + assert_eq!(fk_action_from_confdeltype("d"), Some("SET DEFAULT")); + assert_eq!(fk_action_from_confdeltype("x"), None); + } + + #[tokio::test] + async fn live_table_names_lists_sqlite_tables() { + let engine = memory_engine().await; + engine + .execute_sql("CREATE TABLE alpha (id integer)") + .await + .unwrap(); + engine + .execute_sql("CREATE TABLE beta (id integer)") + .await + .unwrap(); + + let names = live_table_names(&engine).await.unwrap(); + assert!(names.contains("alpha")); + assert!(names.contains("beta")); + assert!(!names.contains("gamma")); + } + + #[tokio::test] + async fn live_table_foreign_keys_reads_single_column_fks_and_skips_composite() { + let engine = memory_engine().await; + engine + .execute_sql("CREATE TABLE connection (id INTEGER PRIMARY KEY)") + .await + .unwrap(); + engine + .execute_sql("CREATE TABLE pair (a integer, b integer, PRIMARY KEY (a, b))") + .await + .unwrap(); + engine + .execute_sql( + "CREATE TABLE account (\ + id INTEGER PRIMARY KEY, \ + connection_id integer, \ + pa integer, pb integer, \ + CONSTRAINT fk_account_connection_id_connection \ + FOREIGN KEY (connection_id) REFERENCES connection (id) \ + ON DELETE SET NULL, \ + FOREIGN KEY (pa, pb) REFERENCES pair (a, b))", + ) + .await + .unwrap(); + + let fks = live_table_foreign_keys(&engine, "account").await.unwrap(); + assert_eq!(fks.len(), 1, "composite FK must be skipped: {fks:?}"); + let fk = &fks[0]; + // PRAGMA foreign_key_list exposes no constraint names. + assert_eq!(fk.name, None); + assert_eq!(fk.column, "connection_id"); + assert_eq!(fk.to_table, "connection"); + assert_eq!(fk.to_column, "id"); + assert_eq!(fk.on_delete, "SET NULL"); + } + #[tokio::test] async fn live_table_columns_reads_sqlite_structure() { let engine = memory_engine().await; diff --git a/src/migrate.rs b/src/migrate.rs index 4b0a044..e6b3576 100644 --- a/src/migrate.rs +++ b/src/migrate.rs @@ -19,7 +19,8 @@ use ferro_schema_ir::{ SchemaModel, SchemaUnique, }; use crate::introspect::{ - LiveColumn, LiveIndex, live_table_columns, live_table_indexes, quote_ident, + LiveColumn, LiveForeignKey, LiveIndex, live_table_columns, live_table_foreign_keys, + live_table_indexes, quote_ident, sqlite_indexes_covering_column, }; use crate::schema::{internal_create_tables, order_models_for_migration}; @@ -141,6 +142,7 @@ fn live_columns_to_schema_ir( table_lower: &str, live: &[LiveColumn], live_indexes: &[LiveIndex], + live_foreign_keys: &[LiveForeignKey], backend: Dialect, ) -> IrEnvelope { let dialect = backend; @@ -177,7 +179,20 @@ fn live_columns_to_schema_ir( model_name: table_lower.to_string(), table_name: table_lower.to_string(), columns, - foreign_keys: Vec::::new(), + foreign_keys: { + let mut fks: Vec = live_foreign_keys + .iter() + .map(|fk| SchemaForeignKey { + column: fk.column.clone(), + to_table: fk.to_table.clone(), + to_column: fk.to_column.clone(), + on_delete: Some(fk.on_delete.clone()), + name: fk.name.clone(), + }) + .collect(); + fks.sort_by(|a, b| a.column.cmp(&b.column)); + fks + }, indexes: live_indexes.iter().map(|i| SchemaIndex { name: i.name.clone(), columns: i.columns.clone(), unique: i.unique, }).collect(), @@ -246,6 +261,7 @@ pub fn plan_table_migration( declared: &IrEnvelope, live: &[LiveColumn], live_indexes: &[LiveIndex], + live_foreign_keys: &[LiveForeignKey], backend: Dialect, opts: MigrateOptions, ) -> PyResult { @@ -253,7 +269,8 @@ pub fn plan_table_migration( return Ok(MigrationPlan::new()); } - let old_ir = live_columns_to_schema_ir(table_lower, live, live_indexes, backend); + let old_ir = + live_columns_to_schema_ir(table_lower, live, live_indexes, live_foreign_keys, backend); let new_ir = declared; let mut typed_plan = plan_from_ir(&old_ir, new_ir, backend); @@ -416,9 +433,18 @@ pub async fn internal_migrate(engine: Arc, opts: MigrateOptions) - continue; }; let live_indexes = live_table_indexes(&engine, &table_lower).await?; + let live_foreign_keys = live_table_foreign_keys(&engine, &table_lower).await?; let Some(declared) = declared_envelope_for(&modelset, &table_lower) else { continue }; - let mut plan = plan_table_migration(&table_lower, &declared, &live, &live_indexes, backend, opts)?; + let mut plan = plan_table_migration( + &table_lower, + &declared, + &live, + &live_indexes, + &live_foreign_keys, + backend, + opts, + )?; if plan.is_empty() { warnings.append(&mut plan.warnings); continue; @@ -568,7 +594,7 @@ pub fn migrate( /// unrecognized, or the diff contains an unsafe change. #[pyfunction] #[pyo3(name = "_render_migration_sql_for_test")] -#[pyo3(signature = (name, schema_ir_json, live_columns_json, dialect, updates=true, destructive=false, live_indexes_json=String::new()))] +#[pyo3(signature = (name, schema_ir_json, live_columns_json, dialect, updates=true, destructive=false, live_indexes_json=String::new(), live_foreign_keys_json=String::new()))] pub fn _render_migration_sql_for_test( name: String, schema_ir_json: String, @@ -577,6 +603,7 @@ pub fn _render_migration_sql_for_test( updates: bool, destructive: bool, live_indexes_json: String, + live_foreign_keys_json: String, ) -> PyResult<(Vec, Vec)> { let backend = match dialect.as_str() { "postgres" => Dialect::Postgres, @@ -601,10 +628,28 @@ pub fn _render_migration_sql_for_test( pyo3::exceptions::PyValueError::new_err(format!("Invalid live-indexes JSON: {}", e)) })? }; + let live_foreign_keys: Vec = if live_foreign_keys_json.is_empty() { + Vec::new() + } else { + serde_json::from_str(&live_foreign_keys_json).map_err(|e| { + pyo3::exceptions::PyValueError::new_err(format!( + "Invalid live-foreign-keys JSON: {}", + e + )) + })? + }; let table_lower = name; let opts = MigrateOptions::laddered(updates, destructive); - let plan = plan_table_migration(&table_lower, &declared, &live, &live_indexes, backend, opts)?; + let plan = plan_table_migration( + &table_lower, + &declared, + &live, + &live_indexes, + &live_foreign_keys, + backend, + opts, + )?; let mut statements = plan.statements; for col_name in &plan.drop_columns { From 566fb9dcc9715dae7167b0876be90cc677d41672 Mon Sep 17 00:00:00 2001 From: Taylor Date: Sun, 19 Jul 2026 12:17:27 -0400 Subject: [PATCH 3/4] feat(migrate): reconcile ferro-owned FK definition drift (#325) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Changing a ForeignKey's on_delete on an existing model was silently ignored — the dangerous kind of miss: the model declares SET NULL, the database keeps CASCADE, and the first parent delete destroys children an operation documented as non-destructive. migrate_updates now diffs each declared FK against the live constraint on the same column. On Postgres, a ferro-owned (fk_-named) constraint whose definition drifts (on_delete, target) is rebuilt — DROP CONSTRAINT + ADD CONSTRAINT under the canonical name, inside the per-table transaction — and a declared FK missing entirely from an existing column is added. A drifting constraint ferro does not own is left untouched but warned about. On SQLite, which cannot alter table constraints, every FK drift warns loudly instead — never silence. New MigrationOp variants AddForeignKey / RebuildForeignKey; ownership predicate is_ferro_fk_name single-sourced in ferro-ddl-lowering. --- crates/ferro-ddl-lowering/src/lib.rs | 7 + crates/ferro-migrate/src/emit.rs | 99 +++++++- crates/ferro-migrate/src/lib.rs | 104 +++++++- crates/ferro-migrate/src/tests.rs | 343 +++++++++++++++++++++++++++ src/introspect.rs | 8 +- tests/test_auto_migrate.py | 105 ++++++++ tests/test_migrate_plan.py | 84 +++++++ 7 files changed, 732 insertions(+), 18 deletions(-) diff --git a/crates/ferro-ddl-lowering/src/lib.rs b/crates/ferro-ddl-lowering/src/lib.rs index 9e6379f..9d27b80 100644 --- a/crates/ferro-ddl-lowering/src/lib.rs +++ b/crates/ferro-ddl-lowering/src/lib.rs @@ -542,6 +542,13 @@ pub fn fk_name(table_lower: &str, col_name: &str, to_table: &str) -> String { raw } +/// Whether a live constraint name follows the ferro FK convention above — the +/// ownership test: reconciliation only ever rebuilds names ferro emits; +/// constraints named any other way belong to the user and are never altered. +pub fn is_ferro_fk_name(name: &str) -> bool { + name.starts_with("fk_") +} + /// Single-column unique name with 63-char guard. pub fn single_unique_index_name(table_lower: &str, col_name: &str) -> String { let raw = format!("uq_{table_lower}_{col_name}"); diff --git a/crates/ferro-migrate/src/emit.rs b/crates/ferro-migrate/src/emit.rs index adbe712..3724446 100644 --- a/crates/ferro-migrate/src/emit.rs +++ b/crates/ferro-migrate/src/emit.rs @@ -408,16 +408,7 @@ fn emit_add_column( if let Some(fk) = model.foreign_keys.iter().find(|fk| fk.column == column) { if dialect == Dialect::Postgres { - let on_delete = fk_action_from_str(fk.on_delete.as_deref()); - result.statements.push(format!( - "ALTER TABLE {} ADD CONSTRAINT {} FOREIGN KEY ({}) REFERENCES {} ({}) ON DELETE {}", - quote_ident(table), - quote_ident(&fk_constraint_name(table, fk)), - quote_ident(column), - quote_ident(&fk.to_table), - quote_ident(&fk.to_column), - fk_action_sql(on_delete), - )); + result.statements.push(render_add_fk_sql(table, fk)); } else { result.warnings.push(format!( "Added foreign-key column '{}.{}' without its FOREIGN KEY constraint \ @@ -432,6 +423,39 @@ fn emit_add_column( Ok(result) } +/// `ALTER TABLE ... ADD CONSTRAINT ... FOREIGN KEY ... ON DELETE ...` for one +/// IR foreign key. Postgres-only — SQLite cannot add table constraints to an +/// existing table; its callers warn instead. +fn render_add_fk_sql(table: &str, fk: &ferro_schema_ir::SchemaForeignKey) -> String { + format!( + "ALTER TABLE {} ADD CONSTRAINT {} FOREIGN KEY ({}) REFERENCES {} ({}) ON DELETE {}", + quote_ident(table), + quote_ident(&fk_constraint_name(table, fk)), + quote_ident(&fk.column), + quote_ident(&fk.to_table), + quote_ident(&fk.to_column), + fk_action_sql(fk_action_from_str(fk.on_delete.as_deref())), + ) +} + +/// Find the declared FK for `column` on `table` in the new-IR model. +fn find_foreign_key<'a>( + model: &'a SchemaModel, + table: &str, + column: &str, +) -> Result<&'a ferro_schema_ir::SchemaForeignKey, EmissionError> { + model + .foreign_keys + .iter() + .find(|fk| fk.column == column) + .ok_or_else(|| EmissionError { + message: format!( + "Foreign-key operation for '{}.{}' has no matching FK in the declared IR", + table, column + ), + }) +} + fn emit_alter_column_type( table: &str, column: &str, @@ -673,6 +697,61 @@ pub fn emit_sql_with_ir( MigrationOp::DropIndex { table: _, name } => { result.statements.push(format!("DROP INDEX IF EXISTS \"{}\"", name)); } + MigrationOp::AddForeignKey { table, column } => { + let model = find_model(&new_models, table)?; + let fk = find_foreign_key(model, table, column)?; + match dialect { + Dialect::Postgres => result.statements.push(render_add_fk_sql(table, fk)), + Dialect::Sqlite => result.warnings.push(format!( + "Declared FOREIGN KEY on '{}.{}' (on_delete {}) has no live \ + constraint, and SQLite cannot add table constraints to an \ + existing table. Referential integrity for this column is not \ + database-enforced; use Alembic if you need the constraint.", + table, + column, + fk_action_sql(fk_action_from_str(fk.on_delete.as_deref())), + )), + } + } + MigrationOp::RebuildForeignKey { + table, + column, + old_name, + } => { + let model = find_model(&new_models, table)?; + let fk = find_foreign_key(model, table, column)?; + match dialect { + Dialect::Postgres => { + result.statements.push(format!( + "ALTER TABLE {} DROP CONSTRAINT {}", + quote_ident(table), + quote_ident(old_name), + )); + result.statements.push(render_add_fk_sql(table, fk)); + } + Dialect::Sqlite => { + let live_action = find_model(&old_models, table) + .ok() + .and_then(|old| { + old.foreign_keys.iter().find(|live| live.column == *column) + }) + .map(|live| { + fk_action_sql(fk_action_from_str(live.on_delete.as_deref())) + }) + .unwrap_or(""); + result.warnings.push(format!( + "Foreign key on '{}.{}' declares on_delete {} but the live \ + constraint enforces {}; SQLite cannot alter constraints in \ + place, so the live behavior remains. Migrate with Alembic to \ + apply the declared action.", + table, + column, + fk_action_sql(fk_action_from_str(fk.on_delete.as_deref())), + live_action, + )); + } + } + } } } diff --git a/crates/ferro-migrate/src/lib.rs b/crates/ferro-migrate/src/lib.rs index 01d1353..8ddaf03 100644 --- a/crates/ferro-migrate/src/lib.rs +++ b/crates/ferro-migrate/src/lib.rs @@ -6,7 +6,9 @@ mod emit; mod order; -use ferro_ddl_lowering::schema_columns_storage_drift; +use ferro_ddl_lowering::{ + fk_action_from_str, fk_action_sql, fk_name, is_ferro_fk_name, schema_columns_storage_drift, +}; use ferro_schema_ir::{IrEnvelope, SchemaIrPayload, SchemaModel}; use std::collections::{BTreeMap, BTreeSet}; @@ -97,6 +99,25 @@ pub enum MigrationOp { /// Index name. name: String, }, + /// A declared FK whose column exists live but has no FK constraint at all. + /// (FKs on newly added columns ride the `AddColumn` emission instead.) + AddForeignKey { + /// Owning table. + table: String, + /// Local FK column. + column: String, + }, + /// A live ferro-owned FK whose definition (`on_delete`, target) drifted + /// from the declared FK on the same column — rebuilt as + /// `DROP CONSTRAINT` + `ADD CONSTRAINT` where the backend allows it. + RebuildForeignKey { + /// Owning table. + table: String, + /// Local FK column. + column: String, + /// Name of the live constraint to drop. + old_name: String, + }, } /// Ordered migration operations plus non-fatal warnings collected during planning. @@ -167,6 +188,13 @@ pub fn emit_sql(plan: &MigrationPlan, dialect: Dialect) -> Vec { MigrationOp::DropIndex { name, .. } => { sql.push(format!("-- index '{}' handled by emit_sql_with_ir", name)); } + MigrationOp::AddForeignKey { table, column } + | MigrationOp::RebuildForeignKey { table, column, .. } => { + sql.push(format!( + "-- foreign key for '{}.{}' handled by emit_sql_with_ir", + table, column + )); + } } } sql @@ -205,6 +233,7 @@ pub fn plan_from_ir( }; diff_model_columns(*table, old_model, new_model, dialect, &mut plan); diff_model_indexes(*table, old_model, new_model, &mut plan); + diff_model_foreign_keys(*table, old_model, new_model, &mut plan); } plan @@ -320,5 +349,78 @@ fn diff_model_indexes( } } +fn diff_model_foreign_keys( + table: &str, + old_model: &SchemaModel, + new_model: &SchemaModel, + plan: &mut MigrationPlan, +) { + let old_col_names: BTreeSet<&str> = old_model.columns.iter().map(|c| c.name.as_str()).collect(); + + for fk in &new_model.foreign_keys { + // An FK on a newly added column rides the AddColumn emission; the + // reconcile step only governs FKs whose column already exists live. + if !old_col_names.contains(fk.column.as_str()) { + continue; + } + + let Some(live) = old_model + .foreign_keys + .iter() + .find(|live| live.column == fk.column) + else { + plan.operations.push(MigrationOp::AddForeignKey { + table: table.to_string(), + column: fk.column.clone(), + }); + continue; + }; + + // Live `to_column` can be empty when the backend reports an + // implicit-PK reference (SQLite); only a stated target can drift. + let target_drift = live.to_table != fk.to_table + || (!live.to_column.is_empty() && live.to_column != fk.to_column); + // Compare via the canonical SQL rendering — sea-query's + // `ForeignKeyAction` has no equality of its own. + let action_drift = fk_action_sql(fk_action_from_str(live.on_delete.as_deref())) + != fk_action_sql(fk_action_from_str(fk.on_delete.as_deref())); + if !target_drift && !action_drift { + continue; + } + + match live.name.as_deref() { + // A drifting constraint ferro does not own is never altered — + // but it is never silent either. + Some(name) if !is_ferro_fk_name(name) => { + plan.warnings.push(format!( + "Foreign key on '{}.{}' drifts from the model (live: REFERENCES {} \ + ON DELETE {}; declared: REFERENCES {} ON DELETE {}), but the live \ + constraint '{}' is not ferro-owned, so it is left untouched. \ + Migrate it manually or with Alembic.", + table, + fk.column, + live.to_table, + fk_action_sql(fk_action_from_str(live.on_delete.as_deref())), + fk.to_table, + fk_action_sql(fk_action_from_str(fk.on_delete.as_deref())), + name, + )); + } + _ => { + plan.operations.push(MigrationOp::RebuildForeignKey { + table: table.to_string(), + column: fk.column.clone(), + // SQLite exposes no live constraint names; fall back to + // the canonical name (unused there — emission warns). + old_name: live + .name + .clone() + .unwrap_or_else(|| fk_name(table, &fk.column, &live.to_table)), + }); + } + } + } +} + #[cfg(test)] mod tests; diff --git a/crates/ferro-migrate/src/tests.rs b/crates/ferro-migrate/src/tests.rs index 08e176f..8093bb2 100644 --- a/crates/ferro-migrate/src/tests.rs +++ b/crates/ferro-migrate/src/tests.rs @@ -1492,3 +1492,346 @@ fn order_models_for_create_self_fk_is_not_an_ordering_constraint() { let tables: Vec<&str> = ordered.iter().map(|m| m.table_name.as_str()).collect(); assert_eq!(tables, vec!["znode", "areferrer"]); } + +// --------------------------------------------------------------------------- +// Foreign-key reconciliation (#325) +// --------------------------------------------------------------------------- + +fn fk( + column: &str, + to_table: &str, + on_delete: Option<&str>, + name: Option<&str>, +) -> SchemaForeignKey { + SchemaForeignKey { + column: column.to_string(), + to_table: to_table.to_string(), + to_column: "id".to_string(), + on_delete: on_delete.map(str::to_string), + name: name.map(str::to_string), + } +} + +fn schema_model_with_fks( + table: &str, + cols: Vec, + fks: Vec, +) -> SchemaModel { + let mut model = schema_model(table, cols); + model.foreign_keys = fks; + model +} + +#[test] +fn plan_from_ir_rebuilds_fk_on_delete_drift() { + let old_ir = envelope(vec![schema_model_with_fks( + "account", + vec![col("connection_id", "integer", true)], + vec![fk( + "connection_id", + "connection", + Some("CASCADE"), + Some("fk_account_connection_id_connection"), + )], + )]); + let new_ir = envelope(vec![schema_model_with_fks( + "account", + vec![col("connection_id", "integer", true)], + vec![fk( + "connection_id", + "connection", + Some("SET NULL"), + Some("fk_account_connection_id_connection"), + )], + )]); + + let plan = plan_from_ir(&old_ir, &new_ir, Dialect::Postgres); + assert_eq!( + plan.operations, + vec![MigrationOp::RebuildForeignKey { + table: "account".to_string(), + column: "connection_id".to_string(), + old_name: "fk_account_connection_id_connection".to_string(), + }] + ); + assert!(plan.warnings.is_empty()); +} + +#[test] +fn plan_from_ir_fk_noop_when_definition_matches() { + // Declared None means CASCADE — a live CASCADE constraint is not drift. + let old_ir = envelope(vec![schema_model_with_fks( + "account", + vec![col("connection_id", "integer", true)], + vec![fk( + "connection_id", + "connection", + Some("CASCADE"), + Some("fk_account_connection_id_connection"), + )], + )]); + let new_ir = envelope(vec![schema_model_with_fks( + "account", + vec![col("connection_id", "integer", true)], + vec![fk( + "connection_id", + "connection", + None, + Some("fk_account_connection_id_connection"), + )], + )]); + + let plan = plan_from_ir(&old_ir, &new_ir, Dialect::Postgres); + assert!( + plan.operations.is_empty(), + "unexpected ops: {:?}", + plan.operations + ); + assert!(plan.warnings.is_empty()); +} + +#[test] +fn plan_from_ir_adds_fk_missing_on_existing_column() { + let old_ir = envelope(vec![schema_model( + "account", + vec![col("connection_id", "integer", true)], + )]); + let new_ir = envelope(vec![schema_model_with_fks( + "account", + vec![col("connection_id", "integer", true)], + vec![fk( + "connection_id", + "connection", + Some("SET NULL"), + Some("fk_account_connection_id_connection"), + )], + )]); + + let plan = plan_from_ir(&old_ir, &new_ir, Dialect::Postgres); + assert_eq!( + plan.operations, + vec![MigrationOp::AddForeignKey { + table: "account".to_string(), + column: "connection_id".to_string(), + }] + ); +} + +#[test] +fn plan_from_ir_fk_on_new_column_rides_add_column() { + // The FK's column does not exist live: AddColumn emission owns the + // constraint, so no standalone FK op may be planned. + let old_ir = envelope(vec![schema_model( + "account", + vec![col("id", "integer", false)], + )]); + let new_ir = envelope(vec![schema_model_with_fks( + "account", + vec![ + col("id", "integer", false), + col("connection_id", "integer", true), + ], + vec![fk( + "connection_id", + "connection", + Some("SET NULL"), + Some("fk_account_connection_id_connection"), + )], + )]); + + let plan = plan_from_ir(&old_ir, &new_ir, Dialect::Postgres); + assert_eq!( + plan.operations, + vec![MigrationOp::AddColumn { + table: "account".to_string(), + column: "connection_id".to_string(), + }] + ); +} + +#[test] +fn plan_from_ir_warns_on_user_owned_fk_drift() { + let old_ir = envelope(vec![schema_model_with_fks( + "account", + vec![col("connection_id", "integer", true)], + vec![fk( + "connection_id", + "connection", + Some("CASCADE"), + Some("account_connection_id_fkey"), + )], + )]); + let new_ir = envelope(vec![schema_model_with_fks( + "account", + vec![col("connection_id", "integer", true)], + vec![fk( + "connection_id", + "connection", + Some("SET NULL"), + Some("fk_account_connection_id_connection"), + )], + )]); + + let plan = plan_from_ir(&old_ir, &new_ir, Dialect::Postgres); + assert!( + plan.operations.is_empty(), + "unexpected ops: {:?}", + plan.operations + ); + assert_eq!(plan.warnings.len(), 1); + assert!( + plan.warnings[0].contains("not ferro-owned"), + "{}", + plan.warnings[0] + ); + assert!(plan.warnings[0].contains("account_connection_id_fkey")); +} + +#[test] +fn plan_from_ir_unnamed_live_fk_drift_rebuilds_with_canonical_name() { + // SQLite live FKs carry no constraint name; the op falls back to the + // canonical name (emission warns on SQLite anyway). + let old_ir = envelope(vec![schema_model_with_fks( + "account", + vec![col("connection_id", "integer", true)], + vec![fk("connection_id", "connection", Some("CASCADE"), None)], + )]); + let new_ir = envelope(vec![schema_model_with_fks( + "account", + vec![col("connection_id", "integer", true)], + vec![fk( + "connection_id", + "connection", + Some("SET NULL"), + Some("fk_account_connection_id_connection"), + )], + )]); + + let plan = plan_from_ir(&old_ir, &new_ir, Dialect::Sqlite); + assert_eq!( + plan.operations, + vec![MigrationOp::RebuildForeignKey { + table: "account".to_string(), + column: "connection_id".to_string(), + old_name: "fk_account_connection_id_connection".to_string(), + }] + ); +} + +#[test] +fn emit_sql_with_ir_rebuild_fk_postgres_drops_then_adds() { + let new_ir = envelope(vec![schema_model_with_fks( + "account", + vec![col("connection_id", "integer", true)], + vec![fk( + "connection_id", + "connection", + Some("SET NULL"), + Some("fk_account_connection_id_connection"), + )], + )]); + let plan = MigrationPlan { + operations: vec![MigrationOp::RebuildForeignKey { + table: "account".to_string(), + column: "connection_id".to_string(), + old_name: "fk_account_connection_id_connection".to_string(), + }], + warnings: Vec::new(), + }; + + let result = emit_sql_with_ir(&plan, &empty_envelope(), &new_ir, Dialect::Postgres).unwrap(); + assert_eq!( + result.statements, + vec![ + "ALTER TABLE \"account\" DROP CONSTRAINT \"fk_account_connection_id_connection\"" + .to_string(), + "ALTER TABLE \"account\" ADD CONSTRAINT \"fk_account_connection_id_connection\" \ + FOREIGN KEY (\"connection_id\") REFERENCES \"connection\" (\"id\") \ + ON DELETE SET NULL" + .to_string(), + ] + ); + assert!(result.warnings.is_empty()); +} + +#[test] +fn emit_sql_with_ir_rebuild_fk_sqlite_warns_and_skips() { + let old_ir = envelope(vec![schema_model_with_fks( + "account", + vec![col("connection_id", "integer", true)], + vec![fk("connection_id", "connection", Some("CASCADE"), None)], + )]); + let new_ir = envelope(vec![schema_model_with_fks( + "account", + vec![col("connection_id", "integer", true)], + vec![fk( + "connection_id", + "connection", + Some("SET NULL"), + Some("fk_account_connection_id_connection"), + )], + )]); + let plan = MigrationPlan { + operations: vec![MigrationOp::RebuildForeignKey { + table: "account".to_string(), + column: "connection_id".to_string(), + old_name: "fk_account_connection_id_connection".to_string(), + }], + warnings: Vec::new(), + }; + + let result = emit_sql_with_ir(&plan, &old_ir, &new_ir, Dialect::Sqlite).unwrap(); + assert!( + result.statements.is_empty(), + "unexpected DDL: {:?}", + result.statements + ); + assert_eq!(result.warnings.len(), 1); + assert!( + result.warnings[0].contains("on_delete SET NULL"), + "{}", + result.warnings[0] + ); + assert!( + result.warnings[0].contains("CASCADE"), + "{}", + result.warnings[0] + ); +} + +#[test] +fn emit_sql_with_ir_add_fk_postgres_and_sqlite() { + let new_ir = envelope(vec![schema_model_with_fks( + "account", + vec![col("connection_id", "integer", true)], + vec![fk( + "connection_id", + "connection", + Some("SET NULL"), + Some("fk_account_connection_id_connection"), + )], + )]); + let plan = MigrationPlan { + operations: vec![MigrationOp::AddForeignKey { + table: "account".to_string(), + column: "connection_id".to_string(), + }], + warnings: Vec::new(), + }; + + let pg = emit_sql_with_ir(&plan, &empty_envelope(), &new_ir, Dialect::Postgres).unwrap(); + assert_eq!( + pg.statements, + vec![ + "ALTER TABLE \"account\" ADD CONSTRAINT \"fk_account_connection_id_connection\" \ + FOREIGN KEY (\"connection_id\") REFERENCES \"connection\" (\"id\") \ + ON DELETE SET NULL" + .to_string(), + ] + ); + + let sqlite = emit_sql_with_ir(&plan, &empty_envelope(), &new_ir, Dialect::Sqlite).unwrap(); + assert!(sqlite.statements.is_empty()); + assert_eq!(sqlite.warnings.len(), 1); + assert!(sqlite.warnings[0].contains("SQLite cannot add table constraints")); +} diff --git a/src/introspect.rs b/src/introspect.rs index 0cdc3b4..1faf959 100644 --- a/src/introspect.rs +++ b/src/introspect.rs @@ -81,13 +81,6 @@ pub struct LiveForeignKey { pub on_delete: String, } -/// Ferro emits FK constraints as `fk_
__` -/// (`ferro_ddl_lowering::fk_name`). Reconciliation only ever touches names it -/// owns. -pub(crate) fn is_ferro_fk_name(name: &str) -> bool { - name.starts_with("fk_") -} - /// Map `pg_constraint.confdeltype` to the declared-IR action vocabulary. /// Returns `None` for codes Postgres does not produce. pub(crate) fn fk_action_from_confdeltype(confdeltype: &str) -> Option<&'static str> { @@ -516,6 +509,7 @@ mod tests { #[test] fn ferro_fk_names_are_recognized() { + use ferro_ddl_lowering::is_ferro_fk_name; assert!(is_ferro_fk_name("fk_account_connection_id_connection")); assert!(!is_ferro_fk_name("account_connection_id_fkey")); assert!(!is_ferro_fk_name("my_custom_fk")); diff --git a/tests/test_auto_migrate.py b/tests/test_auto_migrate.py index ccab386..c19fbe8 100644 --- a/tests/test_auto_migrate.py +++ b/tests/test_auto_migrate.py @@ -951,6 +951,111 @@ class ProvTxn(Model): # noqa: F811 — intentional redefinition await ProvTxn.create(account_id=2, provider_transaction_id="p1") +@pytest.mark.asyncio +@pytest.mark.postgres_only +async def test_migrate_updates_rebuilds_fk_on_delete_drift(db_url, clean_registry): + """Issue #325: changing a ForeignKey's on_delete on an existing model must + rebuild the live constraint, not silently keep the old action. The Pinch + shape: CASCADE (the default) shipped, then the upgrade declares SET NULL — + deleting the parent must sever the reference, never destroy the child.""" + from ferro import ForeignKey + + class FkDriftConn(Model): + id: Annotated[int | None, FerroField(primary_key=True)] = None + name: str + accounts: Relation[list["FkDriftAccount"]] = BackRef() + + class FkDriftAccount(Model): + id: Annotated[int | None, FerroField(primary_key=True)] = None + connection: Annotated[ + FkDriftConn | None, ForeignKey(related_name="accounts") + ] = None + + # Phase A: the pre-upgrade release creates the schema (default CASCADE) + # and populates it. + await ferro.connect(db_url, auto_migrate=True) + async with ferro.engines.session(): + parent = await FkDriftConn.create(name="c1") + await FkDriftAccount.create(connection=parent) + ferro.reset_engine() + + from ferro import clear_registry + from ferro.registry import REGISTRY + + clear_registry() + REGISTRY.reset_for_test() + + # Phase B: identical models except the FK now declares SET NULL. + class FkDriftConn(Model): # noqa: F811 — intentional redefinition + id: Annotated[int | None, FerroField(primary_key=True)] = None + name: str + accounts: Relation[list["FkDriftAccount"]] = BackRef() + + class FkDriftAccount(Model): # noqa: F811 — intentional redefinition + id: Annotated[int | None, FerroField(primary_key=True)] = None + connection: Annotated[ + FkDriftConn | None, + ForeignKey(related_name="accounts", on_delete="SET NULL"), + ] = None + + await ferro.connect(db_url, migrate_updates=True) + async with ferro.engines.session(): + parents = await FkDriftConn.all() + await parents[0].delete() + + # Sever, never destroy: the child survives with a nulled reference. + children = await FkDriftAccount.all() + assert len(children) == 1 + assert children[0].connection_id is None + + +@pytest.mark.asyncio +@pytest.mark.sqlite_only +async def test_sqlite_fk_on_delete_drift_warns_loudly(db_url, clean_registry): + """Issue #325 on SQLite: FK constraints cannot be altered in place, so an + on_delete change on an existing table must warn loudly (naming the + constraint) instead of diverging silently.""" + from ferro import ForeignKey + + class FkWarnConn(Model): + id: Annotated[int | None, FerroField(primary_key=True)] = None + name: str + accounts: Relation[list["FkWarnAccount"]] = BackRef() + + class FkWarnAccount(Model): + id: Annotated[int | None, FerroField(primary_key=True)] = None + connection: Annotated[ + FkWarnConn | None, ForeignKey(related_name="accounts") + ] = None + + await ferro.connect(db_url, auto_migrate=True) + async with ferro.engines.session(): + parent = await FkWarnConn.create(name="c1") + await FkWarnAccount.create(connection=parent) + ferro.reset_engine() + + from ferro import clear_registry + from ferro.registry import REGISTRY + + clear_registry() + REGISTRY.reset_for_test() + + class FkWarnConn(Model): # noqa: F811 — intentional redefinition + id: Annotated[int | None, FerroField(primary_key=True)] = None + name: str + accounts: Relation[list["FkWarnAccount"]] = BackRef() + + class FkWarnAccount(Model): # noqa: F811 — intentional redefinition + id: Annotated[int | None, FerroField(primary_key=True)] = None + connection: Annotated[ + FkWarnConn | None, + ForeignKey(related_name="accounts", on_delete="SET NULL"), + ] = None + + with pytest.warns(UserWarning, match=r"on_delete|fk_fkwarnaccount"): + await ferro.connect(db_url, migrate_updates=True) + + @pytest.mark.asyncio @pytest.mark.backend_matrix async def test_index_reconcile_noop_when_index_already_present( diff --git a/tests/test_migrate_plan.py b/tests/test_migrate_plan.py index 82cdbd3..7e1a205 100644 --- a/tests/test_migrate_plan.py +++ b/tests/test_migrate_plan.py @@ -496,3 +496,87 @@ def test_json_jsonb_edit_is_noop_on_sqlite(self): stmts, warns = render(schema, live, "sqlite") assert stmts == [] assert warns == [] + + +class TestForeignKeyReconcile: + """Plan-level rendering for FK definition drift on existing columns (#325).""" + + LIVE_WITH_FK_COLUMN = PK_ONLY_LIVE + [ + {"name": "connection_id", "declared_type": "integer", "is_nullable": True} + ] + LIVE_FK_CASCADE = [ + { + "name": "fk_invoice_connection_id_connection", + "column": "connection_id", + "to_table": "connection", + "to_column": "id", + "on_delete": "CASCADE", + } + ] + + def _schema(self, on_delete): + return schema_with( + { + "connection_id": { + "type": "integer", + "ferro_nullable": True, + "foreign_key": {"to_table": "connection", "on_delete": on_delete}, + } + } + ) + + def _render(self, dialect, *, live_fks, on_delete="SET NULL"): + schema = self._schema(on_delete) + return _render_migration_sql_for_test( + "invoice", + _compile_schema_ir_json(schema, "invoice"), + json.dumps(self.LIVE_WITH_FK_COLUMN), + dialect, + True, + False, + "", + json.dumps(live_fks), + ) + + def test_pg_on_delete_drift_rebuilds_constraint(self): + stmts, warns = self._render("postgres", live_fks=self.LIVE_FK_CASCADE) + assert stmts == [ + 'ALTER TABLE "invoice" DROP CONSTRAINT "fk_invoice_connection_id_connection"', + 'ALTER TABLE "invoice" ADD CONSTRAINT "fk_invoice_connection_id_connection"' + ' FOREIGN KEY ("connection_id") REFERENCES "connection" ("id")' + " ON DELETE SET NULL", + ] + assert warns == [] + + def test_pg_matching_on_delete_is_noop(self): + stmts, warns = self._render( + "postgres", live_fks=self.LIVE_FK_CASCADE, on_delete="CASCADE" + ) + assert stmts == [] + assert warns == [] + + def test_pg_missing_fk_on_existing_column_is_added(self): + stmts, warns = self._render("postgres", live_fks=[]) + assert stmts == [ + 'ALTER TABLE "invoice" ADD CONSTRAINT "fk_invoice_connection_id_connection"' + ' FOREIGN KEY ("connection_id") REFERENCES "connection" ("id")' + " ON DELETE SET NULL", + ] + assert warns == [] + + def test_pg_user_owned_fk_drift_warns_and_leaves_constraint(self): + user_fk = [dict(self.LIVE_FK_CASCADE[0], name="invoice_connection_id_fkey")] + stmts, warns = self._render("postgres", live_fks=user_fk) + assert stmts == [] + assert len(warns) == 1 + assert "not ferro-owned" in warns[0] + assert "invoice_connection_id_fkey" in warns[0] + + def test_sqlite_on_delete_drift_warns_and_emits_no_ddl(self): + unnamed_fk = [dict(self.LIVE_FK_CASCADE[0], name=None)] + stmts, warns = self._render("sqlite", live_fks=unnamed_fk) + assert stmts == [] + assert len(warns) == 1 + assert "on_delete SET NULL" in warns[0] + assert "CASCADE" in warns[0] + assert "Alembic" in warns[0] From 0ea0e1fcbeedf8204ddd80dcb158e4010150aa66 Mon Sep 17 00:00:00 2001 From: Taylor Date: Sun, 19 Jul 2026 12:19:02 -0400 Subject: [PATCH 4/4] docs(migrate): pass-ownership contract, FK reconciliation coverage, ADR-0010 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Document the auto-migrate pass-ownership decision (create pass owns only missing tables; the reconciliation pass is the sole authority for DDL against existing tables) as ADR-0010, with the new CONTEXT.md terms (create pass, reconciliation pass, ferro-owned artifact, constraint rebuild). Extend the migrate_updates coverage docs — guide table, architecture, FAQ, and the connect()/create_tables()/migrate() docstrings — with FK reconciliation semantics per backend. --- CONTEXT.md | 16 ++++++++ ...10-create-pass-owns-only-missing-tables.md | 39 +++++++++++++++++++ docs/pages/concepts/architecture.md | 2 +- docs/pages/faq.md | 2 +- docs/pages/guide/migrations.md | 3 ++ src/ferro/__init__.py | 20 ++++++++-- 6 files changed, 76 insertions(+), 6 deletions(-) create mode 100644 docs/adr/0010-create-pass-owns-only-missing-tables.md diff --git a/CONTEXT.md b/CONTEXT.md index 4bbe690..c6ddf0c 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -123,3 +123,19 @@ _Avoid_: Import-time registration, partial registry **Resolved registration**: The registry epoch after relationship resolution completes — join tables exist, shadow FK columns are wired, and the SchemaIR modelset is authoritative for DDL and auto-migrate. _Avoid_: Final registration, committed registry + +**Create pass**: +The auto-migrate step that brings missing tables into existence — the table, its columns, its indexes and constraints, together. A table that already exists is left completely untouched by this pass, whatever its shape. +_Avoid_: Bootstrap, ensure-tables, table sync + +**Reconciliation pass**: +The `migrate_updates` step that alters existing tables to match the registered models — the only authority for DDL against a table that already exists. Within one table, column changes land before the indexes and constraints that reference them. +_Avoid_: Update pass, schema sync, drift repair + +**Ferro-owned artifact**: +An index or constraint whose name follows ferro's canonical naming (`idx_`, `uq_`, `fk_`, `ck_`), marking it as reconcilable: auto-migrate may create, rebuild, or drop it to match the declared model. Artifacts named any other way belong to the user and are never altered or dropped. +_Avoid_: Managed index, system constraint, internal index + +**Constraint rebuild**: +Drop-and-recreate of a ferro-owned constraint whose live definition no longer matches the declared model — a foreign key's `on_delete`, its target, its columns. Metadata-only: rows are never touched. On a backend that cannot alter constraints, ferro warns loudly and skips; it never diverges silently. +_Avoid_: Constraint alter, FK patch, in-place constraint update diff --git a/docs/adr/0010-create-pass-owns-only-missing-tables.md b/docs/adr/0010-create-pass-owns-only-missing-tables.md new file mode 100644 index 0000000..c284fc1 --- /dev/null +++ b/docs/adr/0010-create-pass-owns-only-missing-tables.md @@ -0,0 +1,39 @@ +# Auto-migrate pass ownership: the create pass creates missing tables; the reconciliation pass owns every existing table + +The auto-migrate **create pass** (`CONTEXT.md`) brings missing tables into +existence — table, columns, indexes, constraints, together — and leaves a +table that already exists completely untouched, whatever its shape. All DDL +against an existing table belongs to the **reconciliation pass** +(`migrate_updates`), which orders column changes before the indexes and +constraints that reference them. This makes the documented contract +("existing tables are left untouched unless `migrate_updates` is set") the +enforced one: previously the create pass also fired `CREATE INDEX IF NOT +EXISTS` at existing tables, which crashed the single-deploy shape of adding +columns plus a composite unique over them — the index DDL ran before the +reconciliation pass could add the columns (#324). + +Decision by owner (2026-07-19), grilling #324/#325. + +Rejected alternatives: + +- **Keep the create pass's index DDL, gated on "all referenced columns + exist"**: preserves two competing owners of index creation — the exact + divergence that produced #324 — and turns the crash into order-dependent + behavior (the index appears or not depending on which pass runs first). +- **Run the reconciliation pass before the create pass**: reconciliation + diffs against live tables and would race table creation for new tables; + the dependency between the passes is "create what's missing, then + reconcile what exists," not the reverse. + +## Consequences + +- Under plain `auto_migrate=True` (no `migrate_updates`), adding an index — + single-column or composite — to an already-live table no longer creates + it. That convenience was undocumented, accidental, and the crash vector; + index reconciliation on existing tables requires `migrate_updates=True`, + where it already lives correctly ordered. +- `create_tables()` shares the create pass and becomes literally "create + missing tables." +- The create pass must learn which tables exist (one introspection query up + front) instead of leaning on `IF NOT EXISTS` to paper over partial + application. diff --git a/docs/pages/concepts/architecture.md b/docs/pages/concepts/architecture.md index 8a8db43..096727c 100644 --- a/docs/pages/concepts/architecture.md +++ b/docs/pages/concepts/architecture.md @@ -203,7 +203,7 @@ await connect("sqlite::memory:", auto_migrate=True) ``` - `auto_migrate=True` creates missing tables for every registered model. -- `migrate_updates=True` (0.11.0) additionally adds missing columns to existing tables, and on PostgreSQL reconciles type and nullability drift. Runtime updates are planned from **SchemaIR** diffing (`ferro-migrate`) and executed as backend-specific DDL. +- `migrate_updates=True` (0.11.0) additionally adds missing columns to existing tables, and on PostgreSQL reconciles type drift, nullability drift, and foreign-key definition drift (`on_delete`, target) for ferro-owned constraints. Runtime updates are planned from **SchemaIR** diffing (`ferro-migrate`) and executed as backend-specific DDL. - `migrate_destructive=True` (0.11.0) additionally drops live columns no longer on the model (never whole tables). For renames, primary-key changes, and complex transforms, use the [Alembic bridge](../guide/migrations.md). **SchemaIR** is the canonical contract for cross-emitter DDL parity — runtime CREATE, auto-migrate, and Alembic `get_metadata()` all derive from the Python-compiled SchemaIR modelset and shared `ferro-ddl-lowering` tokens. Parity is enforced structurally (`test_cross_emitter_parity.py`, `test_db_type_cross_emitter_parity.py`, `test_migrate_plan.py`). The enriched JSON schema in the Rust registry remains the runtime source for query bind typing and hydration metadata. diff --git a/docs/pages/faq.md b/docs/pages/faq.md index 374faa8..6a6c7e6 100644 --- a/docs/pages/faq.md +++ b/docs/pages/faq.md @@ -38,7 +38,7 @@ SQLite and PostgreSQL (including hosted providers such as Supabase). MySQL and o Two tiers: -- **Auto-migration at connect time** — `connect(url, auto_migrate=True)` creates missing tables. As of 0.11.0, `migrate_updates=True` also adds missing columns (and reconciles type/nullability drift on PostgreSQL), and `migrate_destructive=True` drops model-removed columns. Great for development and simple deployments. +- **Auto-migration at connect time** — `connect(url, auto_migrate=True)` creates missing tables. As of 0.11.0, `migrate_updates=True` also adds missing columns (and reconciles type, nullability, and foreign-key `on_delete` drift on PostgreSQL), and `migrate_destructive=True` drops model-removed columns. Great for development and simple deployments. - **Alembic bridge** — for versioned, reviewable production migrations and anything auto-migrate can't express (renames, primary-key changes, complex transforms). Install with `pip install "ferro-orm[alembic]"`. See [Migrations](guide/migrations.md). diff --git a/docs/pages/guide/migrations.md b/docs/pages/guide/migrations.md index fbb52aa..66a5ed3 100644 --- a/docs/pages/guide/migrations.md +++ b/docs/pages/guide/migrations.md @@ -43,6 +43,8 @@ What it covers is capability-relative per backend: | Add composite index (`__ferro_composite_indexes__`) to existing columns | ✅ `CREATE INDEX` | ✅ `CREATE INDEX` | | Add unique column (`unique=True`) | ✅ via explicit unique index + warning | ✅ inline `UNIQUE` | | Add foreign-key column | ✅ column only, no FK constraint + warning | ✅ column + FK constraint | +| Add missing FK constraint to an existing column | ⚠️ `UserWarning`, no DDL | ✅ `ADD CONSTRAINT` | +| Change a foreign key's `on_delete` (or target) | ⚠️ `UserWarning`, no DDL | ✅ rebuild: `DROP CONSTRAINT` + `ADD CONSTRAINT` | | Change column type | ⚠️ `UserWarning`, no DDL (SQLite type affinity makes drift mostly cosmetic) | ✅ `ALTER COLUMN ... TYPE ... USING` cast | | Change nullability | ⚠️ `UserWarning`, no DDL | ✅ `SET NOT NULL` / `DROP NOT NULL` | | Drop orphaned Ferro-named index (`idx_*` / `uq_*`) | ✅ with `migrate_destructive=True` | ✅ with `migrate_destructive=True` | @@ -53,6 +55,7 @@ Rules worth knowing: - **NOT NULL additions need a literal default.** Existing rows must be backfilled, so a new required field without a literal default fails the connect with a clear error. Make it nullable, give it a default, or use Alembic. - **Added columns reuse the exact `CREATE TABLE` DDL**, so a database brought forward by `migrate_updates` matches one created fresh, and `alembic revision --autogenerate` stays clean afterwards. +- **Only ferro-owned constraints are rebuilt.** FK reconciliation matches the `fk_
__` names ferro emits (just as index reconciliation only touches `idx_*`/`uq_*`). A drifting constraint with any other name is left untouched and reported with a `UserWarning` — user-created schema survives auto-migrate. Rebuilding is metadata-only: rows are never touched, and the new `ADD CONSTRAINT` validates existing rows, failing loudly (and rolling back the table's plan on Postgres) if they violate it. - **Postgres type changes take an exclusive lock** and fail the connect if existing data does not cast cleanly — fine for a development flag, but worth knowing. - **The pool refreshes after any schema change**, so no cached statement or stale identity-mapped instance can observe the pre-migration schema. diff --git a/src/ferro/__init__.py b/src/ferro/__init__.py index 39059f4..b5263c5 100644 --- a/src/ferro/__init__.py +++ b/src/ferro/__init__.py @@ -216,8 +216,8 @@ async def connect( Args: url: The database connection string (e.g., "sqlite:example.db?mode=rwc"). auto_migrate: If True, automatically create tables for all registered models. - Existing tables are left untouched unless ``migrate_updates`` / - ``migrate_destructive`` are also set. + Existing tables are left completely untouched — whatever their shape — + unless ``migrate_updates`` / ``migrate_destructive`` are also set. name: Optional connection name. Omitted connections register as "default". default: If True, make this named connection the default for unqualified operations. pool: Optional per-connection pool configuration. @@ -239,9 +239,18 @@ async def connect( - **Postgres only**: column type changes (``ALTER COLUMN ... TYPE ... USING`` cast) and nullability changes (``SET/DROP NOT NULL``) when the live column disagrees with the model. + Also foreign-key reconciliation: a ferro-owned (``fk_``-named) + constraint whose definition drifts from the declared FK + (``on_delete``, target) is rebuilt (``DROP CONSTRAINT`` + + ``ADD CONSTRAINT``), and a declared FK missing entirely from an + existing column is added. A drifting constraint ferro does not + own is warned about, never altered. - **SQLite**: type/nullability drift cannot be changed in place; ferro emits a ``UserWarning`` naming the column and pointing at Alembic. (SQLite's type affinity makes declared-type drift mostly cosmetic.) + FK constraints likewise cannot be added or altered on an existing + table; any foreign-key drift warns loudly instead of diverging + silently. - **Transactionality**: on Postgres each table's migration plan runs inside a single transaction — a mid-plan failure rolls the table back to exactly its pre-migration state. On SQLite, @@ -284,7 +293,9 @@ async def connect( async def create_tables(using=None): """ - Manually create tables for all registered models on a connected engine. + Manually create the *missing* tables for registered models on a connected + engine. A table that already exists is left completely untouched; altering + existing tables belongs to ``migrate(updates=True)``. Compiles and pushes the current registry SchemaIR to the Rust runtime before delegating to the Rust create entrypoint, so a model defined after @@ -308,7 +319,8 @@ async def migrate(using=None, updates=True, destructive=False): Args: using: Named connection to migrate, or None for the default. - updates: If True (default), add missing columns and reconcile type/nullability drift. + updates: If True (default), add missing columns and reconcile + type, nullability, and foreign-key definition drift (see ``connect``). destructive: If True, also drop live columns absent from the model. Implies ``updates``. """ _ensure_rust_registration_synced()