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 <deluan@navidrome.org>

* 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 <deluan@navidrome.org>

* default order when not specified is `asc`

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

---------

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

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() {

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,
 	}
 )
 

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 {

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() {

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{

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

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) {