-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Value transformers in global, profiles, type maps, and members #2261
Changes from 8 commits
3a0b4fe
a98c14b
159918f
14a1c52
cfdf4f8
1b5a7be
32ec282
03c70e3
8e58d72
e54b53d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,7 @@ namespace AutoMapper.Execution | |
using System.Linq; | ||
using System.Linq.Expressions; | ||
using System.Reflection; | ||
using AutoMapper.Configuration; | ||
using Configuration; | ||
using static System.Linq.Expressions.Expression; | ||
using static Internal.ExpressionFactory; | ||
using static ExpressionBuilder; | ||
|
@@ -468,7 +468,7 @@ private Expression BuildValueResolverFunc(PropertyMap propertyMap, Expression de | |
var returnType = destinationNullable && destinationPropertyType.GetTypeOfNullable() == | ||
nullCheckedExpression.Type | ||
? destinationPropertyType | ||
: nullCheckedExpression.Type; | ||
: nullCheckedExpression.Type; | ||
valueResolverFunc = | ||
TryCatch( | ||
ToType(nullCheckedExpression, returnType), | ||
|
@@ -522,6 +522,12 @@ private Expression BuildValueResolverFunc(PropertyMap propertyMap, Expression de | |
); | ||
} | ||
|
||
valueResolverFunc = propertyMap.ValueTransformers | ||
.Concat(_typeMap.ValueTransformers) | ||
.Concat(_typeMap.Profile.ValueTransformers) | ||
.Where(vt => vt.IsMatch(propertyMap)) | ||
.Aggregate(valueResolverFunc, (current, vtConfig) => ToType(ReplaceParameters(vtConfig.TransformerExpression, ToType(current, vtConfig.ValueType)), propertyMap.DestinationPropertyType)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this change to having ValueTransformerConfiguration have function likeMapExpression? Kinda like what IObjectMapper does now where it's an interface like IValueTransformerConfiguration. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah that's a further task on the list There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wasn't going to make it like IObjectMapper though, more like IValueResolver and ITypeConverter. You can go the expression route, which can be used in LINQ etc. or the runtime route that can get access to ResolutionContext. That would also allow a DI-able version. |
||
|
||
return valueResolverFunc; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,7 +10,7 @@ namespace AutoMapper | |
/// <typeparam name="TSource">Source type for this member</typeparam> | ||
/// <typeparam name="TMember">Type for this member</typeparam> | ||
/// <typeparam name="TDestination">Destination type for this map</typeparam> | ||
public interface IMemberConfigurationExpression<TSource, out TDestination, TMember> | ||
public interface IMemberConfigurationExpression<TSource, TDestination, TMember> | ||
{ | ||
/// <summary> | ||
/// Do not precompute the execution plan for this member, just map it at runtime. | ||
|
@@ -199,6 +199,13 @@ void ResolveUsing<TValueResolver>() | |
/// The destination member being configured. | ||
/// </summary> | ||
MemberInfo DestinationMember { get; } | ||
|
||
/// <summary> | ||
/// Apply a transformation function after any resolved destination member value with the given type | ||
/// </summary> | ||
/// <typeparam name="TValue">Value type to match and transform</typeparam> | ||
/// <param name="transformer">Transformation expression</param> | ||
void AddTransform<TValue>(Expression<Func<TValue, TValue>> transformer); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe a non generic overload that just uses TMember. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know if we need an overload as much as just get rid of this version. I don't remember why I put a transform where you could specify a different type. Transformers get applied after resolving/mapping so it's always going to be of the destination member type. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After resolving and before mapping. And I think you meant for TValue to be the type of the resolved value. Which doesn't have to be TMember. Right? :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah then I wonder if I have value transformations in the right place. I think it's really before you assign it, you transform it but I have it as part of resolving. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I moved the value transformations to after mapping and right before setting. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like it. I've always thought AfterPropertyMap is a good idea :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ha well maybe this will take care of people's scenarios. |
||
} | ||
|
||
/// <summary> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
using System; | ||
using System.Collections.Generic; | ||
using System.Linq.Expressions; | ||
|
||
namespace AutoMapper | ||
{ | ||
public class ValueTransformerConfiguration | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess this could be a struct. |
||
{ | ||
public ValueTransformerConfiguration(Type valueType, LambdaExpression transformerExpression) | ||
{ | ||
ValueType = valueType; | ||
TransformerExpression = transformerExpression; | ||
} | ||
|
||
public Type ValueType { get; } | ||
public LambdaExpression TransformerExpression { get; } | ||
|
||
public bool IsMatch(PropertyMap propertyMap) | ||
{ | ||
return ValueType.IsAssignableFrom(propertyMap.DestinationPropertyType); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe the other way around? |
||
} | ||
} | ||
|
||
public static class ValueTransformerConfigurationExtensions | ||
{ | ||
/// <summary> | ||
/// Apply a transformation function after any resolved destination member value with the given type | ||
/// </summary> | ||
/// <typeparam name="TValue">Value type to match and transform</typeparam> | ||
/// <param name="valueTransformers">Value transformer list</param> | ||
/// <param name="transformer">Transformation expression</param> | ||
public static void Add<TValue>(this IList<ValueTransformerConfiguration> valueTransformers, | ||
Expression<Func<TValue, TValue>> transformer) | ||
{ | ||
var config = new ValueTransformerConfiguration(typeof(TValue), transformer); | ||
|
||
valueTransformers.Add(config); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There might be a lot of transformers here, so a cache would help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know much about transformers, but that sounded weird :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few hundred mappings, maybe a few thousand properties, it seems reasonable. For all the existing transformers. This will hurt eventually :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would we cache? The list could be different since you can define at the property map level.
Alternatively we could push the transformers down from the parent levels, kinda like we do for Include.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but not the transformers per member are the problem, all the rest are. And yes, I thought about smth like your second suggestion, but I'm not sure how that would look like, caching by type seemed more obvious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will only be an issue with large amounts of value transformers. And I'm not sure that would really happen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we can wait :)