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
35.22k stars 9.95k forks source link

Add support for TypeConverter on propertes/parameters in MVC #8857

Open rynowak opened 5 years ago

rynowak commented 5 years ago

Discussed here originally: https://github.com/aspnet/AspNetCore/issues/8846

This is a really simple way to implement model binding behaviour, and is a totally appropriate thing to do when you have a single field in the request that maps to a complex object.

It would be easy for us to support this because we already have infrastructure that users model binding to call a type converter. We already use type converters defined on types, what this would add would be the ability to use types like List<> and apply the attribute to provide a converter.

rynowak commented 5 years ago

@artakm - creating this to track the scenario, we just had this suggested and it's a pretty good idea.

dahauns commented 3 years ago

I'd add to this that it's not just for complex objects, even something simple like, say, mapping a 0/1 query parameter to a bool isn't possible right now with a TypeConverterAttribute annotation on the property.

Xriuk commented 2 years ago

No updates?

andriysavin commented 2 years ago

I'm surprised this issue is so overlooked and is not even planned for v7.0! There are many scenarios when for a view model property you need to use a type which you don't own, so can't attach a converter to the type itself.

brunolins16 commented 2 years ago

That sounds a good idea, and I am not sure if that is the case, but probably it was overlooked because the issue was opened back in 2019 and not a lot of people asking for it since then, can you share more scenarios where it will be a great addition?

Also, just to give an explanation about the current implementation, most of the logic to detect a TypeConverter rely on TypeDescriptor.GetConverter(Type), so, to implement the suggested idea we will need to figure out an additional approach to detect on properties/parameters itself, not only on the type.

Probably it is not new for you 😅 and it should not be the first option, since we recommended you create a type converter if you have a simple string -> SomeType mapping that doesn't require external resources, but you can create a Custom Model Binder and use the ModelBinder attribute to workaround this missing behavior.

That said I would like to share that, we still missing documentation about this new feature, but since in .NET 7 Preview 3, you can bind types that implement a TryParse method in MVC/API action controllers. Of course, it does not cover all scenarios but using the original issue https://github.com/dotnet/aspnetcore/issues/8846 scenario as an example, you can achieve the same without a need of a TypeConverter.

Controller action

/**
* GET /home/personget?friends=james,john
**/
[HttpGet]
public IActionResult PersonGet([FromQuery] Person person)
{
    return Ok(person);
}

Model

public class Person
{
    public Friends Friends { get; set; }
}

public class Friends : List<string>
{
    public Friends(IEnumerable<string> collection)
        : base(collection)
    {

    }

    public static bool TryParse(string? s, out Friends result)
    {
        if  (s is null)
        {
            result = new Friends(Array.Empty<string>());
            return false;
        }

        result = new Friends(s.Split(','));
        return true;
    }
}
andriysavin commented 2 years ago

@brunolins16 Thanks for the hint with TryParse, I think this is a good alternative to TypeConverters. However, since this requires the method to be part of the type, this will not work with third-party types. This can be enhanced by supporting extension methods in addition, but discovery of such extension methods at run time might be problematic.

Regarding custom model binders, this approach is less preferrable due to couple of reasons. First, it's framework-specific, while TypeConverter is a unified way to define a conversion and, once written, can be used from any framework/library. In other words, reusability. Note that there are many already existing TypeConverters, which it'd be nice to reuse as well. Second, it's more advanced/complicated and, while one can learn how it works, any new developer who needs to maintain that code will have to learn that as well. Third, as a consequence of previous two, is discoverability: when someone familiar with .Net needs to use some existing converter, they will intuitively look for a TypeConverter. I believe even advance ASP.NET developers will not initially look for custom binders, knowing they are targeted more advanced scenarios.

As for implementation, it looks like TypeDescriptor.GetConverter(Type) is outdated and asking for new features, e.g. TypeDescriptor.GetConverter(PropertyInfo/ParameterInfo).

Looks like I'm commenting from bottom to top, so more about scenarios. As I already mentioned, there many useful third-party types which can be used in models, but don't define type converters on them or the converters are not doing exactly what you want. Another scenario is when you have some primitive domain model types which are ok to be exposed within a view model, but you don't want to pollute the domain model with additional types and attributes and want to keep TypeConverters separately, attaching them in concrete places where needed.

Another big problem, which probably requires separate discussion (and I believe there is an issue for it) is that all being discussed above is only relevant for binding simple strings to complex types when value provider is query string, form etc. But when it comes to JSON payload in e.g. a POST request, a deserialization happens instead of type conversion, meaning that whole process is now controlled by the deserializer. In this case, even if you have a TypeConverter defined on a model type, it's not guaranteed to work with particular deserializer, like in case with System.Text.Json. Now you have to define deserializer-specific converter (JsonConverter in this case), producing library-specific solution. Now imagine you need to support different media types for payload, e.g. XML or something else. Now you have to provide a converter doing basically the same thing for each of them. No good. In my opinion, every serializer/deserializer should provide support for TypeConverter or maybe use some new unified, framework independent type converter abstraction.

BenMcLean commented 4 months ago

I may be naive here but it seems like the intuitive thing to do would be to try to cast when an explicit TypeConverter is not specified, and then only throw an exception if the cast fails. Or to have a decorator available which would implement this behavior.

tb-mtg commented 3 months ago

Any updates?