diff --git a/docs/opensubsonic-gap-analysis.md b/docs/opensubsonic-gap-analysis.md index 6aaf3e0..e253c3b 100644 --- a/docs/opensubsonic-gap-analysis.md +++ b/docs/opensubsonic-gap-analysis.md @@ -133,18 +133,30 @@ qui ne se manifeste qu'en production, chez un utilisateur, une seule fois. > rejoués le 23 août 2026 contre le modèle aligné, et > [`subsonic-compatibility.md`](subsonic-compatibility.md) porte le résultat. +> **Addendum du 2026-08-24.** Le point 2 ci-dessous est clos. Les cinq champs +> de sortie d'album que l'addendum de la veille laissait ouverts sont livrés : +> `originalReleaseDate`, `releaseDate`, `releaseTypes[]` et `recordLabels[]` +> pendent de l'album et se remplissent comme `year`, `discTitles[]` se dérive +> des pistes disponibles comme les genres. Les trois tableaux sont émis vides +> plutôt qu'absents, sous les deux noms d'élément qu'un album porte — `album` +> et, dans un dossier, `child`. Les deux dates sont omises quand aucun tag ne +> les nomme, comme le fait la référence : un `ItemDate` sans année n'est pas +> une date, et les tableaux portent déjà le signal de présence du groupe. +> +> **Le fil a bougé.** Les quatre clients demandent d'être rejoués avant un tag +> stable, ce que le §4 du handover du 23 août exige déjà. + Rien de structurel. La liste tient en quatre lignes, et deux d'entre elles sont des dettes nommées plutôt que des défauts. 1. **Le correctif OAuth durable** — porter les portées à travers la concession. Deux colonnes, deux migrations. Le chemin est fermé aujourd'hui ; ce qui reste, c'est que la propriété soit structurelle et non locale à une route. -2. **Champs `AlbumID3` et `ArtistID3`** demandant des colonnes : `sortName` sur - les deux — le moins cher, le scanner lit déjà `sort_title` sur la piste — - puis `moods[]`, `explicitStatus`, `originalReleaseDate`, `releaseDate`, - `releaseTypes[]`, `recordLabels[]`, `discTitles[]`, `roles[]`, et - `contributors[]`/`displayComposer` côté piste. Absents plutôt que vides, - ce qui sous la règle de présence dit exactement « non supporté ». +2. ~~**Champs `AlbumID3` et `ArtistID3`** demandant des colonnes.~~ Livrés : + `sortName` par la PR #123, `moods[]`, `explicitStatus`, `roles[]`, + `contributors[]` et `displayComposer` par la PR #126, puis + `originalReleaseDate`, `releaseDate`, `releaseTypes[]`, `recordLabels[]` et + `discTitles[]` le 24 août 2026. 3. **`song.parent` retombe sur `library_id`** sans album (`src/subsonic.rs:1706`) : un `getMusicDirectory` sur cet identifiant ne renverra pas la piste. 4. **Deux inexactitudes de surface** : `getLicense` expire en dur au diff --git a/migrations-v2/20260824010000_album_release_details.sql b/migrations-v2/20260824010000_album_release_details.sql new file mode 100644 index 0000000..647fb91 --- /dev/null +++ b/migrations-v2/20260824010000_album_release_details.sql @@ -0,0 +1,21 @@ +-- The album output fields OpenSubsonic names and this server did not answer. +-- +-- `originalReleaseDate`, `releaseDate`, `releaseTypes[]` and `recordLabels[]` +-- describe the release rather than the recording, so they sit on the album and +-- are filled exactly the way `year` is: a track that names a value writes it, +-- and a track that names none leaves what is there. The last writer wins, which +-- is what lets a corrected tag reach the album on a rescan instead of being +-- held off by the first spelling the catalogue ever saw. +-- +-- `discTitles[]` cannot be stored that way — it holds one title per disc — so +-- the tag lands on the track and the album derives the list from its available +-- tracks, the way it already derives its genres and its credits. +-- +-- Added empty and filled by the next scan, like every other tag column: an +-- instance that never rescans reports the fields supported and unset rather +-- than reporting something wrong. +ALTER TABLE album ADD COLUMN original_release_date TEXT; +ALTER TABLE album ADD COLUMN release_date TEXT; +ALTER TABLE album ADD COLUMN release_types TEXT; +ALTER TABLE album ADD COLUMN record_labels TEXT; +ALTER TABLE track ADD COLUMN disc_subtitle TEXT; diff --git a/src/catalog.rs b/src/catalog.rs index 1284a18..0af23eb 100644 --- a/src/catalog.rs +++ b/src/catalog.rs @@ -119,6 +119,18 @@ pub struct CatalogTrackInput { pub moods: Option, /// Normalised to `explicit` or `clean`; any other tag value is no value. pub explicit_status: Option, + /// The two release dates, as the file spelled them. Kept as written and + /// taken apart only at the wire, where OpenSubsonic wants year, month and + /// day as separate numbers: a tag that names only a year must not be + /// reported as the first of January. + pub original_release_date: Option, + pub release_date: Option, + /// Multi-valued, split like `moods`. + pub release_types: Option, + pub record_labels: Option, + /// The title of the disc this track sits on. Track-level because an album + /// has one per disc, which is the whole point of the field. + pub disc_subtitle: Option, pub artwork: Option, pub lyrics_hash: String, pub lyrics: Vec, @@ -366,6 +378,17 @@ impl Database { "catalogue identity setting changed since the last scan" ); } + // Before the rescan, while the artist rows still hold the identifiers + // their favourites name. + if changed.iter().any(|(key, _, _)| *key == "pid.artist") { + let moved = self.remap_artist_user_data(specs).await?; + if moved > 0 { + tracing::warn!( + rows = moved, + "favourites and ratings moved onto the artist identifiers the new rule derives" + ); + } + } let libraries = self.request_full_scan_everywhere().await?; tracing::warn!( libraries, @@ -416,6 +439,72 @@ impl Database { Ok(()) } + /// Carries artist favourites and ratings across a change of artist spec. + /// + /// `user_star` and `user_rating` hold an untyped identifier with no foreign + /// key, so a rescan that re-derives artist ids leaves their rows pointing at + /// identifiers nothing answers for — invisible rather than wrong, since every + /// projection resolves through an `EXISTS`, but lost all the same. The old + /// identifier and the name that produced it are both still on the `artist` + /// row at this point, which is the only moment the two can be paired. + /// + /// Albums are not remapped and cannot be: their spec reads `albumversion` + /// and `releasedate`, which live on the files rather than on the album row, + /// so there is nothing here to derive the new identifier from. + /// + /// **Moved in two phases, through a namespace no identifier can occupy.** + /// One artist's new identifier can be another's old one, and moving them one + /// at a time would then carry the first artist's favourite onto the second's + /// row — a result that depends on the order the rows came back in, which is + /// no result at all. Staging every row first and landing them afterwards + /// makes the outcome the same whatever that order was. Both phases run in + /// one transaction, so a staged value is never visible to anything. + /// + /// `UPDATE OR IGNORE` then `DELETE` at each phase: a coarser spec can fold + /// two artists onto one identifier, and the second row would collide on + /// `(user_id, entity_type, entity_id)`. Losing the duplicate is right — the + /// user already stars what it would have become. + async fn remap_artist_user_data( + &self, + specs: &crate::pid::PidSpecs, + ) -> Result { + let rows = sqlx::query("SELECT id, library_id, name FROM artist") + .fetch_all(self.pool()) + .await?; + let mut moves = Vec::new(); + for row in rows { + let old: String = row.try_get("id")?; + let library_id = parse_uuid(row.try_get("library_id")?)?; + let name: String = row.try_get("name")?; + let new = specs.artist_id(library_id, &name).to_string(); + if new != old { + // A UUID holds no colon, so nothing that reaches these columns + // by any other route can be mistaken for a staged value. + moves.push((old, format!("{REMAP_STAGE}{new}"), new)); + } + } + if moves.is_empty() { + return Ok(0); + } + let _writer = self.writer_guard().await; + let mut tx = self.pool().begin().await?; + let mut moved = 0; + for (from, to) in moves + .iter() + .map(|(old, staged, _)| (old, staged)) + .chain(moves.iter().map(|(_, staged, new)| (staged, new))) + { + let landed = move_artist_user_data(&mut tx, from, to).await?; + // Counted on the second pass only, where a row reaches the + // identifier it will actually be read under. + if from.starts_with(REMAP_STAGE) { + moved += landed; + } + } + tx.commit().await?; + Ok(moved) + } + pub async fn library_for_user( &self, user_id: Uuid, @@ -1118,10 +1207,11 @@ impl Database { replay_gain_album_gain, replay_gain_album_peak, bpm, sort_title, sort_album, \ comment, isrc, \ moods, explicit_status, \ - lyrics_hash, pid, is_available, last_seen_scan_id, created_at, updated_at) \ + lyrics_hash, disc_subtitle, pid, is_available, last_seen_scan_id, \ + created_at, updated_at) \ VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, \ ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, \ - ?, 1, ?, ?, ?) \ + ?, ?, 1, ?, ?, ?) \ ON CONFLICT (id) DO UPDATE SET album_id=excluded.album_id, artwork_hash=excluded.artwork_hash, \ relative_path=excluded.relative_path, file_size=excluded.file_size, \ file_modified_at=excluded.file_modified_at, quick_hash=excluded.quick_hash, \ @@ -1142,7 +1232,8 @@ impl Database { comment=excluded.comment, \ isrc=excluded.isrc, moods=excluded.moods, \ explicit_status=excluded.explicit_status, \ - lyrics_hash=excluded.lyrics_hash, pid=excluded.pid, \ + lyrics_hash=excluded.lyrics_hash, disc_subtitle=excluded.disc_subtitle, \ + pid=excluded.pid, \ is_available=1, last_seen_scan_id=excluded.last_seen_scan_id, \ updated_at=excluded.updated_at", ) @@ -1164,6 +1255,7 @@ impl Database { .bind(input.isrc.as_deref()) .bind(input.moods.as_deref()).bind(input.explicit_status.as_deref()) .bind(&input.lyrics_hash) + .bind(input.disc_subtitle.as_deref()) .bind(track_pid(pid, library_id, input).to_string()) .bind(scan_id.to_string()).bind(now).bind(now) .execute(&mut **tx).await?; @@ -1655,8 +1747,10 @@ async fn upsert_album( ); sqlx::query( "INSERT INTO album (id, library_id, title, canonical_title, album_artist_id, \ - album_artist_name, is_compilation, year, artwork_hash, created_at, updated_at) \ - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) \ + album_artist_name, is_compilation, year, artwork_hash, \ + original_release_date, release_date, release_types, record_labels, \ + created_at, updated_at) \ + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) \ ON CONFLICT (id) DO UPDATE SET title=excluded.title, \ canonical_title=excluded.canonical_title, \ album_artist_id=excluded.album_artist_id, \ @@ -1664,6 +1758,10 @@ async fn upsert_album( is_compilation=excluded.is_compilation, \ year=COALESCE(excluded.year, album.year), \ artwork_hash=COALESCE(excluded.artwork_hash, album.artwork_hash), \ + original_release_date=COALESCE(excluded.original_release_date, album.original_release_date), \ + release_date=COALESCE(excluded.release_date, album.release_date), \ + release_types=COALESCE(excluded.release_types, album.release_types), \ + record_labels=COALESCE(excluded.record_labels, album.record_labels), \ updated_at=excluded.updated_at", ) .bind(id.to_string()) @@ -1675,6 +1773,10 @@ async fn upsert_album( .bind(i64::from(input.is_compilation)) .bind(input.year) .bind(artwork) + .bind(input.original_release_date.as_deref()) + .bind(input.release_date.as_deref()) + .bind(input.release_types.as_deref()) + .bind(input.record_labels.as_deref()) .bind(now) .bind(now) .execute(&mut **tx) @@ -1691,6 +1793,48 @@ fn split_values(raw: Option<&str>) -> Vec { .collect() } +/// The namespace a remap stages through. A UUID holds no colon, so a staged +/// value cannot be mistaken for an identifier and no identifier for it. +const REMAP_STAGE: &str = "pid-remap:"; + +/// Moves one artist identifier onto another, in both tables that hold one. +/// +/// Spelled out per table rather than looped: sqlx takes static SQL only, which +/// is what keeps every query in this crate injection-proof by construction. +async fn move_artist_user_data( + tx: &mut Transaction<'_, Sqlite>, + from: &str, + to: &str, +) -> Result { + let mut moved = sqlx::query( + "UPDATE OR IGNORE user_star SET entity_id = ? \ + WHERE entity_type = 'artist' AND entity_id = ?", + ) + .bind(to) + .bind(from) + .execute(&mut **tx) + .await? + .rows_affected(); + sqlx::query("DELETE FROM user_star WHERE entity_type = 'artist' AND entity_id = ?") + .bind(from) + .execute(&mut **tx) + .await?; + moved += sqlx::query( + "UPDATE OR IGNORE user_rating SET entity_id = ? \ + WHERE entity_type = 'artist' AND entity_id = ?", + ) + .bind(to) + .bind(from) + .execute(&mut **tx) + .await? + .rows_affected(); + sqlx::query("DELETE FROM user_rating WHERE entity_type = 'artist' AND entity_id = ?") + .bind(from) + .execute(&mut **tx) + .await?; + Ok(moved) +} + fn library_from_row(row: sqlx::sqlite::SqliteRow) -> Result { Ok(LibraryRecord { id: parse_uuid(row.try_get("id")?)?, diff --git a/src/scanner.rs b/src/scanner.rs index c94914e..69ac996 100644 --- a/src/scanner.rs +++ b/src/scanner.rs @@ -536,6 +536,11 @@ fn extract_file(path: &Path, artwork_dir: &Path) -> Result, moods: Option, explicit_status: Option, + original_release_date: Option, + release_date: Option, + release_types: Option, + record_labels: Option, + disc_subtitle: Option, } /// Reads every credit a file names, in tag order. @@ -761,6 +776,17 @@ fn extended_tags(tag: Option<&lofty::tag::Tag>) -> ExtendedTags { .filter(|value| !value.is_empty()) .map(str::to_owned) }; + // A tag written as several items rather than one `;`-joined string. Joined + // here so the column holds one spelling, and split again on the way out by + // the same helper that splits `moods`. + let joined = |key: ItemKey| { + let values = tag + .get_strings(key) + .map(str::trim) + .filter(|value| !value.is_empty()) + .collect::>(); + (!values.is_empty()).then(|| values.join("; ")) + }; // ReplayGain tags carry their unit: `-7.32 dB`. Reading only the first // token keeps the suffix from turning a valid measurement into none. A // non-finite value is discarded rather than stored: it would travel all @@ -810,6 +836,14 @@ fn extended_tags(tag: Option<&lofty::tag::Tag>) -> ExtendedTags { _ => None, } }), + original_release_date: text(ItemKey::OriginalReleaseDate), + release_date: text(ItemKey::ReleaseDate).or_else(|| text(ItemKey::RecordingDate)), + release_types: joined(ItemKey::MusicBrainzReleaseType), + // `LABEL` is the tag Picard writes and the one the reference reads; + // `PUBLISHER` is the older spelling the same value arrives under on + // files tagged by anything else. + record_labels: joined(ItemKey::Label).or_else(|| joined(ItemKey::Publisher)), + disc_subtitle: text(ItemKey::SetSubtitle), } } diff --git a/src/services/catalog.rs b/src/services/catalog.rs index 5475f4c..64fb84e 100644 --- a/src/services/catalog.rs +++ b/src/services/catalog.rs @@ -133,11 +133,20 @@ impl DomainServices { /// which the previous lowercase substring test did not. What it gives up is /// matching inside a word: "cho" no longer finds "Echo". The trailing term /// is treated as a prefix so search-as-you-type still works. + /// Paged in SQL, one page per kind. + /// + /// The three pages are independent because `search3` has always let a + /// client page songs past the end of the artists. Slicing a full result in + /// the renderer, which is what this used to leave it to do, read the whole + /// matching catalogue to answer for twenty rows of it. pub async fn catalog_search( &self, user_id: Uuid, folder_ids: &[Uuid], query: &str, + artist_page: BrowsePage, + album_page: BrowsePage, + song_page: BrowsePage, ) -> Result { let Some(fts) = crate::catalog::fts_prefix_query(query) else { return Ok(CatalogSearch { @@ -153,12 +162,14 @@ impl DomainServices { song_select!(), " AND (? IS NULL OR t.library_id IN (SELECT value FROM json_each(?))) \ AND t.id IN (SELECT track_id FROM track_fts WHERE track_fts MATCH ?) \ - ORDER BY t.title COLLATE NOCASE, t.id" + ORDER BY t.title COLLATE NOCASE, t.id LIMIT ? OFFSET ?" )) .bind(user_id.to_string()) .bind(folder_filter) .bind(folder_filter) .bind(&fts) + .bind(song_page.limit) + .bind(song_page.offset) .fetch_all(self.db.pool()) .await? .into_iter() @@ -171,12 +182,14 @@ impl DomainServices { " AND (? IS NULL OR al.library_id IN (SELECT value FROM json_each(?))) \ AND al.id IN (SELECT t.album_id FROM track t WHERE t.album_id IS NOT NULL \ AND t.id IN (SELECT track_id FROM track_fts WHERE track_fts MATCH ?)) \ - ORDER BY al.title COLLATE NOCASE, al.id" + ORDER BY al.title COLLATE NOCASE, al.id LIMIT ? OFFSET ?" )) .bind(user_id.to_string()) .bind(folder_filter) .bind(folder_filter) .bind(&fts) + .bind(album_page.limit) + .bind(album_page.offset) .fetch_all(self.db.pool()) .await? .into_iter() @@ -188,12 +201,14 @@ impl DomainServices { artist_select!(), " AND (? IS NULL OR ar.library_id IN (SELECT value FROM json_each(?))) \ AND ar.id IN (SELECT artist_id FROM artist_fts WHERE artist_fts MATCH ?) \ - ORDER BY ar.name COLLATE NOCASE" + ORDER BY ar.name COLLATE NOCASE, ar.id LIMIT ? OFFSET ?" )) .bind(user_id.to_string()) .bind(folder_filter) .bind(folder_filter) .bind(&fts) + .bind(artist_page.limit) + .bind(artist_page.offset) .fetch_all(self.db.pool()) .await? .into_iter() diff --git a/src/services/mod.rs b/src/services/mod.rs index 515fe64..93beaa1 100644 --- a/src/services/mod.rs +++ b/src/services/mod.rs @@ -79,6 +79,7 @@ macro_rules! album_select { () => { "SELECT al.id, al.library_id, al.title, al.album_artist_name, al.album_artist_id, \ al.artwork_hash, al.year, al.is_compilation, al.musicbrainz_id, al.sort_name, \ + al.original_release_date, al.release_date, al.release_types, al.record_labels, \ al.created_at, us.starred_at, \ ur.rating AS user_rating, \ (SELECT COUNT(*) FROM play_event pe JOIN track pt ON pt.id=pe.track_id \ @@ -180,6 +181,13 @@ pub struct ArtistItem { pub roles: Vec, } +/// A disc of an album, and the title its tracks give it. +#[derive(Debug, Clone, Serialize, ToSchema)] +pub struct DiscTitle { + pub disc: i64, + pub title: String, +} + #[derive(Debug, Clone, Serialize, ToSchema)] pub struct AlbumItem { pub id: Uuid, @@ -198,6 +206,16 @@ pub struct AlbumItem { /// or genre of its own in the schema, only the union of its files'. pub artists: Vec, pub genres: Vec, + /// The release description OpenSubsonic asks an album for. The dates are + /// kept as the file spelled them and taken apart only at the wire — a tag + /// naming a year alone must not be reported as the first of January. + pub original_release_date: Option, + pub release_date: Option, + pub release_types: Vec, + pub record_labels: Vec, + /// One entry per disc the album's available tracks name a title for. + /// Derived like the genres, because an album has as many as it has discs. + pub disc_titles: Vec, pub created_at: i64, pub starred_at: Option, pub user_rating: Option, @@ -351,6 +369,15 @@ pub const MAX_HISTORY_LIMIT: i64 = 500; pub const MAX_QUEUE_TRACKS: usize = 400; /// Applies the same request-size and writer-gate bound to public shares. pub const MAX_SHARE_TRACKS: usize = MAX_QUEUE_TRACKS; +/// Upper bound on the tracks one playlist may hold. +/// +/// Deliberately not [`MAX_QUEUE_TRACKS`]: that one bounds a request, and a +/// queue is written whole by every call. A playlist grows across many calls, so +/// the same number would refuse ordinary libraries. What this bounds is the +/// rewrite — `replace_playlist_tracks` deletes and reinserts the whole list on +/// every edit, under the process-wide writer gate — and ten thousand keeps that +/// bounded while sitting far above any playlist a person curates by hand. +pub const MAX_PLAYLIST_TRACKS: usize = 10_000; /// Offset/limit pair validated once, at the HTTP boundary, so the SQL layer can /// bind it without re-checking bounds. @@ -832,9 +859,37 @@ async fn attach_album_relations( .or_default() .push(row.try_get("name")?); } + let mut disc_titles: HashMap> = HashMap::new(); + for row in sqlx::query( + // One title per disc, and the first spelling in disc order when the + // tracks of one disc disagree — the same `MIN` the genres use, for the + // same reason: an album must not report a disc twice because two of + // its files were tagged by different hands. + "SELECT t.album_id, t.disc_number, MIN(t.disc_subtitle) AS title FROM track t \ + JOIN library_member m ON m.library_id=t.library_id \ + WHERE m.user_id=? AND t.is_available=1 AND t.disc_subtitle IS NOT NULL \ + AND t.disc_number IS NOT NULL \ + AND t.album_id IN (SELECT value FROM json_each(?)) \ + GROUP BY t.album_id, t.disc_number \ + ORDER BY t.album_id, t.disc_number", + ) + .bind(user_id.to_string()) + .bind(&ids) + .fetch_all(&mut *connection) + .await? + { + disc_titles + .entry(parse_uuid(row.try_get("album_id")?)?) + .or_default() + .push(DiscTitle { + disc: row.try_get("disc_number")?, + title: row.try_get("title")?, + }); + } for album in albums { album.artists = artists.remove(&album.id).unwrap_or_default(); album.genres = genres.remove(&album.id).unwrap_or_default(); + album.disc_titles = disc_titles.remove(&album.id).unwrap_or_default(); } Ok(()) } @@ -1075,9 +1130,20 @@ fn album_from_row(row: sqlx::sqlite::SqliteRow) -> Result("is_compilation")? != 0, sort_name: row.try_get("sort_name")?, musicbrainz_id: row.try_get("musicbrainz_id")?, + original_release_date: row.try_get("original_release_date")?, + release_date: row.try_get("release_date")?, + release_types: split_tag_values( + row.try_get::, _>("release_types")? + .as_deref(), + ), + record_labels: split_tag_values( + row.try_get::, _>("record_labels")? + .as_deref(), + ), // Loaded in a batch by `attach_album_relations`, never row by row. artists: Vec::new(), genres: Vec::new(), + disc_titles: Vec::new(), created_at: row.try_get("created_at")?, starred_at: row.try_get("starred_at")?, user_rating: row.try_get("user_rating")?, diff --git a/src/services/playback.rs b/src/services/playback.rs index 170827b..bd2e4d9 100644 --- a/src/services/playback.rs +++ b/src/services/playback.rs @@ -108,18 +108,32 @@ impl DomainServices { ) .fetch_all(self.db.pool()) .await?; + // One lookup for every listener at once. Resolving them one by one cost + // a query per row, and every row of this table is a different person + // playing something right now — the list is short but the shape was + // wrong. + let mut track_ids = rows + .iter() + .map(|row| parse_uuid(row.try_get("track_id")?)) + .collect::, _>>()?; + track_ids.sort_unstable(); + track_ids.dedup(); + let mut connection = self.db.pool().acquire().await?; + let visible = self + .songs_by_ids_lenient_on(&mut connection, user_id, &track_ids) + .await? + .into_iter() + .map(|song| (song.id, song)) + .collect::>(); let mut result = Vec::new(); for row in rows { let id = parse_uuid(row.try_get("track_id")?)?; - match self.songs_by_ids(user_id, &[id]).await { - Ok(mut songs) => { - if let Some(song) = songs.pop() { - result.push((row.try_get("username")?, song, row.try_get("started_at")?)); - } - } - Err(ServiceError::NotFound) => continue, - Err(error) => return Err(error), - } + // A track this account cannot see is skipped, as it was when the + // strict lookup answered `NotFound` for it. + let Some(song) = visible.get(&id).cloned() else { + continue; + }; + result.push((row.try_get("username")?, song, row.try_get("started_at")?)); } Ok(result) } diff --git a/src/services/playlists.rs b/src/services/playlists.rs index f8caf23..be6b8a3 100644 --- a/src/services/playlists.rs +++ b/src/services/playlists.rs @@ -22,9 +22,45 @@ impl DomainServices { .bind(user_id.to_string()) .fetch_all(&mut *connection) .await?; + // One query for every playlist's track list and one for the songs they + // name, rather than two per playlist: an account with fifty playlists + // was costing a hundred round trips to answer `getPlaylists`. + let playlist_ids = rows + .iter() + .map(|row| parse_uuid(row.try_get("id")?)) + .collect::, _>>()?; + let mut ordered: HashMap> = HashMap::new(); + if !playlist_ids.is_empty() { + let ids_json = + serde_json::to_string(&playlist_ids).map_err(|_| ServiceError::Invalid)?; + for row in sqlx::query( + "SELECT pt.playlist_id, pt.track_id FROM playlist_track pt JOIN playlist p ON p.id=pt.playlist_id WHERE p.owner_user_id=? AND p.id IN (SELECT value FROM json_each(?)) ORDER BY pt.playlist_id, pt.position", + ) + .bind(user_id.to_string()) + .bind(ids_json) + .fetch_all(&mut *connection) + .await? + { + ordered + .entry(parse_uuid(row.try_get("playlist_id")?)?) + .or_default() + .push(parse_uuid(row.try_get("track_id")?)?); + } + } + let mut union: Vec = ordered.values().flatten().copied().collect(); + union.sort_unstable(); + union.dedup(); + // Resolved once for every playlist at once. The per-playlist order and + // the dropping of a track this account cannot see are reapplied below, + // exactly as `songs_by_ids_lenient_on` would have applied them. + let visible = self + .songs_by_ids_lenient_on(connection, user_id, &union) + .await? + .into_iter() + .map(|song| (song.id, song)) + .collect::>(); let mut result = Vec::with_capacity(rows.len()); - for row in rows { - let id = parse_uuid(row.try_get("id")?)?; + for (row, id) in rows.into_iter().zip(playlist_ids) { result.push(PlaylistItem { id, name: row.try_get("name")?, @@ -32,7 +68,12 @@ impl DomainServices { public: row.try_get::("public")? != 0, created_at: row.try_get("created_at")?, updated_at: row.try_get("updated_at")?, - songs: self.playlist_songs_on(connection, user_id, id).await?, + songs: ordered + .get(&id) + .into_iter() + .flatten() + .filter_map(|track| visible.get(track).cloned()) + .collect(), }); } Ok(result) @@ -142,6 +183,14 @@ impl DomainServices { drop(_writer); return self.playlist(user_id, id).await; } + // Checked after the replay branch and not before it. A replay owes the + // caller the outcome the original call had, and this ceiling is a + // policy number rather than a fact of the domain: lower it in a later + // release and an operation that was valid when it ran would start + // answering an error to its own retry. Still ahead of every write. + if track_ids.len() > MAX_PLAYLIST_TRACKS { + return Err(ServiceError::Invalid); + } validate_name(name)?; self.songs_by_ids_on(&mut tx, user_id, track_ids).await?; let id = Uuid::new_v4(); @@ -243,6 +292,13 @@ impl DomainServices { drop(_writer); return self.playlist(user_id, id).await; } + // A request naming more tracks than a playlist may hold can never be + // valid, whatever this playlist currently holds, so it is refused + // before anything is read. After the replay branch for the same reason + // the create path is: a retry is owed its original outcome. + if add.len() > MAX_PLAYLIST_TRACKS { + return Err(ServiceError::Invalid); + } let current = self.playlist_on(&mut tx, user_id, id).await?; if let Some(name) = name { validate_name(name)?; @@ -260,6 +316,12 @@ impl DomainServices { ids.remove(index); } ids.extend_from_slice(add); + // Checked on the result rather than on `add`: an update that adds one + // track to a playlist already at the ceiling is what has to be refused, + // and the request that gets there is small. + if ids.len() > MAX_PLAYLIST_TRACKS { + return Err(ServiceError::Invalid); + } let changed_at = now_ms(); sqlx::query( "UPDATE playlist SET name=COALESCE(?, name), \ diff --git a/src/subsonic/browse.rs b/src/subsonic/browse.rs index 57f398c..7c943a0 100644 --- a/src/subsonic/browse.rs +++ b/src/subsonic/browse.rs @@ -398,18 +398,30 @@ pub(super) async fn search( )); } + let page = |offset: usize, count: usize| { + BrowsePage::new(Some(offset as i64), Some(count as i64)).map_err(service_protocol) + }; let found = state .services - .catalog_search(principal.id, &folders, raw_query) + .catalog_search( + principal.id, + &folders, + raw_query, + page(artist_offset, artist_count.max(1))?, + page(album_offset, album_count.max(1))?, + page(song_offset, song_count.max(1))?, + ) .await - .map_err(internal)?; + .map_err(service_protocol)?; + // The service applied the offsets, so the renderer must not — the same + // division the match-all branch above already makes. Ok(search_result( found.artists.iter(), found.albums.iter(), found.songs.iter(), - (artist_offset, artist_count), - (album_offset, album_count), - (song_offset, song_count), + (0, artist_count), + (0, album_count), + (0, song_count), )) } diff --git a/src/subsonic/nodes.rs b/src/subsonic/nodes.rs index 845eac8..5b4883d 100644 --- a/src/subsonic/nodes.rs +++ b/src/subsonic/nodes.rs @@ -78,6 +78,68 @@ pub(super) fn album_node(album: &AlbumItem) -> Node { "musicBrainzId", album.musicbrainz_id.clone().unwrap_or_default(), ) + .children( + album + .record_labels + .iter() + .map(|label| Node::new("recordLabels").attr("name", label.clone())), + ) + .children( + album + .release_types + .iter() + .map(|kind| Node::new("releaseTypes").text(kind.clone())), + ) + .children(album.disc_titles.iter().map(|disc| { + Node::new("discTitles") + .attr("disc", disc.disc) + .attr("title", disc.title.clone()) + })) + // Omitted rather than emitted empty when the tag says nothing, which is + // what the reference does: an `ItemDate` with no year is not a date. + // The three arrays above already carry the presence signal for the + // group, so a client can still tell "unknown" from "not supported". + .children( + item_date( + "originalReleaseDate", + album.original_release_date.as_deref(), + ) + .into_iter() + .chain(item_date("releaseDate", album.release_date.as_deref())), + ) +} + +/// An `ItemDate` from the tag as the file spelled it. +/// +/// Only the parts the tag actually names are emitted: `2019` is a year and +/// nothing more, and reporting it as 1 January 2019 would invent a precision +/// the file never claimed. Anything that does not start with a four-digit year +/// is no date at all — `19980405` written without separators is not the year +/// 19,980,405, and a bare `5` is not the year 5. +fn item_date(name: &'static str, raw: Option<&str>) -> Option { + let raw = raw?.trim(); + let mut parts = raw.split(['-', '/', '.']); + let head = parts.next()?.trim(); + if head.len() != 4 || !head.bytes().all(|byte| byte.is_ascii_digit()) { + return None; + } + let year: i64 = head.parse().ok().filter(|y| *y > 0)?; + let month = parts + .next() + .and_then(|value| value.trim().parse::().ok()) + .filter(|value| (1..=12).contains(value)); + let day = month.and( + parts + .next() + .and_then(|value| value.trim().parse::().ok()) + .filter(|value| (1..=31).contains(value)), + ); + Some( + Node::new(name) + .attr("year", year) + .maybe_attr("month", month) + .maybe_attr("day", day), + ) } pub(super) fn song_node(song: &SongItem) -> Node { diff --git a/src/subsonic/protocol.rs b/src/subsonic/protocol.rs index e53d1a5..70cd24b 100644 --- a/src/subsonic/protocol.rs +++ b/src/subsonic/protocol.rs @@ -86,7 +86,10 @@ pub(super) fn node_json(node: &Node, parent: &str) -> Value { // identifier off a directory, and `artists: []` would be the list // of the artists of an artist. EntryKind::Artist => *name == "roles", - EntryKind::Album => matches!(*name, "artists" | "genres"), + EntryKind::Album => matches!( + *name, + "artists" | "genres" | "recordLabels" | "releaseTypes" | "discTitles" + ), EntryKind::Song => true, }; if !injected { @@ -130,8 +133,17 @@ pub(super) fn json_required_array_fields(parent: &str, name: &str) -> &'static [ "moods", "albumArtists", "contributors", + "recordLabels", + "releaseTypes", + "discTitles", + ], + "album" => &[ + "artists", + "genres", + "recordLabels", + "releaseTypes", + "discTitles", ], - "album" => &["artists", "genres"], // The roles an artist is credited in, empty rather than absent for // the same reason: absent would say the server does not read them. "artist" => &["roles"], @@ -205,6 +217,11 @@ pub(super) fn json_array_field(parent: &str, name: &str) -> bool { // a playlist or share and to `child` inside a directory. Its // OpenSubsonic relations are arrays under all three names. | ("song" | "entry" | "child" | "album", "artists" | "genres") + // `getMusicDirectory` renders an album as `child`, so its arrays + // have to keep their shape under that name too — otherwise one + // record label collapses into a bare object the moment a directory + // carries the album. + | ("album" | "child", "recordLabels" | "releaseTypes" | "discTitles") | ("song" | "entry" | "child", "isrc" | "moods" | "albumArtists") | ("song" | "entry" | "child", "contributors") // An artist rendered as a browsing child keeps the record's shape, diff --git a/src/subsonic/userdata.rs b/src/subsonic/userdata.rs index e52dffa..a8cb53d 100644 --- a/src/subsonic/userdata.rs +++ b/src/subsonic/userdata.rs @@ -51,13 +51,31 @@ pub(super) async fn delete_bookmark( Ok(Node::new("deleteBookmark")) } +/// How many identifiers one request may name, across every parameter that +/// carries them. +/// +/// Each one costs a mutation that takes the process-wide writer gate, and the +/// form body admits some fifteen hundred UUIDs — enough for one well-formed +/// request to hold the gate against every other writer for as long as it takes +/// to serialise them. The bound is the one the queue and shares already use, +/// and for the same reason. +const MAX_IDENTIFIERS: usize = crate::services::MAX_QUEUE_TRACKS; + pub(super) async fn set_star( state: &AppState, principal: &Principal, params: &Params, starred: bool, ) -> Result { - for id in params.uuids("id")? { + // Read before any of them is applied: a request refused halfway would leave + // the first few identifiers starred and report an error for the whole. + let tracks = params.uuids("id")?; + let albums = params.uuids("albumId")?; + let artists = params.uuids("artistId")?; + if tracks.len() + albums.len() + artists.len() > MAX_IDENTIFIERS { + return Err(invalid("Too many identifiers in one request")); + } + for id in tracks { let kind = state .services .entity_kind(principal.id, id) @@ -70,8 +88,8 @@ pub(super) async fn set_star( .await .map_err(service_protocol)?; } - for (key, kind) in [("albumId", "album"), ("artistId", "artist")] { - for id in params.uuids(key)? { + for (ids, kind) in [(albums, "album"), (artists, "artist")] { + for id in ids { state .services .set_star(principal.id, kind, id, starred) @@ -137,6 +155,9 @@ pub(super) async fn scrobble( if ids.is_empty() { return Err(missing()); } + if ids.len() > MAX_IDENTIFIERS { + return Err(invalid("Too many identifiers in one request")); + } let times = params .all("time") .iter() diff --git a/tests/v2_foundations.rs b/tests/v2_foundations.rs index fc80f28..41bc09f 100644 --- a/tests/v2_foundations.rs +++ b/tests/v2_foundations.rs @@ -3342,6 +3342,11 @@ fn catalog_input(index: usize, artist: &str) -> CatalogTrackInput { isrc: None, moods: None, explicit_status: None, + original_release_date: None, + release_date: None, + release_types: None, + record_labels: None, + disc_subtitle: None, artwork: None, lyrics_hash: blake3::hash(b"").to_hex().to_string(), lyrics: Vec::new(), @@ -6333,6 +6338,11 @@ fn browse_input( isrc: None, moods: None, explicit_status: None, + original_release_date: None, + release_date: None, + release_types: None, + record_labels: None, + disc_subtitle: None, artwork: None, lyrics_hash: blake3::hash(b"").to_hex().to_string(), lyrics: Vec::new(), @@ -9871,7 +9881,14 @@ async fn a_contributor_is_not_one_of_the_track_artists() { // track carries thirteen roles now where it carried one list of names. let by_title = state .services - .catalog_search(owner, &[], "Only Track") + .catalog_search( + owner, + &[], + "Only Track", + waveflow_server::services::BrowsePage::default(), + waveflow_server::services::BrowsePage::default(), + waveflow_server::services::BrowsePage::default(), + ) .await .unwrap(); assert_eq!( @@ -9893,7 +9910,14 @@ async fn a_contributor_is_not_one_of_the_track_artists() { // whole point of indexing artists rather than deriving them. let by_name = state .services - .catalog_search(owner, &[], "Rita") + .catalog_search( + owner, + &[], + "Rita", + waveflow_server::services::BrowsePage::default(), + waveflow_server::services::BrowsePage::default(), + waveflow_server::services::BrowsePage::default(), + ) .await .unwrap(); assert_eq!( @@ -10477,3 +10501,546 @@ async fn a_changed_track_spec_drops_every_relocation_hint_without_a_rescan() { "clearing a hint must not disturb what it points at" ); } + +#[tokio::test] +async fn a_changed_artist_spec_carries_the_favourite_onto_the_new_identifier() { + let (_temp, config, state) = test_app().await; + let hash = security::hash_password("correct horse battery staple").unwrap(); + let owner = state + .db + .create_account("remap", &hash, AccountRole::Admin, now_ms()) + .await + .unwrap(); + let music = config.data_dir.join("remap-music"); + std::fs::create_dir_all(&music).unwrap(); + let library_id = state + .db + .create_library( + owner, + "Remap library", + &std::fs::canonicalize(&music).unwrap(), + LibraryVisibility::Private, + now_ms(), + ) + .await + .unwrap(); + let scan_id = state + .db + .create_scan_job(library_id, Some(owner), "manual") + .await + .unwrap(); + state.db.start_scan_job(scan_id, 1, false).await.unwrap(); + state + .db + .apply_catalog_track( + library_id, + scan_id, + &catalog_input(0, "Nova Kern"), + None, + false, + ) + .await + .unwrap(); + // The rules the catalogue was written under are recorded when the scan + // completes, which is what a later boot compares against. + state.db.finish_scan_job(scan_id, 0).await.unwrap(); + + let artist_id: String = + sqlx::query_scalar("SELECT id FROM artist WHERE library_id = ? AND name = 'Nova Kern'") + .bind(library_id.to_string()) + .fetch_one(state.db.pool()) + .await + .unwrap(); + let artist_id = uuid::Uuid::parse_str(&artist_id).unwrap(); + state + .services + .set_star(owner, "artist", artist_id, true) + .await + .unwrap(); + + let altered = waveflow_server::pid::PidSpecs { + album: config.pid.album.clone(), + track: config.pid.track.clone(), + artist: waveflow_server::pid::PidSpec::parse("albumartistid,title", false).unwrap(), + }; + let expected = altered.artist_id(library_id, "Nova Kern"); + assert_ne!( + expected, artist_id, + "the altered spec has to actually move the identifier for this to test anything" + ); + + let libraries = state.db.reconcile_catalog_identity(&altered).await.unwrap(); + assert_eq!( + libraries, 1, + "an artist spec change still re-identifies, and still costs a full rescan" + ); + + let starred: Vec = sqlx::query_scalar( + "SELECT entity_id FROM user_star WHERE user_id = ? AND entity_type = 'artist'", + ) + .bind(owner.to_string()) + .fetch_all(state.db.pool()) + .await + .unwrap(); + assert_eq!( + starred, + vec![expected.to_string()], + "the favourite must name the identifier the new rule derives, not the one it replaced" + ); +} + +#[tokio::test] +async fn an_album_reports_its_release_details_and_its_disc_titles() { + let (_temp, config, state) = test_app().await; + let router = waveflow_server::app(&config, state.clone()); + let hash = security::hash_password("correct horse battery staple").unwrap(); + let owner = state + .db + .create_account("release", &hash, AccountRole::Admin, now_ms()) + .await + .unwrap(); + let encrypted = state + .secret_box + .encrypt(b"dedicated-subsonic-secret") + .unwrap(); + let api_key = "wfsk_release-key"; + state + .db + .set_subsonic_credential( + owner, + owner, + &encrypted, + &security::token_hash(api_key), + now_ms(), + ) + .await + .unwrap(); + let music = config.data_dir.join("release-music"); + std::fs::create_dir_all(&music).unwrap(); + let library_id = state + .db + .create_library( + owner, + "Release library", + &std::fs::canonicalize(&music).unwrap(), + LibraryVisibility::Private, + now_ms(), + ) + .await + .unwrap(); + let scan_id = state + .db + .create_scan_job(library_id, Some(owner), "manual") + .await + .unwrap(); + state.db.start_scan_job(scan_id, 2, false).await.unwrap(); + + for (index, disc, subtitle) in [(0usize, 1i64, "The Session"), (1, 2, "The Rehearsal")] { + let mut input = catalog_input(index, "Nova Kern"); + input.disc_number = Some(disc); + input.disc_subtitle = Some(subtitle.into()); + if index == 0 { + // Only the first track carries them: the album takes the first + // value it is given and later tracks do not overwrite it. + input.original_release_date = Some("1998-11".into()); + input.release_date = Some("2019-04-05".into()); + input.release_types = Some("Album; Compilation".into()); + input.record_labels = Some("Nightfall Records; Second Imprint".into()); + } + state + .db + .apply_catalog_track(library_id, scan_id, &input, None, false) + .await + .unwrap(); + } + state.db.finish_scan_job(scan_id, 0).await.unwrap(); + + let album_id = state + .services + .catalog_snapshot(owner, &[]) + .await + .unwrap() + .albums[0] + .id; + let detail = state.services.album(owner, album_id).await.unwrap(); + assert_eq!( + detail.album.record_labels, + vec!["Nightfall Records", "Second Imprint"] + ); + assert_eq!(detail.album.release_types, vec!["Album", "Compilation"]); + assert_eq!( + detail + .album + .disc_titles + .iter() + .map(|disc| (disc.disc, disc.title.as_str())) + .collect::>(), + vec![(1, "The Session"), (2, "The Rehearsal")] + ); + + let album = subsonic_json(&router, "getAlbum", api_key, &format!("&id={album_id}")).await; + let album = &album["subsonic-response"]["album"]; + assert_eq!( + album["recordLabels"], + serde_json::json!([{"name": "Nightfall Records"}, {"name": "Second Imprint"}]) + ); + assert_eq!( + album["releaseTypes"], + serde_json::json!(["Album", "Compilation"]) + ); + assert_eq!( + album["discTitles"], + serde_json::json!([ + {"disc": 1, "title": "The Session"}, + {"disc": 2, "title": "The Rehearsal"} + ]) + ); + // A tag naming a year and a month is a year and a month. Reporting a day + // it never claimed would be inventing precision. + assert_eq!( + album["originalReleaseDate"], + serde_json::json!({"year": 1998, "month": 11}) + ); + assert_eq!( + album["releaseDate"], + serde_json::json!({"year": 2019, "month": 4, "day": 5}) + ); + + // An album with none of these tags declares the fields supported and + // unset — empty arrays rather than absent keys — and names no date at all. + let bare_scan = state + .db + .create_scan_job(library_id, Some(owner), "manual") + .await + .unwrap(); + state.db.start_scan_job(bare_scan, 1, false).await.unwrap(); + let mut bare = catalog_input(7, "Quiet Hand"); + bare.album = Some("Bare release".into()); + bare.is_compilation = false; + state + .db + .apply_catalog_track(library_id, bare_scan, &bare, None, false) + .await + .unwrap(); + state.db.finish_scan_job(bare_scan, 0).await.unwrap(); + let bare_id = state + .services + .catalog_snapshot(owner, &[]) + .await + .unwrap() + .albums + .into_iter() + .find(|album| album.title == "Bare release") + .unwrap() + .id; + let bare = subsonic_json(&router, "getAlbum", api_key, &format!("&id={bare_id}")).await; + let bare = &bare["subsonic-response"]["album"]; + assert_eq!(bare["recordLabels"], serde_json::json!([])); + assert_eq!(bare["releaseTypes"], serde_json::json!([])); + assert_eq!(bare["discTitles"], serde_json::json!([])); + assert!(bare["originalReleaseDate"].is_null()); + assert!(bare["releaseDate"].is_null()); +} + +#[tokio::test] +async fn search_pages_each_kind_in_sql_and_bounds_what_one_request_may_name() { + let (_temp, config, state) = test_app().await; + let router = waveflow_server::app(&config, state.clone()); + let hash = security::hash_password("correct horse battery staple").unwrap(); + let owner = state + .db + .create_account("pager", &hash, AccountRole::Admin, now_ms()) + .await + .unwrap(); + let encrypted = state + .secret_box + .encrypt(b"dedicated-subsonic-secret") + .unwrap(); + let api_key = "wfsk_pager-key"; + state + .db + .set_subsonic_credential( + owner, + owner, + &encrypted, + &security::token_hash(api_key), + now_ms(), + ) + .await + .unwrap(); + let music = config.data_dir.join("pager-music"); + std::fs::create_dir_all(&music).unwrap(); + let library_id = state + .db + .create_library( + owner, + "Pager library", + &std::fs::canonicalize(&music).unwrap(), + LibraryVisibility::Private, + now_ms(), + ) + .await + .unwrap(); + let scan_id = state + .db + .create_scan_job(library_id, Some(owner), "manual") + .await + .unwrap(); + state.db.start_scan_job(scan_id, 5, false).await.unwrap(); + // One token every kind matches on, so the three pages are exercised by one + // query rather than by three that happen not to overlap. + for (index, name) in ["Aria", "Bela", "Cyd", "Dara", "Eno"] + .into_iter() + .enumerate() + { + let mut input = catalog_input(index, &format!("Nocturne {name}")); + input.title = format!("Nocturne {index}"); + state + .db + .apply_catalog_track(library_id, scan_id, &input, None, false) + .await + .unwrap(); + } + state.db.finish_scan_job(scan_id, 0).await.unwrap(); + // What a real scan does after applying its rows, and what builds the + // artist search index this query has to reach. + state + .db + .consolidate_catalog_derivations(library_id) + .await + .unwrap(); + + let paged = subsonic_json( + &router, + "search3", + api_key, + "&query=Nocturne&songCount=2&songOffset=1&artistCount=1&artistOffset=2&albumCount=5&albumOffset=1", + ) + .await; + let result = &paged["subsonic-response"]["searchResult3"]; + assert_eq!( + result["song"] + .as_array() + .unwrap() + .iter() + .map(|song| song["title"].as_str().unwrap()) + .collect::>(), + vec!["Nocturne 1", "Nocturne 2"], + "the song offset has to skip in SQL and still land on the same rows" + ); + assert_eq!( + result["artist"] + .as_array() + .unwrap() + .iter() + .map(|artist| artist["name"].as_str().unwrap()) + .collect::>(), + vec!["Nocturne Cyd"], + "each kind pages independently, which is what search3 has always allowed" + ); + // Five tracks, one album between them: an offset of one leaves nothing, and + // `searchResult3` omits a kind it has no rows for rather than sending `[]`. + assert!(result["album"].is_null()); + + // A request may not name more identifiers than the queue may hold: each one + // costs a mutation under the process-wide writer gate. + let track_id = state + .services + .catalog_snapshot(owner, &[]) + .await + .unwrap() + .songs[0] + .id; + let oversized = (0..=waveflow_server::services::MAX_QUEUE_TRACKS) + .map(|_| format!("&id={track_id}")) + .collect::(); + let refused = subsonic_json(&router, "star", api_key, &oversized).await; + assert_eq!(refused["subsonic-response"]["status"], "failed"); + assert_eq!(refused["subsonic-response"]["error"]["code"], 10); + // And one below the ceiling still works. + let accepted = subsonic_json(&router, "star", api_key, &format!("&id={track_id}")).await; + assert_eq!(accepted["subsonic-response"]["status"], "ok"); + + // A playlist is bounded on what it holds rather than on what one request + // carries, because it grows across many of them. + let too_many = vec![track_id; waveflow_server::services::MAX_PLAYLIST_TRACKS + 1]; + assert!(matches!( + state + .services + .create_playlist(owner, "Oversized", &too_many) + .await, + Err(ServiceError::Invalid) + )); +} + +/// Fixture for the two remap shapes: a library whose recorded artist spec a +/// later boot can be shown to disagree with. +async fn remap_fixture( + state: &waveflow_server::AppState, + config: &Config, + label: &str, +) -> (uuid::Uuid, uuid::Uuid) { + let hash = security::hash_password("correct horse battery staple").unwrap(); + let owner = state + .db + .create_account(label, &hash, AccountRole::Admin, now_ms()) + .await + .unwrap(); + let music = config.data_dir.join(format!("{label}-music")); + std::fs::create_dir_all(&music).unwrap(); + let library_id = state + .db + .create_library( + owner, + "Remap library", + &std::fs::canonicalize(&music).unwrap(), + LibraryVisibility::Private, + now_ms(), + ) + .await + .unwrap(); + let scan_id = state + .db + .create_scan_job(library_id, Some(owner), "manual") + .await + .unwrap(); + state.db.start_scan_job(scan_id, 1, false).await.unwrap(); + state + .db + .apply_catalog_track( + library_id, + scan_id, + &catalog_input(0, "Seed Artist"), + None, + false, + ) + .await + .unwrap(); + state.db.finish_scan_job(scan_id, 0).await.unwrap(); + (owner, library_id) +} + +async fn seed_artist( + state: &waveflow_server::AppState, + library_id: uuid::Uuid, + id: uuid::Uuid, + name: &str, +) { + sqlx::query( + "INSERT INTO artist (id, library_id, name, canonical_name, created_at, updated_at) \ + VALUES (?, ?, ?, ?, ?, ?)", + ) + .bind(id.to_string()) + .bind(library_id.to_string()) + .bind(name) + .bind(name.to_lowercase().replace(' ', "")) + .bind(now_ms()) + .bind(now_ms()) + .execute(state.db.pool()) + .await + .unwrap(); +} + +async fn seed_star( + state: &waveflow_server::AppState, + owner: uuid::Uuid, + entity_id: uuid::Uuid, + starred_at: i64, +) { + sqlx::query( + "INSERT INTO user_star (user_id, entity_type, entity_id, starred_at) \ + VALUES (?, 'artist', ?, ?)", + ) + .bind(owner.to_string()) + .bind(entity_id.to_string()) + .bind(starred_at) + .execute(state.db.pool()) + .await + .unwrap(); +} + +async fn artist_stars(state: &waveflow_server::AppState, owner: uuid::Uuid) -> Vec<(String, i64)> { + let mut rows: Vec<(String, i64)> = sqlx::query_as( + "SELECT entity_id, starred_at FROM user_star WHERE user_id = ? AND entity_type = 'artist'", + ) + .bind(owner.to_string()) + .fetch_all(state.db.pool()) + .await + .unwrap(); + rows.sort(); + rows +} + +#[tokio::test] +async fn a_remap_whose_targets_chain_does_not_change_who_owns_what() { + let (_temp, config, state) = test_app().await; + let (owner, library_id) = remap_fixture(&state, &config, "chained").await; + + let altered = waveflow_server::pid::PidSpecs { + album: config.pid.album.clone(), + track: config.pid.track.clone(), + artist: waveflow_server::pid::PidSpec::parse("albumartistid,title", false).unwrap(), + }; + // Built by hand: no spec this engine can parse makes one artist's new + // identifier another's old one, because only `albumartistid` carries a + // value for an artist. The property still has to hold, because the code + // cannot see that and a wider `PidSource` would make it reachable. + let first_row = uuid::Uuid::new_v4(); + let first_new = altered.artist_id(library_id, "Chain One"); + let second_new = altered.artist_id(library_id, "Chain Two"); + assert_ne!(first_row, first_new); + assert_ne!(first_new, second_new); + seed_artist(&state, library_id, first_row, "Chain One").await; + // Its row id is the identifier the first artist is about to move onto. + seed_artist(&state, library_id, first_new, "Chain Two").await; + seed_star(&state, owner, first_row, 111).await; + seed_star(&state, owner, first_new, 222).await; + + state.db.reconcile_catalog_identity(&altered).await.unwrap(); + + let mut expected = vec![(first_new.to_string(), 111), (second_new.to_string(), 222)]; + expected.sort(); + assert_eq!( + artist_stars(&state, owner).await, + expected, + "each favourite has to land on its own artist's new identifier, whatever \ + order the rows came back in" + ); +} + +#[tokio::test] +async fn two_artists_folding_onto_one_identifier_keep_a_single_favourite() { + let (_temp, config, state) = test_app().await; + let (owner, library_id) = remap_fixture(&state, &config, "folded").await; + + // `title` names nothing for an artist, so every artist evaluates to the + // same empty string and the whole library folds onto one identifier. A + // degenerate spec, and exactly the shape the collision handling is for. + let altered = waveflow_server::pid::PidSpecs { + album: config.pid.album.clone(), + track: config.pid.track.clone(), + artist: waveflow_server::pid::PidSpec::parse("title", false).unwrap(), + }; + let folded = altered.artist_id(library_id, "Fold One"); + assert_eq!(folded, altered.artist_id(library_id, "Fold Two")); + + let first = uuid::Uuid::new_v4(); + let second = uuid::Uuid::new_v4(); + seed_artist(&state, library_id, first, "Fold One").await; + seed_artist(&state, library_id, second, "Fold Two").await; + seed_star(&state, owner, first, 111).await; + seed_star(&state, owner, second, 222).await; + + state.db.reconcile_catalog_identity(&altered).await.unwrap(); + + let stars = artist_stars(&state, owner).await; + assert_eq!( + stars.len(), + 1, + "the two rows collide on the primary key and one is dropped: {stars:?}" + ); + assert_eq!(stars[0].0, folded.to_string()); + // The seed artist folds onto the same identifier, so nothing is left + // pointing at an identifier no projection answers for. + assert!(!stars[0].0.contains("pid-remap:")); +}