Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

Custom ComplexTypeModelBinder doesn't work properly for Api Controllers or i do something wrong? #7707

Closed
iyilm4z opened this issue Apr 25, 2018 · 11 comments
Assignees
Labels
cost: XS Will take up to half a day to complete investigate question

Comments

@iyilm4z
Copy link

iyilm4z commented Apr 25, 2018

Model binder doesn't work or i do something wrong.
Here is my implementation.

public class BaseAppModel
{
    public virtual void BindModel(ModelBindingContext bindingContext)
    {
    }
}
public class TestModel : BaseAppModel
{
    public int Id { get; set; }
    public string Name { get; set; }
}
public class AppModelBinderProvider : IModelBinderProvider
{
    public IModelBinder GetBinder(ModelBinderProviderContext context)
    {
        if (context == null)
            throw new ArgumentNullException(nameof(context));

        var modelType = context.Metadata.ModelType;
        if (!typeof(BaseAppModel).IsAssignableFrom(modelType))
            return null;

        if (!context.Metadata.IsComplexType || context.Metadata.IsCollectionType) 
            return null;
        
        var propertyBinders = new Dictionary<ModelMetadata, IModelBinder>();
        foreach (var property in context.Metadata.Properties)
            propertyBinders.Add(property, context.CreateBinder(property));

        return new AppModelBinder(propertyBinders);
    }
}
public class AppModelBinder : ComplexTypeModelBinder
{
    public AppModelBinder(IDictionary<ModelMetadata, IModelBinder> propertyBinders)
        : base(propertyBinders)
    {
    }

    protected override object CreateModel(ModelBindingContext bindingContext)
    {
        if (bindingContext == null)
            throw new ArgumentNullException(nameof(bindingContext));

        //create base model
        var model = base.CreateModel(bindingContext);

        //add custom model binding
        (model as BaseAppModel)?.BindModel(bindingContext);

        return model;
    }

    protected override void SetProperty(ModelBindingContext bindingContext, string modelName,
        ModelMetadata propertyMetadata, ModelBindingResult bindingResult)
    {
        if (bindingContext == null)
            throw new ArgumentNullException(nameof(bindingContext));

        //trim property string values 
        var valueAsString = bindingResult.Model as string;
        if (bindingContext.Model is BaseAppModel && !string.IsNullOrEmpty(valueAsString))
        {
            //excluding properties with [NoTrim] attribute
            var noTrim = (propertyMetadata as DefaultModelMetadata)?.Attributes?.Attributes
                ?.OfType<NoTrimAttribute>().Any();
            if (!noTrim.HasValue || !noTrim.Value)
                bindingResult = ModelBindingResult.Success(valueAsString.Trim());
        }

        base.SetProperty(bindingContext, modelName, propertyMetadata, bindingResult);
    }
}

[Produces("application/json")]
[Route("api/[controller]")]
public class ValuesController : Controller
{
    [HttpPost]
    public void Post(TestModel model)
    {
		//ping!
    }
}

When i debug ComplexTypeModelBinder.cs of mvc with Rider, i get result below.

private async Task BindModelCoreAsync(ModelBindingContext bindingContext){
//... 
   nestedScope = new ModelBindingContext.NestedScope();
    if (result.IsModelSet)// false
      complexTypeModelBinder.SetProperty(bindingContext, modelName, property, result);// that's why my binder's SetProperty doesn't fire
    else if (property.IsBindingRequired) // false
      bindingContext.ModelState.TryAddModelError(modelName, property.ModelBindingMessageProvider.MissingBindRequiredValueAccessor(fieldName));
    property = (ModelMetadata) null;
//..
}

i choose body/raw(JSON(application/json)) and it doesn't work.

fail

Surprisingly it works with body/form-data.

success

What's wrong with model binding feature of MVC? Does it behaves different for Web Api? What i'm missing?

@iyilm4z iyilm4z changed the title Model binder doesn't work or i do something wrong. Model binder doesn't work or i do something wrong? Apr 25, 2018
@iyilm4z iyilm4z changed the title Model binder doesn't work or i do something wrong? Custom ComplexTypeModelBinder doesn't work for Api Controllers or i do something wrong? Apr 25, 2018
@iyilm4z iyilm4z changed the title Custom ComplexTypeModelBinder doesn't work for Api Controllers or i do something wrong? Custom ComplexTypeModelBinder doesn't work properly for Api Controllers or i do something wrong? Apr 25, 2018
@iyilm4z iyilm4z closed this as completed Apr 25, 2018
@iyilm4z iyilm4z reopened this Apr 25, 2018
@kichalla
Copy link
Member

If you are trying to bind data coming in as application/json, then you need to decorate your parameter with [FromBody] attribute. This causes the input formatters to deserialize your request body and construct the parameter instance.

@iyilm4z
Copy link
Author

iyilm4z commented Apr 25, 2018

I did it but i had the same result.

mvcBuilder.AddMvcOptions(options => options.ModelBinderProviders.Insert(0, new AppModelBinderProvider()));

I also registered my custom model binder as my first. Maybe this causes issue? If so, why it works properly with form-data?

@Tarig0
Copy link

Tarig0 commented Apr 25, 2018

Have you debugged your provider and see which branch/line is failing?

@kichalla
Copy link
Member

I did it but i had the same result.

Your scenario should just work with any custom model binder as its the input formatters which do the binding here. So, remove any modifications to options related to modelbinderproviders.

@mkArtakMSFT
Copy link
Member

Thanks for looking into this, @kichalla. Assigning this to you to complete the investigation.

@iyilm4z
Copy link
Author

iyilm4z commented Apr 25, 2018

@kichalla But unfortunately it doesn't. @Tarig0 I made a demo. You can check it out by yourself.

@kichalla
Copy link
Member

@iyilm4z I still see you are using this binder in options https://github.com/iyilm4z/Demo.ModelBinding/blob/master/Demo.ModelBinding.Api/Startup.cs#L23

Remove it and things should start working

@iyilm4z
Copy link
Author

iyilm4z commented Apr 25, 2018

@kichalla I know that default binder is already working. If i remove that line how MVC ecosystem will understand my custom ComplexTypeModelBinder for BaseAppModel? As you see, there in BaseAppModel i have some logic for my all VMs/DTOs. I really need it for the infrastructure of my architecture.

@iyilm4z
Copy link
Author

iyilm4z commented Apr 26, 2018

Your scenario should just work with any custom model binder as its the input formatters which do the binding here.

@kichalla Do you mean i should implement my own input formatter? Please provide me a solution with some code. Similar issue #4553

@kichalla
Copy link
Member

I realized there is a typo in my earlier comment, sorry about that. What I really meant was that you should not write a custom model binder in your scenario as its the input formatters which deserialize the request body and construct the model. In your case, since your are sending data as json, the JsonInputFormatter is used for deserializing the request body.

Now coming to the inheritance of the models, even though JsonInputFormatter's underlying JSON.NET supports this scenario by using TypeNameHandling setting, in general it's not recommended to use it due to security concerns. Here are some articles describing more about it:
https://stackoverflow.com/questions/39565954/typenamehandling-caution-in-newtonsoft-json
https://www.alphabot.com/security/blog/2017/net/How-to-configure-Json.NET-to-create-a-vulnerable-web-API.html

This is the reason MVC by default always sets the setting to None:
https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNetCore.Mvc.Formatters.Json/JsonSerializerSettingsProvider.cs#L39-L41

@kichalla kichalla added question cost: XS Will take up to half a day to complete 2 - Working labels Apr 26, 2018
@kichalla
Copy link
Member

Closing this issue as there's no action item on our side.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cost: XS Will take up to half a day to complete investigate question
Projects
None yet
Development

No branches or pull requests

4 participants