Fix handling of record types in validations source generator #61402
Conversation
ghost
added
the
area-networking
captainsafia
added
area-minimal
dotnet-policy-service
bot
added
the
pending-ci-rerun
|
/azp run |
dotnet-policy-service
bot
removed
the
pending-ci-rerun
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
Bump @BrennanConroy and @DeagleGross for a review if you have the chance. |
.../gen/Microsoft.AspNetCore.Http.ValidationsGenerator/Emitters/ValidationsGenerator.Emitter.cs
Outdated
Show resolved
Hide resolved
...sions/gen/Microsoft.AspNetCore.Http.ValidationsGenerator/Extensions/ITypeSymbolExtensions.cs
Outdated
Show resolved
Hide resolved
...n/Microsoft.AspNetCore.Http.ValidationsGenerator/Parsers/ValidationsGenerator.TypesParser.cs
Show resolved
Hide resolved
| public override bool IsValid(object? value) => value is int number && number % 2 == 0; | ||
| } | ||
| public record SubType([Required] string RequiredProperty = "some-value", [StringLength(10)] string? StringWithLength = default); |
There was a problem hiding this comment.
Should test with a record like:
public record Person
{
public required string FirstName { get; init; }
public required string LastName { get; init; }
};
There was a problem hiding this comment.
I'm guessing we don't have any tests for non-records with primary ctors?
There was a problem hiding this comment.
Nope, I need to add tests for both those scenarios.
mikekistler
left a comment
mikekistler
left a comment
There was a problem hiding this comment.
Left one comment on the attribute syntax. Rest looked good to a PM's eyes.
| } | ||
| public record ValidatableRecord( | ||
| [Range(10, 100)] |
There was a problem hiding this comment.
I thought validation attributes had to have property: on them to be effective -- is that not true?
I have confirmed that the attribute on the parameter does not result in the range being reflected in the generated OpenApi document -- so at the very least there is some inconsistency here I think.
There was a problem hiding this comment.
I thought validation attributes had to have property: on them to be effective -- is that not true?
That's only a constraint that exists in the MVC universe. Since we have the opportunity to deliver a better experience here I figured we'd do that.
I have confirmed that the attribute on the parameter does not result in the range being reflected in the generated OpenApi document -- so at the very least there is some inconsistency here I think.
Yep, that's a separate bug we should file -- especially now that minimal APIs support both. Although we'll want to be mindful of only respecting it for minimal APIs. Hmmm.... 🤔
Addresses #61379.