Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Register built-in filter by default #287

Merged
merged 10 commits into from
Oct 26, 2023

Conversation

zhiyuanliang-ms
Copy link
Contributor

Mitigate issue #248

@jimmyca15
Copy link
Member

Should be targeting main branch.

@jimmyca15
Copy link
Member

I don't expect any issues, but can you confirm no issues occur if someone manually adds these with

featureManagementBuilder.AddFeatureFilter or services.AddSingleton<TargetingFilter>

@@ -58,6 +58,12 @@ public static IFeatureManagementBuilder AddFeatureManagement(this IServiceCollec

services.TryAddScoped<IVariantFeatureManagerSnapshot>(sp => sp.GetRequiredService<FeatureManagerSnapshot>());

services.AddSingleton<PercentageFilter>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably a good idea to call our AddFeatureFilter overload. It only adds the filter if it hasn't already been added.

Copy link
Contributor Author

@zhiyuanliang-ms zhiyuanliang-ms Oct 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry. This is my fault. I found that services.AddSingleton<PercentageFilter>(); is completly wrong. Because the feature filter will be injected as IFeatureFilterMetadata. Besides, AddFeatureFilter() will also check whether the feature filter only implement exactly one of IFeatureFilter and IContextualFeatureFilter.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test didn't catch that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All previous testcases called AddFeatureFilter() by themselves. I have not written any new testcases.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like it would be worth to have a test that would've caught the issue.

@@ -58,6 +58,12 @@ public static IFeatureManagementBuilder AddFeatureManagement(this IServiceCollec

services.TryAddScoped<IVariantFeatureManagerSnapshot>(sp => sp.GetRequiredService<FeatureManagerSnapshot>());

services.AddSingleton<PercentageFilter>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably shouldn't add PercentageFilter. It was created to showcase how to write a feature filter. We may want to drop it one day.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed with @zhenlan offline. My concern is that if we have the filter in the library, it makes sense to add it by default. If we want to exclude it by default, then most likely we should just remove it from the library as well.

Given that we do have current usage of this filter, it doesn't make sense to remove it at this point. We agreed that we should add the percentage filter by default.

@zhiyuanliang-ms
Copy link
Contributor Author

@jimmyca15
Currently, we have ContextualTargetingFilter and TargetingFilter which both have alias "Microsoft.Targeting". They cannot be registered at the same time.
We need to allow different types of filter to share the same alias first (PR #262)

@zhiyuanliang-ms zhiyuanliang-ms changed the base branch from preview to main October 19, 2023 05:16
@zhiyuanliang-ms zhiyuanliang-ms force-pushed the zhiyuanliang/register-builtin-filter branch from 6859335 to 089b4ce Compare October 19, 2023 05:17
/// </summary>
/// <typeparam name="T">An implementation of <see cref="ITargetingContextAccessor"/></typeparam>
/// <returns>The feature management builder.</returns>
IFeatureManagementBuilder WithTargeting<T>() where T : ITargetingContextAccessor;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a method on an interface is a breaking change. This should be an extension method. I believe we already have IFeatureManagementBuilderExtensions

@zhiyuanliang-ms zhiyuanliang-ms merged commit b6bade5 into main Oct 26, 2023
2 checks passed
@zhiyuanliang-ms zhiyuanliang-ms deleted the zhiyuanliang/register-builtin-filter branch November 10, 2023 01:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants