diff --git a/cmd/wire_gen.go b/cmd/wire_gen.go index 0939eef4d..d6ffc44d4 100644 --- a/cmd/wire_gen.go +++ b/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) diff --git a/core/lyrics/lyrics.go b/core/lyrics/lyrics.go index 82e60a64d..b9fb8cb74 100644 --- a/core/lyrics/lyrics.go +++ b/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) diff --git a/core/lyrics/lyrics_test.go b/core/lyrics/lyrics_test.go index 3f6c99eae..a097b90fc 100644 --- a/core/lyrics/lyrics_test.go +++ b/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 diff --git a/server/e2e/e2e_suite_test.go b/server/e2e/e2e_suite_test.go index 12a7c95e0..a6fe33457 100644 --- a/server/e2e/e2e_suite_test.go +++ b/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, ) diff --git a/server/e2e/subsonic_sonic_similarity_test.go b/server/e2e/subsonic_sonic_similarity_test.go index 40161470b..52cfffa11 100644 --- a/server/e2e/subsonic_sonic_similarity_test.go +++ b/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, ) diff --git a/server/subsonic/filter/filters.go b/server/subsonic/filter/filters.go index c3710394f..d19e163dd 100644 --- a/server/subsonic/filter/filters.go +++ b/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 diff --git a/server/subsonic/media_retrieval.go b/server/subsonic/media_retrieval.go index 9acda1929..089a1fdda 100644 --- a/server/subsonic/media_retrieval.go +++ b/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 } diff --git a/server/subsonic/media_retrieval_test.go b/server/subsonic/media_retrieval_test.go index c7db51da6..be7ca4515 100644 --- a/server/subsonic/media_retrieval_test.go +++ b/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)) }) })