add policies api endpoints #1897
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here: app.greptile.com/review/github.
2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
WalkthroughThe changes introduce a new API group within the backend's bootstrap process, adding endpoints to manage policy data. In the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant R as Router
participant PC as PolicyController
participant DB as Database
C->>R: GET /policy/{policy_type}
R->>PC: Invoke PolicyOrgGetApi
PC->>DB: Query Organization
DB-->>PC: Return organization details or error
PC->>DB: Query Policy by type
DB-->>PC: Return policy details or error
PC-->>C: Return policy details or error response
sequenceDiagram
participant C as Client
participant R as Router
participant PC as PolicyController
participant DB as Database
C->>R: PUT /policy with JSON {policy_type, text}
R->>PC: Invoke PolicyOrgUpsertApi
PC->>DB: Query Organization
DB-->>PC: Return organization details or error
PC->>DB: Check for existing policy
DB-->>PC: Return policy exists status or not found
alt Policy exists
PC->>DB: Update policy with new text
else Policy not found
PC->>DB: Insert new policy entry
end
DB-->>PC: Confirmation or error
PC-->>C: Return success response or error
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (3)
backend/bootstrap/main.go (1)
235-237: Harmonize route structure for policy endpoints.
While using a GET route that includespolicy_typeas a URL path parameter is consistent, the PUT route uses the request body for specifyingpolicy_type. Consider adding/:policy_typeto the PUT route for a more uniform REST design, unless there is a specific reason for this difference.- policyApiGroup.PUT("/", controllers.PolicyOrgUpsertApi) + policyApiGroup.PUT("/:policy_type", controllers.PolicyOrgUpsertApi)backend/controllers/policies_api.go (2)
16-19: Centralize policy type validation for reuse.
InPolicyOrgGetApi, the policy type corresponds only to"plan"or"access". This assumption may appear again inPolicyOrgUpsertApi. Consider creating a helper or constants to define valid policy types, ensuring consistent handling.
53-57: Mirror policy type checks to prevent invalid types.
UnlikePolicyOrgGetApi,PolicyOrgUpsertApidoes not block invalidpolicy_type. To maintain consistency, consider validating against the same set of allowed values ("plan"or"access") when upserting.+ if request.PolicyType != "plan" && request.PolicyType != "access" { + c.JSON(http.StatusBadRequest, gin.H{"error": "invalid policy type requested: " + request.PolicyType}) + return + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/bootstrap/main.go(1 hunks)backend/controllers/policies_api.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
🔇 Additional comments (2)
backend/bootstrap/main.go (1)
234-234: Confirm authorization checks for VCS deletion.
TheDELETE("/:id", controllers.DeleteVCSConnection)endpoint is newly added and presumably protected by internal and header-based auth. Ensure that only authorized roles or accounts can delete VCS connections to prevent inadvertent resource removal.Please confirm that appropriate role-based logic is enforced. If necessary, we can create or extend a permission check to ensure only privileged users can trigger this endpoint.
backend/controllers/policies_api.go (1)
36-48: Confirm correctness of joined query for organization-level policies.
You're usingJoinedOrganisationRepoProjectQuery()to ensurerepos.id IS NULL AND projects.id IS NULLfor pure org-level policies. Confirm that the intended logic is to strictly match org-level policies without any associated repo or project. Otherwise, partial matches or unexpected records might slip through.Do you want a script to search across the codebase for how
JoinedOrganisationRepoProjectQuery()is generally used to confirm that it only retrieves purely org-scoped policies?
| if policyResult.RowsAffected == 0 { | ||
| err := models.DB.GormDB.Create(&models.Policy{ | ||
| OrganisationID: org.ID, | ||
| Type: policyType, | ||
| Policy: policyData, | ||
| }).Error | ||
|
|
||
| if err != nil { | ||
| log.Printf("Error creating policy: %v", err) | ||
| c.String(http.StatusInternalServerError, "Error creating policy") | ||
| return | ||
| } | ||
| } else { | ||
| policy.Policy = policyData | ||
| err := models.DB.GormDB.Save(policy).Error | ||
| if err != nil { | ||
| log.Printf("Error updating policy: %v", err) | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Error updating policy"}) | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent race conditions or duplicates for upserted policies.
Two parallel requests with different payloads might cause conflicting updates. Ensure there's a unique constraint on (organisation_id, repo_id, project_id, type) or wrap this logic into a transactional approach to avoid inconsistent states.
Summary by CodeRabbit