From cbe8f1f5b5eb1911e280f45c53e9f534c42f3096 Mon Sep 17 00:00:00 2001 From: kolaente Date: Fri, 19 Jun 2026 22:39:48 +0200 Subject: [PATCH] fix(tasks): show filter-matched subtasks whose parent is filtered out (#2646) When expand=subtasks was used, phase 1 decided which tasks are "roots" (top-level rows) by excluding any task whose parent lived in the same project: `parent_tasks.project_id != tasks.project_id`. That comparison was only ever a proxy for the real question - "is the parent actually part of this result set?" - and the proxy keeps being wrong: - #1000 cross-project parent - #2494 deleted parent - #2646 same-project parent that is filtered out In the #2646 case a subtask that matches the filter but whose parent does not was wrongly dropped from the roots, so a filter matching only a subtask returned []. Replace the proxy with the real predicate. A task is excluded from roots only when its parent is genuinely in the matched set, i.e. ALL of: - it has a parenttask relation, AND - the parent task exists, AND - the parent is within the queried project scope, AND - the parent satisfies the active filter. The parent-satisfies-the-filter check rebuilds the parsed filter against the parent_tasks alias (convertFiltersToDBFilterCondWithAlias). It applies the filter and project scope but deliberately NOT the text-search predicate, so ParadeDB's search operators are never composed against the parent_tasks alias. Semantics are unchanged: count = roots. The LIMIT still slices roots in phase 1; subtasks ride along in phase 2 (recursive CTE, deduped via NOT IN) and are not counted toward the per-page limit. totalCount stays the number of roots and uses the same corrected condition. Verified on SQLite and a real ParadeDB instance (no "unsupported query shape"). Fixes #2646 --- pkg/models/task_collection_test.go | 185 +++++++++++++++++++++++++++++ pkg/models/task_search.go | 117 +++++++++++++++--- 2 files changed, 283 insertions(+), 19 deletions(-) diff --git a/pkg/models/task_collection_test.go b/pkg/models/task_collection_test.go index e66d945b2..1a7294b3a 100644 --- a/pkg/models/task_collection_test.go +++ b/pkg/models/task_collection_test.go @@ -1972,3 +1972,188 @@ func TestTaskCollection_SubtaskWithMultipleParentsNoDuplicates(t *testing.T) { assert.True(t, foundParent1, "Parent task 41 should be present") assert.True(t, foundParent2, "Parent task 42 should be present") } + +// TestTaskCollection_ExpandSubtasksPaginatesRoots verifies the maintainer's exact +// pagination scenario: with expand=subtasks the LIMIT must slice ROOTS (top-level +// tasks), and subtasks ride along beyond the limit without consuming it. totalCount +// equals the number of roots, not the flat task count. Guards #2345. +func TestTaskCollection_ExpandSubtasksPaginatesRoots(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + u := &user.User{ID: 1} + + project := &Project{Title: "pagination-roots", OwnerID: u.ID} + _, err := s.Insert(project) + require.NoError(t, err) + + // 40 top-level tasks + topLevel := make([]*Task, 0, 40) + for i := 1; i <= 40; i++ { + task := &Task{ + Title: "root", + ProjectID: project.ID, + CreatedByID: u.ID, + Index: int64(i), + } + _, err = s.Insert(task) + require.NoError(t, err) + topLevel = append(topLevel, task) + } + + // 10 subtasks, each scattered as a child of some top-level task + parentIdx := []int{0, 4, 9, 12, 18, 22, 27, 31, 35, 39} + for i, idx := range parentIdx { + sub := &Task{ + Title: "sub", + ProjectID: project.ID, + CreatedByID: u.ID, + Index: int64(41 + i), + } + _, err = s.Insert(sub) + require.NoError(t, err) + + rel := &TaskRelation{ + TaskID: topLevel[idx].ID, + OtherTaskID: sub.ID, + RelationKind: RelationKindSubtask, + } + require.NoError(t, rel.Create(s, u)) + } + require.NoError(t, s.Commit()) + + s2 := db.NewSession() + defer s2.Close() + + expand := []TaskCollectionExpandable{TaskCollectionExpandSubtasks} + + // Page 1: 25 roots + their subtasks; totalCount = 40 roots + page1, _, total1, err := getRawTasksForProjects(s2, []*Project{project}, u, &taskSearchOptions{ + expand: expand, + page: 1, + perPage: 25, + }) + require.NoError(t, err) + assert.Equal(t, int64(40), total1, "totalCount must count roots only (40), not the flat 50") + + roots1, subs1 := countRootsAndSubs(page1) + assert.Equal(t, 25, roots1, "page 1 must contain exactly 25 root tasks") + assert.GreaterOrEqual(t, subs1, 1, "page 1 must also carry the subtasks of its roots") + + // Page 2: 15 roots (26-40) + their subtasks + page2, _, total2, err := getRawTasksForProjects(s2, []*Project{project}, u, &taskSearchOptions{ + expand: expand, + page: 2, + perPage: 25, + }) + require.NoError(t, err) + assert.Equal(t, int64(40), total2) + + roots2, _ := countRootsAndSubs(page2) + assert.Equal(t, 15, roots2, "page 2 must contain the remaining 15 root tasks") +} + +// countRootsAndSubs splits the pagination result set into top-level tasks and +// subtasks using the titles assigned by the test ("root" vs "sub"). +func countRootsAndSubs(tasks []*Task) (roots, subs int) { + for _, tsk := range tasks { + if tsk.Title == "sub" { + subs++ + } else { + roots++ + } + } + return roots, subs +} + +// TestTaskCollection_ExpandSubtasksFilterMatchesSubtaskOnly covers #2646 case A: +// a filter that matches only a subtask (whose same-project parent is filtered out) +// must return that subtask as a root. The old same-project proxy returned []. +func TestTaskCollection_ExpandSubtasksFilterMatchesSubtaskOnly(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + u := &user.User{ID: 1} + + project := &Project{Title: "filter-matches-subtask", OwnerID: u.ID} + _, err := s.Insert(project) + require.NoError(t, err) + + parent := &Task{Title: "parent", ProjectID: project.ID, CreatedByID: u.ID, Index: 1, Priority: 1} + _, err = s.Insert(parent) + require.NoError(t, err) + + sub := &Task{Title: "matching subtask", ProjectID: project.ID, CreatedByID: u.ID, Index: 2, Priority: 5} + _, err = s.Insert(sub) + require.NoError(t, err) + + rel := &TaskRelation{TaskID: parent.ID, OtherTaskID: sub.ID, RelationKind: RelationKindSubtask} + require.NoError(t, rel.Create(s, u)) + require.NoError(t, s.Commit()) + + s2 := db.NewSession() + defer s2.Close() + + filters, err := getTaskFiltersFromFilterString("priority = 5", "") + require.NoError(t, err) + + tasks, _, _, err := getRawTasksForProjects(s2, []*Project{project}, u, &taskSearchOptions{ + expand: []TaskCollectionExpandable{TaskCollectionExpandSubtasks}, + parsedFilters: filters, + }) + require.NoError(t, err) + + require.Len(t, tasks, 1, "the matching subtask must be returned even though its parent is filtered out") + assert.Equal(t, sub.ID, tasks[0].ID) +} + +// TestTaskCollection_ExpandSubtasksFilterMatchesParentOnly covers #2646 case B: +// a filter matching only the parent returns the parent plus its (non-matching) +// subtask, nested, with no duplication. +func TestTaskCollection_ExpandSubtasksFilterMatchesParentOnly(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + u := &user.User{ID: 1} + + project := &Project{Title: "filter-matches-parent", OwnerID: u.ID} + _, err := s.Insert(project) + require.NoError(t, err) + + parent := &Task{Title: "matching parent", ProjectID: project.ID, CreatedByID: u.ID, Index: 1, Priority: 5} + _, err = s.Insert(parent) + require.NoError(t, err) + + sub := &Task{Title: "subtask", ProjectID: project.ID, CreatedByID: u.ID, Index: 2, Priority: 1} + _, err = s.Insert(sub) + require.NoError(t, err) + + rel := &TaskRelation{TaskID: parent.ID, OtherTaskID: sub.ID, RelationKind: RelationKindSubtask} + require.NoError(t, rel.Create(s, u)) + require.NoError(t, s.Commit()) + + s2 := db.NewSession() + defer s2.Close() + + filters, err := getTaskFiltersFromFilterString("priority = 5", "") + require.NoError(t, err) + + tasks, _, total, err := getRawTasksForProjects(s2, []*Project{project}, u, &taskSearchOptions{ + expand: []TaskCollectionExpandable{TaskCollectionExpandSubtasks}, + parsedFilters: filters, + }) + require.NoError(t, err) + + assert.Equal(t, int64(1), total, "only the parent is a root, so the count is 1") + + ids := map[int64]int{} + for _, tsk := range tasks { + ids[tsk.ID]++ + } + assert.Equal(t, 1, ids[parent.ID], "parent present exactly once") + assert.Equal(t, 1, ids[sub.ID], "subtask rides along exactly once, no duplication") + assert.Len(t, tasks, 2) +} diff --git a/pkg/models/task_search.go b/pkg/models/task_search.go index 07da3f809..a00a2fdcf 100644 --- a/pkg/models/task_search.go +++ b/pkg/models/task_search.go @@ -103,11 +103,16 @@ type dbTaskSearcher struct { hasFavoritesProject bool } -func (sf *SubTableFilter) ToBaseSubQuery() *builder.Builder { +func (sf *SubTableFilter) ToBaseSubQuery(taskAlias string) *builder.Builder { + baseFilter := sf.BaseFilter + if taskAlias != "tasks" { + baseFilter = strings.ReplaceAll(baseFilter, "tasks.", taskAlias+".") + } + var cond = builder. Select("1"). From(sf.Table). - Where(builder.Expr(sf.BaseFilter)) + Where(builder.Expr(baseFilter)) // little hack to add users table for assignees filter if sf.Table == "task_assignees" { @@ -161,6 +166,13 @@ func getOrderByDBStatement(opts *taskSearchOptions) (orderby string, err error) } func convertFiltersToDBFilterCond(rawFilters []*taskFilter, includeNulls bool) (filterCond builder.Cond, err error) { + return convertFiltersToDBFilterCondWithAlias(rawFilters, includeNulls, "tasks") +} + +// convertFiltersToDBFilterCondWithAlias builds the filter condition against the +// given task table alias. Passing "parent_tasks" lets the subtask-expansion root +// condition ask "does the parent satisfy the filter" (see #2646). +func convertFiltersToDBFilterCondWithAlias(rawFilters []*taskFilter, includeNulls bool, taskAlias string) (filterCond builder.Cond, err error) { var dbFilters = make([]builder.Cond, 0, len(rawFilters)) // Track join types separately because after merging consecutive sub-table @@ -171,7 +183,7 @@ func convertFiltersToDBFilterCond(rawFilters []*taskFilter, includeNulls bool) ( f := rawFilters[i] if nested, is := f.value.([]*taskFilter); is { - nestedDBFilters, err := convertFiltersToDBFilterCond(nested, includeNulls) + nestedDBFilters, err := convertFiltersToDBFilterCondWithAlias(nested, includeNulls, taskAlias) if err != nil { return nil, err } @@ -234,7 +246,7 @@ func convertFiltersToDBFilterCond(rawFilters []*taskFilter, includeNulls bool) ( } } - filterSubQuery := subTableFilterParams.ToBaseSubQuery().And(combinedInnerCond) + filterSubQuery := subTableFilterParams.ToBaseSubQuery(taskAlias).And(combinedInnerCond) var filter builder.Cond if f.comparator == taskFilterComparatorNotEquals || f.comparator == taskFilterComparatorNotIn { @@ -244,7 +256,7 @@ func convertFiltersToDBFilterCond(rawFilters []*taskFilter, includeNulls bool) ( } if includeNulls && subTableFilterParams.AllowNullCheck { - filter = builder.Or(filter, builder.NotExists(subTableFilterParams.ToBaseSubQuery())) + filter = builder.Or(filter, builder.NotExists(subTableFilterParams.ToBaseSubQuery(taskAlias))) } dbFilters = append(dbFilters, filter) @@ -258,7 +270,7 @@ func convertFiltersToDBFilterCond(rawFilters []*taskFilter, includeNulls bool) ( if f.field == taskPropertyBucketID { f.field = "task_buckets.`bucket_id`" } else { - f.field = "tasks.`" + f.field + "`" + f.field = taskAlias + ".`" + f.field + "`" } filter, err := getFilterCond(f, includeNulls) if err != nil { @@ -303,6 +315,66 @@ func hasBucketIDInParsedFilter(filters []*taskFilter) bool { return false } +// cloneTaskFilters deep-copies the parsed filters so the parent-scoped filter +// build does not mutate the shared field names (convertFiltersToDBFilterCond +// rewrites f.field in place, which must not leak back into the main query). +func cloneTaskFilters(filters []*taskFilter) []*taskFilter { + cloned := make([]*taskFilter, len(filters)) + for i, f := range filters { + c := *f + if nested, is := f.value.([]*taskFilter); is { + c.value = cloneTaskFilters(nested) + } + cloned[i] = &c + } + return cloned +} + +// buildSubtaskRootCondition decides which tasks count as "roots" when expanding +// subtasks: a task is a root unless its parent is itself part of this result set. +// +// The previous implementation used parent_tasks.project_id != tasks.project_id as +// a proxy for "parent is not in this list". That proxy is the recurring source of +// root-placement bugs - it was wrong for cross-project parents (#1000), deleted +// parents (#2494) and now for same-project parents that are filtered out (#2646). +// +// A task is excluded from roots only when ALL of the following hold: +// - it has a parenttask relation, AND +// - the parent task exists, AND +// - the parent is within the queried project scope, AND +// - the parent satisfies the active filter. +// +// Note the filter (and project scope) is applied here, but not the text-search +// predicate: search uses ParadeDB operators that don't compose against the +// parent_tasks alias, and #2646 is purely about filters. +func buildSubtaskRootCondition(opts *taskSearchOptions) (builder.Cond, error) { + parentInScope := builder.Cond(builder.Expr("1 = 1")) + if len(opts.projectIDs) > 0 { + parentInScope = builder.In("parent_tasks.project_id", opts.projectIDs) + } + + parentMatchesFilter := builder.Cond(builder.Expr("1 = 1")) + if len(opts.parsedFilters) > 0 { + parentFilters := cloneTaskFilters(opts.parsedFilters) + filterCond, err := convertFiltersToDBFilterCondWithAlias(parentFilters, opts.filterIncludeNulls, "parent_tasks") + if err != nil { + return nil, err + } + if filterCond != nil { + parentMatchesFilter = filterCond + } + } + + parentIsRoot := builder.And( + builder.NotNull{"task_relations.id"}, + builder.NotNull{"parent_tasks.id"}, + parentInScope, + parentMatchesFilter, + ) + + return builder.Not{parentIsRoot}, nil +} + //nolint:gocyclo func (d *dbTaskSearcher) Search(opts *taskSearchOptions) (tasks []*Task, totalCount int64, err error) { @@ -313,6 +385,25 @@ func (d *dbTaskSearcher) Search(opts *taskSearchOptions) (tasks []*Task, totalCo joinTaskBuckets := hasBucketIDInParsedFilter(opts.parsedFilters) + var expandSubtasks = false + for _, expandable := range opts.expand { + if expandable == TaskCollectionExpandSubtasks { + expandSubtasks = true + break + } + } + + // The root condition asks whether a task's parent is part of this result set, + // which means re-building the filter against the parent_tasks alias. Compute it + // before convertFiltersToDBFilterCond mutates the shared filter field names. + var subtaskRootCond builder.Cond + if expandSubtasks { + subtaskRootCond, err = buildSubtaskRootCondition(opts) + if err != nil { + return nil, 0, err + } + } + filterCond, err := convertFiltersToDBFilterCond(opts.parsedFilters, opts.filterIncludeNulls) if err != nil { return nil, 0, err @@ -358,20 +449,8 @@ func (d *dbTaskSearcher) Search(opts *taskSearchOptions) (tasks []*Task, totalCo distinct += ", task_positions.position" } - var expandSubtasks = false - for _, expandable := range opts.expand { - if expandable == TaskCollectionExpandSubtasks { - expandSubtasks = true - break - } - } - if expandSubtasks { - cond = builder.And(cond, builder.Or( - builder.IsNull{"task_relations.id"}, - builder.IsNull{"parent_tasks.id"}, - builder.Expr("parent_tasks.project_id != tasks.project_id"), - )) + cond = builder.And(cond, subtaskRootCond) } query := d.s.