Clean up setting type mismatch handling & logging logic + eliminate d…

…efault value boxing + fix default value logging
configcat · Oct 25, 2023 · 0349391 · 0349391
1 parent 30cecc0
commit 0349391
Show file tree

Hide file tree

Showing 9 changed files with 67 additions and 51 deletions.
diff --git a/src/ConfigCat.Client.Tests/ConfigCatClientTests.cs b/src/ConfigCat.Client.Tests/ConfigCatClientTests.cs
@@ -192,7 +192,7 @@ public void GetValue_EvaluateServiceThrowException_ShouldReturnDefaultValue()
             .Throws<Exception>();
 
         this.evaluatorMock
-            .Setup(m => m.Evaluate(ref It.Ref<EvaluateContext>.IsAny))
+            .Setup(m => m.Evaluate(It.IsAny<It.IsAnyType>(), ref It.Ref<EvaluateContext>.IsAny, out It.Ref<It.IsAnyType>.IsAny))
             .Throws<Exception>();
 
         var client = new ConfigCatClient(this.configServiceMock.Object, this.loggerMock.Object, this.evaluatorMock.Object, new Hooks());
@@ -225,7 +225,7 @@ public async Task GetValueAsync_EvaluateServiceThrowException_ShouldReturnDefaul
             .Throws<Exception>();
 
         this.evaluatorMock
-            .Setup(m => m.Evaluate(ref It.Ref<EvaluateContext>.IsAny))
+            .Setup(m => m.Evaluate(It.IsAny<It.IsAnyType>(), ref It.Ref<EvaluateContext>.IsAny, out It.Ref<It.IsAnyType>.IsAny))
             .Throws<Exception>();
 
         var client = new ConfigCatClient(this.configServiceMock.Object, this.loggerMock.Object, this.evaluatorMock.Object, new Hooks());
@@ -509,7 +509,7 @@ public async Task GetValueDetails_EvaluateServiceThrowException_ShouldReturnDefa
         var timeStamp = ProjectConfig.GenerateTimeStamp();
 
         this.evaluatorMock
-            .Setup(m => m.Evaluate(ref It.Ref<EvaluateContext>.IsAny))
+            .Setup(m => m.Evaluate(It.IsAny<It.IsAnyType>(), ref It.Ref<EvaluateContext>.IsAny, out It.Ref<It.IsAnyType>.IsAny))
             .Throws(new ApplicationException(errorMessage));
 
         var client = CreateClientWithMockedFetcher(cacheKey, this.loggerMock, this.fetcherMock,
@@ -708,7 +708,7 @@ public async Task GetAllValueDetails_EvaluateServiceThrowException_ShouldReturnD
         var timeStamp = ProjectConfig.GenerateTimeStamp();
 
         this.evaluatorMock
-            .Setup(m => m.Evaluate(ref It.Ref<EvaluateContext>.IsAny))
+            .Setup(m => m.Evaluate(It.IsAny<It.IsAnyType>(), ref It.Ref<EvaluateContext>.IsAny, out It.Ref<It.IsAnyType>.IsAny))
             .Throws(new ApplicationException(errorMessage));
 
         var client = CreateClientWithMockedFetcher(cacheKey, this.loggerMock, this.fetcherMock,

diff --git a/src/ConfigCat.Client.Tests/EvaluationLogTests.cs b/src/ConfigCat.Client.Tests/EvaluationLogTests.cs
@@ -320,10 +320,10 @@ public void EvaluationLogShouldBeBuiltOnlyWhenNecessary(LogLevel logLevel, bool
         var actualIsLogBuilt = false;
         var evaluatorMock = new Mock<IRolloutEvaluator>();
         evaluatorMock
-            .Setup(e => e.Evaluate(ref It.Ref<EvaluateContext>.IsAny))
-            .Returns((ref EvaluateContext ctx) =>
+            .Setup(e => e.Evaluate(It.IsAny<bool?>(), ref It.Ref<EvaluateContext>.IsAny, out It.Ref<bool?>.IsAny))
+            .Returns((bool? defaultValue, ref EvaluateContext ctx, out bool? returnValue) =>
             {
-                var result = evaluator.Evaluate(ref ctx);
+                var result = evaluator.Evaluate(defaultValue, ref ctx, out returnValue);
                 actualIsLogBuilt = ctx.LogBuilder is not null;
                 return result;
             });

diff --git a/src/ConfigCatClient/Evaluation/EvaluateContext.cs b/src/ConfigCatClient/Evaluation/EvaluateContext.cs
@@ -7,7 +7,6 @@ internal struct EvaluateContext
 {
     public readonly string Key;
     public readonly Setting Setting;
-    public readonly SettingValue DefaultValue;
     public readonly User? User;
     public readonly IReadOnlyDictionary<string, Setting> Settings;
 
@@ -22,11 +21,10 @@ internal struct EvaluateContext
 
     public IndentedTextBuilder? LogBuilder;
 
-    public EvaluateContext(string key, Setting setting, SettingValue defaultValue, User? user, IReadOnlyDictionary<string, Setting> settings)
+    public EvaluateContext(string key, Setting setting, User? user, IReadOnlyDictionary<string, Setting> settings)
     {
         this.Key = key;
         this.Setting = setting;
-        this.DefaultValue = defaultValue;
         this.User = user;
         this.Settings = settings;
 
@@ -37,7 +35,7 @@ public EvaluateContext(string key, Setting setting, SettingValue defaultValue, U
     }
 
     public EvaluateContext(string key, Setting setting, ref EvaluateContext dependentFlagContext)
-        : this(key, setting, dependentFlagContext.DefaultValue, dependentFlagContext.User, dependentFlagContext.Settings)
+        : this(key, setting, dependentFlagContext.User, dependentFlagContext.Settings)
     {
         this.userAttributes = dependentFlagContext.userAttributes;
         this.visitedFlags = dependentFlagContext.VisitedFlags; // crucial to use the property here to make sure the list is created!

diff --git a/src/ConfigCatClient/Evaluation/EvaluateLogHelper.cs b/src/ConfigCatClient/Evaluation/EvaluateLogHelper.cs
@@ -1,5 +1,4 @@
 using ConfigCat.Client.Utils;
-using System;
 using System.Globalization;
 
 namespace ConfigCat.Client.Evaluation;

diff --git a/src/ConfigCatClient/Evaluation/EvaluationDetails.cs b/src/ConfigCatClient/Evaluation/EvaluationDetails.cs
@@ -1,5 +1,4 @@
 using System;
-using System.Diagnostics;
 using ConfigCat.Client.Evaluation;
 
 namespace ConfigCat.Client;
@@ -9,40 +8,21 @@ namespace ConfigCat.Client;
 /// </summary>
 public abstract record class EvaluationDetails
 {
-    internal static EvaluationDetails<TValue> FromEvaluateResult<TValue>(string key, in EvaluateResult evaluateResult, SettingType settingType,
+    internal static EvaluationDetails<TValue> FromEvaluateResult<TValue>(string key, TValue value, in EvaluateResult evaluateResult,
         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() != Setting.UnknownType, "Type is not supported.");
-
-        EvaluationDetails<TValue> instance;
-
-        if (typeof(TValue) != typeof(object))
-        {
-            if (settingType != Setting.UnknownType && settingType != typeof(TValue).ToSettingType())
-            {
-                throw new InvalidOperationException(
-                    "The type of a setting must match the type of the setting's default value. "
-                    + $"Setting's type was {settingType} but the default value's type was {typeof(TValue)}. "
-                    + $"Please use a default value which corresponds to the setting type {settingType}.");
-            }
-
-            instance = new EvaluationDetails<TValue>(key, evaluateResult.Value.GetValue<TValue>(settingType));
-        }
-        else
+        var instance = new EvaluationDetails<TValue>(key, value)
         {
-            EvaluationDetails evaluationDetails = new EvaluationDetails<object>(key, evaluateResult.Value.GetValue(settingType)!);
-            instance = (EvaluationDetails<TValue>)evaluationDetails;
-        }
+            User = user,
+            VariationId = evaluateResult.VariationId,
+            MatchedTargetingRule = evaluateResult.MatchedTargetingRule,
+            MatchedPercentageOption = evaluateResult.MatchedPercentageOption
+        };
 
-        instance.VariationId = evaluateResult.VariationId;
         if (fetchTime is not null)
         {
             instance.FetchTime = fetchTime.Value;
         }
-        instance.User = user;
-        instance.MatchedTargetingRule = evaluateResult.MatchedTargetingRule;
-        instance.MatchedPercentageOption = evaluateResult.MatchedPercentageOption;
 
         return instance;
     }

diff --git a/src/ConfigCatClient/Evaluation/IRolloutEvaluator.cs b/src/ConfigCatClient/Evaluation/IRolloutEvaluator.cs
@@ -1,6 +1,8 @@
+using System.Diagnostics.CodeAnalysis;
+
 namespace ConfigCat.Client.Evaluation;
 
 internal interface IRolloutEvaluator
 {
-    EvaluateResult Evaluate(ref EvaluateContext context);
+    EvaluateResult Evaluate<T>(T defaultValue, ref EvaluateContext context, [NotNull] out T returnValue);
 }
diff --git a/src/ConfigCatClient/Evaluation/RolloutEvaluator.cs b/src/ConfigCatClient/Evaluation/RolloutEvaluator.cs
@@ -23,11 +23,11 @@ public RolloutEvaluator(LoggerWrapper logger)
         this.logger = logger;
     }
 
-    public EvaluateResult Evaluate(ref EvaluateContext context)
+    public EvaluateResult Evaluate<T>(T defaultValue, ref EvaluateContext context, [NotNull] out T returnValue)
     {
         ref var logBuilder = ref context.LogBuilder;
 
-        // Building the evaluation log is relatively expensive, so let's not do it if it wouldn't be logged anyway.
+        // Building the evaluation log is expensive, so let's not do it if it wouldn't be logged anyway.
         if (this.logger.IsEnabled(LogLevel.Info))
         {
             logBuilder = new IndentedTextBuilder();
@@ -42,16 +42,31 @@ public EvaluateResult Evaluate(ref EvaluateContext context)
             logBuilder.IncreaseIndent();
         }
 
-        object? returnValue = null;
+        returnValue = default!;
         try
         {
+            var expectedSettingType = typeof(T).ToSettingType();
+
+            // NOTE: We've already checked earlier in the call chain that TValue is an allowed type (see also TypeExtensions.EnsureSupportedSettingClrType).
+            Debug.Assert(typeof(T) == typeof(object) || expectedSettingType != Setting.UnknownType, "Type is not supported.");
+
+            if (typeof(T) != typeof(object) && context.Setting.SettingType != Setting.UnknownType && expectedSettingType != context.Setting.SettingType)
+            {
+                throw new InvalidOperationException(
+                    "The type of a setting must match the type of the setting's default value. "
+                    + $"Setting's type was {context.Setting.SettingType} but the default value's type was {typeof(T)}. "
+                    + $"Please use a default value which corresponds to the setting type {context.Setting.SettingType}.");
+            }
+
             var result = EvaluateSetting(ref context);
-            returnValue = result.Value.GetValue(context.Setting.SettingType, throwIfInvalid: false) ?? EvaluateLogHelper.InvalidValuePlaceholder;
+            returnValue = typeof(T) != typeof(object) ? result.Value.GetValue<T>(expectedSettingType)! : (T)result.Value.GetValue()!;
             return result;
         }
         catch
         {
-            returnValue = context.DefaultValue.GetValue(context.Setting.SettingType, throwIfInvalid: false);
+            logBuilder?.ResetIndent().IncreaseIndent();
+
+            returnValue = defaultValue;
             throw;
         }
         finally

diff --git a/src/ConfigCatClient/Evaluation/RolloutEvaluatorExtensions.cs b/src/ConfigCatClient/Evaluation/RolloutEvaluatorExtensions.cs
@@ -1,12 +1,19 @@
 using System;
 using System.Collections.Generic;
 using System.Diagnostics.CodeAnalysis;
+using System.Runtime.CompilerServices;
 using ConfigCat.Client.Utils;
 
 namespace ConfigCat.Client.Evaluation;
 
 internal static class RolloutEvaluatorExtensions
 {
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    private static EvaluateResult EvaluateVirtual<T>(this IRolloutEvaluator evaluator, T defaultValue, ref EvaluateContext context, [NotNull] out T returnValue)
+    {
+        return evaluator.Evaluate(defaultValue, ref context, out returnValue);
+    }
+
     public static EvaluationDetails<T> Evaluate<T>(this IRolloutEvaluator evaluator, Dictionary<string, Setting>? settings, string key, T defaultValue, User? user,
         ProjectConfig? remoteConfig, LoggerWrapper logger)
     {
@@ -25,9 +32,13 @@ public static EvaluationDetails<T> Evaluate<T>(this IRolloutEvaluator evaluator,
             return EvaluationDetails.FromDefaultValue(key, defaultValue, fetchTime: remoteConfig?.TimeStamp, user, logMessage.InvariantFormattedMessage);
         }
 
-        var evaluateContext = new EvaluateContext(key, setting, defaultValue.ToSettingValue(out _), user, settings);
-        var evaluateResult = evaluator.Evaluate(ref evaluateContext);
-        return EvaluationDetails.FromEvaluateResult<T>(key, evaluateResult, setting.SettingType, fetchTime: remoteConfig?.TimeStamp, user);
+        var evaluateContext = new EvaluateContext(key, setting, user, settings);
+        // NOTE: It's better to avoid virtual generic method calls as they are slow and may be problematic for older AOT compilers (like Mono AOT or IL2CPP),
+        // especially, when targeting platforms which disallow the execution of dynamically generated code (e.g. Xamarin.iOS).
+        var evaluateResult = evaluator is RolloutEvaluator rolloutEvaluator
+            ? rolloutEvaluator.Evaluate(defaultValue, ref evaluateContext, out var value)
+            : evaluator.EvaluateVirtual(defaultValue, ref evaluateContext, out value);
+        return EvaluationDetails.FromEvaluateResult(key, value, evaluateResult, fetchTime: remoteConfig?.TimeStamp, user);
     }
 
     public static EvaluationDetails[] EvaluateAll(this IRolloutEvaluator evaluator, Dictionary<string, Setting>? settings, User? user,
@@ -41,16 +52,21 @@ public static EvaluationDetails[] EvaluateAll(this IRolloutEvaluator evaluator,
 
         var evaluationDetailsArray = new EvaluationDetails[settings.Count];
         List<Exception>? exceptionList = null;
+        var rolloutEvaluator = evaluator as RolloutEvaluator;
 
         var index = 0;
         foreach (var kvp in settings)
         {
             EvaluationDetails evaluationDetails;
             try
             {
-                var evaluateContext = new EvaluateContext(kvp.Key, kvp.Value, defaultValue: default, user, settings);
-                var evaluateResult = evaluator.Evaluate(ref evaluateContext);
-                evaluationDetails = EvaluationDetails.FromEvaluateResult<object>(kvp.Key, evaluateResult, kvp.Value.SettingType, fetchTime: remoteConfig?.TimeStamp, user);
+                var evaluateContext = new EvaluateContext(kvp.Key, kvp.Value, user, settings);
+                // NOTE: It's better to avoid virtual generic method calls as they are slow and may be problematic for older AOT compilers (like Mono AOT or IL2CPP),
+                // especially, when targeting platforms which disallow the execution of dynamically generated code (e.g. Xamarin.iOS).
+                var evaluateResult = rolloutEvaluator is not null
+                    ? rolloutEvaluator.Evaluate<object?>(defaultValue: null, ref evaluateContext, out var value)
+                    : evaluator.Evaluate(defaultValue: null, ref evaluateContext, out value);
+                evaluationDetails = EvaluationDetails.FromEvaluateResult(kvp.Key, value, evaluateResult, fetchTime: remoteConfig?.TimeStamp, user);
             }
             catch (Exception ex)
             {

diff --git a/src/ConfigCatClient/Utils/IndentedTextBuilder.cs b/src/ConfigCatClient/Utils/IndentedTextBuilder.cs
@@ -11,6 +11,12 @@ internal class IndentedTextBuilder
     private readonly StringBuilder stringBuilder = new();
     private int indentLevel;
 
+    public IndentedTextBuilder ResetIndent()
+    {
+        this.indentLevel = 0;
+        return this;
+    }
+
     public IndentedTextBuilder IncreaseIndent()
     {
         this.indentLevel++;