diff --git a/model/searchable.go b/model/searchable.go index 631a11726..a64a0171c 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 58bfcca51..077e33d17 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 f801787d8..07824e21f 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 15340eeb4..90e449e8d 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 264be6f3f..394ca5b70 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 853480b32..7a04d79e4 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 27207c45d..02162387c 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 ea0f802af..32f418b8e 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 e5c245bdf..43965ebb7 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 ea70518b6..9eb01f0cf 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 e0fead8ae..337d54201 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 000000000..769a911d5 --- /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 000000000..8ee4ef93c --- /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 b59570af3..68ee205b4 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 5981f4a86..0e0ca606a 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 f7a5af173..d3d24a7c5 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 403e1ba6f..5a2da8737 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 1eb04d41c..aa4a0c626 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 d0447ecd7..bfcbbc8ee 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 5a19e0a3b..fd7e29587 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 8b5f5d9c1..3428813f6 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 6d4792f83..b7a6fb811 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 2812fd507..1d4527c88 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] }