diff --git a/AGENTS.md b/AGENTS.md index 01aaaf5..5e65701 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -85,6 +85,19 @@ For a single model, every emitter must agree on: executes the byte-identical statements. Pinned by `tests/test_table_check_rebuild.py`. See ADR-0015 (canonical render vs catalog, both through one normalizer) and ADR-0014 (SQLite warn-skip). +14. **Check leftovers / drops** — the leftover-name decision (which live + ferro-owned `ck_*` names a table carries that the model no longer + declares) and the rendered DROP are decided by ONE trio of functions: + `ferro_ddl_lowering::extra_check_names` / + `extra_check_names_warning` / `render_check_drop`. The auto-migrate + reconciliation pass consumes them through + `ferro_migrate::plan_check_drops` (ops only under + `migrate_destructive`; the warning always fires on `migrate_updates`). + The Alembic autogenerate comparator consumes them over FFI + (`_core._plan_check_drop`) with **no** destructive gate and executes + the byte-identical statements. Pinned by + `tests/test_table_check_orphans.py`. See ADR-0013 (leftover warning + + destructive ladder) and ADR-0014 (SQLite warn-skip). ### Why this invariant exists diff --git a/crates/ferro-ddl-lowering/src/lib.rs b/crates/ferro-ddl-lowering/src/lib.rs index fe44a48..5f6744f 100644 --- a/crates/ferro-ddl-lowering/src/lib.rs +++ b/crates/ferro-ddl-lowering/src/lib.rs @@ -949,6 +949,71 @@ fn declared_check_body(model: &ferro_schema_ir::SchemaModel, name: &str) -> Opti .map(render_check_body) } +/// Live ferro-owned CHECK names the model no longer declares, in live order +/// (ADR-0013, #345). +/// +/// Set-difference only: callers pass *already-filtered* ferro-owned names +/// (`LiveCheck.ferro_owned`, Alembic `ck_*`). This function does not inspect +/// the prefix — a user-owned name that leaked into `live_ferro_owned_names` +/// would be reported as extra. Missing names are an add +/// (`missing_check_names`, #343); same-name body drift is a rebuild +/// (`drifted_check_names`, #344). +pub fn extra_check_names( + declared_names: &[String], + live_ferro_owned_names: &[String], +) -> Vec { + live_ferro_owned_names + .iter() + .filter(|name| !declared_names.contains(name)) + .cloned() + .collect() +} + +/// The leftover-CHECK warning for one table, or `None` when nothing is extra. +/// Single-sourced like [`extra_enum_labels_warning`]: callers emit it verbatim, +/// never re-derive the wording. Names every leftover and points at +/// `migrate_destructive` / Alembic. +pub fn extra_check_names_warning(table: &str, extra: &[String]) -> Option { + if extra.is_empty() { + return None; + } + let listed: Vec = extra.iter().map(|name| format!("'{name}'")).collect(); + Some(format!( + "Table '{table}' has CHECK constraint(s) {} that the model no longer \ + declares. Leftover CHECKs keep rejecting rows the model now allows. \ + They stay in place unless you pass migrate_destructive=True (Postgres) \ + or drop them with a reviewed Alembic migration.", + listed.join(", "), + )) +} + +/// Render the DROP for one leftover CHECK constraint (ADR-0013, ADR-0014). +/// +/// Postgres: one `ALTER TABLE … DROP CONSTRAINT`. SQLite: no statement, a +/// warning that names the constraint and points at Alembic batch mode — SQLite +/// cannot drop a table constraint without a full table rebuild. +pub fn render_check_drop(table: &str, name: &str, dialect: Dialect) -> CheckEmission { + match dialect { + Dialect::Postgres => CheckEmission { + statement: Some(format!( + "ALTER TABLE {} DROP CONSTRAINT {}", + quote_ident(table), + quote_ident(name), + )), + warning: None, + }, + Dialect::Sqlite => CheckEmission { + statement: None, + warning: Some(format!( + "CHECK constraint '{name}' on table '{table}' is no longer declared, \ + and SQLite cannot drop a table constraint in place (it requires a \ + full table rebuild). The live constraint remains; use Alembic's \ + batch mode to drop it." + )), + }, + } +} + #[derive(Clone, Debug, PartialEq, Eq)] enum CheckToken { Word(String), @@ -1784,6 +1849,85 @@ mod tests { ); } + // Leftover ferro-owned CHECKs (#345; ADR-0013): live name not in the + // declared set. extra_check_names is a set-difference only — the + // ferro_owned / ck_* filter is the caller's (migrate.rs / Alembic). + + #[test] + fn extra_check_names_returns_undeclared_live_names_in_live_order() { + let declared = vec![ + "ck_transfer_at_most_one_outflow".to_string(), + "ck_transfer_kind".to_string(), + ]; + let live = vec![ + "ck_transfer_orphan".to_string(), + "ck_transfer_kind".to_string(), + "ck_transfer_old".to_string(), + ]; + assert_eq!( + extra_check_names(&declared, &live), + vec!["ck_transfer_orphan", "ck_transfer_old"] + ); + assert!(extra_check_names(&declared, &declared).is_empty()); + } + + #[test] + fn extra_check_names_is_a_set_difference_only() { + let declared = vec!["ck_transfer_kind".to_string()]; + let live = vec![ + "ck_transfer_kind".to_string(), + "transfer_positive_amount".to_string(), + ]; + assert_eq!( + extra_check_names(&declared, &live), + vec!["transfer_positive_amount"], + "prefix filtering is the caller's; this function does not drop non-ck_* names" + ); + assert!(extra_check_names(&declared, &[]).is_empty()); + } + + #[test] + fn extra_check_names_warning_is_pinned_and_names_every_leftover() { + assert_eq!( + extra_check_names_warning( + "transfer", + &[ + "ck_transfer_orphan".to_string(), + "ck_transfer_kind".to_string(), + ], + ), + Some( + "Table 'transfer' has CHECK constraint(s) 'ck_transfer_orphan', \ + 'ck_transfer_kind' that the model no longer declares. Leftover \ + CHECKs keep rejecting rows the model now allows. They stay in \ + place unless you pass migrate_destructive=True (Postgres) or \ + drop them with a reviewed Alembic migration." + .to_string() + ) + ); + assert_eq!(extra_check_names_warning("transfer", &[]), None); + } + + #[test] + fn render_check_drop_is_one_alter_on_postgres() { + let emission = render_check_drop("transfer", "ck_transfer_orphan", Dialect::Postgres); + assert_eq!( + emission.statement.as_deref(), + Some(r#"ALTER TABLE "transfer" DROP CONSTRAINT "ck_transfer_orphan""#) + ); + assert!(emission.warning.is_none()); + } + + #[test] + fn render_check_drop_warns_and_skips_on_sqlite() { + let emission = render_check_drop("transfer", "ck_transfer_orphan", Dialect::Sqlite); + assert!(emission.statement.is_none(), "ADR-0014: no SQLite ALTER"); + let warning = emission.warning.expect("SQLite must never skip silently"); + assert!(warning.contains("ck_transfer_orphan"), "{warning}"); + assert!(warning.contains("Alembic"), "{warning}"); + assert!(warning.contains("batch"), "{warning}"); + } + #[test] fn render_check_expr_covers_cmp_in_and_like() { use ferro_schema_ir::{CheckCmpOp, CheckExpr, CheckOperand}; diff --git a/crates/ferro-migrate/src/emit.rs b/crates/ferro-migrate/src/emit.rs index dc46ce7..0caccee 100644 --- a/crates/ferro-migrate/src/emit.rs +++ b/crates/ferro-migrate/src/emit.rs @@ -5,7 +5,8 @@ use ferro_ddl_lowering::{ self, ResolvedStorage, apply_canonical_type_for, canonical_from_schema_column, canonical_to_db_type_token, db_check_constraint_name, fk_action_from_str, fk_action_sql, fk_name, literal_default_value, pg_alter_type_target, quote_ident, refused_conversion, - refused_conversion_warning, render_check_addition, render_check_rebuild, render_db_check, + refused_conversion_warning, render_check_addition, render_check_drop, render_check_rebuild, + render_db_check, render_pg_enum_create_type, render_table_check_body, resolve_column_storage, single_index_name, single_unique_index_name, sqlite_declared_type, sqlite_type_storage_drift, }; @@ -788,6 +789,26 @@ pub fn emit_sql_with_ir( result.warnings.push(warning); } } + MigrationOp::DropCheck { table, name } => { + let model = find_model(&new_models, table)?; + let still_declared = model.table_checks.iter().any(|check| check.name == *name) + || model.checks.iter().any(|check| check.name == *name); + if still_declared { + return Err(EmissionError { + message: format!( + "Check-drop operation for '{name}' on table '{table}' is still \ + declared in the model IR" + ), + }); + } + let emission = render_check_drop(table, name, dialect); + if let Some(statement) = emission.statement { + result.statements.push(statement); + } + if let Some(warning) = emission.warning { + result.warnings.push(warning); + } + } MigrationOp::RebuildForeignKey { table, column, diff --git a/crates/ferro-migrate/src/lib.rs b/crates/ferro-migrate/src/lib.rs index 12b12ca..0171222 100644 --- a/crates/ferro-migrate/src/lib.rs +++ b/crates/ferro-migrate/src/lib.rs @@ -7,8 +7,8 @@ mod emit; mod order; use ferro_ddl_lowering::{ - drifted_check_names, fk_action_from_str, fk_action_sql, fk_name, is_ferro_fk_name, - missing_check_names, schema_columns_storage_drift, + drifted_check_names, extra_check_names, fk_action_from_str, fk_action_sql, fk_name, + is_ferro_fk_name, missing_check_names, schema_columns_storage_drift, }; use ferro_schema_ir::{IrEnvelope, SchemaIrPayload, SchemaModel}; use std::collections::{BTreeMap, BTreeSet}; @@ -128,6 +128,16 @@ pub enum MigrationOp { /// Canonical constraint name (`ck__`). name: String, }, + /// A live ferro-owned CHECK the model no longer declares (#345; + /// ADR-0013). Planned only under `migrate_destructive`; Alembic + /// autogenerate always proposes the drop. The name must *not* still + /// be declared — emitting a drop for a declared name is a loud error. + DropCheck { + /// Owning table. + table: String, + /// Live constraint name (`ck_
_`). + name: 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. @@ -216,7 +226,9 @@ pub fn emit_sql(plan: &MigrationPlan, dialect: Dialect) -> Vec { table, column )); } - MigrationOp::AddCheck { name, .. } | MigrationOp::RebuildCheck { name, .. } => { + MigrationOp::AddCheck { name, .. } + | MigrationOp::RebuildCheck { name, .. } + | MigrationOp::DropCheck { name, .. } => { sql.push(format!("-- check '{}' handled by emit_sql_with_ir", name)); } } @@ -335,6 +347,39 @@ pub fn plan_check_rebuilds( .collect() } +/// Plan the [`MigrationOp::DropCheck`] operations for one table (#345; +/// ADR-0013): every live ferro-owned CHECK name the model no longer +/// declares, in live order. +/// +/// Callers append the result **after** [`plan_check_rebuilds`]. The +/// `live_ferro_owned_names` slice is already filtered (`ferro_owned`); +/// [`extra_check_names`] is a set-difference only. Connect-time callers +/// gate the ops on `migrate_destructive`; the warning for leftovers is +/// planned separately so a non-destructive retain-filter cannot swallow it. +pub fn plan_check_drops( + table: &str, + new_ir: &IrEnvelope, + live_ferro_owned_names: &[String], +) -> Vec { + let new_models = index_models(&new_ir.payload.models); + let Some(new_model) = new_models.get(table) else { + return Vec::new(); + }; + let declared: Vec = new_model + .table_checks + .iter() + .map(|check| check.name.clone()) + .chain(new_model.checks.iter().map(|check| check.name.clone())) + .collect(); + extra_check_names(&declared, live_ferro_owned_names) + .into_iter() + .map(|name| MigrationOp::DropCheck { + table: table.to_string(), + name, + }) + .collect() +} + fn index_models<'a>(models: &'a [SchemaModel]) -> BTreeMap { let mut indexed = BTreeMap::new(); for model in models { diff --git a/crates/ferro-migrate/src/tests.rs b/crates/ferro-migrate/src/tests.rs index 0b02de4..a237f02 100644 --- a/crates/ferro-migrate/src/tests.rs +++ b/crates/ferro-migrate/src/tests.rs @@ -1768,6 +1768,94 @@ fn emit_sql_with_ir_rebuild_check_fails_loudly_for_an_undeclared_name() { assert!(err.message.contains("ck_transfer_nope"), "{}", err.message); } +// --------------------------------------------------------------------------- +// Leftover ferro-owned CHECKs (#345; ADR-0013): live name gone from the model. +// Planned after rebuilds. User-owned names never enter live_ferro_owned_names. +// --------------------------------------------------------------------------- + +#[test] +fn plan_check_drops_plans_live_ferro_owned_names_the_model_does_not_declare() { + let new_ir = envelope(vec![transfer_model_with_table_checks(vec![ + transfer_outflow_table_check(), + ])]); + let live = vec![ + "ck_transfer_orphan".to_string(), + "ck_transfer_at_most_one_outflow".to_string(), + "ck_transfer_old".to_string(), + ]; + assert_eq!( + plan_check_drops("transfer", &new_ir, &live), + vec![ + MigrationOp::DropCheck { + table: "transfer".to_string(), + name: "ck_transfer_orphan".to_string(), + }, + MigrationOp::DropCheck { + table: "transfer".to_string(), + name: "ck_transfer_old".to_string(), + }, + ] + ); +} + +#[test] +fn plan_check_drops_is_a_noop_when_every_live_name_is_declared() { + let new_ir = envelope(vec![transfer_model_with_table_checks(vec![ + transfer_outflow_table_check(), + ])]); + let live = vec!["ck_transfer_at_most_one_outflow".to_string()]; + assert!(plan_check_drops("transfer", &new_ir, &live).is_empty()); +} + +#[test] +fn emit_sql_with_ir_drop_check_drops_on_postgres_and_warns_on_sqlite() { + let new_ir = envelope(vec![transfer_model_with_table_checks(vec![])]); + let plan = MigrationPlan { + operations: vec![MigrationOp::DropCheck { + table: "transfer".to_string(), + name: "ck_transfer_orphan".to_string(), + }], + warnings: Vec::new(), + }; + + let pg = emit_sql_with_ir(&plan, &live_transfer_ir(), &new_ir, Dialect::Postgres).unwrap(); + assert_eq!( + pg.statements, + vec![r#"ALTER TABLE "transfer" DROP CONSTRAINT "ck_transfer_orphan""#.to_string()] + ); + assert!(pg.warnings.is_empty(), "{:?}", pg.warnings); + + let lite = emit_sql_with_ir(&plan, &live_transfer_ir(), &new_ir, Dialect::Sqlite).unwrap(); + assert!(lite.statements.is_empty(), "{:?}", lite.statements); + assert_eq!(lite.warnings.len(), 1); + assert!( + lite.warnings[0].contains("ck_transfer_orphan"), + "{}", + lite.warnings[0] + ); + assert!(lite.warnings[0].contains("Alembic"), "{}", lite.warnings[0]); +} + +#[test] +fn emit_sql_with_ir_drop_check_fails_loudly_for_a_still_declared_name() { + let new_ir = envelope(vec![transfer_model_with_table_checks(vec![ + transfer_outflow_table_check(), + ])]); + let plan = MigrationPlan { + operations: vec![MigrationOp::DropCheck { + table: "transfer".to_string(), + name: "ck_transfer_at_most_one_outflow".to_string(), + }], + warnings: Vec::new(), + }; + let err = emit_sql_with_ir(&plan, &live_transfer_ir(), &new_ir, Dialect::Postgres).unwrap_err(); + assert!( + err.message.contains("ck_transfer_at_most_one_outflow"), + "{}", + err.message + ); +} + #[test] fn render_check_body_quotes_column_and_joins_values() { let check = SchemaCheck { diff --git a/src/ferro/_core.pyi b/src/ferro/_core.pyi index 69ac30c..3b81a96 100644 --- a/src/ferro/_core.pyi +++ b/src/ferro/_core.pyi @@ -278,3 +278,17 @@ def _plan_check_rebuild( reconciliation pass executes (I-1). """ ... + +def _plan_check_drop( + table: str, model_ir_json: str, live_ferro_owned_names: list[str] +) -> str: + """The leftover-CHECK drop decision (ADR-0013) for one table. + + Returns JSON: ``{"statements": [...], "names": [...]}`` — the Rust-rendered + Postgres ``DROP CONSTRAINT`` statements for live ferro-owned CHECK names + the model no longer declares, plus those names. Byte-identical to what + the reconciliation pass executes under ``migrate_destructive`` (I-1). + There is no destructive gate here: running autogenerate is itself the + request for a diff. + """ + ... diff --git a/src/ferro/migrations/alembic.py b/src/ferro/migrations/alembic.py index 04e3331..f7a9c3a 100644 --- a/src/ferro/migrations/alembic.py +++ b/src/ferro/migrations/alembic.py @@ -10,6 +10,7 @@ from .._core import ( _ddl_fk_name, _plan_check_addition, + _plan_check_drop, _plan_check_rebuild, _plan_enum_label_addition, _render_check_body, @@ -409,24 +410,29 @@ def _compare_enum_labels(autogen_context, upgrade_ops, schemas) -> None: upgrade_ops.ops[:0] = drifted # ----------------------------------------------------------------------- - # Check-addition / check-rebuild comparator (ADR-0013, ADR-0015; - # CONTEXT.md *table check*, *column check*, *constraint rebuild*). + # Check-addition / check-rebuild / leftover-drop comparator (ADR-0013, + # ADR-0015; CONTEXT.md *table check*, *column check*, *constraint rebuild*). # # Alembic core does not compare CHECK constraints, so autogenerate against # a database whose table is missing a declared ``ck_*`` — or whose live - # body drifted — produces an empty revision and the invariant silently - # goes unenforced. This comparator is the mechanical consumer of both + # body drifted, or whose live ferro-owned ``ck_*`` the model no longer + # declares — produces an empty revision and the invariant silently goes + # unenforced. This comparator is the mechanical consumer of all three # decision tables (AGENTS.md § I-1): the diff AND the rendered statements # come from the Rust core over FFI (``_plan_check_addition``, - # ``_plan_check_rebuild``), byte-identical to what the auto-migrate - # reconciliation pass executes. There is no Python normalizer. + # ``_plan_check_rebuild``, ``_plan_check_drop``), byte-identical to what + # the auto-migrate reconciliation pass executes. There is no Python + # normalizer. # - # There is no ``migrate_updates`` gate here — running autogenerate is itself - # the request for a diff. Additions then rebuilds are appended after the - # revision's table ops so a CHECK over a newly added column lands after - # its ADD COLUMN. Postgres-only, like the reconciliation pass: on SQLite, - # adding or rebuilding a table constraint needs a full table rebuild, - # which is Alembic's batch-mode door (ADR-0014). + # There is no ``migrate_updates`` / ``migrate_destructive`` gate here — + # running autogenerate is itself the request for a diff. The destructive + # flag is connect-time safety only (ADR-0013 / ADR-0011: parity is the + # SQL, not the flag). Additions then rebuilds then leftover drops are + # appended after the revision's table ops so a CHECK over a newly added + # column lands after its ADD COLUMN. Postgres-only, like the + # reconciliation pass: on SQLite, adding, rebuilding, or dropping a table + # constraint needs a full table rebuild, which is Alembic's batch-mode + # door (ADR-0014). # ----------------------------------------------------------------------- class FerroCheckConstraintsOp(_MigrateOperation): @@ -512,6 +518,7 @@ def _compare_check_constraints(autogen_context, upgrade_ops, schemas) -> None: # (which already render their CHECKs inline). additions = [] rebuilds = [] + drops = [] for model_ir in models: if not isinstance(model_ir, dict): continue @@ -540,8 +547,21 @@ def _compare_check_constraints(autogen_context, upgrade_ops, schemas) -> None: table_name, rebuild_plan["statements"], rebuild_plan["names"] ) ) + live_ferro_owned = [ + name for name, _ in live_checks if name.startswith("ck_") + ] + drop_plan = json.loads( + _plan_check_drop(table_name, json.dumps(model_ir), live_ferro_owned) + ) + if drop_plan["statements"]: + drops.append( + FerroCheckDropOp( + table_name, drop_plan["statements"], drop_plan["names"] + ) + ) upgrade_ops.ops.extend(additions) upgrade_ops.ops.extend(rebuilds) + upgrade_ops.ops.extend(drops) class FerroCheckRebuildOp(_MigrateOperation): """Autogenerate carrier for one table's drifted CHECK bodies. @@ -574,6 +594,37 @@ def reverse(self) -> "FerroCheckRebuildOp": # is a reviewed edit, not an autogenerated downgrade. return FerroCheckRebuildOp(self.table_name, [], self.names) + class FerroCheckDropOp(_MigrateOperation): + """Autogenerate carrier for one table's leftover ferro-owned CHECKs. + + Renders to plain ``op.execute`` of the Rust-rendered DROP (I-1) — a + generated revision does not import ferro to run. There is no + ``migrate_destructive`` gate: running autogenerate is itself the + request for a diff. + """ + + def __init__( + self, + table_name: str, + statements: list[str], + names: list[str], + ) -> None: + self.table_name = table_name + self.statements = statements + self.names = names + + def to_diff_tuple(self): + return ( + "ferro_drop_check_constraints", + self.table_name, + tuple(self.names), + ) + + def reverse(self) -> "FerroCheckDropOp": + # Recreating the leftover body is a reviewed edit, not an + # autogenerated downgrade. + return FerroCheckDropOp(self.table_name, [], self.names) + @_alembic_renderers.dispatch_for(FerroCheckConstraintsOp) def _render_check_constraints( autogen_context, op: FerroCheckConstraintsOp @@ -589,6 +640,10 @@ def _render_check_constraints( def _render_check_rebuilds(autogen_context, op: FerroCheckRebuildOp) -> list[str]: return [f"op.execute({stmt!r})" for stmt in op.statements] + @_alembic_renderers.dispatch_for(FerroCheckDropOp) + def _render_check_drops(autogen_context, op: FerroCheckDropOp) -> list[str]: + return [f"op.execute({stmt!r})" for stmt in op.statements] + @_alembic_renderers.dispatch_for(AddEnumLabelsOp) def _render_add_enum_labels(autogen_context, op: AddEnumLabelsOp) -> list[str]: lines: list[str] = [] diff --git a/src/lib.rs b/src/lib.rs index 3357487..a46931f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -161,6 +161,7 @@ fn _core(m: &Bound<'_, PyModule>) -> PyResult<()> { m.add_function(wrap_pyfunction!(naming_ffi::_plan_enum_label_addition, m)?)?; m.add_function(wrap_pyfunction!(naming_ffi::_plan_check_addition, m)?)?; m.add_function(wrap_pyfunction!(naming_ffi::_plan_check_rebuild, m)?)?; + m.add_function(wrap_pyfunction!(naming_ffi::_plan_check_drop, m)?)?; m.add_function(wrap_pyfunction!( introspect::_live_table_checks_for_test, m diff --git a/src/migrate.rs b/src/migrate.rs index 370805d..5e2fdf0 100644 --- a/src/migrate.rs +++ b/src/migrate.rs @@ -13,11 +13,14 @@ use crate::backend::EngineHandle; use ferro_ddl_lowering::{ - Dialect, ResolvedStorage, extra_enum_labels, extra_enum_labels_warning, - information_schema_to_db_type_token, missing_enum_labels, render_pg_enum_add_value, - resolve_column_storage, + Dialect, ResolvedStorage, extra_check_names, extra_check_names_warning, extra_enum_labels, + extra_enum_labels_warning, information_schema_to_db_type_token, missing_enum_labels, + render_pg_enum_add_value, resolve_column_storage, +}; +use ferro_migrate::{ + MigrationOp, emit_sql_with_ir, plan_check_drops, plan_check_rebuilds, plan_from_ir, + plan_missing_checks, }; -use ferro_migrate::{MigrationOp, emit_sql_with_ir, plan_check_rebuilds, plan_from_ir, plan_missing_checks}; use ferro_schema_ir::{ IrEnvelope, SchemaCheck, SchemaColumn, SchemaForeignKey, SchemaIndex, SchemaIrPayload, SchemaModel, SchemaUnique, @@ -305,6 +308,40 @@ pub fn plan_table_migration( new_ir, &live_for_rebuild, )); + // Leftovers (#345; ADR-0013): live ferro-owned names the model does not + // declare. Always warn (silence is wrong — leftover CHECKs keep rejecting + // rows the model now allows). DropCheck ops only when destructive — do + // not piggy-back on the DropIndex retain-filter, which would also + // swallow this warning on a warning-only plan. + let live_ferro_owned_names: Vec = live_for_rebuild + .iter() + .map(|(name, _)| name.clone()) + .collect(); + let declared_check_names: Vec = new_ir + .payload + .models + .iter() + .find(|model| model.table_name == table_lower) + .map(|model| { + model + .table_checks + .iter() + .map(|check| check.name.clone()) + .chain(model.checks.iter().map(|check| check.name.clone())) + .collect() + }) + .unwrap_or_default(); + let extras = extra_check_names(&declared_check_names, &live_ferro_owned_names); + if let Some(warning) = extra_check_names_warning(table_lower, &extras) { + typed_plan.warnings.push(warning); + } + if opts.destructive { + typed_plan.operations.extend(plan_check_drops( + table_lower, + new_ir, + &live_ferro_owned_names, + )); + } if !opts.destructive { typed_plan diff --git a/src/naming_ffi.rs b/src/naming_ffi.rs index f394622..542a35f 100644 --- a/src/naming_ffi.rs +++ b/src/naming_ffi.rs @@ -179,6 +179,43 @@ pub fn _plan_check_rebuild( Ok(serde_json::json!({ "statements": statements, "names": names }).to_string()) } +/// The leftover-CHECK drop decision over FFI (ADR-0013): given one model's +/// compiled SchemaIR and the live ferro-owned CHECK names, return the +/// Rust-rendered Postgres `DROP CONSTRAINT` statements (in live order) and +/// the names they drop. +/// +/// The Alembic autogenerate comparator consumes this instead of re-deriving +/// the diff or re-rendering the SQL (AGENTS.md § I-1). Postgres-only +/// (ADR-0014). There is no `migrate_destructive` gate — running autogenerate +/// is itself the request for a diff; the destructive flag is connect-time +/// safety only. +#[pyfunction] +pub fn _plan_check_drop( + table: String, + model_ir_json: String, + live_ferro_owned_names: Vec, +) -> PyResult { + let model: ferro_schema_ir::SchemaModel = + serde_json::from_str(&model_ir_json).map_err(|e| { + pyo3::exceptions::PyValueError::new_err(format!("Invalid SchemaIR model: {e}")) + })?; + let declared: Vec = model + .table_checks + .iter() + .map(|check| check.name.clone()) + .chain(model.checks.iter().map(|check| check.name.clone())) + .collect(); + let names = ferro_ddl_lowering::extra_check_names(&declared, &live_ferro_owned_names); + let mut statements = Vec::with_capacity(names.len()); + for name in &names { + let emission = ferro_ddl_lowering::render_check_drop(&table, name, Dialect::Postgres); + if let Some(statement) = emission.statement { + statements.push(statement); + } + } + Ok(serde_json::json!({ "statements": statements, "names": names }).to_string()) +} + /// Render the shared `db_check` CHECK body (`"col" IN (v1, v2, ...)`) — /// byte-identical to the Rust emitters. `values` arrive pre-rendered (quoted) /// from the IR compiler. diff --git a/tests/test_table_check_orphans.py b/tests/test_table_check_orphans.py new file mode 100644 index 0000000..1b30b05 --- /dev/null +++ b/tests/test_table_check_orphans.py @@ -0,0 +1,546 @@ +"""Leftover ferro-owned CHECKs (#345): a live ``ck_*`` the model no longer +declares stays under ``migrate_updates`` (with a warning that names it) and +drops under ``migrate_destructive`` on Postgres. + +ADR-0013: leftover CHECKs keep rejecting rows the model now allows, so +silence (the index-orphan behavior) is not acceptable. User-created CHECKs +(any other name) are never touched. ADR-0014 makes the drop Postgres-only: +SQLite warns with the constraint name and leaves the live body. + +Adding a missing name is #343; same-name body drift is #344. Neither is +exercised here. +""" + +import json +from enum import StrEnum +from typing import Annotated, ClassVar + +import pytest + +from ferro import ( + Check, + CheckViolationError, + Field, + Model, + clear_registry, + connect, + engines, + reset_engine, +) +from ferro._core import _plan_check_drop, _render_migration_sql_for_test +from ferro.ir.compiler import compile_registry_schema_ir +from ferro.raw import execute, fetch_all + +SIDE_CHECK_NAME = "ck_orphan_at_most_one_side" +SIDE_CHECK_BODY = '("left" IS NULL) OR ("right" IS NULL)' +SIDE_CHECK_DROP = f'ALTER TABLE "orphan" DROP CONSTRAINT "{SIDE_CHECK_NAME}"' +USER_CHECK_NAME = "orphan_left_not_blank" +COOKIE_CHECK_NAME = "ck_cookie_flavor" +COOKIE_CHECK_DROP = f'ALTER TABLE "cookie" DROP CONSTRAINT "{COOKIE_CHECK_NAME}"' + + +def _rewind_registry() -> None: + """Drop every registered model so the same table can be redeclared.""" + from ferro.registry import REGISTRY + + reset_engine() + clear_registry() + REGISTRY.reset_for_test() + + +@pytest.fixture(autouse=True) +def cleanup_registry(): + _rewind_registry() + yield + _rewind_registry() + + +# --------------------------------------------------------------------------- +# Model shapes +# --------------------------------------------------------------------------- + + +class Flavor(StrEnum): + SWEET = "sweet" + SALTY = "salty" + + +def _or_check() -> Check: + return Check( + "at_most_one_side", + lambda orphan: ( + (orphan.left == None) # noqa: E711 + | (orphan.right == None) + ), # noqa: E711 + ) + + +def _define_orphan(*, with_check: bool) -> type[Model]: + class Orphan(Model): + if with_check: + __ferro_checks__: ClassVar[tuple[Check, ...]] = (_or_check(),) + + id: int | None = Field(default=None, primary_key=True) + left: str | None = None + right: str | None = None + + return Orphan + + +def _define_cookie(*, db_check: bool) -> type[Model]: + class Cookie(Model): + id: int | None = Field(default=None, primary_key=True) + flavor: Annotated[Flavor, Field(db_type="text", db_check=db_check)] = ( + Flavor.SWEET + ) + + return Cookie + + +ORPHAN_LIVE_COLUMNS = [ + { + "name": "id", + "declared_type": "integer", + "is_primary_key": True, + "is_nullable": False, + }, + {"name": "left", "declared_type": "varchar", "is_nullable": True}, + {"name": "right", "declared_type": "varchar", "is_nullable": True}, +] + + +def _render( + table: str, + live_columns: list[dict], + live_checks: list[dict], + dialect: str, + *, + updates: bool = True, + destructive: bool = False, +) -> tuple[list[str], list[str]]: + return _render_migration_sql_for_test( + table, + json.dumps(compile_registry_schema_ir()), + json.dumps(live_columns), + dialect, + updates, + destructive, + "", + "", + json.dumps(live_checks), + ) + + +def _live_check(name: str, definition: str, *, ferro_owned: bool = True) -> dict: + return {"name": name, "definition": definition, "ferro_owned": ferro_owned} + + +def _model_ir(table: str) -> dict: + return next( + model + for model in compile_registry_schema_ir()["payload"]["models"] + if model["table_name"] == table + ) + + +# --------------------------------------------------------------------------- +# Render level +# --------------------------------------------------------------------------- + + +def test_leftover_table_check_warns_and_stays_under_migrate_updates(): + _define_orphan(with_check=False) + live = [_live_check(SIDE_CHECK_NAME, f"CHECK ({SIDE_CHECK_BODY})")] + for dialect in ("postgres", "sqlite"): + statements, warnings = _render("orphan", ORPHAN_LIVE_COLUMNS, live, dialect) + assert statements == [], dialect + assert len(warnings) == 1, (dialect, warnings) + assert SIDE_CHECK_NAME in warnings[0] + assert "migrate_destructive" in warnings[0] + assert "Alembic" in warnings[0] + + +def test_leftover_table_check_drops_under_migrate_destructive_on_postgres(): + _define_orphan(with_check=False) + live = [_live_check(SIDE_CHECK_NAME, f"CHECK ({SIDE_CHECK_BODY})")] + statements, warnings = _render( + "orphan", ORPHAN_LIVE_COLUMNS, live, "postgres", destructive=True + ) + assert statements == [SIDE_CHECK_DROP] + assert len(warnings) == 1 + assert SIDE_CHECK_NAME in warnings[0] + + +def test_leftover_table_check_warns_and_skips_on_sqlite_even_when_destructive(): + _define_orphan(with_check=False) + live = [_live_check(SIDE_CHECK_NAME, f"CHECK ({SIDE_CHECK_BODY})")] + statements, warnings = _render( + "orphan", ORPHAN_LIVE_COLUMNS, live, "sqlite", destructive=True + ) + assert statements == [] + assert any(SIDE_CHECK_NAME in warning for warning in warnings) + assert any("Alembic" in warning for warning in warnings) + + +def test_clearing_db_check_follows_the_same_warn_and_drop_rule(): + _define_cookie(db_check=False) + live_columns = [ + { + "name": "id", + "declared_type": "integer", + "is_primary_key": True, + "is_nullable": False, + }, + {"name": "flavor", "declared_type": "text", "is_nullable": False}, + ] + live = [_live_check(COOKIE_CHECK_NAME, "CHECK (\"flavor\" IN ('sweet', 'salty'))")] + statements, warnings = _render("cookie", live_columns, live, "postgres") + assert statements == [] + assert any(COOKIE_CHECK_NAME in warning for warning in warnings) + + statements, _ = _render("cookie", live_columns, live, "postgres", destructive=True) + assert statements == [COOKIE_CHECK_DROP] + + +def test_user_owned_live_check_is_never_warned_or_dropped(): + _define_orphan(with_check=False) + live = [ + _live_check( + USER_CHECK_NAME, + "CHECK ((\"left\" <> ''))", + ferro_owned=False, + ) + ] + for dialect in ("postgres", "sqlite"): + for destructive in (False, True): + statements, warnings = _render( + "orphan", + ORPHAN_LIVE_COLUMNS, + live, + dialect, + destructive=destructive, + ) + assert statements == [], (dialect, destructive) + assert warnings == [], (dialect, destructive, warnings) + assert not any(USER_CHECK_NAME in sql for sql in statements) + + +def test_declared_check_is_not_a_leftover(): + _define_orphan(with_check=True) + live = [_live_check(SIDE_CHECK_NAME, f"CHECK ({SIDE_CHECK_BODY})")] + for dialect in ("postgres", "sqlite"): + statements, warnings = _render( + "orphan", ORPHAN_LIVE_COLUMNS, live, dialect, destructive=True + ) + assert statements == [], dialect + assert warnings == [], dialect + + +def test_without_migrate_updates_no_leftover_is_planned(): + _define_orphan(with_check=False) + live = [_live_check(SIDE_CHECK_NAME, f"CHECK ({SIDE_CHECK_BODY})")] + for dialect in ("postgres", "sqlite"): + statements, warnings = _render( + "orphan", + ORPHAN_LIVE_COLUMNS, + live, + dialect, + updates=False, + ) + assert statements == [], dialect + assert warnings == [], dialect + + +# --------------------------------------------------------------------------- +# Cross-emitter parity (AGENTS.md § I-1) +# --------------------------------------------------------------------------- + + +def test_check_drop_statement_parity_pin(): + """The FFI the Alembic comparator consumes renders the same bytes the + reconciliation pass executes under ``migrate_destructive``.""" + _define_orphan(with_check=False) + live_names = [SIDE_CHECK_NAME] + plan = json.loads( + _plan_check_drop("orphan", json.dumps(_model_ir("orphan")), live_names) + ) + assert plan["names"] == [SIDE_CHECK_NAME] + assert plan["statements"] == [SIDE_CHECK_DROP] + + runtime, _ = _render( + "orphan", + ORPHAN_LIVE_COLUMNS, + [_live_check(SIDE_CHECK_NAME, f"CHECK ({SIDE_CHECK_BODY})")], + "postgres", + destructive=True, + ) + assert plan["statements"] == runtime + + +# --------------------------------------------------------------------------- +# Live behavior +# --------------------------------------------------------------------------- + + +async def _pg_check_names(table: str) -> set[str]: + rows = await fetch_all( + "SELECT conname FROM pg_constraint " + f"WHERE conrelid = '\"{table}\"'::regclass AND contype = 'c'" + ) + return {row["conname"] for row in rows} + + +@pytest.mark.backend_matrix +@pytest.mark.postgres_only +@pytest.mark.asyncio +async def test_migrate_updates_leaves_a_removed_table_check_and_warns(db_url): + Orphan = _define_orphan(with_check=True) + await connect(db_url, auto_migrate=True) + async with engines.session(): + await Orphan.create(left=None, right=None) + assert SIDE_CHECK_NAME in await _pg_check_names("orphan") + _rewind_registry() + + _define_orphan(with_check=False) + with pytest.warns(UserWarning, match=SIDE_CHECK_NAME) as record: + await connect(db_url, migrate_updates=True) + named = [w for w in record if SIDE_CHECK_NAME in str(w.message)] + assert len(named) == 1 + assert "migrate_destructive" in str(named[0].message) + + async with engines.session(): + assert SIDE_CHECK_NAME in await _pg_check_names("orphan") + + +@pytest.mark.backend_matrix +@pytest.mark.postgres_only +@pytest.mark.asyncio +async def test_migrate_destructive_drops_the_orphaned_table_check(db_url): + Orphan = _define_orphan(with_check=True) + await connect(db_url, auto_migrate=True) + async with engines.session(): + await Orphan.create(left="a", right=None) + assert SIDE_CHECK_NAME in await _pg_check_names("orphan") + _rewind_registry() + + Orphan = _define_orphan(with_check=False) + with pytest.warns(UserWarning, match=SIDE_CHECK_NAME): + await connect(db_url, migrate_destructive=True) + async with engines.session(): + assert SIDE_CHECK_NAME not in await _pg_check_names("orphan") + row = await Orphan.create(left="c", right="d") + assert row.id is not None + + +@pytest.mark.backend_matrix +@pytest.mark.postgres_only +@pytest.mark.asyncio +async def test_clearing_db_check_warns_then_drops_on_destructive(db_url): + Cookie = _define_cookie(db_check=True) + await connect(db_url, auto_migrate=True) + async with engines.session(): + await Cookie.create(flavor=Flavor.SWEET) + assert COOKIE_CHECK_NAME in await _pg_check_names("cookie") + _rewind_registry() + + _define_cookie(db_check=False) + with pytest.warns(UserWarning, match=COOKIE_CHECK_NAME): + await connect(db_url, migrate_updates=True) + async with engines.session(): + assert COOKIE_CHECK_NAME in await _pg_check_names("cookie") + + _rewind_registry() + Cookie = _define_cookie(db_check=False) + with pytest.warns(UserWarning, match=COOKIE_CHECK_NAME): + await connect(db_url, migrate_destructive=True) + async with engines.session(): + assert COOKIE_CHECK_NAME not in await _pg_check_names("cookie") + await execute('INSERT INTO "cookie" ("flavor") VALUES (\'sour\')') + + +@pytest.mark.backend_matrix +@pytest.mark.postgres_only +@pytest.mark.asyncio +async def test_user_created_non_ck_check_survives_both_flags(db_url, recwarn): + _define_orphan(with_check=True) + await connect(db_url, auto_migrate=True) + async with engines.session(): + await execute( + f'ALTER TABLE "orphan" ADD CONSTRAINT "{USER_CHECK_NAME}" ' + "CHECK ((\"left\" IS DISTINCT FROM ''))" + ) + assert USER_CHECK_NAME in await _pg_check_names("orphan") + _rewind_registry() + + _define_orphan(with_check=True) + recwarn.clear() + await connect(db_url, migrate_updates=True) + assert not [w for w in recwarn if USER_CHECK_NAME in str(w.message)] + async with engines.session(): + assert USER_CHECK_NAME in await _pg_check_names("orphan") + + _rewind_registry() + recwarn.clear() + _define_orphan(with_check=True) + await connect(db_url, migrate_destructive=True) + assert not [w for w in recwarn if USER_CHECK_NAME in str(w.message)] + async with engines.session(): + assert USER_CHECK_NAME in await _pg_check_names("orphan") + assert SIDE_CHECK_NAME in await _pg_check_names("orphan") + + +@pytest.mark.backend_matrix +@pytest.mark.postgres_only +@pytest.mark.asyncio +async def test_second_updates_boot_does_not_remove_the_leftover(db_url, recwarn): + Orphan = _define_orphan(with_check=True) + await connect(db_url, auto_migrate=True) + async with engines.session(): + await Orphan.create(left=None, right=None) + _rewind_registry() + + _define_orphan(with_check=False) + await connect(db_url, migrate_updates=True) + recwarn.clear() + + _rewind_registry() + _define_orphan(with_check=False) + with pytest.warns(UserWarning, match=SIDE_CHECK_NAME): + await connect(db_url, migrate_updates=True) + async with engines.session(): + assert SIDE_CHECK_NAME in await _pg_check_names("orphan") + + +@pytest.mark.backend_matrix +@pytest.mark.sqlite_only +@pytest.mark.asyncio +async def test_sqlite_leftover_warns_and_rewrites_nothing(db_url): + Orphan = _define_orphan(with_check=True) + await connect(db_url, auto_migrate=True) + async with engines.session(): + await Orphan.create(left=None, right=None) + before = ( + await fetch_all( + "SELECT sql FROM sqlite_master WHERE type = 'table' AND name = 'orphan'" + ) + )[0]["sql"] + _rewind_registry() + + _define_orphan(with_check=False) + with pytest.warns(UserWarning, match=SIDE_CHECK_NAME) as record: + await connect(db_url, migrate_updates=True) + named = [w for w in record if SIDE_CHECK_NAME in str(w.message)] + assert named, "silence is wrong for leftover CHECKs" + + async with engines.session(): + after = ( + await fetch_all( + "SELECT sql FROM sqlite_master WHERE type = 'table' AND name = 'orphan'" + ) + )[0]["sql"] + assert after == before, "no table rebuild, no ALTER" + # The leftover OR body still holds. + with pytest.raises(CheckViolationError): + await execute( + 'INSERT INTO "orphan" ("left", "right") VALUES (\'a\', \'b\')' + ) + + +@pytest.mark.backend_matrix +@pytest.mark.sqlite_only +@pytest.mark.asyncio +async def test_sqlite_destructive_still_skips_the_drop(db_url): + _define_orphan(with_check=True) + await connect(db_url, auto_migrate=True) + async with engines.session(): + before = ( + await fetch_all( + "SELECT sql FROM sqlite_master WHERE type = 'table' AND name = 'orphan'" + ) + )[0]["sql"] + _rewind_registry() + + _define_orphan(with_check=False) + with pytest.warns(UserWarning, match=SIDE_CHECK_NAME): + await connect(db_url, migrate_destructive=True) + async with engines.session(): + after = ( + await fetch_all( + "SELECT sql FROM sqlite_master WHERE type = 'table' AND name = 'orphan'" + ) + )[0]["sql"] + assert after == before + + +# --------------------------------------------------------------------------- +# Alembic autogenerate +# --------------------------------------------------------------------------- + + +def _autogen_upgrade_code(postgres_base_url, db_schema_name) -> str: + import sqlalchemy as sa + from alembic.autogenerate import produce_migrations, render_python_code + from alembic.migration import MigrationContext + + from ferro.migrations import get_metadata + + metadata = get_metadata() + for scheme in ("postgresql://", "postgres://"): + if postgres_base_url.startswith(scheme): + sync_url = "postgresql+psycopg://" + postgres_base_url[len(scheme) :] + break + else: + sync_url = postgres_base_url + engine = sa.create_engine(sync_url) + try: + with engine.connect() as conn: + conn.execute(sa.text(f'SET search_path TO "{db_schema_name}"')) + ctx = MigrationContext.configure( + conn, opts={"compare_type": True, "compare_server_default": True} + ) + script = produce_migrations(ctx, metadata) + return render_python_code(script.upgrade_ops) + finally: + engine.dispose() + + +@pytest.mark.backend_matrix +@pytest.mark.postgres_only +@pytest.mark.asyncio +async def test_autogenerate_proposes_the_runtime_drop_after_a_non_destructive_connect( + db_url, postgres_base_url, db_schema_name +): + Orphan = _define_orphan(with_check=True) + await connect(db_url, auto_migrate=True) + async with engines.session(): + await Orphan.create(left=None, right=None) + _rewind_registry() + + _define_orphan(with_check=False) + with pytest.warns(UserWarning, match=SIDE_CHECK_NAME): + await connect(db_url, migrate_updates=True) + + code = _autogen_upgrade_code(postgres_base_url, db_schema_name) + assert SIDE_CHECK_DROP in code, code + assert "import ferro" not in code, code + + +@pytest.mark.backend_matrix +@pytest.mark.postgres_only +@pytest.mark.asyncio +async def test_autogenerate_is_empty_once_the_leftover_is_dropped( + db_url, postgres_base_url, db_schema_name +): + Orphan = _define_orphan(with_check=True) + await connect(db_url, auto_migrate=True) + async with engines.session(): + await Orphan.create(left=None, right=None) + _rewind_registry() + + _define_orphan(with_check=False) + with pytest.warns(UserWarning, match=SIDE_CHECK_NAME): + await connect(db_url, migrate_destructive=True) + + code = _autogen_upgrade_code(postgres_base_url, db_schema_name) + assert SIDE_CHECK_DROP not in code, code + assert SIDE_CHECK_NAME not in code, code