From 15d6a07ea9e35da535af4489748474004bc6511d Mon Sep 17 00:00:00 2001 From: zhiyuanliang Date: Thu, 19 Oct 2023 14:25:20 +0800 Subject: [PATCH 1/9] allow dup alias & target on main branch --- .../ContextualFeatureFilterEvaluator.cs | 5 - .../FeatureManager.cs | 92 +++++++++------ .../FeatureManagement.cs | 109 ++++++++++++++++++ tests/Tests.FeatureManagement/Features.cs | 3 +- .../FiltersWithDuplicatedAlias.cs | 73 ++++++++++++ .../Tests.FeatureManagement/appsettings.json | 7 ++ 6 files changed, 249 insertions(+), 40 deletions(-) create mode 100644 tests/Tests.FeatureManagement/FiltersWithDuplicatedAlias.cs diff --git a/src/Microsoft.FeatureManagement/ContextualFeatureFilterEvaluator.cs b/src/Microsoft.FeatureManagement/ContextualFeatureFilterEvaluator.cs index baf9220a..cab33c6a 100644 --- a/src/Microsoft.FeatureManagement/ContextualFeatureFilterEvaluator.cs +++ b/src/Microsoft.FeatureManagement/ContextualFeatureFilterEvaluator.cs @@ -53,11 +53,6 @@ public Task EvaluateAsync(FeatureFilterEvaluationContext evaluationContext return _evaluateFunc(_filter, evaluationContext, context); } - public static bool IsContextualFilter(IFeatureFilterMetadata filter, Type appContextType) - { - return GetContextualFilterInterface(filter, appContextType) != null; - } - private static Type GetContextualFilterInterface(IFeatureFilterMetadata filter, Type appContextType) { IEnumerable contextualFilterInterfaces = filter.GetType().GetInterfaces().Where(i => i.IsGenericType && i.GetGenericTypeDefinition().IsAssignableFrom(typeof(IContextualFeatureFilter<>))); diff --git a/src/Microsoft.FeatureManagement/FeatureManager.cs b/src/Microsoft.FeatureManagement/FeatureManager.cs index 1136b714..eaceea55 100644 --- a/src/Microsoft.FeatureManagement/FeatureManager.cs +++ b/src/Microsoft.FeatureManagement/FeatureManager.cs @@ -25,8 +25,8 @@ class FeatureManager : IFeatureManager, IDisposable private readonly IEnumerable _featureFilters; private readonly IEnumerable _sessionManagers; private readonly ILogger _logger; - private readonly ConcurrentDictionary _filterMetadataCache; - private readonly ConcurrentDictionary _contextualFeatureFilterCache; + private readonly ConcurrentDictionary, IFeatureFilterMetadata> _filterMetadataCache; + private readonly ConcurrentDictionary, ContextualFeatureFilterEvaluator> _contextualFeatureFilterCache; private readonly FeatureManagementOptions _options; private readonly IMemoryCache _parametersCache; @@ -48,8 +48,8 @@ public FeatureManager( _featureFilters = featureFilters ?? throw new ArgumentNullException(nameof(featureFilters)); _sessionManagers = sessionManagers ?? throw new ArgumentNullException(nameof(sessionManagers)); _logger = loggerFactory.CreateLogger(); - _filterMetadataCache = new ConcurrentDictionary(); - _contextualFeatureFilterCache = new ConcurrentDictionary(); + _filterMetadataCache = new ConcurrentDictionary, IFeatureFilterMetadata>(); + _contextualFeatureFilterCache = new ConcurrentDictionary, ContextualFeatureFilterEvaluator>(); _options = options?.Value ?? throw new ArgumentNullException(nameof(options)); _parametersCache = new MemoryCache(new MemoryCacheOptions()); } @@ -141,11 +141,13 @@ private async Task IsEnabledAsync(string feature, TContext appCo continue; } - IFeatureFilterMetadata filter = GetFeatureFilterMetadata(featureFilterConfiguration.Name); + IFeatureFilterMetadata filter = GetFeatureFilterMetadata(featureFilterConfiguration.Name, appContext?.GetType()); if (filter == null) { - string errorMessage = $"The feature filter '{featureFilterConfiguration.Name}' specified for feature '{feature}' was not found."; + string errorMessage = appContext == null + ? $"The feature filter '{featureFilterConfiguration.Name}' specified for feature '{featureDefinition.Name}' was not found." + : $"The contextual feature filter '{featureFilterConfiguration.Name}' with context type '{appContext.GetType()}', specified for feature '{featureDefinition.Name}' was not found."; if (!_options.IgnoreMissingFeatureFilters) { @@ -267,48 +269,72 @@ private void BindSettings(IFeatureFilterMetadata filter, FeatureFilterEvaluation context.Settings = settings; } - private IFeatureFilterMetadata GetFeatureFilterMetadata(string filterName) + private bool IsFilterNameMatched(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 IFeatureFilterMetadata GetFeatureFilterMetadata(string filterName, Type appContextType) + { IFeatureFilterMetadata filter = _filterMetadataCache.GetOrAdd( - filterName, + (filterName, appContextType), (_) => { 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 (appContextType == null) { - name = t.Name.EndsWith(filterSuffix, StringComparison.OrdinalIgnoreCase) ? t.Name.Substring(0, t.Name.Length - filterSuffix.Length) : t.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); + return (f is IFeatureFilter) && IsFilterNameMatched(filterType, filterName); } else { - // - // We take the simple name of a filter, E.g. 'MyFilter' for 'MyOrg.MyProduct.MyFilter' - string simpleName = name.Contains('.') ? name.Split('.').Last() : name; + IEnumerable contextualFilterInterfaces = filterType.GetInterfaces().Where( + i => i.IsGenericType && + i.GetGenericTypeDefinition().IsAssignableFrom(typeof(IContextualFeatureFilter<>)) && + i.GetGenericArguments()[0].IsAssignableFrom(appContextType)); - return string.Equals(simpleName, filterName, StringComparison.OrdinalIgnoreCase); + return contextualFilterInterfaces.Any() && IsFilterNameMatched(filterType, filterName); } }); 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(); @@ -326,14 +352,12 @@ private ContextualFeatureFilterEvaluator GetContextualFeatureFilter(string filte } ContextualFeatureFilterEvaluator filter = _contextualFeatureFilterCache.GetOrAdd( - $"{filterName}{Environment.NewLine}{appContextType.FullName}", + (filterName, appContextType), (_) => { - IFeatureFilterMetadata metadata = GetFeatureFilterMetadata(filterName); + IFeatureFilterMetadata metadata = GetFeatureFilterMetadata(filterName, appContextType); - 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..34f13d2a 100644 --- a/tests/Tests.FeatureManagement/FeatureManagement.cs +++ b/tests/Tests.FeatureManagement/FeatureManagement.cs @@ -85,6 +85,115 @@ public async Task ReadsOnlyFeatureManagementSection() } } + [Fact] + public async Task AllowDuplicatedFilterAlias() + { + const string duplicatedFilterName = "DuplicatedFilterName"; + + IConfiguration config = new ConfigurationBuilder().AddJsonFile("appsettings.json").Build(); + + var services = new ServiceCollection(); + + services + .AddSingleton(config) + .AddFeatureManagement() + .AddFeatureFilter() + .AddFeatureFilter() + .AddFeatureFilter(); + + ServiceProvider serviceProvider = services.BuildServiceProvider(); + + IFeatureManager featureManager = serviceProvider.GetRequiredService(); + + AppContext appContext = new AppContext(); + + DummyContext dummyContext = new DummyContext(); + + Assert.True(await featureManager.IsEnabledAsync(Enum.GetName(typeof(Features), Features.FeatureUsesFiltersWithDuplicatedAlias))); + + Assert.True(await featureManager.IsEnabledAsync(Enum.GetName(typeof(Features), Features.FeatureUsesFiltersWithDuplicatedAlias), appContext)); + + Assert.True(await featureManager.IsEnabledAsync(Enum.GetName(typeof(Features), Features.FeatureUsesFiltersWithDuplicatedAlias), dummyContext)); + + services = new ServiceCollection(); + + services + .AddSingleton(config) + .AddFeatureManagement() + .AddFeatureFilter() + .AddFeatureFilter(); + + serviceProvider = services.BuildServiceProvider(); + + featureManager = serviceProvider.GetRequiredService(); + + FeatureManagementException ex = await Assert.ThrowsAsync( + async () => + { + await featureManager.IsEnabledAsync(Enum.GetName(typeof(Features), Features.FeatureUsesFiltersWithDuplicatedAlias)); + }); + + Assert.Equal($"Multiple feature filters match the configured filter named '{duplicatedFilterName}'.", 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(Enum.GetName(typeof(Features), Features.FeatureUsesFiltersWithDuplicatedAlias), dummyContext); + }); + + Assert.Equal($"Multiple contextual feature filters match the configured filter named '{duplicatedFilterName}' and context type '{dummyContext.GetType()}'.", ex.Message); + + services = new ServiceCollection(); + + services + .AddSingleton(config) + .AddFeatureManagement() + .AddFeatureFilter(); + + serviceProvider = services.BuildServiceProvider(); + + featureManager = serviceProvider.GetRequiredService(); + + ex = await Assert.ThrowsAsync( + async () => + { + await featureManager.IsEnabledAsync(Enum.GetName(typeof(Features), Features.FeatureUsesFiltersWithDuplicatedAlias)); + }); + + Assert.Equal($"The feature filter '{duplicatedFilterName}' specified for feature '{Enum.GetName(typeof(Features), Features.FeatureUsesFiltersWithDuplicatedAlias)}' was not found.", ex.Message); + + services = new ServiceCollection(); + + services + .AddSingleton(config) + .AddFeatureManagement() + .AddFeatureFilter(); + + serviceProvider = services.BuildServiceProvider(); + + featureManager = serviceProvider.GetRequiredService(); + + ex = await Assert.ThrowsAsync( + async () => + { + await featureManager.IsEnabledAsync(Enum.GetName(typeof(Features), Features.FeatureUsesFiltersWithDuplicatedAlias), dummyContext); + }); + + Assert.Equal($"The contextual feature filter '{duplicatedFilterName}' with context type '{dummyContext.GetType()}', specified for feature '{Enum.GetName(typeof(Features), Features.FeatureUsesFiltersWithDuplicatedAlias)}' was not found.", ex.Message); + } + [Fact] public async Task CustomFilterContextualTargetingWithNullSetting() { 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..a4963e2f --- /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; } + } + + 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..0741f3ed 100644 --- a/tests/Tests.FeatureManagement/appsettings.json +++ b/tests/Tests.FeatureManagement/appsettings.json @@ -9,6 +9,13 @@ "FeatureManagement": { "OnTestFeature": true, "OffTestFeature": false, + "FeatureUsesFiltersWithDuplicatedAlias": { + "EnabledFor": [ + { + "Name": "DuplicatedFilterName" + } + ] + }, "TargetingTestFeature": { "EnabledFor": [ { From eb412f463bf4ab6a67926b62559f00b9170b500f Mon Sep 17 00:00:00 2001 From: zhiyuanliang Date: Mon, 23 Oct 2023 12:59:20 +0800 Subject: [PATCH 2/9] fix bug: non-contextual filter will not be found when there is context --- .../FeatureManager.cs | 18 +++++--- .../FeatureManagement.cs | 41 +++++++++---------- .../FiltersWithDuplicatedAlias.cs | 2 +- .../Tests.FeatureManagement/appsettings.json | 7 ++++ 4 files changed, 41 insertions(+), 27 deletions(-) diff --git a/src/Microsoft.FeatureManagement/FeatureManager.cs b/src/Microsoft.FeatureManagement/FeatureManager.cs index eaceea55..66c3f6cf 100644 --- a/src/Microsoft.FeatureManagement/FeatureManager.cs +++ b/src/Microsoft.FeatureManagement/FeatureManager.cs @@ -141,13 +141,21 @@ private async Task IsEnabledAsync(string feature, TContext appCo continue; } - IFeatureFilterMetadata filter = GetFeatureFilterMetadata(featureFilterConfiguration.Name, appContext?.GetType()); + IFeatureFilterMetadata filter; + + if (appContext == null) + { + filter = GetFeatureFilterMetadata(featureFilterConfiguration.Name, null); + } + else + { + filter = GetFeatureFilterMetadata(featureFilterConfiguration.Name, appContext.GetType()) ?? + GetFeatureFilterMetadata(featureFilterConfiguration.Name, null); + } if (filter == null) { - string errorMessage = appContext == null - ? $"The feature filter '{featureFilterConfiguration.Name}' specified for feature '{featureDefinition.Name}' was not found." - : $"The contextual feature filter '{featureFilterConfiguration.Name}' with context type '{appContext.GetType()}', specified for feature '{featureDefinition.Name}' was not found."; + string errorMessage = $"The feature filter '{featureFilterConfiguration.Name}' specified for feature '{feature}' was not found."; if (!_options.IgnoreMissingFeatureFilters) { @@ -167,7 +175,7 @@ private async Task IsEnabledAsync(string feature, TContext appCo // // IContextualFeatureFilter - if (useAppContext) + if (useAppContext && !(filter is IFeatureFilter)) { ContextualFeatureFilterEvaluator contextualFilter = GetContextualFeatureFilter(featureFilterConfiguration.Name, typeof(TContext)); diff --git a/tests/Tests.FeatureManagement/FeatureManagement.cs b/tests/Tests.FeatureManagement/FeatureManagement.cs index 34f13d2a..68e27695 100644 --- a/tests/Tests.FeatureManagement/FeatureManagement.cs +++ b/tests/Tests.FeatureManagement/FeatureManagement.cs @@ -99,7 +99,8 @@ public async Task AllowDuplicatedFilterAlias() .AddFeatureManagement() .AddFeatureFilter() .AddFeatureFilter() - .AddFeatureFilter(); + .AddFeatureFilter() + .AddFeatureFilter(); ServiceProvider serviceProvider = services.BuildServiceProvider(); @@ -121,46 +122,43 @@ public async Task AllowDuplicatedFilterAlias() .AddSingleton(config) .AddFeatureManagement() .AddFeatureFilter() - .AddFeatureFilter(); + .AddFeatureFilter(); serviceProvider = services.BuildServiceProvider(); featureManager = serviceProvider.GetRequiredService(); - FeatureManagementException ex = await Assert.ThrowsAsync( - async () => - { - await featureManager.IsEnabledAsync(Enum.GetName(typeof(Features), Features.FeatureUsesFiltersWithDuplicatedAlias)); - }); - - Assert.Equal($"Multiple feature filters match the configured filter named '{duplicatedFilterName}'.", ex.Message); + Assert.True(await featureManager.IsEnabledAsync(Enum.GetName(typeof(Features), Features.FeatureUsesFiltersWithDuplicatedAlias), dummyContext)); services = new ServiceCollection(); services .AddSingleton(config) .AddFeatureManagement() - .AddFeatureFilter() - .AddFeatureFilter(); + .AddFeatureFilter() + .AddFeatureFilter() + .AddFeatureFilter(); serviceProvider = services.BuildServiceProvider(); featureManager = serviceProvider.GetRequiredService(); - ex = await Assert.ThrowsAsync( + FeatureManagementException ex = await Assert.ThrowsAsync( async () => { - await featureManager.IsEnabledAsync(Enum.GetName(typeof(Features), Features.FeatureUsesFiltersWithDuplicatedAlias), dummyContext); + await featureManager.IsEnabledAsync(Enum.GetName(typeof(Features), Features.FeatureUsesFiltersWithDuplicatedAlias)); }); - Assert.Equal($"Multiple contextual feature filters match the configured filter named '{duplicatedFilterName}' and context type '{dummyContext.GetType()}'.", ex.Message); + Assert.Equal($"Multiple feature filters match the configured filter named '{duplicatedFilterName}'.", ex.Message); services = new ServiceCollection(); services .AddSingleton(config) .AddFeatureManagement() - .AddFeatureFilter(); + .AddFeatureFilter() + .AddFeatureFilter() + .AddFeatureFilter(); serviceProvider = services.BuildServiceProvider(); @@ -169,17 +167,18 @@ public async Task AllowDuplicatedFilterAlias() ex = await Assert.ThrowsAsync( async () => { - await featureManager.IsEnabledAsync(Enum.GetName(typeof(Features), Features.FeatureUsesFiltersWithDuplicatedAlias)); + await featureManager.IsEnabledAsync(Enum.GetName(typeof(Features), Features.FeatureUsesFiltersWithDuplicatedAlias), dummyContext); }); - Assert.Equal($"The feature filter '{duplicatedFilterName}' specified for feature '{Enum.GetName(typeof(Features), Features.FeatureUsesFiltersWithDuplicatedAlias)}' was not found.", ex.Message); + Assert.Equal($"Multiple contextual feature filters match the configured filter named '{duplicatedFilterName}' and context type '{dummyContext.GetType()}'.", ex.Message); services = new ServiceCollection(); services .AddSingleton(config) .AddFeatureManagement() - .AddFeatureFilter(); + .AddFeatureFilter() + .AddFeatureFilter(); serviceProvider = services.BuildServiceProvider(); @@ -188,10 +187,10 @@ public async Task AllowDuplicatedFilterAlias() ex = await Assert.ThrowsAsync( async () => { - await featureManager.IsEnabledAsync(Enum.GetName(typeof(Features), Features.FeatureUsesFiltersWithDuplicatedAlias), dummyContext); + await featureManager.IsEnabledAsync(Enum.GetName(typeof(Features), Features.FeatureUsesFiltersWithDuplicatedAlias)); }); - Assert.Equal($"The contextual feature filter '{duplicatedFilterName}' with context type '{dummyContext.GetType()}', specified for feature '{Enum.GetName(typeof(Features), Features.FeatureUsesFiltersWithDuplicatedAlias)}' was not found.", ex.Message); + Assert.Equal($"The feature filter '{duplicatedFilterName}' specified for feature '{Enum.GetName(typeof(Features), Features.FeatureUsesFiltersWithDuplicatedAlias)}' was not found.", ex.Message); } [Fact] @@ -284,7 +283,7 @@ public async Task Percentage() } } - Assert.True(enabledCount > 0 && enabledCount < 10); + Assert.True(enabledCount >= 0 && enabledCount < 10); } [Fact] diff --git a/tests/Tests.FeatureManagement/FiltersWithDuplicatedAlias.cs b/tests/Tests.FeatureManagement/FiltersWithDuplicatedAlias.cs index a4963e2f..c1e5f9b4 100644 --- a/tests/Tests.FeatureManagement/FiltersWithDuplicatedAlias.cs +++ b/tests/Tests.FeatureManagement/FiltersWithDuplicatedAlias.cs @@ -8,7 +8,7 @@ namespace Tests.FeatureManagement { interface IDummyContext { - string DummyProperty { get; } + string DummyProperty { get; set; } } class DummyContext : IDummyContext diff --git a/tests/Tests.FeatureManagement/appsettings.json b/tests/Tests.FeatureManagement/appsettings.json index 0741f3ed..77aa687e 100644 --- a/tests/Tests.FeatureManagement/appsettings.json +++ b/tests/Tests.FeatureManagement/appsettings.json @@ -10,9 +10,16 @@ "OnTestFeature": true, "OffTestFeature": false, "FeatureUsesFiltersWithDuplicatedAlias": { + "RequirementType": "all", "EnabledFor": [ { "Name": "DuplicatedFilterName" + }, + { + "Name": "Percentage", + "Parameters": { + "Value": 100 + } } ] }, From 745ea3e51fbb932f12ace1c1c5f6c5798122ffca Mon Sep 17 00:00:00 2001 From: zhiyuanliang Date: Mon, 23 Oct 2023 13:40:46 +0800 Subject: [PATCH 3/9] keep using ContextualFeatureFilterEvaluator.IsContextualFilter --- .../ContextualFeatureFilterEvaluator.cs | 5 +++++ src/Microsoft.FeatureManagement/FeatureManager.cs | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/Microsoft.FeatureManagement/ContextualFeatureFilterEvaluator.cs b/src/Microsoft.FeatureManagement/ContextualFeatureFilterEvaluator.cs index cab33c6a..baf9220a 100644 --- a/src/Microsoft.FeatureManagement/ContextualFeatureFilterEvaluator.cs +++ b/src/Microsoft.FeatureManagement/ContextualFeatureFilterEvaluator.cs @@ -53,6 +53,11 @@ public Task EvaluateAsync(FeatureFilterEvaluationContext evaluationContext return _evaluateFunc(_filter, evaluationContext, context); } + public static bool IsContextualFilter(IFeatureFilterMetadata filter, Type appContextType) + { + return GetContextualFilterInterface(filter, appContextType) != null; + } + private static Type GetContextualFilterInterface(IFeatureFilterMetadata filter, Type appContextType) { IEnumerable contextualFilterInterfaces = filter.GetType().GetInterfaces().Where(i => i.IsGenericType && i.GetGenericTypeDefinition().IsAssignableFrom(typeof(IContextualFeatureFilter<>))); diff --git a/src/Microsoft.FeatureManagement/FeatureManager.cs b/src/Microsoft.FeatureManagement/FeatureManager.cs index 66c3f6cf..15df738b 100644 --- a/src/Microsoft.FeatureManagement/FeatureManager.cs +++ b/src/Microsoft.FeatureManagement/FeatureManager.cs @@ -175,7 +175,7 @@ private async Task IsEnabledAsync(string feature, TContext appCo // // IContextualFeatureFilter - if (useAppContext && !(filter is IFeatureFilter)) + if (useAppContext && ContextualFeatureFilterEvaluator.IsContextualFilter(filter, typeof(TContext))) { ContextualFeatureFilterEvaluator contextualFilter = GetContextualFeatureFilter(featureFilterConfiguration.Name, typeof(TContext)); From b5a10f681d551d838ec33a8f08d1ef72a9714194 Mon Sep 17 00:00:00 2001 From: zhiyuanliang Date: Mon, 23 Oct 2023 16:27:21 +0800 Subject: [PATCH 4/9] fix bug --- src/Microsoft.FeatureManagement/FeatureManager.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Microsoft.FeatureManagement/FeatureManager.cs b/src/Microsoft.FeatureManagement/FeatureManager.cs index 15df738b..31a62198 100644 --- a/src/Microsoft.FeatureManagement/FeatureManager.cs +++ b/src/Microsoft.FeatureManagement/FeatureManager.cs @@ -143,14 +143,14 @@ private async Task IsEnabledAsync(string feature, TContext appCo IFeatureFilterMetadata filter; - if (appContext == null) + if (useAppContext) { - filter = GetFeatureFilterMetadata(featureFilterConfiguration.Name, null); + filter = GetFeatureFilterMetadata(featureFilterConfiguration.Name, typeof(TContext)) ?? + GetFeatureFilterMetadata(featureFilterConfiguration.Name, null); } else { - filter = GetFeatureFilterMetadata(featureFilterConfiguration.Name, appContext.GetType()) ?? - GetFeatureFilterMetadata(featureFilterConfiguration.Name, null); + filter = GetFeatureFilterMetadata(featureFilterConfiguration.Name, null); } if (filter == null) From c4192a53804577b09ba51cdd045f4c789e7bea5f Mon Sep 17 00:00:00 2001 From: zhiyuanliang Date: Mon, 23 Oct 2023 16:43:58 +0800 Subject: [PATCH 5/9] use typeof & avoid using repeated code --- .../FeatureManagement.cs | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/tests/Tests.FeatureManagement/FeatureManagement.cs b/tests/Tests.FeatureManagement/FeatureManagement.cs index 68e27695..cb9f69a0 100644 --- a/tests/Tests.FeatureManagement/FeatureManagement.cs +++ b/tests/Tests.FeatureManagement/FeatureManagement.cs @@ -90,6 +90,8 @@ 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(); @@ -110,11 +112,11 @@ public async Task AllowDuplicatedFilterAlias() DummyContext dummyContext = new DummyContext(); - Assert.True(await featureManager.IsEnabledAsync(Enum.GetName(typeof(Features), Features.FeatureUsesFiltersWithDuplicatedAlias))); + Assert.True(await featureManager.IsEnabledAsync(featureName)); - Assert.True(await featureManager.IsEnabledAsync(Enum.GetName(typeof(Features), Features.FeatureUsesFiltersWithDuplicatedAlias), appContext)); + Assert.True(await featureManager.IsEnabledAsync(featureName, appContext)); - Assert.True(await featureManager.IsEnabledAsync(Enum.GetName(typeof(Features), Features.FeatureUsesFiltersWithDuplicatedAlias), dummyContext)); + Assert.True(await featureManager.IsEnabledAsync(featureName, dummyContext)); services = new ServiceCollection(); @@ -128,7 +130,7 @@ public async Task AllowDuplicatedFilterAlias() featureManager = serviceProvider.GetRequiredService(); - Assert.True(await featureManager.IsEnabledAsync(Enum.GetName(typeof(Features), Features.FeatureUsesFiltersWithDuplicatedAlias), dummyContext)); + Assert.True(await featureManager.IsEnabledAsync(featureName, dummyContext)); services = new ServiceCollection(); @@ -146,7 +148,7 @@ public async Task AllowDuplicatedFilterAlias() FeatureManagementException ex = await Assert.ThrowsAsync( async () => { - await featureManager.IsEnabledAsync(Enum.GetName(typeof(Features), Features.FeatureUsesFiltersWithDuplicatedAlias)); + await featureManager.IsEnabledAsync(featureName); }); Assert.Equal($"Multiple feature filters match the configured filter named '{duplicatedFilterName}'.", ex.Message); @@ -167,10 +169,10 @@ public async Task AllowDuplicatedFilterAlias() ex = await Assert.ThrowsAsync( async () => { - await featureManager.IsEnabledAsync(Enum.GetName(typeof(Features), Features.FeatureUsesFiltersWithDuplicatedAlias), dummyContext); + await featureManager.IsEnabledAsync(featureName, dummyContext); }); - Assert.Equal($"Multiple contextual feature filters match the configured filter named '{duplicatedFilterName}' and context type '{dummyContext.GetType()}'.", ex.Message); + Assert.Equal($"Multiple contextual feature filters match the configured filter named '{duplicatedFilterName}' and context type '{typeof(DummyContext)}'.", ex.Message); services = new ServiceCollection(); @@ -187,10 +189,10 @@ public async Task AllowDuplicatedFilterAlias() ex = await Assert.ThrowsAsync( async () => { - await featureManager.IsEnabledAsync(Enum.GetName(typeof(Features), Features.FeatureUsesFiltersWithDuplicatedAlias)); + await featureManager.IsEnabledAsync(featureName); }); - Assert.Equal($"The feature filter '{duplicatedFilterName}' specified for feature '{Enum.GetName(typeof(Features), Features.FeatureUsesFiltersWithDuplicatedAlias)}' was not found.", ex.Message); + Assert.Equal($"The feature filter '{duplicatedFilterName}' specified for feature '{featureName}' was not found.", ex.Message); } [Fact] From 6082e9f29d6f814d86e64039635f0ddb9f59bb31 Mon Sep 17 00:00:00 2001 From: zhiyuanliang Date: Wed, 25 Oct 2023 10:55:07 +0800 Subject: [PATCH 6/9] resolve comments & add testcase --- .../FeatureManager.cs | 117 ++++++++---------- .../FeatureManagement.cs | 8 +- 2 files changed, 61 insertions(+), 64 deletions(-) diff --git a/src/Microsoft.FeatureManagement/FeatureManager.cs b/src/Microsoft.FeatureManagement/FeatureManager.cs index 31a62198..f8a8d73b 100644 --- a/src/Microsoft.FeatureManagement/FeatureManager.cs +++ b/src/Microsoft.FeatureManagement/FeatureManager.cs @@ -25,8 +25,8 @@ class FeatureManager : IFeatureManager, IDisposable private readonly IEnumerable _featureFilters; private readonly IEnumerable _sessionManagers; private readonly ILogger _logger; - private readonly ConcurrentDictionary, IFeatureFilterMetadata> _filterMetadataCache; - private readonly ConcurrentDictionary, ContextualFeatureFilterEvaluator> _contextualFeatureFilterCache; + private readonly ConcurrentDictionary _filterMetadataCache; + private readonly ConcurrentDictionary _contextualFeatureFilterCache; private readonly FeatureManagementOptions _options; private readonly IMemoryCache _parametersCache; @@ -48,8 +48,8 @@ public FeatureManager( _featureFilters = featureFilters ?? throw new ArgumentNullException(nameof(featureFilters)); _sessionManagers = sessionManagers ?? throw new ArgumentNullException(nameof(sessionManagers)); _logger = loggerFactory.CreateLogger(); - _filterMetadataCache = new ConcurrentDictionary, IFeatureFilterMetadata>(); - _contextualFeatureFilterCache = new ConcurrentDictionary, ContextualFeatureFilterEvaluator>(); + _filterMetadataCache = new ConcurrentDictionary(); + _contextualFeatureFilterCache = new ConcurrentDictionary(); _options = options?.Value ?? throw new ArgumentNullException(nameof(options)); _parametersCache = new MemoryCache(new MemoryCacheOptions()); } @@ -146,11 +146,11 @@ private async Task IsEnabledAsync(string feature, TContext appCo if (useAppContext) { filter = GetFeatureFilterMetadata(featureFilterConfiguration.Name, typeof(TContext)) ?? - GetFeatureFilterMetadata(featureFilterConfiguration.Name, null); + GetFeatureFilterMetadata(featureFilterConfiguration.Name); } else { - filter = GetFeatureFilterMetadata(featureFilterConfiguration.Name, null); + filter = GetFeatureFilterMetadata(featureFilterConfiguration.Name); } if (filter == null) @@ -173,30 +173,25 @@ private async Task IsEnabledAsync(string feature, TContext appCo Parameters = featureFilterConfiguration.Parameters }; - // - // IContextualFeatureFilter - if (useAppContext && ContextualFeatureFilterEvaluator.IsContextualFilter(filter, typeof(TContext))) + if (filter is IFeatureFilter featureFilter) { - ContextualFeatureFilterEvaluator contextualFilter = GetContextualFeatureFilter(featureFilterConfiguration.Name, typeof(TContext)); - BindSettings(filter, context, filterIndex); - if (contextualFilter != null && - await contextualFilter.EvaluateAsync(context, appContext).ConfigureAwait(false) == targetEvaluation) - { + if (await featureFilter.EvaluateAsync(context).ConfigureAwait(false) == targetEvaluation) { enabled = targetEvaluation; break; } } - - // - // IFeatureFilter - if (filter is IFeatureFilter featureFilter) + else { + ContextualFeatureFilterEvaluator contextualFilter = GetContextualFeatureFilter(featureFilterConfiguration.Name, typeof(TContext)); + BindSettings(filter, context, filterIndex); - if (await featureFilter.EvaluateAsync(context).ConfigureAwait(false) == targetEvaluation) { + if (contextualFilter != null && + await contextualFilter.EvaluateAsync(context, appContext).ConfigureAwait(false) == targetEvaluation) + { enabled = targetEvaluation; break; @@ -277,60 +272,27 @@ private void BindSettings(IFeatureFilterMetadata filter, FeatureFilterEvaluation context.Settings = settings; } - private bool IsFilterNameMatched(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 IFeatureFilterMetadata GetFeatureFilterMetadata(string filterName, Type appContextType) + private IFeatureFilterMetadata GetFeatureFilterMetadata(string filterName, Type appContextType = null) { IFeatureFilterMetadata filter = _filterMetadataCache.GetOrAdd( - (filterName, appContextType), + $"{filterName}{Environment.NewLine}{appContextType?.FullName}", (_) => { IEnumerable matchingFilters = _featureFilters.Where(f => { Type filterType = f.GetType(); - if (appContextType == null) + if (!IsMatchingName(filterType, filterName)) { - return (f is IFeatureFilter) && IsFilterNameMatched(filterType, filterName); + return false; } - else - { - IEnumerable contextualFilterInterfaces = filterType.GetInterfaces().Where( - i => i.IsGenericType && - i.GetGenericTypeDefinition().IsAssignableFrom(typeof(IContextualFeatureFilter<>)) && - i.GetGenericArguments()[0].IsAssignableFrom(appContextType)); - return contextualFilterInterfaces.Any() && IsFilterNameMatched(filterType, filterName); + if (appContextType == null) + { + return (f is IFeatureFilter); } + + return ContextualFeatureFilterEvaluator.IsContextualFilter(f, appContextType); }); if (matchingFilters.Count() > 1) @@ -352,6 +314,37 @@ private IFeatureFilterMetadata GetFeatureFilterMetadata(string filterName, Type 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) @@ -360,7 +353,7 @@ private ContextualFeatureFilterEvaluator GetContextualFeatureFilter(string filte } ContextualFeatureFilterEvaluator filter = _contextualFeatureFilterCache.GetOrAdd( - (filterName, appContextType), + $"{filterName}{Environment.NewLine}{appContextType?.FullName}", (_) => { IFeatureFilterMetadata metadata = GetFeatureFilterMetadata(filterName, appContextType); diff --git a/tests/Tests.FeatureManagement/FeatureManagement.cs b/tests/Tests.FeatureManagement/FeatureManagement.cs index cb9f69a0..5c88d6a3 100644 --- a/tests/Tests.FeatureManagement/FeatureManagement.cs +++ b/tests/Tests.FeatureManagement/FeatureManagement.cs @@ -108,9 +108,11 @@ public async Task AllowDuplicatedFilterAlias() IFeatureManager featureManager = serviceProvider.GetRequiredService(); - AppContext appContext = new AppContext(); + var appContext = new AppContext(); - DummyContext dummyContext = new DummyContext(); + var dummyContext = new DummyContext(); + + var targetingContext = new TargetingContext(); Assert.True(await featureManager.IsEnabledAsync(featureName)); @@ -118,6 +120,8 @@ public async Task AllowDuplicatedFilterAlias() Assert.True(await featureManager.IsEnabledAsync(featureName, dummyContext)); + Assert.True(await featureManager.IsEnabledAsync(featureName, targetingContext)); + services = new ServiceCollection(); services From b0bd5df0a726bcd04c0b904021c913169672d783 Mon Sep 17 00:00:00 2001 From: zhiyuanliang Date: Wed, 25 Oct 2023 12:31:04 +0800 Subject: [PATCH 7/9] resolve comments --- .../FeatureManager.cs | 29 ++++++++++++------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/src/Microsoft.FeatureManagement/FeatureManager.cs b/src/Microsoft.FeatureManagement/FeatureManager.cs index f8a8d73b..91a23da7 100644 --- a/src/Microsoft.FeatureManagement/FeatureManager.cs +++ b/src/Microsoft.FeatureManagement/FeatureManager.cs @@ -173,24 +173,28 @@ private async Task IsEnabledAsync(string feature, TContext appCo Parameters = featureFilterConfiguration.Parameters }; - if (filter is IFeatureFilter featureFilter) + BindSettings(filter, context, filterIndex); + + // + // IContextualFeatureFilter + if (useAppContext) { - BindSettings(filter, context, filterIndex); + ContextualFeatureFilterEvaluator contextualFilter = GetContextualFeatureFilter(featureFilterConfiguration.Name, typeof(TContext)); - if (await featureFilter.EvaluateAsync(context).ConfigureAwait(false) == targetEvaluation) { + if (contextualFilter != null && + await contextualFilter.EvaluateAsync(context, appContext).ConfigureAwait(false) == targetEvaluation) + { enabled = targetEvaluation; break; } } - else - { - ContextualFeatureFilterEvaluator contextualFilter = GetContextualFeatureFilter(featureFilterConfiguration.Name, typeof(TContext)); - - BindSettings(filter, context, filterIndex); - if (contextualFilter != null && - await contextualFilter.EvaluateAsync(context, appContext).ConfigureAwait(false) == targetEvaluation) + // + // IFeatureFilter + if (filter is IFeatureFilter featureFilter) + { + if (await featureFilter.EvaluateAsync(context).ConfigureAwait(false) == targetEvaluation) { enabled = targetEvaluation; @@ -358,6 +362,11 @@ private ContextualFeatureFilterEvaluator GetContextualFeatureFilter(string filte IFeatureFilterMetadata metadata = GetFeatureFilterMetadata(filterName, appContextType); + if (metadata == null) + { + return null; + } + return new ContextualFeatureFilterEvaluator(metadata, appContextType); } ); From f57bd7fe358e21e0039e795c36e5f42fdf37f671 Mon Sep 17 00:00:00 2001 From: zhiyuanliang Date: Thu, 26 Oct 2023 10:57:26 +0800 Subject: [PATCH 8/9] remove unnecessary '?' --- src/Microsoft.FeatureManagement/FeatureManager.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Microsoft.FeatureManagement/FeatureManager.cs b/src/Microsoft.FeatureManagement/FeatureManager.cs index 91a23da7..9e0c3cde 100644 --- a/src/Microsoft.FeatureManagement/FeatureManager.cs +++ b/src/Microsoft.FeatureManagement/FeatureManager.cs @@ -357,7 +357,7 @@ private ContextualFeatureFilterEvaluator GetContextualFeatureFilter(string filte } ContextualFeatureFilterEvaluator filter = _contextualFeatureFilterCache.GetOrAdd( - $"{filterName}{Environment.NewLine}{appContextType?.FullName}", + $"{filterName}{Environment.NewLine}{appContextType.FullName}", (_) => { IFeatureFilterMetadata metadata = GetFeatureFilterMetadata(filterName, appContextType); From d39de057897c2d466a6d2504ac742f8ad584e619 Mon Sep 17 00:00:00 2001 From: zhiyuanliang Date: Thu, 26 Oct 2023 11:12:10 +0800 Subject: [PATCH 9/9] use var --- tests/Tests.FeatureManagement/FeatureManagement.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Tests.FeatureManagement/FeatureManagement.cs b/tests/Tests.FeatureManagement/FeatureManagement.cs index 5c88d6a3..27c36c68 100644 --- a/tests/Tests.FeatureManagement/FeatureManagement.cs +++ b/tests/Tests.FeatureManagement/FeatureManagement.cs @@ -149,7 +149,7 @@ public async Task AllowDuplicatedFilterAlias() featureManager = serviceProvider.GetRequiredService(); - FeatureManagementException ex = await Assert.ThrowsAsync( + var ex = await Assert.ThrowsAsync( async () => { await featureManager.IsEnabledAsync(featureName);