From 64159d715a66dc214764ebda1b93544f5e4d056d Mon Sep 17 00:00:00 2001 From: Deluan Date: Fri, 27 Jun 2025 09:32:55 -0400 Subject: [PATCH] feat: enhance selectedMusicFolderIds function to handle valid music folder IDs and improve fallback logic Signed-off-by: Deluan --- server/subsonic/helpers.go | 14 +++-- server/subsonic/helpers_test.go | 99 +++++++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+), 6 deletions(-) diff --git a/server/subsonic/helpers.go b/server/subsonic/helpers.go index e8436965c..2a139063d 100644 --- a/server/subsonic/helpers.go +++ b/server/subsonic/helpers.go @@ -482,18 +482,20 @@ func getUserAccessibleLibraries(ctx context.Context) []model.Library { return user.Libraries } -// selectedMusicFolderIds returns the music folder IDs from the request parameters. -// If no IDs are provided, it returns all libraries accessible to the user. -func selectedMusicFolderIds(r *http.Request) ([]int, error) { - user := getUser(r.Context()) +func selectedMusicFolderIds(r *http.Request, required bool) ([]int, error) { p := req.Params(r) musicFolderIds, err := p.Ints("musicFolderId") - if !errors.Is(err, req.ErrMissingParam) { + + // If the parameter is not present, it returns an error if it is required. + if errors.Is(err, req.ErrMissingParam) && required { return nil, err } if len(musicFolderIds) > 0 { return musicFolderIds, nil } - return slice.Map(user.Libraries, func(lib model.Library) int { return lib.ID }), nil + + // If no musicFolderId is provided, return all libraries the user has access to. + libraries := getUserAccessibleLibraries(r.Context()) + return slice.Map(libraries, func(lib model.Library) int { return lib.ID }), nil } diff --git a/server/subsonic/helpers_test.go b/server/subsonic/helpers_test.go index a4978237b..9ebfd3750 100644 --- a/server/subsonic/helpers_test.go +++ b/server/subsonic/helpers_test.go @@ -1,10 +1,15 @@ package subsonic import ( + "context" + "net/http/httptest" + "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/conf/configtest" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/server/subsonic/responses" + "github.com/navidrome/navidrome/utils/req" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) @@ -163,4 +168,98 @@ var _ = Describe("helpers", func() { Expect(result).To(Equal(int32(4))) }) }) + + Describe("selectedMusicFolderIds", func() { + var user model.User + var ctx context.Context + + BeforeEach(func() { + user = model.User{ + ID: "test-user", + Libraries: []model.Library{ + {ID: 1, Name: "Library 1"}, + {ID: 2, Name: "Library 2"}, + {ID: 3, Name: "Library 3"}, + }, + } + ctx = request.WithUser(context.Background(), user) + }) + + Context("when musicFolderId parameter is provided", func() { + It("should return the specified musicFolderId values", func() { + r := httptest.NewRequest("GET", "/test?musicFolderId=1&musicFolderId=3", nil) + r = r.WithContext(ctx) + + ids, err := selectedMusicFolderIds(r, false) + Expect(err).ToNot(HaveOccurred()) + Expect(ids).To(Equal([]int{1, 3})) + }) + + It("should ignore invalid musicFolderId parameter values", func() { + r := httptest.NewRequest("GET", "/test?musicFolderId=invalid&musicFolderId=2", nil) + r = r.WithContext(ctx) + + ids, err := selectedMusicFolderIds(r, false) + Expect(err).ToNot(HaveOccurred()) + Expect(ids).To(Equal([]int{2})) // Only valid ID is returned + }) + }) + + Context("when musicFolderId parameter is not provided", func() { + Context("and required is false", func() { + It("should return all user's library IDs", func() { + r := httptest.NewRequest("GET", "/test", nil) + r = r.WithContext(ctx) + + ids, err := selectedMusicFolderIds(r, false) + Expect(err).ToNot(HaveOccurred()) + Expect(ids).To(Equal([]int{1, 2, 3})) + }) + + It("should return empty slice when user has no libraries", func() { + userWithoutLibs := model.User{ID: "no-libs-user", Libraries: []model.Library{}} + ctxWithoutLibs := request.WithUser(context.Background(), userWithoutLibs) + r := httptest.NewRequest("GET", "/test", nil) + r = r.WithContext(ctxWithoutLibs) + + ids, err := selectedMusicFolderIds(r, false) + Expect(err).ToNot(HaveOccurred()) + Expect(ids).To(Equal([]int{})) + }) + }) + + Context("and required is true", func() { + It("should return ErrMissingParam error", func() { + r := httptest.NewRequest("GET", "/test", nil) + r = r.WithContext(ctx) + + ids, err := selectedMusicFolderIds(r, true) + Expect(err).To(MatchError(req.ErrMissingParam)) + Expect(ids).To(BeNil()) + }) + }) + }) + + Context("when musicFolderId parameter is empty", func() { + It("should return all user's library IDs even when empty parameter is provided", func() { + r := httptest.NewRequest("GET", "/test?musicFolderId=", nil) + r = r.WithContext(ctx) + + ids, err := selectedMusicFolderIds(r, false) + Expect(err).ToNot(HaveOccurred()) + Expect(ids).To(Equal([]int{1, 2, 3})) + }) + }) + + Context("when all musicFolderId parameters are invalid", func() { + It("should return all user libraries when all musicFolderId parameters are invalid", func() { + r := httptest.NewRequest("GET", "/test?musicFolderId=invalid&musicFolderId=notanumber", nil) + r = r.WithContext(ctx) + + ids, err := selectedMusicFolderIds(r, false) + Expect(err).ToNot(HaveOccurred()) + Expect(ids).To(Equal([]int{1, 2, 3})) // Falls back to all user libraries + }) + }) + }) })