fix: handle in/notin operators in token search filters#1656
fix: handle in/notin operators in token search filters#1656rohilsurana wants to merge 3 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR extends OrgTokensRepository.addFilter to support "in" and "notin" operators by parsing comma-separated values (using strings) and generating TEXT-cast SQL IN/NOT IN predicates from the resulting list. ChangesFilter Operator Extension
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e693f884-d065-4e95-abdd-bb22f50d3118
📒 Files selected for processing (1)
internal/store/postgres/org_tokens_repository.go
Coverage Report for CI Build 26624250328Coverage decreased (-0.008%) to 42.839%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
| case "in": | ||
| values := make([]string, 0) | ||
| for _, v := range strings.Split(filter.Value.(string), ",") { | ||
| if trimmed := strings.TrimSpace(v); trimmed != "" { | ||
| values = append(values, trimmed) | ||
| } | ||
| } | ||
| return query.Where(goqu.Cast(goqu.I(field), "TEXT").In(values)), nil | ||
| case "notin": | ||
| values := make([]string, 0) | ||
| for _, v := range strings.Split(filter.Value.(string), ",") { | ||
| if trimmed := strings.TrimSpace(v); trimmed != "" { | ||
| values = append(values, trimmed) | ||
| } | ||
| } | ||
| return query.Where(goqu.Cast(goqu.I(field), "TEXT").NotIn(values)), nil |
There was a problem hiding this comment.
A couple of small things -
The two cases are pretty much duplicated. They could be folded into one branch in the same shape as the like/notlike case right above:
case "in", "notin":
// ... build values ...
return query.Where(goqu.Ex{field: goqu.Op{filter.Operator: values}}), nil
```
Also, if someone sends an empty value (like "," or just ""), the split ends up as an empty list, and NOT IN () quietly returns every row — probably worth bailing out:
```
if len(values) == 0 {
return nil, fmt.Errorf("empty value list for operator %s on field %s", filter.Operator, filter.Name)
} One propagation note on that: the handler in organization_tokens.go:32-38 only maps postgres.ErrBadInput to CodeInvalidArgument; everything else becomes a 500. So to get a proper 400, this would need to wrap postgres.ErrBadInput — same convention you'd want for the existing "unsupported filter field" error at line 159, which also becomes a 500 today. We could fix both.
Could we also add a couple of test cases for these in org_tokens_repository_test.go? Single value, comma-separated, and a notin — that way the empty-input behavior is easy to pin down too.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/store/postgres/org_tokens_repository.go (1)
171-176:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEmpty value list yields
NOT IN ()/IN ()and silently matches every / no row.When the input is
""or just",",valuesends up empty and the predicate degrades into an always-true (notin) or always-false (in) clause. Consider bailing out with apostgres.ErrBadInput-wrapped error so the handler can map it to a 400 instead of returning unexpected rows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aede523b-db47-4675-94a0-ef0eb7b84c82
📒 Files selected for processing (1)
internal/store/postgres/org_tokens_repository.go
| case "in", "notin": | ||
| values := make([]string, 0) | ||
| for _, v := range strings.Split(filter.Value.(string), ",") { | ||
| if trimmed := strings.TrimSpace(v); trimmed != "" { | ||
| values = append(values, trimmed) | ||
| } | ||
| } | ||
| return query.Where(goqu.Ex{field: goqu.Op{filter.Operator: values}}), nil |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
goqu v9 In with string slice on integer column prepared statement postgres operator does not exist integer = text
💡 Result:
Root cause: you’re generating a SQL comparison where Postgres sees an integer column on one side and a text/varchar parameter on the other. In Postgres this triggers “operator does not exist: integer = text” (same pattern as integer vs text/varying errors) because there is no matching operator for those types without an explicit cast.[1][2] In goqu v9, build an IN clause using a slice whose element type matches the integer column type (e.g., []int64). goqu will render "col" IN (...) with those values.[3] What to do 1) Pass integer-typed slice values - Good: goqu.Ex{"id": []int64{1, 2, 3}}. - Bad: goqu.Ex{"id": []string{"1","2"}} (produces text parameters, leading to integer-vs-text operator errors at execution time).[1][2] 2) If your values are strings, convert them before building the query - Convert []string -> []int64 (e.g., with strconv.ParseInt) and pass the resulting []int64 slice to goqu. 3) If you can’t change the types, cast explicitly in SQL - goqu supports raw SQL fragments via goqu.L(...), which is commonly used when you need explicit casts.[4] Example (working shape) - goqu.Dialect("postgres").From("test").Where(goqu.Ex{"baz": []int64{1,2,3}}) produces ("baz" IN (1, 2, 3)), and in prepared mode it uses $1..$N placeholders with args of the right integer type.[3] If you paste your exact goqu snippet (how you build the slice and which column you target), I can rewrite it into the correct v9 code for your case.
Citations:
- 1: https://www.depesz.com/2008/05/05/error-operator-does-not-exist-integer-text-how-to-fix-it/
- 2: https://stackoverflow.com/questions/18128806/error-operator-does-not-exist-integer-character-varying-using-postgres-8-2
- 3: https://github.com/doug-martin/goqu/blob/master/goqu_example_test.go
- 4: https://github.com/vocoded/goqu/tree/12210d5e75d02418d421124a85ae98c7f898e843
Restore a type-safe cast for in/notin on integer amount filters
The current refactor builds values as strings and generates amount IN ($1, $2)-style comparisons without casting, which in Postgres commonly fails with operator does not exist: integer = text when the column is integer and the bound parameters are text. This aligns with why addSearch casts columns (including amount) to TEXT.
🛠️ Proposed fix restoring the TEXT cast
case "in", "notin":
values := make([]string, 0)
for _, v := range strings.Split(filter.Value.(string), ",") {
if trimmed := strings.TrimSpace(v); trimmed != "" {
values = append(values, trimmed)
}
}
- return query.Where(goqu.Ex{field: goqu.Op{filter.Operator: values}}), nil
+ castField := goqu.Cast(goqu.I(field), "TEXT")
+ if filter.Operator == "in" {
+ return query.Where(castField.In(values)), nil
+ }
+ return query.Where(castField.NotIn(values)), nil3 participants
Summary
in/notinoperator handling inOrgTokensRepository.addFilterinoperator requires splitting comma-separated string values into a slice for goqu'sIn()methodgoqu.Op{"in": "system.buy"}generates invalid SQL — it passes a raw string instead of a list toIN ()OrgBillingRepository.addRQLFiltersInQuerywhich already handles this correctlyRoot cause
The DataTable multiselect filter sends
operator: "in"withstringValue: "system.buy"(comma-separated for multiple values). TheaddFilterdefault case passes this directly to goqu asgoqu.Op{"in": filter.Value}, but goqu'sinneeds a slice, not a string. The fix splits the value by commas before passing toIn()/NotIn().Test plan
notinworks