Skip to content
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 built in support for validating options #2388

Closed
aspnet-hello opened this issue Jan 1, 2018 · 10 comments
Closed

Add built in support for validating options #2388

aspnet-hello opened this issue Jan 1, 2018 · 10 comments
Assignees
Labels
Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one

Comments

@aspnet-hello
Copy link

From @Alxandr on Wednesday, August 23, 2017 4:37:39 AM

I just made an Option class that takes strings that are required to be a certain format. I would like the configuration to fail after hydration if the values provided are invalid. A good example is if you need an email (ignoring how hard it can be to validate emails).

Today I can solve this by creating an IPostConfigureOptions that resolves to a ValidateOptions class that checks if the options in question implement an IValidatable interface, and if so calls Validate().

It would, however, be preferable if I knew that validation always ran last (after all post configure handlers - if any). Therefore I suggest adding first class support for IValidateOptions which runs after runs after IPostConfigureOptions. This interface would behave the exact same as IPostConfigureOptions, except it would be guaranteed to run after all IPostConfigureOptions.

It can also be discussed whether or not default implementation that does something similar to my described ValidateOptions class would be useful.

Copied from original issue: aspnet/Options#227

@aspnet-hello
Copy link
Author

From @MaKCbIMKo on Friday, August 25, 2017 2:40:03 AM

I think this is something similar with what you're looking for.

Am I right?

@aspnet-hello aspnet-hello added this to the Backlog milestone Jan 1, 2018
@aspnet-hello aspnet-hello added enhancement This issue represents an ask for new feature or an enhancement to an existing one Needs: Design This issue requires design work before implementating. repo:Options labels Jan 1, 2018
@aspnet-hello
Copy link
Author

From @HaoK on Friday, August 25, 2017 2:54:58 AM

This feature is on our radar, I'm not sure when we will add first class support for validation of options, (today we roll our own validation pattern in Auth), but sooner or later this seems like something Options should support in a first class way.

cc @divega @ajcvickers @davidfowl

@aspnet-hello
Copy link
Author

From @MaKCbIMKo on Tuesday, August 29, 2017 6:29:25 AM

Some time ago I was thinking about being able to validate options. In my opinion, this is really useful feature - being able to validate options and handle invalid scenarios.

Moreover, I was looking for possibility to validate all registered options at start-up time (not only when it will be required) - this is just a simple early problem detection.

For that purpose I have created PR (aspnet/Options#171) which adds the possibility to configure the validation of option that's going after all configure steps. And that PR introduces the possibility to validate all options at the start up time.

@georgiosd
Copy link

Is this dead? I saw the PR and comments so I'm not sure why it's closed.

The use case seems obvious. There are things like API keys which if not configured correctly can result in serious problem in production. Some support to at least mark properties as [Required] could make a big difference.

@HaoK
Copy link
Member

HaoK commented Apr 6, 2018

Its not dead, but needs more votes/requests for its priority to go up

@georgiosd
Copy link

How do I vote? :)

@HaoK
Copy link
Member

HaoK commented Apr 6, 2018

I think you already have :)

@HaoK
Copy link
Member

HaoK commented Apr 6, 2018

I'll bring this up again as a potential feature for 2.2, but compelling scenarios that would be enabled, like maybe describing how you'd like something like [Required] to work might help. We would probably use something like that in Security for auth as well, as we have a few features that we'd require to be configured, like client id/secret

@georgiosd
Copy link

Oh good :)

Uhm, let's see. I guess it depends on what the application is and what it does and what's acceptable as misconfiguration.

In my case, it would make sense for the application to fail to load completely and trigger an exception handler (which would go to App Insights and I'd get notified and revert the deployment).

I say that's the only thing that makes sense because the app can't function without the keys that were missing and there's no way for me or the app to guess them or provide defaults.

I imagine however that this is possible in some other cases (to provide defaults) so perhaps an enum-style flag can be passed somewhere to say "fail" or "revert to defaults". It would make sense to me that the defaults are either provided by another file and/or by constructing a new options object.

@HaoK
Copy link
Member

HaoK commented Aug 4, 2018

aspnet/Options#266

@HaoK HaoK closed this as completed Aug 4, 2018
@HaoK HaoK added Done This issue has been fixed and removed 2 - Working Needs: Design This issue requires design work before implementating. labels Aug 4, 2018
natemcmaster pushed a commit that referenced this issue Nov 14, 2018
- Check if the post handle is disposed and noop if it is.
We also catch an ObjectDisposedException because it's an inherent race condition.
ryanbrandenburg pushed a commit that referenced this issue Nov 27, 2018
@ghost ghost locked as resolved and limited conversation to collaborators Dec 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one
Projects
None yet
Development

No branches or pull requests

3 participants