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

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

Open
RashmiBK5 opened this issue Jan 23, 2024 · 13 comments
Assignees
Labels
bug (cc: fix) Something isn't working question Further information is requested

Comments

@RashmiBK5
Copy link

RashmiBK5 commented Jan 23, 2024

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();

@RashmiBK5 RashmiBK5 added the question Further information is requested label Jan 23, 2024
@Mike-E-angelo Mike-E-angelo added the bug (cc: fix) Something isn't working label Jan 23, 2024
@Mike-E-angelo Mike-E-angelo self-assigned this Jan 23, 2024
Copy link

Branch issues/fix/i609 created!

@Mike-E-angelo
Copy link
Member

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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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
Copy link
Author

RashmiBK5 commented Jan 24, 2024

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

GitHub
Contribute to RashmiBK5/ExtendedXML development by creating an account on GitHub.

@Mike-E-angelo
Copy link
Member

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

@Mike-E-angelo
Copy link
Member

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
Copy link
Author

RashmiBK5 commented Jan 24, 2024

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
Copy link
Member

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
Copy link
Member

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:

sealed class MemberComparer : IMemberComparer

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
Copy link
Author

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug (cc: fix) Something isn't working question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants