From 475f261dc18809a9d31531df56543d24e5770a39 Mon Sep 17 00:00:00 2001 From: vicrodh <103399283+vicrodh@users.noreply.github.com> Date: Sun, 16 Aug 2026 11:53:37 -0600 Subject: [PATCH] feat(purchases): shared backend for the Qt port, with tags, covers and goodies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The frontend-agnostic half of Purchases. Ported against the Tauri reference, which is the only implementation ever validated by a real user; the Slint controller was read for divergences only and never as a source of truth. Context that sets the bar for all of it: Qobuz does not sell Purchases in this account's region, so no populated screen and no download can be exercised here. Anything merely plausible ships unseen. Hence the tests below are the deliverable as much as the code. Behaviour ported or corrected: - Album "downloaded" now comes from the LOCAL REGISTRY. The reference derives it from `album.tracks.items`, but `getUserPurchases?type=albums` carries no nested tracks page (measured against a live account), so that predicate is unsatisfiable: no album card can ever mark as downloaded and "Hide downloaded" can never hide anything. `get_downloaded_purchase_album_counts` answers it instead, giving a reader at last to the `album_id` column and to the index that has always sat on it. - `album_id` is written by every download path. It used to be set only by the album loop, so a release downloaded track by track produced rows the rule could never attribute. The `/track/get` payload each download already fetches carries the id. - `remove_downloaded_purchase` keys on `(track_id, format_id)`, the table's actual primary key; erasing every format is now a separate, named call. - `sanitize_filename` no longer panics. The 200 cap is in BYTES and `truncate` panics off a char boundary — reachable because the mapping keeps Unicode alphanumerics. Measured: the crash belongs to the THREE-byte class (CJK), where 200 % 3 lands mid-character; two-byte scripts happen to land on a boundary. The cap stays in bytes so no already-downloaded path moves. - The purchase path sends `/album/get` and `/track/get` UNSIGNED, matching both the reference and Qobuz's own desktop client, whose request helper takes a sign flag and passes it false for these and true only for `getFileUrl`. That signature is the entitlement proof and is untouched. - The purchase-type guard runs before any I/O; a bogus type no longer issues a live authenticated GET before failing. - Track subtitles survive. `PurchaseTrack` gained `version` and `build_purchase_album` maps it from the catalog track, closing a latent regression where every purchased track lost its edition. - Downloads write tags, `cover.jpg`/`back.jpg` and goodies. All three are best-effort: the audio file is the deliverable and is already on disk and registered before any of it runs. - `goodies` is deserialized leniently. Its populated shape has never been observed — it comes back empty on every album nobody owns — and `/album/get` is parsed with the strict structs, so an unexpected shape would take the whole album down rather than merely hide a booklet. Riding along, both found while building the above and both unrelated to Purchases: - The metadata EDITOR silently discarded saves. `write_album_tags_to_files` picked its write target as "the primary tag, or else the first one I can find", and lofty refuses to WRITE a tag type a container only supports READING — silently, skipping it rather than erroring. A FLAC carrying an ID3v2 tag has no Vorbis comments, so every edited field went into that ID3v2 tag, the save dropped the lot, and the call returned Ok. The user pressed Save, saw a success toast, and the file never changed. Same defect, same fix, in the new purchase tagger. - `fetch_asset_bytes` does not reuse the audio downloader. That one sizes its buffer from the response's own Content-Length and sets no total timeout, which is fine for a track from a trusted CDN and wrong for an arbitrary goodie URL on a machine that hard-freezes under memory pressure. Tests: 277 across the four crates, covering every acceptance item the contract set. The signature preimage character for character, the pagination loop including the zero-total case, the sanitizer against the inputs that used to panic, every deserializer default, and the wire envelopes against real bytes captured from a live account. A download harness stands up a local HTTP server and drives the pipeline end to end — real bytes over a real socket, a real FLAC tagged and read back, a real database — and the two tag regressions were both verified to FAIL against the old code before being accepted. cc --- crates/qbz-library/src/database.rs | 230 ++++- crates/qbz-library/src/lib.rs | 3 +- crates/qbz-library/src/tag_writer.rs | 301 +++++- crates/qbz-models/src/lib.rs | 1 + crates/qbz-models/src/purchase_serde.rs | 30 +- crates/qbz-models/src/types.rs | 290 +++++- .../purchases/getUserPurchases-noparams.json | 18 + .../getUserPurchases-type-albums.json | 12 + .../getUserPurchases-type-tracks.json | 12 + .../getUserPurchasesIds-noparams.json | 14 + .../getUserPurchasesIds-type-albums.json | 10 + .../tests/purchase_wire_fixtures.rs | 142 +++ crates/qbz-offline-cache/Cargo.toml | 12 + crates/qbz-offline-cache/src/metadata.rs | 21 +- .../src/purchases_service.rs | 909 ++++++++++++++++-- .../tests/purchase_download_harness.rs | 562 +++++++++++ crates/qbz-qobuz/src/purchases.rs | 309 +++++- crates/qbz/src/purchases.rs | 7 + 18 files changed, 2767 insertions(+), 116 deletions(-) create mode 100644 crates/qbz-models/tests/fixtures/purchases/getUserPurchases-noparams.json create mode 100644 crates/qbz-models/tests/fixtures/purchases/getUserPurchases-type-albums.json create mode 100644 crates/qbz-models/tests/fixtures/purchases/getUserPurchases-type-tracks.json create mode 100644 crates/qbz-models/tests/fixtures/purchases/getUserPurchasesIds-noparams.json create mode 100644 crates/qbz-models/tests/fixtures/purchases/getUserPurchasesIds-type-albums.json create mode 100644 crates/qbz-models/tests/purchase_wire_fixtures.rs create mode 100644 crates/qbz-offline-cache/tests/purchase_download_harness.rs 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 {