From 62f09bebc501dd81ade577a57019ea0c524ed0d9 Mon Sep 17 00:00:00 2001 From: "claude@clouddev1" Date: Fri, 22 May 2026 19:17:43 +0000 Subject: [PATCH] db: fix self-referential cascade over-count + SQL-delete render test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A self-referential ON DELETE CASCADE FK (e.g. T.ParentId -> T.id) is returned by read_relationships_inbound as a child whose table IS the delete target. The before/after row-count diff then includes the directly-deleted rows (already in rows_affected), so deleting a chain root reported 3 cascaded rows when only 2 were removed via the self-reference. Fix in both do_delete (DSL) and do_sql_delete (SQL): when the child table equals the target, subtract rows_affected from the diff and guard on the corrected count (a leaf delete no longer reports a phantom 0-row self-cascade); the target's CSV is already queued, so a self-ref child is not re-added to rewritten_tables. Pre-existing in do_delete; surfaced by the 3f DA pass, fixed in both paths to keep DSL/SQL parity. Behaviour: report only the rows removed via the self-reference (user-confirmed). Also adds an app-level render test for the SQL DELETE path (handle_dsl_delete_success via CommandOutcome::Delete) — the shared renderer's ok-summary + per-relationship cascade line were exercised only through the DSL path before. Test-first: self_referential_cascade_counts_only_cascaded_rows added for both paths (asserted 2, failed at 3 before the fix). 1545 pass / 0 fail / 1 ignored. Clippy clean. --- src/app.rs | 42 ++++++++++++++++ src/db.rs | 114 ++++++++++++++++++++++++++++++++++++++++---- tests/sql_delete.rs | 34 +++++++++++++ 3 files changed, 182 insertions(+), 8 deletions(-) diff --git a/src/app.rs b/src/app.rs index e44d030..4ab0802 100644 --- a/src/app.rs +++ b/src/app.rs @@ -3413,4 +3413,46 @@ mod tests { "header row rendered: {texts:?}", ); } + + #[test] + fn sql_delete_success_renders_count_and_cascade_summary() { + // ADR-0033 sub-phase 3f: a SQL DELETE reuses the DSL delete + // renderer (CommandOutcome::Delete -> handle_dsl_delete_ + // success). This pins that the SHARED renderer produces the + // right user-facing strings for the SQL path — the ok-summary + // (verb + subject, where SqlDelete's subject is its target + // table) and the per-relationship cascade line. The integration + // tests check the DeleteResult struct; this checks the render. + use crate::dsl::ReferentialAction; + let mut app = App::new(); + app.update(AppEvent::DslDeleteSucceeded { + command: Command::SqlDelete { + sql: "delete from Customers where id = 1".to_string(), + target_table: "Customers".to_string(), + }, + result: crate::db::DeleteResult { + rows_affected: 1, + cascade: vec![crate::db::CascadeEffect { + relationship_name: "places".to_string(), + child_table: "Orders".to_string(), + rows_changed: 2, + action: ReferentialAction::Cascade, + }], + }, + }); + let texts: Vec = app.output.iter().map(|l| l.text.clone()).collect(); + assert!( + texts.iter().any(|t| t.contains("delete from") && t.contains("Customers")), + "ok summary names the verb + target table: {texts:?}", + ); + assert!( + texts.iter().any(|t| t.contains("1 row(s) deleted")), + "directly-deleted count surfaced: {texts:?}", + ); + assert!( + texts.iter().any(|t| t.contains("2 row(s) deleted in `Orders`") + && t.contains("relationship `places`")), + "per-relationship cascade summary surfaced: {texts:?}", + ); + } } diff --git a/src/db.rs b/src/db.rs index becbb9b..27b4f6c 100644 --- a/src/db.rs +++ b/src/db.rs @@ -5790,15 +5790,28 @@ fn do_delete( let mut rewritten_tables: Vec = vec![table.to_string()]; for (rel, (_child_table, before_count)) in inbound.iter().zip(before_counts.iter()) { let after_count = count_rows(conn, &rel.other_table)?; - let diff = before_count - after_count; - if diff > 0 { + let mut rows_changed = before_count - after_count; + // A self-referential FK (child == target): the before/after + // diff also covers the directly-deleted rows, which are + // already reported in `rows_affected` and are not cascade + // effects. Subtract them so the summary reports only the rows + // removed *via* the self-reference. Shared with `do_sql_delete`. + let self_referential = rel.other_table == table; + if self_referential { + rows_changed -= rows_affected as i64; + } + if rows_changed > 0 { cascade.push(CascadeEffect { relationship_name: rel.name.clone(), child_table: rel.other_table.clone(), - rows_changed: diff, + rows_changed, action: rel.on_delete, }); - rewritten_tables.push(rel.other_table.clone()); + // The target's CSV is already queued; only add a distinct + // child table (a self-ref child is the target itself). + if !self_referential { + rewritten_tables.push(rel.other_table.clone()); + } } } @@ -6179,15 +6192,28 @@ fn do_sql_delete( let mut rewritten_tables: Vec = vec![target_table.to_string()]; for (rel, before_count) in inbound.iter().zip(before_counts.iter()) { let after_count = count_rows(conn, &rel.other_table)?; - let diff = before_count - after_count; - if diff > 0 { + let mut rows_changed = before_count - after_count; + // A self-referential FK (child == target): the before/after + // diff also covers the directly-deleted rows, which are + // already reported in `rows_affected` and are not cascade + // effects. Subtract them so the summary reports only the rows + // removed *via* the self-reference. + let self_referential = rel.other_table == target_table; + if self_referential { + rows_changed -= rows_affected as i64; + } + if rows_changed > 0 { cascade.push(CascadeEffect { relationship_name: rel.name.clone(), child_table: rel.other_table.clone(), - rows_changed: diff, + rows_changed, action: rel.on_delete, }); - rewritten_tables.push(rel.other_table.clone()); + // The target's CSV is already queued; only add a distinct + // child table (a self-ref child is the target itself). + if !self_referential { + rewritten_tables.push(rel.other_table.clone()); + } } } @@ -10346,6 +10372,78 @@ mod tests { assert!(orders.rows.is_empty(), "child rows should be cascaded"); } + #[tokio::test] + async fn self_referential_cascade_counts_only_cascaded_rows() { + // A self-referential ON DELETE CASCADE FK (T.ParentId -> T.id): + // deleting the root of a chain cascades down within T. The + // directly-deleted root is reported in `rows_affected`, so the + // cascade summary must report only the *additional* rows + // removed by the self-reference — the raw before/after diff + // would double-count the direct delete. Parity fix shared with + // `do_sql_delete` (ADR-0033 Amendment 2 mechanism). + let db = db(); + db.create_table( + "T".to_string(), + vec![col("id", Type::Int), col("ParentId", Type::Int)], + vec!["id".to_string()], + None, + ) + .await + .unwrap(); + db.add_relationship( + Some("parent_of".to_string()), + "T".to_string(), + "id".to_string(), + "T".to_string(), + "ParentId".to_string(), + ReferentialAction::Cascade, + ReferentialAction::NoAction, + false, + None, + ) + .await + .unwrap(); + // Chain: 1 (root) <- 2 <- 3. + db.insert( + "T".to_string(), + Some(vec!["id".to_string(), "ParentId".to_string()]), + vec![Value::Number("1".to_string()), Value::Null], + None, + ) + .await + .unwrap(); + db.insert( + "T".to_string(), + Some(vec!["id".to_string(), "ParentId".to_string()]), + vec![Value::Number("2".to_string()), Value::Number("1".to_string())], + None, + ) + .await + .unwrap(); + db.insert( + "T".to_string(), + Some(vec!["id".to_string(), "ParentId".to_string()]), + vec![Value::Number("3".to_string()), Value::Number("2".to_string())], + None, + ) + .await + .unwrap(); + let result = db + .delete( + "T".to_string(), + RowFilter::eq("id", Value::Number("1".to_string())), + None, + ) + .await + .unwrap(); + assert_eq!(result.rows_affected, 1, "one row matched the filter directly"); + assert_eq!(result.cascade.len(), 1, "self-ref relationship reported once"); + assert_eq!( + result.cascade[0].rows_changed, 2, + "only the 2 cascaded rows, not the directly-deleted root too" + ); + } + #[tokio::test] async fn query_data_renders_bools_as_words() { let db = db(); diff --git a/tests/sql_delete.rs b/tests/sql_delete.rs index 66b68fa..db075ae 100644 --- a/tests/sql_delete.rs +++ b/tests/sql_delete.rs @@ -380,6 +380,40 @@ fn delete_violating_fk_fails_and_persists_nothing() { assert!(!history.contains(input), "failed delete not logged: {history:?}"); } +#[test] +fn self_referential_cascade_counts_only_cascaded_rows() { + // A self-referential ON DELETE CASCADE FK: deleting the root of a + // chain cascades down within the same table. The directly-deleted + // row is reported in rows_affected, so the cascade summary must + // report only the *additional* rows removed via the self- + // reference — not the raw before/after diff (which includes the + // direct delete). Without the self-ref correction this reports 3. + let (_project, db, _dir) = open_project_db(); + let rt = rt(); + create_cols(&db, &rt, "T", &[("id", Type::Int), ("ParentId", Type::Int)], &["id"]); + rt.block_on(db.add_relationship( + Some("parent_of".to_string()), + "T".to_string(), + "id".to_string(), + "T".to_string(), + "ParentId".to_string(), + ReferentialAction::Cascade, + ReferentialAction::NoAction, + false, + None, + )) + .expect("add self-referential relationship"); + seed(&db, &rt, "insert into T (id, ParentId) values (1, null), (2, 1), (3, 2)", "T"); + let result = + run_delete(&db, &rt, "sql_delete from T where id = 1").expect("self-ref delete runs"); + assert_eq!(result.rows_affected, 1, "one row matched the WHERE directly"); + assert_eq!(result.cascade.len(), 1, "self-ref relationship reported once"); + assert_eq!( + result.cascade[0].rows_changed, 2, + "only the 2 cascaded rows, not the directly-deleted root too" + ); +} + #[test] fn internal_target_table_rejected_at_parse() { // ADR-0030 §6 / ADR-0033 §1: the `__rdbms_*` metadata tables are