mirror/navidrome
1
0
Fork 0
mirror of https://github.com/navidrome/navidrome.git synced 2025-12-23 23:18:05 -05:00
Code Issues Packages Projects Releases Wiki Activity

fix(server): optimize search3 performance with multi-library (#4382)

Browse Source 
* fix(server): remove includeMissing from search (always false)

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

* fix(search): optimize search order by using natural order for improved performance

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

---------

Signed-off-by: Deluan <deluan@navidrome.org>
This commit is contained in:
Deluan Quintão
2025-07-25 18:53:40 -04:00
committed by GitHub
parent be83d68956
commit eeef98e2ca
11 changed files with 45 additions and 88 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
model/searchable.go
View File

@@ -1,5 +1,5 @@
package model
type SearchableRepository[T any] interface {
Search(q string, offset, size int, includeMissing bool, options ...QueryOptions) (T, error)
Search(q string, offset, size int, options ...QueryOptions) (T, error)
}

6
persistence/album_repository.go
View File

@@ -349,15 +349,15 @@ func (r *albumRepository) purgeEmpty() error {
return nil
}
func (r *albumRepository) Search(q string, offset int, size int, includeMissing bool, options ...model.QueryOptions) (model.Albums, error) {
func (r *albumRepository) Search(q string, offset int, size int, options ...model.QueryOptions) (model.Albums, error) {
var res dbAlbums
if uuid.Validate(q) == nil {
err := r.searchByMBID(r.selectAlbum(options...), q, []string{"mbz_album_id", "mbz_release_group_id"}, includeMissing, &res)
err := r.searchByMBID(r.selectAlbum(options...), q, []string{"mbz_album_id", "mbz_release_group_id"}, &res)
if err != nil {
return nil, fmt.Errorf("searching album by MBID %q: %w", q, err)
}
} else {
err := r.doSearch(r.selectAlbum(options...), q, offset, size, includeMissing, &res, "name")
err := r.doSearch(r.selectAlbum(options...), q, offset, size, &res, "album.rowid", "name")
if err != nil {
return nil, fmt.Errorf("searching album by query %q: %w", q, err)
}

7
persistence/artist_repository.go
View File

@@ -518,15 +518,16 @@ func (r *artistRepository) RefreshStats(allArtists bool) (int64, error) {
return totalRowsAffected, nil
}
func (r *artistRepository) Search(q string, offset int, size int, includeMissing bool, options ...model.QueryOptions) (model.Artists, error) {
func (r *artistRepository) Search(q string, offset int, size int, options ...model.QueryOptions) (model.Artists, error) {
var res dbArtists
if uuid.Validate(q) == nil {
err := r.searchByMBID(r.selectArtist(options...), q, []string{"mbz_artist_id"}, includeMissing, &res)
err := r.searchByMBID(r.selectArtist(options...), q, []string{"mbz_artist_id"}, &res)
if err != nil {
return nil, fmt.Errorf("searching artist by MBID %q: %w", q, err)
}
} else {
err := r.doSearch(r.selectArtist(options...), q, offset, size, includeMissing, &res,
// Natural order for artists is more performant by ID, due to GROUP BY clause in selectArtist
err := r.doSearch(r.selectArtist(options...), q, offset, size, &res, "artist.id",
"sum(json_extract(stats, '$.total.m')) desc", "name")
if err != nil {
return nil, fmt.Errorf("searching artist by query %q: %w", q, err)

57
persistence/artist_repository_test.go
View File

@@ -439,7 +439,7 @@ var _ = Describe("ArtistRepository", func() {
Expect(err).ToNot(HaveOccurred())
// Test the search
results, err := (*testRepo).Search("550e8400-e29b-41d4-a716-446655440010", 0, 10, false)
results, err := (*testRepo).Search("550e8400-e29b-41d4-a716-446655440010", 0, 10)
Expect(err).ToNot(HaveOccurred())
if shouldFind {
@@ -470,12 +470,12 @@ var _ = Describe("ArtistRepository", func() {
Expect(err).ToNot(HaveOccurred())
// Restricted user should not find this artist
results, err := restrictedRepo.Search("a74b1b7f-71a5-4011-9441-d0b5e4122711", 0, 10, false)
results, err := restrictedRepo.Search("a74b1b7f-71a5-4011-9441-d0b5e4122711", 0, 10)
Expect(err).ToNot(HaveOccurred())
Expect(results).To(BeEmpty())
// But admin should find it
results, err = repo.Search("a74b1b7f-71a5-4011-9441-d0b5e4122711", 0, 10, false)
results, err = repo.Search("a74b1b7f-71a5-4011-9441-d0b5e4122711", 0, 10)
Expect(err).ToNot(HaveOccurred())
Expect(results).To(HaveLen(1))
@@ -485,40 +485,9 @@ var _ = Describe("ArtistRepository", func() {
}
})
It("handles includeMissing parameter for MBID search", func() {
// Create a missing artist with MBID
missingArtist := createTestArtistWithMBID("test-missing-mbid-artist", "Test Missing MBID Artist", "550e8400-e29b-41d4-a716-446655440012")
missingArtist.Missing = true
err := createArtistWithLibrary(repo, &missingArtist, 1)
Expect(err).ToNot(HaveOccurred())
// Mark as missing
if raw, ok := repo.(*artistRepository); ok {
_, err = raw.executeSQL(squirrel.Update(raw.tableName).Set("missing", true).Where(squirrel.Eq{"id": missingArtist.ID}))
Expect(err).ToNot(HaveOccurred())
}
// Should not find missing artist when includeMissing is false
results, err := repo.Search("550e8400-e29b-41d4-a716-446655440012", 0, 10, false)
Expect(err).ToNot(HaveOccurred())
Expect(results).To(BeEmpty())
// Should find missing artist when includeMissing is true
results, err = repo.Search("550e8400-e29b-41d4-a716-446655440012", 0, 10, true)
Expect(err).ToNot(HaveOccurred())
Expect(results).To(HaveLen(1))
Expect(results[0].ID).To(Equal("test-missing-mbid-artist"))
// Clean up
if raw, ok := repo.(*artistRepository); ok {
_, _ = raw.executeSQL(squirrel.Delete(raw.tableName).Where(squirrel.Eq{"id": missingArtist.ID}))
}
})
Context("Text Search", func() {
It("allows admin to find artists by name regardless of library", func() {
results, err := repo.Search("Beatles", 0, 10, false)
results, err := repo.Search("Beatles", 0, 10)
Expect(err).ToNot(HaveOccurred())
Expect(results).To(HaveLen(1))
Expect(results[0].Name).To(Equal("The Beatles"))
@@ -538,7 +507,7 @@ var _ = Describe("ArtistRepository", func() {
Expect(err).ToNot(HaveOccurred())
// Restricted user should not find this artist
results, err := restrictedRepo.Search("Unique Search Name", 0, 10, false)
results, err := restrictedRepo.Search("Unique Search Name", 0, 10)
Expect(err).ToNot(HaveOccurred())
Expect(results).To(BeEmpty(), "Text search should respect library filtering")
@@ -639,20 +608,14 @@ var _ = Describe("ArtistRepository", func() {
_, _ = raw.executeSQL(squirrel.Delete(raw.tableName).Where(squirrel.Eq{"id": missingArtist.ID}))
})
It("admin can see missing artists when explicitly included", func() {
It("missing artists are never returned by search", func() {
// Should see missing artist in GetAll by default for admin users
artists, err := repo.GetAll()
Expect(err).ToNot(HaveOccurred())
Expect(artists).To(HaveLen(3)) // Including the missing artist
// Should see missing artist when searching with includeMissing=true
results, err := repo.Search("Missing Artist", 0, 10, true)
Expect(err).ToNot(HaveOccurred())
Expect(results).To(HaveLen(1))
Expect(results[0].ID).To(Equal("missing_test"))
// Should not see missing artist when searching with includeMissing=false
results, err = repo.Search("Missing Artist", 0, 10, false)
// Search never returns missing artists (hardcoded behavior)
results, err := repo.Search("Missing Artist", 0, 10)
Expect(err).ToNot(HaveOccurred())
Expect(results).To(BeEmpty())
})
@@ -706,11 +669,11 @@ var _ = Describe("ArtistRepository", func() {
})
It("Search returns empty results for users without library access", func() {
results, err := restrictedRepo.Search("Beatles", 0, 10, false)
results, err := restrictedRepo.Search("Beatles", 0, 10)
Expect(err).ToNot(HaveOccurred())
Expect(results).To(BeEmpty())
results, err = restrictedRepo.Search("Kraftwerk", 0, 10, false)
results, err = restrictedRepo.Search("Kraftwerk", 0, 10)
Expect(err).ToNot(HaveOccurred())
Expect(results).To(BeEmpty())
})

6
persistence/mediafile_repository.go
View File

@@ -339,15 +339,15 @@ func (r *mediaFileRepository) FindRecentFilesByProperties(missing model.MediaFil
return res.toModels(), nil
}
func (r *mediaFileRepository) Search(q string, offset int, size int, includeMissing bool, options ...model.QueryOptions) (model.MediaFiles, error) {
func (r *mediaFileRepository) Search(q string, offset int, size int, options ...model.QueryOptions) (model.MediaFiles, error) {
var res dbMediaFiles
if uuid.Validate(q) == nil {
err := r.searchByMBID(r.selectMediaFile(options...), q, []string{"mbz_recording_id", "mbz_release_track_id"}, includeMissing, &res)
err := r.searchByMBID(r.selectMediaFile(options...), q, []string{"mbz_recording_id", "mbz_release_track_id"}, &res)
if err != nil {
return nil, fmt.Errorf("searching media_file by MBID %q: %w", q, err)
}
} else {
err := r.doSearch(r.selectMediaFile(options...), q, offset, size, includeMissing, &res, "title")
err := r.doSearch(r.selectMediaFile(options...), q, offset, size, &res, "media_file.rowid", "title")
if err != nil {
return nil, fmt.Errorf("searching media_file by query %q: %w", q, err)
}

24
persistence/mediafile_repository_test.go
View File

@@ -314,7 +314,7 @@ var _ = Describe("MediaRepository", func() {
Describe("Search", func() {
Context("text search", func() {
It("finds media files by title", func() {
results, err := mr.Search("Antenna", 0, 10, false)
results, err := mr.Search("Antenna", 0, 10)
Expect(err).ToNot(HaveOccurred())
Expect(results).To(HaveLen(3)) // songAntenna, songAntennaWithLyrics, songAntenna2
for _, result := range results {
@@ -323,7 +323,7 @@ var _ = Describe("MediaRepository", func() {
})
It("finds media files case insensitively", func() {
results, err := mr.Search("antenna", 0, 10, false)
results, err := mr.Search("antenna", 0, 10)
Expect(err).ToNot(HaveOccurred())
Expect(results).To(HaveLen(3))
for _, result := range results {
@@ -332,7 +332,7 @@ var _ = Describe("MediaRepository", func() {
})
It("returns empty result when no matches found", func() {
results, err := mr.Search("nonexistent", 0, 10, false)
results, err := mr.Search("nonexistent", 0, 10)
Expect(err).ToNot(HaveOccurred())
Expect(results).To(BeEmpty())
})
@@ -365,7 +365,7 @@ var _ = Describe("MediaRepository", func() {
})
It("finds media file by mbz_recording_id", func() {
results, err := mr.Search("550e8400-e29b-41d4-a716-446655440020", 0, 10, false)
results, err := mr.Search("550e8400-e29b-41d4-a716-446655440020", 0, 10)
Expect(err).ToNot(HaveOccurred())
Expect(results).To(HaveLen(1))
Expect(results[0].ID).To(Equal("test-mbid-mediafile"))
@@ -373,7 +373,7 @@ var _ = Describe("MediaRepository", func() {
})
It("finds media file by mbz_release_track_id", func() {
results, err := mr.Search("550e8400-e29b-41d4-a716-446655440021", 0, 10, false)
results, err := mr.Search("550e8400-e29b-41d4-a716-446655440021", 0, 10)
Expect(err).ToNot(HaveOccurred())
Expect(results).To(HaveLen(1))
Expect(results[0].ID).To(Equal("test-mbid-mediafile"))
@@ -381,12 +381,12 @@ var _ = Describe("MediaRepository", func() {
})
It("returns empty result when MBID is not found", func() {
results, err := mr.Search("550e8400-e29b-41d4-a716-446655440099", 0, 10, false)
results, err := mr.Search("550e8400-e29b-41d4-a716-446655440099", 0, 10)
Expect(err).ToNot(HaveOccurred())
Expect(results).To(BeEmpty())
})
It("handles includeMissing parameter for MBID search", func() {
It("missing media files are never returned by search", func() {
// Create a missing media file with MBID
missingMediaFile := model.MediaFile{
ID: "test-missing-mbid-mediafile",
@@ -400,17 +400,11 @@ var _ = Describe("MediaRepository", func() {
err := mr.Put(&missingMediaFile)
Expect(err).ToNot(HaveOccurred())
// Should not find missing media file when includeMissing is false
results, err := mr.Search("550e8400-e29b-41d4-a716-446655440022", 0, 10, false)
// Search never returns missing media files (hardcoded behavior)
results, err := mr.Search("550e8400-e29b-41d4-a716-446655440022", 0, 10)
Expect(err).ToNot(HaveOccurred())
Expect(results).To(BeEmpty())
// Should find missing media file when includeMissing is true
results, err = mr.Search("550e8400-e29b-41d4-a716-446655440022", 0, 10, true)
Expect(err).ToNot(HaveOccurred())
Expect(results).To(HaveLen(1))
Expect(results[0].ID).To(Equal("test-missing-mbid-mediafile"))
// Clean up
_, _ = raw.executeSQL(squirrel.Delete(raw.tableName).Where(squirrel.Eq{"id": missingMediaFile.ID}))
})

21
persistence/sql_search.go
View File

@@ -15,7 +15,11 @@ func formatFullText(text ...string) string {
return " " + fullText
}
func (r sqlRepository) doSearch(sq SelectBuilder, q string, offset, size int, includeMissing bool, results any, orderBys ...string) error {
// doSearch performs a full-text search with the specified parameters.
// The naturalOrder is used to sort results when no full-text filter is applied. It is useful for cases like
// OpenSubsonic, where an empty search query should return all results in a natural order. Normally the parameter
// should be `tableName + ".rowid"`, but some repositories (ex: artist) may use a different natural order.
func (r sqlRepository) doSearch(sq SelectBuilder, q string, offset, size int, results any, naturalOrder string, orderBys ...string) error {
q = strings.TrimSpace(q)
q = strings.TrimSuffix(q, "*")
if len(q) < 2 {
@@ -27,23 +31,18 @@ func (r sqlRepository) doSearch(sq SelectBuilder, q string, offset, size int, in
sq = sq.Where(filter)
sq = sq.OrderBy(orderBys...)
} else {
// If the filter is empty, we sort by id.
// This is to speed up the results of `search3?query=""`, for OpenSubsonic
sq = sq.OrderBy(r.tableName + ".id")
}
if !includeMissing {
sq = sq.Where(Eq{r.tableName + ".missing": false})
// If the filter is empty, we sort by the specified natural order.
sq = sq.OrderBy(naturalOrder)
}
sq = sq.Where(Eq{r.tableName + ".missing": false})
sq = sq.Limit(uint64(size)).Offset(uint64(offset))
return r.queryAll(sq, results, model.QueryOptions{Offset: offset})
}
func (r sqlRepository) searchByMBID(sq SelectBuilder, mbid string, mbidFields []string, includeMissing bool, results any) error {
func (r sqlRepository) searchByMBID(sq SelectBuilder, mbid string, mbidFields []string, results any) error {
sq = sq.Where(mbidExpr(r.tableName, mbid, mbidFields...))
if !includeMissing {
sq = sq.Where(Eq{r.tableName + ".missing": false})
}
sq = sq.Where(Eq{r.tableName + ".missing": false})
return r.queryAll(sq, results)
}

4
server/subsonic/searching.go
View File

@@ -42,7 +42,7 @@ func (api *Router) getSearchParams(r *http.Request) (*searchParams, error) {
return sp, nil
}
type searchFunc[T any] func(q string, offset int, size int, includeMissing bool, options ...model.QueryOptions) (T, error)
type searchFunc[T any] func(q string, offset int, size int, options ...model.QueryOptions) (T, error)
func callSearch[T any](ctx context.Context, s searchFunc[T], q string, offset, size int, result *T, options ...model.QueryOptions) func() error {
return func() error {
@@ -52,7 +52,7 @@ func callSearch[T any](ctx context.Context, s searchFunc[T], q string, offset, s
typ := strings.TrimPrefix(reflect.TypeOf(*result).String(), "model.")
var err error
start := time.Now()
*result, err = s(q, offset, size, false, options...)
*result, err = s(q, offset, size, options...)
if err != nil {
log.Error(ctx, "Error searching "+typ, "query", q, "elapsed", time.Since(start), err)
} else {

2
tests/mock_album_repo.go
View File

@@ -118,7 +118,7 @@ func (m *MockAlbumRepo) UpdateExternalInfo(album *model.Album) error {
return nil
}
func (m *MockAlbumRepo) Search(q string, offset int, size int, includeMissing bool, options ...model.QueryOptions) (model.Albums, error) {
func (m *MockAlbumRepo) Search(q string, offset int, size int, options ...model.QueryOptions) (model.Albums, error) {
if len(options) > 0 {
m.Options = options[0]
}

2
tests/mock_artist_repo.go
View File

@@ -145,7 +145,7 @@ func (m *MockArtistRepo) GetIndex(includeMissing bool, libraryIds []int, roles .
return result, nil
}
func (m *MockArtistRepo) Search(q string, offset int, size int, includeMissing bool, options ...model.QueryOptions) (model.Artists, error) {
func (m *MockArtistRepo) Search(q string, offset int, size int, options ...model.QueryOptions) (model.Artists, error) {
if len(options) > 0 {
m.Options = options[0]
}

2
tests/mock_mediafile_repo.go
View File

@@ -234,7 +234,7 @@ func (m *MockMediaFileRepo) NewInstance() interface{} {
return &model.MediaFile{}
}
func (m *MockMediaFileRepo) Search(q string, offset int, size int, includeMissing bool, options ...model.QueryOptions) (model.MediaFiles, error) {
func (m *MockMediaFileRepo) Search(q string, offset int, size int, options ...model.QueryOptions) (model.MediaFiles, error) {
if len(options) > 0 {
m.Options = options[0]
}