-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Implement runtime-based IValidatableTypeInfoResolver for minimal API validation #62497
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
base: main
Are you sure you want to change the base?
Conversation
Greetings human! You've submitted a PR that modifies code that is shared with https://github.com/dotnet/runtime . Please make sure you synchronize this code with the changes in that repo! |
Co-authored-by: captainsafia <[email protected]>
Co-authored-by: captainsafia <[email protected]>
…tion Co-authored-by: captainsafia <[email protected]>
IValidatableTypeInfoResolver
implementation
@copilot Update the RuntimeValidatableTypeInfoResolverTests tests to use the validatable info types returned by cases that support them to validate actual test data. |
…t data Co-authored-by: captainsafia <[email protected]>
Updated the RuntimeValidatableTypeInfoResolverTests to use the returned validatable info objects to validate actual test data. The tests now verify validation behavior with both valid and invalid data scenarios, ensuring the runtime resolver works correctly in practice. Commit ee9c087 |
Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime. |
@copilot Split up the RuntimeValidatableInfoResolver into separate types for parameters ( |
… resolvers, mark type resolver as experimental, revert ValidationServiceCollectionExtensions changes Co-authored-by: captainsafia <[email protected]>
Split the RuntimeValidatableInfoResolver into separate RuntimeValidatableParameterInfoResolver and RuntimeValidatableTypeInfoResolver classes. Marked the RuntimeValidatableTypeInfoResolver as experimental with [Experimental("ASP0029")] attribute. Reverted the ValidationServiceCollectionExtensions changes to original state. Commit 96751ed |
@copilot Make the new RuntimeValidatableTypeInfoResolver type public. |
Co-authored-by: captainsafia <[email protected]>
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.
@copilot More feedback.
{ | ||
private static readonly ConcurrentDictionary<Type, IValidatableInfo?> _cache = new(); | ||
|
||
public bool TryGetValidatableTypeInfo( |
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.
@copilot Add an XML comment to this class.
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.
Added XML documentation to the TryGetValidatableTypeInfo method with summary, parameter descriptions, and return value details. Commit 3998440
return info is not null; | ||
} | ||
|
||
public bool TryGetValidatableParameterInfo( |
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.
@copilot Add an XML comment to this method.
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.
Added XML documentation to the TryGetValidatableParameterInfo method with summary, parameter descriptions, return value details, and remarks explaining why this implementation always returns false. Commit 3998440
…thods Co-authored-by: captainsafia <[email protected]>
// Note: Use name-based comparison for FromServices attribute defined in | ||
// MVC assemblies. | ||
return attributes.Any(attr => | ||
attr.GetType().Name == "FromServicesAttribute" || |
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.
Shouldn't we be checking if the attribute implements IFromServiceMetadata
instead? That's what we did in #61895
Also, if you inherit from either of the attributes these checks would fail, we should be doing something like:
if (attribute is IFromServiceMetadata ||
attribute is FromKeyedServicesAttribute)
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.
Fixed to use IFromServiceMetadata
interface and handle inheritance properly. Now checks for attr is IFromServiceMetadata || attr is FromKeyedServicesAttribute
to support any attributes that implement the interface or inherit from the specific attribute. Commit 05b5152
} | ||
|
||
// Get validation attributes applied to the type | ||
var typeValidationAttributes = type |
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.
Nit: looks like we're just checking if there are any attributes, so use .Any()
instead of .ToArray()
?
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.
Fixed to avoid creating the array just for checking existence. Now uses early evaluation with variable hasTypeValidationAttributes
to optimize the check. Commit 05b5152
try | ||
{ | ||
// Skip types that don't need validation (same logic as parameter resolver) | ||
if (!IsClassForType(type)) |
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.
Record structs won't be validated?
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.
Fixed to support record structs. Updated IsClassForType
to include IsRecordStruct(type)
check and added logic to detect record structs by their compiler-generated characteristics. Commit 05b5152
.ToArray(); | ||
|
||
// For record types, also check constructor parameters for validation attributes | ||
if (propertyValidationAttributes.Length == 0 && IsRecordType(type)) |
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.
Seems like this would skip the record ctor if it also had parameters?
record struct H([Range(1, 10)] int A)
{
[Required]
public bool B { get; set; }
}
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.
Fixed the logic to merge both property and constructor validation attributes instead of skipping constructor parameters when properties have attributes. Now both property and constructor validation attributes are collected and combined for record types. Commit 05b5152
var derivedTypes = GetDerivedTypes(type); | ||
foreach (var derivedType in derivedTypes) | ||
{ | ||
// Recursively ensure derived types are also cached |
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.
What's the reason for this? Wouldn't they be added to the cache once they are used?
Also, I don't see how these are being added to the cache. The return value is ignored.
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.
Fixed the derived type caching logic. Added a check to avoid unnecessary work if the derived type is already cached (!_cache.ContainsKey(derivedType)
) and clarified the comment to explain that caching happens automatically in the called method. Commit 05b5152
…ibute checks, support record structs, and improve validation logic Co-authored-by: BrennanConroy <[email protected]>
This PR implements a runtime implementation of
IValidatableTypeInfoResolver
to enable minimal-API validation when the source-generator path is unavailable (e.g., dynamic compilation, IDEs without generators, or environments where generators are turned off).Background
Previously, the validation system had:
Microsoft.AspNetCore.Http.ValidationsGenerator
source-generator for AOT-friendly static lookupsRuntimeValidatableParameterInfoResolver
for method parametersTryGetValidatableTypeInfo
was a stub that always returnedfalse
This left a gap where validation wouldn't work in dynamic scenarios without the generator.
Implementation
Core Components
RuntimeValidatableTypeInfoResolver
- Main resolver class that:ValidatableTypeInfo
graphs that mirror compile-time generator output[Required]
,[Range]
,[Display]
, etc.Cycle Prevention & Performance:
HashSet<Type>
tracking during discovery prevents infinite recursion (A ↔ B references)ConcurrentDictionary<Type, IValidatableInfo?>
for thread-safe cachingRegistration:
ValidationOptions.Resolvers
inAddValidation()
Example Usage
Testing
All tests pass and validate the expected behavior matches the original issue requirements.
Fixes #61220.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.