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_test
|
|
|
|
|
|
|
|
|
|
import (
|
|
|
|
|
"context"
|
2026-03-01 10:07:18 -09:00
|
|
|
"os"
|
|
|
|
|
"path/filepath"
|
|
|
|
|
"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
|
|
|
|
2026-03-01 10:07:18 -09:00
|
|
|
"github.com/navidrome/navidrome/conf"
|
|
|
|
|
"github.com/navidrome/navidrome/conf/configtest"
|
2026-03-15 18:19:55 -08:00
|
|
|
"github.com/navidrome/navidrome/core"
|
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/core/playlists"
|
|
|
|
|
"github.com/navidrome/navidrome/model"
|
|
|
|
|
"github.com/navidrome/navidrome/model/criteria"
|
|
|
|
|
"github.com/navidrome/navidrome/model/request"
|
|
|
|
|
"github.com/navidrome/navidrome/tests"
|
|
|
|
|
. "github.com/onsi/ginkgo/v2"
|
|
|
|
|
. "github.com/onsi/gomega"
|
|
|
|
|
)
|
|
|
|
|
|
|
|
|
|
var _ = Describe("Playlists", func() {
|
|
|
|
|
var ds *tests.MockDataStore
|
|
|
|
|
var ps playlists.Playlists
|
|
|
|
|
var mockPlsRepo *tests.MockPlaylistRepo
|
|
|
|
|
ctx := context.Background()
|
|
|
|
|
|
|
|
|
|
BeforeEach(func() {
|
|
|
|
|
mockPlsRepo = tests.CreateMockPlaylistRepo()
|
|
|
|
|
ds = &tests.MockDataStore{
|
|
|
|
|
MockedPlaylist: mockPlsRepo,
|
|
|
|
|
MockedLibrary: &tests.MockLibraryRepo{},
|
|
|
|
|
}
|
|
|
|
|
ctx = request.WithUser(ctx, model.User{ID: "123"})
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
Describe("Delete", func() {
|
|
|
|
|
var mockTracks *tests.MockPlaylistTrackRepo
|
|
|
|
|
|
|
|
|
|
BeforeEach(func() {
|
|
|
|
|
mockTracks = &tests.MockPlaylistTrackRepo{AddCount: 3}
|
|
|
|
|
mockPlsRepo.Data = map[string]*model.Playlist{
|
|
|
|
|
"pls-1": {ID: "pls-1", Name: "My Playlist", OwnerID: "user-1"},
|
|
|
|
|
}
|
|
|
|
|
mockPlsRepo.TracksRepo = mockTracks
|
2026-03-15 18:19:55 -08:00
|
|
|
ps = playlists.NewPlaylists(ds, core.NewImageUploadService())
|
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
|
|
|
})
|
|
|
|
|
|
|
|
|
|
It("allows owner to delete their playlist", func() {
|
|
|
|
|
ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false})
|
|
|
|
|
err := ps.Delete(ctx, "pls-1")
|
|
|
|
|
Expect(err).ToNot(HaveOccurred())
|
|
|
|
|
Expect(mockPlsRepo.Deleted).To(ContainElement("pls-1"))
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
It("allows admin to delete any playlist", func() {
|
|
|
|
|
ctx = request.WithUser(ctx, model.User{ID: "admin-1", IsAdmin: true})
|
|
|
|
|
err := ps.Delete(ctx, "pls-1")
|
|
|
|
|
Expect(err).ToNot(HaveOccurred())
|
|
|
|
|
Expect(mockPlsRepo.Deleted).To(ContainElement("pls-1"))
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
It("denies non-owner, non-admin from deleting", func() {
|
|
|
|
|
ctx = request.WithUser(ctx, model.User{ID: "other-user", IsAdmin: false})
|
|
|
|
|
err := ps.Delete(ctx, "pls-1")
|
|
|
|
|
Expect(err).To(MatchError(model.ErrNotAuthorized))
|
|
|
|
|
Expect(mockPlsRepo.Deleted).To(BeEmpty())
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
It("returns error when playlist not found", func() {
|
|
|
|
|
ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false})
|
|
|
|
|
err := ps.Delete(ctx, "nonexistent")
|
|
|
|
|
Expect(err).To(Equal(model.ErrNotFound))
|
|
|
|
|
})
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
Describe("Create", func() {
|
|
|
|
|
BeforeEach(func() {
|
|
|
|
|
mockPlsRepo.Data = map[string]*model.Playlist{
|
|
|
|
|
"pls-1": {ID: "pls-1", Name: "Existing", OwnerID: "user-1"},
|
|
|
|
|
"pls-2": {ID: "pls-2", Name: "Other's", OwnerID: "other-user"},
|
|
|
|
|
"pls-smart": {ID: "pls-smart", Name: "Smart", OwnerID: "user-1",
|
|
|
|
|
Rules: &criteria.Criteria{Expression: criteria.Contains{"title": "test"}}},
|
|
|
|
|
}
|
2026-03-15 18:19:55 -08:00
|
|
|
ps = playlists.NewPlaylists(ds, core.NewImageUploadService())
|
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
|
|
|
})
|
|
|
|
|
|
|
|
|
|
It("creates a new playlist with owner set from context", func() {
|
|
|
|
|
ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false})
|
|
|
|
|
id, err := ps.Create(ctx, "", "New Playlist", []string{"song-1", "song-2"})
|
|
|
|
|
Expect(err).ToNot(HaveOccurred())
|
|
|
|
|
Expect(id).ToNot(BeEmpty())
|
|
|
|
|
Expect(mockPlsRepo.Last.Name).To(Equal("New Playlist"))
|
|
|
|
|
Expect(mockPlsRepo.Last.OwnerID).To(Equal("user-1"))
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
It("replaces tracks on existing playlist when owner matches", func() {
|
|
|
|
|
ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false})
|
|
|
|
|
id, err := ps.Create(ctx, "pls-1", "", []string{"song-3"})
|
|
|
|
|
Expect(err).ToNot(HaveOccurred())
|
|
|
|
|
Expect(id).To(Equal("pls-1"))
|
|
|
|
|
Expect(mockPlsRepo.Last.Tracks).To(HaveLen(1))
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
It("allows admin to replace tracks on any playlist", func() {
|
|
|
|
|
ctx = request.WithUser(ctx, model.User{ID: "admin-1", IsAdmin: true})
|
|
|
|
|
id, err := ps.Create(ctx, "pls-2", "", []string{"song-3"})
|
|
|
|
|
Expect(err).ToNot(HaveOccurred())
|
|
|
|
|
Expect(id).To(Equal("pls-2"))
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
It("denies non-owner, non-admin from replacing tracks on existing playlist", func() {
|
|
|
|
|
ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false})
|
|
|
|
|
_, err := ps.Create(ctx, "pls-2", "", []string{"song-3"})
|
|
|
|
|
Expect(err).To(MatchError(model.ErrNotAuthorized))
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
It("returns error when existing playlistId not found", func() {
|
|
|
|
|
ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false})
|
|
|
|
|
_, err := ps.Create(ctx, "nonexistent", "", []string{"song-1"})
|
|
|
|
|
Expect(err).To(Equal(model.ErrNotFound))
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
It("denies replacing tracks on a smart playlist", func() {
|
|
|
|
|
ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false})
|
|
|
|
|
_, err := ps.Create(ctx, "pls-smart", "", []string{"song-1"})
|
|
|
|
|
Expect(err).To(MatchError(model.ErrNotAuthorized))
|
|
|
|
|
})
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
Describe("Update", func() {
|
|
|
|
|
var mockTracks *tests.MockPlaylistTrackRepo
|
|
|
|
|
|
|
|
|
|
BeforeEach(func() {
|
|
|
|
|
mockTracks = &tests.MockPlaylistTrackRepo{AddCount: 2}
|
|
|
|
|
mockPlsRepo.Data = map[string]*model.Playlist{
|
|
|
|
|
"pls-1": {ID: "pls-1", Name: "My Playlist", OwnerID: "user-1"},
|
|
|
|
|
"pls-other": {ID: "pls-other", Name: "Other's", OwnerID: "other-user"},
|
|
|
|
|
"pls-smart": {ID: "pls-smart", Name: "Smart", OwnerID: "user-1",
|
|
|
|
|
Rules: &criteria.Criteria{Expression: criteria.Contains{"title": "test"}}},
|
|
|
|
|
}
|
|
|
|
|
mockPlsRepo.TracksRepo = mockTracks
|
2026-03-15 18:19:55 -08:00
|
|
|
ps = playlists.NewPlaylists(ds, core.NewImageUploadService())
|
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
|
|
|
})
|
|
|
|
|
|
|
|
|
|
It("allows owner to update their playlist", func() {
|
|
|
|
|
ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false})
|
2026-05-19 13:02:29 -08:00
|
|
|
err := ps.Update(ctx, "pls-1", new("Updated Name"), nil, nil, nil, nil)
|
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
|
|
|
Expect(err).ToNot(HaveOccurred())
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
It("allows admin to update any playlist", func() {
|
|
|
|
|
ctx = request.WithUser(ctx, model.User{ID: "admin-1", IsAdmin: true})
|
2026-05-19 13:02:29 -08:00
|
|
|
err := ps.Update(ctx, "pls-other", new("Updated Name"), nil, nil, nil, nil)
|
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
|
|
|
Expect(err).ToNot(HaveOccurred())
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
It("denies non-owner, non-admin from updating", func() {
|
|
|
|
|
ctx = request.WithUser(ctx, model.User{ID: "other-user", IsAdmin: false})
|
2026-05-19 13:02:29 -08:00
|
|
|
err := ps.Update(ctx, "pls-1", new("Updated Name"), nil, nil, nil, nil)
|
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
|
|
|
Expect(err).To(MatchError(model.ErrNotAuthorized))
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
It("returns error when playlist not found", func() {
|
|
|
|
|
ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false})
|
2026-05-19 13:02:29 -08:00
|
|
|
err := ps.Update(ctx, "nonexistent", new("Updated Name"), nil, nil, nil, nil)
|
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
|
|
|
Expect(err).To(Equal(model.ErrNotFound))
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
It("denies adding tracks to a smart playlist", func() {
|
|
|
|
|
ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false})
|
|
|
|
|
err := ps.Update(ctx, "pls-smart", nil, nil, nil, []string{"song-1"}, nil)
|
|
|
|
|
Expect(err).To(MatchError(model.ErrNotAuthorized))
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
It("denies removing tracks from a smart playlist", func() {
|
|
|
|
|
ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false})
|
|
|
|
|
err := ps.Update(ctx, "pls-smart", nil, nil, nil, nil, []int{0})
|
|
|
|
|
Expect(err).To(MatchError(model.ErrNotAuthorized))
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
It("allows metadata updates on a smart playlist", func() {
|
|
|
|
|
ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false})
|
2026-05-19 13:02:29 -08:00
|
|
|
err := ps.Update(ctx, "pls-smart", new("Updated Smart"), nil, nil, nil, nil)
|
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
|
|
|
Expect(err).ToNot(HaveOccurred())
|
|
|
|
|
})
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
Describe("AddTracks", func() {
|
|
|
|
|
var mockTracks *tests.MockPlaylistTrackRepo
|
|
|
|
|
|
|
|
|
|
BeforeEach(func() {
|
|
|
|
|
mockTracks = &tests.MockPlaylistTrackRepo{AddCount: 2}
|
|
|
|
|
mockPlsRepo.Data = map[string]*model.Playlist{
|
|
|
|
|
"pls-1": {ID: "pls-1", Name: "My Playlist", OwnerID: "user-1"},
|
|
|
|
|
"pls-smart": {ID: "pls-smart", Name: "Smart", OwnerID: "user-1",
|
|
|
|
|
Rules: &criteria.Criteria{Expression: criteria.Contains{"title": "test"}}},
|
|
|
|
|
"pls-other": {ID: "pls-other", Name: "Other's", OwnerID: "other-user"},
|
|
|
|
|
}
|
|
|
|
|
mockPlsRepo.TracksRepo = mockTracks
|
2026-03-15 18:19:55 -08:00
|
|
|
ps = playlists.NewPlaylists(ds, core.NewImageUploadService())
|
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
|
|
|
})
|
|
|
|
|
|
|
|
|
|
It("allows owner to add tracks", func() {
|
|
|
|
|
ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false})
|
|
|
|
|
count, err := ps.AddTracks(ctx, "pls-1", []string{"song-1", "song-2"})
|
|
|
|
|
Expect(err).ToNot(HaveOccurred())
|
|
|
|
|
Expect(count).To(Equal(2))
|
|
|
|
|
Expect(mockTracks.AddedIds).To(ConsistOf("song-1", "song-2"))
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
It("allows admin to add tracks to any playlist", func() {
|
|
|
|
|
ctx = request.WithUser(ctx, model.User{ID: "admin-1", IsAdmin: true})
|
|
|
|
|
count, err := ps.AddTracks(ctx, "pls-other", []string{"song-1"})
|
|
|
|
|
Expect(err).ToNot(HaveOccurred())
|
|
|
|
|
Expect(count).To(Equal(2))
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
It("denies non-owner, non-admin", func() {
|
|
|
|
|
ctx = request.WithUser(ctx, model.User{ID: "other-user", IsAdmin: false})
|
|
|
|
|
_, err := ps.AddTracks(ctx, "pls-1", []string{"song-1"})
|
|
|
|
|
Expect(err).To(MatchError(model.ErrNotAuthorized))
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
It("denies editing smart playlists", func() {
|
|
|
|
|
ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false})
|
|
|
|
|
_, err := ps.AddTracks(ctx, "pls-smart", []string{"song-1"})
|
|
|
|
|
Expect(err).To(MatchError(model.ErrNotAuthorized))
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
It("returns error when playlist not found", func() {
|
|
|
|
|
ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false})
|
|
|
|
|
_, err := ps.AddTracks(ctx, "nonexistent", []string{"song-1"})
|
|
|
|
|
Expect(err).To(Equal(model.ErrNotFound))
|
|
|
|
|
})
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
Describe("RemoveTracks", func() {
|
|
|
|
|
var mockTracks *tests.MockPlaylistTrackRepo
|
|
|
|
|
|
|
|
|
|
BeforeEach(func() {
|
|
|
|
|
mockTracks = &tests.MockPlaylistTrackRepo{}
|
|
|
|
|
mockPlsRepo.Data = map[string]*model.Playlist{
|
|
|
|
|
"pls-1": {ID: "pls-1", Name: "My Playlist", OwnerID: "user-1"},
|
|
|
|
|
"pls-smart": {ID: "pls-smart", Name: "Smart", OwnerID: "user-1",
|
|
|
|
|
Rules: &criteria.Criteria{Expression: criteria.Contains{"title": "test"}}},
|
|
|
|
|
}
|
|
|
|
|
mockPlsRepo.TracksRepo = mockTracks
|
2026-03-15 18:19:55 -08:00
|
|
|
ps = playlists.NewPlaylists(ds, core.NewImageUploadService())
|
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
|
|
|
})
|
|
|
|
|
|
|
|
|
|
It("allows owner to remove tracks", func() {
|
|
|
|
|
ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false})
|
|
|
|
|
err := ps.RemoveTracks(ctx, "pls-1", []string{"track-1", "track-2"})
|
|
|
|
|
Expect(err).ToNot(HaveOccurred())
|
|
|
|
|
Expect(mockTracks.DeletedIds).To(ConsistOf("track-1", "track-2"))
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
It("denies on smart playlist", func() {
|
|
|
|
|
ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false})
|
|
|
|
|
err := ps.RemoveTracks(ctx, "pls-smart", []string{"track-1"})
|
|
|
|
|
Expect(err).To(MatchError(model.ErrNotAuthorized))
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
It("denies non-owner", func() {
|
|
|
|
|
ctx = request.WithUser(ctx, model.User{ID: "other-user", IsAdmin: false})
|
|
|
|
|
err := ps.RemoveTracks(ctx, "pls-1", []string{"track-1"})
|
|
|
|
|
Expect(err).To(MatchError(model.ErrNotAuthorized))
|
|
|
|
|
})
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
Describe("ReorderTrack", func() {
|
|
|
|
|
var mockTracks *tests.MockPlaylistTrackRepo
|
|
|
|
|
|
|
|
|
|
BeforeEach(func() {
|
|
|
|
|
mockTracks = &tests.MockPlaylistTrackRepo{}
|
|
|
|
|
mockPlsRepo.Data = map[string]*model.Playlist{
|
|
|
|
|
"pls-1": {ID: "pls-1", Name: "My Playlist", OwnerID: "user-1"},
|
|
|
|
|
"pls-smart": {ID: "pls-smart", Name: "Smart", OwnerID: "user-1",
|
|
|
|
|
Rules: &criteria.Criteria{Expression: criteria.Contains{"title": "test"}}},
|
|
|
|
|
}
|
|
|
|
|
mockPlsRepo.TracksRepo = mockTracks
|
2026-03-15 18:19:55 -08:00
|
|
|
ps = playlists.NewPlaylists(ds, core.NewImageUploadService())
|
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
|
|
|
})
|
|
|
|
|
|
|
|
|
|
It("allows owner to reorder", func() {
|
|
|
|
|
ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false})
|
|
|
|
|
err := ps.ReorderTrack(ctx, "pls-1", 1, 3)
|
|
|
|
|
Expect(err).ToNot(HaveOccurred())
|
|
|
|
|
Expect(mockTracks.Reordered).To(BeTrue())
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
It("denies on smart playlist", func() {
|
|
|
|
|
ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false})
|
|
|
|
|
err := ps.ReorderTrack(ctx, "pls-smart", 1, 3)
|
|
|
|
|
Expect(err).To(MatchError(model.ErrNotAuthorized))
|
|
|
|
|
})
|
|
|
|
|
})
|
2026-03-01 10:07:18 -09:00
|
|
|
|
|
|
|
|
Describe("SetImage", func() {
|
|
|
|
|
var tmpDir string
|
|
|
|
|
|
|
|
|
|
BeforeEach(func() {
|
|
|
|
|
DeferCleanup(configtest.SetupConfig())
|
|
|
|
|
tmpDir = GinkgoT().TempDir()
|
refactor(conf): replace eager dir creation with lazy Dir type (#5495)
* feat(conf): add Dir type with lazy directory creation
Introduces the Dir type that wraps a directory path string and defers
os.MkdirAll until the first call to Path() or MustPath(), using sync.Once
to ensure the creation happens exactly once. Implements fmt.Stringer,
encoding.TextMarshaler, and encoding.TextUnmarshaler for config integration.
Includes Ginkgo/Gomega tests covering all methods and error paths.
* refactor(conf): replace eager dir creation with lazy Dir type
Change DataFolder, CacheFolder, Plugins.Folder, and Backup.Path from
string to Dir. Remove all os.MkdirAll calls from Load() so directories
are created lazily on first Path()/MustPath() call. Artwork folder
creation was already handled at point-of-use in image_upload.go.
Add SnapshotConfig() to conf package for safe test config save/restore
that avoids copying sync.Once inside Dir fields. Fix copy-lock vet
warning in nativeapi/config.go by marshalling pointer instead of value.
* refactor(conf): migrate tests and db init to lazy Dir type
Update all test files to use conf.NewDir() for Dir field assignments.
Ensure DataFolder is created lazily when the database is first opened
in db.Db(). Remove eager directory creation from conf.Load() tests.
* fix(conf): address review findings for Dir type
- Use os.ModePerm for DataFolder/CacheFolder (was 0700, should match
original behavior). Add NewDirWithPerm for PluginsFolder (0700).
- Use Path() instead of MustPath() in db.Prune() to avoid logFatal
from background cron job.
- Panic on marshal/unmarshal errors in SnapshotConfig (test helper).
- Clean up redundant String()/MustPath() calls in plugin manager.
- Remove dead code in dir_test.go.
Signed-off-by: Deluan <deluan@navidrome.org>
* fix(conf): add GoString to Dir for clean config dump output
Implement fmt.GoStringer on Dir so pretty.Sprintf shows the path
string instead of internal struct fields (sync.Once, perm, err).
Also add TODO comment to configtest about removing the indirection.
* fix(dir): improve error logging in MustPath method
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor(tests): remove redundant tests for unwritable DataFolder and CacheFolder
Signed-off-by: Deluan <deluan@navidrome.org>
* fix(conf): address PR review feedback
- Ensure Plugins.Folder always uses 0700, even when user-configured
(previously only the derived default got restrictive permissions).
- Create LogFile parent directory before opening, so LogFile paths
inside a not-yet-created DataFolder work correctly.
---------
Signed-off-by: Deluan <deluan@navidrome.org>
2026-05-13 12:44:22 -08:00
|
|
|
conf.Server.DataFolder = conf.NewDir(tmpDir)
|
2026-03-01 10:07:18 -09:00
|
|
|
|
|
|
|
|
mockPlsRepo.Data = map[string]*model.Playlist{
|
|
|
|
|
"pls-1": {ID: "pls-1", Name: "My Playlist", OwnerID: "user-1"},
|
|
|
|
|
"pls-other": {ID: "pls-other", Name: "Other's", OwnerID: "other-user"},
|
|
|
|
|
}
|
2026-03-15 18:19:55 -08:00
|
|
|
ps = playlists.NewPlaylists(ds, core.NewImageUploadService())
|
2026-03-01 10:07:18 -09:00
|
|
|
})
|
|
|
|
|
|
feat(playlist): support #EXTALBUMARTURL directive and sidecar images (#5131)
* feat(playlist): add migration for playlist image field rename and external URL
* refactor(playlist): rename ImageFile to UploadedImage and ArtworkPath to UploadedImagePath
Rename playlist model fields and methods for clarity in preparation for
adding external image URL and sidecar image support. Add the new
ExternalImageURL field to the Playlist model.
* feat(playlist): parse #EXTALBUMARTURL directive in M3U imports
* feat(playlist): always sync ExternalImageURL on re-scan, preserve UploadedImage
* feat(artwork): add sidecar image discovery and cache invalidation for playlists
Add playlist sidecar image support to the artwork reader fallback chain.
A sidecar image (e.g., MyPlaylist.jpg next to MyPlaylist.m3u) is discovered
via case-insensitive base name matching using model.IsImageFile(). Cache
invalidation uses max(playlist.UpdatedAt, imageFile.ModTime()) to bust
stale artwork when sidecar or ExternalImageURL local files change.
* feat(artwork): add external image URL source to playlist artwork reader
Add fromPlaylistExternalImage source function that resolves playlist
cover art from ExternalImageURL, supporting both HTTP(S) URLs (via
the existing fromURL helper) and local file paths (via os.Open).
Insert it in the Reader() fallback chain between sidecar and tiled cover.
* refactor(artwork): simplify playlist artwork source functions
Extract shared fromLocalFile helper, use url.Parse for scheme check,
and collapse sidecar directory scan conditions.
* test(artwork): remove redundant fromPlaylistSidecar tests
These tests duplicated scenarios already covered by findPlaylistSidecarPath
tests combined with fromLocalFile (tested via fromPlaylistExternalImage).
After refactoring fromPlaylistSidecar to a one-liner composing those two
functions, the wrapper tests add no value.
* fix(playlist): address security review comments from PR #5131:
- Use url.PathUnescape instead of url.QueryUnescape for file:// URLs so
that '+' in filenames is preserved (not decoded as space).
- Validate all local image paths (file://, absolute, relative) against
known library boundaries via libraryMatcher, rejecting paths outside
any configured library.
- Harden #EXTALBUMARTURL against path traversal and SSRF by adding EnableM3UExternalAlbumArt config flag (default false, also
disabled by EnableExternalServices=false) to gate HTTP(S) URL storage
at parse time and fetching at read time (defense in depth).
- Log a warning when os.ReadDir fails in findPlaylistSidecarPath for
diagnosability.
- Extract resolveLocalPath helper to simplify resolveImageURL.
Signed-off-by: Deluan <deluan@navidrome.org>
* feat(playlist): implement human-friendly filename generation for uploaded playlist cover images
Signed-off-by: Deluan <deluan@navidrome.org>
---------
Signed-off-by: Deluan <deluan@navidrome.org>
2026-03-02 07:39:59 -09:00
|
|
|
It("saves image file and updates UploadedImage", func() {
|
2026-03-01 10:07:18 -09:00
|
|
|
ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false})
|
|
|
|
|
reader := strings.NewReader("fake image data")
|
|
|
|
|
err := ps.SetImage(ctx, "pls-1", reader, ".jpg")
|
|
|
|
|
Expect(err).ToNot(HaveOccurred())
|
|
|
|
|
|
feat(playlist): support #EXTALBUMARTURL directive and sidecar images (#5131)
* feat(playlist): add migration for playlist image field rename and external URL
* refactor(playlist): rename ImageFile to UploadedImage and ArtworkPath to UploadedImagePath
Rename playlist model fields and methods for clarity in preparation for
adding external image URL and sidecar image support. Add the new
ExternalImageURL field to the Playlist model.
* feat(playlist): parse #EXTALBUMARTURL directive in M3U imports
* feat(playlist): always sync ExternalImageURL on re-scan, preserve UploadedImage
* feat(artwork): add sidecar image discovery and cache invalidation for playlists
Add playlist sidecar image support to the artwork reader fallback chain.
A sidecar image (e.g., MyPlaylist.jpg next to MyPlaylist.m3u) is discovered
via case-insensitive base name matching using model.IsImageFile(). Cache
invalidation uses max(playlist.UpdatedAt, imageFile.ModTime()) to bust
stale artwork when sidecar or ExternalImageURL local files change.
* feat(artwork): add external image URL source to playlist artwork reader
Add fromPlaylistExternalImage source function that resolves playlist
cover art from ExternalImageURL, supporting both HTTP(S) URLs (via
the existing fromURL helper) and local file paths (via os.Open).
Insert it in the Reader() fallback chain between sidecar and tiled cover.
* refactor(artwork): simplify playlist artwork source functions
Extract shared fromLocalFile helper, use url.Parse for scheme check,
and collapse sidecar directory scan conditions.
* test(artwork): remove redundant fromPlaylistSidecar tests
These tests duplicated scenarios already covered by findPlaylistSidecarPath
tests combined with fromLocalFile (tested via fromPlaylistExternalImage).
After refactoring fromPlaylistSidecar to a one-liner composing those two
functions, the wrapper tests add no value.
* fix(playlist): address security review comments from PR #5131:
- Use url.PathUnescape instead of url.QueryUnescape for file:// URLs so
that '+' in filenames is preserved (not decoded as space).
- Validate all local image paths (file://, absolute, relative) against
known library boundaries via libraryMatcher, rejecting paths outside
any configured library.
- Harden #EXTALBUMARTURL against path traversal and SSRF by adding EnableM3UExternalAlbumArt config flag (default false, also
disabled by EnableExternalServices=false) to gate HTTP(S) URL storage
at parse time and fetching at read time (defense in depth).
- Log a warning when os.ReadDir fails in findPlaylistSidecarPath for
diagnosability.
- Extract resolveLocalPath helper to simplify resolveImageURL.
Signed-off-by: Deluan <deluan@navidrome.org>
* feat(playlist): implement human-friendly filename generation for uploaded playlist cover images
Signed-off-by: Deluan <deluan@navidrome.org>
---------
Signed-off-by: Deluan <deluan@navidrome.org>
2026-03-02 07:39:59 -09:00
|
|
|
Expect(mockPlsRepo.Last.UploadedImage).To(Equal("pls-1_my_playlist.jpg"))
|
|
|
|
|
absPath := filepath.Join(tmpDir, "artwork", "playlist", "pls-1_my_playlist.jpg")
|
2026-03-01 10:07:18 -09:00
|
|
|
data, err := os.ReadFile(absPath)
|
|
|
|
|
Expect(err).ToNot(HaveOccurred())
|
|
|
|
|
Expect(string(data)).To(Equal("fake image data"))
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
It("removes old image when replacing", func() {
|
|
|
|
|
ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false})
|
|
|
|
|
|
|
|
|
|
// Upload first image
|
|
|
|
|
err := ps.SetImage(ctx, "pls-1", strings.NewReader("first"), ".png")
|
|
|
|
|
Expect(err).ToNot(HaveOccurred())
|
feat(playlist): support #EXTALBUMARTURL directive and sidecar images (#5131)
* feat(playlist): add migration for playlist image field rename and external URL
* refactor(playlist): rename ImageFile to UploadedImage and ArtworkPath to UploadedImagePath
Rename playlist model fields and methods for clarity in preparation for
adding external image URL and sidecar image support. Add the new
ExternalImageURL field to the Playlist model.
* feat(playlist): parse #EXTALBUMARTURL directive in M3U imports
* feat(playlist): always sync ExternalImageURL on re-scan, preserve UploadedImage
* feat(artwork): add sidecar image discovery and cache invalidation for playlists
Add playlist sidecar image support to the artwork reader fallback chain.
A sidecar image (e.g., MyPlaylist.jpg next to MyPlaylist.m3u) is discovered
via case-insensitive base name matching using model.IsImageFile(). Cache
invalidation uses max(playlist.UpdatedAt, imageFile.ModTime()) to bust
stale artwork when sidecar or ExternalImageURL local files change.
* feat(artwork): add external image URL source to playlist artwork reader
Add fromPlaylistExternalImage source function that resolves playlist
cover art from ExternalImageURL, supporting both HTTP(S) URLs (via
the existing fromURL helper) and local file paths (via os.Open).
Insert it in the Reader() fallback chain between sidecar and tiled cover.
* refactor(artwork): simplify playlist artwork source functions
Extract shared fromLocalFile helper, use url.Parse for scheme check,
and collapse sidecar directory scan conditions.
* test(artwork): remove redundant fromPlaylistSidecar tests
These tests duplicated scenarios already covered by findPlaylistSidecarPath
tests combined with fromLocalFile (tested via fromPlaylistExternalImage).
After refactoring fromPlaylistSidecar to a one-liner composing those two
functions, the wrapper tests add no value.
* fix(playlist): address security review comments from PR #5131:
- Use url.PathUnescape instead of url.QueryUnescape for file:// URLs so
that '+' in filenames is preserved (not decoded as space).
- Validate all local image paths (file://, absolute, relative) against
known library boundaries via libraryMatcher, rejecting paths outside
any configured library.
- Harden #EXTALBUMARTURL against path traversal and SSRF by adding EnableM3UExternalAlbumArt config flag (default false, also
disabled by EnableExternalServices=false) to gate HTTP(S) URL storage
at parse time and fetching at read time (defense in depth).
- Log a warning when os.ReadDir fails in findPlaylistSidecarPath for
diagnosability.
- Extract resolveLocalPath helper to simplify resolveImageURL.
Signed-off-by: Deluan <deluan@navidrome.org>
* feat(playlist): implement human-friendly filename generation for uploaded playlist cover images
Signed-off-by: Deluan <deluan@navidrome.org>
---------
Signed-off-by: Deluan <deluan@navidrome.org>
2026-03-02 07:39:59 -09:00
|
|
|
oldPath := filepath.Join(tmpDir, "artwork", "playlist", "pls-1_my_playlist.png")
|
2026-03-01 10:07:18 -09:00
|
|
|
Expect(oldPath).To(BeAnExistingFile())
|
|
|
|
|
|
|
|
|
|
// Upload replacement image
|
|
|
|
|
err = ps.SetImage(ctx, "pls-1", strings.NewReader("second"), ".jpg")
|
|
|
|
|
Expect(err).ToNot(HaveOccurred())
|
|
|
|
|
Expect(oldPath).ToNot(BeAnExistingFile())
|
feat(playlist): support #EXTALBUMARTURL directive and sidecar images (#5131)
* feat(playlist): add migration for playlist image field rename and external URL
* refactor(playlist): rename ImageFile to UploadedImage and ArtworkPath to UploadedImagePath
Rename playlist model fields and methods for clarity in preparation for
adding external image URL and sidecar image support. Add the new
ExternalImageURL field to the Playlist model.
* feat(playlist): parse #EXTALBUMARTURL directive in M3U imports
* feat(playlist): always sync ExternalImageURL on re-scan, preserve UploadedImage
* feat(artwork): add sidecar image discovery and cache invalidation for playlists
Add playlist sidecar image support to the artwork reader fallback chain.
A sidecar image (e.g., MyPlaylist.jpg next to MyPlaylist.m3u) is discovered
via case-insensitive base name matching using model.IsImageFile(). Cache
invalidation uses max(playlist.UpdatedAt, imageFile.ModTime()) to bust
stale artwork when sidecar or ExternalImageURL local files change.
* feat(artwork): add external image URL source to playlist artwork reader
Add fromPlaylistExternalImage source function that resolves playlist
cover art from ExternalImageURL, supporting both HTTP(S) URLs (via
the existing fromURL helper) and local file paths (via os.Open).
Insert it in the Reader() fallback chain between sidecar and tiled cover.
* refactor(artwork): simplify playlist artwork source functions
Extract shared fromLocalFile helper, use url.Parse for scheme check,
and collapse sidecar directory scan conditions.
* test(artwork): remove redundant fromPlaylistSidecar tests
These tests duplicated scenarios already covered by findPlaylistSidecarPath
tests combined with fromLocalFile (tested via fromPlaylistExternalImage).
After refactoring fromPlaylistSidecar to a one-liner composing those two
functions, the wrapper tests add no value.
* fix(playlist): address security review comments from PR #5131:
- Use url.PathUnescape instead of url.QueryUnescape for file:// URLs so
that '+' in filenames is preserved (not decoded as space).
- Validate all local image paths (file://, absolute, relative) against
known library boundaries via libraryMatcher, rejecting paths outside
any configured library.
- Harden #EXTALBUMARTURL against path traversal and SSRF by adding EnableM3UExternalAlbumArt config flag (default false, also
disabled by EnableExternalServices=false) to gate HTTP(S) URL storage
at parse time and fetching at read time (defense in depth).
- Log a warning when os.ReadDir fails in findPlaylistSidecarPath for
diagnosability.
- Extract resolveLocalPath helper to simplify resolveImageURL.
Signed-off-by: Deluan <deluan@navidrome.org>
* feat(playlist): implement human-friendly filename generation for uploaded playlist cover images
Signed-off-by: Deluan <deluan@navidrome.org>
---------
Signed-off-by: Deluan <deluan@navidrome.org>
2026-03-02 07:39:59 -09:00
|
|
|
newPath := filepath.Join(tmpDir, "artwork", "playlist", "pls-1_my_playlist.jpg")
|
2026-03-01 10:07:18 -09:00
|
|
|
Expect(newPath).To(BeAnExistingFile())
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
It("allows admin to set image on any playlist", func() {
|
|
|
|
|
ctx = request.WithUser(ctx, model.User{ID: "admin-1", IsAdmin: true})
|
|
|
|
|
err := ps.SetImage(ctx, "pls-other", strings.NewReader("data"), ".jpg")
|
|
|
|
|
Expect(err).ToNot(HaveOccurred())
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
It("denies non-owner", func() {
|
|
|
|
|
ctx = request.WithUser(ctx, model.User{ID: "other-user", IsAdmin: false})
|
|
|
|
|
err := ps.SetImage(ctx, "pls-1", strings.NewReader("data"), ".jpg")
|
|
|
|
|
Expect(err).To(MatchError(model.ErrNotAuthorized))
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
It("returns error when playlist not found", func() {
|
|
|
|
|
ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false})
|
|
|
|
|
err := ps.SetImage(ctx, "nonexistent", strings.NewReader("data"), ".jpg")
|
|
|
|
|
Expect(err).To(Equal(model.ErrNotFound))
|
|
|
|
|
})
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
Describe("RemoveImage", func() {
|
|
|
|
|
var tmpDir string
|
|
|
|
|
|
|
|
|
|
BeforeEach(func() {
|
|
|
|
|
DeferCleanup(configtest.SetupConfig())
|
|
|
|
|
tmpDir = GinkgoT().TempDir()
|
refactor(conf): replace eager dir creation with lazy Dir type (#5495)
* feat(conf): add Dir type with lazy directory creation
Introduces the Dir type that wraps a directory path string and defers
os.MkdirAll until the first call to Path() or MustPath(), using sync.Once
to ensure the creation happens exactly once. Implements fmt.Stringer,
encoding.TextMarshaler, and encoding.TextUnmarshaler for config integration.
Includes Ginkgo/Gomega tests covering all methods and error paths.
* refactor(conf): replace eager dir creation with lazy Dir type
Change DataFolder, CacheFolder, Plugins.Folder, and Backup.Path from
string to Dir. Remove all os.MkdirAll calls from Load() so directories
are created lazily on first Path()/MustPath() call. Artwork folder
creation was already handled at point-of-use in image_upload.go.
Add SnapshotConfig() to conf package for safe test config save/restore
that avoids copying sync.Once inside Dir fields. Fix copy-lock vet
warning in nativeapi/config.go by marshalling pointer instead of value.
* refactor(conf): migrate tests and db init to lazy Dir type
Update all test files to use conf.NewDir() for Dir field assignments.
Ensure DataFolder is created lazily when the database is first opened
in db.Db(). Remove eager directory creation from conf.Load() tests.
* fix(conf): address review findings for Dir type
- Use os.ModePerm for DataFolder/CacheFolder (was 0700, should match
original behavior). Add NewDirWithPerm for PluginsFolder (0700).
- Use Path() instead of MustPath() in db.Prune() to avoid logFatal
from background cron job.
- Panic on marshal/unmarshal errors in SnapshotConfig (test helper).
- Clean up redundant String()/MustPath() calls in plugin manager.
- Remove dead code in dir_test.go.
Signed-off-by: Deluan <deluan@navidrome.org>
* fix(conf): add GoString to Dir for clean config dump output
Implement fmt.GoStringer on Dir so pretty.Sprintf shows the path
string instead of internal struct fields (sync.Once, perm, err).
Also add TODO comment to configtest about removing the indirection.
* fix(dir): improve error logging in MustPath method
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor(tests): remove redundant tests for unwritable DataFolder and CacheFolder
Signed-off-by: Deluan <deluan@navidrome.org>
* fix(conf): address PR review feedback
- Ensure Plugins.Folder always uses 0700, even when user-configured
(previously only the derived default got restrictive permissions).
- Create LogFile parent directory before opening, so LogFile paths
inside a not-yet-created DataFolder work correctly.
---------
Signed-off-by: Deluan <deluan@navidrome.org>
2026-05-13 12:44:22 -08:00
|
|
|
conf.Server.DataFolder = conf.NewDir(tmpDir)
|
2026-03-01 10:07:18 -09:00
|
|
|
|
|
|
|
|
// Create a real image file on disk
|
|
|
|
|
imgDir := filepath.Join(tmpDir, "artwork", "playlist")
|
|
|
|
|
Expect(os.MkdirAll(imgDir, 0755)).To(Succeed())
|
|
|
|
|
Expect(os.WriteFile(filepath.Join(imgDir, "pls-1.jpg"), []byte("img data"), 0600)).To(Succeed())
|
|
|
|
|
|
|
|
|
|
mockPlsRepo.Data = map[string]*model.Playlist{
|
feat(playlist): support #EXTALBUMARTURL directive and sidecar images (#5131)
* feat(playlist): add migration for playlist image field rename and external URL
* refactor(playlist): rename ImageFile to UploadedImage and ArtworkPath to UploadedImagePath
Rename playlist model fields and methods for clarity in preparation for
adding external image URL and sidecar image support. Add the new
ExternalImageURL field to the Playlist model.
* feat(playlist): parse #EXTALBUMARTURL directive in M3U imports
* feat(playlist): always sync ExternalImageURL on re-scan, preserve UploadedImage
* feat(artwork): add sidecar image discovery and cache invalidation for playlists
Add playlist sidecar image support to the artwork reader fallback chain.
A sidecar image (e.g., MyPlaylist.jpg next to MyPlaylist.m3u) is discovered
via case-insensitive base name matching using model.IsImageFile(). Cache
invalidation uses max(playlist.UpdatedAt, imageFile.ModTime()) to bust
stale artwork when sidecar or ExternalImageURL local files change.
* feat(artwork): add external image URL source to playlist artwork reader
Add fromPlaylistExternalImage source function that resolves playlist
cover art from ExternalImageURL, supporting both HTTP(S) URLs (via
the existing fromURL helper) and local file paths (via os.Open).
Insert it in the Reader() fallback chain between sidecar and tiled cover.
* refactor(artwork): simplify playlist artwork source functions
Extract shared fromLocalFile helper, use url.Parse for scheme check,
and collapse sidecar directory scan conditions.
* test(artwork): remove redundant fromPlaylistSidecar tests
These tests duplicated scenarios already covered by findPlaylistSidecarPath
tests combined with fromLocalFile (tested via fromPlaylistExternalImage).
After refactoring fromPlaylistSidecar to a one-liner composing those two
functions, the wrapper tests add no value.
* fix(playlist): address security review comments from PR #5131:
- Use url.PathUnescape instead of url.QueryUnescape for file:// URLs so
that '+' in filenames is preserved (not decoded as space).
- Validate all local image paths (file://, absolute, relative) against
known library boundaries via libraryMatcher, rejecting paths outside
any configured library.
- Harden #EXTALBUMARTURL against path traversal and SSRF by adding EnableM3UExternalAlbumArt config flag (default false, also
disabled by EnableExternalServices=false) to gate HTTP(S) URL storage
at parse time and fetching at read time (defense in depth).
- Log a warning when os.ReadDir fails in findPlaylistSidecarPath for
diagnosability.
- Extract resolveLocalPath helper to simplify resolveImageURL.
Signed-off-by: Deluan <deluan@navidrome.org>
* feat(playlist): implement human-friendly filename generation for uploaded playlist cover images
Signed-off-by: Deluan <deluan@navidrome.org>
---------
Signed-off-by: Deluan <deluan@navidrome.org>
2026-03-02 07:39:59 -09:00
|
|
|
"pls-1": {ID: "pls-1", Name: "My Playlist", OwnerID: "user-1", UploadedImage: "pls-1.jpg"},
|
2026-03-01 10:07:18 -09:00
|
|
|
"pls-empty": {ID: "pls-empty", Name: "No Cover", OwnerID: "user-1"},
|
|
|
|
|
"pls-other": {ID: "pls-other", Name: "Other's", OwnerID: "other-user"},
|
|
|
|
|
}
|
2026-03-15 18:19:55 -08:00
|
|
|
ps = playlists.NewPlaylists(ds, core.NewImageUploadService())
|
2026-03-01 10:07:18 -09:00
|
|
|
})
|
|
|
|
|
|
feat(playlist): support #EXTALBUMARTURL directive and sidecar images (#5131)
* feat(playlist): add migration for playlist image field rename and external URL
* refactor(playlist): rename ImageFile to UploadedImage and ArtworkPath to UploadedImagePath
Rename playlist model fields and methods for clarity in preparation for
adding external image URL and sidecar image support. Add the new
ExternalImageURL field to the Playlist model.
* feat(playlist): parse #EXTALBUMARTURL directive in M3U imports
* feat(playlist): always sync ExternalImageURL on re-scan, preserve UploadedImage
* feat(artwork): add sidecar image discovery and cache invalidation for playlists
Add playlist sidecar image support to the artwork reader fallback chain.
A sidecar image (e.g., MyPlaylist.jpg next to MyPlaylist.m3u) is discovered
via case-insensitive base name matching using model.IsImageFile(). Cache
invalidation uses max(playlist.UpdatedAt, imageFile.ModTime()) to bust
stale artwork when sidecar or ExternalImageURL local files change.
* feat(artwork): add external image URL source to playlist artwork reader
Add fromPlaylistExternalImage source function that resolves playlist
cover art from ExternalImageURL, supporting both HTTP(S) URLs (via
the existing fromURL helper) and local file paths (via os.Open).
Insert it in the Reader() fallback chain between sidecar and tiled cover.
* refactor(artwork): simplify playlist artwork source functions
Extract shared fromLocalFile helper, use url.Parse for scheme check,
and collapse sidecar directory scan conditions.
* test(artwork): remove redundant fromPlaylistSidecar tests
These tests duplicated scenarios already covered by findPlaylistSidecarPath
tests combined with fromLocalFile (tested via fromPlaylistExternalImage).
After refactoring fromPlaylistSidecar to a one-liner composing those two
functions, the wrapper tests add no value.
* fix(playlist): address security review comments from PR #5131:
- Use url.PathUnescape instead of url.QueryUnescape for file:// URLs so
that '+' in filenames is preserved (not decoded as space).
- Validate all local image paths (file://, absolute, relative) against
known library boundaries via libraryMatcher, rejecting paths outside
any configured library.
- Harden #EXTALBUMARTURL against path traversal and SSRF by adding EnableM3UExternalAlbumArt config flag (default false, also
disabled by EnableExternalServices=false) to gate HTTP(S) URL storage
at parse time and fetching at read time (defense in depth).
- Log a warning when os.ReadDir fails in findPlaylistSidecarPath for
diagnosability.
- Extract resolveLocalPath helper to simplify resolveImageURL.
Signed-off-by: Deluan <deluan@navidrome.org>
* feat(playlist): implement human-friendly filename generation for uploaded playlist cover images
Signed-off-by: Deluan <deluan@navidrome.org>
---------
Signed-off-by: Deluan <deluan@navidrome.org>
2026-03-02 07:39:59 -09:00
|
|
|
It("removes file and clears UploadedImage", func() {
|
2026-03-01 10:07:18 -09:00
|
|
|
ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false})
|
|
|
|
|
err := ps.RemoveImage(ctx, "pls-1")
|
|
|
|
|
Expect(err).ToNot(HaveOccurred())
|
|
|
|
|
|
feat(playlist): support #EXTALBUMARTURL directive and sidecar images (#5131)
* feat(playlist): add migration for playlist image field rename and external URL
* refactor(playlist): rename ImageFile to UploadedImage and ArtworkPath to UploadedImagePath
Rename playlist model fields and methods for clarity in preparation for
adding external image URL and sidecar image support. Add the new
ExternalImageURL field to the Playlist model.
* feat(playlist): parse #EXTALBUMARTURL directive in M3U imports
* feat(playlist): always sync ExternalImageURL on re-scan, preserve UploadedImage
* feat(artwork): add sidecar image discovery and cache invalidation for playlists
Add playlist sidecar image support to the artwork reader fallback chain.
A sidecar image (e.g., MyPlaylist.jpg next to MyPlaylist.m3u) is discovered
via case-insensitive base name matching using model.IsImageFile(). Cache
invalidation uses max(playlist.UpdatedAt, imageFile.ModTime()) to bust
stale artwork when sidecar or ExternalImageURL local files change.
* feat(artwork): add external image URL source to playlist artwork reader
Add fromPlaylistExternalImage source function that resolves playlist
cover art from ExternalImageURL, supporting both HTTP(S) URLs (via
the existing fromURL helper) and local file paths (via os.Open).
Insert it in the Reader() fallback chain between sidecar and tiled cover.
* refactor(artwork): simplify playlist artwork source functions
Extract shared fromLocalFile helper, use url.Parse for scheme check,
and collapse sidecar directory scan conditions.
* test(artwork): remove redundant fromPlaylistSidecar tests
These tests duplicated scenarios already covered by findPlaylistSidecarPath
tests combined with fromLocalFile (tested via fromPlaylistExternalImage).
After refactoring fromPlaylistSidecar to a one-liner composing those two
functions, the wrapper tests add no value.
* fix(playlist): address security review comments from PR #5131:
- Use url.PathUnescape instead of url.QueryUnescape for file:// URLs so
that '+' in filenames is preserved (not decoded as space).
- Validate all local image paths (file://, absolute, relative) against
known library boundaries via libraryMatcher, rejecting paths outside
any configured library.
- Harden #EXTALBUMARTURL against path traversal and SSRF by adding EnableM3UExternalAlbumArt config flag (default false, also
disabled by EnableExternalServices=false) to gate HTTP(S) URL storage
at parse time and fetching at read time (defense in depth).
- Log a warning when os.ReadDir fails in findPlaylistSidecarPath for
diagnosability.
- Extract resolveLocalPath helper to simplify resolveImageURL.
Signed-off-by: Deluan <deluan@navidrome.org>
* feat(playlist): implement human-friendly filename generation for uploaded playlist cover images
Signed-off-by: Deluan <deluan@navidrome.org>
---------
Signed-off-by: Deluan <deluan@navidrome.org>
2026-03-02 07:39:59 -09:00
|
|
|
Expect(mockPlsRepo.Last.UploadedImage).To(BeEmpty())
|
2026-03-01 10:07:18 -09:00
|
|
|
absPath := filepath.Join(tmpDir, "artwork", "playlist", "pls-1.jpg")
|
|
|
|
|
Expect(absPath).ToNot(BeAnExistingFile())
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
It("succeeds even if playlist has no image", func() {
|
|
|
|
|
ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false})
|
|
|
|
|
err := ps.RemoveImage(ctx, "pls-empty")
|
|
|
|
|
Expect(err).ToNot(HaveOccurred())
|
feat(playlist): support #EXTALBUMARTURL directive and sidecar images (#5131)
* feat(playlist): add migration for playlist image field rename and external URL
* refactor(playlist): rename ImageFile to UploadedImage and ArtworkPath to UploadedImagePath
Rename playlist model fields and methods for clarity in preparation for
adding external image URL and sidecar image support. Add the new
ExternalImageURL field to the Playlist model.
* feat(playlist): parse #EXTALBUMARTURL directive in M3U imports
* feat(playlist): always sync ExternalImageURL on re-scan, preserve UploadedImage
* feat(artwork): add sidecar image discovery and cache invalidation for playlists
Add playlist sidecar image support to the artwork reader fallback chain.
A sidecar image (e.g., MyPlaylist.jpg next to MyPlaylist.m3u) is discovered
via case-insensitive base name matching using model.IsImageFile(). Cache
invalidation uses max(playlist.UpdatedAt, imageFile.ModTime()) to bust
stale artwork when sidecar or ExternalImageURL local files change.
* feat(artwork): add external image URL source to playlist artwork reader
Add fromPlaylistExternalImage source function that resolves playlist
cover art from ExternalImageURL, supporting both HTTP(S) URLs (via
the existing fromURL helper) and local file paths (via os.Open).
Insert it in the Reader() fallback chain between sidecar and tiled cover.
* refactor(artwork): simplify playlist artwork source functions
Extract shared fromLocalFile helper, use url.Parse for scheme check,
and collapse sidecar directory scan conditions.
* test(artwork): remove redundant fromPlaylistSidecar tests
These tests duplicated scenarios already covered by findPlaylistSidecarPath
tests combined with fromLocalFile (tested via fromPlaylistExternalImage).
After refactoring fromPlaylistSidecar to a one-liner composing those two
functions, the wrapper tests add no value.
* fix(playlist): address security review comments from PR #5131:
- Use url.PathUnescape instead of url.QueryUnescape for file:// URLs so
that '+' in filenames is preserved (not decoded as space).
- Validate all local image paths (file://, absolute, relative) against
known library boundaries via libraryMatcher, rejecting paths outside
any configured library.
- Harden #EXTALBUMARTURL against path traversal and SSRF by adding EnableM3UExternalAlbumArt config flag (default false, also
disabled by EnableExternalServices=false) to gate HTTP(S) URL storage
at parse time and fetching at read time (defense in depth).
- Log a warning when os.ReadDir fails in findPlaylistSidecarPath for
diagnosability.
- Extract resolveLocalPath helper to simplify resolveImageURL.
Signed-off-by: Deluan <deluan@navidrome.org>
* feat(playlist): implement human-friendly filename generation for uploaded playlist cover images
Signed-off-by: Deluan <deluan@navidrome.org>
---------
Signed-off-by: Deluan <deluan@navidrome.org>
2026-03-02 07:39:59 -09:00
|
|
|
Expect(mockPlsRepo.Last.UploadedImage).To(BeEmpty())
|
2026-03-01 10:07:18 -09:00
|
|
|
})
|
|
|
|
|
|
|
|
|
|
It("denies non-owner", func() {
|
|
|
|
|
ctx = request.WithUser(ctx, model.User{ID: "other-user", IsAdmin: false})
|
|
|
|
|
err := ps.RemoveImage(ctx, "pls-1")
|
|
|
|
|
Expect(err).To(MatchError(model.ErrNotAuthorized))
|
|
|
|
|
})
|
|
|
|
|
|
|
|
|
|
It("returns error when playlist not found", func() {
|
|
|
|
|
ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false})
|
|
|
|
|
err := ps.RemoveImage(ctx, "nonexistent")
|
|
|
|
|
Expect(err).To(Equal(model.ErrNotFound))
|
|
|
|
|
})
|
|
|
|
|
})
|
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
|
|
|
})
|