diff --git a/internal/catalog/episode_catalog_source.go b/internal/catalog/episode_catalog_source.go index 3988672c..56b19e3a 100644 --- a/internal/catalog/episode_catalog_source.go +++ b/internal/catalog/episode_catalog_source.go @@ -12,6 +12,7 @@ const episodeCatalogSelectBody = `( 'episode'::text AS type, COALESCE(NULLIF(BTRIM(e.title), ''), 'Episode ' || e.episode_number::text) AS title, COALESCE(NULLIF(BTRIM(e.title), ''), 'Episode ' || e.episode_number::text) AS sort_title, + LOWER(COALESCE(NULLIF(BTRIM(e.title), ''), 'Episode ' || e.episode_number::text)) AS sort_key, COALESCE(NULLIF(BTRIM(e.default_metadata_language), ''), COALESCE(si.default_metadata_language, '')) AS default_metadata_language, ''::text AS original_title, COALESCE(si.year, EXTRACT(YEAR FROM e.air_date)::integer, 0) AS year, @@ -45,6 +46,7 @@ const episodeCatalogSelectBody = `( NULL::text AS last_air_date, e.air_date AS last_air_date_at, si.air_time, + COALESCE(si.show_status, '') AS show_status, si.matched_at, si.last_refreshed, si.refresh_failures, diff --git a/internal/catalog/episode_library_repo.go b/internal/catalog/episode_library_repo.go index 8b4e4d2a..f07500e7 100644 --- a/internal/catalog/episode_library_repo.go +++ b/internal/catalog/episode_library_repo.go @@ -17,9 +17,24 @@ func NewEpisodeLibraryRepository(pool *pgxpool.Pool) *EpisodeLibraryRepository { return &EpisodeLibraryRepository{pool: pool} } -// ReconcileFolderMembership removes episode memberships for episodes that no longer -// have any present files in the given folder. +// ReconcileFolderMembership restores missing episode memberships and removes +// memberships for episodes that no longer have any present files in the given +// folder. The returned count is the number of removed stale memberships. func (r *EpisodeLibraryRepository) ReconcileFolderMembership(ctx context.Context, folderID int) (int, error) { + if _, err := r.pool.Exec(ctx, ` + INSERT INTO episode_libraries (episode_id, media_folder_id, first_seen_at) + SELECT mf.episode_id, mf.media_folder_id, MIN(mf.created_at) + FROM media_files mf + JOIN episodes e ON e.content_id = mf.episode_id + WHERE mf.media_folder_id = $1 + AND mf.missing_since IS NULL + AND mf.episode_id IS NOT NULL + GROUP BY mf.episode_id, mf.media_folder_id + ON CONFLICT (episode_id, media_folder_id) DO NOTHING + `, folderID); err != nil { + return 0, fmt.Errorf("restoring episode library membership: %w", err) + } + tag, err := r.pool.Exec(ctx, ` DELETE FROM episode_libraries el WHERE el.media_folder_id = $1 diff --git a/internal/catalog/name_prefix_sql_test.go b/internal/catalog/name_prefix_sql_test.go index 629b8b6d..9f30bffa 100644 --- a/internal/catalog/name_prefix_sql_test.go +++ b/internal/catalog/name_prefix_sql_test.go @@ -23,7 +23,7 @@ func TestQueryExecutor_NamePrefix_PushedIntoWHERE(t *testing.T) { } // First arm: sort-key expression matching idx_media_items_sort_key. - if !strings.Contains(sql, "LOWER(COALESCE(NULLIF(BTRIM(mi.sort_title),''), mi.title)) LIKE") { + if !strings.Contains(sql, "LOWER(COALESCE(NULLIF(BTRIM(mi.sort_title), ''), mi.title)) LIKE") { t.Fatalf("expected sort-key LIKE arm matching idx_media_items_sort_key; got %q", sql) } // Second arm: LOWER(title) matching idx_media_items_search_exact_title; @@ -53,6 +53,31 @@ func TestQueryExecutor_NamePrefix_PushedIntoWHERE(t *testing.T) { } } +func TestQueryExecutor_NamePrefix_UsesEpisodeSortKeyForEpisodeScope(t *testing.T) { + exec := &QueryExecutor{} + access := AccessFilter{NamePrefix: "Pilot"} + + sql, args, err := exec.buildPreviewPageSQL( + QueryDefinition{MediaScope: "episode", LibraryIDs: []int{2}}, + access, + 20, + 0, + true, + ) + if err != nil { + t.Fatalf("buildPreviewPageSQL returned error: %v", err) + } + if !strings.Contains(sql, "mi.sort_key LIKE") { + t.Fatalf("expected episode prefix to use projected sort_key; got %q", sql) + } + if strings.Contains(sql, "BTRIM(mi.sort_title)") { + t.Fatalf("episode prefix should not recompute sort_title expression; got %q", sql) + } + if len(args) < 2 || args[1] != "pilot%" { + t.Fatalf("expected episode library args followed by prefix arg; got %v", args) + } +} + // TestBrowseFilters_NamePrefix_BothArmsSargable asserts that the dual-column // LIKE in BrowseRepository's WHERE clause uses an expression that matches // idx_media_items_sort_key (migration 102) on the first arm and diff --git a/internal/catalog/query_builder.go b/internal/catalog/query_builder.go index 2126aecd..f56a222f 100644 --- a/internal/catalog/query_builder.go +++ b/internal/catalog/query_builder.go @@ -234,10 +234,10 @@ func (qb *QueryBuilder) BuildSortPlan(sortConfig QuerySort) (QuerySortPlan, erro plan.OrderBy = qb.orderByExpr(qb.releaseDateSortExpr(), dir, true, titleExpr) return plan, nil case "added_at": - expr, joins, args := qb.addedAtSortPlan() + expr, joins, args, nullsLast := qb.addedAtSortPlan() plan.Joins = joins plan.Args = args - plan.OrderBy = qb.orderByExpr(expr, dir, len(joins) > 0, titleExpr) + plan.OrderBy = qb.orderByExpr(expr, dir, nullsLast, titleExpr) return plan, nil case "content_rating": rankExpr := qb.contentRatingRankExpr() @@ -1016,6 +1016,9 @@ func (qb *QueryBuilder) buildTimestampComparisonClause(column, op string, value } func (qb *QueryBuilder) normalizedTitleExpr() string { + if isEpisodeCatalogScope(qb.mediaScope) { + return fmt.Sprintf("%s.sort_key", qb.alias) + } return fmt.Sprintf( "LOWER(COALESCE(NULLIF(BTRIM(%s.sort_title), ''), %s.title))", qb.alias, @@ -1039,13 +1042,22 @@ func (qb *QueryBuilder) orderByExpr(expr, dir string, nullsLast bool, titleExpr return clause } -func (qb *QueryBuilder) addedAtSortPlan() (string, []string, []any) { +func (qb *QueryBuilder) addedAtSortPlan() (string, []string, []any, bool) { if len(qb.libraryIDs) == 0 { - return fmt.Sprintf("%s.created_at", qb.alias), nil, nil + return fmt.Sprintf("%s.created_at", qb.alias), nil, nil, false } placeholders, args := qb.consumeIntArgs(qb.libraryIDs) if isEpisodeCatalogScope(qb.mediaScope) { + if len(qb.libraryIDs) == 1 { + joinSQL := fmt.Sprintf( + `JOIN episode_libraries sort_added ON sort_added.episode_id = %s.content_id AND sort_added.media_folder_id = %s`, + qb.alias, + placeholders[0], + ) + return "sort_added.first_seen_at", []string{joinSQL}, args, false + } + joinSQL := fmt.Sprintf( `LEFT JOIN ( SELECT el.episode_id AS content_id, MIN(el.first_seen_at) AS added_at @@ -1056,7 +1068,7 @@ func (qb *QueryBuilder) addedAtSortPlan() (string, []string, []any) { strings.Join(placeholders, ", "), qb.alias, ) - return "sort_added.added_at", []string{joinSQL}, args + return "sort_added.added_at", []string{joinSQL}, args, true } joinSQL := fmt.Sprintf( @@ -1069,7 +1081,7 @@ func (qb *QueryBuilder) addedAtSortPlan() (string, []string, []any) { strings.Join(placeholders, ", "), qb.libraryContentExpr(), ) - return "sort_added.added_at", []string{joinSQL}, args + return "sort_added.added_at", []string{joinSQL}, args, true } func (qb *QueryBuilder) contentRatingRankExpr() string { diff --git a/internal/catalog/query_builder_test.go b/internal/catalog/query_builder_test.go index dd493de0..41400c14 100644 --- a/internal/catalog/query_builder_test.go +++ b/internal/catalog/query_builder_test.go @@ -74,6 +74,27 @@ func TestBuildSortClause_ReleaseDateUsesEpisodeAirDateForEpisodeScope(t *testing } } +func TestBuildSortClause_TitleUsesEpisodeSortKeyForEpisodeScope(t *testing.T) { + clause, args, err := NewQueryBuilder("mi"). + WithMediaScope("episode"). + BuildSortClause(QuerySort{ + Field: "title", + Order: "asc", + }) + if err != nil { + t.Fatalf("BuildSortClause returned error: %v", err) + } + if len(args) != 0 { + t.Fatalf("expected no args, got %v", args) + } + if !strings.Contains(clause, "ORDER BY mi.sort_key ASC, mi.content_id ASC") { + t.Fatalf("expected episode title sort to use indexed sort_key, got %q", clause) + } + if strings.Contains(clause, "BTRIM(mi.sort_title)") { + t.Fatalf("episode title sort should not recompute sort_title expression, got %q", clause) + } +} + func TestBuildSortClause_AddedAtUsesScopedFirstSeenAt(t *testing.T) { plan, err := NewQueryBuilder("mi"). WithLibraryScope([]int{3, 7}). @@ -201,15 +222,43 @@ func TestBuildSortPlan_AddedAtUsesEpisodeLibraryMembershipForEpisodeScope(t *tes if len(plan.Joins) != 1 { t.Fatalf("expected one join, got %v", plan.Joins) } - if !strings.Contains(plan.Joins[0], "FROM episode_libraries el") { + if !strings.Contains(plan.Joins[0], "JOIN episode_libraries sort_added") { t.Fatalf("expected episode added_at join to use episode_libraries, got %q", plan.Joins[0]) } - if !strings.Contains(plan.Joins[0], "GROUP BY el.episode_id") { - t.Fatalf("expected episode added_at join to group by episode_id, got %q", plan.Joins[0]) + if !strings.Contains(plan.Joins[0], "sort_added.media_folder_id = $1") { + t.Fatalf("expected direct episode added_at join to bind one library, got %q", plan.Joins[0]) } - if !strings.Contains(plan.Joins[0], "sort_added.content_id = mi.content_id") { + if !strings.Contains(plan.Joins[0], "sort_added.episode_id = mi.content_id") { t.Fatalf("expected episode added_at join to match episode content_id, got %q", plan.Joins[0]) } + if !strings.Contains(plan.OrderBy, "ORDER BY sort_added.first_seen_at DESC, mi.sort_key ASC, mi.content_id ASC") { + t.Fatalf("expected single-library episode added_at sort to use first_seen_at without NULLS LAST, got %q", plan.OrderBy) + } +} + +func TestBuildSortPlan_AddedAtAggregatesEpisodeLibrariesForMultiLibraryScope(t *testing.T) { + plan, err := NewQueryBuilder("mi"). + WithMediaScope("episode"). + WithLibraryScope([]int{6, 7}). + BuildSortPlan(QuerySort{Field: "added_at", Order: "desc"}) + if err != nil { + t.Fatalf("BuildSortPlan returned error: %v", err) + } + if len(plan.Args) != 2 || plan.Args[0] != 6 || plan.Args[1] != 7 { + t.Fatalf("expected scoped library args [6 7], got %v", plan.Args) + } + if len(plan.Joins) != 1 { + t.Fatalf("expected one join, got %v", plan.Joins) + } + if !strings.Contains(plan.Joins[0], "FROM episode_libraries el") { + t.Fatalf("expected episode added_at aggregate to use episode_libraries, got %q", plan.Joins[0]) + } + if !strings.Contains(plan.Joins[0], "GROUP BY el.episode_id") { + t.Fatalf("expected multi-library episode added_at join to group by episode_id, got %q", plan.Joins[0]) + } + if !strings.Contains(plan.OrderBy, "sort_added.added_at DESC NULLS LAST") { + t.Fatalf("expected multi-library episode added_at sort to keep NULLS LAST, got %q", plan.OrderBy) + } } func TestBuildSortPlan_FileSortUsesEpisodeIDsForEpisodeScope(t *testing.T) { diff --git a/internal/catalog/query_executor.go b/internal/catalog/query_executor.go index 6c0ea0a0..21317399 100644 --- a/internal/catalog/query_executor.go +++ b/internal/catalog/query_executor.go @@ -47,7 +47,7 @@ func (e *QueryExecutor) PreviewPage( return nil, 0, false, err } - pagedSQL, pagedArgs := build.pagedSQL(includeTotal) + pagedSQL, pagedArgs := build.pagedSQL(false) rows, err := e.Pool.Query(ctx, pagedSQL, pagedArgs...) if err != nil { return nil, 0, false, fmt.Errorf("querying preview items: %w", err) @@ -58,14 +58,15 @@ func (e *QueryExecutor) PreviewPage( items []*models.MediaItem total int ) - if includeTotal { - items, total, err = scanItemsWithTotal(rows) - } else { - items, err = scanItems(rows) - } + items, err = scanItems(rows) if err != nil { return nil, 0, false, err } + hasMore := false + if len(items) > build.limit { + hasMore = true + items = items[:build.limit] + } // The preview path uses itemColumns which scans CreatedAt but not // AddedAt (set only by browse queries via MIN(mil.first_seen_at)). // Fall back to CreatedAt so the API response includes added_at. @@ -76,32 +77,15 @@ func (e *QueryExecutor) PreviewPage( } } - hasMore := false 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. - // - // Use build.offset (normalized in buildPreviewPagePlan) rather than - // the raw offset parameter — a caller-supplied negative offset is - // floored to 0 in the plan, and the SQL uses the plan's value, so - // the fallback condition and HasMore must match. - if len(items) == 0 && build.offset > 0 { - countSQL, countArgs := build.countSQL() - if err := e.Pool.QueryRow(ctx, countSQL, countArgs...).Scan(&total); err != nil { - return nil, 0, false, fmt.Errorf("count fallback for empty page: %w", err) - } + countSQL, countArgs := build.countSQL() + if err := e.Pool.QueryRow(ctx, countSQL, countArgs...).Scan(&total); err != nil { + return nil, 0, false, fmt.Errorf("counting preview items: %w", err) } hasMore = total > build.offset+len(items) return items, total, hasMore, nil } - if len(items) > build.limit { - hasMore = true - items = items[:build.limit] - } return items, 0, hasMore, nil } @@ -121,6 +105,9 @@ type previewPagePlan struct { // fromClausePaged is the FROM clause for the paged query (includes any // sort-plan joins). fromClausePaged string + // fromClauseCount is the FROM clause for exact totals. It includes filter + // joins, but intentionally excludes sort-only joins. + fromClauseCount string whereClause string args []any orderBy string @@ -131,41 +118,33 @@ type previewPagePlan struct { } // countSQL renders a count-only query that returns the total number of rows -// matching the plan's WHERE clause, 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). +// matching the plan's WHERE clause, ignoring LIMIT/OFFSET/ORDER BY. PreviewPage +// uses it when callers need an exact total; keeping the count separate lets the +// data SELECT use top-N/index plans instead of forcing COUNT(*) OVER () across +// every matching row. Wraps the inner query in `SELECT COUNT(*) FROM (...) sub` +// so any GROUP BY in the inner query is preserved. // -// Bind cteArgs + args + sortArgs in the same order as pagedSQL. fromClausePaged -// embeds sort-plan join clauses (ORDER BY needs them to project the join -// columns), and those clauses reference $N placeholders for sortArgs; omitting -// sortArgs here would break sorts that need bound join args (added_at, -// progress, date_viewed, plays, resolution, bitrate). LIMIT/OFFSET are -// intentionally dropped — count is over the full filtered set. +// Bind cteArgs + args only. Sort-only joins and their args are intentionally +// excluded because ordering does not affect the filtered row count. func (p previewPagePlan) countSQL() (string, []any) { args := append([]any{}, p.cteArgs...) args = append(args, p.args...) - args = append(args, p.sortArgs...) withClause := "" if len(p.ctes) > 0 { withClause = "WITH " + strings.Join(p.ctes, ",\n") + "\n" } sql := fmt.Sprintf( "%sSELECT COUNT(*) FROM (SELECT 1 %s %s) sub", - withClause, p.fromClausePaged, p.whereClause, + withClause, p.fromClauseCount, p.whereClause, ) return sql, args } // 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 we -// ask the database for one extra row to detect more pages without an exact -// count. +// fully-bound arg list. When includeTotal is false we ask the database for one +// extra row to detect more pages without an exact count. Exact totals are +// handled by countSQL instead of COUNT(*) OVER () so the page query can stop +// after the requested rows. func (p previewPagePlan) pagedSQL(includeTotal bool) (string, []any) { queryLimit := p.limit if !includeTotal { @@ -182,9 +161,6 @@ func (p previewPagePlan) pagedSQL(includeTotal bool) (string, []any) { args = append(args, p.offset) } selectList := qualifiedListItemColumns("mi") - if includeTotal { - selectList += ", COUNT(*) OVER () AS total_count" - } withClause := "" if len(p.ctes) > 0 { withClause = "WITH " + strings.Join(p.ctes, ",\n") + "\n" @@ -257,7 +233,7 @@ func (e *QueryExecutor) buildPreviewPagePlan( builder := NewQueryBuilder("mi"). WithArgIdx(len(baseArgs)+1). WithUserScope(access.UserID, access.ProfileID). - WithMediaScope(def.MediaScope). + WithMediaScope(effectiveScope). WithLibraryScope(libraryIDs) filterWhere, filterArgs, err := builder.Build(def) if err != nil { @@ -314,16 +290,15 @@ func (e *QueryExecutor) buildPreviewPagePlan( } if prefix := strings.TrimSpace(access.NamePrefix); prefix != "" { - // Dual-column OR matching browse.go and favorites_browse.go: items - // where a curated sort_title differs from title (e.g. title="The Office", - // sort_title="Office, The") would be silently lost on prefix="the" if - // we only checked the COALESCE'd sort-key expression. First arm uses - // the idx_media_items_sort_key expression (migration 102); second arm - // uses idx_media_items_search_exact_title on LOWER(title) (migration 001). - // Both arms are sargable; the planner can BitmapOr them. + // Dual-column OR matching browse.go and favorites_browse.go: items where + // a curated sort_title differs from title (e.g. title="The Office", + // sort_title="Office, The") would be silently lost on prefix="the" if we + // only checked the COALESCE'd sort-key expression. The first arm uses the + // scope-specific sort key; the second arm keeps literal title prefixes. + prefixSortExpr := builder.normalizedTitleExpr() conditions = append(conditions, fmt.Sprintf( - "(LOWER(COALESCE(NULLIF(BTRIM(mi.sort_title),''), mi.title)) LIKE $%d ESCAPE '\\' OR LOWER(mi.title) LIKE $%d ESCAPE '\\')", - argIdx, argIdx, + "(%s LIKE $%d ESCAPE '\\' OR LOWER(mi.title) LIKE $%d ESCAPE '\\')", + prefixSortExpr, argIdx, argIdx, )) args = append(args, escapePrefixForLike(prefix)+"%") argIdx++ @@ -334,6 +309,7 @@ func (e *QueryExecutor) buildPreviewPagePlan( whereClause = "WHERE " + strings.Join(conditions, " AND ") } fromClauseBase := "FROM " + baseRelation + fromClauseCount := fromClauseBase if limit <= 0 { limit = 20 @@ -363,9 +339,11 @@ func (e *QueryExecutor) buildPreviewPagePlan( ctes = []string{UserHistoryCTESQL(1)} fromClausePaged = rebindSQLPlaceholders(fromClausePaged, cteShift) + fromClauseCount = rebindSQLPlaceholders(fromClauseCount, cteShift) whereClause = rebindSQLPlaceholders(whereClause, cteShift) sortPlan.OrderBy = rebindSQLPlaceholders(sortPlan.OrderBy, cteShift) fromClausePaged += " LEFT JOIN user_last_watched uhist ON uhist.media_item_id = mi.content_id" + fromClauseCount += " LEFT JOIN user_last_watched uhist ON uhist.media_item_id = mi.content_id" limitArgIdx += cteShift } @@ -373,6 +351,7 @@ func (e *QueryExecutor) buildPreviewPagePlan( ctes: ctes, cteArgs: cteArgs, fromClausePaged: fromClausePaged, + fromClauseCount: fromClauseCount, whereClause: whereClause, args: args, orderBy: sortPlan.OrderBy, @@ -385,8 +364,9 @@ func (e *QueryExecutor) buildPreviewPagePlan( // buildPreviewPageSQL is a test-friendly facade over buildPreviewPagePlan that // returns the rendered paged SELECT plus the bound args. It performs no I/O. -// When includeTotal is true the emitted SELECT carries COUNT(*) OVER () as a -// total_count column so PreviewPage can avoid a separate count query. +// includeTotal only controls whether the page query fetches exactly limit rows +// or an extra row for has-more detection; exact totals are rendered separately +// by previewPagePlan.countSQL. func (e *QueryExecutor) buildPreviewPageSQL( def QueryDefinition, access AccessFilter, @@ -413,11 +393,10 @@ func escapePrefixForLike(s string) string { } // buildLibraryScopeJoin returns a WHERE clause that scopes the outer query -// to items whose membership in media_item_libraries (or episode_libraries for -// the episode catalog scope) matches the allow/deny lists. The clause is an -// EXISTS / NOT EXISTS semi-join that uses the (content_id, media_folder_id) -// PRIMARY KEY index directly without fanning out for items present in -// multiple libraries — Audit Pattern D (2026-05-01 §3 Pattern D). The prior +// to items whose library membership matches the allow/deny lists. The clause +// is an EXISTS / NOT EXISTS semi-join that uses membership indexes +// directly without fanning out for items present in multiple libraries — Audit +// Pattern D (2026-05-01 §3 Pattern D). The prior // shape wrapped the join in a SELECT DISTINCT subquery to defuse that // fanout; the DISTINCT was load-bearing because the PK is on the (content, // folder) PAIR, not on content alone. EXISTS is the canonical non-fanout diff --git a/internal/catalog/query_executor_test.go b/internal/catalog/query_executor_test.go index 128fdaeb..98881851 100644 --- a/internal/catalog/query_executor_test.go +++ b/internal/catalog/query_executor_test.go @@ -167,6 +167,64 @@ func TestEpisodeCatalogBaseRelationForLibraries_UsesEpisodeLibraries(t *testing. } } +func TestEpisodeCatalogProjectionIncludesSharedCatalogColumns(t *testing.T) { + sql, _, err := (&QueryExecutor{}).buildPreviewPageSQL( + QueryDefinition{ + MediaScope: "episode", + LibraryIDs: []int{2}, + Sort: QuerySort{Field: "title", Order: "asc"}, + }, + AccessFilter{}, + 20, + 0, + true, + ) + if err != nil { + t.Fatalf("buildPreviewPageSQL error: %v", err) + } + if !strings.Contains(sql, "COALESCE(si.show_status, '') AS show_status") { + t.Fatalf("expected episode projection to include show_status, got %s", sql) + } + if !strings.Contains(sql, "mi.show_status") { + t.Fatalf("expected outer catalog select to reference show_status, got %s", sql) + } + if !strings.Contains(sql, "LOWER(COALESCE(NULLIF(BTRIM(e.title), ''), 'Episode ' || e.episode_number::text)) AS sort_key") { + t.Fatalf("expected episode projection to include sort_key, got %s", sql) + } + if !strings.Contains(sql, "ORDER BY mi.sort_key ASC, mi.content_id ASC") { + t.Fatalf("expected episode title sort to use sort_key, got %s", sql) + } +} + +func TestEpisodeCatalogSingleLibraryAddedAtUsesDirectMembershipJoin(t *testing.T) { + sql, _, err := (&QueryExecutor{}).buildPreviewPageSQL( + QueryDefinition{ + MediaScope: "episode", + LibraryIDs: []int{2}, + Sort: QuerySort{Field: "added_at", Order: "desc"}, + }, + AccessFilter{}, + 20, + 0, + true, + ) + if err != nil { + t.Fatalf("buildPreviewPageSQL error: %v", err) + } + if !strings.Contains(sql, "JOIN episode_libraries sort_added") { + t.Fatalf("expected direct episode_libraries join for single-library added_at sort, got %s", sql) + } + if !strings.Contains(sql, "sort_added.media_folder_id = $2") { + t.Fatalf("expected direct added_at join to bind the single library, got %s", sql) + } + if strings.Contains(sql, "GROUP BY el.episode_id") { + t.Fatalf("single-library added_at sort should avoid aggregate membership join, got %s", sql) + } + if !strings.Contains(sql, "ORDER BY sort_added.first_seen_at DESC, mi.sort_key ASC, mi.content_id ASC") { + t.Fatalf("expected added_at order to use first_seen_at without NULLS LAST, got %s", sql) + } +} + func TestRebindSQLPlaceholders(t *testing.T) { got := rebindSQLPlaceholders("mi.created_at <= $1 AND mi.year >= $2", 3) want := "mi.created_at <= $4 AND mi.year >= $5" diff --git a/internal/catalog/window_count_test.go b/internal/catalog/window_count_test.go index 5f5c5f39..3ac01402 100644 --- a/internal/catalog/window_count_test.go +++ b/internal/catalog/window_count_test.go @@ -12,17 +12,18 @@ import ( // for use in arg-count assertions across countSQL tests. var placeholderRE = regexp.MustCompile(`\$(\d+)`) -// TestQueryExecutor_PreviewPage_UsesWindowCount asserts that buildPreviewPageSQL -// emits a single-pass paged SELECT that includes COUNT(*) OVER () so PreviewPage -// no longer needs a separate count query when includeTotal is true. -func TestQueryExecutor_PreviewPage_UsesWindowCount(t *testing.T) { +// TestQueryExecutor_PreviewPage_ExactTotalOmitsWindowCount asserts that +// buildPreviewPageSQL keeps the data SELECT free of COUNT(*) OVER (). PreviewPage +// runs the exact count separately so large ordered catalogs can use top-N/index +// plans for the page fetch. +func TestQueryExecutor_PreviewPage_ExactTotalOmitsWindowCount(t *testing.T) { exec := &QueryExecutor{Scope: "movie", BaseRelationSQL: "media_items mi"} sql, _, err := exec.buildPreviewPageSQL(QueryDefinition{}, AccessFilter{}, 20, 0, true /* includeTotal */) if err != nil { t.Fatalf("buildPreviewPageSQL error: %v", err) } - if !strings.Contains(sql, "COUNT(*) OVER ()") { - t.Fatalf("expected COUNT(*) OVER () for single-pass count; got:\n%s", sql) + if strings.Contains(sql, "COUNT(*) OVER ()") { + t.Fatalf("PreviewPage exact totals must omit COUNT(*) OVER (); got:\n%s", sql) } } @@ -74,16 +75,11 @@ func TestBrowseRepository_browse_SkipTotal_OmitsWindowCount(t *testing.T) { } } -// TestQueryExecutor_PreviewPage_CountSQL_BindsSortJoinArgs pins that -// previewPagePlan.countSQL binds the sort-plan join args (sortArgs) — not -// just cteArgs+args. fromClausePaged embeds sort-plan join clauses -// (added_at, progress, date_viewed, plays, resolution, bitrate all need -// LIBRARY-id-bound joins), so omitting sortArgs would leave bound -// placeholders inside the FROM clause unfilled and Postgres would error -// out with "missing argument" at the count fallback path. -// -// Regression guard for the post-perf-overhaul code review (macroscope High). -func TestQueryExecutor_PreviewPage_CountSQL_BindsSortJoinArgs(t *testing.T) { +// 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 +// relation plus filter joins. +func TestQueryExecutor_PreviewPage_CountSQL_OmitsSortOnlyJoins(t *testing.T) { exec := &QueryExecutor{Scope: "movie", BaseRelationSQL: "media_items mi"} plan, err := exec.buildPreviewPagePlan( QueryDefinition{ @@ -102,15 +98,15 @@ func TestQueryExecutor_PreviewPage_CountSQL_BindsSortJoinArgs(t *testing.T) { sql, args := plan.countSQL() - // The sort-plan LEFT JOIN must be embedded in the count SQL — that's the - // reason sortArgs binding matters at all. - if !strings.Contains(sql, "sort_added") { - t.Fatalf("countSQL must include addedAtSortPlan's LEFT JOIN; got:\n%s", sql) + if strings.Contains(sql, "sort_added") { + t.Fatalf("countSQL must omit addedAtSortPlan's LEFT JOIN; got:\n%s", sql) + } + if len(args) != 1 { + t.Fatalf("countSQL must omit sortArgs and bind only the library-scope arg; got %v", args) } - // Verify args length covers every $N placeholder in the SQL. If we - // dropped sortArgs, the highest $N would exceed len(args) and Postgres - // would fail with "missing argument". + // Verify args still cover every $N placeholder after dropping sort-only + // joins and sortArgs. maxIdx := 0 for _, m := range placeholderRE.FindAllStringSubmatch(sql, -1) { idx, _ := strconv.Atoi(m[1]) @@ -122,23 +118,20 @@ func TestQueryExecutor_PreviewPage_CountSQL_BindsSortJoinArgs(t *testing.T) { t.Fatalf("expected at least one $N placeholder in countSQL; got:\n%s", sql) } if len(args) < maxIdx { - t.Fatalf("countSQL references $%d but only %d args bound (sortArgs likely dropped); sql:\n%s\nargs: %v", + t.Fatalf("countSQL references $%d but only %d args bound; sql:\n%s\nargs: %v", maxIdx, len(args), sql, args) } } // TestQueryExecutor_PreviewPage_CountSQL_OmitsLimitOffsetOrderBy pins the -// empty-page fallback SQL shape on previewPagePlan. When pagedSQL(true) -// returns an empty page past offset 0, the executor invokes countSQL() to -// recover the real total — COUNT(*) OVER () would otherwise emit no rows -// and leave the caller seeing total=0 even when broader matches exist. +// exact-total SQL shape on previewPagePlan. PreviewPage invokes countSQL() +// separately instead of making the data SELECT calculate COUNT(*) OVER (). // // The countSQL must: // - omit LIMIT/OFFSET (we want the unpaginated total) // - omit ORDER BY (irrelevant for a count, and may reference unbound args) // - wrap the inner FROM/WHERE in `SELECT COUNT(*) FROM (SELECT 1 ...) sub` -// so any GROUP BY in the inner query counts groups (not rows), matching -// what COUNT(*) OVER () would have computed. +// so any GROUP BY in the inner query counts groups (not rows). func TestQueryExecutor_PreviewPage_CountSQL_OmitsLimitOffsetOrderBy(t *testing.T) { exec := &QueryExecutor{Scope: "movie", BaseRelationSQL: "media_items mi"} plan, err := exec.buildPreviewPagePlan(QueryDefinition{}, AccessFilter{}, 20, 0) diff --git a/migrations/141_episode_title_sort_index.down.sql b/migrations/141_episode_title_sort_index.down.sql new file mode 100644 index 00000000..959a7406 --- /dev/null +++ b/migrations/141_episode_title_sort_index.down.sql @@ -0,0 +1 @@ +DROP INDEX IF EXISTS public.idx_episodes_sort_key_content; diff --git a/migrations/141_episode_title_sort_index.up.sql b/migrations/141_episode_title_sort_index.up.sql new file mode 100644 index 00000000..f249b2f4 --- /dev/null +++ b/migrations/141_episode_title_sort_index.up.sql @@ -0,0 +1,7 @@ +-- Episode library title browse. Matches episodeCatalogSelectBody's sort_key +-- expression so PostgreSQL can satisfy ORDER BY title from the episodes index. +CREATE INDEX IF NOT EXISTS idx_episodes_sort_key_content +ON public.episodes USING btree ( + LOWER(COALESCE(NULLIF(BTRIM(title), ''), 'Episode ' || episode_number::text)), + content_id +);