Condition when mapping from Nullable<T> to T

[MarkSFrancis]

When using a condition to check the value of a member of type Nullable before mapping to a destination member with a type T, the value is converted to T before Condition is checked, and so checking for null will not work correctly.

Source/destination types

class Source
{
    public int? Value { get; set; }
}

class Destination
{
    public int Value { get; set; }
}

Mapping configuration

cfg.CreateMap<Source, Destination>().ForAllMembers(o => o.Condition((source, destination, sourceProperty, destinationProperty) =>
{
    sourceProperty.ShouldBeNull(); // Fails as it's already converted int? to int before it reaches this, and so the value is 0
    
    // Always true
    return sourceProperty != null;
}));

Version: x.y.z

8.1.0

Expected behavior

Type of sourceProperty should be Nullable<Int32>

Actual behavior

Type of sourceProperty is Int32

Steps to reproduce

public class when_mapping_from_nullable_clr_member : AutoMapperSpecBase
{
    Source _source = new Source { Value = null };
    Destination _destination = new Destination { Value = 7 };

    class Source
    {
        public int? Value { get; set; }
    }

    class Destination
    {
        public int Value { get; set; }
    }

    protected override MapperConfiguration Configuration => new MapperConfiguration(cfg =>
    {
        cfg.CreateMap<Source, Destination>().ForAllMembers(o => o.Condition((source, destination, sourceProperty, destinationProperty) =>
        {
            sourceProperty.ShouldBeNull();

            // Always true
            return sourceProperty != null;
        }));
    });

    protected override void Because_of()
    {
        Mapper.Map(_source, _destination);
    }
}

[lbargaoanu]

Yes, and there are known workarounds. You'll have to use whatever works best in your case.

[MarkSFrancis]

@lbargaoanu Can you link some of these workarounds? I can't find anything in the docs mentioning them, and the most obvious issue I could find was #2918, which just says that documentation is needed (but again - still doesn't say what those workarounds are). Although this issue is closed, I can't find the related docs.
Sorry if I'm just being block-headed and unable to see something obvious :/

[MarkSFrancis]

On another note, is this something that I could help resolve? I'm happy to add the test for it and then try and fix this issue, submitting a PR if I'm successful.
Would this be something that would be helpful for you?

[lbargaoanu]

This is not exactly a recent thing, so you'll have to search a little more. A PR might help. But for that, you would have to really understand the issue first and come up with a proposal.

[MarkSFrancis]

@lbargaoanu The issue seems to be the in TypeMapPlanBuilder.CreatePropertyMapFunc where, when creating the memberMap condition, it has already converted the property value to the value resolver expression type.

Proposal

(Let me know if this isn't the right place for a proposal)

My proposal is to switch the memberMap.Condition.ConvertReplaceParameters's source type from propertyValue to resolvedValue depending on the Condition types. This should stop this implicit cast when it's not wanted, without introducing a breaking change.

Current code

if (memberMap.Condition != null)
    mapperExpr = IfThen(
        memberMap.Condition.ConvertReplaceParameters(
            source,
            _destination,
            ToType(propertyValue, memberMap.Condition.Parameters[2].Type),
            ToType(getter, memberMap.Condition.Parameters[2].Type),
            Context
        ),
        mapperExpr
    );

After the change

if (memberMap.Condition != null)
{
    Expression conditionExpression;
    if (propertyValue.Type == memberMap.Condition.Parameters[2].Type)
    {
        // Use implied value from propertyValue
        conditionExpression = propertyValue;
    }
    else
    {
        // Use value mapped from
        conditionExpression = resolvedValue;
    }

    mapperExpr = IfThen(
        memberMap.Condition.ConvertReplaceParameters(
            source,
            _destination,
            ToType(conditionExpression, memberMap.Condition.Parameters[2].Type),
            ToType(getter, memberMap.Condition.Parameters[2].Type),
            Context
        ),
        mapperExpr
    );
}

Some Condition calls may have relied on this auto-cast, such as the EnumMapperTest.Mapper_respects_condition test. The Condition takes a different type to what the MapFrom function is returning. This causes issues if I simply use the resolvedValue instead of propertyValue - this Condition relies on the exact implicit cast we're trying to stop.
In order to support both scenarios, I check the input that the condition wants. If it asks for the implied type, I use the implied mapping. If it doesn't, I use the original value without it being implied. This ensures that this change is not a breaking one.

New unit tests

public class when_mapping_from_nullable_struct_member : AutoMapperSpecBase
{
    Source _source = new Source { Value = null };
    Destination _destination = new Destination { Value = 7 };

    class Source
    {
        public int? Value { get; set; }
    }

    class Destination
    {
        public int Value { get; set; }
    }

    protected override MapperConfiguration Configuration => new MapperConfiguration(cfg =>
    {
        cfg.CreateMap<Source, Destination>().ForAllMembers(o => o.Condition((source, destination, sourceProperty, destinationProperty) =>
        {
            sourceProperty.ShouldBeNull();

            // Always true
            return sourceProperty != null;
        }));
    });

    protected override void Because_of()
    {
        Mapper.Map(_source, _destination);
    }
}

If you're interested in taking a look, I've already made the change in my fork
https://github.com/MarkSFrancis/AutoMapper/commits/master

[lbargaoanu]

Your test should fail on master and pass on your branch. So the same condition receives a different value on your branch. That's a breaking change.

[MarkSFrancis]

Yes, you're right sorry,
A condition with a Nullable parameter when mapping to a non-nullable T will now be able to receive a null value, whereas before it couldn't, thus being a breaking change

[MarkSFrancis]

@lbargaoanu
Would you like me to submit a PR for this change? If so - should I target master branch?

[lbargaoanu]

I don't think that's a good idea. It seems a bit arbitrary and a breaking change. To be honest I'm not sure how an acceptable solution would look like.

[MarkSFrancis]

No problem.
In case anyone else comes across this issue, I've created a workaround based on this comment (updated for version 8.x)

Resolver

public class Resolver : IMemberValueResolver<object, object, object, object>
{
    public object Resolve(object source, object destination, object sourceMember, object destMember, ResolutionContext context)
    {
        return sourceMember ?? destMember;
    }
}

Sample usage

internal class Program
{
    private static void Main(string[] args)
    {
        Mapper.Initialize(cfg =>
        {
            cfg.ForAllPropertyMaps(
                pm => true, 
                (pm, c) =>
                    c.MapFrom<Resolver, object>(pm.SourceMember.Name));

            cfg.CreateMap<Source, Destination>();
        });

        var result = new Destination { StatusId = 9 };

        // Output: 9
        Console.WriteLine(result.StatusId);

        var source = new Source { StatusId = 0 };

        // Maps status ID onto result
        Mapper.Map(source, result);

        // Output: 0
        Console.WriteLine(result.StatusId);

        // Reset result.StatusId
        result = new Destination { StatusId = 7 };

        // Output: 7
        Console.WriteLine(result.StatusId);

        source = new Source { StatusId = null };

        // Will not map the status ID because the source is null, leaving the destination as 7
        Mapper.Map(source, result);

        // Output: 7
        Console.WriteLine(result.StatusId);

        Console.ReadLine();
    }
}

public class Source
{
    public int? StatusId { get; set; }
}

public class Destination
{
    public int StatusId { get; set; }
}

[tchuat]

@MarkSFrancis I having similar problem but in reverse way. I had my Dto as nullable. I try to implement your solution in version 8 and I having issue when it try to map the inner object.

    public class InnerObject
    {
        public string Name { get; set; }
    }
    public class SourceObject
    {      
        public InnerObject InnerObject { get; set; }
    }
    public class ModelObject
    {       
        public string InnerObjectName { get; set; }
    }

I had modify the code combine from #1703 (comment)
not sure I did it correctly or not as I added a check before resolve

Appreciate anyone can help. I also post my question here https://stackoverflow.com/questions/55137405/how-to-set-automapper-ignore-null

My final code as below, seem working... but not sure will cause uncertain mapping or not.

public static class AutoMapperExtensions
    {
        class Resolver : IMemberValueResolver<object, object, object, object>
        {
            public object Resolve(object source, object destination, object sourceMember, object destinationMember, ResolutionContext context)
            {
                return sourceMember ?? destinationMember;
            }
        }

        public static void IgnoreSourceWhenNull(this IMapperConfigurationExpression cfg)
        {
            cfg.ForAllPropertyMaps(pm =>
            {
                if (pm.SourceMember != null && (pm.DestinationMember.Name != pm.SourceMember.Name))
                    return false;
                if (pm.SourceType == null)
                    return false;
                var isNullable = pm.SourceType.IsGenericType && (pm.SourceType.GetGenericTypeDefinition() == typeof(Nullable<>));
                return isNullable || pm.SourceType.IsValueType || pm.SourceType.IsPrimitive;
            }, (pm, c) =>
            {       
                    c.MapFrom<Resolver, object>(pm.SourceMember.Name);
            });
             
        }
    }

[MarkSFrancis]

@tchuat I've replied to your Stack Overflow question.
That isNullable check can be removed. If the item cannot be null, then the resolver will always return the assigned value instead of the existing one, as if the resolver had been skipped.
In fact, I'm fairly sure the conditional function can be removed as a whole to only return true.

[tchuat]

@MarkSFrancis If I set all to true, I will be hitting exception on inner object mapping.
I wish to set conditional mapping for reservermap and auto ignore source if null. And also when the source is nullable it shouldn't use the default value.

If can i wish to set all to use default resolver expect nullable type.

Thanks.