AutoMapper / AutoMapper.Extensions.ExpressionMapping

MIT License
143 stars 39 forks source link

Fix exceptions with generic source types #91

Closed mycroes closed 4 years ago

mycroes commented 4 years ago

Today I ran into issues when mapping generic source types with UseAsDataSource(). I noticed ExpressionMapper.VisitAllParametersExpression was substituting the source type for it's generic argument when trying to find a match, which of course breaks the matching.

I changed the method to only take the generic type argument if the type implements IEnumerable<>, because I'm assuming what this logic is for. All tests still pass, so I'm assuming all is still fine. I also added a test which breaks without this change.

mycroes commented 4 years ago

Seems to me CI build is failing due to misconfiguration, hope you can look into this PR shortly.

BlaiseD commented 4 years ago

Never mind the build failure - probably because the myget keys are only accessible the AutoMapper repositories.

mycroes commented 4 years ago

Hi @BlaiseD, I must admit I didn't check your suggestion before I implemented mine (which basically ensures that the generic argument of the IEnumerable<> implementation is used), but following your suggestions results in 27 failed tests (all across the board).

mycroes commented 4 years ago

Sorry, that's incorrect, not sure what I messed up, but 3 other tests fail with your suggestion:

BlaiseD commented 4 years ago

Worked for me. With the code from your branch, the only changes were:

        public static bool IsEnumerableType(this Type type)
            => type.IsGenericType && typeof(System.Collections.IEnumerable).IsAssignableFrom(type);

        //public static bool IsEnumerableType(this Type type, out Type itemType)
        //{
        //    itemType = type.GetInterfaces()
        //        .FirstOrDefault(t => t.GetGenericTypeDefinitionIfGeneric() == typeof(IEnumerable<>))
        //        ?.GetGenericArguments()[0];

        //    return itemType != null;
        //}

and

                    let a = destParamType.IsEnumerableType() ? destParamType.GetGenericElementType() : destParamType
                    //let a = destParamType.IsEnumerableType(out var itemType) ? itemType : destParamType

Or what am I missing.

mycroes commented 4 years ago

My bad, messed up big time 😉 I used GetElementType() instead of GetGenericElementType(), which then results in the 3 failed cases... Applied suggested changes.

mycroes commented 4 years ago

Hi @BlaiseD, is there any timeline on a next release? We're currently using a local build to work around the issue fixed with this PR, but we'd prefer to use the upstream release again.

BlaiseD commented 4 years ago

I'm sure it's part of 4.0.2-preview-2..

You can compare releases here.

mycroes commented 4 years ago

Thanks, I didn't notice there was a preview release. I'm really leaving a good impression of my analytic skills here here 😉

BlaiseD commented 4 years ago

I'd usually leave a note in the issue or PR - didn't this time. BTW there's also a MyGet package which gets the CI build.