Skip to content

Commit b528c1c

Browse files
CopilotCopilot
authored
Fix XmlDeserializer to use Elements() instead of Descendants() for nested XML
* Fix XmlDeserializer nested element bugs - Fix Bug #1: HandleListDerivative now uses Elements() on containers instead of Descendants() - Fix Bug #2: Deserialize RootElement selection prefers shallowest match - Fix Bug #3: RemoveNamespace filters null values - Add comprehensive tests for all three fixes
1 parent 1abceab commit b528c1c

File tree

3 files changed

+292
-24
lines changed

3 files changed

+292
-24
lines changed

‎src/RestSharp.Serializers.Xml/XmlDeserializer.cs‎

Lines changed: 102 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -40,8 +40,16 @@ public class XmlDeserializer : IXmlDeserializer, IWithRootElement, IWithDateForm
4040
var root = doc.Root;
4141
var rootElement = response.RootElement ?? RootElement;
4242

43-
if (rootElement != null && doc.Root != null)
44-
root = doc.Root.DescendantsAndSelf(rootElement.AsNamespaced(Namespace)).SingleOrDefault();
43+
if (rootElement != null && doc.Root != null) {
44+
var namespacedRoot = rootElement.AsNamespaced(Namespace);
45+
// Prefer shallowest match to avoid nested elements with same name
46+
root = namespacedRoot != null
47+
? doc.Root.Element(namespacedRoot)
48+
?? doc.Root.DescendantsAndSelf(namespacedRoot)
49+
.OrderBy(e => e.Ancestors().Count())
50+
.FirstOrDefault()
51+
: null;
52+
}
4553

4654
// autodetect xml namespace
4755
if (Namespace.IsEmpty())
@@ -76,10 +84,16 @@ static void RemoveNamespace(XDocument xdoc) {
7684
? new(XNamespace.None.GetName(a.Name.LocalName), a.Value)
7785
: a
7886
)
87+
.Where(a => a != null)
7988
);
8089
}
8190
}
8291

92+
static bool IsValidXmlElementName(string name) {
93+
// Generic type names contain backtick (e.g., "List`1") which is invalid in XML element names
94+
return !name.Contains('`');
95+
}
96+
8397
protected virtual object Map(object x, XElement? root) {
8498
var objType = x.GetType();
8599
var props = objType.GetProperties();
@@ -236,7 +250,20 @@ protected virtual object Map(object x, XElement? root) {
236250
}
237251
else if (type.IsGenericType) {
238252
var list = (IList)Activator.CreateInstance(asType)!;
239-
var container = GetElementByName(root, name);
253+
XElement? container = null;
254+
255+
// First check if root itself is the container (matches property name)
256+
if (root != null && name?.LocalName != null) {
257+
var rootName = root.Name.LocalName;
258+
if (rootName.Equals(name.LocalName, StringComparison.OrdinalIgnoreCase)) {
259+
container = root;
260+
}
261+
}
262+
263+
// If root is not the container, try to find it as a child element
264+
if (container == null) {
265+
container = GetElementByName(root, name);
266+
}
240267

241268
if (container?.HasElements == true) {
242269
var first = container.Elements().FirstOrDefault();
@@ -306,37 +333,89 @@ object HandleListDerivative(XElement root, string propName, Type type) {
306333

307334
var list = (IList)Activator.CreateInstance(type)!;
308335

309-
IList<XElement> elements = root.Descendants(t.Name.AsNamespaced(Namespace)).ToList();
310-
311336
var name = t.Name;
312337
var attribute = t.GetAttribute<DeserializeAsAttribute>();
313338

314339
if (attribute != null)
315340
name = attribute.Name;
316341

317-
if (!elements.Any()) {
318-
var lowerName = name?.ToLower(Culture).AsNamespaced(Namespace);
319-
320-
elements = root.Descendants(lowerName).ToList();
342+
// Try to find a container element first using property name
343+
XElement? container = null;
344+
345+
// Try property name first (skip if it contains invalid XML name characters like ` in generic types)
346+
if (IsValidXmlElementName(propName)) {
347+
container = GetElementByName(root, propName.AsNamespaced(Namespace));
321348
}
322-
323-
if (!elements.Any()) {
324-
var camelName = name?.ToCamelCase(Culture).AsNamespaced(Namespace);
325-
326-
elements = root.Descendants(camelName).ToList();
349+
350+
// Check if root itself matches the container naming
351+
if (container == null && IsValidXmlElementName(propName)) {
352+
var rootName = root.Name.LocalName;
353+
var propNameLower = propName.ToLower(Culture);
354+
355+
if (rootName.Equals(propName, StringComparison.OrdinalIgnoreCase) ||
356+
rootName.Equals(propNameLower, StringComparison.OrdinalIgnoreCase)) {
357+
container = root;
358+
}
327359
}
360+
361+
IList<XElement> elements;
362+
363+
// If we found a container, get only its direct children (Elements)
364+
if (container != null && container.HasElements) {
365+
// Try to match item elements by name variations
366+
var itemName = t.Name.AsNamespaced(Namespace);
367+
var directChildren = container.Elements(itemName).ToList();
368+
369+
if (!directChildren.Any()) {
370+
var lowerName = name?.ToLower(Culture).AsNamespaced(Namespace);
371+
directChildren = container.Elements(lowerName).ToList();
372+
}
373+
374+
if (!directChildren.Any()) {
375+
var camelName = name?.ToCamelCase(Culture).AsNamespaced(Namespace);
376+
directChildren = container.Elements(camelName).ToList();
377+
}
378+
379+
if (!directChildren.Any()) {
380+
directChildren = container.Elements()
381+
.Where(e => e.Name.LocalName.RemoveUnderscoresAndDashes() == name)
382+
.ToList();
383+
}
384+
385+
if (!directChildren.Any()) {
386+
var lowerName = name?.ToLower(Culture);
387+
directChildren = container.Elements()
388+
.Where(e => e.Name.LocalName.RemoveUnderscoresAndDashes() == lowerName)
389+
.ToList();
390+
}
391+
392+
elements = directChildren;
393+
}
394+
else {
395+
// Fallback: No container found, use Descendants for backward compatibility
396+
elements = root.Descendants(t.Name.AsNamespaced(Namespace)).ToList();
328397

329-
if (!elements.Any())
330-
elements = root.Descendants()
331-
.Where(e => e.Name.LocalName.RemoveUnderscoresAndDashes() == name)
332-
.ToList();
398+
if (!elements.Any()) {
399+
var lowerName = name?.ToLower(Culture).AsNamespaced(Namespace);
400+
elements = root.Descendants(lowerName).ToList();
401+
}
333402

334-
if (!elements.Any()) {
335-
var lowerName = name?.ToLower(Culture).AsNamespaced(Namespace);
403+
if (!elements.Any()) {
404+
var camelName = name?.ToCamelCase(Culture).AsNamespaced(Namespace);
405+
elements = root.Descendants(camelName).ToList();
406+
}
336407

337-
elements = root.Descendants()
338-
.Where(e => e.Name.LocalName.RemoveUnderscoresAndDashes() == lowerName)
339-
.ToList();
408+
if (!elements.Any())
409+
elements = root.Descendants()
410+
.Where(e => e.Name.LocalName.RemoveUnderscoresAndDashes() == name)
411+
.ToList();
412+
413+
if (!elements.Any()) {
414+
var lowerName = name?.ToLower(Culture);
415+
elements = root.Descendants()
416+
.Where(e => e.Name.LocalName.RemoveUnderscoresAndDashes() == lowerName)
417+
.ToList();
418+
}
340419
}
341420

342421
PopulateListFromElements(t, elements, list);

‎test/RestSharp.Tests.Serializers.Xml/SampleClasses/NestedElementTestClasses.cs‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
namespace RestSharp.Tests.Serializers.Xml.SampleClasses;
2+
3+
// Test classes for nested element bugs
4+
5+
public class Item {
6+
public int Id { get; set; }
7+
public List<Item> SubItems { get; set; }
8+
}
9+
10+
public class ItemContainer {
11+
public List<Item> Items { get; set; } = new();
12+
}
13+
14+
public class ItemWithGroup {
15+
public int Id { get; set; }
16+
public ItemGroup Group { get; set; }
17+
}
18+
19+
public class ItemGroup {
20+
public List<Item> Items { get; set; }
21+
}
22+
23+
public class ItemsResponse {
24+
public List<ItemWithGroup> Items { get; set; } = new();
25+
}

‎test/RestSharp.Tests.Serializers.Xml/XmlDeserializerTests.cs‎

Lines changed: 165 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1076,4 +1076,168 @@ public void Ignore_ReadOnly_Property_That_Exists_In_Data() {
10761076

10771077
Assert.Null(p.ReadOnlyProxy);
10781078
}
1079-
}
1079+
1080+
[Fact]
1081+
public void Deserialize_Nested_List_Should_Not_Include_Deeply_Nested_Items() {
1082+
// Bug #1 test: HandleListDerivative should use Elements() on containers, not Descendants()
1083+
const string xml = """
1084+
<root>
1085+
<items>
1086+
<item>
1087+
<id>1</id>
1088+
<subitems>
1089+
<item><id>2</id></item>
1090+
<item><id>3</id></item>
1091+
</subitems>
1092+
</item>
1093+
</items>
1094+
</root>
1095+
""";
1096+
1097+
var deserializer = new XmlDeserializer();
1098+
var result = deserializer.Deserialize<ItemContainer>(new RestResponse { Content = xml })!;
1099+
1100+
// Should only have 1 item (id=1), not 3 (id=1,2,3)
1101+
Assert.NotNull(result);
1102+
Assert.NotNull(result.Items);
1103+
Assert.Single(result.Items);
1104+
Assert.Equal(1, result.Items[0].Id);
1105+
}
1106+
1107+
[Fact]
1108+
public void Deserialize_RootElement_Should_Not_Throw_On_Duplicate_Nested_Names() {
1109+
// Bug #2 test: RootElement selection should handle duplicate names gracefully
1110+
const string xml = """
1111+
<root>
1112+
<items>
1113+
<item>
1114+
<id>72</id>
1115+
<group>
1116+
<items>
1117+
<item><id>74</id></item>
1118+
</items>
1119+
</group>
1120+
</item>
1121+
</items>
1122+
</root>
1123+
""";
1124+
1125+
var deserializer = new XmlDeserializer { RootElement = "items" };
1126+
1127+
// Should not throw InvalidOperationException: Sequence contains more than one element
1128+
var exception = Record.Exception(() =>
1129+
deserializer.Deserialize<ItemsResponse>(new RestResponse { Content = xml })
1130+
);
1131+
1132+
Assert.Null(exception);
1133+
}
1134+
1135+
[Fact]
1136+
public void Deserialize_RootElement_Should_Prefer_Shallowest_Match() {
1137+
// Bug #2 test: When multiple elements match RootElement, prefer the shallowest one
1138+
const string xml = """
1139+
<root>
1140+
<items>
1141+
<item>
1142+
<id>72</id>
1143+
<group>
1144+
<items>
1145+
<item><id>74</id></item>
1146+
</items>
1147+
</group>
1148+
</item>
1149+
</items>
1150+
</root>
1151+
""";
1152+
1153+
var deserializer = new XmlDeserializer { RootElement = "items" };
1154+
var result = deserializer.Deserialize<ItemsResponse>(new RestResponse { Content = xml })!;
1155+
1156+
// Should deserialize from the top-level <items>, not the nested one
1157+
Assert.NotNull(result);
1158+
Assert.NotNull(result.Items);
1159+
Assert.Single(result.Items);
1160+
Assert.Equal(72, result.Items[0].Id);
1161+
}
1162+
1163+
[Fact]
1164+
public void Deserialize_RootElement_Should_Try_Direct_Child_First() {
1165+
// Bug #2 test: Should try Element() before DescendantsAndSelf()
1166+
const string xml = """
1167+
<root>
1168+
<data>
1169+
<one>direct</one>
1170+
</data>
1171+
</root>
1172+
""";
1173+
1174+
var deserializer = new XmlDeserializer { RootElement = "data" };
1175+
var result = deserializer.Deserialize<SimpleStruct>(new RestResponse { Content = xml });
1176+
1177+
Assert.Equal("direct", result.One);
1178+
}
1179+
1180+
[Fact]
1181+
public void Deserialize_List_With_Container_Should_Use_Direct_Children_Only() {
1182+
// Test that container-based list deserialization uses Elements() not Descendants()
1183+
const string xml = """
1184+
<root>
1185+
<items>
1186+
<item>
1187+
<id>10</id>
1188+
<subitems>
1189+
<item><id>20</id></item>
1190+
</subitems>
1191+
</item>
1192+
<item>
1193+
<id>11</id>
1194+
</item>
1195+
</items>
1196+
</root>
1197+
""";
1198+
1199+
var deserializer = new XmlDeserializer();
1200+
var result = deserializer.Deserialize<ItemContainer>(new RestResponse { Content = xml })!;
1201+
1202+
Assert.NotNull(result.Items);
1203+
Assert.Equal(2, result.Items.Count);
1204+
Assert.Equal(10, result.Items[0].Id);
1205+
Assert.Equal(11, result.Items[1].Id);
1206+
}
1207+
1208+
[Fact]
1209+
public void Deserialize_Nested_Items_Should_Also_Use_Direct_Children() {
1210+
// Test that nested subitems also correctly use direct children
1211+
const string xml = """
1212+
<root>
1213+
<items>
1214+
<item>
1215+
<id>1</id>
1216+
<subitems>
1217+
<item><id>2</id></item>
1218+
<item>
1219+
<id>3</id>
1220+
<subitems>
1221+
<item><id>4</id></item>
1222+
</subitems>
1223+
</item>
1224+
</subitems>
1225+
</item>
1226+
</items>
1227+
</root>
1228+
""";
1229+
1230+
var deserializer = new XmlDeserializer();
1231+
var result = deserializer.Deserialize<ItemContainer>(new RestResponse { Content = xml })!;
1232+
1233+
Assert.NotNull(result.Items);
1234+
Assert.Single(result.Items);
1235+
1236+
var topItem = result.Items[0];
1237+
Assert.Equal(1, topItem.Id);
1238+
Assert.NotNull(topItem.SubItems);
1239+
Assert.Equal(2, topItem.SubItems.Count);
1240+
Assert.Equal(2, topItem.SubItems[0].Id);
1241+
Assert.Equal(3, topItem.SubItems[1].Id);
1242+
}
1243+
}

0 commit comments

Comments
 (0)