From e6f63ac0760b9fe3a4c4f1c9bba588de0ccf8fd4 Mon Sep 17 00:00:00 2001 From: Amolith Date: Wed, 25 Feb 2026 18:55:14 -0700 Subject: [PATCH] =?UTF-8?q?Add=20FK=20constraint=20on=20blockers.blocker?= =?UTF-8?q?=5Fid=20=E2=86=92=20tasks(id)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/cmd/dep.rs | 6 ++ src/db.rs | 8 ++ src/migrate.rs | 6 ++ src/migrations/0003_blocker_fk.down.sql | 15 ++++ src/migrations/0003_blocker_fk.up.sql | 20 +++++ tests/cli_dep.rs | 24 ++++++ tests/cli_migrate.rs | 101 +++++++++++++++++++++++- 7 files changed, 177 insertions(+), 3 deletions(-) create mode 100644 src/migrations/0003_blocker_fk.down.sql create mode 100644 src/migrations/0003_blocker_fk.up.sql diff --git a/src/cmd/dep.rs b/src/cmd/dep.rs index f1dd2a22ed868da1ffffb6f3a65a44b5158f1ca1..eb1d8d817787661da6c59ecfeee1f49be965edde 100644 --- a/src/cmd/dep.rs +++ b/src/cmd/dep.rs @@ -9,6 +9,12 @@ pub fn run(root: &Path, action: &DepAction, json: bool) -> Result<()> { match action { DepAction::Add { child, parent } => { + if !db::task_exists(&conn, child)? { + bail!("task '{child}' not found"); + } + if !db::task_exists(&conn, parent)? { + bail!("task '{parent}' not found"); + } if db::would_cycle(&conn, parent, child)? { bail!("adding dependency would create a cycle: {child} → {parent} → … → {child}"); } diff --git a/src/db.rs b/src/db.rs index fa019d8cc8d59988ec7a3fbff3529a0480184091..e0608a9682312357489db98ed850be561ef44642 100644 --- a/src/db.rs +++ b/src/db.rs @@ -199,6 +199,14 @@ pub fn would_cycle(conn: &Connection, from: &str, to: &str) -> Result { Ok(false) } +/// Check whether a task with the given ID exists. +pub fn task_exists(conn: &Connection, id: &str) -> Result { + let count: i32 = conn.query_row("SELECT COUNT(*) FROM tasks WHERE id = ?1", [id], |r| { + r.get(0) + })?; + Ok(count > 0) +} + /// Load a full task with labels and blockers. pub fn load_task_detail(conn: &Connection, id: &str) -> Result { let task = conn.query_row( diff --git a/src/migrate.rs b/src/migrate.rs index 069541f489a7543d1542b06eb1f844c0781de3b2..2113c8509716376349aa78aadc2bf1ad650dffda 100644 --- a/src/migrate.rs +++ b/src/migrate.rs @@ -33,6 +33,12 @@ static MIGRATIONS: &[Migration] = &[ post_hook_up: None, post_hook_down: None, }, + Migration { + up_sql: include_str!("migrations/0003_blocker_fk.up.sql"), + down_sql: include_str!("migrations/0003_blocker_fk.down.sql"), + post_hook_up: None, + post_hook_down: None, + }, ]; /// Read the current schema version from the database. diff --git a/src/migrations/0003_blocker_fk.down.sql b/src/migrations/0003_blocker_fk.down.sql new file mode 100644 index 0000000000000000000000000000000000000000..1348807d59c90d84c7dc7d8215f3da17c6b3c83d --- /dev/null +++ b/src/migrations/0003_blocker_fk.down.sql @@ -0,0 +1,15 @@ +-- Revert to the original blockers table without the blocker_id FK. + +CREATE TABLE blockers_old ( + task_id TEXT, + blocker_id TEXT, + PRIMARY KEY (task_id, blocker_id), + FOREIGN KEY (task_id) REFERENCES tasks(id) +); + +INSERT INTO blockers_old (task_id, blocker_id) + SELECT task_id, blocker_id FROM blockers; + +DROP TABLE blockers; + +ALTER TABLE blockers_old RENAME TO blockers; diff --git a/src/migrations/0003_blocker_fk.up.sql b/src/migrations/0003_blocker_fk.up.sql new file mode 100644 index 0000000000000000000000000000000000000000..085ee886009235de6b347f4466e9a7f237cc437f --- /dev/null +++ b/src/migrations/0003_blocker_fk.up.sql @@ -0,0 +1,20 @@ +-- Add FOREIGN KEY (blocker_id) REFERENCES tasks(id) to the blockers table. +-- SQLite has no ALTER TABLE ADD CONSTRAINT, so we rebuild. + +-- Drop dangling blocker_id rows that reference nonexistent tasks. +DELETE FROM blockers WHERE blocker_id NOT IN (SELECT id FROM tasks); + +CREATE TABLE blockers_new ( + task_id TEXT, + blocker_id TEXT, + PRIMARY KEY (task_id, blocker_id), + FOREIGN KEY (task_id) REFERENCES tasks(id), + FOREIGN KEY (blocker_id) REFERENCES tasks(id) +); + +INSERT INTO blockers_new (task_id, blocker_id) + SELECT task_id, blocker_id FROM blockers; + +DROP TABLE blockers; + +ALTER TABLE blockers_new RENAME TO blockers; diff --git a/tests/cli_dep.rs b/tests/cli_dep.rs index 20f115f37d7b5a106dceb43fbad343633567d6c0..3d1bcab3b5a9b2565c2c104bb8f2293bc122d53e 100644 --- a/tests/cli_dep.rs +++ b/tests/cli_dep.rs @@ -179,3 +179,27 @@ fn dep_add_allows_diamond_without_cycle() { let blockers = t["blockers"].as_array().unwrap(); assert_eq!(blockers.len(), 2); } + +#[test] +fn dep_add_rejects_nonexistent_child() { + let tmp = init_tmp(); + let real = create_task(&tmp, "Real task"); + + td().args(["dep", "add", "td-ghost", &real]) + .current_dir(&tmp) + .assert() + .failure() + .stderr(predicate::str::contains("task 'td-ghost' not found")); +} + +#[test] +fn dep_add_rejects_nonexistent_parent() { + let tmp = init_tmp(); + let real = create_task(&tmp, "Real task"); + + td().args(["dep", "add", &real, "td-phantom"]) + .current_dir(&tmp) + .assert() + .failure() + .stderr(predicate::str::contains("task 'td-phantom' not found")); +} diff --git a/tests/cli_migrate.rs b/tests/cli_migrate.rs index 9acd19ddef0af764fc30a41c4f7614dbccf0654b..993b760decfd8bbd9d335f4c7d740e89a2fe98df 100644 --- a/tests/cli_migrate.rs +++ b/tests/cli_migrate.rs @@ -20,8 +20,8 @@ fn fresh_init_sets_latest_version() { let version: u32 = conn .pragma_query_value(None, "user_version", |row| row.get(0)) .unwrap(); - // Version should be 2 (migration 0001 + 0002). - assert_eq!(version, 2); + // Version should be 3 (migration 0001 + 0002 + 0003). + assert_eq!(version, 3); } #[test] @@ -81,7 +81,7 @@ fn legacy_db_is_migrated_on_open() { let version: u32 = conn .pragma_query_value(None, "user_version", |row| row.get(0)) .unwrap(); - assert_eq!(version, 2); + assert_eq!(version, 3); } #[test] @@ -103,3 +103,98 @@ fn effort_column_exists_after_init() { .unwrap(); assert_eq!(effort, 3); } + +#[test] +fn blocker_fk_rejects_nonexistent_blocker_id() { + let tmp = init_tmp(); + let conn = rusqlite::Connection::open(tmp.path().join(".td/tasks.db")).unwrap(); + conn.execute_batch("PRAGMA foreign_keys = ON").unwrap(); + + conn.execute( + "INSERT INTO tasks (id, title, created, updated) \ + VALUES ('td-real', 'Real task', '2024-01-01T00:00:00Z', '2024-01-01T00:00:00Z')", + [], + ) + .unwrap(); + + // Inserting a blocker that references a nonexistent task should fail. + let result = conn.execute( + "INSERT INTO blockers (task_id, blocker_id) VALUES ('td-real', 'td-ghost')", + [], + ); + assert!( + result.is_err(), + "expected FK violation for nonexistent blocker_id" + ); +} + +#[test] +fn migration_cleans_dangling_blocker_ids() { + let tmp = TempDir::new().unwrap(); + let td_dir = tmp.path().join(".td"); + std::fs::create_dir_all(&td_dir).unwrap(); + + // Create a v2 database (pre-0003) with a dangling blocker_id. + let conn = rusqlite::Connection::open(td_dir.join("tasks.db")).unwrap(); + conn.execute_batch( + "CREATE TABLE tasks ( + id TEXT PRIMARY KEY, + title TEXT NOT NULL, + description TEXT DEFAULT '', + type TEXT DEFAULT 'task', + priority INTEGER DEFAULT 2, + status TEXT DEFAULT 'open', + parent TEXT DEFAULT '', + created TEXT NOT NULL, + updated TEXT NOT NULL, + effort INTEGER NOT NULL DEFAULT 2 + ); + CREATE TABLE labels ( + task_id TEXT, label TEXT, + PRIMARY KEY (task_id, label), + FOREIGN KEY (task_id) REFERENCES tasks(id) + ); + CREATE TABLE blockers ( + task_id TEXT, blocker_id TEXT, + PRIMARY KEY (task_id, blocker_id), + FOREIGN KEY (task_id) REFERENCES tasks(id) + ); + INSERT INTO tasks (id, title, created, updated) + VALUES ('td-a', 'Task A', '2024-01-01T00:00:00Z', '2024-01-01T00:00:00Z'); + INSERT INTO tasks (id, title, created, updated) + VALUES ('td-b', 'Task B', '2024-01-01T00:00:00Z', '2024-01-01T00:00:00Z'); + -- Valid blocker + INSERT INTO blockers (task_id, blocker_id) VALUES ('td-a', 'td-b'); + -- Dangling blocker referencing a task that doesn't exist + INSERT INTO blockers (task_id, blocker_id) VALUES ('td-a', 'td-gone'); + PRAGMA user_version = 2;", + ) + .unwrap(); + drop(conn); + + // Running any command triggers migration. + td().args(["--json", "list"]) + .current_dir(&tmp) + .assert() + .success(); + + // The valid blocker should survive; the dangling one should be gone. + let conn = rusqlite::Connection::open(td_dir.join("tasks.db")).unwrap(); + let count: i32 = conn + .query_row( + "SELECT COUNT(*) FROM blockers WHERE task_id = 'td-a'", + [], + |r| r.get(0), + ) + .unwrap(); + assert_eq!(count, 1, "only the valid blocker should remain"); + + let blocker: String = conn + .query_row( + "SELECT blocker_id FROM blockers WHERE task_id = 'td-a'", + [], + |r| r.get(0), + ) + .unwrap(); + assert_eq!(blocker, "td-b"); +}