Minor improvements and fixes (#74)

* Add missing unsupported value check when evaluating multiple settings + simplify IRolloutEvaluator.Evaluate + eliminate EvaluationDetailsFactory * Fix return value of GetAllValues/GetAllValuesAsync when one or more settings evaluate with error * Bump version
configcat · Jun 23, 2023 · 939f897 · 939f897
1 parent 4b3efff
commit 939f897
Show file tree

Hide file tree

Showing 9 changed files with 136 additions and 125 deletions.
diff --git a/appveyor.yml b/appveyor.yml
@@ -1,5 +1,5 @@
 environment:
-  build_version: 8.1.0
+  build_version: 8.1.1
 version: $(build_version)-{build}
 image: Visual Studio 2022
 configuration: Release

diff --git a/src/ConfigCat.Client.Tests/ConfigCatClientTests.cs b/src/ConfigCat.Client.Tests/ConfigCatClientTests.cs
@@ -150,7 +150,7 @@ public void GetValue_EvaluateServiceThrowException_ShouldReturnDefaultValue()
         const string defaultValue = "Victory for the Firstborn!";
 
         this.evaluatorMock
-            .Setup(m => m.Evaluate(It.IsAny<Setting>(), It.IsAny<string>(), defaultValue, null, It.IsAny<ProjectConfig>(), It.IsAny<EvaluationDetailsFactory>()))
+            .Setup(m => m.Evaluate(It.IsAny<Setting>(), It.IsAny<string>(), defaultValue, null))
             .Throws<Exception>();
 
         var client = new ConfigCatClient(this.configServiceMock.Object, this.loggerMock.Object, this.evaluatorMock.Object, new Hooks());
@@ -179,7 +179,7 @@ public async Task GetValueAsync_EvaluateServiceThrowException_ShouldReturnDefaul
         const string defaultValue = "Victory for the Firstborn!";
 
         this.evaluatorMock
-            .Setup(m => m.Evaluate(It.IsAny<Setting>(), It.IsAny<string>(), defaultValue, null, It.IsAny<ProjectConfig>(), It.IsAny<EvaluationDetailsFactory>()))
+            .Setup(m => m.Evaluate(It.IsAny<Setting>(), It.IsAny<string>(), defaultValue, null))
             .Throws<Exception>();
 
         var client = new ConfigCatClient(this.configServiceMock.Object, this.loggerMock.Object, this.evaluatorMock.Object, new Hooks());
@@ -463,7 +463,7 @@ public async Task GetValueDetails_EvaluateServiceThrowException_ShouldReturnDefa
         var timeStamp = ProjectConfig.GenerateTimeStamp();
 
         this.evaluatorMock
-            .Setup(m => m.Evaluate(It.IsAny<Setting>(), It.IsAny<string>(), defaultValue, It.IsAny<User>(), It.IsAny<ProjectConfig>(), It.IsNotNull<EvaluationDetailsFactory>()))
+            .Setup(m => m.Evaluate(It.IsAny<Setting>(), It.IsAny<string>(), defaultValue, It.IsAny<User>()))
             .Throws(new ApplicationException(errorMessage));
 
         var client = CreateClientWithMockedFetcher(cacheKey, this.loggerMock, this.fetcherMock,
@@ -658,7 +658,7 @@ public async Task GetAllValueDetails_EvaluateServiceThrowException_ShouldReturnD
         var timeStamp = ProjectConfig.GenerateTimeStamp();
 
         this.evaluatorMock
-            .Setup(m => m.Evaluate(It.IsAny<Setting>(), It.IsAny<string>(), It.IsAny<string>(), It.IsAny<User>(), It.IsAny<ProjectConfig>(), It.IsNotNull<EvaluationDetailsFactory>()))
+            .Setup(m => m.Evaluate(It.IsAny<Setting>(), It.IsAny<string>(), It.IsAny<string>(), It.IsAny<User>()))
             .Throws(new ApplicationException(errorMessage));
 
         var client = CreateClientWithMockedFetcher(cacheKey, this.loggerMock, this.fetcherMock,

diff --git a/src/ConfigCat.Client.Tests/OverrideTests.cs b/src/ConfigCat.Client.Tests/OverrideTests.cs
@@ -1,9 +1,16 @@
 using System;
 using System.Collections.Generic;
 using System.IO;
+using System.Linq;
 using System.Threading.Tasks;
 using Microsoft.VisualStudio.TestTools.UnitTesting;
 
+#if USE_NEWTONSOFT_JSON
+using JsonValue = Newtonsoft.Json.Linq.JValue;
+#else
+using JsonValue = System.Text.Json.JsonElement;
+#endif
+
 namespace ConfigCat.Client.Tests;
 
 [TestClass]
@@ -511,8 +518,16 @@ public void OverrideValueTypeMismatchShouldBeHandledCorrectly_Dictionary(object
             .MakeGenericMethod(defaultValue.GetType());
 
         var actualEvaluatedValue = method.Invoke(client, new[] { key, defaultValue, null });
+        var actualEvaluatedValues = client.GetAllValues(user: null);
 
         Assert.AreEqual(expectedEvaluatedValue, actualEvaluatedValue);
+
+        var overrideValueSettingType = overrideValue.DetermineSettingType();
+        var expectedEvaluatedValues = new KeyValuePair<string, object?>[]
+        {
+             new(key, overrideValueSettingType != SettingType.Unknown ? overrideValue : null)
+        };
+        CollectionAssert.AreEquivalent(expectedEvaluatedValues, actualEvaluatedValues.ToArray());
     }
 
     [DataRow("true", false, true)]
@@ -542,6 +557,13 @@ public void OverrideValueTypeMismatchShouldBeHandledCorrectly_Dictionary(object
     public void OverrideValueTypeMismatchShouldBeHandledCorrectly_SimplifiedConfig(string overrideValueJson, object defaultValue, object expectedEvaluatedValue)
     {
         const string key = "flag";
+        var overrideValue =
+#if USE_NEWTONSOFT_JSON
+            overrideValueJson.Deserialize<Newtonsoft.Json.Linq.JToken>();
+#else
+            overrideValueJson.Deserialize<System.Text.Json.JsonElement>();
+#endif
+
 
         var filePath = Path.GetTempFileName();
         File.WriteAllText(filePath, $"{{ \"flags\": {{ \"{key}\": {overrideValueJson} }} }}");
@@ -559,8 +581,17 @@ public void OverrideValueTypeMismatchShouldBeHandledCorrectly_SimplifiedConfig(s
                 .MakeGenericMethod(defaultValue.GetType());
 
             var actualEvaluatedValue = method.Invoke(client, new[] { key, defaultValue, null });
+            var actualEvaluatedValues = client.GetAllValues(user: null);
 
             Assert.AreEqual(expectedEvaluatedValue, actualEvaluatedValue);
+            var overrideValueSettingType = overrideValue.DetermineSettingType();
+            var expectedEvaluatedValues = new KeyValuePair<string, object?>[]
+            {
+                new(key, overrideValueSettingType != SettingType.Unknown
+                    ? (overrideValue is JsonValue jsonValue ? jsonValue.ConvertToObject(overrideValueSettingType) : overrideValue)
+                    : null)
+            };
+            CollectionAssert.AreEquivalent(expectedEvaluatedValues, actualEvaluatedValues.ToArray());
         }
         finally
         {

diff --git a/src/ConfigCatClient/ConfigCatClient.cs b/src/ConfigCatClient/ConfigCatClient.cs
@@ -419,7 +419,6 @@ public async Task<IReadOnlyCollection<string>> GetAllKeysAsync(CancellationToken
     public IReadOnlyDictionary<string, object?> GetAllValues(User? user = null)
     {
         const string defaultReturnValue = "empty dictionary";
-        IReadOnlyDictionary<string, object?> result;
         EvaluationDetails[]? evaluationDetailsArray = null;
         user ??= this.defaultUser;
         try
@@ -430,15 +429,15 @@ public async Task<IReadOnlyCollection<string>> GetAllKeysAsync(CancellationToken
             {
                 throw new AggregateException(exceptions);
             }
-            result = evaluationDetailsArray.ToDictionary(details => details.Key, details => details.Value);
         }
         catch (Exception ex)
         {
             this.logger.SettingEvaluationError(nameof(GetAllValues), defaultReturnValue, ex);
             evaluationDetailsArray ??= ArrayUtils.EmptyArray<EvaluationDetails>();
-            result = new Dictionary<string, object?>();
         }
 
+        var result = evaluationDetailsArray.ToDictionary(details => details.Key, details => details.Value);
+
         foreach (var evaluationDetails in evaluationDetailsArray)
         {
             this.hooks.RaiseFlagEvaluated(evaluationDetails);
@@ -451,7 +450,6 @@ public async Task<IReadOnlyCollection<string>> GetAllKeysAsync(CancellationToken
     public async Task<IReadOnlyDictionary<string, object?>> GetAllValuesAsync(User? user = null, CancellationToken cancellationToken = default)
     {
         const string defaultReturnValue = "empty dictionary";
-        IReadOnlyDictionary<string, object?> result;
         EvaluationDetails[]? evaluationDetailsArray = null;
         user ??= this.defaultUser;
         try
@@ -462,7 +460,6 @@ public async Task<IReadOnlyCollection<string>> GetAllKeysAsync(CancellationToken
             {
                 throw new AggregateException(exceptions);
             }
-            result = evaluationDetailsArray.ToDictionary(details => details.Key, details => details.Value);
         }
         catch (OperationCanceledException ex) when (ex.CancellationToken == cancellationToken)
         {
@@ -472,9 +469,10 @@ public async Task<IReadOnlyCollection<string>> GetAllKeysAsync(CancellationToken
         {
             this.logger.SettingEvaluationError(nameof(GetAllValuesAsync), defaultReturnValue, ex);
             evaluationDetailsArray ??= ArrayUtils.EmptyArray<EvaluationDetails>();
-            result = new Dictionary<string, object?>();
         }
 
+        var result = evaluationDetailsArray.ToDictionary(details => details.Key, details => details.Value);
+
         foreach (var evaluationDetails in evaluationDetailsArray)
         {
             this.hooks.RaiseFlagEvaluated(evaluationDetails);

diff --git a/src/ConfigCatClient/Evaluation/EvaluateResult.cs b/src/ConfigCatClient/Evaluation/EvaluateResult.cs
@@ -0,0 +1,23 @@
+#if USE_NEWTONSOFT_JSON
+using JsonValue = Newtonsoft.Json.Linq.JValue;
+#else
+using JsonValue = System.Text.Json.JsonElement;
+#endif
+
+namespace ConfigCat.Client.Evaluation;
+
+internal readonly struct EvaluateResult
+{
+    public EvaluateResult(JsonValue value, string? variationId, RolloutRule? matchedTargetingRule = null, RolloutPercentageItem? matchedPercentageOption = null)
+    {
+        Value = value;
+        VariationId = variationId;
+        MatchedTargetingRule = matchedTargetingRule;
+        MatchedPercentageOption = matchedPercentageOption;
+    }
+
+    public JsonValue Value { get; }
+    public string? VariationId { get; }
+    public RolloutRule? MatchedTargetingRule { get; }
+    public RolloutPercentageItem? MatchedPercentageOption { get; }
+}
diff --git a/src/ConfigCatClient/Evaluation/EvaluationDetails.cs b/src/ConfigCatClient/Evaluation/EvaluationDetails.cs
@@ -1,5 +1,6 @@
 using System;
 using System.Diagnostics;
+using ConfigCat.Client.Evaluation;
 
 #if USE_NEWTONSOFT_JSON
 using JsonValue = Newtonsoft.Json.Linq.JValue;
@@ -9,23 +10,13 @@
 
 namespace ConfigCat.Client;
 
-internal delegate EvaluationDetails EvaluationDetailsFactory(Setting setting, JsonValue value);
-
 /// <summary>
 /// The evaluated value and additional information about the evaluation of a feature flag or setting.
 /// </summary>
 public abstract record class EvaluationDetails
 {
-    private static EvaluationDetails<TValue> Create<TValue>(JsonValue value)
+    private static void EnsureValidSettingValue(JsonValue value, ref SettingType settingType, string? unsupportedTypeError)
     {
-        return new EvaluationDetails<TValue> { Value = value.ConvertTo<TValue>() };
-    }
-
-    internal static EvaluationDetails<TValue> Create<TValue>(SettingType settingType, JsonValue value, string? unsupportedTypeError)
-    {
-        // NOTE: We've already checked earlier in the call chain that TValue is an allowed type (see also TypeExtensions.EnsureSupportedSettingClrType).
-        Debug.Assert(typeof(TValue) == typeof(object) || typeof(TValue).ToSettingType() != SettingType.Unknown, "Type is not supported.");
-
         // Setting type is not known (it's not present in the config JSON, it's an unsupported value coming from a flag override, etc.)?
         if (settingType == SettingType.Unknown)
         {
@@ -37,6 +28,23 @@ internal static EvaluationDetails<TValue> Create<TValue>(SettingType settingType
                 throw new ArgumentException(unsupportedTypeError ?? $"Setting value '{value}' is of an unsupported type.", nameof(value));
             }
         }
+    }
+
+    private static EvaluationDetails<TValue> Create<TValue>(string key, JsonValue value)
+    {
+        return new EvaluationDetails<TValue>(key, value.ConvertTo<TValue>());
+    }
+
+    internal static EvaluationDetails<TValue> FromEvaluateResult<TValue>(string key, in EvaluateResult evaluateResult, SettingType settingType, string? unsupportedTypeError,
+        DateTime? fetchTime, User? user)
+    {
+        // NOTE: We've already checked earlier in the call chain that TValue is an allowed type (see also TypeExtensions.EnsureSupportedSettingClrType).
+        Debug.Assert(typeof(TValue) == typeof(object) || typeof(TValue).ToSettingType() != SettingType.Unknown, "Type is not supported.");
+
+        var value = evaluateResult.Value;
+        EnsureValidSettingValue(value, ref settingType, unsupportedTypeError);
+
+        EvaluationDetails<TValue> instance;
 
         if (typeof(TValue) != typeof(object))
         {
@@ -45,60 +53,42 @@ internal static EvaluationDetails<TValue> Create<TValue>(SettingType settingType
                 throw new InvalidOperationException($"The type of a setting must match the type of the setting's default value.{Environment.NewLine}Setting's type was {settingType} but the default value's type was {typeof(TValue)}.{Environment.NewLine}Please use a default value which corresponds to the setting type {settingType}.");
             }
 
-            return Create<TValue>(value);
+            instance = Create<TValue>(key, value);
         }
         else
         {
-            EvaluationDetails evaluationDetails = new EvaluationDetails<object> { Value = value.ConvertToObject(settingType) };
-            return (EvaluationDetails<TValue>)evaluationDetails;
+            EvaluationDetails evaluationDetails = new EvaluationDetails<object>(key, value.ConvertToObject(settingType));
+            instance = (EvaluationDetails<TValue>)evaluationDetails;
         }
-    }
 
-    internal static EvaluationDetails Create(SettingType settingType, JsonValue value)
-    {
-        return settingType switch
-        {
-            SettingType.Boolean => Create<bool>(value),
-            SettingType.String => Create<string>(value),
-            SettingType.Int => Create<int>(value),
-            SettingType.Double => Create<double>(value),
-            _ => throw new ArgumentOutOfRangeException(nameof(settingType), settingType, null)
-        };
+        instance.Initialize(evaluateResult, fetchTime, user);
+        return instance;
     }
 
-    internal static EvaluationDetails FromJsonValue(
-        EvaluationDetailsFactory factory,
-        Setting setting,
-        string key,
-        JsonValue value,
-        string? variationId,
-        DateTime? fetchTime,
-        User? user,
-        RolloutRule? matchedEvaluationRule = null,
-        RolloutPercentageItem? matchedEvaluationPercentageRule = null)
+    internal static EvaluationDetails FromEvaluateResult(string key, in EvaluateResult evaluateResult, SettingType settingType, string? unsupportedTypeError,
+        DateTime? fetchTime, User? user)
     {
-        var instance = factory(setting, value);
+        var value = evaluateResult.Value;
+        EnsureValidSettingValue(value, ref settingType, unsupportedTypeError);
 
-        instance.Key = key;
-        instance.VariationId = variationId;
-        if (fetchTime is not null)
+        EvaluationDetails instance = settingType switch
         {
-            instance.FetchTime = fetchTime.Value;
-        }
-        instance.User = user;
-        instance.MatchedEvaluationRule = matchedEvaluationRule;
-        instance.MatchedEvaluationPercentageRule = matchedEvaluationPercentageRule;
+            SettingType.Boolean => Create<bool>(key, value),
+            SettingType.String => Create<string>(key, value),
+            SettingType.Int => Create<int>(key, value),
+            SettingType.Double => Create<double>(key, value),
+            _ => throw new ArgumentOutOfRangeException(nameof(settingType), settingType, null)
+        };
 
+        instance.Initialize(evaluateResult, fetchTime, user);
         return instance;
     }
 
     internal static EvaluationDetails<TValue> FromDefaultValue<TValue>(string key, TValue defaultValue, DateTime? fetchTime, User? user,
         string? errorMessage = null, Exception? errorException = null)
     {
-        var instance = new EvaluationDetails<TValue>
+        var instance = new EvaluationDetails<TValue>(key, defaultValue)
         {
-            Key = key,
-            Value = defaultValue,
             User = user,
             IsDefaultValue = true,
             ErrorMessage = errorMessage,
@@ -118,6 +108,18 @@ private protected EvaluationDetails(string key)
         Key = key;
     }
 
+    private void Initialize(in EvaluateResult evaluateResult, DateTime? fetchTime, User? user)
+    {
+        VariationId = evaluateResult.VariationId;
+        if (fetchTime is not null)
+        {
+            FetchTime = fetchTime.Value;
+        }
+        User = user;
+        MatchedEvaluationRule = evaluateResult.MatchedTargetingRule;
+        MatchedEvaluationPercentageRule = evaluateResult.MatchedPercentageOption;
+    }
+
     /// <summary>
     /// Key of the feature flag or setting.
     /// </summary>
@@ -175,8 +177,6 @@ private protected EvaluationDetails(string key)
 /// <inheritdoc/>
 public sealed record class EvaluationDetails<TValue> : EvaluationDetails
 {
-    internal EvaluationDetails() : this(key: null!, value: default!) { }
-
     /// <summary>
     /// Initializes a new instance of the <see cref="EvaluationDetails{TValue}"/> class.
     /// </summary>

diff --git a/src/ConfigCatClient/Evaluation/IRolloutEvaluator.cs b/src/ConfigCatClient/Evaluation/IRolloutEvaluator.cs
@@ -2,6 +2,5 @@ namespace ConfigCat.Client.Evaluation;
 
 internal interface IRolloutEvaluator
 {
-    EvaluationDetails Evaluate(Setting setting, string key, string? logDefaultValue, User? user,
-        ProjectConfig? remoteConfig, EvaluationDetailsFactory detailsFactory);
+    EvaluateResult Evaluate(Setting setting, string key, string? logDefaultValue, User? user);
 }