From 8d7d92042964eef26a918d9607866dca306b4ca8 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Mon, 23 Jan 2023 13:13:00 -0800 Subject: [PATCH 1/9] Fix Configuration Binding with Instantiated Objects --- .../src/ConfigurationBinder.cs | 86 ++++----- .../tests/ConfigurationBinderTests.cs | 45 ++--- .../ConfigurationCollectionBindingTests.cs | 175 ++++++++++++++++-- 3 files changed, 221 insertions(+), 85 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs index 0d2e4e47ce7389..f9fe83e48ca6b7 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs @@ -302,10 +302,12 @@ 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 || IsImmutableArrayCompatibleInterface(type)) + // for arrays we concatenate on to what is already there, if we can + // for read-only list-like interfaces, we do the same only if the bindingPoint.Value is null. + bool isImmutableArrayCompatibleInterface = IsImmutableArrayCompatibleInterface(type); + if (type.IsArray || isImmutableArrayCompatibleInterface) { - if (!bindingPoint.IsReadOnly) + if (!bindingPoint.IsReadOnly && (!isImmutableArrayCompatibleInterface || bindingPoint.Value is null)) { bindingPoint.SetValue(BindArray(type, (IEnumerable?)bindingPoint.Value, config, options)); } @@ -317,7 +319,7 @@ 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) { - object? newValue = BindSet(type, (IEnumerable?)bindingPoint.Value, config, options); + object? newValue = BindSet(type, bindingPoint.Value, config, options); if (newValue != null) { bindingPoint.SetValue(newValue); @@ -530,33 +532,26 @@ 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]; + MethodInfo? addMethod = dictionaryType.GetMethod("Add", DeclaredOnlyLookup); + if (addMethod is null && source is not null) + { + // Instantiated IReadOnlyDictionary cannot be mutated by the configuration. + return null; + } - if (orig != null) + if (source is null) { - 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); - } + dictionaryType = typeof(Dictionary<,>).MakeGenericType(keyType, valueType); + source = Activator.CreateInstance(dictionaryType); + addMethod ??= dictionaryType.GetMethod("Add", DeclaredOnlyLookup)!; } - 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. @@ -733,36 +728,37 @@ private static Array BindArray(Type type, IEnumerable? source, IConfiguration co [RequiresDynamicCode(DynamicCodeWarningMessage)] [RequiresUnreferencedCode("Cannot statically analyze what the element type is of the Array so its members may be trimmed.")] - private static object? BindSet(Type type, IEnumerable? source, IConfiguration config, BinderOptions options) + private static object? BindSet(Type type, object? source, IConfiguration config, BinderOptions options) { 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]; + MethodInfo? addMethod = type.GetMethod("Add", DeclaredOnlyLookup); + if (addMethod is null && source is not null) + { + // It is readonly Set interface. We bind the readonly interfaces only when its value is not instantiated. + return null; + } - if (source != null) + if (source is null) { - foreach (object? item in source) - { - arguments[0] = item; - addMethod.Invoke(instance, arguments); - } + Type genericType = typeof(HashSet<>).MakeGenericType(elementType); + source = Activator.CreateInstance(genericType)!; + addMethod ??= genericType.GetMethod("Add", DeclaredOnlyLookup)!; } + Debug.Assert(source is not null); + Debug.Assert(addMethod is not null); + + object?[] arguments = new object?[1]; + foreach (IConfigurationSection section in config.GetChildren()) { var itemBindingPoint = new BindingPoint(); @@ -777,7 +773,7 @@ private static Array BindArray(Type type, IEnumerable? source, IConfiguration co { arguments[0] = itemBindingPoint.Value; - addMethod.Invoke(instance, arguments); + addMethod.Invoke(source, arguments); } } catch (Exception ex) @@ -790,7 +786,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/ConfigurationBinderTests.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationBinderTests.cs index 138d13a650f65d..7f299f0dda83dd 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationBinderTests.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationBinderTests.cs @@ -835,9 +835,8 @@ public void CanBindInstantiatedIReadOnlySet() var options = config.Get()!; - Assert.Equal(2, options.InstantiatedIReadOnlySet.Count); - Assert.Equal("Yo1", options.InstantiatedIReadOnlySet.ElementAt(0)); - Assert.Equal("Yo2", options.InstantiatedIReadOnlySet.ElementAt(1)); + // Instantiated IReadOnlySet cannot get mutated. + Assert.Equal(0, options.InstantiatedIReadOnlySet.Count); } [Fact] @@ -855,11 +854,10 @@ public void CanBindInstantiatedIReadOnlyWithSomeValues() var options = config.Get()!; - Assert.Equal(4, options.InstantiatedIReadOnlySetWithSomeValues.Count); + // Instantiated IReadOnlySet doesn't get mutated. + Assert.Equal(2, options.InstantiatedIReadOnlySetWithSomeValues.Count); Assert.Equal("existing1", options.InstantiatedIReadOnlySetWithSomeValues.ElementAt(0)); Assert.Equal("existing2", options.InstantiatedIReadOnlySetWithSomeValues.ElementAt(1)); - Assert.Equal("Yo1", options.InstantiatedIReadOnlySetWithSomeValues.ElementAt(2)); - Assert.Equal("Yo2", options.InstantiatedIReadOnlySetWithSomeValues.ElementAt(3)); } [Fact] @@ -932,13 +930,10 @@ public void CanBindInstantiatedReadOnlyDictionary2() var options = config.Get()!; - Assert.Equal(4, options.Items.Count); + // Readonly dictionary with instantiated value cannot get mutated by the configuration. + Assert.Equal(2, options.Items.Count); Assert.Equal(1, options.Items["existing-item1"]); Assert.Equal(2, options.Items["existing-item2"]); - Assert.Equal(3, options.Items["item3"]); - Assert.Equal(4, options.Items["item4"]); - - } [Fact] @@ -956,14 +951,14 @@ public void BindInstantiatedIReadOnlyDictionary_CreatesCopyOfOriginal() var options = config.Get()!; - Assert.Equal(3, options.Dictionary.Count); + // Readonly dictionary with instantiated value cannot be mutated by the configuration + Assert.Equal(2, options.Dictionary.Count); // does not overwrite original Assert.Equal(1, ConfigWithInstantiatedIReadOnlyDictionary._existingDictionary["existing-item1"]); - Assert.Equal(666, options.Dictionary["existing-item1"]); + Assert.Equal(1, options.Dictionary["existing-item1"]); Assert.Equal(2, options.Dictionary["existing-item2"]); - Assert.Equal(3, options.Dictionary["item3"]); } [Fact] @@ -1027,11 +1022,11 @@ public void CanBindInstantiatedReadOnlyDictionary() var options = config.Get()!; var resultingDictionary = options.InstantiatedReadOnlyDictionaryWithWithSomeValues; - Assert.Equal(4, resultingDictionary.Count); + + // Readonly dictionary with instantiated value cannot be mutated by the configuration. + Assert.Equal(2, resultingDictionary.Count); Assert.Equal(1, resultingDictionary["existing-item1"]); Assert.Equal(2, resultingDictionary["existing-item2"]); - Assert.Equal(3, resultingDictionary["item3"]); - Assert.Equal(4, resultingDictionary["item4"]); } [Fact] @@ -1376,9 +1371,8 @@ public void CanBindInstantiatedIEnumerableWithItems() var options = config.Get()!; - Assert.Equal(2, options.InstantiatedIEnumerable.Count()); - Assert.Equal("Yo1", options.InstantiatedIEnumerable.ElementAt(0)); - Assert.Equal("Yo2", options.InstantiatedIEnumerable.ElementAt(1)); + // Instantiated readonly IEnumerable which cannot be mutated by the configuration + Assert.Equal(0, options.InstantiatedIEnumerable.Count()); } [Fact] @@ -1456,9 +1450,9 @@ public void CanBindInstantiatedIReadOnlyCollectionWithItems() var options = config.Get()!; - Assert.Equal(2, options.InstantiatedIReadOnlyCollection.Count); - Assert.Equal("Yo1", options.InstantiatedIReadOnlyCollection.ElementAt(0)); - Assert.Equal("Yo2", options.InstantiatedIReadOnlyCollection.ElementAt(1)); + + // Instantiated readonly collection which cannot be mutated by the configuration + Assert.Equal(0, options.InstantiatedIReadOnlyCollection.Count); } [Fact] @@ -1477,9 +1471,8 @@ public void CanBindInstantiatedIEnumerableWithNullItems() var options = config.Get()!; - Assert.Equal(2, options.InstantiatedIEnumerable.Count()); - Assert.Equal("Yo1", options.InstantiatedIEnumerable.ElementAt(0)); - Assert.Equal("Yo2", options.InstantiatedIEnumerable.ElementAt(1)); + // Instantiated IEnumerable is a readonly collection which cannot be mutated by the configuration + Assert.Equal(0, options.InstantiatedIEnumerable.Count()); } [Fact] diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs index b604f3c054d7d2..94b0b2623b0bde 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs @@ -1086,14 +1086,11 @@ public void CanBindInitializedIEnumerableAndTheOriginalItemsAreNotMutated() var array = options.AlreadyInitializedIEnumerableInterface.ToArray(); - Assert.Equal(6, array.Length); + // Instantiated readonly IEnumerable which cannot be mutated by the configuration + Assert.Equal(2, array.Length); Assert.Equal("This was here too", array[0]); Assert.Equal("Don't touch me!", array[1]); - Assert.Equal("val0", array[2]); - Assert.Equal("val1", array[3]); - Assert.Equal("val2", array[4]); - Assert.Equal("valx", array[5]); // the original list hasn't been touched Assert.Equal(2, options.ListUsedInIEnumerableFieldAndShouldNotBeTouched.Count); @@ -1132,12 +1129,11 @@ public void CanBindInitializedCustomIEnumerableBasedList() var array = options.AlreadyInitializedCustomListDerivedFromIEnumerable.ToArray(); - Assert.Equal(4, array.Length); + // Instantiated IEnumerable is a readonly collection which cannot be mutated by the configuration + Assert.Equal(2, array.Length); Assert.Equal("Item1", array[0]); Assert.Equal("Item2", array[1]); - Assert.Equal("val0", array[2]); - Assert.Equal("val1", array[3]); } [Fact] @@ -1162,12 +1158,11 @@ public void CanBindInitializedCustomIndirectlyDerivedIEnumerableList() var array = options.AlreadyInitializedCustomListIndirectlyDerivedFromIEnumerable.ToArray(); - Assert.Equal(4, array.Length); + // Instantiated readonly collection/IEnumerable which cannot be mutated by the configuration + Assert.Equal(2, array.Length); Assert.Equal("Item1", array[0]); Assert.Equal("Item2", array[1]); - Assert.Equal("val0", array[2]); - Assert.Equal("val1", array[3]); } [Fact] @@ -1193,10 +1188,10 @@ public void CanBindInitializedIReadOnlyDictionaryAndDoesNotModifyTheOriginal() Assert.Equal(2, array.Length); - Assert.Equal("overridden!", options.AlreadyInitializedDictionary["existing_key_1"]); + Assert.Equal("val_1", options.AlreadyInitializedDictionary["existing_key_1"]); Assert.Equal("val_2", options.AlreadyInitializedDictionary["existing_key_2"]); - Assert.NotEqual(options.AlreadyInitializedDictionary, InitializedCollectionsOptions.ExistingDictionary); + Assert.Equal(options.AlreadyInitializedDictionary, InitializedCollectionsOptions.ExistingDictionary); Assert.Equal("val_1", InitializedCollectionsOptions.ExistingDictionary["existing_key_1"]); Assert.Equal("val_2", InitializedCollectionsOptions.ExistingDictionary["existing_key_2"]); @@ -1678,12 +1673,164 @@ public class ImplementerOfIDictionaryClass : IDictionary v; set => v = value; } public bool TryGetValue() { return true; } - } 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); + + 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); + + 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 IReadOnlySet UnInstantiatedIReadOnlySet { 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.Equal(2, options.InstantiatedIEnumerable.Count()); + Assert.Equal("value1", options.InstantiatedIEnumerable.ElementAt(0)); + Assert.Equal("value2", options.InstantiatedIEnumerable.ElementAt(1)); + Assert.True(options.IsSameInstantiatedIEnumerable()); + + Assert.Equal(1, options.UnInstantiatedIEnumerable.Count()); + Assert.Equal("value1", options.UnInstantiatedIEnumerable.ElementAt(0)); + + Assert.Equal(3, options.InstantiatedIList.Count()); + 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.Equal(2, options.InstantiatedIReadOnlyList.Count()); + Assert.Equal("value1", options.InstantiatedIReadOnlyList[0]); + Assert.Equal("value2", options.InstantiatedIReadOnlyList[1]); + Assert.True(options.IsSameInstantiatedIReadOnlyList()); + + Assert.Equal(1, options.UnInstantiatedIReadOnlyList.Count()); + Assert.Equal("value", options.UnInstantiatedIReadOnlyList[0]); + + Assert.Equal(3, options.InstantiatedIDictionary.Count()); + 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.Equal(2, options.InstantiatedIReadOnlyDictionary.Count()); + Assert.Equal(new string[] { "Key1", "Key2" }, options.InstantiatedIReadOnlyDictionary.Keys); + Assert.Equal(new string[] { "value1", "value2" }, options.InstantiatedIReadOnlyDictionary.Values); + Assert.True(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.Equal(3, options.InstantiatedISet.Count()); + Assert.Equal(new string[] { "a", "b", "C" }, options.InstantiatedISet); + Assert.True(options.IsSameInstantiatedISet()); + + Assert.Equal(3, options.UnInstantiatedISet.Count()); + Assert.Equal(new string[] { "a", "A", "B" }, options.UnInstantiatedISet); + + Assert.Equal(2, options.InstantiatedIReadOnlySet.Count()); + Assert.Equal(new string[] { "a", "b" }, options.InstantiatedIReadOnlySet); + Assert.True(options.IsSameInstantiatedIReadOnlySet()); + + Assert.Equal(2, options.UnInstantiatedIReadOnlySet.Count()); + Assert.Equal(new string[] { "y", "z" }, options.UnInstantiatedIReadOnlySet); + + 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(3, options.InstantiatedIReadOnlyCollection.Count()); + Assert.Equal(new string[] { "a", "b", "c" }, options.InstantiatedIReadOnlyCollection); + Assert.True(options.IsSameInstantiatedIReadOnlyCollection()); + + Assert.Equal(2, options.UnInstantiatedIReadOnlyCollection.Count()); + Assert.Equal(new string[] { "r", "e" }, options.UnInstantiatedIReadOnlyCollection); + } } } From 149393532a4de9393aa9faa1a282f679fb68a47f Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Mon, 23 Jan 2023 14:34:38 -0800 Subject: [PATCH 2/9] Add Extra test --- .../ConfigurationCollectionBindingTests.cs | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs index 94b0b2623b0bde..df767100d29bc4 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs @@ -1832,5 +1832,25 @@ public void TestOptionsWithDifferentCollectionInterfaces() 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]); + } } } From ea81ebad4f4ee46d53eeabceb2aad746f7ba9069 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Mon, 23 Jan 2023 15:55:06 -0800 Subject: [PATCH 3/9] Fix test build break --- .../tests/ConfigurationCollectionBindingTests.cs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs index df767100d29bc4..e2270731675ca6 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs @@ -1706,10 +1706,13 @@ private class OptionsWithDifferentCollectionInterfaces 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); @@ -1720,7 +1723,6 @@ private class OptionsWithDifferentCollectionInterfaces public IReadOnlyCollection UnInstantiatedIReadOnlyCollection { get; set; } public ICollection UnInstantiatedICollection { get; set; } - public IReadOnlySet UnInstantiatedIReadOnlySet { get; set; } public ISet UnInstantiatedISet { get; set; } public IReadOnlyDictionary UnInstantiatedIReadOnlyDictionary { get; set; } public IEnumerable UnInstantiatedIEnumerable { get; set; } @@ -1811,13 +1813,14 @@ public void TestOptionsWithDifferentCollectionInterfaces() Assert.Equal(3, options.UnInstantiatedISet.Count()); Assert.Equal(new string[] { "a", "A", "B" }, options.UnInstantiatedISet); +#if NETCOREAPP Assert.Equal(2, options.InstantiatedIReadOnlySet.Count()); Assert.Equal(new string[] { "a", "b" }, options.InstantiatedIReadOnlySet); Assert.True(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()); From a7761d583674aad6a266836742b4581cd5562e81 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Tue, 24 Jan 2023 09:28:40 -0800 Subject: [PATCH 4/9] Scoping the fix to settable collections only --- .../src/ConfigurationBinder.cs | 67 ++++++++++++------- .../tests/ConfigurationBinderTests.cs | 41 +++++++----- .../ConfigurationCollectionBindingTests.cs | 51 ++++++++------ 3 files changed, 95 insertions(+), 64 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs index f9fe83e48ca6b7..bb7669a3170efa 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs @@ -302,12 +302,10 @@ private static void BindInstance( if (config != null && config.GetChildren().Any()) { - // for arrays we concatenate on to what is already there, if we can - // for read-only list-like interfaces, we do the same only if the bindingPoint.Value is null. - bool isImmutableArrayCompatibleInterface = IsImmutableArrayCompatibleInterface(type); - if (type.IsArray || isImmutableArrayCompatibleInterface) + // 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 && (!isImmutableArrayCompatibleInterface || bindingPoint.Value is null)) + if (!bindingPoint.IsReadOnly) { bindingPoint.SetValue(BindArray(type, (IEnumerable?)bindingPoint.Value, config, options)); } @@ -533,17 +531,31 @@ private static bool CanBindToTheseConstructorParameters(ParameterInfo[] construc } MethodInfo? addMethod = dictionaryType.GetMethod("Add", DeclaredOnlyLookup); - if (addMethod is null && source is not null) - { - // Instantiated IReadOnlyDictionary cannot be mutated by the configuration. - return null; - } - - if (source is null) + if (addMethod is null || source is null) { dictionaryType = typeof(Dictionary<,>).MakeGenericType(keyType, valueType); - source = Activator.CreateInstance(dictionaryType); - addMethod ??= dictionaryType.GetMethod("Add", DeclaredOnlyLookup)!; + var dictionary = Activator.CreateInstance(dictionaryType); + addMethod = dictionaryType.GetMethod("Add", DeclaredOnlyLookup); + + var orig = source as IEnumerable; + if (orig is not null) + { + 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; } Debug.Assert(source is not null); @@ -740,25 +752,30 @@ private static Array BindArray(Type type, IEnumerable? source, IConfiguration co return null; } + object?[] arguments = new object?[1]; MethodInfo? addMethod = type.GetMethod("Add", DeclaredOnlyLookup); - if (addMethod is null && source is not null) - { - // It is readonly Set interface. We bind the readonly interfaces only when its value is not instantiated. - return null; - } - - if (source is null) + if (addMethod is null || source is null) { Type genericType = typeof(HashSet<>).MakeGenericType(elementType); - source = Activator.CreateInstance(genericType)!; - addMethod ??= genericType.GetMethod("Add", DeclaredOnlyLookup)!; + object instance = Activator.CreateInstance(genericType)!; + addMethod = genericType.GetMethod("Add", DeclaredOnlyLookup); + + var orig = source as IEnumerable; + if (orig != null) + { + foreach (object? item in orig) + { + arguments[0] = item; + addMethod!.Invoke(instance, arguments); + } + } + + source = instance; } Debug.Assert(source is not null); Debug.Assert(addMethod is not null); - object?[] arguments = new object?[1]; - foreach (IConfigurationSection section in config.GetChildren()) { var itemBindingPoint = new BindingPoint(); diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationBinderTests.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationBinderTests.cs index 7f299f0dda83dd..9020ef3a85be05 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationBinderTests.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationBinderTests.cs @@ -835,8 +835,9 @@ public void CanBindInstantiatedIReadOnlySet() var options = config.Get()!; - // Instantiated IReadOnlySet cannot get mutated. - Assert.Equal(0, options.InstantiatedIReadOnlySet.Count); + Assert.Equal(2, options.InstantiatedIReadOnlySet.Count); + Assert.Equal("Yo1", options.InstantiatedIReadOnlySet.ElementAt(0)); + Assert.Equal("Yo2", options.InstantiatedIReadOnlySet.ElementAt(1)); } [Fact] @@ -854,10 +855,11 @@ public void CanBindInstantiatedIReadOnlyWithSomeValues() var options = config.Get()!; - // Instantiated IReadOnlySet doesn't get mutated. - Assert.Equal(2, options.InstantiatedIReadOnlySetWithSomeValues.Count); + Assert.Equal(4, options.InstantiatedIReadOnlySetWithSomeValues.Count); Assert.Equal("existing1", options.InstantiatedIReadOnlySetWithSomeValues.ElementAt(0)); Assert.Equal("existing2", options.InstantiatedIReadOnlySetWithSomeValues.ElementAt(1)); + Assert.Equal("Yo1", options.InstantiatedIReadOnlySetWithSomeValues.ElementAt(2)); + Assert.Equal("Yo2", options.InstantiatedIReadOnlySetWithSomeValues.ElementAt(3)); } [Fact] @@ -930,10 +932,11 @@ public void CanBindInstantiatedReadOnlyDictionary2() var options = config.Get()!; - // Readonly dictionary with instantiated value cannot get mutated by the configuration. - Assert.Equal(2, options.Items.Count); + Assert.Equal(4, options.Items.Count); Assert.Equal(1, options.Items["existing-item1"]); Assert.Equal(2, options.Items["existing-item2"]); + Assert.Equal(3, options.Items["item3"]); + Assert.Equal(4, options.Items["item4"]); } [Fact] @@ -952,13 +955,14 @@ public void BindInstantiatedIReadOnlyDictionary_CreatesCopyOfOriginal() var options = config.Get()!; // Readonly dictionary with instantiated value cannot be mutated by the configuration - Assert.Equal(2, options.Dictionary.Count); + Assert.Equal(3, options.Dictionary.Count); // does not overwrite original Assert.Equal(1, ConfigWithInstantiatedIReadOnlyDictionary._existingDictionary["existing-item1"]); - Assert.Equal(1, options.Dictionary["existing-item1"]); + Assert.Equal(666, options.Dictionary["existing-item1"]); Assert.Equal(2, options.Dictionary["existing-item2"]); + Assert.Equal(3, options.Dictionary["item3"]); } [Fact] @@ -1023,10 +1027,11 @@ public void CanBindInstantiatedReadOnlyDictionary() var resultingDictionary = options.InstantiatedReadOnlyDictionaryWithWithSomeValues; - // Readonly dictionary with instantiated value cannot be mutated by the configuration. - Assert.Equal(2, resultingDictionary.Count); + Assert.Equal(4, resultingDictionary.Count); Assert.Equal(1, resultingDictionary["existing-item1"]); Assert.Equal(2, resultingDictionary["existing-item2"]); + Assert.Equal(3, resultingDictionary["item3"]); + Assert.Equal(4, resultingDictionary["item4"]); } [Fact] @@ -1371,8 +1376,9 @@ public void CanBindInstantiatedIEnumerableWithItems() var options = config.Get()!; - // Instantiated readonly IEnumerable which cannot be mutated by the configuration - Assert.Equal(0, options.InstantiatedIEnumerable.Count()); + Assert.Equal(2, options.InstantiatedIEnumerable.Count()); + Assert.Equal("Yo1", options.InstantiatedIEnumerable.ElementAt(0)); + Assert.Equal("Yo2", options.InstantiatedIEnumerable.ElementAt(1)); } [Fact] @@ -1450,9 +1456,9 @@ public void CanBindInstantiatedIReadOnlyCollectionWithItems() var options = config.Get()!; - - // Instantiated readonly collection which cannot be mutated by the configuration - Assert.Equal(0, options.InstantiatedIReadOnlyCollection.Count); + Assert.Equal(2, options.InstantiatedIReadOnlyCollection.Count); + Assert.Equal("Yo1", options.InstantiatedIReadOnlyCollection.ElementAt(0)); + Assert.Equal("Yo2", options.InstantiatedIReadOnlyCollection.ElementAt(1)); } [Fact] @@ -1471,8 +1477,9 @@ public void CanBindInstantiatedIEnumerableWithNullItems() var options = config.Get()!; - // Instantiated IEnumerable is a readonly collection which cannot be mutated by the configuration - Assert.Equal(0, options.InstantiatedIEnumerable.Count()); + Assert.Equal(2, options.InstantiatedIEnumerable.Count()); + Assert.Equal("Yo1", options.InstantiatedIEnumerable.ElementAt(0)); + Assert.Equal("Yo2", options.InstantiatedIEnumerable.ElementAt(1)); } [Fact] diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs index e2270731675ca6..eb9bb6ed481be3 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs @@ -1086,11 +1086,14 @@ public void CanBindInitializedIEnumerableAndTheOriginalItemsAreNotMutated() var array = options.AlreadyInitializedIEnumerableInterface.ToArray(); - // Instantiated readonly IEnumerable which cannot be mutated by the configuration - Assert.Equal(2, array.Length); + Assert.Equal(6, array.Length); Assert.Equal("This was here too", array[0]); Assert.Equal("Don't touch me!", array[1]); + Assert.Equal("val0", array[2]); + Assert.Equal("val1", array[3]); + Assert.Equal("val2", array[4]); + Assert.Equal("valx", array[5]); // the original list hasn't been touched Assert.Equal(2, options.ListUsedInIEnumerableFieldAndShouldNotBeTouched.Count); @@ -1129,11 +1132,12 @@ public void CanBindInitializedCustomIEnumerableBasedList() var array = options.AlreadyInitializedCustomListDerivedFromIEnumerable.ToArray(); - // Instantiated IEnumerable is a readonly collection which cannot be mutated by the configuration - Assert.Equal(2, array.Length); + Assert.Equal(4, array.Length); Assert.Equal("Item1", array[0]); Assert.Equal("Item2", array[1]); + Assert.Equal("val0", array[2]); + Assert.Equal("val1", array[3]); } [Fact] @@ -1158,11 +1162,12 @@ public void CanBindInitializedCustomIndirectlyDerivedIEnumerableList() var array = options.AlreadyInitializedCustomListIndirectlyDerivedFromIEnumerable.ToArray(); - // Instantiated readonly collection/IEnumerable which cannot be mutated by the configuration - Assert.Equal(2, array.Length); + Assert.Equal(4, array.Length); Assert.Equal("Item1", array[0]); Assert.Equal("Item2", array[1]); + Assert.Equal("val0", array[2]); + Assert.Equal("val1", array[3]); } [Fact] @@ -1188,10 +1193,10 @@ public void CanBindInitializedIReadOnlyDictionaryAndDoesNotModifyTheOriginal() Assert.Equal(2, array.Length); - Assert.Equal("val_1", options.AlreadyInitializedDictionary["existing_key_1"]); + Assert.Equal("overridden!", options.AlreadyInitializedDictionary["existing_key_1"]); Assert.Equal("val_2", options.AlreadyInitializedDictionary["existing_key_2"]); - Assert.Equal(options.AlreadyInitializedDictionary, InitializedCollectionsOptions.ExistingDictionary); + Assert.NotEqual(options.AlreadyInitializedDictionary, InitializedCollectionsOptions.ExistingDictionary); Assert.Equal("val_1", InitializedCollectionsOptions.ExistingDictionary["existing_key_1"]); Assert.Equal("val_2", InitializedCollectionsOptions.ExistingDictionary["existing_key_2"]); @@ -1767,10 +1772,11 @@ public void TestOptionsWithDifferentCollectionInterfaces() var options = new OptionsWithDifferentCollectionInterfaces(); config.Bind(options); - Assert.Equal(2, options.InstantiatedIEnumerable.Count()); + Assert.Equal(3, options.InstantiatedIEnumerable.Count()); Assert.Equal("value1", options.InstantiatedIEnumerable.ElementAt(0)); Assert.Equal("value2", options.InstantiatedIEnumerable.ElementAt(1)); - Assert.True(options.IsSameInstantiatedIEnumerable()); + Assert.Equal("value3", options.InstantiatedIEnumerable.ElementAt(2)); + Assert.False(options.IsSameInstantiatedIEnumerable()); Assert.Equal(1, options.UnInstantiatedIEnumerable.Count()); Assert.Equal("value1", options.UnInstantiatedIEnumerable.ElementAt(0)); @@ -1784,10 +1790,11 @@ public void TestOptionsWithDifferentCollectionInterfaces() Assert.Equal(1, options.UnInstantiatedIList.Count()); Assert.Equal("value", options.UnInstantiatedIList[0]); - Assert.Equal(2, options.InstantiatedIReadOnlyList.Count()); + Assert.Equal(3, options.InstantiatedIReadOnlyList.Count()); Assert.Equal("value1", options.InstantiatedIReadOnlyList[0]); Assert.Equal("value2", options.InstantiatedIReadOnlyList[1]); - Assert.True(options.IsSameInstantiatedIReadOnlyList()); + Assert.Equal("value3", options.InstantiatedIReadOnlyList[2]); + Assert.False(options.IsSameInstantiatedIReadOnlyList()); Assert.Equal(1, options.UnInstantiatedIReadOnlyList.Count()); Assert.Equal("value", options.UnInstantiatedIReadOnlyList[0]); @@ -1797,10 +1804,10 @@ public void TestOptionsWithDifferentCollectionInterfaces() Assert.Equal(new string[] { "value1", "value2", "value3" }, options.InstantiatedIDictionary.Values); Assert.True(options.IsSameInstantiatedIDictionary()); - Assert.Equal(2, options.InstantiatedIReadOnlyDictionary.Count()); - Assert.Equal(new string[] { "Key1", "Key2" }, options.InstantiatedIReadOnlyDictionary.Keys); - Assert.Equal(new string[] { "value1", "value2" }, options.InstantiatedIReadOnlyDictionary.Values); - Assert.True(options.IsSameInstantiatedIReadOnlyDictionary()); + Assert.Equal(3, options.InstantiatedIReadOnlyDictionary.Count()); + 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); @@ -1814,9 +1821,9 @@ public void TestOptionsWithDifferentCollectionInterfaces() Assert.Equal(new string[] { "a", "A", "B" }, options.UnInstantiatedISet); #if NETCOREAPP - Assert.Equal(2, options.InstantiatedIReadOnlySet.Count()); - Assert.Equal(new string[] { "a", "b" }, options.InstantiatedIReadOnlySet); - Assert.True(options.IsSameInstantiatedIReadOnlySet()); + Assert.Equal(3, options.InstantiatedIReadOnlySet.Count()); + 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); @@ -1828,9 +1835,9 @@ public void TestOptionsWithDifferentCollectionInterfaces() Assert.Equal(2, options.UnInstantiatedICollection.Count()); Assert.Equal(new string[] { "t", "a" }, options.UnInstantiatedICollection); - Assert.Equal(3, options.InstantiatedIReadOnlyCollection.Count()); - Assert.Equal(new string[] { "a", "b", "c" }, options.InstantiatedIReadOnlyCollection); - Assert.True(options.IsSameInstantiatedIReadOnlyCollection()); + 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); From 36c25caa1a1b9b24b9775e359004992b5e01b5d1 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Tue, 24 Jan 2023 15:55:05 -0800 Subject: [PATCH 5/9] Address the feedback --- .../src/ConfigurationBinder.cs | 22 +++++++++++-------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs index bb7669a3170efa..bbe7e4dc22f99f 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, 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; @@ -530,6 +533,7 @@ private static bool CanBindToTheseConstructorParameters(ParameterInfo[] construc return 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) { @@ -740,7 +744,7 @@ private static Array BindArray(Type type, IEnumerable? source, IConfiguration co [RequiresDynamicCode(DynamicCodeWarningMessage)] [RequiresUnreferencedCode("Cannot statically analyze what the element type is of the Array so its members may be trimmed.")] - private static object? BindSet(Type type, object? source, IConfiguration config, BinderOptions options) + private static object? BindSet(Type type, IEnumerable? source, IConfiguration config, BinderOptions options) { Type elementType = type.GetGenericArguments()[0]; @@ -753,6 +757,7 @@ private static Array BindArray(Type type, IEnumerable? source, IConfiguration co } object?[] arguments = new object?[1]; + // 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) { @@ -760,17 +765,16 @@ private static Array BindArray(Type type, IEnumerable? source, IConfiguration co object instance = Activator.CreateInstance(genericType)!; addMethod = genericType.GetMethod("Add", DeclaredOnlyLookup); - var orig = source as IEnumerable; - if (orig != null) + if (source != null) { - foreach (object? item in orig) + foreach (object? item in source) { arguments[0] = item; addMethod!.Invoke(instance, arguments); } } - source = instance; + source = (IEnumerable)instance; } Debug.Assert(source is not null); From 438a77ca82571a860080ba4cf4643b959b458edf Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Tue, 24 Jan 2023 16:23:10 -0800 Subject: [PATCH 6/9] Add log messages to the test to get more info about CI failure --- .../tests/ConfigurationCollectionBindingTests.cs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs index eb9bb6ed481be3..a87a49c6ea8e92 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs @@ -1772,7 +1772,7 @@ public void TestOptionsWithDifferentCollectionInterfaces() var options = new OptionsWithDifferentCollectionInterfaces(); config.Bind(options); - Assert.Equal(3, options.InstantiatedIEnumerable.Count()); + 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)); @@ -1781,7 +1781,7 @@ public void TestOptionsWithDifferentCollectionInterfaces() Assert.Equal(1, options.UnInstantiatedIEnumerable.Count()); Assert.Equal("value1", options.UnInstantiatedIEnumerable.ElementAt(0)); - Assert.Equal(3, options.InstantiatedIList.Count()); + 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]); @@ -1790,7 +1790,7 @@ public void TestOptionsWithDifferentCollectionInterfaces() Assert.Equal(1, options.UnInstantiatedIList.Count()); Assert.Equal("value", options.UnInstantiatedIList[0]); - Assert.Equal(3, options.InstantiatedIReadOnlyList.Count()); + 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]); @@ -1799,12 +1799,12 @@ public void TestOptionsWithDifferentCollectionInterfaces() Assert.Equal(1, options.UnInstantiatedIReadOnlyList.Count()); Assert.Equal("value", options.UnInstantiatedIReadOnlyList[0]); - Assert.Equal(3, options.InstantiatedIDictionary.Count()); + 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.Equal(3, options.InstantiatedIReadOnlyDictionary.Count()); + 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()); @@ -1813,15 +1813,15 @@ public void TestOptionsWithDifferentCollectionInterfaces() Assert.Equal(new string[] { "Key" }, options.UnInstantiatedIReadOnlyDictionary.Keys); Assert.Equal(new string[] { "value" }, options.UnInstantiatedIReadOnlyDictionary.Values); - Assert.Equal(3, options.InstantiatedISet.Count()); + Assert.True(3 == options.InstantiatedISet.Count(), $"InstantiatedISet count is {options.InstantiatedISet.Count()} .. {options.InstantiatedISet.ElementAt(options.InstantiatedISet.Count() - 1)}"); Assert.Equal(new string[] { "a", "b", "C" }, options.InstantiatedISet); Assert.True(options.IsSameInstantiatedISet()); - Assert.Equal(3, options.UnInstantiatedISet.Count()); + 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.Equal(3, options.InstantiatedIReadOnlySet.Count()); + 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()); From b8c73f960b6c590083b410c35e07438df3940a12 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Tue, 24 Jan 2023 18:32:43 -0800 Subject: [PATCH 7/9] Add logs to test --- .../tests/ConfigurationBinderTests.cs | 2 +- .../tests/ConfigurationCollectionBindingTests.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationBinderTests.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationBinderTests.cs index 9020ef3a85be05..0242fda8515361 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationBinderTests.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationBinderTests.cs @@ -812,7 +812,7 @@ public void CanBindISetNoSetter() var config = configurationBuilder.Build(); - var options = config.Get()!; + var options = config.Get(o => o.ErrorOnUnknownConfiguration = true)!; Assert.Equal(2, options.ISetNoSetter.Count); Assert.Equal("Yo1", options.ISetNoSetter.ElementAt(0)); diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs index a87a49c6ea8e92..141e1dd3d077b5 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs @@ -1813,7 +1813,7 @@ public void TestOptionsWithDifferentCollectionInterfaces() 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()} .. {options.InstantiatedISet.ElementAt(options.InstantiatedISet.Count() - 1)}"); + 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()); From 4f7a43c4b5897d1f81966ff9da9751fc7aa7139d Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Wed, 25 Jan 2023 12:14:03 -0800 Subject: [PATCH 8/9] Exclude ISET from the test trimming --- .../tests/ILLink.Descriptors.xml | 1 + 1 file changed, 1 insertion(+) 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..1c6fab270e35c7 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ILLink.Descriptors.xml +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ILLink.Descriptors.xml @@ -14,5 +14,6 @@ + From c047d04edc9e998e7a7f0918e5a706747511272a Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Wed, 25 Jan 2023 16:26:20 -0800 Subject: [PATCH 9/9] Fix the trimming file to point at correct assembly --- .../tests/ILLink.Descriptors.xml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) 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 1c6fab270e35c7..adbcf5f7d26e99 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ILLink.Descriptors.xml +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ILLink.Descriptors.xml @@ -14,6 +14,9 @@ - + + +   +