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/playlist_repository.go b/persistence/playlist_repository.go index 41b75266c..47e43236e 100644 --- a/persistence/playlist_repository.go +++ b/persistence/playlist_repository.go @@ -260,6 +260,17 @@ func (r *playlistRepository) selectPlaylist(ctx context.Context, options ...mode return r.withAnnotation(ctx, sel, r.tableName+".id") } +// inTx runs fn in a transaction, joining the caller's if one is already open. +func (r *playlistRepository) inTx(fn func(tx *playlistRepository) error) error { + conn, ok := r.db.(*dbx.DB) + if !ok { + return fn(r) + } + return conn.Transactional(func(tx *dbx.Tx) error { + return fn(NewPlaylistRepository(tx).(*playlistRepository)) + }) +} + func (r *playlistRepository) updateTracks(ctx context.Context, id string, tracks model.MediaFiles) error { ids := make([]string, len(tracks)) for i := range tracks { 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..fc78dcc50 100644 --- a/persistence/smart_playlist_repository.go +++ b/persistence/smart_playlist_repository.go @@ -2,6 +2,9 @@ package persistence import ( "context" + "encoding/json" + "errors" + "fmt" "slices" "time" @@ -18,6 +21,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{}{}) @@ -39,12 +59,6 @@ func (r *playlistRepository) refreshSmartPlaylistTree(ctx context.Context, pls * log.Debug(ctx, "Refreshing smart playlist", "playlist", pls.Name, "id", pls.ID) start := time.Now() - del := Delete("playlist_tracks").Where(Eq{"playlist_id": pls.ID}) - if _, err := r.executeSQL(ctx, del); err != nil { - log.Error(ctx, "Error deleting old smart playlist tracks", "playlist", pls.Name, "id", pls.ID, err) - return false - } - rulesSQL := newSmartPlaylistCriteria(*pls.NormalizedRules(), withSmartPlaylistOwner(*usr)) if !r.refreshChildPlaylists(ctx, pls, rulesSQL, visited) { @@ -55,37 +69,56 @@ func (r *playlistRepository) refreshSmartPlaylistTree(ctx context.Context, pls * return false } - sq := r.buildSmartPlaylistQuery(ctx, pls, rulesSQL, usr.ID) - sq, err := r.addCriteria(sq, rulesSQL) + sq, err := r.addCriteria(r.buildSmartPlaylistQuery(ctx, rulesSQL, usr.ID), rulesSQL) if err != nil { log.Error(ctx, "Error building smart playlist criteria", "playlist", pls.Name, "id", pls.ID, err) return false } - insSql := Insert("playlist_tracks").Columns("id", "playlist_id", "media_file_id").Select(sq) - if _, err = r.executeSQL(ctx, insSql); err != nil { + // Evaluate the criteria before writing, so the write lock is only held for the short replace below + var ids []string + if err = r.queryAllSlice(ctx, sq, &ids); err != nil && !errors.Is(err, model.ErrNotFound) { + log.Error(ctx, "Error evaluating smart playlist criteria", "playlist", pls.Name, "id", pls.ID, err) + return false + } + + err = r.inTx(func(tx *playlistRepository) error { return tx.replaceSmartPlaylistTracks(ctx, pls, ids) }) + if err != nil { log.Error(ctx, "Error refreshing smart playlist tracks", "playlist", pls.Name, "id", pls.ID, err) return false } - if err = r.refreshCounters(ctx, pls); err != nil { - log.Error(ctx, "Error updating smart playlist stats", "playlist", pls.Name, "id", pls.ID, err) - return false - } - - // Reuse the stamp refreshCounters just wrote, so evaluated_at and updated_at agree - now := pls.UpdatedAt - updSql := Update(r.tableName).Set("evaluated_at", now).Where(Eq{"id": pls.ID}) - if _, err = r.executeSQL(ctx, updSql); err != nil { - log.Error(ctx, "Error updating smart playlist", "playlist", pls.Name, "id", pls.ID, err) - return false - } - pls.EvaluatedAt = &now - log.Debug(ctx, "Refreshed playlist", "playlist", pls.Name, "id", pls.ID, "numTracks", pls.SongCount, "elapsed", time.Since(start)) return true } +func (r *playlistRepository) replaceSmartPlaylistTracks(ctx context.Context, pls *model.Playlist, ids []string) error { + if _, err := r.executeSQL(ctx, Delete("playlist_tracks").Where(Eq{"playlist_id": pls.ID})); err != nil { + return err + } + if len(ids) > 0 { + idsJSON, err := json.Marshal(ids) + if err != nil { + return err + } + ins := Expr("INSERT INTO playlist_tracks (id, playlist_id, media_file_id) SELECT key + 1, ?, value FROM json_each(?)", + pls.ID, string(idsJSON)) + if _, err = r.executeSQL(ctx, ins); err != nil { + return err + } + } + if err := r.refreshCounters(ctx, pls); err != nil { + return err + } + // Reuse the stamp refreshCounters just wrote, so evaluated_at and updated_at agree + now := pls.UpdatedAt + if _, err := r.executeSQL(ctx, Update(r.tableName).Set("evaluated_at", now).Where(Eq{"id": pls.ID})); err != nil { + return err + } + pls.EvaluatedAt = &now + return nil +} + // shouldRefreshSmartPlaylist determines if a smart playlist needs to be refreshed based on its type, last evaluated // time, and ownership. func (r *playlistRepository) shouldRefreshSmartPlaylist(ctx context.Context, pls *model.Playlist, usr *model.User) bool { @@ -176,12 +209,10 @@ func (r *playlistRepository) resolvePercentageLimit(ctx context.Context, pls *mo return nil } -// buildSmartPlaylistQuery constructs the SQL query to select media files matching the smart playlist criteria, -// including the joins its fields require and library filtering. -func (r *playlistRepository) buildSmartPlaylistQuery(ctx context.Context, pls *model.Playlist, rulesSQL smartPlaylistCriteria, userID string) SelectBuilder { - orderBy := rulesSQL.orderBy() - sq := Select("row_number() over (order by "+orderBy+") as id", "'"+pls.ID+"' as playlist_id", "media_file.id as media_file_id"). - From("media_file") +// buildSmartPlaylistQuery constructs the SQL query to select the ids of media files matching the smart playlist +// criteria, including the joins its fields require and library filtering. +func (r *playlistRepository) buildSmartPlaylistQuery(ctx context.Context, rulesSQL smartPlaylistCriteria, userID string) SelectBuilder { + sq := Select("media_file.id").From("media_file") sq = rulesSQL.applyRequiredJoins(sq, userID) sq = r.applyLibraryFilter(ctx, sq, "media_file") return sq 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..fc1556b65 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,21 @@ 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() + if err := s.ds.Playlist().Evaluate(ctx, id); 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)