From 4a5d6bb71b8c0c078844ccd0c2c49dd5bee74f73 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Thu, 3 Nov 2022 12:45:32 -0700 Subject: [PATCH 1/3] Fix Configuration with IList and ICollection --- .../src/ConfigurationBinder.cs | 38 +++++++++++++++++-- .../ConfigurationCollectionBindingTests.cs | 20 ++++++++++ 2 files changed, 55 insertions(+), 3 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs index 1d0992781f08e..a63ee8a68572e 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs @@ -299,7 +299,7 @@ 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 + // for arrays and read-only list-like interfaces, we concatenate on to what is already there, if we can if (type.IsArray || IsArrayCompatibleInterface(type)) { if (!bindingPoint.IsReadOnly) @@ -315,6 +315,19 @@ private static void BindInstance( } } + Type? iCollectionInterfaceType = GetICollectionInterfaceType(type); + if (iCollectionInterfaceType is not null) + { + if (bindingPoint.Value is null) + { + Type genericType = typeof(List<>).MakeGenericType(type.GenericTypeArguments[0]); + bindingPoint.SetValue(Activator.CreateInstance(genericType)); + } + + BindCollection(bindingPoint.Value!, iCollectionInterfaceType, config, options); + return; + } + // for sets and read-only set interfaces, we clone what's there into a new collection, if we can if (TypeIsASetInterface(type) && !bindingPoint.IsReadOnly) { @@ -836,6 +849,27 @@ private static bool TryConvertValue( return result; } + private static Type? GetICollectionInterfaceType([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] Type type) + { + if (!type.IsInterface || !type.IsConstructedGenericType) { return null; } + + Type genericTypeDefinition = type.GetGenericTypeDefinition(); + + if (genericTypeDefinition == typeof(ICollection<>)) + { + return type; + } + + // We always try to use ICollection type because during binding we use the Add method. Currently the Reflection + // cannot provide the Add method from the IList<> interface even IList<> extend ICollection<>. + if (genericTypeDefinition == typeof(IList<>)) + { + return FindOpenGenericInterface(typeof(ICollection<>), type); + } + + return null; + } + private static bool TypeIsADictionaryInterface(Type type) { if (!type.IsInterface || !type.IsConstructedGenericType) { return false; } @@ -851,8 +885,6 @@ private static bool IsArrayCompatibleInterface(Type type) Type genericTypeDefinition = type.GetGenericTypeDefinition(); return genericTypeDefinition == typeof(IEnumerable<>) - || genericTypeDefinition == typeof(ICollection<>) - || genericTypeDefinition == typeof(IList<>) || genericTypeDefinition == typeof(IReadOnlyCollection<>) || genericTypeDefinition == typeof(IReadOnlyList<>); } 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] From 778fc7bed2080f811e821a1ed51339787037421b Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Mon, 7 Nov 2022 14:24:39 -0800 Subject: [PATCH 2/3] Address the feedback --- .../src/ConfigurationBinder.cs | 77 +++++-------------- 1 file changed, 21 insertions(+), 56 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs index a63ee8a68572e..b3df77639b304 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs @@ -300,31 +300,14 @@ 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 we can - if (type.IsArray || IsArrayCompatibleInterface(type)) + 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; - } - } - - Type? iCollectionInterfaceType = GetICollectionInterfaceType(type); - if (iCollectionInterfaceType is not null) - { - if (bindingPoint.Value is null) - { - Type genericType = typeof(List<>).MakeGenericType(type.GenericTypeArguments[0]); - bindingPoint.SetValue(Activator.CreateInstance(genericType)); - } - - BindCollection(bindingPoint.Value!, iCollectionInterfaceType, config, options); return; } @@ -363,12 +346,24 @@ 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 + { + // 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. + + 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 @@ -597,9 +592,10 @@ private static void BindConcreteDictionary( return; } - MethodInfo tryGetValue = dictionaryType.GetMethod("TryGetValue", BindingFlags.Public | BindingFlags.Instance)!; - Debug.Assert(dictionary is not null); + + MethodInfo tryGetValue = dictionary.GetType().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); if (setter is null || !setter.CanWrite) @@ -849,27 +845,6 @@ private static bool TryConvertValue( return result; } - private static Type? GetICollectionInterfaceType([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] Type type) - { - if (!type.IsInterface || !type.IsConstructedGenericType) { return null; } - - Type genericTypeDefinition = type.GetGenericTypeDefinition(); - - if (genericTypeDefinition == typeof(ICollection<>)) - { - return type; - } - - // We always try to use ICollection type because during binding we use the Add method. Currently the Reflection - // cannot provide the Add method from the IList<> interface even IList<> extend ICollection<>. - if (genericTypeDefinition == typeof(IList<>)) - { - return FindOpenGenericInterface(typeof(ICollection<>), type); - } - - return null; - } - private static bool TypeIsADictionaryInterface(Type type) { if (!type.IsInterface || !type.IsConstructedGenericType) { return false; } @@ -879,16 +854,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(IReadOnlyCollection<>) - || genericTypeDefinition == typeof(IReadOnlyList<>); - } - private static bool IsImmutableArrayCompatibleInterface(Type type) { if (!type.IsInterface || !type.IsConstructedGenericType) { return false; } From f2af9ad90ba9007f5d4aa660fd341c756642bc20 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Tue, 8 Nov 2022 09:12:57 -0800 Subject: [PATCH 3/3] More feedback --- .../src/ConfigurationBinder.cs | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs index b3df77639b304..c384ba50c7314 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs @@ -357,11 +357,6 @@ private static void BindInstance( } else { - // 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. - bindingPoint.SetValue(CreateInstance(type, config, options)); } } @@ -594,10 +589,12 @@ private static void BindConcreteDictionary( Debug.Assert(dictionary is not null); - MethodInfo tryGetValue = dictionary.GetType().GetMethod("TryGetValue", BindingFlags.Public | BindingFlags.Instance)!; + 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.