feat: [#508] Add Restore method for Orm, to recover soft deleted data #775
Conversation
WalkthroughThe pull request introduces a comprehensive enhancement to the ORM (Object-Relational Mapping) system, focusing on implementing a Changes
Sequence DiagramsequenceDiagram
participant Client
participant ORM
participant Model
participant Database
Client->>ORM: Restore(model)
ORM->>ORM: Trigger Restoring Event
ORM->>Database: Update DeletedAt column
ORM->>Model: Trigger Restored Event
ORM-->>Client: Return Result
Assessment against linked issues
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #775 +/- ##
==========================================
- Coverage 69.95% 69.91% -0.05%
==========================================
Files 213 213
Lines 18172 18246 +74
==========================================
+ Hits 12713 12756 +43
- Misses 4762 4785 +23
- Partials 697 705 +8 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: 2ed1065 | Previous: 8e5426f | Ratio |
|---|---|---|---|
Benchmark_DecryptString |
6150 ns/op 1592 B/op 17 allocs/op |
2037 ns/op 1592 B/op 17 allocs/op |
3.02 |
Benchmark_DecryptString - ns/op |
6150 ns/op |
2037 ns/op |
3.02 |
This comment was automatically generated by workflow using github-action-benchmark.
| Saving(Event) error | ||
| } | ||
|
|
||
| type ObserverWithUpdating interface { |
There was a problem hiding this comment.
Don't initiate these methods in the observer file. Add them according to requirement.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
mocks/database/orm/ObserverWithDeleting.go (1)
64-67:⚠️ Potential issueFix incorrect implementation of RunAndReturn method
The same RunAndReturn implementation issue exists in this mock.
Apply the same fix as in ObserverWithUpdating.
mocks/database/orm/ObserverWithRetrieved.go (1)
64-67:⚠️ Potential issueFix incorrect implementation of RunAndReturn method
The same RunAndReturn implementation issue exists in this mock.
Apply the same fix as in ObserverWithUpdating.
🧹 Nitpick comments (8)
database/gorm/query.go (2)
1603-1636: Consider adding fallback for custom DeletedAt field names.The getDeletedAtColumn function loops over embedded fields for a gorm.DeletedAt type. This might miss custom field names or data structures for soft deletes. If that’s intended, it’s fine, but ensure the developer experience is documented or enforced consistently.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 1605-1606: database/gorm/query.go#L1605-L1606
Added lines #L1605 - L1606 were not covered by tests
[warning] 1615-1615: database/gorm/query.go#L1615
Added line #L1615 was not covered by tests
[warning] 1625-1625: database/gorm/query.go#L1625
Added line #L1625 was not covered by tests
[warning] 1635-1635: database/gorm/query.go#L1635
Added line #L1635 was not covered by tests
1694-1701: Observer event retrieval for restored/restoring is well-structured.The extended getObserverEvent method now neatly includes EventRestored and EventRestoring. Confirm that all newly introduced events (e.g., “restoring”/“restored”) are tested with at least one observer to guarantee coverage.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 1694-1701: database/gorm/query.go#L1694-L1701
Added lines #L1694 - L1701 were not covered by testsdatabase/gorm/query_test.go (2)
1447-1467: Add concurrency or multi-model coverage for TestEvent_Restored.The test only verifies a single record scenario. Consider testing multiple restores at once or partial restores, verifying the correct event firing for each model instance.
1469-1489: Expand test coverage for negative Restoring events.TestEvent_Restoring thoroughly checks normal flow, but lacks negative scenarios (e.g., if restoration fails). This would strengthen confidence in error handling and event flow.
support/database/database.go (1)
30-37: Enhance coverage for empty primary key checks.You handle empty strings and zero numeric IDs. Lines 33-34 are uncovered by tests. Add a test scenario with a struct containing an empty primary key to confirm that nil is returned as expected.
Would you like help drafting a test snippet for this case?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 33-34: support/database/database.go#L33-L34
Added lines #L33 - L34 were not covered by testscontracts/database/orm/observer.go (1)
14-58: Well-structured interface segregation for lifecycle eventsThe breakdown of the monolithic Observer interface into smaller, focused interfaces follows the Interface Segregation Principle (ISP) effectively. This allows consumers to implement only the lifecycle events they need, reducing coupling.
The new
ObserverWithRestoringandObserverWithRestoredinterfaces align well with the PR's objective of implementing soft delete recovery.Consider documenting the recommended usage patterns for these interfaces in the package documentation, especially focusing on:
- When to use each interface
- Common implementation patterns
- Integration with the new Restore functionality
mocks/database/orm/ObserverWithRetrieved.go (1)
1-81: Consider implementing integration tests for observer chainThese mocks are part of the event system for the Restore functionality. To ensure robust testing:
- Implement integration tests that verify the correct order of events (Deleting → Retrieved → Updating) during restore operations
- Add test cases for error propagation through the observer chain
- Verify that all observers are properly cleaned up after tests
Would you like me to provide example test cases for these scenarios?
contracts/database/orm/orm.go (1)
129-130: Add documentation comments for the Restore method.The new
Restoremethod should include documentation comments explaining:
- Its purpose and behavior
- The relationship with
WithTrashed()- Any prerequisites or side effects
Add documentation comments like this:
+// Restore restores one or more soft deleted models. +// Must be used with WithTrashed() to include soft deleted records in the query scope. +// Returns the number of restored records and any error that occurred. Restore(model ...any) (*Result, error)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
contracts/database/orm/events.go(1 hunks)contracts/database/orm/observer.go(1 hunks)contracts/database/orm/orm.go(1 hunks)database/console/stubs.go(0 hunks)database/gorm/query.go(7 hunks)database/gorm/query_test.go(4 hunks)database/gorm/test_models.go(1 hunks)errors/list.go(1 hunks)mocks/database/orm/Observer.go(0 hunks)mocks/database/orm/ObserverWithCreating.go(1 hunks)mocks/database/orm/ObserverWithDeleting.go(1 hunks)mocks/database/orm/ObserverWithForceDeleting.go(1 hunks)mocks/database/orm/ObserverWithRestored.go(1 hunks)mocks/database/orm/ObserverWithRestoring.go(1 hunks)mocks/database/orm/ObserverWithRetrieved.go(1 hunks)mocks/database/orm/ObserverWithSaved.go(1 hunks)mocks/database/orm/ObserverWithSaving.go(1 hunks)mocks/database/orm/ObserverWithUpdating.go(1 hunks)mocks/database/orm/Query.go(1 hunks)support/database/database.go(1 hunks)
💤 Files with no reviewable changes (2)
- database/console/stubs.go
- mocks/database/orm/Observer.go
✅ Files skipped from review due to trivial changes (3)
- mocks/database/orm/ObserverWithCreating.go
- mocks/database/orm/ObserverWithRestored.go
- mocks/database/orm/ObserverWithRestoring.go
🧰 Additional context used
🪛 GitHub Check: codecov/patch
support/database/database.go
[warning] 33-34: support/database/database.go#L33-L34
Added lines #L33 - L34 were not covered by tests
database/gorm/query.go
[warning] 658-659: database/gorm/query.go#L658-L659
Added lines #L658 - L659 were not covered by tests
[warning] 674-675: database/gorm/query.go#L674-L675
Added lines #L674 - L675 were not covered by tests
[warning] 678-679: database/gorm/query.go#L678-L679
Added lines #L678 - L679 were not covered by tests
[warning] 683-684: database/gorm/query.go#L683-L684
Added lines #L683 - L684 were not covered by tests
[warning] 687-688: database/gorm/query.go#L687-L688
Added lines #L687 - L688 were not covered by tests
[warning] 1605-1606: database/gorm/query.go#L1605-L1606
Added lines #L1605 - L1606 were not covered by tests
[warning] 1615-1615: database/gorm/query.go#L1615
Added line #L1615 was not covered by tests
[warning] 1625-1625: database/gorm/query.go#L1625
Added line #L1625 was not covered by tests
[warning] 1635-1635: database/gorm/query.go#L1635
Added line #L1635 was not covered by tests
[warning] 1694-1701: database/gorm/query.go#L1694-L1701
Added lines #L1694 - L1701 were not covered by tests
🔇 Additional comments (16)
database/gorm/query.go (2)
1333-1347: Events hooking for delete/forceDelete operations are correct.
You have introduced separate methods (deleting, deleted, forceDeleting, forceDeleted), which is good for event hooking. However, confirm if the conventions remain consistent with all relevant model states.
1537-1537: Ensure consistent concurrency handling in update method.
This “update” method can be invoked concurrently. Consider verifying whether any concurrency or transaction usage might require a row-level lock or transaction pattern to avoid mid-update race conditions.
database/gorm/query_test.go (2)
3050-3094: Test multiple restoration flows in a single test method.
The TestRestore method verifies various “WithTrashed” usage patterns, which is thorough. However, lines 3069-3072 were specifically flagged for coverage. Add a scenario for attempting to restore an already active record or a model missing a deleted-at column to ensure robust coverage.
3772-3786: TestGetDeletedAtColumnName is well structured.
These lines confirm that embedded gorm.DeletedAt is recognized. Everything looks good with broad coverage for nested structs.
contracts/database/orm/events.go (1)
10-16: Enforce naming consistency.
You changed EventUpdated to “updated” for consistency. Everything else is neatly grouped. Ensure that any references or string matching outside this file are also updated (including logs or external references).
contracts/database/orm/observer.go (1)
29-37: Verify restore event ordering and error handling
The new restore-related interfaces look good, but we should ensure proper event ordering and error handling in the implementation.
Let's verify the usage of these interfaces in the codebase:
✅ Verification successful
Let me gather more information about the implementation to verify the event ordering and error handling.
Event ordering and error handling are properly implemented
The implementation in database/gorm/query.go shows correct event ordering and proper error handling:
- The
Restoreoperation follows a clear sequence:- Calls
restoring(model)before the actual restore operation - Performs the restore operation
- Calls
restored(model)after successful restore
- Calls
- Error handling is robust:
- Both
restoringandrestoredevents properly propagate errors (lines 677-679 and 686-688) - Each event handler returns early if an error occurs
- The error chain is maintained throughout the operation
- Both
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for implementations and usages of restore events
echo "Searching for restore event implementations..."
rg -l "ObserverWithRestor(ing|ed)" --type go
echo "Searching for restore event handling..."
rg "Restor(ing|ed)\(" --type go -A 5
Length of output: 4823
Script:
#!/bin/bash
# Search for the implementation of restore functionality in gorm query
echo "Searching for restore implementation in query.go..."
rg "func.*[Rr]estore" database/gorm/query.go -A 20
echo "\nSearching for restore event firing..."
ast-grep --pattern 'if err := $_; err != nil {
return $$$
}' database/gorm/query.go
Length of output: 7650
mocks/database/orm/ObserverWithSaved.go (1)
1-81: Auto-generated mock implementation looks correct
This mock file is auto-generated by mockery and follows the expected patterns for mock implementations.
mocks/database/orm/ObserverWithSaving.go (1)
1-81: Auto-generated mock implementation looks correct
This mock file is auto-generated by mockery and follows the expected patterns for mock implementations.
mocks/database/orm/ObserverWithRetrieved.go (1)
23-39: Verify Retrieved event handling for restored records
Since this mock is crucial for the Restore functionality, ensure that the Retrieved event is properly triggered when records are restored.
mocks/database/orm/ObserverWithForceDeleting.go (2)
23-39: LGTM! The ForceDeleting mock implementation is well-structured.
The implementation includes proper error handling for unspecified return values and supports both direct returns and function callbacks.
69-81: LGTM! The mock constructor follows best practices.
The NewObserverWithForceDeleting constructor properly:
- Registers with the testing interface
- Sets up automatic expectation verification via cleanup
- Returns a properly initialized mock instance
errors/list.go (1)
105-105: LGTM! The new error constant is well-defined.
The OrmDeletedAtColumnNotFound error constant follows the established pattern and provides a clear error message for soft delete functionality.
contracts/database/orm/orm.go (1)
129-130: Verify the interaction between Restore and WithTrashed methods.
Let's verify that the implementation properly handles the relationship between these methods.
✅ Verification successful
Based on the results, I can now generate the final response:
Restore method implementation correctly handles WithTrashed interaction
The test coverage shows that:
- Restore is properly used in conjunction with WithTrashed() in most cases
- There's appropriate error handling for cases where deleted_at column is not found
- Test cases cover various scenarios including:
- Restoring with and without WithTrashed()
- Restoring with different model configurations (single model, model slice)
- Restoring with additional query conditions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if Restore is always used with WithTrashed in the codebase
# and verify test coverage for different scenarios
# Look for Restore usage patterns
echo "Checking Restore usage patterns:"
rg -A 2 '\.Restore\('
# Look for test cases covering Restore functionality
echo "Checking test coverage:"
rg -A 5 'TestRestore'
# Check for potential error handling patterns
echo "Checking error handling patterns:"
rg -A 3 'OrmDeletedAtColumnNotFound'
Length of output: 2728
mocks/database/orm/Query.go (1)
2393-2458: LGTM: Restore method mock implementation is complete and consistent
The implementation includes:
- Proper method signature with variadic parameters
- Complete error handling
- All necessary mock helper types and methods
- Consistent documentation
database/gorm/test_models.go (2)
309-316: LGTM! Consider adding test coverage validation.
The EventRestored handler implementation follows the established patterns and conventions. The handler correctly modifies the "name" attribute for test scenarios.
Let's verify the test coverage for this handler:
✅ Verification successful
Let me check the test coverage by examining the actual test cases.
The EventRestored handler is properly tested
The test coverage is comprehensive in query_test.go. The test case TestEvent_Restored verifies:
- The initial state with "event_restored_name"
- The handler's modification to "event_restored_name1"
- The persistence of changes
- The proper restoration behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for the Restored event handler
# Look for test cases that verify the Restored event behavior
rg -l "event_restored_name" --type go
Length of output: 94
Script:
#!/bin/bash
# Look for test cases related to the Restored event
rg "event_restored_name" -B 2 -A 2 database/gorm/query_test.go
Length of output: 546
317-322: LGTM! Ensure consistent behavior between Restoring and Restored events.
The EventRestoring handler implementation is well-structured and follows the established patterns. Consider adding validation to ensure that when both handlers are triggered in sequence, they maintain data consistency.
Let's verify the test coverage and consistency between the handlers:
✅ Verification successful
Both Restoring and Restored events are properly tested with consistent behavior
The test coverage shows that both events are thoroughly tested in separate test cases (TestEvent_Restoring and TestEvent_Restored), and they follow the same pattern:
- Both handlers modify the name attribute in a consistent way (appending "1" to the name)
- Both have proper test cases verifying the attribute modifications
- Both test cases verify the state before and after the event handling
- The handlers are tested independently to ensure clean test isolation
While the events aren't tested in sequence in a single test case, this is actually a good practice as it allows for better isolation and clearer failure diagnosis. The current implementation and test coverage are sufficient to ensure reliable behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage and consistency between Restoring and Restored events
# Look for test cases that verify both events are triggered in sequence
rg -A 5 "event_restoring_name.*event_restored_name" --type go
Length of output: 63
Script:
#!/bin/bash
# Let's search for test cases involving these events separately and then analyze their relationship
# Search for event_restoring_name tests
echo "=== Searching for event_restoring_name tests ==="
rg -B 2 -A 5 "event_restoring_name" --type go
# Search for event_restored_name tests
echo -e "\n=== Searching for event_restored_name tests ==="
rg -B 2 -A 5 "event_restored_name" --type go
# Search for any test functions involving restore events
echo -e "\n=== Searching for restore event test functions ==="
rg -B 2 -A 5 "Test.*Restor.*Event" --type go
Length of output: 4016
📑 Description
Closes goravel/goravel#508
Implementing the
Restoremethod andRestoring,Restoredevents.Summary by CodeRabbit
New Features
Usermodel with new restoration events.Bug Fixes
EventUpdatedconstant.Documentation
Tests
Chores
✅ Checks