Repository navigation
fix(store): return every org service user and honour the requested sort - #1929
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 SummarySummary by CodeRabbit
WalkthroughThe service-user search path now uses explicit RQL metadata, shared query utilities, deterministic pagination, active-project aggregation, organization resolution, API validation, and PostgreSQL coverage. ChangesService-user search behavior
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The service-user search changes are mergeable after normal checks; no concrete unresolved behavior risk was identified. 🚥 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Coverage Report for CI Build 35575440706Coverage increased (+0.2%) to 50.478%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Advanced
Run ID: 2e96cca8-97bb-426a-92f9-84faefa5bf30
📒 Files selected for processing (2)
internal/store/postgres/org_serviceuser_repository.gointernal/store/postgres/org_serviceuser_repository_test.go
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Assert the returned pagination metadata. · org_serviceuser_repository.go:100-110
internal/store/postgres/org_serviceuser_repository.go:100-110
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert the returned pagination metadata. The
titleshelper discardsSearch'sPagination, and the unit tests assert only the derived limit. Add assertions for the default limit and the requested offset and limit, including the explicit offset/limit path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Advanced
Run ID: ca4e3a1b-5273-4a01-82c9-33dd33edfb64
📒 Files selected for processing (6)
Makefilecore/aggregates/orgserviceuser/orgserviceuser.gointernal/api/v1beta1connect/organization_serviceuser.gointernal/store/postgres/org_serviceuser_repository.gointernal/store/postgres/org_serviceuser_repository_pg_test.gointernal/store/postgres/org_serviceuser_repository_test.go
💤 Files with no reviewable changes (1)
- core/aggregates/orgserviceuser/orgserviceuser.go
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.
Fair point, fixed in d22f197. The unit test now asserts the whole assert.Equal(t, tt.wantPage, gotPage)The integration suite gained
That last case matters for the admin UI, which decides whether to fetch another page by comparing the row count against the limit it reads off the response. |
Endpoint comparison, run against a live serverI ran both endpoints side by side on a local Frontier (postgres and SpiceDB in docker) and recorded what each actually returned. Every row below is a real call, not a reading of the code. The Search (main) column is the same request against a binary built from Re-run and corrected on 18 Sep 2026, after the rebase. Branch at FixturesOne org, six service users, two projects. Everything except the state changes was created through the API so SpiceDB stays consistent. Creating a service user always gives it a viewer policy on its org, so every row below has one unless it says otherwise. The policy on
Which rows come back
Response shape
Rows 7 to 9 are a deliberate difference. The two endpoints answer different questions, and the proto messages are unchanged by this PR. Query support (List takes only an org id, so it has no query surface)
Rejections
Addressing the organization
Rows 24 and 25 were found by this comparison and fixed in the PR. Search now resolves the org first, the way List already did. Rows 27 and 28 are the soft delete check. Search resolves the org through Row 26 is deliberate. Search uses What is left different on purpose
What changed in this re-runThe earlier version of this comment had a few cells that were wrong or vague. They are corrected above:
Everything else came out the same as before. |
0e0cdac to
0e47579
Compare
Deleted organizations are skipped, including on the admin APIRebased onto What the admin search does nowThe handler resolves the organization through Verified against a live server, one call per row:
The orphan row is the one that matters. The rest of the queryEvery table the search touches carries a LEFT JOIN "policies" ON (... AND "policies"."deleted_at" IS NULL)
LEFT JOIN "projects" ON (... AND "projects"."deleted_at" IS NULL)
WHERE "serviceusers"."org_id" = $3 AND "serviceusers"."deleted_at" IS NULLThe projects filter was already there, written by hand; it now uses Nothing sets
One behaviour change to be aware ofSearching the platform org id now returns 404 where it used to return the bootstrap superuser. The admin UI never does this. Its only use of that id is Still open
|
SearchOrganizationServiceUsersreturned fewer service users thanListOrganizationServiceUsersfor the same org. Digging in turned up five separate bugs. All three are confirmed on a local Frontier, before and after.Bug 1: service users without a project policy were dropped
ListOrganizationServiceUsersreads theserviceuserstable directly, so it returns every service user in the org. The search query built its rows with two inner joins:A service user only survived those joins if it had at least one policy on a project. Service users with only an org level policy, or with no policy at all, were dropped before any filter or limit ran. The projects join also never checked
projects.deleted_at, so a deleted project still showed up in the response.Both joins are now
LEFT JOIN, the projects join requiresprojects.deleted_at IS NULL, and the aggregate is wrapped inCOALESCE(... FILTER (WHERE projects.id IS NOT NULL), '[]'). The filter drops the null row a left join produces, and the coalesce returns[]rather than SQLNULL, which the row struct scans as a plain string.Bug 2: the requested sort was ignored
The base query ended with a fixed
ORDER BY serviceusers.title ASC, and the requested sort was appended after it. So the fixed term always won and the requested sort only broke ties between rows with the same title. Titles are mostly unique, so it did nothing.The old expected SQL showed it plainly. Asking for title descending produced:
which is just title ascending.
The base query no longer sets an order. The requested sort leads,
title ASCis used only when no sort is asked for, and a finalid ASCterm makes the order total so paging by offset cannot repeat or skip a row.Bug 3: filtering on created_at always failed
AggregatedServiceUsertagged its fields likerql:"filter,sort,column=created_at". Salt resolves an rql field by thenametag (or the lowercased Go field name) and reads its type from thetypetag. It ignorescolumn. Socreated_at,org_idandupdated_atdid not resolve at all, and every field that did resolve fell back to typestring.The result was that any filter on
created_atfailed in the handler before reaching the database:Tags are now
rql:"name=created_at,type=datetime"and so on, matchingcore/prospect/prospect.go.Refactor: use the shared rql helpers
The repository had its own
addFilter,addSearchandaddSort. These are gone, replaced byutils.AddRQLFiltersInQuery,AddRQLSearchInQuery,AddRQLSortInQueryandAddRQLPaginationInQueryfrompkg/utils/rql.go, as used byprospect_repository.go,userpat_repository.goandaudit_record_repository.go.One wrinkle. Those helpers emit unqualified column names, and the three joined tables here all have an
id, atitleand acreated_at. An unqualified name inORDER BYresolves to the select alias and is fine, but inWHEREPostgres rejects it:So the joined and grouped query is now wrapped in a subquery, and rql is applied to the wrapper where only the aliased output columns are visible. Explicit allowlists say which columns may be filtered, searched and sorted, and unsupported ones are rejected as
ErrBadInput, which the handler maps toinvalid_argumentinstead ofinternal.Bug 4: a project could be listed twice
If a service user holds two roles on the same project, it has two policy rows, so the join produces two rows and the project appeared twice in the response. The admin UI renders the titles joined by a comma, so it showed "Live Project, Live Project". Confirmed on a local database.
The aggregate is now
JSONB_AGG(DISTINCT JSONB_BUILD_OBJECT(...)). It has to bejsonbrather thanjsonbecause thejsontype has no equality operator, soDISTINCTcannot be applied to it.Bug 5: the UI offers a Projects filter the API rejected
web/sdk/admin/views/organizations/details/apis/columns.tsxmarks the Projects columnenableColumnFilter: true. That filter never worked. The old repository silently ignored any filter it did not recognise, so the results came back unfiltered. Filtering for%Live%and for%zzzz%both returned the same row.The base query now also aggregates the project titles into a
projectstext column, using the same comma separated form the UI renders. That column is allowlisted for filters and for search, so the filter does what it says. Filtering for%zzzz%now correctly returns nothing.Checked against the admin UI
web/in this repo holds the admin UI, so the request shapes are not guesswork.likechange is a non-issue. I flagged earlier that the shared helper stops auto-wrappinglikevalues in%. Apsara's DataTable never sendslike. It mapscontains,starts_withandends_withto operatorilikeand adds the wildcards itself infilter-operations.cjs.iliketook the same default code path before and after, so nothing changes. Verified: the UI's exact request,ilikewith%policy%, returns the right rows.transformDataTableQueryToRQLRequestsendslimit: query.limit || defaultLimitwithdefaultLimit = 50, so the UI always sends a limit.useInfiniteQuery, andgetConnectNextPageParamcomputeshasMorePagesasitems.length === pagination.limitand the next offset aspagination.offset + pagination.limit, both read from the response. The response now reports the limit actually applied rather than echoing the request, and the newid ASCtie breaker keeps pages from repeating or skipping rows.filterType: "date"on Created on and mapscreatedAttocreated_at. Every such request failed with'created_at' is not a valid fieldbecause of bug 3. It works now.DEFAULT_SORTis{ name: 'createdAt', order: 'desc' }, which is exactly the request in the original report.Verified end to end on a local Frontier
Postgres and SpiceDB in docker, server built from this branch, seeded with four service users in one org: one with a project policy, one with only an org policy, one with no policy, and one whose policy points at a deleted project. Same request, two binaries.
Before, built from
main:After, built from this branch, sorted
created_at desc:Also checked against the running server, with the UI's own request shapes:
title descd, c, b, acreated_at descd, b, c, a, newest firstcreated_atrange, as the date picker sends itinvalid_argumenttitleilike%policy%, as the UI sends itb, cprojectsilike%Live%a-with-projectprojectsilike%zzzz%a, bthenc, d, no overlapupdated_atinternalinvalid_argumentTwo regressions this PR introduced, now fixed
An independent review caught two cases where this branch turned a working or harmless request into a 500. Both reproduced against
mainand this branch on a local server.group_byreturned a 500.AddRQLSortInQueryprependsORDER BY <group_by column>for every group_by entry, and the wrapper select has no such column.mainreturned 200 and ignored group_by. The repository now rejects group_by asErrBadInput, which is whatuser_orgs_repository.godoes, since this response has no group block to fill.Bad filter values returned a 500. The handler never called
rql.ValidateQuery, unlike ten other handlers including its closest siblingSearchOrganizationServiceUserCredentials. Onmaina badcreated_atvalue happened to fail earlier with a 400, only because bug 3 meant the field never resolved. Once it resolved, the raw value reached Postgres. The handler now validates, and theErrBadInputbranch returns the real reason instead of a generic message.group_by: ["foo"]'foo' is not a valid sort keygroup_by: ["title"]group_by is not supportedcreated_at gt "yesterday-ish"not a valid ISO datetime stringcreated_atwith operatorinTests
There are two layers.
TestOrgServiceUserRepository_prepareDataQuerypins the generated SQL. The joined part lives in one constant so each case shows only the WHERE, ORDER BY and pagination it exercises. That catches the sort and tag bugs, but on its own it would pass unchanged with the old inner joins, so it does not prove much about the main bug.org_serviceuser_repository_pg_test.gois new and runs the real query against a real Postgres through the existing dockertest harness. It seeds the five shapes that actually broke: a service user with a live project policy, one with only an org policy, one with no policy, one whose project is soft deleted, and one holding two roles on a single project. It asserts the row set, the per-user project lists, both sorts, thecreated_atfilter, theprojectsfilter, search across both columns, disjoint pages, and the rejected inputs.I checked that it fails on the bug rather than merely passing on the fix. Putting the two
INNER JOINs back fails 5 of its 8 tests.golangci-lint runreports 0 issues. The dockertest suites in this package need a reachable docker socket. WithDOCKER_HOST=unix:///.../docker.sockthe whole ofinternal/store/postgres,internal/api/v1beta1connectandcore/...passes with no failures.One CI change
The
unitjob failed withpanic: test timed out after 2m30s, and the log names the package:That is
make testrunning-count 2 -timeout 150s. The timeout is per package, and this package now has 25 dockertest suites, each starting its own postgres container, each run twice.The budget was already almost gone before this PR. Measured locally with the CI flags, the package takes 117.2s without the new suite and 120.3s with it. The new suite costs 3.1s, about 3 percent. CI stopped at 150.2s, so it was sitting at roughly 147s already. This is consistent with the history on this branch, where the same timeout panic appeared at
61bb71caand821c0a6d, both before the new suite existed, while other pushes passed. It was flaky at the limit.So
-timeoutgoes from 150s to 600s. That gives the package real headroom rather than leaving it one suite away from failing again. The new suite itself is not flaky:-race -count 3passes.The better fix is for these suites to share one postgres container instead of starting 25. That is a change to shared test infrastructure and does not belong in this PR.
Follow-ups, not in this PR
policies.principal_id, so every search sequentially scanspolicies. Pre-existing, and the real scaling limit here. A migration adding(principal_id, principal_type, resource_type)would fix it.org_projects_repository.gohas the same inner-join shape: a project with no human members disappears fromSearchOrganizationProjects, and there is nodeleted_atguard on projects or users.SearchOrganizationServiceUsersreturns 200 with an empty list for an org that does not exist, whereListOrganizationServiceUsersreturns 404.state; the search does not. That is the last shape difference between the two endpoints.internal/store/postgreseach start their own container. Sharing one would cut minutes off every CI run.