refactor: extract common helpers in PAT handler and service#1556
refactor: extract common helpers in PAT handler and service#1556
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 42 minutes and 12 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR updates the Proton dependency hash in the Makefile and refactors Personal Access Token (PAT) service, API handler, and repository code to consolidate policy creation logic, centralize error mapping, and delegate sort operations to external utilities, while updating associated tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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}
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/store/postgres/userpat_repository.go (1)
122-140:⚠️ Potential issue | 🟡 MinorPotential nil-pointer dereference on
rqlQueryinList.
buildPATFilteredQueryguards againstrqlQuery == nilby reassigning the parameter to a newrql.Query, but that reassignment is local (Go passes the pointer by value). The outerrqlQueryinListis unaffected, so the subsequentutils.AddRQLSortInQuery(..., rqlQuery)at Line 133 andrqlQuery.Sortaccess at Line 137 will panic if a caller passesnil.Consider initializing the query once at the top of
List, or havingbuildPATFilteredQueryreturn the (possibly-substituted)*rql.Queryalong with the statement:🛡️ Proposed fix
func (r UserPATRepository) List(ctx context.Context, userID, orgID string, rqlQuery *rql.Query) (models.PATList, error) { + if rqlQuery == nil { + rqlQuery = utils.NewRQLQuery("", utils.DefaultOffset, utils.DefaultLimit, []rql.Filter{}, []rql.Sort{}, []string{}) + } baseStmt, err := r.buildPATFilteredQuery(userID, orgID, rqlQuery)…and drop the redundant nil guard inside
buildPATFilteredQuery.
🧹 Nitpick comments (2)
core/userpat/service.go (2)
608-621: Minor: simplifycreatePATPolicybody.The
if err != nil { return err }; return nilcan collapse to a single return of the call's error:♻️ Proposed tweak
func (s *Service) createPATPolicy(ctx context.Context, patID, roleID, resourceID, resourceType, grantRelation string) error { - if _, err := s.policyService.Create(ctx, policy.Policy{ + _, err := s.policyService.Create(ctx, policy.Policy{ RoleID: roleID, ResourceID: resourceID, ResourceType: resourceType, PrincipalID: patID, PrincipalType: schema.PATPrincipal, GrantRelation: grantRelation, - }); err != nil { - return err - } - return nil + }) + return err }
30-35: Optional: makesupportedPATResourceTypesimmutable.As a package-level slice, it is mutable and vulnerable to accidental modification (e.g.,
appendaliasing) from any function in the package. Only read-sites exist today, so the risk is low, but since the value is logically a constant set, consider one of:
- A small accessor that returns a fresh slice, or
- A
map[string]struct{}plus an exported helper if membership-test is the only use (bothvalidateScopesandListAllowedRolesonly callslices.Containson it).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 34e0aeea-edf8-409d-ad5a-b3e0119da98a
⛔ Files ignored due to path filters (4)
proto/v1beta1/admin.pb.gois excluded by!**/*.pb.go,!proto/**proto/v1beta1/frontier.pb.gois excluded by!**/*.pb.go,!proto/**proto/v1beta1/frontierv1beta1connect/admin.connect.gois excluded by!proto/**proto/v1beta1/frontierv1beta1connect/frontier.connect.gois excluded by!proto/**
📒 Files selected for processing (6)
Makefilecore/userpat/service.gocore/userpat/service_test.gointernal/api/v1beta1connect/user_pat.gointernal/api/v1beta1connect/user_pat_test.gointernal/store/postgres/userpat_repository.go
Coverage Report for CI Build 24666310198Coverage decreased (-0.07%) to 42.192%Details
Uncovered Changes
Coverage Regressions1 previously-covered line in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
3 participants
Description:
Summary
mapPATErrorhelper — single error-to-gRPC-code mapping replacing 8 duplicate switch blocks in PAT handlersgetLoggedInPrincipalWithUserhelper — replaces repeated GetLoggedInPrincipal + nil check across 5 handlerscreatePATPolicyhelper in service — single policy creation method replacing repeatedpolicy.Policystruct buildingsupportedPATResourceTypespackage-level var replacing 3 hardcoded[]string{OrganizationNamespace, ProjectNamespace}applySortwith sharedutils.AddRQLSortInQuery+ default sort fallbackschema.RoleGrantRelationNameinstead of relying on DB default for grant relationTest plan
go build ./...passesmake lint-fix— 0 issues