JsonLayout - Protect against cyclic object graphs when DottedRecursion#6084
Merged
JsonLayout - Protect against cyclic object graphs when DottedRecursion#6084
Conversation
WalkthroughRemoved the comparer parameter from SingleItemScopedInsert to use ReferenceEqualityComparer.Default; added dotted-recursion flattening in JsonLayout with new traversal and cycle-tracking helpers; updated call-sites, ObjectReflectionCache decision logic, and tests (renamed Recursive → RecursiveCollection, added RecursiveProperty, new flattening test). Changes
Sequence DiagramsequenceDiagram
participant JsonLayout as JsonLayout (Renderer)
participant Eligibility as PropertyValueSupportsDottedRecursion
participant Flattener as AppendFlattenedPropertyValue
participant Tracker as SingleItemOptimizedHashSet (Tracker)
participant Walker as FlattenObjectProperties
JsonLayout->>Eligibility: Check DottedRecursion & value eligibility
alt flattenable
JsonLayout->>Flattener: Request flattened append
Flattener->>Tracker: Start scoped insert (prevent cycles)
Flattener->>Walker: Traverse object properties (with startDepth)
Walker->>Tracker: Add nested objects to tracking set
Walker->>Walker: Recurse and collect flattened property pairs
Walker->>Tracker: Remove objects from tracking (end scope)
Walker-->>Flattener: Return flattened properties
Flattener-->>JsonLayout: Emit flattened JSON properties
else not flattenable
JsonLayout->>JsonLayout: Use AppendJsonPropertyValue / AppendPropertyValueInternal
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
![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: 1
🤖 Fix all issues with AI agents
In `@src/NLog/Layouts/JSON/JsonLayout.cs`:
- Around line 541-556: The loop in FlattenObjectProperties uses
SingleItemOptimizedHashSet<object>.SingleItemScopedInsert with
forceHashSet=false which can leave the first inserted item in objectsInPath
across sibling iterations; change the scoped insert to force a HashSet so the
disposed scope always removes the item and prevents leaking first child state —
update the using(...) around SingleItemScopedInsert(objectPropertyValue, ref
objectsInPath, false) to pass true for the forceHashSet parameter (or otherwise
ensure a HashSet-backed scoped insert) so objectsInPath is correctly restored
after each FlattenObjectProperties call.
snakefoot
force-pushed
the
jsonlayout_cyclic
branch
4 times, most recently
from
1c735f7 to
609825c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/NLog/Layouts/JSON/JsonLayout.cs`:
- Around line 545-548: The flattening loop in JsonLayout.cs currently skips
properties when objectPropertyValue is null, causing null-valued properties to
be lost even if ExcludeEmptyProperties is false; update the logic around
objectPropertyValue and objectProperty.Name so only empty names are skipped and
null values are passed into AppendJsonPropertyValue (so AppendJsonPropertyValue
and the ExcludeEmptyProperties handling decide whether to emit or skip the
property), preserving nulls in flattened output; refer to the variables
objectPropertyValue, objectProperty.Name and the method AppendJsonPropertyValue
when making the change.
609825c to
c2f41e9
Compare
snakefoot
force-pushed
the
jsonlayout_cyclic
branch
from
c2f41e9 to
8c60aa0
Compare
|
snakefoot
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Followup to #5999 to complete #5991