Options 2.0 - Add support for named options, validation, and caching #176

HaoK · 2017-03-16T19:16:02Z

Iteration 2:

IOptionsFactory is responsible for creating new options instances from the registered Configures
IOptionsValidator is responsible for validation of new options
IOptionsCache is in charge of cache semantics (and has an API that mimics ConcurrentDictionary
IOptionsService puts it all together

@ajcvickers @divega @davidfowl @DamianEdwards @Eilon @Tratcher

MaKCbIMKo

I've got a few questions (see the comments)

MaKCbIMKo · 2017-03-16T19:50:26Z

src/Microsoft.Extensions.Options/OptionsFactory.cs

+            }
+            else
+            {
+                lock (_lock)


Why just not to use ConcurrentDictionary with GetOrAdd method? Looks like it's exactly what's needed.

Good point, will switch to use that

Now I remember why i didn't, Concurrent is in netstandard 1.1, Options currently targets 1.0, I'll see if we can bump it up

MaKCbIMKo · 2017-03-16T20:03:32Z

src/Microsoft.Extensions.Options/OptionsServiceCollectionExtensions.cs

+            return services;
+        }
+
+        public static IServiceCollection ConfigureAll<TOptions>(this IServiceCollection services, Action<TOptions> configureOptions)


What the difference between this method and Configure<TOptions>(this IServiceCollection services, Action<TOptions> configureOptions). One of them can be a sugar for another.

This is configuring all named instances, while the original Configure only targets the 'default' instance.

Are 'default' instance and named with 'null' name the same options? If not, then what the difference?

MaKCbIMKo · 2017-03-16T20:04:33Z

src/Microsoft.Extensions.Options/IConfigureNamedOptions.cs

+    /// Represents something that configures the TOptions type.
+    /// </summary>
+    /// <typeparam name="TOptions"></typeparam>
+    public interface IConfigureNamedOptions<in TOptions> where TOptions : class


With name = null this will be the same like IConfigureOptions. Do we really need both of these interfaces?

The idea is to leave the original interfaces and functionality alone so existing usage of IOptions is completely unchanged. So basically you can think of this is as the IConfigureOptions2 interface. And OptionsFactory2 as IOptions2. So we need to decide how the new stuff interacts with the old (unnamed options). But for now the old stuff completely ignores any of the new apis (expect potentially with ConfigureAll which probably should result in configuring the 'default' option as well.

I think that if all old stuff will use all new new stuff with 'null' name - it will work as expected. Without any problems (except that validation might be applied). The one change is inject factory into OptionsManager and ask for unnamed options.

Am I missing something?
Or the original stuff is left just in case some unexpected issues.

The default instances are only targeted with the old interface. The null name is used to signify special configures that are applied to every named instances.

The null Name is just saving me from having to creating a second ConfigureAllNamedOptions class that eliminates the Name check.

Old API's will do (behave) the same, but only in a bit different way. End users won't notice the different, but they will know that named and unnamed options work alike.

Yes, I can write a helper method that will call Configure for each name that I need (in cycle call Configure for each name with the same action). But having something like: Configure(["name1", "name2"], o => DoStuff()) could help reduce the count of total IConfigureOptions instances (and because we iterate through all of them - potentially reduce the time).

Its easy enough to add sugar that supports configuring multiple names at once in a single IConfigureNamedOptions instance.

It's not just sugar, but it requires to change the interface IConfigureNamedOptions and replace string Name with string[] Names (and change the realization for Configure method a bit).

Its just sugar, the interface shouldn't actually have Name on it, that's a mistake I'll remedy, the name is supplied via Configure/Validate the property is something specific to the implementation, so it should be possible to have a IConfigureNamedOptions that handles as many different names as it likes, it is free to do whatever via its Validate since all of them will be called.

Name is now removed on the interface

MaKCbIMKo · 2017-03-16T20:06:26Z

src/Microsoft.Extensions.Options/IOptionsFactory.cs

+        /// <summary>
+        /// Returns a configured and validated TOptions instance with the given name.
+        /// </summary>
+        TOptions Get(string name);


Maybe we also need a factory for unnamed options? Calling Get(null) isn't obvious.

Possibly, we'd probably just keep the old .Value if we went that route. Or maybe consistently call that the 'default' option

Still don't get why we need to extend an interface for both these scenarios if we could modify it a bit and handle in the same way.

MaKCbIMKo · 2017-03-16T20:18:32Z

We also might need a different name other than Validate since its really not limited to validation, its really a set of Configures that happen latter, where likely usages will be normalizing options in addition to validating them, Validate doesn't signify that these can mutate the options just like configure...

You mean that's possible to change somehow options during validation process?

HaoK · 2017-03-16T20:26:56Z

Yes, the name of the classes is just a suggestion for intent. But its equivalent to Configures that run later, you can happily mutate the options instances as part of 'Validate' and this is somewhat intentional as I use this in the Auth stack to convert nulls to appropriate default values. I guess this is always true in general for options as anyone can manipulate the instance.

MaKCbIMKo · 2017-03-16T20:44:48Z

Then the question: how to only read access of options to validation?
Copy options and validate the copy? or wrap (somehow) options and track the changes (if any)?
Use predicates that only return the value (but it will have the same vulnerability in case if property is complex type).

HaoK · 2017-03-16T22:36:31Z

The goal is actually explicitly the opposite of trying to limit the 'Validation' to being read only. Think of it as a suggestion for what you do in Validate(), its basically a ConfigureLater, or the Initializer concept you started with.

HaoK · 2017-03-16T23:53:12Z

Question, should we make this new options API async at the core? Auth 2.0 is able to call it async, and this would be needed options configuration via DbContext which is what people keep asking about...

MaKCbIMKo · 2017-03-17T00:22:09Z

The goal is actually explicitly the opposite of trying to limit the 'Validation' to being read only. Think of it as a suggestion for what you do in Validate(), its basically a ConfigureLater, or the Initializer concept you started with.

Ok, then we will have two steps, let's say: 1) InitializeOptions and 2) ConfigureOptions. Then we need the step 3) Validation. I don't know if we should have one step that will perform configuration[later] and validation at the same time (for me that's two different concerns that need to be separated)

MaKCbIMKo · 2017-03-17T00:24:51Z

should we make this new options API async at the core?

Why not? I think we could have both APIs. And maybe we could have other situations where async would be helpful.

HaoK · 2017-03-17T05:35:05Z

It looks like we need a bit more functionality for Auth since there's the ability to add new schemes after ConfigureServices, so we're going to need similar functionality as well in options. What this means is additional Add/Remove(string name) methods, and probably a new name, something like:

   // Looks kind of like a cache now as well
   interface IOptionsService(or Manager)<TOptions> {
      TOptions Get(string name); // Cache's the TOptions
      void Add(string name, TOptions options);
      bool Remove(string name); // Would evict any instance, so new Get's would recreate
   }

MaKCbIMKo · 2017-03-17T11:01:15Z

IOptionsService.Get(name) looks similar with IOptionsFactory.Get(name). Only different is that we factory not only returns options, but also creates.

We could have another interface:

interface ICachedOptionsFactory<TOptions> : IOptionsFactory<TOptions>
{
    void Evict(or Reset)(string name);
}

that will extend basic functionality (and basic factory will be injected in cached factory).

what do you think?

Do we really need to separate Get and Add methods?

HaoK · 2017-03-17T17:33:56Z

Yeah I'll update the PR to make things clearer, the Service/Manager is the replacement/rename for OptionsFactory. Add isn't needed for the normal options usage where everything is setup in the container during ConfigureServices. Auth wants to support the ability to add things dynamically, since that's driving this feature, options will also need to support the ability to dynmically add named options after the serviceProvider has already been built.

Tratcher · 2017-03-17T19:24:06Z

The Validate name seems fine. If your validation involves fixup that's OK.

HaoK · 2017-03-17T21:14:13Z

Updated with iteration 2:

IOptionsFactory is responsible for creating new options instances from the registered Configures
IOptionsValidator is responsible for validation of new options
IOptionsCache is in charge of cache semantics (and has an API that mimics ConcurrentDictionary
IOptionsService puts it all together

HaoK · 2017-03-17T21:17:28Z

src/Microsoft.Extensions.Options/Microsoft.Extensions.Options.csproj

@@ -4,7 +4,7 @@

  <PropertyGroup>
    <Description>Provides a strongly typed way of specifying and accessing settings using dependency injection.</Description>
-    <TargetFramework>netstandard1.0</TargetFramework>
+    <TargetFramework>netstandard1.1</TargetFramework>


needed to bump to netstandard 1.1 to use ConcurrentDictionary

MaKCbIMKo

How do we want to handle uncached options?

MaKCbIMKo · 2017-03-18T11:10:29Z

src/Microsoft.Extensions.Options/OptionsServiceCollectionExtensions.cs

-            services.TryAdd(ServiceDescriptor.Singleton(typeof(IOptionsFactory<>), typeof(OptionsFactory<>)));
+            services.TryAdd(ServiceDescriptor.Transient(typeof(IOptionsService<>), typeof(OptionsService<>)));
+            services.TryAdd(ServiceDescriptor.Transient(typeof(IOptionsFactory<>), typeof(OptionsFactory<>)));
+            services.TryAdd(ServiceDescriptor.Transient(typeof(IOptionsValidator<>), typeof(OptionsValidator<>)));


Factory and Validator might be singleton as well (unless we might need to add additional IConfigureOptions and IValidateOptions after Configure step).

Can't be singleton since that would prevent any transient/scoped IConfigure/Validates from having the correct lifetimes, those would become singletons as well.

I think that IConfigure/Validates should be stateless. And if they stateless then it's fine if they are singletons.

Why it might be required to have IConfigure/Validates with different lifetimes? Configure by values from HttpContext? (sounds ugly)

MaKCbIMKo · 2017-03-18T11:16:37Z

src/Microsoft.Extensions.Options/IOptionsCache.cs

+
+        bool TryRemove(string name);
+
+        // Do we need a Clear all?


At some point it might be required.

I also was thinking about using some kind of IEvictionPolicy and TryAdd(TOptions, IEvictionPolicy<TOptions>). In that case we might add ClearEvicted method (perhaps we no need it at all (or no need right now), but just like a thing to consider).

MaKCbIMKo · 2017-03-18T11:32:16Z

src/Microsoft.Extensions.Options/OptionsService.cs

+    /// Implementation of IOptionsFactory.
+    /// </summary>
+    /// <typeparam name="TOptions">The type of options being requested.</typeparam>
+    public class OptionsService<TOptions> : IOptionsService<TOptions> where TOptions : class, new()


This realization of IOptionsService always uses cache. What if we don't want to use cache (for some options)?

Implement a NoCache cache :)

Or implement your own options service that doesn't use the cache, either way will work.

How about something like:

interface IOptionsService<TOptions> { TOptions Get(...) } class OptionsService<TOptions> : IOptionsService<TOptions> { // .... } interface ICachedOptionsService<TOptions> : IOptionsService<TOptions> { // adding cache required stuff (e.g. reset, add etc) } class CachedOptionsService<TOptions> : ICachedOptionsService<TOptions> { ctor(IOptionsService<TOptions> commonService) { // body } // cache related stuff (e.g. reset, add etc) }

In that case we use different entities for different needs.

Thoughts?

You can already replace the service or the cache, so I don't see any reason to have a cached and no cached version. If you don't want to use the cache, just replace the service with one that doesn't use the cache service.

That's a good point.

But it will apply for all place where I'm using this options. What if I need don't use cache only in one place?

Use the factory and validator services directly

That's weird, isn't it?

No weirder than different access patterns for the same options :) Regardless that's not a mainline scenario, as long as its not blocked, seems fine

I'm fine with using factories, validators, configurators directly somewhere in the core or like API for providing custom things, but like API for using options - that's weird.

I agree that having different access patterns is also weird :)

But maybe we can figure out another options to allowing different accesses but with the same pattern? How about attributes?

Something like:

class ThatUsesOptions([OptionsNamed("name")] IOptions<TOptions> options) class ThatUsesOptions([OptionsCached] IOptions<TOptions> options) class ThatUsesOptions([OptionsSnapshot] IOptions<TOptions> options) class ThatUsesOptions([OptionsNotCached] IOptions<TOptions> options)

What do you think?

MaKCbIMKo · 2017-03-18T11:36:48Z

src/Microsoft.Extensions.Options/IOptionsValidator.cs

+    /// Used to validate TOptions instances.
+    /// </summary>
+    /// <typeparam name="TOptions">The type of options being requested.</typeparam>
+    public interface IOptionsValidator<TOptions> where TOptions : class, new()


For now that's fine.

According my PR (#171) there are some changes that might be required to improve validation feature. But it definitely might be applied over this PR, so that's ok.

Yeah so the basic idea is to make the signature of the most basic validation pieces flexible enough so more complex validation can be layered on, but keeping what's in core as simple as possible.

I think that core should be not only simple, but flexible as well in order to build complex validation based on it. Because if it will be only simple then it will become the point that needs to be refactored for enabling different validation scenarios.

Sure, but is there something that you have in mind that we can't accomplish with an Action<TOptions> as a starting point for all validation logic.

Stuff like aggregation of exceptions across multiple validations can be an implementation detail of the IOptionsValidator, but I think its reasonable for the validator to be responsible for generating a proper exception explaining why validation failed.

I think that having aggregated validation result is better. Because if you have a few validators and each will throw an exception, it will require you to run validation a few time to fix all errors (because information about new exceptions you will see only when you fixed previous error).

The same scenario with global validation of options at start up time.

A validator implementation can choose to aggregate the results if it wants by wrapping each action in a try/catch, so its not an issue.

HaoK · 2017-03-18T18:24:00Z

In regards to interop with today's nameless Configures, we could eliminate the ConfigureAll/ValidateAll and treat the overloads that don't specify names as global actions, but that might be super confusing

MaKCbIMKo · 2017-03-18T21:11:27Z

In regards to interop with today's nameless Configures, we could eliminate the ConfigureAll/ValidateAll and treat the overloads that don't specify names as global actions, but that might be super confusing

I like the idea of having only two kind of overloads: 1) for today's/nameless (Configure(...)) and 2) named (Configure(name, ...)).

But it rises two questions for me:

How it will be implemented in core (will it be configured regardless of type)?
In which order apply unnamed and names configurations/validations (unnamed first?)?

HaoK · 2017-03-19T09:51:50Z

Interop would require the new named IConfigures to also implement the original IConfigureOption, so the order would be the enumeration order of the original IConfigureOptions, the options factory would check whether its new or old, and call the appropriate Configure called (named or unnamed).

The downside is the new named options would show up for the old IOptions as well, but we could implement the old method to be a no-op by default.

MaKCbIMKo · 2017-03-19T16:52:26Z

Perhaps we need not new IConfigures implement hte original IConfigureOptions, but vice verse?
I thought that 'named' options are based over unnamed. Am I wrong?

Like:

services.Configure<Options>(...) // configuration 1
services.Configure<Options>(name, ...) // configuration 2

provider.Get<Options>() // returns options with only configuration 1
provider.Get<Options>(name) // returns options with both configurations applied in registered order.

HaoK · 2017-03-19T18:53:34Z

It depends on what we want the old Configure to mean, we can choose for it to be equivalent to ConfigureAll instances, or we can choose for it mean only configure the 'default' instance which we could define to be a null/empty name. I'm not sure which behavior is better honestly.

davidfowl · 2017-03-19T19:09:44Z

src/Microsoft.Extensions.Options/Microsoft.Extensions.Options.csproj

@@ -14,6 +14,7 @@
    <PackageReference Include="Microsoft.Extensions.DependencyInjection.Abstractions" Version="$(AspNetCoreVersion)" />
    <PackageReference Include="Microsoft.Extensions.Primitives" Version="$(AspNetCoreVersion)" />
    <PackageReference Include="System.ComponentModel" Version="$(CoreFxVersion)" />
+    <PackageReference Include="System.Collections.Concurrent" Version="$(CoreFxVersion)" />


Isn't this in NetStandard.Library? Do you actually need to explicitly reference it here?

Oops yeah I tried adding this before realizing that I just needed to bump it up to 1.1 for it to show up, I'll remove

HaoK · 2017-03-21T21:43:03Z

Reviewed current design with @ajcvickers @divega @glennc will replace with next iteration.

Decided to split Validation feature into its own PR that will come later, which should enable the full validate all options scenario at the same time.

Also decided to try and make this change keeping the old IOptions name and modifying the behavior (if we are able to preserve the existing scenarios that IOptionsMonitor/Snapshot, we should obsolete those)

HaoK · 2017-03-21T22:28:40Z

Replaced by #179

OptionsFactory support

40eb483

dnfclas added the cla-already-signed label Mar 16, 2017

HaoK mentioned this pull request Mar 16, 2017

Add possibility to configure options validation in order to validate options at the start up time (not at runtime). #171

Closed

MaKCbIMKo reviewed Mar 16, 2017

View reviewed changes

HaoK mentioned this pull request Mar 16, 2017

Authentication 2.0 Iteration 3 (with Names/OptionFactory) aspnet/Security#1151

Closed

HaoK mentioned this pull request Mar 17, 2017

Rationalize IOptionsMonitor for Options 2.0 #167

Closed

Iteration 2

5fd3090

HaoK changed the title ~~Options 2.0 - Add OptionsFactory support~~ Options 2.0 - Add support for named options, validation, and caching Mar 17, 2017

HaoK commented Mar 17, 2017

View reviewed changes

MaKCbIMKo reviewed Mar 18, 2017

View reviewed changes

davidfowl reviewed Mar 19, 2017

View reviewed changes

Remove concurrent ref

c7bca40

HaoK closed this Mar 21, 2017

aspnet-hello mentioned this pull request Jan 1, 2018

Add possibility to configure options validation in order to validate options at the start up time (not at runtime). dotnet/aspnetcore#2392

Closed

Options 2.0 - Add support for named options, validation, and caching #176

Options 2.0 - Add support for named options, validation, and caching #176

Conversation

HaoK commented Mar 16, 2017 • edited Loading

MaKCbIMKo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaKCbIMKo commented Mar 16, 2017

HaoK commented Mar 16, 2017

MaKCbIMKo commented Mar 16, 2017

HaoK commented Mar 16, 2017

HaoK commented Mar 16, 2017

MaKCbIMKo commented Mar 17, 2017

MaKCbIMKo commented Mar 17, 2017

HaoK commented Mar 17, 2017 • edited Loading

MaKCbIMKo commented Mar 17, 2017

HaoK commented Mar 17, 2017 • edited Loading

Tratcher commented Mar 17, 2017

HaoK commented Mar 17, 2017

Choose a reason for hiding this comment

MaKCbIMKo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoK commented Mar 18, 2017

MaKCbIMKo commented Mar 18, 2017

HaoK commented Mar 19, 2017

MaKCbIMKo commented Mar 19, 2017

HaoK commented Mar 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoK commented Mar 21, 2017

HaoK commented Mar 21, 2017

HaoK commented Mar 16, 2017 •

edited

Loading

HaoK commented Mar 17, 2017 •

edited

Loading

HaoK commented Mar 17, 2017 •

edited

Loading