mirror of
https://github.com/navidrome/navidrome.git
synced 2026-08-31 07:30:32 +00: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.
290 lines
9.3 KiB
Go
290 lines
9.3 KiB
Go
package subsonic
|
|
|
|
import (
|
|
"context"
|
|
"fmt"
|
|
"net/http"
|
|
"time"
|
|
|
|
"github.com/navidrome/navidrome/core/scrobbler"
|
|
"github.com/navidrome/navidrome/model"
|
|
"github.com/navidrome/navidrome/model/request"
|
|
"github.com/navidrome/navidrome/server/events"
|
|
"github.com/navidrome/navidrome/server/subsonic/responses"
|
|
"github.com/navidrome/navidrome/tests"
|
|
. "github.com/onsi/ginkgo/v2"
|
|
. "github.com/onsi/gomega"
|
|
)
|
|
|
|
var _ = Describe("MediaAnnotationController", func() {
|
|
var router *Router
|
|
var ds model.DataStore
|
|
var playTracker *fakePlayTracker
|
|
var eventBroker *fakeEventBroker
|
|
var ctx context.Context
|
|
|
|
BeforeEach(func() {
|
|
ctx = context.Background()
|
|
ds = &tests.MockDataStore{}
|
|
playTracker = &fakePlayTracker{}
|
|
eventBroker = &fakeEventBroker{}
|
|
router = New(ds, nil, nil, nil, nil, nil, nil, eventBroker, nil, playTracker, nil, nil, nil, nil, nil, nil)
|
|
})
|
|
|
|
Describe("Scrobble", func() {
|
|
It("submit all scrobbles with only the id", func() {
|
|
// Back-date the baseline so the assertion still passes on platforms
|
|
// with millisecond clock resolution (e.g. Windows).
|
|
submissionTime := time.Now().Add(-time.Second)
|
|
r := newGetRequest("id=12", "id=34")
|
|
|
|
_, err := router.Scrobble(r)
|
|
|
|
Expect(err).ToNot(HaveOccurred())
|
|
Expect(playTracker.Submissions).To(HaveLen(2))
|
|
Expect(playTracker.Submissions[0].Timestamp).To(BeTemporally(">", submissionTime))
|
|
Expect(playTracker.Submissions[0].TrackID).To(Equal("12"))
|
|
Expect(playTracker.Submissions[1].Timestamp).To(BeTemporally(">", submissionTime))
|
|
Expect(playTracker.Submissions[1].TrackID).To(Equal("34"))
|
|
})
|
|
|
|
It("submit all scrobbles with respective times", func() {
|
|
time1 := time.Now().Add(-20 * time.Minute)
|
|
t1 := time1.UnixMilli()
|
|
time2 := time.Now().Add(-10 * time.Minute)
|
|
t2 := time2.UnixMilli()
|
|
r := newGetRequest("id=12", "id=34", fmt.Sprintf("time=%d", t1), fmt.Sprintf("time=%d", t2))
|
|
|
|
_, err := router.Scrobble(r)
|
|
|
|
Expect(err).ToNot(HaveOccurred())
|
|
Expect(playTracker.Submissions).To(HaveLen(2))
|
|
Expect(playTracker.Submissions[0].Timestamp).To(BeTemporally("~", time1))
|
|
Expect(playTracker.Submissions[0].TrackID).To(Equal("12"))
|
|
Expect(playTracker.Submissions[1].Timestamp).To(BeTemporally("~", time2))
|
|
Expect(playTracker.Submissions[1].TrackID).To(Equal("34"))
|
|
})
|
|
|
|
It("checks if number of ids match number of times", func() {
|
|
r := newGetRequest("id=12", "id=34", "time=1111")
|
|
|
|
_, err := router.Scrobble(r)
|
|
|
|
Expect(err).To(HaveOccurred())
|
|
Expect(playTracker.Submissions).To(BeEmpty())
|
|
})
|
|
|
|
Context("submission=false", func() {
|
|
var req *http.Request
|
|
BeforeEach(func() {
|
|
_ = ds.MediaFile(ctx).Put(&model.MediaFile{ID: "12"})
|
|
ctx = request.WithPlayer(ctx, model.Player{ID: "player-1"})
|
|
req = newGetRequest("id=12", "submission=false")
|
|
req = req.WithContext(ctx)
|
|
})
|
|
|
|
It("does not scrobble", func() {
|
|
_, err := router.Scrobble(req)
|
|
|
|
Expect(err).ToNot(HaveOccurred())
|
|
Expect(playTracker.Submissions).To(BeEmpty())
|
|
})
|
|
|
|
It("registers a NowPlaying via ReportPlayback", func() {
|
|
_, err := router.Scrobble(req)
|
|
|
|
Expect(err).ToNot(HaveOccurred())
|
|
Expect(playTracker.ReportedPlayback).To(HaveLen(1))
|
|
Expect(playTracker.ReportedPlayback[0].MediaId).To(Equal("12"))
|
|
Expect(playTracker.ReportedPlayback[0].State).To(Equal(scrobbler.StatePlaying))
|
|
Expect(playTracker.ReportedPlayback[0].ClientId).To(Equal("player-1"))
|
|
})
|
|
})
|
|
})
|
|
|
|
Describe("ReportPlayback", func() {
|
|
It("returns error when mediaId is missing", func() {
|
|
r := newGetRequest("mediaType=song", "positionMs=0", "state=playing")
|
|
_, err := router.ReportPlayback(r)
|
|
Expect(err).To(HaveOccurred())
|
|
})
|
|
|
|
It("returns error when mediaType is missing", func() {
|
|
r := newGetRequest("mediaId=123", "positionMs=0", "state=playing")
|
|
_, err := router.ReportPlayback(r)
|
|
Expect(err).To(HaveOccurred())
|
|
})
|
|
|
|
It("returns error when positionMs is missing", func() {
|
|
r := newGetRequest("mediaId=123", "mediaType=song", "state=playing")
|
|
_, err := router.ReportPlayback(r)
|
|
Expect(err).To(HaveOccurred())
|
|
})
|
|
|
|
It("returns error when state is missing", func() {
|
|
r := newGetRequest("mediaId=123", "mediaType=song", "positionMs=0")
|
|
_, err := router.ReportPlayback(r)
|
|
Expect(err).To(HaveOccurred())
|
|
})
|
|
|
|
It("returns error for invalid state value", func() {
|
|
r := newGetRequest("mediaId=123", "mediaType=song", "positionMs=0", "state=invalid")
|
|
_, err := router.ReportPlayback(r)
|
|
Expect(err).To(HaveOccurred())
|
|
})
|
|
|
|
It("returns error for negative positionMs", func() {
|
|
r := newGetRequest("mediaId=123", "mediaType=song", "positionMs=-1", "state=playing")
|
|
_, err := router.ReportPlayback(r)
|
|
Expect(err).To(HaveOccurred())
|
|
})
|
|
|
|
It("returns error for NaN playbackRate", func() {
|
|
r := newGetRequest("mediaId=123", "mediaType=song", "positionMs=0", "state=playing", "playbackRate=NaN")
|
|
_, err := router.ReportPlayback(r)
|
|
Expect(err).To(HaveOccurred())
|
|
})
|
|
|
|
It("returns error for Inf playbackRate", func() {
|
|
r := newGetRequest("mediaId=123", "mediaType=song", "positionMs=0", "state=playing", "playbackRate=Inf")
|
|
_, err := router.ReportPlayback(r)
|
|
Expect(err).To(HaveOccurred())
|
|
})
|
|
|
|
It("returns error for negative playbackRate", func() {
|
|
r := newGetRequest("mediaId=123", "mediaType=song", "positionMs=0", "state=playing", "playbackRate=-1.0")
|
|
_, err := router.ReportPlayback(r)
|
|
Expect(err).To(HaveOccurred())
|
|
})
|
|
|
|
It("returns error for zero playbackRate", func() {
|
|
r := newGetRequest("mediaId=123", "mediaType=song", "positionMs=0", "state=playing", "playbackRate=0")
|
|
_, err := router.ReportPlayback(r)
|
|
Expect(err).To(HaveOccurred())
|
|
})
|
|
|
|
It("accepts mediaType=podcast without error", func() {
|
|
r := newGetRequest("mediaId=123", "mediaType=podcast", "positionMs=0", "state=playing")
|
|
ctx := request.WithPlayer(r.Context(), model.Player{ID: "p1"})
|
|
r = r.WithContext(ctx)
|
|
resp, err := router.ReportPlayback(r)
|
|
Expect(err).ToNot(HaveOccurred())
|
|
Expect(resp.Status).To(Equal(responses.StatusOK))
|
|
})
|
|
|
|
It("defaults playbackRate to 1.0 and ignoreScrobble to false", func() {
|
|
r := newGetRequest("mediaId=123", "mediaType=song", "positionMs=5000", "state=playing")
|
|
ctx := request.WithPlayer(r.Context(), model.Player{ID: "p1"})
|
|
r = r.WithContext(ctx)
|
|
_, err := router.ReportPlayback(r)
|
|
Expect(err).ToNot(HaveOccurred())
|
|
Expect(playTracker.ReportedPlayback).To(HaveLen(1))
|
|
Expect(playTracker.ReportedPlayback[0].PlaybackRate).To(Equal(1.0))
|
|
Expect(playTracker.ReportedPlayback[0].IgnoreScrobble).To(BeFalse())
|
|
Expect(playTracker.ReportedPlayback[0].ClientId).To(Equal("p1"))
|
|
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 {
|
|
Submissions []scrobbler.Submission
|
|
ReportedPlayback []scrobbler.ReportPlaybackParams
|
|
Error error
|
|
}
|
|
|
|
func (f *fakePlayTracker) GetNowPlaying(_ context.Context) ([]scrobbler.PlaybackSession, error) {
|
|
return nil, f.Error
|
|
}
|
|
|
|
func (f *fakePlayTracker) Submit(_ context.Context, submissions []scrobbler.Submission) error {
|
|
if f.Error != nil {
|
|
return f.Error
|
|
}
|
|
f.Submissions = append(f.Submissions, submissions...)
|
|
return nil
|
|
}
|
|
|
|
func (f *fakePlayTracker) ReportPlayback(_ context.Context, params scrobbler.ReportPlaybackParams) error {
|
|
if f.Error != nil {
|
|
return f.Error
|
|
}
|
|
f.ReportedPlayback = append(f.ReportedPlayback, params)
|
|
return nil
|
|
}
|
|
|
|
var _ scrobbler.PlayTracker = (*fakePlayTracker)(nil)
|
|
|
|
type fakeEventBroker struct {
|
|
http.Handler
|
|
Events []events.Event
|
|
}
|
|
|
|
func (f *fakeEventBroker) SendMessage(_ context.Context, event events.Event) {
|
|
f.Events = append(f.Events, event)
|
|
}
|
|
|
|
func (f *fakeEventBroker) SendBroadcastMessage(_ context.Context, event events.Event) {
|
|
f.Events = append(f.Events, event)
|
|
}
|
|
|
|
var _ events.Broker = (*fakeEventBroker)(nil)
|