feat: add builtin validation rules and filters#1416
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@claude review |
|
@copilot review |
📝 WalkthroughWalkthroughThis PR implements a comprehensive validation framework by populating rule definitions, filter functions, validation messages, and utility helpers. It includes extensive test coverage through new test files and refactored existing tests, plus integration with the ORM facade. Core validation behavior is preserved with backward compatibility through deprecated rule aliases. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can disable poems in the walkthrough.Disable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
validation/utils.go (1)
423-435: Potential edge case: non-standard types may unexpectedly match as "declined".For non-string types,
cast.ToInt(val)returns 0 when the value can't be meaningfully converted (e.g., slices, maps, structs). This causes the function to returntrue, treating arbitrary non-castable values as "declined".While this is unlikely to affect typical form validation (strings/booleans/integers), it creates an asymmetry with
isAcceptedValuewhich returnsfalsefor such types.Consider adding an explicit boolean type check to match
isAcceptedValue's behavior:♻️ Proposed fix to add explicit boolean handling
func isDeclinedValue(val any) bool { if val == nil { return false } switch v := val.(type) { case string: v = strings.ToLower(strings.TrimSpace(v)) return v == "no" || v == "off" || v == "0" || v == "false" + case bool: + return !v } v := cast.ToInt(val) return v == 0 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@validation/utils.go` around lines 423 - 435, isDeclinedValue currently treats any non-string, non-bool/castable value as declined because cast.ToInt returns 0 for non-castable types; update isDeclinedValue to mirror isAcceptedValue's type handling: add an explicit type switch to handle bool (return true for false), numeric types (int/float -> compare to 0), and for any other non-castable types return false instead of using cast.ToInt blindly; reference the isDeclinedValue function and align behavior with isAcceptedValue to avoid treating slices/maps/structs as "declined".validation/rules.go (1)
973-985: Document the DNS lookup behavior of theactive_urlrule to inform users of I/O implications.
ruleActiveUrlperforms a synchronous DNS lookup viaresolver.LookupHost(). This blocking I/O operation can introduce latency on validation hot paths, potentially causing request timeouts if DNS resolution is slow. Consider adding documentation or code comments explaining this behavior so users understand the performance characteristics when using theactive_urlrule.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@validation/rules.go` around lines 973 - 985, The ruleActiveUrl function performs a synchronous DNS lookup using net.Resolver.LookupHost which is blocking I/O and can add latency to validation hot paths; add a clear code comment above ruleActiveUrl describing that it performs a blocking DNS resolution (via resolver.LookupHost / u.Hostname()), may block request handling, can cause timeouts, and suggest alternatives (e.g., async/timeout-wrapped validation, caching, or a non-networking variant) so callers are aware of the performance implications.validation/errors_test.go (1)
189-229: Consider adding a test case forHas()returning false on a non-existent field.The current test only verifies
Has("a")returnstruewhen errors exist. Adding a case that checksHas("nonexistent")returnsfalsewould improve coverage.💡 Suggested additional test case
{ describe: "errors isn't empty", data: map[string]any{"c": "cc"}, rules: map[string]any{"a": "required", "b": "required"}, filters: map[string]any{"a": "trim", "b": "trim"}, expectRes: true, }, + { + describe: "Has returns false for non-existent field", + data: map[string]any{"c": "cc"}, + rules: map[string]any{"a": "required"}, + filters: map[string]any{"a": "trim"}, + expectRes: true, + }, }Then in the loop, add a check:
// Also verify Has returns false for non-existent field assert.False(t, errors.Has("nonexistent"), test.describe)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@validation/errors_test.go` around lines 189 - 229, Add an assertion in TestHas to verify that Errors().Has returns false for a non-existent field: after creating the validator via NewValidation().Make and obtaining errors := validator.Errors(), keep the existing NotNil/Has("a") checks and also assert that errors.Has("nonexistent") is false (or use assert.False) for the same test cases where errors are present; this should reference the TestHas function, the validator variable from Make, and the Errors().Has method so the new assertion is added inside the t.Run subtest after errors is obtained.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@validation/errors_test.go`:
- Around line 189-229: Add an assertion in TestHas to verify that Errors().Has
returns false for a non-existent field: after creating the validator via
NewValidation().Make and obtaining errors := validator.Errors(), keep the
existing NotNil/Has("a") checks and also assert that errors.Has("nonexistent")
is false (or use assert.False) for the same test cases where errors are present;
this should reference the TestHas function, the validator variable from Make,
and the Errors().Has method so the new assertion is added inside the t.Run
subtest after errors is obtained.
In `@validation/rules.go`:
- Around line 973-985: The ruleActiveUrl function performs a synchronous DNS
lookup using net.Resolver.LookupHost which is blocking I/O and can add latency
to validation hot paths; add a clear code comment above ruleActiveUrl describing
that it performs a blocking DNS resolution (via resolver.LookupHost /
u.Hostname()), may block request handling, can cause timeouts, and suggest
alternatives (e.g., async/timeout-wrapped validation, caching, or a
non-networking variant) so callers are aware of the performance implications.
In `@validation/utils.go`:
- Around line 423-435: isDeclinedValue currently treats any non-string,
non-bool/castable value as declined because cast.ToInt returns 0 for
non-castable types; update isDeclinedValue to mirror isAcceptedValue's type
handling: add an explicit type switch to handle bool (return true for false),
numeric types (int/float -> compare to 0), and for any other non-castable types
return false instead of using cast.ToInt blindly; reference the isDeclinedValue
function and align behavior with isAcceptedValue to avoid treating
slices/maps/structs as "declined".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 352e082b-4f47-40c1-b900-404a39e90d84
📒 Files selected for processing (14)
validation/benchmark_test.govalidation/errors_test.govalidation/filters.govalidation/filters_test.govalidation/messages.govalidation/options_test.govalidation/rules.govalidation/rules_test.govalidation/service_provider.govalidation/utils.govalidation/utils_test.govalidation/validation.govalidation/validation_test.govalidation/validator_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1416 +/- ##
==========================================
+ Coverage 65.94% 67.82% +1.87%
==========================================
Files 353 354 +1
Lines 26083 27345 +1262
==========================================
+ Hits 17201 18546 +1345
+ Misses 8141 7952 -189
- Partials 741 847 +106 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
hwbrzzl
left a comment
hwbrzzl
left a comment
There was a problem hiding this comment.
Thanks, awesome. Only a few questions.
| } | ||
| } | ||
| } | ||
| return false |
There was a problem hiding this comment.
How about []int, []float, etc.?
There was a problem hiding this comment.
ruleArray 使用 reflect.ValueOf(val).Kind(), 所以 []int, []float64, []string 之类的会返回 reflect.Slice，已经支持了。
reflect.ValueOf([]int{1,2}).Kind() // reflect.Slice ✓
reflect.ValueOf([]float64{}).Kind() // reflect.Slice ✓
reflect.ValueOf([3]int{}).Kind() // reflect.Array ✓There was a problem hiding this comment.
已有 []int, []string 的测试用例，补充了 []float64, []bool, [3]int（固定长度数组）的 case，全部通过。
ruleArray 使用 reflect.ValueOf(val).Kind()，Go 的所有切片类型（[]int, []float64, []bool 等）Kind 都是 reflect.Slice，固定数组 [N]T 的 Kind 是 reflect.Array，都已覆盖。
| // numericRuleNames are rules that indicate a field should be treated as numeric for size rules. | ||
| var numericRuleNames = map[string]bool{} | ||
| var numericRuleNames = map[string]bool{ | ||
| "numeric": true, "integer": true, "decimal": true, |
There was a problem hiding this comment.
numeric alias rules are incomplete for size/type resolution (int, uint, float are missing from numericRuleNames), so size/comparison rules can be evaluated as string-length instead of numeric value when alias rules are used with string inputs.
Example impact: int|max:3 with "42" can pass via string length (2 <= 3) instead of failing numerically (42 <= 3).
There was a problem hiding this comment.
Good catch! 已将 int, uint, float 加入 numericRuleNames，并补充了 TestNumericAliasesWithSizeRules 测试覆盖这个场景。
关键 case: int|max:3 + "42" 现在正确按数值 42 > 3 判断失败，而不是按字符串长度 2 <= 3 通过。
| rules map[string]any | ||
| fails bool | ||
| }{ | ||
| {"pass_google", map[string]any{"u": "https://google.com"}, map[string]any{"u": "active_url"}, false}, |
There was a problem hiding this comment.
https://goravel.dev can be used here instead of https://google.com, given google can't be visit in Mainland China.
There was a problem hiding this comment.
Done, 已改为 https://goravel.dev。
- Add int, uint, float to numericRuleNames for correct size resolution
- Replace google.com with goravel.dev in active_url test
- Add explicit bool handling in isAcceptedValue/isDeclinedValue
- Add DNS lookup comment on ruleActiveUrl
- Add Has("nonexistent") assertion in TestHas
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Verify int, uint, float aliases resolve size rules as numeric values instead of string length (e.g. int|max:3 with "42" correctly fails). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Cover []float64, []bool, and [N]int fixed array types. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
hwbrzzl
left a comment
hwbrzzl
left a comment
There was a problem hiding this comment.
LGTM, could you help optimize goravel/gin and goravel/fiber as well? And FYI, I created a PR to test the original validation, and will test the new one using the test cases once goravel/gin and goravel/fiber are completed: goravel/example#111
2 participants
Summary
Close goravel/goravel#693
Add all builtin validation rules, filters, and their backward-compatible aliases from the old gookit/validate-based implementation.
This PR also introduces a deprecation plan: all camelCase-named rules and filters (e.g.
eq_field→same,trimSpace→trim,toInt→to_int) are kept as backward-compatible aliases but marked as deprecated. They will be removed in the next major version. Going forward, all rule and filter names follow snake_case convention to stay consistent with Laravel and reduce maintenance overhead.Validation Rules
New Rules (not in old version)
required_if_acceptedrequired_if_declinedfilledpresentpresent_ifpresent_unlesspresent_withpresent_with_allmissingmissing_ifmissing_unlessmissing_withmissing_with_allacceptedaccepted_ifdeclineddeclined_ifprohibitedprohibited_ifprohibited_unlessprohibited_if_acceptedprohibited_if_declinedprohibitsintegerint)booleanbool)listslice)sizegteltedigitsdigits_betweendecimalmultiple_ofmin_digitsmax_digitsasciiactive_urlmac_address/maculidhex_colornot_regexlowercaseuppercasedoesnt_start_withdoesnt_end_withcontainsdoesnt_containconfirmedsamedifferentin_arrayin_array_keysdate_formatdate_equalsbeforebefore_or_equalafterafter_or_equaltimezoneexcludeexclude_ifexclude_unlessexclude_withexclude_withoutmimesmimetypesextensionsdimensionsencodingbailnullablesometimesdistinctrequired_array_keysexistsuniqueKept Rules (from old version, same or enhanced)
requiredrequiredrequired_ifrequired_ifrequired_unlessrequired_unlessrequired_withrequired_withrequired_with_allrequired_with_allrequired_withoutrequired_withoutrequired_without_allrequired_without_allstringstringinteger/intintintegeras primary name,intas Go aliasuintuintnumericnumberboolean/boolboolbooleanas primary namefloatfloatsliceslicelistarrayarraymapmapininnot_innot_instarts_withstarts_withends_withends_withbetweenbetweenminminmin_len, type-awaremaxmaxmax_len, type-awaresizelenlen→size(Laravel naming)gtgt+gt_fieldgtegte_fieldltlt+lt_fieldltelte_fieldeqeqnenesameeq_fielddifferentne_fieldemailemailurlfull_urlipipipv4ipv4ipv6ipv6jsonjsonalphaalphaalpha_numalpha_numalpha_dashalpha_dashregexregexuuiduuiduuid3uuid3uuid4uuid4uuid5uuid5datedateaftergt_datebeforelt_dateafter_or_equalgte_datebefore_or_equallte_datefilefileimageimageDeprecated Rules (backward-compatible, will be removed next version)
lensizemin_lenminminmax_lenmaxmaxeq_fieldsamene_fielddifferentgt_fieldgtgtgte_fieldgtegtelt_fieldltltlte_fieldlteltegt_dateafterlt_datebeforegte_dateafter_or_equallte_datebefore_or_equalnumbernumericfull_urlurlFilters
Primary Filters (snake_case)
trimltrimrtrimloweruppertitleucfirstlcfirstcamelsnaketo_intto_int64to_uintto_floatto_boolto_stringto_timestrip_tagsescape_jsescape_htmlurl_encodeurl_decodestr_to_intsstr_to_arraystr_to_timeShort Name Aliases
intto_intint64to_int64uintto_uintfloatto_floatboolto_boolOld Filters → New Filters Mapping
int/toIntto_int(orint)toIntdeprecateduint/toUintto_uint(oruint)toUintdeprecatedint64/toInt64to_int64(orint64)toInt64deprecatedfloat/toFloatto_float(orfloat)toFloatdeprecatedbool/toBoolto_bool(orbool)toBooldeprecatedtrim/trimSpacetrimtrimSpacedeprecatedltrim/trimLeftltrimtrimLeftdeprecatedrtrim/trimRightrtrimtrimRightdeprecatedlower/lowercaselowerlowercasedeprecatedupper/uppercaseupperuppercasedeprecatedlcFirst/lowerFirstlcfirstlcFirst/lowerFirstdeprecateducFirst/upperFirstucfirstucFirst/upperFirstdeprecateducWord/upperWordtitleucWord/upperWorddeprecatedcamel/camelCasecamelcamelCasedeprecatedsnake/snakeCasesnakesnakeCasedeprecatedescapeJs/escapeJSescape_jsescapeJs/escapeJSdeprecatedescapeHtml/escapeHTMLescape_htmlescapeHtml/escapeHTMLdeprecatedstr2ints/strToIntsstr_to_intsstr2ints/strToIntsdeprecatedstr2time/strToTimestr_to_timestr2time/strToTimedeprecatedstr2arr/str2array/strToArraystr_to_arrayTest Plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests