Skip to content

Commit ed3fce0

Browse files
zhiyuanliang-mszhiyuanliang-ms
authored
Allow different type of filters to share the same Alias (#262)
* allow dup alias & target on main branch * fix bug: non-contextual filter will not be found when there is context * keep using ContextualFeatureFilterEvaluator.IsContextualFilter * fix bug * use typeof & avoid using repeated code * resolve comments & add testcase * resolve comments * remove unnecessary '?' * use var
1 parent bcbc5d7 commit ed3fce0

File tree

5 files changed

+273
-37
lines changed

5 files changed

+273
-37
lines changed

‎src/Microsoft.FeatureManagement/FeatureManager.cs‎

Lines changed: 69 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,17 @@ private async Task<bool> IsEnabledAsync<TContext>(string feature, TContext appCo
141141
continue;
142142
}
143143

144-
IFeatureFilterMetadata filter = GetFeatureFilterMetadata(featureFilterConfiguration.Name);
144+
IFeatureFilterMetadata filter;
145+
146+
if (useAppContext)
147+
{
148+
filter = GetFeatureFilterMetadata(featureFilterConfiguration.Name, typeof(TContext)) ??
149+
GetFeatureFilterMetadata(featureFilterConfiguration.Name);
150+
}
151+
else
152+
{
153+
filter = GetFeatureFilterMetadata(featureFilterConfiguration.Name);
154+
}
145155

146156
if (filter == null)
147157
{
@@ -163,14 +173,14 @@ private async Task<bool> IsEnabledAsync<TContext>(string feature, TContext appCo
163173
Parameters = featureFilterConfiguration.Parameters
164174
};
165175

176+
BindSettings(filter, context, filterIndex);
177+
166178
//
167179
// IContextualFeatureFilter
168180
if (useAppContext)
169181
{
170182
ContextualFeatureFilterEvaluator contextualFilter = GetContextualFeatureFilter(featureFilterConfiguration.Name, typeof(TContext));
171183

172-
BindSettings(filter, context, filterIndex);
173-
174184
if (contextualFilter != null &&
175185
await contextualFilter.EvaluateAsync(context, appContext).ConfigureAwait(false) == targetEvaluation)
176186
{
@@ -184,9 +194,8 @@ await contextualFilter.EvaluateAsync(context, appContext).ConfigureAwait(false)
184194
// IFeatureFilter
185195
if (filter is IFeatureFilter featureFilter)
186196
{
187-
BindSettings(filter, context, filterIndex);
188-
189-
if (await featureFilter.EvaluateAsync(context).ConfigureAwait(false) == targetEvaluation) {
197+
if (await featureFilter.EvaluateAsync(context).ConfigureAwait(false) == targetEvaluation)
198+
{
190199
enabled = targetEvaluation;
191200

192201
break;
@@ -267,48 +276,39 @@ private void BindSettings(IFeatureFilterMetadata filter, FeatureFilterEvaluation
267276
context.Settings = settings;
268277
}
269278

270-
private IFeatureFilterMetadata GetFeatureFilterMetadata(string filterName)
279+
private IFeatureFilterMetadata GetFeatureFilterMetadata(string filterName, Type appContextType = null)
271280
{
272-
const string filterSuffix = "filter";
273-
274281
IFeatureFilterMetadata filter = _filterMetadataCache.GetOrAdd(
275-
filterName,
282+
$"{filterName}{Environment.NewLine}{appContextType?.FullName}",
276283
(_) => {
277284

278285
IEnumerable<IFeatureFilterMetadata> matchingFilters = _featureFilters.Where(f =>
279286
{
280-
Type t = f.GetType();
281-
282-
string name = ((FilterAliasAttribute)Attribute.GetCustomAttribute(t, typeof(FilterAliasAttribute)))?.Alias;
287+
Type filterType = f.GetType();
283288

284-
if (name == null)
289+
if (!IsMatchingName(filterType, filterName))
285290
{
286-
name = t.Name.EndsWith(filterSuffix, StringComparison.OrdinalIgnoreCase) ? t.Name.Substring(0, t.Name.Length - filterSuffix.Length) : t.Name;
291+
return false;
287292
}
288293

289-
//
290-
// Feature filters can have namespaces in their alias
291-
// 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'
292-
// 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'
293-
if (filterName.Contains('.'))
294+
if (appContextType == null)
294295
{
295-
//
296-
// The configured filter name is namespaced. It must be an exact match.
297-
return string.Equals(name, filterName, StringComparison.OrdinalIgnoreCase);
296+
return (f is IFeatureFilter);
298297
}
299-
else
300-
{
301-
//
302-
// We take the simple name of a filter, E.g. 'MyFilter' for 'MyOrg.MyProduct.MyFilter'
303-
string simpleName = name.Contains('.') ? name.Split('.').Last() : name;
304298

305-
return string.Equals(simpleName, filterName, StringComparison.OrdinalIgnoreCase);
306-
}
299+
return ContextualFeatureFilterEvaluator.IsContextualFilter(f, appContextType);
307300
});
308301

309302
if (matchingFilters.Count() > 1)
310303
{
311-
throw new FeatureManagementException(FeatureManagementError.AmbiguousFeatureFilter, $"Multiple feature filters match the configured filter named '{filterName}'.");
304+
if (appContextType == null)
305+
{
306+
throw new FeatureManagementException(FeatureManagementError.AmbiguousFeatureFilter, $"Multiple feature filters match the configured filter named '{filterName}'.");
307+
}
308+
else
309+
{
310+
throw new FeatureManagementException(FeatureManagementError.AmbiguousFeatureFilter, $"Multiple contextual feature filters match the configured filter named '{filterName}' and context type '{appContextType}'.");
311+
}
312312
}
313313

314314
return matchingFilters.FirstOrDefault();
@@ -318,6 +318,37 @@ private IFeatureFilterMetadata GetFeatureFilterMetadata(string filterName)
318318
return filter;
319319
}
320320

321+
private bool IsMatchingName(Type filterType, string filterName)
322+
{
323+
const string filterSuffix = "filter";
324+
325+
string name = ((FilterAliasAttribute)Attribute.GetCustomAttribute(filterType, typeof(FilterAliasAttribute)))?.Alias;
326+
327+
if (name == null)
328+
{
329+
name = filterType.Name.EndsWith(filterSuffix, StringComparison.OrdinalIgnoreCase) ? filterType.Name.Substring(0, filterType.Name.Length - filterSuffix.Length) : filterType.Name;
330+
}
331+
332+
//
333+
// Feature filters can have namespaces in their alias
334+
// 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'
335+
// 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'
336+
if (filterName.Contains('.'))
337+
{
338+
//
339+
// The configured filter name is namespaced. It must be an exact match.
340+
return string.Equals(name, filterName, StringComparison.OrdinalIgnoreCase);
341+
}
342+
else
343+
{
344+
//
345+
// We take the simple name of a filter, E.g. 'MyFilter' for 'MyOrg.MyProduct.MyFilter'
346+
string simpleName = name.Contains('.') ? name.Split('.').Last() : name;
347+
348+
return string.Equals(simpleName, filterName, StringComparison.OrdinalIgnoreCase);
349+
}
350+
}
351+
321352
private ContextualFeatureFilterEvaluator GetContextualFeatureFilter(string filterName, Type appContextType)
322353
{
323354
if (appContextType == null)
@@ -329,11 +360,14 @@ private ContextualFeatureFilterEvaluator GetContextualFeatureFilter(string filte
329360
$"{filterName}{Environment.NewLine}{appContextType.FullName}",
330361
(_) => {
331362

332-
IFeatureFilterMetadata metadata = GetFeatureFilterMetadata(filterName);
363+
IFeatureFilterMetadata metadata = GetFeatureFilterMetadata(filterName, appContextType);
364+
365+
if (metadata == null)
366+
{
367+
return null;
368+
}
333369

334-
return ContextualFeatureFilterEvaluator.IsContextualFilter(metadata, appContextType) ?
335-
new ContextualFeatureFilterEvaluator(metadata, appContextType) :
336-
null;
370+
return new ContextualFeatureFilterEvaluator(metadata, appContextType);
337371
}
338372
);
339373

‎tests/Tests.FeatureManagement/FeatureManagement.cs‎

Lines changed: 115 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,120 @@ public async Task ReadsOnlyFeatureManagementSection()
8585
}
8686
}
8787

88+
[Fact]
89+
public async Task AllowDuplicatedFilterAlias()
90+
{
91+
const string duplicatedFilterName = "DuplicatedFilterName";
92+
93+
string featureName = Enum.GetName(typeof(Features), Features.FeatureUsesFiltersWithDuplicatedAlias);
94+
95+
IConfiguration config = new ConfigurationBuilder().AddJsonFile("appsettings.json").Build();
96+
97+
var services = new ServiceCollection();
98+
99+
services
100+
.AddSingleton(config)
101+
.AddFeatureManagement()
102+
.AddFeatureFilter<DuplicatedAliasFeatureFilter1>()
103+
.AddFeatureFilter<ContextualDuplicatedAliasFeatureFilterWithAccountContext>()
104+
.AddFeatureFilter<ContextualDuplicatedAliasFeatureFilterWithDummyContext1>()
105+
.AddFeatureFilter<PercentageFilter>();
106+
107+
ServiceProvider serviceProvider = services.BuildServiceProvider();
108+
109+
IFeatureManager featureManager = serviceProvider.GetRequiredService<IFeatureManager>();
110+
111+
var appContext = new AppContext();
112+
113+
var dummyContext = new DummyContext();
114+
115+
var targetingContext = new TargetingContext();
116+
117+
Assert.True(await featureManager.IsEnabledAsync(featureName));
118+
119+
Assert.True(await featureManager.IsEnabledAsync(featureName, appContext));
120+
121+
Assert.True(await featureManager.IsEnabledAsync(featureName, dummyContext));
122+
123+
Assert.True(await featureManager.IsEnabledAsync(featureName, targetingContext));
124+
125+
services = new ServiceCollection();
126+
127+
services
128+
.AddSingleton(config)
129+
.AddFeatureManagement()
130+
.AddFeatureFilter<DuplicatedAliasFeatureFilter1>()
131+
.AddFeatureFilter<PercentageFilter>();
132+
133+
serviceProvider = services.BuildServiceProvider();
134+
135+
featureManager = serviceProvider.GetRequiredService<IFeatureManager>();
136+
137+
Assert.True(await featureManager.IsEnabledAsync(featureName, dummyContext));
138+
139+
services = new ServiceCollection();
140+
141+
services
142+
.AddSingleton(config)
143+
.AddFeatureManagement()
144+
.AddFeatureFilter<DuplicatedAliasFeatureFilter1>()
145+
.AddFeatureFilter<DuplicatedAliasFeatureFilter2>()
146+
.AddFeatureFilter<PercentageFilter>();
147+
148+
serviceProvider = services.BuildServiceProvider();
149+
150+
featureManager = serviceProvider.GetRequiredService<IFeatureManager>();
151+
152+
var ex = await Assert.ThrowsAsync<FeatureManagementException>(
153+
async () =>
154+
{
155+
await featureManager.IsEnabledAsync(featureName);
156+
});
157+
158+
Assert.Equal($"Multiple feature filters match the configured filter named '{duplicatedFilterName}'.", ex.Message);
159+
160+
services = new ServiceCollection();
161+
162+
services
163+
.AddSingleton(config)
164+
.AddFeatureManagement()
165+
.AddFeatureFilter<ContextualDuplicatedAliasFeatureFilterWithDummyContext1>()
166+
.AddFeatureFilter<ContextualDuplicatedAliasFeatureFilterWithDummyContext2>()
167+
.AddFeatureFilter<PercentageFilter>();
168+
169+
serviceProvider = services.BuildServiceProvider();
170+
171+
featureManager = serviceProvider.GetRequiredService<IFeatureManager>();
172+
173+
ex = await Assert.ThrowsAsync<FeatureManagementException>(
174+
async () =>
175+
{
176+
await featureManager.IsEnabledAsync(featureName, dummyContext);
177+
});
178+
179+
Assert.Equal($"Multiple contextual feature filters match the configured filter named '{duplicatedFilterName}' and context type '{typeof(DummyContext)}'.", ex.Message);
180+
181+
services = new ServiceCollection();
182+
183+
services
184+
.AddSingleton(config)
185+
.AddFeatureManagement()
186+
.AddFeatureFilter<ContextualDuplicatedAliasFeatureFilterWithAccountContext>()
187+
.AddFeatureFilter<PercentageFilter>();
188+
189+
serviceProvider = services.BuildServiceProvider();
190+
191+
featureManager = serviceProvider.GetRequiredService<IFeatureManager>();
192+
193+
ex = await Assert.ThrowsAsync<FeatureManagementException>(
194+
async () =>
195+
{
196+
await featureManager.IsEnabledAsync(featureName);
197+
});
198+
199+
Assert.Equal($"The feature filter '{duplicatedFilterName}' specified for feature '{featureName}' was not found.", ex.Message);
200+
}
201+
88202
[Fact]
89203
public async Task CustomFilterContextualTargetingWithNullSetting()
90204
{
@@ -175,7 +289,7 @@ public async Task Percentage()
175289
}
176290
}
177291

178-
Assert.True(enabledCount > 0 && enabledCount < 10);
292+
Assert.True(enabledCount >= 0 && enabledCount < 10);
179293
}
180294

181295
[Fact]

‎tests/Tests.FeatureManagement/Features.cs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ enum Features
1313
ConditionalFeature2,
1414
ContextualFeature,
1515
AnyFilterFeature,
16-
AllFilterFeature
16+
AllFilterFeature,
17+
FeatureUsesFiltersWithDuplicatedAlias
1718
}
1819
}

‎tests/Tests.FeatureManagement/FiltersWithDuplicatedAlias.cs‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
// Copyright (c) Microsoft Corporation.
2+
// Licensed under the MIT license.
3+
//
4+
using Microsoft.FeatureManagement;
5+
using System.Threading.Tasks;
6+
7+
namespace Tests.FeatureManagement
8+
{
9+
interface IDummyContext
10+
{
11+
string DummyProperty { get; set; }
12+
}
13+
14+
class DummyContext : IDummyContext
15+
{
16+
public string DummyProperty { get; set; }
17+
}
18+
19+
[FilterAlias(Alias)]
20+
class DuplicatedAliasFeatureFilter1 : IFeatureFilter
21+
{
22+
private const string Alias = "DuplicatedFilterName";
23+
24+
public Task<bool> EvaluateAsync(FeatureFilterEvaluationContext context)
25+
{
26+
return Task.FromResult(true);
27+
}
28+
}
29+
30+
[FilterAlias(Alias)]
31+
class DuplicatedAliasFeatureFilter2 : IFeatureFilter
32+
{
33+
private const string Alias = "DuplicatedFilterName";
34+
35+
public Task<bool> EvaluateAsync(FeatureFilterEvaluationContext context)
36+
{
37+
return Task.FromResult(true);
38+
}
39+
}
40+
41+
[FilterAlias(Alias)]
42+
class ContextualDuplicatedAliasFeatureFilterWithAccountContext : IContextualFeatureFilter<IAccountContext>
43+
{
44+
private const string Alias = "DuplicatedFilterName";
45+
46+
public Task<bool> EvaluateAsync(FeatureFilterEvaluationContext context, IAccountContext accountContext)
47+
{
48+
return Task.FromResult(true);
49+
}
50+
}
51+
52+
[FilterAlias(Alias)]
53+
class ContextualDuplicatedAliasFeatureFilterWithDummyContext1 : IContextualFeatureFilter<IDummyContext>
54+
{
55+
private const string Alias = "DuplicatedFilterName";
56+
57+
public Task<bool> EvaluateAsync(FeatureFilterEvaluationContext context, IDummyContext dummyContext)
58+
{
59+
return Task.FromResult(true);
60+
}
61+
}
62+
63+
[FilterAlias(Alias)]
64+
class ContextualDuplicatedAliasFeatureFilterWithDummyContext2 : IContextualFeatureFilter<IDummyContext>
65+
{
66+
private const string Alias = "DuplicatedFilterName";
67+
68+
public Task<bool> EvaluateAsync(FeatureFilterEvaluationContext context, IDummyContext dummyContext)
69+
{
70+
return Task.FromResult(true);
71+
}
72+
}
73+
}

0 commit comments

Comments
 (0)