search

AutoMapper / AutoMapper.Extensions.Microsoft.DependencyInjection

MIT License
258 stars 79 forks source link

Add support for injection of dependencies into Profiles constructor #40

Closed Carl-Hugo closed 6 years ago

Carl-Hugo commented 6 years ago

Add support for injection of dependencies into Profiles constructor when UseStaticRegistration == false. This is not supported when UseStaticRegistration == true because there is no IServiceProvider available there (that I know of at least).

jbogard commented 6 years ago

Hrm you really shouldn’t be doing this, Profiles are statically cached. Are your dependencies Singleton?

On Thu, Dec 7, 2017 at 8:10 PM Carl-Hugo Marcotte notifications@github.com wrote:

Add support for injection of dependencies into Profiles constructor when UseStaticRegistration == false. This is not supported when UseStaticRegistration == true because there is no IServiceProvider available there (that I know of at least).

You can view, comment on, or merge this pull request online at:

https://github.com/AutoMapper/AutoMapper.Extensions.Microsoft.DependencyInjection/pull/40 Commit Summary

  • Added support for DI in Profile.
  • Cleanup
  • Replaced \r\n to \n
  • Updated comments
  • Moved profile registration block.

File Changes

Patch Links:

- https://github.com/AutoMapper/AutoMapper.Extensions.Microsoft.DependencyInjection/pull/40.patch

https://github.com/AutoMapper/AutoMapper.Extensions.Microsoft.DependencyInjection/pull/40.diff

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/AutoMapper/AutoMapper.Extensions.Microsoft.DependencyInjection/pull/40, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGYMlW73D4FEtQ4GyRqTKNvHQgUypVcks5s-LatgaJpZM4Q6kFZ .

Carl-Hugo commented 6 years ago

you really shouldn’t be doing this

Why not?

Profiles are statically cached

That's why I did not mind adding profiles twice to the IServiceProvider; since I knew the profiles were only built once.

Are your dependencies Singleton?

Yes, as most of my dependencies (whenever it's possible). I could have added Profiles as Singleton instead of transient, now that I think about it.

jbogard commented 6 years ago

It’s like wanting to inject dependencies into appsettings.json. Profiles are configuration, what are you injecting?

On Fri, Dec 8, 2017 at 7:31 AM Carl-Hugo Marcotte notifications@github.com wrote:

you really shouldn’t be doing this

Why not?

Profiles are statically cached

That's why I did not mind adding profiles twice to the IServiceProvider; since I knew the profiles were only built once.

Are your dependencies Singleton?

Yes, as most of my dependencies (whenever it's possible). I could have added Profiles as Singleton instead of transient, now that I think about it.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/AutoMapper/AutoMapper.Extensions.Microsoft.DependencyInjection/pull/40#issuecomment-350263684, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGYMmEn_rJo49u3YnxWqsM0NNmQyymeks5s-To6gaJpZM4Q6kFZ .

Carl-Hugo commented 6 years ago

I share multiple AfterMap actions between multiple profiles. All profiles are not supporting the same number of maps (ex.: one may need one AfterMap, the other might need 2, etc.). At this point, options are endless.

Therefore, instead of using inheritance, I'd prefer to go for composition, injecting only the services that I need for each profile. That would also lead to a more granular control over my entities. For example, my model classes won't have to inherit from a big base class that has to support all the features of the base profile (let's call it BaseProfile<TDataEntity>). My model classes could instead implement smaller interfaces (even inherit from some other class if needed; without impacting the shared code).

With DI support in Profile, I could inject these different services handling different scenario without requiring a base profile that support all use cases. I would not have to create a static class either, nor extension methods (allowing me to configure these tiny pieces of logic at the composition root level).

More on that, in one of the AfterMap action that I was talking about, I want to map the value of an HTTP header to a property of the destination object. To do that, I need an IHttpContextAccessor, and DI allows me to have that.

I felt it was convenient to set that value during mapping since mapping does occur one way or another at the right place of the overall application flow.

If you have a better idea, I am all ears.

Thanks for your feedback

Carl-Hugo commented 6 years ago

Humm... I just thought about a little part of the code that I may have overlooked (it was kinda late):

var openTypes = new[]
{
    typeof(IValueResolver<,,>),
    typeof(IMemberValueResolver<,,,>),
    typeof(ITypeConverter<,>),
    typeof(IMappingAction<,>)
};

I guess that IMappingAction<,> do exactly what I am trying to achieve, right?

I will experiment with this a little...

Carl-Hugo commented 6 years ago

A few minutes of experimenting later and yes IMappingAction<,> do exactly what I was trying to achieve using DI in profile.

Back to your initial reply:

Hrm you really shouldn’t be doing this

You were right; there was no need for this.

The positive part is that doing this helped me find out about IMappingAction<,>. This interface seems to lack documentation, is there a place where I could contribute to it?

Sorry to have bothered you with this PR and thanks for your time.

jbogard commented 6 years ago

No worries! This just tells me I need better docs.

They’re at docs.automapper.org and you’ll see an edit button there. Thanks!

On Fri, Dec 8, 2017 at 3:00 PM Carl-Hugo Marcotte notifications@github.com wrote:

A few minutes of experimenting later and yes IMappingAction<,> do exactly what I was trying to achieve using DI in profile.

Back to your initial reply:

Hrm you really shouldn’t be doing this

You were right; there was no need for this.

The positive part is that doing this helped me find out about IMappingAction<,>. This interface seems to lack documentation, is there a place where I could contribute to it?

Sorry to have bothered you with this PR and thanks for your time.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/AutoMapper/AutoMapper.Extensions.Microsoft.DependencyInjection/pull/40#issuecomment-350370606, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGYMmHGFqKrXLk0nZ8HOLoi3I9KqMWKks5s-aNygaJpZM4Q6kFZ .

Carl-Hugo commented 6 years ago

I added a little bit of doc covering this topic in Before and After Map Action. I opened that PR: https://github.com/AutoMapper/AutoMapper/pull/2477

I also created a working sample here: https://github.com/Carl-Hugo/AutoMapperDependencyInjectionSample

I have not included any link to the docs, so I leave it here if you want to do something with it (I may write a little article about that if I find the time).

I will close this PR since it seems useless.

Happy new year and keep up the good work

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.