diff --git a/cmd/artwork.go b/cmd/artwork.go index 8b9e28f0e..5cd4fc146 100644 --- a/cmd/artwork.go +++ b/cmd/artwork.go @@ -648,7 +648,7 @@ func refreshItems(ctx context.Context, ds model.DataStore, targets []model.Artwo for _, t := range targets { kind, id := t.Kind, t.ID // artwork.Refresh would happily queue an id that does not exist, orphaning a queue row. - if _, err := artworkItemName(ctx, ds, kind, id); err != nil { + if _, err := artwork.ItemName(ctx, ds, kind, id); err != nil { log.Error(ctx, "Item not found", "kind", kind, "id", id, err) failed++ continue @@ -963,7 +963,7 @@ func runExplain(ctx context.Context, args []string) { } kind, id := targets[0].Kind, targets[0].ID - name, err := artworkItemName(ctx, ds, kind, id) + name, err := artwork.ItemName(ctx, ds, kind, id) if err != nil { log.Fatal(ctx, "Item not found", "kind", kind, "id", id, err) } @@ -1005,60 +1005,3 @@ func runExplain(ctx context.Context, args []string) { log.Fatal(ctx, "Failed to resolve artwork", "kind", kind, "id", id, rep.resolveErr) } } - -// artworkItemName looks the entity up under its own kind, so a mismatched kind/id pair is -// reported as not found instead of silently explaining another entity's artwork. -func artworkItemName(ctx context.Context, ds model.DataStore, kind model.Kind, id string) (string, error) { - switch kind { - case model.KindArtistArtwork: - ar, err := ds.Artist(ctx).Get(id) - if err != nil { - return "", err - } - return ar.Name, nil - case model.KindAlbumArtwork: - al, err := ds.Album(ctx).Get(id) - if err != nil { - return "", err - } - return al.Name, nil - case model.KindPlaylistArtwork: - pls, err := ds.Playlist(ctx).Get(id) - if err != nil { - return "", err - } - return pls.Name, nil - case model.KindRadioArtwork: - rd, err := ds.Radio(ctx).Get(id) - if err != nil { - return "", err - } - return rd.Name, nil - case model.KindMediaFileArtwork: - mf, err := ds.MediaFile(ctx).Get(id) - if err != nil { - return "", err - } - return mf.Title, nil - case model.KindDiscArtwork: - return discArtworkName(ctx, ds, id) - } - return "", fmt.Errorf("unsupported kind %q", kind.Prefix()) -} - -func discArtworkName(ctx context.Context, ds model.DataStore, id string) (string, error) { - albumID, discNumber, err := model.ParseDiscArtworkID(id) - if err != nil { - return "", err - } - al, err := ds.Album(ctx).Get(albumID) - if err != nil { - return "", err - } - name := fmt.Sprintf("%s (disc %d)", al.Name, discNumber) - // The subtitle is itself a DiscArtPriority candidate, so name it where the chain can be read against it. - if subtitle := strings.TrimSpace(al.Discs[discNumber]); subtitle != "" { - name += ": " + subtitle - } - return name, nil -} diff --git a/cmd/artwork_test.go b/cmd/artwork_test.go index f17206aaa..a7220d2d5 100644 --- a/cmd/artwork_test.go +++ b/cmd/artwork_test.go @@ -424,33 +424,6 @@ var _ = Describe("explainConfig", func() { ) }) -var _ = Describe("discArtworkName", func() { - var ds *tests.MockDataStore - - BeforeEach(func() { - albumRepo := tests.CreateMockAlbumRepo() - albumRepo.SetData(model.Albums{{ID: "al-1", Name: "Sandinista!", Discs: model.Discs{2: "Side Three"}}}) - ds = &tests.MockDataStore{MockedAlbum: albumRepo} - }) - - It("names the album, the disc and its subtitle", func() { - name, err := artworkItemName(context.Background(), ds, model.KindDiscArtwork, "al-1:2") - Expect(err).ToNot(HaveOccurred()) - Expect(name).To(Equal("Sandinista! (disc 2): Side Three")) - }) - - It("omits the subtitle when the disc has none", func() { - name, err := artworkItemName(context.Background(), ds, model.KindDiscArtwork, "al-1:1") - Expect(err).ToNot(HaveOccurred()) - Expect(name).To(Equal("Sandinista! (disc 1)")) - }) - - It("rejects an id that is not :", func() { - _, err := artworkItemName(context.Background(), ds, model.KindDiscArtwork, "al-1") - Expect(err).To(HaveOccurred()) - }) -}) - var _ = Describe("artwork refresh command", func() { It("requires at least one argument", func() { Expect(artworkRefreshCmd.Args(artworkRefreshCmd, []string{})).To(HaveOccurred()) diff --git a/cmd/wire_gen.go b/cmd/wire_gen.go index 49a99f8ca..de4c55a1e 100644 --- a/cmd/wire_gen.go +++ b/cmd/wire_gen.go @@ -76,7 +76,10 @@ func CreateNativeAPIRouter(ctx context.Context) *nativeapi.Router { library := core.NewLibrary(dataStore, modelScanner, watcher, broker, manager) user := core.NewUser(dataStore, manager) maintenance := core.NewMaintenance(dataStore) - router := nativeapi.New(dataStore, share, playlistsPlaylists, insights, library, user, maintenance, manager, uploader) + agentsAgents := agents.GetAgents(dataStore, manager) + matcherMatcher := matcher.New(dataStore) + provider := external.NewProvider(dataStore, agentsAgents, matcherMatcher, broker) + router := nativeapi.New(dataStore, share, playlistsPlaylists, insights, library, user, maintenance, manager, uploader, provider) return router } @@ -97,7 +100,7 @@ func CreateSubsonicAPIRouter(ctx context.Context) *subsonic.Router { manager := plugins.GetManager(dataStore, broker, metricsMetrics) agentsAgents := agents.GetAgents(dataStore, manager) matcherMatcher := matcher.New(dataStore) - provider := external.NewProvider(dataStore, agentsAgents, matcherMatcher) + provider := external.NewProvider(dataStore, agentsAgents, matcherMatcher, broker) uploader := artwork.NewUploader(dataStore) playlistsPlaylists := playlists.NewPlaylists(dataStore, uploader) modelScanner := scanner.New(ctx, dataStore, broker, playlistsPlaylists, metricsMetrics) @@ -129,7 +132,7 @@ func CreateJellyfinAPIRouter(ctx context.Context) *jellyfin.Router { playlistsPlaylists := playlists.NewPlaylists(dataStore, uploader) agentsAgents := agents.GetAgents(dataStore, manager) matcherMatcher := matcher.New(dataStore) - provider := external.NewProvider(dataStore, agentsAgents, matcherMatcher) + provider := external.NewProvider(dataStore, agentsAgents, matcherMatcher, broker) sonicSonic := sonic.New(dataStore, manager, matcherMatcher) lyricsLyrics := lyrics.NewLyrics(dataStore, manager) router := jellyfin.New(dataStore, artworkArtwork, mediaStreamer, transcodeDecider, players, playTracker, playlistsPlaylists, provider, sonicSonic, lyricsLyrics, broker) diff --git a/core/artwork/housekeeping.go b/core/artwork/housekeeping.go index ce98e2a03..a3330d7cf 100644 --- a/core/artwork/housekeeping.go +++ b/core/artwork/housekeeping.go @@ -166,6 +166,63 @@ func enqueueMissingAll(ctx context.Context, ds model.DataStore) error { return nil } +// ItemName resolves a kind+id to the entity's display name, and errors when the item +// does not exist. Callers use it to reject ids that would otherwise orphan a queue row. +func ItemName(ctx context.Context, ds model.DataStore, kind model.Kind, id string) (string, error) { + switch kind { + case model.KindArtistArtwork: + ar, err := ds.Artist(ctx).Get(id) + if err != nil { + return "", err + } + return ar.Name, nil + case model.KindAlbumArtwork: + al, err := ds.Album(ctx).Get(id) + if err != nil { + return "", err + } + return al.Name, nil + case model.KindPlaylistArtwork: + pls, err := ds.Playlist(ctx).Get(id) + if err != nil { + return "", err + } + return pls.Name, nil + case model.KindRadioArtwork: + rd, err := ds.Radio(ctx).Get(id) + if err != nil { + return "", err + } + return rd.Name, nil + case model.KindMediaFileArtwork: + mf, err := ds.MediaFile(ctx).Get(id) + if err != nil { + return "", err + } + return mf.Title, nil + case model.KindDiscArtwork: + return discArtworkName(ctx, ds, id) + } + return "", fmt.Errorf("unsupported kind %q", kind.Prefix()) +} + +func discArtworkName(ctx context.Context, ds model.DataStore, id string) (string, error) { + albumID, discNumber, err := model.ParseDiscArtworkID(id) + if err != nil { + return "", err + } + al, err := ds.Album(ctx).Get(albumID) + if err != nil { + return "", err + } + name := fmt.Sprintf("%s (disc %d)", al.Name, discNumber) + // The subtitle is itself a DiscArtPriority candidate, so name it where the chain can be read against it. + if subtitle := strings.TrimSpace(al.Discs[discNumber]); subtitle != "" { + name += ": " + subtitle + } + return name, nil +} + // Refresh drops an item's resolved artwork state and re-queues it at Bump priority. func Refresh(ctx context.Context, ds model.DataStore, kind model.Kind, id string) error { if err := ds.Artwork(ctx).DeleteForItems(kind, []string{id}); err != nil { diff --git a/core/artwork/housekeeping_test.go b/core/artwork/housekeeping_test.go index 7aecd2760..c9809203a 100644 --- a/core/artwork/housekeeping_test.go +++ b/core/artwork/housekeeping_test.go @@ -332,3 +332,56 @@ var _ = Describe("Housekeeping", func() { }) }) }) + +var _ = Describe("ItemName", func() { + var ds *tests.MockDataStore + var ctx context.Context + + BeforeEach(func() { + ctx = context.Background() + albumRepo := tests.CreateMockAlbumRepo() + albumRepo.SetData(model.Albums{ + {ID: "al-1", Name: "Kid A"}, + {ID: "al-2", Name: "Sandinista!", Discs: model.Discs{2: "Side Three"}}, + }) + ds = &tests.MockDataStore{MockedAlbum: albumRepo} + Expect(ds.Artist(ctx).(*tests.MockArtistRepo).Put(&model.Artist{ID: "ar-1", Name: "Radiohead"})).To(Succeed()) + }) + + It("returns the album name", func() { + Expect(ItemName(ctx, ds, model.KindAlbumArtwork, "al-1")).To(Equal("Kid A")) + }) + + It("returns the artist name", func() { + Expect(ItemName(ctx, ds, model.KindArtistArtwork, "ar-1")).To(Equal("Radiohead")) + }) + + It("errors for an unknown album", func() { + _, err := ItemName(ctx, ds, model.KindAlbumArtwork, "nope") + Expect(err).To(MatchError(model.ErrNotFound)) + }) + + It("errors for an unsupported kind", func() { + // model.Kind is a struct with unexported fields, so the zero value is the only + // unsupported Kind constructible from outside package model. + _, err := ItemName(ctx, ds, model.Kind{}, "al-1") + Expect(err).To(HaveOccurred()) + }) + + Context("disc artwork", func() { + It("names the album, the disc and its subtitle", func() { + Expect(ItemName(ctx, ds, model.KindDiscArtwork, "al-2:2")). + To(Equal("Sandinista! (disc 2): Side Three")) + }) + + It("omits the subtitle when the disc has none", func() { + Expect(ItemName(ctx, ds, model.KindDiscArtwork, "al-2:1")). + To(Equal("Sandinista! (disc 1)")) + }) + + It("rejects an id that is not :", func() { + _, err := ItemName(ctx, ds, model.KindDiscArtwork, "al-2") + Expect(err).To(HaveOccurred()) + }) + }) +}) diff --git a/core/external/provider.go b/core/external/provider.go index 782c7c3aa..5c46dc644 100644 --- a/core/external/provider.go +++ b/core/external/provider.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "slices" "sort" "strings" "time" @@ -14,6 +15,7 @@ import ( "github.com/navidrome/navidrome/core/matcher" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/server/events" "github.com/navidrome/navidrome/utils" . "github.com/navidrome/navidrome/utils/gg" "github.com/navidrome/navidrome/utils/slice" @@ -33,12 +35,14 @@ type Provider interface { UpdateArtistInfo(ctx context.Context, id string, count int, includeNotPresent bool) (*model.Artist, error) SimilarSongs(ctx context.Context, id string, count int) (model.MediaFiles, error) TopSongs(ctx context.Context, artist, artistId string, count int) (model.MediaFiles, error) + RefreshInfo(ctx context.Context, kind model.Kind, id string) error } type provider struct { ds model.DataStore ag Agents matcher *matcher.Matcher + broker events.Broker artistQueue refreshQueue[auxArtist] albumQueue refreshQueue[auxAlbum] } @@ -83,13 +87,17 @@ type Agents interface { agents.SimilarSongsByArtistRetriever } -func NewProvider(ds model.DataStore, agents Agents, m *matcher.Matcher) Provider { - e := &provider{ds: ds, ag: agents, matcher: m} +func NewProvider(ds model.DataStore, agents Agents, m *matcher.Matcher, broker events.Broker) Provider { + e := &provider{ds: ds, ag: agents, matcher: m, broker: broker} e.artistQueue = newRefreshQueue(context.TODO(), e.populateArtistInfo) e.albumQueue = newRefreshQueue(context.TODO(), e.populateAlbumInfo) return e } +func (e *provider) broadcastRefresh(ctx context.Context, resource, id string) { + e.broker.SendBroadcastMessage(ctx, (&events.RefreshResource{}).With(resource, id)) +} + func (e *provider) getAlbum(ctx context.Context, id string) (auxAlbum, error) { var entity any entity, err := model.GetEntityByID(ctx, e.ds, id) @@ -179,6 +187,7 @@ func (e *provider) populateAlbumInfo(ctx context.Context, album auxAlbum) (auxAl "elapsed", time.Since(start), err) } else { log.Trace(ctx, "AlbumInfo collected", "album", album, "elapsed", time.Since(start)) + e.broadcastRefresh(ctx, "album", album.ID) } return album, nil @@ -272,10 +281,44 @@ func (e *provider) populateArtistInfo(ctx context.Context, artist auxArtist) (au "elapsed", time.Since(start), err) } else { log.Trace(ctx, "ArtistInfo collected", "artist", artist, "elapsed", time.Since(start)) + e.broadcastRefresh(ctx, "artist", artist.ID) } return artist, nil } +// infoKinds are the kinds RefreshInfo can act on. Callers check this instead of restating +// the set, so the switch below stays the only place that has to know how each kind loads. +var infoKinds = []model.Kind{model.KindArtistArtwork, model.KindAlbumArtwork} + +// HasInfo reports whether a kind has external info to refresh. +func HasInfo(kind model.Kind) bool { return slices.Contains(infoKinds, kind) } + +// RefreshInfo re-fetches external info for one item, ignoring the TTL. It is synchronous: +// callers that must not block are responsible for detaching it. +func (e *provider) RefreshInfo(ctx context.Context, kind model.Kind, id string) error { + ctx, cancel := context.WithTimeout(ctx, refreshTimeout) + defer cancel() + + switch kind { + case model.KindArtistArtwork: + artist, err := e.getArtist(ctx, id) + if err != nil { + return err + } + _, err = e.populateArtistInfo(ctx, artist) + return err + case model.KindAlbumArtwork: + album, err := e.getAlbum(ctx, id) + if err != nil { + return err + } + _, err = e.populateAlbumInfo(ctx, album) + return err + default: + return model.ErrNotFound + } +} + func (e *provider) TopSongs(ctx context.Context, artistName, id string, count int) (model.MediaFiles, error) { artist, err := e.findArtist(ctx, artistName, id) if err != nil { diff --git a/core/external/provider_refreshinfo_test.go b/core/external/provider_refreshinfo_test.go new file mode 100644 index 000000000..e7910a734 --- /dev/null +++ b/core/external/provider_refreshinfo_test.go @@ -0,0 +1,156 @@ +package external_test + +import ( + "context" + "slices" + "sync" + "time" + + "github.com/navidrome/navidrome/core/agents" + "github.com/navidrome/navidrome/core/external" + "github.com/navidrome/navidrome/core/matcher" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/server/events" + "github.com/navidrome/navidrome/tests" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "github.com/stretchr/testify/mock" +) + +type fakeBroker struct { + events.Broker + mu sync.Mutex + events []events.Event +} + +func (f *fakeBroker) SendBroadcastMessage(_ context.Context, e events.Event) { + f.mu.Lock() + defer f.mu.Unlock() + f.events = append(f.events, e) +} + +func (f *fakeBroker) sent() []events.Event { + f.mu.Lock() + defer f.mu.Unlock() + return slices.Clone(f.events) +} + +var _ = Describe("Provider - RefreshInfo", func() { + var ( + ctx context.Context + p external.Provider + ds *tests.MockDataStore + ag *mockAgents + broker *fakeBroker + mockArtistRepo *tests.MockArtistRepo + mockAlbumRepo *tests.MockAlbumRepo + ) + + expectArtistAgents := func() { + ag.On("GetArtistMBID", mock.Anything, mock.Anything, mock.Anything).Return("mbid-1", nil) + ag.On("GetArtistImages", mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return([]agents.ExternalImage{}, nil) + ag.On("GetArtistBiography", mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return("Fresh Bio", nil) + ag.On("GetArtistURL", mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return("http://artist.url", nil) + ag.On("GetSimilarArtists", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return([]agents.Artist{}, nil) + } + + expectAlbumAgents := func() { + ag.On("GetAlbumInfo", mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return(&agents.AlbumInfo{URL: "http://album.url", Description: "Fresh Notes"}, nil) + ag.On("GetAlbumImages", mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return([]agents.ExternalImage{}, nil) + } + + BeforeEach(func() { + ctx = GinkgoT().Context() + ds = new(tests.MockDataStore) + ag = new(mockAgents) + broker = &fakeBroker{} + p = external.NewProvider(ds, ag, matcher.New(ds), broker) + mockArtistRepo = ds.Artist(ctx).(*tests.MockArtistRepo) + mockAlbumRepo = ds.Album(ctx).(*tests.MockAlbumRepo) + }) + + It("repopulates an artist even when its info is fresh", func() { + fresh := time.Now() + mockArtistRepo.SetData(model.Artists{{ + ID: "ar-1", Name: "Test Artist", Biography: "stale", ExternalInfoUpdatedAt: &fresh, + }}) + expectArtistAgents() + + Expect(p.RefreshInfo(ctx, model.KindArtistArtwork, "ar-1")).To(Succeed()) + + saved, err := mockArtistRepo.Get("ar-1") + Expect(err).ToNot(HaveOccurred()) + Expect(saved.Biography).To(Equal("Fresh Bio")) + }) + + It("repopulates an album even when its info is fresh", func() { + fresh := time.Now() + mockAlbumRepo.SetData(model.Albums{{ + ID: "al-1", Name: "Test Album", AlbumArtist: "Test Artist", + Description: "stale", ExternalInfoUpdatedAt: &fresh, + }}) + expectAlbumAgents() + + Expect(p.RefreshInfo(ctx, model.KindAlbumArtwork, "al-1")).To(Succeed()) + + saved, err := mockAlbumRepo.Get("al-1") + Expect(err).ToNot(HaveOccurred()) + Expect(saved.Description).To(Equal("Fresh Notes")) + }) + + It("returns ErrNotFound for an unknown id", func() { + Expect(p.RefreshInfo(ctx, model.KindArtistArtwork, "nope")).To(MatchError(model.ErrNotFound)) + }) + + It("returns ErrNotFound for a kind with no external info", func() { + Expect(p.RefreshInfo(ctx, model.KindPlaylistArtwork, "pl-1")).To(MatchError(model.ErrNotFound)) + }) + + It("broadcasts a RefreshResource naming the artist", func() { + mockArtistRepo.SetData(model.Artists{{ID: "ar-1", Name: "Test Artist"}}) + expectArtistAgents() + + Expect(p.RefreshInfo(ctx, model.KindArtistArtwork, "ar-1")).To(Succeed()) + + sent := broker.sent() + Expect(sent).To(HaveLen(1)) + rr, ok := sent[0].(*events.RefreshResource) + Expect(ok).To(BeTrue()) + Expect(rr.Data(rr)).To(ContainSubstring("ar-1")) + Expect(rr.Data(rr)).To(ContainSubstring("artist")) + }) + + It("broadcasts a RefreshResource naming the album", func() { + mockAlbumRepo.SetData(model.Albums{{ID: "al-1", Name: "Test Album", AlbumArtist: "Test Artist"}}) + expectAlbumAgents() + + Expect(p.RefreshInfo(ctx, model.KindAlbumArtwork, "al-1")).To(Succeed()) + + sent := broker.sent() + Expect(sent).To(HaveLen(1)) + Expect(sent[0].Data(sent[0])).To(ContainSubstring("album")) + Expect(sent[0].Data(sent[0])).To(ContainSubstring("al-1")) + }) + + It("does not broadcast when the artist cannot be loaded", func() { + mockArtistRepo.SetData(model.Artists{{ID: "ar-1", Name: "Test Artist"}}) + expectArtistAgents() + mockArtistRepo.SetError(true) + + _ = p.RefreshInfo(ctx, model.KindArtistArtwork, "ar-1") + + Expect(broker.sent()).To(BeEmpty()) + }) + + It("reports which kinds have external info", func() { + Expect(external.HasInfo(model.KindArtistArtwork)).To(BeTrue()) + Expect(external.HasInfo(model.KindAlbumArtwork)).To(BeTrue()) + Expect(external.HasInfo(model.KindPlaylistArtwork)).To(BeFalse()) + }) +}) diff --git a/core/external/provider_similarsongs_test.go b/core/external/provider_similarsongs_test.go index ac54495f4..3944edb68 100644 --- a/core/external/provider_similarsongs_test.go +++ b/core/external/provider_similarsongs_test.go @@ -61,7 +61,7 @@ var _ = Describe("Provider - SimilarSongs", func() { similarAgent: mockSimilarAgent, } - provider = NewProvider(ds, agentsCombined, matcher.New(ds)) + provider = NewProvider(ds, agentsCombined, matcher.New(ds), &fakeBroker{}) }) // Resolves track-1 through the GetEntityByID probe order and on to its artist. Left permissive: diff --git a/core/external/provider_topsongs_test.go b/core/external/provider_topsongs_test.go index 795a7ff74..16e8ad840 100644 --- a/core/external/provider_topsongs_test.go +++ b/core/external/provider_topsongs_test.go @@ -45,7 +45,7 @@ var _ = Describe("Provider - TopSongs", func() { ag = new(mockAgents) - p = NewProvider(ds, ag, matcher.New(ds)) + p = NewProvider(ds, ag, matcher.New(ds), &fakeBroker{}) }) It("returns top songs for a known artist", func() { diff --git a/core/external/provider_updatealbuminfo_test.go b/core/external/provider_updatealbuminfo_test.go index 21824c93f..a54ef922c 100644 --- a/core/external/provider_updatealbuminfo_test.go +++ b/core/external/provider_updatealbuminfo_test.go @@ -34,7 +34,7 @@ var _ = Describe("Provider - UpdateAlbumInfo", func() { ctx = GinkgoT().Context() ds = new(tests.MockDataStore) ag = new(mockAgents) - p = external.NewProvider(ds, ag, matcher.New(ds)) + p = external.NewProvider(ds, ag, matcher.New(ds), &fakeBroker{}) mockAlbumRepo = ds.Album(ctx).(*tests.MockAlbumRepo) conf.Server.DevAlbumInfoTimeToLive = 1 * time.Hour }) diff --git a/core/external/provider_updateartistinfo_test.go b/core/external/provider_updateartistinfo_test.go index d783128fb..853d56bbc 100644 --- a/core/external/provider_updateartistinfo_test.go +++ b/core/external/provider_updateartistinfo_test.go @@ -37,7 +37,7 @@ var _ = Describe("Provider - UpdateArtistInfo", func() { ctx = GinkgoT().Context() ds = new(tests.MockDataStore) ag = new(mockAgents) - p = external.NewProvider(ds, ag, matcher.New(ds)) + p = external.NewProvider(ds, ag, matcher.New(ds), &fakeBroker{}) mockArtistRepo = ds.Artist(ctx).(*tests.MockArtistRepo) }) diff --git a/resources/i18n/pt-br.json b/resources/i18n/pt-br.json index 9a6338ae1..a4ad6bc8c 100644 --- a/resources/i18n/pt-br.json +++ b/resources/i18n/pt-br.json @@ -93,6 +93,7 @@ "addToPlaylist": "Adicionar à playlist", "download": "Baixar", "info": "Detalhes", + "refresh": "Atualizar Metadados", "share": "Compartilhar" }, "lists": { @@ -602,7 +603,8 @@ "coverUploaded": "Capa atualizada", "coverRemoved": "Capa removida", "coverUploadError": "Erro ao enviar capa", - "coverRemoveError": "Erro ao remover capa" + "coverRemoveError": "Erro ao remover capa", + "metadataRefreshStarted": "Atualizando metadados em segundo plano" }, "menu": { "library": "Biblioteca", diff --git a/server/nativeapi/artwork.go b/server/nativeapi/artwork.go deleted file mode 100644 index cfd943b1f..000000000 --- a/server/nativeapi/artwork.go +++ /dev/null @@ -1,34 +0,0 @@ -package nativeapi - -import ( - "net/http" - "slices" - - "github.com/go-chi/chi/v5" - "github.com/navidrome/navidrome/core/artwork" - "github.com/navidrome/navidrome/log" - "github.com/navidrome/navidrome/model" -) - -func (api *Router) addArtworkRoute(r chi.Router) { - r.Post("/artwork/{kind}/{id}/refresh", api.refreshArtwork()) -} - -// State is deliberately cleared so a wrong pick disappears immediately (placeholder until re-resolved). -func (api *Router) refreshArtwork() http.HandlerFunc { - return func(w http.ResponseWriter, r *http.Request) { - ctx := r.Context() - kind, _ := model.ParseKind(chi.URLParam(r, "kind")) - id := chi.URLParam(r, "id") - if !slices.Contains(artwork.RefreshableKinds, kind) { - http.Error(w, "invalid artwork kind", http.StatusBadRequest) - return - } - if err := artwork.Refresh(ctx, api.ds, kind, id); err != nil { - log.Error(ctx, "Error refreshing artwork", "kind", kind, "id", id, err) - http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError) - return - } - w.WriteHeader(http.StatusNoContent) - } -} diff --git a/server/nativeapi/artwork_test.go b/server/nativeapi/artwork_test.go deleted file mode 100644 index b26a94e6a..000000000 --- a/server/nativeapi/artwork_test.go +++ /dev/null @@ -1,95 +0,0 @@ -package nativeapi - -import ( - "context" - "net/http" - "net/http/httptest" - - "github.com/navidrome/navidrome/conf" - "github.com/navidrome/navidrome/conf/configtest" - "github.com/navidrome/navidrome/core/auth" - "github.com/navidrome/navidrome/model" - "github.com/navidrome/navidrome/server" - "github.com/navidrome/navidrome/tests" - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" -) - -var _ = Describe("Artwork API", func() { - var ds *tests.MockDataStore - var artRepo *tests.MockArtworkRepo - var queueRepo *tests.MockArtworkQueueRepo - var router http.Handler - var adminToken, userToken string - - BeforeEach(func() { - DeferCleanup(configtest.SetupConfig()) - conf.Server.EnableSharing = false - artRepo = tests.CreateMockArtworkRepo() - queueRepo = tests.CreateMockArtworkQueueRepo() - ds = &tests.MockDataStore{MockedArtwork: artRepo, MockedArtworkQueue: queueRepo} - auth.Init(ds) - nativeRouter := New(ds, nil, nil, nil, tests.NewMockLibraryService(), tests.NewMockUserService(), nil, nil, nil) - router = server.JWTVerifier(nativeRouter) - - adminUser := model.User{ID: "admin-1", UserName: "admin", IsAdmin: true, NewPassword: "adminpass"} - regularUser := model.User{ID: "user-1", UserName: "regular", IsAdmin: false, NewPassword: "userpass"} - Expect(ds.User(context.TODO()).Put(&adminUser)).To(Succeed()) - Expect(ds.User(context.TODO()).Put(®ularUser)).To(Succeed()) - - var err error - adminToken, err = auth.CreateToken(&adminUser) - Expect(err).ToNot(HaveOccurred()) - userToken, err = auth.CreateToken(®ularUser) - Expect(err).ToNot(HaveOccurred()) - }) - - Describe("POST /api/artwork/{kind}/{id}/refresh", func() { - It("clears state and enqueues a Bump for admins", func() { - Expect(artRepo.PutItemArtwork(&model.ItemArtwork{ - ItemKind: "al", ItemID: "al-1", Hash: "oldhash", Source: "external", - })).To(Succeed()) - - req := createAuthenticatedRequest("POST", "/artwork/al/al-1/refresh", nil, adminToken) - w := httptest.NewRecorder() - router.ServeHTTP(w, req) - - Expect(w.Code).To(Equal(http.StatusNoContent)) - - _, err := artRepo.GetItemArtwork(model.KindAlbumArtwork, "al-1", model.ImageTypePrimary) - Expect(err).To(MatchError(model.ErrNotFound)) - - queued, err := queueRepo.DequeueBatch(1000) - Expect(err).ToNot(HaveOccurred()) - Expect(queued).To(ContainElement(SatisfyAll( - HaveField("ItemKind", "al"), - HaveField("ItemID", "al-1"), - HaveField("Priority", model.ArtworkPriorityBump), - ))) - }) - - It("returns 400 for an invalid kind", func() { - req := createAuthenticatedRequest("POST", "/artwork/xx/id-1/refresh", nil, adminToken) - w := httptest.NewRecorder() - router.ServeHTTP(w, req) - - Expect(w.Code).To(Equal(http.StatusBadRequest)) - }) - - It("denies access to regular users", func() { - req := createAuthenticatedRequest("POST", "/artwork/al/al-1/refresh", nil, userToken) - w := httptest.NewRecorder() - router.ServeHTTP(w, req) - - Expect(w.Code).To(Equal(http.StatusForbidden)) - }) - - It("denies access without authentication", func() { - req := createUnauthenticatedRequest("POST", "/artwork/al/al-1/refresh", nil) - w := httptest.NewRecorder() - router.ServeHTTP(w, req) - - Expect(w.Code).To(Equal(http.StatusUnauthorized)) - }) - }) -}) diff --git a/server/nativeapi/config_test.go b/server/nativeapi/config_test.go index 107b01e01..d1007f457 100644 --- a/server/nativeapi/config_test.go +++ b/server/nativeapi/config_test.go @@ -29,7 +29,7 @@ var _ = Describe("Config API", func() { conf.Server.DevUIShowConfig = true // Enable config endpoint for tests ds = &tests.MockDataStore{} auth.Init(ds) - nativeRouter := New(ds, nil, nil, nil, tests.NewMockLibraryService(), tests.NewMockUserService(), nil, nil, nil) + nativeRouter := New(ds, nil, nil, nil, tests.NewMockLibraryService(), tests.NewMockUserService(), nil, nil, nil, nil) router = server.JWTVerifier(nativeRouter) // Create test users diff --git a/server/nativeapi/library_test.go b/server/nativeapi/library_test.go index 9b7061845..13b33c238 100644 --- a/server/nativeapi/library_test.go +++ b/server/nativeapi/library_test.go @@ -31,7 +31,7 @@ var _ = Describe("Library API", func() { conf.Server.EnableSharing = false ds = &tests.MockDataStore{} auth.Init(ds) - nativeRouter := New(ds, nil, nil, nil, tests.NewMockLibraryService(), tests.NewMockUserService(), nil, nil, nil) + nativeRouter := New(ds, nil, nil, nil, tests.NewMockLibraryService(), tests.NewMockUserService(), nil, nil, nil, nil) router = server.JWTVerifier(nativeRouter) // Create test users diff --git a/server/nativeapi/metadata.go b/server/nativeapi/metadata.go new file mode 100644 index 000000000..913ab0471 --- /dev/null +++ b/server/nativeapi/metadata.go @@ -0,0 +1,56 @@ +package nativeapi + +import ( + "context" + "errors" + "net/http" + "slices" + + "github.com/go-chi/chi/v5" + "github.com/navidrome/navidrome/core/artwork" + "github.com/navidrome/navidrome/core/external" + "github.com/navidrome/navidrome/log" + "github.com/navidrome/navidrome/model" +) + +func (api *Router) addMetadataRoute(r chi.Router) { + r.Post("/metadata/{kind}/{id}/refresh", api.refreshMetadata()) +} + +// refreshMetadata clears the artwork state deliberately, so a wrong pick cannot be served from +// cache again; reads fall back to local resolution while the worker re-runs the chain at Bump. +func (api *Router) refreshMetadata() http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + ctx := r.Context() + kind, _ := model.ParseKind(chi.URLParam(r, "kind")) + id := chi.URLParam(r, "id") + if !slices.Contains(artwork.RefreshableKinds, kind) { + http.Error(w, "invalid artwork kind", http.StatusBadRequest) + return + } + if _, err := artwork.ItemName(ctx, api.ds, kind, id); err != nil { + if errors.Is(err, model.ErrNotFound) { + http.Error(w, http.StatusText(http.StatusNotFound), http.StatusNotFound) + return + } + log.Error(ctx, "Error looking up item to refresh", "kind", kind, "id", id, err) + http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError) + return + } + if err := artwork.Refresh(ctx, api.ds, kind, id); err != nil { + log.Error(ctx, "Error refreshing artwork", "kind", kind, "id", id, err) + http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError) + return + } + if external.HasInfo(kind) { + // Detached: the request context is cancelled the moment this handler returns 204. + bg := context.WithoutCancel(ctx) + go func() { + if err := api.provider.RefreshInfo(bg, kind, id); err != nil { + log.Error(bg, "Error refreshing external info", "kind", kind, "id", id, err) + } + }() + } + w.WriteHeader(http.StatusNoContent) + } +} diff --git a/server/nativeapi/metadata_test.go b/server/nativeapi/metadata_test.go new file mode 100644 index 000000000..ebc9aeb28 --- /dev/null +++ b/server/nativeapi/metadata_test.go @@ -0,0 +1,177 @@ +package nativeapi + +import ( + "context" + "net/http" + "net/http/httptest" + "slices" + "sync" + + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/conf/configtest" + "github.com/navidrome/navidrome/core/auth" + "github.com/navidrome/navidrome/core/external" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/server" + "github.com/navidrome/navidrome/tests" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +type fakeProvider struct { + external.Provider + mu sync.Mutex + called []string +} + +func (f *fakeProvider) RefreshInfo(_ context.Context, kind model.Kind, id string) error { + f.mu.Lock() + defer f.mu.Unlock() + f.called = append(f.called, kind.Prefix()+"/"+id) + return nil +} + +func (f *fakeProvider) calls() []string { + f.mu.Lock() + defer f.mu.Unlock() + return slices.Clone(f.called) +} + +var _ = Describe("Metadata API", func() { + var ds *tests.MockDataStore + var artRepo *tests.MockArtworkRepo + var queueRepo *tests.MockArtworkQueueRepo + var albumRepo *tests.MockAlbumRepo + var provider *fakeProvider + var router http.Handler + var adminToken, userToken string + + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + conf.Server.EnableSharing = false + artRepo = tests.CreateMockArtworkRepo() + queueRepo = tests.CreateMockArtworkQueueRepo() + albumRepo = tests.CreateMockAlbumRepo() + artistRepo := tests.CreateMockArtistRepo() + playlistRepo := tests.CreateMockPlaylistRepo() + Expect(albumRepo.Put(&model.Album{ID: "al-1", Name: "Kid A"})).To(Succeed()) + Expect(artistRepo.Put(&model.Artist{ID: "ar-1", Name: "Radiohead"})).To(Succeed()) + Expect(playlistRepo.Put(&model.Playlist{ID: "pl-1", Name: "My Playlist"})).To(Succeed()) + ds = &tests.MockDataStore{ + MockedArtwork: artRepo, + MockedArtworkQueue: queueRepo, + MockedAlbum: albumRepo, + MockedArtist: artistRepo, + MockedPlaylist: playlistRepo, + } + auth.Init(ds) + provider = &fakeProvider{} + nativeRouter := New(ds, nil, nil, nil, tests.NewMockLibraryService(), tests.NewMockUserService(), nil, nil, nil, provider) + router = server.JWTVerifier(nativeRouter) + + adminUser := model.User{ID: "admin-1", UserName: "admin", IsAdmin: true, NewPassword: "adminpass"} + regularUser := model.User{ID: "user-1", UserName: "regular", IsAdmin: false, NewPassword: "userpass"} + Expect(ds.User(context.TODO()).Put(&adminUser)).To(Succeed()) + Expect(ds.User(context.TODO()).Put(®ularUser)).To(Succeed()) + + var err error + adminToken, err = auth.CreateToken(&adminUser) + Expect(err).ToNot(HaveOccurred()) + userToken, err = auth.CreateToken(®ularUser) + Expect(err).ToNot(HaveOccurred()) + }) + + Describe("POST /api/metadata/{kind}/{id}/refresh", func() { + It("clears state and enqueues a Bump for admins", func() { + Expect(artRepo.PutItemArtwork(&model.ItemArtwork{ + ItemKind: "al", ItemID: "al-1", Hash: "oldhash", Source: "external", + })).To(Succeed()) + + req := createAuthenticatedRequest("POST", "/metadata/al/al-1/refresh", nil, adminToken) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + Expect(w.Code).To(Equal(http.StatusNoContent)) + + _, err := artRepo.GetItemArtwork(model.KindAlbumArtwork, "al-1", model.ImageTypePrimary) + Expect(err).To(MatchError(model.ErrNotFound)) + + queued, err := queueRepo.DequeueBatch(1000) + Expect(err).ToNot(HaveOccurred()) + Expect(queued).To(ContainElement(SatisfyAll( + HaveField("ItemKind", "al"), + HaveField("ItemID", "al-1"), + HaveField("Priority", model.ArtworkPriorityBump), + ))) + }) + + It("returns 400 for an invalid kind", func() { + req := createAuthenticatedRequest("POST", "/metadata/xx/id-1/refresh", nil, adminToken) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + Expect(w.Code).To(Equal(http.StatusBadRequest)) + }) + + It("denies access to regular users", func() { + req := createAuthenticatedRequest("POST", "/metadata/al/al-1/refresh", nil, userToken) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + Expect(w.Code).To(Equal(http.StatusForbidden)) + }) + + It("denies access without authentication", func() { + req := createUnauthenticatedRequest("POST", "/metadata/al/al-1/refresh", nil) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + Expect(w.Code).To(Equal(http.StatusUnauthorized)) + }) + + It("triggers an external info refresh for albums", func() { + req := createAuthenticatedRequest("POST", "/metadata/al/al-1/refresh", nil, adminToken) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + Expect(w.Code).To(Equal(http.StatusNoContent)) + Eventually(provider.calls).Should(ContainElement("al/al-1")) + }) + + It("triggers an external info refresh for artists", func() { + req := createAuthenticatedRequest("POST", "/metadata/ar/ar-1/refresh", nil, adminToken) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + Expect(w.Code).To(Equal(http.StatusNoContent)) + Eventually(provider.calls).Should(ContainElement("ar/ar-1")) + }) + + It("skips the external info refresh for kinds without external info", func() { + req := createAuthenticatedRequest("POST", "/metadata/pl/pl-1/refresh", nil, adminToken) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + Expect(w.Code).To(Equal(http.StatusNoContent)) + Consistently(provider.calls).ShouldNot(ContainElement("pl/pl-1")) + }) + + It("returns 404 for an unknown id", func() { + req := createAuthenticatedRequest("POST", "/metadata/al/nope/refresh", nil, adminToken) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + Expect(w.Code).To(Equal(http.StatusNotFound)) + }) + + It("returns 500 when the lookup fails for a reason other than not-found", func() { + albumRepo.SetError(true) + + req := createAuthenticatedRequest("POST", "/metadata/al/al-1/refresh", nil, adminToken) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + Expect(w.Code).To(Equal(http.StatusInternalServerError)) + }) + }) +}) diff --git a/server/nativeapi/native_api.go b/server/nativeapi/native_api.go index f97bd26b2..57a712a20 100644 --- a/server/nativeapi/native_api.go +++ b/server/nativeapi/native_api.go @@ -14,6 +14,7 @@ import ( "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/core" "github.com/navidrome/navidrome/core/artwork" + "github.com/navidrome/navidrome/core/external" "github.com/navidrome/navidrome/core/metrics" playlistsvc "github.com/navidrome/navidrome/core/playlists" "github.com/navidrome/navidrome/log" @@ -46,10 +47,11 @@ type Router struct { maintenance core.Maintenance pluginManager PluginManager imgUpload artwork.Uploader + provider external.Provider } -func New(ds model.DataStore, share core.Share, playlists playlistsvc.Playlists, insights metrics.Insights, libraryService core.Library, userService core.User, maintenance core.Maintenance, pluginManager PluginManager, imgUpload artwork.Uploader) *Router { - r := &Router{ds: ds, share: share, playlists: playlists, insights: insights, libs: libraryService, users: userService, maintenance: maintenance, pluginManager: pluginManager, imgUpload: imgUpload} +func New(ds model.DataStore, share core.Share, playlists playlistsvc.Playlists, insights metrics.Insights, libraryService core.Library, userService core.User, maintenance core.Maintenance, pluginManager PluginManager, imgUpload artwork.Uploader, provider external.Provider) *Router { + r := &Router{ds: ds, share: share, playlists: playlists, insights: insights, libs: libraryService, users: userService, maintenance: maintenance, pluginManager: pluginManager, imgUpload: imgUpload, provider: provider} r.Handler = r.routes() return r } @@ -92,7 +94,7 @@ func (api *Router) routes() http.Handler { api.addConfigRoute(r) api.addUserLibraryRoute(r) api.addPluginRoute(r) - api.addArtworkRoute(r) + api.addMetadataRoute(r) api.RX(r, "/library", api.libs.NewRepository, true) }) }) diff --git a/server/nativeapi/native_api_song_test.go b/server/nativeapi/native_api_song_test.go index b1ed09d65..203fcd4cf 100644 --- a/server/nativeapi/native_api_song_test.go +++ b/server/nativeapi/native_api_song_test.go @@ -95,7 +95,7 @@ var _ = Describe("Song Endpoints", func() { mfRepo.SetData(testSongs) // Create the native API router and wrap it with the JWTVerifier middleware - nativeRouter := New(ds, nil, nil, nil, tests.NewMockLibraryService(), tests.NewMockUserService(), nil, nil, nil) + nativeRouter := New(ds, nil, nil, nil, tests.NewMockLibraryService(), tests.NewMockUserService(), nil, nil, nil, nil) router = server.JWTVerifier(nativeRouter) w = httptest.NewRecorder() }) diff --git a/server/nativeapi/playlists_test.go b/server/nativeapi/playlists_test.go index 74ef58cab..9abcc477f 100644 --- a/server/nativeapi/playlists_test.go +++ b/server/nativeapi/playlists_test.go @@ -99,7 +99,7 @@ var _ = Describe("Playlist Tracks Endpoint", func() { err := userRepo.Put(&testUser) Expect(err).ToNot(HaveOccurred()) - nativeRouter := New(ds, nil, plsSvc, nil, tests.NewMockLibraryService(), tests.NewMockUserService(), nil, nil, nil) + nativeRouter := New(ds, nil, plsSvc, nil, tests.NewMockLibraryService(), tests.NewMockUserService(), nil, nil, nil, nil) router = server.JWTVerifier(nativeRouter) w = httptest.NewRecorder() }) diff --git a/server/nativeapi/plugin_test.go b/server/nativeapi/plugin_test.go index aa91a7951..1683885e7 100644 --- a/server/nativeapi/plugin_test.go +++ b/server/nativeapi/plugin_test.go @@ -34,7 +34,7 @@ var _ = Describe("Plugin API", func() { ds = &tests.MockDataStore{} mockManager = &tests.MockPluginManager{} auth.Init(ds) - nativeRouter := New(ds, nil, nil, nil, tests.NewMockLibraryService(), tests.NewMockUserService(), nil, mockManager, nil) + nativeRouter := New(ds, nil, nil, nil, tests.NewMockLibraryService(), tests.NewMockUserService(), nil, mockManager, nil, nil) router = server.JWTVerifier(nativeRouter) // Create test users diff --git a/server/nativeapi/user_password_token_refresh_test.go b/server/nativeapi/user_password_token_refresh_test.go index 32f4b13cb..2a363980f 100644 --- a/server/nativeapi/user_password_token_refresh_test.go +++ b/server/nativeapi/user_password_token_refresh_test.go @@ -45,7 +45,7 @@ var _ = Describe("PUT /user/{id}: token refresh on self password change", func() auth.Init(ds) userService := core.NewUser(ds, noopPluginUnloader{}) - nativeRouter := New(ds, nil, nil, nil, tests.NewMockLibraryService(), userService, nil, nil, nil) + nativeRouter := New(ds, nil, nil, nil, tests.NewMockLibraryService(), userService, nil, nil, nil, nil) router = server.JWTVerifier(nativeRouter) }) diff --git a/server/subsonic/e2e/e2e_suite_test.go b/server/subsonic/e2e/e2e_suite_test.go index 8998731a4..58e877b0d 100644 --- a/server/subsonic/e2e/e2e_suite_test.go +++ b/server/subsonic/e2e/e2e_suite_test.go @@ -357,6 +357,10 @@ func (n noopProvider) TopSongs(context.Context, string, string, int) (model.Medi return nil, nil } +func (n noopProvider) RefreshInfo(context.Context, model.Kind, string) error { + return nil +} + // Compile-time interface checks var ( _ artwork.Artwork = noopArtwork{} diff --git a/tests/mock_album_repo.go b/tests/mock_album_repo.go index ff6f9cff6..c63f7c425 100644 --- a/tests/mock_album_repo.go +++ b/tests/mock_album_repo.go @@ -65,6 +65,9 @@ func (m *MockAlbumRepo) Put(al *model.Album) error { if al.ID == "" { al.ID = id.NewRandom() } + if m.Data == nil { + m.Data = make(map[string]*model.Album) + } m.Data[al.ID] = al return nil } @@ -142,10 +145,7 @@ func (m *MockAlbumRepo) GetTouchedAlbums(libID int) (model.AlbumCursor, error) { } func (m *MockAlbumRepo) UpdateExternalInfo(album *model.Album) error { - if m.Err { - return errors.New("unexpected error") - } - return nil + return m.Put(album) } func (m *MockAlbumRepo) Search(q string, options ...model.QueryOptions) (model.Albums, error) { diff --git a/tests/mock_artist_repo.go b/tests/mock_artist_repo.go index 9691a6584..b71942208 100644 --- a/tests/mock_artist_repo.go +++ b/tests/mock_artist_repo.go @@ -58,6 +58,9 @@ func (m *MockArtistRepo) Put(ar *model.Artist, columsToUpdate ...string) error { if ar.ID == "" { ar.ID = id.NewRandom() } + if m.Data == nil { + m.Data = make(map[string]*model.Artist) + } m.Data[ar.ID] = ar return nil } @@ -137,10 +140,7 @@ func (m *MockArtistRepo) GetCursor(options ...model.QueryOptions) (model.ArtistC } func (m *MockArtistRepo) UpdateExternalInfo(artist *model.Artist) error { - if m.Err { - return errors.New("mock repo error") - } - return nil + return m.Put(artist) } func (m *MockArtistRepo) RefreshStats(allArtists bool) (int64, error) { diff --git a/ui/src/artist/ArtistShow.jsx b/ui/src/artist/ArtistShow.jsx index eef2989d5..515c1ab08 100644 --- a/ui/src/artist/ArtistShow.jsx +++ b/ui/src/artist/ArtistShow.jsx @@ -53,7 +53,7 @@ const useStyles = makeStyles( }, ) -const ArtistDetails = (props) => { +export const ArtistDetails = (props) => { const record = useRecordContext(props) const isDesktop = useMediaQuery((theme) => theme.breakpoints.up('sm'), { noSsr: true, @@ -75,7 +75,9 @@ const ArtistDetails = (props) => { // eslint-disable-next-line no-console console.error('error on artist page', e) }) - }, [record.id]) + // Keyed on the record, not its id: a refreshed record must re-fetch, or the stale + // artistInfo state keeps winning the `||` above. + }, [record]) const Component = isDesktop ? DesktopArtistDetails : MobileArtistDetails return ( diff --git a/ui/src/artist/ArtistShow.test.jsx b/ui/src/artist/ArtistShow.test.jsx new file mode 100644 index 000000000..f51cde5ed --- /dev/null +++ b/ui/src/artist/ArtistShow.test.jsx @@ -0,0 +1,63 @@ +import React from 'react' +import { render, waitFor } from '@testing-library/react' +import { RecordContextProvider } from 'react-admin' +import { ThemeProvider, createTheme } from '@material-ui/core/styles' +import { describe, it, expect, vi, beforeEach } from 'vitest' +import { ArtistDetails } from './ArtistShow' +import subsonic from '../subsonic' + +vi.mock('../subsonic', () => ({ + default: { getArtistInfo: vi.fn(), getCoverArtUrl: vi.fn() }, +})) + +// Not under test here: isolate ArtistDetails from the leaf presentational views. +vi.mock('./DesktopArtistDetails', () => ({ default: () => null })) +vi.mock('./MobileArtistDetails', () => ({ default: () => null })) + +const mockGetArtistInfo = subsonic.getArtistInfo + +describe('ArtistDetails', () => { + beforeEach(() => { + vi.clearAllMocks() + mockGetArtistInfo.mockResolvedValue({ + json: { + 'subsonic-response': { + status: 'ok', + artistInfo: { biography: 'fetched' }, + }, + }, + }) + }) + + const theme = createTheme() + + const wrap = (record) => ( + + + + + + ) + + const renderDetails = (record) => render(wrap(record)) + + it('re-fetches the artist info when the record object changes', async () => { + const record = { id: 'ar1', name: 'Artist', biography: 'old' } + const { rerender } = renderDetails(record) + await waitFor(() => expect(mockGetArtistInfo).toHaveBeenCalledTimes(1)) + + rerender(wrap({ ...record, biography: 'new' })) + + await waitFor(() => expect(mockGetArtistInfo).toHaveBeenCalledTimes(2)) + }) + + it('does not re-fetch when the same record object is passed again', async () => { + const record = { id: 'ar1', name: 'Artist', biography: 'old' } + const { rerender } = renderDetails(record) + await waitFor(() => expect(mockGetArtistInfo).toHaveBeenCalledTimes(1)) + + rerender(wrap(record)) + + expect(mockGetArtistInfo).toHaveBeenCalledTimes(1) + }) +}) diff --git a/ui/src/common/ContextMenus.jsx b/ui/src/common/ContextMenus.jsx index 7ad8c735c..606506459 100644 --- a/ui/src/common/ContextMenus.jsx +++ b/ui/src/common/ContextMenus.jsx @@ -7,7 +7,12 @@ import MenuItem from '@material-ui/core/MenuItem' import MoreVertIcon from '@material-ui/icons/MoreVert' import { MdQuestionMark } from 'react-icons/md' import { makeStyles } from '@material-ui/core/styles' -import { useDataProvider, useNotify, useTranslate } from 'react-admin' +import { + useDataProvider, + useNotify, + usePermissions, + useTranslate, +} from 'react-admin' import clsx from 'clsx' import { playNext, @@ -69,6 +74,7 @@ const ContextMenu = ({ const dispatch = useDispatch() const translate = useTranslate() const notify = useNotify() + const { permissions } = usePermissions() const [anchorEl, setAnchorEl] = useState(null) const isArtist = resource === 'artist' @@ -129,6 +135,16 @@ const ContextMenu = ({ ) }, }, + refresh: { + enabled: permissions === 'admin', + needData: false, + label: translate('resources.album.actions.refresh'), + action: (record) => + dataProvider + .refreshMetadata(resource, record.id) + .then(() => notify('message.metadataRefreshStarted')) + .catch(() => notify('ra.page.error', 'warning')), + }, ...(!hideInfo && { info: { enabled: true, diff --git a/ui/src/common/ContextMenus.test.jsx b/ui/src/common/ContextMenus.test.jsx index 72a98b64b..59a7de966 100644 --- a/ui/src/common/ContextMenus.test.jsx +++ b/ui/src/common/ContextMenus.test.jsx @@ -17,12 +17,21 @@ const { mockConfig } = vi.hoisted(() => ({ })) vi.mock('../config', () => ({ default: mockConfig })) +const { mockPermissions, mockRefreshMetadata } = vi.hoisted(() => ({ + mockPermissions: { value: 'admin' }, + mockRefreshMetadata: vi.fn(), +})) + vi.mock('react-admin', async (importOriginal) => { const actual = await importOriginal() return { ...actual, useNotify: () => vi.fn(), - useDataProvider: () => ({ getList: vi.fn() }), + usePermissions: () => ({ permissions: mockPermissions.value }), + useDataProvider: () => ({ + getList: vi.fn(), + refreshMetadata: mockRefreshMetadata, + }), useTranslate: () => (x) => x, } }) @@ -43,6 +52,7 @@ describe('ContextMenus', () => { vi.clearAllMocks() mockConfig.enableSharing = true mockConfig.enableDownloads = true + mockPermissions.value = 'admin' }) describe('ArtistContextMenu', () => { @@ -75,4 +85,49 @@ describe('ContextMenus', () => { expect(screen.getByText('ra.action.download (1 MB)')).toBeInTheDocument() }) }) + + describe('refresh metadata', () => { + it('shows the item for admins on the album menu', () => { + renderMenu(AlbumContextMenu, { id: 'al1', name: 'Album', songCount: 1 }) + expect( + screen.getByText('resources.album.actions.refresh'), + ).toBeInTheDocument() + }) + + // Menu order comes from key insertion order in the options object, so it is easy to + // change by accident when adding an entry. + it('places the item directly above Get Info', () => { + renderMenu(AlbumContextMenu, { id: 'al1', name: 'Album', songCount: 1 }) + const labels = screen + .getAllByRole('menuitem') + .map((item) => item.textContent) + const refreshAt = labels.indexOf('resources.album.actions.refresh') + const infoAt = labels.indexOf('resources.album.actions.info') + + expect(refreshAt).toBeGreaterThanOrEqual(0) + expect(infoAt).toEqual(refreshAt + 1) + }) + + it('shows the item for admins on the artist menu', () => { + renderMenu(ArtistContextMenu, { id: 'ar1', name: 'Artist', stats: {} }) + expect( + screen.getByText('resources.album.actions.refresh'), + ).toBeInTheDocument() + }) + + it('hides the item for regular users', () => { + mockPermissions.value = 'regular' + renderMenu(AlbumContextMenu, { id: 'al1', name: 'Album', songCount: 1 }) + expect( + screen.queryByText('resources.album.actions.refresh'), + ).not.toBeInTheDocument() + }) + + it('calls refreshMetadata with the resource and id', () => { + mockRefreshMetadata.mockResolvedValue({}) + renderMenu(AlbumContextMenu, { id: 'al1', name: 'Album', songCount: 1 }) + fireEvent.click(screen.getByText('resources.album.actions.refresh')) + expect(mockRefreshMetadata).toHaveBeenCalledWith('album', 'al1') + }) + }) }) diff --git a/ui/src/dataProvider/wrapperDataProvider.js b/ui/src/dataProvider/wrapperDataProvider.js index f5004308b..e79beb787 100644 --- a/ui/src/dataProvider/wrapperDataProvider.js +++ b/ui/src/dataProvider/wrapperDataProvider.js @@ -4,6 +4,8 @@ import { REST_URL } from '../consts' const dataProvider = jsonServerProvider(REST_URL, httpClient) +const REFRESH_KIND = { album: 'al', artist: 'ar' } + const isAdmin = () => { const role = localStorage.getItem('role') return role === 'admin' @@ -221,6 +223,12 @@ const wrapperDataProvider = { data: json, })) }, + // The endpoint answers 204 with no body, but react-admin rejects any response without a + // `data` key, so the id stands in for one. + refreshMetadata: (resource, id) => + httpClient(`${REST_URL}/metadata/${REFRESH_KIND[resource]}/${id}/refresh`, { + method: 'POST', + }).then(() => ({ data: { id } })), } export default wrapperDataProvider diff --git a/ui/src/dataProvider/wrapperDataProvider.test.js b/ui/src/dataProvider/wrapperDataProvider.test.js index fbc82f969..4225a5a54 100644 --- a/ui/src/dataProvider/wrapperDataProvider.test.js +++ b/ui/src/dataProvider/wrapperDataProvider.test.js @@ -87,4 +87,37 @@ describe('wrapperDataProvider', () => { ) }) }) + + describe('refreshMetadata', () => { + it('posts to the album metadata refresh endpoint', () => { + mockHttpClient.mockResolvedValue({ json: {} }) + wrapperDataProvider.refreshMetadata('album', 'al-1') + expect(mockHttpClient).toHaveBeenCalledWith( + expect.stringContaining('/metadata/al/al-1/refresh'), + { method: 'POST' }, + ) + }) + + it('posts to the artist metadata refresh endpoint', () => { + mockHttpClient.mockResolvedValue({ json: {} }) + wrapperDataProvider.refreshMetadata('artist', 'ar-1') + expect(mockHttpClient).toHaveBeenCalledWith( + expect.stringContaining('/metadata/ar/ar-1/refresh'), + { method: 'POST' }, + ) + }) + + // react-admin rejects a custom method whose response has no `data` key, and the + // endpoint answers 204 with no body. + it('resolves to a react-admin shaped response', async () => { + mockHttpClient.mockResolvedValue({ + status: 204, + body: '', + json: undefined, + }) + await expect( + wrapperDataProvider.refreshMetadata('album', 'al-1'), + ).resolves.toEqual({ data: { id: 'al-1' } }) + }) + }) }) diff --git a/ui/src/i18n/en.json b/ui/src/i18n/en.json index 12968afbd..8823a6749 100644 --- a/ui/src/i18n/en.json +++ b/ui/src/i18n/en.json @@ -93,7 +93,8 @@ "shuffle": "Shuffle", "addToPlaylist": "Add to Playlist", "download": "Download", - "info": "Get Info" + "info": "Get Info", + "refresh": "Refresh Metadata" }, "lists": { "all": "All", @@ -569,6 +570,7 @@ "coverRemoved": "Cover art removed", "coverUploadError": "Error uploading cover art", "coverRemoveError": "Error removing cover art", + "metadataRefreshStarted": "Refreshing metadata in the background", "note": "NOTE", "transcodingDisabled": "Changing the transcoding configuration through the web interface is disabled for security reasons. If you would like to change (edit or add) transcoding options, restart the server with the %{config} configuration option.", "transcodingEnabled": "Navidrome is currently running with %{config}, making it possible to run system commands from the transcoding settings using the web interface. We recommend to disable it for security reasons and only enable it when configuring Transcoding options.",