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)) + }) + }) +})