feat: extension the Create method of Orm #653
Conversation
|
Warning Rate limit exceeded@hwbrzzl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 47 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes encompass modifications to the testing and implementation of the Changes
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 #653 +/- ##
==========================================
+ Coverage 70.31% 70.40% +0.08%
==========================================
Files 188 188
Lines 11645 11677 +32
==========================================
+ Hits 8188 8221 +33
+ Misses 2866 2865 -1
Partials 591 591 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (6)
database/gorm/event_test.go (1)
112-138: LGTM! Consider minor refactoring for improved readability.The new test case for
TestSetAttributewith a struct destination is well-implemented and complements the existing test case with a map destination. It thoroughly tests both camel case and lowercase key scenarios, ensuring theSetAttributeandGetAttributemethods work correctly with struct destinations.To improve code organization and readability, consider extracting the common test logic into a helper function. This would reduce duplication and make it easier to add more test cases in the future if needed.
Here's a suggested refactoring:
func (s *EventTestSuite) TestSetAttribute() { testCases := []struct { name string dest interface{} }{ {"Map Destination", map[string]any{"avatar": "avatar1"}}, {"Struct Destination", &TestEventModel{Avatar: "avatar1"}}, } for _, tc := range testCases { s.Run(tc.name, func() { query := &QueryImpl{ instance: &gorm.DB{ Statement: &gorm.Statement{ Selects: []string{}, Omits: []string{}, Dest: tc.dest, }, }, } event := NewEvent(query, &testEventModel, tc.dest) s.testSetAndGetAttribute(event, "Name", "name1") s.testSetAndGetAttribute(event, "Avatar", "avatar2") }) } } func (s *EventTestSuite) testSetAndGetAttribute(event *Event, key, value string) { event.SetAttribute(key, value) s.Equal(value, event.GetAttribute(key)) s.Equal(value, event.GetAttribute(strings.ToLower(key))) }This refactoring improves readability, reduces duplication, and makes it easier to add more test cases in the future.
database/gorm/test_models.go (3)
43-51: LGTM: Additional conditions for various creation scenarios.These new conditions provide more granular control over different creation scenarios, which is beneficial for testing and handling various use cases. The naming conventions are consistent with the existing code.
Consider grouping related conditions together for improved readability. For example:
if name.(string) == "event_creating_by_map_name" { event.SetAttribute("avatar", "event_creating_by_map_avatar1") } else if name.(string) == "event_creating_omit_create_name" { event.SetAttribute("avatar", "event_creating_omit_create_avatar") } else if name.(string) == "event_creating_select_create_name" { event.SetAttribute("avatar", "event_creating_select_create_avatar") } else if name.(string) == "event_creating_save_name" { event.SetAttribute("avatar", "event_creating_save_avatar") }This grouping can make it easier to understand and maintain related conditions.
75-93: LGTM: Corresponding conditions for "Created" events.These new conditions align well with the previously added "Creating" event conditions, providing consistency in handling various creation scenarios. The use of record ID in some conditions is appropriate for ensuring uniqueness.
For consistency, consider using the record ID in all new conditions. For example:
if name.(string) == "event_created_by_map_name" { id := event.GetAttribute("ID") event.SetAttribute("avatar", fmt.Sprintf("event_created_by_map_avatar_%d", id)) }This change would make all new conditions consistent in their use of the record ID.
147-155: LGTM: Corresponding conditions for "Saved" events.These new conditions align well with the previously added "Saving" event conditions, providing consistency in handling various saving scenarios. The naming conventions are consistent with the existing code.
For consistency with earlier suggestions, consider using the record ID in these conditions as well. For example:
if name == "event_saved_create_by_map_name" { id := event.GetAttribute("ID") event.SetAttribute("avatar", fmt.Sprintf("event_saved_create_by_map_avatar_%d", id)) }This change would make all new conditions consistent in their use of the record ID and align with the earlier suggestions for the "Created" events.
database/gorm/query_test.go (2)
420-463: Clarify the key casing in maps when creating recordsThe test uses different key casing in maps (
"name"vs."Name","avatar"vs."Avatar"). This difference stems from usingquery.Table("users")(requiring database column names) andquery.Model(User{})(requiring struct field names). Consider adding comments to explain this distinction for future maintainers.
Line range hint
369-578: Consider refactoring tests to reduce code duplicationMultiple test cases within these lines have similar structures and logic, differing mainly in test data and expected outcomes. Refactoring to use table-driven tests can minimize code repetition, improve readability, and make it easier to add new test cases in the future.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- database/gorm/event_test.go (2 hunks)
- database/gorm/query.go (13 hunks)
- database/gorm/query_test.go (27 hunks)
- database/gorm/test_models.go (4 hunks)
Additional comments not posted (30)
database/gorm/test_models.go (3)
37-39: LGTM: New condition for map-based creation event.This addition aligns with the PR objective to support map-based record creation. The naming convention is consistent with other conditions, and the implementation looks correct.
105-107: LGTM: New condition for map-based saving event.This addition aligns with the new map-based creation functionality and maintains consistency with other conditions in the method. The implementation looks correct.
114-119: LGTM: Additional conditions for various saving scenarios.These new conditions provide more granular control over different saving scenarios, which is beneficial for testing and handling various use cases. The naming conventions are consistent with the existing code, and the implementation looks correct.
database/gorm/query.go (13)
166-168: Proper implementation ofDrivermethod.The
Drivermethod correctly returns the driver type usingr.instance.Dialector.Name(), ensuring consistent retrieval of the database driver.
Line range hint
598-634: Refactored event handling inSavemethod for consistency.The
Savemethod now consistently uses thevalueparameter in event method calls (saving,updating,updated,saved), enhancing readability and maintaining a uniform interface across methods.
731-745: Consistent event handling inUpdatemethod.The
Updatemethod correctly calls event methods (saving,updated,saved) withquery.instance.Statement.Dest, ensuring proper event dispatch during updates and aligning with the refactoring to usedestorvalue.
1136-1151: Simplifiedcreatemethod by usingdestparameter in event methods.Refactoring the
createmethod to usedestdirectly in event method calls (saving,creating,created,saved) improves clarity by removing unnecessary parameters and aligns with the overall codebase refactoring.
1159-1163: Updated event methodscreatedandcreatingto usedest.Simplifying the function signatures by removing the redundant
modelparameter enhances code maintainability and reduces potential errors.
1190-1191: Corrected event dispatch ineventmethod for custom events.The implementation properly checks for custom event handlers in the
DispatchesEventsmap and invokes them when they exist, ensuring custom event logic is executed as expected.
1206-1217: Enhanced observer pattern implementation ineventmethod.The code accurately retrieves observers for both
destandmodeland invokes the appropriate event methods usingobserverEvent, improving the extensibility of the event handling system.
Line range hint
1246-1266: RefactoredomitCreatemethod to usevaluein event methods.Consistently using the
valueparameter insavingandsavedevent method calls within theomitCreatefunction enhances code consistency and readability.
1319-1324: Simplifiedsavedandsavingmethods to usedestparameter.This change removes unnecessary parameters, aligning with the overall refactoring effort to streamline method signatures.
Line range hint
1340-1354: Consistent use ofvalueinselectCreateevent methods.Updating the
selectCreatemethod to usevalueinsavingandsavedevent calls maintains consistency across create methods.
1382-1387: Refactoredupdatingandupdatedmethods to usedest.Simplifying these method signatures by using
destenhances clarity and ensures consistent event handling.
1457-1484: EnhancedgetModelConnectionto support map types.By checking if
modelType.Kind()isreflect.Mapand returning an empty string when true, the function now gracefully handles map types, which is essential for supporting map-based record creation as per the PR objectives.
1492-1495: Renamedobserverfunction togetObserverfor clarity.The function has been renamed to
getObserver, which more accurately reflects its purpose of retrieving the observer associated with a given model.database/gorm/query_test.go (14)
21-21: Importing 'carbon' package is appropriate and necessaryThe
carbonpackage is used in the code for handling timestamps (e.g.,carbon.Now()). Including this import ensures that the necessary functionality is available.
369-376: Test case 'success by struct' is correctly implementedThis test verifies that a user can be successfully created using a struct. The assertions ensure that the creation operation does not return an error and that the user ID is set.
378-388: Test case 'batch create success by struct' effectively tests batch creationThe test checks the ability to create multiple users in a single operation using a slice of structs. It correctly asserts that all users receive valid IDs upon creation.
390-418: Test case 'success by map' accurately validates map-based creationThis test confirms that users can be created using a map of field values both when specifying the table directly and when using a model. The assertions verify that the users are correctly inserted into the database.
Line range hint
481-488: Test 'success when create with no relationships' appropriately verifies exclusion of associationsThe test ensures that when creating a user without specifying associations, the related
AddressandBooksare not saved. The assertions confirm that the IDs for these associations remain zero.
Line range hint
495-502: Test 'success when create with select orm.Associations' confirms associations are createdBy selecting
orm.Associations, this test verifies that the associatedAddressandBooksare saved along with the user. The assertions correctly check that all IDs are greater than zero.
Line range hint
509-518: Test 'success when create with select fields' validates selective field creationThe test checks that only specified fields and associations (
"Name","Avatar", and"Address") are saved when usingSelect. It correctly asserts that theAddressis created while theBooksare not.
Line range hint
523-532: Test 'success when create with omit fields' correctly omits specified associationBy omitting the
"Address"field, this test ensures that theAddressassociation is not created while other associations (Books) are saved. The assertions confirm this behavior.
Line range hint
537-546: Test 'success create with omit orm.Associations' ensures no associations are savedThis test verifies that when
orm.Associationsis omitted, none of the associated records are created. The IDs forAddressandBooksremain zero as expected.
Line range hint
551-558: Proper error handling when using bothSelectandOmitThe test correctly checks that specifying both
SelectandOmittriggers an error, as this is not allowed. It asserts that the error message matches the expected output.
Line range hint
561-568: Validating error when combiningSelectwithorm.Associationsand fieldsThis test ensures that selecting both
orm.Associationsand specific fields results in the appropriate error. It confirms that the operation is invalid and that the error message is as expected.
Line range hint
571-578: Ensuring error is raised whenOmitincludes both fields andorm.AssociationsThe test confirms that omitting both specific fields and
orm.Associationsis not permitted. It correctly asserts that the appropriate error message is returned.
Line range hint
781-790: Test 'trigger when create by struct' correctly verifies event triggeringThis test checks that creating a user by struct triggers the expected events, and that the
Avatarfield is set appropriately. The assertions confirm that the user's data is correctly saved and that the events have the intended effect.
793-807: Verify the correctness of event handling in 'trigger when create by map'The test expects the
Avatarfield to be modified during the creation process. Ensure that the event handlers are correctly implemented to modify theAvatarfield as intended.Run the following script to check if the
Avatarfield is updated during the creation event:
hwbrzzl
changed the title
| @@ -34,9 +34,21 @@ func (u *User) DispatchesEvents() map[ormcontract.EventType]func(ormcontract.Eve | |||
| if name.(string) == "event_creating_name" { | |||
There was a problem hiding this comment.
Instead of this, would it be better to cast it? Because if this is not asserted, it will panic.
There was a problem hiding this comment.
Sure, I'll optimize it in another PR.
hwbrzzl
dismissed stale reviews from kkumar-gcc and coderabbitai[bot]
via
ca9c144
hwbrzzl
force-pushed
the
bowen/optimize-create
branch
from
7a2854a
to
ca9c144
Compare
📑 Description
The
Createmethod of Orm supports map:Summary by CodeRabbit
New Features
Userstruct to accommodate various event names and set corresponding attributes.Bug Fixes
Refactor
QueryImplstruct methods.Tests
✅ Checks