Add Initialize<TOptions> #187

HaoK · 2017-04-26T19:57:04Z

Needed to replace double locking/options fixup in auth as we discussed last week.

MaKCbIMKo · 2017-04-27T13:27:40Z

For me it looks like the same as Configure step (does the same). But another step right after the configuration one. Is the main goal of that is make sure that it will be called right after all configurations?

What if something will require step number 3 (PostConfigure or something else)?

Maybe need to create more complex solution? I know that's too much, but more long-term solution.

Eilon · 2017-04-27T16:20:51Z

src/Microsoft.Extensions.Options/OptionsFactory.cs


        /// <summary>
        /// Initializes a new instance with the specified options configurations.
        /// </summary>
        /// <param name="setups">The configuration actions to run.</param>
-        public OptionsFactory(IEnumerable<IConfigureOptions<TOptions>> setups)


Should we keep the old ctor for compat, and have it chain to the new ctor? DI would only call the new ctor anyway, but it would avoid a breaking change.

We added this type in 2.0 so no breaking change thankfully

Eilon · 2017-04-27T16:22:07Z

src/Microsoft.Extensions.Options/OptionsServiceCollectionExtensions.cs

@@ -69,5 +69,43 @@ public static IServiceCollection Configure<TOptions>(this IServiceCollection ser

        public static IServiceCollection ConfigureAll<TOptions>(this IServiceCollection services, Action<TOptions> configureOptions) where TOptions : class
            => services.Configure(name: null, configureOptions: configureOptions);
+
+        /// <summary>
+        /// Registers an action used to Initialize a particular type of options.


Don't need to capitalize the I of Initialize (it's just a word here, not a method name).

(Applies to other doc comments in this PR.)

Yeah I'll fix, global find/replace fail, I just cloned the ConfigureOptions files and replaced

Eilon · 2017-04-27T16:25:12Z

src/Microsoft.Extensions.Options/OptionsServiceCollectionExtensions.cs

+        /// <param name="name">The name of the options instance.</param>
+        /// <param name="InitializeOptions">The action used to Initialize the options.</param>
+        /// <returns>The <see cref="IServiceCollection"/> so that additional calls can be chained.</returns>
+        public static IServiceCollection Initialize<TOptions>(this IServiceCollection services, string name, Action<TOptions> InitializeOptions)


Rename InitializeOptions param to start with lowercase letter.

Eilon · 2017-04-27T16:25:33Z

src/Microsoft.Extensions.Options/InitializeOptions.cs

+    public class InitializeOptions<TOptions> : IInitializeOptions<TOptions> where TOptions : class
+    {
+        /// <summary>
+        /// Constructor.


Creates a new instance of...

Eilon · 2017-04-27T16:26:11Z

src/Microsoft.Extensions.Options/InitializeOptions.cs

+        /// <param name="action">The action to register.</param>
+        public InitializeOptions(string name, Action<TOptions> action)
+        {
+            Name = name;


Do these params need any checks? Can they be empty? Null? Anything?

null name signify's applying to all names.
string.empty is used to target the 'default' name, i.e. the overloads that don't specify name

Passing in a null action is equivalent to passing in a no-op o => { }, so I don't think its something we explicitly need to block (we didn't for the existing https://github.com/aspnet/Options/blob/dev/src/Microsoft.Extensions.Options/ConfigureOptions.cs, so we need to be consistent) Note: we do require non null actions on the sugar Configure/Initialize apis since it doesn't make sense to add no-op/null actions there.

Eilon · 2017-04-27T16:29:46Z

src/Microsoft.Extensions.Options/InitializeOptions.cs

+namespace Microsoft.Extensions.Options
+{
+    /// <summary>
+    /// Implementation of IInitializeNamedOptions.


Can use a typeref for the type in this doc.

Eilon · 2017-04-27T16:36:26Z

test/Microsoft.Extensions.Options.Test/OptionsSnapshotTest.cs

+            services.Configure<FakeOptions>(o => o.Message += "0");
+            services.Configure<FakeOptions>("1", o => o.Message += "1");
+            services.InitializeAll<FakeOptions>(o => o.Message += "Initialize");
+            services.Initialize<FakeOptions>(o => o.Message += "2");


I think I'm just confused, but doesn't this Initialize initialize all FakeOptions, because no name was specified??

Yeah its a bit confusing,

All overloads that don't take a name target the Options.DefaultName = string.Empty; This was done to make all of the existing options.Configures behave the same in Options 2.0 since we added names, and wanted to preserve the existing semantics and behaviors.

I think we should define some non-empty string to have this meaning. Like "Default", or something more specific if we think avoiding collisions is important. The empty-string is very un-documenting.

Yeah let's get that figured out.

Well, the actual value for the default name doesn't matter too much, we use Options.DefaultName everywhere, its currently set to string.empty since that seemed like a good default to me, in general the constant should be used everywhere but if we want to change it, its just changing this line: https://github.com/aspnet/Options/blob/dev/src/Microsoft.Extensions.Options/Options.cs#L11

To clarify, whatever value we pick, its just used as the default for the overloads that don't specify a name, so it doesn't really have any documention value other than if you were debugging a custom IConfigure/InitializeOptions.

I think its pretty clear today if you look at the usage in the overloads with the constant:

https://github.com/aspnet/Options/blob/dev/src/Microsoft.Extensions.Options/OptionsServiceCollectionExtensions.cs#L43

I don't have a strong opinion on the exact name, I just feel strongly about "let's get that figured out" 😄 @ajcvickers ?

I don't feel strongly, so I'll let @HaoK make the call. I would be a little confused if I was attempting to debug code that was using this, or if I passed empty string and go back something, but I guess most people won't do those things.

Okay lets keep it as empty string for now, since this isn't new for Initialize.

empty string is pretty weird name to pick, and you wouldn't actually see any issues with using it as your name, unless you configured using both methods.

services.Configure(o => o.Foo = "bar");
services.Configure("", o => o.Foo = "what??");

HaoK · 2017-05-08T20:50:28Z

Updated to fix casing, also added references in the doc comments for Configure/Initialize to each other and whether they run before/after each other.

Also added a test/fix for a minor issue with old IConfigureOptions being run for all named instances instead of only the default instance

HaoK · 2017-05-15T19:49:16Z

Merged 52ab5f1

dnfclas added the cla-already-signed label Apr 26, 2017

This was referenced Apr 26, 2017

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

Closed

Auth 2.0 Part II: Revenge of AuthZ aspnet/Security#1190

Closed

Eilon suggested changes Apr 27, 2017

View reviewed changes

HaoK added 2 commits May 8, 2017 13:28

Add Initialize<TOptions>

f4cf0e5

PR fixes

00e0b0b

HaoK force-pushed the haok/init branch from d47bb95 to 00e0b0b Compare May 8, 2017 20:29

HaoK added 2 commits May 8, 2017 13:38

Fix for named options and IConfigure

1c541a0

Fix more casing

cadba0e

HaoK mentioned this pull request May 8, 2017

Switch to services.Initialize<TOptions> aspnet/Security#1206

Closed

HaoK closed this May 15, 2017

natemcmaster deleted the haok/init branch November 16, 2017 05:05

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Initialize<TOptions> #187

Add Initialize<TOptions> #187

HaoK commented Apr 26, 2017

MaKCbIMKo commented Apr 27, 2017

Eilon Apr 27, 2017

HaoK Apr 27, 2017

Eilon Apr 27, 2017

Eilon Apr 27, 2017

HaoK Apr 27, 2017

Eilon Apr 27, 2017

Eilon Apr 27, 2017

Eilon Apr 27, 2017

HaoK Apr 27, 2017

Eilon Apr 27, 2017

Eilon Apr 27, 2017

Eilon Apr 27, 2017

HaoK Apr 27, 2017

ajcvickers Apr 27, 2017

Eilon Apr 27, 2017

HaoK Apr 27, 2017

HaoK Apr 27, 2017

HaoK Apr 27, 2017

Eilon Apr 27, 2017

ajcvickers Apr 27, 2017

HaoK Apr 27, 2017 •

edited

Loading

HaoK commented May 8, 2017

HaoK commented May 15, 2017

Add Initialize<TOptions> #187

Add Initialize<TOptions> #187

Conversation

HaoK commented Apr 26, 2017

MaKCbIMKo commented Apr 27, 2017

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 Apr 27, 2017 • edited Loading

Choose a reason for hiding this comment

HaoK commented May 8, 2017

HaoK commented May 15, 2017

HaoK Apr 27, 2017 •

edited

Loading