Fix Configuration Binding with Instantiated Objects

src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs

@@ -317,7 +317,7 @@ private static void BindInstance(
                 // for sets and read-only set interfaces, we clone what's there into a new collection, if we can
                 if (TypeIsASetInterface(type) && !bindingPoint.IsReadOnly)
                 {
-                    object? newValue = BindSet(type, (IEnumerable?)bindingPoint.Value, config, options);
+                    object? newValue = BindSet(type, bindingPoint.Value, config, options);
                     if (newValue != null)
                     {
                         bindingPoint.SetValue(newValue);
@@ -530,33 +530,40 @@ private static bool CanBindToTheseConstructorParameters(ParameterInfo[] construc
                 return null;
             }
 
-            Type genericType = typeof(Dictionary<,>).MakeGenericType(keyType, valueType);
-            MethodInfo addMethod = genericType.GetMethod("Add", DeclaredOnlyLookup)!;
-
-            Type kvpType = typeof(KeyValuePair<,>).MakeGenericType(keyType, valueType);
-            PropertyInfo keyMethod = kvpType.GetProperty("Key", DeclaredOnlyLookup)!;
-            PropertyInfo valueMethod = kvpType.GetProperty("Value", DeclaredOnlyLookup)!;
-
-            object dictionary = Activator.CreateInstance(genericType)!;
-
-            var orig = source as IEnumerable;
-            object?[] arguments = new object?[2];
-
-            if (orig != null)
+            MethodInfo? addMethod = dictionaryType.GetMethod("Add", DeclaredOnlyLookup);
+            if (addMethod is null || source is null)
             {
-                foreach (object? item in orig)
+                dictionaryType = typeof(Dictionary<,>).MakeGenericType(keyType, valueType);
+                var dictionary = Activator.CreateInstance(dictionaryType);
+                addMethod = dictionaryType.GetMethod("Add", DeclaredOnlyLookup);
+
+                var orig = source as IEnumerable;
+                if (orig is not null)
                 {
-                    object? k = keyMethod.GetMethod!.Invoke(item, null);
-                    object? v = valueMethod.GetMethod!.Invoke(item, null);
-                    arguments[0] = k;
-                    arguments[1] = v;
-                    addMethod.Invoke(dictionary, arguments);
+                    Type kvpType = typeof(KeyValuePair<,>).MakeGenericType(keyType, valueType);
+                    PropertyInfo keyMethod = kvpType.GetProperty("Key", DeclaredOnlyLookup)!;
+                    PropertyInfo valueMethod = kvpType.GetProperty("Value", DeclaredOnlyLookup)!;
+                    object?[] arguments = new object?[2];
+
+                    foreach (object? item in orig)
+                    {
+                        object? k = keyMethod.GetMethod!.Invoke(item, null);
+                        object? v = valueMethod.GetMethod!.Invoke(item, null);
+                        arguments[0] = k;
+                        arguments[1] = v;
+                        addMethod!.Invoke(dictionary, arguments);
+                    }
                 }
+
+                source = dictionary;
             }
 
-            BindDictionary(dictionary, genericType, config, options);
+            Debug.Assert(source is not null);
+            Debug.Assert(addMethod is not null);
 
-            return dictionary;
+            BindDictionary(source, dictionaryType, config, options);
+
+            return source;
         }
 
         // Binds and potentially overwrites a dictionary object.
@@ -733,36 +740,42 @@ private static Array BindArray(Type type, IEnumerable? source, IConfiguration co
 
         [RequiresDynamicCode(DynamicCodeWarningMessage)]
         [RequiresUnreferencedCode("Cannot statically analyze what the element type is of the Array so its members may be trimmed.")]
-        private static object? BindSet(Type type, IEnumerable? source, IConfiguration config, BinderOptions options)
+        private static object? BindSet(Type type, object? source, IConfiguration config, BinderOptions options)
         {
             Type elementType = type.GetGenericArguments()[0];
 
-            Type keyType = type.GenericTypeArguments[0];
-
-            bool keyTypeIsEnum = keyType.IsEnum;
+            bool keyTypeIsEnum = elementType.IsEnum;
 
-            if (keyType != typeof(string) && !keyTypeIsEnum)
+            if (elementType != typeof(string) && !keyTypeIsEnum)
             {
                 // We only support string and enum keys
                 return null;
             }
 
-            Type genericType = typeof(HashSet<>).MakeGenericType(keyType);
-            object instance = Activator.CreateInstance(genericType)!;
-
-            MethodInfo addMethod = genericType.GetMethod("Add", DeclaredOnlyLookup)!;
-
             object?[] arguments = new object?[1];
-
-            if (source != null)
+            MethodInfo? addMethod = type.GetMethod("Add", DeclaredOnlyLookup);
+            if (addMethod is null || source is null)
             {
-                foreach (object? item in source)
+                Type genericType = typeof(HashSet<>).MakeGenericType(elementType);
+                object instance = Activator.CreateInstance(genericType)!;
+                addMethod = genericType.GetMethod("Add", DeclaredOnlyLookup);
+
+                var orig = source as IEnumerable;
+                if (orig != null)
                 {
-                    arguments[0] = item;
-                    addMethod.Invoke(instance, arguments);
+                    foreach (object? item in orig)
+                    {
+                        arguments[0] = item;
+                        addMethod!.Invoke(instance, arguments);
+                    }
                 }
+
+                source = instance;
             }
 
+            Debug.Assert(source is not null);
+            Debug.Assert(addMethod is not null);
+
             foreach (IConfigurationSection section in config.GetChildren())
             {
                 var itemBindingPoint = new BindingPoint();
@@ -777,7 +790,7 @@ private static Array BindArray(Type type, IEnumerable? source, IConfiguration co
                     {
                         arguments[0] = itemBindingPoint.Value;
 
-                        addMethod.Invoke(instance, arguments);
+                        addMethod.Invoke(source, arguments);
                     }
                 }
                 catch (Exception ex)
@@ -790,7 +803,7 @@ private static Array BindArray(Type type, IEnumerable? source, IConfiguration co
                 }
             }
 
-            return instance;
+            return source;
         }
 
         [RequiresUnreferencedCode(TrimmingWarningMessage)]

src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationBinderTests.cs

@@ -937,8 +937,6 @@ public void CanBindInstantiatedReadOnlyDictionary2()
             Assert.Equal(2, options.Items["existing-item2"]);
             Assert.Equal(3, options.Items["item3"]);
             Assert.Equal(4, options.Items["item4"]);
-
-
         }
 
         [Fact]
@@ -956,6 +954,7 @@ public void BindInstantiatedIReadOnlyDictionary_CreatesCopyOfOriginal()
 
             var options = config.Get<ConfigWithInstantiatedIReadOnlyDictionary>()!;
 
+            // Readonly dictionary with instantiated value cannot be mutated by the configuration
             Assert.Equal(3, options.Dictionary.Count);
 
             // does not overwrite original
@@ -1027,6 +1026,7 @@ public void CanBindInstantiatedReadOnlyDictionary()
             var options = config.Get<ComplexOptions>()!;
 
             var resultingDictionary = options.InstantiatedReadOnlyDictionaryWithWithSomeValues;
+
             Assert.Equal(4, resultingDictionary.Count);
             Assert.Equal(1, resultingDictionary["existing-item1"]);
             Assert.Equal(2, resultingDictionary["existing-item2"]);

src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs

@@ -1678,12 +1678,189 @@ public class ImplementerOfIDictionaryClass<TKey, TValue> : IDictionary<TKey, TVa
             private string? v;
             public string? this[string key] { get => v; set => v = value; }
             public bool TryGetValue() { return true; }
-
         }
 
         public class ExtendedDictionary<TKey, TValue> : Dictionary<TKey, TValue>
         {
 
         }
+
+        private class OptionsWithDifferentCollectionInterfaces
+        {
+            private static IEnumerable<string> s_instantiatedIEnumerable = new List<string> { "value1", "value2" };
+            public bool IsSameInstantiatedIEnumerable() => object.ReferenceEquals(s_instantiatedIEnumerable, InstantiatedIEnumerable);
+            public IEnumerable<string> InstantiatedIEnumerable { get; set; } = s_instantiatedIEnumerable;
+
+            private static IList<string> s_instantiatedIList = new List<string> { "value1", "value2" };
+            public bool IsSameInstantiatedIList() => object.ReferenceEquals(s_instantiatedIList, InstantiatedIList);
+            public IList<string> InstantiatedIList { get; set; } = s_instantiatedIList;
+
+            private static IReadOnlyList<string> s_instantiatedIReadOnlyList = new List<string> { "value1", "value2" };
+            public bool IsSameInstantiatedIReadOnlyList() => object.ReferenceEquals(s_instantiatedIReadOnlyList, InstantiatedIReadOnlyList);
+            public IReadOnlyList<string> InstantiatedIReadOnlyList { get; set; } = s_instantiatedIReadOnlyList;
+
+            private static IDictionary<string, string> s_instantiatedIDictionary = new Dictionary<string, string> { ["Key1"] = "value1", ["Key2"] = "value2" };
+            public IDictionary<string, string> InstantiatedIDictionary { get; set; } = s_instantiatedIDictionary;
+            public bool IsSameInstantiatedIDictionary() => object.ReferenceEquals(s_instantiatedIDictionary, InstantiatedIDictionary);
+
+            private static IReadOnlyDictionary<string, string> s_instantiatedIReadOnlyDictionary = new Dictionary<string, string> { ["Key1"] = "value1", ["Key2"] = "value2" };
+            public IReadOnlyDictionary<string, string> InstantiatedIReadOnlyDictionary { get; set; } = s_instantiatedIReadOnlyDictionary;
+            public bool IsSameInstantiatedIReadOnlyDictionary() => object.ReferenceEquals(s_instantiatedIReadOnlyDictionary, InstantiatedIReadOnlyDictionary);
+
+            private static ISet<string> s_instantiatedISet = new HashSet<string>(StringComparer.OrdinalIgnoreCase) { "a", "A", "b" };
+            public ISet<string> InstantiatedISet { get; set; } = s_instantiatedISet;
+            public bool IsSameInstantiatedISet() => object.ReferenceEquals(s_instantiatedISet, InstantiatedISet);
+
+#if NETCOREAPP
+            private static IReadOnlySet<string> s_instantiatedIReadOnlySet = new HashSet<string>(StringComparer.OrdinalIgnoreCase) { "a", "A", "b" };
+            public IReadOnlySet<string> InstantiatedIReadOnlySet { get; set; } = s_instantiatedIReadOnlySet;
+            public bool IsSameInstantiatedIReadOnlySet() => object.ReferenceEquals(s_instantiatedIReadOnlySet, InstantiatedIReadOnlySet);
+
+            public IReadOnlySet<string> UnInstantiatedIReadOnlySet { get; set; }
+#endif
+            private static ICollection<string> s_instantiatedICollection = new List<string> { "a", "b", "c" };
+            public ICollection<string> InstantiatedICollection { get; set; } = s_instantiatedICollection;
+            public bool IsSameInstantiatedICollection() => object.ReferenceEquals(s_instantiatedICollection, InstantiatedICollection);
+
+            private static IReadOnlyCollection<string> s_instantiatedIReadOnlyCollection = new List<string> { "a", "b", "c" };
+            public IReadOnlyCollection<string> InstantiatedIReadOnlyCollection { get; set; } = s_instantiatedIReadOnlyCollection;
+            public bool IsSameInstantiatedIReadOnlyCollection() => object.ReferenceEquals(s_instantiatedIReadOnlyCollection, InstantiatedIReadOnlyCollection);
+
+            public IReadOnlyCollection<string> UnInstantiatedIReadOnlyCollection { get; set; }
+            public ICollection<string> UnInstantiatedICollection { get; set; }
+            public ISet<string> UnInstantiatedISet { get; set; }
+            public IReadOnlyDictionary<string, string> UnInstantiatedIReadOnlyDictionary { get; set; }
+            public IEnumerable<string> UnInstantiatedIEnumerable { get; set; }
+            public IList<string> UnInstantiatedIList { get; set; }
+            public IReadOnlyList<string> UnInstantiatedIReadOnlyList { get; set; }
+        }
+
+        [Fact]
+        public void TestOptionsWithDifferentCollectionInterfaces()
+        {
+            var input = new Dictionary<string, string>
+            {
+                {"InstantiatedIEnumerable:0", "value3"},
+                {"UnInstantiatedIEnumerable:0", "value1"},
+                {"InstantiatedIList:0", "value3"},
+                {"InstantiatedIReadOnlyList:0", "value3"},
+                {"UnInstantiatedIReadOnlyList:0", "value"},
+                {"UnInstantiatedIList:0", "value"},
+                {"InstantiatedIDictionary:Key3", "value3"},
+                {"InstantiatedIReadOnlyDictionary:Key3", "value3"},
+                {"UnInstantiatedIReadOnlyDictionary:Key", "value"},
+                {"InstantiatedISet:0", "B"},
+                {"InstantiatedISet:1", "C"},
+                {"UnInstantiatedISet:0", "a"},
+                {"UnInstantiatedISet:1", "A"},
+                {"UnInstantiatedISet:2", "B"},
+                {"InstantiatedIReadOnlySet:0", "Z"},
+                {"UnInstantiatedIReadOnlySet:0", "y"},
+                {"UnInstantiatedIReadOnlySet:1", "z"},
+                {"InstantiatedICollection:0", "d"},
+                {"UnInstantiatedICollection:0", "t"},
+                {"UnInstantiatedICollection:1", "a"},
+                {"InstantiatedIReadOnlyCollection:0", "d"},
+                {"UnInstantiatedIReadOnlyCollection:0", "r"},
+                {"UnInstantiatedIReadOnlyCollection:1", "e"},
+            };
+
+            var configurationBuilder = new ConfigurationBuilder();
+            configurationBuilder.AddInMemoryCollection(input);
+            var config = configurationBuilder.Build();
+
+            var options = new OptionsWithDifferentCollectionInterfaces();
+            config.Bind(options);
+
+            Assert.Equal(3, options.InstantiatedIEnumerable.Count());
+            Assert.Equal("value1", options.InstantiatedIEnumerable.ElementAt(0));
+            Assert.Equal("value2", options.InstantiatedIEnumerable.ElementAt(1));
+            Assert.Equal("value3", options.InstantiatedIEnumerable.ElementAt(2));
+            Assert.False(options.IsSameInstantiatedIEnumerable());
+
+            Assert.Equal(1, options.UnInstantiatedIEnumerable.Count());
+            Assert.Equal("value1", options.UnInstantiatedIEnumerable.ElementAt(0));
+
+            Assert.Equal(3, options.InstantiatedIList.Count());
+            Assert.Equal("value1", options.InstantiatedIList[0]);
+            Assert.Equal("value2", options.InstantiatedIList[1]);
+            Assert.Equal("value3", options.InstantiatedIList[2]);
+            Assert.True(options.IsSameInstantiatedIList());
+
+            Assert.Equal(1, options.UnInstantiatedIList.Count());
+            Assert.Equal("value", options.UnInstantiatedIList[0]);
+
+            Assert.Equal(3, options.InstantiatedIReadOnlyList.Count());
+            Assert.Equal("value1", options.InstantiatedIReadOnlyList[0]);
+            Assert.Equal("value2", options.InstantiatedIReadOnlyList[1]);
+            Assert.Equal("value3", options.InstantiatedIReadOnlyList[2]);
+            Assert.False(options.IsSameInstantiatedIReadOnlyList());
+
+            Assert.Equal(1, options.UnInstantiatedIReadOnlyList.Count());
+            Assert.Equal("value", options.UnInstantiatedIReadOnlyList[0]);
+
+            Assert.Equal(3, options.InstantiatedIDictionary.Count());
+            Assert.Equal(new string[] { "Key1", "Key2", "Key3" }, options.InstantiatedIDictionary.Keys);
+            Assert.Equal(new string[] { "value1", "value2", "value3" }, options.InstantiatedIDictionary.Values);
+            Assert.True(options.IsSameInstantiatedIDictionary());
+
+            Assert.Equal(3, options.InstantiatedIReadOnlyDictionary.Count());
+            Assert.Equal(new string[] { "Key1", "Key2", "Key3" }, options.InstantiatedIReadOnlyDictionary.Keys);
+            Assert.Equal(new string[] { "value1", "value2", "value3" }, options.InstantiatedIReadOnlyDictionary.Values);
+            Assert.False(options.IsSameInstantiatedIReadOnlyDictionary());
+
+            Assert.Equal(1, options.UnInstantiatedIReadOnlyDictionary.Count());
+            Assert.Equal(new string[] { "Key" }, options.UnInstantiatedIReadOnlyDictionary.Keys);
+            Assert.Equal(new string[] { "value" }, options.UnInstantiatedIReadOnlyDictionary.Values);
+
+            Assert.Equal(3, options.InstantiatedISet.Count());
+            Assert.Equal(new string[] { "a", "b", "C" }, options.InstantiatedISet);
+            Assert.True(options.IsSameInstantiatedISet());
+
+            Assert.Equal(3, options.UnInstantiatedISet.Count());
+            Assert.Equal(new string[] { "a", "A", "B" }, options.UnInstantiatedISet);
+
+#if NETCOREAPP
+            Assert.Equal(3, options.InstantiatedIReadOnlySet.Count());
+            Assert.Equal(new string[] { "a", "b", "Z" }, options.InstantiatedIReadOnlySet);
+            Assert.False(options.IsSameInstantiatedIReadOnlySet());
+
+            Assert.Equal(2, options.UnInstantiatedIReadOnlySet.Count());
+            Assert.Equal(new string[] { "y", "z" }, options.UnInstantiatedIReadOnlySet);
+#endif
+            Assert.Equal(4, options.InstantiatedICollection.Count());
+            Assert.Equal(new string[] { "a", "b", "c", "d" }, options.InstantiatedICollection);
+            Assert.True(options.IsSameInstantiatedICollection());
+
+            Assert.Equal(2, options.UnInstantiatedICollection.Count());
+            Assert.Equal(new string[] { "t", "a" }, options.UnInstantiatedICollection);
+
+            Assert.Equal(4, options.InstantiatedIReadOnlyCollection.Count());
+            Assert.Equal(new string[] { "a", "b", "c", "d" }, options.InstantiatedIReadOnlyCollection);
+            Assert.False(options.IsSameInstantiatedIReadOnlyCollection());
+
+            Assert.Equal(2, options.UnInstantiatedIReadOnlyCollection.Count());
+            Assert.Equal(new string[] { "r", "e" }, options.UnInstantiatedIReadOnlyCollection);
+        }
+
+        [Fact]
+        public void TestMutatingDictionaryValues()
+        {
+            IConfiguration config = new ConfigurationBuilder()
+                .AddInMemoryCollection()
+                .Build();
+
+            config["Key:0"] = "NewValue";
+            var dict = new Dictionary<string, string[]>() { { "Key", new[] { "InitialValue" } } };
+
+            Assert.Equal(1, dict["Key"].Length);
+            Assert.Equal("InitialValue", dict["Key"][0]);
+
+            // Binding will accumulate to the values inside the dictionary.
+            config.Bind(dict);
+            Assert.Equal(2, dict["Key"].Length);
+            Assert.Equal("InitialValue", dict["Key"][0]);
+            Assert.Equal("NewValue", dict["Key"][1]);
+        }
     }
 }