JsonLayout - Changed SuppressSpaces to default true#5781
Conversation
snakefoot
added
json / json-layout
breaking behavior change
WalkthroughThis pull request adapts JSON output formatting in several NLog components. The key modifications include setting the default for the Changes
Sequence Diagram(s)sequenceDiagram
participant L as Logger
participant JL as JsonLayout
L->>JL: Set IndentJson = true
Note right of JL: Check IndentJson property
JL-->>JL: Set SuppressSpaces = false
JL->>L: Render compact JSON with indenting adjustments
sequenceDiagram
participant VF as ValueFormatter
participant SC as ServiceContainer
participant JC as IJsonConverter
participant JCS as JsonConverterWithSpaces
VF->>SC: Retrieve IJsonConverter instance
SC-->>VF: Return jsonConverter instance
VF->>JCS: Call CreateJsonConverter(jsonConverter)
alt jsonConverter is DefaultJsonSerializer
JCS-->>VF: Return wrapped JsonConverterWithSpaces
else
JCS-->>VF: Return original jsonConverter
end
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (14)
🚧 Files skipped from review as they are similar to previous changes (13)
🧰 Additional context used🧬 Code Graph Analysis (1)src/NLog/MessageTemplates/ValueFormatter.cs (2)
🔇 Additional comments (2)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
snakefoot
force-pushed
the
JsonSpaces
branch
2 times, most recently
from
3ecc646 to
f9f9c81
Compare
![coderabbitai[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/NLog/MessageTemplates/ValueFormatter.cs (1)
61-84: Good implementation of the JsonConverterWithSpaces wrapper classThe wrapper class provides a clean solution to maintain spaces in ValueFormatter JSON output while allowing the default to be changed to
SuppressSpaces=trueglobally. The implementation correctly handles both DefaultJsonSerializer and other converter types.Consider adding a brief XML comment to explain why this class explicitly sets
SuppressSpaces = falsewhen the PR's main purpose is to change the default totrueelsewhere, to help future maintainers understand this specific exception.+ /// <summary> + /// Wrapper for JSON converters that ensures spaces are included in JSON output, + /// overriding the global default which suppresses spaces. + /// </summary> private sealed class JsonConverterWithSpaces : IJsonConverter {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/NLog/Layouts/JSON/JsonArrayLayout.cs(1 hunks)src/NLog/Layouts/JSON/JsonLayout.cs(6 hunks)src/NLog/MessageTemplates/ValueFormatter.cs(1 hunks)src/NLog/Targets/DefaultJsonSerializer.cs(3 hunks)src/NLog/Targets/JsonSerializeOptions.cs(2 hunks)tests/NLog.UnitTests/LayoutRenderers/ExceptionTests.cs(1 hunks)tests/NLog.UnitTests/LayoutRenderers/VariableLayoutRendererTests.cs(1 hunks)tests/NLog.UnitTests/Layouts/CompoundLayoutTests.cs(2 hunks)tests/NLog.UnitTests/Layouts/JsonArrayLayoutTests.cs(4 hunks)tests/NLog.UnitTests/Layouts/JsonLayoutTests.cs(22 hunks)tests/NLog.UnitTests/LogMessageFormatterTests.cs(4 hunks)tests/NLog.UnitTests/LoggerTests.cs(3 hunks)tests/NLog.UnitTests/Targets/DefaultJsonSerializerClassTests.cs(2 hunks)tests/NLog.UnitTests/Targets/DefaultJsonSerializerTestsBase.cs(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/NLog.UnitTests/LayoutRenderers/ExceptionTests.cs
🚧 Files skipped from review as they are similar to previous changes (11)
- tests/NLog.UnitTests/Targets/DefaultJsonSerializerClassTests.cs
- tests/NLog.UnitTests/Targets/DefaultJsonSerializerTestsBase.cs
- src/NLog/Layouts/JSON/JsonArrayLayout.cs
- src/NLog/Targets/DefaultJsonSerializer.cs
- src/NLog/Targets/JsonSerializeOptions.cs
- tests/NLog.UnitTests/Layouts/JsonArrayLayoutTests.cs
- tests/NLog.UnitTests/LayoutRenderers/VariableLayoutRendererTests.cs
- tests/NLog.UnitTests/Layouts/JsonLayoutTests.cs
- tests/NLog.UnitTests/LogMessageFormatterTests.cs
- src/NLog/Layouts/JSON/JsonLayout.cs
- tests/NLog.UnitTests/LoggerTests.cs
🔇 Additional comments (3)
tests/NLog.UnitTests/Layouts/CompoundLayoutTests.cs (2)
55-55: Good adaptation to the new default!Great job setting
SuppressSpaces = falseexplicitly in the test. This properly maintains the test's expected behavior after changing the default value totruein theJsonLayoutclass. The expected output on line 74 includes spaces in the JSON, which is ensured by this property setting.
157-157: Good consistency in XML configuration!Correctly added the
suppressSpaces='false'attribute to the XML configuration to match the code-based configuration change. This ensures that both test methods continue to work with the same JSON formatting style, maintaining consistency between the two test approaches.src/NLog/MessageTemplates/ValueFormatter.cs (1)
55-58: The JsonConverter property now uses a wrapper to ensure spaces in JSON outputThis change ensures that ValueFormatter still produces JSON with spaces (
SuppressSpaces=false) while allowing the default to be changed totrueelsewhere, maintaining readable output in message templates.
snakefoot
force-pushed
the
JsonSpaces
branch
2 times, most recently
from
f48ae93 to
d6329ed
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/NLog/Layouts/JSON/JsonLayout.cs (1)
151-151: Optional: Allow indentation with selective spacing.
WhenIndentJsonis enabled,_suppressSpacesis forced tofalse. Though likely intended, consider a scenario in which a user wants indentation but minimal spacing around property delimiters. You could add a user-facing override if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/NLog/Layouts/JSON/JsonArrayLayout.cs(1 hunks)src/NLog/Layouts/JSON/JsonLayout.cs(6 hunks)src/NLog/MessageTemplates/ValueFormatter.cs(1 hunks)src/NLog/Targets/DefaultJsonSerializer.cs(3 hunks)src/NLog/Targets/JsonSerializeOptions.cs(2 hunks)tests/NLog.UnitTests/LayoutRenderers/ExceptionTests.cs(1 hunks)tests/NLog.UnitTests/LayoutRenderers/VariableLayoutRendererTests.cs(1 hunks)tests/NLog.UnitTests/Layouts/CompoundLayoutTests.cs(2 hunks)tests/NLog.UnitTests/Layouts/JsonArrayLayoutTests.cs(4 hunks)tests/NLog.UnitTests/Layouts/JsonLayoutTests.cs(22 hunks)tests/NLog.UnitTests/LogMessageFormatterTests.cs(4 hunks)tests/NLog.UnitTests/LoggerTests.cs(3 hunks)tests/NLog.UnitTests/Targets/DefaultJsonSerializerClassTests.cs(2 hunks)tests/NLog.UnitTests/Targets/DefaultJsonSerializerTestsBase.cs(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- tests/NLog.UnitTests/LayoutRenderers/VariableLayoutRendererTests.cs
- src/NLog/Targets/DefaultJsonSerializer.cs
- tests/NLog.UnitTests/LayoutRenderers/ExceptionTests.cs
- tests/NLog.UnitTests/Targets/DefaultJsonSerializerTestsBase.cs
- tests/NLog.UnitTests/Layouts/JsonLayoutTests.cs
- src/NLog/Layouts/JSON/JsonArrayLayout.cs
- tests/NLog.UnitTests/Layouts/CompoundLayoutTests.cs
- tests/NLog.UnitTests/LoggerTests.cs
- src/NLog/Targets/JsonSerializeOptions.cs
- tests/NLog.UnitTests/Targets/DefaultJsonSerializerClassTests.cs
- tests/NLog.UnitTests/Layouts/JsonArrayLayoutTests.cs
- tests/NLog.UnitTests/LogMessageFormatterTests.cs
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/NLog/MessageTemplates/ValueFormatter.cs (2)
src/NLog/Targets/DefaultJsonSerializer.cs (7)
DefaultJsonSerializer(47-757)DefaultJsonSerializer(69-72)SerializeObject(79-82)SerializeObject(90-146)SerializeObject(154-157)SerializeObject(166-169)SerializeObject(180-198)src/NLog/Targets/JsonSerializeOptions.cs (1)
JsonSerializeOptions(42-98)
🔇 Additional comments (6)
src/NLog/Layouts/JSON/JsonLayout.cs (4)
59-59: Use ofSuppressSpacesinLimitRecursionJsonConvertinstantiation looks good.
This properly initializes the custom JSON converter with the new parameter, aligning with the updated layout behavior.
76-80: Correctly passingsuppressSpacesto_serializerOptions.
Including theSuppressSpacesparameter in this constructor ensures consistent behavior throughout the JSON serialization process. No immediate issues detected.
114-129: Documentation and default value consistency.
Marking the XML documentation to “Default: true” matches the initialization of_suppressSpaces = true;This alignment helps maintain clarity for users configuring JSON layouts.
383-386: New inline delimiters match theSuppressSpacesgoal.
These default strings now exclude spaces, which is consistent withSuppressSpaces = true. The logic is straightforward and should produce more compact JSON.src/NLog/MessageTemplates/ValueFormatter.cs (2)
55-58: Lazy initialization ofJsonConverteris well-structured.
The property usesJsonConverterWithSpaces.CreateJsonConverter(...)to ensure any default or custom serializer is wrapped consistently. Implementation is clear and concise.
61-90: Verify forcingSuppressSpaces = falseinJsonConverterWithSpaces.
This class always setsSuppressSpaces = false, potentially ignoring user configuration. If that’s intentional, it might be worth clarifying, as it differs from other defaults that allow true/false.Do you want to confirm that no user-set “SuppressSpaces” option is overridden by this hard-coded false value? If needed, you could pass a parameter to preserve user preferences.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/NLog/MessageTemplates/ValueFormatter.cs (2)
61-89: Add XML documentation to explain the purpose of JsonConverterWithSpacesThe new JsonConverterWithSpaces class effectively forces
SuppressSpaces = falseregardless of global defaults. While the implementation is solid, adding XML documentation would clarify why this class exists and why it needs to override the default space suppression behavior.+ /// <summary> + /// Wrapper for JSON converters that ensures spaces are not suppressed in value formatting, + /// maintaining consistent behavior for message templates even when the global default changes. + /// </summary> private sealed class JsonConverterWithSpaces : IJsonConverter { private readonly Targets.DefaultJsonSerializer _serializer; private readonly Targets.JsonSerializeOptions _serializerOptions; private readonly Targets.JsonSerializeOptions _exceptionSerializerOptions; + /// <summary> + /// Creates a JsonConverter that ensures spaces are not suppressed if the provided converter is DefaultJsonSerializer. + /// Otherwise, returns the original converter. + /// </summary> + /// <param name="jsonConverter">The original JSON converter from the service provider</param> + /// <returns>A converter that maintains spaces in JSON output</returns> public static IJsonConverter CreateJsonConverter(IJsonConverter jsonConverter) { if (jsonConverter is Targets.DefaultJsonSerializer defaultJsonConverter) return new JsonConverterWithSpaces(defaultJsonConverter); else return jsonConverter; }
75-80: Consider making space suppression configurable rather than hard-codedThe
JsonConverterWithSpacesconstructor hard-codesSuppressSpaces = falsefor both regular and exception serialization. Consider making this configurable if there are scenarios where ValueFormatter might need to follow the global default.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/NLog/Layouts/JSON/JsonArrayLayout.cs(1 hunks)src/NLog/Layouts/JSON/JsonLayout.cs(7 hunks)src/NLog/MessageTemplates/ValueFormatter.cs(1 hunks)src/NLog/Targets/DefaultJsonSerializer.cs(3 hunks)src/NLog/Targets/JsonSerializeOptions.cs(2 hunks)tests/NLog.UnitTests/LayoutRenderers/ExceptionTests.cs(1 hunks)tests/NLog.UnitTests/LayoutRenderers/VariableLayoutRendererTests.cs(1 hunks)tests/NLog.UnitTests/Layouts/CompoundLayoutTests.cs(2 hunks)tests/NLog.UnitTests/Layouts/JsonArrayLayoutTests.cs(4 hunks)tests/NLog.UnitTests/Layouts/JsonLayoutTests.cs(22 hunks)tests/NLog.UnitTests/LogMessageFormatterTests.cs(4 hunks)tests/NLog.UnitTests/LoggerTests.cs(3 hunks)tests/NLog.UnitTests/Targets/DefaultJsonSerializerClassTests.cs(2 hunks)tests/NLog.UnitTests/Targets/DefaultJsonSerializerTestsBase.cs(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- tests/NLog.UnitTests/LayoutRenderers/VariableLayoutRendererTests.cs
- tests/NLog.UnitTests/Layouts/CompoundLayoutTests.cs
- src/NLog/Targets/DefaultJsonSerializer.cs
- tests/NLog.UnitTests/LayoutRenderers/ExceptionTests.cs
- tests/NLog.UnitTests/Targets/DefaultJsonSerializerTestsBase.cs
- src/NLog/Targets/JsonSerializeOptions.cs
- tests/NLog.UnitTests/Layouts/JsonLayoutTests.cs
- src/NLog/Layouts/JSON/JsonArrayLayout.cs
- tests/NLog.UnitTests/Targets/DefaultJsonSerializerClassTests.cs
- tests/NLog.UnitTests/Layouts/JsonArrayLayoutTests.cs
- tests/NLog.UnitTests/LoggerTests.cs
- tests/NLog.UnitTests/LogMessageFormatterTests.cs
- src/NLog/Layouts/JSON/JsonLayout.cs
🔇 Additional comments (2)
src/NLog/MessageTemplates/ValueFormatter.cs (2)
55-58: Modification of JsonConverter property to use JsonConverterWithSpaces wrapperThe JsonConverter property now uses a wrapper class to ensure that spaces are not suppressed in ValueFormatter, even though the default for JsonLayout is changing to suppress spaces. This maintains backward compatibility for the message template formatting.
82-88: Method implements correct conditional serialization based on value typeThe
SerializeObjectmethod correctly differentiates between exceptions and other objects, applying the appropriate serialization options for each case. This ensures consistent formatting while properly sanitizing dictionary keys for exceptions.
snakefoot
force-pushed
the
JsonSpaces
branch
2 times, most recently
from
04b9a52 to
32476b1
Compare
|
|
Updated wiki: https://github.com/NLog/NLog/wiki/JsonLayout |
snakefoot
added
the
documentation done
1 participant
Resolves #5772