fix: generic and runtime target type mappings with derived types#2230
fix: generic and runtime target type mappings with derived types#2230
Conversation
a7e108c to
32ee630
Compare
| // If the mapping is a derived type mapping, the runtime target type is the source of the IsAssignableFrom check, otherwise it is the runtime target type. | ||
| // For example, when we map to ADto which is a BaseDto, it is typeof(BaseDto).IsAssignableFrom(typeof(ADto)). |
There was a problem hiding this comment.
I don't fully get this comment… either it is the runtime target type otherwise it is the runtime target type too?
| .Select(x => new RuntimeTargetTypeMapping( | ||
| x, | ||
| ctx.Compilation.HasImplicitConversion(x.TargetType, ctx.Target), | ||
| (x as UserDefinedNewInstanceMethodMapping)?.IsDerivedTypeMapping == true |
There was a problem hiding this comment.
Prefer pattern matching x is UserDefinedNewInstanceMethodMapping { IsDerivedTypeMapping: true }
| @@ -92,9 +92,13 @@ public override IEnumerable<StatementSyntax> BuildBody(TypeMappingBuildContext c | |||
| protected virtual ExpressionSyntax? BuildSwitchArmWhenClause(ExpressionSyntax runtimeTargetType, RuntimeTargetTypeMapping mapping) | |||
| { | |||
| // targetType.IsAssignableFrom(typeof(ADto)) | |||
There was a problem hiding this comment.
This comment isn't 100% accurate anymore, is it?
| MemberAccess(mapping.IsDerivedTypeMapping ? mappingTargetType : runtimeTargetType, IsAssignableFromMethodName), | ||
| mapping.IsDerivedTypeMapping ? runtimeTargetType : mappingTargetType |
There was a problem hiding this comment.
This probably is a breaking change, isn't it? Document it in the breaking change docs.
| return source switch | ||
| { | ||
| global::Base x when targetType.IsAssignableFrom(typeof(global::BaseDto)) => MapDerivedTypes(x), | ||
| global::Base x when typeof(global::BaseDto).IsAssignableFrom(targetType) => MapDerivedTypes(x), |
There was a problem hiding this comment.
Create a new test case reflecting the exact use-case from the referenced issue.
| [MapDerivedType<A, B>] | ||
| [MapDerivedType<C, D>] |
There was a problem hiding this comment.
Add the exact same test but with [MapDerivedType(typeof(A), typeof(B))] instead of the generic one. I don't think it works as expected.
| ctx.SymbolAccessor.UpgradeNullable(methodSymbol.ReturnType), | ||
| ctx.Configuration.Mapper.UseReferenceHandling | ||
| ctx.Configuration.Mapper.UseReferenceHandling, | ||
| ctx.AttributeAccessor.HasAttribute<MapDerivedTypeAttribute<object, object>>(methodSymbol) |
There was a problem hiding this comment.
IMO the correct location to set this would be DerivedTypeMappingBuilder…
|
Example for clarification: Lets assume we have BananaDto : FruitDto, and FruitDto : OrganicDto. The previously released code in public partial class DerivedTypeMapper
{
[global::System.CodeDom.Compiler.GeneratedCode("Riok.Mapperly", "4.3.1.0")]
public partial T MapGeneric<T>(object source)
{
return source switch
{
// --- Relevant Part ---
global::MapperlyRepro.Fruit x when typeof(T).IsAssignableFrom(typeof(global::MapperlyRepro.FruitDto)) => (T)(object)MapDerivedTypes(x),
_ => throw new global::System.ArgumentException($"Cannot map {source.GetType()} to {typeof(T)} as there is no known type mapping", nameof(source)),
};
}
[global::System.CodeDom.Compiler.GeneratedCode("Riok.Mapperly", "4.3.1.0")]
public partial global::MapperlyRepro.FruitDto MapDerivedTypes(global::MapperlyRepro.Fruit source)
{
return source switch
{
global::MapperlyRepro.Banana x => MapToBananaDto(x),
_ => throw new global::System.ArgumentException($"Cannot map {source.GetType()} to MapperlyRepro.FruitDto as there is no known derived type mapping", nameof(source)),
};
}
[global::System.CodeDom.Compiler.GeneratedCode("Riok.Mapperly", "4.3.1.0")]
private global::MapperlyRepro.BananaDto MapToBananaDto(global::MapperlyRepro.Banana source)
{
...
}
}by [Mapper]
public partial class DerivedTypeMapper
{
public partial T MapGeneric<T>(object source);
[MapDerivedType<Banana, BananaDto>]
public partial FruitDto MapDerivedTypes(Fruit source);
}So previously, the mapping would work for:
If we reverse the AssignableFrom argument order (like currently in this PR), we would reverse the order of what would work:
So previously, if someone called |
Fix generic and runtime target type mappings with derived types
Description
If a user defined generic type or runtime target type mapping has a child mapping method which uses the
[MapDerivedType]attribute, the parent method switch arm when clause was created wrongly (but only in that specific case). Fixes #1989This got fixed in this PR
Fixes # (issue)
Checklist