AutoMapper / AutoMapper.Extensions.Microsoft.DependencyInjection

MIT License
258 stars 79 forks source link

Please add support for calling AddAutoMapper() multiple times #25

Closed prochnowc closed 7 years ago

prochnowc commented 7 years ago

Please consider adding support for calling AddAutoMapper() multiple times.

Reason: We have very modular application design, each module is responsible of registering own mapper profiles. Currently services.AddAutoMapper(i => i.AddProfile(...)); cannot be called multiple times because of Mapper.Initialize being overwriting previous setup. Additionally services are not added with TryAdd...

lbargaoanu commented 7 years ago

You can call AddProfile itself more than once, or use any of the other overloads that take multiple types/assemblies.

prochnowc commented 7 years ago

@lbargaoanu Hmm. But if you call AddProfile itself multiple times the AddAutoMapper call need to know all profiles which is not feasible.

Consider this:

In my opinion registration of AutoMapper is an implementation detail of the module and the application should not know anything about it.

It's true that i can simply add all assemblies which implement module's to the AddAutoMapper call. But then the application needs to know that the module uses AutoMapper. And this would scan all assemblies / types - but i worry about startup performance.

lbargaoanu commented 7 years ago

Registration needs to happen once per AppDomain (or process, whatever they have in .NET Core). So it's not the case that modules can initialize on their own. I'm sure that other libraries work the same way, nothing unusual here.

prochnowc commented 7 years ago

Hmm i understand that it depends on Mapper.Initialize() static call .. but this can be delayed until first resolve of IMapper - are you willing to accept PRs ?

lbargaoanu commented 7 years ago

@jbogard will reopen this if he's interested.

lbargaoanu commented 7 years ago

Until then you could try to do this with your DI container. All this stuff sounds like its job. It's more of a convenience thing that AM does this.

prochnowc commented 7 years ago

Yeah its a job of the DI container. But it cant be done when AddAutoMapper cannot be called multiple times, because then the application needs to know about AutoMapper.

lbargaoanu commented 7 years ago

Of course it can. You just have to do it yourself. You can always encapsulate smth. The only difference is that AM doesn't do it for you.

prochnowc commented 7 years ago

OK - thanks for your suggestions. I will prepare PR which enhances AddAutoMapper() maybe you will like it and accept.

I have other problem running unit tests in parallel because AddAutoMapper() does use static configuration. Eg. using cfg => cfg.AddCollectionMappers() sometimes throws NullReferenceException because it internally uses static caches. Shall i open issue about AddCollectionMappers() throwing exception on concurrent scenarios?

lbargaoanu commented 7 years ago

Yes, but that's smth outside AM per se. If you have a repro only with AM, I'll take a look. There are no threading issues with AM that we are aware at this time.

lbargaoanu commented 7 years ago

Open an issue in https://github.com/AutoMapper/AutoMapper.Collection. And @TylerCarlson1 will let us know if there is smth we should do.

jbogard commented 7 years ago

There's a simple solution here. Pass around an Action<IMapperConfigurationExpression> instead of (or along with) your other Action. Then the calling class gathers them all together to execute in AddAutoMapper. Or have them return an action.

lbargaoanu commented 7 years ago

@jbogard I think the idea was to avoid that last step. Just have every module do some work and somehow, transparently, to apply all the configuration.

jbogard commented 7 years ago

Yeah I guess I don't know what that would look like. We did so much work to have an explicit configuration step.

prochnowc commented 7 years ago

@lbargaoanu @jbogard It will still be explicit configuration .. even more explicit than now. I've prepared some changes please have a look at this branch: https://github.com/smarthouse/AutoMapper.Extensions.Microsoft.DependencyInjection/tree/non-static-init

Should i open PR to discuss changes?

jbogard commented 7 years ago

Yeah, just mark as WIP.

On Mon, Jul 3, 2017 at 3:10 AM, prochnowc notifications@github.com wrote:

@lbargaoanu https://github.com/lbargaoanu @jbogard https://github.com/jbogard It will still be explicit configuration .. even more explicit than now. I've prepared some changes please have a look at this branch: https://github.com/smarthouse/AutoMapper.Extensions. Microsoft.DependencyInjection/tree/non-static-init

Should i open PR to discuss changes?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/AutoMapper/AutoMapper.Extensions.Microsoft.DependencyInjection/issues/25#issuecomment-312578636, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGYMuVoN7Pcaz-7IVjzsW7iKG1rx_gTks5sKKIPgaJpZM4OI79S .

prochnowc commented 7 years ago

Created PR https://github.com/AutoMapper/AutoMapper.Extensions.Microsoft.DependencyInjection/pull/26

prochnowc commented 7 years ago

@jbogard any comments, thoughts, suggestions?

sfmskywalker commented 6 years ago

@prochnowc Did you ever get anywhere with this? I'm running into the same scenario: I have several class libraries acting as modules that are unaware of one another. Each of them have their own mapping profiles that I want to add, and ideally they can all just call AddAutoMapper on services that is passed around to each one of them.

lbargaoanu commented 6 years ago

There is no need for the modules to call AddAutoMapper, they just define the profiles. The main app calls AddAutoMapper and passes the modules's assemblies.

sfmskywalker commented 6 years ago

Thanks for the prompt reply. I understand, and that certainly does work. In my case however I have each module expose "features" that can be configured to be turned on or off. If a feature is enabled, its own startup class will execute. This makes the main app more or less unaware of which modules are there.

Anyway, what I'm doing now is having each module's Startup class register their profiles with the container explicitly instead of letting the AddAutoMapper extension method discover them for me:

Module 1

services
   .AddAutoMapper()
   .AddTransient<Profile1>()
   .AddTransient<Profile2>()

Module 2

services
   .AddAutoMapper()
   .AddTransient<Profile1>()
   .AddTransient<Profile2>()

etc.

This works fine, but it would be a little bit nicer if I could rely on the profile discovery code provided by AutoMapper (without having to use Autofac or others as I prefer to stick with the built-in DI for as long as I can).

I also understand this may be an edge case, so probably not worthwhile to update AddAutoMapper to check if it's already registered, and simply perform the assembly scanning.

VictorioBerra commented 6 years ago

I just ran into this exact problem today. I am also going to go to the main assembly and add this assemblies profiles but if someone NuGet one of my supporting libraries they would have to be the ones to set up AutoMapper and that is not desirable in the slightest.

Even a TryAddAutoMapperProfiles(params Assemblies[]) would be really useful. You would only use this if you were not sure if AddAutoMapper was already called. Using this would either add AutoMapper, or update the profiles with the latest assembly or whatever assemblies were passed in.

in general I think theres a bigger non-AutoMapper issue here, as Core grows in usage more people are rolling libraries that depend on DI. Just look at the IdentityServer project. We need a standard way to safely manipulate the service collection and not clobber it with an overwrite or a silent ignore if something has already been added.

jbogard commented 6 years ago

If this is a new issue then please open a new issue in GitHub. I don't track comments from closed issues.

On Thu, Oct 4, 2018 at 1:06 PM Victorio Berra notifications@github.com wrote:

I just ran into this exact problem today. I am also going to go to the main assembly and add this assemblies profiles but if someone NuGet one of my supporting libraries they would have to be the ones to set up AutoMapper and that is not desirable in the slightest.

Even a TryAddAutoMapperProfiles(params Assemblies[]) would be really useful. You would only use this if you were not sure if AddAutoMapper was already called. Using this would either add AutoMapper, or update the profiles with the latest assembly or whatever assemblies were passed in.

in general I think theres a bigger non-AutoMapper issue here, as Core grows in usage more people are rolling libraries that depend on DI. Just look at the IdentityServer project. We need a standard way to safely manipulate the service collection and not clobber it with an overwrite or a silent ignore if something has already been added.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/AutoMapper/AutoMapper.Extensions.Microsoft.DependencyInjection/issues/25#issuecomment-427115718, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGYMkIvO4cSLCRDgpCjf-PqrMmpx4dWks5uhk4ggaJpZM4OI79S .

VictorioBerra commented 6 years ago

@jbogard this is an open issue (the one and only open issue on AutoMapper.Extensions.Microsoft.DependencyInjection as of writing this), I am just casting my vote for a solution.

jbogard commented 6 years ago

It's been closed for over a year?

VictorioBerra commented 6 years ago

Oops got my wires crossed. Ill think on this and open a new issue. Sorry about that.

lock[bot] commented 5 years ago

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.