AutoMapper / AutoMapper.Extensions.Microsoft.DependencyInjection

MIT License
258 stars 79 forks source link

Allow usage of Options Pattern to configure AutoMapper #132

Closed berhir closed 4 years ago

berhir commented 4 years ago

In ASP.NET Core (and in .NET in general with the generic host) the options pattern is used in many places. It would be a nice addition if you also support it in this library.

Reason: In our projects, we usually have multiple modules and each module has its own extension method to register its services and dependencies in the DI system. We don't want to register AutoMapper in one place in the Startup.cs, instead each module should register and configure AutoMapper itself.

Therefore we created our own extensions to register AutoMapper. This looks like so:

In Startup.cs we register only the modules. AutoMapper is not required.

public void ConfigureServices(IServiceCollection services)
{
    services.AddModule1();
    services.AddModule2();
}

The extension method of each module looks like this:

public static void AddModule1(this IServiceCollection services)
{
    services.AddAutoMapper(cfg => cfg.AddProfile<Module1Profile>());
}

And this are the extensions to register AutoMapper:

public static IServiceCollection AddAutoMapper(this IServiceCollection services, Action<MapperConfigurationExpression> configureOptions)
{
    services.Configure(configureOptions);
    return services.AddAutoMapper();
}

public static IServiceCollection AddAutoMapper(this IServiceCollection services)
{
    // Just return if we've already added AutoMapper to avoid double-registration
    if (services.Any(sd => sd.ServiceType == typeof(IMapper)))
    {
        return services;
    }

    services.AddSingleton<IConfigurationProvider>(sp =>
    {
        // A mapper configuration is required
        var options = sp.GetRequiredService<IOptions<MapperConfigurationExpression>>();
        return new MapperConfiguration(options.Value);
    });

    services.AddSingleton<IMapper>(sp => new Mapper(sp.GetRequiredService<IConfigurationProvider>()));
    return services;
}

I don't say this should be the only option. It should work together with the current way to add AutoMapper via assembly scanning. Did you already consider this approach? Is there a reason why you don't support it?

lbargaoanu commented 4 years ago

You should have your own package that works this way. But you need a new name for the extension method :)

jbogard commented 4 years ago

I don’t know how we’d technically support this anyway. Maybe some code to show how this might work?

berhir commented 4 years ago

@jbogard @lbargaoanu I have created a sample here: https://github.com/berhir/AutoMapperOptions Please let me know if you have further questions.

jbogard commented 4 years ago

Ahhh I see now. I didn't realize that the IOptions stuff had its own internal lifecycle, that the options get executed first. I like this way much better than how we do it today - where we provide a tiny window into the configuration if you want. TBQH I would ditch all of the current API for just doing this 😆

One thing I tried is combining scanning - so in your individual projects, instead of adding profiles manually (which can be a pain), calling AddMaps inside there.

I'd likely still want to register all of the ancillary services (resolvers, type converters etc.) because this is consistent with other libraries like Fluent Validation.

Going to reopen for further discussion.

lbargaoanu commented 4 years ago

Reusing the name for smth new is just looking for trouble :) Also, this is clearly more complicated and I think the vast majority of people don't need it. So having this as the default seems like a bad idea to me.

jbogard commented 4 years ago

I like the idea of IOptions - but I think we'd want to simplify the interface. Perhaps create our own class to preserve the simplicity?

EF Core for example has Action<DbContextOptionsBuilder> and only allows explicit parameters for service lifetime. Everything else is done through that action.

lbargaoanu commented 4 years ago

+1 for our own thing. I don't know much about ASP.NET Core, but options means app configuration to me.

berhir commented 4 years ago

I have added a draft PR to show how the options pattern can be added in a non-breaking way.

The main difference is that AddAutoMapper can be called multiple times now. The first call registers the IConfigurationProvider and the IMapper. This means only the first call can set the service lifetime.

jbogard commented 4 years ago

@lbargaoanu yeah me too, I assumed it was just "things coming in thru configuration" but reviewing some APIs from MS look different. Looks like it's a way to have a sort of 2-step config process - gathering all the config, then waiting until the first usage to kick off the final registration.

lbargaoanu commented 4 years ago

Yes, but you don't really need that, right? You can simply have a method passing an Action<MapperConfigurationExpression> (or smth else) as you've said.

AddAutoMapperModule(Action<MapperConfigurationExpression> moduleConfig);
jbogard commented 4 years ago

Kinda - except we also register other services. It's not as simple as just reusing MapperConfigurationExpression.

lbargaoanu commented 4 years ago

Right, so you scan for those in the current assembly.

jbogard commented 4 years ago

Fixed by #135

github-actions[bot] commented 4 years ago

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