quintodrome/core/playlists/rest_adapter.go

Ignoring revisions in .git-blame-ignore-revs. Click here to bypass and see the normal blame view.

201 lines
6.7 KiB
Go
Raw Normal View History

refactor: move playlist business logic from repositories to service layer (#5027) * refactor: move playlist business logic from repositories to core.Playlists service Move authorization, permission checks, and orchestration logic from playlist repositories to the core.Playlists service, following the existing pattern used by core.Share and core.Library. Changes: - Expand core.Playlists interface with read, mutation, track management, and REST adapter methods - Add playlistRepositoryWrapper for REST Save/Update/Delete with permission checks (follows Share/Library pattern) - Simplify persistence/playlist_repository.go: remove isWritable(), auth checks from Delete()/Put()/updatePlaylist() - Simplify persistence/playlist_track_repository.go: remove isTracksEditable() and permission checks from Add/Delete/Reorder - Update Subsonic API handlers to route through service - Update Native API handlers to accept core.Playlists instead of model.DataStore * test: add coverage for playlist service methods and REST wrapper Add 30 new tests covering the service methods added during the playlist refactoring: - Delete: owner, admin, denied, not found - Create: new playlist, replace tracks, admin bypass, denied, not found - AddTracks: owner, admin, denied, smart playlist, not found - RemoveTracks: owner, smart playlist denied, non-owner denied - ReorderTrack: owner, smart playlist denied - NewRepository wrapper: Save (owner assignment, ID clearing), Update (owner, admin, denied, ownership change, not found), Delete (delegation with permission checks) Expand mockedPlaylistRepo with Get, Delete, Tracks, GetWithTracks, and rest.Persistable methods. Add mockedPlaylistTrackRepo for track operation verification. * fix: add authorization check to playlist Update method Added ownership verification to the Subsonic Update endpoint in the playlist service layer. The authorization check was present in the old repository code but was not carried over during the refactoring to the service layer, allowing any authenticated user to modify playlists they don't own via the Subsonic API. Also added corresponding tests for the Update method's permission logic. * refactor: improve playlist permission checks and error handling, add e2e tests Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename core.Playlists to playlists package and update references Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename playlists_internal_test.go to parse_m3u_test.go and update tests; add new parse_nsp.go and rest_adapter.go files Signed-off-by: Deluan <deluan@navidrome.org> * fix: block track mutations on smart playlists in Create and Update Create now rejects replacing tracks on smart playlists (pre-existing gap). Update now uses checkTracksEditable instead of checkWritable when track changes are requested, restoring the protection that was removed from the repository layer during the refactoring. Metadata-only updates on smart playlists remain allowed. * test: add smart playlist protection tests to ensure readonly behavior and mutation restrictions * refactor: optimize track removal and renumbering in playlists Signed-off-by: Deluan <deluan@navidrome.org> * refactor: implement track reordering in playlists with SQL updates Signed-off-by: Deluan <deluan@navidrome.org> * refactor: wrap track deletion and reordering in transactions for consistency Signed-off-by: Deluan <deluan@navidrome.org> * refactor: remove unused getTracks method from playlistTrackRepository Signed-off-by: Deluan <deluan@navidrome.org> * refactor: optimize playlist track renumbering with CTE-based UPDATE Replace the DELETE + re-INSERT renumbering strategy with a two-step UPDATE approach using a materialized CTE and ROW_NUMBER() window function. The previous approach (SELECT all IDs, DELETE all tracks, re-INSERT in chunks of 200) required 13 SQL operations for a 2000-track playlist. The new approach uses just 2 UPDATEs: first negating all IDs to clear the positive space, then assigning sequential positions via UPDATE...FROM with a CTE. This avoids the UNIQUE constraint violations that affected the original correlated subquery while reducing per-delete request time from ~110ms to ~12ms on a 2000-track playlist. Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename New function to NewPlaylists for clarity Signed-off-by: Deluan <deluan@navidrome.org> * refactor: update mock playlist repository and tests for consistency Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>
2026-02-21 15:57:13 -09:00
package playlists
import (
"context"
"errors"
"reflect"
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.
2026-05-27 18:29:17 -08:00
"strings"
refactor: move playlist business logic from repositories to service layer (#5027) * refactor: move playlist business logic from repositories to core.Playlists service Move authorization, permission checks, and orchestration logic from playlist repositories to the core.Playlists service, following the existing pattern used by core.Share and core.Library. Changes: - Expand core.Playlists interface with read, mutation, track management, and REST adapter methods - Add playlistRepositoryWrapper for REST Save/Update/Delete with permission checks (follows Share/Library pattern) - Simplify persistence/playlist_repository.go: remove isWritable(), auth checks from Delete()/Put()/updatePlaylist() - Simplify persistence/playlist_track_repository.go: remove isTracksEditable() and permission checks from Add/Delete/Reorder - Update Subsonic API handlers to route through service - Update Native API handlers to accept core.Playlists instead of model.DataStore * test: add coverage for playlist service methods and REST wrapper Add 30 new tests covering the service methods added during the playlist refactoring: - Delete: owner, admin, denied, not found - Create: new playlist, replace tracks, admin bypass, denied, not found - AddTracks: owner, admin, denied, smart playlist, not found - RemoveTracks: owner, smart playlist denied, non-owner denied - ReorderTrack: owner, smart playlist denied - NewRepository wrapper: Save (owner assignment, ID clearing), Update (owner, admin, denied, ownership change, not found), Delete (delegation with permission checks) Expand mockedPlaylistRepo with Get, Delete, Tracks, GetWithTracks, and rest.Persistable methods. Add mockedPlaylistTrackRepo for track operation verification. * fix: add authorization check to playlist Update method Added ownership verification to the Subsonic Update endpoint in the playlist service layer. The authorization check was present in the old repository code but was not carried over during the refactoring to the service layer, allowing any authenticated user to modify playlists they don't own via the Subsonic API. Also added corresponding tests for the Update method's permission logic. * refactor: improve playlist permission checks and error handling, add e2e tests Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename core.Playlists to playlists package and update references Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename playlists_internal_test.go to parse_m3u_test.go and update tests; add new parse_nsp.go and rest_adapter.go files Signed-off-by: Deluan <deluan@navidrome.org> * fix: block track mutations on smart playlists in Create and Update Create now rejects replacing tracks on smart playlists (pre-existing gap). Update now uses checkTracksEditable instead of checkWritable when track changes are requested, restoring the protection that was removed from the repository layer during the refactoring. Metadata-only updates on smart playlists remain allowed. * test: add smart playlist protection tests to ensure readonly behavior and mutation restrictions * refactor: optimize track removal and renumbering in playlists Signed-off-by: Deluan <deluan@navidrome.org> * refactor: implement track reordering in playlists with SQL updates Signed-off-by: Deluan <deluan@navidrome.org> * refactor: wrap track deletion and reordering in transactions for consistency Signed-off-by: Deluan <deluan@navidrome.org> * refactor: remove unused getTracks method from playlistTrackRepository Signed-off-by: Deluan <deluan@navidrome.org> * refactor: optimize playlist track renumbering with CTE-based UPDATE Replace the DELETE + re-INSERT renumbering strategy with a two-step UPDATE approach using a materialized CTE and ROW_NUMBER() window function. The previous approach (SELECT all IDs, DELETE all tracks, re-INSERT in chunks of 200) required 13 SQL operations for a 2000-track playlist. The new approach uses just 2 UPDATEs: first negating all IDs to clear the positive space, then assigning sequential positions via UPDATE...FROM with a CTE. This avoids the UNIQUE constraint violations that affected the original correlated subquery while reducing per-delete request time from ~110ms to ~12ms on a 2000-track playlist. Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename New function to NewPlaylists for clarity Signed-off-by: Deluan <deluan@navidrome.org> * refactor: update mock playlist repository and tests for consistency Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>
2026-02-21 15:57:13 -09:00
"github.com/deluan/rest"
"github.com/navidrome/navidrome/model"
"github.com/navidrome/navidrome/model/criteria"
refactor: move playlist business logic from repositories to service layer (#5027) * refactor: move playlist business logic from repositories to core.Playlists service Move authorization, permission checks, and orchestration logic from playlist repositories to the core.Playlists service, following the existing pattern used by core.Share and core.Library. Changes: - Expand core.Playlists interface with read, mutation, track management, and REST adapter methods - Add playlistRepositoryWrapper for REST Save/Update/Delete with permission checks (follows Share/Library pattern) - Simplify persistence/playlist_repository.go: remove isWritable(), auth checks from Delete()/Put()/updatePlaylist() - Simplify persistence/playlist_track_repository.go: remove isTracksEditable() and permission checks from Add/Delete/Reorder - Update Subsonic API handlers to route through service - Update Native API handlers to accept core.Playlists instead of model.DataStore * test: add coverage for playlist service methods and REST wrapper Add 30 new tests covering the service methods added during the playlist refactoring: - Delete: owner, admin, denied, not found - Create: new playlist, replace tracks, admin bypass, denied, not found - AddTracks: owner, admin, denied, smart playlist, not found - RemoveTracks: owner, smart playlist denied, non-owner denied - ReorderTrack: owner, smart playlist denied - NewRepository wrapper: Save (owner assignment, ID clearing), Update (owner, admin, denied, ownership change, not found), Delete (delegation with permission checks) Expand mockedPlaylistRepo with Get, Delete, Tracks, GetWithTracks, and rest.Persistable methods. Add mockedPlaylistTrackRepo for track operation verification. * fix: add authorization check to playlist Update method Added ownership verification to the Subsonic Update endpoint in the playlist service layer. The authorization check was present in the old repository code but was not carried over during the refactoring to the service layer, allowing any authenticated user to modify playlists they don't own via the Subsonic API. Also added corresponding tests for the Update method's permission logic. * refactor: improve playlist permission checks and error handling, add e2e tests Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename core.Playlists to playlists package and update references Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename playlists_internal_test.go to parse_m3u_test.go and update tests; add new parse_nsp.go and rest_adapter.go files Signed-off-by: Deluan <deluan@navidrome.org> * fix: block track mutations on smart playlists in Create and Update Create now rejects replacing tracks on smart playlists (pre-existing gap). Update now uses checkTracksEditable instead of checkWritable when track changes are requested, restoring the protection that was removed from the repository layer during the refactoring. Metadata-only updates on smart playlists remain allowed. * test: add smart playlist protection tests to ensure readonly behavior and mutation restrictions * refactor: optimize track removal and renumbering in playlists Signed-off-by: Deluan <deluan@navidrome.org> * refactor: implement track reordering in playlists with SQL updates Signed-off-by: Deluan <deluan@navidrome.org> * refactor: wrap track deletion and reordering in transactions for consistency Signed-off-by: Deluan <deluan@navidrome.org> * refactor: remove unused getTracks method from playlistTrackRepository Signed-off-by: Deluan <deluan@navidrome.org> * refactor: optimize playlist track renumbering with CTE-based UPDATE Replace the DELETE + re-INSERT renumbering strategy with a two-step UPDATE approach using a materialized CTE and ROW_NUMBER() window function. The previous approach (SELECT all IDs, DELETE all tracks, re-INSERT in chunks of 200) required 13 SQL operations for a 2000-track playlist. The new approach uses just 2 UPDATEs: first negating all IDs to clear the positive space, then assigning sequential positions via UPDATE...FROM with a CTE. This avoids the UNIQUE constraint violations that affected the original correlated subquery while reducing per-delete request time from ~110ms to ~12ms on a 2000-track playlist. Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename New function to NewPlaylists for clarity Signed-off-by: Deluan <deluan@navidrome.org> * refactor: update mock playlist repository and tests for consistency Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>
2026-02-21 15:57:13 -09:00
"github.com/navidrome/navidrome/model/request"
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.
2026-05-27 18:29:17 -08:00
"github.com/navidrome/navidrome/utils/slice"
refactor: move playlist business logic from repositories to service layer (#5027) * refactor: move playlist business logic from repositories to core.Playlists service Move authorization, permission checks, and orchestration logic from playlist repositories to the core.Playlists service, following the existing pattern used by core.Share and core.Library. Changes: - Expand core.Playlists interface with read, mutation, track management, and REST adapter methods - Add playlistRepositoryWrapper for REST Save/Update/Delete with permission checks (follows Share/Library pattern) - Simplify persistence/playlist_repository.go: remove isWritable(), auth checks from Delete()/Put()/updatePlaylist() - Simplify persistence/playlist_track_repository.go: remove isTracksEditable() and permission checks from Add/Delete/Reorder - Update Subsonic API handlers to route through service - Update Native API handlers to accept core.Playlists instead of model.DataStore * test: add coverage for playlist service methods and REST wrapper Add 30 new tests covering the service methods added during the playlist refactoring: - Delete: owner, admin, denied, not found - Create: new playlist, replace tracks, admin bypass, denied, not found - AddTracks: owner, admin, denied, smart playlist, not found - RemoveTracks: owner, smart playlist denied, non-owner denied - ReorderTrack: owner, smart playlist denied - NewRepository wrapper: Save (owner assignment, ID clearing), Update (owner, admin, denied, ownership change, not found), Delete (delegation with permission checks) Expand mockedPlaylistRepo with Get, Delete, Tracks, GetWithTracks, and rest.Persistable methods. Add mockedPlaylistTrackRepo for track operation verification. * fix: add authorization check to playlist Update method Added ownership verification to the Subsonic Update endpoint in the playlist service layer. The authorization check was present in the old repository code but was not carried over during the refactoring to the service layer, allowing any authenticated user to modify playlists they don't own via the Subsonic API. Also added corresponding tests for the Update method's permission logic. * refactor: improve playlist permission checks and error handling, add e2e tests Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename core.Playlists to playlists package and update references Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename playlists_internal_test.go to parse_m3u_test.go and update tests; add new parse_nsp.go and rest_adapter.go files Signed-off-by: Deluan <deluan@navidrome.org> * fix: block track mutations on smart playlists in Create and Update Create now rejects replacing tracks on smart playlists (pre-existing gap). Update now uses checkTracksEditable instead of checkWritable when track changes are requested, restoring the protection that was removed from the repository layer during the refactoring. Metadata-only updates on smart playlists remain allowed. * test: add smart playlist protection tests to ensure readonly behavior and mutation restrictions * refactor: optimize track removal and renumbering in playlists Signed-off-by: Deluan <deluan@navidrome.org> * refactor: implement track reordering in playlists with SQL updates Signed-off-by: Deluan <deluan@navidrome.org> * refactor: wrap track deletion and reordering in transactions for consistency Signed-off-by: Deluan <deluan@navidrome.org> * refactor: remove unused getTracks method from playlistTrackRepository Signed-off-by: Deluan <deluan@navidrome.org> * refactor: optimize playlist track renumbering with CTE-based UPDATE Replace the DELETE + re-INSERT renumbering strategy with a two-step UPDATE approach using a materialized CTE and ROW_NUMBER() window function. The previous approach (SELECT all IDs, DELETE all tracks, re-INSERT in chunks of 200) required 13 SQL operations for a 2000-track playlist. The new approach uses just 2 UPDATEs: first negating all IDs to clear the positive space, then assigning sequential positions via UPDATE...FROM with a CTE. This avoids the UNIQUE constraint violations that affected the original correlated subquery while reducing per-delete request time from ~110ms to ~12ms on a 2000-track playlist. Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename New function to NewPlaylists for clarity Signed-off-by: Deluan <deluan@navidrome.org> * refactor: update mock playlist repository and tests for consistency Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>
2026-02-21 15:57:13 -09:00
)
// --- REST adapter (follows Share/Library pattern) ---
func (s *playlists) NewRepository(ctx context.Context) rest.Repository {
return &playlistRepositoryWrapper{
ctx: ctx,
PlaylistRepository: s.ds.Playlist(ctx),
service: s,
}
}
// playlistRepositoryWrapper wraps the playlist repository as a thin REST-to-service adapter.
// It satisfies rest.Repository through the embedded PlaylistRepository (via ResourceRepository),
// and rest.Persistable by delegating to service methods for all mutations.
type playlistRepositoryWrapper struct {
model.PlaylistRepository
ctx context.Context
service *playlists
}
func (r *playlistRepositoryWrapper) Save(entity any) (string, error) {
return r.service.savePlaylist(r.ctx, entity.(*model.Playlist))
}
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.
2026-05-27 18:29:17 -08:00
func (r *playlistRepositoryWrapper) Update(id string, entity any, cols ...string) error {
return r.service.updatePlaylistEntity(r.ctx, id, entity.(*model.Playlist), cols...)
refactor: move playlist business logic from repositories to service layer (#5027) * refactor: move playlist business logic from repositories to core.Playlists service Move authorization, permission checks, and orchestration logic from playlist repositories to the core.Playlists service, following the existing pattern used by core.Share and core.Library. Changes: - Expand core.Playlists interface with read, mutation, track management, and REST adapter methods - Add playlistRepositoryWrapper for REST Save/Update/Delete with permission checks (follows Share/Library pattern) - Simplify persistence/playlist_repository.go: remove isWritable(), auth checks from Delete()/Put()/updatePlaylist() - Simplify persistence/playlist_track_repository.go: remove isTracksEditable() and permission checks from Add/Delete/Reorder - Update Subsonic API handlers to route through service - Update Native API handlers to accept core.Playlists instead of model.DataStore * test: add coverage for playlist service methods and REST wrapper Add 30 new tests covering the service methods added during the playlist refactoring: - Delete: owner, admin, denied, not found - Create: new playlist, replace tracks, admin bypass, denied, not found - AddTracks: owner, admin, denied, smart playlist, not found - RemoveTracks: owner, smart playlist denied, non-owner denied - ReorderTrack: owner, smart playlist denied - NewRepository wrapper: Save (owner assignment, ID clearing), Update (owner, admin, denied, ownership change, not found), Delete (delegation with permission checks) Expand mockedPlaylistRepo with Get, Delete, Tracks, GetWithTracks, and rest.Persistable methods. Add mockedPlaylistTrackRepo for track operation verification. * fix: add authorization check to playlist Update method Added ownership verification to the Subsonic Update endpoint in the playlist service layer. The authorization check was present in the old repository code but was not carried over during the refactoring to the service layer, allowing any authenticated user to modify playlists they don't own via the Subsonic API. Also added corresponding tests for the Update method's permission logic. * refactor: improve playlist permission checks and error handling, add e2e tests Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename core.Playlists to playlists package and update references Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename playlists_internal_test.go to parse_m3u_test.go and update tests; add new parse_nsp.go and rest_adapter.go files Signed-off-by: Deluan <deluan@navidrome.org> * fix: block track mutations on smart playlists in Create and Update Create now rejects replacing tracks on smart playlists (pre-existing gap). Update now uses checkTracksEditable instead of checkWritable when track changes are requested, restoring the protection that was removed from the repository layer during the refactoring. Metadata-only updates on smart playlists remain allowed. * test: add smart playlist protection tests to ensure readonly behavior and mutation restrictions * refactor: optimize track removal and renumbering in playlists Signed-off-by: Deluan <deluan@navidrome.org> * refactor: implement track reordering in playlists with SQL updates Signed-off-by: Deluan <deluan@navidrome.org> * refactor: wrap track deletion and reordering in transactions for consistency Signed-off-by: Deluan <deluan@navidrome.org> * refactor: remove unused getTracks method from playlistTrackRepository Signed-off-by: Deluan <deluan@navidrome.org> * refactor: optimize playlist track renumbering with CTE-based UPDATE Replace the DELETE + re-INSERT renumbering strategy with a two-step UPDATE approach using a materialized CTE and ROW_NUMBER() window function. The previous approach (SELECT all IDs, DELETE all tracks, re-INSERT in chunks of 200) required 13 SQL operations for a 2000-track playlist. The new approach uses just 2 UPDATEs: first negating all IDs to clear the positive space, then assigning sequential positions via UPDATE...FROM with a CTE. This avoids the UNIQUE constraint violations that affected the original correlated subquery while reducing per-delete request time from ~110ms to ~12ms on a 2000-track playlist. Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename New function to NewPlaylists for clarity Signed-off-by: Deluan <deluan@navidrome.org> * refactor: update mock playlist repository and tests for consistency Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>
2026-02-21 15:57:13 -09:00
}
func (r *playlistRepositoryWrapper) Delete(id string) error {
err := r.service.Delete(r.ctx, id)
switch {
case errors.Is(err, model.ErrNotFound):
return rest.ErrNotFound
case errors.Is(err, model.ErrNotAuthorized):
return rest.ErrPermissionDenied
default:
return err
}
}
func (s *playlists) TracksRepository(ctx context.Context, playlistId string, refreshSmartPlaylist bool) rest.Repository {
repo := s.ds.Playlist(ctx)
tracks := repo.Tracks(playlistId, refreshSmartPlaylist)
if tracks == nil {
return nil
}
return tracks.(rest.Repository)
}
// savePlaylist creates a new playlist, assigning the owner from context.
// Only Name, Comment, Public, and Rules are user-settable via the REST API.
refactor: move playlist business logic from repositories to service layer (#5027) * refactor: move playlist business logic from repositories to core.Playlists service Move authorization, permission checks, and orchestration logic from playlist repositories to the core.Playlists service, following the existing pattern used by core.Share and core.Library. Changes: - Expand core.Playlists interface with read, mutation, track management, and REST adapter methods - Add playlistRepositoryWrapper for REST Save/Update/Delete with permission checks (follows Share/Library pattern) - Simplify persistence/playlist_repository.go: remove isWritable(), auth checks from Delete()/Put()/updatePlaylist() - Simplify persistence/playlist_track_repository.go: remove isTracksEditable() and permission checks from Add/Delete/Reorder - Update Subsonic API handlers to route through service - Update Native API handlers to accept core.Playlists instead of model.DataStore * test: add coverage for playlist service methods and REST wrapper Add 30 new tests covering the service methods added during the playlist refactoring: - Delete: owner, admin, denied, not found - Create: new playlist, replace tracks, admin bypass, denied, not found - AddTracks: owner, admin, denied, smart playlist, not found - RemoveTracks: owner, smart playlist denied, non-owner denied - ReorderTrack: owner, smart playlist denied - NewRepository wrapper: Save (owner assignment, ID clearing), Update (owner, admin, denied, ownership change, not found), Delete (delegation with permission checks) Expand mockedPlaylistRepo with Get, Delete, Tracks, GetWithTracks, and rest.Persistable methods. Add mockedPlaylistTrackRepo for track operation verification. * fix: add authorization check to playlist Update method Added ownership verification to the Subsonic Update endpoint in the playlist service layer. The authorization check was present in the old repository code but was not carried over during the refactoring to the service layer, allowing any authenticated user to modify playlists they don't own via the Subsonic API. Also added corresponding tests for the Update method's permission logic. * refactor: improve playlist permission checks and error handling, add e2e tests Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename core.Playlists to playlists package and update references Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename playlists_internal_test.go to parse_m3u_test.go and update tests; add new parse_nsp.go and rest_adapter.go files Signed-off-by: Deluan <deluan@navidrome.org> * fix: block track mutations on smart playlists in Create and Update Create now rejects replacing tracks on smart playlists (pre-existing gap). Update now uses checkTracksEditable instead of checkWritable when track changes are requested, restoring the protection that was removed from the repository layer during the refactoring. Metadata-only updates on smart playlists remain allowed. * test: add smart playlist protection tests to ensure readonly behavior and mutation restrictions * refactor: optimize track removal and renumbering in playlists Signed-off-by: Deluan <deluan@navidrome.org> * refactor: implement track reordering in playlists with SQL updates Signed-off-by: Deluan <deluan@navidrome.org> * refactor: wrap track deletion and reordering in transactions for consistency Signed-off-by: Deluan <deluan@navidrome.org> * refactor: remove unused getTracks method from playlistTrackRepository Signed-off-by: Deluan <deluan@navidrome.org> * refactor: optimize playlist track renumbering with CTE-based UPDATE Replace the DELETE + re-INSERT renumbering strategy with a two-step UPDATE approach using a materialized CTE and ROW_NUMBER() window function. The previous approach (SELECT all IDs, DELETE all tracks, re-INSERT in chunks of 200) required 13 SQL operations for a 2000-track playlist. The new approach uses just 2 UPDATEs: first negating all IDs to clear the positive space, then assigning sequential positions via UPDATE...FROM with a CTE. This avoids the UNIQUE constraint violations that affected the original correlated subquery while reducing per-delete request time from ~110ms to ~12ms on a 2000-track playlist. Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename New function to NewPlaylists for clarity Signed-off-by: Deluan <deluan@navidrome.org> * refactor: update mock playlist repository and tests for consistency Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>
2026-02-21 15:57:13 -09:00
func (s *playlists) savePlaylist(ctx context.Context, pls *model.Playlist) (string, error) {
usr, _ := request.UserFrom(ctx)
pls.OwnerID = usr.ID
pls.ID = "" // Force new creation
pls.Path = "" // Server-managed (M3U file path)
pls.Sync = false // Server-managed (M3U sync flag)
pls.UploadedImage = "" // Managed by image upload endpoint
pls.ExternalImageURL = "" // Managed by M3U import / plugins only
pls.EvaluatedAt = nil // Server-managed
refactor: move playlist business logic from repositories to service layer (#5027) * refactor: move playlist business logic from repositories to core.Playlists service Move authorization, permission checks, and orchestration logic from playlist repositories to the core.Playlists service, following the existing pattern used by core.Share and core.Library. Changes: - Expand core.Playlists interface with read, mutation, track management, and REST adapter methods - Add playlistRepositoryWrapper for REST Save/Update/Delete with permission checks (follows Share/Library pattern) - Simplify persistence/playlist_repository.go: remove isWritable(), auth checks from Delete()/Put()/updatePlaylist() - Simplify persistence/playlist_track_repository.go: remove isTracksEditable() and permission checks from Add/Delete/Reorder - Update Subsonic API handlers to route through service - Update Native API handlers to accept core.Playlists instead of model.DataStore * test: add coverage for playlist service methods and REST wrapper Add 30 new tests covering the service methods added during the playlist refactoring: - Delete: owner, admin, denied, not found - Create: new playlist, replace tracks, admin bypass, denied, not found - AddTracks: owner, admin, denied, smart playlist, not found - RemoveTracks: owner, smart playlist denied, non-owner denied - ReorderTrack: owner, smart playlist denied - NewRepository wrapper: Save (owner assignment, ID clearing), Update (owner, admin, denied, ownership change, not found), Delete (delegation with permission checks) Expand mockedPlaylistRepo with Get, Delete, Tracks, GetWithTracks, and rest.Persistable methods. Add mockedPlaylistTrackRepo for track operation verification. * fix: add authorization check to playlist Update method Added ownership verification to the Subsonic Update endpoint in the playlist service layer. The authorization check was present in the old repository code but was not carried over during the refactoring to the service layer, allowing any authenticated user to modify playlists they don't own via the Subsonic API. Also added corresponding tests for the Update method's permission logic. * refactor: improve playlist permission checks and error handling, add e2e tests Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename core.Playlists to playlists package and update references Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename playlists_internal_test.go to parse_m3u_test.go and update tests; add new parse_nsp.go and rest_adapter.go files Signed-off-by: Deluan <deluan@navidrome.org> * fix: block track mutations on smart playlists in Create and Update Create now rejects replacing tracks on smart playlists (pre-existing gap). Update now uses checkTracksEditable instead of checkWritable when track changes are requested, restoring the protection that was removed from the repository layer during the refactoring. Metadata-only updates on smart playlists remain allowed. * test: add smart playlist protection tests to ensure readonly behavior and mutation restrictions * refactor: optimize track removal and renumbering in playlists Signed-off-by: Deluan <deluan@navidrome.org> * refactor: implement track reordering in playlists with SQL updates Signed-off-by: Deluan <deluan@navidrome.org> * refactor: wrap track deletion and reordering in transactions for consistency Signed-off-by: Deluan <deluan@navidrome.org> * refactor: remove unused getTracks method from playlistTrackRepository Signed-off-by: Deluan <deluan@navidrome.org> * refactor: optimize playlist track renumbering with CTE-based UPDATE Replace the DELETE + re-INSERT renumbering strategy with a two-step UPDATE approach using a materialized CTE and ROW_NUMBER() window function. The previous approach (SELECT all IDs, DELETE all tracks, re-INSERT in chunks of 200) required 13 SQL operations for a 2000-track playlist. The new approach uses just 2 UPDATEs: first negating all IDs to clear the positive space, then assigning sequential positions via UPDATE...FROM with a CTE. This avoids the UNIQUE constraint violations that affected the original correlated subquery while reducing per-delete request time from ~110ms to ~12ms on a 2000-track playlist. Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename New function to NewPlaylists for clarity Signed-off-by: Deluan <deluan@navidrome.org> * refactor: update mock playlist repository and tests for consistency Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>
2026-02-21 15:57:13 -09:00
err := s.ds.Playlist(ctx).Put(pls)
if err != nil {
return "", err
}
return pls.ID, nil
}
// updatePlaylistEntity updates playlist metadata with permission checks.
// Used by the REST API wrapper.
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.
2026-05-27 18:29:17 -08:00
//
// cols names the fields the client actually sent in the JSON body (extracted by
// rest.Put). When non-empty, fields outside cols are not considered changed and
// are left untouched — this prevents partial requests like bulk "Make Public"
// (body: {"public": true}) from wiping fields that just happen to be zero in
// the deserialized entity (see issue #5541). An empty cols means "treat the
// entity as a complete record" — preserved for callers that don't use the REST
// wrapper.
func (s *playlists) updatePlaylistEntity(ctx context.Context, id string, entity *model.Playlist, cols ...string) error {
refactor: move playlist business logic from repositories to service layer (#5027) * refactor: move playlist business logic from repositories to core.Playlists service Move authorization, permission checks, and orchestration logic from playlist repositories to the core.Playlists service, following the existing pattern used by core.Share and core.Library. Changes: - Expand core.Playlists interface with read, mutation, track management, and REST adapter methods - Add playlistRepositoryWrapper for REST Save/Update/Delete with permission checks (follows Share/Library pattern) - Simplify persistence/playlist_repository.go: remove isWritable(), auth checks from Delete()/Put()/updatePlaylist() - Simplify persistence/playlist_track_repository.go: remove isTracksEditable() and permission checks from Add/Delete/Reorder - Update Subsonic API handlers to route through service - Update Native API handlers to accept core.Playlists instead of model.DataStore * test: add coverage for playlist service methods and REST wrapper Add 30 new tests covering the service methods added during the playlist refactoring: - Delete: owner, admin, denied, not found - Create: new playlist, replace tracks, admin bypass, denied, not found - AddTracks: owner, admin, denied, smart playlist, not found - RemoveTracks: owner, smart playlist denied, non-owner denied - ReorderTrack: owner, smart playlist denied - NewRepository wrapper: Save (owner assignment, ID clearing), Update (owner, admin, denied, ownership change, not found), Delete (delegation with permission checks) Expand mockedPlaylistRepo with Get, Delete, Tracks, GetWithTracks, and rest.Persistable methods. Add mockedPlaylistTrackRepo for track operation verification. * fix: add authorization check to playlist Update method Added ownership verification to the Subsonic Update endpoint in the playlist service layer. The authorization check was present in the old repository code but was not carried over during the refactoring to the service layer, allowing any authenticated user to modify playlists they don't own via the Subsonic API. Also added corresponding tests for the Update method's permission logic. * refactor: improve playlist permission checks and error handling, add e2e tests Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename core.Playlists to playlists package and update references Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename playlists_internal_test.go to parse_m3u_test.go and update tests; add new parse_nsp.go and rest_adapter.go files Signed-off-by: Deluan <deluan@navidrome.org> * fix: block track mutations on smart playlists in Create and Update Create now rejects replacing tracks on smart playlists (pre-existing gap). Update now uses checkTracksEditable instead of checkWritable when track changes are requested, restoring the protection that was removed from the repository layer during the refactoring. Metadata-only updates on smart playlists remain allowed. * test: add smart playlist protection tests to ensure readonly behavior and mutation restrictions * refactor: optimize track removal and renumbering in playlists Signed-off-by: Deluan <deluan@navidrome.org> * refactor: implement track reordering in playlists with SQL updates Signed-off-by: Deluan <deluan@navidrome.org> * refactor: wrap track deletion and reordering in transactions for consistency Signed-off-by: Deluan <deluan@navidrome.org> * refactor: remove unused getTracks method from playlistTrackRepository Signed-off-by: Deluan <deluan@navidrome.org> * refactor: optimize playlist track renumbering with CTE-based UPDATE Replace the DELETE + re-INSERT renumbering strategy with a two-step UPDATE approach using a materialized CTE and ROW_NUMBER() window function. The previous approach (SELECT all IDs, DELETE all tracks, re-INSERT in chunks of 200) required 13 SQL operations for a 2000-track playlist. The new approach uses just 2 UPDATEs: first negating all IDs to clear the positive space, then assigning sequential positions via UPDATE...FROM with a CTE. This avoids the UNIQUE constraint violations that affected the original correlated subquery while reducing per-delete request time from ~110ms to ~12ms on a 2000-track playlist. Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename New function to NewPlaylists for clarity Signed-off-by: Deluan <deluan@navidrome.org> * refactor: update mock playlist repository and tests for consistency Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>
2026-02-21 15:57:13 -09:00
current, err := s.checkWritable(ctx, id)
if err != nil {
switch {
case errors.Is(err, model.ErrNotFound):
return rest.ErrNotFound
case errors.Is(err, model.ErrNotAuthorized):
return rest.ErrPermissionDenied
default:
return err
}
}
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.
2026-05-27 18:29:17 -08:00
sent := sentFields(cols)
refactor: move playlist business logic from repositories to service layer (#5027) * refactor: move playlist business logic from repositories to core.Playlists service Move authorization, permission checks, and orchestration logic from playlist repositories to the core.Playlists service, following the existing pattern used by core.Share and core.Library. Changes: - Expand core.Playlists interface with read, mutation, track management, and REST adapter methods - Add playlistRepositoryWrapper for REST Save/Update/Delete with permission checks (follows Share/Library pattern) - Simplify persistence/playlist_repository.go: remove isWritable(), auth checks from Delete()/Put()/updatePlaylist() - Simplify persistence/playlist_track_repository.go: remove isTracksEditable() and permission checks from Add/Delete/Reorder - Update Subsonic API handlers to route through service - Update Native API handlers to accept core.Playlists instead of model.DataStore * test: add coverage for playlist service methods and REST wrapper Add 30 new tests covering the service methods added during the playlist refactoring: - Delete: owner, admin, denied, not found - Create: new playlist, replace tracks, admin bypass, denied, not found - AddTracks: owner, admin, denied, smart playlist, not found - RemoveTracks: owner, smart playlist denied, non-owner denied - ReorderTrack: owner, smart playlist denied - NewRepository wrapper: Save (owner assignment, ID clearing), Update (owner, admin, denied, ownership change, not found), Delete (delegation with permission checks) Expand mockedPlaylistRepo with Get, Delete, Tracks, GetWithTracks, and rest.Persistable methods. Add mockedPlaylistTrackRepo for track operation verification. * fix: add authorization check to playlist Update method Added ownership verification to the Subsonic Update endpoint in the playlist service layer. The authorization check was present in the old repository code but was not carried over during the refactoring to the service layer, allowing any authenticated user to modify playlists they don't own via the Subsonic API. Also added corresponding tests for the Update method's permission logic. * refactor: improve playlist permission checks and error handling, add e2e tests Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename core.Playlists to playlists package and update references Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename playlists_internal_test.go to parse_m3u_test.go and update tests; add new parse_nsp.go and rest_adapter.go files Signed-off-by: Deluan <deluan@navidrome.org> * fix: block track mutations on smart playlists in Create and Update Create now rejects replacing tracks on smart playlists (pre-existing gap). Update now uses checkTracksEditable instead of checkWritable when track changes are requested, restoring the protection that was removed from the repository layer during the refactoring. Metadata-only updates on smart playlists remain allowed. * test: add smart playlist protection tests to ensure readonly behavior and mutation restrictions * refactor: optimize track removal and renumbering in playlists Signed-off-by: Deluan <deluan@navidrome.org> * refactor: implement track reordering in playlists with SQL updates Signed-off-by: Deluan <deluan@navidrome.org> * refactor: wrap track deletion and reordering in transactions for consistency Signed-off-by: Deluan <deluan@navidrome.org> * refactor: remove unused getTracks method from playlistTrackRepository Signed-off-by: Deluan <deluan@navidrome.org> * refactor: optimize playlist track renumbering with CTE-based UPDATE Replace the DELETE + re-INSERT renumbering strategy with a two-step UPDATE approach using a materialized CTE and ROW_NUMBER() window function. The previous approach (SELECT all IDs, DELETE all tracks, re-INSERT in chunks of 200) required 13 SQL operations for a 2000-track playlist. The new approach uses just 2 UPDATEs: first negating all IDs to clear the positive space, then assigning sequential positions via UPDATE...FROM with a CTE. This avoids the UNIQUE constraint violations that affected the original correlated subquery while reducing per-delete request time from ~110ms to ~12ms on a 2000-track playlist. Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename New function to NewPlaylists for clarity Signed-off-by: Deluan <deluan@navidrome.org> * refactor: update mock playlist repository and tests for consistency Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>
2026-02-21 15:57:13 -09:00
usr, _ := request.UserFrom(ctx)
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.
2026-05-27 18:29:17 -08:00
ownerChanged := sent("ownerId") && entity.OwnerID != "" && entity.OwnerID != current.OwnerID
if !usr.IsAdmin && ownerChanged {
refactor: move playlist business logic from repositories to service layer (#5027) * refactor: move playlist business logic from repositories to core.Playlists service Move authorization, permission checks, and orchestration logic from playlist repositories to the core.Playlists service, following the existing pattern used by core.Share and core.Library. Changes: - Expand core.Playlists interface with read, mutation, track management, and REST adapter methods - Add playlistRepositoryWrapper for REST Save/Update/Delete with permission checks (follows Share/Library pattern) - Simplify persistence/playlist_repository.go: remove isWritable(), auth checks from Delete()/Put()/updatePlaylist() - Simplify persistence/playlist_track_repository.go: remove isTracksEditable() and permission checks from Add/Delete/Reorder - Update Subsonic API handlers to route through service - Update Native API handlers to accept core.Playlists instead of model.DataStore * test: add coverage for playlist service methods and REST wrapper Add 30 new tests covering the service methods added during the playlist refactoring: - Delete: owner, admin, denied, not found - Create: new playlist, replace tracks, admin bypass, denied, not found - AddTracks: owner, admin, denied, smart playlist, not found - RemoveTracks: owner, smart playlist denied, non-owner denied - ReorderTrack: owner, smart playlist denied - NewRepository wrapper: Save (owner assignment, ID clearing), Update (owner, admin, denied, ownership change, not found), Delete (delegation with permission checks) Expand mockedPlaylistRepo with Get, Delete, Tracks, GetWithTracks, and rest.Persistable methods. Add mockedPlaylistTrackRepo for track operation verification. * fix: add authorization check to playlist Update method Added ownership verification to the Subsonic Update endpoint in the playlist service layer. The authorization check was present in the old repository code but was not carried over during the refactoring to the service layer, allowing any authenticated user to modify playlists they don't own via the Subsonic API. Also added corresponding tests for the Update method's permission logic. * refactor: improve playlist permission checks and error handling, add e2e tests Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename core.Playlists to playlists package and update references Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename playlists_internal_test.go to parse_m3u_test.go and update tests; add new parse_nsp.go and rest_adapter.go files Signed-off-by: Deluan <deluan@navidrome.org> * fix: block track mutations on smart playlists in Create and Update Create now rejects replacing tracks on smart playlists (pre-existing gap). Update now uses checkTracksEditable instead of checkWritable when track changes are requested, restoring the protection that was removed from the repository layer during the refactoring. Metadata-only updates on smart playlists remain allowed. * test: add smart playlist protection tests to ensure readonly behavior and mutation restrictions * refactor: optimize track removal and renumbering in playlists Signed-off-by: Deluan <deluan@navidrome.org> * refactor: implement track reordering in playlists with SQL updates Signed-off-by: Deluan <deluan@navidrome.org> * refactor: wrap track deletion and reordering in transactions for consistency Signed-off-by: Deluan <deluan@navidrome.org> * refactor: remove unused getTracks method from playlistTrackRepository Signed-off-by: Deluan <deluan@navidrome.org> * refactor: optimize playlist track renumbering with CTE-based UPDATE Replace the DELETE + re-INSERT renumbering strategy with a two-step UPDATE approach using a materialized CTE and ROW_NUMBER() window function. The previous approach (SELECT all IDs, DELETE all tracks, re-INSERT in chunks of 200) required 13 SQL operations for a 2000-track playlist. The new approach uses just 2 UPDATEs: first negating all IDs to clear the positive space, then assigning sequential positions via UPDATE...FROM with a CTE. This avoids the UNIQUE constraint violations that affected the original correlated subquery while reducing per-delete request time from ~110ms to ~12ms on a 2000-track playlist. Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename New function to NewPlaylists for clarity Signed-off-by: Deluan <deluan@navidrome.org> * refactor: update mock playlist repository and tests for consistency Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>
2026-02-21 15:57:13 -09:00
return rest.ErrPermissionDenied
}
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.
2026-05-27 18:29:17 -08:00
nameChanged := sent("name") && entity.Name != current.Name
commentChanged := sent("comment") && entity.Comment != current.Comment
rulesChanged := sent("rules") && !rulesEqual(current.Rules, entity.Rules)
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.
2026-05-27 18:29:17 -08:00
if nameChanged || commentChanged || ownerChanged || rulesChanged {
return s.applyContentUpdate(ctx, current, entity, sent,
nameChanged, commentChanged, ownerChanged, rulesChanged)
}
return s.applyFlagsOnly(ctx, current, entity, sent)
}
// applyContentUpdate handles updates that change at least one of name/comment/
// owner/rules. It goes through updateMetadata, which always bumps updatedAt
// (invalidating cached cover-art URLs). namePtr/commentPtr are nil when the
// field is absent from the request OR present-but-unchanged (so updateMetadata
// skips them); publicPtr is nil only when public is absent from the request
// (an idempotent public value is still forwarded).
func (s *playlists) applyContentUpdate(ctx context.Context, current, entity *model.Playlist,
sent func(string) bool, nameChanged, commentChanged, ownerChanged, rulesChanged bool,
) error {
if ownerChanged {
current.OwnerID = entity.OwnerID
}
if rulesChanged {
current.Rules = entity.Rules
}
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.
2026-05-27 18:29:17 -08:00
if sent("sync") && current.Path != "" && current.Sync != entity.Sync {
current.Sync = entity.Sync
}
var namePtr, commentPtr *string
var publicPtr *bool
if nameChanged {
namePtr = &entity.Name
}
if commentChanged {
commentPtr = &entity.Comment
}
if sent("public") {
publicPtr = &entity.Public
}
return s.updateMetadata(ctx, s.ds, current, namePtr, commentPtr, publicPtr)
}
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.
2026-05-27 18:29:17 -08:00
// applyFlagsOnly handles updates that only toggle sync/public — skips
// updatedAt so cover art URLs stay stable.
func (s *playlists) applyFlagsOnly(ctx context.Context, current, entity *model.Playlist,
sent func(string) bool,
) error {
var updateCols []string
if sent("sync") && current.Path != "" && current.Sync != entity.Sync {
current.Sync = entity.Sync
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.
2026-05-27 18:29:17 -08:00
updateCols = append(updateCols, "sync")
}
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.
2026-05-27 18:29:17 -08:00
if sent("public") && current.Public != entity.Public {
current.Public = entity.Public
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.
2026-05-27 18:29:17 -08:00
updateCols = append(updateCols, "public")
}
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.
2026-05-27 18:29:17 -08:00
if len(updateCols) == 0 {
return nil
}
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.
2026-05-27 18:29:17 -08:00
return s.ds.Playlist(ctx).Put(current, updateCols...)
}
// sentFields returns a predicate that reports whether a JSON field was present
// in the request body. Matching is case-insensitive to mirror Go's json
// decoder, which populates struct fields from case-variant keys like
// {"Name":"x"} or {"OWNERID":"y"}. An empty cols list means "treat the entity
// as a full record" — every field is considered sent.
func sentFields(cols []string) func(string) bool {
if len(cols) == 0 {
return func(string) bool { return true }
}
set := slice.ToMap(cols, func(c string) (string, struct{}) { return strings.ToLower(c), struct{}{} })
return func(field string) bool {
_, ok := set[strings.ToLower(field)]
return ok
}
}
func rulesEqual(a, b *criteria.Criteria) bool {
if a == b {
return true
}
if a == nil || b == nil {
return false
refactor: move playlist business logic from repositories to service layer (#5027) * refactor: move playlist business logic from repositories to core.Playlists service Move authorization, permission checks, and orchestration logic from playlist repositories to the core.Playlists service, following the existing pattern used by core.Share and core.Library. Changes: - Expand core.Playlists interface with read, mutation, track management, and REST adapter methods - Add playlistRepositoryWrapper for REST Save/Update/Delete with permission checks (follows Share/Library pattern) - Simplify persistence/playlist_repository.go: remove isWritable(), auth checks from Delete()/Put()/updatePlaylist() - Simplify persistence/playlist_track_repository.go: remove isTracksEditable() and permission checks from Add/Delete/Reorder - Update Subsonic API handlers to route through service - Update Native API handlers to accept core.Playlists instead of model.DataStore * test: add coverage for playlist service methods and REST wrapper Add 30 new tests covering the service methods added during the playlist refactoring: - Delete: owner, admin, denied, not found - Create: new playlist, replace tracks, admin bypass, denied, not found - AddTracks: owner, admin, denied, smart playlist, not found - RemoveTracks: owner, smart playlist denied, non-owner denied - ReorderTrack: owner, smart playlist denied - NewRepository wrapper: Save (owner assignment, ID clearing), Update (owner, admin, denied, ownership change, not found), Delete (delegation with permission checks) Expand mockedPlaylistRepo with Get, Delete, Tracks, GetWithTracks, and rest.Persistable methods. Add mockedPlaylistTrackRepo for track operation verification. * fix: add authorization check to playlist Update method Added ownership verification to the Subsonic Update endpoint in the playlist service layer. The authorization check was present in the old repository code but was not carried over during the refactoring to the service layer, allowing any authenticated user to modify playlists they don't own via the Subsonic API. Also added corresponding tests for the Update method's permission logic. * refactor: improve playlist permission checks and error handling, add e2e tests Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename core.Playlists to playlists package and update references Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename playlists_internal_test.go to parse_m3u_test.go and update tests; add new parse_nsp.go and rest_adapter.go files Signed-off-by: Deluan <deluan@navidrome.org> * fix: block track mutations on smart playlists in Create and Update Create now rejects replacing tracks on smart playlists (pre-existing gap). Update now uses checkTracksEditable instead of checkWritable when track changes are requested, restoring the protection that was removed from the repository layer during the refactoring. Metadata-only updates on smart playlists remain allowed. * test: add smart playlist protection tests to ensure readonly behavior and mutation restrictions * refactor: optimize track removal and renumbering in playlists Signed-off-by: Deluan <deluan@navidrome.org> * refactor: implement track reordering in playlists with SQL updates Signed-off-by: Deluan <deluan@navidrome.org> * refactor: wrap track deletion and reordering in transactions for consistency Signed-off-by: Deluan <deluan@navidrome.org> * refactor: remove unused getTracks method from playlistTrackRepository Signed-off-by: Deluan <deluan@navidrome.org> * refactor: optimize playlist track renumbering with CTE-based UPDATE Replace the DELETE + re-INSERT renumbering strategy with a two-step UPDATE approach using a materialized CTE and ROW_NUMBER() window function. The previous approach (SELECT all IDs, DELETE all tracks, re-INSERT in chunks of 200) required 13 SQL operations for a 2000-track playlist. The new approach uses just 2 UPDATEs: first negating all IDs to clear the positive space, then assigning sequential positions via UPDATE...FROM with a CTE. This avoids the UNIQUE constraint violations that affected the original correlated subquery while reducing per-delete request time from ~110ms to ~12ms on a 2000-track playlist. Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename New function to NewPlaylists for clarity Signed-off-by: Deluan <deluan@navidrome.org> * refactor: update mock playlist repository and tests for consistency Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>
2026-02-21 15:57:13 -09:00
}
return reflect.DeepEqual(a, b)
refactor: move playlist business logic from repositories to service layer (#5027) * refactor: move playlist business logic from repositories to core.Playlists service Move authorization, permission checks, and orchestration logic from playlist repositories to the core.Playlists service, following the existing pattern used by core.Share and core.Library. Changes: - Expand core.Playlists interface with read, mutation, track management, and REST adapter methods - Add playlistRepositoryWrapper for REST Save/Update/Delete with permission checks (follows Share/Library pattern) - Simplify persistence/playlist_repository.go: remove isWritable(), auth checks from Delete()/Put()/updatePlaylist() - Simplify persistence/playlist_track_repository.go: remove isTracksEditable() and permission checks from Add/Delete/Reorder - Update Subsonic API handlers to route through service - Update Native API handlers to accept core.Playlists instead of model.DataStore * test: add coverage for playlist service methods and REST wrapper Add 30 new tests covering the service methods added during the playlist refactoring: - Delete: owner, admin, denied, not found - Create: new playlist, replace tracks, admin bypass, denied, not found - AddTracks: owner, admin, denied, smart playlist, not found - RemoveTracks: owner, smart playlist denied, non-owner denied - ReorderTrack: owner, smart playlist denied - NewRepository wrapper: Save (owner assignment, ID clearing), Update (owner, admin, denied, ownership change, not found), Delete (delegation with permission checks) Expand mockedPlaylistRepo with Get, Delete, Tracks, GetWithTracks, and rest.Persistable methods. Add mockedPlaylistTrackRepo for track operation verification. * fix: add authorization check to playlist Update method Added ownership verification to the Subsonic Update endpoint in the playlist service layer. The authorization check was present in the old repository code but was not carried over during the refactoring to the service layer, allowing any authenticated user to modify playlists they don't own via the Subsonic API. Also added corresponding tests for the Update method's permission logic. * refactor: improve playlist permission checks and error handling, add e2e tests Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename core.Playlists to playlists package and update references Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename playlists_internal_test.go to parse_m3u_test.go and update tests; add new parse_nsp.go and rest_adapter.go files Signed-off-by: Deluan <deluan@navidrome.org> * fix: block track mutations on smart playlists in Create and Update Create now rejects replacing tracks on smart playlists (pre-existing gap). Update now uses checkTracksEditable instead of checkWritable when track changes are requested, restoring the protection that was removed from the repository layer during the refactoring. Metadata-only updates on smart playlists remain allowed. * test: add smart playlist protection tests to ensure readonly behavior and mutation restrictions * refactor: optimize track removal and renumbering in playlists Signed-off-by: Deluan <deluan@navidrome.org> * refactor: implement track reordering in playlists with SQL updates Signed-off-by: Deluan <deluan@navidrome.org> * refactor: wrap track deletion and reordering in transactions for consistency Signed-off-by: Deluan <deluan@navidrome.org> * refactor: remove unused getTracks method from playlistTrackRepository Signed-off-by: Deluan <deluan@navidrome.org> * refactor: optimize playlist track renumbering with CTE-based UPDATE Replace the DELETE + re-INSERT renumbering strategy with a two-step UPDATE approach using a materialized CTE and ROW_NUMBER() window function. The previous approach (SELECT all IDs, DELETE all tracks, re-INSERT in chunks of 200) required 13 SQL operations for a 2000-track playlist. The new approach uses just 2 UPDATEs: first negating all IDs to clear the positive space, then assigning sequential positions via UPDATE...FROM with a CTE. This avoids the UNIQUE constraint violations that affected the original correlated subquery while reducing per-delete request time from ~110ms to ~12ms on a 2000-track playlist. Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename New function to NewPlaylists for clarity Signed-off-by: Deluan <deluan@navidrome.org> * refactor: update mock playlist repository and tests for consistency Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>
2026-02-21 15:57:13 -09:00
}