AutoMapper / AutoMapper.Extensions.ExpressionMapping

MIT License
143 stars 39 forks source link

Expression.Call passed arguments with incorrect type when using MapExpression #158

Closed wbuck closed 1 year ago

wbuck commented 1 year ago

Synopsis

When attempting to convert an Expression generated from an OData query a System.InvalidOperationException is thrown:

No generic method 'Contains' on type 'System.Linq.Enumerable' is compatible with the supplied type arguments and arguments. No type arguments should be provided if the method is non-generic.

I've been able to reproduce the error below.

It happens when the Type (in this case an enum) argument for a collection needs to be mapped to a different Type. For example: List<SimpleEnum> -> List<SimpleEnumModel>

Entities


public enum SimpleEnum
{
    Value1,
    Value2,
    Value3
}

public record Entity
{
    public int Id { get; init; }
    public SimpleEnum SimpleEnum { get; init; }
}

Models


public enum SimpleEnumModel
{
    Value1,
    Value2,
    Value3
}

public record EntityModel
{
    public int Id { get; init; }
    public SimpleEnumModel SimpleEnum { get; init; }
}

Mappings

public class EntityProfile : Profile
{
    public EntityProfile()
    {
        CreateMap<EntityModel, Entity>();                            
    }
}

Expression using List<T>.Contains

List<SimpleEnum> enums = new() { SimpleEnum.Value1, SimpleEnum.Value2 };

Expression<Func<Entity, bool>> filter = e => enums.Contains(e.SimpleEnum);

// Throws here.
Expression<Func<EntityModel, bool>> mappedFilter = mapper.MapExpression<Expression<Func<EntityModel, bool>>>(filter);

Expression using Enumerable.Contains<T> extension method

List<SimpleEnum> enums = new() { SimpleEnum.Value1, SimpleEnum.Value2 };

ParameterExpression parameter = Expression.Parameter(typeof(Entity));

MemberExpression memberExpression = Expression.MakeMemberAccess
(
    parameter,
    parameter.Type.GetProperty(nameof(Entity.SimpleEnum))
);

Type elementType = enums.GetType().GenericTypeArguments[0];
MethodCallExpression callExpression = Expression.Call
(
    typeof(Enumerable),
    nameof(Enumerable.Contains),
    new Type[] { elementType },
    Expression.Constant(enums),
    memberExpression
);

Expression<Func<Entity, bool>> filter = Expression.Lambda<Func<Entity, bool>>
(
    callExpression, 
    parameter
);

// Throws here.
Expression<Func<EntityModel, bool>> mappedFilter = mapper.MapExpression<Expression<Func<EntityModel, bool>>>(filter);

The exception is getting thrown here

Cause of exception when mapping List<T>.Contains

The reason the exception is thrown in this case is because the MethodCallExpression node arguments Method property has the wrong MethodInfo information. That MethodInfo is passed to Expression.Call.

node.Method == List<SimpleEnum>.Contains this causes a problem as the ConstantExpression.Type == SimpleEnumModel.


Cause of exception when mapping Enumerable.Contains<T>

The reason the exception is thrown in this case is because the listOfArgumentsForNewMethod contains an argument with an incorrect Type .

  1. ConstantExpression.Type == List<SimpleEnum>
  2. PropertyExpression.Type == SimpleEnumModel

In this case the first argument is incorrect and should be a constant Expression with the Type == List<SimpleEnumModel>.

BlaiseD commented 1 year ago

The MyGet build should work now. Thanks for reporting.

wbuck commented 1 year ago

@BlaiseD Thanks for the quick turn around. This was the cause of the issue using the IN operator in the AutoMapper.Extensions.OData repo.

wbuck commented 1 year ago

@BlaiseD One concern I have regarding your fix is the need to create an explicit mapping between the source and destination enum types.

If I'm not mistaken Automapper does not require an explicit mapping between enum types (assuming they're compatible), take a look at Jimmy's answer here.

By using ResolveTypeMap in CanMapConstant this of course would require the user to create a explicit mapping between their enum types (which you've done in the tests).

Would it make sense to add a special case for enum types here?

First checking if the enum types are compatible (first by name and if that fails by value)?

Or just return true from CanMapConstant if the srcType and destType are enum types and let Automapper proper determine if the enum types are compatible?

It's also a bit confusing for the user because they have to create an opposite type map. E.g.,

// Mapping from model to entity.
cfg.CreateMap<EntityModel, Entity>();
// Reverse the mapping for the enum.
cfg.CreateMap<SimpleEnum, SimpleEnumModel>();

I guess I'm just a bit worried about the consequences of requiring an explicit enum mapping on the users part.

BlaiseD commented 1 year ago

You are correct - type maps are not needed for enums.