diff --git a/crates/qbz-library/src/database.rs b/crates/qbz-library/src/database.rs index dae54bfcc..e15a83fc7 100644 --- a/crates/qbz-library/src/database.rs +++ b/crates/qbz-library/src/database.rs @@ -5730,8 +5730,40 @@ impl LibraryDatabase { Ok(()) } - /// Remove a downloaded purchase record (e.g. user deleted the file). - pub fn remove_downloaded_purchase(&self, track_id: i64) -> Result<(), LibraryError> { + /// Remove ONE downloaded purchase record (e.g. the user deleted the file). + /// + /// The table's primary key is `(track_id, format_id)` — the same purchased + /// track may be downloaded in several formats at once, and the whole + /// "downloaded" UI is format-scoped because of it. This delete is therefore + /// keyed by the full pair. It previously deleted by `track_id` alone, which + /// silently dropped the OTHER formats' rows; that never fired because nothing + /// called it, and it is corrected here before Qt gives it a caller. The stale + /// prune in `get_downloaded_purchase_track_ids` already deletes by the pair. + pub fn remove_downloaded_purchase( + &self, + track_id: i64, + format_id: i64, + ) -> Result<(), LibraryError> { + self.conn + .execute( + "DELETE FROM downloaded_purchases WHERE track_id = ?1 AND format_id = ?2", + rusqlite::params![track_id, format_id], + ) + .map_err(|e| { + LibraryError::Database(format!("Failed to remove downloaded purchase: {}", e)) + })?; + Ok(()) + } + + /// Remove EVERY downloaded record for a track, across all formats. + /// + /// Split out from `remove_downloaded_purchase` so that erasing all formats is + /// something a caller has to ask for by name rather than something it gets by + /// accident from an under-specified key. + pub fn remove_downloaded_purchase_all_formats( + &self, + track_id: i64, + ) -> Result<(), LibraryError> { self.conn .execute( "DELETE FROM downloaded_purchases WHERE track_id = ?1", @@ -5743,6 +5775,73 @@ impl LibraryDatabase { Ok(()) } + /// Count DISTINCT downloaded tracks per purchased album id. + /// + /// This is what makes the Albums tab's "downloaded" mark and its + /// "Hide downloaded" filter work at all. The reference derived an album's + /// downloaded state from `album.tracks.items` in the purchases response, but + /// `getUserPurchases?type=albums` carries no nested tracks page (measured + /// against a live account, contract §2.5b), so that predicate is unsatisfiable + /// and the filter can never hide anything. The local registry is the only + /// place that knows, and `album_id` — written since the table was created and + /// never read until now — is the column that answers it. There is already an + /// index on it (`idx_downloaded_purchases_album`). + /// + /// Counting is format-AGNOSTIC (`DISTINCT track_id`), matching the reference's + /// list-level rule: a track downloaded in two formats counts once. Rows with a + /// NULL `album_id` are skipped — they cannot be attributed to an album. + /// + /// **Rows whose file no longer exists on disk are excluded**, the same way + /// `get_downloaded_purchase_track_ids` excludes them. Without that, an album + /// whose files the user deleted keeps reporting as downloaded — and, worse, + /// stays HIDDEN behind "Hide downloaded" — until some unrelated call happens + /// to prune first. Making the answer depend on which accessor ran last is the + /// kind of order-coupling nobody remembers six months later, so this query + /// stands on its own. + /// + /// Unlike the track-ids accessor this one only FILTERS; it does not delete. + /// Pruning is left to that accessor so there is exactly one writer, and a + /// read taken while a download is mid-flight cannot race it. + /// + /// The caller compares each count against the album's `tracks_count`; this + /// method deliberately does not decide "fully downloaded", because only the + /// caller holds the purchase metadata. + pub fn get_downloaded_purchase_album_counts( + &self, + ) -> Result, LibraryError> { + let mut stmt = self + .conn + .prepare( + "SELECT album_id, track_id, file_path FROM downloaded_purchases + WHERE album_id IS NOT NULL", + ) + .map_err(|e| LibraryError::Database(format!("Failed to prepare statement: {}", e)))?; + + let rows: Vec<(String, i64, String)> = stmt + .query_map([], |row| Ok((row.get(0)?, row.get(1)?, row.get(2)?))) + .map_err(|e| { + LibraryError::Database(format!("Failed to query purchase album counts: {}", e)) + })? + .collect::, _>>() + .map_err(|e| LibraryError::Database(format!("Failed to collect album counts: {}", e)))?; + + // DISTINCT over (album_id, track_id) is done here rather than in SQL so + // the existence check can drop a row before it is counted. + let mut seen: std::collections::HashSet<(String, i64)> = std::collections::HashSet::new(); + let mut counts: std::collections::HashMap = std::collections::HashMap::new(); + + for (album_id, track_id, file_path) in rows { + if !std::path::Path::new(&file_path).exists() { + continue; + } + if seen.insert((album_id.clone(), track_id)) { + *counts.entry(album_id).or_insert(0) += 1; + } + } + + Ok(counts) + } + /// Get all downloaded track IDs for fast lookup (any format). /// Automatically removes stale entries where the file no longer exists on disk. pub fn get_downloaded_purchase_track_ids(&self) -> Result, LibraryError> { @@ -6245,6 +6344,133 @@ mod folder_tree_tests { db.insert_track(&t).unwrap(); } + // ── §12-5: the gold purchase badge, end to end ─────────────────────── + // + // The badge chain is: a purchase download writes `downloaded_purchases` + // (file_path, format_id) → a later library scan inserts the same file → + // `insert_track` stamps `source = 'qobuz_purchase'` → the UI branches on + // that literal to draw the gold mark. + // + // The join is by EXACT `file_path` string equality, which is the whole + // reason these tests exist. Nobody here can smoke-test Purchases, and a + // path that differs by one character — a Unicode title sanitized + // differently, a trailing space from an empty quality folder — breaks the + // stamp silently: the file is there, the registry row is there, and the + // badge simply never appears with nothing logged. + + #[test] + fn a_scanned_file_matching_the_registry_is_stamped_as_a_purchase() { + let tmp = tempfile::tempdir().unwrap(); + let db = LibraryDatabase::open(&tmp.path().join("lib.db")).unwrap(); + + let path = "/music/Artist/Album [FLAC][24-bit,96kHz]/01 - Song.flac"; + db.mark_purchase_downloaded(1001, Some("alb-1"), path, 7) + .unwrap(); + + insert_at(&db, path, Some(1), Some(1), "Song"); + + let source: String = db + .with_connection(|conn| { + conn.query_row( + "SELECT source FROM local_tracks WHERE file_path = ?1", + rusqlite::params![path], + |row| row.get(0), + ) + .map_err(|e| LibraryError::Database(e.to_string())) + }) + .unwrap(); + + assert_eq!( + source, "qobuz_purchase", + "the scan must stamp a file that the purchase registry already knows" + ); + } + + #[test] + fn a_scanned_file_the_registry_does_not_know_stays_user() { + let tmp = tempfile::tempdir().unwrap(); + let db = LibraryDatabase::open(&tmp.path().join("lib.db")).unwrap(); + + insert_at(&db, "/music/Other/Album/01 - Song.flac", Some(1), Some(1), "Song"); + + let source: String = db + .with_connection(|conn| { + conn.query_row( + "SELECT source FROM local_tracks WHERE file_path = ?1", + rusqlite::params!["/music/Other/Album/01 - Song.flac"], + |row| row.get(0), + ) + .map_err(|e| LibraryError::Database(e.to_string())) + }) + .unwrap(); + + assert_eq!(source, "user"); + } + + /// The failure mode the exact-equality join actually has. A registry row and + /// a scanned file that differ by ONE character — here the trailing space an + /// implementer gets by formatting the album folder as `"{album} {quality}"` + /// with an empty quality — do not join, and the badge silently never appears. + /// + /// This is a characterisation test: it asserts the join is exact, so that if + /// anyone ever makes it fuzzy they have to come here and say so deliberately. + #[test] + fn a_one_character_path_difference_breaks_the_stamp_silently() { + let tmp = tempfile::tempdir().unwrap(); + let db = LibraryDatabase::open(&tmp.path().join("lib.db")).unwrap(); + + db.mark_purchase_downloaded(1002, Some("alb-2"), "/music/A/Album /01 - S.flac", 6) + .unwrap(); + insert_at(&db, "/music/A/Album/01 - S.flac", Some(1), Some(1), "S"); + + let source: String = db + .with_connection(|conn| { + conn.query_row( + "SELECT source FROM local_tracks WHERE file_path = ?1", + rusqlite::params!["/music/A/Album/01 - S.flac"], + |row| row.get(0), + ) + .map_err(|e| LibraryError::Database(e.to_string())) + }) + .unwrap(); + + assert_eq!( + source, "user", + "documented: the join is exact, so a one-character path drift loses the badge" + ); + } + + /// A re-scan re-stamps. The contract records that if a scan runs BEFORE the + /// registry write the row is stamped `'user'` until the next scan of that + /// folder — this proves the "until" half, which is why no repair migration + /// or re-stamp UI is owed. + #[test] + fn a_rescan_after_the_registry_write_upgrades_the_stamp() { + let tmp = tempfile::tempdir().unwrap(); + let db = LibraryDatabase::open(&tmp.path().join("lib.db")).unwrap(); + let path = "/music/A/Album/01 - Late.flac"; + + // Scan first: nothing in the registry yet. + insert_at(&db, path, Some(1), Some(1), "Late"); + + // The download registers afterwards, then the folder is scanned again. + db.mark_purchase_downloaded(1003, Some("alb-3"), path, 6).unwrap(); + insert_at(&db, path, Some(1), Some(1), "Late"); + + let source: String = db + .with_connection(|conn| { + conn.query_row( + "SELECT source FROM local_tracks WHERE file_path = ?1", + rusqlite::params![path], + |row| row.get(0), + ) + .map_err(|e| LibraryError::Database(e.to_string())) + }) + .unwrap(); + + assert_eq!(source, "qobuz_purchase", "the next scan re-stamps it"); + } + /// Insert a row directly with `source = 'qobuz_download'`. /// `insert_track` overrides the source field so we go through raw /// SQL to model the offline-cache code path that DOES write that diff --git a/crates/qbz-library/src/lib.rs b/crates/qbz-library/src/lib.rs index 85fd785e1..58f2355d0 100644 --- a/crates/qbz-library/src/lib.rs +++ b/crates/qbz-library/src/lib.rs @@ -56,7 +56,8 @@ pub use models::*; pub use mount_info::{is_network_path, network_fs_label}; pub use scan::{scan_with_progress, ScanEvent}; pub use tag_writer::{ - compute_track_artist_match, write_album_tags_to_files, AlbumTagWrite, TrackTagWrite, + compute_track_artist_match, write_album_tags_to_files, write_purchase_tags, AlbumTagWrite, + PurchaseTagWrite, TrackTagWrite, }; pub use scanner::{LibraryScanner, ScanResult}; pub use thumbnails::{ diff --git a/crates/qbz-library/src/tag_writer.rs b/crates/qbz-library/src/tag_writer.rs index 54f5797a9..35717137a 100644 --- a/crates/qbz-library/src/tag_writer.rs +++ b/crates/qbz-library/src/tag_writer.rs @@ -124,21 +124,32 @@ pub fn write_album_tags_to_files( let mut tagged_file = lofty::read_from_path(path) .map_err(|_| LibraryError::Metadata("Failed to read audio file tags.".to_string()))?; + // ALWAYS the format's PRIMARY tag type. There used to be a "primary, or + // else the first tag I can find" fallback here, and it silently threw + // away the user's edit: + // + // lofty will not WRITE a tag type a container only supports READING, and + // it refuses silently — the save loop skips non-writable tags with + // `continue` rather than erroring, because callers are not expected to + // know which ones are read-only. A FLAC that carries an ID3v2 tag (an + // ordinary shape; plenty of tooling stamps one on) has NO Vorbis + // comments, so `primary_tag_mut()` was `None` and the fallback handed + // back that ID3v2 tag. Every edited field went into it, `save_to_path` + // dropped the lot, and this function returned `Ok(())`. The user pressed + // Save, saw a success toast, and nothing changed on disk. + // + // Reading through `first_tag()` is still fine and still happens in + // `uniform_file_artist` — read-only tags are, after all, readable. It is + // only the WRITE target that must be the primary type. let primary_type = tagged_file.primary_tag_type(); - if tagged_file.primary_tag_mut().is_none() && tagged_file.first_tag_mut().is_none() { + if tagged_file.primary_tag_mut().is_none() { tagged_file.insert_tag(Tag::new(primary_type)); } { - let tag = if let Some(tag) = tagged_file.primary_tag_mut() { - tag - } else if let Some(tag) = tagged_file.first_tag_mut() { - tag - } else { - return Err(LibraryError::Metadata( - "Failed to access audio file tags.".to_string(), - )); - }; + let tag = tagged_file.primary_tag_mut().ok_or_else(|| { + LibraryError::Metadata("Failed to access audio file tags.".to_string()) + })?; tag.set_title(track.title.trim().to_string()); tag.set_album(album.album_title.trim().to_string()); @@ -238,6 +249,276 @@ pub fn compute_track_artist_match(tracks: &[LocalTrack]) -> Option { artists.into_iter().next() } +// ─── Purchase downloads ────────────────────────────────────────────────────── + +/// Everything a freshly downloaded purchased track should carry in its embedded +/// tags. Every field comes from the API payload the download already fetched +/// (`/track/get` runs before every purchase download), so nothing here is +/// guessed from the file. +#[derive(Debug, Clone, Default)] +pub struct PurchaseTagWrite { + pub title: String, + /// Track subtitle ("Live", "Remastered"). Written to its own tag rather than + /// glued onto TITLE, so the value survives losslessly and TITLE keeps + /// matching the on-disk filename. + pub version: Option, + /// The track's OWN performer. This is the field that makes a purpose-built + /// writer necessary — see [`write_purchase_tags`]. + pub artist: String, + pub album_title: String, + pub album_artist: String, + pub track_number: Option, + pub disc_number: Option, + pub year: Option, + pub genre: Option, + pub label: Option, + pub isrc: Option, + pub composer: Option, + pub copyright: Option, +} + +/// Tag a freshly downloaded purchased file, optionally embedding cover art. +/// +/// **Why this is not `write_album_tags_to_files`.** That writer serves the +/// metadata EDITOR, and two of its defining behaviours are wrong here: +/// +/// 1. It never writes ARTIST outright. It renames the track artist only where +/// the file's existing value still matches the album's previously-uniform +/// one (`should_rename_artist`), and it derives that prior value by READING +/// the files. A just-downloaded file has no tags at all, so the uniform +/// lookup yields `None`, the rename is declined, and every purchased track +/// would land with NO artist. A purchase knows the real per-track performer +/// from the API, so it writes it directly and skips the rename machinery. +/// 2. It is destructive by design — a `None` field REMOVES that tag, which is +/// what an editor means by clearing a field. A download has nothing to +/// clear, so absent fields are skipped instead. +/// +/// It is also single-file and independent: the album loop tags each track as it +/// lands, so a cancelled or partly-failed album still leaves correct tags on +/// whatever did download. +/// +/// **Failure is never fatal to the download.** The file is the deliverable; tags +/// are an enhancement over the reference, which wrote none at all. Callers log +/// the error and move on. +pub fn write_purchase_tags( + file_path: &str, + meta: &PurchaseTagWrite, + cover_jpeg: Option<&[u8]>, +) -> Result<(), LibraryError> { + use lofty::config::WriteOptions; + use lofty::picture::{MimeType, Picture, PictureType}; + use lofty::prelude::*; + use lofty::tag::{ItemKey, Tag}; + + let path = Path::new(file_path); + let mut tagged_file = lofty::read_from_path(path) + .map_err(|e| LibraryError::Metadata(format!("Failed to read downloaded file tags: {e}")))?; + + // ALWAYS the format's PRIMARY tag type — never a "first tag we can find" + // fallback. This is the difference between tagging the file and silently + // tagging nothing: + // + // lofty will not WRITE a tag type a container only supports READING. A FLAC + // may legitimately arrive carrying an ID3v2 tag; for such a file + // `primary_tag_mut()` (VorbisComments) is `None` while `first_tag_mut()` is + // `Some(id3v2)`. A fallback would happily fill that ID3v2 tag, and then + // `save_to_path` would SKIP it — `tagged_file.rs` does `continue` on any + // non-writable tag rather than erroring, precisely because callers are not + // expected to know which ones are read-only. The result is a file with none + // of its tags and a `Ok(())` return: no log line, no failure, nothing to + // notice. The same trap costs six fields plus all artwork on an MP3 that + // carries only ID3v1, whose key set is eight entries wide. + // + // Inserting the primary type is safe unconditionally — `insert_tag` replaces + // any same-type tag — but it is done only when absent so an existing tag's + // unrelated fields survive. + let primary_type = tagged_file.primary_tag_type(); + if tagged_file.primary_tag_mut().is_none() { + tagged_file.insert_tag(Tag::new(primary_type)); + } + + { + let tag = tagged_file.primary_tag_mut().ok_or_else(|| { + LibraryError::Metadata("Failed to access downloaded file tags.".to_string()) + })?; + + let set_text = |tag: &mut Tag, key: ItemKey, value: Option<&str>| { + if let Some(value) = value.map(str::trim).filter(|v| !v.is_empty()) { + tag.insert_text(key, value.to_string()); + } + }; + + // Empties are SKIPPED, not written. A download has nothing to clear, and + // an explicit empty TITLE/ALBUM is worse than an absent one — it defeats + // any later repair pass that looks for missing tags. + if !meta.title.trim().is_empty() { + tag.set_title(meta.title.trim().to_string()); + } + if !meta.album_title.trim().is_empty() { + tag.set_album(meta.album_title.trim().to_string()); + } + + // Written unconditionally, from the API — never inferred from the file. + if !meta.artist.trim().is_empty() { + tag.set_artist(meta.artist.trim().to_string()); + } + + // `Track.track_number` is a non-optional `u32` that DEFAULTS to 0 when + // the API omits it, so an unguarded write stamps `TRACKNUMBER=0` on + // singles and unnumbered releases. Zero is not a track number. + if let Some(no) = meta.track_number.filter(|n| *n > 0) { + tag.set_track(no); + } + if let Some(disc) = meta.disc_number.filter(|d| *d > 0) { + tag.set_disk(disc); + } + if let Some(year) = meta.year { + tag.set_date(lofty::tag::items::Timestamp { + year: year as u16, + ..Default::default() + }); + } + if let Some(genre) = meta.genre.as_deref().map(str::trim).filter(|g| !g.is_empty()) { + tag.set_genre(genre.to_string()); + } + + set_text(tag, ItemKey::AlbumArtist, Some(meta.album_artist.as_str())); + set_text(tag, ItemKey::TrackSubtitle, meta.version.as_deref()); + set_text(tag, ItemKey::Label, meta.label.as_deref()); + set_text(tag, ItemKey::Isrc, meta.isrc.as_deref()); + set_text(tag, ItemKey::Composer, meta.composer.as_deref()); + set_text(tag, ItemKey::CopyrightMessage, meta.copyright.as_deref()); + + if let Some(bytes) = cover_jpeg.filter(|b| !b.is_empty()) { + // Drop any existing embedded art before pushing. On the download path + // the file is always brand new, so today this is a no-op — but + // `push_picture` APPENDS, so the moment anything re-tags an existing + // file (a retry against a file the rename did not replace, or a + // future re-tag action) the art would accumulate a second copy and + // bloat every track by the size of the cover. + // + // BOTH types are removed, not just `CoverFront`: vendor-tagged files + // commonly store the front cover as `Other`, and clearing only + // `CoverFront` would leave that one in place and produce exactly the + // duplicate this guard exists to prevent. + tag.remove_picture_type(PictureType::CoverFront); + tag.remove_picture_type(PictureType::Other); + // `unchecked` skips lofty's format sniffing: the bytes come from the + // album's `image.large`, which Qobuz serves as JPEG, and the mime is + // declared rather than inferred. + tag.push_picture( + Picture::unchecked(bytes.to_vec()) + .pic_type(PictureType::CoverFront) + .mime_type(MimeType::Jpeg) + .build(), + ); + } + } + + tagged_file + .save_to_path(path, WriteOptions::default()) + .map_err(|e| LibraryError::Metadata(format!("Failed to write downloaded file tags: {e}"))) +} + +#[cfg(test)] +mod editor_write_target_tests { + use super::*; + + /// A FLAC that arrives carrying an ID3v2 tag — an ordinary shape, and the + /// one that exposed a silent data-loss bug in the metadata editor. + fn flac_with_leading_id3v2() -> Vec { + let mut frame = b"TIT2".to_vec(); + let text = b"\x03OldTitle"; // 0x03 = UTF-8 + frame.extend_from_slice(&(text.len() as u32).to_be_bytes()); + frame.extend_from_slice(&[0x00, 0x00]); + frame.extend_from_slice(text); + + let size = frame.len() as u32; + let syncsafe = [ + ((size >> 21) & 0x7F) as u8, + ((size >> 14) & 0x7F) as u8, + ((size >> 7) & 0x7F) as u8, + (size & 0x7F) as u8, + ]; + + let mut out = b"ID3".to_vec(); + out.extend_from_slice(&[0x04, 0x00, 0x00]); + out.extend_from_slice(&syncsafe); + out.extend_from_slice(&frame); + + // A minimal but valid FLAC: the marker plus one final STREAMINFO block. + out.extend_from_slice(b"fLaC"); + out.extend_from_slice(&[0x80, 0x00, 0x00, 0x22]); + let mut info = vec![0u8; 34]; + info[0..2].copy_from_slice(&4096u16.to_be_bytes()); + info[2..4].copy_from_slice(&4096u16.to_be_bytes()); + let sample_rate: u32 = 44_100; + info[10] = (sample_rate >> 12) as u8; + info[11] = ((sample_rate >> 4) & 0xFF) as u8; + info[12] = (((sample_rate & 0x0F) as u8) << 4) | (1 << 1); + info[13] = 0xF0; + out.extend_from_slice(&info); + out + } + + /// The metadata editor must write into the tag type the container can + /// actually SAVE, not merely into the first tag the file happens to carry. + /// + /// This is the regression for a silent data-loss bug: the write target used + /// to fall back to `first_tag_mut()`, which for this file is the ID3v2 tag. + /// lofty refuses to write ID3v2 to FLAC and skips it WITHOUT erroring, so + /// every edited field was discarded while the call returned `Ok(())` — the + /// user pressed Save, got a success toast, and the file never changed. + /// + /// Verified to fail against the old target selection before being accepted. + #[test] + fn editing_a_flac_that_carries_id3v2_actually_writes() { + use lofty::prelude::*; + + let tmp = tempfile::tempdir().unwrap(); + let file = tmp.path().join("legacy.flac"); + std::fs::write(&file, flac_with_leading_id3v2()).unwrap(); + + // The fixture must genuinely present the trap, or the test proves nothing. + let before = lofty::read_from_path(&file).expect("readable FLAC fixture"); + assert!( + before.tag(lofty::tag::TagType::Id3v2).is_some(), + "fixture must carry ID3v2" + ); + assert!( + before.tag(lofty::tag::TagType::VorbisComments).is_none(), + "fixture must not already have Vorbis comments" + ); + + let album = AlbumTagWrite { + album_title: "Edited Album".to_string(), + album_artist: "Edited Artist".to_string(), + year: Some(1999), + genre: Some("Jazz".to_string()), + catalog_number: None, + }; + let tracks = vec![TrackTagWrite { + file_path: file.to_string_lossy().to_string(), + title: "Edited Title".to_string(), + track_number: Some(4), + disc_number: Some(2), + }]; + + write_album_tags_to_files(&album, &tracks, |_, _| {}).expect("the edit must succeed"); + + let after = lofty::read_from_path(&file).expect("re-read"); + let vorbis = after + .tag(lofty::tag::TagType::VorbisComments) + .expect("the edit must land in VORBIS COMMENTS — the type FLAC can write"); + + assert_eq!(vorbis.title().as_deref(), Some("Edited Title")); + assert_eq!(vorbis.album().as_deref(), Some("Edited Album")); + assert_eq!(vorbis.track(), Some(4)); + assert_eq!(vorbis.disk(), Some(2)); + assert_eq!(vorbis.genre().as_deref(), Some("Jazz")); + } +} + #[cfg(test)] mod tests { use super::should_rename_artist; diff --git a/crates/qbz-models/src/lib.rs b/crates/qbz-models/src/lib.rs index 73485e9a4..94754f947 100644 --- a/crates/qbz-models/src/lib.rs +++ b/crates/qbz-models/src/lib.rs @@ -54,6 +54,7 @@ pub use types::{ RadioResponse, ArtistAlbums, ArtistBiography, + Goody, // Discover types DiscoverAlbum, DiscoverAlbumDates, diff --git a/crates/qbz-models/src/purchase_serde.rs b/crates/qbz-models/src/purchase_serde.rs index 310290384..624edd1f4 100644 --- a/crates/qbz-models/src/purchase_serde.rs +++ b/crates/qbz-models/src/purchase_serde.rs @@ -281,23 +281,39 @@ mod tests { } #[test] - fn purchase_track_has_no_version_field() { - // Compile-time + behavioral guard: an incoming `version` key must be - // silently ignored (PurchaseTrack does not declare it). A faithful port - // never renders a subtitle/version on purchased tracks. + fn purchase_track_captures_version_when_present() { + // INVERTED 2026-08-16 (contract §10-C). This test used to be named + // `purchase_track_has_no_version_field` and asserted that an incoming + // `version` key was silently ignored — except it never asserted anything + // about `version` at all, so it passed either way while documenting the + // opposite of the behaviour we now want. A guard that guards nothing and + // misleads the next reader is worse than no guard. + // + // `version` is now captured. The purchases ENDPOINTS do not send it, but + // the album-detail screen builds its tracks from the catalog album, which + // does, and the reference renders `formatTrackTitle(title, version)`. const TRACK: &str = r#"{ "id": 1, - "title": "No Version", + "title": "Song", "version": "Remastered 2024" }"#; let track: PurchaseTrack = - serde_json::from_str(TRACK).expect("track with stray version must deserialize"); + serde_json::from_str(TRACK).expect("track with a version must deserialize"); assert_eq!(track.id, 1); - assert_eq!(track.title, "No Version"); + assert_eq!(track.title, "Song"); + assert_eq!(track.version.as_deref(), Some("Remastered 2024")); // `streamable` defaults TRUE even on the bare track shape. assert!(track.streamable); } + #[test] + fn purchase_track_without_a_version_is_none() { + // The purchases list shape, which genuinely carries no `version`. + let track: PurchaseTrack = + serde_json::from_str(r#"{"id": 2, "title": "Plain"}"#).expect("bare track"); + assert_eq!(track.version, None); + } + #[test] fn radio_response_tolerates_missing_total() { use crate::types::RadioResponse; diff --git a/crates/qbz-models/src/types.rs b/crates/qbz-models/src/types.rs index e877be01f..27fe5866c 100644 --- a/crates/qbz-models/src/types.rs +++ b/crates/qbz-models/src/types.rs @@ -572,8 +572,18 @@ pub struct Album { pub upc: Option, /// Editorial description/review of the album pub description: Option, - /// Album goodies (booklets, liner notes PDFs) - #[serde(default)] + /// Album goodies (booklets, liner notes PDFs, videos). + /// + /// LENIENT on purpose. `/album/get` is parsed with the strict structs — its + /// `Option` fields carry no lenient wrapper, so ONE wrong-typed value + /// fails the entire album parse and the album simply will not open. That + /// risk is concentrated exactly here: on albums nobody owns, `goodies` comes + /// back an empty array, so its populated item shape has never been observed. + /// The vendor's own downloader gates goodie fetching behind purchases, which + /// is the hypothesis for why. We cannot capture a populated one without + /// owning a purchase, so the shape stays UNVERIFIED — and an unverified + /// shape must degrade to `None`, never take the album down with it. + #[serde(default, deserialize_with = "crate::purchase_serde::lenient_option")] pub goodies: Option>, /// Editorial awards (Qobuzissime, Album of the Week, press accolades). #[serde(default)] @@ -609,7 +619,14 @@ pub struct AlbumArtist { pub roles: Option>, } -/// A downloadable extra bundled with an album (e.g. PDF booklet) +/// A downloadable extra bundled with an album (e.g. PDF booklet). +/// +/// Every field defaults, so a missing key never fails an item, and the whole +/// list is behind `lenient_option` on `Album::goodies`, so a surprising list or +/// item shape degrades to "no goodies" rather than to "the album will not open". +/// Read `url`/`name` through [`Goody::best_url`] / [`Goody::display_name`] rather +/// than directly — the populated shape has never been observed and those helpers +/// are where the fallbacks live. #[derive(Debug, Clone, Serialize, Deserialize)] pub struct Goody { #[serde(default)] @@ -621,6 +638,11 @@ pub struct Goody { /// Original (full-size) URL #[serde(default)] pub original_url: String, + /// A third spelling seen in vendor payloads; kept as a fallback source for + /// [`Goody::best_url`] because we cannot confirm which key a real, + /// purchase-gated goodie uses. + #[serde(default)] + pub file_url: Option, /// File format id (e.g. 21 for PDF) #[serde(default)] pub file_format_id: Option, @@ -628,6 +650,39 @@ pub struct Goody { pub description: Option, } +impl Goody { + /// The URL to fetch, trying every spelling we know of. `original_url` wins + /// because it is documented as the full-size asset; `url` is the common + /// shape; `file_url` is the unconfirmed third. Returns `None` when the item + /// carries no usable URL at all, which is the signal to skip it rather than + /// to fail anything. + pub fn best_url(&self) -> Option<&str> { + [ + self.original_url.as_str(), + self.url.as_str(), + self.file_url.as_deref().unwrap_or(""), + ] + .into_iter() + .map(str::trim) + .find(|candidate| !candidate.is_empty()) + } + + /// A human label, falling back through the fields most likely to carry one + /// and finally to the id, so a goodie is never rendered nameless. + pub fn display_name(&self) -> String { + for candidate in [Some(self.name.as_str()), self.description.as_deref()] + .into_iter() + .flatten() + { + let trimmed = candidate.trim(); + if !trimmed.is_empty() { + return trimmed.to_string(); + } + } + format!("Goody {}", self.id) + } +} + #[derive(Debug, Clone, Serialize, Deserialize)] pub struct TracksContainer { pub items: Vec, @@ -1063,9 +1118,18 @@ pub struct PurchaseAlbum { pub tracks: Option>, } -/// A purchased track. NOTE: there is intentionally **no `version` field** — the -/// purchases path never carries the track subtitle/edition (see source-of-truth -/// §4.6). `streamable` defaults TRUE; `downloaded`/`downloaded_format_ids` are +/// A purchased track. +/// +/// It DOES carry a `version` (the track subtitle/edition). It deliberately did +/// not, on the reasoning that the purchases endpoints never send one — which is +/// true of the endpoints and beside the point for the screen: the album-detail +/// view builds its tracks from the CATALOG album (`/album/get`), which does send +/// `version`, and the reference's frontend renders +/// `formatTrackTitle(title, version)`. Dropping the field is what made every +/// purchased track lose its subtitle (issue #360); contract §10-C rules the +/// field back in and `build_purchase_album` maps it across. +/// +/// `streamable` defaults TRUE; `downloaded`/`downloaded_format_ids` are /// server-computed from the local registry; `media_number` is the disc number /// used for disc-grouping. `purchased_at` is unix epoch seconds. #[derive(Debug, Clone, Serialize, Deserialize, Default)] @@ -1074,6 +1138,14 @@ pub struct PurchaseTrack { pub id: u64, #[serde(default)] pub title: String, + /// Track subtitle ("Remastered", "Live at …"). The purchases endpoints do + /// not send it, but the catalog `/album/get` track does, and the detail + /// screen renders `formatTrackTitle(title, version)` — so `build_purchase_album` + /// carries it across. Tauri declared the same field on its frontend type and + /// never mapped it, which is why purchased track titles lost their version + /// (issue #360); this field is what closes that. + #[serde(default, deserialize_with = "crate::purchase_serde::lenient_option")] + pub version: Option, #[serde(default)] pub track_number: u32, #[serde(default, deserialize_with = "crate::purchase_serde::lenient_option")] @@ -1759,3 +1831,209 @@ mod tests { assert_eq!(qobuz_cover_at_px("/home/u/Music/cover.jpg", 600), None); } } + +// ─── §12-4: purchase deserializers, one payload per missing optional field ──── +// +// The whole point of this module is that the purchase path cannot be smoke- +// tested: Qobuz Purchases is not sold in the owner's region, so the only +// populated payloads anyone will ever see belong to end users. Every default +// below is therefore asserted rather than assumed, and the ones that DIFFER +// between two structs with the same field name get their own test, because that +// is the trap most likely to invert a screen's behaviour silently. +#[cfg(test)] +mod purchase_deserializer_tests { + use super::*; + + // ── §2.6: the `streamable` split ───────────────────────────────────────── + + /// A purchases-list track defaults `streamable` to **true**. + #[test] + fn purchase_track_streamable_defaults_true() { + let t: PurchaseTrack = serde_json::from_str(r#"{"id":1,"title":"T"}"#).unwrap(); + assert!( + t.streamable, + "PurchaseTrack.streamable defaults TRUE (serde_true)" + ); + } + + /// A CATALOG track — the one `/album/get` returns, which is what builds the + /// album-detail screen — defaults `streamable` to **false**. + /// + /// These two together are the trap: the detail screen gates click-to-play on + /// `streamable`, so a blanket "streamable defaults true" inverts that whole + /// screen's behaviour, and nobody here can click a purchased album to notice. + #[test] + fn catalog_track_streamable_defaults_false() { + let t: Track = serde_json::from_str(r#"{"id":1,"title":"T"}"#).unwrap(); + assert!( + !t.streamable, + "catalog Track.streamable defaults FALSE — the detail screen depends on it" + ); + } + + /// `downloadable` drives three list behaviours (the hide-unavailable filter, + /// the album click gate, the unavailable marker), so its default is not + /// decoration: getting it wrong ships clickable unavailable albums. + #[test] + fn purchase_album_downloadable_defaults_true() { + let a: PurchaseAlbum = serde_json::from_str(r#"{"id":"x","title":"A"}"#).unwrap(); + assert!(a.downloadable); + assert!(!a.downloaded, "downloaded is local-only, never from the wire"); + assert!(a.tracks.is_none()); + assert!(a.purchased_at.is_none()); + } + + // ── §2.5: leniency is load-bearing, and has a documented limit ──────────── + + /// Wrong-typed optional fields degrade to `None` instead of failing the item. + #[test] + fn wrong_typed_optionals_degrade_to_none() { + let json = r#"{ + "id": 7, "title": "T", + "media_number": "not a number", + "maximum_sampling_rate": {"nope": true}, + "maximum_bit_depth": [], + "album": 12345, + "purchased_at": "yesterday", + "version": [] + }"#; + let t: PurchaseTrack = serde_json::from_str(json).unwrap(); + assert_eq!(t.id, 7); + assert_eq!(t.media_number, None); + assert_eq!(t.maximum_sampling_rate, None); + assert_eq!(t.maximum_bit_depth, None); + assert!(t.album.is_none()); + assert_eq!(t.purchased_at, None); + assert_eq!(t.version, None); + } + + /// A malformed page collapses to an EMPTY page while its sibling still + /// parses. This is why a 401/403 body shaped like JSON and "you own nothing" + /// are indistinguishable — stated here so nobody later mistakes the empty + /// state for proof that the request succeeded. + #[test] + fn a_malformed_page_empties_itself_without_taking_its_sibling() { + let json = r#"{ + "albums": "this is not a page", + "tracks": {"offset":0,"limit":50,"total":1,"items":[{"id":9,"title":"Nine"}]} + }"#; + let r: PurchaseResponse = serde_json::from_str(json).unwrap(); + assert!(r.albums.items.is_empty()); + assert_eq!(r.albums.total, 0); + assert_eq!(r.tracks.items.len(), 1); + assert_eq!(r.tracks.items[0].title, "Nine"); + } + + /// §2.5b-2, measured against a live account: `?type=albums` OMITS the + /// `tracks` key entirely — it is not present-and-zero. A port that models a + /// present-but-empty sibling page is modelling the wrong thing. + #[test] + fn typed_response_with_an_absent_sibling_page_parses() { + let json = r#"{ + "albums": {"offset":0,"limit":500,"total":0,"items":[]}, + "user": {"id": 1, "login": "someone"} + }"#; + let r: PurchaseResponse = serde_json::from_str(json).unwrap(); + assert!(r.albums.items.is_empty()); + assert!(r.tracks.items.is_empty(), "absent sibling defaults, never fails"); + assert_eq!(r.tracks.total, 0); + } + + /// §2.5b-3: the IDS envelope has a DIFFERENT page shape — `{total, items}` + /// with no `offset` and no `limit`. If those scalars were required, the + /// lenient page wrapper would swallow the failure and hand back an empty + /// page, and both tab counters would read 0 forever with nothing logged. + #[test] + fn ids_response_page_shape_preserves_total_without_offset_or_limit() { + let json = r#"{ + "albums": {"total": 42, "items": [1,2,3]}, + "tracks": {"total": 7, "items": []}, + "user": {"id": 1, "login": "someone"} + }"#; + let r: PurchaseIdsResponse = serde_json::from_str(json).unwrap(); + assert_eq!(r.albums.total, 42, "the tab counter comes from here"); + assert_eq!(r.tracks.total, 7); + assert_eq!(r.albums.offset, 0); + assert_eq!(r.albums.limit, 0); + } + + /// The documented LIMIT of the leniency: a non-object top level still fails. + /// Recorded so the empty-list-on-everything claim is not overstated. + /// The documented LIMIT of the leniency — MEASURED 2026-08-16, and it is + /// wider than the contract stated. + /// + /// The contract (§2.5) says "a transport error, a non-JSON body and an + /// invalid top-level shape still FAIL". The first two hold. The third does + /// not, for ARRAYS: serde's derived `Deserialize` accepts a struct from a + /// JSON sequence as well as a map, and since both fields carry + /// `#[serde(default)]` plus a lenient page wrapper, `[]` — and even + /// `[1, 2]`, whose elements are consumed positionally and swallowed — + /// deserialize into a perfectly valid EMPTY response. + /// + /// What genuinely fails is a SCALAR top level: string, number, bool, null. + /// + /// This does not change the contract's conclusion (a JSON-shaped 401/403 + /// body is an object, and it collapses to "you own nothing" exactly as + /// described) but it does widen the silent-empty surface by one shape, and + /// on a feature nobody can smoke-test the exact boundary is worth pinning + /// down rather than approximating. + #[test] + fn only_scalar_top_levels_fail_arrays_collapse_to_empty() { + for ok in ["{}", "[]", "[1,2]"] { + let parsed = serde_json::from_str::(ok) + .unwrap_or_else(|e| panic!("expected {ok} to collapse to empty, got {e}")); + assert!(parsed.albums.items.is_empty()); + assert!(parsed.tracks.items.is_empty()); + } + + for err in ["\"nope\"", "7", "null", "true"] { + assert!( + serde_json::from_str::(err).is_err(), + "a scalar top level must still fail: {err}" + ); + } + } + + // ── §10-C: the version field that closes the latent #360 regression ─────── + + #[test] + fn purchase_track_carries_a_version_when_the_catalog_supplies_one() { + let t: PurchaseTrack = + serde_json::from_str(r#"{"id":1,"title":"Song","version":"Live"}"#).unwrap(); + assert_eq!(t.version.as_deref(), Some("Live")); + } + + // ── §14.3: goodies must never take an album down ────────────────────────── + + /// The shape of a POPULATED goodies list has never been observed — it comes + /// back empty on every album nobody owns. So an unexpected shape has to + /// degrade to "no goodies", not to "this album will not open". `/album/get` + /// is parsed with the strict structs, which is exactly where that would bite. + #[test] + fn a_surprising_goodies_shape_does_not_fail_the_album() { + for weird in [ + r#"{"id":"1","title":"A","goodies":"a string"}"#, + r#"{"id":"1","title":"A","goodies":{"unexpected":"object"}}"#, + r#"{"id":"1","title":"A","goodies":[{"totally":"different"}]}"#, + r#"{"id":"1","title":"A","goodies":7}"#, + ] { + let album: Album = serde_json::from_str(weird) + .unwrap_or_else(|e| panic!("goodies shape took the album down: {weird} → {e}")); + assert_eq!(album.title, "A"); + } + } + + #[test] + fn a_well_formed_goodie_parses_and_reads_defensively() { + let album: Album = serde_json::from_str( + r#"{"id":"1","title":"A","goodies":[ + {"id":5,"name":"Booklet","url":"https://u/b.pdf","original_url":"https://o/b.pdf"} + ]}"#, + ) + .unwrap(); + let goodies = album.goodies.expect("goodies present"); + assert_eq!(goodies.len(), 1); + assert_eq!(goodies[0].best_url(), Some("https://o/b.pdf")); + assert_eq!(goodies[0].display_name(), "Booklet"); + } +} diff --git a/crates/qbz-models/tests/fixtures/purchases/getUserPurchases-noparams.json b/crates/qbz-models/tests/fixtures/purchases/getUserPurchases-noparams.json new file mode 100644 index 000000000..848e76893 --- /dev/null +++ b/crates/qbz-models/tests/fixtures/purchases/getUserPurchases-noparams.json @@ -0,0 +1,18 @@ +{ + "albums": { + "offset": 0, + "limit": 50, + "total": 0, + "items": [] + }, + "tracks": { + "offset": 0, + "limit": 50, + "total": 0, + "items": [] + }, + "user": { + "id": "", + "login": "" + } +} diff --git a/crates/qbz-models/tests/fixtures/purchases/getUserPurchases-type-albums.json b/crates/qbz-models/tests/fixtures/purchases/getUserPurchases-type-albums.json new file mode 100644 index 000000000..65aa531e5 --- /dev/null +++ b/crates/qbz-models/tests/fixtures/purchases/getUserPurchases-type-albums.json @@ -0,0 +1,12 @@ +{ + "albums": { + "offset": 0, + "limit": 500, + "total": 0, + "items": [] + }, + "user": { + "id": "", + "login": "" + } +} diff --git a/crates/qbz-models/tests/fixtures/purchases/getUserPurchases-type-tracks.json b/crates/qbz-models/tests/fixtures/purchases/getUserPurchases-type-tracks.json new file mode 100644 index 000000000..033cf88b7 --- /dev/null +++ b/crates/qbz-models/tests/fixtures/purchases/getUserPurchases-type-tracks.json @@ -0,0 +1,12 @@ +{ + "tracks": { + "offset": 0, + "limit": 500, + "total": 0, + "items": [] + }, + "user": { + "id": "", + "login": "" + } +} diff --git a/crates/qbz-models/tests/fixtures/purchases/getUserPurchasesIds-noparams.json b/crates/qbz-models/tests/fixtures/purchases/getUserPurchasesIds-noparams.json new file mode 100644 index 000000000..f81af4739 --- /dev/null +++ b/crates/qbz-models/tests/fixtures/purchases/getUserPurchasesIds-noparams.json @@ -0,0 +1,14 @@ +{ + "albums": { + "total": 0, + "items": [] + }, + "tracks": { + "total": 0, + "items": [] + }, + "user": { + "id": "", + "login": "" + } +} diff --git a/crates/qbz-models/tests/fixtures/purchases/getUserPurchasesIds-type-albums.json b/crates/qbz-models/tests/fixtures/purchases/getUserPurchasesIds-type-albums.json new file mode 100644 index 000000000..fb30d776d --- /dev/null +++ b/crates/qbz-models/tests/fixtures/purchases/getUserPurchasesIds-type-albums.json @@ -0,0 +1,10 @@ +{ + "albums": { + "total": 0, + "items": [] + }, + "user": { + "id": "", + "login": "" + } +} diff --git a/crates/qbz-models/tests/purchase_wire_fixtures.rs b/crates/qbz-models/tests/purchase_wire_fixtures.rs new file mode 100644 index 000000000..5a2974e16 --- /dev/null +++ b/crates/qbz-models/tests/purchase_wire_fixtures.rs @@ -0,0 +1,142 @@ +//! The purchase envelopes, checked against REAL CAPTURED BYTES. +//! +//! # Why these are not the synthetic tests in `types.rs` +//! +//! The unit tests beside the models assert the shapes we BELIEVE Qobuz sends. +//! These assert the shapes it ACTUALLY sent. The files in `tests/fixtures/purchases/` +//! were captured on 2026-08-15 against the owner's own account, read-only and with +//! their authorisation, and the ids and login were redacted afterwards. They +//! replace the inferred dummy response this project started from — before them, +//! every claim about the purchase envelope was a reading of somebody else's code. +//! +//! They matter more than usual here because nobody on this team can smoke-test +//! Purchases: Qobuz does not sell it in the owner's region, so no screen will +//! ever be exercised locally. A fixture is the only thing in the loop that +//! cannot drift from what the server really does. +//! +//! # What they can and cannot prove +//! +//! Every capture is from an account that owns NOTHING, so `items` is empty in +//! all of them. They pin the ENVELOPE — which keys exist, which are absent, what +//! the pagination scalars look like — and they cannot say anything about a +//! populated item. That gap is real and is why the contract's §11-1 question +//! could not be answered from this account. + +use qbz_models::{PurchaseIdsResponse, PurchaseResponse}; + +fn fixture(name: &str) -> String { + let path = std::path::Path::new(env!("CARGO_MANIFEST_DIR")) + .join("tests/fixtures/purchases") + .join(name); + std::fs::read_to_string(&path) + .unwrap_or_else(|e| panic!("missing capture {}: {e}", path.display())) +} + +/// The unfiltered call returns BOTH pages, and the server's default `limit` is +/// 50 — not 500, which is what the client asks for when it paginates. +#[test] +fn unfiltered_purchases_carry_both_pages_and_a_default_limit_of_50() { + let r: PurchaseResponse = serde_json::from_str(&fixture("getUserPurchases-noparams.json")) + .expect("the captured envelope must deserialize"); + + assert_eq!(r.albums.limit, 50, "the server's default page size"); + assert_eq!(r.tracks.limit, 50); + assert_eq!(r.albums.offset, 0); + assert_eq!(r.albums.total, 0); + assert!(r.albums.items.is_empty()); + assert!(r.tracks.items.is_empty()); +} + +/// THE correction that mattered: `?type=albums` **omits the `tracks` key +/// entirely**. It is not present-and-zeroed, which is what the first reading of +/// the reference concluded from a function that zeroes the sibling total. +/// +/// The observable effect is the same — an empty sibling page — which is why the +/// two-`getUserPurchasesIds`-calls requirement stands either way. But a port that +/// models a present-but-empty sibling is modelling something the server does not +/// send, and it is exactly the assumption that made the reference's album +/// "downloaded" predicate unsatisfiable (contract §11-1). +#[test] +fn a_typed_albums_response_omits_the_tracks_key_entirely() { + let raw = fixture("getUserPurchases-type-albums.json"); + assert!( + !raw.contains("\"tracks\""), + "the capture must NOT contain a tracks key — that absence is the point" + ); + + let r: PurchaseResponse = serde_json::from_str(&raw).expect("must still deserialize"); + assert_eq!(r.albums.limit, 500, "the client's requested page size comes back"); + assert!(r.tracks.items.is_empty(), "the absent sibling defaults, never fails"); + assert_eq!(r.tracks.total, 0); +} + +/// And symmetrically for tracks, so nobody assumes the omission is albums-only. +#[test] +fn a_typed_tracks_response_omits_the_albums_key_entirely() { + let raw = fixture("getUserPurchases-type-tracks.json"); + assert!(!raw.contains("\"albums\"")); + + let r: PurchaseResponse = serde_json::from_str(&raw).expect("must still deserialize"); + assert_eq!(r.tracks.limit, 500); + assert!(r.albums.items.is_empty()); +} + +/// The ids envelope is a DIFFERENT page shape: `{total, items}` with no `offset` +/// and no `limit`. +/// +/// This is the one that would have failed loudly and been misdiagnosed. Both tab +/// counters are read from `.total` here, and the page sits behind a lenient +/// deserializer that swallows a parse failure into an EMPTY page. Had `offset` +/// and `limit` been required fields, both counters would have read 0 forever with +/// nothing logged, and the obvious conclusion would have been "the user owns +/// nothing" rather than "the page did not parse". +#[test] +fn the_ids_envelope_has_no_offset_or_limit_and_still_yields_its_total() { + let raw = fixture("getUserPurchasesIds-noparams.json"); + assert!(!raw.contains("\"offset\""), "the ids page carries no offset"); + assert!(!raw.contains("\"limit\""), "nor a limit"); + + let r: PurchaseIdsResponse = serde_json::from_str(&raw).expect("must deserialize"); + assert_eq!(r.albums.total, 0); + assert_eq!(r.tracks.total, 0); + assert_eq!(r.albums.offset, 0, "defaulted, not sent"); + assert_eq!(r.albums.limit, 0, "defaulted, not sent"); +} + +/// The typed ids call drops the sibling too — so the per-type totals genuinely +/// need one call each, which is what the reference does and what the contract +/// requires. A two-parameter port that fires a single combined call reads one +/// type's total and silently reports 0 for the other. +#[test] +fn a_typed_ids_response_omits_its_sibling_which_is_why_two_calls_are_needed() { + let raw = fixture("getUserPurchasesIds-type-albums.json"); + assert!(!raw.contains("\"tracks\"")); + + let r: PurchaseIdsResponse = serde_json::from_str(&raw).expect("must deserialize"); + assert_eq!(r.albums.total, 0); + assert_eq!(r.tracks.total, 0, "absent sibling defaults to 0, never fails"); +} + +/// Every capture carries a top-level `user` block that no model declares, and +/// none of them may fail because of it. Unknown top-level keys must be ignored — +/// if Qobuz adds another one tomorrow, the purchases screen must not go blank. +#[test] +fn an_unknown_top_level_key_never_fails_a_response() { + for name in [ + "getUserPurchases-noparams.json", + "getUserPurchases-type-albums.json", + "getUserPurchases-type-tracks.json", + ] { + let raw = fixture(name); + assert!(raw.contains("\"user\""), "{name} should carry the user block"); + serde_json::from_str::(&raw) + .unwrap_or_else(|e| panic!("{name} must ignore the unknown `user` key: {e}")); + } + for name in [ + "getUserPurchasesIds-noparams.json", + "getUserPurchasesIds-type-albums.json", + ] { + serde_json::from_str::(&fixture(name)) + .unwrap_or_else(|e| panic!("{name} must ignore the unknown `user` key: {e}")); + } +} diff --git a/crates/qbz-offline-cache/Cargo.toml b/crates/qbz-offline-cache/Cargo.toml index afe546f25..bc9cdc99e 100644 --- a/crates/qbz-offline-cache/Cargo.toml +++ b/crates/qbz-offline-cache/Cargo.toml @@ -36,3 +36,15 @@ log = { workspace = true } [dev-dependencies] tempfile = "3" +# The purchase download harness (`tests/purchase_download_harness.rs`) stands up +# a real local HTTP server and drives the pipeline with real bytes. Integration +# tests do not inherit `[dependencies]`, so the crates it names are repeated here. +tokio = { workspace = true } +lofty = "0.23" +qbz-models = { path = "../qbz-models" } +qbz-library = { path = "../qbz-library" } +# reqwest is built with `rustls-tls-webpki-roots-no-provider`, so building ANY +# client panics with "No provider set" until a process-level CryptoProvider is +# installed. The app does that at startup (`qbz_app::ensure_crypto_provider`); +# a test binary has no startup, so the harness installs it itself. +rustls = { version = "0.23", features = ["aws_lc_rs"] } diff --git a/crates/qbz-offline-cache/src/metadata.rs b/crates/qbz-offline-cache/src/metadata.rs index 0a4584b94..9c53a874d 100644 --- a/crates/qbz-offline-cache/src/metadata.rs +++ b/crates/qbz-offline-cache/src/metadata.rs @@ -254,9 +254,26 @@ pub fn sanitize_filename(name: &str) -> String { // Trim dashes and whitespace from ends sanitized = sanitized.trim_matches('-').trim().to_string(); - // Limit length to 200 chars (leaving room for extension and path) + // Limit length to 200 BYTES (leaving room for extension and path). + // + // The cap is deliberately in bytes, not chars: it is what the shipped + // implementation used, and these strings become on-disk path segments that + // the library-to-registry join matches by EXACT string equality + // (`database.rs`, the `qobuz_purchase` source stamp). Recomputing the cap in + // chars would silently relocate every already-downloaded non-ASCII album. + // + // `String::truncate` panics when the index is not a char boundary, and that + // is reachable here rather than theoretical: the mapping above keeps every + // Unicode ALPHANUMERIC (`c.is_alphanumeric()` is Unicode-aware), so accented + // Latin, Greek, Cyrillic and CJK titles all survive as multibyte characters. + // A 200-byte cut lands mid-character for any such title long enough to hit + // it. Walk back to the nearest boundary instead. if sanitized.len() > 200 { - sanitized.truncate(200); + let end = (0..=200) + .rev() + .find(|&i| sanitized.is_char_boundary(i)) + .unwrap_or(0); + sanitized.truncate(end); sanitized = sanitized.trim_matches('-').trim().to_string(); } diff --git a/crates/qbz-offline-cache/src/purchases_service.rs b/crates/qbz-offline-cache/src/purchases_service.rs index bda56b46c..52d2b9086 100644 --- a/crates/qbz-offline-cache/src/purchases_service.rs +++ b/crates/qbz-offline-cache/src/purchases_service.rs @@ -30,9 +30,10 @@ use std::collections::{HashMap, HashSet}; use std::path::PathBuf; -use qbz_library::LibraryDatabase; +use qbz_library::{write_purchase_tags, LibraryDatabase, PurchaseTagWrite}; use qbz_models::{ Album, PurchaseAlbum, PurchaseFormatOption, PurchaseResponse, PurchaseTrack, SearchResultsPage, + Track, }; use qbz_qobuz::QobuzClient; use qbz_qobuz::Result as QobuzResult; @@ -223,13 +224,45 @@ pub fn synth_formats(album: &Album) -> Vec { /// * `downloaded = downloaded_ids.contains(track.id)`; /// * `downloaded_format_ids = format_map.get(track.id).cloned().unwrap_or_default()`. /// -/// Per album (the all-mode / by-type path where albums and tracks are sibling -/// pages): collect the ids of `response.tracks.items` whose -/// `track.album.id == album.id`; then +/// Per album: **DELIBERATE DIVERGENCE from the reference, contract §11-1, ruled +/// by the owner on 2026-08-16.** +/// +/// The reference collected the ids of `response.tracks.items` whose +/// `track.album.id == album.id` and set /// `album.downloaded = !ids.is_empty() && all ids ∈ downloaded_ids`. -/// An album with NO matching tracks in this response → `downloaded = false` -/// (the empty-set rule; partial-page albums may flip to not-downloaded — this -/// is the documented page-mode gotcha and is replicated verbatim). +/// That predicate is UNSATISFIABLE on the screen that uses it: the Albums tab +/// loads `getUserPurchases?type=albums`, and that response omits the `tracks` +/// key entirely (measured against a live account, §2.5b). With no sibling +/// tracks page the id set is always empty, so the empty-set rule pins +/// `downloaded` to `false` — forever, and silently, because the field is +/// optional behind a lenient deserializer. The visible consequence in the +/// reference is that no album card can ever show as downloaded and the +/// "Hide downloaded" filter can never hide anything. The guide-dog user would +/// have had no way to notice a filter that simply does nothing. +/// +/// So the album rule reads the LOCAL REGISTRY instead, which is the only party +/// that actually knows: `album_counts` maps a purchase album id to its count of +/// DISTINCT downloaded tracks (`get_downloaded_purchase_album_counts`), and an +/// album is downloaded when that count covers its `tracks_count`. +/// +/// Two properties worth keeping in mind: +/// * it is format-AGNOSTIC, matching the reference's list-level semantics (the +/// detail screen is the format-scoped one); +/// * an absent or zero `tracks_count` yields `false`. `PurchaseAlbum.tracks_count` +/// is an `Option`, and without it there is no denominator — "every track is +/// downloaded" is unprovable, so the answer is no. That is the safe direction +/// (it under-claims rather than showing a green mark for an album that may be +/// half on disk) and it preserves the reference's empty-set rule. +/// +/// **Status, stated so this comment does not describe behaviour that does not +/// exist yet:** as of 2026-08-16 this function has NO production caller. The Qt +/// controller that will call it on every list path — so that browsing and +/// searching cannot disagree about the same album — is not written. The shipping +/// Slint controller does NOT use this function; it computes album state itself +/// with its own copy of the old nested-tracks predicate +/// (`crates/qbz/src/purchases.rs`, `enrich_albums`), and therefore still shows +/// the unsatisfiable behaviour described above. Until the Qt side lands the two +/// frontends will disagree, and that is expected rather than a bug to chase. /// /// `downloaded_ids`/`format_map` are keyed by `i64` (registry track ids); track /// ids are `u64` and compared via `track.id as i64`, exactly as the source. @@ -237,6 +270,7 @@ pub fn apply_download_flags( response: &mut PurchaseResponse, downloaded_ids: &HashSet, format_map: &HashMap>, + album_counts: &HashMap, ) { for track in &mut response.tracks.items { let tid = track.id as i64; @@ -245,24 +279,23 @@ pub fn apply_download_flags( } for album in &mut response.albums.items { - let album_track_ids: Vec = response - .tracks - .items - .iter() - .filter(|track| { - track - .album - .as_ref() - .map(|album_ref| album_ref.id == album.id) - .unwrap_or(false) - }) - .map(|track| track.id as i64) - .collect(); + album.downloaded = album_downloaded_from_registry(&album.id, album.tracks_count, album_counts); + } +} - album.downloaded = !album_track_ids.is_empty() - && album_track_ids - .iter() - .all(|track_id| downloaded_ids.contains(track_id)); +/// The shared "is this purchased album fully downloaded" predicate (§11-1). +/// Pure, so both the list annotation and the detail builder can agree, and so it +/// is unit-testable without a database. +pub fn album_downloaded_from_registry( + album_id: &str, + tracks_count: Option, + album_counts: &HashMap, +) -> bool { + match tracks_count { + Some(expected) if expected > 0 => { + album_counts.get(album_id).copied().unwrap_or(0) >= expected + } + _ => false, } } @@ -275,9 +308,14 @@ pub fn apply_download_flags( /// /// Mapping rules (verbatim from command #5): /// * the nested track list comes from `album.tracks.items` mapped to -/// `PurchaseTrack` — the `version`/subtitle is DROPPED (purchases have no -/// `version` field, §4.6); `performer` defaults when the catalog track has -/// none; per-track `purchased_at` is the album-level meta value; +/// `PurchaseTrack`; `performer` defaults when the catalog track has none; +/// per-track `purchased_at` is the album-level meta value; +/// * the `version`/subtitle IS carried across. This is a deliberate divergence +/// from the reference: Tauri's frontend type declares `version` and its detail +/// view renders `formatTrackTitle(title, version)`, but the mapping here never +/// populated it, so every purchased track lost its subtitle (issue #360). The +/// catalog track does carry it (`qbz_models::Track::version`), so the fix is a +/// field copy. Contract §10-C rules this in. /// * `downloadable = purchase_meta.map(|m| m.downloadable).unwrap_or(true)` /// (defaults TRUE when the album is not found in the purchases listing); /// * `purchased_at = purchase_meta.and_then(|m| m.purchased_at)`; @@ -310,6 +348,7 @@ pub fn build_purchase_album( .map(|track| PurchaseTrack { id: track.id, title: track.title.clone(), + version: track.version.clone(), track_number: track.track_number, media_number: track.media_number, duration: track.duration, @@ -508,10 +547,16 @@ fn write_track_file( Ok(target.to_string_lossy().to_string()) } +/// PUBLIC so the download harness (`tests/purchase_download_harness.rs`) can +/// drive the write + registry tail with real bytes without standing up a +/// `QobuzClient`. That harness is the only thing that executes this code before +/// a user does — Purchases cannot be smoke-tested here — so reachability from a +/// test is worth the wider surface. #[allow(clippy::too_many_arguments)] -fn write_and_register_track( +pub fn write_and_register_track( db: &LibraryDatabase, track_id: u64, + album_id: Option<&str>, requested_format_id: u32, data: &[u8], artist_name: &str, @@ -536,11 +581,18 @@ fn write_and_register_track( )?; // Addendum B.1/B.2: registry write AFTER the file is on disk, with the - // REQUESTED format_id (album_id None for single-track downloads). A registry - // failure here returns Err while the file stays on disk (orphaned). + // REQUESTED format_id. A registry failure here returns Err while the file + // stays on disk (orphaned) — the reference does the same and does not roll back. + // + // `album_id` is folded into THIS write rather than left to a follow-up call. + // The reference wrote `None` here and then backfilled the album id from the + // frontend (`markTrackDownloaded(...).catch(() => {})`, both in the album loop + // and in the single-track path). Collapsing the two writes reaches the same end + // state with one statement and no window in which the column is null — which + // matters now that the column has a reader (the album-downloaded rule below). db.mark_purchase_downloaded( track_id as i64, - None, + album_id, &file_path, requested_format_id as i64, ) @@ -549,6 +601,309 @@ fn write_and_register_track( Ok(file_path) } +// ─── Scope expansion §14: tags, covers, goodies ────────────────────────────── +// +// Everything in this block is ADDITIVE over the reference, which wrote no tags, +// no cover and no goodies. Nothing here changes a request or the entitlement +// boundary, and every part degrades to reference behaviour when it fails: the +// audio file is the deliverable, and it is already on disk and registered before +// any of this runs. + +/// Album-level facts a track download needs in order to tag itself, gathered +/// once by the caller before the album loop rather than re-derived per track. +/// +/// `cover_jpeg` is carried as bytes because it is embedded into EVERY track of +/// the album; fetching it per track would issue one CDN request per file for a +/// value that never changes. +#[derive(Debug, Clone, Default)] +pub struct PurchaseAlbumContext { + pub album_artist: String, + pub year: Option, + pub genre: Option, + pub label: Option, + pub cover_jpeg: Option>, +} + +impl PurchaseAlbumContext { + /// Build the context from the catalog album the detail screen already holds. + /// The cover is fetched separately (it is I/O) and attached with + /// [`Self::with_cover`]. + pub fn from_album(album: &Album) -> Self { + // The V2 nested `dates.original` wins over the flat + // `release_date_original` when present — the model says so on the field + // itself, and on a V2-shaped album the flat field is absent, which would + // silently drop the DATE tag from every track of that release. + let date = album + .dates + .as_ref() + .and_then(|d| d.original.as_deref()) + .or(album.release_date_original.as_deref()); + + Self { + album_artist: album.artist.name.clone(), + year: parse_release_year(date), + genre: album.genre.as_ref().map(|g| g.name.clone()), + label: album.label.as_ref().map(|l| l.name.clone()), + cover_jpeg: None, + } + } + + pub fn with_cover(mut self, cover_jpeg: Option>) -> Self { + self.cover_jpeg = cover_jpeg; + self + } +} + +/// Pull the year out of a Qobuz `release_date_original` (`"2019-05-31"`). +/// Anything that does not start with four digits yields `None` rather than a +/// wrong year — a wrong DATE tag is worse than an absent one. +pub fn parse_release_year(date: Option<&str>) -> Option { + let date = date?; + let head: String = date.chars().take(4).collect(); + if head.len() == 4 && head.chars().all(|c| c.is_ascii_digit()) { + head.parse().ok() + } else { + None + } +} + +/// The largest asset this will pull into memory. Covers are ~100 KB and booklets +/// a few MB; the ceiling exists so a hostile or wrong `Content-Length` cannot +/// decide our allocation. +const MAX_ASSET_BYTES: usize = 32 * 1024 * 1024; + +/// How long a single asset may take, end to end. +const ASSET_TIMEOUT_SECS: u64 = 60; + +/// Fetch an image or extra asset over HTTP, returning `None` on any failure. +/// +/// Deliberately infallible-by-return: every caller treats a missing cover or +/// goodie as "skip it", never as a download failure. +/// +/// **This does NOT reuse `QobuzClient::download_audio`, and the difference is +/// deliberate.** That function is tuned for one job — a multi-minute hi-res +/// track from the Qobuz CDN — and three of its properties are wrong here: +/// * it sizes its buffer with `Vec::with_capacity(content_length)`, i.e. from +/// a number the remote chose. For a track that is a trusted CDN; for a +/// goodie it is an arbitrary URL out of an album payload whose item shape has +/// never been observed. A bogus length would attempt the allocation +/// immediately, and an allocation failure ABORTS the process — on a machine +/// this project's own build rules describe as hard-freezing under memory +/// pressure; +/// * it sets no total timeout, because a large track legitimately exceeds any +/// fixed budget. A cover that never finishes would hang the album loop with +/// nothing to cancel it; +/// * it logs every fetch as "Downloading audio", which is simply wrong for a +/// PDF and makes the logs lie during exactly the investigation that needs +/// them. +/// +/// So assets get their own client: a hard size ceiling, a total timeout, and a +/// capacity hint that is `min(content_length, ceiling)`. +pub async fn fetch_asset_bytes(url: &str) -> Option> { + use std::time::Duration; + + let url = url.trim(); + if url.is_empty() { + return None; + } + + let client = match reqwest::Client::builder() + .connect_timeout(Duration::from_secs(10)) + .timeout(Duration::from_secs(ASSET_TIMEOUT_SECS)) + .build() + { + Ok(client) => client, + Err(e) => { + log::warn!("[Purchases] could not build the asset client: {e}"); + return None; + } + }; + + let response = match client.get(url).header("User-Agent", "Mozilla/5.0").send().await { + Ok(response) => response, + Err(e) => { + log::warn!("[Purchases] asset fetch failed ({url}): {e}"); + return None; + } + }; + + if !response.status().is_success() { + log::warn!("[Purchases] asset fetch got HTTP {} ({url})", response.status()); + return None; + } + + // Refuse oversized assets on the ANNOUNCED length before reading a byte, and + // then again on the actual bytes, because the announcement is not binding. + if let Some(len) = response.content_length() { + if len as usize > MAX_ASSET_BYTES { + log::warn!("[Purchases] asset is {len} bytes, over the {MAX_ASSET_BYTES} cap ({url})"); + return None; + } + } + + let bytes = match response.bytes().await { + Ok(bytes) => bytes, + Err(e) => { + log::warn!("[Purchases] asset body failed ({url}): {e}"); + return None; + } + }; + + if bytes.is_empty() { + log::warn!("[Purchases] asset fetch returned 0 bytes: {url}"); + return None; + } + if bytes.len() > MAX_ASSET_BYTES { + log::warn!( + "[Purchases] asset body was {} bytes despite its headers, over the cap ({url})", + bytes.len() + ); + return None; + } + + Some(bytes.to_vec()) +} + +/// Write `cover.jpg` / `back.jpg` beside the album's tracks (§14.2). +/// +/// `album/get`'s `image` object carries exactly `small`, `thumbnail`, `large` +/// and `back` — measured 2026-08-15; there is no `mega`, matching the earlier +/// finding that Qobuz album art tops out around 600 px in practice. So `large` +/// is the best available and `back` is a bonus nothing else in QBZ uses. +/// +/// Best-effort by contract: a write failure is logged and swallowed. +pub fn write_album_cover_files( + album_dir: &std::path::Path, + cover_jpeg: Option<&[u8]>, + back_jpeg: Option<&[u8]>, +) { + for (bytes, name) in [(cover_jpeg, "cover.jpg"), (back_jpeg, "back.jpg")] { + let Some(bytes) = bytes.filter(|b| !b.is_empty()) else { + continue; + }; + let path = album_dir.join(name); + if let Err(e) = std::fs::write(&path, bytes) { + log::warn!("[Purchases] failed to write {}: {e}", path.display()); + } + } +} + +/// Tag a file that has just been downloaded (§14.1). Never fails the download — +/// it returns nothing and logs on error, so a caller cannot accidentally +/// propagate it. +pub fn tag_downloaded_file(file_path: &str, track: &Track, ctx: &PurchaseAlbumContext) { + let meta = PurchaseTagWrite { + title: track.title.clone(), + version: track.version.clone(), + // The track's OWN performer, straight from the API. This is the whole + // reason purchases do not reuse the editor's writer, whose artist rule + // reads the file — and a just-downloaded file has nothing to read. + artist: track + .performer + .as_ref() + .map(|a| a.name.clone()) + .unwrap_or_else(|| ctx.album_artist.clone()), + // `"Singles"` matches what the PATH builder uses for a track with no + // album (`download_purchase_track`). Defaulting to an empty string here + // instead would file the track in a folder called `Singles` while + // stamping `ALBUM=` on it — the folder and the tag disagreeing about the + // same track. + album_title: track + .album + .as_ref() + .map(|a| a.title.clone()) + .unwrap_or_else(|| "Singles".to_string()), + album_artist: ctx.album_artist.clone(), + track_number: Some(track.track_number), + disc_number: track.media_number, + year: ctx.year, + genre: ctx.genre.clone(), + label: ctx.label.clone(), + isrc: track.isrc.clone(), + composer: track.composer.as_ref().map(|c| c.name.clone()), + copyright: track.copyright.clone(), + }; + + if let Err(e) = write_purchase_tags(file_path, &meta, ctx.cover_jpeg.as_deref()) { + log::warn!("[Purchases] failed to tag {file_path}: {e}"); + } +} + +/// Download one album goodie (booklet PDF, video, …) into the album folder +/// (§14.3). +/// +/// The item shape is UNVERIFIED — goodies come back as an empty array on albums +/// nobody owns, and we cannot capture a populated one without owning a purchase. +/// So this reads the URL and name through the defensive accessors and gives up +/// quietly on anything it cannot make sense of. Goodies are counted separately +/// from track progress (a goodie is not a track) and a goodie failure never +/// fails the album. +pub async fn download_goodie( + goody: &qbz_models::Goody, + album_dir: &std::path::Path, +) -> Option { + let url = goody.best_url()?; + + // Create the folder BEFORE spending the download. Doing it afterwards throws + // away however many megabytes were just fetched if the directory cannot be + // made. + if let Err(e) = std::fs::create_dir_all(album_dir) { + log::warn!("[Purchases] failed to create the goodie folder: {e}"); + return None; + } + + let path = goodie_target_path(album_dir, &goody.display_name(), url); + let bytes = fetch_asset_bytes(url).await?; + + match std::fs::write(&path, &bytes) { + Ok(()) => Some(path.to_string_lossy().to_string()), + Err(e) => { + log::warn!("[Purchases] failed to write goodie {}: {e}", path.display()); + None + } + } +} + +/// Pick a safe, non-colliding on-disk name for a goodie. Pure, so the naming +/// rules are testable without a download. +/// +/// Three hazards, all handled here: +/// * **the extension must be read from the PATH, not the whole URL.** Query and +/// fragment are stripped FIRST. Doing it the other way round — splitting on +/// the last `.` and then removing the query — turns a signed +/// `…/booklet.pdf?sig=a.b3f` into a file called `Booklet.b3f`; +/// * **a leading dot would hide the file.** `sanitize_filename` keeps ASCII +/// `.`, so a goodie named `.notes` would land as a dotfile the user never +/// sees. (It also maps `/` and `\` to `-`, which is what makes escaping the +/// album folder impossible — that part needs no help here.) +/// * **two goodies can sanitize to the same name.** Rather than let the second +/// silently overwrite the first, later ones get a ` (2)`, ` (3)` suffix. +fn goodie_target_path(album_dir: &std::path::Path, display_name: &str, url: &str) -> PathBuf { + let path_part = url.split(['?', '#']).next().unwrap_or(url); + let ext = path_part + .rsplit('/') + .next() + .and_then(|segment| segment.rsplit_once('.')) + .map(|(_, ext)| ext) + // Booklets are the only goodie kind ever observed, and a wrong extension + // is recoverable while a missing file is not. + .filter(|ext| !ext.is_empty() && ext.len() <= 5 && ext.chars().all(char::is_alphanumeric)) + .unwrap_or("pdf"); + + let mut stem = sanitize_filename(display_name); + if stem.starts_with('.') { + stem.insert(0, '_'); + } + + let mut candidate = album_dir.join(format!("{stem}.{ext}")); + let mut n = 2; + while candidate.exists() && n < 100 { + candidate = album_dir.join(format!("{stem} ({n}).{ext}")); + n += 1; + } + candidate +} + /// The single canonical single-track download primitive (Slice 5). /// /// Ported from `v2_download_purchase_track_impl` (`legacy_compat.rs:2651-2702`) @@ -581,12 +936,18 @@ pub async fn download_purchase_track( client: &QobuzClient, db: &LibraryDatabase, track_id: u64, + album_id: Option<&str>, format_id: u32, destination: &str, quality_dir: &str, + ctx: Option<&PurchaseAlbumContext>, ) -> Result { + // UNSIGNED on the purchase path (contract §11-6): the vendor's own desktop + // client sends `/track/get` without a signature, and so did the reference. + // The entitlement proof is the signature on `getFileUrl` below, which is + // untouched. let track = client - .get_track(track_id) + .get_track_for_purchase(track_id) .await .map_err(|e| format!("Failed to fetch track {}: {}", track_id, e))?; let stream = client @@ -607,9 +968,19 @@ pub async fn download_purchase_track( .map(|album| album.title.clone()) .unwrap_or_else(|| "Singles".to_string()); - write_and_register_track( + // Fall back to the album the TRACK itself reports. `album_id` is what makes + // an album's downloaded state answerable at all (§11-1), and the reference + // only ever populated it from the album loop — so a user who downloaded a + // release one track at a time produced rows the rule can never attribute, + // and their album stayed un-downloaded forever with no way to notice. The + // `/track/get` payload fetched two lines above already carries the id, so + // there is no reason for any path to write NULL. + let resolved_album_id = album_id.or_else(|| track.album.as_ref().map(|a| a.id.as_str())); + + let file_path = write_and_register_track( db, track_id, + resolved_album_id, format_id, &data, &artist_name, @@ -620,27 +991,51 @@ pub async fn download_purchase_track( stream.format_id, &stream.mime_type, destination, - ) + )?; + + // §14.1, additive: tag AFTER the file is on disk and registered, so the + // deliverable is already durable and a tagging failure can only cost tags. + // `None` reproduces reference behaviour exactly (no tags written at all), + // which is what the legacy Slint call site passes. + if let Some(ctx) = ctx { + tag_downloaded_file(&file_path, &track, ctx); + } + + Ok(file_path) } /// Download-ONLY variant of the single-track primitive: identical CDN fetch + /// `.part`→rename pipeline as `download_purchase_track`, but it does **NOT** /// write the `downloaded_purchases` registry. Returns the final on-disk path. /// -/// This exists for the ALBUM loop (`qbz-slint::execute_album_download`), which -/// matches Svelte `executeAlbumDownload` (`purchaseDownloadStore.ts:130-147`): -/// the album loop marks the track `'complete'` on download success FIRST, then -/// performs the registry write as a SEPARATE best-effort step that SWALLOWS -/// failure (`await markTrackDownloaded(...).catch(() => {})`). So a registry-write -/// failure during an album download leaves the track `'complete'` (file on disk, -/// just unregistered) — UNLIKE the single-track path, where a registry error -/// propagates and the track shows `'failed'` (§B.1). +/// **This function's original rationale was WRONG, and it is kept only because +/// `qbz-slint` calls it.** New code must not use it. Verified against the +/// reference on 2026-08-16: /// -/// The caller is responsible for the best-effort registry write afterwards -/// (`db.mark_purchase_downloaded(track_id, Some(album_id), &file_path, -/// format_id)`), ignoring its error. Addendum B.2 (extension from RESPONSE, -/// registry/qualityDir from REQUESTED), B.3 (silent overwrite), and B.5 -/// (restrictions ignored) all hold identically to `download_purchase_track`. +/// The comment here used to claim that Svelte's `executeAlbumDownload` marks a +/// track `'complete'` first and then writes the registry as a best-effort step +/// that swallows failure, and therefore that an album-path registry failure +/// leaves the track `'complete'` but unregistered. That misreads the reference. +/// `executeAlbumDownload` awaits `downloadTrack`, which invokes +/// `v2_purchases_download_track` — and THAT command writes the registry itself +/// and propagates the error with `?` (`legacy_compat.rs:3019-3021`). So a +/// registry failure throws out of `downloadTrack`, is caught by the loop's +/// `catch`, and the track is marked **`'failed'`**, exactly as on the +/// single-track path. The `markTrackDownloaded(...).catch(() => {})` that follows +/// (`purchaseDownloadStore.ts:147`) is a SECOND, backfill-only write whose sole +/// effect is filling the `album_id` column; its swallowed failure costs that +/// column and nothing else. +/// +/// Consequence: this "write the file, register separately" split has no basis in +/// the reference, and the Slint controller's use of it is the one silent +/// divergence in the purchases port — it leaves files on disk with no registry +/// row while reporting success. The Qt controller uses +/// `download_purchase_track` with `Some(album_id)` instead, which registers +/// inside the same call and propagates. +/// +/// Addendum B.2 (extension from RESPONSE, registry/qualityDir from REQUESTED), +/// B.3 (silent overwrite), and B.5 (restrictions ignored) hold identically to +/// `download_purchase_track`. pub async fn download_purchase_track_file_only( client: &QobuzClient, track_id: u64, @@ -648,8 +1043,12 @@ pub async fn download_purchase_track_file_only( destination: &str, quality_dir: &str, ) -> Result { + // UNSIGNED on the purchase path (contract §11-6): the vendor's own desktop + // client sends `/track/get` without a signature, and so did the reference. + // The entitlement proof is the signature on `getFileUrl` below, which is + // untouched. let track = client - .get_track(track_id) + .get_track_for_purchase(track_id) .await .map_err(|e| format!("Failed to fetch track {}: {}", track_id, e))?; let stream = client @@ -878,6 +1277,21 @@ mod tests { } } + /// An album that declares how many tracks it has — the denominator the + /// registry-backed downloaded rule needs (§11-1). + fn album_with_tracks(id: &str, tracks_count: u32) -> PurchaseAlbum { + PurchaseAlbum { + id: id.to_string(), + tracks_count: Some(tracks_count), + ..Default::default() + } + } + + /// Registry counts: purchase album id → DISTINCT downloaded track count. + fn reg(pairs: &[(&str, u32)]) -> HashMap { + pairs.iter().map(|(k, v)| (k.to_string(), *v)).collect() + } + fn track_for_album(id: u64, album_id: &str) -> PurchaseTrack { PurchaseTrack { id, @@ -903,7 +1317,7 @@ mod tests { let mut format_map: HashMap> = HashMap::new(); format_map.insert(10, vec![27, 6]); - apply_download_flags(&mut resp, &downloaded, &format_map); + apply_download_flags(&mut resp, &downloaded, &format_map, &HashMap::new()); assert!(resp.tracks.items[0].downloaded); assert_eq!(resp.tracks.items[0].downloaded_format_ids, vec![27, 6]); @@ -912,48 +1326,72 @@ mod tests { assert!(resp.tracks.items[1].downloaded_format_ids.is_empty()); } + // The three album tests below were rewritten on 2026-08-16. They used to + // assert the reference's nested-tracks predicate (every track whose + // `album.id` matches must be in `downloaded_ids`). That predicate was + // replaced under contract §11-1 because the screen that uses it never + // receives a tracks page, so it could only ever answer `false`. The INTENT + // of each test is preserved — complete, partial, unknown — but the source of + // truth is now the local registry. + #[test] - fn apply_flags_album_downloaded_when_all_nested_track_ids_present() { - // Every track whose album.id == "a1" is in dlIds → album downloaded. + fn apply_flags_album_downloaded_when_the_registry_covers_it() { let mut resp = response( - vec![album_id("a1")], + vec![album_with_tracks("a1", 2)], vec![track_for_album(10, "a1"), track_for_album(20, "a1")], ); - apply_download_flags(&mut resp, &dl_ids(&[10, 20]), &HashMap::new()); + apply_download_flags( + &mut resp, + &dl_ids(&[10, 20]), + &HashMap::new(), + ®(&[("a1", 2)]), + ); assert!(resp.albums.items[0].downloaded); } #[test] fn apply_flags_album_not_downloaded_when_partially_owned() { - // One of the two album tracks missing from dlIds → album NOT downloaded. let mut resp = response( - vec![album_id("a1")], + vec![album_with_tracks("a1", 2)], vec![track_for_album(10, "a1"), track_for_album(20, "a1")], ); - apply_download_flags(&mut resp, &dl_ids(&[10]), &HashMap::new()); + apply_download_flags( + &mut resp, + &dl_ids(&[10]), + &HashMap::new(), + ®(&[("a1", 1)]), + ); assert!(!resp.albums.items[0].downloaded); } #[test] - fn apply_flags_album_with_no_matching_tracks_is_not_downloaded() { - // No tracks reference this album (empty set rule) → false, never panic. - let mut resp = response(vec![album_id("a1")], vec![track_for_album(10, "other")]); - apply_download_flags(&mut resp, &dl_ids(&[10]), &HashMap::new()); + fn apply_flags_album_absent_from_the_registry_is_not_downloaded() { + // Nothing of this album has been downloaded → false, never panic. + let mut resp = response( + vec![album_with_tracks("a1", 2)], + vec![track_for_album(10, "other")], + ); + apply_download_flags( + &mut resp, + &dl_ids(&[10]), + &HashMap::new(), + ®(&[("other", 1)]), + ); assert!(!resp.albums.items[0].downloaded); } #[test] fn apply_flags_frontend_overrides_stale_backend_downloaded() { - // Backend wrongly set downloaded=true; frontend recomputes to false. + // Backend wrongly set downloaded=true; the annotation recomputes to false. let mut track = track_for_album(10, "a1"); track.downloaded = true; track.downloaded_format_ids = vec![99]; - let mut backend_true_album = album_id("a1"); + let mut backend_true_album = album_with_tracks("a1", 1); backend_true_album.downloaded = true; let mut resp = response(vec![backend_true_album], vec![track]); - // dlIds empty → both must be overridden to false / cleared. - apply_download_flags(&mut resp, &dl_ids(&[]), &HashMap::new()); + // Nothing downloaded → both must be overridden to false / cleared. + apply_download_flags(&mut resp, &dl_ids(&[]), &HashMap::new(), &HashMap::new()); assert!(!resp.tracks.items[0].downloaded); assert!(resp.tracks.items[0].downloaded_format_ids.is_empty()); assert!(!resp.albums.items[0].downloaded); @@ -1152,6 +1590,7 @@ mod tests { let path = write_and_register_track( &db, /*track_id*/ 4242, + /*album_id*/ Some("alb-4242"), /*requested_format_id*/ 27, &data, "Miles Davis", @@ -1190,6 +1629,7 @@ mod tests { let path = write_and_register_track( &db, 1, + /*album_id*/ None, /*requested*/ 7, b"x", "A", @@ -1216,14 +1656,16 @@ mod tests { let dest = tmp.path().join("dl"); let first = write_and_register_track( - &db, 9, 6, b"old", "A", "Alb", "", 2, "Song", 6, "audio/flac", dest.to_str().unwrap(), + &db, 9, None, 6, b"old", "A", "Alb", "", 2, "Song", 6, "audio/flac", + dest.to_str().unwrap(), ) .unwrap(); assert_eq!(std::fs::read(&first).unwrap(), b"old"); // Second write to the SAME deterministic path with new bytes overwrites. let second = write_and_register_track( - &db, 9, 6, b"new", "A", "Alb", "", 2, "Song", 6, "audio/flac", dest.to_str().unwrap(), + &db, 9, None, 6, b"new", "A", "Alb", "", 2, "Song", 6, "audio/flac", + dest.to_str().unwrap(), ) .unwrap(); assert_eq!(first, second, "same deterministic target path"); @@ -1256,7 +1698,8 @@ mod tests { let dest = tmp.path().join("downloads"); let res = write_and_register_track( - &db, 77, 6, b"bytes", "A", "Alb", "", 1, "Song", 6, "audio/flac", dest.to_str().unwrap(), + &db, 77, None, 6, b"bytes", "A", "Alb", "", 1, "Song", 6, "audio/flac", + dest.to_str().unwrap(), ); // Registry INSERT failed (no such table) → Err. @@ -1267,3 +1710,335 @@ mod tests { assert!(!orphan.with_extension("flac.part").exists(), "`.part` already renamed away"); } } + +// ─── §12-3: the path builder and sanitizer, against the inputs that break them ── +#[cfg(test)] +mod path_contract_tests { + use super::*; + use crate::metadata::sanitize_filename; + + /// §4.5. `char::is_alphanumeric` is Unicode-aware, so accented Latin, Greek, + /// Cyrillic and CJK all SURVIVE sanitization — only non-ASCII + /// NON-alphanumerics (em dash, curly quotes, ™, ♭) become `-`. + /// + /// This matters far beyond aesthetics: the library-to-registry join is exact + /// `file_path` string equality, so an implementer who "knows" that non-ASCII + /// becomes dashes writes different paths for most non-English releases and + /// the gold purchase badge silently stops working for them. + #[test] + fn sanitize_keeps_unicode_alphanumerics() { + assert_eq!(sanitize_filename("Björk"), "Björk"); + assert_eq!(sanitize_filename("Café Tacvba"), "Café Tacvba"); + assert_eq!(sanitize_filename("Ελλάδα"), "Ελλάδα"); + assert_eq!(sanitize_filename("Кино"), "Кино"); + assert_eq!(sanitize_filename("日本語"), "日本語"); + } + + /// The other half of the same rule: non-ASCII punctuation does NOT survive. + #[test] + fn sanitize_replaces_non_ascii_punctuation() { + // Em dash and curly apostrophe are non-ASCII and non-alphanumeric. + assert_eq!(sanitize_filename("A—B"), "A-B"); + assert_eq!(sanitize_filename("Don’t"), "Don-t"); + // Consecutive dashes collapse, ends are trimmed. + assert_eq!(sanitize_filename("™™Hits™™"), "Hits"); + } + + /// Filesystem-invalid ASCII still becomes `-`; ASCII brackets do NOT. + /// + /// The bracket half is load-bearing and was mis-documented: the quality + /// folder suffix is literally `[FLAC][24-bit,96kHz]`, and `[` is ASCII, so + /// it is kept. "Fixing" the brackets would relocate every downloaded album. + #[test] + fn sanitize_keeps_ascii_brackets_but_replaces_path_separators() { + assert_eq!(sanitize_filename("[FLAC][24-bit,96kHz]"), "[FLAC][24-bit,96kHz]"); + assert_eq!(sanitize_filename("AC/DC"), "AC-DC"); + assert_eq!(sanitize_filename("a:b*c?d\"eg|h"), "a-b-c-d-e-f-g-h"); + } + + /// §4.5 / §10-E: the 200 cap is in BYTES and `String::truncate` panics on a + /// non-char-boundary index — reachable exactly BECAUSE multibyte characters + /// survive the mapping above. + /// + /// Which characters actually crash is worth stating precisely, because the + /// obvious guess is wrong. 200 is even, so a run of TWO-byte characters + /// (`é`, `Ω`, Cyrillic) lands on a boundary and truncates fine. The crash + /// belongs to the THREE-byte class — CJK and most of the BMP beyond Latin + /// and Cyrillic — where 200 % 3 == 2 always lands mid-character. Measured, + /// not reasoned: `'本'` and `'日'` panic under the old code, the rest do not. + /// The two-byte cases are kept here anyway as regression cover for the + /// boundary walk itself. + #[test] + fn sanitize_does_not_panic_truncating_multibyte_titles() { + // '本' and '日' are the ones that panicked before the fix. + for ch in ['é', 'Ω', '本', 'ы', '日'] { + let long: String = std::iter::repeat(ch).take(400).collect(); + let out = sanitize_filename(&long); + assert!(out.len() <= 200, "cap is bytes: {} bytes", out.len()); + assert!(!out.is_empty()); + // Still valid UTF-8 with no replacement damage: every char is the input char. + assert!(out.chars().all(|c| c == ch)); + } + } + + /// A byte cap must not silently become a char cap: pure ASCII is unchanged, + /// so already-downloaded libraries keep their paths. + #[test] + fn sanitize_ascii_cap_is_exactly_200() { + let long = "a".repeat(400); + assert_eq!(sanitize_filename(&long).len(), 200); + } + + #[test] + fn sanitize_empty_result_falls_back() { + assert_eq!(sanitize_filename("—"), "track"); + assert_eq!(sanitize_filename(""), "track"); + } + + /// §4.4: an EMPTY quality dir drops the album folder's trailing space too. + /// Copying the formula literally yields `"Album "` and a different directory. + #[test] + fn target_path_empty_quality_dir_has_no_trailing_space() { + let with_quality = target_path("/d", "A", "Album", "[FLAC][16-bit,44.1kHz]", 1, "T", "flac"); + let without = target_path("/d", "A", "Album", "", 1, "T", "flac"); + + assert_eq!( + without, + std::path::PathBuf::from("/d/A/Album/01 - T.flac"), + "no trailing space when the quality dir is empty" + ); + assert_eq!( + with_quality, + std::path::PathBuf::from("/d/A/Album [FLAC][16-bit,44.1kHz]/01 - T.flac") + ); + } + + /// §4.4, recorded as a KNOWN reference behaviour rather than a bug to fix: + /// `media_number` never reaches the path, so two discs of one release collide + /// on track number and `fs::rename` overwrites silently. Disc 2 track 1 + /// lands on top of disc 1 track 1. + /// + /// This test exists to make the collision explicit and to fail loudly if + /// someone changes the scheme without deciding to — the scheme is what the + /// reference shipped and what existing users' folders look like. + #[test] + fn target_path_multi_disc_collides_on_track_number() { + let disc1 = target_path("/d", "A", "Album", "", 1, "Opening", "flac"); + let disc2 = target_path("/d", "A", "Album", "", 1, "Opening", "flac"); + assert_eq!( + disc1, disc2, + "documented reference behaviour: no media_number in the path" + ); + + // Different titles still collide on the NUMBER prefix only, not the name, + // so the practical collision needs the same title as well. + let other_title = target_path("/d", "A", "Album", "", 1, "Reprise", "flac"); + assert_ne!(disc1, other_title); + } + + /// Non-ASCII flows through the whole builder, not just the sanitizer. + #[test] + fn target_path_preserves_unicode_end_to_end() { + let p = target_path( + "/music", + "Café Tacvba", + "Ré", + "[FLAC][24-bit,96kHz]", + 7, + "El Ciclón", + "flac", + ); + assert_eq!( + p, + std::path::PathBuf::from("/music/Café Tacvba/Ré [FLAC][24-bit,96kHz]/07 - El Ciclón.flac") + ); + } + + /// §4.4: the extension comes from the SERVED format, the folder suffix from + /// the REQUESTED label — they are allowed to disagree, and do when Qobuz + /// downgrades a request. + #[test] + fn extension_follows_the_served_format_not_the_request() { + assert_eq!(purchase_extension(5, "audio/flac"), "mp3"); + assert_eq!(purchase_extension(27, "audio/mpeg"), "mp3"); + assert_eq!(purchase_extension(27, "audio/flac"), "flac"); + assert_eq!(purchase_extension(6, ""), "flac"); + } +} + +// ─── §11-1: the album-downloaded rule, which no UI test could reach ──────────── +#[cfg(test)] +mod album_downloaded_tests { + use super::*; + + fn counts(pairs: &[(&str, u32)]) -> HashMap { + pairs.iter().map(|(k, v)| (k.to_string(), *v)).collect() + } + + #[test] + fn album_is_downloaded_when_the_registry_covers_every_track() { + let c = counts(&[("alb1", 12)]); + assert!(album_downloaded_from_registry("alb1", Some(12), &c)); + // Over-count (same track in two formats is already de-duplicated by the + // query, but a stale row must not un-mark the album). + assert!(album_downloaded_from_registry("alb1", Some(10), &c)); + } + + #[test] + fn partial_downloads_do_not_mark_the_album() { + let c = counts(&[("alb1", 11)]); + assert!(!album_downloaded_from_registry("alb1", Some(12), &c)); + } + + #[test] + fn unknown_album_is_not_downloaded() { + let c = counts(&[("alb1", 12)]); + assert!(!album_downloaded_from_registry("other", Some(12), &c)); + } + + /// No denominator means the answer is NO. Showing a green mark for an album + /// that might be half on disk is the worse failure. + #[test] + fn absent_or_zero_track_count_is_never_downloaded() { + let c = counts(&[("alb1", 99)]); + assert!(!album_downloaded_from_registry("alb1", None, &c)); + assert!(!album_downloaded_from_registry("alb1", Some(0), &c)); + } + + /// The regression this rule exists to prevent: with the reference's + /// predicate, an albums-only response (which carries NO tracks page) pinned + /// every album to `false`. Here the registry answers instead. + #[test] + fn albums_only_response_can_still_report_downloaded() { + let mut response = PurchaseResponse { + albums: SearchResultsPage { + items: vec![PurchaseAlbum { + id: "alb1".to_string(), + tracks_count: Some(2), + ..Default::default() + }], + total: 1, + offset: 0, + limit: 500, + }, + // `?type=albums` omits the tracks key entirely — this is that shape. + tracks: SearchResultsPage { + items: vec![], + total: 0, + offset: 0, + limit: 0, + }, + }; + + apply_download_flags( + &mut response, + &HashSet::new(), + &HashMap::new(), + &counts(&[("alb1", 2)]), + ); + + assert!( + response.albums.items[0].downloaded, + "the registry knows this album is complete even with no tracks page" + ); + } + + #[test] + fn track_flags_are_still_format_scoped_and_registry_driven() { + let mut response = PurchaseResponse { + albums: SearchResultsPage { items: vec![], total: 0, offset: 0, limit: 0 }, + tracks: SearchResultsPage { + items: vec![ + PurchaseTrack { id: 10, ..Default::default() }, + PurchaseTrack { id: 11, ..Default::default() }, + ], + total: 2, + offset: 0, + limit: 500, + }, + }; + + let downloaded: HashSet = [10i64].into_iter().collect(); + let mut formats: HashMap> = HashMap::new(); + formats.insert(10, vec![6, 27]); + + apply_download_flags(&mut response, &downloaded, &formats, &HashMap::new()); + + assert!(response.tracks.items[0].downloaded); + assert_eq!(response.tracks.items[0].downloaded_format_ids, vec![6, 27]); + assert!(!response.tracks.items[1].downloaded); + assert!(response.tracks.items[1].downloaded_format_ids.is_empty()); + } +} + +// ─── §14: the scope-expansion helpers ────────────────────────────────────────── +#[cfg(test)] +mod scope_expansion_tests { + use super::*; + + #[test] + fn release_year_parses_only_a_real_leading_year() { + assert_eq!(parse_release_year(Some("2019-05-31")), Some(2019)); + assert_eq!(parse_release_year(Some("1971")), Some(1971)); + // A wrong year is worse than no year. + assert_eq!(parse_release_year(Some("n/a")), None); + assert_eq!(parse_release_year(Some("")), None); + assert_eq!(parse_release_year(Some("19-05-31")), None); + assert_eq!(parse_release_year(None), None); + } + + #[test] + fn goody_url_falls_back_through_every_known_spelling() { + use qbz_models::Goody; + + let original = Goody { + original_url: "https://o/a.pdf".into(), + url: "https://u/a.pdf".into(), + ..goody_blank() + }; + assert_eq!(original.best_url(), Some("https://o/a.pdf")); + + let only_url = Goody { url: "https://u/a.pdf".into(), ..goody_blank() }; + assert_eq!(only_url.best_url(), Some("https://u/a.pdf")); + + let only_file_url = Goody { + file_url: Some("https://f/a.pdf".into()), + ..goody_blank() + }; + assert_eq!(only_file_url.best_url(), Some("https://f/a.pdf")); + + // Nothing usable → skip the item, never fail the album. + assert_eq!(goody_blank().best_url(), None); + let whitespace = Goody { url: " ".into(), ..goody_blank() }; + assert_eq!(whitespace.best_url(), None); + } + + #[test] + fn goody_display_name_is_never_empty() { + use qbz_models::Goody; + + let named = Goody { name: "Booklet".into(), ..goody_blank() }; + assert_eq!(named.display_name(), "Booklet"); + + let described = Goody { + description: Some("Liner notes".into()), + ..goody_blank() + }; + assert_eq!(described.display_name(), "Liner notes"); + + let bare = Goody { id: 42, ..goody_blank() }; + assert_eq!(bare.display_name(), "Goody 42"); + } + + fn goody_blank() -> qbz_models::Goody { + qbz_models::Goody { + id: 0, + name: String::new(), + url: String::new(), + original_url: String::new(), + file_url: None, + file_format_id: None, + description: None, + } + } +} diff --git a/crates/qbz-offline-cache/tests/purchase_download_harness.rs b/crates/qbz-offline-cache/tests/purchase_download_harness.rs new file mode 100644 index 000000000..0899c0929 --- /dev/null +++ b/crates/qbz-offline-cache/tests/purchase_download_harness.rs @@ -0,0 +1,562 @@ +//! The Purchases download harness (contract §12, owner-approved 2026-08-16). +//! +//! # Why this file exists +//! +//! Nobody on this team can smoke-test Purchases. Qobuz does not sell it in the +//! owner's region, so their account returns `{total: 0, items: []}` forever and +//! the populated screens — and every line of the download pipeline — will first +//! run on a stranger's machine. The contract's §12 established that the Tauri +//! build shipped no harness either: its "mock backend at :8787" turned out to be +//! unserved `fetch` stubs behind `import.meta.env.DEV`, with no server, no +//! fixtures and no npm script. So there is nothing to port, and this is new work. +//! It is also the only thing that turns the feature from ship-blind into +//! ship-testable. +//! +//! # What it covers, and what it does NOT +//! +//! It exercises the DOWNLOAD half for real: a live local HTTP server stands in +//! for the Qobuz CDN, real bytes travel over a real socket through the real +//! client, and the result lands in a real temp directory and a real SQLite +//! database. Path building, the `.part`→rename dance, the registry write, tag +//! writing, cover files and goodies are genuinely executed rather than inspected. +//! +//! It does NOT cover the LIST half. `endpoints::BASE_URL` is a `const`, so +//! `getUserPurchases` cannot be pointed at a local server without changing +//! production code — and that is not worth doing, because the request shape is +//! already pinned by the signature and URL tests in `qbz-qobuz`, and the response +//! shapes by the deserializer tests in `qbz-models` against payloads captured +//! from a live account. Said plainly so nobody reads this file as proof of more +//! than it proves. +//! +//! The server is hand-rolled on `std::net::TcpListener` — about forty lines — +//! rather than pulling a mock-HTTP crate in for one test. + +use std::collections::HashMap; +use std::io::{BufRead, BufReader, Write}; +use std::net::{TcpListener, TcpStream}; +use std::sync::mpsc; +use std::thread; + +use qbz_library::LibraryDatabase; +use qbz_models::{Goody, Track}; +use qbz_offline_cache::purchases_service as svc; + +// ─── The fixture server ─────────────────────────────────────────────────────── + +/// A tiny HTTP/1.1 server serving a fixed route table. Also records the paths it +/// was actually asked for, so a test can assert what was REQUESTED and not +/// merely what came back. +struct FixtureServer { + base: String, + hits: mpsc::Receiver, +} + +impl FixtureServer { + fn start(routes: HashMap)>) -> Self { + let listener = TcpListener::bind("127.0.0.1:0").expect("bind an ephemeral port"); + let port = listener.local_addr().unwrap().port(); + let (tx, hits) = mpsc::channel(); + + thread::spawn(move || { + for stream in listener.incoming() { + let Ok(stream) = stream else { continue }; + let routes = routes.clone(); + let tx = tx.clone(); + thread::spawn(move || serve_one(stream, &routes, &tx)); + } + }); + + FixtureServer { + base: format!("http://127.0.0.1:{port}"), + hits, + } + } + + fn url(&self, path: &str) -> String { + format!("{}{}", self.base, path) + } + + fn requested_paths(&self) -> Vec { + self.hits.try_iter().collect() + } +} + +fn serve_one( + mut stream: TcpStream, + routes: &HashMap)>, + tx: &mpsc::Sender, +) { + let mut reader = BufReader::new(stream.try_clone().expect("clone the stream")); + let mut request_line = String::new(); + if reader.read_line(&mut request_line).is_err() { + return; + } + // Drain the headers so the client sees a well-formed exchange. + loop { + let mut line = String::new(); + match reader.read_line(&mut line) { + Ok(0) => break, + Ok(_) if line.trim().is_empty() => break, + Ok(_) => {} + Err(_) => break, + } + } + + let path = request_line + .split_whitespace() + .nth(1) + .unwrap_or("/") + .to_string(); + let _ = tx.send(path.clone()); + + // Route on the path WITHOUT its query string, so a test can register + // "/cdn/track.flac" and still be hit by a signed, query-carrying URL. + let bare = path.split('?').next().unwrap_or(&path); + let (status, body) = routes + .get(bare) + .cloned() + .unwrap_or((404, b"not found".to_vec())); + + let reason = if status == 200 { "OK" } else { "Error" }; + let head = format!( + "HTTP/1.1 {status} {reason}\r\nContent-Length: {}\r\nConnection: close\r\n\r\n", + body.len() + ); + let _ = stream.write_all(head.as_bytes()); + let _ = stream.write_all(&body); + let _ = stream.flush(); +} + +// ─── Fixtures ───────────────────────────────────────────────────────────────── + +/// A minimal but VALID FLAC stream: the `fLaC` marker plus a single, final +/// STREAMINFO block. It carries no audio frames, which is fine — the pipeline +/// only ever writes tags to it and never decodes it — and it is enough for lofty +/// to identify the container and create a Vorbis comment block. +fn minimal_flac() -> Vec { + let mut out = b"fLaC".to_vec(); + // Metadata block header: last-block flag (0x80) | type 0 (STREAMINFO), then + // a 24-bit big-endian length of 34. + out.extend_from_slice(&[0x80, 0x00, 0x00, 0x22]); + + let mut info = vec![0u8; 34]; + info[0..2].copy_from_slice(&4096u16.to_be_bytes()); // min block size + info[2..4].copy_from_slice(&4096u16.to_be_bytes()); // max block size + // Sample rate 44100 (20 bits), then channels-1 (3 bits) and bits-per-sample-1 + // (5 bits), packed across bytes 10..13 as the FLAC spec lays STREAMINFO out. + let sample_rate: u32 = 44_100; + info[10] = (sample_rate >> 12) as u8; + info[11] = ((sample_rate >> 4) & 0xFF) as u8; + info[12] = (((sample_rate & 0x0F) as u8) << 4) | (1 << 1); // ch-1 = 1 → stereo + info[13] = 0xF0; // bits-per-sample-1 = 15 → 16-bit + out.extend_from_slice(&info); + out +} + +/// A FLAC that arrives carrying an **ID3v2** tag in front of the `fLaC` marker. +/// +/// This is not a contrived shape — plenty of tooling stamps ID3v2 onto FLAC, and +/// lofty reads it — and it is the shape that exposes the worst failure mode this +/// pipeline had. See `a_flac_carrying_id3v2_is_still_tagged`. +fn flac_with_leading_id3v2() -> Vec { + // ID3v2.4 header: "ID3", version 4.0, no flags, then a SYNCSAFE size (7 bits + // per byte) covering the frames that follow. + let mut frame = b"TIT2".to_vec(); + let text = b"\x03OldTitle"; // 0x03 = UTF-8 encoding byte + frame.extend_from_slice(&(text.len() as u32).to_be_bytes()); + frame.extend_from_slice(&[0x00, 0x00]); // frame flags + frame.extend_from_slice(text); + + let size = frame.len() as u32; + let syncsafe = [ + ((size >> 21) & 0x7F) as u8, + ((size >> 14) & 0x7F) as u8, + ((size >> 7) & 0x7F) as u8, + (size & 0x7F) as u8, + ]; + + let mut out = b"ID3".to_vec(); + out.extend_from_slice(&[0x04, 0x00, 0x00]); + out.extend_from_slice(&syncsafe); + out.extend_from_slice(&frame); + out.extend_from_slice(&minimal_flac()); + out +} + +/// Opaque bytes for the asset routes, where only the round trip matters. +fn fake_jpeg() -> Vec { + let mut out = vec![0xFF, 0xD8, 0xFF, 0xE0]; // a JPEG SOI, so it is at least plausible + out.extend_from_slice(b"qbz-test-cover-bytes"); + out +} + +/// A catalog track built through the real deserializer, so the harness exercises +/// the same mapping the download path does rather than a hand-made struct. +fn catalog_track(json: &str) -> Track { + serde_json::from_str(json).expect("catalog track fixture deserializes") +} + +fn goody(name: &str, url: String) -> Goody { + Goody { + id: 5, + name: name.to_string(), + url, + original_url: String::new(), + file_url: None, + file_format_id: Some(21), + description: None, + } +} + +fn open_db(dir: &std::path::Path) -> LibraryDatabase { + LibraryDatabase::open(&dir.join("library.db")).expect("open a temp library db") +} + +/// Install the process-level rustls `CryptoProvider`, once. +/// +/// Not optional and not TLS-specific in effect: reqwest is built with +/// `rustls-tls-webpki-roots-no-provider`, so **constructing any client panics +/// with "No provider set"** until a provider is installed — even for a plain +/// `http://` URL, because the TLS backend is initialised at builder time rather +/// than at connect time. The application does this at startup +/// (`qbz_app::ensure_crypto_provider`); a test binary has no startup, so it does +/// it here. Worth knowing beyond this file: any future tool or test that reaches +/// for `download_audio` without an app around it will hit the same panic. +fn ensure_crypto_provider() { + use std::sync::Once; + static INIT: Once = Once::new(); + INIT.call_once(|| { + let _ = rustls::crypto::aws_lc_rs::default_provider().install_default(); + }); +} + +fn rt() -> tokio::runtime::Runtime { + ensure_crypto_provider(); + tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("build a test runtime") +} + +// ─── The tests ──────────────────────────────────────────────────────────────── + +/// The asset fetcher moves real bytes over a real socket, unchanged. +#[test] +fn asset_bytes_round_trip_over_a_real_socket() { + let cover = fake_jpeg(); + let mut routes = HashMap::new(); + routes.insert("/img/cover.jpg".to_string(), (200u16, cover.clone())); + let server = FixtureServer::start(routes); + + let got = rt().block_on(svc::fetch_asset_bytes(&server.url("/img/cover.jpg"))); + assert_eq!(got.as_deref(), Some(cover.as_slice())); + assert_eq!(server.requested_paths(), vec!["/img/cover.jpg".to_string()]); +} + +/// Every failure mode of the asset fetcher degrades to `None`. This is the +/// property the whole scope expansion rests on: a missing cover or goodie must +/// never fail a download that already succeeded. +#[test] +fn asset_failures_never_raise() { + let mut routes = HashMap::new(); + routes.insert("/empty".to_string(), (200u16, Vec::new())); + routes.insert("/boom".to_string(), (500u16, b"server on fire".to_vec())); + let server = FixtureServer::start(routes); + + let runtime = rt(); + assert_eq!(runtime.block_on(svc::fetch_asset_bytes(&server.url("/empty"))), None); + assert_eq!(runtime.block_on(svc::fetch_asset_bytes(&server.url("/boom"))), None); + assert_eq!(runtime.block_on(svc::fetch_asset_bytes(&server.url("/nope"))), None); + assert_eq!(runtime.block_on(svc::fetch_asset_bytes("")), None); + // Nothing is listening on port 1; the connect timeout bounds the attempt. + assert_eq!( + runtime.block_on(svc::fetch_asset_bytes("http://127.0.0.1:1/x")), + None + ); +} + +/// `cover.jpg` / `back.jpg` land beside the tracks; an absent one writes nothing. +#[test] +fn cover_files_are_written_beside_the_tracks() { + let tmp = tempfile::tempdir().unwrap(); + let album_dir = tmp.path().join("Artist").join("Album"); + std::fs::create_dir_all(&album_dir).unwrap(); + + let cover = fake_jpeg(); + svc::write_album_cover_files(&album_dir, Some(&cover), None); + assert_eq!(std::fs::read(album_dir.join("cover.jpg")).unwrap(), cover); + assert!( + !album_dir.join("back.jpg").exists(), + "an absent back cover must write nothing" + ); + + let back = b"back-cover-bytes".to_vec(); + svc::write_album_cover_files(&album_dir, Some(&cover), Some(&back)); + assert_eq!(std::fs::read(album_dir.join("back.jpg")).unwrap(), back); + + // Empty slices count as absent, not as 0-byte files. + let empty_dir = tmp.path().join("empty"); + std::fs::create_dir_all(&empty_dir).unwrap(); + svc::write_album_cover_files(&empty_dir, Some(&[]), Some(&[])); + assert!(!empty_dir.join("cover.jpg").exists()); + assert!(!empty_dir.join("back.jpg").exists()); +} + +/// A goodie is fetched and written into the album folder under a sanitized name. +#[test] +fn a_goodie_is_downloaded_into_the_album_folder() { + let pdf = b"%PDF-1.4 fake booklet".to_vec(); + let mut routes = HashMap::new(); + routes.insert("/goodies/booklet.pdf".to_string(), (200u16, pdf.clone())); + let server = FixtureServer::start(routes); + + let tmp = tempfile::tempdir().unwrap(); + let album_dir = tmp.path().join("Album"); + + let path = rt() + .block_on(svc::download_goodie( + &goody("Digital Booklet", server.url("/goodies/booklet.pdf")), + &album_dir, + )) + .expect("the goodie downloads"); + + assert!(path.ends_with("Digital Booklet.pdf"), "got {path}"); + assert_eq!(std::fs::read(&path).unwrap(), pdf); +} + +/// A goodie name cannot escape the album folder. The name is the least +/// trustworthy field on the least verified struct in the feature — its populated +/// shape has never been observed — so containment is asserted, not assumed. +#[test] +fn a_goodie_name_cannot_escape_the_album_folder() { + let mut routes = HashMap::new(); + routes.insert("/g/x.pdf".to_string(), (200u16, b"x".to_vec())); + let server = FixtureServer::start(routes); + + let tmp = tempfile::tempdir().unwrap(); + let album_dir = tmp.path().join("Album"); + let runtime = rt(); + + for hostile in [ + "../../../../etc/passwd", + "/etc/passwd", + "..\\..\\windows\\system32", + "a/b/c", + ] { + let path = runtime + .block_on(svc::download_goodie( + &goody(hostile, server.url("/g/x.pdf")), + &album_dir, + )) + .expect("still downloads, just somewhere safe"); + assert_eq!( + std::path::Path::new(&path).parent(), + Some(album_dir.as_path()), + "{hostile:?} escaped to {path}" + ); + } +} + +/// A goodie with no usable URL is skipped, never fatal. +#[test] +fn a_goodie_without_a_url_is_skipped() { + let tmp = tempfile::tempdir().unwrap(); + assert_eq!( + rt().block_on(svc::download_goodie(&goody("Nameless", " ".to_string()), tmp.path())), + None + ); +} + +/// THE end-to-end one: real bytes over a real socket land at the contract's +/// deterministic path, register under the REQUESTED format, and carry tags taken +/// from the API payload plus an embedded cover. +#[test] +fn a_downloaded_file_is_written_registered_and_tagged() { + use lofty::prelude::*; + + let audio = minimal_flac(); + let cover = fake_jpeg(); + let mut routes = HashMap::new(); + routes.insert("/cdn/track.flac".to_string(), (200u16, audio.clone())); + let server = FixtureServer::start(routes); + + let tmp = tempfile::tempdir().unwrap(); + let db = open_db(tmp.path()); + let dest = tmp.path().join("Downloads"); + + // Fetch through the real client, exactly as the download path does. + let data = rt() + .block_on(svc::fetch_asset_bytes(&server.url("/cdn/track.flac"))) + .expect("the CDN stand-in serves the audio"); + assert_eq!(data, audio, "bytes survive the socket unchanged"); + + // The write + registry tail, with the album id folded into the one write. + // Requested format 27, SERVED format 6 — they are allowed to disagree. + let file_path = svc::write_and_register_track( + &db, + 4242, + Some("alb-1"), + 27, + &data, + "Café Tacvba", + "Ré", + "[FLAC][24-bit,192kHz]", + 7, + "El Ciclón", + 6, + "audio/flac", + dest.to_str().unwrap(), + ) + .expect("write and register"); + + // §4.4: deterministic path, Unicode survives, the quality suffix keeps its + // ASCII brackets, and the `.part` file is gone. + assert!( + file_path.ends_with("Café Tacvba/Ré [FLAC][24-bit,192kHz]/07 - El Ciclón.flac"), + "unexpected path: {file_path}" + ); + assert!(!std::path::Path::new(&format!("{file_path}.part")).exists()); + + // §3: the registry records the REQUESTED format, and `album_id` is set — + // which is what makes the album-downloaded rule answerable at all. + let formats = db.get_downloaded_purchase_formats().unwrap(); + assert!( + formats.contains(&(4242, 27)), + "registry must hold the REQUESTED format 27, not the served 6: {formats:?}" + ); + let counts = db.get_downloaded_purchase_album_counts().unwrap(); + assert_eq!(counts.get("alb-1"), Some(&1)); + + // §14.1: tags come from the API payload, including the per-track artist, + // and the cover is embedded exactly once. + let track = catalog_track( + r#"{ + "id": 4242, + "title": "El Ciclón", + "version": "Live", + "track_number": 7, + "media_number": 1, + "isrc": "MX1234567890", + "performer": {"id": 1, "name": "Café Tacvba"}, + "album": {"id": "alb-1", "title": "Ré"}, + "copyright": "1994 WEA" + }"#, + ); + let ctx = svc::PurchaseAlbumContext { + album_artist: "Café Tacvba".to_string(), + year: Some(1994), + genre: Some("Rock".to_string()), + label: Some("WEA".to_string()), + cover_jpeg: Some(cover), + }; + svc::tag_downloaded_file(&file_path, &track, &ctx); + + let tagged = lofty::read_from_path(std::path::Path::new(&file_path)).expect("re-read the file"); + let tag = tagged + .primary_tag() + .or_else(|| tagged.first_tag()) + .expect("a tag exists"); + + assert_eq!(tag.title().as_deref(), Some("El Ciclón")); + assert_eq!( + tag.artist().as_deref(), + Some("Café Tacvba"), + "the per-track performer must be written outright — the editor's rename \ + rule reads the file, and a fresh download has nothing to read" + ); + assert_eq!(tag.album().as_deref(), Some("Ré")); + assert_eq!(tag.track(), Some(7)); + assert_eq!(tag.disk(), Some(1)); + assert_eq!( + tag.pictures().len(), + 1, + "exactly one embedded cover, never a second copy" + ); + + // Tagging twice must not accumulate art — the guard that makes a retry safe. + svc::tag_downloaded_file(&file_path, &track, &ctx); + let retagged = lofty::read_from_path(std::path::Path::new(&file_path)).unwrap(); + let tag2 = retagged.primary_tag().or_else(|| retagged.first_tag()).unwrap(); + assert_eq!(tag2.pictures().len(), 1, "re-tagging must not duplicate the cover"); +} + +/// THE regression test for the worst failure this pipeline had, and the reason +/// the harness above was not enough on its own. +/// +/// The tag writer used to pick its target tag as "the primary one, or else the +/// first one I can find". For a FLAC that arrives carrying an ID3v2 tag, the +/// primary (VorbisComments) is absent while the first is that ID3v2 — so every +/// field went into a tag type lofty **refuses to write** for FLAC. And it +/// refuses silently: the save loop skips non-writable tags with `continue` +/// rather than erroring, so the call returned `Ok(())`, nothing was logged, and +/// the file came out with none of its metadata. On a feature nobody here can +/// smoke-test, that is a bug that reaches users untouched. +/// +/// The clean FLAC fixture used everywhere else in this file cannot catch it, +/// because with NO tag at all both accessors return `None` and the correct +/// branch runs by luck. +#[test] +fn a_flac_carrying_id3v2_is_still_tagged() { + use lofty::prelude::*; + + let tmp = tempfile::tempdir().unwrap(); + let file = tmp.path().join("with-id3.flac"); + std::fs::write(&file, flac_with_leading_id3v2()).unwrap(); + + // Sanity: the fixture really does present an ID3v2 tag, and really does NOT + // present the Vorbis comments that FLAC actually writes. Without this the + // test could pass while proving nothing. + let before = lofty::read_from_path(&file).expect("the fixture is a readable FLAC"); + assert!( + before.tag(lofty::tag::TagType::Id3v2).is_some(), + "the fixture must carry an ID3v2 tag, or it does not exercise the bug" + ); + assert!( + before.tag(lofty::tag::TagType::VorbisComments).is_none(), + "the fixture must NOT already have Vorbis comments" + ); + + let track = catalog_track( + r#"{"id":1,"title":"New Title","track_number":3, + "performer":{"id":1,"name":"The Artist"}, + "album":{"id":"a","title":"The Album"}}"#, + ); + let ctx = svc::PurchaseAlbumContext { + album_artist: "The Artist".to_string(), + year: Some(2001), + genre: None, + label: None, + cover_jpeg: None, + }; + svc::tag_downloaded_file(&file.to_string_lossy(), &track, &ctx); + + let after = lofty::read_from_path(&file).expect("re-read"); + let vorbis = after + .tag(lofty::tag::TagType::VorbisComments) + .expect("the tags must land in VORBIS COMMENTS — the type FLAC can actually write"); + + assert_eq!(vorbis.title().as_deref(), Some("New Title")); + assert_eq!(vorbis.artist().as_deref(), Some("The Artist")); + assert_eq!(vorbis.album().as_deref(), Some("The Album")); + assert_eq!(vorbis.track(), Some(3)); +} + +/// A tagging failure must never be able to fail a download: the function returns +/// nothing, so a caller cannot propagate it even by accident. +#[test] +fn tagging_a_missing_or_unreadable_file_is_survivable() { + let tmp = tempfile::tempdir().unwrap(); + let track = catalog_track(r#"{"id":1,"title":"T","track_number":1}"#); + let ctx = svc::PurchaseAlbumContext::default(); + + // Missing file. + svc::tag_downloaded_file(&tmp.path().join("nope.flac").to_string_lossy(), &track, &ctx); + + // Present but not audio. + let junk = tmp.path().join("junk.flac"); + std::fs::write(&junk, b"this is not a flac stream").unwrap(); + svc::tag_downloaded_file(&junk.to_string_lossy(), &track, &ctx); + + // Reaching here at all is the assertion: neither call panicked or returned. + assert!(junk.exists(), "the file is left exactly as it was"); +} diff --git a/crates/qbz-qobuz/src/purchases.rs b/crates/qbz-qobuz/src/purchases.rs index 4b9f725f1..f244b07cd 100644 --- a/crates/qbz-qobuz/src/purchases.rs +++ b/crates/qbz-qobuz/src/purchases.rs @@ -32,11 +32,126 @@ use super::client::QobuzClient; use super::endpoints::{self, paths}; use super::error::{ApiError, Result}; use qbz_models::{ - PurchaseAlbum, PurchaseIdsResponse, PurchaseResponse, PurchaseTrack, SearchResultsPage, - StreamRestriction, StreamUrl, + Album, PurchaseAlbum, PurchaseIdsResponse, PurchaseResponse, PurchaseTrack, SearchResultsPage, + StreamRestriction, StreamUrl, Track, }; +/// What the purchases pagination loop should do after receiving a page. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum PurchasePageStep { + /// Stop; every item has been collected (or the page came back empty). + Stop, + /// Request the next page starting at this offset. + Continue { next_offset: u32 }, +} + +/// The purchases pagination loop's terminating condition, extracted so it can be +/// asserted rather than merely inspected (contract §12-2). +/// +/// Verbatim from the reference (`src-tauri/src/api/client.rs:1694-1729`): `total` +/// is captured only on the FIRST page, and the loop stops when the page came back +/// empty or when `offset + got >= total`. +/// +/// The case worth naming is `total == 0` with a non-empty page: `0 + got >= 0` +/// holds immediately, so the loop stops after ONE page. That is not a +/// "truncation at 500" — the caller then reports that page's actual length as the +/// total. The behaviour is replicated because the reference shipped it and the +/// endpoint is untestable from here; the caller logs a warning so the symptom is +/// diagnosable if it ever bites. +pub fn purchase_page_step(offset: u32, got: u32, total: u32) -> PurchasePageStep { + if got == 0 || offset.saturating_add(got) >= total { + PurchasePageStep::Stop + } else { + PurchasePageStep::Continue { + next_offset: offset + got, + } + } +} + impl QobuzClient { + /// `/album/get` for the PURCHASE path — header-auth, **UNSIGNED**. + /// + /// The general `get_album` signs this call. Signing evidently works (QBZ + /// browses the catalogue with it every day), so this is not a bug being + /// fixed — but on the purchase path it is a variable nobody can test, and it + /// diverges from both implementations known to work there. Neither the + /// reference (`src-tauri/src/api/client.rs:641`) nor Qobuz's own Electron + /// desktop client sign it: in the vendor bundle the request helper takes a + /// SIGN flag, and `album/get`, `track/get` and `purchase/getUserPurchases` + /// all pass it false while `track/getFileUrl` passes it true (contract + /// §2.1b). So the purchase path matches the vendor exactly. + /// + /// Unsigned is NOT anonymous: measured 2026-08-15, `/album/get` with + /// `X-App-Id` alone returns **401**. The user token is required, which is + /// what `authenticated_headers()` supplies. + /// + /// Status IS hard-checked here, unlike the two purchases list endpoints. That + /// asymmetry is the reference's (`client.rs:653-669`) and it is load-bearing: + /// this response is parsed with the STRICT `Album`/`Track` structs, whose + /// optional fields carry no lenient wrapper, so one wrong-typed value fails + /// the whole album rather than degrading to an empty page. + pub async fn get_album_for_purchase(&self, album_id: &str) -> Result { + let url = endpoints::build_url(paths::ALBUM_GET); + let http_response = self + .http()? + .get(&url) + .headers(self.authenticated_headers().await?) + .query(&[("album_id", album_id)]) + .send() + .await?; + let status = http_response.status(); + log::debug!("[Purchases] get_album_for_purchase({album_id}) status={status}"); + + if status == reqwest::StatusCode::NOT_FOUND { + return Err(ApiError::ApiResponse(format!( + "Album {album_id} not found (404)" + ))); + } + if !status.is_success() { + return Err(ApiError::ApiResponse(format!( + "get_album_for_purchase({album_id}) status {status}" + ))); + } + + let response: Value = http_response.json().await?; + Ok(serde_json::from_value(response)?) + } + + /// `/track/get` for the PURCHASE path — header-auth, **UNSIGNED**. + /// + /// Same rationale as `get_album_for_purchase`. This call runs before EVERY + /// purchase download (`legacy_compat.rs:2659` calls `get_track` immediately + /// before `get_track_file_url_by_format`) and supplies the artist, album + /// title and track number that build the on-disk path — and, since the scope + /// expansion, the metadata written into the file's tags. + /// + /// It carries no entitlement. `/track/getFileUrl` is where the right to the + /// file is proved, by its signature, and that call is untouched. + pub async fn get_track_for_purchase(&self, track_id: u64) -> Result { + let url = endpoints::build_url(paths::TRACK_GET); + let http_response = self + .http()? + .get(&url) + .headers(self.authenticated_headers().await?) + .query(&[("track_id", track_id.to_string())]) + .send() + .await?; + let status = http_response.status(); + log::debug!("[Purchases] get_track_for_purchase({track_id}) status={status}"); + + if status == reqwest::StatusCode::NOT_FOUND { + return Err(ApiError::TrackUnavailable(track_id)); + } + if !status.is_success() { + return Err(ApiError::ApiResponse(format!( + "get_track_for_purchase({track_id}) status {status}" + ))); + } + + let response: Value = http_response.json().await?; + Ok(serde_json::from_value(response)?) + } + /// Get one purchases page from Qobuz, optionally constrained by purchase /// type (`"albums"` / `"tracks"`; omitted if `None`). /// @@ -200,6 +315,18 @@ impl QobuzClient { &self, purchase_type: &str, ) -> Result { + // Pre-flight validation (contract §8-D2). The reference validates the + // type BEFORE any I/O (`legacy_compat.rs:2773-2778`); this loop used to + // fetch a page first and only reject inside the match arm, so a bogus + // type issued a live authenticated GET before failing. Same error, no + // request. + if purchase_type != "albums" && purchase_type != "tracks" { + return Err(ApiError::ApiResponse(format!( + "Unsupported purchase type: {}", + purchase_type + ))); + } + let page_limit = 500u32; let mut offset = 0u32; @@ -219,10 +346,10 @@ impl QobuzClient { } let got = page.albums.items.len() as u32; all_albums.extend(page.albums.items); - if got == 0 || offset + got >= total { - break; + match purchase_page_step(offset, got, total) { + PurchasePageStep::Stop => break, + PurchasePageStep::Continue { next_offset } => offset = next_offset, } - offset += got; } "tracks" => { if offset == 0 { @@ -230,29 +357,39 @@ impl QobuzClient { } let got = page.tracks.items.len() as u32; all_tracks.extend(page.tracks.items); - if got == 0 || offset + got >= total { - break; + match purchase_page_step(offset, got, total) { + PurchasePageStep::Stop => break, + PurchasePageStep::Continue { next_offset } => offset = next_offset, } - offset += got; - } - _ => { - return Err(ApiError::ApiResponse(format!( - "Unsupported purchase type: {}", - purchase_type - ))); } + // Unreachable: the type is validated before the first request. + // Kept so the match stays exhaustive without an unreachable!(). + _ => break, } } - let final_total = if total == 0 { - if purchase_type == "albums" { - all_albums.len() as u32 - } else { - all_tracks.len() as u32 - } + // §2.3: a ZERO server total terminates the loop after a single page, + // because `offset + got >= total` holds immediately. The returned total + // then falls back to the accumulated length — which is THAT PAGE's actual + // length, not a 500-item truncation. Replicated verbatim because the + // reference shipped it, but it is worth a log line: a user whose library + // silently stops at one page has no other signal, and without this the + // symptom gets misdiagnosed as "truncated at 500". + let accumulated = if purchase_type == "albums" { + all_albums.len() as u32 } else { - total + all_tracks.len() as u32 }; + if total == 0 && accumulated > 0 { + log::warn!( + "[Purchases] {purchase_type}: server reported total=0 while returning \ + {accumulated} item(s); pagination stopped after the first page. \ + Reporting total={accumulated}. If the real library is larger, this \ + is where the items were lost." + ); + } + + let final_total = if total == 0 { accumulated } else { total }; Ok(PurchaseResponse { albums: SearchResultsPage { @@ -483,3 +620,133 @@ impl QobuzClient { Ok(all_data) } } + +#[cfg(test)] +mod purchase_contract_tests { + use super::*; + use crate::auth::{generate_signature, sign_get_file_url}; + + // ── §12-1: the requests must be byte-identical to the reference ────────── + // + // These assert the SIGNATURE PREIMAGE and the endpoint paths, not just that + // "a signature is produced". The purchase path is the one feature nobody on + // this team can smoke-test — Qobuz Purchases is not sold in the owner's + // region — so inspection is not evidence and these tests are the only thing + // standing between a wrong request and a shipped, invisible failure. + + /// The preimage has NO separators anywhere, and the intent baked into it is + /// `stream` — not `download`, which is what the inferred OpenAPI lists. + /// Both facts come from the implementation that demonstrably worked, and + /// from Qobuz's own desktop client, which signs this call and only this call. + #[test] + fn file_url_signature_preimage_is_exact() { + let track_id = 123_456_u64; + let format_id = 27_u32; + let timestamp = 1_700_000_000_u64; + let secret = "0123456789abcdef0123456789abcdef"; + + // Assembled here by hand, character for character, from the contract: + // trackgetFileUrl + format_id{fid} + intentstream + track_id{tid} + // + {timestamp} + {secret} + let expected_preimage = + format!("trackgetFileUrlformat_id{format_id}intentstreamtrack_id{track_id}{timestamp}{secret}"); + let expected = { + use md5::{Digest, Md5}; + let mut hasher = Md5::new(); + hasher.update(expected_preimage.as_bytes()); + format!("{:x}", hasher.finalize()) + }; + + assert_eq!( + sign_get_file_url(track_id, format_id, timestamp, secret), + expected, + "the getFileUrl signature preimage drifted; every purchase download \ + will fail auth and nobody here can smoke-test it" + ); + } + + /// Guard the two halves of the preimage that a well-meaning edit would + /// "fix": the intent, and the absence of separators. + #[test] + fn file_url_signature_rejects_download_intent_and_separators() { + let (tid, fid, ts, secret) = (99_u64, 6_u32, 1_u64, "s"); + let actual = sign_get_file_url(tid, fid, ts, secret); + + let with_download_intent = generate_signature( + "trackgetFileUrl", + &format!("format_id{fid}intentdownloadtrack_id{tid}"), + ts, + secret, + ); + assert_ne!( + actual, with_download_intent, + "intent must stay `stream`; the OpenAPI's `download` is not what shipped" + ); + + let with_separators = generate_signature( + "trackgetFileUrl", + &format!("format_id{fid}&intentstream&track_id{tid}"), + ts, + secret, + ); + assert_ne!(actual, with_separators, "the preimage carries no separators"); + } + + /// The endpoint constants themselves — a typo here fails silently into an + /// empty purchases list, because the list endpoints never check status. + #[test] + fn purchase_endpoint_paths_are_the_reference_paths() { + assert_eq!(paths::PURCHASE_GET_USER_PURCHASES, "/purchase/getUserPurchases"); + assert_eq!( + paths::PURCHASE_GET_USER_PURCHASES_IDS, + "/purchase/getUserPurchasesIds" + ); + assert_eq!(paths::ALBUM_GET, "/album/get"); + assert_eq!(paths::TRACK_GET, "/track/get"); + } + + // ── §12-2: the pagination loop's terminating condition ─────────────────── + + #[test] + fn pagination_stops_on_an_empty_page() { + assert_eq!(purchase_page_step(0, 0, 1_000), PurchasePageStep::Stop); + // Even mid-walk, and even when the server claims more remain. + assert_eq!(purchase_page_step(500, 0, 1_000), PurchasePageStep::Stop); + } + + #[test] + fn pagination_walks_while_items_remain() { + assert_eq!( + purchase_page_step(0, 500, 1_200), + PurchasePageStep::Continue { next_offset: 500 } + ); + assert_eq!( + purchase_page_step(500, 500, 1_200), + PurchasePageStep::Continue { next_offset: 1_000 } + ); + // Final page: 1000 + 200 >= 1200. + assert_eq!(purchase_page_step(1_000, 200, 1_200), PurchasePageStep::Stop); + } + + /// THE case from §2.3. A zero total stops the walk after one page even + /// though that page was full. The caller then reports the accumulated + /// length, so the symptom is "my library is exactly one page long", not + /// "truncated at 500" — a distinction that has to survive in a test because + /// it cannot be reproduced against a live account here. + #[test] + fn pagination_zero_total_stops_after_one_page_even_when_full() { + assert_eq!(purchase_page_step(0, 500, 0), PurchasePageStep::Stop); + assert_eq!(purchase_page_step(0, 1, 0), PurchasePageStep::Stop); + } + + /// Exact-boundary and overshoot: `>=`, never `>`. + #[test] + fn pagination_boundary_is_inclusive() { + assert_eq!(purchase_page_step(0, 500, 500), PurchasePageStep::Stop); + assert_eq!(purchase_page_step(0, 501, 500), PurchasePageStep::Stop); + assert_eq!( + purchase_page_step(0, 499, 500), + PurchasePageStep::Continue { next_offset: 499 } + ); + } +} diff --git a/crates/qbz/src/purchases.rs b/crates/qbz/src/purchases.rs index 828280ea9..99ff12f02 100644 --- a/crates/qbz/src/purchases.rs +++ b/crates/qbz/src/purchases.rs @@ -1081,13 +1081,20 @@ pub fn start_track_download( }); return; }; + // `None`: this call site is unchanged in behaviour — the primitive + // used to hard-code `None` for the album id. It became a parameter + // so the Qt controller can register the album a track came from. let status = match purchases_service::download_purchase_track( &client, &db, track_id, + None, format_id, &destination, &quality_dir, + // No album context: this legacy path writes no tags, exactly + // as the reference did. + None, ) .await {