From e3910e9f28f9302a6eb8f759a4fe365a5ed714e0 Mon Sep 17 00:00:00 2001 From: Nicholas Blumhardt Date: Wed, 31 May 2017 09:46:51 +1000 Subject: [PATCH 1/6] ValueTuple to sequence value conversion --- .../Parameters/PropertyValueConverter.cs | 29 +++++++++- src/Serilog/Serilog.csproj | 56 ++++++++++--------- .../Parameters/PropertyValueConverterTests.cs | 24 +++++++- 3 files changed, 79 insertions(+), 30 deletions(-) diff --git a/src/Serilog/Parameters/PropertyValueConverter.cs b/src/Serilog/Parameters/PropertyValueConverter.cs index 712af6f18..3c9333332 100755 --- a/src/Serilog/Parameters/PropertyValueConverter.cs +++ b/src/Serilog/Parameters/PropertyValueConverter.cs @@ -74,7 +74,7 @@ public PropertyValueConverter( new SimpleScalarConversionPolicy(BuiltInScalarTypes.Concat(additionalScalarTypes)), new NullableScalarConversionPolicy(), new EnumScalarConversionPolicy(), - new ByteArrayScalarConversionPolicy(), + new ByteArrayScalarConversionPolicy() }; _destructuringPolicies = additionalDestructuringPolicies @@ -190,6 +190,33 @@ LogEventPropertyValue CreatePropertyValue(object value, Destructuring destructur enumerable.Cast().Take(_maximumCollectionCount).Select(o => limiter.CreatePropertyValue(o, destructuring))); } + if (value is IStructuralEquatable) + { + var type = value.GetType(); + if (type.IsConstructedGenericType) + { + var definition = type.GetGenericTypeDefinition(); + if (definition == typeof(ValueTuple<>) || definition == typeof(ValueTuple<,>) || + definition == typeof(ValueTuple<,,>) || definition == typeof(ValueTuple<,,,>) || + definition == typeof(ValueTuple<,,,,>) || definition == typeof(ValueTuple<,,,,,>) || + definition == typeof(ValueTuple<,,,,,,>)) // Ignore the 8+ value case for now. + { + var elements = new List(); + foreach (var field in type.GetTypeInfo().DeclaredFields) + { + if (field.IsPublic && !field.IsStatic) + { + var fieldValue = field.GetValue(value); + var propertyValue = limiter.CreatePropertyValue(fieldValue, destructuring); + elements.Add(propertyValue); + } + } + + return new SequenceValue(elements); + } + } + } + if (destructuring == Destructuring.Destructure) { var type = value.GetType(); diff --git a/src/Serilog/Serilog.csproj b/src/Serilog/Serilog.csproj index 698c466f6..2362223d5 100644 --- a/src/Serilog/Serilog.csproj +++ b/src/Serilog/Serilog.csproj @@ -23,25 +23,44 @@ + + - - - - - - - - - - - + + + + + + + + + + + + + + + + + + + + + + + + + + + + @@ -56,19 +75,4 @@ $(DefineConstants);ASYNCLOCAL;HASHTABLE - - - - - - - - - - - - - - - diff --git a/test/Serilog.Tests/Parameters/PropertyValueConverterTests.cs b/test/Serilog.Tests/Parameters/PropertyValueConverterTests.cs index 025f1e979..63d5a1a71 100644 --- a/test/Serilog.Tests/Parameters/PropertyValueConverterTests.cs +++ b/test/Serilog.Tests/Parameters/PropertyValueConverterTests.cs @@ -8,6 +8,8 @@ using Serilog.Parsing; using Serilog.Tests.Support; +// ReSharper disable UnusedAutoPropertyAccessor.Global, UnusedParameter.Local + namespace Serilog.Tests.Parameters { public class PropertyValueConverterTests @@ -188,7 +190,7 @@ public class BaseWithProps public class DerivedWithOverrides : BaseWithProps { - new public string PropA { get; set; } + public new string PropA { get; set; } public override string PropB { get; set; } public string PropD { get; set; } } @@ -216,7 +218,7 @@ public void NewAndInheritedPropertiesAppearOnlyOnce() class HasIndexer { - public string this[int index] { get { return "Indexer"; } } + public string this[int index] => "Indexer"; } [Fact] @@ -231,7 +233,7 @@ public void IndexerPropertiesAreIgnoredWhenDestructuring() // (reducing garbage). class HasItem { - public string Item { get { return "Item"; } } + public string Item => "Item"; } [Fact] @@ -253,6 +255,22 @@ public void CSharpAnonymousTypesAreRecognizedWhenDestructuring() var structuredValue = (StructureValue)result; Assert.Equal(null, structuredValue.TypeTag); } + + [Fact] + public void ValueTuplesAreRecognizedWhenDestructuring() + { + var o = (1, "A", new[] { "B" }); + var result = _converter.CreatePropertyValue(o); + + var sequenceValue = Assert.IsType(result); + + Assert.Equal(3, sequenceValue.Elements.Count); + Assert.Equal(new ScalarValue(1), sequenceValue.Elements[0]); + Assert.Equal(new ScalarValue("A"), sequenceValue.Elements[1]); + var nested = Assert.IsType(sequenceValue.Elements[2]); + Assert.Equal(1, nested.Elements.Count); + Assert.Equal(new ScalarValue("B"), nested.Elements[0]); + } } } From ce9b893c76785929d44632c6fb03c9c6d8321a85 Mon Sep 17 00:00:00 2001 From: Nicholas Blumhardt Date: Wed, 31 May 2017 10:26:29 +1000 Subject: [PATCH 2/6] A quick extract-method refactoring --- .../Parameters/PropertyValueConverter.cs | 100 +++++++++++------- 1 file changed, 63 insertions(+), 37 deletions(-) diff --git a/src/Serilog/Parameters/PropertyValueConverter.cs b/src/Serilog/Parameters/PropertyValueConverter.cs index 3c9333332..12ae58195 100755 --- a/src/Serilog/Parameters/PropertyValueConverter.cs +++ b/src/Serilog/Parameters/PropertyValueConverter.cs @@ -162,6 +162,29 @@ LogEventPropertyValue CreatePropertyValue(object value, Destructuring destructur } } + if (TryConvertEnumerable(value, destructuring, valueType, limiter, out var enumerableResult)) + return enumerableResult; + + if (TryConvertValueTuple(value, destructuring, valueType, limiter, out var tupleResult)) + return tupleResult; + + if (destructuring == Destructuring.Destructure) + { + var type = value.GetType(); + var typeTag = type.Name; + if (typeTag.Length <= 0 || IsCompilerGeneratedType(type)) + { + typeTag = null; + } + + return new StructureValue(GetProperties(value, limiter), typeTag); + } + + return new ScalarValue(value.ToString()); + } + + bool TryConvertEnumerable(object value, Destructuring destructuring, Type valueType, DepthLimiter limiter, out LogEventPropertyValue result) + { var enumerable = value as IEnumerable; if (enumerable != null) { @@ -179,57 +202,60 @@ LogEventPropertyValue CreatePropertyValue(object value, Destructuring destructur var keyProperty = typeInfo.GetDeclaredProperty("Key"); var valueProperty = typeInfo.GetDeclaredProperty("Value"); - return new DictionaryValue(enumerable.Cast().Take(_maximumCollectionCount) - .Select(kvp => new KeyValuePair( - (ScalarValue)limiter.CreatePropertyValue(keyProperty.GetValue(kvp), destructuring), - limiter.CreatePropertyValue(valueProperty.GetValue(kvp), destructuring))) - .Where(kvp => kvp.Key.Value != null)); + { + result = new DictionaryValue(enumerable.Cast().Take(_maximumCollectionCount) + .Select(kvp => new KeyValuePair( + (ScalarValue) limiter.CreatePropertyValue(keyProperty.GetValue(kvp), destructuring), + limiter.CreatePropertyValue(valueProperty.GetValue(kvp), destructuring))) + .Where(kvp => kvp.Key.Value != null)); + return true; + } } - return new SequenceValue( - enumerable.Cast().Take(_maximumCollectionCount).Select(o => limiter.CreatePropertyValue(o, destructuring))); + { + result = new SequenceValue( + enumerable.Cast().Take(_maximumCollectionCount).Select(o => limiter.CreatePropertyValue(o, destructuring))); + return true; + } + } + + result = null; + return false; + } + + static bool TryConvertValueTuple(object value, Destructuring destructuring, Type valueType, DepthLimiter limiter, out LogEventPropertyValue result) + { + if (!(value is IStructuralEquatable && valueType.IsConstructedGenericType)) + { + result = null; + return false; } - if (value is IStructuralEquatable) + var definition = valueType.GetGenericTypeDefinition(); + if (definition == typeof(ValueTuple<>) || definition == typeof(ValueTuple<,>) || + definition == typeof(ValueTuple<,,>) || definition == typeof(ValueTuple<,,,>) || + definition == typeof(ValueTuple<,,,,>) || definition == typeof(ValueTuple<,,,,,>) || + definition == typeof(ValueTuple<,,,,,,>)) // Ignore the 8+ value case for now. { - var type = value.GetType(); - if (type.IsConstructedGenericType) + var elements = new List(); + foreach (var field in valueType.GetTypeInfo().DeclaredFields) { - var definition = type.GetGenericTypeDefinition(); - if (definition == typeof(ValueTuple<>) || definition == typeof(ValueTuple<,>) || - definition == typeof(ValueTuple<,,>) || definition == typeof(ValueTuple<,,,>) || - definition == typeof(ValueTuple<,,,,>) || definition == typeof(ValueTuple<,,,,,>) || - definition == typeof(ValueTuple<,,,,,,>)) // Ignore the 8+ value case for now. + if (field.IsPublic && !field.IsStatic) { - var elements = new List(); - foreach (var field in type.GetTypeInfo().DeclaredFields) - { - if (field.IsPublic && !field.IsStatic) - { - var fieldValue = field.GetValue(value); - var propertyValue = limiter.CreatePropertyValue(fieldValue, destructuring); - elements.Add(propertyValue); - } - } - - return new SequenceValue(elements); + var fieldValue = field.GetValue(value); + var propertyValue = limiter.CreatePropertyValue(fieldValue, destructuring); + elements.Add(propertyValue); } } - } - if (destructuring == Destructuring.Destructure) - { - var type = value.GetType(); - var typeTag = type.Name; - if (typeTag.Length <= 0 || IsCompilerGeneratedType(type)) { - typeTag = null; + result = new SequenceValue(elements); + return true; } - - return new StructureValue(GetProperties(value, limiter), typeTag); } - return new ScalarValue(value.ToString()); + result = null; + return false; } LogEventPropertyValue Stringify(object value) From 3d80e9ee01e8f952f45daff378549b518f650940 Mon Sep 17 00:00:00 2001 From: Nicholas Blumhardt Date: Wed, 31 May 2017 10:29:36 +1000 Subject: [PATCH 3/6] Remove redundant blocks --- .../Parameters/PropertyValueConverter.cs | 33 +++++++++---------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/src/Serilog/Parameters/PropertyValueConverter.cs b/src/Serilog/Parameters/PropertyValueConverter.cs index 12ae58195..1e627ab40 100755 --- a/src/Serilog/Parameters/PropertyValueConverter.cs +++ b/src/Serilog/Parameters/PropertyValueConverter.cs @@ -202,21 +202,22 @@ bool TryConvertEnumerable(object value, Destructuring destructuring, Type valueT var keyProperty = typeInfo.GetDeclaredProperty("Key"); var valueProperty = typeInfo.GetDeclaredProperty("Value"); - { - result = new DictionaryValue(enumerable.Cast().Take(_maximumCollectionCount) - .Select(kvp => new KeyValuePair( - (ScalarValue) limiter.CreatePropertyValue(keyProperty.GetValue(kvp), destructuring), - limiter.CreatePropertyValue(valueProperty.GetValue(kvp), destructuring))) - .Where(kvp => kvp.Key.Value != null)); - return true; - } - } - - { - result = new SequenceValue( - enumerable.Cast().Take(_maximumCollectionCount).Select(o => limiter.CreatePropertyValue(o, destructuring))); + result = new DictionaryValue(enumerable + .Cast() + .Take(_maximumCollectionCount) + .Select(kvp => new KeyValuePair( + (ScalarValue) limiter.CreatePropertyValue(keyProperty.GetValue(kvp), destructuring), + limiter.CreatePropertyValue(valueProperty.GetValue(kvp), destructuring))) + .Where(kvp => kvp.Key.Value != null)); return true; } + + result = new SequenceValue(enumerable + .Cast() + .Take(_maximumCollectionCount) + .Select(o => limiter.CreatePropertyValue(o, destructuring))); + + return true; } result = null; @@ -248,10 +249,8 @@ static bool TryConvertValueTuple(object value, Destructuring destructuring, Type } } - { - result = new SequenceValue(elements); - return true; - } + result = new SequenceValue(elements); + return true; } result = null; From f3d2edd4a3efcd8cf2e6c1559e0ee2a18d912550 Mon Sep 17 00:00:00 2001 From: Nicholas Blumhardt Date: Wed, 31 May 2017 10:37:53 +1000 Subject: [PATCH 4/6] Minimal test cases --- .../Parameters/PropertyValueConverterTests.cs | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/test/Serilog.Tests/Parameters/PropertyValueConverterTests.cs b/test/Serilog.Tests/Parameters/PropertyValueConverterTests.cs index 63d5a1a71..bb73ef177 100644 --- a/test/Serilog.Tests/Parameters/PropertyValueConverterTests.cs +++ b/test/Serilog.Tests/Parameters/PropertyValueConverterTests.cs @@ -271,6 +271,39 @@ public void ValueTuplesAreRecognizedWhenDestructuring() Assert.Equal(1, nested.Elements.Count); Assert.Equal(new ScalarValue("B"), nested.Elements[0]); } + + [Fact] + public void AllTupleLengthsUpToSevenAreSupported() + { + var tuples = new object[] + { + ValueTuple.Create(1), + (1, 2), + (1, 2, 3), + (1, 2, 3, 4), + (1, 2, 3, 4, 5), + (1, 2, 3, 4, 5, 6), + (1, 2, 3, 4, 5, 6, 7) + }; + + foreach (var t in tuples) + Assert.IsType(_converter.CreatePropertyValue(t)); + } + + [Fact] + public void EightPlusValueTupleElementsAreIgnored() + { + var scalar = _converter.CreatePropertyValue((1, 2, 3, 4, 5, 6, 7, 8)); + Assert.IsType(scalar); + } + + [Fact] + public void DestructuringIsTransitivelyApplied() + { + var tuple = _converter.CreatePropertyValue(ValueTuple.Create(new {A = 1}), true); + var sequence = Assert.IsType(tuple); + Assert.IsType(sequence.Elements[0]); + } } } From 888a49866d65256bb75ed1f59c019f725c1cbac8 Mon Sep 17 00:00:00 2001 From: Nicholas Blumhardt Date: Thu, 1 Jun 2017 19:47:15 +1000 Subject: [PATCH 5/6] Review feedback - avoid package dependency on platforms without native ValueTuple --- .../Parameters/PropertyValueConverter.cs | 12 ++++- src/Serilog/Serilog.csproj | 50 +++++++++---------- test/Serilog.Tests/Serilog.Tests.csproj | 1 + 3 files changed, 35 insertions(+), 28 deletions(-) diff --git a/src/Serilog/Parameters/PropertyValueConverter.cs b/src/Serilog/Parameters/PropertyValueConverter.cs index 1e627ab40..657836dba 100755 --- a/src/Serilog/Parameters/PropertyValueConverter.cs +++ b/src/Serilog/Parameters/PropertyValueConverter.cs @@ -233,10 +233,20 @@ static bool TryConvertValueTuple(object value, Destructuring destructuring, Type } var definition = valueType.GetGenericTypeDefinition(); + + // Ignore the 8+ value case for now. +#if VALUETUPLE if (definition == typeof(ValueTuple<>) || definition == typeof(ValueTuple<,>) || definition == typeof(ValueTuple<,,>) || definition == typeof(ValueTuple<,,,>) || definition == typeof(ValueTuple<,,,,>) || definition == typeof(ValueTuple<,,,,,>) || - definition == typeof(ValueTuple<,,,,,,>)) // Ignore the 8+ value case for now. + definition == typeof(ValueTuple<,,,,,,>)) +#else + var defn = definition.FullName; + if (defn == "System.ValueTuple`1" || defn == "System.ValueTuple`2" || + defn == "System.ValueTuple`3" || defn == "System.ValueTuple`4" || + defn == "System.ValueTuple`5" || defn == "System.ValueTuple`6" || + defn == "System.ValueTuple`7") +#endif { var elements = new List(); foreach (var field in valueType.GetTypeInfo().DeclaredFields) diff --git a/src/Serilog/Serilog.csproj b/src/Serilog/Serilog.csproj index 2362223d5..ad479c83b 100644 --- a/src/Serilog/Serilog.csproj +++ b/src/Serilog/Serilog.csproj @@ -23,44 +23,40 @@ - - - - - - - - - - - - - - + + + + + + + + + + + - - - - - - - - - - - - - + + + + + + + + + + + + diff --git a/test/Serilog.Tests/Serilog.Tests.csproj b/test/Serilog.Tests/Serilog.Tests.csproj index dfa074061..389b2f5f4 100644 --- a/test/Serilog.Tests/Serilog.Tests.csproj +++ b/test/Serilog.Tests/Serilog.Tests.csproj @@ -18,6 +18,7 @@ + From 8c33f2357fcffb3f89fbac19b057316a56bd6754 Mon Sep 17 00:00:00 2001 From: Nicholas Blumhardt Date: Sat, 3 Jun 2017 11:13:40 +1000 Subject: [PATCH 6/6] Review feedback/test case names. --- .../Serilog.Tests/Parameters/PropertyValueConverterTests.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/Serilog.Tests/Parameters/PropertyValueConverterTests.cs b/test/Serilog.Tests/Parameters/PropertyValueConverterTests.cs index bb73ef177..3728fb0a2 100644 --- a/test/Serilog.Tests/Parameters/PropertyValueConverterTests.cs +++ b/test/Serilog.Tests/Parameters/PropertyValueConverterTests.cs @@ -273,7 +273,7 @@ public void ValueTuplesAreRecognizedWhenDestructuring() } [Fact] - public void AllTupleLengthsUpToSevenAreSupported() + public void AllTupleLengthsUpToSevenAreSupportedForCapturing() { var tuples = new object[] { @@ -291,14 +291,14 @@ public void AllTupleLengthsUpToSevenAreSupported() } [Fact] - public void EightPlusValueTupleElementsAreIgnored() + public void EightPlusValueTupleElementsAreIgnoredByCapturing() { var scalar = _converter.CreatePropertyValue((1, 2, 3, 4, 5, 6, 7, 8)); Assert.IsType(scalar); } [Fact] - public void DestructuringIsTransitivelyApplied() + public void ValueTupleDestructuringIsTransitivelyApplied() { var tuple = _converter.CreatePropertyValue(ValueTuple.Create(new {A = 1}), true); var sequence = Assert.IsType(tuple);