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.