[release/7.0] Fix Configuration Binding with Instantiated Objects (#81250)

* Fix Configuration Binding with Instantiated Objects

* Increment the servicing version

Author: tarekgh

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

@@ -315,12 +315,15 @@ 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)
+                if (TypeIsASetInterface(type))
                 {
-                    object? newValue = BindSet(type, (IEnumerable?)bindingPoint.Value, config, options);
-                    if (newValue != null)
+                    if (!bindingPoint.IsReadOnly || bindingPoint.Value is not null)
                     {
-                        bindingPoint.SetValue(newValue);
+                        object? newValue = BindSet(type, (IEnumerable?)bindingPoint.Value, config, options);
+                        if (!bindingPoint.IsReadOnly && newValue != null)
+                        {
+                            bindingPoint.SetValue(newValue);
+                        }
                     }
 
                     return;
@@ -528,33 +531,41 @@ 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)
+            // addMethod can only be null if dictionaryType is IReadOnlyDictionary<TKey, TValue> rather than IDictionary<TKey, TValue>.
+            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);
+
+            BindDictionary(source, dictionaryType, config, options);
 
-            return dictionary;
+            return source;
         }
 
         // Binds and potentially overwrites a dictionary object.
@@ -727,32 +738,38 @@ private static Array BindArray(Type type, IEnumerable? source, IConfiguration co
         {
             Type elementType = type.GetGenericArguments()[0];
 
-            Type keyType = type.GenericTypeArguments[0];
+            bool keyTypeIsEnum = elementType.IsEnum;
 
-            bool keyTypeIsEnum = keyType.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)
+            // addMethod can only be null if type is IReadOnlySet<T> rather than ISet<T>.
+            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);
+
+                if (source != null)
                 {
-                    arguments[0] = item;
-                    addMethod.Invoke(instance, arguments);
+                    foreach (object? item in source)
+                    {
+                        arguments[0] = item;
+                        addMethod!.Invoke(instance, arguments);
+                    }
                 }
+
+                source = (IEnumerable)instance;
             }
 
+            Debug.Assert(source is not null);
+            Debug.Assert(addMethod is not null);
+
             foreach (IConfigurationSection section in config.GetChildren())
             {
                 var itemBindingPoint = new BindingPoint();
@@ -767,15 +784,15 @@ private static Array BindArray(Type type, IEnumerable? source, IConfiguration co
                     {
                         arguments[0] = itemBindingPoint.Value;
 
-                        addMethod.Invoke(instance, arguments);
+                        addMethod.Invoke(source, arguments);
                     }
                 }
                 catch
                 {
                 }
             }
 
-            return instance;
+            return source;
         }
 
         [RequiresUnreferencedCode(TrimmingWarningMessage)]

src/libraries/Microsoft.Extensions.Configuration.Binder/src/Microsoft.Extensions.Configuration.Binder.csproj

@@ -5,7 +5,7 @@
     <EnableDefaultItems>true</EnableDefaultItems>
     <IsPackable>true</IsPackable>
     <EnableAOTAnalyzer>true</EnableAOTAnalyzer>
-    <ServicingVersion>3</ServicingVersion>
+    <ServicingVersion>4</ServicingVersion>
     <GeneratePackageOnBuild>true</GeneratePackageOnBuild>
     <PackageDescription>Functionality to bind an object to data in configuration providers for Microsoft.Extensions.Configuration.</PackageDescription>
   </PropertyGroup>

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

@@ -1685,5 +1685,182 @@ 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.True(3 == options.InstantiatedIEnumerable.Count(), $"InstantiatedIEnumerable count is {options.InstantiatedIEnumerable.Count()} .. {options.InstantiatedIEnumerable.ElementAt(options.InstantiatedIEnumerable.Count() - 1)}");
+            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.True(3 == options.InstantiatedIList.Count(), $"InstantiatedIList count is {options.InstantiatedIList.Count()} .. {options.InstantiatedIList[options.InstantiatedIList.Count() - 1]}");
+            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.True(3 == options.InstantiatedIReadOnlyList.Count(), $"InstantiatedIReadOnlyList count is {options.InstantiatedIReadOnlyList.Count()} .. {options.InstantiatedIReadOnlyList[options.InstantiatedIReadOnlyList.Count() - 1]}");
+            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.True(3 == options.InstantiatedIReadOnlyList.Count(), $"InstantiatedIReadOnlyList count is {options.InstantiatedIReadOnlyList.Count()} .. {options.InstantiatedIReadOnlyList[options.InstantiatedIReadOnlyList.Count() - 1]}");
+            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.True(3 == options.InstantiatedIReadOnlyDictionary.Count(), $"InstantiatedIReadOnlyDictionary count is {options.InstantiatedIReadOnlyDictionary.Count()} .. {options.InstantiatedIReadOnlyDictionary.ElementAt(options.InstantiatedIReadOnlyDictionary.Count() - 1)}");
+            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.True(3 == options.InstantiatedISet.Count(), $"InstantiatedISet count is {options.InstantiatedISet.Count()} .. {string.Join(", ", options.InstantiatedISet)} .. {options.IsSameInstantiatedISet()}");
+            Assert.Equal(new string[] { "a", "b", "C" }, options.InstantiatedISet);
+            Assert.True(options.IsSameInstantiatedISet());
+
+            Assert.True(3 == options.UnInstantiatedISet.Count(), $"UnInstantiatedISet count is {options.UnInstantiatedISet.Count()} .. {options.UnInstantiatedISet.ElementAt(options.UnInstantiatedISet.Count() - 1)}");
+            Assert.Equal(new string[] { "a", "A", "B" }, options.UnInstantiatedISet);
+
+#if NETCOREAPP
+            Assert.True(3 == options.InstantiatedIReadOnlySet.Count(), $"InstantiatedIReadOnlySet count is {options.InstantiatedIReadOnlySet.Count()} .. {options.InstantiatedIReadOnlySet.ElementAt(options.InstantiatedIReadOnlySet.Count() - 1)}");
+            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]);
+        }
     }
 }

src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ILLink.Descriptors.xml

@@ -15,4 +15,8 @@
         <method signature="System.Void .ctor()" />
     </type>
   </assembly>
+
+  <assembly fullname="System.Private.Corelib">
+    <type fullname="System.Collections.Generic.ISet`1" preserve="methods" />
+   </assembly>
 </linker>