mirror of
https://github.com/navidrome/navidrome.git
synced 2026-08-01 07:21:17 +00:00
feat(jellyfin): emit refreshResource events on favorite/rating changes
Like Subsonic's setStar/setRating, the Jellyfin favorite and rating endpoints now broadcast a refreshResource event, so the web UI updates immediately when a Jellyfin client changes an annotation. Also fixes model.GetEntityByID to propagate unexpected repository errors instead of reporting them as not-found, preserving the 500-vs-404 distinction for all its callers.
This commit is contained in:
parent
f61b4eee21
commit
e4a423db11
@ -138,7 +138,7 @@ func CreateJellyfinAPIRouter(ctx context.Context) *jellyfin.Router {
|
||||
playlistsPlaylists := playlists.NewPlaylists(dataStore, imageUploadService)
|
||||
sonicSonic := sonic.New(dataStore, manager, matcherMatcher)
|
||||
lyricsLyrics := lyrics.NewLyrics(dataStore, manager)
|
||||
router := jellyfin.New(dataStore, artworkArtwork, mediaStreamer, transcodeDecider, players, playTracker, playlistsPlaylists, provider, sonicSonic, lyricsLyrics)
|
||||
router := jellyfin.New(dataStore, artworkArtwork, mediaStreamer, transcodeDecider, players, playTracker, playlistsPlaylists, provider, sonicSonic, lyricsLyrics, broker)
|
||||
return router
|
||||
}
|
||||
|
||||
|
||||
@ -2,29 +2,26 @@ package model
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
)
|
||||
|
||||
// TODO: Should the type be encoded in the ID?
|
||||
func GetEntityByID(ctx context.Context, ds DataStore, id string) (any, error) {
|
||||
ar, err := ds.Artist(ctx).Get(id)
|
||||
if err == nil {
|
||||
return ar, nil
|
||||
getters := []func() (any, error){
|
||||
func() (any, error) { return ds.Artist(ctx).Get(id) },
|
||||
func() (any, error) { return ds.Album(ctx).Get(id) },
|
||||
func() (any, error) { return ds.Playlist(ctx).Get(id) },
|
||||
func() (any, error) { return ds.MediaFile(ctx).Get(id) },
|
||||
func() (any, error) { return ds.Radio(ctx).Get(id) },
|
||||
}
|
||||
al, err := ds.Album(ctx).Get(id)
|
||||
if err == nil {
|
||||
return al, nil
|
||||
for _, get := range getters {
|
||||
entity, err := get()
|
||||
if err == nil {
|
||||
return entity, nil
|
||||
}
|
||||
if !errors.Is(err, ErrNotFound) {
|
||||
return nil, err
|
||||
}
|
||||
}
|
||||
pls, err := ds.Playlist(ctx).Get(id)
|
||||
if err == nil {
|
||||
return pls, nil
|
||||
}
|
||||
mf, err := ds.MediaFile(ctx).Get(id)
|
||||
if err == nil {
|
||||
return mf, nil
|
||||
}
|
||||
r, err := ds.Radio(ctx).Get(id)
|
||||
if err == nil {
|
||||
return r, nil
|
||||
}
|
||||
return nil, err
|
||||
return nil, ErrNotFound
|
||||
}
|
||||
|
||||
40
model/get_entity_test.go
Normal file
40
model/get_entity_test.go
Normal file
@ -0,0 +1,40 @@
|
||||
package model_test
|
||||
|
||||
import (
|
||||
"context"
|
||||
|
||||
"github.com/navidrome/navidrome/model"
|
||||
"github.com/navidrome/navidrome/tests"
|
||||
. "github.com/onsi/ginkgo/v2"
|
||||
. "github.com/onsi/gomega"
|
||||
)
|
||||
|
||||
var _ = Describe("GetEntityByID", func() {
|
||||
var ds *tests.MockDataStore
|
||||
var ctx context.Context
|
||||
|
||||
BeforeEach(func() {
|
||||
ds = &tests.MockDataStore{}
|
||||
ctx = GinkgoT().Context()
|
||||
})
|
||||
|
||||
It("returns the entity matching the id", func() {
|
||||
ds.Album(ctx).(*tests.MockAlbumRepo).SetData(model.Albums{{ID: "a1", Name: "One"}})
|
||||
entity, err := model.GetEntityByID(ctx, ds, "a1")
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
Expect(entity).To(BeAssignableToTypeOf(&model.Album{}))
|
||||
Expect(entity.(*model.Album).ID).To(Equal("a1"))
|
||||
})
|
||||
|
||||
It("returns ErrNotFound when no entity matches", func() {
|
||||
_, err := model.GetEntityByID(ctx, ds, "missing")
|
||||
Expect(err).To(MatchError(model.ErrNotFound))
|
||||
})
|
||||
|
||||
It("propagates unexpected repository errors instead of reporting not-found", func() {
|
||||
ds.Album(ctx).(*tests.MockAlbumRepo).SetError(true)
|
||||
_, err := model.GetEntityByID(ctx, ds, "a1")
|
||||
Expect(err).To(HaveOccurred())
|
||||
Expect(err).ToNot(MatchError(model.ErrNotFound))
|
||||
})
|
||||
})
|
||||
@ -8,53 +8,41 @@ import (
|
||||
"github.com/go-chi/chi/v5"
|
||||
"github.com/navidrome/navidrome/model"
|
||||
"github.com/navidrome/navidrome/model/request"
|
||||
"github.com/navidrome/navidrome/server/events"
|
||||
"github.com/navidrome/navidrome/server/jellyfin/dto"
|
||||
"github.com/navidrome/navidrome/utils/req"
|
||||
)
|
||||
|
||||
// resolveAnnotated finds which annotated repo owns id. Albums and songs 404 when the user can't
|
||||
// access their library; artists span libraries (library_artist), so have no single LibraryID to
|
||||
// gate on and rely on list-time scoping. PlaylistRepository.Get enforces playlist visibility.
|
||||
// When ok is false the response has already been written, so callers must return without writing
|
||||
// the annotation.
|
||||
func (api *Router) resolveAnnotated(w http.ResponseWriter, r *http.Request, id string) (repo model.AnnotatedRepository, ok bool) {
|
||||
// resolveAnnotated finds which annotated repo owns id, returning the resource name used in
|
||||
// refreshResource events. Albums and songs 404 when the user can't access their library; artists
|
||||
// span libraries (library_artist), so have no single LibraryID to gate on and rely on list-time
|
||||
// scoping. PlaylistRepository.Get enforces playlist visibility. When repo is nil the response has
|
||||
// already been written, so callers must return without writing the annotation.
|
||||
func (api *Router) resolveAnnotated(w http.ResponseWriter, r *http.Request, id string) (repo model.AnnotatedRepository, resource string) {
|
||||
ctx := r.Context()
|
||||
entity, err := model.GetEntityByID(ctx, api.ds, id)
|
||||
if err != nil && !errors.Is(err, model.ErrNotFound) {
|
||||
api.internalError(w, r, err)
|
||||
return nil, ""
|
||||
}
|
||||
u, _ := request.UserFrom(ctx)
|
||||
if al, err := api.ds.Album(ctx).Get(id); err == nil {
|
||||
if !u.HasLibraryAccess(al.LibraryID) {
|
||||
http.Error(w, "Not Found", http.StatusNotFound)
|
||||
return nil, false
|
||||
switch e := entity.(type) {
|
||||
case *model.Album:
|
||||
if u.HasLibraryAccess(e.LibraryID) {
|
||||
return api.ds.Album(ctx), "album"
|
||||
}
|
||||
return api.ds.Album(ctx), true
|
||||
} else if !errors.Is(err, model.ErrNotFound) {
|
||||
api.internalError(w, r, err)
|
||||
return nil, false
|
||||
}
|
||||
if _, err := api.ds.Artist(ctx).Get(id); err == nil {
|
||||
return api.ds.Artist(ctx), true
|
||||
} else if !errors.Is(err, model.ErrNotFound) {
|
||||
api.internalError(w, r, err)
|
||||
return nil, false
|
||||
}
|
||||
if mf, err := api.ds.MediaFile(ctx).Get(id); err == nil {
|
||||
if !u.HasLibraryAccess(mf.LibraryID) {
|
||||
http.Error(w, "Not Found", http.StatusNotFound)
|
||||
return nil, false
|
||||
case *model.Artist:
|
||||
return api.ds.Artist(ctx), "artist"
|
||||
case *model.MediaFile:
|
||||
if u.HasLibraryAccess(e.LibraryID) {
|
||||
return api.ds.MediaFile(ctx), "song"
|
||||
}
|
||||
return api.ds.MediaFile(ctx), true
|
||||
} else if !errors.Is(err, model.ErrNotFound) {
|
||||
api.internalError(w, r, err)
|
||||
return nil, false
|
||||
}
|
||||
playlistRepo := api.ds.Playlist(ctx)
|
||||
if _, err := playlistRepo.Get(id); err == nil {
|
||||
return playlistRepo, true
|
||||
} else if !errors.Is(err, model.ErrNotFound) {
|
||||
api.internalError(w, r, err)
|
||||
return nil, false
|
||||
case *model.Playlist:
|
||||
return api.ds.Playlist(ctx), "playlist"
|
||||
}
|
||||
// Unknown ids, inaccessible-library items and non-annotatable entities (radios) all read as absent.
|
||||
http.Error(w, "Not Found", http.StatusNotFound)
|
||||
return nil, false
|
||||
return nil, ""
|
||||
}
|
||||
|
||||
// getUserItemData returns the caller's play/favorite/rating state for a single item. Jellify
|
||||
@ -77,14 +65,15 @@ func (api *Router) getUserItemData(w http.ResponseWriter, r *http.Request) {
|
||||
|
||||
func (api *Router) setFavorite(w http.ResponseWriter, r *http.Request, starred bool) {
|
||||
id := api.resolveItemID(r.Context(), dto.DecodeID(chi.URLParam(r, "itemId")))
|
||||
repo, ok := api.resolveAnnotated(w, r, id)
|
||||
if !ok {
|
||||
repo, resource := api.resolveAnnotated(w, r, id)
|
||||
if repo == nil {
|
||||
return
|
||||
}
|
||||
if err := repo.SetStar(starred, id); err != nil {
|
||||
api.internalError(w, r, err)
|
||||
return
|
||||
}
|
||||
api.broker.SendMessage(r.Context(), (&events.RefreshResource{}).With(resource, id))
|
||||
encodedID := dto.EncodeID(id)
|
||||
api.ok(w, r, &dto.UserItemDataDto{IsFavorite: starred, Key: encodedID, ItemId: encodedID})
|
||||
}
|
||||
@ -96,14 +85,15 @@ func (api *Router) unmarkFavorite(w http.ResponseWriter, r *http.Request) {
|
||||
|
||||
func (api *Router) setItemRating(w http.ResponseWriter, r *http.Request, rating int) {
|
||||
id := api.resolveItemID(r.Context(), dto.DecodeID(chi.URLParam(r, "itemId")))
|
||||
repo, ok := api.resolveAnnotated(w, r, id)
|
||||
if !ok {
|
||||
repo, resource := api.resolveAnnotated(w, r, id)
|
||||
if repo == nil {
|
||||
return
|
||||
}
|
||||
if err := repo.SetRating(rating, id); err != nil {
|
||||
api.internalError(w, r, err)
|
||||
return
|
||||
}
|
||||
api.broker.SendMessage(r.Context(), (&events.RefreshResource{}).With(resource, id))
|
||||
encodedID := dto.EncodeID(id)
|
||||
d := &dto.UserItemDataDto{Key: encodedID, ItemId: encodedID}
|
||||
if rating > 0 {
|
||||
|
||||
@ -8,6 +8,7 @@ import (
|
||||
|
||||
"github.com/navidrome/navidrome/model"
|
||||
"github.com/navidrome/navidrome/model/request"
|
||||
"github.com/navidrome/navidrome/server/events"
|
||||
"github.com/navidrome/navidrome/server/jellyfin/dto"
|
||||
"github.com/navidrome/navidrome/tests"
|
||||
. "github.com/onsi/ginkgo/v2"
|
||||
@ -17,6 +18,7 @@ import (
|
||||
var _ = Describe("Annotations", func() {
|
||||
var api *Router
|
||||
var ds *tests.MockDataStore
|
||||
var broker *fakeEventBroker
|
||||
// alice has access to library 1 only.
|
||||
ctxUser := func() context.Context {
|
||||
return request.WithUser(context.Background(), model.User{ID: "u1", UserName: "alice", Libraries: model.Libraries{{ID: 1, Name: "Music"}}})
|
||||
@ -24,7 +26,8 @@ var _ = Describe("Annotations", func() {
|
||||
|
||||
BeforeEach(func() {
|
||||
ds = &tests.MockDataStore{}
|
||||
api = &Router{ds: ds}
|
||||
broker = &fakeEventBroker{}
|
||||
api = &Router{ds: ds, broker: broker}
|
||||
})
|
||||
|
||||
Describe("markFavorite / unmarkFavorite", func() {
|
||||
@ -134,6 +137,39 @@ var _ = Describe("Annotations", func() {
|
||||
invoke(api.markFavorite, w, r)
|
||||
Expect(w.Code).To(Equal(http.StatusInternalServerError))
|
||||
})
|
||||
|
||||
It("emits a refreshResource event when starring a song", func() {
|
||||
mfRepo := ds.MediaFile(context.Background()).(*tests.MockMediaFileRepo)
|
||||
mfRepo.SetData(model.MediaFiles{{ID: "s1", Title: "Song", LibraryID: 1}})
|
||||
w := httptest.NewRecorder()
|
||||
r := httptest.NewRequest("POST", "/Users/u1/FavoriteItems/s1", nil).WithContext(ctxUser())
|
||||
r = withChiURLParam(r, "itemId", "s1")
|
||||
invoke(api.markFavorite, w, r)
|
||||
Expect(broker.Events).To(HaveLen(1))
|
||||
Expect(broker.Events[0].Data(broker.Events[0])).To(Equal(`{"song":["s1"]}`))
|
||||
})
|
||||
|
||||
It("emits a refreshResource event when starring an album", func() {
|
||||
albumRepo := ds.Album(context.Background()).(*tests.MockAlbumRepo)
|
||||
albumRepo.SetData(model.Albums{{ID: "a1", Name: "One", LibraryID: 1}})
|
||||
w := httptest.NewRecorder()
|
||||
r := httptest.NewRequest("POST", "/Users/u1/FavoriteItems/"+dto.EncodeID("a1"), nil).WithContext(ctxUser())
|
||||
r = withChiURLParam(r, "itemId", dto.EncodeID("a1"))
|
||||
invoke(api.markFavorite, w, r)
|
||||
Expect(broker.Events).To(HaveLen(1))
|
||||
Expect(broker.Events[0].Data(broker.Events[0])).To(Equal(`{"album":["a1"]}`))
|
||||
})
|
||||
|
||||
It("does not emit an event when the item is not accessible", func() {
|
||||
albumRepo := ds.Album(context.Background()).(*tests.MockAlbumRepo)
|
||||
albumRepo.SetData(model.Albums{{ID: "a1", Name: "One", LibraryID: 2}})
|
||||
w := httptest.NewRecorder()
|
||||
r := httptest.NewRequest("POST", "/Users/u1/FavoriteItems/"+dto.EncodeID("a1"), nil).WithContext(ctxUser())
|
||||
r = withChiURLParam(r, "itemId", dto.EncodeID("a1"))
|
||||
invoke(api.markFavorite, w, r)
|
||||
Expect(w.Code).To(Equal(http.StatusNotFound))
|
||||
Expect(broker.Events).To(BeEmpty())
|
||||
})
|
||||
})
|
||||
|
||||
Describe("setRating / removeRating", func() {
|
||||
@ -253,5 +289,31 @@ var _ = Describe("Annotations", func() {
|
||||
Expect(w.Code).To(Equal(http.StatusOK))
|
||||
Expect(mfRepo.Data["s1"].Rating).To(Equal(0))
|
||||
})
|
||||
|
||||
It("emits a refreshResource event when rating a song", func() {
|
||||
mfRepo := ds.MediaFile(context.Background()).(*tests.MockMediaFileRepo)
|
||||
mfRepo.SetData(model.MediaFiles{{ID: "s1", Title: "Song", LibraryID: 1}})
|
||||
w := httptest.NewRecorder()
|
||||
r := httptest.NewRequest("POST", "/Users/u1/Items/s1/Rating?Rating=8", nil).WithContext(ctxUser())
|
||||
r = withChiURLParam(r, "itemId", "s1")
|
||||
invoke(api.setRating, w, r)
|
||||
Expect(broker.Events).To(HaveLen(1))
|
||||
Expect(broker.Events[0].Data(broker.Events[0])).To(Equal(`{"song":["s1"]}`))
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
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)
|
||||
|
||||
@ -22,6 +22,7 @@ import (
|
||||
"github.com/navidrome/navidrome/log"
|
||||
"github.com/navidrome/navidrome/model"
|
||||
"github.com/navidrome/navidrome/server"
|
||||
"github.com/navidrome/navidrome/server/events"
|
||||
"github.com/navidrome/navidrome/server/jellyfin/dto"
|
||||
"github.com/navidrome/navidrome/utils/cache"
|
||||
)
|
||||
@ -38,6 +39,7 @@ type Router struct {
|
||||
provider external.Provider
|
||||
sonic sonic.Engine
|
||||
lyrics lyrics.Lyrics
|
||||
broker events.Broker
|
||||
lyricsCache cache.SimpleCache[string, model.LyricList]
|
||||
similarFlight singleflight.Group
|
||||
serverIDMu sync.Mutex
|
||||
@ -47,11 +49,11 @@ type Router struct {
|
||||
func New(ds model.DataStore, artwork artwork.Artwork, streamer stream.MediaStreamer,
|
||||
transcodeDecider stream.TranscodeDecider, players core.Players,
|
||||
scrobbler scrobbler.PlayTracker, playlists playlists.Playlists, provider external.Provider,
|
||||
sonicSvc sonic.Engine, lyricsSvc lyrics.Lyrics) *Router {
|
||||
sonicSvc sonic.Engine, lyricsSvc lyrics.Lyrics, broker events.Broker) *Router {
|
||||
r := &Router{
|
||||
ds: ds, artwork: artwork, streamer: streamer, transcodeDecider: transcodeDecider,
|
||||
players: players, scrobbler: scrobbler, playlists: playlists, provider: provider,
|
||||
sonic: sonicSvc, lyrics: lyricsSvc,
|
||||
sonic: sonicSvc, lyrics: lyricsSvc, broker: broker,
|
||||
lyricsCache: cache.NewSimpleCache[string, model.LyricList](cache.Options{
|
||||
SizeLimit: 1000,
|
||||
DefaultTTL: 5 * time.Minute,
|
||||
|
||||
@ -18,7 +18,7 @@ import (
|
||||
var _ = Describe("Router", func() {
|
||||
It("serves the public handshake through the mounted handler", func() {
|
||||
ds := &tests.MockDataStore{}
|
||||
api := New(ds, nil, nil, nil, nil, nil, nil, nil, nil, nil)
|
||||
api := New(ds, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil)
|
||||
w := httptest.NewRecorder()
|
||||
r := httptest.NewRequest("GET", "/System/Info/Public", nil)
|
||||
api.ServeHTTP(w, r)
|
||||
@ -26,7 +26,7 @@ var _ = Describe("Router", func() {
|
||||
})
|
||||
|
||||
It("returns 404 JSON for unknown routes", func() {
|
||||
api := New(&tests.MockDataStore{}, nil, nil, nil, nil, nil, nil, nil, nil, nil)
|
||||
api := New(&tests.MockDataStore{}, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil)
|
||||
w := httptest.NewRecorder()
|
||||
r := httptest.NewRequest("GET", "/Nonexistent/Route", nil)
|
||||
api.ServeHTTP(w, r)
|
||||
@ -36,7 +36,7 @@ var _ = Describe("Router", func() {
|
||||
})
|
||||
|
||||
It("returns 404 JSON for a known path with an unsupported method", func() {
|
||||
api := New(&tests.MockDataStore{}, nil, nil, nil, nil, nil, nil, nil, nil, nil)
|
||||
api := New(&tests.MockDataStore{}, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil)
|
||||
w := httptest.NewRecorder()
|
||||
r := httptest.NewRequest("PATCH", "/System/Info/Public", nil)
|
||||
api.ServeHTTP(w, r)
|
||||
@ -53,7 +53,7 @@ var _ = Describe("Router", func() {
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
|
||||
fp := &fakePlayers{}
|
||||
api := New(ds, nil, nil, nil, fp, nil, nil, nil, nil, nil)
|
||||
api := New(ds, nil, nil, nil, fp, nil, nil, nil, nil, nil, nil)
|
||||
|
||||
w := httptest.NewRecorder()
|
||||
r := httptest.NewRequest("GET", "/Users/Me", nil)
|
||||
@ -70,7 +70,7 @@ var _ = Describe("Router", func() {
|
||||
DeferCleanup(configtest.SetupConfig())
|
||||
conf.Server.AuthRequestLimit = 2
|
||||
conf.Server.AuthWindowLength = time.Minute
|
||||
api := New(&tests.MockDataStore{}, nil, nil, nil, nil, nil, nil, nil, nil, nil)
|
||||
api := New(&tests.MockDataStore{}, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil)
|
||||
|
||||
login := func() int {
|
||||
w := httptest.NewRecorder()
|
||||
|
||||
@ -329,6 +329,7 @@ func setupTestDB() {
|
||||
providerFake,
|
||||
sonicSvc,
|
||||
lyrics.NewLyrics(ds, nil),
|
||||
events.NoopBroker(),
|
||||
)
|
||||
}
|
||||
|
||||
|
||||
@ -19,7 +19,7 @@ var _ = Describe("Case-insensitive routing", func() {
|
||||
var api *Router
|
||||
|
||||
BeforeEach(func() {
|
||||
api = New(&tests.MockDataStore{}, nil, nil, nil, nil, nil, nil, nil, nil, nil)
|
||||
api = New(&tests.MockDataStore{}, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil)
|
||||
})
|
||||
|
||||
It("serves a fully lowercase path directly", func() {
|
||||
|
||||
@ -94,7 +94,7 @@ var _ = Describe("handleSocket", func() {
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
token = t
|
||||
|
||||
api = New(ds, nil, nil, nil, nil, nil, nil, nil, nil, nil)
|
||||
api = New(ds, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil)
|
||||
})
|
||||
|
||||
It("upgrades when authenticated via the api_key query parameter", func() {
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user