diff --git a/cmd/server/tests/filter_builder_test.go b/cmd/server/tests/filter_builder_test.go index 002d5c34..3e6f7c4d 100644 --- a/cmd/server/tests/filter_builder_test.go +++ b/cmd/server/tests/filter_builder_test.go @@ -295,6 +295,115 @@ func TestFilterComposedSemantics(t *testing.T) { assert.Equal(t, []string{"Sem Tide"}, got, "published_year at_least") } +// TestFilterScopesAndEpisodeFields pins item 3.1: the books scope +// excludes podcasts (the leak the composed head used to have), the +// episodes scope is episodes only, episode fields evaluate in SQL +// (feed-in, downloaded, played, published-date-within), and episode +// fields are 400s in books trees. +func TestFilterScopesAndEpisodeFields(t *testing.T) { + setup := setupTestServer(t) + t.Cleanup(func() { _ = setup.Close() }) + + regUUID := getRegularUserID(t, setup.DB) + builderFixtureLib(t, setup, regUUID, "Scope") + + // Podcasts library + a subscribed feed with three downloaded + // episodes: two fresh/unplayed, one played (~finished) — the + // played-rule fixture. + feedLib, _ := createPodcastsLibrary(t, setup, "test Scope Podcasts") + feed, err := setup.DB.CreatePodcastFeed(adminSeedCtx(t, setup), database.CreatePodcastFeedParams{ + LibraryID: feedLib.ID, + FeedUrl: "https://feed.invalid/scope-probe.xml", + Title: "Scope Probe Feed", + }) + require.NoError(t, err) + epFresh := upsertTestEpisode(t, setup, feed, "scope-fresh", "Scope Fresh Episode", 1) + epPlayed := upsertTestEpisode(t, setup, feed, "scope-played", "Scope Played Episode", 2) + upsertTestEpisode(t, setup, feed, "scope-pending", "Scope Pending Episode", 3) + subscribeRaw(t, setup, regUUID, feed.ID) + adminScopeExec(t, setup, adminCtxValue(t, setup), ` + UPDATE media_items SET download_state = 'downloaded', file_size = 1000, + date_published = CURRENT_DATE WHERE id IN ($1, $2)`, epFresh.ID, epPlayed.ID) + adminScopeExec(t, setup, adminCtxValue(t, setup), ` + INSERT INTO reading_progress (media_item_id, user_id, percentage) + VALUES ($1, $2, 1.0) ON CONFLICT (media_item_id, user_id) + DO UPDATE SET percentage = 1.0`, epPlayed.ID, regUUID) + + feedID := uuid.UUID(feed.ID.Bytes).String() + + qfScoped := func(scope string, children ...map[string]interface{}) url.Values { + b, _ := json.Marshal(map[string]interface{}{"scope": scope, "mode": "all", "children": children}) + return url.Values{"qf": {string(b)}} + } + const booksScope = "books" + const episodesScope = "episodes" + + search := func(p url.Values) []string { + rows := p + status, body := qfSearch(t, setup, setup.RegularToken, rows) + // 404 is the endpoint's pinned no-results contract — an empty set. + require.Contains(t, []int{http.StatusOK, http.StatusNotFound}, status, "qf must parse: %v", rows) + return qfTitles(t, body) + } + + t.Run("books_scope_excludes_podcasts", func(t *testing.T) { + // An exact-ish title hits BOTH a book fixture ("Scope Tide") and + // the episode fixture if the head leaks; books scope must return + // the book, never the episodes. + got := search(qfScoped(booksScope, qfRule("title", "is", "Scope Tide"))) + assert.Equal(t, []string{"Scope Tide"}, got, "books scope returns the book") + got = search(qfScoped(booksScope, qfRule("title", "is", "Scope Fresh Episode"))) + assert.Empty(t, got, + "books scope must never return podcast episodes (the composed-head leak is CLOSED)") + }) + + t.Run("episodes_scope_is_episodes_only", func(t *testing.T) { + got := search(qfScoped(episodesScope, qfRule("title", "is", "Scope Fresh Episode"))) + assert.Equal(t, []string{"Scope Fresh Episode"}, got, + "episodes scope: only episodes — books drop") + }) + + t.Run("episode_rules_evaluate_in_sql", func(t *testing.T) { + got := search(qfScoped(episodesScope, + qfRule("feed", "in", feedID))) + assert.ElementsMatch(t, []string{"Scope Fresh Episode", "Scope Played Episode", "Scope Pending Episode"}, + got, "feed-in matches every episode of the feed (download state is a separate rule)") + + got = search(qfScoped(episodesScope, + qfRule("downloaded", "is_true"))) + assert.ElementsMatch(t, []string{"Scope Fresh Episode", "Scope Played Episode"}, + got, "downloaded:true drops the pending episode") + + got = search(qfScoped(episodesScope, + qfRule("played", "is_false"), + qfRule("downloaded", "is_true"))) + assert.ElementsMatch(t, []string{"Scope Fresh Episode"}, got, + "played:false + downloaded = the unplayed playable episode (viewer progress)") + + got = search(qfScoped(episodesScope, + qfRule("played", "is_true"))) + assert.ElementsMatch(t, []string{"Scope Played Episode"}, got, + "played:true matches the finished episode") + + got = search(qfScoped(episodesScope, + qfRule("published_date", "within_days", "7"), + qfRule("downloaded", "is_true"))) + assert.ElementsMatch(t, []string{"Scope Fresh Episode", "Scope Played Episode"}, + got, "published-within includes the dated episodes") + }) + + t.Run("episode_fields_are_400_in_books_scope", func(t *testing.T) { + p := qfScoped(booksScope, qfRule("played", "is_true")) + req, _ := http.NewRequest("GET", setup.Server.URL+"/api/media-items/search?"+p.Encode(), nil) + req.Header.Set("Authorization", "Bearer "+setup.RegularToken) + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + defer func() { _ = resp.Body.Close() }() + assert.Equal(t, http.StatusBadRequest, resp.StatusCode, + "episode-only fields are rejected in books trees at parse time") + }) +} + func TestFilterUserRelativeFields(t *testing.T) { setup := setupTestServer(t) t.Cleanup(func() { _ = setup.Close() }) diff --git a/internal/services/filter_query.go b/internal/services/filter_query.go index f5751b31..fa7229d2 100644 --- a/internal/services/filter_query.go +++ b/internal/services/filter_query.go @@ -75,11 +75,20 @@ JOIN libraries l ON mi.library_id = l.id JOIN library_types lt ON l.library_type_id = lt.id LEFT JOIN library_visibility lv ON l.id = lv.library_id AND lv.user_id = ` -// headWhere renders the mandatory WHERE head: the family lens + -// integrity facts, plus the optional library scope (validated upstream — -// an explicit out-of-scope library never reaches the SQL). -func (b *filterBuilder) headWhere(libraryID pgtype.UUID) string { +// headWhere renders the mandatory WHERE head: the SCOPE (books excludes +// podcasts; episodes is episodes only — plan item 3.1 closes the leak +// where the composed head happily returned episodes on All-Books +// queries), the family lens, integrity facts, plus the optional library +// scope (validated upstream — an explicit out-of-scope library never +// reaches the SQL). +func (b *filterBuilder) headWhere(libraryID pgtype.UUID, scope string) string { w := "COALESCE(lv.is_visible, true) = true AND mi.archived_at IS NULL AND mi.missing_scan_count = 0" + switch scope { + case ScopeEpisodes: + w += " AND mi.source = 'podcast_episode'" + default: // books — the default legacy scope + w += " AND mi.library_type_name <> 'podcasts'" + } if libraryID.Valid { w += " AND mi.library_id = " + b.bind(libraryID) } @@ -637,6 +646,50 @@ func clauseLibrary(r FilterRule, b *filterBuilder) string { // ─── tree → SQL ────────────────────────────────────────────────────── +// clauseFeed — the episode's source feed (episodes scope, item 3.1). +// Values are podcast-feed UUIDs; visibility rides RLS (podcast_feeds is +// governed by the same subscription predicate as episodes, so a rule +// naming an unsubscribed feed matches nothing). +func clauseFeed(r FilterRule, b *filterBuilder) string { + b.needJoin("pf", "LEFT JOIN podcast_feeds pf ON pf.id = mi.source_feed_id") + ids := make([]uuid.UUID, 0, len(r.Values)) + for _, v := range r.Values { + if id, err := uuid.Parse(v); err == nil { + ids = append(ids, id) + } + } + in := "(pf.id = ANY(" + b.bind(ids) + "::uuid[]))" + switch r.Op { + case OpIs, OpIn: + return in + default: + return "(NOT " + in + ")" + } +} + +// clauseDownloaded — the episode's download state (episodes scope). +// is_true = the playable file is on disk; is_false = anything else. +func clauseDownloaded(r FilterRule, b *filterBuilder) string { + if r.Op == OpIsTrue { + return "(mi.download_state = 'downloaded')" + } + return "(mi.download_state <> 'downloaded')" +} + +// clausePlayed — THE episode finished predicate (episodes scope): the +// viewer's progress >= 0.999, the same definition as hide_finished and +// the item-2.4 rules engine. Ties to the viewer via the shared +// reading_progress join (R2/D5 lineage: progress is per-user). +func clausePlayed(r FilterRule, b *filterBuilder) string { + b.readingProgress() + if r.Op == OpIsTrue { + return "(COALESCE(rp.percentage, 0) >= 0.999)" + } + return "(COALESCE(rp.percentage, 0) < 0.999)" +} + +// ─── tree → SQL (end) ───────────────────────────────────────────────── + // buildGroupClause renders a group: children joined by the group mode, // each child a rule clause or a nested group. A group ANDs into its // parent. @@ -742,14 +795,13 @@ func (s *SearchService) ComposedSearch(ctx context.Context, params SearchParams) // Build the WHERE first — clause construction requests the // user-relative JOINs lazily, and joinBlock() must render AFTER // they are collected. - whereSQL := b.headWhere(params.LibraryID) + treeWhere(params.FilterTree, b) + ` + whereSQL := b.headWhere(params.LibraryID, params.FilterTree.FilterScope()) + treeWhere(params.FilterTree, b) + ` AND (` + libBind + `::uuid IS NULL OR mi.library_id = ` + libBind + `::uuid) AND (` + qBind + ` = '' OR ` + composedQueryBlock(qBind, exactBind) + `)` sqlStr := composedSelect + filterFrom + b.userIDBind + b.joinBlock() + ` WHERE ` + whereSQL + composedSortOrder(sortBind, qBind, exactBind) + ` ` + limitBind + ` OFFSET ` + offsetBind - fmt.Printf("[DEBUG COMPOSED] %s\n", sqlStr) rows, err := s.raw.Query(ctx, sqlStr, b.args...) if err != nil { @@ -826,7 +878,7 @@ func composedQueryBlock(qBind, exactBind string) string { func (s *SearchService) ComposedCount(ctx context.Context, userID, libraryID pgtype.UUID, tree *FilterTree) (int, error) { b := newFilterBuilder(userID) libBind := b.bind(libraryID) - whereSQL := b.headWhere(libraryID) + treeWhere(tree, b) + ` + whereSQL := b.headWhere(libraryID, tree.FilterScope()) + treeWhere(tree, b) + ` AND (` + libBind + `::uuid IS NULL OR mi.library_id = ` + libBind + `::uuid)` sqlStr := `SELECT COUNT(*) ` + filterFrom + b.userIDBind + b.joinBlock() + ` @@ -921,6 +973,14 @@ func (s *SearchService) ComposedFieldValues(ctx context.Context, p ComposedSugge return s.composedIDSourceValues(ctx, b, p, "users u JOIN media_items mi ON mi.added_by_admin_id = u.id", "u.id::text", "u.username", "", limitBind, libBind) + case suggestFeed: + // The viewer's feed universe — RLS (podcast_feeds subscription + // predicate + admin escape) scopes both suggestions and matches. + return s.composedIDSourceValues(ctx, b, p, + "podcast_feeds pf", + "pf.id::text", "pf.title", + "JOIN media_items mi ON mi.source_feed_id = pf.id", + limitBind, libBind) default: // Column / array sources: the value expr mirrors the field's // match column (tags via the *_search column, like the flat @@ -956,7 +1016,7 @@ func (s *SearchService) ComposedFieldValues(ctx context.Context, p ComposedSugge scoreExpr = fmt.Sprintf("CASE WHEN %s = '' THEN 0 ELSE word_similarity(%s, src.value) END AS score", tb, tb) } - whereSQL := b.headWhere(p.LibraryID) + ` + whereSQL := b.headWhere(p.LibraryID, p.Tree.FilterScope()) + ` AND (` + libBind + `::uuid IS NULL OR mi.library_id = ` + libBind + `::uuid)` + treeWhere(p.Tree, b) + ` AND src.value IS NOT NULL AND src.value <> ''` + typedFilter diff --git a/internal/services/filter_registry.go b/internal/services/filter_registry.go index 41625d08..6ba6ba7d 100644 --- a/internal/services/filter_registry.go +++ b/internal/services/filter_registry.go @@ -87,6 +87,7 @@ const ( suggestHighlight SuggestSource = "highlight_text" suggestLibrary SuggestSource = "library" // visibility-scoped libraries suggestCollection SuggestSource = "collection" // the viewer's collections + suggestFeed SuggestSource = "feed" // the viewer's subscribed feeds (RLS-governed) suggestAddedBy SuggestSource = "added_by" // admins who added books in scope ) @@ -99,8 +100,27 @@ const ( GroupComics = "Comics & manga" GroupMyReading = "My reading" GroupLibrary = "Library" + // Episodes (plan item 3.1, filter-system unification): fields that + // only make sense on podcast-episode rows. Hidden in the books + // scope ("hide podcasts from the rules on the all books page, but + // allow them in collections"). + GroupEpisodes = "Episodes" ) +// Filter scopes (plan item 3.1): which row universe a tree matches. +// The top-level `scope` key on the qf tree declares it; absent = books +// (every existing saved tree and every legacy client stays valid). +const ( + ScopeBooks = "books" + ScopeEpisodes = "episodes" +) + +// episodeOnlyFields are meaningful ONLY in the episodes scope — a +// books-scope tree using one is a user error (400 at parse). +var episodeOnlyFields = map[string]bool{ + "feed": true, "downloaded": true, "played": true, +} + // FilterRule is one leaf: field + operator + values (all bound params). type FilterRule struct { Field string @@ -112,10 +132,21 @@ type FilterRule struct { // children are rules or nested groups. Groups AND/OR their children // together; a group ANDs into its parent. type FilterTree struct { + Scope string `json:"scope,omitempty"` Mode string `json:"mode"` Children []FilterChild `json:"children"` } +// FilterScope resolves the tree's row universe: an absent scope key is +// the books scope (legacy trees, saved filters, every pre-unification +// client). nil-safe (suggest paths can carry a nil tree). +func (t *FilterTree) FilterScope() string { + if t != nil && t.Scope == ScopeEpisodes { + return ScopeEpisodes + } + return ScopeBooks +} + // FilterChild is one node: a rule (Field set) or a group (Mode set). type FilterChild struct { Field string `json:"field,omitempty"` @@ -135,6 +166,24 @@ type FilterField struct { DefaultOp string // operator a new rule starts on (field-switch reset) Clause func(r FilterRule, b *filterBuilder) string Suggest SuggestSource + // Scopes limits the field to the listed scopes (nil = available in + // every scope — most book fields no-op against episode rows since + // episode rows carry NULLs there). Episode-only fields declare + // exactly [ScopeEpisodes]; the books scope rejects them at parse. + Scopes []string +} + +// InScope reports whether the field may appear in a scope tree. +func (f FilterField) InScope(scope string) bool { + if len(f.Scopes) == 0 { + return true + } + for _, s := range f.Scopes { + if s == scope { + return true + } + } + return false } // opsWithValue / opsWithoutValue drive validation. @@ -176,6 +225,9 @@ var filterFields = []FilterField{ {Key: "published_year", Label: "Published year", Group: GroupPublish, Kind: KindNumber, Ops: []string{OpBetween, OpAtLeast, OpAtMost, OpEmpty}, DefaultOp: OpAtLeast, Clause: clausePublishedYear, Suggest: suggestNone}, + {Key: "published_date", Label: "Published date", Group: GroupPublish, Kind: KindDate, + Ops: []string{OpWithinDays, OpBetween, OpEmpty}, DefaultOp: OpWithinDays, + Clause: clauseDateFactory("mi.date_published"), Suggest: suggestNone}, // ── Topics ──────────────────────────────────────────────────── {Key: "genre", Label: "Genre", Group: GroupTopics, Kind: KindScalarFuzzy, @@ -349,6 +401,23 @@ var filterFields = []FilterField{ Ops: []string{OpIn, OpNotIn, OpInAny, OpInNone}, DefaultOp: OpIn, Clause: clauseCollection, Suggest: suggestCollection}, + // ── Episodes (podcast-episode rows; episodes scope only) ────── + // Plan item 3.1 / §0 item 3: the filter engine gains the episode + // fields so smart episode collections can ride the SAME engine. + // `feed` suggests the viewer's subscribed feeds (RLS governs both + // suggest and match); `downloaded` reads the download state; + // `played` is THE finished predicate (viewer progress >= 0.999 — + // the same definition as hide_finished, item 2.4). + {Key: "feed", Label: "Feed", Group: GroupEpisodes, Kind: KindIdentifier, + Ops: []string{OpIs, OpIsNot, OpIn, OpNotIn, OpHas, OpMissing}, DefaultOp: OpIn, + Clause: clauseFeed, Suggest: suggestFeed, Scopes: []string{ScopeEpisodes}}, + {Key: "downloaded", Label: "Downloaded", Group: GroupEpisodes, Kind: KindBool, + Ops: []string{OpIsTrue, OpIsFalse}, DefaultOp: OpIsTrue, + Clause: clauseDownloaded, Suggest: suggestNone, Scopes: []string{ScopeEpisodes}}, + {Key: "played", Label: "Played", Group: GroupEpisodes, Kind: KindBool, + Ops: []string{OpIsTrue, OpIsFalse}, DefaultOp: OpIsFalse, + Clause: clausePlayed, Suggest: suggestNone, Scopes: []string{ScopeEpisodes}}, + // ── Library (holding structure + family lens) ───────────────── {Key: "library", Label: "Library", Group: GroupLibrary, Kind: KindLibrary, // Values are library IDs; the menu only ever offers the viewer's @@ -426,13 +495,16 @@ func ParseFilterTree(raw string) (*FilterTree, error) { if err := json.Unmarshal([]byte(raw), &t); err != nil { return nil, fmt.Errorf("invalid qf JSON: %w", err) } - if err := validateGroup(&FilterChild{Mode: t.Mode, Children: t.Children}, 1); err != nil { + if t.Scope != "" && t.Scope != ScopeBooks && t.Scope != ScopeEpisodes { + return nil, fmt.Errorf("filter scope must be books|episodes, got %q", t.Scope) + } + if err := validateGroup(&FilterChild{Mode: t.Mode, Children: t.Children}, 1, t.FilterScope()); err != nil { return nil, err } return &t, nil } -func validateGroup(g *FilterChild, depth int) error { +func validateGroup(g *FilterChild, depth int, scope string) error { if depth > maxFilterDepth { return fmt.Errorf("filter group nesting deeper than %d", maxFilterDepth) } @@ -444,18 +516,19 @@ func validateGroup(g *FilterChild, depth int) error { } for _, c := range g.Children { if c.Field == "" && c.Mode != "" { - if err := validateGroup(&c, depth+1); err != nil { + if err := validateGroup(&c, depth+1, scope); err != nil { return err } continue } - if err := validateRule(c); err != nil { + if err := validateRule(c, scope); err != nil { return err } } return nil } + // numericParsers map each numeric field's wire values to the Go value // bound into SQL — validation and clause-building share one parser per // field, so a value that passes validation always binds. @@ -475,11 +548,18 @@ var numericParsers = map[string]func(string) (any, error){ "pages_read": parseNumberFloat, } -func validateRule(c FilterChild) error { +func validateRule(c FilterChild, scope string) error { f, ok := FilterFieldByKey(c.Field) if !ok { return fmt.Errorf("unknown filter field %q", c.Field) } + // Episode-only fields are a user error in a books tree (plan item + // 3.1: "hide podcasts from the rules on the all books page"). The + // reverse is NOT an error: book fields that no-op against episode + // rows (NULLs) are allowed in episodes trees. + if scope == ScopeBooks && episodeOnlyFields[c.Field] { + return fmt.Errorf("field %q only exists in the episodes scope", c.Field) + } if !fieldOpsAllowed(f, c.Op) { return fmt.Errorf("operator %q not allowed on field %q", c.Op, c.Field) } diff --git a/internal/services/filter_registry_test.go b/internal/services/filter_registry_test.go index 3a184f00..f14179d7 100644 --- a/internal/services/filter_registry_test.go +++ b/internal/services/filter_registry_test.go @@ -55,6 +55,15 @@ func TestFilterRegistryCompleteness(t *testing.T) { "pages_read", "collection", // holding structure "library", + // episode quantifier (item 3.1) + "published_date", + } + // Episode-scope fields (item 3.1): they declare exactly the + // episodes scope; no other field declares a scope restriction. + for _, key := range []string{"feed", "downloaded", "played"} { + assert.True(t, seen[key], "episode filter field %q missing from the registry", key) + f, _ := FilterFieldByKey(key) + assert.Equal(t, []string{ScopeEpisodes}, f.Scopes, "%q must be episodes-scope only", key) } for _, key := range mandated { assert.True(t, seen[key], "mandated filter field %q missing from the registry", key) @@ -102,6 +111,39 @@ func TestFilterParseValidation(t *testing.T) { invalid(`{"mode":"all","children":[{"field":"author","op":"contains"}]}`, "needs a value") } +func TestFilterScopeValidation(t *testing.T) { + valid := func(raw string) { + _, err := ParseFilterTree(raw) + require.NoError(t, err, "expected %s to parse", raw) + } + invalid := func(raw, wantContain string) { + _, err := ParseFilterTree(raw) + require.Error(t, err, "expected %s to be rejected", raw) + require.Contains(t, err.Error(), wantContain) + } + + // Legacy trees (no scope key) stay valid — the default is books. + valid(`{"mode":"all","children":[{"field":"author","op":"contains","values":["Voss"]}]}`) + + // Books trees REJECT episode-only fields (the item-3 hide rule). + invalid(`{"scope":"books","mode":"all","children":[{"field":"played","op":"is_true"}]}`, + "only exists in the episodes scope") + invalid(`{"mode":"all","children":[{"field":"downloaded","op":"is_true"}]}`, + "only exists in the episodes scope") + + // Episodes trees carry the episode fields. + valid(`{"scope":"episodes","mode":"all","children":[ + {"field":"feed","op":"in","values":["12345678-1234-1234-1234-123456789012"]}, + {"field":"downloaded","op":"is_true"}, + {"field":"played","op":"is_false"}, + {"field":"published_date","op":"within_days","values":["7"]}, + {"field":"title","op":"contains","values":["news"]}]}`) + + // Unknown scope values are rejected. + invalid(`{"scope":"manga","mode":"all","children":[{"field":"title","op":"contains","values":["x"]}]}`, + "scope must be books|episodes") +} + func TestFilterParseDepthCap(t *testing.T) { // Nesting beyond maxFilterDepth is rejected. raw := `{"mode":"all","children":[`