From f6b1daabf30d005149bcc8113197b1107c038167 Mon Sep 17 00:00:00 2001 From: krajek Date: Sat, 15 May 2021 09:35:55 +0200 Subject: [PATCH 01/11] Fix reflection based destructuring for redefined property --- .../Core/ExceptionPropertiesBag.cs | 19 +++++++++++++++ .../ReflectionBasedDestructurerTest.cs | 23 +++++++++++++++++++ 2 files changed, 42 insertions(+) diff --git a/Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs b/Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs index 84db0002..e082ddb6 100644 --- a/Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs +++ b/Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs @@ -56,9 +56,28 @@ public void AddProperty(string key, object? value) } } + key = this.MakeSureKeyIsUnique(key); + this.properties.Add(key, value); } + /// + /// 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. + /// + private string MakeSureKeyIsUnique(string key) + { + var i = 0; + while (this.properties.ContainsKey(key) && i < 5) + { + key += "$"; + i++; + } + + return key; + } + /// public bool ContainsProperty(string key) => this.properties.ContainsKey(key); } diff --git a/Tests/Serilog.Exceptions.Test/Destructurers/ReflectionBasedDestructurerTest.cs b/Tests/Serilog.Exceptions.Test/Destructurers/ReflectionBasedDestructurerTest.cs index 74e95b3f..9c726a50 100644 --- a/Tests/Serilog.Exceptions.Test/Destructurers/ReflectionBasedDestructurerTest.cs +++ b/Tests/Serilog.Exceptions.Test/Destructurers/ReflectionBasedDestructurerTest.cs @@ -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 @@ -190,6 +191,8 @@ public void CanDestructureClassDataItem() Assert.Equal("ABC", destructuredStructDictionary[nameof(TestClass.ReferenceType)]); } + + [Fact] public void DestructuringDepthIsLimitedByConfiguredDepth() { @@ -384,6 +387,21 @@ 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)]; + } + + + private static void Test_ResultOfReflectionDestructurerShouldBeEquivalentToCustomOne( Exception exception, IExceptionDestructurer customDestructurer) @@ -578,6 +596,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; } From f76a3e6a8fd8db57cda6dbebf7fe3e80fec61cbf Mon Sep 17 00:00:00 2001 From: krajek Date: Sat, 15 May 2021 10:02:20 +0200 Subject: [PATCH 02/11] Optimized version for NET5 --- .../Core/ExceptionPropertiesBag.cs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs b/Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs index e082ddb6..3cd07431 100644 --- a/Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs +++ b/Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs @@ -56,9 +56,23 @@ public void AddProperty(string key, object? value) } } + this.AddPairToProperties(key, value); + } + + private void AddPairToProperties(string key, object? value) + { +#if NET5_0 + int i = 0; + while (!this.properties.TryAdd(key, value) && i < 5) + { + key += "$"; + i++; + } +#else key = this.MakeSureKeyIsUnique(key); this.properties.Add(key, value); +#endif } /// @@ -68,6 +82,7 @@ public void AddProperty(string key, object? value) /// private string MakeSureKeyIsUnique(string key) { + var i = 0; while (this.properties.ContainsKey(key) && i < 5) { From c7a037e8c291e3ef95d31dfa60dec52ddae069fe Mon Sep 17 00:00:00 2001 From: krajek Date: Sat, 15 May 2021 10:12:19 +0200 Subject: [PATCH 03/11] Fix style warnings --- Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs b/Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs index 3cd07431..d23df257 100644 --- a/Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs +++ b/Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs @@ -59,10 +59,13 @@ public void AddProperty(string key, object? value) this.AddPairToProperties(key, value); } + /// + public bool ContainsProperty(string key) => this.properties.ContainsKey(key); + private void AddPairToProperties(string key, object? value) { #if NET5_0 - int i = 0; + var i = 0; while (!this.properties.TryAdd(key, value) && i < 5) { key += "$"; @@ -82,7 +85,6 @@ private void AddPairToProperties(string key, object? value) /// private string MakeSureKeyIsUnique(string key) { - var i = 0; while (this.properties.ContainsKey(key) && i < 5) { @@ -92,8 +94,5 @@ private string MakeSureKeyIsUnique(string key) return key; } - - /// - public bool ContainsProperty(string key) => this.properties.ContainsKey(key); } } From b37bb300ac5a5b9df508d0eb323e1b93410c4386 Mon Sep 17 00:00:00 2001 From: krajek Date: Sat, 15 May 2021 10:14:36 +0200 Subject: [PATCH 04/11] MakeSureKeyIsUnique only for !NET5_0 --- Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs b/Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs index d23df257..51930f6e 100644 --- a/Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs +++ b/Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs @@ -78,6 +78,7 @@ private void AddPairToProperties(string key, object? value) #endif } +#if !NET5_0 /// /// We want to be as robust as possible /// so even in case of multiple properties with the same name @@ -95,4 +96,5 @@ private string MakeSureKeyIsUnique(string key) return key; } } +#endif } From e8798b4acb15cfcc43f7b1baf6f553c56baa1430 Mon Sep 17 00:00:00 2001 From: krajek Date: Sat, 15 May 2021 10:15:00 +0200 Subject: [PATCH 05/11] Build fix --- Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs b/Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs index 51930f6e..8922ef51 100644 --- a/Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs +++ b/Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs @@ -95,6 +95,6 @@ private string MakeSureKeyIsUnique(string key) return key; } - } #endif + } } From e510174951c4bba64431382403d35a8fd574016b Mon Sep 17 00:00:00 2001 From: krajek Date: Sat, 15 May 2021 15:17:41 +0200 Subject: [PATCH 06/11] Remove multipe blank lines in row --- .../Destructurers/ReflectionBasedDestructurerTest.cs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/Tests/Serilog.Exceptions.Test/Destructurers/ReflectionBasedDestructurerTest.cs b/Tests/Serilog.Exceptions.Test/Destructurers/ReflectionBasedDestructurerTest.cs index 9c726a50..1c4b0e09 100644 --- a/Tests/Serilog.Exceptions.Test/Destructurers/ReflectionBasedDestructurerTest.cs +++ b/Tests/Serilog.Exceptions.Test/Destructurers/ReflectionBasedDestructurerTest.cs @@ -191,8 +191,6 @@ public void CanDestructureClassDataItem() Assert.Equal("ABC", destructuredStructDictionary[nameof(TestClass.ReferenceType)]); } - - [Fact] public void DestructuringDepthIsLimitedByConfiguredDepth() { @@ -390,7 +388,6 @@ public void CanDestructureObjectWithHiddenProperty() [Fact] public void CanDestructureObjectWithRedefinedProperty() { - var exception = new TestExceptionClassWithNewDefinition() { PublicProperty = 20 }; var propertiesBag = new ExceptionPropertiesBag(exception); @@ -400,8 +397,6 @@ public void CanDestructureObjectWithRedefinedProperty() var info = properties[nameof(TestExceptionClassWithNewDefinition.PublicProperty)]; } - - private static void Test_ResultOfReflectionDestructurerShouldBeEquivalentToCustomOne( Exception exception, IExceptionDestructurer customDestructurer) From 9f9ed8be14c6f169e331a246d2341f76113f8d01 Mon Sep 17 00:00:00 2001 From: krajek Date: Mon, 17 May 2021 09:12:55 +0200 Subject: [PATCH 07/11] Document limit of 5 same named properties --- .../Core/ExceptionPropertiesBag.cs | 24 ++++++++++++------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs b/Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs index 8922ef51..eb423319 100644 --- a/Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs +++ b/Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs @@ -7,9 +7,16 @@ namespace Serilog.Exceptions.Core /// internal class ExceptionPropertiesBag : IExceptionPropertiesBag { + /// + /// 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. + /// + private const int AcceptableNumberOfSameNameProperties = 5; private readonly Exception exception; private readonly IExceptionPropertyFilter? filter; - private readonly Dictionary properties = new(); + private readonly Dictionary properties = new (); /// /// We keep a note on whether the results were collected to be sure that after that there are no changes. This @@ -62,11 +69,16 @@ public void AddProperty(string key, object? value) /// public bool ContainsProperty(string key) => this.properties.ContainsKey(key); + /// + /// 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. + /// private void AddPairToProperties(string key, object? value) { #if NET5_0 var i = 0; - while (!this.properties.TryAdd(key, value) && i < 5) + while (!this.properties.TryAdd(key, value) && i < AcceptableNumberOfSameNameProperties) { key += "$"; i++; @@ -79,15 +91,11 @@ private void AddPairToProperties(string key, object? value) } #if !NET5_0 - /// - /// 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. - /// + private string MakeSureKeyIsUnique(string key) { var i = 0; - while (this.properties.ContainsKey(key) && i < 5) + while (this.properties.ContainsKey(key) && i < AcceptableNumberOfSameNameProperties) { key += "$"; i++; From dfcf04c92b0be333f2f99e166999aa39abf0f818 Mon Sep 17 00:00:00 2001 From: krajek Date: Mon, 17 May 2021 11:58:27 +0200 Subject: [PATCH 08/11] Extract GetReplacementKey --- Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs b/Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs index eb423319..7353c7ad 100644 --- a/Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs +++ b/Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs @@ -69,6 +69,9 @@ public void AddProperty(string key, object? value) /// public bool ContainsProperty(string key) => this.properties.ContainsKey(key); + private static string GetReplacementKey(string key) => key + "$"; + + /// /// We want to be as robust as possible /// so even in case of multiple properties with the same name @@ -80,7 +83,7 @@ private void AddPairToProperties(string key, object? value) var i = 0; while (!this.properties.TryAdd(key, value) && i < AcceptableNumberOfSameNameProperties) { - key += "$"; + key = GetReplacementKey(key); i++; } #else @@ -97,12 +100,13 @@ private string MakeSureKeyIsUnique(string key) var i = 0; while (this.properties.ContainsKey(key) && i < AcceptableNumberOfSameNameProperties) { - key += "$"; + key = GetReplacementKey(key); i++; } return key; } #endif + } } From e0e4d9b87e69d71a39eedcb9ba321f1b3e7f31f1 Mon Sep 17 00:00:00 2001 From: krajek Date: Sat, 22 May 2021 10:39:57 +0200 Subject: [PATCH 09/11] Add one more test --- .../ReflectionBasedDestructurerTest.cs | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/Tests/Serilog.Exceptions.Test/Destructurers/ReflectionBasedDestructurerTest.cs b/Tests/Serilog.Exceptions.Test/Destructurers/ReflectionBasedDestructurerTest.cs index 1c4b0e09..f000a1b0 100644 --- a/Tests/Serilog.Exceptions.Test/Destructurers/ReflectionBasedDestructurerTest.cs +++ b/Tests/Serilog.Exceptions.Test/Destructurers/ReflectionBasedDestructurerTest.cs @@ -397,6 +397,25 @@ public void CanDestructureObjectWithRedefinedProperty() 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?)properties[nameof(RecursiveException.Node)]; + Assert.Equal(123, parent?[nameof(RecursiveNode.Name)]); + } + private static void Test_ResultOfReflectionDestructurerShouldBeEquivalentToCustomOne( Exception exception, IExceptionDestructurer customDestructurer) @@ -568,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; } From 4d9283a2f1a93a3f59aefef688804d90c5799997 Mon Sep 17 00:00:00 2001 From: krajek Date: Mon, 24 May 2021 12:22:33 +0200 Subject: [PATCH 10/11] Fix some formatting warnings --- Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs b/Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs index 7353c7ad..9a1608ae 100644 --- a/Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs +++ b/Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs @@ -16,7 +16,7 @@ internal class ExceptionPropertiesBag : IExceptionPropertiesBag private const int AcceptableNumberOfSameNameProperties = 5; private readonly Exception exception; private readonly IExceptionPropertyFilter? filter; - private readonly Dictionary properties = new (); + private readonly Dictionary properties = new(); /// /// We keep a note on whether the results were collected to be sure that after that there are no changes. This @@ -71,7 +71,6 @@ public void AddProperty(string key, object? value) private static string GetReplacementKey(string key) => key + "$"; - /// /// We want to be as robust as possible /// so even in case of multiple properties with the same name @@ -92,9 +91,7 @@ private void AddPairToProperties(string key, object? value) this.properties.Add(key, value); #endif } - #if !NET5_0 - private string MakeSureKeyIsUnique(string key) { var i = 0; From d867b86c2451674b5516cddbced4f35e633e01fe Mon Sep 17 00:00:00 2001 From: krajek Date: Mon, 24 May 2021 12:30:47 +0200 Subject: [PATCH 11/11] Fix one last formatting issue --- Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs b/Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs index 9a1608ae..caa5c0ae 100644 --- a/Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs +++ b/Source/Serilog.Exceptions/Core/ExceptionPropertiesBag.cs @@ -91,6 +91,7 @@ private void AddPairToProperties(string key, object? value) this.properties.Add(key, value); #endif } + #if !NET5_0 private string MakeSureKeyIsUnique(string key) {