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

Options: Validate named registrations #45671

Closed
aelij opened this issue Dec 7, 2020 · 12 comments
Closed

Options: Validate named registrations #45671

aelij opened this issue Dec 7, 2020 · 12 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Extensions-Options untriaged New issue has not been triaged by the area owner

Comments

@aelij
Copy link
Contributor

aelij commented Dec 7, 2020

Background and Motivation

Currently the Options API will return a new instance of the options class even if it was not configured. This is especially problematic with named options, where a misconfiguration can easily occur. This is compounded by the fact that validation is configured per named instance. I suggest a way to validate that a named option has been explicitly registered.

Proposed API

namespace Microsoft.Extensions.Options
{
    public class OptionsBuilder<T> {
+        ValidateNames();
    }
}

Usage Examples

var services = new ServiceCollection();
services.AddOptions<MyOptions>().ValidateNames();
services.AddOptions<MyOptions>("X").Configure(o => { ... });
var serviceProvider = services.BuildServiceProvider();
var myOptions = serviceProvider.GetService<IOptionsMonitor<MyOptions>>();
myOptions.Get("X") // returns value
myOptions.Get("Y") // throws

Alternative Designs

Another option is to allow validations to be defined for all instances. Currently there's a ConfigureAll method that allows defining an action that would run on all named options, but no API to configure validation for all instances. So we could have something like AddAllOptions (not the best name, I know):

services.AddAllOptions<MyOptions>().ValidateDataAnnotations();

Risks

None - this behavior does not change existing options as it must be opted into.

@aelij aelij added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Dec 7, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-Extensions-Options untriaged New issue has not been triaged by the area owner labels Dec 7, 2020
@ghost
Copy link

ghost commented Dec 7, 2020

Tagging subscribers to this area: @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and Motivation

Currently the Options API will return a new instance of the options class even if it was not configured. This is especially problematic with named options, where a misconfiguration can easily occur. This is compounded by the fact that validation is configured per named instance. I suggest a way to validate that a named option has been explicitly registered.

Proposed API

namespace Microsoft.Extensions.Options
{
    public class OptionsBuilder<T> {
+        ValidateNames();
    }
}

Usage Examples

var services = new ServiceCollection();
services.AddOptions<MyOptions>().ValidateNames();
services.AddOptions<MyOptions>("X").Configure(o => { ... });
var serviceProvider = services.BuildServiceProvider();
var myOptions = serviceProvider.GetService<IOptionsMonitor<MyOptions>>();
myOptions.Get("X") // returns value
myOptions.Get("Y") // throws

-->

Alternative Designs

Another option is to allow validations to be defined for all instances. Currently there's a ConfigureAll method that allows defining an action that would run on all named options, but no API to configure validation for all instances. So we could have something like AddAllOptions (not the best name, I know):

services.AddAllOptions<MyOptions>().ValidateDataAnnotations();

Risks

None - this behavior does not change existing options as it must be opted into.

Author: aelij
Assignees: -
Labels:

api-suggestion, area-Extensions-Options, untriaged

Milestone: -

@maryamariyan
Copy link
Member

We added a new feature into .NET 6 Preview 2 for allowing options validation also for named options. For more information check out this original issue: #36391

Closing as fixed.

@aelij
Copy link
Contributor Author

aelij commented Feb 28, 2021

@maryamariyan I took a quick look at the PR and I don't think this use case is covered.

It's not about validating the options at start-up, it's validating that only pre-registered names can be used when calling IOptions<T>.Get(string).

An unconfigured named option is very hard to debug without this.

@davidfowl
Copy link
Member

We would need to add another API for that. This one can't cover arbitrary names

@aelij
Copy link
Contributor Author

aelij commented Mar 2, 2021

@maryamariyan Could you please reopen the issue?

@davidfowl
Copy link
Member

davidfowl commented Mar 2, 2021

How would this work if all names aren't known up front?

@aelij
Copy link
Contributor Author

aelij commented Mar 2, 2021

In our use case all names are registered to the DI engine.

I experimented a bit, and it turns out the following works for validating all named options (the alternate design I suggested):

services.AddSingleton<IValidateOptions<MyOptions>>(new ValidateOptions<MyOptions>(null, o => Validate(o), "invalid"));
services.AddSingleton<IValidateOptions<MyOptions>>(new DataAnnotationValidateOptions<MyOptions>(null))

But the API is not easily discoverable (passing null as name). Maybe consider adding extension methods for the above?

@davidfowl
Copy link
Member

I'm not sure I'm following... What works. Can you describe the end to end here?

@aelij
Copy link
Contributor Author

aelij commented Mar 2, 2021

The problem we had was that an unexpected name was used, which retrieved an unconfigured and unvalidated instance of an option.

I suggested 2 solutions (not mutually exclusive):

  • Ensure that only names that were registered could be fetched from IOptionsMonitor<T>
  • Enforce validation for all instances, whether pre-registered or not

The latter is possible with the above code sample.

@davidfowl
Copy link
Member

Enforce validation for all instances, whether pre-registered or not

And this has nothing to do with validation on startup right?

@aelij
Copy link
Contributor Author

aelij commented Mar 2, 2021

Right. Validating this at start-up could be an added bonus.

@davidfowl
Copy link
Member

Right. Validating this at start-up could be an added bonus.

Got it, this only works when you specify which names up front.

@ghost ghost locked as resolved and limited conversation to collaborators Apr 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Extensions-Options untriaged New issue has not been triaged by the area owner
Projects
None yet
Development

No branches or pull requests

4 participants