diff --git a/core/storage/local/local.go b/core/storage/local/local.go index 686838565..e2ce00a1b 100644 --- a/core/storage/local/local.go +++ b/core/storage/local/local.go @@ -76,6 +76,17 @@ 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 d9d417312..b9a5c3374 100644 --- a/persistence/mediafile_repository.go +++ b/persistence/mediafile_repository.go @@ -96,8 +96,7 @@ 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, media_file.id", - "updated_at": "media_file.updated_at, media_file.id", + "created_at": "media_file.created_at", "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 f0ac36536..03cc6a01b 100644 --- a/persistence/sql_base_repository.go +++ b/persistence/sql_base_repository.go @@ -409,7 +409,9 @@ 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) { - sq = r.paginate(sq, options...) + if len(options) > 0 && options[0].Offset > 0 { + sq = r.optimizePagination(sq, options[0]) + } query, args, err := r.toSQL(sq) if err != nil { return nil, err @@ -437,7 +439,9 @@ 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 { - sq = r.paginate(sq, options...) + if len(options) > 0 && options[0].Offset > 0 { + sq = r.optimizePagination(sq, options[0]) + } query, args, err := r.toSQL(sq) if err != nil { return err @@ -468,28 +472,15 @@ 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() - // 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)) + rowidSq := sq.RemoveColumns().Columns(r.tableName + ".rowid") + rowidSq = rowidSq.Limit(uint64(options.Offset)) rowidSql, args, _ := rowidSq.ToSql() - sq = sq.Where(r.tableName+".rowid not in (SELECT _paginated_rowid FROM ("+rowidSql+"))", args...) + sq = sq.Where(r.tableName+".rowid not in ("+rowidSql+")", args...) } return sq } diff --git a/persistence/sql_base_repository_test.go b/persistence/sql_base_repository_test.go index ef5b2462a..4b42e7f1d 100644 --- a/persistence/sql_base_repository_test.go +++ b/persistence/sql_base_repository_test.go @@ -2,12 +2,8 @@ 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" @@ -425,133 +421,3 @@ 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 1864a6a44..d39cad5f6 100644 --- a/scanner/walk_dir_tree.go +++ b/scanner/walk_dir_tree.go @@ -43,6 +43,13 @@ 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) @@ -225,6 +232,18 @@ 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 43939e5c2..0c2aba3e0 100644 --- a/scanner/walk_dir_tree_test.go +++ b/scanner/walk_dir_tree_test.go @@ -4,8 +4,10 @@ import ( "context" "fmt" "io/fs" + "maps" "os" "path/filepath" + "slices" "testing/fstest" "github.com/navidrome/navidrome/conf" @@ -260,6 +262,44 @@ 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")) + }) + }) }) }) @@ -433,8 +473,8 @@ var _ = Describe("walk_dir_tree", func() { }) // Regression for #5752: the production localFS must resolve file symlinks. - // It wraps os.DirFS behind the fs.FS interface, so fs.ReadLink-based - // resolution is not available and full OS-level resolution is required. + // fs.ReadLink-based resolution can't follow targets outside the library + // root, so full OS-level resolution is required. Context("production local storage FS", func() { var libRoot string var musicFS storage.MusicFS @@ -460,12 +500,7 @@ 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()) - 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()) + musicFS = newLocalMusicFS(libRoot) }) walkRoot := func() *folderEntry { @@ -700,6 +735,17 @@ 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