refactor(group): resolve org from the group instead of the request#1739
refactor(group): resolve org from the group instead of the request#1739AmanGIT07 wants to merge 1 commit into
Conversation
Group read and update RPCs took an org_id that was trusted but never checked against the group. Remove org_id from those request protos and derive the org from the group record. A group's org is now fixed at creation and cannot change through an update. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughGroup operations across CLI, core service, Connect API handlers, Postgres repository, and tests are refactored to derive organization context from the group record instead of requiring an OrgId parameter. A new Estimated code review effort: 4 (Complex) | ~60 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 28920251707Coverage decreased (-0.01%) to 44.865%Details
Uncovered Changes
Coverage Regressions2 previously-covered lines in 1 file lost coverage.
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/api/v1beta1connect/group.go (1)
398-418: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winEnableGroup returns Internal instead of NotFound for invalid group IDs.
h.groupService.GetByIDscan returngroup.ErrInvalidID(empty id) orgroup.ErrInvalidUUID(malformed id) — both are expected, "not found"-class errors elsewhere in this file (seegetGroupInEnabledOrg). Here they're unconditionally wrapped asCodeInternal, unlikeDisableGroup/GetGroup/etc., which correctly map these toCodeNotFound. This is an inconsistent API contract for the same class of client error.🐛 Proposed fix
grps, err := h.groupService.GetByIDs(ctx, []string{request.Msg.GetId()}, group.Filter{IncludeDisabled: true}) if err != nil { - return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("EnableGroup.GetByIDs: group_id=%s: %w", request.Msg.GetId(), err)) + switch { + case errors.Is(err, group.ErrInvalidID), errors.Is(err, group.ErrInvalidUUID): + return nil, connect.NewError(connect.CodeNotFound, ErrGroupNotFound) + default: + return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("EnableGroup.GetByIDs: group_id=%s: %w", request.Msg.GetId(), err)) + } }
🧹 Nitpick comments (2)
internal/api/v1beta1connect/group.go (1)
264-274: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead error-mapping branches after removing
org_idfrom update.
organization.ErrInvalidUUIDandorganization.ErrNotExistcan no longer be returned bygroupService.UpdatesinceUpdateByIDno longer referencesorg_idin itsSETclause. These two cases are now unreachable.♻️ Proposed cleanup
- case errors.Is(err, group.ErrInvalidDetail), errors.Is(err, organization.ErrInvalidUUID), errors.Is(err, organization.ErrNotExist): + case errors.Is(err, group.ErrInvalidDetail): return nil, connect.NewError(connect.CodeInvalidArgument, ErrBadRequest)internal/api/v1beta1connect/group_test.go (1)
1443-1526: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing test coverage for invalid group id in
EnableGroup.None of the cases exercise
GetByIDsreturninggroup.ErrInvalidID/group.ErrInvalidUUID, which would have caught the Internal-vs-NotFound inconsistency flagged ingroup.go'sEnableGrouphandler.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6c995f43-09a8-4c41-ac5f-ce6be3796d2a
⛔ Files ignored due to path filters (1)
proto/v1beta1/frontier.pb.gois excluded by!**/*.pb.go,!proto/**
📒 Files selected for processing (12)
Makefilecmd/group.gocmd/group_test.gocore/group/filter.gocore/group/service.gointernal/api/v1beta1connect/group.gointernal/api/v1beta1connect/group_test.gointernal/api/v1beta1connect/interfaces.gointernal/api/v1beta1connect/mocks/group_service.gointernal/store/postgres/group_repository.gointernal/store/postgres/group_repository_test.gotest/e2e/regression/api_test.go
| s.Run("6. group admin update a new team with empty group id should return invalid arg", func() { | ||
| _, err := s.testBench.Client.UpdateGroup(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.UpdateGroupRequest{ | ||
| Id: "", | ||
| OrgId: myOrg.GetId(), | ||
| Body: &frontierv1beta1.GroupRequestBody{}, | ||
| Id: "", | ||
| Body: &frontierv1beta1.GroupRequestBody{}, | ||
| })) | ||
| s.Assert().Equal(connect.CodeInvalidArgument, connect.CodeOf(err)) | ||
| }) | ||
| s.Run("7. group admin update a new team without group id should fail", func() { | ||
| _, err := s.testBench.Client.UpdateGroup(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.UpdateGroupRequest{ | ||
| OrgId: myOrg.GetId(), | ||
| Body: &frontierv1beta1.GroupRequestBody{ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect UpdateGroup handler to verify empty ID validation before getGroupInEnabledOrg
# Get the AST outline of the group handler file, focusing on UpdateGroup
ast-grep outline internal/api/v1beta1connect/group.go --match UpdateGroup --view expanded
# Then read the UpdateGroup handler implementation
rg -n -A 40 'func \(h \*ConnectHandler\) UpdateGroup' internal/api/v1beta1connect/group.goRepository: raystack/frontier
Length of output: 2553
Return InvalidArgument for empty/missing group IDs
UpdateGroup sends empty or missing IDs into getGroupInEnabledOrg, which maps invalid IDs to NotFound. Validate the ID before that lookup, or subtests 6 and 7 will keep failing.
2 participants
What changed
org_idfrom the request protos ofGetGroup,UpdateGroup,ListGroupUsers,RemoveGroupUser,EnableGroup,DisableGroup,DeleteGroup, andSetGroupMemberRole. Each addresses a group by its id.longer carries an org id.
GroupRepository.UpdateByIDno longer writesorg_id. A group's org isfixed at creation and cannot change through an update.
GroupService.GetByIDstakes an optionalFilter. A newIncludeDisabledoption lets
EnableGroupfind a group that is currently disabled.group viewCLI takes<group-id>only.Behavior
org_idon these calls used to returnOrgNotFound.The field is gone. Clients that still send it are ignored (unknown JSON
fields are dropped), so nothing breaks on the wire.
FailedPrecondition.A read of a disabled group returns
NotFound.DeleteGroupis no longer blocked when the group's org is disabled.CreateGroupandListOrganizationGroupskeeporg_id— the org is areal input there.
Testing
TestGroupAPI, including a disable → enable round-trip.Depends on
refactor/group-rpcs(removesorg_idfrom the group request messages). Merge proton first, then bumpPROTON_COMMITto the merged SHA.🤖 Generated with Claude Code