Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds runtime SQL column-name validation to QueryBuilder via a new unexported helper that uses Changes
Sequence Diagram(s)(Skipped — changes are internal validation and tests without multi-component sequential flow requiring visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)builder_test.go (4)
🔇 Additional comments (3)
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 (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
builder.go (1)
54-64: Move validation before building placeholders.The validation check happens after building the
placeholdersarray (lines 55-58). If the column is invalid, this work is wasted. Move the validation to the beginning of the method for consistency with other methods likeEqual,Not, andOr.The same issue exists in
NotIn(lines 139-150).🔎 Proposed fix
func (q *QueryBuilder) In(column string, values ...interface{}) *QueryBuilder { + if !isValidColumn(column) { + return q + } placeholders := make([]string, len(values)) for i := range values { placeholders[i] = "?" } - if !isValidColumn(column) { - return q - } query := column + " IN (" + strings.Join(placeholders, ", ") + ")" q.qb = q.qb.Where(query, values...) return q }
| if !isValidColumn(column) { | ||
| return q | ||
| } |
There was a problem hiding this comment.
Major: Silent validation failure obscures errors and could cause incorrect query results.
When validation fails, the method returns the unchanged QueryBuilder without any error, log, or indication. This means:
- Queries silently omit the intended WHERE clause
- Developers have no feedback that validation failed
- Security-critical filters could be silently dropped, potentially causing data leaks
Consider one of these approaches:
- Return an error alongside the QueryBuilder
- Log a warning when validation fails
- Panic if invalid columns indicate a programming error
This comment applies to all 14 methods with validation guards.
| func isValidColumn(column string) bool { | ||
| return validator.IsAlphanumeric(column) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the validator package and IsAlphanumeric implementation
rg -nP 'IsAlphanumeric' --type=go -B 2 -A 5Repository: tinh-tinh/sqlorm
Length of output: 204
🏁 Script executed:
#!/bin/bash
# Check if validator is in the imports and from where
rg -nP 'package\s+validator|func\s+IsAlphanumeric' --type=go -A 5Repository: tinh-tinh/sqlorm
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Search for column usages with underscores in builder method calls
rg -nP '\.(Equal|Not|Or|In|MoreThan|LessThan|Like|ILike|Between|NotEqual|NotIn|IsNull)\s*\(\s*["\x60][a-zA-Z_][a-zA-Z0-9_]*' --type=goRepository: tinh-tinh/sqlorm
Length of output: 744
🏁 Script executed:
#!/bin/bash
# Check the import statement for the validator package
head -20 builder.go | grep -E 'import|validator'Repository: tinh-tinh/sqlorm
Length of output: 118
🏁 Script executed:
#!/bin/bash
# Search for any documentation, comments, or examples about column validation
rg -nP 'column|Column' --type=go builder.go -B 2 -A 2Repository: tinh-tinh/sqlorm
Length of output: 3692
🏁 Script executed:
#!/bin/bash
# Look for the validator package files in vendor or any local definitions
find . -type f -name "validator.go" -o -name "validator" -type dRepository: tinh-tinh/sqlorm
Length of output: 42
🌐 Web query:
github.com/tinh-tinh/tinhtinh validator IsAlphanumeric documentation
💡 Result:
Do you mean the IsAlphanumeric validator inside the TinhTinh repo (github.com/tinh-tinh/tinhtinh) or the isAlphanumeric from validator.js (npm/validator)? I can look up the specific documentation — which one should I fetch?
🏁 Script executed:
#!/bin/bash
# Check go.mod for validator package details
cat go.mod | grep -E 'tinhtinh|require'Repository: tinh-tinh/sqlorm
Length of output: 120
🏁 Script executed:
#!/bin/bash
# Check if there's any documentation in comments about column validation
rg -nP '//.*column|/\*.*column' --type=go -iRepository: tinh-tinh/sqlorm
Length of output: 42
🌐 Web query:
TinhTinh Go validator IsAlphanumeric dto package behavior implementation
💡 Result:
I’m not finding a clear match for “TinhTinh” + Go validator. Do you mean a specific GitHub repo or package (e.g., govalidator / go-playground/validator), or do you want how an IsAlphanumeric validator should behave/be implemented for a DTO in Go? If you point me to the repo or confirm which you mean, I’ll fetch the code and explain the behavior and implementation.
🏁 Script executed:
#!/bin/bash
# Try to find more about the validator package in go.sum
cat go.sum | grep tinhtinhRepository: tinh-tinh/sqlorm
Length of output: 242
🌐 Web query:
site:github.com tinh-tinh/tinhtinh dto validator IsAlphanumeric
💡 Result:
I couldn't find a GitHub file matching "site:github.com tinh-tinh/tinhtinh dto validator IsAlphanumeric". The tinh-tinh organization and tinhtinh modules exist (Go packages) but no direct DTO validator/IsAlphanumeric hit under tinhtinh was returned by the search. [1][2]
If you want, I can:
- search the tinhtinh repo tree on GitHub for "validator", "dto" or "IsAlphanumeric"; or
- search more broadly for DTO validators (e.g., class-validator, validator.js) and examples of an "IsAlphanumeric" check.
Sources:
[1] Libraries.io entry for github.com/tinh-tinh/tinhtinh.
[2] validator.js (string validator reference).
🌐 Web query:
IsAlphanumeric validator Go implementation standard behavior
💡 Result:
Short answer: no — in the common Go validator (go-playground/validator) the plain "alphanum" / IsAlphanumeric validator is ASCII-only; there is a separate "alphanumunicode" validator that accepts Unicode letters/digits. [1][2]
Sources:
- go-playground validator baked-in validators list showing "alphanum" and "alphanumunicode". [1]
- go-playground/validator README (validators list / docs). [2]
Critical: IsAlphanumeric validation will reject valid SQL column names with underscores.
Standard IsAlphanumeric validators only accept [a-zA-Z0-9] and will reject snake_case column names like user_id, created_at, first_name — which are the standard SQL naming convention. This will silently fail at runtime (returns q without error), making bugs difficult to debug.
Either expand the validation to allow underscores and other valid SQL identifier characters, or document why this restrictive validation is intentional.
🤖 Prompt for AI Agents
In builder.go around lines 166 to 168, isValidColumn currently uses
validator.IsAlphanumeric which rejects common SQL identifiers containing
underscores; change the validation to allow underscores and valid identifier
forms (e.g. use a regex like ^[A-Za-z_][A-Za-z0-9_]*$ or a configurable pattern
that permits underscores and digits after the first character), and ensure
callers return an explicit error when a column name fails validation instead of
silently proceeding; update tests/docs accordingly.
ren0503
force-pushed
the
fix/ren/84-validate-column-before-query
branch
from
ee82d5e to
ed0da54
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @builder_test.go:
- Line 188: Replace the incorrect type reference sqlorm.Model with gorm.Model in
the struct definition (the line that currently reads sqlorm.Model
`gorm:"embedded"`); update the embedded field to use GORM's Model type so it
matches the earlier test usage and compiles (search for the embedded declaration
in builder_test.go and change sqlorm.Model to gorm.Model).
🧹 Nitpick comments (3)
builder_test.go (3)
197-204: Consider unconditional test data setup for reliability.The conditional creation (
if count == 0) makes the test dependent on database state. If the database isn't properly cleaned between runs, test behavior may change.♻️ Proposed approach for deterministic test data
Consider either:
- Truncating the table before each test to ensure a clean state
- Using a unique database name per test run
- Explicitly creating known test data regardless of existing count
Example:
- // Create test data - count, err := repo.Count(nil) - require.Nil(t, err) - - if count == 0 { - _, err = repo.Create(&TestEntity{Name: "valid", Value: 1}) - require.Nil(t, err) - } + // Ensure clean state and create test data + db.Exec("TRUNCATE TABLE test_entities") + _, err = repo.Create(&TestEntity{Name: "valid", Value: 1}) + require.Nil(t, err)
238-390: Consider refactoring to reduce test duplication.All 13 sub-tests follow an identical pattern with only the QueryBuilder method changing. While the current structure is readable, a table-driven approach could reduce duplication and make it easier to add new methods in the future.
♻️ Table-driven test example
testCases := []struct { name string method func(*sqlorm.QueryBuilder, string) }{ {"Equal", func(qb *sqlorm.QueryBuilder, col string) { qb.Equal(col, "test") }}, {"Not", func(qb *sqlorm.QueryBuilder, col string) { qb.Not(col, "test") }}, {"Or", func(qb *sqlorm.QueryBuilder, col string) { qb.Or(col, "test") }}, // ... other methods } for _, tc := range testCases { t.Run(tc.name+"_InvalidColumn", func(t *testing.T) { for _, invalidCol := range invalidColumns { docs, err := repo.FindAll(func(qb *sqlorm.QueryBuilder) { tc.method(qb, invalidCol) }) require.Nil(t, err) require.Equal(t, totalCount, int64(len(docs)), "Invalid column %q should not filter results", invalidCol) } }) }
392-408: Expand valid column test coverage.The valid column tests only cover
EqualandMoreThan, while the invalid column tests cover 13 different methods. Consider adding valid column tests for the remaining methods to ensure validation doesn't break any functionality.This ensures comprehensive validation that the column validation logic doesn't inadvertently block legitimate queries for methods like
Like,Between,NotIn, etc.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
builder.gobuilder_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- builder.go
🧰 Additional context used
🧬 Code graph analysis (1)
builder_test.go (5)
module.go (1)
Config(20-27)model.go (1)
Model(11-16)repository.go (1)
Repository(20-22)query_test.go (1)
Count(75-80)builder.go (1)
QueryBuilder(23-25)
🪛 GitHub Actions: Go
builder_test.go
[error] 188-188: undefined: sqlorm.Model
🔇 Additional comments (2)
builder_test.go (2)
207-236: Comprehensive invalid column list.The test includes a thorough set of invalid column patterns covering SQL injection attempts and special characters. This provides good coverage for validation testing.
175-409: Test assumptions are correct and match the builder implementation.Verification confirms:
Invalid columns trigger early returns without errors: All query methods (
Equal,Not,Or,In,MoreThan, etc.) checkif !isValidColumn(column) { return q }, returning the unmodified QueryBuilder without applying filters.Queries return unfiltered results: Since no WHERE clause is added for invalid columns, all existing records are returned, matching the test expectation that
len(docs) >= 1.Validation uses
validator.IsAlphanumeric: TheisValidColumnfunction at line 166-168 of builder.go correctly validates usingvalidator.IsAlphanumeric(column), which accepts only letters and digits, rejecting all special characters tested in the invalid columns list.The test implementation is sound and accurately reflects the production code behavior.
| // Test Equal with invalid columns | ||
| t.Run("Equal_InvalidColumn", func(t *testing.T) { | ||
| for _, invalidCol := range invalidColumns { | ||
| docs, err := repo.FindAll(func(qb *sqlorm.QueryBuilder) { | ||
| qb.Equal(invalidCol, "test") | ||
| }) | ||
| require.Nil(t, err) | ||
| require.GreaterOrEqual(t, len(docs), 1, "Invalid column %q should not filter results", invalidCol) | ||
| } | ||
| }) | ||
|
|
||
| // Test Not with invalid columns | ||
| t.Run("Not_InvalidColumn", func(t *testing.T) { | ||
| for _, invalidCol := range invalidColumns { | ||
| docs, err := repo.FindAll(func(qb *sqlorm.QueryBuilder) { | ||
| qb.Not(invalidCol, "test") | ||
| }) | ||
| require.Nil(t, err) | ||
| require.GreaterOrEqual(t, len(docs), 1, "Invalid column %q should not filter results", invalidCol) | ||
| } | ||
| }) | ||
|
|
||
| // Test Or with invalid columns | ||
| t.Run("Or_InvalidColumn", func(t *testing.T) { | ||
| for _, invalidCol := range invalidColumns { | ||
| docs, err := repo.FindAll(func(qb *sqlorm.QueryBuilder) { | ||
| qb.Or(invalidCol, "test") | ||
| }) | ||
| require.Nil(t, err) | ||
| require.GreaterOrEqual(t, len(docs), 1, "Invalid column %q should not filter results", invalidCol) | ||
| } | ||
| }) | ||
|
|
||
| // Test In with invalid columns | ||
| t.Run("In_InvalidColumn", func(t *testing.T) { | ||
| for _, invalidCol := range invalidColumns { | ||
| docs, err := repo.FindAll(func(qb *sqlorm.QueryBuilder) { | ||
| qb.In(invalidCol, "test", "test2") | ||
| }) | ||
| require.Nil(t, err) | ||
| require.GreaterOrEqual(t, len(docs), 1, "Invalid column %q should not filter results", invalidCol) | ||
| } | ||
| }) | ||
|
|
||
| // Test MoreThan with invalid columns | ||
| t.Run("MoreThan_InvalidColumn", func(t *testing.T) { | ||
| for _, invalidCol := range invalidColumns { | ||
| docs, err := repo.FindAll(func(qb *sqlorm.QueryBuilder) { | ||
| qb.MoreThan(invalidCol, 0) | ||
| }) | ||
| require.Nil(t, err) | ||
| require.GreaterOrEqual(t, len(docs), 1, "Invalid column %q should not filter results", invalidCol) | ||
| } | ||
| }) | ||
|
|
||
| // Test MoreThanOrEqual with invalid columns | ||
| t.Run("MoreThanOrEqual_InvalidColumn", func(t *testing.T) { | ||
| for _, invalidCol := range invalidColumns { | ||
| docs, err := repo.FindAll(func(qb *sqlorm.QueryBuilder) { | ||
| qb.MoreThanOrEqual(invalidCol, 0) | ||
| }) | ||
| require.Nil(t, err) | ||
| require.GreaterOrEqual(t, len(docs), 1, "Invalid column %q should not filter results", invalidCol) | ||
| } | ||
| }) | ||
|
|
||
| // Test LessThan with invalid columns | ||
| t.Run("LessThan_InvalidColumn", func(t *testing.T) { | ||
| for _, invalidCol := range invalidColumns { | ||
| docs, err := repo.FindAll(func(qb *sqlorm.QueryBuilder) { | ||
| qb.LessThan(invalidCol, 100) | ||
| }) | ||
| require.Nil(t, err) | ||
| require.GreaterOrEqual(t, len(docs), 1, "Invalid column %q should not filter results", invalidCol) | ||
| } | ||
| }) | ||
|
|
||
| // Test LessThanOrEqual with invalid columns | ||
| t.Run("LessThanOrEqual_InvalidColumn", func(t *testing.T) { | ||
| for _, invalidCol := range invalidColumns { | ||
| docs, err := repo.FindAll(func(qb *sqlorm.QueryBuilder) { | ||
| qb.LessThanOrEqual(invalidCol, 100) | ||
| }) | ||
| require.Nil(t, err) | ||
| require.GreaterOrEqual(t, len(docs), 1, "Invalid column %q should not filter results", invalidCol) | ||
| } | ||
| }) | ||
|
|
||
| // Test Like with invalid columns | ||
| t.Run("Like_InvalidColumn", func(t *testing.T) { | ||
| for _, invalidCol := range invalidColumns { | ||
| docs, err := repo.FindAll(func(qb *sqlorm.QueryBuilder) { | ||
| qb.Like(invalidCol, "%test%") | ||
| }) | ||
| require.Nil(t, err) | ||
| require.GreaterOrEqual(t, len(docs), 1, "Invalid column %q should not filter results", invalidCol) | ||
| } | ||
| }) | ||
|
|
||
| // Test ILike with invalid columns | ||
| t.Run("ILike_InvalidColumn", func(t *testing.T) { | ||
| for _, invalidCol := range invalidColumns { | ||
| docs, err := repo.FindAll(func(qb *sqlorm.QueryBuilder) { | ||
| qb.ILike(invalidCol, "%TEST%") | ||
| }) | ||
| require.Nil(t, err) | ||
| require.GreaterOrEqual(t, len(docs), 1, "Invalid column %q should not filter results", invalidCol) | ||
| } | ||
| }) | ||
|
|
||
| // Test Between with invalid columns | ||
| t.Run("Between_InvalidColumn", func(t *testing.T) { | ||
| for _, invalidCol := range invalidColumns { | ||
| docs, err := repo.FindAll(func(qb *sqlorm.QueryBuilder) { | ||
| qb.Between(invalidCol, 0, 100) | ||
| }) | ||
| require.Nil(t, err) | ||
| require.GreaterOrEqual(t, len(docs), 1, "Invalid column %q should not filter results", invalidCol) | ||
| } | ||
| }) | ||
|
|
||
| // Test NotEqual with invalid columns | ||
| t.Run("NotEqual_InvalidColumn", func(t *testing.T) { | ||
| for _, invalidCol := range invalidColumns { | ||
| docs, err := repo.FindAll(func(qb *sqlorm.QueryBuilder) { | ||
| qb.NotEqual(invalidCol, "nonexistent") | ||
| }) | ||
| require.Nil(t, err) | ||
| require.GreaterOrEqual(t, len(docs), 1, "Invalid column %q should not filter results", invalidCol) | ||
| } | ||
| }) | ||
|
|
||
| // Test NotIn with invalid columns | ||
| t.Run("NotIn_InvalidColumn", func(t *testing.T) { | ||
| for _, invalidCol := range invalidColumns { | ||
| docs, err := repo.FindAll(func(qb *sqlorm.QueryBuilder) { | ||
| qb.NotIn(invalidCol, "nonexistent1", "nonexistent2") | ||
| }) | ||
| require.Nil(t, err) | ||
| require.GreaterOrEqual(t, len(docs), 1, "Invalid column %q should not filter results", invalidCol) | ||
| } | ||
| }) | ||
|
|
||
| // Test IsNull with invalid columns | ||
| t.Run("IsNull_InvalidColumn", func(t *testing.T) { | ||
| for _, invalidCol := range invalidColumns { | ||
| docs, err := repo.FindAll(func(qb *sqlorm.QueryBuilder) { | ||
| qb.IsNull(invalidCol) | ||
| }) | ||
| require.Nil(t, err) | ||
| require.GreaterOrEqual(t, len(docs), 1, "Invalid column %q should not filter results", invalidCol) | ||
| } | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Strengthen the test assertions to verify filters are ignored.
The current assertions use require.GreaterOrEqual(t, len(docs), 1, ...), which only confirms at least one result was returned. This doesn't definitively prove that the invalid column filter was ignored—it just shows some data came back.
A more robust approach would be to compare against the total count in the database, proving that no filtering occurred:
♻️ Stronger assertion approach
Before the sub-tests, capture the expected count:
+ // Get total count for validation that filters are ignored
+ totalCount, err := repo.Count(nil)
+ require.Nil(t, err)
+
// Test Equal with invalid columns
t.Run("Equal_InvalidColumn", func(t *testing.T) {
for _, invalidCol := range invalidColumns {
docs, err := repo.FindAll(func(qb *sqlorm.QueryBuilder) {
qb.Equal(invalidCol, "test")
})
require.Nil(t, err)
- require.GreaterOrEqual(t, len(docs), 1, "Invalid column %q should not filter results", invalidCol)
+ require.Equal(t, totalCount, int64(len(docs)), "Invalid column %q should not filter results", invalidCol)
}
})Apply the same pattern to all other sub-tests.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @builder_test.go:
- Line 188: Replace the incorrect embedded type sqlorm.Model with gorm.Model
where it's used (the struct embedding on the line containing `sqlorm.Model`) so
the test compiles; update the embedded tag to use `gorm.Model
\`gorm:"embedded"\`` (i.e., change the type name only) and run `go test` to
verify compilation.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
builder_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
builder_test.go (4)
module.go (1)
Config(20-27)repository.go (1)
Repository(20-22)query_test.go (1)
Count(75-80)builder.go (1)
QueryBuilder(23-25)
🪛 GitHub Actions: Go
builder_test.go
[error] 188-188: Command 'go test -cover -coverprofile=coverage.txt ./...' failed with exit code 1. Error: undefined: sqlorm.Model in builder_test.go:188:10.
🔇 Additional comments (4)
builder_test.go (4)
175-185: LGTM! Clean test setup.The test database setup follows the established pattern and properly isolates this test with a separate database name.
192-204: LGTM! Proper migration and seeding logic.The conditional seeding ensures test data exists without creating duplicates on repeated runs.
207-236: Excellent comprehensive coverage of invalid column names.The test cases cover SQL injection attempts, special characters, and various problematic patterns, ensuring robust validation of the column name sanitization logic.
392-408: LGTM! Important regression tests for valid columns.These tests ensure that the new validation logic doesn't break legitimate column name usage, with strong assertions verifying exact expected behavior.
ren0503
force-pushed
the
fix/ren/84-validate-column-before-query
branch
from
434f956 to
190036d
Compare
2 participants
No description provided.