Option AllowDuplicateCallsPerMapping #305

Closed
wants to merge 2 commits into
from

Projects

None yet

5 participants

@dariusdamalakas

New option AllowDuplicateCallsPerMapping to track whether a typemap pair has already been registered or not.

Owner
jbogard commented Jun 20, 2013

Why would you call CreateMap multiple times? BTW, it doesn't override things, it re-uses type maps.

Why would you call CreateMap multiple times?
In a perfect world - never. In real world, when we have more 250 calls to that, spread over 25 files, there are cases when people accidentally call CreateMap second time.

BTW, it doesn't override things, it re-uses type maps.
And that's the problem - when you have 2 calls to CreateMap for the same pair of types, what you end up is a mixture of two, basically just unpredictable results.

If you set this setting (we do that in unit-tests), then AutoMapper will blow-up saying you can't call CreateMap twice for same type pair, thus, developer will become aware that he tried to re-map things again.

P.s. we've already discussed this, and you where happy with this proposal here:
https://groups.google.com/forum/?fromgroups#!searchin/automapper-users/from$3Adarius/automapper-users/pDoesWMYgXg/nOFkgYb8N7IJ

Owner
jbogard commented Jun 21, 2013

Ha. Duh. Thanks.

On Thu, Jun 20, 2013 at 1:45 AM, Darius Damalakas
notifications@github.comwrote:

Why would you call CreateMap multiple times?
In a perfect world - never. In real world, when we have more 250 calls to
that, spread over 25 files, there are cases when people accidentally call
CreateMap second time.

BTW, it doesn't override things, it re-uses type maps.
And that's the problem - when you have 2 calls to CreateMap for the same
pair of types, what you end up is a mixture of two, basically just
unpredictable results.

If you set this setting (we do that in unit-tests), then AutoMapper will
blow-up saying you can't call CreateMap twice for same type pair, thus,
developer will become aware that he tried to re-map things again.

P.s. we've already discussed this, and you where happy with this proposal
here:

https://groups.google.com/forum/?fromgroups#!searchin/automapper-users/from$3Adarius/automapper-users/pDoesWMYgXg/nOFkgYb8N7IJ


Reply to this email directly or view it on GitHubhttps://github.com/AutoMapper/AutoMapper/pull/305#issuecomment-19733533
.

So what do you think about it? Do you want to include this, or would rather not add this? Would be good to know whether this will be included or not, as we are using our forked build of AutoMapper right now.

We would like to see this included also. For large projects, especially with profiles, etc, this is very helpful.

adexios commented Oct 23, 2013

Love the mapper, but we're having trouble managing mappings in a large project, and manually searching for duplicates is nearly impossible. Tried testing for duplicates, but apparently create map doesn't actually create duplicates, so this will amount to some obscure bugs from time to time.

Please include this update as an option. Thanks!

Glad to hear this is a feature useful for the others too. This really has
hit as a couple of times in the past, but not anymore.

We are using this option in a single unit test, which loads all mappings,
an in Release mode we leave it off, so that default behaviour stays the
same.

On 23 October 2013 17:37, Paul notifications@github.com wrote:

Love the mapper, but we're having trouble managing mappings in a large
project, and manually searching for duplicates is nearly impossible. Tried
testing for duplicates, but apparently create map doesn't actually create
duplicates, so this will amount to some obscure bugs from time to time.

Please include this update as an option. Thanks!


Reply to this email directly or view it on GitHubhttps://github.com/AutoMapper/AutoMapper/pull/305#issuecomment-26921017
.

Darius

fduman commented Sep 28, 2015

We are also having trouble managing mappings in a large project. We are facing really annoying mapping bugs. We have 5 people in our team. Unfortunately, nobody searches whether or not there are the same type mappings in solution. Developers can write the same type mapping in different ways. Ok, some developers may not understand the Automapper usage well, but I don't know what can I do.

I have to inherit the Automapper's Profile class and override new CreateMap functions which check this situation in our profile classes.

@jbogard I wonder why you didn't add this option? What is your opinion about this matter? Are we using the Automapper in a wrong way (of course we are wrong but we are some miserable developers :) ) or what?

@fduman , I can rebase my branch to resolve conflicts if this is OK to be accepted. Unfortunately, this is being ignored.

Owner
jbogard commented Sep 29, 2015

@dariusdamalakas Are you sure this is still an issue with the 4.0/4.1 packages?

fduman commented Sep 29, 2015

@jbogard We are still using 3.3.1 in our production environment. I don't think we can replace soon to the Automapper's 4.0+ package.

I wonder what is the current behavior of the Automapper?

fduman commented Sep 29, 2015

@dariusdamalakas we really appreciate, if you can apply changes for 3.3.1

@fduman , actually, if 4.0 solves this issue, I am happy to drop this PR. @jbogard, sorry, I have glanced over release notes, and could not find whether any of the features/bug fixes do address this issue. What is the standard behaviour then when adding duplicate mapping?

Owner
jbogard commented Sep 29, 2015

@dariusdamalakas What's supposed to happen when you call CreateMap twice is that the first one creates it, the second one uses the existing map, nothing gets overwritten.

ok, this does solve the issue by ignoring the second call. This is not ideal, as this would still lead to heavy head-scratch on massive projects. Take this as an example:

# file A in assembly A
AutoMapper.Mapper.CreateMap<Book, BookViewModel>()
    .ForMember(dest => dest.Author, opts => opts.MapFrom(src => throw null));

# file B in assembly B
AutoMapper.Mapper.CreateMap<Book, BookViewModel>()
    .ForMember(dest => dest.Author, opts => opts.MapFrom(src => src.Author.Name));

Now, depending on the order how the assemblies and auto mapper initialisation modules are scanned and run, this will either work fine, or will blow up with NullReferenceException when mapping is used. The proposal (as suggested in this PR) is to throw an exception when a duplicate mapping is about to be added. This way the issue will be spotted immediately after a second mapping is added and AutoMapper is initialised.

Owner
jbogard commented Sep 29, 2015

I think as long as it defaults to not throw, it should be OK.

fduman commented Sep 29, 2015

@dariusdamalakas I think your first solution is better.

We use Automapper profiles. I inherit our profile classes from MappingProfile which is something like this:

public abstract class MappingProfile : AutoMapper.Profile
{
    public static readonly List<string> MappingErrors = new List<string>();
    protected MappingProfile()
    {

    }

    public new IMappingExpression CreateMap(Type sourceType, Type destinationType)
    {
        CheckIfAlreadyMapped(sourceType, destinationType);
        return base.CreateMap(sourceType, destinationType);
    }

    public new IMappingExpression CreateMap(Type sourceType, Type destinationType, MemberList memberList)
    {
        CheckIfAlreadyMapped(sourceType, destinationType);
        return base.CreateMap(sourceType, destinationType, memberList);
    }

    public new IMappingExpression<TSource, TDestination> CreateMap<TSource, TDestination>()
    {
        CheckIfAlreadyMapped<TSource, TDestination>();
        return base.CreateMap<TSource, TDestination>();
    }

    public new IMappingExpression<TSource, TDestination> CreateMap<TSource, TDestination>(MemberList memberList)
    {
        CheckIfAlreadyMapped<TSource, TDestination>();
        return base.CreateMap<TSource, TDestination>(memberList);
    }

    [Conditional("DEBUG")]
    private void CheckIfAlreadyMapped(Type sourceType, Type destinationType)
    {
        if (Mapper.FindTypeMapFor(sourceType, destinationType) != null)
        {
            MappingErrors.Add(string.Format("Type is already mapped in profile:{0} for source:{1} destination {2}", this.GetType().FullName, sourceType.FullName, destinationType.FullName));
            return;
            //throw new InvalidOperationException(string.Format("Type is already mapped in profile:{0} for source:{1} destination {2}", this.GetType().FullName, sourceType.FullName, destinationType.FullName));
        }
    }

    [Conditional("DEBUG")]
    private void CheckIfAlreadyMapped<TSource, TDestination>()
    {
        if (Mapper.FindTypeMapFor<TSource, TDestination>() != null)
        {
            MappingErrors.Add(string.Format("Type is already mapped in profile:{0} for source:{1} destination {2}", this.GetType().FullName, typeof(TSource).FullName, typeof(TDestination).FullName));
            return;
            //throw new InvalidOperationException(string.Format("Type is already mapped in profile:{0} for source:{1} destination {2}", this.GetType().FullName, typeof(TSource).FullName, typeof(TDestination).FullName));
        }
    }
}

So Profile classes use this CreateMap methods. I can validate in Application_Start if double mapping issue happens.

@jbogard I don't know why didn't you answer me directly? Also, I think you couldn't understand what we mean. It's not important to us that first call creates mappings and seconds do nothing. Important thing is that we don't know which one is called first. Maybe wrong one called first and destroys all system. So, there must be a general rule which is "there should be always one CreateMap call for two different types". Am I correct?

Regards.

Owner
jbogard commented Sep 29, 2015

@fduman Not really correct, the desired behavior that's been in AutoMapper for a while is if you have 2 CreateMap, the first one creates it and the second one builds on top of it. It doesn't replace it unless you explicitly wire in conflicting mapping configuration.

So in order to not break existing clients, we'd need the default behavior to simply build on top of maps, not replace them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment