!fix(orderby): use BuildOptions instead of allowedColumns #46
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @cnlangzi - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 Complexity: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| func (ob *OrderByBuilder) isAllowed(col string) bool { | ||
| if ob.allowedColumns == nil { | ||
| return true | ||
| func (ob *OrderByBuilder) getColumn(col string) (string, bool) { |
There was a problem hiding this comment.
suggestion (code_refinement): Consider renaming getColumn to reflect its functionality.
The function does more than just getting a column; it also checks if the column is allowed and applies transformations. A name like 'processAndValidateColumn' might be more descriptive.
| func (ob *OrderByBuilder) getColumn(col string) (string, bool) { | |
| func (ob *OrderByBuilder) processAndValidateColumn(col string) (string, bool) { |
sqlbuilder_orderby_test.go
Outdated
| build: func() *Builder { | ||
| b := New("SELECT * FROM users") | ||
|
|
||
| ob := NewOrderBy(WithToName(strcase.ToSnake), WithAllow("created_at")). |
There was a problem hiding this comment.
suggestion (testing): Missing test case for WithToName functionality.
The test case 'with_to_name_order_by_should_work' checks the transformation of column names but does not verify if non-allowed columns are correctly excluded when transformed. It would be beneficial to add assertions to ensure that transformed but disallowed columns do not appear in the final SQL.
| ob := NewOrderBy(WithToName(strcase.ToSnake), WithAllow("created_at")). | |
| ob := NewOrderBy(WithToName(strcase.ToSnake), WithAllow("created_at")). | |
| ByAsc("id", "name"). | |
| ByDesc("createdAt", "unsafe_input") | |
| assert.False(t, strings.Contains(ob.String(), "unsafe_input"), "Disallowed column 'unsafe_input' should not appear in the final SQL") | |
| assert.False(t, strings.Contains(ob.String(), "created_at"), "Allowed column 'created_at' should not appear in the final SQL when not explicitly ordered") |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #46 +/- ##
==========================================
+ Coverage 76.65% 76.76% +0.11%
==========================================
Files 44 44
Lines 1829 1838 +9
==========================================
+ Hits 1402 1411 +9
Misses 304 304
Partials 123 123
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Here's the code health analysis summary for commits Analysis Summary
|
Changes
BuildOptioninstead ofallowedColumns