From 191049890324a9bb412483e5bead63986f6f4050 Mon Sep 17 00:00:00 2001 From: Marius Gravdal Date: Sun, 23 Aug 2026 10:49:12 +0200 Subject: [PATCH 1/3] fix: correct episode pagination for sorts other than newest-first MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit return_podcast_episodes_capitalized capped each UNION branch (Episodes, YouTubeVideos) with `ORDER BY pubdate DESC LIMIT (limit+offset)` regardless of the requested sort, then re-sorted the truncated set in the outer query. That's only valid when sorting newest-first: for any other sort (oldest first, title, duration) on a podcast with more episodes than the current page window, the per-branch cap discarded rows the outer sort actually needed. In practice this made "oldest first" pagination loop over the same newest-N episodes forever instead of ever reaching older ones, and made completed/incomplete/in-progress filters silently drop matches that weren't among the newest N raw rows (even though the separate, unrestricted count query correctly reported them). Fixes, for both the Postgres and MySQL code paths: - Each per-branch subquery now orders by the actually-requested sort column/direction instead of a hardcoded recency order, restoring the top-K-merge correctness of the per-branch LIMIT for every sort. - The completed/incomplete/in-progress filter is now pushed into each branch's WHERE before its LIMIT, instead of only being applied after truncation. - Added an explicit tiebreaker (episodeid/videoid per branch, is_youtube + Episodeid at the outer level) so ties in the sort column can't produce a different row order across separate page requests as per_branch_limit grows, which could otherwise reintroduce the same duplicate/skip behavior via a different path. - Consolidated the sort-column and filter-clause mappings, which had drifted into 4 and 6 duplicated match blocks respectively across the two DB backends — the kind of divergence that caused the original bug. Adds in-binary regression tests (rust-api has no lib target, so these can't live under tests/) covering: oldest-first pagination visiting every episode without duplicates, a completed-filter match outside the newest page, and pagination stability when the sort column is fully tied. Verified against a real Postgres, including confirming each test fails against the pre-fix query and passes after. Wires a new rust-backend-tests CI job (its own ephemeral Postgres service) to run these, since the existing backend-tests job's Python suite is already broken (imports a FastAPI app module that no longer exists) and untested. Also gitignores rust-api/target/, which wasn't previously excluded. --- .github/workflows/ci.yaml | 34 +++ .gitignore | 3 + rust-api/src/database.rs | 505 +++++++++++++++++++++++++++++++++++--- 3 files changed, 508 insertions(+), 34 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 9e9d0ee68..07a1338ad 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -56,6 +56,40 @@ jobs: chmod +x ./run-tests.sh ./run-tests.sh postgresql + rust-backend-tests: + runs-on: ubuntu-latest + services: + postgres: + image: postgres:latest + env: + POSTGRES_USER: test_user + POSTGRES_PASSWORD: test_password + POSTGRES_DB: test_db + ports: + - 5432:5432 + options: >- + --health-cmd pg_isready + --health-interval 10s + --health-timeout 5s + --health-retries 5 + + steps: + - uses: actions/checkout@v4 + + - uses: hecrj/setup-rust-action@v2 + with: + rust-version: 1.95 + + - name: Run episode pagination/sort/filter regression tests + working-directory: ./rust-api + env: + DB_HOST: localhost + DB_PORT: 5432 + DB_USER: test_user + DB_PASSWORD: test_password + DB_NAME: test_db + run: cargo test episode_pagination -- --ignored --test-threads=1 + frontend-tests: runs-on: ubuntu-latest steps: diff --git a/.gitignore b/.gitignore index 50518385b..421fe51d5 100644 --- a/.gitignore +++ b/.gitignore @@ -87,6 +87,9 @@ clients/mac-app/pinepods.spec # Web Removals web/target/* web/.idea/* + +# Rust build artifacts +rust-api/target/* keystore.properties key.properties **/key.properties diff --git a/rust-api/src/database.rs b/rust-api/src/database.rs index 30a7de4c1..528b78757 100644 --- a/rust-api/src/database.rs +++ b/rust-api/src/database.rs @@ -12327,18 +12327,48 @@ impl DatabasePool { ) -> AppResult<(Vec, i64)> { // Validate sort column/direction to prevent SQL injection (only fixed string literals // are ever spliced into the query — the actual user input is bound as a parameter). - let pg_order_col = match sort_by { - "duration" => "\"Episodeduration\"", - "title" => "\"Episodetitle\"", - _ => "\"Episodepubdate\"", // "date" or unknown + // + // Each per-branch subquery below sorts by the same logical column as the outer query — + // the per-branch LIMIT only yields a correct top-K merge when every branch is pre-sorted + // the way the outer sort will read it back. Mapping sort_by -> columns once here, shared + // by both DB backends' per-branch and outer clauses, keeps a future new sort option from + // reintroducing the "per-branch always sorted by pubdate DESC regardless of the request" + // divergence that caused episode pagination to loop/duplicate for non-default sorts. + let (pg_order_col, pg_ep_order_col, pg_yt_order_col) = match sort_by { + "duration" => ("\"Episodeduration\"", "\"Episodes\".episodeduration", "\"YouTubeVideos\".duration"), + "title" => ("\"Episodetitle\"", "\"Episodes\".episodetitle", "\"YouTubeVideos\".videotitle"), + _ => ("\"Episodepubdate\"", "\"Episodes\".episodepubdate", "\"YouTubeVideos\".publishedat"), // "date" or unknown }; - let my_order_col = match sort_by { - "duration" => "Episodeduration", - "title" => "Episodetitle", - _ => "Episodepubdate", + let (my_order_col, my_ep_order_col, my_yt_order_col) = match sort_by { + "duration" => ("Episodeduration", "Episodes.EpisodeDuration", "YouTubeVideos.Duration"), + "title" => ("Episodetitle", "Episodes.EpisodeTitle", "YouTubeVideos.VideoTitle"), + _ => ("Episodepubdate", "Episodes.EpisodePubDate", "YouTubeVideos.PublishedAt"), }; let order_dir = if sort_order == "asc" { "ASC" } else { "DESC" }; let has_search = !search.is_empty(); + + // Per-branch filter: pushed into each branch's WHERE, before the per-branch LIMIT, so a + // match outside the "top N by sort" window isn't discarded before the filter ever sees + // it (see the pagination-loop comment above — same root cause, same fix shape). Every + // row that reaches `combined` already satisfies this, so no outer-level filter is needed. + let (pg_ep_filter_clause, pg_yt_filter_clause) = match filter { + "completed" => (" AND \"Episodes\".completed = TRUE", " AND \"YouTubeVideos\".completed = TRUE"), + "in_progress" => ( + " AND \"Episodes\".completed = FALSE AND \"UserEpisodeHistory\".listenduration IS NOT NULL AND \"UserEpisodeHistory\".listenduration > 0", + " AND \"YouTubeVideos\".completed = FALSE AND \"YouTubeVideos\".listenposition IS NOT NULL AND \"YouTubeVideos\".listenposition > 0", + ), + "incomplete" => (" AND \"Episodes\".completed = FALSE", " AND \"YouTubeVideos\".completed = FALSE"), + _ => ("", ""), + }; + let (my_ep_filter_clause, my_yt_filter_clause) = match filter { + "completed" => (" AND Episodes.Completed = 1", " AND YouTubeVideos.Completed = 1"), + "in_progress" => ( + " AND Episodes.Completed = 0 AND UserEpisodeHistory.ListenDuration IS NOT NULL AND UserEpisodeHistory.ListenDuration > 0", + " AND YouTubeVideos.Completed = 0 AND YouTubeVideos.ListenPosition IS NOT NULL AND YouTubeVideos.ListenPosition > 0", + ), + "incomplete" => (" AND Episodes.Completed = 0", " AND YouTubeVideos.Completed = 0"), + _ => ("", ""), + }; match self { DatabasePool::Postgres(pool) => { let lim: i64 = limit.unwrap_or(i64::MAX); @@ -12371,12 +12401,6 @@ impl DatabasePool { } } - let filter_clause = match filter { - "completed" => " WHERE \"Completed\" = TRUE", - "in_progress" => " WHERE \"Completed\" = FALSE AND \"Listenduration\" IS NOT NULL AND \"Listenduration\" > 0", - "incomplete" => " WHERE \"Completed\" = FALSE", - _ => "", - }; let ep_search_clause = if has_search { " AND (\"Episodes\".episodetitle ILIKE $3 OR \"Episodes\".episodedescription ILIKE $3)" } else { "" }; @@ -12386,6 +12410,16 @@ impl DatabasePool { let search_pattern = if has_search { format!("%{}%", search) } else { String::new() }; + let (ep_order_col, yt_order_col) = (pg_ep_order_col, pg_yt_order_col); + let (ep_filter_clause, yt_filter_clause) = (pg_ep_filter_clause, pg_yt_filter_clause); + // Used only by the separate count_sql below, which has no per-branch LIMIT to + // push the filter ahead of — it filters its own (unrestricted) combined result. + let filter_clause = match filter { + "completed" => " WHERE \"Completed\" = TRUE", + "in_progress" => " WHERE \"Completed\" = FALSE AND \"Listenduration\" IS NOT NULL AND \"Listenduration\" > 0", + "incomplete" => " WHERE \"Completed\" = FALSE", + _ => "", + }; // Bind positions shift when a search pattern is present (it takes $3). let (pb_lim_pos, outer_lim_pos, outer_off_pos) = if has_search { @@ -12432,8 +12466,8 @@ impl DatabasePool { LEFT JOIN "DownloadedEpisodes" ON "Episodes".episodeid = "DownloadedEpisodes".episodeid AND "DownloadedEpisodes".userid = $1 - WHERE "Podcasts".userid = $1 AND "Podcasts".podcastid = ANY($2){ep_search_clause} - ORDER BY "Episodes".episodepubdate DESC + WHERE "Podcasts".userid = $1 AND "Podcasts".podcastid = ANY($2){ep_search_clause}{ep_filter_clause} + ORDER BY {ep_order_col} {order_dir} NULLS LAST, "Episodes".episodeid ASC LIMIT {pb_lim_pos}) UNION ALL @@ -12471,18 +12505,21 @@ impl DatabasePool { LEFT JOIN "DownloadedVideos" ON "YouTubeVideos".videoid = "DownloadedVideos".videoid AND "DownloadedVideos".userid = $1 - WHERE "Podcasts".userid = $1 AND "Podcasts".podcastid = ANY($2){yt_search_clause} - ORDER BY "YouTubeVideos".publishedat DESC + WHERE "Podcasts".userid = $1 AND "Podcasts".podcastid = ANY($2){yt_search_clause}{yt_filter_clause} + ORDER BY {yt_order_col} {order_dir} NULLS LAST, "YouTubeVideos".videoid ASC LIMIT {pb_lim_pos}) - ) combined{filter_clause} - ORDER BY {pg_order_col} {order_dir} NULLS LAST + ) combined + ORDER BY {pg_order_col} {order_dir} NULLS LAST, is_youtube ASC, "Episodeid" ASC LIMIT {outer_lim_pos} OFFSET {outer_off_pos}"#, ep_search_clause = ep_search_clause, yt_search_clause = yt_search_clause, + ep_filter_clause = ep_filter_clause, + yt_filter_clause = yt_filter_clause, + ep_order_col = ep_order_col, + yt_order_col = yt_order_col, pb_lim_pos = pb_lim_pos, pg_order_col = pg_order_col, order_dir = order_dir, - filter_clause = filter_clause, outer_lim_pos = outer_lim_pos, outer_off_pos = outer_off_pos, ); @@ -12599,12 +12636,6 @@ impl DatabasePool { // string sized to the resolved list and bind each ID below. let id_placeholders = vec!["?"; podcast_ids.len()].join(","); - let mysql_filter = match filter { - "completed" => " WHERE Completed = 1", - "in_progress" => " WHERE Completed = 0 AND Listenduration IS NOT NULL AND Listenduration > 0", - "incomplete" => " WHERE Completed = 0", - _ => "", - }; let ep_search_clause = if has_search { " AND (Episodes.EpisodeTitle LIKE ? OR Episodes.EpisodeDescription LIKE ?)" } else { "" }; @@ -12614,6 +12645,16 @@ impl DatabasePool { let search_pattern = if has_search { format!("%{}%", search) } else { String::new() }; + let (ep_order_col, yt_order_col) = (my_ep_order_col, my_yt_order_col); + let (ep_mysql_filter, yt_mysql_filter) = (my_ep_filter_clause, my_yt_filter_clause); + // Used only by the separate count_sql below, which has no per-branch LIMIT to + // push the filter ahead of — it filters its own (unrestricted) combined result. + let mysql_filter = match filter { + "completed" => " WHERE Completed = 1", + "in_progress" => " WHERE Completed = 0 AND Listenduration IS NOT NULL AND Listenduration > 0", + "incomplete" => " WHERE Completed = 0", + _ => "", + }; // Main query: LIMIT pushed inside each branch so joins only fire on the rows // that can possibly contribute to the final page. The outer ORDER BY merges @@ -12657,8 +12698,8 @@ impl DatabasePool { LEFT JOIN DownloadedEpisodes ON Episodes.EpisodeID = DownloadedEpisodes.EpisodeID AND DownloadedEpisodes.UserID = ? - WHERE Podcasts.UserID = ? AND Podcasts.PodcastID IN ({ph}){ep_search_clause} - ORDER BY Episodes.EpisodePubDate DESC + WHERE Podcasts.UserID = ? AND Podcasts.PodcastID IN ({ph}){ep_search_clause}{ep_mysql_filter} + ORDER BY {ep_order_col} {order_dir}, Episodes.EpisodeID ASC LIMIT ?) UNION ALL @@ -12696,16 +12737,19 @@ impl DatabasePool { LEFT JOIN DownloadedVideos ON YouTubeVideos.VideoID = DownloadedVideos.VideoID AND DownloadedVideos.UserID = ? - WHERE Podcasts.UserID = ? AND Podcasts.PodcastID IN ({ph}){yt_search_clause} - ORDER BY YouTubeVideos.PublishedAt DESC + WHERE Podcasts.UserID = ? AND Podcasts.PodcastID IN ({ph}){yt_search_clause}{yt_mysql_filter} + ORDER BY {yt_order_col} {order_dir}, YouTubeVideos.VideoID ASC LIMIT ?) - ) combined{mysql_filter} - ORDER BY {my_order_col} {order_dir} + ) combined + ORDER BY {my_order_col} {order_dir}, is_youtube ASC, Episodeid ASC LIMIT ? OFFSET ?", ph = id_placeholders, ep_search_clause = ep_search_clause, yt_search_clause = yt_search_clause, - mysql_filter = mysql_filter, + ep_mysql_filter = ep_mysql_filter, + yt_mysql_filter = yt_mysql_filter, + ep_order_col = ep_order_col, + yt_order_col = yt_order_col, my_order_col = my_order_col, order_dir = order_dir, ); @@ -31482,3 +31526,396 @@ mod restore_filter_tests { assert!(!out.contains("LOCK TABLES `Sessions`")); } } + +/// Regression coverage for the per-branch pagination bug in +/// `return_podcast_episodes_capitalized`: each UNION branch's candidate-row LIMIT used to be +/// ordered by `pubdate DESC` unconditionally, regardless of the requested sort. That made any +/// non-default sort (e.g. "oldest first") re-serve the same newest-page-size episodes forever +/// instead of ever advancing to older ones, and made filters silently drop matches that weren't +/// among the newest N raw rows. `rust-api` has no library target, so these live as in-binary +/// `#[cfg(test)]` tests (same pattern as `restore_filter_tests` above) rather than under +/// `tests/`. They need a real Postgres, so they're `#[ignore]`d by default — run them with: +/// +/// ```sh +/// cargo test episode_pagination -- --ignored --test-threads=1 +/// ``` +/// +/// against a Postgres reachable via the `DB_HOST`/`DB_PORT`/`DB_USER`/`DB_PASSWORD`/`DB_NAME` +/// env vars (defaults below match the CI service in `.github/workflows/ci.yaml` and the +/// `docker run` command in `run-tests.sh`). `--test-threads=1` isn't load-bearing (each test +/// uses its own sentinel id range) but keeps things simple to reason about. +#[cfg(test)] +mod episode_pagination_tests { + use super::DatabasePool; + use sqlx::postgres::PgPoolOptions; + use sqlx::{Pool, Postgres}; + + async fn test_pool() -> Pool { + let host = std::env::var("DB_HOST").unwrap_or_else(|_| "localhost".into()); + let port = std::env::var("DB_PORT").unwrap_or_else(|_| "5432".into()); + let user = std::env::var("DB_USER").unwrap_or_else(|_| "test_user".into()); + let password = std::env::var("DB_PASSWORD").unwrap_or_else(|_| "test_password".into()); + let name = std::env::var("DB_NAME").unwrap_or_else(|_| "test_db".into()); + let url = format!("postgres://{user}:{password}@{host}:{port}/{name}"); + PgPoolOptions::new() + .max_connections(5) + .connect(&url) + .await + .unwrap_or_else(|e| { + panic!("connect to test Postgres at {host}:{port}/{name} failed: {e}") + }) + } + + /// Creates only the columns `return_podcast_episodes_capitalized` actually touches. + /// `IF NOT EXISTS` so this is a no-op against an already fully-migrated DB. + async fn ensure_schema(pool: &Pool) { + let statements = [ + r#"CREATE TABLE IF NOT EXISTS "Users" ( + userid SERIAL PRIMARY KEY, + usepodcastcovers BOOLEAN DEFAULT FALSE + )"#, + r#"CREATE TABLE IF NOT EXISTS "Podcasts" ( + podcastid SERIAL PRIMARY KEY, + userid INT, + podcastname TEXT, + artworkurl TEXT, + usepodcastcovers BOOLEAN DEFAULT FALSE, + usepodcastcoverscustomized BOOLEAN DEFAULT FALSE, + mergedpodcastids TEXT + )"#, + r#"CREATE TABLE IF NOT EXISTS "Episodes" ( + episodeid SERIAL PRIMARY KEY, + podcastid INT, + episodetitle TEXT, + episodepubdate TIMESTAMP, + episodedescription TEXT, + episodeartwork TEXT, + episodeurl TEXT, + episodeduration INT, + completed BOOLEAN DEFAULT FALSE, + is_video BOOLEAN DEFAULT FALSE + )"#, + r#"CREATE TABLE IF NOT EXISTS "UserEpisodeHistory" ( + userid INT, + episodeid INT, + listenduration INT + )"#, + r#"CREATE TABLE IF NOT EXISTS "SavedEpisodes" ( + userid INT, + episodeid INT + )"#, + r#"CREATE TABLE IF NOT EXISTS "EpisodeQueue" ( + userid INT, + episodeid INT, + is_youtube BOOLEAN DEFAULT FALSE + )"#, + r#"CREATE TABLE IF NOT EXISTS "DownloadedEpisodes" ( + userid INT, + episodeid INT + )"#, + r#"CREATE TABLE IF NOT EXISTS "YouTubeVideos" ( + videoid SERIAL PRIMARY KEY, + podcastid INT, + videotitle TEXT, + publishedat TIMESTAMP, + videodescription TEXT, + thumbnailurl TEXT, + videourl TEXT, + duration INT, + listenposition INT, + completed BOOLEAN DEFAULT FALSE + )"#, + r#"CREATE TABLE IF NOT EXISTS "SavedVideos" ( + userid INT, + videoid INT + )"#, + r#"CREATE TABLE IF NOT EXISTS "DownloadedVideos" ( + userid INT, + videoid INT + )"#, + ]; + for stmt in statements { + sqlx::query(stmt) + .execute(pool) + .await + .unwrap_or_else(|e| panic!("create test schema failed for `{stmt}`: {e}")); + } + } + + /// Deletes any rows for a sentinel user/podcast id, so a test is safely re-runnable and + /// distinct sentinel ranges across tests can't collide. + async fn cleanup(pool: &Pool, user_id: i32, podcast_id: i32) { + for (sql, id) in [ + (r#"DELETE FROM "UserEpisodeHistory" WHERE userid = $1"#, user_id), + (r#"DELETE FROM "SavedEpisodes" WHERE userid = $1"#, user_id), + (r#"DELETE FROM "EpisodeQueue" WHERE userid = $1"#, user_id), + (r#"DELETE FROM "DownloadedEpisodes" WHERE userid = $1"#, user_id), + (r#"DELETE FROM "Episodes" WHERE podcastid = $1"#, podcast_id), + (r#"DELETE FROM "Podcasts" WHERE podcastid = $1"#, podcast_id), + (r#"DELETE FROM "Users" WHERE userid = $1"#, user_id), + ] { + let _ = sqlx::query(sql).bind(id).execute(pool).await; + } + } + + async fn seed_podcast(pool: &Pool, user_id: i32, podcast_id: i32) { + sqlx::query(r#"INSERT INTO "Users" (userid) VALUES ($1)"#) + .bind(user_id) + .execute(pool) + .await + .unwrap_or_else(|e| panic!("insert test user failed: {e}")); + sqlx::query( + r#"INSERT INTO "Podcasts" (podcastid, userid, podcastname) VALUES ($1, $2, 'Test Show')"#, + ) + .bind(podcast_id) + .bind(user_id) + .execute(pool) + .await + .unwrap_or_else(|e| panic!("insert test podcast failed: {e}")); + } + + async fn insert_episode( + pool: &Pool, + episode_id: i32, + podcast_id: i32, + title: &str, + pubdate: &str, + duration: i32, + completed: bool, + ) { + sqlx::query( + r#"INSERT INTO "Episodes" + (episodeid, podcastid, episodetitle, episodepubdate, episodedescription, episodeartwork, episodeurl, episodeduration, completed) + VALUES ($1, $2, $3, $4::timestamp, '', 'https://example.test/art.png', 'https://example.test/ep.mp3', $5, $6)"#, + ) + .bind(episode_id) + .bind(podcast_id) + .bind(title) + .bind(pubdate) + .bind(duration) + .bind(completed) + .execute(pool) + .await + .unwrap_or_else(|e| panic!("insert test episode failed: {e}")); + } + + /// The exact bug reported: sorting "oldest first" on a show with more episodes than the + /// page size used to re-serve the same newest-page-size episodes forever, because the + /// per-branch LIMIT was ordered by pubdate DESC regardless of the requested sort. With 5 + /// episodes and a page size of 2, paginating with sort=date/asc must walk through all 5 + /// distinct episodes, oldest to newest, with no repeats. + #[tokio::test] + #[ignore = "requires a live Postgres; see module docs"] + async fn oldest_first_pagination_visits_every_episode_without_duplicates() { + let pool = test_pool().await; + ensure_schema(&pool).await; + let user_id = 900_001; + let podcast_id = 900_001; + cleanup(&pool, user_id, podcast_id).await; + seed_podcast(&pool, user_id, podcast_id).await; + + let episode_ids = [900_001, 900_002, 900_003, 900_004, 900_005]; + for (i, &eid) in episode_ids.iter().enumerate() { + insert_episode( + &pool, + eid, + podcast_id, + &format!("Episode {i}"), + &format!("2020-01-{:02}T00:00:00", i + 1), + 600, + false, + ) + .await; + } + + let db = DatabasePool::Postgres(pool.clone()); + let page_size = 2i64; + let mut offset = 0i64; + let mut seen: Vec = Vec::new(); + loop { + let (episodes, total) = db + .return_podcast_episodes_capitalized( + user_id, + podcast_id, + Some(page_size), + Some(offset), + "date", + "asc", + "", + "all", + ) + .await + .unwrap_or_else(|e| panic!("query episodes failed: {e}")); + assert_eq!(total, episode_ids.len() as i64); + if episodes.is_empty() { + break; + } + for ep in &episodes { + assert!( + !seen.contains(&ep.episodeid), + "episode {} was returned twice across pages — pagination is looping \ + instead of advancing (offset={offset})", + ep.episodeid + ); + seen.push(ep.episodeid); + } + offset += page_size; + if offset >= total { + break; + } + } + + assert_eq!( + seen.len(), + episode_ids.len(), + "expected every episode to be visited exactly once across pages, got {seen:?}" + ); + let mut sorted_seen = seen.clone(); + sorted_seen.sort(); + let mut expected = episode_ids.to_vec(); + expected.sort(); + assert_eq!(sorted_seen, expected, "wrong set of episodes returned overall"); + + cleanup(&pool, user_id, podcast_id).await; + } + + /// The filter push-down bug: filtering to "completed" used to run *after* the per-branch + /// LIMIT had already discarded everything but the newest N raw episodes, so a completed + /// episode outside that window was silently dropped even though the (correctly + /// unrestricted) total count said it existed. Here the two *oldest* episodes are the + /// completed ones, while a page size of 2 means the old per-branch cap would only ever + /// have seen the 2 *newest* (both incomplete) — reproducing the exact failure mode. + #[tokio::test] + #[ignore = "requires a live Postgres; see module docs"] + async fn completed_filter_finds_matches_outside_the_newest_page() { + let pool = test_pool().await; + ensure_schema(&pool).await; + let user_id = 900_002; + let podcast_id = 900_002; + cleanup(&pool, user_id, podcast_id).await; + seed_podcast(&pool, user_id, podcast_id).await; + + for i in 0..6 { + let eid = 900_010 + i; + let completed = i < 2; // the two OLDEST episodes are completed + insert_episode( + &pool, + eid, + podcast_id, + &format!("Episode {i}"), + &format!("2020-01-{:02}T00:00:00", i + 1), + 600, + completed, + ) + .await; + } + + let db = DatabasePool::Postgres(pool.clone()); + let (episodes, total) = db + .return_podcast_episodes_capitalized( + user_id, + podcast_id, + Some(2), + Some(0), + "date", + "desc", + "", + "completed", + ) + .await + .unwrap_or_else(|e| panic!("query completed episodes failed: {e}")); + + assert_eq!(total, 2, "count query should find both completed episodes"); + assert_eq!( + episodes.len(), + 2, + "rows query should also find both completed episodes, not just whichever \ + episodes happened to be in the newest page" + ); + assert!(episodes.iter().all(|e| e.completed)); + + cleanup(&pool, user_id, podcast_id).await; + } + + /// Tie-breaking: when every episode shares the same value in the requested sort column + /// (all durations equal here), SQL gives no ordering guarantee among ties on repeated + /// queries — and `per_branch_limit` grows on every page, which can change the query plan + /// (e.g. a top-N heap sort vs. a full sort) and thus which tie order comes back. Without an + /// explicit deterministic tiebreaker (added alongside `episodeid`/`videoid` per branch and + /// `is_youtube, Episodeid` at the outer level), that reordering can duplicate or skip + /// episodes across pages exactly like the original bug, just triggered by ties instead of + /// a wrong per-branch order. This asserts full, duplicate-free coverage when paginating a + /// fully-tied sort column. + #[tokio::test] + #[ignore = "requires a live Postgres; see module docs"] + async fn tied_sort_value_pagination_is_complete_without_duplicates() { + let pool = test_pool().await; + ensure_schema(&pool).await; + let user_id = 900_003; + let podcast_id = 900_003; + cleanup(&pool, user_id, podcast_id).await; + seed_podcast(&pool, user_id, podcast_id).await; + + let episode_ids = [900_020, 900_021, 900_022, 900_023, 900_024, 900_025]; + for (i, &eid) in episode_ids.iter().enumerate() { + insert_episode( + &pool, + eid, + podcast_id, + &format!("Episode {i}"), + &format!("2020-01-{:02}T00:00:00", i + 1), + 600, // identical duration for every episode: a full tie on the sort column + false, + ) + .await; + } + + let db = DatabasePool::Postgres(pool.clone()); + let page_size = 2i64; + let mut offset = 0i64; + let mut seen: Vec = Vec::new(); + loop { + let (episodes, total) = db + .return_podcast_episodes_capitalized( + user_id, + podcast_id, + Some(page_size), + Some(offset), + "duration", + "asc", + "", + "all", + ) + .await + .unwrap_or_else(|e| panic!("query tied-duration episodes failed: {e}")); + assert_eq!(total, episode_ids.len() as i64); + if episodes.is_empty() { + break; + } + for ep in &episodes { + assert!( + !seen.contains(&ep.episodeid), + "episode {} was returned twice across pages of a fully-tied sort — \ + missing a deterministic tiebreaker (offset={offset})", + ep.episodeid + ); + seen.push(ep.episodeid); + } + offset += page_size; + if offset >= total { + break; + } + } + + let mut sorted_seen = seen.clone(); + sorted_seen.sort(); + let mut expected = episode_ids.to_vec(); + expected.sort(); + assert_eq!( + sorted_seen, expected, + "expected every tied-duration episode to be visited exactly once, got {seen:?}" + ); + + cleanup(&pool, user_id, podcast_id).await; + } +} From 265d3b22c821fe9e40fa60734d9f527ecb4ba7ad Mon Sep 17 00:00:00 2001 From: Ashton Kinslow Date: Sat, 17 Oct 2026 20:30:25 -0500 Subject: [PATCH 2/3] test(api): cover mixed-source pagination on both databases --- .github/workflows/ci.yaml | 18 +- rust-api/src/database.rs | 3 + .../src/database/pagination_backend_tests.rs | 191 ++++++++++++++++++ 3 files changed, 209 insertions(+), 3 deletions(-) create mode 100644 rust-api/src/database/pagination_backend_tests.rs diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 07a1338ad..9c5ab187e 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -73,14 +73,24 @@ jobs: --health-timeout 5s --health-retries 5 + mariadb: + image: mariadb:11.4 + env: + MARIADB_ROOT_PASSWORD: test_password + MARIADB_DATABASE: test_db + ports: ["3306:3306"] + options: >- + --health-cmd "healthcheck.sh --connect --innodb_initialized" + --health-interval 5s --health-timeout 5s --health-retries 10 + steps: - uses: actions/checkout@v4 - uses: hecrj/setup-rust-action@v2 with: - rust-version: 1.95 + rust-version: "1.99.0" - - name: Run episode pagination/sort/filter regression tests + - name: Run API and pagination regressions on PostgreSQL and MariaDB working-directory: ./rust-api env: DB_HOST: localhost @@ -88,7 +98,9 @@ jobs: DB_USER: test_user DB_PASSWORD: test_password DB_NAME: test_db - run: cargo test episode_pagination -- --ignored --test-threads=1 + PINEPODS_TEST_POSTGRES_URL: postgres://test_user:test_password@localhost:5432/test_db + PINEPODS_TEST_MYSQL_URL: mysql://root:test_password@127.0.0.1:3306/test_db + run: cargo test --locked -- --include-ignored --test-threads=1 frontend-tests: runs-on: ubuntu-latest diff --git a/rust-api/src/database.rs b/rust-api/src/database.rs index 528b78757..8c56eb62d 100644 --- a/rust-api/src/database.rs +++ b/rust-api/src/database.rs @@ -1,3 +1,6 @@ +#[cfg(test)] +mod pagination_backend_tests; + use sqlx::{MySql, Pool, Postgres, Row}; use std::time::Duration; use crate::{config::{Config, OIDCConfig}, error::{AppError, AppResult}}; diff --git a/rust-api/src/database/pagination_backend_tests.rs b/rust-api/src/database/pagination_backend_tests.rs new file mode 100644 index 000000000..be2ab4cf9 --- /dev/null +++ b/rust-api/src/database/pagination_backend_tests.rs @@ -0,0 +1,191 @@ +//! Run with explicit disposable database URLs: cargo test pagination_backend_tests -- --ignored. +//! Each backend gets a unique schema/database; existing tables are never touched. + +use super::DatabasePool; + +async fn execute(db: &DatabasePool, sql: &str) { + match db { + DatabasePool::Postgres(pool) => { + sqlx::query(sqlx::AssertSqlSafe(sql)) + .execute(pool) + .await + .unwrap(); + } + DatabasePool::MySQL(pool) => { + // Column names are case-insensitive; table names retain their schema spelling. + let sql = sql.replace('"', "").replace("is_video", "IsVideo"); + sqlx::query(sqlx::AssertSqlSafe(sql.as_str())) + .execute(pool) + .await + .unwrap(); + } + } +} + +async fn fixtures(db: &DatabasePool) { + for sql in [ + r#"CREATE TABLE "Users" (userid INT PRIMARY KEY, usepodcastcovers BOOLEAN DEFAULT FALSE, playbackspeed DECIMAL(3,2))"#, + r#"CREATE TABLE "Podcasts" (podcastid INT PRIMARY KEY, userid INT, podcastname TEXT, artworkurl TEXT, + usepodcastcovers BOOLEAN DEFAULT FALSE, usepodcastcoverscustomized BOOLEAN DEFAULT FALSE, + mergedpodcastids TEXT, playbackspeed DECIMAL(3,2), playbackspeedcustomized BOOLEAN, + startskip INT, endskip INT)"#, + r#"CREATE TABLE "Episodes" (episodeid INT PRIMARY KEY, podcastid INT, episodetitle TEXT, + episodepubdate TIMESTAMP, episodedescription TEXT, episodeartwork TEXT, episodeurl TEXT, + episodeduration INT, completed BOOLEAN DEFAULT FALSE, is_video BOOLEAN DEFAULT FALSE)"#, + r#"CREATE TABLE "YouTubeVideos" (videoid INT PRIMARY KEY, podcastid INT, videotitle TEXT, + publishedat TIMESTAMP, videodescription TEXT, thumbnailurl TEXT, videourl TEXT, + duration INT, listenposition INT, completed BOOLEAN DEFAULT FALSE)"#, + r#"CREATE TABLE "UserEpisodeHistory" (userid INT, episodeid INT, listenduration INT)"#, + r#"CREATE TABLE "SavedEpisodes" (userid INT, episodeid INT)"#, + r#"CREATE TABLE "DownloadedEpisodes" (userid INT, episodeid INT)"#, + r#"CREATE TABLE "EpisodeQueue" (userid INT, episodeid INT, is_youtube BOOLEAN)"#, + r#"CREATE TABLE "SavedVideos" (userid INT, videoid INT)"#, + r#"CREATE TABLE "DownloadedVideos" (userid INT, videoid INT)"#, + r#"INSERT INTO "Users" VALUES (1, FALSE, 1.75), (2, FALSE, NULL)"#, + r#"INSERT INTO "Podcasts" (podcastid, userid, podcastname, mergedpodcastids, playbackspeed, + playbackspeedcustomized, startskip, endskip) VALUES + (1, 1, 'Main', '[2,3,2]', 2.50, FALSE, 10, 20), + (2, 1, 'Merged', NULL, 2.25, TRUE, 30, 40), + (3, 2, 'Other user', NULL, 3.00, TRUE, 50, 60), + (4, 1, 'Null override', NULL, NULL, TRUE, NULL, NULL), + (5, 2, 'Defaults', NULL, NULL, NULL, NULL, NULL)"#, + ] { + execute(db, sql).await; + } + for id in 1..=6 { + let podcast = if id % 2 == 0 { 2 } else { 1 }; + let done = if id <= 2 { "TRUE" } else { "FALSE" }; + // Adjacent items tie on each sort key; RSS and YouTube also share IDs. + let key = (id + 1) / 2; + execute(db, &format!(r#"INSERT INTO "Episodes" VALUES + ({id}, {podcast}, 'Title {key}', '2026-01-0{key} 00:00:00', 'searchable', '', 'https://example.test/a', + {duration}, {done}, FALSE)"#, duration = 700 - key * 100)).await; + execute(db, &format!(r#"INSERT INTO "YouTubeVideos" VALUES + ({id}, {podcast}, 'Title {key}', '2026-01-0{key} 00:00:00', 'searchable', '', 'https://example.test/v', + {duration}, 15, {done})"#, duration = 700 - key * 100)).await; + execute( + db, + &format!(r#"INSERT INTO "UserEpisodeHistory" VALUES (1, {id}, 15)"#), + ) + .await; + } + // A merged ID belonging to someone else must not leak into either results or totals. + execute( + db, + r#"INSERT INTO "Episodes" VALUES + (99, 3, 'Other', '2026-01-01 00:00:00', '', '', '', 1, FALSE, FALSE)"#, + ) + .await; +} + +async fn check_pages(db: &DatabasePool) { + for sort in ["date", "title", "duration"] { + for direction in ["asc", "desc"] { + for filter in ["", "completed", "incomplete", "in_progress"] { + let mut expected: Vec<(bool, i32)> = (1..=6) + .filter(|id| match filter { + "completed" => *id <= 2, + "incomplete" | "in_progress" => *id > 2, + _ => true, + }) + .flat_map(|id| [(false, id), (true, id)]) + .collect(); + expected.sort_by_key(|(youtube, id)| { + let mut key = (id + 1) / 2; + if sort == "duration" { + key = -key; + } + if direction == "desc" { + key = -key; + } + (key, *youtube, *id) + }); + let mut actual = Vec::new(); + for offset in (0..=expected.len()).step_by(3) { + let (page, total) = db + .return_podcast_episodes_capitalized( + 1, + 1, + Some(3), + Some(offset as i64), + sort, + direction, + "", + filter, + ) + .await + .unwrap(); + assert_eq!(total, expected.len() as i64); + actual.extend(page.into_iter().map(|e| (e.is_youtube, e.episodeid))); + } + assert_eq!(actual, expected, "{sort}/{direction}/{filter}"); + } + } + } + let (page, total) = db + .return_podcast_episodes_capitalized( + 1, + 1, + Some(2), + Some(0), + "date", + "asc", + "Title 2", + "in_progress", + ) + .await + .unwrap(); + assert_eq!(total, 4); + assert_eq!( + page.iter().map(|e| e.episodeid).collect::>(), + vec![3, 4] + ); + let (page, total) = db + .return_podcast_episodes_capitalized(2, 1, Some(3), Some(0), "date", "asc", "", "") + .await + .unwrap(); + assert!(page.is_empty()); + assert_eq!(total, 0); +} + +#[tokio::test] +#[ignore = "requires PINEPODS_TEST_POSTGRES_URL pointing to a disposable test server"] +async fn postgres_mixed_source_pagination() { + let url = std::env::var("PINEPODS_TEST_POSTGRES_URL").expect("explicit test URL required"); + let pool = sqlx::postgres::PgPoolOptions::new() + .max_connections(1) + .idle_timeout(None) + .max_lifetime(None) + .connect(&url) + .await + .unwrap(); + let db = DatabasePool::Postgres(pool.clone()); + let schema = format!("pinepods_test_{}", uuid::Uuid::new_v4().simple()); + execute(&db, &format!("CREATE SCHEMA {schema}")).await; + execute(&db, &format!("SET search_path TO {schema}")).await; + fixtures(&db).await; + check_pages(&db).await; + execute(&db, &format!("DROP SCHEMA {schema} CASCADE")).await; + pool.close().await; +} + +#[tokio::test] +#[ignore = "requires PINEPODS_TEST_MYSQL_URL pointing to a disposable test server"] +async fn mysql_mixed_source_pagination() { + let url = std::env::var("PINEPODS_TEST_MYSQL_URL").expect("explicit test URL required"); + let pool = sqlx::mysql::MySqlPoolOptions::new() + .max_connections(1) + .idle_timeout(None) + .max_lifetime(None) + .connect(&url) + .await + .unwrap(); + let db = DatabasePool::MySQL(pool.clone()); + let schema = format!("pinepods_test_{}", uuid::Uuid::new_v4().simple()); + execute(&db, &format!("CREATE DATABASE {schema}")).await; + execute(&db, &format!("USE {schema}")).await; + fixtures(&db).await; + check_pages(&db).await; + execute(&db, &format!("DROP DATABASE {schema}")).await; + pool.close().await; +} From 2fd9557bdf7ebb42397c1bba74177a80f32bd552 Mon Sep 17 00:00:00 2001 From: Ashton Kinslow Date: Sat, 17 Oct 2026 21:07:57 -0500 Subject: [PATCH 3/3] test(api): support MySQL fixture database selection --- rust-api/src/database/pagination_backend_tests.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/rust-api/src/database/pagination_backend_tests.rs b/rust-api/src/database/pagination_backend_tests.rs index be2ab4cf9..8d8487b80 100644 --- a/rust-api/src/database/pagination_backend_tests.rs +++ b/rust-api/src/database/pagination_backend_tests.rs @@ -14,7 +14,8 @@ async fn execute(db: &DatabasePool, sql: &str) { DatabasePool::MySQL(pool) => { // Column names are case-insensitive; table names retain their schema spelling. let sql = sql.replace('"', "").replace("is_video", "IsVideo"); - sqlx::query(sqlx::AssertSqlSafe(sql.as_str())) + // MySQL does not support USE through the prepared-statement protocol. + sqlx::raw_sql(sqlx::AssertSqlSafe(sql.as_str())) .execute(pool) .await .unwrap();