PR feedback

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

@@ -288,8 +288,8 @@ private static void BindInstance(
 
             if (config != null && config.GetChildren().Any())
             {
-                // for arrays and read-only list-like interfaces, we concatenate on to what is already there
-                if (type.IsArray || IsArrayCompatibleReadOnlyInterface(type))
+                // for arrays, collections, and read-only list-like interfaces, we concatenate on to what is already there
+                if (type.IsArray || IsArrayCompatibleInterface(type))
                 {
                     if (!bindingPoint.IsReadOnly)
                     {
@@ -298,7 +298,7 @@ private static void BindInstance(
                     return;
                 }
 
-                // for sets and read-only set interfaces, we concatenate on to what is already there
+                // for sets and read-only set interfaces, we clone what's there into a new collection.
                 if (TypeIsASetInterface(type))
                 {
                     if (!bindingPoint.IsReadOnly)
@@ -312,13 +312,10 @@ private static void BindInstance(
                     return;
                 }
 
-                bool typeIsADictionaryInterface = TypeIsADictionaryInterface(type);
-
-                // For other mutable interfaces like ICollection<> and ISet<>, we prefer copying values and setting them
+                // For other mutable interfaces like ICollection<>, IDictionary<,> and ISet<>, we prefer copying values and setting them
                 // on a new instance of the interface over populating the existing instance implementing the interface.
-                // This has already been done, so there's not need to check again. For dictionaries, we fill the existing
-                // instance if there is one (which hasn't happened yet), and only create a new instance if necessary.
-                if (typeIsADictionaryInterface)
+                // This has already been done, so there's not need to check again.
+                if (TypeIsADictionaryInterface(type))
                 {
                     if (!bindingPoint.IsReadOnly)
                     {
@@ -344,28 +341,17 @@ private static void BindInstance(
                     // on a new instance of the interface over populating the existing instance implementing the interface.
                     // This has already been done, so there's not need to check again. For dictionaries, we fill the existing
                     // instance if there is one (which hasn't happened yet), and only create a new instance if necessary.
-                    if (typeIsADictionaryInterface)
-                    {
-                        Type typeOfKey = type.GenericTypeArguments[0];
-                        Type typeOfValue = type.GenericTypeArguments[1];
-                        // Overwrite type in case it was a IReadOnlyDictionary<>. We still want to be able to bind items.
-                        // REVIEW: What about settable IReadOnlyDictionary<> instances with an initial value?
-                        // I think we should consider preferring copying like we do for all other collection interfaces.
-                        type = typeof(Dictionary<,>).MakeGenericType(typeOfKey, typeOfValue);
-                    }
 
                     bindingPoint.SetValue(CreateInstance(type, config, options));
                 }
 
                 // At this point we know that we have a non-null bindingPoint.Value, we just have to populate the items
                 // using the IDictionary<> or ICollection<> interfaces, or properties using reflection.
                 Type? dictionaryInterface = FindOpenGenericInterface(typeof(IDictionary<,>), type);
-                Type? dictionaryInterface2 = FindOpenGenericInterface(typeof(IReadOnlyDictionary<,>), type);
 
-                Type? di = dictionaryInterface ?? dictionaryInterface2;
-                if (di != null)
+                if (dictionaryInterface != null)
                 {
-                    BindConcreteDictionary(bindingPoint.Value!, di, config, options);
+                    BindConcreteDictionary(bindingPoint.Value!, dictionaryInterface, config, options);
                 }
                 else
                 {
@@ -812,7 +798,7 @@ private static bool TypeIsADictionaryInterface(Type type)
                 || genericTypeDefinition == typeof(IReadOnlyDictionary<,>);
         }
 
-        private static bool IsArrayCompatibleReadOnlyInterface(Type type)
+        private static bool IsArrayCompatibleInterface(Type type)
         {
             if (!type.IsInterface || !type.IsConstructedGenericType) { return false; }
 

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

@@ -4,6 +4,7 @@
 using System;
 using System.Collections;
 using System.Collections.Generic;
+using System.Collections.ObjectModel;
 using System.Linq;
 using Xunit;
 
@@ -1129,6 +1130,38 @@ public void CanBindInitializedCustomIndirectlyDerivedIEnumerableList()
             Assert.Equal("val1", array[3]);
         }
 
+        [Fact]
+        public void CanBindInitializedIReadOnlyDictionaryAndDoesNotMofifyTheOriginal()
+        {
+            // A field declared as IEnumerable<T> that is instantiated with a class
+            // that indirectly implements IEnumerable<T> is still bound, but with
+            // a new List<T> with the original values copied over.
+
+            var input = new Dictionary<string, string>
+            {
+                {"AlreadyInitializedDictionary:existing_key_1", "overridden!"},
+            };
+
+            var configurationBuilder = new ConfigurationBuilder();
+            configurationBuilder.AddInMemoryCollection(input);
+            var config = configurationBuilder.Build();
+
+            var options = new InitializedCollectionsOptions();
+            config.Bind(options);
+
+            var array = options.AlreadyInitializedDictionary.ToArray();
+
+            Assert.Equal(2, array.Length);
+
+            Assert.Equal("overridden!", options.AlreadyInitializedDictionary["existing_key_1"]);
+            Assert.Equal("val_2", options.AlreadyInitializedDictionary["existing_key_2"]);
+
+            Assert.NotEqual(options.AlreadyInitializedDictionary, InitializedCollectionsOptions.ExistingDictionary);
+
+            Assert.Equal("val_1", InitializedCollectionsOptions.ExistingDictionary["existing_key_1"]);
+            Assert.Equal("val_2", InitializedCollectionsOptions.ExistingDictionary["existing_key_2"]);
+        }
+
         [Fact]
         public void CanBindUninitializedICollection()
         {
@@ -1322,21 +1355,31 @@ private class InitializedCollectionsOptions
             public InitializedCollectionsOptions()
             {
                 AlreadyInitializedIEnumerableInterface = ListUsedInIEnumerableFieldAndShouldNotBeTouched;
+                AlreadyInitializedDictionary = ExistingDictionary;
             }
 
-            public List<string> ListUsedInIEnumerableFieldAndShouldNotBeTouched = new List<string>
+            public List<string> ListUsedInIEnumerableFieldAndShouldNotBeTouched = new()
             {
                 "This was here too",
                 "Don't touch me!"
             };
 
+            public static ReadOnlyDictionary<string, string> ExistingDictionary = new(
+                new Dictionary<string, string>
+                {
+                    {"existing_key_1", "val_1"},
+                    {"existing_key_2", "val_2"}
+                });
+
             public IEnumerable<string> AlreadyInitializedIEnumerableInterface { get; set; }
 
             public IEnumerable<string> AlreadyInitializedCustomListDerivedFromIEnumerable { get; set; } =
                 new CustomListDerivedFromIEnumerable();
 
             public IEnumerable<string> AlreadyInitializedCustomListIndirectlyDerivedFromIEnumerable { get; set; } =
                 new CustomListIndirectlyDerivedFromIEnumerable();
+
+            public IReadOnlyDictionary<string, string> AlreadyInitializedDictionary { get; set; }
         }
 
         private class CustomList : List<string>