Fix double validation and state leaks in Attributes recursion - #1832
Fix double validation and state leaks in Attributes recursion#1832alganet wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1832 +/- ##
============================================
+ Coverage 97.12% 97.16% +0.04%
- Complexity 1069 1087 +18
============================================
Files 198 198
Lines 2505 2542 +37
============================================
+ Hits 2433 2470 +37
Misses 72 72 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
51de53d to
b5511d1
Compare
There was a problem hiding this comment.
Pull request overview
Fixes recursive attribute validation by isolating traversal state and detecting wrapped Attributes validators.
Changes:
- Uses immutable, path-scoped circular-reference tracking.
- Prevents duplicate recursion for wrapped rules and overlapping unions.
- Adds regression tests and documentation.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/Validators/Attributes.php |
Implements recursion and wrapper detection fixes. |
docs/validators/Attributes.md |
Documents recursion and cycle behavior. |
tests/unit/Validators/AttributesTest.php |
Adds unit regression coverage. |
tests/feature/Validators/AttributesTest.php |
Verifies wrapped-rule error output. |
tests/src/Stubs/WithWrappedAttributesOnNested.php |
Provides wrapped-rule fixture. |
tests/src/Stubs/WithSharedNested.php |
Provides shared-reference fixture. |
tests/src/Stubs/WithOverlappingUnionTypeNested.php |
Provides overlapping-union fixture. |
tests/src/Stubs/WithCyclicValidator.php |
Uses the cyclic validator fixture. |
tests/src/Stubs/WithArrayValidator.php |
Uses the array-backed validator fixture. |
tests/src/Stubs/CyclicValidator.php |
Models cyclic validator state. |
tests/src/Stubs/ArrayValidator.php |
Models array-backed validator state. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
a6436d4 to
cbe412c
Compare
Recursive validation for nested objects guarded against validating a nested object twice by checking whether the property already carried #[Attributes] directly on the property. That guard did not recognise the rule inside a wrapper, and the state backing the recursion was never released, producing five defects: - #[NullOr(new Attributes())] on a class-typed property validated the nested object twice and reported every nested failure twice. This was the documented way of validating a nullable nested object before recursion existed, so upgrading silently duplicated messages. - The same object held by two sibling properties failed as a circular reference, because visited objects accumulated for the whole traversal instead of the current path. - An Attributes instance could only be used once: nothing ever cleared the visited objects, so every evaluation after the first failed. - A union type whose value satisfied more than one of its class members recursed once per member, and the second one reported the object it had just visited as a circular reference. - A custom validator attribute with cyclic internal state made wrapped-rule detection recurse indefinitely. Detect the rule anywhere inside a property's attributes rather than only at the top level, so a wrapped Attributes suppresses implicit recursion as an explicit one does. Make the rule immutable and give each recursion level its own instance carrying the path to the object being evaluated, so visited objects represent the path from the root rather than every object ever seen. Collapse union recursion into a single Given over the disjunction of its class members. Track the validators visited while inspecting a wrapped rule, preventing a cyclic validator graph from overflowing the stack. The detection only runs for properties whose type can hold an object, so attributes with large arguments, such as #[In], no longer pay for it.
cbe412c to
e2a17c7
Compare
| public function __construct(private readonly Resolver|null $resolver = null) | ||
| public function __construct(private Resolver|null $resolver = null) | ||
| { | ||
| $this->path = self::rootPath(); |
There was a problem hiding this comment.
Nica catch. Could everything that's related to tracking this be a different object? The class became quite big after this change, and it's already not the smallest 😅
There was a problem hiding this comment.
@henriquemoody likely yes, but then injecting those objects and designing that surface becomes a concern.
The class would shrink a little (it's actually only 267 lines total with my change), but the PR would grow significantly.
It is something I want to do eventually. Also, I want to make the array<int, true> $path readonly as well, but that also requires deeper surgery.
There was a problem hiding this comment.
I think we don't event need to inject it as a dependency, just instantiating the object (that could also hold the state) is enough. I see it more as a way to separate concerns but not change the contract of the constructor.
henriquemoody
left a comment
henriquemoody
left a comment
There was a problem hiding this comment.
I'm fine to keep it as is. I will pre-approve it, feel free to merge it if you would prefer keeping all the code in the Attributes
Recursive validation for nested objects guarded against validating a nested object twice by checking whether the property already carried #[Attributes] directly on the property. That guard did not recognise the rule inside a wrapper, and the state backing the recursion was never released, producing five defects:
Detect the rule anywhere inside a property's attributes rather than only at the top level, so a wrapped Attributes suppresses implicit recursion as an explicit one does. Make the rule immutable and give each recursion level its own instance carrying the path to the object being evaluated, so visited objects represent the path from the root rather than every object ever seen. Collapse union recursion into a single Given over the disjunction of its class members. Track the validators visited while inspecting a wrapped rule, preventing a cyclic validator graph from overflowing the stack.
The detection only runs for properties whose type can hold an object, so attributes with large arguments, such as #[In], no longer pay for it.
This is a smaller, more focused fix for the recursion problem that does not introduce an external resolver like #1799. It is intended to focus on the behavior, leaving open the possibility of introducing such resolver in the dependency chain as a substitute non-breaking change.