diff --git a/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs b/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs index 19004d6d..d487eee5 100644 --- a/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs +++ b/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs @@ -27,9 +27,6 @@ public sealed class ConfigurationFeatureDefinitionProvider : IFeatureDefinitionP private readonly ConcurrentDictionary _definitions; private IDisposable _changeSubscription; private int _stale = 0; - private long _initialized = 0; - private bool _microsoftFeatureManagementSchemaEnabled; - private readonly object _lock = new object(); const string ParseValueErrorString = "Invalid setting '{0}' with value '{1}' for feature '{2}'."; @@ -84,18 +81,15 @@ public Task GetFeatureDefinitionAsync(string featureName) throw new ArgumentException($"The value '{ConfigurationPath.KeyDelimiter}' is not allowed in the feature name.", nameof(featureName)); } - EnsureInit(); - if (Interlocked.Exchange(ref _stale, 0) != 0) { _definitions.Clear(); } - // - // Query by feature name - FeatureDefinition definition = _definitions.GetOrAdd(featureName, (name) => ReadFeatureDefinition(name)); - - return Task.FromResult(definition); + return Task.FromResult( + _definitions.GetOrAdd( + featureName, + (_) => GetMicrosoftSchemaFeatureDefinition(featureName) ?? GetDotnetSchemaFeatureDefinition(featureName))); } /// @@ -109,18 +103,16 @@ public Task GetFeatureDefinitionAsync(string featureName) public async IAsyncEnumerable GetAllFeatureDefinitionsAsync() #pragma warning restore CS1998 { - EnsureInit(); - if (Interlocked.Exchange(ref _stale, 0) != 0) { _definitions.Clear(); } - // - // Iterate over all features registered in the system at initial invocation time - foreach (IConfigurationSection featureSection in GetFeatureDefinitionSections()) + IEnumerable microsoftFeatureDefinitionSections = GetMicrosoftFeatureDefinitionSections(); + + foreach (IConfigurationSection featureSection in microsoftFeatureDefinitionSections) { - string featureName = GetFeatureName(featureSection); + string featureName = featureSection[MicrosoftFeatureManagementFields.Id]; if (string.IsNullOrEmpty(featureName)) { @@ -129,80 +121,99 @@ public async IAsyncEnumerable GetAllFeatureDefinitionsAsync() // // Underlying IConfigurationSection data is dynamic so latest feature definitions are returned - FeatureDefinition definition = _definitions.GetOrAdd(featureName, (_) => ReadFeatureDefinition(featureSection)); + FeatureDefinition definition = _definitions.GetOrAdd(featureName, (_) => ParseMicrosoftSchemaFeatureDefinition(featureSection)); - // - // Null cache entry possible if someone accesses non-existent flag directly (IsEnabled) if (definition != null) { yield return definition; } } - } - private void EnsureInit() - { - if (_initialized == 0) - { - IConfiguration MicrosoftFeatureManagementConfigurationSection = _configuration - .GetChildren() - .FirstOrDefault(section => - string.Equals( - section.Key, - MicrosoftFeatureManagementFields.FeatureManagementSectionName, - StringComparison.OrdinalIgnoreCase)); + IEnumerable dotnetFeatureDefinitionSections = GetDotnetFeatureDefinitionSections(); - bool hasMicrosoftFeatureManagementSchema = MicrosoftFeatureManagementConfigurationSection != null; + foreach (IConfigurationSection featureSection in dotnetFeatureDefinitionSections) + { + string featureName = featureSection.Key; - if (MicrosoftFeatureManagementConfigurationSection == null & RootConfigurationFallbackEnabled) + if (string.IsNullOrEmpty(featureName)) { - IConfiguration featureFlagsSection = _configuration - .GetChildren() - .FirstOrDefault(section => - string.Equals( - section.Key, - MicrosoftFeatureManagementFields.FeatureFlagsSectionName, - StringComparison.OrdinalIgnoreCase)); - - hasMicrosoftFeatureManagementSchema = featureFlagsSection != null; + continue; } - lock (_lock) - { - if (Interlocked.Read(ref _initialized) == 0) - { - _microsoftFeatureManagementSchemaEnabled = hasMicrosoftFeatureManagementSchema; + // + // Underlying IConfigurationSection data is dynamic so latest feature definitions are returned + FeatureDefinition definition = _definitions.GetOrAdd(featureName, (_) => ParseDotnetSchemaFeatureDefinition(featureSection)); - Interlocked.Exchange(ref _initialized, 1); - } + if (definition != null) + { + yield return definition; } } } - private FeatureDefinition ReadFeatureDefinition(string featureName) + private FeatureDefinition GetDotnetSchemaFeatureDefinition(string featureName) { - IConfigurationSection configuration = GetFeatureDefinitionSections() - .FirstOrDefault(section => string.Equals(GetFeatureName(section), featureName, StringComparison.OrdinalIgnoreCase)); + IEnumerable dotnetFeatureDefinitionSections = GetDotnetFeatureDefinitionSections(); + + IConfigurationSection configuration = dotnetFeatureDefinitionSections + .FirstOrDefault(section => + string.Equals(section.Key, featureName, StringComparison.OrdinalIgnoreCase)); if (configuration == null) { return null; } - return ReadFeatureDefinition(configuration); + return ParseDotnetSchemaFeatureDefinition(configuration); } - private FeatureDefinition ReadFeatureDefinition(IConfigurationSection configurationSection) + private FeatureDefinition GetMicrosoftSchemaFeatureDefinition(string featureName) { - if (_microsoftFeatureManagementSchemaEnabled) + IEnumerable microsoftFeatureDefinitionSections = GetMicrosoftFeatureDefinitionSections(); + + IConfigurationSection configuration = microsoftFeatureDefinitionSections + .FirstOrDefault(section => + string.Equals(section[MicrosoftFeatureManagementFields.Id], featureName, StringComparison.OrdinalIgnoreCase)); + + if (configuration == null) + { + return null; + } + + return ParseMicrosoftSchemaFeatureDefinition(configuration); + } + + private IEnumerable GetDotnetFeatureDefinitionSections() + { + IConfigurationSection featureManagementConfigurationSection = _configuration.GetSection(DotnetFeatureManagementFields.FeatureManagementSectionName); + + if (featureManagementConfigurationSection.Exists()) + { + return featureManagementConfigurationSection.GetChildren(); + } + + // + // Root configuration fallback only applies to .NET schema. + // If Microsoft schema can be found, root configuration fallback will not be effective. + if (RootConfigurationFallbackEnabled && + !_configuration.GetChildren() + .Any(section => + string.Equals(section.Key, MicrosoftFeatureManagementFields.FeatureManagementSectionName, StringComparison.OrdinalIgnoreCase))) { - return ParseMicrosoftFeatureDefinition(configurationSection); + return _configuration.GetChildren(); } - return ParseDotnetFeatureDefinition(configurationSection); + return Enumerable.Empty(); } - private FeatureDefinition ParseDotnetFeatureDefinition(IConfigurationSection configurationSection) + private IEnumerable GetMicrosoftFeatureDefinitionSections() + { + return _configuration.GetSection(MicrosoftFeatureManagementFields.FeatureManagementSectionName) + .GetSection(MicrosoftFeatureManagementFields.FeatureFlagsSectionName) + .GetChildren(); + } + + private FeatureDefinition ParseDotnetSchemaFeatureDefinition(IConfigurationSection configurationSection) { /* @@ -229,7 +240,7 @@ We support */ - string featureName = GetFeatureName(configurationSection); + string featureName = configurationSection.Key; var enabledFor = new List(); @@ -293,7 +304,7 @@ We support }; } - private FeatureDefinition ParseMicrosoftFeatureDefinition(IConfigurationSection configurationSection) + private FeatureDefinition ParseMicrosoftSchemaFeatureDefinition(IConfigurationSection configurationSection) { /* @@ -323,7 +334,7 @@ private FeatureDefinition ParseMicrosoftFeatureDefinition(IConfigurationSection */ - string featureName = GetFeatureName(configurationSection); + string featureName = configurationSection[MicrosoftFeatureManagementFields.Id]; var enabledFor = new List(); @@ -354,13 +365,9 @@ private FeatureDefinition ParseMicrosoftFeatureDefinition(IConfigurationSection { string rawRequirementType = conditionsSection[MicrosoftFeatureManagementFields.RequirementType]; - // - // If requirement type is specified, parse it and set the requirementType variable - if (!string.IsNullOrEmpty(rawRequirementType) && !Enum.TryParse(rawRequirementType, ignoreCase: true, out requirementType)) + if (!string.IsNullOrEmpty(rawRequirementType)) { - throw new FeatureManagementException( - FeatureManagementError.InvalidConfigurationSetting, - $"Invalid value '{rawRequirementType}' for field '{MicrosoftFeatureManagementFields.RequirementType}' of feature '{featureName}'."); + requirementType = ParseEnum(featureName, rawRequirementType, MicrosoftFeatureManagementFields.RequirementType); } featureStatus = FeatureStatus.Conditional; @@ -512,59 +519,6 @@ private FeatureDefinition ParseMicrosoftFeatureDefinition(IConfigurationSection }; } - private string GetFeatureName(IConfigurationSection section) - { - if (_microsoftFeatureManagementSchemaEnabled) - { - return section[MicrosoftFeatureManagementFields.Id]; - } - - return section.Key; - } - - private IEnumerable GetFeatureDefinitionSections() - { - if (!_configuration.GetChildren().Any()) - { - Logger?.LogDebug($"Configuration is empty."); - - return Enumerable.Empty(); - } - - IConfiguration featureManagementConfigurationSection = _configuration - .GetChildren() - .FirstOrDefault(section => - string.Equals( - section.Key, - _microsoftFeatureManagementSchemaEnabled ? - MicrosoftFeatureManagementFields.FeatureManagementSectionName : - DotnetFeatureManagementFields.FeatureManagementSectionName, - StringComparison.OrdinalIgnoreCase)); - - if (featureManagementConfigurationSection == null) - { - if (RootConfigurationFallbackEnabled) - { - featureManagementConfigurationSection = _configuration; - } - else - { - Logger?.LogDebug($"No feature management configuration section was found."); - - return Enumerable.Empty(); - } - } - - if (_microsoftFeatureManagementSchemaEnabled) - { - IConfigurationSection featureFlagsSection = featureManagementConfigurationSection.GetSection(MicrosoftFeatureManagementFields.FeatureFlagsSectionName); - - return featureFlagsSection.GetChildren(); - } - - return featureManagementConfigurationSection.GetChildren(); - } - private T ParseEnum(string feature, string rawValue, string fieldKeyword) where T : struct, Enum { diff --git a/tests/Tests.FeatureManagement/DotnetFeatureManagementSchema.json b/tests/Tests.FeatureManagement/DotnetFeatureManagementSchema.json index 426b0581..57b068b5 100644 --- a/tests/Tests.FeatureManagement/DotnetFeatureManagementSchema.json +++ b/tests/Tests.FeatureManagement/DotnetFeatureManagementSchema.json @@ -11,80 +11,6 @@ } } ] - }, - "ContextualFeature": { - "EnabledFor": [ - { - "Name": "ContextualTest", - "Parameters": { - "AllowedAccounts": [ - "abc" - ] - } - } - ] - }, - "AnyFilterFeature": { - "RequirementType": "Any", - "EnabledFor": [ - { - "Name": "Test", - "Parameters": { - "Id": "1" - } - }, - { - "Name": "Test", - "Parameters": { - "Id": "2" - } - } - ] - }, - "AllFilterFeature": { - "RequirementType": "all", - "EnabledFor": [ - { - "Name": "Test", - "Parameters": { - "Id": "1" - } - }, - { - "Name": "Test", - "Parameters": { - "Id": "2" - } - } - ] - }, - "FeatureUsesFiltersWithDuplicatedAlias": { - "RequirementType": "all", - "EnabledFor": [ - { - "Name": "DuplicatedFilterName" - }, - { - "Name": "Percentage", - "Parameters": { - "Value": 100 - } - } - ] - }, - "CustomFilterFeature": { - "EnabledFor": [ - { - "Name": "CustomTargetingFilter", - "Parameters": { - "Audience": { - "Users": [ - "Jeff" - ] - } - } - } - ] } } } \ No newline at end of file diff --git a/tests/Tests.FeatureManagement/FeatureManagementTest.cs b/tests/Tests.FeatureManagement/FeatureManagementTest.cs index 1a162f58..77a422af 100644 --- a/tests/Tests.FeatureManagement/FeatureManagementTest.cs +++ b/tests/Tests.FeatureManagement/FeatureManagementTest.cs @@ -78,9 +78,7 @@ public async Task ReadsConfiguration() [Fact] public async Task ReadsTopLevelConfiguration() { - const string feature = "FeatureX"; - - var stream = new MemoryStream(Encoding.UTF8.GetBytes($"{{\"AllowedHosts\": \"*\", \"FeatureFlags\": {{\"{feature}\": true}}}}")); + var stream = new MemoryStream(Encoding.UTF8.GetBytes($"{{\"AllowedHosts\": \"*\", \"FeatureFlags\": {{\"FeatureX\": true}}}}")); IConfiguration config = new ConfigurationBuilder().AddJsonStream(stream).Build(); @@ -92,7 +90,39 @@ public async Task ReadsTopLevelConfiguration() IFeatureManager featureManager = serviceProvider.GetRequiredService(); - Assert.True(await featureManager.IsEnabledAsync(feature)); + //Assert.True(await featureManager.IsEnabledAsync("FeatureX")); + + string json = @" + { + ""FeatureFlags"": { + ""FeatureX"": true, + ""feature_management"": { + ""feature_flags"": [ + { + ""id"": ""FeatureY"", + ""enabled"": true + } + ] + } + } + }"; + + stream = new MemoryStream(Encoding.UTF8.GetBytes(json)); + + config = new ConfigurationBuilder().AddJsonStream(stream).Build(); + + services = new ServiceCollection(); + + services.AddFeatureManagement(config.GetSection("FeatureFlags")); + + serviceProvider = services.BuildServiceProvider(); + + featureManager = serviceProvider.GetRequiredService(); + + // If Microsoft schema can be found, it will not fall back to root configuration. + Assert.False(await featureManager.IsEnabledAsync("FeatureX")); + + Assert.True(await featureManager.IsEnabledAsync("FeatureY")); } } @@ -151,6 +181,8 @@ public async Task ReadsConfiguration() } Assert.True(hasItems); + + Assert.False(await featureManager.IsEnabledAsync("NonExistentFeature")); } [Fact] @@ -179,52 +211,26 @@ public async Task ReadsOnlyFeatureManagementSection() } [Fact] - public async Task ReadsTopLevelConfiguration() + public async Task RespectsAllFeatureManagementSchemas() { string json = @" { ""AllowedHosts"": ""*"", ""FeatureManagement"": { - ""feature_flags"": [ - { - ""id"": ""FeatureX"", - ""enabled"": true - } - ] - } - }"; - - var stream = new MemoryStream(Encoding.UTF8.GetBytes(json)); - - IConfiguration config = new ConfigurationBuilder().AddJsonStream(stream).Build(); - - var services = new ServiceCollection(); - - services.AddFeatureManagement(config.GetSection("FeatureManagement")); - - ServiceProvider serviceProvider = services.BuildServiceProvider(); - - IFeatureManager featureManager = serviceProvider.GetRequiredService(); - - Assert.True(await featureManager.IsEnabledAsync("FeatureX")); - } - - [Fact] - public async Task RespectsMicrosoftFeatureManagementSchemaIfAny() - { - string json = @" - { - ""AllowedHosts"": ""*"", + ""FeatureX"": true, + ""FeatureY"": true + }, ""feature_management"": { ""feature_flags"": [ { - ""id"": ""FeatureX"", + ""id"": ""FeatureZ"", ""enabled"": true + }, + { + ""id"": ""FeatureY"", + ""enabled"": false } ] - }, - ""FeatureManagement"": { - ""FeatureY"": true } }"; @@ -243,7 +249,10 @@ public async Task RespectsMicrosoftFeatureManagementSchemaIfAny() Assert.True(await featureManager.IsEnabledAsync("FeatureX")); + // feature flag written in Microsoft schema has higher priority Assert.False(await featureManager.IsEnabledAsync("FeatureY")); + + Assert.True(await featureManager.IsEnabledAsync("FeatureZ")); } [Fact] @@ -915,7 +924,7 @@ public async Task Percentage() Environment.SetEnvironmentVariable($"feature_management:feature_flags:0:conditions:client_filters:0:name", "Percentage"); Environment.SetEnvironmentVariable($"feature_management:feature_flags:0:conditions:client_filters:0:parameters:Value", "50"); - IConfiguration config = new ConfigurationBuilder().AddEnvironmentVariables().AddJsonFile("appsettings.json").Build(); + IConfiguration config = new ConfigurationBuilder().AddEnvironmentVariables().Build(); var serviceCollection = new ServiceCollection();