diff --git a/docs/rfcs/RFC-002-waveflow-server-v2.md b/docs/rfcs/RFC-002-waveflow-server-v2.md index 6910bb5..d1ee477 100644 --- a/docs/rfcs/RFC-002-waveflow-server-v2.md +++ b/docs/rfcs/RFC-002-waveflow-server-v2.md @@ -58,7 +58,7 @@ The columns behind the tag fields are added empty and filled by the next scan, s Pagination is capped at 500 items per page. `offset` is zero-based; `search3` applies its independent `artistOffset`, `albumOffset` and `songOffset` after tenant filtering. Repeated `songId`, `songIdToAdd`, `songIndexToRemove`, scrobble `id`/`time`, queue `id`, star IDs, share IDs and `musicFolderId` values retain request order. Catalogue endpoints accept the union of repeated authorized `musicFolderId` values and return no foreign data for inaccessible IDs. `createUser` grants every current library when the parameter is absent or exactly the repeated selection when present; `updateUser` replaces Subsonic-managed listener memberships only, preserving owner/manager roles. `getUser` and `getUsers` expose the effective UUID list as `folder[]`. Default catalogue order is case-insensitive display name through the SQLite `NOCASE` collation, which folds ASCII; indexes group by uppercase ASCII initial with `#` fallback. Album lists implement `random`, `newest`, `highest`, `frequent`, `recent`, `starred`, both alphabetical modes, `byYear` (including reversed ranges) and `byGenre`; non-random ties use stable title/UUID ordering. All ten resolve in `DomainServices::list_albums` and are ordered, filtered and paged in SQL. `byGenre` matches on the canonical genre name — case, punctuation and spacing folded — rather than on a raw display string, so one genre is never split in two; it requires `genre` and is rejected with error code 10 without it, rather than answering with an unfiltered catalogue. A `size` of zero is an empty container, not an error. Random-song year and genre filters are applied after repository authorization. Playlist positions are contiguous and removals are applied from highest index downward before additions. The legacy `getAlbumList` method remains an alias for older clients and uses the `albumList` response container; `getAlbumList2` uses `albumList2`. `star` and `unstar` accept the generic `id` form for tracks, albums and artists in addition to the typed album/artist parameters; resolution remains tenant-scoped and rejects ambiguous or invisible entities. -For full-catalogue pagination, `search3` treats the literal query `""` as match-all, as used by Symfonium. There is nothing to match and FTS5 has no expression meaning "everything", so that request is three ordinary listings wearing the search response, each ordered and paged in SQL through `DomainServices::browse_all`. It previously read the whole visible catalogue and sliced it in Rust once per page, which made a client's initial synchronization quadratic in the library. `getBookmarks`, `createBookmark` and `deleteBookmark` are backed by a real `bookmark` row per account and track: a bookmark answers "where did I stop in this file", so setting one twice moves it rather than adding a second, and omitting the comment clears it. Deleting one that is not there succeeds — the caller asked for the track to carry no bookmark, and it does not — which also avoids answering a question about another account's catalogue. Bookmarks are a domain mutation like every other piece of user data, so they claim a sync operation, appear in the journal under the `bookmark` entity type and ship in the bootstrap snapshot; the journal's `entity_type` vocabulary is CHECK-constrained, so admitting them was a deliberate contract change rather than a side effect. The bookmarked entry is rendered through the shared media projection and carries `bookmarkPosition`, which no other media item does because a track only has a position inside the bookmark holding it. `getSimilarSongs` and `getSimilarSongs2` used to answer the same way and no longer do once an operator configures a metadata provider ([RFC-011](RFC-011-metadata-enrichment.md)): they resolve the artist, album or song id first and for its refusal, an album standing for its album artist and a song for its track artist, and answer a random draw of at most `count` (default 50) from that artist's tracks and their neighbours' in the libraries the caller belongs to. With no provider configured, which is the default, they answer the standard empty container they always did, for any id the caller can reach. `getInternetRadioStations` used to be in that company and no longer is. It reads a `radio_station` row per account, and `createInternetRadioStation`, `updateInternetRadioStation` and `deleteInternetRadioStation` write them, each answering the bare envelope the specification documents. **The server never fetches a station's address**: it validates it as absolute `http` or `https`, stores it as typed and hands it to whichever client asked. That is the whole of the security question the feature raises — a station is a URL a user supplies, so a server that dereferenced one would be a request forger aimed at whatever it can reach — and it is also why the feature needs no allowlist, no timeout and no egress policy. Two deviations from the reference server are deliberate. The list is **per account**, where the reference server keeps one global list only an administrator may change: WaveFlow has no global library setting of any kind, and a per-account list needs no new privilege to be safe, so a station belonging to somebody else is 404 like everything else here. And `homePageUrl` is spelled with a capital `P` on the way out while the parameter that sets it is `homepageUrl` — the specification's own asymmetry, copied rather than corrected, because a client following the document reads the field it asked for. Filling the container distinguished it from an object in JSON, exactly as `getTopSongs` had been: `internetRadioStations.internetRadioStation` is an array whatever its length. Stations are account configuration rather than playback state, so they sit beside the API tokens and the scrobbling links and **are not in the sync journal** — admitting a new `entity_type` is a contract change the desktop client has to be ready for, and nothing has asked for it. `getTopSongs` no longer answers that way. It returns the artist's most played tracks as counted from the calling account's own `play_event` rows, which is a narrower question than the reference server's — it asks a remote service what is popular, and this reports what was played here — and the only one the catalogue can answer. `artist` is a name, so it is matched on `artist.canonical_name`, the folding rule every other catalogue match uses, and a name that resolves in two libraries contributes both; an unknown name is an empty container rather than error 70, because a name is not an identifier and there is no existence to blur. A missing `artist` stays error 10. `count` defaults to the specification's fifty, is capped at five hundred, and `count=0` is an empty container as it is for `getAlbumList`. A track the account has never played is absent rather than ranked last, so the list is a history and not a discography in disguise. Filling the container is also what first distinguished it from an object in JSON: `topSongs.song` is now an array whatever its length, like every other collection field. `getAvatar` answers error code 70 rather than code 0, because no avatars are stored: the data is missing, not the method. `search2` and `getStarred` are container renames of `search3` and `getStarred2`, as `getAlbumList` is of `getAlbumList2`. +For full-catalogue pagination, `search3` treats the literal query `""` as match-all, as used by Symfonium. There is nothing to match and FTS5 has no expression meaning "everything", so that request is three ordinary listings wearing the search response, each ordered and paged in SQL through `DomainServices::browse_all`. It previously read the whole visible catalogue and sliced it in Rust once per page, which made a client's initial synchronization quadratic in the library. `getBookmarks`, `createBookmark` and `deleteBookmark` are backed by a real `bookmark` row per account and track: a bookmark answers "where did I stop in this file", so setting one twice moves it rather than adding a second, and omitting the comment clears it. Deleting one that is not there succeeds — the caller asked for the track to carry no bookmark, and it does not — which also avoids answering a question about another account's catalogue. Bookmarks are a domain mutation like every other piece of user data, so they claim a sync operation, appear in the journal under the `bookmark` entity type and ship in the bootstrap snapshot; the journal's `entity_type` vocabulary is CHECK-constrained, so admitting them was a deliberate contract change rather than a side effect. The bookmarked entry is rendered through the shared media projection and carries `bookmarkPosition`, which no other media item does because a track only has a position inside the bookmark holding it. `getSimilarSongs` and `getSimilarSongs2` used to answer the same way and no longer do once an operator configures a metadata provider ([RFC-011](RFC-011-metadata-enrichment.md)): they resolve the artist, album or song id first and for its refusal, an album standing for its album artist and a song for its track artist, and answer a random draw of at most `count` (default 50) from that artist's tracks and their neighbours' in the libraries the caller belongs to, balanced across artists so a prolific one does not crowd the neighbours out. With no provider configured, which is the default, they answer the standard empty container they always did, for any id the caller can reach. `getInternetRadioStations` used to be in that company and no longer is. It reads a `radio_station` row per account, and `createInternetRadioStation`, `updateInternetRadioStation` and `deleteInternetRadioStation` write them, each answering the bare envelope the specification documents. **The server never fetches a station's address**: it validates it as absolute `http` or `https`, stores it as typed and hands it to whichever client asked. That is the whole of the security question the feature raises — a station is a URL a user supplies, so a server that dereferenced one would be a request forger aimed at whatever it can reach — and it is also why the feature needs no allowlist, no timeout and no egress policy. Two deviations from the reference server are deliberate. The list is **per account**, where the reference server keeps one global list only an administrator may change: WaveFlow has no global library setting of any kind, and a per-account list needs no new privilege to be safe, so a station belonging to somebody else is 404 like everything else here. And `homePageUrl` is spelled with a capital `P` on the way out while the parameter that sets it is `homepageUrl` — the specification's own asymmetry, copied rather than corrected, because a client following the document reads the field it asked for. Filling the container distinguished it from an object in JSON, exactly as `getTopSongs` had been: `internetRadioStations.internetRadioStation` is an array whatever its length. Stations are account configuration rather than playback state, so they sit beside the API tokens and the scrobbling links and **are not in the sync journal** — admitting a new `entity_type` is a contract change the desktop client has to be ready for, and nothing has asked for it. `getTopSongs` no longer answers that way. It returns the artist's most played tracks as counted from the calling account's own `play_event` rows, which is a narrower question than the reference server's — it asks a remote service what is popular, and this reports what was played here — and the only one the catalogue can answer. `artist` is a name, so it is matched on `artist.canonical_name`, the folding rule every other catalogue match uses, and a name that resolves in two libraries contributes both; an unknown name is an empty container rather than error 70, because a name is not an identifier and there is no existence to blur. A missing `artist` stays error 10. `count` defaults to the specification's fifty, is capped at five hundred, and `count=0` is an empty container as it is for `getAlbumList`. A track the account has never played is absent rather than ranked last, so the list is a history and not a discography in disguise. Filling the container is also what first distinguished it from an object in JSON: `topSongs.song` is now an array whatever its length, like every other collection field. `getAvatar` answers error code 70 rather than code 0, because no avatars are stored: the data is missing, not the method. `search2` and `getStarred` are container renames of `search3` and `getStarred2`, as `getAlbumList` is of `getAlbumList2`. `startScan` and `getScanStatus` close the last operational asymmetry with the native API. Subsonic has no library parameter, so `startScan` fans out over every library the account may scan and answers with the `scanStatus` element. Both surfaces queue through `DomainServices::start_library_scan`, so the question of who may scan what has one implementation and cannot drift. Membership alone is not the answer to it: a scan walks the owner's files and takes the process-wide writer gate, so it is reserved to the `owner` and `manager` roles, enforced in the `scan_job` insert itself rather than in a handler. The per-library native route answers `404`, indistinguishably from a library that does not exist. `startScan` names no library, so it skips the ones the account may only listen to instead of failing on them — refusing the whole call because one library is read-only would put the scannable ones out of reach from Subsonic entirely — and an account that may scan nothing queues nothing and succeeds, like one that reaches no library at all. The fan-out is best effort: a library that cannot be queued does not cancel the others, and the error surfaces only when nothing could be queued at all. Re-queuing a library that is already scanning is allowed, as it is natively: the scanner serialises jobs per library and a scan converges on file content, so a redundant pass costs time and changes nothing. `scanStatus` reports `scanning` across those libraries and a `count` of the available tracks the account can reach, never a total spanning another tenant. diff --git a/docs/rfcs/RFC-011-metadata-enrichment.md b/docs/rfcs/RFC-011-metadata-enrichment.md index 3d18a94..4f471ef 100644 --- a/docs/rfcs/RFC-011-metadata-enrichment.md +++ b/docs/rfcs/RFC-011-metadata-enrichment.md @@ -331,7 +331,10 @@ as if they were. - **The selection is random, and it includes the artist themselves.** The specification reads "a random collection of songs from the given artist *and* similar artists" — not the neighbours alone, which is what the first draft - said. `count` bounds it, and its default is **50**. + said. `count` bounds it, and its default is **50**. *(Revised with #273's + follow-up: the draw takes turns across artists rather than drawing from one + pool of tracks, so an artist with two hundred tracks beside neighbours with + ten each does not fill the answer with their own songs.)* Both are in the compatibility matrix with real callers behind them: Feishin calls `getSimilarSongs`, and without the `.view` suffix, which the router diff --git a/docs/subsonic-api-guide.md b/docs/subsonic-api-guide.md index f4cf2e5..fd83984 100644 --- a/docs/subsonic-api-guide.md +++ b/docs/subsonic-api-guide.md @@ -241,7 +241,7 @@ with Subsonic error code `0`, like every other protocol failure. | Library maintenance | `startScan`, `getScanStatus` | Rescans every library the account owns or manages and reports progress. | | Bookmarks | `getBookmarks`, `createBookmark`, `deleteBookmark` | One position per account and track; setting it again moves it. | | Internet radio | `getInternetRadioStations`, `createInternetRadioStation`, `updateInternetRadioStation`, `deleteInternetRadioStation` | One list **per account**, where the reference server keeps a global one. The three writes answer the bare envelope. `homePageUrl` is returned with a capital `P` and set with `homepageUrl`, as the specification spells them. An address must be absolute `http` or `https`; the server stores it and never connects to it. `homepageUrl=` with no value clears the page rather than failing. | -| Compatibility | `getTopSongs`, `getSimilarSongs`, `getSimilarSongs2` | `getTopSongs` answers the artist's most played tracks from this account's history. The other two take an artist, album or song id, resolved first (error 70 when the caller cannot reach it), and answer a random draw of at most `count` (default 50) from the artist's tracks and their visible neighbours', read from the metadata cache. The standard empty container when no provider is configured, which is the default. | +| Compatibility | `getTopSongs`, `getSimilarSongs`, `getSimilarSongs2` | `getTopSongs` answers the artist's most played tracks from this account's history. The other two take an artist, album or song id (`getSimilarSongs2` documents only an artist id, and accepts all three), resolved first (error 70 when the caller cannot reach it), and answer a random draw of at most `count` (default 50) from the artist's tracks and their visible neighbours', read from the metadata cache. The draw takes turns across artists, one track each, so an artist with many tracks does not fill the answer. The standard empty container when no provider is configured, which is the default. | | Missing data | `getAvatar` | No avatars are stored, so it answers error code 70. | ### Genres diff --git a/src/services/artist_enrichment.rs b/src/services/artist_enrichment.rs index aa31a02..bd4ada4 100644 --- a/src/services/artist_enrichment.rs +++ b/src/services/artist_enrichment.rs @@ -127,6 +127,13 @@ impl DomainServices { /// through `song_select!`, whose membership bind is what keeps a track in /// a library the caller is not in out of the draw. /// + /// **The draw is balanced across names**, not over one pool of tracks: + /// with one pool, an artist with two hundred tracks beside neighbours + /// with ten each fills most of the answer with their own songs, which is + /// not what a client asking for similar songs means. Names take turns, + /// one track each at random, and a name that runs out leaves its turns + /// to the others, so `limit` is still reached while tracks remain. + /// /// With enrichment disabled — the default — the answer is the empty /// container these methods always gave, rather than a draw from the /// artist alone: no operator switched anything on, so nothing on the wire @@ -169,20 +176,45 @@ impl DomainServices { // No library narrowing: every library the caller reaches. let folders = folder_filter(&[]); let mut connection = self.db.pool().acquire().await?; + // Innermost, the pool: every visible track credited to one of the + // names. Each is filed under the name that put it there — its first + // track artist among the names, else its album's first album artist + // among them — and ranked at random inside it. Taking rank 1 of every + // name, then rank 2, and so on, is the round robin; the outermost + // shuffle keeps the answer from playing name by name. The filing + // joins come after the projection in the text, so the user id stays + // the first bind; each picks one row per track or album through + // SQLite's bare column beside `MIN`, which is the row holding it. let mut songs = sqlx::query(concat!( + "SELECT * FROM (SELECT * FROM (SELECT s.*, ROW_NUMBER() OVER (\ + PARTITION BY COALESCE(tn.name, an.name) \ + ORDER BY RANDOM()) AS draw_rank FROM (", song_select!(), song_folder_clause!(), artist_credit_clause!( "SELECT a.id FROM artist a \ WHERE a.canonical_name IN (SELECT value FROM json_each(?))" ), - " ORDER BY RANDOM() LIMIT ?" + ") AS s \ + LEFT JOIN (SELECT tp.track_id, a.canonical_name AS name, MIN(tp.position) \ + FROM track_participant tp JOIN artist a ON a.id=tp.artist_id \ + WHERE tp.role='artist' \ + AND a.canonical_name IN (SELECT value FROM json_each(?)) \ + GROUP BY tp.track_id) tn ON tn.track_id=s.id \ + LEFT JOIN (SELECT ap.album_id, a.canonical_name AS name, MIN(ap.position) \ + FROM album_participant ap JOIN artist a ON a.id=ap.artist_id \ + WHERE ap.role='albumartist' \ + AND a.canonical_name IN (SELECT value FROM json_each(?)) \ + GROUP BY ap.album_id) an ON an.album_id=s.album_id) \ + ORDER BY draw_rank, RANDOM() LIMIT ?) ORDER BY RANDOM()" )) .bind(user_id.to_string()) .bind(folders.as_deref()) .bind(folders.as_deref()) .bind(&names) .bind(&names) + .bind(&names) + .bind(&names) .bind(limit) .fetch_all(&mut *connection) .await? diff --git a/src/subsonic/browse.rs b/src/subsonic/browse.rs index 8f87475..73c1423 100644 --- a/src/subsonic/browse.rs +++ b/src/subsonic/browse.rs @@ -422,9 +422,9 @@ pub(super) async fn top_songs( /// The id is resolved first and for its refusal, so one the caller cannot /// reach is error 70, like one naming nothing. /// -/// A random draw from the artist's own tracks and their visible neighbours', -/// at most `count` (default 50, as specified). With enrichment disabled, the -/// empty container these methods always answered. +/// A random draw, balanced across artists, from the artist's own tracks and +/// their visible neighbours', at most `count` (default 50, as specified). With +/// enrichment disabled, the empty container these methods always answered. pub(super) async fn similar_songs( state: &AppState, principal: &Principal, diff --git a/tests/enrichment.rs b/tests/enrichment.rs index 144ed3c..322ae0e 100644 --- a/tests/enrichment.rs +++ b/tests/enrichment.rs @@ -497,6 +497,19 @@ async fn library_of( owner: uuid::Uuid, name: &str, artists: &[&str], +) { + let credits: Vec<_> = artists.iter().map(|artist| (*artist, *artist)).collect(); + library_with(state, config, owner, name, &credits).await; +} + +/// The same, one track per `(track artist, album artist)` pair, on the album +/// named after its album artist. +async fn library_with( + state: &waveflow_server::AppState, + config: &waveflow_server::Config, + owner: uuid::Uuid, + name: &str, + artists: &[(&str, &str)], ) { let now = waveflow_server::authentication::now_ms(); let root = config.data_dir.join(name); @@ -522,15 +535,16 @@ async fn library_of( .start_scan_job(scan, artists.len() as i64, false) .await .unwrap(); - for (index, artist) in artists.iter().enumerate() { + for (index, (artist, album_artist)) in artists.iter().enumerate() { let mut input = support::browse_input( index + name.len() * 100, "A song", - &format!("{artist} album"), + &format!("{album_artist} album"), artist, Some(1), Some(1), ); + input.album_artist = Some((*album_artist).into()); 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); @@ -977,3 +991,73 @@ async fn count_absent_yields_at_most_fifty() { } enrichment.idle().await; } + +/// The draw is balanced across names, not over one pool of tracks: sixty of +/// Björk's beside five of Sigur Rós's, twenty drawn, and every one of Sigur +/// Rós's five is in — the turns they cannot take go to Björk. One pool would +/// draw about one and a half of them. +#[tokio::test] +async fn a_prolific_artist_does_not_crowd_out_the_neighbours() { + let (_temp, config, state) = support::test_app().await; + let owner = account(&state, "balanced-owner").await; + library_of(&state, &config, owner, "prolific", &["Björk"; 60]).await; + library_of(&state, &config, owner, "sparse", &["Sigur Rós"; 5]).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.similar_songs(owner, bjork, 1).await.unwrap(); + enrichment.idle().await; + + let songs = state + .services + .similar_songs(owner, bjork, 20) + .await + .unwrap(); + let neighbours = songs + .iter() + .filter(|song| song.artist.as_deref() == Some("Sigur Rós")) + .count(); + assert_eq!(songs.len(), 20); + assert_eq!(neighbours, 5); +} + +/// A track is filed under the name that put it in the pool, not under its +/// first track artist: ten tracks on Sigur Rós's album, each by a different +/// guest, are one neighbour's share. Filed by track artist they would be ten +/// names, each taking a turn before Björk's second, and a draw of four would +/// hold one of Björk's at most. +#[tokio::test] +async fn a_neighbour_s_album_of_guests_takes_one_share() { + let (_temp, config, state) = support::test_app().await; + let owner = account(&state, "guests-owner").await; + library_of(&state, &config, owner, "prolific", &["Björk"; 60]).await; + let guests: Vec = (0..10).map(|n| format!("Guest {n}")).collect(); + let credits: Vec<_> = guests + .iter() + .map(|guest| (guest.as_str(), "Sigur Rós")) + .collect(); + library_with(&state, &config, owner, "guests", &credits).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.similar_songs(owner, bjork, 1).await.unwrap(); + enrichment.idle().await; + + let songs = state.services.similar_songs(owner, bjork, 4).await.unwrap(); + let own = songs + .iter() + .filter(|song| song.artist.as_deref() == Some("Björk")) + .count(); + assert_eq!(songs.len(), 4); + assert_eq!(own, 2); +}