Modify SqlStreamingXml XmlWriter to internally use a MemoryStream#3974
Modify SqlStreamingXml XmlWriter to internally use a MemoryStream#3974jimhblythe wants to merge 1 commit intodotnet:mainfrom
Conversation
…tead of a StringBuilder. (dotnet#1877) Note: UTF8Encoding(false) addition in s_writerSettings is consistent with prior default used within StringWriter/StringBuilder
|
@dotnet-policy-service agree |
| //_xmlWriter.WriteNode(_xmlReader, true); | ||
| // _xmlWriter.Flush(); | ||
| WriteXmlElement(); | ||
| // Update memoryStreamRemaining based on the number of chars just written to the MemoryStream |
There was a problem hiding this comment.
What's written to the MemoryStream is bytes, the number of bytes and chars can be quite different depending on encoding. It might be clearer to just refer to byte counts in the comment.
There was a problem hiding this comment.
I agree for a MemoryStream, but within method GetChars() the stream is passed into XmlWriter.Create() with specific Encoding = new UTF8Encoding(false) to prevent the BOM, as well as meet the XML expectation.
The only use for writing is within WriteXmlElement() consistent with only XML characters being written. Changing the comment for this specific method might dilute that only chars are written. Thoughts?
There was a problem hiding this comment.
Ultimately, the comment should assist future maintenance so I'm good with modifying it to achieve that goal while also being more inline with MemoryStream semantics.
Something felt unexpected about the prior StringWriter using the default UTF-8 encoding, and why I included "Note: UTF8Encoding(false)" within the PR comment. Perhaps there is a different, existing issue with SqlClient support of other encodings. I've seen references to using FOR XML with non-XML columns causing concerns.
2 participants
Modify SqlStreamingXml XmlWriter to internally use a MemoryStream instead of a StringBuilder.
Note: UTF8Encoding(false) addition in s_writerSettings is consistent with prior default used within StringWriter/StringBuilder
Issues
Fixes #1877 to be O(n)
Testing
Added 2 new Manual tests to ensure linear behavior for single large node, and secondary validation for multiple nodes
