fix: [#738] The Orm Creating event can be triggered when the query with the Model method#1163
fix: [#738] The Orm Creating event can be triggered when the query with the Model method#1163
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v1.15.x #1163 +/- ##
==========================================
Coverage ? 68.69%
==========================================
Files ? 219
Lines ? 18962
Branches ? 0
==========================================
Hits ? 13026
Misses ? 5259
Partials ? 677 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
database/gorm/query.go
Outdated
|
|
||
| func isSlice(dest any) bool { | ||
| destType := reflect.Indirect(reflect.ValueOf(dest)).Type() | ||
| return destType.Kind() == reflect.Slice |
There was a problem hiding this comment.
Would it be better to write
return destType.Kind() == reflect.Slice || destType.Kind() == reflect.Array
so that both slices and arrays are supported?
There was a problem hiding this comment.
Good catch, updated.
There was a problem hiding this comment.
Pull Request Overview
This pull request addresses a bug where ORM lifecycle events were incorrectly triggering during batch operations and mass deletes. The fix ensures that event handlers only execute for single-record operations by adding early returns when the destination is a slice (for create/update/save/retrieve events) or when deleting records without a specific ID (for delete events).
- Added guards to prevent event triggering on slice destinations and mass deletes
- Enhanced test coverage to verify the new event behavior
- Improved error handling throughout the codebase by explicitly handling return values
Reviewed Changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| database/gorm/query.go | Core event logic improvements with slice/ID checks and helper functions |
| database/gorm/query_test.go | Expanded test coverage for slice operations and mass delete scenarios |
| database/gorm/test_models.go | Added panic checks in event handlers to enforce new constraints |
| database/gorm/event.go | Minor boolean logic simplification |
| Various files | Improved error handling by explicitly handling function return values |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
I noticed that there are many places where errors are explicitly ignored, typically written as func silence(fn func() error) { _ = fn() }This would also make usages with |
|
@almas-x Good point, we can create another PR to optimize this. |
|
I modified them due to the lint error. |
📑 Description
Closes goravel/goravel#738
This pull request refines how ORM lifecycle events are triggered in the GORM query logic, especially for batch operations involving slices and mass deletes. The core improvement is to prevent event hooks from being triggered when the destination is a slice (for create, update, save, retrieve events) or when deleting/force-deleting mass records without a specific ID. This ensures event handlers only run for single-record operations, improving correctness and preventing unintended side effects. The test suite has been expanded to cover these new behaviors, and the event handler code has been updated to panic if the new constraints are violated, aiding debugging.
Event Handling Logic Improvements:
database/gorm/query.goso that lifecycle event hooks (creating,created,saving,saved,updating,updated,retrieved) return early and do not trigger when the destination is a slice, preventing events from firing on batch operations. [1] [2] [3] [4]deleting,deleted,forceDeleting,forceDeleted), hooks now only trigger if the destination has an ID, ensuring events are not fired for mass deletes.Helper Functions:
isSliceandhasIDhelper functions indatabase/gorm/query.goto support the new event logic, checking if a destination is a slice or has an ID.Test Coverage Enhancements:
database/gorm/query_test.goto verify that events are not triggered for slice-based operations and mass deletes, including new test cases for create, save, retrieve, and delete scenarios. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12]Event Handler Safety:
User.DispatchesEventsindatabase/gorm/test_models.goto panic if event hooks are called with a slice destination or without an ID, enforcing the new contract and surfacing violations during testing. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11]These changes collectively make event triggering more robust and predictable, especially in batch operations and mass record modifications.
✅ Checks