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

Model bind to Interface via DI #6014

Closed
benaadams opened this issue Mar 22, 2017 · 21 comments
Closed

Model bind to Interface via DI #6014

benaadams opened this issue Mar 22, 2017 · 21 comments

Comments

@benaadams
Copy link
Contributor

benaadams commented Mar 22, 2017

Requirement

I'd like to bind to a model interface via DI but it doesn't seem to work? Am I doing something wrong?

Steps to reproduce

Models

public interface IMyModel
{
    string Name { get; set; }
    int Count { get; set; }
}

public class MyModel : IMyModel
{
    public string Name { get; set; }
    public int Count { get; set; }
}

ConfigureServices

public void ConfigureServices(IServiceCollection services)
{
    services.AddTransient<IMyModel>((provider) => new MyModel());
    services.AddMvc();
}

Controller Method

public IActionResult Contact(IMyModel model)
{
    ViewData["Message"] = $"{model.Name} {model.Count}";

    return View();
}

Expected behavior

Transient object obtained from DI; then its properties set as per non DI type

Actual behavior

fail: Microsoft.AspNetCore.Diagnostics.ExceptionHandlerMiddleware[0]
      An unhandled exception has occurred: Could not create an instance of type 'mvc.IMyModel'. 
Model bound complex types must not be abstract or value types and must have a parameterless constructor.
System.InvalidOperationException: Could not create an instance of type 'mvc.IMyModel'. Model bound complex types must not be abstract or value types and must have a parameterless constructor.
   at Microsoft.AspNetCore.Mvc.ModelBinding.Binders.ComplexTypeModelBinder.CreateModel(ModelBindingContext bindingContext)
   at Microsoft.AspNetCore.Mvc.ModelBinding.Binders.ComplexTypeModelBinder.<BindModelCoreAsync>d__4.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.AspNetCore.Mvc.Internal.DefaultControllerArgumentBinder.<BindModelAsync>d__8.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.AspNetCore.Mvc.Internal.DefaultControllerArgumentBinder.<BindArgumentsCoreAsync>d__6.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.AspNetCore.Mvc.Internal.ControllerActionInvoker.<InvokeNextResourceFilter>d__22.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at Microsoft.AspNetCore.Mvc.Internal.ControllerActionInvoker.Rethrow(ResourceExecutedContext context)
   at Microsoft.AspNetCore.Mvc.Internal.ControllerActionInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Internal.ControllerActionInvoker.<InvokeAsync>d__20.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.AspNetCore.Builder.RouterMiddleware.<Invoke>d__4.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.AspNetCore.Diagnostics.ExceptionHandlerMiddleware.<Invoke>d__6.MoveNext()

/cc @DamianEdwards @rynowak

@pranavkm
Copy link
Contributor

public IActionResult Contact([FromServices] IMyModel model)

We don't look in DI by default (similar to how formatting doesn't kick in until you say FromBody.

@benaadams
Copy link
Contributor Author

@pranavkm Perfect thank you!

@rynowak
Copy link
Member

rynowak commented Mar 22, 2017

then its properties set as per non DI type

[FromServices] does not do this.

@rynowak
Copy link
Member

rynowak commented Mar 22, 2017

@benaadams - here's a sample that someone else wrote https://github.com/Serjster/IOCModelBinderExample/tree/ASPNETRTMVersion

@benaadams
Copy link
Contributor Author

[FromServices] does not do this.

You give with one hand then take away with the other 😢

@rynowak
Copy link
Member

rynowak commented Mar 22, 2017

I gave you a sample?

And yeah [FromServices] is a missed opportunity

@benaadams
Copy link
Contributor Author

[FromServicesAndBody]? 😉

@rynowak
Copy link
Member

rynowak commented Mar 22, 2017

What I really mean is that it would have been dope to be able to create a model using deserialization (like [FromBody]) - and enrich that with model binding some other sources as an opt-in model. It would have been cool if we decided that this was how things should work by default in 1.0.0 but I think that 🚢 has ⛵️ 'ed

@benaadams
Copy link
Contributor Author

Wouldn't need an object mapper for simple cases though if could just use the language feature of interfaces to do it

@frankabbruzzese
Copy link

I wrote a post about how to model bind interfaces in the way you need:

http://www.dotnet-programming.com/post/2017/03/17/Custom-Model-Binding-in-Aspnet-Core-2-Model-Binding-Interfaces.aspx

There you will find a custom binder for interfaces, and how to make validation works properly when using interfaces.

@frankabbruzzese
Copy link

frankabbruzzese commented Mar 23, 2017

@benaadams , anyway, I dont know if interfaces are enough to avoid object mapping at all:

  • You need a IQueryable projection operator, in any case, otherwise you are forced to retrive from DB also data you don't need, in case you need just a subset of the data object properties.

  • While client side validation is done according to the attributes you place on the interface, server side validation is done according to the actual type that is behind the interface. This means that if you want everything works properly you must add the interface as a MetaDataType of the class that implements it. However, in turn, this means you may use just one interface implemented by that type. Which in turn means you can't use a different interface in case you want to modify just some of the object properties in the user interface.

I dont know if the validation behavior is correct or a kinda bug, but for sure there are classes where you need the actual runtime type be validated instead of the declared type (model binding subclasses for instance).

@benaadams
Copy link
Contributor Author

benaadams commented Mar 23, 2017

Scenario: I have one microservice posting to another microservice; they both have their own data models - however they share an interface/contract for data exchange; but don't share models.

So easy approach is to serialise and post the through interface; then deserialize the posted interface (and populate the rest of the local model as needed)

@frankabbruzzese
Copy link

frankabbruzzese commented Mar 24, 2017

@benaadams , if you dont use IQueryables forget my first point.

Probably, also the validation problem is not an issue for you. Anyway, I explain better the issue of validation.

Suppose, You have two interfaces, say IShort and IComplete that inherits from IShort. Suppose, also some transaction uses IShort, since you need to communicate just a few properties of IComplete.

The issue is that you can't use a single class to resolve both interfaces with DI, because of the way validation works.
In fact to have server side validation works properly you should write something like

[ModelMetadataType(typeof(IComplete ))]
    public class CompleteClass : IComplete 
    {
        ....
        ....
    }

Obviously, since "CompleteClass" implements also IShort you might use it also for resolving IShort. However, if some of the properties of IComplete that are not contained in IShort contain [Required], then
model binding of IShort would always fail, because of this required attribute.

Anyway, Since one of my customer is very interested in this approach, I hope to be able to solve this problem, ie the need to use ModelMetadataType by adding some smart validation logics in the custom model binder.

If I'll come up with a solution I will post it here.

@frankabbruzzese
Copy link

Update! The problem with server side validation takes place ONLY WHEN the interface is the root model where model binding starts. The problem being caused by the way the interface method IObjectModelValidator.Validate is defined. It doesn't accept a parameter containing the declared type of the model, so its default implementation computes this type with model.GetType(), which returns the type behind the interface instead of the interface (or the runtime subclass of a class).

I can write a custom implementation of IObjectModelValidator that might solve the problem by comparing the model type, and name against the list of action parameters one may get from the ActionContext. However, this is not optimal.

The optimal solution would be adding a new param to IObjectModelValidator.Validate (or defining a new interface or interface method) and then updating both the default implementation and the ParameterBinder code.

@rynowak
Copy link
Member

rynowak commented Mar 24, 2017

Our system is a model validation system, not an input validation system. This means that we ensure that the model object is in a valid state, which may include properties that were not set from the model binding system. http://bradwilson.typepad.com/blog/2010/01/input-validation-vs-model-validation-in-aspnet-mvc.html

It sounds like what you're asking for is fundamentally an input validation system, which we unfortunately don't have today.

@frankabbruzzese
Copy link

@rynowak , it is not about model versus input validation. It is more subtle, it is about what to do when an action method parameter or ViewModel property that as been declared as "Animal" is bound with a subclass, say "Dog". Should it be validated as "Animal" or as "Dog"?
Both approaches are somehow correct, and may be considered "model validation" if they validate the whole objects. There are vantages and disadvantages for both of them, so for me any choice among them is "acceptable".

However, in the current implementation it happens that if "Animal" is the root model it is validated as a "Dog" , while if it is nested in the property of another class it is validated as an "Animal". So the behavior of the validation system is incoherent, and I consider it a bug.

Probably, it was implicitly assumed that the run time type is always equal to the declared type, which is true if only the built-in binders are used, but may be false in case custom binders are used. So different ways to get type metadata have been mixed.

Validation system should be moved toward a coherent behavior (any of the two possibilties is acceptable for me), but it is not acceptable that the way an objecy is validated depends on the way it is nested in the whole object tree.

@rynowak
Copy link
Member

rynowak commented Mar 27, 2017

However, in the current implementation it happens that if "Animal" is the root model it is validated as a "Dog" , while if it is nested in the property of another class it is validated as an "Animal". So the behavior of the validation system is incoherent, and I consider it a bug.

If this is true then yes this is a bug. It just doesn't surface through any default path, but we should address this.

@frankabbruzzese
Copy link

@rynowak , if this can help I have two projects with a custom model binder for interfaces. One where the interface is the root model and the other where it is nested. They show the different behaviors. If you need them I may upload them either to some cloud drive or to Github.

Anyway,the simplest fix is the one outlined in my previous post about IObjectModelValidator.Validate. It follows the "Animal" path. This fix should be quite easy.

Taking the other path should be more difficult instead, because it would involve a substantial rewrite of the validation visitors, and of the metadata structures they rely on.

About the optimal solution, instead,, thinking more about the subject, I concluded that it should do the following ("pro arguments" are attached to each point):

  1. If run time type and declared type are the same (which should occur most of the times) choose the more efficient path (since the result is the same).
  2. If run time type and declared type are different and declared type is an interface, let choose the "animal" path ie let use interface metadata, since the "dog" path would break the interface (when I use an interface I don't want its behavior depends on the way it is implemented)
  3. If run time type and declared type are different and declared type is a class let follow the "dog" path, ie let use run time type matadata, in fact choosing the "animal" path would break polymorphism (when I use a subclass of a class I want its "more specific" behavior ).

The above behavior would allow both usage of interfaces in Views and custom model binders able to recognize subclasses (from the validation point of view...binder code is another stuff).

However, I understand that it is quite difficult to move toward this very dfferent behavior, so probably the best compromise is an overall "animal" path.

@frankabbruzzese
Copy link

@benaadams ,
I implemented custom logics for validating interfaces. As promised here the link: http://www.dotnet-programming.com/post/2017/04/06/Model-binding-interfaces-fixing-server-side-validation.aspx

@rynowak
Copy link
Member

rynowak commented Jul 17, 2017

Reading through this thread again, I think there's value in the design idea of considering DI/Formatters/new as different kinds of factories for input objects.

@rynowak
Copy link
Member

rynowak commented Jul 17, 2017

I'm closing this issue because I think we've answered the concerns here, and we have no immediate plans to invest in this area.

@rynowak rynowak closed this as completed Jul 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants