Skip to content
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

Fix invalid behavior of MoveToAttribute methods #60556

Conversation

Maximys
Copy link
Contributor

@Maximys Maximys commented Oct 18, 2021

This PR fix Issue-34443.

@@ -1395,6 +1386,7 @@ public override bool MoveToAttribute(string name, string? namespaceURI)
return true;
}
_readerNav.RollBackMove(ref _curDepth);
_nodeType = savedNodeType;
Copy link
Member

Choose a reason for hiding this comment

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

do _curDepth or _bInReadBinary need any adjustments? (I've done quick tests with checking XmlReader.Depth and that looks consistent across readers so possibly the answer is no)

@krwq
Copy link
Member

krwq commented Dec 20, 2021

@Maximys I gave this a second look and per https://docs.microsoft.com/en-us/dotnet/api/system.xml.xmlreader.movetoattribute?view=net-6.0#System_Xml_XmlReader_MoveToAttribute_System_String_ the single string MoveToAttribute can take a qualified name while (localName, ns) does not. I.e. see implementation https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlTextReaderImpl.cs#L1043 here - can you add tests to ensure this scenario works? (I'm not sure if it worked without this change as well so might be worth validating that as well)

@krwq
Copy link
Member

krwq commented Jan 17, 2022

@Maximys have you perhaps had a chance to test out what I suggested above?

@Maximys
Copy link
Contributor Author

Maximys commented Jan 17, 2022

@krwq , am I correctly understand you, that your want to check is next XML will read fine or not:
"<root><child someNamespace:attr1='value1'><other /></child></root>"

@danmoseley
Copy link
Member

@krwq question for you above

@krwq
Copy link
Member

krwq commented Feb 11, 2022

@Maximys sorry for late response, I want you to exercise if XmlNodeReader.MoveToAttribute(string) will work correctly if you specify fully qualified name of the attribute (i.e. someNamespace:attr1 from your example but assuming that namespace is defined somewhere). If it doesn't work verify if it was also the case before your change.

@krwq
Copy link
Member

krwq commented Mar 4, 2022

@Maximys had a chance to look in the scenario I mentioned above?

@danmoseley
Copy link
Member

I'm going to close this due to not hearing back. Feel free to reopen if you wish to continue.

@danmoseley danmoseley closed this Mar 23, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Apr 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

XmlNodeReader MoveToAttribute bug, leaves XmlReader in inconsistent state
3 participants