refactor(organization): migrate ListByUser callers to membership, delete legacy method#1639
Merged
AmanGIT07 merged 4 commits intoMay 22, 2026
Conversation
…ete legacy method Switches every reader of "orgs a principal belongs to" off `organization.Service.ListByUser` (SpiceDB `LookupResources` + repo enrichment) onto Postgres policy reads via `membership.Service.ListOrgsByPrincipal`. Adds `organization.Filter.Principal` so the composition lives inside `Service.List` instead of the handler. Five handler call sites and three internal callers migrated; old `ListByUser`, `Filter.UserID`, and the org service's `RelationService.LookupResources` dependency are deleted. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
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)
📝 WalkthroughSummary by CodeRabbit
WalkthroughReplaces OrganizationService.ListByUser with MembershipService.ListResourcesByPrincipal, switches organization filters from UserID to Principal, and updates services, API handlers, mocks, and tests to use membership-based principal lookups. ChangesPrincipal-based organization API refactoring
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
core/organization/service_test.go (1)
207-297: ⚡ Quick winAdd principal-path coverage for
Service.List.This suite no longer verifies the new
Filter.Principalflow (membershipService.ListOrgsByPrincipal, ID intersection, empty short-circuit, and unwired membership error). Please add focused subtests for those branches to protect the refactor’s core behavior.Suggested test additions
func TestService_List(t *testing.T) { ctx := context.Background() - newService := func() (*organization.Service, *mocks.Repository) { + newService := func() (*organization.Service, *mocks.Repository, *mocks.MembershipService) { mockRepo := mocks.NewRepository(t) + mockMembership := mocks.NewMembershipService(t) ... svc := organization.NewService(...) + svc.SetMembershipService(mockMembership) - return svc, mockRepo + return svc, mockRepo, mockMembership } + t.Run("filters by principal orgs", func(t *testing.T) { + svc, mockRepo, mockMembership := newService() + p := authenticate.Principal{ID: "u1", Type: schema.UserPrincipal} + mockMembership.EXPECT().ListOrgsByPrincipal(mock.Anything, p).Return([]string{"org-1", "org-2"}, nil).Once() + mockRepo.On("List", ctx, organization.Filter{Principal: &p, IDs: []string{"org-1", "org-2"}}). + Return([]organization.Organization{{ID: "org-1"}, {ID: "org-2"}}, nil).Once() + _, err := svc.List(ctx, organization.Filter{Principal: &p}) + assert.NoError(t, err) + }) + + t.Run("returns empty when principal has no matching orgs", func(t *testing.T) { + svc, _, mockMembership := newService() + p := authenticate.Principal{ID: "u1", Type: schema.UserPrincipal} + mockMembership.EXPECT().ListOrgsByPrincipal(mock.Anything, p).Return([]string{}, nil).Once() + got, err := svc.List(ctx, organization.Filter{Principal: &p}) + assert.NoError(t, err) + assert.Empty(t, got) + }) }core/deleter/service_test.go (1)
263-264: ⚡ Quick winTighten the membership expectation to assert principal and filter shape.
Using
mock.Anythingfor principal/filter makes this test pass even ifDeleteUsersends wrong principal type/ID or non-empty filter.Proposed tightening
- mbrSvc.EXPECT().ListResourcesByPrincipal(mock.Anything, mock.Anything, schema.OrganizationNamespace, mock.Anything). + mbrSvc.EXPECT().ListResourcesByPrincipal( + mock.Anything, + authenticate.Principal{ID: "user-1", Type: schema.UserPrincipal}, + schema.OrganizationNamespace, + membership.ResourceFilter{}, + ). Return(nil, nil)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b5a1bbdc-3384-4d90-a664-3ab1a3962789
📒 Files selected for processing (26)
core/deleter/mocks/membership_service.gocore/deleter/mocks/organization_service.gocore/deleter/service.gocore/deleter/service_test.gocore/domain/mocks/membership_service.gocore/domain/mocks/org_service.gocore/domain/mocks/repository.gocore/domain/mocks/user_service.gocore/domain/service.gocore/domain/service_test.gocore/invitation/mocks/membership_service.gocore/invitation/mocks/organization_service.gocore/invitation/service.gocore/invitation/service_test.gocore/membership/service.gocore/organization/filter.gocore/organization/mocks/membership_service.gocore/organization/mocks/relation_service.gocore/organization/service.gocore/organization/service_test.gointernal/api/v1beta1connect/authenticate.gointernal/api/v1beta1connect/interfaces.gointernal/api/v1beta1connect/mocks/organization_service.gointernal/api/v1beta1connect/organization.gointernal/api/v1beta1connect/user.gointernal/api/v1beta1connect/user_test.go
💤 Files with no reviewable changes (4)
- core/organization/mocks/relation_service.go
- internal/api/v1beta1connect/interfaces.go
- core/invitation/mocks/organization_service.go
- core/deleter/mocks/organization_service.go
Coverage Report for CI Build 26277922087Warning 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.08%) to 42.675%Details
Uncovered Changes
Coverage Regressions2 previously-covered lines in 2 files lost coverage.
Coverage Stats
💛 - Coveralls |
Contributor
Author
End-to-end test resultsRan the matrix against a local server built from this branch. Fixtures (users + orgs) were created fresh per run — no pre-existing data was relied on for assertions. Setup
Results
Summary
Key behavior-change confirmations
Gaps from the PR's "RPCs to verify" listNot run yet:
Adjacent finding (not introduced here)While running test 37 I tripped a pre-existing 500 in the joinable-by-domain path for a different shape: user has no policy on the disabled org (so the new membership filter can't help). Filed separately as #1638. Reproduces on ArtifactsTest harness lives under |
Contributor
Author
Gap tests — follow-upRan the four cases left over from the previous comment.
Notes
Updated section totals
All items from the PR's "RPCs to verify end-to-end" checklist are now covered. |
whoAbhishekSah
approved these changes
May 22, 2026
4 tasks
Mock cleanup and expectation assertions were attached to the outer test instead of each subtest. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Satisfies the thelper lint after taking *testing.T as a parameter. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
AmanGIT07
deleted the
feature/orglistbyuser-migrate-callers-to-membership-and-delete
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Migrates every reader of "orgs a principal belongs to" off
organization.Service.ListByUser(which queried SpiceDB viarelationService.LookupResources) onto Postgres policies viamembership.Service.ListOrgsByPrincipal.Changes
New surface
organization.Filter.Principal *authenticate.Principal— when set, narrows the result to orgs the principal has a policy on. WhenFilter.IDsis also set, the two are intersected.membership.Service.ListOrgsByPrincipal(ctx, principal)— thin shim overListResourcesByPrincipal(..., OrganizationNamespace, ResourceFilter{}). Exists soorganization.MembershipServicecan declare it without a circular import (membership already imports organization).organization.Service.Listnow branches onFilter.Principal: nil-checks the membership dep, calls the shim, intersects with caller-supplied IDs, short-circuits empty, falls through to the repo. Re-instruments withmetrics.ServiceOprLatency(\"organization\", \"List\")(the old\"ListByUser\"histogram is gone).Handler migrations — every site that used
orgService.ListByUsernow callsorgService.List(Filter{Principal: …}):internal/api/v1beta1connect/authenticate.go— JWTorgsclaim (excludes disabled orgs viaState: organization.Enabled)internal/api/v1beta1connect/user.go—ListOrganizationsByUser,ListOrganizationsByCurrentUserinternal/api/v1beta1connect/organization.go—ListOrganizations,ListAllOrganizations. The two were near-identical; collapsed into asearchOrgsprivate helper. The only response-shape diff (Count) lives in the 3-line RPC wrappers.Internal callers — kept on the direct membership path (no enrichment needed):
core/deleter/service.go—DeleteUsercore/invitation/service.go—isUserOrgMembercore/domain/service.go—ListJoinableOrgsByDomainDeletions
organization.Service.ListByUserorganization.Filter.UserIDif f.UserID != \"\"branch inorganization.Service.Listorganization.RelationService.LookupResourcesfrom the interface (onlyListByUserused it)OrgService.ListByUserfrom thedeleter,invitation,domain, andv1beta1connectinterfacesDeliberate behavior changes
These are intentional fixes, not regressions:
DeleteUsernow visits disabled orgs. Old path wentListByUser → repo.List → notDisabledOrgExp, which silently skipped disabled orgs and left orphan policies pointing at the deleted user. New path reads policies directly. Verify with: delete a user who has a stale policy on a disabled org → no remaining rows inpoliciestable referencing that user_id.invitation.isUserOrgMembercorrectly counts stale policies on disabled orgs as membership. Prevents re-inviting a user to a disabled org they technically still have a policy on. OldListByUserfiltered disabled orgs out of the membership check, so the invite would (incorrectly) succeed and then later fail atorgService.Get → ErrDisabled.domain.ListJoinableOrgsByDomainexcludes disabled orgs the user has stale policies on from the joinable suggestions. Same root cause.orgsclaim path preserves the old behavior (disabled orgs excluded) via the explicitState: organization.Enabledfilter on the enrichment call.Architecture notes
Filter.Principalshape is morally the same as the deletedFilter.UserID, but defensible because: (1) typed*authenticate.Principalcarries principal type and PAT context, (2) both branches read the same store (Postgres) — no hidden subsystem switch, (3) named for what it semantically does.organization.Service.List, not the handler. Handler decodes the request, the service decides how to satisfy the query. Pattern is forward-compatible with the next two PRs (groups in CLD-3182, projects in CLD-3183) — each adds its ownFilter.Principaland a matchingService.List{Resource}ByPrincipalshim on membership.deleter,invitation,domain) deliberately skip enrichment — they don't need org rows, just IDs, so they hit membership directly without going throughorgService.List.RPCs to verify end-to-end
FrontierService/ListOrganizations(with and withoutuser_id)FrontierService/ListAllOrganizations(with and withoutuser_id)FrontierService/ListOrganizationsByUserFrontierService/ListOrganizationsByCurrentUser— exercise as user, service-user, and PAT; PAT must yield the intersected setFrontierService/AuthToken(withauth.token.claims.add_org_ids: true) — JWT claim should exclude disabled orgs; PAT path should yield PAT-narrowed orgsFrontierService/DeleteUser— verify the disabled-org-cleanup fixFrontierService/CreateOrganizationInvitation— verify stale-policy-on-disabled-org now blocks re-inviteTest plan
make buildmake lint(0 issues on touched packages)make test -racecore/domain/service_test.gocovers the disabled-org-policy-holder case inListJoinableOrgsByDomainTestService_Listincore/organizationreplaces the oldTestService_ListByUserwith broader pass-through coverage (empty filter, IDs, state, combined, error propagation, no-rows)TestConnectHandler_ListOrganizationsByUser/TestConnectHandler_ListOrganizationsByCurrentUsermock expectations to the newFilter{Principal}pattern; dropped now-unused membership-service mocks from those handlersmake e2e-test— regression suite around org, user, deleter, invitation, domainNot in this PR
🤖 Generated with Claude Code