[Breaking change]: ConfigurationBinder raises exceptions if ErrorOnUnknownConfiguration is set

[SteveDunn]

Description

If BinderOptions.ErrorOnUnknownConfiguration is set and a value in specified in config that cannot be bound to the corresponding property in the bound model, an InvalidOperationException is now thrown.

Previously, BinderOptions.ErrorOnUnknownConfiguration was solely used to raise an exception if a value existed in config but didn't exist in the model being bound to.

As part of PR dotnet/runtime#77708 (which fixes #73915), this property is now also used to throw exceptions if the value in config cannot be converted to the type of value in the model being bound to.

Version

.NET 8 Preview 1

Previous behavior

Previously, this would silently swallow the exceptions for the fields containing invalid enums:

public enum TestSettingsEnum
{
    Option1,
    Option2,
}

public class MyModelContainingArray
{
    public TestSettingsEnum[] Enums { get; set; }
}

public void SilentlySwallowsInvalidItems()
{
    var dic = new Dictionary<string, string>
        {
            {"Section:Enums:0", "Option1"},
            {"Section:Enums:1", "Option3"}, // invalid - ignored
            {"Section:Enums:2", "Option4"}, // invalid - ignored
            {"Section:Enums:3", "Option2"},
        };

    var configurationBuilder = new ConfigurationBuilder();
    configurationBuilder.AddInMemoryCollection(dic);
    var config = configurationBuilder.Build();
    var configSection = config.GetSection("Section");

    var model = configSection.Get<MyModelContainingArray>(o => o.ErrorOnUnknownConfiguration = true);

    // we previously would end up with just Option1 and Option2 in the bound collection
}

New behavior

Now, if the value cannot be converted to the type of the value in the model, an InvalidOperationException is thrown.

Type of breaking change

• Binary incompatible: Existing binaries may encounter a breaking change in behavior, such as failure to load/execute or different run-time behavior.
• Source incompatible: Source code may encounter a breaking change in behavior when targeting the new runtime/component/SDK, such as compile errors or different run-time behavior.

Reason for change

In the bug report, users were setting BinderOptions.ErrorOnUnknownConfiguration and expecting that to throw if there were any issues in binding configuration.

But the original intention of it was to just throw if something was missing from the bound model.

Perhaps if I'd originally chosen a clearer name, this might've avoided this confusion. I suggested adding another property in BinderOptions, but we went instead with making the existing property dual purpose.

Recommended action

The recommended action is to change or remove the configuration values that cannot be converted to the properties in the bound model.

Alternatively, change BinderOptions.ErrorOnUnknownConfiguration from true to false

Feature area

Core .NET libraries, Serialization

Affected APIs

All of the following in ConfigurationBinder:

• public static T? Get<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] T>(this IConfiguration configuration, Action<BinderOptions>? configureOptions)
• public static object? Get(this IConfiguration configuration, Type type, Action<BinderOptions>? configureOptions)
• public static void Bind(this IConfiguration configuration, object? instance, Action<BinderOptions>? configureOptions)

---

Associated WorkItem - 61240

[0xced]

I'd like to challenge the repurposing of the ErrorOnUnknownConfiguration property to control whether an exception must be thrown when a conversion fails. I think the swallowing should never have happened and the exception should be thrown unconditionally.

Surprisingly the exception swallowing was introduced on August 26, 2015 but only reported 7 years later in dotnet/runtime#73915. And I happen to discover it today!

Another option could be to throw an AggregateException since the try/catch happens in a loop.

Any thoughts, @SteveDunn, @tarekgh ?

[tarekgh]

@0xced I am not sure I understand the question here. The change you pointed at dotnet/runtime#73915 which force not swallow the exception. isn't the behavior you were asking for when you said I think the swallowing should never have happened and the exception should be thrown unconditionally.?

[0xced]

Sorry it was not clear. What I meant by unconditionally is that the exception should always be thrown, no matter the value of ErrorOnUnknownConfiguration.

So I would prefer going from this:

foreach (IConfigurationSection section in config.GetChildren())
{
    var itemBindingPoint = new BindingPoint();
    try
    {
        BindInstance(
            type: elementType,
            bindingPoint: itemBindingPoint,
            config: section,
            options: options);
        if (itemBindingPoint.HasNewValue)
        {
            list.Add(itemBindingPoint.Value);
        }
    }
    catch
    {
    }
}

Straight to this, without any exception wrapping:

foreach (IConfigurationSection section in config.GetChildren())
{
    var itemBindingPoint = new BindingPoint();
    BindInstance(
        type: elementType,
        bindingPoint: itemBindingPoint,
        config: section,
        options: options);
    if (itemBindingPoint.HasNewValue)
    {
        list.Add(itemBindingPoint.Value);
    }
}

Or alternatively with an aggregate exception since we are in a loop:

var exceptions = new List<Exception>();
foreach (IConfigurationSection section in config.GetChildren())
{
    var itemBindingPoint = new BindingPoint();
    try
    {
        BindInstance(
            type: elementType,
            bindingPoint: itemBindingPoint,
            config: section,
            options: options);
        if (itemBindingPoint.HasNewValue)
        {
            list.Add(itemBindingPoint.Value);
        }
    }
    catch (Exception ex)
    {
        exceptions.Add(ex);
    }
}
if (exceptions.Any())
{
    throw new AggregateException(exceptions);
}

Exception thrown on conversion error is what happens when binding an object, why would it be different when binding a collection or an array? Also, hijacking the original purpose of the ErrorOnUnknownConfiguration property is not a good idea in my opinion. No matter how well documented, the property is still named ErrorOnUnknownConfiguration and confusion will inevitably ensue.

[tarekgh]

What I meant by unconditionally is that the exception should always be thrown, no matter the value of ErrorOnUnknownConfiguration.

This will be a big concern for app compatibility. This breaking change will not be trivial. Many working apps may start to experience such unhandled exceptions. This is why we are doing the throwing behavior with the option ErrorOnUnknownConfiguration. Any reason you cannot opt-in to ErrorOnUnknownConfiguration if you want the exceptions?