[release/7.0] Fix Binding with IList<>, ICollection and IDictionary<,> implementer types (#78118)

* Fix Binding with IDictionary<,> implementer types

* Address the feedback

* Feedback addressing

* Fix Configuration with IList and ICollection (#77857)

* Add servicing version to the source project

Co-authored-by: Tarek Mahmoud Sayed <[email protected]>

Author: github-actions[bot]

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

@@ -299,20 +299,16 @@ private static void BindInstance(
 
             if (config != null && config.GetChildren().Any())
             {
-                // for arrays, collections, and read-only list-like interfaces, we concatenate on to what is already there, if we can
-                if (type.IsArray || IsArrayCompatibleInterface(type))
+                // for arrays and read-only list-like interfaces, we concatenate on to what is already there, if we can
+                if (type.IsArray || IsImmutableArrayCompatibleInterface(type))
                 {
                     if (!bindingPoint.IsReadOnly)
                     {
                         bindingPoint.SetValue(BindArray(type, (IEnumerable?)bindingPoint.Value, config, options));
-                        return;
                     }
 
                     // for getter-only collection properties that we can't add to, nothing more we can do
-                    if (type.IsArray || IsImmutableArrayCompatibleInterface(type))
-                    {
-                        return;
-                    }
+                    return;
                 }
 
                 // for sets and read-only set interfaces, we clone what's there into a new collection, if we can
@@ -350,12 +346,19 @@ private static void BindInstance(
                         return;
                     }
 
-                    // For other mutable interfaces like ICollection<> 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.
+                    Type? interfaceGenericType = type.IsInterface && type.IsConstructedGenericType ?  type.GetGenericTypeDefinition() : null;
 
-                    bindingPoint.SetValue(CreateInstance(type, config, options));
+                    if (interfaceGenericType is not null &&
+                        (interfaceGenericType == typeof(ICollection<>) || interfaceGenericType == typeof(IList<>)))
+                    {
+                        // For ICollection<T> and IList<T> we bind them to mutable List<T> type.
+                        Type genericType = typeof(List<>).MakeGenericType(type.GenericTypeArguments[0]);
+                        bindingPoint.SetValue(Activator.CreateInstance(genericType));
+                    }
+                    else
+                    {
+                        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
@@ -554,9 +557,9 @@ private static bool CanBindToTheseConstructorParameters(ParameterInfo[] construc
         // Binds and potentially overwrites a concrete dictionary.
         // This differs from BindDictionaryInterface because this method doesn't clone
         // the dictionary; it sets and/or overwrites values directly.
-        // When a user specifies a concrete dictionary in their config class, then that
-        // value is used as-us. When a user specifies an interface (instantiated) in their config class,
-        // then it is cloned to a new dictionary, the same way as other collections.
+        // When a user specifies a concrete dictionary or a concrete class implementing IDictionary<,>
+        // in their config class, then that value is used as-is. When a user specifies an interface (instantiated)
+        // in their config class, then it is cloned to a new dictionary, the same way as other collections.
         [RequiresDynamicCode(DynamicCodeWarningMessage)]
         [RequiresUnreferencedCode("Cannot statically analyze what the element type is of the value objects in the dictionary so its members may be trimmed.")]
         private static void BindConcreteDictionary(
@@ -584,10 +587,20 @@ private static void BindConcreteDictionary(
                 return;
             }
 
-            Type genericType = typeof(Dictionary<,>).MakeGenericType(keyType, valueType);
+            Debug.Assert(dictionary is not null);
+
+            Type dictionaryObjectType = dictionary.GetType();
+
+            MethodInfo tryGetValue = dictionaryObjectType.GetMethod("TryGetValue", BindingFlags.Public | BindingFlags.Instance)!;
+
+            // dictionary should be of type Dictionary<,> or of type implementing IDictionary<,>
+            PropertyInfo? setter = dictionaryObjectType.GetProperty("Item", BindingFlags.Public | BindingFlags.Instance);
+            if (setter is null || !setter.CanWrite)
+            {
+                // Cannot set any item on the dictionary object.
+                return;
+            }
 
-            MethodInfo tryGetValue = dictionaryType.GetMethod("TryGetValue")!;
-            PropertyInfo setter = genericType.GetProperty("Item", DeclaredOnlyLookup)!;
             foreach (IConfigurationSection child in config.GetChildren())
             {
                 try
@@ -838,18 +851,6 @@ private static bool TypeIsADictionaryInterface(Type type)
                 || genericTypeDefinition == typeof(IReadOnlyDictionary<,>);
         }
 
-        private static bool IsArrayCompatibleInterface(Type type)
-        {
-            if (!type.IsInterface || !type.IsConstructedGenericType) { return false; }
-
-            Type genericTypeDefinition = type.GetGenericTypeDefinition();
-            return genericTypeDefinition == typeof(IEnumerable<>)
-                || genericTypeDefinition == typeof(ICollection<>)
-                || genericTypeDefinition == typeof(IList<>)
-                || genericTypeDefinition == typeof(IReadOnlyCollection<>)
-                || genericTypeDefinition == typeof(IReadOnlyList<>);
-        }
-
         private static bool IsImmutableArrayCompatibleInterface(Type type)
         {
             if (!type.IsInterface || !type.IsConstructedGenericType) { return false; }

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

@@ -5,6 +5,8 @@
     <EnableDefaultItems>true</EnableDefaultItems>
     <IsPackable>true</IsPackable>
     <EnableAOTAnalyzer>true</EnableAOTAnalyzer>
+    <GeneratePackageOnBuild>true</GeneratePackageOnBuild>
+    <ServicingVersion>1</ServicingVersion>
     <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

@@ -393,6 +393,11 @@ public void AlreadyInitializedListInterfaceBinding()
             Assert.Equal("val1", list[2]);
             Assert.Equal("val2", list[3]);
             Assert.Equal("valx", list[4]);
+
+            // Ensure expandability of the returned list
+            options.AlreadyInitializedListInterface.Add("ExtraItem");
+            Assert.Equal(6, options.AlreadyInitializedListInterface.Count);
+            Assert.Equal("ExtraItem", options.AlreadyInitializedListInterface[5]);
         }
 
         [Fact]
@@ -1067,7 +1072,7 @@ public void CanBindInitializedIEnumerableAndTheOriginalItemsAreNotMutated()
                 {"AlreadyInitializedIEnumerableInterface:1", "val1"},
                 {"AlreadyInitializedIEnumerableInterface:2", "val2"},
                 {"AlreadyInitializedIEnumerableInterface:x", "valx"},
-                
+
                 {"ICollectionNoSetter:0", "val0"},
                 {"ICollectionNoSetter:1", "val1"},
             };
@@ -1098,6 +1103,11 @@ public void CanBindInitializedIEnumerableAndTheOriginalItemsAreNotMutated()
             Assert.Equal(2, options.ICollectionNoSetter.Count);
             Assert.Equal("val0", options.ICollectionNoSetter.ElementAt(0));
             Assert.Equal("val1", options.ICollectionNoSetter.ElementAt(1));
+
+            // Ensure expandability of the returned collection
+            options.ICollectionNoSetter.Add("ExtraItem");
+            Assert.Equal(3, options.ICollectionNoSetter.Count);
+            Assert.Equal("ExtraItem", options.ICollectionNoSetter.ElementAt(2));
         }
 
         [Fact]
@@ -1218,6 +1228,11 @@ public void CanBindUninitializedICollection()
             Assert.Equal("val1", array[1]);
             Assert.Equal("val2", array[2]);
             Assert.Equal("valx", array[3]);
+
+            // Ensure expandability of the returned collection
+            options.ICollection.Add("ExtraItem");
+            Assert.Equal(5, options.ICollection.Count);
+            Assert.Equal("ExtraItem", options.ICollection.ElementAt(4));
         }
 
         [Fact]
@@ -1246,6 +1261,11 @@ public void CanBindUninitializedIList()
             Assert.Equal("val1", list[1]);
             Assert.Equal("val2", list[2]);
             Assert.Equal("valx", list[3]);
+
+            // Ensure expandability of the returned list
+            options.IList.Add("ExtraItem");
+            Assert.Equal(5, options.IList.Count);
+            Assert.Equal("ExtraItem", options.IList[4]);
         }
 
         [Fact]
@@ -1602,5 +1622,61 @@ private class OptionsWithInterdependentProperties
             public IEnumerable<int> FilteredConfigValues => ConfigValues.Where(p => p > 10);
             public IEnumerable<int> ConfigValues { get; set; }
         }
+
+        [Fact]
+        public void DifferentDictionaryBindingCasesTest()
+        {
+            var dic = new Dictionary<string, string>() { { "key", "value" } };
+            var config = new ConfigurationBuilder()
+                .AddInMemoryCollection(dic)
+                .Build();
+
+            Assert.Single(config.Get<Dictionary<string, string>>());
+            Assert.Single(config.Get<IDictionary<string, string>>());
+            Assert.Single(config.Get<ExtendedDictionary<string, string>>());
+            Assert.Single(config.Get<ImplementerOfIDictionaryClass<string, string>>());
+        }
+
+        public class ImplementerOfIDictionaryClass<TKey, TValue> : IDictionary<TKey, TValue>
+        {
+            private Dictionary<TKey, TValue> _dict = new();
+
+            public TValue this[TKey key] { get => _dict[key]; set => _dict[key] = value; }
+
+            public ICollection<TKey> Keys => _dict.Keys;
+
+            public ICollection<TValue> Values => _dict.Values;
+
+            public int Count => _dict.Count;
+
+            public bool IsReadOnly => false;
+
+            public void Add(TKey key, TValue value) => _dict.Add(key, value);
+
+            public void Add(KeyValuePair<TKey, TValue> item) => _dict.Add(item.Key, item.Value);
+
+            public void Clear() => _dict.Clear();
+
+            public bool Contains(KeyValuePair<TKey, TValue> item) => _dict.Contains(item);
+
+            public bool ContainsKey(TKey key) => _dict.ContainsKey(key);
+
+            public void CopyTo(KeyValuePair<TKey, TValue>[] array, int arrayIndex) => throw new NotImplementedException();
+
+            public IEnumerator<KeyValuePair<TKey, TValue>> GetEnumerator() => _dict.GetEnumerator();
+
+            public bool Remove(TKey key) => _dict.Remove(key);
+
+            public bool Remove(KeyValuePair<TKey, TValue> item) => _dict.Remove(item.Key);
+
+            public bool TryGetValue(TKey key, out TValue value) => _dict.TryGetValue(key, out value);
+
+            System.Collections.IEnumerator System.Collections.IEnumerable.GetEnumerator() => _dict.GetEnumerator();
+        }
+
+        public class ExtendedDictionary<TKey, TValue> : Dictionary<TKey, TValue>
+        {
+
+        }
     }
 }