From a00e5f4a7a57f47677aa417e48d83fcd3e97aa9c Mon Sep 17 00:00:00 2001 From: Robert Danci Date: Thu, 6 Apr 2023 11:58:04 +0900 Subject: [PATCH 1/4] PROC-1219: Fix rule return result check so that it supports method calls --- .../indeed/proctor/common/RuleEvaluator.java | 41 ++++--------------- .../proctor/common/RuleVerifyUtils.java | 13 +++--- .../proctor/common/TestRuleEvaluator.java | 29 +++++++------ proctor-tomcat-deps-provided/pom.xml | 18 -------- 4 files changed, 33 insertions(+), 68 deletions(-) diff --git a/proctor-common/src/main/java/com/indeed/proctor/common/RuleEvaluator.java b/proctor-common/src/main/java/com/indeed/proctor/common/RuleEvaluator.java index f01782f67..21a7b8b10 100644 --- a/proctor-common/src/main/java/com/indeed/proctor/common/RuleEvaluator.java +++ b/proctor-common/src/main/java/com/indeed/proctor/common/RuleEvaluator.java @@ -131,51 +131,26 @@ public boolean evaluateBooleanRule(final String rule, @Nonnull final Map 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 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); } } diff --git a/proctor-common/src/main/java/com/indeed/proctor/common/RuleVerifyUtils.java b/proctor-common/src/main/java/com/indeed/proctor/common/RuleVerifyUtils.java index d26534f59..16a48098a 100644 --- a/proctor-common/src/main/java/com/indeed/proctor/common/RuleVerifyUtils.java +++ b/proctor-common/src/main/java/com/indeed/proctor/common/RuleVerifyUtils.java @@ -37,10 +37,13 @@ public static void verifyRule( final Set 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 " + @@ -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)); diff --git a/proctor-common/src/test/java/com/indeed/proctor/common/TestRuleEvaluator.java b/proctor-common/src/test/java/com/indeed/proctor/common/TestRuleEvaluator.java index 86abe5103..18e42fc5a 100644 --- a/proctor-common/src/test/java/com/indeed/proctor/common/TestRuleEvaluator.java +++ b/proctor-common/src/test/java/com/indeed/proctor/common/TestRuleEvaluator.java @@ -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; @@ -84,11 +83,24 @@ 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())); + } + } + + @Test + public void testStandaloneMethodCalls() { + final Map 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 true for " + context, ruleEvaluator.evaluateBooleanRule(rule, context)); } + + assertThatThrownBy(() -> ruleEvaluator.evaluateBooleanRule("${context.isNotFortyTwo('42')}", context)) + .isInstanceOf(Exception.class); // different versions of EL throw different exceptions } @Test @@ -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"); - } } diff --git a/proctor-tomcat-deps-provided/pom.xml b/proctor-tomcat-deps-provided/pom.xml index 43479104e..448bae877 100644 --- a/proctor-tomcat-deps-provided/pom.xml +++ b/proctor-tomcat-deps-provided/pom.xml @@ -32,14 +32,6 @@ org.apache.tomcat tomcat-jasper-el - - org.apache.tomcat - tomcat-jsp-api - - - org.apache.tomcat - tomcat-servlet-api - @@ -52,15 +44,5 @@ tomcat-jasper-el provided - - org.apache.tomcat - tomcat-jsp-api - provided - - - org.apache.tomcat - tomcat-servlet-api - provided - From 5e2c6972876848821985e951b8b3e0c383750221 Mon Sep 17 00:00:00 2001 From: Robert Danci Date: Fri, 7 Apr 2023 13:12:04 +0900 Subject: [PATCH 2/4] PROC-1219: Make method arg final --- .../src/main/java/com/indeed/proctor/common/RuleEvaluator.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/proctor-common/src/main/java/com/indeed/proctor/common/RuleEvaluator.java b/proctor-common/src/main/java/com/indeed/proctor/common/RuleEvaluator.java index 21a7b8b10..f80c2f3d3 100644 --- a/proctor-common/src/main/java/com/indeed/proctor/common/RuleEvaluator.java +++ b/proctor-common/src/main/java/com/indeed/proctor/common/RuleEvaluator.java @@ -141,7 +141,7 @@ public boolean evaluateBooleanRule(final String rule, @Nonnull final Map Date: Fri, 7 Apr 2023 15:13:23 +0900 Subject: [PATCH 3/4] PROC-1219: Remove other tomcat7 deps unrelated to EL --- pom.xml | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/pom.xml b/pom.xml index ccf648fdb..46cb56f0d 100644 --- a/pom.xml +++ b/pom.xml @@ -401,16 +401,6 @@ tomcat-jasper-el ${tomcat-api7.version} - - org.apache.tomcat - tomcat-jsp-api - ${tomcat-api7.version} - - - org.apache.tomcat - tomcat-servlet-api - ${tomcat-api7.version} - From 4960d9733db3cfe469eca84f36144ffbcb7d3a9a Mon Sep 17 00:00:00 2001 From: Robert Danci Date: Thu, 13 Apr 2023 11:39:31 +0900 Subject: [PATCH 4/4] PROC-1219: Fix error message in case test fails Make it clear that Strings 'true' and 'false' are valid --- .../java/com/indeed/proctor/common/TestRuleEvaluator.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/proctor-common/src/test/java/com/indeed/proctor/common/TestRuleEvaluator.java b/proctor-common/src/test/java/com/indeed/proctor/common/TestRuleEvaluator.java index 18e42fc5a..719832c49 100644 --- a/proctor-common/src/test/java/com/indeed/proctor/common/TestRuleEvaluator.java +++ b/proctor-common/src/test/java/com/indeed/proctor/common/TestRuleEvaluator.java @@ -49,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())); } } @@ -96,7 +96,7 @@ public void testStandaloneMethodCalls() { } for (String rule : ImmutableList.of("${!context.isValid()}", "${context.isFortyTwo('47')}")) { - assertFalse("rule '" + rule + "' should be true for " + context, ruleEvaluator.evaluateBooleanRule(rule, context)); + assertFalse("rule '" + rule + "' should be false for " + context, ruleEvaluator.evaluateBooleanRule(rule, context)); } assertThatThrownBy(() -> ruleEvaluator.evaluateBooleanRule("${context.isNotFortyTwo('42')}", context))