Skip to content

Commit

Permalink
Don't use event MulticastDelegate for OnFeatureConfigurationUpdate si…
Browse files Browse the repository at this point in the history 
…gnal as async exception behavhiour is not fit for purpose
  • Loading branch information
@ickers
ickers committed Jan 16, 2024
1 parent 7394714 commit fa68f9e
Show file tree
Hide file tree
Showing 8 changed files with 136 additions and 63 deletions.
24 changes: 12 additions & 12 deletions libs/dotnet-sdk-test/FeatureBoardClientTests.cs
View file
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
using System.Text.Json.Nodes;
using Bogus;
using FeatureBoard.DotnetSdk.Helpers;
using FeatureBoard.DotnetSdk.Models;
using FeatureBoard.DotnetSdk.State;
using FeatureBoard.DotnetSdk.Test.Extensions;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Logging.Abstractions;
using Bogus;
using Shouldly;
using FeatureBoard.DotnetSdk.Helpers;
using FeatureBoard.DotnetSdk.Models;
using FeatureBoard.DotnetSdk.State;

namespace FeatureBoard.DotnetSdk.Test;

Expand All @@ -30,7 +30,7 @@ public FeatureBoardClientTests()
[Fact]
public void ItReturnsTheDefaultValueWhenNoValueIsFound()
{
Services.AddTransient<FeatureBoardStateSnapshot>(_ => new FeatureBoardStateSnapshot(new Dictionary<string, FeatureConfiguration>(0)));
Services.AddTransient(_ => new FeatureBoardStateSnapshot(new Dictionary<string, FeatureConfiguration>(0)));

var client = Services.BuildServiceProvider().GetRequiredService<FeatureBoardClient<TestFeatures>>();

Expand All @@ -45,7 +45,7 @@ public void ItReturnsTheDefaultValueFromFeatureBoardWhenNoAudienceIsFound()
{
var faker = new Faker();
var defaultFeatureValue = faker.Lorem.Sentence();
Services.AddTransient<FeatureBoardStateSnapshot>(_ => new FeatureBoardStateSnapshot(new Dictionary<string, FeatureConfiguration>
Services.AddTransient(_ => new FeatureBoardStateSnapshot(new Dictionary<string, FeatureConfiguration>
{
{
nameof(TestFeatures.StringFeature).ToFeatureBoardKey(), new FeatureConfiguration
Expand All @@ -67,7 +67,7 @@ public void ItReturnsTheAudienceValueFromFeatureBoardWhenAnAudienceIsFound()
{
var faker = new Faker();
var defaultAudienceValue = faker.Lorem.Paragraph();
Services.AddTransient<FeatureBoardStateSnapshot>(_ => new FeatureBoardStateSnapshot(new Dictionary<string, FeatureConfiguration>
Services.AddTransient(_ => new FeatureBoardStateSnapshot(new Dictionary<string, FeatureConfiguration>
{
{
nameof(TestFeatures.StringFeature).ToFeatureBoardKey(), new FeatureConfiguration
Expand Down Expand Up @@ -96,7 +96,7 @@ public void ItReturnsADecimal()
{
var faker = new Faker();
var defaultAudienceValue = faker.Random.Decimal();
Services.AddTransient<FeatureBoardStateSnapshot>(_ => new FeatureBoardStateSnapshot(new Dictionary<string, FeatureConfiguration>
Services.AddTransient(_ => new FeatureBoardStateSnapshot(new Dictionary<string, FeatureConfiguration>
{
{
nameof(TestFeatures.NumberFeature).ToFeatureBoardKey(), new FeatureConfiguration
Expand Down Expand Up @@ -126,7 +126,7 @@ public void ItReturnsABool()
{
var faker = new Faker();
var defaultAudienceValue = faker.Random.Bool();
Services.AddTransient<FeatureBoardStateSnapshot>(_ => new FeatureBoardStateSnapshot(new Dictionary<string, FeatureConfiguration> {
Services.AddTransient(_ => new FeatureBoardStateSnapshot(new Dictionary<string, FeatureConfiguration> {
{
nameof(TestFeatures.BoolFeature).ToFeatureBoardKey(), new FeatureConfiguration
{
Expand Down Expand Up @@ -154,7 +154,7 @@ public void ItReturnsAString()
{
var faker = new Faker();
var defaultAudienceValue = Guid.NewGuid().ToString();
Services.AddTransient<FeatureBoardStateSnapshot>(_ => new FeatureBoardStateSnapshot(new Dictionary<string, FeatureConfiguration>
Services.AddTransient(_ => new FeatureBoardStateSnapshot(new Dictionary<string, FeatureConfiguration>
{
{
nameof(TestFeatures.StringFeature).ToFeatureBoardKey(), new FeatureConfiguration
Expand Down Expand Up @@ -183,7 +183,7 @@ public void ItReturnsAnEnum()
{
var faker = new Faker();
var defaultAudienceValue = faker.PickRandom<TestEnum>();
Services.AddTransient<FeatureBoardStateSnapshot>(_ => new FeatureBoardStateSnapshot(new Dictionary<string, FeatureConfiguration> {
Services.AddTransient(_ => new FeatureBoardStateSnapshot(new Dictionary<string, FeatureConfiguration> {
{
nameof(TestFeatures.EnumFeature).ToFeatureBoardKey(), new FeatureConfiguration
{
Expand Down Expand Up @@ -212,7 +212,7 @@ public void ItLooksUpTheFeatureKeyWithFeatureKeyNameAttribute()
{
var faker = new Faker();
var defaultAudienceValue = Guid.NewGuid().ToString();
Services.AddTransient<FeatureBoardStateSnapshot>(_ => new FeatureBoardStateSnapshot(new Dictionary<string, FeatureConfiguration> {
Services.AddTransient(_ => new FeatureBoardStateSnapshot(new Dictionary<string, FeatureConfiguration> {
{
"a-strange-name", new FeatureConfiguration
{
Expand Down
83 changes: 58 additions & 25 deletions libs/dotnet-sdk-test/FeatureBoardHttpClientTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Text.Json.Nodes;
using Bogus;
using FeatureBoard.DotnetSdk.Models;
using FeatureBoard.DotnetSdk.State;
using Microsoft.Extensions.Logging.Abstractions;
using Moq;

Expand All @@ -17,14 +18,15 @@ public class FeatureBoardHttpClientTests

private static readonly Expression<Func<HttpRequestMessage, bool>> _defaultRequestMatcher = msg => msg.Method == HttpMethod.Get && msg.RequestUri!.OriginalString == FeatureBoardHttpClient.Action;

private readonly Mock<HttpClient> _mockHttpClient = new Mock<HttpClient>();
private readonly Mock<HttpClient> _mockHttpClient = new();
private readonly Mock<FeatureBoardStateUpdater> _mockStateUpdater = new(null);

private readonly FeatureConfiguration testFeatureConfig = CreateFeature();
private readonly FeatureConfiguration _testFeatureConfig = CreateFeature();


public FeatureBoardHttpClientTests()
{
var content = JsonContent.Create(new[] { testFeatureConfig });
var content = JsonContent.Create(new[] { _testFeatureConfig });
_mockHttpClient
.Setup(client => client.SendAsync(It.Is<HttpRequestMessage>(_defaultRequestMatcher), It.IsAny<CancellationToken>()))
.ReturnsAsync((HttpRequestMessage request, CancellationToken _) =>
Expand All @@ -34,20 +36,21 @@ public FeatureBoardHttpClientTests()
return response;
});

_mockStateUpdater
.Setup(m => m.UpdateState(It.IsAny<IReadOnlyCollection<FeatureConfiguration>>(), It.IsAny<CancellationToken>()))
.Verifiable();
}

[Fact]
public async Task ItReturnsCorrectListOfFeatures()
{
// Arrange
IReadOnlyCollection<FeatureConfiguration>? actionArg = null;
Task captureArgAction(IReadOnlyCollection<FeatureConfiguration> features, CancellationToken token)
{
actionArg = features;
return Task.CompletedTask;
}

var testSubject = new FeatureBoardHttpClient(_mockHttpClient.Object, () => ref _nullETag, new FeatureConfigurationUpdated[] { captureArgAction }, new NullLogger<FeatureBoardHttpClient>());
_mockStateUpdater.Setup(m => m.UpdateState(It.IsAny<IReadOnlyCollection<FeatureConfiguration>>(), It.IsAny<CancellationToken>()))
.Callback((IReadOnlyCollection<FeatureConfiguration> features, CancellationToken token) => actionArg = features);

var testSubject = new FeatureBoardHttpClient(_mockHttpClient.Object, () => ref _nullETag, new[] { _mockStateUpdater.Object }, new NullLogger<FeatureBoardHttpClient>());

// Act
var result = await testSubject.RefreshFeatureConfiguration(CancellationToken.None);
Expand All @@ -58,18 +61,18 @@ Task captureArgAction(IReadOnlyCollection<FeatureConfiguration> features, Cancel
// DEBT: Refactor FeatureConfiguration to remove use of JsonValue and override Equals so get *full* ValueType equality semantics hence can just replace below with use of Assert.Equals()
Assert.Collection(actionArg, item =>
{
Assert.Equal(testFeatureConfig.FeatureKey, item.FeatureKey);
Assert.Equal(testFeatureConfig.DefaultValue.GetValue<string>(), item.DefaultValue.GetValue<string>());
Assert.Equal(_testFeatureConfig.FeatureKey, item.FeatureKey);
Assert.Equal(_testFeatureConfig.DefaultValue.GetValue<string>(), item.DefaultValue.GetValue<string>());
Assert.Collection(item.AudienceExceptions,
ex =>
{
var expected = testFeatureConfig.AudienceExceptions[0];
var expected = _testFeatureConfig.AudienceExceptions[0];
Assert.Equal(expected.AudienceKey, ex.AudienceKey);
Assert.Equal(expected.Value.GetValue<string>(), ex.Value.GetValue<string>());
},
ex =>
{
var expected = testFeatureConfig.AudienceExceptions[1];
var expected = _testFeatureConfig.AudienceExceptions[1];
Assert.Equal(expected.AudienceKey, ex.AudienceKey);
Assert.Equal(expected.Value.GetValue<string>(), ex.Value.GetValue<string>());
}
Expand All @@ -81,15 +84,13 @@ Task captureArgAction(IReadOnlyCollection<FeatureConfiguration> features, Cancel
public async Task ItDoesNotProcessResponseIfNotModified()
{
// Arrange
static Task nopAction(IReadOnlyCollection<FeatureConfiguration> features, CancellationToken token) => Task.CompletedTask;

Expression<Func<HttpRequestMessage, bool>> hasEtagMatcher = msg => _defaultRequestMatcher.Compile()(msg) && msg.Headers.IfNoneMatch.Any(t => t.Equals(new EntityTagHeaderValue(TestETag)));

_mockHttpClient
.Setup(client => client.SendAsync(It.Is<HttpRequestMessage>(hasEtagMatcher), It.IsAny<CancellationToken>()))
.ReturnsAsync(new HttpResponseMessage() { StatusCode = HttpStatusCode.NotModified });

var testSubject = new FeatureBoardHttpClient(_mockHttpClient.Object, () => ref _nullETag, new FeatureConfigurationUpdated[] { nopAction }, new NullLogger<FeatureBoardHttpClient>());
var testSubject = new FeatureBoardHttpClient(_mockHttpClient.Object, () => ref _nullETag, new[] { _mockStateUpdater.Object }, new NullLogger<FeatureBoardHttpClient>());

// Act
var initialResult = await testSubject.RefreshFeatureConfiguration(CancellationToken.None);
Expand All @@ -113,36 +114,68 @@ public async Task ItDoesNotProcessResponseOnNonOkayResponse(HttpStatusCode httpS
_mockHttpClient
.Setup(client => client.SendAsync(It.Is<HttpRequestMessage>(_defaultRequestMatcher), It.IsAny<CancellationToken>()))
.ReturnsAsync((HttpRequestMessage request, CancellationToken _) => new HttpResponseMessage(httpStatusCode) { RequestMessage = request });
var testSubject = new FeatureBoardHttpClient(_mockHttpClient.Object, () => ref _nullETag, Array.Empty<FeatureConfigurationUpdated>(), new NullLogger<FeatureBoardHttpClient>());
var testSubject = new FeatureBoardHttpClient(_mockHttpClient.Object, () => ref _nullETag, Array.Empty<IFeatureBoardStateUpdateHandler>(), new NullLogger<FeatureBoardHttpClient>());

// Act
var result = await testSubject.RefreshFeatureConfiguration(CancellationToken.None);

// Assert
Assert.Null(result);
}


public static object[][] ExternalStateUpdateExceptions => new[]
{
new [] { new Exception() },
};

[Theory]
[MemberData(nameof(ExternalStateUpdateExceptions))]
public async Task ItDoesNotAllowExternalStateUpdateHandlerExceptionToBubble(Exception exception)
{
// Arrange
var exceptioningUpdater = new Mock<IFeatureBoardStateUpdateHandler>();
exceptioningUpdater.Setup(m => m.UpdateState(It.Is((IReadOnlyCollection<FeatureConfiguration> features) => features.Any(config => config.FeatureKey == _testFeatureConfig.FeatureKey)), It.IsAny<CancellationToken>()))
.ThrowsAsync(exception);

var testSubject = new FeatureBoardHttpClient(_mockHttpClient.Object, () => ref _nullETag, new[] { exceptioningUpdater.Object, _mockStateUpdater.Object }, new NullLogger<FeatureBoardHttpClient>());

// Act
var result = await testSubject.RefreshFeatureConfiguration(CancellationToken.None);

// Assert
Assert.Null(result);
Assert.NotNull(_nullETag);
_mockStateUpdater.Verify(); // Verify StateUpdater was called, even though exceptioningUpdater appears 1st in the list of update handlers
}


public static object[][] HandlerExceptions => new[]
public static object[][] FeatureBoardStateUpdaterExceptions => new[]
{
new [] { new ArgumentException() }, // eg. what would happen if duplicate feature keys are returned
new Exception[] { new ArgumentException() },
new Exception[] { new TaskCanceledException() }
};

[Theory]
[MemberData(nameof(HandlerExceptions))]
public async Task ItDoesNotAllowUpdateHandlerExceptionToBubble(Exception exception)
[MemberData(nameof(FeatureBoardStateUpdaterExceptions))]
public async Task ItDoesNotAllowFeatureBoardUpdateHandlerExceptionToBubbleAndDoesNotUpdateETag(Exception exception)
{
// Arrange
static Task nopAction(IReadOnlyCollection<FeatureConfiguration> features, CancellationToken token) => Task.CompletedTask;
Task exceptionAction(IReadOnlyCollection<FeatureConfiguration> features, CancellationToken token) => throw exception;
var otherHandler = new Mock<IFeatureBoardStateUpdateHandler>();
otherHandler.Setup(m => m.UpdateState(It.IsAny<IReadOnlyCollection<FeatureConfiguration>>(), It.IsAny<CancellationToken>()));

_mockStateUpdater.Setup(m => m.UpdateState(It.Is((IReadOnlyCollection<FeatureConfiguration> features) => features.Any(config => config.FeatureKey == _testFeatureConfig.FeatureKey)), It.IsAny<CancellationToken>()))
.ThrowsAsync(exception); // eg. what would happen if duplicate feature keys are returned

var testSubject = new FeatureBoardHttpClient(_mockHttpClient.Object, () => ref _nullETag, new FeatureConfigurationUpdated[] { nopAction, exceptionAction }, new NullLogger<FeatureBoardHttpClient>());
var testSubject = new FeatureBoardHttpClient(_mockHttpClient.Object, () => ref _nullETag, new[] { otherHandler.Object, _mockStateUpdater.Object, }, new NullLogger<FeatureBoardHttpClient>());

// Act
var result = await testSubject.RefreshFeatureConfiguration(CancellationToken.None);

// Assert
Assert.Null(result);
Assert.Null(_nullETag);
Assert.Empty(otherHandler.Invocations); // Verify "other" handler is never even called after our FeatureBoardStateUpdater exceptioned
}


Expand All @@ -154,7 +187,7 @@ public async Task ItDoesNotAllowTransientNetworkRequestErrorsToBubble()
.Setup(client => client.SendAsync(It.Is<HttpRequestMessage>(_defaultRequestMatcher), It.IsAny<CancellationToken>()))
.ThrowsAsync(new HttpRequestException());

var testSubject = new FeatureBoardHttpClient(_mockHttpClient.Object, () => ref _nullETag, Array.Empty<FeatureConfigurationUpdated>(), new NullLogger<FeatureBoardHttpClient>());
var testSubject = new FeatureBoardHttpClient(_mockHttpClient.Object, () => ref _nullETag, Array.Empty<IFeatureBoardStateUpdateHandler>(), new NullLogger<FeatureBoardHttpClient>());

// Act
var result = await testSubject.RefreshFeatureConfiguration(CancellationToken.None);
Expand Down
33 changes: 20 additions & 13 deletions libs/dotnet-sdk/FeatureBoardHttpClient.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,25 +8,24 @@ namespace FeatureBoard.DotnetSdk;

public delegate ref EntityTagHeaderValue? LastETagProvider();

public delegate Task FeatureConfigurationUpdated(IReadOnlyCollection<FeatureConfiguration> configuration, CancellationToken cancellationToken);

internal sealed class FeatureBoardHttpClient : IFeatureBoardHttpClient
{
private readonly IOrderedEnumerable<State.IFeatureBoardStateUpdateHandler> _featureConfigurationUpdatedHandlers;

internal static readonly string Action = "all";

private LastETagProvider _eTag;

private readonly HttpClient _httpClient;
private event FeatureConfigurationUpdated OnFeatureConfigurationUpdated = null!;
private readonly ILogger _logger;


public FeatureBoardHttpClient(HttpClient httpClient, LastETagProvider lastModifiedTimeProvider, IEnumerable<FeatureConfigurationUpdated> updateHandlers, ILogger<FeatureBoardHttpClient> logger)
public FeatureBoardHttpClient(HttpClient httpClient, LastETagProvider lastModifiedTimeProvider, IEnumerable<State.IFeatureBoardStateUpdateHandler> updateHandlers, ILogger<FeatureBoardHttpClient> logger)
{
_httpClient = httpClient;
foreach (var handler in updateHandlers)
OnFeatureConfigurationUpdated += handler;
_logger = logger;
_eTag = lastModifiedTimeProvider;
_logger = logger;
_featureConfigurationUpdatedHandlers = updateHandlers.OrderByDescending(h => h is State.FeatureBoardStateUpdater); // register state update handlers, ensuring "ours" is always first
}

public async Task<bool?> RefreshFeatureConfiguration(CancellationToken cancellationToken)
Expand Down Expand Up @@ -55,7 +54,8 @@ public FeatureBoardHttpClient(HttpClient httpClient, LastETagProvider lastModifi
features = await response.Content.ReadFromJsonAsync<List<FeatureConfiguration>>(cancellationToken: cancellationToken)
?? throw new ApplicationException("Unable to retrieve decode response content");

updateEtagRef(response.Headers.ETag);
updateEtagRef(response.Headers.ETag ?? eTag);
_logger.LogDebug("Fetching updates done, eTag={eTag}", eTag);
}
catch (HttpRequestException e)
{
Expand All @@ -65,20 +65,27 @@ public FeatureBoardHttpClient(HttpClient httpClient, LastETagProvider lastModifi

try
{
await OnFeatureConfigurationUpdated(features, cancellationToken);
foreach (var handler in _featureConfigurationUpdatedHandlers) // DEBT: find way to use Task.WhenAll but if first task exceptions abort/cancel all others
await handler.UpdateState(features, cancellationToken);
return true;
}
catch (ArgumentException e) // eg. thrown due to duplicate feature key
catch (Exception e) when (e is ArgumentException // eg. thrown due to null or duplicate feature key
|| e is TaskCanceledException)
{
updateEtagRef(eTag); // revert to original eTag since we failed to update featureboard internal state
_logger.LogError(e, "Failed to update flags");
}
catch (Exception e) // all other exceptions just log but assume we at least successfully updated featureboard internal state
{
_logger.LogError(e, "Failed to update flags");
return null;
}

return null;

void updateEtagRef(EntityTagHeaderValue? responseTag) // Sync method to allow use of eTag ref-local variable
{
ref var eTag = ref _eTag();
eTag = responseTag ?? eTag; // if didn't get eTag just retain previous eTag
_logger.LogDebug("Fetching updates done, eTag={eTag}", eTag);
eTag = responseTag;
}
}
}
12 changes: 2 additions & 10 deletions libs/dotnet-sdk/Registration/RegisterFeatureBoard.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,15 +38,7 @@ public static FeatureBoardBuilder AddFeatureBoard<TFeatures>(this IServiceCollec
services.AddSingleton<FeatureBoardState>()
.AddHostedService(static provider => provider.GetRequiredService<FeatureBoardState>())
.AddScoped(static provider => new Lazy<FeatureBoardStateSnapshot>(provider.GetRequiredService<FeatureBoardState>().GetSnapshot))
.AddTransient<FeatureConfigurationUpdated>(static provider =>
{
var service = provider.GetRequiredService<FeatureBoardState>();
return (config, _) =>
{
service.Update(config);
return Task.CompletedTask;
};
});
.AddTransient<IFeatureBoardStateUpdateHandler, FeatureBoardStateUpdater>();

return new FeatureBoardBuilder(services);
}
Expand Down Expand Up @@ -83,7 +75,7 @@ public static FeatureBoardBuilder WithPollingUpdateStrategy(this FeatureBoardBui
public static FeatureBoardBuilder WithExternalState<TStateStore>(this FeatureBoardBuilder builder) where TStateStore : class, IFeatureBoardExternalState
{
builder.Services.AddSingleton<IFeatureBoardExternalState, TStateStore>()
.AddTransient<FeatureConfigurationUpdated>(static provider => provider.GetRequiredService<FeatureBoard.DotnetSdk.State.IFeatureBoardExternalState>().UpdateState);
.AddTransient<IFeatureBoardStateUpdateHandler>(static provider => provider.GetRequiredService<IFeatureBoardExternalState>());

return builder;
}
Expand Down
Loading

0 comments on commit fa68f9e

Please sign in to comment.