Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix reflection based destructuring for redefined property #344

Merged
merged 11 commits into from
May 26, 2021
47 changes: 46 additions & 1 deletion Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,13 @@ 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();
Expand Down Expand Up @@ -56,10 +63,48 @@ 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

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ namespace Serilog.Exceptions.Test.Destructurers
using Serilog.Exceptions.Core;
using Serilog.Exceptions.Destructurers;
using Xunit;
using Xunit.Abstractions;
using static LogJsonOutputUtils;

public class ReflectionBasedDestructurerTest
Expand Down Expand Up @@ -384,6 +385,37 @@ public void CanDestructureObjectWithHiddenProperty()
Assert.Equal(baseClass.HiddenProperty, info?[$"{typeof(BaseClass).FullName}.{nameof(BaseClass.HiddenProperty)}"]);
}

[Fact]
public void CanDestructureObjectWithRedefinedProperty()
{
var exception = new TestExceptionClassWithNewDefinition() { PublicProperty = 20 };

var propertiesBag = new ExceptionPropertiesBag(exception);
CreateReflectionBasedDestructurer().Destructure(exception, propertiesBag, EmptyDestructurer());

var properties = propertiesBag.GetResultDictionary();
var info = properties[nameof(TestExceptionClassWithNewDefinition.PublicProperty)];
}

[Fact]
public void CanDestructureObjectWithDataWithRedefinedProperty()
{
var exception = new RecursiveException
{
Node = new RecursiveNodeWithRedefinedProperty
{
Name = 123,
},
};

var propertiesBag = new ExceptionPropertiesBag(exception);
CreateReflectionBasedDestructurer().Destructure(exception, propertiesBag, EmptyDestructurer());

var properties = propertiesBag.GetResultDictionary();
var parent = (IDictionary<string, object?>?)properties[nameof(RecursiveException.Node)];
Assert.Equal(123, parent?[nameof(RecursiveNode.Name)]);
}

private static void Test_ResultOfReflectionDestructurerShouldBeEquivalentToCustomOne(
Exception exception,
IExceptionDestructurer customDestructurer)
Expand Down Expand Up @@ -555,6 +587,11 @@ public class RecursiveNode
public RecursiveNode? Child { get; set; }
}

public class RecursiveNodeWithRedefinedProperty : RecursiveNode
{
public new int Name { get; set; }
}

public class RecursiveException : Exception
{
public RecursiveNode? Node { get; set; }
Expand All @@ -578,6 +615,11 @@ internal class TestClass
public string? ReferenceType { get; set; }
}

internal class TestExceptionClassWithNewDefinition : TestException
{
public new int PublicProperty { get; set; }
}

internal class BaseClass
{
public virtual int HiddenProperty { get; set; }
Expand Down