657 Commits

Author	SHA1	Message	Date
Deluan Quintão	838ceee26d	perf(subsonic): speed up artist search3 deep-offset pagination (#5620) ... * perf(subsonic): speed up artist search3 deep-offset pagination Empty-query and FTS artist search (search3/search2) paginated via a CROSS JOIN library_artist + DISTINCT in Phase 1 purely for library access control. The DISTINCT forced a temp b-tree over the whole junction table on every page, making deep offsets O(offset): ~200ms at offset 299k on 300k artists. Replace it with a join-free EXISTS predicate keyed on artist.id, backed by a new covering index on library_artist(artist_id, library_id). EXISTS keeps artist as the ordered driver and never fans out rowids, so Phase 1 stays a plain ordered scan that LIMIT/OFFSET can short-circuit. Admin, headless, and all-libraries users skip the filter entirely (the dominant case) for a flat ordered walk over the primary key. Measured on a 300k-artist / 1M-song library: admin/all-libs pagination is ~4.5-5.4x faster at depth (~180ms to ~33ms at offset 400k); restricted subset users keep correct, gap-free pages while also getting faster. The narrowing artist filter is applied at the subsonic layer only when the request targets a strict subset of the user's libraries, so the common case (and the admin fast-path) is never burdened with a redundant predicate. * fix(subsonic): narrow artist search by library set, not count narrowsArtistLibraries decided whether to add the subsonic-layer artist narrowing filter by comparing len(requested) < len(accessible). musicFolderId is not deduplicated, so duplicate IDs inflated the requested count: a user requesting ?musicFolderId=1&musicFolderId=1&musicFolderId=2 against three accessible libraries produced len([1,1,2])==3, which is not < 3, so the filter was skipped and the user saw artists from the third library too. Compare as set membership instead: the request narrows iff some accessible library is absent from it (requested is always a subset of accessible, validated upstream by selectedMusicFolderIds). This is immune to duplicate IDs. Add a regression test that fails against the old length-based check. Also consolidate the repeated EXISTS/no-DISTINCT/O(page) rationale that the prior commit spread across five sites down to a single authoritative comment on ArtistLibraryFilter, with the call sites referencing it. * perf(subsonic): drop redundant library_artist covering index The migration added an index on library_artist(artist_id, library_id) on the theory that the restricted-subset artist-search EXISTS needed it to seek by artist_id. Benchmarking on a 405k-artist / 5-library dataset showed no benefit: the EXISTS subquery constrains both columns (artist_id = and library_id IN), so SQLite already resolves it as a covering-index seek on the existing (library_id, artist_id) UNIQUE autoindex. With the new index present the planner still picks the autoindex and ignores it. Drop the migration and correct the comment. Removing ~11MB of dead index plus its write-amplification on every library_artist insert/delete, for zero query gain. * fix(scanner): mark artists missing when they lose their last library Artist search Phase 1 filters on artist.missing and Phase 2 inner-joins library_artist, so a non-missing artist with no library_artist row (an orphan) takes a pagination slot in Phase 1 and then vanishes in Phase 2, shortening the page and shifting deep offsets. The admin/headless search fast-path walks artist unfiltered, so it is fully exposed to this. Two paths created such orphans without updating artist.missing: - RefreshStats deletes library_artist rows whose stats are '{}' (artist lost all content in a library) after every scan. This is the common source. - Library deletion cascades away the library's library_artist rows. Mark newly-orphaned artists missing at both sources, so the shared 'missing = false' search filter excludes them immediately instead of waiting for a later scan. In RefreshStats the update only runs when the cleanup actually removed rows (the only way a new orphan can appear), so steady-state scans pay nothing; measured ~160ms on 300k artists only when orphans can exist. * refactor(subsonic): address review feedback on artist search filter Code-review follow-ups to the artist search pagination change: - ArtistLibraryFilter: short-circuit to a constant-false predicate when no library IDs are given, avoiding a degenerate empty IN () subquery. - ArtistLibraryFilter: add an inner LIMIT 1 to the correlated EXISTS so SQLite cannot flatten it into a fan-out join (an artist in multiple of the user's libraries would otherwise yield duplicate rowids and corrupt pagination). - narrowsArtistLibraries: compare accessible-vs-requested as a set lookup instead of slices.Contains in a loop. - searchConfig.LibraryFilter: document that a join-free filter is now a correctness requirement (DISTINCT was removed), not just a performance one. * docs: trim verbose comments in artist search/orphan code Condense the over-explained comments added in this PR to the essential 'why', removing repeated cross-references and restatements of the adjacent code. * fix(scanner): heal pre-existing orphan artists on full refresh The orphan-marking added to RefreshStats only ran when its empty-stats cleanup deleted rows, so it reconciled newly-created orphans but not ones already left in the database by older versions (whose library_artist row was deleted before this fix existed). Such legacy orphans would surface in the admin/headless search fast-path as short/gappy pages. Also run the orphan-marking on a full refresh (allArtists), so a full scan — which upgrades commonly trigger and users can run manually — reconciles the backlog. No migration needed; the runtime fixes prevent recurrence. * perf(subsonic): extend artist search fast-path to all-library users applyLibraryFilterToSearchQuery only skipped the library filter for admin and headless processes. A regular (non-admin) user who can access every library has the same result set as an admin, but was still given the EXISTS filter — an O(offset) cost for a predicate that matches every non-missing artist anyway. Skip the filter for them too, using a cheap library CountAll() (a count over the tiny library table) compared against the user's library count. On any error it falls back to the filtered path, which is correct, just slower. * fix(scanner): log error as trailing arg, not explicit error key Signed-off-by: Deluan <deluan@navidrome.org> * test(scanner): e2e guard for orphan artists under PurgeMissing Adds an end-to-end scanner test for the orphan-artist invariant fixed in RefreshStats: with Scanner.PurgeMissing enabled, removing all of an artist's files hard-deletes them, cascades away their media_file_artists rows, and RefreshStats then drops the artist's emptied library_artist row. The test asserts no non-missing artist is left without a library_artist row. Verified it fails without the RefreshStats orphan-marking and passes with it. * test(scanner): assert the orphaned artist is marked missing The orphan e2e test only checked the aggregate no-orphan invariant (orphanCount == 0), which a fully-deleted artist or an un-cleaned row would also satisfy — so it could pass without exercising the fix. Assert Pink Floyd's row specifically: missing=false before, missing=true after, and absent from the non-missing results. Verified it fails without the RefreshStats orphan-marking. * test(scanner): drop misleading non-missing-list assertion for orphan GetAll has no default missing filter, but selectArtist inner-joins library_artist, so an orphaned artist (no junction row) is excluded from the results whether or not it is marked missing. The Not(ContainElement) check therefore passed for the wrong reason. The direct floydMissing() == 1 query is the assertion that actually validates the missing flag; keep that plus the orphan-count invariant and an over-marking guard on The Beatles. * test(scanner): document why orphan check reads the artist row directly Clarify that GetAll cannot observe the orphan: selectArtist inner-joins library_artist, so an artist with no junction row is excluded from results whether or not it is marked missing. Asserting on GetAll would pass even without the fix, so the test reads the artist row directly to check the missing flag. * test(scanner): return descriptive artist state for clearer failures floydState returns PRESENT/MISSING/NOT_FOUND instead of 0/1/-1, so a failure reads '<string>: PRESENT to equal MISSING' rather than '0 to equal 1'. * refactor(subsonic): keep artist library scoping in the repository The search endpoint built a persistence-layer EXISTS predicate (persistence.ArtistLibraryFilter) and injected it into artistOpts.Filters — the only place the subsonic package reached into persistence, leaking a storage detail up two layers. Pass the same Eq{"library_id": ids} filter used for albums and songs, and let the artist repository translate it to the join-free library_artist predicate (scopeSearchToLibraries), where the junction knowledge belongs. The subset-vs- fast-path decision moves there too, so narrowsArtistLibraries and the persistence import are gone from the subsonic layer. Behavior is unchanged; coverage for the translation moves to artist_repository_test. * refactor(persistence): extract canonical markOrphansMissing helper The 'mark non-missing artists with no library_artist row as missing' invariant was hand-written as SQL in two places (RefreshStats and libraryRepository.Delete), in two slightly different dialects (not exists vs id not in). Extract a single artistRepository.markOrphansMissing method next to markMissing and call it from both sites, so the invariant has one definition. * fix(persistence): apply scoped library filter in both search phases Two bugs from moving the artist library-scoping into the repository: - Search() scoped opts.Filters for Phase 1 but still passed the original (unscoped) options to selectArtist, so Phase 2 re-applied the raw Eq{library_id} against the wrong columns and a restricted user's search returned nothing. Pass the scoped opts to both phases. - scopeSearchToLibraries dropped the filter unconditionally for admins, so an admin explicitly narrowing via musicFolderId (e.g. search3?musicFolderId=2) leaked content from other libraries. Compare the request against the user's visible library set (all libraries for admin/headless), narrowing whenever it is a strict subset. Both regressions were caught by the server/e2e multi-library suite. * fix(core): delete library and reconcile orphans in one transaction libraryRepository.Delete runs the FK-cascade delete and the orphaned-artist reconciliation (markOrphansMissing) as two writes on r.db. Called directly they autocommit separately, so an interruption between them could leave non-missing artists with no library_artist row — the orphan state the artist search fast-path forbids. Wrap the deletion in ds.WithTx at the core wrapper so both writes commit atomically; the watcher/scanner/broker side-effects stay post-commit. * refactor(persistence): unify artist search library scoping into one filter Phase 1 previously applied two overlapping library predicates: cfg.LibraryFilter (scoped to the user's libraries) AND options.Filters (the requested subset), producing two correlated EXISTS subqueries per rowid even though the request is always a subset of the user's libraries. And the 'does this user see everything' decision was implemented twice (userHasAllLibraries via CountAll vs scopeSearchToLibraries via set-membership), with applyLibraryFilterToSearchQuery as a third scoping path. Resolve the effective library scope once in Search() via searchScope (intersect the requested set with the user's visible libraries; nil = fast-path), clear opts.Filters, and realize that single scope as the only Phase-1 LibraryFilter. The visibility logic is now one pipeline: requestedLibraryIDs + visibleLibraryIDs + userSeesAllLibraries. Behavior unchanged; one EXISTS instead of two on the hot path, one source of truth for library visibility. * fix(persistence): harden artist search against malformed library_id filter Search consumed only an Eq{"library_id": []int} filter; an Eq whose library_id value wasn't []int slipped through unconsumed and would reach Phase 1's bare artist table (no library_id column) → SQL error. Recognize any Eq carrying a library_id key (isLibraryIDFilter) and always consume it, falling back to the user's visible scope for a malformed value. Non-library filters are still left in place for doSearch. * refactor(persistence): trim redundant comments and unexport artist library filter The artist-search-pagination work left dense explanatory comments, with the join-free / LIMIT-1 anti-flatten rationale and the orphan-artist mechanics each restated in several places. Consolidate each rationale into one canonical home (artistLibraryFilter for the EXISTS/LIMIT-1 trick, markOrphansMissing for the orphan lifecycle) and have the other sites reference it instead of repeating it. Also unexport ArtistLibraryFilter to artistLibraryFilter: its only caller is searchCfg in the same package and no test references it, so it never needed to be part of the package's exported surface. Comments only plus the rename; no behavior change. * refactor: add slice.ToSet and use it for the artist search subset check searchScope's subset test compared the requested libraries against the visible set with a nested slices.Contains, which is O(visible * requested). On an instance with many libraries (e.g. 100 libraries, a user granted 99) and an explicit musicFolderId request, that is ~9.8k comparisons; with a set it is ~200. Add a small reusable slice.ToSet helper (a slice -> map[T]struct{} set, collapsing duplicates) and use it to make the membership lookups O(1), restoring O(n+m) without the throwaway struct{}{} literal that an inline ToMap would need. No behavior change. * refactor(artist): move artistLibraryFilter to artist_repository Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-06-16 21:47:15 -04:00
Deluan Quintão	f0625ff709	perf(subsonic): speed up getRandomSongs with two-phase random-rowid selection (#5618) ... getRandomSongs used a single ORDER BY random() over the media_file table. At large library sizes that forces SQLite to scan the wide table and sort every matching row before applying the limit — roughly 4 seconds for a 1M-track library, regardless of how many songs are requested. Add MediaFileRepository.GetRandom, which does this in two passes: first select N random rowids over a narrow index (filters + library scope only, no wide columns or joins), so the random sort runs over compact, index-friendly data; then hydrate just those rows with the full select. The wide media_file row is never part of the sort. End-to-end this drops getRandomSongs from ~4s to ~0.3s on a 1M-track library, and the cost no longer grows with the requested size. The handler now calls GetRandom directly. The filter helper that builds the genre/year filters is renamed SongsByRandom -> SongsByGenreAndYearRange to reflect what it does, since the random ordering is now owned by GetRandom rather than a Sort option. Filters (genre, year, library) compose into the first pass unchanged. The album random list path is left as-is (far fewer rows, already fast).	2026-06-15 16:23:39 -04:00
Deluan Quintão	2c90685bc2	fix(scanner): import playlists skipped when no admin existed yet (#5609) ... * fix(scanner): import playlists skipped when no admin existed yet (#5499) On a fresh install the first scan runs before any admin user exists, so the scanner's playlist phase skips all playlists (playlists are owned by the first admin). Nothing re-imported them afterwards because folder selection is gated on updated_at > last_scan_at, which nothing bumps. The playlist phase now: - resolves the admin at phase time (FindFirstAdmin) instead of trusting the context snapshot taken at scan start, so a long admin-less scan still imports playlists in its own phase if an admin was created meanwhile; - records a persisted PlaylistsImportPending flag when no admin exists yet; - when that flag is set, imports ALL playlist folders via a new GetAllWithPlaylists (bypassing the timestamp gate) and clears the flag. Playlists are recovered by the next scan that runs with an admin, with no dependency on scan duration and no changes to the auth/server layers. * fix(scanner): surface datastore errors in playlist import deferral (#5499) Address review feedback: - distinguish model.ErrNotFound (no admin yet -> defer) from real datastore errors when resolving the admin, so DB failures are propagated, not swallowed; - propagate the error if the pending-import flag can't be persisted, so a scan doesn't complete as successful without recording the recovery; - surface read errors when checking the pending flag. Also name the no-admin condition for readability. * fix(scanner): simplify admin existence check in playlist import Signed-off-by: Deluan <deluan@navidrome.org> * fix(scanner): streamline folder access in playlist import logic Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-06-14 13:39:16 -04:00
Deluan Quintão	f3887df334	perf(smartplaylists): merge negated artist/tag rules into one NOT EXISTS ... * fix(smartplaylists): merge negated artist/tag rules in AND groups Smart playlists with many negated role/tag conditions ANDed together (e.g. 100+ "isNot artist" rules, issue #5511) generated one correlated NOT EXISTS subquery per rule, scanning media_file_artists for every candidate row. On large libraries this took minutes and triggered API timeouts and SQLite lock contention. By De Morgan, "NOT EXISTS(role=X) AND NOT EXISTS(role=Y)" is equivalent to "NOT EXISTS(role=X OR role=Y)", so multiple negated conditions for the same field can be collapsed into a single batched NOT EXISTS. This mirrors the existing OR-group merge that #5515 added for positive conditions. The shared grouping/batching logic is extracted into mergeSameFieldConds, parameterized by polarity, so the OR/positive and AND/negated paths reuse one algorithm instead of duplicating it. roleCondGroup/tagCondGroup gain a 'not' flag to emit the negated subquery. Benchmark (323k tracks, 120 isNot artist rules, reporter's exact shape): merged ~54ms vs unmerged ~8.7s steady-state (~160x faster). * docs: trim redundant comments on merge helpers The De Morgan explanation was repeated across three doc comments. Keep it in one place (mergeNegatedJsonConds, where negation is introduced) and reduce the shared core and group-type comments to concise one-liners.	2026-06-14 10:47:11 -04:00
Deluan Quintão	af78bdeb3a	fix(artwork): never serve artist folder images as album art (#5596) ... * test(artwork): add failing e2e tests for artist image leaking as album art Reproduces a v0.62.0 regression (#5451/#5457): the album cover-art parent-folder fallback can include the artist folder, serving the artist thumbnail (e.g. Artist/folder.jpg) as album art for any album without image files in its own folder(s). Covers three scenarios: a plain Artist/Album layout with no album images, a single-disc album spread across sibling folders under the artist folder, and a spread album whose own front.jpg is shadowed by the artist's cover.jpg via CoverArtPriority order. Also adds an albumByName test helper for multi-album layouts. The tests are expected to fail until the parent-folder inclusion is gated by a structural check (skip the common parent when audio from other albums lives under it). * fix(artwork): never serve artist folder images as album art The album cover-art parent-folder fallback (introduced in #5451/#5457) could include the artist folder as a source of album images, serving the artist thumbnail (e.g. Artist/folder.jpg) as cover art for any album without image files in its own folder(s). This affected both plain Artist/Album layouts and single-disc albums spread across sibling folders under the artist folder. Gate the common-parent inclusion with a structural check: the parent only qualifies as an album root when no audio belonging to other albums lives in it or anywhere beneath it. An artist folder contains other albums' tracks, while an album root above disc subfolders contains only this album's, so the check works for any disc folder naming scheme and never affects the multi-disc fixes from #5376/#5456. A single-album artist with no images anywhere remains structurally indistinguishable from an album root and is a known residual case. * refactor(artwork): move album-root audio check into folder repository Replace the raw subtree SQL (LIKE/ESCAPE expression and wildcard escaping) that lived in core/artwork with an explicit FolderRepository.HasAudioOutsideFolders method, implemented in the persistence layer next to the existing folder-subtree query pattern. This also removes the test mock's brittle dispatch that sniffed the generated SQL to recognize the query; the fake now overrides the new method directly. Extract the whole parent-folder resolution from loadAlbumFoldersPaths into an albumRootParent helper, flattening four levels of nesting back into a linear flow. Behavior is unchanged; the unit test for a parent containing audio moved to the persistence suite, with added coverage for subtree boundaries, missing folders, and LIKE-wildcard escaping in folder paths. * refactor(persistence): use exists helper in HasAudioOutsideFolders Replace the hand-rolled count(*) query with the repository's canonical exists helper, as suggested in PR review.	2026-06-13 13:29:29 -04:00
Deluan Quintão	da56df3160	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
Deluan Quintão	5ec6e6a8d4	fix(opensubsonic): make search3 empty-query pagination fast at large offsets (#5601) ... * fix(subsonic): make search3 empty-query pagination fast at large offsets Empty-query search3 (used by clients like Symfonium to sync the whole library) degraded linearly with songOffset: the offset optimization in optimizePagination keeps the original query's LEFT JOINs (annotation, bookmark, library) inside its rowid NOT IN subquery, making it as slow as plain OFFSET (~5s per page at offset 900K on a 920K-track library). Rewrite the empty-query branch of doSearch to use the same two-phase approach as the FTS search: Phase 1 paginates rowids on the bare main table, which SQLite satisfies with a covering index at any offset; Phase 2 hydrates only the page's rows with all JOINs. The Phase 2 hydration logic is extracted into hydrateRowidPage, now shared with ftsSearch.execute. Also replace the media_file_missing index with a composite covering index on (missing, library_id), so Phase 1 stays covering for non-admin users, whose queries include a library_id filter. The composite serves all missing-only lookups via its prefix. With a 920K-track / 85K-album test library, search3 empty-query responses are now flat (~0.1s) at every offset, for both admin and non-admin users (previously 3-5s at offsets above 600K). * refactor(persistence): share search Phase 1 contract and dedup junction fan-out Extract the Phase 1 query assembly that was duplicated between the FTS search and the empty-query search into executeTwoPhase: both paths now supply only their strategy-specific FROM/JOINs and ORDER BY, while the shared contract (missing filter, library access, options.Filters, and Max/Offset semantics) lives in one place. Also fix a pagination integrity bug: the artist library filter joins the library_artist junction table, so an artist present in multiple libraries produced duplicate rowids in Phase 1, corrupting offset-based pagination (short pages and repeated artists during full-library syncs). Phase 1 now applies DISTINCT whenever a junction-based LibraryFilter is set. DISTINCT is used instead of GROUP BY because bm25() cannot be evaluated in a grouped query; plain-filter tables (media_file, album) skip the dedup so their Phase 1 keeps the streaming covering-index plan. This also fixes the same duplication in the pre-existing FTS search path. * fix(persistence): pin artist search Phase 1 join order with CROSS JOIN search3 always filters artists by library (library_artist.library_id IN ...), and with the junction JOIN in the search Phase 1 rowid query SQLite chose to drive from library_artist, sorting every junction row with a temp b-tree on each page — a flat ~200ms penalty per request at 405K artists, even at offset 0 (the previous code avoided this by accident: its GROUP BY artist.id pinned an artist-driven plan). Use CROSS JOIN (SQLite's explicit join-order override) in a search-only variant of the artist library filter, keeping artist as the outer table so Phase 1 streams rowids in artist.id order from the primary key index and LIMIT/OFFSET short-circuits. The DISTINCT dedup stays and costs nothing under the streaming plan. Other artist queries keep the planner's freedom. With 405K artists, empty-query artist search is now 0.07s at offset 0 and 0.25s at offset 399K end-to-end (was 0.31s/0.34s before this fix, and up to 1.2s on master at deep offsets). Artist FTS text search is unaffected.	2026-06-12 15:53:37 -04:00
Deluan Quintão	08fd222791	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
Deluan	1e7996f5d7	fix(share): enforce per-user ownership on share reads ... Share repository read methods (Get, GetAll, Read, ReadAll, Exists, Count, CountAll) did not apply an owner filter, so non-admin users saw shares belonging to other users. The write paths already enforced per-user ownership; this brings reads in line with them. Add an addRestriction()/ownerFilter() based scope to share reads, keeping admins and the headless public-share resolution path unrestricted. Route share and player Delete through a new base-repo deleteOwned() primitive that applies the ownership predicate in the DELETE's WHERE clause (atomic, no select-then- delete window) and classifies a zero-row result as permission-denied vs not-found, mirroring updateOwned. The addRestriction helper and the write-miss classifier are hoisted onto the base repository so player and share share one implementation. Also map rest.ErrPermissionDenied and rest.ErrNotFound in the Subsonic error handler so ownership/not-found failures from the rest-backed repositories return the proper Subsonic codes (50 / 70) instead of a generic error. Covered by unit tests (persistence, subsonic error mapping) and an end-to-end cross-user sharing isolation test.	2026-06-05 15:50:59 -04:00
Deluan Quintão	174621f259	fix(nativeapi): make /api/song path filter work and use startsWith (#5566) ... The native API exposes a `path` query param on /api/song, but it was not registered in the media file filter map. Unmapped real columns fall through to a default LIKE predicate that emits an unqualified `path LIKE ?`. Since the song query joins the library table (which also has a `path` column), SQLite returned "ambiguous column name: path" and the request failed with HTTP 500. Register a dedicated path filter qualified to media_file.path, resolving the ambiguity. The value is matched with startsWith semantics (LIKE arg || '%') against the library-relative path stored in media_file.path. To register it inline (without a one-off wrapper), startsWithFilter now takes a bound field and returns a filterFunc, mirroring containsFilter. The two existing callers are updated accordingly, and the now-unused withTableName helper is removed. The user 'name' filter, which previously relied on withTableName, is now qualified directly as user.name; tests are added to guard that filter against the same column-ambiguity class (the user query also joins the library table, which has a name column). Signed-off-by: Deluan <deluan@navidrome.org>	2026-06-05 14:00:08 -04:00
Deluan Quintão	11640f2e4d	fix: restrict transcoding config reads to admins (#5564) ... * fix(security): restrict transcoding config reads to admins Authenticated non-admin users could read transcoding configs through the native API (GET /api/transcoding and /api/transcoding/{id}) when EnableTranscodingConfig was enabled. The responses included the full command templates, disclosing admin-configured ffmpeg invocations and local command paths. Write operations were already admin-only. The /transcoding route was registered in the general authenticated group, and only the repository's write methods checked IsAdmin. This applies the boundary at two layers: - Move the route under adminOnlyMiddleware, alongside the other admin-only resources (/library, /config, /inspect). - Add an IsAdmin guard to the repository's rest.Repository read methods (Read, ReadAll, Count) as defense-in-depth. The guard is scoped to the REST methods only. The streaming pipeline resolves profiles via Get/FindByFormat (model.TranscodingRepository), which stay open so transcoding keeps working for non-admin users. Adds regression tests covering non-admin read denial and confirming non-admin streaming lookups (Get/FindByFormat) still succeed. * fix(security): redact transcoding Command for non-admins instead of blocking reads Reworks the previous approach after review (Codex P2): moving /transcoding under adminOnlyMiddleware and denying non-admin reads broke legitimate non-admin UI flows. The web UI reads the transcoding resource as a regular user in several places that need only the profile name and target format: the player edit dropdown (ReferenceInput), the player list (ReferenceField), and the share/download format pickers (useGetList -> {targetFormat, name}). The only sensitive field is Command (the admin-owned ffmpeg template). So: - Revert the route move; /transcoding stays in the authenticated group. - Read/ReadAll now return the profiles to any authenticated user but blank the Command field for non-admins (mirrors user_repository's field-level redaction). Count is no longer denied (the UI needs list pagination). - Writes remain admin-only (Save/Update/Delete/Put). - Streaming is unaffected: it resolves profiles via Get/FindByFormat, which are not redacted, so on-the-fly transcoding keeps working for non-admins. Tests updated: non-admin reads succeed with Command blank, admin reads keep Command, non-admin Get/FindByFormat keep Command, writes still denied.	2026-06-04 23:07:13 -04:00
Deluan Quintão	37908d3cea	fix: enforce ownership atomically on player and share updates (#5563) ... * fix(player): enforce ownership atomically on player update The native API PUT /api/player/{id} authorized writes using the userId in the request body via isPermitted, while the actual write targeted the row by the URL id. A non-admin user could set userId to their own id in the body to pass the check, then overwrite and reassign ownership of another user's player row identified by the URL id (cross-tenant takeover). Add updateOwned on the base repository: an atomic, ownership-restricted UPDATE that folds the owner predicate (user_id = caller) into the WHERE clause for non-admins, so a row owned by another user simply does not match and no write happens. It also never writes user_id, so ownership is immutable on update and no caller (admin included) can reassign a player to a different owner. Unlike put, it never falls through to an INSERT, so a non-matching id returns ErrNotFound instead of creating a row. playerRepository.Update now uses updateOwned. Extract filterUpdateValues, shared by put and updateOwned, so the update-column filtering lives in one place. The create path (Save) keeps the body-based isPermitted check, which is correct for new records. Add regression tests covering the spoofed-userId hijack, regular-user and admin ownership reassignment, legitimate owner updates, and the nonexistent-player case. * fix(share): enforce ownership atomically on share update shareRepository.Update authorized writes with a separate checkOwnership SELECT, then wrote the row via put(). The check and the write were two statements (a TOCTOU window), put() could fall through to an INSERT on a missing id, and put() would write user_id if present in the update columns, so ownership was mutable on update. Switch Update to updateOwned, which folds the owner predicate into the UPDATE's WHERE clause, never writes user_id, and never inserts. This makes the write atomic and ownership immutable, and drops the extra ownership SELECT on the happy path. To preserve the previous 403/404 distinction, updateOwned now classifies a non-matching id: it runs a follow-up existence check only on the failure path (count == 0, where no write happened, so no TOCTOU) and returns ErrPermissionDenied when the row exists but is owned by another user, ErrNotFound when the id is missing. The player path inherits this: its tests now expect ErrPermissionDenied for a non-owner targeting an existing row, and ErrNotFound only for a genuinely missing id. Add share regression tests for the nonexistent-id and ownership- reassignment cases. checkOwnership remains in use by Delete. * refactor(persistence): extract canonical ownerFilter predicate The non-admin owner-restriction predicate (user_id = me, exempting admins and headless contexts) was spelled out independently in updateOwned and in playerRepository.addRestriction. The two copies had drifted: addRestriction did not exempt the headless/invalid user, so a headless context restricted to user_id = "-1" (matching nothing) while updateOwned exempted it. Extract sqlRepository.ownerFilter as the single definition and route both call sites through it. addRestriction now exempts the headless user too; that path is only reachable from the authenticated native API, so there is no production behavior change, but the latent divergence is removed. playlistRepository.userFilter is intentionally left alone: it encodes a different policy (public OR owner_id = me, on the owner_id column). * fix(share): preserve all-columns update path in Update shareRepository.Update unconditionally appended "updated_at" to cols. filterUpdateValues treats an empty cols as "update every column", so when a caller passes no columns, appending "updated_at" turned an all-columns update into an updated_at-only one, silently dropping every other field. The REST controller always populates cols from the request-body field names, so this path is not reachable through the native API and the behavior was latent (and pre-existing). Guard the append so the all-columns path is preserved, and add a regression test that updates with no columns and asserts the other fields persist. Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-06-04 19:43:13 -04:00
Deluan	2a43c4683e	chore: go fix ... Signed-off-by: Deluan <deluan@navidrome.org>	2026-05-28 22:13:05 -03:00
Deluan Quintão	74185dc6d1	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
Deluan	03ac02d964	refactor: more warnings clean up ... Signed-off-by: Deluan <deluan@navidrome.org>	2026-05-20 17:43:12 -03:00
Deluan	efe9291db0	refactor: multiple syntax updates for Go 1.26 ... Signed-off-by: Deluan <deluan@navidrome.org>	2026-05-19 18:02:36 -03:00
Deluan Quintão	8f0b4930ff	refactor(conf): replace eager dir creation with lazy Dir type (#5495) ... * feat(conf): add Dir type with lazy directory creation Introduces the Dir type that wraps a directory path string and defers os.MkdirAll until the first call to Path() or MustPath(), using sync.Once to ensure the creation happens exactly once. Implements fmt.Stringer, encoding.TextMarshaler, and encoding.TextUnmarshaler for config integration. Includes Ginkgo/Gomega tests covering all methods and error paths. * refactor(conf): replace eager dir creation with lazy Dir type Change DataFolder, CacheFolder, Plugins.Folder, and Backup.Path from string to Dir. Remove all os.MkdirAll calls from Load() so directories are created lazily on first Path()/MustPath() call. Artwork folder creation was already handled at point-of-use in image_upload.go. Add SnapshotConfig() to conf package for safe test config save/restore that avoids copying sync.Once inside Dir fields. Fix copy-lock vet warning in nativeapi/config.go by marshalling pointer instead of value. * refactor(conf): migrate tests and db init to lazy Dir type Update all test files to use conf.NewDir() for Dir field assignments. Ensure DataFolder is created lazily when the database is first opened in db.Db(). Remove eager directory creation from conf.Load() tests. * fix(conf): address review findings for Dir type - Use os.ModePerm for DataFolder/CacheFolder (was 0700, should match original behavior). Add NewDirWithPerm for PluginsFolder (0700). - Use Path() instead of MustPath() in db.Prune() to avoid logFatal from background cron job. - Panic on marshal/unmarshal errors in SnapshotConfig (test helper). - Clean up redundant String()/MustPath() calls in plugin manager. - Remove dead code in dir_test.go. Signed-off-by: Deluan <deluan@navidrome.org> * fix(conf): add GoString to Dir for clean config dump output Implement fmt.GoStringer on Dir so pretty.Sprintf shows the path string instead of internal struct fields (sync.Once, perm, err). Also add TODO comment to configtest about removing the indirection. * fix(dir): improve error logging in MustPath method Signed-off-by: Deluan <deluan@navidrome.org> * refactor(tests): remove redundant tests for unwritable DataFolder and CacheFolder Signed-off-by: Deluan <deluan@navidrome.org> * fix(conf): address PR review feedback - Ensure Plugins.Folder always uses 0700, even when user-configured (previously only the derived default got restrictive permissions). - Create LogFile parent directory before opening, so LogFile paths inside a not-yet-created DataFolder work correctly. --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-05-13 17:44:22 -03:00
Deluan Quintão	13c48b38a0	fix(smartplaylists): coerce string booleans in smart playlist rules (#5450) ... * fix(criteria): coerce string booleans in smart playlist rules - #4826 When clients (e.g. Feishin) send boolean values as strings ("true"/"false") in smart playlist JSON rules, the SQL comparison fails because SQLite stores booleans as 0/1 integers. For example, `COALESCE(annotation.starred, false) = 'true'` never matches. This adds a `boolean` flag to mapped fields and coerces string values to native Go bools in `mapFields`, so squirrel generates correct SQL parameters. Signed-off-by: mango766 <mango766@users.noreply.github.com> Signed-off-by: easonysliu <easonysliu@tencent.com> * fix(criteria): implement boolean string coercion for smart playlist rules Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: mango766 <mango766@users.noreply.github.com> Signed-off-by: easonysliu <easonysliu@tencent.com> Signed-off-by: Deluan <deluan@navidrome.org> Co-authored-by: easonysliu <easonysliu@tencent.com>	2026-05-01 19:21:48 -04:00
Deluan	57fc85f434	refactor(smartplaylist): remove unused 'value' field and clarify 'random' usage ... Signed-off-by: Deluan <deluan@navidrome.org>	2026-04-28 20:44:54 -04:00
Deluan	0fd9c6df2e	refactor(smartplaylist): clarify FieldInfo naming in criteria package ... Rename FieldInfo.Name to Alias (only meaningful for backward-compat entries like albumtype→releasetype) and FieldInfo.alias to tagAlias (tag names from mappings.yml that resolve to existing fields). Add a Name() method that derives the canonical name from the map key, eliminating 60+ redundant Name declarations where the value matched the key. LookupField now populates the private name field automatically. Signed-off-by: Deluan <deluan@navidrome.org>	2026-04-28 20:24:04 -04:00
Deluan	d9dac44456	feat(smartplaylist): add ReplayGain fields to criteria system ... Expose the four ReplayGain database columns (rg_album_gain, rg_album_peak, rg_track_gain, rg_track_peak) as first-class numeric criteria fields for smart playlists. This allows users to filter tracks by ReplayGain values, e.g. finding tracks with album gain below a threshold.	2026-04-28 19:46:12 -04:00
Deluan Quintão	46b4dcd5f6	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
Deluan Quintão	e6680c904b	fix(playlists): allow toggling auto-import and avoid unnecessary artwork reloads (#5421) ... * fix(playlists): allow toggling auto-import (sync) via REST API The updatePlaylistEntity handler was not applying the sync field from incoming requests, causing the auto-import toggle in the UI to have no effect. Apply the sync value for file-backed playlists only. * fix(playlists): enhance update logic for playlist metadata and sync toggle Signed-off-by: Deluan <deluan@navidrome.org> * fix(playlists): address code review feedback - Add pointer equality short-circuit in rulesEqual before reflect.DeepEqual - Guard against empty ID in Put's partial-update path - Only apply Sync when it actually differs from current value, preventing zero-value overwrites from partial payloads * fix(playlists): remove unused parameters from Update method Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-04-27 12:20:27 -04:00
Deluan Quintão	1bd736dae9	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
Deluan Quintão	81a17f6bbb	fix(search): normalization for non-NFKD Unicode letters (ø, æ, œ, ß) (#5413) ... * fix(search): transliterate non-ASCII letters symmetrically in FTS5 path Songs and artists with letters like ø, æ, œ, ß were unsearchable. The query path in server/subsonic/searching.go transliterates with sanitize.Accents (Øystein → Oystein), but the FTS5 tokenizer's remove_diacritics 2 only strips NFKD-decomposable marks — atomic letters with built-in strokes/ligatures survive tokenization, so the query side and index side disagreed. Apply sanitize.Accents on both sides: - normalizeForFTS now also emits an ASCII-transliterated form for each word, so search_normalized contains the variant the query produces. - buildFTS5Query transliterates the unquoted portion of the input so every caller (Subsonic, REST fullTextFilter) gets the same handling. Quoted phrases stay as typed, preserving phrase matches against the original title/artist columns. Existing libraries pick up the fix as records are re-scanned; users can trigger a manual full rescan to refresh older entries. * fix(search): cache transliteration and add ß/quoted-phrase test coverage Address review feedback: call sanitize.Accents once per word and reuse the result for both the punct-stripped and accent-only paths. Add missing test entries for ß→ss transliteration and quoted Unicode phrase preservation. --------- Co-authored-by: Claude <noreply@anthropic.com>	2026-04-25 20:27:38 -04:00
Deluan Quintão	ca09070a6c	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
Deluan Quintão	251cc71e2d	refactor: move smart playlist criteria SQL to persistence (#5408) ... * refactor: move criteria SQL generation to persistence Keep model/criteria as a domain DSL with JSON parsing, field metadata, expression traversal, and child playlist extraction only. Move smart playlist SQL translation, sort SQL, and join planning into persistence behind smartPlaylistCriteria so repository code uses a small query-building API. * refactor: simplify criteria translator metadata Use generic helper functions for criteria operator maps so the SQL translator can pass named criteria map types directly. Remove unused pseudo-field metadata from the criteria field API while preserving special field name lookup. * test: add coverage check for criteria-to-SQL field mappings Add a test that iterates all fields registered in the criteria package and verifies that every non-tag/non-role field has a corresponding entry in the persistence layer's smartPlaylistFields map. This prevents silent drift between the domain field registry and the SQL translation layer. Also adds an AllFieldNames() function to the criteria package to support field enumeration from outside the package.	2026-04-24 23:18:20 -04:00
Deluan	2954c052f5	fix(tests): update media file paths in tests to be relative ... Signed-off-by: Deluan <deluan@navidrome.org>	2026-04-19 20:07:23 -04:00
Deluan Quintão	64c8d3f4c5	ci: run Go tests on Windows (#5380) ... * ci(windows): add skeleton go-windows job (compile-only smoke test) * ci(windows): fix comment to reference Task 7 not Task 6 * ci(windows): harden PATH visibility and set explicit bash shell * ci(windows): enable full go test suite and ndpgen check * test(gotaglib): skip Unix-only permission tests on Windows * test(lyrics): skip Windows-incompatible tests * test(utils): skip Windows-incompatible tests * test(mpv): skip Windows-incompatible playback tests Skip 3 subprocess-execution tests that rely on Unix-style mpv invocation; .bat output includes \r-terminated lines that break argument parsing (#TBD-mpv-windows). * test(storage): skip Windows-incompatible tests Skip relative-path test where filepath.Join uses backslash but the storage implementation returns a forward-slash URL path (#TBD-path-sep-storage). * test(storage/local): skip Windows-incompatible tests Skip 13 tests that fail because url.Parse("file://" + windowsPath) treats the drive letter colon as an invalid port; also skip the Windows drive-letter path test that exposes a backslash vs forward-slash normalisation bug (#TBD-path-sep-storage-local). * test(playlists): skip Windows-incompatible tests * test(model): skip Windows-incompatible tests * test(model/metadata): skip Windows-incompatible tests * test(core): skip Windows-incompatible tests AbsolutePath uses filepath.Join which produces OS-native path separators; skip the assertion test on Windows until the production code is fixed (#TBD-path-sep-core). * test(artwork): skip Windows-incompatible tests Artwork readers produce OS-native path separators on Windows while tests assert forward-slash paths; skip 11 affected tests pending a fix in production code (#TBD-path-sep-artwork). * test(persistence): skip Windows-incompatible tests Skip flaky timestamp comparison (#TBD-flake-persistence) and path-separator real-bugs (#TBD-path-sep-persistence) in FolderRepository.GetFolderUpdateInfo which uses filepath.Clean/os.PathSeparator converting stored forward-slash paths to backslashes on Windows. * test(scanner): skip Windows-incompatible tests Skip symlink tests (Unix-assumption), ndignore path-separator bugs (#TBD-path-sep-scanner) in processLibraryEvents/resolveFolderPath where filepath.Rel/filepath.Split return backslash paths incompatible with fs.FS forward-slash expectations, error message mismatch on Windows, and file format upgrade detection (#TBD-path-sep-scanner). * test(plugins): skip Windows-incompatible tests Add //go:build !windows tags to test files that reference the suite bootstrap (testManager, testdataDir, createTestManager) which is only compiled on non-Windows. Add a Windows-only suite stub that skips all specs via BeforeEach to prevent [build failed] on Windows CI. * test(server): skip Windows-incompatible tests Skip createUnixSocketFile tests that rely on Unix file permission bits (chmod/fchmod) which are not supported on Windows. * test(nativeapi): skip Windows-incompatible tests Skip the i18n JSON validation test that uses filepath.Join to build embedded-FS paths; filepath.Join produces backslashes on Windows which breaks fs.Open (embedded FS always uses forward slashes). * test(e2e): skip Windows-incompatible tests On Windows, SQLite holds file locks that prevent the Ginkgo TempDir DeferCleanup from deleting the DB file. Register an explicit db.Close DeferCleanup (LIFO before TempDir cleanup) on Windows so the file lock is released before the temp directory is removed. * test(windows): fix e2e AfterSuite and skip remaining scanner path test * test(scanner): skip another Windows path-sep test (#TBD-path-sep-scanner) * test(subsonic): skip timing-flaky test on Windows (#TBD-flake-time-resolution-subsonic) * test(scanner): skip 'detects file moved to different folder' on Windows * test(scanner): consolidate 'Library changes' Windows skips into BeforeEach * test(scanner): close DB before TempDir cleanup to fix Windows file lock * test(scanner): skip ScanFolders suite on Windows instead of closing shared DB * ci: retrigger for Windows soak run 2/3 * ci: retrigger for Windows soak run 3/3 * ci: retrigger for Windows soak run 3/3 (take 2) * test(scanner): skip Multi-Library suite on Windows (SQLite file lock) * ci(windows): promote go-windows to blocking status check * test(plugins): run platform-neutral specs on Windows, drop blanket Skip * test(windows): make tests cross-platform instead of skipping - subsonic: back-date submissionTime baseline by 1s so BeTemporally(">") passes under millisecond clock resolution - persistence: sleep briefly between Put calls so UpdatedAt is strictly after CreatedAt on low-resolution clocks - utils/files: close tempFile before os.Remove so the test works on Windows (where an open handle holds a file lock) - tests.TempFile: close the handle before returning; metadata tests no longer leak the open file into Ginkgo's TempDir cleanup Resolves Copilot review comments on #5380. * test(tests): add SkipOnWindows helper to reduce boilerplate Introduces tests.SkipOnWindows(reason) that wraps the 3-line runtime.GOOS guard pattern used in every Windows-skipped spec. * test(adapters): use tests.SkipOnWindows helper * test(core): use tests.SkipOnWindows helper * test(model): use tests.SkipOnWindows helper * test(persistence): use tests.SkipOnWindows helper * test(scanner): use tests.SkipOnWindows helper * test(server): use tests.SkipOnWindows helper * test(plugins): run pure-Go unit tests on Windows config_validation_test, manager_loader_test, and migrate_test have no WASM/exec dependencies and don't rely on the make-built test plugins from plugins_suite_test.go. Let them run on Windows too.	2026-04-19 13:16:47 -04:00
Deluan Quintão	9b0bfc606b	fix(subsonic): always emit required created field on AlbumID3 (#5340) ... * fix(subsonic): always emit required `created` field on AlbumID3 Strict OpenSubsonic clients (e.g. Navic via dev.zt64.subsonic) reject search3/getAlbum/getAlbumList2 responses that omit the `created` field, which the spec marks as required. Navidrome was dropping it whenever the album's CreatedAt was zero. Root cause was threefold: 1. buildAlbumID3/childFromAlbum conditionally emitted `created`, so a zero CreatedAt became a missing JSON key. 2. ToAlbum's `older()` helper treated a zero BirthTime as the minimum, so a single track with missing filesystem birth time could poison the album aggregation. 3. phase_1_folders' CopyAttributes copied `created_at` from the previous album row unconditionally, propagating an already-zero value forward on every metadata-driven album ID change. Since sql_base_repository drops `created_at` on UPDATE, a poisoned row could never self-heal. Fixes: - Always emit `created`, falling back to UpdatedAt/ImportedAt when CreatedAt is zero. Adds albumCreatedAt() helper used by both buildAlbumID3 and childFromAlbum. - Guard `older()` against a zero second argument. - Skip the CopyAttributes call in phase_1_folders when the previous album's created_at is zero, so the freshly-computed value survives. - New migration backfills existing broken rows from media_file.birth_time (falling back to updated_at). Tested against a real DB: repaired 605/6922 affected rows, no side effects on healthy rows. Signed-off-by: Deluan <deluan@navidrome.org> * refactor(subsonic): return albumCreatedAt by value to avoid heap escape Returning *time.Time from albumCreatedAt caused Go escape analysis to move the entire model.Album parameter to the heap, since the returned pointer aliased a field of the value receiver. For hot endpoints like getAlbumList2 and search3, this meant one full-struct heap allocation per album result. Return time.Time by value and let callers wrap it with gg.P() to take the address locally. Only the small time.Time value escapes; the model.Album struct stays on the stack. Also corrects the doc comment to reflect the actual guarantee ("best-effort" rather than "non-zero"), matching the test case that exercises the all-zero fallback. --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-04-10 19:29:20 -04:00
Deluan	ccee33f474	fix(search): use explicit AND in FTS5 queries to fix apostrophe search ... FTS5's implicit AND (space-separated tokens) silently fails when combined with parenthesized OR groups produced by processPunctuatedWords. For example, searching "you've got" generated the query `("you ve" OR youve*) got*` which returned no results. Using explicit AND (`("you ve" OR youve*) AND got*`) resolves this FTS5 quirk. Since implicit and explicit AND are semantically identical in FTS5, this change is safe for all queries unconditionally.	2026-03-26 20:15:28 -04:00
Deluan Quintão	ba8d427890	feat(ui): add cover art support for internet radio stations (#5229) ... * feat(artwork): add KindRadioArtwork and EntityRadio constant * feat(model): add UploadedImage field and artwork methods to Radio * feat(model): add Radio to GetEntityByID lookup chain * feat(db): add uploaded_image column to radio table * feat(artwork): add radio artwork reader with uploaded image fallback * feat(api): add radio image upload/delete endpoints * feat(ui): add radio artwork ID prefix to getCoverArtUrl * feat(ui): add cover art display and upload to RadioEdit * feat(ui): add cover art thumbnails to radio list * feat(ui): prefer artwork URL in radio player helper * refactor: remove redundant code in radio artwork - Remove duplicate Avatar rendering in RadioList by reusing CoverArtField - Remove redundant UpdatedAt assignment in radio image handlers (already set by repository Put) * refactor(ui): extract shared useImageLoadingState hook Move image loading/error/lightbox state management into a shared useImageLoadingState hook in common/. Consolidates duplicated logic from AlbumDetails, PlaylistDetails, RadioEdit, and artist detail views. * feat(ui): use radio placeholder icon when no uploaded image Remove album placeholder fallback from radio artwork reader so radios without an uploaded image return ErrUnavailable. On the frontend, show the internet-radio-icon.svg placeholder instead of requesting server artwork when no image is uploaded, allowing favicon fallback in the player. * refactor(ui): update defaultOff fields in useSelectedFields for RadioList Signed-off-by: Deluan <deluan@navidrome.org> * fix: address code review feedback - Add missing alt attribute to CardMedia in RadioEdit for accessibility - Fix UpdateInternetRadio to preserve UploadedImage field by fetching existing radio before updating (prevents Subsonic API from clearing custom artwork) - Add Reader() level tests to verify ErrUnavailable is returned when radio has no uploaded image * refactor: add colsToUpdate to RadioRepository.Put Use the base sqlRepository.put with column filtering instead of hand-rolled SQL. UpdateInternetRadio now specifies only the Subsonic API fields, preventing UploadedImage from being cleared. Image upload/delete handlers specify only UploadedImage. * fix: ensure UpdatedAt is included in colsToUpdate for radio Put --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-03-18 18:57:33 -04:00
Deluan	b013b71ba9	fix(server): clean up uploaded artist images during GC ... When artists are purged during garbage collection, any custom uploaded cover images were left orphaned on disk. Modified purgeEmpty() to query for uploaded_image filenames before the bulk DELETE, then remove the corresponding files from disk afterwards. Image cleanup is best-effort to avoid failing the GC if a file is already missing or inaccessible. Also populated album_artists entries in the persistence test suite setup to reflect the actual album-artist relationships from test data, ensuring purgeEmpty() doesn't inadvertently delete shared test artists.	2026-03-17 19:47:09 -04:00
Deluan Quintão	e7c6e78dd0	fix(db): normalize timestamps and fix recently added album sorting (#5176) ... * fix(db): normalize timestamps and fix recently added album sorting SQLite stores timestamps as TEXT and uses string comparison for ORDER BY. Timestamps in RFC3339 T-format ('2024-01-01T10:00:00Z') sort incorrectly against space-format ('2024-01-01 10:00:00+00:00') because 'T' (ASCII 84) > ' ' (ASCII 32), causing albums with T-format timestamps to appear as newer than they are in the "Recently Added" list. This adds a migration to normalize all T-format timestamps across all tables to the space-format expected by go-sqlite3, wraps the recently_added sort with datetime() to make it format-agnostic, and replaces the plain album timestamp indexes with expression indexes to maintain query performance. * fix(test): improve recently_added sort test robustness Use same-date timestamps (2024-01-15T08:00:00Z vs 2024-01-15 20:00:00) so the T-vs-space character difference at position 10 actually triggers the sorting bug. Initialize index variables to -1 and assert both test albums are found before comparing positions. * chore(db): update migration timestamp to 2026-03-16	2026-03-16 07:55:22 -04:00
Deluan Quintão	6b8fcc37c6	fix(share): add ownership checks to Delete and Update (#5189) ... * test(share): add failing tests for Delete ownership checks * fix(share): add ownership check to Delete * test(share): add failing tests for Update ownership checks * fix(share): add ownership check to Update * refactor(share): extract checkOwnership helper with lightweight query - Deduplicate ownership check from Delete and Update into a single helper - Use a minimal single-column SELECT instead of Get (avoids loadMedia overhead) - Use positive bypass form (IsAdmin || invalidUserId) matching codebase convention * fix(share): convert model.ErrNotFound to rest.ErrNotFound in checkOwnership Ensure consistent 404 responses when a nonexistent share ID is passed to Delete or Update, by handling the conversion in checkOwnership rather than relying on the subsequent write operation.	2026-03-15 00:12:58 -04:00
Deluan Quintão	ae1e0ddb11	feat(subsonic): implement OpenSubsonic Transcoding extension (#4990) ... * feat(subsonic): implement transcode decision logic and codec handling for media files Signed-off-by: Deluan <deluan@navidrome.org> * fix(subsonic): update codec limitation structure and decision logic for improved clarity Signed-off-by: Deluan <deluan@navidrome.org> * fix(transcoding): update bitrate handling to use kilobits per second (kbps) across transcode decision logic Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): simplify container alias handling in matchesContainer function Signed-off-by: Deluan <deluan@navidrome.org> * fix(transcoding): enforce POST method for GetTranscodeDecision and handle non-POST requests Signed-off-by: Deluan <deluan@navidrome.org> * feat(transcoding): add enums for protocol, comparison operators, limitations, and codec profiles in transcode decision logic Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): streamline limitation checks and applyLimitation logic for improved readability and maintainability Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): replace strings.EqualFold with direct comparison for protocol and limitation checks Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): rename token methods to CreateTranscodeParams and ParseTranscodeParams for clarity Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): enhance logging for transcode decision process and client info conversion Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): rename TranscodeDecision to Decider and update related methods for clarity Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): enhance transcoding config lookup logic for audio codecs Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): enhance transcoding options with sample rate support and improve command handling Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): add bit depth support for audio transcoding and enhance related logic Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): enhance AAC command handling and support for audio channels in streaming Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): streamline transcoding logic by consolidating stream parameter handling and enhancing alias mapping Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): update default command handling and add codec support for transcoding Signed-off-by: Deluan <deluan@navidrome.org> * fix: implement noopDecider for transcoding decision handling in tests Signed-off-by: Deluan <deluan@navidrome.org> * fix: address review findings for OpenSubsonic transcoding PR Fix multiple issues identified during code review of the transcoding extension: add missing return after error in shared stream handler preventing nil pointer panic, replace dead r.Body nil check with MaxBytesReader size limit, distinguish not-found from other DB errors, fix bpsToKbps integer truncation with rounding, add "pcm" to isLosslessFormat for consistency with model.IsLossless(), add sampleRate/bitDepth/channels to streaming log, fix outdated test comment, and add tests for conversion functions and GetTranscodeStream parameter passing. * feat(transcoding): add sourceUpdatedAt to decision and validate transcode parameters Signed-off-by: Deluan <deluan@navidrome.org> * fix: small issues Updated mock AAC transcoding command to use the new default (ipod with fragmented MP4) matching the migration, ensuring tests exercise the same buildDynamicArgs code path as production. Improved archiver test mock to match on the whole StreamRequest struct instead of decomposing fields, making it resilient to future field additions. Added named constants for JWT claim keys in the transcode token and wrapped ParseTranscodeParams errors with ErrTokenInvalid for consistency. Documented the IsLossless BitDepth fallback heuristic as temporary until Codec column is populated. Signed-off-by: Deluan <deluan@navidrome.org> * fix(transcoding): adapt transcode claims to struct-based auth.Claims Updated transcode token handling to use the struct-based auth.Claims introduced on master, replacing the previous map[string]any approach. Extended auth.Claims with transcoding-specific fields (MediaID, DirectPlay, UpdatedAt, Channels, SampleRate, BitDepth) and added float64 fallback in ClaimsFromToken for numeric claims that lose their Go type during JWT string serialization. Also added the missing lyrics parameter to all subsonic.New() calls in test files. * feat(model): add ProbeData field and UpdateProbeData repository method Add probe_data TEXT column to media_file for caching ffprobe results. Add UpdateProbeData to MediaFileRepository interface and implementations. Use hash:"ignore" tag so probe data doesn't affect MediaFile fingerprints. * feat(ffmpeg): add ProbeAudioStream for authoritative audio metadata Add ProbeAudioStream to FFmpeg interface, using ffprobe to extract codec, profile, bitrate, sample rate, bit depth, and channels. Parse bits_per_raw_sample as fallback for FLAC/ALAC bit depth. Normalize "unknown" profile to empty string. All parseProbeOutput tests use real ffprobe JSON from actual files. * feat(transcoding): integrate ffprobe into transcode decisions Add ensureProbed to probe media files on first transcode decision, caching results in probe_data. Build SourceStream from probe data with fallback to tag-based metadata. Refactor decision logic to pass StreamDetails instead of MediaFile, enabling codec profile limitations (e.g., audioProfile) to use probe data. Add normalizeProbeCodec to map ffprobe codec names (dsd_lsbf_planar, pcm_s16le) to internal names (dsd, pcm). NewDecider now accepts ffmpeg.FFmpeg; wire_gen.go regenerated. * feat(transcoding): add DevEnableMediaFileProbe config flag Add DevEnableMediaFileProbe (default true) to allow disabling ffprobe- based media file probing as a safety fallback. When disabled, the decider uses tag-based metadata from the scanner instead. * test(transcode): add ensureProbed unit tests Test probing when ProbeData is empty, skipping when already set, error propagation from ffprobe, and DevEnableMediaFileProbe flag. * refactor(ffmpeg): use command constant and select_streams for ProbeAudioStream Move ffprobe arguments to a probeAudioStreamCmd constant, following the same pattern as extractImageCmd and probeCmd. Add -select_streams a:0 to only probe the first audio stream, avoiding unnecessary parsing of video and artwork streams. Derive the ffprobe binary path safely using filepath.Dir/Base instead of replacing within the full path string. * refactor(transcode): decouple transcode token claims from auth.Claims Remove six transcode-specific fields (MediaID, DirectPlay, UpdatedAt, Channels, SampleRate, BitDepth) from auth.Claims, which is shared with session and share tokens. Transcode tokens are signed parameter-passing tokens, not authentication tokens, so coupling them to auth created misleading dependencies. The transcode package now owns its own JWT claim serialization via Decision.toClaimsMap() and paramsFromToken(), using generic auth.EncodeToken/DecodeAndVerifyToken wrappers that keep TokenAuth encapsulated. Wire format (JWT claim keys) is unchanged, so in-flight tokens remain compatible. Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcode): simplify code after review Extract getIntClaim helper to eliminate repeated int/int64/float64 JWT claim extraction pattern in paramsFromToken and ClaimsFromToken. Rewrite checkIntLimitation as a one-liner delegating to applyIntLimitation. Return probe result from ensureProbed to avoid redundant JSON round-trip. Extract toResponseStreamDetails helper and mediaTypeSong constant in the API layer, and use transcode.ProtocolHTTP constant instead of hardcoded string. Signed-off-by: Deluan <deluan@navidrome.org> * fix(ffmpeg): enhance bit_rate parsing logic for audio streams Signed-off-by: Deluan <deluan@navidrome.org> * fix(transcode): improve code review findings across transcode implementation - Fix parseProbeData to return nil on JSON unmarshal failure instead of a zero-valued struct, preventing silent degradation of source stream details - Use probe-resolved codec for lossless detection in buildSourceStream instead of the potentially stale scanner data - Remove MediaFile.IsLossless() (dead code) and consolidate lossless detection in isLosslessFormat(), using codec name only — bit depth is not reliable since lossy codecs like ADPCM report non-zero values - Add "wavpack" to lossless codec list (ffprobe codec_name for WavPack) - Guard bpsToKbps against negative input values - Fix misleading comment in buildTemplateArgs about conditional injection - Avoid leaking internal error details in Subsonic API responses - Add missing test for ErrNotFound branch in GetTranscodeDecision - Add TODO for hardcoded protocol in toResponseStreamDetails * refactor(transcode): streamline transcoding command lookup and format resolution Signed-off-by: Deluan <deluan@navidrome.org> * feat(transcode): implement server-side transcoding override for player formats Signed-off-by: Deluan <deluan@navidrome.org> * fix(transcode): honor bit depth and channel constraints in transcoding selection selectTranscodingOptions only checked sample rate when deciding whether same-format transcoding was needed, ignoring requested bit depth and channel reductions. This caused the streamer to return raw audio when the transcode decision requested downmix or bit-depth conversion. * refactor(transcode): unify streaming decision engine via MakeDecision Move transcoding decision-making out of mediaStreamer and into the subsonic Stream/Download handlers, using transcode.Decider.MakeDecision as the single decision engine. This eliminates selectTranscodingOptions and the mismatch between decision and streaming code paths (decision used LookupTranscodeCommand with built-in fallbacks, while streaming used FindByFormat which only checked the DB). - Add DecisionOptions with SkipProbe to MakeDecision so the legacy streaming path never calls ffprobe - Add buildLegacyClientInfo to translate legacy stream params (format, maxBitRate, DefaultDownsamplingFormat) into a synthetic ClientInfo - Add resolveStreamRequest on the subsonic Router to resolve legacy params into a fully specified StreamRequest via MakeDecision - Simplify DoStream to a dumb executor that receives pre-resolved params - Remove selectTranscodingOptions entirely Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcode): move MediaStreamer into core/transcode and unify StreamRequest Moved MediaStreamer, Stream, TranscodingCache and related types from core/media_streamer.go into core/transcode/, eliminating the duplicate StreamRequest type. The transcode.StreamRequest now carries all fields (ID, Format, BitRate, SampleRate, BitDepth, Channels, Offset) and ResolveStream returns a fully-populated value, removing manual field copying at every call site. Also moved buildLegacyClientInfo into the transcode package alongside ResolveStream, and unexported ParseTranscodeParams since it was only used internally by ValidateTranscodeParams. * refactor(transcode): rename Decider methods and unexport Params type Rename ResolveStream → ResolveRequest and ValidateTranscodeParams → ResolveRequestFromToken for clarity and consistency. The new ResolveRequestFromToken returns a StreamRequest directly (instead of the intermediate Params type), eliminating manual Params→StreamRequest conversion in callers. Unexport Params to params since it is now only used internally for JWT token parsing. * test(transcode): remove redundant tests and use constants Remove tests that duplicate coverage from integration-level tests (toClaimsMap, paramsFromToken round-trips, applyServerOverride direct call, duplicate 410 handler test). Replace raw "http" strings with ProtocolHTTP constant. Consolidate lossy -sample_fmt tests into DescribeTable. * refactor(transcode): split oversized files into focused modules Split transcode.go and transcode_test.go into focused files by concern: - decider.go: decision engine (MakeDecision, direct play/transcode evaluation, probe) - token.go: JWT token encode/decode (params, toClaimsMap, paramsFromToken, CreateTranscodeParams, ResolveRequestFromToken) - legacy_client.go: legacy Subsonic bridge (buildLegacyClientInfo, ResolveRequest) - codec_test.go: isLosslessFormat and normalizeProbeCodec tests - token_test.go: token round-trip and ResolveRequestFromToken tests Moved the Decider interface from types.go to decider.go to keep it near its implementation, and cleaned up types.go to contain only pure type definitions and constants. No public API changes. * refactor(transcode): reorder parameters in applyServerOverride function Signed-off-by: Deluan <deluan@navidrome.org> * test(e2e): add NewTestStream function and implement spyStreamer for testing Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-03-08 23:57:49 -04:00
Deluan Quintão	11e4aaed1b	feat(server): add percentage-based limits to smart playlists (#5144) ... * feat(playlists): add percentage-based limits to smart playlists Add a new `limitPercent` JSON field to Criteria that allows smart playlist limits to be expressed as a percentage of matching tracks rather than a fixed number. For example, a playlist matching 450 songs with a 10% limit returns 45 songs, scaling dynamically as the library grows. When `limitPercent` is set, refreshSmartPlaylist runs a COUNT query first to determine the total matching tracks, then resolves the percentage to an absolute LIMIT before executing the main query. The fixed `limit` field takes precedence when both are set. Values are clamped to [0, 100] during JSON unmarshaling. No database migration is needed since rules are stored as a JSON string. * fix(criteria): validate percentage limit range in IsPercentageLimit method Signed-off-by: Deluan <deluan@navidrome.org> * fix(criteria): ensure idempotency of ToSql method for expressions Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-03-04 22:42:49 -05:00
Deluan	24ba655dc3	refactor: simplify error handling in updateParticipants and toModels methods ... Signed-off-by: Deluan <deluan@navidrome.org>	2026-03-03 08:14:54 -05:00
Deluan Quintão	ed4c0ef432	fix(scanner): add nil guards to cursor wrapping (#5139) ... * fix(persistence): add nil guards to cursor wrapping in folder and mediafile repos Prevent SIGSEGV panic when queryWithStableResults yields a zero-value struct on the rows.Err() path (e.g., "database is locked" during concurrent scanning). Extract cursor wrapping into wrapFolderCursor and wrapMediaFileCursor with nil checks matching the existing pattern in album_repository.go. Fixes #5138 * fix(persistence): wrap original cursor error in nil guard messages Use %w to preserve the underlying error (e.g., "database is locked") so callers can use errors.Is/As for root cause analysis. Tests now verify the original error is accessible via errors.Is. * fix(persistence): add nil guards and error wrapping in album, folder, and mediafile cursor functions Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-03-03 07:58:14 -05:00
Deluan	27a83547f7	fix(plugins): clear plugin errors on startup to allow retrying ... Plugins that entered an error state (e.g., incompatible with the Navidrome version) would remain in that state across restarts, blocking the user from retrying. This adds a ClearErrors method to PluginRepository that resets the last_error field on all plugins, and calls it during plugin manager startup before syncing and loading. Signed-off-by: Deluan <deluan@navidrome.org>	2026-03-02 08:56:56 -05:00
Deluan Quintão	d9a215e1e3	feat(plugins): allow mounting library directories as read-write (#5122) ... * feat(plugins): mount library directories as read-only by default Add an AllowWriteAccess boolean to the plugin model, defaulting to false. When off, library directories are mounted with the extism "ro:" prefix (read-only). Admins can explicitly grant write access via a new toggle in the Library Permission card. * test: add tests to buildAllowedPaths Signed-off-by: Deluan <deluan@navidrome.org> * chore: improve allowed paths logging for library access Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-02-28 10:59:13 -05:00
Deluan	d134de1061	feat(server): add 'has_rating' filter to artist and mediafile repositories ... Signed-off-by: Deluan <deluan@navidrome.org>	2026-02-28 10:55:19 -05:00
Deluan Quintão	b59eb32961	feat(subsonic): sort search3 results by relevance (#5086) ... * fix(subsonic): optimize search3 for high-cardinality FTS queries Use a two-phase query strategy for FTS5 searches to avoid the performance penalty of expensive LEFT JOINs (annotation, bookmark, library) on high-cardinality results like "the". Phase 1 runs a lightweight query (main table + FTS index only) to get sorted, paginated rowids. Phase 2 hydrates only those few rowids with the full JOINs, making them nearly free. For queries with complex ORDER BY expressions that reference joined tables (e.g. artist search sorted by play count), the optimization is skipped and the original single-query approach is used. * fix(search): update order by clauses to include 'rank' for FTS queries Signed-off-by: Deluan <deluan@navidrome.org> * fix(search): reintroduce 'rank' in Phase 2 ORDER BY for FTS queries Signed-off-by: Deluan <deluan@navidrome.org> * fix(search): remove 'rank' from ORDER BY in non-FTS queries and adjust two-phase query handling Signed-off-by: Deluan <deluan@navidrome.org> * fix(search): update FTS ranking to use bm25 weights and simplify ORDER BY qualification Signed-off-by: Deluan <deluan@navidrome.org> * fix(search): refine FTS query handling and improve comments for clarity Signed-off-by: Deluan <deluan@navidrome.org> * fix(search): refactor full-text search handling to streamline query strategy selection and improve LIKE fallback logic. Increase e2e coverage for search3 Signed-off-by: Deluan <deluan@navidrome.org> * refactor: enhance FTS column definitions and relevance weights Signed-off-by: Deluan <deluan@navidrome.org> * fix(search): refactor Search method signatures to remove offset and size parameters, streamline query handling Signed-off-by: Deluan <deluan@navidrome.org> * fix(search): allow single-character queries in search strategies and update related tests Signed-off-by: Deluan <deluan@navidrome.org> * fix(search): make FTS Phase 1 treat Max=0 as no limit, reorganize tests FTS Phase 1 unconditionally called Limit(uint64(options.Max)), which produced LIMIT 0 when Max was zero. This diverged from applyOptions where Max=0 means no limit. Now Phase 1 mirrors applyOptions: only add LIMIT/OFFSET when the value is positive. Also moved legacy backend integration tests from sql_search_fts_test.go to sql_search_like_test.go and added regression tests for the Max=0 behavior on both backends. * refactor: simplify callSearch function by removing variadic options and directly using QueryOptions Signed-off-by: Deluan <deluan@navidrome.org> * fix(search): implement ftsQueryDegraded function to detect significant content loss in FTS queries Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-02-23 08:51:54 -05:00
Valeri Sokolov	23bf256a66	feat: make album and artist annotations available to smart playlists (#4927) ... * feat(criteria): make album ratings available to smart playlist queries Expose an "albumrating" field mapping to album annotations. Signed-off-by: Valeri Sokolov <ulfurinn@ulfurinn.net> * fix(criteria): use query parameters Signed-off-by: Valeri Sokolov <ulfurinn@ulfurinn.net> * feat: add album and artist annotation fields to smart playlists Extend smart playlists to filter songs by album or artist annotations (rating, loved, play count, last played, date loved, date rated). This adds 12 new fields (6 album, 6 artist) with conditional JOINs that are only added when the criteria or sort references them, avoiding unnecessary query overhead. The album table JOIN is also removed since media_file.album_id can be used directly. --------- Signed-off-by: Valeri Sokolov <ulfurinn@ulfurinn.net> Co-authored-by: Deluan <deluan@navidrome.org>	2026-02-22 22:05:59 -05:00
Deluan	ec75808153	fix(subsonic): handle empty quoted phrases in FTS5 query and search expression ... Signed-off-by: Deluan <deluan@navidrome.org>	2026-02-21 22:00:00 -05:00
Deluan Quintão	7ad2907719	refactor: move playlist business logic from repositories to service layer (#5027) ... * refactor: move playlist business logic from repositories to core.Playlists service Move authorization, permission checks, and orchestration logic from playlist repositories to the core.Playlists service, following the existing pattern used by core.Share and core.Library. Changes: - Expand core.Playlists interface with read, mutation, track management, and REST adapter methods - Add playlistRepositoryWrapper for REST Save/Update/Delete with permission checks (follows Share/Library pattern) - Simplify persistence/playlist_repository.go: remove isWritable(), auth checks from Delete()/Put()/updatePlaylist() - Simplify persistence/playlist_track_repository.go: remove isTracksEditable() and permission checks from Add/Delete/Reorder - Update Subsonic API handlers to route through service - Update Native API handlers to accept core.Playlists instead of model.DataStore * test: add coverage for playlist service methods and REST wrapper Add 30 new tests covering the service methods added during the playlist refactoring: - Delete: owner, admin, denied, not found - Create: new playlist, replace tracks, admin bypass, denied, not found - AddTracks: owner, admin, denied, smart playlist, not found - RemoveTracks: owner, smart playlist denied, non-owner denied - ReorderTrack: owner, smart playlist denied - NewRepository wrapper: Save (owner assignment, ID clearing), Update (owner, admin, denied, ownership change, not found), Delete (delegation with permission checks) Expand mockedPlaylistRepo with Get, Delete, Tracks, GetWithTracks, and rest.Persistable methods. Add mockedPlaylistTrackRepo for track operation verification. * fix: add authorization check to playlist Update method Added ownership verification to the Subsonic Update endpoint in the playlist service layer. The authorization check was present in the old repository code but was not carried over during the refactoring to the service layer, allowing any authenticated user to modify playlists they don't own via the Subsonic API. Also added corresponding tests for the Update method's permission logic. * refactor: improve playlist permission checks and error handling, add e2e tests Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename core.Playlists to playlists package and update references Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename playlists_internal_test.go to parse_m3u_test.go and update tests; add new parse_nsp.go and rest_adapter.go files Signed-off-by: Deluan <deluan@navidrome.org> * fix: block track mutations on smart playlists in Create and Update Create now rejects replacing tracks on smart playlists (pre-existing gap). Update now uses checkTracksEditable instead of checkWritable when track changes are requested, restoring the protection that was removed from the repository layer during the refactoring. Metadata-only updates on smart playlists remain allowed. * test: add smart playlist protection tests to ensure readonly behavior and mutation restrictions * refactor: optimize track removal and renumbering in playlists Signed-off-by: Deluan <deluan@navidrome.org> * refactor: implement track reordering in playlists with SQL updates Signed-off-by: Deluan <deluan@navidrome.org> * refactor: wrap track deletion and reordering in transactions for consistency Signed-off-by: Deluan <deluan@navidrome.org> * refactor: remove unused getTracks method from playlistTrackRepository Signed-off-by: Deluan <deluan@navidrome.org> * refactor: optimize playlist track renumbering with CTE-based UPDATE Replace the DELETE + re-INSERT renumbering strategy with a two-step UPDATE approach using a materialized CTE and ROW_NUMBER() window function. The previous approach (SELECT all IDs, DELETE all tracks, re-INSERT in chunks of 200) required 13 SQL operations for a 2000-track playlist. The new approach uses just 2 UPDATEs: first negating all IDs to clear the positive space, then assigning sequential positions via UPDATE...FROM with a CTE. This avoids the UNIQUE constraint violations that affected the original correlated subquery while reducing per-delete request time from ~110ms to ~12ms on a 2000-track playlist. Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename New function to NewPlaylists for clarity Signed-off-by: Deluan <deluan@navidrome.org> * refactor: update mock playlist repository and tests for consistency Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-02-21 19:57:13 -05:00
Deluan Quintão	54de0dbc52	feat(server): implement FTS5-based full-text search (#5079) ... * build: add sqlite_fts5 build tag to enable FTS5 support * feat: add SearchBackend config option (default: fts) * feat: add buildFTS5Query for safe FTS5 query preprocessing * feat: add FTS5 search backend with config toggle, refactor legacy search - Add searchExprFunc type and getSearchExpr() for backend selection - Rename fullTextExpr to legacySearchExpr - Add ftsSearchExpr using FTS5 MATCH subquery - Update fullTextFilter in sql_restful.go to use configured backend * feat: add FTS5 migration with virtual tables, triggers, and search_participants Creates FTS5 virtual tables for media_file, album, and artist with unicode61 tokenizer and diacritic folding. Adds search_participants column, populates from JSON, and sets up INSERT/UPDATE/DELETE triggers. * feat: populate search_participants in PostMapArgs for FTS5 indexing * test: add FTS5 search integration tests * fix: exclude FTS5 virtual tables from e2e DB restore The restoreDB function iterates all tables in sqlite_master and runs DELETE + INSERT to reset state. FTS5 contentless virtual tables cannot be directly deleted from. Since triggers handle FTS5 sync automatically, simply skip tables matching *_fts and *_fts_* patterns. * build: add compile-time guard for sqlite_fts5 build tag Same pattern as netgo: compilation fails with a clear error if the sqlite_fts5 build tag is missing. * build: add sqlite_fts5 tag to reflex dev server config * build: extract GO_BUILD_TAGS variable in Makefile to avoid duplication * fix: strip leading * from FTS5 queries to prevent "unknown special query" error * feat: auto-append prefix wildcard to FTS5 search tokens for broader matching Every plain search token now gets a trailing * appended (e.g., "love" becomes "love*"), so searching for "love" also matches "lovelace", "lovely", etc. Quoted phrases are preserved as exact matches without wildcards. Results are ordered alphabetically by name/title, so shorter exact matches naturally appear first. * fix: clarify comments about FTS5 operator neutralization The comments said "strip" but the code lowercases operators to neutralize them (FTS5 operators are case-sensitive). Updated comments to accurately describe the behavior. * fix: use fmt.Sprintf for FTS5 phrase placeholders The previous encoding used rune('0'+index) which silently breaks with 10+ quoted phrases. Use fmt.Sprintf for arbitrary index support. * fix: validate and normalize SearchBackend config option Normalize the value to lowercase and fall back to "fts" with a log warning for unrecognized values. This prevents silent misconfiguration from typos like "FTS", "Legacy", or "fts5". * refactor: improve documentation for build tags and FTS5 requirements Signed-off-by: Deluan <deluan@navidrome.org> * refactor: convert FTS5 query and search backend normalization tests to DescribeTable format Signed-off-by: Deluan <deluan@navidrome.org> * fix: add sqlite_fts5 build tag to golangci configuration Signed-off-by: Deluan <deluan@navidrome.org> * feat: add UISearchDebounceMs configuration option and update related components Signed-off-by: Deluan <deluan@navidrome.org> * fix: fall back to legacy search when SearchFullString is enabled FTS5 is token-based and cannot match substrings within words, so getSearchExpr now returns legacySearchExpr when SearchFullString is true, regardless of SearchBackend setting. * fix: add sqlite_fts5 build tag to CI pipeline and Dockerfile * fix: add WHEN clauses to FTS5 AFTER UPDATE triggers Added WHEN clauses to the media_file_fts_au, album_fts_au, and artist_fts_au triggers so they only fire when FTS-indexed columns actually change. Previously, every row update (e.g., play count, rating, starred status) triggered an unnecessary delete+insert cycle in the FTS shadow tables. The WHEN clauses use IS NOT for NULL-safe comparison of each indexed column, avoiding FTS index churn for non-indexed updates. * feat: add SearchBackend configuration option to data and insights components Signed-off-by: Deluan <deluan@navidrome.org> * fix: enhance input sanitization for FTS5 by stripping additional punctuation and special characters Signed-off-by: Deluan <deluan@navidrome.org> * feat: add search_normalized column for punctuated name search (R.E.M., AC/DC) Add index-time normalization and query-time single-letter collapsing to fix FTS5 search for punctuated names. A new search_normalized column stores concatenated forms of punctuated words (e.g., "R.E.M." → "REM", "AC/DC" → "ACDC") and is indexed in FTS5 tables. At query time, runs of consecutive single letters (from dot-stripping) are collapsed into OR expressions like ("R E M" OR REM*) to match both the original tokens and the normalized form. This enables searching by "R.E.M.", "REM", "AC/DC", "ACDC", "A-ha", or "Aha" and finding the correct results. * refactor: simplify isSingleUnicodeLetter to avoid []rune allocation Use utf8.DecodeRuneInString to check for a single Unicode letter instead of converting the entire string to a []rune slice. * feat: define ftsSearchColumns for flexible FTS5 search column inclusion Signed-off-by: Deluan <deluan@navidrome.org> * feat: update collapseSingleLetterRuns to return quoted phrases for abbreviations Signed-off-by: Deluan <deluan@navidrome.org> * feat: implement extractPunctuatedWords to handle artist/album names with embedded punctuation Signed-off-by: Deluan <deluan@navidrome.org> * feat: implement extractPunctuatedWords to handle artist/album names with embedded punctuation Signed-off-by: Deluan <deluan@navidrome.org> * refactor: punctuated word handling to improve processing of artist/album names Signed-off-by: Deluan <deluan@navidrome.org> * feat: add CJK support for search queries with LIKE filters Signed-off-by: Deluan <deluan@navidrome.org> * feat: enhance FTS5 search by adding album version support and CJK handling Signed-off-by: Deluan <deluan@navidrome.org> * refactor: search configuration to use structured options Signed-off-by: Deluan <deluan@navidrome.org> * feat: enhance search functionality to support punctuation-only queries and update related tests Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-02-21 17:52:42 -05:00
Deluan	5fa8356b31	chore(deps): bump golangci-lint to v2.10.0 and suppress new gosec false positives ... Bump golangci-lint from v2.9.0 to v2.10.0, which includes a newer gosec with additional taint-analysis rules (G117, G703, G704, G705) and a stricter G101 check. Added inline //nolint:gosec comments to suppress 21 false positives across 19 files: struct fields flagged as secrets (G117), w.Write calls flagged as XSS (G705), HTTP client calls flagged as SSRF (G704), os.Stat/os.ReadFile/os.Remove flagged as path traversal (G703), and a sort mapping flagged as hardcoded credentials (G101). Signed-off-by: Deluan <deluan@navidrome.org>	2026-02-17 09:28:42 -05:00
Deluan Quintão	cad9cdc53e	fix(scanner): preserve created_at when moving songs between libraries (#5055) ... * fix: preserve created_at when moving songs between libraries (#5050) When songs are moved between libraries, their creation date was being reset to the current time, causing them to incorrectly appear in "Recently Added". Three changes fix this: 1. Add hash:"ignore" to AlbumID in MediaFile struct so that Equals() works for cross-library moves (AlbumID includes library prefix, making hashes always differ between libraries) 2. Preserve album created_at in moveMatched() via CopyAttributes, matching the pattern already used in persistAlbum() for within-library album ID changes 3. Only set CreatedAt in Put() when it's zero (new files), and explicitly copy missing.CreatedAt to the target in moveMatched() as defense-in-depth for the INSERT code path * test: add regression tests for created_at preservation (#5050) Add tests covering the three aspects of the fix: - Scanner: moveMatched preserves missing track's created_at - Scanner: CopyAttributes called for album created_at on album change - Scanner: CopyAttributes not called when album ID stays the same - Persistence: Put sets CreatedAt to now for new files with zero value - Persistence: Put preserves non-zero CreatedAt on insert - Persistence: Put does not reset CreatedAt on update Also adds CopyAttributes to MockAlbumRepo for test support. * test: verify album created_at is updated in cross-library move test (#5050) Added end-to-end assertion in the cross-library move test to verify that the new album's CreatedAt field is actually set to the original value after CopyAttributes runs, not just that the method was called. This strengthens the test by confirming the mock correctly propagates the timestamp.	2026-02-17 08:37:05 -05:00
Maximilian	a704e86ac1	refactor: run Go modernize (#5002)	2026-02-08 09:57:30 -05:00