Skip to content

Conversation

Copy link

Copilot AI commented Jan 6, 2026

  • Understand the repository structure and current implementation
  • Create test cases for nested list elements bug
    • Test case: nested elements should not be included in parent list
    • Test case: direct children only should be deserialized
    • Test case: backward compatibility with existing XML without containers
    • Test case: empty lists handled properly
    • Test case: container naming conventions (plural, lowercase, etc.)
  • Fix HandleListDerivative() method to use Elements() instead of Descendants()
    • Add logic to find container element using naming conventions
    • Use Elements() on container for direct children only
    • Fall back to Descendants() for backward compatibility
  • Fix RemoveNamespace() method to filter null attributes
  • Run tests to validate changes
  • Review and finalize
Original prompt

Problem

The XmlDeserializer.HandleListDerivative() method has a critical bug that causes incorrect deserialization when deserializing List<T> from XML that contains nested elements with the same name as the list item type.

Current Behavior

When deserializing List<Category> from XML like this:

<root>
    <categories>
        <category>
            <id>1</id>
            <name>Main Category</name>
            <subcategories>
                <category>
                    <id>2</id>
                    <name>Sub Category 1</name>
                </category>
                <category>
                    <id>3</id>
                    <name>Sub Category 2</name>
                </category>
            </subcategories>
        </category>
    </categories>
</root>

Expected Result: 1 category (id=1)
Actual Result: 3 categories (id=1, id=2, id=3) - incorrectly includes nested categories!

Root Cause

In HandleListDerivative() method (lines 309, 320, 326, 330, 337), the code uses root.Descendants() to find list elements:

IList<XElement> elements = root.Descendants(t.Name.AsNamespaced(Namespace)).ToList();

Problem: Descendants() searches the entire XML subtree, matching ALL elements with the given name regardless of nesting level. This means:

  • When searching for <category> elements from the root
  • It finds the main <category> at <categories>/<category>
  • But also finds nested <category> at <categories>/<category>/<subcategories>/<category>

This violates the expected behavior where a list property should only deserialize its direct children, not deeply nested elements.

Solution

Modify HandleListDerivative() to:

  1. First attempt to find a container element (e.g., <categories> for List<Category>)
  2. Use Elements() instead of Descendants() on the container to get only direct children
  3. Fall back to current Descendants() behavior only if no container is found (for backward compatibility)

Implementation Details

Step 1: Look for container elements using common naming conventions:

  • Plural form: categories for Category
  • Lowercase: category
  • Property name if provided

Step 2: When container found, use:

container.Elements() // Gets only direct children

Instead of:

root.Descendants() // Gets ALL descendants (current buggy behavior)

Step 3: Only use Descendants() as fallback when no container exists.

Files to Modify

  1. src/RestSharp.Serializers.Xml/XmlDeserializer.cs
    • Fix HandleListDerivative() method (lines 302-350)
    • Fix RemoveNamespace() method to filter null attributes (lines 70-79)

Additional Bug Fix

While fixing this, also address a related bug in RemoveNamespace() at line 70-79:

e.ReplaceAttributes(
    e.Attributes()
        .Select(a => a.IsNamespaceDeclaration ? null : ...)
);

Problem: Select() can return null values which ReplaceAttributes() cannot handle.
Fix: Add .Where(a => a != null) before passing to ReplaceAttributes().

Test Cases

Create tests verifying:

  1. Nested elements not included: List deserialization ignores deeply nested elements with same name
  2. Direct children included: Only direct children of container are deserialized
  3. Container matching: Various container naming conventions work (plural, lowercase, etc.)
  4. Backward compatibility: XML without containers still works
  5. Empty lists: Properly handles when no matching elements exist

Benefits

  • ✅ Fixes incorrect list deserialization with nested elements
  • ✅ Maintains backward compatibility with existing code
  • ✅ Follows XML deserialization best practices (hierarchical structure)
  • ✅ Prevents silent data corruption from including wrong elements
  • ✅ Works correctly with APIs that use nested structures with repeated element names

Breaking Changes

None - the fix maintains backward compatibility by falling back to Descendants() when no container element is found.

This pull request was created from Copilot chat.


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Co-authored-by: pumamood <14088232+pumamood@users.noreply.github.com>
@pumamood pumamood closed this Jan 6, 2026
Copilot AI requested a review from pumamood January 6, 2026 20:46
Copilot stopped work on behalf of pumamood due to an error January 6, 2026 20:46
Copilot AI added a commit that referenced this pull request Jan 6, 2026
- 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

Co-authored-by: pumamood <14088232+pumamood@users.noreply.github.com>
pumamood pushed a commit that referenced this pull request Jan 6, 2026
…sted 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
pumamood pushed a commit that referenced this pull request Jan 6, 2026
…sted 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants