From 8d594671c43bbc739c54d41f7159e07440db3ede Mon Sep 17 00:00:00 2001 From: Kendall Garner <17521368+kgarner7@users.noreply.github.com> Date: Mon, 16 Jun 2025 16:04:41 +0000 Subject: [PATCH] fix(subsonic): Sort songs by presence of lyrics for `getLyrics` (#4237) * fix(subsonic): Sort songs by presence of lyrics for `getLyrics` The current implementation of `getLyrics` fetches any songs matching the artist and title. However, this misses a case where there may be multiple matches for the same artist/song, and one has lyrics while the other doesn't. Resolve this by adding a custom SQL dynamic column that checks for the presence of lyrics. * add options to selectMediaFile, update test * more robust testing of GetAllByLyrics * fix(subsonic): refactor GetAllByLyrics to GetAll with lyrics sorting Signed-off-by: Deluan * use has_lyrics, and properly support multiple sort parts * better handle complicated internal sorts * just use a simpler filter * add note to setSortMappings * remove custom sort mapping, improve test with different updatedat * refactor tests and mock Signed-off-by: Deluan * default order when not specified is `asc` Signed-off-by: Deluan --------- Signed-off-by: Deluan Co-authored-by: Deluan --- persistence/mediafile_repository_test.go | 26 +++++++++++- persistence/persistence_suite_test.go | 16 ++++++- persistence/sql_base_repository.go | 4 ++ persistence/sql_base_repository_test.go | 12 ++++++ server/subsonic/filter/filters.go | 4 +- server/subsonic/media_retrieval.go | 2 +- server/subsonic/media_retrieval_test.go | 53 +++++++++++++++++++++--- 7 files changed, 106 insertions(+), 11 deletions(-) diff --git a/persistence/mediafile_repository_test.go b/persistence/mediafile_repository_test.go index c17bc595..9dbb8080 100644 --- a/persistence/mediafile_repository_test.go +++ b/persistence/mediafile_repository_test.go @@ -35,7 +35,31 @@ var _ = Describe("MediaRepository", func() { }) It("counts the number of mediafiles in the DB", func() { - Expect(mr.CountAll()).To(Equal(int64(4))) + Expect(mr.CountAll()).To(Equal(int64(6))) + }) + + It("returns songs ordered by lyrics with a specific title/artist", func() { + // attempt to mimic filters.SongsByArtistTitleWithLyricsFirst, except we want all items + results, err := mr.GetAll(model.QueryOptions{ + Sort: "lyrics, updated_at", + Order: "desc", + Filters: squirrel.And{ + squirrel.Eq{"title": "Antenna"}, + squirrel.Or{ + Exists("json_tree(participants, '$.albumartist')", squirrel.Eq{"value": "Kraftwerk"}), + Exists("json_tree(participants, '$.artist')", squirrel.Eq{"value": "Kraftwerk"}), + }, + }, + }) + + Expect(err).To(BeNil()) + Expect(results).To(HaveLen(3)) + Expect(results[0].Lyrics).To(Equal(`[{"lang":"xxx","line":[{"value":"This is a set of lyrics"}],"synced":false}]`)) + for _, item := range results[1:] { + Expect(item.Lyrics).To(Equal("[]")) + Expect(item.Title).To(Equal("Antenna")) + Expect(item.Participants[model.RoleArtist][0].Name).To(Equal("Kraftwerk")) + } }) It("checks existence of mediafiles in the DB", func() { diff --git a/persistence/persistence_suite_test.go b/persistence/persistence_suite_test.go index 43e4c292..fc451913 100644 --- a/persistence/persistence_suite_test.go +++ b/persistence/persistence_suite_test.go @@ -38,6 +38,9 @@ func mf(mf model.MediaFile) model.MediaFile { model.Participant{Artist: model.Artist{ID: mf.ArtistID, Name: mf.Artist}}, }, } + if mf.Lyrics == "" { + mf.Lyrics = "[]" + } return mf } @@ -78,11 +81,22 @@ var ( Path: p("/kraft/radio/antenna.mp3"), RGAlbumGain: 1.0, RGAlbumPeak: 2.0, RGTrackGain: 3.0, RGTrackPeak: 4.0, }) - testSongs = model.MediaFiles{ + songAntennaWithLyrics = mf(model.MediaFile{ + ID: "1005", + Title: "Antenna", + ArtistID: "2", + Artist: "Kraftwerk", + AlbumID: "103", + Lyrics: `[{"lang":"xxx","line":[{"value":"This is a set of lyrics"}],"synced":false}]`, + }) + songAntenna2 = mf(model.MediaFile{ID: "1006", Title: "Antenna", ArtistID: "2", Artist: "Kraftwerk", AlbumID: "103"}) + testSongs = model.MediaFiles{ songDayInALife, songComeTogether, songRadioactivity, songAntenna, + songAntennaWithLyrics, + songAntenna2, } ) diff --git a/persistence/sql_base_repository.go b/persistence/sql_base_repository.go index 7cc24b6c..ea22389a 100644 --- a/persistence/sql_base_repository.go +++ b/persistence/sql_base_repository.go @@ -86,6 +86,10 @@ func (r *sqlRepository) registerModel(instance any, filters map[string]filterFun // which gives precedence to sort tags. // Ex: order_title => (coalesce(nullif(sort_title,”),order_title) collate nocase) // To avoid performance issues, indexes should be created for these sort expressions +// +// NOTE: if an individual item has spaces, it should be wrapped in parentheses. For example, +// you should write "(lyrics != '[]')". This prevents the item being split unexpectedly. +// Without parentheses, "lyrics != '[]'" would be mapped as simply "lyrics" func (r *sqlRepository) setSortMappings(mappings map[string]string, tableName ...string) { tn := r.tableName if len(tableName) > 0 { diff --git a/persistence/sql_base_repository_test.go b/persistence/sql_base_repository_test.go index 4b380e29..7ba2a002 100644 --- a/persistence/sql_base_repository_test.go +++ b/persistence/sql_base_repository_test.go @@ -136,6 +136,10 @@ var _ = Describe("sqlRepository", func() { }) Describe("buildSortOrder", func() { + BeforeEach(func() { + r.sortMappings = map[string]string{} + }) + Context("single field", func() { It("sorts by specified field", func() { sql := r.buildSortOrder("name", "desc") @@ -163,6 +167,14 @@ var _ = Describe("sqlRepository", func() { sql := r.buildSortOrder("name desc, age, status asc", "desc") Expect(sql).To(Equal("name asc, age desc, status desc")) }) + It("handles spaces in mapped field", func() { + r.sortMappings = map[string]string{ + "has_lyrics": "(lyrics != '[]'), updated_at", + } + sql := r.buildSortOrder("has_lyrics", "desc") + Expect(sql).To(Equal("(lyrics != '[]') desc, updated_at desc")) + }) + }) Context("function fields", func() { It("handles functions with multiple params", func() { diff --git a/server/subsonic/filter/filters.go b/server/subsonic/filter/filters.go index ab507fb4..656973a4 100644 --- a/server/subsonic/filter/filters.go +++ b/server/subsonic/filter/filters.go @@ -108,9 +108,9 @@ func SongsByRandom(genre string, fromYear, toYear int) Options { return addDefaultFilters(options) } -func SongWithArtistTitle(artist, title string) Options { +func SongsByArtistTitleWithLyricsFirst(artist, title string) Options { return addDefaultFilters(Options{ - Sort: "updated_at", + Sort: "lyrics, updated_at", Order: "desc", Max: 1, Filters: And{ diff --git a/server/subsonic/media_retrieval.go b/server/subsonic/media_retrieval.go index 35a3fd3d..a72e4865 100644 --- a/server/subsonic/media_retrieval.go +++ b/server/subsonic/media_retrieval.go @@ -98,7 +98,7 @@ func (api *Router) GetLyrics(r *http.Request) (*responses.Subsonic, error) { response := newResponse() lyricsResponse := responses.Lyrics{} response.Lyrics = &lyricsResponse - mediaFiles, err := api.ds.MediaFile(r.Context()).GetAll(filter.SongWithArtistTitle(artist, title)) + mediaFiles, err := api.ds.MediaFile(r.Context()).GetAll(filter.SongsByArtistTitleWithLyricsFirst(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 a0e9754c..c2c49552 100644 --- a/server/subsonic/media_retrieval_test.go +++ b/server/subsonic/media_retrieval_test.go @@ -2,11 +2,13 @@ package subsonic import ( "bytes" + "cmp" "context" "encoding/json" "errors" "io" "net/http/httptest" + "slices" "time" "github.com/navidrome/navidrome/conf" @@ -84,12 +86,28 @@ var _ = Describe("MediaRetrievalController", func() { }) Expect(err).ToNot(HaveOccurred()) + baseTime := time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC) mockRepo.SetData(model.MediaFiles{ { - ID: "1", - Artist: "Rick Astley", - Title: "Never Gonna Give You Up", - Lyrics: string(lyricsJson), + ID: "2", + Artist: "Rick Astley", + Title: "Never Gonna Give You Up", + Lyrics: "[]", + UpdatedAt: baseTime.Add(2 * time.Hour), // No lyrics, newer + }, + { + ID: "1", + Artist: "Rick Astley", + Title: "Never Gonna Give You Up", + Lyrics: string(lyricsJson), + UpdatedAt: baseTime.Add(1 * time.Hour), // Has lyrics, older + }, + { + ID: "3", + Artist: "Rick Astley", + Title: "Never Gonna Give You Up", + Lyrics: "[]", + UpdatedAt: baseTime.Add(3 * time.Hour), // No lyrics, newest }, }) response, err := router.GetLyrics(r) @@ -122,6 +140,12 @@ var _ = Describe("MediaRetrievalController", func() { Artist: "Rick Astley", Title: "Never Gonna Give You Up", }, + { + Path: "tests/fixtures/test.mp3", + ID: "2", + Artist: "Rick Astley", + Title: "Never Gonna Give You Up", + }, }) response, err := router.GetLyrics(r) Expect(err).To(BeNil()) @@ -295,8 +319,25 @@ func (m *mockedMediaFile) SetData(mfs model.MediaFiles) { m.data = mfs } -func (m *mockedMediaFile) GetAll(...model.QueryOptions) (model.MediaFiles, error) { - return m.data, nil +func (m *mockedMediaFile) GetAll(opts ...model.QueryOptions) (model.MediaFiles, error) { + if len(opts) == 0 || opts[0].Sort != "lyrics, updated_at" { + return m.data, nil + } + + // Hardcoded support for lyrics sorting + result := slices.Clone(m.data) + // Sort by presence of lyrics, then by updated_at. Respect the order specified in opts. + slices.SortFunc(result, func(a, b model.MediaFile) int { + diff := cmp.Or( + cmp.Compare(a.Lyrics, b.Lyrics), + cmp.Compare(a.UpdatedAt.Unix(), b.UpdatedAt.Unix()), + ) + if opts[0].Order == "desc" { + return -diff + } + return diff + }) + return result, nil } func (m *mockedMediaFile) Get(id string) (*model.MediaFile, error) {