2026-04-24 23:18:20 -04:00
|
|
|
package persistence
|
|
|
|
|
|
|
|
|
|
import (
|
|
|
|
|
"errors"
|
|
|
|
|
"fmt"
|
2026-05-22 18:00:13 -03:00
|
|
|
"maps"
|
2026-04-24 23:18:20 -04:00
|
|
|
"reflect"
|
2026-05-22 18:00:13 -03:00
|
|
|
"slices"
|
2026-04-24 23:18:20 -04:00
|
|
|
"strconv"
|
|
|
|
|
"strings"
|
|
|
|
|
"time"
|
|
|
|
|
|
2026-04-25 14:59:06 -04:00
|
|
|
"github.com/Masterminds/squirrel"
|
refactor: centralize criteria sort parsing and extract smart playlist logic (#5415)
* test: add tests for recordingdate alias resolution in smart playlists
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: update FieldInfo structure and simplify fieldMap initialization
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: move sort parsing logic from persistence to criteria package
Extracted sort field parsing, validation, and direction handling from
persistence/criteria_sql.go into model/criteria/sort.go. The new
OrderByFields method on Criteria parses the Sort/Order strings into
validated SortField structs (field name + direction), resolving aliases
and handling +/- prefixes and order inversion. The persistence layer now
consumes these parsed fields and only handles SQL expression mapping.
This centralizes sort parsing to enforce consistent implementations.
* refactor: standardize field access in smartPlaylistCriteria structure
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: add ResolveLimit method to Criteria
Moved the percentage-limit resolution logic from playlist_repository
into Criteria.ResolveLimit, replacing the 3-line mutate-after-query
pattern with a single method call. The method preserves LimitPercent
rather than zeroing it, since IsPercentageLimit already returns false
once Limit is set, making the clear redundant and lossy.
* refactor: improve child playlist loading and error handling in refresh logic
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: extract smart playlist logic to dedicated files
Moved refreshSmartPlaylist, addSmartPlaylistAnnotationJoins, and
addCriteria methods from playlist_repository.go to a new
smart_playlist_repository.go file. Extracted all smart playlist tests
to smart_playlist_repository_test.go. Added DeferCleanup to the
"valid rules" test to fix ordering flakiness when Ginkgo randomizes
test execution across files.
* refactor: break refreshSmartPlaylist into smaller focused methods
Split the monolithic refreshSmartPlaylist method into discrete helpers
for readability: shouldRefreshSmartPlaylist for guard checks,
refreshChildPlaylists for recursive dependency refresh,
resolvePercentageLimit for count-based limit resolution,
buildSmartPlaylistQuery for assembling the SELECT with joins, and
addMediaFileAnnotationJoin to DRY up the repeated annotation join clause.
* refactor: deduplicate child playlist IDs in Criteria
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: simplify withSmartPlaylistOwner to accept model.User
Replaced separate ownerID string and ownerIsAdmin bool parameters with a
single model.User struct, reducing the field count in smartPlaylistCriteria
and making the option function signature clearer. Updated all call sites
and tests accordingly.
* fix: handle empty sort fields and propagate child playlist load errors
OrderByFields now falls back to [{title, asc}] when all user-supplied
sort fields are invalid, preventing empty ORDER BY clauses that would
produce invalid SQL in row_number() window functions. Also restored the
original behavior where a DB error loading child playlists aborts the
parent smart playlist refresh, by making refreshChildPlaylists return a
bool.
* refactor: log warning when no valid sort fields are found
Signed-off-by: Deluan <deluan@navidrome.org>
---------
Signed-off-by: Deluan <deluan@navidrome.org>
2026-04-26 14:49:59 -04:00
|
|
|
"github.com/navidrome/navidrome/model"
|
2026-04-24 23:18:20 -04:00
|
|
|
"github.com/navidrome/navidrome/model/criteria"
|
feat(smartplaylists): add support for referencing playlists using paths (#5187)
* feat: Add support for referencing playlists using paths
Signed-off-by: David <dvedvick@gmail.com>
* feat: Support relative playlist paths in smartlists
Signed-off-by: David <dvedvick@gmail.com>
* fix(smartplaylists): protect against nil panic
Signed-off-by: David <dvedvick@gmail.com>
* fix(smartplaylists): refreshing child playlists
Signed-off-by: David <dvedvick@gmail.com>
* chore(smartplaylists): log field parsing error
Signed-off-by: David <dvedvick@gmail.com>
* fix(smartplaylists): handle empty playlist paths
Signed-off-by: David <dvedvick@gmail.com>
* refactor(smartplaylists): make NormalizeChildPaths non-mutating
Signed-off-by: David <dvedvick@gmail.com>
* fix(smartplaylists): stop warning on every inPlaylist rule without the looked-up field
Rules that reference a playlist by id have no path field, and the reverse, so
the warning fired on every refresh. The log call also had a bad argument count.
* fix(smartplaylists): ignore empty inPlaylist id and path references
An empty path matched every playlist without a file path, including the
referencing playlist itself, so the refresh recursed until the stack overflowed.
An empty id also shadowed a valid path in the same rule.
* fix(smartplaylists): match inPlaylist paths in both NFC and NFD forms
A playlist path is stored in the Unicode form the filesystem reports, which can
differ from the form typed in the .nsp file. The exact comparison then found no
playlist for names with accents.
* fix(smartplaylists): keep all criteria fields when normalizing child paths
The field-by-field copy dropped RefreshDelay.
* fix(smartplaylists): clean absolute inPlaylist path references
Only relative references were cleaned, so an absolute reference such as
/music/./child.nsp never matched the stored /music/child.nsp.
* fix(smartplaylists): stop infinite recursion on playlists that reference each other
Two smart playlists referencing each other, by id or by path, recursed until the
stack overflowed and the server died. The refresh now tracks visited playlists.
* fix(smartplaylists): resolve inPlaylist path references with OS-native separators
Playlist.Path is OS-native, but references in a .nsp file use forward slashes.
On Windows they never matched, and a leading slash was not seen as absolute.
The specs now build OS-native paths, so they also run on Windows.
* fix(smartplaylists): warn when a relative inPlaylist path cannot be resolved
A playlist created in the UI has no file path, so a relative reference silently
matched nothing.
* refactor(smartplaylists): simplify child playlist reference handling
Share one extractor for child ids and paths, return only the normalized rules
instead of a playlist copy, and resolve each path reference in a single switch.
* test(smartplaylists): store the Unicode child path in OS-native form
Playlist.Path is OS-native, so on Windows the forward-slash fixture never matched
the normalized reference.
---------
Signed-off-by: David <dvedvick@gmail.com>
Co-authored-by: Deluan Quintão <deluan@navidrome.org>
2026-09-19 20:05:58 -05:00
|
|
|
"golang.org/x/text/unicode/norm"
|
2026-04-24 23:18:20 -04:00
|
|
|
)
|
|
|
|
|
|
|
|
|
|
type smartPlaylistJoinType int
|
|
|
|
|
|
|
|
|
|
const (
|
|
|
|
|
smartPlaylistJoinNone smartPlaylistJoinType = 0
|
|
|
|
|
smartPlaylistJoinAlbumAnnotation smartPlaylistJoinType = 1 << iota
|
|
|
|
|
smartPlaylistJoinArtistAnnotation
|
feat(smartplaylist): add album-level fields for sorting and filtering (#5899)
Smart playlists could only sort by the track-level `dateadded`, which scatters
an album's tracks because every track has its own timestamp. There was no way
to express "newest albums first, tracks in album order".
Adds five fields backed by the album table: albumdateadded, albumdatemodified,
albumduration, albumsongcount and albumsize, so `"sort": "-albumdateadded,
tracknumber"` now works. They filter as well as sort, which also enables rules
like "everything from albums added this month" or "skip singles and EPs".
These need the album table rather than the album_annotation table the existing
album* fields join, so they get their own bit in the join mask. The bitmask
already unions joins from both the expression and the sort fields, so a
sort-only reference pulls in the join for the main query while correctly
staying out of the percentage-limit count query.
No COALESCE default is used. That mechanism exists because an album_annotation
row is genuinely often absent; the album row always exists and song_count,
duration and size are NOT NULL, so leaving the columns bare keeps filters
index-friendly.
albumdatemodified follows the existing track-level `datemodified` naming rather
than the `albumdateupdated` spelling used in the request.
albumreleasedate was considered and rejected: album.release_date is
allOrNothing() over the tracks, so it equals the track-level releasedate when
they agree and collapses to empty when they disagree.
Closes https://github.com/navidrome/navidrome/discussions/5347
2026-08-06 10:59:50 -04:00
|
|
|
smartPlaylistJoinAlbum
|
2026-04-24 23:18:20 -04:00
|
|
|
)
|
|
|
|
|
|
|
|
|
|
func (j smartPlaylistJoinType) has(other smartPlaylistJoinType) bool {
|
|
|
|
|
return j&other != 0
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
type smartPlaylistField struct {
|
perf(smartplaylist): use annotation index for playcount/rating/loved filters (#5662)
* 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>
2026-06-24 22:26:41 -04:00
|
|
|
expr string
|
|
|
|
|
order string
|
|
|
|
|
joinType smartPlaylistJoinType
|
|
|
|
|
emptyValues []string // additional values that encode "missing" for string columns (e.g. '[]' for lyrics)
|
|
|
|
|
coalesceDefault any // missing-row default for a nullable annotation column; nil = none. See annotationCond.
|
2026-04-24 23:18:20 -04:00
|
|
|
}
|
|
|
|
|
|
|
|
|
|
type smartPlaylistCriteria struct {
|
refactor: centralize criteria sort parsing and extract smart playlist logic (#5415)
* test: add tests for recordingdate alias resolution in smart playlists
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: update FieldInfo structure and simplify fieldMap initialization
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: move sort parsing logic from persistence to criteria package
Extracted sort field parsing, validation, and direction handling from
persistence/criteria_sql.go into model/criteria/sort.go. The new
OrderByFields method on Criteria parses the Sort/Order strings into
validated SortField structs (field name + direction), resolving aliases
and handling +/- prefixes and order inversion. The persistence layer now
consumes these parsed fields and only handles SQL expression mapping.
This centralizes sort parsing to enforce consistent implementations.
* refactor: standardize field access in smartPlaylistCriteria structure
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: add ResolveLimit method to Criteria
Moved the percentage-limit resolution logic from playlist_repository
into Criteria.ResolveLimit, replacing the 3-line mutate-after-query
pattern with a single method call. The method preserves LimitPercent
rather than zeroing it, since IsPercentageLimit already returns false
once Limit is set, making the clear redundant and lossy.
* refactor: improve child playlist loading and error handling in refresh logic
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: extract smart playlist logic to dedicated files
Moved refreshSmartPlaylist, addSmartPlaylistAnnotationJoins, and
addCriteria methods from playlist_repository.go to a new
smart_playlist_repository.go file. Extracted all smart playlist tests
to smart_playlist_repository_test.go. Added DeferCleanup to the
"valid rules" test to fix ordering flakiness when Ginkgo randomizes
test execution across files.
* refactor: break refreshSmartPlaylist into smaller focused methods
Split the monolithic refreshSmartPlaylist method into discrete helpers
for readability: shouldRefreshSmartPlaylist for guard checks,
refreshChildPlaylists for recursive dependency refresh,
resolvePercentageLimit for count-based limit resolution,
buildSmartPlaylistQuery for assembling the SELECT with joins, and
addMediaFileAnnotationJoin to DRY up the repeated annotation join clause.
* refactor: deduplicate child playlist IDs in Criteria
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: simplify withSmartPlaylistOwner to accept model.User
Replaced separate ownerID string and ownerIsAdmin bool parameters with a
single model.User struct, reducing the field count in smartPlaylistCriteria
and making the option function signature clearer. Updated all call sites
and tests accordingly.
* fix: handle empty sort fields and propagate child playlist load errors
OrderByFields now falls back to [{title, asc}] when all user-supplied
sort fields are invalid, preventing empty ORDER BY clauses that would
produce invalid SQL in row_number() window functions. Also restored the
original behavior where a DB error loading child playlists aborts the
parent smart playlist refresh, by making refreshChildPlaylists return a
bool.
* refactor: log warning when no valid sort fields are found
Signed-off-by: Deluan <deluan@navidrome.org>
---------
Signed-off-by: Deluan <deluan@navidrome.org>
2026-04-26 14:49:59 -04:00
|
|
|
criteria.Criteria
|
|
|
|
|
owner model.User
|
2026-04-24 23:18:20 -04:00
|
|
|
}
|
|
|
|
|
|
2026-04-25 14:59:06 -04:00
|
|
|
func newSmartPlaylistCriteria(c criteria.Criteria, opts ...func(*smartPlaylistCriteria)) smartPlaylistCriteria {
|
refactor: centralize criteria sort parsing and extract smart playlist logic (#5415)
* test: add tests for recordingdate alias resolution in smart playlists
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: update FieldInfo structure and simplify fieldMap initialization
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: move sort parsing logic from persistence to criteria package
Extracted sort field parsing, validation, and direction handling from
persistence/criteria_sql.go into model/criteria/sort.go. The new
OrderByFields method on Criteria parses the Sort/Order strings into
validated SortField structs (field name + direction), resolving aliases
and handling +/- prefixes and order inversion. The persistence layer now
consumes these parsed fields and only handles SQL expression mapping.
This centralizes sort parsing to enforce consistent implementations.
* refactor: standardize field access in smartPlaylistCriteria structure
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: add ResolveLimit method to Criteria
Moved the percentage-limit resolution logic from playlist_repository
into Criteria.ResolveLimit, replacing the 3-line mutate-after-query
pattern with a single method call. The method preserves LimitPercent
rather than zeroing it, since IsPercentageLimit already returns false
once Limit is set, making the clear redundant and lossy.
* refactor: improve child playlist loading and error handling in refresh logic
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: extract smart playlist logic to dedicated files
Moved refreshSmartPlaylist, addSmartPlaylistAnnotationJoins, and
addCriteria methods from playlist_repository.go to a new
smart_playlist_repository.go file. Extracted all smart playlist tests
to smart_playlist_repository_test.go. Added DeferCleanup to the
"valid rules" test to fix ordering flakiness when Ginkgo randomizes
test execution across files.
* refactor: break refreshSmartPlaylist into smaller focused methods
Split the monolithic refreshSmartPlaylist method into discrete helpers
for readability: shouldRefreshSmartPlaylist for guard checks,
refreshChildPlaylists for recursive dependency refresh,
resolvePercentageLimit for count-based limit resolution,
buildSmartPlaylistQuery for assembling the SELECT with joins, and
addMediaFileAnnotationJoin to DRY up the repeated annotation join clause.
* refactor: deduplicate child playlist IDs in Criteria
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: simplify withSmartPlaylistOwner to accept model.User
Replaced separate ownerID string and ownerIsAdmin bool parameters with a
single model.User struct, reducing the field count in smartPlaylistCriteria
and making the option function signature clearer. Updated all call sites
and tests accordingly.
* fix: handle empty sort fields and propagate child playlist load errors
OrderByFields now falls back to [{title, asc}] when all user-supplied
sort fields are invalid, preventing empty ORDER BY clauses that would
produce invalid SQL in row_number() window functions. Also restored the
original behavior where a DB error loading child playlists aborts the
parent smart playlist refresh, by making refreshChildPlaylists return a
bool.
* refactor: log warning when no valid sort fields are found
Signed-off-by: Deluan <deluan@navidrome.org>
---------
Signed-off-by: Deluan <deluan@navidrome.org>
2026-04-26 14:49:59 -04:00
|
|
|
cSQL := smartPlaylistCriteria{Criteria: c}
|
2026-04-25 14:59:06 -04:00
|
|
|
for _, opt := range opts {
|
|
|
|
|
opt(&cSQL)
|
|
|
|
|
}
|
|
|
|
|
return cSQL
|
|
|
|
|
}
|
|
|
|
|
|
refactor: centralize criteria sort parsing and extract smart playlist logic (#5415)
* test: add tests for recordingdate alias resolution in smart playlists
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: update FieldInfo structure and simplify fieldMap initialization
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: move sort parsing logic from persistence to criteria package
Extracted sort field parsing, validation, and direction handling from
persistence/criteria_sql.go into model/criteria/sort.go. The new
OrderByFields method on Criteria parses the Sort/Order strings into
validated SortField structs (field name + direction), resolving aliases
and handling +/- prefixes and order inversion. The persistence layer now
consumes these parsed fields and only handles SQL expression mapping.
This centralizes sort parsing to enforce consistent implementations.
* refactor: standardize field access in smartPlaylistCriteria structure
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: add ResolveLimit method to Criteria
Moved the percentage-limit resolution logic from playlist_repository
into Criteria.ResolveLimit, replacing the 3-line mutate-after-query
pattern with a single method call. The method preserves LimitPercent
rather than zeroing it, since IsPercentageLimit already returns false
once Limit is set, making the clear redundant and lossy.
* refactor: improve child playlist loading and error handling in refresh logic
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: extract smart playlist logic to dedicated files
Moved refreshSmartPlaylist, addSmartPlaylistAnnotationJoins, and
addCriteria methods from playlist_repository.go to a new
smart_playlist_repository.go file. Extracted all smart playlist tests
to smart_playlist_repository_test.go. Added DeferCleanup to the
"valid rules" test to fix ordering flakiness when Ginkgo randomizes
test execution across files.
* refactor: break refreshSmartPlaylist into smaller focused methods
Split the monolithic refreshSmartPlaylist method into discrete helpers
for readability: shouldRefreshSmartPlaylist for guard checks,
refreshChildPlaylists for recursive dependency refresh,
resolvePercentageLimit for count-based limit resolution,
buildSmartPlaylistQuery for assembling the SELECT with joins, and
addMediaFileAnnotationJoin to DRY up the repeated annotation join clause.
* refactor: deduplicate child playlist IDs in Criteria
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: simplify withSmartPlaylistOwner to accept model.User
Replaced separate ownerID string and ownerIsAdmin bool parameters with a
single model.User struct, reducing the field count in smartPlaylistCriteria
and making the option function signature clearer. Updated all call sites
and tests accordingly.
* fix: handle empty sort fields and propagate child playlist load errors
OrderByFields now falls back to [{title, asc}] when all user-supplied
sort fields are invalid, preventing empty ORDER BY clauses that would
produce invalid SQL in row_number() window functions. Also restored the
original behavior where a DB error loading child playlists aborts the
parent smart playlist refresh, by making refreshChildPlaylists return a
bool.
* refactor: log warning when no valid sort fields are found
Signed-off-by: Deluan <deluan@navidrome.org>
---------
Signed-off-by: Deluan <deluan@navidrome.org>
2026-04-26 14:49:59 -04:00
|
|
|
func withSmartPlaylistOwner(owner model.User) func(*smartPlaylistCriteria) {
|
2026-04-25 14:59:06 -04:00
|
|
|
return func(c *smartPlaylistCriteria) {
|
refactor: centralize criteria sort parsing and extract smart playlist logic (#5415)
* test: add tests for recordingdate alias resolution in smart playlists
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: update FieldInfo structure and simplify fieldMap initialization
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: move sort parsing logic from persistence to criteria package
Extracted sort field parsing, validation, and direction handling from
persistence/criteria_sql.go into model/criteria/sort.go. The new
OrderByFields method on Criteria parses the Sort/Order strings into
validated SortField structs (field name + direction), resolving aliases
and handling +/- prefixes and order inversion. The persistence layer now
consumes these parsed fields and only handles SQL expression mapping.
This centralizes sort parsing to enforce consistent implementations.
* refactor: standardize field access in smartPlaylistCriteria structure
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: add ResolveLimit method to Criteria
Moved the percentage-limit resolution logic from playlist_repository
into Criteria.ResolveLimit, replacing the 3-line mutate-after-query
pattern with a single method call. The method preserves LimitPercent
rather than zeroing it, since IsPercentageLimit already returns false
once Limit is set, making the clear redundant and lossy.
* refactor: improve child playlist loading and error handling in refresh logic
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: extract smart playlist logic to dedicated files
Moved refreshSmartPlaylist, addSmartPlaylistAnnotationJoins, and
addCriteria methods from playlist_repository.go to a new
smart_playlist_repository.go file. Extracted all smart playlist tests
to smart_playlist_repository_test.go. Added DeferCleanup to the
"valid rules" test to fix ordering flakiness when Ginkgo randomizes
test execution across files.
* refactor: break refreshSmartPlaylist into smaller focused methods
Split the monolithic refreshSmartPlaylist method into discrete helpers
for readability: shouldRefreshSmartPlaylist for guard checks,
refreshChildPlaylists for recursive dependency refresh,
resolvePercentageLimit for count-based limit resolution,
buildSmartPlaylistQuery for assembling the SELECT with joins, and
addMediaFileAnnotationJoin to DRY up the repeated annotation join clause.
* refactor: deduplicate child playlist IDs in Criteria
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: simplify withSmartPlaylistOwner to accept model.User
Replaced separate ownerID string and ownerIsAdmin bool parameters with a
single model.User struct, reducing the field count in smartPlaylistCriteria
and making the option function signature clearer. Updated all call sites
and tests accordingly.
* fix: handle empty sort fields and propagate child playlist load errors
OrderByFields now falls back to [{title, asc}] when all user-supplied
sort fields are invalid, preventing empty ORDER BY clauses that would
produce invalid SQL in row_number() window functions. Also restored the
original behavior where a DB error loading child playlists aborts the
parent smart playlist refresh, by making refreshChildPlaylists return a
bool.
* refactor: log warning when no valid sort fields are found
Signed-off-by: Deluan <deluan@navidrome.org>
---------
Signed-off-by: Deluan <deluan@navidrome.org>
2026-04-26 14:49:59 -04:00
|
|
|
c.owner = owner
|
2026-04-25 14:59:06 -04:00
|
|
|
}
|
2026-04-24 23:18:20 -04:00
|
|
|
}
|
|
|
|
|
|
|
|
|
|
var smartPlaylistFields = map[string]smartPlaylistField{
|
|
|
|
|
"title": {expr: "media_file.title"},
|
|
|
|
|
"album": {expr: "media_file.album"},
|
|
|
|
|
"hascoverart": {expr: "media_file.has_cover_art"},
|
|
|
|
|
"tracknumber": {expr: "media_file.track_number"},
|
|
|
|
|
"discnumber": {expr: "media_file.disc_number"},
|
|
|
|
|
"year": {expr: "media_file.year"},
|
|
|
|
|
"date": {expr: "media_file.date"},
|
|
|
|
|
"originalyear": {expr: "media_file.original_year"},
|
|
|
|
|
"originaldate": {expr: "media_file.original_date"},
|
|
|
|
|
"releaseyear": {expr: "media_file.release_year"},
|
|
|
|
|
"releasedate": {expr: "media_file.release_date"},
|
|
|
|
|
"size": {expr: "media_file.size"},
|
|
|
|
|
"compilation": {expr: "media_file.compilation"},
|
|
|
|
|
"missing": {expr: "media_file.missing"},
|
|
|
|
|
"explicitstatus": {expr: "media_file.explicit_status"},
|
|
|
|
|
"dateadded": {expr: "media_file.created_at"},
|
|
|
|
|
"datemodified": {expr: "media_file.updated_at"},
|
|
|
|
|
"discsubtitle": {expr: "media_file.disc_subtitle"},
|
|
|
|
|
"comment": {expr: "media_file.comment"},
|
feat(smartplaylist): extend isMissing/isPresent to bpm, bitDepth and many text fields (#5603)
* feat(smartplaylist): support isMissing/isPresent on mbz_* and lyrics fields
Mark the six mbz_* MusicBrainz ID columns and the lyrics column as Nullable
in the criteria field map, then extend missingExpr to handle string columns
where absence is encoded as NULL or empty string (plus '[]' for lyrics). The
Numeric/Boolean path (ReplayGain) is preserved via an explicit type check.
* refactor(model): make MediaFile BPM and BitDepth nullable pointers
Convert BPM and BitDepth fields in model.MediaFile from int to *int so that
'tag absent' is distinguishable from zero. The metadata mapper now uses
NullableFloat for BPM (nil when absent or zero/unparseable) and only sets
BitDepth when the audio property is non-zero (lossy codecs report 0).
All read sites use gg.V() for zero-fallback deref so Subsonic API output and
transcoding behaviour are byte-identical to before. The persistence layer
bridges the existing NOT NULL DB columns by coercing nil to 0 on write and 0
back to nil on read in PostMapArgs/PostScan; a later migration task will drop
those constraints.
Hash upgrade safety is verified by a new MediaFile.Hash describe block: nil
*int hashes identically to the old int(0) default via ZeroNil+IgnoreZeroValue,
so no files will be spuriously re-imported after this change.
Extra files touched beyond the plan's list: core/stream/legacy_client_test.go
(BitDepth in model.MediaFile literals), persistence/mediafile_repository.go
(NOT NULL bridge).
* test(model): pin pre-conversion golden hashes for BPM/BitDepth
* feat(smartplaylist): support isMissing/isPresent on bpm and bitDepth
* feat(db): make bpm and bit_depth columns nullable, backfill 0 to NULL
Drop the NOT NULL constraint on media_file.bpm and bit_depth via a
lossless migration that converts legacy 0-means-absent values to real
NULL. Remove the temporary shim in PostScan/PostMapArgs that was bridging
the old NOT NULL columns to the *int model fields. Add round-trip
persistence tests asserting NULL storage for nil pointers and correct
value round-trip for non-nil pointers.
* test(e2e): verify isMissing/isPresent partition for nullable fields
Add DescribeTable covering bpm, bitdepth, lyrics, and mbz_recording_id:
for each field, isMissing + isPresent song counts must equal the total
library count, proving the nullable-column SQL is exhaustive and correct.
* test(e2e): seed bpm tag so isMissing/isPresent partition is non-trivial
* fix(model): omit bitDepth from JSON when absent instead of emitting null
* feat(smartplaylist): support isMissing/isPresent on more string fields
Enable isMissing/isPresent operators for album, comment, catalognumber,
discsubtitle, albumcomment, sorttitle, sortalbum, sortartist,
sortalbumartist, and explicitstatus by marking them Nullable in fieldMap.
* refactor(smartplaylist): unify missingExpr column logic into one flow
Collapse the numeric/string fork in missingExpr into a single
empties-driven loop (numeric/boolean fields simply have no empties),
and replace the duplicated IsTag/IsRole guard with a three-way switch
that expresses the dispatch model once. No SQL semantics change for
string fields; numeric/boolean fields now emit a single-element Or/And
which squirrel parenthesizes (e.g. `(col IS NULL)` instead of bare
`col IS NULL`) — update the affected test expectations accordingly.
2026-06-13 13:15:20 -04:00
|
|
|
"lyrics": {expr: "media_file.lyrics", emptyValues: []string{"[]"}},
|
2026-04-24 23:18:20 -04:00
|
|
|
"sorttitle": {expr: "media_file.sort_title"},
|
|
|
|
|
"sortalbum": {expr: "media_file.sort_album_name"},
|
|
|
|
|
"sortartist": {expr: "media_file.sort_artist_name"},
|
|
|
|
|
"sortalbumartist": {expr: "media_file.sort_album_artist_name"},
|
|
|
|
|
"albumcomment": {expr: "media_file.mbz_album_comment"},
|
|
|
|
|
"catalognumber": {expr: "media_file.catalog_num"},
|
|
|
|
|
"filepath": {expr: "media_file.path"},
|
|
|
|
|
"filetype": {expr: "media_file.suffix"},
|
|
|
|
|
"codec": {expr: "media_file.codec"},
|
|
|
|
|
"duration": {expr: "media_file.duration"},
|
|
|
|
|
"bitrate": {expr: "media_file.bit_rate"},
|
|
|
|
|
"bitdepth": {expr: "media_file.bit_depth"},
|
|
|
|
|
"samplerate": {expr: "media_file.sample_rate"},
|
|
|
|
|
"bpm": {expr: "media_file.bpm"},
|
|
|
|
|
"channels": {expr: "media_file.channels"},
|
perf(smartplaylist): use annotation index for playcount/rating/loved filters (#5662)
* 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>
2026-06-24 22:26:41 -04:00
|
|
|
"loved": {expr: "annotation.starred", coalesceDefault: false},
|
2026-04-24 23:18:20 -04:00
|
|
|
"dateloved": {expr: "annotation.starred_at"},
|
|
|
|
|
"lastplayed": {expr: "annotation.play_date"},
|
|
|
|
|
"daterated": {expr: "annotation.rated_at"},
|
perf(smartplaylist): use annotation index for playcount/rating/loved filters (#5662)
* 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>
2026-06-24 22:26:41 -04:00
|
|
|
"playcount": {expr: "annotation.play_count", coalesceDefault: 0},
|
|
|
|
|
"rating": {expr: "annotation.rating", coalesceDefault: 0},
|
2026-04-24 23:18:20 -04:00
|
|
|
"averagerating": {expr: "media_file.average_rating"},
|
perf(smartplaylist): use annotation index for playcount/rating/loved filters (#5662)
* 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>
2026-06-24 22:26:41 -04:00
|
|
|
"albumrating": {expr: "album_annotation.rating", coalesceDefault: 0, joinType: smartPlaylistJoinAlbumAnnotation},
|
|
|
|
|
"albumloved": {expr: "album_annotation.starred", coalesceDefault: false, joinType: smartPlaylistJoinAlbumAnnotation},
|
|
|
|
|
"albumplaycount": {expr: "album_annotation.play_count", coalesceDefault: 0, joinType: smartPlaylistJoinAlbumAnnotation},
|
2026-04-24 23:18:20 -04:00
|
|
|
"albumlastplayed": {expr: "album_annotation.play_date", joinType: smartPlaylistJoinAlbumAnnotation},
|
|
|
|
|
"albumdateloved": {expr: "album_annotation.starred_at", joinType: smartPlaylistJoinAlbumAnnotation},
|
|
|
|
|
"albumdaterated": {expr: "album_annotation.rated_at", joinType: smartPlaylistJoinAlbumAnnotation},
|
feat(smartplaylist): add album-level fields for sorting and filtering (#5899)
Smart playlists could only sort by the track-level `dateadded`, which scatters
an album's tracks because every track has its own timestamp. There was no way
to express "newest albums first, tracks in album order".
Adds five fields backed by the album table: albumdateadded, albumdatemodified,
albumduration, albumsongcount and albumsize, so `"sort": "-albumdateadded,
tracknumber"` now works. They filter as well as sort, which also enables rules
like "everything from albums added this month" or "skip singles and EPs".
These need the album table rather than the album_annotation table the existing
album* fields join, so they get their own bit in the join mask. The bitmask
already unions joins from both the expression and the sort fields, so a
sort-only reference pulls in the join for the main query while correctly
staying out of the percentage-limit count query.
No COALESCE default is used. That mechanism exists because an album_annotation
row is genuinely often absent; the album row always exists and song_count,
duration and size are NOT NULL, so leaving the columns bare keeps filters
index-friendly.
albumdatemodified follows the existing track-level `datemodified` naming rather
than the `albumdateupdated` spelling used in the request.
albumreleasedate was considered and rejected: album.release_date is
allOrNothing() over the tracks, so it equals the track-level releasedate when
they agree and collapses to empty when they disagree.
Closes https://github.com/navidrome/navidrome/discussions/5347
2026-08-06 10:59:50 -04:00
|
|
|
"albumdateadded": {expr: "album.created_at", joinType: smartPlaylistJoinAlbum},
|
|
|
|
|
"albumdatemodified": {expr: "album.updated_at", joinType: smartPlaylistJoinAlbum},
|
|
|
|
|
"albumduration": {expr: "album.duration", joinType: smartPlaylistJoinAlbum},
|
|
|
|
|
"albumsongcount": {expr: "album.song_count", joinType: smartPlaylistJoinAlbum},
|
|
|
|
|
"albumsize": {expr: "album.size", joinType: smartPlaylistJoinAlbum},
|
perf(smartplaylist): use annotation index for playcount/rating/loved filters (#5662)
* 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>
2026-06-24 22:26:41 -04:00
|
|
|
"artistrating": {expr: "artist_annotation.rating", coalesceDefault: 0, joinType: smartPlaylistJoinArtistAnnotation},
|
|
|
|
|
"artistloved": {expr: "artist_annotation.starred", coalesceDefault: false, joinType: smartPlaylistJoinArtistAnnotation},
|
|
|
|
|
"artistplaycount": {expr: "artist_annotation.play_count", coalesceDefault: 0, joinType: smartPlaylistJoinArtistAnnotation},
|
2026-04-24 23:18:20 -04:00
|
|
|
"artistlastplayed": {expr: "artist_annotation.play_date", joinType: smartPlaylistJoinArtistAnnotation},
|
|
|
|
|
"artistdateloved": {expr: "artist_annotation.starred_at", joinType: smartPlaylistJoinArtistAnnotation},
|
|
|
|
|
"artistdaterated": {expr: "artist_annotation.rated_at", joinType: smartPlaylistJoinArtistAnnotation},
|
|
|
|
|
"mbz_album_id": {expr: "media_file.mbz_album_id"},
|
|
|
|
|
"mbz_album_artist_id": {expr: "media_file.mbz_album_artist_id"},
|
|
|
|
|
"mbz_artist_id": {expr: "media_file.mbz_artist_id"},
|
|
|
|
|
"mbz_recording_id": {expr: "media_file.mbz_recording_id"},
|
|
|
|
|
"mbz_release_track_id": {expr: "media_file.mbz_release_track_id"},
|
|
|
|
|
"mbz_release_group_id": {expr: "media_file.mbz_release_group_id"},
|
2026-04-28 19:46:12 -04:00
|
|
|
"rgalbumgain": {expr: "media_file.rg_album_gain"},
|
|
|
|
|
"rgalbumpeak": {expr: "media_file.rg_album_peak"},
|
|
|
|
|
"rgtrackgain": {expr: "media_file.rg_track_gain"},
|
|
|
|
|
"rgtrackpeak": {expr: "media_file.rg_track_peak"},
|
2026-04-24 23:18:20 -04:00
|
|
|
"library_id": {expr: "media_file.library_id"},
|
|
|
|
|
"random": {order: "random()"},
|
|
|
|
|
}
|
|
|
|
|
|
feat(scrobbler): add per-user scrobble filter (#5964)
* feat(scrobbler): add scrobble_filter column to user
* feat(scrobbler): validate scrobble filter criteria on user save
* refactor(persistence): make smart playlist join helpers package-level
* feat(scrobbler): add MediaFileRepository.MatchesCriteria
* feat(scrobbler): filter external scrobbles with per-user criteria
* feat(ui): add scrobble filter field to user form
* fix(scrobbler): default scrobble_filter to empty string for existing users
* refactor(scrobbler): also gate playback reports on the scrobble filter
Playback reports carry the same track metadata to plugin scrobblers, so a
filtered track leaked through that third dispatch path. Skip the filter
evaluation entirely when no scrobbler is active.
* refactor(persistence): move criteria join building into criteria_sql.go
The join set a criteria needs was decided in criteria_sql.go but built in
smart_playlist_repository.go, so both callers had to pair the two by hand.
* refactor(persistence): unexport smartPlaylistCriteria methods
The type never leaves the package, so the exported names advertised an API
that callers outside persistence could never reach. Also disambiguates
where/orderBy from squirrel's SelectBuilder methods of the same name.
* fix(ui): cap the scrobble filter field width
fullWidth stretched it across the whole page next to 256px inputs. Bounded
at 40em, with two rows and a resize handle so JSON rules stay readable.
* refactor(ui): move scrobble filter input in UserEdit component
* feat(ui): add pt-BR translations for the scrobble filter
* fix(scrobbler): take the filter verdict before incPlay
incPlay mutates play counts and dates a filter can test on, so evaluating at
dispatch time let one play decide differently on either side of the increment:
a track could be scrobbled despite matching, or lose only its stopped report
and strand presence plugins. Reject limit/offset too, rather than silently
ignoring part of a rule copied from a smart playlist.
* fix(scrobbler): filter the report from an expired session
The expiry callback runs with a stub user carrying no filter, so evaluating
there always returned false and leaked the track to plugin scrobblers. That is
the normal path for clients that never send stopped, such as legacy Subsonic
now-playing. Carry the last verdict on the session instead.
* refactor(scrobbler): skip the now-playing enqueue instead of threading the verdict
Queuing an entry only to drop it at dispatch also cancelled a pending
announcement for the previous, unfiltered track, since the queue is keyed by
player and a new entry replaces the old one.
* fix(scrobbler): evaluate the filter regardless of active scrobblers
The verdict is stored on the session and dispatched at expiry, so skipping
evaluation when no scrobbler was active let a plugin enabled mid-session
receive a filtered track. The empty-filter guard above already gives servers
without scrobbling the same free path, so the shortcut only ever applied to
users who had a filter set.
2026-08-15 16:10:53 -04:00
|
|
|
func (c smartPlaylistCriteria) where() (squirrel.Sqlizer, error) {
|
refactor: centralize criteria sort parsing and extract smart playlist logic (#5415)
* test: add tests for recordingdate alias resolution in smart playlists
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: update FieldInfo structure and simplify fieldMap initialization
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: move sort parsing logic from persistence to criteria package
Extracted sort field parsing, validation, and direction handling from
persistence/criteria_sql.go into model/criteria/sort.go. The new
OrderByFields method on Criteria parses the Sort/Order strings into
validated SortField structs (field name + direction), resolving aliases
and handling +/- prefixes and order inversion. The persistence layer now
consumes these parsed fields and only handles SQL expression mapping.
This centralizes sort parsing to enforce consistent implementations.
* refactor: standardize field access in smartPlaylistCriteria structure
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: add ResolveLimit method to Criteria
Moved the percentage-limit resolution logic from playlist_repository
into Criteria.ResolveLimit, replacing the 3-line mutate-after-query
pattern with a single method call. The method preserves LimitPercent
rather than zeroing it, since IsPercentageLimit already returns false
once Limit is set, making the clear redundant and lossy.
* refactor: improve child playlist loading and error handling in refresh logic
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: extract smart playlist logic to dedicated files
Moved refreshSmartPlaylist, addSmartPlaylistAnnotationJoins, and
addCriteria methods from playlist_repository.go to a new
smart_playlist_repository.go file. Extracted all smart playlist tests
to smart_playlist_repository_test.go. Added DeferCleanup to the
"valid rules" test to fix ordering flakiness when Ginkgo randomizes
test execution across files.
* refactor: break refreshSmartPlaylist into smaller focused methods
Split the monolithic refreshSmartPlaylist method into discrete helpers
for readability: shouldRefreshSmartPlaylist for guard checks,
refreshChildPlaylists for recursive dependency refresh,
resolvePercentageLimit for count-based limit resolution,
buildSmartPlaylistQuery for assembling the SELECT with joins, and
addMediaFileAnnotationJoin to DRY up the repeated annotation join clause.
* refactor: deduplicate child playlist IDs in Criteria
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: simplify withSmartPlaylistOwner to accept model.User
Replaced separate ownerID string and ownerIsAdmin bool parameters with a
single model.User struct, reducing the field count in smartPlaylistCriteria
and making the option function signature clearer. Updated all call sites
and tests accordingly.
* fix: handle empty sort fields and propagate child playlist load errors
OrderByFields now falls back to [{title, asc}] when all user-supplied
sort fields are invalid, preventing empty ORDER BY clauses that would
produce invalid SQL in row_number() window functions. Also restored the
original behavior where a DB error loading child playlists aborts the
parent smart playlist refresh, by making refreshChildPlaylists return a
bool.
* refactor: log warning when no valid sort fields are found
Signed-off-by: Deluan <deluan@navidrome.org>
---------
Signed-off-by: Deluan <deluan@navidrome.org>
2026-04-26 14:49:59 -04:00
|
|
|
if c.Criteria.Expression == nil {
|
2026-04-24 23:18:20 -04:00
|
|
|
return squirrel.Expr("1 = 1"), nil
|
|
|
|
|
}
|
refactor: centralize criteria sort parsing and extract smart playlist logic (#5415)
* test: add tests for recordingdate alias resolution in smart playlists
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: update FieldInfo structure and simplify fieldMap initialization
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: move sort parsing logic from persistence to criteria package
Extracted sort field parsing, validation, and direction handling from
persistence/criteria_sql.go into model/criteria/sort.go. The new
OrderByFields method on Criteria parses the Sort/Order strings into
validated SortField structs (field name + direction), resolving aliases
and handling +/- prefixes and order inversion. The persistence layer now
consumes these parsed fields and only handles SQL expression mapping.
This centralizes sort parsing to enforce consistent implementations.
* refactor: standardize field access in smartPlaylistCriteria structure
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: add ResolveLimit method to Criteria
Moved the percentage-limit resolution logic from playlist_repository
into Criteria.ResolveLimit, replacing the 3-line mutate-after-query
pattern with a single method call. The method preserves LimitPercent
rather than zeroing it, since IsPercentageLimit already returns false
once Limit is set, making the clear redundant and lossy.
* refactor: improve child playlist loading and error handling in refresh logic
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: extract smart playlist logic to dedicated files
Moved refreshSmartPlaylist, addSmartPlaylistAnnotationJoins, and
addCriteria methods from playlist_repository.go to a new
smart_playlist_repository.go file. Extracted all smart playlist tests
to smart_playlist_repository_test.go. Added DeferCleanup to the
"valid rules" test to fix ordering flakiness when Ginkgo randomizes
test execution across files.
* refactor: break refreshSmartPlaylist into smaller focused methods
Split the monolithic refreshSmartPlaylist method into discrete helpers
for readability: shouldRefreshSmartPlaylist for guard checks,
refreshChildPlaylists for recursive dependency refresh,
resolvePercentageLimit for count-based limit resolution,
buildSmartPlaylistQuery for assembling the SELECT with joins, and
addMediaFileAnnotationJoin to DRY up the repeated annotation join clause.
* refactor: deduplicate child playlist IDs in Criteria
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: simplify withSmartPlaylistOwner to accept model.User
Replaced separate ownerID string and ownerIsAdmin bool parameters with a
single model.User struct, reducing the field count in smartPlaylistCriteria
and making the option function signature clearer. Updated all call sites
and tests accordingly.
* fix: handle empty sort fields and propagate child playlist load errors
OrderByFields now falls back to [{title, asc}] when all user-supplied
sort fields are invalid, preventing empty ORDER BY clauses that would
produce invalid SQL in row_number() window functions. Also restored the
original behavior where a DB error loading child playlists aborts the
parent smart playlist refresh, by making refreshChildPlaylists return a
bool.
* refactor: log warning when no valid sort fields are found
Signed-off-by: Deluan <deluan@navidrome.org>
---------
Signed-off-by: Deluan <deluan@navidrome.org>
2026-04-26 14:49:59 -04:00
|
|
|
return c.exprSQL(c.Criteria.Expression)
|
2026-04-24 23:18:20 -04:00
|
|
|
}
|
|
|
|
|
|
2026-04-25 14:59:06 -04:00
|
|
|
func (c smartPlaylistCriteria) exprSQL(expr criteria.Expression) (squirrel.Sqlizer, error) {
|
2026-04-24 23:18:20 -04:00
|
|
|
switch e := expr.(type) {
|
|
|
|
|
case criteria.All:
|
|
|
|
|
and := squirrel.And{}
|
|
|
|
|
for _, child := range e {
|
2026-04-25 14:59:06 -04:00
|
|
|
cond, err := c.exprSQL(child)
|
2026-04-24 23:18:20 -04:00
|
|
|
if err != nil {
|
|
|
|
|
return nil, err
|
|
|
|
|
}
|
|
|
|
|
and = append(and, cond)
|
|
|
|
|
}
|
2026-06-14 10:47:11 -04:00
|
|
|
return mergeNegatedJsonConds(and), nil
|
2026-04-24 23:18:20 -04:00
|
|
|
case criteria.Any:
|
|
|
|
|
or := squirrel.Or{}
|
|
|
|
|
for _, child := range e {
|
2026-04-25 14:59:06 -04:00
|
|
|
cond, err := c.exprSQL(child)
|
2026-04-24 23:18:20 -04:00
|
|
|
if err != nil {
|
|
|
|
|
return nil, err
|
|
|
|
|
}
|
|
|
|
|
or = append(or, cond)
|
|
|
|
|
}
|
2026-05-22 18:00:13 -03:00
|
|
|
return mergeJsonConds(or), nil
|
2026-04-24 23:18:20 -04:00
|
|
|
case criteria.Is:
|
perf(smartplaylist): use annotation index for playcount/rating/loved filters (#5662)
* 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>
2026-06-24 22:26:41 -04:00
|
|
|
return comparisonExpr(e, cmpEq)
|
2026-04-24 23:18:20 -04:00
|
|
|
case criteria.IsNot:
|
|
|
|
|
return isNotExpr(e)
|
|
|
|
|
case criteria.Gt:
|
perf(smartplaylist): use annotation index for playcount/rating/loved filters (#5662)
* 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>
2026-06-24 22:26:41 -04:00
|
|
|
return comparisonExpr(e, cmpGt)
|
2026-04-24 23:18:20 -04:00
|
|
|
case criteria.Lt:
|
perf(smartplaylist): use annotation index for playcount/rating/loved filters (#5662)
* 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>
2026-06-24 22:26:41 -04:00
|
|
|
return comparisonExpr(e, cmpLt)
|
2026-04-24 23:18:20 -04:00
|
|
|
case criteria.Before:
|
perf(smartplaylist): use annotation index for playcount/rating/loved filters (#5662)
* 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>
2026-06-24 22:26:41 -04:00
|
|
|
return comparisonExpr(e, cmpLt)
|
2026-04-24 23:18:20 -04:00
|
|
|
case criteria.After:
|
perf(smartplaylist): use annotation index for playcount/rating/loved filters (#5662)
* 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>
2026-06-24 22:26:41 -04:00
|
|
|
return comparisonExpr(e, cmpGt)
|
2026-04-24 23:18:20 -04:00
|
|
|
case criteria.Contains:
|
|
|
|
|
return likeExpr(e, "%%%v%%", false)
|
|
|
|
|
case criteria.NotContains:
|
|
|
|
|
return likeExpr(e, "%%%v%%", true)
|
|
|
|
|
case criteria.StartsWith:
|
|
|
|
|
return likeExpr(e, "%v%%", false)
|
|
|
|
|
case criteria.EndsWith:
|
|
|
|
|
return likeExpr(e, "%%%v", false)
|
|
|
|
|
case criteria.InTheRange:
|
|
|
|
|
return rangeExpr(e)
|
|
|
|
|
case criteria.InTheLast:
|
|
|
|
|
return periodExpr(e, false)
|
|
|
|
|
case criteria.NotInTheLast:
|
|
|
|
|
return periodExpr(e, true)
|
|
|
|
|
case criteria.InPlaylist:
|
2026-04-25 14:59:06 -04:00
|
|
|
return c.inList(e, false)
|
2026-04-24 23:18:20 -04:00
|
|
|
case criteria.NotInPlaylist:
|
2026-04-25 14:59:06 -04:00
|
|
|
return c.inList(e, true)
|
2026-04-28 19:40:08 -04:00
|
|
|
case criteria.IsMissing:
|
|
|
|
|
return missingExpr(e, true)
|
|
|
|
|
case criteria.IsPresent:
|
|
|
|
|
return missingExpr(e, false)
|
2026-04-24 23:18:20 -04:00
|
|
|
default:
|
|
|
|
|
return nil, fmt.Errorf("unknown criteria expression type %T", expr)
|
|
|
|
|
}
|
|
|
|
|
}
|
|
|
|
|
|
2026-04-25 14:59:06 -04:00
|
|
|
func isNotExpr(values map[string]any) (squirrel.Sqlizer, error) {
|
2026-04-24 23:18:20 -04:00
|
|
|
if _, value, info, ok := singleField(values); ok && (info.IsTag || info.IsRole) {
|
|
|
|
|
return jsonExpr(info, squirrel.Eq{"value": value}, true), nil
|
|
|
|
|
}
|
perf(smartplaylist): use annotation index for playcount/rating/loved filters (#5662)
* 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>
2026-06-24 22:26:41 -04:00
|
|
|
return comparisonExpr(values, cmpNe)
|
2026-04-24 23:18:20 -04:00
|
|
|
}
|
|
|
|
|
|
2026-04-28 19:40:08 -04:00
|
|
|
func missingExpr(values map[string]any, checkAbsence bool) (squirrel.Sqlizer, error) {
|
|
|
|
|
field, value, info, ok := singleField(values)
|
|
|
|
|
if !ok {
|
|
|
|
|
if len(values) != 1 {
|
|
|
|
|
return nil, fmt.Errorf("invalid field in criteria: isMissing/isPresent requires exactly one field")
|
|
|
|
|
}
|
|
|
|
|
return nil, fmt.Errorf("invalid field in criteria: %s", field)
|
|
|
|
|
}
|
2026-05-01 19:21:48 -04:00
|
|
|
b, ok := value.(bool)
|
|
|
|
|
if !ok {
|
|
|
|
|
return nil, fmt.Errorf("invalid boolean value for 'missing' expression: %s: %v", field, value)
|
|
|
|
|
}
|
|
|
|
|
negate := checkAbsence == b
|
fix(smartplaylist): support isMissing/isPresent operators on ReplayGain fields (#5585)
* fix(smartplaylist): support isMissing/isPresent operators on ReplayGain fields
ReplayGain values are stored in dedicated nullable columns (rg_album_gain,
rg_album_peak, rg_track_gain, rg_track_peak) rather than in the media_file.tags
JSON blob. The isMissing/isPresent operators previously only supported tag and
role fields, causing two failure modes:
1. Using the documented alias names (replaygain_album_gain etc.) from PR #5256:
these got registered as JSON tags from mappings.yaml, so isMissing queried
json_tree(media_file.tags, '$.replaygain_album_gain') which is always empty
-> the playlist matched ALL songs.
2. Using the canonical field names (rgalbumgain etc.): not a tag/role, so SQL
generation returned an error. Because refreshSmartPlaylist deletes old tracks
before regenerating, the abort left the playlist empty.
Fix: add Nullable bool to FieldInfo and mark the four ReplayGain fields. Add
static alias entries (replaygain_album_gain -> rgalbumgain etc.) with
Numeric+Nullable set; because AddTagNames skips names already in the field map,
these static entries take precedence over the mappings.yaml tag registration.
missingExpr now emits IS NULL / IS NOT NULL for nullable column fields instead
of the json_tree lookup.
Fixes #5584
* chore(smartplaylist): address code review feedback
- Simplify the isMissing/isPresent unsupported-field error message,
removing the internal "nullable fields" jargon
- Standardize comments on mappings.yaml (the actual filename)
- Clarify the alias precedence comment in the LookupField test
2026-06-10 21:12:31 -04:00
|
|
|
|
feat(smartplaylist): extend isMissing/isPresent to bpm, bitDepth and many text fields (#5603)
* feat(smartplaylist): support isMissing/isPresent on mbz_* and lyrics fields
Mark the six mbz_* MusicBrainz ID columns and the lyrics column as Nullable
in the criteria field map, then extend missingExpr to handle string columns
where absence is encoded as NULL or empty string (plus '[]' for lyrics). The
Numeric/Boolean path (ReplayGain) is preserved via an explicit type check.
* refactor(model): make MediaFile BPM and BitDepth nullable pointers
Convert BPM and BitDepth fields in model.MediaFile from int to *int so that
'tag absent' is distinguishable from zero. The metadata mapper now uses
NullableFloat for BPM (nil when absent or zero/unparseable) and only sets
BitDepth when the audio property is non-zero (lossy codecs report 0).
All read sites use gg.V() for zero-fallback deref so Subsonic API output and
transcoding behaviour are byte-identical to before. The persistence layer
bridges the existing NOT NULL DB columns by coercing nil to 0 on write and 0
back to nil on read in PostMapArgs/PostScan; a later migration task will drop
those constraints.
Hash upgrade safety is verified by a new MediaFile.Hash describe block: nil
*int hashes identically to the old int(0) default via ZeroNil+IgnoreZeroValue,
so no files will be spuriously re-imported after this change.
Extra files touched beyond the plan's list: core/stream/legacy_client_test.go
(BitDepth in model.MediaFile literals), persistence/mediafile_repository.go
(NOT NULL bridge).
* test(model): pin pre-conversion golden hashes for BPM/BitDepth
* feat(smartplaylist): support isMissing/isPresent on bpm and bitDepth
* feat(db): make bpm and bit_depth columns nullable, backfill 0 to NULL
Drop the NOT NULL constraint on media_file.bpm and bit_depth via a
lossless migration that converts legacy 0-means-absent values to real
NULL. Remove the temporary shim in PostScan/PostMapArgs that was bridging
the old NOT NULL columns to the *int model fields. Add round-trip
persistence tests asserting NULL storage for nil pointers and correct
value round-trip for non-nil pointers.
* test(e2e): verify isMissing/isPresent partition for nullable fields
Add DescribeTable covering bpm, bitdepth, lyrics, and mbz_recording_id:
for each field, isMissing + isPresent song counts must equal the total
library count, proving the nullable-column SQL is exhaustive and correct.
* test(e2e): seed bpm tag so isMissing/isPresent partition is non-trivial
* fix(model): omit bitDepth from JSON when absent instead of emitting null
* feat(smartplaylist): support isMissing/isPresent on more string fields
Enable isMissing/isPresent operators for album, comment, catalognumber,
discsubtitle, albumcomment, sorttitle, sortalbum, sortartist,
sortalbumartist, and explicitstatus by marking them Nullable in fieldMap.
* refactor(smartplaylist): unify missingExpr column logic into one flow
Collapse the numeric/string fork in missingExpr into a single
empties-driven loop (numeric/boolean fields simply have no empties),
and replace the duplicated IsTag/IsRole guard with a three-way switch
that expresses the dispatch model once. No SQL semantics change for
string fields; numeric/boolean fields now emit a single-element Or/And
which squirrel parenthesizes (e.g. `(col IS NULL)` instead of bare
`col IS NULL`) — update the affected test expectations accordingly.
2026-06-13 13:15:20 -04:00
|
|
|
switch {
|
|
|
|
|
case info.IsTag || info.IsRole:
|
|
|
|
|
return jsonExpr(info, nil, negate), nil
|
|
|
|
|
case info.Nullable:
|
|
|
|
|
// Nullable column fields are stored in dedicated columns, not in the tags JSON, so
|
|
|
|
|
// "missing" maps to a column check rather than a json_tree lookup. Numeric/boolean
|
|
|
|
|
// columns (e.g. ReplayGain, BPM) encode absence as NULL only; string columns (e.g.
|
|
|
|
|
// mbz_* IDs, lyrics) additionally treat empty string — and any field-specific empty
|
|
|
|
|
// encodings (e.g. '[]' for lyrics) — as missing. The unified flow below handles both:
|
|
|
|
|
// numeric/boolean fields simply have no empties, so the loops are no-ops.
|
|
|
|
|
f, ok := smartPlaylistFields[info.Name()]
|
|
|
|
|
if !ok || f.expr == "" {
|
fix(smartplaylist): support isMissing/isPresent operators on ReplayGain fields (#5585)
* fix(smartplaylist): support isMissing/isPresent operators on ReplayGain fields
ReplayGain values are stored in dedicated nullable columns (rg_album_gain,
rg_album_peak, rg_track_gain, rg_track_peak) rather than in the media_file.tags
JSON blob. The isMissing/isPresent operators previously only supported tag and
role fields, causing two failure modes:
1. Using the documented alias names (replaygain_album_gain etc.) from PR #5256:
these got registered as JSON tags from mappings.yaml, so isMissing queried
json_tree(media_file.tags, '$.replaygain_album_gain') which is always empty
-> the playlist matched ALL songs.
2. Using the canonical field names (rgalbumgain etc.): not a tag/role, so SQL
generation returned an error. Because refreshSmartPlaylist deletes old tracks
before regenerating, the abort left the playlist empty.
Fix: add Nullable bool to FieldInfo and mark the four ReplayGain fields. Add
static alias entries (replaygain_album_gain -> rgalbumgain etc.) with
Numeric+Nullable set; because AddTagNames skips names already in the field map,
these static entries take precedence over the mappings.yaml tag registration.
missingExpr now emits IS NULL / IS NOT NULL for nullable column fields instead
of the json_tree lookup.
Fixes #5584
* chore(smartplaylist): address code review feedback
- Simplify the isMissing/isPresent unsupported-field error message,
removing the internal "nullable fields" jargon
- Standardize comments on mappings.yaml (the actual filename)
- Clarify the alias precedence comment in the LookupField test
2026-06-10 21:12:31 -04:00
|
|
|
return nil, fmt.Errorf("invalid field in criteria: %s", field)
|
|
|
|
|
}
|
feat(smartplaylist): extend isMissing/isPresent to bpm, bitDepth and many text fields (#5603)
* feat(smartplaylist): support isMissing/isPresent on mbz_* and lyrics fields
Mark the six mbz_* MusicBrainz ID columns and the lyrics column as Nullable
in the criteria field map, then extend missingExpr to handle string columns
where absence is encoded as NULL or empty string (plus '[]' for lyrics). The
Numeric/Boolean path (ReplayGain) is preserved via an explicit type check.
* refactor(model): make MediaFile BPM and BitDepth nullable pointers
Convert BPM and BitDepth fields in model.MediaFile from int to *int so that
'tag absent' is distinguishable from zero. The metadata mapper now uses
NullableFloat for BPM (nil when absent or zero/unparseable) and only sets
BitDepth when the audio property is non-zero (lossy codecs report 0).
All read sites use gg.V() for zero-fallback deref so Subsonic API output and
transcoding behaviour are byte-identical to before. The persistence layer
bridges the existing NOT NULL DB columns by coercing nil to 0 on write and 0
back to nil on read in PostMapArgs/PostScan; a later migration task will drop
those constraints.
Hash upgrade safety is verified by a new MediaFile.Hash describe block: nil
*int hashes identically to the old int(0) default via ZeroNil+IgnoreZeroValue,
so no files will be spuriously re-imported after this change.
Extra files touched beyond the plan's list: core/stream/legacy_client_test.go
(BitDepth in model.MediaFile literals), persistence/mediafile_repository.go
(NOT NULL bridge).
* test(model): pin pre-conversion golden hashes for BPM/BitDepth
* feat(smartplaylist): support isMissing/isPresent on bpm and bitDepth
* feat(db): make bpm and bit_depth columns nullable, backfill 0 to NULL
Drop the NOT NULL constraint on media_file.bpm and bit_depth via a
lossless migration that converts legacy 0-means-absent values to real
NULL. Remove the temporary shim in PostScan/PostMapArgs that was bridging
the old NOT NULL columns to the *int model fields. Add round-trip
persistence tests asserting NULL storage for nil pointers and correct
value round-trip for non-nil pointers.
* test(e2e): verify isMissing/isPresent partition for nullable fields
Add DescribeTable covering bpm, bitdepth, lyrics, and mbz_recording_id:
for each field, isMissing + isPresent song counts must equal the total
library count, proving the nullable-column SQL is exhaustive and correct.
* test(e2e): seed bpm tag so isMissing/isPresent partition is non-trivial
* fix(model): omit bitDepth from JSON when absent instead of emitting null
* feat(smartplaylist): support isMissing/isPresent on more string fields
Enable isMissing/isPresent operators for album, comment, catalognumber,
discsubtitle, albumcomment, sorttitle, sortalbum, sortartist,
sortalbumartist, and explicitstatus by marking them Nullable in fieldMap.
* refactor(smartplaylist): unify missingExpr column logic into one flow
Collapse the numeric/string fork in missingExpr into a single
empties-driven loop (numeric/boolean fields simply have no empties),
and replace the duplicated IsTag/IsRole guard with a three-way switch
that expresses the dispatch model once. No SQL semantics change for
string fields; numeric/boolean fields now emit a single-element Or/And
which squirrel parenthesizes (e.g. `(col IS NULL)` instead of bare
`col IS NULL`) — update the affected test expectations accordingly.
2026-06-13 13:15:20 -04:00
|
|
|
col := f.expr
|
|
|
|
|
var empties []string
|
|
|
|
|
if !info.Numeric && !info.Boolean {
|
|
|
|
|
empties = append([]string{""}, f.emptyValues...)
|
|
|
|
|
}
|
|
|
|
|
missing := squirrel.Or{squirrel.Eq{col: nil}}
|
|
|
|
|
present := squirrel.And{squirrel.NotEq{col: nil}}
|
|
|
|
|
for _, e := range empties {
|
|
|
|
|
missing = append(missing, squirrel.Eq{col: e})
|
|
|
|
|
present = append(present, squirrel.NotEq{col: e})
|
|
|
|
|
}
|
fix(smartplaylist): support isMissing/isPresent operators on ReplayGain fields (#5585)
* fix(smartplaylist): support isMissing/isPresent operators on ReplayGain fields
ReplayGain values are stored in dedicated nullable columns (rg_album_gain,
rg_album_peak, rg_track_gain, rg_track_peak) rather than in the media_file.tags
JSON blob. The isMissing/isPresent operators previously only supported tag and
role fields, causing two failure modes:
1. Using the documented alias names (replaygain_album_gain etc.) from PR #5256:
these got registered as JSON tags from mappings.yaml, so isMissing queried
json_tree(media_file.tags, '$.replaygain_album_gain') which is always empty
-> the playlist matched ALL songs.
2. Using the canonical field names (rgalbumgain etc.): not a tag/role, so SQL
generation returned an error. Because refreshSmartPlaylist deletes old tracks
before regenerating, the abort left the playlist empty.
Fix: add Nullable bool to FieldInfo and mark the four ReplayGain fields. Add
static alias entries (replaygain_album_gain -> rgalbumgain etc.) with
Numeric+Nullable set; because AddTagNames skips names already in the field map,
these static entries take precedence over the mappings.yaml tag registration.
missingExpr now emits IS NULL / IS NOT NULL for nullable column fields instead
of the json_tree lookup.
Fixes #5584
* chore(smartplaylist): address code review feedback
- Simplify the isMissing/isPresent unsupported-field error message,
removing the internal "nullable fields" jargon
- Standardize comments on mappings.yaml (the actual filename)
- Clarify the alias precedence comment in the LookupField test
2026-06-10 21:12:31 -04:00
|
|
|
if negate {
|
feat(smartplaylist): extend isMissing/isPresent to bpm, bitDepth and many text fields (#5603)
* feat(smartplaylist): support isMissing/isPresent on mbz_* and lyrics fields
Mark the six mbz_* MusicBrainz ID columns and the lyrics column as Nullable
in the criteria field map, then extend missingExpr to handle string columns
where absence is encoded as NULL or empty string (plus '[]' for lyrics). The
Numeric/Boolean path (ReplayGain) is preserved via an explicit type check.
* refactor(model): make MediaFile BPM and BitDepth nullable pointers
Convert BPM and BitDepth fields in model.MediaFile from int to *int so that
'tag absent' is distinguishable from zero. The metadata mapper now uses
NullableFloat for BPM (nil when absent or zero/unparseable) and only sets
BitDepth when the audio property is non-zero (lossy codecs report 0).
All read sites use gg.V() for zero-fallback deref so Subsonic API output and
transcoding behaviour are byte-identical to before. The persistence layer
bridges the existing NOT NULL DB columns by coercing nil to 0 on write and 0
back to nil on read in PostMapArgs/PostScan; a later migration task will drop
those constraints.
Hash upgrade safety is verified by a new MediaFile.Hash describe block: nil
*int hashes identically to the old int(0) default via ZeroNil+IgnoreZeroValue,
so no files will be spuriously re-imported after this change.
Extra files touched beyond the plan's list: core/stream/legacy_client_test.go
(BitDepth in model.MediaFile literals), persistence/mediafile_repository.go
(NOT NULL bridge).
* test(model): pin pre-conversion golden hashes for BPM/BitDepth
* feat(smartplaylist): support isMissing/isPresent on bpm and bitDepth
* feat(db): make bpm and bit_depth columns nullable, backfill 0 to NULL
Drop the NOT NULL constraint on media_file.bpm and bit_depth via a
lossless migration that converts legacy 0-means-absent values to real
NULL. Remove the temporary shim in PostScan/PostMapArgs that was bridging
the old NOT NULL columns to the *int model fields. Add round-trip
persistence tests asserting NULL storage for nil pointers and correct
value round-trip for non-nil pointers.
* test(e2e): verify isMissing/isPresent partition for nullable fields
Add DescribeTable covering bpm, bitdepth, lyrics, and mbz_recording_id:
for each field, isMissing + isPresent song counts must equal the total
library count, proving the nullable-column SQL is exhaustive and correct.
* test(e2e): seed bpm tag so isMissing/isPresent partition is non-trivial
* fix(model): omit bitDepth from JSON when absent instead of emitting null
* feat(smartplaylist): support isMissing/isPresent on more string fields
Enable isMissing/isPresent operators for album, comment, catalognumber,
discsubtitle, albumcomment, sorttitle, sortalbum, sortartist,
sortalbumartist, and explicitstatus by marking them Nullable in fieldMap.
* refactor(smartplaylist): unify missingExpr column logic into one flow
Collapse the numeric/string fork in missingExpr into a single
empties-driven loop (numeric/boolean fields simply have no empties),
and replace the duplicated IsTag/IsRole guard with a three-way switch
that expresses the dispatch model once. No SQL semantics change for
string fields; numeric/boolean fields now emit a single-element Or/And
which squirrel parenthesizes (e.g. `(col IS NULL)` instead of bare
`col IS NULL`) — update the affected test expectations accordingly.
2026-06-13 13:15:20 -04:00
|
|
|
return missing, nil
|
fix(smartplaylist): support isMissing/isPresent operators on ReplayGain fields (#5585)
* fix(smartplaylist): support isMissing/isPresent operators on ReplayGain fields
ReplayGain values are stored in dedicated nullable columns (rg_album_gain,
rg_album_peak, rg_track_gain, rg_track_peak) rather than in the media_file.tags
JSON blob. The isMissing/isPresent operators previously only supported tag and
role fields, causing two failure modes:
1. Using the documented alias names (replaygain_album_gain etc.) from PR #5256:
these got registered as JSON tags from mappings.yaml, so isMissing queried
json_tree(media_file.tags, '$.replaygain_album_gain') which is always empty
-> the playlist matched ALL songs.
2. Using the canonical field names (rgalbumgain etc.): not a tag/role, so SQL
generation returned an error. Because refreshSmartPlaylist deletes old tracks
before regenerating, the abort left the playlist empty.
Fix: add Nullable bool to FieldInfo and mark the four ReplayGain fields. Add
static alias entries (replaygain_album_gain -> rgalbumgain etc.) with
Numeric+Nullable set; because AddTagNames skips names already in the field map,
these static entries take precedence over the mappings.yaml tag registration.
missingExpr now emits IS NULL / IS NOT NULL for nullable column fields instead
of the json_tree lookup.
Fixes #5584
* chore(smartplaylist): address code review feedback
- Simplify the isMissing/isPresent unsupported-field error message,
removing the internal "nullable fields" jargon
- Standardize comments on mappings.yaml (the actual filename)
- Clarify the alias precedence comment in the LookupField test
2026-06-10 21:12:31 -04:00
|
|
|
}
|
feat(smartplaylist): extend isMissing/isPresent to bpm, bitDepth and many text fields (#5603)
* feat(smartplaylist): support isMissing/isPresent on mbz_* and lyrics fields
Mark the six mbz_* MusicBrainz ID columns and the lyrics column as Nullable
in the criteria field map, then extend missingExpr to handle string columns
where absence is encoded as NULL or empty string (plus '[]' for lyrics). The
Numeric/Boolean path (ReplayGain) is preserved via an explicit type check.
* refactor(model): make MediaFile BPM and BitDepth nullable pointers
Convert BPM and BitDepth fields in model.MediaFile from int to *int so that
'tag absent' is distinguishable from zero. The metadata mapper now uses
NullableFloat for BPM (nil when absent or zero/unparseable) and only sets
BitDepth when the audio property is non-zero (lossy codecs report 0).
All read sites use gg.V() for zero-fallback deref so Subsonic API output and
transcoding behaviour are byte-identical to before. The persistence layer
bridges the existing NOT NULL DB columns by coercing nil to 0 on write and 0
back to nil on read in PostMapArgs/PostScan; a later migration task will drop
those constraints.
Hash upgrade safety is verified by a new MediaFile.Hash describe block: nil
*int hashes identically to the old int(0) default via ZeroNil+IgnoreZeroValue,
so no files will be spuriously re-imported after this change.
Extra files touched beyond the plan's list: core/stream/legacy_client_test.go
(BitDepth in model.MediaFile literals), persistence/mediafile_repository.go
(NOT NULL bridge).
* test(model): pin pre-conversion golden hashes for BPM/BitDepth
* feat(smartplaylist): support isMissing/isPresent on bpm and bitDepth
* feat(db): make bpm and bit_depth columns nullable, backfill 0 to NULL
Drop the NOT NULL constraint on media_file.bpm and bit_depth via a
lossless migration that converts legacy 0-means-absent values to real
NULL. Remove the temporary shim in PostScan/PostMapArgs that was bridging
the old NOT NULL columns to the *int model fields. Add round-trip
persistence tests asserting NULL storage for nil pointers and correct
value round-trip for non-nil pointers.
* test(e2e): verify isMissing/isPresent partition for nullable fields
Add DescribeTable covering bpm, bitdepth, lyrics, and mbz_recording_id:
for each field, isMissing + isPresent song counts must equal the total
library count, proving the nullable-column SQL is exhaustive and correct.
* test(e2e): seed bpm tag so isMissing/isPresent partition is non-trivial
* fix(model): omit bitDepth from JSON when absent instead of emitting null
* feat(smartplaylist): support isMissing/isPresent on more string fields
Enable isMissing/isPresent operators for album, comment, catalognumber,
discsubtitle, albumcomment, sorttitle, sortalbum, sortartist,
sortalbumartist, and explicitstatus by marking them Nullable in fieldMap.
* refactor(smartplaylist): unify missingExpr column logic into one flow
Collapse the numeric/string fork in missingExpr into a single
empties-driven loop (numeric/boolean fields simply have no empties),
and replace the duplicated IsTag/IsRole guard with a three-way switch
that expresses the dispatch model once. No SQL semantics change for
string fields; numeric/boolean fields now emit a single-element Or/And
which squirrel parenthesizes (e.g. `(col IS NULL)` instead of bare
`col IS NULL`) — update the affected test expectations accordingly.
2026-06-13 13:15:20 -04:00
|
|
|
return present, nil
|
|
|
|
|
default:
|
|
|
|
|
return nil, fmt.Errorf("isMissing/isPresent operator is not supported for field: %s", field)
|
fix(smartplaylist): support isMissing/isPresent operators on ReplayGain fields (#5585)
* fix(smartplaylist): support isMissing/isPresent operators on ReplayGain fields
ReplayGain values are stored in dedicated nullable columns (rg_album_gain,
rg_album_peak, rg_track_gain, rg_track_peak) rather than in the media_file.tags
JSON blob. The isMissing/isPresent operators previously only supported tag and
role fields, causing two failure modes:
1. Using the documented alias names (replaygain_album_gain etc.) from PR #5256:
these got registered as JSON tags from mappings.yaml, so isMissing queried
json_tree(media_file.tags, '$.replaygain_album_gain') which is always empty
-> the playlist matched ALL songs.
2. Using the canonical field names (rgalbumgain etc.): not a tag/role, so SQL
generation returned an error. Because refreshSmartPlaylist deletes old tracks
before regenerating, the abort left the playlist empty.
Fix: add Nullable bool to FieldInfo and mark the four ReplayGain fields. Add
static alias entries (replaygain_album_gain -> rgalbumgain etc.) with
Numeric+Nullable set; because AddTagNames skips names already in the field map,
these static entries take precedence over the mappings.yaml tag registration.
missingExpr now emits IS NULL / IS NOT NULL for nullable column fields instead
of the json_tree lookup.
Fixes #5584
* chore(smartplaylist): address code review feedback
- Simplify the isMissing/isPresent unsupported-field error message,
removing the internal "nullable fields" jargon
- Standardize comments on mappings.yaml (the actual filename)
- Clarify the alias precedence comment in the LookupField test
2026-06-10 21:12:31 -04:00
|
|
|
}
|
2026-04-28 19:40:08 -04:00
|
|
|
}
|
|
|
|
|
|
2026-04-25 14:59:06 -04:00
|
|
|
func likeExpr(values map[string]any, pattern string, negate bool) (squirrel.Sqlizer, error) {
|
perf(smartplaylist): use annotation index for playcount/rating/loved filters (#5662)
* 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>
2026-06-24 22:26:41 -04:00
|
|
|
if _, value, info, ok := singleField(values); ok {
|
|
|
|
|
if info.IsTag || info.IsRole {
|
|
|
|
|
return jsonExpr(info, squirrel.Like{"value": fmt.Sprintf(pattern, value)}, negate), nil
|
|
|
|
|
}
|
|
|
|
|
// LIKE can't use the column index, so annotation fields keep the COALESCE form: a NULL
|
|
|
|
|
// column never matches LIKE, which would silently drop missing-annotation rows the original
|
|
|
|
|
// COALESCE form included.
|
|
|
|
|
if f, isAnnotation := annotationField(info); isAnnotation {
|
|
|
|
|
return likeCond(f.coalesced(), fmt.Sprintf(pattern, value), negate), nil
|
|
|
|
|
}
|
2026-04-24 23:18:20 -04:00
|
|
|
}
|
|
|
|
|
fields, err := sqlFields(values)
|
|
|
|
|
if err != nil {
|
|
|
|
|
return nil, err
|
|
|
|
|
}
|
|
|
|
|
if negate {
|
|
|
|
|
lk := squirrel.NotLike{}
|
|
|
|
|
for field, value := range fields {
|
|
|
|
|
lk[field] = fmt.Sprintf(pattern, value)
|
|
|
|
|
}
|
|
|
|
|
return lk, nil
|
|
|
|
|
}
|
|
|
|
|
lk := squirrel.Like{}
|
|
|
|
|
for field, value := range fields {
|
|
|
|
|
lk[field] = fmt.Sprintf(pattern, value)
|
|
|
|
|
}
|
|
|
|
|
return lk, nil
|
|
|
|
|
}
|
|
|
|
|
|
perf(smartplaylist): use annotation index for playcount/rating/loved filters (#5662)
* 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>
2026-06-24 22:26:41 -04:00
|
|
|
func likeCond(col, pattern string, negate bool) squirrel.Sqlizer {
|
|
|
|
|
if negate {
|
|
|
|
|
return squirrel.NotLike{col: pattern}
|
|
|
|
|
}
|
|
|
|
|
return squirrel.Like{col: pattern}
|
|
|
|
|
}
|
|
|
|
|
|
2026-04-25 14:59:06 -04:00
|
|
|
func rangeExpr(values map[string]any) (squirrel.Sqlizer, error) {
|
perf(smartplaylist): use annotation index for playcount/rating/loved filters (#5662)
* 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>
2026-06-24 22:26:41 -04:00
|
|
|
field, value, info, ok := singleField(values)
|
|
|
|
|
if !ok {
|
|
|
|
|
return nil, fmt.Errorf("invalid field in criteria: %s", field)
|
|
|
|
|
}
|
|
|
|
|
if info.IsTag || info.IsRole {
|
|
|
|
|
// Tags/roles are multi-valued JSON, so splitting a range into two independent EXISTS
|
|
|
|
|
// subqueries would let different values satisfy each bound. Ranges are unsupported there.
|
|
|
|
|
return nil, fmt.Errorf("range operator not supported for tag/role field: %s", field)
|
|
|
|
|
}
|
|
|
|
|
s := reflect.ValueOf(value)
|
|
|
|
|
if s.Kind() != reflect.Slice || s.Len() != 2 {
|
|
|
|
|
return nil, fmt.Errorf("range criteria for %q must be a [min, max] pair, got: %v", field, value)
|
|
|
|
|
}
|
|
|
|
|
low, err := comparisonExpr(map[string]any{field: s.Index(0).Interface()}, cmpGe)
|
2026-04-24 23:18:20 -04:00
|
|
|
if err != nil {
|
|
|
|
|
return nil, err
|
|
|
|
|
}
|
perf(smartplaylist): use annotation index for playcount/rating/loved filters (#5662)
* 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>
2026-06-24 22:26:41 -04:00
|
|
|
high, err := comparisonExpr(map[string]any{field: s.Index(1).Interface()}, cmpLe)
|
|
|
|
|
if err != nil {
|
|
|
|
|
return nil, err
|
2026-04-24 23:18:20 -04:00
|
|
|
}
|
perf(smartplaylist): use annotation index for playcount/rating/loved filters (#5662)
* 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>
2026-06-24 22:26:41 -04:00
|
|
|
return squirrel.And{low, high}, nil
|
2026-04-24 23:18:20 -04:00
|
|
|
}
|
|
|
|
|
|
2026-04-25 14:59:06 -04:00
|
|
|
func periodExpr(values map[string]any, negate bool) (squirrel.Sqlizer, error) {
|
2026-04-24 23:18:20 -04:00
|
|
|
fields, err := sqlFields(values)
|
|
|
|
|
if err != nil {
|
|
|
|
|
return nil, err
|
|
|
|
|
}
|
|
|
|
|
var field string
|
|
|
|
|
var value any
|
|
|
|
|
for f, v := range fields {
|
|
|
|
|
field, value = f, v
|
|
|
|
|
break
|
|
|
|
|
}
|
|
|
|
|
days, err := strconv.ParseInt(fmt.Sprintf("%v", value), 10, 64)
|
|
|
|
|
if err != nil {
|
|
|
|
|
return nil, err
|
|
|
|
|
}
|
|
|
|
|
firstDate := startOfPeriod(days, time.Now())
|
|
|
|
|
if negate {
|
|
|
|
|
return squirrel.Or{
|
|
|
|
|
squirrel.Lt{field: firstDate},
|
|
|
|
|
squirrel.Eq{field: nil},
|
|
|
|
|
}, nil
|
|
|
|
|
}
|
|
|
|
|
return squirrel.Gt{field: firstDate}, nil
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
func startOfPeriod(numDays int64, from time.Time) string {
|
|
|
|
|
return from.Add(time.Duration(-24*numDays) * time.Hour).Format("2006-01-02")
|
|
|
|
|
}
|
|
|
|
|
|
2026-04-25 14:59:06 -04:00
|
|
|
func (c smartPlaylistCriteria) inList(values map[string]any, negate bool) (squirrel.Sqlizer, error) {
|
feat(smartplaylists): add support for referencing playlists using paths (#5187)
* feat: Add support for referencing playlists using paths
Signed-off-by: David <dvedvick@gmail.com>
* feat: Support relative playlist paths in smartlists
Signed-off-by: David <dvedvick@gmail.com>
* fix(smartplaylists): protect against nil panic
Signed-off-by: David <dvedvick@gmail.com>
* fix(smartplaylists): refreshing child playlists
Signed-off-by: David <dvedvick@gmail.com>
* chore(smartplaylists): log field parsing error
Signed-off-by: David <dvedvick@gmail.com>
* fix(smartplaylists): handle empty playlist paths
Signed-off-by: David <dvedvick@gmail.com>
* refactor(smartplaylists): make NormalizeChildPaths non-mutating
Signed-off-by: David <dvedvick@gmail.com>
* fix(smartplaylists): stop warning on every inPlaylist rule without the looked-up field
Rules that reference a playlist by id have no path field, and the reverse, so
the warning fired on every refresh. The log call also had a bad argument count.
* fix(smartplaylists): ignore empty inPlaylist id and path references
An empty path matched every playlist without a file path, including the
referencing playlist itself, so the refresh recursed until the stack overflowed.
An empty id also shadowed a valid path in the same rule.
* fix(smartplaylists): match inPlaylist paths in both NFC and NFD forms
A playlist path is stored in the Unicode form the filesystem reports, which can
differ from the form typed in the .nsp file. The exact comparison then found no
playlist for names with accents.
* fix(smartplaylists): keep all criteria fields when normalizing child paths
The field-by-field copy dropped RefreshDelay.
* fix(smartplaylists): clean absolute inPlaylist path references
Only relative references were cleaned, so an absolute reference such as
/music/./child.nsp never matched the stored /music/child.nsp.
* fix(smartplaylists): stop infinite recursion on playlists that reference each other
Two smart playlists referencing each other, by id or by path, recursed until the
stack overflowed and the server died. The refresh now tracks visited playlists.
* fix(smartplaylists): resolve inPlaylist path references with OS-native separators
Playlist.Path is OS-native, but references in a .nsp file use forward slashes.
On Windows they never matched, and a leading slash was not seen as absolute.
The specs now build OS-native paths, so they also run on Windows.
* fix(smartplaylists): warn when a relative inPlaylist path cannot be resolved
A playlist created in the UI has no file path, so a relative reference silently
matched nothing.
* refactor(smartplaylists): simplify child playlist reference handling
Share one extractor for child ids and paths, return only the normalized rules
instead of a playlist copy, and resolve each path reference in a single switch.
* test(smartplaylists): store the Unicode child path in OS-native form
Playlist.Path is OS-native, so on Windows the forward-slash fixture never matched
the normalized reference.
---------
Signed-off-by: David <dvedvick@gmail.com>
Co-authored-by: Deluan Quintão <deluan@navidrome.org>
2026-09-19 20:05:58 -05:00
|
|
|
var condition squirrel.Sqlizer
|
|
|
|
|
if playlistId, ok := values["id"].(string); ok && playlistId != "" {
|
|
|
|
|
condition = squirrel.Eq{"pl.playlist_id": playlistId}
|
|
|
|
|
} else if playlistPath, ok := values["path"].(string); ok && playlistPath != "" {
|
|
|
|
|
condition = squirrel.Eq{"playlist.path": pathVariants(playlistPath)}
|
|
|
|
|
} else {
|
|
|
|
|
return nil, errors.New("playlist id or path not given")
|
2026-04-24 23:18:20 -04:00
|
|
|
}
|
feat(smartplaylists): add support for referencing playlists using paths (#5187)
* feat: Add support for referencing playlists using paths
Signed-off-by: David <dvedvick@gmail.com>
* feat: Support relative playlist paths in smartlists
Signed-off-by: David <dvedvick@gmail.com>
* fix(smartplaylists): protect against nil panic
Signed-off-by: David <dvedvick@gmail.com>
* fix(smartplaylists): refreshing child playlists
Signed-off-by: David <dvedvick@gmail.com>
* chore(smartplaylists): log field parsing error
Signed-off-by: David <dvedvick@gmail.com>
* fix(smartplaylists): handle empty playlist paths
Signed-off-by: David <dvedvick@gmail.com>
* refactor(smartplaylists): make NormalizeChildPaths non-mutating
Signed-off-by: David <dvedvick@gmail.com>
* fix(smartplaylists): stop warning on every inPlaylist rule without the looked-up field
Rules that reference a playlist by id have no path field, and the reverse, so
the warning fired on every refresh. The log call also had a bad argument count.
* fix(smartplaylists): ignore empty inPlaylist id and path references
An empty path matched every playlist without a file path, including the
referencing playlist itself, so the refresh recursed until the stack overflowed.
An empty id also shadowed a valid path in the same rule.
* fix(smartplaylists): match inPlaylist paths in both NFC and NFD forms
A playlist path is stored in the Unicode form the filesystem reports, which can
differ from the form typed in the .nsp file. The exact comparison then found no
playlist for names with accents.
* fix(smartplaylists): keep all criteria fields when normalizing child paths
The field-by-field copy dropped RefreshDelay.
* fix(smartplaylists): clean absolute inPlaylist path references
Only relative references were cleaned, so an absolute reference such as
/music/./child.nsp never matched the stored /music/child.nsp.
* fix(smartplaylists): stop infinite recursion on playlists that reference each other
Two smart playlists referencing each other, by id or by path, recursed until the
stack overflowed and the server died. The refresh now tracks visited playlists.
* fix(smartplaylists): resolve inPlaylist path references with OS-native separators
Playlist.Path is OS-native, but references in a .nsp file use forward slashes.
On Windows they never matched, and a leading slash was not seen as absolute.
The specs now build OS-native paths, so they also run on Windows.
* fix(smartplaylists): warn when a relative inPlaylist path cannot be resolved
A playlist created in the UI has no file path, so a relative reference silently
matched nothing.
* refactor(smartplaylists): simplify child playlist reference handling
Share one extractor for child ids and paths, return only the normalized rules
instead of a playlist copy, and resolve each path reference in a single switch.
* test(smartplaylists): store the Unicode child path in OS-native form
Playlist.Path is OS-native, so on Windows the forward-slash fixture never matched
the normalized reference.
---------
Signed-off-by: David <dvedvick@gmail.com>
Co-authored-by: Deluan Quintão <deluan@navidrome.org>
2026-09-19 20:05:58 -05:00
|
|
|
filters := squirrel.And{condition}
|
refactor: centralize criteria sort parsing and extract smart playlist logic (#5415)
* test: add tests for recordingdate alias resolution in smart playlists
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: update FieldInfo structure and simplify fieldMap initialization
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: move sort parsing logic from persistence to criteria package
Extracted sort field parsing, validation, and direction handling from
persistence/criteria_sql.go into model/criteria/sort.go. The new
OrderByFields method on Criteria parses the Sort/Order strings into
validated SortField structs (field name + direction), resolving aliases
and handling +/- prefixes and order inversion. The persistence layer now
consumes these parsed fields and only handles SQL expression mapping.
This centralizes sort parsing to enforce consistent implementations.
* refactor: standardize field access in smartPlaylistCriteria structure
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: add ResolveLimit method to Criteria
Moved the percentage-limit resolution logic from playlist_repository
into Criteria.ResolveLimit, replacing the 3-line mutate-after-query
pattern with a single method call. The method preserves LimitPercent
rather than zeroing it, since IsPercentageLimit already returns false
once Limit is set, making the clear redundant and lossy.
* refactor: improve child playlist loading and error handling in refresh logic
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: extract smart playlist logic to dedicated files
Moved refreshSmartPlaylist, addSmartPlaylistAnnotationJoins, and
addCriteria methods from playlist_repository.go to a new
smart_playlist_repository.go file. Extracted all smart playlist tests
to smart_playlist_repository_test.go. Added DeferCleanup to the
"valid rules" test to fix ordering flakiness when Ginkgo randomizes
test execution across files.
* refactor: break refreshSmartPlaylist into smaller focused methods
Split the monolithic refreshSmartPlaylist method into discrete helpers
for readability: shouldRefreshSmartPlaylist for guard checks,
refreshChildPlaylists for recursive dependency refresh,
resolvePercentageLimit for count-based limit resolution,
buildSmartPlaylistQuery for assembling the SELECT with joins, and
addMediaFileAnnotationJoin to DRY up the repeated annotation join clause.
* refactor: deduplicate child playlist IDs in Criteria
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: simplify withSmartPlaylistOwner to accept model.User
Replaced separate ownerID string and ownerIsAdmin bool parameters with a
single model.User struct, reducing the field count in smartPlaylistCriteria
and making the option function signature clearer. Updated all call sites
and tests accordingly.
* fix: handle empty sort fields and propagate child playlist load errors
OrderByFields now falls back to [{title, asc}] when all user-supplied
sort fields are invalid, preventing empty ORDER BY clauses that would
produce invalid SQL in row_number() window functions. Also restored the
original behavior where a DB error loading child playlists aborts the
parent smart playlist refresh, by making refreshChildPlaylists return a
bool.
* refactor: log warning when no valid sort fields are found
Signed-off-by: Deluan <deluan@navidrome.org>
---------
Signed-off-by: Deluan <deluan@navidrome.org>
2026-04-26 14:49:59 -04:00
|
|
|
if !c.owner.IsAdmin {
|
|
|
|
|
if c.owner.ID == "" {
|
2026-04-25 14:59:06 -04:00
|
|
|
filters = append(filters, squirrel.Eq{"playlist.public": 1})
|
|
|
|
|
} else {
|
|
|
|
|
filters = append(filters, squirrel.Or{
|
|
|
|
|
squirrel.Eq{"playlist.public": 1},
|
refactor: centralize criteria sort parsing and extract smart playlist logic (#5415)
* test: add tests for recordingdate alias resolution in smart playlists
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: update FieldInfo structure and simplify fieldMap initialization
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: move sort parsing logic from persistence to criteria package
Extracted sort field parsing, validation, and direction handling from
persistence/criteria_sql.go into model/criteria/sort.go. The new
OrderByFields method on Criteria parses the Sort/Order strings into
validated SortField structs (field name + direction), resolving aliases
and handling +/- prefixes and order inversion. The persistence layer now
consumes these parsed fields and only handles SQL expression mapping.
This centralizes sort parsing to enforce consistent implementations.
* refactor: standardize field access in smartPlaylistCriteria structure
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: add ResolveLimit method to Criteria
Moved the percentage-limit resolution logic from playlist_repository
into Criteria.ResolveLimit, replacing the 3-line mutate-after-query
pattern with a single method call. The method preserves LimitPercent
rather than zeroing it, since IsPercentageLimit already returns false
once Limit is set, making the clear redundant and lossy.
* refactor: improve child playlist loading and error handling in refresh logic
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: extract smart playlist logic to dedicated files
Moved refreshSmartPlaylist, addSmartPlaylistAnnotationJoins, and
addCriteria methods from playlist_repository.go to a new
smart_playlist_repository.go file. Extracted all smart playlist tests
to smart_playlist_repository_test.go. Added DeferCleanup to the
"valid rules" test to fix ordering flakiness when Ginkgo randomizes
test execution across files.
* refactor: break refreshSmartPlaylist into smaller focused methods
Split the monolithic refreshSmartPlaylist method into discrete helpers
for readability: shouldRefreshSmartPlaylist for guard checks,
refreshChildPlaylists for recursive dependency refresh,
resolvePercentageLimit for count-based limit resolution,
buildSmartPlaylistQuery for assembling the SELECT with joins, and
addMediaFileAnnotationJoin to DRY up the repeated annotation join clause.
* refactor: deduplicate child playlist IDs in Criteria
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: simplify withSmartPlaylistOwner to accept model.User
Replaced separate ownerID string and ownerIsAdmin bool parameters with a
single model.User struct, reducing the field count in smartPlaylistCriteria
and making the option function signature clearer. Updated all call sites
and tests accordingly.
* fix: handle empty sort fields and propagate child playlist load errors
OrderByFields now falls back to [{title, asc}] when all user-supplied
sort fields are invalid, preventing empty ORDER BY clauses that would
produce invalid SQL in row_number() window functions. Also restored the
original behavior where a DB error loading child playlists aborts the
parent smart playlist refresh, by making refreshChildPlaylists return a
bool.
* refactor: log warning when no valid sort fields are found
Signed-off-by: Deluan <deluan@navidrome.org>
---------
Signed-off-by: Deluan <deluan@navidrome.org>
2026-04-26 14:49:59 -04:00
|
|
|
squirrel.Eq{"playlist.owner_id": c.owner.ID},
|
2026-04-25 14:59:06 -04:00
|
|
|
})
|
|
|
|
|
}
|
|
|
|
|
}
|
2026-04-24 23:18:20 -04:00
|
|
|
subQuery := squirrel.Select("media_file_id").
|
|
|
|
|
From("playlist_tracks pl").
|
|
|
|
|
LeftJoin("playlist on pl.playlist_id = playlist.id").
|
2026-04-25 14:59:06 -04:00
|
|
|
Where(filters)
|
2026-04-24 23:18:20 -04:00
|
|
|
subSQL, subArgs, err := subQuery.PlaceholderFormat(squirrel.Question).ToSql()
|
|
|
|
|
if err != nil {
|
|
|
|
|
return nil, err
|
|
|
|
|
}
|
|
|
|
|
if negate {
|
|
|
|
|
return squirrel.Expr("media_file.id NOT IN ("+subSQL+")", subArgs...), nil
|
|
|
|
|
}
|
|
|
|
|
return squirrel.Expr("media_file.id IN ("+subSQL+")", subArgs...), nil
|
|
|
|
|
}
|
|
|
|
|
|
feat(smartplaylists): add support for referencing playlists using paths (#5187)
* feat: Add support for referencing playlists using paths
Signed-off-by: David <dvedvick@gmail.com>
* feat: Support relative playlist paths in smartlists
Signed-off-by: David <dvedvick@gmail.com>
* fix(smartplaylists): protect against nil panic
Signed-off-by: David <dvedvick@gmail.com>
* fix(smartplaylists): refreshing child playlists
Signed-off-by: David <dvedvick@gmail.com>
* chore(smartplaylists): log field parsing error
Signed-off-by: David <dvedvick@gmail.com>
* fix(smartplaylists): handle empty playlist paths
Signed-off-by: David <dvedvick@gmail.com>
* refactor(smartplaylists): make NormalizeChildPaths non-mutating
Signed-off-by: David <dvedvick@gmail.com>
* fix(smartplaylists): stop warning on every inPlaylist rule without the looked-up field
Rules that reference a playlist by id have no path field, and the reverse, so
the warning fired on every refresh. The log call also had a bad argument count.
* fix(smartplaylists): ignore empty inPlaylist id and path references
An empty path matched every playlist without a file path, including the
referencing playlist itself, so the refresh recursed until the stack overflowed.
An empty id also shadowed a valid path in the same rule.
* fix(smartplaylists): match inPlaylist paths in both NFC and NFD forms
A playlist path is stored in the Unicode form the filesystem reports, which can
differ from the form typed in the .nsp file. The exact comparison then found no
playlist for names with accents.
* fix(smartplaylists): keep all criteria fields when normalizing child paths
The field-by-field copy dropped RefreshDelay.
* fix(smartplaylists): clean absolute inPlaylist path references
Only relative references were cleaned, so an absolute reference such as
/music/./child.nsp never matched the stored /music/child.nsp.
* fix(smartplaylists): stop infinite recursion on playlists that reference each other
Two smart playlists referencing each other, by id or by path, recursed until the
stack overflowed and the server died. The refresh now tracks visited playlists.
* fix(smartplaylists): resolve inPlaylist path references with OS-native separators
Playlist.Path is OS-native, but references in a .nsp file use forward slashes.
On Windows they never matched, and a leading slash was not seen as absolute.
The specs now build OS-native paths, so they also run on Windows.
* fix(smartplaylists): warn when a relative inPlaylist path cannot be resolved
A playlist created in the UI has no file path, so a relative reference silently
matched nothing.
* refactor(smartplaylists): simplify child playlist reference handling
Share one extractor for child ids and paths, return only the normalized rules
instead of a playlist copy, and resolve each path reference in a single switch.
* test(smartplaylists): store the Unicode child path in OS-native form
Playlist.Path is OS-native, so on Windows the forward-slash fixture never matched
the normalized reference.
---------
Signed-off-by: David <dvedvick@gmail.com>
Co-authored-by: Deluan Quintão <deluan@navidrome.org>
2026-09-19 20:05:58 -05:00
|
|
|
// Filesystems disagree on the Unicode form of a name, so match the path in NFC and NFD.
|
|
|
|
|
func pathVariants(path string) []string {
|
|
|
|
|
variants := []string{path}
|
|
|
|
|
if alt := norm.NFC.String(path); alt != path {
|
|
|
|
|
variants = append(variants, alt)
|
|
|
|
|
}
|
|
|
|
|
if alt := norm.NFD.String(path); alt != path {
|
|
|
|
|
variants = append(variants, alt)
|
|
|
|
|
}
|
|
|
|
|
return variants
|
|
|
|
|
}
|
|
|
|
|
|
2026-04-24 23:18:20 -04:00
|
|
|
func jsonExpr(info criteria.FieldInfo, cond squirrel.Sqlizer, negate bool) squirrel.Sqlizer {
|
|
|
|
|
if info.IsRole {
|
2026-04-28 20:22:48 -04:00
|
|
|
return roleCond{role: info.Name(), cond: cond, not: negate}
|
2026-04-24 23:18:20 -04:00
|
|
|
}
|
2026-04-28 20:22:48 -04:00
|
|
|
return tagCond{tag: info.Name(), numeric: info.Numeric, cond: cond, not: negate}
|
2026-04-24 23:18:20 -04:00
|
|
|
}
|
|
|
|
|
|
|
|
|
|
type tagCond struct {
|
|
|
|
|
tag string
|
|
|
|
|
numeric bool
|
|
|
|
|
cond squirrel.Sqlizer
|
|
|
|
|
not bool
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
func (e tagCond) ToSql() (string, []any, error) {
|
2026-04-28 19:40:08 -04:00
|
|
|
var cond string
|
|
|
|
|
var args []any
|
|
|
|
|
var err error
|
|
|
|
|
if e.cond != nil {
|
|
|
|
|
cond, args, err = e.cond.ToSql()
|
|
|
|
|
if e.numeric {
|
|
|
|
|
cond = strings.ReplaceAll(cond, "value", "CAST(value AS REAL)")
|
|
|
|
|
}
|
|
|
|
|
cond = fmt.Sprintf("exists (select 1 from json_tree(media_file.tags, '$.%s') where key='value' and %s)", e.tag, cond)
|
|
|
|
|
} else {
|
|
|
|
|
cond = fmt.Sprintf("exists (select 1 from json_tree(media_file.tags, '$.%s') where key='value')", e.tag)
|
2026-04-24 23:18:20 -04:00
|
|
|
}
|
|
|
|
|
if e.not {
|
|
|
|
|
cond = "not " + cond
|
|
|
|
|
}
|
|
|
|
|
return cond, args, err
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
type roleCond struct {
|
|
|
|
|
role string
|
|
|
|
|
cond squirrel.Sqlizer
|
|
|
|
|
not bool
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
func (e roleCond) ToSql() (string, []any, error) {
|
2026-04-28 19:40:08 -04:00
|
|
|
var cond string
|
|
|
|
|
var args []any
|
|
|
|
|
if e.cond != nil {
|
2026-05-22 18:00:13 -03:00
|
|
|
innerSQL, innerArgs, err := roleCondSQL(e.cond)
|
|
|
|
|
if err != nil {
|
|
|
|
|
return "", nil, err
|
|
|
|
|
}
|
|
|
|
|
cond = roleExistsSQL(innerSQL)
|
|
|
|
|
args = append([]any{e.role}, innerArgs...)
|
2026-04-28 19:40:08 -04:00
|
|
|
} else {
|
2026-05-22 18:00:13 -03:00
|
|
|
cond = "exists (select 1 from media_file_artists mfa where mfa.media_file_id = media_file.id and mfa.role = ?)"
|
|
|
|
|
args = []any{e.role}
|
2026-04-28 19:40:08 -04:00
|
|
|
}
|
2026-04-24 23:18:20 -04:00
|
|
|
if e.not {
|
|
|
|
|
cond = "not " + cond
|
|
|
|
|
}
|
2026-05-22 18:00:13 -03:00
|
|
|
return cond, args, nil
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
// roleCondSQL extracts SQL from a squirrel condition and rewrites the placeholder column name.
|
|
|
|
|
func roleCondSQL(cond squirrel.Sqlizer) (string, []any, error) {
|
|
|
|
|
sql, args, err := cond.ToSql()
|
|
|
|
|
if err != nil {
|
|
|
|
|
return "", nil, err
|
|
|
|
|
}
|
|
|
|
|
return strings.ReplaceAll(sql, "value", "artist.name"), args, nil
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
// roleExistsSQL wraps a condition fragment in the standard role EXISTS subquery.
|
|
|
|
|
func roleExistsSQL(innerCond string) string {
|
|
|
|
|
return fmt.Sprintf("exists (select 1 from media_file_artists mfa join artist on artist.id = mfa.artist_id "+
|
|
|
|
|
"where mfa.media_file_id = media_file.id and mfa.role = ? and %s)", innerCond)
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
// jsonCondBatchSize limits how many conditions are ORed inside a single EXISTS subquery
|
|
|
|
|
// to stay within SQLite's expression tree depth limit (max 1000). The EXISTS wrapper
|
|
|
|
|
// consumes ~4 levels; each ORed condition adds 1 level. Empirically, 496 is the maximum.
|
|
|
|
|
const jsonCondBatchSize = 350
|
|
|
|
|
|
|
|
|
|
// mergeJsonConds collapses multiple non-negated roleCond or tagCond entries for the same
|
|
|
|
|
// field within an OR group into batched EXISTS subqueries with the conditions ORed inside.
|
|
|
|
|
// This turns N separate correlated subqueries into ceil(N/batchSize), dramatically
|
|
|
|
|
// improving performance for smart playlists with many patterns.
|
|
|
|
|
func mergeJsonConds(or squirrel.Or) squirrel.Sqlizer {
|
2026-06-14 10:47:11 -04:00
|
|
|
if merged, ok := mergeSameFieldConds(or, false); ok {
|
|
|
|
|
return squirrel.Or(merged)
|
|
|
|
|
}
|
|
|
|
|
return or
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
// mergeNegatedJsonConds is the AND-group counterpart to mergeJsonConds, merging negated
|
|
|
|
|
// conditions. By De Morgan, "NOT EXISTS(X) AND NOT EXISTS(Y)" == "NOT EXISTS(X OR Y)".
|
|
|
|
|
func mergeNegatedJsonConds(and squirrel.And) squirrel.Sqlizer {
|
|
|
|
|
if merged, ok := mergeSameFieldConds(and, true); ok {
|
|
|
|
|
return squirrel.And(merged)
|
|
|
|
|
}
|
|
|
|
|
return and
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
// mergeSameFieldConds groups roleCond/tagCond entries that share a field and the requested
|
|
|
|
|
// polarity, replacing each group of 2+ with batched roleCondGroup/tagCondGroup subqueries.
|
|
|
|
|
// Returns the rewritten conditions and whether any merge happened.
|
|
|
|
|
func mergeSameFieldConds(conds []squirrel.Sqlizer, negated bool) ([]squirrel.Sqlizer, bool) {
|
2026-05-22 18:00:13 -03:00
|
|
|
type condEntry struct {
|
|
|
|
|
index int
|
|
|
|
|
cond squirrel.Sqlizer
|
|
|
|
|
}
|
|
|
|
|
type group struct {
|
|
|
|
|
entries []condEntry
|
|
|
|
|
isRole bool
|
|
|
|
|
numeric bool
|
|
|
|
|
tag string
|
|
|
|
|
}
|
|
|
|
|
groups := make(map[string]*group)
|
2026-06-14 10:47:11 -04:00
|
|
|
for i, s := range conds {
|
2026-05-22 18:00:13 -03:00
|
|
|
switch c := s.(type) {
|
|
|
|
|
case roleCond:
|
2026-06-14 10:47:11 -04:00
|
|
|
if c.not != negated || c.cond == nil {
|
2026-05-22 18:00:13 -03:00
|
|
|
continue
|
|
|
|
|
}
|
|
|
|
|
g, exists := groups["role:"+c.role]
|
|
|
|
|
if !exists {
|
|
|
|
|
g = &group{isRole: true}
|
|
|
|
|
groups["role:"+c.role] = g
|
|
|
|
|
}
|
|
|
|
|
g.entries = append(g.entries, condEntry{index: i, cond: c.cond})
|
|
|
|
|
case tagCond:
|
2026-06-14 10:47:11 -04:00
|
|
|
if c.not != negated || c.cond == nil {
|
2026-05-22 18:00:13 -03:00
|
|
|
continue
|
|
|
|
|
}
|
|
|
|
|
g, exists := groups["tag:"+c.tag]
|
|
|
|
|
if !exists {
|
|
|
|
|
g = &group{tag: c.tag, numeric: c.numeric}
|
|
|
|
|
groups["tag:"+c.tag] = g
|
|
|
|
|
}
|
|
|
|
|
g.entries = append(g.entries, condEntry{index: i, cond: c.cond})
|
|
|
|
|
}
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
remove := make(map[int]bool)
|
|
|
|
|
var additions []squirrel.Sqlizer
|
|
|
|
|
for _, key := range slices.Sorted(maps.Keys(groups)) {
|
|
|
|
|
g := groups[key]
|
|
|
|
|
if len(g.entries) < 2 {
|
|
|
|
|
continue
|
|
|
|
|
}
|
2026-06-14 10:47:11 -04:00
|
|
|
batchConds := make([]squirrel.Sqlizer, len(g.entries))
|
2026-05-22 18:00:13 -03:00
|
|
|
for i, e := range g.entries {
|
2026-06-14 10:47:11 -04:00
|
|
|
remove[e.index] = true
|
|
|
|
|
batchConds[i] = e.cond
|
2026-05-22 18:00:13 -03:00
|
|
|
}
|
|
|
|
|
if g.isRole {
|
|
|
|
|
role := key[len("role:"):]
|
2026-06-14 10:47:11 -04:00
|
|
|
for batch := range slices.Chunk(batchConds, jsonCondBatchSize) {
|
|
|
|
|
additions = append(additions, roleCondGroup{role: role, conds: batch, not: negated})
|
2026-05-22 18:00:13 -03:00
|
|
|
}
|
|
|
|
|
} else {
|
2026-06-14 10:47:11 -04:00
|
|
|
for batch := range slices.Chunk(batchConds, jsonCondBatchSize) {
|
|
|
|
|
additions = append(additions, tagCondGroup{tag: g.tag, numeric: g.numeric, conds: batch, not: negated})
|
2026-05-22 18:00:13 -03:00
|
|
|
}
|
|
|
|
|
}
|
|
|
|
|
}
|
|
|
|
|
|
2026-06-14 10:47:11 -04:00
|
|
|
if len(remove) == 0 {
|
|
|
|
|
return conds, false
|
2026-05-22 18:00:13 -03:00
|
|
|
}
|
|
|
|
|
|
2026-06-14 10:47:11 -04:00
|
|
|
result := make([]squirrel.Sqlizer, 0, len(conds)-len(remove)+len(additions))
|
|
|
|
|
for i, s := range conds {
|
2026-05-22 18:00:13 -03:00
|
|
|
if !remove[i] {
|
|
|
|
|
result = append(result, s)
|
|
|
|
|
}
|
|
|
|
|
}
|
2026-06-14 10:47:11 -04:00
|
|
|
return append(result, additions...), true
|
2026-05-22 18:00:13 -03:00
|
|
|
}
|
|
|
|
|
|
2026-06-14 10:47:11 -04:00
|
|
|
// roleCondGroup represents multiple role conditions for the same role, merged into a single
|
|
|
|
|
// (optionally negated) EXISTS subquery for performance.
|
2026-05-22 18:00:13 -03:00
|
|
|
type roleCondGroup struct {
|
|
|
|
|
role string
|
|
|
|
|
conds []squirrel.Sqlizer
|
2026-06-14 10:47:11 -04:00
|
|
|
not bool
|
2026-05-22 18:00:13 -03:00
|
|
|
}
|
|
|
|
|
|
|
|
|
|
func (g roleCondGroup) ToSql() (string, []any, error) {
|
|
|
|
|
innerParts := make([]string, 0, len(g.conds))
|
|
|
|
|
allArgs := []any{g.role}
|
|
|
|
|
for _, c := range g.conds {
|
|
|
|
|
part, args, err := roleCondSQL(c)
|
|
|
|
|
if err != nil {
|
|
|
|
|
return "", nil, err
|
|
|
|
|
}
|
|
|
|
|
innerParts = append(innerParts, part)
|
|
|
|
|
allArgs = append(allArgs, args...)
|
|
|
|
|
}
|
|
|
|
|
cond := roleExistsSQL("(" + strings.Join(innerParts, " OR ") + ")")
|
2026-06-14 10:47:11 -04:00
|
|
|
if g.not {
|
|
|
|
|
cond = "not " + cond
|
|
|
|
|
}
|
2026-05-22 18:00:13 -03:00
|
|
|
return cond, allArgs, nil
|
|
|
|
|
}
|
|
|
|
|
|
2026-06-14 10:47:11 -04:00
|
|
|
// tagCondGroup represents multiple tag conditions for the same tag, merged into a single
|
|
|
|
|
// (optionally negated) EXISTS subquery for performance.
|
2026-05-22 18:00:13 -03:00
|
|
|
type tagCondGroup struct {
|
|
|
|
|
tag string
|
|
|
|
|
numeric bool
|
|
|
|
|
conds []squirrel.Sqlizer
|
2026-06-14 10:47:11 -04:00
|
|
|
not bool
|
2026-05-22 18:00:13 -03:00
|
|
|
}
|
|
|
|
|
|
|
|
|
|
func (g tagCondGroup) ToSql() (string, []any, error) {
|
|
|
|
|
innerParts := make([]string, 0, len(g.conds))
|
|
|
|
|
var allArgs []any
|
|
|
|
|
for _, c := range g.conds {
|
|
|
|
|
part, args, err := c.ToSql()
|
|
|
|
|
if err != nil {
|
|
|
|
|
return "", nil, err
|
|
|
|
|
}
|
|
|
|
|
if g.numeric {
|
|
|
|
|
part = strings.ReplaceAll(part, "value", "CAST(value AS REAL)")
|
|
|
|
|
}
|
|
|
|
|
innerParts = append(innerParts, part)
|
|
|
|
|
allArgs = append(allArgs, args...)
|
|
|
|
|
}
|
|
|
|
|
cond := fmt.Sprintf("exists (select 1 from json_tree(media_file.tags, '$.%s') where key='value' and (%s))",
|
|
|
|
|
g.tag, strings.Join(innerParts, " OR "))
|
2026-06-14 10:47:11 -04:00
|
|
|
if g.not {
|
|
|
|
|
cond = "not " + cond
|
|
|
|
|
}
|
2026-05-22 18:00:13 -03:00
|
|
|
return cond, allArgs, nil
|
2026-04-24 23:18:20 -04:00
|
|
|
}
|
|
|
|
|
|
2026-04-25 14:59:06 -04:00
|
|
|
func singleField(values map[string]any) (string, any, criteria.FieldInfo, bool) {
|
2026-04-24 23:18:20 -04:00
|
|
|
if len(values) != 1 {
|
|
|
|
|
return "", nil, criteria.FieldInfo{}, false
|
|
|
|
|
}
|
|
|
|
|
for field, value := range values {
|
|
|
|
|
info, ok := criteria.LookupField(field)
|
|
|
|
|
return field, value, info, ok
|
|
|
|
|
}
|
|
|
|
|
return "", nil, criteria.FieldInfo{}, false
|
|
|
|
|
}
|
|
|
|
|
|
2026-04-25 14:59:06 -04:00
|
|
|
func sqlFields(values map[string]any) (map[string]any, error) {
|
2026-04-24 23:18:20 -04:00
|
|
|
fields := make(map[string]any, len(values))
|
|
|
|
|
for field, value := range values {
|
|
|
|
|
info, ok := criteria.LookupField(field)
|
|
|
|
|
if !ok {
|
|
|
|
|
return nil, fmt.Errorf("invalid field in criteria: %s", field)
|
|
|
|
|
}
|
|
|
|
|
if info.IsTag || info.IsRole {
|
|
|
|
|
return nil, fmt.Errorf("tag and role criteria must contain exactly one field: %s", field)
|
|
|
|
|
}
|
2026-04-28 20:22:48 -04:00
|
|
|
sqlField, ok := fieldExpr(info.Name())
|
2026-04-24 23:18:20 -04:00
|
|
|
if !ok || sqlField == "" {
|
|
|
|
|
return nil, fmt.Errorf("invalid field in criteria: %s", field)
|
|
|
|
|
}
|
|
|
|
|
fields[sqlField] = value
|
|
|
|
|
}
|
|
|
|
|
return fields, nil
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
func fieldExpr(name string) (string, bool) {
|
|
|
|
|
field, ok := smartPlaylistFields[strings.ToLower(name)]
|
|
|
|
|
return field.expr, ok
|
|
|
|
|
}
|
|
|
|
|
|
perf(smartplaylist): use annotation index for playcount/rating/loved filters (#5662)
* 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>
2026-06-24 22:26:41 -04:00
|
|
|
// comparator is a scalar SQL comparison operator used by smart playlist criteria.
|
|
|
|
|
type comparator struct {
|
|
|
|
|
build func(map[string]any) squirrel.Sqlizer
|
|
|
|
|
satisfy func(a, b float64) bool // the operator as a predicate, to reason about a column's COALESCE default
|
|
|
|
|
// ordering is false only for = and <>, the only operators with a clean bare form over bool columns.
|
|
|
|
|
ordering bool
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
var (
|
|
|
|
|
cmpEq = comparator{build: func(f map[string]any) squirrel.Sqlizer { return squirrel.Eq(f) }, satisfy: func(a, b float64) bool { return a == b }}
|
|
|
|
|
cmpNe = comparator{build: func(f map[string]any) squirrel.Sqlizer { return squirrel.NotEq(f) }, satisfy: func(a, b float64) bool { return a != b }}
|
|
|
|
|
cmpGt = comparator{build: func(f map[string]any) squirrel.Sqlizer { return squirrel.Gt(f) }, satisfy: func(a, b float64) bool { return a > b }, ordering: true}
|
|
|
|
|
cmpGe = comparator{build: func(f map[string]any) squirrel.Sqlizer { return squirrel.GtOrEq(f) }, satisfy: func(a, b float64) bool { return a >= b }, ordering: true}
|
|
|
|
|
cmpLt = comparator{build: func(f map[string]any) squirrel.Sqlizer { return squirrel.Lt(f) }, satisfy: func(a, b float64) bool { return a < b }, ordering: true}
|
|
|
|
|
cmpLe = comparator{build: func(f map[string]any) squirrel.Sqlizer { return squirrel.LtOrEq(f) }, satisfy: func(a, b float64) bool { return a <= b }, ordering: true}
|
|
|
|
|
)
|
|
|
|
|
|
|
|
|
|
// annotationField returns the field definition only for nullable annotation columns that have a
|
|
|
|
|
// COALESCE default (playcount, rating, loved). Date annotation columns (lastplayed, dateloved, ...)
|
|
|
|
|
// have no default and return ok=false.
|
|
|
|
|
func annotationField(info criteria.FieldInfo) (smartPlaylistField, bool) {
|
|
|
|
|
f, ok := smartPlaylistFields[info.Name()]
|
|
|
|
|
if !ok || f.coalesceDefault == nil {
|
|
|
|
|
return smartPlaylistField{}, false
|
|
|
|
|
}
|
|
|
|
|
return f, true
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
// coalesced wraps the field in COALESCE(col, default) (bare expression if it has no default). Used
|
|
|
|
|
// where index-friendliness does not apply (ORDER BY, list comparisons) and the missing-row-as-default
|
|
|
|
|
// semantics must be kept.
|
|
|
|
|
func (f smartPlaylistField) coalesced() string {
|
|
|
|
|
if f.coalesceDefault == nil {
|
|
|
|
|
return f.expr
|
|
|
|
|
}
|
|
|
|
|
return fmt.Sprintf("COALESCE(%s, %s)", f.expr, sqlLiteral(f.coalesceDefault))
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
func comparisonExpr(values map[string]any, cmp comparator) (squirrel.Sqlizer, error) {
|
|
|
|
|
if _, value, info, ok := singleField(values); ok {
|
|
|
|
|
if info.IsTag || info.IsRole {
|
|
|
|
|
return jsonExpr(info, cmp.build(map[string]any{"value": value}), false), nil
|
|
|
|
|
}
|
|
|
|
|
if f, isAnnotation := annotationField(info); isAnnotation {
|
|
|
|
|
return annotationCond(f, cmp, value), nil
|
|
|
|
|
}
|
|
|
|
|
}
|
|
|
|
|
fields, err := sqlFields(values)
|
|
|
|
|
if err != nil {
|
|
|
|
|
return nil, err
|
|
|
|
|
}
|
|
|
|
|
return cmp.build(fields), nil
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
// annotationCond builds a comparison against a nullable annotation column (see smartPlaylistField
|
|
|
|
|
// for why). When bareNullInclusion can reason about the comparison exactly it emits the
|
|
|
|
|
// index-friendly bare `col <cmp> ?`, adding `OR col IS NULL` only when the default would match;
|
|
|
|
|
// otherwise it falls back to the COALESCE form, which can't use the index but is always equivalent.
|
|
|
|
|
func annotationCond(f smartPlaylistField, cmp comparator, value any) squirrel.Sqlizer {
|
|
|
|
|
wrapNull, ok := bareNullInclusion(f.coalesceDefault, cmp, value)
|
|
|
|
|
if !ok {
|
|
|
|
|
return cmp.build(map[string]any{f.coalesced(): value})
|
|
|
|
|
}
|
|
|
|
|
base := cmp.build(map[string]any{f.expr: value})
|
|
|
|
|
if !wrapNull {
|
|
|
|
|
return base
|
|
|
|
|
}
|
|
|
|
|
// The default satisfies the predicate, so missing rows (NULL) must be included. All comparators
|
|
|
|
|
// (including <>) evaluate to false against NULL, so an explicit IS NULL restores them.
|
|
|
|
|
return squirrel.Or{base, squirrel.Eq{f.expr: nil}}
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
// bareNullInclusion decides whether the index-friendly bare form is safe for this comparison and,
|
|
|
|
|
// if so, whether missing (NULL) rows must be re-included via `OR col IS NULL`. It returns ok=false
|
|
|
|
|
// when the bare form can't be proven equivalent to COALESCE(col, default) <cmp> ? — i.e. the value
|
|
|
|
|
// isn't a scalar the comparator can order exactly (lists, bool ordering, unparseable values) — in
|
|
|
|
|
// which case the caller keeps the COALESCE form. When ok=true, wrapNull is true iff the default
|
|
|
|
|
// value itself satisfies the predicate (so missing rows would match and must be preserved).
|
|
|
|
|
func bareNullInclusion(defaultVal any, cmp comparator, value any) (wrapNull, ok bool) {
|
|
|
|
|
if b, isBool := defaultVal.(bool); isBool {
|
|
|
|
|
// Bool columns have an exact bare form only for equality; ordering operators fall back to
|
|
|
|
|
// COALESCE. Mapping both bools to 0/1 lets cmp.satisfy reuse the numeric eq/ne predicate.
|
|
|
|
|
if cmp.ordering {
|
|
|
|
|
return false, false
|
|
|
|
|
}
|
|
|
|
|
v, okV := criteria.ToBool(value)
|
|
|
|
|
if !okV {
|
|
|
|
|
return false, false
|
|
|
|
|
}
|
|
|
|
|
return cmp.satisfy(boolToFloat(b), boolToFloat(v)), true
|
|
|
|
|
}
|
|
|
|
|
d, okD := toFloat(defaultVal)
|
|
|
|
|
v, okV := toFloat(value)
|
|
|
|
|
if !okD || !okV {
|
|
|
|
|
// Non-scalar or unparseable value: no exact bare form, keep COALESCE.
|
|
|
|
|
return false, false
|
|
|
|
|
}
|
|
|
|
|
return cmp.satisfy(d, v), true
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
func boolToFloat(b bool) float64 {
|
|
|
|
|
if b {
|
|
|
|
|
return 1
|
|
|
|
|
}
|
|
|
|
|
return 0
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
// toFloat coerces a scalar criteria value to float64. Criteria values come from JSON (float64,
|
|
|
|
|
// string) or are built in Go (int, float64), so only those types are handled; anything else —
|
|
|
|
|
// including a slice or an unparseable string — reports ok=false so the caller keeps the COALESCE
|
|
|
|
|
// form instead of an index-friendly bare comparison.
|
|
|
|
|
func toFloat(v any) (float64, bool) {
|
|
|
|
|
switch n := v.(type) {
|
|
|
|
|
case float64:
|
|
|
|
|
return n, true
|
|
|
|
|
case int:
|
|
|
|
|
return float64(n), true
|
|
|
|
|
case int64:
|
|
|
|
|
return float64(n), true
|
|
|
|
|
case string:
|
|
|
|
|
f, err := strconv.ParseFloat(n, 64)
|
|
|
|
|
return f, err == nil
|
|
|
|
|
default:
|
|
|
|
|
return 0, false
|
|
|
|
|
}
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
// sqlLiteral renders an annotation field's COALESCE default (0 or false) as a SQL literal for ORDER
|
|
|
|
|
// BY. %v renders both bool and numeric defaults correctly (false/true, 0).
|
|
|
|
|
func sqlLiteral(v any) string {
|
|
|
|
|
return fmt.Sprintf("%v", v)
|
|
|
|
|
}
|
|
|
|
|
|
2026-04-24 23:18:20 -04:00
|
|
|
func fieldJoinType(name string) smartPlaylistJoinType {
|
|
|
|
|
info, ok := criteria.LookupField(name)
|
|
|
|
|
if !ok {
|
|
|
|
|
return smartPlaylistJoinNone
|
|
|
|
|
}
|
2026-04-28 20:22:48 -04:00
|
|
|
field, ok := smartPlaylistFields[info.Name()]
|
2026-04-24 23:18:20 -04:00
|
|
|
if !ok {
|
|
|
|
|
return smartPlaylistJoinNone
|
|
|
|
|
}
|
|
|
|
|
return field.joinType
|
|
|
|
|
}
|
|
|
|
|
|
feat(scrobbler): add per-user scrobble filter (#5964)
* feat(scrobbler): add scrobble_filter column to user
* feat(scrobbler): validate scrobble filter criteria on user save
* refactor(persistence): make smart playlist join helpers package-level
* feat(scrobbler): add MediaFileRepository.MatchesCriteria
* feat(scrobbler): filter external scrobbles with per-user criteria
* feat(ui): add scrobble filter field to user form
* fix(scrobbler): default scrobble_filter to empty string for existing users
* refactor(scrobbler): also gate playback reports on the scrobble filter
Playback reports carry the same track metadata to plugin scrobblers, so a
filtered track leaked through that third dispatch path. Skip the filter
evaluation entirely when no scrobbler is active.
* refactor(persistence): move criteria join building into criteria_sql.go
The join set a criteria needs was decided in criteria_sql.go but built in
smart_playlist_repository.go, so both callers had to pair the two by hand.
* refactor(persistence): unexport smartPlaylistCriteria methods
The type never leaves the package, so the exported names advertised an API
that callers outside persistence could never reach. Also disambiguates
where/orderBy from squirrel's SelectBuilder methods of the same name.
* fix(ui): cap the scrobble filter field width
fullWidth stretched it across the whole page next to 256px inputs. Bounded
at 40em, with two rows and a resize handle so JSON rules stay readable.
* refactor(ui): move scrobble filter input in UserEdit component
* feat(ui): add pt-BR translations for the scrobble filter
* fix(scrobbler): take the filter verdict before incPlay
incPlay mutates play counts and dates a filter can test on, so evaluating at
dispatch time let one play decide differently on either side of the increment:
a track could be scrobbled despite matching, or lose only its stopped report
and strand presence plugins. Reject limit/offset too, rather than silently
ignoring part of a rule copied from a smart playlist.
* fix(scrobbler): filter the report from an expired session
The expiry callback runs with a stub user carrying no filter, so evaluating
there always returned false and leaked the track to plugin scrobblers. That is
the normal path for clients that never send stopped, such as legacy Subsonic
now-playing. Carry the last verdict on the session instead.
* refactor(scrobbler): skip the now-playing enqueue instead of threading the verdict
Queuing an entry only to drop it at dispatch also cancelled a pending
announcement for the previous, unfiltered track, since the queue is keyed by
player and a new entry replaces the old one.
* fix(scrobbler): evaluate the filter regardless of active scrobblers
The verdict is stored on the session and dispatched at expiry, so skipping
evaluation when no scrobbler was active let a plugin enabled mid-session
receive a filtered track. The empty-filter guard above already gives servers
without scrobbling the same free path, so the shortcut only ever applied to
users who had a filter set.
2026-08-15 16:10:53 -04:00
|
|
|
func (c smartPlaylistCriteria) expressionJoins() smartPlaylistJoinType {
|
2026-04-24 23:18:20 -04:00
|
|
|
var joins smartPlaylistJoinType
|
refactor: centralize criteria sort parsing and extract smart playlist logic (#5415)
* test: add tests for recordingdate alias resolution in smart playlists
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: update FieldInfo structure and simplify fieldMap initialization
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: move sort parsing logic from persistence to criteria package
Extracted sort field parsing, validation, and direction handling from
persistence/criteria_sql.go into model/criteria/sort.go. The new
OrderByFields method on Criteria parses the Sort/Order strings into
validated SortField structs (field name + direction), resolving aliases
and handling +/- prefixes and order inversion. The persistence layer now
consumes these parsed fields and only handles SQL expression mapping.
This centralizes sort parsing to enforce consistent implementations.
* refactor: standardize field access in smartPlaylistCriteria structure
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: add ResolveLimit method to Criteria
Moved the percentage-limit resolution logic from playlist_repository
into Criteria.ResolveLimit, replacing the 3-line mutate-after-query
pattern with a single method call. The method preserves LimitPercent
rather than zeroing it, since IsPercentageLimit already returns false
once Limit is set, making the clear redundant and lossy.
* refactor: improve child playlist loading and error handling in refresh logic
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: extract smart playlist logic to dedicated files
Moved refreshSmartPlaylist, addSmartPlaylistAnnotationJoins, and
addCriteria methods from playlist_repository.go to a new
smart_playlist_repository.go file. Extracted all smart playlist tests
to smart_playlist_repository_test.go. Added DeferCleanup to the
"valid rules" test to fix ordering flakiness when Ginkgo randomizes
test execution across files.
* refactor: break refreshSmartPlaylist into smaller focused methods
Split the monolithic refreshSmartPlaylist method into discrete helpers
for readability: shouldRefreshSmartPlaylist for guard checks,
refreshChildPlaylists for recursive dependency refresh,
resolvePercentageLimit for count-based limit resolution,
buildSmartPlaylistQuery for assembling the SELECT with joins, and
addMediaFileAnnotationJoin to DRY up the repeated annotation join clause.
* refactor: deduplicate child playlist IDs in Criteria
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: simplify withSmartPlaylistOwner to accept model.User
Replaced separate ownerID string and ownerIsAdmin bool parameters with a
single model.User struct, reducing the field count in smartPlaylistCriteria
and making the option function signature clearer. Updated all call sites
and tests accordingly.
* fix: handle empty sort fields and propagate child playlist load errors
OrderByFields now falls back to [{title, asc}] when all user-supplied
sort fields are invalid, preventing empty ORDER BY clauses that would
produce invalid SQL in row_number() window functions. Also restored the
original behavior where a DB error loading child playlists aborts the
parent smart playlist refresh, by making refreshChildPlaylists return a
bool.
* refactor: log warning when no valid sort fields are found
Signed-off-by: Deluan <deluan@navidrome.org>
---------
Signed-off-by: Deluan <deluan@navidrome.org>
2026-04-26 14:49:59 -04:00
|
|
|
_ = criteria.Walk(c.Criteria.Expression, func(expr criteria.Expression) error {
|
2026-04-24 23:18:20 -04:00
|
|
|
for field := range criteria.Fields(expr) {
|
|
|
|
|
joins |= fieldJoinType(field)
|
|
|
|
|
}
|
|
|
|
|
return nil
|
|
|
|
|
})
|
|
|
|
|
return joins
|
|
|
|
|
}
|
|
|
|
|
|
feat(scrobbler): add per-user scrobble filter (#5964)
* feat(scrobbler): add scrobble_filter column to user
* feat(scrobbler): validate scrobble filter criteria on user save
* refactor(persistence): make smart playlist join helpers package-level
* feat(scrobbler): add MediaFileRepository.MatchesCriteria
* feat(scrobbler): filter external scrobbles with per-user criteria
* feat(ui): add scrobble filter field to user form
* fix(scrobbler): default scrobble_filter to empty string for existing users
* refactor(scrobbler): also gate playback reports on the scrobble filter
Playback reports carry the same track metadata to plugin scrobblers, so a
filtered track leaked through that third dispatch path. Skip the filter
evaluation entirely when no scrobbler is active.
* refactor(persistence): move criteria join building into criteria_sql.go
The join set a criteria needs was decided in criteria_sql.go but built in
smart_playlist_repository.go, so both callers had to pair the two by hand.
* refactor(persistence): unexport smartPlaylistCriteria methods
The type never leaves the package, so the exported names advertised an API
that callers outside persistence could never reach. Also disambiguates
where/orderBy from squirrel's SelectBuilder methods of the same name.
* fix(ui): cap the scrobble filter field width
fullWidth stretched it across the whole page next to 256px inputs. Bounded
at 40em, with two rows and a resize handle so JSON rules stay readable.
* refactor(ui): move scrobble filter input in UserEdit component
* feat(ui): add pt-BR translations for the scrobble filter
* fix(scrobbler): take the filter verdict before incPlay
incPlay mutates play counts and dates a filter can test on, so evaluating at
dispatch time let one play decide differently on either side of the increment:
a track could be scrobbled despite matching, or lose only its stopped report
and strand presence plugins. Reject limit/offset too, rather than silently
ignoring part of a rule copied from a smart playlist.
* fix(scrobbler): filter the report from an expired session
The expiry callback runs with a stub user carrying no filter, so evaluating
there always returned false and leaked the track to plugin scrobblers. That is
the normal path for clients that never send stopped, such as legacy Subsonic
now-playing. Carry the last verdict on the session instead.
* refactor(scrobbler): skip the now-playing enqueue instead of threading the verdict
Queuing an entry only to drop it at dispatch also cancelled a pending
announcement for the previous, unfiltered track, since the queue is keyed by
player and a new entry replaces the old one.
* fix(scrobbler): evaluate the filter regardless of active scrobblers
The verdict is stored on the session and dispatched at expiry, so skipping
evaluation when no scrobbler was active let a plugin enabled mid-session
receive a filtered track. The empty-filter guard above already gives servers
without scrobbling the same free path, so the shortcut only ever applied to
users who had a filter set.
2026-08-15 16:10:53 -04:00
|
|
|
func (c smartPlaylistCriteria) requiredJoins() smartPlaylistJoinType {
|
|
|
|
|
joins := c.expressionJoins()
|
refactor: centralize criteria sort parsing and extract smart playlist logic (#5415)
* test: add tests for recordingdate alias resolution in smart playlists
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: update FieldInfo structure and simplify fieldMap initialization
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: move sort parsing logic from persistence to criteria package
Extracted sort field parsing, validation, and direction handling from
persistence/criteria_sql.go into model/criteria/sort.go. The new
OrderByFields method on Criteria parses the Sort/Order strings into
validated SortField structs (field name + direction), resolving aliases
and handling +/- prefixes and order inversion. The persistence layer now
consumes these parsed fields and only handles SQL expression mapping.
This centralizes sort parsing to enforce consistent implementations.
* refactor: standardize field access in smartPlaylistCriteria structure
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: add ResolveLimit method to Criteria
Moved the percentage-limit resolution logic from playlist_repository
into Criteria.ResolveLimit, replacing the 3-line mutate-after-query
pattern with a single method call. The method preserves LimitPercent
rather than zeroing it, since IsPercentageLimit already returns false
once Limit is set, making the clear redundant and lossy.
* refactor: improve child playlist loading and error handling in refresh logic
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: extract smart playlist logic to dedicated files
Moved refreshSmartPlaylist, addSmartPlaylistAnnotationJoins, and
addCriteria methods from playlist_repository.go to a new
smart_playlist_repository.go file. Extracted all smart playlist tests
to smart_playlist_repository_test.go. Added DeferCleanup to the
"valid rules" test to fix ordering flakiness when Ginkgo randomizes
test execution across files.
* refactor: break refreshSmartPlaylist into smaller focused methods
Split the monolithic refreshSmartPlaylist method into discrete helpers
for readability: shouldRefreshSmartPlaylist for guard checks,
refreshChildPlaylists for recursive dependency refresh,
resolvePercentageLimit for count-based limit resolution,
buildSmartPlaylistQuery for assembling the SELECT with joins, and
addMediaFileAnnotationJoin to DRY up the repeated annotation join clause.
* refactor: deduplicate child playlist IDs in Criteria
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: simplify withSmartPlaylistOwner to accept model.User
Replaced separate ownerID string and ownerIsAdmin bool parameters with a
single model.User struct, reducing the field count in smartPlaylistCriteria
and making the option function signature clearer. Updated all call sites
and tests accordingly.
* fix: handle empty sort fields and propagate child playlist load errors
OrderByFields now falls back to [{title, asc}] when all user-supplied
sort fields are invalid, preventing empty ORDER BY clauses that would
produce invalid SQL in row_number() window functions. Also restored the
original behavior where a DB error loading child playlists aborts the
parent smart playlist refresh, by making refreshChildPlaylists return a
bool.
* refactor: log warning when no valid sort fields are found
Signed-off-by: Deluan <deluan@navidrome.org>
---------
Signed-off-by: Deluan <deluan@navidrome.org>
2026-04-26 14:49:59 -04:00
|
|
|
for _, name := range c.Criteria.SortFieldNames() {
|
|
|
|
|
joins |= fieldJoinType(name)
|
2026-04-24 23:18:20 -04:00
|
|
|
}
|
|
|
|
|
return joins
|
|
|
|
|
}
|
|
|
|
|
|
feat(scrobbler): add per-user scrobble filter (#5964)
* feat(scrobbler): add scrobble_filter column to user
* feat(scrobbler): validate scrobble filter criteria on user save
* refactor(persistence): make smart playlist join helpers package-level
* feat(scrobbler): add MediaFileRepository.MatchesCriteria
* feat(scrobbler): filter external scrobbles with per-user criteria
* feat(ui): add scrobble filter field to user form
* fix(scrobbler): default scrobble_filter to empty string for existing users
* refactor(scrobbler): also gate playback reports on the scrobble filter
Playback reports carry the same track metadata to plugin scrobblers, so a
filtered track leaked through that third dispatch path. Skip the filter
evaluation entirely when no scrobbler is active.
* refactor(persistence): move criteria join building into criteria_sql.go
The join set a criteria needs was decided in criteria_sql.go but built in
smart_playlist_repository.go, so both callers had to pair the two by hand.
* refactor(persistence): unexport smartPlaylistCriteria methods
The type never leaves the package, so the exported names advertised an API
that callers outside persistence could never reach. Also disambiguates
where/orderBy from squirrel's SelectBuilder methods of the same name.
* fix(ui): cap the scrobble filter field width
fullWidth stretched it across the whole page next to 256px inputs. Bounded
at 40em, with two rows and a resize handle so JSON rules stay readable.
* refactor(ui): move scrobble filter input in UserEdit component
* feat(ui): add pt-BR translations for the scrobble filter
* fix(scrobbler): take the filter verdict before incPlay
incPlay mutates play counts and dates a filter can test on, so evaluating at
dispatch time let one play decide differently on either side of the increment:
a track could be scrobbled despite matching, or lose only its stopped report
and strand presence plugins. Reject limit/offset too, rather than silently
ignoring part of a rule copied from a smart playlist.
* fix(scrobbler): filter the report from an expired session
The expiry callback runs with a stub user carrying no filter, so evaluating
there always returned false and leaked the track to plugin scrobblers. That is
the normal path for clients that never send stopped, such as legacy Subsonic
now-playing. Carry the last verdict on the session instead.
* refactor(scrobbler): skip the now-playing enqueue instead of threading the verdict
Queuing an entry only to drop it at dispatch also cancelled a pending
announcement for the previous, unfiltered track, since the queue is keyed by
player and a new entry replaces the old one.
* fix(scrobbler): evaluate the filter regardless of active scrobblers
The verdict is stored on the session and dispatched at expiry, so skipping
evaluation when no scrobbler was active let a plugin enabled mid-session
receive a filtered track. The empty-filter guard above already gives servers
without scrobbling the same free path, so the shortcut only ever applied to
users who had a filter set.
2026-08-15 16:10:53 -04:00
|
|
|
// applyExpressionJoins adds every join the criteria's WHERE clause resolves against.
|
|
|
|
|
func (c smartPlaylistCriteria) applyExpressionJoins(sq squirrel.SelectBuilder, userID string) squirrel.SelectBuilder {
|
|
|
|
|
return c.applyJoins(sq, c.expressionJoins(), userID)
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
// applyRequiredJoins adds the WHERE joins plus any the ORDER BY resolves against.
|
|
|
|
|
func (c smartPlaylistCriteria) applyRequiredJoins(sq squirrel.SelectBuilder, userID string) squirrel.SelectBuilder {
|
|
|
|
|
return c.applyJoins(sq, c.requiredJoins(), userID)
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
// applyJoins joins the media_file annotation unconditionally — annotation fields
|
|
|
|
|
// COALESCE a missing row to a default, so the row has to be reachable to be absent.
|
|
|
|
|
func (c smartPlaylistCriteria) applyJoins(sq squirrel.SelectBuilder, joins smartPlaylistJoinType, userID string) squirrel.SelectBuilder {
|
|
|
|
|
sq = sq.LeftJoin("annotation on ("+
|
|
|
|
|
"annotation.item_id = media_file.id"+
|
|
|
|
|
" AND annotation.item_type = 'media_file'"+
|
|
|
|
|
" AND annotation.user_id = ?)", userID)
|
|
|
|
|
if joins.has(smartPlaylistJoinAlbumAnnotation) {
|
|
|
|
|
sq = sq.LeftJoin("annotation AS album_annotation ON ("+
|
|
|
|
|
"album_annotation.item_id = media_file.album_id"+
|
|
|
|
|
" AND album_annotation.item_type = 'album'"+
|
|
|
|
|
" AND album_annotation.user_id = ?)", userID)
|
|
|
|
|
}
|
|
|
|
|
if joins.has(smartPlaylistJoinArtistAnnotation) {
|
|
|
|
|
sq = sq.LeftJoin("annotation AS artist_annotation ON ("+
|
|
|
|
|
"artist_annotation.item_id = media_file.artist_id"+
|
|
|
|
|
" AND artist_annotation.item_type = 'artist'"+
|
|
|
|
|
" AND artist_annotation.user_id = ?)", userID)
|
|
|
|
|
}
|
|
|
|
|
if joins.has(smartPlaylistJoinAlbum) {
|
|
|
|
|
sq = sq.LeftJoin("album ON album.id = media_file.album_id")
|
|
|
|
|
}
|
|
|
|
|
return sq
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
func (c smartPlaylistCriteria) orderBy() string {
|
refactor: centralize criteria sort parsing and extract smart playlist logic (#5415)
* test: add tests for recordingdate alias resolution in smart playlists
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: update FieldInfo structure and simplify fieldMap initialization
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: move sort parsing logic from persistence to criteria package
Extracted sort field parsing, validation, and direction handling from
persistence/criteria_sql.go into model/criteria/sort.go. The new
OrderByFields method on Criteria parses the Sort/Order strings into
validated SortField structs (field name + direction), resolving aliases
and handling +/- prefixes and order inversion. The persistence layer now
consumes these parsed fields and only handles SQL expression mapping.
This centralizes sort parsing to enforce consistent implementations.
* refactor: standardize field access in smartPlaylistCriteria structure
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: add ResolveLimit method to Criteria
Moved the percentage-limit resolution logic from playlist_repository
into Criteria.ResolveLimit, replacing the 3-line mutate-after-query
pattern with a single method call. The method preserves LimitPercent
rather than zeroing it, since IsPercentageLimit already returns false
once Limit is set, making the clear redundant and lossy.
* refactor: improve child playlist loading and error handling in refresh logic
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: extract smart playlist logic to dedicated files
Moved refreshSmartPlaylist, addSmartPlaylistAnnotationJoins, and
addCriteria methods from playlist_repository.go to a new
smart_playlist_repository.go file. Extracted all smart playlist tests
to smart_playlist_repository_test.go. Added DeferCleanup to the
"valid rules" test to fix ordering flakiness when Ginkgo randomizes
test execution across files.
* refactor: break refreshSmartPlaylist into smaller focused methods
Split the monolithic refreshSmartPlaylist method into discrete helpers
for readability: shouldRefreshSmartPlaylist for guard checks,
refreshChildPlaylists for recursive dependency refresh,
resolvePercentageLimit for count-based limit resolution,
buildSmartPlaylistQuery for assembling the SELECT with joins, and
addMediaFileAnnotationJoin to DRY up the repeated annotation join clause.
* refactor: deduplicate child playlist IDs in Criteria
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: simplify withSmartPlaylistOwner to accept model.User
Replaced separate ownerID string and ownerIsAdmin bool parameters with a
single model.User struct, reducing the field count in smartPlaylistCriteria
and making the option function signature clearer. Updated all call sites
and tests accordingly.
* fix: handle empty sort fields and propagate child playlist load errors
OrderByFields now falls back to [{title, asc}] when all user-supplied
sort fields are invalid, preventing empty ORDER BY clauses that would
produce invalid SQL in row_number() window functions. Also restored the
original behavior where a DB error loading child playlists aborts the
parent smart playlist refresh, by making refreshChildPlaylists return a
bool.
* refactor: log warning when no valid sort fields are found
Signed-off-by: Deluan <deluan@navidrome.org>
---------
Signed-off-by: Deluan <deluan@navidrome.org>
2026-04-26 14:49:59 -04:00
|
|
|
sortFields := c.Criteria.OrderByFields()
|
|
|
|
|
parts := make([]string, 0, len(sortFields))
|
|
|
|
|
for _, sf := range sortFields {
|
|
|
|
|
mapped, ok := sortExpr(sf.Field)
|
2026-04-24 23:18:20 -04:00
|
|
|
if !ok {
|
|
|
|
|
continue
|
|
|
|
|
}
|
refactor: centralize criteria sort parsing and extract smart playlist logic (#5415)
* test: add tests for recordingdate alias resolution in smart playlists
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: update FieldInfo structure and simplify fieldMap initialization
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: move sort parsing logic from persistence to criteria package
Extracted sort field parsing, validation, and direction handling from
persistence/criteria_sql.go into model/criteria/sort.go. The new
OrderByFields method on Criteria parses the Sort/Order strings into
validated SortField structs (field name + direction), resolving aliases
and handling +/- prefixes and order inversion. The persistence layer now
consumes these parsed fields and only handles SQL expression mapping.
This centralizes sort parsing to enforce consistent implementations.
* refactor: standardize field access in smartPlaylistCriteria structure
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: add ResolveLimit method to Criteria
Moved the percentage-limit resolution logic from playlist_repository
into Criteria.ResolveLimit, replacing the 3-line mutate-after-query
pattern with a single method call. The method preserves LimitPercent
rather than zeroing it, since IsPercentageLimit already returns false
once Limit is set, making the clear redundant and lossy.
* refactor: improve child playlist loading and error handling in refresh logic
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: extract smart playlist logic to dedicated files
Moved refreshSmartPlaylist, addSmartPlaylistAnnotationJoins, and
addCriteria methods from playlist_repository.go to a new
smart_playlist_repository.go file. Extracted all smart playlist tests
to smart_playlist_repository_test.go. Added DeferCleanup to the
"valid rules" test to fix ordering flakiness when Ginkgo randomizes
test execution across files.
* refactor: break refreshSmartPlaylist into smaller focused methods
Split the monolithic refreshSmartPlaylist method into discrete helpers
for readability: shouldRefreshSmartPlaylist for guard checks,
refreshChildPlaylists for recursive dependency refresh,
resolvePercentageLimit for count-based limit resolution,
buildSmartPlaylistQuery for assembling the SELECT with joins, and
addMediaFileAnnotationJoin to DRY up the repeated annotation join clause.
* refactor: deduplicate child playlist IDs in Criteria
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: simplify withSmartPlaylistOwner to accept model.User
Replaced separate ownerID string and ownerIsAdmin bool parameters with a
single model.User struct, reducing the field count in smartPlaylistCriteria
and making the option function signature clearer. Updated all call sites
and tests accordingly.
* fix: handle empty sort fields and propagate child playlist load errors
OrderByFields now falls back to [{title, asc}] when all user-supplied
sort fields are invalid, preventing empty ORDER BY clauses that would
produce invalid SQL in row_number() window functions. Also restored the
original behavior where a DB error loading child playlists aborts the
parent smart playlist refresh, by making refreshChildPlaylists return a
bool.
* refactor: log warning when no valid sort fields are found
Signed-off-by: Deluan <deluan@navidrome.org>
---------
Signed-off-by: Deluan <deluan@navidrome.org>
2026-04-26 14:49:59 -04:00
|
|
|
dir := "asc"
|
|
|
|
|
if sf.Desc {
|
|
|
|
|
dir = "desc"
|
2026-04-24 23:18:20 -04:00
|
|
|
}
|
refactor: centralize criteria sort parsing and extract smart playlist logic (#5415)
* test: add tests for recordingdate alias resolution in smart playlists
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: update FieldInfo structure and simplify fieldMap initialization
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: move sort parsing logic from persistence to criteria package
Extracted sort field parsing, validation, and direction handling from
persistence/criteria_sql.go into model/criteria/sort.go. The new
OrderByFields method on Criteria parses the Sort/Order strings into
validated SortField structs (field name + direction), resolving aliases
and handling +/- prefixes and order inversion. The persistence layer now
consumes these parsed fields and only handles SQL expression mapping.
This centralizes sort parsing to enforce consistent implementations.
* refactor: standardize field access in smartPlaylistCriteria structure
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: add ResolveLimit method to Criteria
Moved the percentage-limit resolution logic from playlist_repository
into Criteria.ResolveLimit, replacing the 3-line mutate-after-query
pattern with a single method call. The method preserves LimitPercent
rather than zeroing it, since IsPercentageLimit already returns false
once Limit is set, making the clear redundant and lossy.
* refactor: improve child playlist loading and error handling in refresh logic
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: extract smart playlist logic to dedicated files
Moved refreshSmartPlaylist, addSmartPlaylistAnnotationJoins, and
addCriteria methods from playlist_repository.go to a new
smart_playlist_repository.go file. Extracted all smart playlist tests
to smart_playlist_repository_test.go. Added DeferCleanup to the
"valid rules" test to fix ordering flakiness when Ginkgo randomizes
test execution across files.
* refactor: break refreshSmartPlaylist into smaller focused methods
Split the monolithic refreshSmartPlaylist method into discrete helpers
for readability: shouldRefreshSmartPlaylist for guard checks,
refreshChildPlaylists for recursive dependency refresh,
resolvePercentageLimit for count-based limit resolution,
buildSmartPlaylistQuery for assembling the SELECT with joins, and
addMediaFileAnnotationJoin to DRY up the repeated annotation join clause.
* refactor: deduplicate child playlist IDs in Criteria
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: simplify withSmartPlaylistOwner to accept model.User
Replaced separate ownerID string and ownerIsAdmin bool parameters with a
single model.User struct, reducing the field count in smartPlaylistCriteria
and making the option function signature clearer. Updated all call sites
and tests accordingly.
* fix: handle empty sort fields and propagate child playlist load errors
OrderByFields now falls back to [{title, asc}] when all user-supplied
sort fields are invalid, preventing empty ORDER BY clauses that would
produce invalid SQL in row_number() window functions. Also restored the
original behavior where a DB error loading child playlists aborts the
parent smart playlist refresh, by making refreshChildPlaylists return a
bool.
* refactor: log warning when no valid sort fields are found
Signed-off-by: Deluan <deluan@navidrome.org>
---------
Signed-off-by: Deluan <deluan@navidrome.org>
2026-04-26 14:49:59 -04:00
|
|
|
parts = append(parts, mapped+" "+dir)
|
2026-04-24 23:18:20 -04:00
|
|
|
}
|
refactor: centralize criteria sort parsing and extract smart playlist logic (#5415)
* test: add tests for recordingdate alias resolution in smart playlists
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: update FieldInfo structure and simplify fieldMap initialization
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: move sort parsing logic from persistence to criteria package
Extracted sort field parsing, validation, and direction handling from
persistence/criteria_sql.go into model/criteria/sort.go. The new
OrderByFields method on Criteria parses the Sort/Order strings into
validated SortField structs (field name + direction), resolving aliases
and handling +/- prefixes and order inversion. The persistence layer now
consumes these parsed fields and only handles SQL expression mapping.
This centralizes sort parsing to enforce consistent implementations.
* refactor: standardize field access in smartPlaylistCriteria structure
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: add ResolveLimit method to Criteria
Moved the percentage-limit resolution logic from playlist_repository
into Criteria.ResolveLimit, replacing the 3-line mutate-after-query
pattern with a single method call. The method preserves LimitPercent
rather than zeroing it, since IsPercentageLimit already returns false
once Limit is set, making the clear redundant and lossy.
* refactor: improve child playlist loading and error handling in refresh logic
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: extract smart playlist logic to dedicated files
Moved refreshSmartPlaylist, addSmartPlaylistAnnotationJoins, and
addCriteria methods from playlist_repository.go to a new
smart_playlist_repository.go file. Extracted all smart playlist tests
to smart_playlist_repository_test.go. Added DeferCleanup to the
"valid rules" test to fix ordering flakiness when Ginkgo randomizes
test execution across files.
* refactor: break refreshSmartPlaylist into smaller focused methods
Split the monolithic refreshSmartPlaylist method into discrete helpers
for readability: shouldRefreshSmartPlaylist for guard checks,
refreshChildPlaylists for recursive dependency refresh,
resolvePercentageLimit for count-based limit resolution,
buildSmartPlaylistQuery for assembling the SELECT with joins, and
addMediaFileAnnotationJoin to DRY up the repeated annotation join clause.
* refactor: deduplicate child playlist IDs in Criteria
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: simplify withSmartPlaylistOwner to accept model.User
Replaced separate ownerID string and ownerIsAdmin bool parameters with a
single model.User struct, reducing the field count in smartPlaylistCriteria
and making the option function signature clearer. Updated all call sites
and tests accordingly.
* fix: handle empty sort fields and propagate child playlist load errors
OrderByFields now falls back to [{title, asc}] when all user-supplied
sort fields are invalid, preventing empty ORDER BY clauses that would
produce invalid SQL in row_number() window functions. Also restored the
original behavior where a DB error loading child playlists aborts the
parent smart playlist refresh, by making refreshChildPlaylists return a
bool.
* refactor: log warning when no valid sort fields are found
Signed-off-by: Deluan <deluan@navidrome.org>
---------
Signed-off-by: Deluan <deluan@navidrome.org>
2026-04-26 14:49:59 -04:00
|
|
|
return strings.Join(parts, ", ")
|
2026-04-24 23:18:20 -04:00
|
|
|
}
|
|
|
|
|
|
|
|
|
|
func sortExpr(sortField string) (string, bool) {
|
|
|
|
|
info, ok := criteria.LookupField(sortField)
|
|
|
|
|
if !ok {
|
|
|
|
|
return "", false
|
|
|
|
|
}
|
2026-04-28 20:22:48 -04:00
|
|
|
if field, ok := smartPlaylistFields[info.Name()]; ok && field.order != "" {
|
2026-04-24 23:18:20 -04:00
|
|
|
return field.order, true
|
|
|
|
|
}
|
|
|
|
|
var mapped string
|
|
|
|
|
switch {
|
|
|
|
|
case info.IsTag:
|
2026-04-28 20:22:48 -04:00
|
|
|
mapped = "COALESCE(json_extract(media_file.tags, '$." + info.Name() + "[0].value'), '')"
|
2026-04-24 23:18:20 -04:00
|
|
|
case info.IsRole:
|
2026-04-28 20:22:48 -04:00
|
|
|
mapped = "COALESCE(json_extract(media_file.participants, '$." + info.Name() + "[0].name'), '')"
|
2026-04-24 23:18:20 -04:00
|
|
|
default:
|
2026-04-28 20:22:48 -04:00
|
|
|
field, ok := smartPlaylistFields[info.Name()]
|
2026-04-24 23:18:20 -04:00
|
|
|
if !ok || field.expr == "" {
|
|
|
|
|
return "", false
|
|
|
|
|
}
|
perf(smartplaylist): use annotation index for playcount/rating/loved filters (#5662)
* 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>
2026-06-24 22:26:41 -04:00
|
|
|
// Sorting keeps the COALESCE default so missing-annotation rows sort as that default
|
|
|
|
|
// (filtering drops COALESCE for index use, but ORDER BY has no index to preserve here).
|
|
|
|
|
mapped = field.coalesced()
|
2026-04-24 23:18:20 -04:00
|
|
|
}
|
|
|
|
|
if info.Numeric {
|
|
|
|
|
mapped = fmt.Sprintf("CAST(%s AS REAL)", mapped)
|
|
|
|
|
}
|
|
|
|
|
return mapped, true
|
|
|
|
|
}
|