Runtime: Add BinderOption to allow callers to specify that configuration binding should throw an exception upon any failure.

[BlacKCaT27]

UPDATED PROPOSAL

Background and Motivation

The configuration binding system provided by Microsoft.Extensions.Configuration currently does not throw any kind of exception should there be any issue binding configuration data to a model object. Scenarios such as a key missing from a configuration provider, or a model object not being updated to provide access to a new configuration key can often result in difficult to diagnose run-time errors, some of which might not be caught until a deployment, as one of the few differences between testing and production environments are configuration values.

There would be value in providing users of the configuration system a mechanism to make model binding in either direction an exception-throwing event to make it more immediately apparent when issues arise due to configuration mismatches.

Proposed API

Create a new exception Microsoft.Extensions.Configuration.ConfigurationException. As much as possible, this exception should be designed to include as much information as possible based on the context in which it is throw. Given the proposals below, the exception should, at a minimum, always include the name of the configuration key and (when possible)/or (when not) the model property that was being bound at the time the exception was thrown.

Add two new properties to the Microsoft.Extensions.Configuration.BinderOptions class:

public class BinderOptions
    {
        /// <summary>
        /// When false (the default), the binder will only attempt to set public properties.
        /// If true, the binder will attempt to set all non read-only properties.
        /// </summary>
        public bool BindNonPublicProperties { get; set; }

+       /// <summary>
+       /// If set to true, the configuration system would throw a `ConfigurationException` if, during configuration binding, a 
+       /// configuration key is found for which the provided model object does not have an appropriate property which matches
+       /// the keys name. Note: appropriate property here refers to a property that is eligible to be bound to (for example, a
+       /// private property would not be considered an `eligible` target property unless 
+       /// </summary>
+       public bool ErrorOnPropertyMissing { get; set; }

+       /// <summary>
+       /// If set to true, the configuration system would throw a `ConfigurationException` if, during configuration binding, a 
+       /// property is found on a target model object which does not have a corresponding configuration key provided by any 
+       /// configuration providers. Default value is false.
+       /// </summary>
+       public bool ErrorOnKeyMissing { get; set; }
    }

Usage Examples

I had considered including a 3rd flag as a new property: BinderOptions.ErrorOnConfigurationMismatch which would perhaps throw an exception in both cases proposed above, and/or additionally in other cases (such as model property count/configuration key count mismatch, which may or may not imply the above scenarios in some cases). Ultimately though I felt a simpler approach was best to start.

Or instead of adding the two properties above, add just one properties that controls throwing on all mismatches instead.

public class BinderOptions
    {
        /// <summary>
        /// When false (the default), the binder will only attempt to set public properties.
        /// If true, the binder will attempt to set all non read-only properties.
        /// </summary>
        public bool BindNonPublicProperties { get; set; }

+       /// <summary>
+       /// If set to true, the configuration system would throw a `ConfigurationException` if, during configuration binding, a 
+       /// configuration key is found for which the provided model object does not have an appropriate property which matches
+       /// the keys name; or if a configuration property is found without a matching key. 
+       /// </summary>
+       public bool ErrorOnConfigurationMismatch { get; set; }
}

Risks

As these flags would be disabled by default, the existing behavior of the configuration binding process would not change. However, developers should be encouraged to thoroughly test their configuration management systems when enabling these flags, given the environment-dependent nature of configuration management and the potential impact of the change should there be any issues with a systems configuration.
#

ORIGINAL POST

Is your feature request related to a problem? Please describe.

Currently, when using configuration binding, in the event that a property does not get bound (either because there's a missing entry in the configuration file or a missing property on the class being bound to), the binder simply defaults to null for that property. This results in run-time null exceptions if there is a configuration issue.

Describe the solution you'd like

I propose we add an optional setting in the new BinderOptions class which allow callers to specify that in the event of a failure to properly bind every configuration option, to throw an exception rather than passively set property values to null.

An optional extension of this idea would be to also allow callers to specify custom validators for specific configuration values. For example, being able to annotate the POCO that a configuration file is being bound to such that something like a Connection String could have a custom validator called on it to make sure all the required arguments are contained in the string, the database is reachable, etc.

Describe alternatives you've considered

The alternative is to use something like an IStartupFilter to manually check all your configuration parameters validity during application start-up. This is tedious and error-prone, and results in application level code having to validate behaviors provided by framework level code, mixing concerns.

Additional context

I believe this feature will add a lot of value at relatively low cost. It's much easier to find misconfiguration errors if start-up fails because of app misconfiguration, rather than having to track down unexpected null exceptions thrown from some piece of code using configuration values.

It looks like there's been some work/discussion on this front in other forms (issue dotnet/extensions#459 and issue dotnet/extensions#763), but those are more focused on the larger problems of IOption and custom validators. Perhaps this issue can focus solely on establishing a starting point via making Bind() throw on error an option.

I'd be more than happy to put up an initial PR to explore this further.

[BlacKCaT27]

@Pilchie I'm just a fan of .net core who wants to make a contribution, not privy to the teams plans, but I don't want to put up a PR that has no chance of being merged if what I submit goes against whatever plan your team has on this topic. Is there already something in the works on this front?

[maryamariyan]

@Pilchie I'm just a fan of .net core who wants to make a contribution, not privy to the teams plans, but I don't want to put up a PR that has no chance of being merged if what I submit goes against whatever plan your team has on this topic. Is there already something in the works on this front?

@BlacKCaT27 thanks for the issue. We usually stay away from throwing first hand exceptions, but since this is configurable and disabled by default this feature makes sense.
cc: @davidfowl

To satisfy this issue there needs to be a new API proposal introduced in configuration binding.

The best next step on this would be to prepare the API proposal, usage, etc. in the issue page here (following https://github.com/dotnet/runtime/blob/master/docs/project/api-review-process.md).

[davidfowl]

It's much easier to find misconfiguration errors if start-up fails because of app misconfiguration, rather than having to track down unexpected null exceptions thrown from some piece of code using configuration values.

It depends on how you bind, it won't fail at startup, it'll fail when you try to resolve the options.

[maryamariyan]

Related to closed dupe: https://github.com/dotnet/runtime/issues/36129 and https://github.com/dotnet/runtime/issues/36502

[BlacKCaT27]

Thank you very much for the update. I'll try to get a proposal together this weekend.

[BlacKCaT27]

Background and Motivation

The configuration binding system provided by Microsoft.Extensions.Configuration currently does not throw any kind of exception should there be any issue binding configuration data to a model object. Scenarios such as a key missing from a configuration provider, or a model object not being updated to provide access to a new configuration key can often result in difficult to diagnose run-time errors, some of which might not be caught until a deployment, as one of the few differences between testing and production environments are configuration values.

There would be value in providing users of the configuration system a mechanism to make model binding in either direction an exception-throwing event to make it more immediately apparent when issues arise due to configuration mismatches.

Proposed API

Create a new exception Microsoft.Extensions.Configuration.ConfigurationException. As much as possible, this exception should be designed to include as much information as possible based on the context in which it is throw. Given the proposals below, the exception should, at a minimum, always include the name of the configuration key and (when possible)/or (when not) the model property that was being bound at the time the exception was thrown.

Add two new properties to the Microsoft.Extensions.Configuration.BinderOptions class:

BinderOptions.ErrorOnKeyMissing - boolean - If set to true, the configuration system would throw a ConfigurationException if, during configuration binding, a property is found on a target model object which does not have a corresponding configuration key provided by any configuration providers. Default value is false.

BinderOptions.ErrorOnPropertyMissing - boolean - If set to true, the configuration system would throw a ConfigurationException if, during configuration binding, a configuration key is found for which the provided model object does not have an appropriate property which matches the keys name. Note: appropriate property here refers to a property that is eligible to be bound to (for example, a private property would not be considered an eligible target property unless BinderOptions.BindNonPublicProperties were set to true).

Usage Examples

I had considered including a 3rd flag as a new property: BinderOptions.ErrorOnConfigurationMismatch which would perhaps throw an exception in both cases proposed above, and/or additionally in other cases (such as model property count/configuration key count mismatch, which may or may not imply the above scenarios in some cases). Ultimately though I felt a simpler approach was best to start.

Risks

As these flags would be disabled by default, the existing behavior of the configuration binding process would not change. However, developers should be encouraged to thoroughly test their configuration management systems when enabling these flags, given the environment-dependent nature of configuration management and the potential impact of the change should there be any issues with a systems configuration.
#

[safern]

@BlacKCaT27 thanks for putting this together. I moved it to the issue description as that is where the reviewers look at the proposal for and also formatted a little bit to use the diff tool in markdown so that it is clearer what APIs are being proposed.

I do have a question, in my opinion, I think it would just be better to have just one property that controls all binding errors, it is either, you want errors or not when binding, it doesn't matter if it is a key or a property, what do you think?

[BlacKCaT27]

Hi there. Thanks for your help.

I honestly feel like it could go either way. Personally, I like more
granular controls but it does feel like it's more the style of .net core to
just have the one. I would say whichever the maintainers feel is more
appropriate would be fine.

On Thu, Oct 15, 2020, 1:19 PM Santiago Fernandez Madero <
[email protected]> wrote:

@BlacKCaT27 https://github.com/BlacKCaT27 thanks for putting this
together. I moved it to the issue description as that is where the
reviewers look at the proposal for and also formatted a little bit to use
the diff tool in markdown so that it is clearer what APIs are being
proposed.

I do have a question, in my opinion, I think it would just be better to
have just one property that controls all binding errors, it is either, you
want errors or not when binding, it doesn't matter if it is a key or a
property, what do you think?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/dotnet/runtime/issues/36015#issuecomment-709471643,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/ABH7TYOBU5KC5TV7AKMF6CDSK4VLDANCNFSM4M3UBCHA
.

[safern]

Thanks, @BlacKCaT27 -- yeah I think having more control is sometimes good, but I would expect both properties to be set to true whenever someone wants error, I don't see a clear case of just setting one to true. I'll leave it as open question on the proposal so that when the design review goes ahead we make a decision.