From 64834f5fd072c433bc8782e86073112e977cab34 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Tue, 8 Nov 2022 16:49:58 -0800 Subject: [PATCH] Fix Configuration with IList and ICollection (#77857) --- .../src/ConfigurationBinder.cs | 48 ++++++++----------- .../ConfigurationCollectionBindingTests.cs | 20 ++++++++ 2 files changed, 41 insertions(+), 27 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs index 6a3d2ae379fd7..fa5d77b2fddec 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs +++ b/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 and IList we bind them to mutable List 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 @@ -584,11 +587,14 @@ private static void BindConcreteDictionary( return; } - MethodInfo tryGetValue = dictionaryType.GetMethod("TryGetValue", BindingFlags.Public | BindingFlags.Instance)!; - 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 = dictionary.GetType().GetProperty("Item", BindingFlags.Public | BindingFlags.Instance); + PropertyInfo? setter = dictionaryObjectType.GetProperty("Item", BindingFlags.Public | BindingFlags.Instance); if (setter is null || !setter.CanWrite) { // Cannot set any item on the dictionary object. @@ -845,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; } diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs index cdf9448e9c7b0..66bc1402bdd8c 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs +++ b/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] @@ -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]