refactor: implement new validation engine infrastructure#1397
Conversation
Replace gookit/validate with a native validation engine that is fully compatible with Laravel's validation API. This PR introduces the core infrastructure without builtin rule/filter implementations (to be added in a follow-up PR). Key changes: - New DataBag for unified data access (JSON, form, multipart, query) - New Engine as the core validation orchestrator - New RuleParser for parsing rule strings and slice syntax - New filter infrastructure with custom filter support via reflection - New message system with type-specific error templates - Contracts updated: map[string]string -> map[string]any for rules/filters - Remove gookit/validate dependency Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1397 +/- ##
==========================================
- Coverage 67.18% 65.95% -1.24%
==========================================
Files 348 353 +5
Lines 25609 26083 +474
==========================================
- Hits 17206 17203 -3
- Misses 7674 8140 +466
- Partials 729 740 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Thanks, awesome. Could you list the break changes in the PR description? And list the to do items as well. |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis PR refactors the validation system from an external library dependency (gookit/validate) to an internal implementation. Rule and filter types shift from Changes
Sequence DiagramsequenceDiagram
participant Client
participant Validation
participant DataBag
participant Engine
participant Rules
participant ErrorBag
Client->>Validation: Make(ctx, data, rules, options)
Validation->>DataBag: NewDataBag/NewDataBagFromRequest
DataBag-->>Validation: *DataBag instance
Validation->>Validation: Parse rules (ParseRules)
Validation->>Engine: NewEngine(ctx, data, parsedRules, options)
Engine-->>Validation: *Engine instance
Validation->>Engine: Validate()
Engine->>Engine: expandWildcardRules
Engine->>Engine: validateField (for each field)
Engine->>Rules: executeRule
Rules->>DataBag: Get value
DataBag-->>Rules: field value
Rules-->>Engine: validation result
alt Validation fails
Engine->>ErrorBag: Add error
end
Engine->>Engine: ValidatedData()
Engine-->>Validation: *Errors
Validation-->>Client: Validator instance with errors
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 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 unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mocks/validation/Option.go (1)
10-26:⚠️ Potential issue | 🟠 MajorRemove or fix the invalid mock generation for
Optionfunction type.The contract defines
Optionas a function type (type Option func(*Options), line 14 incontracts/validation/validation.go), not an interface. The generated mock implements it as a struct with anExecutemethod, which cannot satisfy the function type expected by the validation code.The
.mockery.yamlconfiguration'sall: Truesetting causes mockery to generate mocks for non-interface types like function aliases. Either exclude function types from mock generation or configure mockery to only mock actual interfaces. This mock struct is not used anywhere in the codebase and represents dead code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mocks/validation/Option.go` around lines 10 - 26, The generated mock for the function type Option is invalid: Option is declared as a function alias (type Option func(*Options)) in contracts/validation/validation.go but mocks/validation/Option.go contains a struct Option with method Execute, which cannot satisfy a function type; fix by removing this mock file or reconfiguring mock generation to exclude non-interface types (adjust .mockery.yaml to not use all: true or add an exclude for function aliases), and if a mock is still needed create a proper function-typed mock helper (or wrap it with a variable of type Option that calls a mocked function) instead of the struct Option/Option_Expecter/Execute pattern so code expecting Option(func(*validation.Options)) compiles.
🧹 Nitpick comments (6)
validation/errors_test.go (1)
9-87: Use atestify/suitefor these unit tests.The coverage is fine, but the repo’s unit-test convention prefers suites over standalone test functions.
As per coding guidelines "Co-locate unit tests with source files using *_test.go naming convention and testify suite and mock libraries".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@validation/errors_test.go` around lines 9 - 87, Convert the standalone table tests into a testify/suite-based test file: create a test suite type (e.g., ErrorsTestSuite) that embeds suite.Suite, move the setup (instantiating NewErrors) into a SetupTest method if needed, and convert each TestErrors_* function into a suite method (e.g., TestOne, TestGet, TestAll, TestHas, TestIsEmpty) that uses s.Require()/s.Assert() instead of t; register the suite with suite.Run in a single TestErrorsSuite entrypoint. Ensure all assertions still exercise NewErrors and its methods Add, One, Get, All, Has, and IsEmpty and that the file keeps the *_test.go naming convention and imports testify/suite.validation/options_test.go (1)
12-111: Use atestify/suitefor these option unit tests.The test coverage is fine, but the repository test guidelines prefer suites for unit tests.
As per coding guidelines "Co-locate unit tests with source files using *_test.go naming convention and testify suite and mock libraries".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@validation/options_test.go` around lines 12 - 111, Convert the standalone unit tests into a testify suite: create a suite struct (e.g., OptionsTestSuite) that embeds suite.Suite, move the logic from TestFiltersOption, TestCustomFiltersOption, TestMessagesOption, TestAttributesOption, TestPrepareForValidationOption and the subtests in TestApplyOptions into suite methods (e.g., TestFilters, TestCustomFilters, TestMessages, TestAttributes, TestPrepareForValidation, TestApplyOptions) that use suite.Require()/suite.Assert(), and add a top-level TestOptionsTestSuite function that calls suite.Run(t, new(OptionsTestSuite)); keep assertions the same and reference existing helpers/constructors (Filters, CustomFilters, Messages, Attributes, PrepareForValidation, applyOptions) inside the suite methods.validation/rule_parser_test.go (1)
9-401: Use atestify/suitefor the parser unit tests.The cases are useful, but the repository test guidelines prefer suites for unit tests.
As per coding guidelines "Co-locate unit tests with source files using *_test.go naming convention and testify suite and mock libraries".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@validation/rule_parser_test.go` around lines 9 - 401, Replace the free-standing test functions (TestParseRules, TestParseRuleSlice, TestSplitRules, TestExtractRuleName, TestParseOneRule, TestSplitParameters) with a testify suite: define a ParserTestSuite struct embedding suite.Suite, convert each Test* function into a method on *ParserTestSuite (e.g., func (s *ParserTestSuite) TestParseRules() { ... } ) using s.Equal / s.Require as assertions, and add a top-level TestParserTestSuite(t *testing.T) that calls suite.Run(t, new(ParserTestSuite)); keep the existing test cases and helper calls (ParseRules, ParseRuleSlice, splitRules, extractRuleName, parseOneRule, splitParameters) intact inside the suite methods and import github.com/stretchr/testify/suite.contracts/http/request.go (1)
97-113: Clarify the allowedmap[string]anyvalue shapes in the contract.This public API now accepts
map[string]any, but the new validation errors only describestringand[]stringas valid rule/filter values. Without that constraint here, callers can satisfy the type checker and still hit runtime type errors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/http/request.go` around lines 97 - 113, The contract currently accepts map[string]any but doesn't specify allowed value shapes, which lets callers pass unsupported types and causes runtime validation errors; update the doc comments for Validate, ValidateRequest, FormRequest.Rules and FormRequestWithFilters.Filters to explicitly state that map values must be either string or []string (the shapes the validator supports) and callers must use those shapes (or a clearly documented nested rule structure if you support it) so the type checker plus the docs prevent runtime type errors; reference the methods Validate, ValidateRequest and the interfaces FormRequest and FormRequestWithFilters in the comments to make the constraint explicit.validation/filters.go (2)
123-137: Extra parameters silently dropped for non-variadic filter functions.When more parameters are provided than the function accepts, they are silently ignored (line 127-129). This is acceptable behavior, but consider documenting this in the function comment for clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@validation/filters.go` around lines 123 - 137, Update the function comment for the filter invocation logic to explicitly state that when calling a non-variadic filter, any extra parameters in params beyond the function's declared inputs are silently ignored; mention the behavior seen in the loop that checks fnType.NumIn(), uses argIdx, calls convertToType and appends to args, and breaks when argIdx >= fnType.NumIn(). Make the comment concise and placed above the function (or the relevant block) so callers/readers know that extra params are dropped for non-variadic filter functions.
72-72: Consider handlingbag.Seterror.The error from
bag.Setis discarded. While unlikely to fail for in-memory operations, ignoring errors makes debugging harder if the DataBag implementation changes.♻️ Proposed fix
- _ = bag.Set(field, val) + if err := bag.Set(field, val); err != nil { + return fmt.Errorf("failed to set filtered value for %s: %w", field, err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@validation/filters.go` at line 72, The discarded error from the call `_ = bag.Set(field, val)` should be handled: capture the returned error from `bag.Set(field, val)` and either return it to the caller or wrap it with context (e.g. using fmt.Errorf) and return, or log it with the function's logger if the surrounding function cannot return errors; include `field` and `val` in the error context to aid debugging. Locate the occurrence of `bag.Set`, replace the discard with error handling (`err := bag.Set(field, val)` ...), and ensure the surrounding function's signature and control flow propagate or surface the error appropriately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@validation/data_bag.go`:
- Around line 79-133: The handler in validation/data_bag.go is swallowing
request parse errors (io.ReadAll, supportjson.Unmarshal, r.ParseMultipartForm,
r.ParseForm) causing truncated or partial data to be validated; update the JSON
branch (strings.HasPrefix(contentType, "application/json")) to return the
read/unmarshal error instead of returning bag, nil when io.ReadAll or
supportjson.Unmarshal fail, and update the multipart and urlencoded branches to
return the parse error when r.ParseMultipartForm or r.ParseForm return non-nil
errors (i.e., do not ignore those errors in the multipart/form-data and
application/x-www-form-urlencoded cases); keep restoring r.Body only after a
successful read. Ensure the function returns (bag, err) consistently so
validation.Make can propagate parse failures.
- Around line 206-227: struct-backed nested values are left as raw Go types so
dotGet and collectKeys only see map[string]any, []any and []map[string]any and
therefore fail for nested structs, typed slices or maps; fix by recursively
normalizing nested values in structToMap (or adding reflective traversal used by
structToMap) so that any nested struct becomes map[string]any, any typed slice
becomes []any and any typed map becomes map[string]any before returning; update
structToMap to call a helper normalizeValue (or inline reflection) for fields,
and ensure dotGet and collectKeys continue to operate on the normalized
map[string]any / []any forms (also apply the same normalization logic to places
where structToMap output is consumed).
In `@validation/engine.go`:
- Around line 196-208: The current branch for unknown rules returns false which
treats typos/unregistered rules as normal validation failures; change this to
propagate an internal error instead: when rule.Name is not found in
builtinRules, return an explicit error (e.g., fmt.Errorf("unknown rule: %s",
rule.Name)) from the rule evaluation function rather than false, and update the
function signature and its callers (the methods that invoke this block and
consume its result) to accept and propagate that error so it surfaces as a
validator configuration/internal error (not added to the error bag). Ensure
references to builtinRules, rule.Name, e.ruleCtx remain intact and that callers
handle the new error return path.
- Around line 109-147: Run all exclude_* rules in a pre-pass before executing
any non-control validation rules to avoid order-dependency; specifically, after
collecting control rules (bail/nullable/sometimes) and after the "Sometimes"
presence check, iterate fieldRules and for each rule where
excludeRules[rule.Name] is true call e.handleExcludeRule(field, rule) and if
e.isExcluded(field) return/skip validating this field entirely. Remove or skip
exclude rule handling from the later per-rule loop so only non-exclude rules run
there.
- Around line 91-96: The expandWildcardRules method must merge expanded rule
slices instead of letting later entries overwrite earlier ones and must drop any
leftover unmatched wildcard entries; change Engine.expandWildcardRules to call
expandWildcardFields(e.rules, e.data.Keys(), true), iterate its result and for
each key append/merge the []ParsedRule into e.expandedRules (instead of
assigning/replacing), and skip any returned keys that still contain a wildcard
(e.g., strings.Contains(key, "*")) so unmatched wildcard paths are not kept.
In `@validation/errors.go`:
- Around line 5-7: The Errors type currently uses unordered maps (messages
map[string]map[string]string) so Errors.One and One(field) return
non-deterministic results; change the storage to preserve insertion order (e.g.,
messages map[string][]string or keep an ordered slice of field keys plus
per-field ordered slice of messages) and update all methods that add/read errors
(e.g., the constructor, Add/Append methods, One, All, and Any) to push messages
into the ordered lists and iterate that order when returning One() or One(field)
so the first-added message is deterministic.
In `@validation/filters.go`:
- Around line 63-69: The current block in applyFilters silently ignores errors
from callFilterFunc when applying customFilterMap[pf.Name]; change this so any
non-nil err from callFilterFunc is returned (with contextual info including
pf.Name) instead of swallowing it, and update applyFilters' signature/Callers if
necessary to propagate the error upward; alternatively, if you prefer to keep
processing, log the error via the existing logger and continue — but do not
ignore the error silently from callFilterFunc in the custom filter handling.
In `@validation/rule_parser_test.go`:
- Around line 60-65: The test shows the parser incorrectly consuming the rest of
the rule string after a regex/not_regex (e.g.,
"required|regex:^(foo|bar)$|string") making "string" part of the regex; update
the rule parser logic (the code that parses rules into ParsedRule entries) so
that when encountering "regex" or "not_regex" it does not silently swallow
trailing pipe-delimited rules — either: 1) reject the ambiguous form and return
an error for patterns containing unescaped/unquoted pipes after regex/not_regex,
or 2) require an explicit slice/array parameter syntax for regex/not_regex;
change the parser branch that handles rule names "regex" and "not_regex" to
detect and enforce the chosen behavior and adjust the tests (ParsedRule
expectations) accordingly to assert rejection or the slice format instead of
consuming the remainder.
In `@validation/rule_parser.go`:
- Around line 60-72: The parser in rule_parser.go currently treats any rule
identified as "regex" or "not_regex" by appending the rest of ruleString into
the same rule (using current.WriteByte and current.WriteString), which swallows
subsequent rules like "max:10"; update the logic in the parsing loop (the branch
that checks ruleName via extractRuleName(rule)) so that instead of
unconditionally consuming the remainder it (a) detects a properly delimited
regex parameter (e.g., a starting delimiter and a matching end delimiter or
explicit anchors) and only consumes up to that delimiter, or (b) if no clear
delimiter/end is found, rejects the ambiguous single-string form with a parse
error instructing callers to supply rules as a []string when the regex contains
pipes; ensure this change is made where ruleString, current, and extractRuleName
are used so subsequent rules are not swallowed.
In `@validation/utils.go`:
- Around line 29-48: getAttributeType currently only returns "numeric" when
numericRuleNames[r.Name] is true (and numericRuleNames is initialized empty), so
numeric runtime values slip through as "string"; update getAttributeType to also
inspect the runtime value kind when no numeric rule is present: if the value's
reflected kind is one of int/int8/.../uint/uint8/.../float32/float64 (or a
pointer/nullable to those) return "numeric". Reference getAttributeType,
numericRuleNames and the rules[attribute] loop to add this fallback check before
returning "string".
- Around line 14-23: The isValueEmpty function only checks concrete types
(string, []any, []string, map[string]any) so typed slices/maps (e.g., []int,
[]MyStruct, map[string]string) are treated as non-empty; update isValueEmpty to
use reflection: detect if val is nil, if it's a string trim and check empty, and
for any kind.Kind() == reflect.Slice || reflect.Array || reflect.Map return true
when reflect.Value.Len() == 0; keep existing cases for direct string handling
but replace the concrete []any/[]string/map checks with the generic
reflect-based length check so required/filled works for typed collections.
In `@validation/validation.go`:
- Around line 107-116: The new code routes all validation through NewEngine
(constructed with NewEngine(ctx, bag, parsedRules, engineOptions{...})) but
NewEngine currently lacks builtin rule/filter parity and returns false for
unknown rules, causing regressions; revert to preserving the old backend as a
fallback or gate the NewEngine path behind a feature flag: detect the flag (or
absence of builtinRules) before calling NewEngine, and if not enabled, call the
previous Make/engine creation path that produced the prior
errorBag/validatedData, otherwise use NewEngine and continue to call
engine.Validate() and engine.ValidatedData() and then return NewValidator(bag,
errorBag, validatedData). Ensure the check references NewEngine,
engine.Validate, engine.ValidatedData and NewValidator so the fallback/flag is
applied exactly where the switch currently happens.
In `@validation/validator.go`:
- Around line 33-42: The merge currently copies v.validated entries directly
into a flat map built from v.data.All(), which leaves dotted keys like
"user.name" as literal keys and doesn't update nested structures for Bind();
instead iterate v.validated and apply each entry through the DataBag.Set method
(or reconstruct a nested map by splitting dot-paths and creating nested maps) so
nested fields overwrite correctly, then decode merged.All() (or the rebuilt map)
into the target; update the merge logic that uses v.data.All() and v.validated
to call DataBag.Set (or build nested maps) before decoding.
---
Outside diff comments:
In `@mocks/validation/Option.go`:
- Around line 10-26: The generated mock for the function type Option is invalid:
Option is declared as a function alias (type Option func(*Options)) in
contracts/validation/validation.go but mocks/validation/Option.go contains a
struct Option with method Execute, which cannot satisfy a function type; fix by
removing this mock file or reconfiguring mock generation to exclude
non-interface types (adjust .mockery.yaml to not use all: true or add an exclude
for function aliases), and if a mock is still needed create a proper
function-typed mock helper (or wrap it with a variable of type Option that calls
a mocked function) instead of the struct Option/Option_Expecter/Execute pattern
so code expecting Option(func(*validation.Options)) compiles.
---
Nitpick comments:
In `@contracts/http/request.go`:
- Around line 97-113: The contract currently accepts map[string]any but doesn't
specify allowed value shapes, which lets callers pass unsupported types and
causes runtime validation errors; update the doc comments for Validate,
ValidateRequest, FormRequest.Rules and FormRequestWithFilters.Filters to
explicitly state that map values must be either string or []string (the shapes
the validator supports) and callers must use those shapes (or a clearly
documented nested rule structure if you support it) so the type checker plus the
docs prevent runtime type errors; reference the methods Validate,
ValidateRequest and the interfaces FormRequest and FormRequestWithFilters in the
comments to make the constraint explicit.
In `@validation/errors_test.go`:
- Around line 9-87: Convert the standalone table tests into a
testify/suite-based test file: create a test suite type (e.g., ErrorsTestSuite)
that embeds suite.Suite, move the setup (instantiating NewErrors) into a
SetupTest method if needed, and convert each TestErrors_* function into a suite
method (e.g., TestOne, TestGet, TestAll, TestHas, TestIsEmpty) that uses
s.Require()/s.Assert() instead of t; register the suite with suite.Run in a
single TestErrorsSuite entrypoint. Ensure all assertions still exercise
NewErrors and its methods Add, One, Get, All, Has, and IsEmpty and that the file
keeps the *_test.go naming convention and imports testify/suite.
In `@validation/filters.go`:
- Around line 123-137: Update the function comment for the filter invocation
logic to explicitly state that when calling a non-variadic filter, any extra
parameters in params beyond the function's declared inputs are silently ignored;
mention the behavior seen in the loop that checks fnType.NumIn(), uses argIdx,
calls convertToType and appends to args, and breaks when argIdx >=
fnType.NumIn(). Make the comment concise and placed above the function (or the
relevant block) so callers/readers know that extra params are dropped for
non-variadic filter functions.
- Line 72: The discarded error from the call `_ = bag.Set(field, val)` should be
handled: capture the returned error from `bag.Set(field, val)` and either return
it to the caller or wrap it with context (e.g. using fmt.Errorf) and return, or
log it with the function's logger if the surrounding function cannot return
errors; include `field` and `val` in the error context to aid debugging. Locate
the occurrence of `bag.Set`, replace the discard with error handling (`err :=
bag.Set(field, val)` ...), and ensure the surrounding function's signature and
control flow propagate or surface the error appropriately.
In `@validation/options_test.go`:
- Around line 12-111: Convert the standalone unit tests into a testify suite:
create a suite struct (e.g., OptionsTestSuite) that embeds suite.Suite, move the
logic from TestFiltersOption, TestCustomFiltersOption, TestMessagesOption,
TestAttributesOption, TestPrepareForValidationOption and the subtests in
TestApplyOptions into suite methods (e.g., TestFilters, TestCustomFilters,
TestMessages, TestAttributes, TestPrepareForValidation, TestApplyOptions) that
use suite.Require()/suite.Assert(), and add a top-level TestOptionsTestSuite
function that calls suite.Run(t, new(OptionsTestSuite)); keep assertions the
same and reference existing helpers/constructors (Filters, CustomFilters,
Messages, Attributes, PrepareForValidation, applyOptions) inside the suite
methods.
In `@validation/rule_parser_test.go`:
- Around line 9-401: Replace the free-standing test functions (TestParseRules,
TestParseRuleSlice, TestSplitRules, TestExtractRuleName, TestParseOneRule,
TestSplitParameters) with a testify suite: define a ParserTestSuite struct
embedding suite.Suite, convert each Test* function into a method on
*ParserTestSuite (e.g., func (s *ParserTestSuite) TestParseRules() { ... } )
using s.Equal / s.Require as assertions, and add a top-level
TestParserTestSuite(t *testing.T) that calls suite.Run(t, new(ParserTestSuite));
keep the existing test cases and helper calls (ParseRules, ParseRuleSlice,
splitRules, extractRuleName, parseOneRule, splitParameters) intact inside the
suite methods and import github.com/stretchr/testify/suite.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 77694720-f5bf-418b-a4ae-fb35f544815f
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (30)
contracts/http/request.gocontracts/validation/validation.goerrors/list.gogo.modhttp/console/stubs.gomocks/http/ContextRequest.gomocks/http/FormRequest.gomocks/http/FormRequestWithFilters.gomocks/validation/Data.gomocks/validation/Option.gomocks/validation/Validation.gomocks/validation/Validator.gotests/go.modvalidation/data.govalidation/data_bag.govalidation/engine.govalidation/errors.govalidation/errors_test.govalidation/filters.govalidation/messages.govalidation/options.govalidation/options_test.govalidation/rule_parser.govalidation/rule_parser_test.govalidation/rules.govalidation/utils.govalidation/validation.govalidation/validation_test.govalidation/validator.govalidation/validator_test.go
💤 Files with no reviewable changes (3)
- go.mod
- validation/data.go
- validation/validator_test.go
|
@copilot 检查所有未解决的AI回复，判断哪些是真实存在的问题，解决它们并关闭无效的问题。 |
- isValueEmpty: use reflection to handle typed slices/maps ([]int{}, map[string]string{}, etc.)
- validateField: run exclude_* rules in pre-pass to ensure order-independence
- Bind: deep merge validated data to preserve unvalidated nested fields
- Make: reject unknown rule names early with ValidationUnknownRule error
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
I'd suggest leaving the original test cases here to avoid break changes for users. We can comment the unimplemented test cases temporarily.
There was a problem hiding this comment.
I'd suggest leaving the original test cases here to avoid break changes for users. We can comment the unimplemented test cases temporarily.
下一个PR加回去，现在没有规则基本所有用例都测不了
There was a problem hiding this comment.
Ok，主要是想通过测试用例检查修改前后的区别，之后的添加的用例最好能直接使用原始的用例。
| } | ||
|
|
||
| // ParseRules parses a pipe-separated rule string into a slice of ParsedRule. | ||
| // Example: "required|string|max:255|in:a,b,c" -> [{required []}, {string []}, {max [255]}, {in [a b c]}] |
There was a problem hiding this comment.
Does all validation styles keep the same as the original?
There was a problem hiding this comment.
Does all validation styles keep the same as the original?
大多数如此，但是有一些变化，例如min_len->min，Laravel没有min_len这种
There was a problem hiding this comment.
还有一点是required规则，gookit对于Go类型的空值（默认值）会拒绝，重构之后会允许通过（更接近Laravel的情况）。
There was a problem hiding this comment.
可以为这样的差异列一个表，如果很多的话，我们可能需要考虑保留 min_len 等以向后兼容，以后慢慢废弃。如果不多，可以考虑直接切换。
There was a problem hiding this comment.
不知道有没有可能以多驱动的形式来实现验证功能的替换？在 service provider 中注入不同的驱动。当前直接替换的方式怕对用户的项目产生很大的影响。
例如在1.18中默认使用新驱动，对于老项目用户可以添加一个配置指定使用 gookit。
这样的话 gookit 的功能我们也不需要一次做到平替，可以先实现基础的基座，然后一个pr添加一个rule的形式小步快跑。
There was a problem hiding this comment.
可以为这样的差异列一个表，如果很多的话，我们可能需要考虑保留 min_len 等以向后兼容，以后慢慢废弃。如果不多，可以考虑直接切换。
Claude Code 生成的对比：
验证规则差异对比
1. 原版有但新版命名不同（需要添加别名或确认语义差异）
| 原版规则 | 新版对应 | 差异说明 |
|---|---|---|
eq |
无 | 检查值等于给定值。新版没有 eq，但有 same（比较两个字段） |
ne |
无 | 检查值不等于给定值。新版没有 ne，但有 different（比较两个字段） |
len |
size |
原版 len 检查长度等于给定值，新版 size 是 Laravel 风格 |
min_len |
min |
原版 min_len 只检查长度，原版 min 只检查数值；新版 min 根据类型自动选择检查数值还是长度 |
max_len |
max |
同上 |
eq_field |
same |
原版 eq_field:field 比较两字段相等；新版 same:field |
ne_field |
different |
原版 ne_field:field；新版 different:field |
gt_field |
gt |
原版 gt_field:field 比较两字段；新版 gt:field 支持字段比较（Laravel 风格） |
gte_field |
gte |
同上 |
lt_field |
lt |
同上 |
lte_field |
lte |
同上 |
gt_date |
after |
原版 gt_date:value；新版 after:date（Laravel 命名） |
gte_date |
after_or_equal |
原版 gte_date:value；新版 after_or_equal:date |
lt_date |
before |
原版 lt_date:value；新版 before:date |
lte_date |
before_or_equal |
原版 lte_date:value；新版 before_or_equal:date |
full_url |
url |
原版 full_url 要求 http/https 开头；新版 url 也检查 scheme |
number |
numeric |
原版 number 检查数字 >= 0；新版 numeric 检查是否为数字 |
uint |
无 | 原版检查 uintX 类型，新版无对应（可用 integer + min:0） |
uuid3 |
无 | 新版只有通用 uuid，无 uuid3/4/5 变体 |
uuid4 |
无 | 同上 |
uuid5 |
无 | 同上 |
2. 新版有但原版没有（Laravel 标准规则）
| 新版规则 | 说明 |
|---|---|
required_if_accepted |
当另一字段为 accepted 时必填 |
required_if_declined |
当另一字段为 declined 时必填 |
present_if/unless/with/with_all |
字段存在性条件检查 |
missing/missing_if/missing_unless/missing_with/missing_with_all |
字段不应存在的条件检查 |
accepted_if, declined_if |
条件接受/拒绝 |
prohibited/prohibited_if/prohibited_unless/prohibited_if_accepted/prohibited_if_declined/prohibits |
禁止规则 |
filled |
不为空（如果存在） |
ascii |
仅 ASCII 字符 |
active_url |
DNS 可解析 URL |
mac_address |
MAC 地址 |
ulid |
ULID 格式 |
hex_color |
十六进制颜色 |
not_regex |
正则不匹配 |
lowercase/uppercase |
大小写检查 |
doesnt_start_with/doesnt_end_with/doesnt_contain |
反向字符串检查 |
contains |
包含子串 |
confirmed |
确认字段匹配 |
in_array/in_array_keys |
数组内检查 |
date_format/date_equals |
日期格式/相等 |
exclude/exclude_if/exclude_unless/exclude_with/exclude_without |
排除规则 |
mimes/mimetypes/extensions/dimensions/encoding |
文件规则 |
digits/digits_between/decimal/multiple_of/min_digits/max_digits |
数字规则 |
distinct/required_array_keys |
数组规则 |
bail/nullable/sometimes |
控制规则 |
list |
列表类型 |
exists/unique |
数据库规则 |
3. 关于 int 规则的注意事项
新版已经正确处理了 float64 -> int 的问题。ruleInteger 在 line 745-749 检查 float64 时，会判断：
v == float64(int64(v))
也就是说，只要 float64 的值本质上是整数，例如 42.0，就会通过验证。
同样，新版还支持：
json.Number类型（line 750-752）- 字符串形式的整数（line 753-755）
因此，原版里关于 int 规则的那个注意事项，在新版中已经解决。
总结
新版验证器的设计理念是：从 gookit/validate 的 Go 本地风格命名，全面迁移到 Laravel 标准命名，同时保留 Go 特有的别名：
int = integerbool = booleanslice = list
需要关注的差异主要有这几类：
-
原版独有规则，新版缺少：
eq、ne、uint、uuid3、uuid4、uuid5其中
eq/ne可以被same/different部分替代，但语义并不相同：same比较字段eq比较固定值
uint则可以通过integer|min:0组合实现。 -
命名变更：
例如eq_field -> same、ne_field -> different、gt_date -> after、lt_date -> before，整体都改成了 Laravel 风格命名。 -
语义合并：
原版的min（只检查数值）和min_len（只检查长度），在新版里统一为min，由规则根据类型自动判断是比较数值还是长度。 -
int的float64问题：
新版已经修复。json.Unmarshal产生的float64，只要数值本身是整数，也能正确通过integer验证。
There was a problem hiding this comment.
不知道有没有可能以多驱动的形式来实现验证功能的替换？在 service provider 中注入不同的驱动。当前直接替换的方式怕对用户的项目产生很大的影响。 例如在1.18中默认使用新驱动，对于老项目用户可以添加一个配置指定使用 gookit。 这样的话 gookit 的功能我们也不需要一次做到平替，可以先实现基础的基座，然后一个pr添加一个rule的形式小步快跑。
感觉有点复杂，目前看主要是规则的命名有变更，语义上没有太大问题（我在gin/fiber上通过了所有测试用例），考虑先保留gookit的大部分别名？
There was a problem hiding this comment.
Good news 👍 如果我们考虑直接替换掉 gookit 的话，需要尽可能的实现 gookit 目前支持的 rule、filter、语法等，让用户可以较为顺畅的过度，即使在这个过程中我们添加一些 Laravel 本身不支持的语法也是可以接受的，这些语法我们可以标注为废弃，慢慢移除。
There was a problem hiding this comment.
Available Validation Rules 逐一对比
| # | 文档原版规则 | 新版对应 | 状态 | 说明 |
|---|---|---|---|---|
| 1 | required |
required |
✅ 保留 | 语义变更：原版对 Go 零值（false/0/""）拒绝，新版允许通过（更接近 Laravel） |
| 2 | required_if |
required_if |
✅ 保留 | - |
| 3 | required_unless |
required_unless |
✅ 保留 | - |
| 4 | required_with |
required_with |
✅ 保留 | - |
| 5 | required_with_all |
required_with_all |
✅ 保留 | - |
| 6 | required_without |
required_without |
✅ 保留 | - |
| 7 | required_without_all |
required_without_all |
✅ 保留 | - |
| 8 | int |
int / integer |
✅ 保留 | 新增 integer 别名；修复 JSON float64 问题（42.0 可通过验证） |
| 9 | uint |
无 | ❌ 移除 | 可用 integer|min:0 替代 |
| 10 | bool |
bool / boolean |
✅ 保留 | 新增 boolean 别名 |
| 11 | string |
string |
✅ 保留 | - |
| 12 | float |
float |
✅ 保留 | - |
| 13 | slice |
slice |
✅ 保留 | 同时新增 list 别名 |
| 14 | in |
in |
✅ 保留 | - |
| 15 | not_in |
not_in |
✅ 保留 | - |
| 16 | starts_with |
starts_with |
✅ 保留 | - |
| 17 | ends_with |
ends_with |
✅ 保留 | - |
| 18 | between |
between |
✅ 保留 | 语义扩展：原版仅检查数值，新版同时支持字符串长度/数组数量/文件大小（类型自动判断） |
| 19 | max |
max |
✅ 保留 | 语义扩展：原版仅检查数值，新版根据类型自动判断（同上） |
| 20 | min |
min |
✅ 保留 | 语义扩展：原版仅检查数值，新版合并了 min_len 的功能 |
| 21 | eq |
无 | ❌ 移除 | 检查值等于固定值；新版 same 是字段间比较，语义不同 |
| 22 | ne |
无 | ❌ 移除 | 检查值不等于固定值；新版 different 是字段间比较，语义不同 |
| 23 | lt |
lt |
✅ 保留 | 语义扩展：合并了原版 lt_field（lt:field 支持跨字段比较） |
| 24 | gt |
gt |
✅ 保留 | 语义扩展：合并了原版 gt_field |
| 25 | len |
size |
len → size（Laravel 命名） |
|
| 26 | min_len |
合并入 min |
min 根据字段类型自动选择检查数值还是长度 |
|
| 27 | max_len |
合并入 max |
同上 | |
| 28 | email |
email |
✅ 保留 | - |
| 29 | array |
array |
✅ 保留 | - |
| 30 | map |
map |
✅ 保留 | - |
| 31 | eq_field |
same |
eq_field:field → same:field |
|
| 32 | ne_field |
different |
ne_field:field → different:field |
|
| 33 | gt_field |
合并入 gt |
gt_field:field → gt:field |
|
| 34 | gte_field |
合并入 gte |
gte_field:field → gte:field |
|
| 35 | lt_field |
合并入 lt |
lt_field:field → lt:field |
|
| 36 | lte_field |
合并入 lte |
lte_field:field → lte:field |
|
| 37 | file |
file |
✅ 保留 | - |
| 38 | image |
image |
✅ 保留 | - |
| 39 | date |
date |
✅ 保留 | - |
| 40 | gt_date |
after |
gt_date:value → after:date |
|
| 41 | lt_date |
before |
lt_date:value → before:date |
|
| 42 | gte_date |
after_or_equal |
gte_date:value → after_or_equal:date |
|
| 43 | lte_date |
before_or_equal |
lte_date:value → before_or_equal:date |
|
| 44 | alpha |
alpha |
✅ 保留 | - |
| 45 | alpha_num |
alpha_num |
✅ 保留 | - |
| 46 | alpha_dash |
alpha_dash |
✅ 保留 | - |
| 47 | json |
json |
✅ 保留 | - |
| 48 | number |
numeric |
原版 number 检查 >= 0 的数字字符串；新版 numeric 检查是否为数值 |
|
| 49 | full_url |
url |
原版 full_url 要求 http/https 开头；新版 url 同样检查 scheme |
|
| 50 | ip |
ip |
✅ 保留 | - |
| 51 | ipv4 |
ipv4 |
✅ 保留 | - |
| 52 | ipv6 |
ipv6 |
✅ 保留 | - |
| 53 | regex |
regex |
✅ 保留 | - |
| 54 | uuid |
uuid |
✅ 保留 | - |
| 55 | uuid3 |
无 | ❌ 移除 | 新版只有通用 uuid，不区分版本 |
| 56 | uuid4 |
无 | ❌ 移除 | 同上 |
| 57 | uuid5 |
无 | ❌ 移除 | 同上 |
Available Validation Filters 逐一对比
新版 builtinFilters 为空 map，所有内置 filter 均尚未实现，待后续 PR 添加。
Filter 基础设施已就绪（applyFilters / callFilterFunc 已实现），缺的是 builtinFilters map 中的具体注册。
| # | 文档原版 Filter | 新版状态 | 说明 |
|---|---|---|---|
| 1 | int / toInt |
❌ 未实现 | 转换为 int |
| 2 | uint / toUint |
❌ 未实现 | 转换为 uint |
| 3 | int64 / toInt64 |
❌ 未实现 | 转换为 int64 |
| 4 | float / toFloat |
❌ 未实现 | 转换为 float |
| 5 | bool / toBool |
❌ 未实现 | 转换为 bool |
| 6 | trim / trimSpace |
❌ 未实现 | 去除两端空白 |
| 7 | ltrim / trimLeft |
❌ 未实现 | 去除左侧空白 |
| 8 | rtrim / trimRight |
❌ 未实现 | 去除右侧空白 |
| 9 | lower / lowercase |
❌ 未实现 | 转小写 |
| 10 | upper / uppercase |
❌ 未实现 | 转大写 |
| 11 | lcFirst / lowerFirst |
❌ 未实现 | 首字母小写 |
| 12 | ucFirst / upperFirst |
❌ 未实现 | 首字母大写 |
| 13 | ucWord / upperWord |
❌ 未实现 | 每个单词首字母大写 |
| 14 | camel / camelCase |
❌ 未实现 | 驼峰命名 |
| 15 | snake / snakeCase |
❌ 未实现 | 下划线命名 |
| 16 | escapeJs / escapeJS |
❌ 未实现 | JS 转义 |
| 17 | escapeHtml / escapeHTML |
❌ 未实现 | HTML 转义 |
| 18 | str2ints / strToInts |
❌ 未实现 | 字符串转 []int |
| 19 | str2time / strToTime |
❌ 未实现 | 字符串转 time.Time |
| 20 | str2arr / str2array / strToArray |
❌ 未实现 | 字符串转 []string |
There was a problem hiding this comment.
Hi @h2zi 新版中可否将除了 uuid3、uuid4、uuid5 的 rules 进行适配？之后我们可以在文档中标注要废弃的 rules。
validation/data_bag.go
Outdated
| return | ||
| } | ||
| if len(remaining) == 1 { | ||
| v[idx] = map[string]any{"": val} |
There was a problem hiding this comment.
Good catch. The map[string]any{"": val} was a hack for assigning a non-map value into a []map[string]any slot. Fixed in 5bfd772 — now only assigns if val is map[string]any, otherwise skips the invalid operation.
validation/data_bag.go
Outdated
| } | ||
|
|
||
| // structToMap converts a struct to map using "form" tags. | ||
| func structToMap(rv reflect.Value) map[string]any { |
There was a problem hiding this comment.
There is a similar function in database/gorm, can they be merged?
There was a problem hiding this comment.
They serve different purposes with different tag conventions — validation uses form/json tags while gorm uses gorm tag with column: format. The tag parsing logic and field handling (e.g. pointer/nil semantics) differ significantly. Merging would require an abstraction layer that adds complexity without clear benefit. Keeping them separate for now.
| if r.Body != nil { | ||
| body, err := io.ReadAll(r.Body) | ||
| if err != nil { | ||
| return bag, nil |
There was a problem hiding this comment.
How about returning err here?
There was a problem hiding this comment.
Intentional graceful degradation — body parse failures (io.ReadAll, JSON unmarshal, multipart parse) should not block validation. The bag already has query parameters parsed, and returning an error would lose those. This matches the existing gin/fiber HTTP driver behavior where malformed bodies fall back to query-only data.
| func getAttributeType(attribute string, value any, rules map[string][]ParsedRule) string { | ||
| if fieldRules, ok := rules[attribute]; ok { | ||
| for _, r := range fieldRules { | ||
| if numericRuleNames[r.Name] { |
There was a problem hiding this comment.
It doen't work, numericRuleNames is always nil.
There was a problem hiding this comment.
Valid concern. Fixed in 5bfd772 — added a runtime value type fallback using reflection. When numericRuleNames has no match (empty in PR 1, will be populated in PR 2), the function now checks the actual value'''s reflect.Kind for int/uint/float types and returns "numeric". Also added "array" fallback for slice/array/map kinds.
validation/filters.go
Outdated
| // Check custom filters | ||
| if customFilter, ok := customFilterMap[pf.Name]; ok { | ||
| result, err := callFilterFunc(ctx, customFilter, val, pf.Parameters) | ||
| if err == nil { |
There was a problem hiding this comment.
Good point. Fixed in 5bfd772 — applyFilters now propagates custom filter errors with context (filter name + field name) instead of silently ignoring them.
validation/data_bag.go
Outdated
| continue | ||
| } | ||
|
|
||
| result[tag] = val.Interface() |
There was a problem hiding this comment.
For the struct:
type User struct {
Name string `form:"name"`
Address Address `form:"address"` // Nest
}
After calling NewDataBag(&User{Name: "Alice", Address: Address{City: "Beijing"}}), the data will be :
map[string]any{
"name": "Alice",
"address": Address{City: "Beijing"}, // struct, not map
}
The result of bag.Get("address.city") is unexpected.
The similar situation:
type Form struct {
Tags []string `form:"tags"`
Agents []Agents `form:agents`
}
There was a problem hiding this comment.
Valid concern. Fixed in 5bfd772 — added normalizeValue() helper that recursively converts nested structs to map[string]any, typed slices ([]string, []Agent) to []any, and typed maps (map[string]string) to map[string]any. Now bag.Get("address.city") and bag.Get("tags.0") work correctly for struct-backed data.
- dotSet: fix "": val hack in []map[string]any case, use direct map assignment - structToMap: recursively normalize nested structs/slices/maps via normalizeValue - getAttributeType: add runtime value type fallback when numericRuleNames is empty - applyFilters: propagate custom filter errors instead of silently ignoring
There was a problem hiding this comment.
Could you add some test cases for this file?
There was a problem hiding this comment.
Added in 8427dd8 — data_bag_test.go covers NewDataBag (map/url.Values/struct/nil/unsupported), NewDataBagFromRequest (JSON/form/multipart/query/graceful degradation), Get/Set (dot notation, array index, missing keys), Has, All, Keys (sort, cache).
There was a problem hiding this comment.
Could you add test for this file as well?
There was a problem hiding this comment.
Added in 8427dd8 — utils_test.go covers isValueEmpty, getAttributeType, matchesOtherValue, isControlRule, dotGet/dotSet, collectKeys, expandWildcardFields, urlValuesToMap, structToMap (form/json/embedded/nested), normalizeValue.
There was a problem hiding this comment.
Could you add test for this file as well?
There was a problem hiding this comment.
Added in 8427dd8 — messages_test.go covers getMessage (custom field+rule > custom rule > type-specific default > generic default > fallback), formatMessage (placeholder replacement, length-based sorting, fast path vs sort path), getDisplayableAttribute.
There was a problem hiding this comment.
We can add test for this file in the next PR.
There was a problem hiding this comment.
Sounds good — will add filter tests in the builtin rules PR.
| if customFilter, ok := customFilterMap[pf.Name]; ok { | ||
| result, err := callFilterFunc(ctx, customFilter, val, pf.Parameters) | ||
| if err != nil { | ||
| return fmt.Errorf("filter %s on field %s: %w", pf.Name, field, err) |
There was a problem hiding this comment.
The error messages can be moved to errors/list.go in another PR.
There was a problem hiding this comment.
Agreed — will move the filter error messages to errors/list.go in a follow-up PR.
There was a problem hiding this comment.
Could you add test for this file as well?
There was a problem hiding this comment.
Added in 8427dd8 — engine_test.go covers Validate (custom rules pass/fail, unknown rule, missing field skip), ValidatedData (ruled fields only, excludes), handleExcludeRule (all 5 variants), expandWildcardRules (wildcard expansion + caching), trackDistinct (duplicate detection), formatErrorMessage (custom rule, custom message, parameter replacement, custom attributes), executeRule (custom/unknown), and control rules (bail, nullable, sometimes).
| } | ||
|
|
||
| // formatErrorMessage creates the error message for a rule failure. | ||
| func (e *Engine) formatErrorMessage(field string, rule ParsedRule, attrType string) string { |
There was a problem hiding this comment.
A small suggestion on formatErrorMessage: since it currently relies on a switch statement for rule-specific preprocessing before formatting, would it be worth extracting that part into an interface such as ErrorMessageFormatter?
That way, each rule could handle its own preprocessing logic (for example, preparing replacements etc.), while formatErrorMessage would only need to call the interface when implemented and otherwise fall back to the default replacements (attribute and value). This could make the code cleaner and easier to extend.
There was a problem hiding this comment.
Thanks for the suggestion! The current switch-based approach is intentional for this stage — with all rules being static and finite, a switch gives the compiler full optimization (inlining, jump tables) and keeps the code straightforward.
An interface like ErrorMessageFormatter would add a per-rule abstraction layer that's not needed when the rule set is fixed and all formatting logic is co-located. In Laravel, formatErrorMessage is similarly centralized rather than delegated to individual rule classes.
If we later support user-defined builtin-style rules (beyond the current custom rule API which already handles its own messages), we can revisit this. For now the switch keeps it simple and fast.
# Conflicts: # go.mod # go.sum # tests/go.sum
Add comprehensive test coverage for core validation infrastructure: - data_bag_test.go: NewDataBag, NewDataBagFromRequest, Get/Set/Has/All/Keys - utils_test.go: dotGet/dotSet, collectKeys, expandWildcardFields, structToMap, normalizeValue, isValueEmpty - messages_test.go: getMessage priority, formatMessage, getDisplayableAttribute - engine_test.go: Validate, ValidatedData, handleExcludeRule, trackDistinct, formatErrorMessage, bail/nullable/sometimes Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace gookit/validate with a native validation engine that is fully compatible with Laravel's validation API. This PR introduces the core infrastructure without builtin rule/filter implementations and some tests (to be added in a follow-up PR).
Key changes:
📑 Description
Closes https://github.com/goravel/goravel/issues/
Summary by CodeRabbit
Release Notes
New Features
Validated()method to expose validated field data post-validation.Improvements
Removals
✅ Checks