From 45e13950f32af4f139200d83a2772afe7e983369 Mon Sep 17 00:00:00 2001
From: DualFroz <me@dualfroz.com>
Date: Wed, 14 Oct 2026 19:07:59 +0200
Subject: [PATCH] fix(smartplaylist): don't let invalid rules hide all
 playlists - #6316

Empty nested groups were saved as {} and could not be read back, failing
every playlist query. Drop them while parsing, and log and skip unreadable
rules stored in the DB instead of failing the whole listing.

Signed-off-by: DualFroz <me@dualfroz.com>
---
 model/criteria/criteria_test.go               | 25 +++++++++++++++++
 model/criteria/json.go                        | 15 +++++++++++
 persistence/playlist_repository.go            |  9 +++++--
 persistence/smart_playlist_repository_test.go | 27 +++++++++++++++++++
 4 files changed, 74 insertions(+), 2 deletions(-)

diff --git a/model/criteria/criteria_test.go b/model/criteria/criteria_test.go
index de51245..15e0cc2 100644
--- a/model/criteria/criteria_test.go
+++ b/model/criteria/criteria_test.go
@@ -103,6 +103,31 @@ var _ = Describe("Criteria", func() {
 		)
 	})
 
+	Context("with empty groups", func() {
+		DescribeTable("rejects criteria with no rules other than empty groups",
+			func(jsonStr string) {
+				var c Criteria
+				err := json.Unmarshal([]byte(jsonStr), &c)
+				gomega.Expect(err).To(gomega.MatchError(gomega.ContainSubstring("missing rules")))
+			},
+			Entry("empty object", `{"all":[{}],"sort":"+lastplayed","limit":10}`),
+			Entry("empty nested all", `{"all":[{"all":[]}],"sort":"+lastplayed","limit":10}`),
+			Entry("empty nested any", `{"any":[{"any":[]}]}`),
+			Entry("nested group holding an empty object", `{"all":[{"any":[{}]}]}`),
+		)
+
+		It("drops empty nested groups, so the rules can be read back after saving", func() {
+			var c Criteria
+			err := json.Unmarshal([]byte(`{"all":[{"is":{"loved":true}},{"any":[]}]}`), &c)
+			gomega.Expect(err).ToNot(gomega.HaveOccurred())
+			gomega.Expect(c.Expression).To(gomega.Equal(All{Is{"loved": true}}))
+
+			j, err := json.Marshal(c)
+			gomega.Expect(err).ToNot(gomega.HaveOccurred())
+			gomega.Expect(string(j)).To(gomega.Equal(`{"all":[{"is":{"loved":true}}]}`))
+		})
+	})
+
 	Describe("LimitPercent", func() {
 		Describe("JSON round-trip", func() {
 			It("marshals and unmarshals limitPercent", func() {
diff --git a/model/criteria/json.go b/model/criteria/json.go
index d0f4535..5645b8b 100644
--- a/model/criteria/json.go
+++ b/model/criteria/json.go
@@ -26,6 +26,9 @@ func (uc *unmarshalConjunctionType) UnmarshalJSON(data []byte) error {
 			if expr == nil {
 				return fmt.Errorf(`invalid expression key '%s'`, k)
 			}
+			if isEmptyConjunction(expr) {
+				continue
+			}
 			es = append(es, expr)
 		}
 	}
@@ -154,6 +157,18 @@ func unmarshalConjunction(conjName string, rawValue json.RawMessage) Expression
 	return nil
 }
 
+// isEmptyConjunction reports whether expr is a group with no rules. Such groups are dropped
+// while parsing: they would be saved as {}, which cannot be read back.
+func isEmptyConjunction(expr Expression) bool {
+	switch e := expr.(type) {
+	case All:
+		return len(e) == 0
+	case Any:
+		return len(e) == 0
+	}
+	return false
+}
+
 func marshalExpression(name string, value map[string]any) ([]byte, error) {
 	if len(value) != 1 {
 		return nil, fmt.Errorf(`invalid %s expression length %d for values %v`, name, len(value), value)
diff --git a/persistence/playlist_repository.go b/persistence/playlist_repository.go
index 1a1b05a..e9b0958 100644
--- a/persistence/playlist_repository.go
+++ b/persistence/playlist_repository.go
@@ -27,8 +27,13 @@ type dbPlaylist struct {
 }
 
 func (p *dbPlaylist) PostScan() error {
-	if p.Rules.String != "" {
-		return json.Unmarshal([]byte(p.Rules.String), &p.Playlist.Rules)
+	if p.Rules.String == "" {
+		return nil
+	}
+	// Rules come from clients and .nsp files, so a bad one must not fail every query listing it
+	if err := json.Unmarshal([]byte(p.Rules.String), &p.Playlist.Rules); err != nil {
+		log.Warn("Ignoring invalid smart playlist rules", "playlist", p.Name, "id", p.ID, "rules", p.Rules.String, err)
+		p.Playlist.Rules = nil
 	}
 	return nil
 }
diff --git a/persistence/smart_playlist_repository_test.go b/persistence/smart_playlist_repository_test.go
index 4d4c5ed..a486cf0 100644
--- a/persistence/smart_playlist_repository_test.go
+++ b/persistence/smart_playlist_repository_test.go
@@ -75,6 +75,33 @@ var _ = Describe("PlaylistRepository - Smart Playlists", func() {
 				newPls := model.Playlist{Name: "Great!", OwnerID: "userid", Rules: rules}
 				Expect(repo.Put(ctx, &newPls)).To(MatchError(ContainSubstring("invalid criteria expression")))
 			})
+
+			Context("already stored in the DB", func() {
+				var broken model.Playlist
+
+				BeforeEach(func() {
+					broken = model.Playlist{Name: "Broken", OwnerID: "userid", Rules: rules}
+					Expect(repo.Put(ctx, &broken)).To(Succeed())
+					DeferCleanup(func() { Expect(repo.Delete(ctx, broken.ID)).To(Succeed()) })
+
+					_, err := GetDBXBuilder().NewQuery("update playlist set rules = {:rules} where id = {:id}").
+						Bind(dbx.Params{"rules": `{"all":[{}],"sort":"+lastplayed","limit":10}`, "id": broken.ID}).Execute()
+					Expect(err).ToNot(HaveOccurred())
+				})
+
+				It("does not hide the other playlists", func() {
+					all, err := repo.GetAll(ctx)
+					Expect(err).ToNot(HaveOccurred())
+					Expect(slice.Map(all, func(p model.Playlist) string { return p.ID })).To(
+						ContainElements(plsBest.ID, plsCool.ID, broken.ID))
+				})
+
+				It("returns the playlist without its rules", func() {
+					pls, err := repo.Get(ctx, broken.ID)
+					Expect(err).ToNot(HaveOccurred())
+					Expect(pls.Rules).To(BeNil())
+				})
+			})
 		})
 
 		Context("re-imported from disk", func() {