feat(playlists): per-user starred/rating annotations (backend) (#5749)

* feat(playlists): add average_rating column to playlist table

* feat(playlists): store and read per-user starred/rating annotations

* feat(playlists): clean up annotations when a playlist is deleted

* feat(subsonic): route star/unstar of a playlist to the playlist repository

* feat(subsonic): route setRating of a playlist to the playlist repository

* test(subsonic): guard that playlist responses never expose annotations

* fix(playlists): clean stale mis-typed annotations on upgrade; cover GetAll read-back

* fix(playlists): scope annotation join by item_type and harden delete

Address code-review findings on the playlist-annotations branch:

- withAnnotation: add an item_type predicate to the LEFT JOIN so a
  mis-typed annotation row sharing an id can no longer leak into (or
  duplicate) another entity's read. Correct for every caller since each
  repo writes annotations with item_type = tableName. Regression test added.
- migration: reclassify legacy media_file-typed rows for playlist ids to
  item_type='playlist' (instead of deleting them), preserving users' prior
  playlist star/rating; run before the average_rating backfill so those
  ratings are included.
- playlist Delete: replace the per-request full-table cleanAnnotations()
  anti-join with a targeted, permission-safe (rows-affected gated),
  best-effort delete so a cleanup failure no longer misreports an
  already-committed delete as an error.
- MockPlaylistRepo: implement GetAll/IncPlayCount/ReassignAnnotation to
  remove the dead All field and the nil-interface panic traps.
- test: use slices.IndexFunc instead of a hand-rolled find loop.

* feat(playlists): streamline playlist deletion by relying on annotation sweep

* docs(playlists): trim comments in annotation migration and test

Condense the verbose comments added in this branch per the project's
comment-minimalism guideline, keeping only the non-obvious rationale.

The migration's reclassify block is shortened while preserving the safety
invariant (playlist and media_file ids never collide, so the item_type
rewrite touches only mis-typed rows and cannot violate the unique key) and
the ordering note. The redundant 'Populate average_rating' comment is
dropped since the UPDATE is self-evident. The repository test's leakage
comment is condensed to two lines. No code behavior changes.

* refactor(subsonic): resolve setStar targets via GetEntityByID

Replace setStar's Album/Artist/Playlist Exists probe chain with a single
model.GetEntityByID lookup and a type switch, mirroring setRating. This
removes three per-id existence queries and keeps the two annotation paths
consistent.

An id that resolves to no known entity is logged and skipped rather than
filed as a spurious media_file annotation, and a lookup failure on one id no
longer aborts the whole batch. Also drop a duplicate empty-ids guard.

* refactor(playlists): drop no-op reclassify/backfill from migration

The average_rating migration carried two data-fix UPDATEs that are no-ops on
any real database:

- The media_file->playlist reclassification only matches rows no released
  build ever created: playlists were never annotatable, so star/setRating of
  a playlist id was never written as item_type='playlist'. Any stray
  media_file-typed row for a playlist id is already removed by the media_file
  annotation GC sweep (item_id not in media_file).
- The average_rating backfill runs before any item_type='playlist' row can
  exist, so it can only ever write the default 0. Going forward SetRating
  keeps average_rating current via updateAvgRating.

Reduce the migration to the column add/drop.

* refactor(persistence): bind annotation join params, derive idField from tableName

Address PR review: use Squirrel parameter binding for item_type/user_id in
the shared withAnnotation join instead of string concatenation, and pass
r.tableName+".id" from selectPlaylist so the join field stays consistent
with the surrounding r.tableName usage.

* fix(subsonic): surface datastore errors in setStar instead of skipping

Address PR review: setStar swallowed every GetEntityByID error and continued,
so a real datastore failure would still commit the transaction and emit a
refresh event as if the star succeeded. Skip only on model.ErrNotFound (an
unknown id); return any other error so the request fails and rolls back.

* test(subsonic): assert absent JSON keys instead of substring matches

Address PR review: substring checks are brittle ("starred" matches "starredAt",
"rating" matches "userRating"). Unmarshal the response and assert the
annotation keys are absent.

* fix(subsonic): skip refresh broadcast when a star request changes nothing

Address PR review (Codex): once setStar began skipping unknown ids, a request
containing only unresolvable ids left the RefreshResource empty, which
SendMessage serializes as a {*:*} wildcard that forces every client to
refresh. Only broadcast when at least one id was actually starred.

* fix(db): rebase playlist average_rating migration timestamp past master

The 20260708011823 migration predated the newest migration merged to
master (20260712211040_add_primary_key...), which Goose would silently
skip on already-upgraded databases. Rename it to a current timestamp so
it applies in order.

db/migrations/20260714120000_add_playlist_average_rating.sql

@@ -0,0 +1,5 @@
+-- +goose Up
+ALTER TABLE playlist ADD COLUMN average_rating REAL NOT NULL DEFAULT 0;
+
+-- +goose Down
+ALTER TABLE playlist DROP COLUMN average_rating;

model/playlist.go

@@ -10,6 +10,8 @@ import (
 )
 
 type Playlist struct {
+	Annotations `structs:"-"`
+
 	ID               string         `structs:"id" json:"id"`
 	Name             string         `structs:"name" json:"name"`
 	Comment          string         `structs:"comment" json:"comment"`
@@ -121,6 +123,7 @@ type Playlists []Playlist
 
 type PlaylistRepository interface {
 	ResourceRepository
+	AnnotatedRepository
 	CountAll(options ...QueryOptions) (int64, error)
 	Exists(id string) (bool, error)
 	Put(pls *Playlist, cols ...string) error

persistence/persistence.go

@@ -193,6 +193,7 @@ func (s *SQLStore) GC(ctx context.Context, libraryIDs ...int) error {
 		trace(ctx, "clean album annotations", func() error { return s.Album(ctx).(*albumRepository).cleanAnnotations() }),
 		trace(ctx, "clean artist annotations", func() error { return s.Artist(ctx).(*artistRepository).cleanAnnotations() }),
 		trace(ctx, "clean media file annotations", func() error { return s.MediaFile(ctx).(*mediaFileRepository).cleanAnnotations() }),
+		trace(ctx, "clean playlist annotations", func() error { return s.Playlist(ctx).(*playlistRepository).cleanAnnotations() }),
 		trace(ctx, "clean media file bookmarks", func() error { return s.MediaFile(ctx).(*mediaFileRepository).cleanBookmarks() }),
 		trace(ctx, "purge non used tags", func() error { return s.Tag(ctx).(*tagRepository).purgeUnused() }),
 		trace(ctx, "remove orphan playlist tracks", func() error { return s.Playlist(ctx).(*playlistRepository).removeOrphans() }),

persistence/playlist_repository.go

@@ -203,8 +203,9 @@ func (r *playlistRepository) GetPlaylists(mediaFileId string) (model.Playlists,
 }
 
 func (r *playlistRepository) selectPlaylist(options ...model.QueryOptions) SelectBuilder {
-	return r.newSelect(options...).Join("user on user.id = owner_id").
+	sel := r.newSelect(options...).Join("user on user.id = owner_id").
 		Columns(r.tableName+".*", "user.user_name as owner_name")
+	return r.withAnnotation(sel, r.tableName+".id")
 }
 
 func (r *playlistRepository) updateTracks(id string, tracks model.MediaFiles) error {

persistence/playlist_repository_test.go

@@ -1,11 +1,14 @@
 package persistence
 
 import (
+	"slices"
+
 	"github.com/navidrome/navidrome/log"
 	"github.com/navidrome/navidrome/model"
 	"github.com/navidrome/navidrome/model/request"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+	"github.com/pocketbase/dbx"
 )
 
 var _ = Describe("PlaylistRepository", func() {
@@ -71,6 +74,98 @@ var _ = Describe("PlaylistRepository", func() {
 		})
 	})
 
+	Describe("Annotations", func() {
+		var plsID string
+
+		BeforeEach(func() {
+			pls := model.Playlist{Name: "Annotated", OwnerID: "userid"}
+			Expect(repo.Put(&pls)).To(Succeed())
+			plsID = pls.ID
+		})
+
+		countAnnotations := func() int {
+			var count int
+			Expect(GetDBXBuilder().NewQuery(
+				"SELECT count(*) FROM annotation WHERE item_type = 'playlist' AND item_id = {:id}").
+				Bind(dbx.Params{"id": plsID}).Row(&count)).To(Succeed())
+			return count
+		}
+
+		It("stores and reads back starred", func() {
+			Expect(repo.SetStar(true, plsID)).To(Succeed())
+
+			p, err := repo.Get(plsID)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(p.Starred).To(BeTrue())
+			Expect(p.StarredAt).ToNot(BeNil())
+		})
+
+		It("stores and reads back rating and average_rating", func() {
+			Expect(repo.SetRating(4, plsID)).To(Succeed())
+
+			p, err := repo.Get(plsID)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(p.Rating).To(Equal(4))
+			Expect(p.RatedAt).ToNot(BeNil())
+			Expect(p.AverageRating).To(Equal(4.0))
+		})
+
+		It("keeps annotations isolated per user", func() {
+			Expect(repo.SetStar(true, plsID)).To(Succeed())
+
+			otherCtx := request.WithUser(log.NewContext(GinkgoT().Context()),
+				model.User{ID: "otheruser", UserName: "otheruser", IsAdmin: true})
+			otherRepo := NewPlaylistRepository(otherCtx, GetDBXBuilder())
+
+			p, err := otherRepo.Get(plsID)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(p.Starred).To(BeFalse())
+		})
+
+		It("reads starred back through GetAll", func() {
+			Expect(repo.SetStar(true, plsID)).To(Succeed())
+
+			all, err := repo.GetAll()
+			Expect(err).ToNot(HaveOccurred())
+			idx := slices.IndexFunc(all, func(p model.Playlist) bool { return p.ID == plsID })
+			Expect(idx).To(BeNumerically(">=", 0))
+			Expect(all[idx].Starred).To(BeTrue())
+		})
+
+		It("does not leak an annotation row of another item_type sharing the playlist id", func() {
+			// Older builds (and the star fallthrough) can leave a media_file-typed row
+			// under a playlist id; the item_type-scoped join must not surface or dupe it.
+			_, err := GetDBXBuilder().NewQuery(
+				"INSERT INTO annotation (user_id, item_id, item_type, starred) VALUES ({:uid}, {:id}, 'media_file', 1)").
+				Bind(dbx.Params{"uid": "userid", "id": plsID}).Execute()
+			Expect(err).ToNot(HaveOccurred())
+
+			p, err := repo.Get(plsID)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(p.Starred).To(BeFalse())
+
+			all, err := repo.GetAll()
+			Expect(err).ToNot(HaveOccurred())
+			matches := 0
+			for _, pl := range all {
+				if pl.ID == plsID {
+					matches++
+				}
+			}
+			Expect(matches).To(Equal(1))
+		})
+
+		It("relies on the annotation sweep, not Delete, to clean up annotations", func() {
+			Expect(repo.SetStar(true, plsID)).To(Succeed())
+
+			Expect(repo.Delete(plsID)).To(Succeed())
+			Expect(countAnnotations()).To(Equal(1))
+
+			Expect(repo.(*playlistRepository).cleanAnnotations()).To(Succeed())
+			Expect(countAnnotations()).To(Equal(0))
+		})
+	})
+
 	It("Put/Exists/Delete", func() {
 		By("saves the playlist to the DB")
 		newPls := model.Playlist{Name: "Great!", OwnerID: "userid"}

persistence/sql_annotations.go

@@ -67,7 +67,8 @@ func (r sqlRepository) withAnnotation(query SelectBuilder, idField string) Selec
 	query = query.
 		LeftJoin("annotation on ("+
 			"annotation.item_id = "+idField+
-			" AND annotation.user_id = '"+userID+"')").
+			" AND annotation.item_type = ?"+
+			" AND annotation.user_id = ?)", r.tableName, userID).
 		Columns(
 			"coalesce(starred, 0) as starred",
 			"coalesce(rating, 0) as rating",

server/subsonic/media_annotation.go

@@ -2,6 +2,7 @@ package subsonic
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"math"
 	"net/http"
@@ -52,6 +53,9 @@ func (api *Router) setRating(ctx context.Context, id string, rating int) error {
 	case *model.Album:
 		repo = api.ds.Album(ctx)
 		resource = "album"
+	case *model.Playlist:
+		repo = api.ds.Playlist(ctx)
+		resource = "playlist"
 	default:
 		repo = api.ds.MediaFile(ctx)
 		resource = "song"
@@ -104,48 +108,50 @@ func (api *Router) Unstar(r *http.Request) (*responses.Subsonic, error) {
 }
 
 func (api *Router) setStar(ctx context.Context, star bool, ids ...string) error {
-	if len(ids) == 0 {
-		return nil
-	}
-	log.Debug(ctx, "Changing starred", "ids", ids, "starred", star)
 	if len(ids) == 0 {
 		log.Warn(ctx, "Cannot star/unstar an empty list of ids")
 		return nil
 	}
-	event := &events.RefreshResource{}
+	log.Debug(ctx, "Changing starred", "ids", ids, "starred", star)
 	err := api.ds.WithTxImmediate(func(tx model.DataStore) error {
+		event := &events.RefreshResource{}
+		changed := false
 		for _, id := range ids {
-			exist, err := tx.Album(ctx).Exists(id)
+			var repo model.AnnotatedRepository
+			var resource string
+			entity, err := model.GetEntityByID(ctx, tx, id)
 			if err != nil {
-				return err
-			}
-			if exist {
-				err = tx.Album(ctx).SetStar(star, id)
-				if err != nil {
+				if !errors.Is(err, model.ErrNotFound) {
 					return err
 				}
-				event = event.With("album", id)
+				log.Warn(ctx, "Cannot star/unstar unknown id, skipping", "id", id)
 				continue
 			}
-			exist, err = tx.Artist(ctx).Exists(id)
-			if err != nil {
+			switch entity.(type) {
+			case *model.Artist:
+				repo = tx.Artist(ctx)
+				resource = "artist"
+			case *model.Album:
+				repo = tx.Album(ctx)
+				resource = "album"
+			case *model.Playlist:
+				repo = tx.Playlist(ctx)
+				resource = "playlist"
+			default:
+				repo = tx.MediaFile(ctx)
+				resource = "song"
+			}
+			if err := repo.SetStar(star, id); err != nil {
 				return err
 			}
-			if exist {
-				err = tx.Artist(ctx).SetStar(star, id)
-				if err != nil {
-					return err
-				}
-				event = event.With("artist", id)
-				continue
-			}
-			err = tx.MediaFile(ctx).SetStar(star, id)
-			if err != nil {
-				return err
-			}
-			event = event.With("song", id)
+			event = event.With(resource, id)
+			changed = true
+		}
+		// Skip the broadcast when nothing changed: an empty RefreshResource
+		// serializes as a "{*:*}" wildcard, forcing every client to refresh.
+		if changed {
+			api.broker.SendMessage(ctx, event)
 		}
-		api.broker.SendMessage(ctx, event)
 		return nil
 	})
 	if err != nil {

server/subsonic/media_annotation_test.go

@@ -185,6 +185,64 @@ var _ = Describe("MediaAnnotationController", func() {
 			Expect(playTracker.ReportedPlayback[0].ClientName).To(BeEmpty())
 		})
 	})
+
+	Describe("Star/Unstar playlists", func() {
+		var plRepo *tests.MockPlaylistRepo
+
+		BeforeEach(func() {
+			plRepo = tests.CreateMockPlaylistRepo()
+			plRepo.SetData(model.Playlists{{ID: "pl-1", Name: "My Playlist", OwnerID: "u1"}})
+			ds.(*tests.MockDataStore).MockedPlaylist = plRepo
+		})
+
+		It("stars a playlist by dispatching to the Playlist repo", func() {
+			r := newGetRequest("id=pl-1")
+
+			_, err := router.Star(r)
+
+			Expect(err).ToNot(HaveOccurred())
+			Expect(plRepo.Starred).To(HaveKeyWithValue("pl-1", true))
+		})
+
+		It("unstars a playlist by dispatching to the Playlist repo", func() {
+			r := newGetRequest("id=pl-1")
+
+			_, err := router.Unstar(r)
+
+			Expect(err).ToNot(HaveOccurred())
+			Expect(plRepo.Starred).To(HaveKeyWithValue("pl-1", false))
+		})
+	})
+
+	Describe("SetRating playlists", func() {
+		var plRepo *tests.MockPlaylistRepo
+
+		BeforeEach(func() {
+			plRepo = tests.CreateMockPlaylistRepo()
+			plRepo.SetData(model.Playlists{{ID: "pl-1", Name: "My Playlist", OwnerID: "u1"}})
+			ds.(*tests.MockDataStore).MockedPlaylist = plRepo
+		})
+
+		It("rates a playlist by dispatching to the Playlist repo", func() {
+			r := newGetRequest("id=pl-1", "rating=4")
+
+			_, err := router.SetRating(r)
+
+			Expect(err).ToNot(HaveOccurred())
+			Expect(plRepo.Ratings).To(HaveKeyWithValue("pl-1", 4))
+		})
+	})
+
+	Describe("Star with an unresolvable id", func() {
+		It("skips the id without broadcasting an empty (wildcard) refresh", func() {
+			r := newGetRequest("id=does-not-exist")
+
+			_, err := router.Star(r)
+
+			Expect(err).ToNot(HaveOccurred())
+			Expect(eventBroker.Events).To(BeEmpty())
+		})
+	})
 })
 
 type fakePlayTracker struct {

server/subsonic/playlists_test.go

@@ -2,6 +2,7 @@ package subsonic
 
 import (
 	"context"
+	"encoding/json"
 	"time"
 
 	"github.com/navidrome/navidrome/conf"
@@ -248,6 +249,27 @@ var _ = Describe("buildPlaylist", func() {
 			})
 		})
 	})
+
+	Describe("annotation leakage", func() {
+		It("does not serialize starred/rating even when the model carries them", func() {
+			p := model.Playlist{ID: "pl-1", Name: "My Playlist"}
+			p.Starred = true
+			p.Rating = 5
+
+			resp := router.buildPlaylist(ctx, p)
+
+			data, err := json.Marshal(resp)
+			Expect(err).ToNot(HaveOccurred())
+			var fields map[string]any
+			Expect(json.Unmarshal(data, &fields)).To(Succeed())
+			Expect(fields).ToNot(HaveKey("starred"))
+			Expect(fields).ToNot(HaveKey("starredAt"))
+			Expect(fields).ToNot(HaveKey("rating"))
+			Expect(fields).ToNot(HaveKey("userRating"))
+			Expect(fields).ToNot(HaveKey("averageRating"))
+			Expect(fields).ToNot(HaveKey("playCount"))
+		})
+	})
 })
 
 var _ = Describe("UpdatePlaylist", func() {

tests/mock_playlist_repo.go

@@ -2,6 +2,7 @@ package tests
 
 import (
 	"errors"
+	"time"
 
 	"github.com/deluan/rest"
 	"github.com/navidrome/navidrome/model"
@@ -19,8 +20,11 @@ type MockPlaylistRepo struct {
 	model.PlaylistRepository
 	Data       map[string]*model.Playlist // keyed by ID
 	PathMap    map[string]*model.Playlist // keyed by path
+	All        model.Playlists
 	Last       *model.Playlist
 	Deleted    []string
+	Starred    map[string]bool // itemID -> starred
+	Ratings    map[string]int  // itemID -> rating
 	Err        bool
 	TracksRepo model.PlaylistTrackRepository
 }
@@ -29,6 +33,14 @@ func (m *MockPlaylistRepo) SetError(err bool) {
 	m.Err = err
 }
 
+func (m *MockPlaylistRepo) SetData(pls model.Playlists) {
+	m.Data = make(map[string]*model.Playlist, len(pls))
+	m.All = pls
+	for i, p := range m.All {
+		m.Data[p.ID] = &m.All[i]
+	}
+}
+
 func (m *MockPlaylistRepo) Get(id string) (*model.Playlist, error) {
 	if m.Err {
 		return nil, errors.New("error")
@@ -45,6 +57,13 @@ func (m *MockPlaylistRepo) GetWithTracks(id string, _, _ bool) (*model.Playlist,
 	return m.Get(id)
 }
 
+func (m *MockPlaylistRepo) GetAll(_ ...model.QueryOptions) (model.Playlists, error) {
+	if m.Err {
+		return nil, errors.New("error")
+	}
+	return m.All, nil
+}
+
 func (m *MockPlaylistRepo) Put(pls *model.Playlist, _ ...string) error {
 	if m.Err {
 		return errors.New("error")
@@ -79,6 +98,44 @@ func (m *MockPlaylistRepo) Delete(id string) error {
 	return nil
 }
 
+func (m *MockPlaylistRepo) SetStar(starred bool, ids ...string) error {
+	if m.Err {
+		return errors.New("error")
+	}
+	if m.Starred == nil {
+		m.Starred = map[string]bool{}
+	}
+	for _, id := range ids {
+		m.Starred[id] = starred
+	}
+	return nil
+}
+
+func (m *MockPlaylistRepo) SetRating(rating int, id string) error {
+	if m.Err {
+		return errors.New("error")
+	}
+	if m.Ratings == nil {
+		m.Ratings = map[string]int{}
+	}
+	m.Ratings[id] = rating
+	return nil
+}
+
+func (m *MockPlaylistRepo) IncPlayCount(string, time.Time) error {
+	if m.Err {
+		return errors.New("error")
+	}
+	return nil
+}
+
+func (m *MockPlaylistRepo) ReassignAnnotation(string, string) error {
+	if m.Err {
+		return errors.New("error")
+	}
+	return nil
+}
+
 func (m *MockPlaylistRepo) Tracks(_ string, _ bool) model.PlaylistTrackRepository {
 	return m.TracksRepo
 }