From 670a5ff179ddfbccb47952140a3e80a5ac0cb86c Mon Sep 17 00:00:00 2001 From: Thibaud DESODT Date: Mon, 23 Oct 2017 07:21:39 +0200 Subject: [PATCH 1/6] Pass a IReadOnlyDictionary to KeyValuePairSettings Refactored KeyValuePairSettings to accept a IReadOnlyDictionary instead of IEnumerable> in order to reveal the fact that key-value settings are not lazily consumed. Added tests on edge cases : duplicate keys, irrelevant settings. --- .../LoggerSettingsConfiguration.cs | 6 +++- .../KeyValuePairs/KeyValuePairSettings.cs | 26 +++++++++------ .../Settings/KeyValuePairSettingsTests.cs | 33 +++++++++++++++++-- 3 files changed, 52 insertions(+), 13 deletions(-) diff --git a/src/Serilog/Configuration/LoggerSettingsConfiguration.cs b/src/Serilog/Configuration/LoggerSettingsConfiguration.cs index afd1499d3..cfde0fa1e 100644 --- a/src/Serilog/Configuration/LoggerSettingsConfiguration.cs +++ b/src/Serilog/Configuration/LoggerSettingsConfiguration.cs @@ -14,6 +14,8 @@ using System; using System.Collections.Generic; +using System.Collections.ObjectModel; +using System.Linq; using Serilog.Settings.KeyValuePairs; namespace Serilog.Configuration @@ -52,7 +54,9 @@ public LoggerConfiguration Settings(ILoggerSettings settings) public LoggerConfiguration KeyValuePairs(IEnumerable> settings) { if (settings == null) throw new ArgumentNullException(nameof(settings)); - return Settings(new KeyValuePairSettings(settings)); + + var settingsDictionary = new ReadOnlyDictionary(settings.ToDictionary(x => x.Key, x => x.Value)); + return Settings(new KeyValuePairSettings(settingsDictionary)); } } } diff --git a/src/Serilog/Settings/KeyValuePairs/KeyValuePairSettings.cs b/src/Serilog/Settings/KeyValuePairs/KeyValuePairSettings.cs index 69c619efd..51bbcb1d1 100644 --- a/src/Serilog/Settings/KeyValuePairs/KeyValuePairSettings.cs +++ b/src/Serilog/Settings/KeyValuePairs/KeyValuePairSettings.cs @@ -73,21 +73,18 @@ class KeyValuePairSettings : ILoggerSettings [typeof(LoggerFilterConfiguration)] = lc => lc.Filter }; - readonly Dictionary _settings; + readonly IReadOnlyDictionary _settings; - public KeyValuePairSettings(IEnumerable> settings) + public KeyValuePairSettings(IReadOnlyDictionary settings) { - if (settings == null) throw new ArgumentNullException(nameof(settings)); - _settings = settings.ToDictionary(s => s.Key, s => s.Value); + _settings = settings ?? throw new ArgumentNullException(nameof(settings)); } public void Configure(LoggerConfiguration loggerConfiguration) { if (loggerConfiguration == null) throw new ArgumentNullException(nameof(loggerConfiguration)); - - var directives = _settings.Keys - .Where(k => _supportedDirectives.Any(k.StartsWith)) - .ToDictionary(k => k, k => _settings[k]); + + var directives = ExtractDirectives(_settings); var declaredLevelSwitches = ParseNamedLevelSwitchDeclarationDirectives(directives); @@ -164,12 +161,21 @@ where matchCallables.IsMatch(wt.Key) } } + internal static IReadOnlyDictionary ExtractDirectives(IReadOnlyDictionary settings) + { + var directives = settings + .Where(kvp => _supportedDirectives.Any(kvp.Key.StartsWith)) + .ToDictionary(kvp => kvp.Key, kvp => kvp.Value); + + return new ReadOnlyDictionary(directives); + } + internal static bool IsValidSwitchName(string input) { return Regex.IsMatch(input, LevelSwitchNameRegex); } - static IReadOnlyDictionary ParseNamedLevelSwitchDeclarationDirectives(Dictionary directives) + static IReadOnlyDictionary ParseNamedLevelSwitchDeclarationDirectives(IReadOnlyDictionary directives) { var matchLevelSwitchDeclarations = new Regex(LevelSwitchDeclarationDirectiveRegex); @@ -255,7 +261,7 @@ internal static MethodInfo SelectConfigurationMethod(IEnumerable can .FirstOrDefault(); } - internal static IEnumerable LoadConfigurationAssemblies(Dictionary directives) + internal static IEnumerable LoadConfigurationAssemblies(IReadOnlyDictionary directives) { var configurationAssemblies = new List { typeof(ILogger).GetTypeInfo().Assembly }; diff --git a/test/Serilog.Tests/Settings/KeyValuePairSettingsTests.cs b/test/Serilog.Tests/Settings/KeyValuePairSettingsTests.cs index 9b7e7f9fd..405a13a29 100644 --- a/test/Serilog.Tests/Settings/KeyValuePairSettingsTests.cs +++ b/test/Serilog.Tests/Settings/KeyValuePairSettingsTests.cs @@ -15,6 +15,35 @@ namespace Serilog.Tests.Settings { public class KeyValuePairSettingsTests { + [Fact] + public void DuplicateKeysCauseArgumentException() + { + Action action = () => new LoggerConfiguration() + .ReadFrom.KeyValuePairs(new List> + { + new KeyValuePair("setting", "Value"), + new KeyValuePair("setting", "SameSettingOtherValue"), + }); + + var ex = Assert.ThrowsAny(action); + Assert.NotNull(ex); + Assert.Contains("An item with the same key has already been added.", ex.Message); + } + + [Fact] + public void IrrelevantKeysAreIgnored() + { + var irrelevantSettings = new Dictionary() + { + ["whatever:foo:bar"] = "willBeIgnored", + ["irrelevant"] = "willBeIgnored", + }; + + var directives = KeyValuePairSettings.ExtractDirectives(irrelevantSettings); + + Assert.False(directives.Any()); + } + [Fact] public void FindsConfigurationAssemblies() { @@ -181,7 +210,7 @@ public void LoggingLevelSwitchWithInvalidNameThrowsFormatException() ["level-switch:switchNameNotStartingWithDollar"] = "Warning", }; - var ex = Assert.Throws(() => new LoggerConfiguration() + var ex = Assert.Throws(() => new LoggerConfiguration() .ReadFrom.KeyValuePairs(settings)); Assert.Contains("\"switchNameNotStartingWithDollar\"", ex.Message); @@ -301,7 +330,7 @@ public void LoggingLevelSwitchCanBeUsedForMinimumLevelOverrides() .CreateLogger(); var systemLogger = log.ForContext(Constants.SourceContextPropertyName, "System.Bar"); - + log.Write(Some.InformationEvent()); Assert.False(evt is null, "Minimul level is Debug. It should log Information messages"); From 1c3449c19d7223cdfe085ebaacc99d299124d7da Mon Sep 17 00:00:00 2001 From: Thibaud DESODT Date: Tue, 24 Oct 2017 07:01:41 +0200 Subject: [PATCH 2/6] Review: No need to create a ReadOnlyDictionary after .ToDictionary() --- src/Serilog/Configuration/LoggerSettingsConfiguration.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Serilog/Configuration/LoggerSettingsConfiguration.cs b/src/Serilog/Configuration/LoggerSettingsConfiguration.cs index cfde0fa1e..3b40ca44d 100644 --- a/src/Serilog/Configuration/LoggerSettingsConfiguration.cs +++ b/src/Serilog/Configuration/LoggerSettingsConfiguration.cs @@ -55,8 +55,7 @@ public LoggerConfiguration KeyValuePairs(IEnumerable(settings.ToDictionary(x => x.Key, x => x.Value)); - return Settings(new KeyValuePairSettings(settingsDictionary)); + return Settings(new KeyValuePairSettings(settings.ToDictionary(x => x.Key, x => x.Value))); } } } From f7323243d273b847e8d4a93bf83b808d0a9fc2d5 Mon Sep 17 00:00:00 2001 From: Thibaud DESODT Date: Tue, 24 Oct 2017 07:07:14 +0200 Subject: [PATCH 3/6] Review : remove unnecessary method ExtractDirectives ... and useless test --- .../KeyValuePairs/KeyValuePairSettings.cs | 15 ++++----------- .../Settings/KeyValuePairSettingsTests.cs | 16 +--------------- 2 files changed, 5 insertions(+), 26 deletions(-) diff --git a/src/Serilog/Settings/KeyValuePairs/KeyValuePairSettings.cs b/src/Serilog/Settings/KeyValuePairs/KeyValuePairSettings.cs index 51bbcb1d1..3870b5dea 100644 --- a/src/Serilog/Settings/KeyValuePairs/KeyValuePairSettings.cs +++ b/src/Serilog/Settings/KeyValuePairs/KeyValuePairSettings.cs @@ -83,8 +83,10 @@ public KeyValuePairSettings(IReadOnlyDictionary settings) public void Configure(LoggerConfiguration loggerConfiguration) { if (loggerConfiguration == null) throw new ArgumentNullException(nameof(loggerConfiguration)); - - var directives = ExtractDirectives(_settings); + + var directives = _settings + .Where(kvp => _supportedDirectives.Any(kvp.Key.StartsWith)) + .ToDictionary(kvp => kvp.Key, kvp => kvp.Value); var declaredLevelSwitches = ParseNamedLevelSwitchDeclarationDirectives(directives); @@ -161,15 +163,6 @@ where matchCallables.IsMatch(wt.Key) } } - internal static IReadOnlyDictionary ExtractDirectives(IReadOnlyDictionary settings) - { - var directives = settings - .Where(kvp => _supportedDirectives.Any(kvp.Key.StartsWith)) - .ToDictionary(kvp => kvp.Key, kvp => kvp.Value); - - return new ReadOnlyDictionary(directives); - } - internal static bool IsValidSwitchName(string input) { return Regex.IsMatch(input, LevelSwitchNameRegex); diff --git a/test/Serilog.Tests/Settings/KeyValuePairSettingsTests.cs b/test/Serilog.Tests/Settings/KeyValuePairSettingsTests.cs index 405a13a29..a16bae19c 100644 --- a/test/Serilog.Tests/Settings/KeyValuePairSettingsTests.cs +++ b/test/Serilog.Tests/Settings/KeyValuePairSettingsTests.cs @@ -28,21 +28,7 @@ public void DuplicateKeysCauseArgumentException() var ex = Assert.ThrowsAny(action); Assert.NotNull(ex); Assert.Contains("An item with the same key has already been added.", ex.Message); - } - - [Fact] - public void IrrelevantKeysAreIgnored() - { - var irrelevantSettings = new Dictionary() - { - ["whatever:foo:bar"] = "willBeIgnored", - ["irrelevant"] = "willBeIgnored", - }; - - var directives = KeyValuePairSettings.ExtractDirectives(irrelevantSettings); - - Assert.False(directives.Any()); - } + } [Fact] public void FindsConfigurationAssemblies() From 3e224a0d3d6529468e78e18147b9a236bea5b96c Mon Sep 17 00:00:00 2001 From: Thibaud DESODT Date: Tue, 24 Oct 2017 07:20:39 +0200 Subject: [PATCH 4/6] Remove "using System.Collections.ObjectModel" --- src/Serilog/Configuration/LoggerSettingsConfiguration.cs | 1 - src/Serilog/Settings/KeyValuePairs/KeyValuePairSettings.cs | 3 +-- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/src/Serilog/Configuration/LoggerSettingsConfiguration.cs b/src/Serilog/Configuration/LoggerSettingsConfiguration.cs index 3b40ca44d..ce279f0eb 100644 --- a/src/Serilog/Configuration/LoggerSettingsConfiguration.cs +++ b/src/Serilog/Configuration/LoggerSettingsConfiguration.cs @@ -14,7 +14,6 @@ using System; using System.Collections.Generic; -using System.Collections.ObjectModel; using System.Linq; using Serilog.Settings.KeyValuePairs; diff --git a/src/Serilog/Settings/KeyValuePairs/KeyValuePairSettings.cs b/src/Serilog/Settings/KeyValuePairs/KeyValuePairSettings.cs index 3870b5dea..96d8a8ae5 100644 --- a/src/Serilog/Settings/KeyValuePairs/KeyValuePairSettings.cs +++ b/src/Serilog/Settings/KeyValuePairs/KeyValuePairSettings.cs @@ -14,7 +14,6 @@ using System; using System.Collections.Generic; -using System.Collections.ObjectModel; using System.Linq; using System.Reflection; using System.Text.RegularExpressions; @@ -203,7 +202,7 @@ where matchLevelSwitchDeclarations.IsMatch(wt.Key) } namedSwitches.Add(switchName, newSwitch); } - return new ReadOnlyDictionary(namedSwitches); + return namedSwitches; } static LoggingLevelSwitch LookUpSwitchByName(string switchName, IReadOnlyDictionary declaredLevelSwitches) From a80ea3e4d1cabdb178dca7ab09a005aaf3f8a09b Mon Sep 17 00:00:00 2001 From: Thibaud DESODT Date: Tue, 7 Nov 2017 07:02:03 +0100 Subject: [PATCH 5/6] Remove ReSharper false positive --- test/Serilog.Tests/Settings/KeyValuePairSettingsTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/Serilog.Tests/Settings/KeyValuePairSettingsTests.cs b/test/Serilog.Tests/Settings/KeyValuePairSettingsTests.cs index a16bae19c..d20f57889 100644 --- a/test/Serilog.Tests/Settings/KeyValuePairSettingsTests.cs +++ b/test/Serilog.Tests/Settings/KeyValuePairSettingsTests.cs @@ -321,14 +321,14 @@ public void LoggingLevelSwitchCanBeUsedForMinimumLevelOverrides() Assert.False(evt is null, "Minimul level is Debug. It should log Information messages"); evt = null; - + // ReSharper disable HeuristicUnreachableCode systemLogger.Write(Some.InformationEvent()); Assert.True(evt is null, "LoggingLevelSwitch initial level was Warning for logger System.*. It should not log Information messages for SourceContext System.Bar"); systemLogger.Write(Some.WarningEvent()); Assert.False(evt is null, "LoggingLevelSwitch initial level was Warning for logger System.*. It should log Warning messages for SourceContext System.Bar"); - // ReSharper disable HeuristicUnreachableCode + evt = null; var controlSwitch = DummyWithLevelSwitchSink.ControlLevelSwitch; From 64e45ab19997ef0b543095835bcde613f5153f0f Mon Sep 17 00:00:00 2001 From: Thibaud DESODT Date: Tue, 7 Nov 2017 07:16:48 +0100 Subject: [PATCH 6/6] Make ReadFrom.KeyValuePairs not throw when duplicate keys Instead, take the last value of each key, to remove work from future Serilog.Settings.Combined --- .../LoggerSettingsConfiguration.cs | 12 ++++++++- .../Settings/KeyValuePairSettingsTests.cs | 25 +++++++++++-------- 2 files changed, 26 insertions(+), 11 deletions(-) diff --git a/src/Serilog/Configuration/LoggerSettingsConfiguration.cs b/src/Serilog/Configuration/LoggerSettingsConfiguration.cs index ce279f0eb..6acbf70ac 100644 --- a/src/Serilog/Configuration/LoggerSettingsConfiguration.cs +++ b/src/Serilog/Configuration/LoggerSettingsConfiguration.cs @@ -50,11 +50,21 @@ public LoggerConfiguration Settings(ILoggerSettings settings) /// /// A list of key-value pairs describing logger settings. /// Configuration object allowing method chaining. + /// In case of duplicate keys, the last value for the key is kept and the previous ones are ignored. public LoggerConfiguration KeyValuePairs(IEnumerable> settings) { if (settings == null) throw new ArgumentNullException(nameof(settings)); + var uniqueSettings = new Dictionary(); + foreach (var kvp in settings) + { + uniqueSettings[kvp.Key] = kvp.Value; + } + return KeyValuePairs(uniqueSettings); + } - return Settings(new KeyValuePairSettings(settings.ToDictionary(x => x.Key, x => x.Value))); + LoggerConfiguration KeyValuePairs(IReadOnlyDictionary settings) + { + return Settings(new KeyValuePairSettings(settings)); } } } diff --git a/test/Serilog.Tests/Settings/KeyValuePairSettingsTests.cs b/test/Serilog.Tests/Settings/KeyValuePairSettingsTests.cs index d20f57889..275f82b82 100644 --- a/test/Serilog.Tests/Settings/KeyValuePairSettingsTests.cs +++ b/test/Serilog.Tests/Settings/KeyValuePairSettingsTests.cs @@ -16,19 +16,24 @@ namespace Serilog.Tests.Settings public class KeyValuePairSettingsTests { [Fact] - public void DuplicateKeysCauseArgumentException() + public void LastValueIsTakenWhenKeysAreDuplicate() { - Action action = () => new LoggerConfiguration() + LogEvent evt = null; + var log = new LoggerConfiguration() .ReadFrom.KeyValuePairs(new List> { - new KeyValuePair("setting", "Value"), - new KeyValuePair("setting", "SameSettingOtherValue"), - }); + new KeyValuePair("enrich:with-property:App", "InitialValue"), + new KeyValuePair("enrich:with-property:App", "OverridenValue"), + new KeyValuePair("enrich:with-property:App", "FinalValue") + }) + .WriteTo.Sink(new DelegatingSink(e => evt = e)) + .CreateLogger(); - var ex = Assert.ThrowsAny(action); - Assert.NotNull(ex); - Assert.Contains("An item with the same key has already been added.", ex.Message); - } + log.Information("Has a test property"); + + Assert.NotNull(evt); + Assert.Equal("FinalValue", evt.Properties["App"].LiteralValue()); + } [Fact] public void FindsConfigurationAssemblies() @@ -328,7 +333,7 @@ public void LoggingLevelSwitchCanBeUsedForMinimumLevelOverrides() systemLogger.Write(Some.WarningEvent()); Assert.False(evt is null, "LoggingLevelSwitch initial level was Warning for logger System.*. It should log Warning messages for SourceContext System.Bar"); - + evt = null; var controlSwitch = DummyWithLevelSwitchSink.ControlLevelSwitch;