-
Notifications
You must be signed in to change notification settings - Fork 10.7k
Fix ValidationsGenerator ITypeParameterSymbol handling #65419
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?
Changes from 7 commits
fd4cedc
e20d0e4
b2b0dce
9e6a22a
bab9c95
fdf8bf6
a58595c
8508a90
85e971f
a8d2674
1556630
9be2650
e95aa0c
333f003
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 |
|---|---|---|
|
|
@@ -75,6 +75,25 @@ internal static bool TryExtractValidatableType(ITypeSymbol incomingTypeSymbol, W | |
| return false; | ||
| } | ||
|
|
||
| // Type parameters (e.g., TRequest from a generic MapCommand<TRequest>() extension) | ||
| // have DeclaredAccessibility == NotApplicable. The concrete type is only known at | ||
| // call sites, not inside the generic method body where the endpoint delegate is | ||
| // defined. Walk constraint types to discover any validatable types reachable | ||
| // through type constraints. | ||
| if (typeSymbol is ITypeParameterSymbol typeParam) | ||
| { | ||
| // Add to visitedTypes BEFORE iterating constraints to prevent | ||
| // infinite recursion through circular constraints such as | ||
| // where T : class, IEnumerable<T> (SEC-001). | ||
| visitedTypes.Add(typeSymbol); | ||
| var foundValidatable = false; | ||
| foreach (var constraintType in typeParam.ConstraintTypes) | ||
| { | ||
| foundValidatable |= TryExtractValidatableType(constraintType, wellKnownTypes, validatableTypes, visitedTypes); | ||
| } | ||
| return foundValidatable; | ||
| } | ||
|
ANcpLua marked this conversation as resolved.
|
||
|
|
||
| // Skip types that are not accessible from generated code | ||
| if (typeSymbol.DeclaredAccessibility is not Accessibility.Public) | ||
| { | ||
|
|
@@ -196,6 +215,16 @@ private static ImmutableArray<ValidatableProperty> ExtractValidatableMembers(ITy | |
| validatableTypes, | ||
| visitedTypes); | ||
|
|
||
| // Skip properties whose type is a type parameter (e.g., TSelf | ||
| // from CRTP pattern RequestBase<TSelf>). The emitter would | ||
| // generate typeof(TSelf) which is not valid C# (CRASH-001). | ||
| // Concrete validatable types reachable through constraints are | ||
| // already discovered by TryExtractValidatableType above. | ||
| if (ContainsTypeParameter(correspondingProperty.Type)) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| members.Add(new ValidatableProperty( | ||
| ContainingTypeFQN: correspondingProperty.ContainingType.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat), | ||
| TypeFQN: correspondingProperty.Type.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat), | ||
|
|
@@ -256,6 +285,14 @@ private static ImmutableArray<ValidatableProperty> ExtractValidatableMembers(ITy | |
| continue; | ||
| } | ||
|
|
||
| // Skip properties whose type is a type parameter (e.g., TSelf | ||
| // from CRTP pattern RequestBase<TSelf>). The emitter would | ||
| // generate typeof(TSelf) which is not valid C# (CRASH-001). | ||
| if (ContainsTypeParameter(member.Type)) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| members.Add(new ValidatableProperty( | ||
| ContainingTypeFQN: member.ContainingType.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat), | ||
| TypeFQN: member.Type.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat), | ||
|
|
@@ -287,4 +324,40 @@ internal static bool HasIValidatableObjectInterface(ITypeSymbol typeSymbol, Well | |
| var validatableObjectSymbol = wellKnownTypes.Get(WellKnownTypeData.WellKnownType.System_ComponentModel_DataAnnotations_IValidatableObject); | ||
| return typeSymbol.ImplementsInterface(validatableObjectSymbol); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Returns true if the given type symbol contains an unresolved type parameter | ||
| /// anywhere in its type tree. This catches not only bare <c>T</c> but also | ||
| /// constructed types like <c>List<T></c>, <c>T[]</c>, <c>T?</c>, and | ||
| /// <c>Dictionary<string, T></c> — all of which would produce invalid | ||
| /// <c>typeof(...)</c> expressions in the emitted code. | ||
| /// </summary> | ||
| private static bool ContainsTypeParameter(ITypeSymbol type) | ||
|
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. Is this code covered by tests?
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. Yes — as of 333f003: |
||
| { | ||
| // Bare type parameter: T, TSelf, TSelf? | ||
| if (type is ITypeParameterSymbol) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| // Array: T[], T[,], List<T>[] | ||
| if (type is IArrayTypeSymbol arrayType) | ||
| { | ||
| return ContainsTypeParameter(arrayType.ElementType); | ||
| } | ||
|
|
||
| // Constructed generic: List<T>, Dictionary<string, T>, Nullable<T>, Func<T, bool> | ||
| if (type is INamedTypeSymbol { IsGenericType: true } namedType) | ||
| { | ||
| foreach (var typeArg in namedType.TypeArguments) | ||
| { | ||
| if (ContainsTypeParameter(typeArg)) | ||
| { | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,293 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using Microsoft.AspNetCore.Http; | ||
|
|
||
| namespace Microsoft.Extensions.Validation.GeneratorTests; | ||
|
|
||
| public partial class ValidationsGeneratorTests : ValidationsGeneratorTestBase | ||
| { | ||
| // Repro for the silent-skip half of issue dotnet/aspnetcore#65418. When the | ||
| // route-handler delegate parameter resolves to an open ITypeParameterSymbol — | ||
| // as happens inside the body of a generic endpoint extension method like | ||
| // MapValidated<T>(this IEndpointRouteBuilder, string) — TryExtractValidatableType | ||
| // on main hits the DeclaredAccessibility check (NotApplicable for type parameters) | ||
| // and silently returns false. The concrete constraint type is therefore never | ||
| // discovered and no typeof(...) check is emitted for it, even though it carries | ||
| // [Required] / [Range] attributes. With the fix the generator walks the type | ||
| // parameter's ConstraintTypes and discovers the concrete validatable type. | ||
| // | ||
| // The snapshot is the bug witness: on main the resolver body is empty (no | ||
| // typeof(global::UserRequest) check); with the fix the resolver contains the | ||
| // type and its members. | ||
| [Fact] | ||
| public async Task CanValidateOpenTypeParameterReachableThroughConstraint() | ||
| { | ||
| var source = """ | ||
| using System; | ||
| using System.ComponentModel.DataAnnotations; | ||
| using Microsoft.AspNetCore.Builder; | ||
| using Microsoft.AspNetCore.Http; | ||
| using Microsoft.AspNetCore.Routing; | ||
| using Microsoft.Extensions.DependencyInjection; | ||
| using Microsoft.Extensions.Validation; | ||
|
|
||
| var builder = WebApplication.CreateBuilder(); | ||
|
|
||
| builder.Services.AddValidation(); | ||
|
|
||
| var app = builder.Build(); | ||
|
|
||
| app.MapValidated<UserRequest>("/users"); | ||
|
|
||
| app.Run(); | ||
|
|
||
| public class UserRequest | ||
| { | ||
| [Required] | ||
| public string Name { get; set; } = "default"; | ||
|
|
||
| [Range(1, 120)] | ||
| public int Age { get; set; } = 25; | ||
| } | ||
|
|
||
| public static class GenericEndpointExtensions | ||
| { | ||
| public static RouteHandlerBuilder MapValidated<T>(this IEndpointRouteBuilder endpoints, string pattern) | ||
| where T : UserRequest | ||
| => endpoints.MapPost(pattern, (T req) => Results.Ok()); | ||
| } | ||
| """; | ||
| await Verify(source, out var compilation); | ||
| await VerifyEndpoint(compilation, "/users", async (endpoint, serviceProvider) => | ||
| { | ||
| await InvalidNameProducesError(endpoint); | ||
| await InvalidAgeProducesError(endpoint); | ||
| await ValidInputProducesNoErrors(endpoint); | ||
|
|
||
| async Task InvalidNameProducesError(Endpoint endpoint) | ||
| { | ||
| var payload = """ | ||
| { | ||
| "Name": "", | ||
| "Age": 30 | ||
| } | ||
| """; | ||
| var context = CreateHttpContextWithPayload(payload, serviceProvider); | ||
|
|
||
| await endpoint.RequestDelegate(context); | ||
|
|
||
| var problemDetails = await AssertBadRequest(context); | ||
| Assert.Collection(problemDetails.Errors, kvp => | ||
| { | ||
| Assert.Equal("Name", kvp.Key); | ||
| Assert.Equal("The Name field is required.", kvp.Value.Single()); | ||
| }); | ||
| } | ||
|
|
||
| async Task InvalidAgeProducesError(Endpoint endpoint) | ||
| { | ||
| var payload = """ | ||
| { | ||
| "Name": "Alice", | ||
| "Age": 0 | ||
| } | ||
| """; | ||
| var context = CreateHttpContextWithPayload(payload, serviceProvider); | ||
|
|
||
| await endpoint.RequestDelegate(context); | ||
|
|
||
| var problemDetails = await AssertBadRequest(context); | ||
| Assert.Collection(problemDetails.Errors, kvp => | ||
| { | ||
| Assert.Equal("Age", kvp.Key); | ||
| Assert.Equal("The field Age must be between 1 and 120.", kvp.Value.Single()); | ||
| }); | ||
| } | ||
|
|
||
| async Task ValidInputProducesNoErrors(Endpoint endpoint) | ||
| { | ||
| var payload = """ | ||
| { | ||
| "Name": "Alice", | ||
| "Age": 30 | ||
| } | ||
| """; | ||
| var context = CreateHttpContextWithPayload(payload, serviceProvider); | ||
| await endpoint.RequestDelegate(context); | ||
|
|
||
| Assert.Equal(200, context.Response.StatusCode); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task CanValidateTypesWithGenericBaseClass() | ||
| { | ||
| var source = """ | ||
| using System; | ||
| using System.ComponentModel.DataAnnotations; | ||
| using Microsoft.AspNetCore.Builder; | ||
| using Microsoft.AspNetCore.Http; | ||
| using Microsoft.Extensions.Validation; | ||
| using Microsoft.Extensions.DependencyInjection; | ||
|
|
||
| var builder = WebApplication.CreateBuilder(); | ||
|
|
||
| builder.Services.AddValidation(); | ||
|
|
||
| var app = builder.Build(); | ||
|
|
||
| app.MapPost("/crtp-class", (CreateOrderCommand request) => Results.Ok("Passed"!)); | ||
| app.MapPost("/crtp-record", (CreateRecordCommand request) => Results.Ok("Passed"!)); | ||
|
|
||
| app.Run(); | ||
|
|
||
| public class CommandBase<TSelf> where TSelf : CommandBase<TSelf> | ||
| { | ||
| [Required] | ||
| public string Name { get; set; } = "default"; | ||
| } | ||
|
|
||
| public class CreateOrderCommand : CommandBase<CreateOrderCommand> | ||
| { | ||
| [Range(1, 1000)] | ||
| public int Quantity { get; set; } = 1; | ||
| } | ||
|
|
||
| public record RecordCommandBase<TSelf> where TSelf : RecordCommandBase<TSelf> | ||
| { | ||
| [Required] | ||
| public string Title { get; set; } = "default"; | ||
| } | ||
|
|
||
| public record CreateRecordCommand : RecordCommandBase<CreateRecordCommand> | ||
| { | ||
| [Range(1, 100)] | ||
| public int Count { get; set; } = 1; | ||
| } | ||
| """; | ||
| await Verify(source, out var compilation); | ||
| await VerifyEndpoint(compilation, "/crtp-class", async (endpoint, serviceProvider) => | ||
| { | ||
| await InvalidNameProducesError(endpoint); | ||
| await InvalidQuantityProducesError(endpoint); | ||
| await ValidInputProducesNoErrors(endpoint); | ||
|
|
||
| async Task InvalidNameProducesError(Endpoint endpoint) | ||
| { | ||
| var payload = """ | ||
| { | ||
| "Name": "", | ||
| "Quantity": 5 | ||
| } | ||
| """; | ||
| var context = CreateHttpContextWithPayload(payload, serviceProvider); | ||
|
|
||
| await endpoint.RequestDelegate(context); | ||
|
|
||
| var problemDetails = await AssertBadRequest(context); | ||
| Assert.Collection(problemDetails.Errors, kvp => | ||
| { | ||
| Assert.Equal("Name", kvp.Key); | ||
| Assert.Equal("The Name field is required.", kvp.Value.Single()); | ||
| }); | ||
| } | ||
|
|
||
| async Task InvalidQuantityProducesError(Endpoint endpoint) | ||
| { | ||
| var payload = """ | ||
| { | ||
| "Name": "valid", | ||
| "Quantity": 0 | ||
| } | ||
| """; | ||
| var context = CreateHttpContextWithPayload(payload, serviceProvider); | ||
|
|
||
| await endpoint.RequestDelegate(context); | ||
|
|
||
| var problemDetails = await AssertBadRequest(context); | ||
| Assert.Collection(problemDetails.Errors, kvp => | ||
| { | ||
| Assert.Equal("Quantity", kvp.Key); | ||
| Assert.Equal("The field Quantity must be between 1 and 1000.", kvp.Value.Single()); | ||
| }); | ||
| } | ||
|
|
||
| async Task ValidInputProducesNoErrors(Endpoint endpoint) | ||
| { | ||
| var payload = """ | ||
| { | ||
| "Name": "valid", | ||
| "Quantity": 5 | ||
| } | ||
| """; | ||
| var context = CreateHttpContextWithPayload(payload, serviceProvider); | ||
| await endpoint.RequestDelegate(context); | ||
|
|
||
| Assert.Equal(200, context.Response.StatusCode); | ||
| } | ||
| }); | ||
|
|
||
| await VerifyEndpoint(compilation, "/crtp-record", async (endpoint, serviceProvider) => | ||
| { | ||
| await InvalidTitleProducesError(endpoint); | ||
| await InvalidCountProducesError(endpoint); | ||
| await ValidRecordInputProducesNoErrors(endpoint); | ||
|
|
||
| async Task InvalidTitleProducesError(Endpoint endpoint) | ||
| { | ||
| var payload = """ | ||
| { | ||
| "Title": "", | ||
| "Count": 5 | ||
| } | ||
| """; | ||
| var context = CreateHttpContextWithPayload(payload, serviceProvider); | ||
|
|
||
| await endpoint.RequestDelegate(context); | ||
|
|
||
| var problemDetails = await AssertBadRequest(context); | ||
| Assert.Collection(problemDetails.Errors, kvp => | ||
| { | ||
| Assert.Equal("Title", kvp.Key); | ||
| Assert.Equal("The Title field is required.", kvp.Value.Single()); | ||
| }); | ||
| } | ||
|
|
||
| async Task InvalidCountProducesError(Endpoint endpoint) | ||
| { | ||
| var payload = """ | ||
| { | ||
| "Title": "valid", | ||
| "Count": 0 | ||
| } | ||
| """; | ||
| var context = CreateHttpContextWithPayload(payload, serviceProvider); | ||
|
|
||
| await endpoint.RequestDelegate(context); | ||
|
|
||
| var problemDetails = await AssertBadRequest(context); | ||
| Assert.Collection(problemDetails.Errors, kvp => | ||
| { | ||
| Assert.Equal("Count", kvp.Key); | ||
| Assert.Equal("The field Count must be between 1 and 100.", kvp.Value.Single()); | ||
| }); | ||
| } | ||
|
|
||
| async Task ValidRecordInputProducesNoErrors(Endpoint endpoint) | ||
| { | ||
| var payload = """ | ||
| { | ||
| "Title": "valid", | ||
| "Count": 5 | ||
| } | ||
| """; | ||
| var context = CreateHttpContextWithPayload(payload, serviceProvider); | ||
| await endpoint.RequestDelegate(context); | ||
|
|
||
| Assert.Equal(200, context.Response.StatusCode); | ||
| } | ||
| }); | ||
| } | ||
| } |
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.
Generally looks reasonable, but I'm not sure of a good use case for this. Why would you make it generic and constrained instead of having the parameter as the type you need right away?
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.
The pattern comes from reusable endpoint mappers in vertical-slice / CQRS-style minimal APIs — one generic helper like
MapValidated<T>(...)registers many request DTOs, so the concrete type only flows through the type parameter. But the stronger motivation is shape-independent: today the generator emitstypeof(TSelf)intoValidatableInfoResolver.g.cswhenever a discovered model uses the CRTP pattern (e.g.class CreateOrderCommand : RequestBase<CreateOrderCommand>used directly as a handler parameter) — invalid C#, so the user's build breaks with errors inside generated code they can't edit. A source generator emitting uncompilable code is a correctness bug regardless of how common the trigger is; the constraint-walking half then makes discovery actually work for the generic-mapper shape instead of silently skipping it.