Skip to content
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

Open
wants to merge 1 commit into
base: development
Choose a base branch
from

Conversation

lofcz
Copy link

@lofcz lofcz commented Jan 7, 2025

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).

@andrerav
Copy link
Contributor

andrerav commented Jan 7, 2025

@DocSvartz Please take a look and let me know if this looks okay to you. I can make a speedy release this afternoon :)

@andrerav andrerav changed the title fix regression in interfaces mapping Fix regression in interfaces mapping introduced by #649 Jan 7, 2025
@andrerav andrerav changed the title Fix regression in interfaces mapping introduced by #649 Fix issue #755 - Regression in interfaces mapping introduced by #649 Jan 7, 2025
@DocSvartz
Copy link

@lofcz
Your solution is suitable as a temporary one.
But there are still very strange things happening there :)

Please Do not Close Issue.

@lofcz
Copy link
Author

lofcz commented Jan 7, 2025

@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 Test.

@DocSvartz
Copy link

As far as I understand, this is done in order to create an instance class that can be filled with values ​​from the source.
because it is impossible to create an interface instance

@lofcz
Copy link
Author

lofcz commented Jan 7, 2025

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.

@DocSvartz
Copy link

If you are talking about the Update scenario:

var t = sourse.Adapt(destination)

Yes, This should not happen.

And in the Create scenario:

var s = sourse.Adapt<IDestination>()

You can't know which implementation will be chosen anyway

@lofcz
Copy link
Author

lofcz commented Jan 7, 2025

Yes, I mean "updating" specifically (cloning an existing object into another existing object).

@DocSvartz
Copy link

DocSvartz commented Jan 7, 2025

But "updating" happens exactly as you want.

[TestMethod]
public void MappingToInteraceWithIgnorePrivateSetProperty()
{
    TypeAdapterConfig<InterfaceSource723, InterfaceDestination723>
        .NewConfig()
        .TwoWays()
        .Ignore(dest => dest.Ignore);

    InterfaceSource723 dataSourse = new Data723() { Inter = "IterDataSourse", Ignore = "IgnoreDataSourse" };
    InterfaceDestination723 dataDestination = new Data723() { Inter = "IterDataDestination", Ignore = "IgnoreDataDestination" };

    var idestination = dataDestination.Adapt<InterfaceDestination723>();
    var isourse = dataDestination.Adapt<InterfaceSource723>();

    var result = dataSourse.Adapt(dataDestination); result this is type = Data723 is not   new created Type

    result.Ignore.ShouldBe("IgnoreDataDestination"); 
    result.Inter.ShouldBe("IterDataSourse");
}

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.

@DocSvartz
Copy link

DocSvartz commented Jan 7, 2025

That's what I mean

IActivityData  data = new MyData() 

 var result =   data.Adapt<IActivityData>() where result is type MyData 

Hm, There is another option though.
When the SourceType also implements the Destination interface.
This should work too.

@DocSvartz DocSvartz assigned DocSvartz and unassigned DocSvartz Jan 7, 2025
@DocSvartz
Copy link

DocSvartz commented Jan 7, 2025

@lofcz @andrerav .
If my proposal solves (from i write in review) the problem as well as the original one.
I think it's better to use it. Until we figure it out.

In any case, I agree with this pull request.

@DocSvartz
Copy link

@lofcz

I even managed to make this option work. ))
Did you want something like this?

  [TestMethod]
  public void MappingToInterface_VerifyReadonlyPropsInterfaceRule()
  {


      SampleInterfaceCls source = new SampleInterfaceCls
      {
          ActivityData = new SampleActivityData
          {
              Data = new SampleActivityParsedData
              {
                  Steps = new List<string> { "A", "B", "C" }
              },
              Temp = "Temp data"

          }

      };
      SampleInterfaceCls source2 = new SampleInterfaceCls
      {
          ActivityData = new SampleActivityData
          {
              Data = new SampleActivityParsedData
              {
                  Steps = new List<string> { "X", "Y", "Z" }
              },
              Temp = "Update Temp data"

          }

      };

      var target = source.Adapt<SampleInterfaceClsParent>();
      var target2 = source2.Adapt<SampleInterfaceCls>();
      var update = target.Adapt(target2);

      target.ShouldNotBeNull();
      target.ShouldSatisfyAllConditions(
          () => target.ActivityData.ShouldBe(source.ActivityData)
      );

      update.ActivityData.ShouldBe(target.ActivityData);

      var resultdata = ((IActivityData)update.ActivityData);

      resultdata.Temp.ShouldBe("Temp data");

      ((SampleActivityData)update.ActivityData).Data.Steps[0].ShouldBe("A");
      ((SampleActivityData)update.ActivityData).Data.Steps[1].ShouldBe("B");
      ((SampleActivityData)update.ActivityData).Data.Steps[2].ShouldBe("C");

  }
    public interface IActivityData : IActivityDataParent
    {
        public string Temp {  get; set; }
    }

    public interface IActivityDataParent
    {

    }

    public class SampleInterfaceCls
    {
        [Newtonsoft.Json.JsonIgnore]
        public IActivityDataParent? ActivityData { get; set; }

        public SampleInterfaceCls()
        {

        }

        public SampleInterfaceCls(IActivityDataParent data)
        {
            SetActivityData(data);
        }

        public void SetActivityData(IActivityDataParent data)
        {
            ActivityData = data;
        }
    }

    public class SampleInterfaceClsParent
    {
        [Newtonsoft.Json.JsonIgnore]
        public IActivityData? ActivityData { get; set; }

        public SampleInterfaceClsParent()
        {

        }

        public SampleInterfaceClsParent(IActivityData data)
        {
            SetActivityData(data);
        }

        public void SetActivityData(IActivityData data)
        {
            ActivityData = data;
        }
    }

@lofcz
Copy link
Author

lofcz commented Jan 8, 2025

@lofcz

I even managed to make this option work. )) Did you want something like this?

  [TestMethod]
  public void MappingToInterface_VerifyReadonlyPropsInterfaceRule()
  {


      SampleInterfaceCls source = new SampleInterfaceCls
      {
          ActivityData = new SampleActivityData
          {
              Data = new SampleActivityParsedData
              {
                  Steps = new List<string> { "A", "B", "C" }
              },
              Temp = "Temp data"

          }

      };
      SampleInterfaceCls source2 = new SampleInterfaceCls
      {
          ActivityData = new SampleActivityData
          {
              Data = new SampleActivityParsedData
              {
                  Steps = new List<string> { "X", "Y", "Z" }
              },
              Temp = "Update Temp data"

          }

      };

      var target = source.Adapt<SampleInterfaceClsParent>();
      var target2 = source2.Adapt<SampleInterfaceCls>();
      var update = target.Adapt(target2);

      target.ShouldNotBeNull();
      target.ShouldSatisfyAllConditions(
          () => target.ActivityData.ShouldBe(source.ActivityData)
      );

      update.ActivityData.ShouldBe(target.ActivityData);

      var resultdata = ((IActivityData)update.ActivityData);

      resultdata.Temp.ShouldBe("Temp data");

      ((SampleActivityData)update.ActivityData).Data.Steps[0].ShouldBe("A");
      ((SampleActivityData)update.ActivityData).Data.Steps[1].ShouldBe("B");
      ((SampleActivityData)update.ActivityData).Data.Steps[2].ShouldBe("C");

  }
    public interface IActivityData : IActivityDataParent
    {
        public string Temp {  get; set; }
    }

    public interface IActivityDataParent
    {

    }

    public class SampleInterfaceCls
    {
        [Newtonsoft.Json.JsonIgnore]
        public IActivityDataParent? ActivityData { get; set; }

        public SampleInterfaceCls()
        {

        }

        public SampleInterfaceCls(IActivityDataParent data)
        {
            SetActivityData(data);
        }

        public void SetActivityData(IActivityDataParent data)
        {
            ActivityData = data;
        }
    }

    public class SampleInterfaceClsParent
    {
        [Newtonsoft.Json.JsonIgnore]
        public IActivityData? ActivityData { get; set; }

        public SampleInterfaceClsParent()
        {

        }

        public SampleInterfaceClsParent(IActivityData data)
        {
            SetActivityData(data);
        }

        public void SetActivityData(IActivityData data)
        {
            ActivityData = data;
        }
    }

Looks great to me.

@DocSvartz
Copy link

@lofcz Thanks!

@andrerav This can be applied and release
What I came up with may require additional discussion :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants