diff --git a/Source/Serilog.Exceptions/Destructurers/ReflectionBasedDestructurer.cs b/Source/Serilog.Exceptions/Destructurers/ReflectionBasedDestructurer.cs index 31dc2aa5..d6e27e0d 100644 --- a/Source/Serilog.Exceptions/Destructurers/ReflectionBasedDestructurer.cs +++ b/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; /// /// 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 reflectionInfoCache = new(); - private readonly PropertyInfo[] baseExceptionPropertiesForDestructuring; + private readonly ReflectionInfoExtractor reflectionInfoExtractor = new(); /// /// Initializes a new instance of the class. /// - /// Maximum depth to which destructurer will go when destructuring exception + /// Maximum depth to which the destructurer will go when destructuring exception /// object graph. public ReflectionBasedDestructurer(int destructuringDepth) { @@ -44,7 +41,6 @@ public ReflectionBasedDestructurer(int destructuringDepth) } this.destructuringDepth = destructuringDepth; - this.baseExceptionPropertiesForDestructuring = GetExceptionPropertiesForDestructuring(typeof(Exception)); } /// @@ -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 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>(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(); 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 getter) - { - this.Name = name; - this.DeclaringType = declaringType; - this.Getter = getter; - } - - public string Name { get; } - - public Type? DeclaringType { get; } - - public Func Getter { get; } - } } } diff --git a/Source/Serilog.Exceptions/Reflection/ReflectionInfo.cs b/Source/Serilog.Exceptions/Reflection/ReflectionInfo.cs new file mode 100644 index 00000000..29fac7ea --- /dev/null +++ b/Source/Serilog.Exceptions/Reflection/ReflectionInfo.cs @@ -0,0 +1,35 @@ +namespace Serilog.Exceptions.Reflection +{ + using System; + + /// + /// Contains metadata information about a type + /// useful for destructuring process. + /// + internal class ReflectionInfo + { + /// + /// Initializes a new instance of the class. + /// + /// All properties for a type. + /// All properties except of properties which are handled separately. + public ReflectionInfo( + ReflectionPropertyInfo[] properties, + ReflectionPropertyInfo[] propertiesExceptBaseOnes) + { + this.Properties = properties; + this.PropertiesExceptBaseOnes = propertiesExceptBaseOnes; + } + + /// + /// Gets information about all properties to be destructured. + /// + public ReflectionPropertyInfo[] Properties { get; } + + /// + /// Gets information about properties other than base exception properties + /// which are handled separately. + /// + public ReflectionPropertyInfo[] PropertiesExceptBaseOnes { get; } + } +} diff --git a/Source/Serilog.Exceptions/Reflection/ReflectionInfoExtractor.cs b/Source/Serilog.Exceptions/Reflection/ReflectionInfoExtractor.cs new file mode 100644 index 00000000..6595259c --- /dev/null +++ b/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; + + /// + /// Utility class that analyzes type using reflection and provides + /// information about properties to be used in destructuring process. + /// + internal class ReflectionInfoExtractor + { + private readonly object lockObj = new(); + private readonly Dictionary reflectionInfoCache = new(); + private readonly IList baseExceptionPropertiesForDestructuring; + + /// + /// Initializes a new instance of the class. + /// + public ReflectionInfoExtractor() => this.baseExceptionPropertiesForDestructuring = GetExceptionPropertiesForDestructuring(typeof(Exception)); + + /// + /// Gets reflection info for relevant properties of "/> + /// from cache or by generating it if they are not yet present in cache. + /// + /// The type for which properties are to be analyzed. + /// The reflection info for relevant properties of . + 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 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>(typedResult, objParam); + return resultLambda.Compile(); + } + + private static List 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>(); + foreach (var propertyInfo in propertyInfos) + { + if (groupedByName.ContainsKey(propertyInfo.Name)) + { + groupedByName[propertyInfo.Name].Add(propertyInfo); + } + else + { + groupedByName[propertyInfo.Name] = new List { 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 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.MarkNameWithTypeNameIfRedefinesThatProperty(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); + } + } +} diff --git a/Source/Serilog.Exceptions/Reflection/ReflectionPropertyInfo.cs b/Source/Serilog.Exceptions/Reflection/ReflectionPropertyInfo.cs new file mode 100644 index 00000000..8ba3c121 --- /dev/null +++ b/Source/Serilog.Exceptions/Reflection/ReflectionPropertyInfo.cs @@ -0,0 +1,105 @@ +namespace Serilog.Exceptions.Reflection +{ + using System; +#if NETSTANDARD1_3 || NETSTANDARD1_6 + using System.Reflection; +#endif + + /// + /// Class that holds reflection information about a single property. + /// + internal class ReflectionPropertyInfo + { + /// + /// Marker that represents a decision whether to extend property name + /// with type name of declaring type, to avoid name uniqueness conflicts. + /// + private bool markedWithTypeName; + + /// + /// Initializes a new instance of the class. + /// + /// The of the property without type name. + /// The type which declares the property. + /// Runtime function that can extract value of the property from object. + public ReflectionPropertyInfo(string name, Type? declaringType, Func getter) + { + this.Name = name; + this.DeclaringType = declaringType; + this.Getter = getter; + } + + /// + /// Gets name of the property. + /// It includes type name if name conflicts with other property of derived class. + /// + public string Name { get; private set; } + + /// + /// Gets the type that declared the property. + /// + public Type? DeclaringType { get; } + + /// + /// Gets a function that extracts value of the property from an object. + /// + public Func Getter { get; } + + /// + /// Idempotent action that extends property name with a type name + /// of its declaring type to avoid name uniqueness conflict. + /// + public void MarkNameWithTypeName() + { + if (!this.markedWithTypeName) + { + this.markedWithTypeName = true; + this.Name = $"{this.DeclaringType?.Name}.{this.Name}"; + } + } + + /// + /// Conditionally marks property with its declaring type name to avoid + /// name uniqueness conflict. + /// + /// Other property info that is be compared to detect name uniqueness conflict. + public void MarkNameWithTypeNameIfRedefinesThatProperty(ReflectionPropertyInfo otherPropertyInfo) + { + if (otherPropertyInfo == this) + { + return; + } + + if (this.DeclaringType == null || otherPropertyInfo?.DeclaringType == null) + { + return; + } + + if (otherPropertyInfo?.Name != this.Name) + { + return; + } + + if (this.markedWithTypeName) + { + return; + } + + var shouldMarkWithTypeName = IsSubTypeOf( + otherPropertyInfo.DeclaringType, + this.DeclaringType); + + if (shouldMarkWithTypeName) + { + this.MarkNameWithTypeName(); + } + } + + private static bool IsSubTypeOf(Type possibleSubType, Type possibleBaseType) => +#if NETSTANDARD1_3 || NETSTANDARD1_6 + possibleBaseType.GetTypeInfo().IsSubclassOf(possibleBaseType); +#else + possibleSubType.IsSubclassOf(possibleBaseType); +#endif + } +} diff --git a/Tests/Serilog.Exceptions.Test/Destructurers/ReflectionBasedDestructurerTest.cs b/Tests/Serilog.Exceptions.Test/Destructurers/ReflectionBasedDestructurerTest.cs index f000a1b0..0915c79e 100644 --- a/Tests/Serilog.Exceptions.Test/Destructurers/ReflectionBasedDestructurerTest.cs +++ b/Tests/Serilog.Exceptions.Test/Destructurers/ReflectionBasedDestructurerTest.cs @@ -382,7 +382,7 @@ public void CanDestructureObjectWithHiddenProperty() var properties = propertiesBag.GetResultDictionary(); var info = properties[nameof(HiddenException.Info)] as IDictionary; Assert.Equal(derived.HiddenProperty, info?[nameof(DerivedClass.HiddenProperty)]); - Assert.Equal(baseClass.HiddenProperty, info?[$"{typeof(BaseClass).FullName}.{nameof(BaseClass.HiddenProperty)}"]); + Assert.Equal(baseClass.HiddenProperty, info?[$"{nameof(BaseClass)}.{nameof(BaseClass.HiddenProperty)}"]); } [Fact] diff --git a/Tests/Serilog.Exceptions.Test/Reflection/ReflectionInfoExtractorTest.cs b/Tests/Serilog.Exceptions.Test/Reflection/ReflectionInfoExtractorTest.cs new file mode 100644 index 00000000..263cd44c --- /dev/null +++ b/Tests/Serilog.Exceptions.Test/Reflection/ReflectionInfoExtractorTest.cs @@ -0,0 +1,76 @@ +namespace Serilog.Exceptions.Test.Reflection +{ + using System.Linq; + using FluentAssertions; + using Serilog.Exceptions.Reflection; + using Xunit; + + public class ReflectionInfoExtractorTest + { + private readonly ReflectionInfoExtractor reflectionInfoExtractor = new(); + + [Fact] + public void GivenObjectWithRedefinedProperty_ShouldDiscardBaseClassProperty() + { + var testObject = new TestObjectWithRedefinedProperty() { Name = 123 }; + + var reflectionInfo = this.reflectionInfoExtractor.GetOrCreateReflectionInfo(typeof(TestObjectWithRedefinedProperty)); + + reflectionInfo.Properties.Should().HaveCount(2); + + var namePropertyInfo = reflectionInfo.Properties.Should().ContainSingle(x => x.Name == "Name").Which; + namePropertyInfo.Name.Should().Be(nameof(TestObjectWithRedefinedProperty.Name)); + namePropertyInfo.DeclaringType.Should().Be(typeof(TestObjectWithRedefinedProperty)); + var nameGetter = namePropertyInfo.Getter; + var testObjectName = nameGetter(testObject); + testObjectName.Should().BeOfType().Which.Should().Be(123); + + var baseClassPropertyInfo = reflectionInfo + .Properties.Should().ContainSingle(x => x.Name == "TestObject.Name").Which; + var baseClassNameGetter = baseClassPropertyInfo.Getter; + var baseClassTestObjectName = baseClassNameGetter(testObject); + baseClassTestObjectName.Should().BeNull(); + } + + [Fact] + public void GivenObjectWithDoubleRedefinedProperty_ShouldMarkBaseClassPropertiesWithFullName() + { + var testObject = new TestObjectWithDoubleRedefinedProperty() { Name = 456.789 }; + + var reflectionInfo = this.reflectionInfoExtractor.GetOrCreateReflectionInfo(typeof(TestObjectWithDoubleRedefinedProperty)); + + var propertyNames = reflectionInfo.Properties + .Select(x => x.Name) + .ToList(); + propertyNames.Should().BeEquivalentTo( + new[] + { + "Name", + "TestObjectWithRedefinedProperty.Name", + "TestObject.Name", + }, + x => x.WithoutStrictOrdering()); + var namePropertyInfo = reflectionInfo.Properties.Should().ContainSingle(x => x.Name == "Name").Which; + namePropertyInfo.Name.Should().Be(nameof(TestObjectWithDoubleRedefinedProperty.Name)); + namePropertyInfo.DeclaringType.Should().Be(typeof(TestObjectWithDoubleRedefinedProperty)); + var nameGetter = namePropertyInfo.Getter; + var testObjectName = nameGetter(testObject); + testObjectName.Should().BeOfType().Which.Should().Be(456.789); + } + + public class TestObject + { + public string? Name { get; set; } + } + + public class TestObjectWithRedefinedProperty : TestObject + { + public new int Name { get; set; } + } + + public class TestObjectWithDoubleRedefinedProperty : TestObjectWithRedefinedProperty + { + public new double Name { get; set; } + } + } +}