-
Notifications
You must be signed in to change notification settings - Fork 23
feat: Extract minimal DI integration to OpenFeature.DependencyInjection.Abstractions #596
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
base: main
Are you sure you want to change the base?
feat: Extract minimal DI integration to OpenFeature.DependencyInjection.Abstractions #596
Conversation
Introduced a new project, `OpenFeature.DependencyInjection.Abstractions`, to the solution. This project supports dependency injection abstractions and targets multiple frameworks (`netstandard2.0`, `net8.0`, `net9.0`, `net462`) for broad compatibility. Configured the project with the `Microsoft.NET.Sdk` SDK and set the root namespace to `OpenFeature.DependencyInjection.Abstractions`. Added dependencies on `Microsoft.Extensions.DependencyInjection.Abstractions` and `Microsoft.Extensions.Options` to enable DI and options configuration.
Refactored the OpenFeature framework to introduce `OpenFeatureProviderBuilder`, enhancing support for dependency injection and provider management. - Changed namespaces to align with DI abstractions. - Made `FeatureCodes` public for broader accessibility. - Added `InternalsVisibleTo` for testing and project references. - Introduced `OpenFeatureProviderBuilder` for managing providers and policies. - Added extension methods for provider and policy registration. - Refactored `OpenFeatureBuilder` to inherit from `OpenFeatureProviderBuilder`. - Consolidated shared functionality in `OpenFeatureProviderOptions`. - Updated `FeatureBuilderExtensions` and `InMemoryProviderOptions` to use the new abstraction. - Updated tests to reflect new method signatures and hierarchy. - Removed redundant methods and properties, improving code organization. These changes improve maintainability, extensibility, and alignment with modern DI patterns.
Simplified the `AddProvider` API by removing the `TFeatureProvider` generic type parameter and directly using the `FeatureProvider` type. Updated the `TOptions` parameter to ensure it derives from `OpenFeatureProviderOptions`. Added a `<ProjectReference>` to `OpenFeature.csproj` in `OpenFeature.DependencyInjection.Abstractions.csproj`. Updated `OpenFeatureOptions` to `OpenFeatureProviderOptions` in the default name selector policy. Refactored `AddInMemoryProvider` methods to align with the new API. Updated tests in `OpenFeatureBuilderExtensionsTests` to reflect the changes and validate the updated functionality. Performed general code cleanup, including XML documentation updates, to improve clarity and maintain consistency across the codebase.
Refactored `FeatureLifecycleManager` to modularize provider, hook, and handler initialization with new methods: `InitializeProvidersAsync`, `InitializeHooks`, and `InitializeHandlers`. Updated `EnsureInitializedAsync` to use these methods for improved readability and maintainability. Revised `AddInMemoryProvider` in `FeatureBuilderExtensions` to use a generic `FeatureProvider` abstraction. Adjusted `CreateProvider` methods accordingly. Improved code clarity in `FeatureFlagIntegrationTest` by renaming variables for consistency and removing redundant assignments.
Updated FeatureLifecycleManagerTests to replace OpenFeatureOptions with OpenFeatureProviderOptions for configuring feature providers. Added support for hooks and keyed singletons to enhance modularity. Introduced additional feature flag retrieval in FeatureFlagIntegrationTest. Added dependency on OpenFeature.DependencyInjection.Abstractions.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #596 +/- ##
==========================================
- Coverage 89.89% 89.81% -0.09%
==========================================
Files 77 80 +3
Lines 3168 3182 +14
Branches 364 369 +5
==========================================
+ Hits 2848 2858 +10
Misses 251 251
- Partials 69 73 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
src/OpenFeature.Providers.DependencyInjection/OpenFeatureProviderOptions.cs
Show resolved
Hide resolved
| @@ -22,7 +22,7 @@ namespace OpenFeature.Hosting.Diagnostics; | |||
| /// - "001" - Unique identifier for a specific feature. | |||
| /// </code> | |||
| /// </example> | |||
| internal static class FeatureCodes | |||
| public static class FeatureCodes | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Do we need these FeatureCodes anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kylejuliandev Answer: Yes - within the scope of this PR, FeatureCodes are still required; the current changes reference them.
Follow-up proposal: It’s a good time to graduate them from Experimental. Not in this PR - I suggest a separate task.
@askpt @beeme1mr what do you think about me opening a follow-up issue to:
- remove the
Experimentalattribute, - update docs/usages,
- add a changelog entry and brief migration note?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think with @kylejuliandev efforts to deprecate the OpenFeature.DependencyInjection library and this would be great candidate for 2.10. Feel free to create a new issue and add it to 2.10 milestone 👍
|
|
||
| <PropertyGroup> | ||
| <TargetFrameworks>netstandard2.0;net8.0;net9.0;net462</TargetFrameworks> | ||
| <RootNamespace>OpenFeature.DependencyInjection.Abstractions</RootNamespace> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Should we have these abstractions in their own namespace? Would it be more accessible to have them in say just the OpenFeature namespace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kylejuliandev Great question - thanks for calling it out.
I’d keep these in their own namespace for now. OpenFeature.DependencyInjection.Abstractions targets provider authors (integrating new providers), not typical app consumers. Keeping it separate avoids cluttering the root OpenFeature namespace and lets us evolve DI-specific types independently.
Of course, I’m open to discussion about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer the @arttonoyan. This is a very specific Abstraction layer.
I am wondering if we should keep in a OpenFeature.Abstractions instead. This would be a "library" of just OpenFeature related abstractions like "hooks", "provider"... I am happy to keep OpenFeature.DependencyInjection.Abstractions if the rest of the team agrees.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Proposal
Rename the package to OpenFeature.Providers.DependencyInjection. This name better reflects scope. It is intuitive for provider authors who want to add DI integration.
Follow-up idea
In a separate discussion, consider extracting common provider contracts into OpenFeature.Providers.Abstractions. We would then have two clear packages. OpenFeature.Providers.Abstractions, and OpenFeature.Providers.DependencyInjection.
Why this helps
- Clear separation. abstractions live in one place, DI glue in another.
- Thinner provider packages. they depend on
Abstractions, optionally on the DI helpers. - Cleaner dependency graph. no DI references inside core contracts.
- Easier discovery on NuGet. consistent
OpenFeature.Providers.*naming.
cc: @beeme1mr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems reasonable to me. What would be the proposed release strategy and impact? I believe the DI package is still marked as experimental, so breaking changes are allowed, but we should try and make it as straightforward for users to migrate. Also, I'd hope that we could remove the experimental badge shortly after making this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now our main pain point is coupling. The current abstractions package depends on the OpenFeature SDK because it includes the OpenFeatureProvider abstraction. This means any SDK release. even when it has no provider changes. forces us to bump the abstractions package. Then every provider sees a new version and feels compelled to upgrade. even though nothing changed for providers. Providers should only react to provider related changes.
Decoupling fixes this. If we move provider contracts into OpenFeature.Providers.Abstractions. we isolate them from SDK churn. The DI helpers stay in OpenFeature.Providers.DependencyInjection. Result. cleaner dependency graph. fewer unnecessary upgrades. thinner provider packages. and clearer ownership of changes.
I realize this is a significant change, and it needs attentive and careful attention. If the direction sounds reasonable, I can open a PR to rename the DI package, then draft a proposal for the abstractions split.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds reasonable to me. We've been looking into something similar (minus the DI part) in Java. FYI @chrfwow @toddbaert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. I’ll proceed and rename the package to OpenFeature.Providers.DependencyInjection in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arttonoyan @beeme1mr @kylejuliandev: I think this is a great discussion.
I would propose for 2.9 to create:
OpenFeature.Providers.DependencyInjection(this PR)
For 2.10:
OpenFeature.Providers.AbstractionsOpenFeature.Hooks.Abstractions
The story that is happening in Providers, is also true for the Hooks.
src/OpenFeature.Providers.DependencyInjection/OpenFeatureProviderBuilder.cs
Show resolved
Hide resolved
| var services = new ServiceCollection(); | ||
| var provider = new NoOpFeatureProvider(); | ||
| services.AddOptions<OpenFeatureOptions>().Configure(options => | ||
| services.AddOptions<OpenFeatureProviderOptions>().Configure(options => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: it should be fine for this to remain OpenFeatureOptions right? OpenFeatureOptions extends OpenFeatureProviderOptions
| options.DefaultNameSelector = provider => | ||
| { | ||
| var options = provider.GetRequiredService<IOptions<OpenFeatureOptions>>().Value; | ||
| var options = provider.GetRequiredService<IOptions<OpenFeatureProviderOptions>>().Value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: it should be fine for this to remain OpenFeatureOptions right? OpenFeatureOptions extends OpenFeatureProviderOptions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kylejuliandev This is a good question. We cannot do that, because when registering a provider we configure OpenFeatureProviderOptions, not OpenFeatureOptions. If we resolve OpenFeatureOptions, the collection of registered providers will be empty.
This is on me. I made OpenFeatureOptions inherit from OpenFeatureProviderOptions, which is confusing, including for me. Your question highlights that design issue and it is totally valid.
I will remove the inheritance and keep these as two independent option types. There is no need for them to be related.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kylejuliandev Fixed: f521321
…hub.com/arttonoyan/dotnet-sdk into feature/di-abstractions-builder-api-587
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! I only added a couple of minor comments.
Please have a chat with @kylejuliandev to try understand the impact on his deprecations efforts. I remember he mentioned a couple of fixes he was working on for the DI and I don't want that effort to get lost during this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file misses the README file necessary for the package documentation.
src/OpenFeature.Providers.DependencyInjection/OpenFeatureProviderBuilderExtensions.cs
Show resolved
Hide resolved
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| private async Task InitializeProvidersAsync(CancellationToken cancellationToken) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cancellationToken seems to not be used here.
| using OpenFeature.DependencyInjection.Abstractions; | ||
| using OpenFeature.Providers.Memory; | ||
|
|
||
| namespace OpenFeature.Hosting.Providers.Memory; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should move to OpenFeature.Providers.Memory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. Moving to OpenFeature.Providers.Memory makes sense. I had the same thought, timing just got in the way. One tweak. I would name it InMemory rather than Memory.
Co-authored-by: André Silva <[email protected]> Signed-off-by: Artyom Tonoyan <[email protected]>
Co-authored-by: André Silva <[email protected]> Signed-off-by: Artyom Tonoyan <[email protected]>
Refactored `OpenFeatureOptions` to `OpenFeatureProviderOptions` to improve clarity and maintainability. Updated all references and test cases to use the new class. Removed inheritance of `OpenFeatureOptions` from `OpenFeatureProviderOptions` and introduced `_hookNames` for managing hook names. Replaced `TestOptions` with `TestProviderOptions` in `OpenFeatureBuilderExtensionsTests`, adding a `SomeFlag` property for more flexible testing. Renamed and updated `OpenFeatureOptionsTests` to `OpenFeatureProviderOptionsTests`. Performed general cleanup, including removing unused namespaces, ensuring consistent naming conventions, and aligning method calls with the new class structure.
src/OpenFeature.Providers.DependencyInjection/OpenFeature.Providers.DependencyInjection.csproj
Outdated
Show resolved
Hide resolved
Migrated `OpenFeature.DependencyInjection.Abstractions` to a new project, `OpenFeature.Providers.DependencyInjection`, to improve modularity and maintainability. Updated namespaces, project references, and `using` directives accordingly. Removed the deprecated `OpenFeature.DependencyInjection.Abstractions` project and its associated files. Introduced `OpenFeature.Providers.DependencyInjection.csproj` to house the migrated functionality. Updated test files to reference the new namespace and adjusted experimental feature annotations. Reorganized build configuration files to align with the new project structure. Ensured documentation and metadata reflect the refactored codebase.
|
@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? |
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
OpenFeatureOptionsinto a new base options type:OpenFeatureProviderOptions.OpenFeatureOptionsnow inherits fromOpenFeatureProviderOptions.OpenFeatureOptionsnow targetsOpenFeatureProviderOptions.Before (internal)
After (internal)
Impact
OpenFeatureOptions) require updates to targetOpenFeatureProviderOptions.How to test
This is a regression-only refactor.
Expectations