dotnet-sdk icon indicating copy to clipboard operation
dotnet-sdk copied to clipboard

feat: Extract minimal DI integration to OpenFeature.Providers.DependencyInjection

Open arttonoyan opened this issue 2 months ago • 9 comments

[!IMPORTANT] Update. 2025-11-10 Following the discussion in https://github.com/open-feature/dotnet-sdk/pull/596#discussion_r2495975595. the package name in this proposal is updated from OpenFeature.DependencyInjection to OpenFeature.Providers.DependencyInjection. Rationale. clearer scope for provider authors. thinner provider packages. reduced coupling with the SDK.

This PR

Extract the minimal, provider-agnostic DI surface into a new package: OpenFeature.DependencyInjection.Abstractions. This isolates the contracts and lightweight wiring needed to integrate any OpenFeature provider without pulling in a concrete implementation.

Related Issues

Fixes #587

Notes

  • Options split: Extracted provider-agnostic configuration from OpenFeatureOptions into a new base options type: OpenFeatureProviderOptions.
  • Inheritance: OpenFeatureOptions now inherits from OpenFeatureProviderOptions.
  • Internal wiring update: Internal configuration that previously targeted OpenFeatureOptions now targets OpenFeatureProviderOptions.

Before (internal)

services.AddOptions<OpenFeatureOptions>()
    .Configure(options =>
    {
        options.AddProviderName(null);
    });

After (internal)

services.AddOptions<OpenFeatureProviderOptions>()
    .Configure(options =>
    {
        options.AddProviderName(null);
    });

Note: This is an internal refactor. It does not affect consumers unless they were directly modifying OpenFeatureOptions via DI options configuration (which is not a supported/typical usage).


Impact

  • No new functionality.
  • Behavior unchanged at runtime (same provider selection, same evaluation outcomes, same logs).
  • Binary/source compatibility: Consumers using the documented extension methods and standard registration remain unaffected.
  • Potential touchpoints: Only internal code paths (or unconventional consumer code directly configuring OpenFeatureOptions) require updates to target OpenFeatureProviderOptions.

How to test

This is a regression-only refactor.

Expectations

  • All existing unit/integration tests pass unchanged.
  • No DI resolution or startup configuration differences.

arttonoyan avatar Oct 04 '25 22:10 arttonoyan

Codecov Report

:x: Patch coverage is 94.84536% with 5 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 90.16%. Comparing base (10a43c9) to head (0a03843).

Files with missing lines Patch % Lines
...yInjection/OpenFeatureProviderBuilderExtensions.cs 90.24% 0 Missing and 4 partials :warning:
...eature.Hosting/Internal/FeatureLifecycleManager.cs 93.75% 0 Missing and 1 partial :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #596      +/-   ##
==========================================
- Coverage   90.25%   90.16%   -0.10%     
==========================================
  Files          79       82       +3     
  Lines        3284     3303      +19     
  Branches      384      390       +6     
==========================================
+ Hits         2964     2978      +14     
  Misses        251      251              
- Partials       69       74       +5     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar Oct 04 '25 22:10 codecov[bot]

@askpt @kylejuliandev I’ve made all the requested changes. I only left out the README.md. If that’s OK, I’ll handle it in a separate ticket. Please review again.

Quick question: Should I apply the same changes to the DI package, or skip it because it’s obsolete? Need Help: I can’t figure out the dotnet format / check-format job error. Could you help with this?

arttonoyan avatar Nov 06 '25 19:11 arttonoyan

@askpt @kylejuliandev I’ve made all the requested changes. I only left out the README.md. If that’s OK, I’ll handle it in a separate ticket. Please review again.

Quick question: Should I apply the same changes to the DI package, or skip it because it’s obsolete? Need Help: I can’t figure out the dotnet format / check-format job error. Could you help with this?

@arttonoyan just execute dotnet format OpenFeature.slnx at the root of the repository to clear the error. You also have a DCO failure.

askpt avatar Nov 07 '25 08:11 askpt

@askpt @kylejuliandev Could you please do a final review? I’d like to merge this PR.

arttonoyan avatar Nov 11 '25 04:11 arttonoyan

@arttonoyan Please update this action: https://github.com/open-feature/dotnet-sdk/blob/main/.github/workflows/release.yml. This package needs to be added to generate the security artefacts.

askpt avatar Nov 11 '25 06:11 askpt

@arttonoyan Please update this action: https://github.com/open-feature/dotnet-sdk/blob/main/.github/workflows/release.yml. This package needs to be added to generate the security artefacts.

done: https://github.com/open-feature/dotnet-sdk/pull/596/commits/579ac49c004c21a942e53d3adaa7fd19a13b6191

arttonoyan avatar Nov 11 '25 08:11 arttonoyan

@askpt @kylejuliandev I ran into a blocking issue. I tried to avoid any major changes, but it looks like I can’t solve it without doing at least some refactoring. If you can help with a decision, that would be great.

The problem

While working on the OpenFeature.Providers.DependencyInjection package, I hit an issue with this configuration:

builder.Services.AddOpenFeature(of => of
    .AddProvider(_ => new TestProvider())
    .AddHook(_ => new TestHook())
);

This fails because:

  • AddProvider is an extension for OpenFeatureProviderBuilder
  • AddHook is an extension for OpenFeatureBuilder
  • AddProvider returns OpenFeatureProviderBuilder, which does not have AddHook, so the second call is not available and we get a compile-time error.

If I reverse the order, it works:

builder.Services.AddOpenFeature(of => of
    .AddHook(_ => new TestHook())
    .AddProvider(_ => new TestProvider())
);

Here AddHook returns OpenFeatureBuilder, which inherits from OpenFeatureProviderBuilder, so both methods are available and everything compiles.

For me this is a design problem, because the API becomes order-dependent and confusing. I tried to make OpenFeatureProviderBuilder generic, but that didn’t really solve it.

I see two options. I’m open to other ideas as well.


Option 1. Move OpenFeatureBuilder into OpenFeature.Providers.DependencyInjection and remove OpenFeatureProviderBuilder

We would keep all extensions targeting OpenFeatureBuilder.

  • Pros. Simple. fast to implement.
  • Cons. All extensions would depend on the OpenFeature.Providers.DependencyInjection package, even when they are not really about providers or DI (for example, hooks).

Option 2. Create OpenFeature.DependencyInjection.Abstractions package

We create a new package. OpenFeature.DependencyInjection.Abstractions, move all general DI-related types there, and remove OpenFeatureProviderBuilder. All extensions would be built on top of OpenFeatureBuilder.

  • Pros. Cleaner separation. All DI logic depends on a lightweight abstractions package. In the future, if we want to split hooks or other concerns into separate packages, it will be easier.
  • Cons. One more package to maintain. This new package will probably contain most of the logic from the old OpenFeature.DependencyInjection package.

I slightly prefer Option 2, even though it introduces an extra package, because it gives us a cleaner and more future-proof structure. cc: @beeme1mr @toddbaert @lukas-reining @thomaspoignant @kinyoklion

arttonoyan avatar Nov 20 '25 17:11 arttonoyan

@askpt @kylejuliandev I ran into a blocking issue. I tried to avoid any major changes, but it looks like I can’t solve it without doing at least some refactoring. If you can help with a decision, that would be great.

The problem

While working on the OpenFeature.Providers.DependencyInjection package, I hit an issue with this configuration:

builder.Services.AddOpenFeature(of => of
    .AddProvider(_ => new TestProvider())
    .AddHook(_ => new TestHook())
);

This fails because:

  • AddProvider is an extension for OpenFeatureProviderBuilder
  • AddHook is an extension for OpenFeatureBuilder
  • AddProvider returns OpenFeatureProviderBuilder, which does not have AddHook, so the second call is not available and we get a compile-time error.

If I reverse the order, it works:

builder.Services.AddOpenFeature(of => of
    .AddHook(_ => new TestHook())
    .AddProvider(_ => new TestProvider())
);

Here AddHook returns OpenFeatureBuilder, which inherits from OpenFeatureProviderBuilder, so both methods are available and everything compiles.

For me this is a design problem, because the API becomes order-dependent and confusing. I tried to make OpenFeatureProviderBuilder generic, but that didn’t really solve it.

I see two options. I’m open to other ideas as well.

Option 1. Move OpenFeatureBuilder into OpenFeature.Providers.DependencyInjection and remove OpenFeatureProviderBuilder

We would keep all extensions targeting OpenFeatureBuilder.

  • Pros. Simple. fast to implement.
  • Cons. All extensions would depend on the OpenFeature.Providers.DependencyInjection package, even when they are not really about providers or DI (for example, hooks).

Option 2. Create OpenFeature.DependencyInjection.Abstractions package

We create a new package. OpenFeature.DependencyInjection.Abstractions, move all general DI-related types there, and remove OpenFeatureProviderBuilder. All extensions would be built on top of OpenFeatureBuilder.

  • Pros. Cleaner separation. All DI logic depends on a lightweight abstractions package. In the future, if we want to split hooks or other concerns into separate packages, it will be easier.
  • Cons. One more package to maintain. This new package will probably contain most of the logic from the old OpenFeature.DependencyInjection package.

I slightly prefer Option 2, even though it introduces an extra package, because it gives us a cleaner and more future-proof structure. cc: @beeme1mr @toddbaert @lukas-reining @thomaspoignant @kinyoklion

Need to have a thought. If this requires another package moving/creation/deprecation we might as well do a v3 like I mentioned in Slack.

I kinda prefer Option B also, but moving the code all the time doesn't bring the necessary stability, even if this is still in Experimental phase.

I would like to know what will be the consequences for the providers and hooks if we go ahead with option B. Which package should it consume? OpenFeature.Providers.Abstractions and OpenFeature.DependencyInjection.Abstractions?

Then the consumers would just install OpenFeature, OpenFeature.Hosting and OpenFeature.Providers.ProviderA?

To finish, I would believe if we go with option B, we would need to do a v3, make the DI not-experimental and this PR will be part of v3.

askpt avatar Nov 20 '25 21:11 askpt

@askpt @kylejuliandev I ran into a blocking issue. I tried to avoid any major changes, but it looks like I can’t solve it without doing at least some refactoring. If you can help with a decision, that would be great. The problem While working on the OpenFeature.Providers.DependencyInjection package, I hit an issue with this configuration:

builder.Services.AddOpenFeature(of => of
    .AddProvider(_ => new TestProvider())
    .AddHook(_ => new TestHook())
);

This fails because:

  • AddProvider is an extension for OpenFeatureProviderBuilder
  • AddHook is an extension for OpenFeatureBuilder
  • AddProvider returns OpenFeatureProviderBuilder, which does not have AddHook, so the second call is not available and we get a compile-time error.

If I reverse the order, it works:

builder.Services.AddOpenFeature(of => of
    .AddHook(_ => new TestHook())
    .AddProvider(_ => new TestProvider())
);

Here AddHook returns OpenFeatureBuilder, which inherits from OpenFeatureProviderBuilder, so both methods are available and everything compiles. For me this is a design problem, because the API becomes order-dependent and confusing. I tried to make OpenFeatureProviderBuilder generic, but that didn’t really solve it. I see two options. I’m open to other ideas as well.

Option 1. Move OpenFeatureBuilder into OpenFeature.Providers.DependencyInjection and remove OpenFeatureProviderBuilder

We would keep all extensions targeting OpenFeatureBuilder.

  • Pros. Simple. fast to implement.
  • Cons. All extensions would depend on the OpenFeature.Providers.DependencyInjection package, even when they are not really about providers or DI (for example, hooks).

Option 2. Create OpenFeature.DependencyInjection.Abstractions package

We create a new package. OpenFeature.DependencyInjection.Abstractions, move all general DI-related types there, and remove OpenFeatureProviderBuilder. All extensions would be built on top of OpenFeatureBuilder.

  • Pros. Cleaner separation. All DI logic depends on a lightweight abstractions package. In the future, if we want to split hooks or other concerns into separate packages, it will be easier.
  • Cons. One more package to maintain. This new package will probably contain most of the logic from the old OpenFeature.DependencyInjection package.

I slightly prefer Option 2, even though it introduces an extra package, because it gives us a cleaner and more future-proof structure. cc: @beeme1mr @toddbaert @lukas-reining @thomaspoignant @kinyoklion

Need to have a thought. If this requires another package moving/creation/deprecation we might as well do a v3 like I mentioned in Slack.

I kinda prefer Option B also, but moving the code all the time doesn't bring the necessary stability, even if this is still in Experimental phase.

I would like to know what will be the consequences for the providers and hooks if we go ahead with option B. Which package should it consume? OpenFeature.Providers.Abstractions and OpenFeature.DependencyInjection.Abstractions?

Then the consumers would just install OpenFeature, OpenFeature.Hosting and OpenFeature.Providers.ProviderA?

To finish, I would believe if we go with option B, we would need to do a v3, make the DI not-experimental and this PR will be part of v3.

Here is an updated version you can paste into the PR with the extra Cons included.


@askpt there is also an Option C that lets us continue without any breaking changes. I tried it locally and it is working.

Option C. Make OpenFeatureProviderBuilder generic internally while keeping the public API stable

We introduce an internal generic version of the builder OpenFeatureProviderBuilder<TBuilder> but keep the existing public OpenFeatureProviderBuilder class as is. The public API does not change. so this is not a breaking change.

Public type (unchanged).

public class OpenFeatureProviderBuilder
    : OpenFeatureProviderBuilder<OpenFeatureProviderBuilder>
{
    public OpenFeatureProviderBuilder(IServiceCollection services)
        : base(services) { }
}

Internal generic base.

internal abstract class OpenFeatureProviderBuilder<TBuilder>
    where TBuilder : OpenFeatureProviderBuilder<TBuilder>
{
    protected IServiceCollection Services { get; }

    protected OpenFeatureProviderBuilder(IServiceCollection services)
    {
        Services = services;
    }

    protected TBuilder This => (TBuilder)this;
}

OpenFeatureBuilder uses the same base.

public sealed class OpenFeatureBuilder
    : OpenFeatureProviderBuilder<OpenFeatureBuilder>
{
    public OpenFeatureBuilder(IServiceCollection services)
        : base(services) { }
}

Extensions target the generic base and return TBuilder.

public static class OpenFeatureBuilderExtensions
{
    public static TBuilder AddProvider<TBuilder>(
        this OpenFeatureProviderBuilder<TBuilder> builder,
        Func<IServiceProvider, IFeatureProvider> factory)
        where TBuilder : OpenFeatureProviderBuilder<TBuilder>
    {
        // registration logic
        return (TBuilder)builder;
    }
}

Outcome

  • Fluent API is order independent. both AddProvider and AddHook can be called in any order.
  • No breaking changes for consumers. they still see the same OpenFeatureProviderBuilder and OpenFeatureBuilder types.
  • From the caller side the API is the same. the type system just preserves the correct builder type under the hood.

Pros

  • Fixes the chaining issue without changing the external API.
  • Works for both OpenFeatureProviderBuilder and OpenFeatureBuilder uniformly.
  • Gives us a cleaner foundation if we add more builders in the future.

Cons

  • Slightly more complex generic constraints for contributors to understand. especially around TBuilder and where TBuilder : OpenFeatureProviderBuilder<TBuilder>.
  • Providers and other extension authors now work with a more complex builder type.
  • We must clearly highlight in the docs that extension methods must return TBuilder to avoid breaking the fluent API. if an extension accidentally returns the base type instead of TBuilder, chaining can degrade.

arttonoyan avatar Nov 21 '25 07:11 arttonoyan