Bring XmlReader{Async} to one behavior #51736
Conversation
|
Tagging subscribers to this area: @buyaa-n, @krwq Issue Detailssee XmlRederAsync.ReadElementContentAsAsync
|
|
@kronic are there any functional differences between those two calls? If yes please update the issue with all info, if not please update the issue we can easily tell the code is the same. This is needed for reference in case we get a regression or if this is intentional break we can understand why this change happened (make sure links use specific commit and not |
|
@krwq Yes, there are differences in return types XmlAtomicValue, XPathItem, XmlAtomicValue[], XPathItem[], XPathNavigator[] |
|
@kronic can you give more specific examples of differences? i.e. input and API output |
|
@krwq In the first case there will be a mistake, in the second there is no using System;
using System.IO;
using System.Xml;
using System.Xml.Schema;
const string xml = "<test a=\"sdf\"/>";
//1
try
{
using var xmlReader = XmlReader.Create(new StringReader(xml));
xmlReader.MoveToContent();
xmlReader.MoveToAttribute(0);
if (xmlReader.ReadContentAs(typeof(XmlAtomicValue), null) is XmlAtomicValue xmlAtomic)
{
//{"Content cannot be converted to the type System.Xml.Schema.XmlAtomicValue. Line 1, position 7."}
Console.WriteLine(xmlAtomic.Value);
}
}
catch (Exception ex)
{
Console.WriteLine(ex);
}
//2
try
{
using var xmlReader = XmlReader.Create(new StringReader(xml), new XmlReaderSettings {Async = true});
await xmlReader.MoveToContentAsync();
xmlReader.MoveToAttribute(0);
if (await xmlReader.ReadContentAsAsync(typeof(XmlAtomicValue), null) is XmlAtomicValue xmlAtomic)
{
//ok
Console.WriteLine(xmlAtomic.Value);
}
}
catch (Exception ex)
{
Console.WriteLine(ex);
} |
|
@krwq ping |
| @@ -534,7 +534,7 @@ public virtual object ReadElementContentAs(Type returnType, IXmlNamespaceResolve | |||
| return value; | |||
| } | |||
|
|
|||
| return returnType == typeof(string) ? string.Empty : XmlUntypedStringConverter.Instance.FromString(string.Empty, returnType, namespaceResolver); | |||
| return returnType == typeof(string) ? string.Empty : XmlUntypedConverter.Untyped.ChangeType(string.Empty, returnType, namespaceResolver); | |||
There was a problem hiding this comment.
I believe the new implementation will not work with list types correctly. I think the correct solution might be just changing Untyped into UntypedList but you will need to add tests which cover this.
There was a problem hiding this comment.
XmlRederAsync use Untyped
|
@kronic sorry for late re-review. You will need to add more tests related to list types as I think there are some differences in the implementation (by default XmlUntypedStringConverter.Instance handles lists while XmlUntypedConverter.Untyped doesn't). I think the solution is to change to XmlUntypedConverter.UntypedList but you will need to add tests to confirm everything works. Make sure to analyze both implementations and ensure there are no differences in behavior. |
terrajobst
added
the
community-contribution
krwq
added
the
NO-MERGE
|
no merge until RC1 snap |
|
@krwq done |
krwq
removed
the
NO-MERGE
| Assert.Equal(4, values.Length); | ||
| Assert.Equal(1, values[0]); | ||
| Assert.Equal(2, values[1]); | ||
| Assert.Equal(3, values[2]); | ||
| Assert.Equal(4, values[3]); | ||
| } | ||
| [Fact] |
There was a problem hiding this comment.
nit: new line between } and [Fact] - here and everywhere
|
/azp list |
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
LGTM assuming no related failures in outerloop tests - we should watch closely for any potential regressions caused by this PR since there might be some obscure cases which we might hit |
ghost
locked as resolved and limited conversation to collaborators
see
XmlRederAsync.ReadElementContentAsAsync
XmlRederAsync.ReadContentAsAsync
fixes #52646