JsonLayout with support for dotted recursion (#5991)#5999
JsonLayout with support for dotted recursion (#5991)#5999
Conversation
|
Thanks for opening this pull request! |
WalkthroughAdds a public JsonLayout property Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant Logger
participant JsonLayout
participant ReflectionCache as ObjectReflectionCache
App->>Logger: Log(event with Properties)
Logger->>JsonLayout: Render(logEvent)
alt DottedRecursion = false
JsonLayout->>JsonLayout: AppendJsonPropertyValue (emit nested JSON)
JsonLayout-->>Logger: JSON (nested structure)
else DottedRecursion = true
JsonLayout->>JsonLayout: AppendPropertyValueInternal
JsonLayout->>JsonLayout: AppendFlattenedPropertyValue
loop Recursion (depth <= MaxRecursionLimit)
JsonLayout->>ReflectionCache: Get members / enumerate properties
ReflectionCache-->>JsonLayout: Member list
JsonLayout->>JsonLayout: FlattenObjectProperties (emit dotted keys)
end
JsonLayout-->>Logger: JSON (flattened dotted keys)
end
Logger-->>App: Output JSON
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas to inspect closely:
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
I'll check the analysis to see what can be done. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/NLog/Layouts/JSON/JsonLayout.cs (3)
377-384: Consider extracting the conditional logic into a helper method.The duplicated conditional logic here could be simplified by extracting it into a dedicated helper method that decides whether to use flattened or regular serialization, improving readability and maintainability.
- if (DottedRecursion) - { - AppendFlattenedPropertyValue(prop.Name, prop.Value, prop.Format, logEvent.FormatProvider, prop.CaptureType, sb, sb.Length == orgLength); - } - else - { - AppendJsonPropertyValue(prop.Name, prop.Value, prop.Format, logEvent.FormatProvider, prop.CaptureType, sb, sb.Length == orgLength); - } + AppendPropertyValue(prop.Name, prop.Value, prop.Format, logEvent.FormatProvider, prop.CaptureType, sb, sb.Length == orgLength);Add this helper method:
private void AppendPropertyValue(string propName, object? propertyValue, string? format, IFormatProvider? formatProvider, MessageTemplates.CaptureType captureType, StringBuilder sb, bool beginJsonMessage) { if (DottedRecursion) { AppendFlattenedPropertyValue(propName, propertyValue, format, formatProvider, captureType, sb, beginJsonMessage); } else { AppendJsonPropertyValue(propName, propertyValue, format, formatProvider, captureType, sb, beginJsonMessage); } }
549-559: Consider using pattern matching for cleaner type checking.The
IsSimpleValuemethod could be simplified using C# pattern matching for better readability.private bool IsSimpleValue(object? value) { - if (value is null) return true; - if (value is string) return true; - if (value is IConvertible convertible) - { - var typeCode = convertible.GetTypeCode(); - return typeCode != TypeCode.Object; - } - return false; + return value switch + { + null => true, + string => true, + IConvertible convertible => convertible.GetTypeCode() != TypeCode.Object, + _ => false + }; }
61-62: Make ObjectReflectionCache lazy-init thread-safe.
Replace the non‑synchronized null-coalescing init (_objectReflectionCache ?? (_objectReflectionCache = ...)) with a thread-safe pattern (e.g. Interlocked.CompareExchange like the repo uses for other caches, or Lazy with ExecutionAndPublication) to avoid races when JsonLayout is rendered concurrently.
Location: src/NLog/Layouts/JSON/JsonLayout.cs (lines 61–62).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/NLog/Layouts/JSON/JsonLayout.cs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/NLog/Layouts/JSON/JsonLayout.cs (3)
src/NLog/Internal/ObjectReflectionCache.cs (1)
ObjectReflectionCache(56-59)src/NLog/Internal/StringHelpers.cs (2)
StringHelpers(45-170)IsNullOrEmptyString(166-169)src/NLog/MessageTemplates/ValueFormatter.cs (3)
ValueFormatter(47-395)ValueFormatter(90-94)FormatValue(111-134)
🔇 Additional comments (1)
src/NLog/Layouts/JSON/JsonLayout.cs (1)
155-160: LGTM! Well-documented public API addition.The new
DottedRecursionproperty is properly documented with XML comments and follows the established patterns in the codebase.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/NLog.UnitTests/Layouts/JsonLayoutTests.cs (2)
1199-1227: Solid coverage for basic flattening; add a guard to ensure parent object isn’t rendered.After asserting the flattened keys, also assert the parent container isn’t emitted as a nested object to prevent regressions.
var result = jsonLayout.Render(logEventInfo); Assert.Contains("\"nestedObject.val\":1", result); Assert.Contains("\"nestedObject.val2\":\"value2\"", result); Assert.Contains("\"nestedObject.nestedLevel.val3\":3", result); Assert.Contains("\"nestedObject.nestedLevel.val4\":\"value4\"", result); + Assert.DoesNotContain("\"nestedObject\":{", result);
1229-1255: Strengthen the recursion-limit assertion.Also assert that deeper flattened paths aren’t present when the limit is reached.
var result = jsonLayout.Render(logEventInfo); Assert.Contains("\"level1.level2.level3\":", result); Assert.Contains("{\"val\":\"deepValue\"}", result); + Assert.DoesNotContain("\"level1.level2.level3.val\"", result);src/NLog/Layouts/JSON/JsonLayout.cs (1)
526-537: Edge-case: dots in member names can collide.Flattening uses '.' as a separator; if inner property names contain dots, collisions/ambiguity can occur. Consider documenting this or making the separator configurable (e.g., via an advanced option), or escaping dots when found.
Would you like a small follow-up PR to add a configurable separator or an escape strategy for '.'?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/NLog/Layouts/JSON/JsonLayout.cs(5 hunks)tests/NLog.UnitTests/Layouts/JsonLayoutTests.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/NLog.UnitTests/Layouts/JsonLayoutTests.cs (2)
src/NLog/Layouts/JSON/JsonLayout.cs (1)
JsonLayout(94-97)src/NLog/LogEventInfo.cs (13)
LogEventInfo(51-844)LogEventInfo(79-85)LogEventInfo(93-96)LogEventInfo(105-119)LogEventInfo(129-134)LogEventInfo(145-158)LogEventInfo(169-185)LogEventInfo(196-199)LogEventInfo(210-219)LogEventInfo(487-490)LogEventInfo(499-502)LogEventInfo(527-539)LogEventInfo(550-553)
src/NLog/Layouts/JSON/JsonLayout.cs (3)
src/NLog/Internal/ObjectReflectionCache.cs (1)
ObjectReflectionCache(56-59)src/NLog/Internal/StringHelpers.cs (2)
StringHelpers(45-170)IsNullOrEmptyString(166-169)src/NLog/MessageTemplates/ValueFormatter.cs (3)
ValueFormatter(47-395)ValueFormatter(90-94)FormatValue(111-134)
🔇 Additional comments (6)
src/NLog/Layouts/JSON/JsonLayout.cs (6)
61-63: Good: Lazily resolving ObjectReflectionCache.Approach aligns with existing service resolution patterns.
155-161: API addition LGTM.
DottedRecursionis a clear opt-in flag and well documented.
382-389: Correct switch to flattened vs. regular rendering.Conditional routing is clean and keeps non-flattened behavior intact.
435-466: Nice dedup: centralizing Serialize/Stringify handling.This resolves prior duplication and keeps escapes consistent.
490-500: Capture-type safe behavior in flattened path.Defer to shared handler for Serialize/Stringify; only flatten on Normal. Matches expected semantics.
502-508: Recursion guard + simple/null fast-paths are correct.Depth check and fallback serialization look good; empty-property filtering is respected.
Also applies to: 510-517
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/NLog/Layouts/JSON/JsonLayout.cs (6)
61-62: Release ObjectReflectionCache on CloseLayoutSet
_objectReflectionCache = null;inCloseLayout()to avoid retainingIServiceProviderand cached metadata across layout disposal.Apply in CloseLayout:
protected override void CloseLayout() { _jsonConverter = null; _valueFormatter = null; _precalculateLayouts = null; + _objectReflectionCache = null; base.CloseLayout(); }Also confirm
ResolveService<IServiceProvider>()cannot be null here; if it can, guard the ctor call.
155-161: Document scope and default for DottedRecursion
- Add
[DefaultValue(false)]for config introspection.- Clarify in XML doc whether this applies only to IncludeEventProperties (current implementation) to prevent confusion.
-/// <remarks>Default: <see langword="false"/></remarks> +/// <remarks>Default: <see langword="false"/>. Applies to event properties when <see cref="IncludeEventProperties"/> is true.</remarks> +[DefaultValue(false)] public bool DottedRecursion { get; set; }
377-384: Flattening only for event properties — confirm intent
DottedRecursionis applied to event properties, not GDC/Scope. If the option is intended to be global, mirror this in GDC/Scope blocks; otherwise, note the scope in docs.
521-529: Sanitize dotted names when deriving from dictionary/unknown keysWhen flattening, consider applying the same dictionary-key sanitization used by the default serializer before concatenation (e.g., via a shared helper) to keep behavior consistent with
SanitizeDictionaryKeys = true, and to handle dots within original keys deterministically.
532-542: Optional: broaden simple-type fast pathTo reduce reflection on hot paths, consider treating
Guid,DateTimeOffset,TimeSpan, andUrias simple values directly.
1-1: Ubuntu CI IOException on /tmp/*.tmp — likely test parallelism/collision
- Ensure file/archiving tests don’t run in parallel or use unique temp paths per test (e.g.,
Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString("N"))).- Open files with
FileOptions.DeleteOnClosewhere possible; ensure all streams are disposed.- If NLog archive/compression is involved, avoid sharing the same target/filename across concurrent tests; add retries on
IOExceptionin test helpers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/NLog/Layouts/JSON/JsonLayout.cs(5 hunks)tests/NLog.UnitTests/Layouts/JsonLayoutTests.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/NLog.UnitTests/Layouts/JsonLayoutTests.cs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-12T01:22:52.382Z
Learnt from: thatrajeevkr
PR: NLog/NLog#5999
File: src/NLog/Layouts/JSON/JsonLayout.cs:162-166
Timestamp: 2025-09-12T01:22:52.382Z
Learning: When suggesting StringBuilder optimizations for NLog layouts marked as [ThreadAgnostic], avoid shared StringBuilder fields as they create thread safety issues. Prefer simple string operations or ThreadLocal<StringBuilder> if allocation profiling shows a real performance impact.
Applied to files:
src/NLog/Layouts/JSON/JsonLayout.cs
🧬 Code graph analysis (1)
src/NLog/Layouts/JSON/JsonLayout.cs (3)
src/NLog/Internal/ObjectReflectionCache.cs (1)
ObjectReflectionCache(56-59)src/NLog/Internal/StringHelpers.cs (2)
StringHelpers(45-170)IsNullOrEmptyString(166-169)src/NLog/MessageTemplates/ValueFormatter.cs (3)
ValueFormatter(47-395)ValueFormatter(90-94)FormatValue(111-134)
🔇 Additional comments (5)
src/NLog/Layouts/JSON/JsonLayout.cs (5)
430-444: Serialize + MaxRecursionLimit semantics — please confirmYou bypass recursion limits only when
CaptureType.Serialize && MaxRecursionLimit <= 1(usingSerializeObjectNoLimit), but use the limited serializer otherwise. Validate this matches prior behavior and expectations forSerializewith higher limits.
481-483: LGTM: centralizing capture-type handlingDelegation to
AppendPropertyValueInternalkeeps logic single-sourced.
485-495: LGTM: correct delegation for flattening vs. non-flatteningReuse for Serialize/Stringify and flatten only for Normal is sound.
497-503: MaxRecursionLimit short-circuit is correctFalling back to regular serialization at/over the limit avoids deep recursion and preserves data.
526-529: LGTM: string.Concat for dotted namesUsing
string.Concatavoids sharedStringBuilderand aligns with thread-agnostic rendering.
|
@snakefoot How does this look? |
|
I like the approach with parsing Would it be possible that |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/NLog/Layouts/JSON/JsonLayout.cs (1)
507-527: Consider null check for serializer cast.While the cast to
Targets.DefaultJsonSerializeris checked for null (line 508), the fallback in line 527 will use the standard non-flattened approach. This is safe but might be unexpected behavior if a custom JSON converter is configured.Consider logging a diagnostic message or documenting that dotted recursion requires the default JSON serializer:
var serializer = ResolveService<IJsonConverter>() as Targets.DefaultJsonSerializer; if (serializer != null) { // ... existing dotted recursion logic ... } +else +{ + // Dotted recursion requires DefaultJsonSerializer + InternalLogger.Debug("JsonLayout: DottedRecursion requires DefaultJsonSerializer, falling back to standard serialization"); +} AppendPropertyValueInternal(propName, propertyValue, format, formatProvider, captureType, sb, beginJsonMessage);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/NLog/Layouts/JSON/JsonLayout.cs(7 hunks)src/NLog/Targets/DefaultJsonSerializer.cs(2 hunks)src/NLog/Targets/JsonSerializeOptions.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-12T01:22:52.382Z
Learnt from: thatrajeevkr
PR: NLog/NLog#5999
File: src/NLog/Layouts/JSON/JsonLayout.cs:162-166
Timestamp: 2025-09-12T01:22:52.382Z
Learning: When suggesting StringBuilder optimizations for NLog layouts marked as [ThreadAgnostic], avoid shared StringBuilder fields as they create thread safety issues. Prefer simple string operations or ThreadLocal<StringBuilder> if allocation profiling shows a real performance impact.
Applied to files:
src/NLog/Layouts/JSON/JsonLayout.cs
🧬 Code graph analysis (2)
src/NLog/Targets/DefaultJsonSerializer.cs (3)
src/NLog/Targets/JsonSerializeOptions.cs (1)
JsonSerializeOptions(42-113)src/NLog/Internal/SingleItemOptimizedHashSet.cs (2)
SingleItemOptimizedHashSet(98-119)SingleItemScopedInsert(64-83)src/NLog/Layouts/JSON/JsonLayout.cs (1)
SerializeObject(75-81)
src/NLog/Layouts/JSON/JsonLayout.cs (2)
src/NLog/Targets/DefaultJsonSerializer.cs (9)
DefaultJsonSerializer(47-866)DefaultJsonSerializer(70-73)SerializeObject(80-83)SerializeObject(91-146)SerializeObject(154-157)SerializeObject(166-169)SerializeObject(190-208)SerializeObjectWithDottedRecursion(175-179)SerializeObjectWithDottedRecursion(231-301)src/NLog/Targets/JsonSerializeOptions.cs (1)
JsonSerializeOptions(42-113)
🔇 Additional comments (14)
src/NLog/Targets/JsonSerializeOptions.cs (1)
108-112: LGTM! Clean API addition for dotted recursion feature.The new
DottedRecursionproperty is well-documented, follows the existing pattern for serialization options, and maintains backward compatibility with a sensible default offalse.src/NLog/Targets/DefaultJsonSerializer.cs (6)
171-179: LGTM! Well-designed entry point for dotted recursion.The public-facing method correctly delegates to the private implementation with appropriate initialization of the
wroteAnyflag.
210-230: LGTM! Clean helper methods for JSON formatting.The
AppendDelimiterWhenNeededandWriteKeyPrefixmethods properly handle delimiter insertion and key formatting, respecting both the custom delimiter and standard JSON options.
231-238: Excellent recursion depth protection.The method correctly checks against
MaxRecursionLimitand falls back to standard serialization when the limit is reached, preventing stack overflow issues.
271-298: Consider extracting dotted property name construction.The string concatenation
string.Concat(basePropertyName, ".", property.Name)appears twice (lines 285 and 294). Whilestring.Concatis efficient, extracting this to a local variable would improve readability and ensure consistency.foreach (var property in objectPropertyList) { if (!property.HasNameAndValue) continue; - string dottedPropertyName = string.Concat(basePropertyName, ".", property.Name); + string dottedPropertyName = string.Concat(basePropertyName, ".", property.Name); if (property.TypeCode != TypeCode.Object) { WriteKeyPrefix(destination, options, dottedPropertyName, propertyDelimiter, ref wroteAny); SerializeSimpleTypeCodeValue((IConvertible?)property.Value, property.TypeCode, destination, options); } else { - if (!SerializeObjectWithDottedRecursion(property.Value, destination, options, objectsInPath, depth + 1, dottedPropertyName, propertyDelimiter, ref wroteAny)) + if (!SerializeObjectWithDottedRecursion(property.Value, destination, options, objectsInPath, depth + 1, dottedPropertyName, propertyDelimiter, ref wroteAny)) return false; } }Actually, the code already follows this pattern correctly. The variable is declared once and reused.
231-301: Excellent implementation of dotted recursion with proper safeguards.The implementation correctly:
- Protects against stack overflow with recursion depth checks
- Handles circular references using
SingleItemOptimizedHashSet- Processes all value types (null, primitives, DateTimeOffset, collections, complex objects)
- Maintains proper JSON structure with appropriate key prefixes
- Gracefully degrades to standard serialization when limits are reached
265-269: Approve — collection serialization preserves valid JSON structure.WriteKeyPrefix emits the quoted key + colon and SerializeObject writes the object/array tokens; existing unit tests exercise dotted recursion and collection cases (JsonLayoutTests.DottedRecursion_WithMixedTypes and DefaultJsonSerializerTestsBase collection tests) and assert the expected output, so no change required.
src/NLog/Layouts/JSON/JsonLayout.cs (7)
154-172: LGTM! Well-integrated DottedRecursion property.The property follows the established pattern, properly invalidates the JSON converter when changed, and includes appropriate XML documentation.
57-57: Thread-safe converter initialization with dotted recursion support.The lazy initialization pattern correctly passes the
DottedRecursionflag to the converter constructor, ensuring thread-safe creation.
441-441: Clean delimiter extraction for dotted properties.The code correctly extracts the delimiter by removing the trailing quote character, maintaining consistency with the JSON formatting options.
388-396: Clean conditional routing based on DottedRecursion flag.The code correctly routes to either flattened or standard property value rendering based on the
DottedRecursionsetting, maintaining backward compatibility.
476-493: Efficient empty property detection logic.The implementation correctly checks for empty string values by detecting consecutive quotes without an escape character, avoiding unnecessary allocations.
68-72: No change required — SanitizeDictionaryKeys affects only dictionary keysVerified: SanitizeDictionaryKeys sanitizes dictionary keys only and does not alter regular object property names, so keeping SanitizeDictionaryKeys = true (with DottedRecursion) is intentional and safe.
499-529: No change required — caller appends the closing brace.AppendFlattenedPropertyValue appends '{' + newline + indentation before calling the serializer, and the caller appends _completeJsonMessage when sb.Length > orgLength, so the closing '}' is added even if this method returns early.
|
@snakefoot I have changed it as per your comment, please take a look. For the coverage part, I will add in a testcase to solve that. |
|
As per your suggestion, created a new function in |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/NLog/Layouts/JSON/JsonLayout.cs (1)
471-476: Guard JSON escaping on Stringify to avoid corruption (repeat)
PerformJsonEscapeIfNeededassumes surrounding quotes. Guard to prevent corrupting output when quotes are omitted.int originalStart = sb.Length; ValueFormatter.FormatValue(propertyValue, format, captureType, formatProvider, sb); -PerformJsonEscapeIfNeeded(sb, originalStart); +if (sb.Length > originalStart && sb[originalStart] == '"' && sb[sb.Length - 1] == '"') +{ + PerformJsonEscapeIfNeeded(sb, originalStart); +}
🧹 Nitpick comments (1)
tests/NLog.UnitTests/Config/ServiceRepositoryTests.cs (1)
205-209: Align nullability with interface to avoid warningsMatch the
IJsonConvertersignature to prevent nullable‑reference warnings and acceptnullwhen the caller passes it.- public bool SerializeObjectDotted(object value, StringBuilder destination, NLog.Targets.JsonSerializeOptions options, string basePropertyName, string propertyDelimiter) + public bool SerializeObjectDotted(object? value, StringBuilder destination, NLog.Targets.JsonSerializeOptions options, string basePropertyName, string? propertyDelimiter)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/NLog/IJsonConverter.cs(1 hunks)src/NLog/Layouts/JSON/JsonLayout.cs(8 hunks)src/NLog/MessageTemplates/ValueFormatter.cs(1 hunks)src/NLog/Targets/DefaultJsonSerializer.cs(2 hunks)tests/NLog.UnitTests/Config/ServiceRepositoryTests.cs(1 hunks)tests/NLog.UnitTests/Layouts/JsonLayoutTests.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/NLog.UnitTests/Layouts/JsonLayoutTests.cs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-12T01:22:52.382Z
Learnt from: thatrajeevkr
PR: NLog/NLog#5999
File: src/NLog/Layouts/JSON/JsonLayout.cs:162-166
Timestamp: 2025-09-12T01:22:52.382Z
Learning: When suggesting StringBuilder optimizations for NLog layouts marked as [ThreadAgnostic], avoid shared StringBuilder fields as they create thread safety issues. Prefer simple string operations or ThreadLocal<StringBuilder> if allocation profiling shows a real performance impact.
Applied to files:
src/NLog/Layouts/JSON/JsonLayout.cs
🧬 Code graph analysis (5)
src/NLog/IJsonConverter.cs (5)
src/NLog/Layouts/JSON/JsonLayout.cs (1)
SerializeObjectDotted(88-93)src/NLog/MessageTemplates/ValueFormatter.cs (1)
SerializeObjectDotted(89-93)src/NLog/Targets/DefaultJsonSerializer.cs (1)
SerializeObjectDotted(180-184)tests/NLog.UnitTests/Config/ServiceRepositoryTests.cs (1)
SerializeObjectDotted(205-209)src/NLog/Targets/JsonSerializeOptions.cs (1)
JsonSerializeOptions(42-113)
src/NLog/MessageTemplates/ValueFormatter.cs (5)
src/NLog/IJsonConverter.cs (1)
SerializeObjectDotted(58-58)src/NLog/Layouts/JSON/JsonLayout.cs (1)
SerializeObjectDotted(88-93)src/NLog/Targets/DefaultJsonSerializer.cs (1)
SerializeObjectDotted(180-184)tests/NLog.UnitTests/Config/ServiceRepositoryTests.cs (1)
SerializeObjectDotted(205-209)src/NLog/Targets/JsonSerializeOptions.cs (1)
JsonSerializeOptions(42-113)
tests/NLog.UnitTests/Config/ServiceRepositoryTests.cs (5)
src/NLog/IJsonConverter.cs (2)
SerializeObjectDotted(58-58)SerializeObject(47-47)src/NLog/Layouts/JSON/JsonLayout.cs (2)
SerializeObjectDotted(88-93)SerializeObject(75-81)src/NLog/MessageTemplates/ValueFormatter.cs (2)
SerializeObjectDotted(89-93)SerializeObject(81-87)src/NLog/Targets/DefaultJsonSerializer.cs (6)
SerializeObjectDotted(180-184)SerializeObject(80-83)SerializeObject(91-146)SerializeObject(154-157)SerializeObject(166-169)SerializeObject(195-213)src/NLog/Targets/JsonSerializeOptions.cs (1)
JsonSerializeOptions(42-113)
src/NLog/Targets/DefaultJsonSerializer.cs (4)
src/NLog/IJsonConverter.cs (2)
SerializeObjectDotted(58-58)SerializeObject(47-47)src/NLog/Layouts/JSON/JsonLayout.cs (2)
SerializeObjectDotted(88-93)SerializeObject(75-81)src/NLog/Targets/JsonSerializeOptions.cs (1)
JsonSerializeOptions(42-113)src/NLog/Internal/SingleItemOptimizedHashSet.cs (2)
SingleItemOptimizedHashSet(98-119)SingleItemScopedInsert(64-83)
src/NLog/Layouts/JSON/JsonLayout.cs (4)
src/NLog/MessageTemplates/ValueFormatter.cs (5)
IJsonConverter(66-72)ValueFormatter(47-401)ValueFormatter(96-100)SerializeObjectDotted(89-93)SerializeObject(81-87)src/NLog/Targets/DefaultJsonSerializer.cs (8)
DefaultJsonSerializer(47-871)DefaultJsonSerializer(70-73)SerializeObjectDotted(180-184)SerializeObject(80-83)SerializeObject(91-146)SerializeObject(154-157)SerializeObject(166-169)SerializeObject(195-213)src/NLog/Targets/JsonSerializeOptions.cs (1)
JsonSerializeOptions(42-113)src/NLog/IJsonConverter.cs (2)
SerializeObjectDotted(58-58)SerializeObject(47-47)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: task-list-completed
🔇 Additional comments (7)
src/NLog/MessageTemplates/ValueFormatter.cs (1)
89-93: Straightforward delegate; LGTMDelegating to serializer is correct; ValueFormatter itself shouldn’t handle dotted recursion.
src/NLog/Targets/DefaultJsonSerializer.cs (1)
171-184: Good additive API surfaceEntry point for dotted serialization is clear and contained.
src/NLog/Layouts/JSON/JsonLayout.cs (5)
57-57: Converter cache correctly invalidated on option flipsPassing
DottedRecursioninto the converter factory and resetting cache when toggled is correct.
68-73: Serializer option wiring looks goodPropagating
MaxRecursionLimit,SuppressSpaces,SanitizeDictionaryKeys, andDottedRecursioninto options is consistent.
161-179: Option surface: DottedRecursionProperty and invalidation look correct.
395-403: Branching to flattened path is fineTop‑level switch based on
DottedRecursionis clear.
448-448: Delimiter derivation is neatReusing
_beginJsonPropertyNameto derive dotted delimiter keeps formatting consistent.
thatrajeevkr
force-pushed
the
feature/dottedrecursion
branch
from
55c3255 to
6d4c2c0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/NLog/Layouts/JSON/JsonLayout.cs(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-12T01:22:52.437Z
Learnt from: thatrajeevkr
PR: NLog/NLog#5999
File: src/NLog/Layouts/JSON/JsonLayout.cs:162-166
Timestamp: 2025-09-12T01:22:52.437Z
Learning: When suggesting StringBuilder optimizations for NLog layouts marked as [ThreadAgnostic], avoid shared StringBuilder fields as they create thread safety issues. Prefer simple string operations or ThreadLocal<StringBuilder> if allocation profiling shows a real performance impact.
Applied to files:
src/NLog/Layouts/JSON/JsonLayout.cs
🧬 Code graph analysis (1)
src/NLog/Layouts/JSON/JsonLayout.cs (3)
src/NLog/Internal/ObjectReflectionCache.cs (1)
ObjectReflectionCache(56-59)src/NLog/Internal/StringHelpers.cs (2)
StringHelpers(45-170)IsNullOrEmptyString(166-169)src/NLog/MessageTemplates/ValueFormatter.cs (3)
ValueFormatter(47-395)ValueFormatter(90-94)FormatValue(111-134)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: task-list-completed
🔇 Additional comments (7)
src/NLog/Layouts/JSON/JsonLayout.cs (7)
155-160: LGTM!The
DottedRecursionproperty is well-documented and defaults tofalse, preserving backward compatibility.
377-385: LGTM!The routing logic correctly switches between flattened and standard property rendering based on the
DottedRecursionflag. Note that this only affects event properties, not GDC or ScopeContext properties.
430-464: LGTM!
AppendPropertyValueInternalcorrectly consolidates the Serialize/Stringify handling that was previously duplicated. The guard on line 455 ensures JSON escape is only applied when the formatted value is actually quoted, addressing the past review concern.
484-487: LGTM!Simple delegation to
AppendPropertyValueInternalmaintains backward compatibility.
489-499: LGTM!Correctly limits flattening to
CaptureType.Normaland delegates Serialize/Stringify to the standard path. Starting recursion at depth 0 is appropriate.
501-534: LGTM! Recursion protection and threading addressed.
FlattenObjectPropertiescorrectly:
- Tracks recursion depth and falls back to standard serialization at
MaxRecursionLimit(line 503-506)- Honors
ExcludeEmptyProperties(line 509-510)- Uses thread-safe
string.Concatfor dotted names (line 530)- Passes
falseforbeginJsonMessagein nested calls (line 532)All issues flagged in past reviews have been resolved.
536-546: LGTM!
IsSimpleValuecorrectly identifies primitives and simple types usingIConvertibleandTypeCode, ensuring only complex objects are flattened.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/NLog/Layouts/JSON/JsonLayout.cs (1)
450-465: LGTM! Proper handling of different capture types.The method correctly delegates
Stringifyto the standard path (since stringification doesn't benefit from property expansion) and handlesSerialize/Normalthrough flattening. The negativestartDepthforSerializemode effectively loosens the depth bound to allow deeper object traversal.Consider adding a named constant for the
-10offset to make the intent more explicit:private const int SerializeModeDepthOffset = 10; // Then use: Math.Min(0, MaxRecursionLimit - SerializeModeDepthOffset)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/NLog/Layouts/JSON/JsonLayout.cs(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-12T01:22:52.437Z
Learnt from: thatrajeevkr
PR: NLog/NLog#5999
File: src/NLog/Layouts/JSON/JsonLayout.cs:162-166
Timestamp: 2025-09-12T01:22:52.437Z
Learning: When suggesting StringBuilder optimizations for NLog layouts marked as [ThreadAgnostic], avoid shared StringBuilder fields as they create thread safety issues. Prefer simple string operations or ThreadLocal<StringBuilder> if allocation profiling shows a real performance impact.
Applied to files:
src/NLog/Layouts/JSON/JsonLayout.cs
🧬 Code graph analysis (1)
src/NLog/Layouts/JSON/JsonLayout.cs (1)
src/NLog/Internal/ObjectReflectionCache.cs (1)
ObjectReflectionCache(56-59)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: task-list-completed
🔇 Additional comments (7)
src/NLog/Layouts/JSON/JsonLayout.cs (7)
61-62: LGTM! Consistent lazy initialization pattern.The
ObjectReflectionCachefield follows the same lazy initialization pattern as_jsonConverterand_valueFormatter, with proper cleanup inCloseLayout(). This is thread-safe for the[ThreadAgnostic]layout.
155-160: LGTM! Well-documented property with safe defaults.The
DottedRecursionproperty is properly documented and defaults tofalse, maintaining backward compatibility with existing behavior.
289-289: LGTM! Proper cleanup added.The
_objectReflectionCachecleanup is correctly added alongside the existing cleanup for_jsonConverterand_valueFormatter, preventing reference retention after layout disposal.
374-381: LGTM! Clean conditional routing.The routing logic properly directs event properties to the flattened or standard rendering path based on the
DottedRecursionsetting, maintaining backward compatibility when the feature is disabled.
446-448: LGTM! Duplication eliminated through refactoring.The refactoring to delegate to
AppendPropertyValueInternalsuccessfully eliminates the code duplication that was flagged in previous reviews, while maintaining the method signature for compatibility.
467-500: LGTM! Shared logic properly extracted.The extracted
AppendPropertyValueInternalsuccessfully consolidates the common handling forSerializeandStringifycapture types. The explicit quote wrapping forStringify(lines 490, 494) properly guards the escape operation, addressing previous review concerns.
502-540: LGTM! Solid recursive flattening implementation.The
FlattenObjectPropertiesmethod properly implements dotted-key flattening with several good safeguards:
- Recursion protection: Depth tracking against
MaxRecursionLimitprevents stack overflow (line 504)- Thread safety:
string.Concat(line 532) avoids the sharedStringBuilderissues from earlier reviews- Empty value handling:
ExcludeEmptyPropertiesis correctly honored (line 510)- Delimiter management: The
isFirstChildflag with length check (lines 533-538) properly handles children that emit nothing, preventing malformed JSONThe fallback to standard serialization at max depth (line 506) and for simple values (lines 513-517, 520-524) ensures the feature degrades gracefully.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/NLog/Layouts/JSON/JsonLayout.cs (2)
155-161: Public optionDottedRecursionadded — consider consistent scopeLooks good. Consider applying the same flattening semantics to GDC and Scope properties for consistency with event properties.
Would you like me to propose the minimal changes to also flatten GDC/Scope when DottedRecursion = true?
374-381: Flattening applies only to event propertiesCurrently DottedRecursion is not applied to GDC/Scope. If the option is meant to be layout-wide, extend the same logic to those sources for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/NLog/Layouts/JSON/JsonLayout.cs(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-12T01:22:52.437Z
Learnt from: thatrajeevkr
PR: NLog/NLog#5999
File: src/NLog/Layouts/JSON/JsonLayout.cs:162-166
Timestamp: 2025-09-12T01:22:52.437Z
Learning: When suggesting StringBuilder optimizations for NLog layouts marked as [ThreadAgnostic], avoid shared StringBuilder fields as they create thread safety issues. Prefer simple string operations or ThreadLocal<StringBuilder> if allocation profiling shows a real performance impact.
Applied to files:
src/NLog/Layouts/JSON/JsonLayout.cs
🧬 Code graph analysis (1)
src/NLog/Layouts/JSON/JsonLayout.cs (1)
src/NLog/Internal/ObjectReflectionCache.cs (1)
ObjectReflectionCache(56-59)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: task-list-completed
🔇 Additional comments (5)
src/NLog/Layouts/JSON/JsonLayout.cs (5)
61-63: Lazy ObjectReflectionCache + disposal cleanup looks goodInitialization via ResolveService and nulling in CloseLayout are correct.
289-289: Dispose pattern updatedClearing
_objectReflectionCacheinCloseLayout()is correct and prevents lingering references.
445-449: Centralized handling viaAppendPropertyValueInternalGood consolidation; reduces duplication and keeps capture-type logic in one place.
526-539: Correctly preservesbeginJsonMessagewhen child emits nothingThe length check before flipping
isFirstChildprevents malformed JSON when first child is skipped. Good.
532-532: Good choice:string.Concatfor dotted namesAvoids shared StringBuilder and keeps layout thread-safe. Based on learnings.
Also moving the logic from DefaultJsonSerializer would require the need to handle cyclic references in a more general way in ObjectReflectionCache than right now which would increase extra complexity and require extra complex API to keep track of the current path. That could be a whole PR on its own since it would require changing and moving a lot of functionality across, do you think its required now along with this change? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/NLog/Layouts/JSON/JsonLayout.cs (4)
37-37: Unused import:System.Collectionsis not referenced in the code.The
System.Collectionsnamespace is imported but no types from it are used. TheDictionary<int, int>on line 521 is fromSystem.Collections.Generic, which is already imported on line 38.Apply this diff to remove the unused import:
- using System.Collections; using System.Collections.Generic;
452-467: Consider normalizingMaxRecursionLimitto prevent edge-case issues with negative values.While
MaxRecursionLimitis typically positive (default is 1), the calculationMaxRecursionLimit - 10on line 462 can overflow ifMaxRecursionLimitis nearint.MinValue, and the depth check on line 506 could behave unexpectedly with negative limits. Normalizing the limit at the start of these methods would make the logic more robust.Apply this diff to normalize the limit:
private void AppendFlattenedPropertyValue(string propName, object? propertyValue, string? format, IFormatProvider? formatProvider, MessageTemplates.CaptureType captureType, StringBuilder sb, bool beginJsonMessage) { if (captureType == MessageTemplates.CaptureType.Stringify) { AppendPropertyValueInternal(propName, propertyValue, format, formatProvider, captureType, sb, beginJsonMessage); } else { + int effectiveMaxDepth = Math.Max(0, MaxRecursionLimit); // Allow flattening also for Serialize, by starting at a negative depth to effectively loosen depth bound int startDepth = captureType == MessageTemplates.CaptureType.Serialize - ? Math.Min(0, MaxRecursionLimit - 10) + ? Math.Min(0, effectiveMaxDepth - 10) : 0; FlattenObjectProperties(propName, propertyValue, sb, beginJsonMessage, startDepth); } }And in
FlattenObjectProperties:private void FlattenObjectProperties(string basePropertyName, object? propertyValue, StringBuilder sb, bool beginJsonMessage, int depth = 0) { - if (depth >= MaxRecursionLimit) + int effectiveMaxDepth = Math.Max(0, MaxRecursionLimit); + if (depth >= effectiveMaxDepth) {
521-530: Collection handling is partial but represents incremental progress.The special case for
Dictionary<int, int>outputs individual items rather than just theCountproperty, which is an improvement over the default object reflection behavior. However, this leaves other non-string-keyed collections (e.g.,Dictionary<int, string>,List<int>) still outputting only metadata properties.This is acceptable as an incremental improvement, but consider expanding collection support in future iterations to handle
IEnumerabletypes more comprehensively.
526-526: Optional: Avoid string interpolation in loop for minor allocation reduction.Line 526 uses string interpolation inside a
foreachloop, which allocates a new string on each iteration. WhileDictionary<int, int>iteration is unlikely to be a hot path in typical logging scenarios, you could reduce allocations by usingstring.Concator aStringBuilderif profiling shows this is a concern.Example with
string.Concat:- string indexedPropertyName = $"{basePropertyName}[{keyString}]"; + string indexedPropertyName = string.Concat(basePropertyName, "[", keyString, "]");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/NLog/Layouts/JSON/JsonLayout.cs(7 hunks)tests/NLog.UnitTests/Layouts/JsonLayoutTests.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/NLog.UnitTests/Layouts/JsonLayoutTests.cs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-12T01:22:52.437Z
Learnt from: thatrajeevkr
Repo: NLog/NLog PR: 5999
File: src/NLog/Layouts/JSON/JsonLayout.cs:162-166
Timestamp: 2025-09-12T01:22:52.437Z
Learning: When suggesting StringBuilder optimizations for NLog layouts marked as [ThreadAgnostic], avoid shared StringBuilder fields as they create thread safety issues. Prefer simple string operations or ThreadLocal<StringBuilder> if allocation profiling shows a real performance impact.
Applied to files:
src/NLog/Layouts/JSON/JsonLayout.cs
🧬 Code graph analysis (1)
src/NLog/Layouts/JSON/JsonLayout.cs (1)
src/NLog/Internal/ObjectReflectionCache.cs (1)
ObjectReflectionCache(56-59)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: task-list-completed
🔇 Additional comments (6)
src/NLog/Layouts/JSON/JsonLayout.cs (6)
63-64: LGTM!The lazy initialization of
ObjectReflectionCachefollows the existing pattern for_jsonConverterand_valueFormatter, and cleanup inCloseLayoutis correctly added.Also applies to: 291-291
157-162: LGTM!The
DottedRecursionproperty is well-documented and follows the existing property pattern. The defaultfalsepreserves backward compatibility.
376-383: LGTM!The conditional dispatch cleanly separates the flattening logic from the existing property rendering path, preserving backward compatibility when
DottedRecursionisfalse.
447-450: LGTM!The refactoring to delegate to
AppendPropertyValueInternaleliminates code duplication and improves maintainability.
469-502: LGTM!The extracted
AppendPropertyValueInternalmethod correctly handles the three capture types (Serialize,Stringify,Normal) with appropriate serialization, formatting, and escaping logic.
544-544: LGTM!The use of
string.Concaton line 544 avoids unnecessary allocations, and the length-check pattern (lines 545-550) correctly preservesbeginJsonMessagesemantics when a child emits no output (e.g., filtered byExcludeEmptyProperties).Based on learnings.
Also applies to: 545-550
| return; | ||
| } | ||
|
|
||
| if (propertyValue is Dictionary<int, int> dict) |
There was a problem hiding this comment.
Not sure I like this approach. Then one have to recognize all Dictionary-types / List-types.
Maybe the value returned here:
var objectPropertyList = ObjectReflectionCache.LookupObjectProperties(propertyValue);
Could get a property that could signal that it was an IEnumerable, but not an expando-object (Ex. Dictionary<int, int> or List<int> etc.). And for that case, then "just" serialize as Json-collection without using dotted-recursion.
There was a problem hiding this comment.
@snakefoot Since there is currently no logic in ObjectReflectionCache that does this, we would need to create a new property (can be called ShouldSerializeDirectly) that does this.
- The new property would explicitly limit to List and Dictionary<int,int> since these are the only ones one which need to be serialised instead of flattened.
- All the other IEnumerables will be flattened as expected (can check current testcases for the output of these)
How does that sound?
There was a problem hiding this comment.
ObjectReflectionCache already recognize something that it calls expando-objects (Ex. Dictionary<string, T>). I was thinking it should be like this:
- If expando-object or object is not IEnumerable - Dotted recursion
- Else simple-objects (string / bool / etc.) or IEnumerable - Standard serialization.
There was a problem hiding this comment.
@snakefoot yup, got the idea! Its all fixed now and serialises simple objects and Dictionary<int,int> instead of dotted recursion.
Changed how dotted recursion handles expando and enumberable objects
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/NLog/Layouts/JSON/JsonLayout.cs (2)
452-467: Normalize MaxRecursionLimit to prevent underflow and ensure consistent behavior.The past review comment about normalizing the recursion limit remains unaddressed. Two issues:
- Integer underflow risk: If
MaxRecursionLimitis very negative (e.g., close toInt32.MinValue), the expressionMaxRecursionLimit - 10can underflow.- Inconsistent normalization: Elsewhere in the codebase (line 76),
MaxRecursionLimitis clamped withMath.Max(0, maxRecursionLimit), but here it's used directly.Apply the suggested normalization:
private void AppendFlattenedPropertyValue(string propName, object? propertyValue, string? format, IFormatProvider? formatProvider, MessageTemplates.CaptureType captureType, StringBuilder sb, bool beginJsonMessage) { if (captureType == MessageTemplates.CaptureType.Stringify) { AppendPropertyValueInternal(propName, propertyValue, format, formatProvider, captureType, sb, beginJsonMessage); } else { + int effectiveMaxDepth = Math.Max(0, MaxRecursionLimit); // Allow flattening also for Serialize, by starting at a negative depth to effectively loosen depth bound int startDepth = captureType == MessageTemplates.CaptureType.Serialize - ? Math.Min(0, MaxRecursionLimit - 10) + ? Math.Min(0, effectiveMaxDepth - 10) : 0; FlattenObjectProperties(propName, propertyValue, sb, beginJsonMessage, startDepth); } }Also apply the same normalization in
FlattenObjectProperties(see next comment).Based on unresolved past review feedback.
504-510: Apply normalization to MaxRecursionLimit in depth check.Continuing from the previous comment, the recursion depth check also needs normalization:
private void FlattenObjectProperties(string basePropertyName, object? propertyValue, StringBuilder sb, bool beginJsonMessage, int depth = 0) { - if (depth >= MaxRecursionLimit) + int effectiveMaxDepth = Math.Max(0, MaxRecursionLimit); + if (depth >= effectiveMaxDepth) { AppendJsonPropertyValue(basePropertyName, propertyValue, sb, beginJsonMessage); return; }This ensures consistent behavior when
MaxRecursionLimitis negative and prevents the method from bailing out immediately.Based on unresolved past review feedback.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/NLog/Layouts/JSON/JsonLayout.cs(7 hunks)tests/NLog.UnitTests/Layouts/JsonLayoutTests.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/NLog.UnitTests/Layouts/JsonLayoutTests.cs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-12T01:22:52.437Z
Learnt from: thatrajeevkr
Repo: NLog/NLog PR: 5999
File: src/NLog/Layouts/JSON/JsonLayout.cs:162-166
Timestamp: 2025-09-12T01:22:52.437Z
Learning: When suggesting StringBuilder optimizations for NLog layouts marked as [ThreadAgnostic], avoid shared StringBuilder fields as they create thread safety issues. Prefer simple string operations or ThreadLocal<StringBuilder> if allocation profiling shows a real performance impact.
Applied to files:
src/NLog/Layouts/JSON/JsonLayout.cs
🧬 Code graph analysis (1)
src/NLog/Layouts/JSON/JsonLayout.cs (1)
src/NLog/Internal/ObjectReflectionCache.cs (2)
ObjectReflectionCache(56-59)TryLookupExpandoObject(133-174)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: task-list-completed
🔇 Additional comments (7)
src/NLog/Layouts/JSON/JsonLayout.cs (7)
63-64: LGTM! Consistent lazy initialization pattern.The
ObjectReflectionCacheinitialization follows the same lazy pattern as the existing_jsonConverterand_valueFormatter, and is properly cleaned up inCloseLayout.
157-162: LGTM! Well-documented property.The
DottedRecursionproperty is properly documented with XML comments and has a sensible default value offalseto maintain backward compatibility.
291-291: LGTM! Proper resource cleanup.The cleanup of
_objectReflectionCacheinCloseLayoutcorrectly prevents reference retention after layout disposal, matching the pattern for other lazy-initialized fields.
376-383: LGTM! Clear conditional routing.The branching logic correctly routes to
AppendFlattenedPropertyValuewhenDottedRecursionis enabled and preserves existing behavior otherwise.
447-450: LGTM! Reduces code duplication.The refactored
AppendJsonPropertyValuenow delegates to the sharedAppendPropertyValueInternalmethod, eliminating duplication as addressed in previous reviews.
469-502: LGTM! Proper handling of capture types.
AppendPropertyValueInternalcorrectly handles all three capture types:
- Serialize with low recursion limit bypasses it safely
- Stringify properly escapes the formatted value
- Normal delegates to standard serialization
521-547: LGTM! Robust flattening logic with proper safeguards.The flattening logic correctly handles:
- IEnumerable types that aren't expando objects (serializes directly instead of flattening)
- Simple values and null checks
- Thread-safe dotted name construction using
string.Concat(line 539)- Proper
isFirstChildtracking with length checks (lines 540-545) to avoid corrupted JSON when children emit nothingThe logic aligns with the maintainer's guidance to serialize non-expando collections directly while flattening expando objects and POCOs.
removed unnecessary header
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/NLog/Layouts/JSON/JsonLayout.cs (1)
451-466: Consider clarifying the Serialize depth adjustment logic.The depth calculation at lines 460-461 intentionally allows deeper recursion for
Serializecapture types, but the logicMath.Min(0, MaxRecursionLimit - 10)is subtle. WhenMaxRecursionLimitis small (e.g., 1), this allows up to 10 flattening levels by starting at a negative depth. A brief inline comment explaining the rationale would help future maintainers.Example:
- // Allow flattening also for Serialize, by starting at a negative depth to effectively loosen depth bound + // Allow flattening also for Serialize, by starting at a negative depth to effectively loosen depth bound. + // When MaxRecursionLimit < 10, we start at a negative depth to guarantee at least 10 levels of flattening, + // since message-template Serialize indicates the object is safe to traverse deeply. int startDepth = captureType == MessageTemplates.CaptureType.Serialize ? Math.Min(0, MaxRecursionLimit - 10) : 0;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/NLog/Layouts/JSON/JsonLayout.cs(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-12T01:22:52.437Z
Learnt from: thatrajeevkr
Repo: NLog/NLog PR: 5999
File: src/NLog/Layouts/JSON/JsonLayout.cs:162-166
Timestamp: 2025-09-12T01:22:52.437Z
Learning: When suggesting StringBuilder optimizations for NLog layouts marked as [ThreadAgnostic], avoid shared StringBuilder fields as they create thread safety issues. Prefer simple string operations or ThreadLocal<StringBuilder> if allocation profiling shows a real performance impact.
Applied to files:
src/NLog/Layouts/JSON/JsonLayout.cs
🧬 Code graph analysis (1)
src/NLog/Layouts/JSON/JsonLayout.cs (1)
src/NLog/Internal/ObjectReflectionCache.cs (2)
ObjectReflectionCache(56-59)TryLookupExpandoObject(133-174)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: task-list-completed
🔇 Additional comments (4)
src/NLog/Layouts/JSON/JsonLayout.cs (4)
62-63: LGTM: ObjectReflectionCache follows established patterns.The lazy initialization pattern matches existing fields (
_jsonConverter,_valueFormatter) and proper cleanup is present inCloseLayout(). The dependency injection viaResolveServiceis consistent with the class design.
156-161: LGTM: DottedRecursion property is well-documented and backward-compatible.The property has clear XML documentation and defaults to
false, ensuring existing behavior is preserved unless explicitly enabled.
375-382: LGTM: Conditional routing correctly implements feature flag.The branching logic cleanly separates the flattened and standard rendering paths, with the default path preserving existing behavior when
DottedRecursionis false.
503-546: LGTM: FlattenObjectProperties implements robust recursive flattening.The method correctly handles:
- Depth limiting (line 505): Falls back to standard serialization when
MaxRecursionLimitis reached- Empty property exclusion (line 511): Honors
ExcludeEmptyProperties- Simple values (lines 514-518): Directly serializes strings, nulls, and primitive types
- Collection vs. expando distinction (line 520): IEnumerables (lists, non-string-keyed dictionaries) serialize normally while expando objects (Dictionary<string,T>) flatten into dotted keys
- Allocation efficiency (line 538): Uses
string.Concatto avoid string interpolation allocations, per learnings- Delimiter tracking (lines 539-544): Only sets
isFirstChild = falsewhen output was actually produced, preventing malformed JSONThe recursive flattening logic is sound and addresses all concerns raised in prior review rounds.
Based on learnings, using
string.Concatinstead of string interpolation or shared StringBuilder is the correct approach for this[ThreadAgnostic]layout.
| val4 = 3.14, | ||
| val5 = new DateTime(2023, 1, 1, 0, 0, 0, DateTimeKind.Utc), | ||
| val6 = Guid.NewGuid() | ||
| } |
There was a problem hiding this comment.
Can we add array of int as property? list = new List<int> { 1, 2, 3 }
Believe with your latest change then it will output the collection and not the list-Count.
There was a problem hiding this comment.
@snakefoot yes it does, I will add that change to the testacase.
|
Think with the latest change to handle IEnumerable, then the pull-request looks ready to me. Do you have any more planned changes? |
added testcase for int list
|
@snakefoot Yes thats all for this, after this I will start working on the other issue (#5990) |
|
|
Really appreciates the work you have done, and also overcoming my bad advice about trying to move the logic into the NLog JsonConverter. Like that the code is short and simple by reusing existing building-blocks, and also thank you for the unit-tests. Seems NLog v6.0.6 is stable, so I think next version will be NLog v6.1.0 where this feature will be included. Sounds great with #5990. Maybe start with creating a pull-request against the NLog-repo, and when ready then it can becomes its git-repo NLog.Targets.HttpClient. |
|
@snakefoot Sure! great to help. |
snakefoot
changed the title
|
Hooray your first pull request is merged! Thanks for the contribution! Looking for more? 👼 Please check the up-for-grabs issues - thanks! |
|
Created #6084 to add protection against cyclic object graphs for DottedRecursion. |
|
Thank you again for the nice contribution. NLog v6.1 has now been released: |
|
@snakefoot Thanks :) and I will be pushing changes for the other issue as well. |
snakefoot
added
the
documentation done
2 participants
Add DottedRecursion to JsonLayout for flattening nested properties
Example Output :
With DottedRecursion = true,