mirror of
https://github.com/navidrome/navidrome.git
synced 2026-08-31 07:30:32 +00:00
437 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
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> |
||
|
Deluan Quintão
|
5f0245ea84
|
fix(server): prevent artwork throttle token starvation on slow clients (#5472)
* fix(server): prevent artwork throttle token starvation on slow clients Replace Chi's ThrottleBacklog middleware for artwork endpoints with a custom RequestThrottle that releases processing tokens before writing the HTTP response. Previously, a slow or stalled client could hold a throttle token indefinitely during io.Copy, exhausting all 2-4 slots and blocking artwork requests for all users (reported after 15+ days uptime). The new approach buffers artwork into memory while holding the token, releases it immediately, then writes the buffered response. A 30-second per-request write deadline (SetWriteTimeout) prevents stalled writes from blocking indefinitely. Throttle exhaustion is now logged with context for operator visibility. * refactor(server): simplify throttle to middleware with same API as Chi Restructure RequestThrottle from a DI-injected type into a drop-in middleware function with the same signature as Chi's ThrottleBacklog. Handlers are reverted to their original simple form (no throttle awareness), and the middleware is applied at route definition time just like before. This eliminates the DI dependency, removes the artworkThrottle field from both Router structs, and consolidates SetWriteTimeout into the throttle file. When limit <= 0, the middleware returns a passthrough so callers don't need a guard. Signed-off-by: Deluan <deluan@navidrome.org> * feat(server): add opt-out flag for buffered artwork throttle Add DevArtworkThrottleBuffered config (default true) that controls whether the new buffered ThrottleBacklog middleware is used. When set to false, it falls back to Chi's original middleware, giving users a safety valve in case the buffered implementation causes issues. Signed-off-by: Deluan <deluan@navidrome.org> * test(server): clean up throttle tests for clarity and speed Consolidate duplicate router setup into runTwoRequests() and slowClientTest() helpers. Replace time.Sleep-based token holding with channel synchronization, reducing suite time from ~7s to ~1.5s. Remove redundant test, fix duplicate comment block, and add comment explaining why slowTestWriter can't embed httptest.ResponseRecorder. * fix: release artwork throttle tokens on panic Defer the buffered artwork throttle release inside the handler closure so tokens are returned even when a downstream handler panics before response flushing. Document that the middleware buffers full responses in memory and add a regression test covering recovery after a panic. * fix: align buffered throttle response behavior Keep only the first status code written to the buffered artwork throttle response writer so it matches net/http semantics. Strengthen the opt-out test to verify DevArtworkThrottleBuffered=false uses Chi's original slow-client behavior instead of only checking shared 429 handling. * refactor(server): remove setWriteTimeout from throttle middleware SetWriteDeadline only constrains the server's Write syscall, not how fast the client reads from the TCP buffer. For artwork-sized responses (up to ~500KB), the kernel accepts the entire write immediately even over real network interfaces due to TCP buffer auto-tuning. Verified by testing with a stalled client over both loopback and en0 — the deadline never triggers. The actual protection comes from buffering + early token release, which is already in place. --------- Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan Quintão
|
ae0e0c89d9
|
feat(plugins): add PlaybackReport to scrobbler capability (#5452)
* feat(plugins): add PlaybackReport to Scrobbler interface and all implementations * feat(plugins): add PlaybackReport worker and dispatch in PlayTracker * feat(plugins): add PlaybackReportRequest to plugin scrobbler capability * chore(plugins): regenerate PDK files with PlaybackReport * feat(plugins): add PlaybackReport to test scrobbler plugin * feat(plugins): add PlaybackReport to plugin scrobbler adapter * refactor(plugins): fix double DB fetch in StateStopped and batch getActiveScrobblers - Hoist mf from scrobble branch so PlaybackReport reuses it instead of fetching again from DB - Call getActiveScrobblers once per drain batch instead of per-entry * chore(plugins): include generated scrobbler schema with PlaybackReport * fix(plugins): skip PlaybackReport for plugins that don't export it Plugins detected as scrobblers only need to export one scrobbler function. Older plugins that don't export nd_scrobbler_playback_report would cause noisy error logs on every reportPlayback call. Now errFunctionNotFound and errNotImplemented are treated as no-ops. * refactor: rename NowPlayingInfo to PlaybackReport Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename stopNowPlayingWorker to stopBackgroundWorkers Signed-off-by: Deluan <deluan@navidrome.org> * refactor: move NowPlaying and PlaybackReport logic to separate worker files Signed-off-by: Deluan <deluan@navidrome.org> * refactor(scrobbler): rename NowPlayingInfo to PlaybackSession and add expired state Rename NowPlayingInfo struct to PlaybackSession to better reflect its role as a complete playback session representation. Add UserId field to make sessions self-contained, removing redundant userId parameters from PlaybackReport interface method and internal dispatch functions. Introduce StateExpired internal state that fires when a session cache entry expires without an explicit stop, ensuring plugins always receive a terminal event regardless of client behavior. * fix(scrobbler): update playback state description to include 'expired' Signed-off-by: Deluan <deluan@navidrome.org> * fix(scrobbler): resolve data race in OnExpiration callback Capture conf.Server.EnableNowPlaying at construction time instead of reading it from the background ttlcache eviction goroutine. The previous code raced with test config cleanup that writes to the same field concurrently. * fix(scrobbler): return error when media file lookup fails in StateStopped Simplify the MediaFile population logic in the stopped case to return an error if the track cannot be found. A stop report with an empty MediaFile is useless to plugins, and returning the error allows clients to retry or alert the user when auto-scrobble is enabled. * refactor(scrobbler): use session data directly in PlaybackReport adapter Use info.Username from PlaybackSession instead of extracting it from context in the plugin adapter, since the session is now self-contained. Add debug/trace logging for session expiration and enqueue the expired report with a user-enriched context so downstream handlers can identify the user. --------- Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan Quintão
|
7e16b6acb5
|
feat(ui): replace UI scrobble with reportPlayback and redesign NowPlaying panel (#5448)
* feat(config): add UIPlaybackReportInterval setting * feat(server): expose playbackReportIntervalMs to UI config * feat(ui): add playbackReportIntervalMs config default * feat(ui): replace scrobble/nowPlaying with reportPlayback in subsonic API layer * feat(ui): replace scrobble logic with reportPlayback state machine in Player * refactor(ui): simplify Player heartbeat using useInterval hook - Replace manual setInterval/clearInterval with existing useInterval hook - Extract shared reportPlaybackUrl helper to deduplicate URL construction - Use ref for currentTrackId in beforeunload to stabilize effect deps - Have heartbeat read lastPositionMsRef instead of audioInstance.currentTime * feat(ui): redesign NowPlaying panel with Discord-style layout Show album art with play/pause overlay icon, track title, artist, album name, progress bar with position/duration, and username. * fix(ui): adjust NowPlaying panel height to fit 3 entries * fix(ui): send stopped on player close and tab close while paused - onBeforeDestroy now sends reportPlayback stopped before clearing queue - beforeunload sends stopped beacon regardless of pause state * feat(ui): animate NowPlaying progress bar with 1s client-side tick * fix(ui): account for playbackRate in NowPlaying progress interpolation * fix(ui): use timestamp-based interpolation for smooth NowPlaying progress Replace tick counter with fetchedAt timestamp so the progress bar advances smoothly without resetting on each server poll. * fix(ui): fix NowPlaying progress bar not animating Pass `now` (Date.now()) as a prop that changes every tick, so React.memo'd components actually re-render each second. * fix(ui): prevent progress bar reset on NowPlaying poll Set fetchedAt and now atomically on fetch so the elapsed offset starts at zero and the server's already-estimated positionMs is used as the base without a visible jump. * fix(ui): stamp entries with fetch time to prevent progress bar reset Embed _fetchedAt timestamp directly into each entry object so the position and its reference timestamp are always in the same state update, eliminating the React 17 multi-setState batching race. * fix(server): estimate position for starting state in GetNowPlaying GetNowPlaying was only estimating elapsed position for the "playing" state, returning raw positionMs=0 for "starting". Since the UI player sends "starting" once and then doesn't update until the 60s heartbeat, NowPlaying polls returned 0 for up to a minute, causing the progress bar to reset on every poll. * fix(ui): send playing immediately after starting to enable position estimation The server only estimates elapsed position for "playing" state in GetNowPlaying. The Player was sending "starting" once and then not updating until the 60s heartbeat, leaving the server state as "starting" with positionMs=0 for up to a minute. Now the Player follows up "starting" with an immediate "playing" call, transitioning the server state so position estimation works from the first poll. * fix(subsonic): fix getNowPlaying returning same playerId for all entries PlayerId was never incremented in the map callback, so every entry got playerId=1. This caused the UI to use duplicate React keys, mixing up rendered entries between players. Also use a stable composite key in the UI instead of the sequential playerId. * fix(ui): only send stopped beacon when tab is actually closing Move the reportPlaybackBeacon call from beforeunload to pagehide. beforeunload fires before the confirmation dialog, so cancelling the close would still send stopped. pagehide only fires when the page is actually being unloaded. * fix(ui): revert to beforeunload for stopped beacon pagehide does not fire reliably in Chrome when closing tabs. Use beforeunload instead — if the user cancels the close dialog, the heartbeat will re-register the NowPlaying entry on its next tick. * fix(ui): use synchronous XHR for stopped report on tab close Replace sendBeacon with synchronous XMLHttpRequest in beforeunload. This blocks the page from closing until the server acknowledges the stopped state, ensuring the NowPlaying entry is always removed. * fix(ui): fix confirmation dialog and use fetch keepalive for tab close - Move e.preventDefault() before the stopped report so the dialog always shows regardless of XHR errors - Use fetch with keepalive:true instead of sync XHR (more reliable, non-blocking, survives page teardown) - Fall back to sendBeacon if fetch throws * fix(ui): prevent heartbeat from re-adding entry after stopped on tab close Set a stoppedRef flag in beforeunload so the heartbeat interval skips sending playing reports after stopped has been sent. Without this, the heartbeat could re-register the NowPlaying entry after the stopped event removed it. * fix(ui): include client unique ID header in stopped report on tab close Root cause: reportPlaybackSync (fetch keepalive) did not include the X-ND-Client-Unique-Id header. Regular reportPlayback calls via httpClient include this header, and the server uses it as the playMap key. Without the header, the stopped call fell back to player.ID as the key, which didn't match the entry added with the UUID key. The playMap.Remove targeted the wrong key, so the entry persisted. Fix: export clientUniqueId from httpClient and include it as a header in the fetch keepalive request. * fix(ui): use pagehide for stopped report to avoid premature send beforeunload fires before the confirmation dialog, so the stopped event was sent even when the user cancelled closing. Move the stopped report to pagehide, which only fires when the page is actually being unloaded (after confirmation). * feat(server): broadcast NowPlaying SSE on every state change Previously, the SSE broadcast only fired when the NowPlaying count changed. Now it fires on every reportPlayback call (starting, playing, paused, stopped), so the NowPlaying panel gets instant updates for state transitions and position changes. The UI reducer stores a nowPlayingLastUpdate timestamp alongside the count, ensuring every SSE event triggers a re-fetch even when the count is unchanged (e.g., pause/resume). * fix(ui): clamp NowPlaying position to prevent negative time display * fix(ui): debounce NowPlaying fetches to prevent progress bar trembling During track changes, rapid SSE events (stopped, starting, playing) triggered multiple refetches within milliseconds, each resetting the interpolation base and causing the progress bar to oscillate. Skip fetches within 1 second of the previous fetch. * feat(ui): report playback position on seek Send a reportPlayback(playing) call when the user seeks/scrubs in the player, so the NowPlaying panel and server position stay in sync immediately instead of waiting for the next 60s heartbeat. * refactor: code review cleanup - Export clientUniqueIdHeader from httpClient, use in subsonic layer - Fix variable shadowing (now → fetchNow) in NowPlayingPanel fetchList - Fix onBeforeDestroy nested dep (read isRadio from ref instead) - Only broadcast SSE on state transitions, not heartbeat position updates - Only enqueue NowPlaying to external scrobblers on state transitions Signed-off-by: Deluan <deluan@navidrome.org> * fix(ui): use trailing-edge debounce for NowPlaying fetch Replace the leading-edge throttle (which fetched on the first event and blocked subsequent ones) with a trailing-edge debounce (300ms). During track transitions, the burst of events (stopped → starting → playing) now collapses into a single fetch after the burst settles, showing the new track immediately instead of briefly showing empty. * fix(ui): only show overlay on NowPlaying artwork when paused Signed-off-by: Deluan <deluan@navidrome.org> * refactor(ui): remove unnecessary sendBeacon fallback from reportPlaybackSync * refactor(ui): rename reportPlaybackSync to reportPlaybackKeepalive The function was never synchronous — it uses fetch with keepalive:true, which is fire-and-forget. The name now reflects the actual behavior. * style: format code with prettier * test: add tests for reportPlayback SSE broadcast and UI changes - play_tracker: verify SSE broadcast on every state transition and that broadcasts are skipped when EnableNowPlaying is false - activityReducer: verify nowPlayingLastUpdate timestamp is set - subsonic/index: verify reportPlayback URL construction Signed-off-by: Deluan <deluan@navidrome.org> * fix(ui): prevent NowPlaying from fetching every second when panel is open fetchList had unstable identity because it depended on doFetch (which depended on notify/dispatch). Each 1s setNow re-render recreated the callback chain, re-triggering the useEffect that calls fetchList. Use a ref for the fetch logic so fetchList has a stable identity with empty deps. * fix(ui): break fetch→dispatch→effect→fetch loop in NowPlaying panel The fetch dispatched nowPlayingCountUpdate on every result, which updated nowPlayingLastUpdate in Redux, which triggered the SSE effect to call fetchList again — creating a fetch loop. Fix: remove dispatch from fetch results. The badge count uses entries.length (from local state) when entries are loaded, falling back to Redux count (from SSE) when they aren't. SSE events remain the only trigger for nowPlayingLastUpdate, breaking the loop. * fix(ui): clear NowPlaying entries on panel close so badge uses SSE count * style: format code with prettier * fix: address code review feedback - Fix currentTime truthiness check to handle position 0 correctly - Report actual player state (playing/paused) on seek instead of always sending 'playing' - Remove idx from React key to avoid reorder issues - Add debounce timer cleanup on unmount - Keep entries on panel close so badge stays accurate from polling - Fix test description to match actual assertion * fix(ui): keep NowPlaying badge count accurate from polling Add a separate nowPlayingCountSync action that updates the Redux count without setting nowPlayingLastUpdate (which would trigger the SSE effect and cause a fetch loop). Polling results now sync the badge count via this action, so the badge stays accurate even when SSE is unavailable. * style: format code with prettier --------- Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan
|
556f345a10 |
chore(lint): upgrade golangci-lint, fix/ignore new errors
Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan Quintão
|
94eb6c522b
|
feat(subsonic): implement playbackReport OpenSubsonic extension (#5442)
* feat(req): add Float64Or helper for parsing float query params * feat(scrobbler): extend NowPlayingInfo with state/position/rate fields * feat(scrobbler): implement ReportPlayback with state machine and auto-scrobble * feat(responses): add state/positionMs/playbackRate to NowPlayingEntry * feat(subsonic): add reportPlayback endpoint handler * feat(subsonic): include state/positionMs/playbackRate in getNowPlaying response * feat(subsonic): register playbackReport OpenSubsonic extension * test(e2e): add reportPlayback endpoint e2e tests * refactor(scrobbler): simplify ReportPlayback — extract helpers, remove duplication - Add state constants and exported ValidStates map - Extract remainingTTL() helper (was duplicated 3x) - Merge playing/paused switch cases into single branch - Use Get instead of GetWithParticipants for non-stopped states - Guard NowPlayingCount broadcast with count-change detection - Use cache entry for NowPlaying dispatch instead of extra DB query - Remove redundant Position field from NowPlayingInfo * refactor(scrobbler): skip DB query in playing/paused when playMap has entry * fix(play_tracker): handle errors when adding/updating NowPlayingInfo in cache Signed-off-by: Deluan <deluan@navidrome.org> * refactor(play_tracker): replace sort with slices.SortFunc for NowPlayingInfo Signed-off-by: Deluan <deluan@navidrome.org> * fix(play_tracker): check all ReportPlayback errors in tests Replace _ = with explicit error assertions to avoid masking failures in intermediate calls. Signed-off-by: Deluan <deluan@navidrome.org> * test(e2e): use real PlayTracker and assert getNowPlaying after reportPlayback Replace noopPlayTracker with a real PlayTracker backed by the E2E database. E2E tests now verify the full round-trip: reportPlayback creates/updates/removes entries visible via getNowPlaying, including state, positionMs, and playbackRate fields. Export NewPlayTracker constructor for use outside the scrobbler package. * fix(play_tracker): account for playback rate in TTL and detect track switches The remainingTTL function now divides remaining time by the playback rate, so cache entries expire correctly at non-1x speeds (e.g., 2x playback halves the TTL). Zero/negative rates default to 1.0. The playing/paused case now checks if the cached MediaFile ID matches the reported mediaId, falling back to a DB fetch when the client switches tracks without sending stopped/starting. Adds parameterized tests for remainingTTL covering rate variations and edge cases. * fix(subsonic): validate positionMs and playbackRate in reportPlayback Reject negative positionMs values and invalid playbackRate values (NaN, Inf, zero, negative) at the API boundary before they reach TTL and position estimation math. Returns clear error messages for each case. * feat(play_tracker): add ClientId and ClientName to ReportPlayback parameters Signed-off-by: Deluan <deluan@navidrome.org> * refactor(play_tracker): replace NowPlaying method with ReportPlayback calls Signed-off-by: Deluan <deluan@navidrome.org> * refactor(play_tracker_test): remove redundant TTL behavior tests and clean up mockPluginLoader Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan Quintão
|
3e25ca3868
|
test: enable Subsonic response snapshot tests on Windows (#5427)
* fix(test): enable Subsonic response snapshot tests on Windows Replaced cupaloy with a simple custom snapshot matcher that normalizes CRLF line endings before comparison. The tests were skipped on Windows via a //go:build unix tag because Git for Windows checks out snapshot files with CRLF, while Go's xml/json.MarshalIndent always produces LF, causing direct string comparison to fail. The new matcher reads snapshot files with os.ReadFile and normalizes \r\n to \n before comparing. Also added a .gitattributes in the .snapshots directory to enforce LF checkout, and removed the now-unused cupaloy dependency. * fix(test): add UPDATE_SNAPSHOTS support to custom snapshot matcher Restore the ability to update snapshots via `make snapshots` (UPDATE_SNAPSHOTS=true), which was lost when replacing cupaloy with the custom matcher. |
||
|
Deluan Quintão
|
259c1a9484
|
feat(subsonic): add sonicSimilarity extension as plugin capability (#5419)
* feat(plugins): add sonicSimilarity capability types
Defines the SonicSimilarity plugin capability interface with
GetSonicSimilarTracks and FindSonicPath methods, along with
their request/response types.
* feat(sonic): add core sonic similarity service
Implements the Sonic service with HasProvider, GetSonicSimilarTracks,
and FindSonicPath, delegating to the PluginLoader and using the
Matcher for index-preserving library resolution.
* test(sonic): add sonic service unit tests
Covers HasProvider, GetSonicSimilarTracks, and FindSonicPath with
mock plugin loader and provider, verifying error propagation and
successful match resolution via the library matcher.
* feat(matcher): add MatchSongsToLibraryMap for index-preserving matching
Adds a new method alongside MatchSongsToLibrary that returns a
map[int]MediaFile keyed by input song index rather than a flat slice,
enabling callers to correlate similarity scores back to the original
position in the results.
* fix(sonic): check provider availability before MediaFile lookup
Avoids unnecessary DB call when no plugin is available, and ensures
the correct error path is tested.
* feat(plugins): add sonic similarity adapter
Adds SonicSimilarityAdapter implementing sonic.Provider, bridging the
plugin system to the core sonic service via Extism plugin functions.
Reuses existing songRefsToAgentSongs helper for SongRef conversion.
* feat(plugins): add LoadSonicSimilarity to plugin manager
Adds Manager.LoadSonicSimilarity method following the pattern of
LoadLyricsProvider, enabling the core sonic service to load a
SonicSimilarityAdapter from a named plugin.
* feat(subsonic): add sonicMatch response type
Add SonicMatch struct with Entry and Similarity fields, and a SonicMatches slice to the Subsonic response struct. These types support the OpenSubsonic sonicSimilarity extension for returning similarity-scored track results.
* feat(subsonic): add getSonicSimilarTracks and findSonicPath handlers
Add two new Subsonic API handlers for the sonicSimilarity OpenSubsonic extension: GetSonicSimilarTracks returns similarity-scored tracks similar to a given song, and FindSonicPath returns a path of tracks connecting two songs. Both handlers delegate to the sonic core service and map results to SonicMatch response types.
* feat(subsonic): advertise sonicSimilarity extension when plugin available
Update GetOpenSubsonicExtensions to conditionally include the sonicSimilarity extension only when a sonic similarity plugin provider is available. The nil guard ensures backward compatibility with tests that pass nil for the sonic field. Also update the existing test to pass the new nil parameter.
* feat(subsonic): wire sonic similarity service into router
Add the sonic.Sonic service to the Router struct and New() constructor, register the getSonicSimilarTracks and findSonicPath routes, and wire sonic.New and its PluginLoader binding into the Wire dependency injection graph. Update all existing test call sites to pass the new nil parameter. Regenerate wire_gen.go.
* fix(e2e): add sonic parameter to subsonic.New call in e2e tests
* test(subsonic): add sonicSimilarity extension advertisement tests
Restructures the GetOpenSubsonicExtensions test into two contexts: one verifying the baseline 5 extensions are returned when no sonic similarity plugin is configured, and one verifying that the sonicSimilarity extension is advertised (making 6 total) when a plugin loader reports an available provider. Adds a mockSonicPluginLoader to satisfy the sonic.PluginLoader interface without requiring a real plugin.
* feat(subsonic): add nil guard and e2e tests for sonic similarity endpoints
Handlers return ErrorDataNotFound when no sonic service is configured,
preventing nil panics. E2e tests verify both endpoints return proper
error responses when no plugin is available.
* fix(subsonic): return HTTP 404 when no sonic similarity plugin available
Endpoints are always registered but return 404 when no provider is
available, rather than a subsonic error code 70.
* refactor: clean up sonic similarity code after review
Extract shared helpers to reduce duplication across the sonic similarity
implementation: loadAllMatches in matcher consolidates the 4-phase
matching pipeline, songRefToAgentSong eliminates per-iteration slice
allocation in the adapter, sonicMatchResponse deduplicates response
building in handlers, and a package-level constant replaces raw
capability name strings in core/sonic.
* fix empty response shapes
Signed-off-by: Deluan <deluan@navidrome.org>
* test(plugins): add testdata plugin and e2e tests for sonic similarity
Add a test-sonic-similarity WASM plugin that implements both
GetSonicSimilarTracks and FindSonicPath via the generated sonicsimilarity
PDK. The plugin returns deterministic test data with decreasing similarity
scores and supports error injection via config. Adapter tests verify the
full round-trip through the WASM plugin including error handling. Also
includes regenerated PDK code from make gen.
* docs: update README to include new capabilities and usage examples for plugins
Signed-off-by: Deluan <deluan@navidrome.org>
* test(e2e): enhance sonic similarity tests with additional scenarios and mock provider
Signed-off-by: Deluan <deluan@navidrome.org>
* fix: address PR review feedback for sonic similarity
Fix incorrect field names in README documentation ({from, to} →
{startSong, endSong}) and remove unnecessary XML serialization test
from e2e suite since OpenSubsonic endpoints only use JSON.
* refactor: rename Matcher methods for conciseness
Rename MatchSongsToLibrary to MatchSongs and MatchSongsToLibraryMap to
MatchSongsIndexed. The Matcher receiver already establishes the "to
library" context, making that suffix redundant, and "Indexed" better
describes the intent (preserving input ordering) than "Map" which
describes the data structure.
* refactor: standardize variable naming for media files in sonic path methods
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: simplify plugin loading by introducing adapter constructors
Signed-off-by: Deluan <deluan@navidrome.org>
---------
Signed-off-by: Deluan <deluan@navidrome.org>
|
||
|
Deluan Quintão
|
64c8d3f4c5
|
ci: run Go tests on Windows (#5380)
* ci(windows): add skeleton go-windows job (compile-only smoke test)
* ci(windows): fix comment to reference Task 7 not Task 6
* ci(windows): harden PATH visibility and set explicit bash shell
* ci(windows): enable full go test suite and ndpgen check
* test(gotaglib): skip Unix-only permission tests on Windows
* test(lyrics): skip Windows-incompatible tests
* test(utils): skip Windows-incompatible tests
* test(mpv): skip Windows-incompatible playback tests
Skip 3 subprocess-execution tests that rely on Unix-style mpv
invocation; .bat output includes \r-terminated lines that break
argument parsing (#TBD-mpv-windows).
* test(storage): skip Windows-incompatible tests
Skip relative-path test where filepath.Join uses backslash but the
storage implementation returns a forward-slash URL path
(#TBD-path-sep-storage).
* test(storage/local): skip Windows-incompatible tests
Skip 13 tests that fail because url.Parse("file://" + windowsPath)
treats the drive letter colon as an invalid port; also skip the
Windows drive-letter path test that exposes a backslash vs
forward-slash normalisation bug (#TBD-path-sep-storage-local).
* test(playlists): skip Windows-incompatible tests
* test(model): skip Windows-incompatible tests
* test(model/metadata): skip Windows-incompatible tests
* test(core): skip Windows-incompatible tests
AbsolutePath uses filepath.Join which produces OS-native path separators;
skip the assertion test on Windows until the production code is fixed
(#TBD-path-sep-core).
* test(artwork): skip Windows-incompatible tests
Artwork readers produce OS-native path separators on Windows while tests
assert forward-slash paths; skip 11 affected tests pending a fix in
production code (#TBD-path-sep-artwork).
* test(persistence): skip Windows-incompatible tests
Skip flaky timestamp comparison (#TBD-flake-persistence) and path-separator
real-bugs (#TBD-path-sep-persistence) in FolderRepository.GetFolderUpdateInfo
which uses filepath.Clean/os.PathSeparator converting stored forward-slash paths
to backslashes on Windows.
* test(scanner): skip Windows-incompatible tests
Skip symlink tests (Unix-assumption), ndignore path-separator bugs
(#TBD-path-sep-scanner) in processLibraryEvents/resolveFolderPath where
filepath.Rel/filepath.Split return backslash paths incompatible with fs.FS
forward-slash expectations, error message mismatch on Windows, and file
format upgrade detection (#TBD-path-sep-scanner).
* test(plugins): skip Windows-incompatible tests
Add //go:build !windows tags to test files that reference the suite
bootstrap (testManager, testdataDir, createTestManager) which is only
compiled on non-Windows. Add a Windows-only suite stub that skips all
specs via BeforeEach to prevent [build failed] on Windows CI.
* test(server): skip Windows-incompatible tests
Skip createUnixSocketFile tests that rely on Unix file permission bits
(chmod/fchmod) which are not supported on Windows.
* test(nativeapi): skip Windows-incompatible tests
Skip the i18n JSON validation test that uses filepath.Join to build
embedded-FS paths; filepath.Join produces backslashes on Windows which
breaks fs.Open (embedded FS always uses forward slashes).
* test(e2e): skip Windows-incompatible tests
On Windows, SQLite holds file locks that prevent the Ginkgo TempDir
DeferCleanup from deleting the DB file. Register an explicit db.Close
DeferCleanup (LIFO before TempDir cleanup) on Windows so the file lock
is released before the temp directory is removed.
* test(windows): fix e2e AfterSuite and skip remaining scanner path test
* test(scanner): skip another Windows path-sep test (#TBD-path-sep-scanner)
* test(subsonic): skip timing-flaky test on Windows (#TBD-flake-time-resolution-subsonic)
* test(scanner): skip 'detects file moved to different folder' on Windows
* test(scanner): consolidate 'Library changes' Windows skips into BeforeEach
* test(scanner): close DB before TempDir cleanup to fix Windows file lock
* test(scanner): skip ScanFolders suite on Windows instead of closing shared DB
* ci: retrigger for Windows soak run 2/3
* ci: retrigger for Windows soak run 3/3
* ci: retrigger for Windows soak run 3/3 (take 2)
* test(scanner): skip Multi-Library suite on Windows (SQLite file lock)
* ci(windows): promote go-windows to blocking status check
* test(plugins): run platform-neutral specs on Windows, drop blanket Skip
* test(windows): make tests cross-platform instead of skipping
- subsonic: back-date submissionTime baseline by 1s so
BeTemporally(">") passes under millisecond clock resolution
- persistence: sleep briefly between Put calls so UpdatedAt is
strictly after CreatedAt on low-resolution clocks
- utils/files: close tempFile before os.Remove so the test works on
Windows (where an open handle holds a file lock)
- tests.TempFile: close the handle before returning; metadata tests
no longer leak the open file into Ginkgo's TempDir cleanup
Resolves Copilot review comments on #5380.
* test(tests): add SkipOnWindows helper to reduce boilerplate
Introduces tests.SkipOnWindows(reason) that wraps the 3-line
runtime.GOOS guard pattern used in every Windows-skipped spec.
* test(adapters): use tests.SkipOnWindows helper
* test(core): use tests.SkipOnWindows helper
* test(model): use tests.SkipOnWindows helper
* test(persistence): use tests.SkipOnWindows helper
* test(scanner): use tests.SkipOnWindows helper
* test(server): use tests.SkipOnWindows helper
* test(plugins): run pure-Go unit tests on Windows
config_validation_test, manager_loader_test, and migrate_test have no
WASM/exec dependencies and don't rely on the make-built test plugins
from plugins_suite_test.go. Let them run on Windows too.
|
||
|
Deluan Quintão
|
ab2f1b45de
|
perf: reduce hot-path heap escapes from value-param pointer aliasing (#5342)
* perf(subsonic): keep album/mediafile params on stack in response helpers Two helpers were forcing their entire value parameter onto the heap via pointer-to-field aliasing, adding one full-struct heap allocation per response item on hot Subsonic endpoints (search3, getAlbumList2, etc.). - childFromMediaFile assigned &mf.BirthTime to the returned Child, pulling the whole ~1KB model.MediaFile to the heap on every call. - buildDiscSubtitles passed &a.UpdatedAt to NewArtworkID inside a loop, pulling the whole model.Album to the heap on every album with discs. Both now copy the time.Time to a stack-local and use gg.P / &local so only the small time.Time escapes. Verified via go build -gcflags=-m=2: moved to heap: mf and moved to heap: a are gone at these sites. * perf(metadata): avoid per-track closure allocations in PID computation createGetPID was a factory that returned nested closures capturing mf model.MediaFile (~992 bytes) by reference. Since it is called three times per track during scans (trackPID, albumID, artistID), every track triggered the allocation of three closures plus a heap copy of the full MediaFile. Refactor the body into package-level functions (computePID, getPIDAttr) that take hash as an explicit parameter and the inner slice.Map callback to an indexed for loop, removing the closure-capture of mf entirely. trackPID/albumID/artistID now call computePID directly. The tiny createGetPID wrapper was kept only for tests; move the closure-building into the test file so production has no dead API. Verified via go build -gcflags=-m=2 on model/metadata: no "moved to heap: mf" anywhere in persistent_ids.go, and the callers in map_mediafile.go / map_participants.go no longer heap-promote their MediaFile argument. |
||
|
Deluan Quintão
|
9b0bfc606b
|
fix(subsonic): always emit required created field on AlbumID3 (#5340)
* fix(subsonic): always emit required `created` field on AlbumID3
Strict OpenSubsonic clients (e.g. Navic via dev.zt64.subsonic) reject
search3/getAlbum/getAlbumList2 responses that omit the `created` field,
which the spec marks as required. Navidrome was dropping it whenever
the album's CreatedAt was zero.
Root cause was threefold:
1. buildAlbumID3/childFromAlbum conditionally emitted `created`, so a
zero CreatedAt became a missing JSON key.
2. ToAlbum's `older()` helper treated a zero BirthTime as the minimum,
so a single track with missing filesystem birth time could poison
the album aggregation.
3. phase_1_folders' CopyAttributes copied `created_at` from the previous
album row unconditionally, propagating an already-zero value forward
on every metadata-driven album ID change. Since sql_base_repository
drops `created_at` on UPDATE, a poisoned row could never self-heal.
Fixes:
- Always emit `created`, falling back to UpdatedAt/ImportedAt when
CreatedAt is zero. Adds albumCreatedAt() helper used by both
buildAlbumID3 and childFromAlbum.
- Guard `older()` against a zero second argument.
- Skip the CopyAttributes call in phase_1_folders when the previous
album's created_at is zero, so the freshly-computed value survives.
- New migration backfills existing broken rows from media_file.birth_time
(falling back to updated_at).
Tested against a real DB: repaired 605/6922 affected rows, no side
effects on healthy rows.
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor(subsonic): return albumCreatedAt by value to avoid heap escape
Returning *time.Time from albumCreatedAt caused Go escape analysis to
move the entire model.Album parameter to the heap, since the returned
pointer aliased a field of the value receiver. For hot endpoints like
getAlbumList2 and search3, this meant one full-struct heap allocation
per album result.
Return time.Time by value and let callers wrap it with gg.P() to take
the address locally. Only the small time.Time value escapes; the
model.Album struct stays on the stack. Also corrects the doc comment
to reflect the actual guarantee ("best-effort" rather than "non-zero"),
matching the test case that exercises the all-zero fallback.
---------
Signed-off-by: Deluan <deluan@navidrome.org>
|
||
|
Deluan
|
23f3556371 |
fix(subsonic): strip OpenSubsonic extensions from playlists for legacy clients
buildOSPlaylist was the only OpenSubsonic builder function missing the LegacyClients guard, causing attributes like `validUntil` and `readonly` to appear in playlist XML responses for legacy clients like DSub2000. This caused a crash when DSub2000 tried to parse evaluated smart playlists containing the `validUntil` attribute. |
||
|
Deluan
|
c60637de24 |
fix(subsonic): return proper artwork ID format in getInternetRadioStations
The coverArt field was returning the raw uploaded image filename instead
of the standard ra-{id} artwork ID format. This caused getCoverArt to
fail when clients passed the coverArt value directly. Now uses
CoverArtID().String() consistent with how albums, artists, and playlists
return their coverArt values. Fixes #5293.
|
||
|
Deluan
|
f33ca75378 |
refactor: rename EnableCoverArtUpload to EnableArtworkUpload
The config flag gates all image uploads (artists, radios, playlists), not just cover art. Rename it to accurately reflect its scope across the backend config, native API permission check, Subsonic CoverArtRole, serve_index JSON key, and frontend config. |
||
|
Deluan
|
03608d3eef |
feat(subsonic): add coverArt to internetRadioStation response
Add OpenSubsonic coverArt extension to GetInternetRadios, showing uploaded radio images for non-legacy clients. Ref: https://github.com/opensubsonic/open-subsonic-api/pull/224 Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan Quintão
|
ba8d427890
|
feat(ui): add cover art support for internet radio stations (#5229)
* feat(artwork): add KindRadioArtwork and EntityRadio constant * feat(model): add UploadedImage field and artwork methods to Radio * feat(model): add Radio to GetEntityByID lookup chain * feat(db): add uploaded_image column to radio table * feat(artwork): add radio artwork reader with uploaded image fallback * feat(api): add radio image upload/delete endpoints * feat(ui): add radio artwork ID prefix to getCoverArtUrl * feat(ui): add cover art display and upload to RadioEdit * feat(ui): add cover art thumbnails to radio list * feat(ui): prefer artwork URL in radio player helper * refactor: remove redundant code in radio artwork - Remove duplicate Avatar rendering in RadioList by reusing CoverArtField - Remove redundant UpdatedAt assignment in radio image handlers (already set by repository Put) * refactor(ui): extract shared useImageLoadingState hook Move image loading/error/lightbox state management into a shared useImageLoadingState hook in common/. Consolidates duplicated logic from AlbumDetails, PlaylistDetails, RadioEdit, and artist detail views. * feat(ui): use radio placeholder icon when no uploaded image Remove album placeholder fallback from radio artwork reader so radios without an uploaded image return ErrUnavailable. On the frontend, show the internet-radio-icon.svg placeholder instead of requesting server artwork when no image is uploaded, allowing favicon fallback in the player. * refactor(ui): update defaultOff fields in useSelectedFields for RadioList Signed-off-by: Deluan <deluan@navidrome.org> * fix: address code review feedback - Add missing alt attribute to CardMedia in RadioEdit for accessibility - Fix UpdateInternetRadio to preserve UploadedImage field by fetching existing radio before updating (prevents Subsonic API from clearing custom artwork) - Add Reader() level tests to verify ErrUnavailable is returned when radio has no uploaded image * refactor: add colsToUpdate to RadioRepository.Put Use the base sqlRepository.put with column filtering instead of hand-rolled SQL. UpdateInternetRadio now specifies only the Subsonic API fields, preventing UploadedImage from being cleared. Image upload/delete handlers specify only UploadedImage. * fix: ensure UpdatedAt is included in colsToUpdate for radio Put --------- Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan Quintão
|
3f7226d253
|
fix(server): improve transcoding failure diagnostics and error responses (#5227)
* fix(server): capture ffmpeg stderr and warn on empty transcoded output When ffmpeg fails during transcoding (e.g., missing codec like libopus), the error was silently discarded because stderr was sent to io.Discard and the HTTP response returned 200 OK with a 0-byte body. - Capture ffmpeg stderr in a bounded buffer (4KB) and include it in the error message when the process exits with a non-zero status code - Log a warning when transcoded output is 0 bytes, guiding users to check codec support and enable Trace logging for details - Remove log level guard so transcoding errors are always logged, not just at Debug level Signed-off-by: Deluan <deluan@navidrome.org> * fix(server): return proper error responses for empty transcoded output Instead of returning HTTP 200 with 0-byte body when transcoding fails, return a Subsonic error response (for stream/download/getTranscodeStream) or HTTP 500 (for public shared streams). This gives clients a clear signal that the request failed rather than a misleading empty success. Signed-off-by: Deluan <deluan@navidrome.org> * test(e2e): add tests for empty transcoded stream error responses Add E2E tests verifying that stream and download endpoints return Subsonic error responses when transcoding produces empty output. Extend spyStreamer with SimulateEmptyStream and SimulateError fields to support failure injection in tests. Signed-off-by: Deluan <deluan@navidrome.org> * refactor(server): extract stream serving logic into Stream.Serve method Extract the duplicated non-seekable stream serving logic (header setup, estimateContentLength, HEAD draining, io.Copy with error/empty detection) from server/subsonic/stream.go and server/public/handle_streams.go into a single Stream.Serve method on core/stream. Both callers now delegate to it, eliminating ~30 lines of near-identical code. * fix(server): return 200 with empty body for stream/download on empty transcoded output Don't return a Subsonic error response when transcoding produces empty output on stream/download endpoints — just log the error and return 200 with an empty body. The getTranscodeStream and public share endpoints still return HTTP 500 for empty output. Stream.Serve now returns (int64, error) so callers can check the byte count. --------- Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Kendall Garner
|
f39d75e7d2
|
fix(subsonic): never omit duration for AlbumID3 (#5217) | ||
|
Simon Teixidor
|
8f05f7815e
|
fix(server): use http.TimeFormat for Last-Modified header (#5219)
Navidrome returns Last-Modified values like `Fri, 12 Dec 2025 03:32:26 UTC`. This is invalid according to RFC 7231 which requires HTTP dates to use GMT instead of UTC. Switch to http.TimeFormat instead of time.RFC1123 to resolve the issue. |
||
|
Deluan
|
a887521d7a |
fix(subsonic): always include mandatory title field in Child responses
Removed `omitempty` from the `Title` struct tag in the `Child` response type. The Subsonic/OpenSubsonic API spec requires `title` to be a mandatory field, but songs with empty titles caused the field to be omitted entirely, crashing clients like Symfonium during sync. Ref: https://support.symfonium.app/t/app-gets-stuck-on-syncing-large-database/13004/8 |
||
|
Deluan Quintão
|
49a14d4583
|
feat(artwork): add per-disc cover art support (#5182)
* feat(artwork): add KindDiscArtwork and ParseDiscArtworkID Add new disc artwork kind with 'dc' prefix for per-disc cover art support. The composite ID format is albumID:discNumber, parsed by the new ParseDiscArtworkID helper. * feat(conf): add DiscArtPriority configuration option Default: 'disc*.*, cd*.*, embedded'. Controls how per-disc cover art is resolved, following the same pattern as CoverArtPriority and ArtistArtPriority. * feat(artwork): implement extractDiscNumber helper Extracts disc number from filenames based on glob patterns by parsing leading digits from the wildcard-matched portion. Used for matching disc-specific artwork files like disc1.jpg. * feat(artwork): implement fromDiscExternalFile source function Disc-aware variant of fromExternalFile that filters image files by disc number (extracted from filename) or folder association (for multi-folder albums). * feat(artwork): implement discArtworkReader Resolves disc artwork using DiscArtPriority config patterns. Supports glob patterns with disc number extraction, embedded images from first track, and falls back to album cover art. Handles both multi-folder and single-folder multi-disc albums. * feat(artwork): register disc artwork reader in dispatcher Add KindDiscArtwork case to getArtworkReader switch, routing disc artwork requests to the new discArtworkReader. * feat(subsonic): add CoverArt field to DiscTitle response Implements OpenSubsonic PR #220: optional cover art ID in DiscTitle responses for per-disc artwork support. * feat(subsonic): populate CoverArt in DiscTitle responses Each DiscTitle now includes a disc artwork ID (dc-albumID:discNum) that clients can use with getCoverArt to retrieve per-disc artwork. * style: fix file permission in test to satisfy gosec * feat(ui): add disc cover art display and lightbox functionality Signed-off-by: Deluan <deluan@navidrome.org> * refactor: simplify disc artwork code - Add DiscArtworkID constructor to encapsulate the "albumID:discNumber" format in one place - Convert fromDiscExternalFile to a method on discArtworkReader, reducing parameter count from 6 to 2 - Remove unused rootFolder field from discArtworkReader * style: fix prettier formatting in subsonic index * style(ui): move cursor style to makeStyles in SongDatagrid * feat(artwork): add discsubtitle option to DiscArtPriority Allow matching disc cover art by the disc's subtitle/name. When the "discsubtitle" keyword is in the priority list, image files whose stem matches the disc subtitle (case-insensitive) are used. This is useful for box sets with named discs (e.g., "The Blue Disc.jpg"). * feat(configuration): update discartpriority to include cover art options Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Kendall Garner
|
903e3f070f
|
fix(subsonic): always return required playqueue fields (#5172) | ||
|
Deluan Quintão
|
d8bc41fbb1
|
fix: use ADTS for AAC transcoding, temporarily exclude AAC from transcode decisions (#5167)
* fix: use ADTS format for AAC transcoding to avoid silent output on ffmpeg 8.0+ The fragmented MP4 muxer (`-f ipod -movflags frag_keyframe+empty_moov`) produces corrupt/silent audio when ffmpeg pipes to stdout, confirmed on ffmpeg 8.0+. The moof atom offset values are zeroed out in pipe mode, causing AAC decoder errors. Switch to `-f adts` (raw AAC framing) which works reliably via pipe and is widely supported by clients including UPnP/Sonos devices. * fix: exclude AAC from transcode decision, as it is not working for Sonos. Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan Quintão
|
767744a301
|
refactor: rename core/transcode to core/stream, simplify MediaStreamer (#5166)
* refactor: rename core/transcode directory to core/stream * refactor: update all imports from core/transcode to core/stream * refactor: rename exported symbols to fit core/stream package name * refactor: simplify MediaStreamer interface to single NewStream method Remove the two-method interface (NewStream + DoStream) in favor of a single NewStream(ctx, mf, req) method. Callers are now responsible for fetching the MediaFile before calling NewStream. This removes the implicit DB lookup from the streamer, making it a pure streaming concern. * refactor: update all callers from DoStream to NewStream * chore: update wire_gen.go and stale comment for core/stream rename * refactor: update wire command to handle GO_BUILD_TAGS correctly Signed-off-by: Deluan <deluan@navidrome.org> * fix: distinguish not-found from internal errors in public stream handler * refactor: remove unused ID field from stream.Request * refactor: simplify ResolveRequestFromToken to receive *model.MediaFile Move MediaFile fetching responsibility to callers, making the method focused on token validation and request resolution. Remove ErrMediaNotFound (no longer produced). Update GetTranscodeStream handler to fetch the media file before calling ResolveRequestFromToken. * refactor: extend tokenTTL from 12 to 48 hours Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan Quintão
|
d4b2499e1e
|
fix(server): return correct scanType in startScan response (#5159)
* fix(api): return correct scanType in startScan response The startScan endpoint launches the scan in a goroutine and immediately calls GetScanStatus to build the response. Because the scanner hasn't had time to initialize and write its state to the database, the response contained stale data from the previous scan (e.g., scanType "quick" when fullScan=true was requested). Add a polling loop that waits briefly (up to 3s, polling every 50ms) for the scanner to report Scanning=true before returning the status. If the timeout expires, it falls back to the current behavior (no regression). Fixes #5158 * fix(api): use ticker/timer with context cancellation for scan polling Replace time.Sleep loop with proper ticker, timer, and ctx.Done() handling so the poll exits cleanly on timeout or client disconnect. * fix(api): handle fast scan completion in poll loop Add a channel to detect when the scan goroutine finishes before the poll loop observes Scanning=true, avoiding a 3s timeout on very fast scans. Use defer close to handle both success and error paths. |
||
|
Deluan Quintão
|
ae1e0ddb11
|
feat(subsonic): implement OpenSubsonic Transcoding extension (#4990)
* feat(subsonic): implement transcode decision logic and codec handling for media files Signed-off-by: Deluan <deluan@navidrome.org> * fix(subsonic): update codec limitation structure and decision logic for improved clarity Signed-off-by: Deluan <deluan@navidrome.org> * fix(transcoding): update bitrate handling to use kilobits per second (kbps) across transcode decision logic Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): simplify container alias handling in matchesContainer function Signed-off-by: Deluan <deluan@navidrome.org> * fix(transcoding): enforce POST method for GetTranscodeDecision and handle non-POST requests Signed-off-by: Deluan <deluan@navidrome.org> * feat(transcoding): add enums for protocol, comparison operators, limitations, and codec profiles in transcode decision logic Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): streamline limitation checks and applyLimitation logic for improved readability and maintainability Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): replace strings.EqualFold with direct comparison for protocol and limitation checks Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): rename token methods to CreateTranscodeParams and ParseTranscodeParams for clarity Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): enhance logging for transcode decision process and client info conversion Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): rename TranscodeDecision to Decider and update related methods for clarity Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): enhance transcoding config lookup logic for audio codecs Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): enhance transcoding options with sample rate support and improve command handling Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): add bit depth support for audio transcoding and enhance related logic Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): enhance AAC command handling and support for audio channels in streaming Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): streamline transcoding logic by consolidating stream parameter handling and enhancing alias mapping Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): update default command handling and add codec support for transcoding Signed-off-by: Deluan <deluan@navidrome.org> * fix: implement noopDecider for transcoding decision handling in tests Signed-off-by: Deluan <deluan@navidrome.org> * fix: address review findings for OpenSubsonic transcoding PR Fix multiple issues identified during code review of the transcoding extension: add missing return after error in shared stream handler preventing nil pointer panic, replace dead r.Body nil check with MaxBytesReader size limit, distinguish not-found from other DB errors, fix bpsToKbps integer truncation with rounding, add "pcm" to isLosslessFormat for consistency with model.IsLossless(), add sampleRate/bitDepth/channels to streaming log, fix outdated test comment, and add tests for conversion functions and GetTranscodeStream parameter passing. * feat(transcoding): add sourceUpdatedAt to decision and validate transcode parameters Signed-off-by: Deluan <deluan@navidrome.org> * fix: small issues Updated mock AAC transcoding command to use the new default (ipod with fragmented MP4) matching the migration, ensuring tests exercise the same buildDynamicArgs code path as production. Improved archiver test mock to match on the whole StreamRequest struct instead of decomposing fields, making it resilient to future field additions. Added named constants for JWT claim keys in the transcode token and wrapped ParseTranscodeParams errors with ErrTokenInvalid for consistency. Documented the IsLossless BitDepth fallback heuristic as temporary until Codec column is populated. Signed-off-by: Deluan <deluan@navidrome.org> * fix(transcoding): adapt transcode claims to struct-based auth.Claims Updated transcode token handling to use the struct-based auth.Claims introduced on master, replacing the previous map[string]any approach. Extended auth.Claims with transcoding-specific fields (MediaID, DirectPlay, UpdatedAt, Channels, SampleRate, BitDepth) and added float64 fallback in ClaimsFromToken for numeric claims that lose their Go type during JWT string serialization. Also added the missing lyrics parameter to all subsonic.New() calls in test files. * feat(model): add ProbeData field and UpdateProbeData repository method Add probe_data TEXT column to media_file for caching ffprobe results. Add UpdateProbeData to MediaFileRepository interface and implementations. Use hash:"ignore" tag so probe data doesn't affect MediaFile fingerprints. * feat(ffmpeg): add ProbeAudioStream for authoritative audio metadata Add ProbeAudioStream to FFmpeg interface, using ffprobe to extract codec, profile, bitrate, sample rate, bit depth, and channels. Parse bits_per_raw_sample as fallback for FLAC/ALAC bit depth. Normalize "unknown" profile to empty string. All parseProbeOutput tests use real ffprobe JSON from actual files. * feat(transcoding): integrate ffprobe into transcode decisions Add ensureProbed to probe media files on first transcode decision, caching results in probe_data. Build SourceStream from probe data with fallback to tag-based metadata. Refactor decision logic to pass StreamDetails instead of MediaFile, enabling codec profile limitations (e.g., audioProfile) to use probe data. Add normalizeProbeCodec to map ffprobe codec names (dsd_lsbf_planar, pcm_s16le) to internal names (dsd, pcm). NewDecider now accepts ffmpeg.FFmpeg; wire_gen.go regenerated. * feat(transcoding): add DevEnableMediaFileProbe config flag Add DevEnableMediaFileProbe (default true) to allow disabling ffprobe- based media file probing as a safety fallback. When disabled, the decider uses tag-based metadata from the scanner instead. * test(transcode): add ensureProbed unit tests Test probing when ProbeData is empty, skipping when already set, error propagation from ffprobe, and DevEnableMediaFileProbe flag. * refactor(ffmpeg): use command constant and select_streams for ProbeAudioStream Move ffprobe arguments to a probeAudioStreamCmd constant, following the same pattern as extractImageCmd and probeCmd. Add -select_streams a:0 to only probe the first audio stream, avoiding unnecessary parsing of video and artwork streams. Derive the ffprobe binary path safely using filepath.Dir/Base instead of replacing within the full path string. * refactor(transcode): decouple transcode token claims from auth.Claims Remove six transcode-specific fields (MediaID, DirectPlay, UpdatedAt, Channels, SampleRate, BitDepth) from auth.Claims, which is shared with session and share tokens. Transcode tokens are signed parameter-passing tokens, not authentication tokens, so coupling them to auth created misleading dependencies. The transcode package now owns its own JWT claim serialization via Decision.toClaimsMap() and paramsFromToken(), using generic auth.EncodeToken/DecodeAndVerifyToken wrappers that keep TokenAuth encapsulated. Wire format (JWT claim keys) is unchanged, so in-flight tokens remain compatible. Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcode): simplify code after review Extract getIntClaim helper to eliminate repeated int/int64/float64 JWT claim extraction pattern in paramsFromToken and ClaimsFromToken. Rewrite checkIntLimitation as a one-liner delegating to applyIntLimitation. Return probe result from ensureProbed to avoid redundant JSON round-trip. Extract toResponseStreamDetails helper and mediaTypeSong constant in the API layer, and use transcode.ProtocolHTTP constant instead of hardcoded string. Signed-off-by: Deluan <deluan@navidrome.org> * fix(ffmpeg): enhance bit_rate parsing logic for audio streams Signed-off-by: Deluan <deluan@navidrome.org> * fix(transcode): improve code review findings across transcode implementation - Fix parseProbeData to return nil on JSON unmarshal failure instead of a zero-valued struct, preventing silent degradation of source stream details - Use probe-resolved codec for lossless detection in buildSourceStream instead of the potentially stale scanner data - Remove MediaFile.IsLossless() (dead code) and consolidate lossless detection in isLosslessFormat(), using codec name only — bit depth is not reliable since lossy codecs like ADPCM report non-zero values - Add "wavpack" to lossless codec list (ffprobe codec_name for WavPack) - Guard bpsToKbps against negative input values - Fix misleading comment in buildTemplateArgs about conditional injection - Avoid leaking internal error details in Subsonic API responses - Add missing test for ErrNotFound branch in GetTranscodeDecision - Add TODO for hardcoded protocol in toResponseStreamDetails * refactor(transcode): streamline transcoding command lookup and format resolution Signed-off-by: Deluan <deluan@navidrome.org> * feat(transcode): implement server-side transcoding override for player formats Signed-off-by: Deluan <deluan@navidrome.org> * fix(transcode): honor bit depth and channel constraints in transcoding selection selectTranscodingOptions only checked sample rate when deciding whether same-format transcoding was needed, ignoring requested bit depth and channel reductions. This caused the streamer to return raw audio when the transcode decision requested downmix or bit-depth conversion. * refactor(transcode): unify streaming decision engine via MakeDecision Move transcoding decision-making out of mediaStreamer and into the subsonic Stream/Download handlers, using transcode.Decider.MakeDecision as the single decision engine. This eliminates selectTranscodingOptions and the mismatch between decision and streaming code paths (decision used LookupTranscodeCommand with built-in fallbacks, while streaming used FindByFormat which only checked the DB). - Add DecisionOptions with SkipProbe to MakeDecision so the legacy streaming path never calls ffprobe - Add buildLegacyClientInfo to translate legacy stream params (format, maxBitRate, DefaultDownsamplingFormat) into a synthetic ClientInfo - Add resolveStreamRequest on the subsonic Router to resolve legacy params into a fully specified StreamRequest via MakeDecision - Simplify DoStream to a dumb executor that receives pre-resolved params - Remove selectTranscodingOptions entirely Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcode): move MediaStreamer into core/transcode and unify StreamRequest Moved MediaStreamer, Stream, TranscodingCache and related types from core/media_streamer.go into core/transcode/, eliminating the duplicate StreamRequest type. The transcode.StreamRequest now carries all fields (ID, Format, BitRate, SampleRate, BitDepth, Channels, Offset) and ResolveStream returns a fully-populated value, removing manual field copying at every call site. Also moved buildLegacyClientInfo into the transcode package alongside ResolveStream, and unexported ParseTranscodeParams since it was only used internally by ValidateTranscodeParams. * refactor(transcode): rename Decider methods and unexport Params type Rename ResolveStream → ResolveRequest and ValidateTranscodeParams → ResolveRequestFromToken for clarity and consistency. The new ResolveRequestFromToken returns a StreamRequest directly (instead of the intermediate Params type), eliminating manual Params→StreamRequest conversion in callers. Unexport Params to params since it is now only used internally for JWT token parsing. * test(transcode): remove redundant tests and use constants Remove tests that duplicate coverage from integration-level tests (toClaimsMap, paramsFromToken round-trips, applyServerOverride direct call, duplicate 410 handler test). Replace raw "http" strings with ProtocolHTTP constant. Consolidate lossy -sample_fmt tests into DescribeTable. * refactor(transcode): split oversized files into focused modules Split transcode.go and transcode_test.go into focused files by concern: - decider.go: decision engine (MakeDecision, direct play/transcode evaluation, probe) - token.go: JWT token encode/decode (params, toClaimsMap, paramsFromToken, CreateTranscodeParams, ResolveRequestFromToken) - legacy_client.go: legacy Subsonic bridge (buildLegacyClientInfo, ResolveRequest) - codec_test.go: isLosslessFormat and normalizeProbeCodec tests - token_test.go: token round-trip and ResolveRequestFromToken tests Moved the Decider interface from types.go to decider.go to keep it near its implementation, and cleaned up types.go to contain only pure type definitions and constants. No public API changes. * refactor(transcode): reorder parameters in applyServerOverride function Signed-off-by: Deluan <deluan@navidrome.org> * test(e2e): add NewTestStream function and implement spyStreamer for testing Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan
|
3cd5d16b0a |
chore: upgrade golangci-lint to 2.11 and fix lint issues
Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan Quintão
|
f03ca44a8e
|
feat(plugins): add lyrics provider plugin capability (#5126)
* feat(plugins): add lyrics provider plugin capability Refactor the lyrics system from a static function to an interface-based service that supports WASM plugin providers. Plugins listed in the LyricsPriority config (alongside "embedded" and file extensions) are now resolved through the plugin system. Includes capability definition, Go/Rust PDK, adapter, Wire integration, and tests for plugin fallback behavior. * test(plugins): add lyrics capability integration test with test plugin * fix(plugins): default lyrics language to 'xxx' when plugin omits it Per the OpenSubsonic spec, the server must return 'und' or 'xxx' when the lyrics language is unknown. The lyrics plugin adapter was passing an empty string through when a plugin didn't provide a language value. This defaults the language to 'xxx', consistent with all other callers of model.ToLyrics() in the codebase. * refactor(plugins): rename lyrics import to improve clarity Signed-off-by: Deluan <deluan@navidrome.org> * refactor(lyrics): update TrackInfo description for clarity Signed-off-by: Deluan <deluan@navidrome.org> * fix(lyrics): enhance lyrics plugin handling and case sensitivity Signed-off-by: Deluan <deluan@navidrome.org> * fix(plugins): update payload type to string with byte format for task data Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan
|
435fb0b076 |
feat(server): add EnableCoverArtUpload config option
Allow administrators to disable playlist cover art upload/removal for non-admin users via the new EnableCoverArtUpload config option (default: true). - Guard uploadPlaylistImage and deletePlaylistImage endpoints (403 for non-admin when disabled) - Set CoverArtRole in Subsonic GetUser/GetUsers responses based on config and admin status - Pass config to frontend and conditionally hide upload/remove UI controls - Admins always retain upload capability regardless of setting |
||
|
Deluan
|
82f9f88c0f |
refactor(auth): replace untyped JWT claims with typed Claims struct
Introduced a typed Claims struct in core/auth to replace the raw map[string]any approach used for JWT claims throughout the codebase. This provides compile-time safety and better readability when creating, validating, and extracting JWT tokens. Also upgraded lestrrat-go/jwx from v2 to v3 and go-chi/jwtauth to v5.4.0, adapting all callers to the new API where token accessor methods now return tuples instead of bare values. Updated all affected handlers, middleware, and tests. Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan Quintão
|
5bc2bbb70e
|
feat(subsonic): append album version to names in Subsonic API (#5111)
* feat(subsonic): append album version to album names in Subsonic API responses Add AppendAlbumVersion config option (default: true) that appends the album version tag to album names in Subsonic API responses, similar to how AppendSubtitle works for track titles. This affects album names in childFromAlbum and buildAlbumID3 responses. Signed-off-by: Deluan <deluan@navidrome.org> * feat(subsonic): append album version to media file album names in Subsonic API Add FullAlbumName() to MediaFile that appends the album version tag, mirroring the Album.FullName() behavior. Use it in childFromMediaFile and fakePath to ensure media file responses also show the album version. Signed-off-by: Deluan <deluan@navidrome.org> * fix(subsonic): use len() check for album version tag to prevent panic on empty slice Use len(tags) > 0 instead of != nil to safely guard against empty slices when accessing the first element of the album version tag. Signed-off-by: Deluan <deluan@navidrome.org> * fix(subsonic): use FullName in buildAlbumDirectory and deduplicate FullName calls Apply album.FullName() in buildAlbumDirectory (getMusicDirectory) so album names are consistent across all Subsonic endpoints. Also compute al.FullName() once in childFromAlbum to avoid redundant calls. Signed-off-by: Deluan <deluan@navidrome.org> * fix: use len() check in MediaFile.FullTitle() to prevent panic on empty slice Apply the same safety improvement as FullAlbumName() and Album.FullName() for consistency. Signed-off-by: Deluan <deluan@navidrome.org> * test: add tests for Album.FullName, MediaFile.FullTitle, and MediaFile.FullAlbumName Cover all cases: config enabled/disabled, tag present, tag absent, and empty tag slice. Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan
|
0c3cc86535 |
fix(subsonic): restore public attribute for playlists in XML responses
This was causing issues with DSub and DSub2000 Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan Quintão
|
b59eb32961
|
feat(subsonic): sort search3 results by relevance (#5086)
* fix(subsonic): optimize search3 for high-cardinality FTS queries Use a two-phase query strategy for FTS5 searches to avoid the performance penalty of expensive LEFT JOINs (annotation, bookmark, library) on high-cardinality results like "the". Phase 1 runs a lightweight query (main table + FTS index only) to get sorted, paginated rowids. Phase 2 hydrates only those few rowids with the full JOINs, making them nearly free. For queries with complex ORDER BY expressions that reference joined tables (e.g. artist search sorted by play count), the optimization is skipped and the original single-query approach is used. * fix(search): update order by clauses to include 'rank' for FTS queries Signed-off-by: Deluan <deluan@navidrome.org> * fix(search): reintroduce 'rank' in Phase 2 ORDER BY for FTS queries Signed-off-by: Deluan <deluan@navidrome.org> * fix(search): remove 'rank' from ORDER BY in non-FTS queries and adjust two-phase query handling Signed-off-by: Deluan <deluan@navidrome.org> * fix(search): update FTS ranking to use bm25 weights and simplify ORDER BY qualification Signed-off-by: Deluan <deluan@navidrome.org> * fix(search): refine FTS query handling and improve comments for clarity Signed-off-by: Deluan <deluan@navidrome.org> * fix(search): refactor full-text search handling to streamline query strategy selection and improve LIKE fallback logic. Increase e2e coverage for search3 Signed-off-by: Deluan <deluan@navidrome.org> * refactor: enhance FTS column definitions and relevance weights Signed-off-by: Deluan <deluan@navidrome.org> * fix(search): refactor Search method signatures to remove offset and size parameters, streamline query handling Signed-off-by: Deluan <deluan@navidrome.org> * fix(search): allow single-character queries in search strategies and update related tests Signed-off-by: Deluan <deluan@navidrome.org> * fix(search): make FTS Phase 1 treat Max=0 as no limit, reorganize tests FTS Phase 1 unconditionally called Limit(uint64(options.Max)), which produced LIMIT 0 when Max was zero. This diverged from applyOptions where Max=0 means no limit. Now Phase 1 mirrors applyOptions: only add LIMIT/OFFSET when the value is positive. Also moved legacy backend integration tests from sql_search_fts_test.go to sql_search_like_test.go and added regression tests for the Max=0 behavior on both backends. * refactor: simplify callSearch function by removing variadic options and directly using QueryOptions Signed-off-by: Deluan <deluan@navidrome.org> * fix(search): implement ftsQueryDegraded function to detect significant content loss in FTS queries Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan Quintão
|
7ad2907719
|
refactor: move playlist business logic from repositories to service layer (#5027)
* refactor: move playlist business logic from repositories to core.Playlists service Move authorization, permission checks, and orchestration logic from playlist repositories to the core.Playlists service, following the existing pattern used by core.Share and core.Library. Changes: - Expand core.Playlists interface with read, mutation, track management, and REST adapter methods - Add playlistRepositoryWrapper for REST Save/Update/Delete with permission checks (follows Share/Library pattern) - Simplify persistence/playlist_repository.go: remove isWritable(), auth checks from Delete()/Put()/updatePlaylist() - Simplify persistence/playlist_track_repository.go: remove isTracksEditable() and permission checks from Add/Delete/Reorder - Update Subsonic API handlers to route through service - Update Native API handlers to accept core.Playlists instead of model.DataStore * test: add coverage for playlist service methods and REST wrapper Add 30 new tests covering the service methods added during the playlist refactoring: - Delete: owner, admin, denied, not found - Create: new playlist, replace tracks, admin bypass, denied, not found - AddTracks: owner, admin, denied, smart playlist, not found - RemoveTracks: owner, smart playlist denied, non-owner denied - ReorderTrack: owner, smart playlist denied - NewRepository wrapper: Save (owner assignment, ID clearing), Update (owner, admin, denied, ownership change, not found), Delete (delegation with permission checks) Expand mockedPlaylistRepo with Get, Delete, Tracks, GetWithTracks, and rest.Persistable methods. Add mockedPlaylistTrackRepo for track operation verification. * fix: add authorization check to playlist Update method Added ownership verification to the Subsonic Update endpoint in the playlist service layer. The authorization check was present in the old repository code but was not carried over during the refactoring to the service layer, allowing any authenticated user to modify playlists they don't own via the Subsonic API. Also added corresponding tests for the Update method's permission logic. * refactor: improve playlist permission checks and error handling, add e2e tests Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename core.Playlists to playlists package and update references Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename playlists_internal_test.go to parse_m3u_test.go and update tests; add new parse_nsp.go and rest_adapter.go files Signed-off-by: Deluan <deluan@navidrome.org> * fix: block track mutations on smart playlists in Create and Update Create now rejects replacing tracks on smart playlists (pre-existing gap). Update now uses checkTracksEditable instead of checkWritable when track changes are requested, restoring the protection that was removed from the repository layer during the refactoring. Metadata-only updates on smart playlists remain allowed. * test: add smart playlist protection tests to ensure readonly behavior and mutation restrictions * refactor: optimize track removal and renumbering in playlists Signed-off-by: Deluan <deluan@navidrome.org> * refactor: implement track reordering in playlists with SQL updates Signed-off-by: Deluan <deluan@navidrome.org> * refactor: wrap track deletion and reordering in transactions for consistency Signed-off-by: Deluan <deluan@navidrome.org> * refactor: remove unused getTracks method from playlistTrackRepository Signed-off-by: Deluan <deluan@navidrome.org> * refactor: optimize playlist track renumbering with CTE-based UPDATE Replace the DELETE + re-INSERT renumbering strategy with a two-step UPDATE approach using a materialized CTE and ROW_NUMBER() window function. The previous approach (SELECT all IDs, DELETE all tracks, re-INSERT in chunks of 200) required 13 SQL operations for a 2000-track playlist. The new approach uses just 2 UPDATEs: first negating all IDs to clear the positive space, then assigning sequential positions via UPDATE...FROM with a CTE. This avoids the UNIQUE constraint violations that affected the original correlated subquery while reducing per-delete request time from ~110ms to ~12ms on a 2000-track playlist. Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename New function to NewPlaylists for clarity Signed-off-by: Deluan <deluan@navidrome.org> * refactor: update mock playlist repository and tests for consistency Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan
|
5fa8356b31 |
chore(deps): bump golangci-lint to v2.10.0 and suppress new gosec false positives
Bump golangci-lint from v2.9.0 to v2.10.0, which includes a newer gosec with additional taint-analysis rules (G117, G703, G704, G705) and a stricter G101 check. Added inline //nolint:gosec comments to suppress 21 false positives across 19 files: struct fields flagged as secrets (G117), w.Write calls flagged as XSS (G705), HTTP client calls flagged as SSRF (G704), os.Stat/os.ReadFile/os.Remove flagged as path traversal (G703), and a sort mapping flagged as hardcoded credentials (G101). Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan
|
0a4722802a |
fix(subsonic): validate JSONP callback parameter
Added validation to ensure the JSONP callback parameter is a valid JavaScript identifier before reflecting it into the response. Invalid callbacks now return a JSON error response instead. This prevents malicious input from being injected into the response body via the callback parameter. |
||
|
Maximilian
|
a704e86ac1
|
refactor: run Go modernize (#5002) | ||
|
Kendall Garner
|
6fb4cd277e
|
feat(subsonic): add OS readonly and validUntil properties in playlists (#4993)
* feat(subsonic): add OS readonly and validUntil properties
* remove duplicated test
* test: fix and enable disabled child smart playlist tests
Fixed the XContext("child smart playlists") tests that were disabled with
a TODO comment. The tests had several issues: nested playlists were missing
Public: true (required by InPlaylist criteria), the criteria matched no
test fixtures, the "not expired" test set EvaluatedAt on the parent too
(preventing it from refreshing at all), and the "expired" test dereferenced
a nil EvaluatedAt. Added proper cleanup with DeferCleanup and config
restoration via configtest.
* fix(subsonic): always include readonly field in JSON playlist responses
Removed omitempty from the JSON tag of the Readonly field in
OpenSubsonicPlaylist so that readonly: false is always serialized in
JSON responses, per the OpenSubsonic spec requirement that supported
fields must be returned with default values. Added a test case with an
empty OpenSubsonicPlaylist to verify the behavior.
---------
Co-authored-by: Deluan Quintão <deluan@navidrome.org>
|
||
|
Deluan
|
4d4740b83b |
fix(subsonic): fix support for LegacyClients
Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan Quintão
|
772d1f359b
|
feat: add similar songs functionality in agents, and Instant Mix (song-based) to UI (#4919)
* refactor: rename ArtistRadio to SimilarSongs for clarity and consistency Signed-off-by: Deluan <deluan@navidrome.org> * feat: implement GetSimilarSongsByTrack and related functionality for song similarity retrieval Signed-off-by: Deluan <deluan@navidrome.org> * feat: enhance GetSimilarSongsByTrack to include artist and album details and update tests Signed-off-by: Deluan <deluan@navidrome.org> * feat: enhance song matching by implementing title and artist filtering in loadTracksByTitleAndArtist Signed-off-by: Deluan <deluan@navidrome.org> * test: add unit tests for song matching functionality in provider Signed-off-by: Deluan <deluan@navidrome.org> * refactor: extract song matching functionality into its own file Signed-off-by: Deluan <deluan@navidrome.org> * docs: clarify similarSongsFallback function description in provider.go Signed-off-by: Deluan <deluan@navidrome.org> * refactor: initialize result slice for songs with capacity based on response length Signed-off-by: Deluan <deluan@navidrome.org> * refactor: simplify agent method calls for retrieving images and similar songs Signed-off-by: Deluan <deluan@navidrome.org> * refactor: simplify agent method calls for retrieving images and similar songs Signed-off-by: Deluan <deluan@navidrome.org> * refactor: remove outdated comments in GetSimilarSongs methods Signed-off-by: Deluan <deluan@navidrome.org> * fix: use composite key for song matches to handle duplicates by title and artist Signed-off-by: Deluan <deluan@navidrome.org> * refactor: consolidate expectations setup for similar songs tests Signed-off-by: Deluan <deluan@navidrome.org> * feat: add instant mix action to song context menu and update translations Signed-off-by: Deluan <deluan@navidrome.org> * fix(provider): handle unknown entity types in GetSimilarSongs Signed-off-by: Deluan <deluan@navidrome.org> * refactor: move playSimilar action to playbackActions and streamline song processing Signed-off-by: Deluan <deluan@navidrome.org> * format Signed-off-by: Deluan <deluan@navidrome.org> * feat: enhance instant mix functionality with loading notification and shuffle option Signed-off-by: Deluan <deluan@navidrome.org> * feat: implement fuzzy matching for similar songs based on configurable threshold Signed-off-by: Deluan <deluan@navidrome.org> * refactor: implement track matching with multiple specificity levels Signed-off-by: Deluan <deluan@navidrome.org> * refactor: enhance track matching by implementing unified scoring with specificity levels Signed-off-by: Deluan <deluan@navidrome.org> * feat: enhance deezer top tracks result with album Signed-off-by: Deluan <deluan@navidrome.org> * feat: enhance track matching with fuzzy album similarity for improved scoring Signed-off-by: Deluan <deluan@navidrome.org> * docs: document multi-phase song matching algorithm with detailed scoring and prioritization Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan Quintão
|
f1e75c40dc
|
feat(plugins): add JSONForms-based plugin configuration UI (#4911)
* feat(plugins): add JSONForms schema for plugin configuration Signed-off-by: Deluan <deluan@navidrome.org> * feat: enhance error handling by formatting validation errors with field names Signed-off-by: Deluan <deluan@navidrome.org> * feat: enforce required fields in config validation and improve error handling Signed-off-by: Deluan <deluan@navidrome.org> * format JS code Signed-off-by: Deluan <deluan@navidrome.org> * feat: add config schema validation and enhance manifest structure Signed-off-by: Deluan <deluan@navidrome.org> * feat: refactor plugin config parsing and add unit tests Signed-off-by: Deluan <deluan@navidrome.org> * feat: add config validation error message in Portuguese * feat: enhance AlwaysExpandedArrayLayout with description support and improve array control testing Signed-off-by: Deluan <deluan@navidrome.org> * feat: update Discord Rust plugin configuration to use JSONForm for user tokens and enhance schema validation Signed-off-by: Deluan <deluan@navidrome.org> * fix: resolve React Hooks linting issues in plugin UI components * Apply suggestions from code review Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * format code Signed-off-by: Deluan <deluan@navidrome.org> * feat: migrate schema validation to use santhosh-tekuri/jsonschema and improve error formatting Signed-off-by: Deluan <deluan@navidrome.org> * address PR comments Signed-off-by: Deluan <deluan@navidrome.org> * fix flaky test Signed-off-by: Deluan <deluan@navidrome.org> * feat: enhance array layout and configuration handling with AJV defaults Signed-off-by: Deluan <deluan@navidrome.org> * feat: implement custom tester to exclude enum arrays from AlwaysExpandedArrayLayout Signed-off-by: Deluan <deluan@navidrome.org> * feat: add error boundary for schema rendering and improve error messages Signed-off-by: Deluan <deluan@navidrome.org> * feat: refine non-enum array control logic by utilizing JSONForms schema resolution Signed-off-by: Deluan <deluan@navidrome.org> * feat: add error styling to ToggleEnabledSwitch for disabled state Signed-off-by: Deluan <deluan@navidrome.org> * feat: adjust label positioning and styling in SchemaConfigEditor for improved layout Signed-off-by: Deluan <deluan@navidrome.org> * feat: implement outlined input controls renderers to replace custom fragile CSS Signed-off-by: Deluan <deluan@navidrome.org> * feat: remove margin from last form control inside array items for better spacing Signed-off-by: Deluan <deluan@navidrome.org> * feat: enhance AJV error handling to transform required errors for field-level validation Signed-off-by: Deluan <deluan@navidrome.org> * feat: set default value for User Tokens in manifest.json to improve user experience Signed-off-by: Deluan <deluan@navidrome.org> * format Signed-off-by: Deluan <deluan@navidrome.org> * feat: add margin to outlined input controls for improved spacing Signed-off-by: Deluan <deluan@navidrome.org> * feat: remove redundant margin rule for last form control in array items Signed-off-by: Deluan <deluan@navidrome.org> * feat: adjust font size of label elements in SchemaConfigEditor for improved readability Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> |