-
Notifications
You must be signed in to change notification settings - Fork 334
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
Fix issue #755 - Regression in interfaces mapping introduced by #649 #756
base: development
Are you sure you want to change the base?
Conversation
@DocSvartz Please take a look and let me know if this looks okay to you. I can make a speedy release this afternoon :) |
@lofcz Please Do not Close Issue. |
@DocSvartz could you elaborate? I find it strange that if we add any props to the interface: [TestMethod]
public void MappingToInterface_VerifyReadonlyPropsInterfaceAnyPropRule()
{
SampleInterfaceClsWithProp source = new SampleInterfaceClsWithProp
{
ActivityData = new SampleActivityDataWithProp
{
Data = new SampleActivityParsedData
{
Steps = new List<string> { "A", "B", "C" }
}
}
};
SampleInterfaceClsWithProp target = source.Adapt<SampleInterfaceClsWithProp>();
target.ShouldNotBeNull();
target.ShouldSatisfyAllConditions(
() => target.ActivityData.ShouldBe(source.ActivityData)
);
}
public interface IActivityDataWithProp
{
int Test { get; set; }
}
public class SampleInterfaceClsWithProp
{
public IActivityDataWithProp? ActivityData { get; set; }
public SampleInterfaceClsWithProp()
{
}
public SampleInterfaceClsWithProp(IActivityDataWithProp data)
{
SetActivityData(data);
}
public void SetActivityData(IActivityDataWithProp data)
{
ActivityData = data;
}
}
public class SampleActivityDataWithProp : IActivityDataWithProp
{
public int Test { get; set; } = 42;
public SampleActivityParsedData Data { get; set; }
}
public class SampleActivityParsedData
{
public List<string> Steps { get; set; } = new List<string>();
} When mapping with default config we run into: // BaseAdapter.cs:
//if mapping to interface, create dynamic type implementing it
else if (arg.DestinationType.GetTypeInfo().IsInterface)
{
... And we synthesize a new type implementing just |
As far as I understand, this is done in order to create an instance class that can be filled with values from the source. |
That's right but wouldn't it be better to do something like https://github.com/lofcz/DeepCloner/blob/9abf692a58da2e76b4dac6a266f45fbd573dc621/DeepCloner.Core/Helpers/DeepClonerExprGenerator.cs instead? The current behavior is unintuitive and I can't think of a scenario where one would intentionally want to do this. Synthesizing a new type also means methods are lost. |
If you are talking about the Update scenario:
Yes, This should not happen. And in the Create scenario:
You can't know which implementation will be chosen anyway |
Yes, I mean "updating" specifically (cloning an existing object into another existing object). |
But "updating" happens exactly as you want.
I think, you want the "Creating" scenario to copy the Implementing "interface Source" Type to the Destination? But it seems that this will only work if they match. As in your example. |
That's what I mean
Hm, There is another option though. |
I even managed to make this option work. ))
|
Looks great to me. |
Fixes #755, @andrerav could you please take a look and potentially release a new pre-release version? This regression could've affected many customers (it did us at least) - but the pattern is common.
@DocSvartz CC, I want to verify this doesn't break your code (tests are passing).