mirror of
https://github.com/Psychotoxical/psysonic.git
synced 2026-07-21 14:55:43 +00:00
* 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`.
This commit is contained in:
@@ -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)**
|
||||
|
||||
@@ -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<String> {
|
||||
/// 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<String> {
|
||||
let mut keys: Vec<String> = 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<String> {
|
||||
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 {
|
||||
|
||||
@@ -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<String> = 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:?}");
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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());
|
||||
|
||||
Reference in New Issue
Block a user