fix(jellycompat): fill large browse pages

This commit is contained in:
Quick
2026-06-05 16:33:40 -04:00
parent cd946b30a3
commit 08a0704a11
9 changed files with 260 additions and 125 deletions
+40 -106
View File
@@ -33,6 +33,7 @@ type BrowseFilters struct {
Sort string // created_at, release_date, rating_imdb, rating_tmdb, year, sort_title, recently_added
Order string // asc, desc
Limit int
MaxLimit int // optional caller-specific cap; zero keeps the default browse cap
Offset int
SnapshotAt *time.Time // pagination fence: exclude items created after this timestamp
}
@@ -77,7 +78,7 @@ func (r *BrowseRepository) browse(ctx context.Context, filters BrowseFilters, in
return &BrowseResult{Items: []*models.MediaItem{}, Total: 0, HasMore: false}, nil
}
dataQuery, queryArgs := plan.pagedSQL(includeTotal)
dataQuery, queryArgs := plan.pagedSQL(false)
rows, err := r.pool.Query(ctx, dataQuery, queryArgs...)
if err != nil {
return nil, fmt.Errorf("browsing media items: %w", err)
@@ -88,32 +89,31 @@ func (r *BrowseRepository) browse(ctx context.Context, filters BrowseFilters, in
items []*models.MediaItem
total int
)
if includeTotal {
items, total, err = scanBrowseItemsWithTotal(rows)
} else {
items, err = scanBrowseItems(rows)
}
items, err = scanBrowseItems(rows)
if err != nil {
return nil, err
}
hasMore := false
hasExtraRow := len(items) > plan.limit
if hasExtraRow {
items = items[:plan.limit]
}
if includeTotal {
// COUNT(*) OVER () emits no rows when the data SELECT is empty, so
// total stays 0 even when the broader result set has matching rows
// (e.g. OFFSET past the last page). Re-query the count to give
// callers the real total. Skip when offset == 0 because in that
// case an empty page genuinely means total = 0.
if len(items) == 0 && plan.offset > 0 {
switch {
case plan.offset == 0 && len(items) == 0:
total = 0
case !hasExtraRow && len(items) > 0:
total = plan.offset + len(items)
default:
countSQL, countArgs := plan.countSQL()
if err := r.pool.QueryRow(ctx, countSQL, countArgs...).Scan(&total); err != nil {
return nil, fmt.Errorf("count fallback for empty browse page: %w", err)
return nil, fmt.Errorf("count browse page: %w", err)
}
}
hasMore = total > plan.offset+plan.limit
} else if len(items) > plan.limit {
} else if hasExtraRow {
hasMore = true
items = items[:plan.limit]
}
return &BrowseResult{
@@ -151,12 +151,9 @@ type browseQueryPlan struct {
// countSQL renders a count-only query that returns the total number of rows
// matching the plan's FROM/WHERE/GROUP BY, ignoring LIMIT/OFFSET/ORDER BY.
// Used as a fallback when pagedSQL(true) returned an empty page past offset 0:
// COUNT(*) OVER () emits no rows when the data SELECT is empty, so the
// caller would otherwise see total=0 even when the broader result set has
// matching rows. Wraps the inner query in `SELECT COUNT(*) FROM (...) sub`
// so any GROUP BY in the inner query is preserved (we count groups, matching
// what COUNT(*) OVER () would compute).
// Used by exact-total callers so the data SELECT can remain a pure page query.
// Wraps the inner query in `SELECT COUNT(*) FROM (...) sub` so any GROUP BY in
// the inner query is preserved.
//
// Binds only p.args (FROM/WHERE/GROUP BY values). orderArgs are deliberately
// excluded — the count SQL has no ORDER BY clause to reference them, and
@@ -172,16 +169,12 @@ func (p browseQueryPlan) countSQL() (string, []any) {
}
// pagedSQL renders the final paged SELECT and returns it together with the
// fully-bound arg list. When includeTotal is true the SELECT list is appended
// with COUNT(*) OVER () AS total_count so the caller can read the total from
// the first scanned row in a single round trip. When includeTotal is false the
// caller has already requested a single-pass page, so we ask for one extra row
// to detect whether more pages exist without an exact count.
func (p browseQueryPlan) pagedSQL(includeTotal bool) (string, []any) {
// fully-bound arg list. The query always asks for one extra row so callers can
// detect whether more pages exist without coupling the sorted page fetch to an
// exact count.
func (p browseQueryPlan) pagedSQL(_ bool) (string, []any) {
queryLimit := p.limit
if !includeTotal {
queryLimit++
}
queryLimit++
// Bind order: where/from args, then order-by args, then LIMIT, then OFFSET.
// limitArgIdx is computed by buildBrowsePlan after consuming order args, so
// it correctly points at LIMIT after the orderArgs are bound below.
@@ -191,9 +184,6 @@ func (p browseQueryPlan) pagedSQL(includeTotal bool) (string, []any) {
offsetArgIdx := p.limitArgIdx + 1
selectList := p.selectClause
if includeTotal {
selectList += ", COUNT(*) OVER () AS total_count"
}
sql := fmt.Sprintf(
"SELECT %s FROM %s %s %s %s LIMIT $%d OFFSET $%d",
selectList, p.fromClause, p.whereClause, p.groupByClause, p.orderBy,
@@ -212,8 +202,12 @@ func (r *BrowseRepository) buildBrowsePlan(filters BrowseFilters) (browseQueryPl
if filters.Limit <= 0 {
filters.Limit = 20
}
if filters.Limit > 100 {
filters.Limit = 100
maxLimit := filters.MaxLimit
if maxLimit <= 0 {
maxLimit = 100
}
if filters.Limit > maxLimit {
filters.Limit = maxLimit
}
if filters.Offset < 0 {
filters.Offset = 0
@@ -367,7 +361,7 @@ func (r *BrowseRepository) buildBrowsePlan(filters BrowseFilters) (browseQueryPl
// bind only its FROM/WHERE/GROUP BY values; pagedSQL binds them together.
// argIdx still advances past them so limitArgIdx points at the right
// LIMIT placeholder once orderArgs are bound positionally before LIMIT.
orderBy, orderArgs := buildOrderByPlan(filters.Sort, filters.Order, filters.SnapshotAt, argIdx, singleLibraryNoDedup)
orderBy, orderArgs := buildOrderByPlan(filters.Sort, filters.Order, filters.SnapshotAt, argIdx, singleLibraryNoDedup, browseFiltersAreMovieOnly(filters))
argIdx += len(orderArgs)
selectClause := browseItemColumns("mi")
@@ -932,81 +926,13 @@ func scanBrowseItems(rows pgx.Rows) ([]*models.MediaItem, error) {
return items, nil
}
// scanBrowseItemsWithTotal scans rows that include a trailing total_count column
// emitted by COUNT(*) OVER (). Used by browse() in the includeTotal path so we
// don't fire a separate count query.
func scanBrowseItemsWithTotal(rows pgx.Rows) ([]*models.MediaItem, int, error) {
var (
items []*models.MediaItem
total int
)
for rows.Next() {
var item models.MediaItem
var rowTotal int
err := rows.Scan(
&item.ContentID,
&item.Type,
&item.Title,
&item.SortTitle,
&item.OriginalTitle,
&item.Year,
&item.Genres,
&item.ContentRating,
&item.Runtime,
&item.Overview,
&item.Tagline,
&item.RatingIMDB,
&item.RatingTMDB,
&item.RatingRTCritic,
&item.RatingRTAudience,
&item.ImdbID,
&item.TmdbID,
&item.TvdbID,
&item.PosterPath,
&item.PosterThumbhash,
&item.BackdropPath,
&item.BackdropThumbhash,
&item.LogoPath,
&item.MetadataS3Path,
&item.MetadataEtag,
&item.SeasonCount,
&item.Studios,
&item.Networks,
&item.Countries,
&item.Keywords,
&item.OriginalLanguage,
&item.ReleaseDate,
&item.FirstAirDate,
&item.LastAirDate,
&item.ShowStatus,
&item.MatchedAt,
&item.EpisodeMetadataIncomplete,
&item.EpisodeMetadataLastCheckedAt,
&item.Status,
&item.CreatedAt,
&item.UpdatedAt,
&item.AddedAt,
&rowTotal,
)
if err != nil {
return nil, 0, fmt.Errorf("scanning browse item row with total: %w", err)
}
items = append(items, &item)
total = rowTotal
}
if err := rows.Err(); err != nil {
return nil, 0, fmt.Errorf("iterating browse item rows: %w", err)
}
return items, total, nil
}
// buildOrderBy constructs the ORDER BY clause from sort and order parameters.
func buildOrderBy(sort, order string) string {
clause, _ := buildOrderByPlan(sort, order, nil, 0, false)
clause, _ := buildOrderByPlan(sort, order, nil, 0, false, false)
return clause
}
func buildOrderByPlan(sort, order string, snapshot *time.Time, argIdx int, singleLibraryNoDedup bool) (string, []any) {
func buildOrderByPlan(sort, order string, snapshot *time.Time, argIdx int, singleLibraryNoDedup bool, movieOnly bool) (string, []any) {
direction := "DESC"
if strings.EqualFold(order, "asc") {
direction = "ASC"
@@ -1027,6 +953,9 @@ func buildOrderByPlan(sort, order string, snapshot *time.Time, argIdx int, singl
}
return "ORDER BY RANDOM()", nil
case "release_date":
if movieOnly {
return fmt.Sprintf("ORDER BY mi.release_date %s%s, mi.content_id ASC", direction, nullsClause), nil
}
return fmt.Sprintf(
"ORDER BY COALESCE(mi.release_date::text, NULLIF(BTRIM(mi.first_air_date), '')) %s%s, mi.content_id ASC",
direction,
@@ -1067,6 +996,11 @@ func buildOrderByPlan(sort, order string, snapshot *time.Time, argIdx int, singl
}
}
func browseFiltersAreMovieOnly(filters BrowseFilters) bool {
types := splitTypes(filters.Type)
return len(types) == 1 && types[0] == "movie"
}
// ParseContentRatings splits a comma-separated content rating string into
// a slice. Empty input returns nil.
func ParseContentRatings(s string) []string {
+23
View File
@@ -42,3 +42,26 @@ func TestBuildOrderBy_LastAirDateReadsDenormColumn(t *testing.T) {
t.Fatalf("expected sparse last_air_date sort to push missing items last, got %q", got)
}
}
func TestBrowsePlan_ReleaseDateMovieOnlyUsesIndexedDateColumn(t *testing.T) {
repo := &BrowseRepository{}
plan, earlyEmpty, err := repo.buildBrowsePlan(BrowseFilters{
Type: "movie",
Sort: "release_date",
Order: "desc",
Limit: 1000,
})
if err != nil {
t.Fatalf("buildBrowsePlan error: %v", err)
}
if earlyEmpty {
t.Fatal("did not expect early empty result")
}
if strings.Contains(plan.orderBy, "COALESCE(") || strings.Contains(plan.orderBy, "::text") {
t.Fatalf("movie-only release_date sort must use the indexed date column, got %q", plan.orderBy)
}
want := "ORDER BY mi.release_date DESC NULLS LAST, mi.content_id ASC"
if plan.orderBy != want {
t.Fatalf("orderBy = %q, want %q", plan.orderBy, want)
}
}
+32 -5
View File
@@ -41,9 +41,11 @@ func TestQueryExecutor_PreviewPage_SkipTotal_OmitsWindowCount(t *testing.T) {
}
}
// TestBrowseRepository_browse_UsesWindowCount asserts that buildBrowsePlan +
// pagedSQL emit COUNT(*) OVER () in the data SELECT when includeTotal is true.
func TestBrowseRepository_browse_UsesWindowCount(t *testing.T) {
// TestBrowseRepository_browse_ExactTotalOmitsWindowCount asserts that
// buildBrowsePlan + pagedSQL keep the data SELECT free of COUNT(*) OVER ().
// BrowsePage runs the exact count separately when needed so large ordered
// catalogs can use top-N/index plans for the page fetch.
func TestBrowseRepository_browse_ExactTotalOmitsWindowCount(t *testing.T) {
repo := &BrowseRepository{}
plan, earlyEmpty, err := repo.buildBrowsePlan(BrowseFilters{Limit: 20})
if err != nil {
@@ -53,8 +55,8 @@ func TestBrowseRepository_browse_UsesWindowCount(t *testing.T) {
t.Fatalf("did not expect early empty result for default filters")
}
sql, _ := plan.pagedSQL(true)
if !strings.Contains(sql, "COUNT(*) OVER ()") {
t.Fatalf("expected COUNT(*) OVER () for single-pass browse count; got:\n%s", sql)
if strings.Contains(sql, "COUNT(*) OVER ()") {
t.Fatalf("exact browse totals must omit COUNT(*) OVER (); got:\n%s", sql)
}
}
@@ -75,6 +77,31 @@ func TestBrowseRepository_browse_SkipTotal_OmitsWindowCount(t *testing.T) {
}
}
func TestBrowseRepository_browse_MaxLimitAllowsCallerSpecificLargePages(t *testing.T) {
repo := &BrowseRepository{}
plan, earlyEmpty, err := repo.buildBrowsePlan(BrowseFilters{Limit: 1000, MaxLimit: 1000})
if err != nil {
t.Fatalf("buildBrowsePlan error: %v", err)
}
if earlyEmpty {
t.Fatalf("did not expect early empty result")
}
if plan.limit != 1000 {
t.Fatalf("plan limit = %d, want 1000", plan.limit)
}
defaultPlan, earlyEmpty, err := repo.buildBrowsePlan(BrowseFilters{Limit: 1000})
if err != nil {
t.Fatalf("buildBrowsePlan default cap error: %v", err)
}
if earlyEmpty {
t.Fatalf("did not expect early empty result for default cap")
}
if defaultPlan.limit != 100 {
t.Fatalf("default plan limit = %d, want 100", defaultPlan.limit)
}
}
// TestQueryExecutor_PreviewPage_CountSQL_OmitsSortOnlyJoins pins that exact
// totals do not carry ORDER BY-only joins. The page query still needs those
// joins for sorts such as added_at, but the count query only needs the base
+38 -12
View File
@@ -27,10 +27,15 @@ type LibraryPosterPresigner interface {
// directContentService relies on. Defined as an interface so tests can
// substitute a stub without standing up a Postgres pool.
type browseSource interface {
Browse(ctx context.Context, filters catalog.BrowseFilters) (*catalog.BrowseResult, error)
BrowsePage(ctx context.Context, filters catalog.BrowseFilters, includeTotal bool) (*catalog.BrowseResult, error)
ListGenres(ctx context.Context, filters catalog.BrowseFilters) ([]string, error)
}
const (
compatBrowseChunkLimit = 100
compatBrowseMaxLimit = 1000
)
// itemAccessSource is the subset of *catalog.ItemRepository that
// directContentService season/episode handlers rely on. Defined as an
// interface so tests can substitute a stub without standing up a Postgres
@@ -165,17 +170,23 @@ func (s *directContentService) BrowseItems(ctx context.Context, session *Session
filter := s.resolveFilter(ctx, session)
isPlayedFilter := params.Get("is_played") // "", "true", or "false"
contentIDs := parseContentIDParam(params.Get("content_ids"))
includeTotal := parseBool(params.Get("include_total"), true)
requestedLimit := catalog.ParseIntParam(params.Get("limit"))
if requestedLimit <= 0 {
requestedLimit = 24
}
requestedOffset := catalog.ParseIntParam(params.Get("offset"))
if requestedLimit > compatBrowseMaxLimit {
requestedLimit = compatBrowseMaxLimit
}
requestedOffset := max(catalog.ParseIntParam(params.Get("offset")), 0)
// When filtering by played status, over-fetch to compensate for filtered-out items.
fetchLimit := requestedLimit
if isPlayedFilter != "" {
fetchLimit = min(requestedLimit*3, 100)
// Played status is a profile overlay, so browse over-fetches bounded
// chunks and filters them locally instead of asking catalog for every
// row in a large library.
fetchLimit = min(max(requestedLimit*3, compatBrowseChunkLimit), compatBrowseMaxLimit)
}
filters := catalog.BrowseFilters{
@@ -191,19 +202,31 @@ func (s *directContentService) BrowseItems(ctx context.Context, session *Session
Sort: params.Get("sort"),
Order: params.Get("order"),
Limit: fetchLimit,
MaxLimit: compatBrowseMaxLimit,
Offset: requestedOffset,
}
var collected []upstreamListItem
totalFromCatalog := 0
maxIterations := 5
totalKnown := false
hasMore := false
scannedRows := 0
maxScannedRows := requestedLimit
if isPlayedFilter != "" {
maxScannedRows = max(requestedLimit*5, compatBrowseChunkLimit)
}
for range maxIterations {
result, err := s.browseRepo.Browse(ctx, filters)
for len(collected) < requestedLimit && scannedRows < maxScannedRows {
wantTotal := includeTotal && !totalKnown
result, err := s.browseRepo.BrowsePage(ctx, filters, wantTotal)
if err != nil {
return nil, fmt.Errorf("browse items: %w", err)
}
totalFromCatalog = result.Total
if wantTotal {
totalFromCatalog = result.Total
totalKnown = true
}
hasMore = result.HasMore
batch := make([]upstreamListItem, 0, len(result.Items))
for _, mi := range result.Items {
@@ -232,21 +255,24 @@ func (s *directContentService) BrowseItems(ctx context.Context, session *Session
collected = append(collected, batch...)
}
// Stop if we have enough or catalog is exhausted.
if len(collected) >= requestedLimit || len(result.Items) < filters.Limit {
scannedRows += len(result.Items)
if len(result.Items) == 0 || !result.HasMore {
break
}
filters.Offset += filters.Limit
filters.Offset += len(result.Items)
}
// Trim to requested limit.
if len(collected) > requestedLimit {
collected = collected[:requestedLimit]
}
if totalKnown {
hasMore = totalFromCatalog > requestedOffset+requestedLimit
}
return &upstreamBrowseResponse{
Total: totalFromCatalog,
HasMore: totalFromCatalog > requestedOffset+requestedLimit,
HasMore: hasMore,
Items: collected,
}, nil
}
+111 -2
View File
@@ -2,6 +2,7 @@ package jellycompat
import (
"context"
"fmt"
"net/url"
"testing"
"time"
@@ -329,10 +330,28 @@ func (s *progressCountingStore) DeleteLibraryPlaybackPreference(context.Context,
type stubBrowseSource struct {
items []*models.MediaItem
total int
calls []stubBrowseCall
}
func (s *stubBrowseSource) Browse(_ context.Context, _ catalog.BrowseFilters) (*catalog.BrowseResult, error) {
return &catalog.BrowseResult{Items: s.items, Total: s.total}, nil
type stubBrowseCall struct {
filters catalog.BrowseFilters
includeTotal bool
}
func (s *stubBrowseSource) BrowsePage(_ context.Context, filters catalog.BrowseFilters, includeTotal bool) (*catalog.BrowseResult, error) {
s.calls = append(s.calls, stubBrowseCall{filters: filters, includeTotal: includeTotal})
start := min(max(filters.Offset, 0), len(s.items))
end := min(start+max(filters.Limit, 0), len(s.items))
total := 0
if includeTotal {
total = s.total
}
return &catalog.BrowseResult{
Items: append([]*models.MediaItem(nil), s.items[start:end]...),
Total: total,
HasMore: end < len(s.items),
}, nil
}
func (s *stubBrowseSource) ListGenres(_ context.Context, _ catalog.BrowseFilters) ([]string, error) {
@@ -413,6 +432,84 @@ func TestBrowseItems_FetchesProgressWhenPlayedFilterSet(t *testing.T) {
}
}
func TestBrowseItems_FillsLargeJellyfinLimitInSingleCatalogPage(t *testing.T) {
provider := newProgressCountingStoreProvider()
browse := &stubBrowseSource{
items: makeBrowseTestMediaItems(400),
total: 400,
}
svc := newDirectContentServiceForTest(browse, provider)
session := &Session{StreamAppUserID: 1, ProfileID: "profile-1"}
params := url.Values{}
params.Set("limit", "250")
params.Set("offset", "10")
params.Set("include_total", "true")
result, err := svc.BrowseItems(context.Background(), session, params)
if err != nil {
t.Fatalf("BrowseItems returned error: %v", err)
}
if got, want := len(result.Items), 250; got != want {
t.Fatalf("items length = %d, want %d", got, want)
}
if result.Total != 400 {
t.Fatalf("Total = %d, want 400", result.Total)
}
if !result.HasMore {
t.Fatal("HasMore = false, want true")
}
if got, want := len(browse.calls), 1; got != want {
t.Fatalf("BrowsePage calls = %d, want %d", got, want)
}
call := browse.calls[0]
if call.filters.Limit != 250 {
t.Fatalf("Limit = %d, want 250", call.filters.Limit)
}
if call.filters.MaxLimit != compatBrowseMaxLimit {
t.Fatalf("MaxLimit = %d, want %d", call.filters.MaxLimit, compatBrowseMaxLimit)
}
if call.filters.Offset != 10 {
t.Fatalf("Offset = %d, want 10", call.filters.Offset)
}
if !call.includeTotal {
t.Fatal("includeTotal = false, want true")
}
}
func TestBrowseItems_HonorsIncludeTotalFalse(t *testing.T) {
browse := &stubBrowseSource{
items: makeBrowseTestMediaItems(300),
total: 300,
}
svc := newDirectContentServiceForTest(browse, nil)
session := &Session{StreamAppUserID: 1, ProfileID: "profile-1"}
params := url.Values{}
params.Set("limit", "150")
params.Set("include_total", "false")
result, err := svc.BrowseItems(context.Background(), session, params)
if err != nil {
t.Fatalf("BrowseItems returned error: %v", err)
}
if got, want := len(result.Items), 150; got != want {
t.Fatalf("items length = %d, want %d", got, want)
}
if result.Total != 0 {
t.Fatalf("Total = %d, want 0 when include_total=false", result.Total)
}
if got, want := len(browse.calls), 1; got != want {
t.Fatalf("BrowsePage calls = %d, want %d", got, want)
}
for i, call := range browse.calls {
if call.includeTotal {
t.Fatalf("call %d includeTotal = true, want false", i)
}
}
}
// TestEnrichListItemsUserData_BatchesIntoSingleFetch verifies that
// enrichListItemsUserData makes exactly one batched ListProgressByMediaItems
// call regardless of batch size — not N+1 per-item fetches.
@@ -437,3 +534,15 @@ func TestEnrichListItemsUserData_BatchesIntoSingleFetch(t *testing.T) {
want, got)
}
}
func makeBrowseTestMediaItems(count int) []*models.MediaItem {
items := make([]*models.MediaItem, 0, count)
for i := range count {
items = append(items, &models.MediaItem{
ContentID: fmt.Sprintf("movie-%03d", i),
Type: "movie",
Title: fmt.Sprintf("Movie %03d", i),
})
}
return items
}
+1
View File
@@ -197,6 +197,7 @@ func buildBrowseParams(query itemsQuery) url.Values {
if query.order != "" {
params.Set("order", query.order)
}
params.Set("include_total", strconv.FormatBool(query.enableTotalRecordCount))
if query.personID > 0 {
params.Set("person_id", strconv.FormatInt(query.personID, 10))
}
+11
View File
@@ -29,6 +29,17 @@ func TestParseItemsQueryAcceptsIsFavoriteParam(t *testing.T) {
}
}
func TestBuildBrowseParamsPropagatesEnableTotalRecordCount(t *testing.T) {
req := httptest.NewRequest("GET", "/Items?EnableTotalRecordCount=false", nil)
query := parseItemsQuery(req, NewResourceIDCodec())
params := buildBrowseParams(query)
if got := params.Get("include_total"); got != "false" {
t.Fatalf("include_total = %q, want false", got)
}
}
func TestMapSortByReleaseDate(t *testing.T) {
tests := []string{
"PremiereDate",
@@ -0,0 +1 @@
DROP INDEX IF EXISTS public.idx_media_items_movie_release_date_desc_content;
@@ -0,0 +1,3 @@
CREATE INDEX IF NOT EXISTS idx_media_items_movie_release_date_desc_content
ON public.media_items USING btree (release_date DESC NULLS LAST, content_id ASC)
WHERE type = 'movie';