From 2531876c0f0042157a25e0bb4f599a6b456a3b2f Mon Sep 17 00:00:00 2001 From: InstaZDLL Date: Thu, 15 Oct 2026 21:36:28 +0200 Subject: [PATCH 1/2] feat(enrichment): getArtistInfo and the native artist page read the cache Step D of the RFC-011 plan. - `getArtistInfo` and `getArtistInfo2` resolve the artist first and for its refusal, then answer from the cache: `biography`, and the `similarArtist` entries the caller can see, at most `count`. The first visit schedules the lookup and still answers the empty container. - Neighbours resolve through `library_member` with the caller as the first bind; one that resolves to nothing visible is dropped. - Fields merge across providers in the operator's order, so one provider's miss does not hide another's biography. - `GET /api/v2/artists/{id}` gains `enrichment`, with its state (`disabled`, `pending`, `ready`, `none`) and a source per field. Subsonic `getArtist` stays out of it: clients that sync call it for every artist. - The engine moves from `AppState` into `DomainServices`, which both surfaces read through. - RFC-002 is amended where it says WaveFlow queries no remote source, and the two guides follow. Signed-off-by: InstaZDLL --- docs/api-v2-guide.md | 22 +- docs/rfcs/RFC-002-waveflow-server-v2.md | 2 + docs/rfcs/RFC-011-metadata-enrichment.md | 9 +- docs/subsonic-api-guide.md | 7 +- src/api/catalog.rs | 2 +- src/enrichment.rs | 8 + src/lib.rs | 11 +- src/services/artist_enrichment.rs | 217 ++++++++++++++++ src/services/artists.rs | 28 ++- src/services/mod.rs | 85 +++++++ src/subsonic/browse.rs | 39 ++- src/subsonic/mod.rs | 17 +- src/subsonic/protocol.rs | 8 +- tests/enrichment.rs | 299 +++++++++++++++++++++++ 14 files changed, 718 insertions(+), 36 deletions(-) create mode 100644 src/services/artist_enrichment.rs diff --git a/docs/api-v2-guide.md b/docs/api-v2-guide.md index 4f70788..17d0333 100644 --- a/docs/api-v2-guide.md +++ b/docs/api-v2-guide.md @@ -252,12 +252,32 @@ curl "https://music.example.com/api/v2/artists/ARTIST_UUID/top-songs?limit=10" \ every other genre filter, and both are the native form of a Subsonic method (`getSongsByGenre`, `getRandomSongs`) resolving through the same service. +`GET /api/v2/artists/{artist_id}` carries an `enrichment` object: what metadata +providers said about the artist, when an operator switched enrichment on +([RFC-011](rfcs/RFC-011-metadata-enrichment.md)). It is always present, and its +`state` says which of four situations holds, because absence could not: + +| `state` | meaning | +| --- | --- | +| `disabled` | no provider is configured; nothing has been asked and nothing will be | +| `pending` | a provider has not answered yet; ask again shortly | +| `ready` | there is something to show | +| `none` | every provider answered, and none has anything this account can be shown | + +`bio` (`summary`, `full`) and `similar` (`artists`, each an artist summary) are +`null` when there is nothing, and each carries its own `source` — `provider` +and `fetched_at` — because the fields are merged across providers and may come +from different ones. `similar` lists only artists in libraries this account +belongs to. The page never waits on a provider: the first request schedules +the lookup and answers `pending`. + `GET /api/v2/artists/{artist_id}/top-songs` answers that artist's most played tracks **for the calling account**, ordered by `play_count` and then by title. It is the native form of `getTopSongs`, through the same service. It is computed from local listening history alone — `play_event` — and not from -any remote notion of popularity, because the server queries no remote source. +any remote notion of popularity. Metadata enrichment, when an operator switches +it on, asks providers about artists, never about plays. Three consequences worth knowing before building on it: - **a track the account has never played is absent**, not last, so a fresh diff --git a/docs/rfcs/RFC-002-waveflow-server-v2.md b/docs/rfcs/RFC-002-waveflow-server-v2.md index 7b4f94a..44b768d 100644 --- a/docs/rfcs/RFC-002-waveflow-server-v2.md +++ b/docs/rfcs/RFC-002-waveflow-server-v2.md @@ -70,6 +70,8 @@ Genre matching is one rule across every method. `getGenres` groups by `genre.can `getArtistInfo` and `getArtistInfo2` resolve the requested artist through tenant-scoped catalogue access and return their standard empty containers until artist biography enrichment is implemented. This preserves compatibility with clients such as DSub without fabricating biography or similar-artist metadata. `getAlbumInfo` and `getAlbumInfo2` behave the same way for albums, for the same reason: Feishin and Symfonium call them as soon as an album page opens, and an unimplemented-method error there reads to the client as a broken album rather than as absent enrichment. They carry one real value, the album's release identifier, as their `musicBrainzId` element; notes and biography images stay absent because WaveFlow queries no remote source. `AlbumInfo` predates the presence rule and its members are elements rather than attributes, so an album with no release id omits the element instead of sending it empty. +**Amended on 2026-10-15 by [RFC-011](RFC-011-metadata-enrichment.md).** "WaveFlow queries no remote source" was a decision, not an observation, and it now holds only while no metadata provider is configured — which is the default, and what an upgraded server keeps. An operator who names a provider in `WAVEFLOW_METADATA_PROVIDERS` switches on lookups about artists. `getArtistInfo` and `getArtistInfo2` then carry the cached biography and the similar artists the caller can see. The lookup is scheduled by the first visit and never waited for, so that visit still receives the empty container. Album notes are not enriched, so `getAlbumInfo` and `getAlbumInfo2` are unchanged. + API tokens are restricted by their scopes on every route, not only on the administrative ones. `Access` names what a route needs — `Read`, `Write` or `Admin` — and is chosen at the single point where the caller is resolved, so a route cannot exist without answering the question. An empty scope list is unrestricted, which is what a password login, a grant made by an unscoped credential and a token issued without scopes carry; a non-empty list grants only what it names, and a name the server does not know grants nothing, so no vocabulary has to be enumerated. `admin` implies `write`, because a credential trusted to create accounts is not usefully barred from creating a playlist. Issuing a token stays administrative on both surfaces: a token carries the authority of the account it belongs to, so who may mint one is a question about the instance. Scopes travel through the authorization code flow, so a session is never broader than the credential that issued it. `oauth_authorization` and `session` each carry a `scopes_json` column: the consent step records the caller's scopes on the grant, redeeming it issues the session under those same scopes, and `authenticate` reads the limit off the session exactly as it already read it off an API token. Rotation carries them too — refreshing a narrowed session returns a narrowed one. Both columns default to the empty list, which is what every row written before them meant. The flaw this replaces was an escalation in two requests: the flow returned a session with the account's whole authority and neither the authorization row nor the session recorded what asked for it, so a `write` token could reach `POST /api/v2/oauth/authorize` and come back answering to `Admin`. The category error was treating "mutate user data" and "issue a reference to this account" as one authority; the answer is not to forbid the second but to make it inherit. Minting therefore asks `Write` — pairing a device is a mutation on the account — rather than the blanket restriction that stood in while the columns did not exist. An ordinary account still pairs its own devices from its own session, which is what the flow is for. diff --git a/docs/rfcs/RFC-011-metadata-enrichment.md b/docs/rfcs/RFC-011-metadata-enrichment.md index 8d472a0..1118dd9 100644 --- a/docs/rfcs/RFC-011-metadata-enrichment.md +++ b/docs/rfcs/RFC-011-metadata-enrichment.md @@ -4,10 +4,11 @@ - **Implemented by**: in progress, in the order of [the implementation plan](../metadata-enrichment-plan.md): InstaZDLL/WaveFlow#855 (the core's clients accept a client, an address and a - key), #270 (the pin moved onto them), and the pull request closing #265 (the - cache and the engine that fills it). No surface answers from the cache yet, - so every container is still the empty one. The *Status* field above never - flips in this project; this line is the one that says what is running. + key), #270 (the pin moved onto them), #271 (the cache and the engine that + fills it), and the pull request closing #266 (`getArtistInfo`, + `getArtistInfo2` and the native artist page read it). Similar songs and + portraits are still to come. The *Status* field above never flips in this + project; this line is the one that says what is running. - **Date**: 2026-10-14 - **Revised**: 2026-10-15, while implementing the cache (#265). Decisions 4, 6 and 10 rested on attachment points the code does not hold; each correction diff --git a/docs/subsonic-api-guide.md b/docs/subsonic-api-guide.md index ac133f9..c5a7415 100644 --- a/docs/subsonic-api-guide.md +++ b/docs/subsonic-api-guide.md @@ -227,7 +227,8 @@ with Subsonic error code `0`, like every other protocol failure. |---|---|---| | System | `ping`, `getLicense`, `getOpenSubsonicExtensions`, `tokenInfo` | Extensions are listed below. `tokenInfo` returns the username the presented credential resolves to. | | Catalogue roots | `getMusicFolders`, `getIndexes`, `getArtists`, `getArtist`, `getAlbum`, `getSong`, `getGenres`, `getMusicDirectory` | IDs and `musicFolderId` values are UUIDs. | -| Artist and album information | `getArtistInfo`, `getArtistInfo2`, `getAlbumInfo`, `getAlbumInfo2` | Validate tenant access and return the standard empty container; enrichment is not implemented. | +| Artist information | `getArtistInfo`, `getArtistInfo2` | Validate tenant access, then answer from the metadata cache: `biography` and the `similarArtist` entries the caller can see (at most `count`, default 20). The empty container when no provider is configured, which is the default, and on the first visit to an artist, which schedules the lookup. No image URLs; `includeNotPresent` is not honoured. | +| Album information | `getAlbumInfo`, `getAlbumInfo2` | Validate tenant access and return the release identifier when the album has one; notes are not enriched. | | Album discovery | `getAlbumList`, `getAlbumList2`, `getRandomSongs`, `getSongsByGenre` | Both legacy and ID3 album-list containers are supported. | | Search | `search3`, `search2` | Independent artist, album and song pagination. `search2` is the same payload in the `searchResult2` container. | | Playlists | `getPlaylists`, `getPlaylist`, `createPlaylist`, `updatePlaylist`, `deletePlaylist` | Shared with native/web user data. | @@ -361,8 +362,8 @@ thing. An artist takes the identifier from the tracks it is the *first* credit of, because the tag is one value on a file that may credit several artists. `getAlbumInfo` and `getAlbumInfo2` carry that release identifier as their -`musicBrainzId` element. They remain otherwise empty: WaveFlow queries no remote -source, so there are no notes and no biography images. Being a classic Subsonic +`musicBrainzId` element. They remain otherwise empty: album notes are not +enriched, so there are no notes and no images. Being a classic Subsonic response rather than an OpenSubsonic one, the element is omitted when the album has no identifier instead of being sent empty. diff --git a/src/api/catalog.rs b/src/api/catalog.rs index 088a7a8..cef99db 100644 --- a/src/api/catalog.rs +++ b/src/api/catalog.rs @@ -169,7 +169,7 @@ pub async fn get_artist( let user = authenticated(&state, &headers, Access::Read).await?; state .services - .artist(user.id, artist_id) + .artist_page(user.id, artist_id) .await .map(Json) .map_err(service_error) diff --git a/src/enrichment.rs b/src/enrichment.rs index 7f2b99e..8bf656c 100644 --- a/src/enrichment.rs +++ b/src/enrichment.rs @@ -247,6 +247,14 @@ impl Enrichment { self.engine.is_some() } + /// How many providers are configured — the number of rows a name has + /// once every one of them has answered. + pub fn provider_count(&self) -> usize { + self.engine + .as_ref() + .map_or(0, |engine| engine.sources.len()) + } + /// What the cache holds about the artist called `name`, one row per /// configured provider that has one, in the operator's order — and a fill /// scheduled for every configured provider whose row is missing or stale. diff --git a/src/lib.rs b/src/lib.rs index b853c54..1e408ca 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -78,9 +78,6 @@ pub struct AppState { /// The body ceiling the canvas route carries, derived from /// `WAVEFLOW_CANVAS_MAX_BYTES` so the two cannot disagree. pub canvas_body_limit: usize, - /// Metadata enrichment — RFC-011. Disabled unless the operator named a - /// provider. - pub enrichment: enrichment::Enrichment, } /// Roughly one offer's worth of JSON — a sixty-four character hash, a size, a @@ -255,6 +252,11 @@ fn chunk_body_limit(limits: &config::UploadLimits) -> usize { services::ArtistSummary, services::AlbumDetail, services::ArtistDetail, + services::ArtistEnrichment, + services::EnrichmentState, + services::EnrichmentSource, + services::EnrichedBio, + services::EnrichedSimilar, services::SearchResult, services::PlaylistItem, services::QueueItem, @@ -681,7 +683,7 @@ pub async fn initialize(config: &Config) -> anyhow::Result { services.reconcile_scrobble_links().await?; // Off unless the operator named a provider, and then it still makes no // request until somebody asks about an artist (RFC-011 decisions 3 and 6). - let enrichment = enrichment::Enrichment::new(db.clone(), &config.metadata)?; + services.use_enrichment(enrichment::Enrichment::new(db.clone(), &config.metadata)?); Ok(AppState { db, auth, @@ -700,7 +702,6 @@ pub async fn initialize(config: &Config) -> anyhow::Result { // Refused at startup if it does not fit, so this conversion cannot be // the place that decides what a ceiling falls back to. canvas_body_limit: usize::try_from(config.canvas.max_bytes).unwrap_or(usize::MAX), - enrichment, }) } diff --git a/src/services/artist_enrichment.rs b/src/services/artist_enrichment.rs new file mode 100644 index 0000000..2018e83 --- /dev/null +++ b/src/services/artist_enrichment.rs @@ -0,0 +1,217 @@ +//! What metadata enrichment says about an artist — RFC-011 decisions 4, 6, 7 +//! and 11. +//! +//! **The tenancy lookup comes first and for its refusal.** An artist the caller +//! cannot see answers exactly as one that does not exist, before the cache is +//! read or a provider is asked anything. +//! +//! **The cache is read, never waited on.** A fill is scheduled for every +//! provider with nothing fresh, and the answer goes out with what is there — +//! the first visit to an artist shows nothing, and says it is `pending`. +//! +//! **A neighbour is only ever an artist the caller can see.** The cached list +//! holds names; they resolve through `library_member` with the caller as the +//! first bind, and a name that resolves to nothing is dropped rather than +//! handed out with an id a client could not follow. + +use super::*; +use crate::enrichment::{CachedArtist, Outcome, Provider}; + +/// How many neighbours the native artist page carries. +pub const ARTIST_PAGE_SIMILAR: usize = 20; + +impl DomainServices { + /// The native artist page: [`Self::artist`] with its enrichment. + pub async fn artist_page( + &self, + user_id: Uuid, + artist_id: Uuid, + ) -> Result { + let mut detail = self.artist(user_id, artist_id).await?; + detail.enrichment = Some( + self.enrichment_of(user_id, &detail.artist.name, ARTIST_PAGE_SIMILAR) + .await?, + ); + Ok(detail) + } + + /// One artist's enrichment, with at most `similar_limit` neighbours. + pub async fn artist_enrichment( + &self, + user_id: Uuid, + artist_id: Uuid, + similar_limit: usize, + ) -> Result { + let summary = self.artist_summary(user_id, artist_id).await?; + self.enrichment_of(user_id, &summary.artist.name, similar_limit) + .await + } + + /// The part after the tenancy lookup, which both entry points have done. + async fn enrichment_of( + &self, + user_id: Uuid, + name: &str, + similar_limit: usize, + ) -> Result { + // Cloned out so no lock is held across the reads below; the clone is + // an `Arc`. + let engine = self + .enrichment + .read() + .unwrap_or_else(|poison| poison.into_inner()) + .clone(); + if !engine.is_enabled() { + return Ok(ArtistEnrichment::disabled()); + } + let rows = engine.artist(name).await?; + let canonical = waveflow_core::scanner::canonical_name(name); + let mut similar = None; + for row in rows.iter().filter(|row| row.outcome == Outcome::Hit) { + let artists = self + .visible_similar(user_id, row.provider, &canonical, similar_limit) + .await?; + if !artists.is_empty() { + similar = Some(EnrichedSimilar { + artists, + source: source_of(row), + }); + break; + } + } + let bio = merged_bio(&rows); + let state = state_of( + bio.is_some() || similar.is_some(), + rows.len(), + engine.provider_count(), + ); + Ok(ArtistEnrichment { + state, + bio, + similar, + }) + } + + /// One provider's cached neighbours for `canonical`, resolved to artists + /// `user_id` can see, best first, at most `limit`. + /// + /// Artist rows are per library, so one name may resolve to several — a + /// musician in two libraries the caller belongs to. One stands for them: + /// the one with the most albums, then the lowest id, so the choice is + /// stable from one request to the next. + async fn visible_similar( + &self, + user_id: Uuid, + provider: Provider, + canonical: &str, + limit: usize, + ) -> Result, ServiceError> { + if limit == 0 { + return Ok(Vec::new()); + } + let ranked = self.db.metadata_artist_similar(provider, canonical).await?; + let rows = sqlx::query(concat!( + artist_select!(album_count), + " AND ar.canonical_name IN (SELECT s.similar_canonical FROM metadata_artist_similar s \ + WHERE s.provider=? AND s.name_canonical=?) \ + ORDER BY album_count DESC, ar.id" + )) + .bind(user_id.to_string()) + .bind(provider.as_str()) + .bind(canonical) + .fetch_all(self.db.pool()) + .await?; + let mut by_name: std::collections::HashMap = + std::collections::HashMap::new(); + for row in rows { + let summary = artist_summary_from_row(row)?; + // The column the query matched on is this function of the name, + // written together with it (`catalog::upsert_artist`), so it is + // recomputed rather than projected. + by_name + .entry(waveflow_core::scanner::canonical_name(&summary.artist.name)) + .or_insert(summary); + } + Ok(ranked + .into_iter() + .filter_map(|neighbour| by_name.remove(&neighbour.name_canonical)) + .take(limit) + .collect()) + } +} + +/// The biography: the first row, in the operator's order, that has one. A +/// provider that knows nothing does not hide the next one's. +fn merged_bio(rows: &[CachedArtist]) -> Option { + rows.iter() + .find(|row| row.bio_summary.is_some() || row.bio_full.is_some()) + .map(|row| EnrichedBio { + summary: row.bio_summary.clone(), + full: row.bio_full.clone(), + source: source_of(row), + }) +} + +/// Content wins over everything; without it, a provider that has not +/// answered makes the lookup `pending`, and only when every one of them has +/// answered is it `none`. +fn state_of(has_content: bool, answered: usize, configured: usize) -> EnrichmentState { + if has_content { + EnrichmentState::Ready + } else if answered < configured { + EnrichmentState::Pending + } else { + EnrichmentState::None + } +} + +fn source_of(row: &CachedArtist) -> EnrichmentSource { + EnrichmentSource { + provider: row.provider.as_str().to_owned(), + fetched_at: row.fetched_at, + } +} + +#[cfg(test)] +mod tests { + use super::{merged_bio, state_of}; + use crate::enrichment::{CachedArtist, Outcome, Provider}; + use crate::services::EnrichmentState; + + fn row(outcome: Outcome, bio: Option<&str>, fetched_at: i64) -> CachedArtist { + CachedArtist { + provider: Provider::LastFm, + outcome, + bio_summary: bio.map(str::to_owned), + bio_full: None, + fetched_at, + expires_at: fetched_at + 1, + } + } + + /// Decision 4: one provider's "never heard of them" is not the world's. + /// Only Last.fm exists as a provider today, so the two rows here stand in + /// for two providers by their position, which is all the merge reads. + #[test] + fn a_miss_at_the_first_provider_does_not_hide_the_second_biography() { + let rows = [ + row(Outcome::Miss, None, 1), + row(Outcome::Hit, Some("Born in Reykjavík."), 2), + ]; + let bio = merged_bio(&rows).expect("the second provider's biography"); + assert_eq!(bio.summary.as_deref(), Some("Born in Reykjavík.")); + assert_eq!(bio.source.fetched_at, 2); + } + + /// Decision 11's table, row by row. + #[test] + fn each_state_is_said_when_it_holds() { + // Something to show, even while another provider is still out. + assert_eq!(state_of(true, 1, 2), EnrichmentState::Ready); + // Nothing yet, and a provider has not answered. + assert_eq!(state_of(false, 0, 1), EnrichmentState::Pending); + assert_eq!(state_of(false, 1, 2), EnrichmentState::Pending); + // Everyone answered, nobody had anything. + assert_eq!(state_of(false, 1, 1), EnrichmentState::None); + } +} diff --git a/src/services/artists.rs b/src/services/artists.rs index 0bb0096..7a6ef58 100644 --- a/src/services/artists.rs +++ b/src/services/artists.rs @@ -36,14 +36,7 @@ impl DomainServices { user_id: Uuid, artist_id: Uuid, ) -> Result { - let summary = sqlx::query(concat!(artist_select!(album_count), " AND ar.id=?")) - .bind(user_id.to_string()) - .bind(artist_id.to_string()) - .fetch_optional(self.db.pool()) - .await? - .map(artist_summary_from_row) - .transpose()? - .ok_or(ServiceError::NotFound)?; + let summary = self.artist_summary(user_id, artist_id).await?; let mut albums = sqlx::query(concat!( album_select!(), " AND EXISTS (SELECT 1 FROM album_participant ap \ @@ -62,9 +55,28 @@ impl DomainServices { artist: summary.artist, album_count: summary.album_count, albums, + enrichment: None, }) } + /// One artist the user can see, or [`ServiceError::NotFound`] — for one + /// that does not exist and one in a library they are not a member of + /// alike. + pub(super) async fn artist_summary( + &self, + user_id: Uuid, + artist_id: Uuid, + ) -> Result { + sqlx::query(concat!(artist_select!(album_count), " AND ar.id=?")) + .bind(user_id.to_string()) + .bind(artist_id.to_string()) + .fetch_optional(self.db.pool()) + .await? + .map(artist_summary_from_row) + .transpose()? + .ok_or(ServiceError::NotFound) + } + /// The account's most played tracks of one artist, counted from local /// history. /// diff --git a/src/services/mod.rs b/src/services/mod.rs index 947c6cc..2a3631c 100644 --- a/src/services/mod.rs +++ b/src/services/mod.rs @@ -912,6 +912,75 @@ pub struct ArtistDetail { /// should not have to depend on one. pub album_count: i64, pub albums: Vec, + /// What metadata enrichment says about this artist — RFC-011 decision 11. + /// + /// Always present on `GET /api/v2/artists/{id}`, whose answer comes from + /// [`DomainServices::artist_page`]. Left out of the Subsonic `getArtist` + /// path, which renders its own nodes: clients that sync call it for every + /// artist, and asking providers about the whole catalogue at sync time is + /// the prefill decision 6 refuses. + #[serde(skip_serializing_if = "Option::is_none")] + pub enrichment: Option, +} + +/// What enrichment holds about one artist, merged across providers in the +/// operator's order, field by field (RFC-011 decisions 4 and 11). +#[derive(Debug, Clone, Serialize, ToSchema)] +pub struct ArtistEnrichment { + pub state: EnrichmentState, + /// The first configured provider with a biography supplies it. + pub bio: Option, + /// The first configured provider whose neighbours include artists the + /// caller can see supplies the list — those artists only, best first. + pub similar: Option, +} + +impl ArtistEnrichment { + pub(crate) fn disabled() -> Self { + Self { + state: EnrichmentState::Disabled, + bio: None, + similar: None, + } + } +} + +/// Four situations that absence alone cannot tell apart. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, ToSchema)] +#[serde(rename_all = "lowercase")] +pub enum EnrichmentState { + /// No provider is configured: nothing has been asked and nothing will be. + Disabled, + /// A provider has not answered yet — scheduled, in flight, or unreached + /// last time, which writes nothing and is retried on the next request. + Pending, + /// There is something to show. + Ready, + /// Every configured provider answered, and none of them has anything this + /// caller can be shown. + None, +} + +/// Where one field came from. +#[derive(Debug, Clone, Serialize, ToSchema)] +pub struct EnrichmentSource { + /// The provider's name as `WAVEFLOW_METADATA_PROVIDERS` spells it. + pub provider: String, + /// When the provider gave this answer, in epoch milliseconds. + pub fetched_at: i64, +} + +#[derive(Debug, Clone, Serialize, ToSchema)] +pub struct EnrichedBio { + pub summary: Option, + pub full: Option, + pub source: EnrichmentSource, +} + +#[derive(Debug, Clone, Serialize, ToSchema)] +pub struct EnrichedSimilar { + pub artists: Vec, + pub source: EnrichmentSource, } /// Inputs for a native client's authorization request. @@ -1185,6 +1254,10 @@ pub struct DomainServices { /// after construction like [`Self::register_scrobble_target`], and for the /// same reason. lastfm_exchanges: lastfm::LastFmExchanges, + /// Metadata enrichment — RFC-011. Disabled at construction and placed by + /// [`Self::use_enrichment`], like the scrobbling adapters and for the same + /// reason: a test drives it against a provider of its own. + enrichment: Arc>, } #[derive(Debug, thiserror::Error)] @@ -1222,6 +1295,7 @@ impl From for ServiceError { mod admin; mod albums; +mod artist_enrichment; mod artists; mod artwork; mod bookmarks; @@ -1307,9 +1381,20 @@ impl DomainServices { lastfm_journey: lastfm::LastFmJourney::from_config(config), lastfm_unavailable: lastfm::lastfm_unavailability(config), lastfm_exchanges: Arc::new(dashmap::DashMap::new()), + enrichment: Arc::new(std::sync::RwLock::new( + crate::enrichment::Enrichment::disabled(), + )), } } + /// Places the enrichment engine every reader goes through. + pub fn use_enrichment(&self, enrichment: crate::enrichment::Enrichment) { + *self + .enrichment + .write() + .unwrap_or_else(|poison| poison.into_inner()) = enrichment; + } + async fn require_admin(&self, actor_id: Uuid) -> Result<(), ServiceError> { let account = self .db diff --git a/src/subsonic/browse.rs b/src/subsonic/browse.rs index 04b0849..4f3486d 100644 --- a/src/subsonic/browse.rs +++ b/src/subsonic/browse.rs @@ -76,24 +76,53 @@ pub(super) async fn get_artist( .children(detail.albums.iter().map(album_node))) } +/// `getArtistInfo` and `getArtistInfo2`. +/// +/// Answered from the enrichment cache (RFC-011), never by waiting on a +/// provider: the first visit to an artist schedules a lookup and answers the +/// empty container, as this method always did; a later one carries what came +/// back. With no provider configured — the default — it stays the empty +/// container for good. +/// +/// `biography` is the provider's summary, or its full text when it gave no +/// summary. `similarArtist` lists only artists the caller can see, at most +/// `count` (default 20), and nothing that is not in a library they belong to: +/// `includeNotPresent` asks for exactly that, and is not honoured — an entry +/// whose id a client cannot follow would hand out one it must not have. The +/// three image URLs stay absent (RFC-011 decision 10, revised). +/// +/// The lookup runs first and for its refusal, so an artist the caller cannot +/// reach answers as one that does not exist. pub(super) async fn artist_info( state: &AppState, principal: &Principal, params: &Params, container: &'static str, ) -> Result { - state + let count = params.usize_or("count", 20, 100)?; + let enrichment = state .services - .artist(principal.id, params.uuid("id")?) + .artist_enrichment(principal.id, params.uuid("id")?, count) .await .map_err(service_protocol)?; - Ok(Node::new(container)) + let biography = enrichment + .bio + .and_then(|bio| bio.summary.or(bio.full)) + .map(|text| Node::new("biography").text(text)); + let similar = enrichment + .similar + .into_iter() + .flat_map(|similar| similar.artists) + .map(|artist| { + artist_node(&artist.artist, artist.album_count as usize).renamed("similarArtist") + }); + Ok(Node::new(container).children(biography).children(similar)) } /// `getAlbumInfo` and `getAlbumInfo2`. /// -/// WaveFlow queries no remote source, so notes and biography images stay -/// absent. The release identifier is the one part of the answer the catalogue +/// Album notes are not enriched — RFC-011 leaves `metadata_album` to an open +/// question — so notes and images stay absent. The release identifier is the one part of the answer the catalogue /// actually holds, and it is emitted when the album has one. `AlbumInfo` /// predates the OpenSubsonic presence rule, so an album without a release id /// omits the element rather than sending it empty. diff --git a/src/subsonic/mod.rs b/src/subsonic/mod.rs index 430f34c..dea165a 100644 --- a/src/subsonic/mod.rs +++ b/src/subsonic/mod.rs @@ -452,16 +452,17 @@ async fn dispatch( "getArtists" => indexes(state, principal, params, true).await, "getArtist" => get_artist(state, principal, params).await, // DSub requests artist information as soon as an artist page opens. - // WaveFlow does not enrich biographies yet, but a successful empty - // standard container avoids turning an optional capability into a - // blocking client error. The artist is still resolved tenant-side. + // Answered from the enrichment cache when an operator switched it on + // (RFC-011), and the empty standard container otherwise, so an + // optional capability never becomes a blocking client error. The + // artist is resolved tenant-side either way. "getArtistInfo" => artist_info(state, principal, params, "artistInfo").await, "getArtistInfo2" => artist_info(state, principal, params, "artistInfo2").await, - // Feishin and Symfonium call these as soon as an album page opens. As - // with getArtistInfo, WaveFlow enriches nothing yet, so the standard - // empty container is the honest answer — and it still resolves the - // album tenant-side, so a foreign id is indistinguishable from a - // missing one. + // Feishin and Symfonium call these as soon as an album page opens. + // Album notes are not enriched, so the standard container with only + // what the catalogue holds is the honest answer — and it still + // resolves the album tenant-side, so a foreign id is + // indistinguishable from a missing one. "getAlbumInfo" => album_info(state, principal, params, "albumInfo").await, "getAlbumInfo2" => album_info(state, principal, params, "albumInfo2").await, "getAlbum" => get_album(state, principal, params).await, diff --git a/src/subsonic/protocol.rs b/src/subsonic/protocol.rs index 209ff34..13d86fb 100644 --- a/src/subsonic/protocol.rs +++ b/src/subsonic/protocol.rs @@ -152,7 +152,7 @@ pub(super) fn json_required_array_fields(parent: &str, name: &str) -> &'static [ ], // 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"], + "artist" | "similarArtist" => &["roles"], _ => &[], } } @@ -242,6 +242,12 @@ pub(super) fn json_array_field(parent: &str, name: &str) -> bool { // so its roles stay an array there too — otherwise the field // collapses into a bare object the moment a directory carries it. | ("artist" | "child", "roles") + // Filled since RFC-011. Empty until then, so nothing had ever told + // the list apart from an object — the case `topSongs` and + // `internetRadioStations` were before it. A neighbour is an artist + // record under another name, so its roles keep their shape too. + | ("artistInfo" | "artistInfo2", "similarArtist") + | ("similarArtist", "roles") ) } diff --git a/tests/enrichment.rs b/tests/enrichment.rs index 0988764..e0168a7 100644 --- a/tests/enrichment.rs +++ b/tests/enrichment.rs @@ -465,3 +465,302 @@ async fn the_key_never_reaches_the_log() { ); assert!(!log.contains(&key), "the key reached the log:\n{log}"); } + +// ── The readers: getArtistInfo, getArtistInfo2 and the native artist page ── +// +// Accounts are inserted as rows rather than through `create_account`: these +// tests need user ids and libraries, not a login, and a password hashed from a +// literal is what makes CodeQL flag a test. They reach HTTP through an API +// token and a Subsonic key minted at run time. + +async fn account(state: &waveflow_server::AppState, username: &str) -> uuid::Uuid { + let id = uuid::Uuid::new_v4(); + let now = waveflow_server::authentication::now_ms(); + sqlx::query( + "INSERT INTO account (id, username, password_hash, role, disabled, created_at, updated_at) \ + VALUES (?, ?, 'this-account-never-authenticates', 'admin', 0, ?, ?)", + ) + .bind(id.to_string()) + .bind(username) + .bind(now) + .bind(now) + .execute(state.db.pool()) + .await + .unwrap(); + id +} + +/// A private library of `owner`'s holding one album by each of `artists`. +async fn library_of( + state: &waveflow_server::AppState, + config: &waveflow_server::Config, + owner: uuid::Uuid, + name: &str, + artists: &[&str], +) { + let now = waveflow_server::authentication::now_ms(); + let root = config.data_dir.join(name); + std::fs::create_dir_all(&root).unwrap(); + let library = state + .db + .create_library( + owner, + name, + &std::fs::canonicalize(&root).unwrap(), + waveflow_server::database::LibraryVisibility::Private, + now, + ) + .await + .unwrap(); + let scan = state + .db + .create_scan_job(library, Some(owner), "manual") + .await + .unwrap(); + state + .db + .start_scan_job(scan, artists.len() as i64, false) + .await + .unwrap(); + for (index, artist) in artists.iter().enumerate() { + let mut input = support::browse_input( + index + name.len() * 100, + "A song", + &format!("{artist} album"), + artist, + Some(1), + Some(1), + ); + input.relative_path = format!("{name}-{index}.flac"); + input.quick_hash = format!("{:064x}", 70_000 + index + name.len() * 100); + input.full_hash = format!("{:064x}", 80_000 + index + name.len() * 100); + state + .db + .apply_catalog_track(library, scan, &input, None, false) + .await + .unwrap(); + } + state + .db + .consolidate_catalog_derivations(library) + .await + .unwrap(); + state.db.finish_scan_job(scan, 0).await.unwrap(); +} + +async fn artist_id(state: &waveflow_server::AppState, user: uuid::Uuid, name: &str) -> uuid::Uuid { + state + .services + .list_artists( + user, + None, + waveflow_server::services::BrowsePage::new(None, None).unwrap(), + ) + .await + .unwrap() + .into_iter() + .find(|artist| artist.artist.name == name) + .unwrap_or_else(|| panic!("{name} is not visible")) + .artist + .id +} + +/// Björk and Sigur Rós in the owner's library; Múm only in the outsider's. +/// The fake answers Sigur Rós and Múm as Björk's neighbours. +async fn two_members() -> ( + tempfile::TempDir, + waveflow_server::Config, + waveflow_server::AppState, + uuid::Uuid, + uuid::Uuid, +) { + let (temp, config, state) = support::test_app().await; + let owner = account(&state, "enrichment-owner").await; + let outsider = account(&state, "enrichment-outsider").await; + library_of(&state, &config, owner, "owned", &["Björk", "Sigur Rós"]).await; + library_of(&state, &config, outsider, "foreign", &["Múm"]).await; + (temp, config, state, owner, outsider) +} + +/// Decision 7: a neighbour is an artist the caller can see, or nothing. Múm +/// is in Last.fm's list and in this instance, in a library the owner is not a +/// member of; returning it would hand out an id `getArtist` then refuses, +/// having already confirmed the row exists. +#[tokio::test] +async fn a_similar_artist_living_only_in_another_members_library_is_not_returned() { + let (_temp, _config, state, owner, _outsider) = two_members().await; + let listener = fake_lastfm(Mode::Hit, Duration::ZERO).await; + let enrichment = engine( + &state, + &settings(vec![Provider::LastFm], &run_key()), + &listener, + ); + state.services.use_enrichment(enrichment.clone()); + let bjork = artist_id(&state, owner, "Björk").await; + + state + .services + .artist_enrichment(owner, bjork, 20) + .await + .unwrap(); + enrichment.idle().await; + let answer = state + .services + .artist_enrichment(owner, bjork, 20) + .await + .unwrap(); + + let similar = answer.similar.expect("a neighbour the owner can see"); + let names: Vec<_> = similar + .artists + .iter() + .map(|artist| artist.artist.name.as_str()) + .collect(); + assert_eq!(names, ["Sigur Rós"]); + assert_eq!(similar.source.provider, "lastfm"); +} + +/// The lookup comes first and for its refusal: an artist in another member's +/// library answers as a missing one, and no provider is asked about it. +#[tokio::test] +async fn a_foreign_artist_id_answers_as_a_missing_one() { + let (_temp, _config, state, owner, outsider) = two_members().await; + let listener = fake_lastfm(Mode::Hit, Duration::ZERO).await; + let enrichment = engine( + &state, + &settings(vec![Provider::LastFm], &run_key()), + &listener, + ); + state.services.use_enrichment(enrichment.clone()); + let mum = artist_id(&state, outsider, "Múm").await; + + let refused = state.services.artist_enrichment(owner, mum, 20).await; + let missing = state + .services + .artist_enrichment(owner, uuid::Uuid::new_v4(), 20) + .await; + enrichment.idle().await; + + assert!(matches!( + refused, + Err(waveflow_server::services::ServiceError::NotFound) + )); + assert!(matches!( + missing, + Err(waveflow_server::services::ServiceError::NotFound) + )); + assert_eq!(listener.fake.info.load(Ordering::SeqCst), 0); +} + +/// Decision 11's four states, each where it holds, through the native page — +/// the one surface that publishes them. +#[tokio::test] +async fn each_of_the_four_states_appears_when_it_should() { + use waveflow_server::services::EnrichmentState; + + let (_temp, _config, state, owner, _outsider) = two_members().await; + let bjork = artist_id(&state, owner, "Björk").await; + let state_now = |state: &waveflow_server::AppState| { + let services = state.services.clone(); + async move { + services + .artist_page(owner, bjork) + .await + .unwrap() + .enrichment + .expect("the native page always carries it") + .state + } + }; + + // Nothing configured: the server's default. + assert_eq!(state_now(&state).await, EnrichmentState::Disabled); + + // Configured, first visit: scheduled, not waited for. + let found = fake_lastfm(Mode::Hit, Duration::ZERO).await; + let enrichment = engine( + &state, + &settings(vec![Provider::LastFm], &run_key()), + &found, + ); + state.services.use_enrichment(enrichment.clone()); + assert_eq!(state_now(&state).await, EnrichmentState::Pending); + enrichment.idle().await; + assert_eq!(state_now(&state).await, EnrichmentState::Ready); + + // A provider that knows nothing about an artist: `none`, not absence. + let unknown = fake_lastfm(Mode::Miss, Duration::ZERO).await; + let enrichment = engine( + &state, + &settings(vec![Provider::LastFm], &run_key()), + &unknown, + ); + state.services.use_enrichment(enrichment.clone()); + let sigur = artist_id(&state, owner, "Sigur Rós").await; + let page = |state: &waveflow_server::AppState| { + let services = state.services.clone(); + async move { + services + .artist_page(owner, sigur) + .await + .unwrap() + .enrichment + .unwrap() + } + }; + assert_eq!(page(&state).await.state, EnrichmentState::Pending); + enrichment.idle().await; + let none = page(&state).await; + assert_eq!(none.state, EnrichmentState::None); + assert!(none.bio.is_none() && none.similar.is_none()); +} + +/// The façade end to end, in JSON: the first visit is the empty container it +/// always was; the next carries the biography, and the neighbours as an array +/// even when there is one of them. +#[tokio::test] +async fn get_artist_info_2_carries_the_biography_and_the_visible_neighbours() { + let (_temp, config, state, owner, _outsider) = two_members().await; + let api_key = format!("wfsk_{}", run_key()); + let password = run_key(); + state + .db + .set_subsonic_credential( + owner, + owner, + &state.secret_box.encrypt(password.as_bytes()).unwrap(), + &waveflow_server::security::token_hash(&api_key), + waveflow_server::authentication::now_ms(), + ) + .await + .unwrap(); + let listener = fake_lastfm(Mode::Hit, Duration::ZERO).await; + let enrichment = engine( + &state, + &settings(vec![Provider::LastFm], &run_key()), + &listener, + ); + state.services.use_enrichment(enrichment.clone()); + let bjork = artist_id(&state, owner, "Björk").await; + let router = waveflow_server::app(&config, state.clone()); + + let first = + support::subsonic_json(&router, "getArtistInfo2", &api_key, &format!("&id={bjork}")).await; + assert_eq!( + first["subsonic-response"]["artistInfo2"], + serde_json::json!({}), + "{first}" + ); + enrichment.idle().await; + let next = + support::subsonic_json(&router, "getArtistInfo2", &api_key, &format!("&id={bjork}")).await; + + let info = &next["subsonic-response"]["artistInfo2"]; + assert_eq!(info["biography"], "Icelandic singer.", "{next}"); + let similar = info["similarArtist"] + .as_array() + .unwrap_or_else(|| panic!("an array even with one entry: {next}")); + assert_eq!(similar.len(), 1); + assert_eq!(similar[0]["name"], "Sigur Rós"); + assert!(similar[0]["roles"].is_array(), "{next}"); +} From 3fbe684a6b1bb852a82971373cac05323e263a41 Mon Sep 17 00:00:00 2001 From: InstaZDLL Date: Thu, 15 Oct 2026 21:53:30 +0200 Subject: [PATCH 2/2] fix(enrichment): an expired miss being refreshed is pending, not none Decision 11 defines `pending` as a lookup scheduled or in flight. An expired row has just had its refresh scheduled, yet it was counted as an answer, so an expired miss read as `none`. Only fresh rows count as answered now; stale content still makes the state `ready`. Signed-off-by: InstaZDLL --- src/services/artist_enrichment.rs | 8 ++++++- tests/enrichment.rs | 35 +++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/src/services/artist_enrichment.rs b/src/services/artist_enrichment.rs index 2018e83..34d39ac 100644 --- a/src/services/artist_enrichment.rs +++ b/src/services/artist_enrichment.rs @@ -80,9 +80,15 @@ impl DomainServices { } } let bio = merged_bio(&rows); + // Only a fresh row is an answer. An expired one has just had its + // refresh scheduled, and an expired miss is not "nobody knows" but + // "being asked again" — `pending`, as decision 11 defines it. Stale + // content still shows, and `ready` wins over both. + let now = crate::authentication::now_ms(); + let answered = rows.iter().filter(|row| row.expires_at > now).count(); let state = state_of( bio.is_some() || similar.is_some(), - rows.len(), + answered, engine.provider_count(), ); Ok(ArtistEnrichment { diff --git a/tests/enrichment.rs b/tests/enrichment.rs index e0168a7..811843e 100644 --- a/tests/enrichment.rs +++ b/tests/enrichment.rs @@ -715,6 +715,41 @@ async fn each_of_the_four_states_appears_when_it_should() { assert!(none.bio.is_none() && none.similar.is_none()); } +/// An expired miss is being asked again, so it is `pending` rather than +/// `none`: the miss window here is zero, so the row is stale the moment it is +/// written, and the next visit schedules its refresh. +#[tokio::test] +async fn an_expired_miss_being_refreshed_is_pending() { + use waveflow_server::services::EnrichmentState; + + let (_temp, _config, state, owner, _outsider) = two_members().await; + let listener = fake_lastfm(Mode::Miss, Duration::ZERO).await; + let enrichment = engine( + &state, + &MetadataSettings { + miss_ttl: Duration::ZERO, + ..settings(vec![Provider::LastFm], &run_key()) + }, + &listener, + ); + state.services.use_enrichment(enrichment.clone()); + let bjork = artist_id(&state, owner, "Björk").await; + + state.services.artist_page(owner, bjork).await.unwrap(); + enrichment.idle().await; + let refreshing = state + .services + .artist_page(owner, bjork) + .await + .unwrap() + .enrichment + .unwrap(); + enrichment.idle().await; + + assert_eq!(refreshing.state, EnrichmentState::Pending); + assert_eq!(listener.fake.info.load(Ordering::SeqCst), 2); +} + /// The façade end to end, in JSON: the first visit is the empty container it /// always was; the next carries the biography, and the neighbours as an array /// even when there is one of them.