Make new validations consistent with System.ComponentModel.DataAnnotations behavior #63231
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
8c0481c to
48cf20a
Compare
javiercn
reviewed
Aug 14, 2025
src/Validation/startvscode.sh
Outdated
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?
javiercn
reviewed
Aug 14, 2025
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
| @@ -244,6 +244,7 @@ public IEnumerable<ValidationResult> Validate(ValidationContext validationContex | |||
|
|
|||
| 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.
3 tasks
...lidation/test/Microsoft.Extensions.Validation.GeneratorTests/ValidationsGeneratorTestBase.cs
Outdated
Show resolved
Hide resolved
…ests/ValidationsGeneratorTestBase.cs
captainsafia
approved these changes
Aug 15, 2025
|
/ba-g Current test failure appears to be an unrelated flake. |
wtgodbe
added a commit
that referenced
this pull request
Aug 18, 2025
* 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>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ValidatableTypeInfo.ValidateAsyncso that it follows the same ordering and short-circuiting rules asSystem.ComponentModel.DataAnnotations.Validator(See the original implementation.)Fixes #62584