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

Basic object mapper for simple model-viewmodel-model property mapping #5883

Closed
danroth27 opened this issue Mar 3, 2017 · 139 comments
Closed

Comments

@danroth27
Copy link
Member

danroth27 commented Mar 3, 2017

We regularly advise customers to separate their input and output models (or view-models) when using MVC. This provides a typed protection from issues like mass assignment and keeps it very clear within a code-base which properties are intended for display, input, or no action at all. The existing facilities in MVC for specifying which properties to bind are somewhat clumsy or ugly (e.g. specifying property names via strings, decorating properties with [Bind], etc.).

We'd like to provide a very basic object mapper in the box to assist in dealing with accepting input models and copying their values to domain or data models, and vice-versa for producing output/view-models. It is not intended to be a fully featured object mapping system, but rather a a simple way to handle the most basic of requirements that people are exposed to when implementing these model binding patterns. To that end, it will only support recursive property assignment based on name matching. It will not include features such as flattening (CustomerName -> Customer.Name), construction, immutable object creation (via constructor params), configurable conventions, etc. Customers looking for features like this and more will be advised to use a fully-featured library such as AutoMapper. This included library thus likely act as an introduction to the concept of object mapping which customers will very often graduate from to libraries like AutoMapper.

The API will be static only with methods to perform a mapping operation, as well as create a mapping delegate that can be stored and invoked later. The implementation will use reflection to build and compile an Expression tree into a delegate. The static mapping function will cache these delegates by the type parameters so as to avoid excessive reflection and object traversal after first use.

@DamianEdwards
Copy link
Member

https://github.com/aspnet/live.asp.net/blob/dev/src/live.asp.net/Services/SimpleMapper.cs

@Eilon
Copy link
Member

Eilon commented Mar 15, 2017

@SteveSandersonMS hello.

@SteveSandersonMS
Copy link
Member

OK, where do you think this best fits? Should it be its own separate repo and NuGet package (albeit a very tiny one), or can we make a case for fitting it into an existing repo/package?

@danroth27
Copy link
Member Author

I vote for a separate repo.

@Eilon
Copy link
Member

Eilon commented Mar 17, 2017

I at least think it definitely doesn't belong in the MVC repo. Perhaps something like Common, or its own repo.

@bjorncoltof
Copy link

Why not leverage automapper for this?

@danroth27
Copy link
Member Author

@bjorncoltof The thinking is that we could provide something really simple, at the expense of flexibility and power. We think we can do something simpler because our scenario is so constrained.

@bjorncoltof
Copy link

I understand that your scenario is relatively constrained, but it is also so easy to implement using e.g. Automapper. If you then 'hide' it behind an interface people can change it to whatever they like. It would not surpride me if the constrained svenario will grow bigger over time and you'll end reproducing more behaviour that is already covered by readily available packages

@dguerin
Copy link

dguerin commented Mar 17, 2017

@bjorncoltof I was thinking the exact same thing. At the very least if it's behind a (supporting) interface, one could simply replace this intended implementation with AutoMapper instead.

@SteveSandersonMS
Copy link
Member

@Eilon Design discussion aside, if this is going in a separate repo, can you create one I can push code to? I guess it should be called ObjectMapping. The APIs will go into the Microsoft.Extensions.ObjectMapping namespace.

@andrewlock
Copy link

I completely agree with @bjorncoltof here - why not just encourage people to use AutoMapper instead of creating your own 'MS sactioned' version?

I get you want to encourage best practice without the initial boilerplate of Automapper, but it feels like if you create a static interface package then you're going against the DI everywhere approach being pushed in ASP.NET Core.

Alternatively, if you put it behind an interface, then you'll end up stuck with the same "lowest common denominator" conflict that riled up the DI container authors previously...

@alexvaluyskiy
Copy link

Not invented here... again

@flq
Copy link

flq commented Mar 18, 2017

Really, that is exactly the kind of stuff why .NET community does not get the traction it should have. There are numerous mapping libraries out there...

I can only speak for AutoMapper which is easy to set up and ramps up with features if you need more power. Now you stick this into your framework, another chance gone to commit to your community and say

"Yes, mapping domain into viewmodels absolutely makes sense in many situations, but we don't need to cover this, the community has it covered quite nicely thank you very much."

@phillip-haydon
Copy link

Doesn't Microsoft have enough to do already instead of wasting it's time attempting to kill off more community projects?

@abatishchev
Copy link

https://github.com/AutoMapper/AutoMapper

@LordZoltan
Copy link

Gotta say I'm not really sure I'm seeing the point either. If encouraging people to use proper View Model-Domain Model separation is the goal, then stick it in the docs, explain why you should do it, show the handwritten method and then show how to do it in a 'proper' mapping lib.

Providing a rudimentary mapper doesn't encourage people to do it; it subverts the usage of already-established mapper libs, chiefly AutoMapper obviously, encouraging people to drift away from what the community already uses.

@dotnetchris
Copy link

dotnetchris commented Mar 18, 2017

While i think there's till plenty of room for Microsoft to get into the mapping game. I'm pretty sure the biggest problem associated with mapping is the lack of language support for intelligent, simple mappings.

The way anonymous objects and object initializers behave, that right there is 85% of the syntax for a gloriously simple mapping system at the language level. Sadly i don't think this can be achieved without language level support, or atleast those projects would all become exercises in insanity expression tree parsing and handling that likely lead to a never ending series of limitations and constraints.

@skleanthous
Copy link

skleanthous commented Mar 18, 2017

I just want to add my voice to the people that say that this is hurting the community. Other languages are thriving just due to community alone and you (Microsoft) should be trying your hardest to nurture the community and not to provide lightweight alternatives.

Since an open source project is really good, what you should be doing is supporting it by recommending it and potentially contributing to it.

@DamianEdwards
Copy link
Member

Thanks for the comments and discussion folks! We're certainly open to other ideas here, nothing is set in stone.

As you can imagine there are many considerations when determining whether we recommend, ship, support, and/or contribute to an existing library when looking to fill a gap in our stack. This case is no different and we're equally sensitive to the issue of being perceived to favor one OSS library over another in cases where there is established choice. Ultimately we're trying to do right by all our customers along with the .NET OSS community (that we're also a part of).

Our intent here was to provide the most basic of utility methods to allow customers to perform simple object mapping in the cases outlined in the original issue. We felt this would encourage better (and safer) code while introducing many customers to the concept of mapping. As stated, we intended to steer customers to libraries like AutoMapper for any needs beyond the most basic. Creating an abstraction, e.g. IObjectMapper, was expressly not a goal given the simplicity of the utility and that it was intended as an application helper rather than a sub-system that the framework itself would utilize (and thus warrant suitable abstraction as to allow replacement).

That said, we're going to revisit the idea of either simply recommending (e.g. via documentation) mapping libraries, or going further and shipping something like AutoMapper in our default application templates (with applicable usages within). We'll keep this issue up to date on our thoughts and discussions as we go.

@mythz
Copy link

mythz commented Mar 18, 2017

To add to the list of existing .NET Auto Mappers ServiceStack.Text also includes simple, config-free and resilient Auto Mapping via extension methods with a Live Demo you can play around with on Gistlyn.

@ejsmith
Copy link

ejsmith commented Mar 18, 2017

I think you guys have done an awesome job of going open and putting most of the process out in the open as well, but it seems like there is still a big tendency to have Not Invented Here syndrome. I'm trying to think of an instance where you guys adopted an existing open source project and contributed to it instead of making your own. Nothing is coming to mind besides maybe JSON.net.

@wwwlicious
Copy link

wwwlicious commented Mar 18, 2017

agree with @dotnetchris that a better path for MS to take would be to improve lang support for object mapping.

Whether this mapping is intended to be a simple/basic/easy/default implementation or not, it could easily be documented or demoed using existing libraries. There really isn't a need for this or an abstraction.

@synhershko
Copy link

synhershko commented Mar 18, 2017

I second the notion of using an existing, battle-tested mapper from the community as opposed to pulling another NIH and writing something "simple" on your own. AutoMapper as an example isn't too bloated to avoiding using it for simple use-cases. If you want to provide simple mapping capabilities OOTB to "educate users", then examples using community project and showing how to use them will achieve both that and flourishing the community and OSS vibe you say you want to preserve.

We, as a community, don't even need you to contribute to those projects. Just point at them, give them the spotlight, even ask them to provide the documentation to the official channels. They will most likely oblige. Don't act as a monarch; act as an equal member in the community, and sometimes let it truly lead you.

This is how it's done pretty much everywhere else today, and writing something on your own YEARS after it's been written by the community and perfected by it would be a terrible waste.

My 2 cents.

A caring .NET MVP and a long time contributor to OSS in the MS stack and elsewhere.

@hhariri
Copy link

hhariri commented Mar 18, 2017

Let's say for the sake of argument that you include this functionality. Unless you're locking down the feature set from day one (which I'd find unlikely), you're going to slowly add features, because those customers that relied on your simple solution out of the box, need just "one more feature" to make it work for them. So at some point, as your customer's applications starts to grow, and so do their needs, you'll end up having to make a decision - do you continue to add features to serve your customers or do you tell them to dump your solution and go with something like AutoMapper. Even if the change of code is minimal, telling customers that you no longer support what they need won't be easy. And we all know that the reality is that most applications start simple, but they don't end that way.

The alternative to this isn't necessarily to ship a library or favour a specific one, but to actually educate users and guide them to options, much like other frameworks do on other platforms.

@wwwlicious
Copy link

OT here but one cli to rule them all... did I miss the memo where the dotnet cli was replacing bash?

The extensibility model is all well and good but I don't understand the rationale for dotnet test, build, pack, new etc, it replicates/replaces discreet binaries that already do these things and in the process seems to cause a lot of additional effort. Effort that could be spend on enriching the .net core x-plat support.

It goes against the *nix philosophy of piping which IMO has stood the test of time. Will dotnet cli be around in 30 years time or will it drown in a thousand verbs under it's own ambition?

I think the x-plat vision for .net core is tremendous, typescript is lovely and rosyln, the most interesting thing to happen to compilers in decades; but there are so many choices being made for core like this one which are not progressing that effort and instead re-inventing yeoman, nuget, msbuild, test runners, ioc containers and now object mapping. It seems as confused as the version schemes and a sincere but misguided effort to please everyone but will end up pleasing no one.

MS I don't think this is working... It's not you ... it me! ;-)

@shanselman
Copy link

shanselman commented Mar 24, 2017 via email

@frankabbruzzese
Copy link

Discussion shifted into another subject: templates. As a matter of fact building templates for visual studio projects is as easy as creating a project containing whatever you want to put in the tamplate, and then replacing some "fixed names" with "placeholder names". So if there is an issue with the current default template we may simply promote a Github repos where people may contribute templates specialized for various purpose: newbies, starting project, starting project three layers, etc....

@frankabbruzzese
Copy link

frankabbruzzese commented Mar 24, 2017

About the main subject.
Thinking more deeply on the subject, what I don't like of this "path" is the "focus on object mapping", instead of separation of layers. Here the focus should be: "repository pattern," and "avoiding references to business objects in the Mvc layer", stop.

Object mapping + ViewModels (and/or DTOs) is just a way to reach the goal. There are several others such as, for instance:

  • Using and model bindining interfaces insteaad of classes. Then one might put the same business object behind several interfaces, thus avoiding object copying.

  • The interface between UI and business layer is a Json interface. Good paradigm that works better in distributed environments and in the cloud(microservices for instance). In this case both layers may communicate through interfaces that are then serialized/deserialized in jSon. Each layer choose different implementation of the same interfaces and Json Serializer/Deserializer substitutes object mappers.

And also when using object mapping a more repository centric approach might be taken instead of using general purpose mappers.

So why leaving people with the idea they MUST use object mappers ?

Here a thread about usage of interfaces instead of object mappers.

I am not saying that object mappers are not useful, but just they are one of several choices. Which is the more adequate depends on the overall system architecture.

@shaynevanasperen
Copy link

@frankabbruzzese your link is pointing to Gmail

@phillip-haydon
Copy link

@frankabbruzzese no. The repository pattern is already misused, and a misunderstood design pattern.

MS's documentation shouldn't be: 'this is how things are done'

MS's documentation is to show people how to use MVC. It's documentation can show some examples of using object mapping, repositories, etc etc, with warning flags above it. But it simply cannot be an authority on how development is done.

@frankabbruzzese
Copy link

frankabbruzzese commented Mar 24, 2017

@shaynevanasperen , I updated the wrong link in my previous post, now it works. sorry :).

@phillip-haydon ,

"MS's documentation is to show people how to use MVC. It's documentation can show some examples of using object mapping, repositories, etc etc, with warning flags above it. But it simply cannot be an authority on how development is done."

I agrre with you, but unluckly a lot of people interpret "examples" like "MUST BE THIS WAY", so the point is what examples must be about, and mainly how default template ...or better templates (I would like to have several template to choose) should look like.

@frankabbruzzese
Copy link

frankabbruzzese commented Mar 24, 2017

Remainig with feet on earth, if you read the initial @danroth27 post the main reason they want to introduce object mapping in their templates is they don't want to expose unused properties of DB objects in the views, and obviously to the model binder, since probably they know a lot of people would leave the code as it is (which might cause vulnerabilities and other problems). So no intention to impose patterns, but just a practical reason.
This problem, is more easily solved with the interfaces approach I mentioned in my previous post: the business object might be exposed through interfaces containing just subsets of the object properties.

All that is needed is a binder for interfaces. Better investing on this, that is useful in its own that on object mapping! There is also some validation problem to be solved, but nothing so terrible!

@danroth27
Copy link
Member Author

Thanks everyone for the feedback on this issue! At this time we've decided to not tackle this feature, but instead point folks to the many existing .NET community solutions.

@danroth27 danroth27 removed this from the 2.0.0-preview1 milestone Apr 10, 2017
@jbogard
Copy link

jbogard commented Apr 11, 2017

@danroth27 Btw, my offer still stands - if there is something missing (features or usability or barrier for entry) from AutoMapper that precluded the team from including it, I'd love to address it.

@Bartmax
Copy link

Bartmax commented Apr 11, 2017

@jbogard since you ask, I'll take the chance to ask what I would like:

I would like a package that doesn't brings all the configuration complexity and full featured automapper, like only mapping type to type without any further complexity in a lightweight package of the core usage of Map, few kbs/classes/methods.

MAYBE adding a mapper via interface strategy (besides class to class) and that's it. :)

@jbogard
Copy link

jbogard commented Apr 11, 2017

@Bartmax Ah, you want a new mapper! Ostensibly with a different philosophy - all mapping libraries started as small as you say (even AutoMapper of course - though flattening was added nearly immediately) but inevitably grew from the long tail of edge cases. If the mapper was suitably constrained, you wouldn't need to test conventions.

I had thought about a "strict" vs "loose" mode of mapping that doesn't require config. But because AutoMapper supports so many scenarios, this would just get you in trouble. So I discourage everyone from using this feature as much as possible. I get enough crap about people abusing AutoMapper, I don't want exacerbate it.

As for size - I think I can make a lot of headway there. AutoMapper supports several different mapping scenarios outside of "object to object" - LINQ projections, expression mappings for OData and more. I'll take a closer look at that!

Thanks for the feedback!

@dotnetchris
Copy link

@jbogard the core premise i've always found missing on AutoMapper is on Mapster RequireDestinationMemberSource. The amount of effort required to configure mappings to pass AssertConfigurationIsValid() in AutoMapper has always just been too high for me. In almost every single mapping scenario I care absolutely nothing about the source object, I only care about the destination object being fully hydrated and want to fail fast if it can't be.

@jbogard
Copy link

jbogard commented Apr 14, 2017

@dotnetchris fyi you can decide how you want validation to happen on CreateMap - "All destination members must be mapped" (default), "All source members must be mapped" or "Neither must be mapped". I'm a little surprised - your scenario of "all destination members must be mapped" has been the default in AutoMapper since....2009? Maybe there's something I'm missing tho. Open a GH issue if you remember the details if you dont mind.

@dotnetchris
Copy link

@jbogard i believe the issue occurs that AssertConfigurationIsValid() throws an exception for source properties that are not ignored. honestly it's been several years since i've used automapper because i was never able to be happy with the behavior.

@shaynevanasperen
Copy link

shaynevanasperen commented Apr 14, 2017

@dotnetchris I think you will find what you're looking for in this post: http://stackoverflow.com/questions/954480/automapper-ignore-the-rest/31182390#31182390

.ForAllOtherMembers(x=>x.Ignore());

@jbogard
Copy link

jbogard commented Apr 15, 2017

@dotnetchris AutoMapper has the opposite behavior, for destination properties, not source. @shaynevanasperen that was a hack - you've got an enum now that lets you decide which members you want to validate - source/destination/none.

@frankabbruzzese
Copy link

frankabbruzzese commented Apr 16, 2017

@Bartmax ,
While what you are asking for might appear reasonable, a detailed analisys shows that it is impossible.

First of all, a simple type mapping that is just name based is totally uneuseful, since instead of copying the object one might simply hidde it behind various interfaces.
Second, if you use a DB you need also a projection operator, otherwise you might extract from DB more columns that you need. Also in this case you might avoid using DTO by specifying the properties you need with an interface (only the properties of the interface are retrieved.)

Actually, what makes copying necessary are "exceptions", that is, properties of the target that somehow have no direct correspondence with properties of the source object. So any mapping library that makes sense should handle somehow these exceptions.
In order to avoid configuration one might pass an expression to both mapping and projection that speciies just how to handle exceptions:

...Map<T, T1>(m => new Target{prop1=....});
...Proj<T, T1>(m => new Target{prop1=....});

Where one specifies just exceptions, while all other assignements are inferred automatically.

Unluckl,y a similar solution would be very slow, since in both Proj, and Map, the expression needs a complex pre-processing before being used to perform the copy, and in the case of the Map it should be also compiled. Thus we need to cache both processed expression sand compiled code for Map. However, caching with a T, T1 key is possible only if the expression passed is always the same for each T, T1 pair. Thus caching may be achieved only by moving the expression argument out from Map and Proj into a configuration instruction that defines the way to process the T, T1 pait once and for all.

Therefore, both exceptions handling, and configuration can't be avoided!

@frankabbruzzese
Copy link

In my opinion the best solution would be enriching languages with object copy instructions where you specify just the "exceptions" (as in the expression arguments in the code snippets before) and all other assignements are inferred automatically. In this case no cache would be necessary since assignement instructions are created at compile time instead of run-time. This would solve efficiently all problems while keeping maximim flexibility.

@abatishchev
Copy link

Why to put anything in the language what can be (and successfully​ years ago) in a library? Let's don't exercise this direction.

@frankabbruzzese
Copy link

frankabbruzzese commented Apr 16, 2017

@abatishchev , simple, because all mapping libraries adds not needed complexity trying to mimic at run-time the compiler behavior by manipulating and compiling expressions. With compile time support there would be no need for any configuration it would be enough to write something like
`
...Map<T, T1>(m => new Target{prop1=....})

`where just "exceptions" are explicitely listed in the assignements, while all same name convention properties are matched automatically. On the contrary run-time libraries need configuration to factor out once and for all time spent manipulating and compiling expressions. Moreover, with compile time support there is no need to associate a mapping to the pair T, T1, but each mapping instruction may have a different mapping rule, since the work needed to manipulate and compile expressions is spent at compile time not at run-time.

Anyway, this discussion is just a theoric excercise. For sure C# will not be modified because of this discussion :). Maybe, this will happen in the future when more compelling needs will claim a similar extension.

@timematcher
Copy link

timematcher commented Jul 23, 2017

I would want to use a different mapper for different clients based on clients requirements. Some hate Automapper because of size and performance (both debatable). I personally would like a very basic mapper (that was already there in .NET core 1.x perhaps, ITranslator???) with a possibility to swap it with any other mapper of choice for a client (yes most of my clients are indeed technical & choosy) without the headache of breaking existing code.

This also helps new programmers and for personal projects which only have a very basic mapping need.

Is it too much to ask from MS?

@phillip-haydon
Copy link

Yes. It's not needed.

@timematcher
Copy link

Should have kept it closed source. It would have been a lot better :P

cheers

@phillip-haydon
Copy link

We should something not needed be added? Why do you need the kitchen sink added? Shouldn't MS focus on making everything else awesome instead of adding useless features?

@phillip-haydon
Copy link

Can this thread be locked?

@Eilon
Copy link
Member

Eilon commented Jul 24, 2017

@phillip-haydon - it's been quiet for 3 months, not too bad for such a "hot" thread 😄 If this thread causes too much noise for people we will consider locking it. BTW anyone can easily "unsubscribe" from the thread's notifications by scrolling up about 5,000,000 pages from here and clicking Unsubscribe 😄 But, we do welcome the discussion - after all, it was the discussion in this very thread that helped us reevaluate what we were doing in this space, and I think most people agree things ended up in the right place.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests