From a0dc07f0856a438aafe4d56e25eac8056a360eeb Mon Sep 17 00:00:00 2001 From: Miguel Palhas Date: Tue, 25 Aug 2026 11:42:59 +0100 Subject: [PATCH] feat(db): let a waiver name the rule it relaxed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `releases` forbade a rule name on anything but a rejection, so §9.3's deck showed a bare `waived` beside rejections that each named their own, and §5.7's "watchable but not what was asked" lost the half that says what was not asked for. Since #210 that is the ordinary outcome of waiving a size rejection, not a rare one. 0032 rebuilds the table with `CHECK (verdict != 'rejected' OR rejected_rule IS NOT NULL)`, and the daemon and arr-api's reclassification both store the waived rule. Existing rows keep NULL and read as they do today. `releases` is a parent — `grabs`, `movie_releases`, `episode_releases` and `season_releases` point at it, three ON DELETE CASCADE — so the rebuild runs `-- no-transaction` with foreign keys off around one explicit transaction, per SQLite's own procedure. Verified against a real database: the pre-0032 binary created and populated it, this build migrated a copy, and every release row, child row and created_at came through byte-identical with `PRAGMA foreign_key_check` clean. Refs #211 --- crates/arr-api/src/movies.rs | 10 + crates/arr-api/src/reclassify.rs | 8 +- crates/arr-api/src/series.rs | 7 +- crates/arr-daemon/src/grab.rs | 25 ++- crates/arr-db/migrations/0032_waived_rule.sql | 68 +++++++ crates/arr-db/src/lib.rs | 171 ++++++++++++++++++ 6 files changed, 281 insertions(+), 8 deletions(-) create mode 100644 crates/arr-db/migrations/0032_waived_rule.sql diff --git a/crates/arr-api/src/movies.rs b/crates/arr-api/src/movies.rs index fabc819..53a5923 100644 --- a/crates/arr-api/src/movies.rs +++ b/crates/arr-api/src/movies.rs @@ -76,6 +76,12 @@ pub struct Release { pub parsed: serde_json::Value, pub score: Option, pub verdict: Option, + // 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, } @@ -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 diff --git a/crates/arr-api/src/reclassify.rs b/crates/arr-api/src/reclassify.rs index e95d265..b01beb7 100644 --- a/crates/arr-api/src/reclassify.rs +++ b/crates/arr-api/src/reclassify.rs @@ -208,11 +208,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; } diff --git a/crates/arr-api/src/series.rs b/crates/arr-api/src/series.rs index de95833..12904f8 100644 --- a/crates/arr-api/src/series.rs +++ b/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() diff --git a/crates/arr-daemon/src/grab.rs b/crates/arr-daemon/src/grab.rs index b38d37d..4a404ca 100644 --- a/crates/arr-daemon/src/grab.rs +++ b/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) { 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] diff --git a/crates/arr-db/migrations/0032_waived_rule.sql b/crates/arr-db/migrations/0032_waived_rule.sql new file mode 100644 index 0000000..3a18906 --- /dev/null +++ b/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; diff --git a/crates/arr-db/src/lib.rs b/crates/arr-db/src/lib.rs index 0d4b5ae..d28eb37 100644 --- a/crates/arr-db/src/lib.rs +++ b/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 = + 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 = + 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;