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 <deluan@navidrome.org>

* refactor(playlists): streamline fakePlaylists implementation in tests

Signed-off-by: Deluan <deluan@navidrome.org>

---------

Signed-off-by: Deluan <deluan@navidrome.org>

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
+	comment := p.StringPtr("comment")
-	if s, err := p.String("comment"); err == nil {
+	public := p.BoolPtr("public")
-		comment = &s
-	}
-	var public *bool
-	if p, err := p.Bool("public"); err == nil {
-		public = &p
-	}
 
 	log.Debug(r, "Updating playlist", "id", playlistId)
 	if plsName != nil {

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
+}

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 == "" {

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())
+		})
+	})
 })