Add FK constraint on blockers.blocker_id → tasks(id)

Amolith created

Change summary

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(-)

Detailed changes

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}");
             }

src/db.rs 🔗

@@ -199,6 +199,14 @@ pub fn would_cycle(conn: &Connection, from: &str, to: &str) -> Result<bool> {
     Ok(false)
 }
 
+/// Check whether a task with the given ID exists.
+pub fn task_exists(conn: &Connection, id: &str) -> Result<bool> {
+    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<TaskDetail> {
     let task = conn.query_row(

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.

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;

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;

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"));
+}

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");
+}