From 1aa48de4ebf13646b05330399fa5d6e47744137d Mon Sep 17 00:00:00 2001 From: Deluan Date: Fri, 27 Jun 2025 18:20:30 -0400 Subject: [PATCH] feat: add musicFolderId parameter support to Search2 and Search3 endpoints Implemented musicFolderId parameter support for Subsonic API Search2 and Search3 endpoints, completing multi-library functionality across all Subsonic endpoints. Key changes: - Added musicFolderId parameter handling to Search2 and Search3 endpoints - Updated search logic to filter results by specified library or all accessible libraries when parameter not provided - Added proper error handling for invalid/inaccessible musicFolderId values - Refactored SearchableRepository interface to support library filtering with variadic QueryOptions - Updated repository implementations (Album, Artist, MediaFile) to handle library filtering in search operations - Added comprehensive test coverage with robust assertions verifying library filtering works correctly - Enhanced mock repositories to capture QueryOptions for test validation Signed-off-by: Deluan --- model/searchable.go | 2 +- persistence/album_repository.go | 6 +- persistence/artist_repository.go | 6 +- persistence/mediafile_repository.go | 6 +- server/subsonic/search_test.go | 208 ++++++++++++++++++++++++++++ server/subsonic/searching.go | 39 ++++-- tests/mock_album_repo.go | 11 ++ tests/mock_artist_repo.go | 12 ++ tests/mock_mediafile_repo.go | 12 ++ 9 files changed, 283 insertions(+), 19 deletions(-) create mode 100644 server/subsonic/search_test.go diff --git a/model/searchable.go b/model/searchable.go index d37299997..cc4f0b44e 100644 --- a/model/searchable.go +++ b/model/searchable.go @@ -1,5 +1,5 @@ package model type SearchableRepository[T any] interface { - Search(q string, offset, size int, includeMissing bool) (T, error) + Search(q string, offset, size int, includeMissing bool, options ...QueryOptions) (T, error) } diff --git a/persistence/album_repository.go b/persistence/album_repository.go index 88d657f35..682a409a1 100644 --- a/persistence/album_repository.go +++ b/persistence/album_repository.go @@ -349,15 +349,15 @@ func (r *albumRepository) purgeEmpty() error { return nil } -func (r *albumRepository) Search(q string, offset int, size int, includeMissing bool) (model.Albums, error) { +func (r *albumRepository) Search(q string, offset int, size int, includeMissing bool, options ...model.QueryOptions) (model.Albums, error) { var res dbAlbums if uuid.Validate(q) == nil { - err := r.searchByMBID(r.selectAlbum(), q, []string{"mbz_album_id", "mbz_release_group_id"}, includeMissing, &res) + err := r.searchByMBID(r.selectAlbum(options...), q, []string{"mbz_album_id", "mbz_release_group_id"}, includeMissing, &res) if err != nil { return nil, fmt.Errorf("searching album by MBID %q: %w", q, err) } } else { - err := r.doSearch(r.selectAlbum(), q, offset, size, includeMissing, &res, "name") + err := r.doSearch(r.selectAlbum(options...), q, offset, size, includeMissing, &res, "name") if err != nil { return nil, fmt.Errorf("searching album by query %q: %w", q, err) } diff --git a/persistence/artist_repository.go b/persistence/artist_repository.go index 13b3dce16..f10e5d777 100644 --- a/persistence/artist_repository.go +++ b/persistence/artist_repository.go @@ -516,15 +516,15 @@ func (r *artistRepository) RefreshStats(allArtists bool) (int64, error) { return totalRowsAffected, nil } -func (r *artistRepository) Search(q string, offset int, size int, includeMissing bool) (model.Artists, error) { +func (r *artistRepository) Search(q string, offset int, size int, includeMissing bool, options ...model.QueryOptions) (model.Artists, error) { var res dbArtists if uuid.Validate(q) == nil { - err := r.searchByMBID(r.selectArtist(), q, []string{"mbz_artist_id"}, includeMissing, &res) + err := r.searchByMBID(r.selectArtist(options...), q, []string{"mbz_artist_id"}, includeMissing, &res) if err != nil { return nil, fmt.Errorf("searching artist by MBID %q: %w", q, err) } } else { - err := r.doSearch(r.selectArtist(), q, offset, size, includeMissing, &res, "json_extract(stats, '$.total.m') desc", "name") + err := r.doSearch(r.selectArtist(options...), q, offset, size, includeMissing, &res, "json_extract(stats, '$.total.m') desc", "name") if err != nil { return nil, fmt.Errorf("searching artist by query %q: %w", q, err) } diff --git a/persistence/mediafile_repository.go b/persistence/mediafile_repository.go index fcb188d03..29a4cf9c0 100644 --- a/persistence/mediafile_repository.go +++ b/persistence/mediafile_repository.go @@ -297,15 +297,15 @@ func (r *mediaFileRepository) GetMissingAndMatching(libId int) (model.MediaFileC }, nil } -func (r *mediaFileRepository) Search(q string, offset int, size int, includeMissing bool) (model.MediaFiles, error) { +func (r *mediaFileRepository) Search(q string, offset int, size int, includeMissing bool, options ...model.QueryOptions) (model.MediaFiles, error) { var res dbMediaFiles if uuid.Validate(q) == nil { - err := r.searchByMBID(r.selectMediaFile(), q, []string{"mbz_recording_id", "mbz_release_track_id"}, includeMissing, &res) + err := r.searchByMBID(r.selectMediaFile(options...), q, []string{"mbz_recording_id", "mbz_release_track_id"}, includeMissing, &res) if err != nil { return nil, fmt.Errorf("searching media_file by MBID %q: %w", q, err) } } else { - err := r.doSearch(r.selectMediaFile(), q, offset, size, includeMissing, &res, "title") + err := r.doSearch(r.selectMediaFile(options...), q, offset, size, includeMissing, &res, "title") if err != nil { return nil, fmt.Errorf("searching media_file by query %q: %w", q, err) } diff --git a/server/subsonic/search_test.go b/server/subsonic/search_test.go new file mode 100644 index 000000000..dfe3a45c4 --- /dev/null +++ b/server/subsonic/search_test.go @@ -0,0 +1,208 @@ +package subsonic + +import ( + "github.com/Masterminds/squirrel" + "github.com/navidrome/navidrome/core/auth" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/request" + "github.com/navidrome/navidrome/tests" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("Search", func() { + var router *Router + var ds model.DataStore + var mockAlbumRepo *tests.MockAlbumRepo + var mockArtistRepo *tests.MockArtistRepo + var mockMediaFileRepo *tests.MockMediaFileRepo + + BeforeEach(func() { + ds = &tests.MockDataStore{} + auth.Init(ds) + + router = New(ds, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil) + + // Get references to the mock repositories so we can inspect their Options + mockAlbumRepo = ds.Album(nil).(*tests.MockAlbumRepo) + mockArtistRepo = ds.Artist(nil).(*tests.MockArtistRepo) + mockMediaFileRepo = ds.MediaFile(nil).(*tests.MockMediaFileRepo) + }) + + Context("musicFolderId parameter", func() { + assertQueryOptions := func(filter squirrel.Sqlizer, expectedQuery string, expectedArgs ...interface{}) { + GinkgoHelper() + query, args, err := filter.ToSql() + Expect(err).ToNot(HaveOccurred()) + Expect(query).To(ContainSubstring(expectedQuery)) + Expect(args).To(ContainElements(expectedArgs...)) + } + + Describe("Search2", func() { + It("should accept musicFolderId parameter", func() { + r := newGetRequest("query=test", "musicFolderId=1") + ctx := request.WithUser(r.Context(), model.User{ + ID: "user1", + UserName: "testuser", + Libraries: []model.Library{{ID: 1, Name: "Library 1"}}, + }) + r = r.WithContext(ctx) + + resp, err := router.Search2(r) + + Expect(err).ToNot(HaveOccurred()) + Expect(resp).ToNot(BeNil()) + Expect(resp.SearchResult2).ToNot(BeNil()) + + // Verify that library filter was applied to all repositories + assertQueryOptions(mockAlbumRepo.Options.Filters, "library_id IN (?)", 1) + assertQueryOptions(mockArtistRepo.Options.Filters, "library_id IN (?)", 1) + assertQueryOptions(mockMediaFileRepo.Options.Filters, "library_id IN (?)", 1) + }) + + It("should return results from all accessible libraries when musicFolderId is not provided", func() { + r := newGetRequest("query=test") + ctx := request.WithUser(r.Context(), model.User{ + ID: "user1", + UserName: "testuser", + Libraries: []model.Library{ + {ID: 1, Name: "Library 1"}, + {ID: 2, Name: "Library 2"}, + {ID: 3, Name: "Library 3"}, + }, + }) + r = r.WithContext(ctx) + + resp, err := router.Search2(r) + + Expect(err).ToNot(HaveOccurred()) + Expect(resp).ToNot(BeNil()) + Expect(resp.SearchResult2).ToNot(BeNil()) + + // Verify that library filter was applied to all repositories with all accessible libraries + assertQueryOptions(mockAlbumRepo.Options.Filters, "library_id IN (?,?,?)", 1, 2, 3) + assertQueryOptions(mockArtistRepo.Options.Filters, "library_id IN (?,?,?)", 1, 2, 3) + assertQueryOptions(mockMediaFileRepo.Options.Filters, "library_id IN (?,?,?)", 1, 2, 3) + }) + + It("should return empty results when user has no accessible libraries", func() { + r := newGetRequest("query=test") + ctx := request.WithUser(r.Context(), model.User{ + ID: "user1", + UserName: "testuser", + Libraries: []model.Library{}, // No libraries + }) + r = r.WithContext(ctx) + + resp, err := router.Search2(r) + + Expect(err).ToNot(HaveOccurred()) + Expect(resp).ToNot(BeNil()) + Expect(resp.SearchResult2).ToNot(BeNil()) + Expect(mockAlbumRepo.Options.Filters).To(BeNil()) + Expect(mockArtistRepo.Options.Filters).To(BeNil()) + Expect(mockMediaFileRepo.Options.Filters).To(BeNil()) + }) + + It("should return error for inaccessible musicFolderId", func() { + r := newGetRequest("query=test", "musicFolderId=999") + ctx := request.WithUser(r.Context(), model.User{ + ID: "user1", + UserName: "testuser", + Libraries: []model.Library{{ID: 1, Name: "Library 1"}}, + }) + r = r.WithContext(ctx) + + resp, err := router.Search2(r) + + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("Library 999 not found or not accessible")) + Expect(resp).To(BeNil()) + }) + }) + + Describe("Search3", func() { + It("should accept musicFolderId parameter", func() { + r := newGetRequest("query=test", "musicFolderId=1") + ctx := request.WithUser(r.Context(), model.User{ + ID: "user1", + UserName: "testuser", + Libraries: []model.Library{{ID: 1, Name: "Library 1"}}, + }) + r = r.WithContext(ctx) + + resp, err := router.Search3(r) + + Expect(err).ToNot(HaveOccurred()) + Expect(resp).ToNot(BeNil()) + Expect(resp.SearchResult3).ToNot(BeNil()) + + // Verify that library filter was applied to all repositories + assertQueryOptions(mockAlbumRepo.Options.Filters, "library_id IN (?)", 1) + assertQueryOptions(mockArtistRepo.Options.Filters, "library_id IN (?)", 1) + assertQueryOptions(mockMediaFileRepo.Options.Filters, "library_id IN (?)", 1) + }) + + It("should return results from all accessible libraries when musicFolderId is not provided", func() { + r := newGetRequest("query=test") + ctx := request.WithUser(r.Context(), model.User{ + ID: "user1", + UserName: "testuser", + Libraries: []model.Library{ + {ID: 1, Name: "Library 1"}, + {ID: 2, Name: "Library 2"}, + {ID: 3, Name: "Library 3"}, + }, + }) + r = r.WithContext(ctx) + + resp, err := router.Search3(r) + + Expect(err).ToNot(HaveOccurred()) + Expect(resp).ToNot(BeNil()) + Expect(resp.SearchResult3).ToNot(BeNil()) + + // Verify that library filter was applied to all repositories with all accessible libraries + assertQueryOptions(mockAlbumRepo.Options.Filters, "library_id IN (?,?,?)", 1, 2, 3) + assertQueryOptions(mockArtistRepo.Options.Filters, "library_id IN (?,?,?)", 1, 2, 3) + assertQueryOptions(mockMediaFileRepo.Options.Filters, "library_id IN (?,?,?)", 1, 2, 3) + }) + + It("should return empty results when user has no accessible libraries", func() { + r := newGetRequest("query=test") + ctx := request.WithUser(r.Context(), model.User{ + ID: "user1", + UserName: "testuser", + Libraries: []model.Library{}, // No libraries + }) + r = r.WithContext(ctx) + + resp, err := router.Search3(r) + + Expect(err).ToNot(HaveOccurred()) + Expect(resp).ToNot(BeNil()) + Expect(resp.SearchResult3).ToNot(BeNil()) + Expect(mockAlbumRepo.Options.Filters).To(BeNil()) + Expect(mockArtistRepo.Options.Filters).To(BeNil()) + Expect(mockMediaFileRepo.Options.Filters).To(BeNil()) + }) + + It("should return error for inaccessible musicFolderId", func() { + // Test that the endpoint returns an error when user tries to access a library they don't have access to + r := newGetRequest("query=test", "musicFolderId=999") + ctx := request.WithUser(r.Context(), model.User{ + ID: "user1", + UserName: "testuser", + Libraries: []model.Library{{ID: 1, Name: "Library 1"}}, + }) + r = r.WithContext(ctx) + + resp, err := router.Search3(r) + + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("Library 999 not found or not accessible")) + Expect(resp).To(BeNil()) + }) + }) + }) +}) diff --git a/server/subsonic/searching.go b/server/subsonic/searching.go index f66846f35..d8f85afeb 100644 --- a/server/subsonic/searching.go +++ b/server/subsonic/searching.go @@ -8,6 +8,7 @@ import ( "strings" "time" + . "github.com/Masterminds/squirrel" "github.com/deluan/sanitize" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" @@ -41,9 +42,9 @@ func (api *Router) getSearchParams(r *http.Request) (*searchParams, error) { return sp, nil } -type searchFunc[T any] func(q string, offset int, size int, includeMissing bool) (T, error) +type searchFunc[T any] func(q string, offset int, size int, includeMissing bool, options ...model.QueryOptions) (T, error) -func callSearch[T any](ctx context.Context, s searchFunc[T], q string, offset, size int, result *T) func() error { +func callSearch[T any](ctx context.Context, s searchFunc[T], q string, offset, size int, result *T, options ...model.QueryOptions) func() error { return func() error { if size == 0 { return nil @@ -51,7 +52,7 @@ func callSearch[T any](ctx context.Context, s searchFunc[T], q string, offset, s typ := strings.TrimPrefix(reflect.TypeOf(*result).String(), "model.") var err error start := time.Now() - *result, err = s(q, offset, size, false) + *result, err = s(q, offset, size, false, options...) if err != nil { log.Error(ctx, "Error searching "+typ, "query", q, "elapsed", time.Since(start), err) } else { @@ -61,15 +62,23 @@ func callSearch[T any](ctx context.Context, s searchFunc[T], q string, offset, s } } -func (api *Router) searchAll(ctx context.Context, sp *searchParams) (mediaFiles model.MediaFiles, albums model.Albums, artists model.Artists) { +func (api *Router) searchAll(ctx context.Context, sp *searchParams, musicFolderIds []int) (mediaFiles model.MediaFiles, albums model.Albums, artists model.Artists) { start := time.Now() q := sanitize.Accents(strings.ToLower(strings.TrimSuffix(sp.query, "*"))) + // Create query options for library filtering + var options []model.QueryOptions + if len(musicFolderIds) > 0 { + options = append(options, model.QueryOptions{ + Filters: Eq{"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)) - g.Go(callSearch(ctx, api.ds.Album(ctx).Search, q, sp.albumOffset, sp.albumCount, &albums)) - g.Go(callSearch(ctx, api.ds.Artist(ctx).Search, q, sp.artistOffset, sp.artistCount, &artists)) + 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, options...)) err := g.Wait() if err == nil { log.Debug(ctx, fmt.Sprintf("Search resulted in %d songs, %d albums and %d artists", @@ -86,7 +95,13 @@ func (api *Router) Search2(r *http.Request) (*responses.Subsonic, error) { if err != nil { return nil, err } - mfs, als, as := api.searchAll(ctx, sp) + + // Get optional library IDs from musicFolderId parameter + musicFolderIds, err := selectedMusicFolderIds(r, false) + if err != nil { + return nil, err + } + mfs, als, as := api.searchAll(ctx, sp, musicFolderIds) response := newResponse() searchResult2 := &responses.SearchResult2{} @@ -115,7 +130,13 @@ func (api *Router) Search3(r *http.Request) (*responses.Subsonic, error) { if err != nil { return nil, err } - mfs, als, as := api.searchAll(ctx, sp) + + // Get optional library IDs from musicFolderId parameter + musicFolderIds, err := selectedMusicFolderIds(r, false) + if err != nil { + return nil, err + } + mfs, als, as := api.searchAll(ctx, sp, musicFolderIds) response := newResponse() searchResult3 := &responses.SearchResult3{} diff --git a/tests/mock_album_repo.go b/tests/mock_album_repo.go index 58c33c97f..6f3cbb0d8 100644 --- a/tests/mock_album_repo.go +++ b/tests/mock_album_repo.go @@ -117,4 +117,15 @@ func (m *MockAlbumRepo) UpdateExternalInfo(album *model.Album) error { return nil } +func (m *MockAlbumRepo) Search(q string, offset int, size int, includeMissing bool, options ...model.QueryOptions) (model.Albums, error) { + if len(options) > 0 { + m.Options = options[0] + } + if m.Err { + return nil, errors.New("unexpected error") + } + // Simple mock implementation - just return all albums for testing + return m.All, nil +} + var _ model.AlbumRepository = (*MockAlbumRepo)(nil) diff --git a/tests/mock_artist_repo.go b/tests/mock_artist_repo.go index 0fcf333f5..1298cbd2a 100644 --- a/tests/mock_artist_repo.go +++ b/tests/mock_artist_repo.go @@ -145,4 +145,16 @@ func (m *MockArtistRepo) GetIndex(includeMissing bool, libraryIds []int, roles . return result, nil } +func (m *MockArtistRepo) Search(q string, offset int, size int, includeMissing bool, options ...model.QueryOptions) (model.Artists, error) { + if len(options) > 0 { + m.Options = options[0] + } + if m.Err { + return nil, errors.New("unexpected error") + } + // Simple mock implementation - just return all artists for testing + allArtists, err := m.GetAll() + return allArtists, err +} + var _ model.ArtistRepository = (*MockArtistRepo)(nil) diff --git a/tests/mock_mediafile_repo.go b/tests/mock_mediafile_repo.go index de6419f01..2a449347f 100644 --- a/tests/mock_mediafile_repo.go +++ b/tests/mock_mediafile_repo.go @@ -231,5 +231,17 @@ func (m *MockMediaFileRepo) NewInstance() interface{} { return &model.MediaFile{} } +func (m *MockMediaFileRepo) Search(q string, offset int, size int, includeMissing bool, options ...model.QueryOptions) (model.MediaFiles, error) { + if len(options) > 0 { + m.Options = options[0] + } + if m.Err { + return nil, errors.New("unexpected error") + } + // Simple mock implementation - just return all media files for testing + allFiles, err := m.GetAll() + return allFiles, err +} + var _ model.MediaFileRepository = (*MockMediaFileRepo)(nil) var _ model.ResourceRepository = (*MockMediaFileRepo)(nil)