refactor(lyrics): own the legacy artist/title lookup in the core service

Move the legacy getLyrics artist/title resolution out of the subsonic handler
and into the core lyrics service as GetLyricsByArtistTitle. The service now
holds a DataStore and owns the candidate query, the bounded duplicate window
(maxLegacyLyricsCandidates), and the source-priority resolution, mirroring the
by-song-id path.

The subsonic handler collapses to a single service call plus response mapping,
no longer reaching into the datastore or knowing the candidate cap. NewLyrics
gains a DataStore parameter (wired via Wire). The now-unused
SongsByArtistTitleWithLyricsFirst filter is removed. Behavior is unchanged.
Adds core tests covering the window bound, empty result, and resolution.

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

cmd/wire_gen.go

@@ -109,7 +109,7 @@ func CreateSubsonicAPIRouter(ctx context.Context) *subsonic.Router {
 	modelScanner := scanner.New(ctx, dataStore, cacheWarmer, broker, playlistsPlaylists, metricsMetrics)
 	playTracker := scrobbler.GetPlayTracker(dataStore, broker, manager)
 	playbackServer := playback.GetInstance(dataStore)
-	lyricsLyrics := lyrics.NewLyrics(manager)
+	lyricsLyrics := lyrics.NewLyrics(dataStore, manager)
 	transcodeDecider := stream.NewTranscodeDecider(dataStore, fFmpeg)
 	sonicSonic := sonic.New(dataStore, manager, matcherMatcher)
 	router := subsonic.New(dataStore, artworkArtwork, mediaStreamer, archiver, players, provider, modelScanner, broker, playlistsPlaylists, playTracker, share, playbackServer, metricsMetrics, lyricsLyrics, transcodeDecider, sonicSonic)

core/lyrics/lyrics.go

@@ -4,11 +4,18 @@ import (
 	"context"
 	"strings"
 
+	. "github.com/Masterminds/squirrel"
 	"github.com/navidrome/navidrome/conf"
 	"github.com/navidrome/navidrome/log"
 	"github.com/navidrome/navidrome/model"
+	"github.com/navidrome/navidrome/persistence"
 )
 
+// maxLegacyLyricsCandidates bounds the duplicate window scanned by the legacy
+// artist/title lookup, so source-priority resolution can still reach older
+// matches without turning it into an unbounded table scan.
+const maxLegacyLyricsCandidates = 10
+
 // Provider fetches lyrics for a single media file. It is the contract
 // implemented by individual lyrics sources, such as plugins.
 type Provider interface {
@@ -16,11 +23,10 @@ type Provider interface {
 }
 
 // Lyrics resolves lyrics for media files, honoring the configured source
-// priority. GetLyricsForMediaFiles preserves that priority across a set of
-// duplicate candidates instead of only consulting the first match.
+// priority.
 type Lyrics interface {
 	Provider
-	GetLyricsForMediaFiles(ctx context.Context, mediaFiles []model.MediaFile) (model.LyricList, error)
+	GetLyricsByArtistTitle(ctx context.Context, artist, title string) (model.LyricList, error)
 }
 
 // PluginLoader discovers and loads lyrics provider plugins.
@@ -29,13 +35,14 @@ type PluginLoader interface {
 }
 
 type lyricsService struct {
+	ds           model.DataStore
 	pluginLoader PluginLoader
 }
 
 // NewLyrics creates a new lyrics service. pluginLoader may be nil if no plugin
 // system is available.
-func NewLyrics(pluginLoader PluginLoader) Lyrics {
-	return &lyricsService{pluginLoader: pluginLoader}
+func NewLyrics(ds model.DataStore, pluginLoader PluginLoader) Lyrics {
+	return &lyricsService{ds: ds, pluginLoader: pluginLoader}
 }
 
 // GetLyrics returns lyrics for the given media file, trying sources in the
@@ -44,9 +51,19 @@ func (l *lyricsService) GetLyrics(ctx context.Context, mf *model.MediaFile) (mod
 	return l.getLyricsForCandidates(ctx, []*model.MediaFile{mf})
 }
 
-// GetLyricsForMediaFiles resolves lyrics across duplicate media files while
-// preserving the configured source priority across the full candidate set.
-func (l *lyricsService) GetLyricsForMediaFiles(ctx context.Context, mediaFiles []model.MediaFile) (model.LyricList, error) {
+// GetLyricsByArtistTitle resolves lyrics for the legacy artist/title lookup,
+// scanning a bounded window of duplicate matches so source priority still wins
+// across them.
+func (l *lyricsService) GetLyricsByArtistTitle(ctx context.Context, artist, title string) (model.LyricList, error) {
+	opts := songsByArtistTitleWithLyricsFirst(artist, title)
+	opts.Max = maxLegacyLyricsCandidates
+	mediaFiles, err := l.ds.MediaFile(ctx).GetAll(opts)
+	if err != nil {
+		return nil, err
+	}
+	if len(mediaFiles) == 0 {
+		return nil, nil
+	}
 	candidates := make([]*model.MediaFile, 0, len(mediaFiles))
 	for i := range mediaFiles {
 		candidates = append(candidates, &mediaFiles[i])
@@ -54,6 +71,21 @@ func (l *lyricsService) GetLyricsForMediaFiles(ctx context.Context, mediaFiles [
 	return l.getLyricsForCandidates(ctx, candidates)
 }
 
+func songsByArtistTitleWithLyricsFirst(artist, title string) model.QueryOptions {
+	return model.QueryOptions{
+		Sort:  "lyrics, updated_at",
+		Order: "desc",
+		Filters: And{
+			Eq{"missing": false},
+			Eq{"title": title},
+			Or{
+				persistence.Exists("json_tree(participants, '$.albumartist')", Eq{"value": artist}),
+				persistence.Exists("json_tree(participants, '$.artist')", Eq{"value": artist}),
+			},
+		},
+	}
+}
+
 func (l *lyricsService) getLyricsForCandidates(ctx context.Context, mediaFiles []*model.MediaFile) (model.LyricList, error) {
 	for pattern := range strings.SplitSeq(conf.Server.LyricsPriority, ",") {
 		pattern = strings.TrimSpace(pattern)

core/lyrics/lyrics_test.go

@@ -158,7 +158,7 @@ var _ = Describe("sources", func() {
 
 	DescribeTable("Lyrics Priority", func(priority string, expected model.LyricList) {
 		conf.Server.LyricsPriority = priority
-		svc := lyrics.NewLyrics(nil)
+		svc := lyrics.NewLyrics(nil, nil)
 		list, err := svc.GetLyrics(ctx, &mf)
 		Expect(err).To(BeNil())
 		Expect(list).To(Equal(expected))
@@ -175,9 +175,8 @@ var _ = Describe("sources", func() {
 		embeddedJSON, err := json.Marshal(embeddedLyrics)
 		Expect(err).To(BeNil())
 
-		svc := lyrics.NewLyrics(nil)
-
-		list, err := svc.GetLyricsForMediaFiles(ctx, []model.MediaFile{
+		repo := &tests.MockMediaFileRepo{}
+		repo.SetData(model.MediaFiles{
 			{
 				Lyrics: string(embeddedJSON),
 				Path:   "tests/fixtures/01 Invisible (RED) Edit Version.mp3",
@@ -187,6 +186,9 @@ var _ = Describe("sources", func() {
 				Path:   "tests/fixtures/test.mp3",
 			},
 		})
+		svc := lyrics.NewLyrics(&tests.MockDataStore{MockedMediaFile: repo}, nil)
+
+		list, err := svc.GetLyricsByArtistTitle(ctx, "Rick Astley", "Never Gonna Give You Up")
 		Expect(err).To(BeNil())
 		Expect(list).To(Equal(ttmlLyrics))
 	})
@@ -209,7 +211,7 @@ var _ = Describe("sources", func() {
 		conf.Server.LyricsPriority = ".LRC"
 		Expect(os.WriteFile(filepath.Join(dir, "song.LRC"), []byte("[00:01.00]Upper suffix"), 0600)).To(Succeed())
 
-		svc := lyrics.NewLyrics(nil)
+		svc := lyrics.NewLyrics(nil, nil)
 		list, err := svc.GetLyrics(ctx, &model.MediaFile{
 			LibraryPath: dir,
 			Path:        "song.mp3",
@@ -233,7 +235,7 @@ var _ = Describe("sources", func() {
 		Expect(os.WriteFile(filepath.Join(dir, "song.lrc"), []byte("[00:01.00]Fallback line"), 0600)).To(Succeed())
 
 		conf.Server.LyricsPriority = ".yaml,.lrc"
-		svc := lyrics.NewLyrics(nil)
+		svc := lyrics.NewLyrics(nil, nil)
 		list, err := svc.GetLyrics(ctx, &model.MediaFile{
 			LibraryPath: dir,
 			Path:        "song.mp3",
@@ -274,7 +276,7 @@ var _ = Describe("sources", func() {
 			It("should fallback to embedded if an error happens when parsing file", func() {
 				conf.Server.LyricsPriority = ".mp3,embedded"
 
-				svc := lyrics.NewLyrics(nil)
+				svc := lyrics.NewLyrics(nil, nil)
 				list, err := svc.GetLyrics(ctx, &mf)
 				Expect(err).To(BeNil())
 				Expect(list).To(Equal(embeddedLyrics))
@@ -283,7 +285,7 @@ var _ = Describe("sources", func() {
 			It("should return nothing if error happens when trying to parse file", func() {
 				conf.Server.LyricsPriority = ".mp3"
 
-				svc := lyrics.NewLyrics(nil)
+				svc := lyrics.NewLyrics(nil, nil)
 				list, err := svc.GetLyrics(ctx, &mf)
 				Expect(err).To(BeNil())
 				Expect(list).To(BeEmpty())
@@ -301,7 +303,7 @@ var _ = Describe("sources", func() {
 		It("should return lyrics from a plugin", func() {
 			conf.Server.LyricsPriority = "test-lyrics-plugin"
 			mockLoader.lyrics = unsyncedLyrics
-			svc := lyrics.NewLyrics(mockLoader)
+			svc := lyrics.NewLyrics(nil, mockLoader)
 			list, err := svc.GetLyrics(ctx, &mf)
 			Expect(err).To(BeNil())
 			Expect(list).To(Equal(unsyncedLyrics))
@@ -311,7 +313,7 @@ var _ = Describe("sources", func() {
 			conf.Server.LyricsPriority = "embedded,test-lyrics-plugin"
 			mf.Lyrics = "" // No embedded lyrics
 			mockLoader.lyrics = unsyncedLyrics
-			svc := lyrics.NewLyrics(mockLoader)
+			svc := lyrics.NewLyrics(nil, mockLoader)
 			list, err := svc.GetLyrics(ctx, &mf)
 			Expect(err).To(BeNil())
 			Expect(list).To(Equal(unsyncedLyrics))
@@ -320,7 +322,7 @@ var _ = Describe("sources", func() {
 		It("should skip plugin if embedded has lyrics", func() {
 			conf.Server.LyricsPriority = "embedded,test-lyrics-plugin"
 			mockLoader.lyrics = unsyncedLyrics
-			svc := lyrics.NewLyrics(mockLoader)
+			svc := lyrics.NewLyrics(nil, mockLoader)
 			list, err := svc.GetLyrics(ctx, &mf)
 			Expect(err).To(BeNil())
 			Expect(list).To(Equal(embeddedLyrics)) // embedded wins
@@ -329,7 +331,7 @@ var _ = Describe("sources", func() {
 		It("should skip unknown plugin names gracefully", func() {
 			conf.Server.LyricsPriority = "nonexistent-plugin,embedded"
 			mockLoader.notFound = true
-			svc := lyrics.NewLyrics(mockLoader)
+			svc := lyrics.NewLyrics(nil, mockLoader)
 			list, err := svc.GetLyrics(ctx, &mf)
 			Expect(err).To(BeNil())
 			Expect(list).To(Equal(embeddedLyrics)) // falls through to embedded
@@ -339,7 +341,7 @@ var _ = Describe("sources", func() {
 			conf.Server.LyricsPriority = "MyLyricsPlugin"
 			mockLoader.pluginName = "MyLyricsPlugin"
 			mockLoader.lyrics = unsyncedLyrics
-			svc := lyrics.NewLyrics(mockLoader)
+			svc := lyrics.NewLyrics(nil, mockLoader)
 			list, err := svc.GetLyrics(ctx, &mf)
 			Expect(err).To(BeNil())
 			Expect(list).To(Equal(unsyncedLyrics))
@@ -348,7 +350,7 @@ var _ = Describe("sources", func() {
 		It("should handle plugin error gracefully", func() {
 			conf.Server.LyricsPriority = "test-lyrics-plugin,embedded"
 			mockLoader.err = fmt.Errorf("plugin error")
-			svc := lyrics.NewLyrics(mockLoader)
+			svc := lyrics.NewLyrics(nil, mockLoader)
 			list, err := svc.GetLyrics(ctx, &mf)
 			Expect(err).To(BeNil())
 			Expect(list).To(Equal(embeddedLyrics)) // falls through to embedded
@@ -356,6 +358,51 @@ var _ = Describe("sources", func() {
 	})
 })
 
+var _ = Describe("GetLyricsByArtistTitle", func() {
+	var svc lyrics.Lyrics
+	var repo *tests.MockMediaFileRepo
+	var ds *tests.MockDataStore
+	var ctx context.Context
+
+	BeforeEach(func() {
+		DeferCleanup(configtest.SetupConfig())
+		conf.Server.LyricsPriority = "embedded"
+		repo = &tests.MockMediaFileRepo{}
+		ds = &tests.MockDataStore{MockedMediaFile: repo}
+		svc = lyrics.NewLyrics(ds, nil)
+		ctx = context.Background()
+	})
+
+	It("bounds the query to a duplicate window", func() {
+		repo.SetData(model.MediaFiles{})
+		_, err := svc.GetLyricsByArtistTitle(ctx, "Rick Astley", "Never Gonna Give You Up")
+		Expect(err).ToNot(HaveOccurred())
+		Expect(repo.Options.Max).To(Equal(10))
+	})
+
+	It("returns nil when no media file matches", func() {
+		repo.SetData(model.MediaFiles{})
+		list, err := svc.GetLyricsByArtistTitle(ctx, "Nobody", "No Song")
+		Expect(err).ToNot(HaveOccurred())
+		Expect(list).To(BeNil())
+	})
+
+	It("resolves lyrics from the matched media files", func() {
+		embedded, err := model.ToLyrics("eng", "Embedded lyrics line")
+		Expect(err).ToNot(HaveOccurred())
+		embeddedJSON, err := json.Marshal(model.LyricList{*embedded})
+		Expect(err).ToNot(HaveOccurred())
+		repo.SetData(model.MediaFiles{
+			{ID: "1", Title: "Never Gonna Give You Up", Lyrics: string(embeddedJSON)},
+		})
+
+		list, err := svc.GetLyricsByArtistTitle(ctx, "Rick Astley", "Never Gonna Give You Up")
+		Expect(err).ToNot(HaveOccurred())
+		Expect(list).To(HaveLen(1))
+		Expect(list[0].Line[0].Value).To(Equal("Embedded lyrics line"))
+	})
+})
+
 type mockPluginLoader struct {
 	lyrics     model.LyricList
 	err        error

server/e2e/e2e_suite_test.go

@@ -501,7 +501,7 @@ func setupTestDB() {
 		core.NewShare(ds),
 		playback.PlaybackServer(nil),
 		metrics.NewNoopInstance(),
-		lyrics.NewLyrics(nil),
+		lyrics.NewLyrics(nil, nil),
 		decider,
 		nil,
 	)

server/e2e/subsonic_sonic_similarity_test.go

@@ -47,7 +47,7 @@ func buildSonicRouter(provider sonic.Provider) *subsonic.Router {
 		core.NewShare(ds),
 		playback.PlaybackServer(nil),
 		metrics.NewNoopInstance(),
-		lyrics.NewLyrics(nil),
+		lyrics.NewLyrics(nil, nil),
 		decider,
 		sonicSvc,
 	)

server/subsonic/filter/filters.go

@@ -106,21 +106,6 @@ func SongsByGenreAndYearRange(genre string, fromYear, toYear int) Options {
 	return addDefaultFilters(options)
 }
 
-func SongsByArtistTitleWithLyricsFirst(artist, title string) Options {
-	return addDefaultFilters(Options{
-		Sort:  "lyrics, updated_at",
-		Order: "desc",
-		Max:   1,
-		Filters: And{
-			Eq{"title": title},
-			Or{
-				persistence.Exists("json_tree(participants, '$.albumartist')", Eq{"value": artist}),
-				persistence.Exists("json_tree(participants, '$.artist')", Eq{"value": artist}),
-			},
-		},
-	})
-}
-
 func ApplyLibraryFilter(opts Options, musicFolderIds []int) Options {
 	if len(musicFolderIds) == 0 {
 		return opts

server/subsonic/media_retrieval.go

@@ -13,14 +13,11 @@ import (
 	"github.com/navidrome/navidrome/log"
 	"github.com/navidrome/navidrome/model"
 	"github.com/navidrome/navidrome/resources"
-	"github.com/navidrome/navidrome/server/subsonic/filter"
 	"github.com/navidrome/navidrome/server/subsonic/responses"
 	"github.com/navidrome/navidrome/utils/gravatar"
 	"github.com/navidrome/navidrome/utils/req"
 )
 
-const maxLegacyLyricsCandidates = 10
-
 func (api *Router) GetAvatar(w http.ResponseWriter, r *http.Request) (*responses.Subsonic, error) {
 	if !conf.Server.EnableGravatar {
 		return api.getPlaceHolderAvatar(w, r)
@@ -100,21 +97,7 @@ func (api *Router) GetLyrics(r *http.Request) (*responses.Subsonic, error) {
 	response := newResponse()
 	lyricsResponse := responses.Lyrics{}
 	response.Lyrics = &lyricsResponse
-	opts := filter.SongsByArtistTitleWithLyricsFirst(artist, title)
-	// Search a bounded duplicate window so source-priority resolution can still
-	// reach older matches without turning legacy getLyrics into an unbounded scan.
-	opts.Max = maxLegacyLyricsCandidates
-	mediaFiles, err := api.ds.MediaFile(r.Context()).GetAll(opts)
-
-	if err != nil {
-		return nil, err
-	}
-
-	if len(mediaFiles) == 0 {
-		return response, nil
-	}
-
-	structuredLyrics, err := api.lyrics.GetLyricsForMediaFiles(r.Context(), mediaFiles)
+	structuredLyrics, err := api.lyrics.GetLyricsByArtistTitle(r.Context(), artist, title)
 	if err != nil {
 		return nil, err
 	}

server/subsonic/media_retrieval_test.go

@@ -34,7 +34,7 @@ var _ = Describe("MediaRetrievalController", func() {
 			MockedMediaFile: mockRepo,
 		}
 		artwork = &fakeArtwork{data: "image data"}
-		router = New(ds, artwork, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, lyrics.NewLyrics(nil), nil, nil)
+		router = New(ds, artwork, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, lyrics.NewLyrics(ds, nil), nil, nil)
 		w = httptest.NewRecorder()
 		DeferCleanup(configtest.SetupConfig())
 		conf.Server.LyricsPriority = "embedded,.lrc"
@@ -233,7 +233,8 @@ var _ = Describe("MediaRetrievalController", func() {
 			Expect(response.Lyrics.Artist).To(Equal("Rick Astley"))
 			Expect(response.Lyrics.Title).To(Equal("Never Gonna Give You Up"))
 			Expect(response.Lyrics.Value).To(Equal("We're no strangers to love\nYou know the rules and so do I\n"))
-			Expect(mockRepo.Options.Max).To(Equal(maxLegacyLyricsCandidates))
+			// The lyrics service bounds the legacy lookup to a duplicate window.
+			Expect(mockRepo.Options.Max).To(Equal(10))
 		})
 	})