From b5a2402f1ac6674c02b1e778b815b20415faea48 Mon Sep 17 00:00:00 2001 From: Norm <88192291+nnusse@users.noreply.github.com> Date: Wed, 22 May 2024 20:10:06 +0200 Subject: [PATCH] Reduce Exceptions in the ConditionEvaluator (#48) * reduce amount of exceptions based on serialized null values and add break statements for most of the switch-cases. * add new test cases and ensure that unexpected exceptions let the tests fail. * trial commit * undo trial commit * Add attributeDataType checking for NULL. * adjust test definition and evaluation implementation. --------- Co-authored-by: Bohdan Akimenko --- .../sdk/java/ConditionEvaluator.java | 47 ++++--- .../sdk/java/ConditionEvaluatorTest.java | 86 ++++++++---- lib/src/test/resources/test-cases.json | 122 ++++++++++++++++++ 3 files changed, 216 insertions(+), 39 deletions(-) diff --git a/lib/src/main/java/growthbook/sdk/java/ConditionEvaluator.java b/lib/src/main/java/growthbook/sdk/java/ConditionEvaluator.java index e4fa3657..f4306292 100644 --- a/lib/src/main/java/growthbook/sdk/java/ConditionEvaluator.java +++ b/lib/src/main/java/growthbook/sdk/java/ConditionEvaluator.java @@ -225,6 +225,8 @@ Boolean evalOperatorCondition(String operatorString, @Nullable JsonElement actua ArrayList conditionsList = jsonUtils.gson.fromJson(expected, listType); return conditionsList.contains(value); } + break; + case NIN: if (actual == null) return false; @@ -258,10 +260,13 @@ Boolean evalOperatorCondition(String operatorString, @Nullable JsonElement actua ArrayList conditionsList = jsonUtils.gson.fromJson(expected, listType); return !conditionsList.contains(value); } + break; + case GT: - if (actual == null) { - return 0.0 > expected.getAsDouble(); + if (actual == null || DataType.NULL.equals(attributeDataType)) { + return (!expected.isJsonPrimitive() || expected.getAsJsonPrimitive().isNumber()) + && 0.0 > expected.getAsDouble(); } if (actual.getAsJsonPrimitive().isNumber()) { return actual.getAsNumber().floatValue() > expected.getAsNumber().floatValue(); @@ -269,10 +274,12 @@ Boolean evalOperatorCondition(String operatorString, @Nullable JsonElement actua if (actual.getAsJsonPrimitive().isString()) { return actual.getAsString().compareTo(expected.getAsString()) > 0; } + break; case GTE: - if (actual == null) { - return 0.0 >= expected.getAsDouble(); + if (actual == null || DataType.NULL.equals(attributeDataType)) { + return (!expected.isJsonPrimitive() || expected.getAsJsonPrimitive().isNumber()) + && 0.0 >= expected.getAsDouble(); } if (actual.getAsJsonPrimitive().isNumber()) { return actual.getAsNumber().floatValue() >= expected.getAsNumber().floatValue(); @@ -280,10 +287,12 @@ Boolean evalOperatorCondition(String operatorString, @Nullable JsonElement actua if (actual.getAsJsonPrimitive().isString()) { return actual.getAsString().compareTo(expected.getAsString()) >= 0; } + break; case LT: - if (actual == null) { - return 0.0 < expected.getAsDouble(); + if (actual == null || DataType.NULL.equals(attributeDataType)) { + return (!expected.isJsonPrimitive() || expected.getAsJsonPrimitive().isNumber()) + && 0.0 < expected.getAsDouble(); } if (actual.getAsString().toLowerCase().matches("\\d+")) { return Double.parseDouble(actual.getAsString()) < expected.getAsDouble(); @@ -294,10 +303,12 @@ Boolean evalOperatorCondition(String operatorString, @Nullable JsonElement actua if (actual.getAsJsonPrimitive().isString()) { return actual.getAsString().compareTo(expected.getAsString()) < 0; } + break; case LTE: - if (actual == null) { - return 0.0 <= expected.getAsDouble(); + if (actual == null || DataType.NULL.equals(attributeDataType)) { + return (!expected.isJsonPrimitive() || expected.getAsJsonPrimitive().isNumber()) + && 0.0 <= expected.getAsDouble(); } if (actual.getAsJsonPrimitive().isNumber()) { return actual.getAsNumber().floatValue() <= expected.getAsNumber().floatValue(); @@ -305,9 +316,10 @@ Boolean evalOperatorCondition(String operatorString, @Nullable JsonElement actua if (actual.getAsJsonPrimitive().isString()) { return actual.getAsString().compareTo(expected.getAsString()) <= 0; } + break; case REGEX: - if (actual == null) return false; + if (actual == null || DataType.NULL.equals(attributeDataType)) return false; Pattern pattern = Pattern.compile(expected.getAsString()); Matcher matcher = pattern.matcher(actual.getAsString()); @@ -320,10 +332,11 @@ Boolean evalOperatorCondition(String operatorString, @Nullable JsonElement actua return matches; case NE: + if (DataType.NULL.equals(attributeDataType)) return false; return !Objects.equals(actual, expected); case EQ: - if (actual == null) return false; + if (actual == null || DataType.NULL.equals(attributeDataType)) return false; return arePrimitivesEqual(actual.getAsJsonPrimitive(), expected.getAsJsonPrimitive(), attributeDataType); case SIZE: @@ -351,6 +364,7 @@ Boolean evalOperatorCondition(String operatorString, @Nullable JsonElement actua } if (!passed) return false; } + return true; case NOT: return !evalConditionValue(expected, actual); @@ -370,37 +384,37 @@ Boolean evalOperatorCondition(String operatorString, @Nullable JsonElement actua } case VERSION_GT: - if (actual == null || expected == null) return false; + if (actual == null || expected == null || DataType.NULL.equals(attributeDataType)) return false; return StringUtils.paddedVersionString(actual.getAsString()) .compareTo(StringUtils.paddedVersionString(expected.getAsString())) > 0; case VERSION_GTE: - if (actual == null || expected == null) return false; + if (actual == null || expected == null || DataType.NULL.equals(attributeDataType)) return false; return StringUtils.paddedVersionString(actual.getAsString()) .compareTo(StringUtils.paddedVersionString(expected.getAsString())) >= 0; case VERSION_LT: - if (actual == null || expected == null) return false; + if (actual == null || expected == null || DataType.NULL.equals(attributeDataType)) return false; return StringUtils.paddedVersionString(actual.getAsString()) .compareTo(StringUtils.paddedVersionString(expected.getAsString())) < 0; case VERSION_LTE: - if (actual == null || expected == null) return false; + if (actual == null || expected == null || DataType.NULL.equals(attributeDataType)) return false; return StringUtils.paddedVersionString(actual.getAsString()) .compareTo(StringUtils.paddedVersionString(expected.getAsString())) <= 0; case VERSION_NE: - if (actual == null || expected == null) return false; + if (actual == null || expected == null || DataType.NULL.equals(attributeDataType)) return false; return StringUtils.paddedVersionString(actual.getAsString()) .compareTo(StringUtils.paddedVersionString(expected.getAsString())) != 0; case VERSION_EQ: - if (actual == null || expected == null) return false; + if (actual == null || expected == null || DataType.NULL.equals(attributeDataType)) return false; return StringUtils.paddedVersionString(actual.getAsString()) .compareTo(StringUtils.paddedVersionString(expected.getAsString())) == 0; @@ -408,6 +422,7 @@ Boolean evalOperatorCondition(String operatorString, @Nullable JsonElement actua default: return false; } + return false; } /** diff --git a/lib/src/test/java/growthbook/sdk/java/ConditionEvaluatorTest.java b/lib/src/test/java/growthbook/sdk/java/ConditionEvaluatorTest.java index 1051baee..ad5406e5 100644 --- a/lib/src/test/java/growthbook/sdk/java/ConditionEvaluatorTest.java +++ b/lib/src/test/java/growthbook/sdk/java/ConditionEvaluatorTest.java @@ -4,6 +4,10 @@ import com.google.gson.JsonElement; import com.google.gson.JsonObject; import growthbook.sdk.java.testhelpers.TestCasesJsonHelper; +import java.io.ByteArrayOutputStream; +import java.io.PrintStream; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import java.util.ArrayList; @@ -13,6 +17,24 @@ class ConditionEvaluatorTest { final TestCasesJsonHelper helper = TestCasesJsonHelper.getInstance(); + final PrintStream originalErrorOutputStream = System.err; + final ByteArrayOutputStream errContent = new ByteArrayOutputStream(); + + static final String[] expectedExceptionStrings = { + "Expected BEGIN_ARRAY but was NUMBER at path $", + "java.util.regex.PatternSyntaxException: Dangling meta character '?' near index 3" + }; + + @BeforeEach + public void setUpErrorStream() { + System.setErr(new PrintStream(errContent)); + } + + @AfterEach + public void restoreErrorStreams() { + System.setErr(originalErrorOutputStream); + } + @Test void test_evaluateCondition_returnsFalseIfWrongShape() { @@ -36,31 +58,32 @@ void test_evaluateCondition_testCases() { JsonArray testCases = helper.evalConditionTestCases(); for (int i = 0; i < testCases.size(); i++) { + resetErrorOutputStream(); // Run only test at index i -// if (i == 71) { - JsonElement jsonElement = testCases.get(i); - JsonArray testCase = (JsonArray) jsonElement; - - String testDescription = testCase.get(0).getAsString(); - - // Get attributes and conditions as JSON objects then convert them to a JSON string - String condition = testCase.get(1).getAsJsonObject().toString(); - String attributes = testCase.get(2).getAsJsonObject().toString(); - - boolean expected = testCase.get(3).getAsBoolean(); - - if (expected == evaluator.evaluateCondition(attributes, condition)) { - passedTests.add(testDescription); - } else { - failingIndexes.add(i); - failedTests.add(testDescription); - } - -// } - + JsonElement jsonElement = testCases.get(i); + JsonArray testCase = (JsonArray) jsonElement; + String testDescription = testCase.get(0).getAsString(); + + // Get attributes and conditions as JSON objects then convert them to a JSON string + String condition = testCase.get(1).getAsJsonObject().toString(); + String attributes = testCase.get(2).getAsJsonObject().toString(); + boolean expected = testCase.get(3).getAsBoolean(); + + boolean evaluationResult = evaluator.evaluateCondition(attributes, condition); + + if (unexpectedExceptionOccurred(errContent.toString())) { + failingIndexes.add(i); + failedTests.add(String.format("Unexpected Exception: %s", testDescription)); + continue; + } + + if (expected == evaluationResult) { + passedTests.add(testDescription); + } else { + failingIndexes.add(i); + failedTests.add(testDescription); + } } - -// System.out.printf("\n\nPassed tests: %s", passedTests); System.out.printf("\n\n\nFailed tests = %s / %s . Failing = %s", failedTests.size(), testCases.size(), failedTests); System.out.printf("\n\n\nFailing indexes = %s", failingIndexes); @@ -146,4 +169,21 @@ void test_paddedVersionString_gt() { assertEquals(paddedVersion.compareTo(paddedOther) > 0, equals); } } + + private boolean unexpectedExceptionOccurred(String stacktrace) { + if (stacktrace.isEmpty()) { + return false; + } + for (String expectedExceptionSubString : expectedExceptionStrings) { + if (stacktrace.contains(expectedExceptionSubString)) { + return false; + } + } + System.out.println(stacktrace.toString()); + return true; + } + + private void resetErrorOutputStream() { + errContent.reset(); + } } diff --git a/lib/src/test/resources/test-cases.json b/lib/src/test/resources/test-cases.json index 643b57c4..93b32b48 100644 --- a/lib/src/test/resources/test-cases.json +++ b/lib/src/test/resources/test-cases.json @@ -1481,6 +1481,7 @@ ["$in - pass", {"num": {"$in": [1, 2, 3]}}, {"num": 2}, true], ["$in - fail", {"num": {"$in": [1, 2, 3]}}, {"num": 4}, false], ["$in - not array", {"num": {"$in": 1}}, {"num": 1}, false], + ["$in - fail with null value", {"num": {"$in": [1, 2, 3]}}, {"num": null}, false], [ "$in - array pass 1", {"tags": {"$in": ["a", "b"]}}, @@ -1509,6 +1510,7 @@ ["$nin - pass", {"num": {"$nin": [1, 2, 3]}}, {"num": 4}, true], ["$nin - fail", {"num": {"$nin": [1, 2, 3]}}, {"num": 2}, false], ["$nin - not array", {"num": {"$nin": 1}}, {"num": 1}, false], + ["$nin - fail with null value", {"num": {"$in": [1, 2, 3]}}, {"num": null}, false], [ "$nin - array fail 1", {"tags": {"$nin": ["a", "b"]}}, @@ -1540,12 +1542,24 @@ {"nums": [0, 5, -20, 15]}, true ], + [ + "$elemMatch - pass - flat arrays with null value", + {"nums": {"$elemMatch": {"$gt": 10}}}, + {"nums": [0, null, -20, 15]}, + true + ], [ "$elemMatch - fail - flat arrays", {"nums": {"$elemMatch": {"$gt": 10}}}, {"nums": [0, 5, -20, 8]}, false ], + [ + "$elemMatch - fail - flat arrays with null value", + {"nums": {"$elemMatch": {"$gt": 10}}}, + {"nums": [0, 5, -20, null]}, + false + ], [ "missing attribute - fail", {"pets.dog.name": {"$in": ["fido"]}}, @@ -1571,8 +1585,11 @@ true ], ["empty $or - pass", {"$or": []}, {"hello": "world"}, true], + ["empty $or - pass with null value", {"$or": []}, {"hello": null}, true], ["empty $and - pass", {"$and": []}, {"hello": "world"}, true], + ["empty $and - pass with null value", {"$and": []}, {"hello": null}, true], ["empty - pass", {}, {"hello": "world"}, true], + ["empty - pass with null value", {}, {"hello": null}, true], [ "$eq - pass", {"occupation": {"$eq": "engineer"}}, @@ -1585,8 +1602,15 @@ {"occupation": "civil engineer"}, false ], + [ + "$eq - fail with null value", + {"occupation": {"$eq": "engineer"}}, + {"occupation": null}, + false + ], ["$ne - pass", {"level": {"$ne": "senior"}}, {"level": "junior"}, true], ["$ne - fail", {"level": {"$ne": "senior"}}, {"level": "senior"}, false], + ["$ne - fail with null value", {"level": {"$ne": "senior"}}, {"level": null}, false], [ "$regex - pass", {"userAgent": {"$regex": "(Mobile|Tablet)"}}, @@ -1599,6 +1623,12 @@ {"userAgent": "Chrome Desktop Browser"}, false ], + [ + "$regex - fail with null value", + {"userAgent": {"$regex": "(Mobile|Tablet)"}}, + {"userAgent": null}, + false + ], [ "$gt/$lt numbers - pass", {"age": {"$gt": 30, "$lt": 60}}, @@ -1611,6 +1641,12 @@ {"age": 60}, false ], + [ + "$gt/$lt numbers - fail with null value", + {"age": {"$gt": 30, "$lt": 60}}, + {"age": null}, + false + ], [ "$gt/$lt numbers - fail $gt", {"age": {"$gt": 30, "$lt": 60}}, @@ -1641,6 +1677,12 @@ {"age": 61}, false ], + [ + "$gte/$lte numbers - fail with null value ", + {"age": {"$gte": 30, "$lte": 60}}, + {"age": null}, + false + ], [ "$gte/$lte numbers - fail $gte", {"age": {"$gt": 30, "$lt": 60}}, @@ -1653,6 +1695,12 @@ {"word": "alphabet"}, false ], + [ + "$gt/$lt string - fail with null value", + {"word": {"$gt": "alphabet", "$lt": "zebra"}}, + {"word": null}, + false + ], [ "$gt/$lt strings - fail $lt", {"word": {"$gt": "alphabet", "$lt": "zebra"}}, @@ -1680,12 +1728,15 @@ ], ["$type string - pass", {"a": {"$type": "string"}}, {"a": "a"}, true], ["$type string - fail", {"a": {"$type": "string"}}, {"a": 1}, false], + ["$type string - fail with null value", {"a": {"$type": "string"}}, {"a": null}, false], ["$type null - pass", {"a": {"$type": "null"}}, {"a": null}, true], ["$type null - fail", {"a": {"$type": "null"}}, {"a": 1}, false], ["$type boolean - pass", {"a": {"$type": "boolean"}}, {"a": false}, true], ["$type boolean - fail", {"a": {"$type": "boolean"}}, {"a": 1}, false], + ["$type boolean - fail with null value", {"a": {"$type": "boolean"}}, {"a": null}, false], ["$type number - pass", {"a": {"$type": "number"}}, {"a": 1}, true], ["$type number - fail", {"a": {"$type": "number"}}, {"a": "a"}, false], + ["$type number - fail with null value", {"a": {"$type": "number"}}, {"a": null}, false], [ "$type object - pass", {"a": {"$type": "object"}}, @@ -1693,8 +1744,10 @@ true ], ["$type object - fail", {"a": {"$type": "object"}}, {"a": 1}, false], + ["$type object - fail with null value", {"a": {"$type": "object"}}, {"a": null}, false], ["$type array - pass", {"a": {"$type": "array"}}, {"a": [1, 2]}, true], ["$type array - fail", {"a": {"$type": "array"}}, {"a": 1}, false], + ["$type array - fail with null value", {"a": {"$type": "array"}}, {"a": null}, false], [ "unknown operator - pass", {"name": {"$regx": "hello"}}, @@ -1739,6 +1792,12 @@ {"tags": "abc"}, false ], + [ + "$size number - fail with null value", + {"tags": {"$size": 3}}, + {"tags": null}, + false + ], [ "$size nested - pass", {"tags": {"$size": {"$gt": 2}}}, @@ -1751,6 +1810,12 @@ {"tags": [0, 1]}, false ], + [ + "$size nested - fail with null value", + {"tags": {"$size": {"$gt": 2}}}, + {"tags": null}, + false + ], [ "$size nested - fail less than", {"tags": {"$size": {"$gt": 2}}}, @@ -1769,6 +1834,13 @@ {"tags": ["foo", "baz"]}, false ], + [ + "$elemMatch contains - false with null value", + {"tags": {"$elemMatch": {"$eq": "bar"}}}, + {"tags": null}, + false + ], + [ "$elemMatch intersection - pass", {"tags": {"$elemMatch": {"$in": ["a", "b"]}}}, @@ -1811,6 +1883,12 @@ {"hobbies": [{"name": "bowling"}, {"name": "tennis"}]}, false ], + [ + "$elemMatch nested - fail with null value", + {"hobbies": {"$elemMatch": {"name": {"$regex": "^ping"}}}}, + {"hobbies": [{"name": null}, {"name": null}]}, + false + ], [ "$elemMatch nested - fail not array", {"hobbies": {"$elemMatch": {"name": {"$regex": "^ping"}}}}, @@ -1823,6 +1901,12 @@ {"name": "world"}, true ], + [ + "$not - pass with null value", + {"name": {"$not": {"$regex": "^hello"}}}, + {"name": null}, + true + ], [ "$not - fail", {"name": {"$not": {"$regex": "^hello"}}}, @@ -1847,12 +1931,30 @@ {"tags": "hello"}, false ], + [ + "$all - fail null value", + {"tags": {"$all": ["one", "three"]}}, + {"tags": null}, + false + ], [ "$nor - pass", {"$nor": [{"name": "john"}, {"age": {"$lt": 30}}]}, {"name": "jim", "age": 40}, true ], + [ + "$nor - pass with null value for name", + {"$nor": [{"name": "john"}, {"age": {"$lt": 30}}]}, + {"name": null, "age": 40}, + true + ], + [ + "$nor - pass with null value for age", + {"$nor": [{"name": "john"}, {"age": {"$lt": 30}}]}, + {"name": "jim", "age": null}, + false + ], [ "$nor - fail both", {"$nor": [{"name": "john"}, {"age": {"$lt": 30}}]}, @@ -1889,6 +1991,12 @@ {"tags": ["hello"]}, false ], + [ + "equals array - fail with null value", + {"tags": ["hello", "world"]}, + {"tags": null}, + false + ], [ "equals array - fail extra item", {"tags": ["hello", "world"]}, @@ -1913,6 +2021,12 @@ {"tags": {"hello": "world", "yes": "please"}}, false ], + [ + "equals object - fail with extra property null", + {"tags": {"hello": "world"}}, + {"tags": {"hello": "world", "yes": null}}, + false + ], [ "equals object - fail missing property", {"tags": {"hello": "world"}}, @@ -1951,6 +2065,12 @@ {"version": "10.12.13"}, true ], + [ + "$vgt/$vlt - fail with null value", + {"version": {"$vgt": "9.99.8", "$vlt": "11.0.1"}}, + {"version": null}, + false + ], [ "$vgt/$vlt - pass - minor", {"version": {"$vgt": "10.2.11", "$vlt": "10.20.11"}}, @@ -2084,6 +2204,7 @@ false ], ["$veq - pass", {"v": {"$veq": "1.2.3"}}, {"v": "1.2.3"}, true], + ["$veq - fail with null value", {"v": {"$veq": "1.2.3"}}, {"v": null}, false], [ "$veq - pass (with build)", {"v": {"$veq": "1.2.3"}}, @@ -2091,6 +2212,7 @@ true ], ["$vne - pass", {"v": {"$vne": "1.2.3"}}, {"v": "2.2.3"}, true], + ["$vne - fail with null value", {"v": {"$vne": "1.2.3"}}, {"v": null}, false], [ "$vne - pass (prerelease)", {"v": {"$vne": "1.2.3"}},