-
Notifications
You must be signed in to change notification settings - Fork 68
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
||
namespace Microsoft.Extensions.Options | ||
{ | ||
/// <summary> | ||
/// Represents something that initializes the TOptions type. | ||
/// Note: These are run after all <see cref="IConfigureOptions{TOptions}"/>. | ||
/// </summary> | ||
/// <typeparam name="TOptions"></typeparam> | ||
public interface IInitializeOptions<in TOptions> where TOptions : class | ||
{ | ||
/// <summary> | ||
/// Invoked to initialize a TOptions instance. | ||
/// </summary> | ||
/// <param name="name">The name of the options instance being initialized.</param> | ||
/// <param name="options">The options instance to initialize.</param> | ||
void Initialize(string name, TOptions options); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
||
using System; | ||
|
||
namespace Microsoft.Extensions.Options | ||
{ | ||
/// <summary> | ||
/// Implementation of <see cref="IInitializeOptions{TOptions}"/>. | ||
/// </summary> | ||
/// <typeparam name="TOptions"></typeparam> | ||
public class InitializeOptions<TOptions> : IInitializeOptions<TOptions> where TOptions : class | ||
{ | ||
/// <summary> | ||
/// Creates a new instance of <see cref="InitializeOptions{TOptions}"/>. | ||
/// </summary> | ||
/// <param name="name">The name of the options.</param> | ||
/// <param name="action">The action to register.</param> | ||
public InitializeOptions(string name, Action<TOptions> action) | ||
{ | ||
Name = name; | ||
Action = action; | ||
} | ||
|
||
/// <summary> | ||
/// The options name. | ||
/// </summary> | ||
public string Name { get; } | ||
|
||
/// <summary> | ||
/// The initialization action. | ||
/// </summary> | ||
public Action<TOptions> Action { get; } | ||
|
||
/// <summary> | ||
/// Invokes the registered initialization Action if the name matches. | ||
/// </summary> | ||
/// <param name="name"></param> | ||
/// <param name="options"></param> | ||
public virtual void Initialize(string name, TOptions options) | ||
{ | ||
if (options == null) | ||
{ | ||
throw new ArgumentNullException(nameof(options)); | ||
} | ||
|
||
// Null name is used to initialize all named options. | ||
if (Name == null || name == Name) | ||
{ | ||
Action?.Invoke(options); | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,14 +12,17 @@ namespace Microsoft.Extensions.Options | |
public class OptionsFactory<TOptions> : IOptionsFactory<TOptions> where TOptions : class, new() | ||
{ | ||
private readonly IEnumerable<IConfigureOptions<TOptions>> _setups; | ||
private readonly IEnumerable<IInitializeOptions<TOptions>> _initializers; | ||
|
||
/// <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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We added this type in 2.0 so no breaking change thankfully |
||
/// <param name="initializers">The initialization actions to run.</param> | ||
public OptionsFactory(IEnumerable<IConfigureOptions<TOptions>> setups, IEnumerable<IInitializeOptions<TOptions>> initializers) | ||
{ | ||
_setups = setups; | ||
_initializers = initializers; | ||
} | ||
|
||
public TOptions Create(string name) | ||
|
@@ -31,11 +34,15 @@ public TOptions Create(string name) | |
{ | ||
namedSetup.Configure(name, options); | ||
} | ||
else | ||
else if (name == Options.DefaultName) | ||
{ | ||
setup.Configure(options); | ||
} | ||
} | ||
foreach (var init in _initializers) | ||
{ | ||
init.Initialize(name, options); | ||
} | ||
return options; | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -208,6 +208,107 @@ public void CanConfigureAllDefaultAndNamedOptions() | |
Assert.Equal("Default1", option.Get("1").Message); | ||
} | ||
|
||
[Fact] | ||
public void CanConfigureAndInitializeAllDefaultAndNamedOptions() | ||
{ | ||
var services = new ServiceCollection().AddOptions(); | ||
services.ConfigureAll<FakeOptions>(o => o.Message += "Default"); | ||
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"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I'm just confused, but doesn't this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah let's get that figured out. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think its pretty clear today if you look at the usage in the overloads with the constant: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't have a strong opinion on the exact name, I just feel strongly about "let's get that figured out" 😄 @ajcvickers ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.Initialize<FakeOptions>("1", o => o.Message += "3"); | ||
|
||
var sp = services.BuildServiceProvider(); | ||
var option = sp.GetRequiredService<IOptionsSnapshot<FakeOptions>>(); | ||
Assert.Equal("DefaultInitialize", option.Get("Default").Message); | ||
Assert.Equal("Default0Initialize2", option.Value.Message); | ||
Assert.Equal("Default1Initialize3", option.Get("1").Message); | ||
} | ||
|
||
[Fact] | ||
public void CanInitializeAllOptions() | ||
{ | ||
var services = new ServiceCollection().AddOptions(); | ||
services.InitializeAll<FakeOptions>(o => o.Message = "Default"); | ||
|
||
var sp = services.BuildServiceProvider(); | ||
var option = sp.GetRequiredService<IOptionsSnapshot<FakeOptions>>(); | ||
Assert.Equal("Default", option.Get("1").Message); | ||
Assert.Equal("Default", option.Get("2").Message); | ||
} | ||
|
||
[Fact] | ||
public void CanConfigureAndInitializeAllOptions() | ||
{ | ||
var services = new ServiceCollection().AddOptions(); | ||
services.ConfigureAll<FakeOptions>(o => o.Message = "D"); | ||
services.InitializeAll<FakeOptions>(o => o.Message += "f"); | ||
services.ConfigureAll<FakeOptions>(o => o.Message += "e"); | ||
services.InitializeAll<FakeOptions>(o => o.Message += "ault"); | ||
|
||
var sp = services.BuildServiceProvider(); | ||
var option = sp.GetRequiredService<IOptionsSnapshot<FakeOptions>>(); | ||
Assert.Equal("Default", option.Get("1").Message); | ||
Assert.Equal("Default", option.Get("2").Message); | ||
} | ||
|
||
[Fact] | ||
public void NamedSnapshotsInitializesInRegistrationOrder() | ||
{ | ||
var services = new ServiceCollection().AddOptions(); | ||
services.Initialize<FakeOptions>("-", o => o.Message += "-"); | ||
services.InitializeAll<FakeOptions>(o => o.Message += "A"); | ||
services.Initialize<FakeOptions>("+", o => o.Message += "+"); | ||
services.InitializeAll<FakeOptions>(o => o.Message += "B"); | ||
services.InitializeAll<FakeOptions>(o => o.Message += "C"); | ||
services.Initialize<FakeOptions>("+", o => o.Message += "+"); | ||
services.Initialize<FakeOptions>("-", o => o.Message += "-"); | ||
|
||
var sp = services.BuildServiceProvider(); | ||
var option = sp.GetRequiredService<IOptionsSnapshot<FakeOptions>>(); | ||
Assert.Equal("ABC", option.Get("1").Message); | ||
Assert.Equal("A+BC+", option.Get("+").Message); | ||
Assert.Equal("-ABC-", option.Get("-").Message); | ||
} | ||
|
||
[Fact] | ||
public void CanInitializeAllDefaultAndNamedOptions() | ||
{ | ||
var services = new ServiceCollection().AddOptions(); | ||
services.InitializeAll<FakeOptions>(o => o.Message += "Default"); | ||
services.Initialize<FakeOptions>(o => o.Message += "0"); | ||
services.Initialize<FakeOptions>("1", o => o.Message += "1"); | ||
|
||
var sp = services.BuildServiceProvider(); | ||
var option = sp.GetRequiredService<IOptionsSnapshot<FakeOptions>>(); | ||
Assert.Equal("Default", option.Get("Default").Message); | ||
Assert.Equal("Default0", option.Value.Message); | ||
Assert.Equal("Default1", option.Get("1").Message); | ||
} | ||
|
||
[Fact] | ||
public void CustomIConfigureOptionsShouldOnlyAffectDefaultInstance() | ||
{ | ||
var services = new ServiceCollection().AddOptions(); | ||
services.AddSingleton<IConfigureOptions<FakeOptions>, CustomSetup>(); | ||
|
||
var sp = services.BuildServiceProvider(); | ||
var option = sp.GetRequiredService<IOptionsSnapshot<FakeOptions>>(); | ||
Assert.Equal("", option.Get("NotDefault").Message); | ||
Assert.Equal("Stomp", option.Get(Options.DefaultName).Message); | ||
Assert.Equal("Stomp", option.Value.Message); | ||
Assert.Equal("Stomp", sp.GetRequiredService<IOptions<FakeOptions>>().Value.Message); | ||
} | ||
|
||
private class CustomSetup : IConfigureOptions<FakeOptions> | ||
{ | ||
public void Configure(FakeOptions options) | ||
{ | ||
options.Message = "Stomp"; | ||
} | ||
} | ||
|
||
[Fact] | ||
public void EnsureAddOptionsLifetimes() | ||
{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these params need any checks? Can they be empty? Null? Anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.