Enable binding to non-null IEnumerable properties.

[SteveDunn]

Binding should work for an IEnumerable property instantiated with a collection type.
In the case of IEnumerable, rather than just checking the type of the property,
we also now check the instance type to see if it's ICollection based in order
to populate it.

Fix #36390

[ghost]

Tagging subscribers to this area: @maryamariyan, @safern
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Binding should work for an IEnumerable property instantiated with a collection type.
In the case of IEnumerable, rather than just checking the type of the property,
we also now check the instance type to see if it's ICollection based in order
to populate it.

Fix #36390

Author:	SteveDunn
Assignees:	-
Labels:	area-Extensions-Configuration
Milestone:	-

[dnfadmin]

All CLA requirements met.

[halter73]

I thought the idea was to actually create a new List<T> and call the the IEnumerable<T> setter with that.

It looks like this is effectively as-casting by checking if the IEnumerable<T> was really initialized with an object implementing ICollection<T> and adding the items to the existing object.

I do not think we should be mutating objects declared to be IEnumerable<T> .

[SteveDunn]

Hi @halter73 . Thank you for the feedback!

I thought the idea was to actually create a new List and call the the IEnumerable setter with that.

My interpretation of the original issue is that if the type of the property is an IEnumerable<T> and is non-null, and is a mutable collection (i.e. has an Add method, à la ICollection<T>), then it should be set. This is from the issue:

When using the binding feature in the config system, for IEnumerable collections that have a default, non-null value, the collection is never assigned into the target model...

Re. mutating:

I do not think we should be mutating objects declared to be IEnumerable .

Objectively, In this case, we're not mutating the object per se, rather, we're checking the runtime type to see if it's a mutable collection, and using what's already there; the Add method.
But subjectively, this is clearly debatable. From reading the comments on the issue (assuming my interpretation is correct), this has previously been debated.

This is my first contribution to this repo, so I wanted to be sure I was fixing the right thing in the right place. To ensure the right thing aspect, I created a separate stand-alone exe and consumed the new DLL containing my fix. I used the exact DTO and config from the issue itself. But as I say, this is my first contribution, and it is entirely possible that I have misinterpreted the original issue.

[halter73]

Looking at the issue, it's not very clear. I don't want to speak for everyone, but I thought the idea was to have ConfigurationBinder treat settable properties declared to be IEnumerable<T> similarly to T[]. That is to say that if the property already has an initial IEnumerable<T> value, ConfigurationBinder would allocate a new List<T> copy over the items from the initial IEnumerable<T> to the new List<T> and then copy over the config items afterwards.

This would be very similar to BindArray:

runtime/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs

Lines 449 to 482 in 0ee10e9

	private static Array BindArray(Array source, IConfiguration config, BinderOptions options)
	{
	IConfigurationSection[] children = config.GetChildren().ToArray();
	int arrayLength = source.Length;
	Type elementType = source.GetType().GetElementType();
	var newArray = Array.CreateInstance(elementType, arrayLength + children.Length);
	// binding to array has to preserve already initialized arrays with values
	if (arrayLength > 0)
	{
	Array.Copy(source, newArray, arrayLength);
	}
	for (int i = 0; i < children.Length; i++)
	{
	try
	{
	object item = BindInstance(
	type: elementType,
	instance: null,
	config: children[i],
	options: options);
	if (item != null)
	{
	newArray.SetValue(item, arrayLength + i);
	}
	}
	catch
	{
	}
	}
	return newArray;
	}

I think that's what we agreed on. Is that right @maryamariyan @ericstj @safern @davidfowl?

[safern]

System.Text.Json and most of the deserializers out there overwrite the collection on the POCO, so IMHO, I think we should be consistent with what other deserializers do. However we at the moment append to a non-null (for Collections, for Array we overwrite) and we had a feature request to support this: #46988.
So I think that would be a breaking change, but I think it is worth making it?

Maybe we could leave the append behavior behind a flag?

[SteveDunn]

I think this comment below relates to issue #46988; that issue and the issue that this PR addresses are intertwined so of course the comment is perfectly apposite:

That is to say that if the property already has an initial IEnumerable value, ConfigurationBinder would allocate a new List copy over the items from the initial IEnumerable to the new List and then copy over the config items afterwards.

I don't think that'd work as we can't assume that a property declared as IEnumerable<T> could be set with a List<T>. Take for example this scenario:

    public class Config {
        private IEnumerable<string> _data = new FunkyList {"item1"};
        
        public IEnumerable<string> Data {
            get => _data;
            set => _data = value;
        }
    }

    class FunkyList : IList<string>  {
        private readonly List<string> _list = new();
        
        public void Add(string item) => _list.Add($"YO {item}");

        // .. other implementing methods
    }

(yes, the setter for Data does allow another collection type, but I'd be miffed if something changed it after I'd explicitly set it, and it could potentially break anything that depended on the behaviour of the funky list)

As mentioned above re. two similar issues: there does appear to be some disparity between them. Paraphrasing from #46988: 'if someone provides a list with some items, we should overwrite those items'. The issue fixed here is 'if someone provides an IEnumerable property which has a runtime type that is a mutable collection, then populate that collection'

These two issues certainly seem to be at odds with each other and seem to be mutually exclusive; we can't fix one without breaking the other.

Just my $0.02: a corollary from the juxtaposition of items #36390 and #46988 is the concept of augmentation. Augmentation goes beyond basic binding of types (IMO). I want a type which contains data which is set at runtime (like in the constructor), but I don't want a straight 'blat whatever I set at runtime with whatever is in cofig', Instead, I'd like to augment with static data from config. #46988 touches on this by mentioning a flag specific to lists, but I don't think it goes far enough. Perhaps we need something like:

public enum AugmentationChoices {
    OverrideListsThatHaveBeenSetByCode,
    OverridePropertiesThatHaveBeenSetByCode,
    AskMeForMyChoiceForEachProperty // user provides a callback with a property name or PropertyInfo
}

Anyway, that was just an idea as the term 'binding' seems rather vague (or more kindly, incomplete) given the complexities and choices that are highlighted by these two issues.

I suppose we are left needing to answer a few questions:

• Is the issue that this PR fixes (Binding to non-null IEnumerable doesn't work #36390) really the right issue to fix?
• Should the issue that suggests overwriting collections (Microsoft.Extensions.Configuration App binding adding to the initialized Collections, not replacing  #46988 ) be re-opened?
• Should I have picked another issue as my first contribution? :)

[SteveDunn]

System.Text.Json and most of the deserializers out there overwrite the collection on the POCO, so IMHO, I think we should be consistent with what other deserializers do. However we at the moment append to a non-null (for Collections, for Array we overwrite) and we had a feature request to support this: #46988.
So I think that would be a breaking change, but I think it is worth making it?

Maybe we could leave the append behavior behind a flag?

Maybe something to cogitate is what this library offers. The mention of deserializers and their mostly common behaviours could hint at a leaky abstraction. From a users perspective, I provide the library some json and a Type and I'd like the library to populate that type with the contents of the json. I think the word populate here is ambiguous though as it could mean 'overwrite' or could mean (as mentioned in my comment above) 'augment'.

From an API design perspective, the entry point is via this:
var val = config.GetSection("config").Get<Config>();

Perhaps, instead of Get we had either DeserializeTo or AugmentTo?

[safern]

Those are really valid points. Thanks, @SteveDunn for your thoughts. I think we should focus on fixing the real issue here which is being able to bind to non-null empty collection.

Then if there is real need to change the behavior and to override always, we could reopen: #46988 and build a real scenario to bring it back to discussion with the API Review crew. @halter73 does that make sense? It seems like we keep looping on what we should do, I think we should keep it "simple" now and fix the reported issue that is affecting customers.

[halter73]

I think we should focus on fixing the real issue here which is being able to bind to non-null empty collection.

What's the real issue? Does fixing the real issue require mutating an object that's declared to be IEnumerable? I think that's a change for the worse not the better.

I don't see what makes empty settable IEnumerables special, especially when empty arrays and empty ICollections are not. I'm fine taking this back to API review because it is a behavioral change. I thought what I suggested was what we agreed on in API review last time, but that clearly wasn't recorded.

I would argue that my suggested behavioral change is far less surprising and more consistent than the current behavior and could be treated as a simple bug fix. I'd be surprised to find code relying on settable IEnumerables not being set by config. This appears to be mutating non-settable IEnumerables which seems to me much more likely to be breaking.

[SteveDunn]

What's the real issue? Does fixing the real issue require mutating an object that's declared to be IEnumerable? I think that's a change for the worse not the better.

The type is IEnumerable, but the object that the user provides is a mutable collection. That distinction is relevant as this is the convention used by binding when it comes across a null enumerable (it mutates it to a list).

I don't see what makes empty settable IEnumerables special, especially when empty arrays and empty ICollections are not.

I think this is a really important observation and really describes the issue! Being new to the party, I was on the fence about this issue and picked it up because it looked fairly easy and would be a nice entry point to what I hope will be many contributions. But by asking asking that question, I now believe that fixing this issue is the correct approach:

Why are empty enumerables special, but empty arrays and collections are not special...
Below are two examples, both, in the current implementation (net5.0): the first one showing that nothing is special and everything is consistent; the second one showing that empty enumerables are special and things are not consistent.
With this config:

{
    "config": {
      "theArray": [ "a", "b" ],
      "theCollection": [ "a", "b" ],
      "theEnumerable": [ "a", "b" ]
    }
}

... and this POCO:

public class Config
{
    public string[] TheArray { get; set; }
    public ICollection<string> TheCollection { get; set; }
    public IEnumerable<string> TheEnumerable { get; set; }
}

.. running this (again, this is net5.0) produces:

System.String[]: 2
a
b

System.Collections.Generic.List`1[System.String]: 2
a
b

System.Collections.Generic.List`1[System.String]: 2
a
b

Empty arrays, empty collections, and empty enumerables are not special. So far so good, we're consistent.

But given this: POCO

public class Config
{
    public string[] TheArray { get; set; } = Array.Empty<string>();
    public ICollection<string> TheCollection { get; set; } = new List<string>();
    public IEnumerable<string> TheEnumerable { get; set; } = new List<string>();
}

... we get:

System.String[]: 2
a
b

System.Collections.Generic.List`1[System.String]: 2
a
b

System.Collections.Generic.List`1[System.String]: 0

Now, empty arrays and collection are not special, but empty enumerables are special.

So, going back to the question, we can now ask it again with additional emphasis:

Why ARE empty enumerables special, but empty arrays and collections are not

Clearly, they shouldn't be, but to answer that question...

In the first run
A convention was applied. That convention turned the enumerable into a list. That's fair enough as it's likely what the user wanted and is pretty safe from the immutability standpoint.

In the second run
The user manually applied that convention (of treating an enumerable as a list). The outcome is still the same: the enumerable property is backed by a list and is represented by an enumerable, and most importantly, it's still safe from the immutability standpoint.

So, if either way, in both example, we have an enumerable backed by a list and the only difference being one is set by convention and one is being set by the user. So the question is now why does functionality differ?

Perhaps if the issue behind this PR was worded something like:

"When manually backing an enumerable as a collection, binding does not populate that collection"

... then it would be more clear-cut.

[maryamariyan]

We discussed this issue offline today.

System.Text.Json and most of the deserializers out there overwrite the collection on the POCO

It was also discussed that a big difference between deserializing and configuration binding was there are multiple sources in binder.

There is a lack of clarity because there was not enough information upfront in the issue before setting this as up-for-grabs.

It is best if we close this PR and do it ourselves.

Thanks for your contribution @SteveDunn.

cc: @HaoK

[SteveDunn]

The discussion so far, as detailed in the examples in this comment in this thread, shows that this is the crux of the problem:

public class Config
{
    public IEnumerable<string> TheEnumerable { get; set; }
}

this works fine - a convention is applied and the enumerable property is set with a List<T> and the contents of config are applied to the list.

This however does not work - the collection is not populated despite it being set to a List<T>:

public class Config
{
    public IEnumerable<string> TheEnumerable { get; set; } = new List<string>();
}

Now that we've boiled it down to this, it has uncovered a potentially more serious problem when users turn on nullable reference types:

warning CS8618: Non-nullable property 'TheEnumerable' must contain a non-null value 
when exiting constructor. Consider declaring the property as nullable.

When turning on nullable reference types, users can mark every field as nullable, or, as would be more common, explicitly initialise those fields to be non null.

The act of explicitly initialising those fields to be non null would now break binding, which is entirely unexpected.