From d446de031f4e37d96ea4b82f89fdfa102cc86da6 Mon Sep 17 00:00:00 2001 From: Psychotoxical <171614930+Psychotoxical@users.noreply.github.com> Date: Wed, 22 Jul 2026 22:18:26 +0200 Subject: [PATCH] fix(library): group artist detail releases by type again (#1329) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(library): group artist detail releases by type again The multi-scope artist detail builds albums from tracks and keeps the album raw_json NULL, so the DTO lost the releaseTypes the network getArtist used to provide and the artist page collapsed its Albums / Singles / EPs / Live / Compilations grouping into one flat list. Derive each album release type set from a representative track raw_json.tags.releasetype (Navidrome MusicBrainz RELEASETYPE tag), order preserved, so the existing frontend grouping works again. * docs(changelog): note artist detail release-type grouping fix (#1329) * fix(library): also read top-level OpenSubsonic release types * fix(library): validate artist release-type candidates before precedence Re-review follow-up. The release-type lookup treated any non-null JSON value as a candidate before validating it, which had three effects: - an empty or malformed top-level OpenSubsonic `releaseTypes` (the S2 ingest copies empty album arrays verbatim) suppressed a valid nested `tags.releasetype`, leaving the album ungrouped; - the representative-track subquery stopped at the first merely non-null value with no ordering, so an unusable track could hide a valid one on the same album, non-deterministically; - only "non-empty array" was checked, so non-string members (e.g. `["EP",null]`) reached ArtistDetail.tsx, where `r.toLowerCase()` throws during render. Select only a non-empty array of strings, validated per representation before precedence and before `LIMIT 1`, with a deterministic order; the top-level API field stays preferred when itself usable, else it falls back to the nested one. The row now reuses the shared `map_album_list_row` + `album_row_to_dto` mapper and attaches the validated array, removing the duplicate DTO construction path. Adds regression coverage for empty top-level, hidden-valid-track, and non-string members. * fix(library): guard release-type lookup against malformed track raw_json Follow-up review finding. `usable_release_types_expr` called SQLite JSON1 functions (json_type/json_each/json_array_length/json_extract) directly on track.raw_json, which is unconstrained text the library tolerates as invalid (LibraryTrackDto::from_row maps invalid JSON to Value::Null). Inside the per-album correlated lookup, one malformed row raised `malformed JSON` and aborted the whole artist-detail query before a later valid track could win. Wrap candidate evaluation in a lazy `CASE WHEN json_valid(raw_json)` guard so a malformed row contributes no release types. Adds a regression test with a malformed earlier track and a valid later track on one album — it fails with `malformed JSON` without the guard. --- CHANGELOG.md | 6 + .../psysonic-library/src/scope_merge.rs | 244 +++++++++++++++++- 2 files changed, 247 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e5735292..36d7309d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -101,6 +101,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * The artist detail header no longer adopts a per-track "featuring" credit when a single track in the discography is tagged with a guest artist. It shows the artist's own name, matching the artist browse list. +### Artist details — group releases by type again + +**By [@Psychotoxical](https://github.com/Psychotoxical), PR [#1329](https://github.com/Psychotoxical/psysonic/pull/1329)** + +* The artist page groups a discography into Albums, Singles, EPs, Live and Compilations again instead of one flat list. The release type is read from each album's tags; releases without a type tag stay in the default group. + ### Multi-server library scope — recover empty persisted selections **By [@cucadmuh](https://github.com/cucadmuh), PR [#1331](https://github.com/Psychotoxical/psysonic/pull/1331)** diff --git a/src-tauri/crates/psysonic-library/src/scope_merge.rs b/src-tauri/crates/psysonic-library/src/scope_merge.rs index 7dad9db4..7b456bfd 100644 --- a/src-tauri/crates/psysonic-library/src/scope_merge.rs +++ b/src-tauri/crates/psysonic-library/src/scope_merge.rs @@ -2187,6 +2187,41 @@ fn fetch_artist_candidates( Ok(rows) } +/// SQL expression selecting a track's *usable* release-type array from `raw_json`, +/// or NULL when neither representation is usable. A candidate is usable only when it +/// is a non-empty JSON array whose members are all strings; that check is applied to +/// each representation *before* precedence, so an empty or malformed top-level +/// OpenSubsonic `releaseTypes` (the ingest copies empty album arrays verbatim) cannot +/// suppress a valid Navidrome-native `tags.releasetype`, and a non-string member +/// cannot survive to the frontend, where `ArtistDetail.tsx` lowercases each entry. +/// The top-level API field stays preferred when it is itself usable. +/// +/// The whole expression is wrapped in a lazy `CASE WHEN json_valid(...)` guard: +/// `track.raw_json` is unconstrained text and the library tolerates invalid JSON +/// (`LibraryTrackDto::from_row` maps it to `Value::Null`), but the JSON1 functions +/// (`json_type`/`json_array_length`/`json_each`/`json_extract`) raise `malformed JSON` +/// on invalid text — which, inside this per-album correlated lookup, would abort the +/// entire artist-detail query instead of skipping the bad row. The guard makes a +/// malformed row contribute no release types, so a later valid track still wins. +fn usable_release_types_expr(json_col: &str) -> String { + let candidate = |path: &str| { + format!( + "CASE WHEN json_type({c}, '{p}') = 'array' \ + AND json_array_length({c}, '{p}') > 0 \ + AND NOT EXISTS (SELECT 1 FROM json_each({c}, '{p}') je WHERE je.type <> 'text') \ + THEN json_extract({c}, '{p}') END", + c = json_col, + p = path, + ) + }; + format!( + "CASE WHEN json_valid({c}) THEN COALESCE({top}, {nested}) END", + c = json_col, + top = candidate("$.releaseTypes"), + nested = candidate("$.tags.releasetype"), + ) +} + fn fetch_albums_for_artist_key( conn: &rusqlite::Connection, scopes: &[LibraryScopePair], @@ -2195,6 +2230,7 @@ fn fetch_albums_for_artist_key( anchor_artist_id: &str, ) -> rusqlite::Result> { let (scope_cte, scope_binds) = scope_cte_sql(scopes); + let release_types_expr = usable_release_types_expr("tt.raw_json"); let (cte, scoped, key_filter, priority) = keyed_detail_track_source( scope_cte, artist_key.map(|_| "artist_key"), @@ -2236,7 +2272,13 @@ fn fetch_albums_for_artist_key( FROM physical_tracks b \ ) \ SELECT p.server_id, p.album_id, p.album, p.artist, p.artist_id, p.album_artist, \ - st.song_count, st.duration_total, p.year, p.genre, p.cover_art_id, p.starred_at, p.synced_at \ + st.song_count, st.duration_total, p.year, p.genre, p.cover_art_id, p.starred_at, p.synced_at, \ + (SELECT {release_types_expr} \ + FROM track tt \ + WHERE tt.server_id = p.server_id AND tt.album_id = p.album_id AND tt.deleted = 0 \ + AND {release_types_expr} IS NOT NULL \ + ORDER BY tt.id ASC \ + LIMIT 1) AS release_types \ FROM album_pick p \ INNER JOIN album_stats st ON p.album_dedup = st.album_dedup \ WHERE p.rn = 1 \ @@ -2250,11 +2292,36 @@ fn fetch_albums_for_artist_key( binds.push(SqlValue::Text(anchor_server.to_string())); binds.push(SqlValue::Text(anchor_artist_id.to_string())); } + // The bulk album pipeline keeps album `raw_json` NULL and the standalone album + // table is unused, so the DTO would otherwise carry no `releaseTypes` and the + // artist page could no longer group releases (Albums / Singles / EPs / Live / + // Compilations) — it collapses to one flat list. Two ingest paths store the + // MusicBrainz RELEASETYPE tag differently: Navidrome-native rows keep it per + // track under `raw_json.tags.releasetype`, while the OpenSubsonic/S2 crawl copies + // the album-level array onto each track at top-level `raw_json.releaseTypes` + // (see `merge_album_open_subsonic_track_raw`). `usable_release_types_expr` picks a + // validated array (`release_types`, column 13); reuse the shared album mapper and + // attach it, so there is one album-DTO construction path. let mut stmt = conn.prepare(&sql)?; let rows = stmt - .query_map(params_from_iter(binds.iter()), map_album_list_row)? + .query_map(params_from_iter(binds.iter()), |r| { + let mut dto = album_row_to_dto(map_album_list_row(r)?); + // SQL already guarantees a non-empty array of strings, or NULL; the + // client-side re-check is a cheap invariant guard, not new filtering. + dto.raw_json = r + .get::<_, Option>(13)? + .and_then(|s| serde_json::from_str::(&s).ok()) + .filter(|v| v.as_array().is_some_and(|a| !a.is_empty())) + .map(|types| { + let mut obj = serde_json::Map::new(); + obj.insert("releaseTypes".to_string(), types); + Value::Object(obj) + }) + .unwrap_or(Value::Null); + Ok(dto) + })? .collect::>>()?; - Ok(rows.into_iter().map(album_row_to_dto).collect()) + Ok(rows) } fn fetch_scope_deduped_tracks_for_artist_key( @@ -2822,6 +2889,177 @@ mod tests { assert_eq!(with_tracks.tracks[0].id, "t1"); } + #[test] + fn artist_detail_albums_carry_release_types_for_grouping() { + // Regression (#1326): the artist page groups a discography into Albums / + // Singles / EPs / Live / Compilations from each album's `releaseTypes`. The + // multi-scope pipeline builds albums from tracks and keeps album `raw_json` + // NULL, so the release types must come from the tracks' raw JSON (order + // preserved), or grouping goes flat. Two ingest paths store them differently + // and both must work: Navidrome-native `raw_json.tags.releasetype` and the + // OpenSubsonic/S2 top-level `raw_json.releaseTypes` + // (`merge_album_open_subsonic_track_raw`). Albums with neither stay null. + let store = LibraryStore::open_in_memory(); + // Native Navidrome shape. + let mut native = track( + "s1", "t1", "Song", Some("Artist"), "A Live EP", "alb1", + Some("art1"), 200, "lib-a", Some(2020), None, None, + ); + native.raw_json = r#"{"tags":{"releasetype":["Single","Live"]}}"#.into(); + // OpenSubsonic/S2 shape: album-level array copied onto the track top-level. + let mut s2 = track( + "s1", "t2", "Song", Some("Artist"), "B Compilation EP", "alb2", + Some("art1"), 200, "lib-a", Some(2021), None, None, + ); + s2.raw_json = r#"{"releaseTypes":["EP"]}"#.into(); + // Neither representation → default (null) group. + let plain = track( + "s1", "t3", "Song", Some("Artist"), "C Plain Album", "alb3", + Some("art1"), 200, "lib-a", Some(2022), None, None, + ); + seed_and_rebuild(&store, &[native, s2, plain]); + + let response = artist_detail( + &store, + &LibraryScopeArtistDetailRequest { + scopes: vec![scope_pair("s1", "lib-a")], + artist_id: "art1".into(), + server_id: "s1".into(), + include_tracks: false, + top_tracks_limit: None, + }, + ) + .unwrap(); + + assert_eq!(response.albums.len(), 3); + let by_id = |id: &str| { + response + .albums + .iter() + .find(|a| a.id == id) + .unwrap_or_else(|| panic!("album {id} missing")) + }; + // Native tag order preserved. + assert_eq!(by_id("alb1").raw_json["releaseTypes"][0], "Single"); + assert_eq!(by_id("alb1").raw_json["releaseTypes"][1], "Live"); + // S2 top-level array surfaced. + assert_eq!(by_id("alb2").raw_json["releaseTypes"][0], "EP"); + // No release types anywhere → null raw_json, so grouping falls back cleanly. + assert!(by_id("alb3").raw_json.is_null()); + } + + #[test] + fn artist_detail_release_types_reject_unusable_candidates() { + // Release-type candidates must be validated (non-empty array of strings) + // before precedence and before the representative-track `LIMIT 1`, or bad + // server metadata leaves valid albums ungrouped and can crash the artist page. + let store = LibraryStore::open_in_memory(); + // (1) Empty top-level array must not suppress the valid nested value. + let mut empty_top = track( + "s1", "et1", "Song", Some("Artist"), "Empty Top", "alb-empty", + Some("art1"), 200, "lib-a", Some(2020), None, None, + ); + empty_top.raw_json = r#"{"releaseTypes":[],"tags":{"releasetype":["EP"]}}"#.into(); + // (2) An unusable earlier track must not hide a valid later track on the same + // album. `hid1` sorts before `hid2`; only `hid2` carries a usable array. + let mut hidden_bad = track( + "s1", "hid1", "First", Some("Artist"), "Hidden", "alb-hidden", + Some("art1"), 200, "lib-a", Some(2021), None, None, + ); + hidden_bad.raw_json = r#"{"releaseTypes":[]}"#.into(); + let mut hidden_good = track( + "s1", "hid2", "Second", Some("Artist"), "Hidden", "alb-hidden", + Some("art1"), 210, "lib-a", Some(2021), None, None, + ); + hidden_good.raw_json = r#"{"tags":{"releasetype":["Album","Live"]}}"#.into(); + // (3a) Non-string members with no usable fallback → no release types at all. + let mut nonstring = track( + "s1", "ns1", "Song", Some("Artist"), "Non String", "alb-nonstring", + Some("art1"), 200, "lib-a", Some(2022), None, None, + ); + nonstring.raw_json = r#"{"releaseTypes":["EP",null]}"#.into(); + // (3b) Non-string top-level → fall back to the valid nested value. + let mut nonstring_fallback = track( + "s1", "nsf1", "Song", Some("Artist"), "Non String Fallback", "alb-nsfb", + Some("art1"), 200, "lib-a", Some(2023), None, None, + ); + nonstring_fallback.raw_json = + r#"{"releaseTypes":["Live",1],"tags":{"releasetype":["Album"]}}"#.into(); + seed_and_rebuild( + &store, + &[empty_top, hidden_bad, hidden_good, nonstring, nonstring_fallback], + ); + + let response = artist_detail( + &store, + &LibraryScopeArtistDetailRequest { + scopes: vec![scope_pair("s1", "lib-a")], + artist_id: "art1".into(), + server_id: "s1".into(), + include_tracks: false, + top_tracks_limit: None, + }, + ) + .unwrap(); + let by_id = |id: &str| { + response + .albums + .iter() + .find(|a| a.id == id) + .unwrap_or_else(|| panic!("album {id} missing")) + }; + // Empty top-level did not suppress the nested value. + assert_eq!(by_id("alb-empty").raw_json["releaseTypes"][0], "EP"); + // The valid later track won over the unusable earlier one, order preserved. + assert_eq!(by_id("alb-hidden").raw_json["releaseTypes"][0], "Album"); + assert_eq!(by_id("alb-hidden").raw_json["releaseTypes"][1], "Live"); + // Non-string members with no fallback → null (never reaches the frontend). + assert!(by_id("alb-nonstring").raw_json.is_null()); + // Non-string top-level fell back to the valid nested array. + assert_eq!(by_id("alb-nsfb").raw_json["releaseTypes"][0], "Album"); + } + + #[test] + fn artist_detail_release_types_tolerate_malformed_raw_json() { + // `track.raw_json` is unconstrained text and the library tolerates invalid + // JSON (from_row → Value::Null). The release-type lookup must not let a + // malformed row raise `malformed JSON` and abort the whole artist-detail + // query: the bad row contributes nothing and a later valid track still wins. + let store = LibraryStore::open_in_memory(); + // Malformed row sorts before the valid one, so an unguarded query would hit + // it first and error out. + let mut bad = track( + "s1", "aa-bad", "Broken", Some("Artist"), "Mixed", "alb-mixed", + Some("art1"), 200, "lib-a", Some(2020), None, None, + ); + bad.raw_json = "{not valid json".into(); + let mut good = track( + "s1", "zz-good", "Fine", Some("Artist"), "Mixed", "alb-mixed", + Some("art1"), 210, "lib-a", Some(2020), None, None, + ); + good.raw_json = r#"{"tags":{"releasetype":["EP"]}}"#.into(); + seed_and_rebuild(&store, &[bad, good]); + + let response = artist_detail( + &store, + &LibraryScopeArtistDetailRequest { + scopes: vec![scope_pair("s1", "lib-a")], + artist_id: "art1".into(), + server_id: "s1".into(), + include_tracks: false, + top_tracks_limit: None, + }, + ) + .unwrap(); + + let album = response + .albums + .iter() + .find(|a| a.id == "alb-mixed") + .expect("album missing"); + assert_eq!(album.raw_json["releaseTypes"][0], "EP"); + } + #[test] fn artist_detail_name_uses_canonical_artist_not_feature_track_credit() { // Regression: a single guest-feature track in a discography carries a