-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Fix handling of parsable types in validations generator #61728
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
Conversation
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.
Pull Request Overview
This PR fixes the handling of parsable types in the validations generator by adding additional known type data and refactoring the generator code.
- Added new well-known types (e.g. JsonDerivedTypeAttribute, DataAnnotations attributes) to support enhanced validations.
- Updated tests and snapshots to cover parsable properties.
- Refactored generator parsers and extensions to replace RequiredSymbols with WellKnownTypes.
Reviewed Changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
src/Shared/RoslynUtils/WellKnownTypeData.cs | Added new entries to support additional attributes. |
src/Http/Http.Extensions/test/ValidationsGenerator/snapshots/... | Updated expected test snapshots to reflect changes. |
src/Http/Http.Extensions/test/ValidationsGenerator/ValidationsGenerator.Parsable.cs | Added tests to validate parsable property handling. |
src/Http/Http.Extensions/gen/Microsoft.AspNetCore.Http.ValidationsGenerator/ValidationsGenerator.cs | Simplified generator initialization by removing unnecessary symbols resolution. |
Other generator/parsers files | Refactored to use WellKnownTypes, ensuring proper resolution of types. |
Files not reviewed (1)
- src/Http/Http.Extensions/gen/Microsoft.AspNetCore.Http.ValidationsGenerator/Microsoft.AspNetCore.Http.ValidationsGenerator.csproj: Language not supported
// Extract validatable types discovered in members of this type and add them to the top-level list. | ||
var members = ExtractValidatableMembers(typeSymbol, requiredSymbols, ref validatableTypes, ref visitedTypes); | ||
ImmutableArray<ValidatableProperty> members = []; | ||
if (ParsabilityHelper.GetParsability(typeSymbol, wellKnownTypes) is Parsability.NotParsable) |
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 and the change in ValidationsGenerator are the only real changes in this PR?
And this is removing TryParseable types from the validatable types? But somehow they are still validatable?
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 and the change in ValidationsGenerator are the only real changes in this PR?
Yep, this is the main change. All the WellKnownType
stuff is a reaction to the fact that this API needs it.
And this is removing TryParseable types from the validatable types? But somehow they are still validatable?
We'll still generate ValidatablePropertyInfo
instances for them so that we'll validate attributes on the properties. This change does mean that we won't validate properties with validation attributes inside the parsable types. None of the properties in the case below get validated. I'd have to check how MVC handles this...if it does...
The right pattern hear might be to require that custom parsable types implement IValidatableObject
to handle validation.
public class Person
{
[Required]
[StringLength(100, MinimumLength = 2)]
public string FirstName { get; private set; } = string.Empty;
[Required]
[StringLength(100, MinimumLength = 2)]
public string LastName { get; private set; } = string.Empty;
[Range(0, 120)]
public int Age { get; private set; }
// Static TryParse method
public static bool TryParse(string input, out Person? person) { }
}
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.
Update: it looks like MVC doesn't validate properties on a TryParse
by default:
#:sdk Microsoft.NET.Sdk.Web
#:property TargetFramework net10.0
using Microsoft.AspNetCore.Mvc;
using System.ComponentModel.DataAnnotations;
var builder = WebApplication.CreateBuilder(args);
builder.Services.AddControllers();
var app = builder.Build();
app.MapControllers();
app.Run();
[ApiController]
[Route("[controller]")]
public class PersonController : ControllerBase
{
// This endpoint binds PersonInput from query string
[HttpGet]
public IActionResult CreatePerson([FromQuery] PersonInput personInput)
{
if (!ModelState.IsValid)
{
return BadRequest(ModelState);
}
return Ok(new { Message = $"Created person: {personInput.FirstName} {personInput.LastName}, Age {personInput.Age}" });
}
}
public class PersonInput
{
[Required]
[StringLength(100, MinimumLength = 2)]
public string FirstName { get; init; } = string.Empty;
[Required]
[StringLength(100, MinimumLength = 2)]
public string LastName { get; init; } = string.Empty;
[Range(0, 120)]
public int Age { get; init; }
// TryParse for parsing raw CSV strings (no validation inside)
public static bool TryParse(string input, out PersonInput? person)
{
person = null;
if (string.IsNullOrWhiteSpace(input))
return false;
var parts = input.Split(',', StringSplitOptions.TrimEntries);
if (parts.Length != 3)
return false;
if (!int.TryParse(parts[2], out int age))
return false;
person = new PersonInput
{
FirstName = parts[0],
LastName = parts[1],
Age = age
};
return true;
}
}
$ curl http://localhost:5000/person?personInput=S,A,128
{
"message": "Created person: S A, Age 128"
}
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 right pattern hear might be to require that custom parsable types implement
IValidatableObject
to handle validation.
Does this work now, or are you saying we might want to make this work in the future?
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.
It doesn't work now. We would need to add a check to see if the type implements IValidatableObject
and if so generate an IValidatableInfo instance for that member type.
Closes #61525.