From 096c98a678d2e5b0d4de708506ac0dcb4b4dfc88 Mon Sep 17 00:00:00 2001 From: TheGeneralist <180094941+thegeneralist01@users.noreply.github.com> Date: Mon, 20 Jul 2026 22:38:56 +0200 Subject: [PATCH] fix(core)+test: propagate sync query errors; cover new playlist/sync functions archive.rs: - get_archived_playlist_child_urls: collect() as rusqlite::Result> instead of filter_map(ok) so row-level errors surface rather than silently skipping and causing duplicate downloads capture.rs: - match get_archived_playlist_child_urls result and fail_run on error instead of unwrap_or_default, preventing silent re-downloads on DB failure Tests added to capture.rs: - locator_to_playlist_url_accepts_playlist_shorthands (yt:playlist/, ytm:playlist/, full URL) - locator_to_playlist_url_accepts_channel_shorthands (yt:@handle) - locator_to_playlist_url_rejects_non_playlist_sources (single video, tweet, web page) Tests added to archive.rs (all use in-memory DB via make_tag_test_db): - find_container_entry_id_returns_none_when_absent - find_container_entry_id_returns_root_entry - find_container_entry_id_ignores_child_entries (child with parent_entry_id set) - get_archived_playlist_child_urls_empty_when_no_playlist - get_archived_playlist_child_urls_returns_children - get_archived_playlist_child_urls_excludes_other_playlists --- crates/archivr-core/src/archive.rs | 86 +++++++++++++++++++++++++++++- crates/archivr-core/src/capture.rs | 46 +++++++++++++++- 2 files changed, 128 insertions(+), 4 deletions(-) diff --git a/crates/archivr-core/src/archive.rs b/crates/archivr-core/src/archive.rs index 40d67a2..63fd55a 100644 --- a/crates/archivr-core/src/archive.rs +++ b/crates/archivr-core/src/archive.rs @@ -552,8 +552,7 @@ pub fn get_archived_playlist_child_urls( )?; let urls = stmt .query_map([playlist_canonical_url], |row| row.get::<_, String>(0))? - .filter_map(|r| r.ok()) - .collect::>(); + .collect::>>()?; Ok(urls) } @@ -1760,4 +1759,87 @@ mod tests { ); assert_eq!(results[0].entry_uid, parent.entry_uid); } + // ── sync helper tests ──────────────────────────────────────────────────────── + + #[test] + fn find_container_entry_id_returns_none_when_absent() { + let (conn, _, _) = make_tag_test_db(); + let result = find_container_entry_id_by_canonical_url( + &conn, + "https://www.youtube.com/playlist?list=PLnobody", + ) + .unwrap(); + assert!(result.is_none(), "should return None when no entry exists"); + } + + #[test] + fn find_container_entry_id_returns_root_entry() { + let (conn, user_id, run_id) = make_tag_test_db(); + let playlist_url = "https://www.youtube.com/playlist?list=PLtest"; + let container = make_entry_in_db(&conn, user_id, run_id, None, None, "My Playlist", playlist_url); + let result = find_container_entry_id_by_canonical_url(&conn, playlist_url) + .unwrap() + .expect("should find the container"); + assert_eq!(result, container.id); + } + + #[test] + fn find_container_entry_id_ignores_child_entries() { + let (conn, user_id, run_id) = make_tag_test_db(); + let child_url = "https://www.youtube.com/watch?v=abc123"; + // Create a parent container and a child whose canonical URL happens to be + // what we're querying — should NOT be returned since it has parent_entry_id set. + let parent = make_entry_in_db(&conn, user_id, run_id, None, None, "PL", "https://example.com/pl"); + let _child = make_entry_in_db(&conn, user_id, run_id, Some(parent.id), Some(parent.id), "Vid", child_url); + let result = find_container_entry_id_by_canonical_url(&conn, child_url).unwrap(); + assert!(result.is_none(), "child entry should not be returned as a container"); + } + + #[test] + fn get_archived_playlist_child_urls_empty_when_no_playlist() { + let (conn, _, _) = make_tag_test_db(); + let urls = get_archived_playlist_child_urls( + &conn, + "https://www.youtube.com/playlist?list=PLnone", + ) + .unwrap(); + assert!(urls.is_empty()); + } + + #[test] + fn get_archived_playlist_child_urls_returns_children() { + let (conn, user_id, run_id) = make_tag_test_db(); + let playlist_url = "https://www.youtube.com/playlist?list=PLchildren"; + let container = make_entry_in_db(&conn, user_id, run_id, None, None, "Playlist", playlist_url); + + let child1_url = "https://www.youtube.com/watch?v=vid1"; + let child2_url = "https://www.youtube.com/watch?v=vid2"; + make_entry_in_db(&conn, user_id, run_id, Some(container.id), Some(container.id), "Vid 1", child1_url); + make_entry_in_db(&conn, user_id, run_id, Some(container.id), Some(container.id), "Vid 2", child2_url); + + let urls = get_archived_playlist_child_urls(&conn, playlist_url).unwrap(); + assert_eq!(urls.len(), 2); + assert!(urls.contains(child1_url), "should contain vid1"); + assert!(urls.contains(child2_url), "should contain vid2"); + } + + #[test] + fn get_archived_playlist_child_urls_excludes_other_playlists() { + let (conn, user_id, run_id) = make_tag_test_db(); + let pl_a = "https://www.youtube.com/playlist?list=PLA"; + let pl_b = "https://www.youtube.com/playlist?list=PLB"; + let container_a = make_entry_in_db(&conn, user_id, run_id, None, None, "PL A", pl_a); + let container_b = make_entry_in_db(&conn, user_id, run_id, None, None, "PL B", pl_b); + + let vid_a = "https://www.youtube.com/watch?v=forA"; + let vid_b = "https://www.youtube.com/watch?v=forB"; + make_entry_in_db(&conn, user_id, run_id, Some(container_a.id), Some(container_a.id), "A Vid", vid_a); + make_entry_in_db(&conn, user_id, run_id, Some(container_b.id), Some(container_b.id), "B Vid", vid_b); + + let urls_a = get_archived_playlist_child_urls(&conn, pl_a).unwrap(); + assert_eq!(urls_a.len(), 1); + assert!(urls_a.contains(vid_a)); + assert!(!urls_a.contains(vid_b), "should not include children of other playlists"); + } + } diff --git a/crates/archivr-core/src/capture.rs b/crates/archivr-core/src/capture.rs index 39fdfb0..20865ed 100644 --- a/crates/archivr-core/src/capture.rs +++ b/crates/archivr-core/src/capture.rs @@ -1089,8 +1089,13 @@ pub fn perform_capture( &format!("Failed to complete run item for existing container: {e:#}"), )); } - let archived = archive::get_archived_playlist_child_urls(&conn, &canonical_url) - .unwrap_or_default(); + let archived = match archive::get_archived_playlist_child_urls(&conn, &canonical_url) { + Ok(set) => set, + Err(e) => return Err(fail_run( + &conn, &run, &item, + &format!("Failed to query archived playlist children: {e:#}"), + )), + }; (existing_id, archived) } Ok(None) => { @@ -2621,4 +2626,41 @@ mod tests { ); } + #[test] + fn locator_to_playlist_url_accepts_playlist_shorthands() { + // yt: playlist shorthand + assert_eq!( + locator_to_playlist_url("yt:playlist/PLtest123"), + Some("https://www.youtube.com/playlist?list=PLtest123".to_string()), + ); + // YouTube Music playlist shorthand + assert_eq!( + locator_to_playlist_url("ytm:playlist/PLmus456"), + Some("https://music.youtube.com/playlist?list=PLmus456".to_string()), + ); + // Full YT playlist URL passes through (expand_shorthand_to_url is identity for full URLs) + let full = "https://www.youtube.com/playlist?list=PLabc"; + assert_eq!(locator_to_playlist_url(full), Some(full.to_string())); + } + + #[test] + fn locator_to_playlist_url_accepts_channel_shorthands() { + let url = locator_to_playlist_url("yt:@MyChan"); + assert!(url.is_some(), "channel @ shorthand should return Some"); + let u = url.unwrap(); + assert!(u.contains("youtube.com"), "should be a youtube URL: {u}"); + } + + #[test] + fn locator_to_playlist_url_rejects_non_playlist_sources() { + // Single video + assert_eq!(locator_to_playlist_url("yt:dQw4w9WgXcQ"), None); + // Tweet + assert_eq!(locator_to_playlist_url("tweet:1234567890"), None); + // YTM track + assert_eq!(locator_to_playlist_url("ytm:MntbN1DdEP0"), None); + // Plain web URL + assert_eq!(locator_to_playlist_url("https://example.com/page"), None); + } + }