dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
34.82k stars 9.84k forks source link

Support property-targetted validation attributes on records #34083

Open bruno-brant opened 3 years ago

bruno-brant commented 3 years ago

Is your feature request related to a problem? Please describe.

I am trying to do use validation attributes on a record that is also used as a controller model but model binding fails with a rather cryptic error, "'Foo' has validation metadata defined on property 'Bar' that will be ignored. 'Bar' is a parameter in the record primary constructor and validation metadata must be associated with the constructor parameter."

(this has already been reported in issue #30650 but it has since been closed)

Describe the solution you'd like

ASP.NET MVC should allow me to use records that have validation attributes on properties as models.

javiercn commented 3 years ago

@bruno-brant thanks for contacting us.

@pranavkm do you have any thoughts?

pranavkm commented 3 years ago

For record types, MVC expects binding and validation to be specified on the parameter, rather than on the property. Here's what this looks like: https://docs.microsoft.com/en-us/aspnet/core/release-notes/aspnetcore-5.0?view=aspnetcore-5.0#model-binding-and-validation-with-c-9-record-types.

bruno-brant commented 3 years ago

Hey @pranavkm, thanks for the reply.

The thing is, what if I want to use a record type that has a validation tag on the property? For instance, I'm using a record that belongs to my domain, where I use ComponentModel to validate the type instance; I opted to reuse that type on the controller.

It seems ASP.NET is assuming that records used as models are exclusively created for that purpose, but I don't think that assumption is really justified. I think it's accidental, isn't it? Otherwise, the same would hold true for a class...

Put it another way: why would ASP.NET apply this restriction to record types and not to class types? Records are special cases of classes (or so I believe) and I think it's arguable that this is a violation of LSP in a way - if a record is a class with synthesized parts, then why is ASP.NET model binder distinguishing between usual classes and records?

pranavkm commented 3 years ago

Thanks @bruno-brant, that's a legitimate use case for this that I don't have a good resolution for you at this time.

Record types were the first time MVC performed model binding and validation on constructor parameters. We wanted to avoid users providing us conflicting constraints and having the framework have it resolve it,

public sealed record MyRecord([StringLength(10, MinimumLength = 4)] string MyString)
{
    [StringLength(8, MinimumLength = 5)] public string MyString { get; init; } = MyString
}

so we picked restricting these attributes to be specific on the ctor. These kinds of scenarios don't really come up with classes. I could see us saying attributes are only allowed on properties based on a switch, because combining attributes across parameters and properties was what we were primarily trying to avoid.

ghost commented 3 years ago

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

bruno-brant commented 3 years ago

@pranavkm all your points make sense. I'm not sure what would be the best way to go about this either, because supporting constructor binding comes with this possibility of conflict.

If I come up with something I'll be sure to chip in. Thanks for the attention.

devtrends commented 2 years ago

Note that you cannot read validation attributes on record constructor parameters from a tag helper, unlike when you use a class or normal record properties.

var stringLengthAttribute = For.Metadata.ValidatorMetadata.OfType<StringLengthAttribute>().FirstOrDefault();

This returns null for [StringLength(50)] but if I change it to [property: StringLength(50)] then the tag helper works but I get the OP's error when binding.

momvart commented 2 years ago

I truly agree with @bruno-brant. A simple example is Swashbuckle, which checks attributes on properties, that cannot currently generate correct documentation if the model is a record.

If the problem is just the conflicts between attributes, why not throw an exception only when multiple attributes are defined for a single member? I mean the validator could perform an XOR for record members' attributes.

DamianEdwards commented 2 years ago

@brunolins16 @captainsafia we might want to look at this given the apparent impact on Swashbuckle scenarios. The inability to use record types nicely with Tag Helpers is also unfortunate.