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

feat: Using factory for provider creation #157

Conversation

vpetrusevici
Copy link

@vpetrusevici vpetrusevici commented Oct 27, 2023

This PR

  • adds possibility to use factories for getting provider. With this feature we can easily control lifecycle of provider instances (not only singleton).

Notes

This is a problem when your provider uses some services like HttpClient (which should be scoped or transient to avoid DNS problems) or configuration that can be changed in runtime. Using factories we can configure lifecycle of providers by registering a client in DI.

How to test

var flagsmithConfig = builder.Configuration.GetSection(nameof(FlagsmithConfiguration));
var providerConfig = new FlagsmithProviderConfiguration();
Api.Instance.SetProvider(() => new FlagsmithProvider(providerConfig, flagsmithConfig.Get<FlagsmithConfiguration>()));
var evaluationContextbuilder = EvaluationContext.Builder();
evaluationContextbuilder.Set(providerConfig.TargetingKey, "Your key");
var context = evaluationContextbuilder.Build();
services.AddScoped<IFeatureClient>(serviceProvider => Api.Instance.GetClient(logger: serviceProvider.GetRequiredService<ILogger<FeatureClient>>(), context: context));

In this example I can change configuration at runtime and be sure that new Provider with all deps will be created per request.
You still can set singleton instance of provider or register client itself as singleton.

This is not the best DI using, but it looks difficult to change current static implementation without major changes

@vpetrusevici vpetrusevici requested a review from a team as a code owner October 27, 2023 09:07
Signed-off-by: Vladimir Petrusevici <V.Petrusevici@maib.md>
@vpetrusevici vpetrusevici force-pushed the use-factories-to-create-providers branch from de9d787 to cc5fd4d Compare October 27, 2023 09:08
@vpetrusevici vpetrusevici changed the title Using factory for provider creation feat: Using factory for provider creation Oct 27, 2023
@codecov
Copy link

codecov bot commented Oct 27, 2023

Codecov Report

Merging #157 (9aff8a7) into main (9c42d4a) will increase coverage by 0.02%.
Report is 6 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #157      +/-   ##
==========================================
+ Coverage   93.91%   93.93%   +0.02%     
==========================================
  Files          20       20              
  Lines         542      544       +2     
  Branches       39       41       +2     
==========================================
+ Hits          509      511       +2     
  Misses         20       20              
  Partials       13       13              
Files Coverage Δ
src/OpenFeature/Api.cs 100.00% <100.00%> (ø)
src/OpenFeature/OpenFeatureClient.cs 96.10% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Vladimir Petrusevici added 4 commits October 27, 2023 12:12
Signed-off-by: Vladimir Petrusevici <V.Petrusevici@maib.md>
Signed-off-by: Vladimir Petrusevici <V.Petrusevici@maib.md>
Signed-off-by: Vladimir Petrusevici <V.Petrusevici@maib.md>
Signed-off-by: Vladimir Petrusevici <V.Petrusevici@maib.md>
@vpetrusevici
Copy link
Author

vpetrusevici commented Oct 27, 2023

I have another (I think better) option. Just make API constuctor public. So we could use smth like this

services.Configure<FlagsmithConfiguration>(builder.Configuration.GetSection(nameof(FlagsmithConfiguration)));
services.AddScoped<IFlagsmithConfiguration>(x => x.GetRequiredService<IOptions<FlagsmithConfiguration>>().Value);
services.AddHttpClient<IFlagsmithClient, FlagsmithClient>();
services.AddScoped<FeatureProvider, FlagsmithProvider>();
services.AddScoped<IFeatureClient>(serviceProvider =>
{
    var instance = new Api();
    instance.SetProvider(serviceProvider.GetRequiredService<FeatureProvider>());
    return instance.GetClient(logger: serviceProvider.GetRequiredService<ILogger<FeatureClient>>(), context: context);
});

@vpetrusevici
Copy link
Author

or even like this


var flagsmithConfig = builder.Configuration.GetSection(nameof(FlagsmithConfiguration));
var providerConfig = new FlagsmithProviderConfiguration();

var evaluationContextbuilder = EvaluationContext.Builder();
evaluationContextbuilder.Set(providerConfig.TargetingKey, "Luna-middleware");
var context = evaluationContextbuilder.Build();

services.Configure<FlagsmithConfiguration>(builder.Configuration.GetSection(nameof(FlagsmithConfiguration)));
services.AddScoped<IFlagsmithConfiguration>(serviceProvider => {
    var config = serviceProvider.GetRequiredService<IOptions<FlagsmithConfiguration>>().Value;
    config.Logger = serviceProvider.GetRequiredService<ILogger<FlagsmithClient>>();
    return config;
    });
services.AddHttpClient<IFlagsmithClient, FlagsmithClient>();
services.AddScoped<FeatureProvider, FlagsmithProvider>();
services.AddScoped((serviceProvider) => {
    var instance = (Api)Activator.CreateInstance(typeof(Api), true)!; //here should be public constructor
    instance.SetProvider(serviceProvider.GetRequiredService<FeatureProvider>());
    return instance;
});
services.AddScoped<IFeatureClient>(serviceProvider =>
{
    var instance = serviceProvider.GetRequiredService<Api>();
    return instance.GetClient(logger: serviceProvider.GetRequiredService<ILogger<FeatureClient>>(), context: context);
});

@vpetrusevici
Copy link
Author

With .net8 we could even use keyed-services to inject specific client.

@vpetrusevici
Copy link
Author

vpetrusevici commented Oct 27, 2023

My final working solution with DI and without factories and singleton providers:

public static class ServiceCollectionExtensions
{
    /// <summary>
    /// Adds all necessary dependencies for feature toggles
    /// </summary>
    /// <param name="services"></param>
    /// <param name="configuration"></param>
    /// <returns></returns>
    public static IServiceCollection AddFeatureToggle(this IServiceCollection services, IConfiguration configuration)
    {
        var evaluationContextbuilder = EvaluationContext.Builder();
        evaluationContextbuilder.Set(new FlagsmithProviderConfiguration().TargetingKey, "Luna-middleware");
        var context = evaluationContextbuilder.Build();

        return services.Configure<FlagsmithConfiguration>(configuration.GetSection(nameof(FlagsmithConfiguration)))
            .AddScoped<IFlagsmithConfiguration>(serviceProvider => {
                var config = serviceProvider.GetRequiredService<IOptions<FlagsmithConfiguration>>().Value;
                config.Logger = serviceProvider.GetRequiredService<ILogger<FlagsmithClient>>();
                return config;
            })
            .AddSingleton<IFlagsmithProviderConfiguration, FlagsmithProviderConfiguration>()
            .AddHttpClient<IFlagsmithClient, CustomFlagsmithClient>(nameof(FlagsmithClient))
            .Services
            .AddScoped<FeatureProvider, CustomFlagsmithProvider>()
            .AddScoped((serviceProvider) => {
                var instance = (OpenFeature.Api)Activator.CreateInstance(typeof(OpenFeature.Api), true)!;
                instance.SetProvider(serviceProvider.GetRequiredService<FeatureProvider>());
                return instance;
            })
            .AddScoped<IFeatureClient>(serviceProvider =>
            {
                var instance = serviceProvider.GetRequiredService<OpenFeature.Api>();
                return instance.GetClient(logger: serviceProvider.GetRequiredService<ILogger<FeatureClient>>(), context: context);
            });
    }
}

@toddbaert
Copy link
Member

In this example I can change configuration at runtime and be sure that new Provider with all deps will be created per request.

I'm a bit confused. I can definitely see the value of a client per request (we use this pattern internally at my org) but not a provider per request. A provider per request seems like a bad idea in many cases - there's lots of overhead associated with initializing some providers. Am I misunderstanding?

@vpetrusevici
Copy link
Author

In this example I can change configuration at runtime and be sure that new Provider with all deps will be created per request.

I'm a bit confused. I can definitely see the value of a client per request (we use this pattern internally at my org) but not a provider per request. A provider per request seems like a bad idea in many cases - there's lots of overhead associated with initializing some providers. Am I misunderstanding?

I want to say that some of implementations need transient or scoped lifecycle (like flagsmith which uses httpClient).
In these examples (with public constructor or with factories) we have options.

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