TT-13870 Fix POST update

[tbuchaillot]

PR Type

Bug fix, Enhancement

---

Description

• Fix issue with POST update requests for existing records

• Add check for record existence before update or create

• Return appropriate errors for missing records or permission issues

• Distinguish between create and update operations based on primary key

---

Changes walkthrough 📝

	Relevant files
Bug fix	crud.go Fix POST update behavior and add existence checks                resource/crud.go Fix issue with POST update requests for existing records Add check for record existence before update or create Return appropriate errors for missing records or permission issues Distinguish between create and update operations based on primary key and HTTP method +43/-5

---

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Error handling The new code introduces several error cases, such as missing records or permission issues. The reviewer should validate that these errors are handled correctly and appropriate error messages are returned to the client. Test various scenarios to ensure proper error handling. if !res.HasPermission(roles.Create, context) { return roles.ErrPermissionDenied } return context.GetDB().Create(result).Error } // If we have a non-zero primary key, first check if it exists var count int primaryField := scope.PrimaryField() if primaryField == nil { return fmt.Errorf("no primary key field found") } query := fmt.Sprintf("%v = ?", scope.Quote(primaryField.DBName)) if err := context.GetDB().Model(result).Where(query, scope.PrimaryKeyValue()).Count(&count).Error; err != nil { return err } // For creation attempts with existing ID if context.Request != nil && context.Request.Method == "POST" { if count > 0 { return fmt.Errorf("record with primary key %v already exists", scope.PrimaryKeyValue()) } if !res.HasPermission(roles.Create, context) { return roles.ErrPermissionDenied } return context.GetDB().Create(result).Error } // Update operation if !res.HasPermission(roles.Update, context) { return roles.ErrPermissionDenied } if count == 0 { return fmt.Errorf("record with primary key %v not found", scope.PrimaryKeyValue()) } DB queries The changes involve additional database queries to check for record existence. Review the performance impact of these queries, especially in high-load scenarios. Consider adding indexes if necessary to optimize the queries. var count int primaryField := scope.PrimaryField() if primaryField == nil { return fmt.Errorf("no primary key field found") } query := fmt.Sprintf("%v = ?", scope.Quote(primaryField.DBName)) if err := context.GetDB().Model(result).Where(query, scope.PrimaryKeyValue()).Count(&count).Error; err != nil { return err }

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Security	Improve error handling when checking record existence to avoid leaking sensitive details Ensure proper error handling when checking if a record with the given primary key exists. The current code directly returns the error from the database query, which could leak sensitive database details. Instead, return a generic error message to the client. resource/crud.go [158-160] if err := context.GetDB().Model(result).Where(query, scope.PrimaryKeyValue()).Count(&count).Error; err != nil { - return err + return fmt.Errorf("failed to check if record exists") } Suggestion importance[1-10]: 8 Why: Properly handling errors and avoiding leaking sensitive database details is important for security. The suggestion provides a good way to return a generic error message instead.	8
General	Refactor duplicated create operation logic into a separate function for better code reuse Extract the duplicated permission check and error handling logic for the create operation into a separate function. This will make the code more DRY and easier to maintain. resource/crud.go [163-170] if context.Request != nil && context.Request.Method == "POST" { if count > 0 { return fmt.Errorf("record with primary key %v already exists", scope.PrimaryKeyValue()) } + return res.createRecord(result, context) +} + +// In a separate function: +func (res *Resource) createRecord(result interface{}, context *qor.Context) error { if !res.HasPermission(roles.Create, context) { return roles.ErrPermissionDenied } return context.GetDB().Create(result).Error } Suggestion importance[1-10]: 5 Why: Extracting the duplicated permission check and create logic into a separate function can improve code reuse and maintainability. However, the impact is moderate as it's more of a refactoring than fixing a critical issue.	5