mirror of
https://github.com/navidrome/navidrome.git
synced 2026-11-12 21:28:37 +01:00
* 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.
348 lines
11 KiB
Go
348 lines
11 KiB
Go
package subsonic
|
|
|
|
import (
|
|
"context"
|
|
"encoding/json"
|
|
"time"
|
|
|
|
"github.com/navidrome/navidrome/conf"
|
|
"github.com/navidrome/navidrome/core/playlists"
|
|
"github.com/navidrome/navidrome/model"
|
|
"github.com/navidrome/navidrome/model/criteria"
|
|
"github.com/navidrome/navidrome/model/request"
|
|
"github.com/navidrome/navidrome/tests"
|
|
. "github.com/onsi/ginkgo/v2"
|
|
. "github.com/onsi/gomega"
|
|
)
|
|
|
|
var _ playlists.Playlists = (*fakePlaylists)(nil)
|
|
|
|
var _ = Describe("buildPlaylist", func() {
|
|
var router *Router
|
|
var ds model.DataStore
|
|
var ctx context.Context
|
|
var playlist model.Playlist
|
|
|
|
BeforeEach(func() {
|
|
ds = &tests.MockDataStore{}
|
|
router = New(ds, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil)
|
|
ctx = context.Background()
|
|
})
|
|
|
|
Describe("normal playlist", func() {
|
|
BeforeEach(func() {
|
|
createdAt := time.Date(2023, 1, 15, 10, 30, 0, 0, time.UTC)
|
|
updatedAt := time.Date(2023, 2, 20, 14, 45, 0, 0, time.UTC)
|
|
|
|
playlist = model.Playlist{
|
|
ID: "pls-1",
|
|
Name: "My Playlist",
|
|
Comment: "Test comment",
|
|
OwnerName: "admin",
|
|
OwnerID: "1234",
|
|
Public: true,
|
|
SongCount: 10,
|
|
Duration: 600,
|
|
CreatedAt: createdAt,
|
|
UpdatedAt: updatedAt,
|
|
}
|
|
})
|
|
|
|
Context("with minimal client", func() {
|
|
BeforeEach(func() {
|
|
conf.Server.Subsonic.MinimalClients = "minimal-client"
|
|
player := model.Player{Client: "minimal-client"}
|
|
ctx = request.WithPlayer(ctx, player)
|
|
})
|
|
|
|
It("returns only basic fields", func() {
|
|
result := router.buildPlaylist(ctx, playlist)
|
|
|
|
Expect(result.Id).To(Equal("pls-1"))
|
|
Expect(result.Name).To(Equal("My Playlist"))
|
|
Expect(result.SongCount).To(Equal(int32(10)))
|
|
Expect(result.Duration).To(Equal(int32(600)))
|
|
Expect(result.Created).To(Equal(playlist.CreatedAt))
|
|
Expect(result.Changed).To(Equal(playlist.UpdatedAt))
|
|
|
|
// These should not be set
|
|
Expect(result.Comment).To(BeEmpty())
|
|
Expect(result.Owner).To(BeEmpty())
|
|
Expect(result.Public).To(BeFalse())
|
|
Expect(result.CoverArt).To(BeEmpty())
|
|
})
|
|
})
|
|
|
|
Context("with non-minimal client", func() {
|
|
BeforeEach(func() {
|
|
conf.Server.Subsonic.MinimalClients = "minimal-client"
|
|
player := model.Player{Client: "regular-client"}
|
|
ctx = request.WithPlayer(ctx, player)
|
|
})
|
|
|
|
It("returns all fields", func() {
|
|
result := router.buildPlaylist(ctx, playlist)
|
|
|
|
Expect(result.Id).To(Equal("pls-1"))
|
|
Expect(result.Name).To(Equal("My Playlist"))
|
|
Expect(result.SongCount).To(Equal(int32(10)))
|
|
Expect(result.Duration).To(Equal(int32(600)))
|
|
Expect(result.Created).To(Equal(playlist.CreatedAt))
|
|
Expect(result.Changed).To(Equal(playlist.UpdatedAt))
|
|
Expect(result.Comment).To(Equal("Test comment"))
|
|
Expect(result.Owner).To(Equal("admin"))
|
|
Expect(result.Public).To(BeTrue())
|
|
Expect(result.Readonly).To(BeTrue())
|
|
})
|
|
|
|
It("returns all fields when as owner", func() {
|
|
ctx = request.WithUser(ctx, model.User{ID: "1234", UserName: "admin"})
|
|
|
|
result := router.buildPlaylist(ctx, playlist)
|
|
|
|
Expect(result.Id).To(Equal("pls-1"))
|
|
Expect(result.Name).To(Equal("My Playlist"))
|
|
Expect(result.SongCount).To(Equal(int32(10)))
|
|
Expect(result.Duration).To(Equal(int32(600)))
|
|
Expect(result.Created).To(Equal(playlist.CreatedAt))
|
|
Expect(result.Changed).To(Equal(playlist.UpdatedAt))
|
|
Expect(result.Comment).To(Equal("Test comment"))
|
|
Expect(result.Owner).To(Equal("admin"))
|
|
Expect(result.Public).To(BeTrue())
|
|
Expect(result.Readonly).To(BeFalse())
|
|
})
|
|
})
|
|
|
|
Context("when minimal clients list is empty", func() {
|
|
BeforeEach(func() {
|
|
conf.Server.Subsonic.MinimalClients = ""
|
|
player := model.Player{Client: "any-client"}
|
|
ctx = request.WithPlayer(ctx, player)
|
|
})
|
|
|
|
It("returns all fields", func() {
|
|
result := router.buildPlaylist(ctx, playlist)
|
|
|
|
Expect(result.Comment).To(Equal("Test comment"))
|
|
Expect(result.Owner).To(Equal("admin"))
|
|
Expect(result.Public).To(BeTrue())
|
|
})
|
|
})
|
|
|
|
Context("with legacy client", func() {
|
|
BeforeEach(func() {
|
|
conf.Server.Subsonic.LegacyClients = "legacy-client"
|
|
player := model.Player{Client: "legacy-client"}
|
|
ctx = request.WithPlayer(ctx, player)
|
|
})
|
|
|
|
It("returns all standard fields but no OpenSubsonic extensions", func() {
|
|
result := router.buildPlaylist(ctx, playlist)
|
|
|
|
Expect(result.Comment).To(Equal("Test comment"))
|
|
Expect(result.Owner).To(Equal("admin"))
|
|
Expect(result.Public).To(BeTrue())
|
|
Expect(result.OpenSubsonicPlaylist).To(BeNil())
|
|
})
|
|
})
|
|
|
|
Context("when no player in context", func() {
|
|
It("returns all fields", func() {
|
|
result := router.buildPlaylist(ctx, playlist)
|
|
|
|
Expect(result.Comment).To(Equal("Test comment"))
|
|
Expect(result.Owner).To(Equal("admin"))
|
|
Expect(result.Public).To(BeTrue())
|
|
})
|
|
})
|
|
})
|
|
|
|
Describe("smart playlist", func() {
|
|
evaluatedAt := time.Date(2023, 2, 20, 15, 45, 0, 0, time.UTC)
|
|
validUntil := evaluatedAt.Add(5 * time.Second)
|
|
|
|
BeforeEach(func() {
|
|
createdAt := time.Date(2023, 1, 15, 10, 30, 0, 0, time.UTC)
|
|
updatedAt := time.Date(2023, 2, 20, 14, 45, 0, 0, time.UTC)
|
|
|
|
playlist = model.Playlist{
|
|
ID: "pls-1",
|
|
Name: "My Playlist",
|
|
Comment: "Test comment",
|
|
OwnerName: "admin",
|
|
OwnerID: "1234",
|
|
Public: true,
|
|
SongCount: 10,
|
|
Duration: 600,
|
|
CreatedAt: createdAt,
|
|
UpdatedAt: updatedAt,
|
|
EvaluatedAt: &evaluatedAt,
|
|
Rules: &criteria.Criteria{
|
|
Expression: criteria.All{criteria.Contains{"title": "title"}},
|
|
},
|
|
}
|
|
})
|
|
|
|
Context("with minimal client", func() {
|
|
BeforeEach(func() {
|
|
conf.Server.Subsonic.MinimalClients = "minimal-client"
|
|
player := model.Player{Client: "minimal-client"}
|
|
ctx = request.WithPlayer(ctx, player)
|
|
})
|
|
|
|
It("returns only basic fields", func() {
|
|
result := router.buildPlaylist(ctx, playlist)
|
|
|
|
Expect(result.Id).To(Equal("pls-1"))
|
|
Expect(result.Name).To(Equal("My Playlist"))
|
|
Expect(result.SongCount).To(Equal(int32(10)))
|
|
Expect(result.Duration).To(Equal(int32(600)))
|
|
Expect(result.Created).To(Equal(playlist.CreatedAt))
|
|
Expect(result.Changed).To(Equal(evaluatedAt))
|
|
|
|
// These should not be set
|
|
Expect(result.Comment).To(BeEmpty())
|
|
Expect(result.Owner).To(BeEmpty())
|
|
Expect(result.Public).To(BeFalse())
|
|
Expect(result.CoverArt).To(BeEmpty())
|
|
Expect(result.OpenSubsonicPlaylist).To(BeNil())
|
|
})
|
|
})
|
|
|
|
Context("with non-minimal client", func() {
|
|
BeforeEach(func() {
|
|
conf.Server.Subsonic.MinimalClients = "minimal-client"
|
|
player := model.Player{Client: "regular-client"}
|
|
ctx = request.WithPlayer(ctx, player)
|
|
})
|
|
|
|
It("returns all fields", func() {
|
|
result := router.buildPlaylist(ctx, playlist)
|
|
Expect(result.Id).To(Equal("pls-1"))
|
|
Expect(result.Name).To(Equal("My Playlist"))
|
|
Expect(result.SongCount).To(Equal(int32(10)))
|
|
Expect(result.Duration).To(Equal(int32(600)))
|
|
Expect(result.Created).To(Equal(playlist.CreatedAt))
|
|
Expect(result.Changed).To(Equal(*playlist.EvaluatedAt))
|
|
Expect(result.Comment).To(Equal("Test comment"))
|
|
Expect(result.Owner).To(Equal("admin"))
|
|
Expect(result.Public).To(BeTrue())
|
|
Expect(result.Readonly).To(BeTrue())
|
|
Expect(result.ValidUntil).To(Equal(&validUntil))
|
|
})
|
|
})
|
|
|
|
Context("with legacy client", func() {
|
|
BeforeEach(func() {
|
|
conf.Server.Subsonic.LegacyClients = "legacy-client"
|
|
player := model.Player{Client: "legacy-client"}
|
|
ctx = request.WithPlayer(ctx, player)
|
|
})
|
|
|
|
It("returns standard fields but no OpenSubsonic extensions", func() {
|
|
result := router.buildPlaylist(ctx, playlist)
|
|
|
|
Expect(result.Comment).To(Equal("Test comment"))
|
|
Expect(result.Owner).To(Equal("admin"))
|
|
Expect(result.Public).To(BeTrue())
|
|
Expect(result.OpenSubsonicPlaylist).To(BeNil())
|
|
})
|
|
})
|
|
})
|
|
|
|
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() {
|
|
var router *Router
|
|
var ds model.DataStore
|
|
var playlists *fakePlaylists
|
|
|
|
BeforeEach(func() {
|
|
ds = &tests.MockDataStore{}
|
|
playlists = &fakePlaylists{}
|
|
router = New(ds, nil, nil, nil, nil, nil, nil, nil, playlists, nil, nil, nil, nil, nil, nil, nil)
|
|
})
|
|
|
|
It("clears the comment when parameter is empty", func() {
|
|
r := newGetRequest("playlistId=123", "comment=")
|
|
_, err := router.UpdatePlaylist(r)
|
|
Expect(err).ToNot(HaveOccurred())
|
|
Expect(playlists.lastPlaylistID).To(Equal("123"))
|
|
Expect(playlists.lastComment).ToNot(BeNil())
|
|
Expect(*playlists.lastComment).To(Equal(""))
|
|
})
|
|
|
|
It("leaves comment unchanged when parameter is missing", func() {
|
|
r := newGetRequest("playlistId=123")
|
|
_, err := router.UpdatePlaylist(r)
|
|
Expect(err).ToNot(HaveOccurred())
|
|
Expect(playlists.lastPlaylistID).To(Equal("123"))
|
|
Expect(playlists.lastComment).To(BeNil())
|
|
})
|
|
|
|
It("sets public to true when parameter is 'true'", func() {
|
|
r := newGetRequest("playlistId=123", "public=true")
|
|
_, err := router.UpdatePlaylist(r)
|
|
Expect(err).ToNot(HaveOccurred())
|
|
Expect(playlists.lastPlaylistID).To(Equal("123"))
|
|
Expect(playlists.lastPublic).ToNot(BeNil())
|
|
Expect(*playlists.lastPublic).To(BeTrue())
|
|
})
|
|
|
|
It("sets public to false when parameter is 'false'", func() {
|
|
r := newGetRequest("playlistId=123", "public=false")
|
|
_, err := router.UpdatePlaylist(r)
|
|
Expect(err).ToNot(HaveOccurred())
|
|
Expect(playlists.lastPlaylistID).To(Equal("123"))
|
|
Expect(playlists.lastPublic).ToNot(BeNil())
|
|
Expect(*playlists.lastPublic).To(BeFalse())
|
|
})
|
|
|
|
It("leaves public unchanged when parameter is missing", func() {
|
|
r := newGetRequest("playlistId=123")
|
|
_, err := router.UpdatePlaylist(r)
|
|
Expect(err).ToNot(HaveOccurred())
|
|
Expect(playlists.lastPlaylistID).To(Equal("123"))
|
|
Expect(playlists.lastPublic).To(BeNil())
|
|
})
|
|
})
|
|
|
|
type fakePlaylists struct {
|
|
playlists.Playlists
|
|
lastPlaylistID string
|
|
lastName *string
|
|
lastComment *string
|
|
lastPublic *bool
|
|
lastAdd []string
|
|
lastRemove []int
|
|
}
|
|
|
|
func (f *fakePlaylists) Update(ctx context.Context, playlistID string, name *string, comment *string, public *bool, idsToAdd []string, idxToRemove []int) error {
|
|
f.lastPlaylistID = playlistID
|
|
f.lastName = name
|
|
f.lastComment = comment
|
|
f.lastPublic = public
|
|
f.lastAdd = idsToAdd
|
|
f.lastRemove = idxToRemove
|
|
return nil
|
|
}
|