Fix failure case in segment evaluation + add more tests for segment e…

…valuation
configcat · Oct 27, 2023 · 0ec75a8 · 0ec75a8
1 parent 8292555
commit 0ec75a8
Show file tree

Hide file tree

Showing 6 changed files with 98 additions and 25 deletions.
diff --git a/src/ConfigCat.Client.Tests/ConfigV1EvaluationTests.cs b/src/ConfigCat.Client.Tests/ConfigV1EvaluationTests.cs
@@ -23,6 +23,14 @@ public class NumericTestsDescriptor : IMatrixTestDescriptor
         public static IEnumerable<object?[]> GetTests() => MatrixTestRunner<NumericTestsDescriptor>.GetTests();
     }
 
+    public class SegmentTestsDescriptor : IMatrixTestDescriptor
+    {
+        // https://app.configcat.com/08d5a03c-feb7-af1e-a1fa-40b3329f8bed/08d9f207-6883-43e5-868c-cbf677af3fe6/244cf8b0-f604-11e8-b543-f23c917f9d8d
+        public ConfigLocation ConfigLocation => new ConfigLocation.Cdn("PKDVCLf-Hq-h-kCzMp-L7Q/LcYz135LE0qbcacz2mgXnA");
+        public string MatrixResultFileName => "testmatrix_segments_old.csv";
+        public static IEnumerable<object?[]> GetTests() => MatrixTestRunner<SegmentTestsDescriptor>.GetTests();
+    }
+
     public class SemanticVersionTestsDescriptor : IMatrixTestDescriptor
     {
         // https://app.configcat.com/08d5a03c-feb7-af1e-a1fa-40b3329f8bed/08d745f1-f315-7daf-d163-5541d3786e6f/244cf8b0-f604-11e8-b543-f23c917f9d8d
@@ -75,6 +83,15 @@ public void NumericTests(string configLocation, string settingKey, string expect
             userId, userEmail, userCountry, userCustomAttributeName, userCustomAttributeValue);
     }
 
+    [DataTestMethod]
+    [DynamicData(nameof(SegmentTestsDescriptor.GetTests), typeof(SegmentTestsDescriptor), DynamicDataSourceType.Method)]
+    public void SegmentTests(string configLocation, string settingKey, string expectedReturnValue,
+        string? userId, string? userEmail, string? userCountry, string? userCustomAttributeName, string? userCustomAttributeValue)
+    {
+        MatrixTestRunner<SegmentTestsDescriptor>.Default.RunTest(this.configEvaluator, this.logger, settingKey, expectedReturnValue,
+            userId, userEmail, userCountry, userCustomAttributeName, userCustomAttributeValue);
+    }
+
     [DataTestMethod]
     [DynamicData(nameof(SemanticVersionTestsDescriptor.GetTests), typeof(SemanticVersionTestsDescriptor), DynamicDataSourceType.Method)]
     public void SemanticVersionTests(string configLocation, string settingKey, string expectedReturnValue,

diff --git a/src/ConfigCat.Client.Tests/ConfigV2EvaluationTests.cs b/src/ConfigCat.Client.Tests/ConfigV2EvaluationTests.cs
@@ -30,6 +30,14 @@ public class NumericTestsDescriptor : IMatrixTestDescriptor
         public static IEnumerable<object?[]> GetTests() => MatrixTestRunner<NumericTestsDescriptor>.GetTests();
     }
 
+    public class SegmentTestsDescriptor : IMatrixTestDescriptor
+    {
+        // https://app.configcat.com/v2/e7a75611-4256-49a5-9320-ce158755e3ba/08d5a03c-feb7-af1e-a1fa-40b3329f8bed/08dbd6ca-a85f-4ed0-888a-2da18def92b5/244cf8b0-f604-11e8-b543-f23c917f9d8d
+        public ConfigLocation ConfigLocation => new ConfigLocation.Cdn("configcat-sdk-1/PKDVCLf-Hq-h-kCzMp-L7Q/y_ZB7o-Xb0Swxth-ZlMSeA");
+        public string MatrixResultFileName => "testmatrix_segments_old.csv";
+        public static IEnumerable<object?[]> GetTests() => MatrixTestRunner<SegmentTestsDescriptor>.GetTests();
+    }
+
     public class SemanticVersionTestsDescriptor : IMatrixTestDescriptor
     {
         // https://app.configcat.com/v2/e7a75611-4256-49a5-9320-ce158755e3ba/08d5a03c-feb7-af1e-a1fa-40b3329f8bed/08dbc4dc-278c-4f83-8d36-db73ad6e2a3a/244cf8b0-f604-11e8-b543-f23c917f9d8d
@@ -122,6 +130,15 @@ public void NumericTests(string configLocation, string settingKey, string expect
             userId, userEmail, userCountry, userCustomAttributeName, userCustomAttributeValue);
     }
 
+    [DataTestMethod]
+    [DynamicData(nameof(SegmentTestsDescriptor.GetTests), typeof(SegmentTestsDescriptor), DynamicDataSourceType.Method)]
+    public void SegmentTests(string configLocation, string settingKey, string expectedReturnValue,
+        string? userId, string? userEmail, string? userCountry, string? userCustomAttributeName, string? userCustomAttributeValue)
+    {
+        MatrixTestRunner<SegmentTestsDescriptor>.Default.RunTest(this.configEvaluator, this.logger, settingKey, expectedReturnValue,
+            userId, userEmail, userCountry, userCustomAttributeName, userCustomAttributeValue);
+    }
+
     [DataTestMethod]
     [DynamicData(nameof(SemanticVersionTestsDescriptor.GetTests), typeof(SemanticVersionTestsDescriptor), DynamicDataSourceType.Method)]
     public void SemanticVersionTests(string configLocation, string settingKey, string expectedReturnValue,

diff --git a/src/ConfigCat.Client.Tests/data/evaluationlog/segment.json b/src/ConfigCat.Client.Tests/data/evaluationlog/segment.json
@@ -8,6 +8,15 @@
       "returnValue": false,
       "expectedLog": "segment_no_user.txt"
     },
+    {
+      "key": "featureWithNegatedSegmentTargetingCleartext",
+      "defaultValue": false,
+      "user": {
+        "Identifier": "12345"
+      },
+      "returnValue": false,
+      "expectedLog": "segment_no_targeted_attribute.txt"
+    },
     {
       "key": "featureWithSegmentTargeting",
       "defaultValue": false,
@@ -18,7 +27,7 @@
       "returnValue": true,
       "expectedLog": "segment_matching.txt"
     },
-        {
+    {
       "key": "featureWithNegatedSegmentTargeting",
       "defaultValue": false,
       "user": {

diff --git a/src/ConfigCat.Client.Tests/data/evaluationlog/segment/segment_no_targeted_attribute.txt b/src/ConfigCat.Client.Tests/data/evaluationlog/segment/segment_no_targeted_attribute.txt
@@ -0,0 +1,13 @@
+WARNING [3003] Cannot evaluate condition (User.Email IS ONE OF ['[email protected]', '[email protected]']) for setting 'featureWithNegatedSegmentTargetingCleartext' (the User.Email attribute is missing). You should set the User.Email attribute in order to make targeting work properly. Read more: https://configcat.com/docs/advanced/user-object/
+INFO [5000] Evaluating 'featureWithNegatedSegmentTargetingCleartext' for User '{"Identifier":"12345"}'
+  Evaluating targeting rules and applying the first match if any:
+  - IF User IS NOT IN SEGMENT 'Beta users (cleartext)'
+    (
+      Evaluating segment 'Beta users (cleartext)':
+      - IF User.Email IS ONE OF ['[email protected]', '[email protected]'] => false, skipping the remaining AND conditions
+      Segment evaluation result: cannot evaluate, the User.Email attribute is missing.
+      Condition (User IS NOT IN SEGMENT 'Beta users (cleartext)') failed to evaluate.
+    )
+    THEN 'True' => cannot evaluate, the User.Email attribute is missing
+    The current targeting rule is ignored and the evaluation continues with the next rule.
+  Returning 'False'.
diff --git a/src/ConfigCat.Client.Tests/data/testmatrix_segments_old.csv b/src/ConfigCat.Client.Tests/data/testmatrix_segments_old.csv
@@ -0,0 +1,6 @@
+Identifier;Email;Country;Custom1;featureWithSegmentTargeting;featureWithSegmentTargetingCleartext;featureWithNegatedSegmentTargeting;featureWithNegatedSegmentTargetingCleartext;featureWithSegmentTargetingInverse;featureWithSegmentTargetingInverseCleartext;featureWithNegatedSegmentTargetingInverse;featureWithNegatedSegmentTargetingInverseCleartext
+##null##;;;;False;False;False;False;False;False;False;False
+;;;;False;False;False;False;False;False;False;False
+[email protected];[email protected];##null##;##null##;True;True;False;False;False;False;True;True
+[email protected];[email protected];##null##;##null##;True;True;False;False;False;False;True;True
+[email protected];[email protected];##null##;##null##;False;False;True;True;True;True;False;False
diff --git a/src/ConfigCatClient/Evaluation/RolloutEvaluator.cs b/src/ConfigCatClient/Evaluation/RolloutEvaluator.cs
@@ -127,16 +127,16 @@ private bool TryEvaluateTargetingRules(TargetingRule[] targetingRules, ref Evalu
             var targetingRule = targetingRules[i];
             var conditions = targetingRule.Conditions;
 
-            if (!TryEvaluateConditions(conditions, targetingRule, contextSalt: context.Key, ref context, out var isMatch))
-            {
-                logBuilder?
-                    .IncreaseIndent()
-                    .NewLine(TargetingRuleIgnoredMessage)
-                    .DecreaseIndent();
-                continue;
-            }
-            else if (!isMatch)
+            var isMatch = EvaluateConditions(conditions, targetingRule, contextSalt: context.Key, ref context, out var error);
+            if (!isMatch)
             {
+                if (error is not null)
+                {
+                    logBuilder?
+                        .IncreaseIndent()
+                        .NewLine(TargetingRuleIgnoredMessage)
+                        .DecreaseIndent();
+                }
                 continue;
             }
 
@@ -244,13 +244,13 @@ private bool TryEvaluatePercentageOptions(PercentageOption[] percentageOptions,
         throw new InvalidOperationException("Sum of percentage option percentages are less than 100.");
     }
 
-    private bool TryEvaluateConditions<TCondition>(TCondition[] conditions, TargetingRule? targetingRule, string contextSalt, ref EvaluateContext context, out bool result)
+    private bool EvaluateConditions<TCondition>(TCondition[] conditions, TargetingRule? targetingRule, string contextSalt, ref EvaluateContext context, out string? error)
         where TCondition : IConditionProvider
     {
-        result = true;
+        error = null;
+        var result = true;
 
         var logBuilder = context.LogBuilder;
-        string? error = null;
         var newLineBeforeThen = false;
 
         logBuilder?.NewLine("- ");
@@ -286,12 +286,12 @@ private bool TryEvaluateConditions<TCondition>(TCondition[] conditions, Targetin
 
                 case PrerequisiteFlagCondition prerequisiteFlagCondition:
                     conditionResult = EvaluatePrerequisiteFlagCondition(prerequisiteFlagCondition, ref context, out error);
-                    newLineBeforeThen = error is null || conditions.Length > 1;
+                    newLineBeforeThen = error is null || error != CircularDependencyError || conditions.Length > 1;
                     break;
 
                 case SegmentCondition segmentCondition:
                     conditionResult = EvaluateSegmentCondition(segmentCondition, ref context, out error);
-                    newLineBeforeThen = error is null || conditions.Length > 1;
+                    newLineBeforeThen = error is null || error != MissingUserObjectError || conditions.Length > 1;
                     break;
 
                 default:
@@ -321,7 +321,7 @@ private bool TryEvaluateConditions<TCondition>(TCondition[] conditions, Targetin
             logBuilder?.AppendTargetingRuleConsequence(targetingRule, error, result, newLineBeforeThen);
         }
 
-        return error is null;
+        return result;
     }
 
     private bool EvaluateUserCondition(UserCondition condition, string contextSalt, ref EvaluateContext context, out string? error)
@@ -804,23 +804,34 @@ private bool EvaluateSegmentCondition(SegmentCondition condition, ref EvaluateCo
             .IncreaseIndent()
             .NewLine().Append($"Evaluating segment '{segment.Name}':");
 
-        TryEvaluateConditions(segment.Conditions, targetingRule: null, contextSalt: segment.Name, ref context, out var segmentResult);
+        var segmentResult = EvaluateConditions(segment.Conditions, targetingRule: null, contextSalt: segment.Name, ref context, out error);
 
         var comparator = condition.Comparator;
-        var result = comparator switch
+        var result = error is null && comparator switch
         {
             SegmentComparator.IsIn => segmentResult,
             SegmentComparator.IsNotIn => !segmentResult,
             _ => throw new InvalidOperationException("Comparison operator is invalid.")
         };
 
-        logBuilder?
-            .NewLine().Append($"Segment evaluation result: User {(segmentResult ? SegmentComparator.IsIn : SegmentComparator.IsNotIn).ToDisplayText()}.")
-            .NewLine("Condition (")
-                .AppendSegmentCondition(condition)
-                .Append(") evaluates to ").AppendEvaluationResult(result).Append(".")
-            .DecreaseIndent()
-            .NewLine(")");
+        if (logBuilder is not null)
+        {
+            logBuilder.NewLine("Segment evaluation result: ");
+            (error is null
+                ? logBuilder.Append($"User {(segmentResult ? SegmentComparator.IsIn : SegmentComparator.IsNotIn).ToDisplayText()}")
+                : logBuilder.Append(error))
+                .Append(".");
+
+            logBuilder.NewLine("Condition (").AppendSegmentCondition(condition).Append(")");
+            (error is null
+                ? logBuilder.Append(" evaluates to ").AppendEvaluationResult(result)
+                : logBuilder.Append(" failed to evaluate"))
+                .Append(".");
+
+            logBuilder
+                .DecreaseIndent()
+                .NewLine(")");
+        }
 
         return result;
     }