Skip to content

Commit

Permalink
Reduce Exceptions in the ConditionEvaluator (#48)
Browse files Browse the repository at this point in the history 
* 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 <[email protected]>
  • Loading branch information
@nnusse @Bohdan-Kim
nnusse and Bohdan-Kim authored May 22, 2024
1 parent fff17ac commit b5a2402
Show file tree
Hide file tree
Showing 3 changed files with 216 additions and 39 deletions.
47 changes: 31 additions & 16 deletions lib/src/main/java/growthbook/sdk/java/ConditionEvaluator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,8 @@ Boolean evalOperatorCondition(String operatorString, @Nullable JsonElement actua
ArrayList<Boolean> conditionsList = jsonUtils.gson.fromJson(expected, listType);
return conditionsList.contains(value);
}
break;


case NIN:
if (actual == null) return false;
Expand Down Expand Up @@ -258,32 +260,39 @@ Boolean evalOperatorCondition(String operatorString, @Nullable JsonElement actua
ArrayList<Boolean> 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();
}
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();
}
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();
Expand All @@ -294,20 +303,23 @@ 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();
}
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());

Expand All @@ -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:
Expand Down Expand Up @@ -351,6 +364,7 @@ Boolean evalOperatorCondition(String operatorString, @Nullable JsonElement actua
}
if (!passed) return false;
}
return true;

case NOT:
return !evalConditionValue(expected, actual);
Expand All @@ -370,44 +384,45 @@ 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;

default:
return false;
}
return false;
}

/**
Expand Down
86 changes: 63 additions & 23 deletions lib/src/test/java/growthbook/sdk/java/ConditionEvaluatorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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() {
Expand All @@ -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);

Expand Down Expand Up @@ -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();
}
}
Loading

0 comments on commit b5a2402

Please sign in to comment.