-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Emit validation info for types that have IValidatableObject interface and no validation attributes #63414
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
Emit validation info for types that have IValidatableObject interface and no validation attributes #63414
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -82,7 +82,7 @@ internal bool TryExtractValidatableType(ITypeSymbol incomingTypeSymbol, WellKnow | |
|
|
||
| visitedTypes.Add(typeSymbol); | ||
|
|
||
| var hasValidationAttributes = HasValidationAttributes(typeSymbol, wellKnownTypes); | ||
| var hasTypeLevelValidation = HasValidationAttributes(typeSymbol, wellKnownTypes) || HasIValidatableObjectInterface(typeSymbol, wellKnownTypes); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider using the ImplementsInterface method here to do the check.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, replaced |
||
|
|
||
| // Extract validatable types discovered in base types of this type and add them to the top-level list. | ||
| var current = typeSymbol.BaseType; | ||
|
|
@@ -109,7 +109,7 @@ internal bool TryExtractValidatableType(ITypeSymbol incomingTypeSymbol, WellKnow | |
| } | ||
|
|
||
| // No validatable members or derived types found, so we don't need to add this type. | ||
| if (members.IsDefaultOrEmpty && !hasValidationAttributes && !hasValidatableBaseType && !hasValidatableDerivedTypes) | ||
| if (members.IsDefaultOrEmpty && !hasTypeLevelValidation && !hasValidatableBaseType && !hasValidatableDerivedTypes) | ||
| { | ||
| return false; | ||
| } | ||
|
|
@@ -301,4 +301,19 @@ internal static bool HasValidationAttributes(ISymbol symbol, WellKnownTypes well | |
|
|
||
| return false; | ||
| } | ||
|
|
||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: explicit type would have been nice for readability |
||
|
|
||
| foreach (var inter in typeSymbol.AllInterfaces) | ||
| { | ||
| if (SymbolEqualityComparer.Default.Equals(inter, validatableObjectSymbol)) | ||
| { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -164,4 +164,101 @@ async Task ValidateForTopLevelInvoked() | |
| } | ||
| }); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task CanValidateIValidatableObject_WithoutPropertyValidations() | ||
| { | ||
| var source = """ | ||
| using System.Collections.Generic; | ||
| using System.ComponentModel.DataAnnotations; | ||
| using System.Threading.Tasks; | ||
| using Microsoft.AspNetCore.Builder; | ||
| using Microsoft.AspNetCore.Http; | ||
| using Microsoft.Extensions.Validation; | ||
| using Microsoft.AspNetCore.Mvc; | ||
| using Microsoft.AspNetCore.Routing; | ||
| using Microsoft.Extensions.DependencyInjection; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a lot of 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; |
||
|
|
||
| WebApplicationBuilder builder = WebApplication.CreateBuilder(args); | ||
|
|
||
| builder.Services.AddValidation(); | ||
|
|
||
| WebApplication app = builder. Build(); | ||
|
|
||
| app.MapPost("/base", (BaseClass model) => Results.Ok(model)); | ||
| app.MapPost("/derived", (DerivedClass model) => Results.Ok(model)); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, added the test case |
||
|
|
||
| app.Run(); | ||
|
|
||
| public class BaseClass : IValidatableObject | ||
| { | ||
| public string? Value { get; set; } | ||
|
|
||
| public IEnumerable<ValidationResult> Validate(ValidationContext validationContext) | ||
| { | ||
| if (string.IsNullOrEmpty(Value)) | ||
| { | ||
| yield return new ValidationResult("Value cannot be null or empty.", [nameof(Value)]); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| public class DerivedClass : BaseClass | ||
| { | ||
| } | ||
| """; | ||
|
|
||
| await Verify(source, out var compilation); | ||
|
|
||
| await VerifyEndpoint(compilation, "/base", async (endpoint, serviceProvider) => | ||
| { | ||
| await ValidateMethodCalled(); | ||
|
|
||
| async Task ValidateMethodCalled() | ||
| { | ||
| var httpContext = CreateHttpContextWithPayload(""" | ||
| { | ||
| "Value": "" | ||
| } | ||
| """, serviceProvider); | ||
|
|
||
| await endpoint.RequestDelegate(httpContext); | ||
|
|
||
| var problemDetails = await AssertBadRequest(httpContext); | ||
| Assert.Collection(problemDetails.Errors, | ||
| error => | ||
| { | ||
| Assert.Equal("Value", error.Key); | ||
| Assert.Collection(error.Value, | ||
| msg => Assert.Equal("Value cannot be null or empty.", msg)); | ||
| }); | ||
| } | ||
| }); | ||
|
|
||
| await VerifyEndpoint(compilation, "/derived", async (endpoint, serviceProvider) => | ||
| { | ||
| await ValidateMethodCalled(); | ||
|
|
||
| async Task ValidateMethodCalled() | ||
| { | ||
| var httpContext = CreateHttpContextWithPayload(""" | ||
| { | ||
| "Value": "" | ||
| } | ||
| """, serviceProvider); | ||
|
|
||
| await endpoint.RequestDelegate(httpContext); | ||
|
|
||
| var problemDetails = await AssertBadRequest(httpContext); | ||
| Assert.Collection(problemDetails.Errors, | ||
| error => | ||
| { | ||
| Assert.Equal("Value", error.Key); | ||
| Assert.Collection(error.Value, | ||
| msg => Assert.Equal("Value cannot be null or empty.", msg)); | ||
| }); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,190 @@ | ||
| //HintName: ValidatableInfoResolver.g.cs | ||
| #nullable enable annotations | ||
| //------------------------------------------------------------------------------ | ||
| // <auto-generated> | ||
| // This code was generated by a tool. | ||
| // | ||
| // Changes to this file may cause incorrect behavior and will be lost if | ||
| // the code is regenerated. | ||
| // </auto-generated> | ||
| //------------------------------------------------------------------------------ | ||
| #nullable enable | ||
| #pragma warning disable ASP0029 | ||
|
|
||
| namespace System.Runtime.CompilerServices | ||
| { | ||
| [global::System.CodeDom.Compiler.GeneratedCodeAttribute("Microsoft.Extensions.Validation.ValidationsGenerator, Version=42.42.42.42, Culture=neutral, PublicKeyToken=adb9793829ddae60", "42.42.42.42")] | ||
| [AttributeUsage(AttributeTargets.Method, AllowMultiple = true)] | ||
| file sealed class InterceptsLocationAttribute : System.Attribute | ||
| { | ||
| public InterceptsLocationAttribute(int version, string data) | ||
| { | ||
| } | ||
| } | ||
| } | ||
|
|
||
| namespace Microsoft.Extensions.Validation.Generated | ||
| { | ||
| [global::System.CodeDom.Compiler.GeneratedCodeAttribute("Microsoft.Extensions.Validation.ValidationsGenerator, Version=42.42.42.42, Culture=neutral, PublicKeyToken=adb9793829ddae60", "42.42.42.42")] | ||
| file sealed class GeneratedValidatablePropertyInfo : global::Microsoft.Extensions.Validation.ValidatablePropertyInfo | ||
| { | ||
| public GeneratedValidatablePropertyInfo( | ||
| [param: global::System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembers(global::System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.PublicProperties | global::System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.PublicConstructors)] | ||
| global::System.Type containingType, | ||
| global::System.Type propertyType, | ||
| string name, | ||
| string displayName) : base(containingType, propertyType, name, displayName) | ||
| { | ||
| ContainingType = containingType; | ||
| Name = name; | ||
| } | ||
|
|
||
| [global::System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembers(global::System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.PublicProperties | global::System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.PublicConstructors)] | ||
| internal global::System.Type ContainingType { get; } | ||
| internal string Name { get; } | ||
|
|
||
| protected override global::System.ComponentModel.DataAnnotations.ValidationAttribute[] GetValidationAttributes() | ||
| => ValidationAttributeCache.GetPropertyValidationAttributes(ContainingType, Name); | ||
| } | ||
|
|
||
| [global::System.CodeDom.Compiler.GeneratedCodeAttribute("Microsoft.Extensions.Validation.ValidationsGenerator, Version=42.42.42.42, Culture=neutral, PublicKeyToken=adb9793829ddae60", "42.42.42.42")] | ||
| file sealed class GeneratedValidatableTypeInfo : global::Microsoft.Extensions.Validation.ValidatableTypeInfo | ||
| { | ||
| public GeneratedValidatableTypeInfo( | ||
| [param: global::System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembers(global::System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.Interfaces)] | ||
| global::System.Type type, | ||
| ValidatablePropertyInfo[] members) : base(type, members) | ||
| { | ||
| Type = type; | ||
| } | ||
|
|
||
| [global::System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembers(global::System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.Interfaces)] | ||
| internal global::System.Type Type { get; } | ||
|
|
||
| protected override global::System.ComponentModel.DataAnnotations.ValidationAttribute[] GetValidationAttributes() | ||
| => ValidationAttributeCache.GetTypeValidationAttributes(Type); | ||
| } | ||
|
|
||
| [global::System.CodeDom.Compiler.GeneratedCodeAttribute("Microsoft.Extensions.Validation.ValidationsGenerator, Version=42.42.42.42, Culture=neutral, PublicKeyToken=adb9793829ddae60", "42.42.42.42")] | ||
| file class GeneratedValidatableInfoResolver : global::Microsoft.Extensions.Validation.IValidatableInfoResolver | ||
| { | ||
| public bool TryGetValidatableTypeInfo(global::System.Type type, [global::System.Diagnostics.CodeAnalysis.NotNullWhen(true)] out global::Microsoft.Extensions.Validation.IValidatableInfo? validatableInfo) | ||
| { | ||
| validatableInfo = null; | ||
| if (type == typeof(global::BaseClass)) | ||
| { | ||
| validatableInfo = new GeneratedValidatableTypeInfo( | ||
| type: typeof(global::BaseClass), | ||
| members: [] | ||
| ); | ||
| return true; | ||
| } | ||
| if (type == typeof(global::DerivedClass)) | ||
| { | ||
| validatableInfo = new GeneratedValidatableTypeInfo( | ||
| type: typeof(global::DerivedClass), | ||
| members: [] | ||
| ); | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| // No-ops, rely on runtime code for ParameterInfo-based resolution | ||
| public bool TryGetValidatableParameterInfo(global::System.Reflection.ParameterInfo parameterInfo, [global::System.Diagnostics.CodeAnalysis.NotNullWhen(true)] out global::Microsoft.Extensions.Validation.IValidatableInfo? validatableInfo) | ||
| { | ||
| validatableInfo = null; | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| [global::System.CodeDom.Compiler.GeneratedCodeAttribute("Microsoft.Extensions.Validation.ValidationsGenerator, Version=42.42.42.42, Culture=neutral, PublicKeyToken=adb9793829ddae60", "42.42.42.42")] | ||
| file static class GeneratedServiceCollectionExtensions | ||
| { | ||
| [InterceptsLocation] | ||
| public static global::Microsoft.Extensions.DependencyInjection.IServiceCollection AddValidation(this global::Microsoft.Extensions.DependencyInjection.IServiceCollection services, global::System.Action<global::Microsoft.Extensions.Validation.ValidationOptions>? configureOptions = null) | ||
| { | ||
| // Use non-extension method to avoid infinite recursion. | ||
| return global::Microsoft.Extensions.DependencyInjection.ValidationServiceCollectionExtensions.AddValidation(services, options => | ||
| { | ||
| options.Resolvers.Insert(0, new GeneratedValidatableInfoResolver()); | ||
| if (configureOptions is not null) | ||
| { | ||
| configureOptions(options); | ||
| } | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| [global::System.CodeDom.Compiler.GeneratedCodeAttribute("Microsoft.Extensions.Validation.ValidationsGenerator, Version=42.42.42.42, Culture=neutral, PublicKeyToken=adb9793829ddae60", "42.42.42.42")] | ||
| file static class ValidationAttributeCache | ||
| { | ||
| private sealed record CacheKey( | ||
| [param: global::System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembers(global::System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.PublicProperties | global::System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.PublicConstructors)] | ||
| [property: global::System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembers(global::System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.PublicProperties | global::System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.PublicConstructors)] | ||
| global::System.Type ContainingType, | ||
| string PropertyName); | ||
| private static readonly global::System.Collections.Concurrent.ConcurrentDictionary<CacheKey, global::System.ComponentModel.DataAnnotations.ValidationAttribute[]> _propertyCache = new(); | ||
| private static readonly global::System.Lazy<global::System.Collections.Concurrent.ConcurrentDictionary<global::System.Type, global::System.ComponentModel.DataAnnotations.ValidationAttribute[]>> _lazyTypeCache = new (() => new ()); | ||
| private static global::System.Collections.Concurrent.ConcurrentDictionary<global::System.Type, global::System.ComponentModel.DataAnnotations.ValidationAttribute[]> TypeCache => _lazyTypeCache.Value; | ||
|
|
||
| public static global::System.ComponentModel.DataAnnotations.ValidationAttribute[] GetPropertyValidationAttributes( | ||
| [global::System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembers(global::System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.PublicProperties | global::System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.PublicConstructors)] | ||
| global::System.Type containingType, | ||
| string propertyName) | ||
| { | ||
| var key = new CacheKey(containingType, propertyName); | ||
| return _propertyCache.GetOrAdd(key, static k => | ||
| { | ||
| var results = new global::System.Collections.Generic.List<global::System.ComponentModel.DataAnnotations.ValidationAttribute>(); | ||
|
|
||
| // Get attributes from the property | ||
| var property = k.ContainingType.GetProperty(k.PropertyName); | ||
| if (property != null) | ||
| { | ||
| var propertyAttributes = global::System.Reflection.CustomAttributeExtensions | ||
| .GetCustomAttributes<global::System.ComponentModel.DataAnnotations.ValidationAttribute>(property, inherit: true); | ||
|
|
||
| results.AddRange(propertyAttributes); | ||
| } | ||
|
|
||
| // Check constructors for parameters that match the property name | ||
| // to handle record scenarios | ||
| foreach (var constructor in k.ContainingType.GetConstructors()) | ||
| { | ||
| // Look for parameter with matching name (case insensitive) | ||
| var parameter = global::System.Linq.Enumerable.FirstOrDefault( | ||
| constructor.GetParameters(), | ||
| p => string.Equals(p.Name, k.PropertyName, global::System.StringComparison.OrdinalIgnoreCase)); | ||
|
|
||
| if (parameter != null) | ||
| { | ||
| var paramAttributes = global::System.Reflection.CustomAttributeExtensions | ||
| .GetCustomAttributes<global::System.ComponentModel.DataAnnotations.ValidationAttribute>(parameter, inherit: true); | ||
|
|
||
| results.AddRange(paramAttributes); | ||
|
|
||
| break; | ||
| } | ||
| } | ||
|
|
||
| return results.ToArray(); | ||
| }); | ||
| } | ||
|
|
||
|
|
||
| public static global::System.ComponentModel.DataAnnotations.ValidationAttribute[] GetTypeValidationAttributes( | ||
| [global::System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembers(global::System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.Interfaces)] | ||
| global::System.Type type | ||
| ) | ||
| { | ||
| return TypeCache.GetOrAdd(type, static t => | ||
| { | ||
| var typeAttributes = global::System.Reflection.CustomAttributeExtensions | ||
| .GetCustomAttributes<global::System.ComponentModel.DataAnnotations.ValidationAttribute>(t, inherit: true); | ||
| return global::System.Linq.Enumerable.ToArray(typeAttributes); | ||
| }); | ||
| } | ||
| } | ||
| } |
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: the name is clearer as hasValidationAttributes.
hasTypeLevelValidationmakes it look like its not considering properties.Uh oh!
There was an error while loading. Please reload this page.
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.
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.
hasValidationAttributesreferred to class attributes,hasTypeLevelValidationnow means the type either has class attributes or implementsIValidatableObject.