dotnet
[API Proposal]: Add WriteValue(Guid/TimeSpan) in System.Xml.XmlWriter
Background and motivation
Writing Guids and TimeSpans in xml is quire normal practice these days. We should allow users to use it instead of casting them to sring and then using WriteValue(string) method or using WriteValue(object).
API Proposal
namespace System.Xml;
public class XmlWriter
{
public virtual void WriteValue(Guid value);
public virtual void WriteValue(TimeSpan value);
}
API Usage
XmlWriter writer;
writer.WriteValue(Guid.NewGuid());
writer.WriteValue(TimeSpan.FromHours(1));
Alternative Designs
We can make these methods internal and use them only in System.Private.Xml project.
Risks
none
Tagging subscribers to this area: @dotnet/area-system-xml See info in area-owners.md if you want to be subscribed.
Issue Details
Background and motivation
Writing Guids and TimeSpans in xml is quire normal practice these days. We should allow users to use it instead of casting them to sring and then using WriteValue(string) method or using WriteValue(object).
API Proposal
namespace System.Xml;
public class XmlWriter
{
public virtual void WriteValue(Guid value);
public virtual void WriteValue(TimeSpan value);
}
API Usage
XmlWriter writer;
writer.WriteValue(Guid.NewGuid());
writer.WriteValue(TimeSpan.FromHours(1));
Alternative Designs
We can make these methods internal and use them only in System.Private.Xml project.
Risks
none
| Author: | TrayanZapryanov |
|---|---|
| Assignees: | - |
| Labels: |
|
| Milestone: | - |
We should allow users to use it instead of casting them to sring and then using WriteValue(string) method or using WriteValue(object).
Perhaps we could simply add a WriteValue(ReadOnlySpan<char>) overload?
We should allow users to use it instead of casting them to sring and then using WriteValue(string) method or using WriteValue(object).
Perhaps we could simply add a
WriteValue(ReadOnlySpan<char>)overload?
Not exactly as we want to format things in a specific way . For example Timespan is serialized like this right now :
public static string ToString(TimeSpan value)
{
return new XsdDuration(value).ToString();
}
Both TimeSpan and Guid expose TryFormat methods that can be used to format into a span destination. A ReadOnlySpan overload should help unlock allocation-free formatting for arbitrary types.
@eiriktsarpalis I agree, just the clients should have some internal knowledge.
Instead of calling writer.WriteValue(TimeSpan.FromHours(1))
they should call
new XsdDuration(value).TryFormat(someSpan)
writer.WriteValue(someSpan)
Personally, I see good use for these proposed overloads (and I'd even add overloads for DateOnly and TimeOnly).
I have no strong opinion on adding TimeSpan or Guid overloads. For parity, the API proposal should contain equivalent overloads in XmlReader, the proposal needs updating to include them as well.
@krwq do you think they're worth adding?
I would just add that the format of TimeSpan, and especially DateOnly and even perhaps DateTime, has to be carefully chosen to ensure it works across all possible values. I'm thinking it has to be "cultureless" to work.
PS. For Guid, it's not that complicated, but still, even if we say that XmlWriter should write the standard "dashed" format, the XmlReader should be able to interpret both the "dashed" and the "N" format, in case XML is generated by something other than .NET or is user modified.
Main benefit will be in writing Guids in XmlWriter as we will save Guid.ToString() allocation. If you like I can remove TimeSpan as I do not expect too many TimeSpans serializations. I've added it as it is tricky and current implementation uses XsdDuration.ToString().
@TrayanZapryanov would it be possible to update the API proposal in the OP to include XmlReader equivalents?
Thanks. I'm deferring to @krwq on whether to mark this as api-ready-for-review as he is the subject matter expert of this codebase.
Small risk that I've found right now: Imagine code which is using XmlWriter.WriteValue(Guid.NewGuid()) - this will fallback to XmlWriter.WriteValue(object) if not overridden. Current implementation of it is using :
// Writes out the specified value.
public virtual void WriteValue(object value)
{
ArgumentNullException.ThrowIfNull(value);
WriteString(XmlUntypedConverter.Untyped.ToString(value, null));
}
and as XmlUntypedConverter.Untyped.ToString(value, null) is not supporting Guid - it will throw exception. With proposed change - this will be accepted or in the base class we need to throw same exception. I would prefer to accept and fallback to something like this :
// Writes out the specified value.
public virtual void WriteValue(Guid value)
{
WriteString(XmlConvert.ToString(value));
}
One more note: According Help documentation, WriteValue methods should map CLR types defined also in Xsd(or this is what I understand). So Guid is somehow "custom" and maybe should be skipped, but at least TimeSpan can be mapped to "time" : XSD Date and Time Data Types.
I'm curious - do we actually care about XmlReader APIs or we just want XmlSerializer work with those? Are you actually using reader/writer directly?
@krwq Nope, I am not using them. Proposal is just to use them when serializing primitive types in XmlSerializer.
I think we should start with trying to handle that in XmlSerializer directly and only add APIs to XmlReader/XmlWriter if there is clear benefit
@krwq You are right. I've added new performance issue. Should I close this proposal or you see some benefit in implementing it ?
I'm not the owner of serialization part of the XML but I'd suggest running profiler with some real-life like payload and seeing if making a fix makes any difference. If there is clear difference then you already got prototype 😄 if change is relatively simple might be still worth to send a patch - I'd personally only do it if my particular scenario I'm trying to improve requires it (i.e. I have app which I'm trying to improve and profiler tells me this has huge impact on it or I'm particularly focusing on allocations/perf in XML)