Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

PROC-1219: Fix rule return result check so that it supports method calls #99

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

PROC-1219: Fix rule return result check so that it supports method calls #99

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a00e5f4
PROC-1219: Fix rule return result check so that it supports method calls
Apr 6, 2023
5e2c697
PROC-1219: Make method arg final
Apr 7, 2023
9e504ec
PROC-1219: Remove other tomcat7 deps unrelated to EL
Apr 7, 2023
4960d97
PROC-1219: Fix error message in case test fails
Apr 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 0 additions & 10 deletions pom.xml
View file
Open in desktop
Expand Up @@ -401,16 +401,6 @@
<artifactId>tomcat-jasper-el</artifactId>
<version>${tomcat-api7.version}</version>
</dependency>
<dependency>
<groupId>org.apache.tomcat</groupId>
<artifactId>tomcat-jsp-api</artifactId>
<version>${tomcat-api7.version}</version>
</dependency>
<dependency>
<groupId>org.apache.tomcat</groupId>
<artifactId>tomcat-servlet-api</artifactId>
<version>${tomcat-api7.version}</version>
</dependency>
</dependencies>
</dependencyManagement>

Expand Down
41 changes: 8 additions & 33 deletions proctor-common/src/main/java/com/indeed/proctor/common/RuleEvaluator.java
View file
Open in desktop
Expand Up @@ -131,51 +131,26 @@ public boolean evaluateBooleanRule(final String rule, @Nonnull final Map<String,
}

final ELContext elContext = createElContext(values);
final ValueExpression ve = expressionFactory.createValueExpression(elContext, rule, boolean.class);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i tested in local, actually boolean.class still works. ve.getValue(elContext); will return the correct boolean value.
It will fail in checkRuleIsBooleanType, which calls ve.getType, which is removed by your changes.

Therefore is there any other reason that we should change the rule type to String?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason is that we still check the return type (see line 136) and that method now expects a String.
If we are happy with removing the call to checkRuleIsBooleanType we can coerce to boolean instead.

Tbh I don't think there's much value of checking that type is boolean at evaluation time since we already did that when the rule was added on the UI (in RuleVerifyUtils). Was thinking to remove it but we have a test that checks this (TestRuleEvaluator#testNonBooleanShouldFail) and wanted to minimize the changes but happy to do it if we have consensus.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Robert.
What i found is, if we keep checkRuleIsBooleanType the same, changing to string "final ValueExpression ve = expressionFactory.createValueExpression(elContext, rule, String.class);" will still result in property not found in checkRuleIsBooleanType.
javax.el.PropertyNotFoundException: Property [isValid] not found on type [com.indeed.proctor.common.TestRuleEvaluator$Temp]
There i am not sure what's the difference between String and boolean in this case.

At least that's what i saw when i run from unit tests. Do you see different behavior?

Or do you mean Class<?> type = ve.getType(elContext); may not be required, using string and check "true" or "false" is just as reliable?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, changing the coerce type in expressionFactory.createValueExpression(elContext, rule, boolean.class); from boolean to String is not sufficient to avoid the error, that's why I've changed checkRuleIsBooleanType method to no longer call getType and I'm doing the check based upon the string result.

Above I was questioning if we should call checkRuleIsBooleanType at all as part of RuleEvaluator#evaluateBooleanRule method because we should have already done so when the rule added on the UI (see RuleVerifyUtils#verifyRule). We can keep it if we want an extra check or we can simplify to:

  final ValueExpression ve = expressionFactory.createValueExpression(elContext, rule, boolean.class);
  return (boolean) ve.getValue(elContext);

in RuleEvaluator#evaluateBooleanRule and remove the tests that use invalid rules (TestRuleEvaluator#testNonBooleanShouldFail). This is just an alternative, we can leave things as in the PR in interest of time since this is just an optimization.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest i am also trying to learn how does the proctor api validate the rule.
Taking your unit tests for example,

final Map<String, Object> context = ImmutableMap.of("context", new Temp());

The Temp can be any class that you need in your application. How does Proctor Api get access to those custom class?

checkRuleIsBooleanType(rule, elContext, ve);
final ValueExpression ve = expressionFactory.createValueExpression(elContext, rule, String.class);
final String result = (String) ve.getValue(elContext);
checkRuleIsBooleanType(rule, result);

final Object result = ve.getValue(elContext);

if (result instanceof Boolean) {
return ((Boolean) result);
}
// this should never happen, evaluateRule throws ELException when it cannot coerce to Boolean
throw new IllegalArgumentException("Received non-boolean return value: "
+ (result == null ? "null" : result.getClass().getCanonicalName())
+ " from rule " + rule);
return Boolean.parseBoolean(result);
}

/**
* @throws IllegalArgumentException if type of expression is not boolean
*/
static void checkRuleIsBooleanType(final String rule, final ELContext elContext, final ValueExpression ve) {
static void checkRuleIsBooleanType(final String rule, final String value) {
// apache-el is an expression language, not a rule language, and it is very lenient
// sadly that means it will just evaluate to false when users make certain mistakes, e.g. by
// coercing String value "xyz" to boolean false, instead of throwing an exception.
// To support users writing rules, be more strict here in requiring the type of the
// value to be expected before coercion
Class<?> type = ve.getType(elContext);
if (ClassUtils.isPrimitiveWrapper(type)) {
type = ClassUtils.wrapperToPrimitive(type);
if (StringUtils.isBlank(value) || "true".equalsIgnoreCase(value) || "false".equalsIgnoreCase(value)) {
return;
}
// allow null to be coerced for historic reasons
if ((type != null) && (type != boolean.class)) {
throw new IllegalArgumentException("Received non-boolean return value: " + type + " from rule " + rule);
}
}

/**
* @param expectedType class to coerce result to, use primitive instead of wrapper, e.g. boolean.class instead of Boolean.class.
* @return null or a Boolean value representing the expression evaluation result
* @throws RuntimeException: E.g. PropertyNotFound or other ELException when not of expectedType
* @deprecated Use evaluateBooleanRule() instead, it checks against more errors
*/
@CheckForNull
@Deprecated
public Object evaluateRule(final String rule, final Map<String, Object> values, final Class expectedType) {
final ELContext elContext = createElContext(values);
final ValueExpression ve = expressionFactory.createValueExpression(elContext, rule, expectedType);
return ve.getValue(elContext);
throw new IllegalArgumentException("Received non-boolean return value: " + value + " from rule " + rule);
}

}
13 changes: 8 additions & 5 deletions proctor-common/src/main/java/com/indeed/proctor/common/RuleVerifyUtils.java
View file
Open in desktop
Expand Up @@ -37,10 +37,13 @@ public static void verifyRule(
final Set<String> absentIdentifiers
) throws InvalidRuleException {
final String bareRule = removeElExpressionBraces(testRule);
if (!StringUtils.isBlank(bareRule)) {
if (StringUtils.isNotBlank(bareRule)) {
final ValueExpression valueExpression;
try {
valueExpression = expressionFactory.createValueExpression(elContext, testRule, Boolean.class);
// coerce the return value to String in order to determine the return type without having to invoke
// valueExpression.getType(elContext) which does not seem to work with expressions that contain method calls
// e.g. #{foo.bar()} will throw an exception when getType is called
valueExpression = expressionFactory.createValueExpression(elContext, testRule, String.class);
} catch (final ELException e) {
throw new InvalidRuleException(e, String.format("Unable to evaluate rule %s due to a syntax error. " +
"Check that your rule is in the correct format and returns a boolean. For more information " +
Expand Down Expand Up @@ -76,15 +79,15 @@ public static void verifyRule(

// Evaluate rule with given context
try {
final String value = (String) valueExpression.getValue(elContext);
LOGGER.debug("Rule {} evaluated to {}", testRule, value);
try {
checkRuleIsBooleanType(testRule, elContext, valueExpression);
checkRuleIsBooleanType(testRule, value);
} catch (final IllegalArgumentException e) {
throw new InvalidRuleException(e, String.format("Unable to evaluate rule %s due to a syntax error. " +
"Check that your rule is in the correct format and returns a boolean. For more information " +
"read: https://opensource.indeedeng.io/proctor/docs/test-rules/.", testRule));
}

valueExpression.getValue(elContext);
} catch (final ELException e) {
if (isIgnorable(root, absentIdentifiers)) {
LOGGER.debug(String.format("Rule %s contains uninstantiated identifier(s) in %s, ignoring the failure", testRule, absentIdentifiers));
Expand Down
33 changes: 19 additions & 14 deletions proctor-common/src/test/java/com/indeed/proctor/common/TestRuleEvaluator.java
View file
Open in desktop
Expand Up @@ -13,7 +13,6 @@

import static java.util.Collections.emptyMap;
import static java.util.Collections.singletonMap;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
Expand Down Expand Up @@ -50,10 +49,10 @@ public void testEmptyRule() {

@Test
public void testLiteralRule() {
for (final String rule : new String[] { "${true}", "${TRUE}", "${ TRUE }" }) {
for (final String rule : new String[] { "${true}", "${TRUE}", "${ TRUE }", "${'TRUE'}", "${'true'}" }) {
assertTrue("rule '" + rule + "' should be true", ruleEvaluator.evaluateBooleanRule(rule, emptyMap()));
}
for (final String rule : new String[] { "${false}", "${FALSE}", "${ FALSE }" }) {
for (final String rule : new String[] { "${false}", "${FALSE}", "${ FALSE }", "${'FALSE'}", "${'false'}" }) {
assertFalse("rule '" + rule + "' should be false", ruleEvaluator.evaluateBooleanRule(rule, emptyMap()));
}
}
Expand Down Expand Up @@ -84,13 +83,26 @@ public void testNonBooleanShouldFail() {
.hasMessageContaining("Received non-boolean return value");
}
{
// numeric result becomes String, not boolean
assertThatThrownBy(() -> ruleEvaluator.evaluateBooleanRule("${'tr'}${'ue'}", emptyMap()))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("Received non-boolean return value");
// String 'true' is true
assertTrue(ruleEvaluator.evaluateBooleanRule("${'tr'}${'ue'}", emptyMap()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think this is a valid format, ${'true'} would be valid but I do not think this format should be supported. cc @stevenchen-indeed for confirmation

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this does not look like convention. If it does not error out, maybe it is a corner case that was not caught before.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, it's okay as I think no one writes the rule like ${'true'}. But this will help validate the custom function like author's team need: placementFilter.matchesMetadata(INDEED_PLUS)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking into this we already allow it to get inputted into a test this just makes parsing it valid. I agree with Van it is okay.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, there was already a test with this rule (which probably should not be allowed on the UI) so I just 'fixed' it here in light of the new approach.

}
}

@Test
public void testStandaloneMethodCalls() {
final Map<String, Object> context = ImmutableMap.of("context", new Temp());
for (String rule : ImmutableList.of("${context.isValid()}", "${context.isFortyTwo('42')}")) {
assertTrue("rule '" + rule + "' should be true for " + context, ruleEvaluator.evaluateBooleanRule(rule, context));
}

for (String rule : ImmutableList.of("${!context.isValid()}", "${context.isFortyTwo('47')}")) {
assertFalse("rule '" + rule + "' should be false for " + context, ruleEvaluator.evaluateBooleanRule(rule, context));
}

assertThatThrownBy(() -> ruleEvaluator.evaluateBooleanRule("${context.isNotFortyTwo('42')}", context))
.isInstanceOf(Exception.class); // different versions of EL throw different exceptions
}

@Test
public void testMethodCalls() {
final Map<String, Object> context = ImmutableMap.of(
Expand Down Expand Up @@ -450,11 +462,4 @@ public void testVersionInRange() {
}
}

@Test
public void testNonBooleanRule() {
assertThat(ruleEvaluator.evaluateRule("${4}", emptyMap(), Integer.class)).isEqualTo(4);
assertThat(ruleEvaluator.evaluateRule("${true}", emptyMap(), Boolean.class)).isEqualTo(true);
assertThat(ruleEvaluator.evaluateRule("${4}", emptyMap(), String.class)).isEqualTo("4");
assertThat(ruleEvaluator.evaluateRule("${true}", emptyMap(), String.class)).isEqualTo("true");
}
}
18 changes: 0 additions & 18 deletions proctor-tomcat-deps-provided/pom.xml
View file
Open in desktop
Expand Up @@ -32,14 +32,6 @@
<groupId>org.apache.tomcat</groupId>
<artifactId>tomcat-jasper-el</artifactId>
</exclusion>
<exclusion>
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The proctor-tomcat-deps does not bring in tomcat-jsp-api and neither tomcat-servlet-api.

<groupId>org.apache.tomcat</groupId>
<artifactId>tomcat-jsp-api</artifactId>
</exclusion>
<exclusion>
<groupId>org.apache.tomcat</groupId>
<artifactId>tomcat-servlet-api</artifactId>
</exclusion>
</exclusions>
</dependency>
<dependency>
Expand All @@ -52,15 +44,5 @@
<artifactId>tomcat-jasper-el</artifactId>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>org.apache.tomcat</groupId>
<artifactId>tomcat-jsp-api</artifactId>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>org.apache.tomcat</groupId>
<artifactId>tomcat-servlet-api</artifactId>
<scope>provided</scope>
</dependency>
</dependencies>
</project>