Skip to content

Reflection-based XmlSerializer - Deserialize empty collections and allow for sub-types in collection items. #111723

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Feb 19, 2025
Merged

Reflection-based XmlSerializer - Deserialize empty collections and allow for sub-types in collection items. #111723

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
fb695c7
Add tests to verify issue and fix.
StephenMolloy Dec 20, 2024
28934e9
Check for type-assignability instead of equivallence. Also fix Choice…
StephenMolloy Dec 20, 2024
f0197fb
Add tests to verify issue and fix.
StephenMolloy Jan 17, 2025
04226b8
Ensure collections are initialized to empty - even if they should be …
StephenMolloy Jan 22, 2025
100dca8
More tests for bringing reflection-based serializer to quirky parity …
StephenMolloy Jan 31, 2025
36093e6
Fix Xml/Type mapping to use proper member info with hidden/inherited …
StephenMolloy Jan 31, 2025
af6124a
Cleanup tests and comments based on PR feedback.
StephenMolloy Feb 7, 2025
3e9f549
While considering initializing empty lists at object creation instead…
StephenMolloy Feb 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5,718 changes: 3,318 additions & 2,400 deletions ...s/Microsoft.XmlSerializer.Generator/tests/Expected.SerializableAssembly.XmlSerializers.cs
View file
Open in desktop

Large diffs are not rendered by default.

25 changes: 25 additions & 0 deletions src/libraries/System.Private.Xml/src/System/Xml/Serialization/Mappings.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -981,6 +981,31 @@ internal MemberMapping Clone()
{
return new MemberMapping(this);
}

internal bool Hides(MemberMapping mapping)
{
// Semantics. But all mappings would hide a null mapping, so indicating that this particular instance of mapping
// specifically hides a null mapping is not necessary and could be misleading. So just return false.
if (mapping == null)
return false;

var baseMember = mapping.MemberInfo;
var derivedMember = this.MemberInfo;

// Similarly, if either mapping (or both) lacks MemberInfo, then its not appropriate to say that one hides the other.
if (baseMember == null || derivedMember == null)
return false;

// If the member names are different, they don't hide each other
if (baseMember.Name != derivedMember.Name)
return false;

// Names are the same. So, regardless of field or property or accessibility or return type, if derivedDeclaringType is a
// subclass of baseDeclaringType, then the base member is hidden.
Type? baseDeclaringType = baseMember.DeclaringType;
Type? derivedDeclaringType = derivedMember.DeclaringType;
return baseDeclaringType != null && derivedDeclaringType != null && baseDeclaringType.IsAssignableFrom(derivedDeclaringType);
}
}

internal sealed class MembersMapping : TypeMapping
Expand Down
101 changes: 77 additions & 24 deletions ...aries/System.Private.Xml/src/System/Xml/Serialization/ReflectionXmlSerializationReader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1633,6 +1633,11 @@ private static XmlSerializationCollectionFixupCallback GetCreateCollectionOfObje
{
if (member.Source == null && mapping.TypeDesc.IsArrayLike && !(mapping.Elements!.Length == 1 && mapping.Elements[0].Mapping is ArrayMapping))
{
// Always create a collection for (non-array) collection-like types, even if the XML data says the collection should be null.
if (!mapping.TypeDesc.IsArray)
{
member.Collection ??= new CollectionMember();
}
Comment on lines +1636 to +1640
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When considering whether it would make more sense to initialize empty lists above here after creating our object - rather than cleaning up after deserialization with an 'ensure' approach - I noticed that this case here was missed. Added one more property to one of the new testcases to hit this code path and ensure serializer parity here.

FWIW, I decided to stick with the 'ensure' approach because to do the proactive initialization above here would require duplicating a lot of the silly property-detection logic happening below here. I was getting messy and duplicative.

member.Source = (item) =>
{
member.Collection ??= new CollectionMember();
Expand All @@ -1645,26 +1650,51 @@ private static XmlSerializationCollectionFixupCallback GetCreateCollectionOfObje

if (member.Source == null)
{
var isList = mapping.TypeDesc.IsArrayLike && !mapping.TypeDesc.IsArray;
var pi = member.Mapping.MemberInfo as PropertyInfo;
if (pi != null && typeof(IList).IsAssignableFrom(pi.PropertyType)
&& (pi.SetMethod == null || !pi.SetMethod.IsPublic))

// Here we have to deal with some special cases for property members. The old serializers would trip over
// private property setters generally - except in the case of list-like properties. Because lists get special
// treatment, a private setter for a list property would only be a problem if the default constructor didn't
// already create a list instance for the property. If it does create a list, then the serializer can still
// populate it. Try to emulate the old serializer behavior here.

// First, for non-list properties, private setters are always a problem.
if (!isList && pi != null && pi.SetMethod != null && !pi.SetMethod.IsPublic)
{
member.Source = (value) =>
member.Source = (value) => throw new InvalidOperationException(SR.Format(SR.XmlReadOnlyPropertyError, pi.Name, pi.DeclaringType!.FullName));
}

// Next, for list properties, we need to handle not only the private setter case, but also the case where
// there is no setter at all. Because we need to give the default constructor a chance to create the list
// first before we make noise about not being able to set a list property.
else if (isList && pi != null && (pi.SetMethod == null || !pi.SetMethod.IsPublic))
{
var addMethod = mapping.TypeDesc.Type!.GetMethod("Add");

if (addMethod != null)
{
var getOnlyList = (IList)pi.GetValue(o)!;
if (value is IList valueList)
member.Source = (value) =>
{
foreach (var v in valueList)
var getOnlyList = pi.GetValue(o)!;
if (getOnlyList == null)
{
getOnlyList.Add(v);
// No-setter lists should just be ignored if they weren't created by constructor. Private-setter lists are the noisy exception.
if (pi.SetMethod != null && !pi.SetMethod.IsPublic)
throw new InvalidOperationException(SR.Format(SR.XmlReadOnlyPropertyError, pi.Name, pi.DeclaringType!.FullName));
}
}
else
{
getOnlyList.Add(value);
}
};
else if (value is IEnumerable valueList)
{
foreach (var v in valueList)
{
addMethod.Invoke(getOnlyList, new object[] { v });
}
}
};
}
}

// For all other members (fields, public setter properties, etc), just carry on as normal
else
{
if (member.Mapping.Xmlns != null)
Expand All @@ -1680,6 +1710,21 @@ private static XmlSerializationCollectionFixupCallback GetCreateCollectionOfObje
member.Source = (value) => setterDelegate(o, value);
}
}

// Finally, special list handling again. ANY list that we can assign/populate should be initialized with
// an empty list if it hasn't been initialized already. Even if the XML data says the list should be null.
// This is an odd legacy behavior, but it's what the old serializers did.
if (isList && member.Source != null)
{
member.EnsureCollection = (obj) =>
{
if (GetMemberValue(obj, mapping.MemberInfo!) == null)
{
var empty = ReflectionCreateObject(mapping.TypeDesc.Type!);
member.Source(empty);
}
};
}
}

if (member.Mapping.CheckSpecified == SpecifiedAccessor.ReadWrite)
Expand Down Expand Up @@ -1729,23 +1774,29 @@ private static XmlSerializationCollectionFixupCallback GetCreateCollectionOfObje
WriteAttributes(allMembers, anyAttribute, unknownNodeAction, ref o);

Reader.MoveToElement();

if (Reader.IsEmptyElement)
{
Reader.Skip();
return o;
}

Reader.ReadStartElement();
bool IsSequenceAllMembers = IsSequence();
if (IsSequenceAllMembers)
else
{
// https://github.com/dotnet/runtime/issues/1402:
// Currently the reflection based method treat this kind of type as normal types.
// But potentially we can do some optimization for types that have ordered properties.
}
Reader.ReadStartElement();
bool IsSequenceAllMembers = IsSequence();
if (IsSequenceAllMembers)
{
// https://github.com/dotnet/runtime/issues/1402:
// Currently the reflection based method treat this kind of type as normal types.
// But potentially we can do some optimization for types that have ordered properties.
}

WriteMembers(allMembers, unknownNodeAction, unknownNodeAction, anyElementMember, anyTextMember);
WriteMembers(allMembers, unknownNodeAction, unknownNodeAction, anyElementMember, anyTextMember);

ReadEndElement();
}

// Empty element or not, we need to ensure all our array-like members have been initialized in the same
// way as the IL / CodeGen - based serializers.
foreach (Member member in allMembers)
{
if (member.Collection != null)
Expand All @@ -1757,9 +1808,10 @@ private static XmlSerializationCollectionFixupCallback GetCreateCollectionOfObje
var setMemberValue = GetSetMemberValueDelegate(o, memberInfo.Name);
setMemberValue(o, collection);
}

member.EnsureCollection?.Invoke(o!);
}

ReadEndElement();
return o;
}
}
Expand Down Expand Up @@ -2031,6 +2083,7 @@ internal sealed class Member
public Action<object?>? CheckSpecifiedSource;
public Action<object>? ChoiceSource;
public Action<string, string>? XmlnsSource;
public Action<object>? EnsureCollection;

public Member(MemberMapping mapping)
{
Expand Down
97 changes: 86 additions & 11 deletions ...aries/System.Private.Xml/src/System/Xml/Serialization/ReflectionXmlSerializationWriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Collections.Generic;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Globalization;
using System.Reflection;
using System.Text;
using System.Xml.Schema;
Expand Down Expand Up @@ -122,7 +123,7 @@ private void WriteMember(object? o, object? choiceSource, ElementAccessor[] elem
}
else
{
WriteElements(o, elements, text, choice, writeAccessors, memberTypeDesc.IsNullable);
WriteElements(o, choiceSource, elements, text, choice, writeAccessors, memberTypeDesc.IsNullable);
}
}

Expand All @@ -146,10 +147,10 @@ private void WriteArray(object o, object? choiceSource, ElementAccessor[] elemen
}
}

WriteArrayItems(elements, text, choice, o);
WriteArrayItems(elements, text, choice, o, choiceSource);
}

private void WriteArrayItems(ElementAccessor[] elements, TextAccessor? text, ChoiceIdentifierAccessor? choice, object o)
private void WriteArrayItems(ElementAccessor[] elements, TextAccessor? text, ChoiceIdentifierAccessor? choice, object o, object? choiceSources)
{
var arr = o as IList;

Expand All @@ -158,7 +159,8 @@ private void WriteArrayItems(ElementAccessor[] elements, TextAccessor? text, Cho
for (int i = 0; i < arr.Count; i++)
{
object? ai = arr[i];
WriteElements(ai, elements, text, choice, true, true);
var choiceSource = ((Array?)choiceSources)?.GetValue(i);
WriteElements(ai, choiceSource, elements, text, choice, true, true);
}
}
else
Expand All @@ -169,16 +171,18 @@ private void WriteArrayItems(ElementAccessor[] elements, TextAccessor? text, Cho
IEnumerator e = a.GetEnumerator();
if (e != null)
{
int c = 0;
while (e.MoveNext())
{
object ai = e.Current;
WriteElements(ai, elements, text, choice, true, true);
var choiceSource = ((Array?)choiceSources)?.GetValue(c++);
WriteElements(ai, choiceSource, elements, text, choice, true, true);
}
}
}
}

private void WriteElements(object? o, ElementAccessor[] elements, TextAccessor? text, ChoiceIdentifierAccessor? choice, bool writeAccessors, bool isNullable)
private void WriteElements(object? o, object? choiceSource, ElementAccessor[] elements, TextAccessor? text, ChoiceIdentifierAccessor? choice, bool writeAccessors, bool isNullable)
{
if (elements.Length == 0 && text == null)
return;
Expand Down Expand Up @@ -216,16 +220,35 @@ private void WriteElements(object? o, ElementAccessor[] elements, TextAccessor?
}
else if (choice != null)
{
if (o != null && o.GetType() == element.Mapping!.TypeDesc!.Type)
// This looks heavy - getting names of enums in string form for comparison rather than just comparing values.
// But this faithfully mimics NetFx, and is necessary to prevent confusion between different enum types.
// ie EnumType.ValueX could == 1, but TotallyDifferentEnumType.ValueY could also == 1.
TypeDesc td = element.Mapping!.TypeDesc!;
bool enumUseReflection = choice.Mapping!.TypeDesc!.UseReflection;
string enumTypeName = choice.Mapping!.TypeDesc!.FullName;
string enumFullName = (enumUseReflection ? "" : enumTypeName + ".@") + FindChoiceEnumValue(element, (EnumMapping)choice.Mapping, enumUseReflection);
string choiceFullName = (enumUseReflection ? "" : choiceSource!.GetType().FullName + ".@") + choiceSource!.ToString();

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you might have a problem here if the enum has the [Flags] attribute as ToString might return multiple names that are comma delimited.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a little trouble reasoning about this. Which is why I decided to not be clever and just do what ILGen does. This sequence here was pretty much modeled after the code here: https://github.com/StephenMolloy/runtime/blob/36093e63bbba5bbea9c970fb45adf336181aa86b/src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializationWriter.cs#L3684

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I don't think the 'enum' here is the property itself. Rather it refers to the list of enum values that are used to identify the type of an object in a choice list. Like is used in the 'choice' classes here: https://github.com/StephenMolloy/runtime/blob/36093e63bbba5bbea9c970fb45adf336181aa86b/src/libraries/System.Runtime.Serialization.Xml/tests/SerializationTypes.cs#L970-L1000
I'm not sure [Flags] has anything to do with this scenario, although I suppose it wouldn't hurt to double check.

if (choiceFullName == enumFullName)
{
WriteElement(o, element, writeAccessors);
return;
// Object is either non-null, or it is allowed to be null
if (o != null || (!isNullable || element.IsNullable))
{
// But if Object is non-null, it's got to match types
if (o != null && !td.Type!.IsAssignableFrom(o!.GetType()))
{
throw CreateMismatchChoiceException(td.FullName, choice.MemberName!, enumFullName);
}

WriteElement(o, element, writeAccessors);
return;
}
}
}
else
{
TypeDesc td = element.IsUnbounded ? element.Mapping!.TypeDesc!.CreateArrayTypeDesc() : element.Mapping!.TypeDesc!;
if (o!.GetType() == td.Type)
if (td.Type!.IsAssignableFrom(o!.GetType()))
{
WriteElement(o, element, writeAccessors);
return;
Expand Down Expand Up @@ -274,6 +297,58 @@ private void WriteElements(object? o, ElementAccessor[] elements, TextAccessor?
}
}

private static string FindChoiceEnumValue(ElementAccessor element, EnumMapping choiceMapping, bool useReflection)
{
string? enumValue = null;

for (int i = 0; i < choiceMapping.Constants!.Length; i++)
{
string xmlName = choiceMapping.Constants[i].XmlName;

if (element.Any && element.Name.Length == 0)
{
if (xmlName == "##any:")
{
if (useReflection)
enumValue = choiceMapping.Constants[i].Value.ToString(CultureInfo.InvariantCulture);
else
enumValue = choiceMapping.Constants[i].Name;
break;
}
continue;
}
int colon = xmlName.LastIndexOf(':');
string? choiceNs = colon < 0 ? choiceMapping.Namespace : xmlName.Substring(0, colon);
string choiceName = colon < 0 ? xmlName : xmlName.Substring(colon + 1);

if (element.Name == choiceName)
{
if ((element.Form == XmlSchemaForm.Unqualified && string.IsNullOrEmpty(choiceNs)) || element.Namespace == choiceNs)
{
if (useReflection)
enumValue = choiceMapping.Constants[i].Value.ToString(CultureInfo.InvariantCulture);
else
enumValue = choiceMapping.Constants[i].Name;
break;
}
}
}

if (string.IsNullOrEmpty(enumValue))
{
if (element.Any && element.Name.Length == 0)
{
// Type {0} is missing enumeration value '##any' for XmlAnyElementAttribute.
throw new InvalidOperationException(SR.Format(SR.XmlChoiceMissingAnyValue, choiceMapping.TypeDesc!.FullName));
}
// Type {0} is missing value for '{1}'.
throw new InvalidOperationException(SR.Format(SR.XmlChoiceMissingValue, choiceMapping.TypeDesc!.FullName, element.Namespace + ":" + element.Name, element.Name, element.Namespace));
}
if (!useReflection)
CodeIdentifier.CheckValidIdentifier(enumValue);
return enumValue;
}

private void WriteText(object o, TextAccessor text)
{
if (text.Mapping is PrimitiveMapping primitiveMapping)
Expand Down Expand Up @@ -369,7 +444,7 @@ private void WriteElement(object? o, ElementAccessor element, bool writeAccessor
if (o != null)
{
WriteStartElement(name, ns, false);
WriteArrayItems(mapping.ElementsSortedByDerivation!, null, null, o);
WriteArrayItems(mapping.ElementsSortedByDerivation!, null, null, o, null);
WriteEndElement();
}
}
Expand Down
Loading
Loading