feat: [#358] Add Insert First methods for DB #888
Conversation
WalkthroughThe changes enhance the database interaction layer by modifying the Query interface and its implementations. New methods such as Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Query
participant Builder
Client->>Query: Insert(data)
Query->>Query: buildInsert(data)
Query->>Builder: Exec(SQL, args)
Builder-->>Query: (sql.Result, error)
Query-->>Client: Return (*Result, error)
sequenceDiagram
participant Client
participant Query
participant Builder
Client->>Query: First(dest)
Query->>Builder: Get(dest, SQL, args)
Builder-->>Query: error (if any)
Query-->>Client: Return error
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)Error: can't load config: the Go language version (go1.23) used to build golangci-lint is lower than the targeted Go version (1.24.0) ✨ 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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #888 +/- ##
=======================================
Coverage 67.65% 67.65%
=======================================
Files 152 152
Lines 10050 10050
=======================================
Hits 6799 6799
Misses 2925 2925
Partials 326 326 ☔ View full report in Codecov by Sentry. |
hwbrzzl
force-pushed
the
bowen/#358-1
branch
from
d01662c to
a445fae
Compare
hwbrzzl
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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: 19a2824 | Previous: 2cdb138 | Ratio |
|---|---|---|---|
Benchmark_EncryptString |
6259 ns/op 2160 B/op 15 allocs/op |
2225 ns/op 2160 B/op 15 allocs/op |
2.81 |
Benchmark_EncryptString - ns/op |
6259 ns/op |
2225 ns/op |
2.81 |
This comment was automatically generated by workflow using github-action-benchmark.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
database/factory/factory_test.go (1)
43-52:⚠️ Potential issueFix inconsistency in factory definition.
The factory definition creates value types for timestamp fields, but the struct fields are now pointer types.
Update the factory definition to use pointer types:
func (u *UserFactory) Definition() map[string]any { faker := gofakeit.New(0) + now := carbon.Now() + deletedAt := time.Now() return map[string]any{ "Name": faker.Name(), "Avatar": faker.Email(), - "CreatedAt": carbon.NewDateTime(carbon.Now()), - "UpdatedAt": carbon.NewDateTime(carbon.Now()), - "DeletedAt": gormio.DeletedAt{Time: time.Now(), Valid: true}, + "CreatedAt": carbon.NewDateTime(now), + "UpdatedAt": carbon.NewDateTime(now), + "DeletedAt": &gormio.DeletedAt{Time: deletedAt, Valid: true}, } }
🧹 Nitpick comments (7)
database/db/query.go (3)
31-40: Use structured logging instead offmt.Println.Within
First, you usefmt.Println(sql, args, err)for debug logging (lines 35-36). Replacing it with a structured logger or code-level debugging tools would align better with production standards.func (r *Query) First(dest any) error { sql, args, err := r.buildSelect() - fmt.Println(sql, args, err) + // logger.Debug("Executing SELECT: ", sql, args, "Error: ", err) if err != nil { return err } return r.builder.Get(dest, sql, args...) }
42-51: Consistency betweenGetandFirst.
Getalso usesfmt.Printlnfor logging but otherwise follows a similar pattern toFirst. Consistency in logging and error handling across these methods is encouraged. Consider extracting shared logic (e.g. building the SELECT query, printing a debug message) into a helper to reduce duplication.
53-78: Excellent insertion pattern; consider returning last insert ID.Your new
Insertmethod correctly leveragesbuildInsertand returns aResultcontainingRowsAffected. For completeness—if the database supports it—consider also retrieving a last inserted ID (e.g. usingresult.LastInsertId()) in the future.contracts/database/db/db.go (1)
16-18:Resultstruct usage.
RowsAffected int64is helpful, but consider a future enhancement to storeLastInsertIdif your DB engine supports it.database/db/utils.go (1)
89-131: Consider optimizing map allocation size.The map is initialized without a size hint, which could lead to reallocations as fields are added. Consider pre-allocating with the expected size.
-result := make(map[string]any) +result := make(map[string]any, typ.NumField())tests/db_test.go (1)
38-134: Add error case tests for Insert operations.While the happy path cases are well covered, consider adding tests for:
- Invalid field types
- Database connection errors
- Constraint violations
database/db/query_test.go (1)
16-20: Consider adding validation tags to the test model.The TestUser model could benefit from validation tags to test validation behavior during operations.
type TestUser struct { - ID uint `db:"id"` - Name string `db:"-"` - Age int + ID uint `db:"id" validate:"required"` + Name string `db:"-" validate:"required"` + Age int `validate:"gte=0"` }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
auth/auth_test.go(1 hunks)contracts/database/db/db.go(1 hunks)database/db/db.go(2 hunks)database/db/db_test.go(1 hunks)database/db/query.go(3 hunks)database/db/query_test.go(1 hunks)database/db/utils.go(1 hunks)database/db/utils_test.go(1 hunks)database/factory/factory_test.go(1 hunks)database/gorm/event_test.go(7 hunks)database/orm/model.go(1 hunks)errors/list.go(1 hunks)go.mod(1 hunks)mocks/database/db/Builder.go(1 hunks)mocks/database/db/Query.go(2 hunks)support/database/database_test.go(1 hunks)tests/db_test.go(3 hunks)tests/go.mod(1 hunks)tests/models.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/go.mod
⏰ Context from checks skipped due to timeout of 300000ms (2)
- GitHub Check: ubuntu (1.24)
- GitHub Check: ubuntu (1.23)
🔇 Additional comments (25)
go.mod (1)
95-95: Dependency Version Upgrade: golang.org/x/netThe dependency version for
golang.org/x/nethas been updated from v0.34.0 to v0.35.0. This change aligns with the updated version in the test module and ensures consistency within the dependency tree. Please verify that the new version maintains compatibility with all modules relying on this package across the codebase.database/db/query.go (4)
5-19: Confirm the necessity of new imports and fields.You introduced
"sort"(line 5),"github.com/goravel/framework/support/str"(line 12), and thebuilder db.Builderfield (line 16). Ensure these dependencies are strictly needed and do not create circular references. Also confirm that thebuilderfield is always initialized with a valid object to avoid runtime errors.
21-29: Constructor logic validation.
NewQuerysets up a freshQueryconfigured with adb.Builder. This design appears consistent with the shift to a builder pattern. Make sure to handle potential edge cases, such as emptytablenames (though you check table presence in other methods, it’s good to consistently protect constructors with validations if needed).
80-89: Immutability approach inWhere.
Wherereturns a newQueryinstance rather than mutating the original, promoting an immutable/fluent API. This is generally safer in concurrent contexts. Confirm that client code expects a newly createdQueryeach time and is not relying on any side effects to the old instance.
91-119: Insertion logic is clear and well-structured.You’ve used
sort.Strings(cols)to ensure consistent column ordering for multi-row inserts, which is a good approach. The method is concise and easy to read.contracts/database/db/db.go (3)
3-4: Validate the standard library import usage.The new import
"database/sql"looks appropriate forsql.Resultusage. Confirm that no advanced features from other packages are required.
10-14: New methods in theQueryinterface are well-defined.Adding
First(dest any) errorandInsert(data any) (*Result, error)extends the interface logically. Ensure that your mocking strategy covers these new methods thoroughly for testing.
20-23: Builder interface alignment.The
Builderinterface’sExec,Get, andSelectmethods make sense for your builder pattern. Confirm that any advanced query building or relation handling is accounted for.database/orm/model.go (1)
18-23: Check nil pointer usage compatibility.Changing
DeletedAt,CreatedAt, andUpdatedAtto pointers allows nil values. Ensure that migrations, queries, and JSON marshaling handle nil values as expected. Also confirm thatgorm:"autoCreateTime"andgorm:"autoUpdateTime"operate correctly with these pointer types in your environment.database/db/db.go (2)
15-18: LGTM! Good architectural improvement.The shift from direct DB instance to builder pattern improves separation of concerns and testability. This change provides better abstraction for the database layer.
20-22:✅ Verification successful
Verify impact of builder pattern transition.
While the implementation looks correct, this is a significant architectural change that affects how database operations are performed.
Let's verify the usage of this new pattern across the codebase:
Also applies to: 43-45
🏁 Script executed:
#!/bin/bash # Search for any remaining direct usage of sqlx.DB that should be using the builder rg -g '!vendor/*' -g '!mocks/*' "sqlx\.DB" # Search for existing builder pattern implementations to ensure consistency ast-grep --pattern 'type $_ struct { $$$ builder db.Builder $$$ }'Length of output: 483
Builder Pattern Transition Verified
Our scan confirms that the transition to using
db.Builderis consistently applied:
- In
database/db/db.go, theNewDBfunction correctly instantiates theDBstruct with bothconfigandbuilderas intended.- In
database/db/query.go, theQuerystruct similarly embedsbuilder db.Builder, following the new architectural pattern.- No direct usage of
sqlx.DBwas detected across the codebase.Everything appears aligned with the intended refactor. Please continue to monitor any future changes to ensure consistent use of the builder pattern.
database/db/db_test.go (1)
16-67: LGTM! Well-structured test implementation.The test suite demonstrates good practices:
- Table-driven tests for better maintainability
- Proper mocking of dependencies
- Coverage of both success and error scenarios
database/factory/factory_test.go (1)
20-27: LGTM! Good change to support nullable timestamps.The change to pointer types for timestamp fields allows proper handling of null values.
database/db/utils_test.go (1)
22-95: LGTM! Comprehensive test coverage for utility function.The test suite demonstrates excellent practices:
- Table-driven tests with diverse scenarios
- Edge case handling (nil, pointers, maps)
- Validation of struct tag behavior
database/db/utils.go (1)
10-61: LGTM! Well-structured conversion function with comprehensive type handling.The function handles various input types robustly, including nil values, pointers, slices, and single values. Good error propagation and type checking.
support/database/database_test.go (1)
19-20: LGTM! Good change to pointer types for timestamp fields.Using pointer types for timestamp fields allows for proper nil value handling, which is essential for optional timestamps.
tests/db_test.go (1)
136-173: LGTM! Comprehensive test cases for Where conditions.Good coverage of different query scenarios including simple conditions, multiple arguments, and raw queries.
database/db/query_test.go (1)
54-145: LGTM! Excellent test coverage for Insert operations.The test cases cover all important scenarios:
- Single and multiple structs
- Single and multiple maps
- Error cases
- Mock result verification
mocks/database/db/Query.go (2)
23-39: LGTM! The First method implementation follows mock best practices.The implementation correctly handles:
- Return value validation with appropriate panic
- Type assertions for function returns
- Error propagation
115-143: LGTM! The Insert method implementation follows mock best practices.The implementation correctly handles:
- Multiple return values (Result and error)
- Type assertions for function returns
- Proper nil checks for Result
mocks/database/db/Builder.go (1)
1-222: LGTM! The Builder mock implementation follows best practices.The implementation correctly handles:
- Consistent error handling across methods
- Proper type assertions
- Flexible return value handling
- Clean separation of concerns with helper methods
errors/list.go (1)
53-53: LGTM! The error message is well-formed.The error message follows the established pattern and provides clear context with placeholders for the unsupported and expected types.
tests/models.go (2)
446-450: LGTM! Added database column mapping.The addition of the
dbtag for theNamefield improves clarity in database mapping and follows ORM best practices.
28-31:❓ Verification inconclusive
Verify the impact of nullable timestamp fields.
Converting
CreatedAtandUpdatedAtfrom value types to pointer types is a breaking change. While it allows for nil values, it requires careful handling in code that accesses these fields.Run the following script to check for potential impacts:
🏁 Script executed:
#!/bin/bash # Description: Check for direct field access and potential nil pointer dereferences. # Search for direct field access rg -A 2 'CreatedAt\.|UpdatedAt\.' # Search for potential nil checks rg -A 2 'if .*CreatedAt|if .*UpdatedAt'Length of output: 2145
Action Required: Revisit Nullable Field Handling
The tests in
tests/query_test.goandtests/factory_test.godirectly call.String()on bothCreatedAtandUpdatedAtwithout checking for nil, which may lead to runtime nil dereference errors when these fields are nil. Please ensure that one of the following approaches is taken:
- Add explicit nil checks before accessing these pointer fields.
- Guarantee that these fields are always populated, even when set as nullable, to avoid breaking existing call sites.
auth/auth_test.go (1)
29-30:❓ Verification inconclusive
Verify test coverage for nil timestamp values.
The change to pointer types for
CreatedAtandUpdatedAtfields is consistent with the model changes. However, ensure that test cases cover scenarios with nil values.Run the following script to check test coverage:
🏁 Script executed:
#!/bin/bash # Description: Check for test cases handling nil timestamp values. # Search for test cases setting nil timestamps rg -A 5 'CreatedAt = nil|UpdatedAt = nil' # Search for test assertions on timestamps rg -A 5 'assert.*CreatedAt|assert.*UpdatedAt'Length of output: 354
Attention: Verify handling of potential nil timestamps in auth tests.
The conversion of
CreatedAtandUpdatedAtto pointer types is consistent with the overall model changes. Our checks show that tests—such as those indatabase/factory/factory_test.go—already confirm these fields are automatically set (asserting they’re not nil). However, please ensure that the tests inauth/auth_test.goalso cover scenarios where, due to any edge case or misconfiguration, these fields might unexpectedly be nil. If such a scenario is plausible, consider adding explicit tests to simulate and handle nil timestamp values.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
database/db/query.go (1)
134-148:⚠️ Potential issuePotential panic in Where clause handling.
The code could panic if where.args is empty but the query doesn't contain placeholders.
🧹 Nitpick comments (3)
database/db/query.go (1)
31-40: Replace println with proper logging.The TODO comment indicates a need for proper logging. This should be addressed before merging.
- // TODO: use logger instead of println - fmt.Println(sql, args, err) + if r.config.Debug { + r.config.Logger.Info("SQL Query", map[string]any{ + "query": sql, + "args": args, + }) + }tests/db_test.go (2)
38-134: Add test cases for error scenarios.The test suite thoroughly covers successful scenarios but lacks tests for error cases. Consider adding tests for:
- Inserting nil/empty data
- Invalid data types
- Concurrent inserts
Example test case:
s.Run("error cases", func() { // Test nil data result, err := query.DB().Table("products").Insert(nil) s.Error(err) s.Nil(result) // Test empty slice result, err = query.DB().Table("products").Insert([]Product{}) s.Error(err) s.Nil(result) // Test invalid type result, err = query.DB().Table("products").Insert(42) s.Error(err) s.Nil(result) })
136-173: Add test cases for complex Where conditions.The Where test cases could be expanded to include:
- Multiple WHERE clauses with AND/OR conditions
- WHERE IN with empty arrays
- WHERE with NULL/NOT NULL conditions
Example test cases:
s.Run("complex where conditions", func() { var products []Product err := query.DB().Table("products"). Where("name = ?", "where model"). Where("created_at > ?", now). Get(&products) s.NoError(err) err = query.DB().Table("products"). Where("name", []string{}). Get(&products) s.NoError(err) s.Empty(products) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
database/db/query.go(3 hunks)tests/db_test.go(3 hunks)tests/models.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/models.go
🔇 Additional comments (1)
database/db/query.go (1)
15-29: LGTM! Good refactoring to use the builder pattern.The Query struct and constructor have been updated to use a builder pattern, which improves modularity and testability.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
database/db/query.go (1)
96-125:⚠️ Potential issueAdd validation for empty data array.
The function assumes
data[0]exists without checking array bounds, which could lead to a panic.Apply this diff to add validation:
func (r *Query) buildInsert(data []map[string]any) (sql string, args []any, err error) { if r.conditions.table == "" { return "", nil, errors.DatabaseTableIsRequired } + if len(data) == 0 { + return "", nil, fmt.Errorf("no data to insert") + }
🧹 Nitpick comments (2)
database/db/query.go (1)
31-40: Replace TODO with proper logging.The function uses a TODO comment for logging. Consider using a proper logging mechanism for SQL queries and their arguments.
Would you like me to help implement a proper logging mechanism for SQL queries?
database/db/query_test.go (1)
37-43: Add more test cases for error scenarios.Consider adding test cases for:
- Empty result
- Database error
- Invalid query conditions
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
database/db/query.go(3 hunks)database/db/query_test.go(1 hunks)
🔇 Additional comments (9)
database/db/query.go (5)
21-29: LGTM! Good use of the builder pattern.The constructor function has been updated to use a builder pattern, which is a good design choice for SQL operations.
42-51: Replace TODO with proper logging.The function uses a TODO comment for logging. Consider using a proper logging mechanism for SQL queries and their arguments.
53-83: LGTM! Good error handling and empty data validation.The function correctly handles empty data and error cases. However, the TODO comment for logging should be addressed.
85-94: LGTM! Good use of immutability.The function creates a new Query instance instead of modifying the existing one, which is a good practice for maintaining immutability.
127-156: LGTM! Good handling of different where conditions.The function correctly handles different types of where conditions and placeholders in raw queries.
database/db/query_test.go (4)
15-35: LGTM! Good test setup with proper mocking.The test setup correctly defines the test model with db tags and initializes the mock builder.
45-52: Add more test cases for error scenarios.Consider adding test cases for:
- Empty result
- Database error
- Invalid query conditions
54-151: LGTM! Excellent test coverage.The test suite comprehensively covers various scenarios:
- Empty data
- Single and multiple structs
- Single and multiple maps
- Error cases (unknown type, exec failure)
153-177: LGTM! Good coverage of where conditions.The test suite effectively covers various where conditions:
- Simple where condition
- Multiple arguments
- Raw query
📑 Description
goravel/goravel#358
Summary by CodeRabbit
New Features
Refactor
Tests
Chores
✅ Checks