navidrome/persistence/criteria_sql_test.go

Ignoring revisions in .git-blame-ignore-revs. Click here to bypass and see the normal blame view.

647 lines
36 KiB
Go
Raw Permalink Normal View History

package persistence
import (
fix(smartplaylists): optimize smart playlist performance for role and tag criteria (#5515) * fix(server): optimize smart playlist role queries for large criteria (#5511) Role-based smart playlist criteria (artist, composer, etc.) now query the indexed media_file_artists join table instead of parsing JSON via json_tree() on every row. Multiple conditions for the same role within an OR group are merged into a single EXISTS subquery (batched at 200 to stay under SQLite's expression tree depth limit). A composite index (media_file_id, role) replaces the now-redundant single-column (media_file_id) index on media_file_artists. Benchmark (40k tracks, 500 patterns, 3 artists/track): - Merged join-table: 15ms (9.3x faster) - Merged json_tree: 30ms (4.6x faster) - Unmerged baseline: 137ms * refactor: simplify role condition SQL generation and benchmark Extract shared roleCondSQL/roleExistsSQL helpers to deduplicate the EXISTS template between roleCond and roleCondGroup. Use slices.Chunk for batching per project convention. Extract runBenchQuery helper to eliminate triplicated benchmark execution loop. * chore: raise roleCondBatchSize to 350 The empirical SQLite limit is 496 conditions per merged EXISTS subquery. Raising from 200 to 350 reduces the number of batches (e.g. 500 patterns now splits into 2 batches instead of 3). * fix(server): apply OR-merge optimization to tag conditions too Generalize mergeRoleConds into mergeJsonConds to also collapse multiple tag conditions for the same tag (e.g. genre) within OR groups. This gives the same ~5x speedup for tag-heavy smart playlists as the role optimization gives for artist-heavy ones. * refactor: benchmark uses real criteria pipeline instead of hand-built SQL The "Current" sub-benchmark now builds criteria.Criteria expressions and runs them through the actual newSmartPlaylistCriteria → Where() → ToSql() pipeline, validating the real production code path. The baseline still uses hand-built SQL representing the old json_tree approach. * fix: stabilize merged group ordering and close rows before error check Sort group keys in mergeJsonConds so the merged additions have deterministic order across runs, improving SQLite statement cache reuse. Move rows.Close() before rows.Err() in benchmark helper.
2026-05-22 18:00:13 -03:00
"fmt"
"strings"
"time"
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"
"github.com/navidrome/navidrome/model/criteria"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
)
var _ = Describe("Smart playlist criteria SQL", func() {
BeforeEach(func() {
criteria.AddRoles([]string{"artist", "composer", "producer"})
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
criteria.AddTagNames([]string{"genre", "mood", "releasetype", "recordingdate", "replaygain_album_gain"})
criteria.AddNumericTags([]string{"rate"})
})
DescribeTable("expressions",
func(expr criteria.Expression, expectedSQL string, expectedArgs ...any) {
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
sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).where()
Expect(err).ToNot(HaveOccurred())
sql, args, err := sqlizer.ToSql()
Expect(err).ToNot(HaveOccurred())
Expect(sql).To(Equal(expectedSQL))
Expect(args).To(HaveExactElements(expectedArgs...))
},
Entry("all group",
criteria.All{criteria.Contains{"title": "love"}, criteria.Gt{"rating": 3}},
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
"(media_file.title LIKE ? AND annotation.rating > ?)", "%love%", 3),
Entry("any group",
criteria.Any{criteria.Is{"title": "Low Rider"}, criteria.Is{"album": "Best Of"}},
"(media_file.title = ? OR media_file.album = ?)", "Low Rider", "Best Of"),
Entry("is string", criteria.Is{"title": "Low Rider"}, "media_file.title = ?", "Low Rider"),
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
Entry("is bool", criteria.Is{"loved": true}, "annotation.starred = ?", true),
Entry("is numeric list", criteria.Is{"library_id": []int{1, 2}}, "media_file.library_id IN (?,?)", 1, 2),
Entry("is not", criteria.IsNot{"title": "Low Rider"}, "media_file.title <> ?", "Low Rider"),
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
Entry("gt", criteria.Gt{"playCount": 10}, "annotation.play_count > ?", 10),
Entry("lt", criteria.Lt{"playCount": 10}, "(annotation.play_count < ? OR annotation.play_count IS NULL)", 10),
Entry("contains", criteria.Contains{"title": "Low Rider"}, "media_file.title LIKE ?", "%Low Rider%"),
Entry("not contains", criteria.NotContains{"title": "Low Rider"}, "media_file.title NOT LIKE ?", "%Low Rider%"),
Entry("starts with", criteria.StartsWith{"title": "Low Rider"}, "media_file.title LIKE ?", "Low Rider%"),
Entry("ends with", criteria.EndsWith{"title": "Low Rider"}, "media_file.title LIKE ?", "%Low Rider"),
Entry("in range", criteria.InTheRange{"year": []int{1980, 1990}}, "(media_file.year >= ? AND media_file.year <= ?)", 1980, 1990),
Entry("before", criteria.Before{"lastPlayed": time.Date(2021, 10, 1, 0, 0, 0, 0, time.Local)}, "annotation.play_date < ?", time.Date(2021, 10, 1, 0, 0, 0, 0, time.Local)),
Entry("after", criteria.After{"lastPlayed": time.Date(2021, 10, 1, 0, 0, 0, 0, time.Local)}, "annotation.play_date > ?", time.Date(2021, 10, 1, 0, 0, 0, 0, time.Local)),
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
Entry("in playlist [path]", criteria.InPlaylist{"path": "lacuslacus.nsp"}, "media_file.id IN (SELECT media_file_id FROM playlist_tracks pl LEFT JOIN playlist on pl.playlist_id = playlist.id WHERE (playlist.path IN (?) AND playlist.public = ?))", "lacuslacus.nsp", 1),
Entry("in playlist [id]", criteria.InPlaylist{"id": "deadbeef-dead-beef"}, "media_file.id IN (SELECT media_file_id FROM playlist_tracks pl LEFT JOIN playlist on pl.playlist_id = playlist.id WHERE (pl.playlist_id = ? AND playlist.public = ?))", "deadbeef-dead-beef", 1),
Entry("in playlist [empty id falls back to path]", criteria.InPlaylist{"id": "", "path": "/music/x.nsp"}, "media_file.id IN (SELECT media_file_id FROM playlist_tracks pl LEFT JOIN playlist on pl.playlist_id = playlist.id WHERE (playlist.path IN (?) AND playlist.public = ?))", "/music/x.nsp", 1),
Entry("in playlist [decomposed unicode path]", criteria.InPlaylist{"path": "/m\u00fasica/x.nsp"}, "media_file.id IN (SELECT media_file_id FROM playlist_tracks pl LEFT JOIN playlist on pl.playlist_id = playlist.id WHERE (playlist.path IN (?,?) AND playlist.public = ?))", "/m\u00fasica/x.nsp", "/mu\u0301sica/x.nsp", 1),
Entry("not in playlist", criteria.NotInPlaylist{"id": "deadbeef-dead-beef"}, "media_file.id NOT IN (SELECT media_file_id FROM playlist_tracks pl LEFT JOIN playlist on pl.playlist_id = playlist.id WHERE (pl.playlist_id = ? AND playlist.public = ?))", "deadbeef-dead-beef", 1),
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
Entry("album annotation", criteria.Gt{"albumRating": 3}, "album_annotation.rating > ?", 3),
Entry("artist annotation", criteria.Is{"artistLoved": true}, "artist_annotation.starred = ?", true),
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
Entry("album column", criteria.Gt{"albumSongCount": 5}, "album.song_count > ?", 5),
Entry("album duration column", criteria.Lt{"albumDuration": 600}, "album.duration < ?", 600),
Entry("album size column", criteria.Gt{"albumSize": 1000}, "album.size > ?", 1000),
Entry("album date column", criteria.After{"albumDateAdded": time.Date(2021, 10, 1, 0, 0, 0, 0, time.Local)}, "album.created_at > ?", time.Date(2021, 10, 1, 0, 0, 0, 0, time.Local)),
Entry("album modified column", criteria.Before{"albumDateModified": time.Date(2021, 10, 1, 0, 0, 0, 0, time.Local)}, "album.updated_at < ?", time.Date(2021, 10, 1, 0, 0, 0, 0, time.Local)),
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
// Annotation fields use a COALESCE default (0 for numeric, false for bool) so that tracks
// with no annotation row behave as that default. To keep the annotation index usable, the
// COALESCE is dropped when the compared value cannot match the default (the missing-row
// case is then naturally excluded); otherwise an explicit `OR col IS NULL` preserves it.
Entry("is safe (value != default)", criteria.Is{"playCount": 3}, "annotation.play_count = ?", 3),
Entry("is unsafe (value == default)", criteria.Is{"playCount": 0},
"(annotation.play_count = ? OR annotation.play_count IS NULL)", 0),
Entry("is bool false (value == default)", criteria.Is{"loved": false},
"(annotation.starred = ? OR annotation.starred IS NULL)", false),
Entry("gt safe (value >= default)", criteria.Gt{"playCount": 0}, "annotation.play_count > ?", 0),
Entry("gt unsafe (value < default)", criteria.Gt{"playCount": -1},
"(annotation.play_count > ? OR annotation.play_count IS NULL)", -1),
Entry("lt safe (value <= default)", criteria.Lt{"playCount": 0}, "annotation.play_count < ?", 0),
Entry("lt unsafe (value > default)", criteria.Lt{"playCount": 5},
"(annotation.play_count < ? OR annotation.play_count IS NULL)", 5),
Entry("isNot annotation keeps null match", criteria.IsNot{"playCount": 3},
"(annotation.play_count <> ? OR annotation.play_count IS NULL)", 3),
Entry("isNot annotation value == default", criteria.IsNot{"playCount": 0},
"annotation.play_count <> ?", 0),
Entry("in range spanning default", criteria.InTheRange{"playCount": []int{-1, 5}},
"((annotation.play_count >= ? OR annotation.play_count IS NULL) AND (annotation.play_count <= ? OR annotation.play_count IS NULL))", -1, 5),
Entry("in range above default", criteria.InTheRange{"playCount": []int{1, 5}},
"(annotation.play_count >= ? AND (annotation.play_count <= ? OR annotation.play_count IS NULL))", 1, 5),
// A list value can't drive the index and a default-inclusive list has per-element NULL
// semantics, so the COALESCE form is kept to stay equivalent to the original.
Entry("is list keeps coalesce", criteria.Is{"playCount": []int{0, 3}},
"COALESCE(annotation.play_count, 0) IN (?,?)", 0, 3),
// LIKE operators can't use the column index, so annotation fields keep the COALESCE form to
// match missing-annotation rows exactly as before (a NULL column never matches LIKE).
Entry("contains annotation keeps coalesce", criteria.Contains{"playCount": 0},
"COALESCE(annotation.play_count, 0) LIKE ?", "%0%"),
Entry("starts with annotation keeps coalesce", criteria.StartsWith{"rating": 5},
"COALESCE(annotation.rating, 0) LIKE ?", "5%"),
Entry("not contains annotation keeps coalesce", criteria.NotContains{"playCount": 0},
"COALESCE(annotation.play_count, 0) NOT LIKE ?", "%0%"),
// Bool annotation fields only have a clean index-friendly form for equality; ordering
// comparators keep the COALESCE form so the missing-row default is honored exactly.
Entry("gt bool keeps coalesce", criteria.Gt{"loved": false},
"COALESCE(annotation.starred, false) > ?", false),
// A list value on a bool field is non-scalar, so it keeps the COALESCE form too (same as the
// numeric list case) — otherwise a NULL column would diverge from the original.
Entry("is bool list keeps coalesce", criteria.Is{"loved": []any{true}},
"COALESCE(annotation.starred, false) IN (?)", true),
Entry("tag is", criteria.Is{"genre": "Rock"}, "exists (select 1 from json_tree(media_file.tags, '$.genre') where key='value' and value = ?)", "Rock"),
Entry("tag is not", criteria.IsNot{"genre": "Rock"}, "not exists (select 1 from json_tree(media_file.tags, '$.genre') where key='value' and value = ?)", "Rock"),
Entry("tag contains", criteria.Contains{"genre": "Rock"}, "exists (select 1 from json_tree(media_file.tags, '$.genre') where key='value' and value LIKE ?)", "%Rock%"),
Entry("tag not contains", criteria.NotContains{"genre": "Rock"}, "not exists (select 1 from json_tree(media_file.tags, '$.genre') where key='value' and value LIKE ?)", "%Rock%"),
Entry("numeric tag", criteria.Lt{"rate": 6}, "exists (select 1 from json_tree(media_file.tags, '$.rate') where key='value' and CAST(value AS REAL) < ?)", 6),
Entry("tag alias", criteria.Is{"albumtype": "album"}, "exists (select 1 from json_tree(media_file.tags, '$.releasetype') where key='value' and value = ?)", "album"),
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
Entry("field alias via tag registration", criteria.Is{"recordingdate": "2024-01-01"}, "media_file.date = ?", "2024-01-01"),
fix(smartplaylists): optimize smart playlist performance for role and tag criteria (#5515) * fix(server): optimize smart playlist role queries for large criteria (#5511) Role-based smart playlist criteria (artist, composer, etc.) now query the indexed media_file_artists join table instead of parsing JSON via json_tree() on every row. Multiple conditions for the same role within an OR group are merged into a single EXISTS subquery (batched at 200 to stay under SQLite's expression tree depth limit). A composite index (media_file_id, role) replaces the now-redundant single-column (media_file_id) index on media_file_artists. Benchmark (40k tracks, 500 patterns, 3 artists/track): - Merged join-table: 15ms (9.3x faster) - Merged json_tree: 30ms (4.6x faster) - Unmerged baseline: 137ms * refactor: simplify role condition SQL generation and benchmark Extract shared roleCondSQL/roleExistsSQL helpers to deduplicate the EXISTS template between roleCond and roleCondGroup. Use slices.Chunk for batching per project convention. Extract runBenchQuery helper to eliminate triplicated benchmark execution loop. * chore: raise roleCondBatchSize to 350 The empirical SQLite limit is 496 conditions per merged EXISTS subquery. Raising from 200 to 350 reduces the number of batches (e.g. 500 patterns now splits into 2 batches instead of 3). * fix(server): apply OR-merge optimization to tag conditions too Generalize mergeRoleConds into mergeJsonConds to also collapse multiple tag conditions for the same tag (e.g. genre) within OR groups. This gives the same ~5x speedup for tag-heavy smart playlists as the role optimization gives for artist-heavy ones. * refactor: benchmark uses real criteria pipeline instead of hand-built SQL The "Current" sub-benchmark now builds criteria.Criteria expressions and runs them through the actual newSmartPlaylistCriteria → Where() → ToSql() pipeline, validating the real production code path. The baseline still uses hand-built SQL representing the old json_tree approach. * fix: stabilize merged group ordering and close rows before error check Sort group keys in mergeJsonConds so the merged additions have deterministic order across runs, improving SQLite statement cache reuse. Move rows.Close() before rows.Err() in benchmark helper.
2026-05-22 18:00:13 -03:00
Entry("role is", criteria.Is{"artist": "u2"}, "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 artist.name = ?)", "artist", "u2"),
Entry("role contains", criteria.Contains{"composer": "Lennon"}, "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 artist.name LIKE ?)", "composer", "%Lennon%"),
Entry("role not contains", criteria.NotContains{"artist": "u2"}, "not 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 artist.name LIKE ?)", "artist", "%u2%"),
// ReplayGain fields
Entry("rgAlbumGain is", criteria.Is{"rgAlbumGain": 0}, "media_file.rg_album_gain = ?", 0),
Entry("rgAlbumGain gt", criteria.Gt{"rgAlbumGain": -6.0}, "media_file.rg_album_gain > ?", -6.0),
Entry("rgTrackPeak lt", criteria.Lt{"rgTrackPeak": 1.0}, "media_file.rg_track_peak < ?", 1.0),
feat(smartplaylist): add isMissing and isPresent operators (#5436) * feat(smartplaylist): add IsMissing and IsPresent operator types Add two new Expression types for detecting absent/present tags and roles in smart playlist criteria. Includes JSON marshal/unmarshal support and Walk visitor registration. * test(smartplaylist): add JSON marshal/unmarshal tests for isMissing/isPresent * feat(smartplaylist): add SQL generation for isMissing/isPresent operators Tags check json_tree(media_file.tags) for key existence. Roles check json_tree(media_file.participants) for key existence. Regular DB column fields are rejected with an error. * test(smartplaylist): add e2e tests for isMissing/isPresent operators Tests cover tag presence/absence with selective matching (grouping), universal absence (lyricist role), universal presence (composer role), and combined operator usage. * refactor(smartplaylist): use strconv.ParseBool in IsTruthy Replace hand-rolled string truthiness check with strconv.ParseBool, which correctly handles standard boolean strings and rejects unrecognized values as false. * refactor(smartplaylist): clarify missingExpr parameter naming Rename defaultNegate to checkAbsence and extract truthy local for readability. The XNOR logic (checkAbsence == truthy) is now easier to follow: isMissing passes true, isPresent passes false. * refactor(smartplaylist): reuse jsonExpr in missingExpr, improve errors - tagCond/roleCond now handle nil cond (existence-only check) - missingExpr delegates to jsonExpr(info, nil, negate) instead of building SQL manually - Better error messages: unknown fields now report the field name
2026-04-28 19:40:08 -04:00
// isMissing — tags
Entry("isMissing tag [true]", criteria.IsMissing{"genre": true},
"not exists (select 1 from json_tree(media_file.tags, '$.genre') where key='value')"),
Entry("isMissing tag [false]", criteria.IsMissing{"genre": false},
"exists (select 1 from json_tree(media_file.tags, '$.genre') where key='value')"),
// isMissing — roles
Entry("isMissing role [true]", criteria.IsMissing{"artist": true},
fix(smartplaylists): optimize smart playlist performance for role and tag criteria (#5515) * fix(server): optimize smart playlist role queries for large criteria (#5511) Role-based smart playlist criteria (artist, composer, etc.) now query the indexed media_file_artists join table instead of parsing JSON via json_tree() on every row. Multiple conditions for the same role within an OR group are merged into a single EXISTS subquery (batched at 200 to stay under SQLite's expression tree depth limit). A composite index (media_file_id, role) replaces the now-redundant single-column (media_file_id) index on media_file_artists. Benchmark (40k tracks, 500 patterns, 3 artists/track): - Merged join-table: 15ms (9.3x faster) - Merged json_tree: 30ms (4.6x faster) - Unmerged baseline: 137ms * refactor: simplify role condition SQL generation and benchmark Extract shared roleCondSQL/roleExistsSQL helpers to deduplicate the EXISTS template between roleCond and roleCondGroup. Use slices.Chunk for batching per project convention. Extract runBenchQuery helper to eliminate triplicated benchmark execution loop. * chore: raise roleCondBatchSize to 350 The empirical SQLite limit is 496 conditions per merged EXISTS subquery. Raising from 200 to 350 reduces the number of batches (e.g. 500 patterns now splits into 2 batches instead of 3). * fix(server): apply OR-merge optimization to tag conditions too Generalize mergeRoleConds into mergeJsonConds to also collapse multiple tag conditions for the same tag (e.g. genre) within OR groups. This gives the same ~5x speedup for tag-heavy smart playlists as the role optimization gives for artist-heavy ones. * refactor: benchmark uses real criteria pipeline instead of hand-built SQL The "Current" sub-benchmark now builds criteria.Criteria expressions and runs them through the actual newSmartPlaylistCriteria → Where() → ToSql() pipeline, validating the real production code path. The baseline still uses hand-built SQL representing the old json_tree approach. * fix: stabilize merged group ordering and close rows before error check Sort group keys in mergeJsonConds so the merged additions have deterministic order across runs, improving SQLite statement cache reuse. Move rows.Close() before rows.Err() in benchmark helper.
2026-05-22 18:00:13 -03:00
"not exists (select 1 from media_file_artists mfa where mfa.media_file_id = media_file.id and mfa.role = ?)", "artist"),
feat(smartplaylist): add isMissing and isPresent operators (#5436) * feat(smartplaylist): add IsMissing and IsPresent operator types Add two new Expression types for detecting absent/present tags and roles in smart playlist criteria. Includes JSON marshal/unmarshal support and Walk visitor registration. * test(smartplaylist): add JSON marshal/unmarshal tests for isMissing/isPresent * feat(smartplaylist): add SQL generation for isMissing/isPresent operators Tags check json_tree(media_file.tags) for key existence. Roles check json_tree(media_file.participants) for key existence. Regular DB column fields are rejected with an error. * test(smartplaylist): add e2e tests for isMissing/isPresent operators Tests cover tag presence/absence with selective matching (grouping), universal absence (lyricist role), universal presence (composer role), and combined operator usage. * refactor(smartplaylist): use strconv.ParseBool in IsTruthy Replace hand-rolled string truthiness check with strconv.ParseBool, which correctly handles standard boolean strings and rejects unrecognized values as false. * refactor(smartplaylist): clarify missingExpr parameter naming Rename defaultNegate to checkAbsence and extract truthy local for readability. The XNOR logic (checkAbsence == truthy) is now easier to follow: isMissing passes true, isPresent passes false. * refactor(smartplaylist): reuse jsonExpr in missingExpr, improve errors - tagCond/roleCond now handle nil cond (existence-only check) - missingExpr delegates to jsonExpr(info, nil, negate) instead of building SQL manually - Better error messages: unknown fields now report the field name
2026-04-28 19:40:08 -04:00
Entry("isMissing role [false]", criteria.IsMissing{"artist": false},
fix(smartplaylists): optimize smart playlist performance for role and tag criteria (#5515) * fix(server): optimize smart playlist role queries for large criteria (#5511) Role-based smart playlist criteria (artist, composer, etc.) now query the indexed media_file_artists join table instead of parsing JSON via json_tree() on every row. Multiple conditions for the same role within an OR group are merged into a single EXISTS subquery (batched at 200 to stay under SQLite's expression tree depth limit). A composite index (media_file_id, role) replaces the now-redundant single-column (media_file_id) index on media_file_artists. Benchmark (40k tracks, 500 patterns, 3 artists/track): - Merged join-table: 15ms (9.3x faster) - Merged json_tree: 30ms (4.6x faster) - Unmerged baseline: 137ms * refactor: simplify role condition SQL generation and benchmark Extract shared roleCondSQL/roleExistsSQL helpers to deduplicate the EXISTS template between roleCond and roleCondGroup. Use slices.Chunk for batching per project convention. Extract runBenchQuery helper to eliminate triplicated benchmark execution loop. * chore: raise roleCondBatchSize to 350 The empirical SQLite limit is 496 conditions per merged EXISTS subquery. Raising from 200 to 350 reduces the number of batches (e.g. 500 patterns now splits into 2 batches instead of 3). * fix(server): apply OR-merge optimization to tag conditions too Generalize mergeRoleConds into mergeJsonConds to also collapse multiple tag conditions for the same tag (e.g. genre) within OR groups. This gives the same ~5x speedup for tag-heavy smart playlists as the role optimization gives for artist-heavy ones. * refactor: benchmark uses real criteria pipeline instead of hand-built SQL The "Current" sub-benchmark now builds criteria.Criteria expressions and runs them through the actual newSmartPlaylistCriteria → Where() → ToSql() pipeline, validating the real production code path. The baseline still uses hand-built SQL representing the old json_tree approach. * fix: stabilize merged group ordering and close rows before error check Sort group keys in mergeJsonConds so the merged additions have deterministic order across runs, improving SQLite statement cache reuse. Move rows.Close() before rows.Err() in benchmark helper.
2026-05-22 18:00:13 -03:00
"exists (select 1 from media_file_artists mfa where mfa.media_file_id = media_file.id and mfa.role = ?)", "artist"),
feat(smartplaylist): add isMissing and isPresent operators (#5436) * feat(smartplaylist): add IsMissing and IsPresent operator types Add two new Expression types for detecting absent/present tags and roles in smart playlist criteria. Includes JSON marshal/unmarshal support and Walk visitor registration. * test(smartplaylist): add JSON marshal/unmarshal tests for isMissing/isPresent * feat(smartplaylist): add SQL generation for isMissing/isPresent operators Tags check json_tree(media_file.tags) for key existence. Roles check json_tree(media_file.participants) for key existence. Regular DB column fields are rejected with an error. * test(smartplaylist): add e2e tests for isMissing/isPresent operators Tests cover tag presence/absence with selective matching (grouping), universal absence (lyricist role), universal presence (composer role), and combined operator usage. * refactor(smartplaylist): use strconv.ParseBool in IsTruthy Replace hand-rolled string truthiness check with strconv.ParseBool, which correctly handles standard boolean strings and rejects unrecognized values as false. * refactor(smartplaylist): clarify missingExpr parameter naming Rename defaultNegate to checkAbsence and extract truthy local for readability. The XNOR logic (checkAbsence == truthy) is now easier to follow: isMissing passes true, isPresent passes false. * refactor(smartplaylist): reuse jsonExpr in missingExpr, improve errors - tagCond/roleCond now handle nil cond (existence-only check) - missingExpr delegates to jsonExpr(info, nil, negate) instead of building SQL manually - Better error messages: unknown fields now report the field name
2026-04-28 19:40:08 -04:00
// isPresent — tags
Entry("isPresent tag [true]", criteria.IsPresent{"genre": true},
"exists (select 1 from json_tree(media_file.tags, '$.genre') where key='value')"),
Entry("isPresent tag [false]", criteria.IsPresent{"genre": false},
"not exists (select 1 from json_tree(media_file.tags, '$.genre') where key='value')"),
// isPresent — roles
Entry("isPresent role [true]", criteria.IsPresent{"composer": true},
fix(smartplaylists): optimize smart playlist performance for role and tag criteria (#5515) * fix(server): optimize smart playlist role queries for large criteria (#5511) Role-based smart playlist criteria (artist, composer, etc.) now query the indexed media_file_artists join table instead of parsing JSON via json_tree() on every row. Multiple conditions for the same role within an OR group are merged into a single EXISTS subquery (batched at 200 to stay under SQLite's expression tree depth limit). A composite index (media_file_id, role) replaces the now-redundant single-column (media_file_id) index on media_file_artists. Benchmark (40k tracks, 500 patterns, 3 artists/track): - Merged join-table: 15ms (9.3x faster) - Merged json_tree: 30ms (4.6x faster) - Unmerged baseline: 137ms * refactor: simplify role condition SQL generation and benchmark Extract shared roleCondSQL/roleExistsSQL helpers to deduplicate the EXISTS template between roleCond and roleCondGroup. Use slices.Chunk for batching per project convention. Extract runBenchQuery helper to eliminate triplicated benchmark execution loop. * chore: raise roleCondBatchSize to 350 The empirical SQLite limit is 496 conditions per merged EXISTS subquery. Raising from 200 to 350 reduces the number of batches (e.g. 500 patterns now splits into 2 batches instead of 3). * fix(server): apply OR-merge optimization to tag conditions too Generalize mergeRoleConds into mergeJsonConds to also collapse multiple tag conditions for the same tag (e.g. genre) within OR groups. This gives the same ~5x speedup for tag-heavy smart playlists as the role optimization gives for artist-heavy ones. * refactor: benchmark uses real criteria pipeline instead of hand-built SQL The "Current" sub-benchmark now builds criteria.Criteria expressions and runs them through the actual newSmartPlaylistCriteria → Where() → ToSql() pipeline, validating the real production code path. The baseline still uses hand-built SQL representing the old json_tree approach. * fix: stabilize merged group ordering and close rows before error check Sort group keys in mergeJsonConds so the merged additions have deterministic order across runs, improving SQLite statement cache reuse. Move rows.Close() before rows.Err() in benchmark helper.
2026-05-22 18:00:13 -03:00
"exists (select 1 from media_file_artists mfa where mfa.media_file_id = media_file.id and mfa.role = ?)", "composer"),
feat(smartplaylist): add isMissing and isPresent operators (#5436) * feat(smartplaylist): add IsMissing and IsPresent operator types Add two new Expression types for detecting absent/present tags and roles in smart playlist criteria. Includes JSON marshal/unmarshal support and Walk visitor registration. * test(smartplaylist): add JSON marshal/unmarshal tests for isMissing/isPresent * feat(smartplaylist): add SQL generation for isMissing/isPresent operators Tags check json_tree(media_file.tags) for key existence. Roles check json_tree(media_file.participants) for key existence. Regular DB column fields are rejected with an error. * test(smartplaylist): add e2e tests for isMissing/isPresent operators Tests cover tag presence/absence with selective matching (grouping), universal absence (lyricist role), universal presence (composer role), and combined operator usage. * refactor(smartplaylist): use strconv.ParseBool in IsTruthy Replace hand-rolled string truthiness check with strconv.ParseBool, which correctly handles standard boolean strings and rejects unrecognized values as false. * refactor(smartplaylist): clarify missingExpr parameter naming Rename defaultNegate to checkAbsence and extract truthy local for readability. The XNOR logic (checkAbsence == truthy) is now easier to follow: isMissing passes true, isPresent passes false. * refactor(smartplaylist): reuse jsonExpr in missingExpr, improve errors - tagCond/roleCond now handle nil cond (existence-only check) - missingExpr delegates to jsonExpr(info, nil, negate) instead of building SQL manually - Better error messages: unknown fields now report the field name
2026-04-28 19:40:08 -04:00
Entry("isPresent role [false]", criteria.IsPresent{"composer": false},
fix(smartplaylists): optimize smart playlist performance for role and tag criteria (#5515) * fix(server): optimize smart playlist role queries for large criteria (#5511) Role-based smart playlist criteria (artist, composer, etc.) now query the indexed media_file_artists join table instead of parsing JSON via json_tree() on every row. Multiple conditions for the same role within an OR group are merged into a single EXISTS subquery (batched at 200 to stay under SQLite's expression tree depth limit). A composite index (media_file_id, role) replaces the now-redundant single-column (media_file_id) index on media_file_artists. Benchmark (40k tracks, 500 patterns, 3 artists/track): - Merged join-table: 15ms (9.3x faster) - Merged json_tree: 30ms (4.6x faster) - Unmerged baseline: 137ms * refactor: simplify role condition SQL generation and benchmark Extract shared roleCondSQL/roleExistsSQL helpers to deduplicate the EXISTS template between roleCond and roleCondGroup. Use slices.Chunk for batching per project convention. Extract runBenchQuery helper to eliminate triplicated benchmark execution loop. * chore: raise roleCondBatchSize to 350 The empirical SQLite limit is 496 conditions per merged EXISTS subquery. Raising from 200 to 350 reduces the number of batches (e.g. 500 patterns now splits into 2 batches instead of 3). * fix(server): apply OR-merge optimization to tag conditions too Generalize mergeRoleConds into mergeJsonConds to also collapse multiple tag conditions for the same tag (e.g. genre) within OR groups. This gives the same ~5x speedup for tag-heavy smart playlists as the role optimization gives for artist-heavy ones. * refactor: benchmark uses real criteria pipeline instead of hand-built SQL The "Current" sub-benchmark now builds criteria.Criteria expressions and runs them through the actual newSmartPlaylistCriteria → Where() → ToSql() pipeline, validating the real production code path. The baseline still uses hand-built SQL representing the old json_tree approach. * fix: stabilize merged group ordering and close rows before error check Sort group keys in mergeJsonConds so the merged additions have deterministic order across runs, improving SQLite statement cache reuse. Move rows.Close() before rows.Err() in benchmark helper.
2026-05-22 18:00:13 -03:00
"not exists (select 1 from media_file_artists mfa where mfa.media_file_id = media_file.id and mfa.role = ?)", "composer"),
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
// isMissing/isPresent — nullable column fields (ReplayGain)
Entry("isMissing rgAlbumGain [true]", criteria.IsMissing{"rgAlbumGain": true},
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
"(media_file.rg_album_gain IS NULL)"),
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
Entry("isMissing rgAlbumGain [false]", criteria.IsMissing{"rgAlbumGain": false},
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
"(media_file.rg_album_gain IS NOT NULL)"),
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
Entry("isPresent rgTrackPeak [true]", criteria.IsPresent{"rgTrackPeak": true},
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
"(media_file.rg_track_peak IS NOT NULL)"),
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
Entry("isPresent rgTrackPeak [false]", criteria.IsPresent{"rgTrackPeak": false},
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
"(media_file.rg_track_peak IS NULL)"),
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
// isMissing — replaygain_* tag-name alias resolves to the nullable column (issue #5584)
Entry("isMissing replaygain_album_gain alias [true]", criteria.IsMissing{"replaygain_album_gain": true},
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
"(media_file.rg_album_gain IS NULL)"),
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
Entry("isPresent replaygain_album_gain alias [true]", criteria.IsPresent{"replaygain_album_gain": true},
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
"(media_file.rg_album_gain IS NOT NULL)"),
// isMissing/isPresent — string column fields (empty string means missing)
Entry("isMissing mbz_recording_id [true]", criteria.IsMissing{"mbz_recording_id": true},
"(media_file.mbz_recording_id IS NULL OR media_file.mbz_recording_id = ?)", ""),
Entry("isMissing mbz_recording_id [false]", criteria.IsMissing{"mbz_recording_id": false},
"(media_file.mbz_recording_id IS NOT NULL AND media_file.mbz_recording_id <> ?)", ""),
Entry("isPresent mbz_album_id [true]", criteria.IsPresent{"mbz_album_id": true},
"(media_file.mbz_album_id IS NOT NULL AND media_file.mbz_album_id <> ?)", ""),
Entry("isPresent mbz_album_id [false]", criteria.IsPresent{"mbz_album_id": false},
"(media_file.mbz_album_id IS NULL OR media_file.mbz_album_id = ?)", ""),
// lyrics: absence is encoded as '' or '[]' (empty serialized LyricList)
Entry("isMissing lyrics [true]", criteria.IsMissing{"lyrics": true},
"(media_file.lyrics IS NULL OR media_file.lyrics = ? OR media_file.lyrics = ?)", "", "[]"),
Entry("isPresent lyrics [true]", criteria.IsPresent{"lyrics": true},
"(media_file.lyrics IS NOT NULL AND media_file.lyrics <> ? AND media_file.lyrics <> ?)", "", "[]"),
Entry("isMissing lyrics [false]", criteria.IsMissing{"lyrics": false},
"(media_file.lyrics IS NOT NULL AND media_file.lyrics <> ? AND media_file.lyrics <> ?)", "", "[]"),
Entry("isPresent lyrics [false]", criteria.IsPresent{"lyrics": false},
"(media_file.lyrics IS NULL OR media_file.lyrics = ? OR media_file.lyrics = ?)", "", "[]"),
// isMissing/isPresent — nullable numeric columns (BPM, BitDepth)
Entry("isMissing bpm [true]", criteria.IsMissing{"bpm": true},
"(media_file.bpm IS NULL)"),
Entry("isPresent bpm [true]", criteria.IsPresent{"bpm": true},
"(media_file.bpm IS NOT NULL)"),
Entry("isMissing bitdepth [true]", criteria.IsMissing{"bitdepth": true},
"(media_file.bit_depth IS NULL)"),
Entry("isPresent bitdepth [false]", criteria.IsPresent{"bitdepth": false},
"(media_file.bit_depth IS NULL)"),
// isMissing/isPresent — more string column fields (empty string means missing)
Entry("isMissing album [true]", criteria.IsMissing{"album": true},
"(media_file.album IS NULL OR media_file.album = ?)", ""),
Entry("isMissing comment [true]", criteria.IsMissing{"comment": true},
"(media_file.comment IS NULL OR media_file.comment = ?)", ""),
Entry("isMissing catalognumber [true]", criteria.IsMissing{"catalognumber": true},
"(media_file.catalog_num IS NULL OR media_file.catalog_num = ?)", ""),
Entry("isMissing discsubtitle [true]", criteria.IsMissing{"discsubtitle": true},
"(media_file.disc_subtitle IS NULL OR media_file.disc_subtitle = ?)", ""),
Entry("isMissing albumcomment [true]", criteria.IsMissing{"albumcomment": true},
"(media_file.mbz_album_comment IS NULL OR media_file.mbz_album_comment = ?)", ""),
Entry("isMissing sorttitle [true]", criteria.IsMissing{"sorttitle": true},
"(media_file.sort_title IS NULL OR media_file.sort_title = ?)", ""),
Entry("isMissing sortalbum [true]", criteria.IsMissing{"sortalbum": true},
"(media_file.sort_album_name IS NULL OR media_file.sort_album_name = ?)", ""),
Entry("isMissing sortartist [true]", criteria.IsMissing{"sortartist": true},
"(media_file.sort_artist_name IS NULL OR media_file.sort_artist_name = ?)", ""),
Entry("isMissing sortalbumartist [true]", criteria.IsMissing{"sortalbumartist": true},
"(media_file.sort_album_artist_name IS NULL OR media_file.sort_album_artist_name = ?)", ""),
Entry("isMissing explicitstatus [true]", criteria.IsMissing{"explicitstatus": true},
"(media_file.explicit_status IS NULL OR media_file.explicit_status = ?)", ""),
Entry("isPresent comment [true]", criteria.IsPresent{"comment": true},
"(media_file.comment IS NOT NULL AND media_file.comment <> ?)", ""),
)
feat(smartplaylists): relax playlist visibility in inPlaylist/notInPlaylist rules (#5411) * test(e2e): add end-to-end tests for smart playlists functionality Signed-off-by: Deluan <deluan@navidrome.org> * fix: enforce playlist visibility in smart playlist InPlaylist/NotInPlaylist rules Previously, the InPlaylist/NotInPlaylist smart playlist criteria only allowed referencing public playlists, regardless of who owned the smart playlist. This was too restrictive for owners referencing their own private playlists and for admins who should have unrestricted access. The fix passes the smart playlist owner's identity and admin status into the criteria SQL builder, so that: admins can reference any playlist, regular users can reference public playlists plus their own private ones, and inaccessible referenced playlists produce a warning instead of a hard error. Also prevents recursive refresh of child playlists the owner cannot access. * test(e2e): clarify user roles and fix playlist visibility tests Renamed testUser/otherUser to adminUser/regularUser to make the admin vs regular user distinction explicit in test code. Fixed three playlist visibility tests that were evaluating as admin (bypassing all access checks) instead of as a regular user, so the public playlist path is now actually exercised. All playlist operator tests now use explicit evaluateRuleAs calls with the appropriate user role. * fix: sync rulesSQL criteria after limitPercent resolution The rulesSQL struct captures a copy of rules at creation time. When limitPercent is resolved later, rules.Limit is updated but rulesSQL still holds the stale value. This caused percentage-based smart playlist limits to be silently ignored. Fix by updating rulesSQL.criteria after the resolution. * refactor: convert inList to a method on smartPlaylistCriteria The inList function already receives ownerID and ownerIsAdmin from the smartPlaylistCriteria caller. Making it a method lets it access those fields directly from the receiver, simplifying the signature and staying consistent with exprSQL which was already converted to a method. * refactor: simplify function signatures by removing type parameters in criteria_sql.go Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>
2026-04-25 14:59:06 -04:00
Describe("playlist permissions", func() {
It("allows public or same-owner playlist references for regular users", func() {
sqlizer, err := newSmartPlaylistCriteria(
criteria.Criteria{Expression: criteria.InPlaylist{"id": "deadbeef-dead-beef"}},
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
withSmartPlaylistOwner(model.User{ID: "owner-id", IsAdmin: false}),
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
).where()
feat(smartplaylists): relax playlist visibility in inPlaylist/notInPlaylist rules (#5411) * test(e2e): add end-to-end tests for smart playlists functionality Signed-off-by: Deluan <deluan@navidrome.org> * fix: enforce playlist visibility in smart playlist InPlaylist/NotInPlaylist rules Previously, the InPlaylist/NotInPlaylist smart playlist criteria only allowed referencing public playlists, regardless of who owned the smart playlist. This was too restrictive for owners referencing their own private playlists and for admins who should have unrestricted access. The fix passes the smart playlist owner's identity and admin status into the criteria SQL builder, so that: admins can reference any playlist, regular users can reference public playlists plus their own private ones, and inaccessible referenced playlists produce a warning instead of a hard error. Also prevents recursive refresh of child playlists the owner cannot access. * test(e2e): clarify user roles and fix playlist visibility tests Renamed testUser/otherUser to adminUser/regularUser to make the admin vs regular user distinction explicit in test code. Fixed three playlist visibility tests that were evaluating as admin (bypassing all access checks) instead of as a regular user, so the public playlist path is now actually exercised. All playlist operator tests now use explicit evaluateRuleAs calls with the appropriate user role. * fix: sync rulesSQL criteria after limitPercent resolution The rulesSQL struct captures a copy of rules at creation time. When limitPercent is resolved later, rules.Limit is updated but rulesSQL still holds the stale value. This caused percentage-based smart playlist limits to be silently ignored. Fix by updating rulesSQL.criteria after the resolution. * refactor: convert inList to a method on smartPlaylistCriteria The inList function already receives ownerID and ownerIsAdmin from the smartPlaylistCriteria caller. Making it a method lets it access those fields directly from the receiver, simplifying the signature and staying consistent with exprSQL which was already converted to a method. * refactor: simplify function signatures by removing type parameters in criteria_sql.go Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>
2026-04-25 14:59:06 -04:00
Expect(err).ToNot(HaveOccurred())
sql, args, err := sqlizer.ToSql()
Expect(err).ToNot(HaveOccurred())
Expect(sql).To(Equal("media_file.id IN (SELECT media_file_id FROM playlist_tracks pl LEFT JOIN playlist on pl.playlist_id = playlist.id WHERE (pl.playlist_id = ? AND (playlist.public = ? OR playlist.owner_id = ?)))"))
Expect(args).To(HaveExactElements("deadbeef-dead-beef", 1, "owner-id"))
})
It("allows all playlist references for admins", func() {
sqlizer, err := newSmartPlaylistCriteria(
criteria.Criteria{Expression: criteria.InPlaylist{"id": "deadbeef-dead-beef"}},
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
withSmartPlaylistOwner(model.User{ID: "admin-id", IsAdmin: true}),
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
).where()
feat(smartplaylists): relax playlist visibility in inPlaylist/notInPlaylist rules (#5411) * test(e2e): add end-to-end tests for smart playlists functionality Signed-off-by: Deluan <deluan@navidrome.org> * fix: enforce playlist visibility in smart playlist InPlaylist/NotInPlaylist rules Previously, the InPlaylist/NotInPlaylist smart playlist criteria only allowed referencing public playlists, regardless of who owned the smart playlist. This was too restrictive for owners referencing their own private playlists and for admins who should have unrestricted access. The fix passes the smart playlist owner's identity and admin status into the criteria SQL builder, so that: admins can reference any playlist, regular users can reference public playlists plus their own private ones, and inaccessible referenced playlists produce a warning instead of a hard error. Also prevents recursive refresh of child playlists the owner cannot access. * test(e2e): clarify user roles and fix playlist visibility tests Renamed testUser/otherUser to adminUser/regularUser to make the admin vs regular user distinction explicit in test code. Fixed three playlist visibility tests that were evaluating as admin (bypassing all access checks) instead of as a regular user, so the public playlist path is now actually exercised. All playlist operator tests now use explicit evaluateRuleAs calls with the appropriate user role. * fix: sync rulesSQL criteria after limitPercent resolution The rulesSQL struct captures a copy of rules at creation time. When limitPercent is resolved later, rules.Limit is updated but rulesSQL still holds the stale value. This caused percentage-based smart playlist limits to be silently ignored. Fix by updating rulesSQL.criteria after the resolution. * refactor: convert inList to a method on smartPlaylistCriteria The inList function already receives ownerID and ownerIsAdmin from the smartPlaylistCriteria caller. Making it a method lets it access those fields directly from the receiver, simplifying the signature and staying consistent with exprSQL which was already converted to a method. * refactor: simplify function signatures by removing type parameters in criteria_sql.go Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>
2026-04-25 14:59:06 -04:00
Expect(err).ToNot(HaveOccurred())
sql, args, err := sqlizer.ToSql()
Expect(err).ToNot(HaveOccurred())
Expect(sql).To(Equal("media_file.id IN (SELECT media_file_id FROM playlist_tracks pl LEFT JOIN playlist on pl.playlist_id = playlist.id WHERE (pl.playlist_id = ?))"))
Expect(args).To(HaveExactElements("deadbeef-dead-beef"))
})
})
It("builds relative date expressions", func() {
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
sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: criteria.InTheLast{"lastPlayed": 30}}).where()
Expect(err).ToNot(HaveOccurred())
sql, args, err := sqlizer.ToSql()
Expect(err).ToNot(HaveOccurred())
Expect(sql).To(Equal("annotation.play_date > ?"))
Expect(args).To(HaveExactElements(startOfPeriod(30, time.Now())))
})
It("builds negated relative date expressions", func() {
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
sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: criteria.NotInTheLast{"lastPlayed": 30}}).where()
Expect(err).ToNot(HaveOccurred())
sql, args, err := sqlizer.ToSql()
Expect(err).ToNot(HaveOccurred())
Expect(sql).To(Equal("(annotation.play_date < ? OR annotation.play_date IS NULL)"))
Expect(args).To(HaveExactElements(startOfPeriod(30, time.Now())))
})
It("returns an error for unknown fields", func() {
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
_, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: criteria.EndsWith{"unknown": "value"}}).where()
Expect(err).To(MatchError("invalid field in criteria: unknown"))
})
feat(smartplaylist): add isMissing and isPresent operators (#5436) * feat(smartplaylist): add IsMissing and IsPresent operator types Add two new Expression types for detecting absent/present tags and roles in smart playlist criteria. Includes JSON marshal/unmarshal support and Walk visitor registration. * test(smartplaylist): add JSON marshal/unmarshal tests for isMissing/isPresent * feat(smartplaylist): add SQL generation for isMissing/isPresent operators Tags check json_tree(media_file.tags) for key existence. Roles check json_tree(media_file.participants) for key existence. Regular DB column fields are rejected with an error. * test(smartplaylist): add e2e tests for isMissing/isPresent operators Tests cover tag presence/absence with selective matching (grouping), universal absence (lyricist role), universal presence (composer role), and combined operator usage. * refactor(smartplaylist): use strconv.ParseBool in IsTruthy Replace hand-rolled string truthiness check with strconv.ParseBool, which correctly handles standard boolean strings and rejects unrecognized values as false. * refactor(smartplaylist): clarify missingExpr parameter naming Rename defaultNegate to checkAbsence and extract truthy local for readability. The XNOR logic (checkAbsence == truthy) is now easier to follow: isMissing passes true, isPresent passes false. * refactor(smartplaylist): reuse jsonExpr in missingExpr, improve errors - tagCond/roleCond now handle nil cond (existence-only check) - missingExpr delegates to jsonExpr(info, nil, negate) instead of building SQL manually - Better error messages: unknown fields now report the field name
2026-04-28 19:40:08 -04:00
It("returns an error when isMissing is used with a regular field", func() {
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
_, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: criteria.IsMissing{"year": true}}).where()
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
Expect(err).To(MatchError(ContainSubstring("isMissing/isPresent operator is not supported for field")))
feat(smartplaylist): add isMissing and isPresent operators (#5436) * feat(smartplaylist): add IsMissing and IsPresent operator types Add two new Expression types for detecting absent/present tags and roles in smart playlist criteria. Includes JSON marshal/unmarshal support and Walk visitor registration. * test(smartplaylist): add JSON marshal/unmarshal tests for isMissing/isPresent * feat(smartplaylist): add SQL generation for isMissing/isPresent operators Tags check json_tree(media_file.tags) for key existence. Roles check json_tree(media_file.participants) for key existence. Regular DB column fields are rejected with an error. * test(smartplaylist): add e2e tests for isMissing/isPresent operators Tests cover tag presence/absence with selective matching (grouping), universal absence (lyricist role), universal presence (composer role), and combined operator usage. * refactor(smartplaylist): use strconv.ParseBool in IsTruthy Replace hand-rolled string truthiness check with strconv.ParseBool, which correctly handles standard boolean strings and rejects unrecognized values as false. * refactor(smartplaylist): clarify missingExpr parameter naming Rename defaultNegate to checkAbsence and extract truthy local for readability. The XNOR logic (checkAbsence == truthy) is now easier to follow: isMissing passes true, isPresent passes false. * refactor(smartplaylist): reuse jsonExpr in missingExpr, improve errors - tagCond/roleCond now handle nil cond (existence-only check) - missingExpr delegates to jsonExpr(info, nil, negate) instead of building SQL manually - Better error messages: unknown fields now report the field name
2026-04-28 19:40:08 -04:00
})
It("returns an error when isPresent is used with a regular field", func() {
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
_, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: criteria.IsPresent{"title": true}}).where()
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
Expect(err).To(MatchError(ContainSubstring("isMissing/isPresent operator is not supported for field")))
feat(smartplaylist): add isMissing and isPresent operators (#5436) * feat(smartplaylist): add IsMissing and IsPresent operator types Add two new Expression types for detecting absent/present tags and roles in smart playlist criteria. Includes JSON marshal/unmarshal support and Walk visitor registration. * test(smartplaylist): add JSON marshal/unmarshal tests for isMissing/isPresent * feat(smartplaylist): add SQL generation for isMissing/isPresent operators Tags check json_tree(media_file.tags) for key existence. Roles check json_tree(media_file.participants) for key existence. Regular DB column fields are rejected with an error. * test(smartplaylist): add e2e tests for isMissing/isPresent operators Tests cover tag presence/absence with selective matching (grouping), universal absence (lyricist role), universal presence (composer role), and combined operator usage. * refactor(smartplaylist): use strconv.ParseBool in IsTruthy Replace hand-rolled string truthiness check with strconv.ParseBool, which correctly handles standard boolean strings and rejects unrecognized values as false. * refactor(smartplaylist): clarify missingExpr parameter naming Rename defaultNegate to checkAbsence and extract truthy local for readability. The XNOR logic (checkAbsence == truthy) is now easier to follow: isMissing passes true, isPresent passes false. * refactor(smartplaylist): reuse jsonExpr in missingExpr, improve errors - tagCond/roleCond now handle nil cond (existence-only check) - missingExpr delegates to jsonExpr(info, nil, negate) instead of building SQL manually - Better error messages: unknown fields now report the field name
2026-04-28 19:40:08 -04:00
})
It("returns an error when isMissing has a non-boolean value", func() {
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
_, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: criteria.IsMissing{"genre": "hello"}}).where()
Expect(err).To(MatchError(ContainSubstring("invalid boolean value for 'missing' expression")))
})
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
It("returns an error when inPlaylist has empty path", func() {
_, err := newSmartPlaylistCriteria(
criteria.Criteria{Expression: criteria.InPlaylist{"path": ""}},
withSmartPlaylistOwner(model.User{ID: "owner-id", IsAdmin: false})).where()
Expect(err).To(MatchError(ContainSubstring("playlist id or path not given")))
})
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
It("returns an error for a range over a tag/role field", func() {
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
_, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: criteria.InTheRange{"rate": []int{1, 5}}}).where()
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
Expect(err).To(MatchError(ContainSubstring("range operator not supported for tag/role field")))
})
It("returns a clear error for a malformed range value", func() {
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
_, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: criteria.InTheRange{"playCount": []int{1, 2, 3}}}).where()
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
Expect(err).To(MatchError(ContainSubstring("must be a [min, max] pair")))
})
Describe("sort", func() {
It("sorts by regular fields", func() {
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
Expect(newSmartPlaylistCriteria(criteria.Criteria{Sort: "title"}).orderBy()).To(Equal("media_file.title asc"))
})
It("sorts by tag fields", func() {
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
Expect(newSmartPlaylistCriteria(criteria.Criteria{Sort: "genre"}).orderBy()).To(Equal("COALESCE(json_extract(media_file.tags, '$.genre[0].value'), '') asc"))
})
It("sorts by role fields", func() {
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
Expect(newSmartPlaylistCriteria(criteria.Criteria{Sort: "artist"}).orderBy()).To(Equal("COALESCE(json_extract(media_file.participants, '$.artist[0].name'), '') asc"))
})
It("casts numeric tags when sorting", func() {
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
Expect(newSmartPlaylistCriteria(criteria.Criteria{Sort: "rate"}).orderBy()).To(Equal("CAST(COALESCE(json_extract(media_file.tags, '$.rate[0].value'), '') AS REAL) asc"))
})
It("sorts by albumtype alias", func() {
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
Expect(newSmartPlaylistCriteria(criteria.Criteria{Sort: "albumtype"}).orderBy()).To(Equal("COALESCE(json_extract(media_file.tags, '$.releasetype[0].value'), '') asc"))
})
It("sorts by random", func() {
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
Expect(newSmartPlaylistCriteria(criteria.Criteria{Sort: "random"}).orderBy()).To(Equal("random() asc"))
})
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
It("sorts by album columns bare, with no COALESCE default", func() {
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
Expect(newSmartPlaylistCriteria(criteria.Criteria{Sort: "-albumDateAdded,trackNumber"}).orderBy()).
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
To(Equal("album.created_at desc, media_file.track_number asc"))
})
It("sorts by multiple fields", func() {
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
Expect(newSmartPlaylistCriteria(criteria.Criteria{Sort: "title,-rating"}).orderBy()).To(Equal("media_file.title asc, COALESCE(annotation.rating, 0) desc"))
})
It("reverts order when order is desc", func() {
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
Expect(newSmartPlaylistCriteria(criteria.Criteria{Sort: "-date,artist", Order: "desc"}).orderBy()).To(Equal("media_file.date asc, COALESCE(json_extract(media_file.participants, '$.artist[0].name'), '') desc"))
})
It("ignores invalid sort fields", func() {
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
Expect(newSmartPlaylistCriteria(criteria.Criteria{Sort: "bogus,title"}).orderBy()).To(Equal("media_file.title asc"))
})
})
It("has SQL mappings for all non-tag/non-role criteria fields", func() {
for _, name := range criteria.AllFieldNames() {
info, ok := criteria.LookupField(name)
Expect(ok).To(BeTrue(), "field %q registered but LookupField fails", name)
if info.IsTag || info.IsRole {
continue
}
_, hasSQLField := smartPlaylistFields[info.Name()]
Expect(hasSQLField).To(BeTrue(), "criteria field %q (name=%q) has no entry in smartPlaylistFields", name, info.Name())
}
})
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
It("declares a joinType matching the table each field selects from", func() {
// Omitting the joinType still compiles, so without this the field would only fail at
// refresh time with "no such column".
joinByTable := map[string]smartPlaylistJoinType{
"media_file": smartPlaylistJoinNone,
"annotation": smartPlaylistJoinNone,
"album": smartPlaylistJoinAlbum,
"album_annotation": smartPlaylistJoinAlbumAnnotation,
"artist_annotation": smartPlaylistJoinArtistAnnotation,
}
for name, field := range smartPlaylistFields {
if field.expr == "" {
continue
}
table, _, ok := strings.Cut(field.expr, ".")
Expect(ok).To(BeTrue(), "field %q has expr %q with no table prefix", name, field.expr)
want, known := joinByTable[table]
Expect(known).To(BeTrue(), "field %q selects from unknown table %q", name, table)
Expect(field.joinType).To(Equal(want), "field %q selects from %q but declares the wrong joinType", name, table)
}
})
fix(smartplaylists): optimize smart playlist performance for role and tag criteria (#5515) * fix(server): optimize smart playlist role queries for large criteria (#5511) Role-based smart playlist criteria (artist, composer, etc.) now query the indexed media_file_artists join table instead of parsing JSON via json_tree() on every row. Multiple conditions for the same role within an OR group are merged into a single EXISTS subquery (batched at 200 to stay under SQLite's expression tree depth limit). A composite index (media_file_id, role) replaces the now-redundant single-column (media_file_id) index on media_file_artists. Benchmark (40k tracks, 500 patterns, 3 artists/track): - Merged join-table: 15ms (9.3x faster) - Merged json_tree: 30ms (4.6x faster) - Unmerged baseline: 137ms * refactor: simplify role condition SQL generation and benchmark Extract shared roleCondSQL/roleExistsSQL helpers to deduplicate the EXISTS template between roleCond and roleCondGroup. Use slices.Chunk for batching per project convention. Extract runBenchQuery helper to eliminate triplicated benchmark execution loop. * chore: raise roleCondBatchSize to 350 The empirical SQLite limit is 496 conditions per merged EXISTS subquery. Raising from 200 to 350 reduces the number of batches (e.g. 500 patterns now splits into 2 batches instead of 3). * fix(server): apply OR-merge optimization to tag conditions too Generalize mergeRoleConds into mergeJsonConds to also collapse multiple tag conditions for the same tag (e.g. genre) within OR groups. This gives the same ~5x speedup for tag-heavy smart playlists as the role optimization gives for artist-heavy ones. * refactor: benchmark uses real criteria pipeline instead of hand-built SQL The "Current" sub-benchmark now builds criteria.Criteria expressions and runs them through the actual newSmartPlaylistCriteria → Where() → ToSql() pipeline, validating the real production code path. The baseline still uses hand-built SQL representing the old json_tree approach. * fix: stabilize merged group ordering and close rows before error check Sort group keys in mergeJsonConds so the merged additions have deterministic order across runs, improving SQLite statement cache reuse. Move rows.Close() before rows.Err() in benchmark helper.
2026-05-22 18:00:13 -03:00
Describe("JSON condition merging", func() {
It("merges multiple role conditions in an OR group into a single EXISTS", func() {
expr := criteria.Any{
criteria.Contains{"artist": "Beatles"},
criteria.Contains{"artist": "Kraftwerk"},
criteria.Contains{"artist": "Pink Floyd"},
}
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
sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).where()
fix(smartplaylists): optimize smart playlist performance for role and tag criteria (#5515) * fix(server): optimize smart playlist role queries for large criteria (#5511) Role-based smart playlist criteria (artist, composer, etc.) now query the indexed media_file_artists join table instead of parsing JSON via json_tree() on every row. Multiple conditions for the same role within an OR group are merged into a single EXISTS subquery (batched at 200 to stay under SQLite's expression tree depth limit). A composite index (media_file_id, role) replaces the now-redundant single-column (media_file_id) index on media_file_artists. Benchmark (40k tracks, 500 patterns, 3 artists/track): - Merged join-table: 15ms (9.3x faster) - Merged json_tree: 30ms (4.6x faster) - Unmerged baseline: 137ms * refactor: simplify role condition SQL generation and benchmark Extract shared roleCondSQL/roleExistsSQL helpers to deduplicate the EXISTS template between roleCond and roleCondGroup. Use slices.Chunk for batching per project convention. Extract runBenchQuery helper to eliminate triplicated benchmark execution loop. * chore: raise roleCondBatchSize to 350 The empirical SQLite limit is 496 conditions per merged EXISTS subquery. Raising from 200 to 350 reduces the number of batches (e.g. 500 patterns now splits into 2 batches instead of 3). * fix(server): apply OR-merge optimization to tag conditions too Generalize mergeRoleConds into mergeJsonConds to also collapse multiple tag conditions for the same tag (e.g. genre) within OR groups. This gives the same ~5x speedup for tag-heavy smart playlists as the role optimization gives for artist-heavy ones. * refactor: benchmark uses real criteria pipeline instead of hand-built SQL The "Current" sub-benchmark now builds criteria.Criteria expressions and runs them through the actual newSmartPlaylistCriteria → Where() → ToSql() pipeline, validating the real production code path. The baseline still uses hand-built SQL representing the old json_tree approach. * fix: stabilize merged group ordering and close rows before error check Sort group keys in mergeJsonConds so the merged additions have deterministic order across runs, improving SQLite statement cache reuse. Move rows.Close() before rows.Err() in benchmark helper.
2026-05-22 18:00:13 -03:00
Expect(err).ToNot(HaveOccurred())
sql, args, err := sqlizer.ToSql()
Expect(err).ToNot(HaveOccurred())
Expect(sql).To(Equal("(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 (artist.name LIKE ? OR artist.name LIKE ? OR artist.name LIKE ?)))"))
Expect(args).To(HaveExactElements("artist", "%Beatles%", "%Kraftwerk%", "%Pink Floyd%"))
})
It("does not merge role conditions from different roles", func() {
expr := criteria.Any{
criteria.Contains{"artist": "Beatles"},
criteria.Contains{"composer": "Lennon"},
}
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
sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).where()
fix(smartplaylists): optimize smart playlist performance for role and tag criteria (#5515) * fix(server): optimize smart playlist role queries for large criteria (#5511) Role-based smart playlist criteria (artist, composer, etc.) now query the indexed media_file_artists join table instead of parsing JSON via json_tree() on every row. Multiple conditions for the same role within an OR group are merged into a single EXISTS subquery (batched at 200 to stay under SQLite's expression tree depth limit). A composite index (media_file_id, role) replaces the now-redundant single-column (media_file_id) index on media_file_artists. Benchmark (40k tracks, 500 patterns, 3 artists/track): - Merged join-table: 15ms (9.3x faster) - Merged json_tree: 30ms (4.6x faster) - Unmerged baseline: 137ms * refactor: simplify role condition SQL generation and benchmark Extract shared roleCondSQL/roleExistsSQL helpers to deduplicate the EXISTS template between roleCond and roleCondGroup. Use slices.Chunk for batching per project convention. Extract runBenchQuery helper to eliminate triplicated benchmark execution loop. * chore: raise roleCondBatchSize to 350 The empirical SQLite limit is 496 conditions per merged EXISTS subquery. Raising from 200 to 350 reduces the number of batches (e.g. 500 patterns now splits into 2 batches instead of 3). * fix(server): apply OR-merge optimization to tag conditions too Generalize mergeRoleConds into mergeJsonConds to also collapse multiple tag conditions for the same tag (e.g. genre) within OR groups. This gives the same ~5x speedup for tag-heavy smart playlists as the role optimization gives for artist-heavy ones. * refactor: benchmark uses real criteria pipeline instead of hand-built SQL The "Current" sub-benchmark now builds criteria.Criteria expressions and runs them through the actual newSmartPlaylistCriteria → Where() → ToSql() pipeline, validating the real production code path. The baseline still uses hand-built SQL representing the old json_tree approach. * fix: stabilize merged group ordering and close rows before error check Sort group keys in mergeJsonConds so the merged additions have deterministic order across runs, improving SQLite statement cache reuse. Move rows.Close() before rows.Err() in benchmark helper.
2026-05-22 18:00:13 -03:00
Expect(err).ToNot(HaveOccurred())
sql, _, err := sqlizer.ToSql()
Expect(err).ToNot(HaveOccurred())
Expect(sql).To(ContainSubstring("mfa.role = ?"))
// Two separate EXISTS since roles differ
Expect(strings.Count(sql, "exists")).To(Equal(2))
})
It("does not merge negated role conditions", func() {
expr := criteria.Any{
criteria.NotContains{"artist": "Beatles"},
criteria.NotContains{"artist": "Kraftwerk"},
}
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
sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).where()
fix(smartplaylists): optimize smart playlist performance for role and tag criteria (#5515) * fix(server): optimize smart playlist role queries for large criteria (#5511) Role-based smart playlist criteria (artist, composer, etc.) now query the indexed media_file_artists join table instead of parsing JSON via json_tree() on every row. Multiple conditions for the same role within an OR group are merged into a single EXISTS subquery (batched at 200 to stay under SQLite's expression tree depth limit). A composite index (media_file_id, role) replaces the now-redundant single-column (media_file_id) index on media_file_artists. Benchmark (40k tracks, 500 patterns, 3 artists/track): - Merged join-table: 15ms (9.3x faster) - Merged json_tree: 30ms (4.6x faster) - Unmerged baseline: 137ms * refactor: simplify role condition SQL generation and benchmark Extract shared roleCondSQL/roleExistsSQL helpers to deduplicate the EXISTS template between roleCond and roleCondGroup. Use slices.Chunk for batching per project convention. Extract runBenchQuery helper to eliminate triplicated benchmark execution loop. * chore: raise roleCondBatchSize to 350 The empirical SQLite limit is 496 conditions per merged EXISTS subquery. Raising from 200 to 350 reduces the number of batches (e.g. 500 patterns now splits into 2 batches instead of 3). * fix(server): apply OR-merge optimization to tag conditions too Generalize mergeRoleConds into mergeJsonConds to also collapse multiple tag conditions for the same tag (e.g. genre) within OR groups. This gives the same ~5x speedup for tag-heavy smart playlists as the role optimization gives for artist-heavy ones. * refactor: benchmark uses real criteria pipeline instead of hand-built SQL The "Current" sub-benchmark now builds criteria.Criteria expressions and runs them through the actual newSmartPlaylistCriteria → Where() → ToSql() pipeline, validating the real production code path. The baseline still uses hand-built SQL representing the old json_tree approach. * fix: stabilize merged group ordering and close rows before error check Sort group keys in mergeJsonConds so the merged additions have deterministic order across runs, improving SQLite statement cache reuse. Move rows.Close() before rows.Err() in benchmark helper.
2026-05-22 18:00:13 -03:00
Expect(err).ToNot(HaveOccurred())
sql, _, err := sqlizer.ToSql()
Expect(err).ToNot(HaveOccurred())
// Two separate "not exists" since they are negated
Expect(strings.Count(sql, "not exists")).To(Equal(2))
})
It("batches large groups to avoid SQLite expression tree depth limit", func() {
// Create jsonCondBatchSize + 1 conditions to trigger batching into 2 groups
anyExprs := make(criteria.Any, jsonCondBatchSize+1)
for i := range anyExprs {
anyExprs[i] = criteria.Contains{"artist": fmt.Sprintf("Artist%d", i)}
}
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
sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: anyExprs}).where()
fix(smartplaylists): optimize smart playlist performance for role and tag criteria (#5515) * fix(server): optimize smart playlist role queries for large criteria (#5511) Role-based smart playlist criteria (artist, composer, etc.) now query the indexed media_file_artists join table instead of parsing JSON via json_tree() on every row. Multiple conditions for the same role within an OR group are merged into a single EXISTS subquery (batched at 200 to stay under SQLite's expression tree depth limit). A composite index (media_file_id, role) replaces the now-redundant single-column (media_file_id) index on media_file_artists. Benchmark (40k tracks, 500 patterns, 3 artists/track): - Merged join-table: 15ms (9.3x faster) - Merged json_tree: 30ms (4.6x faster) - Unmerged baseline: 137ms * refactor: simplify role condition SQL generation and benchmark Extract shared roleCondSQL/roleExistsSQL helpers to deduplicate the EXISTS template between roleCond and roleCondGroup. Use slices.Chunk for batching per project convention. Extract runBenchQuery helper to eliminate triplicated benchmark execution loop. * chore: raise roleCondBatchSize to 350 The empirical SQLite limit is 496 conditions per merged EXISTS subquery. Raising from 200 to 350 reduces the number of batches (e.g. 500 patterns now splits into 2 batches instead of 3). * fix(server): apply OR-merge optimization to tag conditions too Generalize mergeRoleConds into mergeJsonConds to also collapse multiple tag conditions for the same tag (e.g. genre) within OR groups. This gives the same ~5x speedup for tag-heavy smart playlists as the role optimization gives for artist-heavy ones. * refactor: benchmark uses real criteria pipeline instead of hand-built SQL The "Current" sub-benchmark now builds criteria.Criteria expressions and runs them through the actual newSmartPlaylistCriteria → Where() → ToSql() pipeline, validating the real production code path. The baseline still uses hand-built SQL representing the old json_tree approach. * fix: stabilize merged group ordering and close rows before error check Sort group keys in mergeJsonConds so the merged additions have deterministic order across runs, improving SQLite statement cache reuse. Move rows.Close() before rows.Err() in benchmark helper.
2026-05-22 18:00:13 -03:00
Expect(err).ToNot(HaveOccurred())
sql, args, err := sqlizer.ToSql()
Expect(err).ToNot(HaveOccurred())
// Should produce 2 EXISTS subqueries (one batch of jsonCondBatchSize, one of 1)
Expect(strings.Count(sql, "exists")).To(Equal(2))
// First batch has jsonCondBatchSize patterns, second has 1 => total args:
// 2 roles + (jsonCondBatchSize + 1) patterns
Expect(args).To(HaveLen(2 + jsonCondBatchSize + 1))
})
It("merges role conditions while preserving non-role conditions", func() {
expr := criteria.Any{
criteria.Contains{"title": "Love"},
criteria.Contains{"artist": "Beatles"},
criteria.Contains{"artist": "Kraftwerk"},
}
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
sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).where()
fix(smartplaylists): optimize smart playlist performance for role and tag criteria (#5515) * fix(server): optimize smart playlist role queries for large criteria (#5511) Role-based smart playlist criteria (artist, composer, etc.) now query the indexed media_file_artists join table instead of parsing JSON via json_tree() on every row. Multiple conditions for the same role within an OR group are merged into a single EXISTS subquery (batched at 200 to stay under SQLite's expression tree depth limit). A composite index (media_file_id, role) replaces the now-redundant single-column (media_file_id) index on media_file_artists. Benchmark (40k tracks, 500 patterns, 3 artists/track): - Merged join-table: 15ms (9.3x faster) - Merged json_tree: 30ms (4.6x faster) - Unmerged baseline: 137ms * refactor: simplify role condition SQL generation and benchmark Extract shared roleCondSQL/roleExistsSQL helpers to deduplicate the EXISTS template between roleCond and roleCondGroup. Use slices.Chunk for batching per project convention. Extract runBenchQuery helper to eliminate triplicated benchmark execution loop. * chore: raise roleCondBatchSize to 350 The empirical SQLite limit is 496 conditions per merged EXISTS subquery. Raising from 200 to 350 reduces the number of batches (e.g. 500 patterns now splits into 2 batches instead of 3). * fix(server): apply OR-merge optimization to tag conditions too Generalize mergeRoleConds into mergeJsonConds to also collapse multiple tag conditions for the same tag (e.g. genre) within OR groups. This gives the same ~5x speedup for tag-heavy smart playlists as the role optimization gives for artist-heavy ones. * refactor: benchmark uses real criteria pipeline instead of hand-built SQL The "Current" sub-benchmark now builds criteria.Criteria expressions and runs them through the actual newSmartPlaylistCriteria → Where() → ToSql() pipeline, validating the real production code path. The baseline still uses hand-built SQL representing the old json_tree approach. * fix: stabilize merged group ordering and close rows before error check Sort group keys in mergeJsonConds so the merged additions have deterministic order across runs, improving SQLite statement cache reuse. Move rows.Close() before rows.Err() in benchmark helper.
2026-05-22 18:00:13 -03:00
Expect(err).ToNot(HaveOccurred())
sql, args, err := sqlizer.ToSql()
Expect(err).ToNot(HaveOccurred())
Expect(sql).To(ContainSubstring("media_file.title LIKE ?"))
Expect(sql).To(ContainSubstring("artist.name LIKE ? OR artist.name LIKE ?"))
Expect(args).To(HaveExactElements("%Love%", "artist", "%Beatles%", "%Kraftwerk%"))
})
It("merges multiple tag conditions in an OR group into a single EXISTS", func() {
expr := criteria.Any{
criteria.Contains{"genre": "Rock"},
criteria.Contains{"genre": "Metal"},
criteria.Contains{"genre": "Punk"},
}
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
sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).where()
fix(smartplaylists): optimize smart playlist performance for role and tag criteria (#5515) * fix(server): optimize smart playlist role queries for large criteria (#5511) Role-based smart playlist criteria (artist, composer, etc.) now query the indexed media_file_artists join table instead of parsing JSON via json_tree() on every row. Multiple conditions for the same role within an OR group are merged into a single EXISTS subquery (batched at 200 to stay under SQLite's expression tree depth limit). A composite index (media_file_id, role) replaces the now-redundant single-column (media_file_id) index on media_file_artists. Benchmark (40k tracks, 500 patterns, 3 artists/track): - Merged join-table: 15ms (9.3x faster) - Merged json_tree: 30ms (4.6x faster) - Unmerged baseline: 137ms * refactor: simplify role condition SQL generation and benchmark Extract shared roleCondSQL/roleExistsSQL helpers to deduplicate the EXISTS template between roleCond and roleCondGroup. Use slices.Chunk for batching per project convention. Extract runBenchQuery helper to eliminate triplicated benchmark execution loop. * chore: raise roleCondBatchSize to 350 The empirical SQLite limit is 496 conditions per merged EXISTS subquery. Raising from 200 to 350 reduces the number of batches (e.g. 500 patterns now splits into 2 batches instead of 3). * fix(server): apply OR-merge optimization to tag conditions too Generalize mergeRoleConds into mergeJsonConds to also collapse multiple tag conditions for the same tag (e.g. genre) within OR groups. This gives the same ~5x speedup for tag-heavy smart playlists as the role optimization gives for artist-heavy ones. * refactor: benchmark uses real criteria pipeline instead of hand-built SQL The "Current" sub-benchmark now builds criteria.Criteria expressions and runs them through the actual newSmartPlaylistCriteria → Where() → ToSql() pipeline, validating the real production code path. The baseline still uses hand-built SQL representing the old json_tree approach. * fix: stabilize merged group ordering and close rows before error check Sort group keys in mergeJsonConds so the merged additions have deterministic order across runs, improving SQLite statement cache reuse. Move rows.Close() before rows.Err() in benchmark helper.
2026-05-22 18:00:13 -03:00
Expect(err).ToNot(HaveOccurred())
sql, args, err := sqlizer.ToSql()
Expect(err).ToNot(HaveOccurred())
Expect(sql).To(Equal("(exists (select 1 from json_tree(media_file.tags, '$.genre') where key='value' and (value LIKE ? OR value LIKE ? OR value LIKE ?)))"))
Expect(args).To(HaveExactElements("%Rock%", "%Metal%", "%Punk%"))
})
It("does not merge tag conditions from different tags", func() {
expr := criteria.Any{
criteria.Contains{"genre": "Rock"},
criteria.Contains{"mood": "Happy"},
}
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
sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).where()
fix(smartplaylists): optimize smart playlist performance for role and tag criteria (#5515) * fix(server): optimize smart playlist role queries for large criteria (#5511) Role-based smart playlist criteria (artist, composer, etc.) now query the indexed media_file_artists join table instead of parsing JSON via json_tree() on every row. Multiple conditions for the same role within an OR group are merged into a single EXISTS subquery (batched at 200 to stay under SQLite's expression tree depth limit). A composite index (media_file_id, role) replaces the now-redundant single-column (media_file_id) index on media_file_artists. Benchmark (40k tracks, 500 patterns, 3 artists/track): - Merged join-table: 15ms (9.3x faster) - Merged json_tree: 30ms (4.6x faster) - Unmerged baseline: 137ms * refactor: simplify role condition SQL generation and benchmark Extract shared roleCondSQL/roleExistsSQL helpers to deduplicate the EXISTS template between roleCond and roleCondGroup. Use slices.Chunk for batching per project convention. Extract runBenchQuery helper to eliminate triplicated benchmark execution loop. * chore: raise roleCondBatchSize to 350 The empirical SQLite limit is 496 conditions per merged EXISTS subquery. Raising from 200 to 350 reduces the number of batches (e.g. 500 patterns now splits into 2 batches instead of 3). * fix(server): apply OR-merge optimization to tag conditions too Generalize mergeRoleConds into mergeJsonConds to also collapse multiple tag conditions for the same tag (e.g. genre) within OR groups. This gives the same ~5x speedup for tag-heavy smart playlists as the role optimization gives for artist-heavy ones. * refactor: benchmark uses real criteria pipeline instead of hand-built SQL The "Current" sub-benchmark now builds criteria.Criteria expressions and runs them through the actual newSmartPlaylistCriteria → Where() → ToSql() pipeline, validating the real production code path. The baseline still uses hand-built SQL representing the old json_tree approach. * fix: stabilize merged group ordering and close rows before error check Sort group keys in mergeJsonConds so the merged additions have deterministic order across runs, improving SQLite statement cache reuse. Move rows.Close() before rows.Err() in benchmark helper.
2026-05-22 18:00:13 -03:00
Expect(err).ToNot(HaveOccurred())
sql, _, err := sqlizer.ToSql()
Expect(err).ToNot(HaveOccurred())
Expect(strings.Count(sql, "exists")).To(Equal(2))
})
It("does not merge negated tag conditions", func() {
expr := criteria.Any{
criteria.NotContains{"genre": "Rock"},
criteria.NotContains{"genre": "Metal"},
}
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
sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).where()
fix(smartplaylists): optimize smart playlist performance for role and tag criteria (#5515) * fix(server): optimize smart playlist role queries for large criteria (#5511) Role-based smart playlist criteria (artist, composer, etc.) now query the indexed media_file_artists join table instead of parsing JSON via json_tree() on every row. Multiple conditions for the same role within an OR group are merged into a single EXISTS subquery (batched at 200 to stay under SQLite's expression tree depth limit). A composite index (media_file_id, role) replaces the now-redundant single-column (media_file_id) index on media_file_artists. Benchmark (40k tracks, 500 patterns, 3 artists/track): - Merged join-table: 15ms (9.3x faster) - Merged json_tree: 30ms (4.6x faster) - Unmerged baseline: 137ms * refactor: simplify role condition SQL generation and benchmark Extract shared roleCondSQL/roleExistsSQL helpers to deduplicate the EXISTS template between roleCond and roleCondGroup. Use slices.Chunk for batching per project convention. Extract runBenchQuery helper to eliminate triplicated benchmark execution loop. * chore: raise roleCondBatchSize to 350 The empirical SQLite limit is 496 conditions per merged EXISTS subquery. Raising from 200 to 350 reduces the number of batches (e.g. 500 patterns now splits into 2 batches instead of 3). * fix(server): apply OR-merge optimization to tag conditions too Generalize mergeRoleConds into mergeJsonConds to also collapse multiple tag conditions for the same tag (e.g. genre) within OR groups. This gives the same ~5x speedup for tag-heavy smart playlists as the role optimization gives for artist-heavy ones. * refactor: benchmark uses real criteria pipeline instead of hand-built SQL The "Current" sub-benchmark now builds criteria.Criteria expressions and runs them through the actual newSmartPlaylistCriteria → Where() → ToSql() pipeline, validating the real production code path. The baseline still uses hand-built SQL representing the old json_tree approach. * fix: stabilize merged group ordering and close rows before error check Sort group keys in mergeJsonConds so the merged additions have deterministic order across runs, improving SQLite statement cache reuse. Move rows.Close() before rows.Err() in benchmark helper.
2026-05-22 18:00:13 -03:00
Expect(err).ToNot(HaveOccurred())
sql, _, err := sqlizer.ToSql()
Expect(err).ToNot(HaveOccurred())
Expect(strings.Count(sql, "not exists")).To(Equal(2))
})
It("merges role and tag conditions independently", func() {
expr := criteria.Any{
criteria.Contains{"artist": "Beatles"},
criteria.Contains{"artist": "Kraftwerk"},
criteria.Contains{"genre": "Rock"},
criteria.Contains{"genre": "Metal"},
}
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
sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).where()
fix(smartplaylists): optimize smart playlist performance for role and tag criteria (#5515) * fix(server): optimize smart playlist role queries for large criteria (#5511) Role-based smart playlist criteria (artist, composer, etc.) now query the indexed media_file_artists join table instead of parsing JSON via json_tree() on every row. Multiple conditions for the same role within an OR group are merged into a single EXISTS subquery (batched at 200 to stay under SQLite's expression tree depth limit). A composite index (media_file_id, role) replaces the now-redundant single-column (media_file_id) index on media_file_artists. Benchmark (40k tracks, 500 patterns, 3 artists/track): - Merged join-table: 15ms (9.3x faster) - Merged json_tree: 30ms (4.6x faster) - Unmerged baseline: 137ms * refactor: simplify role condition SQL generation and benchmark Extract shared roleCondSQL/roleExistsSQL helpers to deduplicate the EXISTS template between roleCond and roleCondGroup. Use slices.Chunk for batching per project convention. Extract runBenchQuery helper to eliminate triplicated benchmark execution loop. * chore: raise roleCondBatchSize to 350 The empirical SQLite limit is 496 conditions per merged EXISTS subquery. Raising from 200 to 350 reduces the number of batches (e.g. 500 patterns now splits into 2 batches instead of 3). * fix(server): apply OR-merge optimization to tag conditions too Generalize mergeRoleConds into mergeJsonConds to also collapse multiple tag conditions for the same tag (e.g. genre) within OR groups. This gives the same ~5x speedup for tag-heavy smart playlists as the role optimization gives for artist-heavy ones. * refactor: benchmark uses real criteria pipeline instead of hand-built SQL The "Current" sub-benchmark now builds criteria.Criteria expressions and runs them through the actual newSmartPlaylistCriteria → Where() → ToSql() pipeline, validating the real production code path. The baseline still uses hand-built SQL representing the old json_tree approach. * fix: stabilize merged group ordering and close rows before error check Sort group keys in mergeJsonConds so the merged additions have deterministic order across runs, improving SQLite statement cache reuse. Move rows.Close() before rows.Err() in benchmark helper.
2026-05-22 18:00:13 -03:00
Expect(err).ToNot(HaveOccurred())
sql, args, err := sqlizer.ToSql()
Expect(err).ToNot(HaveOccurred())
// Two merged EXISTS: one for roles, one for tags
Expect(strings.Count(sql, "exists")).To(Equal(2))
Expect(sql).To(ContainSubstring("artist.name LIKE ? OR artist.name LIKE ?"))
Expect(sql).To(ContainSubstring("value LIKE ? OR value LIKE ?"))
Expect(args).To(HaveLen(2 + 2 + 1)) // 2 tag patterns + 2 role patterns + 1 role name
})
It("merges negated role conditions in an AND group into a single NOT EXISTS", func() {
expr := criteria.All{
criteria.IsNot{"artist": "Beatles"},
criteria.IsNot{"artist": "Kraftwerk"},
}
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
sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).where()
Expect(err).ToNot(HaveOccurred())
sql, args, err := sqlizer.ToSql()
Expect(err).ToNot(HaveOccurred())
// A single NOT EXISTS with both names ORed inside (De Morgan)
Expect(strings.Count(sql, "not exists")).To(Equal(1))
Expect(sql).To(ContainSubstring("artist.name = ? OR artist.name = ?"))
Expect(args).To(HaveExactElements("artist", "Beatles", "Kraftwerk"))
})
It("merges negated notContains role conditions in an AND group", func() {
expr := criteria.All{
criteria.NotContains{"artist": "Beatles"},
criteria.NotContains{"artist": "Kraftwerk"},
}
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
sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).where()
Expect(err).ToNot(HaveOccurred())
sql, args, err := sqlizer.ToSql()
Expect(err).ToNot(HaveOccurred())
Expect(strings.Count(sql, "not exists")).To(Equal(1))
Expect(sql).To(ContainSubstring("artist.name LIKE ? OR artist.name LIKE ?"))
Expect(args).To(HaveExactElements("artist", "%Beatles%", "%Kraftwerk%"))
})
It("merges negated tag conditions in an AND group into a single NOT EXISTS", func() {
expr := criteria.All{
criteria.NotContains{"genre": "Rock"},
criteria.NotContains{"genre": "Metal"},
}
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
sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).where()
Expect(err).ToNot(HaveOccurred())
sql, args, err := sqlizer.ToSql()
Expect(err).ToNot(HaveOccurred())
Expect(strings.Count(sql, "not exists")).To(Equal(1))
Expect(sql).To(ContainSubstring("value LIKE ? OR value LIKE ?"))
Expect(args).To(HaveExactElements("%Rock%", "%Metal%"))
})
It("does not merge a single negated condition with a positive one of the same role in AND", func() {
// AND of mixed polarity must not be collapsed: NOT EXISTS(a) AND EXISTS(b)
// is not equivalent to any single merged subquery.
expr := criteria.All{
criteria.Contains{"artist": "Beatles"},
criteria.IsNot{"artist": "Kraftwerk"},
}
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
sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).where()
Expect(err).ToNot(HaveOccurred())
sql, _, err := sqlizer.ToSql()
Expect(err).ToNot(HaveOccurred())
// One positive EXISTS and one negated NOT EXISTS, kept separate
Expect(strings.Count(sql, "not exists")).To(Equal(1))
Expect(strings.Count(sql, "exists")).To(Equal(2)) // "not exists" contains "exists"
})
It("does not merge negated conditions of different roles in AND", func() {
expr := criteria.All{
criteria.IsNot{"artist": "Beatles"},
criteria.IsNot{"composer": "Lennon"},
}
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
sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).where()
Expect(err).ToNot(HaveOccurred())
sql, _, err := sqlizer.ToSql()
Expect(err).ToNot(HaveOccurred())
Expect(strings.Count(sql, "not exists")).To(Equal(2))
})
It("batches large negated AND groups to avoid SQLite expression tree depth limit", func() {
allExprs := make(criteria.All, jsonCondBatchSize+1)
for i := range allExprs {
allExprs[i] = criteria.IsNot{"artist": fmt.Sprintf("Artist%d", i)}
}
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
sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: allExprs}).where()
Expect(err).ToNot(HaveOccurred())
sql, args, err := sqlizer.ToSql()
Expect(err).ToNot(HaveOccurred())
// Two NOT EXISTS subqueries (one batch of jsonCondBatchSize, one of 1)
Expect(strings.Count(sql, "not exists")).To(Equal(2))
Expect(args).To(HaveLen(2 + jsonCondBatchSize + 1))
})
fix(smartplaylists): optimize smart playlist performance for role and tag criteria (#5515) * fix(server): optimize smart playlist role queries for large criteria (#5511) Role-based smart playlist criteria (artist, composer, etc.) now query the indexed media_file_artists join table instead of parsing JSON via json_tree() on every row. Multiple conditions for the same role within an OR group are merged into a single EXISTS subquery (batched at 200 to stay under SQLite's expression tree depth limit). A composite index (media_file_id, role) replaces the now-redundant single-column (media_file_id) index on media_file_artists. Benchmark (40k tracks, 500 patterns, 3 artists/track): - Merged join-table: 15ms (9.3x faster) - Merged json_tree: 30ms (4.6x faster) - Unmerged baseline: 137ms * refactor: simplify role condition SQL generation and benchmark Extract shared roleCondSQL/roleExistsSQL helpers to deduplicate the EXISTS template between roleCond and roleCondGroup. Use slices.Chunk for batching per project convention. Extract runBenchQuery helper to eliminate triplicated benchmark execution loop. * chore: raise roleCondBatchSize to 350 The empirical SQLite limit is 496 conditions per merged EXISTS subquery. Raising from 200 to 350 reduces the number of batches (e.g. 500 patterns now splits into 2 batches instead of 3). * fix(server): apply OR-merge optimization to tag conditions too Generalize mergeRoleConds into mergeJsonConds to also collapse multiple tag conditions for the same tag (e.g. genre) within OR groups. This gives the same ~5x speedup for tag-heavy smart playlists as the role optimization gives for artist-heavy ones. * refactor: benchmark uses real criteria pipeline instead of hand-built SQL The "Current" sub-benchmark now builds criteria.Criteria expressions and runs them through the actual newSmartPlaylistCriteria → Where() → ToSql() pipeline, validating the real production code path. The baseline still uses hand-built SQL representing the old json_tree approach. * fix: stabilize merged group ordering and close rows before error check Sort group keys in mergeJsonConds so the merged additions have deterministic order across runs, improving SQLite statement cache reuse. Move rows.Close() before rows.Err() in benchmark helper.
2026-05-22 18:00:13 -03:00
})
Describe("joins", func() {
It("excludes sort-only joins from expression joins", func() {
c := criteria.Criteria{Expression: criteria.All{criteria.Contains{"title": "love"}}, Sort: "albumRating"}
cSQL := newSmartPlaylistCriteria(c)
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
Expect(cSQL.expressionJoins()).To(Equal(smartPlaylistJoinNone))
Expect(cSQL.requiredJoins().has(smartPlaylistJoinAlbumAnnotation)).To(BeTrue())
})
It("includes expression-based joins", func() {
c := criteria.Criteria{Expression: criteria.All{criteria.Gt{"albumRating": 3}}}
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
Expect(newSmartPlaylistCriteria(c).expressionJoins().has(smartPlaylistJoinAlbumAnnotation)).To(BeTrue())
})
It("detects nested album and artist joins", func() {
c := criteria.Criteria{Expression: criteria.All{
criteria.Any{criteria.All{criteria.Is{"albumLoved": true}}},
criteria.Any{criteria.Gt{"artistPlayCount": 10}},
}}
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
joins := newSmartPlaylistCriteria(c).requiredJoins()
Expect(joins.has(smartPlaylistJoinAlbumAnnotation)).To(BeTrue())
Expect(joins.has(smartPlaylistJoinArtistAnnotation)).To(BeTrue())
})
It("detects join types from sort fields with direction prefixes", func() {
c := criteria.Criteria{Expression: criteria.All{criteria.Contains{"title": "love"}}, Sort: "-artistRating"}
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
Expect(newSmartPlaylistCriteria(c).requiredJoins().has(smartPlaylistJoinArtistAnnotation)).To(BeTrue())
})
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
It("keeps a sort-only album join out of the expression joins", func() {
c := criteria.Criteria{Expression: criteria.All{criteria.Contains{"title": "love"}}, Sort: "-albumDateAdded"}
cSQL := newSmartPlaylistCriteria(c)
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
Expect(cSQL.expressionJoins()).To(Equal(smartPlaylistJoinNone))
Expect(cSQL.requiredJoins().has(smartPlaylistJoinAlbum)).To(BeTrue())
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
})
It("distinguishes the album join from the album annotation join", func() {
c := criteria.Criteria{Expression: criteria.All{criteria.Gt{"albumRating": 3}}}
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
joins := newSmartPlaylistCriteria(c).requiredJoins()
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
Expect(joins.has(smartPlaylistJoinAlbumAnnotation)).To(BeTrue())
Expect(joins.has(smartPlaylistJoinAlbum)).To(BeFalse())
})
})
})