mirror of
https://github.com/navidrome/navidrome.git
synced 2026-10-26 10:17:09 +01:00
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 <deluan@navidrome.org>
This commit is contained in:
parent
19e1830e9d
commit
1aa48de4eb
9 changed files with 283 additions and 19 deletions
|
|
@ -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)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
}
|
||||
|
|
|
|||
208
server/subsonic/search_test.go
Normal file
208
server/subsonic/search_test.go
Normal file
|
|
@ -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())
|
||||
})
|
||||
})
|
||||
})
|
||||
})
|
||||
|
|
@ -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{}
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue