Skip to content

Commit 9151dcd

Browse files
kylejuliandevaskpt
andauthored
fix: ArgumentNullException when creating a client with optional name (#508)
<!-- Please use this template for your pull request. --> <!-- Please use the sections that you need and delete other sections --> ## This PR <!-- add the description of the PR here --> - Adds a default placeholder name that gets generated when a Client is generated. This is inspired from the Java SDK (see [EventSource.java](https://github.com/open-feature/java-sdk/blob/0515ad54c4f71863373eb1b7f429393923b27d90/src/main/java/dev/openfeature/sdk/EventSupport.java#L40)). The exception is thrown when an optional client name is specified when retrieving a client from `Api.Instance`. When no name is specified we use a generated one. ### Related Issues <!-- add here the GitHub issue that this PR resolves if applicable --> Fixes #491 ### Notes <!-- any additional notes for this PR --> ### Follow-up Tasks <!-- anything that is related to this PR but not done here should be noted under this section --> <!-- if there is a need for a new issue, please link it here --> ### How to test <!-- if applicable, add testing instructions under this section --> --------- Signed-off-by: Kyle Julian <[email protected]> Co-authored-by: André Silva <[email protected]>
1 parent f923cea commit 9151dcd

File tree

2 files changed

+42
-9
lines changed

2 files changed

+42
-9
lines changed

src/OpenFeature/EventExecutor.cs

Lines changed: 27 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,9 @@ internal sealed partial class EventExecutor : IAsyncDisposable
1414
private readonly Dictionary<string, FeatureProvider> _namedProviderReferences = [];
1515
private readonly List<FeatureProvider> _activeSubscriptions = [];
1616

17+
/// placeholder for anonymous clients
18+
private static Guid _defaultClientName = Guid.NewGuid();
19+
1720
private readonly Dictionary<ProviderEventTypes, List<EventHandlerDelegate>> _apiHandlers = [];
1821
private readonly Dictionary<string, Dictionary<ProviderEventTypes, List<EventHandlerDelegate>>> _clientHandlers = [];
1922

@@ -58,35 +61,39 @@ internal void RemoveApiLevelHandler(ProviderEventTypes type, EventHandlerDelegat
5861

5962
internal void AddClientHandler(string client, ProviderEventTypes eventType, EventHandlerDelegate handler)
6063
{
64+
var clientName = GetClientName(client);
65+
6166
lock (this._lockObj)
6267
{
6368
// check if there is already a list of handlers for the given client and event type
64-
if (!this._clientHandlers.TryGetValue(client, out var registry))
69+
if (!this._clientHandlers.TryGetValue(clientName, out var registry))
6570
{
6671
registry = [];
67-
this._clientHandlers[client] = registry;
72+
this._clientHandlers[clientName] = registry;
6873
}
6974

70-
if (!this._clientHandlers[client].TryGetValue(eventType, out var eventHandlers))
75+
if (!this._clientHandlers[clientName].TryGetValue(eventType, out var eventHandlers))
7176
{
7277
eventHandlers = [];
73-
this._clientHandlers[client][eventType] = eventHandlers;
78+
this._clientHandlers[clientName][eventType] = eventHandlers;
7479
}
7580

76-
this._clientHandlers[client][eventType].Add(handler);
81+
this._clientHandlers[clientName][eventType].Add(handler);
7782

7883
this.EmitOnRegistration(
79-
this._namedProviderReferences.TryGetValue(client, out var clientProviderReference)
84+
this._namedProviderReferences.TryGetValue(clientName, out var clientProviderReference)
8085
? clientProviderReference
8186
: this._defaultProvider, eventType, handler);
8287
}
8388
}
8489

8590
internal void RemoveClientHandler(string client, ProviderEventTypes type, EventHandlerDelegate handler)
8691
{
92+
var clientName = GetClientName(client);
93+
8794
lock (this._lockObj)
8895
{
89-
if (this._clientHandlers.TryGetValue(client, out var clientEventHandlers))
96+
if (this._clientHandlers.TryGetValue(clientName, out var clientEventHandlers))
9097
{
9198
if (clientEventHandlers.TryGetValue(type, out var eventHandlers))
9299
{
@@ -118,15 +125,18 @@ internal void RegisterClientFeatureProvider(string client, FeatureProvider? prov
118125
{
119126
return;
120127
}
128+
129+
var clientName = GetClientName(client);
130+
121131
lock (this._lockObj)
122132
{
123133
FeatureProvider? oldProvider = null;
124-
if (this._namedProviderReferences.TryGetValue(client, out var foundOldProvider))
134+
if (this._namedProviderReferences.TryGetValue(clientName, out var foundOldProvider))
125135
{
126136
oldProvider = foundOldProvider;
127137
}
128138

129-
this._namedProviderReferences[client] = provider;
139+
this._namedProviderReferences[clientName] = provider;
130140

131141
this.StartListeningAndShutdownOld(provider, oldProvider);
132142
}
@@ -303,6 +313,14 @@ private void ProcessDefaultProviderHandlers(Event e)
303313
}
304314
}
305315

316+
private static string GetClientName(string client)
317+
{
318+
if (string.IsNullOrWhiteSpace(client))
319+
{
320+
return _defaultClientName.ToString();
321+
}
322+
return client;
323+
}
306324

307325
// map events to provider status as per spec: https://openfeature.dev/specification/sections/events/#requirement-535
308326
private static void UpdateProviderStatus(FeatureProvider provider, ProviderEventPayload eventPayload)

test/OpenFeature.Tests/OpenFeatureClientTests.cs

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -597,6 +597,21 @@ public async Task PassingABlankStringAsTrackingEventName_ThrowsArgumentException
597597
Assert.Throws<ArgumentException>(() => client.Track(" \n "));
598598
}
599599

600+
[Theory]
601+
[InlineData(null)]
602+
[InlineData("")]
603+
[InlineData(" ")]
604+
public async Task PassingBlankClientName_DoesNotThrowArgumentNullException(string? clientName)
605+
{
606+
var provider = new TestProvider();
607+
await Api.Instance.SetProviderAsync(provider);
608+
var client = Api.Instance.GetClient(clientName);
609+
610+
var ex = Record.Exception(() => client.AddHandler(ProviderEventTypes.ProviderReady, (args) => { }));
611+
612+
Assert.Null(ex);
613+
}
614+
600615
public static TheoryData<string, EvaluationContext?, EvaluationContext?, EvaluationContext?, string> GenerateMergeEvaluationContextTestData()
601616
{
602617
const string key = "key";

0 commit comments

Comments
 (0)