simpleinjector / SimpleInjector

An easy, flexible, and fast Dependency Injection library that promotes best practice to steer developers towards the pit of success.
https://simpleinjector.org
MIT License
1.21k stars 153 forks source link

Integration with AutoMapper #899

Closed aa-infr closed 3 years ago

aa-infr commented 3 years ago

I've the following two diagnostic warnings:

-[Lifestyle Mismatch] MapperConfiguration (Singleton) depends on Profile implemented by SampleMapping (Transient). SampleMapping is part of the collection of Profile services that is injected into MapperConfiguration. The problem in MapperConfiguration is that instead of storing the injected collection of Profile services in a private field and iterating over it at the point its instances are required, SampleMapping is being resolved (from the collection) during object construction. Resolving services from an injected collection during object construction (e.g. by calling collection.ToList() in the constructor) is not advised. -[Lifestyle Mismatch] MapperConfiguration (Singleton) depends on Profile implemented by PaginatedListResultMappingProfile (Transient). PaginatedListResultMappingProfile is part of the collection of Profile services that is injected into MapperConfiguration. The problem in MapperConfiguration is that instead of storing the injected collection of Profile services in a private field and iterating over it at the point its instances are required, PaginatedListResultMappingProfile is being resolved (from the collection) during object construction. Resolving services from an injected collection during object construction (e.g. by calling collection.ToList() in the constructor) is not advised.

I've the following registration:

container.Collection.Append(typeof(MappingProfile), typeof(SampleMapping), Lifestyle.Transient);
container.Collection.Append(typeof(Profile), typeof(SampleMapping), Lifestyle.Transient);
container.Register(() =>
{
    return new MapperConfiguration(cfg =>
    {
        cfg.ConstructServicesUsing(t => container.GetInstance(t));
        var profiles = container.GetAllInstances<Profile>();
        foreach (var profile in profiles)
            cfg.AddProfile(profile);
    });
}, Lifestyle.Singleton);

container.Register(() => container.GetInstance<MapperConfiguration>().CreateMapper(), Lifestyle.Scoped);

The classes related to Automapper are the following:

[IoCRegistration(RegistrationLifeTime.Transient)]
public abstract class MappingProfile : Profile
{
    protected MappingProfile()
    {
    }
}

public class PaginatedListResultMappingProfile : MappingProfile
{
    public PaginatedListResultMappingProfile()
    {
        CreateMap(typeof(PaginatedListResult<>), typeof(PaginatedListResultDto<>))
            .ConvertUsing(typeof(PaginatedListResultConverter<,>));
        CreateMap(typeof(PaginatedItems<>), typeof(PaginatedItems<>))
            .ConvertUsing(typeof(PaginatedItemsConverter<,>));
    }
}

public class SampleMapping : MappingProfile
{
    public SampleMapping(CultureConverter cultureConverter)
    {
        CreateMap<Sample, SampleDto>()
        .ForMember(dst => dst.Name, opt => opt.MapFrom(src => src.Name))
        .ForMember(dst => dst.CreationDate, opt => opt.MapFrom(src => src.CreationDate
                                                                         .ToIso8601(true)))
        .ForMember(dst => dst.UpdateDate, opt => opt.MapFrom(src => src.UpdateDate
                                                                       .ToIso8601(true)))
        .ForMember(dst => dst.Creator, opt => opt.MapFrom(src => src.CreatedBy))
        .ForMember(dst => dst.Updator, opt => opt.MapFrom(src => src.UpdatedBy));

        CreateMap<CreateSampleDto, Sample>();

        CreateMap<UpdateSampleDto, Sample>();
    }
}

What I'm doing wrong here?

dotnetjunkie commented 3 years ago

Simple Injector is warning you about a lifestyle mismatch (captive dependency) between MapperConfiguration and the collection of Profile instances.

The code that causes problems is the following:

return new MapperConfiguration(cfg =>
{
    cfg.ConstructServicesUsing(t => container.GetInstance(t));
    var profiles = container.GetAllInstances<Profile>();
    foreach (var profile in profiles)
        cfg.AddProfile(profile);
});

What is happening here is that during the construction of MapperConfiguration, you resolve a stream of Profile instances. That by itself is fine, just as it is fine to inject an IEnumerable<T> of transient dependencies into a Singleton (when using Simple Injector).

You, however, are at that point iterating the collection:

foreach (var profile in profiles)

This is what trips Simple Injector's detection, because it thinks you might be storing those dependencies for later use; which you are:

cfg.AddProfile(profile);

This means that Simple Injector is right. The profiles are will be held captive inside the MapperConfiguration.

Whether this is a problem, however, depends on whether those dependencies contain state, or have stateful dependencies dependencies. But even if they don't contain state, by marking them as Transient, you explicitly state that they might contain state, either now or in the future.

As I see it, there are several solutions.

Probably the simplest solution is to make the Profile registrations Singleton. This makes it very explicit that they should be stateless (or at least thread safe). This means that Simple Injector will again warn when they get Transient or Scoped dependencies:

container.Collection.Append(typeof(MappingProfile), typeof(SampleMapping), Lifestyle.Singleton);
container.Collection.Append(typeof(Profile), typeof(SampleMapping), Lifestyle.Singleton);
container.Register(() =>
{
    return new MapperConfiguration(...);
}, ...);

Making the profiles Singleton prevents Simple Injector from warning about a captive dependency, because even though the collection is still iterated, all elements are Singletons and can, therefore, be safely stored.

MapperConfiguration, however, is something that might not require to be constructed by the DI Container. So what you could do instead is to construct MapperConfiguration with its dependencies beforehand:

var config = new MapperConfiguration(cfg =>
{
    cfg.ConstructServicesUsing(container.GetInstance);
    cfg.AddProfile(new SampleMapping());
    cfg.AddProfile(new PaginatedListResultMappingProfile());
};

container.Register(config.CreateMapper, Lifestyle.Scoped);

Advantage of this is that it becomes much clearer that SampleMapping and PaginatedListResultMappingProfile shouldn't get dependencies (other than simple singletons that can be constructed upfront) to prevent captive dependencies.

Of course, both situations might cause trouble when you need to inject a DbContext into them, but that trouble already exists in the current case (and possibly AutoMapper's design) because MapperConfiguration is expected to be Singleton.

J-Hauser commented 3 years ago

There is also an example how to use AutoMapper with SimpleInjector in the AutoMapper Documentation.