feat/internal apis for users #1885
Conversation
WalkthroughThe pull request introduces conditional logic for internal API endpoints in Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant M as InternalApiAuth Middleware
participant D as DiggerController
participant DB as Database
C->>M: POST /_internal/api/create_user
M-->>C: Proceed if authenticated
M->>D: Forward request
D->>DB: GetOrganisation(externalOrgId)
DB-->>D: Organisation data or error
D->>DB: CreateUser(email, externalId, orgId, username)
DB-->>D: New User record or error
D->>C: Respond with JSON { user_id }
sequenceDiagram
participant C as Client
participant M as InternalApiAuth Middleware
participant D as DiggerController
participant DB as Database
C->>M: POST /_internal/api/upsert_org
M-->>C: Proceed if authenticated
M->>D: Forward request
D->>DB: CreateOrganisation(OrgCreateRequest)
DB-->>D: New Organisation record or error
D->>C: Respond with JSON { org_id }
Possibly related PRs
Poem
✨ Finishing Touches
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 PR introduces internal API endpoints for user and organization management, adding necessary database schema changes and controller implementations for user creation and organization management.
- Added new
CreateUserInternalandCreateOrgInternalendpoints in/backend/controllers/internal_users.gowith basic auth protection - Modified User model in
/backend/models/user.goto includeEmail,ExternalId, andOrgIdfields - Added migration
20250220084846.sqlwith type mismatch betweenorg_id(bigint) and model'sOrgId(*uint) - Renamed
WebhookAuthtoInternalApiAuthin/backend/middleware/webhook.gowith bearer token validation - Missing input validation and error handling in controller endpoints (e.g., email format, required fields)
💡 (2/5) Greptile learns from your feedback when you react with 👍/👎!
7 file(s) reviewed, 6 comment(s)
Edit PR Review Bot Settings | Greptile
backend/bootstrap/main.go
Outdated
| @@ -222,7 +222,9 @@ func Bootstrap(templates embed.FS, diggerController controllers.DiggerController | |||
| runsApiGroup.POST("/:run_id/approve", controllers.ApproveRun) | |||
|
|
|||
| // internal endpoints not meant to be exposed to public and protected behing webhook secret | |||
There was a problem hiding this comment.
syntax: 'behing' is misspelled in the comment
| // internal endpoints not meant to be exposed to public and protected behing webhook secret | |
| // internal endpoints not meant to be exposed to public and protected behind webhook secret |
backend/models/user.go
Outdated
| @@ -4,5 +4,8 @@ import "gorm.io/gorm" | |||
|
|
|||
| type User struct { | |||
| gorm.Model | |||
| Username string `gorm:"uniqueIndex:idx_user"` | |||
| Email string | |||
| ExternalId string | |||
There was a problem hiding this comment.
style: ExternalId should likely have an index for faster lookups since it's used for external identity mapping
![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 (4)
backend/models/user.go (1)
7-10: Consider adding field validation and documentation.The new fields look good, but consider adding:
- Field validation tags for email format.
- Field documentation to describe the purpose of each field.
- JSON tags for API serialization.
Apply this diff to improve the field definitions:
- Email string - ExternalId string - OrgId *uint - Username string `gorm:"uniqueIndex:idx_user"` + // Email is the user's primary email address + Email string `gorm:"not null" json:"email" validate:"required,email"` + // ExternalId is the user's ID from the external authentication system + ExternalId string `gorm:"not null" json:"external_id" validate:"required"` + // OrgId is the optional ID of the organization this user belongs to + OrgId *uint `json:"org_id"` + // Username is a unique identifier for the user + Username string `gorm:"uniqueIndex:idx_user" json:"username" validate:"required"`backend/controllers/internal_users.go (1)
10-38: Enhance error handling and input validation.The function logic is correct, but consider these improvements:
- Add input validation for required fields.
- Return appropriate HTTP status codes (e.g., 400 for invalid input).
- Add structured logging with request context.
Apply this diff to improve the implementation:
func (d DiggerController) CreateOrgInternal(c *gin.Context) { type OrgCreateRequest struct { - Name string `json:"org_name"` - ExternalSource string `json:"external_source"` - ExternalId string `json:"external_id"` + Name string `json:"org_name" binding:"required"` + ExternalSource string `json:"external_source" binding:"required"` + ExternalId string `json:"external_id" binding:"required"` } var request OrgCreateRequest err := c.BindJSON(&request) if err != nil { - log.Printf("Error binding JSON: %v", err) - c.JSON(http.StatusInternalServerError, gin.H{"error": "Error binding JSON"}) + log.Printf("Invalid request payload: %v", err) + c.JSON(http.StatusBadRequest, gin.H{"error": "Invalid request payload"}) return } name := request.Name externalSource := request.ExternalSource externalId := request.ExternalId - log.Printf("creating org for %v %v %v", name, externalSource, externalId) + log.Printf("Creating organization - name: %v, source: %v, id: %v", name, externalSource, externalId) org, err := models.DB.CreateOrganisation(name, externalSource, externalId) if err != nil { log.Printf("Error creating org: %v", err) c.JSON(http.StatusInternalServerError, gin.H{"error": "Error creating org"}) return } - c.JSON(200, gin.H{"status": "success", "org_id": org.ID}) + c.JSON(http.StatusCreated, gin.H{"status": "success", "org_id": org.ID}) }backend/models/storage.go (1)
1035-1044: Enhance error handling and add input validation.The function logic is correct, but consider these improvements:
- Add input validation for required fields.
- Check for duplicate email/username before saving.
- Add structured logging with request context.
Apply this diff to improve the implementation:
func (db *Database) CreateUser(email string, externalId string, orgId uint, username string) (*User, error) { + // Validate input + if email == "" || externalId == "" || username == "" { + return nil, fmt.Errorf("email, externalId, and username are required") + } + + // Check for existing user + var existingUser User + if err := db.GormDB.Where("email = ? OR username = ?", email, username).First(&existingUser).Error; err == nil { + return nil, fmt.Errorf("user with email %s or username %s already exists", email, username) + } else if !errors.Is(err, gorm.ErrRecordNotFound) { + return nil, fmt.Errorf("error checking for existing user: %v", err) + } + user := &User{Email: email, ExternalId: externalId, OrgId: &orgId, Username: username} result := db.GormDB.Save(user) if result.Error != nil { - log.Printf("Failed to create user: %v, error: %v\n", externalId, result.Error) + log.Printf("Failed to create user - email: %v, externalId: %v, error: %v\n", email, externalId, result.Error) return nil, result.Error } - log.Printf("User (id: %v) has been created successfully\n", externalId) + log.Printf("User created successfully - id: %v, email: %v, externalId: %v\n", user.ID, email, externalId) return user, nil }backend/migrations/20250220084846.sql (1)
1-3: Review Migration Syntax and Best Practices.
The migration correctly adds the new nullable columns"email","external_id", and"org_id"to the"public"."users"table. Consider adding inline comments or documentation notes to explain the purpose of these fields for future maintainability. Additionally, if rollback support is required in your deployment process, you might want to create a corresponding down migration. For large tables in production, also verify that the single ALTER TABLE statement does not introduce extended lock times.
📜 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 (6)
backend/bootstrap/main.go(1 hunks)backend/controllers/internal_users.go(1 hunks)backend/middleware/webhook.go(1 hunks)backend/migrations/20250220084846.sql(1 hunks)backend/models/storage.go(1 hunks)backend/models/user.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/middleware/webhook.go
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
🔇 Additional comments (1)
backend/bootstrap/main.go (1)
225-227: LGTM! Consistent use of internal API authentication.The endpoints are correctly configured with the
InternalApiAuthmiddleware, ensuring consistent authentication across internal APIs.
| func (d DiggerController) CreateUserInternal(c *gin.Context) { | ||
| type UserCreateRequest struct { | ||
| UserExternalId string `json:"external_id"` | ||
| UserEmail string `json:"email"` | ||
| OrgExternalId string `json:"external_org_id"` | ||
| } | ||
|
|
||
| var request UserCreateRequest | ||
| err := c.BindJSON(&request) | ||
| if err != nil { | ||
| log.Printf("Error binding JSON: %v", err) | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Error binding JSON"}) | ||
| return | ||
| } | ||
|
|
||
| extUserId := request.UserExternalId | ||
| userEmail := request.UserEmail | ||
| externalOrgId := request.OrgExternalId | ||
|
|
||
| org, err := models.DB.GetOrganisation(externalOrgId) | ||
| if err != nil { | ||
| log.Printf("Error retriving org: %v", err) | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Error retriving org"}) | ||
| return | ||
| } | ||
|
|
||
| // for now using email for username since we want to deprecate that field | ||
| username := userEmail | ||
| user, err := models.DB.CreateUser(userEmail, extUserId, org.ID, username) | ||
| if err != nil { | ||
| log.Printf("Error creating user: %v", err) | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Error creating user"}) | ||
| return | ||
| } | ||
|
|
||
| c.JSON(200, gin.H{"status": "success", "user_id": user.ID}) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and reconsider username strategy.
The function logic is correct, but consider these improvements:
- Add input validation for required fields.
- Return appropriate HTTP status codes.
- Add structured logging.
- Reconsider using email as username, as it might need to change if the user's email changes.
Apply this diff to improve the implementation:
func (d DiggerController) CreateUserInternal(c *gin.Context) {
type UserCreateRequest struct {
- UserExternalId string `json:"external_id"`
- UserEmail string `json:"email"`
- OrgExternalId string `json:"external_org_id"`
+ UserExternalId string `json:"external_id" binding:"required"`
+ UserEmail string `json:"email" binding:"required,email"`
+ OrgExternalId string `json:"external_org_id" binding:"required"`
}
var request UserCreateRequest
err := c.BindJSON(&request)
if err != nil {
- log.Printf("Error binding JSON: %v", err)
- c.JSON(http.StatusInternalServerError, gin.H{"error": "Error binding JSON"})
+ log.Printf("Invalid request payload: %v", err)
+ c.JSON(http.StatusBadRequest, gin.H{"error": "Invalid request payload"})
return
}
extUserId := request.UserExternalId
userEmail := request.UserEmail
externalOrgId := request.OrgExternalId
org, err := models.DB.GetOrganisation(externalOrgId)
if err != nil {
- log.Printf("Error retriving org: %v", err)
- c.JSON(http.StatusInternalServerError, gin.H{"error": "Error retriving org"})
+ log.Printf("Error retrieving org: %v", err)
+ c.JSON(http.StatusInternalServerError, gin.H{"error": "Error retrieving org"})
return
}
+ if org == nil {
+ log.Printf("Organization not found: %v", externalOrgId)
+ c.JSON(http.StatusNotFound, gin.H{"error": "Organization not found"})
+ return
+ }
- // for now using email for username since we want to deprecate that field
- username := userEmail
+ // Generate a unique username based on external ID
+ username := fmt.Sprintf("user_%s", extUserId)
user, err := models.DB.CreateUser(userEmail, extUserId, org.ID, username)
if err != nil {
log.Printf("Error creating user: %v", err)
c.JSON(http.StatusInternalServerError, gin.H{"error": "Error creating user"})
return
}
- c.JSON(200, gin.H{"status": "success", "user_id": user.ID})
+ c.JSON(http.StatusCreated, gin.H{"status": "success", "user_id": user.ID})
}📝 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.
| func (d DiggerController) CreateUserInternal(c *gin.Context) { | |
| type UserCreateRequest struct { | |
| UserExternalId string `json:"external_id"` | |
| UserEmail string `json:"email"` | |
| OrgExternalId string `json:"external_org_id"` | |
| } | |
| var request UserCreateRequest | |
| err := c.BindJSON(&request) | |
| if err != nil { | |
| log.Printf("Error binding JSON: %v", err) | |
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Error binding JSON"}) | |
| return | |
| } | |
| extUserId := request.UserExternalId | |
| userEmail := request.UserEmail | |
| externalOrgId := request.OrgExternalId | |
| org, err := models.DB.GetOrganisation(externalOrgId) | |
| if err != nil { | |
| log.Printf("Error retriving org: %v", err) | |
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Error retriving org"}) | |
| return | |
| } | |
| // for now using email for username since we want to deprecate that field | |
| username := userEmail | |
| user, err := models.DB.CreateUser(userEmail, extUserId, org.ID, username) | |
| if err != nil { | |
| log.Printf("Error creating user: %v", err) | |
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Error creating user"}) | |
| return | |
| } | |
| c.JSON(200, gin.H{"status": "success", "user_id": user.ID}) | |
| } | |
| func (d DiggerController) CreateUserInternal(c *gin.Context) { | |
| type UserCreateRequest struct { | |
| UserExternalId string `json:"external_id" binding:"required"` | |
| UserEmail string `json:"email" binding:"required,email"` | |
| OrgExternalId string `json:"external_org_id" binding:"required"` | |
| } | |
| var request UserCreateRequest | |
| err := c.BindJSON(&request) | |
| if err != nil { | |
| log.Printf("Invalid request payload: %v", err) | |
| c.JSON(http.StatusBadRequest, gin.H{"error": "Invalid request payload"}) | |
| return | |
| } | |
| extUserId := request.UserExternalId | |
| userEmail := request.UserEmail | |
| externalOrgId := request.OrgExternalId | |
| org, err := models.DB.GetOrganisation(externalOrgId) | |
| if err != nil { | |
| log.Printf("Error retrieving org: %v", err) | |
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Error retrieving org"}) | |
| return | |
| } | |
| if org == nil { | |
| log.Printf("Organization not found: %v", externalOrgId) | |
| c.JSON(http.StatusNotFound, gin.H{"error": "Organization not found"}) | |
| return | |
| } | |
| // Generate a unique username based on external ID | |
| username := fmt.Sprintf("user_%s", extUserId) | |
| user, err := models.DB.CreateUser(userEmail, extUserId, org.ID, username) | |
| if err != nil { | |
| log.Printf("Error creating user: %v", err) | |
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Error creating user"}) | |
| return | |
| } | |
| c.JSON(http.StatusCreated, gin.H{"status": "success", "user_id": user.ID}) | |
| } |
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
backend/models/user.go (3)
7-7: Add email format validation.While the unique index is good, consider adding validation for email format using a validator tag.
- Email string `gorm:"uniqueIndex"` + Email string `gorm:"uniqueIndex" validate:"required,email"`
9-9: Add foreign key constraint for OrgId.Consider adding a foreign key constraint to ensure referential integrity with the organizations table.
- OrgId *uint + OrgId *uint `gorm:"foreignKey:id;references:organizations"`
10-10: Use more specific index name.The index name "idx_user" is too generic. Consider using a more specific name that indicates the indexed field.
- Username string `gorm:"uniqueIndex:idx_user"` + Username string `gorm:"uniqueIndex:idx_username"`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/controllers/internal_users.go(1 hunks)backend/models/user.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/controllers/internal_users.go
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
🔇 Additional comments (1)
backend/models/user.go (1)
8-8: Add index to ExternalId field for faster lookups.Since ExternalId is used for external identity mapping, it should be indexed for better query performance.
- ExternalId string + ExternalId string `gorm:"index"`
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/controllers/internal_users.go (1)
63-65:⚠️ Potential issueAdd a return statement after handling organization retrieval error.
If there's an error retrieving the organization, the function continues to create a user with a potentially nil
org, which can cause undefined behavior or runtime panics.if err != nil { log.Printf("Error retrieving org: %v", err) c.JSON(http.StatusInternalServerError, gin.H{"error": "Error retrieving org"}) + return }
🧹 Nitpick comments (2)
backend/controllers/internal_users.go (2)
10-38: Consider returning 201 status and adding optional request validation.While the logic here seems solid, returning
http.StatusCreatedwould be more appropriate for newly created resources, rather thanhttp.StatusOK. Additionally, consider validating the request fields (e.g., ensuringorg_nameisn't empty) to prevent downstream issues.
76-76: Return 201 for successful resource creation.When creating a new user, return a 201 status to reflect that a new resource has been created:
- c.JSON(200, gin.H{"status": "success", "user_id": user.ID}) + c.JSON(http.StatusCreated, gin.H{"status": "success", "user_id": user.ID})
📜 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 (10)
backend/bootstrap/main.go(1 hunks)backend/controllers/internal_users.go(1 hunks)backend/migrations/20250220172054.sql(1 hunks)backend/migrations/20250220172321.sql(1 hunks)backend/migrations/20250220173053.sql(1 hunks)backend/migrations/20250220173439.sql(1 hunks)backend/migrations/20250221044813.sql(1 hunks)backend/models/orgs.go(1 hunks)backend/models/storage.go(1 hunks)backend/models/user.go(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- backend/migrations/20250220173053.sql
- backend/migrations/20250220172321.sql
- backend/migrations/20250221044813.sql
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/bootstrap/main.go
- backend/models/storage.go
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
🔇 Additional comments (6)
backend/controllers/internal_users.go (1)
49-53: Use 400 Bad Request instead of 500 Internal Server Error for JSON binding failures.Similar to a previously noted comment, responding with 500 for a client-side payload error can be misleading. Use a 4xx status code to indicate request errors, for example:
- c.JSON(http.StatusInternalServerError, gin.H{"error": "Error binding JSON"}) + c.JSON(http.StatusBadRequest, gin.H{"error": "Invalid request payload"})backend/models/user.go (1)
7-13: Looks great.All newly added fields and unique constraints appear correctly set up for typical usage.
backend/models/orgs.go (1)
10-10:❓ Verification inconclusive
Verify removal of the unique constraint on organisation name.
Changing this to a non-unique index allows multiple orgs to have the same name, which could be intentional or could introduce confusion. Please confirm this design choice.
🏁 Script executed:
#!/bin/bash # Searching for references to organisation indexing in the codebase to see if anything relies on uniqueness rg "Index:idx_organisation|uniqueIndex:idx_organisation" -A 3Length of output: 340
Design Confirmation on Organisation Name Unique Constraint
- The output confirms that the
Namefield now uses a non-unique index:Name string `gorm:"Index:idx_organisation"`- Unlike the external source fields, which continue to use unique constraints via
uniqueIndex:idx_external_source, the unique constraint for the organisation name has been removed.- Please confirm that allowing duplicate organisation names is an intentional design decision.
backend/migrations/20250220172054.sql (1)
1-2: SQL Migration: Adding External Source ColumnThe migration correctly adds an
external_sourcecolumn of typetextto the"public"."users"table, allowing for null values. Verify that the data type is appropriate for the external data you expect and that the application layer (models, controllers) is updated to handle this new field.backend/migrations/20250220173439.sql (2)
1-2: SQL Migration: Renaming ColumnRenaming
"org_id"to"organisation_id"enhances clarity and consistency. Ensure that this change is fully propagated throughout the codebase, including model definitions and any SQL queries that reference this column.
3-4: SQL Migration: Adding Foreign Key ConstraintAdding the foreign key constraint
"fk_users_organisation"on"organisation_id"is a good step for maintaining referential integrity with the"public"."organisations"table. Please verify that:
- The
"organisations"table exists with an indexed and primary key"id".- The chosen referential actions (
ON UPDATE NO ACTION ON DELETE NO ACTION) align with your application's data handling and integrity needs.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
backend/controllers/internal_users.go (2)
61-64:⚠️ Potential issueFix incorrect error status code.
JSON binding errors should return 400 Bad Request, not 500 Internal Server Error.
Apply this diff to fix the status code:
if err != nil { log.Printf("Error binding JSON: %v", err) - c.JSON(http.StatusInternalServerError, gin.H{"error": "Error binding JSON"}) + c.JSON(http.StatusBadRequest, gin.H{"error": "Error binding JSON"}) return }
72-76:⚠️ Potential issueAdd missing return statement and nil check.
The function continues execution after returning an error response, and it doesn't check if the organization exists.
Apply this diff to fix the error handling:
org, err := models.DB.GetOrganisation(externalOrgId) if err != nil { log.Printf("Error retrieving org: %v", err) c.JSON(http.StatusInternalServerError, gin.H{"error": "Error retrieving org"}) + return + } + if org == nil { + log.Printf("Organization not found: %v", externalOrgId) + c.JSON(http.StatusNotFound, gin.H{"error": "Organization not found"}) + 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/internal_users.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build
- GitHub Check: Build
🔇 Additional comments (2)
backend/controllers/internal_users.go (1)
87-87: Use http.StatusOK instead of hard-coded status code.For consistency and maintainability, use the standard HTTP status constants.
Apply this diff to use the constant:
- c.JSON(200, gin.H{"status": "success", "user_id": user.ID}) + c.JSON(http.StatusOK, gin.H{"status": "success", "user_id": user.ID})backend/bootstrap/main.go (1)
224-229: LGTM! The internal endpoints implementation is secure and well-structured.The implementation:
- Uses environment variable to control access to internal endpoints
- Consistently applies
InternalApiAuthmiddleware- Groups related internal endpoints together
| func (d DiggerController) UpsertOrgInternal(c *gin.Context) { | ||
| type OrgCreateRequest struct { | ||
| Name string `json:"org_name"` | ||
| ExternalSource string `json:"external_source"` | ||
| ExternalId string `json:"external_id"` | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for required fields.
The request structure should enforce required fields using binding tags to ensure data integrity.
Apply this diff to add input validation:
type OrgCreateRequest struct {
- Name string `json:"org_name"`
- ExternalSource string `json:"external_source"`
- ExternalId string `json:"external_id"`
+ Name string `json:"org_name" binding:"required"`
+ ExternalSource string `json:"external_source" binding:"required"`
+ ExternalId string `json:"external_id" binding:"required"`
}📝 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.
| func (d DiggerController) UpsertOrgInternal(c *gin.Context) { | |
| type OrgCreateRequest struct { | |
| Name string `json:"org_name"` | |
| ExternalSource string `json:"external_source"` | |
| ExternalId string `json:"external_id"` | |
| } | |
| func (d DiggerController) UpsertOrgInternal(c *gin.Context) { | |
| type OrgCreateRequest struct { | |
| Name string `json:"org_name" binding:"required"` | |
| ExternalSource string `json:"external_source" binding:"required"` | |
| ExternalId string `json:"external_id" binding:"required"` | |
| } | |
| // Rest of the implementation... | |
| } |
| log.Printf("Error while retriving org: %v", err) | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Error creating org"}) | ||
| return |
There was a problem hiding this comment.
Fix error message inconsistency.
The error message doesn't match the actual error scenario. The message says "Error creating org" but the error occurs while retrieving the organization.
Apply this diff to fix the error message:
- log.Printf("Error while retriving org: %v", err)
- c.JSON(http.StatusInternalServerError, gin.H{"error": "Error creating org"})
+ log.Printf("Error while retrieving org: %v", err)
+ c.JSON(http.StatusInternalServerError, gin.H{"error": "Error retrieving org"})📝 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.
| log.Printf("Error while retriving org: %v", err) | |
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Error creating org"}) | |
| return | |
| log.Printf("Error while retrieving org: %v", err) | |
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Error retrieving org"}) | |
| return |
Summary by CodeRabbit