fix(orgusers): support case-insensitive name filtering#1723
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesNon-role Filter Operator Validation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
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 |
Coverage Report for CI Build 28582274495Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+1.0%) to 44.847%Details
Uncovered Changes
Coverage Regressions443 previously-covered lines in 16 files lost coverage.
Coverage Stats
💛 - Coveralls |
| searchPattern := "%" + filter.Value.(string) + "%" | ||
| return goqu.Cast(goqu.I(columnName), "TEXT").NotILike(searchPattern), nil | ||
| case "ilike": | ||
| // case-insensitive match; the frontend already includes the wildcards (e.g. "%foo%") |
There was a problem hiding this comment.
Why not follow the same pattern? In one case, the frontend adds %..%, and in another, it does not. We should keep it consistent.
There was a problem hiding this comment.
Both the ilike operator and the %..% wildcards are added by Apsara's DataTable filter transform before the request leaves the browser — not by our app code.
In @raystack/apsara (components/data-table/utils/), transformToDataTableQuery builds each filter via two helpers (filter-operations.tsx):
getFilterOperatormaps the UI string operators toilike:if (filterType === FilterType.string && (operator === 'contains' || operator === 'starts_with' || operator === 'ends_with')) { return 'ilike'; }
getFilterValue→handleStringBasedTypesbakes the wildcards into the value:if (operator === 'contains') processedValue = `%${stringVal}%`; else if (operator === 'starts_with') processedValue = `${stringVal}%`; else if (operator === 'ends_with') processedValue = `%${stringVal}`; else if (operator === 'ilike') { if (!stringVal.includes('%')) processedValue = `%${stringVal}%`; }
So for the org-users name filter the client always sends { operator: "ilike", value: "%foo%" } — never like. That's why the ilike branch here treats wildcards as caller-provided.
You're right that it's inconsistent, though: like/ilike both compile to SQL ILIKE (via goqu .ILike()), and since the real caller only ever emits ilike with wildcards pre-added, the like/notlike branches that wrap server-side are effectively dead for this list. I'll align them to one convention — keeping ilike/notilike (caller owns the wildcards) and dropping the server-side wrapping in like/notlike — so there's a single, consistent path.
There was a problem hiding this comment.
Done — wildcards are now caller-owned for all four operators (like/notlike/ilike/notilike), so there is a single consistent convention. The server no longer wraps %..% on like/notlike; the caller (Apsara DataTable, which only ever emits ilike with wildcards pre-baked) owns them in every case. This matches org_pats_repository/org_billing_repository.
Updated the like/notlike tests accordingly to pass pre-wrapped values.
| case "like": | ||
| // case-insensitive match; wildcards are added here | ||
| searchPattern := "%" + filter.Value.(string) + "%" | ||
| return goqu.Cast(goqu.I(columnName), "TEXT").ILike(searchPattern), nil |
There was a problem hiding this comment.
Like also uses iLIKE only.
There was a problem hiding this comment.
Correct — goqu's .ILike() emits SQL ILIKE, so like here is already case-insensitive. The only real difference between the like and ilike branches is who owns the %..% wildcards (server-side for like, caller-side for ilike). See my note on the ilike case below — Apsara only ever sends ilike with wildcards baked in, so I'll collapse these to one convention.
There was a problem hiding this comment.
Good catch — fixed. I aligned this file to the same convention org_pats_repository and org_billing_repository already use (the two other repos that own wildcards caller-side + cast to TEXT):
case "like": return goqu.Cast(goqu.I(columnName), "TEXT").Like(filter.Value.(string)), nil // case-sensitive
case "notlike": return goqu.Cast(goqu.I(columnName), "TEXT").NotLike(filter.Value.(string)), nil
case "ilike": return goqu.Cast(goqu.I(columnName), "TEXT").ILike(filter.Value.(string)), nil // case-insensitive
case "notilike": return goqu.Cast(goqu.I(columnName), "TEXT").NotILike(filter.Value.(string)), nilSo like now maps to real SQL LIKE (case-sensitive) instead of ILIKE, and ilike stays ILIKE. No more silent case-insensitivity on like.
![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: 4f879278-05b7-407a-92d3-60dce59bd68d
📒 Files selected for processing (2)
internal/store/postgres/org_users_repository.gointernal/store/postgres/org_users_repository_test.go
💤 Files with no reviewable changes (1)
- internal/store/postgres/org_users_repository_test.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4f879278-05b7-407a-92d3-60dce59bd68d
📒 Files selected for processing (2)
internal/store/postgres/org_users_repository.gointernal/store/postgres/org_users_repository_test.go
💤 Files with no reviewable changes (1)
- internal/store/postgres/org_users_repository_test.go
🛑 Comments failed to post (1)
internal/store/postgres/org_users_repository.go (1)
327-337: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash rg -n -A6 'org_joined_at' internal/store/postgres/org_users_repository_test.goRepository: raystack/frontier
Length of output: 6564
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- org_users_repository.go (relevant function) ---' sed -n '327,380p' internal/store/postgres/org_users_repository.go | cat -n echo echo '--- org_users_repository_test.go (org_joined_at-related tests) ---' sed -n '150,230p' internal/store/postgres/org_users_repository_test.go | cat -n echo echo '--- search for org_joined_at + ilike / empty / notempty / bad operator cases ---' rg -n -A4 -B4 'org_joined_at|ilike|empty|notempty|wrapBadOperatorError|GetDataTypeOfField' internal/store/postgres org_users_repository_test.goRepository: raystack/frontier
Length of output: 50374
Datatype routing is missing for
org_joined_at.buildNonRoleFilterConditionstill applies the same operator allowlist to every field, then mapsorg_joined_attopolicies.created_atwithout checking the field type. That letslike/ilike/in/emptythrough on a datetime column instead of rejecting them; add a rejection test fororg_joined_at+ilike/empty.Source: Learnings
|
@coderabbitai Addressed the datatype-routing finding.
Restored the |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
cd /tmp && rm -rf frontier && git clone --depth 50 https://github.com/raystack/frontier.git frontier 2>&1 | tail -5
cd frontier && git fetch origin pull/1723/head:pr1723 2>&1 | tail -5 && git checkout pr1723 2>&1 | tail -5
sed -n '1,400p' internal/store/postgres/org_users_repository.go | grep -n "buildNonRoleFilterCondition" -A 80Length of output: 6890 Confirmed — the updated |
3 participants
Summary
Fixes "unable to filter by name" on the organization users list. The non-rolefilter builder did not support the
ilike/notilikeoperators, so a name filter sent asilikewas rejected withwrapBadOperatorErrorand the filter silently failed. This addsilike/notilikesupport and, while there, hardens operator handling by routing on the column's declared datatype so string-only operators can no longer be applied to the datetimeorg_joined_atcolumn.Changes
ilike/notilikeoperator support for string columns.rql.GetDataTypeOfField) instead of one combined operator list.org_joined_atcolumn toeq/neq/gt/lt/gte/lte; any other operator (includinglike/ilike/in/empty) now returnswrapBadOperatorError.ilike/notilike, a valid datetime operator (gte), andilikerejected on the datetime column.Technical Details
buildNonRoleFilterConditionlooks up the field's datatype up front:eq/neq/gt/lt/gte/lte.eq/neq/like/ilike/in/notin/notlike/notilike/empty/notempty.like/notlikewrap the value in%...%themselves;ilike/notilikeexpect the caller to include the wildcards (e.g.%foo%). The local operator lists mirrorsalt/rql's upstreamvalidString/DatetimeOperations, so the new branches are reachable from the API. Side effect: an unknown field name now fails fast viaGetDataTypeOfFieldinstead of falling through to the raw column name.Test Plan
SQL Safety (if your PR touches
*_repository.goorgoqu.*)?placeholders,goqu.Ex{}, orgoqu.Record{}— neverfmt.Sprintfor+building a query that gets executed.ToSQL()callers capture and forward params (query, params, err := stmt.ToSQL(); db.…Context(ctx, …, query, params...)). Neverquery, _, err := ….?placeholders inside single-quoted SQL literals ingoqu.L(usemake_interval(hours => ?)-style functions instead).//nolint:forbidigoor// #nosec G20xannotation has a one-line justification on the same line that a reviewer can verify.