Skip to content

support variant service match mode#607

Closed
zhiyuanliang-ms wants to merge 2 commits into
zhiyuanliang/keyed-vspfrom
zhiyuanliang/vs-attribute
Closed

support variant service match mode#607
zhiyuanliang-ms wants to merge 2 commits into
zhiyuanliang/keyed-vspfrom
zhiyuanliang/vs-attribute

Conversation

@zhiyuanliang-ms
Copy link
Copy Markdown
Member

Why this PR?

A follow up PR for #606, trying to resolve #604

Today VariantServiceProvider<T> only selects an implementation based on the assigned variant name. This PR adds an alternative mode where the implementation is selected based on the feature flag's enabled status (true/false), useful when a feature has no variants (e.g. a simple toggle flag).

Introduce VariantServiceMatchMode enum which has Variant (default) and Status value.

Usage:

// Service implementations
[VariantServiceAlias(true)]
public class EnabledCalculator : ICalculator { /* ... */ }

[VariantServiceAlias(false)]
public class DisabledCalculator : ICalculator { /* ... */ }

// Registration (keyed for lazy instantiation)
services.AddKeyedSingleton<ICalculator, EnabledCalculator>(true);
services.AddKeyedSingleton<ICalculator, DisabledCalculator>(false);

// Or normal registration
// services.AddSingleton<ICalculator, EnabledCalculator>();
// services.AddSingleton<ICalculator, DisabledCalculator>();

services.AddFeatureManagement()
    .WithVariantService<ICalculator>("AdvancedMath", VariantServiceMatchMode.Status);

Note that the AddKeyed* family takes object? serviceKey, not just string.
image

string implementationName = ((VariantServiceAliasAttribute)Attribute.GetCustomAttribute(implementationType, typeof(VariantServiceAliasAttribute)))?.Alias;
if (_serviceProvider is IKeyedServiceProvider)
{
TService keyedService = _serviceProvider.GetKeyedService<TService>(enabled);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

what if i want to register my feature services against other keys?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I didn't test it. But my expectation is that you can register the same service multiple times using different keys,

Copy link
Copy Markdown

@Stepami Stepami May 28, 2026

Choose a reason for hiding this comment

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

I'm trying to tell that if I want to use some key other than the feature status the variant sp won't get my impl as it doesn't know and can't know it.

The point is for VariantSp to be able to resolve my service not just with boolean key but any user-provided one

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hey, @Stepami This is a valid call. The solution we are considering is that:
Remove the VariantServiceMatchMode
Introduce a new VariantServiceProviderOptions (the name is not decided)

Service.AddFeatureManagement()
       .WithVariantService<TService>("MyFeature", new VariantServiceProviderOptions()
       {
           EnabledKey (NTD, we can have a better name) = "VariantServiceA", // work as fallback, pnly take effect where there is no variant allocated
           DisabledKey = "VariantServiceB"
       });

What do you think about this?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yeah, i think that's right direction into flexibility. But i also liked the idea of feature status value as default key if there's none from user. Maybe we could do some static factory methods, like:

VariantServiceProviderOptions.Variant()
VariantServiceProviderOptions.Status() // same as Status(enabledKey: true, disabledKey: false)
VariantServiceProviderOptions.Status(enabledKey: "VariantA", disabledKey: "VariantB")

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The existing options in this library: FeatureManagementOptions and TargetingEvaluationOptions. We don't have the pattern like Options.XXX()

My preference would be to keep the options class property-based, this is more common in .NET

We can have default value for EnabledKey and DisabledKey

But, the feature flag status matching will be the fallback. If variant is not null and the name can match any implementation, that implementation will be returned. Variant has higher priority than flag status during the service matching.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

But, the feature flag status matching will be the fallback. If variant is not null and the name can match any implementation, that implementation will be returned. Variant has higher priority than flag status during the service matching.

Yeah it's totally makes sense considering that the old schema is the fallback

image

@zhiyuanliang-ms
Copy link
Copy Markdown
Member Author

Integrated to #606

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.

2 participants