diff --git a/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs b/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs index 72ed0757..b720f94f 100644 --- a/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs +++ b/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs @@ -23,6 +23,7 @@ public sealed class ConfigurationFeatureDefinitionProvider : IFeatureDefinitionP private readonly ConfigurationFeatureDefinitionProviderOptions _options; private IEnumerable _dotnetFeatureDefinitionSections; private IEnumerable _microsoftFeatureDefinitionSections; + private IDictionary _featureDefinitionSchemas; private readonly ConcurrentDictionary> _definitions; private IDisposable _changeSubscription; private int _stale = 0; @@ -31,6 +32,12 @@ public sealed class ConfigurationFeatureDefinitionProvider : IFeatureDefinitionP const string ParseValueErrorString = "Invalid setting '{0}' with value '{1}' for feature '{2}'."; + private enum FeatureDefinitionSchema + { + Dotnet, + Microsoft + } + /// /// Creates a configuration feature definition provider. /// @@ -58,7 +65,7 @@ public ConfigurationFeatureDefinitionProvider( _getFeatureDefinitionFunc = (featureName) => { - return Task.FromResult(GetMicrosoftSchemaFeatureDefinition(featureName) ?? GetDotnetSchemaFeatureDefinition(featureName)); + return Task.FromResult(GetFeatureDefinition(featureName)); }; } @@ -103,9 +110,7 @@ public Task GetFeatureDefinitionAsync(string featureName) if (Interlocked.Exchange(ref _stale, 0) != 0) { - _dotnetFeatureDefinitionSections = GetDotnetFeatureDefinitionSections(); - - _microsoftFeatureDefinitionSections = GetMicrosoftFeatureDefinitionSections(); + LoadFeatureDefinitionSections(); _definitions.Clear(); } @@ -128,18 +133,21 @@ public async IAsyncEnumerable GetAllFeatureDefinitionsAsync() if (Interlocked.Exchange(ref _stale, 0) != 0) { - _dotnetFeatureDefinitionSections = GetDotnetFeatureDefinitionSections(); - - _microsoftFeatureDefinitionSections = GetMicrosoftFeatureDefinitionSections(); + LoadFeatureDefinitionSections(); _definitions.Clear(); } + HashSet processedFeatureNames = _options.CustomConfigurationMergingEnabled + ? new HashSet(StringComparer.OrdinalIgnoreCase) + : null; + foreach (IConfigurationSection featureSection in _microsoftFeatureDefinitionSections) { string featureName = featureSection[MicrosoftFeatureManagementFields.Id]; - if (string.IsNullOrEmpty(featureName)) + if (string.IsNullOrEmpty(featureName) || + (processedFeatureNames != null && !processedFeatureNames.Add(featureName))) { continue; } @@ -158,7 +166,8 @@ public async IAsyncEnumerable GetAllFeatureDefinitionsAsync() { string featureName = featureSection.Key; - if (string.IsNullOrEmpty(featureName)) + if (string.IsNullOrEmpty(featureName) || + (processedFeatureNames != null && !processedFeatureNames.Add(featureName))) { continue; } @@ -178,12 +187,54 @@ private void EnsureInit() { if (_initialized == 0) { - _dotnetFeatureDefinitionSections = GetDotnetFeatureDefinitionSections(); + LoadFeatureDefinitionSections(); + _initialized = 1; + } + } + + private void LoadFeatureDefinitionSections() + { + _dotnetFeatureDefinitionSections = GetDotnetFeatureDefinitionSections(); + + if (!_options.CustomConfigurationMergingEnabled) + { _microsoftFeatureDefinitionSections = GetMicrosoftFeatureDefinitionSections(); + _featureDefinitionSchemas = null; + return; + } - _initialized = 1; + var microsoftFeatureDefinitionSections = new List(); + var featureDefinitionSchemas = new Dictionary(StringComparer.OrdinalIgnoreCase); + + FindFeatureDefinitions(_configuration, microsoftFeatureDefinitionSections, featureDefinitionSchemas); + + // + // Root configuration fallback definitions cannot conflict with Microsoft schema definitions. + foreach (IConfigurationSection featureSection in _dotnetFeatureDefinitionSections.Where(section => !featureDefinitionSchemas.ContainsKey(section.Key))) + { + featureDefinitionSchemas[featureSection.Key] = FeatureDefinitionSchema.Dotnet; } + + _microsoftFeatureDefinitionSections = microsoftFeatureDefinitionSections; + _featureDefinitionSchemas = featureDefinitionSchemas; + } + + private FeatureDefinition GetFeatureDefinition(string featureName) + { + if (!_options.CustomConfigurationMergingEnabled) + { + return GetMicrosoftSchemaFeatureDefinition(featureName) ?? GetDotnetSchemaFeatureDefinition(featureName); + } + + if (!_featureDefinitionSchemas.TryGetValue(featureName, out FeatureDefinitionSchema schema)) + { + return null; + } + + return schema == FeatureDefinitionSchema.Microsoft + ? GetMicrosoftSchemaFeatureDefinition(featureName) + : GetDotnetSchemaFeatureDefinition(featureName); } private FeatureDefinition GetDotnetSchemaFeatureDefinition(string featureName) @@ -239,35 +290,21 @@ private IEnumerable GetDotnetFeatureDefinitionSections() private IEnumerable GetMicrosoftFeatureDefinitionSections() { - if (!_options.CustomConfigurationMergingEnabled) - { - return _configuration.GetSection(MicrosoftFeatureManagementFields.FeatureManagementSectionName) - .GetSection(MicrosoftFeatureManagementFields.FeatureFlagsSectionName) - .GetChildren(); - } - - var featureDefinitionSections = new List(); - - FindFeatureFlags(_configuration, featureDefinitionSections); - - return featureDefinitionSections; + return _configuration.GetSection(MicrosoftFeatureManagementFields.FeatureManagementSectionName) + .GetSection(MicrosoftFeatureManagementFields.FeatureFlagsSectionName) + .GetChildren(); } - private void FindFeatureFlags(IConfiguration configuration, List featureDefinitionSections) + private void FindFeatureDefinitions( + IConfiguration configuration, + List microsoftFeatureDefinitionSections, + IDictionary featureDefinitionSchemas) { if (!(configuration is IConfigurationRoot configurationRoot) || configurationRoot.Providers.Any(provider => !(provider is ConfigurationProvider) && !(provider is ChainedConfigurationProvider))) { - IConfigurationSection featureFlagsSection = configuration - .GetSection(MicrosoftFeatureManagementFields.FeatureManagementSectionName) - .GetSection(MicrosoftFeatureManagementFields.FeatureFlagsSectionName); - - if (featureFlagsSection.Exists()) - { - featureDefinitionSections.AddRange(featureFlagsSection.GetChildren()); - } - + AddFeatureDefinitions(configuration, microsoftFeatureDefinitionSections, featureDefinitionSchemas); return; } @@ -281,18 +318,41 @@ private void FindFeatureFlags(IConfiguration configuration, List microsoftFeatureDefinitionSections, + IDictionary featureDefinitionSchemas) + { + IConfigurationSection dotnetFeatureManagementSection = configuration + .GetSection(DotnetFeatureManagementFields.FeatureManagementSectionName); + + foreach (IConfigurationSection featureSection in dotnetFeatureManagementSection.GetChildren()) + { + featureDefinitionSchemas[featureSection.Key] = FeatureDefinitionSchema.Dotnet; + } + + IConfigurationSection microsoftFeatureFlagsSection = configuration + .GetSection(MicrosoftFeatureManagementFields.FeatureManagementSectionName) + .GetSection(MicrosoftFeatureManagementFields.FeatureFlagsSectionName); + + foreach (IConfigurationSection featureSection in microsoftFeatureFlagsSection.GetChildren()) + { + microsoftFeatureDefinitionSections.Add(featureSection); + + string featureName = featureSection[MicrosoftFeatureManagementFields.Id]; + + if (!string.IsNullOrEmpty(featureName)) + { + featureDefinitionSchemas[featureName] = FeatureDefinitionSchema.Microsoft; } } } diff --git a/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProviderOptions.cs b/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProviderOptions.cs index 3893edc4..f36b3e3b 100644 --- a/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProviderOptions.cs +++ b/src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProviderOptions.cs @@ -9,14 +9,17 @@ namespace Microsoft.FeatureManagement public class ConfigurationFeatureDefinitionProviderOptions { /// - /// Controls whether to enable the custom configuration merging logic for Microsoft schema feature flags or fall back to .NET's native configuration merging behavior. + /// Controls whether to enable custom configuration merging for feature flags from multiple configuration sources. /// /// - /// This option only affects Microsoft schema feature flags (e.g. feature_management:feature_flags arrays). .NET schema feature flags are not affected by this setting. - /// - /// The uses custom configuration merging logic for Microsoft schema feature flags to ensure that - /// feature flags with the same ID from different configuration sources are merged correctly based on their logical identity rather than array position. - /// By default, the provider bypasses .NET's native array merging behavior which merges arrays by index position and can lead to unexpected results when feature flags are defined across multiple configuration sources. + /// The uses custom configuration merging logic to ensure that feature flags with the same ID from + /// different configuration sources are merged correctly based on their logical identity rather than array position. The last configuration source that + /// defines a feature flag wins, even when earlier and later sources use different feature management schemas. If the same configuration source defines a + /// feature flag in both the .NET schema and the Microsoft schema, the Microsoft schema definition takes precedence. + /// + /// .NET schema feature flags continue to use .NET's native configuration merging behavior within that schema. + /// When custom merging is enabled, the provider bypasses .NET's native array merging behavior which merges arrays by index position and can lead to unexpected results when feature flags are defined across multiple configuration sources. + /// When custom merging is disabled, Microsoft schema definitions take precedence over .NET schema definitions regardless of configuration source order. /// /// Consider the following configuration sources: /// Configuration Source 1: diff --git a/tests/Tests.FeatureManagement/FeatureManagementTest.cs b/tests/Tests.FeatureManagement/FeatureManagementTest.cs index a2aaeed1..e485eacf 100644 --- a/tests/Tests.FeatureManagement/FeatureManagementTest.cs +++ b/tests/Tests.FeatureManagement/FeatureManagementTest.cs @@ -255,6 +255,167 @@ public async Task RespectsAllFeatureManagementSchemas() Assert.True(await featureManager.IsEnabledAsync("FeatureZ")); } + [Fact] + public async Task CustomMergingUsesConfigurationSourceOrderAcrossSchemas() + { + var microsoftSchemaFeature = new Dictionary(StringComparer.OrdinalIgnoreCase) + { + ["feature_management:feature_flags:0:id"] = "CrossSchemaFeature", + ["feature_management:feature_flags:0:enabled"] = bool.FalseString + }; + var dotnetSchemaFeature = new Dictionary(StringComparer.OrdinalIgnoreCase) + { + ["FeatureManagement:crossschemafeature:EnabledFor:0:Name"] = "Test" + }; + var dotnetSchemaFeatureParameters = new Dictionary(StringComparer.OrdinalIgnoreCase) + { + ["FeatureManagement:crossschemafeature:EnabledFor:0:Parameters:Source"] = "Dotnet" + }; + var mergeOptions = new ConfigurationFeatureDefinitionProviderOptions + { + CustomConfigurationMergingEnabled = true + }; + + IConfiguration microsoftThenDotnet = new ConfigurationBuilder() + .AddInMemoryCollection(microsoftSchemaFeature) + .AddInMemoryCollection(dotnetSchemaFeature) + .AddInMemoryCollection(dotnetSchemaFeatureParameters) + .Build(); + + using (var provider = new ConfigurationFeatureDefinitionProvider(microsoftThenDotnet, mergeOptions)) + { + FeatureDefinition definition = await provider.GetFeatureDefinitionAsync("CROSSSCHEMAFEATURE"); + FeatureFilterConfiguration filter = Assert.Single(definition.EnabledFor); + + Assert.Equal(FeatureStatus.Conditional, definition.Status); + Assert.Equal("Test", filter.Name); + Assert.Equal("Dotnet", filter.Parameters["Source"]); + } + + IConfiguration dotnetThenMicrosoft = new ConfigurationBuilder() + .AddInMemoryCollection(dotnetSchemaFeature) + .AddInMemoryCollection(microsoftSchemaFeature) + .Build(); + + using (var provider = new ConfigurationFeatureDefinitionProvider(dotnetThenMicrosoft, mergeOptions)) + { + FeatureDefinition definition = await provider.GetFeatureDefinitionAsync("CrossSchemaFeature"); + + Assert.Equal(FeatureStatus.Disabled, definition.Status); + Assert.Empty(definition.EnabledFor); + } + } + + [Fact] + public async Task CustomMergingPrefersMicrosoftSchemaWithinSameConfigurationSource() + { + IConfiguration configuration = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary(StringComparer.OrdinalIgnoreCase) + { + ["FeatureManagement:CrossSchemaFeature"] = bool.TrueString, + ["feature_management:feature_flags:0:id"] = "CrossSchemaFeature", + ["feature_management:feature_flags:0:enabled"] = bool.FalseString + }) + .Build(); + var mergeOptions = new ConfigurationFeatureDefinitionProviderOptions + { + CustomConfigurationMergingEnabled = true + }; + + using var provider = new ConfigurationFeatureDefinitionProvider(configuration, mergeOptions); + + FeatureDefinition definition = await provider.GetFeatureDefinitionAsync("CrossSchemaFeature"); + + Assert.Equal(FeatureStatus.Disabled, definition.Status); + } + + [Fact] + public async Task DefaultMergingContinuesToPreferMicrosoftSchema() + { + IConfiguration configuration = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary(StringComparer.OrdinalIgnoreCase) + { + ["feature_management:feature_flags:0:id"] = "CrossSchemaFeature", + ["feature_management:feature_flags:0:enabled"] = bool.FalseString + }) + .AddInMemoryCollection(new Dictionary(StringComparer.OrdinalIgnoreCase) + { + ["FeatureManagement:CrossSchemaFeature"] = bool.TrueString + }) + .Build(); + + using var provider = new ConfigurationFeatureDefinitionProvider(configuration); + + FeatureDefinition definition = await provider.GetFeatureDefinitionAsync("CrossSchemaFeature"); + + Assert.Equal(FeatureStatus.Disabled, definition.Status); + } + + [Fact] + public async Task CustomMergingDeduplicatesCrossSchemaFeaturesFromChainedConfiguration() + { + IConfiguration innerConfiguration = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary(StringComparer.OrdinalIgnoreCase) + { + ["feature_management:feature_flags:0:id"] = "CrossSchemaFeature", + ["feature_management:feature_flags:0:enabled"] = bool.FalseString + }) + .Build(); + IConfiguration configuration = new ConfigurationBuilder() + .AddConfiguration(innerConfiguration) + .AddInMemoryCollection(new Dictionary(StringComparer.OrdinalIgnoreCase) + { + ["FeatureManagement:crossschemafeature"] = bool.TrueString + }) + .Build(); + var mergeOptions = new ConfigurationFeatureDefinitionProviderOptions + { + CustomConfigurationMergingEnabled = true + }; + + using var provider = new ConfigurationFeatureDefinitionProvider(configuration, mergeOptions); + var definitions = new List(); + + await foreach (FeatureDefinition definition in provider.GetAllFeatureDefinitionsAsync()) + { + definitions.Add(definition); + } + + FeatureDefinition crossSchemaDefinition = Assert.Single(definitions); + Assert.Equal(FeatureStatus.Conditional, crossSchemaDefinition.Status); + Assert.Single(crossSchemaDefinition.EnabledFor); + } + + [Fact] + public async Task CustomMergingRecalculatesSchemaPrecedenceAfterReload() + { + IConfigurationRoot configuration = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary(StringComparer.OrdinalIgnoreCase) + { + ["feature_management:feature_flags:0:id"] = "ReloadFeature", + ["feature_management:feature_flags:0:enabled"] = bool.FalseString + }) + .AddInMemoryCollection() + .Build(); + IConfigurationProvider lastConfigurationProvider = configuration.Providers.Last(); + var mergeOptions = new ConfigurationFeatureDefinitionProviderOptions + { + CustomConfigurationMergingEnabled = true + }; + + using var provider = new ConfigurationFeatureDefinitionProvider(configuration, mergeOptions); + + FeatureDefinition definition = await provider.GetFeatureDefinitionAsync("ReloadFeature"); + Assert.Equal(FeatureStatus.Disabled, definition.Status); + + lastConfigurationProvider.Set("FeatureManagement:ReloadFeature", bool.TrueString); + configuration.Reload(); + + definition = await provider.GetFeatureDefinitionAsync("ReloadFeature"); + Assert.Equal(FeatureStatus.Conditional, definition.Status); + Assert.Single(definition.EnabledFor); + } + [Fact] public async Task ThrowsForMissingFeatures() {