Fix property metadata retrieval in case of redefined properties

Source/Serilog.Exceptions/Destructurers/ReflectionBasedDestructurer.cs

@@ -5,11 +5,11 @@ namespace Serilog.Exceptions.Destructurers
     using System.Collections.Generic;
     using System.Globalization;
     using System.Linq;
-    using System.Linq.Expressions;
     using System.Reflection;
     using System.Threading;
     using System.Threading.Tasks;
     using Serilog.Exceptions.Core;
+    using Serilog.Exceptions.Reflection;
 
     /// <summary>
     /// Destructures exceptions by gathering all public non-indexer properties using reflection and then dynamically
@@ -23,15 +23,12 @@ public class ReflectionBasedDestructurer : IExceptionDestructurer
         private const string RefLabel = "$ref";
         private const string CyclicReferenceMessage = "Cyclic reference";
         private readonly int destructuringDepth;
-        private readonly object lockObj = new();
-
-        private readonly Dictionary<Type, ReflectionInfo> reflectionInfoCache = new();
-        private readonly PropertyInfo[] baseExceptionPropertiesForDestructuring;
+        private readonly ReflectionInfoExtractor reflectionInfoExtractor = new();
 
         /// <summary>
         /// Initializes a new instance of the <see cref="ReflectionBasedDestructurer"/> class.
         /// </summary>
-        /// <param name="destructuringDepth">Maximum depth to which destructurer will go when destructuring exception
+        /// <param name="destructuringDepth">Maximum depth to which the destructurer will go when destructuring exception
         /// object graph.</param>
         public ReflectionBasedDestructurer(int destructuringDepth)
         {
@@ -44,7 +41,6 @@ public ReflectionBasedDestructurer(int destructuringDepth)
             }
 
             this.destructuringDepth = destructuringDepth;
-            this.baseExceptionPropertiesForDestructuring = GetExceptionPropertiesForDestructuring(typeof(Exception));
         }
 
         /// <inheritdoc cref="IExceptionDestructurer.TargetTypes"/>
@@ -82,7 +78,7 @@ public void Destructure(
                 destructureException,
                 data => this.DestructureValueDictionary(data, 1, destructuredObjects, ref nextCyclicRefId));
 
-            var reflectionInfo = this.GetOrCreateReflectionInfo(exception.GetType());
+            var reflectionInfo = this.reflectionInfoExtractor.GetOrCreateReflectionInfo(exception.GetType());
 
             this.AppendProperties(
                 exception,
@@ -113,22 +109,6 @@ public void Destructure(
             return refId;
         }
 
-        private static Func<object, object> GenerateFastGetterForProperty(Type type, PropertyInfo property)
-        {
-            var objParam = Expression.Parameter(typeof(object), "num");
-            var typedObj = Expression.Convert(objParam, type);
-            var memberExpression = Expression.Property(typedObj, property);
-            var typedResult = Expression.Convert(memberExpression, typeof(object));
-            var resultLambda = Expression.Lambda<Func<object, object>>(typedResult, objParam);
-            return resultLambda.Compile();
-        }
-
-        private static PropertyInfo[] GetExceptionPropertiesForDestructuring(Type valueType) =>
-            valueType
-                .GetProperties(BindingFlags.Public | BindingFlags.Instance)
-                .Where(x => x.CanRead && x.GetIndexParameters().Length == 0)
-                .ToArray();
-
         private static object DestructureUri(Uri value) => value.ToString();
 
         private static void AppendTypeIfPossible(IExceptionPropertiesBag propertiesBag, Type valueType)
@@ -190,19 +170,6 @@ private void AppendProperties(
             }
         }
 
-        private ReflectionInfo GenerateReflectionInfoForType(Type valueType)
-        {
-            var properties = GetExceptionPropertiesForDestructuring(valueType);
-            var propertyInfos = properties
-                .Select(p => new ReflectionPropertyInfo(p.Name, p.DeclaringType, GenerateFastGetterForProperty(valueType, p)))
-                .ToArray();
-            var propertiesInfosExceptBaseOnes = propertyInfos
-                .Where(p => this.baseExceptionPropertiesForDestructuring.All(bp => bp.Name != p.Name))
-                .ToArray();
-
-            return new ReflectionInfo(propertyInfos, propertiesInfosExceptBaseOnes);
-        }
-
         private object? DestructureValue(
             object? value,
             int level,
@@ -335,7 +302,7 @@ private object DestructureValueDictionary(
             var values = new Dictionary<string, object?>();
             destructuredObjects.Add(value, values);
 
-            var reflectionInfo = this.GetOrCreateReflectionInfo(valueType);
+            var reflectionInfo = this.reflectionInfoExtractor.GetOrCreateReflectionInfo(valueType);
 
             foreach (var property in reflectionInfo.Properties)
             {
@@ -347,8 +314,7 @@ private object DestructureValueDictionary(
                         level + 1,
                         destructuredObjects,
                         ref nextCyclicRefId);
-                    var key = values.ContainsKey(property.Name) ? $"{property.DeclaringType?.FullName}.{property.Name}" : property.Name;
-                    values.Add(key, destructuredValue);
+                    values.Add(property.Name, destructuredValue);
                 }
                 catch (TargetInvocationException targetInvocationException)
                 {
@@ -398,48 +364,5 @@ private object DestructureTask(
 
             return values;
         }
-
-        private ReflectionInfo GetOrCreateReflectionInfo(Type valueType)
-        {
-            lock (this.lockObj)
-            {
-                if (!this.reflectionInfoCache.TryGetValue(valueType, out var reflectionInfo))
-                {
-                    reflectionInfo = this.GenerateReflectionInfoForType(valueType);
-                    this.reflectionInfoCache.Add(valueType, reflectionInfo);
-                }
-
-                return reflectionInfo;
-            }
-        }
-
-        private class ReflectionInfo
-        {
-            public ReflectionInfo(ReflectionPropertyInfo[] properties, ReflectionPropertyInfo[] propertiesExceptBaseOnes)
-            {
-                this.Properties = properties;
-                this.PropertiesExceptBaseOnes = propertiesExceptBaseOnes;
-            }
-
-            public ReflectionPropertyInfo[] Properties { get; }
-
-            public ReflectionPropertyInfo[] PropertiesExceptBaseOnes { get; }
-        }
-
-        private class ReflectionPropertyInfo
-        {
-            public ReflectionPropertyInfo(string name, Type? declaringType, Func<object, object> getter)
-            {
-                this.Name = name;
-                this.DeclaringType = declaringType;
-                this.Getter = getter;
-            }
-
-            public string Name { get; }
-
-            public Type? DeclaringType { get; }
-
-            public Func<object, object> Getter { get; }
-        }
     }
 }

Source/Serilog.Exceptions/Reflection/ReflectionInfo.cs

@@ -0,0 +1,35 @@
+namespace Serilog.Exceptions.Reflection
+{
+    using System;
+
+    /// <summary>
+    /// Contains metadata information about a type
+    /// useful for destructuring process.
+    /// </summary>
+    internal class ReflectionInfo
+    {
+        /// <summary>
+        /// Initializes a new instance of the <see cref="ReflectionInfo"/> class.
+        /// </summary>
+        /// <param name="properties">All properties for a type.</param>
+        /// <param name="propertiesExceptBaseOnes">All properties except of <see cref="Exception"/> properties which are handled separately.</param>
+        public ReflectionInfo(
+            ReflectionPropertyInfo[] properties,
+            ReflectionPropertyInfo[] propertiesExceptBaseOnes)
+        {
+            this.Properties = properties;
+            this.PropertiesExceptBaseOnes = propertiesExceptBaseOnes;
+        }
+
+        /// <summary>
+        /// Gets information about all properties to be destructured.
+        /// </summary>
+        public ReflectionPropertyInfo[] Properties { get; }
+
+        /// <summary>
+        /// Gets information about properties other than base exception properties
+        /// which are handled separately.
+        /// </summary>
+        public ReflectionPropertyInfo[] PropertiesExceptBaseOnes { get; }
+    }
+}

Source/Serilog.Exceptions/Reflection/ReflectionInfoExtractor.cs

@@ -0,0 +1,122 @@
+namespace Serilog.Exceptions.Reflection
+{
+    using System;
+    using System.Collections.Generic;
+    using System.Linq;
+    using System.Linq.Expressions;
+    using System.Reflection;
+
+    /// <summary>
+    /// Utility class that analyzes type using reflection and provides
+    /// information about properties to be used in destructuring process.
+    /// </summary>
+    internal class ReflectionInfoExtractor
+    {
+        private readonly object lockObj = new();
+        private readonly Dictionary<Type, ReflectionInfo> reflectionInfoCache = new();
+        private readonly IList<PropertyInfo> baseExceptionPropertiesForDestructuring;
+
+        /// <summary>
+        /// Initializes a new instance of the <see cref="ReflectionInfoExtractor"/> class.
+        /// </summary>
+        public ReflectionInfoExtractor() => this.baseExceptionPropertiesForDestructuring = GetExceptionPropertiesForDestructuring(typeof(Exception));
+
+        /// <summary>
+        /// Gets reflection info for relevant properties of <paramref name="valueType"/>"/>
+        /// from cache or by generating it if they are not yet present in cache.
+        /// </summary>
+        /// <param name="valueType">The type for which properties are to be analyzed.</param>
+        /// <returns>The reflection info for relevant properties of <paramref name="valueType"/>.</returns>
+        public ReflectionInfo GetOrCreateReflectionInfo(Type valueType)
+        {
+            lock (this.lockObj)
+            {
+                if (!this.reflectionInfoCache.TryGetValue(valueType, out var reflectionInfo))
+                {
+                    reflectionInfo = this.GenerateReflectionInfoForType(valueType);
+                    this.reflectionInfoCache.Add(valueType, reflectionInfo);
+                }
+
+                return reflectionInfo;
+            }
+        }
+
+        private static Func<object, object> GenerateFastGetterForProperty(Type type, PropertyInfo property)
+        {
+            var objParam = Expression.Parameter(typeof(object), "num");
+            var typedObj = Expression.Convert(objParam, type);
+            var memberExpression = Expression.Property(typedObj, property);
+            var typedResult = Expression.Convert(memberExpression, typeof(object));
+            var resultLambda = Expression.Lambda<Func<object, object>>(typedResult, objParam);
+            return resultLambda.Compile();
+        }
+
+        private static List<PropertyInfo> GetExceptionPropertiesForDestructuring(Type valueType)
+        {
+            var allProperties = valueType
+                .GetProperties(BindingFlags.Public | BindingFlags.Instance)
+                .Where(x => x.CanRead && x.GetIndexParameters().Length == 0)
+                .ToList();
+
+            return allProperties;
+        }
+
+        private static void MarkRedefinedPropertiesWithFullName(ReflectionPropertyInfo[] propertyInfos)
+        {
+            // First, prepare a dictionary of properties grouped by name
+            var groupedByName = new Dictionary<string, List<ReflectionPropertyInfo>>();
+            foreach (var propertyInfo in propertyInfos)
+            {
+                if (groupedByName.ContainsKey(propertyInfo.Name))
+                {
+                    groupedByName[propertyInfo.Name].Add(propertyInfo);
+                }
+                else
+                {
+                    groupedByName[propertyInfo.Name] = new List<ReflectionPropertyInfo> { propertyInfo };
+                }
+            }
+
+            // Then, fix groups that have more than one property in it
+            // It means that there is a name uniqueness conflict which needs to be resolved
+            foreach (var nameAndProperties in groupedByName)
+            {
+                var properties = nameAndProperties.Value;
+                if (properties.Count > 1)
+                {
+                    FixGroupOfPropertiesWithConflictingNames(properties);
+                }
+            }
+        }
+
+        private static void FixGroupOfPropertiesWithConflictingNames(List<ReflectionPropertyInfo> properties)
+        {
+            // Very simplistic approach, just check each pair separately.
+            // The implementation has O(N^2) complexity but in practice
+            // N will be extremely rarely other than 2.
+            foreach (var property in properties)
+            {
+                foreach (var otherProperty in properties)
+                {
+                    property.MarkNameWithFullNameIfRedefinesThatProperty(otherProperty);
+                }
+            }
+        }
+
+        private ReflectionInfo GenerateReflectionInfoForType(Type valueType)
+        {
+            var properties = GetExceptionPropertiesForDestructuring(valueType);
+            var propertyInfos = properties
+                .Select(p => new ReflectionPropertyInfo(p.Name, p.DeclaringType, GenerateFastGetterForProperty(valueType, p)))
+                .ToArray();
+
+            MarkRedefinedPropertiesWithFullName(propertyInfos);
+
+            var propertiesInfosExceptBaseOnes = propertyInfos
+                .Where(p => this.baseExceptionPropertiesForDestructuring.All(bp => bp.Name != p.Name))
+                .ToArray();
+
+            return new ReflectionInfo(propertyInfos, propertiesInfosExceptBaseOnes);
+        }
+    }
+}