refactor: update database schema to include review and comment tables #418
Conversation
- Created a new Review struct to manage note verification independently - Removed Verified field from Note model - Added unique foreign key relationship between Note and Review - Implemented flexible review status tracking with timestamps
- Implemented methods for retrieving, saving, updating, and deleting note reviews - Added methods for managing note comments with full CRUD functionality - Updated query methods to preload reviews and comments with consistent ordering - Introduced virtual Verified field population for notes - Moved database management utility functions to a new manage.go file - Updated auto-migration to include new NoteReview and NoteComment models
…onfiguration - Implemented comprehensive backup mechanism for SQLite database - Added disk space and permission checks before creating backups - Improved database initialization with more robust pragma configuration - Simplified database connection and close methods - Enhanced error handling and logging for database operations
- Split Note model's comment and review fields into new NoteReview and NoteComment models - Added one-to-one and one-to-many relationships for reviews and comments - Introduced a virtual Verified field to maintain template compatibility - Updated model structure to improve data organization and relationship management
- Refactored ReviewDetection handler to support separate comment and review updates - Added support for creating, updating, and retrieving note comments - Improved handling of species ignore list updates during review - Updated review modal to display existing comments - Enhanced error handling and SSE notifications for review and comment operations
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces significant enhancements to the Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ 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 (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
internal/datastore/interfaces.go(7 hunks)internal/datastore/manage.go(1 hunks)internal/datastore/model.go(2 hunks)internal/datastore/sqlite.go(2 hunks)internal/httpcontroller/handlers/detections.go(2 hunks)views/elements/reviewModal.html(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (40)
internal/datastore/sqlite.go (19)
5-12: Imports look good
No major issues spotted in the newly added imports.
17-20: Doc comment updated
Stating explicitly thatSQLiteStoreimplementsStoreInterfaceis clearer than referencingDataStore.
29-33: Skipping backup for non-existent database
Returning early when the database file does not exist avoids unnecessary backup operations.
36-39: Robust error handling
Terminating on stat failure ensures we don't attempt backing up a file with invalid metadata.
42-53: Disk space validation
Proactively checking space helps prevent partial backups or silent failures.
56-59: Write permission check
Verifies the backup directory is writable before proceeding, which is good for early error detection.
62-65: Timestamped backup path
Appending a formatted timestamp is a clean approach to organizing multiple backups.
66-71: Source file opening
Clearly handles file-open errors, preventing further steps on invalid file descriptors.
73-79: Creating the backup file
Properly rolls back on creation failure with immediate error returns.
80-82: File copy operation
Usingio.Copyis a straightforward method for duplicating the database contents.
85-86: Backup success logging
Logging the newly created backup path aids in both debugging and auditing.
89-98: Creating database directory
Ensuring the directory path exists prevents the open call from failing on missing parent folders.
99-101: Custom GORM logger
Assigning a dedicated logger provides flexibility in log formatting and verbosity.
102-109: SQLite database opening
Handles errors gracefully by returning a wrapped error message about the DB initialization failure.
110-113: *Retrieving sql.DB
Exposes lower-level DB settings and connection properties if needed.
116-129: SQLite PRAGMA configurations
These performance-optimized settings can boost throughput, but consider reliability implications of in-memory journaling.Are you certain these PRAMA settings provide the desired balance of speed and data integrity for your environment?
131-133: Assigning GORM instance
Preserving thegorm.DBreference for future use withinSQLiteStore.
134-136: Auto-migration
Automatically brings the schema in sync with new or modified models.
142-152: Proper database closure
Accounts for the absence of a DB handle and neatly closes the connection if it exists.internal/datastore/interfaces.go (13)
35-40: New interface methods for note reviews and comments
Helps manage review statuses and user feedback in a more structured way.
163-174: Preloading review & comments in Get()
Unifies retrieval of aNotealong with itsReviewandComments. PopulatingVerifiedis straightforward.
Line range hint
349-370: SpeciesDetections preload
Ensures each detection has its associatedReviewandCommentsfor comprehensive data in one query.
380-392: GetLastDetections
Consistent approach to preloading related data. IntegratingVerifiedlogic keeps note objects self-contained.
416-430: SearchNotes
Enables searching bycommon_nameorscientific_name, while still fetchingReviewandComments.
573-595: GetNoteReview
Returnsnilif no review exists, which is a clean way to distinguish unreviewed notes.
596-609: SaveNoteReview
Upsert approach is practical, letting you seamlessly create or update an existing review record.
611-625: GetNoteComments
Returns comments ordered by creation time. Straightforward for retrieving discussion history.
626-633: SaveNoteComment
Standard insertion logic handles new comment creation with minimal overhead.
634-646: DeleteNoteComment
Removes a comment directly by ID, with clear error handling if the ID is invalid.
647-669: UpdateNoteComment
Updates the comment's text and timestamps the edit. A conventional approach.
671-679: getHourRange utility
Neatly calculates start/end times accounting for day wrap-around using modulo arithmetic.
680-686: sortAscendingString utility
Logical helper returning "ASC" or "DESC" for queries based on a boolean.internal/datastore/manage.go (2)
13-23: GORM logger creation
Sets a warning threshold for slow queries and levels. Keeps logs more readable.
25-38: Auto-migration
Successfully migratesNoteReviewandNoteCommentalong with existing models. Streamlines schema updates.internal/datastore/model.go (3)
26-31: LGTM! Well-structured relationships and virtual field.The relationships are properly defined with appropriate GORM annotations. The virtual field maintains backward compatibility with templates.
52-60: LGTM! Well-designed review status model.The NoteReview struct is well-structured with:
- Unique index on NoteID ensuring one review per note
- Appropriate field types and GORM annotations
- Proper timestamps for tracking review history
62-70: LGTM! Well-designed comment model.The NoteComment struct is well-structured with:
- Indexed NoteID for efficient queries
- Text field for comment content
- Proper timestamps for tracking comment history
internal/httpcontroller/handlers/detections.go (2)
376-415: LGTM! Well-structured comment handling.The comment handling logic is well-implemented with:
- Proper error handling
- Efficient update/create logic
- Good use of notifications
417-504: LGTM! Comprehensive review status handling.The review status handling is well-implemented with:
- Proper validation of status values
- Efficient handling of ignore species
- Good error handling and user notifications
views/elements/reviewModal.html (1)
84-86: LGTM! Well-integrated with new comment model.The comment field is properly updated to:
- Use the new Comments array
- Display the first comment if it exists
- Simplified performAutoMigration function by removing unnecessary log message - Kept error handling and migration logic intact - Reduced verbosity of database initialization process
- Added explicit error handling for detection ID parsing - Return a clear error message for invalid detection ID format - Prevent potential runtime errors with proper type conversion check
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/datastore/manage.go(1 hunks)internal/httpcontroller/handlers/detections.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/datastore/manage.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
internal/httpcontroller/handlers/detections.go (2)
372-374: LGTM! Error handling has been improved.The error handling for
ParseUinthas been properly implemented, addressing the previous review comment.
495-508: Skip comment as issues are addressed in previous review comment.
- Added comment length validation (max 1000 characters) - Implemented database transaction for comment update/create - Enhanced error handling for comment operations - Prevent race conditions during comment management
- Introduced Transaction method in datastore Interface - Implemented Transaction method for DataStore struct - Enables executing database operations within a transaction context - Supports more robust database interaction patterns
- Updated performAutoMigration function to use a more concise log message - Removed detailed connection info from log output - Maintained debug logging with a clearer, more generic message
…ies exclusion - Extracted species exclusion logic into a separate method - Implemented database transaction for review processing - Enhanced error handling and SSE notifications - Simplified ReviewDetection handler logic - Added validation for review status and species exclusion
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
internal/datastore/interfaces.go (2)
636-647: Verify comment existence before deletion.The function should verify if the comment exists before attempting to delete it.
Apply this diff to add existence verification:
func (ds *DataStore) DeleteNoteComment(commentID string) error { id, err := strconv.ParseUint(commentID, 10, 32) if err != nil { return fmt.Errorf("invalid comment ID: %w", err) } + // Verify comment existence + var comment NoteComment + if err := ds.DB.First(&comment, id).Error; err != nil { + if errors.Is(err, gorm.ErrRecordNotFound) { + return fmt.Errorf("comment with ID %s not found", commentID) + } + return fmt.Errorf("error checking comment existence: %w", err) + } + if err := ds.DB.Delete(&NoteComment{}, id).Error; err != nil { return fmt.Errorf("failed to delete note comment: %w", err) } return nil }
681-687: Simplify the implementation.The function can be more concise using a ternary-like expression.
Apply this diff to simplify the implementation:
-func sortAscendingString(asc bool) string { - if asc { - return "ASC" - } - return "DESC" -} +func sortAscendingString(asc bool) string { + return map[bool]string{true: "ASC", false: "DESC"}[asc] +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/datastore/interfaces.go(8 hunks)internal/datastore/manage.go(1 hunks)internal/httpcontroller/handlers/detections.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/datastore/manage.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (10)
internal/httpcontroller/handlers/detections.go (3)
408-410: LGTM! Improved ID parsing and error handling.The ID parsing now uses
strconv.ParseUintwith proper error handling, which is more robust than the previous implementation.
413-477: LGTM! Improved comment handling with validation and transaction support.The comment handling has been significantly improved with:
- Length validation to prevent excessively long comments.
- Transaction support to prevent race conditions.
- Clear error messages for better user experience.
479-524: LGTM! Improved review status handling with validation and transaction support.The review status handling has been significantly improved with:
- Early validation of review status.
- Extracted species exclusion logic for better maintainability.
- Transaction support to ensure atomicity.
- Clear success and error notifications.
internal/datastore/interfaces.go (7)
35-40: LGTM! Well-organized interface updates for note reviews and comments.The new methods are logically grouped, follow a consistent naming pattern, and provide a comprehensive API for managing note reviews and comments.
Also applies to: 49-49
164-175: LGTM! Efficient preloading of reviews and comments.The function efficiently preloads reviews and comments with proper ordering, while also populating the virtual Verified field.
Line range hint
350-371: LGTM! Consistent preloading of reviews and comments.The function maintains consistency with other functions by preloading reviews and comments with proper ordering.
381-393: LGTM! Consistent preloading of reviews and comments.The function maintains consistency with other functions by preloading reviews and comments with proper ordering.
417-431: LGTM! Consistent preloading of reviews and comments.The function maintains consistency with other functions by preloading reviews and comments with proper ordering.
575-596: LGTM! Well-implemented review retrieval with proper error handling.The function properly validates input, handles errors gracefully, and uses a session to control logging behavior.
612-626: LGTM! Well-implemented comment retrieval with proper ordering.The function properly validates input, orders comments by creation time, and handles errors gracefully.
| // handleSpeciesExclusion handles the logic for managing species in the exclusion list | ||
| func (h *Handlers) handleSpeciesExclusion(note *datastore.Note, verified string, ignoreSpecies string) error { | ||
| settings := conf.Setting() | ||
|
|
||
| if verified == "false_positive" && ignoreSpecies != "" { | ||
| // Check if species is already excluded | ||
| for _, s := range settings.Realtime.Species.Exclude { | ||
| if s == ignoreSpecies { | ||
| return nil | ||
| } | ||
| } | ||
|
|
||
| // Add to excluded list | ||
| settings.Realtime.Species.Exclude = append(settings.Realtime.Species.Exclude, ignoreSpecies) | ||
| if err := conf.SaveSettings(); err != nil { | ||
| return fmt.Errorf("failed to save settings: %w", err) | ||
| } | ||
|
|
||
| h.SSE.SendNotification(Notification{ | ||
| Message: fmt.Sprintf("%s added to ignore list", ignoreSpecies), | ||
| Type: "success", | ||
| }) | ||
| } else if verified == "correct" { | ||
| // Check if species is in exclude list | ||
| for _, s := range settings.Realtime.Species.Exclude { | ||
| if s == note.CommonName { | ||
| h.SSE.SendNotification(Notification{ | ||
| Message: fmt.Sprintf("%s is currently in ignore list. You may want to remove it from Settings.", note.CommonName), | ||
| Type: "warning", | ||
| }) | ||
| break | ||
| } | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Protect against race conditions in settings access.
The function directly accesses and modifies the global settings instance, which could lead to race conditions in a concurrent environment.
Apply this diff to protect against race conditions:
func (h *Handlers) handleSpeciesExclusion(note *datastore.Note, verified string, ignoreSpecies string) error {
+ // Use mutex to protect settings access
+ h.settingsMutex.Lock()
+ defer h.settingsMutex.Unlock()
+
settings := conf.Setting()
if verified == "false_positive" && ignoreSpecies != "" {
// Check if species is already excluded
for _, s := range settings.Realtime.Species.Exclude {
if s == ignoreSpecies {
return nil
}
}
// Add to excluded list
settings.Realtime.Species.Exclude = append(settings.Realtime.Species.Exclude, ignoreSpecies)
if err := conf.SaveSettings(); err != nil {
return fmt.Errorf("failed to save settings: %w", err)
}
h.SSE.SendNotification(Notification{
Message: fmt.Sprintf("%s added to ignore list", ignoreSpecies),
Type: "success",
})
} else if verified == "correct" {
// Check if species is in exclude list
for _, s := range settings.Realtime.Species.Exclude {
if s == note.CommonName {
h.SSE.SendNotification(Notification{
Message: fmt.Sprintf("%s is currently in ignore list. You may want to remove it from Settings.", note.CommonName),
Type: "warning",
})
break
}
}
}
return nil
}Committable suggestion skipped: line range outside the PR's diff.
internal/datastore/interfaces.go
Outdated
| // Transaction executes a function within a transaction. | ||
| func (ds *DataStore) Transaction(fc func(tx *gorm.DB) error) error { | ||
| return ds.DB.Transaction(fc) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation.
The function should validate the input function before attempting to execute it.
Apply this diff to add input validation:
func (ds *DataStore) Transaction(fc func(tx *gorm.DB) error) error {
+ if fc == nil {
+ return fmt.Errorf("transaction function cannot be nil")
+ }
return ds.DB.Transaction(fc)
}📝 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.
| // Transaction executes a function within a transaction. | |
| func (ds *DataStore) Transaction(fc func(tx *gorm.DB) error) error { | |
| return ds.DB.Transaction(fc) | |
| } | |
| // Transaction executes a function within a transaction. | |
| func (ds *DataStore) Transaction(fc func(tx *gorm.DB) error) error { | |
| if fc == nil { | |
| return fmt.Errorf("transaction function cannot be nil") | |
| } | |
| return ds.DB.Transaction(fc) | |
| } |
internal/datastore/interfaces.go
Outdated
| // SaveNoteComment saves a new comment for a note | ||
| func (ds *DataStore) SaveNoteComment(comment *NoteComment) error { | ||
| if err := ds.DB.Create(comment).Error; err != nil { | ||
| return fmt.Errorf("failed to save note comment: %w", err) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation.
The function should validate the input comment before attempting to save it.
Apply this diff to add input validation:
func (ds *DataStore) SaveNoteComment(comment *NoteComment) error {
+ // Validate input
+ if comment == nil {
+ return fmt.Errorf("comment cannot be nil")
+ }
+ if comment.NoteID == 0 {
+ return fmt.Errorf("note ID cannot be zero")
+ }
+ if len(comment.Entry) == 0 {
+ return fmt.Errorf("comment entry cannot be empty")
+ }
+ if len(comment.Entry) > 1000 {
+ return fmt.Errorf("comment entry exceeds maximum length of 1000 characters")
+ }
+
if err := ds.DB.Create(comment).Error; err != nil {
return fmt.Errorf("failed to save note comment: %w", err)
}
return nil
}📝 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.
| // SaveNoteComment saves a new comment for a note | |
| func (ds *DataStore) SaveNoteComment(comment *NoteComment) error { | |
| if err := ds.DB.Create(comment).Error; err != nil { | |
| return fmt.Errorf("failed to save note comment: %w", err) | |
| } | |
| return nil | |
| } | |
| // SaveNoteComment saves a new comment for a note | |
| func (ds *DataStore) SaveNoteComment(comment *NoteComment) error { | |
| // Validate input | |
| if comment == nil { | |
| return fmt.Errorf("comment cannot be nil") | |
| } | |
| if comment.NoteID == 0 { | |
| return fmt.Errorf("note ID cannot be zero") | |
| } | |
| if len(comment.Entry) == 0 { | |
| return fmt.Errorf("comment entry cannot be empty") | |
| } | |
| if len(comment.Entry) > 1000 { | |
| return fmt.Errorf("comment entry exceeds maximum length of 1000 characters") | |
| } | |
| if err := ds.DB.Create(comment).Error; err != nil { | |
| return fmt.Errorf("failed to save note comment: %w", err) | |
| } | |
| return nil | |
| } |
| // getHourRange returns the start and end times for a given hour and duration. | ||
| func getHourRange(hour string, duration int) (startTime, endTime string) { | ||
| startHour, _ := strconv.Atoi(hour) | ||
| endHour := (startHour + duration) % 24 | ||
| startTime = fmt.Sprintf("%02d:00:00", startHour) | ||
| endTime = fmt.Sprintf("%02d:00:00", endHour) | ||
| return startTime, endTime | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation.
The function should validate the hour and duration inputs.
Apply this diff to add input validation:
func getHourRange(hour string, duration int) (startTime, endTime string) {
+ // Validate hour
+ h, err := strconv.Atoi(hour)
+ if err != nil || h < 0 || h > 23 {
+ return "", ""
+ }
+
+ // Validate duration
+ if duration <= 0 || duration > 24 {
+ return "", ""
+ }
+
- startHour, _ := strconv.Atoi(hour)
- endHour := (startHour + duration) % 24
+ endHour := (h + duration) % 24
- startTime = fmt.Sprintf("%02d:00:00", startHour)
+ startTime = fmt.Sprintf("%02d:00:00", h)
endTime = fmt.Sprintf("%02d:00:00", endHour)
return startTime, endTime
}📝 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.
| // getHourRange returns the start and end times for a given hour and duration. | |
| func getHourRange(hour string, duration int) (startTime, endTime string) { | |
| startHour, _ := strconv.Atoi(hour) | |
| endHour := (startHour + duration) % 24 | |
| startTime = fmt.Sprintf("%02d:00:00", startHour) | |
| endTime = fmt.Sprintf("%02d:00:00", endHour) | |
| return startTime, endTime | |
| } | |
| // getHourRange returns the start and end times for a given hour and duration. | |
| func getHourRange(hour string, duration int) (startTime, endTime string) { | |
| // Validate hour | |
| h, err := strconv.Atoi(hour) | |
| if err != nil || h < 0 || h > 23 { | |
| return "", "" | |
| } | |
| // Validate duration | |
| if duration <= 0 || duration > 24 { | |
| return "", "" | |
| } | |
| endHour := (h + duration) % 24 | |
| startTime = fmt.Sprintf("%02d:00:00", h) | |
| endTime = fmt.Sprintf("%02d:00:00", endHour) | |
| return startTime, endTime | |
| } |
| // UpdateNoteComment updates an existing comment's entry | ||
| func (ds *DataStore) UpdateNoteComment(commentID string, entry string) error { | ||
| id, err := strconv.ParseUint(commentID, 10, 32) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid comment ID: %w", err) | ||
| } | ||
|
|
||
| result := ds.DB.Model(&NoteComment{}).Where("id = ?", id).Updates(map[string]interface{}{ | ||
| "entry": entry, | ||
| "updated_at": time.Now(), | ||
| }) | ||
|
|
||
| if result.Error != nil { | ||
| return fmt.Errorf("failed to update note comment: %w", result.Error) | ||
| } | ||
|
|
||
| if result.RowsAffected == 0 { | ||
| return fmt.Errorf("comment with ID %s not found", commentID) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for the entry.
The function should validate the entry content before attempting to update it.
Apply this diff to add input validation:
func (ds *DataStore) UpdateNoteComment(commentID string, entry string) error {
id, err := strconv.ParseUint(commentID, 10, 32)
if err != nil {
return fmt.Errorf("invalid comment ID: %w", err)
}
+ // Validate entry
+ if len(entry) == 0 {
+ return fmt.Errorf("comment entry cannot be empty")
+ }
+ if len(entry) > 1000 {
+ return fmt.Errorf("comment entry exceeds maximum length of 1000 characters")
+ }
+
result := ds.DB.Model(&NoteComment{}).Where("id = ?", id).Updates(map[string]interface{}{
"entry": entry,
"updated_at": time.Now(),
})
if result.Error != nil {
return fmt.Errorf("failed to update note comment: %w", result.Error)
}
if result.RowsAffected == 0 {
return fmt.Errorf("comment with ID %s not found", commentID)
}
return nil
}📝 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.
| // UpdateNoteComment updates an existing comment's entry | |
| func (ds *DataStore) UpdateNoteComment(commentID string, entry string) error { | |
| id, err := strconv.ParseUint(commentID, 10, 32) | |
| if err != nil { | |
| return fmt.Errorf("invalid comment ID: %w", err) | |
| } | |
| result := ds.DB.Model(&NoteComment{}).Where("id = ?", id).Updates(map[string]interface{}{ | |
| "entry": entry, | |
| "updated_at": time.Now(), | |
| }) | |
| if result.Error != nil { | |
| return fmt.Errorf("failed to update note comment: %w", result.Error) | |
| } | |
| if result.RowsAffected == 0 { | |
| return fmt.Errorf("comment with ID %s not found", commentID) | |
| } | |
| return nil | |
| } | |
| // UpdateNoteComment updates an existing comment's entry | |
| func (ds *DataStore) UpdateNoteComment(commentID string, entry string) error { | |
| id, err := strconv.ParseUint(commentID, 10, 32) | |
| if err != nil { | |
| return fmt.Errorf("invalid comment ID: %w", err) | |
| } | |
| // Validate entry | |
| if len(entry) == 0 { | |
| return fmt.Errorf("comment entry cannot be empty") | |
| } | |
| if len(entry) > 1000 { | |
| return fmt.Errorf("comment entry exceeds maximum length of 1000 characters") | |
| } | |
| result := ds.DB.Model(&NoteComment{}).Where("id = ?", id).Updates(map[string]interface{}{ | |
| "entry": entry, | |
| "updated_at": time.Now(), | |
| }) | |
| if result.Error != nil { | |
| return fmt.Errorf("failed to update note comment: %w", result.Error) | |
| } | |
| if result.RowsAffected == 0 { | |
| return fmt.Errorf("comment with ID %s not found", commentID) | |
| } | |
| return nil | |
| } |
| // SaveNoteReview saves or updates a note review | ||
| func (ds *DataStore) SaveNoteReview(review *NoteReview) error { | ||
| // Use upsert operation to either create or update the review | ||
| result := ds.DB.Where("note_id = ?", review.NoteID). | ||
| Assign(*review). | ||
| FirstOrCreate(review) | ||
|
|
||
| if result.Error != nil { | ||
| return fmt.Errorf("failed to save note review: %w", result.Error) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation.
The function should validate the input review before attempting to save it.
Apply this diff to add input validation:
func (ds *DataStore) SaveNoteReview(review *NoteReview) error {
+ // Validate input
+ if review == nil {
+ return fmt.Errorf("review cannot be nil")
+ }
+ if review.NoteID == 0 {
+ return fmt.Errorf("note ID cannot be zero")
+ }
+ if review.Verified != "correct" && review.Verified != "false_positive" {
+ return fmt.Errorf("invalid verification status: %s", review.Verified)
+ }
+
// Use upsert operation to either create or update the review
result := ds.DB.Where("note_id = ?", review.NoteID).
Assign(*review).
FirstOrCreate(review)
if result.Error != nil {
return fmt.Errorf("failed to save note review: %w", result.Error)
}
return nil
}📝 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.
| // SaveNoteReview saves or updates a note review | |
| func (ds *DataStore) SaveNoteReview(review *NoteReview) error { | |
| // Use upsert operation to either create or update the review | |
| result := ds.DB.Where("note_id = ?", review.NoteID). | |
| Assign(*review). | |
| FirstOrCreate(review) | |
| if result.Error != nil { | |
| return fmt.Errorf("failed to save note review: %w", result.Error) | |
| } | |
| return nil | |
| } | |
| // SaveNoteReview saves or updates a note review | |
| func (ds *DataStore) SaveNoteReview(review *NoteReview) error { | |
| // Validate input | |
| if review == nil { | |
| return fmt.Errorf("review cannot be nil") | |
| } | |
| if review.NoteID == 0 { | |
| return fmt.Errorf("note ID cannot be zero") | |
| } | |
| if review.Verified != "correct" && review.Verified != "false_positive" { | |
| return fmt.Errorf("invalid verification status: %s", review.Verified) | |
| } | |
| // Use upsert operation to either create or update the review | |
| result := ds.DB.Where("note_id = ?", review.NoteID). | |
| Assign(*review). | |
| FirstOrCreate(review) | |
| if result.Error != nil { | |
| return fmt.Errorf("failed to save note review: %w", result.Error) | |
| } | |
| return nil | |
| } |
- Added nil and input validation for SaveNoteComment method - Implemented length check for comment entries (max 1000 characters) - Added nil check for transaction function to prevent runtime errors - Improved error handling for comment and transaction operations
Instead of adding verify and comment columns to notes table this PR adds new tables for review status and comments, this approach avoids slow auto migration process with SQLite with other benefits also