db: fix self-referential cascade over-count + SQL-delete render test
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.
This commit is contained in:
+42
@@ -3413,4 +3413,46 @@ mod tests {
|
|||||||
"header row rendered: {texts:?}",
|
"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<String> = 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:?}",
|
||||||
|
);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -5790,17 +5790,30 @@ fn do_delete(
|
|||||||
let mut rewritten_tables: Vec<String> = vec![table.to_string()];
|
let mut rewritten_tables: Vec<String> = vec![table.to_string()];
|
||||||
for (rel, (_child_table, before_count)) in inbound.iter().zip(before_counts.iter()) {
|
for (rel, (_child_table, before_count)) in inbound.iter().zip(before_counts.iter()) {
|
||||||
let after_count = count_rows(conn, &rel.other_table)?;
|
let after_count = count_rows(conn, &rel.other_table)?;
|
||||||
let diff = before_count - after_count;
|
let mut rows_changed = before_count - after_count;
|
||||||
if diff > 0 {
|
// 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 {
|
cascade.push(CascadeEffect {
|
||||||
relationship_name: rel.name.clone(),
|
relationship_name: rel.name.clone(),
|
||||||
child_table: rel.other_table.clone(),
|
child_table: rel.other_table.clone(),
|
||||||
rows_changed: diff,
|
rows_changed,
|
||||||
action: rel.on_delete,
|
action: rel.on_delete,
|
||||||
});
|
});
|
||||||
|
// 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());
|
rewritten_tables.push(rel.other_table.clone());
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
}
|
||||||
|
|
||||||
let changes = Changes {
|
let changes = Changes {
|
||||||
schema_dirty: false,
|
schema_dirty: false,
|
||||||
@@ -6179,17 +6192,30 @@ fn do_sql_delete(
|
|||||||
let mut rewritten_tables: Vec<String> = vec![target_table.to_string()];
|
let mut rewritten_tables: Vec<String> = vec![target_table.to_string()];
|
||||||
for (rel, before_count) in inbound.iter().zip(before_counts.iter()) {
|
for (rel, before_count) in inbound.iter().zip(before_counts.iter()) {
|
||||||
let after_count = count_rows(conn, &rel.other_table)?;
|
let after_count = count_rows(conn, &rel.other_table)?;
|
||||||
let diff = before_count - after_count;
|
let mut rows_changed = before_count - after_count;
|
||||||
if diff > 0 {
|
// 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 {
|
cascade.push(CascadeEffect {
|
||||||
relationship_name: rel.name.clone(),
|
relationship_name: rel.name.clone(),
|
||||||
child_table: rel.other_table.clone(),
|
child_table: rel.other_table.clone(),
|
||||||
rows_changed: diff,
|
rows_changed,
|
||||||
action: rel.on_delete,
|
action: rel.on_delete,
|
||||||
});
|
});
|
||||||
|
// 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());
|
rewritten_tables.push(rel.other_table.clone());
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
}
|
||||||
|
|
||||||
let changes = Changes {
|
let changes = Changes {
|
||||||
schema_dirty: false,
|
schema_dirty: false,
|
||||||
@@ -10346,6 +10372,78 @@ mod tests {
|
|||||||
assert!(orders.rows.is_empty(), "child rows should be cascaded");
|
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]
|
#[tokio::test]
|
||||||
async fn query_data_renders_bools_as_words() {
|
async fn query_data_renders_bools_as_words() {
|
||||||
let db = db();
|
let db = db();
|
||||||
|
|||||||
@@ -380,6 +380,40 @@ fn delete_violating_fk_fails_and_persists_nothing() {
|
|||||||
assert!(!history.contains(input), "failed delete not logged: {history:?}");
|
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]
|
#[test]
|
||||||
fn internal_target_table_rejected_at_parse() {
|
fn internal_target_table_rejected_at_parse() {
|
||||||
// ADR-0030 §6 / ADR-0033 §1: the `__rdbms_*` metadata tables are
|
// ADR-0030 §6 / ADR-0033 §1: the `__rdbms_*` metadata tables are
|
||||||
|
|||||||
Reference in New Issue
Block a user