ExtendedXmlSerializer / home

A configurable and eXtensible Xml serializer for .NET.
https://extendedxmlserializer.github.io/
MIT License
334 stars 47 forks source link

Will Configuration for a base class automatically apply to its derived classes? #609

Open RashmiBK5 opened 8 months ago

RashmiBK5 commented 8 months ago

even though I have ignored property at base class, when serializing DerivedClass the XML is still having Property1.

Also, I am upgrading from 3.2.3 to 3.2.7, is there a list of breaking changes available. I looked through the release note I could not find any specific details, could you please help.

Version :3.2.7

public class BaseClass
{
    public string Property1 { get; set; }
}

public class DerivedClass : BaseClass
{
    public string Property2 { get; set; }
}

public class DerivedClass2 : BaseClass
{
    public string Property3 { get; set; }
}

public class BaseClassXmlConfiguration : IXmlConfiguration<BaseClass>
{
    public ITypeConfiguration<BaseClass> Configure(IConfigurationContainer config)
    {
        return config.Type<BaseClass>()
             .Member(s => s.Property1 ).Ignore();
    }
}

public class DerivedClass1XmlConfiguration : IXmlConfiguration<DerivedClass1>
{
    public ITypeConfiguration<DerivedClass1> Configure(IConfigurationContainer config)
    {
        return config.Type<DerivedClass1>();
    }
}

public class DerivedClass2XmlConfiguration : IXmlConfiguration<DerivedClass2>
{
    public ITypeConfiguration<DerivedClass2> Configure(IConfigurationContainer config)
    {
        return config.Type<DerivedClass2>();
    }
}

var serializer = new ConfigurationContainer()
    .UseOptimizedNamespaces()
    .EnableReferences()
    .UseAutoFormatting()
    .Configure(new BaseClassXmlConfiguration())
    .Configure(new DerivedClass1XmlConfiguration())
    .Configure(new DerivedClass2XmlConfiguration())
    .Create();
create-issue-branch[bot] commented 8 months ago

Branch issues/fix/i609 created!

Mike-E-angelo commented 8 months ago

Hey there @RashmiBK5 thank you for writing in and for reporting this. There should not be any breaking changes for minor versions so this appears to be a bug. I will look into this for you and see what happened. 👍

Mike-E-angelo commented 8 months ago

Alright I spent some time looking into this for you @RashmiBK5 and please do pardon my confusion as I didn't realize when 3.2.x was released as it was back in 2020. To be honest I am not sure how well I will be able to support you here.

Nonetheless I did try to load your code with these versions, but there does not appear to be neither an IXmlConfiguration<T> nor an IConfigurationContainer.Configure that supports it.

If you are able to provide code that better illustrates the issue I can try seeing what I can do here.

RashmiBK5 commented 8 months ago

ConsoleApp1.zip

I have updated the code, could you please check. But in sample application base configuration is never considered in 3.2.3.

Mike-E-angelo commented 8 months ago

Thank you @RashmiBK5. I appreciate you creating a project. If you are further able to post it as a repository I would greatly appreciate it. I am not a fan of zips as they are a security consideration.

RashmiBK5 commented 8 months ago

code is available in https://github.com/RashmiBK5/ExtendedXML

GitHub
GitHub - RashmiBK5/ExtendedXML
Contribute to RashmiBK5/ExtendedXML development by creating an account on GitHub.
Mike-E-angelo commented 8 months ago

Thank you very much for that @RashmiBK5 and for your understanding. 🙏 Looking into this now for you.

Mike-E-angelo commented 8 months ago

Alright @RashmiBK5 I see that IXmlConfiguration is a custom class but I unfortunately fail to see how you are configuring it. I tried using the code you initially reported, but it results in errors:

image

Can you update the repository with the code you opened the ticket with?

RashmiBK5 commented 8 months ago

I have updated the Code, now you should be able to run the code https://github.com/RashmiBK5/ExtendedXML

version 3.2.3


<? xml version = "1.0" encoding = "utf-8" ?>
< Details xmlns = "clr-namespace:;assembly=ConsoleApp1" >
   < Description > test </ Description >
   < Objects xmlns: sys = "https://extendedxmlserializer.github.io/system" xmlns: exs = "https://extendedxmlserializer.github.io/v2" exs: type = "sys:List[BaseClass]" >
       < Capacity > 4 </ Capacity >
       < DerivedClass >
           < IsSelected > false </ IsSelected >
           < Property2 > Property2 </ Property2 >
       </ DerivedClass >
       < DerivedClass2 >
           < IsSelected > false </ IsSelected >
           < Property3 > Property3 </ Property3 >
       </ DerivedClass2 >
   </ Objects >
</ Details >

version 3.2.4 onwards


<? xml version = "1.0" encoding = "utf-8" ?>
< Details xmlns = "clr-namespace:;assembly=ConsoleApp1" >
   < Description > test </ Description >
   < Objects xmlns: sys = "https://extendedxmlserializer.github.io/system" xmlns: exs = "https://extendedxmlserializer.github.io/v2" exs: type = "sys:List[BaseClass]" >
       < Capacity > 4 </ Capacity >
       < DerivedClass >
           < Property1 > BaseDerivedClass1 </ Property1 >
           < IsSelected > false </ IsSelected >
           < Property2 > Property2 </ Property2 >
       </ DerivedClass >
       < DerivedClass2 >
           < Property1 > BaseDerivedClass2 </ Property1 >
           < IsSelected > false </ IsSelected >
           < Property3 > Property3 </ Property3 >
       </ DerivedClass2 >
   </ Objects >
</ Details >
Mike-E-angelo commented 8 months ago

Thank you very much for your provided information @RashmiBK5. I have isolated this to #416 and while all the tests passed at the time, you're right here I should have put more thought into the change as technically it is breaking. What we need is some way of assigning the preferred member comparer, or maybe some other fix for this. I'll continue to think about it.

Mike-E-angelo commented 8 months ago

Hi @RashmiBK5 unfortunately I have not been able to figure out a good solution for you, or at least one that I can incorporate into the codebase as a PR after a few hours of investigation. I've tried a few approaches but all of them result in broken tests.

The class in question that is causing this issue is MemberComparer:

https://github.com/ExtendedXmlSerializer/home/blob/4ab31a26cfa72fe4d27ee8de3b00f0366aea73b0/src/ExtendedXmlSerializer/ReflectionModel/MemberComparer.cs#L5

I have several suggestions:

  1. Repeat the member configurations on all types (yuck, I know)
  2. Attempt to create a PR with a fix in MemberComparer above that addresses your issues and passes all existing tests.
  3. Fork this repository and use a special build of ExtendedXmlSerializer, using the code below to replace MemberComparer. This worked for all tests except for one, which I was not able to dertermine why in the time I spent in it:
using System.Reflection;

namespace ExtendedXmlSerializer.ReflectionModel
{
    sealed class MemberComparer : IMemberComparer
    {
        public static MemberComparer Default { get; } = new MemberComparer();

        MemberComparer() : this(Defaults.TypeComparer) {}

        readonly ITypeComparer _type;

        public MemberComparer(ITypeComparer type) => _type = type;

        public bool Equals(MemberInfo x, MemberInfo y)
            => x.Name.Equals(y.Name) &&
               (_type.Equals(x.ReflectedType.GetTypeInfo(), y.ReflectedType.GetTypeInfo()) ||
                x.ReflectedType.IsAssignableFrom(y.DeclaringType));

        public int GetHashCode(MemberInfo obj) => 0;
    }
}

I wish I could assist more but unfortunately my time is limited these days. Please see #383 for more information. Please let me know of any further questions you may have and I will further assist you.

RashmiBK5 commented 8 months ago

Thanks a lot @Mike-E-angelo

What should I do to get the fix as part of Nuget package. I want the fix to be added to version 3.7.6.

And will the same fix be added to latest version as well? in future when we Upgrade to latest version, I will face same issue.

Mike-E-angelo commented 8 months ago

Hi @RashmiBK5 thank you for your continued collaboration. Getting this into the official package is the tricky part, as when I attempted to do so I could not get all tests to pass.

So the idea here is to submit a Pull Request that fixes this issue while ensuring all tests as written pass. I would then review it and if it looks OK, accept it and create a new NuGet package from there.

If this is something you are interested in participating in, I can help/support you through the process.

Please note that this might take some time as it has been a while since the last release and there are some considerations to account for, one of which is to generate a new publishing key on NuGet as the last one expired.