AutoMapper / AutoMapper.Extensions.ExpressionMapping

MIT License
143 stars 39 forks source link

Added enum to int conversion for unmapped side of binary expressions #163

Closed ErikGjers closed 1 year ago

ErikGjers commented 1 year ago

Fixes #162

Was not able to do anything that would fix the issue at the ConstantExpression part of the visitor that was pointed out, but instead edited the BinaryExpression visiting part to have a conversion expression added.

Feedback appreciated.

BlaiseD commented 1 year ago

Doing it in VisitConstant makes the change in the context to the TypeMappings dictionary. Something like the following:

            if (newType != node.Type)
            {
                if (node.Value == null)
                    return base.VisitConstant(Expression.Constant(null, newType));

                if (ConfigurationProvider.CanMapConstant(node.Type, newType))
                    return base.VisitConstant(Expression.Constant(Mapper.MapObject(node.Value, node.Type, newType), newType));
                else if (BothEnumOrLiteral())//this would be the new piece
                {
                    //Expression nodeValue = node.ConvertTypeIfNecessary(newType);
                    return node.ConvertTypeIfNecessary(newType);
                }

                bool BothEnumOrLiteral()
                    => (node.Type.IsLiteralType() || node.Type.IsEnumType()) && (newType.IsLiteralType() || newType.IsEnumType());
            }

The IsEnumType covers the null case.

        public static bool IsEnumType(this Type type)
        {
            if (type.IsNullableType())
                type = Nullable.GetUnderlyingType(type);

            return type.IsEnum();
        }

Your method works but to me the purpose is less clear. ConvertTypeIfNecessary removes existing conversions first.

ErikGjers commented 1 year ago

The changes I made to the PR cover the requirements I believe. What you had written worked. Just dropping a message here in case the change went unnoticed. Feedback is always appreciated.

BlaiseD commented 1 year ago

Not unnoticed - will post further feedback - or just add changes. Maybe the logic will have to be more targeted than BothEnumOrLiteral(). Enums do not always derive form Int32.

        public enum MyEnum : short
        {
            Value1 = 1,
            Value2 = 2,
            Value3 = 3
        }

Also the tests should include:

Need to look closer before merging.

ErikGjers commented 1 year ago

I'll add some tests and potential changes in the next couple of days.

ErikGjers commented 1 year ago

I tried to include some string -> enum and enum -> string conversions, but these were failing. Since they weren't specifically part of this issue, though I figured it would be nice to have, I removed them for the time being.

Should I expand further on these tests, or do they cover the needs for the feature?

Sorry for the delay, travel and work got in the way.

BlaiseD commented 1 year ago

No hurry - correct that string conversions don't apply here.