From f475ddcc926d00c5ac2973e2d0d459801af1c9a1 Mon Sep 17 00:00:00 2001 From: Rhys Ickeringill <22509882+ickers@users.noreply.github.com> Date: Thu, 11 Jan 2024 18:00:39 +1100 Subject: [PATCH 1/5] FB-279: Sync FeatureBoard API state back to ExternalState provider if registered --- .../FeatureBoardHttpClientTests.cs | 65 +++++++++++++++---- .../State/FeatureboardStateTests.cs | 38 ++++++++++- libs/dotnet-sdk/FeatureBoard.DotnetSdk.csproj | 1 + libs/dotnet-sdk/FeatureBoardHttpClient.cs | 64 +++++++++++------- .../Registration/RegisterFeatureBoard.cs | 30 ++++++--- libs/dotnet-sdk/State/FeatureBoardState.cs | 11 ++-- 6 files changed, 157 insertions(+), 52 deletions(-) diff --git a/libs/dotnet-sdk-test/FeatureBoardHttpClientTests.cs b/libs/dotnet-sdk-test/FeatureBoardHttpClientTests.cs index 000f0599..c86022fc 100644 --- a/libs/dotnet-sdk-test/FeatureBoardHttpClientTests.cs +++ b/libs/dotnet-sdk-test/FeatureBoardHttpClientTests.cs @@ -1,13 +1,12 @@ -using System; using System.Linq.Expressions; using System.Net; +using System.Net.Http.Headers; using System.Net.Http.Json; using System.Text.Json.Nodes; -using Microsoft.Extensions.Logging.Abstractions; using Bogus; -using Moq; using FeatureBoard.DotnetSdk.Models; -using System.Net.Http.Headers; +using Microsoft.Extensions.Logging.Abstractions; +using Moq; namespace FeatureBoard.DotnetSdk.Test { @@ -42,9 +41,13 @@ public async Task ItReturnsCorrectListOfFeatures() { // Arrange IReadOnlyCollection? actionArg = null; - void captureArgAction(IReadOnlyCollection features) => actionArg = features; + Task captureArgAction(IReadOnlyCollection features, CancellationToken token) + { + actionArg = features; + return Task.CompletedTask; + } - var testSubject = new FeatureBoardHttpClient(_mockHttpClient.Object, () => ref _nullETag, captureArgAction, new NullLogger()); + var testSubject = new FeatureBoardHttpClient(_mockHttpClient.Object, () => ref _nullETag, new FeatureConfigurationUpdated[] { captureArgAction }, new NullLogger()); // Act var result = await testSubject.RefreshFeatureConfiguration(CancellationToken.None); @@ -78,7 +81,7 @@ public async Task ItReturnsCorrectListOfFeatures() public async Task ItDoesNotProcessResponseIfNotModified() { // Arrange - static void nopAction(IReadOnlyCollection features) { } + static Task nopAction(IReadOnlyCollection features, CancellationToken token) => Task.CompletedTask; Expression> hasEtagMatcher = msg => _defaultRequestMatcher.Compile()(msg) && msg.Headers.IfNoneMatch.Any(t => t.Equals(new EntityTagHeaderValue(TestETag))); @@ -86,7 +89,7 @@ static void nopAction(IReadOnlyCollection features) { } .Setup(client => client.SendAsync(It.Is(hasEtagMatcher), It.IsAny())) .ReturnsAsync(new HttpResponseMessage() { StatusCode = HttpStatusCode.NotModified }); - var testSubject = new FeatureBoardHttpClient(_mockHttpClient.Object, () => ref _nullETag, nopAction, new NullLogger()); + var testSubject = new FeatureBoardHttpClient(_mockHttpClient.Object, () => ref _nullETag, new FeatureConfigurationUpdated[] { nopAction }, new NullLogger()); // Act var initialResult = await testSubject.RefreshFeatureConfiguration(CancellationToken.None); @@ -107,12 +110,10 @@ static void nopAction(IReadOnlyCollection features) { } public async Task ItDoesNotProcessResponseOnNonOkayResponse(HttpStatusCode httpStatusCode) { // Arrange - static void exceptionAction(IReadOnlyCollection features) => throw new InvalidOperationException(); - _mockHttpClient .Setup(client => client.SendAsync(It.Is(_defaultRequestMatcher), It.IsAny())) .ReturnsAsync((HttpRequestMessage request, CancellationToken _) => new HttpResponseMessage(httpStatusCode) { RequestMessage = request }); - var testSubject = new FeatureBoardHttpClient(_mockHttpClient.Object, () => ref _nullETag, exceptionAction, new NullLogger()); + var testSubject = new FeatureBoardHttpClient(_mockHttpClient.Object, () => ref _nullETag, Array.Empty(), new NullLogger()); // Act var result = await testSubject.RefreshFeatureConfiguration(CancellationToken.None); @@ -122,6 +123,48 @@ public async Task ItDoesNotProcessResponseOnNonOkayResponse(HttpStatusCode httpS } + public static object[][] HandlerExceptions => new[] + { + new [] { new ArgumentException() }, // eg. what would happen if duplicate feature keys are returned + }; + + [Theory] + [MemberData(nameof(HandlerExceptions))] + public async Task ItDoesNotAllowUpdateHandlerExceptionToBubble(Exception exception) + { + // Arrange + static Task nopAction(IReadOnlyCollection features, CancellationToken token) => Task.CompletedTask; + Task exceptionAction(IReadOnlyCollection features, CancellationToken token) => throw exception; + + var testSubject = new FeatureBoardHttpClient(_mockHttpClient.Object, () => ref _nullETag, new FeatureConfigurationUpdated[] { nopAction, exceptionAction }, new NullLogger()); + + // Act + var result = await testSubject.RefreshFeatureConfiguration(CancellationToken.None); + + // Assert + Assert.Null(result); + } + + + [Fact] + public async Task ItDoesNotAllowTransientNetworkRequestErrorsToBubble() + { + // Arrange + _mockHttpClient + .Setup(client => client.SendAsync(It.Is(_defaultRequestMatcher), It.IsAny())) + .ThrowsAsync(new HttpRequestException()); + + var testSubject = new FeatureBoardHttpClient(_mockHttpClient.Object, () => ref _nullETag, Array.Empty(), new NullLogger()); + + // Act + var result = await testSubject.RefreshFeatureConfiguration(CancellationToken.None); + + // Assert + Assert.Null(result); + } + + + private static FeatureConfiguration CreateFeature() { var faker = new Faker(); diff --git a/libs/dotnet-sdk-test/State/FeatureboardStateTests.cs b/libs/dotnet-sdk-test/State/FeatureboardStateTests.cs index 34c8a8df..e49768c1 100644 --- a/libs/dotnet-sdk-test/State/FeatureboardStateTests.cs +++ b/libs/dotnet-sdk-test/State/FeatureboardStateTests.cs @@ -22,12 +22,44 @@ public FeatureBoardStateTests() } - [Fact] - public async Task StartAsyncLoadsStateFromExternalStateIfProvided() + [Theory] + [InlineData(false)] + [InlineData(null)] + public async Task StartAsyncLoadsStateFromExternalStateIfFeatureBoardServiceDoesNotUpdate(bool? serviceReturn) + { + // Arrange + var featureConfiguration = CreateFeature(); + + Services.AddServiceMock((_, mock) => + mock.Setup(x => x.RefreshFeatureConfiguration(It.IsAny())) + .ReturnsAsync(serviceReturn) + ); + Services.AddServiceMock((_, mock) => + mock + .Setup(x => x.GetState(It.IsAny())) + .ReturnsAsync(new[] { featureConfiguration }) + ); + + var featureBoardState = Services.BuildServiceProvider().GetRequiredService(); + + // Act + await featureBoardState.StartAsync(CancellationToken.None); + var resolvedFeature = featureBoardState.GetSnapshot().Get(featureConfiguration.FeatureKey); + + // Assert + resolvedFeature.ShouldBe(featureConfiguration); + } + + [Fact(Skip = "Unclear what the behaviour should be in this scenario")] + public async Task StartAsyncLoadsStateFromExternalStateIfFeatureBoardServiceThrowsException() { // Arrange var featureConfiguration = CreateFeature(); + Services.AddServiceMock((_, mock) => + mock.Setup(x => x.RefreshFeatureConfiguration(It.IsAny())) + .ThrowsAsync(new Exception("Fail!")) + ); Services.AddServiceMock((_, mock) => mock .Setup(x => x.GetState(It.IsAny())) @@ -45,7 +77,7 @@ public async Task StartAsyncLoadsStateFromExternalStateIfProvided() } [Fact] - public async Task StartAsyncLoadsStateFromServiceIfExternalStateNotProvided() + public async Task StartAsyncLoadsStateFromServiceEvenIfExternalStateNotProvided() { // Arrange var featureConfiguration = CreateFeature(); diff --git a/libs/dotnet-sdk/FeatureBoard.DotnetSdk.csproj b/libs/dotnet-sdk/FeatureBoard.DotnetSdk.csproj index 71606eba..3154fd9b 100644 --- a/libs/dotnet-sdk/FeatureBoard.DotnetSdk.csproj +++ b/libs/dotnet-sdk/FeatureBoard.DotnetSdk.csproj @@ -35,6 +35,7 @@ + diff --git a/libs/dotnet-sdk/FeatureBoardHttpClient.cs b/libs/dotnet-sdk/FeatureBoardHttpClient.cs index 22cc88ed..e43ed8bf 100644 --- a/libs/dotnet-sdk/FeatureBoardHttpClient.cs +++ b/libs/dotnet-sdk/FeatureBoardHttpClient.cs @@ -1,27 +1,30 @@ using System.Net; +using System.Net.Http.Headers; using System.Net.Http.Json; -using Microsoft.Extensions.Logging; using FeatureBoard.DotnetSdk.Models; -using System.Net.Http.Headers; +using Microsoft.Extensions.Logging; namespace FeatureBoard.DotnetSdk; public delegate ref EntityTagHeaderValue? LastETagProvider(); +public delegate Task FeatureConfigurationUpdated(IReadOnlyCollection configuration, CancellationToken cancellationToken); + internal sealed class FeatureBoardHttpClient : IFeatureBoardHttpClient { internal static readonly string Action = "all"; private LastETagProvider _eTag; private readonly HttpClient _httpClient; - private readonly Action> _processResult; + private event FeatureConfigurationUpdated OnFeatureConfigurationUpdated = null!; private readonly ILogger _logger; - public FeatureBoardHttpClient(HttpClient httpClient, LastETagProvider lastModifiedTimeProvider, Action> processResult, ILogger logger) + public FeatureBoardHttpClient(HttpClient httpClient, LastETagProvider lastModifiedTimeProvider, IEnumerable updateHandlers, ILogger logger) { _httpClient = httpClient; - _processResult = processResult; + foreach (var handler in updateHandlers) + OnFeatureConfigurationUpdated += handler; _logger = logger; _eTag = lastModifiedTimeProvider; } @@ -33,32 +36,49 @@ public FeatureBoardHttpClient(HttpClient httpClient, LastETagProvider lastModifi if (null != eTag) request.Headers.IfNoneMatch.Add(eTag); - using var response = await _httpClient.SendAsync(request, cancellationToken); - - switch (response.StatusCode) + IReadOnlyCollection? features = null; + try { - case HttpStatusCode.NotModified: - _logger.LogDebug("No changes"); - return false; + using var response = await _httpClient.SendAsync(request, cancellationToken); - case not HttpStatusCode.OK: - _logger.LogError("Failed to get latest flags: Service returned error {statusCode}({responseBody})", response.StatusCode, await response.Content.ReadAsStringAsync()); - return null; - } + switch (response.StatusCode) + { + case HttpStatusCode.NotModified: + _logger.LogDebug("No changes"); + return false; - var features = await response.Content.ReadFromJsonAsync>(cancellationToken: cancellationToken) - ?? throw new ApplicationException("Unable to retrieve decode response content"); + case not HttpStatusCode.OK: + _logger.LogError("Failed to get latest flags: Service returned error {statusCode}({responseBody})", response.StatusCode, await response.Content.ReadAsStringAsync()); + return null; + } - _processResult(features); - updateEtagRef(response.Headers.ETag); + features = await response.Content.ReadFromJsonAsync>(cancellationToken: cancellationToken) + ?? throw new ApplicationException("Unable to retrieve decode response content"); + + updateEtagRef(response.Headers.ETag); + } + catch (HttpRequestException e) + { + _logger.LogError(e, "Failed to get latest flags"); + return null; + } + + try + { + await OnFeatureConfigurationUpdated(features, cancellationToken); + return true; + } + catch (ArgumentException e) // eg. thrown due to duplicate feature key + { + _logger.LogError(e, "Failed to update flags"); + 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); + _logger.LogDebug("Fetching updates done, eTag={eTag}", eTag); } - - return true; } } diff --git a/libs/dotnet-sdk/Registration/RegisterFeatureBoard.cs b/libs/dotnet-sdk/Registration/RegisterFeatureBoard.cs index 15a4dfd4..1b94838c 100644 --- a/libs/dotnet-sdk/Registration/RegisterFeatureBoard.cs +++ b/libs/dotnet-sdk/Registration/RegisterFeatureBoard.cs @@ -6,6 +6,7 @@ using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Options; +using Polly; namespace FeatureBoard.DotnetSdk.Registration; @@ -25,17 +26,27 @@ public static FeatureBoardBuilder AddFeatureBoard(this IServiceCollec client.BaseAddress = options.Value.HttpEndpoint; client.DefaultRequestHeaders.Add("x-environment-key", options.Value.EnvironmentApiKey); // client.Timeout = options.Value.MaxAge - TimeSpan.FromMilliseconds(3); //prevent multiple requests running at the same time. - }); + }).AddTransientHttpErrorPolicy(static policyBuilder => // DEBT: Get number retries from config + policyBuilder.WaitAndRetryAsync(retryCount: 5, static retryAttempt => TimeSpan.FromSeconds(Math.Pow(2, retryAttempt))) // TODO: Consider adding jitter + ); services.AddSingleton(() => ref lastETag); - services.AddScoped, FeatureBoardClient>(); - services.AddScoped(static c => c.GetRequiredService>()); - - services.AddSingleton(); - services.AddHostedService(static provider => provider.GetRequiredService()); - services.AddTransient>>(static provider => provider.GetRequiredService().Update); - services.AddScoped(static provider => provider.GetRequiredService().GetSnapshot()); + services.AddScoped, FeatureBoardClient>() + .AddScoped(static c => c.GetRequiredService>()); + + services.AddSingleton() + .AddHostedService(static provider => provider.GetRequiredService()) + .AddScoped(static provider => provider.GetRequiredService().GetSnapshot()) + .AddTransient(static provider => + { + var service = provider.GetRequiredService(); + return (config, _) => + { + service.Update(config); + return Task.CompletedTask; + }; + }); return new FeatureBoardBuilder(services); } @@ -71,7 +82,8 @@ public static FeatureBoardBuilder WithPollingUpdateStrategy(this FeatureBoardBui public static FeatureBoardBuilder WithExternalState(this FeatureBoardBuilder builder) where TStateStore : class, IFeatureBoardExternalState { - builder.Services.AddSingleton(); + builder.Services.AddSingleton() + .AddTransient(static provider => provider.GetRequiredService().UpdateState); return builder; } diff --git a/libs/dotnet-sdk/State/FeatureBoardState.cs b/libs/dotnet-sdk/State/FeatureBoardState.cs index dcdc856a..356167cc 100644 --- a/libs/dotnet-sdk/State/FeatureBoardState.cs +++ b/libs/dotnet-sdk/State/FeatureBoardState.cs @@ -1,7 +1,6 @@ -using System.Threading; +using FeatureBoard.DotnetSdk.Models; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Hosting; -using FeatureBoard.DotnetSdk.Models; namespace FeatureBoard.DotnetSdk.State; @@ -26,12 +25,10 @@ public FeatureBoardState(IServiceScopeFactory scopeFactory, IFeatureBoardExterna public async Task StartAsync(CancellationToken cancellationToken) { - if (_externalState is null) - { - using var scope = _scopeFactory.CreateScope(); - await scope.ServiceProvider.GetRequiredService().RefreshFeatureConfiguration(cancellationToken); + using var scope = _scopeFactory.CreateScope(); + var updated = await scope.ServiceProvider.GetRequiredService().RefreshFeatureConfiguration(cancellationToken) ?? false; + if (updated || _externalState is null) return; - } var state = await _externalState.GetState(cancellationToken); if (state == null) From 73947141fe0cc41034967206e17712398809ba12 Mon Sep 17 00:00:00 2001 From: Rhys Ickeringill <22509882+ickers@users.noreply.github.com> Date: Fri, 12 Jan 2024 13:04:53 +1100 Subject: [PATCH 2/5] FB-272: Lazily acquire FeatureBoardStateSnapshot to fix(?) startup race condition --- libs/dotnet-sdk-test/FeatureBoardClientTests.cs | 1 + libs/dotnet-sdk/FeatureBoardClient.cs | 9 ++++----- libs/dotnet-sdk/Registration/RegisterFeatureBoard.cs | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/libs/dotnet-sdk-test/FeatureBoardClientTests.cs b/libs/dotnet-sdk-test/FeatureBoardClientTests.cs index 50ce00f6..4b63abcd 100644 --- a/libs/dotnet-sdk-test/FeatureBoardClientTests.cs +++ b/libs/dotnet-sdk-test/FeatureBoardClientTests.cs @@ -18,6 +18,7 @@ public class FeatureBoardClientTests public FeatureBoardClientTests() { Services.AddTransient(typeof(ILogger<>), typeof(NullLogger<>)); + Services.AddTransient(typeof(Lazy<>)); Services.AddTransient>(); var audienceMock = Services.AddServiceMock((_, mock) => diff --git a/libs/dotnet-sdk/FeatureBoardClient.cs b/libs/dotnet-sdk/FeatureBoardClient.cs index d30c8db7..8871de57 100644 --- a/libs/dotnet-sdk/FeatureBoardClient.cs +++ b/libs/dotnet-sdk/FeatureBoardClient.cs @@ -1,22 +1,21 @@ -using System; using System.Linq.Expressions; using System.Reflection; using System.Text.Json.Nodes; -using Microsoft.Extensions.Logging; using FeatureBoard.DotnetSdk.Attributes; using FeatureBoard.DotnetSdk.Helpers; using FeatureBoard.DotnetSdk.Models; using FeatureBoard.DotnetSdk.State; +using Microsoft.Extensions.Logging; namespace FeatureBoard.DotnetSdk; internal class FeatureBoardClient : IFeatureBoardClient where TFeatures : class, IFeatures { - private readonly FeatureBoardStateSnapshot _state; + private readonly Lazy _state; private readonly IAudienceProvider _audienceProvider; private readonly ILogger _logger; - public FeatureBoardClient(FeatureBoardStateSnapshot state, IAudienceProvider audienceProvider, ILogger> logger) + public FeatureBoardClient(Lazy state, IAudienceProvider audienceProvider, ILogger> logger) { _state = state; _audienceProvider = audienceProvider; @@ -84,7 +83,7 @@ private static TProp GetFeatureValue(Expression> e private JsonValue? GetFeatureConfigurationValue(string featureKey, string? defaultValue) { - var feature = _state.Get(featureKey); + var feature = _state.Value.Get(featureKey); if (feature == null) { _logger.LogDebug("GetFeatureValue - no value, returning user fallback: {defaultValue}", defaultValue); diff --git a/libs/dotnet-sdk/Registration/RegisterFeatureBoard.cs b/libs/dotnet-sdk/Registration/RegisterFeatureBoard.cs index 1b94838c..18459767 100644 --- a/libs/dotnet-sdk/Registration/RegisterFeatureBoard.cs +++ b/libs/dotnet-sdk/Registration/RegisterFeatureBoard.cs @@ -37,7 +37,7 @@ public static FeatureBoardBuilder AddFeatureBoard(this IServiceCollec services.AddSingleton() .AddHostedService(static provider => provider.GetRequiredService()) - .AddScoped(static provider => provider.GetRequiredService().GetSnapshot()) + .AddScoped(static provider => new Lazy(provider.GetRequiredService().GetSnapshot)) .AddTransient(static provider => { var service = provider.GetRequiredService(); From fa68f9e3271b263c49cdfa692bc562b5fd108351 Mon Sep 17 00:00:00 2001 From: Rhys Ickeringill <22509882+ickers@users.noreply.github.com> Date: Tue, 16 Jan 2024 12:44:40 +1100 Subject: [PATCH 3/5] Don't use event MulticastDelegate for OnFeatureConfigurationUpdate signal as async exception behavhiour is not fit for purpose --- .../FeatureBoardClientTests.cs | 24 +++--- .../FeatureBoardHttpClientTests.cs | 83 +++++++++++++------ libs/dotnet-sdk/FeatureBoardHttpClient.cs | 33 +++++--- .../Registration/RegisterFeatureBoard.cs | 12 +-- libs/dotnet-sdk/State/FeatureBoardState.cs | 7 +- .../State/FeatureBoardStateUpdater.cs | 29 +++++++ .../State/IFeatureBoardExternalState.cs | 3 +- .../State/IFeatureBoardStateUpdateHandler.cs | 8 ++ 8 files changed, 136 insertions(+), 63 deletions(-) create mode 100644 libs/dotnet-sdk/State/FeatureBoardStateUpdater.cs create mode 100644 libs/dotnet-sdk/State/IFeatureBoardStateUpdateHandler.cs diff --git a/libs/dotnet-sdk-test/FeatureBoardClientTests.cs b/libs/dotnet-sdk-test/FeatureBoardClientTests.cs index 4b63abcd..f150c73c 100644 --- a/libs/dotnet-sdk-test/FeatureBoardClientTests.cs +++ b/libs/dotnet-sdk-test/FeatureBoardClientTests.cs @@ -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; @@ -30,7 +30,7 @@ public FeatureBoardClientTests() [Fact] public void ItReturnsTheDefaultValueWhenNoValueIsFound() { - Services.AddTransient(_ => new FeatureBoardStateSnapshot(new Dictionary(0))); + Services.AddTransient(_ => new FeatureBoardStateSnapshot(new Dictionary(0))); var client = Services.BuildServiceProvider().GetRequiredService>(); @@ -45,7 +45,7 @@ public void ItReturnsTheDefaultValueFromFeatureBoardWhenNoAudienceIsFound() { var faker = new Faker(); var defaultFeatureValue = faker.Lorem.Sentence(); - Services.AddTransient(_ => new FeatureBoardStateSnapshot(new Dictionary + Services.AddTransient(_ => new FeatureBoardStateSnapshot(new Dictionary { { nameof(TestFeatures.StringFeature).ToFeatureBoardKey(), new FeatureConfiguration @@ -67,7 +67,7 @@ public void ItReturnsTheAudienceValueFromFeatureBoardWhenAnAudienceIsFound() { var faker = new Faker(); var defaultAudienceValue = faker.Lorem.Paragraph(); - Services.AddTransient(_ => new FeatureBoardStateSnapshot(new Dictionary + Services.AddTransient(_ => new FeatureBoardStateSnapshot(new Dictionary { { nameof(TestFeatures.StringFeature).ToFeatureBoardKey(), new FeatureConfiguration @@ -96,7 +96,7 @@ public void ItReturnsADecimal() { var faker = new Faker(); var defaultAudienceValue = faker.Random.Decimal(); - Services.AddTransient(_ => new FeatureBoardStateSnapshot(new Dictionary + Services.AddTransient(_ => new FeatureBoardStateSnapshot(new Dictionary { { nameof(TestFeatures.NumberFeature).ToFeatureBoardKey(), new FeatureConfiguration @@ -126,7 +126,7 @@ public void ItReturnsABool() { var faker = new Faker(); var defaultAudienceValue = faker.Random.Bool(); - Services.AddTransient(_ => new FeatureBoardStateSnapshot(new Dictionary { + Services.AddTransient(_ => new FeatureBoardStateSnapshot(new Dictionary { { nameof(TestFeatures.BoolFeature).ToFeatureBoardKey(), new FeatureConfiguration { @@ -154,7 +154,7 @@ public void ItReturnsAString() { var faker = new Faker(); var defaultAudienceValue = Guid.NewGuid().ToString(); - Services.AddTransient(_ => new FeatureBoardStateSnapshot(new Dictionary + Services.AddTransient(_ => new FeatureBoardStateSnapshot(new Dictionary { { nameof(TestFeatures.StringFeature).ToFeatureBoardKey(), new FeatureConfiguration @@ -183,7 +183,7 @@ public void ItReturnsAnEnum() { var faker = new Faker(); var defaultAudienceValue = faker.PickRandom(); - Services.AddTransient(_ => new FeatureBoardStateSnapshot(new Dictionary { + Services.AddTransient(_ => new FeatureBoardStateSnapshot(new Dictionary { { nameof(TestFeatures.EnumFeature).ToFeatureBoardKey(), new FeatureConfiguration { @@ -212,7 +212,7 @@ public void ItLooksUpTheFeatureKeyWithFeatureKeyNameAttribute() { var faker = new Faker(); var defaultAudienceValue = Guid.NewGuid().ToString(); - Services.AddTransient(_ => new FeatureBoardStateSnapshot(new Dictionary { + Services.AddTransient(_ => new FeatureBoardStateSnapshot(new Dictionary { { "a-strange-name", new FeatureConfiguration { diff --git a/libs/dotnet-sdk-test/FeatureBoardHttpClientTests.cs b/libs/dotnet-sdk-test/FeatureBoardHttpClientTests.cs index c86022fc..bccf0dd0 100644 --- a/libs/dotnet-sdk-test/FeatureBoardHttpClientTests.cs +++ b/libs/dotnet-sdk-test/FeatureBoardHttpClientTests.cs @@ -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; @@ -17,14 +18,15 @@ public class FeatureBoardHttpClientTests private static readonly Expression> _defaultRequestMatcher = msg => msg.Method == HttpMethod.Get && msg.RequestUri!.OriginalString == FeatureBoardHttpClient.Action; - private readonly Mock _mockHttpClient = new Mock(); + private readonly Mock _mockHttpClient = new(); + private readonly Mock _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(_defaultRequestMatcher), It.IsAny())) .ReturnsAsync((HttpRequestMessage request, CancellationToken _) => @@ -34,6 +36,9 @@ public FeatureBoardHttpClientTests() return response; }); + _mockStateUpdater + .Setup(m => m.UpdateState(It.IsAny>(), It.IsAny())) + .Verifiable(); } [Fact] @@ -41,13 +46,11 @@ public async Task ItReturnsCorrectListOfFeatures() { // Arrange IReadOnlyCollection? actionArg = null; - Task captureArgAction(IReadOnlyCollection features, CancellationToken token) - { - actionArg = features; - return Task.CompletedTask; - } - var testSubject = new FeatureBoardHttpClient(_mockHttpClient.Object, () => ref _nullETag, new FeatureConfigurationUpdated[] { captureArgAction }, new NullLogger()); + _mockStateUpdater.Setup(m => m.UpdateState(It.IsAny>(), It.IsAny())) + .Callback((IReadOnlyCollection features, CancellationToken token) => actionArg = features); + + var testSubject = new FeatureBoardHttpClient(_mockHttpClient.Object, () => ref _nullETag, new[] { _mockStateUpdater.Object }, new NullLogger()); // Act var result = await testSubject.RefreshFeatureConfiguration(CancellationToken.None); @@ -58,18 +61,18 @@ Task captureArgAction(IReadOnlyCollection 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(), item.DefaultValue.GetValue()); + Assert.Equal(_testFeatureConfig.FeatureKey, item.FeatureKey); + Assert.Equal(_testFeatureConfig.DefaultValue.GetValue(), item.DefaultValue.GetValue()); 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(), ex.Value.GetValue()); }, ex => { - var expected = testFeatureConfig.AudienceExceptions[1]; + var expected = _testFeatureConfig.AudienceExceptions[1]; Assert.Equal(expected.AudienceKey, ex.AudienceKey); Assert.Equal(expected.Value.GetValue(), ex.Value.GetValue()); } @@ -81,15 +84,13 @@ Task captureArgAction(IReadOnlyCollection features, Cancel public async Task ItDoesNotProcessResponseIfNotModified() { // Arrange - static Task nopAction(IReadOnlyCollection features, CancellationToken token) => Task.CompletedTask; - Expression> hasEtagMatcher = msg => _defaultRequestMatcher.Compile()(msg) && msg.Headers.IfNoneMatch.Any(t => t.Equals(new EntityTagHeaderValue(TestETag))); _mockHttpClient .Setup(client => client.SendAsync(It.Is(hasEtagMatcher), It.IsAny())) .ReturnsAsync(new HttpResponseMessage() { StatusCode = HttpStatusCode.NotModified }); - var testSubject = new FeatureBoardHttpClient(_mockHttpClient.Object, () => ref _nullETag, new FeatureConfigurationUpdated[] { nopAction }, new NullLogger()); + var testSubject = new FeatureBoardHttpClient(_mockHttpClient.Object, () => ref _nullETag, new[] { _mockStateUpdater.Object }, new NullLogger()); // Act var initialResult = await testSubject.RefreshFeatureConfiguration(CancellationToken.None); @@ -113,36 +114,68 @@ public async Task ItDoesNotProcessResponseOnNonOkayResponse(HttpStatusCode httpS _mockHttpClient .Setup(client => client.SendAsync(It.Is(_defaultRequestMatcher), It.IsAny())) .ReturnsAsync((HttpRequestMessage request, CancellationToken _) => new HttpResponseMessage(httpStatusCode) { RequestMessage = request }); - var testSubject = new FeatureBoardHttpClient(_mockHttpClient.Object, () => ref _nullETag, Array.Empty(), new NullLogger()); + var testSubject = new FeatureBoardHttpClient(_mockHttpClient.Object, () => ref _nullETag, Array.Empty(), new NullLogger()); + + // 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(); + exceptioningUpdater.Setup(m => m.UpdateState(It.Is((IReadOnlyCollection features) => features.Any(config => config.FeatureKey == _testFeatureConfig.FeatureKey)), It.IsAny())) + .ThrowsAsync(exception); + + var testSubject = new FeatureBoardHttpClient(_mockHttpClient.Object, () => ref _nullETag, new[] { exceptioningUpdater.Object, _mockStateUpdater.Object }, new NullLogger()); // 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 features, CancellationToken token) => Task.CompletedTask; - Task exceptionAction(IReadOnlyCollection features, CancellationToken token) => throw exception; + var otherHandler = new Mock(); + otherHandler.Setup(m => m.UpdateState(It.IsAny>(), It.IsAny())); + + _mockStateUpdater.Setup(m => m.UpdateState(It.Is((IReadOnlyCollection features) => features.Any(config => config.FeatureKey == _testFeatureConfig.FeatureKey)), It.IsAny())) + .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()); + var testSubject = new FeatureBoardHttpClient(_mockHttpClient.Object, () => ref _nullETag, new[] { otherHandler.Object, _mockStateUpdater.Object, }, new NullLogger()); // 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 } @@ -154,7 +187,7 @@ public async Task ItDoesNotAllowTransientNetworkRequestErrorsToBubble() .Setup(client => client.SendAsync(It.Is(_defaultRequestMatcher), It.IsAny())) .ThrowsAsync(new HttpRequestException()); - var testSubject = new FeatureBoardHttpClient(_mockHttpClient.Object, () => ref _nullETag, Array.Empty(), new NullLogger()); + var testSubject = new FeatureBoardHttpClient(_mockHttpClient.Object, () => ref _nullETag, Array.Empty(), new NullLogger()); // Act var result = await testSubject.RefreshFeatureConfiguration(CancellationToken.None); diff --git a/libs/dotnet-sdk/FeatureBoardHttpClient.cs b/libs/dotnet-sdk/FeatureBoardHttpClient.cs index e43ed8bf..065d872f 100644 --- a/libs/dotnet-sdk/FeatureBoardHttpClient.cs +++ b/libs/dotnet-sdk/FeatureBoardHttpClient.cs @@ -8,25 +8,24 @@ namespace FeatureBoard.DotnetSdk; public delegate ref EntityTagHeaderValue? LastETagProvider(); -public delegate Task FeatureConfigurationUpdated(IReadOnlyCollection configuration, CancellationToken cancellationToken); internal sealed class FeatureBoardHttpClient : IFeatureBoardHttpClient { + private readonly IOrderedEnumerable _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 updateHandlers, ILogger logger) + public FeatureBoardHttpClient(HttpClient httpClient, LastETagProvider lastModifiedTimeProvider, IEnumerable updateHandlers, ILogger 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 RefreshFeatureConfiguration(CancellationToken cancellationToken) @@ -55,7 +54,8 @@ public FeatureBoardHttpClient(HttpClient httpClient, LastETagProvider lastModifi features = await response.Content.ReadFromJsonAsync>(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) { @@ -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; } } } diff --git a/libs/dotnet-sdk/Registration/RegisterFeatureBoard.cs b/libs/dotnet-sdk/Registration/RegisterFeatureBoard.cs index 18459767..89b6bfe5 100644 --- a/libs/dotnet-sdk/Registration/RegisterFeatureBoard.cs +++ b/libs/dotnet-sdk/Registration/RegisterFeatureBoard.cs @@ -38,15 +38,7 @@ public static FeatureBoardBuilder AddFeatureBoard(this IServiceCollec services.AddSingleton() .AddHostedService(static provider => provider.GetRequiredService()) .AddScoped(static provider => new Lazy(provider.GetRequiredService().GetSnapshot)) - .AddTransient(static provider => - { - var service = provider.GetRequiredService(); - return (config, _) => - { - service.Update(config); - return Task.CompletedTask; - }; - }); + .AddTransient(); return new FeatureBoardBuilder(services); } @@ -83,7 +75,7 @@ public static FeatureBoardBuilder WithPollingUpdateStrategy(this FeatureBoardBui public static FeatureBoardBuilder WithExternalState(this FeatureBoardBuilder builder) where TStateStore : class, IFeatureBoardExternalState { builder.Services.AddSingleton() - .AddTransient(static provider => provider.GetRequiredService().UpdateState); + .AddTransient(static provider => provider.GetRequiredService()); return builder; } diff --git a/libs/dotnet-sdk/State/FeatureBoardState.cs b/libs/dotnet-sdk/State/FeatureBoardState.cs index 356167cc..3a56c408 100644 --- a/libs/dotnet-sdk/State/FeatureBoardState.cs +++ b/libs/dotnet-sdk/State/FeatureBoardState.cs @@ -21,7 +21,12 @@ public FeatureBoardState(IServiceScopeFactory scopeFactory, IFeatureBoardExterna public FeatureBoardStateSnapshot GetSnapshot() => new FeatureBoardStateSnapshot(_cache); - public void Update(IReadOnlyCollection state) => _cache = state.ToDictionary(s => s.FeatureKey, s => s); + /// + /// Update internal cache of feature configuration based on supplied collection + /// + /// + /// Thrown if is null or is reused in collection + internal void Update(IReadOnlyCollection state) => _cache = state.ToDictionary(s => s.FeatureKey, s => s); public async Task StartAsync(CancellationToken cancellationToken) { diff --git a/libs/dotnet-sdk/State/FeatureBoardStateUpdater.cs b/libs/dotnet-sdk/State/FeatureBoardStateUpdater.cs new file mode 100644 index 00000000..844435d3 --- /dev/null +++ b/libs/dotnet-sdk/State/FeatureBoardStateUpdater.cs @@ -0,0 +1,29 @@ +using FeatureBoard.DotnetSdk.Models; + +namespace FeatureBoard.DotnetSdk.State; + +/// +/// Adapter around FeatureBoardState to allow Update to be called in IFeatureBoardStateUpdateHandler conformant manner +/// +internal class FeatureBoardStateUpdater : IFeatureBoardStateUpdateHandler +{ + private readonly FeatureBoardState _state; + + public FeatureBoardStateUpdater(FeatureBoardState state) => _state = state; + + public virtual Task UpdateState(IReadOnlyCollection configuration, CancellationToken cancellation) + { + if (cancellation.IsCancellationRequested) + return Task.FromCanceled(cancellation); + + try + { + _state.Update(configuration); + return Task.CompletedTask; + } + catch (Exception e) + { + return Task.FromException(e); + } + } +} diff --git a/libs/dotnet-sdk/State/IFeatureBoardExternalState.cs b/libs/dotnet-sdk/State/IFeatureBoardExternalState.cs index bcd06cd6..f1218bbb 100644 --- a/libs/dotnet-sdk/State/IFeatureBoardExternalState.cs +++ b/libs/dotnet-sdk/State/IFeatureBoardExternalState.cs @@ -2,8 +2,7 @@ namespace FeatureBoard.DotnetSdk.State; -public interface IFeatureBoardExternalState +public interface IFeatureBoardExternalState : IFeatureBoardStateUpdateHandler { Task> GetState(CancellationToken cancellationToken); - Task UpdateState(IReadOnlyCollection features, CancellationToken cancellationToken); } diff --git a/libs/dotnet-sdk/State/IFeatureBoardStateUpdateHandler.cs b/libs/dotnet-sdk/State/IFeatureBoardStateUpdateHandler.cs new file mode 100644 index 00000000..a106198d --- /dev/null +++ b/libs/dotnet-sdk/State/IFeatureBoardStateUpdateHandler.cs @@ -0,0 +1,8 @@ +using FeatureBoard.DotnetSdk.Models; + +namespace FeatureBoard.DotnetSdk.State; + +public interface IFeatureBoardStateUpdateHandler +{ + Task UpdateState(IReadOnlyCollection configuration, CancellationToken cancellation); +} From 821e4ff04874079a5f70a8c257e3d8cf4eab0bee Mon Sep 17 00:00:00 2001 From: Rhys Ickeringill <22509882+ickers@users.noreply.github.com> Date: Tue, 16 Jan 2024 15:24:26 +1100 Subject: [PATCH 4/5] Add support for retry-after header when retrying under polly policy --- .../Registration/RegisterFeatureBoard.cs | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/libs/dotnet-sdk/Registration/RegisterFeatureBoard.cs b/libs/dotnet-sdk/Registration/RegisterFeatureBoard.cs index 89b6bfe5..c4c1e516 100644 --- a/libs/dotnet-sdk/Registration/RegisterFeatureBoard.cs +++ b/libs/dotnet-sdk/Registration/RegisterFeatureBoard.cs @@ -1,3 +1,4 @@ +using System.Net; using System.Net.Http.Headers; using FeatureBoard.DotnetSdk.Models; using FeatureBoard.DotnetSdk.State; @@ -27,7 +28,19 @@ public static FeatureBoardBuilder AddFeatureBoard(this IServiceCollec client.DefaultRequestHeaders.Add("x-environment-key", options.Value.EnvironmentApiKey); // client.Timeout = options.Value.MaxAge - TimeSpan.FromMilliseconds(3); //prevent multiple requests running at the same time. }).AddTransientHttpErrorPolicy(static policyBuilder => // DEBT: Get number retries from config - policyBuilder.WaitAndRetryAsync(retryCount: 5, static retryAttempt => TimeSpan.FromSeconds(Math.Pow(2, retryAttempt))) // TODO: Consider adding jitter + policyBuilder +#if NET6_0_OR_GREATER + .OrResult(result => result.StatusCode == HttpStatusCode.TooManyRequests) +#else + .OrResult(result => (int)result.StatusCode == 429) +#endif + .WaitAndRetryAsync( + retryCount: 5, + sleepDurationProvider: static (retryAttempt, response, context) => + response.Result?.Headers.RetryAfter?.Delta // obey retry-after header if present in response + ?? TimeSpan.FromSeconds(Math.Pow(2, retryAttempt)), // fallback to basic exponential backoff if not. TODO: Consider adding jitter + onRetryAsync: static (response, delay, retryAttempt, context) => Task.CompletedTask + ) ); services.AddSingleton(() => ref lastETag); From 367990ad6a2c1a5cd302599b0ce9250737e3372f Mon Sep 17 00:00:00 2001 From: Rhys Ickeringill <22509882+ickers@users.noreply.github.com> Date: Tue, 16 Jan 2024 17:58:19 +1100 Subject: [PATCH 5/5] Fix file encoding --- libs/dotnet-sdk/State/IFeatureBoardStateUpdateHandler.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/dotnet-sdk/State/IFeatureBoardStateUpdateHandler.cs b/libs/dotnet-sdk/State/IFeatureBoardStateUpdateHandler.cs index a106198d..30def922 100644 --- a/libs/dotnet-sdk/State/IFeatureBoardStateUpdateHandler.cs +++ b/libs/dotnet-sdk/State/IFeatureBoardStateUpdateHandler.cs @@ -1,4 +1,4 @@ -using FeatureBoard.DotnetSdk.Models; +using FeatureBoard.DotnetSdk.Models; namespace FeatureBoard.DotnetSdk.State;