diff --git a/cmd/scan.go b/cmd/scan.go index daf58b29..ffb77b10 100644 --- a/cmd/scan.go +++ b/cmd/scan.go @@ -8,7 +8,7 @@ import ( "os" "strings" - "github.com/navidrome/navidrome/core" + "github.com/navidrome/navidrome/core/playlists" "github.com/navidrome/navidrome/db" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" @@ -74,7 +74,7 @@ func runScanner(ctx context.Context) { sqlDB := db.Db() defer db.Db().Close() ds := persistence.New(sqlDB) - pls := core.NewPlaylists(ds) + pls := playlists.NewPlaylists(ds) // Parse targets from command line or file var scanTargets []model.ScanTarget diff --git a/cmd/wire_gen.go b/cmd/wire_gen.go index 7a9d38d9..204d90ba 100644 --- a/cmd/wire_gen.go +++ b/cmd/wire_gen.go @@ -18,6 +18,7 @@ import ( "github.com/navidrome/navidrome/core/ffmpeg" "github.com/navidrome/navidrome/core/metrics" "github.com/navidrome/navidrome/core/playback" + "github.com/navidrome/navidrome/core/playlists" "github.com/navidrome/navidrome/core/scrobbler" "github.com/navidrome/navidrome/db" "github.com/navidrome/navidrome/model" @@ -61,7 +62,7 @@ func CreateNativeAPIRouter(ctx context.Context) *nativeapi.Router { sqlDB := db.Db() dataStore := persistence.New(sqlDB) share := core.NewShare(dataStore) - playlists := core.NewPlaylists(dataStore) + playlistsPlaylists := playlists.NewPlaylists(dataStore) insights := metrics.GetInstance(dataStore) fileCache := artwork.GetImageCache() fFmpeg := ffmpeg.New() @@ -72,12 +73,12 @@ func CreateNativeAPIRouter(ctx context.Context) *nativeapi.Router { provider := external.NewProvider(dataStore, agentsAgents) artworkArtwork := artwork.NewArtwork(dataStore, fileCache, fFmpeg, provider) cacheWarmer := artwork.NewCacheWarmer(artworkArtwork, fileCache) - modelScanner := scanner.New(ctx, dataStore, cacheWarmer, broker, playlists, metricsMetrics) + modelScanner := scanner.New(ctx, dataStore, cacheWarmer, broker, playlistsPlaylists, metricsMetrics) watcher := scanner.GetWatcher(dataStore, modelScanner) library := core.NewLibrary(dataStore, modelScanner, watcher, broker, manager) user := core.NewUser(dataStore, manager) maintenance := core.NewMaintenance(dataStore) - router := nativeapi.New(dataStore, share, playlists, insights, library, user, maintenance, manager) + router := nativeapi.New(dataStore, share, playlistsPlaylists, insights, library, user, maintenance, manager) return router } @@ -98,11 +99,11 @@ func CreateSubsonicAPIRouter(ctx context.Context) *subsonic.Router { archiver := core.NewArchiver(mediaStreamer, dataStore, share) players := core.NewPlayers(dataStore) cacheWarmer := artwork.NewCacheWarmer(artworkArtwork, fileCache) - playlists := core.NewPlaylists(dataStore) - modelScanner := scanner.New(ctx, dataStore, cacheWarmer, broker, playlists, metricsMetrics) + playlistsPlaylists := playlists.NewPlaylists(dataStore) + modelScanner := scanner.New(ctx, dataStore, cacheWarmer, broker, playlistsPlaylists, metricsMetrics) playTracker := scrobbler.GetPlayTracker(dataStore, broker, manager) playbackServer := playback.GetInstance(dataStore) - router := subsonic.New(dataStore, artworkArtwork, mediaStreamer, archiver, players, provider, modelScanner, broker, playlists, playTracker, share, playbackServer, metricsMetrics) + router := subsonic.New(dataStore, artworkArtwork, mediaStreamer, archiver, players, provider, modelScanner, broker, playlistsPlaylists, playTracker, share, playbackServer, metricsMetrics) return router } @@ -165,8 +166,8 @@ func CreateScanner(ctx context.Context) model.Scanner { provider := external.NewProvider(dataStore, agentsAgents) artworkArtwork := artwork.NewArtwork(dataStore, fileCache, fFmpeg, provider) cacheWarmer := artwork.NewCacheWarmer(artworkArtwork, fileCache) - playlists := core.NewPlaylists(dataStore) - modelScanner := scanner.New(ctx, dataStore, cacheWarmer, broker, playlists, metricsMetrics) + playlistsPlaylists := playlists.NewPlaylists(dataStore) + modelScanner := scanner.New(ctx, dataStore, cacheWarmer, broker, playlistsPlaylists, metricsMetrics) return modelScanner } @@ -182,8 +183,8 @@ func CreateScanWatcher(ctx context.Context) scanner.Watcher { provider := external.NewProvider(dataStore, agentsAgents) artworkArtwork := artwork.NewArtwork(dataStore, fileCache, fFmpeg, provider) cacheWarmer := artwork.NewCacheWarmer(artworkArtwork, fileCache) - playlists := core.NewPlaylists(dataStore) - modelScanner := scanner.New(ctx, dataStore, cacheWarmer, broker, playlists, metricsMetrics) + playlistsPlaylists := playlists.NewPlaylists(dataStore) + modelScanner := scanner.New(ctx, dataStore, cacheWarmer, broker, playlistsPlaylists, metricsMetrics) watcher := scanner.GetWatcher(dataStore, modelScanner) return watcher } diff --git a/core/playlists/import.go b/core/playlists/import.go new file mode 100644 index 00000000..40e23052 --- /dev/null +++ b/core/playlists/import.go @@ -0,0 +1,119 @@ +package playlists + +import ( + "context" + "errors" + "io" + "os" + "path/filepath" + "strings" + "time" + + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/log" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/request" + "github.com/navidrome/navidrome/utils/ioutils" + "golang.org/x/text/unicode/norm" +) + +func (s *playlists) ImportFile(ctx context.Context, folder *model.Folder, filename string) (*model.Playlist, error) { + pls, err := s.parsePlaylist(ctx, filename, folder) + if err != nil { + log.Error(ctx, "Error parsing playlist", "path", filepath.Join(folder.AbsolutePath(), filename), err) + return nil, err + } + log.Debug(ctx, "Found playlist", "name", pls.Name, "lastUpdated", pls.UpdatedAt, "path", pls.Path, "numTracks", len(pls.Tracks)) + err = s.updatePlaylist(ctx, pls) + if err != nil { + log.Error(ctx, "Error updating playlist", "path", filepath.Join(folder.AbsolutePath(), filename), err) + } + return pls, err +} + +func (s *playlists) ImportM3U(ctx context.Context, reader io.Reader) (*model.Playlist, error) { + owner, _ := request.UserFrom(ctx) + pls := &model.Playlist{ + OwnerID: owner.ID, + Public: false, + Sync: false, + } + err := s.parseM3U(ctx, pls, nil, reader) + if err != nil { + log.Error(ctx, "Error parsing playlist", err) + return nil, err + } + err = s.ds.Playlist(ctx).Put(pls) + if err != nil { + log.Error(ctx, "Error saving playlist", err) + return nil, err + } + return pls, nil +} + +func (s *playlists) parsePlaylist(ctx context.Context, playlistFile string, folder *model.Folder) (*model.Playlist, error) { + pls, err := s.newSyncedPlaylist(folder.AbsolutePath(), playlistFile) + if err != nil { + return nil, err + } + + file, err := os.Open(pls.Path) + if err != nil { + return nil, err + } + defer file.Close() + + reader := ioutils.UTF8Reader(file) + extension := strings.ToLower(filepath.Ext(playlistFile)) + switch extension { + case ".nsp": + err = s.parseNSP(ctx, pls, reader) + default: + err = s.parseM3U(ctx, pls, folder, reader) + } + return pls, err +} + +func (s *playlists) updatePlaylist(ctx context.Context, newPls *model.Playlist) error { + owner, _ := request.UserFrom(ctx) + + // Try to find existing playlist by path. Since filesystem normalization differs across + // platforms (macOS uses NFD, Linux/Windows use NFC), we try both forms to match + // playlists that may have been imported on a different platform. + pls, err := s.ds.Playlist(ctx).FindByPath(newPls.Path) + if errors.Is(err, model.ErrNotFound) { + // Try alternate normalization form + altPath := norm.NFD.String(newPls.Path) + if altPath == newPls.Path { + altPath = norm.NFC.String(newPls.Path) + } + if altPath != newPls.Path { + pls, err = s.ds.Playlist(ctx).FindByPath(altPath) + } + } + if err != nil && !errors.Is(err, model.ErrNotFound) { + return err + } + if err == nil && !pls.Sync { + log.Debug(ctx, "Playlist already imported and not synced", "playlist", pls.Name, "path", pls.Path) + return nil + } + + if err == nil { + log.Info(ctx, "Updating synced playlist", "playlist", pls.Name, "path", newPls.Path) + newPls.ID = pls.ID + newPls.Name = pls.Name + newPls.Comment = pls.Comment + newPls.OwnerID = pls.OwnerID + newPls.Public = pls.Public + newPls.EvaluatedAt = &time.Time{} + } else { + log.Info(ctx, "Adding synced playlist", "playlist", newPls.Name, "path", newPls.Path, "owner", owner.UserName) + newPls.OwnerID = owner.ID + // For NSP files, Public may already be set from the file; for M3U, use server default + if !newPls.IsSmartPlaylist() { + newPls.Public = conf.Server.DefaultPlaylistPublicVisibility + } + } + return s.ds.Playlist(ctx).Put(newPls) +} diff --git a/core/playlists_test.go b/core/playlists/import_test.go similarity index 94% rename from core/playlists_test.go rename to core/playlists/import_test.go index 7712a268..a42c3f3e 100644 --- a/core/playlists_test.go +++ b/core/playlists/import_test.go @@ -1,4 +1,4 @@ -package core_test +package playlists_test import ( "context" @@ -9,7 +9,7 @@ import ( "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/conf/configtest" - "github.com/navidrome/navidrome/core" + "github.com/navidrome/navidrome/core/playlists" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/criteria" "github.com/navidrome/navidrome/model/request" @@ -19,18 +19,18 @@ import ( "golang.org/x/text/unicode/norm" ) -var _ = Describe("Playlists", func() { +var _ = Describe("Playlists - Import", func() { var ds *tests.MockDataStore - var ps core.Playlists - var mockPlsRepo mockedPlaylistRepo + var ps playlists.Playlists + var mockPlsRepo *tests.MockPlaylistRepo var mockLibRepo *tests.MockLibraryRepo ctx := context.Background() BeforeEach(func() { - mockPlsRepo = mockedPlaylistRepo{} + mockPlsRepo = tests.CreateMockPlaylistRepo() mockLibRepo = &tests.MockLibraryRepo{} ds = &tests.MockDataStore{ - MockedPlaylist: &mockPlsRepo, + MockedPlaylist: mockPlsRepo, MockedLibrary: mockLibRepo, } ctx = request.WithUser(ctx, model.User{ID: "123"}) @@ -39,7 +39,7 @@ var _ = Describe("Playlists", func() { Describe("ImportFile", func() { var folder *model.Folder BeforeEach(func() { - ps = core.NewPlaylists(ds) + ps = playlists.NewPlaylists(ds) ds.MockedMediaFile = &mockedMediaFileRepo{} libPath, _ := os.Getwd() // Set up library with the actual library path that matches the folder @@ -61,7 +61,7 @@ var _ = Describe("Playlists", func() { Expect(pls.Tracks).To(HaveLen(2)) Expect(pls.Tracks[0].Path).To(Equal("tests/fixtures/playlists/test.mp3")) Expect(pls.Tracks[1].Path).To(Equal("tests/fixtures/playlists/test.ogg")) - Expect(mockPlsRepo.last).To(Equal(pls)) + Expect(mockPlsRepo.Last).To(Equal(pls)) }) It("parses playlists using LF ending", func() { @@ -99,7 +99,7 @@ var _ = Describe("Playlists", func() { It("parses well-formed playlists", func() { pls, err := ps.ImportFile(ctx, folder, "recently_played.nsp") Expect(err).ToNot(HaveOccurred()) - Expect(mockPlsRepo.last).To(Equal(pls)) + Expect(mockPlsRepo.Last).To(Equal(pls)) Expect(pls.OwnerID).To(Equal("123")) Expect(pls.Name).To(Equal("Recently Played")) Expect(pls.Comment).To(Equal("Recently played tracks")) @@ -149,7 +149,7 @@ var _ = Describe("Playlists", func() { tmpDir := GinkgoT().TempDir() mockLibRepo.SetData([]model.Library{{ID: 1, Path: tmpDir}}) ds.MockedMediaFile = &mockedMediaFileFromListRepo{data: []string{}} - ps = core.NewPlaylists(ds) + ps = playlists.NewPlaylists(ds) // Create the playlist file on disk with the filesystem's normalization form plsFile := tmpDir + "/" + filesystemName + ".m3u" @@ -163,7 +163,7 @@ var _ = Describe("Playlists", func() { Path: storedPath, Sync: true, } - mockPlsRepo.data = map[string]*model.Playlist{storedPath: existingPls} + mockPlsRepo.PathMap = map[string]*model.Playlist{storedPath: existingPls} // Import using the filesystem's normalization form plsFolder := &model.Folder{ @@ -209,7 +209,7 @@ var _ = Describe("Playlists", func() { "def.mp3", // This is playlists/def.mp3 relative to plsDir }, } - ps = core.NewPlaylists(ds) + ps = playlists.NewPlaylists(ds) }) It("handles relative paths that reference files in other libraries", func() { @@ -365,7 +365,7 @@ var _ = Describe("Playlists", func() { }, } // Recreate playlists service to pick up new mock - ps = core.NewPlaylists(ds) + ps = playlists.NewPlaylists(ds) // Create playlist in music library that references both tracks plsContent := "#PLAYLIST:Same Path Test\nalbum/track.mp3\n../classical/album/track.mp3" @@ -408,7 +408,7 @@ var _ = Describe("Playlists", func() { BeforeEach(func() { repo = &mockedMediaFileFromListRepo{} ds.MockedMediaFile = repo - ps = core.NewPlaylists(ds) + ps = playlists.NewPlaylists(ds) mockLibRepo.SetData([]model.Library{{ID: 1, Path: "/music"}, {ID: 2, Path: "/new"}}) ctx = request.WithUser(ctx, model.User{ID: "123"}) }) @@ -439,7 +439,7 @@ var _ = Describe("Playlists", func() { Expect(pls.Tracks[1].Path).To(Equal("tests/test.ogg")) Expect(pls.Tracks[2].Path).To(Equal("downloads/newfile.flac")) Expect(pls.Tracks[3].Path).To(Equal("tests/01 Invisible (RED) Edit Version.mp3")) - Expect(mockPlsRepo.last).To(Equal(pls)) + Expect(mockPlsRepo.Last).To(Equal(pls)) }) It("sets the playlist name as a timestamp if the #PLAYLIST directive is not present", func() { @@ -460,7 +460,7 @@ var _ = Describe("Playlists", func() { Expect(pls.Tracks).To(HaveLen(2)) }) - It("returns only tracks that exist in the database and in the same other as the m3u", func() { + It("returns only tracks that exist in the database and in the same order as the m3u", func() { repo.data = []string{ "album1/test1.mp3", "album2/test2.mp3", @@ -570,7 +570,7 @@ var _ = Describe("Playlists", func() { }) - Describe("InPlaylistsPath", func() { + Describe("InPath", func() { var folder model.Folder BeforeEach(func() { @@ -584,27 +584,27 @@ var _ = Describe("Playlists", func() { It("returns true if PlaylistsPath is empty", func() { conf.Server.PlaylistsPath = "" - Expect(core.InPlaylistsPath(folder)).To(BeTrue()) + Expect(playlists.InPath(folder)).To(BeTrue()) }) It("returns true if PlaylistsPath is any (**/**)", func() { conf.Server.PlaylistsPath = "**/**" - Expect(core.InPlaylistsPath(folder)).To(BeTrue()) + Expect(playlists.InPath(folder)).To(BeTrue()) }) It("returns true if folder is in PlaylistsPath", func() { conf.Server.PlaylistsPath = "other/**:playlists/**" - Expect(core.InPlaylistsPath(folder)).To(BeTrue()) + Expect(playlists.InPath(folder)).To(BeTrue()) }) It("returns false if folder is not in PlaylistsPath", func() { conf.Server.PlaylistsPath = "other" - Expect(core.InPlaylistsPath(folder)).To(BeFalse()) + Expect(playlists.InPath(folder)).To(BeFalse()) }) It("returns true if for a playlist in root of MusicFolder if PlaylistsPath is '.'", func() { conf.Server.PlaylistsPath = "." - Expect(core.InPlaylistsPath(folder)).To(BeFalse()) + Expect(playlists.InPath(folder)).To(BeFalse()) folder2 := model.Folder{ LibraryPath: "/music", @@ -612,7 +612,7 @@ var _ = Describe("Playlists", func() { Name: ".", } - Expect(core.InPlaylistsPath(folder2)).To(BeTrue()) + Expect(playlists.InPath(folder2)).To(BeTrue()) }) }) }) @@ -693,23 +693,3 @@ func (r *mockedMediaFileFromListRepo) FindByPaths(paths []string) (model.MediaFi } return mfs, nil } - -type mockedPlaylistRepo struct { - last *model.Playlist - data map[string]*model.Playlist // keyed by path - model.PlaylistRepository -} - -func (r *mockedPlaylistRepo) FindByPath(path string) (*model.Playlist, error) { - if r.data != nil { - if pls, ok := r.data[path]; ok { - return pls, nil - } - } - return nil, model.ErrNotFound -} - -func (r *mockedPlaylistRepo) Put(pls *model.Playlist) error { - r.last = pls - return nil -} diff --git a/core/playlists.go b/core/playlists/parse_m3u.go similarity index 53% rename from core/playlists.go rename to core/playlists/parse_m3u.go index 1c070a11..4e79d15c 100644 --- a/core/playlists.go +++ b/core/playlists/parse_m3u.go @@ -1,183 +1,28 @@ -package core +package playlists import ( "cmp" "context" - "encoding/json" - "errors" "fmt" "io" "net/url" - "os" "path/filepath" "slices" "strings" "time" - "github.com/RaveNoX/go-jsoncommentstrip" - "github.com/bmatcuk/doublestar/v4" - "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" - "github.com/navidrome/navidrome/model/criteria" - "github.com/navidrome/navidrome/model/request" - "github.com/navidrome/navidrome/utils/ioutils" "github.com/navidrome/navidrome/utils/slice" "golang.org/x/text/unicode/norm" ) -type Playlists interface { - ImportFile(ctx context.Context, folder *model.Folder, filename string) (*model.Playlist, error) - Update(ctx context.Context, playlistID string, name *string, comment *string, public *bool, idsToAdd []string, idxToRemove []int) error - ImportM3U(ctx context.Context, reader io.Reader) (*model.Playlist, error) -} - -type playlists struct { - ds model.DataStore -} - -func NewPlaylists(ds model.DataStore) Playlists { - return &playlists{ds: ds} -} - -func InPlaylistsPath(folder model.Folder) bool { - if conf.Server.PlaylistsPath == "" { - return true - } - rel, _ := filepath.Rel(folder.LibraryPath, folder.AbsolutePath()) - for path := range strings.SplitSeq(conf.Server.PlaylistsPath, string(filepath.ListSeparator)) { - if match, _ := doublestar.Match(path, rel); match { - return true - } - } - return false -} - -func (s *playlists) ImportFile(ctx context.Context, folder *model.Folder, filename string) (*model.Playlist, error) { - pls, err := s.parsePlaylist(ctx, filename, folder) - if err != nil { - log.Error(ctx, "Error parsing playlist", "path", filepath.Join(folder.AbsolutePath(), filename), err) - return nil, err - } - log.Debug("Found playlist", "name", pls.Name, "lastUpdated", pls.UpdatedAt, "path", pls.Path, "numTracks", len(pls.Tracks)) - err = s.updatePlaylist(ctx, pls) - if err != nil { - log.Error(ctx, "Error updating playlist", "path", filepath.Join(folder.AbsolutePath(), filename), err) - } - return pls, err -} - -func (s *playlists) ImportM3U(ctx context.Context, reader io.Reader) (*model.Playlist, error) { - owner, _ := request.UserFrom(ctx) - pls := &model.Playlist{ - OwnerID: owner.ID, - Public: false, - Sync: false, - } - err := s.parseM3U(ctx, pls, nil, reader) - if err != nil { - log.Error(ctx, "Error parsing playlist", err) - return nil, err - } - err = s.ds.Playlist(ctx).Put(pls) - if err != nil { - log.Error(ctx, "Error saving playlist", err) - return nil, err - } - return pls, nil -} - -func (s *playlists) parsePlaylist(ctx context.Context, playlistFile string, folder *model.Folder) (*model.Playlist, error) { - pls, err := s.newSyncedPlaylist(folder.AbsolutePath(), playlistFile) - if err != nil { - return nil, err - } - - file, err := os.Open(pls.Path) - if err != nil { - return nil, err - } - defer file.Close() - - reader := ioutils.UTF8Reader(file) - extension := strings.ToLower(filepath.Ext(playlistFile)) - switch extension { - case ".nsp": - err = s.parseNSP(ctx, pls, reader) - default: - err = s.parseM3U(ctx, pls, folder, reader) - } - return pls, err -} - -func (s *playlists) newSyncedPlaylist(baseDir string, playlistFile string) (*model.Playlist, error) { - playlistPath := filepath.Join(baseDir, playlistFile) - info, err := os.Stat(playlistPath) - if err != nil { - return nil, err - } - - var extension = filepath.Ext(playlistFile) - var name = playlistFile[0 : len(playlistFile)-len(extension)] - - pls := &model.Playlist{ - Name: name, - Comment: fmt.Sprintf("Auto-imported from '%s'", playlistFile), - Public: false, - Path: playlistPath, - Sync: true, - UpdatedAt: info.ModTime(), - } - return pls, nil -} - -func getPositionFromOffset(data []byte, offset int64) (line, column int) { - line = 1 - for _, b := range data[:offset] { - if b == '\n' { - line++ - column = 1 - } else { - column++ - } - } - return -} - -func (s *playlists) parseNSP(_ context.Context, pls *model.Playlist, reader io.Reader) error { - nsp := &nspFile{} - reader = io.LimitReader(reader, 100*1024) // Limit to 100KB - reader = jsoncommentstrip.NewReader(reader) - input, err := io.ReadAll(reader) - if err != nil { - return fmt.Errorf("reading SmartPlaylist: %w", err) - } - err = json.Unmarshal(input, nsp) - if err != nil { - var syntaxErr *json.SyntaxError - if errors.As(err, &syntaxErr) { - line, col := getPositionFromOffset(input, syntaxErr.Offset) - return fmt.Errorf("JSON syntax error in SmartPlaylist at line %d, column %d: %w", line, col, err) - } - return fmt.Errorf("JSON parsing error in SmartPlaylist: %w", err) - } - pls.Rules = &nsp.Criteria - if nsp.Name != "" { - pls.Name = nsp.Name - } - if nsp.Comment != "" { - pls.Comment = nsp.Comment - } - if nsp.Public != nil { - pls.Public = *nsp.Public - } else { - pls.Public = conf.Server.DefaultPlaylistPublicVisibility - } - return nil -} - func (s *playlists) parseM3U(ctx context.Context, pls *model.Playlist, folder *model.Folder, reader io.Reader) error { mediaFileRepository := s.ds.MediaFile(ctx) + resolver, err := newPathResolver(ctx, s.ds) + if err != nil { + return err + } var mfs model.MediaFiles // Chunk size of 100 lines, as each line can generate up to 4 lookup candidates // (NFC/NFD × raw/lowercase), and SQLite has a max expression tree depth of 1000. @@ -202,7 +47,7 @@ func (s *playlists) parseM3U(ctx context.Context, pls *model.Playlist, folder *m } filteredLines = append(filteredLines, line) } - resolvedPaths, err := s.resolvePaths(ctx, folder, filteredLines) + resolvedPaths, err := resolver.resolvePaths(ctx, folder, filteredLines) if err != nil { log.Warn(ctx, "Error resolving paths in playlist", "playlist", pls.Name, err) continue @@ -258,7 +103,9 @@ func (s *playlists) parseM3U(ctx context.Context, pls *model.Playlist, folder *m existing[key] = idx } - // Find media files in the order of the resolved paths, to keep playlist order + // Find media files in the order of the resolved paths, to keep playlist order. + // Both `existing` keys and `resolvedPaths` use the library-qualified format "libraryID:relativePath", + // so normalizing the full string produces matching keys (digits and ':' are ASCII-invariant). for _, path := range resolvedPaths { key := strings.ToLower(norm.NFC.String(path)) idx, ok := existing[key] @@ -398,15 +245,10 @@ func (r *pathResolver) findInLibraries(absolutePath string) pathResolution { // resolvePaths converts playlist file paths to library-qualified paths (format: "libraryID:relativePath"). // For relative paths, it resolves them to absolute paths first, then determines which // library they belong to. This allows playlists to reference files across library boundaries. -func (s *playlists) resolvePaths(ctx context.Context, folder *model.Folder, lines []string) ([]string, error) { - resolver, err := newPathResolver(ctx, s.ds) - if err != nil { - return nil, err - } - +func (r *pathResolver) resolvePaths(ctx context.Context, folder *model.Folder, lines []string) ([]string, error) { results := make([]string, 0, len(lines)) for idx, line := range lines { - resolution := resolver.resolvePath(line, folder) + resolution := r.resolvePath(line, folder) if !resolution.valid { log.Warn(ctx, "Path in playlist not found in any library", "path", line, "line", idx) @@ -425,123 +267,3 @@ func (s *playlists) resolvePaths(ctx context.Context, folder *model.Folder, line return results, nil } - -func (s *playlists) updatePlaylist(ctx context.Context, newPls *model.Playlist) error { - owner, _ := request.UserFrom(ctx) - - // Try to find existing playlist by path. Since filesystem normalization differs across - // platforms (macOS uses NFD, Linux/Windows use NFC), we try both forms to match - // playlists that may have been imported on a different platform. - pls, err := s.ds.Playlist(ctx).FindByPath(newPls.Path) - if errors.Is(err, model.ErrNotFound) { - // Try alternate normalization form - altPath := norm.NFD.String(newPls.Path) - if altPath == newPls.Path { - altPath = norm.NFC.String(newPls.Path) - } - if altPath != newPls.Path { - pls, err = s.ds.Playlist(ctx).FindByPath(altPath) - } - } - if err != nil && !errors.Is(err, model.ErrNotFound) { - return err - } - if err == nil && !pls.Sync { - log.Debug(ctx, "Playlist already imported and not synced", "playlist", pls.Name, "path", pls.Path) - return nil - } - - if err == nil { - log.Info(ctx, "Updating synced playlist", "playlist", pls.Name, "path", newPls.Path) - newPls.ID = pls.ID - newPls.Name = pls.Name - newPls.Comment = pls.Comment - newPls.OwnerID = pls.OwnerID - newPls.Public = pls.Public - newPls.EvaluatedAt = &time.Time{} - } else { - log.Info(ctx, "Adding synced playlist", "playlist", newPls.Name, "path", newPls.Path, "owner", owner.UserName) - newPls.OwnerID = owner.ID - // For NSP files, Public may already be set from the file; for M3U, use server default - if !newPls.IsSmartPlaylist() { - newPls.Public = conf.Server.DefaultPlaylistPublicVisibility - } - } - return s.ds.Playlist(ctx).Put(newPls) -} - -func (s *playlists) Update(ctx context.Context, playlistID string, - name *string, comment *string, public *bool, - idsToAdd []string, idxToRemove []int) error { - needsInfoUpdate := name != nil || comment != nil || public != nil - needsTrackRefresh := len(idxToRemove) > 0 - - return s.ds.WithTxImmediate(func(tx model.DataStore) error { - var pls *model.Playlist - var err error - repo := tx.Playlist(ctx) - tracks := repo.Tracks(playlistID, true) - if tracks == nil { - return fmt.Errorf("%w: playlist '%s'", model.ErrNotFound, playlistID) - } - if needsTrackRefresh { - pls, err = repo.GetWithTracks(playlistID, true, false) - pls.RemoveTracks(idxToRemove) - pls.AddMediaFilesByID(idsToAdd) - } else { - if len(idsToAdd) > 0 { - _, err = tracks.Add(idsToAdd) - if err != nil { - return err - } - } - if needsInfoUpdate { - pls, err = repo.Get(playlistID) - } - } - if err != nil { - return err - } - if !needsTrackRefresh && !needsInfoUpdate { - return nil - } - - if name != nil { - pls.Name = *name - } - if comment != nil { - pls.Comment = *comment - } - if public != nil { - pls.Public = *public - } - // Special case: The playlist is now empty - if len(idxToRemove) > 0 && len(pls.Tracks) == 0 { - if err = tracks.DeleteAll(); err != nil { - return err - } - } - return repo.Put(pls) - }) -} - -type nspFile struct { - criteria.Criteria - Name string `json:"name"` - Comment string `json:"comment"` - Public *bool `json:"public"` -} - -func (i *nspFile) UnmarshalJSON(data []byte) error { - m := map[string]any{} - err := json.Unmarshal(data, &m) - if err != nil { - return err - } - i.Name, _ = m["name"].(string) - i.Comment, _ = m["comment"].(string) - if public, ok := m["public"].(bool); ok { - i.Public = &public - } - return json.Unmarshal(data, &i.Criteria) -} diff --git a/core/playlists_internal_test.go b/core/playlists/parse_m3u_test.go similarity index 91% rename from core/playlists_internal_test.go rename to core/playlists/parse_m3u_test.go index 88e36cc3..05e1c30e 100644 --- a/core/playlists_internal_test.go +++ b/core/playlists/parse_m3u_test.go @@ -1,4 +1,4 @@ -package core +package playlists import ( "context" @@ -214,38 +214,38 @@ var _ = Describe("pathResolver", func() { }) Describe("resolvePath", func() { - It("resolves absolute paths", func() { - resolution := resolver.resolvePath("/music/artist/album/track.mp3", nil) + Context("basic", func() { + It("resolves absolute paths", func() { + resolution := resolver.resolvePath("/music/artist/album/track.mp3", nil) - Expect(resolution.valid).To(BeTrue()) - Expect(resolution.libraryID).To(Equal(1)) - Expect(resolution.libraryPath).To(Equal("/music")) - Expect(resolution.absolutePath).To(Equal("/music/artist/album/track.mp3")) + Expect(resolution.valid).To(BeTrue()) + Expect(resolution.libraryID).To(Equal(1)) + Expect(resolution.libraryPath).To(Equal("/music")) + Expect(resolution.absolutePath).To(Equal("/music/artist/album/track.mp3")) + }) + + It("resolves relative paths when folder is provided", func() { + folder := &model.Folder{ + Path: "playlists", + LibraryPath: "/music", + LibraryID: 1, + } + + resolution := resolver.resolvePath("../artist/album/track.mp3", folder) + + Expect(resolution.valid).To(BeTrue()) + Expect(resolution.libraryID).To(Equal(1)) + Expect(resolution.absolutePath).To(Equal("/music/artist/album/track.mp3")) + }) + + It("returns invalid resolution for paths outside any library", func() { + resolution := resolver.resolvePath("/outside/library/track.mp3", nil) + + Expect(resolution.valid).To(BeFalse()) + }) }) - It("resolves relative paths when folder is provided", func() { - folder := &model.Folder{ - Path: "playlists", - LibraryPath: "/music", - LibraryID: 1, - } - - resolution := resolver.resolvePath("../artist/album/track.mp3", folder) - - Expect(resolution.valid).To(BeTrue()) - Expect(resolution.libraryID).To(Equal(1)) - Expect(resolution.absolutePath).To(Equal("/music/artist/album/track.mp3")) - }) - - It("returns invalid resolution for paths outside any library", func() { - resolution := resolver.resolvePath("/outside/library/track.mp3", nil) - - Expect(resolution.valid).To(BeFalse()) - }) - }) - - Describe("resolvePath", func() { - Context("With absolute paths", func() { + Context("cross-library", func() { It("resolves path within a library", func() { resolution := resolver.resolvePath("/music/track.mp3", nil) diff --git a/core/playlists/parse_nsp.go b/core/playlists/parse_nsp.go new file mode 100644 index 00000000..c7f7dd94 --- /dev/null +++ b/core/playlists/parse_nsp.go @@ -0,0 +1,103 @@ +package playlists + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "io" + "os" + "path/filepath" + + "github.com/RaveNoX/go-jsoncommentstrip" + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/criteria" +) + +func (s *playlists) newSyncedPlaylist(baseDir string, playlistFile string) (*model.Playlist, error) { + playlistPath := filepath.Join(baseDir, playlistFile) + info, err := os.Stat(playlistPath) + if err != nil { + return nil, err + } + + var extension = filepath.Ext(playlistFile) + var name = playlistFile[0 : len(playlistFile)-len(extension)] + + pls := &model.Playlist{ + Name: name, + Comment: fmt.Sprintf("Auto-imported from '%s'", playlistFile), + Public: false, + Path: playlistPath, + Sync: true, + UpdatedAt: info.ModTime(), + } + return pls, nil +} + +func getPositionFromOffset(data []byte, offset int64) (line, column int) { + line = 1 + for _, b := range data[:offset] { + if b == '\n' { + line++ + column = 1 + } else { + column++ + } + } + return +} + +func (s *playlists) parseNSP(_ context.Context, pls *model.Playlist, reader io.Reader) error { + nsp := &nspFile{} + reader = io.LimitReader(reader, 100*1024) // Limit to 100KB + reader = jsoncommentstrip.NewReader(reader) + input, err := io.ReadAll(reader) + if err != nil { + return fmt.Errorf("reading SmartPlaylist: %w", err) + } + err = json.Unmarshal(input, nsp) + if err != nil { + var syntaxErr *json.SyntaxError + if errors.As(err, &syntaxErr) { + line, col := getPositionFromOffset(input, syntaxErr.Offset) + return fmt.Errorf("JSON syntax error in SmartPlaylist at line %d, column %d: %w", line, col, err) + } + return fmt.Errorf("JSON parsing error in SmartPlaylist: %w", err) + } + pls.Rules = &nsp.Criteria + if nsp.Name != "" { + pls.Name = nsp.Name + } + if nsp.Comment != "" { + pls.Comment = nsp.Comment + } + if nsp.Public != nil { + pls.Public = *nsp.Public + } else { + pls.Public = conf.Server.DefaultPlaylistPublicVisibility + } + return nil +} + +type nspFile struct { + criteria.Criteria + Name string `json:"name"` + Comment string `json:"comment"` + Public *bool `json:"public"` +} + +func (i *nspFile) UnmarshalJSON(data []byte) error { + m := map[string]any{} + err := json.Unmarshal(data, &m) + if err != nil { + return err + } + i.Name, _ = m["name"].(string) + i.Comment, _ = m["comment"].(string) + if public, ok := m["public"].(bool); ok { + i.Public = &public + } + return json.Unmarshal(data, &i.Criteria) +} diff --git a/core/playlists/parse_nsp_test.go b/core/playlists/parse_nsp_test.go new file mode 100644 index 00000000..0a7b2727 --- /dev/null +++ b/core/playlists/parse_nsp_test.go @@ -0,0 +1,213 @@ +package playlists + +import ( + "context" + "os" + "path/filepath" + "strings" + + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/conf/configtest" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/criteria" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("parseNSP", func() { + var s *playlists + ctx := context.Background() + + BeforeEach(func() { + s = &playlists{} + }) + + It("parses a well-formed NSP with all fields", func() { + nsp := `{ + "name": "My Smart Playlist", + "comment": "A test playlist", + "public": true, + "all": [{"is": {"loved": true}}], + "sort": "title", + "order": "asc", + "limit": 50 + }` + pls := &model.Playlist{Name: "default-name"} + err := s.parseNSP(ctx, pls, strings.NewReader(nsp)) + Expect(err).ToNot(HaveOccurred()) + Expect(pls.Name).To(Equal("My Smart Playlist")) + Expect(pls.Comment).To(Equal("A test playlist")) + Expect(pls.Public).To(BeTrue()) + Expect(pls.Rules).ToNot(BeNil()) + Expect(pls.Rules.Sort).To(Equal("title")) + Expect(pls.Rules.Order).To(Equal("asc")) + Expect(pls.Rules.Limit).To(Equal(50)) + Expect(pls.Rules.Expression).To(BeAssignableToTypeOf(criteria.All{})) + }) + + It("keeps existing name when NSP has no name field", func() { + nsp := `{"all": [{"is": {"loved": true}}]}` + pls := &model.Playlist{Name: "Original Name"} + err := s.parseNSP(ctx, pls, strings.NewReader(nsp)) + Expect(err).ToNot(HaveOccurred()) + Expect(pls.Name).To(Equal("Original Name")) + }) + + It("keeps existing comment when NSP has no comment field", func() { + nsp := `{"all": [{"is": {"loved": true}}]}` + pls := &model.Playlist{Comment: "Original Comment"} + err := s.parseNSP(ctx, pls, strings.NewReader(nsp)) + Expect(err).ToNot(HaveOccurred()) + Expect(pls.Comment).To(Equal("Original Comment")) + }) + + It("strips JSON comments before parsing", func() { + nsp := `{ + // Line comment + "name": "Commented Playlist", + /* Block comment */ + "all": [{"is": {"loved": true}}] + }` + pls := &model.Playlist{} + err := s.parseNSP(ctx, pls, strings.NewReader(nsp)) + Expect(err).ToNot(HaveOccurred()) + Expect(pls.Name).To(Equal("Commented Playlist")) + }) + + It("uses server default when public field is absent", func() { + DeferCleanup(configtest.SetupConfig()) + conf.Server.DefaultPlaylistPublicVisibility = true + + nsp := `{"all": [{"is": {"loved": true}}]}` + pls := &model.Playlist{} + err := s.parseNSP(ctx, pls, strings.NewReader(nsp)) + Expect(err).ToNot(HaveOccurred()) + Expect(pls.Public).To(BeTrue()) + }) + + It("honors explicit public: false over server default", func() { + DeferCleanup(configtest.SetupConfig()) + conf.Server.DefaultPlaylistPublicVisibility = true + + nsp := `{"public": false, "all": [{"is": {"loved": true}}]}` + pls := &model.Playlist{} + err := s.parseNSP(ctx, pls, strings.NewReader(nsp)) + Expect(err).ToNot(HaveOccurred()) + Expect(pls.Public).To(BeFalse()) + }) + + It("returns a syntax error with line and column info", func() { + nsp := "{\n \"name\": \"Bad\",\n \"all\": [INVALID]\n}" + pls := &model.Playlist{} + err := s.parseNSP(ctx, pls, strings.NewReader(nsp)) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("JSON syntax error in SmartPlaylist")) + Expect(err.Error()).To(MatchRegexp(`line \d+, column \d+`)) + }) + + It("returns a parsing error for completely invalid JSON", func() { + nsp := `not json at all` + pls := &model.Playlist{} + err := s.parseNSP(ctx, pls, strings.NewReader(nsp)) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("SmartPlaylist")) + }) + + It("gracefully handles non-string name field", func() { + nsp := `{"name": 123, "all": [{"is": {"loved": true}}]}` + pls := &model.Playlist{Name: "Original"} + err := s.parseNSP(ctx, pls, strings.NewReader(nsp)) + Expect(err).ToNot(HaveOccurred()) + // Type assertion in UnmarshalJSON fails silently; name stays as original + Expect(pls.Name).To(Equal("Original")) + }) + + It("parses criteria with multiple rules", func() { + nsp := `{ + "all": [ + {"is": {"loved": true}}, + {"contains": {"title": "rock"}} + ], + "sort": "lastPlayed", + "order": "desc", + "limit": 100 + }` + pls := &model.Playlist{} + err := s.parseNSP(ctx, pls, strings.NewReader(nsp)) + Expect(err).ToNot(HaveOccurred()) + Expect(pls.Rules).ToNot(BeNil()) + Expect(pls.Rules.Sort).To(Equal("lastPlayed")) + Expect(pls.Rules.Order).To(Equal("desc")) + Expect(pls.Rules.Limit).To(Equal(100)) + }) +}) + +var _ = Describe("getPositionFromOffset", func() { + It("returns correct position on first line", func() { + data := []byte("hello world") + line, col := getPositionFromOffset(data, 5) + Expect(line).To(Equal(1)) + Expect(col).To(Equal(5)) + }) + + It("returns correct position after newlines", func() { + data := []byte("line1\nline2\nline3") + // Offsets: l(0) i(1) n(2) e(3) 1(4) \n(5) l(6) i(7) n(8) + line, col := getPositionFromOffset(data, 8) + Expect(line).To(Equal(2)) + Expect(col).To(Equal(3)) + }) + + It("returns correct position at start of new line", func() { + data := []byte("line1\nline2") + // After \n at offset 5, col resets to 1; offset 6 is 'l' -> col=1 + line, col := getPositionFromOffset(data, 6) + Expect(line).To(Equal(2)) + Expect(col).To(Equal(1)) + }) + + It("handles multiple newlines", func() { + data := []byte("a\nb\nc\nd") + // a(0) \n(1) b(2) \n(3) c(4) \n(5) d(6) + line, col := getPositionFromOffset(data, 6) + Expect(line).To(Equal(4)) + Expect(col).To(Equal(1)) + }) +}) + +var _ = Describe("newSyncedPlaylist", func() { + var s *playlists + + BeforeEach(func() { + s = &playlists{} + }) + + It("creates a synced playlist with correct attributes", func() { + tmpDir := GinkgoT().TempDir() + Expect(os.WriteFile(filepath.Join(tmpDir, "test.m3u"), []byte("content"), 0600)).To(Succeed()) + + pls, err := s.newSyncedPlaylist(tmpDir, "test.m3u") + Expect(err).ToNot(HaveOccurred()) + Expect(pls.Name).To(Equal("test")) + Expect(pls.Comment).To(Equal("Auto-imported from 'test.m3u'")) + Expect(pls.Public).To(BeFalse()) + Expect(pls.Path).To(Equal(filepath.Join(tmpDir, "test.m3u"))) + Expect(pls.Sync).To(BeTrue()) + Expect(pls.UpdatedAt).ToNot(BeZero()) + }) + + It("strips extension from filename to derive name", func() { + tmpDir := GinkgoT().TempDir() + Expect(os.WriteFile(filepath.Join(tmpDir, "My Favorites.nsp"), []byte("{}"), 0600)).To(Succeed()) + + pls, err := s.newSyncedPlaylist(tmpDir, "My Favorites.nsp") + Expect(err).ToNot(HaveOccurred()) + Expect(pls.Name).To(Equal("My Favorites")) + }) + + It("returns error for non-existent file", func() { + tmpDir := GinkgoT().TempDir() + _, err := s.newSyncedPlaylist(tmpDir, "nonexistent.m3u") + Expect(err).To(HaveOccurred()) + }) +}) diff --git a/core/playlists/playlists.go b/core/playlists/playlists.go new file mode 100644 index 00000000..5a9a908e --- /dev/null +++ b/core/playlists/playlists.go @@ -0,0 +1,265 @@ +package playlists + +import ( + "context" + "io" + "path/filepath" + "strconv" + "strings" + + "github.com/bmatcuk/doublestar/v4" + "github.com/deluan/rest" + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/request" +) + +type Playlists interface { + // Reads + GetAll(ctx context.Context, options ...model.QueryOptions) (model.Playlists, error) + Get(ctx context.Context, id string) (*model.Playlist, error) + GetWithTracks(ctx context.Context, id string) (*model.Playlist, error) + GetPlaylists(ctx context.Context, mediaFileId string) (model.Playlists, error) + + // Mutations + Create(ctx context.Context, playlistId string, name string, ids []string) (string, error) + Delete(ctx context.Context, id string) error + Update(ctx context.Context, playlistID string, name *string, comment *string, public *bool, idsToAdd []string, idxToRemove []int) error + + // Track management + AddTracks(ctx context.Context, playlistID string, ids []string) (int, error) + AddAlbums(ctx context.Context, playlistID string, albumIds []string) (int, error) + AddArtists(ctx context.Context, playlistID string, artistIds []string) (int, error) + AddDiscs(ctx context.Context, playlistID string, discs []model.DiscID) (int, error) + RemoveTracks(ctx context.Context, playlistID string, trackIds []string) error + ReorderTrack(ctx context.Context, playlistID string, pos int, newPos int) error + + // Import + ImportFile(ctx context.Context, folder *model.Folder, filename string) (*model.Playlist, error) + ImportM3U(ctx context.Context, reader io.Reader) (*model.Playlist, error) + + // REST adapters (follows Share/Library pattern) + NewRepository(ctx context.Context) rest.Repository + TracksRepository(ctx context.Context, playlistId string, refreshSmartPlaylist bool) rest.Repository +} + +type playlists struct { + ds model.DataStore +} + +func NewPlaylists(ds model.DataStore) Playlists { + return &playlists{ds: ds} +} + +func InPath(folder model.Folder) bool { + if conf.Server.PlaylistsPath == "" { + return true + } + rel, _ := filepath.Rel(folder.LibraryPath, folder.AbsolutePath()) + for path := range strings.SplitSeq(conf.Server.PlaylistsPath, string(filepath.ListSeparator)) { + if match, _ := doublestar.Match(path, rel); match { + return true + } + } + return false +} + +// --- Read operations --- + +func (s *playlists) GetAll(ctx context.Context, options ...model.QueryOptions) (model.Playlists, error) { + return s.ds.Playlist(ctx).GetAll(options...) +} + +func (s *playlists) Get(ctx context.Context, id string) (*model.Playlist, error) { + return s.ds.Playlist(ctx).Get(id) +} + +func (s *playlists) GetWithTracks(ctx context.Context, id string) (*model.Playlist, error) { + return s.ds.Playlist(ctx).GetWithTracks(id, true, false) +} + +func (s *playlists) GetPlaylists(ctx context.Context, mediaFileId string) (model.Playlists, error) { + return s.ds.Playlist(ctx).GetPlaylists(mediaFileId) +} + +// --- Mutation operations --- + +// Create creates a new playlist (when name is provided) or replaces tracks on an existing +// playlist (when playlistId is provided). This matches the Subsonic createPlaylist semantics. +func (s *playlists) Create(ctx context.Context, playlistId string, name string, ids []string) (string, error) { + usr, _ := request.UserFrom(ctx) + err := s.ds.WithTxImmediate(func(tx model.DataStore) error { + var pls *model.Playlist + var err error + + if playlistId != "" { + pls, err = tx.Playlist(ctx).Get(playlistId) + if err != nil { + return err + } + if pls.IsSmartPlaylist() { + return model.ErrNotAuthorized + } + if !usr.IsAdmin && pls.OwnerID != usr.ID { + return model.ErrNotAuthorized + } + } else { + pls = &model.Playlist{Name: name} + pls.OwnerID = usr.ID + } + pls.Tracks = nil + pls.AddMediaFilesByID(ids) + + err = tx.Playlist(ctx).Put(pls) + playlistId = pls.ID + return err + }) + return playlistId, err +} + +func (s *playlists) Delete(ctx context.Context, id string) error { + if _, err := s.checkWritable(ctx, id); err != nil { + return err + } + return s.ds.Playlist(ctx).Delete(id) +} + +func (s *playlists) Update(ctx context.Context, playlistID string, + name *string, comment *string, public *bool, + idsToAdd []string, idxToRemove []int) error { + var pls *model.Playlist + var err error + hasTrackChanges := len(idsToAdd) > 0 || len(idxToRemove) > 0 + if hasTrackChanges { + pls, err = s.checkTracksEditable(ctx, playlistID) + } else { + pls, err = s.checkWritable(ctx, playlistID) + } + if err != nil { + return err + } + return s.ds.WithTxImmediate(func(tx model.DataStore) error { + repo := tx.Playlist(ctx) + + if len(idxToRemove) > 0 { + tracksRepo := repo.Tracks(playlistID, false) + // Convert 0-based indices to 1-based position IDs and delete them directly, + // avoiding the need to load all tracks into memory. + positions := make([]string, len(idxToRemove)) + for i, idx := range idxToRemove { + positions[i] = strconv.Itoa(idx + 1) + } + if err := tracksRepo.Delete(positions...); err != nil { + return err + } + if len(idsToAdd) > 0 { + if _, err := tracksRepo.Add(idsToAdd); err != nil { + return err + } + } + return s.updateMetadata(ctx, tx, pls, name, comment, public) + } + + if len(idsToAdd) > 0 { + if _, err := repo.Tracks(playlistID, false).Add(idsToAdd); err != nil { + return err + } + } + if name == nil && comment == nil && public == nil { + return nil + } + // Reuse the playlist from checkWritable (no tracks loaded, so Put only refreshes counters) + return s.updateMetadata(ctx, tx, pls, name, comment, public) + }) +} + +// --- Permission helpers --- + +// checkWritable fetches the playlist and verifies the current user can modify it. +func (s *playlists) checkWritable(ctx context.Context, id string) (*model.Playlist, error) { + pls, err := s.ds.Playlist(ctx).Get(id) + if err != nil { + return nil, err + } + usr, _ := request.UserFrom(ctx) + if !usr.IsAdmin && pls.OwnerID != usr.ID { + return nil, model.ErrNotAuthorized + } + return pls, nil +} + +// checkTracksEditable verifies the user can modify tracks (ownership + not smart playlist). +func (s *playlists) checkTracksEditable(ctx context.Context, playlistID string) (*model.Playlist, error) { + pls, err := s.checkWritable(ctx, playlistID) + if err != nil { + return nil, err + } + if pls.IsSmartPlaylist() { + return nil, model.ErrNotAuthorized + } + return pls, nil +} + +// updateMetadata applies optional metadata changes to a playlist and persists it. +// Accepts a DataStore parameter so it can be used inside transactions. +// The caller is responsible for permission checks. +func (s *playlists) updateMetadata(ctx context.Context, ds model.DataStore, pls *model.Playlist, name *string, comment *string, public *bool) error { + if name != nil { + pls.Name = *name + } + if comment != nil { + pls.Comment = *comment + } + if public != nil { + pls.Public = *public + } + return ds.Playlist(ctx).Put(pls) +} + +// --- Track management operations --- + +func (s *playlists) AddTracks(ctx context.Context, playlistID string, ids []string) (int, error) { + if _, err := s.checkTracksEditable(ctx, playlistID); err != nil { + return 0, err + } + return s.ds.Playlist(ctx).Tracks(playlistID, false).Add(ids) +} + +func (s *playlists) AddAlbums(ctx context.Context, playlistID string, albumIds []string) (int, error) { + if _, err := s.checkTracksEditable(ctx, playlistID); err != nil { + return 0, err + } + return s.ds.Playlist(ctx).Tracks(playlistID, false).AddAlbums(albumIds) +} + +func (s *playlists) AddArtists(ctx context.Context, playlistID string, artistIds []string) (int, error) { + if _, err := s.checkTracksEditable(ctx, playlistID); err != nil { + return 0, err + } + return s.ds.Playlist(ctx).Tracks(playlistID, false).AddArtists(artistIds) +} + +func (s *playlists) AddDiscs(ctx context.Context, playlistID string, discs []model.DiscID) (int, error) { + if _, err := s.checkTracksEditable(ctx, playlistID); err != nil { + return 0, err + } + return s.ds.Playlist(ctx).Tracks(playlistID, false).AddDiscs(discs) +} + +func (s *playlists) RemoveTracks(ctx context.Context, playlistID string, trackIds []string) error { + if _, err := s.checkTracksEditable(ctx, playlistID); err != nil { + return err + } + return s.ds.WithTx(func(tx model.DataStore) error { + return tx.Playlist(ctx).Tracks(playlistID, false).Delete(trackIds...) + }) +} + +func (s *playlists) ReorderTrack(ctx context.Context, playlistID string, pos int, newPos int) error { + if _, err := s.checkTracksEditable(ctx, playlistID); err != nil { + return err + } + return s.ds.WithTx(func(tx model.DataStore) error { + return tx.Playlist(ctx).Tracks(playlistID, false).Reorder(pos, newPos) + }) +} diff --git a/core/playlists/playlists_suite_test.go b/core/playlists/playlists_suite_test.go new file mode 100644 index 00000000..b5724849 --- /dev/null +++ b/core/playlists/playlists_suite_test.go @@ -0,0 +1,17 @@ +package playlists_test + +import ( + "testing" + + "github.com/navidrome/navidrome/log" + "github.com/navidrome/navidrome/tests" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestPlaylists(t *testing.T) { + tests.Init(t, false) + log.SetLevel(log.LevelFatal) + RegisterFailHandler(Fail) + RunSpecs(t, "Playlists Suite") +} diff --git a/core/playlists/playlists_test.go b/core/playlists/playlists_test.go new file mode 100644 index 00000000..a4c309d7 --- /dev/null +++ b/core/playlists/playlists_test.go @@ -0,0 +1,297 @@ +package playlists_test + +import ( + "context" + + "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 + ps = playlists.NewPlaylists(ds) + }) + + 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"}}}, + } + ps = playlists.NewPlaylists(ds) + }) + + 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 + ps = playlists.NewPlaylists(ds) + }) + + It("allows owner to update their playlist", func() { + ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false}) + newName := "Updated Name" + err := ps.Update(ctx, "pls-1", &newName, nil, nil, nil, nil) + Expect(err).ToNot(HaveOccurred()) + }) + + It("allows admin to update any playlist", func() { + ctx = request.WithUser(ctx, model.User{ID: "admin-1", IsAdmin: true}) + newName := "Updated Name" + err := ps.Update(ctx, "pls-other", &newName, nil, nil, nil, nil) + Expect(err).ToNot(HaveOccurred()) + }) + + It("denies non-owner, non-admin from updating", func() { + ctx = request.WithUser(ctx, model.User{ID: "other-user", IsAdmin: false}) + newName := "Updated Name" + err := ps.Update(ctx, "pls-1", &newName, nil, nil, nil, nil) + 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}) + newName := "Updated Name" + err := ps.Update(ctx, "nonexistent", &newName, nil, nil, nil, nil) + 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}) + newName := "Updated Smart" + err := ps.Update(ctx, "pls-smart", &newName, nil, nil, nil, nil) + 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 + ps = playlists.NewPlaylists(ds) + }) + + 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 + ps = playlists.NewPlaylists(ds) + }) + + 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 + ps = playlists.NewPlaylists(ds) + }) + + 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)) + }) + }) +}) diff --git a/core/playlists/rest_adapter.go b/core/playlists/rest_adapter.go new file mode 100644 index 00000000..3865d97e --- /dev/null +++ b/core/playlists/rest_adapter.go @@ -0,0 +1,95 @@ +package playlists + +import ( + "context" + "errors" + + "github.com/deluan/rest" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/request" +) + +// --- REST adapter (follows Share/Library pattern) --- + +func (s *playlists) NewRepository(ctx context.Context) rest.Repository { + return &playlistRepositoryWrapper{ + ctx: ctx, + PlaylistRepository: s.ds.Playlist(ctx), + service: s, + } +} + +// playlistRepositoryWrapper wraps the playlist repository as a thin REST-to-service adapter. +// It satisfies rest.Repository through the embedded PlaylistRepository (via ResourceRepository), +// and rest.Persistable by delegating to service methods for all mutations. +type playlistRepositoryWrapper struct { + model.PlaylistRepository + ctx context.Context + service *playlists +} + +func (r *playlistRepositoryWrapper) Save(entity any) (string, error) { + return r.service.savePlaylist(r.ctx, entity.(*model.Playlist)) +} + +func (r *playlistRepositoryWrapper) Update(id string, entity any, cols ...string) error { + return r.service.updatePlaylistEntity(r.ctx, id, entity.(*model.Playlist), cols...) +} + +func (r *playlistRepositoryWrapper) Delete(id string) error { + err := r.service.Delete(r.ctx, id) + switch { + case errors.Is(err, model.ErrNotFound): + return rest.ErrNotFound + case errors.Is(err, model.ErrNotAuthorized): + return rest.ErrPermissionDenied + default: + return err + } +} + +func (s *playlists) TracksRepository(ctx context.Context, playlistId string, refreshSmartPlaylist bool) rest.Repository { + repo := s.ds.Playlist(ctx) + tracks := repo.Tracks(playlistId, refreshSmartPlaylist) + if tracks == nil { + return nil + } + return tracks.(rest.Repository) +} + +// savePlaylist creates a new playlist, assigning the owner from context. +func (s *playlists) savePlaylist(ctx context.Context, pls *model.Playlist) (string, error) { + usr, _ := request.UserFrom(ctx) + pls.OwnerID = usr.ID + pls.ID = "" // Force new creation + err := s.ds.Playlist(ctx).Put(pls) + if err != nil { + return "", err + } + return pls.ID, nil +} + +// updatePlaylistEntity updates playlist metadata with permission checks. +// Used by the REST API wrapper. +func (s *playlists) updatePlaylistEntity(ctx context.Context, id string, entity *model.Playlist, cols ...string) error { + current, err := s.checkWritable(ctx, id) + if err != nil { + switch { + case errors.Is(err, model.ErrNotFound): + return rest.ErrNotFound + case errors.Is(err, model.ErrNotAuthorized): + return rest.ErrPermissionDenied + default: + return err + } + } + usr, _ := request.UserFrom(ctx) + if !usr.IsAdmin && entity.OwnerID != "" && entity.OwnerID != current.OwnerID { + return rest.ErrPermissionDenied + } + // Apply ownership change (admin only) + if entity.OwnerID != "" { + current.OwnerID = entity.OwnerID + } + return s.updateMetadata(ctx, s.ds, current, &entity.Name, &entity.Comment, &entity.Public) +} diff --git a/core/playlists/rest_adapter_test.go b/core/playlists/rest_adapter_test.go new file mode 100644 index 00000000..b6509595 --- /dev/null +++ b/core/playlists/rest_adapter_test.go @@ -0,0 +1,120 @@ +package playlists_test + +import ( + "context" + + "github.com/deluan/rest" + "github.com/navidrome/navidrome/core/playlists" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/request" + "github.com/navidrome/navidrome/tests" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("REST Adapter", 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("NewRepository", func() { + var repo rest.Persistable + + BeforeEach(func() { + mockPlsRepo.Data = map[string]*model.Playlist{ + "pls-1": {ID: "pls-1", Name: "My Playlist", OwnerID: "user-1"}, + } + ps = playlists.NewPlaylists(ds) + }) + + Describe("Save", func() { + It("sets the owner from the context user", func() { + ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false}) + repo = ps.NewRepository(ctx).(rest.Persistable) + pls := &model.Playlist{Name: "New Playlist"} + id, err := repo.Save(pls) + Expect(err).ToNot(HaveOccurred()) + Expect(id).ToNot(BeEmpty()) + Expect(pls.OwnerID).To(Equal("user-1")) + }) + + It("forces a new creation by clearing ID", func() { + ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false}) + repo = ps.NewRepository(ctx).(rest.Persistable) + pls := &model.Playlist{ID: "should-be-cleared", Name: "New"} + _, err := repo.Save(pls) + Expect(err).ToNot(HaveOccurred()) + Expect(pls.ID).ToNot(Equal("should-be-cleared")) + }) + }) + + Describe("Update", func() { + It("allows owner to update their playlist", func() { + ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false}) + repo = ps.NewRepository(ctx).(rest.Persistable) + pls := &model.Playlist{Name: "Updated"} + err := repo.Update("pls-1", pls) + Expect(err).ToNot(HaveOccurred()) + }) + + It("allows admin to update any playlist", func() { + ctx = request.WithUser(ctx, model.User{ID: "admin-1", IsAdmin: true}) + repo = ps.NewRepository(ctx).(rest.Persistable) + pls := &model.Playlist{Name: "Updated"} + err := repo.Update("pls-1", pls) + Expect(err).ToNot(HaveOccurred()) + }) + + It("denies non-owner, non-admin", func() { + ctx = request.WithUser(ctx, model.User{ID: "other-user", IsAdmin: false}) + repo = ps.NewRepository(ctx).(rest.Persistable) + pls := &model.Playlist{Name: "Updated"} + err := repo.Update("pls-1", pls) + Expect(err).To(Equal(rest.ErrPermissionDenied)) + }) + + It("denies regular user from changing ownership", func() { + ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false}) + repo = ps.NewRepository(ctx).(rest.Persistable) + pls := &model.Playlist{Name: "Updated", OwnerID: "other-user"} + err := repo.Update("pls-1", pls) + Expect(err).To(Equal(rest.ErrPermissionDenied)) + }) + + It("returns rest.ErrNotFound when playlist doesn't exist", func() { + ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false}) + repo = ps.NewRepository(ctx).(rest.Persistable) + pls := &model.Playlist{Name: "Updated"} + err := repo.Update("nonexistent", pls) + Expect(err).To(Equal(rest.ErrNotFound)) + }) + }) + + Describe("Delete", func() { + It("delegates to service Delete with permission checks", func() { + ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false}) + repo = ps.NewRepository(ctx).(rest.Persistable) + err := repo.Delete("pls-1") + Expect(err).ToNot(HaveOccurred()) + Expect(mockPlsRepo.Deleted).To(ContainElement("pls-1")) + }) + + It("denies non-owner", func() { + ctx = request.WithUser(ctx, model.User{ID: "other-user", IsAdmin: false}) + repo = ps.NewRepository(ctx).(rest.Persistable) + err := repo.Delete("pls-1") + Expect(err).To(Equal(rest.ErrPermissionDenied)) + }) + }) + }) +}) diff --git a/core/wire_providers.go b/core/wire_providers.go index a8b1fde0..503feb78 100644 --- a/core/wire_providers.go +++ b/core/wire_providers.go @@ -7,6 +7,7 @@ import ( "github.com/navidrome/navidrome/core/ffmpeg" "github.com/navidrome/navidrome/core/metrics" "github.com/navidrome/navidrome/core/playback" + "github.com/navidrome/navidrome/core/playlists" "github.com/navidrome/navidrome/core/scrobbler" ) @@ -16,7 +17,7 @@ var Set = wire.NewSet( NewArchiver, NewPlayers, NewShare, - NewPlaylists, + playlists.NewPlaylists, NewLibrary, NewUser, NewMaintenance, diff --git a/persistence/playlist_repository.go b/persistence/playlist_repository.go index 220c1210..955a13bc 100644 --- a/persistence/playlist_repository.go +++ b/persistence/playlist_repository.go @@ -96,16 +96,6 @@ func (r *playlistRepository) Exists(id string) (bool, error) { } func (r *playlistRepository) Delete(id string) error { - usr := loggedUser(r.ctx) - if !usr.IsAdmin { - pls, err := r.Get(id) - if err != nil { - return err - } - if pls.OwnerID != usr.ID { - return rest.ErrPermissionDenied - } - } return r.delete(And{Eq{"id": id}, r.userFilter()}) } @@ -113,14 +103,6 @@ func (r *playlistRepository) Put(p *model.Playlist) error { pls := dbPlaylist{Playlist: *p} if pls.ID == "" { pls.CreatedAt = time.Now() - } else { - ok, err := r.Exists(pls.ID) - if err != nil { - return err - } - if !ok { - return model.ErrNotAuthorized - } } pls.UpdatedAt = time.Now() @@ -132,7 +114,6 @@ func (r *playlistRepository) Put(p *model.Playlist) error { if p.IsSmartPlaylist() { // Do not update tracks at this point, as it may take a long time and lock the DB, breaking the scan process - //r.refreshSmartPlaylist(p) return nil } // Only update tracks if they were specified @@ -320,10 +301,6 @@ func (r *playlistRepository) updateTracks(id string, tracks model.MediaFiles) er } func (r *playlistRepository) updatePlaylist(playlistId string, mediaFileIds []string) error { - if !r.isWritable(playlistId) { - return rest.ErrPermissionDenied - } - // Remove old tracks del := Delete("playlist_tracks").Where(Eq{"playlist_id": playlistId}) _, err := r.executeSQL(del) @@ -439,8 +416,7 @@ func (r *playlistRepository) NewInstance() any { func (r *playlistRepository) Save(entity any) (string, error) { pls := entity.(*model.Playlist) - pls.OwnerID = loggedUser(r.ctx).ID - pls.ID = "" // Make sure we don't override an existing playlist + pls.ID = "" // Force new creation err := r.Put(pls) if err != nil { return "", err @@ -450,24 +426,9 @@ func (r *playlistRepository) Save(entity any) (string, error) { func (r *playlistRepository) Update(id string, entity any, cols ...string) error { pls := dbPlaylist{Playlist: *entity.(*model.Playlist)} - current, err := r.Get(id) - if err != nil { - return err - } - usr := loggedUser(r.ctx) - if !usr.IsAdmin { - // Only the owner can update the playlist - if current.OwnerID != usr.ID { - return rest.ErrPermissionDenied - } - // Regular users can't change the ownership of a playlist - if pls.OwnerID != "" && pls.OwnerID != usr.ID { - return rest.ErrPermissionDenied - } - } pls.ID = id pls.UpdatedAt = time.Now() - _, err = r.put(id, pls, append(cols, "updatedAt")...) + _, err := r.put(id, pls, append(cols, "updatedAt")...) if errors.Is(err, model.ErrNotFound) { return rest.ErrNotFound } @@ -507,23 +468,31 @@ func (r *playlistRepository) removeOrphans() error { return nil } +// renumber updates the position of all tracks in the playlist to be sequential starting from 1, ordered by their +// current position. This is needed after removing orphan tracks, to ensure there are no gaps in the track numbering. +// The two-step approach (negate then reassign via CTE) avoids UNIQUE constraint violations on (playlist_id, id). func (r *playlistRepository) renumber(id string) error { - var ids []string - sq := Select("media_file_id").From("playlist_tracks").Where(Eq{"playlist_id": id}).OrderBy("id") - err := r.queryAllSlice(sq, &ids) + // Step 1: Negate all IDs to clear the positive ID space + _, err := r.executeSQL(Expr( + `UPDATE playlist_tracks SET id = -id WHERE playlist_id = ? AND id > 0`, id)) if err != nil { return err } - return r.updatePlaylist(id, ids) -} - -func (r *playlistRepository) isWritable(playlistId string) bool { - usr := loggedUser(r.ctx) - if usr.IsAdmin { - return true + // Step 2: Assign new sequential positive IDs using UPDATE...FROM with a CTE. + // The CTE is fully materialized before the UPDATE begins, avoiding self-referencing issues. + // ORDER BY id DESC restores original order since IDs are now negative. + _, err = r.executeSQL(Expr( + `WITH new_ids AS ( + SELECT rowid as rid, ROW_NUMBER() OVER (ORDER BY id DESC) as new_id + FROM playlist_tracks WHERE playlist_id = ? + ) + UPDATE playlist_tracks SET id = new_ids.new_id + FROM new_ids + WHERE playlist_tracks.rowid = new_ids.rid AND playlist_tracks.playlist_id = ?`, id, id)) + if err != nil { + return err } - pls, err := r.Get(playlistId) - return err == nil && pls.OwnerID == usr.ID + return r.refreshCounters(&model.Playlist{ID: id}) } var _ model.PlaylistRepository = (*playlistRepository)(nil) diff --git a/persistence/playlist_repository_test.go b/persistence/playlist_repository_test.go index 232eb14b..5230390f 100644 --- a/persistence/playlist_repository_test.go +++ b/persistence/playlist_repository_test.go @@ -401,6 +401,79 @@ var _ = Describe("PlaylistRepository", func() { }) }) + Describe("Track Deletion and Renumbering", func() { + var testPlaylistID string + + AfterEach(func() { + if testPlaylistID != "" { + Expect(repo.Delete(testPlaylistID)).To(BeNil()) + testPlaylistID = "" + } + }) + + // helper to get track positions and media file IDs + getTrackInfo := func(playlistID string) (ids []string, mediaFileIDs []string) { + pls, err := repo.GetWithTracks(playlistID, false, false) + Expect(err).ToNot(HaveOccurred()) + for _, t := range pls.Tracks { + ids = append(ids, t.ID) + mediaFileIDs = append(mediaFileIDs, t.MediaFileID) + } + return + } + + It("renumbers correctly after deleting a track from the middle", func() { + By("creating a playlist with 4 tracks") + newPls := model.Playlist{Name: "Renumber Test Middle", OwnerID: "userid"} + newPls.AddMediaFilesByID([]string{"1001", "1002", "1003", "1004"}) + Expect(repo.Put(&newPls)).To(Succeed()) + testPlaylistID = newPls.ID + + By("deleting the second track (position 2)") + tracksRepo := repo.Tracks(newPls.ID, false) + Expect(tracksRepo.Delete("2")).To(Succeed()) + + By("verifying remaining tracks are renumbered sequentially") + ids, mediaFileIDs := getTrackInfo(newPls.ID) + Expect(ids).To(Equal([]string{"1", "2", "3"})) + Expect(mediaFileIDs).To(Equal([]string{"1001", "1003", "1004"})) + }) + + It("renumbers correctly after deleting the first track", func() { + By("creating a playlist with 3 tracks") + newPls := model.Playlist{Name: "Renumber Test First", OwnerID: "userid"} + newPls.AddMediaFilesByID([]string{"1001", "1002", "1003"}) + Expect(repo.Put(&newPls)).To(Succeed()) + testPlaylistID = newPls.ID + + By("deleting the first track (position 1)") + tracksRepo := repo.Tracks(newPls.ID, false) + Expect(tracksRepo.Delete("1")).To(Succeed()) + + By("verifying remaining tracks are renumbered sequentially") + ids, mediaFileIDs := getTrackInfo(newPls.ID) + Expect(ids).To(Equal([]string{"1", "2"})) + Expect(mediaFileIDs).To(Equal([]string{"1002", "1003"})) + }) + + It("renumbers correctly after deleting the last track", func() { + By("creating a playlist with 3 tracks") + newPls := model.Playlist{Name: "Renumber Test Last", OwnerID: "userid"} + newPls.AddMediaFilesByID([]string{"1001", "1002", "1003"}) + Expect(repo.Put(&newPls)).To(Succeed()) + testPlaylistID = newPls.ID + + By("deleting the last track (position 3)") + tracksRepo := repo.Tracks(newPls.ID, false) + Expect(tracksRepo.Delete("3")).To(Succeed()) + + By("verifying remaining tracks are renumbered sequentially") + ids, mediaFileIDs := getTrackInfo(newPls.ID) + Expect(ids).To(Equal([]string{"1", "2"})) + Expect(mediaFileIDs).To(Equal([]string{"1001", "1002"})) + }) + }) + Describe("Smart Playlists Library Filtering", func() { var mfRepo model.MediaFileRepository var testPlaylistID string diff --git a/persistence/playlist_track_repository.go b/persistence/playlist_track_repository.go index c72abb18..1a7062cc 100644 --- a/persistence/playlist_track_repository.go +++ b/persistence/playlist_track_repository.go @@ -140,15 +140,7 @@ func (r *playlistTrackRepository) NewInstance() any { return &model.PlaylistTrack{} } -func (r *playlistTrackRepository) isTracksEditable() bool { - return r.playlistRepo.isWritable(r.playlistId) && !r.playlist.IsSmartPlaylist() -} - func (r *playlistTrackRepository) Add(mediaFileIds []string) (int, error) { - if !r.isTracksEditable() { - return 0, rest.ErrPermissionDenied - } - if len(mediaFileIds) > 0 { log.Debug(r.ctx, "Adding songs to playlist", "playlistId", r.playlistId, "mediaFileIds", mediaFileIds) } else { @@ -196,22 +188,7 @@ func (r *playlistTrackRepository) AddDiscs(discs []model.DiscID) (int, error) { return r.addMediaFileIds(clauses) } -// Get ids from all current tracks -func (r *playlistTrackRepository) getTracks() ([]string, error) { - all := r.newSelect().Columns("media_file_id").Where(Eq{"playlist_id": r.playlistId}).OrderBy("id") - var ids []string - err := r.queryAllSlice(all, &ids) - if err != nil { - log.Error(r.ctx, "Error querying current tracks from playlist", "playlistId", r.playlistId, err) - return nil, err - } - return ids, nil -} - func (r *playlistTrackRepository) Delete(ids ...string) error { - if !r.isTracksEditable() { - return rest.ErrPermissionDenied - } err := r.delete(And{Eq{"playlist_id": r.playlistId}, Eq{"id": ids}}) if err != nil { return err @@ -221,9 +198,6 @@ func (r *playlistTrackRepository) Delete(ids ...string) error { } func (r *playlistTrackRepository) DeleteAll() error { - if !r.isTracksEditable() { - return rest.ErrPermissionDenied - } err := r.delete(Eq{"playlist_id": r.playlistId}) if err != nil { return err @@ -232,16 +206,45 @@ func (r *playlistTrackRepository) DeleteAll() error { return r.playlistRepo.renumber(r.playlistId) } +// Reorder moves a track from pos to newPos, shifting other tracks accordingly. func (r *playlistTrackRepository) Reorder(pos int, newPos int) error { - if !r.isTracksEditable() { - return rest.ErrPermissionDenied + if pos == newPos { + return nil } - ids, err := r.getTracks() + pid := r.playlistId + + // Step 1: Move the source track out of the way (temporary sentinel value) + _, err := r.executeSQL(Expr( + `UPDATE playlist_tracks SET id = -999999 WHERE playlist_id = ? AND id = ?`, pid, pos)) if err != nil { return err } - newOrder := slice.Move(ids, pos-1, newPos-1) - return r.playlistRepo.updatePlaylist(r.playlistId, newOrder) + + // Step 2: Shift the affected range using negative values to avoid unique constraint violations + if pos < newPos { + _, err = r.executeSQL(Expr( + `UPDATE playlist_tracks SET id = -(id - 1) WHERE playlist_id = ? AND id > ? AND id <= ?`, + pid, pos, newPos)) + } else { + _, err = r.executeSQL(Expr( + `UPDATE playlist_tracks SET id = -(id + 1) WHERE playlist_id = ? AND id >= ? AND id < ?`, + pid, newPos, pos)) + } + if err != nil { + return err + } + + // Step 3: Flip the shifted range back to positive + _, err = r.executeSQL(Expr( + `UPDATE playlist_tracks SET id = -id WHERE playlist_id = ? AND id < 0 AND id != -999999`, pid)) + if err != nil { + return err + } + + // Step 4: Place the source track at its new position + _, err = r.executeSQL(Expr( + `UPDATE playlist_tracks SET id = ? WHERE playlist_id = ? AND id = -999999`, newPos, pid)) + return err } var _ model.PlaylistTrackRepository = (*playlistTrackRepository)(nil) diff --git a/scanner/controller.go b/scanner/controller.go index db6444fa..94248ffd 100644 --- a/scanner/controller.go +++ b/scanner/controller.go @@ -9,10 +9,10 @@ import ( "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/consts" - "github.com/navidrome/navidrome/core" "github.com/navidrome/navidrome/core/artwork" "github.com/navidrome/navidrome/core/auth" "github.com/navidrome/navidrome/core/metrics" + "github.com/navidrome/navidrome/core/playlists" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/request" @@ -27,7 +27,7 @@ var ( ) func New(rootCtx context.Context, ds model.DataStore, cw artwork.CacheWarmer, broker events.Broker, - pls core.Playlists, m metrics.Metrics) model.Scanner { + pls playlists.Playlists, m metrics.Metrics) model.Scanner { c := &controller{ rootCtx: rootCtx, ds: ds, @@ -53,7 +53,7 @@ func (s *controller) getScanner() scanner { // CallScan starts an in-process scan of specific library/folder pairs. // If targets is empty, it scans all libraries. // This is meant to be called from the command line (see cmd/scan.go). -func CallScan(ctx context.Context, ds model.DataStore, pls core.Playlists, fullScan bool, targets []model.ScanTarget) (<-chan *ProgressInfo, error) { +func CallScan(ctx context.Context, ds model.DataStore, pls playlists.Playlists, fullScan bool, targets []model.ScanTarget) (<-chan *ProgressInfo, error) { release, err := lockScan(ctx) if err != nil { return nil, err @@ -98,7 +98,7 @@ type controller struct { cw artwork.CacheWarmer broker events.Broker metrics metrics.Metrics - pls core.Playlists + pls playlists.Playlists limiter *rate.Sometimes devExternalScanner bool count atomic.Uint32 diff --git a/scanner/controller_test.go b/scanner/controller_test.go index f5ccabc8..2af52066 100644 --- a/scanner/controller_test.go +++ b/scanner/controller_test.go @@ -5,9 +5,9 @@ import ( "github.com/navidrome/navidrome/conf/configtest" "github.com/navidrome/navidrome/consts" - "github.com/navidrome/navidrome/core" "github.com/navidrome/navidrome/core/artwork" "github.com/navidrome/navidrome/core/metrics" + "github.com/navidrome/navidrome/core/playlists" "github.com/navidrome/navidrome/db" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/persistence" @@ -31,7 +31,7 @@ var _ = Describe("Controller", func() { DeferCleanup(configtest.SetupConfig()) ds = &tests.MockDataStore{RealDS: persistence.New(db.Db())} ds.MockedProperty = &tests.MockedPropertyRepo{} - ctrl = scanner.New(ctx, ds, artwork.NoopCacheWarmer(), events.NoopBroker(), core.NewPlaylists(ds), metrics.NewNoopInstance()) + ctrl = scanner.New(ctx, ds, artwork.NoopCacheWarmer(), events.NoopBroker(), playlists.NewPlaylists(ds), metrics.NewNoopInstance()) }) It("includes last scan error", func() { diff --git a/scanner/folder_entry.go b/scanner/folder_entry.go index 9d8d0c57..c7cc88ee 100644 --- a/scanner/folder_entry.go +++ b/scanner/folder_entry.go @@ -10,7 +10,7 @@ import ( "slices" "time" - "github.com/navidrome/navidrome/core" + "github.com/navidrome/navidrome/core/playlists" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/utils/chrono" ) @@ -72,7 +72,7 @@ func (f *folderEntry) isOutdated() bool { func (f *folderEntry) toFolder() *model.Folder { folder := model.NewFolder(f.job.lib, f.path) folder.NumAudioFiles = len(f.audioFiles) - if core.InPlaylistsPath(*folder) { + if playlists.InPath(*folder) { folder.NumPlaylists = f.numPlaylists } folder.ImageFiles = slices.Collect(maps.Keys(f.imageFiles)) diff --git a/scanner/phase_4_playlists.go b/scanner/phase_4_playlists.go index c98b51ee..ab5f77ae 100644 --- a/scanner/phase_4_playlists.go +++ b/scanner/phase_4_playlists.go @@ -10,8 +10,8 @@ import ( ppl "github.com/google/go-pipeline/pkg/pipeline" "github.com/navidrome/navidrome/conf" - "github.com/navidrome/navidrome/core" "github.com/navidrome/navidrome/core/artwork" + "github.com/navidrome/navidrome/core/playlists" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/request" @@ -21,12 +21,12 @@ type phasePlaylists struct { ctx context.Context scanState *scanState ds model.DataStore - pls core.Playlists + pls playlists.Playlists cw artwork.CacheWarmer refreshed atomic.Uint32 } -func createPhasePlaylists(ctx context.Context, scanState *scanState, ds model.DataStore, pls core.Playlists, cw artwork.CacheWarmer) *phasePlaylists { +func createPhasePlaylists(ctx context.Context, scanState *scanState, ds model.DataStore, pls playlists.Playlists, cw artwork.CacheWarmer) *phasePlaylists { return &phasePlaylists{ ctx: ctx, scanState: scanState, diff --git a/scanner/phase_4_playlists_test.go b/scanner/phase_4_playlists_test.go index 218aa3c7..0b50d39c 100644 --- a/scanner/phase_4_playlists_test.go +++ b/scanner/phase_4_playlists_test.go @@ -9,8 +9,8 @@ import ( "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/conf/configtest" - "github.com/navidrome/navidrome/core" "github.com/navidrome/navidrome/core/artwork" + "github.com/navidrome/navidrome/core/playlists" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/tests" @@ -130,7 +130,7 @@ var _ = Describe("phasePlaylists", func() { type mockPlaylists struct { mock.Mock - core.Playlists + playlists.Playlists } func (p *mockPlaylists) ImportFile(ctx context.Context, folder *model.Folder, filename string) (*model.Playlist, error) { diff --git a/scanner/scanner.go b/scanner/scanner.go index ba1e76ff..871b0c69 100644 --- a/scanner/scanner.go +++ b/scanner/scanner.go @@ -11,8 +11,8 @@ import ( ppl "github.com/google/go-pipeline/pkg/pipeline" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/consts" - "github.com/navidrome/navidrome/core" "github.com/navidrome/navidrome/core/artwork" + "github.com/navidrome/navidrome/core/playlists" "github.com/navidrome/navidrome/db" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" @@ -23,7 +23,7 @@ import ( type scannerImpl struct { ds model.DataStore cw artwork.CacheWarmer - pls core.Playlists + pls playlists.Playlists } // scanState holds the state of an in-progress scan, to be passed to the various phases diff --git a/scanner/scanner_benchmark_test.go b/scanner/scanner_benchmark_test.go index 2b1c0a14..1ac7b50a 100644 --- a/scanner/scanner_benchmark_test.go +++ b/scanner/scanner_benchmark_test.go @@ -12,9 +12,9 @@ import ( "github.com/dustin/go-humanize" "github.com/google/uuid" "github.com/navidrome/navidrome/conf" - "github.com/navidrome/navidrome/core" "github.com/navidrome/navidrome/core/artwork" "github.com/navidrome/navidrome/core/metrics" + "github.com/navidrome/navidrome/core/playlists" "github.com/navidrome/navidrome/core/storage/storagetest" "github.com/navidrome/navidrome/db" "github.com/navidrome/navidrome/model" @@ -40,7 +40,7 @@ func BenchmarkScan(b *testing.B) { ds := persistence.New(db.Db()) conf.Server.DevExternalScanner = false s := scanner.New(context.Background(), ds, artwork.NoopCacheWarmer(), events.NoopBroker(), - core.NewPlaylists(ds), metrics.NewNoopInstance()) + playlists.NewPlaylists(ds), metrics.NewNoopInstance()) fs := storagetest.FakeFS{} storagetest.Register("fake", &fs) diff --git a/scanner/scanner_multilibrary_test.go b/scanner/scanner_multilibrary_test.go index 107e66a9..6990f198 100644 --- a/scanner/scanner_multilibrary_test.go +++ b/scanner/scanner_multilibrary_test.go @@ -11,9 +11,9 @@ import ( "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/conf/configtest" "github.com/navidrome/navidrome/consts" - "github.com/navidrome/navidrome/core" "github.com/navidrome/navidrome/core/artwork" "github.com/navidrome/navidrome/core/metrics" + "github.com/navidrome/navidrome/core/playlists" "github.com/navidrome/navidrome/core/storage/storagetest" "github.com/navidrome/navidrome/db" "github.com/navidrome/navidrome/log" @@ -77,7 +77,7 @@ var _ = Describe("Scanner - Multi-Library", Ordered, func() { Expect(ds.User(ctx).Put(&adminUser)).To(Succeed()) s = scanner.New(ctx, ds, artwork.NoopCacheWarmer(), events.NoopBroker(), - core.NewPlaylists(ds), metrics.NewNoopInstance()) + playlists.NewPlaylists(ds), metrics.NewNoopInstance()) // Create two test libraries (let DB auto-assign IDs) lib1 = model.Library{Name: "Rock Collection", Path: "rock:///music"} diff --git a/scanner/scanner_selective_test.go b/scanner/scanner_selective_test.go index 629826db..6e451117 100644 --- a/scanner/scanner_selective_test.go +++ b/scanner/scanner_selective_test.go @@ -8,9 +8,9 @@ import ( "github.com/Masterminds/squirrel" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/conf/configtest" - "github.com/navidrome/navidrome/core" "github.com/navidrome/navidrome/core/artwork" "github.com/navidrome/navidrome/core/metrics" + "github.com/navidrome/navidrome/core/playlists" "github.com/navidrome/navidrome/core/storage/storagetest" "github.com/navidrome/navidrome/db" "github.com/navidrome/navidrome/log" @@ -63,7 +63,7 @@ var _ = Describe("ScanFolders", Ordered, func() { Expect(ds.User(ctx).Put(&adminUser)).To(Succeed()) s = scanner.New(ctx, ds, artwork.NoopCacheWarmer(), events.NoopBroker(), - core.NewPlaylists(ds), metrics.NewNoopInstance()) + playlists.NewPlaylists(ds), metrics.NewNoopInstance()) lib = model.Library{ID: 1, Name: "Fake Library", Path: "fake:///music"} Expect(ds.Library(ctx).Put(&lib)).To(Succeed()) diff --git a/scanner/scanner_test.go b/scanner/scanner_test.go index 351255ae..d5688a1d 100644 --- a/scanner/scanner_test.go +++ b/scanner/scanner_test.go @@ -11,9 +11,9 @@ import ( "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/conf/configtest" "github.com/navidrome/navidrome/consts" - "github.com/navidrome/navidrome/core" "github.com/navidrome/navidrome/core/artwork" "github.com/navidrome/navidrome/core/metrics" + "github.com/navidrome/navidrome/core/playlists" "github.com/navidrome/navidrome/core/storage/storagetest" "github.com/navidrome/navidrome/db" "github.com/navidrome/navidrome/log" @@ -84,7 +84,7 @@ var _ = Describe("Scanner", Ordered, func() { Expect(ds.User(ctx).Put(&adminUser)).To(Succeed()) s = scanner.New(ctx, ds, artwork.NoopCacheWarmer(), events.NoopBroker(), - core.NewPlaylists(ds), metrics.NewNoopInstance()) + playlists.NewPlaylists(ds), metrics.NewNoopInstance()) lib = model.Library{ID: 1, Name: "Fake Library", Path: "fake:///music"} Expect(ds.Library(ctx).Put(&lib)).To(Succeed()) diff --git a/server/e2e/e2e_suite_test.go b/server/e2e/e2e_suite_test.go index 92214950..7bda1527 100644 --- a/server/e2e/e2e_suite_test.go +++ b/server/e2e/e2e_suite_test.go @@ -21,6 +21,7 @@ import ( "github.com/navidrome/navidrome/core/external" "github.com/navidrome/navidrome/core/metrics" "github.com/navidrome/navidrome/core/playback" + "github.com/navidrome/navidrome/core/playlists" "github.com/navidrome/navidrome/core/scrobbler" "github.com/navidrome/navidrome/core/storage/storagetest" "github.com/navidrome/navidrome/db" @@ -69,6 +70,14 @@ var ( Name: "Admin User", IsAdmin: true, } + + // Regular (non-admin) user for permission tests + regularUser = model.User{ + ID: "regular-1", + UserName: "regular", + Name: "Regular User", + IsAdmin: false, + } ) func createFS(files fstest.MapFS) storagetest.FakeFS { @@ -288,19 +297,29 @@ var _ = BeforeSuite(func() { adminUserWithPass.NewPassword = "password" Expect(initDS.User(ctx).Put(&adminUserWithPass)).To(Succeed()) + regularUserWithPass := regularUser + regularUserWithPass.NewPassword = "password" + Expect(initDS.User(ctx).Put(®ularUserWithPass)).To(Succeed()) + lib = model.Library{ID: 1, Name: "Music Library", Path: "fake:///music"} Expect(initDS.Library(ctx).Put(&lib)).To(Succeed()) Expect(initDS.User(ctx).SetUserLibraries(adminUser.ID, []int{lib.ID})).To(Succeed()) + Expect(initDS.User(ctx).SetUserLibraries(regularUser.ID, []int{lib.ID})).To(Succeed()) loadedUser, err := initDS.User(ctx).FindByUsername(adminUser.UserName) Expect(err).ToNot(HaveOccurred()) adminUser.Libraries = loadedUser.Libraries + + loadedRegular, err := initDS.User(ctx).FindByUsername(regularUser.UserName) + Expect(err).ToNot(HaveOccurred()) + regularUser.Libraries = loadedRegular.Libraries + ctx = request.WithUser(GinkgoT().Context(), adminUser) buildTestFS() s := scanner.New(ctx, initDS, artwork.NoopCacheWarmer(), events.NoopBroker(), - core.NewPlaylists(initDS), metrics.NewNoopInstance()) + playlists.NewPlaylists(initDS), metrics.NewNoopInstance()) _, err = s.ScanAll(ctx, true) Expect(err).ToNot(HaveOccurred()) @@ -334,7 +353,7 @@ func setupTestDB() { // Create the Subsonic Router with real DS + noop stubs s := scanner.New(ctx, ds, artwork.NoopCacheWarmer(), events.NoopBroker(), - core.NewPlaylists(ds), metrics.NewNoopInstance()) + playlists.NewPlaylists(ds), metrics.NewNoopInstance()) router = subsonic.New( ds, noopArtwork{}, @@ -344,7 +363,7 @@ func setupTestDB() { noopProvider{}, s, events.NoopBroker(), - core.NewPlaylists(ds), + playlists.NewPlaylists(ds), noopPlayTracker{}, core.NewShare(ds), playback.PlaybackServer(nil), diff --git a/server/e2e/subsonic_multilibrary_test.go b/server/e2e/subsonic_multilibrary_test.go index 2292bfab..1eb04d41 100644 --- a/server/e2e/subsonic_multilibrary_test.go +++ b/server/e2e/subsonic_multilibrary_test.go @@ -6,9 +6,9 @@ import ( "github.com/Masterminds/squirrel" "github.com/navidrome/navidrome/conf" - "github.com/navidrome/navidrome/core" "github.com/navidrome/navidrome/core/artwork" "github.com/navidrome/navidrome/core/metrics" + "github.com/navidrome/navidrome/core/playlists" "github.com/navidrome/navidrome/core/storage/storagetest" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/scanner" @@ -53,7 +53,7 @@ var _ = Describe("Multi-Library Support", Ordered, func() { // Run incremental scan to import lib2 content (lib1 files unchanged → skipped) s := scanner.New(ctx, ds, artwork.NoopCacheWarmer(), events.NoopBroker(), - core.NewPlaylists(ds), metrics.NewNoopInstance()) + playlists.NewPlaylists(ds), metrics.NewNoopInstance()) _, err = s.ScanAll(ctx, false) Expect(err).ToNot(HaveOccurred()) diff --git a/server/e2e/subsonic_playlists_test.go b/server/e2e/subsonic_playlists_test.go index 6e9c2376..3468979f 100644 --- a/server/e2e/subsonic_playlists_test.go +++ b/server/e2e/subsonic_playlists_test.go @@ -1,7 +1,11 @@ package e2e import ( + "time" + + "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/criteria" "github.com/navidrome/navidrome/server/subsonic/responses" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -15,9 +19,9 @@ var _ = Describe("Playlist Endpoints", Ordered, func() { setupTestDB() // Look up song IDs from scanned data for playlist operations - songs, err := ds.MediaFile(ctx).GetAll(model.QueryOptions{Sort: "title", Max: 3}) + songs, err := ds.MediaFile(ctx).GetAll(model.QueryOptions{Sort: "title", Max: 6}) Expect(err).ToNot(HaveOccurred()) - Expect(len(songs)).To(BeNumerically(">=", 3)) + Expect(len(songs)).To(BeNumerically(">=", 5)) for _, s := range songs { songIDs = append(songIDs, s.ID) } @@ -32,24 +36,30 @@ var _ = Describe("Playlist Endpoints", Ordered, func() { }) It("createPlaylist creates a new playlist with songs", func() { - resp := doReq("createPlaylist", "name", "Test Playlist", "songId", songIDs[0], "songId", songIDs[1]) + resp := doReq("createPlaylist", "name", "Test Playlist", + "songId", songIDs[0], "songId", songIDs[1], "songId", songIDs[2]) Expect(resp.Status).To(Equal(responses.StatusOK)) Expect(resp.Playlist).ToNot(BeNil()) Expect(resp.Playlist.Name).To(Equal("Test Playlist")) - Expect(resp.Playlist.SongCount).To(Equal(int32(2))) + Expect(resp.Playlist.SongCount).To(Equal(int32(3))) + Expect(resp.Playlist.Entry).To(HaveLen(3)) + Expect(resp.Playlist.Entry[0].Id).To(Equal(songIDs[0])) + Expect(resp.Playlist.Entry[1].Id).To(Equal(songIDs[1])) + Expect(resp.Playlist.Entry[2].Id).To(Equal(songIDs[2])) playlistID = resp.Playlist.Id }) - It("getPlaylist returns playlist with tracks", func() { + It("getPlaylist returns playlist with tracks in order", func() { resp := doReq("getPlaylist", "id", playlistID) Expect(resp.Status).To(Equal(responses.StatusOK)) Expect(resp.Playlist).ToNot(BeNil()) Expect(resp.Playlist.Name).To(Equal("Test Playlist")) - Expect(resp.Playlist.Entry).To(HaveLen(2)) + Expect(resp.Playlist.Entry).To(HaveLen(3)) Expect(resp.Playlist.Entry[0].Id).To(Equal(songIDs[0])) Expect(resp.Playlist.Entry[1].Id).To(Equal(songIDs[1])) + Expect(resp.Playlist.Entry[2].Id).To(Equal(songIDs[2])) }) It("createPlaylist without name or playlistId returns error", func() { @@ -59,40 +69,150 @@ var _ = Describe("Playlist Endpoints", Ordered, func() { Expect(resp.Error).ToNot(BeNil()) }) + It("createPlaylist with playlistId replaces tracks on existing playlist", func() { + // Replace tracks: the playlist had [song0, song1, song2], replace with [song3, song4] + resp := doReq("createPlaylist", "playlistId", playlistID, + "songId", songIDs[3], "songId", songIDs[4]) + + Expect(resp.Status).To(Equal(responses.StatusOK)) + Expect(resp.Playlist).ToNot(BeNil()) + Expect(resp.Playlist.Id).To(Equal(playlistID)) + Expect(resp.Playlist.SongCount).To(Equal(int32(2))) + Expect(resp.Playlist.Entry).To(HaveLen(2)) + Expect(resp.Playlist.Entry[0].Id).To(Equal(songIDs[3])) + Expect(resp.Playlist.Entry[1].Id).To(Equal(songIDs[4])) + }) + It("updatePlaylist can rename the playlist", func() { resp := doReq("updatePlaylist", "playlistId", playlistID, "name", "Renamed Playlist") - Expect(resp.Status).To(Equal(responses.StatusOK)) // Verify the rename resp = doReq("getPlaylist", "id", playlistID) - Expect(resp.Playlist.Name).To(Equal("Renamed Playlist")) + // Tracks should be unchanged + Expect(resp.Playlist.SongCount).To(Equal(int32(2))) + }) + + It("updatePlaylist can set comment", func() { + resp := doReq("updatePlaylist", "playlistId", playlistID, "comment", "My favorite songs") + Expect(resp.Status).To(Equal(responses.StatusOK)) + + resp = doReq("getPlaylist", "id", playlistID) + Expect(resp.Playlist.Comment).To(Equal("My favorite songs")) + }) + + It("updatePlaylist can set public visibility", func() { + resp := doReq("updatePlaylist", "playlistId", playlistID, "public", "true") + Expect(resp.Status).To(Equal(responses.StatusOK)) + + resp = doReq("getPlaylist", "id", playlistID) + Expect(resp.Playlist.Public).To(BeTrue()) }) It("updatePlaylist can add songs", func() { - resp := doReq("updatePlaylist", "playlistId", playlistID, "songIdToAdd", songIDs[2]) - + // Playlist currently has [song3, song4], add song0 + resp := doReq("updatePlaylist", "playlistId", playlistID, "songIdToAdd", songIDs[0]) Expect(resp.Status).To(Equal(responses.StatusOK)) - // Verify the song was added resp = doReq("getPlaylist", "id", playlistID) - Expect(resp.Playlist.SongCount).To(Equal(int32(3))) Expect(resp.Playlist.Entry).To(HaveLen(3)) + Expect(resp.Playlist.Entry[0].Id).To(Equal(songIDs[3])) + Expect(resp.Playlist.Entry[1].Id).To(Equal(songIDs[4])) + Expect(resp.Playlist.Entry[2].Id).To(Equal(songIDs[0])) }) - It("updatePlaylist can remove songs by index", func() { - // Remove the first song (index 0) - resp := doReq("updatePlaylist", "playlistId", playlistID, "songIndexToRemove", "0") - + It("updatePlaylist can add multiple songs at once", func() { + // Playlist currently has [song3, song4, song0], add song1 and song2 + resp := doReq("updatePlaylist", "playlistId", playlistID, + "songIdToAdd", songIDs[1], "songIdToAdd", songIDs[2]) Expect(resp.Status).To(Equal(responses.StatusOK)) - // Verify the song was removed resp = doReq("getPlaylist", "id", playlistID) + Expect(resp.Playlist.SongCount).To(Equal(int32(5))) + Expect(resp.Playlist.Entry).To(HaveLen(5)) + }) + It("updatePlaylist can remove songs by index and verifies correct songs remain", func() { + // Playlist has [song3, song4, song0, song1, song2] + // Remove index 0 (song3) and index 2 (song0) + resp := doReq("updatePlaylist", "playlistId", playlistID, + "songIndexToRemove", "0", "songIndexToRemove", "2") + Expect(resp.Status).To(Equal(responses.StatusOK)) + + resp = doReq("getPlaylist", "id", playlistID) + Expect(resp.Playlist.SongCount).To(Equal(int32(3))) + Expect(resp.Playlist.Entry).To(HaveLen(3)) + Expect(resp.Playlist.Entry[0].Id).To(Equal(songIDs[4])) + Expect(resp.Playlist.Entry[1].Id).To(Equal(songIDs[1])) + Expect(resp.Playlist.Entry[2].Id).To(Equal(songIDs[2])) + }) + + It("updatePlaylist can remove and add songs in a single call", func() { + // Playlist has [song4, song1, song2] + // Remove index 1 (song1) and add song3 + resp := doReq("updatePlaylist", "playlistId", playlistID, + "songIndexToRemove", "1", "songIdToAdd", songIDs[3]) + Expect(resp.Status).To(Equal(responses.StatusOK)) + + resp = doReq("getPlaylist", "id", playlistID) + Expect(resp.Playlist.SongCount).To(Equal(int32(3))) + Expect(resp.Playlist.Entry).To(HaveLen(3)) + Expect(resp.Playlist.Entry[0].Id).To(Equal(songIDs[4])) + Expect(resp.Playlist.Entry[1].Id).To(Equal(songIDs[2])) + Expect(resp.Playlist.Entry[2].Id).To(Equal(songIDs[3])) + }) + + It("updatePlaylist can combine metadata change with track removal", func() { + // Playlist has [song4, song2, song3] + // Rename + remove index 0 (song4) + resp := doReq("updatePlaylist", "playlistId", playlistID, + "name", "Final Playlist", "songIndexToRemove", "0") + Expect(resp.Status).To(Equal(responses.StatusOK)) + + resp = doReq("getPlaylist", "id", playlistID) + Expect(resp.Playlist.Name).To(Equal("Final Playlist")) Expect(resp.Playlist.SongCount).To(Equal(int32(2))) - Expect(resp.Playlist.Entry).To(HaveLen(2)) + Expect(resp.Playlist.Entry[0].Id).To(Equal(songIDs[2])) + Expect(resp.Playlist.Entry[1].Id).To(Equal(songIDs[3])) + }) + + It("updatePlaylist can remove all songs from playlist", func() { + // Playlist has [song2, song3] — remove both + resp := doReq("updatePlaylist", "playlistId", playlistID, + "songIndexToRemove", "0", "songIndexToRemove", "1") + Expect(resp.Status).To(Equal(responses.StatusOK)) + + resp = doReq("getPlaylist", "id", playlistID) + Expect(resp.Playlist.SongCount).To(Equal(int32(0))) + Expect(resp.Playlist.Entry).To(BeEmpty()) + }) + + It("updatePlaylist can add songs to an empty playlist", func() { + resp := doReq("updatePlaylist", "playlistId", playlistID, + "songIdToAdd", songIDs[0]) + Expect(resp.Status).To(Equal(responses.StatusOK)) + + resp = doReq("getPlaylist", "id", playlistID) + Expect(resp.Playlist.SongCount).To(Equal(int32(1))) + Expect(resp.Playlist.Entry).To(HaveLen(1)) + Expect(resp.Playlist.Entry[0].Id).To(Equal(songIDs[0])) + }) + + It("updatePlaylist without playlistId returns error", func() { + resp := doReq("updatePlaylist", "name", "No ID") + + Expect(resp.Status).To(Equal(responses.StatusFailed)) + Expect(resp.Error).ToNot(BeNil()) + }) + + It("getPlaylists shows the playlist", func() { + resp := doReq("getPlaylists") + + Expect(resp.Status).To(Equal(responses.StatusOK)) + Expect(resp.Playlists.Playlist).To(HaveLen(1)) + Expect(resp.Playlists.Playlist[0].Id).To(Equal(playlistID)) }) It("deletePlaylist removes the playlist", func() { @@ -107,4 +227,294 @@ var _ = Describe("Playlist Endpoints", Ordered, func() { Expect(resp.Status).To(Equal(responses.StatusFailed)) Expect(resp.Error).ToNot(BeNil()) }) + + It("getPlaylists returns empty after deletion", func() { + resp := doReq("getPlaylists") + + Expect(resp.Status).To(Equal(responses.StatusOK)) + Expect(resp.Playlists.Playlist).To(BeEmpty()) + }) + + Describe("Playlist Permissions", Ordered, func() { + var songIDs []string + var adminPrivateID string + var adminPublicID string + var regularPlaylistID string + + BeforeAll(func() { + setupTestDB() + + songs, err := ds.MediaFile(ctx).GetAll(model.QueryOptions{Sort: "title", Max: 6}) + Expect(err).ToNot(HaveOccurred()) + Expect(len(songs)).To(BeNumerically(">=", 3)) + for _, s := range songs { + songIDs = append(songIDs, s.ID) + } + }) + + It("admin creates a private playlist", func() { + resp := doReqWithUser(adminUser, "createPlaylist", "name", "Admin Private", + "songId", songIDs[0], "songId", songIDs[1]) + + Expect(resp.Status).To(Equal(responses.StatusOK)) + adminPrivateID = resp.Playlist.Id + }) + + It("admin creates a public playlist", func() { + resp := doReqWithUser(adminUser, "createPlaylist", "name", "Admin Public", + "songId", songIDs[0], "songId", songIDs[1]) + + Expect(resp.Status).To(Equal(responses.StatusOK)) + adminPublicID = resp.Playlist.Id + + // Make it public + resp = doReqWithUser(adminUser, "updatePlaylist", + "playlistId", adminPublicID, "public", "true") + Expect(resp.Status).To(Equal(responses.StatusOK)) + }) + + It("regular user creates a playlist", func() { + resp := doReqWithUser(regularUser, "createPlaylist", "name", "Regular Playlist", + "songId", songIDs[0]) + + Expect(resp.Status).To(Equal(responses.StatusOK)) + regularPlaylistID = resp.Playlist.Id + }) + + // --- Private playlist: regular user gets "not found" (repo hides it entirely) --- + + It("regular user cannot see admin's private playlist", func() { + resp := doReqWithUser(regularUser, "getPlaylist", "id", adminPrivateID) + + Expect(resp.Status).To(Equal(responses.StatusFailed)) + Expect(resp.Error).ToNot(BeNil()) + }) + + It("regular user cannot update admin's private playlist (not found)", func() { + resp := doReqWithUser(regularUser, "updatePlaylist", + "playlistId", adminPrivateID, "name", "Hacked") + + Expect(resp.Status).To(Equal(responses.StatusFailed)) + Expect(resp.Error).ToNot(BeNil()) + }) + + It("regular user cannot delete admin's private playlist (not found)", func() { + resp := doReqWithUser(regularUser, "deletePlaylist", "id", adminPrivateID) + + Expect(resp.Status).To(Equal(responses.StatusFailed)) + Expect(resp.Error).ToNot(BeNil()) + }) + + // --- Public playlist: regular user can see but cannot modify (authorization fail, code 50) --- + + It("regular user can see admin's public playlist", func() { + resp := doReqWithUser(regularUser, "getPlaylist", "id", adminPublicID) + + Expect(resp.Status).To(Equal(responses.StatusOK)) + Expect(resp.Playlist.Name).To(Equal("Admin Public")) + }) + + It("regular user cannot update admin's public playlist", func() { + resp := doReqWithUser(regularUser, "updatePlaylist", + "playlistId", adminPublicID, "name", "Hacked") + + Expect(resp.Status).To(Equal(responses.StatusFailed)) + Expect(resp.Error).ToNot(BeNil()) + Expect(resp.Error.Code).To(Equal(int32(50))) + }) + + It("regular user cannot add songs to admin's public playlist", func() { + resp := doReqWithUser(regularUser, "updatePlaylist", + "playlistId", adminPublicID, "songIdToAdd", songIDs[2]) + + Expect(resp.Status).To(Equal(responses.StatusFailed)) + Expect(resp.Error.Code).To(Equal(int32(50))) + }) + + It("regular user cannot remove songs from admin's public playlist", func() { + resp := doReqWithUser(regularUser, "updatePlaylist", + "playlistId", adminPublicID, "songIndexToRemove", "0") + + Expect(resp.Status).To(Equal(responses.StatusFailed)) + Expect(resp.Error.Code).To(Equal(int32(50))) + }) + + It("regular user cannot delete admin's public playlist", func() { + resp := doReqWithUser(regularUser, "deletePlaylist", "id", adminPublicID) + + Expect(resp.Status).To(Equal(responses.StatusFailed)) + Expect(resp.Error.Code).To(Equal(int32(50))) + }) + + It("regular user cannot replace tracks on admin's public playlist via createPlaylist", func() { + resp := doReqWithUser(regularUser, "createPlaylist", + "playlistId", adminPublicID, "songId", songIDs[2]) + + Expect(resp.Status).To(Equal(responses.StatusFailed)) + Expect(resp.Error).ToNot(BeNil()) + }) + + // --- Regular user can manage their own playlists --- + + It("regular user can update their own playlist", func() { + resp := doReqWithUser(regularUser, "updatePlaylist", + "playlistId", regularPlaylistID, "name", "My Updated Playlist") + + Expect(resp.Status).To(Equal(responses.StatusOK)) + + resp = doReqWithUser(regularUser, "getPlaylist", "id", regularPlaylistID) + Expect(resp.Playlist.Name).To(Equal("My Updated Playlist")) + }) + + It("regular user can add songs to their own playlist", func() { + resp := doReqWithUser(regularUser, "updatePlaylist", + "playlistId", regularPlaylistID, "songIdToAdd", songIDs[1]) + + Expect(resp.Status).To(Equal(responses.StatusOK)) + + resp = doReqWithUser(regularUser, "getPlaylist", "id", regularPlaylistID) + Expect(resp.Playlist.SongCount).To(Equal(int32(2))) + }) + + It("regular user can delete their own playlist", func() { + resp := doReqWithUser(regularUser, "deletePlaylist", "id", regularPlaylistID) + + Expect(resp.Status).To(Equal(responses.StatusOK)) + }) + + // --- Admin can manage any user's playlists --- + + It("admin can update any user's playlist", func() { + resp := doReqWithUser(regularUser, "createPlaylist", "name", "To Be Admin-Edited", + "songId", songIDs[0]) + Expect(resp.Status).To(Equal(responses.StatusOK)) + plsID := resp.Playlist.Id + + resp = doReqWithUser(adminUser, "updatePlaylist", + "playlistId", plsID, "name", "Admin Edited") + Expect(resp.Status).To(Equal(responses.StatusOK)) + + resp = doReqWithUser(adminUser, "getPlaylist", "id", plsID) + Expect(resp.Playlist.Name).To(Equal("Admin Edited")) + }) + + It("admin can delete any user's playlist", func() { + resp := doReqWithUser(regularUser, "createPlaylist", "name", "To Be Admin-Deleted", + "songId", songIDs[0]) + Expect(resp.Status).To(Equal(responses.StatusOK)) + plsID := resp.Playlist.Id + + resp = doReqWithUser(adminUser, "deletePlaylist", "id", plsID) + Expect(resp.Status).To(Equal(responses.StatusOK)) + + resp = doReqWithUser(adminUser, "getPlaylist", "id", plsID) + Expect(resp.Status).To(Equal(responses.StatusFailed)) + }) + + // --- Verify admin's playlists are unchanged --- + + It("admin's private playlist is unchanged after failed regular user operations", func() { + resp := doReqWithUser(adminUser, "getPlaylist", "id", adminPrivateID) + + Expect(resp.Status).To(Equal(responses.StatusOK)) + Expect(resp.Playlist.Name).To(Equal("Admin Private")) + Expect(resp.Playlist.SongCount).To(Equal(int32(2))) + }) + + It("admin's public playlist is unchanged after failed regular user operations", func() { + resp := doReqWithUser(adminUser, "getPlaylist", "id", adminPublicID) + + Expect(resp.Status).To(Equal(responses.StatusOK)) + Expect(resp.Playlist.Name).To(Equal("Admin Public")) + Expect(resp.Playlist.SongCount).To(Equal(int32(2))) + }) + }) + + Describe("Smart Playlist Protection", Ordered, func() { + var smartPlaylistID string + var songID string + + BeforeAll(func() { + setupTestDB() + + // Look up a song ID for mutation tests + songs, err := ds.MediaFile(ctx).GetAll(model.QueryOptions{Sort: "title", Max: 1}) + Expect(err).ToNot(HaveOccurred()) + Expect(songs).ToNot(BeEmpty()) + songID = songs[0].ID + + // Insert a smart playlist directly into the DB + smartPls := &model.Playlist{ + Name: "Smart Playlist", + OwnerID: adminUser.ID, + Public: false, + Rules: &criteria.Criteria{Expression: criteria.Contains{"title": ""}}, + } + Expect(ds.Playlist(ctx).Put(smartPls)).To(Succeed()) + smartPlaylistID = smartPls.ID + }) + + It("getPlaylist returns smart playlist with readonly flag and validUntil", func() { + resp := doReq("getPlaylist", "id", smartPlaylistID) + + Expect(resp.Status).To(Equal(responses.StatusOK)) + Expect(resp.Playlist.Name).To(Equal("Smart Playlist")) + Expect(resp.Playlist.OpenSubsonicPlaylist).ToNot(BeNil()) + Expect(resp.Playlist.OpenSubsonicPlaylist.Readonly).To(BeTrue()) + expectedValidUntil := time.Now().Add(conf.Server.SmartPlaylistRefreshDelay) + Expect(*resp.Playlist.OpenSubsonicPlaylist.ValidUntil).To(BeTemporally("~", expectedValidUntil, time.Second)) + }) + + It("createPlaylist rejects replacing tracks on smart playlist", func() { + resp := doReq("createPlaylist", "playlistId", smartPlaylistID, "songId", songID) + + Expect(resp.Status).To(Equal(responses.StatusFailed)) + Expect(resp.Error).ToNot(BeNil()) + Expect(resp.Error.Code).To(Equal(int32(50))) + }) + + It("updatePlaylist rejects adding songs to smart playlist", func() { + resp := doReq("updatePlaylist", "playlistId", smartPlaylistID, + "songIdToAdd", songID) + + Expect(resp.Status).To(Equal(responses.StatusFailed)) + Expect(resp.Error).ToNot(BeNil()) + Expect(resp.Error.Code).To(Equal(int32(50))) + }) + + It("updatePlaylist rejects removing songs from smart playlist", func() { + resp := doReq("updatePlaylist", "playlistId", smartPlaylistID, + "songIndexToRemove", "0") + + Expect(resp.Status).To(Equal(responses.StatusFailed)) + Expect(resp.Error).ToNot(BeNil()) + Expect(resp.Error.Code).To(Equal(int32(50))) + }) + + It("updatePlaylist allows renaming smart playlist", func() { + resp := doReq("updatePlaylist", "playlistId", smartPlaylistID, + "name", "Renamed Smart") + Expect(resp.Status).To(Equal(responses.StatusOK)) + + resp = doReq("getPlaylist", "id", smartPlaylistID) + Expect(resp.Playlist.Name).To(Equal("Renamed Smart")) + }) + + It("updatePlaylist allows setting comment on smart playlist", func() { + resp := doReq("updatePlaylist", "playlistId", smartPlaylistID, + "comment", "Auto-generated playlist") + Expect(resp.Status).To(Equal(responses.StatusOK)) + + resp = doReq("getPlaylist", "id", smartPlaylistID) + Expect(resp.Playlist.Comment).To(Equal("Auto-generated playlist")) + }) + + It("deletePlaylist can delete smart playlist", func() { + resp := doReq("deletePlaylist", "id", smartPlaylistID) + Expect(resp.Status).To(Equal(responses.StatusOK)) + + resp = doReq("getPlaylist", "id", smartPlaylistID) + Expect(resp.Status).To(Equal(responses.StatusFailed)) + }) + }) }) diff --git a/server/e2e/subsonic_scan_test.go b/server/e2e/subsonic_scan_test.go index a6fb28bc..8bac6fe9 100644 --- a/server/e2e/subsonic_scan_test.go +++ b/server/e2e/subsonic_scan_test.go @@ -22,8 +22,6 @@ var _ = Describe("Scan Endpoints", func() { }) It("startScan requires admin user", func() { - regularUser := createUser("user-2", "regular", "Regular User", false) - resp := doReqWithUser(regularUser, "startScan") Expect(resp.Status).To(Equal(responses.StatusFailed)) diff --git a/server/nativeapi/native_api.go b/server/nativeapi/native_api.go index 52e633be..27b85a60 100644 --- a/server/nativeapi/native_api.go +++ b/server/nativeapi/native_api.go @@ -14,6 +14,7 @@ import ( "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/core" "github.com/navidrome/navidrome/core/metrics" + playlistsvc "github.com/navidrome/navidrome/core/playlists" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/request" @@ -37,7 +38,7 @@ type Router struct { http.Handler ds model.DataStore share core.Share - playlists core.Playlists + playlists playlistsvc.Playlists insights metrics.Insights libs core.Library users core.User @@ -45,7 +46,7 @@ type Router struct { pluginManager PluginManager } -func New(ds model.DataStore, share core.Share, playlists core.Playlists, insights metrics.Insights, libraryService core.Library, userService core.User, maintenance core.Maintenance, pluginManager PluginManager) *Router { +func New(ds model.DataStore, share core.Share, playlists playlistsvc.Playlists, insights metrics.Insights, libraryService core.Library, userService core.User, maintenance core.Maintenance, pluginManager PluginManager) *Router { r := &Router{ds: ds, share: share, playlists: playlists, insights: insights, libs: libraryService, users: userService, maintenance: maintenance, pluginManager: pluginManager} r.Handler = r.routes() return r @@ -121,7 +122,7 @@ func (api *Router) RX(r chi.Router, pathPrefix string, constructor rest.Reposito func (api *Router) addPlaylistRoute(r chi.Router) { constructor := func(ctx context.Context) rest.Repository { - return api.ds.Resource(ctx, model.Playlist{}) + return api.playlists.NewRepository(ctx) } r.Route("/playlist", func(r chi.Router) { @@ -146,26 +147,26 @@ func (api *Router) addPlaylistRoute(r chi.Router) { func (api *Router) addPlaylistTrackRoute(r chi.Router) { r.Route("/playlist/{playlistId}/tracks", func(r chi.Router) { r.Get("/", func(w http.ResponseWriter, r *http.Request) { - getPlaylist(api.ds)(w, r) + getPlaylist(api.playlists)(w, r) }) r.With(server.URLParamsMiddleware).Route("/", func(r chi.Router) { r.Delete("/", func(w http.ResponseWriter, r *http.Request) { - deleteFromPlaylist(api.ds)(w, r) + deleteFromPlaylist(api.playlists)(w, r) }) r.Post("/", func(w http.ResponseWriter, r *http.Request) { - addToPlaylist(api.ds)(w, r) + addToPlaylist(api.playlists)(w, r) }) }) r.Route("/{id}", func(r chi.Router) { r.Use(server.URLParamsMiddleware) r.Get("/", func(w http.ResponseWriter, r *http.Request) { - getPlaylistTrack(api.ds)(w, r) + getPlaylistTrack(api.playlists)(w, r) }) r.Put("/", func(w http.ResponseWriter, r *http.Request) { - reorderItem(api.ds)(w, r) + reorderItem(api.playlists)(w, r) }) r.Delete("/", func(w http.ResponseWriter, r *http.Request) { - deleteFromPlaylist(api.ds)(w, r) + deleteFromPlaylist(api.playlists)(w, r) }) }) }) @@ -173,7 +174,7 @@ func (api *Router) addPlaylistTrackRoute(r chi.Router) { func (api *Router) addSongPlaylistsRoute(r chi.Router) { r.With(server.URLParamsMiddleware).Get("/song/{id}/playlists", func(w http.ResponseWriter, r *http.Request) { - getSongPlaylists(api.ds)(w, r) + getSongPlaylists(api.playlists)(w, r) }) } diff --git a/server/nativeapi/playlists.go b/server/nativeapi/playlists.go index 1e2c5e07..60e8024b 100644 --- a/server/nativeapi/playlists.go +++ b/server/nativeapi/playlists.go @@ -11,7 +11,7 @@ import ( "github.com/deluan/rest" "github.com/go-chi/chi/v5" - "github.com/navidrome/navidrome/core" + "github.com/navidrome/navidrome/core/playlists" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/utils/req" @@ -19,10 +19,10 @@ import ( type restHandler = func(rest.RepositoryConstructor, ...rest.Logger) http.HandlerFunc -func playlistTracksHandler(ds model.DataStore, handler restHandler, refreshSmartPlaylist func(*http.Request) bool) http.HandlerFunc { +func playlistTracksHandler(pls playlists.Playlists, handler restHandler, refreshSmartPlaylist func(*http.Request) bool) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { plsId := chi.URLParam(r, "playlistId") - tracks := ds.Playlist(r.Context()).Tracks(plsId, refreshSmartPlaylist(r)) + tracks := pls.TracksRepository(r.Context(), plsId, refreshSmartPlaylist(r)) if tracks == nil { http.Error(w, "not found", http.StatusNotFound) return @@ -31,27 +31,27 @@ func playlistTracksHandler(ds model.DataStore, handler restHandler, refreshSmart } } -func getPlaylist(ds model.DataStore) http.HandlerFunc { - handler := playlistTracksHandler(ds, rest.GetAll, func(r *http.Request) bool { +func getPlaylist(pls playlists.Playlists) http.HandlerFunc { + handler := playlistTracksHandler(pls, rest.GetAll, func(r *http.Request) bool { return req.Params(r).Int64Or("_start", 0) == 0 }) return func(w http.ResponseWriter, r *http.Request) { if strings.ToLower(r.Header.Get("accept")) == "audio/x-mpegurl" { - handleExportPlaylist(ds)(w, r) + handleExportPlaylist(pls)(w, r) return } handler(w, r) } } -func getPlaylistTrack(ds model.DataStore) http.HandlerFunc { - return playlistTracksHandler(ds, rest.Get, func(*http.Request) bool { return true }) +func getPlaylistTrack(pls playlists.Playlists) http.HandlerFunc { + return playlistTracksHandler(pls, rest.Get, func(*http.Request) bool { return true }) } -func createPlaylistFromM3U(playlists core.Playlists) http.HandlerFunc { +func createPlaylistFromM3U(pls playlists.Playlists) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { ctx := r.Context() - pls, err := playlists.ImportM3U(ctx, r.Body) + pl, err := pls.ImportM3U(ctx, r.Body) if err != nil { log.Error(r.Context(), "Error parsing playlist", err) // TODO: consider returning StatusBadRequest for playlists that are malformed @@ -59,7 +59,7 @@ func createPlaylistFromM3U(playlists core.Playlists) http.HandlerFunc { return } w.WriteHeader(http.StatusCreated) - _, err = w.Write([]byte(pls.ToM3U8())) //nolint:gosec + _, err = w.Write([]byte(pl.ToM3U8())) //nolint:gosec if err != nil { log.Error(ctx, "Error sending m3u contents", err) http.Error(w, err.Error(), http.StatusInternalServerError) @@ -68,45 +68,41 @@ func createPlaylistFromM3U(playlists core.Playlists) http.HandlerFunc { } } -func handleExportPlaylist(ds model.DataStore) http.HandlerFunc { +func handleExportPlaylist(pls playlists.Playlists) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { ctx := r.Context() - plsRepo := ds.Playlist(ctx) plsId := chi.URLParam(r, "playlistId") - pls, err := plsRepo.GetWithTracks(plsId, true, false) + playlist, err := pls.GetWithTracks(ctx, plsId) if errors.Is(err, model.ErrNotFound) { - log.Warn(r.Context(), "Playlist not found", "playlistId", plsId) + log.Warn(ctx, "Playlist not found", "playlistId", plsId) http.Error(w, "not found", http.StatusNotFound) return } if err != nil { - log.Error(r.Context(), "Error retrieving the playlist", "playlistId", plsId, err) + log.Error(ctx, "Error retrieving the playlist", "playlistId", plsId, err) http.Error(w, err.Error(), http.StatusInternalServerError) return } - log.Debug(ctx, "Exporting playlist as M3U", "playlistId", plsId, "name", pls.Name) + log.Debug(ctx, "Exporting playlist as M3U", "playlistId", plsId, "name", playlist.Name) w.Header().Set("Content-Type", "audio/x-mpegurl") - disposition := fmt.Sprintf("attachment; filename=\"%s.m3u\"", pls.Name) + disposition := fmt.Sprintf("attachment; filename=\"%s.m3u\"", playlist.Name) w.Header().Set("Content-Disposition", disposition) - _, err = w.Write([]byte(pls.ToM3U8())) //nolint:gosec + _, err = w.Write([]byte(playlist.ToM3U8())) //nolint:gosec if err != nil { - log.Error(ctx, "Error sending playlist", "name", pls.Name) + log.Error(ctx, "Error sending playlist", "name", playlist.Name) return } } } -func deleteFromPlaylist(ds model.DataStore) http.HandlerFunc { +func deleteFromPlaylist(pls playlists.Playlists) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { p := req.Params(r) playlistId, _ := p.String(":playlistId") ids, _ := p.Strings("id") - err := ds.WithTxImmediate(func(tx model.DataStore) error { - tracksRepo := tx.Playlist(r.Context()).Tracks(playlistId, true) - return tracksRepo.Delete(ids...) - }) + err := pls.RemoveTracks(r.Context(), playlistId, ids) if len(ids) == 1 && errors.Is(err, model.ErrNotFound) { log.Warn(r.Context(), "Track not found in playlist", "playlistId", playlistId, "id", ids[0]) http.Error(w, "not found", http.StatusNotFound) @@ -121,7 +117,7 @@ func deleteFromPlaylist(ds model.DataStore) http.HandlerFunc { } } -func addToPlaylist(ds model.DataStore) http.HandlerFunc { +func addToPlaylist(pls playlists.Playlists) http.HandlerFunc { type addTracksPayload struct { Ids []string `json:"ids"` AlbumIds []string `json:"albumIds"` @@ -130,6 +126,7 @@ func addToPlaylist(ds model.DataStore) http.HandlerFunc { } return func(w http.ResponseWriter, r *http.Request) { + ctx := r.Context() p := req.Params(r) playlistId, _ := p.String(":playlistId") var payload addTracksPayload @@ -138,24 +135,23 @@ func addToPlaylist(ds model.DataStore) http.HandlerFunc { http.Error(w, err.Error(), http.StatusBadRequest) return } - tracksRepo := ds.Playlist(r.Context()).Tracks(playlistId, true) count, c := 0, 0 - if c, err = tracksRepo.Add(payload.Ids); err != nil { + if c, err = pls.AddTracks(ctx, playlistId, payload.Ids); err != nil { http.Error(w, err.Error(), http.StatusBadRequest) return } count += c - if c, err = tracksRepo.AddAlbums(payload.AlbumIds); err != nil { + if c, err = pls.AddAlbums(ctx, playlistId, payload.AlbumIds); err != nil { http.Error(w, err.Error(), http.StatusBadRequest) return } count += c - if c, err = tracksRepo.AddArtists(payload.ArtistIds); err != nil { + if c, err = pls.AddArtists(ctx, playlistId, payload.ArtistIds); err != nil { http.Error(w, err.Error(), http.StatusBadRequest) return } count += c - if c, err = tracksRepo.AddDiscs(payload.Discs); err != nil { + if c, err = pls.AddDiscs(ctx, playlistId, payload.Discs); err != nil { http.Error(w, err.Error(), http.StatusBadRequest) return } @@ -169,12 +165,13 @@ func addToPlaylist(ds model.DataStore) http.HandlerFunc { } } -func reorderItem(ds model.DataStore) http.HandlerFunc { +func reorderItem(pls playlists.Playlists) http.HandlerFunc { type reorderPayload struct { InsertBefore string `json:"insert_before"` } return func(w http.ResponseWriter, r *http.Request) { + ctx := r.Context() p := req.Params(r) playlistId, _ := p.String(":playlistId") id := p.IntOr(":id", 0) @@ -193,9 +190,8 @@ func reorderItem(ds model.DataStore) http.HandlerFunc { http.Error(w, err.Error(), http.StatusBadRequest) return } - tracksRepo := ds.Playlist(r.Context()).Tracks(playlistId, true) - err = tracksRepo.Reorder(id, newPos) - if errors.Is(err, rest.ErrPermissionDenied) { + err = pls.ReorderTrack(ctx, playlistId, id, newPos) + if errors.Is(err, model.ErrNotAuthorized) { http.Error(w, err.Error(), http.StatusForbidden) return } @@ -211,11 +207,11 @@ func reorderItem(ds model.DataStore) http.HandlerFunc { } } -func getSongPlaylists(ds model.DataStore) http.HandlerFunc { +func getSongPlaylists(svc playlists.Playlists) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { p := req.Params(r) trackId, _ := p.String(":id") - playlists, err := ds.Playlist(r.Context()).GetPlaylists(trackId) + playlists, err := svc.GetPlaylists(r.Context(), trackId) if err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) return diff --git a/server/nativeapi/playlists_test.go b/server/nativeapi/playlists_test.go index 319e41cd..961d10b6 100644 --- a/server/nativeapi/playlists_test.go +++ b/server/nativeapi/playlists_test.go @@ -1,6 +1,7 @@ package nativeapi import ( + "context" "encoding/json" "net/http" "net/http/httptest" @@ -11,6 +12,7 @@ import ( "github.com/navidrome/navidrome/conf/configtest" "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/core/auth" + "github.com/navidrome/navidrome/core/playlists" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/server" "github.com/navidrome/navidrome/tests" @@ -48,11 +50,19 @@ func (m *mockPlaylistTrackRepo) Read(id string) (any, error) { return nil, rest.ErrNotFound } +type mockPlaylistsService struct { + playlists.Playlists + tracksRepo rest.Repository +} + +func (m *mockPlaylistsService) TracksRepository(_ context.Context, _ string, _ bool) rest.Repository { + return m.tracksRepo +} + var _ = Describe("Playlist Tracks Endpoint", func() { var ( router http.Handler - ds *tests.MockDataStore - plsRepo *tests.MockPlaylistRepo + plsSvc *mockPlaylistsService userRepo *tests.MockedUserRepo w *httptest.ResponseRecorder ) @@ -61,11 +71,10 @@ var _ = Describe("Playlist Tracks Endpoint", func() { DeferCleanup(configtest.SetupConfig()) conf.Server.SessionTimeout = time.Minute - plsRepo = &tests.MockPlaylistRepo{} + plsSvc = &mockPlaylistsService{} userRepo = tests.CreateMockUserRepo() - ds = &tests.MockDataStore{ - MockedPlaylist: plsRepo, + ds := &tests.MockDataStore{ MockedUser: userRepo, MockedProperty: &tests.MockedPropertyRepo{}, } @@ -82,7 +91,7 @@ var _ = Describe("Playlist Tracks Endpoint", func() { err := userRepo.Put(&testUser) Expect(err).ToNot(HaveOccurred()) - nativeRouter := New(ds, nil, nil, nil, tests.NewMockLibraryService(), tests.NewMockUserService(), nil, nil) + nativeRouter := New(ds, nil, plsSvc, nil, tests.NewMockLibraryService(), tests.NewMockUserService(), nil, nil) router = server.JWTVerifier(nativeRouter) w = httptest.NewRecorder() }) @@ -105,7 +114,7 @@ var _ = Describe("Playlist Tracks Endpoint", func() { }) It("returns tracks when playlist exists", func() { - plsRepo.TracksReturn = &mockPlaylistTrackRepo{ + plsSvc.tracksRepo = &mockPlaylistTrackRepo{ tracks: model.PlaylistTracks{ {ID: "1", MediaFileID: "mf-1", PlaylistID: "pls-1"}, {ID: "2", MediaFileID: "mf-2", PlaylistID: "pls-1"}, @@ -135,7 +144,7 @@ var _ = Describe("Playlist Tracks Endpoint", func() { }) It("returns the track when playlist exists", func() { - plsRepo.TracksReturn = &mockPlaylistTrackRepo{ + plsSvc.tracksRepo = &mockPlaylistTrackRepo{ tracks: model.PlaylistTracks{ {ID: "1", MediaFileID: "mf-1", PlaylistID: "pls-1"}, }, @@ -154,7 +163,7 @@ var _ = Describe("Playlist Tracks Endpoint", func() { }) It("returns 404 when track does not exist in playlist", func() { - plsRepo.TracksReturn = &mockPlaylistTrackRepo{ + plsSvc.tracksRepo = &mockPlaylistTrackRepo{ tracks: model.PlaylistTracks{}, } diff --git a/server/subsonic/api.go b/server/subsonic/api.go index c3108ea5..d0d9bb16 100644 --- a/server/subsonic/api.go +++ b/server/subsonic/api.go @@ -16,6 +16,7 @@ import ( "github.com/navidrome/navidrome/core/external" "github.com/navidrome/navidrome/core/metrics" "github.com/navidrome/navidrome/core/playback" + playlistsvc "github.com/navidrome/navidrome/core/playlists" "github.com/navidrome/navidrome/core/scrobbler" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" @@ -40,7 +41,7 @@ type Router struct { archiver core.Archiver players core.Players provider external.Provider - playlists core.Playlists + playlists playlistsvc.Playlists scanner model.Scanner broker events.Broker scrobbler scrobbler.PlayTracker @@ -51,7 +52,7 @@ type Router struct { func New(ds model.DataStore, artwork artwork.Artwork, streamer core.MediaStreamer, archiver core.Archiver, players core.Players, provider external.Provider, scanner model.Scanner, broker events.Broker, - playlists core.Playlists, scrobbler scrobbler.PlayTracker, share core.Share, playback playback.PlaybackServer, + playlists playlistsvc.Playlists, scrobbler scrobbler.PlayTracker, share core.Share, playback playback.PlaybackServer, metrics metrics.Metrics, ) *Router { r := &Router{ @@ -290,6 +291,8 @@ func mapToSubsonicError(err error) subError { err = newError(responses.ErrorGeneric, err.Error()) case errors.Is(err, model.ErrNotFound): err = newError(responses.ErrorDataNotFound, "data not found") + case errors.Is(err, model.ErrNotAuthorized): + err = newError(responses.ErrorAuthorizationFail) default: err = newError(responses.ErrorGeneric, fmt.Sprintf("Internal Server Error: %s", err)) } diff --git a/server/subsonic/playlists.go b/server/subsonic/playlists.go index b8807563..baae7514 100644 --- a/server/subsonic/playlists.go +++ b/server/subsonic/playlists.go @@ -19,7 +19,7 @@ import ( func (api *Router) GetPlaylists(r *http.Request) (*responses.Subsonic, error) { ctx := r.Context() - allPls, err := api.ds.Playlist(ctx).GetAll(model.QueryOptions{Sort: "name"}) + allPls, err := api.playlists.GetAll(ctx, model.QueryOptions{Sort: "name"}) if err != nil { log.Error(r, err) return nil, err @@ -42,7 +42,7 @@ func (api *Router) GetPlaylist(r *http.Request) (*responses.Subsonic, error) { } func (api *Router) getPlaylist(ctx context.Context, id string) (*responses.Subsonic, error) { - pls, err := api.ds.Playlist(ctx).GetWithTracks(id, true, false) + pls, err := api.playlists.GetWithTracks(ctx, id) if errors.Is(err, model.ErrNotFound) { log.Error(ctx, err.Error(), "id", id) return nil, newError(responses.ErrorDataNotFound, "playlist not found") @@ -60,34 +60,6 @@ func (api *Router) getPlaylist(ctx context.Context, id string) (*responses.Subso return response, nil } -func (api *Router) create(ctx context.Context, playlistId, name string, ids []string) (string, error) { - err := api.ds.WithTxImmediate(func(tx model.DataStore) error { - owner := getUser(ctx) - var pls *model.Playlist - var err error - - if playlistId != "" { - pls, err = tx.Playlist(ctx).Get(playlistId) - if err != nil { - return err - } - if owner.ID != pls.OwnerID { - return model.ErrNotAuthorized - } - } else { - pls = &model.Playlist{Name: name} - pls.OwnerID = owner.ID - } - pls.Tracks = nil - pls.AddMediaFilesByID(ids) - - err = tx.Playlist(ctx).Put(pls) - playlistId = pls.ID - return err - }) - return playlistId, err -} - func (api *Router) CreatePlaylist(r *http.Request) (*responses.Subsonic, error) { ctx := r.Context() p := req.Params(r) @@ -97,7 +69,7 @@ func (api *Router) CreatePlaylist(r *http.Request) (*responses.Subsonic, error) if playlistId == "" && name == "" { return nil, errors.New("required parameter name is missing") } - id, err := api.create(ctx, playlistId, name, songIds) + id, err := api.playlists.Create(ctx, playlistId, name, songIds) if err != nil { log.Error(r, err) return nil, err @@ -111,7 +83,7 @@ func (api *Router) DeletePlaylist(r *http.Request) (*responses.Subsonic, error) if err != nil { return nil, err } - err = api.ds.Playlist(r.Context()).Delete(id) + err = api.playlists.Delete(r.Context(), id) if errors.Is(err, model.ErrNotAuthorized) { return nil, newError(responses.ErrorAuthorizationFail) } diff --git a/server/subsonic/playlists_test.go b/server/subsonic/playlists_test.go index 05701fc1..d99e244b 100644 --- a/server/subsonic/playlists_test.go +++ b/server/subsonic/playlists_test.go @@ -5,7 +5,7 @@ import ( "time" "github.com/navidrome/navidrome/conf" - "github.com/navidrome/navidrome/core" + "github.com/navidrome/navidrome/core/playlists" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/criteria" "github.com/navidrome/navidrome/model/request" @@ -14,7 +14,7 @@ import ( . "github.com/onsi/gomega" ) -var _ core.Playlists = (*fakePlaylists)(nil) +var _ playlists.Playlists = (*fakePlaylists)(nil) var _ = Describe("buildPlaylist", func() { var router *Router @@ -272,7 +272,7 @@ var _ = Describe("UpdatePlaylist", func() { }) type fakePlaylists struct { - core.Playlists + playlists.Playlists lastPlaylistID string lastName *string lastComment *string diff --git a/tests/mock_data_store.go b/tests/mock_data_store.go index c4b0113f..754f0c08 100644 --- a/tests/mock_data_store.go +++ b/tests/mock_data_store.go @@ -121,7 +121,7 @@ func (db *MockDataStore) Playlist(ctx context.Context) model.PlaylistRepository if db.RealDS != nil { return db.RealDS.Playlist(ctx) } - db.MockedPlaylist = &MockPlaylistRepo{} + db.MockedPlaylist = CreateMockPlaylistRepo() return db.MockedPlaylist } diff --git a/tests/mock_playlist_repo.go b/tests/mock_playlist_repo.go index 1c37107e..9bdc5215 100644 --- a/tests/mock_playlist_repo.go +++ b/tests/mock_playlist_repo.go @@ -1,38 +1,111 @@ package tests import ( + "errors" + "github.com/deluan/rest" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/id" ) +func CreateMockPlaylistRepo() *MockPlaylistRepo { + return &MockPlaylistRepo{ + Data: make(map[string]*model.Playlist), + PathMap: make(map[string]*model.Playlist), + } +} + type MockPlaylistRepo struct { model.PlaylistRepository - - Entity *model.Playlist - Error error - TracksReturn model.PlaylistTrackRepository + Data map[string]*model.Playlist // keyed by ID + PathMap map[string]*model.Playlist // keyed by path + Last *model.Playlist + Deleted []string + Err bool + TracksRepo model.PlaylistTrackRepository } -func (m *MockPlaylistRepo) Get(_ string) (*model.Playlist, error) { - if m.Error != nil { - return nil, m.Error +func (m *MockPlaylistRepo) SetError(err bool) { + m.Err = err +} + +func (m *MockPlaylistRepo) Get(id string) (*model.Playlist, error) { + if m.Err { + return nil, errors.New("error") } - if m.Entity == nil { - return nil, model.ErrNotFound + if m.Data != nil { + if pls, ok := m.Data[id]; ok { + return pls, nil + } } - return m.Entity, nil + return nil, model.ErrNotFound +} + +func (m *MockPlaylistRepo) GetWithTracks(id string, _, _ bool) (*model.Playlist, error) { + return m.Get(id) +} + +func (m *MockPlaylistRepo) Put(pls *model.Playlist) error { + if m.Err { + return errors.New("error") + } + if pls.ID == "" { + pls.ID = id.NewRandom() + } + m.Last = pls + if m.Data != nil { + m.Data[pls.ID] = pls + } + return nil +} + +func (m *MockPlaylistRepo) FindByPath(path string) (*model.Playlist, error) { + if m.Err { + return nil, errors.New("error") + } + if m.PathMap != nil { + if pls, ok := m.PathMap[path]; ok { + return pls, nil + } + } + return nil, model.ErrNotFound +} + +func (m *MockPlaylistRepo) Delete(id string) error { + if m.Err { + return errors.New("error") + } + m.Deleted = append(m.Deleted, id) + return nil } func (m *MockPlaylistRepo) Tracks(_ string, _ bool) model.PlaylistTrackRepository { - return m.TracksReturn + return m.TracksRepo +} + +func (m *MockPlaylistRepo) Exists(id string) (bool, error) { + if m.Err { + return false, errors.New("error") + } + if m.Data != nil { + _, found := m.Data[id] + return found, nil + } + return false, nil } func (m *MockPlaylistRepo) Count(_ ...rest.QueryOptions) (int64, error) { - if m.Error != nil { - return 0, m.Error + if m.Err { + return 0, errors.New("error") } - if m.Entity == nil { - return 0, nil - } - return 1, nil + return int64(len(m.Data)), nil } + +func (m *MockPlaylistRepo) CountAll(_ ...model.QueryOptions) (int64, error) { + if m.Err { + return 0, errors.New("error") + } + return int64(len(m.Data)), nil +} + +var _ model.PlaylistRepository = (*MockPlaylistRepo)(nil) diff --git a/tests/mock_playlist_track_repo.go b/tests/mock_playlist_track_repo.go new file mode 100644 index 00000000..c11b077d --- /dev/null +++ b/tests/mock_playlist_track_repo.go @@ -0,0 +1,53 @@ +package tests + +import "github.com/navidrome/navidrome/model" + +type MockPlaylistTrackRepo struct { + model.PlaylistTrackRepository + AddedIds []string + DeletedIds []string + Reordered bool + AddCount int + Err error +} + +func (m *MockPlaylistTrackRepo) Add(ids []string) (int, error) { + m.AddedIds = append(m.AddedIds, ids...) + if m.Err != nil { + return 0, m.Err + } + return m.AddCount, nil +} + +func (m *MockPlaylistTrackRepo) AddAlbums(_ []string) (int, error) { + if m.Err != nil { + return 0, m.Err + } + return m.AddCount, nil +} + +func (m *MockPlaylistTrackRepo) AddArtists(_ []string) (int, error) { + if m.Err != nil { + return 0, m.Err + } + return m.AddCount, nil +} + +func (m *MockPlaylistTrackRepo) AddDiscs(_ []model.DiscID) (int, error) { + if m.Err != nil { + return 0, m.Err + } + return m.AddCount, nil +} + +func (m *MockPlaylistTrackRepo) Delete(ids ...string) error { + m.DeletedIds = append(m.DeletedIds, ids...) + return m.Err +} + +func (m *MockPlaylistTrackRepo) Reorder(_, _ int) error { + m.Reordered = true + return m.Err +} + +var _ model.PlaylistTrackRepository = (*MockPlaylistTrackRepo)(nil)