Skip to content

Commit

Permalink
Improve setting type mismatch handling & logging
Browse files Browse the repository at this point in the history
  • Loading branch information
adams85 committed Oct 24, 2023
1 parent fb0332a commit d410808
Show file tree
Hide file tree
Showing 6 changed files with 37 additions and 33 deletions.
7 changes: 5 additions & 2 deletions src/ConfigCatClient/Evaluation/EvaluateContext.cs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using System;
using System.Collections.Generic;
using ConfigCat.Client.Utils;

Expand All @@ -7,6 +8,7 @@ internal struct EvaluateContext
{
public readonly string Key;
public readonly Setting Setting;
public readonly Type ReturnValueType;
public readonly SettingValue DefaultValue;
public readonly User? User;
public readonly IReadOnlyDictionary<string, Setting> Settings;
Expand All @@ -22,10 +24,11 @@ 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, Type returnValueType, SettingValue defaultValue, User? user, IReadOnlyDictionary<string, Setting> settings)
{
this.Key = key;
this.Setting = setting;
this.ReturnValueType = returnValueType;
this.DefaultValue = defaultValue;
this.User = user;
this.Settings = settings;
Expand All @@ -37,7 +40,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.ReturnValueType, dependentFlagContext.DefaultValue, 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!
Expand Down
27 changes: 4 additions & 23 deletions src/ConfigCatClient/Evaluation/EvaluationDetails.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,31 +9,12 @@ 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, 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
{
EvaluationDetails evaluationDetails = new EvaluationDetails<object>(key, evaluateResult.Value.GetValue(settingType)!);
instance = (EvaluationDetails<TValue>)evaluationDetails;
}
var instance = typeof(TValue) != typeof(object)
? new EvaluationDetails<TValue>(key, evaluateResult.Value.GetValue<TValue>())
: (EvaluationDetails<TValue>)(EvaluationDetails)new EvaluationDetails<object>(key, evaluateResult.Value.GetValue()!);

instance.VariationId = evaluateResult.VariationId;
if (fetchTime is not null)
Expand Down
18 changes: 16 additions & 2 deletions src/ConfigCatClient/Evaluation/RolloutEvaluator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,27 @@ public EvaluateResult Evaluate(ref EvaluateContext context)
object? returnValue = null;
try
{
var expectedSettingType = context.ReturnValueType.ToSettingType();

// NOTE: We've already checked earlier in the call chain that TValue is an allowed type (see also TypeExtensions.EnsureSupportedSettingClrType).
Debug.Assert(context.ReturnValueType == typeof(object) || expectedSettingType != Setting.UnknownType, "Type is not supported.");

if (context.ReturnValueType != 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 {context.ReturnValueType}. "
+ $"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 = context.ReturnValueType != typeof(object) ? result.Value.GetValue(expectedSettingType) : result.Value.GetValue();
return result;
}
catch
{
returnValue = context.DefaultValue.GetValue(context.Setting.SettingType, throwIfInvalid: false);
logBuilder?.ResetIndent().IncreaseIndent();
returnValue = context.DefaultValue.GetValue(throwIfInvalid: false);
throw;
}
finally
Expand Down
8 changes: 4 additions & 4 deletions src/ConfigCatClient/Evaluation/RolloutEvaluatorExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,9 @@ 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 evaluateContext = new EvaluateContext(key, setting, typeof(T), defaultValue.ToSettingValue(out _), user, settings);
var evaluateResult = evaluator.Evaluate(ref evaluateContext);
return EvaluationDetails.FromEvaluateResult<T>(key, evaluateResult, setting.SettingType, fetchTime: remoteConfig?.TimeStamp, user);
return EvaluationDetails.FromEvaluateResult<T>(key, evaluateResult, fetchTime: remoteConfig?.TimeStamp, user);
}

public static EvaluationDetails[] EvaluateAll(this IRolloutEvaluator evaluator, Dictionary<string, Setting>? settings, User? user,
Expand All @@ -48,9 +48,9 @@ public static EvaluationDetails[] EvaluateAll(this IRolloutEvaluator evaluator,
EvaluationDetails evaluationDetails;
try
{
var evaluateContext = new EvaluateContext(kvp.Key, kvp.Value, defaultValue: default, user, settings);
var evaluateContext = new EvaluateContext(kvp.Key, kvp.Value, typeof(object), defaultValue: default, user, settings);
var evaluateResult = evaluator.Evaluate(ref evaluateContext);
evaluationDetails = EvaluationDetails.FromEvaluateResult<object>(kvp.Key, evaluateResult, kvp.Value.SettingType, fetchTime: remoteConfig?.TimeStamp, user);
evaluationDetails = EvaluationDetails.FromEvaluateResult<object>(kvp.Key, evaluateResult, fetchTime: remoteConfig?.TimeStamp, user);
}
catch (Exception ex)
{
Expand Down
4 changes: 2 additions & 2 deletions src/ConfigCatClient/Models/SettingValue.cs
Original file line number Diff line number Diff line change
Expand Up @@ -109,9 +109,9 @@ public object? UnsupportedValue
return value;
}

public readonly TValue GetValue<TValue>(SettingType settingType)
public readonly TValue GetValue<TValue>()
{
var value = GetValue(settingType)!;
var value = GetValue()!;

// In the case of Int settings, we also allow long and long? return types.
return typeof(TValue) switch
Expand Down
6 changes: 6 additions & 0 deletions src/ConfigCatClient/Utils/IndentedTextBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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++;
Expand Down

0 comments on commit d410808

Please sign in to comment.