From 1126bfcf78bca113cdd8274286da099dc50074a6 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 5 Aug 2026 13:22:30 -0400 Subject: [PATCH] =?UTF-8?q?feat(library):=20genre=20+=20year=20browse=20qu?= =?UTF-8?q?eries=20and=20endpoints=20=E2=80=94=20#367?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Server half of #367. Web UI follows. Genres are exposed AS-IS per the operator: split on the delimiter, trimmed, but no case folding and no synonym mapping. So "Rock" and "rock" appear as separate rows, as does "Rock/Pop" alongside "Rock" and "Pop". The raw spread has to be visible before anyone can judge whether it needs normalising, and the alternative is a mapping table to invent and then maintain. Trimming is not an exception to that. Splitting "Rock; Pop" yields " Pop", and showing that as a genre distinct from "Pop" would be a bug in OUR splitting, not fidelity to the operator's tags. ## The correctness trap this had to avoid ListAlbumsByGenre compared tracks.genre verbatim, while recommendation.sql and discover.sql have always split it on [;,]. Building the browse index by splitting while matching exactly would have listed genres whose pages are empty — every multi-genre track unreachable from either of its genres. So ListAlbumsByGenre now splits too. That also fixes Subsonic getAlbumList?type=byGenre, its only caller, which silently missed every multi-genre track. Its Genre param went *string → string as a result. EXISTS rather than JOIN + DISTINCT ON throughout: the lateral split emits one row per (track, fragment), so a join multiplies rows per album and needs DISTINCT to undo itself. EXISTS asks the question directly, and the count query then matches the list query by construction rather than by coincidence. ## Genre is a query parameter, not a path segment Because "Rock/Pop" is a real ID3 tag — the one the task itself cites — and a slash cannot survive a path segment: Go normalises %2F and the router would split the value in two. So filtering rides GET /api/library/albums?genre=, which also reuses the existing paged album surface instead of adding a parallel one. Endpoints: GET /api/library/genres unpaged index + track counts GET /api/library/years unpaged index + album counts GET /api/library/albums?genre= filtered page GET /api/library/albums?year_from=&year_to= filtered page, either edge open The indexes are unpaged deliberately: a client needs the whole set to render a browsable picker, and paging would let it show only a prefix of an ordering the user didn't choose. Two refusals rather than guesses: genre+year together is a 400 (the UI browses them as separate axes, and quietly dropping half a filter would report a narrower result than it returned), and an inverted year range is a 400 rather than being silently swapped. Undated albums are absent from the year axis rather than bucketed under 0 — "unknown" is not a year, and a 0 row would sort to one end of a chronological list looking like data. Tests: parseYearFilter is pure and runs in the fast lane. The integration tests assert the thing that would otherwise be silently broken — that a "Rock;Pop" track is reachable from BOTH genres, that "Rock/Pop" survives as a filter value, that fragment whitespace is trimmed, and that undated albums stay out of every year range. Reused the existing seedAlbum/seedTrackWithGenre fixtures, which already took exactly the year and genre arguments needed. --- internal/api/api.go | 5 + internal/api/library_albums.go | 177 +++++++++++++-- internal/api/library_browse.go | 65 ++++++ internal/api/library_browse_test.go | 325 ++++++++++++++++++++++++++++ internal/api/library_test.go | 3 + internal/db/dbq/albums.sql.go | 35 ++- internal/db/dbq/browse.sql.go | 268 +++++++++++++++++++++++ internal/db/queries/albums.sql | 27 ++- internal/db/queries/browse.sql | 88 ++++++++ internal/subsonic/browse.go | 6 +- 10 files changed, 969 insertions(+), 30 deletions(-) create mode 100644 internal/api/library_browse.go create mode 100644 internal/api/library_browse_test.go create mode 100644 internal/db/dbq/browse.sql.go create mode 100644 internal/db/queries/browse.sql diff --git a/internal/api/api.go b/internal/api/api.go index e9c481b5..2f9f189e 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -102,6 +102,11 @@ func Mount(r chi.Router, pool *pgxpool.Pool, logger *slog.Logger, events *playev authed.Get("/albums/{id}/cover", h.handleGetCover) authed.Get("/library/shuffle", h.handleLibraryShuffle) authed.Get("/library/albums", h.handleListLibraryAlbums) + // Browse indexes (#367). Genre filtering rides + // /library/albums?genre= rather than a path segment, because raw + // ID3 genres contain slashes ("Rock/Pop") that a path can't carry. + authed.Get("/library/genres", h.handleListGenres) + authed.Get("/library/years", h.handleListAlbumYears) authed.Get("/library/sync", h.handleLibrarySync) authed.Get("/tracks/{id}", h.handleGetTrack) // /tracks/{id}/stream is mounted above with OptionalUser so diff --git a/internal/api/library_albums.go b/internal/api/library_albums.go index cff8e0d6..f480d311 100644 --- a/internal/api/library_albums.go +++ b/internal/api/library_albums.go @@ -1,42 +1,189 @@ package api import ( + "context" + "errors" "net/http" + "net/url" + "strconv" + "strings" "git.fabledsword.com/bvandeusen/minstrel/internal/apierror" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" ) +// Widest plausible bounds for an open-ended year filter. A missing year_from +// means "from the beginning" rather than "from year zero of the query", and +// likewise for year_to, so the caller can filter on one edge only. +const ( + minBrowseYear = 0 + maxBrowseYear = 9999 +) + +var ( + errBadYear = errors.New("year_from and year_to must be integers") + errInvertedYearRange = errors.New("year_from must not be greater than year_to") +) + +// yearFilter carries a parsed, validated inclusive year range. active is false +// when the request asked for no year filtering at all — distinct from a range +// that happens to cover everything, because the two take different code paths. +type yearFilter struct { + from int32 + to int32 + active bool +} + // handleListLibraryAlbums implements GET /api/library/albums. Mirrors // /api/artists?sort=alpha but for albums. The new wrapping-grid page on // the SPA infinite-scrolls against this endpoint via TanStack // createInfiniteQuery. +// +// Optional filters (#367): `genre` and `year_from`/`year_to`. +// +// Genre arrives as a QUERY parameter rather than a path segment on purpose. +// Raw ID3 genres routinely contain a slash — "Rock/Pop" is a real tag, and +// the one the task itself cites — which cannot survive a path segment: Go +// normalises %2F and the router would split the value into two segments. func (h *handlers) handleListLibraryAlbums(w http.ResponseWriter, r *http.Request) { limit, offset, err := parsePaging(r.URL.Query()) if err != nil { writeErr(w, apierror.BadRequest("bad_request", err.Error())) return } - q := dbq.New(h.pool) - rows, err := q.ListAlbumsAlphaWithArtist(r.Context(), dbq.ListAlbumsAlphaWithArtistParams{ - Limit: int32(limit), Offset: int32(offset), - }) + genre := strings.TrimSpace(r.URL.Query().Get("genre")) + years, err := parseYearFilter(r.URL.Query()) if err != nil { - h.logger.Error("api: list library albums", "err", err) + writeErr(w, apierror.BadRequest("bad_request", err.Error())) + return + } + if genre != "" && years.active { + // Refused rather than silently honouring one: the UI browses these as + // separate axes (a genres page, a year filter on the albums page), so + // the combination can only arrive from a caller that has misunderstood + // the contract — and quietly dropping half a filter would report a + // narrower result set than it actually returned. + writeErr(w, apierror.BadRequest("unsupported_filter_combination", + "genre and year filters cannot be combined")) + return + } + + q := dbq.New(h.pool) + var ( + items []AlbumRef + total int64 + ) + switch { + case genre != "": + items, total, err = albumsByGenre(r.Context(), q, genre, limit, offset) + case years.active: + items, total, err = albumsByYear(r.Context(), q, years, limit, offset) + default: + items, total, err = albumsAlpha(r.Context(), q, limit, offset) + } + if err != nil { + h.logger.Error("api: list library albums", "err", err, "genre", genre, "years", years.active) writeErr(w, apierror.InternalMsg("lookup failed", err)) return } - total, err := q.CountAlbums(r.Context()) - if err != nil { - h.logger.Error("api: count albums", "err", err) - writeErr(w, apierror.InternalMsg("count failed", err)) - return - } - items := make([]AlbumRef, 0, len(rows)) - for _, row := range rows { - items = append(items, albumRefFrom(row.Album, row.ArtistName, 0, 0)) - } writeJSON(w, http.StatusOK, Page[AlbumRef]{ Items: items, Total: int(total), Limit: limit, Offset: offset, }) } + +func albumsAlpha( + ctx context.Context, q *dbq.Queries, limit, offset int, +) ([]AlbumRef, int64, error) { + rows, err := q.ListAlbumsAlphaWithArtist(ctx, dbq.ListAlbumsAlphaWithArtistParams{ + Limit: int32(limit), Offset: int32(offset), + }) + if err != nil { + return nil, 0, err + } + total, err := q.CountAlbums(ctx) + if err != nil { + return nil, 0, err + } + items := make([]AlbumRef, 0, len(rows)) + for _, row := range rows { + items = append(items, albumRefFrom(row.Album, row.ArtistName, 0, 0)) + } + return items, total, nil +} + +func albumsByGenre( + ctx context.Context, q *dbq.Queries, genre string, limit, offset int, +) ([]AlbumRef, int64, error) { + rows, err := q.ListAlbumsByGenreWithArtist(ctx, dbq.ListAlbumsByGenreWithArtistParams{ + Genre: genre, Lim: int32(limit), Off: int32(offset), + }) + if err != nil { + return nil, 0, err + } + total, err := q.CountAlbumsByGenre(ctx, genre) + if err != nil { + return nil, 0, err + } + items := make([]AlbumRef, 0, len(rows)) + for _, row := range rows { + items = append(items, albumRefFrom(row.Album, row.ArtistName, 0, 0)) + } + return items, total, nil +} + +func albumsByYear( + ctx context.Context, q *dbq.Queries, years yearFilter, limit, offset int, +) ([]AlbumRef, int64, error) { + rows, err := q.ListAlbumsByYearRangeWithArtist(ctx, + dbq.ListAlbumsByYearRangeWithArtistParams{ + YearFrom: years.from, YearTo: years.to, + Lim: int32(limit), Off: int32(offset), + }) + if err != nil { + return nil, 0, err + } + total, err := q.CountAlbumsByYearRange(ctx, dbq.CountAlbumsByYearRangeParams{ + YearFrom: years.from, YearTo: years.to, + }) + if err != nil { + return nil, 0, err + } + items := make([]AlbumRef, 0, len(rows)) + for _, row := range rows { + items = append(items, albumRefFrom(row.Album, row.ArtistName, 0, 0)) + } + return items, total, nil +} + +// parseYearFilter reads year_from / year_to. Either may be omitted, which +// leaves that edge open — filtering "everything before 1990" shouldn't +// require inventing a lower bound. +func parseYearFilter(raw url.Values) (yearFilter, error) { + fromRaw := strings.TrimSpace(raw.Get("year_from")) + toRaw := strings.TrimSpace(raw.Get("year_to")) + if fromRaw == "" && toRaw == "" { + return yearFilter{}, nil + } + f := yearFilter{from: minBrowseYear, to: maxBrowseYear, active: true} + if fromRaw != "" { + n, err := strconv.Atoi(fromRaw) + if err != nil { + return yearFilter{}, errBadYear + } + f.from = int32(n) + } + if toRaw != "" { + n, err := strconv.Atoi(toRaw) + if err != nil { + return yearFilter{}, errBadYear + } + f.to = int32(n) + } + if f.from > f.to { + // Rejected rather than swapped: silently reordering would return + // results for a range the caller didn't ask for, and an inverted + // range is far more likely a bug than an intent. + return yearFilter{}, errInvertedYearRange + } + return f, nil +} diff --git a/internal/api/library_browse.go b/internal/api/library_browse.go new file mode 100644 index 00000000..d4f237e1 --- /dev/null +++ b/internal/api/library_browse.go @@ -0,0 +1,65 @@ +package api + +import ( + "net/http" + + "git.fabledsword.com/bvandeusen/minstrel/internal/apierror" + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" +) + +// genreCount is one row of the genre browse index (#367). +// +// Genres are the raw ID3 strings, split on [;,] but otherwise untouched — no +// case folding and no synonym mapping. So "Rock" and "rock" can both appear, +// as can "Rock/Pop" alongside "Rock" and "Pop". That's deliberate for v1: the +// alternative is a normalisation table to invent and maintain, and the raw +// spread has to be visible before anyone can judge whether it's a problem. +type genreCount struct { + Genre string `json:"genre"` + TrackCount int `json:"track_count"` +} + +// yearCount is one row of the year browse index. +type yearCount struct { + Year int `json:"year"` + AlbumCount int `json:"album_count"` +} + +// handleListGenres implements GET /api/library/genres. +// +// Unpaged on purpose. Even a messy library yields hundreds of distinct tag +// strings, not thousands, and the client needs the whole set at once to render +// a browsable index — paging it would mean the UI could only ever show a +// prefix of an ordering the user didn't choose. +func (h *handlers) handleListGenres(w http.ResponseWriter, r *http.Request) { + rows, err := dbq.New(h.pool).ListGenresWithCount(r.Context()) + if err != nil { + h.logger.Error("api: list genres", "err", err) + writeErr(w, apierror.InternalMsg("lookup failed", err)) + return + } + out := make([]genreCount, 0, len(rows)) + for _, row := range rows { + out = append(out, genreCount{Genre: row.Genre, TrackCount: int(row.TrackCount)}) + } + writeJSON(w, http.StatusOK, out) +} + +// handleListAlbumYears implements GET /api/library/years. +// +// Albums with no release_date are absent rather than bucketed under 0 — "year +// unknown" isn't a year, and inventing a row for it would put a fake entry at +// one end of a chronological list. +func (h *handlers) handleListAlbumYears(w http.ResponseWriter, r *http.Request) { + rows, err := dbq.New(h.pool).ListAlbumYearsWithCount(r.Context()) + if err != nil { + h.logger.Error("api: list album years", "err", err) + writeErr(w, apierror.InternalMsg("lookup failed", err)) + return + } + out := make([]yearCount, 0, len(rows)) + for _, row := range rows { + out = append(out, yearCount{Year: int(row.Year), AlbumCount: int(row.AlbumCount)}) + } + writeJSON(w, http.StatusOK, out) +} diff --git a/internal/api/library_browse_test.go b/internal/api/library_browse_test.go new file mode 100644 index 00000000..a20386e9 --- /dev/null +++ b/internal/api/library_browse_test.go @@ -0,0 +1,325 @@ +package api + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "net/url" + "testing" +) + +// parseYearFilter is pure, so this runs in the fast lane rather than waiting +// on the integration job. +func TestParseYearFilter(t *testing.T) { + tests := []struct { + name string + query string + wantActive bool + wantFrom int32 + wantTo int32 + wantErr error + }{ + {name: "no params means no filtering", query: "", wantActive: false}, + { + name: "both bounds", query: "year_from=1990&year_to=1999", + wantActive: true, wantFrom: 1990, wantTo: 1999, + }, + { + // "everything from 2000 onward" shouldn't require the caller to + // invent an upper bound. + name: "from only leaves the upper edge open", query: "year_from=2000", + wantActive: true, wantFrom: 2000, wantTo: maxBrowseYear, + }, + { + name: "to only leaves the lower edge open", query: "year_to=1979", + wantActive: true, wantFrom: minBrowseYear, wantTo: 1979, + }, + { + name: "a single year is a degenerate range", query: "year_from=1985&year_to=1985", + wantActive: true, wantFrom: 1985, wantTo: 1985, + }, + {name: "non-numeric from", query: "year_from=nineteen", wantErr: errBadYear}, + {name: "non-numeric to", query: "year_to=x", wantErr: errBadYear}, + { + // Rejected, not silently swapped — reordering would answer a + // question the caller didn't ask. + name: "inverted range", query: "year_from=2000&year_to=1990", + wantErr: errInvertedYearRange, + }, + { + name: "whitespace-only values are treated as absent", + query: "year_from=%20&year_to=%20", wantActive: false, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + raw, err := url.ParseQuery(tc.query) + if err != nil { + t.Fatalf("ParseQuery: %v", err) + } + got, gotErr := parseYearFilter(raw) + if tc.wantErr != nil { + if gotErr != tc.wantErr { + t.Fatalf("error = %v, want %v", gotErr, tc.wantErr) + } + return + } + if gotErr != nil { + t.Fatalf("unexpected error: %v", gotErr) + } + if got.active != tc.wantActive { + t.Errorf("active = %v, want %v", got.active, tc.wantActive) + } + if tc.wantActive && (got.from != tc.wantFrom || got.to != tc.wantTo) { + t.Errorf("range = [%d,%d], want [%d,%d]", + got.from, got.to, tc.wantFrom, tc.wantTo) + } + }) + } +} + +// The crux of #367: a track tagged "Rock;Pop" must be reachable from BOTH +// genres. An exact-string match — which is what ListAlbumsByGenre did before +// this task — makes every multi-genre track invisible from either of its +// genres, so the index would list a genre whose page is empty. +func TestListGenres_SplitsMultiGenreTags(t *testing.T) { + h, pool := testHandlers(t) + artist := seedArtist(t, pool, "Genre Splitter") + album := seedAlbum(t, pool, artist.ID, "Split Album", 1995) + seedTrackWithGenre(t, pool, album.ID, artist.ID, "Both Genres", 1, 200000, "Rock;Pop") + + req := httptest.NewRequest(http.MethodGet, "/api/library/genres", nil) + w := httptest.NewRecorder() + h.handleListGenres(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want 200", w.Code) + } + var got []genreCount + if err := json.NewDecoder(w.Body).Decode(&got); err != nil { + t.Fatalf("decode: %v", err) + } + counts := map[string]int{} + for _, g := range got { + counts[g.Genre] = g.TrackCount + } + for _, want := range []string{"Rock", "Pop"} { + if counts[want] < 1 { + t.Errorf("genre %q missing from index (got %v)", want, counts) + } + } + // The undivided string must NOT appear as its own genre. + if _, ok := counts["Rock;Pop"]; ok { + t.Error(`"Rock;Pop" surfaced as a single genre — the split didn't happen`) + } +} + +// Splitting produces leading spaces on every fragment after the first, and +// showing " Pop" as a genre distinct from "Pop" would be a bug. Trimming is a +// repair for our own splitting, not normalisation of the operator's tags. +func TestListGenres_TrimsFragmentWhitespace(t *testing.T) { + h, pool := testHandlers(t) + artist := seedArtist(t, pool, "Spacey Tags") + album := seedAlbum(t, pool, artist.ID, "Spacey Album", 2001) + seedTrackWithGenre(t, pool, album.ID, artist.ID, "Spaced", 1, 200000, "Jazz; Blues ;") + + req := httptest.NewRequest(http.MethodGet, "/api/library/genres", nil) + w := httptest.NewRecorder() + h.handleListGenres(w, req) + + var got []genreCount + if err := json.NewDecoder(w.Body).Decode(&got); err != nil { + t.Fatalf("decode: %v", err) + } + seen := map[string]bool{} + for _, g := range got { + seen[g.Genre] = true + if g.Genre == "" { + t.Error("empty genre in index — a trailing delimiter leaked through") + } + } + for _, want := range []string{"Jazz", "Blues"} { + if !seen[want] { + t.Errorf("genre %q missing (got %v)", want, keysOf(seen)) + } + } + for _, unwanted := range []string{" Blues", "Blues ", " Blues "} { + if seen[unwanted] { + t.Errorf("untrimmed genre %q present", unwanted) + } + } +} + +// Genre filtering must agree with the index: every genre the index lists has +// to lead to a non-empty page, which is exactly what the old exact-match +// query could not guarantee. +func TestListLibraryAlbums_GenreFilterReachesMultiGenreTracks(t *testing.T) { + h, pool := testHandlers(t) + artist := seedArtist(t, pool, "Reachable") + album := seedAlbum(t, pool, artist.ID, "Reachable Album", 1998) + seedTrackWithGenre(t, pool, album.ID, artist.ID, "Multi", 1, 200000, "Rock;Pop") + + for _, genre := range []string{"Rock", "Pop"} { + t.Run(genre, func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, + "/api/library/albums?genre="+url.QueryEscape(genre), nil) + w := httptest.NewRecorder() + h.handleListLibraryAlbums(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want 200", w.Code) + } + var page Page[AlbumRef] + if err := json.NewDecoder(w.Body).Decode(&page); err != nil { + t.Fatalf("decode: %v", err) + } + if page.Total < 1 { + t.Fatalf("total = %d, want >=1 — genre %q led to an empty page", + page.Total, genre) + } + found := false + for _, a := range page.Items { + if a.Title == "Reachable Album" { + found = true + } + } + if !found { + t.Errorf("seeded album absent from genre %q results", genre) + } + }) + } +} + +// A genre containing a slash is why filtering is a query parameter rather +// than a path segment — "Rock/Pop" cannot survive a path. +func TestListLibraryAlbums_GenreWithSlashSurvives(t *testing.T) { + h, pool := testHandlers(t) + artist := seedArtist(t, pool, "Slashed") + album := seedAlbum(t, pool, artist.ID, "Slashed Album", 2003) + seedTrackWithGenre(t, pool, album.ID, artist.ID, "Slashy", 1, 200000, "Rock/Pop") + + req := httptest.NewRequest(http.MethodGet, + "/api/library/albums?genre="+url.QueryEscape("Rock/Pop"), nil) + w := httptest.NewRecorder() + h.handleListLibraryAlbums(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want 200", w.Code) + } + var page Page[AlbumRef] + if err := json.NewDecoder(w.Body).Decode(&page); err != nil { + t.Fatalf("decode: %v", err) + } + if page.Total < 1 { + t.Errorf(`total = %d, want >=1 for genre "Rock/Pop"`, page.Total) + } +} + +func TestListLibraryAlbums_YearRangeFilter(t *testing.T) { + h, pool := testHandlers(t) + artist := seedArtist(t, pool, "Chronology") + seedAlbum(t, pool, artist.ID, "Old Record", 1972) + seedAlbum(t, pool, artist.ID, "Middle Record", 1995) + seedAlbum(t, pool, artist.ID, "New Record", 2020) + // An undated album must not appear in ANY year range. + seedAlbum(t, pool, artist.ID, "Undated Record", 0) + + titles := func(query string) map[string]bool { + t.Helper() + req := httptest.NewRequest(http.MethodGet, "/api/library/albums?"+query, nil) + w := httptest.NewRecorder() + h.handleListLibraryAlbums(w, req) + if w.Code != http.StatusOK { + t.Fatalf("status = %d for %q, want 200", w.Code, query) + } + var page Page[AlbumRef] + if err := json.NewDecoder(w.Body).Decode(&page); err != nil { + t.Fatalf("decode: %v", err) + } + out := map[string]bool{} + for _, a := range page.Items { + out[a.Title] = true + } + return out + } + + got := titles("year_from=1990&year_to=2000&limit=200") + if !got["Middle Record"] { + t.Error("Middle Record (1995) missing from 1990-2000") + } + for _, absent := range []string{"Old Record", "New Record", "Undated Record"} { + if got[absent] { + t.Errorf("%s present in 1990-2000 range", absent) + } + } + + // Open upper edge. + got = titles("year_from=1990&limit=200") + if !got["Middle Record"] || !got["New Record"] { + t.Error("open-ended year_from should include 1995 and 2020") + } + if got["Old Record"] { + t.Error("Old Record (1972) present in year_from=1990") + } + if got["Undated Record"] { + t.Error("undated album present in an open-ended range") + } +} + +func TestListLibraryAlbums_RejectsGenreAndYearTogether(t *testing.T) { + h, _ := testHandlers(t) + req := httptest.NewRequest(http.MethodGet, + "/api/library/albums?genre=Rock&year_from=1990", nil) + w := httptest.NewRecorder() + h.handleListLibraryAlbums(w, req) + + if w.Code != http.StatusBadRequest { + t.Errorf("status = %d, want 400 for combined filters", w.Code) + } +} + +func TestListAlbumYears_ExcludesUndatedAlbums(t *testing.T) { + h, pool := testHandlers(t) + artist := seedArtist(t, pool, "Years Only") + seedAlbum(t, pool, artist.ID, "Dated One", 1984) + seedAlbum(t, pool, artist.ID, "No Date", 0) + + req := httptest.NewRequest(http.MethodGet, "/api/library/years", nil) + w := httptest.NewRecorder() + h.handleListAlbumYears(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want 200", w.Code) + } + var got []yearCount + if err := json.NewDecoder(w.Body).Decode(&got); err != nil { + t.Fatalf("decode: %v", err) + } + found1984 := false + for _, y := range got { + if y.Year == 1984 { + found1984 = true + } + if y.Year == 0 { + t.Error("year 0 present — undated albums leaked into the index") + } + } + if !found1984 { + t.Error("1984 missing from the year index") + } + // Newest-first ordering, so a picker reads chronologically without the + // client re-sorting. + for i := 1; i < len(got); i++ { + if got[i-1].Year < got[i].Year { + t.Errorf("years not descending at %d: %d then %d", i, got[i-1].Year, got[i].Year) + } + } +} + +func keysOf(m map[string]bool) []string { + out := make([]string, 0, len(m)) + for k := range m { + out = append(out, k) + } + return out +} diff --git a/internal/api/library_test.go b/internal/api/library_test.go index e99ce9fe..7edb4407 100644 --- a/internal/api/library_test.go +++ b/internal/api/library_test.go @@ -475,6 +475,9 @@ func TestRoutesRegisteredInMount(t *testing.T) { "/api/tracks/00000000-0000-0000-0000-000000000001", "/api/tracks/00000000-0000-0000-0000-000000000001/stream", "/api/search?q=x", + // Browse indexes (#367). + "/api/library/genres", + "/api/library/years", } for _, p := range paths { req := httptest.NewRequest(http.MethodGet, p, nil) diff --git a/internal/db/dbq/albums.sql.go b/internal/db/dbq/albums.sql.go index 3ebea2a2..98b07b4a 100644 --- a/internal/db/dbq/albums.sql.go +++ b/internal/db/dbq/albums.sql.go @@ -478,23 +478,40 @@ func (q *Queries) ListAlbumsByArtistWithTrackCount(ctx context.Context, artistID } const listAlbumsByGenre = `-- name: ListAlbumsByGenre :many -SELECT DISTINCT ON (albums.id) albums.id, albums.title, albums.sort_title, albums.artist_id, albums.release_date, albums.mbid, albums.cover_art_path, albums.created_at, albums.updated_at, albums.cover_art_source, albums.cover_art_sources_version +SELECT albums.id, albums.title, albums.sort_title, albums.artist_id, albums.release_date, albums.mbid, albums.cover_art_path, albums.created_at, albums.updated_at, albums.cover_art_source, albums.cover_art_sources_version FROM albums -JOIN tracks ON tracks.album_id = albums.id -WHERE tracks.genre = $1 -ORDER BY albums.id, albums.sort_title -LIMIT $2 OFFSET $3 +WHERE EXISTS ( + SELECT 1 + FROM tracks + JOIN LATERAL regexp_split_to_table(coalesce(tracks.genre, ''), '[;,]') AS g(genre) ON true + WHERE tracks.album_id = albums.id + AND trim(g.genre) = trim($1::text) +) +ORDER BY albums.sort_title, albums.id +LIMIT $3 OFFSET $2 ` type ListAlbumsByGenreParams struct { - Genre *string - Limit int32 - Offset int32 + Genre string + Off int32 + Lim int32 } // Album "belongs to" a genre if any of its tracks carry that genre. +// Serves Subsonic getAlbumList?type=byGenre. +// +// Splits tracks.genre on [;,] as of #367. It previously compared the whole +// column verbatim, so a track tagged "Rock;Pop" was unreachable from EITHER +// "Rock" or "Pop" — a Subsonic client asking for a genre silently missed +// every multi-genre track. This also aligns the endpoint with +// recommendation.sql / discover.sql, which have always split, and with the +// genre browse index that #367 adds. +// +// EXISTS rather than JOIN + DISTINCT ON: the lateral split emits one row per +// (track, genre-fragment), so a join would multiply rows per album and lean +// on DISTINCT to undo it. EXISTS asks the question directly. func (q *Queries) ListAlbumsByGenre(ctx context.Context, arg ListAlbumsByGenreParams) ([]Album, error) { - rows, err := q.db.Query(ctx, listAlbumsByGenre, arg.Genre, arg.Limit, arg.Offset) + rows, err := q.db.Query(ctx, listAlbumsByGenre, arg.Genre, arg.Off, arg.Lim) if err != nil { return nil, err } diff --git a/internal/db/dbq/browse.sql.go b/internal/db/dbq/browse.sql.go new file mode 100644 index 00000000..bbb7fc8e --- /dev/null +++ b/internal/db/dbq/browse.sql.go @@ -0,0 +1,268 @@ +// Code generated by sqlc. DO NOT EDIT. +// versions: +// sqlc v1.31.1 +// source: browse.sql + +package dbq + +import ( + "context" +) + +const countAlbumsByGenre = `-- name: CountAlbumsByGenre :one +SELECT COUNT(*) FROM albums +WHERE EXISTS ( + SELECT 1 + FROM tracks + JOIN LATERAL regexp_split_to_table(coalesce(tracks.genre, ''), '[;,]') AS g(genre) ON true + WHERE tracks.album_id = albums.id + AND trim(g.genre) = trim($1::text) +) +` + +// Total for the paging envelope. EXISTS mirrors the list query exactly; a +// JOIN + DISTINCT here would count differently the moment an album has two +// tracks carrying the same genre. +func (q *Queries) CountAlbumsByGenre(ctx context.Context, genre string) (int64, error) { + row := q.db.QueryRow(ctx, countAlbumsByGenre, genre) + var count int64 + err := row.Scan(&count) + return count, err +} + +const countAlbumsByYearRange = `-- name: CountAlbumsByYearRange :one +SELECT COUNT(*) FROM albums +WHERE release_date IS NOT NULL + AND EXTRACT(YEAR FROM release_date)::int + BETWEEN $1::int AND $2::int +` + +type CountAlbumsByYearRangeParams struct { + YearFrom int32 + YearTo int32 +} + +func (q *Queries) CountAlbumsByYearRange(ctx context.Context, arg CountAlbumsByYearRangeParams) (int64, error) { + row := q.db.QueryRow(ctx, countAlbumsByYearRange, arg.YearFrom, arg.YearTo) + var count int64 + err := row.Scan(&count) + return count, err +} + +const listAlbumYearsWithCount = `-- name: ListAlbumYearsWithCount :many +SELECT EXTRACT(YEAR FROM release_date)::int AS year, COUNT(*)::bigint AS album_count +FROM albums +WHERE release_date IS NOT NULL +GROUP BY year +ORDER BY year DESC +` + +type ListAlbumYearsWithCountRow struct { + Year int32 + AlbumCount int64 +} + +// Year browse index (#367). Only albums with a release_date appear — an +// album with no date isn't "year unknown" as a browsable bucket, it's absent +// from this axis, and the UI says so rather than inventing a 0 row. +// Newest first: recent releases are the likelier browse target. +func (q *Queries) ListAlbumYearsWithCount(ctx context.Context) ([]ListAlbumYearsWithCountRow, error) { + rows, err := q.db.Query(ctx, listAlbumYearsWithCount) + if err != nil { + return nil, err + } + defer rows.Close() + var items []ListAlbumYearsWithCountRow + for rows.Next() { + var i ListAlbumYearsWithCountRow + if err := rows.Scan(&i.Year, &i.AlbumCount); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + +const listAlbumsByGenreWithArtist = `-- name: ListAlbumsByGenreWithArtist :many +SELECT albums.id, albums.title, albums.sort_title, albums.artist_id, albums.release_date, albums.mbid, albums.cover_art_path, albums.created_at, albums.updated_at, albums.cover_art_source, albums.cover_art_sources_version, artists.name AS artist_name +FROM albums +JOIN artists ON artists.id = albums.artist_id +WHERE EXISTS ( + SELECT 1 + FROM tracks + JOIN LATERAL regexp_split_to_table(coalesce(tracks.genre, ''), '[;,]') AS g(genre) ON true + WHERE tracks.album_id = albums.id + AND trim(g.genre) = trim($1::text) +) +ORDER BY albums.sort_title, albums.id +LIMIT $3 OFFSET $2 +` + +type ListAlbumsByGenreWithArtistParams struct { + Genre string + Off int32 + Lim int32 +} + +type ListAlbumsByGenreWithArtistRow struct { + Album Album + ArtistName string +} + +// Albums for one genre, joined with artist_name for the browse grid. +// An album belongs to a genre when ANY of its tracks carry it. Splits and +// trims identically to ListGenresWithCount — if the list is built by +// splitting and the detail matched exactly, every multi-genre track would +// produce a genre row that leads to an empty page. +func (q *Queries) ListAlbumsByGenreWithArtist(ctx context.Context, arg ListAlbumsByGenreWithArtistParams) ([]ListAlbumsByGenreWithArtistRow, error) { + rows, err := q.db.Query(ctx, listAlbumsByGenreWithArtist, arg.Genre, arg.Off, arg.Lim) + if err != nil { + return nil, err + } + defer rows.Close() + var items []ListAlbumsByGenreWithArtistRow + for rows.Next() { + var i ListAlbumsByGenreWithArtistRow + if err := rows.Scan( + &i.Album.ID, + &i.Album.Title, + &i.Album.SortTitle, + &i.Album.ArtistID, + &i.Album.ReleaseDate, + &i.Album.Mbid, + &i.Album.CoverArtPath, + &i.Album.CreatedAt, + &i.Album.UpdatedAt, + &i.Album.CoverArtSource, + &i.Album.CoverArtSourcesVersion, + &i.ArtistName, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + +const listAlbumsByYearRangeWithArtist = `-- name: ListAlbumsByYearRangeWithArtist :many +SELECT albums.id, albums.title, albums.sort_title, albums.artist_id, albums.release_date, albums.mbid, albums.cover_art_path, albums.created_at, albums.updated_at, albums.cover_art_source, albums.cover_art_sources_version, artists.name AS artist_name +FROM albums +JOIN artists ON artists.id = albums.artist_id +WHERE albums.release_date IS NOT NULL + AND EXTRACT(YEAR FROM albums.release_date)::int + BETWEEN $1::int AND $2::int +ORDER BY albums.sort_title, albums.id +LIMIT $4 OFFSET $3 +` + +type ListAlbumsByYearRangeWithArtistParams struct { + YearFrom int32 + YearTo int32 + Off int32 + Lim int32 +} + +type ListAlbumsByYearRangeWithArtistRow struct { + Album Album + ArtistName string +} + +// Albums released within an inclusive year range, for the albums-page filter. +func (q *Queries) ListAlbumsByYearRangeWithArtist(ctx context.Context, arg ListAlbumsByYearRangeWithArtistParams) ([]ListAlbumsByYearRangeWithArtistRow, error) { + rows, err := q.db.Query(ctx, listAlbumsByYearRangeWithArtist, + arg.YearFrom, + arg.YearTo, + arg.Off, + arg.Lim, + ) + if err != nil { + return nil, err + } + defer rows.Close() + var items []ListAlbumsByYearRangeWithArtistRow + for rows.Next() { + var i ListAlbumsByYearRangeWithArtistRow + if err := rows.Scan( + &i.Album.ID, + &i.Album.Title, + &i.Album.SortTitle, + &i.Album.ArtistID, + &i.Album.ReleaseDate, + &i.Album.Mbid, + &i.Album.CoverArtPath, + &i.Album.CreatedAt, + &i.Album.UpdatedAt, + &i.Album.CoverArtSource, + &i.Album.CoverArtSourcesVersion, + &i.ArtistName, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + +const listGenresWithCount = `-- name: ListGenresWithCount :many +SELECT trim(g.genre) AS genre, COUNT(DISTINCT tracks.id)::bigint AS track_count +FROM tracks +JOIN LATERAL regexp_split_to_table(coalesce(tracks.genre, ''), '[;,]') AS g(genre) ON true +WHERE trim(g.genre) <> '' +GROUP BY trim(g.genre) +ORDER BY track_count DESC, trim(g.genre) +` + +type ListGenresWithCountRow struct { + Genre string + TrackCount int64 +} + +// Genre browse index (#367). +// +// Genres live inline on tracks.genre as a delimited string, so this splits on +// the same [;,] pattern already used by recommendation.sql and discover.sql — +// a track tagged "Rock;Pop" must count toward both, and diverging from the +// established pattern here would make the browse surface disagree with what +// the recommendation engine believes the library contains. +// +// trim() but deliberately NO lower(): trimming repairs an artifact of OUR +// splitting ("Rock; Pop" yields " Pop", and showing that as a distinct genre +// would be a bug), whereas case is what the tag actually says. Raw ID3 is +// exposed as-is for v1, so "Rock" and "rock" appear as separate rows. +// +// COUNT(DISTINCT) because a sloppy tag like "Rock;Rock" would otherwise +// inflate its own row. +// +// Ordered by count first: raw ID3 data has a long tail of one-off junk tags, +// so alphabetical would bury the handful of genres an operator actually has a +// library's worth of. Name breaks ties for a stable order. +// Ordered by the expression, not the output alias: `ORDER BY genre` is +// ambiguous between the alias and tracks.genre, and sqlc rejects it. +func (q *Queries) ListGenresWithCount(ctx context.Context) ([]ListGenresWithCountRow, error) { + rows, err := q.db.Query(ctx, listGenresWithCount) + if err != nil { + return nil, err + } + defer rows.Close() + var items []ListGenresWithCountRow + for rows.Next() { + var i ListGenresWithCountRow + if err := rows.Scan(&i.Genre, &i.TrackCount); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} diff --git a/internal/db/queries/albums.sql b/internal/db/queries/albums.sql index 4aaaadd0..6f87dfc8 100644 --- a/internal/db/queries/albums.sql +++ b/internal/db/queries/albums.sql @@ -61,12 +61,29 @@ SELECT * FROM albums ORDER BY random() LIMIT $1; -- name: ListAlbumsByGenre :many -- Album "belongs to" a genre if any of its tracks carry that genre. -SELECT DISTINCT ON (albums.id) albums.* +-- Serves Subsonic getAlbumList?type=byGenre. +-- +-- Splits tracks.genre on [;,] as of #367. It previously compared the whole +-- column verbatim, so a track tagged "Rock;Pop" was unreachable from EITHER +-- "Rock" or "Pop" — a Subsonic client asking for a genre silently missed +-- every multi-genre track. This also aligns the endpoint with +-- recommendation.sql / discover.sql, which have always split, and with the +-- genre browse index that #367 adds. +-- +-- EXISTS rather than JOIN + DISTINCT ON: the lateral split emits one row per +-- (track, genre-fragment), so a join would multiply rows per album and lean +-- on DISTINCT to undo it. EXISTS asks the question directly. +SELECT albums.* FROM albums -JOIN tracks ON tracks.album_id = albums.id -WHERE tracks.genre = $1 -ORDER BY albums.id, albums.sort_title -LIMIT $2 OFFSET $3; +WHERE EXISTS ( + SELECT 1 + FROM tracks + JOIN LATERAL regexp_split_to_table(coalesce(tracks.genre, ''), '[;,]') AS g(genre) ON true + WHERE tracks.album_id = albums.id + AND trim(g.genre) = trim(sqlc.arg(genre)::text) +) +ORDER BY albums.sort_title, albums.id +LIMIT sqlc.arg(lim) OFFSET sqlc.arg(off); -- name: SearchAlbums :many SELECT * FROM albums diff --git a/internal/db/queries/browse.sql b/internal/db/queries/browse.sql new file mode 100644 index 00000000..58002299 --- /dev/null +++ b/internal/db/queries/browse.sql @@ -0,0 +1,88 @@ +-- name: ListGenresWithCount :many +-- Genre browse index (#367). +-- +-- Genres live inline on tracks.genre as a delimited string, so this splits on +-- the same [;,] pattern already used by recommendation.sql and discover.sql — +-- a track tagged "Rock;Pop" must count toward both, and diverging from the +-- established pattern here would make the browse surface disagree with what +-- the recommendation engine believes the library contains. +-- +-- trim() but deliberately NO lower(): trimming repairs an artifact of OUR +-- splitting ("Rock; Pop" yields " Pop", and showing that as a distinct genre +-- would be a bug), whereas case is what the tag actually says. Raw ID3 is +-- exposed as-is for v1, so "Rock" and "rock" appear as separate rows. +-- +-- COUNT(DISTINCT) because a sloppy tag like "Rock;Rock" would otherwise +-- inflate its own row. +-- +-- Ordered by count first: raw ID3 data has a long tail of one-off junk tags, +-- so alphabetical would bury the handful of genres an operator actually has a +-- library's worth of. Name breaks ties for a stable order. +SELECT trim(g.genre) AS genre, COUNT(DISTINCT tracks.id)::bigint AS track_count +FROM tracks +JOIN LATERAL regexp_split_to_table(coalesce(tracks.genre, ''), '[;,]') AS g(genre) ON true +WHERE trim(g.genre) <> '' +GROUP BY trim(g.genre) +-- Ordered by the expression, not the output alias: `ORDER BY genre` is +-- ambiguous between the alias and tracks.genre, and sqlc rejects it. +ORDER BY track_count DESC, trim(g.genre); + +-- name: ListAlbumsByGenreWithArtist :many +-- Albums for one genre, joined with artist_name for the browse grid. +-- An album belongs to a genre when ANY of its tracks carry it. Splits and +-- trims identically to ListGenresWithCount — if the list is built by +-- splitting and the detail matched exactly, every multi-genre track would +-- produce a genre row that leads to an empty page. +SELECT sqlc.embed(albums), artists.name AS artist_name +FROM albums +JOIN artists ON artists.id = albums.artist_id +WHERE EXISTS ( + SELECT 1 + FROM tracks + JOIN LATERAL regexp_split_to_table(coalesce(tracks.genre, ''), '[;,]') AS g(genre) ON true + WHERE tracks.album_id = albums.id + AND trim(g.genre) = trim(sqlc.arg(genre)::text) +) +ORDER BY albums.sort_title, albums.id +LIMIT sqlc.arg(lim) OFFSET sqlc.arg(off); + +-- name: CountAlbumsByGenre :one +-- Total for the paging envelope. EXISTS mirrors the list query exactly; a +-- JOIN + DISTINCT here would count differently the moment an album has two +-- tracks carrying the same genre. +SELECT COUNT(*) FROM albums +WHERE EXISTS ( + SELECT 1 + FROM tracks + JOIN LATERAL regexp_split_to_table(coalesce(tracks.genre, ''), '[;,]') AS g(genre) ON true + WHERE tracks.album_id = albums.id + AND trim(g.genre) = trim(sqlc.arg(genre)::text) +); + +-- name: ListAlbumYearsWithCount :many +-- Year browse index (#367). Only albums with a release_date appear — an +-- album with no date isn't "year unknown" as a browsable bucket, it's absent +-- from this axis, and the UI says so rather than inventing a 0 row. +-- Newest first: recent releases are the likelier browse target. +SELECT EXTRACT(YEAR FROM release_date)::int AS year, COUNT(*)::bigint AS album_count +FROM albums +WHERE release_date IS NOT NULL +GROUP BY year +ORDER BY year DESC; + +-- name: ListAlbumsByYearRangeWithArtist :many +-- Albums released within an inclusive year range, for the albums-page filter. +SELECT sqlc.embed(albums), artists.name AS artist_name +FROM albums +JOIN artists ON artists.id = albums.artist_id +WHERE albums.release_date IS NOT NULL + AND EXTRACT(YEAR FROM albums.release_date)::int + BETWEEN sqlc.arg(year_from)::int AND sqlc.arg(year_to)::int +ORDER BY albums.sort_title, albums.id +LIMIT sqlc.arg(lim) OFFSET sqlc.arg(off); + +-- name: CountAlbumsByYearRange :one +SELECT COUNT(*) FROM albums +WHERE release_date IS NOT NULL + AND EXTRACT(YEAR FROM release_date)::int + BETWEEN sqlc.arg(year_from)::int AND sqlc.arg(year_to)::int; diff --git a/internal/subsonic/browse.go b/internal/subsonic/browse.go index 9bdc9d6f..70225084 100644 --- a/internal/subsonic/browse.go +++ b/internal/subsonic/browse.go @@ -284,8 +284,12 @@ func (b *browseHandlers) getAlbumList2(w http.ResponseWriter, r *http.Request) { WriteFail(w, r, ErrMissingParameter, "Missing required parameter: genre") return } + // Genre is a plain string as of #367 — the query now splits + // tracks.genre on [;,] instead of comparing the whole column, so a + // client asking for "Rock" also reaches tracks tagged "Rock;Pop". + // Previously those were unreachable from either of their genres. albums, err = q.ListAlbumsByGenre(r.Context(), dbq.ListAlbumsByGenreParams{ - Genre: &genre, Limit: int32(size), Offset: int32(offset), + Genre: genre, Lim: int32(size), Off: int32(offset), }) case "recent", "frequent": // Play history lands in M2; return empty to keep clients happy.