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.61k stars 2.14k forks source link

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

Closed danroth27 closed 7 years ago

danroth27 commented 7 years ago

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.

RehanSaeed commented 7 years ago

@nbarnwell I'm not raining on anybodies Automapper parade. Even the use of Automapper requires an interface, in fact Automapper has one built in itself called IMapper. Tying everybody into Automapper as opposed to the other mappers is not a good thing. All I'm saying is that if there has to be a common interface that third party tools like Automapper and others implement, then consider one with generic arguments in the interface declaration.

phillip-haydon commented 7 years ago

That's a bit unfair. Maybe I'm naive, but surely that's not their goal. Yes, there are people who use MS because it's MS and this can be the unfortunate side-effect. Typically where MS have obviously and actively taken that approach, it's because they want to make money. In this case, there's no money to be made, so there's no motive to "kill off" AutoMapper et al.

It's totally fair. MS needs to spend more time on ACTUAL problems, and focusing on getting people onto Azure, regardless of platform they use. Adding mapping is only wasting their time. They have so much more stuff in .net core to worry about, if they have so many free resources in the asp.net team then get them working on missing functionality of dotnet core.

nbarnwell commented 7 years ago

Better to leave the generic arguments out of this implementation, which will therefore allow people to have a little go, but when they feel the pain of casting etc, they'll look to the docs, which will say "Learn about Object Mapping and use a dedicated library for it.". I wouldn't even have an interface - make it a static method on a static class. There will be no choice to extend it, one will HAVE to go looking elsewhere.

davidroth commented 7 years ago

@RehanSaeed I don't think it makes sense to introduce a new abstraction over mapping. These "common interfaces" tend to work not well because they only define the lowest common denominator. If someone wants to hide the mapper of choice, one should create it`s own application interface of choice - this can be done within minutes.

nbarnwell commented 7 years ago

@phillip-haydon How MS should spend their resources is a fair point, except the impl being suggested, if they go ahead at all, could've been done in less time than has been spent debating it on this thread.

The reason not to do it is because they could be doing something else, and everyone is panicking that it will somehow kill off AutoMapper et al. These are valid concerns, and I agree entirely.

The reason to go for it, is that it will hopefully guide a certain kind of developer away from using their EF models as viewmodels yet avoid friction that would come from having to go off on a tangent to "get started" with AutoMapper.

The only way it will succeed though, is if it removes that friction but is still SIMPLE ENOUGH TO NOT COMPETE with established libraries, and in fact embraces that and points users to those libraries. No-one believes MS is capable of this, that some innate greed will force them to twhirl their moustaches and crush all other object mapping libraries in their wake.

There is a precedent for that, so it's up to MS to avoid that trap, and turn things around so the community can begin to trust them again.

It's like teaching someone to fish rather than giving them a fish, but just letting them have a little taste first to motivate them.

phillip-haydon commented 7 years ago

@nbarnwell - the whole reason people use EF as view models is because Microsoft taught them.

OMG Every single year MS does demo's "hey checkout web api, we will just add EF, query the database, return the data, bam done" then a flood of questions on Stack Overflow popup "it throws an exception, it times out" all caused by returning EF models and allowing lazy loading to occur.

Microsoft screwed up, fixing the problem of bad designs doesn't mean "lets introduce another new concept", it's going to be yet another pattern which is abused. View Model reuse it abused. Repositories are abused. EF is abused. The entire MS ecosystem is abused every day.

But let's debate adding another abstract baked right into mvc so that all the MS shills can be jump for joy.

I'm against MS trying to get their fingers into everything, every time they do this they kill off more of the community.

dylanbeattie commented 7 years ago

Adding my voice to the conversation because, like many people above, I think this sort of behaviour is at odds with Microsoft's stated commitment to the .NET open source community. Don't reinvent the wheel - take what already exists and invest in making it better. Please.

Microsoft has proved they can deliver the kind of "out-of-the-box" experience that developers are asking for without having to reinvent every component in the stack. File > New > ASP.NET Web Application installs jQuery, Modernizr, Bootstrap, Newtonsoft.Json - even Antlr3.Runtime. Newing up a Single Page Application gives you Knockout, Sammy and Respond.js as well. It works, it works well, and it's probably the single most effective way of getting 'dark matter developers' using these libraries - make it a seamless part of the native tooling experience. When those developers' requirements become more complex, they don't have to try and swap out SimpleComponent for a more fully-featured open source alternative. It's a pattern that I think works really well - so why can't we do the same with AutoMapper? Instead of creating a new 'simple' mapper, take an existing, proven solution with a rich community around it, and use that. If there's some limitation of AutoMapper which means you can't deliver a great out-of-the-box experience, then open a PR for it.

If the rationale behind this request is enterprise customers insisting that every line of code in their stack is covered by Microsoft support agreements, then start offering Microsoft support for AutoMapper.

Another object mapper isn't going to do anything for .NET OSS - but a show of support for the existing community, like embracing AutoMapper as the de facto mapping solution for new VS projects, could be really valuable.

nbarnwell commented 7 years ago

I think perhaps a general dislike of MS and past mistakes is confusing the issue somewhat. Fine, MS taught people how easy it is to query an EF model and cause lazy-loading etc. But that's fixable by calling ".ToList()" on the result, whether you return the resulting objects or map them to something else.

The question here is, do we just write off everything MS does because they are MS, or do we look at ways to avoid those past mistakes, learn from them, and offer better guidance in the future? How do we turn around this perceived problem where "The entire MS ecosystem is abused every day."? Is it always MS's fault, or is it partly the fault of the people doing the abusing? Who is better placed to help those people change, than the only people they look to for solutions?

Yes, what I'm saying all hinges on MS not repeating those past mistakes. I know that feels like a long-shot, but we can't just let our emotions run our decisions all the time, else we risk cutting our nose off to spite our face.

And for the record, I don't recommend adding another abstract baked into mvc. I'm hoping they do the opposite - a locked-down, bare-minimum thing (it could even throw exceptions when run outside of a debugger, telling you to use something else once you're done spiking) to get amateurs/newbies who've never heard of "object mapping" over that hump without distracting from mvc itself.

On the whole, less code is better. I'm fine with them not going ahead with this, though there is that one small possible benefit of it. But if they are determined, then at least they should make sure it does not "kill off" other community efforts, but instead PROMOTES them to those who perhaps currently only speak Microsoft.

nbarnwell commented 7 years ago

@dylanbeattie That is a fair point, and I agree in principle. The challenge, is that if MS do choose AutoMapper as the golden child, isn't that another way of them "killing off" part of the existing community who are building/using the other available frameworks? Damned if they do, damned if they don't.

They are in a position of power for those devs who blindly take them at their word, those devs who still write business logic code in their OnClick handlers because their expensive MCAD course told them to. The challenge is not just to aid the community, but to reveal it, in all it's glory and variety. To educate, rather than just give another specific "right answer".

Maybe it's a case-by-case basis, and object mapping ends up being debated where no-one (myself included) would suggest they try to offer a "simple jQuery alternative" that points people to the real jQuery when they hit the limits.

phillip-haydon commented 7 years ago

I think Microsoft should add specialized tags to NuGet. Like [object-mapping] that need to be approved so people don't abuse the tag system as they already do (there's stuff tagged for things the project doesn't even do).

Then in their documentation they can say "here's the object-mapping concept, if you want to use an object-mapper, here are the object mappers on NuGet". This means:

  1. MS doesn't need to implement all these things.
  2. MS doesn't promote any single framework over another.
  3. MS helps the community, and itself.
flq commented 7 years ago

I am currently not understanding the obsession with AutoMapper. The point is that there are mapping libraries out there and the developer should be free to choose one.

ASP.NET MVC provides plenty of hooks to insert the mapping at an adequate position. If MS fears lack of LTS of certain functionality provided by the community, it may choose a preferred library and provide the means that said library gets LTS (.NET foundation?).

In other words, on an organizational and community level there are a number of solutions to this problem that

nbarnwell commented 7 years ago

@phillip-haydon Not a bad shout. I never trust tags for that reason, so yeah, if they're moderated somehow, that'd be the perfect way to point people at the current "landscape" without having out-of-date docs.

The only challenge with your point 1 (which would be a goal, don't get me wrong) is that if there isn't an OOTB impl, and they don't promote/use any single framework over another, that you get additional friction when trying to do File-New Project-[bit more stuff]-F5. Maybe that's okay. It's hard for me to say because while I'm not god's gift to programming, I am experienced enough that it's not a problem for me (I know where to get AutoMapper and how to use it, so I'd "just" go and do that). The challenge is how to have minimal friction for those new to it. I'm split fairly evenly down the middle on this, tbh.

wwwlicious commented 7 years ago
/// <summary>
/// Did you know? : It isn't recommended to return EF entities directly so we 'map' them to a data transfer object (DTO) class
/// Want to know more? : http://msdn....
/// </summary>
public class MappingCode {
  public PersonDTO MapEntity(PersonEntity ef, PersonDTO dto) {

    // did you know? : There are libraries called 'automappers' available which can reduce this kind of code
    // want to know more? : http://msdn....

    // we'll do this by hand for now though by assigning the left / right properties we want to return
    dto.Name = ef.Name;
    dto.Age = ef.Age;
    return dto;

  }
}

is that really so complicated?

Instead of answering these for the next umpteen years of coding pattern torture

dylanbeattie commented 7 years ago

@flq AutoMapper's always been my 'de facto' mapping tool - I've used others, most notably the one that's baked into ServiceStack, but when I'm working on a project and I need to map something, AutoMapper's just the one I always go for.

There are mapping tools out there. Many developers are quite comfortable choosing one, plugging it into their project and configuring it to do what they need. This seems to me to be very much a conversation about providing a simple solution aimed at developers who - for whatever reason - do not want to choose and configure one of the many that are already available.

I'd far rather see them adopt and support an existing OSS solution than build something new, and I don't honestly care whether it's AutoMapper or something else.

iancooper commented 7 years ago

I'd recommend asking some of these projects to joing the .NET Foundation (or equivalent) and for MS employees to contribute to that. Using the .NET Foundation to promote a range of possible OSS projects makes long term sense because it insulates consumers against someone 'throwing their toys on the floor' or simply 'getting bored' and makes MS contribution easier.

It also flushes out 'alternative' OSS stacks that are funded by business models and thus cannot move to a foundation; those I think should rely on their own marketing.

I don't think everything needs to go a foundation - but where we want to recommend best practice around usage it makes a lot of sense to pick packages that have foundation backing to promote.

jbogard commented 7 years ago

@flq just an FYI - nearly all the other mapping tools copied the AutoMapper API to make it easy to switch. The others, at least that I'm aware of, are:

AgileMapper ExpressMapper Mapster ValueInjecter

I hesitated to include EmitMapper, it hasn't had a release in several years.

I'm not sure it would be possible to "kill off" these other mappers - I've collaborated with all of them - because AutoMapper's been the de facto mapper library in .NET since before any of these projects started. Each was started not to copy AutoMapper but to do something it couldn't (or I wouldn't). This wouldn't do any more than what happened with JSON.Net. There are other JSON serializers, for more specialized scenarios.

@wwwlicious

If you're interested in the motivations behind AutoMapper, I've got blog posts from 8-9 years ago that go into it. I don't apologize for it either - it solves a real, specific problem that our teams have on nearly all of our projects, and it allows them to focus on solving business problems rather than rote code (and the bikeshedding discussions that follow) that can be surprisingly buggy.

And that link you have - yes, we support mapping expressions, because for OData-type scenarios it can be super helpful. On top of that, we support building LINQ Select projections directly, so you never even load your entity, you get your mapping passed to your query provider to go straight from SQL -> DTO. Now that I assure isn't easy to write over and over and over again.

RehanSaeed commented 7 years ago

@davidroth There are already abstractions over DI, where people can choose to use the excellent Autofac. There needed to be a common IServiceCollection interface so all DI projects could implement it and the interface could be used in MVC internally.

Similarly, there could be a common IObjectMapper interface (I'm not suggesting Microsofts builds their own Automapper, I'm giving one reason for an interface) which third parties implement. This would allow MVC to make the call to do the mapping for you and you could end up with EF models in your action method instead of view models, thus removing the need for you to ever have to call the IObjectMapper.Map method. Something like this:

[Mappable(typeof(Customer))]
public class CustomerViewModel { ... }

public class Customer : DbContext { ... }

public class CustomersController : Controller
{
    [HttpPost]
    public IActionResult PostCustomer([FromMappedBody] Customer customer) { ... }
}

Just an idea, a flight of fancy if you will...

davidroth commented 7 years ago

@RehanSaeed Well this abstraction has been rightly criticizes several times so I don`t think this can be used as an argument for another common abstraction which reflects the lowest common denominator. See this thread for example (about confirming containers): https://github.com/aspnet/DependencyInjection/pull/416

davidroth commented 7 years ago

@RehanSaeed I dont think that its a good idea to build this kind of magic into the framework. It is definitely not the recommended approach to use entities in controllers and this kind of magic always tends to be harmful in the long way. So i don`t see why it would be beneficial to implement this kind of magic into mvc as it would encourage bad design.

wwwlicious commented 7 years ago

@jbogard

I think you misunderstood my intent, I use automapper, its useful, been using for yonks, read your posts years ago and apart from a breaking change circa 3,2.1, nor should you apologise, you should be thanked for your efforts! ;)

My point was that no such default, simple, basic, really won't change implementation of automapping is required in the core framework. Simply an explanation of what automapping is in the appropriate places would be enough.

The link was to demonstrate the type of questions that have been answered a thousand times over for projects like Automapper and all MS are doing is inviting a thousand more. And, as if to create a parody of itself, the requests in this thread to 'just add an interface...one more thing' .. only serve to prove the point that many here are making. Well that plus....no-one actually enjoys writing reflection-heavy expression code do they .... masochists? ;)

...and 'coding pattern torture' was about the horrible crimes all devs commit when you have shiny a new hammer, and everything looks like a nail. Thwak thwak thwak!

jbogard commented 7 years ago

Perhaps we can shift the discussion a little bit. I'm not generally in favor of a common mapping interface - switching from one mapper implementation to another is really just an exercise in Find + Replace with regular expressions.

Since @danroth27 and others were already discussing making a separate NuGet package, whatever default choice wouldn't be "baked in" anyway. This is really a discussion around 1) what the project templates should include and 2) what docs.asp.net should "recommend".

From my perspective, I'm most interested in the motivations for originally considering a new MS-built mapper library. If one of the motivations was to provide a simpler alternative, then regardless of what happens I want to incorporate that feedback to make my library simpler/easier to adopt. Half the current features of AutoMapper were inspired by competing libraries, I'm more than happy to continue that trend.

hudo commented 7 years ago

Creating "simpler" implementation by MS, even if its behind interface, doesn't make much sense to me.

As customer project grows, that simple mapping implementation is never good enough, can't scale beyond simplest use case, and 99.9% projects are growing far beyond initial planned feature set. Most devs will keep using and abusing it (like it started happening to MVC Core IOC now!).

AutoMapper and other libs are already simple enough, and as jbogard is saying, they can adopt to meet that new MVC service contract. I understand MS wants to give full experience out-of-the-box, but hurting OSS projects along the way its not a good message to the community.

martinjt commented 7 years ago

I suppose I just don't understand the "goal". Automapper has a simple interface to work with, I'm not sure how much simpler you can get.

If the goal is around "introducing people to mapping", then why introduce them to a different library, when it's likely to be the same setup as Automapper (and the numerous other frameworks).

Therefore, I'm not sure why creating a new library would help with the goal "Introducing people to mapping". If you're simply suggesting that you create a new library, put that in the boilerplate, and document it, it makes no difference what the library is. If that's the case, why spend time doing it, just skip the "create new library" part and use an OSS library.

This also has the side benefit of introducing those customers to the idea that there are OSS projects out there that do interesting/weird/scary things. The old chinese proverb... "Give a man (or person) a fish...."

benaadams commented 7 years ago

Create something that's only goal is to avoid mass assignment. Include it in templates.

Templates should help people not to shoot themselves in the foot by default.

In usage add comment to look at docs (+url) for more advanced scenarios; in docs suggest other OSS mapping libraries - additionally provides promotion for OSS libraries.

Looking at the database landscape is this not like complaining that SqlCommand (and other db variants) taking parameters to avoid sql injection is stifling the OSS community around ORMs?

shaynevanasperen commented 7 years ago

I've read every comment here, and I think the best solution is to not ship this as a binary, but rather as source code in project templates, along with a comment stating something along the lines of

// If you outgrow this (which you probably will) then there are several
// more capable open source libraries available on http://nuget.org

Either that, or just use AutoMapper.

jkodroff commented 7 years ago

I don't like this idea. Please just support the existing libraries in the community and stay focused on core MVC concerns.

ctolkien commented 7 years ago

After watching the https://live.asp.net standup, I'm on board with this. It's not about MS replacing AutoMapper, it's a ~100 line method to make the templates simpler and more secure. At the moment, I think the docs/template lead people down the path of not using discreet view models.

benaadams commented 7 years ago

As a new user I want to quickly create a new website from a template and avoid common pitfalls like mass assignment for my few simple models.

Automapper would be in the top 5 dlls by size adding another 1/4 MB to a file new publish

Automapper-size

abatishchev commented 7 years ago

OMG, 400 kilobytes! Do you really care?

benaadams commented 7 years ago

OMG, 400 kilobytes! Do you really care?

Yes, I really do

alaatm commented 7 years ago

Just include source code of a simple mapper in the template, point people to more advanced ones in comments. Problem solved!

jbogard commented 7 years ago

...why?

On Tue, Mar 21, 2017 at 7:52 PM Ben Adams notifications@github.com wrote:

OMG, 400 kilobytes! Do you really care?

Yes, I really do

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/aspnet/Mvc/issues/5883#issuecomment-288255602, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGYMo8Qp8iUp-eYC0mZ4hhQLcku0C6Aks5roGKngaJpZM4MRuBp .

synhershko commented 7 years ago

@jbogard Jimmy why your code so bloated?

And seriously MS guys, let's just lock comments here and make a decision...

jbogard commented 7 years ago

It started out as 100 lines of code ;)

On Tue, Mar 21, 2017 at 7:56 PM Itamar Syn-Hershko notifications@github.com wrote:

@jbogard https://github.com/jbogard Jimmy why your code so bloated?

And seriously MS guys, let's just lock comments here and make a decision...

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/aspnet/Mvc/issues/5883#issuecomment-288256380, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGYMjgbFNA2GdtQ9zO7LsNffu3QXrVLks5roGOygaJpZM4MRuBp .

edandersen commented 7 years ago

Here is a crazy idea. How about stop making the built in MVC scaffolding present EF models to the View? Autogenerate a ViewModel class and a left/right mapping function that clearly looks stupid, forcing people to look for a better way of doing it. The current autogenning of [Bind] to prevent overposting is not great.

benaadams commented 7 years ago

OMG, 400 kilobytes! Do you really care?

Yes, I really do

...why?

Density. There isn't a good dead code removal linker for .NET currently. I don't use a server just for one thing but can run hundreds or thousand of services; want to start them fast and shut them down fast. Why is docker popular vs VMs; why is nano server better then full Windows server. Why are people interested in using alpine for their services (which gives you an 8MB base container).

Other people will have different reasons; maybe they are loading up a raspberry pi full of microservices.

I'm not complaining about the size of Automapper; just complaining about the minimum size for the base feature set.

benaadams commented 7 years ago

that clearly looks stupid, forcing people to look for a better way of doing it.

Then new people to the ecosystem will say .NET is rubbish. Templates are a lot of people's first exprience

andrewlock commented 7 years ago

I think @shaynevanasperen's suggestion is probably the best so far - don't ship a binary, just include the simple mapper in the templates, with comments about using other libraries for more complicated/production scenarios

jbogard commented 7 years ago

The old MVC 3/4 templates had code that shipped in them. And everyone was annoyed with having to immediately delete them.

brockallen commented 7 years ago

Then new people to the ecosystem will say .NET is rubbish.

I think the templates already go a long way down this path. Browser link and application insights are the first two culprits that come to mind.

edandersen commented 7 years ago

@benaadams the problem is that the templates and example code always go for the EF models in the View approach, implying it's good, if not best practice. An autogenned mapping function will be annoying to edit, prompting people to wonder if there is a better way, which of course there is.

NotMyself commented 7 years ago

Just adding my input and support for existing projects. Invest in your community, support the popular projects that already exist. AutoMapper & IS4 would both make a great addition to the .net foundation.

benaadams commented 7 years ago

From a different direction; like sample apps rather than starter templates, for example eShopOnContainers I think would be a very good place to demonstrate wider OSS components like Automapper; and it would be good to have more full featured sample apps.

martinjt commented 7 years ago

With respect, you're not the target audience of a starter template are you? This is a template meant to "introduce" people to the concept of mapping. If someone is actively looking to reduce their project size, or more to microservices, they're already on a path that would take through many, many, concepts that far beyond just mapping.

The basic empty template wouldn't include automapper I'm sure, but a more complete example should (imho) draw on multiple opensource projects to should that there is a world beyond the core libraries.

jbogard commented 7 years ago

That's kinda what I would assume. Empty is actually empty. A starter template would be representative. Because if the template includes EF, then it's not empty.

benaadams commented 7 years ago

With respect, you're not the target audience of a starter template are you?

Probably not; though you have to file new now and again and have a look around to catch up with the apis 😉

Empty is actually empty. A starter template would be representative. Because if the template includes EF, then it's not empty.

The starter templates don't currently include EF. Maybe the answer is a few more templates then?

ASP.NET Core Empty ASP.NET Core Web App ASP.NET Core Web API

Empty and WebAPI are fine. Web App is fairly opinionated on the front-end; but there could be some that explore the server-side a bit more? (Where other OSS libraries maybe appropriate)

Eilon commented 7 years ago

ASP.NET Core Web App + Individual Accounts (for auth) uses EF for the Identity/membership stuff. But no "traditional" data access in the sense of typical CRUD.

Bartmax commented 7 years ago

I think it would be good to share a kinda complete template with some code of what/where this will impact.

It's important to note how few code is around the mapping. I don't want, nor like to have AutoMapper as default. I don't need all it's complex stuff and also in my experience, it was easier to mess it up than doing plain.

The other idea here is that from my experience, BindingModels are plain contracts, and an Interface suits great for contracts, but they are not always possible to use, maybe something can be done on this front.

So, here is what it looks like. (I omitted the nested BindingModels inside a view model for brevity, but that's also another area I think the model binder can be improved too.)

public class ApplicationDbContext : DbContext
{
    public DbSet<Person> People { get; set; }
    public DbSet<Country> Countries { get; set; }
}

public class Country
{
    public int CountryId { get; set; }
    public string Name { get; set; }
    public bool IsAvailable { get; set; }
}

public class Person
{
    public int Id { get; set; }

    public string Name { get; set; }

    public Country Country { get; set; }

    public bool IsAdmin { get; set; }
}

public class PersonViewModel : IPersonFullBindingModel // optional but useful interface here.
{
    public PersonViewModel(Person person, List<Country> availableCountries)
    {
        // CURRENT CODE: MAPPING NEEDED HERE.
        Id = person.Id;
        Name = person.Name;
        IsAdmin = person.IsAdmin;
        Country = person.Country;
        Countries = availableCountries;
        // NEW MAP CODE: of course, it doesn't need to be here.
        // Map(person, this);
    }

    public int Id { get; }

    public string Name { get; }

    public bool IsAdmin { get; }

    public List<Country> Countries { get; }

    public Country Country { get; }
}

public interface IPersonBasicBindingModel
{
    [Required]
    int Id { get; }

    [Required]
    [MinLength(3)]
    string Name { get; }

}

public interface IPersonDetailsBindingModel
{
    [Required]
    Country Country { get; }
}

public interface IPersonFullBindingModel : IPersonBasicBindingModel, IPersonDetailsBindingModel
{

}

public class PersonService
{
    private readonly ApplicationDbContext context;

    public PersonService(ApplicationDbContext context)
    {
        this.context = context;
    }

    public Task Update(Person person, IPersonFullBindingModel source)
    {
        // CURRENT CODE: MAPPING NEEDED HERE.
        person.Country = source.Country;
        person.Name = source.Name;

        // NEW MAP CODE:
        // Map(source, person);
        context.People.Update(person);
        return context.SaveChangesAsync();
    }

    public Task Update(Person person, IPersonBasicBindingModel source)
    {
        // MAPPING NEEDED HERE.
        person.Name = source.Name;
        context.People.Update(person);
        return context.SaveChangesAsync();
    }

    public Task Update(Person person, IPersonDetailsBindingModel source)
    {
        // MAPPING NEEDED HERE.
        person.Country = source.Country;
        context.People.Update(person);
        return context.SaveChangesAsync();
    }

    internal Task<Person> GetAsync(int id)
    {
        return context.People.SingleOrDefaultAsync(p => p.Id == id);
    }
}

public class CountryService
{
    private readonly ApplicationDbContext context;

    public CountryService(ApplicationDbContext context)
    {
        this.context = context;
    }
    internal Task<List<Country>> GetAvailableCountriesAsync()
    {
        return context.Countries.Where(c => c.IsAvailable).ToListAsync();
    }
}

public class PersonController : Controller
{
    private readonly PersonService personService;
    private readonly CountryService countryService;

    public PersonController(PersonService personService, CountryService countryService)
    {
        this.personService = personService;
        this.countryService = countryService;
    }

    [HttpGet]
    public IActionResult Index()
    {
        return View();
    }

    [HttpGet]
    public async Task<IActionResult> Edit(int id)
    {
        var person = await personService.GetAsync(id);
        var countries = await countryService.GetAvailableCountriesAsync();
        if (person == null)
        {
            return NotFound();
        }
        // CURRENT CODE: MAPPING HERE
        ViewData.Model = new PersonViewModel(person, countries);
        // Instead of the constructor, it can be a factory-like method that returns a new instance:
        // ViewData.Model = Map.From(person).From(countries);
        // or
        // ViewData.Model = Map.From(person, countries);
        return View();
    }

    [HttpPost]
    public async Task<IActionResult> Edit(IPersonFullBindingModel binding)
    {
        if (!ModelState.IsValid)
        {
            return await Edit(binding.Id);
        }
        var person = await personService.GetAsync(binding.Id);
        if (person == null)
        {
            return BadRequest();
        }
        await personService.Update(person, binding);
        // another approach (without a service) would be
        // var updated = Map(person, binding)
        // and save the updated object.

        return RedirectToAction(nameof(Index));
    }
}
Bartmax commented 7 years ago

I think that even without a mapper on the framework, using a file new template more like ☝️ will be enough.

wastaz commented 7 years ago

Since at least part of this problem stems from people incorrectly using EF models in the views...how about just going in a different direction and ditching EF completely instead? Pushing people towards using EF as the default instead of good OSS community alternatives is just as bad as pusing people towards using a "new simple microsoft mapper" instead of Automapper/Tinymapper/Whatevermapper. Tbh, it's probably worse since EF can cause so much worse performance problems and encourage horrible patterns than a relatively small component such as an objectmapper. :trollface: