Allow different type of filters to share the same Alias #262
Conversation
zhiyuanliang-ms
requested review from
rossgrambo,
jimmyca15 and
amerjusupovic
zhiyuanliang-ms
changed the title
zhiyuanliang-ms
force-pushed
the
zhiyuanliang/allow-dup-alias
branch
from
d434317
to
223aee1
Compare
zhiyuanliang-ms
force-pushed
the
zhiyuanliang/allow-dup-alias
branch
from
223aee1
to
15d6a07
Compare
|
Tuple of filter name and context type will be used as the key for the filter metadata cache: No matter whether the feature flag contains any contextual filter, IsEnabledAsync(string feature, TContext appContext) can be called to evaluate this feature flag. One IFeatureFIlter and multiple IContextualFeatureFilters with different types of context can share one alias. If multiple IFeatureFIlters or multiple IContextualFeatureFilters use the same alias, FeatureManagementError.AmbiguousFeatureFilter will be thrown. Let's say we have a non-contextual filter called FilterA and two contextual filters FilterB and FilterC which accept TypeB and TypeC contexts respectively. All of three filters share the same alias "SharedFilterName". If all of three filters are registered: If only FilterA is registered: If FilterC is registered, but neither Filter A nor FIlterB is registered, |
|
Just noting this down for myself. For targeting context accessor, nothing changes. Because the filter's code controls what accessor/context it depends on. This means you wouldn't be able to have two filters with aliases "AdvancedTargetingFilter" if you wanted them to use accessors to get their context. |
| IFeatureFilterMetadata filter = GetFeatureFilterMetadata(featureFilterConfiguration.Name); | ||
| IFeatureFilterMetadata filter; | ||
|
|
||
| if (useAppContext) |
There was a problem hiding this comment.
If we're now saying a call to IsEnabledAsync("name", Context); will use "name" even if it has no context, then I don't think the useAppContext bool needs to exist anymore.
Just use contextual filter if its defined for the context, otherwise use the no contextual filter.
There was a problem hiding this comment.
Context may be null. I think we still need to distinguish the case that users intend to pass null as the context.
| @@ -165,7 +175,7 @@ private async Task<bool> IsEnabledAsync<TContext>(string feature, TContext appCo | |||
|
|
|||
| // | |||
| // IContextualFeatureFilter | |||
| if (useAppContext) | |||
| if (useAppContext && ContextualFeatureFilterEvaluator.IsContextualFilter(filter, typeof(TContext))) | |||
There was a problem hiding this comment.
Why is the IsContextualFilter function written like it is? Couldn't it just be interfaceType.IsAssignableFrom(classType) ? I know it's not a part of this PR, but curious.
There was a problem hiding this comment.
There is a staic method IsContextualFilter() of ContextualFeatureFilterEvaluator.
In my previous commit, my code looked like:
if (useAppContext && ! (filter is IFeatureFilter))
I think it is better to use IsContextualFilter() because it is more readable.
| // | ||
| // 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' |
There was a problem hiding this comment.
Out of curiosity, what if there is a filter MyProduct.MyFilter as well as MyOrg.MyProduct.MyFilter? We treat them as distinct names?
There was a problem hiding this comment.
Yes. If the configuration of a feature flag contains any filters with a namespace, the feature manager will try to find a registered filter whose alias is the same as the filter name in the feature flag configuration.
| @@ -267,48 +277,72 @@ private void BindSettings(IFeatureFilterMetadata filter, FeatureFilterEvaluation | |||
| context.Settings = settings; | |||
| } | |||
|
|
|||
| private IFeatureFilterMetadata GetFeatureFilterMetadata(string filterName) | |||
| private bool IsFilterNameMatched(Type filterType, string filterName) | |||
There was a problem hiding this comment.
Needs to move after the private methods that call it to preserve consistent scroll-flow.
There was a problem hiding this comment.
| private bool IsFilterNameMatched(Type filterType, string filterName) | |
| private bool IsMatchingName(Type filterType, string filterName) |
There was a problem hiding this comment.
I don't see an update to the name
| @@ -165,7 +175,7 @@ private async Task<bool> IsEnabledAsync<TContext>(string feature, TContext appCo | |||
|
|
|||
| // | |||
| // IContextualFeatureFilter | |||
There was a problem hiding this comment.
if (useAppContext && ContextualFeatureFilterEvaluator.IsContextualFilter(filter, typeof(TContext)))
At this point we should already know whether the filter is a contextual filter or not by whether filter was populated with or without an app context.
zhiyuanliang-ms
force-pushed
the
zhiyuanliang/allow-dup-alias
branch
from
416bd5c
to
b0bd5df
Compare
Allow filters to share the same Alias. This PR will resolve #98.
One FeatureFilter and multiple ContextualFeatureFilters using different types of context can share the same Alias.
We plan to register built-in filter including percentage, time window and contextual targeting filters by default when AddFeatureManagement() is called.
However, the contextual targeting and targeting filter share the same alias "Microsoft.Targeting". Currently, feature management does not allow filters share the same alias. This PR will also un-block this.