From 0b26a3e09f4548ceebcf3840496dd162258a6a5f Mon Sep 17 00:00:00 2001 From: InstaZDLL Date: Mon, 24 Aug 2026 17:01:30 +0200 Subject: [PATCH 1/4] feat(catalog): carry artist favourites across a change of artist spec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The second named follow-up. `user_star` and `user_rating` hold an untyped identifier with no foreign key, so a rescan that re-derives artist ids left 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 documentation said so plainly, which was the honest position until it stopped being necessary. `reconcile_catalog_identity` now remaps them when `pid.artist` changed, before requesting the rescan: the old identifier and the name that produced it are both still on the `artist` row at that moment, and that is the only moment the two can be paired. `UPDATE OR IGNORE` then `DELETE` rather than a plain update, because a coarser spec can fold two artists onto one identifier and the second row would collide on `(user_id, entity_type, entity_id)`. Dropping the duplicate is right: the user already stars what it would have become. 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 to derive the new identifier from until the scan that has already discarded the old one. Signed-off-by: InstaZDLL --- src/catalog.rs | 80 +++++++++++++++++++++++++++++++++++++ tests/v2_foundations.rs | 87 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 167 insertions(+) diff --git a/src/catalog.rs b/src/catalog.rs index 1284a18..d4ebd3c 100644 --- a/src/catalog.rs +++ b/src/catalog.rs @@ -366,6 +366,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 +427,75 @@ 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. + /// + /// `UPDATE OR IGNORE` then `DELETE` rather than a plain update: 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 _writer = self.writer_guard().await; + let mut tx = self.pool().begin().await?; + let mut moved = 0; + 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 { + continue; + } + // 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. + moved += sqlx::query( + "UPDATE OR IGNORE user_star SET entity_id = ? \ + WHERE entity_type = 'artist' AND entity_id = ?", + ) + .bind(&new) + .bind(&old) + .execute(&mut *tx) + .await? + .rows_affected(); + sqlx::query("DELETE FROM user_star WHERE entity_type = 'artist' AND entity_id = ?") + .bind(&old) + .execute(&mut *tx) + .await?; + moved += sqlx::query( + "UPDATE OR IGNORE user_rating SET entity_id = ? \ + WHERE entity_type = 'artist' AND entity_id = ?", + ) + .bind(&new) + .bind(&old) + .execute(&mut *tx) + .await? + .rows_affected(); + sqlx::query("DELETE FROM user_rating WHERE entity_type = 'artist' AND entity_id = ?") + .bind(&old) + .execute(&mut *tx) + .await?; + } + tx.commit().await?; + Ok(moved) + } + pub async fn library_for_user( &self, user_id: Uuid, diff --git a/tests/v2_foundations.rs b/tests/v2_foundations.rs index fc80f28..cc1fd64 100644 --- a/tests/v2_foundations.rs +++ b/tests/v2_foundations.rs @@ -10477,3 +10477,90 @@ 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" + ); +} From b178875dc1ebf656f49f6c59d83b5f2564b39445 Mon Sep 17 00:00:00 2001 From: InstaZDLL Date: Mon, 24 Aug 2026 17:13:46 +0200 Subject: [PATCH 2/4] feat(subsonic): answer the album output fields that were still absent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The third item on the handoff list. `originalReleaseDate`, `releaseDate`, `releaseTypes[]`, `recordLabels[]` and `discTitles[]` were declared absent under the presence rule, which was honest and is no longer necessary. The first four describe the release rather than the recording, so they sit on the album and fill the way `year` and `musicbrainz_id` already do: the first track to carry a value writes it, later tracks do not overwrite it. `discTitles[]` holds one title per disc, so the tag lands on the track and the album derives the list from its available tracks — the same batch the genres and the credits already come from, grouped per disc with `MIN` so an album whose files were tagged by different hands does not report a disc twice. `LABEL` first and `PUBLISHER` second: the one Picard writes and the one the same value arrives under from everything else. A tag written as several items is joined here and split again by the helper that already splits `moods`. The dates are stored as the file spelled them and taken apart only at the wire, emitting nothing the tag did not claim — `1998-11` is a year and a month, not the first of November read as a full date. They are omitted when unknown, as the reference omits them; the three arrays are emitted empty rather than absent, so the group still declares itself supported. That last part needed the JSON injection guard widened: it lists per entry shape which arrays may be filled in, and an album was allowed only `artists` and `genres`, so the three new ones came back absent on an album carrying none of the tags. The wire moved. Under §4 of the handoff the four clients want replaying before a stable tag. Signed-off-by: InstaZDLL --- docs/opensubsonic-gap-analysis.md | 24 +-- .../20260824010000_album_release_details.sql | 19 ++ src/catalog.rs | 35 +++- src/scanner.rs | 34 ++++ src/services/mod.rs | 57 ++++++ src/subsonic/nodes.rs | 57 ++++++ src/subsonic/protocol.rs | 14 +- tests/v2_foundations.rs | 163 ++++++++++++++++++ 8 files changed, 386 insertions(+), 17 deletions(-) create mode 100644 migrations-v2/20260824010000_album_release_details.sql diff --git a/docs/opensubsonic-gap-analysis.md b/docs/opensubsonic-gap-analysis.md index 6aaf3e0..4a7c08e 100644 --- a/docs/opensubsonic-gap-analysis.md +++ b/docs/opensubsonic-gap-analysis.md @@ -124,10 +124,15 @@ qui ne se manifeste qu'en production, chez un utilisateur, une seule fois. > la plus lourde. La PR #126 a livré `roles[]`, `contributors[]` et > `displayComposer` avec les colonnes qu'ils réclamaient — treize rôles, un > sous-rôle d'instrument, et un album qui pend de chacun de ses artistes -> crédités. `sortName` était déjà arrivé avec la PR #123. Restent de ce point -> les champs de sortie d'album (`originalReleaseDate`, `releaseDate`, -> `releaseTypes[]`, `recordLabels[]`, `discTitles[]`), toujours absents et -> toujours honnêtement déclarés tels. +> crédités. `sortName` était déjà arrivé avec la PR #123. Les champs de sortie +> d'album +> (`originalReleaseDate`, `releaseDate`, `releaseTypes[]`, `recordLabels[]`, +> `discTitles[]`) ont suivi le 24 août 2026 : les quatre premiers pendent de +> l'album, `discTitles[]` se dérive des pistes disponibles comme les genres, et +> les trois tableaux sont émis vides plutôt qu'absents. 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. **Ce point est clos.** > > La question de cadrage du §5.1 est close : les quatre clients ont été > rejoués le 23 août 2026 contre le modèle aligné, et @@ -139,12 +144,11 @@ 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..003c129 --- /dev/null +++ b/migrations-v2/20260824010000_album_release_details.sql @@ -0,0 +1,19 @@ +-- 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 the way `year` and `musicbrainz_id` already are: the first track +-- to carry a value writes it, and later tracks do not overwrite it. +-- +-- `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 d4ebd3c..1778bd5 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, @@ -1198,10 +1210,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, \ @@ -1222,7 +1235,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", ) @@ -1244,6 +1258,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?; @@ -1735,8 +1750,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, \ @@ -1744,6 +1761,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(album.original_release_date, excluded.original_release_date), \ + release_date=COALESCE(album.release_date, excluded.release_date), \ + release_types=COALESCE(album.release_types, excluded.release_types), \ + record_labels=COALESCE(album.record_labels, excluded.record_labels), \ updated_at=excluded.updated_at", ) .bind(id.to_string()) @@ -1755,6 +1776,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) 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/mod.rs b/src/services/mod.rs index 515fe64..c66bb6a 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, @@ -832,9 +850,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 +1121,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/subsonic/nodes.rs b/src/subsonic/nodes.rs index 845eac8..6f3966b 100644 --- a/src/subsonic/nodes.rs +++ b/src/subsonic/nodes.rs @@ -78,6 +78,63 @@ 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. +fn item_date(name: &'static str, raw: Option<&str>) -> Option { + let raw = raw?.trim(); + let mut parts = raw.split(['-', '/', '.']); + let year: i64 = parts.next()?.trim().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..84d965e 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 { @@ -131,7 +134,13 @@ pub(super) fn json_required_array_fields(parent: &str, name: &str) -> &'static [ "albumArtists", "contributors", ], - "album" => &["artists", "genres"], + "album" => &[ + "artists", + "genres", + "recordLabels", + "releaseTypes", + "discTitles", + ], // 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 +214,7 @@ 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") + | ("album", "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/tests/v2_foundations.rs b/tests/v2_foundations.rs index cc1fd64..559081a 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(), @@ -10564,3 +10574,156 @@ async fn a_changed_artist_spec_carries_the_favourite_onto_the_new_identifier() { "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()); +} From 48156274d1e8cbf399a5cd4a69f911d1171b73f6 Mon Sep 17 00:00:00 2001 From: InstaZDLL Date: Mon, 24 Aug 2026 17:28:27 +0200 Subject: [PATCH 3/4] perf: close the four findings deferred from the module split MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deferred at the time because each was a rewrite rather than a minimal fix, and the split's value was being readable as movement. **Two N+1 reads.** `playlists_on` ran two queries per playlist, so an account with fifty of them paid a hundred round trips to answer `getPlaylists`; `now_playing` resolved one track per row. Both now read their identifiers in one query and resolve the union in one more. The per-playlist order and the dropping of a track the account cannot see are reapplied from the batch, exactly as the lenient resolver applied them. **`search3` paged in memory.** The match-all branch already paged in SQL; the FTS branch read every matching artist, album and song to hand back twenty of each. `catalog_search` now takes the three pages and applies them as `LIMIT`/`OFFSET`, and the renderer no longer skips — the same division the match-all branch makes. The artist ordering gains `ar.id` as a tie-break, without which a page boundary is not stable. **Unbounded identifier lists.** `star`, `unstar` and `scrobble` looped over whatever the request named, one writer-gate mutation each, and the form body admits some fifteen hundred UUIDs. One shared bound across `id`, `albumId` and `artistId`, checked before any of them is applied so a refused request does not leave the first few starred. **Unbounded playlists.** `MAX_PLAYLIST_TRACKS` is deliberately not the queue's 400: that bounds a request, and a queue is rewritten whole by every call, whereas a playlist grows across many. What needed bounding is the rewrite — `replace_playlist_tracks` deletes and reinserts the whole list under the writer gate — so the ceiling is ten thousand, far above any hand-curated playlist. One test covers the paging and both bounds; the paging half was confirmed to fail with the SQL page removed, returning the first row instead of the second. Signed-off-by: InstaZDLL --- src/services/catalog.rs | 21 +++++- src/services/mod.rs | 9 +++ src/services/playback.rs | 32 +++++--- src/services/playlists.rs | 59 ++++++++++++++- src/subsonic/browse.rs | 22 ++++-- src/subsonic/userdata.rs | 27 ++++++- tests/v2_foundations.rs | 149 +++++++++++++++++++++++++++++++++++++- 7 files changed, 294 insertions(+), 25 deletions(-) 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 c66bb6a..93beaa1 100644 --- a/src/services/mod.rs +++ b/src/services/mod.rs @@ -369,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. 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..4c8994b 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) @@ -124,6 +165,12 @@ impl DomainServices { track_ids: &[Uuid], context: MutationContext, ) -> Result { + // Refused before the writer gate rather than after the claim: an + // oversized request is refused on its own terms, without queuing behind + // a scan for the right to be told so. + if track_ids.len() > MAX_PLAYLIST_TRACKS { + return Err(ServiceError::Invalid); + } let intent = MutationIntent::new( "create", "playlist", @@ -260,6 +307,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/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 559081a..e7d96b5 100644 --- a/tests/v2_foundations.rs +++ b/tests/v2_foundations.rs @@ -9881,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!( @@ -9903,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!( @@ -10727,3 +10741,134 @@ async fn an_album_reports_its_release_details_and_its_disc_titles() { 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) + )); +} From 2aa522f49d08c39d1472296719a7a04864fcc093 Mon Sep 17 00:00:00 2001 From: InstaZDLL Date: Mon, 24 Aug 2026 18:36:03 +0200 Subject: [PATCH 4/4] fix: seven review findings on the handoff follow-ups MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **The remap depended on the order its rows came back in.** One artist's new identifier can be another's old one, and moving them one at a time then carried the first artist's favourite onto the second's row — in the test that reproduces it, the first favourite vanishes entirely. Every pair is now computed first and moved in two phases through a namespace no identifier can occupy, in one transaction, so the outcome is the same whatever order the rows arrived in. Two tests: identifiers that chain, and two artists folding onto one. No spec this engine can parse actually produces a chain, because only `albumartistid` carries a value for an artist — the chained test builds the rows by hand and says so. The property still has to hold: the code cannot see that argument, and a wider `PidSource` would make it reachable. **A rescan could not correct an album's release tags.** The four new columns used `COALESCE(album.x, excluded.x)`, so the first value the catalogue ever saw held against every later scan. They now match `year` and `artwork_hash`, where the incoming value wins. **An album rendered as a folder child lost its array shape.** `getMusicDirectory` renames the node to `child`, and both the array rule and the injection guard are keyed on that name, so one record label collapsed into a bare object and an empty list came back absent — "unsupported" — under a name where `getAlbum` says otherwise. **`item_date` read any leading integer as a year.** `19980405` written without separators became the year 19,980,405 and a bare `5` the year 5. The head must be exactly four ASCII digits or it is no date. **Both playlist bounds sat ahead of the replay branch.** A replay owes its 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 answer an error to its own retry. Both checks move behind the replay branch and stay ahead of every read and write — which is also where the review asked for the second one, minus the writer gate it cannot sit ahead of without reintroducing the first problem. **The album fields were recorded under the previous day's addendum.** The 2026-08-23 entry is restored to what it said on the 23rd, and the delivery gets an addendum of its own. Signed-off-by: InstaZDLL --- docs/opensubsonic-gap-analysis.md | 26 ++- .../20260824010000_album_release_details.sql | 6 +- src/catalog.rs | 123 ++++++++----- src/services/playlists.rs | 21 ++- src/subsonic/nodes.rs | 9 +- src/subsonic/protocol.rs | 9 +- tests/v2_foundations.rs | 172 ++++++++++++++++++ 7 files changed, 304 insertions(+), 62 deletions(-) diff --git a/docs/opensubsonic-gap-analysis.md b/docs/opensubsonic-gap-analysis.md index 4a7c08e..e253c3b 100644 --- a/docs/opensubsonic-gap-analysis.md +++ b/docs/opensubsonic-gap-analysis.md @@ -124,20 +124,28 @@ qui ne se manifeste qu'en production, chez un utilisateur, une seule fois. > la plus lourde. La PR #126 a livré `roles[]`, `contributors[]` et > `displayComposer` avec les colonnes qu'ils réclamaient — treize rôles, un > sous-rôle d'instrument, et un album qui pend de chacun de ses artistes -> crédités. `sortName` était déjà arrivé avec la PR #123. Les champs de sortie -> d'album -> (`originalReleaseDate`, `releaseDate`, `releaseTypes[]`, `recordLabels[]`, -> `discTitles[]`) ont suivi le 24 août 2026 : les quatre premiers pendent de -> l'album, `discTitles[]` se dérive des pistes disponibles comme les genres, et -> les trois tableaux sont émis vides plutôt qu'absents. 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. **Ce point est clos.** +> crédités. `sortName` était déjà arrivé avec la PR #123. Restent de ce point +> les champs de sortie d'album (`originalReleaseDate`, `releaseDate`, +> `releaseTypes[]`, `recordLabels[]`, `discTitles[]`), toujours absents et +> toujours honnêtement déclarés tels. > > La question de cadrage du §5.1 est close : les quatre clients ont été > 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. diff --git a/migrations-v2/20260824010000_album_release_details.sql b/migrations-v2/20260824010000_album_release_details.sql index 003c129..647fb91 100644 --- a/migrations-v2/20260824010000_album_release_details.sql +++ b/migrations-v2/20260824010000_album_release_details.sql @@ -2,8 +2,10 @@ -- -- `originalReleaseDate`, `releaseDate`, `releaseTypes[]` and `recordLabels[]` -- describe the release rather than the recording, so they sit on the album and --- are filled the way `year` and `musicbrainz_id` already are: the first track --- to carry a value writes it, and later tracks do not overwrite it. +-- 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 diff --git a/src/catalog.rs b/src/catalog.rs index 1778bd5..0af23eb 100644 --- a/src/catalog.rs +++ b/src/catalog.rs @@ -452,10 +452,18 @@ impl Database { /// and `releasedate`, which live on the files rather than on the album row, /// so there is nothing here to derive the new identifier from. /// - /// `UPDATE OR IGNORE` then `DELETE` rather than a plain update: 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. + /// **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, @@ -463,46 +471,35 @@ impl Database { let rows = sqlx::query("SELECT id, library_id, name FROM artist") .fetch_all(self.pool()) .await?; - let _writer = self.writer_guard().await; - let mut tx = self.pool().begin().await?; - let mut moved = 0; + 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 { - continue; + 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; } - // 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. - moved += sqlx::query( - "UPDATE OR IGNORE user_star SET entity_id = ? \ - WHERE entity_type = 'artist' AND entity_id = ?", - ) - .bind(&new) - .bind(&old) - .execute(&mut *tx) - .await? - .rows_affected(); - sqlx::query("DELETE FROM user_star WHERE entity_type = 'artist' AND entity_id = ?") - .bind(&old) - .execute(&mut *tx) - .await?; - moved += sqlx::query( - "UPDATE OR IGNORE user_rating SET entity_id = ? \ - WHERE entity_type = 'artist' AND entity_id = ?", - ) - .bind(&new) - .bind(&old) - .execute(&mut *tx) - .await? - .rows_affected(); - sqlx::query("DELETE FROM user_rating WHERE entity_type = 'artist' AND entity_id = ?") - .bind(&old) - .execute(&mut *tx) - .await?; } tx.commit().await?; Ok(moved) @@ -1761,10 +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(album.original_release_date, excluded.original_release_date), \ - release_date=COALESCE(album.release_date, excluded.release_date), \ - release_types=COALESCE(album.release_types, excluded.release_types), \ - record_labels=COALESCE(album.record_labels, excluded.record_labels), \ + 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()) @@ -1796,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/services/playlists.rs b/src/services/playlists.rs index 4c8994b..be6b8a3 100644 --- a/src/services/playlists.rs +++ b/src/services/playlists.rs @@ -165,12 +165,6 @@ impl DomainServices { track_ids: &[Uuid], context: MutationContext, ) -> Result { - // Refused before the writer gate rather than after the claim: an - // oversized request is refused on its own terms, without queuing behind - // a scan for the right to be told so. - if track_ids.len() > MAX_PLAYLIST_TRACKS { - return Err(ServiceError::Invalid); - } let intent = MutationIntent::new( "create", "playlist", @@ -189,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(); @@ -290,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)?; diff --git a/src/subsonic/nodes.rs b/src/subsonic/nodes.rs index 6f3966b..5b4883d 100644 --- a/src/subsonic/nodes.rs +++ b/src/subsonic/nodes.rs @@ -114,11 +114,16 @@ pub(super) fn album_node(album: &AlbumItem) -> Node { /// 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. +/// 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 year: i64 = parts.next()?.trim().parse().ok().filter(|y| *y > 0)?; + 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()) diff --git a/src/subsonic/protocol.rs b/src/subsonic/protocol.rs index 84d965e..70cd24b 100644 --- a/src/subsonic/protocol.rs +++ b/src/subsonic/protocol.rs @@ -133,6 +133,9 @@ pub(super) fn json_required_array_fields(parent: &str, name: &str) -> &'static [ "moods", "albumArtists", "contributors", + "recordLabels", + "releaseTypes", + "discTitles", ], "album" => &[ "artists", @@ -214,7 +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") - | ("album", "recordLabels" | "releaseTypes" | "discTitles") + // `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/tests/v2_foundations.rs b/tests/v2_foundations.rs index e7d96b5..41bc09f 100644 --- a/tests/v2_foundations.rs +++ b/tests/v2_foundations.rs @@ -10872,3 +10872,175 @@ async fn search_pages_each_kind_in_sql_and_bounds_what_one_request_may_name() { 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:")); +}