Compare commits

...
2 Commits
Author SHA1 Message Date
bvandeusenandClaude Opus 5.5 f3b124f9b2 feat(subsonic): getAlbumList2 recent and frequent read the caller's play history (#6293)
release / govulncheck (push) Successful in 20s
release / web (push) Successful in 1m37s
release / go (push) Successful in 1m45s
release / integration (push) Failing after 5m27s
release / android (push) Successful in 5m40s
release / Build signed APK (releases and dev) (push) Successful in 5m55s
release / Attach APK to the Release (tag releases only) (push) Skipped
release / Build + push container image (push) Skipped
release / Verify release artifacts (tag releases only) (push) Skipped
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 <noreply@anthropic.com>
2026-10-23 13:58:09 -04:00
bvandeusenandClaude Opus 5.5 09e78962b2 fix(subsonic): getUser answers an admin's lookup with the requested user (#6292)
An admin asking for another username got their own name and roles back.
The handler now looks the user up by exact username and answers with
that user's payload; an unknown username is ErrDataNotFound. A
non-admin asking about someone else is still ErrNotAuthorized.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-23 13:58:09 -04:00
7 changed files with 457 additions and 18 deletions
+130
View File
@@ -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
+50
View File
@@ -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 || '%'
+178
View File
@@ -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)
}
}
+17 -4
View File
@@ -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
+3 -1
View File
@@ -13,6 +13,7 @@ import (
"github.com/jackc/pgx/v5/pgxpool"
"git.fabledsword.com/bvandeusen/minstrel/internal/auth"
"git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq"
"git.fabledsword.com/bvandeusen/minstrel/internal/playevents"
)
@@ -26,6 +27,7 @@ import (
func Mount(r chi.Router, pool *pgxpool.Pool, logger *slog.Logger, cfg Config, events *playevents.Writer, hops func() int) {
b := &browseHandlers{pool: pool}
m := newMediaHandlers(pool, events, logger)
u := &userHandlers{lookup: dbq.New(pool).GetUserByUsername}
r.Route("/rest", func(sub chi.Router) {
sub.Use(Middleware(pool, cfg, auth.NewLoginGuard(), hops))
register(sub, "/ping", handlePing)
@@ -46,7 +48,7 @@ func Mount(r chi.Router, pool *pgxpool.Pool, logger *slog.Logger, cfg Config, ev
register(sub, "/unstar", m.handleUnstar)
register(sub, "/getStarred", m.handleGetStarred)
register(sub, "/getStarred2", m.handleGetStarred2)
register(sub, "/getUser", handleGetUser)
register(sub, "/getUser", u.getUser)
// Subsonic clients expect every /rest/* miss to come back as a
// Subsonic-shaped error envelope, not chi's plain-text 404. Without
+30 -11
View File
@@ -1,9 +1,13 @@
package subsonic
import (
"context"
"encoding/xml"
"errors"
"net/http"
"github.com/jackc/pgx/v5"
"git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq"
)
@@ -38,26 +42,41 @@ type User struct {
VideoConversionRole bool `json:"videoConversionRole" xml:"videoConversionRole,attr"`
}
// handleGetUser returns the authenticated user when no username is provided
// or when the requested username matches; admins may look up any user. Per
// spec a non-admin asking about another user gets ErrNotAuthorized.
func handleGetUser(w http.ResponseWriter, r *http.Request) {
// userHandlers serves getUser. lookup finds a user by exact username; it is
// a field so tests can answer without a database.
type userHandlers struct {
lookup func(ctx context.Context, username string) (dbq.User, error)
}
// getUser returns the authenticated user when no username is provided or
// when the requested username matches; admins may look up any user, and an
// unknown username is ErrDataNotFound. Per spec a non-admin asking about
// another user gets ErrNotAuthorized.
func (h *userHandlers) getUser(w http.ResponseWriter, r *http.Request) {
caller, ok := UserFromContext(r.Context())
if !ok {
WriteFail(w, r, ErrNotAuthorized, "Not authenticated")
return
}
requested := r.URL.Query().Get("username")
if requested != "" && requested != caller.Username && !caller.IsAdmin {
if requested == "" || requested == caller.Username {
Write(w, r, UserResponse{Envelope: NewEnvelope("ok"), User: userPayload(caller)})
return
}
if !caller.IsAdmin {
WriteFail(w, r, ErrNotAuthorized, "Not authorized to view other users")
return
}
// No per-user lookup: an admin asking for another username is answered
// with the caller's own roles.
Write(w, r, UserResponse{
Envelope: NewEnvelope("ok"),
User: userPayload(caller),
})
target, err := h.lookup(r.Context(), requested)
if errors.Is(err, pgx.ErrNoRows) {
WriteFail(w, r, ErrDataNotFound, "User not found")
return
}
if err != nil {
WriteFail(w, r, ErrGeneric, "Failed to look up user")
return
}
Write(w, r, UserResponse{Envelope: NewEnvelope("ok"), User: userPayload(target)})
}
func userPayload(u dbq.User) User {
+49 -2
View File
@@ -8,6 +8,8 @@ import (
"strings"
"testing"
"github.com/jackc/pgx/v5"
"git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq"
)
@@ -51,6 +53,41 @@ func TestHandleGetUser_NonAdminCannotLookupOthers(t *testing.T) {
}
}
func TestHandleGetUser_AdminLooksUpAnotherUser(t *testing.T) {
admin := dbq.User{Username: "admin", IsAdmin: true}
bob := dbq.User{Username: "bob", IsAdmin: false}
_, body := callGetUser(t, admin, "bob", bob)
if body.Resp.Status != "ok" {
t.Fatalf("status = %q, want ok (err=%+v)", body.Resp.Status, body.Resp.Error)
}
if body.Resp.User.Username != "bob" {
t.Errorf("username = %q, want bob: the answer must describe the user asked about, not the caller", body.Resp.User.Username)
}
if body.Resp.User.AdminRole || body.Resp.User.SettingsRole {
t.Errorf("bob is not an admin, but the answer carries admin/settings roles %v/%v",
body.Resp.User.AdminRole, body.Resp.User.SettingsRole)
}
}
func TestHandleGetUser_AdminLooksUpUnknownUser(t *testing.T) {
admin := dbq.User{Username: "admin", IsAdmin: true}
_, body := callGetUser(t, admin, "nobody")
if body.Resp.Status != "failed" || body.Resp.Error == nil ||
body.Resp.Error.Code != ErrDataNotFound {
t.Errorf("expected ErrDataNotFound, got status=%q err=%+v",
body.Resp.Status, body.Resp.Error)
}
}
func TestHandleGetUser_OwnUsernameNeedsNoLookup(t *testing.T) {
// No known users: a lookup would fail, so success proves none was made.
user := dbq.User{Username: "alice", IsAdmin: false}
_, body := callGetUser(t, user, "alice")
if body.Resp.Status != "ok" || body.Resp.User.Username != "alice" {
t.Errorf("status=%q username=%q, want ok/alice", body.Resp.Status, body.Resp.User.Username)
}
}
type getUserPayload struct {
Resp struct {
Status string `json:"status"`
@@ -59,8 +96,18 @@ type getUserPayload struct {
} `json:"subsonic-response"`
}
func callGetUser(t *testing.T, user dbq.User, requestedUsername string) (*httptest.ResponseRecorder, getUserPayload) {
// callGetUser runs getUser as caller, with the users in known as the only
// accounts a lookup can find.
func callGetUser(t *testing.T, user dbq.User, requestedUsername string, known ...dbq.User) (*httptest.ResponseRecorder, getUserPayload) {
t.Helper()
h := &userHandlers{lookup: func(_ context.Context, username string) (dbq.User, error) {
for _, u := range known {
if u.Username == username {
return u, nil
}
}
return dbq.User{}, pgx.ErrNoRows
}}
url := "/rest/getUser.view?f=json"
if requestedUsername != "" {
url += "&username=" + requestedUsername
@@ -68,7 +115,7 @@ func callGetUser(t *testing.T, user dbq.User, requestedUsername string) (*httpte
req := httptest.NewRequest("GET", url, nil)
req = req.WithContext(context.WithValue(req.Context(), userCtxKey, user))
w := httptest.NewRecorder()
handleGetUser(w, req)
h.getUser(w, req)
var body getUserPayload
if err := json.NewDecoder(strings.NewReader(w.Body.String())).Decode(&body); err != nil {