Use consistent binding logic for nested properties in object argument values

Author: nblumhardt

src/Serilog.Settings.Configuration/Settings/Configuration/ConfigurationArgumentValue.cs

@@ -0,0 +1,34 @@
+using System.Reflection;
+using Microsoft.Extensions.Configuration;
+
+namespace Serilog.Settings.Configuration;
+
+abstract class ConfigurationArgumentValue
+{
+    public abstract object? ConvertTo(Type toType, ResolutionContext resolutionContext);
+
+    public static ConfigurationArgumentValue FromSection(IConfigurationSection argumentSection, IReadOnlyCollection<Assembly> configurationAssemblies)
+    {
+        ConfigurationArgumentValue argumentValue;
+
+        // Reject configurations where an element has both scalar and complex
+        // values as a result of reading multiple configuration sources.
+        if (argumentSection.Value != null && argumentSection.GetChildren().Any())
+            throw new InvalidOperationException(
+                $"The value for the argument '{argumentSection.Path}' is assigned different value " +
+                "types in more than one configuration source. Ensure all configurations consistently " +
+                "use either a scalar (int, string, boolean) or a complex (array, section, list, " +
+                "POCO, etc.) type for this argument value.");
+
+        if (argumentSection.Value != null)
+        {
+            argumentValue = new StringArgumentValue(argumentSection.Value);
+        }
+        else
+        {
+            argumentValue = new ObjectArgumentValue(argumentSection, configurationAssemblies);
+        }
+
+        return argumentValue;
+    }
+}

src/Serilog.Settings.Configuration/Settings/Configuration/ConfigurationReader.cs

@@ -303,14 +303,14 @@ void ApplyEnrichment(LoggerConfiguration loggerConfiguration)
         }
     }
 
-    internal ILookup<string, Dictionary<string, IConfigurationArgumentValue>> GetMethodCalls(IConfiguration directive)
+    internal ILookup<string, Dictionary<string, ConfigurationArgumentValue>> GetMethodCalls(IConfiguration directive)
     {
         var children = directive.GetChildren().ToList();
 
         var result =
             (from child in children
              where child.Value != null // Plain string
-             select new { Name = child.Value, Args = new Dictionary<string, IConfigurationArgumentValue>() })
+             select new { Name = child.Value, Args = new Dictionary<string, ConfigurationArgumentValue>() })
                  .Concat(
             (from child in children
              where child.Value == null
@@ -319,7 +319,7 @@ internal ILookup<string, Dictionary<string, IConfigurationArgumentValue>> GetMet
                              select new
                              {
                                  Name = argument.Key,
-                                 Value = GetArgumentValue(argument, _configurationAssemblies)
+                                 Value = ConfigurationArgumentValue.FromSection(argument, _configurationAssemblies)
                              }).ToDictionary(p => p.Name, p => p.Value)
              select new { Name = name, Args = callArgs }))
                  .ToLookup(p => p.Name, p => p.Args);
@@ -336,31 +336,6 @@ static string GetSectionName(IConfigurationSection s)
         }
     }
 
-    internal static IConfigurationArgumentValue GetArgumentValue(IConfigurationSection argumentSection, IReadOnlyCollection<Assembly> configurationAssemblies)
-    {
-        IConfigurationArgumentValue argumentValue;
-
-        // Reject configurations where an element has both scalar and complex
-        // values as a result of reading multiple configuration sources.
-        if (argumentSection.Value != null && argumentSection.GetChildren().Any())
-            throw new InvalidOperationException(
-                $"The value for the argument '{argumentSection.Path}' is assigned different value " +
-                "types in more than one configuration source. Ensure all configurations consistently " +
-                "use either a scalar (int, string, boolean) or a complex (array, section, list, " +
-                "POCO, etc.) type for this argument value.");
-
-        if (argumentSection.Value != null)
-        {
-            argumentValue = new StringArgumentValue(argumentSection.Value);
-        }
-        else
-        {
-            argumentValue = new ObjectArgumentValue(argumentSection, configurationAssemblies);
-        }
-
-        return argumentValue;
-    }
-
     static IReadOnlyCollection<Assembly> LoadConfigurationAssemblies(IConfiguration section, AssemblyFinder assemblyFinder)
     {
         var serilogAssembly = typeof(ILogger).Assembly;
@@ -404,7 +379,7 @@ This is most likely because the application is published as single-file.
         return assemblies;
     }
 
-    void CallConfigurationMethods(ILookup<string, Dictionary<string, IConfigurationArgumentValue>> methods, IReadOnlyCollection<MethodInfo> configurationMethods, object receiver)
+    void CallConfigurationMethods(ILookup<string, Dictionary<string, ConfigurationArgumentValue>> methods, IReadOnlyCollection<MethodInfo> configurationMethods, object receiver)
     {
         foreach (var method in methods.SelectMany(g => g.Select(x => new { g.Key, Value = x })))
         {

src/Serilog.Settings.Configuration/Settings/Configuration/IConfigurationArgumentValue.cs

@@ -4,10 +4,11 @@
 using Microsoft.Extensions.Configuration;
 
 using Serilog.Configuration;
+using Serilog.Debugging;
 
 namespace Serilog.Settings.Configuration;
 
-class ObjectArgumentValue : IConfigurationArgumentValue
+class ObjectArgumentValue : ConfigurationArgumentValue
 {
     readonly IConfigurationSection _section;
     readonly IReadOnlyCollection<Assembly> _configurationAssemblies;
@@ -20,15 +21,15 @@ public ObjectArgumentValue(IConfigurationSection section, IReadOnlyCollection<As
         _configurationAssemblies = configurationAssemblies ?? throw new ArgumentNullException(nameof(configurationAssemblies));
     }
 
-    public object? ConvertTo(Type toType, ResolutionContext resolutionContext)
+    public override object? ConvertTo(Type toType, ResolutionContext resolutionContext)
     {
         // return the entire section for internal processing
         if (toType == typeof(IConfigurationSection)) return _section;
 
         // process a nested configuration to populate an Action<> logger/sink config parameter?
         var typeInfo = toType.GetTypeInfo();
         if (typeInfo.IsGenericType &&
-            typeInfo.GetGenericTypeDefinition() is Type genericType && genericType == typeof(Action<>))
+            typeInfo.GetGenericTypeDefinition() is {} genericType && genericType == typeof(Action<>))
         {
             var configType = typeInfo.GenericTypeArguments[0];
             IConfigurationReader configReader = new ConfigurationReader(_section, _configurationAssemblies, resolutionContext);
@@ -64,9 +65,9 @@ object CreateArray()
             var arrayElementType = toType.GetElementType()!;
             var configurationElements = _section.GetChildren().ToArray();
             var array = Array.CreateInstance(arrayElementType, configurationElements.Length);
-            for (int i = 0; i < configurationElements.Length; ++i)
+            for (var i = 0; i < configurationElements.Length; ++i)
             {
-                var argumentValue = ConfigurationReader.GetArgumentValue(configurationElements[i], _configurationAssemblies);
+                var argumentValue = FromSection(configurationElements[i], _configurationAssemblies);
                 var value = argumentValue.ConvertTo(arrayElementType, resolutionContext);
                 array.SetValue(value, i);
             }
@@ -84,29 +85,28 @@ bool TryCreateContainer([NotNullWhen(true)] out object? result)
 
                 foreach (var section in _section.GetChildren())
                 {
-                    var argumentValue = ConfigurationReader.GetArgumentValue(section, _configurationAssemblies);
+                    var argumentValue = FromSection(section, _configurationAssemblies);
                     var key = new StringArgumentValue(section.Key).ConvertTo(keyType, resolutionContext);
                     var value = argumentValue.ConvertTo(valueType, resolutionContext);
-                    addMethod.Invoke(result, new[] { key, value });
+                    addMethod.Invoke(result, [key, value]);
                 }
                 return true;
             }
-            else if (IsConstructableContainer(toType, elementType, out concreteType, out addMethod))
+
+            if (IsConstructableContainer(toType, elementType, out concreteType, out addMethod))
             {
                 result = Activator.CreateInstance(concreteType) ?? throw new InvalidOperationException($"Activator.CreateInstance returned null for {concreteType}");
 
                 foreach (var section in _section.GetChildren())
                 {
-                    var argumentValue = ConfigurationReader.GetArgumentValue(section, _configurationAssemblies);
+                    var argumentValue = FromSection(section, _configurationAssemblies);
                     var value = argumentValue.ConvertTo(elementType, resolutionContext);
                     addMethod.Invoke(result, new[] { value });
                 }
                 return true;
             }
-            else
-            {
-                return false;
-            }
+
+            return false;
         }
     }
 
@@ -153,58 +153,84 @@ bool TryCallCtorImplicit(
 
     bool TryCallCtor(Type type, Dictionary<string, IConfigurationSection> suppliedArguments, ResolutionContext resolutionContext, [NotNullWhen(true)] out object? value)
     {
-        value = null;
+        var binding = type.GetConstructors()
+            .Select(ci =>
+            {
+                var args = new List<object?>();
+                var matches = 0;
+                var stringMatches = 0;
+                var suppliedNames = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
+                foreach (var p in ci.GetParameters())
+                {
+                    if (suppliedArguments.TryGetValue(p.Name ?? "", out var argValue))
+                    {
+                        args.Add(argValue);
+                        matches += 1;
+
+                        if (p.ParameterType == typeof(string))
+                        {
+                            stringMatches += 1;
+                        }
+
+                        if (p.Name != null)
+                        {
+                            suppliedNames.Add(p.Name);
+                        }
+                    }
+                    else
+                    {
+                        if (p.HasDefaultValue)
+                        {
+                            args.Add(p.DefaultValue);
+                        }
+                        else
+                        {
+                            return new { ci, args, isCallable = false, matches, stringMatches, suppliedNames };
+                        }
+                    }
+                }
 
-        if (suppliedArguments.Count == 0 &&
-            type.GetConstructor(Type.EmptyTypes) is ConstructorInfo parameterlessCtor)
+                return new { ci, args, isCallable = true, matches, stringMatches, suppliedNames };
+            })
+            .Where(binding => binding.isCallable)
+            .OrderByDescending(binding => binding.matches)
+            .ThenByDescending(binding => binding.stringMatches)
+            .ThenBy(binding => binding.args.Count)
+            .FirstOrDefault();
+
+        if (binding == null)
         {
-            value = parameterlessCtor.Invoke([]);
-            return true;
+            value = null;
+            return false;
         }
 
-        var ctor =
-            (from c in type.GetConstructors()
-             from p in c.GetParameters()
-             let argumentBindResult = suppliedArguments.TryGetValue(p.Name ?? "", out var argValue) switch
-             {
-                 true => new { success = true, hasMatch = true, value = (object?)argValue },
-                 false => p.HasDefaultValue switch
-                 {
-                     true  => new { success = true,  hasMatch = false, value = (object?)p.DefaultValue },
-                     false => new { success = false, hasMatch = false, value = (object?)null },
-                 },
-             }
-             group new { argumentBindResult, p.ParameterType } by c into gr
-             where gr.All(z => z.argumentBindResult.success)
-             let matchedArgs = gr.Where(z => z.argumentBindResult.hasMatch).ToList()
-             orderby matchedArgs.Count descending,
-                     matchedArgs.Count(p => p.ParameterType == typeof(string)) descending
-             select new
-             {
-                 ConstructorInfo = gr.Key,
-                 ArgumentValues = gr.Select(z => new { Value = z.argumentBindResult.value, Type = z.ParameterType })
-                                    .ToList()
-             }).FirstOrDefault();
-
-        if (ctor is null)
+        for (var i = 0; i < binding.ci.GetParameters().Length; ++i)
         {
-            return false;
+            if (binding.args[i] is IConfigurationSection section)
+            {
+                var argumentValue = FromSection(section, _configurationAssemblies);
+                binding.args[i] = argumentValue.ConvertTo(binding.ci.GetParameters()[i].ParameterType, resolutionContext);
+            }
         }
 
-        var ctorArguments = new object?[ctor.ArgumentValues.Count];
-        for (var i = 0; i < ctor.ArgumentValues.Count; i++)
+        value = binding.ci.Invoke(binding.args.ToArray());
+
+        foreach (var pi in type.GetProperties(BindingFlags.Instance | BindingFlags.Public))
         {
-            var argument = ctor.ArgumentValues[i];
-            var valueValue = argument.Value;
-            if (valueValue is IConfigurationSection s)
+            if (!binding.suppliedNames.Contains(pi.Name) && suppliedArguments.TryGetValue(pi.Name, out var section) && pi.CanWrite)
             {
-                var argumentValue = ConfigurationReader.GetArgumentValue(s, _configurationAssemblies);
-                valueValue = argumentValue.ConvertTo(argument.Type, resolutionContext);
+                var propertyValue = FromSection(section, _configurationAssemblies);
+                try
+                {
+                    pi.SetValue(value, propertyValue.ConvertTo(pi.PropertyType, resolutionContext));
+                }
+                catch (Exception ex)
+                {
+                    SelfLog.WriteLine($"Serilog.Settings.Configuration: Property setter on {type} failed: {ex}");
+                }
             }
-            ctorArguments[i] = valueValue;
         }
 
-        value = ctor.ConstructorInfo.Invoke(ctorArguments);
         return true;
     }
 
@@ -262,7 +288,7 @@ static bool IsConstructableDictionary(Type type, Type elementType, [NotNullWhen(
         }
         foreach (var method in concreteType.GetMethods())
         {
-            if (!method.IsStatic && method.Name == "Add")
+            if (method is { IsStatic: false, Name: "Add" })
             {
                 var parameters = method.GetParameters();
                 if (parameters.Length == 2 && parameters[0].ParameterType == keyType && parameters[1].ParameterType == valueType)
@@ -301,7 +327,7 @@ static bool IsConstructableContainer(Type type, Type elementType, [NotNullWhen(t
         }
         foreach (var method in concreteType.GetMethods())
         {
-            if (!method.IsStatic && method.Name == "Add")
+            if (method is { IsStatic: false, Name: "Add" })
             {
                 var parameters = method.GetParameters();
                 if (parameters.Length == 1 && parameters[0].ParameterType == elementType)

src/Serilog.Settings.Configuration/Settings/Configuration/ObjectArgumentValue.cs

@@ -6,7 +6,7 @@
 
 namespace Serilog.Settings.Configuration;
 
-class StringArgumentValue : IConfigurationArgumentValue
+class StringArgumentValue : ConfigurationArgumentValue
 {
     readonly string _providedValue;
 
@@ -24,7 +24,7 @@ public StringArgumentValue(string providedValue)
             { typeof(Type), s => Type.GetType(s, throwOnError:true)! },
         };
 
-    public object? ConvertTo(Type toType, ResolutionContext resolutionContext)
+    public override object? ConvertTo(Type toType, ResolutionContext resolutionContext)
     {
         var argumentValue = Environment.ExpandEnvironmentVariables(_providedValue);