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() {