Merge #211: let a waiver name the rule it relaxed

Closes #211

crates/arr-api/src/movies.rs

@@ -76,6 +76,12 @@ pub struct Release {
     pub parsed: serde_json::Value,
     pub score: Option<f64>,
     pub verdict: Option<String>,
+    // The rule behind the verdict: the one that killed a `rejected` row, or
+    // the one a `waived` row relaxed (#211). Null on an `eligible` row, and
+    // on a `waived` row stored before migration 0032, which could not record
+    // it. A plain comment, not a doc comment: doc comments here become
+    // OpenAPI descriptions and would put the generated client in web/ out of
+    // date, which #227 and #232 own.
     pub rejected_rule: Option<String>,
 }
 
@@ -1303,6 +1309,10 @@ mod tests {
                 .expect("releases json");
         assert_eq!(releases.len(), 1);
         assert_eq!(releases[0]["verdict"], "waived");
+        // #211: and it says which rule the waiver relaxed, rather than
+        // sitting in the deck as a bare `waived` beside rejections that each
+        // name their own.
+        assert_eq!(releases[0]["rejected_rule"], "size");
     }
 
     /// #241: moving a title to a root with a different policy re-derives its

crates/arr-api/src/reclassify.rs

@@ -234,11 +234,11 @@ async fn apply(
             episodes,
             runtime_minutes,
         );
-        // `releases` allows a rule name only on a rejected row
-        // (`CHECK ((verdict = 'rejected') = (rejected_rule IS NOT NULL))`),
-        // which is also how the daemon writes a waiver.
+        // A waiver records the rule it relaxed, the same as a rejection
+        // (#211). Migration 0032 relaxed `releases` to
+        // `CHECK (verdict != 'rejected' OR rejected_rule IS NOT NULL)` so it
+        // can, and the daemon writes waivers the same way.
         let (verdict, rule) = verdict_columns(&evaluation.verdict);
-        let rule = if verdict == "rejected" { rule } else { None };
         if release.verdict.as_deref() == Some(verdict) && release.rejected_rule == rule {
             continue;
         }

crates/arr-api/src/series.rs

@@ -2014,9 +2014,10 @@ mod tests {
             releases[0]["verdict"], "waived",
             "a waived grab stays a waiver; nothing here makes it eligible"
         );
-        // The row's rule name goes with the rejection; what survives is the
-        // dashed `waived` verdict the deck reads (§9.3).
-        assert!(releases[0]["rejected_rule"].is_null());
+        // #211: the waiver keeps the rule it relaxed, so the deck names it
+        // the way it names a rejection (§9.3) instead of showing a bare
+        // `waived`.
+        assert_eq!(releases[0]["rejected_rule"], "size");
 
         // The grab the deck's one click sends is now accepted.
         let response = reqwest::Client::new()

crates/arr-daemon/src/grab.rs

@@ -1534,10 +1534,17 @@ fn search_query(movie: &PendingMovie) -> String {
     )
 }
 
+/// The `verdict` and `rejected_rule` columns for a verdict.
+///
+/// A waiver names the rule it relaxed (#211). §5.7 calls a soft fail
+/// "watchable but not what was asked", and which rule was relaxed is the
+/// whole content of that sentence, so §9.3's deck can name it the way it
+/// names a rejection. Rows written before 0032 hold `NULL` there and stay
+/// readable.
 fn verdict_columns(verdict: &Verdict) -> (&'static str, Option<String>) {
     match verdict {
         Verdict::Eligible => ("eligible", None),
-        Verdict::Waived(_) => ("waived", None),
+        Verdict::Waived(rule) => ("waived", Some(rule.name())),
         Verdict::Rejected(rule) => ("rejected", Some(rule.name())),
     }
 }
@@ -2029,6 +2036,22 @@ mod tests {
         );
     }
 
+    /// #211: a waiver names the rule it relaxed, the same as a rejection, so
+    /// §9.3's deck shows what was given up instead of a bare `waived`.
+    /// Migration 0032 relaxed the constraint that forbade it.
+    #[test]
+    fn a_waiver_records_the_rule_it_relaxed() {
+        assert_eq!(
+            verdict_columns(&Verdict::Waived(arr_core::Rule::Size)),
+            ("waived", Some("size".to_owned()))
+        );
+        assert_eq!(verdict_columns(&Verdict::Eligible), ("eligible", None));
+        assert_eq!(
+            verdict_columns(&Verdict::Rejected(arr_core::Rule::RequiredAudio)),
+            ("rejected", Some("required_audio".to_owned()))
+        );
+    }
+
     /// Every candidate is cached with its verdict, which is what the manual
     /// search view and the attention queues read (§9.3).
     #[tokio::test]

crates/arr-db/migrations/0032_waived_rule.sql

@@ -0,0 +1,68 @@
+-- no-transaction
+-- #211: a waived release could not record which rule it relaxed. The old
+-- constraint made the rule name an exact synonym for rejection:
+--
+--     CHECK ((verdict = 'rejected') = (rejected_rule IS NOT NULL))
+--
+-- so §9.3's deck showed a bare `waived` beside rejected rows that each named
+-- their own rule, and §5.7's "watchable but not what was asked" lost the half
+-- that says what was not asked for. The relaxed form still demands a rule on
+-- a rejection and stops demanding its absence elsewhere.
+--
+-- SQLite cannot alter a CHECK, so the table is rebuilt (see 0014). Unlike the
+-- rebuilds there, `releases` is a parent: `grabs`, `movie_releases`,
+-- `episode_releases` and `season_releases` all point at it, three of them
+-- ON DELETE CASCADE. Dropping the old table with foreign keys enforced would
+-- delete those children (or, for `grabs`, refuse outright), so this follows
+-- SQLite's own procedure — foreign keys off, the rebuild in one transaction,
+-- foreign keys back on. `PRAGMA foreign_keys` is a no-op inside a
+-- transaction, which is why the file opens `-- no-transaction` and manages
+-- its own; the migration is still all-or-nothing.
+--
+-- Rows are copied verbatim. Every existing `waived` row keeps its NULL and
+-- goes on reading as it does today; only rows written after this migration
+-- carry a waived rule.
+PRAGMA foreign_keys = OFF;
+
+BEGIN;
+
+CREATE TABLE releases_new (
+    id            INTEGER PRIMARY KEY,
+    -- Prowlarr's indexer id. Not a foreign key: indexers live in Prowlarr.
+    indexer_id    INTEGER NOT NULL,
+    guid          TEXT NOT NULL,
+    name          TEXT NOT NULL,
+    size          INTEGER NOT NULL,
+    seeders       INTEGER,
+    publish_date  TEXT,
+    download_url  TEXT NOT NULL,
+    -- §5.6. What the release name claims, before anything is downloaded.
+    parsed        TEXT NOT NULL CHECK (json_valid(parsed)),
+    score         REAL,
+    -- §9.3. Three buckets. `rejected_rule` names the rule that killed it so
+    -- an over-strict filter is visible without reading release names, and
+    -- §5.7's waiver names the rule it relaxed for the same reason.
+    verdict       TEXT CHECK (verdict IN ('eligible', 'waived', 'rejected')),
+    rejected_rule TEXT,
+    created_at    TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')),
+    UNIQUE (indexer_id, guid),
+    CHECK (verdict != 'rejected' OR rejected_rule IS NOT NULL)
+) STRICT;
+
+INSERT INTO releases_new (
+    id, indexer_id, guid, name, size, seeders, publish_date, download_url,
+    parsed, score, verdict, rejected_rule, created_at
+)
+SELECT
+    id, indexer_id, guid, name, size, seeders, publish_date, download_url,
+    parsed, score, verdict, rejected_rule, created_at
+FROM releases;
+
+DROP TABLE releases;
+ALTER TABLE releases_new RENAME TO releases;
+
+CREATE INDEX releases_verdict ON releases (verdict, score);
+
+COMMIT;
+
+PRAGMA foreign_keys = ON;

crates/arr-db/src/lib.rs

@@ -517,6 +517,177 @@ mod tests {
         );
     }
 
+    /// #211: 0032 rebuilds `releases`, which is a parent — `grabs`,
+    /// `movie_releases`, `episode_releases` and `season_releases` all point
+    /// at it, three of them ON DELETE CASCADE. The rebuild runs with foreign
+    /// keys off so dropping the old table neither cascades those children
+    /// away nor is refused by `grabs`.
+    #[tokio::test]
+    async fn the_releases_rebuild_preserves_children_and_rows() {
+        let (_dir, db) = a_deck_with_every_child_row().await;
+
+        db.migrate().await.expect("remaining migrations");
+
+        for (label, query, expected) in [
+            ("releases", "SELECT count(*) FROM releases", 3),
+            ("movie_releases", "SELECT count(*) FROM movie_releases", 1),
+            (
+                "episode_releases",
+                "SELECT count(*) FROM episode_releases",
+                1,
+            ),
+            ("season_releases", "SELECT count(*) FROM season_releases", 1),
+            ("grabs", "SELECT count(*) FROM grabs", 1),
+        ] {
+            let rows: i64 = sqlx::query_scalar(query)
+                .fetch_one(db.pool())
+                .await
+                .expect("count");
+            assert_eq!(rows, expected, "{label} survives the releases rebuild");
+        }
+
+        // An existing waived row keeps its NULL and reads as it did before.
+        let waived: Option<String> =
+            sqlx::query_scalar("SELECT rejected_rule FROM releases WHERE guid = 'waived-guid'")
+                .fetch_one(db.pool())
+                .await
+                .expect("waived row");
+        assert_eq!(waived, None);
+        let rejected: Option<String> =
+            sqlx::query_scalar("SELECT rejected_rule FROM releases WHERE guid = 'rejected-guid'")
+                .fetch_one(db.pool())
+                .await
+                .expect("rejected row");
+        assert_eq!(rejected.as_deref(), Some("size"));
+
+        // Foreign keys are back on for the connection the migration used.
+        let foreign_keys: i64 = sqlx::query_scalar("PRAGMA foreign_keys")
+            .fetch_one(db.pool())
+            .await
+            .expect("foreign_keys");
+        assert_eq!(foreign_keys, 1);
+    }
+
+    /// A database migrated to just before 0032, holding one release per
+    /// verdict and one row in every table that points at `releases`.
+    async fn a_deck_with_every_child_row() -> (tempfile::TempDir, Db) {
+        let dir = tempfile::tempdir().expect("tempdir");
+        let db = Db::connect(dir.path().join("arr.db"))
+            .await
+            .expect("connect");
+
+        MIGRATOR
+            .run_to(31, db.pool())
+            .await
+            .expect("migrations before the releases rebuild");
+
+        let movie_id = sqlx::query(
+            "INSERT INTO movies (tmdb_id, title, root_id)
+             SELECT 693134, 'Dune Part Two', id FROM roots WHERE kind = 'movie' LIMIT 1",
+        )
+        .execute(db.pool())
+        .await
+        .expect("movie")
+        .last_insert_rowid();
+        let series_id = sqlx::query(
+            "INSERT INTO series (tmdb_id, title, root_id)
+             SELECT 82728, 'Bluey', id FROM roots WHERE kind = 'tv' LIMIT 1",
+        )
+        .execute(db.pool())
+        .await
+        .expect("series")
+        .last_insert_rowid();
+        let season_id = sqlx::query("INSERT INTO seasons (series_id, number) VALUES (?, 1)")
+            .bind(series_id)
+            .execute(db.pool())
+            .await
+            .expect("season")
+            .last_insert_rowid();
+        let episode_id =
+            sqlx::query("INSERT INTO episodes (season_id, number, title) VALUES (?, 1, 'x')")
+                .bind(season_id)
+                .execute(db.pool())
+                .await
+                .expect("episode")
+                .last_insert_rowid();
+        // One row per verdict, including the waived row this issue is about,
+        // which under the old constraint could only hold NULL.
+        for (guid, verdict, rule) in [
+            ("eligible-guid", "eligible", None),
+            ("waived-guid", "waived", None),
+            ("rejected-guid", "rejected", Some("size")),
+        ] {
+            sqlx::query(
+                "INSERT INTO releases (indexer_id, guid, name, size, download_url, parsed, verdict, rejected_rule)
+                 VALUES (1, ?, ?, 1024, 'http://x', '{}', ?, ?)",
+            )
+            .bind(guid)
+            .bind(guid)
+            .bind(verdict)
+            .bind(rule)
+            .execute(db.pool())
+            .await
+            .expect("release");
+        }
+        let release_id: i64 =
+            sqlx::query_scalar("SELECT id FROM releases WHERE guid = 'eligible-guid'")
+                .fetch_one(db.pool())
+                .await
+                .expect("release id");
+        sqlx::query("INSERT INTO movie_releases (movie_id, release_id) VALUES (?, ?)")
+            .bind(movie_id)
+            .bind(release_id)
+            .execute(db.pool())
+            .await
+            .expect("movie link");
+        sqlx::query("INSERT INTO episode_releases (episode_id, release_id) VALUES (?, ?)")
+            .bind(episode_id)
+            .bind(release_id)
+            .execute(db.pool())
+            .await
+            .expect("episode link");
+        sqlx::query("INSERT INTO season_releases (season_id, release_id) VALUES (?, ?)")
+            .bind(season_id)
+            .bind(release_id)
+            .execute(db.pool())
+            .await
+            .expect("season link");
+        sqlx::query(
+            "INSERT INTO grabs (release_id, target_kind, target_id, infohash)
+             VALUES (?, 'movie', ?, 'infohash-1')",
+        )
+        .bind(release_id)
+        .bind(movie_id)
+        .execute(db.pool())
+        .await
+        .expect("grab");
+
+        (dir, db)
+    }
+
+    /// The relaxed constraint (#211) admits a rule on a waiver and still
+    /// refuses a rejection without one.
+    #[tokio::test]
+    async fn a_waiver_may_name_the_rule_it_relaxed() {
+        let (_dir, db) = fresh().await;
+
+        sqlx::query(
+            "INSERT INTO releases (indexer_id, guid, name, size, download_url, parsed, verdict, rejected_rule)
+             VALUES (1, 'waived', 'Some.Release', 1024, 'http://x', '{}', 'waived', 'size')",
+        )
+        .execute(db.pool())
+        .await
+        .expect("a waiver names its rule");
+
+        sqlx::query(
+            "INSERT INTO releases (indexer_id, guid, name, size, download_url, parsed, verdict)
+             VALUES (1, 'rejected', 'Some.Release', 1024, 'http://x', '{}', 'rejected')",
+        )
+        .execute(db.pool())
+        .await
+        .expect_err("a rejection still has to name its rule");
+    }
+
     #[tokio::test]
     async fn seeds_two_tv_roots_with_distinct_policies() {
         let (_dir, db) = fresh().await;