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

robertdanci · 2023-04-06T02:59:37Z

No description provided.

robertdanci · 2023-04-06T03:01:47Z

proctor-tomcat-deps-provided/pom.xml

@@ -32,14 +32,6 @@
                    <groupId>org.apache.tomcat</groupId>
                    <artifactId>tomcat-jasper-el</artifactId>
                </exclusion>
-                <exclusion>


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

zacharygoodwin · 2023-04-12T15:19:39Z

proctor-common/src/test/java/com/indeed/proctor/common/TestRuleEvaluator.java

-                    .isInstanceOf(IllegalArgumentException.class)
-                    .hasMessageContaining("Received non-boolean return value");
+            // String 'true' is true
+            assertTrue(ruleEvaluator.evaluateBooleanRule("${'tr'}${'ue'}", emptyMap()));


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

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

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)

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.

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.

zacharygoodwin · 2023-04-12T20:09:34Z

proctor-common/src/test/java/com/indeed/proctor/common/TestRuleEvaluator.java

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


Nitpick: "' should be false for "

stevenchen-indeed · 2023-04-12T20:15:59Z

proctor-common/src/main/java/com/indeed/proctor/common/RuleEvaluator.java

@@ -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);


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?

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.

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?

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.

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?

Make it clear that Strings 'true' and 'false' are valid

robertdanci commented Apr 6, 2023

View reviewed changes

zacharygoodwin reviewed Apr 12, 2023

View reviewed changes

zacharygoodwin requested review from vanhaxl and stevenchen-indeed April 12, 2023 15:20

zacharygoodwin reviewed Apr 12, 2023

View reviewed changes

zacharygoodwin approved these changes Apr 12, 2023

View reviewed changes

stevenchen-indeed reviewed Apr 12, 2023

View reviewed changes

zacharygoodwin self-requested a review April 12, 2023 20:18

Robert Danci added 4 commits April 18, 2023 11:35

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

a00e5f4

PROC-1219: Make method arg final

5e2c697

PROC-1219: Remove other tomcat7 deps unrelated to EL

9e504ec

PROC-1219: Fix error message in case test fails

4960d97

Make it clear that Strings 'true' and 'false' are valid

robertdanci force-pushed the jira/PROC-1219 branch from cac5323 to 4960d97 Compare April 18, 2023 02:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

robertdanci commented Apr 6, 2023

robertdanci Apr 6, 2023

zacharygoodwin Apr 12, 2023

stevenchen-indeed Apr 12, 2023 •

edited

vanhaxl Apr 12, 2023

zacharygoodwin Apr 12, 2023 •

edited

robertdanci Apr 13, 2023

zacharygoodwin Apr 12, 2023

stevenchen-indeed Apr 12, 2023

robertdanci Apr 13, 2023

stevenchen-indeed Apr 13, 2023

robertdanci Apr 14, 2023 •

edited

stevenchen-indeed Apr 26, 2023

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

Are you sure you want to change the base?

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

Conversation

robertdanci commented Apr 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenchen-indeed Apr 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zacharygoodwin Apr 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertdanci Apr 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenchen-indeed Apr 12, 2023 •

edited

zacharygoodwin Apr 12, 2023 •

edited

robertdanci Apr 14, 2023 •

edited