PR comments and unit test

Signed-off-by: Torben Hørup <torben@t-hoerup.dk>

Author: hoerup

src/OpenFeature.Contrib.Providers.GOFeatureFlag/GoFeatureFlagProvider.cs

@@ -5,6 +5,7 @@
 using System.Net;
 using System.Net.Http;
 using System.Net.Http.Headers;
+using System.Runtime.CompilerServices;
 using System.Text;
 using System.Text.Json;
 using System.Threading;
@@ -19,6 +20,7 @@
 using OpenFeature.Model;
 using ZiggyCreatures.Caching.Fusion;
 
+[assembly: InternalsVisibleTo("OpenFeature.Contrib.Providers.GOFeatureFlag.Test")]
 namespace OpenFeature.Contrib.Providers.GOFeatureFlag
 {
     /// <summary>
@@ -30,7 +32,8 @@ public class GoFeatureFlagProvider : FeatureProvider
         private ExporterMetadata _exporterMetadata;
         private HttpClient _httpClient;
 
-        private IFusionCache _cache = null;
+        internal IFusionCache _cache = null;
+        internal MemoryCache _backingCache = null;
 
         /// <summary>
         ///     Constructor of the provider.
@@ -93,27 +96,25 @@ private void InitializeProvider(GoFeatureFlagProviderOptions options)
                     new AuthenticationHeaderValue("Bearer", options.ApiKey);
 
 
-            var cacheMaxSize = options.CacheMaxSize ?? 10000;
-            var cacheMaxTTL = options.CacheMaxTTL ?? TimeSpan.FromSeconds(60);
 
-            var backingMemoryCache = new MemoryCache(new MemoryCacheOptions
+            _backingCache = new MemoryCache(new MemoryCacheOptions
             {
-                SizeLimit = cacheMaxSize,
-                CompactionPercentage = 0.1,                    
+                SizeLimit = options.CacheMaxSize,
+                CompactionPercentage = 0.1,
             });
-            
+
             _cache = new FusionCache(new FusionCacheOptions
             {
                 DefaultEntryOptions = new FusionCacheEntryOptions
                 {
                     Size = 1,
-                    Duration = cacheMaxTTL,
+                    Duration = options.CacheMaxTTL,
                 },
-                
-            }, backingMemoryCache);
-           
+
+            }, _backingCache);
         }
 
+
         /// <summary>
         ///     Return the metadata associated to this provider.
         /// </summary>
@@ -176,7 +177,7 @@ public override async Task<ResolutionDetails<string>> ResolveStringValueAsync(st
             try
             {
                 var key = GenerateCacheKey(flagKey, context);
-                return await _cache.GetOrSetAsync<ResolutionDetails<string>>(key, async (_,_) =>
+                return await _cache.GetOrSetAsync<ResolutionDetails<string>>(key, async (_, _) =>
                 {
                     var resp = await CallApi(flagKey, defaultValue, context).ConfigureAwait(false);
                     if (!(resp.Value is JsonElement element && element.ValueKind == JsonValueKind.String))
@@ -213,8 +214,8 @@ public override async Task<ResolutionDetails<int>> ResolveIntegerValueAsync(stri
             try
             {
                 var key = GenerateCacheKey(flagKey, context);
-                return await _cache.GetOrSetAsync<ResolutionDetails<int>>(key, async (_,_) =>
-                { 
+                return await _cache.GetOrSetAsync<ResolutionDetails<int>>(key, async (_, _) =>
+                {
                     var resp = await CallApi(flagKey, defaultValue, context).ConfigureAwait(false);
                     return new ResolutionDetails<int>(flagKey, int.Parse(resp.Value.ToString()), ErrorType.None,
                         resp.Reason, resp.Variant, resp.ErrorDetails, resp.Metadata.ToImmutableMetadata());
@@ -249,7 +250,7 @@ public override async Task<ResolutionDetails<double>> ResolveDoubleValueAsync(st
             try
             {
                 var key = GenerateCacheKey(flagKey, context);
-                return await _cache.GetOrSetAsync<ResolutionDetails<double>>(key, async (_,_) =>
+                return await _cache.GetOrSetAsync<ResolutionDetails<double>>(key, async (_, _) =>
                 {
                     var resp = await CallApi(flagKey, defaultValue, context).ConfigureAwait(false);
                     return new ResolutionDetails<double>(flagKey,
@@ -286,7 +287,7 @@ public override async Task<ResolutionDetails<Value>> ResolveStructureValueAsync(
             try
             {
                 var key = GenerateCacheKey(flagKey, context);
-                return await _cache.GetOrSetAsync<ResolutionDetails<Value>>(key, async (_,_) =>
+                return await _cache.GetOrSetAsync<ResolutionDetails<Value>>(key, async (_, _) =>
                 {
                     var resp = await CallApi(flagKey, defaultValue, context).ConfigureAwait(false);
                     if (resp.Value is JsonElement)
@@ -358,7 +359,7 @@ private async Task<OfrepResponse> CallApi<T>(string flagKey, T defaultValue,
 
         private string GenerateCacheKey(string flagKey, EvaluationContext ctx)
         {
-            return ctx != null ? new OfrepRequest(ctx).AsJsonString() : flagKey;
+            return ctx != null ? flagKey + ":" + new OfrepRequest(ctx).AsJsonString() : flagKey;
         }
 
         /// <summary>

src/OpenFeature.Contrib.Providers.GOFeatureFlag/GoFeatureFlagProviderOptions.cs

@@ -47,12 +47,12 @@ public class GoFeatureFlagProviderOptions
         ///     (optional) How long is an entry allowed to be cached in memory 
         ///     Default: 60 seconds
         /// </summary>
-        public TimeSpan? CacheMaxTTL { get; set; }
+        public TimeSpan CacheMaxTTL { get; set; } = TimeSpan.FromSeconds(60);
 
         /// <summary>
         ///     (optional) How many entries may be cached
         ///     Default: 10000
         /// </summary>
-        public int? CacheMaxSize { get; set; }
+        public int CacheMaxSize { get; set; } = 10000;
     }
 }

src/OpenFeature.Contrib.Providers.GOFeatureFlag/OpenFeature.Contrib.Providers.GOFeatureFlag.csproj

@@ -16,7 +16,7 @@
   </ItemGroup>
 
   <ItemGroup>
-    <PackageReference Include="ZiggyCreatures.FusionCache" Version="2.2.0" />
+    <PackageReference Include="ZiggyCreatures.FusionCache" Version="2.1.0" />
   </ItemGroup>
 
   <PropertyGroup>

test/OpenFeature.Contrib.Providers.GOFeatureFlag.Test/GoFeatureFlagProviderTest.cs

@@ -6,6 +6,7 @@
 using System.Text.Json;
 using System.Threading.Tasks;
 using Newtonsoft.Json.Linq;
+using NSubstitute;
 using OpenFeature.Constant;
 using OpenFeature.Contrib.Providers.GOFeatureFlag.exception;
 using OpenFeature.Contrib.Providers.GOFeatureFlag.models;
@@ -17,14 +18,16 @@ namespace OpenFeature.Contrib.Providers.GOFeatureFlag.Test;
 
 public class GoFeatureFlagProviderTest
 {
+    private static readonly string mediaType = "application/json";
     private static readonly string baseUrl = "http://gofeatureflag.org";
     private static readonly string prefixEval = baseUrl + "/ofrep/v1/evaluate/flags/";
     private readonly EvaluationContext _defaultEvaluationCtx = InitDefaultEvaluationCtx();
-    private readonly HttpMessageHandler _mockHttp = InitMock();
-
-    private static HttpMessageHandler InitMock()
-    {
-        const string mediaType = "application/json";
+    private readonly MockHttpMessageHandler _mockHttp = InitMock();
+
+
+    private static MockHttpMessageHandler InitMock()
+    {
+
         var mockHttp = new MockHttpMessageHandler();
         mockHttp.When($"{prefixEval}fail_500").Respond(HttpStatusCode.InternalServerError);
         mockHttp.When($"{prefixEval}api_key_missing").Respond(HttpStatusCode.BadRequest);
@@ -59,8 +62,15 @@ private static HttpMessageHandler InitMock()
         mockHttp.When($"{prefixEval}does_not_exists").Respond(mediaType,
             "{ \"value\":\"\", \"key\":\"does_not_exists\", \"errorCode\":\"FLAG_NOT_FOUND\", \"variant\":\"defaultSdk\", \"cacheable\":true, \"errorDetails\":\"flag does_not_exists was not found in your configuration\"}");
         mockHttp.When($"{prefixEval}integer_with_metadata").Respond(mediaType,
-            "{ \"value\":100, \"key\":\"integer_key\", \"reason\":\"TARGETING_MATCH\", \"variant\":\"True\", \"cacheable\":true, \"metadata\":{\"key1\": \"key1\", \"key2\": 1, \"key3\": 1.345, \"key4\": true}}");
+            "{ \"value\":100, \"key\":\"integer_key\", \"reason\":\"TARGETING_MATCH\", \"variant\":\"True\", \"cacheable\":true, \"metadata\":{\"key1\": \"key1\", \"key2\": 1, \"key3\": 1.345, \"key4\": true}}");
+        for (int i = 0; i < 5; i++)
+        {
+            mockHttp.When($"{prefixEval}string_key{i}").Respond(mediaType,
+                $"{{ \"value\":\"C{i}\", \"key\":\"string_key{i}\", \"reason\":\"TARGETING_MATCH\", \"variant\":\"True\", \"cacheable\":true}}");
+        }
+
         return mockHttp;
+
     }
 
     private static EvaluationContext InitDefaultEvaluationCtx()
@@ -300,8 +310,89 @@ public async Task should_throw_an_error_if_we_expect_a_string_and_got_another_ty
         Assert.Equal("default", result.Value);
         Assert.Equal(ErrorType.TypeMismatch, result.ErrorType);
         Assert.Equal(Reason.Error, result.Reason);
-    }
+    }
+
+    [Fact]
+    public async Task should_cache_2nd_call()
+    {
+        var g = new GoFeatureFlagProvider(new GoFeatureFlagProviderOptions
+        {
+            Endpoint = baseUrl,
+            HttpMessageHandler = _mockHttp,
+            Timeout = new TimeSpan(1000 * TimeSpan.TicksPerMillisecond)
+        });
+        var req = _mockHttp.When($"{prefixEval}string_key_cache").Respond(mediaType,
+            "{ \"value\":\"CC00AA\", \"key\":\"string_key_cache\", \"reason\":\"TARGETING_MATCH\", \"variant\":\"True\", \"cacheable\":true}");
+        Assert.Equal(0, _mockHttp.GetMatchCount(req));
+
+        await Api.Instance.SetProviderAsync(g);
+        var client = Api.Instance.GetClient("test-client");
+        var result = await client.GetStringDetailsAsync("string_key_cache", "defaultValue", _defaultEvaluationCtx);
+        Assert.NotNull(result);
+        Assert.Equal("CC00AA", result.Value);
+        Assert.Equal(1, _mockHttp.GetMatchCount(req));
+
+        var result2 = await client.GetStringDetailsAsync("string_key_cache", "defaultValue", _defaultEvaluationCtx);
+        Assert.NotNull(result2);
+        Assert.Equal("CC00AA", result.Value);
+        Assert.Equal(1, _mockHttp.GetMatchCount(req)); // 2nd lookup should hit cache and not activate http
+    }
+
+
+
+    [Fact]
+    public async Task should_limit_cached_items()
+    {
+        var g = new GoFeatureFlagProvider(new GoFeatureFlagProviderOptions
+        {
+            Endpoint = baseUrl,
+            HttpMessageHandler = _mockHttp,
+            Timeout = new TimeSpan(1000 * TimeSpan.TicksPerMillisecond),
+            CacheMaxSize = 2
+        });
+        await Api.Instance.SetProviderAsync(g);
+        var client = Api.Instance.GetClient("test-client");
+
+        for (var i = 0; i < 5; i++)
+        {
+            var res1 = await client.GetStringDetailsAsync($"string_key{i}", "defaultValue", _defaultEvaluationCtx);
+            Assert.NotNull(res1);
+            Assert.Equal($"C{i}", res1.Value);
+        }
+
+        Assert.Equal(2, g._backingCache.Count);
+    }
+
+    [Fact]
+    public async Task should_not_cache()
+    {
+        var g = new GoFeatureFlagProvider(new GoFeatureFlagProviderOptions
+        {
+            Endpoint = baseUrl,
+            HttpMessageHandler = _mockHttp,
+            Timeout = new TimeSpan(1000 * TimeSpan.TicksPerMillisecond),
+            CacheMaxTTL = TimeSpan.FromSeconds(0)
+        });
+        var req = _mockHttp.When($"{prefixEval}string_key_cache").Respond(mediaType,
+            "{ \"value\":\"CC00AA\", \"key\":\"string_key_cache\", \"reason\":\"TARGETING_MATCH\", \"variant\":\"True\", \"cacheable\":true}");
+
+
+        Assert.Equal(0, _mockHttp.GetMatchCount(req));
 
+        await Api.Instance.SetProviderAsync(g);
+        var client = Api.Instance.GetClient("test-client");
+        var result = await client.GetStringDetailsAsync("string_key_cache", "defaultValue", _defaultEvaluationCtx);
+        Assert.NotNull(result);
+        Assert.Equal("CC00AA", result.Value);
+        Assert.Equal(1, _mockHttp.GetMatchCount(req));
+
+        var result2 = await client.GetStringDetailsAsync("string_key_cache", "defaultValue", _defaultEvaluationCtx);
+        Assert.NotNull(result2);
+        Assert.Equal("CC00AA", result.Value);
+        Assert.Equal(2, _mockHttp.GetMatchCount(req)); // 2nd lookup should not be cached, but hit http bringing total matches up
+    }
+
+
     [Fact]
     public async Task should_resolve_a_valid_string_flag_with_TARGETING_MATCH_reason()
     {
@@ -387,6 +478,7 @@ public async Task should_use_integer_default_value_if_the_flag_is_disabled()
         await Api.Instance.SetProviderAsync(g);
         var client = Api.Instance.GetClient("test-client");
         var result = await client.GetIntegerDetailsAsync("disabled_integer", 1225, _defaultEvaluationCtx);
+
         Assert.NotNull(result);
         Assert.Equal(1225, result.Value);
         Assert.Equal(Reason.Disabled, result.Reason);