2026-02-15 22:36:58 -05:00
|
|
|
package nativeapi
|
|
|
|
|
|
|
|
|
|
import (
|
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 19:57:13 -05:00
|
|
|
"context"
|
2026-02-15 22:36:58 -05:00
|
|
|
"encoding/json"
|
2026-03-02 16:59:05 -05:00
|
|
|
"io"
|
2026-02-15 22:36:58 -05:00
|
|
|
"net/http"
|
|
|
|
|
"net/http/httptest"
|
|
|
|
|
"time"
|
|
|
|
|
|
|
|
|
|
"github.com/deluan/rest"
|
|
|
|
|
"github.com/navidrome/navidrome/conf"
|
|
|
|
|
"github.com/navidrome/navidrome/conf/configtest"
|
|
|
|
|
"github.com/navidrome/navidrome/consts"
|
|
|
|
|
"github.com/navidrome/navidrome/core/auth"
|
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 19:57:13 -05:00
|
|
|
"github.com/navidrome/navidrome/core/playlists"
|
2026-02-15 22:36:58 -05:00
|
|
|
"github.com/navidrome/navidrome/model"
|
2026-03-02 16:59:05 -05:00
|
|
|
"github.com/navidrome/navidrome/model/request"
|
2026-02-15 22:36:58 -05:00
|
|
|
"github.com/navidrome/navidrome/server"
|
|
|
|
|
"github.com/navidrome/navidrome/tests"
|
|
|
|
|
. "github.com/onsi/ginkgo/v2"
|
|
|
|
|
. "github.com/onsi/gomega"
|
|
|
|
|
)
|
|
|
|
|
|
2026-03-02 16:59:05 -05:00
|
|
|
var _ = Describe("Playlist Image Endpoints", func() {
|
|
|
|
|
BeforeEach(func() {
|
|
|
|
|
DeferCleanup(configtest.SetupConfig())
|
|
|
|
|
})
|
2026-02-15 22:36:58 -05:00
|
|
|
|
2026-03-02 16:59:05 -05:00
|
|
|
DescribeTable("uploadPlaylistImage guard",
|
2026-03-27 19:33:46 -04:00
|
|
|
func(enableArtworkUpload, isAdmin bool, expectedStatus int) {
|
|
|
|
|
conf.Server.EnableArtworkUpload = enableArtworkUpload
|
2026-03-02 16:59:05 -05:00
|
|
|
handler := uploadPlaylistImage(&mockPlaylistsService{})
|
|
|
|
|
|
|
|
|
|
req := httptest.NewRequest("POST", "/playlist/pls-1/image", nil)
|
|
|
|
|
ctx := request.WithUser(GinkgoT().Context(), model.User{ID: "user-1", IsAdmin: isAdmin})
|
|
|
|
|
req = req.WithContext(ctx)
|
|
|
|
|
|
|
|
|
|
w := httptest.NewRecorder()
|
|
|
|
|
handler.ServeHTTP(w, req)
|
|
|
|
|
Expect(w.Code).To(Equal(expectedStatus))
|
|
|
|
|
},
|
|
|
|
|
Entry("enabled, regular user passes guard", true, false, http.StatusBadRequest),
|
|
|
|
|
Entry("enabled, admin passes guard", true, true, http.StatusBadRequest),
|
|
|
|
|
Entry("disabled, admin passes guard", false, true, http.StatusBadRequest),
|
|
|
|
|
Entry("disabled, regular user is forbidden", false, false, http.StatusForbidden),
|
|
|
|
|
)
|
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 19:57:13 -05:00
|
|
|
|
2026-03-02 16:59:05 -05:00
|
|
|
DescribeTable("deletePlaylistImage guard",
|
2026-03-27 19:33:46 -04:00
|
|
|
func(enableArtworkUpload, isAdmin bool, expectedStatus int) {
|
|
|
|
|
conf.Server.EnableArtworkUpload = enableArtworkUpload
|
2026-03-02 16:59:05 -05:00
|
|
|
handler := deletePlaylistImage(&mockPlaylistsService{})
|
|
|
|
|
|
|
|
|
|
req := httptest.NewRequest("DELETE", "/playlist/pls-1/image", nil)
|
|
|
|
|
ctx := request.WithUser(GinkgoT().Context(), model.User{ID: "user-1", IsAdmin: isAdmin})
|
|
|
|
|
req = req.WithContext(ctx)
|
|
|
|
|
|
|
|
|
|
w := httptest.NewRecorder()
|
|
|
|
|
handler.ServeHTTP(w, req)
|
|
|
|
|
Expect(w.Code).To(Equal(expectedStatus))
|
|
|
|
|
},
|
|
|
|
|
Entry("enabled, regular user passes guard", true, false, http.StatusNotFound),
|
|
|
|
|
Entry("enabled, admin passes guard", true, true, http.StatusNotFound),
|
|
|
|
|
Entry("disabled, admin passes guard", false, true, http.StatusNotFound),
|
|
|
|
|
Entry("disabled, regular user is forbidden", false, false, http.StatusForbidden),
|
|
|
|
|
)
|
|
|
|
|
})
|
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 19:57:13 -05:00
|
|
|
|
2026-02-15 22:36:58 -05:00
|
|
|
var _ = Describe("Playlist Tracks Endpoint", func() {
|
|
|
|
|
var (
|
|
|
|
|
router http.Handler
|
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 19:57:13 -05:00
|
|
|
plsSvc *mockPlaylistsService
|
2026-02-15 22:36:58 -05:00
|
|
|
userRepo *tests.MockedUserRepo
|
|
|
|
|
w *httptest.ResponseRecorder
|
|
|
|
|
)
|
|
|
|
|
|
|
|
|
|
BeforeEach(func() {
|
|
|
|
|
DeferCleanup(configtest.SetupConfig())
|
2026-07-04 19:55:34 -04:00
|
|
|
conf.Server.EnableSharing = false
|
2026-02-15 22:36:58 -05:00
|
|
|
conf.Server.SessionTimeout = time.Minute
|
|
|
|
|
|
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 19:57:13 -05:00
|
|
|
plsSvc = &mockPlaylistsService{}
|
2026-02-15 22:36:58 -05:00
|
|
|
userRepo = tests.CreateMockUserRepo()
|
|
|
|
|
|
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 19:57:13 -05:00
|
|
|
ds := &tests.MockDataStore{
|
2026-02-15 22:36:58 -05:00
|
|
|
MockedUser: userRepo,
|
|
|
|
|
MockedProperty: &tests.MockedPropertyRepo{},
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
auth.Init(ds)
|
|
|
|
|
|
|
|
|
|
testUser := model.User{
|
|
|
|
|
ID: "user-1",
|
|
|
|
|
UserName: "testuser",
|
|
|
|
|
Name: "Test User",
|
|
|
|
|
IsAdmin: false,
|
|
|
|
|
NewPassword: "testpass",
|
|
|
|
|
}
|
|
|
|
|
err := userRepo.Put(&testUser)
|
|
|
|
|
Expect(err).ToNot(HaveOccurred())
|
|
|
|
|
|
2026-09-03 23:19:49 -04:00
|
|
|
nativeRouter := New(ds, nil, plsSvc, nil, tests.NewMockLibraryService(), tests.NewMockUserService(), nil, nil, nil, nil, nil)
|
2026-02-15 22:36:58 -05:00
|
|
|
router = server.JWTVerifier(nativeRouter)
|
|
|
|
|
w = httptest.NewRecorder()
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
createAuthenticatedRequest := func(method, path string) *http.Request {
|
|
|
|
|
req := httptest.NewRequest(method, path, nil)
|
|
|
|
|
testUser := model.User{ID: "user-1", UserName: "testuser"}
|
|
|
|
|
token, err := auth.CreateToken(&testUser)
|
|
|
|
|
Expect(err).ToNot(HaveOccurred())
|
|
|
|
|
req.Header.Set(consts.UIAuthorizationHeader, "Bearer "+token)
|
|
|
|
|
return req
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
Describe("GET /playlist/{playlistId}/tracks", func() {
|
|
|
|
|
It("returns 404 when playlist does not exist", func() {
|
|
|
|
|
req := createAuthenticatedRequest("GET", "/playlist/non-existent/tracks")
|
|
|
|
|
router.ServeHTTP(w, req)
|
|
|
|
|
|
|
|
|
|
Expect(w.Code).To(Equal(http.StatusNotFound))
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
It("returns tracks when playlist exists", func() {
|
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 19:57:13 -05:00
|
|
|
plsSvc.tracksRepo = &mockPlaylistTrackRepo{
|
2026-02-15 22:36:58 -05:00
|
|
|
tracks: model.PlaylistTracks{
|
|
|
|
|
{ID: "1", MediaFileID: "mf-1", PlaylistID: "pls-1"},
|
|
|
|
|
{ID: "2", MediaFileID: "mf-2", PlaylistID: "pls-1"},
|
|
|
|
|
},
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
req := createAuthenticatedRequest("GET", "/playlist/pls-1/tracks")
|
|
|
|
|
router.ServeHTTP(w, req)
|
|
|
|
|
|
|
|
|
|
Expect(w.Code).To(Equal(http.StatusOK))
|
|
|
|
|
|
|
|
|
|
var response []model.PlaylistTrack
|
|
|
|
|
err := json.Unmarshal(w.Body.Bytes(), &response)
|
|
|
|
|
Expect(err).ToNot(HaveOccurred())
|
|
|
|
|
Expect(response).To(HaveLen(2))
|
|
|
|
|
Expect(response[0].ID).To(Equal("1"))
|
|
|
|
|
Expect(response[1].ID).To(Equal("2"))
|
|
|
|
|
})
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
Describe("GET /playlist/{playlistId}/tracks/{id}", func() {
|
|
|
|
|
It("returns 404 when playlist does not exist", func() {
|
|
|
|
|
req := createAuthenticatedRequest("GET", "/playlist/non-existent/tracks/1")
|
|
|
|
|
router.ServeHTTP(w, req)
|
|
|
|
|
|
|
|
|
|
Expect(w.Code).To(Equal(http.StatusNotFound))
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
It("returns the track when playlist exists", func() {
|
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 19:57:13 -05:00
|
|
|
plsSvc.tracksRepo = &mockPlaylistTrackRepo{
|
2026-02-15 22:36:58 -05:00
|
|
|
tracks: model.PlaylistTracks{
|
|
|
|
|
{ID: "1", MediaFileID: "mf-1", PlaylistID: "pls-1"},
|
|
|
|
|
},
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
req := createAuthenticatedRequest("GET", "/playlist/pls-1/tracks/1")
|
|
|
|
|
router.ServeHTTP(w, req)
|
|
|
|
|
|
|
|
|
|
Expect(w.Code).To(Equal(http.StatusOK))
|
|
|
|
|
|
|
|
|
|
var response model.PlaylistTrack
|
|
|
|
|
err := json.Unmarshal(w.Body.Bytes(), &response)
|
|
|
|
|
Expect(err).ToNot(HaveOccurred())
|
|
|
|
|
Expect(response.ID).To(Equal("1"))
|
|
|
|
|
Expect(response.MediaFileID).To(Equal("mf-1"))
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
It("returns 404 when track does not exist in playlist", func() {
|
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 19:57:13 -05:00
|
|
|
plsSvc.tracksRepo = &mockPlaylistTrackRepo{
|
2026-02-15 22:36:58 -05:00
|
|
|
tracks: model.PlaylistTracks{},
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
req := createAuthenticatedRequest("GET", "/playlist/pls-1/tracks/999")
|
|
|
|
|
router.ServeHTTP(w, req)
|
|
|
|
|
|
|
|
|
|
Expect(w.Code).To(Equal(http.StatusNotFound))
|
|
|
|
|
})
|
|
|
|
|
})
|
|
|
|
|
})
|
2026-03-02 16:59:05 -05:00
|
|
|
|
fix(playlist): block track edits on synced playlists across all APIs (#5984)
* fix(playlist): block track edits on synced playlists across all APIs
A synced playlist's tracks come from its source file, so any track edit made
through the UI or an API was silently reverted on the next scan. Track mutations
funnel through two service guards, checkTracksEditable (incremental edits) and
Create (wholesale replace, used by Subsonic createPlaylist and Jellyfin's
replace path), which each duplicated the smart-playlist check. Both now consult
a shared model.Playlist.TracksEditable() predicate, so the native, Subsonic, and
Jellyfin paths are all locked: track edits return ErrNotAuthorized (403, or
Subsonic error 50) instead of being accepted and lost. Metadata-only edits
(name, comment, public, the sync flag itself) still go through checkWritable and
are unaffected. In the UI, a synced playlist's track list becomes read-only,
mirroring how smart playlists already behave.
* fix(playlist): return 409 Conflict for non-editable playlist track edits
The previous commit rejected track edits on smart and synced playlists with
ErrNotAuthorized (403). That conflates two different things: a 403 says the
caller lacks permission, but a synced or smart playlist's tracks are immutable
for everyone, including the owner and admins. It is a property of the resource,
not the caller.
Introduce ErrPlaylistNotEditable and return it from both track-edit guards. The
Native and Jellyfin APIs now map it to 409 Conflict; Subsonic maps it to error
50, the closest code it has (it has no read-only concept). The Native track
handlers previously mapped this rejection inconsistently (400 on add, 500 on
remove, 403 on reorder) through a new shared writePlaylistError helper. Genuine
authorization failures (non-owner, non-admin) still return ErrNotAuthorized.
* fix(playlist): surface synced read-only state in picker, Jellyfin, and OpenSubsonic
Follow-up to the track-edit lock: the read-only state was enforced but not
advertised consistently, so clients still offered edits that the server rejects.
- UI: the Add to Playlist picker filtered targets by isWritable only, offering
synced playlists that then 409 on add. It now filters with canChangeTracks.
- Jellyfin: addToPlaylist/removeFromPlaylist hard-coded every error to 404, so a
locked playlist reported "not found" instead of 409. They now return 409 for
ErrPlaylistNotEditable while keeping the deliberate anti-probing 404 for every
other error (a non-owner never reaches ErrPlaylistNotEditable, so 409 leaks
nothing).
- OpenSubsonic: buildOSPlaylist marked only smart playlists readonly; owned
synced playlists advertised readonly=false. Readonly now also covers
!TracksEditable(), matching the existing smart-playlist treatment.
* fix(jellyfin): report CanEdit from playlist editability in permission probes
getPlaylistUsers and getPlaylistUser returned CanEdit: true unconditionally, so
Finamp (which probes this before showing edit controls) offered track editing on
synced/smart playlists whose add/remove requests now return 409. Both handlers
now fetch the playlist and set CanEdit from TracksEditable(), keeping the
deliberate non-owner looseness (CanEdit stays true for a normal playlist a
non-owner views) and mapping any lookup error to 404 like the sibling probes.
* fix(playlist): check ownership before editability when replacing tracks
Create checked TracksEditable() before ownership, so a non-owner replacing
another user's public smart/synced playlist (Jellyfin updatePlaylist with a
non-empty Ids list) received a 409 read-only conflict instead of a 403
authorization failure. The incremental guards check ownership first via
checkWritable; Create now matches that order. Subsonic is unaffected (both errors
map to code 50). Owners of their own smart/synced playlists still get the
read-only conflict.
* fix(jellyfin): return 403 for locked playlists, matching Jellyfin
Jellyfin itself refuses edits on its file-backed playlists with Forbid() (403):
PlaylistsController gates every mutation on OwnerUserId == caller or a share with
CanEdit, and playlists imported from .m3u files satisfy neither. Its CanEdit is
an ACL field, not a read-only marker, and Jellyfin core has no server-managed
playlist type at all.
Our Jellyfin routes exist to imitate that API, so ErrPlaylistNotEditable now maps
to 403 there instead of 409. The native API keeps 409 (a resource-state conflict
is the accurate REST answer where we define the contract) and Subsonic keeps
error 50, its closest code.
* chore(playlist): trim comments added by this branch
Several comments ran to three or four lines and carried rationale that belongs in
the commit history rather than the code: what Jellyfin does with its own
file-backed playlists, and restatements of the expressions directly below them.
Each block is now one or two lines covering only the non-obvious why.
2026-08-19 08:47:53 -04:00
|
|
|
var _ = Describe("writePlaylistError", func() {
|
|
|
|
|
DescribeTable("maps a service error to an HTTP status",
|
|
|
|
|
func(err error, expected int) {
|
|
|
|
|
w := httptest.NewRecorder()
|
|
|
|
|
writePlaylistError(w, err, http.StatusBadRequest)
|
|
|
|
|
Expect(w.Code).To(Equal(expected))
|
|
|
|
|
},
|
|
|
|
|
Entry("not found -> 404", model.ErrNotFound, http.StatusNotFound),
|
|
|
|
|
Entry("not authorized -> 403", model.ErrNotAuthorized, http.StatusForbidden),
|
|
|
|
|
Entry("not editable -> 409", model.ErrPlaylistNotEditable, http.StatusConflict),
|
|
|
|
|
Entry("unrecognized -> default", model.ErrValidation, http.StatusBadRequest),
|
|
|
|
|
)
|
|
|
|
|
})
|
|
|
|
|
|
2026-03-02 16:59:05 -05:00
|
|
|
type mockPlaylistTrackRepo struct {
|
|
|
|
|
model.PlaylistTrackRepository
|
|
|
|
|
tracks model.PlaylistTracks
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
func (m *mockPlaylistTrackRepo) Count(...rest.QueryOptions) (int64, error) {
|
|
|
|
|
return int64(len(m.tracks)), nil
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
func (m *mockPlaylistTrackRepo) ReadAll(...rest.QueryOptions) (any, error) {
|
|
|
|
|
return m.tracks, nil
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
func (m *mockPlaylistTrackRepo) EntityName() string {
|
|
|
|
|
return "playlist_track"
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
func (m *mockPlaylistTrackRepo) NewInstance() any {
|
|
|
|
|
return &model.PlaylistTrack{}
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
func (m *mockPlaylistTrackRepo) Read(id string) (any, error) {
|
|
|
|
|
for _, t := range m.tracks {
|
|
|
|
|
if t.ID == id {
|
|
|
|
|
return &t, nil
|
|
|
|
|
}
|
|
|
|
|
}
|
|
|
|
|
return nil, rest.ErrNotFound
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
type mockPlaylistsService struct {
|
|
|
|
|
playlists.Playlists
|
|
|
|
|
tracksRepo rest.Repository
|
|
|
|
|
removeImageFn func(ctx context.Context, id string) error
|
|
|
|
|
setImageFn func(ctx context.Context, id string, reader io.Reader, ext string) error
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
func (m *mockPlaylistsService) RemoveImage(ctx context.Context, id string) error {
|
|
|
|
|
if m.removeImageFn != nil {
|
|
|
|
|
return m.removeImageFn(ctx, id)
|
|
|
|
|
}
|
|
|
|
|
return model.ErrNotFound
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
func (m *mockPlaylistsService) SetImage(ctx context.Context, id string, reader io.Reader, ext string) error {
|
|
|
|
|
if m.setImageFn != nil {
|
|
|
|
|
return m.setImageFn(ctx, id, reader, ext)
|
|
|
|
|
}
|
|
|
|
|
return model.ErrNotFound
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
func (m *mockPlaylistsService) TracksRepository(_ context.Context, _ string, _ bool) rest.Repository {
|
|
|
|
|
return m.tracksRepo
|
|
|
|
|
}
|