From 1d78f9dec5f2f0be2ef4d418949289b3aa7390a9 Mon Sep 17 00:00:00 2001 From: Zhiyuan Liang Date: Thu, 27 Jun 2024 15:05:18 +0800 Subject: [PATCH 01/10] respect all feature management schemas --- .../ConfigurationFeatureDefinitionProvider.cs | 202 ++++++++---------- .../DotnetFeatureManagementSchema.json | 74 ------- .../FeatureManagementTest.cs | 35 +-- 3 files changed, 92 insertions(+), 219 deletions(-) diff --git a/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs b/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs index 19004d6d..608a81bf 100644 --- a/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs +++ b/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs @@ -12,6 +12,8 @@ using System.Threading; using System.Threading.Tasks; +using static System.Collections.Specialized.BitVector32; + namespace Microsoft.FeatureManagement { /// @@ -27,9 +29,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,16 +83,49 @@ 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)); + if (_definitions.TryGetValue(featureName, out FeatureDefinition definition)) + { + return Task.FromResult(definition); + } + + IEnumerable microsoftFeatureManagementSection = GetFeatureDefinitionSections(MicrosoftFeatureManagementFields.FeatureManagementSectionName); + + if (microsoftFeatureManagementSection != null) + { + IConfigurationSection configuration = microsoftFeatureManagementSection + .FirstOrDefault(section => + string.Equals(section[MicrosoftFeatureManagementFields.Id], featureName, StringComparison.OrdinalIgnoreCase)); + + if (configuration != null) + { + definition = _definitions.GetOrAdd(featureName, (_) => ParseMicrosoftFeatureDefinition(configuration)); + + return Task.FromResult(definition); + } + } + + bool rootConfigurationFallback = microsoftFeatureManagementSection == null && RootConfigurationFallbackEnabled; + + IEnumerable dotnetFeatureManagementSection = GetDotnetFeatureManagementSection(rootConfigurationFallback); + + if (dotnetFeatureManagementSection != null) + { + IConfigurationSection configuration = dotnetFeatureManagementSection + .FirstOrDefault(section => + string.Equals(section.Key, featureName, StringComparison.OrdinalIgnoreCase)); + + if (configuration != null) + { + definition = _definitions.GetOrAdd(featureName, (_) => ParseDotnetFeatureDefinition(configuration)); + + return Task.FromResult(definition); + } + } return Task.FromResult(definition); } @@ -109,97 +141,77 @@ 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()) - { - string featureName = GetFeatureName(featureSection); + IEnumerable microsoftFeatureManagementSection = GetFeatureDefinitionSections(MicrosoftFeatureManagementFields.FeatureManagementSectionName); - if (string.IsNullOrEmpty(featureName)) + if (microsoftFeatureManagementSection != null) + { + foreach (IConfigurationSection featureSection in microsoftFeatureManagementSection) { - continue; - } + string featureName = featureSection[MicrosoftFeatureManagementFields.Id]; - // - // Underlying IConfigurationSection data is dynamic so latest feature definitions are returned - FeatureDefinition definition = _definitions.GetOrAdd(featureName, (_) => ReadFeatureDefinition(featureSection)); + if (string.IsNullOrEmpty(featureName)) + { + continue; + } - // - // Null cache entry possible if someone accesses non-existent flag directly (IsEnabled) - if (definition != null) - { - yield return definition; + // + // Underlying IConfigurationSection data is dynamic so latest feature definitions are returned + FeatureDefinition definition = _definitions.GetOrAdd(featureName, (_) => ParseMicrosoftFeatureDefinition(featureSection)); + + 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)); + bool rootConfigurationFallback = microsoftFeatureManagementSection == null && RootConfigurationFallbackEnabled; - bool hasMicrosoftFeatureManagementSchema = MicrosoftFeatureManagementConfigurationSection != null; + IEnumerable dotnetFeatureManagementSection = GetDotnetFeatureManagementSection(rootConfigurationFallback); - if (MicrosoftFeatureManagementConfigurationSection == null & RootConfigurationFallbackEnabled) + if (dotnetFeatureManagementSection != null) + { + foreach (IConfigurationSection featureSection in dotnetFeatureManagementSection) { - IConfiguration featureFlagsSection = _configuration - .GetChildren() - .FirstOrDefault(section => - string.Equals( - section.Key, - MicrosoftFeatureManagementFields.FeatureFlagsSectionName, - StringComparison.OrdinalIgnoreCase)); - - hasMicrosoftFeatureManagementSchema = featureFlagsSection != null; - } + string featureName = featureSection.Key; - lock (_lock) - { - if (Interlocked.Read(ref _initialized) == 0) + if (string.IsNullOrEmpty(featureName)) { - _microsoftFeatureManagementSchemaEnabled = hasMicrosoftFeatureManagementSchema; + continue; + } + + // + // Underlying IConfigurationSection data is dynamic so latest feature definitions are returned + FeatureDefinition definition = _definitions.GetOrAdd(featureName, (_) => ParseDotnetFeatureDefinition(featureSection)); - Interlocked.Exchange(ref _initialized, 1); + if (definition != null) + { + yield return definition; } } } } - private FeatureDefinition ReadFeatureDefinition(string featureName) + private IEnumerable GetDotnetFeatureManagementSection(bool rootConfigurationFallback) { - IConfigurationSection configuration = GetFeatureDefinitionSections() - .FirstOrDefault(section => string.Equals(GetFeatureName(section), featureName, StringComparison.OrdinalIgnoreCase)); + IEnumerable dotnetFeatureManagementSection = GetFeatureDefinitionSections(DotnetFeatureManagementFields.FeatureManagementSectionName); - if (configuration == null) + if (dotnetFeatureManagementSection == null && rootConfigurationFallback) { - return null; - } - - return ReadFeatureDefinition(configuration); - } + if (!_configuration.GetChildren().Any()) + { + Logger?.LogDebug($"Configuration is empty."); + } - private FeatureDefinition ReadFeatureDefinition(IConfigurationSection configurationSection) - { - if (_microsoftFeatureManagementSchemaEnabled) - { - return ParseMicrosoftFeatureDefinition(configurationSection); + dotnetFeatureManagementSection = _configuration.GetChildren(); } - return ParseDotnetFeatureDefinition(configurationSection); + return dotnetFeatureManagementSection; } private FeatureDefinition ParseDotnetFeatureDefinition(IConfigurationSection configurationSection) @@ -229,7 +241,7 @@ We support */ - string featureName = GetFeatureName(configurationSection); + string featureName = configurationSection.Key; var enabledFor = new List(); @@ -323,7 +335,7 @@ private FeatureDefinition ParseMicrosoftFeatureDefinition(IConfigurationSection */ - string featureName = GetFeatureName(configurationSection); + string featureName = configurationSection[MicrosoftFeatureManagementFields.Id]; var enabledFor = new List(); @@ -512,57 +524,23 @@ private FeatureDefinition ParseMicrosoftFeatureDefinition(IConfigurationSection }; } - private string GetFeatureName(IConfigurationSection section) + private IEnumerable GetFeatureDefinitionSections(string featureManagementSectionName) { - 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, + featureManagementSectionName, StringComparison.OrdinalIgnoreCase)); - if (featureManagementConfigurationSection == null) + if (featureManagementConfigurationSection != null && + featureManagementSectionName.Equals(MicrosoftFeatureManagementFields.FeatureManagementSectionName)) { - 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.GetSection(MicrosoftFeatureManagementFields.FeatureFlagsSectionName).GetChildren(); } - return featureManagementConfigurationSection.GetChildren(); + return featureManagementConfigurationSection?.GetChildren(); } private T ParseEnum(string feature, string rawValue, string fieldKeyword) 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..d2ca3d28 100644 --- a/tests/Tests.FeatureManagement/FeatureManagementTest.cs +++ b/tests/Tests.FeatureManagement/FeatureManagementTest.cs @@ -179,38 +179,7 @@ public async Task ReadsOnlyFeatureManagementSection() } [Fact] - public async Task ReadsTopLevelConfiguration() - { - 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() + public async Task RespectsAllFeatureManagementSchemas() { string json = @" { @@ -243,7 +212,7 @@ public async Task RespectsMicrosoftFeatureManagementSchemaIfAny() Assert.True(await featureManager.IsEnabledAsync("FeatureX")); - Assert.False(await featureManager.IsEnabledAsync("FeatureY")); + Assert.True(await featureManager.IsEnabledAsync("FeatureY")); } [Fact] From eac71a9f2bc99ef919c139462ac38ef85d0f95f2 Mon Sep 17 00:00:00 2001 From: Zhiyuan Liang Date: Thu, 27 Jun 2024 15:47:26 +0800 Subject: [PATCH 02/10] add testcase --- .../ConfigurationFeatureDefinitionProvider.cs | 4 ---- .../FeatureManagementTest.cs | 20 ++++++++++++++----- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs b/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs index 608a81bf..c447ef1a 100644 --- a/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs +++ b/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs @@ -12,8 +12,6 @@ using System.Threading; using System.Threading.Tasks; -using static System.Collections.Specialized.BitVector32; - namespace Microsoft.FeatureManagement { /// @@ -122,8 +120,6 @@ public Task GetFeatureDefinitionAsync(string featureName) if (configuration != null) { definition = _definitions.GetOrAdd(featureName, (_) => ParseDotnetFeatureDefinition(configuration)); - - return Task.FromResult(definition); } } diff --git a/tests/Tests.FeatureManagement/FeatureManagementTest.cs b/tests/Tests.FeatureManagement/FeatureManagementTest.cs index d2ca3d28..480b9f88 100644 --- a/tests/Tests.FeatureManagement/FeatureManagementTest.cs +++ b/tests/Tests.FeatureManagement/FeatureManagementTest.cs @@ -151,6 +151,8 @@ public async Task ReadsConfiguration() } Assert.True(hasItems); + + Assert.False(await featureManager.IsEnabledAsync("NonExistentFeature")); } [Fact] @@ -184,16 +186,21 @@ public async Task RespectsAllFeatureManagementSchemas() string json = @" { ""AllowedHosts"": ""*"", + ""FeatureManagement"": { + ""FeatureX"": true, + ""FeatureY"": true + }, ""feature_management"": { ""feature_flags"": [ { - ""id"": ""FeatureX"", + ""id"": ""FeatureZ"", ""enabled"": true + }, + { + ""id"": ""FeatureY"", + ""enabled"": false } ] - }, - ""FeatureManagement"": { - ""FeatureY"": true } }"; @@ -212,7 +219,10 @@ public async Task RespectsAllFeatureManagementSchemas() Assert.True(await featureManager.IsEnabledAsync("FeatureX")); - Assert.True(await featureManager.IsEnabledAsync("FeatureY")); + // feature flag written in Microsoft schema has higher priority + Assert.False(await featureManager.IsEnabledAsync("FeatureY")); + + Assert.True(await featureManager.IsEnabledAsync("FeatureZ")); } [Fact] From 051711a1193b98fae04779ea6f0c5926fb9a7455 Mon Sep 17 00:00:00 2001 From: Zhiyuan Liang Date: Thu, 27 Jun 2024 17:29:09 +0800 Subject: [PATCH 03/10] add testcase --- .../FeatureManagementTest.cs | 38 +++++++++++++++++-- 1 file changed, 34 insertions(+), 4 deletions(-) diff --git a/tests/Tests.FeatureManagement/FeatureManagementTest.cs b/tests/Tests.FeatureManagement/FeatureManagementTest.cs index 480b9f88..eb70fcbd 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"": false + } + ] + } + } + }"; + + 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")); } } From 817d99219916834e29de00b3aeadc96e95d5415b Mon Sep 17 00:00:00 2001 From: zhiyuanliang Date: Fri, 28 Jun 2024 00:33:30 +0800 Subject: [PATCH 04/10] correct test case --- tests/Tests.FeatureManagement/FeatureManagementTest.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Tests.FeatureManagement/FeatureManagementTest.cs b/tests/Tests.FeatureManagement/FeatureManagementTest.cs index eb70fcbd..2b6572b3 100644 --- a/tests/Tests.FeatureManagement/FeatureManagementTest.cs +++ b/tests/Tests.FeatureManagement/FeatureManagementTest.cs @@ -100,7 +100,7 @@ public async Task ReadsTopLevelConfiguration() ""feature_flags"": [ { ""id"": ""FeatureY"", - ""enabled"": false + ""enabled"": true } ] } From fbffdaee219f9e9f0aa7fd33b29df8b42a46ab2f Mon Sep 17 00:00:00 2001 From: Zhiyuan Liang Date: Wed, 3 Jul 2024 16:12:36 +0800 Subject: [PATCH 05/10] simplify methods --- .../ConfigurationFeatureDefinitionProvider.cs | 185 +++++++++--------- .../FeatureManagementTest.cs | 2 +- 2 files changed, 90 insertions(+), 97 deletions(-) diff --git a/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs b/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs index c447ef1a..7488e797 100644 --- a/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs +++ b/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs @@ -11,6 +11,7 @@ using System.Linq; using System.Threading; using System.Threading.Tasks; +using System.Xml.Linq; namespace Microsoft.FeatureManagement { @@ -91,39 +92,10 @@ public Task GetFeatureDefinitionAsync(string featureName) return Task.FromResult(definition); } - IEnumerable microsoftFeatureManagementSection = GetFeatureDefinitionSections(MicrosoftFeatureManagementFields.FeatureManagementSectionName); - - if (microsoftFeatureManagementSection != null) - { - IConfigurationSection configuration = microsoftFeatureManagementSection - .FirstOrDefault(section => - string.Equals(section[MicrosoftFeatureManagementFields.Id], featureName, StringComparison.OrdinalIgnoreCase)); - - if (configuration != null) - { - definition = _definitions.GetOrAdd(featureName, (_) => ParseMicrosoftFeatureDefinition(configuration)); - - return Task.FromResult(definition); - } - } - - bool rootConfigurationFallback = microsoftFeatureManagementSection == null && RootConfigurationFallbackEnabled; - - IEnumerable dotnetFeatureManagementSection = GetDotnetFeatureManagementSection(rootConfigurationFallback); - - if (dotnetFeatureManagementSection != null) - { - IConfigurationSection configuration = dotnetFeatureManagementSection - .FirstOrDefault(section => - string.Equals(section.Key, featureName, StringComparison.OrdinalIgnoreCase)); - - if (configuration != null) - { - definition = _definitions.GetOrAdd(featureName, (_) => ParseDotnetFeatureDefinition(configuration)); - } - } - - return Task.FromResult(definition); + return Task.FromResult( + _definitions.GetOrAdd( + featureName, + (_) => GetMicrosoftSchemaFeatureDefinition(featureName) ?? GetDotnetSchemaFeatureDefinition(featureName))); } /// @@ -142,75 +114,115 @@ public async IAsyncEnumerable GetAllFeatureDefinitionsAsync() _definitions.Clear(); } - IEnumerable microsoftFeatureManagementSection = GetFeatureDefinitionSections(MicrosoftFeatureManagementFields.FeatureManagementSectionName); + IEnumerable microsoftFeatureManagementSection = GetMicrosoftFeatureDefinitionSections(); - if (microsoftFeatureManagementSection != null) + foreach (IConfigurationSection featureSection in microsoftFeatureManagementSection) { - foreach (IConfigurationSection featureSection in microsoftFeatureManagementSection) + string featureName = featureSection[MicrosoftFeatureManagementFields.Id]; + + if (string.IsNullOrEmpty(featureName)) { - string featureName = featureSection[MicrosoftFeatureManagementFields.Id]; + continue; + } - if (string.IsNullOrEmpty(featureName)) - { - continue; - } + // + // Underlying IConfigurationSection data is dynamic so latest feature definitions are returned + FeatureDefinition definition = _definitions.GetOrAdd(featureName, (_) => ParseMicrosoftSchemaFeatureDefinition(featureSection)); - // - // Underlying IConfigurationSection data is dynamic so latest feature definitions are returned - FeatureDefinition definition = _definitions.GetOrAdd(featureName, (_) => ParseMicrosoftFeatureDefinition(featureSection)); + if (definition != null) + { + yield return definition; + } + } - if (definition != null) - { - yield return definition; - } + IEnumerable dotnetFeatureManagementSection = GetDotnetFeatureDefinitionSections(); + + foreach (IConfigurationSection featureSection in dotnetFeatureManagementSection) + { + string featureName = featureSection.Key; + + if (string.IsNullOrEmpty(featureName)) + { + continue; + } + + // + // Underlying IConfigurationSection data is dynamic so latest feature definitions are returned + FeatureDefinition definition = _definitions.GetOrAdd(featureName, (_) => ParseDotnetSchemaFeatureDefinition(featureSection)); + + if (definition != null) + { + yield return definition; } } + } - bool rootConfigurationFallback = microsoftFeatureManagementSection == null && RootConfigurationFallbackEnabled; + private FeatureDefinition GetDotnetSchemaFeatureDefinition(string featureName) + { + IEnumerable dotnetFeatureManagementSection = GetDotnetFeatureDefinitionSections(); - IEnumerable dotnetFeatureManagementSection = GetDotnetFeatureManagementSection(rootConfigurationFallback); + IConfigurationSection configuration = dotnetFeatureManagementSection + .FirstOrDefault(section => + string.Equals(section.Key, featureName, StringComparison.OrdinalIgnoreCase)); - if (dotnetFeatureManagementSection != null) + if (configuration == null) { - foreach (IConfigurationSection featureSection in dotnetFeatureManagementSection) - { - string featureName = featureSection.Key; + return null; + } - if (string.IsNullOrEmpty(featureName)) - { - continue; - } + return ParseDotnetSchemaFeatureDefinition(configuration); + } - // - // Underlying IConfigurationSection data is dynamic so latest feature definitions are returned - FeatureDefinition definition = _definitions.GetOrAdd(featureName, (_) => ParseDotnetFeatureDefinition(featureSection)); + private FeatureDefinition GetMicrosoftSchemaFeatureDefinition(string featureName) + { + IEnumerable microsoftFeatureManagementSection = GetMicrosoftFeatureDefinitionSections(); - if (definition != null) - { - yield return definition; - } - } + IConfigurationSection configuration = microsoftFeatureManagementSection + .FirstOrDefault(section => + string.Equals(section[MicrosoftFeatureManagementFields.Id], featureName, StringComparison.OrdinalIgnoreCase)); + + if (configuration == null) + { + return null; } + + return ParseMicrosoftSchemaFeatureDefinition(configuration); } - private IEnumerable GetDotnetFeatureManagementSection(bool rootConfigurationFallback) + private IEnumerable GetDotnetFeatureDefinitionSections() { - IEnumerable dotnetFeatureManagementSection = GetFeatureDefinitionSections(DotnetFeatureManagementFields.FeatureManagementSectionName); + IConfigurationSection featureManagementConfigurationSection = _configuration.GetSection(DotnetFeatureManagementFields.FeatureManagementSectionName); - if (dotnetFeatureManagementSection == null && rootConfigurationFallback) + if (featureManagementConfigurationSection.Exists()) { - if (!_configuration.GetChildren().Any()) - { - Logger?.LogDebug($"Configuration is empty."); - } + return featureManagementConfigurationSection.GetChildren(); + } - dotnetFeatureManagementSection = _configuration.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 _configuration.GetChildren(); } - return dotnetFeatureManagementSection; + 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) { /* @@ -301,7 +313,7 @@ We support }; } - private FeatureDefinition ParseMicrosoftFeatureDefinition(IConfigurationSection configurationSection) + private FeatureDefinition ParseMicrosoftSchemaFeatureDefinition(IConfigurationSection configurationSection) { /* @@ -520,25 +532,6 @@ private FeatureDefinition ParseMicrosoftFeatureDefinition(IConfigurationSection }; } - private IEnumerable GetFeatureDefinitionSections(string featureManagementSectionName) - { - IConfiguration featureManagementConfigurationSection = _configuration - .GetChildren() - .FirstOrDefault(section => - string.Equals( - section.Key, - featureManagementSectionName, - StringComparison.OrdinalIgnoreCase)); - - if (featureManagementConfigurationSection != null && - featureManagementSectionName.Equals(MicrosoftFeatureManagementFields.FeatureManagementSectionName)) - { - return featureManagementConfigurationSection.GetSection(MicrosoftFeatureManagementFields.FeatureFlagsSectionName).GetChildren(); - } - - return featureManagementConfigurationSection?.GetChildren(); - } - private T ParseEnum(string feature, string rawValue, string fieldKeyword) where T : struct, Enum { diff --git a/tests/Tests.FeatureManagement/FeatureManagementTest.cs b/tests/Tests.FeatureManagement/FeatureManagementTest.cs index 2b6572b3..77a422af 100644 --- a/tests/Tests.FeatureManagement/FeatureManagementTest.cs +++ b/tests/Tests.FeatureManagement/FeatureManagementTest.cs @@ -924,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(); From ba4e68963cbfa6038e4a39658474b74628f11149 Mon Sep 17 00:00:00 2001 From: Zhiyuan Liang Date: Mon, 8 Jul 2024 10:22:16 +0800 Subject: [PATCH 06/10] remove unused package --- .../ConfigurationFeatureDefinitionProvider.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs b/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs index 7488e797..e0518388 100644 --- a/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs +++ b/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs @@ -11,7 +11,6 @@ using System.Linq; using System.Threading; using System.Threading.Tasks; -using System.Xml.Linq; namespace Microsoft.FeatureManagement { From 286d86c65920d81601b91a411e6ff15b3023c63d Mon Sep 17 00:00:00 2001 From: Zhiyuan Liang Date: Mon, 8 Jul 2024 11:00:57 +0800 Subject: [PATCH 07/10] unify newline usage --- .../ConfigurationFeatureDefinitionProvider.cs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs b/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs index e0518388..864c329a 100644 --- a/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs +++ b/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs @@ -203,10 +203,7 @@ private IEnumerable GetDotnetFeatureDefinitionSections() if (RootConfigurationFallbackEnabled && !_configuration.GetChildren() .Any(section => - string.Equals( - section.Key, - MicrosoftFeatureManagementFields.FeatureManagementSectionName, - StringComparison.OrdinalIgnoreCase))) + string.Equals(section.Key, MicrosoftFeatureManagementFields.FeatureManagementSectionName, StringComparison.OrdinalIgnoreCase))) { return _configuration.GetChildren(); } From 66292840ccc9571f820435646f1793c88f9a803e Mon Sep 17 00:00:00 2001 From: Zhiyuan Liang Date: Wed, 10 Jul 2024 11:07:32 +0800 Subject: [PATCH 08/10] remove useless code & rename variables --- .../ConfigurationFeatureDefinitionProvider.cs | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs b/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs index 864c329a..899ab7ac 100644 --- a/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs +++ b/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs @@ -86,11 +86,6 @@ public Task GetFeatureDefinitionAsync(string featureName) _definitions.Clear(); } - if (_definitions.TryGetValue(featureName, out FeatureDefinition definition)) - { - return Task.FromResult(definition); - } - return Task.FromResult( _definitions.GetOrAdd( featureName, @@ -158,9 +153,9 @@ public async IAsyncEnumerable GetAllFeatureDefinitionsAsync() private FeatureDefinition GetDotnetSchemaFeatureDefinition(string featureName) { - IEnumerable dotnetFeatureManagementSection = GetDotnetFeatureDefinitionSections(); + IEnumerable dotnetFeatureDefinitionSections = GetDotnetFeatureDefinitionSections(); - IConfigurationSection configuration = dotnetFeatureManagementSection + IConfigurationSection configuration = dotnetFeatureDefinitionSections .FirstOrDefault(section => string.Equals(section.Key, featureName, StringComparison.OrdinalIgnoreCase)); @@ -174,9 +169,9 @@ private FeatureDefinition GetDotnetSchemaFeatureDefinition(string featureName) private FeatureDefinition GetMicrosoftSchemaFeatureDefinition(string featureName) { - IEnumerable microsoftFeatureManagementSection = GetMicrosoftFeatureDefinitionSections(); + IEnumerable microsoftFeatureDefinitionSections = GetMicrosoftFeatureDefinitionSections(); - IConfigurationSection configuration = microsoftFeatureManagementSection + IConfigurationSection configuration = microsoftFeatureDefinitionSections .FirstOrDefault(section => string.Equals(section[MicrosoftFeatureManagementFields.Id], featureName, StringComparison.OrdinalIgnoreCase)); From aba39106b29599ca8d91e4f26d07d7e033f1c5cd Mon Sep 17 00:00:00 2001 From: Zhiyuan Liang Date: Wed, 10 Jul 2024 11:32:46 +0800 Subject: [PATCH 09/10] reuse ParseEnum --- .../ConfigurationFeatureDefinitionProvider.cs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs b/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs index 899ab7ac..c46a1bd7 100644 --- a/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs +++ b/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs @@ -365,13 +365,9 @@ private FeatureDefinition ParseMicrosoftSchemaFeatureDefinition(IConfigurationSe { 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; From c49e1e4ee0791c424eb8952ca2eb4f2fe7ec0227 Mon Sep 17 00:00:00 2001 From: Zhiyuan Liang Date: Wed, 10 Jul 2024 11:37:43 +0800 Subject: [PATCH 10/10] rename variable --- .../ConfigurationFeatureDefinitionProvider.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs b/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs index c46a1bd7..d487eee5 100644 --- a/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs +++ b/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs @@ -108,9 +108,9 @@ public async IAsyncEnumerable GetAllFeatureDefinitionsAsync() _definitions.Clear(); } - IEnumerable microsoftFeatureManagementSection = GetMicrosoftFeatureDefinitionSections(); + IEnumerable microsoftFeatureDefinitionSections = GetMicrosoftFeatureDefinitionSections(); - foreach (IConfigurationSection featureSection in microsoftFeatureManagementSection) + foreach (IConfigurationSection featureSection in microsoftFeatureDefinitionSections) { string featureName = featureSection[MicrosoftFeatureManagementFields.Id]; @@ -129,9 +129,9 @@ public async IAsyncEnumerable GetAllFeatureDefinitionsAsync() } } - IEnumerable dotnetFeatureManagementSection = GetDotnetFeatureDefinitionSections(); + IEnumerable dotnetFeatureDefinitionSections = GetDotnetFeatureDefinitionSections(); - foreach (IConfigurationSection featureSection in dotnetFeatureManagementSection) + foreach (IConfigurationSection featureSection in dotnetFeatureDefinitionSections) { string featureName = featureSection.Key;