From 0349391f65635421ec5a5a5bbd6af55edddf7b35 Mon Sep 17 00:00:00 2001 From: Adam Simon Date: Tue, 24 Oct 2023 18:27:04 +0200 Subject: [PATCH] Clean up setting type mismatch handling & logging logic + eliminate default value boxing + fix default value logging --- .../ConfigCatClientTests.cs | 8 ++--- .../EvaluationLogTests.cs | 6 ++-- .../Evaluation/EvaluateContext.cs | 6 ++-- .../Evaluation/EvaluateLogHelper.cs | 1 - .../Evaluation/EvaluationDetails.cs | 34 ++++--------------- .../Evaluation/IRolloutEvaluator.cs | 4 ++- .../Evaluation/RolloutEvaluator.cs | 25 +++++++++++--- .../Evaluation/RolloutEvaluatorExtensions.cs | 28 +++++++++++---- .../Utils/IndentedTextBuilder.cs | 6 ++++ 9 files changed, 67 insertions(+), 51 deletions(-) diff --git a/src/ConfigCat.Client.Tests/ConfigCatClientTests.cs b/src/ConfigCat.Client.Tests/ConfigCatClientTests.cs index e84c3e31..afe82ccc 100644 --- a/src/ConfigCat.Client.Tests/ConfigCatClientTests.cs +++ b/src/ConfigCat.Client.Tests/ConfigCatClientTests.cs @@ -192,7 +192,7 @@ public void GetValue_EvaluateServiceThrowException_ShouldReturnDefaultValue() .Throws(); this.evaluatorMock - .Setup(m => m.Evaluate(ref It.Ref.IsAny)) + .Setup(m => m.Evaluate(It.IsAny(), ref It.Ref.IsAny, out It.Ref.IsAny)) .Throws(); 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(); this.evaluatorMock - .Setup(m => m.Evaluate(ref It.Ref.IsAny)) + .Setup(m => m.Evaluate(It.IsAny(), ref It.Ref.IsAny, out It.Ref.IsAny)) .Throws(); 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.IsAny)) + .Setup(m => m.Evaluate(It.IsAny(), ref It.Ref.IsAny, out It.Ref.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.IsAny)) + .Setup(m => m.Evaluate(It.IsAny(), ref It.Ref.IsAny, out It.Ref.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 index 808ba8e8..54f6faf2 100644 --- 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(); evaluatorMock - .Setup(e => e.Evaluate(ref It.Ref.IsAny)) - .Returns((ref EvaluateContext ctx) => + .Setup(e => e.Evaluate(It.IsAny(), ref It.Ref.IsAny, out It.Ref.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 index 1aadebb3..5ec5d16b 100644 --- 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 Settings; @@ -22,11 +21,10 @@ internal struct EvaluateContext public IndentedTextBuilder? LogBuilder; - public EvaluateContext(string key, Setting setting, SettingValue defaultValue, User? user, IReadOnlyDictionary settings) + public EvaluateContext(string key, Setting setting, User? user, IReadOnlyDictionary 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 index c0b1fa80..30a90c8c 100644 --- 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 index 5fb0565b..5cee4682 100644 --- 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; /// public abstract record class EvaluationDetails { - internal static EvaluationDetails FromEvaluateResult(string key, in EvaluateResult evaluateResult, SettingType settingType, + internal static EvaluationDetails FromEvaluateResult(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 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(key, evaluateResult.Value.GetValue(settingType)); - } - else + var instance = new EvaluationDetails(key, value) { - EvaluationDetails evaluationDetails = new EvaluationDetails(key, evaluateResult.Value.GetValue(settingType)!); - instance = (EvaluationDetails)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 index a1f34434..dc4954c9 100644 --- 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 defaultValue, ref EvaluateContext context, [NotNull] out T returnValue); } diff --git a/src/ConfigCatClient/Evaluation/RolloutEvaluator.cs b/src/ConfigCatClient/Evaluation/RolloutEvaluator.cs index 120216e7..0a9ba443 100644 --- 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 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(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 index 1d1a0d28..ca3a55ff 100644 --- 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(this IRolloutEvaluator evaluator, T defaultValue, ref EvaluateContext context, [NotNull] out T returnValue) + { + return evaluator.Evaluate(defaultValue, ref context, out returnValue); + } + public static EvaluationDetails Evaluate(this IRolloutEvaluator evaluator, Dictionary? settings, string key, T defaultValue, User? user, ProjectConfig? remoteConfig, LoggerWrapper logger) { @@ -25,9 +32,13 @@ public static EvaluationDetails Evaluate(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(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? settings, User? user, @@ -41,6 +52,7 @@ public static EvaluationDetails[] EvaluateAll(this IRolloutEvaluator evaluator, var evaluationDetailsArray = new EvaluationDetails[settings.Count]; List? exceptionList = null; + var rolloutEvaluator = evaluator as RolloutEvaluator; var index = 0; foreach (var kvp in settings) @@ -48,9 +60,13 @@ public static EvaluationDetails[] EvaluateAll(this IRolloutEvaluator evaluator, EvaluationDetails evaluationDetails; try { - var evaluateContext = new EvaluateContext(kvp.Key, kvp.Value, defaultValue: default, user, settings); - var evaluateResult = evaluator.Evaluate(ref evaluateContext); - evaluationDetails = EvaluationDetails.FromEvaluateResult(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(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 index 8c25d5ae..6bbc69c9 100644 --- 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++;