From f3b124f9b2eb9cea4c15fd53a12bfcacc74a5ae1 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 23 Oct 2026 13:58:09 -0400 Subject: [PATCH] feat(subsonic): getAlbumList2 recent and frequent read the caller's play history (#6293) type=recent orders the caller's albums by their latest play (skips included); type=frequent orders them by completed plays (skips excluded). Plays of tracks whose file is gone do not count, and both page by size/offset like the other types. Co-Authored-By: Claude Opus 5.5 --- internal/db/dbq/albums.sql.go | 130 ++++++++++++++++++++ internal/db/queries/albums.sql | 50 ++++++++ internal/subsonic/albumlist_test.go | 178 ++++++++++++++++++++++++++++ internal/subsonic/browse.go | 21 +++- 4 files changed, 375 insertions(+), 4 deletions(-) create mode 100644 internal/subsonic/albumlist_test.go diff --git a/internal/db/dbq/albums.sql.go b/internal/db/dbq/albums.sql.go index 81dc812a..fc3c8768 100644 --- a/internal/db/dbq/albums.sql.go +++ b/internal/db/dbq/albums.sql.go @@ -585,6 +585,69 @@ func (q *Queries) ListAlbumsByGenre(ctx context.Context, arg ListAlbumsByGenrePa return items, nil } +const listAlbumsFrequentlyPlayedByUser = `-- name: ListAlbumsFrequentlyPlayedByUser :many +WITH plays AS ( + SELECT track_id, count(*) AS cnt + FROM play_events + WHERE user_id = $3 AND was_skipped = false + GROUP BY track_id +), per_album AS ( + SELECT t.album_id, sum(p.cnt) AS cnt + FROM plays p + JOIN tracks t ON t.id = p.track_id + WHERE t.missing_since IS NULL + GROUP BY t.album_id +) +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, albums.release_group_mbid +FROM per_album pa +JOIN albums ON albums.id = pa.album_id +ORDER BY pa.cnt DESC, albums.id +LIMIT $2 OFFSET $1 +` + +type ListAlbumsFrequentlyPlayedByUserParams struct { + Off int32 + Lim int32 + UserID pgtype.UUID +} + +// The user's albums, most played first: an album's count is the user's +// completed plays across its tracks. Skips do not count, so skipping +// through an album does not make it a favourite. Plays of tracks whose file +// is gone do not count. Serves Subsonic getAlbumList2?type=frequent. +func (q *Queries) ListAlbumsFrequentlyPlayedByUser(ctx context.Context, arg ListAlbumsFrequentlyPlayedByUserParams) ([]Album, error) { + rows, err := q.db.Query(ctx, listAlbumsFrequentlyPlayedByUser, arg.Off, arg.Lim, arg.UserID) + if err != nil { + return nil, err + } + defer rows.Close() + var items []Album + for rows.Next() { + var i Album + if err := rows.Scan( + &i.ID, + &i.Title, + &i.SortTitle, + &i.ArtistID, + &i.ReleaseDate, + &i.Mbid, + &i.CoverArtPath, + &i.CreatedAt, + &i.UpdatedAt, + &i.CoverArtSource, + &i.CoverArtSourcesVersion, + &i.ReleaseGroupMbid, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const listAlbumsMissingMbidWithTrack = `-- name: ListAlbumsMissingMbidWithTrack :many SELECT a.id AS album_id, a.artist_id AS artist_id, @@ -720,6 +783,73 @@ func (q *Queries) ListAlbumsRandom(ctx context.Context, limit int32) ([]Album, e return items, nil } +const listAlbumsRecentlyPlayedByUser = `-- name: ListAlbumsRecentlyPlayedByUser :many +WITH plays AS ( + SELECT track_id, max(started_at) AS last_played + FROM play_events + WHERE user_id = $3 + GROUP BY track_id +), per_album AS ( + SELECT t.album_id, max(p.last_played) AS last_played + FROM plays p + JOIN tracks t ON t.id = p.track_id + WHERE t.missing_since IS NULL + GROUP BY t.album_id +) +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, albums.release_group_mbid +FROM per_album pa +JOIN albums ON albums.id = pa.album_id +ORDER BY pa.last_played DESC, albums.id +LIMIT $2 OFFSET $1 +` + +type ListAlbumsRecentlyPlayedByUserParams struct { + Off int32 + Lim int32 + UserID pgtype.UUID +} + +// The user's albums, most recently played first: an album's time is its +// latest play of any of its tracks, skips included, since a skip is still +// the user reaching for that album. Plays of tracks whose file is gone do +// not count. Serves Subsonic getAlbumList2?type=recent. +// +// Aggregates play_events per track first (play_events_user_track_idx), then +// folds tracks into albums, so the join runs over distinct tracks rather +// than every play. +func (q *Queries) ListAlbumsRecentlyPlayedByUser(ctx context.Context, arg ListAlbumsRecentlyPlayedByUserParams) ([]Album, error) { + rows, err := q.db.Query(ctx, listAlbumsRecentlyPlayedByUser, arg.Off, arg.Lim, arg.UserID) + if err != nil { + return nil, err + } + defer rows.Close() + var items []Album + for rows.Next() { + var i Album + if err := rows.Scan( + &i.ID, + &i.Title, + &i.SortTitle, + &i.ArtistID, + &i.ReleaseDate, + &i.Mbid, + &i.CoverArtPath, + &i.CreatedAt, + &i.UpdatedAt, + &i.CoverArtSource, + &i.CoverArtSourcesVersion, + &i.ReleaseGroupMbid, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const listRecentlyAddedAlbumsWithArtist = `-- name: ListRecentlyAddedAlbumsWithArtist :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, albums.release_group_mbid, artists.name AS artist_name FROM albums diff --git a/internal/db/queries/albums.sql b/internal/db/queries/albums.sql index e6a4e6f0..b74cb1fb 100644 --- a/internal/db/queries/albums.sql +++ b/internal/db/queries/albums.sql @@ -99,6 +99,56 @@ WHERE EXISTS ( ORDER BY albums.sort_title, albums.id LIMIT sqlc.arg(lim) OFFSET sqlc.arg(off); +-- name: ListAlbumsRecentlyPlayedByUser :many +-- The user's albums, most recently played first: an album's time is its +-- latest play of any of its tracks, skips included, since a skip is still +-- the user reaching for that album. Plays of tracks whose file is gone do +-- not count. Serves Subsonic getAlbumList2?type=recent. +-- +-- Aggregates play_events per track first (play_events_user_track_idx), then +-- folds tracks into albums, so the join runs over distinct tracks rather +-- than every play. +WITH plays AS ( + SELECT track_id, max(started_at) AS last_played + FROM play_events + WHERE user_id = sqlc.arg(user_id) + GROUP BY track_id +), per_album AS ( + SELECT t.album_id, max(p.last_played) AS last_played + FROM plays p + JOIN tracks t ON t.id = p.track_id + WHERE t.missing_since IS NULL + GROUP BY t.album_id +) +SELECT albums.* +FROM per_album pa +JOIN albums ON albums.id = pa.album_id +ORDER BY pa.last_played DESC, albums.id +LIMIT sqlc.arg(lim) OFFSET sqlc.arg(off); + +-- name: ListAlbumsFrequentlyPlayedByUser :many +-- The user's albums, most played first: an album's count is the user's +-- completed plays across its tracks. Skips do not count, so skipping +-- through an album does not make it a favourite. Plays of tracks whose file +-- is gone do not count. Serves Subsonic getAlbumList2?type=frequent. +WITH plays AS ( + SELECT track_id, count(*) AS cnt + FROM play_events + WHERE user_id = sqlc.arg(user_id) AND was_skipped = false + GROUP BY track_id +), per_album AS ( + SELECT t.album_id, sum(p.cnt) AS cnt + FROM plays p + JOIN tracks t ON t.id = p.track_id + WHERE t.missing_since IS NULL + GROUP BY t.album_id +) +SELECT albums.* +FROM per_album pa +JOIN albums ON albums.id = pa.album_id +ORDER BY pa.cnt DESC, albums.id +LIMIT sqlc.arg(lim) OFFSET sqlc.arg(off); + -- name: SearchAlbums :many SELECT * FROM albums WHERE title ILIKE '%' || $1 || '%' diff --git a/internal/subsonic/albumlist_test.go b/internal/subsonic/albumlist_test.go new file mode 100644 index 00000000..4df7f0b7 --- /dev/null +++ b/internal/subsonic/albumlist_test.go @@ -0,0 +1,178 @@ +package subsonic + +import ( + "context" + "encoding/json" + "io" + "log/slog" + "net/http" + "net/http/httptest" + "os" + "slices" + "testing" + "time" + + "github.com/jackc/pgx/v5/pgtype" + "github.com/jackc/pgx/v5/pgxpool" + + "git.fabledsword.com/bvandeusen/minstrel/internal/db" + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" + "git.fabledsword.com/bvandeusen/minstrel/internal/dbtest" +) + +// playHistoryFixture is a library of single-track albums and a listener +// whose plays rank them differently by recency and by count. +type playHistoryFixture struct { + pool *pgxpool.Pool + user dbq.User + albums map[string]dbq.Album // keyed by title +} + +// seedPlayHistory builds the fixture: +// +// A: one completed play 1h ago +// B: three completed plays 5h ago +// D: five skipped plays 30m ago (recent counts it, frequent does not) +// C: played 10m ago, but its file is gone (in neither list) +// E: played many times by another user (in neither list) +func seedPlayHistory(t *testing.T) playHistoryFixture { + t.Helper() + if testing.Short() { + t.Skip("skipping in -short mode") + } + dsn := os.Getenv("MINSTREL_TEST_DATABASE_URL") + if dsn == "" { + t.Skip("MINSTREL_TEST_DATABASE_URL not set") + } + if err := db.Migrate(dsn, slog.New(slog.NewTextHandler(io.Discard, nil))); err != nil { + t.Fatalf("migrate: %v", err) + } + pool, err := pgxpool.New(context.Background(), dsn) + if err != nil { + t.Fatalf("pool: %v", err) + } + t.Cleanup(pool.Close) + dbtest.ResetDB(t, pool) + + ctx := context.Background() + q := dbq.New(pool) + user, err := q.CreateUser(ctx, dbq.CreateUserParams{ + Username: dbtest.TestUserPrefix + "listener", PasswordHash: "x", ApiTokenHash: "x", + }) + if err != nil { + t.Fatalf("create user: %v", err) + } + other, err := q.CreateUser(ctx, dbq.CreateUserParams{ + Username: dbtest.TestUserPrefix + "other", PasswordHash: "x", ApiTokenHash: "x", + }) + if err != nil { + t.Fatalf("create other user: %v", err) + } + artist, err := q.UpsertArtist(ctx, dbq.UpsertArtistParams{Name: "Artist", SortName: "Artist"}) + if err != nil { + t.Fatalf("artist: %v", err) + } + + f := playHistoryFixture{pool: pool, user: user, albums: map[string]dbq.Album{}} + tracks := map[string]dbq.Track{} + for _, title := range []string{"A", "B", "C", "D", "E"} { + al, err := q.UpsertAlbum(ctx, dbq.UpsertAlbumParams{Title: title, SortTitle: title, ArtistID: artist.ID}) + if err != nil { + t.Fatalf("album %s: %v", title, err) + } + tr, err := q.UpsertTrack(ctx, dbq.UpsertTrackParams{ + Title: title, AlbumID: al.ID, ArtistID: artist.ID, + FilePath: "/tmp/albumlist-" + title + ".flac", DurationMs: 100_000, + }) + if err != nil { + t.Fatalf("track %s: %v", title, err) + } + f.albums[title] = al + tracks[title] = tr + } + if _, err := pool.Exec(ctx, `UPDATE tracks SET missing_since = now() WHERE id = $1`, tracks["C"].ID); err != nil { + t.Fatalf("mark missing: %v", err) + } + + now := time.Now() + play := func(u dbq.User, title string, ago time.Duration, skipped bool, times int) { + t.Helper() + at := now.Add(-ago) + var sessionID pgtype.UUID + if err := pool.QueryRow(ctx, + `INSERT INTO play_sessions (user_id, started_at, last_event_at, client_id) + VALUES ($1, $2, $2, 'albumlist-test') RETURNING id`, + u.ID, at).Scan(&sessionID); err != nil { + t.Fatalf("insert play_session: %v", err) + } + for i := 0; i < times; i++ { + if _, err := pool.Exec(ctx, + `INSERT INTO play_events (user_id, track_id, session_id, started_at, was_skipped) + VALUES ($1, $2, $3, $4, $5)`, + u.ID, tracks[title].ID, sessionID, at, skipped); err != nil { + t.Fatalf("insert play_event: %v", err) + } + } + } + play(user, "A", time.Hour, false, 1) + play(user, "B", 5*time.Hour, false, 3) + play(user, "D", 30*time.Minute, true, 5) + play(user, "C", 10*time.Minute, false, 4) + play(other, "E", time.Minute, false, 9) + return f +} + +// albumListTitles calls getAlbumList2 as the fixture's user and returns the +// album titles in the order served. +func (f playHistoryFixture) albumListTitles(t *testing.T, query string) []string { + t.Helper() + b := &browseHandlers{pool: f.pool} + req := httptest.NewRequest(http.MethodGet, "/rest/getAlbumList2?f=json&"+query, nil) + ctx := context.WithValue(req.Context(), userCtxKey, f.user) + w := httptest.NewRecorder() + b.getAlbumList2(w, req.WithContext(ctx)) + if w.Code != http.StatusOK { + t.Fatalf("status = %d body=%s", w.Code, w.Body.String()) + } + var raw struct { + Response struct { + Status string `json:"status"` + AlbumList2 AlbumList2Container `json:"albumList2"` + } `json:"subsonic-response"` + } + if err := json.Unmarshal(w.Body.Bytes(), &raw); err != nil { + t.Fatalf("decode: %v body=%s", err, w.Body.String()) + } + if raw.Response.Status != "ok" { + t.Fatalf("status %q body=%s", raw.Response.Status, w.Body.String()) + } + titles := make([]string, len(raw.Response.AlbumList2.Albums)) + for i, a := range raw.Response.AlbumList2.Albums { + titles[i] = a.Name + } + return titles +} + +func TestGetAlbumList2_RecentOrdersByLatestPlay(t *testing.T) { + f := seedPlayHistory(t) + got := f.albumListTitles(t, "type=recent") + if want := []string{"D", "A", "B"}; !slices.Equal(got, want) { + t.Errorf("recent = %v, want %v", got, want) + } +} + +func TestGetAlbumList2_FrequentOrdersByCompletedPlays(t *testing.T) { + f := seedPlayHistory(t) + got := f.albumListTitles(t, "type=frequent") + if want := []string{"B", "A"}; !slices.Equal(got, want) { + t.Errorf("frequent = %v, want %v", got, want) + } +} + +func TestGetAlbumList2_RecentPages(t *testing.T) { + f := seedPlayHistory(t) + got := f.albumListTitles(t, "type=recent&size=1&offset=1") + if want := []string{"A"}; !slices.Equal(got, want) { + t.Errorf("recent page = %v, want %v", got, want) + } +} diff --git a/internal/subsonic/browse.go b/internal/subsonic/browse.go index de51ae01..99b18a14 100644 --- a/internal/subsonic/browse.go +++ b/internal/subsonic/browse.go @@ -236,8 +236,9 @@ func (b *browseHandlers) getSong(w http.ResponseWriter, r *http.Request) { } // getAlbumList2 returns a paged album list filtered by type. Supported types -// match the Subsonic spec § getAlbumList2. "recent" and "frequent" return an -// empty list rather than an error, so clients that poll them don't break. +// match the Subsonic spec § getAlbumList2. "recent" and "frequent" read the +// caller's own play history: most recently played first, and most played +// first. func (b *browseHandlers) getAlbumList2(w http.ResponseWriter, r *http.Request) { params := r.URL.Query() listType := params.Get("type") @@ -291,8 +292,20 @@ func (b *browseHandlers) getAlbumList2(w http.ResponseWriter, r *http.Request) { Genre: genre, Lim: int32(size), Off: int32(offset), }) case "recent", "frequent": - // Not served from play history: empty keeps clients working. - albums = nil + user, ok := UserFromContext(r.Context()) + if !ok { + WriteFail(w, r, ErrNotAuthorized, "Not authenticated") + return + } + if listType == "recent" { + albums, err = q.ListAlbumsRecentlyPlayedByUser(r.Context(), dbq.ListAlbumsRecentlyPlayedByUserParams{ + UserID: user.ID, Lim: int32(size), Off: int32(offset), + }) + } else { + albums, err = q.ListAlbumsFrequentlyPlayedByUser(r.Context(), dbq.ListAlbumsFrequentlyPlayedByUserParams{ + UserID: user.ID, Lim: int32(size), Off: int32(offset), + }) + } default: WriteFail(w, r, ErrGeneric, "Invalid list type: "+listType) return