diff --git a/adapters/deezer/deezer.go b/adapters/deezer/deezer.go index d8e832cf1..02185bfb5 100644 --- a/adapters/deezer/deezer.go +++ b/adapters/deezer/deezer.go @@ -70,16 +70,28 @@ func (s *deezerAgent) GetArtistImages(ctx context.Context, _, name, _ string) ([ {artist.PictureSmall, deezerApiPictureSmallSize}, } for _, imgData := range possibleImages { - if imgData.URL != "" { + if imgData.URL != "" && !s.IsArtistImagePlaceholder(imgData.URL) { res = append(res, agents.ExternalImage{ URL: imgData.URL, Size: imgData.Size, }) } } + if len(res) == 0 { + return nil, agents.ErrNotFound + } return res, nil } +// deezerEmptyPicturePath is Deezer's empty-image-id path shape for artists with no picture +// (e.g. .../images/artist//1000x1000-...jpg), which serves a generic silhouette on any CDN host. +const deezerEmptyPicturePath = "/images/artist//" + +// IsArtistImagePlaceholder recognizes Deezer's empty-image-id silhouette URLs. +func (s *deezerAgent) IsArtistImagePlaceholder(url string) bool { + return strings.Contains(url, deezerEmptyPicturePath) +} + func (s *deezerAgent) searchArtist(ctx context.Context, name string) (*Artist, error) { artists, err := s.client.searchArtists(ctx, name, deezerArtistSearchLimit) if errors.Is(err, ErrNotFound) || len(artists) == 0 { diff --git a/adapters/deezer/deezer_test.go b/adapters/deezer/deezer_test.go index f478af115..360db1f13 100644 --- a/adapters/deezer/deezer_test.go +++ b/adapters/deezer/deezer_test.go @@ -94,6 +94,54 @@ var _ = Describe("deezerAgent", func() { }) }) + Describe("GetArtistImages", func() { + var agent *deezerAgent + var httpClient *fakeHttpClient + + BeforeEach(func() { + httpClient = &fakeHttpClient{} + agent = &deezerAgent{ + dataStore: &tests.MockDataStore{}, + client: newClient(httpClient), + } + }) + + It("returns the real images when the artist has a picture", func() { + httpClient.mock("https://api.deezer.com/search/artist", http.Response{ + StatusCode: 200, + Body: io.NopCloser(bytes.NewBufferString(`{"data":[ + {"id":412,"name":"Queen","nb_fan":12744378, + "picture_xl":"https://cdn-images.dzcdn.net/images/artist/abc/1000x1000-000000-80-0-0.jpg", + "picture_big":"https://cdn-images.dzcdn.net/images/artist/abc/500x500-000000-80-0-0.jpg"} + ],"total":1}`)), + }) + + images, err := agent.GetArtistImages(ctx, "", "Queen", "") + + Expect(err).ToNot(HaveOccurred()) + Expect(images).To(HaveLen(2)) + Expect(images[0].URL).To(ContainSubstring("1000x1000")) + }) + + It("returns ErrNotFound when the artist only has empty-id placeholder pictures", func() { + httpClient.mock("https://api.deezer.com/search/artist", http.Response{ + StatusCode: 200, + Body: io.NopCloser(bytes.NewBufferString(`{"data":[ + {"id":412,"name":"Queen","nb_fan":12744378, + "picture_xl":"https://cdn-images.dzcdn.net/images/artist//1000x1000-000000-80-0-0.jpg", + "picture_big":"https://cdn-images.dzcdn.net/images/artist//500x500-000000-80-0-0.jpg", + "picture_medium":"https://cdn-images.dzcdn.net/images/artist//250x250-000000-80-0-0.jpg", + "picture_small":"https://cdn-images.dzcdn.net/images/artist//56x56-000000-80-0-0.jpg"} + ],"total":1}`)), + }) + + images, err := agent.GetArtistImages(ctx, "", "Queen", "") + + Expect(err).To(MatchError(agents.ErrNotFound)) + Expect(images).To(BeEmpty()) + }) + }) + Describe("GetArtistBiography - Language Fallback", func() { var agent *deezerAgent var httpClient *langAwareHttpClient diff --git a/adapters/lastfm/agent.go b/adapters/lastfm/agent.go index eb8f3d36e..9336601df 100644 --- a/adapters/lastfm/agent.go +++ b/adapters/lastfm/agent.go @@ -244,6 +244,11 @@ var ( artistIgnoredImage = "2a96cbd8b46e442fc41c2b86b821562f" // Last.fm artist placeholder image name ) +// IsArtistImagePlaceholder recognizes Last.fm's generic gray-star artist image. +func (l *lastfmAgent) IsArtistImagePlaceholder(url string) bool { + return strings.Contains(url, artistIgnoredImage) +} + func (l *lastfmAgent) GetArtistImages(ctx context.Context, _, name, mbid string) ([]agents.ExternalImage, error) { log.Debug(ctx, "Getting artist images from Last.fm", "name", name) a, err := l.callArtistGetInfo(ctx, name, l.languages[0]) @@ -274,7 +279,7 @@ func (l *lastfmAgent) GetArtistImages(ctx context.Context, _, name, mbid string) if attr.Key != "content" { continue } - if strings.Contains(attr.Val, artistIgnoredImage) { + if l.IsArtistImagePlaceholder(attr.Val) { log.Debug(ctx, "Artist image is ignored default image", "name", name, "url", attr.Val) return res, nil } diff --git a/core/agents/agents.go b/core/agents/agents.go index ead6dacd0..ccf4d70f0 100644 --- a/core/agents/agents.go +++ b/core/agents/agents.go @@ -230,6 +230,21 @@ func (a *Agents) GetArtistImages(ctx context.Context, id, name, mbid string) ([] }) } +// IsArtistImagePlaceholder reports whether any enabled agent recognizes url as its own provider's +// default/placeholder artist image. +func (a *Agents) IsArtistImagePlaceholder(url string) bool { + if url == "" { + return false + } + for _, ea := range a.getEnabledAgentNames() { + ag := a.getAgent(ea) + if detector, ok := ag.(ArtistImagePlaceholderDetector); ok && detector.IsArtistImagePlaceholder(url) { + return true + } + } + return false +} + // GetArtistTopSongs returns top songs by id, name, and/or mbid. Because some songs returned from an enabled // agent may not exist in the database, return at most limit * conf.Server.DevExternalArtistFetchMultiplier items. func (a *Agents) GetArtistTopSongs(ctx context.Context, id, artistName, mbid string, count int) ([]Song, error) { diff --git a/core/agents/artist_image_placeholder_test.go b/core/agents/artist_image_placeholder_test.go new file mode 100644 index 000000000..32922c6b0 --- /dev/null +++ b/core/agents/artist_image_placeholder_test.go @@ -0,0 +1,59 @@ +package agents + +import ( + "strings" + + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/conf/configtest" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/tests" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("Agents.IsArtistImagePlaceholder", func() { + var ds model.DataStore + + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + ds = &tests.MockDataStore{} + Register("phDetector", func(model.DataStore) Interface { return &placeholderDetectorAgent{marker: "PLACEHOLDER"} }) + Register("phPlain", func(model.DataStore) Interface { return &plainAgent{} }) + }) + + It("returns true when any enabled agent's detector recognizes the URL", func() { + conf.Server.Agents = "phDetector" + ag := createAgents(ds, nil) + + Expect(ag.IsArtistImagePlaceholder("http://cdn/PLACEHOLDER.jpg")).To(BeTrue()) + Expect(ag.IsArtistImagePlaceholder("http://cdn/real.jpg")).To(BeFalse()) + }) + + It("returns false when no enabled agent implements the detector", func() { + conf.Server.Agents = "phPlain" + ag := createAgents(ds, nil) + + Expect(ag.IsArtistImagePlaceholder("http://cdn/PLACEHOLDER.jpg")).To(BeFalse()) + }) + + It("returns false for an empty URL", func() { + conf.Server.Agents = "phDetector" + ag := createAgents(ds, nil) + + Expect(ag.IsArtistImagePlaceholder("")).To(BeFalse()) + }) +}) + +type placeholderDetectorAgent struct { + Interface + marker string +} + +func (a *placeholderDetectorAgent) AgentName() string { return "phDetector" } +func (a *placeholderDetectorAgent) IsArtistImagePlaceholder(url string) bool { + return strings.Contains(url, a.marker) +} + +type plainAgent struct{ Interface } + +func (a *plainAgent) AgentName() string { return "phPlain" } diff --git a/core/agents/interfaces.go b/core/agents/interfaces.go index d5f4a6580..43ca10de0 100644 --- a/core/agents/interfaces.go +++ b/core/agents/interfaces.go @@ -86,6 +86,12 @@ type ArtistImageRetriever interface { GetArtistImages(ctx context.Context, id, name, mbid string) ([]ExternalImage, error) } +// ArtistImagePlaceholderDetector is implemented by agents that can recognize their own provider's +// default/placeholder artist images, so those are never served or persisted as real artwork. +type ArtistImagePlaceholderDetector interface { + IsArtistImagePlaceholder(url string) bool +} + type ArtistTopSongsRetriever interface { GetArtistTopSongs(ctx context.Context, id, artistName, mbid string, count int) ([]Song, error) } diff --git a/core/external/extdata_helper_test.go b/core/external/extdata_helper_test.go index 8fabf4490..86b582b0e 100644 --- a/core/external/extdata_helper_test.go +++ b/core/external/extdata_helper_test.go @@ -3,12 +3,16 @@ package external_test import ( "context" "errors" + "strings" "github.com/navidrome/navidrome/core/agents" "github.com/navidrome/navidrome/model" "github.com/stretchr/testify/mock" ) +// placeholderImageURL is a stand-in provider placeholder the mock agents recognize in tests. +const placeholderImageURL = "http://provider.example.com/placeholder.png" + // --- Shared Mock Implementations --- // mockArtistRepo mocks model.ArtistRepository @@ -288,6 +292,17 @@ func (m *mockAgents) GetAlbumImages(ctx context.Context, name, artist, mbid stri return nil, args.Error(1) } +func (m *mockAgents) IsArtistImagePlaceholder(url string) bool { + if url == "" { + return false + } + // Delegate to a configured image agent that owns its detection; otherwise recognize the stand-in. + if d, ok := m.imageAgent.(agents.ArtistImagePlaceholderDetector); ok { + return d.IsArtistImagePlaceholder(url) + } + return strings.Contains(url, "placeholder") +} + func (m *mockAgents) GetSimilarSongsByTrack(ctx context.Context, id, name, artist, mbid string, count int) ([]agents.Song, error) { args := m.Called(ctx, id, name, artist, mbid, count) if args.Get(0) != nil { diff --git a/core/external/provider.go b/core/external/provider.go index 459e8a205..34e41c2bc 100644 --- a/core/external/provider.go +++ b/core/external/provider.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "net/url" + "slices" "sort" "strings" "time" @@ -85,6 +86,7 @@ type Agents interface { agents.SimilarSongsByTrackRetriever agents.SimilarSongsByAlbumRetriever agents.SimilarSongsByArtistRetriever + agents.ArtistImagePlaceholderDetector } func NewProvider(ds model.DataStore, agents Agents, m *matcher.Matcher) Provider { @@ -377,6 +379,10 @@ func (e *provider) ArtistImage(ctx context.Context, id string) (*url.URL, error) } imageUrl := artist.ArtistImageUrl() + if e.ag.IsArtistImagePlaceholder(imageUrl) { + // Provider placeholder cached before filtering existed; treat as absent and refetch. + imageUrl = "" + } if imageUrl == "" { // No cached URL — must fetch from external source synchronously e.callGetImage(ctx, e.ag, &artist) @@ -524,6 +530,10 @@ func (e *provider) callGetImage(ctx context.Context, agent agents.ArtistImageRet if err != nil { return } + // Never persist provider placeholders as real artwork. + images = slices.DeleteFunc(images, func(i agents.ExternalImage) bool { + return e.ag.IsArtistImagePlaceholder(i.URL) + }) sort.Slice(images, func(i, j int) bool { return images[i].Size > images[j].Size }) if len(images) >= 1 { diff --git a/core/external/provider_artistimage_test.go b/core/external/provider_artistimage_test.go index 79612d651..4f9feacb8 100644 --- a/core/external/provider_artistimage_test.go +++ b/core/external/provider_artistimage_test.go @@ -5,6 +5,7 @@ import ( "context" "errors" "net/url" + "strings" "time" "github.com/navidrome/navidrome/conf" @@ -300,6 +301,29 @@ var _ = Describe("Provider - ArtistImage", func() { }) + It("treats a cached provider placeholder URL as absent and fetches fresh", func() { + // Legacy row still holding Last.fm's star placeholder cached before agents filtered it out + starURL := "https://lastfm.freetls.fastly.net/i/u/ar0/2a96cbd8b46e442fc41c2b86b821562f.jpg" + cachedArtist := &model.Artist{ + ID: "artist-placeholder", + Name: "Placeholder Artist", + LargeImageUrl: starURL, + ExternalInfoUpdatedAt: new(time.Now().Add(-1 * time.Minute)), + } + mockArtistRepo.On("Get", "artist-placeholder").Return(cachedArtist, nil).Maybe() + mockImageAgent.On("GetArtistImages", mock.Anything, "artist-placeholder", "Placeholder Artist", ""). + Return([]agents.ExternalImage{{URL: "http://example.com/real.jpg", Size: 1000}}, nil).Once() + expectedURL, _ := url.Parse("http://example.com/real.jpg") + + // Act + imgURL, err := provider.ArtistImage(ctx, "artist-placeholder") + + // Assert: placeholder ignored, agent consulted, real image returned + Expect(err).ToNot(HaveOccurred()) + Expect(imgURL).To(Equal(expectedURL)) + mockImageAgent.AssertCalled(GinkgoT(), "GetArtistImages", ctx, "artist-placeholder", "Placeholder Artist", "") + }) + It("returns stale URL and enqueues refresh when info is expired", func() { // Arrange conf.Server.DevArtistInfoTimeToLive = 1 * time.Nanosecond @@ -412,6 +436,10 @@ func (m *mockArtistImageAgent) AgentName() string { return args.String(0) } +func (m *mockArtistImageAgent) IsArtistImagePlaceholder(url string) bool { + return strings.Contains(url, "2a96cbd8b46e442fc41c2b86b821562f") +} + func (m *mockArtistImageAgent) GetArtistImages(ctx context.Context, id, artistName, mbid string) ([]agents.ExternalImage, error) { args := m.Called(ctx, id, artistName, mbid) // Need careful type assertion for potentially nil slice diff --git a/core/external/provider_updateartistinfo_test.go b/core/external/provider_updateartistinfo_test.go index d783128fb..50ece7d69 100644 --- a/core/external/provider_updateartistinfo_test.go +++ b/core/external/provider_updateartistinfo_test.go @@ -250,6 +250,25 @@ var _ = Describe("Provider - UpdateArtistInfo", func() { ag.AssertExpectations(GinkgoT()) }) + It("does not persist provider placeholder image URLs returned by the agents", func() { + originalArtist := &model.Artist{ID: "ar-ph-image", Name: "Placeholder Image Artist"} + mockArtistRepo.SetData(model.Artists{*originalArtist}) + + ag.On("GetArtistMBID", ctx, "ar-ph-image", "Placeholder Image Artist").Return("", nil).Once() + ag.On("GetArtistImages", ctx, "ar-ph-image", "Placeholder Image Artist", ""). + Return([]agents.ExternalImage{{URL: placeholderImageURL, Size: 1000}}, nil).Once() + ag.On("GetArtistBiography", ctx, "ar-ph-image", "Placeholder Image Artist", "").Return("", nil).Maybe() + ag.On("GetArtistURL", ctx, "ar-ph-image", "Placeholder Image Artist", "").Return("", nil).Maybe() + ag.On("GetSimilarArtists", ctx, "ar-ph-image", "Placeholder Image Artist", "", 100).Return(nil, nil).Maybe() + + updatedArtist, err := p.UpdateArtistInfo(ctx, "ar-ph-image", 10, false) + + Expect(err).NotTo(HaveOccurred()) + Expect(updatedArtist.LargeImageUrl).To(BeEmpty()) + Expect(updatedArtist.MediumImageUrl).To(BeEmpty()) + Expect(updatedArtist.SmallImageUrl).To(BeEmpty()) + }) + It("matches similar artists by ID first when agent provides IDs", func() { originalArtist := &model.Artist{ ID: "ar-id-match",