From aab3223e00ec3a219fbddbadd4f1ba16d5700db7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Tue, 24 Jun 2025 08:50:06 -0400 Subject: [PATCH] fix(subsonic): clearing playlist `comment` and `public` in Subsonic API (#4258) * fix(subsonic): allow clearing playlist comment * fix(playlists): simplify comment and public parameter handling Signed-off-by: Deluan * refactor(playlists): streamline fakePlaylists implementation in tests Signed-off-by: Deluan --------- Signed-off-by: Deluan --- server/subsonic/playlists.go | 10 +--- server/subsonic/playlists_test.go | 88 +++++++++++++++++++++++++++++++ utils/req/req.go | 19 +++++++ utils/req/req_test.go | 55 +++++++++++++++++++ 4 files changed, 164 insertions(+), 8 deletions(-) create mode 100644 server/subsonic/playlists_test.go diff --git a/server/subsonic/playlists.go b/server/subsonic/playlists.go index 555c9eb48..83b0408ff 100644 --- a/server/subsonic/playlists.go +++ b/server/subsonic/playlists.go @@ -131,14 +131,8 @@ func (api *Router) UpdatePlaylist(r *http.Request) (*responses.Subsonic, error) if s, err := p.String("name"); err == nil { plsName = &s } - var comment *string - if s, err := p.String("comment"); err == nil { - comment = &s - } - var public *bool - if p, err := p.Bool("public"); err == nil { - public = &p - } + comment := p.StringPtr("comment") + public := p.BoolPtr("public") log.Debug(r, "Updating playlist", "id", playlistId) if plsName != nil { diff --git a/server/subsonic/playlists_test.go b/server/subsonic/playlists_test.go new file mode 100644 index 000000000..cf9865231 --- /dev/null +++ b/server/subsonic/playlists_test.go @@ -0,0 +1,88 @@ +package subsonic + +import ( + "context" + + "github.com/navidrome/navidrome/core" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/tests" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ core.Playlists = (*fakePlaylists)(nil) + +var _ = Describe("UpdatePlaylist", func() { + var router *Router + var ds model.DataStore + var playlists *fakePlaylists + + BeforeEach(func() { + ds = &tests.MockDataStore{} + playlists = &fakePlaylists{} + router = New(ds, nil, nil, nil, nil, nil, nil, nil, playlists, nil, nil, nil) + }) + + It("clears the comment when parameter is empty", func() { + r := newGetRequest("playlistId=123", "comment=") + _, err := router.UpdatePlaylist(r) + Expect(err).ToNot(HaveOccurred()) + Expect(playlists.lastPlaylistID).To(Equal("123")) + Expect(playlists.lastComment).ToNot(BeNil()) + Expect(*playlists.lastComment).To(Equal("")) + }) + + It("leaves comment unchanged when parameter is missing", func() { + r := newGetRequest("playlistId=123") + _, err := router.UpdatePlaylist(r) + Expect(err).ToNot(HaveOccurred()) + Expect(playlists.lastPlaylistID).To(Equal("123")) + Expect(playlists.lastComment).To(BeNil()) + }) + + It("sets public to true when parameter is 'true'", func() { + r := newGetRequest("playlistId=123", "public=true") + _, err := router.UpdatePlaylist(r) + Expect(err).ToNot(HaveOccurred()) + Expect(playlists.lastPlaylistID).To(Equal("123")) + Expect(playlists.lastPublic).ToNot(BeNil()) + Expect(*playlists.lastPublic).To(BeTrue()) + }) + + It("sets public to false when parameter is 'false'", func() { + r := newGetRequest("playlistId=123", "public=false") + _, err := router.UpdatePlaylist(r) + Expect(err).ToNot(HaveOccurred()) + Expect(playlists.lastPlaylistID).To(Equal("123")) + Expect(playlists.lastPublic).ToNot(BeNil()) + Expect(*playlists.lastPublic).To(BeFalse()) + }) + + It("leaves public unchanged when parameter is missing", func() { + r := newGetRequest("playlistId=123") + _, err := router.UpdatePlaylist(r) + Expect(err).ToNot(HaveOccurred()) + Expect(playlists.lastPlaylistID).To(Equal("123")) + Expect(playlists.lastPublic).To(BeNil()) + }) +}) + +type fakePlaylists struct { + core.Playlists + lastPlaylistID string + lastName *string + lastComment *string + lastPublic *bool + lastAdd []string + lastRemove []int +} + +func (f *fakePlaylists) Update(ctx context.Context, playlistID string, name *string, comment *string, public *bool, idsToAdd []string, idxToRemove []int) error { + f.lastPlaylistID = playlistID + f.lastName = name + f.lastComment = comment + f.lastPublic = public + f.lastAdd = idsToAdd + f.lastRemove = idxToRemove + return nil +} diff --git a/utils/req/req.go b/utils/req/req.go index cf498f322..f9fa5724b 100644 --- a/utils/req/req.go +++ b/utils/req/req.go @@ -35,6 +35,25 @@ func (r *Values) String(param string) (string, error) { return v, nil } +func (r *Values) StringPtr(param string) *string { + var v *string + if _, exists := r.URL.Query()[param]; exists { + s := r.URL.Query().Get(param) + v = &s + } + return v +} + +func (r *Values) BoolPtr(param string) *bool { + var v *bool + if _, exists := r.URL.Query()[param]; exists { + s := r.URL.Query().Get(param) + b := strings.Contains("/true/on/1/", "/"+strings.ToLower(s)+"/") + v = &b + } + return v +} + func (r *Values) StringOr(param, def string) string { v, _ := r.String(param) if v == "" { diff --git a/utils/req/req_test.go b/utils/req/req_test.go index 041aca220..e710365bd 100644 --- a/utils/req/req_test.go +++ b/utils/req/req_test.go @@ -219,4 +219,59 @@ var _ = Describe("Request Helpers", func() { }) }) }) + + Describe("ParamStringPtr", func() { + BeforeEach(func() { + r = req.Params(httptest.NewRequest("GET", "/ping?a=123", nil)) + }) + + It("returns pointer to string if param exists", func() { + ptr := r.StringPtr("a") + Expect(ptr).ToNot(BeNil()) + Expect(*ptr).To(Equal("123")) + }) + + It("returns nil if param does not exist", func() { + ptr := r.StringPtr("xx") + Expect(ptr).To(BeNil()) + }) + + It("returns pointer to empty string if param exists but is empty", func() { + r = req.Params(httptest.NewRequest("GET", "/ping?a=", nil)) + ptr := r.StringPtr("a") + Expect(ptr).ToNot(BeNil()) + Expect(*ptr).To(Equal("")) + }) + }) + + Describe("ParamBoolPtr", func() { + Context("value is true", func() { + BeforeEach(func() { + r = req.Params(httptest.NewRequest("GET", "/ping?b=true", nil)) + }) + + It("returns pointer to true if param is 'true'", func() { + ptr := r.BoolPtr("b") + Expect(ptr).ToNot(BeNil()) + Expect(*ptr).To(BeTrue()) + }) + }) + + Context("value is false", func() { + BeforeEach(func() { + r = req.Params(httptest.NewRequest("GET", "/ping?b=false", nil)) + }) + + It("returns pointer to false if param is 'false'", func() { + ptr := r.BoolPtr("b") + Expect(ptr).ToNot(BeNil()) + Expect(*ptr).To(BeFalse()) + }) + }) + + It("returns nil if param does not exist", func() { + ptr := r.BoolPtr("xx") + Expect(ptr).To(BeNil()) + }) + }) })