Skip to content

Commit 10a6130

Browse files
StephenMolloyStephenMolloy
authored
Backport pr 111723 to 9.0 staging (#112322)
* Add tests to verify issue and fix. * Check for type-assignability instead of equivallence. Also fix Choice logic. * Add tests to verify issue and fix. * Ensure collections are initialized to empty - even if they should be null according to the xml. * Disable test scenarios that find failures that aren't fixed until .Net 10.
1 parent 49a1042 commit 10a6130

File tree

8 files changed

+4310
-2365
lines changed

8 files changed

+4310
-2365
lines changed

‎src/libraries/Microsoft.XmlSerializer.Generator/tests/Expected.SerializableAssembly.XmlSerializers.cs‎

Lines changed: 3224 additions & 2306 deletions
Large diffs are not rendered by default.

‎src/libraries/System.Private.Xml/src/System/Xml/Serialization/ReflectionXmlSerializationReader.cs‎

Lines changed: 77 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1682,6 +1682,11 @@ void Wrapper(object? collection, object? collectionItems)
16821682
{
16831683
if (member.Source == null && mapping.TypeDesc.IsArrayLike && !(mapping.Elements!.Length == 1 && mapping.Elements[0].Mapping is ArrayMapping))
16841684
{
1685+
// Always create a collection for (non-array) collection-like types, even if the XML data says the collection should be null.
1686+
if (!mapping.TypeDesc.IsArray)
1687+
{
1688+
member.Collection ??= new CollectionMember();
1689+
}
16851690
member.Source = (item) =>
16861691
{
16871692
member.Collection ??= new CollectionMember();
@@ -1694,26 +1699,51 @@ void Wrapper(object? collection, object? collectionItems)
16941699

16951700
if (member.Source == null)
16961701
{
1702+
var isList = mapping.TypeDesc.IsArrayLike && !mapping.TypeDesc.IsArray;
16971703
var pi = member.Mapping.MemberInfo as PropertyInfo;
1698-
if (pi != null && typeof(IList).IsAssignableFrom(pi.PropertyType)
1699-
&& (pi.SetMethod == null || !pi.SetMethod.IsPublic))
1704+
1705+
// Here we have to deal with some special cases for property members. The old serializers would trip over
1706+
// private property setters generally - except in the case of list-like properties. Because lists get special
1707+
// treatment, a private setter for a list property would only be a problem if the default constructor didn't
1708+
// already create a list instance for the property. If it does create a list, then the serializer can still
1709+
// populate it. Try to emulate the old serializer behavior here.
1710+
1711+
// First, for non-list properties, private setters are always a problem.
1712+
if (!isList && pi != null && pi.SetMethod != null && !pi.SetMethod.IsPublic)
17001713
{
1701-
member.Source = (value) =>
1714+
member.Source = (value) => throw new InvalidOperationException(SR.Format(SR.XmlReadOnlyPropertyError, pi.Name, pi.DeclaringType!.FullName));
1715+
}
1716+
1717+
// Next, for list properties, we need to handle not only the private setter case, but also the case where
1718+
// there is no setter at all. Because we need to give the default constructor a chance to create the list
1719+
// first before we make noise about not being able to set a list property.
1720+
else if (isList && pi != null && (pi.SetMethod == null || !pi.SetMethod.IsPublic))
1721+
{
1722+
var addMethod = mapping.TypeDesc.Type!.GetMethod("Add");
1723+
1724+
if (addMethod != null)
17021725
{
1703-
var getOnlyList = (IList)pi.GetValue(o)!;
1704-
if (value is IList valueList)
1726+
member.Source = (value) =>
17051727
{
1706-
foreach (var v in valueList)
1728+
var getOnlyList = pi.GetValue(o)!;
1729+
if (getOnlyList == null)
17071730
{
1708-
getOnlyList.Add(v);
1731+
// No-setter lists should just be ignored if they weren't created by constructor. Private-setter lists are the noisy exception.
1732+
if (pi.SetMethod != null && !pi.SetMethod.IsPublic)
1733+
throw new InvalidOperationException(SR.Format(SR.XmlReadOnlyPropertyError, pi.Name, pi.DeclaringType!.FullName));
17091734
}
1710-
}
1711-
else
1712-
{
1713-
getOnlyList.Add(value);
1714-
}
1715-
};
1735+
else if (value is IEnumerable valueList)
1736+
{
1737+
foreach (var v in valueList)
1738+
{
1739+
addMethod.Invoke(getOnlyList, new object[] { v });
1740+
}
1741+
}
1742+
};
1743+
}
17161744
}
1745+
1746+
// For all other members (fields, public setter properties, etc), just carry on as normal
17171747
else
17181748
{
17191749
if (member.Mapping.Xmlns != null)
@@ -1729,6 +1759,21 @@ void Wrapper(object? collection, object? collectionItems)
17291759
member.Source = (value) => setterDelegate(o, value);
17301760
}
17311761
}
1762+
1763+
// Finally, special list handling again. ANY list that we can assign/populate should be initialized with
1764+
// an empty list if it hasn't been initialized already. Even if the XML data says the list should be null.
1765+
// This is an odd legacy behavior, but it's what the old serializers did.
1766+
if (isList && member.Source != null)
1767+
{
1768+
member.EnsureCollection = (obj) =>
1769+
{
1770+
if (GetMemberValue(obj, mapping.MemberInfo!) == null)
1771+
{
1772+
var empty = ReflectionCreateObject(mapping.TypeDesc.Type!);
1773+
member.Source(empty);
1774+
}
1775+
};
1776+
}
17321777
}
17331778

17341779
if (member.Mapping.CheckSpecified == SpecifiedAccessor.ReadWrite)
@@ -1782,23 +1827,29 @@ void Wrapper(object elementNameObject)
17821827
WriteAttributes(allMembers, anyAttribute, unknownNodeAction, ref o);
17831828

17841829
Reader.MoveToElement();
1830+
17851831
if (Reader.IsEmptyElement)
17861832
{
17871833
Reader.Skip();
1788-
return o;
17891834
}
1790-
1791-
Reader.ReadStartElement();
1792-
bool IsSequenceAllMembers = IsSequence();
1793-
if (IsSequenceAllMembers)
1835+
else
17941836
{
1795-
// https://github.com/dotnet/runtime/issues/1402:
1796-
// Currently the reflection based method treat this kind of type as normal types.
1797-
// But potentially we can do some optimization for types that have ordered properties.
1798-
}
1837+
Reader.ReadStartElement();
1838+
bool IsSequenceAllMembers = IsSequence();
1839+
if (IsSequenceAllMembers)
1840+
{
1841+
// https://github.com/dotnet/runtime/issues/1402:
1842+
// Currently the reflection based method treat this kind of type as normal types.
1843+
// But potentially we can do some optimization for types that have ordered properties.
1844+
}
17991845

1800-
WriteMembers(allMembers, unknownNodeAction, unknownNodeAction, anyElementMember, anyTextMember);
1846+
WriteMembers(allMembers, unknownNodeAction, unknownNodeAction, anyElementMember, anyTextMember);
18011847

1848+
ReadEndElement();
1849+
}
1850+
1851+
// Empty element or not, we need to ensure all our array-like members have been initialized in the same
1852+
// way as the IL / CodeGen - based serializers.
18021853
foreach (Member member in allMembers)
18031854
{
18041855
if (member.Collection != null)
@@ -1810,9 +1861,10 @@ void Wrapper(object elementNameObject)
18101861
var setMemberValue = GetSetMemberValueDelegate(o, memberInfo.Name);
18111862
setMemberValue(o, collection);
18121863
}
1864+
1865+
member.EnsureCollection?.Invoke(o!);
18131866
}
18141867

1815-
ReadEndElement();
18161868
return o;
18171869
}
18181870
}
@@ -2090,6 +2142,7 @@ internal sealed class Member
20902142
public Action<object?>? CheckSpecifiedSource;
20912143
public Action<object>? ChoiceSource;
20922144
public Action<string, string>? XmlnsSource;
2145+
public Action<object>? EnsureCollection;
20932146

20942147
public Member(MemberMapping mapping)
20952148
{

‎src/libraries/System.Private.Xml/src/System/Xml/Serialization/ReflectionXmlSerializationWriter.cs‎

Lines changed: 86 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
using System.Collections.Generic;
66
using System.Diagnostics;
77
using System.Diagnostics.CodeAnalysis;
8+
using System.Globalization;
89
using System.Reflection;
910
using System.Text;
1011
using System.Xml.Schema;
@@ -125,7 +126,7 @@ private void WriteMember(object? o, object? choiceSource, ElementAccessor[] elem
125126
}
126127
else
127128
{
128-
WriteElements(o, elements, text, choice, writeAccessors, memberTypeDesc.IsNullable);
129+
WriteElements(o, choiceSource, elements, text, choice, writeAccessors, memberTypeDesc.IsNullable);
129130
}
130131
}
131132

@@ -150,11 +151,11 @@ private void WriteArray(object o, object? choiceSource, ElementAccessor[] elemen
150151
}
151152
}
152153

153-
WriteArrayItems(elements, text, choice, o);
154+
WriteArrayItems(elements, text, choice, o, choiceSource);
154155
}
155156

156157
[RequiresUnreferencedCode("calls WriteElements")]
157-
private void WriteArrayItems(ElementAccessor[] elements, TextAccessor? text, ChoiceIdentifierAccessor? choice, object o)
158+
private void WriteArrayItems(ElementAccessor[] elements, TextAccessor? text, ChoiceIdentifierAccessor? choice, object o, object? choiceSources)
158159
{
159160
var arr = o as IList;
160161

@@ -163,7 +164,8 @@ private void WriteArrayItems(ElementAccessor[] elements, TextAccessor? text, Cho
163164
for (int i = 0; i < arr.Count; i++)
164165
{
165166
object? ai = arr[i];
166-
WriteElements(ai, elements, text, choice, true, true);
167+
var choiceSource = ((Array?)choiceSources)?.GetValue(i);
168+
WriteElements(ai, choiceSource, elements, text, choice, true, true);
167169
}
168170
}
169171
else
@@ -174,17 +176,18 @@ private void WriteArrayItems(ElementAccessor[] elements, TextAccessor? text, Cho
174176
IEnumerator e = a.GetEnumerator();
175177
if (e != null)
176178
{
179+
int c = 0;
177180
while (e.MoveNext())
178181
{
179182
object ai = e.Current;
180-
WriteElements(ai, elements, text, choice, true, true);
183+
var choiceSource = ((Array?)choiceSources)?.GetValue(c++);
184+
WriteElements(ai, choiceSource, elements, text, choice, true, true);
181185
}
182186
}
183187
}
184188
}
185-
186189
[RequiresUnreferencedCode("calls CreateUnknownTypeException")]
187-
private void WriteElements(object? o, ElementAccessor[] elements, TextAccessor? text, ChoiceIdentifierAccessor? choice, bool writeAccessors, bool isNullable)
190+
private void WriteElements(object? o, object? choiceSource, ElementAccessor[] elements, TextAccessor? text, ChoiceIdentifierAccessor? choice, bool writeAccessors, bool isNullable)
188191
{
189192
if (elements.Length == 0 && text == null)
190193
return;
@@ -222,16 +225,35 @@ private void WriteElements(object? o, ElementAccessor[] elements, TextAccessor?
222225
}
223226
else if (choice != null)
224227
{
225-
if (o != null && o.GetType() == element.Mapping!.TypeDesc!.Type)
228+
// This looks heavy - getting names of enums in string form for comparison rather than just comparing values.
229+
// But this faithfully mimics NetFx, and is necessary to prevent confusion between different enum types.
230+
// ie EnumType.ValueX could == 1, but TotallyDifferentEnumType.ValueY could also == 1.
231+
TypeDesc td = element.Mapping!.TypeDesc!;
232+
bool enumUseReflection = choice.Mapping!.TypeDesc!.UseReflection;
233+
string enumTypeName = choice.Mapping!.TypeDesc!.FullName;
234+
string enumFullName = (enumUseReflection ? "" : enumTypeName + ".@") + FindChoiceEnumValue(element, (EnumMapping)choice.Mapping, enumUseReflection);
235+
string choiceFullName = (enumUseReflection ? "" : choiceSource!.GetType().FullName + ".@") + choiceSource!.ToString();
236+
237+
if (choiceFullName == enumFullName)
226238
{
227-
WriteElement(o, element, writeAccessors);
228-
return;
239+
// Object is either non-null, or it is allowed to be null
240+
if (o != null || (!isNullable || element.IsNullable))
241+
{
242+
// But if Object is non-null, it's got to match types
243+
if (o != null && !td.Type!.IsAssignableFrom(o!.GetType()))
244+
{
245+
throw CreateMismatchChoiceException(td.FullName, choice.MemberName!, enumFullName);
246+
}
247+
248+
WriteElement(o, element, writeAccessors);
249+
return;
250+
}
229251
}
230252
}
231253
else
232254
{
233255
TypeDesc td = element.IsUnbounded ? element.Mapping!.TypeDesc!.CreateArrayTypeDesc() : element.Mapping!.TypeDesc!;
234-
if (o!.GetType() == td.Type)
256+
if (td.Type!.IsAssignableFrom(o!.GetType()))
235257
{
236258
WriteElement(o, element, writeAccessors);
237259
return;
@@ -280,6 +302,58 @@ private void WriteElements(object? o, ElementAccessor[] elements, TextAccessor?
280302
}
281303
}
282304

305+
private static string FindChoiceEnumValue(ElementAccessor element, EnumMapping choiceMapping, bool useReflection)
306+
{
307+
string? enumValue = null;
308+
309+
for (int i = 0; i < choiceMapping.Constants!.Length; i++)
310+
{
311+
string xmlName = choiceMapping.Constants[i].XmlName;
312+
313+
if (element.Any && element.Name.Length == 0)
314+
{
315+
if (xmlName == "##any:")
316+
{
317+
if (useReflection)
318+
enumValue = choiceMapping.Constants[i].Value.ToString(CultureInfo.InvariantCulture);
319+
else
320+
enumValue = choiceMapping.Constants[i].Name;
321+
break;
322+
}
323+
continue;
324+
}
325+
int colon = xmlName.LastIndexOf(':');
326+
string? choiceNs = colon < 0 ? choiceMapping.Namespace : xmlName.Substring(0, colon);
327+
string choiceName = colon < 0 ? xmlName : xmlName.Substring(colon + 1);
328+
329+
if (element.Name == choiceName)
330+
{
331+
if ((element.Form == XmlSchemaForm.Unqualified && string.IsNullOrEmpty(choiceNs)) || element.Namespace == choiceNs)
332+
{
333+
if (useReflection)
334+
enumValue = choiceMapping.Constants[i].Value.ToString(CultureInfo.InvariantCulture);
335+
else
336+
enumValue = choiceMapping.Constants[i].Name;
337+
break;
338+
}
339+
}
340+
}
341+
342+
if (string.IsNullOrEmpty(enumValue))
343+
{
344+
if (element.Any && element.Name.Length == 0)
345+
{
346+
// Type {0} is missing enumeration value '##any' for XmlAnyElementAttribute.
347+
throw new InvalidOperationException(SR.Format(SR.XmlChoiceMissingAnyValue, choiceMapping.TypeDesc!.FullName));
348+
}
349+
// Type {0} is missing value for '{1}'.
350+
throw new InvalidOperationException(SR.Format(SR.XmlChoiceMissingValue, choiceMapping.TypeDesc!.FullName, element.Namespace + ":" + element.Name, element.Name, element.Namespace));
351+
}
352+
if (!useReflection)
353+
CodeIdentifier.CheckValidIdentifier(enumValue);
354+
return enumValue;
355+
}
356+
283357
private void WriteText(object o, TextAccessor text)
284358
{
285359
if (text.Mapping is PrimitiveMapping primitiveMapping)
@@ -376,7 +450,7 @@ private void WriteElement(object? o, ElementAccessor element, bool writeAccessor
376450
if (o != null)
377451
{
378452
WriteStartElement(name, ns, false);
379-
WriteArrayItems(mapping.ElementsSortedByDerivation!, null, null, o);
453+
WriteArrayItems(mapping.ElementsSortedByDerivation!, null, null, o, null);
380454
WriteEndElement();
381455
}
382456
}

‎src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializer.cs‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -379,7 +379,6 @@ public void Serialize(XmlWriter xmlWriter, object? o, XmlSerializerNamespaces? n
379379
}
380380
else if (_tempAssembly == null || _typedSerializer)
381381
{
382-
// The contion for the block is never true, thus the block is never hit.
383382
XmlSerializationWriter writer = CreateWriter();
384383
writer.Init(xmlWriter, namespaces == null || namespaces.Count == 0 ? DefaultNamespaces : namespaces, encodingStyle, id);
385384
Serialize(o, writer);

0 commit comments

Comments
 (0)