Fix Configuration.Binder for collection properties with no setters (#75723)

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

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>

Author: github-actions[bot]

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; }

src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationBinderTests.cs

@@ -72,6 +72,8 @@ public string ReadOnly
 
             public ISet<string> InstantiatedISet { get; set; } = new HashSet<string>();
 
+            public ISet<string> ISetNoSetter { get; } = new HashSet<string>();
+
             public HashSet<string> InstantiatedHashSetWithSomeValues { get; set; } =
                 new HashSet<string>(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<string, string>
+            {
+                {"ISetNoSetter:0", "Yo1"},
+                {"ISetNoSetter:1", "Yo2"},
+                {"ISetNoSetter:2", "Yo2"},
+            };
+            var configurationBuilder = new ConfigurationBuilder();
+            configurationBuilder.AddInMemoryCollection(dic);
+
+            var config = configurationBuilder.Build();
+
+            var options = config.Get<ComplexOptions>()!;
+
+            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()

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<string, string> AlreadyInitializedDictionary { get; set; }
+
+            public ICollection<string> ICollectionNoSetter { get; } = new List<string>();
         }
 
         private class CustomList : List<string>
@@ -1564,6 +1580,8 @@ public OptionsWithDictionary()
 
             public Dictionary<string, string> StringDictionary { get; set; }
 
+            public IDictionary<string, string> IDictionaryNoSetter { get; } = new Dictionary<string, string>();
+
             public Dictionary<string, NestedOptions> ObjectDictionary { get; set; }
 
             public Dictionary<string, ISet<string>> ISetDictionary { get; set; }