Skip to content

Commit

Permalink
Fix short circuiting of boolean expressions (#7060)
Browse files Browse the repository at this point in the history
Boolean expressions with AND (&&) or OR (||) should short circuit
as expected for any languages. It helps to support correctly
expressions like
`isDefined(@exception) && contains(@exception.detailMessage, 'closed')`
We are just delegating to the operator evaluation the evaluation of
the left and right instead of evaluating operands before the operator.
  • Loading branch information
jpbempel committed May 22, 2024
1 parent c126d0c commit 187601c
Show file tree
Hide file tree
Showing 4 changed files with 57 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ public BinaryExpression(

@Override
public Boolean evaluate(ValueReferenceResolver valueRefResolver) {
return operator.apply(left.evaluate(valueRefResolver), right.evaluate(valueRefResolver));
return operator.apply(left, right, valueRefResolver);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
@@ -1,18 +1,21 @@
package com.datadog.debugger.el.expressions;

import com.datadog.debugger.el.Visitor;
import datadog.trace.bootstrap.debugger.el.ValueReferenceResolver;

public enum BinaryOperator {
AND("&&") {
@Override
public Boolean apply(Boolean left, Boolean right) {
return left && right;
public Boolean apply(
BooleanExpression left, BooleanExpression right, ValueReferenceResolver resolver) {
return left.evaluate(resolver) && right.evaluate(resolver);
}
},
OR("||") {
@Override
public Boolean apply(Boolean left, Boolean right) {
return left || right;
public Boolean apply(
BooleanExpression left, BooleanExpression right, ValueReferenceResolver resolver) {
return left.evaluate(resolver) || right.evaluate(resolver);
}
};

Expand All @@ -22,7 +25,8 @@ public Boolean apply(Boolean left, Boolean right) {
this.symbol = symbol;
}

public abstract Boolean apply(Boolean left, Boolean right);
public abstract Boolean apply(
BooleanExpression left, BooleanExpression right, ValueReferenceResolver resolver);

public <R> R accept(Visitor<R> visitor) {
return visitor.visit(this);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@
import static com.datadog.debugger.el.PrettyPrintVisitor.print;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;

import com.datadog.debugger.el.RefResolverHelper;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;

class BinaryExpressionTest {
Expand All @@ -23,4 +25,26 @@ void testRightNull() {
assertFalse(expression.evaluate(RefResolverHelper.createResolver(this)));
assertEquals("true && false", print(expression));
}

@Test
void testShortCircuitAnd() {
BinaryExpression expression =
new BinaryExpression(
BooleanExpression.FALSE,
valueRefResolver -> Assertions.fail("should not reach"),
BinaryOperator.AND);
assertFalse(expression.evaluate(RefResolverHelper.createResolver(this)));
assertEquals("false && null", print(expression));
}

@Test
void testShortCircuitOr() {
BinaryExpression expression =
new BinaryExpression(
BooleanExpression.TRUE,
valueRefResolver -> Assertions.fail("should not reach"),
BinaryOperator.OR);
assertTrue(expression.evaluate(RefResolverHelper.createResolver(this)));
assertEquals("true || null", print(expression));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -1121,6 +1121,29 @@ public void nullCondition() throws IOException, URISyntaxException {
"Cannot dereference to field: fld", evaluationErrors.get(0).getMessage());
}

@Test
public void shortCircuitingCondition() throws IOException, URISyntaxException {
final String CLASS_NAME = "CapturedSnapshot08";
LogProbe logProbes =
createProbeBuilder(PROBE_ID, CLASS_NAME, "doit", "int (java.lang.String)")
.when(
new ProbeCondition(
DSL.when(
DSL.and(
DSL.isDefined(DSL.ref("@exception")),
DSL.contains(
DSL.getMember(DSL.ref("@exception"), "detailMessage"),
new StringValue("closed")))),
"isDefined(@exception) && contains(@exception.detailMessage, 'closed')"))
.build();
TestSnapshotListener listener = installProbes(CLASS_NAME, logProbes);
Class<?> testClass = compileAndLoadClass(CLASS_NAME);
int result = Reflect.onClass(testClass).call("main", "1").get();
assertEquals(3, result);
// no snapshot, no eval error, isDefined returns false and do not evaluate the second part
assertEquals(0, listener.snapshots.size());
}

@Test
public void wellKnownClassesCondition() throws IOException, URISyntaxException {
final String CLASS_NAME = "CapturedSnapshot08";
Expand Down

0 comments on commit 187601c

Please sign in to comment.