Fix property metadata retrieval in case of redefined properties

Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs

@@ -7,9 +7,16 @@ namespace Serilog.Exceptions.Core
     /// <inheritdoc />
     internal class ExceptionPropertiesBag : IExceptionPropertiesBag
     {
+        /// <summary>
+        /// In theory there should not be any properties with same names passed
+        /// but just as a defensive and robustness measure, we allow small amount
+        /// so that library can keep working in case of minor error (or unexpected
+        /// scenario) on the properties provider side.
+        /// </summary>
+        private const int AcceptableNumberOfSameNameProperties = 5;
         private readonly Exception exception;
         private readonly IExceptionPropertyFilter? filter;
-        private readonly Dictionary<string, object?> properties = new();
+        private readonly Dictionary<string, object?> properties = new ();
 
         /// <summary>
         /// We keep a note on whether the results were collected to be sure that after that there are no changes. This
@@ -56,10 +63,50 @@ public void AddProperty(string key, object? value)
                 }
             }
 
-            this.properties.Add(key, value);
+            this.AddPairToProperties(key, value);
         }
 
         /// <inheritdoc />
         public bool ContainsProperty(string key) => this.properties.ContainsKey(key);
+
+        private static string GetReplacementKey(string key) => key + "$";
+
+
+        /// <summary>
+        /// We want to be as robust as possible
+        /// so even in case of multiple properties with the same name
+        /// we want to at least try carrying on and keep working.
+        /// </summary>
+        private void AddPairToProperties(string key, object? value)
+        {
+#if NET5_0
+            var i = 0;
+            while (!this.properties.TryAdd(key, value) && i < AcceptableNumberOfSameNameProperties)
+            {
+                key = GetReplacementKey(key);
+                i++;
+            }
+#else
+            key = this.MakeSureKeyIsUnique(key);
+
+            this.properties.Add(key, value);
+#endif
+        }
+
+#if !NET5_0
+
+        private string MakeSureKeyIsUnique(string key)
+        {
+            var i = 0;
+            while (this.properties.ContainsKey(key) && i < AcceptableNumberOfSameNameProperties)
+            {
+                key = GetReplacementKey(key);
+                i++;
+            }
+
+            return key;
+        }
+#endif
+
     }
 }

Source/Serilog.Exceptions/Destructurers/ReflectionBasedDestructurer.cs

@@ -9,6 +9,7 @@ namespace Serilog.Exceptions.Destructurers
     using System.Reflection;
     using System.Threading;
     using System.Threading.Tasks;
+    using Reflection;
     using Serilog.Exceptions.Core;
 
     /// <summary>
@@ -23,10 +24,7 @@ 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 ReflectionInfoExtractor reflectionInfoExtractor = new();
 
         /// <summary>
         /// Initializes a new instance of the <see cref="ReflectionBasedDestructurer"/> class.
@@ -44,7 +42,6 @@ public ReflectionBasedDestructurer(int destructuringDepth)
             }
 
             this.destructuringDepth = destructuringDepth;
-            this.baseExceptionPropertiesForDestructuring = GetExceptionPropertiesForDestructuring(typeof(Exception));
         }
 
         /// <inheritdoc cref="IExceptionDestructurer.TargetTypes"/>
@@ -82,7 +79,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 +110,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 +171,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 +303,7 @@ private object DestructureValueDictionary(
             var values = new Dictionary<string, object?>();
             destructuredObjects.Add(value, values);
 
-            var reflectionInfo = this.GetOrCreateReflectionInfo(valueType);
+            var reflectionInfo = reflectionInfoExtractor.GetOrCreateReflectionInfo(valueType);
 
             foreach (var property in reflectionInfo.Properties)
             {
@@ -347,8 +315,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 +365,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,15 @@
+namespace Serilog.Exceptions.Reflection
+{
+    internal class ReflectionInfo
+    {
+        public ReflectionInfo(ReflectionPropertyInfo[] properties, ReflectionPropertyInfo[] propertiesExceptBaseOnes)
+        {
+            this.Properties = properties;
+            this.PropertiesExceptBaseOnes = propertiesExceptBaseOnes;
+        }
+
+        public ReflectionPropertyInfo[] Properties { get; }
+
+        public ReflectionPropertyInfo[] PropertiesExceptBaseOnes { get; }
+    }
+}

Source/Serilog.Exceptions/Reflection/ReflectionInfoExtractor.cs

@@ -0,0 +1,104 @@
+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;
+
+        public ReflectionInfoExtractor() => this.baseExceptionPropertiesForDestructuring = GetExceptionPropertiesForDestructuring(typeof(Exception));
+
+
+        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 group 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};
+                }
+            }
+
+            // Fix groups that have more than one property in it
+            foreach (var nameGroup in groupedByName)
+            {
+                if (nameGroup.Value.Count > 1)
+                {
+                    foreach (var propertyInfoInGroupName in nameGroup.Value)
+                    {
+                        foreach (var otherPropertyInfoInGroupName in nameGroup.Value)
+                        {
+                            propertyInfoInGroupName.MarkNameWithFullNameIRedefineThatProperty(otherPropertyInfoInGroupName);
+                        }
+                    }
+                }
+            }
+        }
+
+        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);
+        }
+    }
+}