From b59eb329610964f226f66fecb7190b06d28d052c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Mon, 23 Feb 2026 08:51:54 -0500 Subject: [PATCH] feat(subsonic): sort search3 results by relevance (#5086) * fix(subsonic): optimize search3 for high-cardinality FTS queries Use a two-phase query strategy for FTS5 searches to avoid the performance penalty of expensive LEFT JOINs (annotation, bookmark, library) on high-cardinality results like "the". Phase 1 runs a lightweight query (main table + FTS index only) to get sorted, paginated rowids. Phase 2 hydrates only those few rowids with the full JOINs, making them nearly free. For queries with complex ORDER BY expressions that reference joined tables (e.g. artist search sorted by play count), the optimization is skipped and the original single-query approach is used. * fix(search): update order by clauses to include 'rank' for FTS queries Signed-off-by: Deluan * fix(search): reintroduce 'rank' in Phase 2 ORDER BY for FTS queries Signed-off-by: Deluan * fix(search): remove 'rank' from ORDER BY in non-FTS queries and adjust two-phase query handling Signed-off-by: Deluan * fix(search): update FTS ranking to use bm25 weights and simplify ORDER BY qualification Signed-off-by: Deluan * fix(search): refine FTS query handling and improve comments for clarity Signed-off-by: Deluan * fix(search): refactor full-text search handling to streamline query strategy selection and improve LIKE fallback logic. Increase e2e coverage for search3 Signed-off-by: Deluan * refactor: enhance FTS column definitions and relevance weights Signed-off-by: Deluan * fix(search): refactor Search method signatures to remove offset and size parameters, streamline query handling Signed-off-by: Deluan * fix(search): allow single-character queries in search strategies and update related tests Signed-off-by: Deluan * fix(search): make FTS Phase 1 treat Max=0 as no limit, reorganize tests FTS Phase 1 unconditionally called Limit(uint64(options.Max)), which produced LIMIT 0 when Max was zero. This diverged from applyOptions where Max=0 means no limit. Now Phase 1 mirrors applyOptions: only add LIMIT/OFFSET when the value is positive. Also moved legacy backend integration tests from sql_search_fts_test.go to sql_search_like_test.go and added regression tests for the Max=0 behavior on both backends. * refactor: simplify callSearch function by removing variadic options and directly using QueryOptions Signed-off-by: Deluan * fix(search): implement ftsQueryDegraded function to detect significant content loss in FTS queries Signed-off-by: Deluan --------- Signed-off-by: Deluan --- model/searchable.go | 2 +- persistence/album_repository.go | 26 +- persistence/artist_repository.go | 32 +-- persistence/artist_repository_test.go | 16 +- persistence/mediafile_repository.go | 26 +- persistence/mediafile_repository_test.go | 14 +- persistence/sql_restful.go | 6 +- persistence/sql_restful_test.go | 122 +++++---- persistence/sql_search.go | 118 +++++---- persistence/sql_search_fts.go | 271 +++++++++++++++----- persistence/sql_search_fts_test.go | 299 +++++++++++++++-------- persistence/sql_search_like.go | 106 ++++++++ persistence/sql_search_like_test.go | 134 ++++++++++ persistence/sql_search_test.go | 83 +++---- server/e2e/e2e_suite_test.go | 3 + server/e2e/subsonic_album_lists_test.go | 40 +-- server/e2e/subsonic_browsing_test.go | 4 +- server/e2e/subsonic_multilibrary_test.go | 18 +- server/e2e/subsonic_searching_test.go | 59 ++++- server/subsonic/searching.go | 35 ++- tests/mock_album_repo.go | 2 +- tests/mock_artist_repo.go | 2 +- tests/mock_mediafile_repo.go | 2 +- 23 files changed, 1005 insertions(+), 415 deletions(-) create mode 100644 persistence/sql_search_like.go create mode 100644 persistence/sql_search_like_test.go diff --git a/model/searchable.go b/model/searchable.go index 631a1172..a64a0171 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, options ...QueryOptions) (T, error) + Search(q string, options ...QueryOptions) (T, error) } diff --git a/persistence/album_repository.go b/persistence/album_repository.go index 58bfcca5..077e33d1 100644 --- a/persistence/album_repository.go +++ b/persistence/album_repository.go @@ -12,7 +12,6 @@ import ( . "github.com/Masterminds/squirrel" "github.com/deluan/rest" - "github.com/google/uuid" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" @@ -353,18 +352,21 @@ func (r *albumRepository) purgeEmpty(libraryIDs ...int) error { return nil } -func (r *albumRepository) Search(q string, offset int, size int, options ...model.QueryOptions) (model.Albums, error) { +var albumSearchConfig = searchConfig{ + NaturalOrder: "album.rowid", + OrderBy: []string{"name"}, + MBIDFields: []string{"mbz_album_id", "mbz_release_group_id"}, +} + +func (r *albumRepository) Search(q string, options ...model.QueryOptions) (model.Albums, error) { + var opts model.QueryOptions + if len(options) > 0 { + opts = options[0] + } var res dbAlbums - if uuid.Validate(q) == nil { - 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, &res, "album.rowid", "name") - if err != nil { - return nil, fmt.Errorf("searching album by query %q: %w", q, err) - } + err := r.doSearch(r.selectAlbum(options...), q, &res, albumSearchConfig, opts) + if err != nil { + return nil, fmt.Errorf("searching album %q: %w", q, err) } return res.toModels(), nil } diff --git a/persistence/artist_repository.go b/persistence/artist_repository.go index f801787d..07824e21 100644 --- a/persistence/artist_repository.go +++ b/persistence/artist_repository.go @@ -11,7 +11,6 @@ import ( . "github.com/Masterminds/squirrel" "github.com/deluan/rest" - "github.com/google/uuid" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" @@ -513,20 +512,25 @@ func (r *artistRepository) RefreshStats(allArtists bool) (int64, error) { return totalRowsAffected, nil } -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"}, &res) - if err != nil { - return nil, fmt.Errorf("searching artist by MBID %q: %w", q, err) - } - } else { +func (r *artistRepository) searchCfg() searchConfig { + return searchConfig{ // 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) - } + NaturalOrder: "artist.id", + OrderBy: []string{"sum(json_extract(stats, '$.total.m')) desc", "name"}, + MBIDFields: []string{"mbz_artist_id"}, + LibraryFilter: r.applyLibraryFilterToArtistQuery, + } +} + +func (r *artistRepository) Search(q string, options ...model.QueryOptions) (model.Artists, error) { + var opts model.QueryOptions + if len(options) > 0 { + opts = options[0] + } + var res dbArtists + err := r.doSearch(r.selectArtist(options...), q, &res, r.searchCfg(), opts) + if err != nil { + return nil, fmt.Errorf("searching artist %q: %w", q, err) } return res.toModels(), nil } diff --git a/persistence/artist_repository_test.go b/persistence/artist_repository_test.go index 15340eeb..90e449e8 100644 --- a/persistence/artist_repository_test.go +++ b/persistence/artist_repository_test.go @@ -512,7 +512,7 @@ var _ = Describe("ArtistRepository", func() { Expect(err).ToNot(HaveOccurred()) // Test the search - results, err := (*testRepo).Search("550e8400-e29b-41d4-a716-446655440010", 0, 10) + results, err := (*testRepo).Search("550e8400-e29b-41d4-a716-446655440010", model.QueryOptions{Max: 10}) Expect(err).ToNot(HaveOccurred()) if shouldFind { @@ -543,12 +543,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) + results, err := restrictedRepo.Search("a74b1b7f-71a5-4011-9441-d0b5e4122711", model.QueryOptions{Max: 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) + results, err = repo.Search("a74b1b7f-71a5-4011-9441-d0b5e4122711", model.QueryOptions{Max: 10}) Expect(err).ToNot(HaveOccurred()) Expect(results).To(HaveLen(1)) @@ -560,7 +560,7 @@ var _ = Describe("ArtistRepository", func() { Context("Text Search", func() { It("allows admin to find artists by name regardless of library", func() { - results, err := repo.Search("Beatles", 0, 10) + results, err := repo.Search("Beatles", model.QueryOptions{Max: 10}) Expect(err).ToNot(HaveOccurred()) Expect(results).To(HaveLen(1)) Expect(results[0].Name).To(Equal("The Beatles")) @@ -580,7 +580,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) + results, err := restrictedRepo.Search("Unique Search Name", model.QueryOptions{Max: 10}) Expect(err).ToNot(HaveOccurred()) Expect(results).To(BeEmpty(), "Text search should respect library filtering") @@ -688,7 +688,7 @@ var _ = Describe("ArtistRepository", func() { Expect(artists).To(HaveLen(5)) // Including the missing artist // Search never returns missing artists (hardcoded behavior) - results, err := repo.Search("Missing Artist", 0, 10) + results, err := repo.Search("Missing Artist", model.QueryOptions{Max: 10}) Expect(err).ToNot(HaveOccurred()) Expect(results).To(BeEmpty()) }) @@ -742,11 +742,11 @@ var _ = Describe("ArtistRepository", func() { }) It("Search returns empty results for users without library access", func() { - results, err := restrictedRepo.Search("Beatles", 0, 10) + results, err := restrictedRepo.Search("Beatles", model.QueryOptions{Max: 10}) Expect(err).ToNot(HaveOccurred()) Expect(results).To(BeEmpty()) - results, err = restrictedRepo.Search("Kraftwerk", 0, 10) + results, err = restrictedRepo.Search("Kraftwerk", model.QueryOptions{Max: 10}) Expect(err).ToNot(HaveOccurred()) Expect(results).To(BeEmpty()) }) diff --git a/persistence/mediafile_repository.go b/persistence/mediafile_repository.go index 264be6f3..394ca5b7 100644 --- a/persistence/mediafile_repository.go +++ b/persistence/mediafile_repository.go @@ -11,7 +11,6 @@ import ( . "github.com/Masterminds/squirrel" "github.com/deluan/rest" - "github.com/google/uuid" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" @@ -428,18 +427,21 @@ func (r *mediaFileRepository) FindRecentFilesByProperties(missing model.MediaFil return res.toModels(), nil } -func (r *mediaFileRepository) Search(q string, offset int, size int, options ...model.QueryOptions) (model.MediaFiles, error) { +var mediaFileSearchConfig = searchConfig{ + NaturalOrder: "media_file.rowid", + OrderBy: []string{"title"}, + MBIDFields: []string{"mbz_recording_id", "mbz_release_track_id"}, +} + +func (r *mediaFileRepository) Search(q string, options ...model.QueryOptions) (model.MediaFiles, error) { + var opts model.QueryOptions + if len(options) > 0 { + opts = options[0] + } var res dbMediaFiles - if uuid.Validate(q) == nil { - 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, &res, "media_file.rowid", "title") - if err != nil { - return nil, fmt.Errorf("searching media_file by query %q: %w", q, err) - } + err := r.doSearch(r.selectMediaFile(options...), q, &res, mediaFileSearchConfig, opts) + if err != nil { + return nil, fmt.Errorf("searching media_file %q: %w", q, err) } return res.toModels(), nil } diff --git a/persistence/mediafile_repository_test.go b/persistence/mediafile_repository_test.go index 853480b3..7a04d79e 100644 --- a/persistence/mediafile_repository_test.go +++ b/persistence/mediafile_repository_test.go @@ -527,7 +527,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) + results, err := mr.Search("Antenna", model.QueryOptions{Max: 10}) Expect(err).ToNot(HaveOccurred()) Expect(results).To(HaveLen(3)) // songAntenna, songAntennaWithLyrics, songAntenna2 for _, result := range results { @@ -536,7 +536,7 @@ var _ = Describe("MediaRepository", func() { }) It("finds media files case insensitively", func() { - results, err := mr.Search("antenna", 0, 10) + results, err := mr.Search("antenna", model.QueryOptions{Max: 10}) Expect(err).ToNot(HaveOccurred()) Expect(results).To(HaveLen(3)) for _, result := range results { @@ -545,7 +545,7 @@ var _ = Describe("MediaRepository", func() { }) It("returns empty result when no matches found", func() { - results, err := mr.Search("nonexistent", 0, 10) + results, err := mr.Search("nonexistent", model.QueryOptions{Max: 10}) Expect(err).ToNot(HaveOccurred()) Expect(results).To(BeEmpty()) }) @@ -578,7 +578,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) + results, err := mr.Search("550e8400-e29b-41d4-a716-446655440020", model.QueryOptions{Max: 10}) Expect(err).ToNot(HaveOccurred()) Expect(results).To(HaveLen(1)) Expect(results[0].ID).To(Equal("test-mbid-mediafile")) @@ -586,7 +586,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) + results, err := mr.Search("550e8400-e29b-41d4-a716-446655440021", model.QueryOptions{Max: 10}) Expect(err).ToNot(HaveOccurred()) Expect(results).To(HaveLen(1)) Expect(results[0].ID).To(Equal("test-mbid-mediafile")) @@ -594,7 +594,7 @@ 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) + results, err := mr.Search("550e8400-e29b-41d4-a716-446655440099", model.QueryOptions{Max: 10}) Expect(err).ToNot(HaveOccurred()) Expect(results).To(BeEmpty()) }) @@ -614,7 +614,7 @@ var _ = Describe("MediaRepository", func() { Expect(err).ToNot(HaveOccurred()) // Search never returns missing media files (hardcoded behavior) - results, err := mr.Search("550e8400-e29b-41d4-a716-446655440022", 0, 10) + results, err := mr.Search("550e8400-e29b-41d4-a716-446655440022", model.QueryOptions{Max: 10}) Expect(err).ToNot(HaveOccurred()) Expect(results).To(BeEmpty()) diff --git a/persistence/sql_restful.go b/persistence/sql_restful.go index 27207c45..02162387 100644 --- a/persistence/sql_restful.go +++ b/persistence/sql_restful.go @@ -109,12 +109,10 @@ func booleanFilter(field string, value any) Sqlizer { func fullTextFilter(tableName string, mbidFields ...string) func(string, any) Sqlizer { return func(field string, value any) Sqlizer { v := strings.ToLower(value.(string)) - searchExpr := getSearchExpr() - cond := cmp.Or( + return cmp.Or[Sqlizer]( mbidExpr(tableName, v, mbidFields...), - searchExpr(tableName, v), + getSearchStrategy(tableName, v), ) - return cond } } diff --git a/persistence/sql_restful_test.go b/persistence/sql_restful_test.go index ea0f802a..32f418b8 100644 --- a/persistence/sql_restful_test.go +++ b/persistence/sql_restful_test.go @@ -102,11 +102,11 @@ var _ = Describe("sqlRestful", func() { uuid := "550e8400-e29b-41d4-a716-446655440000" result := noMbidFilter("search", uuid) - // mbidExpr with no fields returns nil, so cmp.Or falls back to fullTextExpr - expected := squirrel.And{ - squirrel.Like{"test_table.full_text": "% 550e8400-e29b-41d4-a716-446655440000%"}, - } - Expect(result).To(Equal(expected)) + // mbidExpr with no fields returns nil, so cmp.Or falls back to search strategy + sql, args, err := result.ToSql() + Expect(err).ToNot(HaveOccurred()) + Expect(sql).To(ContainSubstring("test_table.full_text LIKE")) + Expect(args).To(ContainElement("% 550e8400-e29b-41d4-a716-446655440000%")) }) }) @@ -114,26 +114,25 @@ var _ = Describe("sqlRestful", func() { It("returns full text search condition only", func() { result := filter("search", "beatles") - // mbidExpr returns nil for non-UUIDs, so fullTextExpr result is returned directly - expected := squirrel.And{ - squirrel.Like{"test_table.full_text": "% beatles%"}, - } - Expect(result).To(Equal(expected)) + // mbidExpr returns nil for non-UUIDs, so search strategy result is returned directly + sql, args, err := result.ToSql() + Expect(err).ToNot(HaveOccurred()) + Expect(sql).To(ContainSubstring("test_table.full_text LIKE")) + Expect(args).To(ContainElement("% beatles%")) }) It("handles multi-word search terms", func() { result := filter("search", "the beatles abbey road") - // Should return And condition directly - andCondition, ok := result.(squirrel.And) - Expect(ok).To(BeTrue()) - Expect(andCondition).To(HaveLen(4)) - - // Check that all words are present (order may vary) - Expect(andCondition).To(ContainElement(squirrel.Like{"test_table.full_text": "% the%"})) - Expect(andCondition).To(ContainElement(squirrel.Like{"test_table.full_text": "% beatles%"})) - Expect(andCondition).To(ContainElement(squirrel.Like{"test_table.full_text": "% abbey%"})) - Expect(andCondition).To(ContainElement(squirrel.Like{"test_table.full_text": "% road%"})) + sql, args, err := result.ToSql() + Expect(err).ToNot(HaveOccurred()) + // All words should be present as LIKE conditions + Expect(sql).To(ContainSubstring("test_table.full_text LIKE")) + Expect(args).To(HaveLen(4)) + Expect(args).To(ContainElement("% the%")) + Expect(args).To(ContainElement("% beatles%")) + Expect(args).To(ContainElement("% abbey%")) + Expect(args).To(ContainElement("% road%")) }) }) @@ -142,26 +141,48 @@ var _ = Describe("sqlRestful", func() { conf.Server.Search.FullString = false result := filter("search", "test query") - andCondition, ok := result.(squirrel.And) - Expect(ok).To(BeTrue()) - Expect(andCondition).To(HaveLen(2)) - - // Check that all words are present with leading space (order may vary) - Expect(andCondition).To(ContainElement(squirrel.Like{"test_table.full_text": "% test%"})) - Expect(andCondition).To(ContainElement(squirrel.Like{"test_table.full_text": "% query%"})) + sql, args, err := result.ToSql() + Expect(err).ToNot(HaveOccurred()) + Expect(sql).To(ContainSubstring("test_table.full_text LIKE")) + Expect(args).To(HaveLen(2)) + Expect(args).To(ContainElement("% test%")) + Expect(args).To(ContainElement("% query%")) }) It("uses no separator with SearchFullString=true", func() { conf.Server.Search.FullString = true result := filter("search", "test query") - andCondition, ok := result.(squirrel.And) - Expect(ok).To(BeTrue()) - Expect(andCondition).To(HaveLen(2)) + sql, args, err := result.ToSql() + Expect(err).ToNot(HaveOccurred()) + Expect(sql).To(ContainSubstring("test_table.full_text LIKE")) + Expect(args).To(HaveLen(2)) + Expect(args).To(ContainElement("%test%")) + Expect(args).To(ContainElement("%query%")) + }) + }) - // Check that all words are present without leading space (order may vary) - Expect(andCondition).To(ContainElement(squirrel.Like{"test_table.full_text": "%test%"})) - Expect(andCondition).To(ContainElement(squirrel.Like{"test_table.full_text": "%query%"})) + Context("single-character queries (regression: must not be rejected)", func() { + It("returns valid filter for single-char query with legacy backend", func() { + conf.Server.Search.Backend = "legacy" + result := filter("search", "a") + Expect(result).ToNot(BeNil(), "single-char REST filter must not be dropped") + sql, args, err := result.ToSql() + Expect(err).ToNot(HaveOccurred()) + Expect(sql).To(ContainSubstring("LIKE")) + Expect(args).ToNot(BeEmpty()) + }) + + It("returns valid filter for single-char query with FTS backend", func() { + conf.Server.Search.Backend = "fts" + conf.Server.Search.FullString = false + ftsFilter := fullTextFilter(tableName, mbidFields...) + result := ftsFilter("search", "a") + Expect(result).ToNot(BeNil(), "single-char REST filter must not be dropped") + sql, args, err := result.ToSql() + Expect(err).ToNot(HaveOccurred()) + Expect(sql).To(ContainSubstring("MATCH")) + Expect(args).ToNot(BeEmpty()) }) }) @@ -179,10 +200,10 @@ var _ = Describe("sqlRestful", func() { It("handles special characters that are sanitized", func() { result := filter("search", "don't") - expected := squirrel.And{ - squirrel.Like{"test_table.full_text": "% dont%"}, // str.SanitizeStrings removes quotes - } - Expect(result).To(Equal(expected)) + sql, args, err := result.ToSql() + Expect(err).ToNot(HaveOccurred()) + Expect(sql).To(ContainSubstring("test_table.full_text LIKE")) + Expect(args).To(ContainElement("% dont%")) }) It("returns nil for single quote (SQL injection protection)", func() { @@ -206,31 +227,30 @@ var _ = Describe("sqlRestful", func() { result := filter("search", "550e8400-invalid-uuid") // Should return full text filter since UUID is invalid - expected := squirrel.And{ - squirrel.Like{"test_table.full_text": "% 550e8400-invalid-uuid%"}, - } - Expect(result).To(Equal(expected)) + sql, args, err := result.ToSql() + Expect(err).ToNot(HaveOccurred()) + Expect(sql).To(ContainSubstring("test_table.full_text LIKE")) + Expect(args).To(ContainElement("% 550e8400-invalid-uuid%")) }) It("handles empty mbid fields array", func() { emptyMbidFilter := fullTextFilter(tableName, []string{}...) result := emptyMbidFilter("search", "test") - // mbidExpr with empty fields returns nil, so cmp.Or falls back to fullTextExpr - expected := squirrel.And{ - squirrel.Like{"test_table.full_text": "% test%"}, - } - Expect(result).To(Equal(expected)) + // mbidExpr with empty fields returns nil, so search strategy result is returned directly + sql, args, err := result.ToSql() + Expect(err).ToNot(HaveOccurred()) + Expect(sql).To(ContainSubstring("test_table.full_text LIKE")) + Expect(args).To(ContainElement("% test%")) }) It("converts value to lowercase before processing", func() { result := filter("search", "TEST") - // The function converts to lowercase internally - expected := squirrel.And{ - squirrel.Like{"test_table.full_text": "% test%"}, - } - Expect(result).To(Equal(expected)) + sql, args, err := result.ToSql() + Expect(err).ToNot(HaveOccurred()) + Expect(sql).To(ContainSubstring("test_table.full_text LIKE")) + Expect(args).To(ContainElement("% test%")) }) }) }) diff --git a/persistence/sql_search.go b/persistence/sql_search.go index e5c245bd..43965ebb 100644 --- a/persistence/sql_search.go +++ b/persistence/sql_search.go @@ -6,7 +6,6 @@ import ( . "github.com/Masterminds/squirrel" "github.com/google/uuid" "github.com/navidrome/navidrome/conf" - "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/utils/str" ) @@ -16,57 +15,71 @@ func formatFullText(text ...string) string { return " " + fullText } -// searchExprFunc is the function signature for search expression builders. -type searchExprFunc func(tableName string, query string) Sqlizer - -// getSearchExpr returns the active search expression function based on config. -// It falls back to legacySearchExpr when Search.FullString is enabled, because -// FTS5 is token-based and cannot match substrings within words. -// CJK queries are routed to likeSearchExpr, since FTS5's unicode61 tokenizer -// cannot segment CJK text. -func getSearchExpr() searchExprFunc { - if conf.Server.Search.Backend == "legacy" || conf.Server.Search.FullString { - return legacySearchExpr - } - return func(tableName, query string) Sqlizer { - if containsCJK(query) { - return likeSearchExpr(tableName, query) - } - return ftsSearchExpr(tableName, query) - } +// searchConfig holds per-repository constants for doSearch. +type searchConfig struct { + NaturalOrder string // ORDER BY for empty-query results (e.g. "album.rowid") + OrderBy []string // ORDER BY for text search results (e.g. ["name"]) + MBIDFields []string // columns to match when query is a UUID + // LibraryFilter overrides the default applyLibraryFilter for FTS Phase 1. + // Needed when library access requires a junction table (e.g. artist → library_artist). + LibraryFilter func(sq SelectBuilder) SelectBuilder } -// 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 { +// searchStrategy defines how to execute a text search against a repository table. +// options carries filters and pagination that must reach all query phases, +// including FTS Phase 1 which builds its own query outside sq. +type searchStrategy interface { + Sqlizer + execute(r sqlRepository, sq SelectBuilder, dest any, cfg searchConfig, options model.QueryOptions) error +} + +// getSearchStrategy returns the appropriate search strategy based on config and query content. +// Returns nil when the query produces no searchable tokens. +func getSearchStrategy(tableName, query string) searchStrategy { + if conf.Server.Search.Backend == "legacy" || conf.Server.Search.FullString { + return newLegacySearch(tableName, query) + } + if containsCJK(query) { + return newLikeSearch(tableName, query) + } + return newFTSSearch(tableName, query) +} + +// doSearch dispatches a search query: empty → natural order, UUID → MBID match, +// otherwise delegates to getSearchStrategy. sq must already have LIMIT/OFFSET set +// via newSelect(options...). options is forwarded so FTS Phase 1 can apply the same +// filters and pagination independently. +func (r sqlRepository) doSearch(sq SelectBuilder, q string, results any, cfg searchConfig, options model.QueryOptions) error { q = strings.TrimSpace(q) q = strings.TrimSuffix(q, "*") + + sq = sq.Where(Eq{r.tableName + ".missing": false}) + + // Empty query (OpenSubsonic `search3?query=""`) — return all in natural order. + if q == "" || q == `""` { + sq = sq.OrderBy(cfg.NaturalOrder) + return r.queryAll(sq, results, options) + } + + // MBID search: if query is a valid UUID, search by MBID fields instead + if uuid.Validate(q) == nil && len(cfg.MBIDFields) > 0 { + sq = sq.Where(mbidExpr(r.tableName, q, cfg.MBIDFields...)) + return r.queryAll(sq, results) + } + + // Min-length guard: single-character queries are too broad for search3. + // This check lives here (not in the strategies) so that fullTextFilter + // (REST filter path) can still use single-character queries. if len(q) < 2 { return nil } - searchExpr := getSearchExpr() - filter := searchExpr(r.tableName, q) - if filter != nil { - sq = sq.Where(filter) - sq = sq.OrderBy(orderBys...) - } else { - // This is to speed up the results of `search3?query=""`, for OpenSubsonic - // If the filter is empty, we sort by the specified natural order. - sq = sq.OrderBy(naturalOrder) + strategy := getSearchStrategy(r.tableName, q) + if strategy == nil { + return nil } - 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, results any) error { - sq = sq.Where(mbidExpr(r.tableName, mbid, mbidFields...)) - sq = sq.Where(Eq{r.tableName + ".missing": false}) - - return r.queryAll(sq, results) + return strategy.execute(r, sq, results, cfg, options) } func mbidExpr(tableName, mbid string, mbidFields ...string) Sqlizer { @@ -80,24 +93,3 @@ func mbidExpr(tableName, mbid string, mbidFields ...string) Sqlizer { } return Or(cond) } - -// legacySearchExpr generates LIKE-based search filters against the full_text column. -// This is the original search implementation, used when Search.Backend="legacy". -func legacySearchExpr(tableName string, s string) Sqlizer { - q := str.SanitizeStrings(s) - if q == "" { - log.Trace("Search using legacy backend, query is empty", "table", tableName) - return nil - } - var sep string - if !conf.Server.Search.FullString { - sep = " " - } - parts := strings.Split(q, " ") - filters := And{} - for _, part := range parts { - filters = append(filters, Like{tableName + ".full_text": "%" + sep + part + "%"}) - } - log.Trace("Search using legacy backend", "query", filters, "table", tableName) - return filters -} diff --git a/persistence/sql_search_fts.go b/persistence/sql_search_fts.go index ea70518b..9eb01f0c 100644 --- a/persistence/sql_search_fts.go +++ b/persistence/sql_search_fts.go @@ -9,6 +9,7 @@ import ( . "github.com/Masterminds/squirrel" "github.com/navidrome/navidrome/log" + "github.com/navidrome/navidrome/model" ) // containsCJK returns true if the string contains any CJK (Chinese/Japanese/Korean) characters. @@ -187,75 +188,235 @@ func buildFTS5Query(userInput string) string { return result } -// likeSearchColumns defines the core columns to search with LIKE queries. -// These are the primary user-visible fields for each entity type. -// Used as a fallback when FTS5 cannot handle the query (e.g., CJK text, punctuation-only input). -var likeSearchColumns = map[string][]string{ - "media_file": {"title", "album", "artist", "album_artist"}, - "album": {"name", "album_artist"}, - "artist": {"name"}, +// ftsColumn pairs an FTS5 column name with its BM25 relevance weight. +type ftsColumn struct { + Name string + Weight float64 } -// likeSearchExpr generates LIKE-based search filters against core columns. -// Each word in the query must match at least one column (AND between words), -// and each word can match any column (OR within a word). -// Used as a fallback when FTS5 cannot handle the query (e.g., CJK text, punctuation-only input). -func likeSearchExpr(tableName string, s string) Sqlizer { - s = strings.TrimSpace(s) - if s == "" { - log.Trace("Search using LIKE backend, query is empty", "table", tableName) - return nil - } - columns, ok := likeSearchColumns[tableName] - if !ok { - log.Trace("Search using LIKE backend, couldn't find columns for this table", "table", tableName) - return nil - } - words := strings.Fields(s) - wordFilters := And{} - for _, word := range words { - colFilters := Or{} - for _, col := range columns { - colFilters = append(colFilters, Like{tableName + "." + col: "%" + word + "%"}) +// ftsColumnDefs defines FTS5 columns and their BM25 relevance weights. +// The order MUST match the column order in the FTS5 table definition (see migrations). +// All columns are both searched and ranked. When adding indexed-but-not-searched +// columns in the future, use Weight: 0 to exclude from the search column filter. +var ftsColumnDefs = map[string][]ftsColumn{ + "media_file": { + {"title", 10.0}, + {"album", 5.0}, + {"artist", 3.0}, + {"album_artist", 3.0}, + {"sort_title", 1.0}, + {"sort_album_name", 1.0}, + {"sort_artist_name", 1.0}, + {"sort_album_artist_name", 1.0}, + {"disc_subtitle", 1.0}, + {"search_participants", 2.0}, + {"search_normalized", 1.0}, + }, + "album": { + {"name", 10.0}, + {"sort_album_name", 1.0}, + {"album_artist", 3.0}, + {"search_participants", 2.0}, + {"discs", 1.0}, + {"catalog_num", 1.0}, + {"album_version", 1.0}, + {"search_normalized", 1.0}, + }, + "artist": { + {"name", 10.0}, + {"sort_artist_name", 1.0}, + {"search_normalized", 1.0}, + }, +} + +// ftsColumnFilters and ftsBM25Weights are precomputed from ftsColumnDefs at init time +// to avoid per-query allocations. +var ( + ftsColumnFilters = map[string]string{} + ftsBM25Weights = map[string]string{} +) + +func init() { + for table, cols := range ftsColumnDefs { + var names []string + weights := make([]string, len(cols)) + for i, c := range cols { + if c.Weight > 0 { + names = append(names, c.Name) + } + weights[i] = fmt.Sprintf("%.1f", c.Weight) } - wordFilters = append(wordFilters, colFilters) + ftsColumnFilters[table] = "{" + strings.Join(names, " ") + "}" + ftsBM25Weights[table] = strings.Join(weights, ", ") } - log.Trace("Search using LIKE backend", "query", wordFilters, "table", tableName) - return wordFilters } -// ftsSearchColumns defines which FTS5 columns are included in general search. -// Columns not listed here are indexed but not searched by default, -// enabling future additions (comments, lyrics, bios) without affecting general search. -var ftsSearchColumns = map[string]string{ - "media_file": "{title album artist album_artist sort_title sort_album_name sort_artist_name sort_album_artist_name disc_subtitle search_participants search_normalized}", - "album": "{name sort_album_name album_artist search_participants discs catalog_num album_version search_normalized}", - "artist": "{name sort_artist_name search_normalized}", +// ftsSearch implements searchStrategy using FTS5 full-text search with BM25 ranking. +type ftsSearch struct { + tableName string + ftsTable string + matchExpr string + rankExpr string } -// ftsSearchExpr generates an FTS5 MATCH-based search filter. -// If the query produces no FTS tokens (e.g., punctuation-only like "!!!!!!!"), -// it falls back to LIKE-based search. -func ftsSearchExpr(tableName string, s string) Sqlizer { - q := buildFTS5Query(s) - if q == "" { - s = strings.TrimSpace(strings.ReplaceAll(s, `"`, "")) - if s != "" { - log.Trace("Search using LIKE fallback for non-tokenizable query", "table", tableName, "query", s) - return likeSearchExpr(tableName, s) +// ToSql returns a single-query fallback for the REST filter path (no two-phase split). +func (s *ftsSearch) ToSql() (string, []interface{}, error) { + sql := s.tableName + ".rowid IN (SELECT rowid FROM " + s.ftsTable + " WHERE " + s.ftsTable + " MATCH ?)" + return sql, []interface{}{s.matchExpr}, nil +} + +// execute runs a two-phase FTS5 search: +// - Phase 1: lightweight rowid query (main table + FTS + library filter) for ranking and pagination. +// - Phase 2: full SELECT with all JOINs, scoped to Phase 1's rowid set. +// +// Complex ORDER BY (function calls, aggregations) are dropped from Phase 1. +func (s *ftsSearch) execute(r sqlRepository, sq SelectBuilder, dest any, cfg searchConfig, options model.QueryOptions) error { + qualifiedOrderBys := []string{s.rankExpr} + for _, ob := range cfg.OrderBy { + if qualified := qualifyOrderBy(s.tableName, ob); qualified != "" { + qualifiedOrderBys = append(qualifiedOrderBys, qualified) + } + } + + // Phase 1: fresh query — must set LIMIT/OFFSET from options explicitly. + // Mirror applyOptions behavior: Max=0 means no limit, not LIMIT 0. + rowidQuery := Select(s.tableName+".rowid"). + From(s.tableName). + Join(s.ftsTable+" ON "+s.ftsTable+".rowid = "+s.tableName+".rowid AND "+s.ftsTable+" MATCH ?", s.matchExpr). + Where(Eq{s.tableName + ".missing": false}). + OrderBy(qualifiedOrderBys...) + if options.Max > 0 { + rowidQuery = rowidQuery.Limit(uint64(options.Max)) + } + if options.Offset > 0 { + rowidQuery = rowidQuery.Offset(uint64(options.Offset)) + } + + // Library filter + musicFolderId must be applied here, before pagination. + if cfg.LibraryFilter != nil { + rowidQuery = cfg.LibraryFilter(rowidQuery) + } else { + rowidQuery = r.applyLibraryFilter(rowidQuery) + } + if options.Filters != nil { + rowidQuery = rowidQuery.Where(options.Filters) + } + + rowidSQL, rowidArgs, err := rowidQuery.ToSql() + if err != nil { + return fmt.Errorf("building FTS rowid query: %w", err) + } + + // Phase 2: strip LIMIT/OFFSET from sq (Phase 1 handled pagination), + // join on the ranked rowid set to hydrate with full columns. + sq = sq.RemoveLimit().RemoveOffset() + rankedSubquery := fmt.Sprintf( + "(SELECT rowid as _rid, row_number() OVER () AS _rn FROM (%s)) AS _ranked", + rowidSQL, + ) + sq = sq.Join(rankedSubquery+" ON "+s.tableName+".rowid = _ranked._rid", rowidArgs...) + sq = sq.OrderBy("_ranked._rn") + return r.queryAll(sq, dest) +} + +// qualifyOrderBy prepends tableName to a simple column name. Returns empty string for +// complex expressions (function calls, aggregations) that can't be used in Phase 1. +func qualifyOrderBy(tableName, orderBy string) string { + orderBy = strings.TrimSpace(orderBy) + if orderBy == "" || strings.ContainsAny(orderBy, "(,") { + return "" + } + parts := strings.Fields(orderBy) + if !strings.Contains(parts[0], ".") { + parts[0] = tableName + "." + parts[0] + } + return strings.Join(parts, " ") +} + +// ftsQueryDegraded returns true when the FTS query lost significant discriminating +// content compared to the original input. This happens when special characters that +// are part of the entity name (e.g., "1+", "C++", "!!!", "C#") get stripped by FTS +// tokenization, leaving only very short/broad tokens. Also detects quoted phrases +// that would be degraded by FTS5's unicode61 tokenizer (e.g., "1+" → token "1"). +func ftsQueryDegraded(original, ftsQuery string) bool { + original = strings.TrimSpace(original) + if original == "" || ftsQuery == "" { + return false + } + // Strip quotes from original for comparison — we want the raw content + stripped := strings.ReplaceAll(original, `"`, "") + // Extract the alphanumeric content from the original query + alphaNum := fts5PunctStrip.ReplaceAllString(stripped, "") + // If the original is entirely alphanumeric, nothing was stripped — not degraded + if len(alphaNum) == len(stripped) { + return false + } + // Check if all effective FTS tokens are very short (≤2 chars). + // Short tokens with prefix matching are too broad when special chars were stripped. + // For quoted phrases, extract the content and check the tokens inside. + tokens := strings.Fields(ftsQuery) + for _, t := range tokens { + t = strings.TrimSuffix(t, "*") + // Skip internal phrase placeholders + if strings.HasPrefix(t, "\x00") { + return false + } + // For OR groups from processPunctuatedWords (e.g., ("a ha" OR aha*)), + // the punctuated word was already handled meaningfully — not degraded. + if strings.HasPrefix(t, "(") { + return false + } + // For quoted phrases, check the tokens inside as FTS5 will tokenize them + if strings.HasPrefix(t, `"`) { + // Extract content between quotes + inner := strings.Trim(t, `"`) + innerAlpha := fts5PunctStrip.ReplaceAllString(inner, " ") + for _, it := range strings.Fields(innerAlpha) { + if len(it) > 2 { + return false + } + } + continue + } + if len(t) > 2 { + return false + } + } + return true +} + +// newFTSSearch creates an FTS5 search strategy. Falls back to LIKE search if the +// query produces no FTS tokens (e.g., punctuation-only like "!!!!!!!") or if FTS +// tokenization stripped significant content from the query (e.g., "1+" → "1*"). +// Returns nil when the query produces no searchable tokens at all. +func newFTSSearch(tableName, query string) searchStrategy { + q := buildFTS5Query(query) + if q == "" || ftsQueryDegraded(query, q) { + // Fallback: try LIKE search with the raw query + cleaned := strings.TrimSpace(strings.ReplaceAll(query, `"`, "")) + if cleaned != "" { + log.Trace("Search using LIKE fallback for non-tokenizable query", "table", tableName, "query", cleaned) + return newLikeSearch(tableName, cleaned) } return nil } ftsTable := tableName + "_fts" matchExpr := q - if cols, ok := ftsSearchColumns[tableName]; ok { + if cols, ok := ftsColumnFilters[tableName]; ok { matchExpr = cols + " : (" + q + ")" } - filter := Expr( - tableName+".rowid IN (SELECT rowid FROM "+ftsTable+" WHERE "+ftsTable+" MATCH ?)", - matchExpr, - ) - log.Trace("Search using FTS5 backend", "table", tableName, "query", q, "filter", filter) - return filter + rankExpr := ftsTable + ".rank" + if weights, ok := ftsBM25Weights[tableName]; ok { + rankExpr = "bm25(" + ftsTable + ", " + weights + ")" + } + + s := &ftsSearch{ + tableName: tableName, + ftsTable: ftsTable, + matchExpr: matchExpr, + rankExpr: rankExpr, + } + log.Trace("Search using FTS5 backend", "table", tableName, "query", q, "filter", s) + return s } diff --git a/persistence/sql_search_fts_test.go b/persistence/sql_search_fts_test.go index e0fead8a..337d5420 100644 --- a/persistence/sql_search_fts_test.go +++ b/persistence/sql_search_fts_test.go @@ -3,8 +3,6 @@ package persistence import ( "context" - "github.com/navidrome/navidrome/conf" - "github.com/navidrome/navidrome/conf/configtest" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/request" @@ -52,6 +50,23 @@ var _ = DescribeTable("buildFTS5Query", Entry("returns empty string for empty quoted phrase", `""`, ""), ) +var _ = DescribeTable("ftsQueryDegraded", + func(original, ftsQuery string, expected bool) { + Expect(ftsQueryDegraded(original, ftsQuery)).To(Equal(expected)) + }, + Entry("not degraded for empty original", "", "1*", false), + Entry("not degraded for empty ftsQuery", "1+", "", false), + Entry("not degraded for purely alphanumeric query", "beatles", "beatles*", false), + Entry("not degraded when long tokens remain", "test^val", "test* val*", false), + Entry("not degraded for quoted phrase with long tokens", `"the beatles"`, `"the beatles"`, false), + Entry("degraded for quoted phrase with only short tokens after tokenizer strips special chars", `"1+"`, `"1+"`, true), + Entry("not degraded for quoted phrase with meaningful content", `"C++ programming"`, `"C++ programming"`, false), + Entry("degraded when special chars stripped leaving short token", "1+", "1*", true), + Entry("degraded when special chars stripped leaving two short tokens", "C# 1", "C* 1*", true), + Entry("not degraded when at least one long token remains", "1+ beatles", "1* beatles*", false), + Entry("not degraded for OR groups from processPunctuatedWords", "AC/DC", `("AC DC" OR ACDC*)`, false), +) + var _ = DescribeTable("normalizeForFTS", func(expected string, values ...string) { Expect(normalizeForFTS(values...)).To(Equal(expected)) @@ -81,133 +96,211 @@ var _ = DescribeTable("containsCJK", Entry("detects single CJK character", "a曲b", true), ) -var _ = Describe("likeSearchExpr", func() { - It("returns nil for empty query", func() { - Expect(likeSearchExpr("media_file", "")).To(BeNil()) +var _ = DescribeTable("qualifyOrderBy", + func(tableName, orderBy, expected string) { + Expect(qualifyOrderBy(tableName, orderBy)).To(Equal(expected)) + }, + Entry("returns empty string for empty input", "artist", "", ""), + Entry("qualifies simple column with table name", "artist", "name", "artist.name"), + Entry("qualifies column with direction", "artist", "name desc", "artist.name desc"), + Entry("preserves already-qualified column", "artist", "artist.name", "artist.name"), + Entry("preserves already-qualified column with direction", "artist", "artist.name desc", "artist.name desc"), + Entry("returns empty for function call expression", "artist", "sum(json_extract(stats, '$.total.m')) desc", ""), + Entry("returns empty for expression with comma", "artist", "a, b", ""), + Entry("qualifies media_file column", "media_file", "title", "media_file.title"), +) + +var _ = Describe("ftsColumnDefs helpers", func() { + Describe("ftsColumnFilters", func() { + It("returns column filter for media_file", func() { + Expect(ftsColumnFilters).To(HaveKeyWithValue("media_file", + "{title album artist album_artist sort_title sort_album_name sort_artist_name sort_album_artist_name disc_subtitle search_participants search_normalized}", + )) + }) + + It("returns column filter for album", func() { + Expect(ftsColumnFilters).To(HaveKeyWithValue("album", + "{name sort_album_name album_artist search_participants discs catalog_num album_version search_normalized}", + )) + }) + + It("returns column filter for artist", func() { + Expect(ftsColumnFilters).To(HaveKeyWithValue("artist", + "{name sort_artist_name search_normalized}", + )) + }) + + It("has no entry for unknown table", func() { + Expect(ftsColumnFilters).ToNot(HaveKey("unknown")) + }) }) - It("returns nil for whitespace-only query", func() { - Expect(likeSearchExpr("media_file", " ")).To(BeNil()) + Describe("ftsBM25Weights", func() { + It("returns weight CSV for media_file", func() { + Expect(ftsBM25Weights).To(HaveKeyWithValue("media_file", + "10.0, 5.0, 3.0, 3.0, 1.0, 1.0, 1.0, 1.0, 1.0, 2.0, 1.0", + )) + }) + + It("returns weight CSV for album", func() { + Expect(ftsBM25Weights).To(HaveKeyWithValue("album", + "10.0, 1.0, 3.0, 2.0, 1.0, 1.0, 1.0, 1.0", + )) + }) + + It("returns weight CSV for artist", func() { + Expect(ftsBM25Weights).To(HaveKeyWithValue("artist", + "10.0, 1.0, 1.0", + )) + }) + + It("has no entry for unknown table", func() { + Expect(ftsBM25Weights).ToNot(HaveKey("unknown")) + }) }) - It("generates LIKE filters against core columns for single CJK word", func() { - expr := likeSearchExpr("media_file", "周杰伦") - sql, args, err := expr.ToSql() - Expect(err).ToNot(HaveOccurred()) - // Should have OR between columns for the single word - Expect(sql).To(ContainSubstring("OR")) - Expect(sql).To(ContainSubstring("media_file.title LIKE")) - Expect(sql).To(ContainSubstring("media_file.album LIKE")) - Expect(sql).To(ContainSubstring("media_file.artist LIKE")) - Expect(sql).To(ContainSubstring("media_file.album_artist LIKE")) - Expect(args).To(HaveLen(4)) - for _, arg := range args { - Expect(arg).To(Equal("%周杰伦%")) + It("has definitions for all known tables", func() { + for _, table := range []string{"media_file", "album", "artist"} { + Expect(ftsColumnDefs).To(HaveKey(table)) + Expect(ftsColumnDefs[table]).ToNot(BeEmpty()) } }) - It("generates AND of OR groups for multi-word query", func() { - expr := likeSearchExpr("media_file", "周杰伦 greatest") - sql, args, err := expr.ToSql() - Expect(err).ToNot(HaveOccurred()) - // Two groups AND'd together, each with 4 columns OR'd - Expect(sql).To(ContainSubstring("AND")) - Expect(args).To(HaveLen(8)) - }) - - It("uses correct columns for album table", func() { - expr := likeSearchExpr("album", "周杰伦") - sql, args, err := expr.ToSql() - Expect(err).ToNot(HaveOccurred()) - Expect(sql).To(ContainSubstring("album.name LIKE")) - Expect(sql).To(ContainSubstring("album.album_artist LIKE")) - Expect(args).To(HaveLen(2)) - }) - - It("uses correct columns for artist table", func() { - expr := likeSearchExpr("artist", "周杰伦") - sql, args, err := expr.ToSql() - Expect(err).ToNot(HaveOccurred()) - Expect(sql).To(ContainSubstring("artist.name LIKE")) - Expect(args).To(HaveLen(1)) - }) - - It("returns nil for unknown table", func() { - Expect(likeSearchExpr("unknown_table", "周杰伦")).To(BeNil()) + It("has matching column count between filter and weights", func() { + for table, cols := range ftsColumnDefs { + // Column filter only includes Weight > 0 columns + filterCount := 0 + for _, c := range cols { + if c.Weight > 0 { + filterCount++ + } + } + // For now, all columns have Weight > 0, so filter count == total count + Expect(filterCount).To(Equal(len(cols)), "table %s: all columns should have positive weights", table) + } }) }) -var _ = Describe("ftsSearchExpr", func() { +var _ = Describe("newFTSSearch", func() { It("returns nil for empty query", func() { - Expect(ftsSearchExpr("media_file", "")).To(BeNil()) + Expect(newFTSSearch("media_file", "")).To(BeNil()) }) - It("generates rowid IN subquery with MATCH and column filter", func() { - expr := ftsSearchExpr("media_file", "beatles") - sql, args, err := expr.ToSql() + It("returns non-nil for single-character query", func() { + strategy := newFTSSearch("media_file", "a") + Expect(strategy).ToNot(BeNil(), "single-char queries must not be rejected; min-length is enforced in doSearch, not here") + sql, _, err := strategy.ToSql() + Expect(err).ToNot(HaveOccurred()) + Expect(sql).To(ContainSubstring("MATCH")) + }) + + It("returns ftsSearch with correct table names and MATCH expression", func() { + strategy := newFTSSearch("media_file", "beatles") + fts, ok := strategy.(*ftsSearch) + Expect(ok).To(BeTrue()) + Expect(fts.tableName).To(Equal("media_file")) + Expect(fts.ftsTable).To(Equal("media_file_fts")) + Expect(fts.matchExpr).To(HavePrefix("{title album artist album_artist")) + Expect(fts.matchExpr).To(ContainSubstring("beatles*")) + }) + + It("ToSql generates rowid IN subquery with MATCH (fallback path)", func() { + strategy := newFTSSearch("media_file", "beatles") + sql, args, err := strategy.ToSql() Expect(err).ToNot(HaveOccurred()) Expect(sql).To(ContainSubstring("media_file.rowid IN")) Expect(sql).To(ContainSubstring("media_file_fts")) Expect(sql).To(ContainSubstring("MATCH")) Expect(args).To(HaveLen(1)) - Expect(args[0]).To(HavePrefix("{title album artist album_artist")) - Expect(args[0]).To(ContainSubstring("beatles*")) }) It("generates correct FTS table name per entity", func() { for _, table := range []string{"media_file", "album", "artist"} { - expr := ftsSearchExpr(table, "test") - sql, _, err := expr.ToSql() - Expect(err).ToNot(HaveOccurred()) - Expect(sql).To(ContainSubstring(table + ".rowid IN")) - Expect(sql).To(ContainSubstring(table + "_fts")) + strategy := newFTSSearch(table, "test") + fts, ok := strategy.(*ftsSearch) + Expect(ok).To(BeTrue()) + Expect(fts.tableName).To(Equal(table)) + Expect(fts.ftsTable).To(Equal(table + "_fts")) } }) + It("builds bm25() rank expression with column weights", func() { + strategy := newFTSSearch("media_file", "beatles") + fts, ok := strategy.(*ftsSearch) + Expect(ok).To(BeTrue()) + Expect(fts.rankExpr).To(HavePrefix("bm25(media_file_fts,")) + Expect(fts.rankExpr).To(ContainSubstring("10.0")) + + strategy = newFTSSearch("artist", "beatles") + fts, ok = strategy.(*ftsSearch) + Expect(ok).To(BeTrue()) + Expect(fts.rankExpr).To(HavePrefix("bm25(artist_fts,")) + }) + + It("falls back to ftsTable.rank for unknown tables", func() { + strategy := newFTSSearch("unknown_table", "test") + fts, ok := strategy.(*ftsSearch) + Expect(ok).To(BeTrue()) + Expect(fts.rankExpr).To(Equal("unknown_table_fts.rank")) + }) + It("wraps query with column filter for known tables", func() { - expr := ftsSearchExpr("artist", "Beatles") - _, args, err := expr.ToSql() - Expect(err).ToNot(HaveOccurred()) - Expect(args[0]).To(Equal("{name sort_artist_name search_normalized} : (Beatles*)")) + strategy := newFTSSearch("artist", "Beatles") + fts, ok := strategy.(*ftsSearch) + Expect(ok).To(BeTrue()) + Expect(fts.matchExpr).To(Equal("{name sort_artist_name search_normalized} : (Beatles*)")) }) It("passes query without column filter for unknown tables", func() { - expr := ftsSearchExpr("unknown_table", "test") - _, args, err := expr.ToSql() - Expect(err).ToNot(HaveOccurred()) - Expect(args[0]).To(Equal("test*")) + strategy := newFTSSearch("unknown_table", "test") + fts, ok := strategy.(*ftsSearch) + Expect(ok).To(BeTrue()) + Expect(fts.matchExpr).To(Equal("test*")) }) It("preserves phrase queries inside column filter", func() { - expr := ftsSearchExpr("media_file", `"the beatles"`) - _, args, err := expr.ToSql() - Expect(err).ToNot(HaveOccurred()) - Expect(args[0]).To(ContainSubstring(`"the beatles"`)) + strategy := newFTSSearch("media_file", `"the beatles"`) + fts, ok := strategy.(*ftsSearch) + Expect(ok).To(BeTrue()) + Expect(fts.matchExpr).To(ContainSubstring(`"the beatles"`)) }) It("preserves prefix queries inside column filter", func() { - expr := ftsSearchExpr("media_file", "beat*") - _, args, err := expr.ToSql() - Expect(err).ToNot(HaveOccurred()) - Expect(args[0]).To(ContainSubstring("beat*")) + strategy := newFTSSearch("media_file", "beat*") + fts, ok := strategy.(*ftsSearch) + Expect(ok).To(BeTrue()) + Expect(fts.matchExpr).To(ContainSubstring("beat*")) }) It("falls back to LIKE search for punctuation-only query", func() { - expr := ftsSearchExpr("media_file", "!!!!!!!") - Expect(expr).ToNot(BeNil()) - sql, args, err := expr.ToSql() + strategy := newFTSSearch("media_file", "!!!!!!!") + Expect(strategy).ToNot(BeNil()) + _, ok := strategy.(*ftsSearch) + Expect(ok).To(BeFalse(), "punctuation-only should fall back to LIKE, not FTS") + sql, args, err := strategy.ToSql() Expect(err).ToNot(HaveOccurred()) Expect(sql).To(ContainSubstring("LIKE")) Expect(args).To(ContainElement("%!!!!!!!%")) }) + It("falls back to LIKE search for degraded query (special chars stripped leaving short tokens)", func() { + strategy := newFTSSearch("album", "1+") + Expect(strategy).ToNot(BeNil()) + _, ok := strategy.(*ftsSearch) + Expect(ok).To(BeFalse(), "degraded query should fall back to LIKE, not FTS") + sql, args, err := strategy.ToSql() + Expect(err).ToNot(HaveOccurred()) + Expect(sql).To(ContainSubstring("LIKE")) + Expect(args).To(ContainElement("%1+%")) + }) + It("returns nil for empty string even with LIKE fallback", func() { - Expect(ftsSearchExpr("media_file", "")).To(BeNil()) - Expect(ftsSearchExpr("media_file", " ")).To(BeNil()) + Expect(newFTSSearch("media_file", "")).To(BeNil()) + Expect(newFTSSearch("media_file", " ")).To(BeNil()) }) It("returns nil for empty quoted phrase", func() { - Expect(ftsSearchExpr("media_file", `""`)).To(BeNil()) + Expect(newFTSSearch("media_file", `""`)).To(BeNil()) }) }) @@ -229,7 +322,7 @@ var _ = Describe("FTS5 Integration Search", func() { Describe("MediaFile search", func() { It("finds media files by title", func() { - results, err := mr.Search("Radioactivity", 0, 10) + results, err := mr.Search("Radioactivity", model.QueryOptions{Max: 10}) Expect(err).ToNot(HaveOccurred()) Expect(results).To(HaveLen(1)) Expect(results[0].Title).To(Equal("Radioactivity")) @@ -237,7 +330,7 @@ var _ = Describe("FTS5 Integration Search", func() { }) It("finds media files by artist name", func() { - results, err := mr.Search("Beatles", 0, 10) + results, err := mr.Search("Beatles", model.QueryOptions{Max: 10}) Expect(err).ToNot(HaveOccurred()) Expect(results).To(HaveLen(3)) for _, r := range results { @@ -248,7 +341,7 @@ var _ = Describe("FTS5 Integration Search", func() { Describe("Album search", func() { It("finds albums by name", func() { - results, err := alr.Search("Sgt Peppers", 0, 10) + results, err := alr.Search("Sgt Peppers", model.QueryOptions{Max: 10}) Expect(err).ToNot(HaveOccurred()) Expect(results).To(HaveLen(1)) Expect(results[0].Name).To(Equal("Sgt Peppers")) @@ -256,7 +349,7 @@ var _ = Describe("FTS5 Integration Search", func() { }) It("finds albums with multi-word search", func() { - results, err := alr.Search("Abbey Road", 0, 10) + results, err := alr.Search("Abbey Road", model.QueryOptions{Max: 10}) Expect(err).ToNot(HaveOccurred()) Expect(results).To(HaveLen(2)) }) @@ -264,7 +357,7 @@ var _ = Describe("FTS5 Integration Search", func() { Describe("Artist search", func() { It("finds artists by name", func() { - results, err := arr.Search("Kraftwerk", 0, 10) + results, err := arr.Search("Kraftwerk", model.QueryOptions{Max: 10}) Expect(err).ToNot(HaveOccurred()) Expect(results).To(HaveLen(1)) Expect(results[0].Name).To(Equal("Kraftwerk")) @@ -274,7 +367,7 @@ var _ = Describe("FTS5 Integration Search", func() { Describe("CJK search", func() { It("finds media files by CJK title", func() { - results, err := mr.Search("プラチナ", 0, 10) + results, err := mr.Search("プラチナ", model.QueryOptions{Max: 10}) Expect(err).ToNot(HaveOccurred()) Expect(results).To(HaveLen(1)) Expect(results[0].Title).To(Equal("プラチナ・ジェット")) @@ -282,14 +375,14 @@ var _ = Describe("FTS5 Integration Search", func() { }) It("finds media files by CJK artist name", func() { - results, err := mr.Search("シートベルツ", 0, 10) + results, err := mr.Search("シートベルツ", model.QueryOptions{Max: 10}) Expect(err).ToNot(HaveOccurred()) Expect(results).To(HaveLen(1)) Expect(results[0].Artist).To(Equal("シートベルツ")) }) It("finds albums by CJK artist name", func() { - results, err := alr.Search("シートベルツ", 0, 10) + results, err := alr.Search("シートベルツ", model.QueryOptions{Max: 10}) Expect(err).ToNot(HaveOccurred()) Expect(results).To(HaveLen(1)) Expect(results[0].Name).To(Equal("COWBOY BEBOP")) @@ -297,7 +390,7 @@ var _ = Describe("FTS5 Integration Search", func() { }) It("finds artists by CJK name", func() { - results, err := arr.Search("シートベルツ", 0, 10) + results, err := arr.Search("シートベルツ", model.QueryOptions{Max: 10}) Expect(err).ToNot(HaveOccurred()) Expect(results).To(HaveLen(1)) Expect(results[0].Name).To(Equal("シートベルツ")) @@ -307,7 +400,7 @@ var _ = Describe("FTS5 Integration Search", func() { Describe("Album version search", func() { It("finds albums by version tag via FTS", func() { - results, err := alr.Search("Deluxe", 0, 10) + results, err := alr.Search("Deluxe", model.QueryOptions{Max: 10}) Expect(err).ToNot(HaveOccurred()) Expect(results).To(HaveLen(1)) Expect(results[0].ID).To(Equal(albumWithVersion.ID)) @@ -316,7 +409,7 @@ var _ = Describe("FTS5 Integration Search", func() { Describe("Punctuation-only search", func() { It("finds media files with punctuation-only title", func() { - results, err := mr.Search("!!!!!!!", 0, 10) + results, err := mr.Search("!!!!!!!", model.QueryOptions{Max: 10}) Expect(err).ToNot(HaveOccurred()) Expect(results).To(HaveLen(1)) Expect(results[0].Title).To(Equal("!!!!!!!")) @@ -324,15 +417,19 @@ var _ = Describe("FTS5 Integration Search", func() { }) }) - Describe("Legacy backend fallback", func() { - It("returns results using legacy LIKE-based search when configured", func() { - DeferCleanup(configtest.SetupConfig()) - conf.Server.Search.Backend = "legacy" - - results, err := mr.Search("Radioactivity", 0, 10) + Describe("Single-character search (doSearch min-length guard)", func() { + It("returns empty results for single-char query via Search", func() { + results, err := mr.Search("a", model.QueryOptions{Max: 10}) Expect(err).ToNot(HaveOccurred()) - Expect(results).To(HaveLen(1)) - Expect(results[0].Title).To(Equal("Radioactivity")) + Expect(results).To(BeEmpty(), "doSearch should reject single-char queries") + }) + }) + + Describe("Max=0 means no limit (regression: must not produce LIMIT 0)", func() { + It("returns results with Max=0", func() { + results, err := mr.Search("Beatles", model.QueryOptions{Max: 0}) + Expect(err).ToNot(HaveOccurred()) + Expect(results).ToNot(BeEmpty(), "Max=0 should mean no limit, not LIMIT 0") }) }) }) diff --git a/persistence/sql_search_like.go b/persistence/sql_search_like.go new file mode 100644 index 00000000..769a911d --- /dev/null +++ b/persistence/sql_search_like.go @@ -0,0 +1,106 @@ +package persistence + +import ( + "strings" + + . "github.com/Masterminds/squirrel" + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/log" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/utils/str" +) + +// likeSearch implements searchStrategy using LIKE-based SQL filters. +// Used for legacy full_text searches, CJK fallback, and punctuation-only fallback. +type likeSearch struct { + filter Sqlizer +} + +func (s *likeSearch) ToSql() (string, []interface{}, error) { + return s.filter.ToSql() +} + +func (s *likeSearch) execute(r sqlRepository, sq SelectBuilder, dest any, cfg searchConfig, options model.QueryOptions) error { + sq = sq.Where(s.filter) + sq = sq.OrderBy(cfg.OrderBy...) + return r.queryAll(sq, dest, options) +} + +// newLegacySearch creates a LIKE search against the full_text column. +// Returns nil when the query produces no searchable tokens. +func newLegacySearch(tableName, query string) searchStrategy { + filter := legacySearchExpr(tableName, query) + if filter == nil { + return nil + } + return &likeSearch{filter: filter} +} + +// newLikeSearch creates a LIKE search against core entity columns (CJK, punctuation fallback). +// No minimum length is enforced, since single CJK characters are meaningful words. +// Returns nil when the query produces no searchable tokens. +func newLikeSearch(tableName, query string) searchStrategy { + filter := likeSearchExpr(tableName, query) + if filter == nil { + return nil + } + return &likeSearch{filter: filter} +} + +// legacySearchExpr generates LIKE-based search filters against the full_text column. +// This is the original search implementation, used when Search.Backend="legacy". +func legacySearchExpr(tableName string, s string) Sqlizer { + q := str.SanitizeStrings(s) + if q == "" { + log.Trace("Search using legacy backend, query is empty", "table", tableName) + return nil + } + var sep string + if !conf.Server.Search.FullString { + sep = " " + } + parts := strings.Split(q, " ") + filters := And{} + for _, part := range parts { + filters = append(filters, Like{tableName + ".full_text": "%" + sep + part + "%"}) + } + log.Trace("Search using legacy backend", "query", filters, "table", tableName) + return filters +} + +// likeSearchColumns defines the core columns to search with LIKE queries. +// These are the primary user-visible fields for each entity type. +// Used as a fallback when FTS5 cannot handle the query (e.g., CJK text, punctuation-only input). +var likeSearchColumns = map[string][]string{ + "media_file": {"title", "album", "artist", "album_artist"}, + "album": {"name", "album_artist"}, + "artist": {"name"}, +} + +// likeSearchExpr generates LIKE-based search filters against core columns. +// Each word in the query must match at least one column (AND between words), +// and each word can match any column (OR within a word). +// Used as a fallback when FTS5 cannot handle the query (e.g., CJK text, punctuation-only input). +func likeSearchExpr(tableName string, s string) Sqlizer { + s = strings.TrimSpace(s) + if s == "" { + log.Trace("Search using LIKE backend, query is empty", "table", tableName) + return nil + } + columns, ok := likeSearchColumns[tableName] + if !ok { + log.Trace("Search using LIKE backend, couldn't find columns for this table", "table", tableName) + return nil + } + words := strings.Fields(s) + wordFilters := And{} + for _, word := range words { + colFilters := Or{} + for _, col := range columns { + colFilters = append(colFilters, Like{tableName + "." + col: "%" + word + "%"}) + } + wordFilters = append(wordFilters, colFilters) + } + log.Trace("Search using LIKE backend", "query", wordFilters, "table", tableName) + return wordFilters +} diff --git a/persistence/sql_search_like_test.go b/persistence/sql_search_like_test.go new file mode 100644 index 00000000..8ee4ef93 --- /dev/null +++ b/persistence/sql_search_like_test.go @@ -0,0 +1,134 @@ +package persistence + +import ( + "context" + + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/conf/configtest" + "github.com/navidrome/navidrome/log" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/request" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("newLegacySearch", func() { + It("returns non-nil for single-character query", func() { + strategy := newLegacySearch("media_file", "a") + Expect(strategy).ToNot(BeNil(), "single-char queries must not be rejected; min-length is enforced in doSearch, not here") + sql, _, err := strategy.ToSql() + Expect(err).ToNot(HaveOccurred()) + Expect(sql).To(ContainSubstring("LIKE")) + }) +}) + +var _ = Describe("legacySearchExpr", func() { + It("returns nil for empty query", func() { + Expect(legacySearchExpr("media_file", "")).To(BeNil()) + }) + + It("generates LIKE filter for single word", func() { + expr := legacySearchExpr("media_file", "beatles") + sql, args, err := expr.ToSql() + Expect(err).ToNot(HaveOccurred()) + Expect(sql).To(ContainSubstring("media_file.full_text LIKE")) + Expect(args).To(ContainElement("% beatles%")) + }) + + It("generates AND of LIKE filters for multiple words", func() { + expr := legacySearchExpr("media_file", "abbey road") + sql, args, err := expr.ToSql() + Expect(err).ToNot(HaveOccurred()) + Expect(sql).To(ContainSubstring("AND")) + Expect(args).To(HaveLen(2)) + }) +}) + +var _ = Describe("likeSearchExpr", func() { + It("returns nil for empty query", func() { + Expect(likeSearchExpr("media_file", "")).To(BeNil()) + }) + + It("returns nil for whitespace-only query", func() { + Expect(likeSearchExpr("media_file", " ")).To(BeNil()) + }) + + It("generates LIKE filters against core columns for single CJK word", func() { + expr := likeSearchExpr("media_file", "周杰伦") + sql, args, err := expr.ToSql() + Expect(err).ToNot(HaveOccurred()) + // Should have OR between columns for the single word + Expect(sql).To(ContainSubstring("OR")) + Expect(sql).To(ContainSubstring("media_file.title LIKE")) + Expect(sql).To(ContainSubstring("media_file.album LIKE")) + Expect(sql).To(ContainSubstring("media_file.artist LIKE")) + Expect(sql).To(ContainSubstring("media_file.album_artist LIKE")) + Expect(args).To(HaveLen(4)) + for _, arg := range args { + Expect(arg).To(Equal("%周杰伦%")) + } + }) + + It("generates AND of OR groups for multi-word query", func() { + expr := likeSearchExpr("media_file", "周杰伦 greatest") + sql, args, err := expr.ToSql() + Expect(err).ToNot(HaveOccurred()) + // Two groups AND'd together, each with 4 columns OR'd + Expect(sql).To(ContainSubstring("AND")) + Expect(args).To(HaveLen(8)) + }) + + It("uses correct columns for album table", func() { + expr := likeSearchExpr("album", "周杰伦") + sql, args, err := expr.ToSql() + Expect(err).ToNot(HaveOccurred()) + Expect(sql).To(ContainSubstring("album.name LIKE")) + Expect(sql).To(ContainSubstring("album.album_artist LIKE")) + Expect(args).To(HaveLen(2)) + }) + + It("uses correct columns for artist table", func() { + expr := likeSearchExpr("artist", "周杰伦") + sql, args, err := expr.ToSql() + Expect(err).ToNot(HaveOccurred()) + Expect(sql).To(ContainSubstring("artist.name LIKE")) + Expect(args).To(HaveLen(1)) + }) + + It("returns nil for unknown table", func() { + Expect(likeSearchExpr("unknown_table", "周杰伦")).To(BeNil()) + }) +}) + +var _ = Describe("Legacy Integration Search", func() { + var mr model.MediaFileRepository + + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + conf.Server.Search.Backend = "legacy" + + ctx := log.NewContext(context.TODO()) + ctx = request.WithUser(ctx, adminUser) + conn := GetDBXBuilder() + mr = NewMediaFileRepository(ctx, conn) + }) + + It("returns results using legacy LIKE-based search", func() { + results, err := mr.Search("Radioactivity", model.QueryOptions{Max: 10}) + Expect(err).ToNot(HaveOccurred()) + Expect(results).To(HaveLen(1)) + Expect(results[0].Title).To(Equal("Radioactivity")) + }) + + It("returns empty results for single-char query (doSearch min-length guard)", func() { + results, err := mr.Search("a", model.QueryOptions{Max: 10}) + Expect(err).ToNot(HaveOccurred()) + Expect(results).To(BeEmpty(), "doSearch should reject single-char queries") + }) + + It("returns results with Max=0 (regression: must not produce LIMIT 0)", func() { + results, err := mr.Search("Beatles", model.QueryOptions{Max: 0}) + Expect(err).ToNot(HaveOccurred()) + Expect(results).ToNot(BeEmpty(), "Max=0 should mean no limit, not LIMIT 0") + }) +}) diff --git a/persistence/sql_search_test.go b/persistence/sql_search_test.go index b59570af..68ee205b 100644 --- a/persistence/sql_search_test.go +++ b/persistence/sql_search_test.go @@ -14,98 +14,99 @@ var _ = Describe("sqlRepository", func() { }) }) - Describe("legacySearchExpr", func() { - It("returns nil for empty query", func() { - Expect(legacySearchExpr("media_file", "")).To(BeNil()) - }) - - It("generates LIKE filter for single word", func() { - expr := legacySearchExpr("media_file", "beatles") - sql, args, err := expr.ToSql() - Expect(err).ToNot(HaveOccurred()) - Expect(sql).To(ContainSubstring("media_file.full_text LIKE")) - Expect(args).To(ContainElement("% beatles%")) - }) - - It("generates AND of LIKE filters for multiple words", func() { - expr := legacySearchExpr("media_file", "abbey road") - sql, args, err := expr.ToSql() - Expect(err).ToNot(HaveOccurred()) - Expect(sql).To(ContainSubstring("AND")) - Expect(args).To(HaveLen(2)) - }) - }) - - Describe("getSearchExpr", func() { - It("returns ftsSearchExpr by default", func() { + Describe("getSearchStrategy", func() { + It("returns FTS strategy by default", func() { DeferCleanup(configtest.SetupConfig()) conf.Server.Search.Backend = "fts" conf.Server.Search.FullString = false - expr := getSearchExpr()("media_file", "test") - sql, _, err := expr.ToSql() + strategy := getSearchStrategy("media_file", "test") + Expect(strategy).ToNot(BeNil()) + sql, _, err := strategy.ToSql() Expect(err).ToNot(HaveOccurred()) Expect(sql).To(ContainSubstring("MATCH")) }) - It("returns legacySearchExpr when SearchBackend is legacy", func() { + It("returns legacy LIKE strategy when SearchBackend is legacy", func() { DeferCleanup(configtest.SetupConfig()) conf.Server.Search.Backend = "legacy" conf.Server.Search.FullString = false - expr := getSearchExpr()("media_file", "test") - sql, _, err := expr.ToSql() + strategy := getSearchStrategy("media_file", "test") + Expect(strategy).ToNot(BeNil()) + sql, _, err := strategy.ToSql() Expect(err).ToNot(HaveOccurred()) Expect(sql).To(ContainSubstring("LIKE")) }) - It("falls back to legacySearchExpr when SearchFullString is enabled", func() { + It("falls back to legacy LIKE strategy when SearchFullString is enabled", func() { DeferCleanup(configtest.SetupConfig()) conf.Server.Search.Backend = "fts" conf.Server.Search.FullString = true - expr := getSearchExpr()("media_file", "test") - sql, _, err := expr.ToSql() + strategy := getSearchStrategy("media_file", "test") + Expect(strategy).ToNot(BeNil()) + sql, _, err := strategy.ToSql() Expect(err).ToNot(HaveOccurred()) Expect(sql).To(ContainSubstring("LIKE")) }) - It("routes CJK queries to likeSearchExpr instead of ftsSearchExpr", func() { + It("routes CJK queries to LIKE strategy instead of FTS", func() { DeferCleanup(configtest.SetupConfig()) conf.Server.Search.Backend = "fts" conf.Server.Search.FullString = false - expr := getSearchExpr()("media_file", "周杰伦") - sql, _, err := expr.ToSql() + strategy := getSearchStrategy("media_file", "周杰伦") + Expect(strategy).ToNot(BeNil()) + sql, _, err := strategy.ToSql() Expect(err).ToNot(HaveOccurred()) // CJK should use LIKE, not MATCH Expect(sql).To(ContainSubstring("LIKE")) Expect(sql).NotTo(ContainSubstring("MATCH")) }) - It("routes non-CJK queries to ftsSearchExpr", func() { + It("routes non-CJK queries to FTS strategy", func() { DeferCleanup(configtest.SetupConfig()) conf.Server.Search.Backend = "fts" conf.Server.Search.FullString = false - expr := getSearchExpr()("media_file", "beatles") - sql, _, err := expr.ToSql() + strategy := getSearchStrategy("media_file", "beatles") + Expect(strategy).ToNot(BeNil()) + sql, _, err := strategy.ToSql() Expect(err).ToNot(HaveOccurred()) Expect(sql).To(ContainSubstring("MATCH")) }) + It("returns non-nil for single-character query (no min-length in strategy)", func() { + DeferCleanup(configtest.SetupConfig()) + conf.Server.Search.Backend = "fts" + conf.Server.Search.FullString = false + + strategy := getSearchStrategy("media_file", "a") + Expect(strategy).ToNot(BeNil(), "single-char queries must be accepted by strategies (min-length is enforced in doSearch)") + }) + + It("returns non-nil for single-character query with legacy backend", func() { + DeferCleanup(configtest.SetupConfig()) + conf.Server.Search.Backend = "legacy" + conf.Server.Search.FullString = false + + strategy := getSearchStrategy("media_file", "a") + Expect(strategy).ToNot(BeNil(), "single-char queries must be accepted by legacy strategy (min-length is enforced in doSearch)") + }) + It("uses legacy for CJK when SearchBackend is legacy", func() { DeferCleanup(configtest.SetupConfig()) conf.Server.Search.Backend = "legacy" conf.Server.Search.FullString = false - expr := getSearchExpr()("media_file", "周杰伦") - sql, _, err := expr.ToSql() + strategy := getSearchStrategy("media_file", "周杰伦") + Expect(strategy).ToNot(BeNil()) + sql, _, err := strategy.ToSql() Expect(err).ToNot(HaveOccurred()) // Legacy should still use full_text column LIKE Expect(sql).To(ContainSubstring("LIKE")) Expect(sql).To(ContainSubstring("full_text")) }) }) - }) diff --git a/server/e2e/e2e_suite_test.go b/server/e2e/e2e_suite_test.go index 5981f4a8..0e0ca606 100644 --- a/server/e2e/e2e_suite_test.go +++ b/server/e2e/e2e_suite_test.go @@ -115,6 +115,7 @@ func buildTestFS() storagetest.FakeFS { ledZepIV := template(_t{"albumartist": "Led Zeppelin", "artist": "Led Zeppelin", "album": "IV", "year": 1971, "genre": "Rock"}) kindOfBlue := template(_t{"albumartist": "Miles Davis", "artist": "Miles Davis", "album": "Kind of Blue", "year": 1959, "genre": "Jazz"}) popTrack := template(_t{"albumartist": "Various", "artist": "Various", "album": "Pop", "year": 2020, "genre": "Pop"}) + cowboyBebop := template(_t{"albumartist": "シートベルツ", "artist": "シートベルツ", "album": "COWBOY BEBOP", "year": 1998, "genre": "Jazz"}) return createFS(fstest.MapFS{ // Rock / The Beatles / Abbey Road (with MBIDs) @@ -132,6 +133,8 @@ func buildTestFS() storagetest.FakeFS { "Jazz/Miles Davis/Kind of Blue/01 - So What.mp3": kindOfBlue(track(1, "So What")), // Pop (standalone track, no MBIDs) "Pop/01 - Standalone Track.mp3": popTrack(track(1, "Standalone Track")), + // CJK / シートベルツ / COWBOY BEBOP (Japanese artist, for CJK search tests) + "CJK/シートベルツ/COWBOY BEBOP/01 - プラチナ・ジェット.mp3": cowboyBebop(track(1, "プラチナ・ジェット")), // _empty folder (directory with no audio) "_empty/.keep": &fstest.MapFile{Data: []byte{}, ModTime: time.Now()}, }) diff --git a/server/e2e/subsonic_album_lists_test.go b/server/e2e/subsonic_album_lists_test.go index f7a5af17..d3d24a7c 100644 --- a/server/e2e/subsonic_album_lists_test.go +++ b/server/e2e/subsonic_album_lists_test.go @@ -19,7 +19,7 @@ var _ = Describe("Album List Endpoints", func() { Expect(resp.Status).To(Equal(responses.StatusOK)) Expect(resp.AlbumList).ToNot(BeNil()) - Expect(resp.AlbumList.Album).To(HaveLen(5)) + Expect(resp.AlbumList.Album).To(HaveLen(6)) }) It("type=alphabeticalByName sorts albums by name", func() { @@ -27,13 +27,14 @@ var _ = Describe("Album List Endpoints", func() { Expect(resp.AlbumList).ToNot(BeNil()) albums := resp.AlbumList.Album - Expect(albums).To(HaveLen(5)) - // Verify alphabetical order: Abbey Road, Help!, IV, Kind of Blue, Pop + Expect(albums).To(HaveLen(6)) + // Verify alphabetical order: Abbey Road, COWBOY BEBOP, Help!, IV, Kind of Blue, Pop Expect(albums[0].Title).To(Equal("Abbey Road")) - Expect(albums[1].Title).To(Equal("Help!")) - Expect(albums[2].Title).To(Equal("IV")) - Expect(albums[3].Title).To(Equal("Kind of Blue")) - Expect(albums[4].Title).To(Equal("Pop")) + Expect(albums[1].Title).To(Equal("COWBOY BEBOP")) + Expect(albums[2].Title).To(Equal("Help!")) + Expect(albums[3].Title).To(Equal("IV")) + Expect(albums[4].Title).To(Equal("Kind of Blue")) + Expect(albums[5].Title).To(Equal("Pop")) }) It("type=alphabeticalByArtist sorts albums by artist name", func() { @@ -41,29 +42,32 @@ var _ = Describe("Album List Endpoints", func() { Expect(resp.AlbumList).ToNot(BeNil()) albums := resp.AlbumList.Album - Expect(albums).To(HaveLen(5)) + Expect(albums).To(HaveLen(6)) // Articles like "The" are stripped for sorting, so "The Beatles" sorts as "Beatles" - // Non-compilations first: Beatles (x2), Led Zeppelin, Miles Davis, then compilations: Various + // Non-compilations first: Beatles (x2), Led Zeppelin, Miles Davis, then compilations: Various, then CJK: シートベルツ Expect(albums[0].Artist).To(Equal("The Beatles")) Expect(albums[1].Artist).To(Equal("The Beatles")) Expect(albums[2].Artist).To(Equal("Led Zeppelin")) Expect(albums[3].Artist).To(Equal("Miles Davis")) Expect(albums[4].Artist).To(Equal("Various")) + Expect(albums[5].Artist).To(Equal("シートベルツ")) }) It("type=random returns albums", func() { resp := doReq("getAlbumList", "type", "random") Expect(resp.AlbumList).ToNot(BeNil()) - Expect(resp.AlbumList.Album).To(HaveLen(5)) + Expect(resp.AlbumList.Album).To(HaveLen(6)) }) It("type=byGenre filters by genre parameter", func() { resp := doReq("getAlbumList", "type", "byGenre", "genre", "Jazz") Expect(resp.AlbumList).ToNot(BeNil()) - Expect(resp.AlbumList.Album).To(HaveLen(1)) - Expect(resp.AlbumList.Album[0].Title).To(Equal("Kind of Blue")) + Expect(resp.AlbumList.Album).To(HaveLen(2)) + for _, a := range resp.AlbumList.Album { + Expect(a.Genre).To(Equal("Jazz")) + } }) It("type=byYear filters by fromYear/toYear range", func() { @@ -184,7 +188,7 @@ var _ = Describe("Album List Endpoints", func() { Expect(resp.Status).To(Equal(responses.StatusOK)) Expect(resp.AlbumList2).ToNot(BeNil()) albums := resp.AlbumList2.Album - Expect(albums).To(HaveLen(5)) + Expect(albums).To(HaveLen(6)) // Verify AlbumID3 format fields Expect(albums[0].Name).To(Equal("Abbey Road")) Expect(albums[0].Id).ToNot(BeEmpty()) @@ -195,7 +199,7 @@ var _ = Describe("Album List Endpoints", func() { resp := doReq("getAlbumList2", "type", "newest") Expect(resp.AlbumList2).ToNot(BeNil()) - Expect(resp.AlbumList2.Album).To(HaveLen(5)) + Expect(resp.AlbumList2.Album).To(HaveLen(6)) }) }) @@ -240,7 +244,7 @@ var _ = Describe("Album List Endpoints", func() { Expect(resp.Status).To(Equal(responses.StatusOK)) Expect(resp.RandomSongs).ToNot(BeNil()) Expect(resp.RandomSongs.Songs).ToNot(BeEmpty()) - Expect(len(resp.RandomSongs.Songs)).To(BeNumerically("<=", 6)) + Expect(resp.RandomSongs.Songs).To(HaveLen(7)) }) It("respects size parameter", func() { @@ -254,8 +258,10 @@ var _ = Describe("Album List Endpoints", func() { resp := doReq("getRandomSongs", "size", "500", "genre", "Jazz") Expect(resp.RandomSongs).ToNot(BeNil()) - Expect(resp.RandomSongs.Songs).To(HaveLen(1)) - Expect(resp.RandomSongs.Songs[0].Genre).To(Equal("Jazz")) + Expect(resp.RandomSongs.Songs).To(HaveLen(2)) + for _, s := range resp.RandomSongs.Songs { + Expect(s.Genre).To(Equal("Jazz")) + } }) }) diff --git a/server/e2e/subsonic_browsing_test.go b/server/e2e/subsonic_browsing_test.go index 403e1ba6..5a2da873 100644 --- a/server/e2e/subsonic_browsing_test.go +++ b/server/e2e/subsonic_browsing_test.go @@ -327,8 +327,8 @@ var _ = Describe("Browsing Endpoints", func() { } } Expect(jazzGenre).ToNot(BeNil()) - Expect(jazzGenre.SongCount).To(Equal(int32(1))) - Expect(jazzGenre.AlbumCount).To(Equal(int32(1))) + Expect(jazzGenre.SongCount).To(Equal(int32(2))) + Expect(jazzGenre.AlbumCount).To(Equal(int32(2))) }) It("reports correct song and album counts for Pop", func() { diff --git a/server/e2e/subsonic_multilibrary_test.go b/server/e2e/subsonic_multilibrary_test.go index 1eb04d41..aa4a0c62 100644 --- a/server/e2e/subsonic_multilibrary_test.go +++ b/server/e2e/subsonic_multilibrary_test.go @@ -141,7 +141,7 @@ var _ = Describe("Multi-Library Support", Ordered, func() { resp := doReqWithUser(adminWithLibs, "getAlbumList", "type", "alphabeticalByName", "musicFolderId", fmt.Sprintf("%d", lib.ID)) Expect(resp.AlbumList).ToNot(BeNil()) - Expect(resp.AlbumList.Album).To(HaveLen(5)) + Expect(resp.AlbumList.Album).To(HaveLen(6)) for _, a := range resp.AlbumList.Album { Expect(a.Title).ToNot(Equal("Symphony No. 9")) } @@ -275,5 +275,21 @@ var _ = Describe("Multi-Library Support", Ordered, func() { Expect(artistNames).To(ContainElements("The Beatles", "Led Zeppelin", "Miles Davis")) Expect(artistNames).ToNot(ContainElement("Ludwig van Beethoven")) }) + + It("non-admin user search returns only their library's content", func() { + resp := doReqWithUser(userLib1Only, "search3", "query", "Beethoven") + + Expect(resp.SearchResult3).ToNot(BeNil()) + Expect(resp.SearchResult3.Artist).To(BeEmpty(), "userLib1Only should not see Beethoven (lib2)") + Expect(resp.SearchResult3.Album).To(BeEmpty()) + Expect(resp.SearchResult3.Song).To(BeEmpty()) + }) + + It("non-admin user search finds content from their library", func() { + resp := doReqWithUser(userLib1Only, "search3", "query", "Beatles") + + Expect(resp.SearchResult3).ToNot(BeNil()) + Expect(resp.SearchResult3.Artist).ToNot(BeEmpty(), "userLib1Only should find Beatles (lib1)") + }) }) }) diff --git a/server/e2e/subsonic_searching_test.go b/server/e2e/subsonic_searching_test.go index d0447ecd..bfcbbc8e 100644 --- a/server/e2e/subsonic_searching_test.go +++ b/server/e2e/subsonic_searching_test.go @@ -2,6 +2,8 @@ package e2e import ( "github.com/google/uuid" + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/conf/configtest" "github.com/navidrome/navidrome/server/subsonic/responses" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -113,9 +115,9 @@ var _ = Describe("Search Endpoints", func() { Expect(resp.Status).To(Equal(responses.StatusOK)) Expect(resp.SearchResult3).ToNot(BeNil()) - Expect(resp.SearchResult3.Artist).To(HaveLen(4)) - Expect(resp.SearchResult3.Album).To(HaveLen(5)) - Expect(resp.SearchResult3.Song).To(HaveLen(6)) + Expect(resp.SearchResult3.Artist).To(HaveLen(5)) + Expect(resp.SearchResult3.Album).To(HaveLen(6)) + Expect(resp.SearchResult3.Song).To(HaveLen(7)) }) It("finds across all entity types simultaneously", func() { @@ -217,5 +219,56 @@ var _ = Describe("Search Endpoints", func() { Expect(resp.SearchResult3.Song).To(BeEmpty()) }) }) + + Describe("CJK search", func() { + It("finds songs by CJK title", func() { + resp := doReq("search3", "query", "プラチナ") + + Expect(resp.Status).To(Equal(responses.StatusOK)) + Expect(resp.SearchResult3).ToNot(BeNil()) + Expect(resp.SearchResult3.Song).To(HaveLen(1)) + Expect(resp.SearchResult3.Song[0].Title).To(Equal("プラチナ・ジェット")) + }) + + It("finds artists by CJK name", func() { + resp := doReq("search3", "query", "シートベルツ") + + Expect(resp.Status).To(Equal(responses.StatusOK)) + Expect(resp.SearchResult3).ToNot(BeNil()) + Expect(resp.SearchResult3.Artist).To(HaveLen(1)) + Expect(resp.SearchResult3.Artist[0].Name).To(Equal("シートベルツ")) + }) + + It("finds albums by CJK artist name", func() { + resp := doReq("search3", "query", "シートベルツ") + + Expect(resp.Status).To(Equal(responses.StatusOK)) + Expect(resp.SearchResult3).ToNot(BeNil()) + Expect(resp.SearchResult3.Album).To(HaveLen(1)) + Expect(resp.SearchResult3.Album[0].Name).To(Equal("COWBOY BEBOP")) + }) + }) + + Describe("Legacy backend", func() { + It("returns results using legacy LIKE-based search when configured", func() { + DeferCleanup(configtest.SetupConfig()) + conf.Server.Search.Backend = "legacy" + + resp := doReq("search3", "query", "Beatles") + + Expect(resp.Status).To(Equal(responses.StatusOK)) + Expect(resp.SearchResult3).ToNot(BeNil()) + Expect(resp.SearchResult3.Artist).ToNot(BeEmpty()) + + found := false + for _, a := range resp.SearchResult3.Artist { + if a.Name == "The Beatles" { + found = true + break + } + } + Expect(found).To(BeTrue(), "expected to find artist 'The Beatles' with legacy backend") + }) + }) }) }) diff --git a/server/subsonic/searching.go b/server/subsonic/searching.go index 5a19e0a3..fd7e2958 100644 --- a/server/subsonic/searching.go +++ b/server/subsonic/searching.go @@ -42,17 +42,17 @@ func (api *Router) getSearchParams(r *http.Request) (*searchParams, error) { return sp, nil } -type searchFunc[T any] func(q string, offset int, size int, options ...model.QueryOptions) (T, error) +type searchFunc[T any] func(q string, 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 { +func callSearch[T any](ctx context.Context, s searchFunc[T], q string, options model.QueryOptions, result *T) func() error { return func() error { - if size == 0 { + if options.Max == 0 { return nil } typ := strings.TrimPrefix(reflect.TypeOf(*result).String(), "model.") var err error start := time.Now() - *result, err = s(q, offset, size, options...) + *result, err = s(q, options) if err != nil { log.Error(ctx, "Error searching "+typ, "query", q, "elapsed", time.Since(start), err) } else { @@ -66,27 +66,22 @@ func (api *Router) searchAll(ctx context.Context, sp *searchParams, musicFolderI start := time.Now() q := sanitize.Accents(strings.ToLower(strings.TrimSuffix(sp.query, "*"))) - // Create query options for library filtering - var options []model.QueryOptions - var artistOptions []model.QueryOptions + // Build options with offset/size/filters packed in + songOpts := model.QueryOptions{Max: sp.songCount, Offset: sp.songOffset} + albumOpts := model.QueryOptions{Max: sp.albumCount, Offset: sp.albumOffset} + artistOpts := model.QueryOptions{Max: sp.artistCount, Offset: sp.artistOffset} + if len(musicFolderIds) > 0 { - // For MediaFiles and Albums, use direct library_id filter - options = append(options, model.QueryOptions{ - Filters: Eq{"library_id": musicFolderIds}, - }) - // For Artists, use the repository's built-in library filtering mechanism - // which properly handles the library_artist table joins - // TODO Revisit library filtering in sql_base_repository.go - artistOptions = append(artistOptions, model.QueryOptions{ - Filters: Eq{"library_artist.library_id": musicFolderIds}, - }) + songOpts.Filters = Eq{"library_id": musicFolderIds} + albumOpts.Filters = Eq{"library_id": musicFolderIds} + artistOpts.Filters = Eq{"library_artist.library_id": musicFolderIds} } // Run searches in parallel g, ctx := errgroup.WithContext(ctx) - g.Go(callSearch(ctx, api.ds.MediaFile(ctx).Search, q, sp.songOffset, sp.songCount, &mediaFiles, options...)) - g.Go(callSearch(ctx, api.ds.Album(ctx).Search, q, sp.albumOffset, sp.albumCount, &albums, options...)) - g.Go(callSearch(ctx, api.ds.Artist(ctx).Search, q, sp.artistOffset, sp.artistCount, &artists, artistOptions...)) + g.Go(callSearch(ctx, api.ds.MediaFile(ctx).Search, q, songOpts, &mediaFiles)) + g.Go(callSearch(ctx, api.ds.Album(ctx).Search, q, albumOpts, &albums)) + g.Go(callSearch(ctx, api.ds.Artist(ctx).Search, q, artistOpts, &artists)) err := g.Wait() if err == nil { log.Debug(ctx, fmt.Sprintf("Search resulted in %d songs, %d albums and %d artists", diff --git a/tests/mock_album_repo.go b/tests/mock_album_repo.go index 8b5f5d9c..3428813f 100644 --- a/tests/mock_album_repo.go +++ b/tests/mock_album_repo.go @@ -119,7 +119,7 @@ func (m *MockAlbumRepo) UpdateExternalInfo(album *model.Album) error { return nil } -func (m *MockAlbumRepo) Search(q string, offset int, size int, options ...model.QueryOptions) (model.Albums, error) { +func (m *MockAlbumRepo) Search(q string, 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 6d4792f8..b7a6fb81 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, options ...model.QueryOptions) (model.Artists, error) { +func (m *MockArtistRepo) Search(q string, 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 2812fd50..1d4527c8 100644 --- a/tests/mock_mediafile_repo.go +++ b/tests/mock_mediafile_repo.go @@ -238,7 +238,7 @@ func (m *MockMediaFileRepo) NewInstance() any { return &model.MediaFile{} } -func (m *MockMediaFileRepo) Search(q string, offset int, size int, options ...model.QueryOptions) (model.MediaFiles, error) { +func (m *MockMediaFileRepo) Search(q string, options ...model.QueryOptions) (model.MediaFiles, error) { if len(options) > 0 { m.Options = options[0] }