feat(VariantBasedInjection): Keyed DI support

[Stepami]

Why this PR?

Keyed DI can be used in FeatureManagement library as package-provided solution despite netstandard targeting

• Lazy VariantServiceProvider #564 (comment)

Visible Changes

VariableServiceProvider accepts IServiceProvider instead of service enumerable. It allows to check whether a keyed service can be retrieved; if so, lazy on-demand instantiation is achieved.

Feature flag status-based fallback. When no allocated variant matches, the provider now resolves an implementation by the flag's enabled status. Lets consumers wire up a simple on/off pair without configuring variants.
New API:
• VariantServiceProviderOptions — FallbackWhenEnabled / FallbackWhenDisabled aliases.
• WithVariantService(featureName, options) overload. The existing overload now passes default options

Closes #564 #604

[Stepami]

@microsoft-github-policy-service agree

[Stepami]

I just thought that to avoid BC i could introduce LazyVariantServiceProvider class with keyed di as new implementation of IVariantServiceProvider. Then add WithLazyVariantService extension method. That could allow mark current implementation as obsolete to prepare consumers

[Stepami]

@zhiyuanliang-ms could you please take a look?

Managed to avoid BC through making Keyed DI implementation as the new feature

[zhiyuanliang-ms]

Hey, @Stepami

Thank you for your contribution!

The implementation looks good to me overall. Instead of introducing a new API like WithLazyVariantService, I think we should consider updating the existing WithVariantService API and make this behavior configurable, for example:

builder.Services.AddFeatureManagement()
    .WithVariantService<ICalculator>("Calculator", useKeyedService: true);

My only concern is around compatibility. Keyed services are a .NET 8 DI feature, as I mentioned in this comment, while we still have customers running on .NET Framework. We have been hitted by a similar compatibility issue before #435

From a compilation perspective, using the keyed DI APIs should be fine since the 8.x DependencyInjection abstractions package supports netstandard.

My concern is runtime compatibility. Although GetKeyedService is exposed as an extension method on IServiceProvider, it only works when the actual provider implements IKeyedServiceProvider. Otherwise, it throws an InvalidOperationException indicating that keyed services are not supported.

Because of that, this is not necessarily a .NET Framework issue by itself. It can work on .NET Framework if the application uses a compatible Microsoft.Extensions.DependencyInjection 8.x service provider. The risk is for consumers using a custom provider.

I think the existing non-keyed behavior should remain the default, and the keyed DI path should be explicitly opt-in with clear validation/error messaging when the current service provider does not support IKeyedServiceProvider.

[zhiyuanliang-ms]

cc @jimmyca15 @linglingye001

[Stepami]

Hey, @zhiyuanliang-ms

Thanks for the feedback! I like your proposal.

To make things clear i have a few questions:

• How do I update WithVariantService API: create new overload or update the current one?
• ServiceProvider is not built during ServiceCollection phase, so we will only find out whether it is keyed or not during service instantiation. Do we really need the validation other than that InvalidOperationException?

[zhiyuanliang-ms]

Hey, @Stepami

How do I update WithVariantService API: create new overload or update the current one?

I think we should add a new overload. Because replacing the existing one with WithVariantService(string, bool = false) would be source-compatible but binary-breaking for already compiled consumers.

ServiceProvider is not built during ServiceCollection phase, so we will only find out whether it is keyed or not during service instantiation. Do we really need the validation other than that InvalidOperationException?

To be honest, I don't have a perfect answer now.

My current thinking is that best effort is probably enough. If users explicitly enable the keyed-service path, the happy path is straightforward: they are expected to register their services with AddKeyedXXX and use a service provider that supports keyed services. In that case, everything should work as expected.

The unhappy path is where the user enables useKeyedService: true, but the actual runtime service provider does not support IKeyedServiceProvider, or the keyed service was not registered with the expected key. Since we cannot reliably validate this during the ServiceCollection phase, I think the best we can do is catch the generic DI exception and re-throw it with "feature management"-specific context

[Stepami]

@zhiyuanliang-ms

I decided to express the intentions around compatibility through typing in LazyVariantServiceProvider.

Now ctor accepts IKeyedServiceProvider. If sp is not keyed then we can't instantiate LazyVariantServiceProvider and tell consumers about it with exception

[zhiyuanliang-ms]

Hey, @Stepami

Could you take a look at #606

Yesterday, I discussed your PR with @jimmyca15 and we realized that we can have the current VariantServiceProvider to take service provider directly and check whether keyed service is available during the runtime. Then, there is no need to have a new LazyVariantServiceProvider class. But we can have fallback logic so that the configurable option useKeyedService can be saved as well.

[Stepami]

Hey, @zhiyuanliang-ms !

Left comments there

[Stepami]

Hey, @zhiyuanliang-ms! Can you take a look at my updates considering status based fallback resolution ?

I have slightly different take on this:

• VariantServiceProviderOptions properties are object instead of string to conform keyed DI flexibility as mentioned in the end of support variant service match mode #607 description
• Implication of previous update is default status keys are true and false respectively
• I don't see much sense in nullable options as we always provide them in builder methods
• Typing updates inside Vsp internal caching

[zhiyuanliang-ms]

Hey, @Stepami

Thanks you for the review.

I don't see much sense in nullable options as we always provide them in builder methods

You are right. My original thought is that VariantServiceProvider is a public class. It is not necessarily used through DI and our APIs. But I agree with you. It doesn't make too much sense. Updated.

I just realized it is an internal class. Then, you are 100% right.

VariantServiceProviderOptions properties are object instead of string to conform keyed DI flexibility

I know that the internal key for keyed service provider is object not just string. But we've already established the pattern that matching string (variant name) to variant service implementain. And we have VariantServiceAlias where we give service implementation an alias. This is the expected and recommended usage. We actually don't want the variant name to match the service class/type time, instead the mapping relationship should be specified through the alias attribute.

So, instead of

service.AddKeyedSingleton<TService, DefaultEnabledService>(true);

I would prefer to have

service.AddKeyedSingleton<TService, DefaultEnabledService>(DefaultServiceAlias.WhenEnabled);

And more importantly, even if user cannot or doesn't use keyed service registration. With VariantServiceAlias + VariantServiceProviderOptions, they can still achieve the feature flag status fallback, because the previous variant service name matching behavior persists.

But I can see the value of your proposal. It is more intuitive. But I want to keep the established matching pattern of string alias -> variant service implementation. What do you think?

[Stepami]

Hey, @zhiyuanliang-ms

I see your point about Variant Alias pattern, but it seems forced to me in context of the feature service provider.

Looks like the bare fact of status resolution implemented inside the VSP obligates FSP use the same concepts whereas the variants have nothing to do with feature flag status

I think there is no need to mix those semantics, that's why originally i thought of separate API

[Stepami]

duplicate of #606