From 04603a1ea2448c4e149af5be86e0da725530ac6e Mon Sep 17 00:00:00 2001 From: naiar Date: Fri, 20 Sep 2024 02:44:29 +0900 Subject: [PATCH] fix(subsonic): honour PreferSortTag when building indexes for `getArtist` and `getIndexes` (#3286) * fix(scanner): use sort_artist_name when the config PreferSortTags is true - #3285 * revert unwanted modifications * refactor(server): use cmp.Or to simplify nested ifs --------- Co-authored-by: Deluan --- persistence/artist_repository.go | 13 ++- persistence/artist_repository_test.go | 144 +++++++++++++++++++++++++- persistence/export_test.go | 5 + persistence/persistence_suite_test.go | 4 +- 4 files changed, 161 insertions(+), 5 deletions(-) create mode 100644 persistence/export_test.go diff --git a/persistence/artist_repository.go b/persistence/artist_repository.go index e55dd2952..af7c30138 100644 --- a/persistence/artist_repository.go +++ b/persistence/artist_repository.go @@ -1,6 +1,7 @@ package persistence import ( + "cmp" "context" "fmt" "net/url" @@ -143,7 +144,11 @@ func (r *artistRepository) toModels(dba []dbArtist) model.Artists { } func (r *artistRepository) getIndexKey(a *model.Artist) string { - name := strings.ToLower(str.RemoveArticle(a.Name)) + source := a.Name + if conf.Server.PreferSortTags { + source = cmp.Or(a.SortArtistName, a.OrderArtistName, source) + } + name := strings.ToLower(str.RemoveArticle(source)) for k, v := range r.indexGroups { key := strings.ToLower(k) if strings.HasPrefix(name, key) { @@ -155,7 +160,11 @@ func (r *artistRepository) getIndexKey(a *model.Artist) string { // TODO Cache the index (recalculate when there are changes to the DB) func (r *artistRepository) GetIndex() (model.ArtistIndexes, error) { - all, err := r.GetAll(model.QueryOptions{Sort: "order_artist_name"}) + sortColumn := "order_artist_name" + if conf.Server.PreferSortTags { + sortColumn = "sort_artist_name, order_artist_name" + } + all, err := r.GetAll(model.QueryOptions{Sort: sortColumn}) if err != nil { return nil, err } diff --git a/persistence/artist_repository_test.go b/persistence/artist_repository_test.go index 1b5b71769..31b035cec 100644 --- a/persistence/artist_repository_test.go +++ b/persistence/artist_repository_test.go @@ -4,10 +4,12 @@ import ( "context" "github.com/fatih/structs" + "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/db" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/request" + "github.com/navidrome/navidrome/utils" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" . "github.com/onsi/gomega/gstruct" @@ -43,8 +45,148 @@ var _ = Describe("ArtistRepository", func() { }) }) + Describe("GetIndexKey", func() { + r := artistRepository{indexGroups: utils.ParseIndexGroups(conf.Server.IndexGroups)} + It("returns the index key when PreferSortTags is true and SortArtistName is not empty", func() { + conf.Server.PreferSortTags = true + a := model.Artist{SortArtistName: "Foo", OrderArtistName: "Bar", Name: "Qux"} + idx := GetIndexKey(&r, &a) // defines export_test.go + Expect(idx).To(Equal("F")) + + a = model.Artist{SortArtistName: "foo", OrderArtistName: "Bar", Name: "Qux"} + idx = GetIndexKey(&r, &a) + Expect(idx).To(Equal("F")) + }) + + It("returns the index key when PreferSortTags is true, SortArtistName is empty and OrderArtistName is not empty", func() { + conf.Server.PreferSortTags = true + a := model.Artist{SortArtistName: "", OrderArtistName: "Bar", Name: "Qux"} + idx := GetIndexKey(&r, &a) + Expect(idx).To(Equal("B")) + + a = model.Artist{SortArtistName: "", OrderArtistName: "bar", Name: "Qux"} + idx = GetIndexKey(&r, &a) + Expect(idx).To(Equal("B")) + }) + + It("returns the index key when PreferSortTags is true, both SortArtistName, OrderArtistName are empty", func() { + conf.Server.PreferSortTags = true + a := model.Artist{SortArtistName: "", OrderArtistName: "", Name: "Qux"} + idx := GetIndexKey(&r, &a) + Expect(idx).To(Equal("Q")) + + a = model.Artist{SortArtistName: "", OrderArtistName: "", Name: "qux"} + idx = GetIndexKey(&r, &a) + Expect(idx).To(Equal("Q")) + }) + + It("returns the index key when PreferSortTags is false and SortArtistName is not empty", func() { + conf.Server.PreferSortTags = false + a := model.Artist{SortArtistName: "Foo", OrderArtistName: "Bar", Name: "Qux"} + idx := GetIndexKey(&r, &a) + Expect(idx).To(Equal("Q")) + }) + + It("returns the index key when PreferSortTags is true, SortArtistName is empty and OrderArtistName is not empty", func() { + conf.Server.PreferSortTags = false + a := model.Artist{SortArtistName: "", OrderArtistName: "Bar", Name: "Qux"} + idx := GetIndexKey(&r, &a) + Expect(idx).To(Equal("Q")) + }) + + It("returns the index key when PreferSortTags is true, both sort_artist_name, order_artist_name are empty", func() { + conf.Server.PreferSortTags = false + a := model.Artist{SortArtistName: "", OrderArtistName: "", Name: "Qux"} + idx := GetIndexKey(&r, &a) + Expect(idx).To(Equal("Q")) + + a = model.Artist{SortArtistName: "", OrderArtistName: "", Name: "qux"} + idx = GetIndexKey(&r, &a) + Expect(idx).To(Equal("Q")) + }) + }) + Describe("GetIndex", func() { - It("returns the index", func() { + It("returns the index when PreferSortTags is true and SortArtistName is not empty", func() { + conf.Server.PreferSortTags = true + + artistBeatles.SortArtistName = "Foo" + er := repo.Put(&artistBeatles) + Expect(er).To(BeNil()) + + idx, err := repo.GetIndex() + Expect(err).To(BeNil()) + Expect(idx).To(Equal(model.ArtistIndexes{ + { + ID: "F", + Artists: model.Artists{ + artistBeatles, + }, + }, + { + ID: "K", + Artists: model.Artists{ + artistKraftwerk, + }, + }, + })) + + artistBeatles.SortArtistName = "" + er = repo.Put(&artistBeatles) + Expect(er).To(BeNil()) + }) + + It("returns the index when PreferSortTags is true and SortArtistName is empty", func() { + conf.Server.PreferSortTags = true + idx, err := repo.GetIndex() + Expect(err).To(BeNil()) + Expect(idx).To(Equal(model.ArtistIndexes{ + { + ID: "B", + Artists: model.Artists{ + artistBeatles, + }, + }, + { + ID: "K", + Artists: model.Artists{ + artistKraftwerk, + }, + }, + })) + }) + + It("returns the index when PreferSortTags is false and SortArtistName is not empty", func() { + conf.Server.PreferSortTags = false + + artistBeatles.SortArtistName = "Foo" + er := repo.Put(&artistBeatles) + Expect(er).To(BeNil()) + + idx, err := repo.GetIndex() + Expect(err).To(BeNil()) + Expect(idx).To(Equal(model.ArtistIndexes{ + { + ID: "B", + Artists: model.Artists{ + artistBeatles, + }, + }, + { + ID: "K", + Artists: model.Artists{ + artistKraftwerk, + }, + }, + })) + + artistBeatles.SortArtistName = "" + er = repo.Put(&artistBeatles) + Expect(er).To(BeNil()) + }) + + It("returns the index when PreferSortTags is false and SortArtistName is empty", func() { + conf.Server.PreferSortTags = false idx, err := repo.GetIndex() Expect(err).To(BeNil()) Expect(idx).To(Equal(model.ArtistIndexes{ diff --git a/persistence/export_test.go b/persistence/export_test.go new file mode 100644 index 000000000..bb22f8536 --- /dev/null +++ b/persistence/export_test.go @@ -0,0 +1,5 @@ +package persistence + +// Definitions for testing private methods + +var GetIndexKey = (*artistRepository).getIndexKey diff --git a/persistence/persistence_suite_test.go b/persistence/persistence_suite_test.go index c56311fe9..06bf15ccc 100644 --- a/persistence/persistence_suite_test.go +++ b/persistence/persistence_suite_test.go @@ -35,8 +35,8 @@ var ( ) var ( - artistKraftwerk = model.Artist{ID: "2", Name: "Kraftwerk", AlbumCount: 1, FullText: " kraftwerk"} - artistBeatles = model.Artist{ID: "3", Name: "The Beatles", AlbumCount: 2, FullText: " beatles the"} + artistKraftwerk = model.Artist{ID: "2", Name: "Kraftwerk", OrderArtistName: "kraftwerk", AlbumCount: 1, FullText: " kraftwerk"} + artistBeatles = model.Artist{ID: "3", Name: "The Beatles", OrderArtistName: "beatles", AlbumCount: 2, FullText: " beatles the"} testArtists = model.Artists{ artistKraftwerk, artistBeatles,