Respect .NET and Microsoft feature management schemas in configuration (#470)

* respect all feature management schemas

* add testcase

* add testcase

* correct test case

* simplify methods

* remove unused package

* unify newline usage

* remove useless code & rename variables

* reuse ParseEnum

* rename variable

Author: zhiyuanliang-ms

src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs

@@ -27,9 +27,6 @@ public sealed class ConfigurationFeatureDefinitionProvider : IFeatureDefinitionP
         private readonly ConcurrentDictionary<string, FeatureDefinition> _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<FeatureDefinition> 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)));
         }
 
         /// <summary>
@@ -109,18 +103,16 @@ public Task<FeatureDefinition> GetFeatureDefinitionAsync(string featureName)
         public async IAsyncEnumerable<FeatureDefinition> 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<IConfigurationSection> 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<FeatureDefinition> 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<IConfigurationSection> 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<IConfigurationSection> 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<IConfigurationSection> 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<IConfigurationSection> 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<IConfigurationSection>();
         }
 
-        private FeatureDefinition ParseDotnetFeatureDefinition(IConfigurationSection configurationSection)
+        private IEnumerable<IConfigurationSection> 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<FeatureFilterConfiguration>();
 
@@ -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<FeatureFilterConfiguration>();
 
@@ -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<RequirementType>(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<IConfigurationSection> GetFeatureDefinitionSections()
-        {
-            if (!_configuration.GetChildren().Any())
-            {
-                Logger?.LogDebug($"Configuration is empty.");
-
-                return Enumerable.Empty<IConfigurationSection>();
-            }
-
-            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<IConfigurationSection>();
-                }
-            }
-
-            if (_microsoftFeatureManagementSchemaEnabled)
-            {
-                IConfigurationSection featureFlagsSection = featureManagementConfigurationSection.GetSection(MicrosoftFeatureManagementFields.FeatureFlagsSectionName);
-
-                return featureFlagsSection.GetChildren();
-            }
-
-            return featureManagementConfigurationSection.GetChildren();
-        }
-
         private T ParseEnum<T>(string feature, string rawValue, string fieldKeyword)
             where T : struct, Enum
         {

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"
-              ]
-            }
-          }
-        }
-      ]
     }
   }
 }