Skip to content

Commit bc2ae9c

Browse files
Fix XmlDeserializer when XML uses same tag in nested elements (#2339)
* 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 * Implement Qodo suggestion and simplify the fallback logic --------- Co-authored-by: Copilot <[email protected]>
1 parent 1abceab commit bc2ae9c

File tree

3 files changed

+307
-30
lines changed

3 files changed

+307
-30
lines changed

src/RestSharp.Serializers.Xml/XmlDeserializer.cs

Lines changed: 117 additions & 29 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 the shallowest match to avoid nested elements with the 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,15 @@ 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+
!name.Contains('`');
95+
8396
protected virtual object Map(object x, XElement? root) {
8497
var objType = x.GetType();
8598
var props = objType.GetProperties();
@@ -177,12 +190,12 @@ protected virtual object Map(object x, XElement? root) {
177190
}
178191
}
179192
else if (type.IsEnum) {
180-
var converted = type.AsType().FindEnumValue(value.ToString()!, Culture);
193+
var converted = type.AsType().FindEnumValue(value.ToString(), Culture);
181194

182195
prop.SetValue(x, converted, null);
183196
}
184197
else if (asType == typeof(Uri)) {
185-
var uri = new Uri(value.ToString()!, UriKind.RelativeOrAbsolute);
198+
var uri = new Uri(value.ToString(), UriKind.RelativeOrAbsolute);
186199

187200
prop.SetValue(x, uri, null);
188201
}
@@ -191,8 +204,8 @@ protected virtual object Map(object x, XElement? root) {
191204
}
192205
else if (asType == typeof(DateTime)) {
193206
value = DateFormat.IsNotEmpty()
194-
? DateTime.ParseExact(value.ToString()!, DateFormat!, Culture)
195-
: DateTime.Parse(value.ToString()!, Culture);
207+
? DateTime.ParseExact(value.ToString(), DateFormat!, Culture)
208+
: DateTime.Parse(value.ToString(), Culture);
196209

197210
prop.SetValue(x, value, null);
198211
}
@@ -219,24 +232,35 @@ protected virtual object Map(object x, XElement? root) {
219232
}
220233
}
221234
else if (asType == typeof(decimal)) {
222-
value = decimal.Parse(value.ToString()!, Culture);
235+
value = decimal.Parse(value.ToString(), Culture);
223236
prop.SetValue(x, value, null);
224237
}
225238
else if (asType == typeof(Guid)) {
226239
var raw = value.ToString();
227240

228-
value = string.IsNullOrEmpty(raw) ? Guid.Empty : new(value.ToString()!);
241+
value = string.IsNullOrEmpty(raw) ? Guid.Empty : new(value.ToString());
229242

230243
prop.SetValue(x, value, null);
231244
}
232245
else if (asType == typeof(TimeSpan)) {
233-
var timeSpan = XmlConvert.ToTimeSpan(value.ToString()!);
246+
var timeSpan = XmlConvert.ToTimeSpan(value.ToString());
234247

235248
prop.SetValue(x, timeSpan, null);
236249
}
237250
else if (type.IsGenericType) {
238251
var list = (IList)Activator.CreateInstance(asType)!;
239-
var container = GetElementByName(root, name);
252+
XElement? container = null;
253+
254+
// First check if root itself is the container (matches property name)
255+
if (root != null && name?.LocalName != null) {
256+
var rootName = root.Name.LocalName;
257+
if (rootName.Equals(name.LocalName, StringComparison.OrdinalIgnoreCase)) {
258+
container = root;
259+
}
260+
}
261+
262+
// If root is not the container, try to find it as a child element
263+
container ??= GetElementByName(root, name);
240264

241265
if (container?.HasElements == true) {
242266
var first = container.Elements().FirstOrDefault();
@@ -261,7 +285,7 @@ protected virtual object Map(object x, XElement? root) {
261285
else {
262286
//fallback to type converters if possible
263287

264-
if (TryGetFromString(value.ToString()!, out var result, asType)) {
288+
if (TryGetFromString(value.ToString(), out var result, asType)) {
265289
prop.SetValue(x, result, null);
266290
}
267291
else {
@@ -306,37 +330,40 @@ object HandleListDerivative(XElement root, string propName, Type type) {
306330

307331
var list = (IList)Activator.CreateInstance(type)!;
308332

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

314336
if (attribute != null)
315337
name = attribute.Name;
316338

317-
if (!elements.Any()) {
318-
var lowerName = name?.ToLower(Culture).AsNamespaced(Namespace);
339+
// Try to find a container element first using the property name
340+
XElement? container = null;
319341

320-
elements = root.Descendants(lowerName).ToList();
342+
// Try the property name first (skip if it contains invalid XML name characters like ` in generic types)
343+
if (IsValidXmlElementName(propName)) {
344+
container = GetElementByName(root, propName.AsNamespaced(Namespace));
321345
}
322346

323-
if (!elements.Any()) {
324-
var camelName = name?.ToCamelCase(Culture).AsNamespaced(Namespace);
347+
// Check if root itself matches the container naming
348+
if (container == null && IsValidXmlElementName(propName)) {
349+
var rootName = root.Name.LocalName;
325350

326-
elements = root.Descendants(camelName).ToList();
351+
if (rootName.Equals(propName, StringComparison.OrdinalIgnoreCase)) {
352+
container = root;
353+
}
327354
}
328355

329-
if (!elements.Any())
330-
elements = root.Descendants()
331-
.Where(e => e.Name.LocalName.RemoveUnderscoresAndDashes() == name)
332-
.ToList();
356+
IList<XElement> elements;
333357

334-
if (!elements.Any()) {
335-
var lowerName = name?.ToLower(Culture).AsNamespaced(Namespace);
336-
337-
elements = root.Descendants()
338-
.Where(e => e.Name.LocalName.RemoveUnderscoresAndDashes() == lowerName)
339-
.ToList();
358+
// If we found a specific container, use Elements() to get only direct children
359+
// This prevents nested lists from being incorrectly flattened
360+
if (container is { HasElements: true }) {
361+
elements = TryFindElementsByNameVariations(container, t.Name, name, useDirectChildrenOnly: true);
362+
}
363+
else {
364+
// No container found - use Descendants() for backward compatibility
365+
// This handles cases where items are nested at varying depths without a clear container
366+
elements = TryFindElementsByNameVariations(root, t.Name, name, useDirectChildrenOnly: false);
340367
}
341368

342369
PopulateListFromElements(t, elements, list);
@@ -349,6 +376,67 @@ object HandleListDerivative(XElement root, string propName, Type type) {
349376
return list;
350377
}
351378

379+
IList<XElement> TryFindElementsByNameVariations(XElement source, string typeName, string? itemName, bool useDirectChildrenOnly) {
380+
IList<XElement> elements;
381+
382+
if (useDirectChildrenOnly) {
383+
// Use Elements() for direct children only
384+
elements = source.Elements(typeName.AsNamespaced(Namespace)).ToList();
385+
386+
if (!elements.Any()) {
387+
var lowerName = itemName?.ToLower(Culture).AsNamespaced(Namespace);
388+
elements = source.Elements(lowerName).ToList();
389+
}
390+
391+
if (!elements.Any()) {
392+
var camelName = itemName?.ToCamelCase(Culture).AsNamespaced(Namespace);
393+
elements = source.Elements(camelName).ToList();
394+
}
395+
396+
if (!elements.Any()) {
397+
elements = source.Elements()
398+
.Where(e => e.Name.LocalName.RemoveUnderscoresAndDashes() == itemName)
399+
.ToList();
400+
}
401+
402+
if (!elements.Any()) {
403+
var lowerName = itemName?.ToLower(Culture);
404+
elements = source.Elements()
405+
.Where(e => e.Name.LocalName.RemoveUnderscoresAndDashes() == lowerName)
406+
.ToList();
407+
}
408+
}
409+
else {
410+
// Use Descendants() for backward compatibility when no container is found
411+
elements = source.Descendants(typeName.AsNamespaced(Namespace)).ToList();
412+
413+
if (!elements.Any()) {
414+
var lowerName = itemName?.ToLower(Culture).AsNamespaced(Namespace);
415+
elements = source.Descendants(lowerName).ToList();
416+
}
417+
418+
if (!elements.Any()) {
419+
var camelName = itemName?.ToCamelCase(Culture).AsNamespaced(Namespace);
420+
elements = source.Descendants(camelName).ToList();
421+
}
422+
423+
if (!elements.Any()) {
424+
elements = source.Descendants()
425+
.Where(e => e.Name.LocalName.RemoveUnderscoresAndDashes() == itemName)
426+
.ToList();
427+
}
428+
429+
if (!elements.Any()) {
430+
var lowerName = itemName?.ToLower(Culture);
431+
elements = source.Descendants()
432+
.Where(e => e.Name.LocalName.RemoveUnderscoresAndDashes() == lowerName)
433+
.ToList();
434+
}
435+
}
436+
437+
return elements;
438+
}
439+
352440
protected virtual object? CreateAndMap(Type t, XElement element) {
353441
object? item;
354442

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+
}

0 commit comments

Comments
 (0)