fix(membership): correct inflated total count in user and identity list queries#6901
fix(membership): correct inflated total count in user and identity list queries#69010xfandom wants to merge 2 commits into
Conversation
…st queries The org/project user list (`membership-user-dal.findUsers`) and the org identity search (`identity-org-dal.searchIdentities`) derived their total count from a window function (`count(...) OVER (PARTITION BY scopeOrgId)`) evaluated over a query that joins `membership_roles`. Because that join is one-to-many, a member with N role assignments produced N rows, so the count returned the number of role assignments rather than the number of distinct members. An org with 5 users where 2 have multiple roles reported 8. Replace the window count with a separate `countDistinct(membership.id)` query over the same filtered set, mirroring the approach already used in `membership-group-dal.findGroups`. Pagination is unaffected.
|
| Filename | Overview |
|---|---|
| backend/src/services/identity/identity-org-dal.ts | Replaces the inflated window-function count with a separate countDistinct query via a buildIdentitySearchBase() factory; logic is correct but the filter callback is duplicated for searchQuery and countQuery, and the two queries run sequentially. |
| backend/src/services/membership-user/membership-user-dal.ts | Splits the former single query into a baseUserQuery, a cloned count query (countDistinct), and a cloned paginated query; correctly applies filters before cloning and restores the distinct on the pagination branch. Logic is sound and mirrors the accepted findGroups pattern. |
Reviews (1): Last reviewed commit: "fix(membership): correct inflated total ..." | Re-trigger Greptile
| buildKnexFilterForSearchResource(countQuery, searchFilter, (attr) => { | ||
| switch (attr) { | ||
| case "role": | ||
| return [`${TableName.Role}.slug`, `${TableName.MembershipRole}.role`]; | ||
| case "name": | ||
| return `${TableName.Identity}.name`; | ||
| default: | ||
| throw new BadRequestError({ message: `Invalid ${String(attr)} provided` }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Duplicate filter callback inside
searchFilter block
The mapper function passed to buildKnexFilterForSearchResource for countQuery (lines 452–461) is byte-for-byte identical to the one applied to searchQuery (lines 442–451). Extracting it into a shared const filterMapper defined once before the if (searchFilter) block would eliminate the duplication and make future changes (e.g. adding a new filterable attribute) easier to keep in sync.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| const [countResult] = (await countQuery.countDistinct(`${TableName.Membership}.id as count`)) as [ | ||
| { count: string | number }? | ||
| ]; | ||
| const totalCount = Number(countResult?.count ?? 0); |
There was a problem hiding this comment.
Count and main queries run sequentially, but they are independent
countQuery is awaited before query is even constructed (line 474), even though totalCount is not needed to build or execute the main query. Both queries share only orgId (a closure variable), so they could be issued concurrently with Promise.all. The current pattern doubles the number of serial round-trips to the database for every page load of the identities list. The same observation applies to findUsers in membership-user-dal.ts. This follows the same approach as findGroups, but a parallel execution would halve the latency here.
… query Address review feedback on searchIdentities: extract the duplicated buildKnexFilterForSearchResource attribute mapper into a single shared function, and run the distinct-count query concurrently with the paginated fetch via Promise.all since they are independent.
Context
Fixes #5885.
The "All users" / "All identities" list endpoints reported an inflated total count. Both DALs derived their total from a window function evaluated over a query that joins
membership_roles:Since the membership →
membership_rolesjoin is one-to-many, a member with N role assignments produces N rows, so the window counted role assignments, not distinct members. For an org with 5 users where 2 have multiple roles, the count came back as 8 instead of 5. This throws off pagination math (page counts, "showing X of Y") in the UI.Two queries were still affected:
membership-user-dal.ts→findUsersidentity-org-dal.ts→searchIdentitiesPR #5850 already fixed the equivalent bug for the project identity membership list, and
membership-group-dal.ts→findGroupswas subsequently fixed the same way. This PR brings the remaining two siblings in line.The fix replaces the window count with a separate
countDistinct("Membership"."id")query over the same filtered set (before pagination is applied), exactly mirroring the pattern already used infindGroups. Pagination and result shape are unchanged.Steps to verify the change
Type
Checklist