Skip to content

Fixed incorrect logic in XmlReader's example (#43799) #44054

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Dec 31, 2024

Conversation

scale-tone
Copy link
Contributor

@scale-tone scale-tone commented Dec 21, 2024

(#43799 is now 3 weeks old without even being triaged, so fixing it myself)

The code sample for XmlReader is incorrect. It relies on the fact that the input XML has some formatting in it. Without formatting the code returns wrong results.

In more details, the issue happens because XElement.ReadFrom() reads the current element and positions the reader at the next element. When the XML has formatting, that next element will be of type Whitespace or smth - and everything works. But without formatting that next element will be the next Child element. Then the reader.Read() happens, which moves the reader to yet another next element. Hence the <Child Key=""02""> tag simply gets skipped.

Here is the code to address the issue.


Internal previews

📄 File 🔗 Preview link
docs/standard/linq/stream-xml-fragments-xmlreader.md How to stream XML fragments from an XmlReader (LINQ to XML)

@scale-tone scale-tone requested a review from a team as a code owner December 21, 2024 19:11
@dotnetrepoman dotnetrepoman bot added this to the December 2024 milestone Dec 21, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates PR is created by someone from the .NET community. label Dec 21, 2024
@scale-tone
Copy link
Contributor Author

@dotnet-policy-service agree

@gewarren gewarren requested a review from BillWagner December 23, 2024 22:06
@adegeo
Copy link
Contributor

adegeo commented Dec 31, 2024

Converting to review comment

Copy link
Contributor

@adegeo adegeo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @scale-tone sorry for the delay. The post US Thanksgiving and Christmas holiday season has a lot of people in and out of office.

Do you know if the VB code also suffers from this bug? It would have to be fixed too. If you're not interested in doing that, I'll schedule some time to take a look and modify your PR with the fixes. Cheers!

@scale-tone
Copy link
Contributor Author

Do you know if the VB code also suffers from this bug?

It does, and that's why this PR already contains a fix for VB part as well :)

@adegeo
Copy link
Contributor

adegeo commented Dec 31, 2024

OH GEEZ did I not spot that! HAHA

@adegeo
Copy link
Contributor

adegeo commented Dec 31, 2024

I totally scrolled down to it too and my brain just saw it as all the same snippet of code. 🤣😂

@adegeo
Copy link
Contributor

adegeo commented Dec 31, 2024

What do you think about modernizing the C# code for the reader too? The switch seems like the wrong check type here. We can eliminate a lot of indentation by changing using, swapping to if and using pattern matching on the type check. Thoughts?

static IEnumerable<XElement> StreamRootChildDoc(StringReader stringReader)
{
    using XmlReader reader = XmlReader.Create(stringReader);

    reader.MoveToContent();

    // Parse the file and display each of the nodes.
    while (true)
    {
        // If the current node is an element and named "Child"
        if (reader.NodeType == XmlNodeType.Element && reader.Name == "Child")
        {
            // Get the current node and advance the reader to the next
            if (XNode.ReadFrom(reader) is XElement el)
                yield return el;
            else
                continue;
        }
        else if (!reader.Read())
            break;
    }
}

Instead of

static IEnumerable<XElement> StreamRootChildDoc(StringReader stringReader)
{
    using (XmlReader reader = XmlReader.Create(stringReader))
    {
        reader.MoveToContent();
        // Parse the file and display each of the nodes.
        while (true)
        {
            switch (reader.NodeType)
            {
                case XmlNodeType.Element:
                    if (reader.Name == "Child") {
                        XElement? el = XNode.ReadFrom(reader) as XElement;
                        if (el != null)
                            yield return el;

                        // Should only call reader.Read(), if XNode.ReadFrom() was NOT called,
                        // because XNode.ReadFrom() advances the reader to the next element by itself.
                        continue;
                    }
                    break;
            }

            if (!reader.Read())
                break;
        }
    }
}

@adegeo
Copy link
Contributor

adegeo commented Dec 31, 2024

OH man, that VB code is soooooooooo old too. I'm going to update that.

@adegeo
Copy link
Contributor

adegeo commented Dec 31, 2024

OK that trimmed about 10 lines from the C# code and almost 70 lines from the VB code!

@scale-tone
Copy link
Contributor Author

Minus two more lines :) :

static IEnumerable<XElement> StreamRootChildDoc(StringReader stringReader)
{
    using XmlReader reader = XmlReader.Create(stringReader);

    reader.MoveToContent();

    // Parse the file and display each of the nodes.
    while (true)
    {
        // If the current node is an element and named "Child"
        if (reader.NodeType == XmlNodeType.Element && reader.Name == "Child")
        {
            // Get the current node and advance the reader to the next
            if (XNode.ReadFrom(reader) is XElement el)
                yield return el;
        }
        else if (!reader.Read())
            break;
    }
}

Copy link
Contributor

@adegeo adegeo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a bunch for the fix! Considering this area is low priority (usually based on page views), I'm unsure if we would have gotten to this. your contribution really matters and forces us to take a look. Thanks again!

@adegeo adegeo enabled auto-merge (squash) December 31, 2024 19:53
@adegeo adegeo merged commit f463647 into dotnet:main Dec 31, 2024
8 checks passed
@scale-tone
Copy link
Contributor Author

My pleasure, and happy holidays! :)

@adegeo
Copy link
Contributor

adegeo commented Dec 31, 2024

Proper link:

Fixes #43799

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-contribution Indicates PR is created by someone from the .NET community. dotnet-fundamentals/svc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants