mirror of
https://github.com/navidrome/navidrome.git
synced 2026-08-01 07:21:17 +00:00
* fix(smartplaylist): use annotation index for playcount/rating/loved filters
Annotation-field criteria wrapped the column in COALESCE(col, default) so
missing annotation rows behave as 0/false. COALESCE prevents SQLite from
using the column index, forcing a full media_file scan during smart playlist
materialization - multi-second loads on large libraries, independent of rule
complexity.
Store the raw column plus its default and drop COALESCE when the compared
value cannot match the default; fall back to 'col <op> ? OR col IS NULL' when
the default would match, so never-annotated tracks are still preserved.
Sorting keeps COALESCE to retain deterministic NULL ordering. Result set is
unchanged; the materialize query now seeks the annotation index.
Signed-off-by: Deluan <deluan@deluan.com>
* fix(smartplaylist): keep COALESCE for list-valued annotation comparisons
Hardening from final review: a list value (IN (...)) can't drive the index
and a default-inclusive list has per-element NULL semantics, so route slice
values through COALESCE(col, default) to stay exactly equivalent to the prior
form. Also make the bool-default branch explicit (loved only supports
equality operators) and share the COALESCE rendering via coalesceExpr.
Signed-off-by: Deluan <deluan@deluan.com>
* refactor(smartplaylist): make coalesced() a field method
Thermo-nuclear review follow-up: promote the free coalesceExpr(f) to a
smartPlaylistField.coalesced() method that returns the bare expression when
there is no default. This lets sortExpr call field.coalesced() unconditionally
and drop its 'if coalesceDefault != nil' branch, removing the 'only annotation
fields get coalesced' special case from the sort path. Behavior unchanged.
Signed-off-by: Deluan <deluan@deluan.com>
* fix(smartplaylist): keep COALESCE for LIKE, bool-ordering, and tag ranges
Code review (xhigh) found the index-friendly rewrite did not cover every
operator, breaking result-set equivalence on a few reachable raw-JSON paths:
- LIKE family (contains/startsWith/endsWith/notContains) on annotation fields
used the bare column, so a NULL column never matched and missing-annotation
rows were dropped.
- Ordering comparators (gt/lt/...) on bool fields (loved) were decided as
equality, wrongly including never-annotated rows.
- InTheRange on a numeric tag split into two independent json_tree EXISTS,
letting different tag values satisfy each bound.
Centralize the decision in annotationCond via bareNullInclusion: emit the
index-friendly bare form only for scalar values under an exactly-orderable
comparator, otherwise fall back to the COALESCE form (always equivalent to the
original). Route LIKE through coalesced(); reject tag/role ranges. Replace the
local toFloat/toBool with spf13/cast (fixes unhandled numeric types and string
bool forms), and drop the redundant LookupField + double reflect.TypeOf.
A 17-case brute-force check confirms row-set equivalence to the prior
COALESCE form across all operators including the fixed LIKE/bool cases.
Signed-off-by: Deluan <deluan@deluan.com>
* fix(smartplaylist): keep COALESCE for list values on bool annotation fields
Second review found the bareNullInclusion bool branch missed the non-scalar
guard the numeric branch has: a list value on loved/albumloved/artistloved
(e.g. {"is":{"loved":[true]}}) coerced through toBool (which swallowed the
cast error) to false, emitting the bare/OR-IS-NULL form and wrongly including
never-annotated rows. Make toBool return (value, ok) like toFloat and bail to
COALESCE when the value isn't a scalar bool. Also add the missing test for the
tag/role range rejection. A 21-case brute-force confirms row-set equivalence to
the original COALESCE form across every operator, including the bool/numeric
list paths.
Signed-off-by: Deluan <deluan@deluan.com>
* refactor(smartplaylist): drop spf13/cast for stdlib value coercion
The value coercion only sees the handful of types criteria produces (int,
float64, string from JSON; bool already normalized at unmarshal), so cast's
broad conversion isn't needed. Use small explicit type switches over strconv
instead, keeping the string fallback (ParseFloat/ParseBool) that closes the
'1'/'t' gap. No dependency change — cast returns to indirect.
Signed-off-by: Deluan <deluan@deluan.com>
* refactor(smartplaylist): share bool coercion via criteria.ToBool
normalizeBoolValue (unmarshal-time) and the persistence bool guard both parsed
bool-ish values independently. Extract the shared logic into an exported
criteria.ToBool(any) (bool, ok): normalizeBoolValue delegates to it (behavior
unchanged), and the persistence layer reuses it via its existing model/criteria
import instead of a local helper. No behavior change.
Signed-off-by: Deluan <deluan@deluan.com>
* refactor(smartplaylist): trim sqlLiteral and dedup rationale comments
/simplify cleanup: fmt %v already renders bool defaults as false/true, so drop
sqlLiteral's redundant bool branch. Consolidate the COALESCE-vs-index rationale
to the smartPlaylistField comment instead of repeating it across annotationCond
and the struct. No behavior change.
Signed-off-by: Deluan <deluan@deluan.com>
* fix(smartplaylist): address review feedback on multi-field maps and *any
From the PR bot reviews:
- sqlFields now uses the field's coalesced() form, so annotation fields in a
multi-field operator map (Is/Gt/Contains with >1 key) keep COALESCE and don't
silently drop never-annotated rows. Covers both the comparison and LIKE
fallback paths. (Gemini high, Copilot)
- Replace coalesceDefault *any with a plain any (0/false are non-nil
interfaces, so nil still means 'no default'); drop the coalesce() boxing
helper and the pointer indirection. (Gemini)
- Give rangeExpr clear, range-specific errors for the multi-field and malformed
-pair cases instead of an empty-field / 'in operator' message. (Copilot)
Adds tests for the multi-field COALESCE behavior and the new range errors.
Signed-off-by: Deluan <deluan@deluan.com>
* Revert multi-field COALESCE handling (YAGNI)
The multi-field operator map case the bots flagged is unreachable: marshalExpression
rejects any operator map with more than one field, so a multi-field map can never be
persisted or loaded. Revert the sqlFields change and its tests rather than harden a
code path no supported input can reach. Keep the two reachable improvements from the
review: coalesceDefault any (not *any), and the clearer malformed-range error.
Signed-off-by: Deluan <deluan@deluan.com>
* refactor(persistence): model comparator as a behavior-carrying struct
The smart-playlist comparator was a bare string alias, forcing two parallel
switches over the same six operators: squirrelCmp mapped each to its squirrel
constructor, and bareNullInclusion restated each as a float predicate. Adding
or changing an operator meant editing both in sync.
Make comparator a struct that bundles those facts per operator (the squirrel
builder, the operator as a float predicate, and whether it's an ordering op).
Both switches collapse: squirrelCmp is deleted in favor of cmp.build, and
bareNullInclusion's numeric switch becomes a single cmp.satisfy call. Generated
SQL is unchanged, as the existing table-driven tests confirm.
* docs(smartplaylist): trim comments that restate the code
Remove or tighten comments that describe what the code already says (likeCond and
comparisonExpr doc lines, redundant clauses in annotationField/coalesced/ToBool/
normalizeBoolValue). Keep the comments that explain non-obvious rationale: the
COALESCE-vs-index tradeoff, the bareNullInclusion/annotationCond contracts, and the
why-we-fall-back notes.
* docs(smartplaylist): collapse coalesceDefault comment to one line
The field's six-line block duplicated the COALESCE-vs-index rationale that already
lives on annotationCond. Reduce it to a one-line description plus a pointer there.
---------
Signed-off-by: Deluan <deluan@deluan.com>
151 lines
6.6 KiB
Go
151 lines
6.6 KiB
Go
package criteria_test
|
|
|
|
import (
|
|
"encoding/json"
|
|
"fmt"
|
|
|
|
. "github.com/navidrome/navidrome/model/criteria"
|
|
. "github.com/onsi/ginkgo/v2"
|
|
"github.com/onsi/gomega"
|
|
)
|
|
|
|
var _ = BeforeSuite(func() {
|
|
AddRoles([]string{"artist", "composer"})
|
|
AddTagNames([]string{"genre"})
|
|
AddNumericTags([]string{"rate"})
|
|
})
|
|
|
|
var _ = Describe("Operators", func() {
|
|
DescribeTable("JSON Marshaling",
|
|
func(op Expression, jsonString string) {
|
|
obj := And{op}
|
|
newJs, err := json.Marshal(obj)
|
|
gomega.Expect(err).ToNot(gomega.HaveOccurred())
|
|
gomega.Expect(string(newJs)).To(gomega.Equal(fmt.Sprintf(`{"all":[%s]}`, jsonString)))
|
|
|
|
var unmarshalObj UnmarshalConjunctionType
|
|
js := "[" + jsonString + "]"
|
|
err = json.Unmarshal([]byte(js), &unmarshalObj)
|
|
gomega.Expect(err).ToNot(gomega.HaveOccurred())
|
|
gomega.Expect(unmarshalObj[0]).To(gomega.Equal(op))
|
|
},
|
|
Entry("is [string]", Is{"title": "Low Rider"}, `{"is":{"title":"Low Rider"}}`),
|
|
Entry("is [bool]", Is{"loved": false}, `{"is":{"loved":false}}`),
|
|
Entry("is [string does not coerce non-boolean field]", Is{"title": "true"}, `{"is":{"title":"true"}}`),
|
|
Entry("isNot", IsNot{"title": "Low Rider"}, `{"isNot":{"title":"Low Rider"}}`),
|
|
Entry("gt", Gt{"playCount": 10.0}, `{"gt":{"playCount":10}}`),
|
|
Entry("lt", Lt{"playCount": 10.0}, `{"lt":{"playCount":10}}`),
|
|
Entry("contains", Contains{"title": "Low Rider"}, `{"contains":{"title":"Low Rider"}}`),
|
|
Entry("notContains", NotContains{"title": "Low Rider"}, `{"notContains":{"title":"Low Rider"}}`),
|
|
Entry("startsWith", StartsWith{"title": "Low Rider"}, `{"startsWith":{"title":"Low Rider"}}`),
|
|
Entry("endsWith", EndsWith{"title": "Low Rider"}, `{"endsWith":{"title":"Low Rider"}}`),
|
|
Entry("inTheRange [number]", InTheRange{"year": []any{1980.0, 1990.0}}, `{"inTheRange":{"year":[1980,1990]}}`),
|
|
Entry("inTheRange [date]", InTheRange{"lastPlayed": []any{"2021-10-01", "2021-11-01"}}, `{"inTheRange":{"lastPlayed":["2021-10-01","2021-11-01"]}}`),
|
|
Entry("before", Before{"lastPlayed": "2021-10-01"}, `{"before":{"lastPlayed":"2021-10-01"}}`),
|
|
Entry("after", After{"lastPlayed": "2021-10-01"}, `{"after":{"lastPlayed":"2021-10-01"}}`),
|
|
Entry("inTheLast", InTheLast{"lastPlayed": 30.0}, `{"inTheLast":{"lastPlayed":30}}`),
|
|
Entry("notInTheLast", NotInTheLast{"lastPlayed": 30.0}, `{"notInTheLast":{"lastPlayed":30}}`),
|
|
Entry("inPlaylist", InPlaylist{"id": "deadbeef-dead-beef"}, `{"inPlaylist":{"id":"deadbeef-dead-beef"}}`),
|
|
Entry("notInPlaylist", NotInPlaylist{"id": "deadbeef-dead-beef"}, `{"notInPlaylist":{"id":"deadbeef-dead-beef"}}`),
|
|
Entry("isMissing [true]", IsMissing{"genre": true}, `{"isMissing":{"genre":true}}`),
|
|
Entry("isMissing [false]", IsMissing{"genre": false}, `{"isMissing":{"genre":false}}`),
|
|
Entry("isPresent [true]", IsPresent{"genre": true}, `{"isPresent":{"genre":true}}`),
|
|
Entry("isPresent [false]", IsPresent{"genre": false}, `{"isPresent":{"genre":false}}`),
|
|
)
|
|
|
|
Describe("Boolean string coercion at unmarshal time (issue #4826)", func() {
|
|
It("coerces string 'true' to bool for boolean fields", func() {
|
|
var obj UnmarshalConjunctionType
|
|
err := json.Unmarshal([]byte(`[{"is":{"loved":"true"}}]`), &obj)
|
|
gomega.Expect(err).ToNot(gomega.HaveOccurred())
|
|
gomega.Expect(obj[0]).To(gomega.Equal(Is{"loved": true}))
|
|
})
|
|
|
|
It("coerces string 'false' to bool for boolean fields", func() {
|
|
var obj UnmarshalConjunctionType
|
|
err := json.Unmarshal([]byte(`[{"is":{"loved":"false"}}]`), &obj)
|
|
gomega.Expect(err).ToNot(gomega.HaveOccurred())
|
|
gomega.Expect(obj[0]).To(gomega.Equal(Is{"loved": false}))
|
|
})
|
|
|
|
It("does not coerce string values for non-boolean fields", func() {
|
|
var obj UnmarshalConjunctionType
|
|
err := json.Unmarshal([]byte(`[{"is":{"title":"true"}}]`), &obj)
|
|
gomega.Expect(err).ToNot(gomega.HaveOccurred())
|
|
gomega.Expect(obj[0]).To(gomega.Equal(Is{"title": "true"}))
|
|
})
|
|
|
|
It("coerces numeric 1 to bool true for boolean fields", func() {
|
|
var obj UnmarshalConjunctionType
|
|
err := json.Unmarshal([]byte(`[{"is":{"loved":1}}]`), &obj)
|
|
gomega.Expect(err).ToNot(gomega.HaveOccurred())
|
|
gomega.Expect(obj[0]).To(gomega.Equal(Is{"loved": true}))
|
|
})
|
|
|
|
It("coerces numeric 0 to bool false for boolean fields", func() {
|
|
var obj UnmarshalConjunctionType
|
|
err := json.Unmarshal([]byte(`[{"is":{"loved":0}}]`), &obj)
|
|
gomega.Expect(err).ToNot(gomega.HaveOccurred())
|
|
gomega.Expect(obj[0]).To(gomega.Equal(Is{"loved": false}))
|
|
})
|
|
|
|
It("coerces in nested any/all groups", func() {
|
|
var c Criteria
|
|
err := json.Unmarshal([]byte(`{"all":[{"contains":{"title":"love"}},{"any":[{"is":{"loved":"true"}}]}]}`), &c)
|
|
gomega.Expect(err).ToNot(gomega.HaveOccurred())
|
|
all := c.Expression.(All)
|
|
nested := all[1].(Any)
|
|
gomega.Expect(nested[0]).To(gomega.Equal(Is{"loved": true}))
|
|
})
|
|
|
|
It("coerces isMissing string 'true' to bool", func() {
|
|
var obj UnmarshalConjunctionType
|
|
err := json.Unmarshal([]byte(`[{"isMissing":{"genre":"true"}}]`), &obj)
|
|
gomega.Expect(err).ToNot(gomega.HaveOccurred())
|
|
gomega.Expect(obj[0]).To(gomega.Equal(IsMissing{"genre": true}))
|
|
})
|
|
|
|
It("coerces isMissing numeric 0 to bool false", func() {
|
|
var obj UnmarshalConjunctionType
|
|
err := json.Unmarshal([]byte(`[{"isMissing":{"genre":0}}]`), &obj)
|
|
gomega.Expect(err).ToNot(gomega.HaveOccurred())
|
|
gomega.Expect(obj[0]).To(gomega.Equal(IsMissing{"genre": false}))
|
|
})
|
|
|
|
It("coerces isPresent string 'false' to bool", func() {
|
|
var obj UnmarshalConjunctionType
|
|
err := json.Unmarshal([]byte(`[{"isPresent":{"genre":"false"}}]`), &obj)
|
|
gomega.Expect(err).ToNot(gomega.HaveOccurred())
|
|
gomega.Expect(obj[0]).To(gomega.Equal(IsPresent{"genre": false}))
|
|
})
|
|
|
|
It("coerces isPresent numeric 1 to bool true", func() {
|
|
var obj UnmarshalConjunctionType
|
|
err := json.Unmarshal([]byte(`[{"isPresent":{"genre":1}}]`), &obj)
|
|
gomega.Expect(err).ToNot(gomega.HaveOccurred())
|
|
gomega.Expect(obj[0]).To(gomega.Equal(IsPresent{"genre": true}))
|
|
})
|
|
})
|
|
|
|
DescribeTable("ToBool",
|
|
func(in any, wantVal, wantOk bool) {
|
|
got, ok := ToBool(in)
|
|
gomega.Expect(ok).To(gomega.Equal(wantOk))
|
|
gomega.Expect(got).To(gomega.Equal(wantVal))
|
|
},
|
|
Entry("real bool true", true, true, true),
|
|
Entry("real bool false", false, false, true),
|
|
Entry("string true", "true", true, true),
|
|
Entry("string false", "false", false, true),
|
|
Entry("string 1", "1", true, true),
|
|
Entry("string t", "t", true, true),
|
|
Entry("string 0", "0", false, true),
|
|
Entry("string unparseable", "yes", false, false),
|
|
Entry("float64 1", float64(1), true, true),
|
|
Entry("float64 0", float64(0), false, true),
|
|
Entry("float64 other", float64(2), false, false),
|
|
Entry("slice", []any{true}, false, false),
|
|
Entry("nil", nil, false, false),
|
|
)
|
|
})
|