diff --git a/src/Microsoft.FeatureManagement/FeatureManager.cs b/src/Microsoft.FeatureManagement/FeatureManager.cs index 1136b714..9e0c3cde 100644 --- a/src/Microsoft.FeatureManagement/FeatureManager.cs +++ b/src/Microsoft.FeatureManagement/FeatureManager.cs @@ -141,7 +141,17 @@ private async Task IsEnabledAsync(string feature, TContext appCo continue; } - IFeatureFilterMetadata filter = GetFeatureFilterMetadata(featureFilterConfiguration.Name); + IFeatureFilterMetadata filter; + + if (useAppContext) + { + filter = GetFeatureFilterMetadata(featureFilterConfiguration.Name, typeof(TContext)) ?? + GetFeatureFilterMetadata(featureFilterConfiguration.Name); + } + else + { + filter = GetFeatureFilterMetadata(featureFilterConfiguration.Name); + } if (filter == null) { @@ -163,14 +173,14 @@ private async Task IsEnabledAsync(string feature, TContext appCo Parameters = featureFilterConfiguration.Parameters }; + BindSettings(filter, context, filterIndex); + // // IContextualFeatureFilter if (useAppContext) { ContextualFeatureFilterEvaluator contextualFilter = GetContextualFeatureFilter(featureFilterConfiguration.Name, typeof(TContext)); - BindSettings(filter, context, filterIndex); - if (contextualFilter != null && await contextualFilter.EvaluateAsync(context, appContext).ConfigureAwait(false) == targetEvaluation) { @@ -184,9 +194,8 @@ await contextualFilter.EvaluateAsync(context, appContext).ConfigureAwait(false) // IFeatureFilter if (filter is IFeatureFilter featureFilter) { - BindSettings(filter, context, filterIndex); - - if (await featureFilter.EvaluateAsync(context).ConfigureAwait(false) == targetEvaluation) { + if (await featureFilter.EvaluateAsync(context).ConfigureAwait(false) == targetEvaluation) + { enabled = targetEvaluation; break; @@ -267,48 +276,39 @@ private void BindSettings(IFeatureFilterMetadata filter, FeatureFilterEvaluation context.Settings = settings; } - private IFeatureFilterMetadata GetFeatureFilterMetadata(string filterName) + private IFeatureFilterMetadata GetFeatureFilterMetadata(string filterName, Type appContextType = null) { - const string filterSuffix = "filter"; - IFeatureFilterMetadata filter = _filterMetadataCache.GetOrAdd( - filterName, + $"{filterName}{Environment.NewLine}{appContextType?.FullName}", (_) => { IEnumerable matchingFilters = _featureFilters.Where(f => { - Type t = f.GetType(); - - string name = ((FilterAliasAttribute)Attribute.GetCustomAttribute(t, typeof(FilterAliasAttribute)))?.Alias; + Type filterType = f.GetType(); - if (name == null) + if (!IsMatchingName(filterType, filterName)) { - name = t.Name.EndsWith(filterSuffix, StringComparison.OrdinalIgnoreCase) ? t.Name.Substring(0, t.Name.Length - filterSuffix.Length) : t.Name; + return false; } - // - // Feature filters can have namespaces in their alias - // If a feature is configured to use a filter without a namespace such as 'MyFilter', then it can match 'MyOrg.MyProduct.MyFilter' or simply 'MyFilter' - // If a feature is configured to use a filter with a namespace such as 'MyOrg.MyProduct.MyFilter' then it can only match 'MyOrg.MyProduct.MyFilter' - if (filterName.Contains('.')) + if (appContextType == null) { - // - // The configured filter name is namespaced. It must be an exact match. - return string.Equals(name, filterName, StringComparison.OrdinalIgnoreCase); + return (f is IFeatureFilter); } - else - { - // - // We take the simple name of a filter, E.g. 'MyFilter' for 'MyOrg.MyProduct.MyFilter' - string simpleName = name.Contains('.') ? name.Split('.').Last() : name; - return string.Equals(simpleName, filterName, StringComparison.OrdinalIgnoreCase); - } + return ContextualFeatureFilterEvaluator.IsContextualFilter(f, appContextType); }); if (matchingFilters.Count() > 1) { - throw new FeatureManagementException(FeatureManagementError.AmbiguousFeatureFilter, $"Multiple feature filters match the configured filter named '{filterName}'."); + if (appContextType == null) + { + throw new FeatureManagementException(FeatureManagementError.AmbiguousFeatureFilter, $"Multiple feature filters match the configured filter named '{filterName}'."); + } + else + { + throw new FeatureManagementException(FeatureManagementError.AmbiguousFeatureFilter, $"Multiple contextual feature filters match the configured filter named '{filterName}' and context type '{appContextType}'."); + } } return matchingFilters.FirstOrDefault(); @@ -318,6 +318,37 @@ private IFeatureFilterMetadata GetFeatureFilterMetadata(string filterName) return filter; } + private bool IsMatchingName(Type filterType, string filterName) + { + const string filterSuffix = "filter"; + + string name = ((FilterAliasAttribute)Attribute.GetCustomAttribute(filterType, typeof(FilterAliasAttribute)))?.Alias; + + if (name == null) + { + name = filterType.Name.EndsWith(filterSuffix, StringComparison.OrdinalIgnoreCase) ? filterType.Name.Substring(0, filterType.Name.Length - filterSuffix.Length) : filterType.Name; + } + + // + // Feature filters can have namespaces in their alias + // If a feature is configured to use a filter without a namespace such as 'MyFilter', then it can match 'MyOrg.MyProduct.MyFilter' or simply 'MyFilter' + // If a feature is configured to use a filter with a namespace such as 'MyOrg.MyProduct.MyFilter' then it can only match 'MyOrg.MyProduct.MyFilter' + if (filterName.Contains('.')) + { + // + // The configured filter name is namespaced. It must be an exact match. + return string.Equals(name, filterName, StringComparison.OrdinalIgnoreCase); + } + else + { + // + // We take the simple name of a filter, E.g. 'MyFilter' for 'MyOrg.MyProduct.MyFilter' + string simpleName = name.Contains('.') ? name.Split('.').Last() : name; + + return string.Equals(simpleName, filterName, StringComparison.OrdinalIgnoreCase); + } + } + private ContextualFeatureFilterEvaluator GetContextualFeatureFilter(string filterName, Type appContextType) { if (appContextType == null) @@ -329,11 +360,14 @@ private ContextualFeatureFilterEvaluator GetContextualFeatureFilter(string filte $"{filterName}{Environment.NewLine}{appContextType.FullName}", (_) => { - IFeatureFilterMetadata metadata = GetFeatureFilterMetadata(filterName); + IFeatureFilterMetadata metadata = GetFeatureFilterMetadata(filterName, appContextType); + + if (metadata == null) + { + return null; + } - return ContextualFeatureFilterEvaluator.IsContextualFilter(metadata, appContextType) ? - new ContextualFeatureFilterEvaluator(metadata, appContextType) : - null; + return new ContextualFeatureFilterEvaluator(metadata, appContextType); } ); diff --git a/tests/Tests.FeatureManagement/FeatureManagement.cs b/tests/Tests.FeatureManagement/FeatureManagement.cs index 439375a6..27c36c68 100644 --- a/tests/Tests.FeatureManagement/FeatureManagement.cs +++ b/tests/Tests.FeatureManagement/FeatureManagement.cs @@ -85,6 +85,120 @@ public async Task ReadsOnlyFeatureManagementSection() } } + [Fact] + public async Task AllowDuplicatedFilterAlias() + { + const string duplicatedFilterName = "DuplicatedFilterName"; + + string featureName = Enum.GetName(typeof(Features), Features.FeatureUsesFiltersWithDuplicatedAlias); + + IConfiguration config = new ConfigurationBuilder().AddJsonFile("appsettings.json").Build(); + + var services = new ServiceCollection(); + + services + .AddSingleton(config) + .AddFeatureManagement() + .AddFeatureFilter() + .AddFeatureFilter() + .AddFeatureFilter() + .AddFeatureFilter(); + + ServiceProvider serviceProvider = services.BuildServiceProvider(); + + IFeatureManager featureManager = serviceProvider.GetRequiredService(); + + var appContext = new AppContext(); + + var dummyContext = new DummyContext(); + + var targetingContext = new TargetingContext(); + + Assert.True(await featureManager.IsEnabledAsync(featureName)); + + Assert.True(await featureManager.IsEnabledAsync(featureName, appContext)); + + Assert.True(await featureManager.IsEnabledAsync(featureName, dummyContext)); + + Assert.True(await featureManager.IsEnabledAsync(featureName, targetingContext)); + + services = new ServiceCollection(); + + services + .AddSingleton(config) + .AddFeatureManagement() + .AddFeatureFilter() + .AddFeatureFilter(); + + serviceProvider = services.BuildServiceProvider(); + + featureManager = serviceProvider.GetRequiredService(); + + Assert.True(await featureManager.IsEnabledAsync(featureName, dummyContext)); + + services = new ServiceCollection(); + + services + .AddSingleton(config) + .AddFeatureManagement() + .AddFeatureFilter() + .AddFeatureFilter() + .AddFeatureFilter(); + + serviceProvider = services.BuildServiceProvider(); + + featureManager = serviceProvider.GetRequiredService(); + + var ex = await Assert.ThrowsAsync( + async () => + { + await featureManager.IsEnabledAsync(featureName); + }); + + Assert.Equal($"Multiple feature filters match the configured filter named '{duplicatedFilterName}'.", ex.Message); + + services = new ServiceCollection(); + + services + .AddSingleton(config) + .AddFeatureManagement() + .AddFeatureFilter() + .AddFeatureFilter() + .AddFeatureFilter(); + + serviceProvider = services.BuildServiceProvider(); + + featureManager = serviceProvider.GetRequiredService(); + + ex = await Assert.ThrowsAsync( + async () => + { + await featureManager.IsEnabledAsync(featureName, dummyContext); + }); + + Assert.Equal($"Multiple contextual feature filters match the configured filter named '{duplicatedFilterName}' and context type '{typeof(DummyContext)}'.", ex.Message); + + services = new ServiceCollection(); + + services + .AddSingleton(config) + .AddFeatureManagement() + .AddFeatureFilter() + .AddFeatureFilter(); + + serviceProvider = services.BuildServiceProvider(); + + featureManager = serviceProvider.GetRequiredService(); + + ex = await Assert.ThrowsAsync( + async () => + { + await featureManager.IsEnabledAsync(featureName); + }); + + Assert.Equal($"The feature filter '{duplicatedFilterName}' specified for feature '{featureName}' was not found.", ex.Message); + } + [Fact] public async Task CustomFilterContextualTargetingWithNullSetting() { @@ -175,7 +289,7 @@ public async Task Percentage() } } - Assert.True(enabledCount > 0 && enabledCount < 10); + Assert.True(enabledCount >= 0 && enabledCount < 10); } [Fact] diff --git a/tests/Tests.FeatureManagement/Features.cs b/tests/Tests.FeatureManagement/Features.cs index 93847df3..79e1105d 100644 --- a/tests/Tests.FeatureManagement/Features.cs +++ b/tests/Tests.FeatureManagement/Features.cs @@ -13,6 +13,7 @@ enum Features ConditionalFeature2, ContextualFeature, AnyFilterFeature, - AllFilterFeature + AllFilterFeature, + FeatureUsesFiltersWithDuplicatedAlias } } diff --git a/tests/Tests.FeatureManagement/FiltersWithDuplicatedAlias.cs b/tests/Tests.FeatureManagement/FiltersWithDuplicatedAlias.cs new file mode 100644 index 00000000..c1e5f9b4 --- /dev/null +++ b/tests/Tests.FeatureManagement/FiltersWithDuplicatedAlias.cs @@ -0,0 +1,73 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. +// +using Microsoft.FeatureManagement; +using System.Threading.Tasks; + +namespace Tests.FeatureManagement +{ + interface IDummyContext + { + string DummyProperty { get; set; } + } + + class DummyContext : IDummyContext + { + public string DummyProperty { get; set; } + } + + [FilterAlias(Alias)] + class DuplicatedAliasFeatureFilter1 : IFeatureFilter + { + private const string Alias = "DuplicatedFilterName"; + + public Task EvaluateAsync(FeatureFilterEvaluationContext context) + { + return Task.FromResult(true); + } + } + + [FilterAlias(Alias)] + class DuplicatedAliasFeatureFilter2 : IFeatureFilter + { + private const string Alias = "DuplicatedFilterName"; + + public Task EvaluateAsync(FeatureFilterEvaluationContext context) + { + return Task.FromResult(true); + } + } + + [FilterAlias(Alias)] + class ContextualDuplicatedAliasFeatureFilterWithAccountContext : IContextualFeatureFilter + { + private const string Alias = "DuplicatedFilterName"; + + public Task EvaluateAsync(FeatureFilterEvaluationContext context, IAccountContext accountContext) + { + return Task.FromResult(true); + } + } + + [FilterAlias(Alias)] + class ContextualDuplicatedAliasFeatureFilterWithDummyContext1 : IContextualFeatureFilter + { + private const string Alias = "DuplicatedFilterName"; + + public Task EvaluateAsync(FeatureFilterEvaluationContext context, IDummyContext dummyContext) + { + return Task.FromResult(true); + } + } + + [FilterAlias(Alias)] + class ContextualDuplicatedAliasFeatureFilterWithDummyContext2 : IContextualFeatureFilter + { + private const string Alias = "DuplicatedFilterName"; + + public Task EvaluateAsync(FeatureFilterEvaluationContext context, IDummyContext dummyContext) + { + return Task.FromResult(true); + } + } +} diff --git a/tests/Tests.FeatureManagement/appsettings.json b/tests/Tests.FeatureManagement/appsettings.json index 2eeff873..77aa687e 100644 --- a/tests/Tests.FeatureManagement/appsettings.json +++ b/tests/Tests.FeatureManagement/appsettings.json @@ -9,6 +9,20 @@ "FeatureManagement": { "OnTestFeature": true, "OffTestFeature": false, + "FeatureUsesFiltersWithDuplicatedAlias": { + "RequirementType": "all", + "EnabledFor": [ + { + "Name": "DuplicatedFilterName" + }, + { + "Name": "Percentage", + "Parameters": { + "Value": 100 + } + } + ] + }, "TargetingTestFeature": { "EnabledFor": [ {