Skip to content

Commit

Permalink
Fix property metadata retrieval in case of redefined properties (#347)
Browse files Browse the repository at this point in the history
  • Loading branch information
krajek authored Jun 14, 2021
1 parent 2df961b commit 3a82416
Show file tree
Hide file tree
Showing 6 changed files with 345 additions and 84 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
{
Expand All @@ -44,7 +41,6 @@ public ReflectionBasedDestructurer(int destructuringDepth)
}

this.destructuringDepth = destructuringDepth;
this.baseExceptionPropertiesForDestructuring = GetExceptionPropertiesForDestructuring(typeof(Exception));
}

/// <inheritdoc cref="IExceptionDestructurer.TargetTypes"/>
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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)
{
Expand All @@ -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)
{
Expand Down Expand Up @@ -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; }
}
}
}
35 changes: 35 additions & 0 deletions Source/Serilog.Exceptions/Reflection/ReflectionInfo.cs
Original file line number Diff line number Diff line change
@@ -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; }
}
}
122 changes: 122 additions & 0 deletions Source/Serilog.Exceptions/Reflection/ReflectionInfoExtractor.cs
Original file line number Diff line number Diff line change
@@ -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.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);
}
}
}
Loading

0 comments on commit 3a82416

Please sign in to comment.