Make new validations consistent with System.ComponentModel.DataAnnotations behavior#63231
Make new validations consistent with System.ComponentModel.DataAnnotations behavior#63231
Conversation
8c0481c to
48cf20a
Compare
There was a problem hiding this comment.
This is just chmod +x, isn't it?
There was a problem hiding this comment.
Yes, is it ok to add it here?
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for type-level validation attributes to the new validation infrastructure and updates validation logic to follow the same ordering and short-circuiting rules as System.ComponentModel.DataAnnotations.Validator. The key changes include:
- Implementation of type-level validation attribute support in the validation framework
- Modification of validation order to match DataAnnotations behavior: property attributes → type attributes → IValidatableObject
- Addition of short-circuiting logic to stop validation when errors are found in earlier phases
Reviewed Changes
Copilot reviewed 31 out of 32 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
ValidatableTypeInfo.cs |
Core validation logic updated with proper ordering and short-circuiting behavior |
TestValidatableTypeInfo.cs |
New test helper class to support type-level attribute testing |
| Generator snapshots | Updated generated code to support type-level validation attributes |
| Test files | Added comprehensive tests for validation ordering and short-circuiting behavior |
| Generator parsers/emitters | Enhanced to detect and generate code for type-level validation attributes |
src/Validation/test/Microsoft.Extensions.Validation.Tests/ValidatableTypeInfoTests.cs
Show resolved
Hide resolved
src/Validation/test/Microsoft.Extensions.Validation.Tests/ValidatableTypeInfoTests.cs
Show resolved
Hide resolved
captainsafia
left a comment
There was a problem hiding this comment.
Looks good overall -- posted one comment about the caches for types.
|
|
||
| private class MockValidatableTypeInfo(Type type, ValidatablePropertyInfo[] members) : ValidatableTypeInfo(type, members) | ||
| { | ||
| protected override ValidationAttribute[] GetValidationAttributes() => []; |
There was a problem hiding this comment.
Since the ValidatableTypeInfo is marked as experimental, we're OK to make this change without API review. Also, it follows the same patter as the ValidatablePropertyInfo that was already code reviewed so we should be good there anyways.
There was a problem hiding this comment.
The changes in this file look good. I appreciate the readability improvements from moving each stage to a separate method.
.../test/Microsoft.Extensions.Validation.GeneratorTests/ValidationsGenerator.ClassAttributes.cs
Outdated
Show resolved
Hide resolved
.../test/Microsoft.Extensions.Validation.GeneratorTests/ValidationsGenerator.ClassAttributes.cs
Outdated
Show resolved
Hide resolved
| // Get attributes from the type itself and its super types | ||
| foreach (var attr in t.GetCustomAttributes(typeof(global::System.ComponentModel.DataAnnotations.ValidationAttribute), true)) | ||
| { | ||
| if (attr is global::System.ComponentModel.DataAnnotations.ValidationAttribute validationAttribute) |
There was a problem hiding this comment.
Isn't this redundant?
Edit: Oh you need the type, use GetCustomAttributes<T> then?
There was a problem hiding this comment.
Thanks, I replaced it with the generic extension. I think I had it like this because I initially did not realize that Type is actually derived from MemberInfo (so I didn't see the overload), and later I forgot to change it.
...lidation/test/Microsoft.Extensions.Validation.GeneratorTests/ValidationsGeneratorTestBase.cs
Outdated
Show resolved
Hide resolved
…ests/ValidationsGeneratorTestBase.cs
|
/ba-g Current test failure appears to be an unrelated flake. |
* Avoid race that can cause Kestrel's RequestAborted to not fire (#62385) * Send Keep-Alive Ping Immediately When Previous Ping Is Overdue (#63195) * Make new validations consistent with System.ComponentModel.DataAnnotations behavior (#63231) * Add support for type-level validation attributes, update validation ordering * Code review fix, test fix * Fix trimming annotation * Fix trimming annotation * Separate caches for property and type attributes * Fix typo Co-authored-by: Brennan <brecon@microsoft.com> * Fix typo Co-authored-by: Brennan <brecon@microsoft.com> * Fix and simplify the emitted code * Update src/Validation/test/Microsoft.Extensions.Validation.GeneratorTests/ValidationsGeneratorTestBase.cs --------- Co-authored-by: Brennan <brecon@microsoft.com> Co-authored-by: Safia Abdalla <safia@microsoft.com> * Search for slnx files when setting solution-relative content root (#61305) * Address API review feedback for what was IApiEndpointMetadata (#63283) --------- Co-authored-by: Stephen Halter <halter73@gmail.com> Co-authored-by: Reagan Yuan <reaganyuan27@gmail.com> Co-authored-by: Ondřej Roztočil <roztocil@outlook.com> Co-authored-by: Brennan <brecon@microsoft.com> Co-authored-by: Safia Abdalla <safia@microsoft.com> Co-authored-by: Jacob Bundgaard <jbu@templafy.com>
ValidatableTypeInfo.ValidateAsyncso that it follows the same ordering and short-circuiting rules asSystem.ComponentModel.DataAnnotations.Validator(See the original implementation.)Fixes #62584