From 9509b78073e49a8cc70dce7c71600efbcb0f5563 Mon Sep 17 00:00:00 2001 From: Psychotoxical <171614930+Psychotoxical@users.noreply.github.com> Date: Wed, 15 Jul 2026 10:48:40 +0200 Subject: [PATCH] fix(library): sort albums by the artist the row actually shows (#1217) (#1292) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(library): sort albums by the artist the row actually shows (#1217) The album browse ordered by MAX(t.artist) -- the raw track artist -- while the row mappers derive the displayed artist with pick_album_group_artist, which prefers the album artist. On an album with featured guests the two differ ("Alpha feat. Zulu" vs "Alpha"), so the album sorted under a name the user never sees and fell out of its artist's year run, sometimes landing behind an entirely different artist. Order by the same rule the row displays, via a shared SQL expression (sql_display_artist_from) that both query shapes feed their own columns into: aggregates for the grouped album browse, projected columns for the multi-library dedup path. That dedup path used to build its ORDER BY by string-replacing MAX(t.x) out of the grouped SQL, which only held while every sort key was a bare aggregate and would have silently mangled the new expression -- it now builds directly from its own columns. Regression test uses a featured-guest album and a second artist that sorts between the two spellings; it fails against the old expression (album lands last) and passes with the fix. * docs(changelog): add entry for PR #1292 * fix(library): bind the album sort to the album aggregates in the scoped browse The scoped browse (scope_merge) feeds the same ORDER BY into three query shapes. Two of them GROUP BY t.album_id and selected the sort columns unaliased, so `artist` / `album_artist` in the ORDER BY resolved to bare table columns -- taken from an arbitrary row of the group -- rather than to the MAX() aggregates the row mapper reads. The featured-guest defect (#1217) therefore survived on that path: sorting could still key on a track credit ("Alpha feat. Zulu") instead of the album artist. Alias the sort columns so the ORDER BY binds to the aggregates. Adds a scoped regression test that fails against the previous expression (featured album lands behind a different artist) and passes now. * fix(library): give the scoped GROUP BY browse its own album sort key Review F1. The scoped browse ran `GROUP BY t.album_id` but received the dedup shape's ORDER BY, whose display-artist key is a CASE over bare `artist` / `album_artist`. SQLite substitutes a result alias into ORDER BY only when the whole term is a plain identifier: `ORDER BY artist COLLATE NOCASE` does bind to `MAX(t.artist) AS artist`, but the same name inside a CASE resolves against `track` instead — and a bare column in a grouped query is read from an arbitrary row of the group. Aliasing the select list, the first attempt at this, therefore never fixed the CASE form: an album whose tracks carry `album_artist` unevenly sorted under whichever row SQLite happened to pick. Verified both halves against SQLite directly. Split the parameter: the two `GROUP BY t.album_id` branches now take a grouped key (aggregates inside the CASE), and only the dedup subquery, which really does project plain columns, takes the deduped one. The genre and scope-list callers pass plain-identifier keys, which alias- resolve correctly in either shape, so they hand the same string to both. Tests (F2, F3): - Deterministic guard on the grouped key — no bare column may survive in it. The behavioural tests can pass by luck when the arbitrary row is a favourable one; this one cannot. - Multi-track album with a sparse `album_artist`, the shape the previous single-track tests could not reach. - SQL/Rust parity for the aggregate form of the display-artist rule, over multi-row groups, guarding drift from `pick_album_group_artist`. --- CHANGELOG.md | 6 + .../psysonic-library/src/advanced_search.rs | 275 +++++++++++++++++- .../src/album_compilation_filter.rs | 79 ++++- .../src/genre_album_browse.rs | 3 + .../psysonic-library/src/scope_merge.rs | 41 ++- 5 files changed, 374 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9ee6119a..740a2293 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -369,6 +369,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * The Discord community banner rendered its icon at an enormous size on Windows, pushing the message out of the bar. The icon now has a fixed size on every platform. +### Albums — "Artist / Year" sorting and albums with featured guests + +**By [@Psychotoxical](https://github.com/Psychotoxical), PR [#1292](https://github.com/Psychotoxical/psysonic/pull/1292)** + +* Sorting albums by artist ordered them by the track artist while showing the album artist. On a release with featured guests the two differ, so it was filed under a name that isn't on screen — the album dropped out of its artist's run of years, sometimes behind a different artist entirely. Album sorting now follows the artist the row actually shows. + ### Themes — album rails no longer cut off card shadows **By [@Psychotoxical](https://github.com/Psychotoxical), reported by Asra on the Psysonic Discord, PR [#1300](https://github.com/Psychotoxical/psysonic/pull/1300)** diff --git a/src-tauri/crates/psysonic-library/src/advanced_search.rs b/src-tauri/crates/psysonic-library/src/advanced_search.rs index f5cb9420..aefe40d8 100644 --- a/src-tauri/crates/psysonic-library/src/advanced_search.rs +++ b/src-tauri/crates/psysonic-library/src/advanced_search.rs @@ -14,6 +14,7 @@ use rusqlite::types::Value as SqlValue; use rusqlite::{params, OptionalExtension}; use serde_json::Value; +use crate::album_compilation_filter::sql_display_artist_from; use crate::browse_support::overlay_album_level_starred_at; use crate::dto::{ ArtistCreditMode, LibraryAdvancedSearchRequest, LibraryAdvancedSearchResponse, LibraryAlbumDto, @@ -338,14 +339,19 @@ fn build_layer1_scope_album( None, applied, )?; - let order = deduped_album_order_sql(&req.sort); + // Two shapes, two order clauses: the `GROUP BY t.album_id` branches need the + // aggregates inside the sort key, the outer dedup subquery projects plain + // columns. Sharing one string silently mis-sorted the grouped branches. + let grouped_order = grouped_album_order_sql(&req.sort); + let deduped_order = deduped_album_order_sql(&req.sort); let fast_browse = scopes.len() > 1 && skip_totals && extra_where.trim().is_empty(); scope_merge::list_albums_layer1_filtered( store, scopes, &extra_where, &extra_params, - &order, + &grouped_order, + &deduped_order, limit, offset, skip_totals, @@ -766,16 +772,32 @@ fn multi_scope_track_filter_sql( Ok((w.where_sql(), w.params().to_vec())) } +/// Same sort, built directly against the dedup shape's projected columns. It used +/// to be the grouped SQL with `MAX(t.x)` string-replaced into column names — that +/// only held while every key was a bare aggregate, and would silently mangle the +/// display-artist expression. pub(crate) fn deduped_album_order_sql(sort: &[LibrarySortClause]) -> String { - album_order_from_track_groups(sort) - .map(|s| { - s.replace("MAX(t.album)", "album") - .replace("MAX(t.artist)", "artist") - .replace("MAX(t.year)", "year") - }) + album_order_sql(sort, &AlbumOrderCols::deduped()) .unwrap_or_else(|| "ORDER BY album COLLATE NOCASE ASC, album_id ASC".to_string()) } +/// Same sort for a `GROUP BY t.album_id` shape, with the default fallback the +/// scoped browse needs. +/// +/// The deduped form must NOT be used on a grouped query. Its keys are bare +/// names, and SQLite resolves a bare name inside an expression (our display- +/// artist `CASE`) against the FROM tables, not against a `MAX(...) AS artist` +/// result alias — aliases only substitute when the whole ORDER BY term is a +/// plain identifier. On a grouped query the bare column is then read from an +/// arbitrary row of the group, so an album whose tracks carry `album_artist` +/// unevenly sorts under whichever row SQLite happened to pick. The grouped form +/// puts the aggregates inside the `CASE`, so there is no bare column left to +/// resolve. +pub(crate) fn grouped_album_order_sql(sort: &[LibrarySortClause]) -> String { + album_order_from_track_groups(sort) + .unwrap_or_else(|| "ORDER BY MAX(t.album) COLLATE NOCASE ASC, t.album_id ASC".to_string()) +} + pub(crate) fn deduped_artist_order_sql(sort: &[LibrarySortClause]) -> String { order_clause(sort, EntityKind::Artist) .map(|s| { @@ -2081,16 +2103,48 @@ pub(crate) fn order_clause(sort: &[LibrarySortClause], entity: EntityKind) -> Op } } -/// Sort for album rows aggregated from `track t` (`GROUP BY t.album_id`). -/// Must not reference `album a` — that alias is absent in this query shape. -pub(crate) fn album_order_from_track_groups(sort: &[LibrarySortClause]) -> Option { +/// Column expressions the album sort orders by, per query shape. +/// +/// `artist` must be the **displayed** album artist, not the raw track artist: +/// the row mappers derive it with `pick_album_group_artist` (album-artist first), +/// so ordering by `MAX(t.artist)` sorted by something the user never sees — on a +/// featured-guest album that is "X feat. Z" while the row reads "X", which tore +/// such albums out of their artist's year run (#1217). +struct AlbumOrderCols { + name: &'static str, + artist: String, + year: &'static str, +} + +impl AlbumOrderCols { + /// Rows aggregated from `track t` (`GROUP BY t.album_id`) — the expressions + /// must be aggregates. Must not reference `album a`: absent in this shape. + fn grouped() -> Self { + Self { + name: "MAX(t.album) COLLATE NOCASE", + artist: sql_display_artist_from("MAX(t.artist)", "MAX(t.album_artist)"), + year: "MAX(t.year)", + } + } + + /// Multi-library dedup shape: the outer select projects plain columns. + fn deduped() -> Self { + Self { + name: "album COLLATE NOCASE", + artist: sql_display_artist_from("artist", "album_artist"), + year: "year", + } + } +} + +fn album_order_sql(sort: &[LibrarySortClause], cols: &AlbumOrderCols) -> Option { let mut keys: Vec = Vec::new(); for s in sort { let col = match s.field.as_str() { - "name" => "MAX(t.album) COLLATE NOCASE", - "artist" => "MAX(t.artist) COLLATE NOCASE", - "year" => "MAX(t.year)", - "random" => "RANDOM()", + "name" => cols.name.to_string(), + "artist" => format!("{} COLLATE NOCASE", cols.artist), + "year" => cols.year.to_string(), + "random" => "RANDOM()".to_string(), _ => continue, }; let dir = match s.dir { @@ -2106,6 +2160,11 @@ pub(crate) fn album_order_from_track_groups(sort: &[LibrarySortClause]) -> Optio } } +/// Sort for album rows aggregated from `track t` (`GROUP BY t.album_id`). +pub(crate) fn album_order_from_track_groups(sort: &[LibrarySortClause]) -> Option { + album_order_sql(sort, &AlbumOrderCols::grouped()) +} + /// Allowlist of sortable fields per entity → trusted column expression. /// Unknown sort fields are ignored (fall back to the default order). pub(crate) fn sort_column(field: &str, entity: EntityKind) -> Option<&'static str> { @@ -3345,6 +3404,192 @@ mod tests { assert_eq!(ids, vec!["t2", "t1"]); } + /// #1217: the album browse sorted by `MAX(t.artist)` — the raw *track* artist — + /// while the row displays the *album* artist. On an album with featured guests + /// the two differ ("Alpha feat. Zulu" vs "Alpha"), so the album sorted under a + /// name nobody could see and fell out of its artist's year run, landing after a + /// completely different artist. + #[test] + fn album_artist_year_sort_keeps_featured_guest_albums_with_their_artist() { + let store = LibraryStore::open_in_memory(); + + // Same album artist "Alpha" throughout; only the middle album carries a + // featured-guest track credit. + let mut solo_early = track("s1", "t1", "One", "Alpha", "Early"); + solo_early.year = Some(2000); + + let mut feat = track("s1", "t2", "Two", "Alpha feat. Zulu", "Featured"); + feat.album_artist = Some("Alpha".into()); + feat.year = Some(2001); + + let mut solo_late = track("s1", "t3", "Three", "Alpha", "Late"); + solo_late.year = Some(2002); + + // A second artist that sorts between "Alpha" and "Alpha feat. Zulu". + let mut other = track("s1", "t4", "Four", "Alpha Beta", "Other"); + other.year = Some(1999); + + TrackRepository::new(&store) + .upsert_batch(&[solo_early, feat, solo_late, other]) + .unwrap(); + + let mut r = req("s1", &[EntityKind::Album]); + r.sort = vec![ + LibrarySortClause { field: "artist".into(), dir: SortDir::Asc }, + LibrarySortClause { field: "year".into(), dir: SortDir::Asc }, + ]; + let resp = run_advanced_search(&store, &r).unwrap(); + let names: Vec<&str> = resp.albums.iter().map(|a| a.name.as_str()).collect(); + + // Alpha's three albums stay together in year order; the other artist follows. + // Before the fix the featured album sorted last, behind "Alpha Beta". + assert_eq!(names, vec!["Early", "Featured", "Late", "Other"]); + } + + #[test] + fn album_sorts_order_by_the_displayed_artist_in_both_query_shapes() { + let sort = vec![LibrarySortClause { field: "artist".into(), dir: SortDir::Asc }]; + + let grouped = album_order_from_track_groups(&sort).unwrap(); + assert!(grouped.contains("MAX(t.album_artist)"), "grouped: {grouped}"); + + let deduped = deduped_album_order_sql(&sort); + assert!(deduped.contains("album_artist"), "deduped: {deduped}"); + // The dedup shape has no aggregates to reference. + assert!(!deduped.contains("MAX("), "deduped: {deduped}"); + } + + /// The `GROUP BY t.album_id` shapes must never receive a sort key that leaves a + /// bare column behind. + /// + /// SQLite substitutes a result alias into ORDER BY **only when the whole term is + /// a plain identifier** — `ORDER BY artist COLLATE NOCASE` does bind to + /// `MAX(t.artist) AS artist`, but the same name *inside* our display-artist + /// `CASE` resolves against `track` instead, and a bare column in a grouped query + /// is read from an arbitrary row of the group. Aliasing the select list (the + /// first attempt at this) therefore does not fix the `CASE` form: the album's + /// sort key silently depends on which row SQLite happens to pick. + /// + /// This is the deterministic guard. The behavioural tests below can pass by luck + /// when that arbitrary row happens to be a favourable one; this one cannot. + #[test] + fn grouped_album_order_key_carries_the_aggregates_and_leaves_no_bare_column() { + let sort = vec![LibrarySortClause { field: "artist".into(), dir: SortDir::Asc }]; + let grouped = grouped_album_order_sql(&sort); + + assert!(grouped.contains("MAX(t.album_artist)"), "grouped: {grouped}"); + assert!(grouped.contains("MAX(t.artist)"), "grouped: {grouped}"); + // The deduped form's bare names — the exact thing that must not reach a + // grouped query. + assert!( + !grouped.contains("coalesce(album_artist"), + "bare column left in a grouped sort key: {grouped}", + ); + assert!( + !grouped.contains("coalesce(artist"), + "bare column left in a grouped sort key: {grouped}", + ); + } + + /// Same defect, other query path: with a library scope selected, the browse + /// runs through `scope_merge`, whose `GROUP BY` shapes now get the grouped sort + /// key (aggregates inside the CASE) while only the dedup subquery, which really + /// does project plain columns, gets the deduped one. + #[test] + fn scoped_album_artist_year_sort_also_keeps_featured_guest_albums_in_place() { + let store = LibraryStore::open_in_memory(); + + let mut solo_early = + scoped_track("s1", "t1", "One", "Alpha", "Early", "al_early", "lib1", None, Some(2000), None); + solo_early.album_artist = Some("Alpha".into()); + + let mut feat = scoped_track( + "s1", "t2", "Two", "Alpha feat. Zulu", "Featured", "al_feat", "lib1", None, Some(2001), None, + ); + feat.album_artist = Some("Alpha".into()); + + let mut solo_late = + scoped_track("s1", "t3", "Three", "Alpha", "Late", "al_late", "lib1", None, Some(2002), None); + solo_late.album_artist = Some("Alpha".into()); + + let mut other = + scoped_track("s1", "t4", "Four", "Alpha Beta", "Other", "al_other", "lib1", None, Some(1999), None); + other.album_artist = Some("Alpha Beta".into()); + + TrackRepository::new(&store) + .upsert_batch(&[solo_early, feat, solo_late, other]) + .unwrap(); + + let mut r = req("s1", &[EntityKind::Album]); + r.library_scopes = Some(vec![scope_pair("s1", "lib1")]); + r.sort = vec![ + LibrarySortClause { field: "artist".into(), dir: SortDir::Asc }, + LibrarySortClause { field: "year".into(), dir: SortDir::Asc }, + ]; + let resp = run_advanced_search(&store, &r).unwrap(); + let names: Vec<&str> = resp.albums.iter().map(|a| a.name.as_str()).collect(); + + assert_eq!(names, vec!["Early", "Featured", "Late", "Other"]); + } + + /// The featured album carries **two** tracks and `album_artist` on only one + /// of them — the shape the single-track tests above cannot reach. + /// + /// With one row per group, a bare `artist` / `album_artist` in the ORDER BY + /// is indistinguishable from the `MAX()` aggregate, so an ORDER BY that + /// resolves those names to table columns still sorts correctly by accident. + /// Two rows split them: `MAX(t.album_artist)` is "Alpha" (the display + /// artist), while the group also holds a row whose `album_artist` is NULL + /// and whose `artist` is the feat credit. An ORDER BY reading the bare + /// column can pick that row and sort the album under "Alpha feat. Zulu", + /// tearing it out of Alpha's year run — #1217 all over again, on the + /// scoped path. + #[test] + fn scoped_album_artist_year_sort_handles_sparse_album_artist_within_an_album() { + let store = LibraryStore::open_in_memory(); + + let mut solo_early = + scoped_track("s1", "t1", "One", "Alpha", "Early", "al_early", "lib1", None, Some(2000), None); + solo_early.album_artist = Some("Alpha".into()); + + // Same album, two tracks: the album-artist row first, the feat row second + // (and without an album_artist at all). + let mut feat_titled = scoped_track( + "s1", "t2a", "Two", "Alpha", "Featured", "al_feat", "lib1", None, Some(2001), None, + ); + feat_titled.album_artist = Some("Alpha".into()); + + let mut feat_guest = scoped_track( + "s1", "t2b", "Three", "Alpha feat. Zulu", "Featured", "al_feat", "lib1", None, Some(2001), None, + ); + feat_guest.album_artist = None; + + let mut solo_late = + scoped_track("s1", "t3", "Four", "Alpha", "Late", "al_late", "lib1", None, Some(2002), None); + solo_late.album_artist = Some("Alpha".into()); + + let mut other = + scoped_track("s1", "t4", "Five", "Alpha Beta", "Other", "al_other", "lib1", None, Some(1999), None); + other.album_artist = Some("Alpha Beta".into()); + + TrackRepository::new(&store) + .upsert_batch(&[solo_early, feat_titled, feat_guest, solo_late, other]) + .unwrap(); + + let mut r = req("s1", &[EntityKind::Album]); + r.library_scopes = Some(vec![scope_pair("s1", "lib1")]); + r.sort = vec![ + LibrarySortClause { field: "artist".into(), dir: SortDir::Asc }, + LibrarySortClause { field: "year".into(), dir: SortDir::Asc }, + ]; + let resp = run_advanced_search(&store, &r).unwrap(); + let names: Vec<&str> = resp.albums.iter().map(|a| a.name.as_str()).collect(); + + // "Featured" displays as Alpha, so it belongs inside Alpha's year run — + // not after "Other" under the feat credit. + assert_eq!(names, vec!["Early", "Featured", "Late", "Other"]); + } + // ── multi-library scope (WO-4b) ───────────────────────────────────── fn scope_pair(server: &str, lib: &str) -> crate::dto::LibraryScopePair { diff --git a/src-tauri/crates/psysonic-library/src/album_compilation_filter.rs b/src-tauri/crates/psysonic-library/src/album_compilation_filter.rs index 5e8ab2eb..74f5dd99 100644 --- a/src-tauri/crates/psysonic-library/src/album_compilation_filter.rs +++ b/src-tauri/crates/psysonic-library/src/album_compilation_filter.rs @@ -49,15 +49,28 @@ pub fn various_artists_label(s: &str) -> bool { s.trim().to_ascii_lowercase().contains("various artists") } +/// SQL mirror of [`pick_album_group_artist`] over arbitrary column *expressions* +/// rather than a table alias — the album browse groups by album and therefore has +/// to feed aggregates (`MAX(t.artist)`, `MAX(t.album_artist)`), while the +/// multi-library dedup path feeds projected columns (`artist`, `album_artist`). +/// Single source of the rule; keep in sync with [`pick_album_group_artist`]. +pub fn sql_display_artist_from(track_artist: &str, album_artist: &str) -> String { + format!( + "CASE WHEN trim(coalesce({aa}, '')) != '' \ + THEN trim({aa}) \ + ELSE NULLIF(trim(coalesce({ta}, '')), '') END", + aa = album_artist, + ta = track_artist, + ) +} + /// SQL mirror of [`pick_album_group_artist`] for track-grouped browse subqueries /// (`la`). Used where `ORDER BY` / `COALESCE(a.artist, …)` must stay in SQL; /// keep both implementations in sync. pub fn sql_track_group_display_artist(alias: &str) -> String { - format!( - "CASE WHEN trim(coalesce({a}.album_artist, '')) != '' \ - THEN trim({a}.album_artist) \ - ELSE NULLIF(trim(coalesce({a}.artist, '')), '') END", - a = alias + sql_display_artist_from( + &format!("{alias}.artist"), + &format!("{alias}.album_artist"), ) } @@ -161,4 +174,60 @@ mod tests { ); } } + + /// Same parity, but for the **aggregate** form the grouped album browse sorts on. + /// + /// `map_album_from_tracks` builds a row's display artist as + /// `pick_album_group_artist(MAX(artist), MAX(album_artist))`, so the sort key + /// must be `sql_display_artist_from("MAX(t.artist)", "MAX(t.album_artist)")` over + /// the same aggregates — anything else sorts the album under a name the row does + /// not show (#1217). The multi-row groups here are the point: a single row cannot + /// tell an aggregate apart from a bare column. + #[test] + fn sql_display_artist_from_aggregates_matches_pick_album_group_artist() { + let conn = rusqlite::Connection::open_in_memory().unwrap(); + conn.execute("CREATE TABLE t (artist TEXT, album_artist TEXT)", []) + .unwrap(); + let sql = format!( + "SELECT {} FROM t", + sql_display_artist_from("MAX(t.artist)", "MAX(t.album_artist)"), + ); + + // Each case is one album's worth of tracks — album_artist deliberately sparse. + let groups: [&[(&str, Option<&str>)]; 5] = [ + // Featured guest on one track only; the album artist is what shows. + &[("Alpha", Some("Alpha")), ("Alpha feat. Zulu", None)], + // Album artist on the *second* track — MAX still has to find it. + &[("Alpha feat. Zulu", None), ("Alpha", Some("Alpha"))], + // No album artist anywhere: falls back to the track credit. + &[("Alpha", None), ("Alpha feat. Zulu", None)], + // Blank strings must not count as an album artist. + &[("Alice", Some(" ")), ("Alice feat. Bob", Some(""))], + // Compilation: every track carries the same album artist. + &[("Alice", Some("Various Artists")), ("Bob", Some("Various Artists"))], + ]; + + for rows in groups { + conn.execute("DELETE FROM t", []).unwrap(); + for (artist, album_artist) in rows { + conn.execute( + "INSERT INTO t (artist, album_artist) VALUES (?1, ?2)", + rusqlite::params![artist, album_artist], + ) + .unwrap(); + } + let sql_out: Option = conn.query_row(&sql, [], |r| r.get(0)).unwrap(); + + // The Rust side of the same decision, over the same aggregates. + let max_artist = rows.iter().map(|(a, _)| *a).max().map(str::to_string); + let max_album_artist = rows + .iter() + .filter_map(|(_, aa)| *aa) + .max() + .map(str::to_string); + let rust_out = pick_album_group_artist(max_artist, max_album_artist); + + assert_eq!(sql_out, rust_out, "rows={rows:?}"); + } + } } diff --git a/src-tauri/crates/psysonic-library/src/genre_album_browse.rs b/src-tauri/crates/psysonic-library/src/genre_album_browse.rs index 32b6eb83..4fe93a82 100644 --- a/src-tauri/crates/psysonic-library/src/genre_album_browse.rs +++ b/src-tauri/crates/psysonic-library/src/genre_album_browse.rs @@ -256,6 +256,8 @@ fn list_albums_by_genre_layer1_scope( WHERE tg.server_id = t.server_id AND tg.track_id = t.id \ AND tg.genre = ? COLLATE NOCASE)"; let extra_params = vec![SqlValue::Text(genre.to_string())]; + // Plain-identifier keys, so the same string is correct for both the grouped and + // the dedup shape (SQLite resolves a bare ORDER BY name to the result alias). let order = genre_multi_scope_order_sql(&req.sort); let (albums, total_count) = scope_merge::list_albums_layer1_filtered( store, @@ -263,6 +265,7 @@ fn list_albums_by_genre_layer1_scope( extra_where, &extra_params, &order, + &order, limit, offset, !req.include_total, diff --git a/src-tauri/crates/psysonic-library/src/scope_merge.rs b/src-tauri/crates/psysonic-library/src/scope_merge.rs index b3bf9e05..a412f416 100644 --- a/src-tauri/crates/psysonic-library/src/scope_merge.rs +++ b/src-tauri/crates/psysonic-library/src/scope_merge.rs @@ -266,12 +266,17 @@ pub fn list_albums( let limit = clamp_limit(request.limit); let offset = clamp_offset(request.offset); if crate::dto::scoped_layer1_eligible(scopes) { + // Plain-identifier keys (`ORDER BY artist COLLATE NOCASE`), which SQLite + // resolves to the `MAX(...) AS x` aliases in the grouped shape and to the + // projected columns in the dedup shape — correct either way, so one string + // serves both. let (albums, _) = list_albums_layer1_filtered( store, scopes, "", &[], &order, + &order, limit, offset, true, @@ -385,7 +390,14 @@ pub(crate) fn list_albums_layer1_filtered( scopes: &[LibraryScopePair], extra_where: &str, extra_params: &[SqlValue], - order_sql: &str, + // `GROUP BY t.album_id` shapes. A sort key that is a plain identifier may be + // passed as-is (SQLite resolves it to the `MAX(...) AS x` result alias), but a + // key that wraps the name in an expression — our display-artist `CASE` — must + // carry the aggregates itself, or the name resolves to the table column and is + // read from an arbitrary row of the group. + grouped_order_sql: &str, + // Dedup shape: the outer select projects plain columns, so plain names are right. + deduped_order_sql: &str, limit: u32, offset: u32, skip_totals: bool, @@ -411,13 +423,19 @@ pub(crate) fn list_albums_layer1_filtered( params.extend_from_slice(extra_params); let count_sql = format!("SELECT COUNT(DISTINCT t.album_id) FROM track t WHERE {where_sql}"); + // Grouped shape: the ORDER BY must carry the aggregates itself. Aliasing the + // sort columns is not enough — SQLite substitutes a result alias only when the + // whole ORDER BY term is a plain identifier, so a bare name inside the + // display-artist CASE would resolve to the table column and be read from an + // arbitrary row of the group. let sql = format!( - "SELECT t.server_id, t.album_id, MAX(t.album), MAX(t.artist), MAX(t.artist_id), \ - MAX(t.album_artist), COUNT(*), SUM(t.duration_sec), MAX(t.year), MAX(t.genre), \ + "SELECT t.server_id, t.album_id, MAX(t.album) AS album, MAX(t.artist) AS artist, \ + MAX(t.artist_id), MAX(t.album_artist) AS album_artist, COUNT(*), \ + SUM(t.duration_sec), MAX(t.year) AS year, MAX(t.genre), \ MAX(t.cover_art_id), MAX(t.starred_at), MAX(t.synced_at) \ FROM track t WHERE {where_sql} \ GROUP BY t.album_id \ - {order_sql} \ + {grouped_order_sql} \ LIMIT ? OFFSET ?" ); let total = if skip_totals { @@ -461,13 +479,15 @@ pub(crate) fn list_albums_layer1_filtered( params.push(SqlValue::Text(p.library_id.clone())); } let count_sql = format!("SELECT COUNT(DISTINCT t.album_id) FROM track t WHERE {where_sql}"); + // Grouped shape — same reasoning as the single-scope branch above. let sql = format!( - "SELECT t.server_id, t.album_id, MAX(t.album), MAX(t.artist), MAX(t.artist_id), \ - MAX(t.album_artist), COUNT(*), SUM(t.duration_sec), MAX(t.year), MAX(t.genre), \ + "SELECT t.server_id, t.album_id, MAX(t.album) AS album, MAX(t.artist) AS artist, \ + MAX(t.artist_id), MAX(t.album_artist) AS album_artist, COUNT(*), \ + SUM(t.duration_sec), MAX(t.year) AS year, MAX(t.genre), \ MAX(t.cover_art_id), MAX(t.starred_at), MAX(t.synced_at) \ FROM track t WHERE {where_sql} \ GROUP BY t.album_id \ - {order_sql} \ + {grouped_order_sql} \ LIMIT ? OFFSET ?" ); let total = if skip_totals { @@ -534,7 +554,7 @@ pub(crate) fn list_albums_layer1_filtered( MIN(_pick) AS _pick \ FROM per_lib GROUP BY album_dedup \ ) \ - {order_sql} \ + {deduped_order_sql} \ LIMIT ? OFFSET ?" ), ); @@ -2538,10 +2558,11 @@ mod tests { let order = "ORDER BY album COLLATE NOCASE ASC, album_id ASC".to_string(); let bench = |label: &str, scopes: &[LibraryScopePair]| { - let _ = list_albums_layer1_filtered(&store, scopes, "", &[], &order, 100, 0, true, false); + let _ = + list_albums_layer1_filtered(&store, scopes, "", &[], &order, &order, 100, 0, true, false); let start = Instant::now(); let (rows, _) = list_albums_layer1_filtered( - &store, scopes, "", &[], &order, 100, 0, true, false, + &store, scopes, "", &[], &order, &order, 100, 0, true, false, ) .unwrap(); println!(" {label}: {:?} ({} albums)", start.elapsed(), rows.len());