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

20 changes: 20 additions & 0 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 @@ -1680,6 +1680,23 @@ private static XmlSerializationCollectionFixupCallback GetCreateCollectionOfObje
member.Source = (value) => setterDelegate(o, value);
}
}

if (mapping.TypeDesc.IsArrayLike && !mapping.TypeDesc.IsArray && member.Source != null)
{
// At this point, the IL/CodeGen-based serializers of the past decide to "declare" all array-like members. Further in the code that
// produces the 'declaration', we end up initializing non-array's with an empty collection - even if the XML data doesn't include
// that collection (or even explicitly says it's null). This is a weird behavior, but we should mimic it here.
// However, we don't want to create an empty collection just for it to be discarded later. So instead, make note that we need to
// create an empty collection if we get to the end of our element and haven't already filled the collection.
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 @@ -1757,6 +1774,8 @@ private static XmlSerializationCollectionFixupCallback GetCreateCollectionOfObje
var setMemberValue = GetSetMemberValueDelegate(o, memberInfo.Name);
setMemberValue(o, collection);
}

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

ReadEndElement();
Expand Down Expand Up @@ -2031,6 +2050,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
1 change: 0 additions & 1 deletion src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,7 +400,6 @@ public void Serialize(XmlWriter xmlWriter, object? o, XmlSerializerNamespaces? n
}
else if (_tempAssembly == null || _typedSerializer)
{
// The contion for the block is never true, thus the block is never hit.
XmlSerializationWriter writer = CreateWriter();
writer.Init(xmlWriter, namespaces == null || namespaces.Count == 0 ? DefaultNamespaces : namespaces, encodingStyle, id);
Serialize(o, writer);
Expand Down
Loading
Loading