AlbumImage tests

Signed-off-by: Deluan <deluan@navidrome.org>

core/extdata/extdata_helper_test.go

@@ -189,6 +189,7 @@ type mockCombinedAgents struct {
 	topSongsAgent    agents.ArtistTopSongsRetriever
 	similarAgent     agents.ArtistSimilarRetriever
 	imageAgent       agents.ArtistImageRetriever
+	albumInfoAgent   agents.AlbumInfoRetriever
 	agents.Interface // Embed to satisfy non-overridden methods
 }
 
@@ -213,6 +214,9 @@ func (m *mockCombinedAgents) GetArtistTopSongs(ctx context.Context, id, artistNa
 // --- Stubs for other Agents interface methods ---
 
 func (m *mockCombinedAgents) GetAlbumInfo(ctx context.Context, name, artist, mbid string) (*agents.AlbumInfo, error) {
+	if m.albumInfoAgent != nil {
+		return m.albumInfoAgent.GetAlbumInfo(ctx, name, artist, mbid)
+	}
 	if m.topSongsAgent != nil {
 		if ar, ok := m.topSongsAgent.(agents.AlbumInfoRetriever); ok {
 			return ar.GetAlbumInfo(ctx, name, artist, mbid)

core/extdata/provider.go

@@ -91,6 +91,7 @@ func (e *provider) getAlbum(ctx context.Context, id string) (auxAlbum, error) {
 	default:
 		return auxAlbum{}, model.ErrNotFound
 	}
+
 	return album, nil
 }
 
@@ -340,12 +341,22 @@ func (e *provider) AlbumImage(ctx context.Context, id string) (*url.URL, error)
 	}
 
 	info, err := e.ag.GetAlbumInfo(ctx, album.Name, album.AlbumArtist, album.MbzAlbumID)
-	if errors.Is(err, agents.ErrNotFound) {
+	if err != nil {
+		switch {
+		case errors.Is(err, agents.ErrNotFound):
+			log.Trace(ctx, "Album not found in agent", "albumID", id, "name", album.Name, "artist", album.AlbumArtist)
+		case errors.Is(err, context.Canceled):
+			log.Debug(ctx, "AlbumImage call canceled", err)
+		default:
+			log.Warn(ctx, "Error getting album info from agent", "albumID", id, "name", album.Name, "artist", album.AlbumArtist, err)
+		}
+
 		return nil, err
 	}
-	if utils.IsCtxDone(ctx) {
+
-		log.Warn(ctx, "AlbumImage call canceled", ctx.Err())
+	if info == nil {
-		return nil, ctx.Err()
+		log.Warn(ctx, "Agent returned nil info without error", "albumID", id, "name", album.Name, "artist", album.AlbumArtist)
+		return nil, agents.ErrNotFound
 	}
 
 	// Return the biggest image
@@ -369,7 +380,6 @@ func (e *provider) TopSongs(ctx context.Context, artistName string, count int) (
 	}
 
 	songs, err := e.getMatchingTopSongs(ctx, e.ag, artist, count)
-	// Return nil for ErrNotFound or any other errors
 	if err != nil {
 		log.Error(ctx, "Error getting top songs from agent", "artist", artistName, err)
 		return nil, nil

core/extdata/provider_albumimage_test.go

@@ -0,0 +1,328 @@
+package extdata
+
+import (
+	"context"
+	"errors"
+	"net/url"
+
+	"github.com/navidrome/navidrome/conf"
+	"github.com/navidrome/navidrome/core/agents"
+	"github.com/navidrome/navidrome/model"
+	"github.com/navidrome/navidrome/tests"
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+	"github.com/stretchr/testify/mock"
+)
+
+var _ = Describe("Provider - AlbumImage", func() {
+	var ds *tests.MockDataStore
+	var provider Provider
+	var mockArtistRepo *mockArtistRepo
+	var mockAlbumRepo *mockAlbumRepo
+	var mockMediaFileRepo *mockMediaFileRepo
+	var mockAlbumAgent *mockAlbumInfoAgent
+	var agentsCombined *mockCombinedAgents
+	var ctx context.Context
+	var cancel context.CancelFunc
+	var originalAgentsConfig string
+
+	BeforeEach(func() {
+		ctx, cancel = context.WithCancel(context.Background())
+		originalAgentsConfig = conf.Server.Agents
+		conf.Server.Agents = "mockAlbum" // Configure mock agent
+
+		mockArtistRepo = newMockArtistRepo()
+		mockAlbumRepo = newMockAlbumRepo()
+		mockMediaFileRepo = newMockMediaFileRepo()
+
+		ds = &tests.MockDataStore{
+			MockedArtist:    mockArtistRepo,
+			MockedAlbum:     mockAlbumRepo,
+			MockedMediaFile: mockMediaFileRepo,
+		}
+
+		mockAlbumAgent = newMockAlbumInfoAgent()
+
+		agentsCombined = &mockCombinedAgents{
+			albumInfoAgent: mockAlbumAgent,
+		}
+
+		provider = NewProvider(ds, agentsCombined)
+
+		// Default mocks
+		// Removed: mockAlbumRepo.On("Get", "album-1").Return(&model.Album{ID: "album-1", Name: "Album One", AlbumArtistID: "artist-1"}, nil).Maybe()
+		// Removed: mockMediaFileRepo.On("Get", "mf-1").Return(&model.MediaFile{ID: "mf-1", Title: "Track One", ArtistID: "artist-1", AlbumID: "album-1"}, nil).Maybe()
+
+		// Mocks for GetEntityByID sequence (initial failed lookups)
+		mockArtistRepo.On("Get", "album-1").Return(nil, model.ErrNotFound).Once()
+		mockArtistRepo.On("Get", "mf-1").Return(nil, model.ErrNotFound).Once()
+		mockAlbumRepo.On("Get", "mf-1").Return(nil, model.ErrNotFound).Once()
+
+		// Default mock for non-existent entities - Use Maybe() for flexibility
+		mockArtistRepo.On("Get", "not-found").Return(nil, model.ErrNotFound).Maybe()
+		mockAlbumRepo.On("Get", "not-found").Return(nil, model.ErrNotFound).Maybe()
+		mockMediaFileRepo.On("Get", "not-found").Return(nil, model.ErrNotFound).Maybe()
+
+		// Default agent response removed - tests will define their own expectations
+		// mockAlbumAgent.On("GetAlbumInfo", mock.Anything, "Album One", "", "").
+		// 	Return(&agents.AlbumInfo{
+		// 		Images: []agents.ExternalImage{
+		// 			{URL: "http://example.com/large.jpg", Size: 1000},
+		// 			{URL: "http://example.com/medium.jpg", Size: 500},
+		// 			{URL: "http://example.com/small.jpg", Size: 200},
+		// 		},
+		// 	}, nil).Maybe()
+	})
+
+	AfterEach(func() {
+		cancel()                                  // Restore context cancellation
+		conf.Server.Agents = originalAgentsConfig // Restore original agent config
+		// Removed mock assertions - rely on Once() in tests and outcome validation
+		// mockArtistRepo.AssertExpectations(GinkgoT())
+		// mockAlbumRepo.AssertExpectations(GinkgoT())
+		// mockMediaFileRepo.AssertExpectations(GinkgoT())
+		// mockAgent.AssertExpectations(GinkgoT())
+	})
+
+	Describe("AlbumImage", func() {
+		It("returns the largest image URL when successful", func() {
+			// Arrange
+			mockArtistRepo.On("Get", "album-1").Return(nil, model.ErrNotFound).Once() // Expect GetEntityByID sequence
+			mockAlbumRepo.On("Get", "album-1").Return(&model.Album{ID: "album-1", Name: "Album One", AlbumArtistID: "artist-1"}, nil).Once()
+			// Explicitly mock agent call for this test
+			mockAlbumAgent.On("GetAlbumInfo", ctx, "Album One", "", "").
+				Return(&agents.AlbumInfo{
+					Images: []agents.ExternalImage{
+						{URL: "http://example.com/large.jpg", Size: 1000},
+						{URL: "http://example.com/medium.jpg", Size: 500},
+						{URL: "http://example.com/small.jpg", Size: 200},
+					},
+				}, nil).Once()
+
+			expectedURL, _ := url.Parse("http://example.com/large.jpg")
+			imgURL, err := provider.AlbumImage(ctx, "album-1")
+
+			Expect(err).ToNot(HaveOccurred())
+			Expect(imgURL).To(Equal(expectedURL))
+			mockArtistRepo.AssertCalled(GinkgoT(), "Get", "album-1") // From GetEntityByID
+			mockAlbumRepo.AssertCalled(GinkgoT(), "Get", "album-1")
+			mockArtistRepo.AssertNotCalled(GinkgoT(), "Get", "artist-1")                     // Artist lookup no longer happens in getAlbum
+			mockAlbumAgent.AssertCalled(GinkgoT(), "GetAlbumInfo", ctx, "Album One", "", "") // Expect empty artist name
+		})
+
+		It("returns ErrNotFound if the album is not found in the DB", func() {
+			// Arrange: Explicitly expect the full GetEntityByID sequence for "not-found"
+			mockArtistRepo.On("Get", "not-found").Return(nil, model.ErrNotFound).Once()
+			mockAlbumRepo.On("Get", "not-found").Return(nil, model.ErrNotFound).Once()
+			mockMediaFileRepo.On("Get", "not-found").Return(nil, model.ErrNotFound).Once()
+
+			imgURL, err := provider.AlbumImage(ctx, "not-found")
+
+			Expect(err).To(MatchError(model.ErrNotFound))
+			Expect(imgURL).To(BeNil())
+			mockArtistRepo.AssertCalled(GinkgoT(), "Get", "not-found")
+			mockAlbumRepo.AssertCalled(GinkgoT(), "Get", "not-found")
+			mockMediaFileRepo.AssertCalled(GinkgoT(), "Get", "not-found")
+			mockAlbumAgent.AssertNotCalled(GinkgoT(), "GetAlbumInfo", mock.Anything, mock.Anything, mock.Anything, mock.Anything)
+		})
+
+		It("returns the agent error if the agent fails", func() {
+			// Arrange
+			mockArtistRepo.On("Get", "album-1").Return(nil, model.ErrNotFound).Once() // Expect GetEntityByID sequence
+			mockAlbumRepo.On("Get", "album-1").Return(&model.Album{ID: "album-1", Name: "Album One", AlbumArtistID: "artist-1"}, nil).Once()
+
+			agentErr := errors.New("agent failure")
+			// Explicitly mock agent call for this test
+			mockAlbumAgent.On("GetAlbumInfo", ctx, "Album One", "", "").Return(nil, agentErr).Once() // Expect empty artist
+
+			imgURL, err := provider.AlbumImage(ctx, "album-1")
+
+			Expect(err).To(MatchError(agentErr))
+			Expect(imgURL).To(BeNil())
+			mockArtistRepo.AssertCalled(GinkgoT(), "Get", "album-1")
+			mockAlbumRepo.AssertCalled(GinkgoT(), "Get", "album-1")
+			mockArtistRepo.AssertNotCalled(GinkgoT(), "Get", "artist-1")
+			mockAlbumAgent.AssertCalled(GinkgoT(), "GetAlbumInfo", ctx, "Album One", "", "") // Expect empty artist
+		})
+
+		It("returns ErrNotFound if the agent returns ErrNotFound", func() {
+			// Arrange
+			mockArtistRepo.On("Get", "album-1").Return(nil, model.ErrNotFound).Once() // Expect GetEntityByID sequence
+			mockAlbumRepo.On("Get", "album-1").Return(&model.Album{ID: "album-1", Name: "Album One", AlbumArtistID: "artist-1"}, nil).Once()
+
+			// Explicitly mock agent call for this test
+			mockAlbumAgent.On("GetAlbumInfo", ctx, "Album One", "", "").Return(nil, agents.ErrNotFound).Once() // Expect empty artist
+
+			imgURL, err := provider.AlbumImage(ctx, "album-1")
+
+			Expect(err).To(MatchError(agents.ErrNotFound))
+			Expect(imgURL).To(BeNil())
+			mockArtistRepo.AssertCalled(GinkgoT(), "Get", "album-1")
+			mockAlbumRepo.AssertCalled(GinkgoT(), "Get", "album-1")
+			mockAlbumAgent.AssertCalled(GinkgoT(), "GetAlbumInfo", ctx, "Album One", "", "") // Expect empty artist
+		})
+
+		It("returns ErrNotFound if the agent returns no images", func() {
+			// Arrange
+			mockArtistRepo.On("Get", "album-1").Return(nil, model.ErrNotFound).Once() // Expect GetEntityByID sequence
+			mockAlbumRepo.On("Get", "album-1").Return(&model.Album{ID: "album-1", Name: "Album One", AlbumArtistID: "artist-1"}, nil).Once()
+
+			// Explicitly mock agent call for this test
+			mockAlbumAgent.On("GetAlbumInfo", ctx, "Album One", "", "").
+				Return(&agents.AlbumInfo{Images: []agents.ExternalImage{}}, nil).Once() // Expect empty artist
+
+			imgURL, err := provider.AlbumImage(ctx, "album-1")
+
+			Expect(err).To(MatchError(agents.ErrNotFound))
+			Expect(imgURL).To(BeNil())
+			mockArtistRepo.AssertCalled(GinkgoT(), "Get", "album-1")
+			mockAlbumRepo.AssertCalled(GinkgoT(), "Get", "album-1")
+			mockAlbumAgent.AssertCalled(GinkgoT(), "GetAlbumInfo", ctx, "Album One", "", "") // Expect empty artist
+		})
+
+		It("returns context error if context is canceled", func() {
+			// Arrange
+			cctx, cancelCtx := context.WithCancel(context.Background())
+			// Mock the necessary DB calls *before* canceling the context
+			mockArtistRepo.On("Get", "album-1").Return(nil, model.ErrNotFound).Once()
+			mockAlbumRepo.On("Get", "album-1").Return(&model.Album{ID: "album-1", Name: "Album One", AlbumArtistID: "artist-1"}, nil).Once()
+			// Expect the agent call even if context is cancelled, returning the context error
+			mockAlbumAgent.On("GetAlbumInfo", cctx, "Album One", "", "").Return(nil, context.Canceled).Once()
+			// Cancel the context *before* calling the function under test
+			cancelCtx()
+
+			imgURL, err := provider.AlbumImage(cctx, "album-1")
+
+			Expect(err).To(MatchError(context.Canceled))
+			Expect(imgURL).To(BeNil())
+			mockArtistRepo.AssertCalled(GinkgoT(), "Get", "album-1")
+			mockAlbumRepo.AssertCalled(GinkgoT(), "Get", "album-1")
+			// Agent should now be called, verify this expectation
+			mockAlbumAgent.AssertCalled(GinkgoT(), "GetAlbumInfo", cctx, "Album One", "", "")
+		})
+
+		It("derives album ID from MediaFile ID", func() {
+			// Arrange: Mock full GetEntityByID for "mf-1" and recursive "album-1"
+			mockArtistRepo.On("Get", "mf-1").Return(nil, model.ErrNotFound).Once()
+			mockAlbumRepo.On("Get", "mf-1").Return(nil, model.ErrNotFound).Once()
+			mockMediaFileRepo.On("Get", "mf-1").Return(&model.MediaFile{ID: "mf-1", Title: "Track One", ArtistID: "artist-1", AlbumID: "album-1"}, nil).Once()
+			mockArtistRepo.On("Get", "album-1").Return(nil, model.ErrNotFound).Once()
+			mockAlbumRepo.On("Get", "album-1").Return(&model.Album{ID: "album-1", Name: "Album One", AlbumArtistID: "artist-1"}, nil).Once()
+
+			// Explicitly mock agent call for this test
+			mockAlbumAgent.On("GetAlbumInfo", ctx, "Album One", "", "").
+				Return(&agents.AlbumInfo{
+					Images: []agents.ExternalImage{
+						{URL: "http://example.com/large.jpg", Size: 1000},
+						{URL: "http://example.com/medium.jpg", Size: 500},
+						{URL: "http://example.com/small.jpg", Size: 200},
+					},
+				}, nil).Once()
+
+			expectedURL, _ := url.Parse("http://example.com/large.jpg")
+			imgURL, err := provider.AlbumImage(ctx, "mf-1")
+
+			Expect(err).ToNot(HaveOccurred())
+			Expect(imgURL).To(Equal(expectedURL))
+			mockArtistRepo.AssertCalled(GinkgoT(), "Get", "mf-1")
+			mockAlbumRepo.AssertCalled(GinkgoT(), "Get", "mf-1")
+			mockMediaFileRepo.AssertCalled(GinkgoT(), "Get", "mf-1")
+			mockArtistRepo.AssertCalled(GinkgoT(), "Get", "album-1")
+			mockAlbumRepo.AssertCalled(GinkgoT(), "Get", "album-1")
+			mockArtistRepo.AssertNotCalled(GinkgoT(), "Get", "artist-1")
+			mockAlbumAgent.AssertCalled(GinkgoT(), "GetAlbumInfo", ctx, "Album One", "", "")
+		})
+
+		It("handles different image orders from agent", func() {
+			// Arrange
+			mockArtistRepo.On("Get", "album-1").Return(nil, model.ErrNotFound).Once() // Expect GetEntityByID sequence
+			mockAlbumRepo.On("Get", "album-1").Return(&model.Album{ID: "album-1", Name: "Album One", AlbumArtistID: "artist-1"}, nil).Once()
+			// Explicitly mock agent call for this test
+			mockAlbumAgent.On("GetAlbumInfo", ctx, "Album One", "", "").
+				Return(&agents.AlbumInfo{
+					Images: []agents.ExternalImage{
+						{URL: "http://example.com/small.jpg", Size: 200},
+						{URL: "http://example.com/large.jpg", Size: 1000},
+						{URL: "http://example.com/medium.jpg", Size: 500},
+					},
+				}, nil).Once()
+
+			expectedURL, _ := url.Parse("http://example.com/large.jpg")
+			imgURL, err := provider.AlbumImage(ctx, "album-1")
+
+			Expect(err).ToNot(HaveOccurred())
+			Expect(imgURL).To(Equal(expectedURL)) // Should still pick the largest
+			mockAlbumAgent.AssertCalled(GinkgoT(), "GetAlbumInfo", ctx, "Album One", "", "")
+		})
+
+		It("handles agent returning only one image", func() {
+			// Arrange
+			mockArtistRepo.On("Get", "album-1").Return(nil, model.ErrNotFound).Once() // Expect GetEntityByID sequence
+			mockAlbumRepo.On("Get", "album-1").Return(&model.Album{ID: "album-1", Name: "Album One", AlbumArtistID: "artist-1"}, nil).Once()
+			// Explicitly mock agent call for this test
+			mockAlbumAgent.On("GetAlbumInfo", ctx, "Album One", "", "").
+				Return(&agents.AlbumInfo{
+					Images: []agents.ExternalImage{
+						{URL: "http://example.com/single.jpg", Size: 700},
+					},
+				}, nil).Once()
+
+			expectedURL, _ := url.Parse("http://example.com/single.jpg")
+			imgURL, err := provider.AlbumImage(ctx, "album-1")
+
+			Expect(err).ToNot(HaveOccurred())
+			Expect(imgURL).To(Equal(expectedURL))
+			mockAlbumAgent.AssertCalled(GinkgoT(), "GetAlbumInfo", ctx, "Album One", "", "")
+		})
+
+		It("returns ErrNotFound if deriving album ID fails", func() {
+			// Arrange: Mock full GetEntityByID for "mf-no-album" and recursive "not-found"
+			mockArtistRepo.On("Get", "mf-no-album").Return(nil, model.ErrNotFound).Once()
+			mockAlbumRepo.On("Get", "mf-no-album").Return(nil, model.ErrNotFound).Once()
+			mockMediaFileRepo.On("Get", "mf-no-album").Return(&model.MediaFile{ID: "mf-no-album", Title: "Track No Album", ArtistID: "artist-1", AlbumID: "not-found"}, nil).Once()
+			mockArtistRepo.On("Get", "not-found").Return(nil, model.ErrNotFound).Once()
+			mockAlbumRepo.On("Get", "not-found").Return(nil, model.ErrNotFound).Once()
+			mockMediaFileRepo.On("Get", "not-found").Return(nil, model.ErrNotFound).Once()
+
+			imgURL, err := provider.AlbumImage(ctx, "mf-no-album")
+
+			Expect(err).To(MatchError(model.ErrNotFound))
+			Expect(imgURL).To(BeNil())
+			mockArtistRepo.AssertCalled(GinkgoT(), "Get", "mf-no-album")
+			mockAlbumRepo.AssertCalled(GinkgoT(), "Get", "mf-no-album")
+			mockMediaFileRepo.AssertCalled(GinkgoT(), "Get", "mf-no-album")
+			mockArtistRepo.AssertCalled(GinkgoT(), "Get", "not-found")
+			mockAlbumRepo.AssertCalled(GinkgoT(), "Get", "not-found")
+			mockMediaFileRepo.AssertCalled(GinkgoT(), "Get", "not-found")
+			mockAlbumAgent.AssertNotCalled(GinkgoT(), "GetAlbumInfo", mock.Anything, mock.Anything, mock.Anything, mock.Anything)
+		})
+	})
+})
+
+// mockAlbumInfoAgent implementation
+type mockAlbumInfoAgent struct {
+	mock.Mock
+	agents.AlbumInfoRetriever // Embed interface
+}
+
+func newMockAlbumInfoAgent() *mockAlbumInfoAgent {
+	m := new(mockAlbumInfoAgent)
+	m.On("AgentName").Return("mockAlbum").Maybe()
+	return m
+}
+
+func (m *mockAlbumInfoAgent) AgentName() string {
+	args := m.Called()
+	return args.String(0)
+}
+
+func (m *mockAlbumInfoAgent) GetAlbumInfo(ctx context.Context, name, artist, mbid string) (*agents.AlbumInfo, error) {
+	args := m.Called(ctx, name, artist, mbid)
+	if args.Get(0) == nil {
+		return nil, args.Error(1)
+	}
+	return args.Get(0).(*agents.AlbumInfo), args.Error(1)
+}
+
+// Ensure mockAgent implements the interface
+var _ agents.AlbumInfoRetriever = (*mockAlbumInfoAgent)(nil)