mirror of
https://github.com/navidrome/navidrome.git
synced 2026-10-18 07:30:29 +00:00
9 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Deluan Quintão
|
b293b96256
|
refactor(persistence): stateless repositories with per-call context (#6149)
* refactor(persistence): adopt generic deluan/rest repository API Pin deluan/rest to the refactor branch. REST-facing repository methods take a context and return typed values. Drop DataStore.Resource and ResourceRepository; the native API names typed repositories directly through a per-request adapter that later commits remove. * refactor(persistence): base repository helpers take a context * refactor(persistence): LibraryRepository takes a context per call * refactor(persistence): PropertyRepository takes a context per call * refactor(persistence): UserPropsRepository takes a context per call * refactor(persistence): TranscodingRepository takes a context per call * refactor(persistence): ShareRepository takes a context per call * refactor(persistence): PlayerRepository takes a context per call * refactor(persistence): RadioRepository takes a context per call * refactor(persistence): PlayQueueRepository takes a context per call * refactor(persistence): Tag and Genre repositories take a context per call * refactor(persistence): PluginRepository takes a context per call * refactor(persistence): Scrobble repositories take a context per call * refactor(persistence): FolderRepository takes a context per call * refactor(persistence): Artwork repositories take a context per call * refactor(persistence): UserRepository takes a context per call * refactor(persistence): ArtistRepository takes a context per call ReadAll no longer rewrites the shared sort mappings for the role filter; it works on a per-call copy. * test(persistence): assert artist role sort sanitization in ReadAll * refactor(persistence): AlbumRepository takes a context per call * test(persistence): pass the test context to album repository helpers * refactor(persistence): MediaFileRepository takes a context per call * refactor(persistence): Playlist repositories take a context per call * refactor(persistence): build all repositories once per store * refactor(core): REST repository wrappers are built once * refactor(persistence): repositories are stateless Remove the context field from the base repository and the per-request REST adapter. Enable the containedctx linter so no repository can hold a request context again. * chore(lint): skip containedctx in test files * refactor: share simplifications from the stateless repositories sweep Add deleteOwnedAll on sqlRepository and use it in player/share Delete to remove the duplicated bulk-delete loop; have Share.Repository() return model.ShareRepository so subsonic sharing.go drops its repeated type assertions. * chore(core): assert REST wrappers implement Persistable * chore: reformat imports * perf(persistence): build repositories on first use Each transaction store used to construct all 21 repositories up front, paying for filter and sort mapping setup the block never touched. Fields are now sync.OnceValue thunks, so a store only builds what it uses. * fix(persistence): clean plugin references per deleted user A bulk user delete that fails on a later id had already removed the earlier rows but skipped their plugin cleanup. Cleanup now runs right after each successful delete. * fix(core): unload disabled plugins even when a user delete fails A bulk delete can fail on a later id after earlier users were removed and their plugins auto-disabled. The wrapper returned before unloading, leaving those plugins running until the next successful delete or a restart. * chore(deps): pin deluan/rest to v1.0.1 Replaces the pseudo-version of the refactor branch with the tagged release. REST error messages now name the bare type (Artist, not model.Artist). * test: use the spec context instead of context.Background() Replace the context.Background()/context.TODO() calls this branch added to tests with the spec's ctx, GinkgoT().Context(), or t/b.Context(), so repository calls are bound to the running spec's lifetime. * test: declare the spec context once per Describe Set ctx from GinkgoT().Context() first in each top-level BeforeEach and reuse it, building user contexts on top of it instead of repeating inline calls. |
||
|
Deluan Quintão
|
3f4b6a642c
|
fix(server): return 404 instead of 500 for missing native API resources (#6131)
* fix: return 404 instead of 500 for missing native API resources The deluan/rest controller only maps rest.ErrNotFound to 404, comparing with ==. Most repositories return model.ErrNotFound, which had the same message but was a different value, so requesting a missing playlist, album, artist, song, radio, player, transcoding or library returned 500. This also applied to other users' private playlists. Make model.ErrNotFound the same value as rest.ErrNotFound. This fixes every REST route at once, with no per-route wrapping. errors.Is checks against either error keep working, and nothing wraps model.ErrNotFound before it reaches the controller. Fixes #6130 * fix(radio): return not found when deleting a missing radio station radioRepository.Delete used the shared delete helper, which never reports a missing row because SQL DELETE on zero rows is not an error. Deleting an unknown id silently succeeded: DELETE /api/radio/{id} returned 200, and the Subsonic deleteInternetRadioStation endpoint returned ok. Delete now checks the affected row count and returns model.ErrNotFound when nothing was deleted. The native API returns 404, and deleteInternetRadioStation returns error 70 (data not found). This matches Subsonic 6.1.6, gonic (both verified live) and Ampache (verified in source). Airsonic-Advanced does not implement this endpoint. The shared delete helper is unchanged, as several callers rely on deletes of absent rows succeeding. * fix(ui): return 404 for missing files that are not missing or do not exist missingRepository.Read filtered media files by bare "id" and "missing" columns. The media file query joins the library table, so SQLite rejected the query as ambiguous and GET /api/missing/{id} returned 500. Read now loads the file with MediaFileRepository.Get, which qualifies the column, and returns not found when the file does not exist or is not marked missing. * fix: report missing rows on single-item deletes and adopt deluan/rest errors.Is Bump github.com/deluan/rest to the version whose controller matches errors with errors.Is and errors.As. model.ErrNotFound stays the same value as rest.ErrNotFound, so the many hand-written conversions from model.ErrNotFound to rest.ErrNotFound in repositories, core services and test mocks did nothing. Remove them, along with the duplicate rest.ErrNotFound check in the Subsonic error mapper. Mappings from model.ErrNotAuthorized stay, as those are different errors. User and transcoding deletes had the same silent success as radio: the shared delete helper never reports a missing row, so their not-found checks never fired and DELETE /api/user/{id} and /api/transcoding/{id} returned 200 for unknown ids. Add deleteByID, which returns model.ErrNotFound when no row matched, and use it for radio, user and transcoding. Also drop the dead sql.ErrNoRows branch from delete, since a DELETE never returns it. The Subsonic deleteUser endpoint is not implemented (501), so this does not change the Subsonic API. Plugin deletes keep the silent helper: there is no REST route for them, and the plugin manager only deletes rows it just read. * chore: drop ErrNotFound comment and its identity test The alias to rest.ErrNotFound is self-explanatory, and the identity test only restated the declaration. * refactor: alias model.ErrNotAuthorized to rest.ErrPermissionDenied Like ErrNotFound, make model.ErrNotAuthorized the same value as the rest library's error, so REST endpoints map it to 403 directly. This removes the ErrNotAuthorized to rest.ErrPermissionDenied mappings in the library and playlist REST adapters and the duplicate check in the Subsonic error mapper. Handlers that check model.ErrNotAuthorized now also recognize rest.ErrPermissionDenied returned by repositories, so writePlaylistError, the image upload handlers and the public share handler return 403 for it instead of their fallback status. The error message changes from "not authorized" to "permission denied". |
||
|
Deluan Quintão
|
2e03766a9d
|
fix(playlist): preserve smart playlist song count on re-import (#5907) (#5908)
* fix(playlist): preserve smart playlist counters on re-import (#5907) * perf(playlist): skip re-importing unchanged NSP files (#5907) * feat(playlist): also store content hash for M3U imports (unused for now) * fix(playlist): return stored record when skipping unchanged NSP import Skipping before copying the stored identity broke the ImportFile(sync=false) contract: callers received an ID-less playlist and the requested Sync change was silently dropped. * refactor(playlist): hash imports once at the caller; protect smart counters in Put Move content hashing out of both parsers into the code that owns the file (parsePlaylist and ImportFile), removing the NSP double-buffer and the duplicated hashing idiom. Put now drops song_count/duration/size for smart playlists (PostMapArgs), disarming the counter-zeroing trap for all callers. * fix(playlist): invalidate imported hash when rules are edited via API Without this, a rules edit through the REST API kept the stored file hash, so every scan skipped the unchanged file and never restored the file-backed rules while sync was on. * test(playlist): verify smart counters survive a re-import, end to end The existing Put test seeds the stored counters with a raw SQL update, so it pins the guard in PostMapArgs but not the pipeline around it. This test drives the counters through a real evaluation instead: it saves a smart playlist, reads it with GetWithTracks to populate song_count/duration/size, then saves the playlist the way the scanner rebuilds it after parsing the .nsp file, with the counters back at zero. Both routes fail without the guard, and the new one covers the exact sequence reported in #5907. Test taken from #5970, which diagnosed the same root cause independently. Co-authored-by: Junker der Provinz <133605895+junkerderprovinz@users.noreply.github.com> * test(playlist): build the service with artwork.NewUploader The artwork pipeline in #5847 replaced core.NewImageUploadService() with artwork.NewUploader(ds) and updated every call site it could see. The five call sites this branch adds were written against the old constructor, so the merge applied cleanly but left the package uncompilable. * fix(db): re-stamp the imported_hash migration after the master merge Master gained three migrations while this branch was open, the newest being 20260816180040. The original 20260808200333 stamp now sorts before them, so any database already upgraded past that point would skip this migration entirely and never get the imported_hash column. Same SQL, current timestamp. * refactor(playlist): hash imported playlists with xxh3 and the id encoding ImportedHash is a change detector, not a security boundary, so it does not need a cryptographic digest. xxh3 is already a direct dependency and is used the same way to fingerprint files in the artwork image store. Encoding the 128-bit digest with id.Encode stores it in the same 22-char base62 form as every other id in the schema, down from 64 hex chars. No migration is needed: the imported_hash column has not shipped in a release, so no database holds a value in the old format. * refactor(playlist): extract the imported-playlist fingerprint helper Both import paths encoded the hash inline, so how a playlist file is fingerprinted lived in two places. A third import path that encoded it differently would silently never match the stored value, turning the unchanged-file skip into a no-op. --------- Co-authored-by: Junker der Provinz <133605895+junkerderprovinz@users.noreply.github.com> |
||
|
Deluan Quintão
|
85132240e0
|
feat(smartplaylist): per-playlist refreshDelay for stable daily/weekly playlists (#5790)
* feat(utils): add ParseDuration/FormatDuration with day and week units * feat(criteria): add per-playlist refreshDelay to smart playlist rules * feat(smartplaylist): honor per-playlist refreshDelay in refresh gate * feat(subsonic): compute smart playlist validUntil from effective refresh delay * fix(playlists): reset smart playlist evaluation window when rules change via API * refactor(utils): flatten FormatDuration recursion, single-pass duration regex * fix(playlists): address PR review feedback - Reset EvaluatedAt to nil instead of zero-time on rules change and NSP re-import, so getPlaylist(s) never reports year-1 Changed/validUntil in the window between an edit and the next owner read - Parse negative d/w durations so they are rejected with the consistent "negative duration" error instead of "unknown unit" - Quote input in the negative-duration error, matching the parse error - Gate per-playlist RefreshDelay behind IsSmartPlaylist, matching its doc |
||
|
Deluan Quintão
|
74a5c0c6d1
|
fix(playlists): preserve unchanged fields on partial REST updates (#5542)
* fix(playlists): preserve unchanged fields on partial REST updates (#5541) The REST adapter for playlists was discarding the `cols` argument that rest.Put provides (the list of fields actually present in the JSON body). updatePlaylistEntity then compared the deserialized entity's zero-valued Name/Comment against the DB row, decided "content changed", and called updateMetadata with &entity.Name — overwriting the name with the empty string. This surfaced via the Playlists list view's bulk "Make Public" action, which sends N parallel `PUT /api/playlist/{id}` requests with body `{"public": true}`. Affected playlists ended up with their names wiped (UI showed "Loading..." indefinitely). The per-row Public toggle was unaffected because it spreads the full record into the payload. Honor the cols list: gate every field-change check and every pointer passed to updateMetadata by whether the field was actually in the request body. Empty cols falls back to the existing "treat as a full record" behavior so non-REST callers are unaffected. * test(playlists): cover rules-only PUT + case-variant owner-change guard Follow-ups from manual testing and code review of the prior commit: - Manual testing confirmed Feishin-style rules-only PUT works correctly on the fix; add ginkgo regression tests for rules-only update, name+ rules combined, idempotent rules PUT (no-op), and bulk Make-Public preserving rules on smart playlists. - Keep the non-admin owner-change permission check gated on the deserialized entity content (not on `sent("ownerId")`) so a case-variant JSON key like {"OwnerId":"x"} can't downgrade the 403 to a silent 200. Go's json decoder is case-insensitive on struct field matching but rest.Put's field-name extraction is case- sensitive; the entity-based guard catches both spellings. The apply-side gating on ownerChanged still prevents the actual mutation, so this was a behavioral (not security) regression, but worth fixing. Adds a regression test asserting the case-variant key still returns rest.ErrPermissionDenied. - Correct misleading doc on applyContentUpdate: the path does not rewrite the backing M3U file; it goes through updateMetadata which bumps updatedAt and invalidates cached cover-art URLs. * fix(playlists): match REST cols case-insensitively (PR #5542 review) Go's encoding/json populates struct fields from case-variant keys like {"Name":"x"} or {"OwnerId":"y"}, but rest.Put's getFieldNames extracts raw JSON keys verbatim. With case-sensitive matching, sentFields would ignore the field on the update side — a request with {"Name":"Renamed"} would parse into entity.Name but then sent("name") returns false and the rename silently no-ops. Normalize both sides to lowercase. The entity-based owner-permission guard added in the previous commit remains as belt-and-suspenders but is now redundant with this change. Also clarify the applyContentUpdate doc comment: namePtr/commentPtr are nil when the field is absent OR present-but-unchanged, while publicPtr only tracks presence (an idempotent public is still forwarded). * refactor(playlists): drop redundant entity-based owner-permission guard The case-insensitive sentFields predicate already prevents case-variant JSON keys like {"OwnerId":"x"} from bypassing the ownerChanged check, so the duplicated entity-content guard is no longer load-bearing. Strengthen the regression test into a DescribeTable covering canonical, PascalCase, all-upper, and all-lower spellings to lock in the case-insensitive contract. |
||
|
Deluan Quintão
|
e6680c904b
|
fix(playlists): allow toggling auto-import and avoid unnecessary artwork reloads (#5421)
* fix(playlists): allow toggling auto-import (sync) via REST API The updatePlaylistEntity handler was not applying the sync field from incoming requests, causing the auto-import toggle in the UI to have no effect. Apply the sync value for file-backed playlists only. * fix(playlists): enhance update logic for playlist metadata and sync toggle Signed-off-by: Deluan <deluan@navidrome.org> * fix(playlists): address code review feedback - Add pointer equality short-circuit in rulesEqual before reflect.DeepEqual - Guard against empty ID in Put's partial-update path - Only apply Sync when it actually differs from current value, preventing zero-value overwrites from partial payloads * fix(playlists): remove unused parameters from Update method Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan
|
55e10b9c77 |
fix(playlist): update smart playlist rules during metadata update
Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan
|
f102036dc6 |
fix(server): clear server-managed fields in savePlaylist to prevent injection via REST API
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> |