From 091801afa135c36e16bed19fc83186c5bb86c92d Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu, 10 Nov 2022 16:56:55 -0800 Subject: [PATCH] [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 --- .../src/ConfigurationBinder.cs | 61 ++++++++------- ...oft.Extensions.Configuration.Binder.csproj | 2 + .../ConfigurationCollectionBindingTests.cs | 78 ++++++++++++++++++- 3 files changed, 110 insertions(+), 31 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs index e36ae28fd4316..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 @@ -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; } diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/src/Microsoft.Extensions.Configuration.Binder.csproj b/src/libraries/Microsoft.Extensions.Configuration.Binder/src/Microsoft.Extensions.Configuration.Binder.csproj index f5517f57f23c6..55eba74864b09 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/src/Microsoft.Extensions.Configuration.Binder.csproj +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/src/Microsoft.Extensions.Configuration.Binder.csproj @@ -5,6 +5,8 @@ true true true + true + 1 Functionality to bind an object to data in configuration providers for Microsoft.Extensions.Configuration. diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs index 4f2b5911b2b7a..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] @@ -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 FilteredConfigValues => ConfigValues.Where(p => p > 10); public IEnumerable ConfigValues { get; set; } } + + [Fact] + public void DifferentDictionaryBindingCasesTest() + { + var dic = new Dictionary() { { "key", "value" } }; + var config = new ConfigurationBuilder() + .AddInMemoryCollection(dic) + .Build(); + + Assert.Single(config.Get>()); + Assert.Single(config.Get>()); + Assert.Single(config.Get>()); + Assert.Single(config.Get>()); + } + + public class ImplementerOfIDictionaryClass : IDictionary + { + private Dictionary _dict = new(); + + public TValue this[TKey key] { get => _dict[key]; set => _dict[key] = value; } + + public ICollection Keys => _dict.Keys; + + public ICollection 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 item) => _dict.Add(item.Key, item.Value); + + public void Clear() => _dict.Clear(); + + public bool Contains(KeyValuePair item) => _dict.Contains(item); + + public bool ContainsKey(TKey key) => _dict.ContainsKey(key); + + public void CopyTo(KeyValuePair[] array, int arrayIndex) => throw new NotImplementedException(); + + public IEnumerator> GetEnumerator() => _dict.GetEnumerator(); + + public bool Remove(TKey key) => _dict.Remove(key); + + public bool Remove(KeyValuePair 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 : Dictionary + { + + } } }