mirror of
https://github.com/go-vikunja/vikunja.git
synced 2026-03-11 17:48:44 -05:00
fix: only merge range comparators in sub-table filter grouping
Restrict the AND-joined sub-table filter merging to range comparators (>, >=, <, <=) only. Equality and negative comparators (=, !=, in, not in) must remain as separate EXISTS/NOT EXISTS subqueries because each matching value lives in its own row. Merging equality filters like `labels = 4 && labels = 5` into a single EXISTS would produce an unsatisfiable condition (no single row has label_id=4 AND label_id=5). Merging negative filters like `labels != 4 && labels != 5` into NOT EXISTS(label_id IN 4 AND label_id IN 5) would be trivially true. Also fix the join tracking to use the first filter's join type (how the group connects to the previous element) instead of the last.
This commit is contained in:
@@ -1481,6 +1481,63 @@ func TestTaskCollection_ReadAll(t *testing.T) {
|
||||
},
|
||||
wantErr: false,
|
||||
},
|
||||
{
|
||||
// Regression: AND-joined equality filters on the same sub-table must
|
||||
// NOT be merged into a single EXISTS (each label lives on a separate row).
|
||||
name: "filter labels AND both must exist",
|
||||
fields: fields{
|
||||
Filter: "labels = 4 && labels = 5",
|
||||
},
|
||||
args: defaultArgs,
|
||||
want: []*Task{
|
||||
task35, // only task with both labels 4 and 5
|
||||
},
|
||||
wantErr: false,
|
||||
},
|
||||
{
|
||||
// Regression: AND-joined negative filters must NOT be merged into a
|
||||
// single NOT EXISTS (would produce trivially-true condition).
|
||||
name: "filter labels not eq AND both excluded",
|
||||
fields: fields{
|
||||
Filter: "labels != 4 && labels != 5",
|
||||
},
|
||||
args: defaultArgs,
|
||||
want: []*Task{
|
||||
// Tasks that have neither label 4 nor label 5
|
||||
task3,
|
||||
task4,
|
||||
task5,
|
||||
task6,
|
||||
task7,
|
||||
task8,
|
||||
task9,
|
||||
task10,
|
||||
task11,
|
||||
task12,
|
||||
task15,
|
||||
task16,
|
||||
task17,
|
||||
task18,
|
||||
task19,
|
||||
task20,
|
||||
task21,
|
||||
task22,
|
||||
task23,
|
||||
task24,
|
||||
task25,
|
||||
task26,
|
||||
task27,
|
||||
task28,
|
||||
task29,
|
||||
task30,
|
||||
task31,
|
||||
task32,
|
||||
task33,
|
||||
task39,
|
||||
task47,
|
||||
},
|
||||
wantErr: false,
|
||||
},
|
||||
{
|
||||
name: "filter project_id",
|
||||
fields: fields{
|
||||
|
||||
@@ -89,6 +89,16 @@ var strictComparators = map[taskFilterComparator]bool{
|
||||
taskFilterComparatorNotEquals: true,
|
||||
}
|
||||
|
||||
// isRangeComparator returns true for comparators where combining multiple
|
||||
// conditions into a single EXISTS subquery is semantically correct (i.e. a
|
||||
// single row can satisfy both conditions simultaneously).
|
||||
func isRangeComparator(c taskFilterComparator) bool {
|
||||
return c == taskFilterComparatorGreater ||
|
||||
c == taskFilterComparatorGreateEquals ||
|
||||
c == taskFilterComparatorLess ||
|
||||
c == taskFilterComparatorLessEquals
|
||||
}
|
||||
|
||||
type taskSearcher interface {
|
||||
Search(opts *taskSearchOptions) (tasks []*Task, totalCount int64, err error)
|
||||
}
|
||||
@@ -182,25 +192,26 @@ func convertFiltersToDBFilterCond(rawFilters []*taskFilter, includeNulls bool) (
|
||||
continue
|
||||
}
|
||||
|
||||
// Collect all consecutive AND-joined filters targeting the same sub-table
|
||||
// Collect all consecutive AND-joined range filters targeting the same sub-table.
|
||||
// Only range comparators (>, >=, <, <=) are merged because they express
|
||||
// conditions a single row can satisfy simultaneously (e.g. reminder > X AND
|
||||
// reminder < Y). Equality/IN/NOT comparators must remain as separate EXISTS
|
||||
// subqueries because each matching value lives in its own row (e.g.
|
||||
// labels = 4 && labels = 5 means two different rows must each exist).
|
||||
group := []*taskFilter{f}
|
||||
for i+1 < len(rawFilters) {
|
||||
next := rawFilters[i+1]
|
||||
nextSubTable, nextOk := subTableFilters[next.field]
|
||||
if !nextOk || nextSubTable.Table != subTableFilterParams.Table || next.join != filterConcatAnd {
|
||||
break
|
||||
if isRangeComparator(f.comparator) {
|
||||
for i+1 < len(rawFilters) {
|
||||
next := rawFilters[i+1]
|
||||
nextSubTable, nextOk := subTableFilters[next.field]
|
||||
if !nextOk || nextSubTable.Table != subTableFilterParams.Table || next.join != filterConcatAnd {
|
||||
break
|
||||
}
|
||||
if !isRangeComparator(next.comparator) {
|
||||
break
|
||||
}
|
||||
group = append(group, next)
|
||||
i++
|
||||
}
|
||||
if next.field == "assignees" && next.comparator == taskFilterComparatorLike {
|
||||
break
|
||||
}
|
||||
// Don't mix positive and negative filters (EXISTS vs NOT EXISTS)
|
||||
currentIsNegative := f.comparator == taskFilterComparatorNotEquals || f.comparator == taskFilterComparatorNotIn
|
||||
nextIsNegative := next.comparator == taskFilterComparatorNotEquals || next.comparator == taskFilterComparatorNotIn
|
||||
if currentIsNegative != nextIsNegative {
|
||||
break
|
||||
}
|
||||
group = append(group, next)
|
||||
i++
|
||||
}
|
||||
|
||||
// Build the combined condition for all filters in the group
|
||||
@@ -232,7 +243,7 @@ func convertFiltersToDBFilterCond(rawFilters []*taskFilter, includeNulls bool) (
|
||||
filterSubQuery := subTableFilterParams.ToBaseSubQuery().And(combinedInnerCond)
|
||||
|
||||
var filter builder.Cond
|
||||
if group[0].comparator == taskFilterComparatorNotEquals || group[0].comparator == taskFilterComparatorNotIn {
|
||||
if f.comparator == taskFilterComparatorNotEquals || f.comparator == taskFilterComparatorNotIn {
|
||||
filter = builder.NotExists(filterSubQuery)
|
||||
} else {
|
||||
filter = builder.Exists(filterSubQuery)
|
||||
@@ -243,8 +254,10 @@ func convertFiltersToDBFilterCond(rawFilters []*taskFilter, includeNulls bool) (
|
||||
}
|
||||
|
||||
dbFilters = append(dbFilters, filter)
|
||||
// Use the join from the last filter in the group (how this group connects to the next filter)
|
||||
dbFilterJoins = append(dbFilterJoins, group[len(group)-1].join)
|
||||
// Use the join from the first filter in the group: f.join describes how
|
||||
// this group connects to the previous element (matches the convention
|
||||
// where dbFilterJoins[i+1] combines dbFilters[i] with dbFilters[i+1]).
|
||||
dbFilterJoins = append(dbFilterJoins, f.join)
|
||||
continue
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user