Emit validation info for types that have IValidatableObject interface and no validation attributes #63414
Conversation
…od and no validation attributes
oroztocil
requested review from
captainsafia and
Copilot
and removed request for
Copilot
github-actions
bot
added
the
needs-area-label
oroztocil
added
feature-validation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug where types implementing IValidatableObject without any validation attributes were not having their validation code generated by the source generator. The fix ensures that such types are properly included in the validation code generation process.
Key changes:
- Modified the type extraction logic to consider
IValidatableObjectinterface implementation - Added a new helper method to check for
IValidatableObjectinterface - Updated well-known types to include
IValidatableObject
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
ValidationsGenerator.TypesParser.cs |
Added logic to check for IValidatableObject interface and include it in type-level validation detection |
WellKnownTypeData.cs |
Added IValidatableObject to the well-known types enum and names array |
ValidationsGenerator.IValidatableObject.cs |
Added comprehensive test case to verify the fix works for both base and derived classes |
ValidatableInfoResolver.g.verified.cs |
Generated test snapshot showing expected output for types with only IValidatableObject validation |
|
/backport to release/10.0 |
|
Started backporting to release/10.0: https://github.com/dotnet/aspnetcore/actions/runs/17212498300 |
| visitedTypes.Add(typeSymbol); | ||
|
|
||
| var hasValidationAttributes = HasValidationAttributes(typeSymbol, wellKnownTypes); | ||
| var hasTypeLevelValidation = HasValidationAttributes(typeSymbol, wellKnownTypes) || HasIValidatableObjectInterface(typeSymbol, wellKnownTypes); |
There was a problem hiding this comment.
nit: the name is clearer as hasValidationAttributes. hasTypeLevelValidation makes it look like its not considering properties.
There was a problem hiding this comment.
This is a result of a partial check that gets ORed in a larger disjunction at the end of the method with other checks (the other checks need to be done because they also collect some data as out params so the logic cannot reasonably short-circuit). Property attributes are handled in one of the other checks.
hasValidationAttributes referred to class attributes, hasTypeLevelValidation now means the type either has class attributes or implements IValidatableObject.
javiercn
left a comment
javiercn
left a comment
There was a problem hiding this comment.
Change looks good, but I defer to @captainsafia for the final review.
| visitedTypes.Add(typeSymbol); | ||
|
|
||
| var hasValidationAttributes = HasValidationAttributes(typeSymbol, wellKnownTypes); | ||
| var hasTypeLevelValidation = HasValidationAttributes(typeSymbol, wellKnownTypes) || HasIValidatableObjectInterface(typeSymbol, wellKnownTypes); |
There was a problem hiding this comment.
Consider using the ImplementsInterface method here to do the check.
| WebApplication app = builder. Build(); | ||
| app.MapPost("/base", (BaseClass model) => Results.Ok(model)); | ||
| app.MapPost("/derived", (DerivedClass model) => Results.Ok(model)); |
There was a problem hiding this comment.
We should add a case that validates that a class with a property that only implements IValidatableObject also works. I don't anticipate that this will require any code changes since the recursion already handles that.
There was a problem hiding this comment.
Agreed, added the test case
captainsafia
left a comment
captainsafia
left a comment
There was a problem hiding this comment.
LGTM overall. I don't have any blocking comments but left a note inline about additional tests.
|
/backport to release/10.0 |
|
Started backporting to release/10.0: https://github.com/dotnet/aspnetcore/actions/runs/17234547157 |
|
|
||
| internal static bool HasIValidatableObjectInterface(ITypeSymbol typeSymbol, WellKnownTypes wellKnownTypes) | ||
| { | ||
| var validatableObjectSymbol = wellKnownTypes.Get(WellKnownTypeData.WellKnownType.System_ComponentModel_DataAnnotations_IValidatableObject); |
There was a problem hiding this comment.
nit: explicit type would have been nice for readability
| using Microsoft.Extensions.Validation; | ||
| using Microsoft.AspNetCore.Mvc; | ||
| using Microsoft.AspNetCore.Routing; | ||
| using Microsoft.Extensions.DependencyInjection; |
There was a problem hiding this comment.
This is a lot of usings for a simple app.
Shouldn't the following be enough,
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Routing;
using Microsoft.Extensions.DependencyInjection;
using System.Collections.Generic;
using System.ComponentModel.DataAnnotations;
Currently, when you have a type that is validated only with the IValidatbleObject method (i.e. the type has no validation attributes or properties with validation attributes), then the validation source generator does not create the necessary code for that type. This leads to the validation rule being ignored during run-time.
This PR changes the logic in
ValidationsGenerator.ExtractValidatableTypesso that such types get their validation code properly emitted.Fixes #63394