Skip to content

feat: Support for name client to given provider #129

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
May 31, 2023
Merged

feat: Support for name client to given provider #129

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 45 additions & 5 deletions src/OpenFeature/Api.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@ namespace OpenFeature
public sealed class Api
{
private EvaluationContext _evaluationContext = EvaluationContext.Empty;
private FeatureProvider _featureProvider = new NoOpFeatureProvider();
private FeatureProvider _defaultProvider = new NoOpFeatureProvider();
private readonly ConcurrentDictionary<string, FeatureProvider> _featureProviders =
new ConcurrentDictionary<string, FeatureProvider>();
private readonly ConcurrentStack<Hook> _hooks = new ConcurrentStack<Hook>();

/// The reader/writer locks are not disposed because the singleton instance should never be disposed.
Expand All @@ -42,22 +44,33 @@ public void SetProvider(FeatureProvider featureProvider)
this._featureProviderLock.EnterWriteLock();
try
{
this._featureProvider = featureProvider;
this._defaultProvider = featureProvider ?? this._defaultProvider;
}
finally
{
this._featureProviderLock.ExitWriteLock();
}
}

/// <summary>
/// Sets the feature provider to given clientName
/// </summary>
/// <param name="clientName">Name of client</param>
/// <param name="featureProvider">Implementation of <see cref="FeatureProvider"/></param>
public void SetProvider(string clientName, FeatureProvider featureProvider)
{
this._featureProviders.AddOrUpdate(clientName, featureProvider,
(key, current) => featureProvider);
}

/// <summary>
/// Gets the feature provider
/// <para>
/// The feature provider may be set from multiple threads, when accessing the global feature provider
/// it should be accessed once for an operation, and then that reference should be used for all dependent
/// operations. For instance, during an evaluation the flag resolution method, and the provider hooks
/// should be accessed from the same reference, not two independent calls to
/// <see cref="GetProvider"/>.
/// <see cref="GetProvider()"/>.
/// </para>
/// </summary>
/// <returns><see cref="FeatureProvider"/></returns>
Expand All @@ -66,25 +79,52 @@ public FeatureProvider GetProvider()
this._featureProviderLock.EnterReadLock();
try
{
return this._featureProvider;
return this._defaultProvider;
}
finally
{
this._featureProviderLock.ExitReadLock();
}
}

/// <summary>
/// Gets the feature provider with given clientName
/// </summary>
/// <param name="clientName">Name of client</param>
/// <returns>A provider associated with the given clientName, if clientName is empty or doesn't
/// have a corresponding provider the default provider will be returned</returns>
public FeatureProvider GetProvider(string clientName)
{
if (string.IsNullOrEmpty(clientName))
{
return this.GetProvider();
}

return this._featureProviders.TryGetValue(clientName, out var featureProvider)
? featureProvider
: this.GetProvider();
}


/// <summary>
/// Gets providers metadata
/// <para>
/// This method is not guaranteed to return the same provider instance that may be used during an evaluation
/// in the case where the provider may be changed from another thread.
/// For multiple dependent provider operations see <see cref="GetProvider"/>.
/// For multiple dependent provider operations see <see cref="GetProvider()"/>.
/// </para>
/// </summary>
/// <returns><see cref="ClientMetadata"/></returns>
public Metadata GetProviderMetadata() => this.GetProvider().GetMetadata();

/// <summary>
/// Gets providers metadata assigned to the given clientName. If the clientName has no provider
/// assigned to it the default provider will be returned
/// </summary>
/// <param name="clientName">Name of client</param>
/// <returns>Metadata assigned to provider</returns>
public Metadata GetProviderMetadata(string clientName) => this.GetProvider(clientName).GetMetadata();

/// <summary>
/// Create a new instance of <see cref="FeatureClient"/> using the current provider
/// </summary>
Expand Down
8 changes: 1 addition & 7 deletions src/OpenFeature/OpenFeatureClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,13 +42,7 @@ public sealed class FeatureClient : IFeatureClient
{
// Alias the provider reference so getting the method and returning the provider are
// guaranteed to be the same object.
var provider = Api.Instance.GetProvider();

if (provider == null)
{
provider = new NoOpFeatureProvider();
this._logger.LogDebug("No provider configured, using no-op provider");
}
var provider = Api.Instance.GetProvider(this._metadata.Name);

return (method(provider), provider);
}
Expand Down
2 changes: 1 addition & 1 deletion test/OpenFeature.Tests/OpenFeatureClientTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ public async Task OpenFeatureClient_Should_Allow_Details_Flag_Evaluation()
}

[Fact]
[Specification("1.1.2", "The `API` MUST provide a function to set the global `provider` singleton, which accepts an API-conformant `provider` implementation.")]
[Specification("1.1.2", "The `API` MUST provide a function to set the default `provider`, which accepts an API-conformant `provider` implementation.")]
[Specification("1.3.3", "The `client` SHOULD guarantee the returned value of any typed flag evaluation method is of the expected type. If the value returned by the underlying provider implementation does not match the expected type, it's to be considered abnormal execution, and the supplied `default value` should be returned.")]
[Specification("1.4.7", "In cases of abnormal execution, the `evaluation details` structure's `error code` field MUST contain an `error code`.")]
[Specification("1.4.8", "In cases of abnormal execution (network failure, unhandled error, etc) the `reason` field in the `evaluation details` SHOULD indicate an error.")]
Expand Down
82 changes: 79 additions & 3 deletions test/OpenFeature.Tests/OpenFeatureTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,65 @@ public void OpenFeature_Should_Be_Singleton()
}

[Fact]
[Specification("1.1.3", "The `API` MUST provide a function to add `hooks` which accepts one or more API-conformant `hooks`, and appends them to the collection of any previously added hooks. When new hooks are added, previously added hooks are not removed.")]
[Specification("1.1.3", "The `API` MUST provide a function to bind a given `provider` to one or more client `name`s. If the client-name already has a bound provider, it is overwritten with the new mapping.")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure about the "or more client names." part of this.

Which implies to me:

var provider = new TestProvider();

openFeature.SetProvider("a", provider);
openFeature.SetProvider("b", provider);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Which will need more consideration with init/shutdown potentially.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have concerns with mapping the same provider instance to 2 names? What's the risk you see here? Maybe we need to spec something additional?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@toddbaert

Basic case.

var provider1 = new TestProvider();

openFeature.SetProvider("a",  provider1);

// Call init on provider1?

openFeature.SetProvider("a",  new TestProvider());

// Call shutdown on provider 1?

// Call init on new provider.

Two Names Scenario 1.

var provider = new TestProvider();

openFeature.SetProvider("a", provider);

// Call init on provider?

openFeature.SetProvider("b", provider);

// Provider already has init called. Do we call it again?

Two Names Scenario 2.

var provider1 = new TestProvider();

openFeature.SetProvider("a", provider1);
openFeature.SetProvider("b", provider1);

openFeature.SetProvider("b",  new TestProvider());

// We don't want to shutdown provider1, because it is still registered to "b".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added this scenario to the tests cases.

This is an interesting thought about the init/shutdown, I haven't yet had a look at the spec for it but I would think it only calls shutdown if there are zero references to the object. I'll create a issue for this and lets continue the discussion under that

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/open-feature/dotnet-sdk/issues/132

public void OpenFeature_Should_Not_Change_Named_Providers_When_Setting_Default_Provider()
{
var openFeature = Api.Instance;

openFeature.SetProvider(new NoOpFeatureProvider());
openFeature.SetProvider(TestProvider.Name, new TestProvider());

var defaultClient = openFeature.GetProviderMetadata();
var namedClient = openFeature.GetProviderMetadata(TestProvider.Name);

defaultClient.Name.Should().Be(NoOpProvider.NoOpProviderName);
namedClient.Name.Should().Be(TestProvider.Name);
}

[Fact]
[Specification("1.1.3", "The `API` MUST provide a function to bind a given `provider` to one or more client `name`s. If the client-name already has a bound provider, it is overwritten with the new mapping.")]
public void OpenFeature_Should_Set_Default_Provide_When_No_Name_Provided()
{
var openFeature = Api.Instance;

openFeature.SetProvider(new TestProvider());

var defaultClient = openFeature.GetProviderMetadata();

defaultClient.Name.Should().Be(TestProvider.Name);
}

[Fact]
[Specification("1.1.3", "The `API` MUST provide a function to bind a given `provider` to one or more client `name`s. If the client-name already has a bound provider, it is overwritten with the new mapping.")]
public void OpenFeature_Should_Assign_Provider_To_Existing_Client()
{
const string name = "new-client";
var openFeature = Api.Instance;

openFeature.SetProvider(name, new TestProvider());
openFeature.SetProvider(name, new NoOpFeatureProvider());

openFeature.GetProviderMetadata(name).Name.Should().Be(NoOpProvider.NoOpProviderName);
}

[Fact]
[Specification("1.1.3", "The `API` MUST provide a function to bind a given `provider` to one or more client `name`s. If the client-name already has a bound provider, it is overwritten with the new mapping.")]
public void OpenFeature_Should_Allow_Multiple_Client_Names_Of_Same_Instance()
{
var openFeature = Api.Instance;
var provider = new TestProvider();

openFeature.SetProvider("a", provider);
openFeature.SetProvider("b", provider);

var clientA = openFeature.GetProvider("a");
var clientB = openFeature.GetProvider("b");

clientA.Should().Be(clientB);
}

[Fact]
[Specification("1.1.4", "The `API` MUST provide a function to add `hooks` which accepts one or more API-conformant `hooks`, and appends them to the collection of any previously added hooks. When new hooks are added, previously added hooks are not removed.")]
public void OpenFeature_Should_Add_Hooks()
{
var openFeature = Api.Instance;
Expand Down Expand Up @@ -50,7 +108,7 @@ public void OpenFeature_Should_Add_Hooks()
}

[Fact]
[Specification("1.1.4", "The API MUST provide a function for retrieving the metadata field of the configured `provider`.")]
[Specification("1.1.5", "The API MUST provide a function for retrieving the metadata field of the configured `provider`.")]
public void OpenFeature_Should_Get_Metadata()
{
Api.Instance.SetProvider(new NoOpFeatureProvider());
Expand All @@ -65,7 +123,7 @@ public void OpenFeature_Should_Get_Metadata()
[InlineData("client1", "version1")]
[InlineData("client2", null)]
[InlineData(null, null)]
[Specification("1.1.5", "The `API` MUST provide a function for creating a `client` which accepts the following options: - name (optional): A logical string identifier for the client.")]
[Specification("1.1.6", "The `API` MUST provide a function for creating a `client` which accepts the following options: - name (optional): A logical string identifier for the client.")]
public void OpenFeature_Should_Create_Client(string name = null, string version = null)
{
var openFeature = Api.Instance;
Expand Down Expand Up @@ -97,5 +155,23 @@ public void Should_Always_Have_Provider()
{
Api.Instance.GetProvider().Should().NotBeNull();
}

[Fact]
public void OpenFeature_Should_Allow_Multiple_Client_Mapping()
{
var openFeature = Api.Instance;

openFeature.SetProvider("client1", new TestProvider());
openFeature.SetProvider("client2", new NoOpFeatureProvider());

var client1 = openFeature.GetClient("client1");
var client2 = openFeature.GetClient("client2");

client1.GetMetadata().Name.Should().Be("client1");
client2.GetMetadata().Name.Should().Be("client2");

client1.GetBooleanValue("test", false).Result.Should().BeTrue();
client2.GetBooleanValue("test", false).Result.Should().BeFalse();
}
}
}
2 changes: 1 addition & 1 deletion test/OpenFeature.Tests/TestImplementations.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ public override Metadata GetMetadata()
public override Task<ResolutionDetails<bool>> ResolveBooleanValue(string flagKey, bool defaultValue,
EvaluationContext context = null)
{
return Task.FromResult(new ResolutionDetails<bool>(flagKey, defaultValue));
return Task.FromResult(new ResolutionDetails<bool>(flagKey, !defaultValue));
}

public override Task<ResolutionDetails<string>> ResolveStringValue(string flagKey, string defaultValue,
Expand Down