-
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?
Changes from all commits
2c42d1f
4d69c6f
7e26ba1
1edb0c9
6cfaeb7
dbdb9b5
acade51
10203f3
3d680f3
f9a6781
4b1237a
04ea063
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file misses the README file necessary for the package documentation. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| <Project Sdk="Microsoft.NET.Sdk"> | ||
|
|
||
| <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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kylejuliandev Great question - thanks for calling it out. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Proposal Follow-up idea Why this helps
cc: @beeme1mr There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
| </PropertyGroup> | ||
|
|
||
| <ItemGroup> | ||
| <PackageReference Include="Microsoft.Extensions.DependencyInjection.Abstractions" /> | ||
| <PackageReference Include="Microsoft.Extensions.Options" /> | ||
| </ItemGroup> | ||
|
|
||
| <ItemGroup> | ||
| <InternalsVisibleTo Include="OpenFeature.Hosting.Tests" /> | ||
| <InternalsVisibleTo Include="DynamicProxyGenAssembly2" /> | ||
| </ItemGroup> | ||
|
|
||
| <ItemGroup> | ||
| <ProjectReference Include="..\OpenFeature\OpenFeature.csproj" /> | ||
| </ItemGroup> | ||
|
|
||
| </Project> | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| using Microsoft.Extensions.DependencyInjection; | ||
|
|
||
| namespace OpenFeature.DependencyInjection.Abstractions; | ||
|
|
||
| /// <summary> | ||
| /// Describes a <see cref="OpenFeatureProviderBuilder"/> backed by an <see cref="IServiceCollection"/>. | ||
| /// </summary> | ||
| public abstract class OpenFeatureProviderBuilder(IServiceCollection services) | ||
kylejuliandev marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| { | ||
| /// <summary> The services being configured. </summary> | ||
| public IServiceCollection Services { get; } = services; | ||
|
|
||
| /// <summary> | ||
| /// Gets a value indicating whether a default provider has been registered. | ||
| /// </summary> | ||
| public bool HasDefaultProvider { get; internal set; } | ||
|
|
||
| /// <summary> | ||
| /// Gets the count of domain-bound providers that have been registered. | ||
| /// This count does not include the default provider. | ||
| /// </summary> | ||
| public int DomainBoundProviderRegistrationCount { get; internal set; } | ||
|
|
||
| /// <summary> | ||
| /// Indicates whether the policy has been configured. | ||
| /// </summary> | ||
| public bool IsPolicyConfigured { get; internal set; } | ||
|
|
||
| /// <summary> | ||
| /// Validates the current configuration, ensuring that a policy is set when multiple providers are registered | ||
| /// or when a default provider is registered alongside another provider. | ||
| /// </summary> | ||
| /// <exception cref="InvalidOperationException"> | ||
| /// Thrown if multiple providers are registered without a policy, or if both a default provider | ||
| /// and an additional provider are registered without a policy configuration. | ||
| /// </exception> | ||
| public void Validate() | ||
| { | ||
| if (IsPolicyConfigured) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| if (DomainBoundProviderRegistrationCount > 1) | ||
| { | ||
| throw new InvalidOperationException("Multiple providers have been registered, but no policy has been configured."); | ||
| } | ||
|
|
||
| if (HasDefaultProvider && DomainBoundProviderRegistrationCount == 1) | ||
| { | ||
| throw new InvalidOperationException("A default provider and an additional provider have been registered without a policy configuration."); | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Adds an IFeatureClient to the container. If <paramref name="name"/> is supplied, | ||
| /// registers a domain-bound client; otherwise registers a global client. If an evaluation context is | ||
| /// configured, it is applied at resolve-time. | ||
| /// </summary> | ||
| /// <returns>The current <see cref="OpenFeatureProviderBuilder"/>.</returns> | ||
| internal protected abstract OpenFeatureProviderBuilder TryAddClient(string? name = null); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,145 @@ | ||
| using Microsoft.Extensions.DependencyInjection; | ||
| using Microsoft.Extensions.DependencyInjection.Extensions; | ||
| using OpenFeature.DependencyInjection.Abstractions; | ||
|
|
||
| namespace OpenFeature; | ||
askpt marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| /// <summary> | ||
| /// Contains extension methods for the <see cref="OpenFeatureProviderBuilder"/> class. | ||
| /// </summary> | ||
| #if NET8_0_OR_GREATER | ||
| [System.Diagnostics.CodeAnalysis.Experimental(DependencyInjection.Abstractions.Diagnostics.FeatureCodes.NewDi)] | ||
| #endif | ||
| public static partial class OpenFeatureProviderBuilderExtensions | ||
| { | ||
| /// <summary> | ||
| /// Adds a feature provider using a factory method without additional configuration options. | ||
| /// This method adds the feature provider as a transient service and sets it as the default provider within the application. | ||
| /// </summary> | ||
| /// <param name="builder">The <see cref="OpenFeatureProviderBuilder"/> used to configure feature flags.</param> | ||
| /// <param name="implementationFactory"> | ||
| /// A factory method that creates and returns a <see cref="FeatureProvider"/> | ||
| /// instance based on the provided service provider. | ||
| /// </param> | ||
| /// <returns>The updated <see cref="OpenFeatureProviderBuilder"/> instance with the default feature provider set and configured.</returns> | ||
| /// <exception cref="ArgumentNullException">Thrown if the <paramref name="builder"/> is null, as a valid builder is required to add and configure providers.</exception> | ||
| public static OpenFeatureProviderBuilder AddProvider(this OpenFeatureProviderBuilder builder, Func<IServiceProvider, FeatureProvider> implementationFactory) | ||
| => AddProvider<OpenFeatureProviderOptions>(builder, implementationFactory, null); | ||
|
|
||
| /// <summary> | ||
| /// Adds a feature provider using a factory method to create the provider instance and optionally configures its settings. | ||
| /// This method adds the feature provider as a transient service and sets it as the default provider within the application. | ||
| /// </summary> | ||
| /// <typeparam name="TOptions"> Type derived from <see cref="OpenFeatureProviderBuilder"/> used to configure the feature provider.</typeparam> | ||
| /// <param name="builder">The <see cref="OpenFeatureProviderBuilder"/> used to configure feature flags.</param> | ||
| /// <param name="implementationFactory"> | ||
| /// A factory method that creates and returns a <see cref="FeatureProvider"/> | ||
| /// instance based on the provided service provider. | ||
| /// </param> | ||
| /// <param name="configureOptions">An optional delegate to configure the provider-specific options.</param> | ||
| /// <returns>The updated <see cref="OpenFeatureProviderBuilder"/> instance with the default feature provider set and configured.</returns> | ||
| /// <exception cref="ArgumentNullException">Thrown if the <paramref name="builder"/> is null, as a valid builder is required to add and configure providers.</exception> | ||
| public static OpenFeatureProviderBuilder AddProvider<TOptions>(this OpenFeatureProviderBuilder builder, Func<IServiceProvider, FeatureProvider> implementationFactory, Action<TOptions>? configureOptions) | ||
| where TOptions : OpenFeatureProviderOptions | ||
| { | ||
| if (builder == null) throw new ArgumentNullException(nameof(builder)); | ||
|
|
||
| builder.HasDefaultProvider = true; | ||
| builder.Services.PostConfigure<TOptions>(options => options.AddDefaultProviderName()); | ||
| if (configureOptions != null) | ||
| { | ||
| builder.Services.Configure(configureOptions); | ||
| } | ||
|
|
||
| builder.Services.TryAddTransient(implementationFactory); | ||
| builder.TryAddClient(); | ||
| return builder; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Adds a feature provider for a specific domain using provided options and a configuration builder. | ||
| /// </summary> | ||
| /// <typeparam name="TOptions"> Type derived from <see cref="OpenFeatureProviderBuilder"/> used to configure the feature provider.</typeparam> | ||
| /// <param name="builder">The <see cref="OpenFeatureProviderBuilder"/> used to configure feature flags.</param> | ||
| /// <param name="domain">The unique name of the provider.</param> | ||
| /// <param name="implementationFactory"> | ||
| /// A factory method that creates a feature provider instance. | ||
| /// It adds the provider as a transient service unless it is already added. | ||
| /// </param> | ||
| /// <param name="configureOptions">An optional delegate to configure the provider-specific options.</param> | ||
| /// <returns>The updated <see cref="OpenFeatureProviderBuilder"/> instance with the new feature provider configured.</returns> | ||
| /// <exception cref="ArgumentNullException"> | ||
| /// Thrown if either <paramref name="builder"/> or <paramref name="domain"/> is null or if the <paramref name="domain"/> is empty. | ||
| /// </exception> | ||
| public static OpenFeatureProviderBuilder AddProvider<TOptions>(this OpenFeatureProviderBuilder builder, string domain, Func<IServiceProvider, string, FeatureProvider> implementationFactory, Action<TOptions>? configureOptions) | ||
| where TOptions : OpenFeatureProviderOptions | ||
| { | ||
| if (builder == null) throw new ArgumentNullException(nameof(builder)); | ||
|
|
||
| builder.DomainBoundProviderRegistrationCount++; | ||
|
|
||
| builder.Services.PostConfigure<TOptions>(options => options.AddProviderName(domain)); | ||
| if (configureOptions != null) | ||
| { | ||
| builder.Services.Configure(domain, configureOptions); | ||
| } | ||
|
|
||
| builder.Services.TryAddKeyedTransient(domain, (provider, key) => | ||
| { | ||
| if (key == null) | ||
| { | ||
| throw new ArgumentNullException(nameof(key)); | ||
| } | ||
| return implementationFactory(provider, key.ToString()!); | ||
| }); | ||
|
|
||
| builder.TryAddClient(domain); | ||
| return builder; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Adds a feature provider for a specified domain using the default options. | ||
| /// This method configures a feature provider without custom options, delegating to the more generic AddProvider method. | ||
| /// </summary> | ||
| /// <param name="builder">The <see cref="OpenFeatureProviderBuilder"/> used to configure feature flags.</param> | ||
| /// <param name="domain">The unique name of the provider.</param> | ||
| /// <param name="implementationFactory"> | ||
| /// A factory method that creates a feature provider instance. | ||
| /// It adds the provider as a transient service unless it is already added. | ||
| /// </param> | ||
| /// <returns>The updated <see cref="OpenFeatureProviderBuilder"/> instance with the new feature provider configured.</returns> | ||
| /// <exception cref="ArgumentNullException"> | ||
| /// Thrown if either <paramref name="builder"/> or <paramref name="domain"/> is null or if the <paramref name="domain"/> is empty. | ||
| /// </exception> | ||
| public static OpenFeatureProviderBuilder AddProvider(this OpenFeatureProviderBuilder builder, string domain, Func<IServiceProvider, string, FeatureProvider> implementationFactory) | ||
| => AddProvider<OpenFeatureProviderOptions>(builder, domain, implementationFactory, configureOptions: null); | ||
|
|
||
| /// <summary> | ||
| /// Configures policy name options for OpenFeature using the specified options type. | ||
| /// </summary> | ||
| /// <typeparam name="TOptions">The type of options used to configure <see cref="OpenFeatureProviderOptions"/>.</typeparam> | ||
| /// <param name="builder">The <see cref="OpenFeatureProviderBuilder"/> instance.</param> | ||
| /// <param name="configureOptions">A delegate to configure <typeparamref name="TOptions"/>.</param> | ||
| /// <returns>The configured <see cref="OpenFeatureProviderBuilder"/> instance.</returns> | ||
| /// <exception cref="ArgumentNullException">Thrown when the <paramref name="builder"/> or <paramref name="configureOptions"/> is null.</exception> | ||
| public static OpenFeatureProviderBuilder AddPolicyName<TOptions>(this OpenFeatureProviderBuilder builder, Action<TOptions> configureOptions) | ||
| where TOptions : PolicyNameOptions | ||
| { | ||
| if (builder == null) throw new ArgumentNullException(nameof(builder)); | ||
| if (configureOptions == null) throw new ArgumentNullException(nameof(configureOptions)); | ||
|
|
||
| builder.IsPolicyConfigured = true; | ||
|
|
||
| builder.Services.Configure(configureOptions); | ||
| return builder; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Configures the default policy name options for OpenFeature. | ||
| /// </summary> | ||
| /// <param name="builder">The <see cref="OpenFeatureProviderBuilder"/> instance.</param> | ||
| /// <param name="configureOptions">A delegate to configure <see cref="OpenFeatureProviderBuilder"/>.</param> | ||
| /// <returns>The configured <see cref="OpenFeatureProviderBuilder"/> instance.</returns> | ||
| public static OpenFeatureProviderBuilder AddPolicyName(this OpenFeatureProviderBuilder builder, Action<PolicyNameOptions> configureOptions) | ||
| => AddPolicyName<PolicyNameOptions>(builder, configureOptions); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| using System.Collections.ObjectModel; | ||
|
|
||
| namespace OpenFeature.DependencyInjection.Abstractions; | ||
|
|
||
| /// <summary> | ||
| /// Provider-focused options for configuring OpenFeature integrations. | ||
| /// Contains only contracts and metadata that integrations may need. | ||
| /// </summary> | ||
| public class OpenFeatureProviderOptions | ||
kylejuliandev marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| { | ||
| private readonly HashSet<string> _providerNames = []; | ||
|
|
||
| /// <summary> | ||
| /// Determines if a default provider has been registered. | ||
| /// </summary> | ||
| public bool HasDefaultProvider { get; private set; } | ||
|
|
||
| /// <summary> | ||
| /// The <see cref="Type"/> of the configured feature provider, if any. | ||
| /// Typically set by higher-level configuration. | ||
| /// </summary> | ||
| public Type FeatureProviderType { get; protected internal set; } = null!; | ||
|
|
||
| /// <summary> | ||
| /// Gets a read-only list of registered provider names. | ||
| /// </summary> | ||
| public IReadOnlyCollection<string> ProviderNames | ||
| { | ||
| get | ||
| { | ||
| lock (_providerNames) | ||
| { | ||
| return new ReadOnlyCollection<string>([.. _providerNames]); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Registers the default provider name if no specific name is provided. | ||
| /// Sets <see cref="HasDefaultProvider"/> to true. | ||
| /// </summary> | ||
| internal void AddDefaultProviderName() => AddProviderName(null); | ||
|
|
||
| /// <summary> | ||
| /// Registers a new feature provider name. This operation is thread-safe. | ||
| /// </summary> | ||
| /// <param name="name">The name of the feature provider to register. Registers as default if null.</param> | ||
| internal void AddProviderName(string? name) | ||
| { | ||
| if (string.IsNullOrWhiteSpace(name)) | ||
| { | ||
| HasDefaultProvider = true; | ||
| return; | ||
| } | ||
|
|
||
| lock (_providerNames) | ||
| { | ||
| _providerNames.Add(name!); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| using Microsoft.Extensions.DependencyInjection; | ||
| using Microsoft.Extensions.Logging; | ||
| using Microsoft.Extensions.Options; | ||
| using OpenFeature.DependencyInjection.Abstractions; | ||
|
|
||
| namespace OpenFeature.Hosting.Internal; | ||
|
|
||
|
|
@@ -22,7 +23,14 @@ public async ValueTask EnsureInitializedAsync(CancellationToken cancellationToke | |
| { | ||
| this.LogStartingInitializationOfFeatureProvider(); | ||
|
|
||
| var options = _serviceProvider.GetRequiredService<IOptions<OpenFeatureOptions>>().Value; | ||
| await InitializeProvidersAsync(cancellationToken).ConfigureAwait(false); | ||
| InitializeHooks(); | ||
| InitializeHandlers(); | ||
| } | ||
|
|
||
| private async Task InitializeProvidersAsync(CancellationToken cancellationToken) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cancellationToken seems to not be used here. |
||
| { | ||
| var options = _serviceProvider.GetRequiredService<IOptions<OpenFeatureProviderOptions>>().Value; | ||
| if (options.HasDefaultProvider) | ||
| { | ||
| var featureProvider = _serviceProvider.GetRequiredService<FeatureProvider>(); | ||
|
|
@@ -34,7 +42,11 @@ public async ValueTask EnsureInitializedAsync(CancellationToken cancellationToke | |
| var featureProvider = _serviceProvider.GetRequiredKeyedService<FeatureProvider>(name); | ||
| await _featureApi.SetProviderAsync(name, featureProvider).ConfigureAwait(false); | ||
| } | ||
| } | ||
|
|
||
| private void InitializeHooks() | ||
| { | ||
| var options = _serviceProvider.GetRequiredService<IOptions<OpenFeatureOptions>>().Value; | ||
| var hooks = new List<Hook>(); | ||
| foreach (var hookName in options.HookNames) | ||
| { | ||
|
|
@@ -43,7 +55,10 @@ public async ValueTask EnsureInitializedAsync(CancellationToken cancellationToke | |
| } | ||
|
|
||
| _featureApi.AddHooks(hooks); | ||
| } | ||
|
|
||
| private void InitializeHandlers() | ||
| { | ||
| var handlers = _serviceProvider.GetServices<EventHandlerDelegateWrapper>(); | ||
| foreach (var handler in handlers) | ||
| { | ||
|
|
||
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,
FeatureCodesare 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:
Experimentalattribute,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.DependencyInjectionlibrary and this would be great candidate for 2.10. Feel free to create a new issue and add it to 2.10 milestone 👍