mirror of
https://github.com/navidrome/navidrome.git
synced 2026-07-30 16:56:22 -04:00
dependabot/github_actions/dot-github/workflows/actions/setup-node-7
677 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Deluan Quintão
|
5927e693d1 |
feat(jellyfin): filter items by year and record label (#5817)
* feat(persistence): add AlbumRepository.GetYears for distinct album years * test(persistence): verify GetYears de-duplicates repeated years Regression test that adds two albums with the same non-zero max_year (2005) and verifies that GetYears() returns that year exactly once, ensuring the SQL DISTINCT clause is applied correctly. Catches any future removal of DISTINCT from the GetYears query. * feat(jellyfin): add legacy /Items/Filters endpoint (genres + years) * feat(jellyfin): add /Studios endpoint from record label tags * refactor(persistence): drop duplicate columns in tagRepository.GetAll * fix(jellyfin): exclude missing albums from filter years GetYears only filtered max_year > 0, so albums whose files were all removed (missing=true, kept when Scanner.PurgeMissing=never) contributed stale years to /Items/Filters. Filter them out like the normal album listings do. Also return an empty slice from the MockAlbumRepo to match the real repository. * feat(jellyfin): filter /Items by Years= * feat(jellyfin): filter /Items by StudioIds= (record labels) * feat(jellyfin): scope filter and studio lists to ParentId library * refactor(jellyfin): extract parentIDScope and libraryScopeFilter helpers Collapse the three inline resolveLibraryScope(dto.DecodeID(parentid)) call sites and the duplicated empty-scope guard into two small helpers, so the empty-scope=unrestricted contract lives in one place. Reuse the existing names() helper in the Years= e2e test. * feat(jellyfin): expose record labels as album Studios Add a Studios field to the album BaseItemDto, populated from the record-label tags and gated behind Fields=Studios (matching Jellyfin's ItemFields convention). Studio ids reuse the record-label tag identity, so they round-trip with the /Studios list and the StudioIds= filter. Real Jellyfin leaves Studios empty for music; Feishin reads it as the album's record label. |
||
|
Deluan Quintão
|
4efd92cf83 |
feat(server): expose album-level ReplayGain in albums (#5816)
* feat(model): aggregate album ReplayGain in ToAlbum * feat(db): add nullable album ReplayGain columns with backfill * feat(persistence): persist album ReplayGain fields * feat(jellyfin): expose album NormalizationGain from ReplayGain * refactor(model): lazy-init mostFrequentPtr map; clean up RG test rows * fix(db): backfill album ReplayGain with most-frequent value, not max A plain max() picked a minority outlier that diverged from MediaFiles.ToAlbum (which uses the most-frequent value), and GetTouchedAlbums never re-derives an unchanged album, so the wrong value would persist. Reproduce the modal aggregation via grouped CTEs, which also groups media_file once instead of a per-album correlated scan. * refactor(model): return an owned pointer from mostFrequentPtr Avoid aliasing a MediaFile field so the resulting Album is independent of the source slice. |
||
|
Deluan Quintão
|
deaa5e6c02 |
feat(ui): playlist favourites (heart, list filter, sidebar favourites-only toggle) (#5805)
* feat(playlists): register starred REST filter on playlist repository
* feat(ui): add persisted sidebarPlaylistsOnlyFavourites setting
* feat(ui): playlist favourites heart column and list filter
* feat(ui): favourite heart on playlist details header
* feat(ui): sidebar favourites-only playlist toggle with live refresh
* fix(playlists): qualify id in REST filter to avoid ambiguous column
The annotation join in selectPlaylist made a bare id filter ambiguous, so
GET /api/playlist?id=X (react-admin getMany, used by the sidebar refetch and
useResourceRefresh) failed with 'ambiguous column name: id'. Register
idFilter("playlist") like the album/artist/mediafile repos.
* fix(ui): refresh favourites sidebar on local star toggle
The SSE broker skips the client that originated a star, so the acting client
never got the refreshResource echo and its favourites-only sidebar went stale.
Key the sidebar query on a fingerprint of locally-known starred playlists so a
star/unstar on this client refetches; SSE still covers other clients.
* style(ui): prettier-format PlaylistsSubMenu test
* feat(ui): refine playlist favourites layout
- Move the favourite heart column to just before the edit button
- Space the Playlists sidebar header text from its action icons
- Use a list icon instead of a cog for the playlist-management action
* feat(ui): make the playlist Favourite column toggleable
Move the heart into the toggleable fields map so users can show/hide it
from the column selector like the other optional columns, keeping it last
so it stays just before the edit button.
* refactor(ui): memoize sidebar star fingerprint and gate it on favourites-only
- Derive starFingerprint via useMemo on the playlist data reference instead
of recomputing sort/join inside useSelector on every app-wide dispatch.
- Only include the fingerprint in the query payload when favourites-only is
on, so a star toggle no longer refetches the sidebar when it shows all
playlists.
* fix(ui): don't refetch favourites sidebar on SSE events when showing all
When favourites-only is off the sidebar shows every playlist, so a star
event from another client changes nothing visible. Gate the SSE-driven
refresh counter (and its payload key) on onlyFavourites so the sidebar no
longer redraws on unrelated playlist star events.
* fix(ui): address automated review feedback on playlist favourites
- Ignore a persisted favourites-only preference when EnableFavourites is off,
so disabling the feature later can't strand a filtered sidebar (Codex P2).
- Make PlaylistLove's datagrid header props explicit (source/sortable via
defaultProps, className forwarded) instead of relying on prop pass-through.
- Add aria-label to the SubMenu secondary action button.
- Cover the PlaylistLove list column with tests (Codex P1).
|
||
|
Deluan Quintão
|
85132240e0 |
feat(smartplaylist): per-playlist refreshDelay for stable daily/weekly playlists (#5790)
* feat(utils): add ParseDuration/FormatDuration with day and week units * feat(criteria): add per-playlist refreshDelay to smart playlist rules * feat(smartplaylist): honor per-playlist refreshDelay in refresh gate * feat(subsonic): compute smart playlist validUntil from effective refresh delay * fix(playlists): reset smart playlist evaluation window when rules change via API * refactor(utils): flatten FormatDuration recursion, single-pass duration regex * fix(playlists): address PR review feedback - Reset EvaluatedAt to nil instead of zero-time on rules change and NSP re-import, so getPlaylist(s) never reports year-1 Changed/validUntil in the window between an edit and the next owner read - Parse negative d/w durations so they are rejected with the consistent "negative duration" error instead of "unknown unit" - Quote input in the negative-duration error, matching the parse error - Gate per-playlist RefreshDelay behind IsSmartPlaylist, matching its doc |
||
|
Deluan Quintão
|
3d438b08ef |
fix(jellyfin): close the unbounded playlist and search paths left by #5783 (#5784)
* fix(jellyfin): stream playlist tracks instead of loading every one PR #5783 left the playlist paths materializing: all three loaded every track of a playlist, whatever the client asked for. A playlist can be the whole library — a smart playlist matching everything — so this is the same OOM class that PR fixed for the other collections. Measured on a 96k-track smart playlist, /Items?ParentId=<playlist>&Limit=10 peaked at 1.3GB to return ten tracks. Playlist tracks now stream from a cursor, like every other collection: - PlaylistTrackRepository gains GetCursor and CountAll, sharing the select builder with loadTracks so cursor rows hydrate identically, plus GetMediaFileIDs for callers that need every id but no track data. - playlists.Tracks(ctx, id) exposes that repo to the HTTP layer with visibility enforced, returning ErrNotFound rather than the repo's nil-and-log-a-warning (which /Items would hit on every album browse, since ParentId is usually not a playlist). - /Playlists/{id}/Items now honors StartIndex/Limit, which it silently ignored before — it always returned the whole playlist. Real Jellyfin pages it. - getPlaylist runs an id-only query: PlaylistInfo carries every track id so it can't be paged, but it no longer hydrates rows it discards. - The route joins the throttled group, as it's now cursor-backed. Measured against a copy of a 96k-track production DB, peak RSS over idle, with byte-for-byte identical responses on every endpoint: /Items?ParentId=<pl>&Limit=10 1275MB -> 1MB 8.8s -> 3.6s (0.27s warm) /Items?ParentId=<pl> unbounded 1353MB -> 8MB 8.7s -> 3.4s /Playlists/<pl>/Items 1217MB -> 7MB 8.8s -> 3.4s /Playlists/<pl> 1178MB -> 33MB 9.1s -> 3.8s TotalRecordCount now costs a count query where the old path got it from len(tracks): 6ms on the largest real playlist in that library (2638 tracks), 288ms on the synthetic all-96k one. The old path paid 1.3GB and 4s+ instead. * fix(jellyfin): bound unbounded /Items searches The other collections stream, so an unbounded one costs about one item of memory. Search can't: the repositories' Search returns a slice, so it materializes every match. PR #5783 left two ways to reach that. A whitespace-only SearchTerm was the first. " " != "", so it took the search path, where doSearch trims it back to empty and hits its "empty query, return everything in natural order" branch — with no LIMIT, since executeTwoPhase only applies one when Max > 0. The whole library, materialized. Trimming at the two parse sites makes the `search != ""` checks mean what they look like they mean: a blank term is not a search, so it takes the unfiltered streaming path, which still returns everything, exactly as real Jellyfin does for an empty term. A real search with no Limit was the second, and needs an actual bound. Search gets a default of 100 when the client sends no Limit — matching the DefaultSearchLimit in Jellyfin's unreleased SqlSearchProvider — plus a ceiling, without which Limit=999999 would still materialize the library. The default alone wouldn't have closed the hole. An explicit Limit under the ceiling is honored unclamped, as upstream does; truncating a search is safe in a way truncating /Items is not, since nothing syncs a library through searchTerm. Note this is upstream's own bug: v10.11's /Search/Hints returns the entire library for a whitespace-only term, which master fixed by switching to ThrowIfNullOrWhiteSpace. * fix(jellyfin): cap the search Limit the client asked for, not the merge window searchPage sees two different things in opts.Max: the client's Limit for a single-type query, and mergeTypes' internal offset+limit window for a multi-type one. Clamping there hit both, so a multi-type search paging past the ceiling fetched only `ceiling` rows of the first type and the merged page skipped into the next one — IncludeItemTypes=Audio,MusicAlbum&SearchTerm=song&StartIndex=2000 &Limit=1 returned an album instead of the 2001st song. The ceiling now applies where the client's Limit is read: queryItems (after the playlist branch, so a playlist parent's page isn't capped by a stray SearchTerm) and getArtists, which reads its own. A limit of 0 stays 0, keeping searchPage's default. What a deep page materializes is then bounded by the client's StartIndex, as it already was for any multi-type query, search or not. Also from review: the playlist-track mock reused the previously stored Options when called without any, so a later no-args call inherited stale paging. * fix(jellyfin): apply the search default to the client's Limit, not per type The default lived in searchPage, which runs per type and after mergeTypes has already picked its branch. So an unbounded multi-type search left q.limit at 0, mergeTypes took its chained branch, and StartIndex was applied to a list each type had already truncated to the default — dropping matches rather than paging them. With 200 matching songs, IncludeItemTypes=Audio,MusicAlbum&SearchTerm=song &StartIndex=150 returned nothing at all, and without StartIndex it returned the default per type instead of in total. clampSearchLimit now applies the default and the ceiling together, to the client's Limit, at the two places it's read (queryItems and getArtists). A search therefore always gives mergeTypes a real window, so it pages the merged result and cuts it once, at the end. * fix(jellyfin): bound the multi-type search window against StartIndex mergeTypes asks each type for offset+limit rows before paginating the merged list, so a search still materialized whatever StartIndex asked for: IncludeItemTypes=Audio,MusicAlbum&SearchTerm=x&StartIndex=500000&Limit=1 pulled ~500001 matches per type. Bounding the window alone isn't enough — below it the merged rows are the client's page, but at it a truncated type is followed by the next one's rows, which is what made a clamped window serve an album where the 2001st song belonged. So the window is capped at maxSearchLimit and the page is clipped to it: pages below the ceiling are served in full and unchanged, a page straddling it is cut at it, and past it the result is empty rather than another type's rows. The total reports what can actually be paged to, so a client stops instead of asking for pages that no longer exist. This is the merge's own limit, not the client's: a single-type search still pages as deep as it likes, since its offset goes to SQL. Non-search multi-type queries keep the unbounded offset+limit window, which predates this and wants the same treatment via CountAll (exact per-type totals let whole types be skipped) rather than a cap. * fix(jellyfin): advertise the pageable search total, not the page window Clipping the multi-type search total to `window` clipped it to StartIndex+Limit on an ordinary page, so a first page of Limit=10 reported TotalRecordCount 10 however many matches there were, and a client paging on the total stopped after one page. The cap belongs at the ceiling — what can be paged to overall — not at the current page. The tests missed it because the only one asserting a total used StartIndex=maxSearchLimit, where the window happens to equal the ceiling. * refactor(jellyfin): fold the search clamp into clampLimit and simplify mergeTypes Cleanup pass over the branch, no behaviour change: - clampSearchLimit was clampLimit (similar.go) with different constants, so the latter takes the default and ceiling as arguments and both call it. Similar's default moves out of three IntOr calls into defaultSimilarLimit. - mergeTypes derived a second `limit` and needed an early return for the empty page, only because paginate reads 0 as "unbounded". Clipping the merged slice to the window instead lets q.limit be passed straight through: window is min(offset+limit, ceiling), so clipping there is the same cut. - playlistTracks returned (result, handled, error) where handled=false always meant error=nil. Splitting the lookup out gives playlistTracksRepo returning (repo, ok), and the nilerr suppression goes with it. - The comment on playlists.Tracks blamed the log warning for its extra Get; the reason is that PlaylistRepository.Tracks discards the error behind a nil. Also drops a stale reference to a renamed variable. |
||
|
Deluan Quintão
|
53d54baef0 |
fix(jellyfin): stream collection responses to prevent OOM on large libraries (#5783)
* fix(jellyfin): stream /Items responses to prevent OOM on large libraries
Finamp's library sync issues an unbounded GET /Items?IncludeItemTypes=Audio
with Fields=MediaSources and no Limit. On a large library this built the whole
result set — every MediaFile and every BaseItemDto, each fat with MediaSources —
in memory, and json.Encoder then buffered the entire ~200MB response before
writing a byte. Measured on a 96k-track library, one such request peaked near
1.6GB RSS; in a memory-limited container the resulting slowness made clients
retry, stacking concurrent full-library builds until the process was OOM-killed.
Stream the song listing straight from a DB cursor (MediaFileRepository.GetCursor),
mapping and encoding one row at a time, so peak memory is bounded to about one
item regardless of library size. The cursor is opened lazily, when streaming
begins, so it doesn't hold a DB connection across the CountAll and ServerId
lookups that run first (ServerId can write on first use and would deadlock
against an open reader). Pagination still works — the cursor query carries
LIMIT/OFFSET/ORDER BY from StartIndex/Limit/SortBy — and TotalRecordCount stays
the full count. Other materialized responses stream their JSON too, avoiding the
encoder's buffer-the-whole-output cost.
Also bound artwork image concurrency with the same server.ThrottleBacklog that
Subsonic's getCoverArt uses, so a burst of image requests during sync can't
exhaust memory through concurrent decode/resize.
Verified against a copy of a 96k-track production database: byte-for-byte
identical response, peak RSS 1593MB -> 53MB, time-to-first-byte 8.1s -> 0.2s.
* fix(jellyfin): fail loudly on /Items streaming errors instead of a truncated 200
Addresses code review: a streamed /Items response commits HTTP 200 before an
error can occur, so failures were surfacing as misleadingly successful bodies.
- A cursor-open failure (e.g. a busy DB under connection pressure) is now
surfaced before the first byte is written: the cursor open is deferred into
the item source and run in writeItems after the ServerId lookup but before the
envelope, returning a clean 500 the client can retry — not a 200 with an empty
item list.
- A mid-stream (row-scan) error now aborts without closing the JSON envelope,
leaving the body malformed. A truncated-but-valid response would let a sync
client (Finamp) treat the short list as the whole library and prune local
tracks; malformed JSON forces the client's parser to fail and retry.
* refactor(jellyfin): stream every collection endpoint from a DB cursor
Streaming was applied only to the song listing, which left two write paths, two
ServerId stamping mechanisms and a listXxx return-type split ("songs is
special") that made the code hard to follow.
Add GetCursor to the album, artist, genre and playlist repositories, mirroring
MediaFileRepository.GetCursor: same select builder as GetAll, so each cursor
yields identical, fully-hydrated rows (hydration happens per-row in PostScan,
and toModels is only a deref loop). The playlist cursor keeps GetAll's
owner/public visibility filter. Each repository test asserts the cursor yields
exactly what GetAll returns, including Max/Offset.
With cursors everywhere, every listXxx returns itemsResult and every collection
streams through one writer:
- listAlbums, listArtists and listPlaylists now stream from their cursors;
search paths stay materialized (Search returns a slice).
- listGenres stays materialized: its total is the length of the full list and it
paginates in memory, so there is nothing for a cursor to page over.
- api.ok's QueryResult case delegates to writeItems, so the ~9 inline callers
funnel into the same writer; streamQueryResult and wrapResult are gone.
- /Items/Latest streams as a bare JSON array (its Jellyfin wire shape) via
writeItemsArray, which shares the item loop and stamping. That also closes the
same unbounded hole /Items had: limit=0 disabled its LIMIT.
- ServerId is now stamped in exactly one place, so api.ok only handles single,
non-collection payloads.
Verified against a copy of a 96k-track production database: every endpoint
byte-for-byte identical to master. Unbounded /Items peak RSS 1691MB -> 54MB;
time-to-first-byte 6.3s -> 0.19s, /Artists 1.14s -> 0.13s.
* refactor(jellyfin): route every response through api.ok
Handlers were split between api.ok and api.writeItems with no clear rule for
which to call. api.ok now accepts itemsResult too, so it is the single entry
point: callers hand it whatever they have and it routes collections (cursor
-backed or materialized) to the streaming writer. writeItems is reached only
through api.ok now. The one exception is /Items/Latest, which returns a bare
JSON array rather than a QueryResult envelope and so writes directly — noted in
both doc comments.
Also drop GenreRepository.GetCursor: listGenres derives its total from the
length of the full list and paginates in memory, so there is nothing for a
cursor to page over, leaving the method unused.
* fix(jellyfin): stream the unbounded multi-type /Items merge
The multi-type merge capped each per-type query at offset+limit only when a
Limit was given. Without one (Finamp's favorites screen sends multi-type), every
type ran an unbounded query and collect() drained each cursor into a slice — so
/Items?IncludeItemTypes=MusicAlbum,Audio with no Limit materialized every album
and every song, the same OOM class this branch set out to fix. The comment
claiming the set was "capped at offset+limit per type" was wrong for limit=0.
Without a limit the merged page is just each type's rows in order minus the
first offset, which is exactly what chaining the per-type cursors yields, so
stream that instead of merging in memory. The bounded path is unchanged: with a
Limit each type holds at most offset+limit rows, so merging and paginating
across the combined list is safe. Cursors are opened one at a time (each pins a
DB connection until drained), with the first opened eagerly so the usual failure
is still a clean error before any byte is written.
Measured on a 96k-track library, /Items?IncludeItemTypes=MusicAlbum,Audio with
no Limit (103,393 items): peak RSS 612MB -> 53MB, time-to-first-byte 4.9s ->
0.6s, response byte-for-byte identical.
* refactor(jellyfin): parse /Items params into a struct
The /Items dispatcher was a single 90-line block that parsed a dozen params and
then threaded them positionally through three layers — queryItemsOfType took 11
arguments, listSongs and listAlbums 9 each — so reading any one of them meant
decoding a long argument list.
Parse once into an itemsQuery and pass that instead. queryItems now reads as the
four things it actually does: parse, the id/playlists-folder/playlist-parent
special cases, single-type dispatch, multi-type merge — with the playlist-parent
and merge bodies moved to playlistTracks and mergeTypes. Every listXxx takes
(ctx, opts, q).
No behavior change: parsing, ordering and the entityParent rule are unchanged,
and /Artists still passes favOnly=false explicitly by building the subset of the
query it uses.
* docs(jellyfin): trim comments on the streaming path
Cut the comments back to the non-obvious why: the deferred cursor open (the
ServerId write would deadlock against an open reader), the deliberate malformed
JSON on a mid-stream error, why chained opens one cursor at a time, why
listGenres stays materialized, and why the cursor helpers take the underlying
func type. Dropped the rest — restatements of the code, doc comments that
repeated a test's own name, and the GetCursor interface comments that the
existing MediaFileRepository.GetCursor does without.
* refactor(persistence): fold the cursor wrappers into a generic wrapCursor
wrapAlbumCursor, wrapArtistCursor, wrapMediaFileCursor and wrapFolderCursor were
the same twelve lines four times over, differing only in the type name, and the
playlist cursor had its own inline copy of the loop.
Add one generic wrapCursor. It takes an extractor func rather than a method on
an interface: a type parameter can't reach an embedded field, and methods that
only satisfy a generic constraint are reported by the unused linter. The
extractor also lets both type parameters be inferred, so call sites need no
explicit instantiation. Each entity keeps its named wrapper as a one-liner,
since the model cursor types are defined types and need the conversion — and the
existing wrapper tests call them directly.
The nil-row error now names the model type via %T ("unexpected nil model.Album")
instead of a hand-written per-entity string; the three tests asserting that
message are updated.
* fix(jellyfin): bound concurrent collection streams
A streamed collection holds a DB cursor — and its pooled connection — for the whole client-paced
response, where the old materialize-then-write path released it as soon as the query finished. The
pool is shared with the scanner, Subsonic, the native API and the UI, so enough slow clients take
every connection and everything else blocks waiting for one. Measured against a copy of a 96k-track
production DB, 20 concurrent unbounded streams against a pool of 16 stalled a write for 16.2s (the
other 14 writes in the run took ~2ms — the signature of connection starvation, not lock contention).
Cap concurrent streams at half the pool. Excess requests queue rather than fail, so no client is
rejected: the same 20 streams now all complete and the worst write is 2.5ms.
- conf.MaxOpenConns() now owns the pool sizing (db calls it). It belongs in conf: the pool is a
tunable, db already imports conf, and putting it in db would force server/jellyfin to import db
just to size the cap against it. Expressing the cap as MaxOpenConns()/2 also keeps the two from
drifting apart.
- Uses chi's ThrottleBacklog, not server.ThrottleBacklog: the latter buffers the whole response to
release its token early, which is right for artwork but would undo the streaming. chi's panics on a
non-positive limit, so throttleStreams guards it — setting MaxConcurrentStreams=0 disables the cap
instead of crashing the server at startup.
* refactor(jellyfin): move throttleStreams to middlewares.go
It's a middleware, so it belongs beside normalizeQueryKeys, authenticate and
withPlayer rather than in api.go. Its tests move to middlewares_test.go with it,
keeping one test file per production file.
* fix(jellyfin): abandon the scan when the client stops reading
encodeItems discarded its write errors and relied on the final Flush to report
them, so once bufio's buffer filled and the flush failed, the loop still pulled
every remaining row through the cursor and serialized it for a client that was
gone. A test with a failing writer confirms it: all 20k items were drained.
That wastes CPU, and holds the cursor's pooled DB connection and a stream slot
(now a capped resource) for the length of a full scan nobody is reading. Check
the per-item writes so the first failure ends the scan; the fixed envelope
writes stay unchecked, since bufio latches for them anyway.
|
||
|
Deluan Quintão
|
fe6ac2e577 |
feat(jellyfin): experimental Jellyfin Music API support (#5730)
* feat(sharing): enable sharing by default
Flip the EnableSharing default from false to true so new installations have the sharing feature available out of the box. Users can still disable it via the EnableSharing config option.
The native API only registers the /share route when sharing is enabled, so the nativeapi tests that build the router without wiring a share service now explicitly disable sharing in their setup to avoid registering a route backed by a nil service.
* feat(jellyfin): add config flag and URL path constant
Adds the disabled-by-default Server.Jellyfin config option (Enabled,
ServerName) and consts.URLPathJellyfinAPI, following the existing
LastFM/ListenBrainz patterns. Later tasks will use these to mount the
Jellyfin-compatible API router.
* fix(jellyfin): default Jellyfin ServerName to "Navidrome"
* feat(jellyfin): package skeleton with System handshake endpoints
Adds server/jellyfin: the Router (mirrors server/subsonic and
server/public), its Wire-friendly New(...) constructor, a chi routes()
table, and the ok() JSON response helper. Implements the unauthenticated
handshake surface Jellyfin clients probe first: GET /System/Info/Public,
GET+POST /System/Ping, and GET /QuickConnect/Enabled (quick connect is
unsupported, so it always reports disabled).
The public info payload's Id must be stable across restarts (Jellyfin
clients cache ServerId), so it reuses the get-or-create Property pattern
already used for InsightsID in core/metrics/insights.go: fetch
consts.JellyfinServerIDKey from the Property repository, generating and
persisting a new UUID on first read. A dedicated key (rather than the
existing InsightsID) keeps the anonymous telemetry identifier from being
exposed on this unauthenticated endpoint.
* test(jellyfin): cover ping and quickConnectEnabled handlers
Both were added alongside the System/Info/Public handshake endpoint but
had no direct test coverage.
* fix(jellyfin): memoize stable server Id and cover get-or-create path
* feat(jellyfin): wire and mount the router behind Jellyfin.Enabled
Add jellyfin.New to the shared Wire provider set and a
CreateJellyfinAPIRouter injector, then mount the router at /jellyfin
in startServer(), gated by conf.Server.Jellyfin.Enabled, mirroring the
existing LastFM/ListenBrainz router mounts.
* feat(jellyfin): add Jellyfin DTOs and model mappers
Adds BaseItemDto, QueryResult, UserItemDataDto, UserDto,
AuthenticationResult, MediaSourceInfo, PlaybackInfoResponse,
NameGuidPair and SessionInfo DTOs to server/jellyfin/dto, plus
model-to-DTO mappers for songs, albums, artists and genres that
later browse/stream/write endpoints will consume.
* test(jellyfin): cover GenreToBaseItem mapper
* feat(jellyfin): advertise Jellyfin-compatible version, brand ServerName with Navidrome version
* fix(jellyfin): map LastPlayedDate and omit zero track/disc numbers
* feat(jellyfin): authentication middleware and AuthenticateByName login
* fix(jellyfin): reject empty login password, wire ServerName, add negative auth tests
* refactor(jellyfin): use new(x) builtin instead of intPtr helper
* feat(jellyfin): user views and current-user endpoints
* feat(jellyfin): Items query engine, item detail, and latest
Adds the /Items universal query endpoint, dispatching by IncludeItemTypes
over albums/artists/songs/genres with ParentId, SearchTerm, Filters=IsFavorite,
SortBy/SortOrder and StartIndex/Limit support, plus GET /Items/{itemId} and
/Users/{userId}/Items/Latest. Reuses server/subsonic/filter builders (by
artist/album/starred) instead of hand-rolled squirrel filters, and resolves
sort keys per item type since each repository maps sort names to different
real/aliased columns.
Also adds the missing CountAll to tests.MockArtistRepo, exposed by this task
(the mock embedded a nil ArtistRepository for it and would panic on use).
* refactor(server): extract shared query filter builders to server/filter
Moves server/subsonic/filter to server/filter so server/jellyfin can use
the shared query-option builders without importing server/subsonic,
enforcing the rule that no API package imports another API's package.
* feat(jellyfin): full multi-library support with per-user access scoping
Replace the single hardcoded "music" UserView with one CollectionFolder
view per library the user can access, and scope every /Items browse
query (albums, songs, artists, /Latest) to those libraries via
server/filter's ApplyLibraryFilter/ApplyArtistLibraryFilter.
ParentId is now disambiguated: a numeric value the user has access to is
treated as a library scope (browsing a UserView), otherwise it falls
through as an entity id (artist/album), which safely matches nothing
rather than leaking another library's content. getItem now 404s when
fetching an album or song outside the user's accessible libraries;
artists are skipped (they can span multiple libraries) with a TODO.
Genres remain unscoped since they're global tags, not per-library
entities.
* test(jellyfin): cover admin library-access path and drop nil test contexts
* feat(jellyfin): Artists and Genres endpoints
Add /Artists, /Artists/AlbumArtists, /Genres and /MusicGenres. Artists
listing is library-scoped (defaulting to the user's accessible
libraries, narrowed by an accessible ParentId), delegating access
control to listArtists/ApplyArtistLibraryFilter. Genres are global and
unscoped, matching the ML decision already made for listGenres.
* refactor(jellyfin): share resolveLibraryScope between items and artists
* feat(jellyfin): item image endpoint via artwork service
* fix(jellyfin): let net/http sniff image Content-Type instead of forcing jpeg
* feat(jellyfin): audio streaming and PlaybackInfo with library access control
* feat(jellyfin): favorites and rating write-back with library access control
Adds POST/DELETE handlers for /Users/{userId}/FavoriteItems/{itemId} and
/Users/{userId}/Items/{itemId}/Rating. A shared resolveAnnotated helper
probes album/artist/media file (mirroring getItem's order) and 404s before
writing if the user lacks access to the album/song's library; artists are
exempt since they span multiple libraries. Ratings are halved coming in
(Jellyfin 0-10 -> Navidrome 0-5) to match the doubling in dto.UserData.
Also teaches MockMediaFileRepo and MockArtistRepo's SetStar/SetRating (and
MockAlbumRepo's, previously a no-op) to actually mutate the backing data so
write-back can be asserted in tests.
* feat(jellyfin): playback reporting and scrobbling
Add /Sessions/Playing[/Progress|/Stopped] and /Sessions/Capabilities[/Full]
handlers, backed by core/scrobbler.PlayTracker. A new withPlayer middleware
resolves/registers a model.Player from the Emby DeviceId header (used
directly as the stable player id, unlike Subsonic's cookie fallback) and
injects it into the request context for scrobbling.
The Stopped report calls ReportPlayback with IgnoreScrobble to end the
now-playing session without double-counting, then calls Submit as the
single source of the play-count increment and external scrobble.
* feat(jellyfin): playlist read and write-back
Adds POST /Playlists, GET/POST/DELETE /Playlists/{id}/Items. Tags each
playlist item with PlaylistItemId (the entry's position within the
playlist) so DELETE .../Items?EntryIds=... can remove a specific
occurrence by the id core/playlists.RemoveTracks actually expects,
rather than the song id used everywhere else.
* test(jellyfin): cover empty Ids/EntryIds in playlist add/remove
* feat(jellyfin): unknown-route logging, generic 500s, rating clamp, docs
Hardening pass ahead of real client testing: unmatched routes and
unsupported methods now return a logged, JSON 404 instead of chi's
default plain-text response, so a missing endpoint a client needs is
easy to spot in the logs. Internal errors (ffmpeg output, file paths,
etc.) no longer leak into 500 response bodies -- a shared
internalError helper logs the real error server-side and always
returns a generic message. Inbound Jellyfin ratings are clamped to
0-10 before being halved into Navidrome's 0-5 scale, and /System/Ping
now replies with a bare plain-text body as real Jellyfin servers do.
Adds a README with an enable/curl walkthrough and known limitations.
* docs(jellyfin): clarify public image endpoint and accessibleLibraryIDs comments
* fix(jellyfin): case-insensitive path routing for Jellyfin client compatibility
* refactor(server): extract case-insensitive path routing to a shared helper
* fix(jellyfin): return User Policy and Configuration so Finamp completes login
* fix(jellyfin): support Playlist type, multi-type Items queries, and PlayCount/DatePlayed sort
Real Finamp requests break against three /Items query engine bugs: Playlist
requests fell through to albums, multi-type IncludeItemTypes (e.g. Finamp's
favorites screen) only returned the first requested type, and comma-separated
SortBy lists (e.g. "DateCreated,SortName") were matched as one opaque string
so they always fell through to the repo default.
Also extends tests/mock_playlist_repo.go with SetData/GetAll so playlist
listing is testable like the other mock repos.
* feat(jellyfin): implement /socket WebSocket for real-time client sessions
* fix(jellyfin): resolve library-view ids in getItem so clients can load the library
* fix(jellyfin): serve direct file at /Items/{id}/File and accept ApiKey query param
* fix(jellyfin): resolve playlist ids in getItem
* fix(jellyfin): read lowercase ids/entryIds playlist params and add playlist-users endpoints
* fix(jellyfin): include MediaSources with Size/Bitrate on tracks so clients show download size
* fix(jellyfin): populate all required MediaSourceInfo bool/array fields to match Jellyfin
* fix(jellyfin): hex-encode item ids at the API boundary for Jellyfin client compatibility
Finamp (and presumably other clients) parses ids as radix-16, but Navidrome's
base62 nanoids aren't valid hex and crash its queue packing. Hex-encode every
id emitted by the Jellyfin API and decode every id received, keeping the
transform reversible and stateless at the boundary rather than touching any
model id.
* fix(jellyfin): populate MediaStreams with the audio stream so clients can size/transcode
* fix(jellyfin): support Ids batch-fetch in /Items so clients can fetch items by id
* fix(jellyfin): do not disable Jellyfin server when disabling external services
* fix(jellyfin): emit per-image ImageBlurHashes so clients de-dupe images and stop warning
* fix(jellyfin): support playlist cover upload/delete and display via item image endpoints
* feat(jellyfin): implement GET /Playlists/{id} for playlist visibility
Finamp's playlist edit screen calls GET /Playlists/{id} to read the
OpenAccess (public visibility) flag; we only had the sub-routes, so it
404'd and the edit screen failed to load. Return the Jellyfin PlaylistDto
shape (OpenAccess from Public, empty Shares, media item ids).
* fix(jellyfin): display playlist cover art
Uploaded playlist covers never showed in clients for two reasons: the
playlist BaseItemDto advertised no Primary ImageTag (so clients didn't
know to fetch a cover), and the public image endpoint resolved artwork
under the request's anonymous context, so a private playlist failed its
visibility filter and fell back to the placeholder. Advertise the Primary
image tag/blurhash on playlists, and resolve artwork under an elevated
context (as core/artwork's cache warmer does).
* fix(jellyfin): expand album/artist/playlist ids when building playlists
Jellyfin clients (Finamp) send container ids — an album, artist or
playlist — in a playlist's Ids list and expect the server to expand each
into its child tracks. core/playlists only understands media file ids, so
creating or adding with an album id silently produced an empty playlist.
Expand container ids to their tracks (in order) before create/add; bare
song ids still pass through. Adds filter.SongsByArtistID.
* fix(jellyfin): support playlist deletion via DELETE /Items/{id}
Finamp deletes a playlist with DELETE /Items/{id}, which we didn't route,
so deletion silently failed. Implement it via core/playlists.Delete (which
enforces ownership and removes the cover file). Only playlists are
deletable through this API; non-playlist ids return 404, non-owners 403.
* test(jellyfin): add e2e suite harness + smoke tests
* test(jellyfin): e2e for system, auth, routing, browsing, annotations
* test(jellyfin): e2e for playlists (CRUD, expansion, cover) and item images
* test(jellyfin): e2e for streaming, sessions, and multi-user access control
* fix(jellyfin): artist search 500 (library filter leaked into FTS query)
getArtists/listArtists reused the browse-path filters (notMissing +
ApplyArtistLibraryFilter) for the search path, but artist Search expects a
sole Eq{library_id} filter it can consume as a scope — artists have no
library_id column, so the compound/join filter leaked into the FTS query
and 500'd. Every Finamp artist search failed (the filter is applied even
unscoped, since admins resolve to all library ids). Build search filters
separately. Adds e2e search coverage.
* feat(jellyfin): implement POST /Playlists/{id} to update name, visibility, tracks
Finamp edits a playlist (make public, rename, reorder) via POST
/Playlists/{id}, which we didn't route, so every edit 404'd. Implement it:
Ids present -> replace track list (Create with existing id, preserving
name); otherwise update Name/IsPublic via core/playlists.Update. Adds a
shared playlistError helper (403/404/500) reused by deleteItem, plus e2e
coverage.
* docs(jellyfin): update README for playlists, images, id encoding, and e2e
* fix(jellyfin): default album track listing to track order
Browsing an album's tracks (Items?ParentId=<albumId>&IncludeItemTypes=Audio)
took only SongsByAlbum's filters and dropped its Sort, so tracks came back
in arbitrary order. Default opts.Sort to the album (disc+track) order when
browsing an album without an explicit SortBy — matching Subsonic's GetAlbum
and real Jellyfin. An explicit SortBy still wins.
* fix(jellyfin): filter items by AlbumArtistIds/ArtistIds
An artist's page in Finamp sends ParentId=<libraryId> (scoping) plus
AlbumArtistIds/ArtistIds/contributingArtistIds for the artist itself, but
queryItems only honored ParentId, so an artist's albums and tracks came
back unfiltered (every artist's content). Parse the artist-id params and
apply AlbumsByArtistID (albums) / SongsByArtistID (tracks). Adds e2e
coverage.
* fix(jellyfin): only count a play past the scrobble threshold
reportPlaybackStopped set IgnoreScrobble and force-submitted a play on
every Stopped report, so a briefly-played track (e.g. an immediate skip)
was marked played. Finamp sends Stopped on every track switch, so the
threshold must be applied server-side. Let ReportPlayback's StateStopped
logic decide (play + scrobble only past 50% of the track / 4-minute cap)
instead. Subsonic differs because there the client gates submission.
* fix(jellyfin): sort album tracks by track number when client sends IndexNumber SortBy
Finamp's album view requests SortBy=ParentIndexNumber,IndexNumber,SortName
(disc, track, name), but applySort didn't recognize ParentIndexNumber or
IndexNumber and fell through to SortName, sorting tracks alphabetically by
title. Map both keys to the album (disc+track) sort. The reversed-title
fixture lets the e2e tell track order from title order.
* fix(jellyfin): honor the isFavorite query param for favorites filtering
Finamp's artist 'Favourite tracks' widget requests favorites via the
standalone isFavorite=true query param, not Filters=IsFavorite, so the
filter was ignored and non-favorited tracks were returned. Detect both
forms. (The widget's reshuffling is Finamp's explicit SortBy=Random.)
* feat(jellyfin): emit DateCreated (Date Added) on items
BaseItemDto had no DateCreated, so clients showed 'No Date Added' and had
nothing to sort 'Recently Added' by. Emit it as ISO 8601 from each entity's
CreatedAt (the same field the recently_added sort uses) for songs, albums
and artists.
* fix(jellyfin): set ArtistItems/AlbumArtists on songs (Now Playing artist)
SongToBaseItem only set Artists (names) and AlbumArtist, not the structured
ArtistItems/AlbumArtists. Finamp's Now Playing screen reads ArtistItems and
shows 'Unknown Artist' when it's absent. Populate both (track artist and
album artist) as name+id pairs, mirroring AlbumToBaseItem.
* docs(jellyfin): note synthetic blurhash as a follow-up
Document that ImageBlurHashes are derived from the item id (a solid-color
placeholder), not computed from the cover art like real Jellyfin, and
outline what a proper implementation would take.
* docs(jellyfin): note WebSocket events and favourited playlists as follow-ups
* fix(jellyfin): filter the Artists page by role (album artist vs performer)
/Artists and /Artists/AlbumArtists both called the same role-agnostic
handler, so Navidrome's per-role artist entries (composers, arrangers,
performers) all showed as Album Artists, and the two tabs were identical.
Filter /Artists/AlbumArtists to RoleAlbumArtist and /Artists to RoleArtist
(and the MusicArtist browse to album artists), via filter.ArtistsByRole.
Verified live: Beatles Singles' album-artists dropped 138->5, composers
excluded, performers (Billy Preston) show only under /Artists.
* fix(jellyfin): sort playlists by name (missing Playlist sort mapping)
applySort had no sortColumnsByType entry for the Playlist type, so
SortBy=SortName was ignored and the Playlists screen showed them in the
repo's default order. Map SortName/Name -> name (as Subsonic does) and
DateCreated -> created_at. Verified live: case-insensitive alphabetical.
* fix(jellyfin): read query params case-insensitively (Jellify support)
Jellify (and the official Jellyfin TypeScript SDK) send query params in
camelCase (parentId, albumArtistIds, artistIds, includeItemTypes), where
Finamp sends PascalCase. Our handlers read fixed-case keys, so every
Jellify filter/sort/paging param was silently dropped:
- an artist's page listed albums and tracks from all artists
- opening an album listed every album instead of its tracks
Real Jellyfin binds query params case-insensitively (ASP.NET model
binding), so add a normalizeQueryKeys middleware that folds every query
key to lowercase once, and read params by their lowercase name. This
also removes the scattered dual-case hacks (Ids/ids, IsFavorite,
ContributingArtistIds, api_key/ApiKey, queryParam) that let this bug
class through.
Also infer the child type from an album parent: Jellify browses an album
with only parentId (no IncludeItemTypes), and Jellyfin infers Audio from
the parent; without it we fell back to listing all albums.
Verified live against Finamp+Jellify and with 4 new e2e specs replaying
Jellify's exact request shapes.
* fix(jellyfin): advertise LocalAddress in the public system info handshake
Jellify (and other @jellyfin/sdk clients) that connect by raw address fall
back to HTTP when TLS isn't available, then adopt the handshake's
LocalAddress as their server base URL. We never populated it, so Jellify's
SDK `api` object was undefined and sign-in crashed with "Cannot read
property 'configuration' of undefined" (getUserApi(api!) with an undefined
api). Over an HTTPS connection Jellify uses connectionType=hostname and
never reads LocalAddress, which is why it worked while an HTTPS proxy was
in front.
Populate LocalAddress from the request — scheme + host (honoring
X-Forwarded-*), plus the /jellyfin mount path — matching real Jellyfin,
which always sends it. Export server.ServerAddress for the resolution.
* fix(jellyfin): separate "Featured On" from an artist's own discography
Jellify's artist page fetches the discography via albumArtistIds and the
"Featured On" section via contributingArtistIds, relying on the server to
return disjoint sets. We collapsed albumArtistIds/artistIds/
contributingArtistIds into one AlbumsByArtistID filter, so an artist's own
albums appeared in both sections.
Add AlbumsByContributingArtistID — albums where the artist is a track
artist but NOT the album artist — matching Jellyfin's ContributingArtistIds
(in Artists, not in AlbumArtists), and route contributingArtistIds to it.
* fix(jellyfin): accept a bare Authorization token for audio streaming
Jellify's native player (react-native-nitro-player) authenticates the audio
stream by setting a bare Authorization header carrying the raw access token
({ AUTHORIZATION: api.accessToken }), not the "MediaBrowser ... Token=" scheme
parseEmbyAuth understands. tokenFromRequest didn't recognize it, so every
/Audio/{id}/stream request from the native player 401'd and no audio played
(the JS client still optimistically posted playback progress, masking it).
Accept a bare (or "Bearer <token>") Authorization header, as real Jellyfin
does. Verified live: the native player's exact request now streams 200.
* fix(jellyfin): embed a self-authenticating stream URL in PlaybackInfo
Jellify's native audio player (react-native-nitro-player / ExoPlayer) fetches
the stream without forwarding any auth — captured request headers were only
Connection/Icy-Metadata/Accept-Encoding/User-Agent, no Authorization and no
api_key — so every /Audio/{id}/stream request 401'd and nothing played (the
JS client still optimistically posted progress, masking it).
Real Jellyfin returns stream URLs with the token embedded; we returned none,
so Jellify fell back to a token-less DirectPlay URL. Populate
MediaSources[0].TranscodingUrl with /Audio/{id}/universal?api_key=<caller
token>, which Jellify's player uses verbatim. Direct-play clients (Finamp
builds its own /Items/{id}/File?ApiKey URL) ignore the field, so they're
unaffected.
* feat(jellyfin): serve per-item UserData (GET /UserItems/{id}/UserData)
Jellify fetches this per item to render played/favourite indicators; we 404'd
it. Resolve the item via the existing getItem resolver (which loads the
caller's annotations and enforces the same library-access gate) and return its
UserData, falling back to an empty-but-valid object for items without
annotations (e.g. playlists). Also registers the legacy
/Users/{userId}/Items/{itemId}/UserData spelling.
* refactor(jellyfin): drop unused bare-Authorization token support
Added mid-troubleshooting on the theory that Jellify's native player sends a
bare Authorization header, but the debug capture showed it sends no auth header
at all — the real fix was embedding api_key in PlaybackInfo's TranscodingUrl.
No client reaches this path (Jellify JS uses the MediaBrowser scheme, Finamp
uses X-Emby-Token, the native player uses the api_key URL), so remove it and
its tests per YAGNI. Effectively reverts
|
||
|
Deluan Quintão
|
ca27335d06 |
feat(playlists): per-user starred/rating annotations (backend) (#5749)
* feat(playlists): add average_rating column to playlist table
* feat(playlists): store and read per-user starred/rating annotations
* feat(playlists): clean up annotations when a playlist is deleted
* feat(subsonic): route star/unstar of a playlist to the playlist repository
* feat(subsonic): route setRating of a playlist to the playlist repository
* test(subsonic): guard that playlist responses never expose annotations
* fix(playlists): clean stale mis-typed annotations on upgrade; cover GetAll read-back
* fix(playlists): scope annotation join by item_type and harden delete
Address code-review findings on the playlist-annotations branch:
- withAnnotation: add an item_type predicate to the LEFT JOIN so a
mis-typed annotation row sharing an id can no longer leak into (or
duplicate) another entity's read. Correct for every caller since each
repo writes annotations with item_type = tableName. Regression test added.
- migration: reclassify legacy media_file-typed rows for playlist ids to
item_type='playlist' (instead of deleting them), preserving users' prior
playlist star/rating; run before the average_rating backfill so those
ratings are included.
- playlist Delete: replace the per-request full-table cleanAnnotations()
anti-join with a targeted, permission-safe (rows-affected gated),
best-effort delete so a cleanup failure no longer misreports an
already-committed delete as an error.
- MockPlaylistRepo: implement GetAll/IncPlayCount/ReassignAnnotation to
remove the dead All field and the nil-interface panic traps.
- test: use slices.IndexFunc instead of a hand-rolled find loop.
* feat(playlists): streamline playlist deletion by relying on annotation sweep
* docs(playlists): trim comments in annotation migration and test
Condense the verbose comments added in this branch per the project's
comment-minimalism guideline, keeping only the non-obvious rationale.
The migration's reclassify block is shortened while preserving the safety
invariant (playlist and media_file ids never collide, so the item_type
rewrite touches only mis-typed rows and cannot violate the unique key) and
the ordering note. The redundant 'Populate average_rating' comment is
dropped since the UPDATE is self-evident. The repository test's leakage
comment is condensed to two lines. No code behavior changes.
* refactor(subsonic): resolve setStar targets via GetEntityByID
Replace setStar's Album/Artist/Playlist Exists probe chain with a single
model.GetEntityByID lookup and a type switch, mirroring setRating. This
removes three per-id existence queries and keeps the two annotation paths
consistent.
An id that resolves to no known entity is logged and skipped rather than
filed as a spurious media_file annotation, and a lookup failure on one id no
longer aborts the whole batch. Also drop a duplicate empty-ids guard.
* refactor(playlists): drop no-op reclassify/backfill from migration
The average_rating migration carried two data-fix UPDATEs that are no-ops on
any real database:
- The media_file->playlist reclassification only matches rows no released
build ever created: playlists were never annotatable, so star/setRating of
a playlist id was never written as item_type='playlist'. Any stray
media_file-typed row for a playlist id is already removed by the media_file
annotation GC sweep (item_id not in media_file).
- The average_rating backfill runs before any item_type='playlist' row can
exist, so it can only ever write the default 0. Going forward SetRating
keeps average_rating current via updateAvgRating.
Reduce the migration to the column add/drop.
* refactor(persistence): bind annotation join params, derive idField from tableName
Address PR review: use Squirrel parameter binding for item_type/user_id in
the shared withAnnotation join instead of string concatenation, and pass
r.tableName+".id" from selectPlaylist so the join field stays consistent
with the surrounding r.tableName usage.
* fix(subsonic): surface datastore errors in setStar instead of skipping
Address PR review: setStar swallowed every GetEntityByID error and continued,
so a real datastore failure would still commit the transaction and emit a
refresh event as if the star succeeded. Skip only on model.ErrNotFound (an
unknown id); return any other error so the request fails and rolls back.
* test(subsonic): assert absent JSON keys instead of substring matches
Address PR review: substring checks are brittle ("starred" matches "starredAt",
"rating" matches "userRating"). Unmarshal the response and assert the
annotation keys are absent.
* fix(subsonic): skip refresh broadcast when a star request changes nothing
Address PR review (Codex): once setStar began skipping unknown ids, a request
containing only unresolvable ids left the RefreshResource empty, which
SendMessage serializes as a {*:*} wildcard that forces every client to
refresh. Only broadcast when at least one id was actually starred.
* fix(db): rebase playlist average_rating migration timestamp past master
The 20260708011823 migration predated the newest migration merged to
master (20260712211040_add_primary_key...), which Goose would silently
skip on already-upgraded databases. Rename it to a current timestamp so
it applies in order.
|
||
|
Deluan Quintão
|
cc315dcc8c |
perf(db): keep query planner statistics trustworthy with full ANALYZE (#5740)
* perf(db): keep query planner statistics trustworthy with full ANALYZE PRAGMA optimize's internal ANALYZE runs with a limited analysis budget (~2000 rows) that writes wrong sqlite_stat1 entries for low-cardinality indexes: on a 96K-track library it claimed (missing, library_id) narrows to ~2000 rows when it matches the whole table. The planner then prefers that index over the sort index and falls back to a full-table temp B-tree sort per request, turning paginated song listings into multi-second queries (reproduced at 5.5s on real hardware; ~90x slower than with correct stats). Every index-creating migration re-triggered the poisoning via the post-migration optimize, and the daily optimizer could re-trigger it on large library changes. Setting analysis_limit on the connection does not help: optimize ignores it. Run a plain full ANALYZE instead: after migrations with schema changes, and in db.Optimize (daily schedule and scan-end). Stats are stored in the database file, so one connection suffices and the per-connection pool loop is gone. The Optimize call at shutdown is removed: stats are maintained at migration/scan/daily points, and an ANALYZE during shutdown only delays it and races container stop timeouts. * perf(db): drop startup PRAGMA optimize that re-poisons planner stats The startup PRAGMA optimize=0x10002 runs SQLite's budget-limited internal ANALYZE (bit 0x02), which writes truncated sqlite_stat1 rows for low-cardinality indexes -- the exact statistics-poisoning this PR set out to eliminate. Because DevOptimizeDB defaults to true, a restart with no pending migrations would re-poison the planner until the next scan or daily Optimize. Remove it: statistics are already refreshed with a full ANALYZE after schema-changing migrations (Init) and via Optimize at scan-end and on the daily schedule, so nothing on the startup path needs to touch them. Also clarify that Optimize is a no-op unless DevOptimizeDB is enabled. * chore(db): remove the DevOptimizeDB flag and skip Optimize on quick scans The flag only gated the optimize/ANALYZE maintenance calls and there is no reason to leave planner statistics unmaintained; the guards are gone along with the flag. The scan-end Optimize now runs only after full scans — quick scans barely move the statistics, and the daily schedule covers drift. * style(scanner): drop redundant comment in runOptimize * chore(persistence): drop the no-op PRAGMA optimize from ScanEnd Mask 0x10000 only selects candidate tables by size change; without the 0x02 action bit optimize does nothing (verified: sqlite_stat1 stays stale after a 100x table growth). The scan-end statistics refresh is db.Optimize's full ANALYZE, and the expression-collation-index concern the old comment guarded against no longer applies. * fix(scanner): run the post-scan ANALYZE in the server process With the external scanner (the default), the scan pipeline runs in a subprocess, so its ANALYZE was invisible to the server: SQLite loads sqlite_stat1 into the process's shared schema cache, and an ANALYZE from another process does not refresh it — verified with the production DSN that even brand-new pool connections keep planning with the old statistics until the server restarts. An in-process ANALYZE, by contrast, is immediately visible to every pooled connection through the same shared cache. Move the full-scan Optimize from the scanner pipeline to the scan controller, which always runs in the server process. * fix(scanner): honor promoted full scans in the optimize gate A quick scan resuming an interrupted full scan is promoted inside the scanner (possibly in a subprocess); mirror the promotion in the controller so the post-scan ANALYZE isn't skipped. * refactor: apply cleanup review findings - drop forceFullRescan's inline ANALYZE: Init already runs a full ANALYZE after any migration batch with schema changes, so upgrades including a full-rescan migration analyzed the whole DB twice - resumingFullScan uses a filtered CountAll instead of fetching and scanning all libraries - document why CallScan (CLI) deliberately skips the post-scan Optimize * perf(db): make planner analysis maintenance resilient Check analysis freshness every 30 minutes and refresh statistics when the last successful run is over 24 hours old or a scan marked them pending. Persist successful analysis state, retry skipped or failed maintenance, coordinate checks with scans, and cover standalone CLI full scans. * perf(db): avoid analyzing routine quick-scan changes Reserve pending analysis for full scans, unscanned libraries, and retry state. Incremental quick scans now rely on the 24-hour freshness window instead of triggering a full ANALYZE at the next maintenance check. * fix(scan): analyze resumed full scans in CLI * fix(db): back off failed analysis retries * feat(db): allow disabling scheduled analysis * test(db): remove redundant analysis coverage * refactor(db): split ANALYZE maintenance into optimize.go and dedupe call sites - move query-planner statistics code from db.go to its own optimize.go (and matching optimize_test.go) - log ANALYZE elapsed time inside Optimize/OptimizeIfNeeded instead of repeating the timing block at every call site - drop the LastDBAnalyzeAttemptAt write on success: it is only read while failures >= 1, and every failure rewrites it first - extract runPostScanAnalysis (cmd) and anyIncludedLibrary (scanner) helpers |
||
|
Kendall Garner
|
4998ac2c59 |
feat(server): add scrobble history Native API (#5761)
* initial scrobble api * feat: add scrobble retrieval api * address feedback (1) * fix spelling * be explicit about get * add primary key field, update index, remove rowid references * use unix timestamp for input and output --------- Co-authored-by: Deluan Quintão <deluan@navidrome.org> |
||
|
Deluan Quintão
|
f48943c058 |
fix(plugins): discard buffered scrobbles when a plugin is removed (#5737)
* fix(plugins): discard buffered scrobbles when a plugin is removed Scrobbles are buffered in the DB per service, keyed by the plugin name. When a plugin was removed (deleted from the plugins folder and detected by the sync), its pending buffer entries were left behind forever: the drain goroutine is stopped on the next scrobbler refresh, so the rows were never retried nor discarded. Worse, if a plugin with the same name was installed later, the stale entries would be drained into it - potentially a completely unrelated plugin that just reuses the name. Add a Discard(service) method to ScrobbleBufferRepository and call it from removePluginFromDB, right after the plugin record is deleted. Disabling a plugin intentionally keeps its buffered scrobbles, consistent with the buffer's purpose of surviving temporary outages, and transient unload/reload cycles during config updates are unaffected since they never delete the plugin record. * fix(plugins): don't wipe builtin scrobbler queues on plugin removal Buffer entries are keyed by service name only, and removePluginFromDB runs for any removed plugin file, so removing a plugin named e.g. lastfm.ndp - regardless of its capability - would discard the builtin Last.fm retry queue. Skip the discard when the plugin name is owned by a registered builtin scrobbler, exposed via a new scrobbler.IsBuiltinScrobbler helper. Reported by Codex review on the PR. Also drop the testBroker usage from the new removePluginFromDB spec: it is defined in manager_test.go which is excluded on Windows, breaking the Windows test build. sendPluginRefreshEvent is nil-safe, so no broker is needed. |
||
|
Deluan Quintão
|
d4387c5502 |
perf(db): index media_file album/artist sort orders (#5706)
* perf(db): add composite indexes for song list album/artist sorts
The media_file sort mappings for album, artist and albumArtist expand to
multi-column ORDER BY clauses that no existing index could satisfy, so SQLite
fell back to a full table scan plus a temp B-tree sort of every row (including
the large lyrics/tags/full_text columns) even for a single 15-item page. On a
96K-track library this made /api/song?_sort=album take 3.6s on a cold cache.
Add composite indexes matching the three sort mappings, allowing the query to
walk the index and stop at the page size, in both directions. Drop the now
redundant single-column order_album_name/order_artist_name indexes (strict
prefixes of the new composites) and three indexes with no query path:
birth_time is only read in Go code, and artist/album_artist text column
lookups go through the media_file_artists table instead.
* fix(ui): make composer and track number columns non-sortable in song list
Clicking the Composer header was a silent no-op: composer is not a media_file
column, so the native API's sanitizeSort drops the sort and returns rows in
table order. Track number sorting across the whole library is not meaningful
and cannot use an index (the existing index leads with disc_number). Mark both
columns sortable={false}, like quality and mood.
* test(persistence): add sort index coverage test for large tables
Guard against sort options silently losing index support: every sort mapping
on media_file, album and artist is now verified with EXPLAIN QUERY PLAN to be
satisfiable by an index (both directions), so adding a mapping or dropping an
index that reintroduces a full-table temp B-tree sort fails the test. Sorts
that genuinely cannot use an index (random, annotation-join columns, JSON
expressions) must be declared in an exceptions list with the reason, keeping
the trade-off visible in review.
To make the sort mappings the complete declared sort surface, add identity
mappings for the media_file columns the UI sorts by without a mapping (year,
genre, duration, channels, bpm, path, comment, play_count, play_date, rating).
These are behaviorally no-ops: the same ORDER BY was previously produced by
the field whitelist fallback.
* perf(db): drop PreferSortTags expression indexes from media_file
The media_file sort_title/sort_artist_name/sort_album_name expression indexes
are only usable when PreferSortTags is enabled - a config reported by ~0.1% of
installations (insights, week of 2026-06-22) - yet every install pays their
storage (~8.6MB on a 96K-track library) and scanner write overhead. Drop them:
PreferSortTags installs fall back to a full sort for title/artist/album orders,
everyone else gets smaller DBs and cheaper writes. The order_album_name and
order_artist_name collation checks remain valid, now satisfied by the composite
sort indexes.
Signed-off-by: Deluan <deluan@navidrome.org>
---------
Signed-off-by: Deluan <deluan@navidrome.org>
|
||
|
Deluan Quintão
|
ae9b8a5fe6 |
feat(search): rank exact matches above prefix matches (#5704)
* feat(search): boost exact token matches over prefix matches buildFTS5Query now emits (word OR word*) instead of word* for plain tokens. The match set is unchanged (exact is a subset of prefix), but bm25 gives the rare exact token a high-IDF contribution, so rows containing the literal query word rank above prefix-only matches. The degraded-query check keeps evaluating the plain prefix form, preserving the LIKE fallback for queries like "1+" and "C++". * feat(search): weight artist search_normalized equal to name in bm25 For the artist table, search_normalized holds only the artist's name in alternate spelling (transliterated/punctuation-stripped), so a hit there is as meaningful as a name hit. Combined with exact-token boosting, artists like MØ now rank in the top results for the query "MO" instead of dead last. media_file and album keep weight 1.0 because their search_normalized mixes title, album, and artist variants. * test(persistence): add exact-match ranking regression test Seeds MØ, Morrissey, and Modest Mouse and asserts MØ ranks first for the queries "MO" and "MØ": the exact transliterated hit in search_normalized must outrank name-prefix matches. The corpus deliberately has no competing exact-word names, since exact-vs-exact ordering depends on corpus statistics rather than the guaranteed exact-over-prefix property. Rows are inserted per-test (with their library_artist associations) and cleaned up to avoid disturbing the shared seed fixtures and their count assertions. * docs(search): document exact-token OR emission in buildFTS5Query * test(persistence): harden exact-match ranking test fixtures Register the corpus cleanup before the insert loop so a mid-loop assertion failure cannot leak fts-rank-% rows into the shared integration DB, and reuse the existing createArtistWithLibrary helper instead of hand-rolling Put+AddArtist (which also replaces the ad-hoc context.TODO with the helper's GinkgoT().Context). * fix(search): flag multi-word degraded queries for the LIKE fallback The degradation probe was joined with explicit " AND " like the real query, so ftsQueryDegraded counted the literal AND as a long token and never flagged queries where every term degrades to a short token (e.g. "1+ 2+"). This predates this branch (the old code passed the same AND-joined string), but the probe now exists separately, so join it with spaces — it only feeds ftsQueryDegraded, which needs no explicit operators. |
||
|
Deluan Quintão
|
427d4b9bce |
fix(search): artists with atomic non-ASCII names unfindable after FTS5 migration (#5703)
* fix(scanner): update artist search_normalized when rescanning The FTS5 migration back-fills artist.search_normalized with a SQL punctuation-strip approximation, relying on the next scan to compute the precise value in Go (normalizeForFTS transliterates atomic letters like Ø/æ/ß that FTS5's remove_diacritics cannot fold). But the scanner persisted artists with an explicit column list that omitted search_normalized, so not even a full scan ever repaired it: an artist migrated from a pre-FTS database (e.g. "GØGGS") stayed unfindable by any ASCII search, while their albums and songs, which are saved with all columns, were fixed by a full scan. Add search_normalized to the column list so a full scan re-indexes the artist via the artist_fts trigger. * refactor(persistence): move normalizeForFTS to utils/str Export it as str.NormalizeForFTS so the upcoming migration can reuse the exact index-time normalization. Migrations cannot import the persistence package (persistence -> db -> db/migrations would be an import cycle). * fix(persistence): backfill artist search_normalized via migration Recompute artist.search_normalized with the precise Go normalization for databases migrated from pre-FTS5 versions, where the SQL back-fill could not transliterate atomic letters (Ø/æ/ß) and the scanner never rewrote the column. Only changed rows are updated, so the artist_fts update trigger re-indexes exactly the affected artists, making artists like GØGGS or MØ findable again without requiring a full scan. * refactor(persistence): share FTS punctuation-strip regex via utils/str Index-time normalization (NormalizeForFTS) and query-time processing (buildFTS5Query/ftsQueryDegraded) must produce matching tokens, so keep the punctuation-strip pattern in a single exported symbol instead of two identical private copies that could drift. Also document that derived columns computed in dbArtist.PostMapArgs must be listed in the scanner's artist Put, which is how search_normalized went stale in the first place. * chore(migrations): announce artist search backfill in the log Match the FTS5 migration's notice() pattern so startup isn't silent while the backfill runs on large libraries. * docs: tighten comments added in this branch * docs: describe FTSPunctStrip by what it matches, not one replacement |
||
|
Deluan Quintão
|
08cfc55d52 |
test(db): fix flaky applyLibraryFilter specs on shared DB (#5697)
The 'sees all libraries' specs hard-coded the user's libraries as {1, 2}
and assumed the shared test DB held exactly two libraries. applyLibraryFilter
only skips the filter when granted count == total library count, so when
another spec left an extra library behind (Ginkgo randomizes spec order),
the count was 3, the filter was applied, and the SQL assertion failed. This
surfaced on the Windows CI run but reproduces on any platform.
Grant the user exactly the library IDs that actually exist in the DB at
runtime instead of hard-coding them.
|
||
|
Deluan Quintão
|
318bb4944f |
perf(db): skip library filter when a non-admin sees all libraries (#5696)
* perf(db): skip library filter when a non-admin sees all libraries applyLibraryFilter already short-circuits for admins and headless contexts, but a non-admin who has been granted every library still paid for the correlated user_library subquery, which filters out nothing yet is the slow non-admin list/count path. Reuse the visibility check search3 already had: skip the subquery when the user's granted libraries cover the whole library table. The two helpers (userSeesAllLibraries/visibleLibraryIDs) are promoted from artist_repository to the base sqlRepository so all ~13 call sites benefit and search3 shares the single implementation. The skip is strictly gated on granted count >= total library count, never on an empty/unknown library set, so access control is unchanged for restricted users. * refactor(db): make all-libraries skip fail closed; test cleanups - userSeesAllLibraries: require len(visible) == total (not >=) so the filter skip can never over-grant if the visible set is ever inflated. - tests: assert ToSql() returns no error; restore r.db in AfterEach to avoid leaking mutated state to other specs. Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan Quintão
|
385e75e9a9 |
perf(db): skip annotation join in CountAll when unused (#5694)
* perf(persistence): skip annotation join in CountAll when unused The Native API list endpoints (/api/song, /api/album, /api/artist) issue a pagination count on every request via rest.GetAll. CountAll unconditionally added a LEFT JOIN on the annotation table to a count(distinct id) query. The join's columns are stripped by count(), but the distinct-over-join forced SQLite to stream and dedup every row, making the count dominate the request time on large libraries (e.g. ~190ms cold for 95k songs, seconds for non-admin users behind the library subquery). Gate the annotation join: only add it when a filter actually references an annotation column. The need is detected by rendering the query to SQL and matching annotation column names as whole words, which covers both named filters (starred, has_rating) and raw squirrel filters. The column set is derived from model.Annotations so it tracks schema changes; average_rating is excluded because it lives on the base table, and word-boundary matching keeps it from matching the annotation column rating. Counts are unchanged; only the query plan changes. Unfiltered song counts drop from ~37ms to ~4ms (warm) on a 95k-song library. * test(persistence): make annotation-join detection case-insensitive Address review feedback: SQLite column names are case-insensitive, so a raw filter using e.g. "RATING" would previously evade the case-sensitive column regex and wrongly drop the annotation join. Add the (?i) flag (average_rating stays excluded — the underscore still prevents a word boundary before rating) and cover it with mixed-case tests. Also make the starred-count assertion an exact value instead of a range. |
||
|
Deluan Quintão
|
0fab1861a0 |
fix(subsonic): make "recently added" order reproducible and consistent with RecentlyAddedByModTime (#5678)
* fix(subsonic): align album `created` with RecentlyAddedByModTime sort The album `created` attribute returned by search3, getAlbumList2 and the other album endpoints was always sourced from the album's CreatedAt (oldest song birth time), while the "recently added" sort is governed by RecentlyAddedByModTime: it orders by album.updated_at when that option is enabled and album.created_at otherwise. As a result, when RecentlyAddedByModTime was enabled, clients that cache album results and sort locally by `created` (e.g. for a "Date added" view) could not reproduce the order returned by getAlbumList2?type=newest, since the exposed value did not match the column driving the sort. Make albumCreatedAt config-aware so the primary timestamp it returns mirrors recentlyAddedSort: UpdatedAt when RecentlyAddedByModTime is set, CreatedAt otherwise. The existing zero-value fallback chain is preserved so this required OpenSubsonic field is never emitted as zero on legacy rows. Note: this is a behavior change for the contractual `created` attribute. With RecentlyAddedByModTime enabled, an album's reported `created` now reflects the newest song modification time and can change when files are modified. Scope is limited to albums; the song-level `created` (BirthTime) is unchanged. * fix(subsonic): order Recently Added by full-precision timestamp with tiebreak The recently_added sort wrapped the timestamp in datetime(), truncating it to whole seconds, and had no secondary sort key. Album timestamps carry sub-second precision (aggregated from song file birth-times), so on a fresh scan many albums tie at the second; SQLite then returns ties in query-plan order, which changes when a library filter is applied. This made the web UI "Recently Added" order invert between library selections and diverge from getAlbumList2?type=newest, and clients receiving the full-precision created value could never reproduce the server order. Sort on the raw, full-precision column with an album.id / media_file.id tiebreak instead. A new migration swaps the album datetime() expression indexes (and the plain media_file indexes) for composite (col, id) indexes that cover the new sort. Timestamps were already normalized to space-format by 20260316000000_normalize_timestamps, so raw-string comparison is safe. * fix(subsonic): make song created follow RecentlyAddedByModTime The song Created field returned BirthTime (file ctime), but the recently_added sort (shared by the Subsonic and native APIs, and the web UI) orders by created_at, or updated_at when RecentlyAddedByModTime is set. A Subsonic client, which can only sort by the created value it receives, could therefore never reproduce the server's "recently added" order in either mode. Add mediaFileCreatedAt mirroring albumCreatedAt and use it for child.Created: CreatedAt by default, UpdatedAt under RecentlyAddedByModTime, with BirthTime as a legacy fallback. This aligns song created with the sort column, matching how album created already works and what the native API/web UI present. |
||
|
Deluan Quintão
|
63a5954e4f |
perf(smartplaylist): use annotation index for playcount/rating/loved filters (#5662)
* fix(smartplaylist): use annotation index for playcount/rating/loved filters Annotation-field criteria wrapped the column in COALESCE(col, default) so missing annotation rows behave as 0/false. COALESCE prevents SQLite from using the column index, forcing a full media_file scan during smart playlist materialization - multi-second loads on large libraries, independent of rule complexity. Store the raw column plus its default and drop COALESCE when the compared value cannot match the default; fall back to 'col <op> ? OR col IS NULL' when the default would match, so never-annotated tracks are still preserved. Sorting keeps COALESCE to retain deterministic NULL ordering. Result set is unchanged; the materialize query now seeks the annotation index. Signed-off-by: Deluan <deluan@deluan.com> * fix(smartplaylist): keep COALESCE for list-valued annotation comparisons Hardening from final review: a list value (IN (...)) can't drive the index and a default-inclusive list has per-element NULL semantics, so route slice values through COALESCE(col, default) to stay exactly equivalent to the prior form. Also make the bool-default branch explicit (loved only supports equality operators) and share the COALESCE rendering via coalesceExpr. Signed-off-by: Deluan <deluan@deluan.com> * refactor(smartplaylist): make coalesced() a field method Thermo-nuclear review follow-up: promote the free coalesceExpr(f) to a smartPlaylistField.coalesced() method that returns the bare expression when there is no default. This lets sortExpr call field.coalesced() unconditionally and drop its 'if coalesceDefault != nil' branch, removing the 'only annotation fields get coalesced' special case from the sort path. Behavior unchanged. Signed-off-by: Deluan <deluan@deluan.com> * fix(smartplaylist): keep COALESCE for LIKE, bool-ordering, and tag ranges Code review (xhigh) found the index-friendly rewrite did not cover every operator, breaking result-set equivalence on a few reachable raw-JSON paths: - LIKE family (contains/startsWith/endsWith/notContains) on annotation fields used the bare column, so a NULL column never matched and missing-annotation rows were dropped. - Ordering comparators (gt/lt/...) on bool fields (loved) were decided as equality, wrongly including never-annotated rows. - InTheRange on a numeric tag split into two independent json_tree EXISTS, letting different tag values satisfy each bound. Centralize the decision in annotationCond via bareNullInclusion: emit the index-friendly bare form only for scalar values under an exactly-orderable comparator, otherwise fall back to the COALESCE form (always equivalent to the original). Route LIKE through coalesced(); reject tag/role ranges. Replace the local toFloat/toBool with spf13/cast (fixes unhandled numeric types and string bool forms), and drop the redundant LookupField + double reflect.TypeOf. A 17-case brute-force check confirms row-set equivalence to the prior COALESCE form across all operators including the fixed LIKE/bool cases. Signed-off-by: Deluan <deluan@deluan.com> * fix(smartplaylist): keep COALESCE for list values on bool annotation fields Second review found the bareNullInclusion bool branch missed the non-scalar guard the numeric branch has: a list value on loved/albumloved/artistloved (e.g. {"is":{"loved":[true]}}) coerced through toBool (which swallowed the cast error) to false, emitting the bare/OR-IS-NULL form and wrongly including never-annotated rows. Make toBool return (value, ok) like toFloat and bail to COALESCE when the value isn't a scalar bool. Also add the missing test for the tag/role range rejection. A 21-case brute-force confirms row-set equivalence to the original COALESCE form across every operator, including the bool/numeric list paths. Signed-off-by: Deluan <deluan@deluan.com> * refactor(smartplaylist): drop spf13/cast for stdlib value coercion The value coercion only sees the handful of types criteria produces (int, float64, string from JSON; bool already normalized at unmarshal), so cast's broad conversion isn't needed. Use small explicit type switches over strconv instead, keeping the string fallback (ParseFloat/ParseBool) that closes the '1'/'t' gap. No dependency change — cast returns to indirect. Signed-off-by: Deluan <deluan@deluan.com> * refactor(smartplaylist): share bool coercion via criteria.ToBool normalizeBoolValue (unmarshal-time) and the persistence bool guard both parsed bool-ish values independently. Extract the shared logic into an exported criteria.ToBool(any) (bool, ok): normalizeBoolValue delegates to it (behavior unchanged), and the persistence layer reuses it via its existing model/criteria import instead of a local helper. No behavior change. Signed-off-by: Deluan <deluan@deluan.com> * refactor(smartplaylist): trim sqlLiteral and dedup rationale comments /simplify cleanup: fmt %v already renders bool defaults as false/true, so drop sqlLiteral's redundant bool branch. Consolidate the COALESCE-vs-index rationale to the smartPlaylistField comment instead of repeating it across annotationCond and the struct. No behavior change. Signed-off-by: Deluan <deluan@deluan.com> * fix(smartplaylist): address review feedback on multi-field maps and *any From the PR bot reviews: - sqlFields now uses the field's coalesced() form, so annotation fields in a multi-field operator map (Is/Gt/Contains with >1 key) keep COALESCE and don't silently drop never-annotated rows. Covers both the comparison and LIKE fallback paths. (Gemini high, Copilot) - Replace coalesceDefault *any with a plain any (0/false are non-nil interfaces, so nil still means 'no default'); drop the coalesce() boxing helper and the pointer indirection. (Gemini) - Give rangeExpr clear, range-specific errors for the multi-field and malformed -pair cases instead of an empty-field / 'in operator' message. (Copilot) Adds tests for the multi-field COALESCE behavior and the new range errors. Signed-off-by: Deluan <deluan@deluan.com> * Revert multi-field COALESCE handling (YAGNI) The multi-field operator map case the bots flagged is unreachable: marshalExpression rejects any operator map with more than one field, so a multi-field map can never be persisted or loaded. Revert the sqlFields change and its tests rather than harden a code path no supported input can reach. Keep the two reachable improvements from the review: coalesceDefault any (not *any), and the clearer malformed-range error. Signed-off-by: Deluan <deluan@deluan.com> * refactor(persistence): model comparator as a behavior-carrying struct The smart-playlist comparator was a bare string alias, forcing two parallel switches over the same six operators: squirrelCmp mapped each to its squirrel constructor, and bareNullInclusion restated each as a float predicate. Adding or changing an operator meant editing both in sync. Make comparator a struct that bundles those facts per operator (the squirrel builder, the operator as a float predicate, and whether it's an ordering op). Both switches collapse: squirrelCmp is deleted in favor of cmp.build, and bareNullInclusion's numeric switch becomes a single cmp.satisfy call. Generated SQL is unchanged, as the existing table-driven tests confirm. * docs(smartplaylist): trim comments that restate the code Remove or tighten comments that describe what the code already says (likeCond and comparisonExpr doc lines, redundant clauses in annotationField/coalesced/ToBool/ normalizeBoolValue). Keep the comments that explain non-obvious rationale: the COALESCE-vs-index tradeoff, the bareNullInclusion/annotationCond contracts, and the why-we-fall-back notes. * docs(smartplaylist): collapse coalesceDefault comment to one line The field's six-line block duplicated the COALESCE-vs-index rationale that already lives on annotationCond. Reduce it to a one-line description plus a pointer there. --------- Signed-off-by: Deluan <deluan@deluan.com> |
||
|
Deluan Quintão
|
fa138afea5 |
fix(playlist/share): apply user library access to import and sharing paths (#5640)
* fix(playlist): respect the user's library access when resolving M3U paths FindByPaths looked up paths across all libraries, so importing an M3U could add tracks from libraries the importing user has no access to. Apply the user's library filter to the lookup, matching every other media_file read. Admins and the (admin-context) scanner are unaffected. * fix(share): scope shared playlist tracks to the owner's libraries loadMedia loaded playlist tracks with a fake-admin context, so a shared playlist could include tracks from libraries the owner has no access to. Load as the share owner instead, so the library filter applies. Admin-owned shares are unchanged. * fix(share): only serve shared tracks the owner can access A shared stream fetched the media file by id without checking the share owner's library access, so it could serve tracks from libraries the owner has no access to. Gate share-scoped streams on the owner's library access. Non-share streams are unaffected. * test(share): tidy library-access test setup Consolidate the repeated share-owner test fixture in handleStream into a helper, assert on track fields with HaveField instead of building an id slice, and delete the scratch media file through the public repository method. * style: trim verbose comments in library-access checks * fix(share): guard against nil owner and clean up test users Add a nil check after loading the share owner so a missing user yields a clear error instead of a possible nil dereference, and delete the users created by the new tests in their AfterEach blocks. * fix(share): avoid panic when a shared playlist is no longer visible to its owner Tracks() returns nil when the playlist can't be loaded under the owner's context (e.g. a public playlist shared by a non-owner that was later made private). Capture the result and return early instead of chaining GetAll on a nil repository, leaving the share with no tracks. |
||
|
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>
|
||
|
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). |
||
|
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> |
||
|
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. |
||
|
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. |
||
|
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. |
||
|
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. |
||
|
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 |
||
|
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. |
||
|
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> |
||
|
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.
|
||
|
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>
|
||
|
Deluan
|
2a43c4683e |
chore: go fix
Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
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. |
||
|
Deluan
|
03ac02d964 |
refactor: more warnings clean up
Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan
|
efe9291db0 |
refactor: multiple syntax updates for Go 1.26
Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
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> |
||
|
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> |
||
|
Deluan
|
57fc85f434 |
refactor(smartplaylist): remove unused 'value' field and clarify 'random' usage
Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
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> |
||
|
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. |
||
|
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 |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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. |
||
|
Deluan
|
2954c052f5 |
fix(tests): update media file paths in tests to be relative
Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
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.
|
||
|
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> |