From b528c1c7a817477865565278dec9470dee25003c Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Tue, 6 Jan 2026 23:27:34 +0100 Subject: [PATCH 1/2] 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 --- .../XmlDeserializer.cs | 125 ++++++++++--- .../SampleClasses/NestedElementTestClasses.cs | 25 +++ .../XmlDeserializerTests.cs | 166 +++++++++++++++++- 3 files changed, 292 insertions(+), 24 deletions(-) create mode 100644 test/RestSharp.Tests.Serializers.Xml/SampleClasses/NestedElementTestClasses.cs diff --git a/src/RestSharp.Serializers.Xml/XmlDeserializer.cs b/src/RestSharp.Serializers.Xml/XmlDeserializer.cs index 99959b221..e2cb46711 100644 --- a/src/RestSharp.Serializers.Xml/XmlDeserializer.cs +++ b/src/RestSharp.Serializers.Xml/XmlDeserializer.cs @@ -40,8 +40,16 @@ public class XmlDeserializer : IXmlDeserializer, IWithRootElement, IWithDateForm var root = doc.Root; var rootElement = response.RootElement ?? RootElement; - if (rootElement != null && doc.Root != null) - root = doc.Root.DescendantsAndSelf(rootElement.AsNamespaced(Namespace)).SingleOrDefault(); + if (rootElement != null && doc.Root != null) { + var namespacedRoot = rootElement.AsNamespaced(Namespace); + // Prefer shallowest match to avoid nested elements with same name + root = namespacedRoot != null + ? doc.Root.Element(namespacedRoot) + ?? doc.Root.DescendantsAndSelf(namespacedRoot) + .OrderBy(e => e.Ancestors().Count()) + .FirstOrDefault() + : null; + } // autodetect xml namespace if (Namespace.IsEmpty()) @@ -76,10 +84,16 @@ static void RemoveNamespace(XDocument xdoc) { ? new(XNamespace.None.GetName(a.Name.LocalName), a.Value) : a ) + .Where(a => a != null) ); } } + static bool IsValidXmlElementName(string name) { + // Generic type names contain backtick (e.g., "List`1") which is invalid in XML element names + return !name.Contains('`'); + } + protected virtual object Map(object x, XElement? root) { var objType = x.GetType(); var props = objType.GetProperties(); @@ -236,7 +250,20 @@ protected virtual object Map(object x, XElement? root) { } else if (type.IsGenericType) { var list = (IList)Activator.CreateInstance(asType)!; - var container = GetElementByName(root, name); + XElement? container = null; + + // First check if root itself is the container (matches property name) + if (root != null && name?.LocalName != null) { + var rootName = root.Name.LocalName; + if (rootName.Equals(name.LocalName, StringComparison.OrdinalIgnoreCase)) { + container = root; + } + } + + // If root is not the container, try to find it as a child element + if (container == null) { + container = GetElementByName(root, name); + } if (container?.HasElements == true) { var first = container.Elements().FirstOrDefault(); @@ -306,37 +333,89 @@ object HandleListDerivative(XElement root, string propName, Type type) { var list = (IList)Activator.CreateInstance(type)!; - IList elements = root.Descendants(t.Name.AsNamespaced(Namespace)).ToList(); - var name = t.Name; var attribute = t.GetAttribute(); if (attribute != null) name = attribute.Name; - if (!elements.Any()) { - var lowerName = name?.ToLower(Culture).AsNamespaced(Namespace); - - elements = root.Descendants(lowerName).ToList(); + // Try to find a container element first using property name + XElement? container = null; + + // Try property name first (skip if it contains invalid XML name characters like ` in generic types) + if (IsValidXmlElementName(propName)) { + container = GetElementByName(root, propName.AsNamespaced(Namespace)); } - - if (!elements.Any()) { - var camelName = name?.ToCamelCase(Culture).AsNamespaced(Namespace); - - elements = root.Descendants(camelName).ToList(); + + // Check if root itself matches the container naming + if (container == null && IsValidXmlElementName(propName)) { + var rootName = root.Name.LocalName; + var propNameLower = propName.ToLower(Culture); + + if (rootName.Equals(propName, StringComparison.OrdinalIgnoreCase) || + rootName.Equals(propNameLower, StringComparison.OrdinalIgnoreCase)) { + container = root; + } } + + IList elements; + + // If we found a container, get only its direct children (Elements) + if (container != null && container.HasElements) { + // Try to match item elements by name variations + var itemName = t.Name.AsNamespaced(Namespace); + var directChildren = container.Elements(itemName).ToList(); + + if (!directChildren.Any()) { + var lowerName = name?.ToLower(Culture).AsNamespaced(Namespace); + directChildren = container.Elements(lowerName).ToList(); + } + + if (!directChildren.Any()) { + var camelName = name?.ToCamelCase(Culture).AsNamespaced(Namespace); + directChildren = container.Elements(camelName).ToList(); + } + + if (!directChildren.Any()) { + directChildren = container.Elements() + .Where(e => e.Name.LocalName.RemoveUnderscoresAndDashes() == name) + .ToList(); + } + + if (!directChildren.Any()) { + var lowerName = name?.ToLower(Culture); + directChildren = container.Elements() + .Where(e => e.Name.LocalName.RemoveUnderscoresAndDashes() == lowerName) + .ToList(); + } + + elements = directChildren; + } + else { + // Fallback: No container found, use Descendants for backward compatibility + elements = root.Descendants(t.Name.AsNamespaced(Namespace)).ToList(); - if (!elements.Any()) - elements = root.Descendants() - .Where(e => e.Name.LocalName.RemoveUnderscoresAndDashes() == name) - .ToList(); + if (!elements.Any()) { + var lowerName = name?.ToLower(Culture).AsNamespaced(Namespace); + elements = root.Descendants(lowerName).ToList(); + } - if (!elements.Any()) { - var lowerName = name?.ToLower(Culture).AsNamespaced(Namespace); + if (!elements.Any()) { + var camelName = name?.ToCamelCase(Culture).AsNamespaced(Namespace); + elements = root.Descendants(camelName).ToList(); + } - elements = root.Descendants() - .Where(e => e.Name.LocalName.RemoveUnderscoresAndDashes() == lowerName) - .ToList(); + if (!elements.Any()) + elements = root.Descendants() + .Where(e => e.Name.LocalName.RemoveUnderscoresAndDashes() == name) + .ToList(); + + if (!elements.Any()) { + var lowerName = name?.ToLower(Culture); + elements = root.Descendants() + .Where(e => e.Name.LocalName.RemoveUnderscoresAndDashes() == lowerName) + .ToList(); + } } PopulateListFromElements(t, elements, list); diff --git a/test/RestSharp.Tests.Serializers.Xml/SampleClasses/NestedElementTestClasses.cs b/test/RestSharp.Tests.Serializers.Xml/SampleClasses/NestedElementTestClasses.cs new file mode 100644 index 000000000..d6a512c44 --- /dev/null +++ b/test/RestSharp.Tests.Serializers.Xml/SampleClasses/NestedElementTestClasses.cs @@ -0,0 +1,25 @@ +namespace RestSharp.Tests.Serializers.Xml.SampleClasses; + +// Test classes for nested element bugs + +public class Item { + public int Id { get; set; } + public List SubItems { get; set; } +} + +public class ItemContainer { + public List Items { get; set; } = new(); +} + +public class ItemWithGroup { + public int Id { get; set; } + public ItemGroup Group { get; set; } +} + +public class ItemGroup { + public List Items { get; set; } +} + +public class ItemsResponse { + public List Items { get; set; } = new(); +} diff --git a/test/RestSharp.Tests.Serializers.Xml/XmlDeserializerTests.cs b/test/RestSharp.Tests.Serializers.Xml/XmlDeserializerTests.cs index 5f26af53b..1c3454356 100644 --- a/test/RestSharp.Tests.Serializers.Xml/XmlDeserializerTests.cs +++ b/test/RestSharp.Tests.Serializers.Xml/XmlDeserializerTests.cs @@ -1076,4 +1076,168 @@ public void Ignore_ReadOnly_Property_That_Exists_In_Data() { Assert.Null(p.ReadOnlyProxy); } -} \ No newline at end of file + + [Fact] + public void Deserialize_Nested_List_Should_Not_Include_Deeply_Nested_Items() { + // Bug #1 test: HandleListDerivative should use Elements() on containers, not Descendants() + const string xml = """ + + + + 1 + + 2 + 3 + + + + + """; + + var deserializer = new XmlDeserializer(); + var result = deserializer.Deserialize(new RestResponse { Content = xml })!; + + // Should only have 1 item (id=1), not 3 (id=1,2,3) + Assert.NotNull(result); + Assert.NotNull(result.Items); + Assert.Single(result.Items); + Assert.Equal(1, result.Items[0].Id); + } + + [Fact] + public void Deserialize_RootElement_Should_Not_Throw_On_Duplicate_Nested_Names() { + // Bug #2 test: RootElement selection should handle duplicate names gracefully + const string xml = """ + + + + 72 + + + 74 + + + + + + """; + + var deserializer = new XmlDeserializer { RootElement = "items" }; + + // Should not throw InvalidOperationException: Sequence contains more than one element + var exception = Record.Exception(() => + deserializer.Deserialize(new RestResponse { Content = xml }) + ); + + Assert.Null(exception); + } + + [Fact] + public void Deserialize_RootElement_Should_Prefer_Shallowest_Match() { + // Bug #2 test: When multiple elements match RootElement, prefer the shallowest one + const string xml = """ + + + + 72 + + + 74 + + + + + + """; + + var deserializer = new XmlDeserializer { RootElement = "items" }; + var result = deserializer.Deserialize(new RestResponse { Content = xml })!; + + // Should deserialize from the top-level , not the nested one + Assert.NotNull(result); + Assert.NotNull(result.Items); + Assert.Single(result.Items); + Assert.Equal(72, result.Items[0].Id); + } + + [Fact] + public void Deserialize_RootElement_Should_Try_Direct_Child_First() { + // Bug #2 test: Should try Element() before DescendantsAndSelf() + const string xml = """ + + + direct + + + """; + + var deserializer = new XmlDeserializer { RootElement = "data" }; + var result = deserializer.Deserialize(new RestResponse { Content = xml }); + + Assert.Equal("direct", result.One); + } + + [Fact] + public void Deserialize_List_With_Container_Should_Use_Direct_Children_Only() { + // Test that container-based list deserialization uses Elements() not Descendants() + const string xml = """ + + + + 10 + + 20 + + + + 11 + + + + """; + + var deserializer = new XmlDeserializer(); + var result = deserializer.Deserialize(new RestResponse { Content = xml })!; + + Assert.NotNull(result.Items); + Assert.Equal(2, result.Items.Count); + Assert.Equal(10, result.Items[0].Id); + Assert.Equal(11, result.Items[1].Id); + } + + [Fact] + public void Deserialize_Nested_Items_Should_Also_Use_Direct_Children() { + // Test that nested subitems also correctly use direct children + const string xml = """ + + + + 1 + + 2 + + 3 + + 4 + + + + + + + """; + + var deserializer = new XmlDeserializer(); + var result = deserializer.Deserialize(new RestResponse { Content = xml })!; + + Assert.NotNull(result.Items); + Assert.Single(result.Items); + + var topItem = result.Items[0]; + Assert.Equal(1, topItem.Id); + Assert.NotNull(topItem.SubItems); + Assert.Equal(2, topItem.SubItems.Count); + Assert.Equal(2, topItem.SubItems[0].Id); + Assert.Equal(3, topItem.SubItems[1].Id); + } +} From 767b6d6a6578d626106dc33afe96c7d176b9f094 Mon Sep 17 00:00:00 2001 From: Alexey Zimarev Date: Mon, 12 Jan 2026 14:34:42 +0100 Subject: [PATCH 2/2] Implement Qodo suggestion and simplify the fallback logic --- .../XmlDeserializer.cs | 143 ++++++++++-------- 1 file changed, 76 insertions(+), 67 deletions(-) diff --git a/src/RestSharp.Serializers.Xml/XmlDeserializer.cs b/src/RestSharp.Serializers.Xml/XmlDeserializer.cs index e2cb46711..4b5a43d35 100644 --- a/src/RestSharp.Serializers.Xml/XmlDeserializer.cs +++ b/src/RestSharp.Serializers.Xml/XmlDeserializer.cs @@ -42,7 +42,7 @@ public class XmlDeserializer : IXmlDeserializer, IWithRootElement, IWithDateForm if (rootElement != null && doc.Root != null) { var namespacedRoot = rootElement.AsNamespaced(Namespace); - // Prefer shallowest match to avoid nested elements with same name + // Prefer the shallowest match to avoid nested elements with the same name root = namespacedRoot != null ? doc.Root.Element(namespacedRoot) ?? doc.Root.DescendantsAndSelf(namespacedRoot) @@ -89,10 +89,9 @@ static void RemoveNamespace(XDocument xdoc) { } } - static bool IsValidXmlElementName(string name) { + static bool IsValidXmlElementName(string name) => // Generic type names contain backtick (e.g., "List`1") which is invalid in XML element names - return !name.Contains('`'); - } + !name.Contains('`'); protected virtual object Map(object x, XElement? root) { var objType = x.GetType(); @@ -191,12 +190,12 @@ protected virtual object Map(object x, XElement? root) { } } else if (type.IsEnum) { - var converted = type.AsType().FindEnumValue(value.ToString()!, Culture); + var converted = type.AsType().FindEnumValue(value.ToString(), Culture); prop.SetValue(x, converted, null); } else if (asType == typeof(Uri)) { - var uri = new Uri(value.ToString()!, UriKind.RelativeOrAbsolute); + var uri = new Uri(value.ToString(), UriKind.RelativeOrAbsolute); prop.SetValue(x, uri, null); } @@ -205,8 +204,8 @@ protected virtual object Map(object x, XElement? root) { } else if (asType == typeof(DateTime)) { value = DateFormat.IsNotEmpty() - ? DateTime.ParseExact(value.ToString()!, DateFormat!, Culture) - : DateTime.Parse(value.ToString()!, Culture); + ? DateTime.ParseExact(value.ToString(), DateFormat!, Culture) + : DateTime.Parse(value.ToString(), Culture); prop.SetValue(x, value, null); } @@ -233,18 +232,18 @@ protected virtual object Map(object x, XElement? root) { } } else if (asType == typeof(decimal)) { - value = decimal.Parse(value.ToString()!, Culture); + value = decimal.Parse(value.ToString(), Culture); prop.SetValue(x, value, null); } else if (asType == typeof(Guid)) { var raw = value.ToString(); - value = string.IsNullOrEmpty(raw) ? Guid.Empty : new(value.ToString()!); + value = string.IsNullOrEmpty(raw) ? Guid.Empty : new(value.ToString()); prop.SetValue(x, value, null); } else if (asType == typeof(TimeSpan)) { - var timeSpan = XmlConvert.ToTimeSpan(value.ToString()!); + var timeSpan = XmlConvert.ToTimeSpan(value.ToString()); prop.SetValue(x, timeSpan, null); } @@ -261,9 +260,7 @@ protected virtual object Map(object x, XElement? root) { } // If root is not the container, try to find it as a child element - if (container == null) { - container = GetElementByName(root, name); - } + container ??= GetElementByName(root, name); if (container?.HasElements == true) { var first = container.Elements().FirstOrDefault(); @@ -288,7 +285,7 @@ protected virtual object Map(object x, XElement? root) { else { //fallback to type converters if possible - if (TryGetFromString(value.ToString()!, out var result, asType)) { + if (TryGetFromString(value.ToString(), out var result, asType)) { prop.SetValue(x, result, null); } else { @@ -339,93 +336,105 @@ object HandleListDerivative(XElement root, string propName, Type type) { if (attribute != null) name = attribute.Name; - // Try to find a container element first using property name + // Try to find a container element first using the property name XElement? container = null; - - // Try property name first (skip if it contains invalid XML name characters like ` in generic types) + + // Try the property name first (skip if it contains invalid XML name characters like ` in generic types) if (IsValidXmlElementName(propName)) { container = GetElementByName(root, propName.AsNamespaced(Namespace)); } - + // Check if root itself matches the container naming if (container == null && IsValidXmlElementName(propName)) { var rootName = root.Name.LocalName; - var propNameLower = propName.ToLower(Culture); - - if (rootName.Equals(propName, StringComparison.OrdinalIgnoreCase) || - rootName.Equals(propNameLower, StringComparison.OrdinalIgnoreCase)) { + + if (rootName.Equals(propName, StringComparison.OrdinalIgnoreCase)) { container = root; } } - + IList elements; - - // If we found a container, get only its direct children (Elements) - if (container != null && container.HasElements) { - // Try to match item elements by name variations - var itemName = t.Name.AsNamespaced(Namespace); - var directChildren = container.Elements(itemName).ToList(); - - if (!directChildren.Any()) { - var lowerName = name?.ToLower(Culture).AsNamespaced(Namespace); - directChildren = container.Elements(lowerName).ToList(); + + // If we found a specific container, use Elements() to get only direct children + // This prevents nested lists from being incorrectly flattened + if (container is { HasElements: true }) { + elements = TryFindElementsByNameVariations(container, t.Name, name, useDirectChildrenOnly: true); + } + else { + // No container found - use Descendants() for backward compatibility + // This handles cases where items are nested at varying depths without a clear container + elements = TryFindElementsByNameVariations(root, t.Name, name, useDirectChildrenOnly: false); + } + + PopulateListFromElements(t, elements, list); + + // get properties too, not just list items + // only if this isn't a generic type + if (!type.IsGenericType) + Map(list, root.Element(propName.AsNamespaced(Namespace)!) ?? root); + + return list; + } + + IList TryFindElementsByNameVariations(XElement source, string typeName, string? itemName, bool useDirectChildrenOnly) { + IList elements; + + if (useDirectChildrenOnly) { + // Use Elements() for direct children only + elements = source.Elements(typeName.AsNamespaced(Namespace)).ToList(); + + if (!elements.Any()) { + var lowerName = itemName?.ToLower(Culture).AsNamespaced(Namespace); + elements = source.Elements(lowerName).ToList(); } - - if (!directChildren.Any()) { - var camelName = name?.ToCamelCase(Culture).AsNamespaced(Namespace); - directChildren = container.Elements(camelName).ToList(); + + if (!elements.Any()) { + var camelName = itemName?.ToCamelCase(Culture).AsNamespaced(Namespace); + elements = source.Elements(camelName).ToList(); } - - if (!directChildren.Any()) { - directChildren = container.Elements() - .Where(e => e.Name.LocalName.RemoveUnderscoresAndDashes() == name) + + if (!elements.Any()) { + elements = source.Elements() + .Where(e => e.Name.LocalName.RemoveUnderscoresAndDashes() == itemName) .ToList(); } - - if (!directChildren.Any()) { - var lowerName = name?.ToLower(Culture); - directChildren = container.Elements() + + if (!elements.Any()) { + var lowerName = itemName?.ToLower(Culture); + elements = source.Elements() .Where(e => e.Name.LocalName.RemoveUnderscoresAndDashes() == lowerName) .ToList(); } - - elements = directChildren; } else { - // Fallback: No container found, use Descendants for backward compatibility - elements = root.Descendants(t.Name.AsNamespaced(Namespace)).ToList(); + // Use Descendants() for backward compatibility when no container is found + elements = source.Descendants(typeName.AsNamespaced(Namespace)).ToList(); if (!elements.Any()) { - var lowerName = name?.ToLower(Culture).AsNamespaced(Namespace); - elements = root.Descendants(lowerName).ToList(); + var lowerName = itemName?.ToLower(Culture).AsNamespaced(Namespace); + elements = source.Descendants(lowerName).ToList(); } if (!elements.Any()) { - var camelName = name?.ToCamelCase(Culture).AsNamespaced(Namespace); - elements = root.Descendants(camelName).ToList(); + var camelName = itemName?.ToCamelCase(Culture).AsNamespaced(Namespace); + elements = source.Descendants(camelName).ToList(); } - if (!elements.Any()) - elements = root.Descendants() - .Where(e => e.Name.LocalName.RemoveUnderscoresAndDashes() == name) + if (!elements.Any()) { + elements = source.Descendants() + .Where(e => e.Name.LocalName.RemoveUnderscoresAndDashes() == itemName) .ToList(); + } if (!elements.Any()) { - var lowerName = name?.ToLower(Culture); - elements = root.Descendants() + var lowerName = itemName?.ToLower(Culture); + elements = source.Descendants() .Where(e => e.Name.LocalName.RemoveUnderscoresAndDashes() == lowerName) .ToList(); } } - PopulateListFromElements(t, elements, list); - - // get properties too, not just list items - // only if this isn't a generic type - if (!type.IsGenericType) - Map(list, root.Element(propName.AsNamespaced(Namespace)!) ?? root); - - return list; + return elements; } protected virtual object? CreateAndMap(Type t, XElement element) {