aspnet / Mvc

[Archived] ASP.NET Core MVC is a model view controller framework for building dynamic web sites with clean separation of concerns, including the merged MVC, Web API, and Web Pages w/ Razor. Project moved to https://github.com/aspnet/AspNetCore
Apache License 2.0
5.62k stars 2.14k forks source link

BodyModelBinder should make use of model providers when binding models #8687

Closed toady closed 5 years ago

toady commented 5 years ago

Currently only ComplexModelBinder is making use of other model binders when binding model properties, while BodyModelBinder is relying on underlying formatter to convert values.

If we have a look at the example of custom model binder described here, it will only work for route values, and not if we have a FromBody model with Author type property which has to be bound from data source.

toady commented 5 years ago

Could probably pass <Type,IModelBinder> dictionary in InputFormatterContext, which would keep backwards compatibility with IInputFormatters that haven't implemented support for this yet.

As for json, that would create a JsonConverter that would attempt to read value as string and pass to corresponding IModelBinder, but preserving correct ModelBindingContext could be an issue.

Also, Newtonsoft.Json does not support async deserializating in JsonConvert yet.

Could have a look at preparing a pull request, if given a green light.

mkArtakMSFT commented 5 years ago

Thanks for contacting us, @lil-Toady. @dougbu, can you please look into this? Thanks!

dougbu commented 5 years ago

@lil-Toady thank-you for your suggestions. Are you proposing an example that uses an inner BodyModelBinder to create an intermediate object and then converts it to the final (requested) type? Or, as the later comment implies, are you proposing a fundamental rewrite of the IInputFormatters?


A few points on your comments:

Currently only ComplexModelBinder is making use of other model binders

The CollectionModelBinder, its subclasses, BinderTypeModelBinder, KeyValuePairModelBinder all use other model binders.

BodyModelBinder is relying on underlying formatter to convert values

The BodyModelBinder uses the underlying formatter to create models. And, the underlying formatters all defer to the underlying serializer (JsonSerializer, XmlSerializer and XmlDataContractSerializer) for the real work.

look at the example of custom model binder described here, it will only work for route values

The AuthorEntityBinder has a few issues e.g. it should not use BodyModelName or a hard-coded default parameter name. But, it supports all value providers, including route values, query string, and form contents.

Note the documentation has been updated slightly. See https://docs.microsoft.com/en-us/aspnet/core/mvc/advanced/custom-model-binding?view=aspnetcore-2.2

Could probably pass <Type,IModelBinder> dictionary in InputFormatterContext

The BodyModelBinder creates the InputFormatterContext and the closest thing to what I think you're proposing (if it's not a new sample in the docs) is the wrapping feature (IWrapperProvider / IWrapperProviderFactory) used for special cases in the XML serializers e.g. dictionaries. Json.NET doesn't have and probably doesn't really need a similar feature.

create a JsonConverter that would attempt to read value as string and pass to corresponding IModelBinder

IModelBinders read from specific sources and only the lowest-level of them (SimpleTypeModelBinder) can convert from a string. Creating IModelBinder implementations that understand JSON or XML would fundamentally alter the entire model binding system.

toady commented 5 years ago

Hi, @dougbu !

I'll try to explain what I'm proposing in detail:

Currently model binding from route values, query string, headers goes by getting the values from IValueProvider which comes in ModelBindingContext and instantiating corresponding models. But that is not the case when we want to bind model from body.

Say we have our models:

public class Foo {
    ...
}
public class Bar {
    public Foo Foo { get; set; }
}

and create our custom model binder FooBinder for Foo, which binds it in our own specific way (say retrieving the value from storage, as in the linked example). An action that accepts Bar will work great, as ComplexModelBinderProvider will give ComplexModelBinder all the binders required to bind properties of Bar, including our custom binder.

But that is not the case if we're trying to bind Bar from body, because BodyModelBinder will outsource all the instantiation to IInputFormatter, that will further pass it to JsonSerializer, XmlSerializer etc. as you mentioned.

Now, if I want the same Bar model to be bound from body, I need to get in depths of IInputFormatter implementation. For example, if it's json, I would have to write a custom JsonConverter that would do the same thing as FooBinder, but now retreiving values from JsonReader; and the same for all the other input formatters supported.

What I'm suggesting is letting IInputFormatters feed values to corresponding IModelBinder during deserialization, instead of letting them do all the work. So in the end when JsonSerializer hits Foo type that we know we have a IModelBinder for, instead of attempting to completely deserialize it itself, it will create an IValueProvider and ModelBindingContext for model binders to take the values they need.

Of course I'm not suggesting to do that at the level of the deserializers themselves, but rather handle at IInputFormatter implementation level, which in case if json (just as an example, again) could work by adding a JsonConverter that will do all the work of providing the values back to instances of IModelBinder that we have.

TLDR: Instead of letting IInputFormatter implementations do the model binding themselves, they should rather work as value providers for the model binding infrastructure.

dougbu commented 5 years ago

if I want the same Bar model to be bound from body, …

As long as Bar is the topmost class that's bound from body, create a custom model binder for Bar that takes an IModelBinder and have the corresponding model binder provider pass a BodyModelBinder instance.

Instead of letting IInputFormatter implementations do the model binding themselves, they should rather work as value providers for the model binding infrastructure.

It's already possible to use [FromBody] and / or BodyModelBinder to bind (input format) properties of a higher-level model. This change would fundamentally alter what a value provider is and does. And, it is not necessary for your scenario.

We have no plans to revise the way model binding uses input formatting and input formatting uses framework-provided and external serializers.


Please let us know if you hit any issues using the BarBinder approach.

mkArtakMSFT commented 5 years ago

Thanks for contacting us. We believe that the question you've raised have been answered. If you still feel a need to continue the discussion, feel free to reopen it and add your comments.