api support for repos #1887
api support for repos #1887
Conversation
|
Warning Rate limit exceeded@motatoes has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 26 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (4)
WalkthroughThis pull request restructures the backend API endpoints. It removes the granular project, activity, and run routes and introduces a conditional API group that provides endpoints for listing repositories and linking GitHub installations when enabled. New middleware is added to validate required headers, and additional JWT context keys are defined. A migration modifies the repos table by adding a VCS column, and the Repo model is updated with a new VCS field and JSON mapping method. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant M as Middleware (HeadersApiAuth)
participant G as GitHub Controller
participant DB as Database
participant R as Response
C->>M: Send request to /api/github/link
M->>G: Pass validated request
G->>DB: Query organization and installation link
DB-->>G: Return organization/link data
G->>DB: Create or verify GitHub installation link
G->>R: Return status (200/400/500)
sequenceDiagram
participant C as Client
participant M as Middleware (HeadersApiAuth)
participant R as Repo Controller
participant DB as Database
C->>M: Send request to /api/repos
M->>R: Forward validated request
R->>DB: Query organization and repository list
DB-->>R: Return associated repositories
R->>C: Respond with JSON list (200)
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This pull request introduces middleware for header validation, modifies JWT middleware, and updates the database schema to support multiple VCS types.
- Added
HeadersApiAuthmiddleware inbackend/middleware/headers.goto validate specific headers and set context parameters. - Introduced
ORGANISATION_SOURCE_KEYandUSER_ID_KEYconstants inbackend/middleware/jwt.gofor enhanced context management. - Added a new
vcscolumn to therepostable inbackend/migrations/20250224152926.sqlto support multiple VCS types. - Updated
backend/models/orgs.goto include aVCSfield in theRepostruct and aMapToJsonStructmethod for JSON serialization.
💡 (3/5) Reply to the bot's comments like "Can you suggest a fix for this @greptileai?" or ask follow-up questions!
5 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile
backend/models/orgs.go
Outdated
| return struct { | ||
| Id uint `json:"id"` | ||
| Name string `json:"name"` | ||
| RepoFullName string `json:"directory"` |
There was a problem hiding this comment.
syntax: RepoFullName should map to repo_full_name
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
backend/controllers/repos.go (1)
21-22: Improve error messageThe error message could be more descriptive by including the organization source.
- c.String(http.StatusNotFound, "Could not find organisation: "+organisationId) + c.String(http.StatusNotFound, "Could not find organisation with ID "+organisationId+" and source "+organisationSource)backend/controllers/github_api.go (2)
46-52: Add context to database operation.The database operation could benefit from context propagation for better request lifecycle management.
Add context to the database operation:
-link, err := models.DB.GetGithubAppInstallationLink(installationId) +link, err := models.DB.GetGithubAppInstallationLink(c.Request.Context(), installationId)
66-71: Add context to database operation and improve error handling.Similar to the previous comment, this database operation could benefit from context propagation. Also, the error response could be more specific.
Add context and improve error handling:
-_, err = models.DB.CreateGithubInstallationLink(&org, installationId) +_, err = models.DB.CreateGithubInstallationLink(c.Request.Context(), &org, installationId) if err != nil { log.Printf("failed to create installation link: %v", err) - c.JSON(http.StatusInternalServerError, gin.H{"status": "failed to create installation link"}) + c.JSON(http.StatusInternalServerError, gin.H{"status": "Failed to create installation link. Please try again later."}) return }backend/bootstrap/main.go (1)
217-226: Consider adding API version prefix.The new API endpoints are added under
/apiwithout version prefix. This could make future API versioning more difficult.Consider adding API version prefix:
if enableApi := os.Getenv("DIGGER_ENABLE_API_ENDPOINTS"); enableApi == "true" { - apiGroup := r.Group("/api") + apiGroup := r.Group("/api/v1") apiGroup.Use(middleware.HeadersApiAuth()) reposApiGroup := apiGroup.Group("/repos") reposApiGroup.GET("/", controllers.ListReposApi) githubApiGroup := apiGroup.Group("/github") githubApiGroup.POST("/link", controllers.LinkGithubInstallationToOrgApi) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
backend/migrations/atlas.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
backend/bootstrap/main.go(1 hunks)backend/controllers/github_api.go(1 hunks)backend/controllers/repos.go(1 hunks)backend/middleware/headers.go(1 hunks)backend/middleware/jwt.go(1 hunks)backend/migrations/20250224152926.sql(1 hunks)backend/models/orgs.go(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/migrations/20250224152926.sql
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
🔇 Additional comments (5)
backend/middleware/jwt.go (1)
311-312: LGTM!The new constants follow the existing naming pattern and are used consistently across the codebase.
backend/middleware/headers.go (1)
8-35: LGTM!The middleware implementation is clean and follows consistent error handling patterns with clear error messages.
backend/models/orgs.go (1)
128-147: LGTM!The
MapToJsonStructmethod follows the established pattern and provides a consistent JSON representation of the Repo entity.backend/controllers/repos.go (1)
12-48: LGTM!The implementation is well-structured with proper error handling and efficient database queries using joins and ordering.
backend/bootstrap/main.go (1)
217-226:❓ Verification inconclusive
Verify the security implications of the new API endpoints.
The new API endpoints are protected by
HeadersApiAuthmiddleware, but we should verify its security measures.Let's check the implementation of the middleware:
🏁 Script executed:
#!/bin/bash # Description: Check the implementation of HeadersApiAuth middleware # and any security-related configurations. # Check middleware implementation rg -A 10 "func HeadersApiAuth" backend/middleware/ # Check for any security-related configurations rg -g '*.go' "DIGGER_ENABLE_API_ENDPOINTS|HeadersApiAuth"Length of output: 1035
Security Validation for New API Endpoints
The new API endpoints are guarded by the
HeadersApiAuthmiddleware. Our verification shows that the middleware checks for the presence of theDIGGER_ORG_IDheader (while also extractingDIGGER_ORG_SOURCEandDIGGER_USER_ID), and returns a bad request ifDIGGER_ORG_IDis missing. Although this provides basic protection, please verify that merely checking for non-empty header values meets our security requirements. In particular, consider whether:
- The minimal header presence check is sufficient to prevent spoofed requests.
- Additional validations (e.g., verifying
DIGGER_USER_IDor enforcing robust authentication tokens) should be implemented.- Any further security measures or documentation are warranted based on our threat model.
backend/middleware/headers.go
Outdated
| return | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary return and add c.Next()
The return statement is unnecessary here, and c.Next() should be called to continue the middleware chain.
- return
+ c.Next()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return | |
| } | |
| c.Next() | |
| } |
backend/models/orgs.go
Outdated
| return struct { | ||
| Id uint `json:"id"` | ||
| Name string `json:"name"` | ||
| RepoFullName string `json:"directory"` |
There was a problem hiding this comment.
Fix incorrect JSON field mapping
The RepoFullName field is incorrectly mapped to "directory" in the JSON output. This seems to be a copy-paste error.
- RepoFullName string `json:"directory"`
+ RepoFullName string `json:"repo_full_name"`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| RepoFullName string `json:"directory"` | |
| RepoFullName string `json:"repo_full_name"` |
backend/controllers/repos.go
Outdated
| if err != nil { | ||
| c.String(http.StatusInternalServerError, "Unknown error occurred while marshalling response") | ||
| return | ||
| } |
There was a problem hiding this comment.
Remove undefined error check
This error check is using an undefined error variable. The marshalling operation doesn't return an error, so this check is unnecessary.
- if err != nil {
- c.String(http.StatusInternalServerError, "Unknown error occurred while marshalling response")
- return
- }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err != nil { | |
| c.String(http.StatusInternalServerError, "Unknown error occurred while marshalling response") | |
| return | |
| } |
backend/controllers/github_api.go
Outdated
| if err != nil { | ||
| log.Printf("Error binding JSON: %v", err) | ||
| return | ||
| } |
There was a problem hiding this comment.
Add error response for JSON binding failure.
The function returns without sending an error response when JSON binding fails. This could leave clients waiting for a response.
Add an error response:
err := c.BindJSON(&request)
if err != nil {
log.Printf("Error binding JSON: %v", err)
+ c.JSON(http.StatusBadRequest, gin.H{"status": "Invalid request format"})
return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err != nil { | |
| log.Printf("Error binding JSON: %v", err) | |
| return | |
| } | |
| if err != nil { | |
| log.Printf("Error binding JSON: %v", err) | |
| c.JSON(http.StatusBadRequest, gin.H{"status": "Invalid request format"}) | |
| return | |
| } |
backend/controllers/github_api.go
Outdated
| if err != nil { | ||
| if errors.Is(err, gorm.ErrRecordNotFound) { | ||
| log.Printf("could not find organisation: %v", err) | ||
| c.JSON(http.StatusNotFound, gin.H{"status": "Could not find organisation: " + organisationId}) | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
Improve error handling for database query.
The function returns without sending an error response for non-"record not found" errors. Also, the error log and response could be more descriptive.
Add proper error handling:
err = models.DB.GormDB.Where("external_id = ? AND external_source = ?", organisationId, organisationSource).First(&org).Error
if err != nil {
if errors.Is(err, gorm.ErrRecordNotFound) {
log.Printf("could not find organisation: %v", err)
c.JSON(http.StatusNotFound, gin.H{"status": "Could not find organisation: " + organisationId})
+ } else {
+ log.Printf("database error while finding organisation: %v", err)
+ c.JSON(http.StatusInternalServerError, gin.H{"status": "Internal server error"})
}
return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err != nil { | |
| if errors.Is(err, gorm.ErrRecordNotFound) { | |
| log.Printf("could not find organisation: %v", err) | |
| c.JSON(http.StatusNotFound, gin.H{"status": "Could not find organisation: " + organisationId}) | |
| } | |
| return | |
| } | |
| err = models.DB.GormDB.Where("external_id = ? AND external_source = ?", organisationId, organisationSource).First(&org).Error | |
| if err != nil { | |
| if errors.Is(err, gorm.ErrRecordNotFound) { | |
| log.Printf("could not find organisation: %v", err) | |
| c.JSON(http.StatusNotFound, gin.H{"status": "Could not find organisation: " + organisationId}) | |
| } else { | |
| log.Printf("database error while finding organisation: %v", err) | |
| c.JSON(http.StatusInternalServerError, gin.H{"status": "Internal server error"}) | |
| } | |
| return | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/models/orgs.go (1)
140-140: Consider usingNameinstead ofRepoNamefor consistency.The
Namefield in the JSON output is mapped toRepoName, which might be confusing since the struct already has aNamefield. Consider using theNamefield directly for consistency.Apply this diff to improve consistency:
- Name: r.RepoName, + Name: r.Name,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/models/orgs.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
🔇 Additional comments (1)
backend/models/orgs.go (1)
23-23:✅ Verification successful
Verify the
DiggerVCSTypetype definition.The
DiggerVCSTypetype used in theVCSfield is not defined in this file. Please ensure it's properly defined and imported.Run the following script to locate the type definition:
🏁 Script executed:
#!/bin/bash # Description: Search for the DiggerVCSType definition in the codebase. # Search for type definition ast-grep --pattern $'type DiggerVCSType' # Search for any usage of DiggerVCSType rg -A 5 $'DiggerVCSType'Length of output: 4882
DiggerVCSType type verification complete
We confirmed that the
DiggerVCSTypetype is correctly defined inbackend/models/scheduler.goas a string alias—accompanied by its constants (e.g.,DiggerVCSGithub,DiggerVCSGitlab). Sincebackend/models/orgs.goandbackend/models/scheduler.gobelong to the same package, no additional import changes are necessary. The default value ('github') in theVCSfield correctly aligns with the defined constant, so no further action is needed.
backend/models/orgs.go
Outdated
| RepoUrl: r.RepoUrl, | ||
| VCS: string(r.VCS), | ||
| OrganisationID: r.OrganisationID, | ||
| OrganisationName: r.Organisation.Name, |
There was a problem hiding this comment.
Add nil check for Organisation field.
Accessing r.Organisation.Name could cause a panic if Organisation is nil. Add a nil check to handle this case gracefully.
Apply this diff to add the nil check:
- OrganisationName: r.Organisation.Name,
+ OrganisationName: func() string {
+ if r.Organisation == nil {
+ return ""
+ }
+ return r.Organisation.Name
+ }(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| OrganisationName: r.Organisation.Name, | |
| OrganisationName: func() string { | |
| if r.Organisation == nil { | |
| return "" | |
| } | |
| return r.Organisation.Name | |
| }(), |
Summary by CodeRabbit
New Features
Chores