-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Fix minimal API validation for record structs #64514
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…IsComplexType Co-authored-by: captainsafia <1857993+captainsafia@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a bug where the built-in validation from AddValidation was not working with record struct types. The issue was that the validation system only checked for reference types (classes) when determining if a type was complex enough to require property-level validation.
Key changes:
- Renamed
IsClasstoIsComplexTypeto accurately reflect the method's purpose - Updated validation logic to include value types (structs, record structs) in addition to reference types
- Added comprehensive test coverage for record struct validation including nested structures and various validation attributes
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/Validation/src/RuntimeValidatableParameterInfoResolver.cs | Renamed method from IsClass to IsComplexType and updated logic to return true for both classes and value types (excluding primitives and framework types), enabling validation of record structs |
| src/Validation/test/Microsoft.Extensions.Validation.GeneratorTests/ValidationsGenerator.RecordType.cs | Added comprehensive test CanValidateRecordStructTypes that validates record structs with Range, Required, StringLength, and Display attributes, including nested record struct validation |
| src/Validation/test/Microsoft.Extensions.Validation.GeneratorTests/snapshots/ValidationsGeneratorTests.CanValidateRecordStructTypes#ValidatableInfoResolver.g.verified.cs | Generated snapshot file showing the validation resolver code for the new record struct test scenarios |
| return type.IsClass; | ||
| // Complex types include both reference types (classes) and value types (structs, record structs) | ||
| // that aren't in the exclusion list above | ||
| return type.IsClass || type.IsValueType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point isn't it just return true;?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are some cases that would still be false here, the notable one being interfaces.
|
/backport to release/10.0 |
|
Started backporting to |
The built-in validation from
AddValidationwas not validatingrecord structtypes because the method checking for complex types only returnedtruefor reference types (classes), excluding all value types including record structs.Changes
IsClasstoIsComplexTypeinRuntimeValidatableParameterInfoResolver.csto reflect actual behaviortype.IsClasstotype.IsClass || type.IsValueTypeExample
The method still excludes primitives, enums, and framework types like
DateTime,Guid,CancellationToken, etc. via the existing exclusion list.Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.