mirror of
https://github.com/navidrome/navidrome.git
synced 2026-08-01 07:21:17 +00:00
fix(smartplaylist): reject NSP mixing top-level 'any' and 'all' (#5759)
* test(scanner): fix flaky Windows search_normalized rescan test The 'repopulates a stale search_normalized on a full rescan' spec runs two full scans back-to-back. Whether the second scan refreshes the unchanged artist depends on folderEntry.isOutdated(), which compares folder.updated_at (written during the first scan) against the second scan's library.last_scan_started_at using a strict time.Before(). Both are time.Now() values captured milliseconds apart. On Linux's fine-grained clock they are always distinct, so the test passes. On Windows the coarse wall-clock granularity frequently makes the two timestamps land in the same tick and compare equal, so Before() returns false, the folder is treated as up-to-date and skipped, the artist is never re-persisted, and search_normalized stays empty -- failing the assertion intermittently across unrelated PRs. Backdate the folder's updated_at an hour before the second scan so the comparison is unambiguous on every platform. This is a test-only timing artifact (real rescans never run milliseconds apart on an unchanged library), so no production code changes are needed. * fix(smartplaylist): reject NSP mixing top-level 'any' and 'all' A smart playlist (.nsp) that specified both a top-level "any" and a top-level "all" group was imported by silently keeping only "any" and discarding "all", regardless of key order. The Criteria model holds a single top-level Expression, so it cannot represent both groups, and the parser picked "any" without reporting the dropped rules. Make Criteria.UnmarshalJSON return an error when both keys are present at the top level, so the scanner fails loudly (logging the playlist as invalid) instead of silently losing rules. Users should nest one group inside the other, as shown in the documented examples. Fixes #5757 * fix(smartplaylist): reject top-level any+all by key presence Address code review feedback: the previous guard checked decoded slice lengths, so it only rejected the mixed top-level any/all form when both groups were non-empty. An input like {"any":[],"all":[...]} (or a null group) slipped past and silently used just one group — the same class of silent drop this change set out to prevent. Decode the two keys as json.RawMessage and detect presence by key rather than length, so any file that provides both top-level keys is rejected regardless of whether one group is empty or null. * refactor(smartplaylist): detect top-level any+all via presence type Replace the json.RawMessage + manual double-unmarshal in Criteria.UnmarshalJSON with a small optionalConjunction wrapper whose UnmarshalJSON records that its key was present. Because encoding/json invokes UnmarshalJSON even for a JSON null, this keeps the exact behavior (a present-but-empty or null group still counts, so mixing both top-level keys is rejected) while decoding in a single pass — no raw-message capture, no re-decode, no shadow variables. No behavior change; existing tests pass unchanged.
This commit is contained in:
parent
205c85da55
commit
e91687e760
@ -113,6 +113,20 @@ var _ = Describe("parseNSP", func() {
|
||||
Expect(err.Error()).To(ContainSubstring("SmartPlaylist"))
|
||||
})
|
||||
|
||||
It("rejects a NSP that mixes top-level 'any' and 'all' instead of silently dropping a group", func() {
|
||||
nsp := `{
|
||||
"name": "Overplayed Favorites",
|
||||
"any": [{"inPlaylist": {"path": "most-played-favorites.nsp"}}],
|
||||
"all": [{"notInPlaylist": {"path": "favorites-not-played-in-4-yrs.nsp"}}],
|
||||
"sort": "playCount, lastPlayed"
|
||||
}`
|
||||
pls := &model.Playlist{}
|
||||
err := s.parseNSP(ctx, pls, strings.NewReader(nsp))
|
||||
Expect(err).To(HaveOccurred())
|
||||
Expect(err.Error()).To(ContainSubstring("SmartPlaylist"))
|
||||
Expect(err.Error()).To(And(ContainSubstring("all"), ContainSubstring("any")))
|
||||
})
|
||||
|
||||
It("gracefully handles non-string name field", func() {
|
||||
nsp := `{"name": 123, "all": [{"is": {"loved": true}}]}`
|
||||
pls := &model.Playlist{Name: "Original"}
|
||||
|
||||
@ -103,21 +103,26 @@ func (c Criteria) MarshalJSON() ([]byte, error) {
|
||||
|
||||
func (c *Criteria) UnmarshalJSON(data []byte) error {
|
||||
var aux struct {
|
||||
All unmarshalConjunctionType `json:"all"`
|
||||
Any unmarshalConjunctionType `json:"any"`
|
||||
Sort string `json:"sort"`
|
||||
Order string `json:"order"`
|
||||
Limit int `json:"limit"`
|
||||
LimitPercent int `json:"limitPercent"`
|
||||
Offset int `json:"offset"`
|
||||
All optionalConjunction `json:"all"`
|
||||
Any optionalConjunction `json:"any"`
|
||||
Sort string `json:"sort"`
|
||||
Order string `json:"order"`
|
||||
Limit int `json:"limit"`
|
||||
LimitPercent int `json:"limitPercent"`
|
||||
Offset int `json:"offset"`
|
||||
}
|
||||
if err := json.Unmarshal(data, &aux); err != nil {
|
||||
return err
|
||||
}
|
||||
if len(aux.Any) > 0 {
|
||||
c.Expression = Any(aux.Any)
|
||||
} else if len(aux.All) > 0 {
|
||||
c.Expression = All(aux.All)
|
||||
// A Criteria has a single top-level group. Reject files that provide both keys
|
||||
// (even when one is [] or null) rather than silently dropping one of them.
|
||||
if aux.All.present && aux.Any.present {
|
||||
return errors.New("invalid criteria json: 'all' and 'any' cannot both be used at the top level; nest one inside the other instead")
|
||||
}
|
||||
if len(aux.Any.rules) > 0 {
|
||||
c.Expression = Any(aux.Any.rules)
|
||||
} else if len(aux.All.rules) > 0 {
|
||||
c.Expression = All(aux.All.rules)
|
||||
} else {
|
||||
return errors.New("invalid criteria json. missing rules (key 'all' or 'any')")
|
||||
}
|
||||
|
||||
@ -80,6 +80,28 @@ var _ = Describe("Criteria", func() {
|
||||
})
|
||||
})
|
||||
|
||||
Context("with both top-level 'all' and 'any'", func() {
|
||||
It("returns an error instead of silently dropping one of the groups", func() {
|
||||
jsonStr := `{"any":[{"inPlaylist":{"path":"a.nsp"}}],"all":[{"notInPlaylist":{"path":"b.nsp"}}]}`
|
||||
var c Criteria
|
||||
err := json.Unmarshal([]byte(jsonStr), &c)
|
||||
gomega.Expect(err).To(gomega.HaveOccurred())
|
||||
gomega.Expect(err.Error()).To(gomega.And(gomega.ContainSubstring("all"), gomega.ContainSubstring("any")))
|
||||
})
|
||||
|
||||
DescribeTable("rejects both keys even when one group is present but empty",
|
||||
func(jsonStr string) {
|
||||
var c Criteria
|
||||
err := json.Unmarshal([]byte(jsonStr), &c)
|
||||
gomega.Expect(err).To(gomega.HaveOccurred())
|
||||
gomega.Expect(err.Error()).To(gomega.And(gomega.ContainSubstring("all"), gomega.ContainSubstring("any")))
|
||||
},
|
||||
Entry("empty any", `{"any":[],"all":[{"is":{"loved":true}}]}`),
|
||||
Entry("empty all", `{"all":[],"any":[{"is":{"loved":true}}]}`),
|
||||
Entry("null any", `{"any":null,"all":[{"is":{"loved":true}}]}`),
|
||||
)
|
||||
})
|
||||
|
||||
Describe("LimitPercent", func() {
|
||||
Describe("JSON round-trip", func() {
|
||||
It("marshals and unmarshals limitPercent", func() {
|
||||
|
||||
@ -33,6 +33,20 @@ func (uc *unmarshalConjunctionType) UnmarshalJSON(data []byte) error {
|
||||
return nil
|
||||
}
|
||||
|
||||
// optionalConjunction is a top-level "all"/"any" value that remembers whether its
|
||||
// key was present at all, so a Criteria providing both can be rejected. encoding/json
|
||||
// calls UnmarshalJSON even for a JSON null, so present is set whenever the key appears
|
||||
// — including as [] or null — while an absent key leaves it false.
|
||||
type optionalConjunction struct {
|
||||
present bool
|
||||
rules unmarshalConjunctionType
|
||||
}
|
||||
|
||||
func (o *optionalConjunction) UnmarshalJSON(data []byte) error {
|
||||
o.present = true
|
||||
return json.Unmarshal(data, &o.rules)
|
||||
}
|
||||
|
||||
func unmarshalExpression(opName string, rawValue json.RawMessage) Expression {
|
||||
m := make(map[string]any)
|
||||
err := json.Unmarshal(rawValue, &m)
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user