AutoMapper / AutoMapper.Extensions.Microsoft.DependencyInjection

MIT License
258 stars 79 forks source link

The lack of modular configuration is making life hard for large solutions #127

Closed oscar54321 closed 4 years ago

oscar54321 commented 4 years ago

Hello All,

We are working on a large solution, 100+ projects with that constitute around 15 executables. The way we use DI throughout our solution, for every assembly exposing services we create a one-tw bootstrapping classes for the current assembly services registration. On executable level, where services are consumed, we call these modules' bootstrapping classes. So, our DI is organized into a hierarchy and is easy to plug in into service collections of individual executables.

AutoMapper DI implementation in its current form requires a single AddAutoMapper call for all the assemblies involved. Such approach does not really scale well. Even when registration by referencing Assembly is used, the registration remains "flat" structure and with the number of assemblies growing is hard to maintain.

I am wondering if it would be possible to add support of modular registration, analogous to, for example, configuration options? It's a straightforward change, to add support for adding multiple configuration callbacks and then injecting them into Mapper singleton creation callback using IEnumerable<MapperConfigurationElement> DI feature. Then Automapper DI configuration could be split in modules.

To have an example, you may reference how Microsoft OptionsFactory consumes multiple options configuration callbacks.

https://github.com/aspnet/Options/blob/master/src/Microsoft.Extensions.Options/OptionsFactory.cs

lbargaoanu commented 4 years ago

You have 1000 assemblies, then you have 1000 profiles (or more). That's how it scales. You can always have one configuration per assembly if you prefer, but that would mean those mappers are completely separate. So I'm not sure I understand what the problem is.

oscar54321 commented 4 years ago

I thought about my issues more and it appears that the biggest sticking point is encapsulation difficulty. Please find my examples below.

Example 1. In this example, let's consider a project with assemblies that implement services requiring DI registration and application that uses them. In this simplest form, we can see that on application level registering auto mapper cannot be inapsulated inside of individual assemblies' Configure services. Instead, application must reference marker types for all the assemblies in the application. This creates two problems:

Problem A Instead of having a maintaining a single bootstrap (sequence ConfigureServices), there must be two : 1) sequence of ConfigureServices and 2) AddAutoMapper() call listing all assemblies. There is the most obvious way to improve code by allowing AutoMapper profile registrations to occur within individual ConfigureServices calls, where each assembly will encapsulate mapping profiles registrations that it introduces.

Problem B Mapping profiles are inviting modular design. It makes a lot of sense to create multiple mapping profiles per assembly, each responsible for mapping of a related set of classes, not a single one per assembly. If this how one designs profiles, referencing assemblies by means of mapping profile as ambiguous and raises questions a) which profile should I use? b) if I move a mapping profile to a different assembly my assembly reference will break, in a very hidden way.

One could argue that it is possible to create a "marker" profile that never leaves assembly, so I am not going to argue a lot about Problem B besides that it is not an apparent workaround.


DataAccess
    DataAccessBootstrap.ConfigureServices
Reports 
    ReportsBootstap.ConfigureServices

Smtp    
    SmtpBootstrap.ConfigureServices

Web
    WebBootstrap.ConfigureServices

Windows
    WindowsBootstrap.ConfigureServices

Azure
    AzureBootstrap.ConfigureServices

Web Application: App1
    App1.ConfigureServices
        AzureBootstrap.ConfigureServices
        SmtpBootstrap.ConfigureServices
        DataAccessBootstrap.ConfigureServices
        WebBootstrap.ConfigureServices (etc, there could be tens of such lines)

        AutoMapper.AddAutoMapper(typeof(Azure.AzureMappingProfile).Assembly, typeof(Smtp.SmtpMappingProfile).Assembly, typeof(DataAccess.DataAcessMappingProfile).Assembly, typeof(WebApp1.WebApp1MappingProfile).Assembly)

Example 2. In this example, I would like to offer an example of a big assembly that implements multiple ConfigureServices calls that are meant for different types of consumers. It is quite possible that different consumers require different set of mapping profiles. It is also possible that different consumers require different functionality of mapping logic (for example, non-secure consumer could have sensitive data encrypted during the mapping for downstream). A possibility to register profiles inside of ConfigureServices would allow only mapping profiles required for this particular set of services to be registered.

lbargaoanu commented 4 years ago

You decide what assemblies to pass. Surely you can define your own conventions so you don't simply list all assemblies of interest. Going further, you can always do your own profile scanning. I would expect a more advanced DI engine to have features to help here. Or extensions like Scrutor.

lbargaoanu commented 4 years ago

Another idea is to have you own generic IMapper interface and use that generic parameter to distinguish between the different mapping strategies you might want. Again, some DI engines support that.

lbargaoanu commented 4 years ago

See also https://github.com/AutoMapper/AutoMapper/blob/master/docs/Setup.md#gathering-configuration-before-initialization.

jbogard commented 4 years ago

Right, this library is merely convenience, but we simply call AddMaps to add all the profiles from the assemblies you pass us. A Profile is a way for an assembly to have its own configuration.

We used to do assembly scanning, but removed it because it had too many problems. You can do your own form of assembly scanning if you like.

oscar54321 commented 4 years ago

My preference is for using profiles, and I envision that that DI support should expose registering multiple callbacks for IMapperConfiguration, not a single call for injecting auto mapper, like this:

public static IServiceCollection ConfigureMapping(this IServiceCollection services, Action<IServiceProvider, IMapperConfigurationAction> configureAction)
{
    AddAutoMapper(this IServiceCollection services)

    services.AddTransient<IMapperConfigurationAction>(new MapperConfigurationAction(configureAction>))
}

public static IServiceCollection AddAutoMapper(this IServiceCollection services)
{
    services.TryAdd(ServiceDescriptor.Transient(typeof(IMapper), (serviceProvider) => 
    {
        IAutoMapperFactory factory = serviceProvider.GetRequiredService<IAutoMapperFactory();
        return factory.CreateMapper();      
    }

    services.TryAdd(ServiceDescriptor.Transient(typeof(IAutoMapperFactory), typeof(AutoMapperFactory)));

    return services;

}

public interface IMapperConfigurationAction
{
    void Configure(IServiceProvider serviceProvider, IMapperConfigurationExpression cfg);
}

public class AutoMapperFactory
{
    public AutoMapperFactory(IServiceProvider serviceProvider, IEnumerable<IMapperConfigurationAction> actions)
    {
        ///
    }

    IMapper IAutoMapperFactory.CreateMapper()
    {
        var cfg = new MapperConfigurationExpression();

        foreach (var configureAction in _actions)
        {
            configureAction.Configure(_serviceProvider, cfg);
        }

        var mapperConfig = new MapperConfiguration(cfg);
        IMapper mapper = new Mapper(mapperConfig);
        return mapper;              
    }
}

I can pay for this functionality added to official library that gets downloaded from VStudio, $300 sounds reasonable?

oscar54321 commented 4 years ago

BTW, if it is expensive to create configuration, IMapper should be scoped instead of transient.

jbogard commented 4 years ago

IMapper is not where configuration lives, the configuration object is singleton.

On Mon, Apr 13, 2020 at 11:03 AM oscar54321 notifications@github.com wrote:

BTW, if it is expensive to create configuration, IMapper should be scoped instead of transient.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/AutoMapper/AutoMapper.Extensions.Microsoft.DependencyInjection/issues/127#issuecomment-612963248, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAZQMUD4B2CHFZL7AAHAVLRMMZUNANCNFSM4MGE4YLQ .

oscar54321 commented 4 years ago

Ok, one can cache in DI as Scoped or Singleton either IMapper or configuration interface after it was configured. I can see both options important to have and at least Singleton being absolutely required.

Jimmie, you are of the obvious preference, as you know the library the best. Would you be interested in taking on the effort? I am not familiar with rules of community development so please forgive me if it is rude.

I can pay for this functionality added to official library that gets downloaded from VStudio, $300 sounds reasonable?

lbargaoanu commented 4 years ago

I think it's a good idea to sponsor someone to implement what you need. But this has nothing to do with this project.

lock[bot] commented 4 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.