Microsoft.Extensions.Configuration: IReadOnlyList Options with default value are not filled

[drauch]

LABEL=area-Extensions-Configuration

Description

When I configure the following Options class:

class MyOptions
{
    // Null if not in appsettings.json, otherwise values from appsettings.json
    public string[] Works1 { get; set; }
    // Null if not in appsettings.json, otherwise values from appsettings.json
    public List<string> Works2 { get; set; }
    // Null if not in appsettings.json, otherwise values from appsettings.json
    public IReadOnlyList<string> Works3 { get; set; }
    // Empty if not in appsettings.json, otherwise values from appsettings.json
    public List<string> Works4 { get; set; } = new List<string>();
    // Empty in any case
    public IReadOnlyList<string> DOES_NOT_WORK { get; set; } = new List<string>();
}

The DOES_NOT_WORK is not properly filled, it is always empty, even if there are values in the appsettings.json

Configuration

.NET Framework 4.8 / Windows 10 / x64 / ASP.NET Core 2.1
(aspnetcore repo says that the Configuration package is now maintained here, which is why I'm filing the issue here, hope it's correct)

Best regards,
D.R.

[Dotnet-GitSync-Bot]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

Tagging subscribers to this area: @eiriktsarpalis
Notify danmosemsft if you want to be subscribed.

[drauch]

@jeffschwMSFT I think System.Collections is not the correct label, it should be area-Extensions-Configuration don't you think?

[ericstj]

Interesting. So if no instance is provided it can bind but if it finds an instance then it cannot.

The problem is here:

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

Lines 309 to 346 in 4cb924d

	// If we don't have an instance, try to create one
	if (instance == null)
	{
	// We are already done if binding to a new collection instance worked
	instance = AttemptBindToCollectionInterfaces(type, config, options);
	if (instance != null)
	{
	return instance;
	}
	instance = CreateInstance(type);
	}
	// See if its a Dictionary
	Type collectionInterface = FindOpenGenericInterface(typeof(IDictionary<,>), type);
	if (collectionInterface != null)
	{
	BindDictionary(instance, collectionInterface, config, options);
	}
	else if (type.IsArray)
	{
	instance = BindArray((Array)instance, config, options);
	}
	else
	{
	// See if its an ICollection
	collectionInterface = FindOpenGenericInterface(typeof(ICollection<>), type);
	if (collectionInterface != null)
	{
	BindCollection(instance, collectionInterface, config, options);
	}
	// Something else
	else
	{
	BindNonScalar(config, instance, options);
	}
	}
	}

If the instance is null then it checks the target type for known generic interfaces, then it will instantiate it as a List<T>.

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

Lines 218 to 223 in 4cb924d

	private static object BindToCollection(TypeInfo typeInfo, IConfiguration config, BinderOptions options)
	{
	Type type = typeof(List<>).MakeGenericType(typeInfo.GenericTypeArguments[0]);
	object instance = Activator.CreateInstance(type);
	BindCollection(instance, type, config, options);
	return instance;

However if it is already initialized, it checks the target type (in this case IReadOnlyList<T>) for only a couple known mutable interfaces:

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

Lines 322 to 345 in 4cb924d

	// See if its a Dictionary
	Type collectionInterface = FindOpenGenericInterface(typeof(IDictionary<,>), type);
	if (collectionInterface != null)
	{
	BindDictionary(instance, collectionInterface, config, options);
	}
	else if (type.IsArray)
	{
	instance = BindArray((Array)instance, config, options);
	}
	else
	{
	// See if its an ICollection
	collectionInterface = FindOpenGenericInterface(typeof(ICollection<>), type);
	if (collectionInterface != null)
	{
	BindCollection(instance, collectionInterface, config, options);
	}
	// Something else
	else
	{
	BindNonScalar(config, instance, options);
	}
	}

I think the fix here would be to use instance.GetType() for checking against these instead of targetType. @layomia this looks a lot like a bug I recall you fixing in the JSON serializer around a similar case.

[drauch]

Thanks for analyzing this. You added the 5.x label, is there any chance such a bug is backported to 2.x if someone fixes it?

Best regards,
D.R.

[ericstj]

@Pilchie + team can consider for backport. Also, since this is a nuget package you can always independently reference the newer version and carry it with your application.

[pinkfloydx33]

I ran into a similar issue the other day. If the type of the property is an IList<T> with a default value of T[], the original array is preserved and binding is "skipped". I had expected it to create a new list of the appropriate size. Consider the following class:

public class MyClass
{
    public IList<string> MyProp { get; set; } = new[] { "ABC" };
}

With configuration sourced from the following json:

{ 
    "MyProp": ["DEF", "GHI"]
}

The property is settable so I would either expect the binder to overwrite it completely or for it to call IList.Add (if the instance is not an array). Instead, it preserves the original value:

var obj = new MyClass();
config.Bind(obj);
Console.WriteLine(obj.MyProp.Count); // 1
Console.WriteLine(obj.MyProp[0]); // ABC

This occurs because BindCollection is invoked for the property, which ends up throwing and swallowing the exception from calling ICollection.Add on an array:

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

Lines 445 to 452 in 4cb924d

	if (item != null)
	{
	addMethod.Invoke(collection, new[] { item });
	}
	}
	catch
	{
	}

(This ends up needlessly binding the element as well)

I was able to get around the issue by setting the property to null before binding:

var obj = new MyClass { MyProp = null };
config.Bind(obj);
Console.WriteLine(obj.MyProp.Count); // 2
Console.WriteLine(obj.MyProp[0]); // DEF
Console.WriteLine(obj.MyProp[1]); // GHI

This works alright if I'm manually calling Bind but it doesn't work otherwise. For example when calling Configure<TOpt>(IConfiguration) I have no control over the object. It also doesn't work if I wanted additive behavior as I'd have to know what the default values were so that I could repeat them in my configuration. (In this case I also don't control the class in question).

If the property is instead declared as T[] we get additive behavior

public class MyClass
{
    public string[] MyProp { get; set; } = { "ABC" };
}

var obj = new MyClass();
config.Bind(obj);
Console.WriteLine(obj.MyProp.Length); // 3
Console.WriteLine(obj.MyProp[0]); // ABC
Console.WriteLine(obj.MyProp[1]); // DEF
Console.WriteLine(obj.MyProp[2]); // GHI

It behaves the same if its declared/defaulted as:

public IList<string> MyProp { get; set; } = new List<string> { "ABC" };

If the property is settable I would expect the binder to overwrite it if necessary. The different behavior for IList<T>/T[], whether or not there's a default value (and it's type) is a bit confusing. It seems like you should check both the static and runtime types. So instead of:

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

Lines 328 to 331 in 4cb924d

	else if (type.IsArray)
	{
	instance = BindArray((Array)instance, config, options);
	}

You could have the following which works (in my limited testing), generating a new array with three values

else if (type.IsArray || instance.GetType().IsArray)
{
  instance = BindArray((Array)instance, config, options);
}

As an aside, in my case I don't even want the additive behavior--I'd like the list to be replaced entirely (losing the default values). I can achieve this by setting null if I'm calling Bind manually but not otherwise. Perhaps a property on BinderOptions to "replace collections" or something like that for a bit more control could help. I may open an issue for it.