feat: [#646] The Where function of Orm supports func condition#986
feat: [#646] The Where function of Orm supports func condition#986
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for passing a function as a condition to the Orm's Where query function while updating references from ormcontract to contractsorm.
- Updated tests to assert the new function condition.
- Added the buildSubquery helper in the query package to handle function conditions.
- Updated import references in the conditions package.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/query_test.go | Added test case for the function condition in the Where clause |
| database/gorm/query.go | Introduced buildSubquery and updated buildWhere logic to handle function conditions |
| database/gorm/conditions.go | Updated import from ormcontract to contractsorm |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #986 +/- ##
=======================================
Coverage 70.04% 70.04%
=======================================
Files 168 168
Lines 11353 11353
=======================================
Hits 7952 7952
Misses 3050 3050
Partials 351 351 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| for _, item := range r.conditions.where { | ||
| if sub, ok := item.query.(func(contractsorm.Query) contractsorm.Query); ok { |
There was a problem hiding this comment.
Can it support OrWhere(func(contractsorm.Query) contractsorm.Query)?
There was a problem hiding this comment.
It is implemented through Gorm’s Group Conditions, so theoretically, OrWhere should be supported. However, since Gorm processes OR conditions based on their position, there might be some issues with how it’s handled.
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: 77db541 | Previous: 9bc11ed | Ratio |
|---|---|---|---|
BenchmarkFile_ReadWrite |
350170 ns/op 2072 B/op 28 allocs/op |
227847 ns/op 2073 B/op 28 allocs/op |
1.54 |
BenchmarkFile_ReadWrite - ns/op |
350170 ns/op |
227847 ns/op |
1.54 |
This comment was automatically generated by workflow using github-action-benchmark.
| return queryImpl.buildWhere(db) | ||
| } | ||
|
|
||
| return db |
There was a problem hiding this comment.
Can nil be returned here? Then continue at line 1231 if r.buildSubquery(sub) == nil.
There was a problem hiding this comment.
Yes, to prevent a panic when returning nil.
There was a problem hiding this comment.
Em, I mean we can return nil here, don't need to return an empty db.
There was a problem hiding this comment.
Yes, since the closure function returns an orm.Query interface, returning nil will still compile successfully. Therefore, it’s better to handle the nil case properly.
📑 Description
Closes goravel/goravel#646
Print SQL
✅ Checks