Merge 1c83f36ddf002ddb718ba3ec65c7acd163b28b64 into 3867fab4da6ea7142bfc6374c94b34a708e03b4e

db/migrations/20260810171732_add_album_library_id_index.sql

@@ -0,0 +1,16 @@
+-- +goose Up
+-- id leads so library_id cannot drive a seek: the index covers library-filtered counts without
+-- tempting the planner to sort a whole library instead of walking a sort-satisfying index.
+create index if not exists album_id_library_id
+	on album(id, library_id);
+
+drop index if exists album_order_album_name;
+create index album_order_album_name
+	on album(order_album_name, order_album_artist_name, id);
+
+-- +goose Down
+drop index if exists album_id_library_id;
+
+drop index if exists album_order_album_name;
+create index album_order_album_name
+	on album(order_album_name);

persistence/album_repository.go

@@ -1,6 +1,7 @@
 package persistence
 
 import (
+	"cmp"
 	"context"
 	"encoding/json"
 	"errors"
@@ -109,7 +110,7 @@ func NewAlbumRepository(ctx context.Context, db dbx.Builder) model.AlbumReposito
 	r.tableName = "album"
 	r.registerModel(&model.Album{}, albumFilters())
 	r.setSortMappings(map[string]string{
-		"name":         "order_album_name, order_album_artist_name",
+		"name":         "order_album_name, order_album_artist_name, album.id",
 		"artist":       "compilation, order_album_artist_name, order_album_name",
 		"album_artist": "compilation, order_album_artist_name, order_album_name",
 		// TODO Rename this to just year (or date)
@@ -242,7 +243,14 @@ func (r *albumRepository) Get(id string) (*model.Album, error) {
 	return &res[0], nil
 }
 
+// Above this page size the id round-trip stops paying for itself and the IN list gets unwieldy.
+const maxRandomPageIDs = 1000
+
 func (r *albumRepository) GetAll(options ...model.QueryOptions) (model.Albums, error) {
+	if len(options) > 0 && options[0].Sort == "random" &&
+		options[0].Max > 0 && options[0].Max <= maxRandomPageIDs {
+		return r.getRandomPage(options[0])
+	}
 	sq := r.selectAlbum(options...)
 	var res dbAlbums
 	err := r.queryAll(sq, &res)
@@ -254,6 +262,28 @@ func (r *albumRepository) GetAll(options ...model.QueryOptions) (model.Albums, e
 	return albums, nil
 }
 
+// No index can serve a random sort, so the sort pass visits every matching row. Resolving ids
+// first keeps that pass on a covering index instead of dragging full rows and joins through it.
+func (r *albumRepository) getRandomPage(options model.QueryOptions) (model.Albums, error) {
+	ids, err := r.GetAllIDs(options)
+	if err != nil {
+		return nil, err
+	}
+	if len(ids) == 0 {
+		return model.Albums{}, nil
+	}
+	albums, err := r.GetAll(model.QueryOptions{Filters: Eq{"album.id": ids}})
+	if err != nil {
+		return nil, err
+	}
+	pos := make(map[string]int, len(ids))
+	for i, albumID := range ids {
+		pos[albumID] = i
+	}
+	slices.SortFunc(albums, func(a, b model.Album) int { return cmp.Compare(pos[a.ID], pos[b.ID]) })
+	return albums, nil
+}
+
 func (r *albumRepository) hydrateArtwork(albums model.Albums) {
 	hydrateItems(r.ctx, r.db, model.KindAlbumArtwork, albums,
 		func(a *model.Album) (string, *model.ItemImage) { return a.ID, &a.ItemImage })

persistence/album_repository_test.go

@@ -247,6 +247,84 @@ var _ = Describe("AlbumRepository", func() {
 			Expect(GetAll()).To(Equal(testAlbums))
 		})
 
+		// The REST layer sends library_id as a string. SQLite only coerces it to the column's
+		// integer affinity while the term stays a plain column reference, so a filter built on an
+		// expression instead would silently match nothing (#5929).
+		DescribeTable("library_id filter from the REST layer",
+			func(sort string) {
+				res, err := albumRepo.ReadAll(rest.QueryOptions{
+					Sort: sort, Max: 10, Filters: map[string]any{"library_id": "1"},
+				})
+				Expect(err).ToNot(HaveOccurred())
+				Expect(res.(model.Albums)).ToNot(BeEmpty())
+			},
+			Entry("name sort", "name"),
+			Entry("random sort", "random"),
+			Entry("no sort", ""),
+		)
+
+		Context("random sort", func() {
+			It("returns the page in the same order the id query produced", func() {
+				opts := model.QueryOptions{Sort: "random", Max: 4, Seed: "a-seed"}
+				ids, err := albumRepo.GetAllIDs(opts)
+				Expect(err).ToNot(HaveOccurred())
+				Expect(ids).To(HaveLen(4))
+
+				albums, err := GetAll(opts)
+				Expect(err).ToNot(HaveOccurred())
+				Expect(slice.Map(albums, func(a model.Album) string { return a.ID })).To(Equal(ids))
+			})
+
+			It("keeps paging on one shuffle for a given seed", func() {
+				ids := func(albums model.Albums) []string {
+					return slice.Map(albums, func(a model.Album) string { return a.ID })
+				}
+				firstTwoPages, err := GetAll(model.QueryOptions{Sort: "random", Max: 3, Seed: "s"})
+				Expect(err).ToNot(HaveOccurred())
+				secondPage, err := GetAll(model.QueryOptions{Sort: "random", Max: 3, Offset: 3, Seed: "s"})
+				Expect(err).ToNot(HaveOccurred())
+				whole, err := GetAll(model.QueryOptions{Sort: "random", Max: 6, Seed: "s"})
+				Expect(err).ToNot(HaveOccurred())
+				Expect(append(ids(firstTwoPages), ids(secondPage)...)).To(Equal(ids(whole)))
+			})
+
+			// Without an explicit seed, only Offset == 0 reseeds, so scrolling never repeats an album.
+			It("does not repeat albums across pages of one scroll", func() {
+				page1, err := GetAll(model.QueryOptions{Sort: "random", Max: 3})
+				Expect(err).ToNot(HaveOccurred())
+				page2, err := GetAll(model.QueryOptions{Sort: "random", Max: 3, Offset: 3})
+				Expect(err).ToNot(HaveOccurred())
+				Expect(page1).To(HaveLen(3))
+				Expect(page2).To(HaveLen(3))
+				for _, a := range page2 {
+					Expect(page1).ToNot(ContainElement(a))
+				}
+			})
+
+			It("returns every record when no limit is given", func() {
+				albums, err := GetAll(model.QueryOptions{Sort: "random"})
+				Expect(err).ToNot(HaveOccurred())
+				Expect(albums).To(ConsistOf(testAlbums))
+			})
+
+			It("applies filters", func() {
+				albums, err := GetAll(model.QueryOptions{
+					Sort: "random", Max: 10, Filters: squirrel.Eq{"album.name": albumSgtPeppers.Name},
+				})
+				Expect(err).ToNot(HaveOccurred())
+				Expect(albums).To(HaveLen(1))
+				Expect(albums[0].ID).To(Equal(albumSgtPeppers.ID))
+			})
+
+			It("hydrates the same fields as a non-random page", func() {
+				albums, err := GetAll(model.QueryOptions{
+					Sort: "random", Max: 1, Filters: squirrel.Eq{"album.id": albumSgtPeppers.ID},
+				})
+				Expect(err).ToNot(HaveOccurred())
+				Expect(albums).To(Equal(model.Albums{albumSgtPeppers}))
+			})
+		})
+
 		It("returns all records sorted", func() {
 			Expect(GetAll(model.QueryOptions{Sort: "name"})).To(Equal(model.Albums{
 				albumAbbeyRoad,

persistence/sql_base_repository.go

@@ -534,13 +534,26 @@ func (r sqlRepository) classifyOwnedWriteMiss(id string) error {
 	return rest.ErrNotFound
 }
 
+var joinRegex = regexp.MustCompile(`\bJOIN\b`)
+
+// countExpression returns count(*) for join-free queries; joins can fan out rows per id and
+// need the much more expensive count(distinct id) (temp b-tree over the whole result set).
+func (r sqlRepository) countExpression(query SelectBuilder) string {
+	sql, _, err := query.Columns("1").ToSql()
+	if err != nil || joinRegex.MatchString(sql) {
+		return "count(distinct " + r.tableName + ".id) as count"
+	}
+	return "count(*) as count"
+}
+
 func (r sqlRepository) count(countQuery SelectBuilder, options ...model.QueryOptions) (int64, error) {
 	countQuery = countQuery.
-		RemoveColumns().Columns("count(distinct " + r.tableName + ".id) as count").
+		RemoveColumns().
 		RemoveOffset().RemoveLimit().
 		OrderBy(r.tableName + ".id"). // To remove any ORDER BY clause that could slow down the query
 		From(r.tableName)
 	countQuery = r.applyFilters(countQuery, options...)
+	countQuery = countQuery.Columns(r.countExpression(countQuery))
 	var res struct{ Count int64 }
 	err := r.queryOne(countQuery, &res)
 	return res.Count, err

persistence/sql_base_repository_test.go

@@ -18,6 +18,22 @@ var _ = Describe("sqlRepository", func() {
 		r.tableName = "table"
 	})
 
+	Describe("countExpression", func() {
+		It("uses a plain count when the query has no joins", func() {
+			sq := squirrel.Select().From("table").Where(squirrel.Eq{"library_id": []int{1, 4}})
+			Expect(r.countExpression(sq)).To(Equal("count(*) as count"))
+		})
+		It("uses a distinct count when the query has a join", func() {
+			sq := squirrel.Select().From("table").
+				LeftJoin("annotation on annotation.item_id = table.id")
+			Expect(r.countExpression(sq)).To(Equal("count(distinct table.id) as count"))
+		})
+		It("is not fooled by parametrized values containing the word join", func() {
+			sq := squirrel.Select().From("table").Where(squirrel.Eq{"name": "the join band"})
+			Expect(r.countExpression(sq)).To(Equal("count(*) as count"))
+		})
+	})
+
 	Describe("applyOptions", func() {
 		var sq squirrel.SelectBuilder
 		BeforeEach(func() {