Split IValidatableInfo to multiple interfaces - #67183
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements the API proposal from #67080 by splitting IValidatableInfo into separate interfaces for type, property, and parameter validation metadata, and updates ValidationOptions/resolvers/generator output to use the more specific contracts.
Changes:
- Introduces
IValidatableTypeInfo,IValidatablePropertyInfo, andIValidatableParameterInfo, and removesIValidatableInfo. - Updates
ValidationOptionsandIValidatableInfoResolversignatures to return the new interface types, and updates runtime/generator resolver implementations accordingly. - Refactors property lookup to call
IValidatableTypeInfo.TryFindProperty(...)instead of relying onValidatableTypeInfotype checks inValidationOptions.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Validation/src/ValidationOptions.cs | Updates TryGet... APIs to return the split interface types and uses TryFindProperty for property resolution. |
| src/Validation/src/ValidateContext.cs | Adjusts documentation text related to the (now removed) IValidatableInfo. |
| src/Validation/src/ValidatableTypeInfo.cs | Implements IValidatableTypeInfo and exposes TryFindProperty for property lookup. |
| src/Validation/src/ValidatablePropertyInfo.cs | Switches base class to IValidatablePropertyInfo. |
| src/Validation/src/ValidatableParameterInfo.cs | Switches base class to IValidatableParameterInfo. |
| src/Validation/src/RuntimeValidatableParameterInfoResolver.cs | Updates resolver method signatures to the new interface types. |
| src/Validation/src/PublicAPI.Unshipped.txt | Records removals/additions for the interface split and updated APIs. |
| src/Validation/src/IValidatableTypeInfo.cs | Adds new interface for type validation metadata + property lookup. |
| src/Validation/src/IValidatablePropertyInfo.cs | Adds new interface for property validation metadata. |
| src/Validation/src/IValidatableParameterInfo.cs | Renames/replaces IValidatableInfo with the parameter-specific interface. |
| src/Validation/src/IValidatableInfoResolver.cs | Updates resolver interface to return the new interface types. |
| src/Validation/gen/Emitters/ValidationsGenerator.Emitter.cs | Updates generated resolver signatures/assignments to the new interface types. |
Comments suppressed due to low confidence (1)
src/Validation/src/IValidatableParameterInfo.cs:12
- The interface split introduces new names, but there are still many references to the removed IValidatableInfo across the repo (e.g., src/Http/Routing/src/ValidationEndpointFilterFactory.cs, src/Components/Forms/src/EditContextDataAnnotationsExtensions.cs, src/Http/Http/perf/Microbenchmarks/ValidatableTypesBenchmark.cs, and multiple Validation tests/snapshots). These will cause compilation failures unless updated to IValidatableTypeInfo/IValidatableParameterInfo/IValidatablePropertyInfo as appropriate.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| /// Represents an interface for validating a value. | ||
| /// Represents an interface for validating a value of a parameter. | ||
| /// </summary> | ||
| [Experimental("ASP0029", UrlFormat = "https://aka.ms/aspnet/analyzer/{0}")] |
There was a problem hiding this comment.
just to confirm: we did not plan to remove [Experimental] on the new types?
There was a problem hiding this comment.
Not yet. That needs to be another API review, I think.
The ask here was only to split the interfaces. Removing experimental and reviewing the whole API surface is a different question.
|
@copilot Update baseline for Microsoft.Extensions.Validation.GeneratorTests so that the tests pass. |
Co-authored-by: Youssef1313 <31348972+Youssef1313@users.noreply.github.com>
Updated the generator test baselines so |
| } | ||
|
|
||
| var baseType = Type.BaseType; | ||
| while (baseType is not null) |
There was a problem hiding this comment.
nit: We used to have a baseType != typeof(object) check here
There was a problem hiding this comment.
It's a small extra call to TryGetValidatableTypeInfo which will most likely return false right away.
And if one of the resolvers (custom IValidatableInfoResolver) is able to resolve typeof(object), then we should actually be respecting it and not silently dropping it IMO, so it's more correct to consider typeof(object).
| DisplayNameInfo = displayNameInfo; | ||
| _membersCount = members.Count; | ||
| _superTypes = type.GetAllImplementedTypes(); | ||
| _implementedInterfaces = type.GetInterfaces(); |
There was a problem hiding this comment.
Any particular reason to change this? Looks like it just adds mostly duplicated code.
There was a problem hiding this comment.
As I'm refactoring the code to avoid the ValidatableTypeInfo type check in this code path, the recursion can no longer call into FindLocalMember and instead needs to call into TryFindProperty.
If I blindly loop over all subtypes and call TryFindProperty, the algorithm will become exponential, as the direct subtype could already search the base types, and then the base types search the same base types again, etc. However, for interfaces, we want to make sure we walk all interfaces.
So I refactored it in a way that we walk all interfaces, but then we walk only the first base type for which we have an IValidatableTypeInfo implementation.
|
Link the issue to this PR please. |
Ahh, Copilot keeps overwriting the PR description 😕 |
|
/ba-g macOS instability |
Fixes #67080