diff --git a/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs b/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs index c71a7ed4..3558e251 100644 --- a/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs +++ b/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs @@ -131,7 +131,7 @@ public Task GetFeatureDefinitionAsync(string featureName) /// An enumerator which provides asynchronous iteration over feature definitions. // // The async key word is necessary for creating IAsyncEnumerable. - // The need to disable this warning occurs when implementing async stream synchronously. + // The need to disable this warning occurs when implementing async stream synchronously. #pragma warning disable CS1998 // Async method lacks 'await' operators and will run synchronously public async IAsyncEnumerable GetAllFeatureDefinitionsAsync() #pragma warning restore CS1998 @@ -297,9 +297,9 @@ private void FindFeatureDefinitionSources( private FeatureDefinition ParseDotnetSchemaFeatureDefinition(IConfigurationSection configurationSection) { /* - + We support - + myFeature: { enabledFor: [{name: "myFeatureFilter1"}, {name: "myFeatureFilter2"}] }, @@ -388,7 +388,7 @@ We support private FeatureDefinition ParseMicrosoftSchemaFeatureDefinition(IConfigurationSection configurationSection) { /* - + If Microsoft feature flag schema is enabled, we support FeatureFlags: [ @@ -544,6 +544,8 @@ private FeatureDefinition ParseMicrosoftSchemaFeatureDefinition(IConfigurationSe { StatusOverride statusOverride = StatusOverride.None; + IConfigurationSection variantConfiguration = section.GetSection(MicrosoftFeatureManagementFields.VariantDefinitionConfigurationValue); + string rawStatusOverride = section[MicrosoftFeatureManagementFields.VariantDefinitionStatusOverride]; if (!string.IsNullOrEmpty(rawStatusOverride)) @@ -554,7 +556,10 @@ private FeatureDefinition ParseMicrosoftSchemaFeatureDefinition(IConfigurationSe var variant = new VariantDefinition() { Name = section[MicrosoftFeatureManagementFields.Name], - ConfigurationValue = section.GetSection(MicrosoftFeatureManagementFields.VariantDefinitionConfigurationValue), + ConfigurationValue = variantConfiguration, + ConfigurationCache = variantConfiguration.Exists() + ? new VariantConfigurationCache(variantConfiguration) + : null, StatusOverride = statusOverride }; diff --git a/src/Microsoft.FeatureManagement/FeatureManager.cs b/src/Microsoft.FeatureManagement/FeatureManager.cs index f7bca84a..0ebbbdd7 100644 --- a/src/Microsoft.FeatureManagement/FeatureManager.cs +++ b/src/Microsoft.FeatureManagement/FeatureManager.cs @@ -842,15 +842,26 @@ private Variant GetVariantFromVariantDefinition(VariantDefinition variantDefinit { IConfigurationSection variantConfiguration = null; - if (variantDefinition.ConfigurationValue.Exists()) + IConfigurationSection definitionConfiguration = variantDefinition.ConfigurationValue; + + if (definitionConfiguration?.Exists() == true) + { + variantConfiguration = definitionConfiguration; + } + + VariantConfigurationCache configurationCache = variantDefinition.ConfigurationCache; + + if (!ReferenceEquals(configurationCache?.Configuration, variantConfiguration)) { - variantConfiguration = variantDefinition.ConfigurationValue; + configurationCache = null; } - return new Variant() + return new Variant { Name = variantDefinition.Name, - Configuration = variantConfiguration + Configuration = variantConfiguration, + ConfigurationObject = variantDefinition.ConfigurationObject, + ConfigurationCache = configurationCache, }; } } diff --git a/src/Microsoft.FeatureManagement/Variant.cs b/src/Microsoft.FeatureManagement/Variant.cs index f69a47ce..024cb233 100644 --- a/src/Microsoft.FeatureManagement/Variant.cs +++ b/src/Microsoft.FeatureManagement/Variant.cs @@ -19,5 +19,13 @@ public class Variant /// The configuration of the variant. /// public IConfigurationSection Configuration { get; set; } + + /// + /// The configuration of the variant. + /// When set, variants should prefer this over . + /// + public object ConfigurationObject { get; set; } + + internal VariantConfigurationCache ConfigurationCache { get; set; } } } diff --git a/src/Microsoft.FeatureManagement/VariantConfigurationCache.cs b/src/Microsoft.FeatureManagement/VariantConfigurationCache.cs new file mode 100644 index 00000000..cf30c654 --- /dev/null +++ b/src/Microsoft.FeatureManagement/VariantConfigurationCache.cs @@ -0,0 +1,74 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +// + +using Microsoft.Extensions.Configuration; +using System; +using System.Collections.Concurrent; +using System.Collections.Generic; +using System.Threading; + +namespace Microsoft.FeatureManagement +{ + internal sealed class VariantConfigurationCache + { + private readonly ConcurrentDictionary> _configurations = + new ConcurrentDictionary>(); + + public VariantConfigurationCache(IConfigurationSection configuration) + { + Configuration = configuration ?? throw new ArgumentNullException(nameof(configuration)); + } + + public IConfigurationSection Configuration { get; } + + public T GetConfiguration() + { + Type configurationType = typeof(T); + + if (_configurations.TryGetValue(configurationType, out Lazy configuration)) + { + return GetValue(configurationType, configuration); + } + + return GetOrAdd(() => Configuration.Get()); + } + + internal T GetOrAdd(Func valueFactory) + { + if (valueFactory == null) + { + throw new ArgumentNullException(nameof(valueFactory)); + } + + Type configurationType = typeof(T); + + if (!_configurations.TryGetValue(configurationType, out Lazy configuration)) + { + var newConfiguration = new Lazy( + () => valueFactory(), + LazyThreadSafetyMode.ExecutionAndPublication); + + configuration = _configurations.GetOrAdd(configurationType, newConfiguration); + } + + return GetValue(configurationType, configuration); + } + + private T GetValue(Type configurationType, Lazy configuration) + { + try + { + return (T)configuration.Value; + } + catch + { + // A failed binding should not permanently poison this configuration type. + ((ICollection>>)_configurations).Remove( + new KeyValuePair>(configurationType, configuration)); + + throw; + } + } + } +} diff --git a/src/Microsoft.FeatureManagement/VariantDefinition.cs b/src/Microsoft.FeatureManagement/VariantDefinition.cs index 138d2fc5..63b5d18b 100644 --- a/src/Microsoft.FeatureManagement/VariantDefinition.cs +++ b/src/Microsoft.FeatureManagement/VariantDefinition.cs @@ -21,6 +21,16 @@ public class VariantDefinition /// public IConfigurationSection ConfigurationValue { get; set; } + /// + /// A configuration object that can be used as an alternative to . + /// Custom implementations can populate this property directly + /// instead of constructing an instance. + /// When set, variants should prefer this over . + /// + public object ConfigurationObject { get; set; } + + internal VariantConfigurationCache ConfigurationCache { get; set; } + /// /// Overrides the state of the feature if this variant has been assigned. /// diff --git a/src/Microsoft.FeatureManagement/VariantExtensions.cs b/src/Microsoft.FeatureManagement/VariantExtensions.cs new file mode 100644 index 00000000..1cb76afb --- /dev/null +++ b/src/Microsoft.FeatureManagement/VariantExtensions.cs @@ -0,0 +1,52 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +// + +using Microsoft.Extensions.Configuration; + +namespace Microsoft.FeatureManagement +{ + /// + /// Extensions for . + /// + public static class VariantExtensions + { + /// + /// Gets the variant configuration as the requested type. + /// + /// The type of the configuration. + /// The variant to read. + /// + /// The supplied configuration object when assignable to ; + /// otherwise, the configuration bound to from . + /// Returns default when the variant or its configuration is absent. + /// + /// + /// Provider-backed variants created from the same feature definition may share cached configuration instances. + /// Reloaded feature definitions use a new cache. + /// Callers should treat the returned configuration as read-only. + /// + public static T GetConfiguration(this Variant variant) + { + if (variant == null) + { + return default; + } + + if (variant.ConfigurationObject is T typedConfigurationObject) + { + return typedConfigurationObject; + } + + if (variant.ConfigurationCache != null && + ReferenceEquals(variant.ConfigurationCache.Configuration, variant.Configuration)) + { + return variant.ConfigurationCache.GetConfiguration(); + } + + return variant.Configuration != null + ? variant.Configuration.Get() + : default; + } + } +} diff --git a/tests/Tests.FeatureManagement/FeatureManagementTest.cs b/tests/Tests.FeatureManagement/FeatureManagementTest.cs index 701e5c8c..8a2f3f38 100644 --- a/tests/Tests.FeatureManagement/FeatureManagementTest.cs +++ b/tests/Tests.FeatureManagement/FeatureManagementTest.cs @@ -2526,6 +2526,7 @@ public async Task UsesVariants() variant = await featureManager.GetVariantAsync(Features.VariantFeatureDefaultEnabled, cancellationToken); Assert.Equal("Medium", variant.Name); + Assert.Null(variant.ConfigurationObject); Assert.Equal("450px", variant.Configuration["Size"]); Assert.True(await featureManager.IsEnabledAsync(Features.VariantFeatureDefaultEnabled, cancellationToken)); diff --git a/tests/Tests.FeatureManagement/VariantConfigurationCacheTest.cs b/tests/Tests.FeatureManagement/VariantConfigurationCacheTest.cs new file mode 100644 index 00000000..763a4680 --- /dev/null +++ b/tests/Tests.FeatureManagement/VariantConfigurationCacheTest.cs @@ -0,0 +1,156 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +// + +using Microsoft.Extensions.Configuration; +using Microsoft.FeatureManagement; +using System; +using System.Collections.Generic; +using System.Threading; +using System.Threading.Tasks; +using Xunit; + +namespace Tests.FeatureManagement +{ + public class VariantConfigurationCacheTest + { + [Fact] + public void GetOrAddCachesNull() + { + VariantConfigurationCache cache = CreateCache(); + int factoryCalls = 0; + + Assert.Null(cache.GetOrAdd(() => + { + factoryCalls++; + return null; + })); + Assert.Null(cache.GetOrAdd(() => + { + factoryCalls++; + return "unexpected"; + })); + Assert.Equal(1, factoryCalls); + } + + [Fact] + public void GetOrAddRetriesAfterFailure() + { + VariantConfigurationCache cache = CreateCache(); + var expected = new object(); + int factoryCalls = 0; + + Assert.Throws(() => cache.GetOrAdd(() => + { + factoryCalls++; + throw new InvalidOperationException(); + })); + + Assert.Same(expected, cache.GetOrAdd(() => + { + factoryCalls++; + return expected; + })); + Assert.Equal(2, factoryCalls); + } + + [Fact] + public async Task GetOrAddInvokesFactoryOnceForConcurrentCalls() + { + const int CallerCount = 8; + VariantConfigurationCache cache = CreateCache(); + var expected = new object(); + using var callState = new ConcurrentCacheCallState(cache, expected, CallerCount); + var calls = new Task[CallerCount]; + + for (int i = 0; i < calls.Length; i++) + { + calls[i] = Task.Factory.StartNew( + state => ((ConcurrentCacheCallState)state).GetConfiguration(), + callState, + CancellationToken.None, + TaskCreationOptions.LongRunning, + TaskScheduler.Default); + } + + bool callersReady = callState.CallersReady.Wait(TimeSpan.FromSeconds(10)); + callState.StartCallers.Set(); + bool callersAtCache = callState.CallersAtCache.Wait(TimeSpan.FromSeconds(10)); + + object[] results = await Task.WhenAll(calls); + + Assert.True(callersReady); + Assert.True(callersAtCache); + Assert.Equal(1, callState.FactoryCalls); + Assert.False(callState.AdditionalFactoryStarted); + Assert.All(results, result => Assert.Same(expected, result)); + } + + private static VariantConfigurationCache CreateCache() + { + IConfigurationRoot configuration = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary()) + .Build(); + + return new VariantConfigurationCache(configuration.GetSection("unused")); + } + + private sealed class ConcurrentCacheCallState : IDisposable + { + private readonly VariantConfigurationCache _cache; + private readonly object _expected; + private readonly ManualResetEventSlim _additionalFactoryStarted = new ManualResetEventSlim(); + private int _factoryCalls; + + public ConcurrentCacheCallState(VariantConfigurationCache cache, object expected, int callerCount) + { + _cache = cache; + _expected = expected; + CallersReady = new CountdownEvent(callerCount); + CallersAtCache = new CountdownEvent(callerCount); + StartCallers = new ManualResetEventSlim(); + } + + public CountdownEvent CallersReady { get; } + + public CountdownEvent CallersAtCache { get; } + + public ManualResetEventSlim StartCallers { get; } + + public int FactoryCalls => Volatile.Read(ref _factoryCalls); + + public bool AdditionalFactoryStarted => _additionalFactoryStarted.IsSet; + + public object GetConfiguration() + { + CallersReady.Signal(); + StartCallers.Wait(); + CallersAtCache.Signal(); + + return _cache.GetOrAdd(() => + { + int invocation = Interlocked.Increment(ref _factoryCalls); + + if (invocation == 1) + { + _additionalFactoryStarted.Wait(TimeSpan.FromSeconds(1)); + } + else + { + _additionalFactoryStarted.Set(); + } + + return _expected; + }); + } + + public void Dispose() + { + CallersReady.Dispose(); + CallersAtCache.Dispose(); + StartCallers.Dispose(); + _additionalFactoryStarted.Dispose(); + } + } + } +} diff --git a/tests/Tests.FeatureManagement/VariantExtensionsTest.cs b/tests/Tests.FeatureManagement/VariantExtensionsTest.cs new file mode 100644 index 00000000..022f088a --- /dev/null +++ b/tests/Tests.FeatureManagement/VariantExtensionsTest.cs @@ -0,0 +1,234 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +// + +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.Configuration.Memory; +using Microsoft.FeatureManagement; +using System.Collections.Generic; +using System.Linq; +using System.Threading.Tasks; +using Xunit; + +namespace Tests.FeatureManagement +{ + public class VariantExtensionsTest + { + private const string CachedFeatureName = "CachedVariantFeature"; + private const string CachedVariantName = "CachedVariant"; + + private const string ConfigurationValuePath = + "feature_management:feature_flags:0:variants:0:configuration_value:Value"; + + [Fact] + public void GetConfigurationReturnsNullForNullVariant() + { + Variant variant = null; + + Assert.Null(variant.GetConfiguration()); + } + + [Fact] + public void GetConfigurationPrefersAssignableObject() + { + var supplied = new List { "supplied" }; + + var provider = new MemoryConfigurationProvider(new MemoryConfigurationSource()); + var configurationSection = new ConfigurationSection( + new ConfigurationRoot(new List { provider }), + "Param"); + provider.Set(configurationSection.Key, "42"); + var variant = new Variant + { + ConfigurationObject = supplied, + Configuration = configurationSection + }; + + Assert.Same(supplied, variant.GetConfiguration>()); + } + + [Theory] + [InlineData(false)] + [InlineData(true)] + public void GetConfigurationFallsBackToSection(bool hasIncompatibleObject) + { + var provider = new MemoryConfigurationProvider(new MemoryConfigurationSource + { + InitialData = new Dictionary + { + ["Value:AccountId"] = "1", + ["Value:UserId"] = "2", + ["Value:Groups:0"] = "Chrome", + ["Value:Groups:1"] = "Edge", + } + }); + var configurationSection = new ConfigurationSection( + new ConfigurationRoot(new List { provider }), + "Value"); + var variant = new Variant + { + ConfigurationObject = hasIncompatibleObject + ? "some value" + : null, + Configuration = configurationSection + }; + + AppContext firstConfiguration = variant.GetConfiguration(); + AppContext secondConfiguration = variant.GetConfiguration(); + + Assert.Equivalent(new AppContext + { + AccountId = "1", + UserId = "2", + Groups = new List { "Chrome", "Edge" } + }, firstConfiguration); + Assert.NotSame(firstConfiguration, secondConfiguration); + } + + [Fact] + public async Task GetConfigurationCachesProviderConfigurationByTypeAcrossVariants() + { + var configuration = CreateVariantConfiguration("initial"); + using var provider = new ConfigurationFeatureDefinitionProvider(configuration); + var featureManager = new FeatureManager(provider); + + Variant firstVariant = await featureManager.GetVariantAsync(CachedFeatureName); + Variant secondVariant = await featureManager.GetVariantAsync(CachedFeatureName); + + CachedVariantConfiguration firstConfiguration = firstVariant.GetConfiguration(); + AlternateCachedVariantConfiguration alternateConfiguration = + firstVariant.GetConfiguration(); + + Assert.NotSame(firstVariant, secondVariant); + Assert.Equal("initial", firstConfiguration.Value); + Assert.Equal("initial", alternateConfiguration.Value); + Assert.Same(firstConfiguration, firstVariant.GetConfiguration()); + Assert.Same(firstConfiguration, secondVariant.GetConfiguration()); + Assert.Same(alternateConfiguration, secondVariant.GetConfiguration()); + Assert.NotSame(firstConfiguration, alternateConfiguration); + } + + [Fact] + public async Task GetConfigurationUsesNewCacheAfterConfigurationReload() + { + var configuration = CreateVariantConfiguration("before"); + using var provider = new ConfigurationFeatureDefinitionProvider(configuration); + var featureManager = new FeatureManager(provider); + + Variant beforeVariant = await featureManager.GetVariantAsync(CachedFeatureName); + CachedVariantConfiguration beforeConfiguration = + beforeVariant.GetConfiguration(); + + configuration.Providers.Last().Set(ConfigurationValuePath, "after"); + configuration.Reload(); + + Variant afterVariant = await featureManager.GetVariantAsync(CachedFeatureName); + CachedVariantConfiguration afterConfiguration = afterVariant.GetConfiguration(); + + Assert.Equal("before", beforeConfiguration.Value); + Assert.Equal("after", afterConfiguration.Value); + Assert.NotSame(beforeConfiguration, afterConfiguration); + Assert.Same(beforeConfiguration, beforeVariant.GetConfiguration()); + Assert.Same(afterConfiguration, afterVariant.GetConfiguration()); + } + + [Fact] + public async Task GetConfigurationBypassesProviderCacheAfterConfigurationIsReplaced() + { + var configuration = CreateVariantConfiguration("initial"); + using var provider = new ConfigurationFeatureDefinitionProvider(configuration); + var featureManager = new FeatureManager(provider); + Variant variant = await featureManager.GetVariantAsync(CachedFeatureName); + CachedVariantConfiguration initialConfiguration = variant.GetConfiguration(); + var replacementConfiguration = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary + { + ["replacement:Value"] = "replacement" + }) + .Build(); + + variant.Configuration = replacementConfiguration.GetSection("replacement"); + + CachedVariantConfiguration firstReplacement = variant.GetConfiguration(); + CachedVariantConfiguration secondReplacement = variant.GetConfiguration(); + + Assert.Equal("initial", initialConfiguration.Value); + Assert.Equal("replacement", firstReplacement.Value); + Assert.Equal("replacement", secondReplacement.Value); + Assert.NotSame(initialConfiguration, firstReplacement); + Assert.NotSame(firstReplacement, secondReplacement); + } + + [Fact] + public async Task GetConfigurationBypassesProviderCacheAfterDefinitionConfigurationIsReplaced() + { + var configuration = CreateVariantConfiguration("initial"); + using var provider = new ConfigurationFeatureDefinitionProvider(configuration); + FeatureDefinition definition = await provider.GetFeatureDefinitionAsync(CachedFeatureName); + VariantDefinition variantDefinition = Assert.Single(definition.Variants); + var replacementConfiguration = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary + { + ["replacement:Value"] = "replacement" + }) + .Build(); + IConfigurationSection replacementSection = replacementConfiguration.GetSection("replacement"); + + variantDefinition.ConfigurationValue = replacementSection; + + var featureManager = new FeatureManager(provider); + Variant variant = await featureManager.GetVariantAsync(CachedFeatureName); + + Assert.Same(replacementSection, variant.Configuration); + Assert.Null(variant.ConfigurationCache); + Assert.Equal("replacement", variant.GetConfiguration().Value); + } + + [Fact] + public void GetConfigurationBindsScalar() + { + var provider = new MemoryConfigurationProvider(new MemoryConfigurationSource()); + var configurationSection = new ConfigurationSection( + new ConfigurationRoot(new List { provider }), + "Param"); + provider.Set(configurationSection.Key, "42"); + var variant = new Variant + { + Configuration = configurationSection + }; + + Assert.Equal(42, variant.GetConfiguration()); + } + + [Fact] + public void GetConfigurationReturnsNullWithoutConfiguration() + { + Assert.Null(new Variant().GetConfiguration()); + Assert.Null(new Variant().GetConfiguration()); + } + + private static IConfigurationRoot CreateVariantConfiguration(string value) + { + return new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary + { + ["feature_management:feature_flags:0:id"] = CachedFeatureName, + ["feature_management:feature_flags:0:enabled"] = bool.TrueString, + ["feature_management:feature_flags:0:variants:0:name"] = CachedVariantName, + [ConfigurationValuePath] = value, + ["feature_management:feature_flags:0:allocation:default_when_enabled"] = CachedVariantName + }) + .Build(); + } + + private sealed class CachedVariantConfiguration + { + public string Value { get; set; } + } + + private sealed class AlternateCachedVariantConfiguration + { + public string Value { get; set; } + } + } +} diff --git a/tests/Tests.FeatureManagement/appsettings.json b/tests/Tests.FeatureManagement/appsettings.json index 018ef5c4..babce2a3 100644 --- a/tests/Tests.FeatureManagement/appsettings.json +++ b/tests/Tests.FeatureManagement/appsettings.json @@ -569,4 +569,4 @@ } ] } -} +}