diff --git a/core/external_metadata_simple_test.go b/core/external_metadata_simple_test.go deleted file mode 100644 index 2bb1d8683..000000000 --- a/core/external_metadata_simple_test.go +++ /dev/null @@ -1,474 +0,0 @@ -package core - -import ( - "context" - "errors" - "log" - "reflect" - "strings" - "unsafe" - - "github.com/Masterminds/squirrel" - "github.com/navidrome/navidrome/conf" - "github.com/navidrome/navidrome/core/agents" - "github.com/navidrome/navidrome/model" - "github.com/navidrome/navidrome/tests" - "github.com/navidrome/navidrome/utils/str" - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" -) - -// Custom implementation of ArtistRepository for testing -type customArtistRepo struct { - model.ArtistRepository - data map[string]*model.Artist - err bool -} - -func newCustomArtistRepo() *customArtistRepo { - return &customArtistRepo{ - data: make(map[string]*model.Artist), - } -} - -func (m *customArtistRepo) SetError(err bool) { - m.err = err -} - -func (m *customArtistRepo) SetData(artists model.Artists) { - m.data = make(map[string]*model.Artist) - for i, a := range artists { - m.data[a.ID] = &artists[i] - } -} - -// Key implementation needed for the test -func (m *customArtistRepo) GetAll(options ...model.QueryOptions) (model.Artists, error) { - if m.err { - return nil, errors.New("error") - } - - // No filters means return all - if len(options) == 0 || options[0].Filters == nil { - result := make(model.Artists, 0, len(m.data)) - for _, a := range m.data { - result = append(result, *a) - } - return result, nil - } - - // Handle filter by name (for findArtistByName) - if len(options) > 0 && options[0].Filters != nil { - switch filter := options[0].Filters.(type) { - case squirrel.Like: - if nameFilter, ok := filter["artist.name"]; ok { - name := strings.Trim(nameFilter.(string), "%") - log.Printf("ArtistRepo.GetAll: Looking for artist by name: %s", name) - - for _, a := range m.data { - log.Printf("ArtistRepo.GetAll: Comparing with artist: %s", a.Name) - if a.Name == name { - log.Printf("ArtistRepo.GetAll: Found artist: %s (ID: %s)", a.Name, a.ID) - return model.Artists{*a}, nil - } - } - } - } - } - - log.Println("ArtistRepo.GetAll: No matching artist found") - return model.Artists{}, nil -} - -// Custom implementation of MediaFileRepository for testing -type customMediaFileRepo struct { - model.MediaFileRepository - data map[string]*model.MediaFile - err bool -} - -func newCustomMediaFileRepo() *customMediaFileRepo { - return &customMediaFileRepo{ - data: make(map[string]*model.MediaFile), - } -} - -func (m *customMediaFileRepo) SetError(err bool) { - m.err = err -} - -func (m *customMediaFileRepo) SetData(mediaFiles model.MediaFiles) { - m.data = make(map[string]*model.MediaFile) - for i, mf := range mediaFiles { - m.data[mf.ID] = &mediaFiles[i] - } -} - -// Key implementation needed for the test -func (m *customMediaFileRepo) GetAll(options ...model.QueryOptions) (model.MediaFiles, error) { - if m.err { - return nil, errors.New("error") - } - - // No filters means return all - if len(options) == 0 || options[0].Filters == nil { - result := make(model.MediaFiles, 0, len(m.data)) - for _, mf := range m.data { - result = append(result, *mf) - } - return result, nil - } - - // Check if we're searching by MBID - if len(options) > 0 && options[0].Filters != nil { - // Log all filter types - log.Printf("MediaFileRepo.GetAll: Filter type: %T", options[0].Filters) - - switch filter := options[0].Filters.(type) { - case squirrel.And: - log.Printf("MediaFileRepo.GetAll: Processing AND filter with %d conditions", len(filter)) - - // First check if there's a mbz_recording_id in one of the AND conditions - for i, cond := range filter { - log.Printf("MediaFileRepo.GetAll: AND condition %d is of type %T", i, cond) - - if eq, ok := cond.(squirrel.Eq); ok { - log.Printf("MediaFileRepo.GetAll: Eq condition: %+v", eq) - - if mbid, hasMbid := eq["mbz_recording_id"]; hasMbid { - log.Printf("MediaFileRepo.GetAll: Looking for MBID: %s", mbid) - - for _, mf := range m.data { - if mf.MbzReleaseTrackID == mbid.(string) && !mf.Missing { - log.Printf("MediaFileRepo.GetAll: Found match by MBID: %s (Title: %s)", mf.ID, mf.Title) - return model.MediaFiles{*mf}, nil - } - } - } - } - } - - // Otherwise, find by artist ID and title - var artistMatches model.MediaFiles - var titleMatches model.MediaFiles - var notMissingMatches model.MediaFiles - - // Get non-missing files - for _, mf := range m.data { - if !mf.Missing { - notMissingMatches = append(notMissingMatches, *mf) - } - } - - log.Printf("MediaFileRepo.GetAll: Found %d non-missing files", len(notMissingMatches)) - - for i, cond := range filter { - log.Printf("MediaFileRepo.GetAll: Processing condition %d of type %T", i, cond) - - switch subFilter := cond.(type) { - case squirrel.Or: - log.Printf("MediaFileRepo.GetAll: Processing OR condition with %d subconditions", len(subFilter)) - - // Check for artist_id - for j, orCond := range subFilter { - log.Printf("MediaFileRepo.GetAll: OR subcondition %d is of type %T", j, orCond) - - if eq, ok := orCond.(squirrel.Eq); ok { - log.Printf("MediaFileRepo.GetAll: Eq condition: %+v", eq) - - if artistID, ok := eq["artist_id"]; ok { - log.Printf("MediaFileRepo.GetAll: Looking for artist_id: %s", artistID) - - for _, mf := range notMissingMatches { - if mf.ArtistID == artistID.(string) { - log.Printf("MediaFileRepo.GetAll: Found match by artist_id: %s (Title: %s)", mf.ID, mf.Title) - artistMatches = append(artistMatches, mf) - } - } - } - } - } - case squirrel.Like: - log.Printf("MediaFileRepo.GetAll: Processing LIKE condition: %+v", subFilter) - - // Check for title match - if orderTitle, ok := subFilter["order_title"]; ok { - normalizedTitle := str.SanitizeFieldForSorting(orderTitle.(string)) - log.Printf("MediaFileRepo.GetAll: Looking for normalized title: %s", normalizedTitle) - - for _, mf := range notMissingMatches { - normalizedMfTitle := str.SanitizeFieldForSorting(mf.Title) - log.Printf("MediaFileRepo.GetAll: Comparing with title: %s (normalized: %s)", mf.Title, normalizedMfTitle) - - if normalizedTitle == normalizedMfTitle { - log.Printf("MediaFileRepo.GetAll: Found title match: %s", mf.ID) - titleMatches = append(titleMatches, mf) - } - } - } - } - } - - log.Printf("MediaFileRepo.GetAll: Found %d artist matches and %d title matches", len(artistMatches), len(titleMatches)) - - // Find records that match both artist and title - var results model.MediaFiles - for _, am := range artistMatches { - for _, tm := range titleMatches { - if am.ID == tm.ID { - log.Printf("MediaFileRepo.GetAll: Found complete match: %s", am.ID) - results = append(results, am) - } - } - } - - if len(results) > 0 { - // Apply Max if specified - if options[0].Max > 0 && len(results) > options[0].Max { - results = results[:options[0].Max] - } - log.Printf("MediaFileRepo.GetAll: Returning %d results", len(results)) - return results, nil - } - case squirrel.Eq: - log.Printf("MediaFileRepo.GetAll: Processing Eq filter: %+v", filter) - - // Handle direct MBID lookup - if mbid, ok := filter["mbz_recording_id"]; ok { - log.Printf("MediaFileRepo.GetAll: Looking for MBID: %s", mbid) - - for _, mf := range m.data { - if mf.MbzReleaseTrackID == mbid.(string) && !mf.Missing { - log.Printf("MediaFileRepo.GetAll: Found match by MBID: %s (Title: %s)", mf.ID, mf.Title) - return model.MediaFiles{*mf}, nil - } - } - } - } - } - - log.Println("MediaFileRepo.GetAll: No matches found") - return model.MediaFiles{}, nil -} - -// Mock Agent implementation -type MockAgent struct { - agents.Interface - topSongs []agents.Song - err error -} - -func (m *MockAgent) AgentName() string { - return "mock" -} - -func (m *MockAgent) GetArtistTopSongs(ctx context.Context, id, artistName, mbid string, count int) ([]agents.Song, error) { - log.Printf("MockAgent.GetArtistTopSongs called: id=%s, name=%s, mbid=%s, count=%d", id, artistName, mbid, count) - - if m.err != nil { - log.Printf("MockAgent.GetArtistTopSongs returning error: %v", m.err) - return nil, m.err - } - - log.Printf("MockAgent.GetArtistTopSongs returning %d songs", len(m.topSongs)) - return m.topSongs, nil -} - -// Ensure MockAgent implements the necessary interface -var _ agents.ArtistTopSongsRetriever = (*MockAgent)(nil) - -// Sets unexported fields in a struct using reflection and unsafe package -func setStructField(obj interface{}, fieldName string, value interface{}) { - v := reflect.ValueOf(obj).Elem() - f := v.FieldByName(fieldName) - rf := reflect.NewAt(f.Type(), unsafe.Pointer(f.UnsafeAddr())).Elem() - rf.Set(reflect.ValueOf(value)) -} - -var _ = Describe("CustomExternalMetadata", func() { - var ctx context.Context - - BeforeEach(func() { - ctx = context.Background() - }) - - Describe("DirectTopSongs", func() { - var mockArtistRepo *customArtistRepo - var mockMediaFileRepo *customMediaFileRepo - var mockDS *tests.MockDataStore - var mockAgent *MockAgent - var agentsImpl *agents.Agents - var em ExternalMetadata - - BeforeEach(func() { - // Create custom mock repositories - mockArtistRepo = newCustomArtistRepo() - mockMediaFileRepo = newCustomMediaFileRepo() - - // Configure mock data - artist := model.Artist{ID: "artist-1", Name: "Artist One"} - mockArtistRepo.SetData(model.Artists{artist}) - - log.Printf("Test: Set up artist: %s (ID: %s)", artist.Name, artist.ID) - - mediaFiles := []model.MediaFile{ - { - ID: "song-1", - Title: "Song One", - Artist: "Artist One", - ArtistID: "artist-1", - MbzReleaseTrackID: "mbid-1", - Missing: false, - }, - { - ID: "song-2", - Title: "Song Two", - Artist: "Artist One", - ArtistID: "artist-1", - MbzReleaseTrackID: "mbid-2", - Missing: false, - }, - } - mockMediaFileRepo.SetData(model.MediaFiles(mediaFiles)) - - for _, mf := range mediaFiles { - log.Printf("Test: Set up media file: %s (ID: %s, MBID: %s, ArtistID: %s)", - mf.Title, mf.ID, mf.MbzReleaseTrackID, mf.ArtistID) - } - - // Create mock datastore - mockDS = &tests.MockDataStore{ - MockedArtist: mockArtistRepo, - MockedMediaFile: mockMediaFileRepo, - } - - // Create mock agent - mockAgent = &MockAgent{ - topSongs: []agents.Song{ - {Name: "Song One", MBID: "mbid-1"}, - {Name: "Song Two", MBID: "mbid-2"}, - }, - } - - // Use the real agents.Agents implementation but with our mock agent - agentsImpl = &agents.Agents{} - - // Set unexported fields using reflection and unsafe - setStructField(agentsImpl, "ds", mockDS) - setStructField(agentsImpl, "agents", []agents.Interface{mockAgent}) - - // Create our service under test - em = NewExternalMetadata(mockDS, agentsImpl) - }) - - It("returns matching songs from the artist results", func() { - log.Printf("Test: Calling TopSongs for artist: %s", "Artist One") - songs, err := em.TopSongs(ctx, "Artist One", 5) - - log.Printf("Test: Result: error=%v, songs=%v", err, songs) - - Expect(err).ToNot(HaveOccurred()) - Expect(songs).To(HaveLen(2)) - Expect(songs[0].ID).To(Equal("song-1")) - Expect(songs[1].ID).To(Equal("song-2")) - }) - }) - - Describe("TopSongs with agent registration", func() { - var mockArtistRepo *customArtistRepo - var mockMediaFileRepo *customMediaFileRepo - var mockDS *tests.MockDataStore - var mockAgent *MockAgent - var em ExternalMetadata - var originalAgentsConfig string - - BeforeEach(func() { - // Store the original config to restore it later - originalAgentsConfig = conf.Server.Agents - - // Set our mock agent as the only agent - conf.Server.Agents = "mock" - - // Clear the agents map to prevent interference - agents.Map = nil - - // Create custom mock repositories - mockArtistRepo = newCustomArtistRepo() - mockMediaFileRepo = newCustomMediaFileRepo() - - // Configure mock data - artist := model.Artist{ID: "artist-1", Name: "Artist One"} - mockArtistRepo.SetData(model.Artists{artist}) - - log.Printf("Test: Set up artist: %s (ID: %s)", artist.Name, artist.ID) - - mediaFiles := []model.MediaFile{ - { - ID: "song-1", - Title: "Song One", - Artist: "Artist One", - ArtistID: "artist-1", - MbzReleaseTrackID: "mbid-1", - Missing: false, - }, - { - ID: "song-2", - Title: "Song Two", - Artist: "Artist One", - ArtistID: "artist-1", - MbzReleaseTrackID: "mbid-2", - Missing: false, - }, - } - mockMediaFileRepo.SetData(model.MediaFiles(mediaFiles)) - - for _, mf := range mediaFiles { - log.Printf("Test: Set up media file: %s (ID: %s, MBID: %s, ArtistID: %s)", - mf.Title, mf.ID, mf.MbzReleaseTrackID, mf.ArtistID) - } - - // Create mock datastore - mockDS = &tests.MockDataStore{ - MockedArtist: mockArtistRepo, - MockedMediaFile: mockMediaFileRepo, - } - - // Create and register a mock agent - mockAgent = &MockAgent{ - topSongs: []agents.Song{ - {Name: "Song One", MBID: "mbid-1"}, - {Name: "Song Two", MBID: "mbid-2"}, - }, - } - - // Register our mock agent - this is key to making it available - agents.Register("mock", func(model.DataStore) agents.Interface { return mockAgent }) - - // Dump the registered agents for debugging - log.Printf("Test: Registered agents:") - for name := range agents.Map { - log.Printf(" - %s", name) - } - - // Create the service to test - log.Printf("Test: Creating ExternalMetadata with conf.Server.Agents=%s", conf.Server.Agents) - em = NewExternalMetadata(mockDS, agents.GetAgents(mockDS)) - }) - - AfterEach(func() { - conf.Server.Agents = originalAgentsConfig - }) - - It("returns matching songs from the registered agent", func() { - log.Printf("Test: Calling TopSongs for artist: %s", "Artist One") - songs, err := em.TopSongs(ctx, "Artist One", 5) - - log.Printf("Test: Result: error=%v, songs=%v", err, songs) - - Expect(err).ToNot(HaveOccurred()) - Expect(songs).To(HaveLen(2)) - Expect(songs[0].ID).To(Equal("song-1")) - Expect(songs[1].ID).To(Equal("song-2")) - }) - }) -}) diff --git a/core/external_metadata_test.go b/core/external_metadata_test.go index 88a447e99..b8c050bd8 100644 --- a/core/external_metadata_test.go +++ b/core/external_metadata_test.go @@ -9,7 +9,11 @@ import ( "unsafe" "github.com/Masterminds/squirrel" + "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/core/agents" + _ "github.com/navidrome/navidrome/core/agents/lastfm" + _ "github.com/navidrome/navidrome/core/agents/listenbrainz" + _ "github.com/navidrome/navidrome/core/agents/spotify" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/tests" @@ -40,14 +44,19 @@ func (m *mockArtistTopSongsAgent) GetArtistTopSongs(ctx context.Context, id, art m.lastArtistName = artistName m.lastMBID = mbid + log.Debug(ctx, "MockAgent.GetArtistTopSongs called", "id", id, "name", artistName, "mbid", mbid, "count", count) + // Use the custom function if available if m.getArtistTopSongsFn != nil { return m.getArtistTopSongsFn(ctx, id, artistName, mbid, count) } if m.err != nil { + log.Debug(ctx, "MockAgent.GetArtistTopSongs returning error", "err", m.err) return nil, m.err } + + log.Debug(ctx, "MockAgent.GetArtistTopSongs returning songs", "count", len(m.topSongs)) return m.topSongs, nil } @@ -92,6 +101,11 @@ func (m *testArtistRepo) GetAll(options ...model.QueryOptions) (model.Artists, e return nil, errors.New("error") } + // No filters means return all + if len(options) == 0 || options[0].Filters == nil { + return m.artists, nil + } + // Basic implementation that returns artists matching name filter if len(options) > 0 && options[0].Filters != nil { switch f := options[0].Filters.(type) { @@ -108,6 +122,18 @@ func (m *testArtistRepo) GetAll(options ...model.QueryOptions) (model.Artists, e } } } + case squirrel.Eq: + if ids, ok := f["artist.id"]; ok { + var result model.Artists + for _, a := range m.artists { + for _, id := range ids.([]string) { + if a.ID == id { + result = append(result, a) + } + } + } + return result, nil + } } } @@ -208,6 +234,15 @@ func (m *testMediaFileRepo) handleAndFilter(andFilter squirrel.And, option model } } } + if albumArtistID, ok := eqCond["album_artist_id"]; ok { + log.Debug("MediaFileRepo.handleAndFilter: Looking for album_artist_id", "albumArtistID", albumArtistID) + for _, mf := range notMissingMatches { + if mf.AlbumArtistID == albumArtistID.(string) { + log.Debug("MediaFileRepo.handleAndFilter: Found match by album_artist_id", "id", mf.ID, "title", mf.Title) + artistMatches = append(artistMatches, mf) + } + } + } } } case squirrel.Like: @@ -225,6 +260,12 @@ func (m *testMediaFileRepo) handleAndFilter(andFilter squirrel.And, option model } } } + case squirrel.Eq: + // Handle missing check + if missingFlag, ok := filter["missing"]; ok && !missingFlag.(bool) { + // This is already handled above when we build notMissingMatches + continue + } } } @@ -257,10 +298,14 @@ var _ = Describe("ExternalMetadata", func() { var mockArtistRepo *testArtistRepo var mockMediaFileRepo *testMediaFileRepo var ctx context.Context + var originalAgentsConfig string BeforeEach(func() { ctx = context.Background() + // Store the original agents config to restore it later + originalAgentsConfig = conf.Server.Agents + // Setup mocks mockArtistRepo = newTestArtistRepo() mockMediaFileRepo = newTestMediaFileRepo() @@ -275,31 +320,21 @@ var _ = Describe("ExternalMetadata", func() { // Create a mock agent mockAgent = &mockArtistTopSongsAgent{} - log.Debug("Creating mock agent", "agent", mockAgent) - agents.Register("mock", func(model.DataStore) agents.Interface { return mockAgent }) - - // Create a custom agents instance directly with our mock agent - agentsImpl := &agents.Agents{} - - // Use reflection to set the unexported fields - setAgentField(agentsImpl, "ds", ds) - setAgentField(agentsImpl, "agents", []agents.Interface{mockAgent}) - - // Create the externalMetadata instance with our custom Agents implementation - em = NewExternalMetadata(ds, agentsImpl) - - // Verify that the agent is available - log.Debug("ExternalMetadata created", "em", em) + log.Debug(ctx, "Creating mock agent", "agent", mockAgent) }) - Describe("TopSongs", func() { + AfterEach(func() { + // Restore original config + conf.Server.Agents = originalAgentsConfig + }) + + Describe("TopSongs with direct agent injection", func() { BeforeEach(func() { // Set up artists data mockArtistRepo.SetData(model.Artists{ {ID: "artist-1", Name: "Artist One"}, {ID: "artist-2", Name: "Artist Two"}, }) - log.Debug("Artist data set up", "count", 2) // Set up mediafiles data with all necessary fields for matching mockMediaFileRepo.SetData(model.MediaFiles{ @@ -331,23 +366,27 @@ var _ = Describe("ExternalMetadata", func() { Missing: false, }, }) - log.Debug("Media files data set up", "count", 3) // Configure the mockAgent to return some top songs mockAgent.topSongs = []agents.Song{ {Name: "Song One", MBID: "mbid-1"}, {Name: "Song Two", MBID: "mbid-2"}, } - log.Debug("Mock agent configured with top songs", "count", len(mockAgent.topSongs)) + + // Create a custom agents instance directly with our mock agent + agentsImpl := &agents.Agents{} + + // Use reflection to set the unexported fields + setAgentField(agentsImpl, "ds", ds) + setAgentField(agentsImpl, "agents", []agents.Interface{mockAgent}) + + // Create the externalMetadata instance with our custom Agents implementation + em = NewExternalMetadata(ds, agentsImpl) }) It("returns matching songs from the agent results", func() { - log.Debug("Running test: returns matching songs from the agent results") - songs, err := em.TopSongs(ctx, "Artist One", 5) - log.Debug("Test results", "err", err, "songs", songs) - Expect(err).ToNot(HaveOccurred()) Expect(songs).To(HaveLen(2)) Expect(songs[0].ID).To(Equal("song-1")) @@ -360,13 +399,11 @@ var _ = Describe("ExternalMetadata", func() { }) It("returns nil when artist is not found", func() { - // Empty mockArtistRepo to simulate artist not found - mockArtistRepo.err = errors.New("artist repo error") + // Set an error for mockArtistRepo to simulate artist not found + mockArtistRepo.SetError(true) songs, err := em.TopSongs(ctx, "Unknown Artist", 5) - log.Debug("Test results after TopSongs call with unknown artist", "err", err, "songs", songs) - Expect(err).To(BeNil()) Expect(songs).To(BeNil()) }) @@ -379,111 +416,21 @@ var _ = Describe("ExternalMetadata", func() { songs, err := em.TopSongs(ctx, "Artist One", 5) - log.Debug("Test results for non-matching songs", "err", err, "songs", songs) - Expect(err).ToNot(HaveOccurred()) Expect(songs).To(HaveLen(0)) }) - It("returns nil when agent returns other errors", func() { - // We need to ensure artist is found first - mockArtistRepo.err = nil + It("returns nil when agent returns errors", func() { + // Reset the agent error state + mockArtistRepo.SetError(false) - // Create the error right away + // Set the error testError := errors.New("some agent error") - - // Reset the default mock agent mockAgent.err = testError - mockAgent.topSongs = nil // Make sure no songs are returned with the error - log.Debug("==================== TEST SETUP ====================") - log.Debug("Using default mock agent for this test", "agent", mockAgent, "err", mockAgent.err) + songs, err := em.TopSongs(ctx, "Artist One", 5) - // Directly test the mock agent's GetArtistTopSongs function - songs, err := mockAgent.GetArtistTopSongs(ctx, "1", "Artist One", "mbz-1", 5) - log.Debug("Direct GetArtistTopSongs result", "songs", songs, "err", err) - Expect(err).To(Equal(testError)) - - // Directly test the agents.Agents implementation to check how it handles errors - agentsObj := &agents.Agents{} - setAgentField(agentsObj, "ds", ds) - setAgentField(agentsObj, "agents", []agents.Interface{mockAgent}) - - // Test the wrapped agent directly - songs, err = agentsObj.GetArtistTopSongs(ctx, "1", "Artist One", "mbz-1", 5) - log.Debug("Agents.GetArtistTopSongs result", "songs", songs, "err", err) - // If Agents.GetArtistTopSongs swallows errors and returns ErrNotFound, that's an issue with agents - // but we're still testing the current behavior - - // Create a direct agent that returns the error directly from GetArtistTopSongs - directAgent := &mockArtistTopSongsAgent{ - getArtistTopSongsFn: func(ctx context.Context, id, artistName, mbid string, count int) ([]agents.Song, error) { - return nil, testError - }, - } - - // Create the ExternalMetadata instance with a direct pipeline to our agent - directAgentsObj := &agents.Agents{} - setAgentField(directAgentsObj, "ds", ds) - setAgentField(directAgentsObj, "agents", []agents.Interface{directAgent}) - - // Create the externalMetadata instance to test - directEM := NewExternalMetadata(ds, directAgentsObj) - - // Call the method we're testing with our direct agent setup - songs2, err := directEM.TopSongs(ctx, "Artist One", 5) - - log.Debug("Direct TopSongs result", "err", err, "songs", songs2) - - // With our improved code, the error should now be propagated if it's passed directly from the agent - // But we keep this test in its original form to ensure the current behavior works - // A new test will be added that tests the improved error propagation - Expect(err).To(BeNil()) - Expect(songs2).To(BeNil()) - }) - - It("propagates errors with direct agent implementation", func() { - // We need to ensure artist is found first - mockArtistRepo.err = nil - - // Create the error right away - testError := errors.New("direct agent error") - - // Create a direct agent that bypasses agents.Agents - // This simulates a case where the error would be properly propagated if agents.Agents - // wasn't silently swallowing errors - directAgent := &mockArtistTopSongsAgent{ - getArtistTopSongsFn: func(ctx context.Context, id, artistName, mbid string, count int) ([]agents.Song, error) { - log.Debug("Direct agent GetArtistTopSongs called", "id", id, "name", artistName, "count", count) - return nil, testError - }, - } - - // Check that our direct agent works as expected - songsTest, errTest := directAgent.GetArtistTopSongs(ctx, "1", "Artist One", "mbz-1", 5) - log.Debug("Testing direct agent", "songs", songsTest, "err", errTest) - Expect(errTest).To(Equal(testError)) - - // Create a custom implementation of agents.Agents that will return our error - directAgentsImpl := &agents.Agents{} - setAgentField(directAgentsImpl, "ds", ds) - setAgentField(directAgentsImpl, "agents", []agents.Interface{directAgent}) - - // Test the agents implementation directly - songsAgents, errAgents := directAgentsImpl.GetArtistTopSongs(ctx, "1", "Artist One", "mbz-1", 5) - log.Debug("Direct agents result", "songs", songsAgents, "err", errAgents) - - // Create a new external metadata instance - directEM := NewExternalMetadata(ds, directAgentsImpl) - - // Call the method we're testing with our direct implementation - songs, err := directEM.TopSongs(ctx, "Artist One", 5) - - log.Debug("Direct TopSongs result with propagation", "err", err, "songs", songs) - - // In theory this would pass with the improved implementation, but in practice - // the root issue is the agents.Agents implementation that swallows non-ErrNotFound errors - // For now we'll expect nil, which matches the current behavior + // Current behavior returns nil for both error and songs Expect(err).To(BeNil()) Expect(songs).To(BeNil()) }) @@ -497,11 +444,95 @@ var _ = Describe("ExternalMetadata", func() { songs, err := em.TopSongs(ctx, "Artist One", 1) - log.Debug("Test results for count parameter", "err", err, "songs", songs, "count", len(songs)) - Expect(err).ToNot(HaveOccurred()) Expect(songs).To(HaveLen(1)) Expect(songs[0].ID).To(Equal("song-1")) }) }) + + Describe("TopSongs with agent registration", func() { + BeforeEach(func() { + // Set our mock agent as the only agent + conf.Server.Agents = "mock" + + // Set up artists data + mockArtistRepo.SetData(model.Artists{ + {ID: "artist-1", Name: "Artist One"}, + }) + + // Set up mediafiles data + mockMediaFileRepo.SetData(model.MediaFiles{ + { + ID: "song-1", + Title: "Song One", + Artist: "Artist One", + ArtistID: "artist-1", + MbzReleaseTrackID: "mbid-1", + Missing: false, + }, + { + ID: "song-2", + Title: "Song Two", + Artist: "Artist One", + ArtistID: "artist-1", + MbzReleaseTrackID: "mbid-2", + Missing: false, + }, + }) + + // Configure and register the agent + mockAgent.topSongs = []agents.Song{ + {Name: "Song One", MBID: "mbid-1"}, + {Name: "Song Two", MBID: "mbid-2"}, + } + + // Register our mock agent + agents.Register("mock", func(model.DataStore) agents.Interface { return mockAgent }) + + // Create the externalMetadata instance with registered agents + em = NewExternalMetadata(ds, agents.GetAgents(ds)) + }) + + It("returns matching songs from the registered agent", func() { + songs, err := em.TopSongs(ctx, "Artist One", 5) + + Expect(err).ToNot(HaveOccurred()) + Expect(songs).To(HaveLen(2)) + Expect(songs[0].ID).To(Equal("song-1")) + Expect(songs[1].ID).To(Equal("song-2")) + }) + }) + + Describe("Error propagation from agents", func() { + BeforeEach(func() { + // Set up artists data + mockArtistRepo.SetData(model.Artists{ + {ID: "artist-1", Name: "Artist One"}, + }) + + // Create a direct agent that returns an error + testError := errors.New("direct agent error") + directAgent := &mockArtistTopSongsAgent{ + getArtistTopSongsFn: func(ctx context.Context, id, artistName, mbid string, count int) ([]agents.Song, error) { + return nil, testError + }, + } + + // Create a custom implementation of agents.Agents that will return our error + directAgentsImpl := &agents.Agents{} + setAgentField(directAgentsImpl, "ds", ds) + setAgentField(directAgentsImpl, "agents", []agents.Interface{directAgent}) + + // Create a new external metadata instance + em = NewExternalMetadata(ds, directAgentsImpl) + }) + + It("handles errors from the agent according to current behavior", func() { + songs, err := em.TopSongs(ctx, "Artist One", 5) + + // Current behavior returns nil for both error and songs + Expect(err).To(BeNil()) + Expect(songs).To(BeNil()) + }) + }) })