mirror of
https://github.com/navidrome/navidrome.git
synced 2026-05-03 06:51:16 +00:00
52e47b896a
* refactor: extract matchSongsToLibrary to core/matcher package Move the song-to-library matching algorithm from core/external into its own core/matcher package. The Matcher struct exposes a single public method MatchSongsToLibrary that implements a multi-phase matching algorithm (ID > MBID > ISRC > fuzzy title+artist). Includes pre-sanitization optimization for the fuzzy matching loop. No behavioral changes — the algorithm is identical to the version in core/external/provider_matching.go. * refactor: inject matcher.Matcher via Wire instead of creating it inline Add *matcher.Matcher as a dependency of external.NewProvider, wired via Google Wire. Update all provider test files to pass matcher.New(ds). This eliminates tight coupling so future consumers can reuse the matcher without depending on the external package. * refactor: remove old provider_matching files Delete core/external/provider_matching.go and its tests. All matching logic now lives in core/matcher/. * test(matcher): restore test coverage lost in extraction Port back 23 specs that existed in the old provider_matching_test.go but were dropped during the extraction. Covers specificity levels, fuzzy matching thresholds, fuzzy album matching, duration matching, and deduplication edge cases. * test(matcher): extract matchFieldInAnd/matchFieldInEq helpers The four inline mock.MatchedBy closures in setupAllPhaseExpectations all followed the same squirrel.And -> squirrel.Eq -> field-name-check pattern. Extract into two small helpers to reduce duplication and make the setup functions read as a concise list of phase expectations. * refactor(matcher): address PR #5348 review feedback - sanitizedTrack now holds *model.MediaFile instead of a value copy. Since MediaFile is a large struct (~74 fields), this avoids the per-track copy into sanitized[] and a second copy when findBestMatch assigns the winner. loadTracksByTitleAndArtist updated to iterate by index and pass &tracks[i]. - loadTracksByISRC now sorts results (starred desc, rating desc, year asc, compilation asc) so that when multiple library tracks share an ISRC the most relevant one is picked deterministically, matching the sort order already used by loadTracksByTitleAndArtist. - Restored the four worked examples (MBID Priority, ISRC Priority, Specificity Ranking, Fuzzy Title Matching) in the MatchSongsToLibrary godoc that were dropped during the extraction. - matcher_test.go: tests now enforce expectations via AssertExpectations in a DeferCleanup. The old setupAllPhaseExpectations helper was replaced with per-phase helpers (expectIDPhase/expectMBIDPhase/expectISRCPhase + allowOtherPhases) so each test deterministically verifies which matching phases fire. This surfaced (and fixes) a latent issue copilot flagged: the old .Once() expectations were not actually asserted, so tests would silently pass even when phases short-circuited unexpectedly.
315 lines
14 KiB
Go
315 lines
14 KiB
Go
package external_test
|
|
|
|
import (
|
|
"context"
|
|
"errors"
|
|
"time"
|
|
|
|
"github.com/navidrome/navidrome/conf"
|
|
"github.com/navidrome/navidrome/conf/configtest"
|
|
"github.com/navidrome/navidrome/core/agents"
|
|
"github.com/navidrome/navidrome/core/external"
|
|
"github.com/navidrome/navidrome/core/matcher"
|
|
"github.com/navidrome/navidrome/log"
|
|
"github.com/navidrome/navidrome/model"
|
|
"github.com/navidrome/navidrome/tests"
|
|
"github.com/navidrome/navidrome/utils/gg"
|
|
. "github.com/onsi/ginkgo/v2"
|
|
. "github.com/onsi/gomega"
|
|
"github.com/stretchr/testify/mock"
|
|
)
|
|
|
|
func init() {
|
|
log.SetLevel(log.LevelDebug)
|
|
}
|
|
|
|
var _ = Describe("Provider - UpdateArtistInfo", func() {
|
|
var (
|
|
ctx context.Context
|
|
p external.Provider
|
|
ds *tests.MockDataStore
|
|
ag *mockAgents
|
|
mockArtistRepo *tests.MockArtistRepo
|
|
)
|
|
|
|
BeforeEach(func() {
|
|
DeferCleanup(configtest.SetupConfig())
|
|
conf.Server.DevArtistInfoTimeToLive = 1 * time.Hour
|
|
ctx = GinkgoT().Context()
|
|
ds = new(tests.MockDataStore)
|
|
ag = new(mockAgents)
|
|
p = external.NewProvider(ds, ag, matcher.New(ds))
|
|
mockArtistRepo = ds.Artist(ctx).(*tests.MockArtistRepo)
|
|
})
|
|
|
|
It("returns error when artist is not found", func() {
|
|
artist, err := p.UpdateArtistInfo(ctx, "ar-not-found", 10, false)
|
|
|
|
Expect(err).To(MatchError(model.ErrNotFound))
|
|
Expect(artist).To(BeNil())
|
|
ag.AssertNotCalled(GinkgoT(), "GetArtistMBID")
|
|
ag.AssertNotCalled(GinkgoT(), "GetArtistImages")
|
|
ag.AssertNotCalled(GinkgoT(), "GetArtistBiography")
|
|
ag.AssertNotCalled(GinkgoT(), "GetArtistURL")
|
|
ag.AssertNotCalled(GinkgoT(), "GetSimilarArtists")
|
|
})
|
|
|
|
It("populates info when artist exists but has no external info", func() {
|
|
originalArtist := &model.Artist{
|
|
ID: "ar-existing",
|
|
Name: "Test Artist",
|
|
}
|
|
mockArtistRepo.SetData(model.Artists{*originalArtist})
|
|
|
|
expectedMBID := "mbid-artist-123"
|
|
expectedBio := "Artist Bio"
|
|
expectedURL := "http://artist.url"
|
|
expectedImages := []agents.ExternalImage{
|
|
{URL: "http://large.jpg", Size: 300},
|
|
{URL: "http://medium.jpg", Size: 200},
|
|
{URL: "http://small.jpg", Size: 100},
|
|
}
|
|
rawSimilar := []agents.Artist{
|
|
{Name: "Similar Artist 1", MBID: "mbid-similar-1"},
|
|
{Name: "Similar Artist 2", MBID: "mbid-similar-2"},
|
|
{Name: "Similar Artist 3", MBID: "mbid-similar-3"},
|
|
}
|
|
similarInDS := model.Artist{ID: "ar-similar-2", Name: "Similar Artist 2"}
|
|
|
|
ag.On("GetArtistMBID", ctx, "ar-existing", "Test Artist").Return(expectedMBID, nil).Once()
|
|
ag.On("GetArtistImages", ctx, "ar-existing", "Test Artist", expectedMBID).Return(expectedImages, nil).Once()
|
|
ag.On("GetArtistBiography", ctx, "ar-existing", "Test Artist", expectedMBID).Return(expectedBio, nil).Once()
|
|
ag.On("GetArtistURL", ctx, "ar-existing", "Test Artist", expectedMBID).Return(expectedURL, nil).Once()
|
|
ag.On("GetSimilarArtists", ctx, "ar-existing", "Test Artist", expectedMBID, 100).Return(rawSimilar, nil).Once()
|
|
|
|
mockArtistRepo.SetData(model.Artists{*originalArtist, similarInDS})
|
|
|
|
updatedArtist, err := p.UpdateArtistInfo(ctx, "ar-existing", 10, false)
|
|
|
|
Expect(err).NotTo(HaveOccurred())
|
|
Expect(updatedArtist).NotTo(BeNil())
|
|
Expect(updatedArtist.ID).To(Equal("ar-existing"))
|
|
Expect(updatedArtist.MbzArtistID).To(Equal(expectedMBID))
|
|
Expect(updatedArtist.Biography).To(Equal("Artist Bio"))
|
|
Expect(updatedArtist.ExternalUrl).To(Equal(expectedURL))
|
|
Expect(updatedArtist.LargeImageUrl).To(Equal("http://large.jpg"))
|
|
Expect(updatedArtist.MediumImageUrl).To(Equal("http://medium.jpg"))
|
|
Expect(updatedArtist.SmallImageUrl).To(Equal("http://small.jpg"))
|
|
Expect(updatedArtist.ExternalInfoUpdatedAt).NotTo(BeNil())
|
|
Expect(*updatedArtist.ExternalInfoUpdatedAt).To(BeTemporally("~", time.Now(), time.Second))
|
|
|
|
Expect(updatedArtist.SimilarArtists).To(HaveLen(1))
|
|
Expect(updatedArtist.SimilarArtists[0].ID).To(Equal("ar-similar-2"))
|
|
Expect(updatedArtist.SimilarArtists[0].Name).To(Equal("Similar Artist 2"))
|
|
|
|
ag.AssertExpectations(GinkgoT())
|
|
})
|
|
|
|
It("returns cached info when artist exists and info is not expired", func() {
|
|
now := time.Now()
|
|
originalArtist := &model.Artist{
|
|
ID: "ar-cached",
|
|
Name: "Cached Artist",
|
|
MbzArtistID: "mbid-cached",
|
|
ExternalUrl: "http://cached.url",
|
|
Biography: "Cached Bio",
|
|
LargeImageUrl: "http://cached_large.jpg",
|
|
ExternalInfoUpdatedAt: gg.P(now.Add(-conf.Server.DevArtistInfoTimeToLive / 2)),
|
|
SimilarArtists: model.Artists{
|
|
{ID: "ar-similar-present", Name: "Similar Present"},
|
|
{ID: "ar-similar-absent", Name: "Similar Absent"},
|
|
},
|
|
}
|
|
similarInDS := model.Artist{ID: "ar-similar-present", Name: "Similar Present Updated"}
|
|
mockArtistRepo.SetData(model.Artists{*originalArtist, similarInDS})
|
|
|
|
updatedArtist, err := p.UpdateArtistInfo(ctx, "ar-cached", 5, false)
|
|
|
|
Expect(err).NotTo(HaveOccurred())
|
|
Expect(updatedArtist).NotTo(BeNil())
|
|
Expect(updatedArtist.ID).To(Equal(originalArtist.ID))
|
|
Expect(updatedArtist.Name).To(Equal(originalArtist.Name))
|
|
Expect(updatedArtist.MbzArtistID).To(Equal(originalArtist.MbzArtistID))
|
|
Expect(updatedArtist.ExternalUrl).To(Equal(originalArtist.ExternalUrl))
|
|
Expect(updatedArtist.Biography).To(Equal(originalArtist.Biography))
|
|
Expect(updatedArtist.LargeImageUrl).To(Equal(originalArtist.LargeImageUrl))
|
|
Expect(updatedArtist.ExternalInfoUpdatedAt).To(Equal(originalArtist.ExternalInfoUpdatedAt))
|
|
|
|
Expect(updatedArtist.SimilarArtists).To(HaveLen(1))
|
|
Expect(updatedArtist.SimilarArtists[0].ID).To(Equal(similarInDS.ID))
|
|
Expect(updatedArtist.SimilarArtists[0].Name).To(Equal(similarInDS.Name))
|
|
|
|
ag.AssertNotCalled(GinkgoT(), "GetArtistMBID")
|
|
ag.AssertNotCalled(GinkgoT(), "GetArtistImages")
|
|
ag.AssertNotCalled(GinkgoT(), "GetArtistBiography")
|
|
ag.AssertNotCalled(GinkgoT(), "GetArtistURL")
|
|
})
|
|
|
|
It("returns cached info and triggers background refresh when info is expired", func() {
|
|
now := time.Now()
|
|
expiredTime := now.Add(-conf.Server.DevArtistInfoTimeToLive * 2)
|
|
originalArtist := &model.Artist{
|
|
ID: "ar-expired",
|
|
Name: "Expired Artist",
|
|
ExternalInfoUpdatedAt: gg.P(expiredTime),
|
|
SimilarArtists: model.Artists{
|
|
{ID: "ar-exp-similar", Name: "Expired Similar"},
|
|
},
|
|
}
|
|
similarInDS := model.Artist{ID: "ar-exp-similar", Name: "Expired Similar Updated"}
|
|
mockArtistRepo.SetData(model.Artists{*originalArtist, similarInDS})
|
|
|
|
updatedArtist, err := p.UpdateArtistInfo(ctx, "ar-expired", 5, false)
|
|
|
|
Expect(err).NotTo(HaveOccurred())
|
|
Expect(updatedArtist).NotTo(BeNil())
|
|
Expect(updatedArtist.ID).To(Equal(originalArtist.ID))
|
|
Expect(updatedArtist.Name).To(Equal(originalArtist.Name))
|
|
Expect(updatedArtist.ExternalInfoUpdatedAt).To(Equal(originalArtist.ExternalInfoUpdatedAt))
|
|
|
|
Expect(updatedArtist.SimilarArtists).To(HaveLen(1))
|
|
Expect(updatedArtist.SimilarArtists[0].ID).To(Equal(similarInDS.ID))
|
|
Expect(updatedArtist.SimilarArtists[0].Name).To(Equal(similarInDS.Name))
|
|
|
|
ag.AssertNotCalled(GinkgoT(), "GetArtistMBID")
|
|
ag.AssertNotCalled(GinkgoT(), "GetArtistImages")
|
|
ag.AssertNotCalled(GinkgoT(), "GetArtistBiography")
|
|
ag.AssertNotCalled(GinkgoT(), "GetArtistURL")
|
|
})
|
|
|
|
It("includes non-present similar artists when includeNotPresent is true", func() {
|
|
now := time.Now()
|
|
originalArtist := &model.Artist{
|
|
ID: "ar-similar-test",
|
|
Name: "Similar Test Artist",
|
|
ExternalInfoUpdatedAt: gg.P(now.Add(-conf.Server.DevArtistInfoTimeToLive / 2)),
|
|
SimilarArtists: model.Artists{
|
|
{ID: "ar-sim-present", Name: "Similar Present"},
|
|
{ID: "", Name: "Similar Absent Raw"},
|
|
{ID: "ar-sim-absent-lookup", Name: "Similar Absent Lookup"},
|
|
},
|
|
}
|
|
similarInDS := model.Artist{ID: "ar-sim-present", Name: "Similar Present Updated"}
|
|
mockArtistRepo.SetData(model.Artists{*originalArtist, similarInDS})
|
|
|
|
updatedArtist, err := p.UpdateArtistInfo(ctx, "ar-similar-test", 5, true)
|
|
|
|
Expect(err).NotTo(HaveOccurred())
|
|
Expect(updatedArtist).NotTo(BeNil())
|
|
|
|
Expect(updatedArtist.SimilarArtists).To(HaveLen(3))
|
|
Expect(updatedArtist.SimilarArtists[0].ID).To(Equal(similarInDS.ID))
|
|
Expect(updatedArtist.SimilarArtists[0].Name).To(Equal(similarInDS.Name))
|
|
Expect(updatedArtist.SimilarArtists[1].ID).To(BeEmpty())
|
|
Expect(updatedArtist.SimilarArtists[1].Name).To(Equal("Similar Absent Raw"))
|
|
Expect(updatedArtist.SimilarArtists[2].ID).To(BeEmpty())
|
|
Expect(updatedArtist.SimilarArtists[2].Name).To(Equal("Similar Absent Lookup"))
|
|
})
|
|
|
|
It("updates ArtistInfo even if an optional agent call fails", func() {
|
|
originalArtist := &model.Artist{
|
|
ID: "ar-agent-fail",
|
|
Name: "Agent Fail Artist",
|
|
}
|
|
mockArtistRepo.SetData(model.Artists{*originalArtist})
|
|
|
|
expectedErr := errors.New("agent MBID failed")
|
|
ag.On("GetArtistMBID", ctx, "ar-agent-fail", "Agent Fail Artist").Return("", expectedErr).Once()
|
|
ag.On("GetArtistImages", ctx, "ar-agent-fail", "Agent Fail Artist", mock.Anything).Return(nil, nil).Maybe()
|
|
ag.On("GetArtistBiography", ctx, "ar-agent-fail", "Agent Fail Artist", mock.Anything).Return("", nil).Maybe()
|
|
ag.On("GetArtistURL", ctx, "ar-agent-fail", "Agent Fail Artist", mock.Anything).Return("", nil).Maybe()
|
|
ag.On("GetSimilarArtists", ctx, "ar-agent-fail", "Agent Fail Artist", mock.Anything, 100).Return(nil, nil).Maybe()
|
|
|
|
updatedArtist, err := p.UpdateArtistInfo(ctx, "ar-agent-fail", 10, false)
|
|
|
|
Expect(err).NotTo(HaveOccurred())
|
|
Expect(updatedArtist).NotTo(BeNil())
|
|
Expect(updatedArtist.ID).To(Equal("ar-agent-fail"))
|
|
ag.AssertExpectations(GinkgoT())
|
|
})
|
|
|
|
It("matches similar artists by ID first when agent provides IDs", func() {
|
|
originalArtist := &model.Artist{
|
|
ID: "ar-id-match",
|
|
Name: "ID Match Artist",
|
|
}
|
|
similarByID := model.Artist{ID: "ar-similar-by-id", Name: "Similar By ID", MbzArtistID: "mbid-similar"}
|
|
mockArtistRepo.SetData(model.Artists{*originalArtist, similarByID})
|
|
|
|
// Agent returns similar artist with ID (highest priority matching)
|
|
rawSimilar := []agents.Artist{
|
|
{ID: "ar-similar-by-id", Name: "Different Name", MBID: "different-mbid"},
|
|
}
|
|
|
|
ag.On("GetArtistMBID", ctx, "ar-id-match", "ID Match Artist").Return("", nil).Once()
|
|
ag.On("GetArtistImages", ctx, "ar-id-match", "ID Match Artist", mock.Anything).Return(nil, nil).Maybe()
|
|
ag.On("GetArtistBiography", ctx, "ar-id-match", "ID Match Artist", mock.Anything).Return("", nil).Maybe()
|
|
ag.On("GetArtistURL", ctx, "ar-id-match", "ID Match Artist", mock.Anything).Return("", nil).Maybe()
|
|
ag.On("GetSimilarArtists", ctx, "ar-id-match", "ID Match Artist", mock.Anything, 100).Return(rawSimilar, nil).Once()
|
|
|
|
updatedArtist, err := p.UpdateArtistInfo(ctx, "ar-id-match", 10, false)
|
|
|
|
Expect(err).NotTo(HaveOccurred())
|
|
Expect(updatedArtist.SimilarArtists).To(HaveLen(1))
|
|
// Should match by ID, not by name or MBID
|
|
Expect(updatedArtist.SimilarArtists[0].ID).To(Equal("ar-similar-by-id"))
|
|
Expect(updatedArtist.SimilarArtists[0].Name).To(Equal("Similar By ID"))
|
|
})
|
|
|
|
It("matches similar artists by MBID when ID is empty", func() {
|
|
originalArtist := &model.Artist{
|
|
ID: "ar-mbid-match",
|
|
Name: "MBID Match Artist",
|
|
}
|
|
similarByMBID := model.Artist{ID: "ar-similar-by-mbid", Name: "Similar By MBID", MbzArtistID: "mbid-similar"}
|
|
mockArtistRepo.SetData(model.Artists{*originalArtist, similarByMBID})
|
|
|
|
// Agent returns similar artist with only MBID (no ID)
|
|
rawSimilar := []agents.Artist{
|
|
{Name: "Different Name", MBID: "mbid-similar"},
|
|
}
|
|
|
|
ag.On("GetArtistMBID", ctx, "ar-mbid-match", "MBID Match Artist").Return("", nil).Once()
|
|
ag.On("GetArtistImages", ctx, "ar-mbid-match", "MBID Match Artist", mock.Anything).Return(nil, nil).Maybe()
|
|
ag.On("GetArtistBiography", ctx, "ar-mbid-match", "MBID Match Artist", mock.Anything).Return("", nil).Maybe()
|
|
ag.On("GetArtistURL", ctx, "ar-mbid-match", "MBID Match Artist", mock.Anything).Return("", nil).Maybe()
|
|
ag.On("GetSimilarArtists", ctx, "ar-mbid-match", "MBID Match Artist", mock.Anything, 100).Return(rawSimilar, nil).Once()
|
|
|
|
updatedArtist, err := p.UpdateArtistInfo(ctx, "ar-mbid-match", 10, false)
|
|
|
|
Expect(err).NotTo(HaveOccurred())
|
|
Expect(updatedArtist.SimilarArtists).To(HaveLen(1))
|
|
// Should match by MBID since ID was empty
|
|
Expect(updatedArtist.SimilarArtists[0].ID).To(Equal("ar-similar-by-mbid"))
|
|
Expect(updatedArtist.SimilarArtists[0].Name).To(Equal("Similar By MBID"))
|
|
})
|
|
|
|
It("falls back to name matching when ID and MBID don't match", func() {
|
|
originalArtist := &model.Artist{
|
|
ID: "ar-name-match",
|
|
Name: "Name Match Artist",
|
|
}
|
|
similarByName := model.Artist{ID: "ar-similar-by-name", Name: "Similar By Name"}
|
|
mockArtistRepo.SetData(model.Artists{*originalArtist, similarByName})
|
|
|
|
// Agent returns similar artist with non-matching ID and MBID
|
|
rawSimilar := []agents.Artist{
|
|
{ID: "non-existent-id", Name: "Similar By Name", MBID: "non-existent-mbid"},
|
|
}
|
|
|
|
ag.On("GetArtistMBID", ctx, "ar-name-match", "Name Match Artist").Return("", nil).Once()
|
|
ag.On("GetArtistImages", ctx, "ar-name-match", "Name Match Artist", mock.Anything).Return(nil, nil).Maybe()
|
|
ag.On("GetArtistBiography", ctx, "ar-name-match", "Name Match Artist", mock.Anything).Return("", nil).Maybe()
|
|
ag.On("GetArtistURL", ctx, "ar-name-match", "Name Match Artist", mock.Anything).Return("", nil).Maybe()
|
|
ag.On("GetSimilarArtists", ctx, "ar-name-match", "Name Match Artist", mock.Anything, 100).Return(rawSimilar, nil).Once()
|
|
|
|
updatedArtist, err := p.UpdateArtistInfo(ctx, "ar-name-match", 10, false)
|
|
|
|
Expect(err).NotTo(HaveOccurred())
|
|
Expect(updatedArtist.SimilarArtists).To(HaveLen(1))
|
|
// Should fall back to name matching since ID and MBID didn't match
|
|
Expect(updatedArtist.SimilarArtists[0].ID).To(Equal("ar-similar-by-name"))
|
|
Expect(updatedArtist.SimilarArtists[0].Name).To(Equal("Similar By Name"))
|
|
})
|
|
})
|