mirror of
https://github.com/navidrome/navidrome.git
synced 2026-10-11 04:26:07 -04:00
Deluan Quintão
63a5954e4f
* 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>