From 803b385920ce50b9d5ec42c152551dfc7de5c48c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Sun, 21 Jun 2026 11:05:58 -0400 Subject: [PATCH] fix(matcher): match by artist credit so artist-MBID specificity works and collaborators match (#5637) * refactor(matcher): carry resolved artist MBID on sanitizedTrack * feat(matcher): two-phase artist resolution for title matching * fix(matcher): wire phase-1 artist mock in consumer tests; guard back-map dupes - External tests for title-fallback paths now register a second `.Maybe()` artistRepo.GetAll expectation for the matcher's phase-1 artist-resolution call, and supply RoleArtist Participants on phase-2 track mocks so the back-map routes tracks to their query buckets correctly. - Back-map loop extracted into buildTracksByQuery helper; guard ensures each track is appended at most once per query bucket even when it credits multiple resolved artists in that bucket, preventing duplicate scoring. - matchTitlePhase2 helper: removed redundant squirrel.Sqlizer type assertion (ranging over squirrel.And already yields Sqlizer). * perf(matcher): use non-correlated IN subquery for phase-2 lookup * refactor(matcher): decompose two-phase title matching into named steps matchByTitle had grown to ~98 lines holding four jobs and eleven locals. Split it into a thin orchestrator over named phases: - groupQueriesByArtist: build the per-artist query buckets. - resolveArtists / resolvedArtists: phase 1, with the three parallel maps (byQuery / mbid / allIDs) folded into one type that owns artist-ID routing and track bucketing. Artist ownership is now two direct map lookups (by order name, by MBID) instead of the prior O(artists x queries) nested scan. - fetchTracksCreditedTo: phase 2, using squirrel.Placeholders instead of a hand-rolled placeholder string. - bucketTracks / scoring loop: unchanged behavior. Pure restructuring; the suite, race, consumers, and the real-DB benchmark are all unchanged. Also dedupes the phase-1 MBID filter as a side effect. * refactor(matcher): apply simplify cleanups to two-phase resolution Behavior-preserving cleanups from a /simplify pass: - struct{} sets instead of bool-valued sets (codebase convention) - move newSanitizedTrack after the dedup guard in bucketTracks, so a track credited to multiple resolved artists is sanitized only when actually bucketed - pre-size the phase-1/phase-2 maps; drop the dead nil-guard in own() - slice.Map for the []string->[]any arg conversion - document why the raw media_file_artists SQL stays in this layer, and that bucketTracks relies on the bulk participants JSON carrying artist IDs - extract an artistParticipants() test helper, collapsing 57 four-level Participants literals (test file -211 lines net) * docs(matcher): rename inner title-matching steps to avoid 'phase' clash The matcher's top-level strategies (ID/MBID/ISRC/Title) are already called phases. Reusing 'phase 1/2' for the two steps inside title matching (resolve artists, fetch their tracks) was confusing. Drop the ordinals and let the function names (resolveArtists, fetchTracksCreditedTo) and prose describe the steps. Renamed the matchTitlePhase2 test helper to matchTracksByArtistQuery. Comment-only; no behavior change. * fix(matcher): own MBID-resolved artists for every aliased query Address bot review on PR #5637. When several agent queries share one ArtistMBID under different sanitized names (agent aliases), the resolver kept only the last query name per MBID, so the others never owned the resolved artist and their songs fell through. Track all query names per MBID instead; add a RED-proven test for the two-alias case. Also invert byQuery into a reverse artist-ID -> query-name index in bucketTracks, dropping the per-participant scan over all queries, and guard fetchTracksCreditedTo against an empty artist-ID slice. Fix the artist mock to forward variadic options with Called(options...) so QueryOptions-shaped matchers receive the same argument shape as real calls. * docs(matcher): trim comments that restated the code Cut three doc comments down to their why: groupQueriesByArtist, own, and bucketTracks no longer restate what the signature and body already show. Condense fetchTracksCreditedTo's rationale from two paragraphs to one, keeping the role='artist', non-correlated-IN, and layer-boundary notes. --- core/external/provider_similarsongs_test.go | 34 +- core/external/provider_topsongs_test.go | 30 +- core/matcher/matcher.go | 238 ++++++++++---- core/matcher/matcher_internal_test.go | 14 + core/matcher/matcher_test.go | 337 ++++++++++++++++++-- 5 files changed, 536 insertions(+), 117 deletions(-) diff --git a/core/external/provider_similarsongs_test.go b/core/external/provider_similarsongs_test.go index c9a1a64ef..f109bf8b1 100644 --- a/core/external/provider_similarsongs_test.go +++ b/core/external/provider_similarsongs_test.go @@ -3,6 +3,7 @@ package external_test import ( "context" "errors" + "strings" "github.com/Masterminds/squirrel" "github.com/navidrome/navidrome/core/agents" @@ -56,7 +57,14 @@ var _ = Describe("Provider - SimilarSongs", func() { Context("when ID is a MediaFile (track)", func() { It("calls GetSimilarSongsByTrack and returns matched songs", func() { track := model.MediaFile{ID: "track-1", Title: "Just Can't Get Enough", Artist: "Depeche Mode", MbzRecordingID: "track-mbid"} - matchedSong := model.MediaFile{ID: "matched-1", Title: "Dreaming of Me", Artist: "Depeche Mode"} + + // Depeche Mode artist row used by matcher artist resolution and track-fetch back-mapping. + dmArtist := model.Artist{ID: "dm-1", Name: "Depeche Mode", OrderArtistName: "depeche mode", MbzArtistID: "artist-mbid"} + dmParticipant := model.Participant{Artist: dmArtist} + matchedSong := model.MediaFile{ + ID: "matched-1", Title: "Dreaming of Me", Artist: "Depeche Mode", + Participants: model.Participants{model.RoleArtist: model.ParticipantList{dmParticipant}}, + } // GetEntityByID tries Artist, Album, Playlist, then MediaFile artistRepo.On("Get", "track-1").Return(nil, model.ErrNotFound).Once() @@ -68,13 +76,16 @@ var _ = Describe("Provider - SimilarSongs", func() { {Name: "Dreaming of Me", MBID: "", Artist: "Depeche Mode", ArtistMBID: "artist-mbid"}, }, nil).Once() - // Mock loadTracksByID - no ID matches + // Matcher artist resolution: resolve Depeche Mode in the artist table. + artistRepo.On("GetAll", mock.Anything).Return(model.Artists{dmArtist}, nil).Maybe() + + // ID phase: no IDs → squirrel.And with media_file.id; won't be called but guard it. mediaFileRepo.On("GetAll", mock.MatchedBy(func(opt model.QueryOptions) bool { _, ok := opt.Filters.(squirrel.Eq) return ok - })).Return(model.MediaFiles{}, nil).Once() + })).Return(model.MediaFiles{}, nil).Maybe() - // Mock loadTracksByMBID - no MBID matches (empty MBID means this won't be called) + // MBID phase: won't fire (empty MBID). mediaFileRepo.On("GetAll", mock.MatchedBy(func(opt model.QueryOptions) bool { and, ok := opt.Filters.(squirrel.And) if !ok || len(and) < 1 { @@ -88,18 +99,19 @@ var _ = Describe("Provider - SimilarSongs", func() { return hasMBID })).Return(model.MediaFiles{}, nil).Maybe() - // Mock loadTracksByTitleAndArtist - queries by artist name + // Matcher track-fetch: subquery returns the matched song with participants. mediaFileRepo.On("GetAll", mock.MatchedBy(func(opt model.QueryOptions) bool { and, ok := opt.Filters.(squirrel.And) - if !ok || len(and) < 2 { + if !ok { return false } - eq, hasEq := and[0].(squirrel.Eq) - if !hasEq { - return false + for _, f := range and { + sql, _, err := f.ToSql() + if err == nil && strings.Contains(sql, "media_file_artists") { + return true + } } - _, hasArtist := eq["order_artist_name"] - return hasArtist + return false })).Return(model.MediaFiles{matchedSong}, nil).Maybe() songs, err := provider.SimilarSongs(ctx, "track-1", 5) diff --git a/core/external/provider_topsongs_test.go b/core/external/provider_topsongs_test.go index d9aff37e8..600524819 100644 --- a/core/external/provider_topsongs_test.go +++ b/core/external/provider_topsongs_test.go @@ -148,6 +148,8 @@ var _ = Describe("Provider - TopSongs", func() { // Mock finding the artist artist1 := model.Artist{ID: "artist-1", Name: "Artist One", MbzArtistID: "mbid-artist-1"} artistRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.Artists{artist1}, nil).Once() + // Matcher artist resolution for the title-match path (song2 falls through). + artistRepo.On("GetAll", mock.Anything).Return(model.Artists{artist1}, nil).Maybe() // Mock agent response agentSongs := []agents.Song{ @@ -159,7 +161,7 @@ var _ = Describe("Provider - TopSongs", func() { // Mock finding matching tracks (only find song 1 on bulk query) song1 := model.MediaFile{ID: "song-1", Title: "Song One", ArtistID: "artist-1", MbzRecordingID: "mbid-song-1"} mediaFileRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.MediaFiles{song1}, nil).Once() // bulk MBID query - mediaFileRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.MediaFiles{}, nil).Once() // title fallback for song2 + mediaFileRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.MediaFiles{}, nil).Once() // title track-fetch for song2: no match songs, err := p.TopSongs(ctx, "Artist One", 2) @@ -195,6 +197,8 @@ var _ = Describe("Provider - TopSongs", func() { // Mock finding the artist artist1 := model.Artist{ID: "artist-1", Name: "Artist One", MbzArtistID: "mbid-artist-1"} artistRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.Artists{artist1}, nil).Once() + // Matcher artist resolution for both title-fallback songs. + artistRepo.On("GetAll", mock.Anything).Return(model.Artists{artist1}, nil).Maybe() // Mock agent response with songs that have NO MBID (empty string) agentSongs := []agents.Song{ @@ -203,10 +207,16 @@ var _ = Describe("Provider - TopSongs", func() { } ag.On("GetArtistTopSongs", ctx, "artist-1", "Artist One", "mbid-artist-1", 2).Return(agentSongs, nil).Once() - // Since there are no MBIDs, loadTracksByMBID should not make any database call - // loadTracksByTitle should make a database call for title matching - song1 := model.MediaFile{ID: "song-1", Title: "Song One", Artist: "Artist One", ArtistID: "artist-1", MbzRecordingID: "", OrderTitle: "song one"} - song2 := model.MediaFile{ID: "song-2", Title: "Song Two", Artist: "Artist One", ArtistID: "artist-1", MbzRecordingID: "", OrderTitle: "song two"} + // Title track-fetch: tracks must carry RoleArtist participants so back-mapping routes them. + participant1 := model.Participant{Artist: model.Artist{ID: "artist-1", Name: "Artist One", OrderArtistName: "artist one"}} + song1 := model.MediaFile{ + ID: "song-1", Title: "Song One", Artist: "Artist One", ArtistID: "artist-1", + Participants: model.Participants{model.RoleArtist: model.ParticipantList{participant1}}, + } + song2 := model.MediaFile{ + ID: "song-2", Title: "Song Two", Artist: "Artist One", ArtistID: "artist-1", + Participants: model.Participants{model.RoleArtist: model.ParticipantList{participant1}}, + } mediaFileRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.MediaFiles{song1, song2}, nil).Once() songs, err := p.TopSongs(ctx, "Artist One", 2) @@ -224,6 +234,8 @@ var _ = Describe("Provider - TopSongs", func() { // Mock finding the artist artist1 := model.Artist{ID: "artist-1", Name: "Artist One", MbzArtistID: "mbid-artist-1"} artistRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.Artists{artist1}, nil).Once() + // Matcher artist resolution for song2's title-fallback path. + artistRepo.On("GetAll", mock.Anything).Return(model.Artists{artist1}, nil).Maybe() // Mock agent response with mixed MBID availability agentSongs := []agents.Song{ @@ -236,8 +248,12 @@ var _ = Describe("Provider - TopSongs", func() { song1 := model.MediaFile{ID: "song-1", Title: "Song One", ArtistID: "artist-1", MbzRecordingID: "mbid-song-1", OrderTitle: "song one"} mediaFileRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.MediaFiles{song1}, nil).Once() - // Mock the title fallback query (finds song2 by title) - song2 := model.MediaFile{ID: "song-2", Title: "Song Two", Artist: "Artist One", ArtistID: "artist-1", MbzRecordingID: "", OrderTitle: "song two"} + // Title track-fetch: song2 must carry RoleArtist participants for back-mapping. + participant1 := model.Participant{Artist: model.Artist{ID: "artist-1", Name: "Artist One", OrderArtistName: "artist one"}} + song2 := model.MediaFile{ + ID: "song-2", Title: "Song Two", Artist: "Artist One", ArtistID: "artist-1", + Participants: model.Participants{model.RoleArtist: model.ParticipantList{participant1}}, + } mediaFileRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.MediaFiles{song2}, nil).Once() songs, err := p.TopSongs(ctx, "Artist One", 2) diff --git a/core/matcher/matcher.go b/core/matcher/matcher.go index 8b52cbad2..5322b649f 100644 --- a/core/matcher/matcher.go +++ b/core/matcher/matcher.go @@ -3,13 +3,16 @@ package matcher import ( "context" "fmt" + "maps" "math" + "slices" "github.com/Masterminds/squirrel" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/core/agents" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/utils/slice" "github.com/navidrome/navidrome/utils/str" "github.com/xrash/smetrics" ) @@ -246,43 +249,40 @@ func (s matchScore) betterThan(other matchScore) bool { // when the same track is scored against multiple queries. The `mf` field is a pointer to avoid // copying the large MediaFile struct into each entry of the sanitized slice. type sanitizedTrack struct { - mf *model.MediaFile - title string - artist string - album string + mf *model.MediaFile + title string + artist string + album string + artistMBID string // resolved from the artist table; mf.MbzArtistID is not populated on the bulk path } -func newSanitizedTrack(mf *model.MediaFile) sanitizedTrack { +func newSanitizedTrack(mf *model.MediaFile, artistMBID string) sanitizedTrack { return sanitizedTrack{ - mf: mf, - title: str.SanitizeFieldForSorting(mf.Title), - artist: str.SanitizeFieldForSortingNoArticle(mf.Artist), - album: str.SanitizeFieldForSorting(mf.Album), + mf: mf, + title: str.SanitizeFieldForSorting(mf.Title), + artist: str.SanitizeFieldForSortingNoArticle(mf.Artist), + album: str.SanitizeFieldForSorting(mf.Album), + artistMBID: artistMBID, } } // computeSpecificityLevel determines how well query metadata matches a track (0-5). -// The track's title, artist, and album fields must be pre-sanitized. -// -// TODO: the artist-MBID levels (5, 4, 2) read the deprecated MediaFile.MbzArtistID -// column, which is not populated — the artist MBID lives in the artist table and is -// only hydrated by GetWithParticipants, not the bulk GetAll path used here. As a -// result those levels never fire. To make them work, hydrate the artist participant -// (or denormalize mbz_artist_id onto media_file) so t.mf carries the artist MBID. +// The track's title, artist, and album fields must be pre-sanitized, and artistMBID +// must hold the resolved artist MBID. func computeSpecificityLevel(q songQuery, t sanitizedTrack, albumThreshold float64) int { if q.artistMBID != "" && q.albumMBID != "" && - t.mf.MbzArtistID == q.artistMBID && t.mf.MbzAlbumID == q.albumMBID { + t.artistMBID == q.artistMBID && t.mf.MbzAlbumID == q.albumMBID { return 5 } if q.artistMBID != "" && q.album != "" && - t.mf.MbzArtistID == q.artistMBID && similarityRatio(t.album, q.album) >= albumThreshold { + t.artistMBID == q.artistMBID && similarityRatio(t.album, q.album) >= albumThreshold { return 4 } if q.artist != "" && q.album != "" && t.artist == q.artist && similarityRatio(t.album, q.album) >= albumThreshold { return 3 } - if q.artistMBID != "" && t.mf.MbzArtistID == q.artistMBID { + if q.artistMBID != "" && t.artistMBID == q.artistMBID { return 2 } if q.artist != "" && t.artist == q.artist { @@ -301,64 +301,25 @@ type indexedQuery struct { // matchByTitle fills result with fuzzy title+artist matches, skipping songs // already matched by a higher-priority loader. func (m *Matcher) matchByTitle(ctx context.Context, songs []agents.Song, result map[int]model.MediaFile) error { - byArtist := map[string][]indexedQuery{} - for i, s := range songs { - if _, done := result[i]; done { - continue - } - artist := str.SanitizeFieldForSortingNoArticle(s.Artist) - if artist == "" { - continue // title matching needs an artist to scope the library query - } - q := songQuery{ - title: str.SanitizeFieldForSorting(s.Name), - artist: artist, - artistMBID: s.ArtistMBID, - album: str.SanitizeFieldForSorting(s.Album), - albumMBID: s.AlbumMBID, - durationMs: s.Duration, - } - byArtist[artist] = append(byArtist[artist], indexedQuery{index: i, query: q}) - } + byArtist := groupQueriesByArtist(songs, result) if len(byArtist) == 0 { return nil } - // One batched query (order_artist_name IN ...) instead of one per artist: on a - // large library the per-query overhead dominates, so this is the main cost saver. - artists := make([]string, 0, len(byArtist)) - for artist := range byArtist { - artists = append(artists, artist) + resolved, err := m.resolveArtists(ctx, byArtist) + if err != nil || len(resolved.allIDs) == 0 { + return err } - tracks, err := m.ds.MediaFile(ctx).GetAll(model.QueryOptions{ - Filters: squirrel.And{ - squirrel.Eq{"order_artist_name": artists}, - squirrel.Eq{"missing": false}, - }, - Sort: "starred desc, rating desc, year asc, compilation asc", - }) + + tracks, err := m.fetchTracksCreditedTo(ctx, resolved.allIDs) if err != nil { return err } - // Key on order_artist_name — the exact field the query filtered on, which matches - // how byArtist is keyed. A track's display Artist can differ (collaborations, - // "feat." credits), so re-deriving from Artist would misbucket. This reads the - // deprecated MediaFile.OrderArtistName column because the bulk GetAll path does - // not hydrate participant detail (only {id, name}), so the participant's order - // name is empty here; the column is the only populated source. - tracksByArtist := make(map[string][]sanitizedTrack, len(byArtist)) - for i := range tracks { - key := tracks[i].OrderArtistName - if key == "" { - key = str.SanitizeFieldForSortingNoArticle(tracks[i].Artist) - } - tracksByArtist[key] = append(tracksByArtist[key], newSanitizedTrack(&tracks[i])) - } - + tracksByQuery := resolved.bucketTracks(tracks) threshold := float64(conf.Server.Matcher.FuzzyThreshold) / 100.0 for artist, queries := range byArtist { - sanitized := tracksByArtist[artist] + sanitized := tracksByQuery[artist] // Each song is matched independently by index, so two songs with the same // (title, artist) but different durations can resolve to different tracks. for _, iq := range queries { @@ -370,6 +331,151 @@ func (m *Matcher) matchByTitle(ctx context.Context, songs []agents.Song, result return nil } +// groupQueriesByArtist buckets the still-unmatched title queries by sanitized artist name. +// Songs without an artist are skipped: title matching needs one to scope the library query. +func groupQueriesByArtist(songs []agents.Song, result map[int]model.MediaFile) map[string][]indexedQuery { + byArtist := map[string][]indexedQuery{} + for i, s := range songs { + if _, done := result[i]; done { + continue + } + artist := str.SanitizeFieldForSortingNoArticle(s.Artist) + if artist == "" { + continue + } + byArtist[artist] = append(byArtist[artist], indexedQuery{index: i, query: songQuery{ + title: str.SanitizeFieldForSorting(s.Name), + artist: artist, + artistMBID: s.ArtistMBID, + album: str.SanitizeFieldForSorting(s.Album), + albumMBID: s.AlbumMBID, + durationMs: s.Duration, + }}) + } + return byArtist +} + +// resolvedArtists holds the agent artists resolved to artist-table rows. Everything routes by +// stable artist ID, never by name, so MBID-resolved artists whose order name differs from the +// query name are not misrouted. +type resolvedArtists struct { + byQuery map[string]map[string]struct{} // sanitized query name -> set of resolved artist IDs + mbid map[string]string // artist ID -> its MBID (the real one, from the artist table) + allIDs []string // every resolved artist ID, for the track lookup +} + +// resolveArtists resolves the queries' artists against the artist table (by sort name or +// agent-provided MBID) and records, for each query, which artist IDs it owns. +func (m *Matcher) resolveArtists(ctx context.Context, byArtist map[string][]indexedQuery) (resolvedArtists, error) { + names := make([]string, 0, len(byArtist)) + mbidToQueries := make(map[string][]string, len(byArtist)) // agent ArtistMBID -> query names that supplied it + for name, queries := range byArtist { + names = append(names, name) + for _, iq := range queries { + if iq.query.artistMBID != "" { + mbidToQueries[iq.query.artistMBID] = append(mbidToQueries[iq.query.artistMBID], name) + } + } + } + + filter := squirrel.Or{squirrel.Eq{"order_artist_name": names}} + if len(mbidToQueries) > 0 { + filter = append(filter, squirrel.Eq{"mbz_artist_id": slices.Collect(maps.Keys(mbidToQueries))}) + } + artists, err := m.ds.Artist(ctx).GetAll(model.QueryOptions{Filters: filter}) + if err != nil { + return resolvedArtists{}, err + } + + res := resolvedArtists{ + byQuery: make(map[string]map[string]struct{}, len(byArtist)), + mbid: make(map[string]string, len(artists)), + allIDs: make([]string, 0, len(artists)), + } + for _, a := range artists { + res.mbid[a.ID] = a.MbzArtistID + res.allIDs = append(res.allIDs, a.ID) + // An artist belongs to a query if its order name matches the query name, or if its MBID + // matches one a query supplied. The same MBID can come from several queries (agent aliases), + // so every one of them owns the artist. + res.own(a.OrderArtistName, a.ID) + if a.MbzArtistID != "" { + for _, name := range mbidToQueries[a.MbzArtistID] { + res.own(name, a.ID) + } + } + } + return res, nil +} + +// own records that the named query owns the given artist ID. A name that is not a query simply +// gets its own (unused) entry. +func (r resolvedArtists) own(name, artistID string) { + if r.byQuery[name] == nil { + r.byQuery[name] = map[string]struct{}{} + } + r.byQuery[name][artistID] = struct{}{} +} + +// bucketTracks groups tracks by query name, at most once per query even when a track credits +// several of that query's artists, so the same track is not scored twice. The participants JSON +// on each track carries artist IDs but not their MBID, so the MBID comes from r.mbid instead. +func (r resolvedArtists) bucketTracks(tracks []model.MediaFile) map[string][]sanitizedTrack { + // Invert byQuery once so each participant maps straight to the queries that own it, instead of + // scanning every query per participant. + queriesByArtist := make(map[string][]string) + for name, ids := range r.byQuery { + for id := range ids { + queriesByArtist[id] = append(queriesByArtist[id], name) + } + } + + byQuery := make(map[string][]sanitizedTrack, len(r.byQuery)) + added := make(map[string]map[string]struct{}, len(r.byQuery)) // query name -> set of track IDs already bucketed + for i := range tracks { + for _, p := range tracks[i].Participants[model.RoleArtist] { + mbid, isResolved := r.mbid[p.ID] + if !isResolved { + continue + } + for _, name := range queriesByArtist[p.ID] { + if added[name] == nil { + added[name] = map[string]struct{}{} + } + if _, dup := added[name][tracks[i].ID]; dup { + continue + } + added[name][tracks[i].ID] = struct{}{} + byQuery[name] = append(byQuery[name], newSanitizedTrack(&tracks[i], mbid)) + } + } + } + return byQuery +} + +// fetchTracksCreditedTo fetches every non-missing track credited to any of the given artists as +// the main artist (role='artist', not albumartist — that avoids tribute/compilation false +// positives). The non-correlated id IN (subquery) materializes the matching ids once from the +// media_file_artists(artist_id) covering index, far cheaper than a correlated EXISTS that re-runs +// per row. That form isn't expressible via the repository's role filters, so the raw squirrel.Expr +// keeps the media_file_artists schema knowledge here; a dedicated repository method would be the +// cleaner home if this is reused. +func (m *Matcher) fetchTracksCreditedTo(ctx context.Context, artistIDs []string) (model.MediaFiles, error) { + if len(artistIDs) == 0 { + return nil, nil + } + args := slice.Map(artistIDs, func(id string) any { return id }) + return m.ds.MediaFile(ctx).GetAll(model.QueryOptions{ + Filters: squirrel.And{ + squirrel.Expr( + "media_file.id IN (SELECT media_file_id FROM media_file_artists "+ + "WHERE role = 'artist' AND artist_id IN ("+squirrel.Placeholders(len(artistIDs))+"))", args...), + squirrel.Eq{"missing": false}, + }, + Sort: "starred desc, rating desc, year asc, compilation asc", + }) +} + // durationProximity returns a score from 0.0 to 1.0 indicating how close the track's duration // is to the target. Returns 1.0 if durationMs is 0 (unknown). func durationProximity(durationMs uint32, mediaFileDurationSec float32) float64 { diff --git a/core/matcher/matcher_internal_test.go b/core/matcher/matcher_internal_test.go index f111364c1..62bccd844 100644 --- a/core/matcher/matcher_internal_test.go +++ b/core/matcher/matcher_internal_test.go @@ -1,6 +1,7 @@ package matcher import ( + "github.com/navidrome/navidrome/model" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) @@ -51,3 +52,16 @@ var _ = Describe("similarityRatio", func() { Expect(ratio1).To(Equal(ratio2)) }) }) + +var _ = Describe("matcher internals", func() { + It("computeSpecificityLevel uses sanitizedTrack.artistMBID for artist-MBID levels", func() { + q := songQuery{ + title: "song", + artistMBID: "artist-mbid-1", + albumMBID: "album-mbid-1", + } + mf := model.MediaFile{Title: "Song", MbzAlbumID: "album-mbid-1"} // note: mf.MbzArtistID intentionally empty + t := newSanitizedTrack(&mf, "artist-mbid-1") // resolved MBID supplied here + Expect(computeSpecificityLevel(q, t, 0.85)).To(Equal(5)) + }) +}) diff --git a/core/matcher/matcher_test.go b/core/matcher/matcher_test.go index 0d08897d1..1fad0ebcc 100644 --- a/core/matcher/matcher_test.go +++ b/core/matcher/matcher_test.go @@ -3,6 +3,7 @@ package matcher_test import ( "context" "errors" + "strings" "github.com/Masterminds/squirrel" "github.com/navidrome/navidrome/conf" @@ -19,6 +20,7 @@ import ( var _ = Describe("Matcher", func() { var ds model.DataStore var mediaFileRepo *mockMediaFileRepo + var artistRepo *mockArtistRepo var ctx context.Context var m *matcher.Matcher @@ -26,11 +28,13 @@ var _ = Describe("Matcher", func() { ctx = GinkgoT().Context() DeferCleanup(configtest.SetupConfig()) mediaFileRepo = newMockMediaFileRepo() + artistRepo = newMockArtistRepo() DeferCleanup(func() { mediaFileRepo.AssertExpectations(GinkgoT()) }) ds = &tests.MockDataStore{ MockedMediaFile: mediaFileRepo, + MockedArtist: artistRepo, } m = matcher.New(ds) }) @@ -69,16 +73,33 @@ var _ = Describe("Matcher", func() { // this after expect*Phase for the phases the test actually wants to verify. allowOtherPhases := func() { allowIdentifierPhases() - mediaFileRepo.On("GetAll", mock.MatchedBy(matchFieldInAnd("order_artist_name"))). + artistRepo.On("GetAll", mock.Anything).Return(model.Artists{}, nil).Maybe() + mediaFileRepo.On("GetAll", mock.MatchedBy(matchTracksByArtistQuery())). Return(model.MediaFiles{}, nil).Maybe() } - // allowTitlePhase is a convenience for fuzzy-match tests that only exercise the - // title+artist phase. It uses .Maybe() because the phase may short-circuit when no - // songs have an artist. - allowTitlePhase := func(artistTracks model.MediaFiles) { - mediaFileRepo.On("GetAll", mock.MatchedBy(matchFieldInAnd("order_artist_name"))). - Return(artistTracks, nil).Maybe() + // allowTitlePhase wires title matching from a list of library tracks. Each track must carry + // Participants[RoleArtist] with the artist IDs that credit it; the helper derives the artist + // rows the artist resolution returns from those participants, then returns the tracks from + // the track-fetch query. + allowTitlePhase := func(tracks model.MediaFiles) { + // Artist resolution: build artist rows from the tracks' participants. + seen := map[string]model.Artist{} + for _, t := range tracks { + for _, p := range t.Participants[model.RoleArtist] { + if _, ok := seen[p.ID]; !ok { + seen[p.ID] = p.Artist + } + } + } + artists := make(model.Artists, 0, len(seen)) + for _, a := range seen { + artists = append(artists, a) + } + artistRepo.On("GetAll", mock.Anything).Return(artists, nil).Maybe() + // Track fetch (media_file_artists subquery). + mediaFileRepo.On("GetAll", mock.MatchedBy(matchTracksByArtistQuery())). + Return(tracks, nil).Maybe() } Describe("MatchSongs", func() { @@ -146,6 +167,7 @@ var _ = Describe("Matcher", func() { } titleMatch := model.MediaFile{ ID: "track-title", Title: "Enjoy the Silence", Artist: "Depeche Mode", + Participants: artistParticipants(model.Artist{ID: "dm", Name: "Depeche Mode", OrderArtistName: "depeche mode"}), } allowTitlePhase(model.MediaFiles{titleMatch}) result, err := m.MatchSongs(ctx, songs, 5) @@ -161,6 +183,7 @@ var _ = Describe("Matcher", func() { } fuzzyMatch := model.MediaFile{ ID: "track-fuzzy", Title: "Bohemian Rhapsody (Live)", Artist: "Queen", + Participants: artistParticipants(model.Artist{ID: "queen", Name: "Queen", OrderArtistName: "queen"}), } allowTitlePhase(model.MediaFiles{fuzzyMatch}) result, err := m.MatchSongs(ctx, songs, 5) @@ -175,7 +198,9 @@ var _ = Describe("Matcher", func() { {Name: "Yesterday", Artist: "The Beatles"}, } differentTracks := model.MediaFiles{ - {ID: "different", Title: "Tomorrow Never Knows", Artist: "The Beatles"}, + {ID: "different", Title: "Tomorrow Never Knows", Artist: "The Beatles", + Participants: artistParticipants(model.Artist{ID: "beatles", Name: "The Beatles", OrderArtistName: "beatles"}), + }, } allowTitlePhase(differentTracks) result, err := m.MatchSongs(ctx, songs, 5) @@ -193,6 +218,7 @@ var _ = Describe("Matcher", func() { } libraryTrack := model.MediaFile{ ID: "br-live", Title: "Bohemian Rhapsody (Live)", Artist: "Queen", + Participants: artistParticipants(model.Artist{ID: "queen", Name: "Queen", OrderArtistName: "queen"}), } allowTitlePhase(model.MediaFiles{libraryTrack}) result, err := m.MatchSongs(ctx, songs, 5) @@ -209,6 +235,7 @@ var _ = Describe("Matcher", func() { } libraryTrack := model.MediaFile{ ID: "br", Title: "Bohemian Rhapsody", Artist: "Queen", Album: "A Night at the Opera", + Participants: artistParticipants(model.Artist{ID: "queen", Name: "Queen", OrderArtistName: "queen"}), } allowTitlePhase(model.MediaFiles{libraryTrack}) result, err := m.MatchSongs(ctx, songs, 5) @@ -249,9 +276,15 @@ var _ = Describe("Matcher", func() { {Name: "Song C", Artist: "Artist"}, } tracks := model.MediaFiles{ - {ID: "a", Title: "Song A", Artist: "Artist"}, - {ID: "b", Title: "Song B", Artist: "Artist"}, - {ID: "c", Title: "Song C", Artist: "Artist"}, + {ID: "a", Title: "Song A", Artist: "Artist", + Participants: artistParticipants(model.Artist{ID: "art", Name: "Artist", OrderArtistName: "artist"}), + }, + {ID: "b", Title: "Song B", Artist: "Artist", + Participants: artistParticipants(model.Artist{ID: "art", Name: "Artist", OrderArtistName: "artist"}), + }, + {ID: "c", Title: "Song C", Artist: "Artist", + Participants: artistParticipants(model.Artist{ID: "art", Name: "Artist", OrderArtistName: "artist"}), + }, } allowTitlePhase(tracks) result, err := m.MatchSongs(ctx, songs, 2) @@ -269,15 +302,19 @@ var _ = Describe("Matcher", func() { }) Context("artist grouping", func() { - It("groups title-phase tracks by order_artist_name, not display Artist", func() { + It("groups title-phase tracks by participant artist ID, not display Artist", func() { songs := []agents.Song{ {Name: "Song A", Artist: "Daft Punk"}, } - // Display Artist differs from the query artist; only OrderArtistName - // matches, so grouping must key on it (a "feat." credit, collaboration, etc.). + // Display Artist differs from the query artist; only the participant + // with order_artist_name "daft punk" routes to this query bucket. track := model.MediaFile{ ID: "oan-track", Title: "Song A", - Artist: "Daft Punk feat. Pharrell", OrderArtistName: "daft punk", + Artist: "Daft Punk feat. Pharrell", + Participants: artistParticipants( + model.Artist{ID: "dp", Name: "Daft Punk", OrderArtistName: "daft punk"}, + model.Artist{ID: "ph", Name: "Pharrell", OrderArtistName: "pharrell"}, + ), } allowTitlePhase(model.MediaFiles{track}) @@ -286,9 +323,105 @@ var _ = Describe("Matcher", func() { Expect(result).To(HaveLen(1)) Expect(result[0].ID).To(Equal("oan-track")) }) + + It("matches a track that credits the searched artist as a collaborator", func() { + songs := []agents.Song{ + {Name: "Crazy", Artist: "INXS"}, + } + // "Par-T-One vs. INXS" — display Artist is the collaboration, but INXS is a + // credited artist participant. Searching INXS must match it. + track := model.MediaFile{ + ID: "collab", Title: "Crazy", Artist: "Par-T-One vs. INXS", + Participants: artistParticipants( + model.Artist{ID: "a-partone", Name: "Par-T-One", OrderArtistName: "par-t-one"}, + model.Artist{ID: "a-inxs", Name: "INXS", OrderArtistName: "inxs"}, + ), + } + allowTitlePhase(model.MediaFiles{track}) + + result, err := m.MatchSongs(ctx, songs, 5) + Expect(err).ToNot(HaveOccurred()) + Expect(result).To(HaveLen(1)) + Expect(result[0].ID).To(Equal("collab")) + }) + + It("does not match a track where the searched artist is only the album artist", func() { + songs := []agents.Song{ + {Name: "Qmart", Artist: "808 State"}, + } + // Track performed by Björk on an "808 State" compilation: 808 State is the + // albumartist, Björk is the performer. Searching 808 State must NOT match it. + track := model.MediaFile{ + ID: "comp", Title: "Qmart", Artist: "Björk", + Participants: model.Participants{ + model.RoleArtist: model.ParticipantList{ + {Artist: model.Artist{ID: "a-bjork", Name: "Björk", OrderArtistName: "bjork"}}, + }, + model.RoleAlbumArtist: model.ParticipantList{ + {Artist: model.Artist{ID: "a-808", Name: "808 State", OrderArtistName: "808 state"}}, + }, + }, + } + // Artist resolution returns "808 state" only if some artist row matches; here the + // album-artist participant exists but is NOT role='artist', so the track-fetch query's + // EXISTS (role='artist') would not return the track in production. The mock + // returns it anyway; back-mapping must drop it because no role='artist' + // participant is a resolved artist for the query "808 state". + artistRepo.On("GetAll", mock.Anything). + Return(model.Artists{{ID: "a-808", Name: "808 State", OrderArtistName: "808 state"}}, nil).Maybe() + mediaFileRepo.On("GetAll", mock.MatchedBy(matchTracksByArtistQuery())). + Return(model.MediaFiles{track}, nil).Maybe() + + result, err := m.MatchSongs(ctx, songs, 5) + Expect(err).ToNot(HaveOccurred()) + Expect(result).To(BeEmpty()) + }) + + It("resolves the artist by ArtistMBID when the name differs", func() { + songs := []agents.Song{ + {Name: "Song A", Artist: "Typo Artist", ArtistMBID: "mbid-9"}, + } + track := model.MediaFile{ + ID: "by-mbid", Title: "Song A", Artist: "Correct Artist", + Participants: artistParticipants(model.Artist{ID: "a9", Name: "Correct Artist", OrderArtistName: "correct artist", MbzArtistID: "mbid-9"}), + } + // Artist resolution returns the artist matched by mbz_artist_id; its order name + // ("correct artist") differs from the query name ("typo artist"), so + // resolution must come from the MBID branch. + artistRepo.On("GetAll", mock.Anything). + Return(model.Artists{{ID: "a9", Name: "Correct Artist", OrderArtistName: "correct artist", MbzArtistID: "mbid-9"}}, nil).Maybe() + mediaFileRepo.On("GetAll", mock.MatchedBy(matchTracksByArtistQuery())). + Return(model.MediaFiles{track}, nil).Maybe() + + result, err := m.MatchSongs(ctx, songs, 5) + Expect(err).ToNot(HaveOccurred()) + Expect(result).To(HaveLen(1)) + Expect(result[0].ID).To(Equal("by-mbid")) + }) + + It("resolves both queries when two share one ArtistMBID under different names", func() { + // Two agent results for the same MusicBrainz artist but spelled differently + // (an alias). Both must match the artist's track via the shared MBID. + songs := []agents.Song{ + {Name: "Song A", Artist: "Alias One", ArtistMBID: "mbid-shared"}, + {Name: "Song B", Artist: "Alias Two", ArtistMBID: "mbid-shared"}, + } + artist := model.Artist{ID: "a-shared", Name: "Canonical", OrderArtistName: "canonical", MbzArtistID: "mbid-shared"} + trackA := model.MediaFile{ID: "ta", Title: "Song A", Artist: "Canonical", Participants: artistParticipants(artist)} + trackB := model.MediaFile{ID: "tb", Title: "Song B", Artist: "Canonical", Participants: artistParticipants(artist)} + artistRepo.On("GetAll", mock.Anything). + Return(model.Artists{{ID: "a-shared", Name: "Canonical", OrderArtistName: "canonical", MbzArtistID: "mbid-shared"}}, nil).Maybe() + mediaFileRepo.On("GetAll", mock.MatchedBy(matchTracksByArtistQuery())). + Return(model.MediaFiles{trackA, trackB}, nil).Maybe() + + result, err := m.MatchSongs(ctx, songs, 5) + Expect(err).ToNot(HaveOccurred()) + Expect(result).To(HaveLen(2)) + Expect([]string{result[0].ID, result[1].ID}).To(ConsistOf("ta", "tb")) + }) }) - // These tests register their own order_artist_name expectation per-test (to inject + // These tests register their own track-fetch expectations per-test (to inject // an error), so they use allowIdentifierPhases — NOT allowOtherPhases, which would // add a .Maybe() title-phase catch-all that masks the injected error. Context("title phase DB errors", func() { @@ -298,7 +431,11 @@ var _ = Describe("Matcher", func() { {Name: "Song B", Artist: "Artist Two"}, } allowIdentifierPhases() - mediaFileRepo.On("GetAll", mock.MatchedBy(matchFieldInAnd("order_artist_name"))). + artistRepo.On("GetAll", mock.Anything).Return(model.Artists{ + {ID: "a1", Name: "Artist One", OrderArtistName: "artist one"}, + {ID: "a2", Name: "Artist Two", OrderArtistName: "artist two"}, + }, nil) + mediaFileRepo.On("GetAll", mock.MatchedBy(matchTracksByArtistQuery())). Return(nil, errors.New("db down")) _, err := m.MatchSongs(ctx, songs, 5) @@ -317,7 +454,10 @@ var _ = Describe("Matcher", func() { Return(model.MediaFiles{}, nil).Maybe() mediaFileRepo.On("GetAll", mock.MatchedBy(matchFieldInEq("missing"))). Return(model.MediaFiles{}, nil).Maybe() - mediaFileRepo.On("GetAll", mock.MatchedBy(matchFieldInAnd("order_artist_name"))). + artistRepo.On("GetAll", mock.Anything).Return(model.Artists{ + {ID: "fa", Name: "Fuzzy Artist", OrderArtistName: "fuzzy artist"}, + }, nil) + mediaFileRepo.On("GetAll", mock.MatchedBy(matchTracksByArtistQuery())). Return(nil, errors.New("db down")) result, err := m.MatchSongs(ctx, songs, 5) @@ -384,10 +524,12 @@ var _ = Describe("Matcher", func() { correctMatch := model.MediaFile{ ID: "correct-match", Title: "Similar Song", Artist: "Depeche Mode", Album: "Violator", MbzArtistID: "artist-mbid-123", MbzAlbumID: "album-mbid-456", + Participants: artistParticipants(model.Artist{ID: "dm", Name: "Depeche Mode", OrderArtistName: "depeche mode", MbzArtistID: "artist-mbid-123"}), } wrongMatch := model.MediaFile{ ID: "wrong-match", Title: "Similar Song", Artist: "Depeche Mode", Album: "Some Other Album", MbzArtistID: "artist-mbid-123", MbzAlbumID: "different-album-mbid", + Participants: artistParticipants(model.Artist{ID: "dm", Name: "Depeche Mode", OrderArtistName: "depeche mode", MbzArtistID: "artist-mbid-123"}), } songs := []agents.Song{ {Name: "Similar Song", Artist: "Depeche Mode", ArtistMBID: "artist-mbid-123", Album: "Violator", AlbumMBID: "album-mbid-456"}, @@ -405,9 +547,11 @@ var _ = Describe("Matcher", func() { It("matches by title + artist name + album name when MBIDs unavailable", func() { correctMatch := model.MediaFile{ ID: "correct-match", Title: "Similar Song", Artist: "depeche mode", Album: "violator", + Participants: artistParticipants(model.Artist{ID: "dm", Name: "Depeche Mode", OrderArtistName: "depeche mode"}), } wrongMatch := model.MediaFile{ ID: "wrong-match", Title: "Similar Song", Artist: "Other Artist", Album: "Other Album", + Participants: artistParticipants(model.Artist{ID: "oa", Name: "Other Artist", OrderArtistName: "other artist"}), } songs := []agents.Song{ {Name: "Similar Song", Artist: "Depeche Mode", Album: "Violator"}, @@ -425,9 +569,11 @@ var _ = Describe("Matcher", func() { It("matches by title + artist only when album info unavailable", func() { correctMatch := model.MediaFile{ ID: "correct-match", Title: "Similar Song", Artist: "depeche mode", Album: "Some Album", + Participants: artistParticipants(model.Artist{ID: "dm", Name: "Depeche Mode", OrderArtistName: "depeche mode"}), } wrongMatch := model.MediaFile{ ID: "wrong-match", Title: "Similar Song", Artist: "Other Artist", Album: "Other Album", + Participants: artistParticipants(model.Artist{ID: "oa", Name: "Other Artist", OrderArtistName: "other artist"}), } songs := []agents.Song{ {Name: "Similar Song", Artist: "Depeche Mode"}, @@ -456,9 +602,15 @@ var _ = Describe("Matcher", func() { }) It("returns distinct matches for each artist's version (covers scenario)", func() { - cover1 := model.MediaFile{ID: "cover-1", Title: "Yesterday", Artist: "The Beatles", Album: "Help!"} - cover2 := model.MediaFile{ID: "cover-2", Title: "Yesterday", Artist: "Ray Charles", Album: "Greatest Hits"} - cover3 := model.MediaFile{ID: "cover-3", Title: "Yesterday", Artist: "Frank Sinatra", Album: "My Way"} + cover1 := model.MediaFile{ID: "cover-1", Title: "Yesterday", Artist: "The Beatles", Album: "Help!", + Participants: artistParticipants(model.Artist{ID: "beatles", Name: "The Beatles", OrderArtistName: "beatles"}), + } + cover2 := model.MediaFile{ID: "cover-2", Title: "Yesterday", Artist: "Ray Charles", Album: "Greatest Hits", + Participants: artistParticipants(model.Artist{ID: "ray-charles", Name: "Ray Charles", OrderArtistName: "ray charles"}), + } + cover3 := model.MediaFile{ID: "cover-3", Title: "Yesterday", Artist: "Frank Sinatra", Album: "My Way", + Participants: artistParticipants(model.Artist{ID: "sinatra", Name: "Frank Sinatra", OrderArtistName: "frank sinatra"}), + } songs := []agents.Song{ {Name: "Yesterday", Artist: "The Beatles", Album: "Help!"}, @@ -480,13 +632,16 @@ var _ = Describe("Matcher", func() { preciseMatch := model.MediaFile{ ID: "precise", Title: "Song A", Artist: "Artist One", Album: "Album One", MbzArtistID: "mbid-1", MbzAlbumID: "album-mbid-1", + Participants: artistParticipants(model.Artist{ID: "a1", Name: "Artist One", OrderArtistName: "artist one", MbzArtistID: "mbid-1"}), } lessAccurateMatch := model.MediaFile{ ID: "less-accurate", Title: "Song A", Artist: "Artist One", Album: "Compilation", - MbzArtistID: "mbid-1", + MbzArtistID: "mbid-1", + Participants: artistParticipants(model.Artist{ID: "a1", Name: "Artist One", OrderArtistName: "artist one", MbzArtistID: "mbid-1"}), } artistTwoMatch := model.MediaFile{ ID: "artist-two", Title: "Song B", Artist: "Artist Two", + Participants: artistParticipants(model.Artist{ID: "a2", Name: "Artist Two", OrderArtistName: "artist two"}), } songs := []agents.Song{ @@ -503,6 +658,31 @@ var _ = Describe("Matcher", func() { Expect(result[0].ID).To(Equal("precise")) Expect(result[1].ID).To(Equal("artist-two")) }) + + It("uses the resolved artist MBID for specificity (level 5)", func() { + songs := []agents.Song{ + {Name: "Song A", Artist: "Artist One", ArtistMBID: "mbid-1", Album: "Album One", AlbumMBID: "album-mbid-1"}, + } + // Two tracks with the same title and album; only the one whose resolved artist + // carries mbid-1 (and whose album MBID matches) wins via Level 5. Without the + // resolved MBID, both tracks tie at Level 3 (name+album) and the first wins by + // chance — verifiable by RED-proof: see task-2-report.md. + precise := model.MediaFile{ + ID: "precise", Title: "Song A", Artist: "Artist One", Album: "Album One", MbzAlbumID: "album-mbid-1", + Participants: artistParticipants(model.Artist{ID: "a1", Name: "Artist One", OrderArtistName: "artist one", MbzArtistID: "mbid-1"}), + } + other := model.MediaFile{ + ID: "other", Title: "Song A", Artist: "Artist One", Album: "Album One", MbzAlbumID: "wrong-album-mbid", + Participants: artistParticipants(model.Artist{ID: "a1b", Name: "Artist One", OrderArtistName: "artist one", MbzArtistID: ""}), + } + // Artist resolution returns both a1 (by name+mbid) and a1b (by name). + allowTitlePhase(model.MediaFiles{other, precise}) + + result, err := m.MatchSongs(ctx, songs, 5) + Expect(err).ToNot(HaveOccurred()) + Expect(result).To(HaveLen(1)) + Expect(result[0].ID).To(Equal("precise")) + }) }) Describe("fuzzy matching thresholds", func() { @@ -514,7 +694,9 @@ var _ = Describe("Matcher", func() { {Name: "Paranoid Android", Artist: "Radiohead"}, } artistTracks := model.MediaFiles{ - {ID: "remastered", Title: "Paranoid Android - Remastered", Artist: "Radiohead"}, + {ID: "remastered", Title: "Paranoid Android - Remastered", Artist: "Radiohead", + Participants: artistParticipants(model.Artist{ID: "rh", Name: "Radiohead", OrderArtistName: "radiohead"}), + }, } allowTitlePhase(artistTracks) @@ -533,7 +715,9 @@ var _ = Describe("Matcher", func() { {Name: "Bohemian Rhapsody", Artist: "Queen"}, } artistTracks := model.MediaFiles{ - {ID: "live", Title: "Bohemian Rhapsody (Live)", Artist: "Queen"}, + {ID: "live", Title: "Bohemian Rhapsody (Live)", Artist: "Queen", + Participants: artistParticipants(model.Artist{ID: "queen", Name: "Queen", OrderArtistName: "queen"}), + }, } allowTitlePhase(artistTracks) @@ -554,7 +738,9 @@ var _ = Describe("Matcher", func() { {Name: "Paranoid Android", Artist: "Radiohead"}, } artistTracks := model.MediaFiles{ - {ID: "remastered", Title: "Paranoid Android - Remastered", Artist: "Radiohead"}, + {ID: "remastered", Title: "Paranoid Android - Remastered", Artist: "Radiohead", + Participants: artistParticipants(model.Artist{ID: "rh", Name: "Radiohead", OrderArtistName: "radiohead"}), + }, } allowTitlePhase(artistTracks) @@ -574,7 +760,9 @@ var _ = Describe("Matcher", func() { {Name: "Song", Artist: "Artist"}, } artistTracks := model.MediaFiles{ - {ID: "extended", Title: "Song (Extended Mix)", Artist: "Artist"}, + {ID: "extended", Title: "Song (Extended Mix)", Artist: "Artist", + Participants: artistParticipants(model.Artist{ID: "art", Name: "Artist", OrderArtistName: "artist"}), + }, } allowTitlePhase(artistTracks) @@ -600,9 +788,11 @@ var _ = Describe("Matcher", func() { } correctMatch := model.MediaFile{ ID: "correct", Title: "Bohemian Rhapsody", Artist: "Queen", Album: "A Night at the Opera (2011 Remaster)", + Participants: artistParticipants(model.Artist{ID: "queen", Name: "Queen", OrderArtistName: "queen"}), } wrongMatch := model.MediaFile{ ID: "wrong", Title: "Bohemian Rhapsody", Artist: "Queen", Album: "Greatest Hits", + Participants: artistParticipants(model.Artist{ID: "queen", Name: "Queen", OrderArtistName: "queen"}), } allowTitlePhase(model.MediaFiles{wrongMatch, correctMatch}) @@ -620,9 +810,11 @@ var _ = Describe("Matcher", func() { } correctMatch := model.MediaFile{ ID: "correct", Title: "Enjoy the Silence", Artist: "Depeche Mode", Album: "Violator (Deluxe Edition)", + Participants: artistParticipants(model.Artist{ID: "dm", Name: "Depeche Mode", OrderArtistName: "depeche mode"}), } wrongMatch := model.MediaFile{ ID: "wrong", Title: "Enjoy the Silence", Artist: "Depeche Mode", Album: "101", + Participants: artistParticipants(model.Artist{ID: "dm", Name: "Depeche Mode", OrderArtistName: "depeche mode"}), } allowTitlePhase(model.MediaFiles{wrongMatch, correctMatch}) @@ -640,9 +832,11 @@ var _ = Describe("Matcher", func() { } exactMatch := model.MediaFile{ ID: "exact", Title: "Enjoy the Silence", Artist: "Depeche Mode", Album: "Violator", + Participants: artistParticipants(model.Artist{ID: "dm", Name: "Depeche Mode", OrderArtistName: "depeche mode"}), } fuzzyMatch := model.MediaFile{ ID: "fuzzy", Title: "Enjoy the Silence", Artist: "Depeche Mode", Album: "Violator (Deluxe Edition)", + Participants: artistParticipants(model.Artist{ID: "dm", Name: "Depeche Mode", OrderArtistName: "depeche mode"}), } allowTitlePhase(model.MediaFiles{fuzzyMatch, exactMatch}) @@ -661,9 +855,12 @@ var _ = Describe("Matcher", func() { } albumMatch := model.MediaFile{ ID: "album-match", Title: "Enjoy the Silence", Artist: "Depeche Mode", Album: "Violator", + Participants: artistParticipants(model.Artist{ID: "dm", Name: "Depeche Mode", OrderArtistName: "depeche mode"}), } starredTrack := model.MediaFile{ - ID: "starred", Title: "Enjoy the Silence", Artist: "Depeche Mode", Album: "Singles", Annotations: model.Annotations{Starred: true}, + ID: "starred", Title: "Enjoy the Silence", Artist: "Depeche Mode", Album: "Singles", + Annotations: model.Annotations{Starred: true}, + Participants: artistParticipants(model.Artist{ID: "dm", Name: "Depeche Mode", OrderArtistName: "depeche mode"}), } allowTitlePhase(model.MediaFiles{albumMatch, starredTrack}) @@ -682,9 +879,12 @@ var _ = Describe("Matcher", func() { } albumMatch := model.MediaFile{ ID: "album-match", Title: "Enjoy the Silence", Artist: "Depeche Mode", Album: "Violator", + Participants: artistParticipants(model.Artist{ID: "dm", Name: "Depeche Mode", OrderArtistName: "depeche mode"}), } ratedTrack := model.MediaFile{ - ID: "rated", Title: "Enjoy the Silence", Artist: "Depeche Mode", Album: "Singles", Annotations: model.Annotations{Rating: 4}, + ID: "rated", Title: "Enjoy the Silence", Artist: "Depeche Mode", Album: "Singles", + Annotations: model.Annotations{Rating: 4}, + Participants: artistParticipants(model.Artist{ID: "dm", Name: "Depeche Mode", OrderArtistName: "depeche mode"}), } allowTitlePhase(model.MediaFiles{albumMatch, ratedTrack}) @@ -708,9 +908,11 @@ var _ = Describe("Matcher", func() { } correctMatch := model.MediaFile{ ID: "correct", Title: "Similar Song", Artist: "Test Artist", Duration: 180.0, + Participants: artistParticipants(model.Artist{ID: "ta", Name: "Test Artist", OrderArtistName: "test artist"}), } wrongDuration := model.MediaFile{ ID: "wrong", Title: "Similar Song", Artist: "Test Artist", Duration: 240.0, + Participants: artistParticipants(model.Artist{ID: "ta", Name: "Test Artist", OrderArtistName: "test artist"}), } allowTitlePhase(model.MediaFiles{wrongDuration, correctMatch}) @@ -728,6 +930,7 @@ var _ = Describe("Matcher", func() { } closeDuration := model.MediaFile{ ID: "close-duration", Title: "Similar Song", Artist: "Test Artist", Duration: 182.5, + Participants: artistParticipants(model.Artist{ID: "ta", Name: "Test Artist", OrderArtistName: "test artist"}), } allowTitlePhase(model.MediaFiles{closeDuration}) @@ -745,9 +948,11 @@ var _ = Describe("Matcher", func() { } closeDuration := model.MediaFile{ ID: "close", Title: "Similar Song", Artist: "Test Artist", Duration: 181.0, + Participants: artistParticipants(model.Artist{ID: "ta", Name: "Test Artist", OrderArtistName: "test artist"}), } farDuration := model.MediaFile{ ID: "far", Title: "Similar Song", Artist: "Test Artist", Duration: 190.0, + Participants: artistParticipants(model.Artist{ID: "ta", Name: "Test Artist", OrderArtistName: "test artist"}), } allowTitlePhase(model.MediaFiles{farDuration, closeDuration}) @@ -765,6 +970,7 @@ var _ = Describe("Matcher", func() { } differentDuration := model.MediaFile{ ID: "different", Title: "Similar Song", Artist: "Test Artist", Duration: 300.0, + Participants: artistParticipants(model.Artist{ID: "ta", Name: "Test Artist", OrderArtistName: "test artist"}), } allowTitlePhase(model.MediaFiles{differentDuration}) @@ -782,9 +988,11 @@ var _ = Describe("Matcher", func() { } differentTitle := model.MediaFile{ ID: "wrong-title", Title: "Different Song", Artist: "Test Artist", Duration: 180.0, + Participants: artistParticipants(model.Artist{ID: "ta", Name: "Test Artist", OrderArtistName: "test artist"}), } correctTitle := model.MediaFile{ ID: "correct-title", Title: "Similar Song", Artist: "Test Artist", Duration: 300.0, + Participants: artistParticipants(model.Artist{ID: "ta", Name: "Test Artist", OrderArtistName: "test artist"}), } allowTitlePhase(model.MediaFiles{differentTitle, correctTitle}) @@ -802,6 +1010,7 @@ var _ = Describe("Matcher", func() { } anyTrack := model.MediaFile{ ID: "any", Title: "Similar Song", Artist: "Test Artist", Duration: 999.0, + Participants: artistParticipants(model.Artist{ID: "ta", Name: "Test Artist", OrderArtistName: "test artist"}), } allowTitlePhase(model.MediaFiles{anyTrack}) @@ -819,6 +1028,7 @@ var _ = Describe("Matcher", func() { } shortTrack := model.MediaFile{ ID: "short", Title: "Short Song", Artist: "Test Artist", Duration: 31.0, + Participants: artistParticipants(model.Artist{ID: "ta", Name: "Test Artist", OrderArtistName: "test artist"}), } allowTitlePhase(model.MediaFiles{shortTrack}) @@ -837,9 +1047,11 @@ var _ = Describe("Matcher", func() { } shortTrack := model.MediaFile{ ID: "short", Title: "Same Song", Artist: "Same Artist", Duration: 180.0, + Participants: artistParticipants(model.Artist{ID: "sa", Name: "Same Artist", OrderArtistName: "same artist"}), } longTrack := model.MediaFile{ ID: "long", Title: "Same Song", Artist: "Same Artist", Duration: 240.0, + Participants: artistParticipants(model.Artist{ID: "sa", Name: "Same Artist", OrderArtistName: "same artist"}), } allowTitlePhase(model.MediaFiles{shortTrack, longTrack}) @@ -867,6 +1079,7 @@ var _ = Describe("Matcher", func() { } libraryTrack := model.MediaFile{ ID: "yesterday", Title: "Yesterday", Artist: "The Beatles", Album: "Help!", + Participants: artistParticipants(model.Artist{ID: "beatles", Name: "The Beatles", OrderArtistName: "beatles"}), } allowTitlePhase(model.MediaFiles{libraryTrack}) @@ -885,9 +1098,15 @@ var _ = Describe("Matcher", func() { {Name: "Song B", Artist: "Artist"}, {Name: "Song C", Artist: "Artist"}, } - trackA := model.MediaFile{ID: "track-a", Title: "Song A", Artist: "Artist"} - trackB := model.MediaFile{ID: "track-b", Title: "Song B", Artist: "Artist"} - trackC := model.MediaFile{ID: "track-c", Title: "Song C", Artist: "Artist"} + trackA := model.MediaFile{ID: "track-a", Title: "Song A", Artist: "Artist", + Participants: artistParticipants(model.Artist{ID: "art", Name: "Artist", OrderArtistName: "artist"}), + } + trackB := model.MediaFile{ID: "track-b", Title: "Song B", Artist: "Artist", + Participants: artistParticipants(model.Artist{ID: "art", Name: "Artist", OrderArtistName: "artist"}), + } + trackC := model.MediaFile{ID: "track-c", Title: "Song C", Artist: "Artist", + Participants: artistParticipants(model.Artist{ID: "art", Name: "Artist", OrderArtistName: "artist"}), + } allowTitlePhase(model.MediaFiles{trackA, trackB, trackC}) @@ -907,8 +1126,12 @@ var _ = Describe("Matcher", func() { {Name: "Song B", Artist: "Artist"}, {Name: "Song B (Remix)", Artist: "Artist"}, } - trackA := model.MediaFile{ID: "track-a", Title: "Song A", Artist: "Artist"} - trackB := model.MediaFile{ID: "track-b", Title: "Song B", Artist: "Artist"} + trackA := model.MediaFile{ID: "track-a", Title: "Song A", Artist: "Artist", + Participants: artistParticipants(model.Artist{ID: "art", Name: "Artist", OrderArtistName: "artist"}), + } + trackB := model.MediaFile{ID: "track-b", Title: "Song B", Artist: "Artist", + Participants: artistParticipants(model.Artist{ID: "art", Name: "Artist", OrderArtistName: "artist"}), + } allowTitlePhase(model.MediaFiles{trackA, trackB}) @@ -953,6 +1176,27 @@ func (m *mockMediaFileRepo) SetError(hasError bool) { } } +type mockArtistRepo struct { + mock.Mock + model.ArtistRepository +} + +func newMockArtistRepo() *mockArtistRepo { + return &mockArtistRepo{} +} + +func (m *mockArtistRepo) GetAll(options ...model.QueryOptions) (model.Artists, error) { + argsSlice := make([]any, len(options)) + for i, v := range options { + argsSlice[i] = v + } + args := m.Called(argsSlice...) + if args.Get(0) == nil { + return nil, args.Error(1) + } + return args.Get(0).(model.Artists), args.Error(1) +} + // matchFieldInAnd returns a matcher that checks whether QueryOptions.Filters is a // squirrel.And whose first element is a squirrel.Eq containing the given field name. func matchFieldInAnd(fieldName string) func(opt model.QueryOptions) bool { @@ -982,3 +1226,30 @@ func matchFieldInEq(fieldName string) func(opt model.QueryOptions) bool { return hasField } } + +// artistParticipants builds a Participants map crediting the given artists under RoleArtist. +func artistParticipants(artists ...model.Artist) model.Participants { + list := make(model.ParticipantList, len(artists)) + for i, a := range artists { + list[i] = model.Participant{Artist: a} + } + return model.Participants{model.RoleArtist: list} +} + +// matchTracksByArtistQuery matches the title phase's track-fetch query, identified by its +// squirrel.And containing a squirrel.Expr whose SQL references media_file_artists. +func matchTracksByArtistQuery() func(opt model.QueryOptions) bool { + return func(opt model.QueryOptions) bool { + and, ok := opt.Filters.(squirrel.And) + if !ok { + return false + } + for _, f := range and { + sql, _, err := f.ToSql() + if err == nil && strings.Contains(sql, "media_file_artists") { + return true + } + } + return false + } +}