Add possibility to configure options validation in order to validate options at the start up time (not at runtime). #171
Conversation
Added possibility to configure options with initializers in order to initialize options at the start up time (not at runtime).
|
Hi @MaKCbIMKo, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! TTYL, DNFBOT; |
|
@MaKCbIMKo, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR. |
Add possibility to configure options validation and perform validation at start up.
MaKCbIMKo
changed the title
MaKCbIMKo
changed the title
| return services; | ||
| } | ||
|
|
||
| public static IServiceCollection Validate<TOptions>(this IServiceCollection services, Expression<Func<TOptions, bool>> validateExpression) |
There was a problem hiding this comment.
What do you think about a less restrictive signature for the validation? If we call it Validation but treat it effectively as a second set of actions that run after the Configure step. This would let you write Validations that both Assert/SetDefaults. What you have today I think would be good as something built on top, sugar that makes it easy for you to register an expression that returns a bool.
Something like Validate(Action<TOptions> options) as well.
What do you think?
There was a problem hiding this comment.
So, you suggest adding a few overloads for Validate method with a lightweight signature, correct?
I was thinking about using Action<TOptions> as a parameter. But it would require to use exceptions to signal that options are invalid. As far as I remember, using bool is faster than exceptions. Moreover, if will expect exception we will have to wrap into try/catch block each separated validation call (in other case validation will be stopped after first validation failed).
I was trying to use Func<TOptions, bool> at first, but I found Expression more informative, because it provides information that might help identify the problem (and might be logged).
There was a problem hiding this comment.
Yeah I was thinking we layer things on. So at the lowest layer there's a Validate that just takes an Action, then we can build additional overloads that take expression/func and return bools, that way there's still the flexibility to drop down and do something different if they don't want to follow the more constrained func/expression/bool route.
We wouldn't necessarily have to do anything to support wrapping exceptions either this way, since that's the lowest level API
There was a problem hiding this comment.
I've got your point. I will add the lowest layer overloads.
There was a problem hiding this comment.
I'm thinking maybe the higher level overload should be Func<TOptions, IEnumerable<Exception>> they can return multiple validation exceptions this way so its slightly more powerful than a bool
|
Take a look at #161 which is tracking some features which would be useful for options to provide for the Auth 2.0 work, named options will add a bit of a wrinkle since there'd need to be the ability to target All instances vs named instances, vs the default instance. We haven't decided that we are bringing back named options yet, but its something to consider |
remove redundant rows.
OptionsCache class replaced with OptionsFactory. OptionsFactory is an additional injection point that might be used.
Add more flexibility of configuration and validation result handling.
|
@HaoK, I've modified (a lot) of validation stuff. Moreover, I took a look on feature (about multi-tenant) and found that it might require some validation 'on creation' time. I also added that (wasn't not a big deal). About multi-tenancy: it all depends on how we will configure all these 'names', where we will store cached instances (for different names), but I think it won't be so difficult to modify current validation to work in a new way. But, only if you have some roadblocks that you still not sure (or don't know) how to break... |
|
Thanks, I'll try to take a look this week |
|
So I'm going to be starting a PR for #161, it will be a pretty lightweight feature to start, I'll submit it by EOD tomorrow, so you can provide input and insight since you've been playing around with validation for a while. The initial critical feature is named options support, so the first iteration of the PR will likely have very minimal validation support (think |
|
Ok, waiting for your PR. |
|
Take a look at #176 |
|
@HaoK - Is it fine to base this PR over your initial Options 2.0 changes? Or there more changes are comming? Maybe something in my PR need to be reconsidered... |
|
Feel free to rebase if you want, there likely will be more changes coming, but it won't be for a few weeks |
Add unit tests for validation.
# Conflicts: # src/Microsoft.Extensions.Options/OptionsCache.cs # src/Microsoft.Extensions.Options/OptionsManager.cs # src/Microsoft.Extensions.Options/OptionsMonitor.cs # src/Microsoft.Extensions.Options/OptionsServiceCollectionExtensions.cs # test/Microsoft.Extensions.Options.Test/Microsoft.Extensions.Options.Test.csproj # test/Microsoft.Extensions.Options.Test/OptionsSnapshotTest.cs
|
@HaoK - I have added a possibility to validate all named options. Please take a look. p.s. there is some problems with Travis CI check (Error: Unable to locate libunwind. Install libunwind to continue) - not sure how to solve that weather the problem with my changes. |
# Conflicts: # src/Microsoft.Extensions.Options.ConfigurationExtensions/ConfigurationChangeTokenSource.cs # src/Microsoft.Extensions.Options/LegacyOptionsCache.cs # src/Microsoft.Extensions.Options/OptionsServiceCollectionExtensions.cs
|
Changes from dev branch has been merged. @HaoK - may I ask you to review the PR? |
|
About 'AppVeyor build failed' - seems like it faced the exception during downloading "https://dotnetcli.azureedge.net/dotnet/Sdk/2.0.0-preview3-006785/dotnet-sdk-2.0.0-preview3-006785-win-x64.zip" (I managed to download it locally). then it switched to alt download link "https://dotnetcli.azureedge.net/dotnet/Sdk/2.0.0-preview3-006785/dotnet-dev-win-x64.2.0.0-preview3-006785.zip" - which seems to be not working. @HaoK - maybe we just need to try to re-run the AppVeyor build? |
|
@HaoK - did you have a chance to take a look at this PR? |
|
I took a look, but we haven't signed up to do this work yet, so drilling into the details/implementation is a bit premature until we actually are trying to get this feature implemented. |
|
Do you mean that this feature is not in roadmap? Can we do anything in order to sign this up? |
|
Yes, feedback/comments in #227 saying why you want/need this feature will help get it prioritized higher/sooner. |
|
This issue was moved to dotnet/aspnetcore#2392 |
|
Sorry I was a bad bot. Re-opening this PR. |
|
See #266 for initial validation support |
This PR introduces the possibility to 'initialize' options at the start up time. It will prevent possible config issues that might occur in runtime because of incorrect config file.
Also, it allows to use initializers to write some custom logic to instantiate options objects.