mirror of
https://github.com/navidrome/navidrome.git
synced 2026-08-31 07:30:32 +00:00
fix(agents): never serve provider placeholder images as artist art
This commit is contained in:
parent
93f6afb684
commit
b66d9d9a89
@ -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 {
|
||||
|
||||
@ -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
|
||||
|
||||
@ -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
|
||||
}
|
||||
|
||||
@ -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) {
|
||||
|
||||
59
core/agents/artist_image_placeholder_test.go
Normal file
59
core/agents/artist_image_placeholder_test.go
Normal file
@ -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" }
|
||||
@ -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)
|
||||
}
|
||||
|
||||
15
core/external/extdata_helper_test.go
vendored
15
core/external/extdata_helper_test.go
vendored
@ -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 {
|
||||
|
||||
10
core/external/provider.go
vendored
10
core/external/provider.go
vendored
@ -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 {
|
||||
|
||||
28
core/external/provider_artistimage_test.go
vendored
28
core/external/provider_artistimage_test.go
vendored
@ -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
|
||||
|
||||
19
core/external/provider_updateartistinfo_test.go
vendored
19
core/external/provider_updateartistinfo_test.go
vendored
@ -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",
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user