Use Utf8JsonWriter in EventSourceLogger #1227
Conversation
| @@ -692,7 +692,7 @@ private static class EventTypes | |||
|
|
|||
| { "E5JS", (e) => VerifySingleEvent(e, "Logger2", EventTypes.MessageJson, 5, null, LogLevel.Critical, | |||
| @"""ArgumentsJson"":{""stringParam"":""bar"",""int1Param"":""23"",""int2Param"":""45""", | |||
| @"""ExceptionJson"":{""TypeName"":""System.Exception"",""Message"":""oops"",""HResult"":""-2146233088"",""VerboseMessage"":""System.Exception: oops ---> System.Exception: inner oops") }, | |||
| @"""ExceptionJson"":{""TypeName"":""System.Exception"",""Message"":""oops"",""HResult"":""-2146233088"",""VerboseMessage"":""System.Exception: oops ---\u003e System.Exception: inner oops") }, | |||
There was a problem hiding this comment.
?
I mean, it's the same, right? \u003e should be >
There was a problem hiding this comment.
Yep, Utf8JsonWriter just decides to escape it for some reason. The end result is equivalent so I didn't spend too much time digging.
There was a problem hiding this comment.
// Only allow ASCII characters between ' ' (0x20) and '~' (0x7E), inclusively,
// but exclude characters that need to be escaped as hex: '"', '\'', '&', '+', '<', '>', '`'
// and exclude characters that need to be escaped by adding a backslash: '\n', '\r', '\t', '\\', '/', '\b', '\f'It's surprising to me that < needs to be escaped in JSON. @ahsonkhan, any particular reason for this? Mostly curious but it does seem like a user would be surprised to have it escaped in a string...
There was a problem hiding this comment.
The JSON escaping here is following a strict mode to be SDL compliant. This is following the semantics of the default Javascript escaper which also escapes all non-ascii characters (and some ascii as well, including those that show up in html). This is primarily for defense in depth since JSON payloads could be embedded within html.
We currently do not have a way to customize the escaping behavior, but I imagine a writer option to plug in a custom escaper. Something that would work with https://github.com/dotnet/corefx/issues/34830
Some related issues:
https://github.com/dotnet/corefx/issues/34958
https://github.com/dotnet/corefx/issues/35385
There was a problem hiding this comment.
Sounds fine to me. I mostly just wanted to confirm that this was an intentional choice because it was a little surprising. Thanks!
| var sw = new StringWriter(); | ||
| var writer = new JsonTextWriter(sw); | ||
| writer.DateFormatString = "O"; // ISO 8601 | ||
| var arrayBufferWriter = new ArrayBufferWriter<byte>(); |
There was a problem hiding this comment.
Hmm... using an internal type, which wasn't meant to be used here. But hey, source package :)
| </PropertyGroup> | ||
|
|
||
| <ItemGroup> | ||
| <Compile Include="../../shared/*.cs" /> | ||
|
|
||
| <Reference Include="Microsoft.Bcl.Json.Sources"> |
There was a problem hiding this comment.
Does this library have to support netstandard2.0?
There was a problem hiding this comment.
Yes, it was netstandard2.0 in 2.2 and I don't think we have plans to change that. Most of Microsoft.Extensions.* is designed to remain netstandard2.0
Fixes: dotnet/extensions#1013 Commit migrated from dotnet/extensions@0a8f632
Fixes: dotnet/extensions#1013 Commit migrated from dotnet/extensions@0a8f632
Fixes: dotnet/extensions#1013 Commit migrated from dotnet/extensions@0a8f632
Fixes: dotnet/extensions#1013 Commit migrated from dotnet/extensions@0a8f632
ghost
locked as resolved and limited conversation to collaborators
Fixes: #1013