Use relaxed JSON escaping for log bodies - #340
Conversation
The body formatter and batch serializer build Utf8JsonWriters with no options, so they use the default HTML-safe JavaScriptEncoder, which unicode-escapes every quote, ', <, >, & and all non-ASCII characters. Because Serilog quotes string property values by default, this makes most stored Loki lines unreadable (a regression from v8.x, which formatted bodies with Serilog's JsonValueFormatter and standard escaping). Construct the writers with JsonWriterOptions(Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping). Output stays valid JSON and UTF-8, so consumers (including the json parser) are unaffected. Add a unit test covering markup and non-ASCII characters.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds a shared JSON writer helper and switches formatter and payload serialization to use relaxed escaping. Unit tests verify that quotes, markup, and non-ASCII characters remain readable in the emitted output. ChangesRelaxed JSON escaping update
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/Serilog.Sinks.Grafana.Loki.UnitTests/WireFormatTests.fs`:
- Around line 255-267: The WireFormatTests ``body: quotes, markup and non-ASCII
are not unicode-escaped`` test currently checks angle brackets, ampersands, and
non-ASCII, but it never verifies quote handling. Update the assertions on
handler.LastBodyText to explicitly check that the quote character from mkInfo in
the payload is emitted as a literal quote and not escaped as \u0022, using the
existing test setup and the raw string checks already in place.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7bc04844-db9c-4a82-95f0-f3e0706a8169
📒 Files selected for processing (3)
src/Serilog.Sinks.Grafana.Loki/LokiJsonTextFormatter.fssrc/Serilog.Sinks.Grafana.Loki/Serialization.fstests/Serilog.Sinks.Grafana.Loki.UnitTests/WireFormatTests.fs
Addresses CodeRabbit review on serilog-contrib#340. Assert at the decoded body level (the raw HTTP payload embeds the body as a JSON string, double-escaping its quotes), so a regression back to " for quotes is caught.
The relaxed-encoder JsonWriterOptions was defined twice (formatter and serializer) with drifting comments, and each Utf8JsonWriter opted in per construction site: a future writer that forgot the options argument would silently revert to HTML-safe \uXXXX escaping with no compiler warning. Extract a single Infrastructure.JsonWriterDefaults.createWriter as the one way the sink builds a writer, so escaping cannot drift between sites. The shared comment also corrects the description: the relaxed encoder still escapes U+2028/U+2029 and DEL, so it is not "only what JSON mandates". Add the two missing wire-format guards: - the public LokiJsonTextFormatter.Format entry point (the sink fast path bypasses it, so its writer site had no test coverage); - promoted label values (the envelope writer escapes label keys/values too). The escaping tests sample neutral, multi-script non-ASCII (accented Latin plus CJK) rather than a single script.
Fixes #339.
Problem
The body formatter (
LokiJsonTextFormatter) and the batch serializer build theirUtf8JsonWriters with no options, so they useSystem.Text.Json's default HTML-safeJavaScriptEncoder. It escapes",',<,>,&and all non-ASCII as\uXXXX. Because Serilog quotes string property values by default, this makes most stored Loki lines unreadable — a regression from v8.x, which used Serilog'sJsonValueFormatter(standard escaping). See #339 for details.Fix
Construct the writers with
JsonWriterOptions(Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping), applied at all three writer sites (formatter body, batch envelope, reused body buffer) via a shared value.Before:
After:
Notes
| jsonqueries) are unaffected. "Unsafe" in the encoder name only refers to embedding JSON directly into HTML, which does not apply to Loki log bodies.<,>,&and non-ASCII survive verbatim.dotnet test(UnitTests, net10.0): 122/122 pass.Summary by CodeRabbit