From 233162224a4b9fef36de86156da2361b241d31cc Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Thu, 26 Jan 2023 16:51:13 -0800 Subject: [PATCH 1/2] Fix Configuration Binding with Instantiated Objects --- .../src/ConfigurationBinder.cs | 99 ++++++---- .../ConfigurationCollectionBindingTests.cs | 177 ++++++++++++++++++ .../tests/ILLink.Descriptors.xml | 4 + 3 files changed, 239 insertions(+), 41 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs index 0d2fa560a4e037..d06473e9c8a264 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs @@ -315,12 +315,15 @@ private static void BindInstance( } // for sets and read-only set interfaces, we clone what's there into a new collection, if we can - if (TypeIsASetInterface(type) && !bindingPoint.IsReadOnly) + if (TypeIsASetInterface(type)) { - object? newValue = BindSet(type, (IEnumerable?)bindingPoint.Value, config, options); - if (newValue != null) + if (!bindingPoint.IsReadOnly || bindingPoint.Value is not null) { - bindingPoint.SetValue(newValue); + object? newValue = BindSet(type, (IEnumerable?)bindingPoint.Value, config, options); + if (!bindingPoint.IsReadOnly && newValue != null) + { + bindingPoint.SetValue(newValue); + } } return; @@ -528,33 +531,41 @@ private static bool CanBindToTheseConstructorParameters(ParameterInfo[] construc return null; } - Type genericType = typeof(Dictionary<,>).MakeGenericType(keyType, valueType); - MethodInfo addMethod = genericType.GetMethod("Add", DeclaredOnlyLookup)!; - - Type kvpType = typeof(KeyValuePair<,>).MakeGenericType(keyType, valueType); - PropertyInfo keyMethod = kvpType.GetProperty("Key", DeclaredOnlyLookup)!; - PropertyInfo valueMethod = kvpType.GetProperty("Value", DeclaredOnlyLookup)!; - - object dictionary = Activator.CreateInstance(genericType)!; - - var orig = source as IEnumerable; - object?[] arguments = new object?[2]; - - if (orig != null) + // addMethod can only be null if dictionaryType is IReadOnlyDictionary rather than IDictionary. + MethodInfo? addMethod = dictionaryType.GetMethod("Add", DeclaredOnlyLookup); + if (addMethod is null || source is null) { - foreach (object? item in orig) + dictionaryType = typeof(Dictionary<,>).MakeGenericType(keyType, valueType); + var dictionary = Activator.CreateInstance(dictionaryType); + addMethod = dictionaryType.GetMethod("Add", DeclaredOnlyLookup); + + var orig = source as IEnumerable; + if (orig is not null) { - object? k = keyMethod.GetMethod!.Invoke(item, null); - object? v = valueMethod.GetMethod!.Invoke(item, null); - arguments[0] = k; - arguments[1] = v; - addMethod.Invoke(dictionary, arguments); + Type kvpType = typeof(KeyValuePair<,>).MakeGenericType(keyType, valueType); + PropertyInfo keyMethod = kvpType.GetProperty("Key", DeclaredOnlyLookup)!; + PropertyInfo valueMethod = kvpType.GetProperty("Value", DeclaredOnlyLookup)!; + object?[] arguments = new object?[2]; + + foreach (object? item in orig) + { + object? k = keyMethod.GetMethod!.Invoke(item, null); + object? v = valueMethod.GetMethod!.Invoke(item, null); + arguments[0] = k; + arguments[1] = v; + addMethod!.Invoke(dictionary, arguments); + } } + + source = dictionary; } - BindDictionary(dictionary, genericType, config, options); + Debug.Assert(source is not null); + Debug.Assert(addMethod is not null); + + BindDictionary(source, dictionaryType, config, options); - return dictionary; + return source; } // Binds and potentially overwrites a dictionary object. @@ -727,32 +738,38 @@ private static Array BindArray(Type type, IEnumerable? source, IConfiguration co { Type elementType = type.GetGenericArguments()[0]; - Type keyType = type.GenericTypeArguments[0]; + bool keyTypeIsEnum = elementType.IsEnum; - bool keyTypeIsEnum = keyType.IsEnum; - - if (keyType != typeof(string) && !keyTypeIsEnum) + if (elementType != typeof(string) && !keyTypeIsEnum) { // We only support string and enum keys return null; } - Type genericType = typeof(HashSet<>).MakeGenericType(keyType); - object instance = Activator.CreateInstance(genericType)!; - - MethodInfo addMethod = genericType.GetMethod("Add", DeclaredOnlyLookup)!; - object?[] arguments = new object?[1]; - - if (source != null) + // addMethod can only be null if type is IReadOnlySet rather than ISet. + MethodInfo? addMethod = type.GetMethod("Add", DeclaredOnlyLookup); + if (addMethod is null || source is null) { - foreach (object? item in source) + Type genericType = typeof(HashSet<>).MakeGenericType(elementType); + object instance = Activator.CreateInstance(genericType)!; + addMethod = genericType.GetMethod("Add", DeclaredOnlyLookup); + + if (source != null) { - arguments[0] = item; - addMethod.Invoke(instance, arguments); + foreach (object? item in source) + { + arguments[0] = item; + addMethod!.Invoke(instance, arguments); + } } + + source = (IEnumerable)instance; } + Debug.Assert(source is not null); + Debug.Assert(addMethod is not null); + foreach (IConfigurationSection section in config.GetChildren()) { var itemBindingPoint = new BindingPoint(); @@ -767,7 +784,7 @@ private static Array BindArray(Type type, IEnumerable? source, IConfiguration co { arguments[0] = itemBindingPoint.Value; - addMethod.Invoke(instance, arguments); + addMethod.Invoke(source, arguments); } } catch @@ -775,7 +792,7 @@ private static Array BindArray(Type type, IEnumerable? source, IConfiguration co } } - return instance; + return source; } [RequiresUnreferencedCode(TrimmingWarningMessage)] diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs index b604f3c054d7d2..7f3bb05cc40ac8 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs @@ -1685,5 +1685,182 @@ public class ExtendedDictionary : Dictionary { } + + private class OptionsWithDifferentCollectionInterfaces + { + private static IEnumerable s_instantiatedIEnumerable = new List { "value1", "value2" }; + public bool IsSameInstantiatedIEnumerable() => object.ReferenceEquals(s_instantiatedIEnumerable, InstantiatedIEnumerable); + public IEnumerable InstantiatedIEnumerable { get; set; } = s_instantiatedIEnumerable; + + private static IList s_instantiatedIList = new List { "value1", "value2" }; + public bool IsSameInstantiatedIList() => object.ReferenceEquals(s_instantiatedIList, InstantiatedIList); + public IList InstantiatedIList { get; set; } = s_instantiatedIList; + + private static IReadOnlyList s_instantiatedIReadOnlyList = new List { "value1", "value2" }; + public bool IsSameInstantiatedIReadOnlyList() => object.ReferenceEquals(s_instantiatedIReadOnlyList, InstantiatedIReadOnlyList); + public IReadOnlyList InstantiatedIReadOnlyList { get; set; } = s_instantiatedIReadOnlyList; + + private static IDictionary s_instantiatedIDictionary = new Dictionary { ["Key1"] = "value1", ["Key2"] = "value2" }; + public IDictionary InstantiatedIDictionary { get; set; } = s_instantiatedIDictionary; + public bool IsSameInstantiatedIDictionary() => object.ReferenceEquals(s_instantiatedIDictionary, InstantiatedIDictionary); + + private static IReadOnlyDictionary s_instantiatedIReadOnlyDictionary = new Dictionary { ["Key1"] = "value1", ["Key2"] = "value2" }; + public IReadOnlyDictionary InstantiatedIReadOnlyDictionary { get; set; } = s_instantiatedIReadOnlyDictionary; + public bool IsSameInstantiatedIReadOnlyDictionary() => object.ReferenceEquals(s_instantiatedIReadOnlyDictionary, InstantiatedIReadOnlyDictionary); + + private static ISet s_instantiatedISet = new HashSet(StringComparer.OrdinalIgnoreCase) { "a", "A", "b" }; + public ISet InstantiatedISet { get; set; } = s_instantiatedISet; + public bool IsSameInstantiatedISet() => object.ReferenceEquals(s_instantiatedISet, InstantiatedISet); + +#if NETCOREAPP + private static IReadOnlySet s_instantiatedIReadOnlySet = new HashSet(StringComparer.OrdinalIgnoreCase) { "a", "A", "b" }; + public IReadOnlySet InstantiatedIReadOnlySet { get; set; } = s_instantiatedIReadOnlySet; + public bool IsSameInstantiatedIReadOnlySet() => object.ReferenceEquals(s_instantiatedIReadOnlySet, InstantiatedIReadOnlySet); + + public IReadOnlySet UnInstantiatedIReadOnlySet { get; set; } +#endif + private static ICollection s_instantiatedICollection = new List { "a", "b", "c" }; + public ICollection InstantiatedICollection { get; set; } = s_instantiatedICollection; + public bool IsSameInstantiatedICollection() => object.ReferenceEquals(s_instantiatedICollection, InstantiatedICollection); + + private static IReadOnlyCollection s_instantiatedIReadOnlyCollection = new List { "a", "b", "c" }; + public IReadOnlyCollection InstantiatedIReadOnlyCollection { get; set; } = s_instantiatedIReadOnlyCollection; + public bool IsSameInstantiatedIReadOnlyCollection() => object.ReferenceEquals(s_instantiatedIReadOnlyCollection, InstantiatedIReadOnlyCollection); + + public IReadOnlyCollection UnInstantiatedIReadOnlyCollection { get; set; } + public ICollection UnInstantiatedICollection { get; set; } + public ISet UnInstantiatedISet { get; set; } + public IReadOnlyDictionary UnInstantiatedIReadOnlyDictionary { get; set; } + public IEnumerable UnInstantiatedIEnumerable { get; set; } + public IList UnInstantiatedIList { get; set; } + public IReadOnlyList UnInstantiatedIReadOnlyList { get; set; } + } + [Fact] + public void TestOptionsWithDifferentCollectionInterfaces() + { + var input = new Dictionary + { + {"InstantiatedIEnumerable:0", "value3"}, + {"UnInstantiatedIEnumerable:0", "value1"}, + {"InstantiatedIList:0", "value3"}, + {"InstantiatedIReadOnlyList:0", "value3"}, + {"UnInstantiatedIReadOnlyList:0", "value"}, + {"UnInstantiatedIList:0", "value"}, + {"InstantiatedIDictionary:Key3", "value3"}, + {"InstantiatedIReadOnlyDictionary:Key3", "value3"}, + {"UnInstantiatedIReadOnlyDictionary:Key", "value"}, + {"InstantiatedISet:0", "B"}, + {"InstantiatedISet:1", "C"}, + {"UnInstantiatedISet:0", "a"}, + {"UnInstantiatedISet:1", "A"}, + {"UnInstantiatedISet:2", "B"}, + {"InstantiatedIReadOnlySet:0", "Z"}, + {"UnInstantiatedIReadOnlySet:0", "y"}, + {"UnInstantiatedIReadOnlySet:1", "z"}, + {"InstantiatedICollection:0", "d"}, + {"UnInstantiatedICollection:0", "t"}, + {"UnInstantiatedICollection:1", "a"}, + {"InstantiatedIReadOnlyCollection:0", "d"}, + {"UnInstantiatedIReadOnlyCollection:0", "r"}, + {"UnInstantiatedIReadOnlyCollection:1", "e"}, + }; + + var configurationBuilder = new ConfigurationBuilder(); + configurationBuilder.AddInMemoryCollection(input); + var config = configurationBuilder.Build(); + + var options = new OptionsWithDifferentCollectionInterfaces(); + config.Bind(options); + + Assert.True(3 == options.InstantiatedIEnumerable.Count(), $"InstantiatedIEnumerable count is {options.InstantiatedIEnumerable.Count()} .. {options.InstantiatedIEnumerable.ElementAt(options.InstantiatedIEnumerable.Count() - 1)}"); + Assert.Equal("value1", options.InstantiatedIEnumerable.ElementAt(0)); + Assert.Equal("value2", options.InstantiatedIEnumerable.ElementAt(1)); + Assert.Equal("value3", options.InstantiatedIEnumerable.ElementAt(2)); + Assert.False(options.IsSameInstantiatedIEnumerable()); + + Assert.Equal(1, options.UnInstantiatedIEnumerable.Count()); + Assert.Equal("value1", options.UnInstantiatedIEnumerable.ElementAt(0)); + + Assert.True(3 == options.InstantiatedIList.Count(), $"InstantiatedIList count is {options.InstantiatedIList.Count()} .. {options.InstantiatedIList[options.InstantiatedIList.Count() - 1]}"); + Assert.Equal("value1", options.InstantiatedIList[0]); + Assert.Equal("value2", options.InstantiatedIList[1]); + Assert.Equal("value3", options.InstantiatedIList[2]); + Assert.True(options.IsSameInstantiatedIList()); + + Assert.Equal(1, options.UnInstantiatedIList.Count()); + Assert.Equal("value", options.UnInstantiatedIList[0]); + + Assert.True(3 == options.InstantiatedIReadOnlyList.Count(), $"InstantiatedIReadOnlyList count is {options.InstantiatedIReadOnlyList.Count()} .. {options.InstantiatedIReadOnlyList[options.InstantiatedIReadOnlyList.Count() - 1]}"); + Assert.Equal("value1", options.InstantiatedIReadOnlyList[0]); + Assert.Equal("value2", options.InstantiatedIReadOnlyList[1]); + Assert.Equal("value3", options.InstantiatedIReadOnlyList[2]); + Assert.False(options.IsSameInstantiatedIReadOnlyList()); + + Assert.Equal(1, options.UnInstantiatedIReadOnlyList.Count()); + Assert.Equal("value", options.UnInstantiatedIReadOnlyList[0]); + + Assert.True(3 == options.InstantiatedIReadOnlyList.Count(), $"InstantiatedIReadOnlyList count is {options.InstantiatedIReadOnlyList.Count()} .. {options.InstantiatedIReadOnlyList[options.InstantiatedIReadOnlyList.Count() - 1]}"); + Assert.Equal(new string[] { "Key1", "Key2", "Key3" }, options.InstantiatedIDictionary.Keys); + Assert.Equal(new string[] { "value1", "value2", "value3" }, options.InstantiatedIDictionary.Values); + Assert.True(options.IsSameInstantiatedIDictionary()); + + Assert.True(3 == options.InstantiatedIReadOnlyDictionary.Count(), $"InstantiatedIReadOnlyDictionary count is {options.InstantiatedIReadOnlyDictionary.Count()} .. {options.InstantiatedIReadOnlyDictionary.ElementAt(options.InstantiatedIReadOnlyDictionary.Count() - 1)}"); + Assert.Equal(new string[] { "Key1", "Key2", "Key3" }, options.InstantiatedIReadOnlyDictionary.Keys); + Assert.Equal(new string[] { "value1", "value2", "value3" }, options.InstantiatedIReadOnlyDictionary.Values); + Assert.False(options.IsSameInstantiatedIReadOnlyDictionary()); + + Assert.Equal(1, options.UnInstantiatedIReadOnlyDictionary.Count()); + Assert.Equal(new string[] { "Key" }, options.UnInstantiatedIReadOnlyDictionary.Keys); + Assert.Equal(new string[] { "value" }, options.UnInstantiatedIReadOnlyDictionary.Values); + + Assert.True(3 == options.InstantiatedISet.Count(), $"InstantiatedISet count is {options.InstantiatedISet.Count()} .. {string.Join(", ", options.InstantiatedISet)} .. {options.IsSameInstantiatedISet()}"); + Assert.Equal(new string[] { "a", "b", "C" }, options.InstantiatedISet); + Assert.True(options.IsSameInstantiatedISet()); + + Assert.True(3 == options.UnInstantiatedISet.Count(), $"UnInstantiatedISet count is {options.UnInstantiatedISet.Count()} .. {options.UnInstantiatedISet.ElementAt(options.UnInstantiatedISet.Count() - 1)}"); + Assert.Equal(new string[] { "a", "A", "B" }, options.UnInstantiatedISet); + +#if NETCOREAPP + Assert.True(3 == options.InstantiatedIReadOnlySet.Count(), $"InstantiatedIReadOnlySet count is {options.InstantiatedIReadOnlySet.Count()} .. {options.InstantiatedIReadOnlySet.ElementAt(options.InstantiatedIReadOnlySet.Count() - 1)}"); + Assert.Equal(new string[] { "a", "b", "Z" }, options.InstantiatedIReadOnlySet); + Assert.False(options.IsSameInstantiatedIReadOnlySet()); + + Assert.Equal(2, options.UnInstantiatedIReadOnlySet.Count()); + Assert.Equal(new string[] { "y", "z" }, options.UnInstantiatedIReadOnlySet); +#endif + Assert.Equal(4, options.InstantiatedICollection.Count()); + Assert.Equal(new string[] { "a", "b", "c", "d" }, options.InstantiatedICollection); + Assert.True(options.IsSameInstantiatedICollection()); + + Assert.Equal(2, options.UnInstantiatedICollection.Count()); + Assert.Equal(new string[] { "t", "a" }, options.UnInstantiatedICollection); + + Assert.Equal(4, options.InstantiatedIReadOnlyCollection.Count()); + Assert.Equal(new string[] { "a", "b", "c", "d" }, options.InstantiatedIReadOnlyCollection); + Assert.False(options.IsSameInstantiatedIReadOnlyCollection()); + + Assert.Equal(2, options.UnInstantiatedIReadOnlyCollection.Count()); + Assert.Equal(new string[] { "r", "e" }, options.UnInstantiatedIReadOnlyCollection); + } + + [Fact] + public void TestMutatingDictionaryValues() + { + IConfiguration config = new ConfigurationBuilder() + .AddInMemoryCollection() + .Build(); + + config["Key:0"] = "NewValue"; + var dict = new Dictionary() { { "Key", new[] { "InitialValue" } } }; + + Assert.Equal(1, dict["Key"].Length); + Assert.Equal("InitialValue", dict["Key"][0]); + + // Binding will accumulate to the values inside the dictionary. + config.Bind(dict); + Assert.Equal(2, dict["Key"].Length); + Assert.Equal("InitialValue", dict["Key"][0]); + Assert.Equal("NewValue", dict["Key"][1]); + } } } diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ILLink.Descriptors.xml b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ILLink.Descriptors.xml index 5b9621080c97b0..034e7f3ea5c734 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ILLink.Descriptors.xml +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ILLink.Descriptors.xml @@ -15,4 +15,8 @@ + + + + From 6d62ee16f64c191d5fb1d685a3d9fa22cfafd010 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Thu, 26 Jan 2023 17:26:58 -0800 Subject: [PATCH 2/2] Increment the servicing version --- .../src/Microsoft.Extensions.Configuration.Binder.csproj | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 c8834b6b4b3d5b..9cc8337165ea9c 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,7 +5,7 @@ true true true - 3 + 4 true Functionality to bind an object to data in configuration providers for Microsoft.Extensions.Configuration.