diff --git a/model/playlist.go b/model/playlist.go index d2ed97682..91fab1b43 100644 --- a/model/playlist.go +++ b/model/playlist.go @@ -221,6 +221,8 @@ type PlaylistRepository interface { FindByPath(ctx context.Context, path string) (*Playlist, error) Tracks(ctx context.Context, playlistId string, refreshSmartPlaylist bool) PlaylistTrackRepository GetPlaylists(ctx context.Context, mediaFileId string) (Playlists, error) + // Evaluate refreshes a smart playlist's tracks as its owner, ignoring visibility and the refresh delay. + Evaluate(ctx context.Context, id string) error } type PlaylistTrack struct { diff --git a/persistence/share_repository.go b/persistence/share_repository.go index 6dd9c3d85..3a9ecfbad 100644 --- a/persistence/share_repository.go +++ b/persistence/share_repository.go @@ -10,7 +10,6 @@ import ( "github.com/deluan/rest" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" - "github.com/navidrome/navidrome/model/request" "github.com/pocketbase/dbx" ) @@ -78,7 +77,7 @@ func (r *shareRepository) loadMedia(ctx context.Context, share *model.Share) err return And{cond, Eq{"missing": false}} } // Load as the share owner so their library access is applied, whoever renders the share. - ownerCtx, err := r.ownerContext(ctx, share) + ownerCtx, err := r.ownerContext(ctx, share.UserID) if err != nil { return err } @@ -128,19 +127,6 @@ func (r *shareRepository) loadMedia(ctx context.Context, share *model.Share) err return nil } -// ownerContext returns a context scoped to the share owner, so repository -// queries apply the owner's library access when a public share is rendered. -func (r *shareRepository) ownerContext(ctx context.Context, share *model.Share) (context.Context, error) { - owner, err := NewUserRepository(r.db).Get(ctx, share.UserID) - if err != nil { - return nil, fmt.Errorf("loading share owner %q: %w", share.UserID, err) - } - if owner == nil { - return nil, fmt.Errorf("share owner %q not found", share.UserID) - } - return request.WithUser(ctx, *owner), nil -} - func sortByIdPosition(mfs model.MediaFiles, ids []string) model.MediaFiles { m := map[string]int{} for i, mf := range mfs { diff --git a/persistence/smart_playlist_repository.go b/persistence/smart_playlist_repository.go index 871f53dd2..602533442 100644 --- a/persistence/smart_playlist_repository.go +++ b/persistence/smart_playlist_repository.go @@ -2,6 +2,7 @@ package persistence import ( "context" + "fmt" "slices" "time" @@ -18,6 +19,23 @@ import ( // in its criteria. To optimize performance, it only refreshes when necessary based on the last evaluated time and // configured refresh delay. +func (r *playlistRepository) Evaluate(ctx context.Context, id string) error { + var res dbPlaylist + if err := r.queryOne(ctx, r.selectPlaylist(ctx).Where(Eq{"playlist.id": id}), &res); err != nil { + return err + } + pls := res.Playlist + ownerCtx, err := r.ownerContext(ctx, pls.OwnerID) + if err != nil { + return err + } + pls.EvaluatedAt = nil + if !r.refreshSmartPlaylist(ownerCtx, &pls) { + return fmt.Errorf("evaluating smart playlist %s", id) + } + return nil +} + // refreshSmartPlaylist evaluates the criteria of a smart playlist and updates its tracks accordingly. func (r *playlistRepository) refreshSmartPlaylist(ctx context.Context, pls *model.Playlist) bool { return r.refreshSmartPlaylistTree(ctx, pls, map[string]struct{}{}) diff --git a/persistence/smart_playlist_repository_test.go b/persistence/smart_playlist_repository_test.go index 4d4c5edb0..5e8890e46 100644 --- a/persistence/smart_playlist_repository_test.go +++ b/persistence/smart_playlist_repository_test.go @@ -330,6 +330,47 @@ var _ = Describe("PlaylistRepository - Smart Playlists", func() { }) }) + Describe("Evaluate", func() { + dayRules := func() *criteria.Criteria { + return &criteria.Criteria{ + Expression: criteria.All{criteria.Contains{"title": "Day"}}, + RefreshDelay: 24 * time.Hour, + } + } + + It("evaluates even when the refresh delay has not elapsed", func() { + evaluatedAt := time.Now().Add(-1 * time.Hour) + pls := model.Playlist{Name: "Evaluate Delay", OwnerID: "userid", Rules: dayRules(), EvaluatedAt: &evaluatedAt} + Expect(repo.Put(ctx, &pls)).To(Succeed()) + DeferCleanup(func() { _ = repo.Delete(ctx, pls.ID) }) + + Expect(repo.Evaluate(ctx, pls.ID)).To(Succeed()) + + got, err := repo.Get(ctx, pls.ID) + Expect(err).ToNot(HaveOccurred()) + Expect(*got.EvaluatedAt).To(BeTemporally("~", time.Now(), 2*time.Second)) + Expect(got.SongCount).To(Equal(1)) + }) + + It("evaluates as the owner, even when the caller cannot see the playlist", func() { + pls := model.Playlist{Name: "Evaluate Owner", OwnerID: regularUser.ID, Rules: dayRules()} + Expect(repo.Put(ctx, &pls)).To(Succeed()) + DeferCleanup(func() { _ = repo.Delete(ctx, pls.ID) }) + otherCtx := request.WithUser(log.NewContext(GinkgoT().Context()), thirdUser) + + Expect(repo.Evaluate(otherCtx, pls.ID)).To(Succeed()) + + got, err := repo.Get(ctx, pls.ID) + Expect(err).ToNot(HaveOccurred()) + Expect(got.EvaluatedAt).ToNot(BeNil()) + Expect(got.SongCount).To(Equal(1)) + }) + + It("returns ErrNotFound for an unknown playlist", func() { + Expect(repo.Evaluate(ctx, "nonexistent-id")).To(MatchError(model.ErrNotFound)) + }) + }) + Describe("Playlist Track Sorting", func() { var testPlaylistID string diff --git a/persistence/sql_base_repository.go b/persistence/sql_base_repository.go index be88156d8..c5ab7cde1 100644 --- a/persistence/sql_base_repository.go +++ b/persistence/sql_base_repository.go @@ -58,6 +58,18 @@ func loggedUser(ctx context.Context) *model.User { } } +// ownerContext scopes ctx to the given user, so queries apply that user's library access and annotations. +func (r sqlRepository) ownerContext(ctx context.Context, userID string) (context.Context, error) { + owner, err := NewUserRepository(r.db).Get(ctx, userID) + if err != nil { + return nil, fmt.Errorf("loading owner %q: %w", userID, err) + } + if owner == nil { + return nil, fmt.Errorf("owner %q not found", userID) + } + return request.WithUser(ctx, *owner), nil +} + // ownerFilter returns the predicate restricting access to rows owned by the logged-in user, for // tables with a user_id column. It returns nil for admins and for headless/system contexts (invalid // user), meaning "no ownership restriction". Callers should skip the WHERE clause when it is nil. diff --git a/scanner/phase_4_playlists.go b/scanner/phase_4_playlists.go index bb67c1ba3..40747f3e3 100644 --- a/scanner/phase_4_playlists.go +++ b/scanner/phase_4_playlists.go @@ -144,6 +144,10 @@ func (p *phasePlaylists) processPlaylistsInFolder(folder *model.Folder) (*model. continue } if pls.IsSmartPlaylist() { + // A nil EvaluatedAt means the playlist is new or its file changed + if pls.ID != "" && pls.EvaluatedAt == nil { + p.scanState.queueSmartPlaylist(pls.ID) + } log.Debug("Scanner: Imported smart playlist", "name", pls.Name, "lastUpdated", pls.UpdatedAt, "path", pls.Path, "elapsed", time.Since(started)) } else { log.Debug("Scanner: Imported playlist", "name", pls.Name, "lastUpdated", pls.UpdatedAt, "path", pls.Path, "numTracks", len(pls.Tracks), "elapsed", time.Since(started)) diff --git a/scanner/phase_4_playlists_test.go b/scanner/phase_4_playlists_test.go index 303af338f..d390f1017 100644 --- a/scanner/phase_4_playlists_test.go +++ b/scanner/phase_4_playlists_test.go @@ -6,12 +6,14 @@ import ( "os" "path/filepath" "sort" + "time" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/conf/configtest" "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/core/playlists" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/criteria" "github.com/navidrome/navidrome/tests" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -213,6 +215,27 @@ var _ = Describe("phasePlaylists", func() { ))) }) + It("queues only smart playlists that were never evaluated", func() { + libPath := GinkgoT().TempDir() + folder := &model.Folder{LibraryPath: libPath, Path: "path/to", Name: "folder"} + _ = os.MkdirAll(folder.AbsolutePath(), 0755) + for _, name := range []string{"new.nsp", "evaluated.nsp", "regular.m3u"} { + _ = os.WriteFile(filepath.Join(folder.AbsolutePath(), name), []byte{}, 0600) + } + + rules := &criteria.Criteria{Expression: criteria.All{criteria.Contains{"title": "Day"}}} + pls.On("ImportFromFolder", mock.Anything, folder, "new.nsp"). + Return(&model.Playlist{ID: "new", Rules: rules}, nil) + pls.On("ImportFromFolder", mock.Anything, folder, "evaluated.nsp"). + Return(&model.Playlist{ID: "evaluated", Rules: rules, EvaluatedAt: new(time.Now())}, nil) + pls.On("ImportFromFolder", mock.Anything, folder, "regular.m3u"). + Return(&model.Playlist{ID: "regular"}, nil) + + _, err := phase.processPlaylistsInFolder(folder) + Expect(err).ToNot(HaveOccurred()) + Expect(state.smartPlaylistsToEvaluate()).To(ConsistOf("new")) + }) + It("reports an error if there is an error reading files", func() { tests.SkipOnWindows("relies on Unix /etc filesystem") progress := make(chan *ProgressInfo) diff --git a/scanner/scanner.go b/scanner/scanner.go index cd2fe3c8d..02a66dff8 100644 --- a/scanner/scanner.go +++ b/scanner/scanner.go @@ -6,6 +6,7 @@ import ( "maps" "path/filepath" "slices" + "sync" "sync/atomic" "time" @@ -32,6 +33,21 @@ type scanState struct { libraries model.Libraries // Store libraries list for consistency across phases targets map[int][]string // Optional: map[libraryID][]folderPaths for selective scans totalLibraryCount int // Total number of libraries (unfiltered), for cross-library move detection + + smartPlaylistsMu sync.Mutex + smartPlaylists []string +} + +func (s *scanState) queueSmartPlaylist(id string) { + s.smartPlaylistsMu.Lock() + defer s.smartPlaylistsMu.Unlock() + s.smartPlaylists = append(s.smartPlaylists, id) +} + +func (s *scanState) smartPlaylistsToEvaluate() []string { + s.smartPlaylistsMu.Lock() + defer s.smartPlaylistsMu.Unlock() + return s.smartPlaylists } func (s *scanState) sendProgress(info *ProgressInfo) { @@ -187,6 +203,9 @@ func (s *scannerImpl) scanFolders(ctx context.Context, fullScan bool, targets [] // Update last_scan_completed_at for all libraries s.runUpdateLibraries(ctx, &state), + + // Evaluate new/changed smart playlists last, so their rules see the final library state + s.runEvaluateSmartPlaylists(ctx, &state), ) if err != nil { log.Error(ctx, "Scanner: Finished with error", "duration", time.Since(startTime), err) @@ -332,6 +351,24 @@ func (s *scannerImpl) runRefreshStats(ctx context.Context, state *scanState) fun } } +// Failures are logged but never fail the scan: the playlist is still evaluated when it is next read. +func (s *scannerImpl) runEvaluateSmartPlaylists(ctx context.Context, state *scanState) func() error { + return func() error { + for _, id := range state.smartPlaylistsToEvaluate() { + start := time.Now() + err := s.ds.WithTxRetry(ctx, func(ctx context.Context, tx model.DataStore) error { + return tx.Playlist().Evaluate(ctx, id) + }, "scanner: evaluate smart playlist") + if err != nil { + log.Warn(ctx, "Scanner: Could not evaluate smart playlist", "id", id, err) + continue + } + log.Debug(ctx, "Scanner: Evaluated smart playlist", "id", id, "elapsed", time.Since(start)) + } + return nil + } +} + func (s *scannerImpl) runUpdateLibraries(ctx context.Context, state *scanState) func() error { return func() error { start := time.Now() diff --git a/scanner/scanner_internal_test.go b/scanner/scanner_internal_test.go index 0778bd6ec..bd5fb0fb9 100644 --- a/scanner/scanner_internal_test.go +++ b/scanner/scanner_internal_test.go @@ -9,6 +9,7 @@ import ( "sync/atomic" ppl "github.com/google/go-pipeline/pkg/pipeline" + "github.com/navidrome/navidrome/tests" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) @@ -135,3 +136,32 @@ var _ = Describe("runPhase", func() { Expect(counter.Load()).To(Equal(int64(3))) }) }) + +var _ = Describe("runEvaluateSmartPlaylists", func() { + var ctx context.Context + var plsRepo *tests.MockPlaylistRepo + var s *scannerImpl + var state *scanState + + BeforeEach(func() { + ctx = GinkgoT().Context() + plsRepo = tests.CreateMockPlaylistRepo() + s = &scannerImpl{ds: &tests.MockDataStore{MockedPlaylist: plsRepo}} + state = &scanState{} + }) + + It("evaluates every queued smart playlist", func() { + state.queueSmartPlaylist("p1") + state.queueSmartPlaylist("p2") + + Expect(s.runEvaluateSmartPlaylists(ctx, state)()).To(Succeed()) + Expect(plsRepo.Evaluated).To(Equal([]string{"p1", "p2"})) + }) + + It("does not fail the scan when an evaluation fails", func() { + plsRepo.SetError(true) + state.queueSmartPlaylist("p1") + + Expect(s.runEvaluateSmartPlaylists(ctx, state)()).To(Succeed()) + }) +}) diff --git a/tests/mock_playlist_repo.go b/tests/mock_playlist_repo.go index 824e701f6..97cf7f351 100644 --- a/tests/mock_playlist_repo.go +++ b/tests/mock_playlist_repo.go @@ -30,6 +30,7 @@ type MockPlaylistRepo struct { Err bool TracksRepo model.PlaylistTrackRepository TracksRefreshed bool + Evaluated []string } func (m *MockPlaylistRepo) SetError(err bool) { @@ -186,4 +187,12 @@ func (m *MockPlaylistRepo) CountAll(_ context.Context, _ ...model.QueryOptions) return int64(len(m.Data)), nil } +func (m *MockPlaylistRepo) Evaluate(_ context.Context, id string) error { + if m.Err { + return errors.New("error") + } + m.Evaluated = append(m.Evaluated, id) + return nil +} + var _ model.PlaylistRepository = (*MockPlaylistRepo)(nil)