diff --git a/model/searchable.go b/model/searchable.go index cc4f0b44e..631a11726 100644 --- a/model/searchable.go +++ b/model/searchable.go @@ -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) } diff --git a/persistence/album_repository.go b/persistence/album_repository.go index 682a409a1..6f9bb3b48 100644 --- a/persistence/album_repository.go +++ b/persistence/album_repository.go @@ -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) } diff --git a/persistence/artist_repository.go b/persistence/artist_repository.go index e2e1f83b3..a7cf9272a 100644 --- a/persistence/artist_repository.go +++ b/persistence/artist_repository.go @@ -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) diff --git a/persistence/artist_repository_test.go b/persistence/artist_repository_test.go index 2259012ac..dfaf499ac 100644 --- a/persistence/artist_repository_test.go +++ b/persistence/artist_repository_test.go @@ -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()) }) diff --git a/persistence/mediafile_repository.go b/persistence/mediafile_repository.go index 7c2ac5778..e7883947a 100644 --- a/persistence/mediafile_repository.go +++ b/persistence/mediafile_repository.go @@ -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) } diff --git a/persistence/mediafile_repository_test.go b/persistence/mediafile_repository_test.go index b1153b317..002b82499 100644 --- a/persistence/mediafile_repository_test.go +++ b/persistence/mediafile_repository_test.go @@ -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})) }) diff --git a/persistence/sql_search.go b/persistence/sql_search.go index d4d068945..0d3bfb743 100644 --- a/persistence/sql_search.go +++ b/persistence/sql_search.go @@ -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) } diff --git a/server/subsonic/searching.go b/server/subsonic/searching.go index e617535ad..ba1071320 100644 --- a/server/subsonic/searching.go +++ b/server/subsonic/searching.go @@ -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 { diff --git a/tests/mock_album_repo.go b/tests/mock_album_repo.go index 27eba2fbb..642ce6b41 100644 --- a/tests/mock_album_repo.go +++ b/tests/mock_album_repo.go @@ -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] } diff --git a/tests/mock_artist_repo.go b/tests/mock_artist_repo.go index 1298cbd2a..6d4792f83 100644 --- a/tests/mock_artist_repo.go +++ b/tests/mock_artist_repo.go @@ -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] } diff --git a/tests/mock_mediafile_repo.go b/tests/mock_mediafile_repo.go index 51c5dd10a..5b38a7187 100644 --- a/tests/mock_mediafile_repo.go +++ b/tests/mock_mediafile_repo.go @@ -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] }