mirror of
https://github.com/navidrome/navidrome.git
synced 2026-10-08 18:37:09 +02:00
477 commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Deluan Quintão
|
95f67d2c4e |
fix(share): reuse cached transcodes for share streams and zip downloads (#6262)
* fix(share): reuse cached transcodes when streaming from share links Public share streams built the stream request with only the share's format and bit rate, leaving sample rate, bit depth and channels at zero. Regular playback resolves those through the transcode decider (e.g. 48000 Hz for Opus), and they are part of the transcoding cache key, so a track already transcoded during normal playback was transcoded again into a separate, identical cache entry when played through a share link. The public router now resolves share stream requests with the same TranscodeDecider.ResolveRequest used by the Subsonic stream endpoint, so both paths produce the same request and share cache entries. Fixes #6261 * fix(archiver): reuse cached transcodes when zipping downloads Zip downloads (album, artist, playlist and share) built the stream request with only the format and bit rate, leaving sample rate, bit depth and channels at zero. Those are part of the transcoding cache key, so a track already transcoded for playback was transcoded again into a separate cache entry when downloaded in a zip, and vice versa. The archiver now resolves each request with TranscodeDecider.ResolveRequest, the same as single-song downloads and streams. This also applies the decider's defaults, so a zip requested without a bit rate uses the target format's default bit rate instead of leaving it to ffmpeg. * fix(archiver): name zip entries after the resolved transcoding format The transcode decider can pick a different format than the one requested (for example a player's forced transcoding, or a fallback to the default downsampling format when the requested one can't be produced). Zip entry names and the playlist M3U were still built from the requested format, so an entry could end in .mp3 or .flac while holding Opus data. Each track's request is now resolved before its entry name is built, and the name uses the resolved format. |
||
|
Deluan Quintão
|
4cdffd5633 |
feat(subsonic): OpenSubsonic API key authentication (#6219)
* feat(persistence): store hashed API keys on players * feat(core): refresh key-bound players without renaming them Add Players.Touch, which records usage for a player already identified by an API key without guessing its identity or overwriting its name. Register also stops renaming players that have an API key. Register no longer returns player save errors (or a stale FindMatch ErrNotFound when the save is rate-limited); save failures are only logged, and only the transcoding lookup error is returned, same as Touch. * feat(subsonic): authenticate with OpenSubsonic API keys Co-authored-by: amCap1712 <amCap1712@users.noreply.github.com> * feat(subsonic): add tokenInfo and advertise apiKeyAuthentication * feat(server): add endpoints to generate and revoke player API keys * feat(ui): manage player API keys Co-authored-by: amCap1712 <amCap1712@users.noreply.github.com> * fix(subsonic): throttle API keys per key and IP A stale key on one device exhausted the shared per-IP bucket and locked out every valid key from the same IP. The limiter only stores a hash of the bucket string, so the key is not retained. Also adds e2e coverage of API key auth through the real repository, and clarifies the player resolution log message. * fix(ui): keep the new API key dialog open until closed The key is shown only once, so Escape and backdrop clicks no longer dismiss it. Also clarifies when the key can be used as a password. * refactor: simplify API key code paths Share the player refresh tail between Register and Touch, fold the ownership-filtered write tail into execOwned, parse the query once for apiKey conflicts, derive HasAPIKey in the player mock, share the player form inputs between create and edit, and pick the delete button by key state instead of spreading conditional props. * feat(players): set API keys through the player record The key is a write-only apiKey field applied on save: required and owner-only on create, optional on edit, empty to revoke. Replaces the generate/revoke endpoints. * fix(players): reject API keys already in use Creating or editing a player with a key another player already has now returns a validation error instead of a 500, and a create that loses the race no longer leaves a keyless player behind. Ownership is checked before the key on create. * feat(ui): edit player API keys as a form field Replaces the show-once dialog, whose icon-less Close button was invisible on mobile. The key is generated in the browser, required and pre-filled on create. * fix(ui): keep new player API keys out of the record cache The json-server create response echoes the request body, and undoable edits merge the payload into the cache, so the key could reappear on the edit page. Strip it from the create result and save player edits pessimistically. Also fall back to a prompt when the clipboard write fails. * fix(ui): polish player API key field Set userId on the created player record so owner actions show immediately, and show a neutral no-key message to non-owners. * refactor: simplify player API key create and field Write the key hash in the create INSERT so the unique index settles races, re-read the created player instead of hand-building the cached record, reuse isWritable for the revoke check, and collapse the key field's derived state and generate/regenerate buttons. * fix(ui): let the API key field size like other inputs fullWidth is now opt-in instead of forced. * fix(ui): align the API key field with other player inputs Apply react-admin's input className, move the actions (now including Copy) below the field, and use a monospace font so the whole key fits. * fix(ui): redirect to the player list after create Matches the other create pages. * refactor(persistence): name the write-access rule for owned rows Owned-row writes now say which row they target and who may write it: ownedRow(rowID, ownerOrAdmin|ownerOnly) builds the WHERE, updateOwnedRow applies it, and SetAPIKey uses ownerOnly instead of a hand-built user_id filter. updateOwned/deleteOwned keep their signatures. * fix(players): apply an edit's key change and fields atomically Update now runs SetAPIKey and the column update in one transaction. Also shares the key format check, drops FindByAPIKey's unneeded empty-key guard, and sets the context username only on the apiKey path. * fix(subsonic): treat any credential param sent with apiKey as a conflict The spec requires error 43 when u, p, t or s is present with apiKey, even with an empty value. * refactor(subsonic): leave the player cookie code unchanged for key-bound requests Return early instead of wrapping the cookie block, so the diff (and CodeQL's view of it) matches master. * fix(subsonic): don't count key lookup errors as failed logins A database error while checking a key sent as the password now surfaces as a server error instead of a bad password, so it no longer feeds the failed-login limiter. * feat(players): use nds_ as the API key prefix Part of a Navidrome secret prefix family (nd + a letter for the kind), alongside ndg_ for API v1 grants. * feat(ui): make player API keys easier to find Label the Settings menu entry "Players & API keys", add an API key filter to the player list, show the key icon in the mobile list, and add Brazilian Portuguese translations for the new player strings. Signed-off-by: Deluan <deluan@navidrome.org> * feat(ui): always show the player API key filter Signed-off-by: Deluan <deluan@navidrome.org> * fix(ui): hide the unset Last Seen date in the player list Players created by hand have no last_seen yet, which showed as 12/31/1. Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org> Co-authored-by: amCap1712 <amCap1712@users.noreply.github.com> |
||
|
Deluan Quintão
|
b293b96256 |
refactor(persistence): stateless repositories with per-call context (#6149)
* refactor(persistence): adopt generic deluan/rest repository API Pin deluan/rest to the refactor branch. REST-facing repository methods take a context and return typed values. Drop DataStore.Resource and ResourceRepository; the native API names typed repositories directly through a per-request adapter that later commits remove. * refactor(persistence): base repository helpers take a context * refactor(persistence): LibraryRepository takes a context per call * refactor(persistence): PropertyRepository takes a context per call * refactor(persistence): UserPropsRepository takes a context per call * refactor(persistence): TranscodingRepository takes a context per call * refactor(persistence): ShareRepository takes a context per call * refactor(persistence): PlayerRepository takes a context per call * refactor(persistence): RadioRepository takes a context per call * refactor(persistence): PlayQueueRepository takes a context per call * refactor(persistence): Tag and Genre repositories take a context per call * refactor(persistence): PluginRepository takes a context per call * refactor(persistence): Scrobble repositories take a context per call * refactor(persistence): FolderRepository takes a context per call * refactor(persistence): Artwork repositories take a context per call * refactor(persistence): UserRepository takes a context per call * refactor(persistence): ArtistRepository takes a context per call ReadAll no longer rewrites the shared sort mappings for the role filter; it works on a per-call copy. * test(persistence): assert artist role sort sanitization in ReadAll * refactor(persistence): AlbumRepository takes a context per call * test(persistence): pass the test context to album repository helpers * refactor(persistence): MediaFileRepository takes a context per call * refactor(persistence): Playlist repositories take a context per call * refactor(persistence): build all repositories once per store * refactor(core): REST repository wrappers are built once * refactor(persistence): repositories are stateless Remove the context field from the base repository and the per-request REST adapter. Enable the containedctx linter so no repository can hold a request context again. * chore(lint): skip containedctx in test files * refactor: share simplifications from the stateless repositories sweep Add deleteOwnedAll on sqlRepository and use it in player/share Delete to remove the duplicated bulk-delete loop; have Share.Repository() return model.ShareRepository so subsonic sharing.go drops its repeated type assertions. * chore(core): assert REST wrappers implement Persistable * chore: reformat imports * perf(persistence): build repositories on first use Each transaction store used to construct all 21 repositories up front, paying for filter and sort mapping setup the block never touched. Fields are now sync.OnceValue thunks, so a store only builds what it uses. * fix(persistence): clean plugin references per deleted user A bulk user delete that fails on a later id had already removed the earlier rows but skipped their plugin cleanup. Cleanup now runs right after each successful delete. * fix(core): unload disabled plugins even when a user delete fails A bulk delete can fail on a later id after earlier users were removed and their plugins auto-disabled. The wrapper returned before unloading, leaving those plugins running until the next successful delete or a restart. * chore(deps): pin deluan/rest to v1.0.1 Replaces the pseudo-version of the refactor branch with the tagged release. REST error messages now name the bare type (Artist, not model.Artist). * test: use the spec context instead of context.Background() Replace the context.Background()/context.TODO() calls this branch added to tests with the spec's ctx, GinkgoT().Context(), or t/b.Context(), so repository calls are bound to the running spec's lifetime. * test: declare the spec context once per Describe Set ctx from GinkgoT().Context() first in each top-level BeforeEach and reuse it, building user contexts on top of it instead of repeating inline calls. |
||
|
Deluan Quintão
|
659d067aba |
fix(archiver): give same-named albums their own folder in artist zips (#6225)
* refactor: add Tags.First and slice.GroupOrdered helpers
Tags.First returns the first value of a tag or an empty string, replacing the inline len-check-then-index pattern in FullTitle, FullAlbumName, Album.FullName and the Subsonic album version mapping.
slice.GroupOrdered is slice.Group returning the groups in first-seen order, for callers that need a deterministic order the map-based Group cannot give.
* fix(archiver): give same-named albums their own folder in artist zips
Artist zips put every album in a folder named after the album, so two albums with the same name (an original and a deluxe edition, or names that only differ in characters the sanitizer replaces) were merged into one folder, with tracks mixed together and duplicate zip entries when file names collided.
The folder is now named after FullAlbumName(), so with AppendAlbumVersion on (the default) the version is part of the name, matching what clients display. Albums whose sanitized names still clash get a " [suffix]" taken from the first field that has a distinct, non-empty value for all of them: album version, year, release type, record label, catalog number, then a short album id. This follows the shape of beets' %aunique{} path function.
Albums are also grouped with slice.GroupOrdered instead of a map, so the zip is deterministic.
* fix(archiver): use the release year to tell same-named albums apart
Taggers often write an edition's date to the Date tag next to an original date, and the scanner then stores the original year in Year and the edition's year in ReleaseYear. Reissues of the same album therefore share Year, so the year disambiguator could not tell them apart and they fell through to the album id suffix. Prefer ReleaseYear and fall back to Year when it is not set.
Found by downloading an artist zip from a live server built from this branch.
* fix(archiver): let one clashing album keep the plain folder name
A disambiguator was only accepted when every clashing album had a non-empty value, so an original and its deluxe edition (with the version not appended to the name) fell through to the album id suffix. Accept a field whose values are distinct across the group even when one of them is empty, as beets' %aunique{} does: that album keeps the plain name, which the suffixed folders cannot clash with. Two or more empty values still count as a tie.
* test(archiver): refactor tests for album naming conventions and query order
|
||
|
Adrián Sánchez Zapico
|
a3f41fb422 |
sec(server): sanitize user-controlled filenames in Content-Disposition (#5895)
* sec(server): sanitize user-controlled filenames in Content-Disposition
Playlist export, Subsonic download and public share download built the
Content-Disposition header by interpolating a user-controlled name into a
quoted-string with fmt.Sprintf. A name containing a double quote closes the
string early and the rest is parsed as additional parameters, so a playlist
named `party"; filename="evil.html` yielded
attachment; filename="party"; filename="evil.html.m3u"
letting whoever chose the name decide what the browser saves the download as.
The names come from playlists, album/artist names and media file tags.
Go's net/http already rewrites CR and LF in header values to spaces, so
response splitting was not reachable; parameter injection was.
Add str.ContentDispositionAttachment, which emits a sanitized ASCII-only
quoted `filename` plus an RFC 5987 `filename*` carrying the original UTF-8
name, and use it at all four call sites. The `filename*` parameter also fixes
non-ASCII names, which previously went out raw or were mangled by sanitizing.
Signed-off-by: zapisanchez <zapisanchez@gmail.com>
* fix(server): keep download names intact and sanitize filename*
Rework ContentDispositionAttachment after review. Names with no ASCII
letters now fall back to download.<ext> instead of a bare extension
(東京.mp3 gave filename="mp3"). filename* is built from the same
sanitized name as the ASCII fallback, so path separators, reserved
characters, control and bidi characters, and invalid UTF-8 no longer
reach it. The ASCII fallback transliterates accents and typographic
punctuation (Legião -> Legiao, She’s -> She's) through the existing
sanitize.Accents and str.Clear helpers, keeps leading dots, and only
trims trailing ones. Names are capped at 255 bytes, keeping the
extension.
Pure ASCII names now get only the quoted filename parameter, so the
header for them matches the previous output byte for byte. filename*
is encoded with mime.FormatMediaType instead of a hand-written RFC 5987
encoder. Adds tests for the M3U export and Subsonic download headers.
* fix(server): handle dot-only names and long fake extensions
A name made only of dots trimmed down to an empty filename. It now
falls back to download, like an empty stem does.
path.Ext treats anything after the last dot as the extension, so a long
suffix with no real extension was kept whole and replaced the stem with
download, going past the 255-byte cap. Suffixes longer than 16 bytes
are now treated as part of the stem and truncated with it.
Neither case is reachable from the current call sites, which always
append a short extension.
---------
Signed-off-by: zapisanchez <zapisanchez@gmail.com>
Co-authored-by: Deluan <deluan@navidrome.org>
|
||
|
Deluan
|
6b3938b5b6 |
fix(subsonic): warn when a nowPlaying scrobble sends multiple ids
The scrobble endpoint accepts multiple ids, but a nowPlaying notification (submission=false) describes a single track, so only the first id is used. The extra ids were dropped silently, which made client bugs invisible. Log a warning instead, keeping the existing behavior for clients that rely on it. |
||
|
Deluan Quintão
|
c3b9b4ecb3 |
fix(subsonic): rate limit failed authentication attempts (#6185)
* fix(subsonic): limit failed authentication attempts per client IP and username The Subsonic API checked credentials on every request with no limit on failures, so any account could be brute-forced over /rest/*. Failed u+p, t+s and jwt attempts are now capped per (client IP, lower-cased username) using AuthRequestLimit and AuthWindowLength, the same settings that guard the UI login. Every request carries credentials, so only failures count: a slot is taken before the check and given back on success or on a server error, which also stops concurrent guesses from overshooting the limit. Blocked attempts get the same response as a wrong password (HTTP 200, error code 40, no Retry-After), so an attacker cannot tell a block from a wrong guess. Reverse proxy and internal authentication are not limited. The client IP helper behind ClientIPRateLimiter is now exported as server.ClientIP, so spoofed forwarding headers cannot open a fresh bucket. * refactor(subsonic): simplify failed authentication limiter Release the limiter slot from a single place in authenticate(), after the user lookup and credential check, instead of separately in the canceled branch. Store attempt counters by value instead of by pointer, and drop limiter unit tests that only repeated the middleware specs. * fix(subsonic): wait for an in-flight auth check instead of rejecting Slots were reserved before the credential check and only released afterwards, so once AuthRequestLimit checks for the same client IP and username overlapped, the next request was answered with error code 40 even when its credentials were valid. Clients that fan out parallel requests hit this constantly: a burst of six valid logins lost one, a burst of fifty lost forty five, and the web UI authenticates its own /rest calls the same way. A key now carries a slot channel of AuthRequestLimit capacity, and a request waits on it rather than failing when other checks for that key are in flight. Failures are recorded after the check, and a request is only rejected when the key already reached the limit within the window. A waiting request gives up if its context is canceled. Concurrent guesses still cannot run unchecked: at most AuthRequestLimit checks run at once and the rest are turned away as soon as the failures land. * docs(subsonic): state the real guess ceiling of the auth limiter The comment claimed a burst cannot overshoot, which reads as a hard cap of AuthRequestLimit. Allowing concurrent checks means a window admits up to 2*limit-1 guesses, so say that instead. |
||
|
Deluan Quintão
|
8e784b6af7 |
fix: apply the per-user library filter to bookmarks, playlists and now-playing (#6179)
On a multi-library instance, a few reads and writes built their own queries without the per-user library filter that every other media read applies. A user granted only some libraries could see, and store, tracks from libraries they had no access to. - getBookmarks now filters the query. It has to be the query and not the result: the loop below it pre-sizes the response from the bookmark count, so a row dropped afterwards would emit an empty bookmark entry. - createBookmark rejects an id the caller cannot read, returning error 70 to match getSong. Stored rows are left alone rather than purged, so a temporary revoke does not lose saved playback positions. - playlistTrackRepository Read, Count and GetAlbumIDs get the filter their siblings CountAll and GetMediaFileIDs already had. Read is the one that mattered most: its id is the integer playlist position, so it needed no track id at all. - Playlist track writes are filtered in playlistRepository.addTracks, the only writer of playlist_tracks rows apart from smart playlists, so Add, Insert, AddAlbums/AddArtists/AddDiscs and a full replace through Put all go through it. Insert reserves a slot per requested id, so when the filter drops one it renumbers to close the hole. - playTracker.GetNowPlaying honours its context instead of discarding it. The cache is process-global, so the filter belongs in the tracker rather than in the Subsonic handler, and any future caller inherits it. Admins and single-library installs are unaffected: applyLibraryFilter and HasLibraryAccess both short-circuit for them. Scanner playlist sync runs as admin, and M3U and CLI imports already resolve tracks through FindByPaths as the same user, so neither changes. |
||
|
Deluan Quintão
|
16567f147b |
fix(jellyfin): match Jellyfin on login SessionInfo, item types and universal streams (#6161)
* fix(jellyfin): send SessionInfo on login so JellyBox gets past sign-in
JellyBox parses AuthenticateByName's SessionInfo as a required object and
fails silently when it is missing, leaving the user on the login screen.
Real Jellyfin always sends it (SessionManager.AuthenticateNewSessionInternal,
10.10.7 and master), so the login response now carries a full SessionInfo
built from the user and the MediaBrowser auth header. It includes every field
JellyBox (Id, PlayState) and Finamp (UserId, LastActivityDate, the activity
and control bools, PlayState's CanSeek/IsPaused/IsMuted) require once the
object is present. The session Id is derived from client and device id, so
repeated logins from one install share it.
* fix(jellyfin): ignore IncludeItemTypes names that aren't Jellyfin kinds
JellyBox opens an album with ParentId=<album>&IncludeItemTypes=music. Music
is not a BaseItemKind, and Jellyfin's comma-delimited binder drops values it
cannot parse, so real Jellyfin treats the request as having no type filter and
lists the album's tracks. Navidrome returned an empty list, so every album
opened empty. Entries that aren't BaseItemKind names are now dropped before
type resolution, so an all-unknown list behaves like an absent one. Real kinds
Navidrome doesn't serve, such as Boxset, still return nothing.
* fix(jellyfin): treat universal Container as the direct-play list
On /Audio/{id}/universal, Container lists the "container|codec" entries the
client can direct play, and TranscodingContainer/AudioCodec name the target
when it can't (UniversalAudioController builds DirectPlayProfiles from it).
Navidrome passed the whole list to the decider as one target format, which
matched nothing and fell back to DefaultDownsamplingFormat, so JellyBox got
every MP3 transcoded to Opus. /universal now has its own handler: a source
matching an entry keeps its format (still downsampled under a bitrate cap),
anything else is transcoded to TranscodingContainer, then AudioCodec. The
/stream routes keep treating Container as the target format.
* refactor(jellyfin): let the stream decider resolve universal requests
streamUniversal matched the Container list itself with plain string equality
and then asked the legacy resolver for the source format. That skipped the
decider's container and codec aliases (mp4 vs m4a, ogg vs opus), and a
direct-playable source over the bitrate cap was transcoded to its own format
instead of the client's TranscodingContainer.
The shared part of ResolveRequest (server-side player override, player
MaxBitRate cap, decision to Request mapping) moves to a resolve helper, and a
new ResolveClientRequest exposes it for callers that build their own
ClientInfo. streamUniversal now turns Container into DirectPlayProfiles and
TranscodingContainer/AudioCodec into a transcoding profile, so the decision
uses the same rules as the Subsonic getTranscodeDecision path. streamFile and
the /stream routes share a serveStream helper, NewSessionInfo reads the clock
itself, and duplicate comments and tests are trimmed.
|
||
|
Deluan Quintão
|
3f4b6a642c |
fix(server): return 404 instead of 500 for missing native API resources (#6131)
* fix: return 404 instead of 500 for missing native API resources The deluan/rest controller only maps rest.ErrNotFound to 404, comparing with ==. Most repositories return model.ErrNotFound, which had the same message but was a different value, so requesting a missing playlist, album, artist, song, radio, player, transcoding or library returned 500. This also applied to other users' private playlists. Make model.ErrNotFound the same value as rest.ErrNotFound. This fixes every REST route at once, with no per-route wrapping. errors.Is checks against either error keep working, and nothing wraps model.ErrNotFound before it reaches the controller. Fixes #6130 * fix(radio): return not found when deleting a missing radio station radioRepository.Delete used the shared delete helper, which never reports a missing row because SQL DELETE on zero rows is not an error. Deleting an unknown id silently succeeded: DELETE /api/radio/{id} returned 200, and the Subsonic deleteInternetRadioStation endpoint returned ok. Delete now checks the affected row count and returns model.ErrNotFound when nothing was deleted. The native API returns 404, and deleteInternetRadioStation returns error 70 (data not found). This matches Subsonic 6.1.6, gonic (both verified live) and Ampache (verified in source). Airsonic-Advanced does not implement this endpoint. The shared delete helper is unchanged, as several callers rely on deletes of absent rows succeeding. * fix(ui): return 404 for missing files that are not missing or do not exist missingRepository.Read filtered media files by bare "id" and "missing" columns. The media file query joins the library table, so SQLite rejected the query as ambiguous and GET /api/missing/{id} returned 500. Read now loads the file with MediaFileRepository.Get, which qualifies the column, and returns not found when the file does not exist or is not marked missing. * fix: report missing rows on single-item deletes and adopt deluan/rest errors.Is Bump github.com/deluan/rest to the version whose controller matches errors with errors.Is and errors.As. model.ErrNotFound stays the same value as rest.ErrNotFound, so the many hand-written conversions from model.ErrNotFound to rest.ErrNotFound in repositories, core services and test mocks did nothing. Remove them, along with the duplicate rest.ErrNotFound check in the Subsonic error mapper. Mappings from model.ErrNotAuthorized stay, as those are different errors. User and transcoding deletes had the same silent success as radio: the shared delete helper never reports a missing row, so their not-found checks never fired and DELETE /api/user/{id} and /api/transcoding/{id} returned 200 for unknown ids. Add deleteByID, which returns model.ErrNotFound when no row matched, and use it for radio, user and transcoding. Also drop the dead sql.ErrNoRows branch from delete, since a DELETE never returns it. The Subsonic deleteUser endpoint is not implemented (501), so this does not change the Subsonic API. Plugin deletes keep the silent helper: there is no REST route for them, and the plugin manager only deletes rows it just read. * chore: drop ErrNotFound comment and its identity test The alias to rest.ErrNotFound is self-explanatory, and the identity test only restated the declaration. * refactor: alias model.ErrNotAuthorized to rest.ErrPermissionDenied Like ErrNotFound, make model.ErrNotAuthorized the same value as the rest library's error, so REST endpoints map it to 403 directly. This removes the ErrNotAuthorized to rest.ErrPermissionDenied mappings in the library and playlist REST adapters and the duplicate check in the Subsonic error mapper. Handlers that check model.ErrNotAuthorized now also recognize rest.ErrPermissionDenied returned by repositories, so writePlaylistError, the image upload handlers and the public share handler return 403 for it instead of their fallback status. The error message changes from "not authorized" to "permission denied". |
||
|
Deluan Quintão
|
08eb46c8ad |
Merge branch 'master' into fix-forceformat-directplay | ||
|
Deluan Quintão
|
72975a95fb |
fix(subsonic): honor DefaultDownloadableShare in createShare (#6121)
* fix(subsonic): honor DefaultDownloadableShare in createShare The DefaultDownloadableShare option was only sent to the web UI, which used it to pre-tick the "Allow Downloads?" checkbox. The Subsonic createShare handler built the model.Share without touching Downloadable, so it fell back to the Go zero value and every share created through the API was stored as non-downloadable, regardless of the configured default. createShare now reads an optional downloadable parameter and falls back to conf.Server.DefaultDownloadableShare when the client omits it, matching the web UI. Fixes #6119. updateShare had a related problem: core's share repository wrapper always writes the downloadable column, but the handler never set the field, so any updateShare call silently reset the share to non-downloadable. It now loads the current share and uses its value as the fallback. * refactor(subsonic): trim the share downloadable lookup and align with the UI updateShare fetched the share with Get to recover the stored downloadable flag, which also runs loadMedia and materializes every album and track the share points at, just to read one boolean. It now uses Read, which skips loadMedia, and only queries at all when the client omitted the parameter. createShare now ANDs the default with EnableDownloads, matching what the web UI already computes, so both paths apply the same rule. The specs collapse the create-path matrix into a DescribeTable, reuse the existing albumIDByName helper, and set the request-time config after setupTestDB so it does not leak into the config snapshot. * fix(subsonic): keep the share description on a downloadable-only update updateShare read the description straight from the request, so a client that sent only id and downloadable got an empty string written over the stored description. shareRepositoryWrapper.Update always writes that column, so the description was silently erased. This predates the downloadable parameter added earlier in this branch: any updateShare that omitted description already cleared it. Adding the parameter just made it easy to hit, since toggling downloads is a natural reason to call updateShare without touching the description. Both fields now use the presence-aware accessors and fall back to the stored share, which still costs at most one read and none when the client sends both. An explicitly empty description still clears the field. |
||
|
Deluan
|
404837799b |
fix(subsonic): don't re-encode a source already in the player's forced format
When a player has a forced transcoding format, ClientInfo.ForceFormat cleared DirectPlayProfiles unconditionally. A FLAC source on a player configured to transcode to FLAC was therefore re-encoded to FLAC, wasting CPU and bandwidth for no gain. Worse, the transcoder pipes ffmpeg output to stdout, so the resulting FLAC has total_samples=0 and no seek table -- an offline copy of it can never be seeked. Reported against getTranscodeDecision by the Symfonium author. ForceFormat now rebuilds DirectPlayProfiles from the matching transcoding profiles instead of dropping them: a client declaring a transcoding profile for a format is proof it can consume that format, so a source already in it is served as-is. Container and codec come from resolveTargetFormat, so a legacy "oga" target_format yields an ogg/opus profile, and the profile's MaxAudioChannels is carried across. DirectPlayProfile has no bitrate field, so restoring direct play needs a ceiling to keep an over-bitrate source out of it. GetTranscodeDecision now seeds that ceiling from the transcoding row's DefaultBitRate when a format was successfully forced, with the player's own MaxBitRate still taking precedence. This also closes a gap where the new endpoint ignored DefaultBitRate entirely: an mp3 320 source on a player forced to mp3@192 was served at 320, while the legacy /rest/stream path correctly gave 192. Applied via CapBitrate, which only ever lowers, so a client declaring a stricter limit keeps it. The legacy path (applyServerOverride) is untouched -- ForceFormat has no other callers. |
||
|
Deluan Quintão
|
a7365e119b |
fix(subsonic): update the playlist changed timestamp when renaming a smart playlist (#6082)
`buildPlaylist` reported `evaluated_at` as `changed` for smart playlists, so a rename or comment edit was invisible to clients until the next evaluation. A never-evaluated smart playlist also reported the current time on every call, which never settled. Report `updated_at` for every playlist. `refreshCounters` now syncs the stamp it writes back onto the model, and `refreshSmartPlaylist` reuses it for `evaluated_at`. `changed` therefore still equals the evaluation time for a just-evaluated playlist, and `validUntil` stays anchored to it. Original Subsonic always bumps `changed` on any playlist update, so this also aligns the behavior with upstream. |
||
|
Deluan Quintão
|
1f861d27ef |
fix(plugins): build public URLs on the caller's address instead of localhost (#6059)
* fix(plugins): build public URLs on the caller's address instead of localhost The artwork host service had no `*http.Request`, so it passed `nil` to `publicurl.ImageURL`. With neither `ShareURL` nor `BaseURL` configured, that produced `http://localhost/share/img/...`, which is useless to anything outside the server. The Discord Rich Presence plugin explicitly drops localhost URLs, so it fell back to the Navidrome logo instead of the real cover art. `serverAddressMiddleware` already works out the client-facing scheme and host from the `X-Forwarded-*` headers. It now also records them in the request context, and `publicurl` takes a `context.Context` instead of an `*http.Request` so any caller can reach them. Extism passes the caller's context through to host functions, so plugins invoked during a request now get a reachable URL with no configuration. Switching the parameter also removes the need for a second, parallel entry point: the package previously wanted only a scheme, a host, and a context, and took a whole request to get them. `AbsoluteURL` no longer dereferences a possibly-nil request on its parse-error path. Plugin calls that start from `context.Background()` (scheduler and websocket callbacks, the buffered scrobble drain) still fall back to localhost, since they have no request to learn from. A debug log now points at `ShareURL` when that happens. * fix(publicurl): include the configured port in the localhost fallback The last-resort fallback built `http://localhost/...`, which points at port 80 and so is unreachable for a server listening anywhere else — the default 4533 included. Use `conf.Server.Port` so a consumer on the same machine can actually fetch the URL. * fix(publicurl): use https in the localhost fallback when TLS is configured The fallback hardcoded the http scheme, so a TLS-only server with no BaseURL advertised a URL it does not answer on. Mirror the server's own switch, which requires both a certificate and a key. * refactor(publicurl): tidy the localhost fallback and its tests Use gg.If for the fallback scheme so it reads as an expression, like the BaseScheme branch above it, instead of assigning http and overwriting it. Drop two tests the ctx refactor left redundant: one asserted PublicURL "works without a request" but became a byte-identical copy of the ShareURL spec once the *http.Request parameter went away, and the two port specs differed only in the integer, where the non-default port is the stronger assertion. * refactor(conf): add TLSEnabled and use it instead of repeating the predicate Whether the server speaks HTTPS was decided inline in three unconnected places. This PR added the third, in a URL-building package that has no business inferring the transport config. Move the rule to conf, next to the fields it derives from, and call it from publicurl and the insights collector. server.Run keeps its own expression: it takes the certificate and key as parameters, and its test passes values that do not come from the config. |
||
|
Deluan Quintão
|
f08b5297ee |
feat(ui): add Refresh Metadata to the album and artist context menus (#6036)
* refactor(artwork): move artworkItemName into core/artwork as ItemName * feat(external): add RefreshInfo to force an external info refresh RefreshInfo re-fetches and re-saves external info for one artist or album, bypassing the TTL check that UpdateArtistInfo/UpdateAlbumInfo use. It is synchronous; callers that must not block detach it themselves. Also makes MockArtistRepo/MockAlbumRepo.UpdateExternalInfo persist to Data (previously a no-op) and adds the new method to the e2e noopProvider, both required so the interface addition compiles and is observable in tests. * feat(external): broadcast RefreshResource after external info is saved populateArtistInfo and populateAlbumInfo now emit the same RefreshResource event the artwork worker uses, so the UI learns about both foreground and background metadata refreshes. * feat(nativeapi): replace artwork refresh endpoint with metadata refresh * feat(ui): add refreshMetadata to the data provider * feat(ui): add a Refresh Metadata item to the album and artist context menus * fix(ui): re-fetch artist info when the record is refreshed * test: fix mislabeled spec, add kind-gate negative case, guard nil mock maps - Rename the RefreshInfo spec that claimed to cover the save-failure/broadcast path: SetError(true) fails Get too, so it only proves RefreshInfo bails out early at getArtist. - Add a spec proving playlist refreshes skip the external-info step, since that asymmetry (al/ar only) was documented but unasserted. - Add lazy nil-map init to MockAlbumRepo/MockArtistRepo.UpdateExternalInfo so a composite-literal-constructed mock doesn't panic on first save. * test: relocate discArtworkName specs from cmd to core/artwork artworkItemName moved into core/artwork as ItemName in an earlier commit, but its disc-name specs stayed behind in cmd/artwork_test.go, reaching across packages. Move them to core/artwork/item_name_test.go where the code now lives. * fix(ui): shape refreshMetadata like a react-admin response react-admin validates custom dataProvider methods and rejects any response without a `data` key, so the raw httpClient promise made every click surface an error toast instead of the success message. The unit test mocked useDataProvider, which skips that validation. Also folds "which kinds have external info" into external.HasInfo so the handler stops restating it, drops the nil-broker guard that only existed for tests, and delegates the mocks' UpdateExternalInfo to Put. * refactor(external): unexport infoKinds Only HasInfo is used outside the package, so the slice itself does not need to be exported. * refactor(artwork): fold ItemName into housekeeping.go next to Refresh ItemName exists to guard Refresh from ids that would orphan a queue row, and both callers invoke them back to back. A separate file hid that pairing; it was only split out to keep the move out of cmd/ legible in review. * fix(nativeapi): return 500 when the refresh lookup fails for a non-ErrNotFound reason A transient repository error told the admin the id did not exist, and the error was dropped without a log line, so nothing pointed at the real cause. Also drops the inherited claim that clearing artwork state shows a placeholder. Reads fall back to local resolution, so that only holds when there is no local art. * fix(ui): move Refresh Metadata above Get Info in the context menu Menu order follows key insertion order in the options object, so the new spec pins the position rather than leaving it to be shuffled by the next addition. |
||
|
Deluan Quintão
|
59810c3d59 |
feat(jellyfin): non-expiring, audience-scoped tokens revocable by password change (#6013)
* feat(auth): add per-user token_epoch column and bump method * feat(auth): add aud and ep claims, omitted when zero * feat(auth): add CreateAPIToken for non-expiring, audience-scoped tokens * feat(auth): add CheckClaims for epoch and audience validation * feat(jellyfin): issue non-expiring, jellyfin-scoped access tokens * fix(subsonic): reject API-scoped and revoked tokens on the jwt path * fix(server): reject API-scoped and revoked tokens on the native API * fix(server): pin the token-subject guard and stop leaking test config Adds a regression spec for the DevAutoLogin/ExtAuth guard in tokenAllowed, switches its comparison to case-insensitive to match the user lookup's own COLLATE NOCASE semantics, and restores Subsonic JWT test config after each spec instead of leaking SessionTimeout. * feat(request): add a token epoch holder for handler-to-middleware signalling * refactor(server): write the refreshed JWT header after the handler runs * feat(auth): revoke all tokens for a user when their password changes * fix(server): restore Unwrap on the JWT refresh writer so SSE write deadlines apply * test(auth): pin that non-session tokens reject API access tokens * test(jellyfin): pin token scoping and epoch revocation end to end Exercises auth.CreateAPIToken and CheckClaims against the real Jellyfin router and SQLite DB: the minted token has no exp and is aud-scoped to jellyfin, and bumping token_epoch through the real UserRepository revokes an already-issued token on the next protected request. * test(nativeapi): pin the token-epoch handoff through a real password-change request Drive a self password change through the real Authenticator/JWTRefresher chain and a real SQLite-backed userRepository, so the epoch handoff between Put and the refreshed-token writer is verified end to end, not as two separately-tested halves. Also fix tokenAllowed to read the enriched ctx it was given instead of r.Context(), so its warning log carries the username. * refactor(server): drop tokenAllowed's now-unused request parameter Finding-2 already moved every use to ctx; r was dead weight. Also note in the new nativeapi test why it must stay the package's only real-DB spec: db.Db() is a process-wide singleton its cleanup closes for good. * refactor(auth): remove duplication in claim decoding and token minting * refactor(auth): group aud with the standard JWT claims * refactor(auth): read aud with the standard-claim accessor pattern * fix(log): redact every api_key spelling the Jellyfin API accepts * fix(auth): bind session tokens to the user id, not just the username * fix(auth): return the token epoch from the same atomic increment * fix(auth): bump the token epoch in the same statement as the password write * chore(auth): trim comments to the why-only budget |
||
|
Deluan
|
bd6b7a6686 | test: increase timeout for cache availability checks to 10 seconds | ||
|
Deluan Quintão
|
1f3034f022 |
fix(playlist): block track edits on synced playlists across all APIs (#5984)
* fix(playlist): block track edits on synced playlists across all APIs A synced playlist's tracks come from its source file, so any track edit made through the UI or an API was silently reverted on the next scan. Track mutations funnel through two service guards, checkTracksEditable (incremental edits) and Create (wholesale replace, used by Subsonic createPlaylist and Jellyfin's replace path), which each duplicated the smart-playlist check. Both now consult a shared model.Playlist.TracksEditable() predicate, so the native, Subsonic, and Jellyfin paths are all locked: track edits return ErrNotAuthorized (403, or Subsonic error 50) instead of being accepted and lost. Metadata-only edits (name, comment, public, the sync flag itself) still go through checkWritable and are unaffected. In the UI, a synced playlist's track list becomes read-only, mirroring how smart playlists already behave. * fix(playlist): return 409 Conflict for non-editable playlist track edits The previous commit rejected track edits on smart and synced playlists with ErrNotAuthorized (403). That conflates two different things: a 403 says the caller lacks permission, but a synced or smart playlist's tracks are immutable for everyone, including the owner and admins. It is a property of the resource, not the caller. Introduce ErrPlaylistNotEditable and return it from both track-edit guards. The Native and Jellyfin APIs now map it to 409 Conflict; Subsonic maps it to error 50, the closest code it has (it has no read-only concept). The Native track handlers previously mapped this rejection inconsistently (400 on add, 500 on remove, 403 on reorder) through a new shared writePlaylistError helper. Genuine authorization failures (non-owner, non-admin) still return ErrNotAuthorized. * fix(playlist): surface synced read-only state in picker, Jellyfin, and OpenSubsonic Follow-up to the track-edit lock: the read-only state was enforced but not advertised consistently, so clients still offered edits that the server rejects. - UI: the Add to Playlist picker filtered targets by isWritable only, offering synced playlists that then 409 on add. It now filters with canChangeTracks. - Jellyfin: addToPlaylist/removeFromPlaylist hard-coded every error to 404, so a locked playlist reported "not found" instead of 409. They now return 409 for ErrPlaylistNotEditable while keeping the deliberate anti-probing 404 for every other error (a non-owner never reaches ErrPlaylistNotEditable, so 409 leaks nothing). - OpenSubsonic: buildOSPlaylist marked only smart playlists readonly; owned synced playlists advertised readonly=false. Readonly now also covers !TracksEditable(), matching the existing smart-playlist treatment. * fix(jellyfin): report CanEdit from playlist editability in permission probes getPlaylistUsers and getPlaylistUser returned CanEdit: true unconditionally, so Finamp (which probes this before showing edit controls) offered track editing on synced/smart playlists whose add/remove requests now return 409. Both handlers now fetch the playlist and set CanEdit from TracksEditable(), keeping the deliberate non-owner looseness (CanEdit stays true for a normal playlist a non-owner views) and mapping any lookup error to 404 like the sibling probes. * fix(playlist): check ownership before editability when replacing tracks Create checked TracksEditable() before ownership, so a non-owner replacing another user's public smart/synced playlist (Jellyfin updatePlaylist with a non-empty Ids list) received a 409 read-only conflict instead of a 403 authorization failure. The incremental guards check ownership first via checkWritable; Create now matches that order. Subsonic is unaffected (both errors map to code 50). Owners of their own smart/synced playlists still get the read-only conflict. * fix(jellyfin): return 403 for locked playlists, matching Jellyfin Jellyfin itself refuses edits on its file-backed playlists with Forbid() (403): PlaylistsController gates every mutation on OwnerUserId == caller or a share with CanEdit, and playlists imported from .m3u files satisfy neither. Its CanEdit is an ACL field, not a read-only marker, and Jellyfin core has no server-managed playlist type at all. Our Jellyfin routes exist to imitate that API, so ErrPlaylistNotEditable now maps to 403 there instead of 409. The native API keeps 409 (a resource-state conflict is the accurate REST answer where we define the contract) and Subsonic keeps error 50, its closest code. * chore(playlist): trim comments added by this branch Several comments ran to three or four lines and carried rationale that belongs in the commit history rather than the code: what Jellyfin does with its own file-backed playlists, and restatements of the expressions directly below them. Each block is now one or two lines covering only the non-obvious why. |
||
|
Deluan Quintão
|
95b8d9dd04 |
perf(genre): index genre filtering via join tables across all APIs (#5940)
Filtering by genre scanned every media_file/album row and JSON-parsed its `tags` column (a per-row json_tree(tags) EXISTS) with no usable index, so Finamp's genre screen took 1.9-6.5s per tap against a ~97k-track library. Album and album-artist genre queries had the same unindexed shape. Add normalized media_file_tags and album_tags join tables (genre only for now, via an indexedTagNames allowlist), populated by a new updateTags in Put (mirroring updateParticipants) and backfilled in the migration. Genre filtering across the Jellyfin, Subsonic and native APIs now runs as an index-backed semi-join through shared TagIDSemiJoin/TagNameSemiJoin helpers instead of a full scan. On a copy of the production DB the per-request cost for a typical genre drops from ~330ms to sub-millisecond, adding ~10MB. |
||
|
Deluan Quintão
|
944ca3100f |
feat(artwork): new artwork pipeline with background resolution and Low Quality Image Placeholders (#5847)
* feat(artwork): add artwork, item_artwork and artwork_queue tables * feat(artwork): add artwork models, repository interfaces and mocks * feat(artwork): implement artwork repository * feat(artwork): implement item_artwork repository with batched hydration * feat(artwork): implement artwork_queue repository * feat(artwork): add content-addressed originals store * feat(artwork): add artwork prune (orphan cleanup) * fix(artwork): never sweep files on transient DB errors during prune * refactor(artwork): fold originals package into core/artwork as ImageStore * refactor(artwork): merge item artwork state into ArtworkRepository * fix(artwork): chunk unbounded IN clauses and restore interface docs * refactor(artwork): apply simplify-pass cleanups Internal item_artwork sqlRepository helper, toSQLArgs upserts, batched queue enqueue, EnqueueStaleAbsent moved to queue repo, snapshot-based prune sweep, mock/real semantics aligned. * fix(artwork): address review findings on prune/sweep races and mock fidelity Sweep now honors an mtime grace window (in-flight acquisitions and temp files), reacquired orphans reset the prune grace window, and the queue mock implements real stale-absent semantics. * fix(artwork): atomic orphan deletion and timestamp semantics from review DeleteOrphans re-checks age+references at delete time, PutItemArtwork defaults attempted_at, queue mock timestamps mirror SQL. * fix(artwork): guard orphan file removal with the prune grace window Duplicate ImageStore writes refresh the file mtime and Remove skips files newer than the cutoff, so overlapping acquisitions cannot lose their store files to a concurrent prune. * fix(artwork): rewrite vanished duplicates and sweep stale mime variants Write falls through to a real write when the liveness touch fails, and sweep retention now matches the recorded mime's extension so obsolete variants are reclaimed. * fix(artwork): index artwork_queue in dequeue order The previous leading retry_at range column forced a temp B-tree sort of the whole eligible set on every DequeueBatch; ordering the index by (priority DESC, enqueued_at) lets scans stop after the batch size. * fix(artwork): honor the orphan cutoff in the repository mock The mock's DeleteOrphans now applies createdBefore like the SQL implementation, and a new spec covers a freshly reacquired row surviving prune. * fix(artwork): reject malformed hashes in ImageStore operations Known-absent states carry an empty hash and malformed persisted hashes could panic path sharding or inject separators; Write/Open/Remove now return an error for anything but 16 lowercase hex chars. * fix(artwork): mock PutImage refreshes created_at like the SQL repository Prune specs now age fixtures directly instead of seeding stale timestamps through the upsert. * fix(artwork): store backing-file provenance per item, not per hash * feat(artwork): import blurhash encoder from #5797 * feat(artwork): add worker-side artwork resolvers * fix(artwork): propagate playlist tile failures and dedupe external step * feat(artwork): add acquisition processor Resolves one queue item end to end: hash/dedup, decode + 128px thumbnail blurhash, place bytes (store vs source file), and persist found/absent/ failed state for the worker (Task 4) to act on. * style(artwork): tighten processor comments to budget * feat(artwork): add acquisition worker service * feat(artwork): enqueue artwork resolution from scan and CRUD paths * feat(artwork): artwork backfill, fingerprint re-resolution and scheduled jobs * test(artwork): leak/soak coverage and deferred assertions * fix(artwork): propagate transient artist image errors to the worker callGetImage swallowed all agent errors, so an agent outage surfaced as ErrNotFound and the worker settled artist artwork as a definitive absent (and reset the breaker). Add an additive ArtistImageResult path that returns the underlying agent error on transient failure while keeping ArtistImage byte-identical for existing callers; the worker's artist external step uses it via fromArtistExternalResult. * fix(artwork): resolve full playlist source chain resolvePlaylist only built the generated grid, dropping the uploaded-image, sidecar and ExternalImageURL sources the old reader_playlist.go chain serves. Port the full chain before the grid fallback: uploaded (upload), sidecar (folder), and ExternalImageURL routed through extGate with the same extError semantics as the other external steps. Also rewires the artist external step onto ArtistImageResult. * fix(artwork): purge dangling queue rows and guard concurrent re-enqueues Queue rows for deleted entities failed forever (Get -> ErrNotFound -> failed -> capped retries, unbounded). Add ArtworkQueueRepository.PurgeDangling, called from Prune next to the item_artwork purge. Separately, the found/absent path unconditionally deleted the dequeued row, erasing a concurrent scan re-enqueue; switch to DeleteIfUnchanged, which deletes only while retry_at still matches the dequeued value (verified retry_at is the column an Enqueue upsert resets). * style(artwork): fix comment accuracy and budget; fingerprint ArtistImageFolder Correct the inverted workerDeps.extGate comment, trim over-budget doc comments, and add conf.Server.ArtistImageFolder to the resolution fingerprint so an image-folder change re-resolves artist artwork. * fix(artwork): treat missing local playlist cover as definitive, not transient A playlist ExternalImageURL pointing at a local file that fails to open was routed through extError, causing failed/48h-retry loops that burn a rate limiter token forever instead of falling through to the generated grid. * refactor(artwork): deduplicate purge loop, backfill table, and extGate alias * fix(artwork): cap resolved image reads A user-editable ExternalImageURL can point at an arbitrarily large endpoint; a fast server could make the worker buffer hundreds of MB inside the 5s HTTP timeout. Bound the read to a fixed 20MB cap (no config knob) via io.LimitReader and fail the item if it is exceeded. * fix(artwork): retry higher-priority external art after fallback hit With CoverArtPriority="external,cover.jpg", a transient external failure followed by a folder hit dropped the external error: the worker recorded found and deleted the queue row, so the configured higher-priority external art was never retried. Carry extError onto the fallback resolution and add an outcomeFoundStale that persists+serves the art but reschedules via MarkFailed, giving the external source another chance. When external later answers definitively-not-found, the hit is not stale and the row is deleted. * fix(artwork): treat playlist cover URL 404 as definitive miss The playlist ExternalImageURL step used sources.go's fromURL, which maps any non-200 to a generic error, so a stale URL returning 404/410 was classified transient: infinite backoff plus it counted toward the circuit breaker, blocking valid external work. Add a local fetch in resolve.go that maps 404/410 to model.ErrNotFound (definitive) while keeping other non-200s transient. sources.go is left untouched. * test(artwork): move soak test into the Ginkgo suite * test(artwork): make leak and permission tests pass on linux goleak now ignores notify's nonrecursive-tree goroutines (linux uses inotify, which spawns dispatch+internal instead of darwin's recursive dispatch), and the read-only-dir prune spec skips under root, where permission bits cannot make Remove fail. * fix(artwork): reject decompression-bomb dimensions before decoding * fix(artwork): keep fresh re-enqueues ahead of stale failure backoff * fix(artwork): include M3U external art flag in the config fingerprint * fix(artwork): resolve private playlists with an admin context * test(artwork): convert non-synctest timing tests to Ginkgo specs TestArtworkBackoffSchedule and TestArtworkWorkerRunNoLeak needed no real *testing.T (no synctest), so move them into worker_test.go as Ginkgo specs. TestArtworkBreakerHalfOpen stays plain since testing/synctest requires a real *testing.T, matching core/scrobbler's precedent. * fix(artwork): store backing-file provenance per item, not per hash * fix(artwork): apply image limits to playlist tile decoding decodeTile ran image.Decode on every sampled album's resolved bytes before processItem's maxImageBytes/maxImagePixels guards applied, letting an oversized or decompression-bomb tile fully decode unbounded. Enforce both caps inside decodeTile itself. * refactor(artwork): reuse auth.WithAdminUser and dedupe image cap guards * perf(artwork): fetch only IDs for backfill enumeration Backfill enumerated every album, artist, playlist and radio via GetAll and mapped out just the ID. GetAll materializes full entities (library joins, participant/stats/tags JSON, annotation, artwork hydration), so on a large library it loaded tens of thousands of heavy structs only to read one field each — spiking transient RSS to ~1GB during the one-time upgrade backfill, a memory risk on small NAS/Pi hardware. Add GetAllIDs to the album, artist, playlist and radio repositories: it reuses each repo's base row-set filter (library visibility, artist content join, playlist userFilter) but projects only id, skipping the heavy columns and post-processing. A per-repo parity test asserts GetAllIDs returns exactly the same id set as GetAll. Verified on a 727MB / 29k-artist production DB copy: peak RSS during backfill dropped from ~1012MB to ~89MB, file descriptors flat, same 36,138 items enqueued. * feat(artwork): promote worker concurrency and external rate to real configs The artwork worker's drain speed was governed by two hidden Dev flags, DevArtworkWorkerConcurrency and DevArtworkExternalRPS, both defaulting to 2. On a large library's one-time backfill the external rate limiter is the real ceiling: every art-less item waits on it before the (rate-limited) external lookup, so the drain crawls at ~RPS items/sec while local-art items are unaffected. Promote both to documented, supported options: ArtworkWorkerConcurrency (default 4) sets local-resolution parallelism, ArtworkExternalMaxRPS (default 2, 0 = unlimited) caps external-agent lookups to stay polite to Last.fm/Deezer/etc. Operators can now trade first-backfill speed against external-API rate limits. The old Dev names still map for backward compat. * fix(deezer): never return empty-image-id placeholder pictures * feat(agents): enumerate enabled image-retriever agents per capability * feat(artwork): worker fetches agent images directly with per-agent rate limits and breakers * fix(artwork): treat agent not-found as breaker success * feat(model): content-hash artwork id suffix and hydratable per-entity image state * feat(persistence): hydrate artwork hash and absence onto entity pages * feat(artwork): resolve media_file embedded art in the worker, invalidate on rescan * feat(artwork): broadcast refresh events when artwork lands * fix(artwork): broadcast refresh for stale-found artwork too * feat(artwork): state-backed serving path with provisional read-through * feat(server): serve artwork from persisted state with content-hash caching * feat(subsonic): content-hash coverArt ids, omit artwork on known-absent * refactor(artwork): delete the legacy reader chain, cache warmer, and provider image methods * feat(artwork): precache on acquisition, bump on upload/radio changes, manual re-resolve API * test(artwork): end-to-end coverage for the serving cutover * chore(artwork): generic 500 bodies on refresh endpoint, trim stale test comments * fix(artwork): request read-through must not reset the failure backoff The provisional read-through and dangling re-enqueue used Enqueue, whose upsert resets retry_at, so any browse of an unresolved entity that was backing off after an external failure made it immediately eligible again — defeating the exponential backoff during a provider outage. Add EnqueueBump, which raises priority but leaves an existing row's retry_at intact, and route the serving path through it. Scan and manual re-resolve keep Enqueue's reset (a detected change wants immediate retry). * fix(artwork): keep an eligible track's cover requestable when its album is absent An embedded-eligible track with no resolved item_artwork row inherited the album's ImageAbsent, so when the album resolved absent (e.g. CoverArtPriority without 'embedded') the track's coverArt was omitted permanently — the client never requested it, so the lazy mediafile path never resolved it — even though the serving path would extract and serve the track's own embedded art. Hydration now never copies the album's absence onto an eligible-but-unresolved track. * fix(artwork): validate each agent image URL before picking the largest bestImageURL selected the largest by size and only then parsed it, so a malformed largest URL (e.g. a bad percent-escape) returned nil and shadowed a valid smaller candidate, contradicting the documented skip-unparseable behavior. Parse per candidate and compare sizes only among URLs that parse. * fix(artwork): fall back to disc art, not the album, for multi-disc tracks serveMediaFile delegated an absent/ineligible track straight to AlbumCoverArtID, skipping the disc-specific lookup that MediaFile.CoverArtID (and the deleted legacy reader) use. On multi-disc albums with per-disc images that served the album cover instead of the configured disc artwork. Delegate through DiscCoverArtID. * fix(artwork): enqueue uploaded artwork only after the filename is persisted SetImage cleared state and enqueued the bump before the caller stored the new filename, so a worker drain in that window could resolve against the old (already deleted) file and settle absent, leaving the upload unused until a later scan. Move the invalidate+enqueue into EnqueueArtwork, which each caller now invokes after the entity Put. * fix(artwork): keep multi-disc tracks requestable when the album is absent Round-1's hydration fix still copied the album's known-absent onto a non-eligible (or own-absent) track, but MediaFile.CoverArtID routes a multi-disc track to disc art, which resolves provisionally and is never known-absent. Marking it absent made Subsonic omit coverArt so clients never requested a valid disc image. Only mark a single-disc track absent, and only when its own art won't resolve. * fix(artwork): serve a local playlist ExternalImageURL as a file-backed reference A local ExternalImageURL was resolved through the external step and labelled external, so placeBytes copied it into the content-addressed store and dropped its path/mtime — replacing the file never tripped the staleness check. Classify local references as file-backed (resolved in place, even on the request path) and keep store-backed behaviour only for http(s) URLs. * fix(artwork): requeue playlist cover when its track set changes A generated-grid cover went stale after track mutations: nothing re-resolved the playlist's artwork, and the request path deliberately never rebuilds the grid, so serveEntity kept returning the old grid hash indefinitely. Enqueue pl artwork from refreshCounters (the choke point for every track-set change); no clear, so the old cover keeps serving until the worker rebuilds. * fix(artwork): open library-backed artwork through its on-disk root A library configured with a file:// path stored absRoot as the raw URI, so Abs produced strings like file:/music/cover.jpg that os.Open/os.Stat reject — folder, upload and embedded art were treated as dangling on every request, looping forever. Normalize a file:// path to its parsed OS path (the same root os.DirFS uses); non-local schemes are left unchanged (out of scope, per the artwork-musicfs TODO). * fix(artwork): only use disc resolution for multi-disc albums DiscCoverArtID returns a dc- id for any track with DiscNumber>0, so serveDisc ran the full DiscArtPriority chain even for single-disc albums, where a stray disc*/ embedded image could shadow higher-priority album art. Gate disc resolution on the album having more than one disc, matching the legacy reader; single-disc tracks serve album art directly. * fix(artwork): invalidate artwork when an uploaded image is deleted Deleting an artist/radio/playlist upload cleared the filename but left the found item_artwork row and its hash, so lists kept advertising the deleted cover's hash-suffixed immutable URL and clients could display it indefinitely. Call EnqueueArtwork after the delete-side Put, symmetric with upload, so the state is cleared and re-resolved to the next source (or absent). * fix(artwork): restore synthetic-artist guard and unicode normalization in agent lookups Moving agent calls into the worker bypassed two behaviors of the aggregate provider: Agents.GetArtistImages' guard for Unknown/Various Artists (a direct retriever call could assign an unrelated image to a synthetic artist), and auxAlbum/auxArtist.Name's DevPreserveUnicodeInExternalCalls normalization (records with typographic quotes/dashes missed exact-name searches). Re-apply both before enumerating retrievers. * fix(artwork): enforce entity visibility on the Subsonic getCoverArt path serveEntity reads persisted item_artwork by id, bypassing the library and private- playlist filters that the legacy entity-load applied. On the authenticated Subsonic path a user could fetch artwork for an inaccessible album or someone else's private playlist by guessing an id. getCoverArt now resolves the underlying entity through the request-scoped (filtered) repositories and serves the placeholder when it is not visible, so existence isn't leaked and the always-an-image invariant holds. The public share (JWT-authorized) and Jellyfin (admin) paths are intentionally untouched. * fix(artwork): version the artwork ETag with the served representation The ETag was the pixel hash of the original image, so a CoverArtQuality or EnableWebPEncoding change altered the resized bytes without changing the ETag — revalidating clients got a spurious 304 and kept the old encoding. Resized responses now carry a representation ETag (hash + size + square + encode settings) used for the ETag header and If-None-Match, while the immutable decision stays on the pixel hash (URLs remain pixel-identity per the spec, so hash-suffixed clients keep zero-request caching). Full-size originals fall back to the pixel hash as before. * fix(artwork): don't stamp the album hash onto multi-disc tracks The hydration fallback assigned a found album hash to every fallback track, but a multi-disc track's CoverArtID emits a dc- id served from disc-specific art whose hash is unknown at hydration time. Advertising dc-..._<albumHash> gave clients a content- version that never changes when the disc image does, breaking id-based refresh. Only stamp the album hash for single-disc tracks (DiscNumber == 0); multi-disc tracks stay unhashed and rely on the correct ETag returned by the served response. * fix(artwork): enqueue new empty playlists by id, and refresh on absent outcomes Two worker/enqueue fixes from review: - playlistRepository.Put assigned the generated id to the caller's Playlist but passed the stale copy (empty id) to refreshCounters, enqueueing a pl|"" row the worker failed until the daily dangling purge while the real playlist went unresolved. Set the id on the copy before enqueueing. - The drain refresh batch only included found/foundStale, so a cover removed by a scan (found -> absent) never notified clients, leaving the old immutable image displayed. Broadcast absent outcomes too; precache still only warms found/foundStale. * fix(artwork): honor disabled per-track art at serve time; use nanosecond mtime provenance Two serving-correctness fixes from review: - serveMediaFile served a persisted mf embedded image even after EnableMediaFileCoverArt was turned off (the setting isn't in the config fingerprint, so found rows aren't reprocessed). Direct mf- URLs now honor the setting at serve time and fall back to disc/album art. - The file-backed staleness check compared whole-second mtimes, so a same-second content replacement (two writes in one second, or timestamp-preserving tools) could serve different bytes under the old hash + immutable policy. RefMtime is now unix-nanoseconds (no schema change; int64 column), detecting sub-second changes where the filesystem records them. * fix(artwork): preserve the drive when normalizing Windows file:// library paths url.Parse puts the volume of file://C:/Music in Host, not Path, so localOSRoot dropped it and returned /Music — os.Open/os.Stat then failed and folder/embedded art on Windows looped as dangling. Rejoin the host volume, matching core/storage/local's newLocalStorage. * fix(artwork): clamp negative sizes to full-size; convert imghttp test to Ginkgo - A negative size (Subsonic size / Jellyfin maxwidth accept signed ints) reached resizeStaticImage, where the square path builds image.NewNRGBA(Rect(0,0,size,size)) — a giant rectangle that panics/OOMs. Clamp size<0 to 0 (full-size) at the Service entry. Positive sizes were already clamped to the original. - imghttp used a plain func Test with a table; convert to a Ginkgo DescribeTable with the suite entry point in imghttp_suite_test.go (AGENTS.md test-framework requirement). * test(artwork): use renamed ArtworkWorkerConcurrency in e2e tests * feat(artwork): carry blurhash through item image hydration * feat(nativeapi): expose artwork hash, absence and blurhash * feat(artwork): hydrate the parent album's artwork state onto tracks * test(artwork): add hydrateArtwork regression guard for AlbumImage wiring Drives hydrateArtwork itself (not applyItemImage directly) over tracks that take each of the loop's continue branches, so a future edit moving the AlbumImage fill below a continue would fail loudly instead of passing silently. * feat(jellyfin): version album and artist image tags by content hash * fix(jellyfin): trim primaryImageTag comment to why-only, within budget * feat(jellyfin): emit real blurhashes and drop the synthesized fallback * feat(ui): version cover art urls by content hash and skip absent art * feat(ui): add BlurHashCanvas placeholder component * fix(ui): clear stale blurhash pixels and assert the draw path in tests Clear the canvas before each decode attempt so a hash change that fails to decode doesn't leave the previous frame's pixels on screen once this wires into a list that recycles items. Also strengthen the specs to assert createImageData/putImageData were actually invoked (and with what), instead of only checking that a <canvas> element exists. * feat(ui): show the blurhash while an album cover loads * fix(artwork): hydrate cursor streams via an id pre-pass The album, artist and playlist GetCursor built their own select and never called hydrateArtwork, so every Jellyfin list endpoint (all six stream via GetCursor) emitted entity-id image tags and no blurhash. Only GetAll hydrated, which is why Subsonic and the native API were unaffected. Each cursor now resolves its ordered/filtered/paginated id set with the cheap id-only GetAllIDs query, then streams those ids in chunks through the repo's existing GetAll, which already hydrates and applies the full select. Max/Offset are consumed by the pre-pass alone; the chunk query carries only the caller's filters, Sort and Order. This also removes a pre-existing deep-pagination cost: keeping OFFSET out of the joined query makes the pre-pass a covering index scan instead of paying the library and annotation joins for every skipped row. Benchmarked on a synthetic 100k-album DB with the real schema, page=500 at offset 90,000: 3.9ms via the id pre-pass, 52.5ms for the current shape, 192.7ms for a naive join. An unpaginated full stream costs ~24% more, which is the trade. GetAllIDs gains the annotation join whenever the caller's filters or sort reference an annotation column (same gate CountAll uses), otherwise Filters=IsFavorite and SortBy=PlayCount would fail in the pre-pass. The playlist pre-pass repeats GetAll's columns so ORDER BY keeps resolving to playlist.name rather than the joined user.name. * fix(jellyfin): hydrate artwork on the song cursor Jellyfin's listSongs streamed media files via GetCursor, which never hydrates artwork, so songs emitted entity-id image tags and no blurhash. media_file now uses the same id pre-pass as the other three cursors (album/artist/playlist), for consistency, but on a separate method, GetCursorWithArtwork: GetCursor itself must stay untouched, since it's also the scanner's hot path and the scanner never reads artwork. Measured on 1,000,000 tracks, the pre-pass over all ids costs +41.8 MB heap and +298 ms versus GetCursor's bounded +0.0 MB. The Jellyfin path is paginated, though, so in practice it only ever pre-passes a page's worth of ids, not the full library, and doesn't pay that cost. * feat(jellyfin): emit a song's own cover art when it differs from the album's Real Jellyfin fills ImageTags from each item's own images before falling back to the parent album, and Finamp checks imageTags.Primary before AlbumId. Our mapper read only the album's image, so a track with distinct embedded art silently showed the album cover. Emit exactly one entry under ImageBlurHashes.Primary: Go marshals map[string]string in sorted key order rather than insertion order, so a second entry could pair the wrong blurhash with the image imageId resolves to, and Finamp pins that pairing in its cache for 365 days. * test(persistence): scope the GetCursorWithArtwork full-stream spec to tie-free ids The fixture has title ties (e.g. three "Antenna" tracks), so the unscoped positional comparison against GetAll only passed because SQLite's tie order happened to coincide between the full scan and the pre-pass's id IN (...) fetch. Scope it to onlySongs like the sibling ordering specs already do. * refactor(artwork): route song own-art through primaryImageTag; align chunk size Cleanups surfaced by /simplify: the song mapper's own-art branch reimplemented primaryImageTag's tag+blurhash-map construction (and its one-entry invariant) — route it through the helper so that invariant lives in one place. Tie artworkChunkSize to a whole multiple of artworkBatchSize so a cursor page re-chunks into even hydration batches. Hoist a duplicated imageLoading && blurHash boolean in the album grid. * fix(ui): serve the placeholder for known-absent art instead of a broken icon getCoverArtUrl returned '' for an imageAbsent record, so <img src={undefined}> rendered as the browser's broken-image icon on every absent cover. The server already serves a proper placeholder for absent art, so build the url and let it render. * feat(ui): show the blurhash as the loading placeholder across cover surfaces Add a shared CoverImage component (useImageUrl blob cache + blurhash + fade) and render the blurhash while a cover loads on the list thumbnails (CoverArtAvatar, radio) and the artist/album/playlist detail pages. The detail pages now go through CoverImage instead of a plain CardMedia, so their images come from the in-memory blob cache and survive React remounts without re-fetching. BlurHashCanvas gains an optional style prop. * refactor(ui): unify list cover surfaces onto the shared CoverImage component Route the album grid, CoverArtAvatar (artist/playlist lists) and the radio list's cover field through CoverImage instead of each carrying its own useImageUrl + blurhash-overlay wiring. CoverImage gains a default object-fit: cover. Radio keeps its uploaded-image gate and the generic radio placeholder for stations with no art. * fix(ui): address CoverImage review findings Restructure CoverImage so the size/shape lives on the root and the blurhash + image are absolute fills: the <img> mounts only once its blob is ready, so an unresolved cover never flashes a broken <img>. Add a fit prop (default cover) so album/playlist detail keep their letterbox instead of being cropped by a hardcoded object-fit. Remove the orphaned coverLoading styles and an unused subsonic import; add a CoverImage unit test. * perf(ui): only refetch already-loaded records on SSE refresh The artwork worker broadcasts a RefreshResource event per resolved chunk, carrying every id in the chunk. useResourceRefresh was doing a getMany for all of them, so any open list/detail page fetched hundreds of artists it was not displaying. Filter the event ids to records already in the store; the rest load fresh (with their new artwork) when navigated to. * refactor(artwork): scale worker concurrency with CPU count ArtworkWorkerConcurrency now defaults to max(2, NumCPU()/2) instead of a fixed 4, mirroring MaxOpenConns: local resolution scales with the host but stays at half the SQLite pool so it never starves the scanner/UI. External RPS stays a fixed 2 — it gates third-party API calls and is bounded by their tolerance, not the host, so it must not scale with CPUs. Also drop the DevArtworkWorkerConcurrency/DevArtworkExternalRPS deprecated aliases: those names were never released, so there is nothing to migrate. * feat(artwork): re-queue an absent cover when its page is viewed serveEntity now schedules a Bump recheck for an entity whose art was recorded absent, so viewing a missing cover re-triggers resolution (e.g. after an external source that was down during the scan comes back), matching the request-time bump that already covers never-resolved entities. Throttled by attempted_at against requestRecheckAge (1h) so repeatedly opening a genuinely-absent page can't hammer external services. EnqueueBump preserves an existing failed-state backoff via MAX(priority,...) and inserts a fresh, immediately-eligible recheck for a settled-absent row (whose queue row was already deleted). * refactor(artwork): move ImageUploadService to artwork.Uploader Relocate the image-upload service from core to core/artwork as artwork.Uploader, co-locating it with the resolver/worker/serving that own the artwork state it invalidates. MaxImageUploadSize moves too — its only callers are the two image-upload handlers — which lets core/image_upload.go be deleted entirely. Extract the shared "clear resolved state + re-queue at Bump" invalidation into artwork.Refresh and fold nativeapi's refreshArtwork handler onto it, removing the duplicated DeleteForItem+Enqueue block that had drifted into three places. The wire provider moves from core's set to artwork's; the playlists.ImageUploadService binding moves to the top-level injector so core/artwork stays unaware of playlists. Behavior is unchanged. * refactor(artwork): thread model.Kind through the artwork API Entity-level artwork queries now take a typed model.Kind instead of a bare prefix string. GetItemArtwork, DeleteForItem(s), GetInfoForItems, EnqueueStaleAbsent, hydrateItemImages, enqueueBackfillKind and artwork.Refresh convert to the prefix string only at the two real boundaries: the SQL item_kind column (kind.Prefix() inside each repo) and external string inputs (a new model.ParseKind for the nativeapi URL param, which also validates it). The Backfill/stale-absent kind slices, the resolve.go dispatch switch, and the kind→resource / kind→table lookup maps now use the Kind vars directly. The queue lifecycle methods (MarkFailed/Delete*) keep string kinds — they operate on a dequeued item's raw ItemKind column, which stays a string field, always populated via kind.Prefix(). Removes every bare "al"/"ar"/… prefix literal from non-test code (27 -> 0); behavior is unchanged. * tune(artwork): drop backoff base from 5m to 15s The exponential retry (base × 4^attempts, cap 48h) started at 5 minutes, so a single transient failure — a timeout under load, an external blip — parked a cover for 5 minutes even though a retry seconds later would have resolved it. Start at 15s instead: transient failures recover almost immediately (15s → 1m → 4m → 16m …), while persistent failures still escalate to the 48h cap (now at the 8th attempt instead of the 5th). * tune(artwork): 5s backoff base + 12h give-up, drop the cap Retry backoff now starts at 5s (was 15s) so a transient failure recovers on essentially the next drain, and jitter widens to ±40% so a wave of correlated failures doesn't re-clump into one poll. Add a 12h give-up budget measured from enqueued_at: once the next backoff would land past it, the worker stops retrying instead of grinding at a cap forever. A bare failure settles absent (handed to the 24h stale-absent sweep, and still recoverable on a page view); a found-stale keeps its already-served art. The budget bounds the tail, so the separate 48h backoffCap is removed. * fix(lastfm): match album.getInfo on name+artist only, not MBID Last.fm's album.getInfo by MBID is unreliable: a correct MBID can return a different album, or none. Observed with black midi's "7-eleven" (whose correct MBID returned a FLEETWOOD release) and both missing The Chats albums (one MBID 404s, the other resolves to a different self-titled release). The worker then recorded covers absent — or would fetch the wrong art — even though the correct cover is on Last.fm by name+artist. Stop passing the MBID to album.getInfo; query by name+artist only, which also drops the now-dead error-6 MBID-retry fallback. The low-level client keeps its MBID support for other callers; only the album lookup changes. * fix(lastfm): return agents.ErrNotFound on error 6 (not found) Last.fm returns error 6 for a missing artist/album — a definitive negative — but the agent returned the raw *lastFMError, so the artwork worker treated every not-found as a real fault: it counted toward the per-source circuit breaker (5 in a row opens it, fast-failing all Last.fm calls including valid ones) and was retried as a transient error instead of settling absent. On a first scan of a library with many artists Last.fm lacks, this stalled valid cover lookups and left entities churning in backoff. Translate error 6 to the shared agents.ErrNotFound at the agent boundary (callAlbumGetInfo / callArtistGetInfo), matching how the Deezer agent maps its client's not-found, and log it at Debug instead of Error — which also removes the not-found log spam. * feat(artwork): log external image-lookup failures at debug The worker's res.reader==nil && extError branch returned outcomeFailed with no log, so a failing external cover lookup (agent error, dead image URL, download timeout) was undiagnosable. Log the agent, entity, and underlying error at the fetch site where it's in hand — this surfaced a Last.fm album.getInfo returning an image URL that itself 404s. * fix(artwork): treat a 404/410 image URL as not-found, not a transient fault An agent (notably Last.fm's album.getInfo) can advertise a cover URL that is itself dead — a 404. sources.go's fromURL returned a generic error for any non-200, so a dead URL was treated as a transient failure: it churned in backoff and counted toward the circuit breaker, stalling valid lookups. Map 404/410 to model.ErrNotFound in fromURL so a dead URL settles absent, and collapse the near-identical fetchPlaylistImageURL (which already did this for M3U covers) into it. * feat(artwork): make artwork re-resolution targeted, not blunt Two gaps in when the pipeline re-resolves artwork: The recheck job only requeued absent-state rows (hash=''), so an entity that was never processed — added between scans, or on a server with the scanner disabled — had no periodic safety net and stayed without artwork indefinitely. Add EnqueueMissing(kind): a SQL set-difference enqueueing entities with no item_artwork row at Recheck priority (ON CONFLICT DO NOTHING, so it never disturbs a queued row). Run it once at startup and hourly alongside the stale-absent recheck. Rename staleAbsentKinds -> recheckKinds accordingly. Conversely, the config fingerprint included consts.Version, which embeds the git SHA and so changed on every build, re-enqueueing every entity in the library (~34k here) and re-querying external agents at the configured RPS for anything without local art. Replace it with an explicit artworkEpoch constant, bumped deliberately when resolution semantics change. The cases that motivated the version input — absent art becoming available — are already covered by the stale-absent and missing-row rechecks; only a corrected wrong-pick needs the epoch. A test guards against reintroducing the version. * test(artwork): restore resolution edge-case e2e coverage The serving cutover removed the album/disc/artist/mediafile/playlist/radio e2e specs that documented the folder-selection rules and guarded the #5376/#5456/ #5451/#5457 regressions; nothing replaced them, so compareImageFiles and the parent-fallback logic were left untested. Restore them driving the real pipeline: a real scanner populates the folder graph from an in-memory library, the real Worker drains the queue, and the real Service serves. Folder-backed art is file-backed (served via os.Open, which the in-memory FS can't satisfy) so its selection is asserted on the persisted state row; store-backed and real-disk sources are asserted byte-for-byte. Single-disc disc resolution now serves album art directly, so only multi-disc disc scenarios are ported. * fix(artwork): run disc resolution for single-disc albums too |
||
|
Deluan Quintão
|
f853ca604a |
refactor(db): migrate all ids to a uniform canonical 128-bit base62 encoding (#5824)
* refactor(model): extract canonical 128-bit base62 id codec * feat(model): generate random ids as canonical 128-bit base62 values * feat(scanner): emit legacy PIDs in canonical base62 encoding * feat(db): add id canonicalization transform for the uniform-ids migration * feat(db): migrate all ids to canonical 128-bit base62 encoding * fix(db): canonicalize ids in junction tables and JSON columns * chore(jellyfin): update id-family notes for uniform canonical ids * test(ids): harden codec input contract and migration edge coverage * refactor(model): use log.Fatal for Encode128 contract guard per project convention * fix(db): force full rescan after id migration for legacy PID configs * test(db): guard id-column inventory against schema drift * refactor(ids): compile-time Encode128 contract and unified column rewrite helper * refactor(db): apply review feedback to id migration Filter empty strings in collectColumn's SQL, reuse a prepared statement for rewriteColumn updates, and clarify the legacy ID functions' comment now that they emit the canonical encoding. * feat(auth): split session and public-link JWT secrets, rotating sessions on id migration * test(subsonic): initialize public token secret in helpers suite The suite sets auth.TokenAuth directly instead of calling auth.Init, so the new PublicTokenAuth was nil whenever Ginkgo's spec order ran a helpers spec before any spec that calls auth.Init, panicking in publicurl.ImageURL. * refactor(db): inline canonicalID into its only consumer, the uniform-ids migration * refactor(model): rename Encode128/Decode128 to Encode/Decode With every id now exactly 128 bits, the width suffix is redundant; the package-qualified id.Encode/id.Decode carries the same information. * test(db): make the id-columns guard classify JSON columns too The guard only inspected columns named id/pid/*_id, so it could not see ids embedded in JSON. Widen it to *_ids and to every JSON column, and drive the "covered" set from a new embeddedIDColumns list instead of the inline calls in the migration. Every JSON column the schema has now carries a verdict. The four denormalized caches -- media_file/album.participants, media_file/album.tags, album.folder_ids and artist.similar_artists -- hold only artist, tag and folder ids. Those all come from id.NewHash, whose 22-char base62 encoding of a 128-bit MD5 is already in canonical range, so canonicalID is the identity on them and the migration correctly leaves them alone. A new codec test pins that invariant, since the exemptions depend on it. Verified on a copy of a 727MB/96k-track production database: canonicalizing those four columns changed zero rows, and artist, tag and folder ids were themselves unchanged by the migration (only media_file ids moved, 95108 of 96666). |
||
|
Kendall Garner
|
b40b41584a |
feat(subsonic): Implement OpenSubsonic topSongsByArtistId extension (#5853)
* implement topSongsByArtistId extension * do not look up by artist name if empty --------- Co-authored-by: Deluan Quintão <deluan@navidrome.org> |
||
|
Deluan Quintão
|
23548f40a0 |
fix(share): give visual feedback when downloading from a share (#5865)
* fix(share): give visual feedback when downloading a share The share page handed the download URL to navidrome-music-player, which fell through to downloadjs and buffered the whole ZIP into memory via XHR before saving it. Nothing was handed to the browser until the last byte arrived, so a large share produced a long silent window with no player feedback and no browser download UI, inviting repeat clicks that each spawn another server-side zip+transcode. Use the player's customDownloader prop to trigger a synthetic anchor instead, so the browser performs the download and reports its own progress. An anchor rather than assigning window.location.href: the share page's service worker registers a NavigationRoute over all navigations, which intercepts the streamed archive and fails it into the offline fallback (observed as HTTP 503 in Chrome). handleDownloads now loads the share before streaming so it can set Content-Disposition and Content-Type. This also fixes error reporting: ZipShare previously wrote to the ResponseWriter before checkShareError ran, locking the status at 200, so expired, missing and non-downloadable shares all returned 200. They now correctly return 410, 404 and 403. * feat(share): acknowledge the download click in the player The browser's download UI is the real progress indicator, but nothing in the page itself reacted to the click, so the moment before the browser catches up still read as unresponsive. Dim the download button and make it unclickable for two seconds after a download starts, reusing the JSS function-value pattern the existing single-track styling already uses. A repeat download restarts the window instead of extending the original, and the timer is cleared on unmount. This also blunts repeat clicking, where every extra click costs another server-side zip and transcode. Add SharePlayer tests covering the download mechanism and this state machine. The dimming itself is verified in a browser rather than jsdom: JSS function values are not evaluated there, so the rule is never emitted and a CSS assertion would pass or fail for the wrong reason. Signed-off-by: Deluan <deluan@navidrome.org> * test(share): assert render counts in SharePlayer feedback tests The two acknowledgement tests compared the props object across renders, which React may reuse, so they passed without proving anything and then failed once the surrounding assertions changed. Count renders instead, and let the pending timer run out rather than advancing exactly to its deadline, which does not cross it. The repeat-download test now also asserts that no render happens at the original deadline, proving the timer was replaced rather than merely that one eventually fired. * fix(share): count one visit per share download The preflight share load added in this branch made every download record two visits: handleDownloads called Share.Load, and ZipShare then loaded the share again internally. Share.Load increments and persists VisitCount, so the counter advanced twice per download and the repository work was duplicated. Pass the already-loaded share into ZipShare instead of its id. handleDownloads is its only production caller, and it now has the share in hand for the Content-Disposition header anyway. The archiver test asserts Load is not called, so the double-load cannot come back unnoticed. Verified against a running server: the counter now advances by one per download. * test(share): derive feedback-window timings from the constant The acknowledgement tests hardcoded clock advances tuned to a 2000ms window. Raising DOWNLOAD_FEEDBACK_MS to 5000 left them advancing 1500ms and 1001ms, which no longer reach the deadline they are meant to cross, so the repeat- download test passed without proving the timer had been replaced. Export the constant and derive the advances from it, and let the pending timer run out in the unmount test rather than advancing a fixed amount. Changing the duration can no longer silently strand a test short of its deadline. --------- Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan Quintão
|
fed9665060 |
fix(streaming): surface why a transcode decision failed (#5820)
* fix(subsonic): surface the reason a transcode decision failed
getTranscodeDecision returned a bare "failed to make transcode decision"
with no clue why, and the probe command ran ffprobe with -v quiet, so even
the server log bottomed out at "exit status 1". A user whose files had been
moved by an external tool only saw the opaque error.
ProbeAudioStream now returns a typed ProbeError that separates the file path
from the reason: ffprobe runs with -v error so its stderr diagnostic is
captured, and a missing or unreadable file is reported as "file not found"
rather than ffprobe's misleading "Invalid data found". The handler logs the
full detail (including the path) and returns the reason to the client with
the server path stripped out.
Reported-by: Tolriq (Symfonium)
* refactor(ffmpeg): use errors.AsType for ExitError match
probeErrorReason used the older var+errors.As form while the rest of the
codebase (and its sibling transcodeFailureReason) uses the generic
errors.AsType. Switch to it for consistency; behavior is unchanged.
* fix(subsonic): return error 70 when the source file is missing
A getTranscodeDecision probe failure was always reported as generic error 0.
When the source file is gone (moved or deleted out from under the DB), that is
a not-found condition, so return the standard Subsonic error 70 ("data not
found") instead — matching what the endpoint already returns for an unknown
mediaId. Files that exist but are corrupt or unreadable stay error 0.
ProbeError now wraps the underlying cause and implements Unwrap, so the handler
detects the case with errors.Is(err, fs.ErrNotExist).
* fix(ffmpeg): keep probe error paths out of client-facing reasons
Addresses review feedback on the ProbeError type: the Reason field doubled as
both the log detail and the client message, so an ffprobe launch failure (a
*os.PathError from fork/exec) could leak the ffprobe binary path to clients,
and an unexpected stat error was reduced to "file not accessible" in the log.
Split the two concerns: Reason now holds only a path-free, client-safe string
(built at construction), while Error() logs the full underlying cause. Launch
failures return a generic "could not read file" instead of the raw exec error.
SafeReason no longer does substring path-stripping (removing the empty-Path
edge case); the stripping happens once, against ffprobe's stderr.
* fix(subsonic): don't report a broken ffprobe as a missing media file
Two issues from review of the previous commit:
Code 70 was selected with errors.Is(err, fs.ErrNotExist), but a launch failure
of a deleted ffprobe binary is an *os.PathError that also wraps fs.ErrNotExist.
A server-side ffprobe problem was therefore reported to clients as a missing
media file. ProbeError now carries an explicit NotFound flag, set only on the
file-access branch, and the handler keys the code off that instead of the chain.
ffprobe can also exit 0 while yielding no audio stream (an audio-suffixed
container holding only video). That parse failure was returned unwrapped, so
clients got "internal error"; it is now wrapped in a ProbeError too.
|
||
|
Deluan Quintão
|
3158451b8d |
refactor(server): drop redundant error return from req.Strings parsing (#5812)
* refactor(req): drop redundant error return from Strings
The error from Strings carried no information beyond emptiness — it fired
exactly when the param was absent — and nearly every caller discarded it with
a blank identifier. Strings now just returns the values (empty when absent),
making the common optional-list reads one clean expression.
The few required-param callers (scrobble, createShare) check for emptiness and
return the same Subsonic error code 10 as before; their e2e tests now pin that
code. Ints and Times keep their contracts by synthesizing ErrMissingParam
themselves, so selectedMusicFolderIds is untouched. The jellyfin parseFields
helper is inlined away, since ParseFields(p.Strings("fields")...) now
compiles directly.
* docs(req): clarify Strings returns nil when param is absent
|
||
|
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
|
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
|
01b7c86f90 |
fix(scanner): stop logging expected lyrics sniff misses as warnings (#5702)
* fix(scanner): stop logging expected lyrics sniff misses as warnings During a scan, embedded lyrics are parsed with an empty suffix, which puts ParseLyrics into content-sniffing mode: it tries the TTML, SRT and Lyricsfile YAML parsers in turn before falling back to plain text. Every plain-text or LRC lyric therefore fails the structured probes on its way to the fallback, and each failure was logged at warning level with no indication of which file triggered it, flooding the scan log with benign "Error parsing lyrics, falling back to plain text" messages. A probe rejecting content it does not own during sniffing is expected control flow, so it is now logged at trace instead. A parse failure under an explicitly requested suffix (e.g. a malformed .yaml/.srt/.ttml sidecar) still warns, since the user declared that format. ParseLyrics gains ctx and path parameters so any warning names the offending file and carries request context where available; all call sites are updated accordingly. Also fixes a test-isolation bug in the new logging spec: the BeforeEach swapped the process-global default logger via SetDefaultLogger but only restored the log level on cleanup, leaking the null logger and its hook into later specs in the shared model suite. * test: use spec-scoped contexts instead of context.Background in lyrics tests Replace context.Background() with GinkgoT().Context() (and b.Context() in the parse benchmarks) across the lyrics-related tests, so contexts are cancelled when each spec ends. The embeddedLyrics fixture in core/lyrics is now a hand-written literal like its sibling fixtures, removing the construction-time ParseLyrics call that could not use a spec-scoped context. * refactor(model): attach lyrics parse log attribution via context Narrow ParseLyrics back to (ctx, suffix, lang, contents), dropping the path parameter added by the previous commit. Attribution now uses the codebase's existing idiom: callers that know the source attach it with log.NewContext (e.g. "file" for the media file or sidecar), and the plugin adapter tags both the plugin name and the track, fixing probe-miss logs that misattributed plugin-returned content to the file's own tags. This removes three adjacent string parameters that were easy to swap silently, and the "" placeholder most call sites had to pass. Also hardens the logging spec from the previous commit: the null test logger is now swapped in before raising the level (SetLevel forces the current default logger to trace, so the old order left the null logger at info and trace entries never reached the hook), the sniff test now asserts probe misses are observable at trace with file attribution instead of only asserting the absence of warnings, and cleanup restores the actual previous logger — via a new return value on log.SetDefaultLogger — instead of a bare logrus.New() that would discard hooks configured on the process-wide logger. * refactor(lyrics): hoist attributed log contexts out of loops Address review feedback on #5702: build the log-attributed context once per operation instead of per iteration, and reuse it on the surrounding log calls so the error/trace lines around ParseLyrics carry the same attribution fields. In fromExternalFile the sidecar path now rides the context for all log lines in the function, replacing the repeated explicit "path" field. * style(model): pass lyrics parse errors as final log arguments Per the project logging convention, errors go as the last argument (the log package normalizes them via its error case) instead of a keyed "error" pair, which stores the raw error value and bypasses that handling. Flagged by review on #5702; the keyed form was inherited from the original warning line. |
||
|
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. |
||
|
Yuuta
|
9b862e8833 |
fix(subsonic): emit agent-specific cueLine values (#5679)
* fix(subsonic): emit agent-specific cue line values * fix(subsonic): preserve cue line edge text |
||
|
Deluan Quintão
|
56f0518830 |
feat(subsonic): add OpenSubsonic work and movement attributes (#5659)
* feat(subsonic): add Work/Movement response types and tag constants * feat(subsonic): surface works and movements in Child response * test(subsonic): verify works/movements JSON serialization Fix G109 lint: use strconv.ParseInt with bitSize=32 to avoid potential integer overflow; add JSON serialization test confirming omitempty on optional sub-fields. * refactor(subsonic): use number.ParseInt idiom in buildMovements * refactor(subsonic): move work/movement builders to MediaFile methods Introduces model.Work and model.Movement types with Works()/Movements() methods on MediaFile. The Subsonic layer maps them to response types inline via slice.Map, replacing the deleted buildWorks/buildMovements helpers. * test(subsonic): cover populated works/movements in response snapshots * test(subsonic): clarify empty-case name and assert JSON structurally |
||
|
Deluan Quintão
|
aa5aa731dc |
refactor(lyrics): single ParseLyrics entry point + all-format plugin lyrics (#5632)
* refactor(lyrics): read sidecar files via library storage FS
Routes fromExternalFile reads through storage.For(mf.LibraryPath) instead
of os.Open on AbsolutePath, fixing sidecar reads for non-local backends.
UTF-16 LE/BE and BOM handling preserved via ioutils.UTF8Reader.
* refactor(lyrics): address review feedback on sidecar FS read
- Move blank local-storage import from sources.go into lyrics_suite_test.go
(the test suite already imports the local package for RegisterExtractor,
so local's init() runs; production binaries get the scheme via normal wiring)
- Fix misleading comment: model.ParseLyrics → model.ParseLyricsFile
- Replace what-comment with why-comment in BeforeSuite explaining the
log.Fatal guard that requires the no-op extractor registration
* test(lyrics): add subsonic e2e baseline for getLyrics endpoints
Establishes a behavioral baseline for getLyricsBySongId (v2 structured)
and getLyrics (legacy) before the lyrics parser refactor. Covers embedded
formats (LRC synced, plain text, TTML) and sidecar formats (LRC, SRT,
YAML), all isolated under a Lyrics/ fixture folder so the new fixtures
do not perturb existing test behavior beyond fixture counts.
Sidecar files are injected as raw &fstest.MapFile{Data: []byte(...)}
entries; the scanner skips non-audio extensions (.lrc, .srt, .yaml) so
they are invisible to scanning but reachable via the fake FS at request
time through fromExternalFile/storage.For.
Update album/artist/song counts in the album-list, multi-library, and
search3 empty-query tests to reflect the six new tracks (1 new artist,
1 new album, 6 new songs).
* test(lyrics): strengthen e2e lyrics baseline (lang assertions, rename helper)
Rename the local helper `main` to `firstLyric` to avoid collision with the
reserved-feeling built-in name. Add `Lang` assertions to both embedded and
sidecar DescribeTable entries, locking the current observed values: "xxx"
(ISO 639-2 "no language specified") for all embedded and LRC/SRT sidecars,
and "eng" for the YAML sidecar (which explicitly sets `language: eng`).
* feat(lyrics): detect Lyricsfile YAML in content-sniffing
* feat(plugins): content-sniff plugin lyrics for all formats
Replace model.ToLyrics (LRC/plain only) with model.ParseEmbedded so plugin
responses are content-sniffed for TTML, SRT, YAML, LRC, and plain text.
ParseEmbedded returns a LyricList, so the loop now flattens multiple tracks
per response entry.
The test-lyrics WASM plugin gains a "ttml" format mode (configured via
pdk.GetConfig) that returns a minimal TTML document; rebuilt with the
standard Go wasip1 toolchain (GOOS=wasip1 GOARCH=wasm). A new Ginkgo test
asserts Synced==true and the exact cue value, which the old plain-text path
could not produce.
GetLyrics doc comment updated to reflect content-sniffing; a later task will
retarget it to ParseLyrics once that function is introduced.
* test(plugins): validate plugin lyrics auto-detect across all formats
The test-lyrics WASM plugin now supports per-format modes via the
"format" config key: ttml, srt, yaml, lrc, and plain, in addition to
the existing default plain-text response. The plugin is rebuilt with the
standard Go wasip1 compiler.
lyrics_adapter_test.go gains a DescribeTable covering all five formats,
asserting both Synced (the discriminator that proves correct format
detection) and the exact line value. This validates the full
auto-detect chain (TTML → SRT → YAML/Lyricsfile → LRC → plain) end-to-end
through the real plugin → adapter → parser flow.
* refactor(lyrics): consolidate parsers into model.ParseLyrics
* refactor(lyrics): retarget legacy callers to model.ParseLyrics
Pin suffix to ".lrc" to preserve byte-identical output for stored
plain/LRC text that was previously handled by the now-removed ToLyrics.
* test(lyrics): fix lyrics tests after parser consolidation
- Rewrite the YAML-fallback test to assert the correct design: a
non-Lyricsfile .yaml sidecar returns as plain text and shadows
lower-priority sources (rather than falling through to .lrc).
- Add LibraryPath + relative Path split to the three subsonic tests
that read sidecar files via storage.For(), so they resolve against
the correct fixtures directory.
- Register a no-op extractor in api_suite_test.go BeforeSuite so
newLocalStorage does not fatal when storage.For is called during
sidecar-lyrics tests.
* test(lyrics): add per-format ParseLyrics benchmarks
Baseline measurements (count=2 runs) on M2:
BenchmarkParseLyrics_LRC-8 5725 178796 ns/op 49.78 MB/s 427877 B/op 523 allocs/op
BenchmarkParseLyrics_Plain-8 5425 230854 ns/op 32.44 MB/s 102508 B/op 16 allocs/op
BenchmarkParseLyrics_EnhancedLRC-8 1942 605893 ns/op 17.66 MB/s 860678 B/op 4256 allocs/op
BenchmarkParseLyrics_SRT-8 3249 373991 ns/op 25.91 MB/s 1113575 B/op 4407 allocs/op
BenchmarkParseLyrics_TTML-8 1483 813027 ns/op 13.86 MB/s 2198052 B/op 8665 allocs/op
BenchmarkParseLyrics_YAML-8 1700 678250 ns/op 13.01 MB/s 1235096 B/op 8288 allocs/op
BenchmarkParseLyrics_SniffTTML-8 1525 776482 ns/op 14.51 MB/s 2225448 B/op 8681 allocs/op
BenchmarkParseLyrics_SniffSRT-8 2528 451210 ns/op 21.48 MB/s 1157000 B/op 4422 allocs/op
BenchmarkParseLyrics_SniffYAML-8 1333 827152 ns/op 10.67 MB/s 1337195 B/op 8718 allocs/op
BenchmarkParseLyrics_SniffLRC-8 2820 413038 ns/op 21.55 MB/s 588934 B/op 1812 allocs/op
BenchmarkParseLyrics_SniffPlain-8 2968 409091 ns/op 18.31 MB/s 254470 B/op 1491 allocs/op
Content-sniff path overhead: 1.5–15% depending on format.
* test(lyrics): use real public-domain fixtures for parser benchmarks
Replace synthetic benchmark payloads with 'Auld Lang Syne' (Robert Burns,
1788, public domain) rendered into every supported format (LRC, plain,
enhanced LRC, SRT, TTML, Lyricsfile YAML) so the numbers reflect realistic
content. Same song across formats makes per-format cost comparable.
Baseline (Apple M-series, -benchmem, real fixtures):
LRC ~28 us/op 42 KB 147 allocs
Plain ~23 us/op 18 KB 22 allocs
EnhancedLRC ~37 us/op 51 KB 374 allocs
SRT ~52 us/op 139 KB 581 allocs
TTML ~119 us/op 276 KB 1227 allocs
YAML ~142 us/op 193 KB 1732 allocs
Sniff(LRC) ~47 us/op 57 KB 237 allocs
Sniff(TTML) ~122 us/op 282 KB 1250 allocs
Sniff(YAML) ~186 us/op 218 KB 1847 allocs
Fixtures in tests/fixtures/lyrics/.
* fix(lyrics): preserve [] (not null) for empty lyrics in backfill migration
ParseLyrics returns nil for zero-line input (whitespace-only stored
lyrics). json.Marshal(nil LyricList) produces null, violating the DB
invariant that media_file.lyrics uses [] for empty lyrics, never null.
Initialize to model.LyricList{} when ParseLyrics returns nil so the
marshalled result is always [].
* refactor(lyrics): unify parser dispatch and centralize empty-list invariant
Apply thermo-nuclear review findings (behavior-preserving):
- Replace the suffix switch + three single-use closure adapters
(parseTTMLKnown/parseSRTKnown + inline YAML closure) with a
bySuffix map of a single lyricParser(lang, contents) signature.
Normalize parseTTMLWithDefaultLang/parseSRTWithLanguage to that
(lang, contents) order so no adapter glue is needed.
- Collapse the parallel sniffLyrics engine into one parseFirstMatch
primitive shared by both the suffix and content-sniff paths
(sniffOrder candidate list). TTML stays gated via parseTTMLIfDocument
in sniff mode to avoid running the XML decoder on plain/LRC text.
- Add LyricList.MarshalJSON so empty/nil always serializes to [] (the
lyrics column invariant), in one canonical place. Delete the
migration's nil-guard, which the marshaler now subsumes.
Behavior verified unchanged: full suite + race + e2e green.
* refactor(lyrics): single registry drives both suffix dispatch and sniff order
Collapse the bySuffix map and sniffOrder slice into one ordered registry:
slice order is the content-sniff probe order, each row's suffixes drive
sidecar dispatch, and per-row bySuffix/byContent parsers preserve the
gated-TTML-when-sniffing distinction. One source of truth, no duplicated
parser references.
* refactor(lyrics): self-skipping parsers collapse the format table to one column
Move the TTML <tt>-document gate into parseTTMLWithDefaultLang itself (after
the encoding fixup, so UTF-16-declared docs are still recognized): non-TTML
content returns (nil, nil) to skip; a malformed <tt> document still errors.
SRT and Lyricsfile YAML already self-skip. With every structured parser
self-skipping, the format table drops to one {suffixes, parse} column named
lyricFormats — no bySuffix/byContent split, no separate sniff-only TTML gate.
Both the suffix and content-sniff paths share the same parser per format.
* refactor(lyrics): strip BOM once at ParseLyrics entry for all paths
Previously only the content-sniff path stripped the BOM; the suffix path
relied on its callers (fromExternalFile via UTF8Reader) having already
stripped it. That implicit contract was fragile — a caller passing raw
BOM-prefixed bytes with a suffix would reach the parsers with the BOM intact
(SanitizeText does not strip it). Strip once at entry so every path and
parser sees clean bytes regardless of caller. No-op for already-stripped
input.
* refactor(lyrics): trim verbose comments to essential why
* refactor(lyrics): move LRC parser to its own lyrics_lrc.go
Extract parseLRC, the enhanced-LRC helpers (parseEnhancedLine, adjustGroup,
stripEnhancedMarkers, shiftELRCCues), parseTime, and the LRC regexes from
lyrics.go into lyrics_lrc.go, with the parseLRC tests in lyrics_lrc_test.go.
This makes the layout symmetric — one file per format (lrc/srt/ttml/yaml) —
and leaves lyrics.go holding only shared types and cue normalization. All
moved symbols were already LRC-private; no behavior change.
* refactor(lyrics): collapse ParseLyrics suffix/sniff branches into one loop
Both modes differ only in which formats to try, so select candidates in a
single loop (all formats when sniffing, the suffix's own otherwise) and run
them through parseFirstMatch once. Drops the projected-slice make+index and
the ContainsFunc closure; unmatched suffixes yield no candidates and fall to
the plain-text floor, as before.
* refactor(lyrics): apply simplify-review cleanups
- stripBOM: bytes.TrimPrefix instead of []byte<->string round-trip (no alloc)
- ParseLyrics: pre-size the candidates slice
- move isTTMLDocument to lyrics_ttml.go beside its only caller (the dispatch
layer should hold no per-format knowledge)
* refactor(lyrics): simplify test descriptions for structured lyrics
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor(lyrics): fold parseLyricsfile into lyricParser signature and rename file
- parseLyricsfile now matches the lyricParser signature directly (reads via
bytes.NewReader), removing the parseLyricsfileBytes adapter and the
string(contents) copy; the lyricFormats table references it directly.
- StructuredLyrics drops the vestigial LyricList{} init (json.Unmarshal
overwrites; MarshalJSON owns the empty->[] invariant).
- Rename lyricsfile.go -> lyrics_lyricsfile.go (and its test) to match the
lyrics_<format>.go convention used by lrc/srt/ttml.
* refactor(lyrics): move test-only parseTTML/parseSRT wrappers to test files
These zero-arg wrappers (defaulting lang to "xxx") had no production callers
after the consolidation — only the format tests used them. Move each beside
its tests so the production files carry no test-only code.
* build: exclude generated *_gen.go files from linting
The plugin host *_gen.go files (ndpgen output) were tripping the whitespace
linter despite carrying a generated marker. Exclude them by path so make lint
and the pre-push hook pass on untouched generated code.
* perf(lyrics): drop []byte/string round-trips in parsers
Apply code-review feedback to remove avoidable allocations in the lyrics
parsers. isTTMLDocument now takes []byte directly, so parseTTMLWithDefaultLang
no longer copies its buffer into a string before the TTML probe. parseSRTBlock
splits its block with strings.Split instead of converting to []byte and back
per line. ParseLyrics hoists strings.ToLower(suffix) out of the format loop.
No behavior change; the dropped len(scanner)==0 SRT guard was dead (strings.Split
never returns an empty slice, and the existing len(lines)==0 check still covers
empty input).
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor(lyrics): colocate and unexport cue-normalization helpers
Move the cue-normalization machinery out of lyrics.go into a dedicated
lyrics_normalize.go (with lyrics_normalize_test.go), leaving lyrics.go to hold
just the shared lyric types and their methods. lyrics.go was mixing the domain
type/contract definitions with format-agnostic post-processing.
Unexport normalizeLyrics, normalizeCueLines, and normalizeLineTiming: they have
no callers outside the model package, so they should not be part of its public
API. NormalizeCueEnds stays exported because the Subsonic enhanced-lyrics
serializer (server/subsonic/lyrics.go) resolves cue ends per agent group while
building the response; that is the only legitimate cross-package caller.
Also includes a small no-op robustness tweak in parseLRC: len(times) == 0
instead of times == nil (equivalent here, more idiomatic).
No behavior change.
* test(lyrics): add direct coverage for NormalizeCueEnds
NormalizeCueEnds is exported and carries the most intricate logic in the
normalization cluster (fill-from-next, fill-from-fallback, both clamps, and the
all-or-none clear), but was only exercised transitively. Add a focused spec
covering each branch plus the empty-input and no-mutation guarantees, bringing
the function to 100% coverage.
* test(lyrics): cover legacy getLyrics across formats and sources
Expand the legacy getLyrics e2e coverage from a single embedded-plain case to a
table over all six fixtures: embedded LRC/plain/TTML and sidecar LRC/SRT/YAML.
Each case asserts the v1 plain-text fallback contract — the structured lyric is
flattened to LRC-style plain text with no timing markup leaking through (no LRC
brackets, SRT arrows, or XML tags), regardless of the source format or whether
it is embedded or a sidecar file. This pins the behavior that synced TTML/SRT/
YAML formats degrade gracefully to plain text on the legacy endpoint.
* test(lyrics): cover songLyrics v1 vs v2 with word-level fixtures
Correct and expand the e2e lyrics coverage to match the OpenSubsonic songLyrics
extension contract:
- v1 (getLyricsBySongId, no enhanced): line-level lyrics with no cueLine, kind,
or agents — even for word-level formats (ELRC, Lyricsfile YAML).
- v2 (getLyricsBySongId?enhanced=true): word-level cueLine surfaces for ELRC and
YAML sources; kind="main" is set; a line-level source (SRT) still yields no
cueLine even when enhanced.
- legacy getLyrics (artist/title): the original Subsonic endpoint, flattening any
format to plain text. A prior commit mislabeled this as the "v1 contract";
getLyrics predates OpenSubsonic and is unrelated to the extension versions.
Drive these with the public-domain tests/fixtures/lyrics files (the same set the
parser benchmarks use) so the e2e content stays in sync and actually carries the
word-level timing needed to distinguish v1 from v2. The embedded "synced LRC"
fixture is upgraded to ELRC (word-level); track counts are unchanged, so the
rest of the suite is unaffected.
* test(lyrics): parameterize v2 enhanced coverage across all formats
Convert the v2 (enhanced) e2e block from three ad-hoc cases into a DescribeTable
covering all six formats, matching the v1 and legacy tables. Each entry declares
whether the source carries word-level timing: ELRC, TTML, and Lyricsfile YAML
surface a cueLine; LRC, SRT, and plain text do not. All six get kind="main".
Add word-level <span> timing to the first line of the auld-lang-syne.ttml
fixture so TTML exercises the word-level cueLine path (the parser already
supports <span begin/end>, but the fixture was line-level only). The first line
now yields the same five word cues as the ELRC and YAML fixtures, keeping the
table assertions uniform across formats.
* fix(lyrics): honor caller language when Lyricsfile YAML omits it
parseLyricsfile discarded the caller's language argument, so a Lyricsfile YAML
parsed from an embedded tag or plugin response with no metadata.language was
labeled "xxx" even when ParseLyrics was given a language. The SRT and TTML
parsers already use the caller language as their default; fall back to it here
too, preferring the document's own metadata.language when present.
Also reword a misleading TTML comment: isTTMLDocument still runs an XML decode
(it stops at the first element), so the skip avoids the full TTML parse, not the
XML decoder entirely.
* refactor(lyrics): consolidate lyrics parsing functions names
Signed-off-by: Deluan <deluan@navidrome.org>
* test(lyrics): drop test-only parse wrappers after parser rename
Commit
|
||
|
Yuuta
|
3a14faa033 |
feat(subsonic): add structured sidecar lyrics support with OpenSubsonic v2 karaoke cues and agent layers (#5076)
Expand backend lyrics support with richer sidecar formats and upgrade the OpenSubsonic songLyrics implementation to the version 2 structured karaoke contract, while preserving version 1 behavior by default. Sidecar formats and parsing: - Add a TTML parser (core/lyrics/ttml.go): clock time, offset time, bare decimal seconds, nested timing contexts, and token-level <span> timing for word/syllable karaoke. Parses Apple Music-style metadata tracks (translation and pronunciation/transliteration) and agent metadata into per-track agents[] plus per-cue-line agentId. Hydrates missing line timing from cue timing. - Add an SRT parser (core/lyrics/srt.go). - Add a LRCLIB Lyricsfile (.yaml/.yml) parser (model/lyricsfile.go): maps per-word lines[].words[] to cues with inclusive UTF-8 byte offsets and attributes overlapping lines to synthetic voice agents so parallel vocals split correctly in the enhanced response. - Extend LRC parsing for Enhanced LRC inline <mm:ss.xx> word-timing markers. - Add UTF-8 BOM and UTF-16 LE support for TTML/LRC sidecars. - Parse the above formats from embedded tags as well as sidecar files. Source resolution: - Default lyricspriority is now ".ttml,.yaml,.yml,.elrc,.lrc,.srt,.txt,embedded" so the new formats are discoverable without manual configuration. - Preserve configured source priority across duplicate media-file candidates instead of only checking the first DB match, so higher-priority sidecar lyrics on older duplicates can still win. - Raise the embedded-lyrics tag maxLength to 1 MB to fit word-timed TTML/Enhanced-LRC karaoke for a full song. OpenSubsonic songLyrics v2: - Advertise songLyrics versions [1, 2]. - With enhanced=true, getLyricsBySongId may return structuredLyrics.kind (main/translation/pronunciation), cueLine[] line-level karaoke groupings, cueLine.cue[] timed words/syllables with required UTF-8 byteStart/byteEnd, reusable structuredLyrics.agents[], and cueLine.agentId references. - Without enhanced=true, the response stays v1-compatible: no kind, no cueLine, no agents, no non-main tracks; the existing line[] payload is always populated so legacy clients keep working. Contract details: - cueLine is emitted only for synced lyrics with cue data. - Within a cueLine, cue.end is normalized all-or-none and overlaps are removed; overlaps across separate cueLines remain valid for parallel vocal layers. - Missing cue end-times are filled from the next cue or the parent line. - When cueLines share an index, the one whose agent has role "main" is first. - LyricCue.Value is serialized as XML chardata; cues with nil start are skipped rather than serialized as 0. Refactoring: - Move pure format parsers into model/ (lyrics.go, lyrics_ttml.go, lyrics_srt.go, lyrics_embedded.go, lyricsfile.go) and extract Subsonic response building into server/subsonic/lyrics.go. - Centralize lyric-kind constants and add Lyrics.EffectiveKind/IsMainKind. - Add gg.Clone helper. Spec references: https://github.com/opensubsonic/open-subsonic-api/discussions/213 https://github.com/opensubsonic/open-subsonic-api/pull/218 (songLyrics v2) https://github.com/opensubsonic/open-subsonic-api/pull/228 (cue byte offsets) |
||
|
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
|
08a027dbcc |
fix(transcoding): honor player forced format on the WebUI transcode flow (#5613)
* feat(stream): add ClientInfo.ForceFormat for browser-aware forced format Restricts the client to a forced transcoding format and suppresses direct play, but only when the client declares it supports that format. Part of #5583. * fix(transcoding): honor player forced format on getTranscodeDecision When the WebUI player has a forced transcoding format configured and the browser declares it can play that format, transcode to it (suppressing direct play). Fall back to normal negotiation with a warning when the format is unsupported. The MaxBitRate cap still applies on top. Fixes #5583. * test(e2e): cover player forced format on getTranscodeDecision Forced format honored when the client supports it, falls back to negotiation otherwise, and the MaxBitRate cap still applies on top. Part of #5583. * feat(ui): remove obsolete 'format ignored' helper text on player form The web player now honors the forced transcoding format, so the caveat added in #5611 no longer applies. Reverts the Transcoding field to a plain selector. Part of #5583. |
||
|
Deluan Quintão
|
c4c70519b5 |
fix(transcoding): enforce server-side player MaxBitRate on /rest/stream (#5611)
* fix(transcoding): enforce player MaxBitRate on getTranscodeDecision The Web UI streams via getTranscodeDecision, which (since #5473) ignored the server-side player config. Apply the player's MaxBitRate as a bitrate ceiling on the client's declared limits before MakeDecision, restoring per-player bitrate enforcement without reintroducing the forced-format override. Fixes #5583. * test(e2e): assert player MaxBitRate is enforced on getTranscodeDecision Invert the assertions added in #5473 that expected the player cap to be ignored; getTranscodeDecision now enforces it (issue #5583). * feat(ui): clarify web player ignores forced transcoding format Add helper text to the Transcoding field on the player edit form when the player is the NavidromeUI web client, since it enforces only the Max. Bit Rate, not the forced format. Part of issue #5583. * refactor(stream): extract ClientInfo.CapBitrate, share across transcode paths Move the player MaxBitRate ceiling logic into a canonical ClientInfo.CapBitrate method in core/stream, used by both getTranscodeDecision and the legacy ResolveRequest path. Removes handler-layer duplication and corrects a misleading comment that wrongly implied the legacy single-field cap was buggy. * fix(transcoding): downsample on legacy /stream when only player MaxBitRate is set A bare /stream or /download request from a player configured with a server-side MaxBitRate (but no forced format) was served raw, ignoring the cap. buildLegacyClientInfo now triggers DefaultDownsamplingFormat when the player MaxBitRate alone is below the source bitrate, matching the already-correct forced-format and request-bitrate paths. Part of #5583. * fix(ui): add Brazilian Portuguese translation for player transcoding helper text Translates the new resources.player.helperTexts.transcodingId key added for the web player transcoding-format clarification. Part of #5583. * fix(ui): restore Transcoding field styling and render helper text The TranscodingInput wrapper swallowed the variant SimpleForm injects into its direct children (field lost its outlined box) and put helperText on the ReferenceInput, which does not forward it to the input. Spread the form props onto ReferenceInput and move helperText to the SelectInput child so both the outlined styling and the helper text render. Part of #5583. * fix(i18n): update Brazilian Portuguese translation for album artist field Signed-off-by: Deluan <deluan@navidrome.org> * fix(ui): clean up comments in PlayerEdit component Signed-off-by: Deluan <deluan@navidrome.org> * test(ui): mock useTranslate in PlayerEdit test for determinism Avoid depending on ra-core's out-of-provider translation behavior, which can vary by version. Part of #5583. --------- Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
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
|
9a2eb483e8 |
fix(transcode): log warning for invalid or stale transcode tokens
Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
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
|
cf1f190bb5 |
fix(subsonic): use SQLite RANDOM() sorting in getRandomSongs, for faster results
Related to #5558 Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan
|
2a43c4683e |
chore: go fix
Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan Quintão
|
945d0ba1e2 |
fix(transcoding): cap concurrent transcodes to prevent ffmpeg DoS (#5522)
* feat(transcoding): add MaxConcurrent and MaxConcurrentPerUser config Introduce Transcoding.MaxConcurrent (default NumCPU()*2) and Transcoding.MaxConcurrentPerUser (default 3) to support upcoming concurrency limits on the streaming pipeline. No behavior change yet. Refs #5246 * feat(transcoding): add TranscodeLimiter with global and per-user caps Introduce a non-blocking limiter that gates concurrent transcodes. Returns ErrTooManyTranscodes immediately when the cap is reached so callers can translate it into a 429 response, rather than queuing requests. The per-user reservation is taken first to avoid burning a global slot that would only be rolled back when the per-user cap rejects the caller. Release is idempotent so wrapping the transcoder reader's Close is safe. Refs #5246 * feat(transcoding): cap concurrent transcodes in media streamer Acquire a TranscodeLimiter slot before spawning ffmpeg in the transcoding cache's read function, and release it when the resulting reader is closed. Raw streams and cache hits bypass the limiter so a single saturating client cannot block ordinary playback. When the cap is reached, ErrTooManyTranscodes bubbles up through cache.Get, ready for the HTTP layer to translate into a 429 response. Refs #5246 * feat(transcoding): return HTTP 429 with Retry-After when transcode cap is hit Map stream.ErrTooManyTranscodes to HTTP 429 in both the Subsonic API (/stream, /download) and the public share endpoint, including a 5s Retry-After hint. The Subsonic response still carries a failed-status envelope so clients that ignore HTTP codes also see the failure. Refs #5246 * feat(transcoding): default MaxConcurrent to 0 (disabled) Ship the limiter opt-in so existing installations are not affected by a behavior change on upgrade. Users hitting the DoS reported in #5246 can enable it by setting Transcoding.MaxConcurrent to a positive value (NumCPU()*2 is a reasonable starting point). Refs #5246 * fix(transcoding): make global and per-user caps independent Previously the limiter short-circuited to a no-op whenever MaxConcurrent was zero, silently ignoring a configured MaxConcurrentPerUser. Treat each cap independently so an operator can throttle per-user without enforcing a global ceiling (or vice versa), and only fall back to the no-op limiter when both caps are disabled. * fix(archiver): abort archive download when the transcode limiter rejects The album/artist/playlist zip writers were silently producing zip entries with headers but no data when ms.NewStream returned ErrTooManyTranscodes, because the per-file error was discarded by `_ = a.addFileToZip(...)`. The client received HTTP 200 with a corrupt zip and no indication that the server was rate-limited. Now the zip loop bails out as soon as it sees ErrTooManyTranscodes, and the Download handler swallows the error (the response status and Content-Disposition are already flushed by the time the limit is hit, so no 429 can be sent). The truncated zip surfaces the problem to the client; operators see a clear "transcode cap reached" warning in the server logs. Refs #5246 * fix(transcoding): release limiter slot on client close, not ffmpeg EOF Previously the slot was wrapped around the ffmpeg source reader, so it was only released by the cache's background copyAndClose goroutine when ffmpeg finished producing the file — meaning a client that disconnected after a single byte still held the slot for the full transcode duration. Under MaxConcurrent=N this serialized fresh requests behind abandoned encodes for minutes. Hand the release function back from the cache producer via the streamJob struct and wire it into the consumer-side Stream.Close. The HTTP handler already runs `defer stream.Close()`, so disconnect now frees the slot immediately. Cache hits never enter the producer and still pay no slot, and singleflight waiters on the same key correctly inherit no release (only the original producer's job holds the slot). Refs #5246 * fix(transcoding): skip per-user cap for anonymous requests Public share viewers have no user in context, so userName(ctx) returned the literal string "UNKNOWN" and the limiter mapped every anonymous viewer to the same bucket. With MaxConcurrentPerUser=N, only N unrelated anonymous clients could stream a viral share at any time — the opposite of the fairness the per-user cap is meant to provide. Introduce a limiterKey(ctx) helper that returns "" for anonymous callers (userName(ctx) is unchanged for logs), and teach Acquire to skip the per-user reservation when the key is empty. The global cap is still enforced for anonymous traffic and remains the protection against runaway anonymous load. Refs #5246 * refactor(transcoding): tidy limiter struct and centralize Retry-After Per review feedback: - Drop the redundant maxConcurrent field on transcodeLimiter; the channel capacity already enforces the global cap and the field was only used inside the constructor. - Only allocate the perUser map when MaxConcurrentPerUser > 0. - Move the Retry-After value into core/stream as RetryAfterSeconds so the Subsonic API and public-share handlers cannot drift if the window is later tuned. * fix(transcoding): do not log limiter rejections as cache failures NewStream was emitting an error-level "Error accessing transcoding cache" log whenever cache.Get returned anything non-nil, including the limiter's ErrTooManyTranscodes — even though the producer had already logged the rejection at warn level. The result was double logging and a misleading "cache failure" classification that buries real cache problems. Skip the error log when the cause is ErrTooManyTranscodes; the warn line from the producer is the canonical signal. * fix(archiver): open stream before writing zip entry header Per review: addFileToZip previously called z.CreateHeader before NewStream, so when the limiter rejected a transcode the zip already contained a 0-byte entry for that track. Open the source first and only write the header once the read side is ready; rejections now skip the entry entirely. The truncation comment in handleArchiveErr was also misleading — z.Close finalises the central directory, so the client receives a well-formed zip containing only the tracks written before the rejection, not a "truncated" archive. Reword to match reality. * fix(transcoding): hold slot for ffmpeg lifetime, force cancellable ctx The previous release-on-consumer-close design let a client open many unique transcodes, disconnect immediately, and still spawn the configured cap's worth of ffmpeg processes — the cache writer goroutine continued draining ffmpeg to disk after the client disappeared, defeating the DoS protection the limiter is meant to provide. Move the release back onto the source reader so the slot is freed only when ffmpeg actually exits (either EOF or context cancellation). To keep disconnects from leaking slots for the full transcode duration, force the request context into ffmpeg whenever the limiter is enabled — so client disconnect cancels the process and frees the slot promptly. When the limiter is disabled, the legacy EnableTranscodingCancellation behavior is preserved unchanged. Reported by codex and Copilot reviewers on #5522. |
||
|
Deluan
|
8897ec918e |
fix(subsonic): mark AlbumID3 songCount and created as required
The Subsonic API spec defines songCount and created as required attributes on AlbumID3, but they were tagged with omitempty in our response struct, allowing them to be silently dropped from responses (e.g. when songCount was 0). Created was also a *time.Time, which compounded the omitempty behavior. Remove omitempty from both fields and change Created from *time.Time to time.Time so they are always serialized, matching the spec contract that clients rely on. The buildAlbumID3 helper and its tests are updated for the non-pointer Created, and the AlbumWithSongsID3 snapshots are regenerated to include the now-always-present fields. |
||
|
Deluan
|
efe9291db0 |
refactor: multiple syntax updates for Go 1.26
Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan Quintão
|
a84f092d00 |
fix(subsonic): require admin access for Subsonic management endpoints (#5510)
* fix: require admin for radio mutations Subsonic internet radio station mutation endpoints are admin-only in the Subsonic and OpenSubsonic specs, but the router only required an authenticated player. Add a reusable Subsonic admin middleware and apply it to create, update, and delete radio routes while leaving the list endpoint available to authenticated users. Cover the middleware and router behavior with unit and e2e tests. * fix: streamline admin-only routes for internet radio station management Signed-off-by: Deluan <deluan@navidrome.org> * fix: use admin-only middleware for starting scans Signed-off-by: Deluan <deluan@navidrome.org> * test: align start scan authorization coverage StartScan authorization now lives in the shared Subsonic admin middleware instead of the handler. Remove the obsolete direct handler unit assertion so the package tests reflect the route-level guard covered by middleware and e2e tests. * fix: require admin for getUsers The Subsonic getUsers endpoint exposes user-list semantics and should use the same shared admin middleware as other admin-only management endpoints. Apply the route-level guard while leaving getUser unchanged, and update the multi-user e2e coverage to expect regular users to receive an authorization failure. * test: cover admin-only Subsonic access Add e2e coverage that admins can still call getUsers after the route-level guard and that regular authenticated users can still list internet radio stations. These cases capture the access boundaries raised during PR review. --------- Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Kendall Garner
|
339a6271f1 |
chore(subsonic): only log response body on send error when at trace level or higher (#5501) | ||
|
Deluan
|
f12e75aa11 |
feat(subsonic): add groupings field to OpenSubsonic Child response
Include the ID3 grouping tag in OpenSubsonic responses as an array of strings, per opensubsonic/open-subsonic-api#232. The grouping tag was already being extracted and stored in MediaFile.Tags via mappings.yaml aliases (GRP1, GROUPING, ©grp, wm/contentgroupdescription), so this change only adds the field to the response struct and populates it in both song and album child builders. Signed-off-by: Deluan <deluan@navidrome.org> |