diff --git a/core/storage/local/local.go b/core/storage/local/local.go index e2ce00a1b..686838565 100644 --- a/core/storage/local/local.go +++ b/core/storage/local/local.go @@ -76,17 +76,6 @@ func (lfs *localFS) ResolveSymlink(name string) (string, error) { return filepath.EvalSymlinks(filepath.Join(lfs.root, filepath.FromSlash(name))) } -// ReadLink and Lstat implement fs.ReadLinkFS, so callers can detect symlinks without following them. -var _ fs.ReadLinkFS = (*localFS)(nil) - -func (lfs *localFS) ReadLink(name string) (string, error) { - return fs.ReadLink(lfs.FS, name) -} - -func (lfs *localFS) Lstat(name string) (fs.FileInfo, error) { - return fs.Lstat(lfs.FS, name) -} - func (lfs *localFS) ReadTags(path ...string) (map[string]metadata.Info, error) { res, err := lfs.extractor.Parse(path...) if err != nil { diff --git a/persistence/mediafile_repository.go b/persistence/mediafile_repository.go index b9a5c3374..d9d417312 100644 --- a/persistence/mediafile_repository.go +++ b/persistence/mediafile_repository.go @@ -96,7 +96,8 @@ func NewMediaFileRepository(db dbx.Builder) model.MediaFileRepository { "album_artist": "order_album_artist_name, order_album_name, release_date, disc_number, track_number", "album": "order_album_name, album_id, disc_number, track_number, order_artist_name, " + naturalSort("media_file.title"), "random": "random", - "created_at": "media_file.created_at", + "created_at": "media_file.created_at, media_file.id", + "updated_at": "media_file.updated_at, media_file.id", "recently_added": mediaFileRecentlyAddedSort(), "starred_at": "starred, starred_at", "rated_at": "rating, rated_at", diff --git a/persistence/sql_base_repository.go b/persistence/sql_base_repository.go index 03cc6a01b..f0ac36536 100644 --- a/persistence/sql_base_repository.go +++ b/persistence/sql_base_repository.go @@ -409,9 +409,7 @@ func wrapCursor[D, T any](cursor iter.Seq2[D, error], toModel func(D) *T) iter.S // queryWithStableResults is a helper function to execute a query and return an iterator that will yield its results // from a cursor, guaranteeing that the results will be stable, even if the underlying data changes. func queryWithStableResults[T any](ctx context.Context, r sqlRepository, sq SelectBuilder, options ...model.QueryOptions) (iter.Seq2[T, error], error) { - if len(options) > 0 && options[0].Offset > 0 { - sq = r.optimizePagination(sq, options[0]) - } + sq = r.paginate(sq, options...) query, args, err := r.toSQL(sq) if err != nil { return nil, err @@ -439,9 +437,7 @@ func queryWithStableResults[T any](ctx context.Context, r sqlRepository, sq Sele } func (r sqlRepository) queryAll(ctx context.Context, sq SelectBuilder, response any, options ...model.QueryOptions) error { - if len(options) > 0 && options[0].Offset > 0 { - sq = r.optimizePagination(sq, options[0]) - } + sq = r.paginate(sq, options...) query, args, err := r.toSQL(sq) if err != nil { return err @@ -472,15 +468,28 @@ func (r sqlRepository) queryAllSlice(ctx context.Context, sq SelectBuilder, resp return err } +// paginate adds a rowid tie-breaker, so the order is total and rows with equal sort values land on the +// same page every time. Indexes already end with the rowid, so index-served sorts stay index-served. +func (r sqlRepository) paginate(sq SelectBuilder, options ...model.QueryOptions) SelectBuilder { + if len(options) == 0 || (options[0].Max == 0 && options[0].Offset == 0) { + return sq + } + order := "asc" + if strings.EqualFold(strings.TrimSpace(options[0].Order), "desc") { + order = "desc" + } + return r.optimizePagination(sq.OrderBy(r.tableName+".rowid "+order), options[0]) +} + // optimizePagination uses a less inefficient pagination, by not using OFFSET. // See https://gist.github.com/ssokolow/262503 func (r sqlRepository) optimizePagination(sq SelectBuilder, options model.QueryOptions) SelectBuilder { if options.Offset > conf.Server.DevOffsetOptimize { sq = sq.RemoveOffset() - rowidSq := sq.RemoveColumns().Columns(r.tableName + ".rowid") - rowidSq = rowidSq.Limit(uint64(options.Offset)) + // Keep the outer projection, so ORDER BY names resolve to the same columns in both queries. + rowidSq := sq.Column(r.tableName + ".rowid as _paginated_rowid").Limit(uint64(options.Offset)) rowidSql, args, _ := rowidSq.ToSql() - sq = sq.Where(r.tableName+".rowid not in ("+rowidSql+")", args...) + sq = sq.Where(r.tableName+".rowid not in (SELECT _paginated_rowid FROM ("+rowidSql+"))", args...) } return sq } diff --git a/persistence/sql_base_repository_test.go b/persistence/sql_base_repository_test.go index 4b42e7f1d..ef5b2462a 100644 --- a/persistence/sql_base_repository_test.go +++ b/persistence/sql_base_repository_test.go @@ -2,8 +2,12 @@ package persistence import ( "context" + "slices" "github.com/Masterminds/squirrel" + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/conf/configtest" + "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/utils/hasher" @@ -421,3 +425,133 @@ var _ = Describe("sqlRepository", func() { }) }) }) + +var _ = Describe("paginated queries", func() { + var ctx context.Context + var mr model.MediaFileRepository + + BeforeEach(func() { + ctx = request.WithUser(log.NewContext(GinkgoT().Context()), adminUser) + DeferCleanup(configtest.SetupConfig()) + mr = NewMediaFileRepository(GetDBXBuilder()) + }) + + Describe("SQL", func() { + var r sqlRepository + BeforeEach(func() { + r = sqlRepository{tableName: "media_file", sortMappings: map[string]string{"name": "title"}} + conf.Server.DevOffsetOptimize = 10 + }) + + DescribeTable("adds a rowid tie-breaker in the requested direction", + func(options model.QueryOptions, expectedOrder string) { + sql, _, err := r.paginate(squirrel.Select("*").From("media_file"), options).ToSql() + Expect(err).ToNot(HaveOccurred()) + Expect(sql).To(HaveSuffix(expectedOrder)) + }, + Entry("no sort", model.QueryOptions{Max: 5}, "ORDER BY media_file.rowid asc"), + Entry("desc", model.QueryOptions{Sort: "name", Order: "desc", Max: 5}, "ORDER BY media_file.rowid desc"), + ) + + It("leaves unpaginated queries alone", func() { + sql, _, _ := r.paginate(squirrel.Select("*").From("media_file"), model.QueryOptions{Sort: "name"}).ToSql() + Expect(sql).To(Equal("SELECT * FROM media_file")) + }) + + It("uses the rowid subquery only past the threshold, keeping the outer projection", func() { + sq := squirrel.Select("media_file.*", "library.path as library_path").From("media_file").OrderBy("id asc") + atThreshold, _, _ := r.paginate(sq.Limit(5).Offset(10), model.QueryOptions{Max: 5, Offset: 10}).ToSql() + Expect(atThreshold).ToNot(ContainSubstring("not in")) + + past, _, _ := r.paginate(sq.Limit(5).Offset(11), model.QueryOptions{Max: 5, Offset: 11}).ToSql() + Expect(past).To(ContainSubstring("media_file.rowid not in (SELECT _paginated_rowid FROM (" + + "SELECT media_file.*, library.path as library_path, media_file.rowid as _paginated_rowid FROM media_file")) + Expect(past).ToNot(ContainSubstring("OFFSET")) + }) + }) + + // walk pages through the repository, crossing the optimizer threshold + walk := func(ctx context.Context, sort, order string, pageSize int) []string { + var ids []string + for offset := 0; ; offset += pageSize { + page, err := mr.GetAll(ctx, model.QueryOptions{Sort: sort, Order: order, Offset: offset, Max: pageSize, Seed: "seed"}) + Expect(err).ToNot(HaveOccurred(), "sort=%s order=%s offset=%d", sort, order, offset) + for _, m := range page { + ids = append(ids, m.ID) + } + if len(page) < pageSize { + return ids + } + } + } + + DescribeTable("returns every song exactly once, in the same order as a single page", + func(sort string) { + conf.Server.DevOffsetOptimize = 2 + for _, order := range []string{"asc", "desc"} { + expected := walk(ctx, sort, order, 1000) + Expect(len(expected)).To(BeNumerically(">=", len(testSongs))) + Expect(slices.Compact(slices.Sorted(slices.Values(expected)))).To(HaveLen(len(expected))) + // Page size 2 hits offset == threshold (plain OFFSET), size 3 hits offset > threshold first. + for _, pageSize := range []int{2, 3} { + Expect(walk(ctx, sort, order, pageSize)).To(Equal(expected), "sort=%s order=%s page=%d", sort, order, pageSize) + } + } + }, + Entry("id", "id"), + Entry("path", "path"), + Entry("updatedAt", "updatedAt"), + Entry("comment", "comment"), + Entry("title", "title"), + Entry("createdAt", "createdAt"), + Entry("starred (ties)", "starred"), + Entry("playCount (ties)", "playCount"), + Entry("rating (ties)", "rating"), + Entry("album", "album"), + Entry("seeded random", "random"), + ) + + It("keeps the library filter on every page", func() { + _, otherLib, restrictedUser := restrictedFixture("paginate") + hidden := model.MediaFile{ID: "paginate-hidden", Title: "Hidden", LibraryID: otherLib.ID, Path: p("other/hidden.mp3")} + Expect(mr.Put(ctx, &hidden)).To(Succeed()) + DeferCleanup(func() { _ = mr.Delete(ctx, hidden.ID) }) + conf.Server.DevOffsetOptimize = 2 + + Expect(walk(ctx, "id", "asc", 1000)).To(ContainElement(hidden.ID)) + restrictedCtx := request.WithUser(ctx, restrictedUser) + expected := walk(restrictedCtx, "id", "asc", 1000) + Expect(expected).ToNot(BeEmpty()) + Expect(expected).ToNot(ContainElement(hidden.ID)) + Expect(walk(restrictedCtx, "id", "asc", 3)).To(Equal(expected)) + }) + + Describe("legacy search", func() { + BeforeEach(func() { + conf.Server.Search.Backend = "legacy" + conf.Server.DevOffsetOptimize = 0 + }) + + It("pages albums whose name is shared with the joined library", func() { + alr := NewAlbumRepository(GetDBXBuilder()) + all, err := alr.Search(ctx, "abbey", model.QueryOptions{Max: 10}) + Expect(err).ToNot(HaveOccurred()) + Expect(all).To(HaveLen(2)) + second, err := alr.Search(ctx, "abbey", model.QueryOptions{Max: 1, Offset: 1}) + Expect(err).ToNot(HaveOccurred()) + Expect(second).To(HaveLen(1)) + Expect(second[0].ID).To(Equal(all[1].ID)) + }) + + It("pages artists", func() { + arr := NewArtistRepository(GetDBXBuilder()) + all, err := arr.Search(ctx, "the", model.QueryOptions{Max: 10}) + Expect(err).ToNot(HaveOccurred()) + Expect(len(all)).To(BeNumerically(">=", 2)) + second, err := arr.Search(ctx, "the", model.QueryOptions{Max: 1, Offset: 1}) + Expect(err).ToNot(HaveOccurred()) + Expect(second).To(HaveLen(1)) + Expect(second[0].ID).To(Equal(all[1].ID)) + }) + }) +}) diff --git a/scanner/walk_dir_tree.go b/scanner/walk_dir_tree.go index d39cad5f6..1864a6a44 100644 --- a/scanner/walk_dir_tree.go +++ b/scanner/walk_dir_tree.go @@ -43,13 +43,6 @@ func walkDirTree(ctx context.Context, job *scanJob, targetFolders ...string) (<- continue } - // A full walk never descends into symlinked folders when following is disabled, so a - // target reached through one (e.g. a watcher event for a new link) is skipped too. - if !conf.Server.Scanner.FollowSymlinks && isSymlinkedPath(job.fs, folderPath) { - log.Debug(ctx, "Scanner: Skipping symlinked target folder, following is disabled", "path", folderPath) - continue - } - // Create checker and push patterns from root to this folder checker := newIgnoreChecker(job.fs) err = checker.PushAllParents(ctx, folderPath) @@ -232,18 +225,6 @@ func isDirOrSymlinkToDir(fsys fs.FS, baseDir string, dirEnt fs.DirEntry) (bool, return fileInfo.IsDir(), nil } -// isSymlinkedPath returns true if folderPath, or any of its parent folders, is a symbolic link. -// It needs fsys to implement fs.ReadLinkFS, otherwise links are followed and never detected. -func isSymlinkedPath(fsys fs.FS, folderPath string) bool { - for p := path.Clean(folderPath); p != "." && p != "/"; p = path.Dir(p) { - info, err := fs.Lstat(fsys, p) - if err == nil && info.Mode()&fs.ModeSymlink != 0 { - return true - } - } - return false -} - const maxSymlinkHops = 40 // resolveEntryName returns the name to classify the entry by, and whether to diff --git a/scanner/walk_dir_tree_test.go b/scanner/walk_dir_tree_test.go index 0c2aba3e0..43939e5c2 100644 --- a/scanner/walk_dir_tree_test.go +++ b/scanner/walk_dir_tree_test.go @@ -4,10 +4,8 @@ import ( "context" "fmt" "io/fs" - "maps" "os" "path/filepath" - "slices" "testing/fstest" "github.com/navidrome/navidrome/conf" @@ -262,44 +260,6 @@ var _ = Describe("walk_dir_tree", func() { // Folders not in targets should remain in lastUpdates Expect(job.lastUpdates).To(HaveKey(model.FolderID(job.lib, "OtherArtist/Album3"))) }) - - // #6292: a watcher event for a new folder symlink makes the link itself a scan target - Context("symlinked target folders (production local storage FS)", func() { - BeforeEach(func() { - libRoot := GinkgoT().TempDir() - Expect(os.MkdirAll(filepath.Join(libRoot, "Mozart", "Album1"), 0755)).To(Succeed()) - Expect(os.WriteFile(filepath.Join(libRoot, "Mozart", "Album1", "track.mp3"), []byte("AUDIO"), 0600)).To(Succeed()) - Expect(os.Symlink("Mozart", filepath.Join(libRoot, "Wolfgang Amadeus Mozart"))).To(Succeed()) - job = &scanJob{fs: newLocalMusicFS(libRoot), lib: model.Library{Path: libRoot}} - }) - - walkTargets := func(targets ...string) map[string]*folderEntry { - results, err := walkDirTree(ctx, job, targets...) - Expect(err).ToNot(HaveOccurred()) - folders := map[string]*folderEntry{} - for folder := range results { - folders[folder.path] = folder - } - return folders - } - - DescribeTable("with FollowSymlinks disabled", - func(target string, expected ...string) { - conf.Server.Scanner.FollowSymlinks = false - Expect(slices.Collect(maps.Keys(walkTargets(target)))).To(ConsistOf(expected)) - }, - Entry("skips a target that is a symlink", "Wolfgang Amadeus Mozart"), - Entry("skips a target under a symlinked folder", "Wolfgang Amadeus Mozart/Album1"), - Entry("walks a regular target", "Mozart", "Mozart", "Mozart/Album1"), - ) - - It("walks a symlinked target when FollowSymlinks is enabled", func() { - conf.Server.Scanner.FollowSymlinks = true - folders := walkTargets("Wolfgang Amadeus Mozart") - Expect(folders).To(HaveKey("Wolfgang Amadeus Mozart/Album1")) - Expect(folders["Wolfgang Amadeus Mozart/Album1"].audioFiles).To(HaveKey("track.mp3")) - }) - }) }) }) @@ -473,8 +433,8 @@ var _ = Describe("walk_dir_tree", func() { }) // Regression for #5752: the production localFS must resolve file symlinks. - // fs.ReadLink-based resolution can't follow targets outside the library - // root, so full OS-level resolution is required. + // It wraps os.DirFS behind the fs.FS interface, so fs.ReadLink-based + // resolution is not available and full OS-level resolution is required. Context("production local storage FS", func() { var libRoot string var musicFS storage.MusicFS @@ -500,7 +460,12 @@ var _ = Describe("walk_dir_tree", func() { Expect(os.Symlink(filepath.Join(pool, "mid.wav"), filepath.Join(libRoot, "evil.wav"))).To(Succeed()) Expect(os.Symlink(filepath.Join(pool, "missing.mp3"), filepath.Join(libRoot, "broken.mp3"))).To(Succeed()) - musicFS = newLocalMusicFS(libRoot) + u, err := storage.LocalPathToURL(libRoot) + Expect(err).ToNot(HaveOccurred()) + s, err := storage.For(u.String()) + Expect(err).ToNot(HaveOccurred()) + musicFS, err = s.FS() + Expect(err).ToNot(HaveOccurred()) }) walkRoot := func() *folderEntry { @@ -735,17 +700,6 @@ func getDirEntry(baseDir, name string) os.DirEntry { panic(fmt.Sprintf("Could not find %s in %s", name, baseDir)) } -// newLocalMusicFS returns the production local storage MusicFS rooted at libRoot -func newLocalMusicFS(libRoot string) storage.MusicFS { - u, err := storage.LocalPathToURL(libRoot) - Expect(err).ToNot(HaveOccurred()) - s, err := storage.For(u.String()) - Expect(err).ToNot(HaveOccurred()) - musicFS, err := s.FS() - Expect(err).ToNot(HaveOccurred()) - return musicFS -} - // mockMusicFS is a mock implementation of the MusicFS interface that supports symlinks type mockMusicFS struct { storage.MusicFS