Automapper 5.2 ignores ExplicitExpansion if it is configured in Base DTO mapping
Asked Answered
C

1

9

Automapper 5.2 (latest by the moment) ignores ExplicitExpansion() configuration if it is configured in the mapping of Base Data Transfer Object. But it still works correctly if mapping is configured directly in Derived DTO. I've got a pair of DTO classes that contain so many duplications in field set and mapping configuration that I am trying to isolate it to common base DTO class, but this issue prevents me from doing it.

Below is the code that illustrates this weird behavior. There are four tests, two of them fail on asserting not expanded property of base DTO. If I move the lines 1-1..1-4 to place 2.1, all the tests pass.

Have I missed some piece of code or is this a bug in Automapper and I have to report this issue to Automapper's bug tracker? Or is it probably "by design", but why? (Ivan Stoev has suggested a working fix, but let me please postpone accepting the answer, because the issue I am facing is not so simple and I added more details in update below).

UnitTest1.cs:

using System.Collections.Generic;
using System.Linq;
using AutoMapper;
using AutoMapper.QueryableExtensions;
using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace AutoMapperIssue 
{
    public class Source { public string Name; public string Desc; }
    public class DtoBase              { public string Name { get; set; } }
    public class DtoDerived : DtoBase { public string Desc { get; set; } }
    [TestClass] public class UnitTest1
    {
        [AssemblyInitialize] public static void AssemblyInit(TestContext context)
        {
            Mapper.Initialize(cfg =>
            {
                cfg.CreateMap<Source, DtoBase>()
                    .ForMember(dto => dto.Name, conf => { // line 1-1
                        conf.MapFrom(src => src.Name);    // line 1-2
                        conf.ExplicitExpansion();         // line 1-3
                    })                                    // line 1-4
                    .Include<Source, DtoDerived>();
                cfg.CreateMap<Source, DtoDerived>()
                    // place 2.1
                    .ForMember(dto => dto.Desc, conf => {
                        conf.MapFrom(src => src.Desc);
                        conf.ExplicitExpansion();
                    });
            });
            Mapper.Configuration.CompileMappings();
            Mapper.AssertConfigurationIsValid();
        }

        private readonly IQueryable<Source> _iq = new List<Source> {
            new Source() { Name = "Name1", Desc = "Descr",},
        } .AsQueryable();

        [TestMethod] public void ProjectAll_Success() 
        {
            var projectTo = _iq.ProjectTo<DtoDerived>(_ => _.Name, _ => _.Desc);
            Assert.AreEqual(1, projectTo.Count()); var first = projectTo.First();
            Assert.IsNotNull(first.Desc); Assert.AreEqual("Descr", first.Desc);
            Assert.IsNotNull(first.Name); Assert.AreEqual("Name1", first.Name);
        }
        [TestMethod] public void SkipDerived_Success() 
        {
            var projectTo = _iq.ProjectTo<DtoDerived>(_ => _.Name);
            Assert.AreEqual(1, projectTo.Count()); var first = projectTo.First();
            Assert.IsNotNull(first.Name); Assert.AreEqual("Name1", first.Name);
            Assert.IsNull(first.Desc, "Should not be expanded.");
        }
        [TestMethod] public void SkipBase_Fail() 
        {
            var projectTo = _iq.ProjectTo<DtoDerived>(_ => _.Desc);
            Assert.AreEqual(1, projectTo.Count()); var first = projectTo.First();
            Assert.IsNotNull(first.Desc); Assert.AreEqual("Descr", first.Desc);
            Assert.IsNull(first.Name, "Should not be expanded. Fails here. Why?");
        }
        [TestMethod] public void SkipAll_Fail() 
        {
            var projectTo = _iq.ProjectTo<DtoDerived>();
            Assert.AreEqual(1, projectTo.Count()); var first = projectTo.First();
            Assert.IsNull(first.Desc, "Should not be expanded.");
            Assert.IsNull(first.Name, "Should not be expanded. Fails here. Why?");
        }
    }
}

packages.config:

<package id="AutoMapper" version="5.2.0" targetFramework="net452" />

UPD. Ivan Stoev has comprehensively answered how to fix the issue coded above. It works pretty well unless I am forced to use string arrays of field names instead of MemberExpressions. This is related to the fact that this approach crashes with members of Value type (Such as int, int?). It is demonstrated in the first unit test below along with the crash stack trace. I'll ask about it in another question, or rather create an issue in the bug tracker since crash is definitely a bug.

UnitTest2.cs - with fix from Ivan Stoev's answer

using System;
using System.Collections.Generic;
using System.Linq;
using AutoMapper;
using AutoMapper.QueryableExtensions;
using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace AutoMapperIssue.StringPropertyNames 
{   /* int? (or any ValueType) instead of string - .ProjectTo<> crashes on using MemberExpressions in projction */
    using NameSourceType = Nullable<int> /* String */; using NameDtoType = Nullable<int> /* String */;
    using DescSourceType = Nullable<int> /* String */; using DescDtoType = Nullable<int> /* String*/;

    public class Source
    {   
        public NameSourceType Name { get; set; }
        public DescSourceType Desc { get; set; }
    }

    public class DtoBase              { public NameDtoType Name { get; set; } }
    public class DtoDerived : DtoBase { public DescDtoType Desc { get; set; } }

    static class MyMappers
    {
        public static IMappingExpression<TSource, TDestination> Configure<TSource, TDestination>(this IMappingExpression<TSource, TDestination> target)
            where TSource : Source
            where TDestination : DtoBase
        {
            return target.ForMember(dto => dto.Name, conf =>
                {
                    conf.MapFrom(src => src.Name);
                    conf.ExplicitExpansion();
                });
        }
    }

    [TestClass] public class UnitTest2
    {
        [ClassInitialize] public static void ClassInit(TestContext context)
        {
            Mapper.Initialize(cfg =>
            {
                cfg.CreateMap<Source, DtoBase>()
                    .Configure()
                    .Include<Source, DtoDerived>();
                cfg.CreateMap<Source, DtoDerived>()
                    .Configure()
                    .ForMember(dto => dto.Desc, conf => {
                        conf.MapFrom(src => src.Desc);
                        conf.ExplicitExpansion();
                    })
                ;
            });
            Mapper.Configuration.CompileMappings();
            Mapper.AssertConfigurationIsValid();
        }

        private static readonly IQueryable<Source> _iq = new List<Source> {
            new Source() { Name = -25 /* "Name1" */, Desc = -12 /* "Descr" */, },
        } .AsQueryable();

        private static readonly Source _iqf = _iq.First();

        [TestMethod] public void ProjectAllWithMemberExpression_Exception() 
        {
            _iq.ProjectTo<DtoDerived>(_ => _.Name, _ => _.Desc); // Exception here, no way to use Expressions with current release
//Test method AutoMapperIssue.StringPropertyNames.UnitTest2.ProjectAllWithMemberExpression_Exception threw exception: 
//System.NullReferenceException: Object reference not set to an instance of an object.
//
//    at System.Linq.Enumerable.<SelectManyIterator>d__16`2.MoveNext()
//   at System.Linq.Enumerable.<DistinctIterator>d__63`1.MoveNext()
//   at System.Linq.Buffer`1..ctor(IEnumerable`1 source)
//   at System.Linq.Enumerable.ToArray[TSource](IEnumerable`1 source)
//   at AutoMapper.QueryableExtensions.ProjectionExpression.To[TResult](IDictionary`2 parameters, IEnumerable`1 memberPathsToExpand)
//   at AutoMapper.QueryableExtensions.ProjectionExpression.To[TResult](Object parameters, Expression`1[] membersToExpand)
//   at AutoMapper.QueryableExtensions.Extensions.ProjectTo[TDestination](IQueryable source, IConfigurationProvider configuration, Object parameters, Expression`1[] membersToExpand)
//   at AutoMapper.QueryableExtensions.Extensions.ProjectTo[TDestination](IQueryable source, Expression`1[] membersToExpand)
//   at AutoMapperIssue.StringPropertyNames.UnitTest2.ProjectAllWithMemberExpression_Exception() in D:\01\AutoMapperIssue\UnitTest2.cs:line 84
        }
#pragma warning disable 649
        private DtoDerived d;
#pragma warning restore 649
        [TestMethod] public void ProjectAll_Fail() 
        {
            var projectTo = _iq.ProjectTo<DtoDerived>(null, new string[] { nameof(d.Name), nameof(d.Desc) } /* _ => _.Name, _ => _.Desc */);
            Assert.AreEqual(1, projectTo.Count()); var first = projectTo.First();
            Assert.IsNotNull(first.Desc, "Should be expanded.");                  Assert.AreEqual(_iqf.Desc, first.Desc);
            Assert.IsNotNull(first.Name, "Should be expanded. Fails here, why?"); Assert.AreEqual(_iqf.Name, first.Name);
        }
        [TestMethod] public void BaseOnly_Fail() 
        {
            var projectTo = _iq.ProjectTo<DtoDerived>(null, new string[] { nameof(d.Name) } /* _ => _.Name */);
            Assert.AreEqual(1, projectTo.Count()); var first = projectTo.First();
            Assert.IsNull(first.Desc, "Should NOT be expanded.");
            Assert.IsNotNull(first.Name, "Should be expanded. Fails here, why?"); Assert.AreEqual(_iqf.Name, first.Name);

        }
        [TestMethod] public void DerivedOnly_Success() 
        {
            var projectTo = _iq.ProjectTo<DtoDerived>(null, new string[] { nameof(d.Desc) } /* _ => _.Desc */);
            Assert.AreEqual(1, projectTo.Count()); var first = projectTo.First();
            Assert.IsNotNull(first.Desc, "Should be expanded."); Assert.AreEqual(_iqf.Desc, first.Desc);
            Assert.IsNull(first.Name, "Should NOT be expanded.");
        }
        [TestMethod] public void SkipAll_Success() 
        {
            var projectTo = _iq.ProjectTo<DtoDerived>(null, new string[] { });
            Assert.AreEqual(1, projectTo.Count()); var first = projectTo.First();
            Assert.IsNull(first.Desc, "Should NOT be expanded.");
            Assert.IsNull(first.Name, "Should NOT be expanded.");
        }
    }
}

UPD2. The updated issue above definitely cannot be fixed outside, see the comment under the accepted answer. This is an issue of AutoMapper itself. If you can't wait to fix the updated issue, you can make your patch of AutoMapper using the following simple (but not minor) diffs: https://github.com/moudrick/AutoMapper/commit/65005429609bb568a9373d7f3ae0a535833a1729

Carolyn answered 30/1, 2017 at 21:36 Comment(0)
K
5

Have I missed some piece of code

You haven't missed anything.

or is this a bug in Automapper and I have to report this issue to Automapper's bug tracker? Or is it probably "by design", but why?

I doubt it's "by design", most likely is bug or incomplete quick-and-dirty implementation. It can be seen inside the source code of the ApplyInheritedPropertyMap method of the PropertyMap class, which is responsible for combining the base property and derived property configurations. The "inherited" mapping properties currently are:

  • CustomExpression
  • CustomResolver
  • Condition
  • PreCondition
  • NullSubstitute
  • MappingOrder
  • ValueResolverConfig

while the following (basically all bool type) properties (including the one in question) are not:

  • AllowNull
  • UseDestinationValue
  • ExplicitExpansion

The problem IMO is that the current implementation cannot determine whether a bool property is being set explicitly or not. Of course it can easily be resolved by replacing the auto properties with explicit bool? backing field and default value logic (and additional fluent configuration method to turn it off in case it's turned on inside the base class configuration). Unfortunately it can be done only in source code, so I would suggest you reporting the issue to their issue tracker.

Until (and if) they fix it, I could suggest as a workaround moving all the common code to a custom extension methods like

static class MyMappers
{
    public static IMappingExpression<TSource, TDestination> Configure<TSource, TDestination>(this IMappingExpression<TSource, TDestination> target)
        where TSource : Source
        where TDestination : DtoBase
    {
        return target
            .ForMember(dto => dto.Name, conf =>
            {
                conf.MapFrom(src => src.Name);
                conf.ExplicitExpansion();
            });
    }
}

and use them from the main configuration code:

Mapper.Initialize(cfg =>
{
    cfg.CreateMap<Source, DtoBase>()
        .Configure();

    cfg.CreateMap<Source, DtoDerived>()
        .Configure()
        .ForMember(dto => dto.Desc, conf => {
            conf.MapFrom(src => src.Desc);
            conf.ExplicitExpansion();
        });
});

Edit: Regarding the additional issues. Both are more serious AM processing bugs, unrelated to configuration.

The problem is that they try to use MemberInfo instance comparison for filtering the projection.

The first case (with expressions) fails for value types because the implementation which tries to extract MemberInfo from Expression<Func<T, object>> expects only MemberExpression, but in case of value types it's wrapped inside a Expression.Convert.

The second case (with property names) fails because they don't consider the fact that MemberInfo for a property inherited from a base class extracted from compile time lambda expression is different from the same retrieved by reflection or runtime created expression, which is demonstrated with the following test:

// From reflection
var nameA = typeof(DtoDerived).GetMember(nameof(DtoDerived.Name)).Single();
// Same as
//var nameA = typeof(DtoDerived).GetProperty(nameof(DtoDerived.Name));

// From compile time expression
Expression<Func<DtoDerived, NameDtoType>> compileTimeExpr = _ => _.Name;
var nameB = ((MemberExpression)compileTimeExpr.Body).Member;

// From runtime expression
var runTimeExpr = Expression.PropertyOrField(Expression.Parameter(typeof(DtoDerived)), nameof(DtoDerived.Name));
var nameC = runTimeExpr.Member;

Assert.AreEqual(nameA, nameC); // Success
Assert.AreEqual(nameA, nameB); // Fail

You definitely need to report both issues. I would say the feature is compromised for any value type property when supplying expression list, and for any inherited property when supplying names.

Kenzie answered 2/2, 2017 at 21:6 Comment(14)
Ivan, thanks for comprehensive answer! I am ready to accept your answer just before the bounty period ends if no one answered the updated issue, but let me please leave the bounty open hoping to obtain a resolution to the actual issue I am facing. Also, could you consider answering the updated issue?Carolyn
Sure, I'll take a look at it.Kenzie
@Carolyn Unfortunately the additional issues cannot be resolved from outside. Actually the one with expressions can easily be solved, but would require using different method name than ProjectTo in order to not clash with the AM QueryableExtensions. Let me know if you are interested. The sad thing is that at the source code level the fix is quite easy.Kenzie
thanks, I think, I have already coded this fix, see it in my fork of Automapper. I'll add a unit test / issue report for this and do a pull-request to main repository once I have time. See github.com/moudrick/AutoMapper/commit/… Anyway, in our project we are considering usage of a locally patched version. Just not sure if tag 5.2.0 means that release was done from the commit with this tag. Do you have any information on this?Carolyn
Just saw it. Cool. That's exactly what I meant! :) About release, I have no clue, sorry.Kenzie
Definitely, I am interested in more details on using other that ProjectTo method. But to be honest, I cannot understand the hints on it in your comment. Please give more information so as to consider this approach. In addition, I can say that we use ProjectTo for mapping to further projecting to database model and SQL instructions. We use ExplicitExpansion to avoid additional joins in flattening fields that are not requested from higher layer. So can accept the approach you suggest only if it allows such kind of projections.Carolyn
You misunderstood me. The idea was to provide separate set of methods with the exactly the same signature as ProjectTo, but with different name just to fix the Expression.Convert issue. Once you fix it inside the source code, the above makes no sense.Kenzie
Btw, once you started fixing the AM source, you could easily fix the original issue also. Just inside the PropertyMap class, replace public bool ExplicitExpansion { get; set; } with private bool? explicitExpansion; public bool ExplicitExpansion { get { return explicitExpansion.GetValueOrDefault(); } set { explicitExpansion = value; } }, add explicitExpansion = explicitExpansion ?? inheritedMappedProperty.explicitExpansion; at the end of the ApplyInheritedPropertyMap and the problem is gone, no need of my workaround :)Kenzie
Hehe, this means more testcases, more reports, more effort, more time spent :) Unless I reach downto "If I get bored" priority level. I'd prefer keeping codebase as close to official release as possible. Anyway, thanks for suggestion.Carolyn
First issue: github.com/AutoMapper/AutoMapper/issues/1956 First PR: github.com/AutoMapper/AutoMapper/pull/1957 preparing the secondCarolyn
Second issue: github.com/AutoMapper/AutoMapper/issues/1958 Second PR: github.com/AutoMapper/AutoMapper/issues/1958Carolyn
Both PRs include Fixie [Fact]s and both are passed CI automated tests and still awaiting for (manual?) review from those who can write to repo. Ivan, can you? On the releases, in the discussion official mailing list they say that the releases are quarterly and that the previous release 5.2.0: Nov 23 (2016). Thus I hurried to send PR hoping that AM team accepts them before the release. It is pretty much possible it will be even this month. groups.google.com/forum/#!topic/automapper-users/3HRJRITDvAMCarolyn
@Carolyn Hi, first congrats for spending time collaborating with AM source. What I about me, I'm afraid I never had time to read open source projects rules, hence to elaborate - all I do is to browse the code to find issues like these.Kenzie
first issue PR has been discussed, improved and eventually merged to master. The second is still being discussed, you can open it and add your thoughts there, anyway I mentioned there your coding offer above for nullable backing field.Carolyn

© 2022 - 2024 — McMap. All rights reserved.