From 59caa55042475fcb04cdfc8b54819d82b2b80178 Mon Sep 17 00:00:00 2001 From: Eric Erhardt Date: Thu, 15 Sep 2022 16:33:12 +0000 Subject: [PATCH] Fix Configuration.Binder for collection properties with no setters Binding to an IDictionary/ICollection/ISet in ConfigurationBinder with no setter was failing because we were returning too early. Only returning early now if we were able to set the property, or if the interface is read-only. Fix #75626 --- .../src/ConfigurationBinder.cs | 46 ++++++++++++------- .../tests/ConfigurationBinderTests.cs | 23 ++++++++++ .../ConfigurationCollectionBindingTests.cs | 22 ++++++++- 3 files changed, 72 insertions(+), 19 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs index a26b2fe68e7190..e36ae28fd4316d 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs @@ -299,43 +299,45 @@ 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 + // 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)) { 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 (TypeIsASetInterface(type)) + // 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 (!bindingPoint.IsReadOnly) + object? newValue = BindSet(type, (IEnumerable?)bindingPoint.Value, config, options); + if (newValue != null) { - object? newValue = BindSet(type, (IEnumerable?)bindingPoint.Value, config, options); - if (newValue != null) - { - bindingPoint.SetValue(newValue); - } + bindingPoint.SetValue(newValue); } + return; } // For other mutable interfaces like ICollection<>, IDictionary<,> 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. - if (TypeIsADictionaryInterface(type)) + if (TypeIsADictionaryInterface(type) && !bindingPoint.IsReadOnly) { - if (!bindingPoint.IsReadOnly) + object? newValue = BindDictionaryInterface(bindingPoint.Value, type, config, options); + if (newValue != null) { - object? newValue = BindDictionaryInterface(bindingPoint.Value, type, config, options); - if (newValue != null) - { - bindingPoint.SetValue(newValue); - } + bindingPoint.SetValue(newValue); } + return; } @@ -848,6 +850,16 @@ private static bool IsArrayCompatibleInterface(Type type) || genericTypeDefinition == typeof(IReadOnlyList<>); } + private static bool IsImmutableArrayCompatibleInterface(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 TypeIsASetInterface(Type type) { if (!type.IsInterface || !type.IsConstructedGenericType) { return false; } diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationBinderTests.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationBinderTests.cs index 9eae194f02cf99..521501d5938de3 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationBinderTests.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationBinderTests.cs @@ -72,6 +72,8 @@ public string ReadOnly public ISet InstantiatedISet { get; set; } = new HashSet(); + public ISet ISetNoSetter { get; } = new HashSet(); + public HashSet InstantiatedHashSetWithSomeValues { get; set; } = new HashSet(new[] {"existing1", "existing2"}); @@ -662,6 +664,27 @@ public void CanBindNonInstantiatedISet() Assert.Equal("Yo2", options.NonInstantiatedISet.ElementAt(1)); } + [Fact] + public void CanBindISetNoSetter() + { + var dic = new Dictionary + { + {"ISetNoSetter:0", "Yo1"}, + {"ISetNoSetter:1", "Yo2"}, + {"ISetNoSetter:2", "Yo2"}, + }; + var configurationBuilder = new ConfigurationBuilder(); + configurationBuilder.AddInMemoryCollection(dic); + + var config = configurationBuilder.Build(); + + var options = config.Get()!; + + Assert.Equal(2, options.ISetNoSetter.Count); + Assert.Equal("Yo1", options.ISetNoSetter.ElementAt(0)); + Assert.Equal("Yo2", options.ISetNoSetter.ElementAt(1)); + } + #if NETCOREAPP [Fact] public void CanBindInstantiatedIReadOnlySet() diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs index 7367d0664cf356..4f2b5911b2b7a9 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs @@ -579,7 +579,10 @@ public void AlreadyInitializedStringDictionaryBinding() { {"AlreadyInitializedStringDictionaryInterface:abc", "val_1"}, {"AlreadyInitializedStringDictionaryInterface:def", "val_2"}, - {"AlreadyInitializedStringDictionaryInterface:ghi", "val_3"} + {"AlreadyInitializedStringDictionaryInterface:ghi", "val_3"}, + + {"IDictionaryNoSetter:Key1", "Value1"}, + {"IDictionaryNoSetter:Key2", "Value2"}, }; var configurationBuilder = new ConfigurationBuilder(); @@ -596,6 +599,10 @@ public void AlreadyInitializedStringDictionaryBinding() Assert.Equal("val_1", options.AlreadyInitializedStringDictionaryInterface["abc"]); Assert.Equal("val_2", options.AlreadyInitializedStringDictionaryInterface["def"]); Assert.Equal("val_3", options.AlreadyInitializedStringDictionaryInterface["ghi"]); + + Assert.Equal(2, options.IDictionaryNoSetter.Count); + Assert.Equal("Value1", options.IDictionaryNoSetter["Key1"]); + Assert.Equal("Value2", options.IDictionaryNoSetter["Key2"]); } [Fact] @@ -1059,7 +1066,10 @@ public void CanBindInitializedIEnumerableAndTheOriginalItemsAreNotMutated() {"AlreadyInitializedIEnumerableInterface:0", "val0"}, {"AlreadyInitializedIEnumerableInterface:1", "val1"}, {"AlreadyInitializedIEnumerableInterface:2", "val2"}, - {"AlreadyInitializedIEnumerableInterface:x", "valx"} + {"AlreadyInitializedIEnumerableInterface:x", "valx"}, + + {"ICollectionNoSetter:0", "val0"}, + {"ICollectionNoSetter:1", "val1"}, }; var configurationBuilder = new ConfigurationBuilder(); @@ -1084,6 +1094,10 @@ public void CanBindInitializedIEnumerableAndTheOriginalItemsAreNotMutated() Assert.Equal(2, options.ListUsedInIEnumerableFieldAndShouldNotBeTouched.Count); Assert.Equal("This was here too", options.ListUsedInIEnumerableFieldAndShouldNotBeTouched.ElementAt(0)); Assert.Equal("Don't touch me!", options.ListUsedInIEnumerableFieldAndShouldNotBeTouched.ElementAt(1)); + + Assert.Equal(2, options.ICollectionNoSetter.Count); + Assert.Equal("val0", options.ICollectionNoSetter.ElementAt(0)); + Assert.Equal("val1", options.ICollectionNoSetter.ElementAt(1)); } [Fact] @@ -1424,6 +1438,8 @@ public InitializedCollectionsOptions() new CustomListIndirectlyDerivedFromIEnumerable(); public IReadOnlyDictionary AlreadyInitializedDictionary { get; set; } + + public ICollection ICollectionNoSetter { get; } = new List(); } private class CustomList : List @@ -1564,6 +1580,8 @@ public OptionsWithDictionary() public Dictionary StringDictionary { get; set; } + public IDictionary IDictionaryNoSetter { get; } = new Dictionary(); + public Dictionary ObjectDictionary { get; set; } public Dictionary> ISetDictionary { get; set; }