refactor(group): migrate ListByUser callers to membership, delete legacy method#1643
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR refactors group-listing APIs to use principal-based filtering through a filter struct field rather than a dedicated method parameter. ChangesPrincipal-Based Group Filtering Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 |
Coverage Report for CI Build 26279162990Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.2%) to 42.847%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
![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
🧹 Nitpick comments (4)
core/group/filter.go (1)
10-12: ⚡ Quick winAlign filter documentation with the new intersection semantics.
The new
Principaldocs describePrincipal ∩ GroupIDs, but the struct still says only one filter is applied. Please update that stale comment to avoid misleading callers.core/membership/service_test.go (1)
2293-2395: ⚡ Quick winAdd failure-path coverage for
ListGroupsByPrincipal.This new test only validates success scenarios. Please add cases for policy list failure and
groupService.Listfailure (whenorgIDis set) to lock in error propagation behavior.core/group/service_test.go (1)
253-354: ⚡ Quick winAdd negative tests for the new principal branch in
List.Please include cases where (1) membership service is not wired and (2)
ListGroupsByPrincipalreturns an error, asserting that the error is returned and repositoryListis not called.core/invitation/service.go (1)
322-323: ⚡ Quick winScope invited-user group lookup to the invitation org.
You already have
invite.OrgID; include it in the filter to avoid querying groups across unrelated orgs during accept flow.Proposed change
principal := authenticate.Principal{ID: userOb.ID, Type: schema.UserPrincipal} - userGroups, err := s.groupSvc.List(ctx, group.Filter{Principal: &principal}) + userGroups, err := s.groupSvc.List(ctx, group.Filter{ + Principal: &principal, + OrganizationID: invite.OrgID, + }) if err != nil { return err }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c347f942-1bf7-4ec2-90aa-6e9b9aba606a
📒 Files selected for processing (16)
core/group/filter.gocore/group/mocks/membership_service.gocore/group/mocks/relation_service.gocore/group/service.gocore/group/service_test.gocore/invitation/mocks/group_service.gocore/invitation/service.gocore/membership/service.gocore/membership/service_test.gocore/project/mocks/group_service.gocore/project/service.gocore/project/service_test.gointernal/api/v1beta1connect/interfaces.gointernal/api/v1beta1connect/mocks/group_service.gointernal/api/v1beta1connect/user.gointernal/api/v1beta1connect/user_test.go
💤 Files with no reviewable changes (3)
- core/group/mocks/relation_service.go
- internal/api/v1beta1connect/mocks/group_service.go
- internal/api/v1beta1connect/interfaces.go
…acy method Switches every reader of "groups a principal belongs to" off `group.Service.ListByUser` (SpiceDB `LookupResources` + repo enrichment) onto Postgres policy reads via `membership.Service.ListGroupsByPrincipal`. Adds `group.Filter.Principal` so the composition lives inside `Service.List` instead of the handler. Four caller sites migrated; old `ListByUser`, `intersectPATScope`, and the group service's `RelationService.LookupResources` dependency are deleted. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PATs scope orgs and projects, not groups, so a PAT's view of groups is just its user's. ListResourcesByPrincipal was intersecting user-side groups with an empty PAT-side set (PATs hold no group policies) and returning []. Resolve up front and skip the PAT branch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
End-to-end test resultsTested the six affected RPCs on a freshly-provisioned org ( Initial run surfaced one regression in PAT-scoped group listings. After applying the fix to Full run
Affected RPCs covered
Case #9 (malformed UUID → |
… and invitation group dedup; correct error log labels
- core/project/service_test.go: PAT principal with NonInherited=true where the resolved user has group memberships and those groups grant project access; verifies the migrated groupService.List(Filter{Principal:&p}) path intersects with PAT scope correctly.
- core/invitation/service_test.go: Accept where the invitee is pre-existing in one of the invite's groups; verifies dedup path inside Accept calls SetGroupMemberRole only for the new group.
- internal/api/v1beta1connect/user.go: ListUserGroups / ListCurrentUserGroups error log operation labels now read "List" instead of stale "ListByUser".
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
AmanGIT07
force-pushed
the
migrate-group-listbyuser-to-membership
branch
from
42a5c50 to
9109a77
Compare
3 participants
Summary
Switches every reader of "groups a principal belongs to" off
group.Service.ListByUser(SpiceDBLookupResources+ repo enrichment) onto Postgres policy reads viamembership.Service.ListGroupsByPrincipal. Mirrors the org migration in #1639.Changes
group.Filter.Principal;group.Service.Listbranches on it.membership.Service.ListGroupsByPrincipal(ctx, principal, orgID)shim (sibling ofListOrgsByPrincipal).core/project/service.go::listNonInheritedProjectIDscore/invitation/service.go::Acceptinternal/api/v1beta1connect/user.go::ListUserGroupsinternal/api/v1beta1connect/user.go::ListCurrentUserGroupsgroup.Service.ListByUser,intersectPATScope, andRelationService.LookupResourcesfrom the group package — no remaining callers.ListByUserfromproject.GroupService,invitation.GroupService, andinternal/api/v1beta1connect.GroupServiceinterfaces.Technical Details
membership.listGroupsForPrincipalreads onlyResourceType=grouppolicies, preserving today'sgroup.membership = member + ownersemantics. No org→group inheritance expansion.membership.ListResourcesByPrincipal, which intersects user-side and PAT-side resource IDs.group.Service.Listbranching: whenFilter.Principal != nil, the shim is called withFilter.OrganizationIDas the org narrowing; result is intersected withFilter.GroupIDsif both are set; empty result short-circuits to[]Group{}.Test Plan
make lint— 0 issuesmake test— affected packages green (core/group,core/project,core/invitation,core/membership,internal/api/v1beta1connect,core/deleter,core/aggregates/orgpats,core/userpat)go test -raceon the same set — all greenListUserGroups/ListCurrentUserGroupsagainst pre-change for a user with mixed direct + PAT principals