Skip to content
This repository was archived by the owner on Nov 7, 2018. It is now read-only.

Options 2.0 Iteration 3 - Breaking change edition: Takeover IOptions #179

Closed
wants to merge 10 commits into from
Closed

Options 2.0 Iteration 3 - Breaking change edition: Takeover IOptions #179

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
40eb483
OptionsFactory support
HaoK Mar 16, 2017
5fd3090
Iteration 2
HaoK Mar 17, 2017
c7bca40
Remove concurrent ref
HaoK Mar 20, 2017
3cdfe99
Takeover IOptions
HaoK Mar 21, 2017
9600087
Split Get from IOptions => IOptionsManager
HaoK Mar 23, 2017
4ba6b21
Fix config to use named
HaoK Mar 23, 2017
563ea44
Retreat to IOptionsSnapshot
HaoK Mar 24, 2017
145337c
Add test for default + named + all
HaoK Mar 24, 2017
e80c497
Wireup snapshot to config changes
HaoK Mar 27, 2017
3d10f70
Update lifetimes
HaoK Mar 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion src/Microsoft.Extensions.Options.ConfigurationExtensions/ConfigurationChangeTokenSource.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,15 @@ public class ConfigurationChangeTokenSource<TOptions> : IOptionsChangeTokenSourc
/// Constructor taking the IConfiguration instance to watch.
/// </summary>
/// <param name="config">The configuration instance.</param>
public ConfigurationChangeTokenSource(IConfiguration config)
public ConfigurationChangeTokenSource(IConfiguration config) : this(Options.DefaultName, config)
{ }

/// <summary>
/// Constructor taking the IConfiguration instance to watch.
/// </summary>
/// <param name="name">The name of the options instance being watche.</param>
/// <param name="config">The configuration instance.</param>
public ConfigurationChangeTokenSource(string name, IConfiguration config)
{
if (config == null)
{
Expand All @@ -28,6 +36,8 @@ public ConfigurationChangeTokenSource(IConfiguration config)
_config = config;
}

public string Name { get; }

/// <summary>
/// Returns the reloadToken from IConfiguration.
/// </summary>
Expand Down
7 changes: 4 additions & 3 deletions ...Microsoft.Extensions.Options.ConfigurationExtensions/ConfigureFromConfigurationOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,16 @@ namespace Microsoft.Extensions.Options
/// Configures an option instance by using ConfigurationBinder.Bind against an IConfiguration.
/// </summary>
/// <typeparam name="TOptions">The type of options to bind.</typeparam>
public class ConfigureFromConfigurationOptions<TOptions> : ConfigureOptions<TOptions>
public class ConfigureFromConfigurationOptions<TOptions> : ConfigureNamedOptions<TOptions>
where TOptions : class
{
/// <summary>
/// Constructor that takes the IConfiguration instance to bind against.
/// </summary>
/// <param name="name">The name of the options instance.</param>
/// <param name="config">The IConfiguration instance.</param>
public ConfigureFromConfigurationOptions(IConfiguration config)
: base(options => ConfigurationBinder.Bind(config, options))
public ConfigureFromConfigurationOptions(string name, IConfiguration config)
: base(name, options => ConfigurationBinder.Bind(config, options))
{
if (config == null)
{
Expand Down
17 changes: 14 additions & 3 deletions ...nsions.Options.ConfigurationExtensions/OptionsConfigurationServiceCollectionExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,18 @@ public static class OptionsConfigurationServiceCollectionExtensions
/// <param name="services">The <see cref="IServiceCollection"/> to add the services to.</param>
/// <param name="config">The configuration being bound.</param>
/// <returns>The <see cref="IServiceCollection"/> so that additional calls can be chained.</returns>
public static IServiceCollection Configure<TOptions>(this IServiceCollection services, IConfiguration config)
public static IServiceCollection Configure<TOptions>(this IServiceCollection services, IConfiguration config) where TOptions : class
=> services.Configure<TOptions>(Options.Options.DefaultName, config);

/// <summary>
/// Registers a configuration instance which TOptions will bind against.
/// </summary>
/// <typeparam name="TOptions">The type of options being configured.</typeparam>
/// <param name="services">The <see cref="IServiceCollection"/> to add the services to.</param>
/// <param name="name">The name of the options instance.</param>
/// <param name="config">The configuration being bound.</param>
/// <returns>The <see cref="IServiceCollection"/> so that additional calls can be chained.</returns>
public static IServiceCollection Configure<TOptions>(this IServiceCollection services, string name, IConfiguration config)
where TOptions : class
{
if (services == null)
Expand All @@ -32,8 +43,8 @@ public static IServiceCollection Configure<TOptions>(this IServiceCollection ser
throw new ArgumentNullException(nameof(config));
}

services.AddSingleton<IOptionsChangeTokenSource<TOptions>>(new ConfigurationChangeTokenSource<TOptions>(config));
return services.AddSingleton<IConfigureOptions<TOptions>>(new ConfigureFromConfigurationOptions<TOptions>(config));
services.AddSingleton<IOptionsChangeTokenSource<TOptions>>(new ConfigurationChangeTokenSource<TOptions>(name, config));
return services.AddSingleton<IConfigureOptions<TOptions>>(new ConfigureFromConfigurationOptions<TOptions>(name, config));
}
}
}
56 changes: 56 additions & 0 deletions src/Microsoft.Extensions.Options/ConfigureNamedOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
// 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 IConfigureNamedOptions.
/// </summary>
/// <typeparam name="TOptions"></typeparam>
public class ConfigureNamedOptions<TOptions> : IConfigureNamedOptions<TOptions>, IConfigureOptions<TOptions> where TOptions : class
{
/// <summary>
/// Constructor.
/// </summary>
/// <param name="name">The name of the options.</param>
/// <param name="action">The action to register.</param>
public ConfigureNamedOptions(string name, Action<TOptions> action)
{
Name = name;
Action = action;
}

/// <summary>
/// The options name.
/// </summary>
public string Name { get; }

/// <summary>
/// The configuration action.
/// </summary>
public Action<TOptions> Action { get; }

/// <summary>
/// Invokes the registered configure Action if the name matches.
/// </summary>
/// <param name="name"></param>
/// <param name="options"></param>
public virtual void Configure(string name, TOptions options)
{
if (options == null)
{
throw new ArgumentNullException(nameof(options));
}

// Null name is used to configure all named options.
if (Name == null || name == Name)
{
Action?.Invoke(options);
}
}

public void Configure(TOptions options) => Configure(Options.DefaultName, options);
}
}
9 changes: 2 additions & 7 deletions src/Microsoft.Extensions.Options/ConfigureOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,6 @@ public class ConfigureOptions<TOptions> : IConfigureOptions<TOptions> where TOpt
/// <param name="action">The action to register.</param>
public ConfigureOptions(Action<TOptions> action)
{
if (action == null)
{
throw new ArgumentNullException(nameof(action));
}

Action = action;
}

Expand All @@ -31,7 +26,7 @@ public ConfigureOptions(Action<TOptions> action)
public Action<TOptions> Action { get; }

/// <summary>
/// Invokes the registered configure Action.
/// Invokes the registered configure Action if the name matches.
/// </summary>
/// <param name="options"></param>
public virtual void Configure(TOptions options)
Expand All @@ -41,7 +36,7 @@ public virtual void Configure(TOptions options)
throw new ArgumentNullException(nameof(options));
}

Action.Invoke(options);
Action?.Invoke(options);
}
}
}
19 changes: 19 additions & 0 deletions src/Microsoft.Extensions.Options/IConfigureNamedOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// 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 configures the TOptions type.
/// </summary>
/// <typeparam name="TOptions"></typeparam>
public interface IConfigureNamedOptions<in TOptions> : IConfigureOptions<TOptions> where TOptions : class
{
/// <summary>
/// Invoked to configure a TOptions instance.
/// </summary>
/// <param name="name">The name of the options instance being configured.</param>
/// <param name="options">The options instance to configure.</param>
void Configure(string name, TOptions options);
}
}
6 changes: 3 additions & 3 deletions src/Microsoft.Extensions.Options/IOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) .NET Foundation. All rights reserved.
// 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
Expand All @@ -7,10 +7,10 @@ namespace Microsoft.Extensions.Options
/// Used to retreive configured TOptions instances.
/// </summary>
/// <typeparam name="TOptions">The type of options being requested.</typeparam>
public interface IOptions<out TOptions> where TOptions : class, new()
public interface IOptions<TOptions> where TOptions : class, new()
Copy link

@MaKCbIMKo MaKCbIMKo Mar 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagine that we might have a place that requires to have not only the options value, but also the API to operate with options (Get, Add, Remove etc). I also agree that this interface is backward-compatible.

But I think that this interface violates with Interface Segregation principle. If I understand it right, methods (Get, Add, Remove) are required for Auth stuff. But for just retrieving options values isn't required.

Maybe create separated interface IOptionsService with methods (Get, Add, Remove). For basic use-cases inject it in IOptions implementation. For Auth - inject the service itself and operate with options.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well before, the reason Add was on the main interface was because it needed to do validation using the validator before inserting into the cache. Now that validation is no longer part of this PR, consumers can directly insert what they want into the cache (using the options factory to create it), so for now, its probably fine dropping Add/Remove

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think that having Get(name) method in end-user interface is fine?
Should options consumer decide which options is needed? Or DI engine should have an ability and all required information for choosing the right named options ? (and consumer will just consume)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public interface IOptions where TOptions : class, new() [](start = 4, length = 65)

Have we considered having INamedOptions extend from IOptions and adds the Get method? With the removal of Add and Remove methods, that would mean we would not break current implementors of IOptions while still adding new functionality in 2.0 for those that implement INamedOptions. It also should not result in two parallel sets of APIs, like the previous PR had. And it seems like the two interfaces and their relationship are a reasonable separation of concerns that we might make even if designing this from scratch.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's closer to the previous iterations where we had a new Options 2.0 interface basically. I definitely prefer not breaking ideally, since already this is a PITA fixing the options usages all over the place. So I thought the concern around potential confusion was related to the usage of the new OptionsFactory/OptionsCache, if we normalizing both IOptions + IOptionsGet to use the new implementation, that sounds good to me as well.

Should we consider a more generic name than INamedOptions since we potentially are looking into adding the validation stuff back soon, I'd hate to rename it right away...

My preference would be to name it something that implies additional logic is being applied on top of IOptions, i.e. IOptionsManager/IOptionsService.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@HaoK IOptions<out T> would remain what it is and INamedOptions<out T>: IOptions<T> would be used to consume named options. If we add validation again, we should add it to both. Then I don't see the advantage of having a new name that implies anything different.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a problem with splitting Get into its own interface, I just really don't like INamedOptions (which I already used in a previous iteration, it did not grow on me at all...)

Copy link
Member Author

@HaoK HaoK Mar 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also are we still trying to eliminate IOptionsSnapshot/IOptionsMonitor assuming there are sane workarounds which can be implemented via IOptionsFactory/Cache?

Copy link

@divega divega Mar 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the behavior difference between IOptions and IOptionsSnapshot is that IOptions will never change, its a cached singleton that never gets updated.

So IOptions might have been a singleton, and aTOptions once returned will never be mutated (by the Options feature at least) but but a new TOptions instance with new values will be returned after the cache is invalidated. Is this correct? If yes, I agree using a method is more appropriate than the Value property. Not sure that determines that the method should always take a name (seems to be an orthogonal question).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, the behavior of the new IOptions.Value implementation that is backed by the IOptionsCache is fully dependent on what's in the cache.

The names stuff is more or less orthogonal, as currently .Value is just returning .Get(string.Empty) name

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still think maybe we should consider just adding Get(name) to IOptions, but not break IConfigureOptions as even though we are changing the IOptions interface), there's not many implementations of IOptions today, while implementing your own IConfigureOptions was a major extensibility point which we should avoid breaking

Ah, so you are actually proposing keeping the Value property and adding the method alongside it, correct? Otherwise we would break all consumers of IOptions. I don't like it being a property if it will potentially return a different instance sometimes when invoked, but it may be just me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah well that's why I was initially proposing we leave IOptions alone entirely and keep the old semantics, and introduce IOptionsManager with the new semantics and no .Value.

Copy link
Member Author

@HaoK HaoK Mar 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we wanted to be crystal clear about what's old and new, we could leave Options alone entirely, and add all the new stuff in a new Options.Manager package, which would reuse only a few things like IConfigureOptions/Configure

{
/// <summary>
/// The configured TOptions instance.
/// The default configured TOptions instance, equivalent to Get(string.Empty).
/// </summary>
TOptions Value { get; }
}
Expand Down
22 changes: 22 additions & 0 deletions src/Microsoft.Extensions.Options/IOptionsCache.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// 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>
/// Used to cache TOptions instances.
/// </summary>
/// <typeparam name="TOptions">The type of options being requested.</typeparam>
public interface IOptionsCache<TOptions> where TOptions : class
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this public? Where would you ever use this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

People were asking for this to be exposed so they can plug in their own caching logic, i.e. using the memory cache to store options and control their own eviction policies, #167

Our default cache will at least have to support the existing reload on change behavior that will refresh any options that are bound to config. I'll be taking a look at that next.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that should be fairly straightforward, and is just hooking up the config OnReload changeTokens to call Remove on the IOptionsCache

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hope is that with this new factoring with IOptionsCache, we can obsolete/delete IOptionsMonitor/IOptionsSnapshot, to only have one IOptions to rule them all. (with IOptionsFactory + potentially in a later PR IOptionsValidator as ringwraiths)

{
TOptions GetOrAdd(string name, Func<TOptions> createOptions);

bool TryAdd(string name, TOptions options);

bool TryRemove(string name);

// Do we need a Clear all?
}
}
5 changes: 5 additions & 0 deletions src/Microsoft.Extensions.Options/IOptionsChangeTokenSource.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,5 +16,10 @@ public interface IOptionsChangeTokenSource<out TOptions>
/// </summary>
/// <returns></returns>
IChangeToken GetChangeToken();

/// <summary>
/// The name of the option instance being changed.
/// </summary>
string Name { get; }
}
}
17 changes: 17 additions & 0 deletions src/Microsoft.Extensions.Options/IOptionsFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// 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>
/// Used to create TOptions instances.
/// </summary>
/// <typeparam name="TOptions">The type of options being requested.</typeparam>
public interface IOptionsFactory<TOptions> where TOptions : class, new()
{
/// <summary>
/// Returns a configured TOptions instance with the given name.
/// </summary>
TOptions Create(string name);
}
}
7 changes: 3 additions & 4 deletions src/Microsoft.Extensions.Options/IOptionsSnapshot.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,11 @@ namespace Microsoft.Extensions.Options
/// Used to access the value of TOptions for the lifetime of a request.
/// </summary>
/// <typeparam name="TOptions"></typeparam>
public interface IOptionsSnapshot<out TOptions>
public interface IOptionsSnapshot<TOptions> : IOptions<TOptions> where TOptions : class, new()
{
/// <summary>
/// Returns the value of the TOptions which will be computed once
/// Returns a configured TOptions instance with the given name.
/// </summary>
/// <returns></returns>
TOptions Value { get; }
TOptions Get(string name);
}
}
49 changes: 49 additions & 0 deletions src/Microsoft.Extensions.Options/LegacyOptionsCache.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
// 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;
using System.Collections.Generic;
using System.Threading;

namespace Microsoft.Extensions.Options
{
internal class LegacyOptionsCache<TOptions> where TOptions : class, new()
{
private readonly Func<TOptions> _createCache;
private object _cacheLock = new object();
private bool _cacheInitialized;
private TOptions _options;
private IEnumerable<IConfigureOptions<TOptions>> _setups;

public LegacyOptionsCache(IEnumerable<IConfigureOptions<TOptions>> setups)
{
_setups = setups;
_createCache = CreateOptions;
}

private TOptions CreateOptions()
{
var result = new TOptions();
if (_setups != null)
{
foreach (var setup in _setups)
{
setup.Configure(result);
}
}
return result;
}

public virtual TOptions Value
{
get
{
return LazyInitializer.EnsureInitialized(
ref _options,
ref _cacheInitialized,
ref _cacheLock,
_createCache);
}
}
}
}
5 changes: 2 additions & 3 deletions src/Microsoft.Extensions.Options/Microsoft.Extensions.Options.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
<NoWarn>$(NoWarn);CS1591</NoWarn>
<GenerateDocumentationFile>true</GenerateDocumentationFile>
<PackageTags>aspnetcore;options</PackageTags>
Expand All @@ -13,7 +13,6 @@
<ItemGroup>
<PackageReference Include="Microsoft.Extensions.DependencyInjection.Abstractions" Version="$(AspNetCoreVersion)" />
<PackageReference Include="Microsoft.Extensions.Primitives" Version="$(AspNetCoreVersion)" />
<PackageReference Include="System.ComponentModel" Version="$(CoreFxVersion)" />
</ItemGroup>
<PackageReference Include="System.ComponentModel" Version="$(CoreFxVersion)" /> </ItemGroup>

</Project>
7 changes: 6 additions & 1 deletion src/Microsoft.Extensions.Options/Options.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
namespace Microsoft.Extensions.Options
// 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>
/// Helper class.
/// </summary>
public static class Options
{
public static readonly string DefaultName = string.Empty;

/// <summary>
/// Creates a wrapper around an instance of TOptions to return itself as an IOptions.
/// </summary>
Expand Down
Loading