-
Notifications
You must be signed in to change notification settings - Fork 4.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add BinderOption to allow callers to specify that configuration binding should throw an exception upon any failure. #36015
Comments
@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. 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). |
It depends on how you bind, it won't fail at startup, it'll fail when you try to resolve the options. |
Thank you very much for the update. I'll try to get a proposal together this weekend. |
Background and MotivationThe configuration binding system provided by 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 APICreate a new exception Add two new properties to the
Usage ExamplesBelow is an example when services.AddOptions<RequestHandlingOptions>().Bind(Configuration.GetSection("RequestHandlingOptions"), options =>
{
options.BindNonPublicProperties = true;
options.ErrorOnKeyMissing = true;
options.ErrorOnPropertyMissing = true;
}); Alternative DesignsI had considered including a 3rd flag as a new property: RisksAs 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. |
@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? |
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 < ***@***.***> 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
<#36015 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABH7TYOBU5KC5TV7AKMF6CDSK4VLDANCNFSM4M3UBCHA>
.
|
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. |
namespace Microsoft.Extensions.Configuration
{
public class BinderOptions
{
public bool ErrorOnUnknownConfiguration{ get; set; }
}
} |
The thought behind That said, I see your point about this being a pretty heavy-handed approach when not all configuration properties necessarily need overrides. I love the idea of some sort of Edit: Totally on board with the name change for PropertyMissing, btw. I agree it was too ambiguous. |
@BlacKCaT27 could you propose that as a separate issue? Would that be tight to this new property? I mean, would we throw if we find |
Great question. I'm personally leaning towards "throw always" and use the attribute in lieu of I can submit this as a separate issue. |
The thing that it's making me doubt about that approach is that if I want the system to throw about any unknown configuration, I would have to add the attribute to all the keys? Maybe we could come up with a behavior where if you add it to the class it should throw for all the keys under that class, but if you have a subclass then you would end up annotating all classes, so in that case a switch would be simpler. So I think having the switch for an all or nothing behavior is helpful and then the attribute for a granular situation? |
True, though I think at that point it depends more on the use case. I could see some scenarios where there's only a couple fields where I want this attribute (so an unmatched configuration throws, like a connection string) but for others I don't care. What if, like you suggest, a switch for all-or-nothing behavior gets added, but then the attribute can be used to configure the systems reaction. Like, instead of the switch saying "do I or do I not throw when there's a problem", the switch becomes "check for required configuration attributes" and then the attribute can have a parameter list that describes what should happen on a failure. That way you could make the attribute more useful by giving it optional params like a default value to assign alongside a flag dictating whether or not to throw.
Going this route you could then safely default the switch to On, since it would be inert until someone used |
While I understand the intent of what you describe above I think not depending on attributes and being able to bind with validation for missing Properties by just turning on a switch is a really good thing to have. Then the attributes could be a separate configuration that doesn't depend on the switch where you can granularly fail for missing properties/keys and also configure the default value for a required configuration. Don't you think it is still valuable for complicated configurations to instead of having to throw an attribute on all properties, to just turn on a switch to get missing properties validation? |
Absolutely. I think my brain was just stuck in attribute mode :)
Having a global switch to start is a good approach. It may even provide us
more insight into how best to introduce an attribute later.
…On Mon, Feb 1, 2021, 5:23 PM Santiago Fernandez Madero < ***@***.***> wrote:
While I understand the intent of what you describe above I think not
depending on attributes and being able to bind with validation for missing
Properties by just turning on a switch is a really good thing to have. Then
the attributes could be a separate configuration that doesn't depend on the
switch where you can granularly fail for missing properties/keys and also
configure the default value for a required configuration.
Don't you think it is still valuable for complicated configurations to
instead of having to throw an attribute on all properties, to just turn on
a switch to get missing properties validation?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#36015 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABH7TYOK22M6W2BHOC6FZN3S44SXLANCNFSM4M3UBCHA>
.
|
Awesome. Thanks for the discussion. Feel free to open a separate issue for the attribute addition where we can discuss the design of it. Are you interested on tackling this issue? |
If you can point me in the right direction as to where to start in the code
I can maybe give it a shot, but it would have to be in my spare time. If
there's an expectation per your teams processes as to when you'd need this,
I'd appreciate a heads up.
Otherwise, it may be better for someone closer to the code to jump in. I'm
more than willing to dive in, but I'm not that familiar with all the
various repos and how things are broken up so it'll take me longer than
someone who knows the codebase.
…On Mon, Feb 1, 2021, 5:30 PM Santiago Fernandez Madero < ***@***.***> wrote:
Having a global switch to start is a good approach. It may even provide us
more insight into how best to introduce an attribute later.
Awesome. Thanks for the discussion. Feel free to open a separate issue for
the attribute addition where we can discuss the design of it.
Are you interested on tackling this issue?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#36015 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABH7TYLOZHLJIQ7VHIQ36ULS44TOZANCNFSM4M3UBCHA>
.
|
I'd be happy to take this one on. My last PR (#52514) was closed as there was not enough information provided before being marked up-for-grabs, and hence the .net team will be doing it themselves at some point. |
@SteveDunn sounds good. |
I created a PR for this: #53852 |
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:Usage Examples
Below is an example when
BinderOptions
is being used alongside theMicrosoft.Extensions.Options
package. The usage would be the same for any other location whereBinderOptions
can be provided as an argument.Alternative Designs
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.
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.
The text was updated successfully, but these errors were encountered: