ObjectReflectionCache - Improve handling of JObject serialization #6030
Conversation
WalkthroughReworked ObjectReflectionCache dictionary enumeration to delegate enumeration via new DictionaryEnumerator constructors and reflection-backed BuildEnumerator/BuildDictionaryEnumerator; added PropertyValue.ToString(); updated ExpandoTestDictionary to be [Serializable], implement IEnumerable, and yield DictionaryEntry in enumerators. Changes
Sequence DiagramsequenceDiagram
participant Caller
participant ObjRef as ObjectReflectionCache
participant DictEnum as DictionaryEnumerator
participant Reflect as Reflection APIs
Caller->>ObjRef: Request dictionary enumeration for value
ObjRef->>ObjRef: Determine type / TryGetDictionaryEnumerator
alt Direct IDictionary<string, T> or IDictionary
ObjRef->>DictEnum: Use BuildDictionaryEnumerator(object) or new DictionaryEnumerator(delegate)
ObjRef->>Reflect: (if needed) BuildEnumerator(type, itemType) -> delegate
Caller->>DictEnum: Iterate -> GetEnumerator(value)
DictEnum-->>Caller: Yield KeyValuePair<string, object?> items
else Enumerable of key/value via reflection
ObjRef->>Reflect: BuildEnumerator(type, itemType)
Reflect-->>DictEnum: delegate (EnumerateItems)
Caller->>DictEnum: Iterate -> delegate yields entries
else Unsupported
ObjRef-->>Caller: EmptyDictionaryEnumerator.Default
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
f76fb76 to
95c0bb2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/NLog.UnitTests/Internal/ExpandoTestDictionary.cs (2)
110-112: Avoid iterator state machine for non-generic IEnumerable.Return the underlying IDictionary enumerator directly to reduce allocations and overhead.
Apply:
- IEnumerator IEnumerable.GetEnumerator() - { - foreach (DictionaryEntry var in (IDictionary)_properties) - yield return var; - } + IEnumerator IEnumerable.GetEnumerator() + { + return ((IDictionary)_properties).GetEnumerator(); + }
114-126: ToString OK; consider simpler join (optional).Current implementation is fine. Optionally, use string.Join over a projection for brevity if targets allow it.
src/NLog/Internal/ObjectReflectionCache.cs (2)
660-667: Constructor visibility nit.DictionaryEnumerator is private; the public ctor is unnecessary. Consider making it private or internal for clarity.
- public DictionaryEnumerator(Type? dictionaryType = null, Type? dictionaryItemType = null) + private DictionaryEnumerator(Type? dictionaryType = null, Type? dictionaryItemType = null)
688-707: Reduce per-item reflection in EnumerateItems (recommended).Currently uses PropertyInfo.GetValue on each item. Reuse compiled getters to cut reflection overhead in hot paths.
Apply:
- private void BuildEnumerator(Type dictionaryType, Type dictionaryItemType) + private void BuildEnumerator(Type dictionaryType, Type dictionaryItemType) { - var enumeratorMethod = dictionaryType.GetMethod(nameof(IEnumerable.GetEnumerator)); - var getKeyProperty = dictionaryItemType.GetProperty(nameof(KeyValuePair<string, object>.Key)); - var getValueProperty = dictionaryItemType.GetProperty(nameof(KeyValuePair<string, object>.Value)); - if (enumeratorMethod != null && getKeyProperty != null && getValueProperty != null) + var enumeratorMethod = dictionaryType.GetMethod(nameof(IEnumerable.GetEnumerator)); + var getKey = dictionaryItemType.GetProperty(nameof(KeyValuePair<string, object>.Key))?.GetGetMethod(); + var getValue = dictionaryItemType.GetProperty(nameof(KeyValuePair<string, object>.Value))?.GetGetMethod(); + if (enumeratorMethod != null && getKey != null && getValue != null) { - _enumerateCollection = (collection) => + var keyGetter = ReflectionHelpers.CreateLateBoundMethod(getKey); + var valueGetter = ReflectionHelpers.CreateLateBoundMethod(getValue); + _enumerateCollection = (collection) => { var enumerator = (IEnumerator?)enumeratorMethod.Invoke(collection, null); if (enumerator is null || !enumerator.MoveNext()) { (enumerator as IDisposable)?.Dispose(); return EmptyDictionaryEnumerator.Default; } - return EnumerateItems(enumerator, getKeyProperty, getValueProperty); + return EnumerateItems(enumerator, keyGetter, valueGetter); }; } } @@ - private static IEnumerator<KeyValuePair<string, object?>> EnumerateItems(IEnumerator enumerator, PropertyInfo getKeyProperty, PropertyInfo getValueProperty) + private static IEnumerator<KeyValuePair<string, object?>> EnumerateItems(IEnumerator enumerator, ReflectionHelpers.LateBoundMethod keyGetter, ReflectionHelpers.LateBoundMethod valueGetter) { try { do { - var item = enumerator.Current; - var keyString = getKeyProperty.GetValue(item, null); - var valueObject = getValueProperty.GetValue(item, null); + var item = enumerator.Current; + var keyString = keyGetter(item, ArrayHelper.Empty<object>()); + var valueObject = valueGetter(item, ArrayHelper.Empty<object>()); yield return new KeyValuePair<string, object?>(keyString?.ToString() ?? string.Empty, valueObject); } while (enumerator.MoveNext()); } finally { (enumerator as IDisposable)?.Dispose(); } }Also applies to: 709-725
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/NLog/Internal/ObjectReflectionCache.cs(4 hunks)tests/NLog.UnitTests/Internal/ExpandoTestDictionary.cs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/NLog/Internal/ObjectReflectionCache.cs (2)
tests/NLog.UnitTests/Internal/ExpandoTestDictionary.cs (4)
ToString(114-126)IEnumerator(88-91)IEnumerator(108-112)IEnumerator(128-132)src/NLog/Internal/DictionaryEntryEnumerable.cs (3)
MoveNext(94-97)MoveNext(112-112)EmptyDictionaryEnumerator(105-117)
⏰ 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). (3)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (5)
tests/NLog.UnitTests/Internal/ExpandoTestDictionary.cs (2)
43-45: Good extension for test coverage (Serializable + IEnumerable).This broadens shape coverage to non-generic enumeration without changing semantics. Looks good.
128-131: Explicit IEnumerable enumerator looks correct.Yields DictionaryEntry items backed by the underlying dictionary. Good for scenarios expecting DictionaryEntry.
src/NLog/Internal/ObjectReflectionCache.cs (3)
329-329: PropertyValue.ToString addition is helpful for diagnostics.Formatting "Name=Value" is concise and null-safe via interpolation. LGTM.
634-643: AOT-safe discovery of IEnumerable<KeyValuePair<,>> is a solid workaround.Iterating base interfaces of IDictionary<,>/IReadOnlyDictionary<,> to locate IEnumerable<KeyValuePair<,>> avoids MakeGenericType and matches JObject-like types. Please ensure tests cover:
- IDictionary<string,T> (mutable), IReadOnlyDictionary<string,T>, non-generic IDictionary, and JObject.
670-679: Good fallback order and empty fast-path.Prefers configured IEnumerable path, then non-generic IDictionary, finally empty enumerator. Disposal handled elsewhere. LGTM.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/NLog.UnitTests/Internal/ExpandoTestDictionary.cs (1)
110-118: Small nits: rename loop variable and consider deduping the two iterator blocks.
- Avoid naming the variable
varto reduce confusion with the contextual keyword.- Both enumerators duplicate the same loop; optional: extract a private iterator to DRY it up.
Apply this minimal rename within the current blocks:
- foreach (DictionaryEntry var in (IDictionary)_properties) - yield return var; + foreach (DictionaryEntry entry in (IDictionary)_properties) + yield return entry;And similarly for the explicit generic implementation.
src/NLog/Internal/ObjectReflectionCache.cs (3)
634-645: AOT interface walk looks correct; minor readability tweak.Logic to locate IEnumerable<KeyValuePair<,>> via interface inspection is sound. Consider renaming
implementedInterfacestosubInterfacefor clarity.- foreach (var implementedInterfaces in interfaceType.GetInterfaces()) + foreach (var subInterface in interfaceType.GetInterfaces()) { - if (implementedInterfaces.IsGenericType - && implementedInterfaces.GetGenericTypeDefinition() == typeof(IEnumerable<>) - && implementedInterfaces.GetGenericArguments()[0].IsGenericType - && implementedInterfaces.GetGenericArguments()[0].GetGenericTypeDefinition() == typeof(KeyValuePair<,>)) + if (subInterface.IsGenericType + && subInterface.GetGenericTypeDefinition() == typeof(IEnumerable<>) + && subInterface.GetGenericArguments()[0].IsGenericType + && subInterface.GetGenericArguments()[0].GetGenericTypeDefinition() == typeof(KeyValuePair<,>)) { - var enumerator = DictionaryEnumerator.BuildEnumerator(implementedInterfaces, implementedInterfaces.GetGenericArguments()[0]); + var enumerator = DictionaryEnumerator.BuildEnumerator(subInterface, subInterface.GetGenericArguments()[0]);Please confirm this path is exercised in tests for both IDictionary<,> and IReadOnlyDictionary<,> with string keys on AOT.
687-709: Avoid potential ambiguity and reduce reflection cost for GetEnumerator.Using
dictionaryType.GetMethod(nameof(IEnumerable.GetEnumerator))can be ambiguous on types that implement both IEnumerable and IEnumerable. Prefer the non-generic interface MethodInfo, or skip reflection and callIEnumerable.GetEnumerator()directly.Minimal change (keep reflection, remove ambiguity):
- var enumeratorMethod = dictionaryType.GetMethod(nameof(IEnumerable.GetEnumerator)); + var enumeratorMethod = typeof(IEnumerable).GetMethod(nameof(IEnumerable.GetEnumerator));Alternative (no reflection, lowest overhead):
- var enumeratorMethod = dictionaryType.GetMethod(nameof(IEnumerable.GetEnumerator)); var getKeyProperty = dictionaryItemType.GetProperty(nameof(KeyValuePair<string, object>.Key)); var getValueProperty = dictionaryItemType.GetProperty(nameof(KeyValuePair<string, object>.Value)); - if (enumeratorMethod != null && getKeyProperty != null && getValueProperty != null) + if (getKeyProperty != null && getValueProperty != null) { return (collection) => { - var enumerator = (IEnumerator?)enumeratorMethod.Invoke(collection, null); + var enumerator = (collection as IEnumerable)?.GetEnumerator(); if (enumerator is null || !enumerator.MoveNext()) { (enumerator as IDisposable)?.Dispose(); return EmptyDictionaryEnumerator.Default; } return EnumerateItems(enumerator, getKeyProperty, getValueProperty); }; }Please run the unit tests on both JIT and AOT targets to ensure interface-method reflection works uniformly.
710-726: Minor: cast Key to string instead of ToString(); ensures no extra allocation.Key type is guaranteed string earlier, so a safe cast is clearer and slightly faster.
- var keyString = getKeyProperty.GetValue(item, null); - var valueObject = getValueProperty.GetValue(item, null); - yield return new KeyValuePair<string, object?>(keyString?.ToString() ?? string.Empty, valueObject); + var keyString = (string?)getKeyProperty.GetValue(item, null) ?? string.Empty; + var valueObject = getValueProperty.GetValue(item, null); + yield return new KeyValuePair<string, object?>(keyString, valueObject);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/NLog/Internal/ObjectReflectionCache.cs(4 hunks)tests/NLog.UnitTests/Internal/ExpandoTestDictionary.cs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/NLog/Internal/ObjectReflectionCache.cs (2)
tests/NLog.UnitTests/Internal/ExpandoTestDictionary.cs (4)
ToString(114-126)IEnumerator(88-91)IEnumerator(108-112)IEnumerator(128-132)src/NLog/Internal/DictionaryEntryEnumerable.cs (3)
MoveNext(94-97)MoveNext(112-112)EmptyDictionaryEnumerator(105-117)
⏰ 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). (2)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (4)
tests/NLog.UnitTests/Internal/ExpandoTestDictionary.cs (1)
43-45: LGTM: Serializable + IEnumerable extension fits test intent.No issues spotted with the added attribute or interface; aligns with the new dictionary-enumeration path.
src/NLog/Internal/ObjectReflectionCache.cs (3)
329-330: LGTM: PropertyValue.ToString()Helpful for diagnostics; no behavior risk.
660-666: Constructor-based delegate injection is a good extension point.Makes the enumerator AOT-friendly without MakeGenericType.
669-679: Good: prefers injected IEnumerable-path, falls back to IDictionary.Branching is safe; returns Empty enumerator when no items.
8140c62 to
ef841fb
Compare
fdf00fe to
0408e7f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/NLog.UnitTests/Internal/ExpandoTestDictionary.cs (1)
110-112: Minor: avoid extra iterator layer in test helperBoth enumerators currently wrap the underlying enumerator with a yield-iterator. You can return the underlying enumerator directly to cut one allocation.
- IEnumerator IEnumerable.GetEnumerator() - { - foreach (DictionaryEntry var in (IDictionary)_properties) - yield return var; - } + IEnumerator IEnumerable.GetEnumerator() + => ((IDictionary)_properties).GetEnumerator(); - IEnumerator<DictionaryEntry> IEnumerable<DictionaryEntry>.GetEnumerator() - { - foreach (DictionaryEntry var in (IDictionary)_properties) - yield return var; - } + IEnumerator<DictionaryEntry> IEnumerable<DictionaryEntry>.GetEnumerator() + { + foreach (DictionaryEntry de in (IDictionary)_properties) + yield return de; + }Also applies to: 114-118
src/NLog/Internal/ObjectReflectionCache.cs (1)
673-685: Micro: cache the IDictionary delegate to avoid per-type allocationsBuildDictionaryEnumerator() returns a new delegate each time. Since it’s type-agnostic, you can cache a single static instance and reuse it wherever needed.
- public static Func<object, IEnumerator<KeyValuePair<string, object?>>> BuildDictionaryEnumerator() - { - return (collection) => + private static readonly Func<object, IEnumerator<KeyValuePair<string, object?>>> s_enumerateIDictionary = + (collection) => { if (collection is IDictionary dictionary) { if (dictionary.Count == 0) return EmptyDictionaryEnumerator.Default; return YieldEnumerator(dictionary); } return EmptyDictionaryEnumerator.Default; }; - } + public static Func<object, IEnumerator<KeyValuePair<string, object?>>> BuildDictionaryEnumerator() => s_enumerateIDictionary;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/NLog/Internal/ObjectReflectionCache.cs(5 hunks)tests/NLog.UnitTests/Internal/ExpandoTestDictionary.cs(2 hunks)
⏰ 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). (7)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (7)
tests/NLog.UnitTests/Internal/ExpandoTestDictionary.cs (1)
43-45: Serializable + IEnumerable addition looks goodGood fit for exercising dictionary-entry enumeration paths in tests. No issues.
src/NLog/Internal/ObjectReflectionCache.cs (6)
329-330: Nice: PropertyValue.ToString()Helpful for debugging/logging. No behavior risk.
606-610: OK: generic IDictionary via non-generic pathUsing a delegate-built DictionaryEnumerator for generic IDictionary detected via non-generic IDictionary is fine and AOT-safe.
634-646: AOT-safe KeyValuePair<> enumeration pathThe reflection walk to find IEnumerable<KeyValuePair<string, T>> and build a late-bound enumerator is sound. Disposal covered in downstream iterator.
If possible, add a quick unit test covering a type that only exposes IEnumerable<KeyValuePair<string,T>> (no IDictionary) to lock this in for JObject-like objects.
661-667: Delegate-backed DictionaryEnumerator is cleanConstructor + indirection through Func<object, IEnumerator<...>> removes MakeGenericType and keeps callsites simple.
Also applies to: 668-672
693-743: Reflection-built enumerator: minor robustness nits
- GetMethod(nameof(IEnumerable.GetEnumerator)) is fine; optional: specify BindingFlags.Instance|Public to be explicit.
- The early MoveNext() probe + do/while yields the first item correctly; disposal is handled in finally. Good.
No changes required.
Please add/confirm a unit test with an IEnumerable<KeyValuePair<string,T>> source (e.g., JObject) to ensure this code path runs under AOT.
687-691: Verify DictionaryEntryEnumerable availability
Ensure thatDictionaryEntryEnumerableis defined and included in all target TFMs before relying on it. If it isn’t, replace:- foreach (var item in new DictionaryEntryEnumerable(dictionary)) + foreach (DictionaryEntry item in dictionary)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/NLog.UnitTests/Internal/ExpandoTestDictionary.cs (1)
114-118: Consider reducing duplication between enumerators.Both
GetEnumerator()implementations have identical logic. Consider having the explicit implementation delegate to the non-generic one to reduce duplication.Apply this diff to eliminate duplication:
IEnumerator<DictionaryEntry> IEnumerable<DictionaryEntry>.GetEnumerator() { - foreach (DictionaryEntry var in (IDictionary)_properties) - yield return var; + return ((IEnumerable)this).GetEnumerator() as IEnumerator<DictionaryEntry>; }Note: This assumes the cast is safe, which it is given both return the same enumeration. Alternatively, extract the loop to a private helper method.
src/NLog/Internal/ObjectReflectionCache.cs (1)
634-646: Consider simplifying the nested reflection logic for clarity.The nested conditions checking generic types work correctly but are difficult to read. Consider extracting intermediate variables to improve maintainability.
Apply this diff to improve readability:
foreach (var subInterfaceType in value.GetType().GetInterfaces()) { - if (subInterfaceType.IsGenericType - && subInterfaceType.GetGenericTypeDefinition() == typeof(IEnumerable<>) - && subInterfaceType.GetGenericArguments()[0].IsGenericType - && subInterfaceType.GetGenericArguments()[0].GetGenericTypeDefinition() == typeof(KeyValuePair<,>) - && subInterfaceType.GetGenericArguments()[0].GetGenericArguments()[0] == typeof(string)) + if (!subInterfaceType.IsGenericType || subInterfaceType.GetGenericTypeDefinition() != typeof(IEnumerable<>)) + continue; + + var enumerableItemType = subInterfaceType.GetGenericArguments()[0]; + if (!enumerableItemType.IsGenericType || enumerableItemType.GetGenericTypeDefinition() != typeof(KeyValuePair<,>)) + continue; + + var keyType = enumerableItemType.GetGenericArguments()[0]; + if (keyType == typeof(string)) { - var enumerator = DictionaryEnumerator.BuildEnumerator(subInterfaceType, subInterfaceType.GetGenericArguments()[0]); + var enumerator = DictionaryEnumerator.BuildEnumerator(subInterfaceType, enumerableItemType); if (enumerator != null) return new DictionaryEnumerator(enumerator); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/NLog/Internal/ObjectReflectionCache.cs(5 hunks)tests/NLog.UnitTests/Internal/ExpandoTestDictionary.cs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/NLog/Internal/ObjectReflectionCache.cs (3)
src/NLog/Internal/ReflectionHelpers.cs (3)
ReflectionHelpers(45-196)LateBoundMethod(72-92)LateBoundMethod(94-123)src/NLog/Internal/DictionaryEntryEnumerable.cs (3)
EmptyDictionaryEnumerator(105-117)MoveNext(94-97)MoveNext(112-112)src/NLog/Internal/ArrayHelper.cs (1)
ArrayHelper(36-53)
⏰ 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)
tests/NLog.UnitTests/Internal/ExpandoTestDictionary.cs (2)
43-44: LGTM: Interface addition aligns with enumeration requirements.The addition of
IEnumerable<DictionaryEntry>supports the new dictionary enumeration pattern tested in this PR. The[Serializable]attribute is acceptable for test fixtures.
108-112: LGTM: Non-generic enumerator correctly yields DictionaryEntry.The implementation properly casts to
IDictionaryand yieldsDictionaryEntryitems, which aligns with the test requirements for dictionary enumeration.src/NLog/Internal/ObjectReflectionCache.cs (4)
329-329: LGTM: Useful ToString() override for debugging.The
ToString()implementation provides a clear representation of the property name-value pair, which is helpful for debugging and logging scenarios.
609-609: LGTM: AOT-compatible approach using BuildDictionaryEnumerator.This change replaces the AOT-incompatible
MakeGenericTypeapproach with a reflection-basedBuildDictionaryEnumeratormethod. This correctly addresses the regression mentioned in the PR objectives.
661-671: LGTM: Clean delegation pattern for enumeration.The new constructor and field enable flexible enumeration strategies via delegation. The implementation correctly forwards the enumeration call to the provided function.
673-739: LGTM: Comprehensive AOT-compatible dictionary enumeration implementation.The new reflection-based enumeration methods correctly avoid
MakeGenericTypewhile maintaining functionality:
BuildDictionaryEnumeratorhandles non-genericIDictionarycollections efficientlyBuildEnumeratorcreates late-bound delegates for type-safe enumerationEnumerateItemsproperly yields items and ensures disposal via finally blockThe implementation correctly handles edge cases (null enumerators, empty collections) and includes proper resource cleanup.
|
AOT does not support MakeGenericType, so workaround is to improve reflection for navigating to the expected GetEnumerator method.
Fixing regression introduced with NLog v6, so it can again perform object-reflection of JObject, while still supporting AOT.