-
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 all 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,10 @@ 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 |
||
| return typeSymbol.ImplementsInterface(validatableObjectSymbol); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -164,4 +164,145 @@ 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.MapPost("/complex", (ComplexClass model) => Results.Ok(model)); | ||
|
|
||
| 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 | ||
| { | ||
| } | ||
|
|
||
| public class ComplexClass | ||
| { | ||
| public NestedClass? NestedObject { get; set; } | ||
| } | ||
|
|
||
| public class NestedClass : 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)]); | ||
| } | ||
| } | ||
| } | ||
| """; | ||
|
|
||
| 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)); | ||
| }); | ||
| } | ||
| }); | ||
|
|
||
| await VerifyEndpoint(compilation, "/complex", async (endpoint, serviceProvider) => | ||
| { | ||
| await ValidateMethodCalled(); | ||
|
|
||
| async Task ValidateMethodCalled() | ||
| { | ||
| var httpContext = CreateHttpContextWithPayload(""" | ||
| { | ||
| "NestedObject": { | ||
| "Value": "" | ||
| } | ||
| } | ||
| """, serviceProvider); | ||
|
|
||
| await endpoint.RequestDelegate(httpContext); | ||
|
|
||
| var problemDetails = await AssertBadRequest(httpContext); | ||
| Assert.Collection(problemDetails.Errors, | ||
| error => | ||
| { | ||
| Assert.Equal("NestedObject.Value", error.Key); | ||
| Assert.Collection(error.Value, | ||
| msg => Assert.Equal("Value cannot be null or empty.", msg)); | ||
| }); | ||
| } | ||
| }); | ||
| } | ||
| } | ||
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.