diff --git a/proctor-common/src/main/java/com/indeed/proctor/common/ProctorRuleFunctions.java b/proctor-common/src/main/java/com/indeed/proctor/common/ProctorRuleFunctions.java index dae020973..28890e940 100644 --- a/proctor-common/src/main/java/com/indeed/proctor/common/ProctorRuleFunctions.java +++ b/proctor-common/src/main/java/com/indeed/proctor/common/ProctorRuleFunctions.java @@ -80,4 +80,44 @@ public static boolean versionInRange( } return inRange(version, start, end); } + + public static MaybeBool maybeAnd(final MaybeBool op1, final MaybeBool op2) { + if (MaybeBool.FALSE == op1 || MaybeBool.FALSE == op2) { + return MaybeBool.FALSE; + } + if (MaybeBool.TRUE == op1 && MaybeBool.TRUE == op2) { + return MaybeBool.TRUE; + } + return MaybeBool.UNKNOWN; + } + + public static MaybeBool maybeOr(final MaybeBool op1, final MaybeBool op2) { + if (MaybeBool.TRUE == op1 || MaybeBool.TRUE == op2) { + return MaybeBool.TRUE; + } + if (MaybeBool.FALSE == op1 && MaybeBool.FALSE == op2) { + return MaybeBool.FALSE; + } + return MaybeBool.UNKNOWN; + } + + public static MaybeBool maybeNot(final MaybeBool maybeBool) { + if (MaybeBool.TRUE == maybeBool) { + return MaybeBool.FALSE; + } + if (MaybeBool.FALSE == maybeBool) { + return MaybeBool.TRUE; + } + return MaybeBool.UNKNOWN; + } + + public static MaybeBool toMaybeBool(final boolean b) { + return b ? MaybeBool.TRUE : MaybeBool.FALSE; + } + + public enum MaybeBool { + TRUE, + FALSE, + UNKNOWN; + } } 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 477149272..fb8459b22 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 @@ -2,6 +2,7 @@ import com.indeed.proctor.common.el.LibraryFunctionMapperBuilder; import com.indeed.proctor.common.el.MulticontextReadOnlyVariableMapper; +import com.indeed.proctor.common.el.PartialExpressionBuilder; import org.apache.commons.lang3.ClassUtils; import org.apache.commons.lang3.StringUtils; import org.apache.el.ExpressionFactoryImpl; @@ -21,7 +22,9 @@ import javax.el.MapELResolver; import javax.el.ValueExpression; import javax.el.VariableMapper; +import java.util.HashSet; import java.util.Map; +import java.util.Set; /** * A nice tidy packaging of javax.el stuff. @@ -159,7 +162,64 @@ public boolean evaluateBooleanRuleWithValueExpr( + " from rule " + rule); } + /** + * @deprecated Use evaluateBooleanRulePartialWithValueExpr(String, Map) instead, it's more + * efficient + */ + @Deprecated + public boolean evaluateBooleanRulePartial( + final String rule, @Nonnull final Map values) + throws IllegalArgumentException { + final Map localContext = + ProctorUtils.convertToValueExpressionMap(expressionFactory, values); + return evaluateBooleanRulePartialWithValueExpr(rule, localContext); + } + + /** + * This method should only be used for partial matching with proctor rules + * @return Evaluates a partial rule + **/ + public boolean evaluateBooleanRulePartialWithValueExpr( + final String rule, @Nonnull final Map values) + throws IllegalArgumentException { + if (StringUtils.isBlank(rule)) { + return true; + } + if (!rule.startsWith("${") || !rule.endsWith("}")) { + LOGGER.error("Invalid rule '" + rule + "'"); // TODO: should this be an exception? + return false; + } + final String bareRule = ProctorUtils.removeElExpressionBraces(rule); + if (StringUtils.isBlank(bareRule) || "true".equalsIgnoreCase(bareRule)) { + return true; // always passes + } + if ("false".equalsIgnoreCase(bareRule)) { + return false; + } + + final ELContext elContext = createElContext(values); + final Set variablesDefined = new HashSet<>(); + variablesDefined.addAll(values.keySet()); + variablesDefined.addAll(testConstants.keySet()); + final PartialExpressionBuilder builder = + new PartialExpressionBuilder(rule, elContext, variablesDefined); + final ValueExpression ve = builder.createValueExpression(boolean.class); + checkRuleIsBooleanType(rule, elContext, ve); + 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); + } + /** @throws IllegalArgumentException if type of expression is not boolean */ static void checkRuleIsBooleanType( final String rule, final ELContext elContext, final ValueExpression ve) { diff --git a/proctor-common/src/main/java/com/indeed/proctor/common/el/NodeHunter.java b/proctor-common/src/main/java/com/indeed/proctor/common/el/NodeHunter.java new file mode 100644 index 000000000..6e50bde8d --- /dev/null +++ b/proctor-common/src/main/java/com/indeed/proctor/common/el/NodeHunter.java @@ -0,0 +1,175 @@ +package com.indeed.proctor.common.el; + +import com.google.common.collect.ImmutableList; +import com.indeed.proctor.common.ProctorRuleFunctions.MaybeBool; +import org.apache.el.parser.AstAnd; +import org.apache.el.parser.AstFunction; +import org.apache.el.parser.AstIdentifier; +import org.apache.el.parser.AstLiteralExpression; +import org.apache.el.parser.AstNot; +import org.apache.el.parser.AstNotEqual; +import org.apache.el.parser.AstOr; +import org.apache.el.parser.ELParserTreeConstants; +import org.apache.el.parser.Node; +import org.apache.el.parser.NodeVisitor; +import org.apache.el.parser.SimpleNode; + +import java.lang.reflect.Constructor; +import java.lang.reflect.InvocationTargetException; +import java.util.Collections; +import java.util.IdentityHashMap; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.Stack; +import java.util.stream.Collectors; + +public class NodeHunter implements NodeVisitor { + private static final List NODE_TYPES = + ImmutableList.copyOf(ELParserTreeConstants.jjtNodeName).stream() + .map(nodeName -> "Ast" + nodeName) + .collect(Collectors.toList()); + private static final Map NODE_TYPE_IDS = + NODE_TYPES.stream() + .collect(Collectors.toMap(nodeType -> nodeType, NODE_TYPES::indexOf)); + private static final int AST_FUNCTION_TYPE = 27; + private static final int AST_NOT_EQUAL_TYPE = 9; + + private final Set initialUnknowns = Collections.newSetFromMap(new IdentityHashMap<>()); + private final Map replacements = new IdentityHashMap<>(); + private final Set variablesDefined; + + NodeHunter(final Set variablesDefined) { + this.variablesDefined = variablesDefined; + } + + public static Node destroyUnknowns(final Node node, final Set variablesDefined) + throws Exception { + final NodeHunter nodeHunter = new NodeHunter(variablesDefined); + node.accept(nodeHunter); + if (nodeHunter.initialUnknowns.isEmpty()) { + // Nothing to do here + return node; + } + nodeHunter.calculateReplacements(); + final Node result = nodeHunter.replaceNodes(node); + // At this point result is a maybebool, we need to convert it to a bool + final Node resultIsNotFalse = nodeHunter.wrapIsNotFalse(result); + return resultIsNotFalse; + } + + private void calculateReplacements() { + final Stack nodesToDestroy = new Stack<>(); + initialUnknowns.forEach(nodesToDestroy::push); + while (!nodesToDestroy.isEmpty()) { + final Node nodeToDestroy = nodesToDestroy.pop(); + if (nodeToDestroy instanceof AstAnd) { + // Replace simple "and" with maybeAnd + replaceWithFunction(nodeToDestroy, "maybeAnd"); + } else if (nodeToDestroy instanceof AstOr) { + // Replace simple "or" with maybeOr + replaceWithFunction(nodeToDestroy, "maybeOr"); + } else if (nodeToDestroy instanceof AstNot) { + // Replace simple "not" with maybeNot + replaceWithFunction(nodeToDestroy, "maybeNot"); + // } else if (nodeToDestroy instanceof AstEqual || nodeToDestroy instanceof + // AstNotEqual) { + // TODO: if someone compares two bools using == that would be + // weird, but we could handle it by making sure any cases that mix + // maybeBool and bool are promoted to maybeBool like we do with the + // other logical operators + } else if (!replacements.containsKey(nodeToDestroy)) { + // Anything else propagate the unknown value + // + // TODO: If a bool is used as an argument to a function we + // could try and do the function if the maybeBool is true or + // false, and only propagate the unknown if any argument is + // unknown, but that seems rare and very complicated so I + // haven't handled that case here. + final AstLiteralExpression replacement = new AstLiteralExpression(1); + replacement.setImage(MaybeBool.UNKNOWN.name()); + replacements.put(nodeToDestroy, replacement); + } + if (nodeToDestroy.jjtGetParent() != null) { + nodesToDestroy.push(nodeToDestroy.jjtGetParent()); + } + } + } + + private AstFunction createFunctionReplacement(final Node node, final String function) { + final AstFunction replacement = new AstFunction(AST_FUNCTION_TYPE); + replacement.setPrefix("proctor"); + replacement.setLocalName(function); + replacement.setImage("proctor:" + function); + for (int i = 0; i < node.jjtGetNumChildren(); i++) { + final Node child = node.jjtGetChild(i); + if (replacements.containsKey(child)) { + replacement.jjtAddChild(replacements.get(child), i); + } else { + final AstFunction replacementChild = new AstFunction(AST_FUNCTION_TYPE); + replacementChild.setPrefix("proctor"); + replacementChild.setLocalName("toMaybeBool"); + replacementChild.setImage("proctor:toMaybeBool"); + replacementChild.jjtAddChild(child, 0); + replacement.jjtAddChild(replacementChild, i); + } + } + + return replacement; + } + + private void replaceWithFunction(final Node node, final String function) { + final AstFunction replacement = createFunctionReplacement(node, function); + replacements.put(node, replacement); + } + + private Node replaceNodes(final Node node) + throws NoSuchMethodException, InvocationTargetException, IllegalAccessException, + InstantiationException { + if (replacements.containsKey(node)) { + Node newNode = node; + while (replacements.containsKey(newNode)) { + newNode = replacements.get(newNode); + } + return newNode; + } + final Class nodeClass = node.getClass(); + final Constructor constructor = nodeClass.getConstructor(int.class); + final SimpleNode newNode = + (SimpleNode) constructor.newInstance(NODE_TYPE_IDS.get(nodeClass.getSimpleName())); + for (int i = 0; i < node.jjtGetNumChildren(); i++) { + final Node newChild = replaceNodes(node.jjtGetChild(i)); + newChild.jjtSetParent(newNode); + newNode.jjtAddChild(newChild, i); + } + newNode.jjtSetParent(node.jjtGetParent()); + newNode.setImage(node.getImage()); + if (newNode instanceof AstFunction) { + ((AstFunction) newNode).setPrefix(((AstFunction) node).getPrefix()); + ((AstFunction) newNode).setLocalName(((AstFunction) node).getLocalName()); + } + return newNode; + } + + @Override + public void visit(final Node node) throws Exception { + if (node instanceof AstIdentifier) { + String variable = node.getImage(); + if (!variablesDefined.contains(variable)) { + initialUnknowns.add(node); + } + } + } + + private Node wrapIsNotFalse(final Node node) { + final Node resultIsNotFalse = new AstNotEqual(AST_NOT_EQUAL_TYPE); + final AstLiteralExpression literalFalse = new AstLiteralExpression(1); + literalFalse.setImage(MaybeBool.FALSE.name()); + literalFalse.jjtSetParent(resultIsNotFalse); + resultIsNotFalse.jjtSetParent(node.jjtGetParent()); + node.jjtSetParent(resultIsNotFalse); + resultIsNotFalse.jjtAddChild(node, 0); + resultIsNotFalse.jjtAddChild(literalFalse, 1); + return resultIsNotFalse; + } +} diff --git a/proctor-common/src/main/java/com/indeed/proctor/common/el/PartialExpressionBuilder.java b/proctor-common/src/main/java/com/indeed/proctor/common/el/PartialExpressionBuilder.java new file mode 100644 index 000000000..8cad9f86c --- /dev/null +++ b/proctor-common/src/main/java/com/indeed/proctor/common/el/PartialExpressionBuilder.java @@ -0,0 +1,184 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.indeed.proctor.common.el; + +import org.apache.el.ValueExpressionImpl; +import org.apache.el.lang.FunctionMapperFactory; +import org.apache.el.lang.VariableMapperFactory; +import org.apache.el.parser.AstDeferredExpression; +import org.apache.el.parser.AstDynamicExpression; +import org.apache.el.parser.AstFunction; +import org.apache.el.parser.AstIdentifier; +import org.apache.el.parser.AstLiteralExpression; +import org.apache.el.parser.ELParser; +import org.apache.el.parser.Node; +import org.apache.el.parser.NodeVisitor; +import org.apache.el.parser.ParseException; +import org.apache.el.util.MessageFactory; + +import javax.el.ELContext; +import javax.el.ELException; +import javax.el.FunctionMapper; +import javax.el.ValueExpression; +import javax.el.VariableMapper; +import java.io.StringReader; +import java.lang.reflect.Method; +import java.util.Set; + +/** Like ExpressionBuilder except removes nodes referring to missing variables */ +public final class PartialExpressionBuilder implements NodeVisitor { + + private FunctionMapper fnMapper; + + private VariableMapper varMapper; + + private ELContext ctx; + + private String expression; + + private final Set variablesDefined; + /** */ + public PartialExpressionBuilder(String expression, ELContext ctx, Set variablesDefined) + throws ELException { + this.expression = expression; + + FunctionMapper ctxFn = ctx.getFunctionMapper(); + VariableMapper ctxVar = ctx.getVariableMapper(); + this.ctx = ctx; + + if (ctxFn != null) { + this.fnMapper = new FunctionMapperFactory(ctxFn); + } + if (ctxVar != null) { + this.varMapper = new VariableMapperFactory(ctxVar); + } + this.variablesDefined = variablesDefined; + } + + private static Node createNodeInternal(String expr) throws ELException { + if (expr == null) { + throw new ELException(MessageFactory.get("error.null")); + } + + // Removed caching as we translate nodes differently depending on variables defined + Node node; + try { + node = (new ELParser(new StringReader(expr))).CompositeExpression(); + + // validate composite expression + int numChildren = node.jjtGetNumChildren(); + if (numChildren == 1) { + node = node.jjtGetChild(0); + } else { + Class type = null; + Node child = null; + for (int i = 0; i < numChildren; i++) { + child = node.jjtGetChild(i); + if (child instanceof AstLiteralExpression) continue; + if (type == null) type = child.getClass(); + else { + if (!type.equals(child.getClass())) { + throw new ELException(MessageFactory.get("error.mixed", expr)); + } + } + } + } + + if (node instanceof AstDeferredExpression || node instanceof AstDynamicExpression) { + node = node.jjtGetChild(0); + } + } catch (ParseException pe) { + throw new ELException("Error Parsing: " + expr, pe); + } + return node; + } + + private void prepare(Node node) throws ELException { + try { + node.accept(this); + } catch (Exception e) { + if (e instanceof ELException) { + throw (ELException) e; + } else { + throw (new ELException(e)); + } + } + if (this.fnMapper instanceof FunctionMapperFactory) { + this.fnMapper = ((FunctionMapperFactory) this.fnMapper).create(); + } + if (this.varMapper instanceof VariableMapperFactory) { + this.varMapper = ((VariableMapperFactory) this.varMapper).create(); + } + } + + private Node build() throws ELException { + Node node = createNodeInternal(this.expression); + try { + node = NodeHunter.destroyUnknowns(node, variablesDefined); + } catch (final Exception e) { + throw new ELException(e); + } + this.prepare(node); + if (node instanceof AstDeferredExpression || node instanceof AstDynamicExpression) { + node = node.jjtGetChild(0); + } + return node; + } + + /* + * (non-Javadoc) + * + * @see com.sun.el.parser.NodeVisitor#visit(com.sun.el.parser.Node) + */ + @Override + public void visit(Node node) throws ELException { + if (node instanceof AstFunction) { + + final AstFunction funcNode = (AstFunction) node; + + if (this.fnMapper == null) { + throw new ELException(MessageFactory.get("error.fnMapper.null")); + } + Method function = + fnMapper.resolveFunction(funcNode.getPrefix(), funcNode.getLocalName()); + if (function == null) { + throw new ELException( + MessageFactory.get("error.fnMapper.method", funcNode.getOutputName())); + } + int pcnt = function.getParameterTypes().length; + if (node.jjtGetNumChildren() != pcnt) { + throw new ELException( + MessageFactory.get( + "error.fnMapper.paramcount", + funcNode.getOutputName(), + "" + pcnt, + "" + node.jjtGetNumChildren())); + } + } else if (node instanceof AstIdentifier && this.varMapper != null) { + String variable = node.getImage(); + + this.varMapper.resolveVariable(variable); + } + } + + public ValueExpression createValueExpression(Class expectedType) throws ELException { + final Node node = this.build(); + return new ValueExpressionImpl( + this.expression, node, this.fnMapper, this.varMapper, expectedType); + } +} 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 b4b232c33..d9c44a499 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 @@ -26,7 +26,33 @@ public class TestRuleEvaluator { @Before public void setUp() throws Exception { final Map testConstants = - singletonMap("LANGUAGES_ENABLED", Lists.newArrayList("en", "fr", "de")); + ImmutableMap.of( + "LANGUAGES_ENABLED", Lists.newArrayList("en", "fr", "de"), + "US_REMOTE_Q", + Lists.newArrayList( + "work from home remote", + "remote work from home", + "remote work from home jobs on indeed", + "at home work", + "online", + "work home online", + "online work from home", + "work at home", + "at home", + "online", + "work from home", + "remote"), + "US_REMOTE_L", + Lists.newArrayList( + "work at home", + "at home", + "online", + "work from home", + "remote", + "remote", + "us", + "remote,us", + "")); ruleEvaluator = new RuleEvaluator( RuleEvaluator.EXPRESSION_FACTORY, @@ -565,4 +591,75 @@ public void testNonBooleanRule() { assertThat(ruleEvaluator.evaluateRule("${true}", emptyMap(), String.class)) .isEqualTo("true"); } + + @Test + public void testPartialMatchEmptyContextMatch() throws Exception { + final String rule = + "${3<4 && (2 > 1 || proctor:versionInRange(version, '1.2.0.0', '2.4.0.0'))}"; + final Map values = emptyMap(); + assertTrue(ruleEvaluator.evaluateBooleanRulePartial(rule, values)); + } + + @Test + public void testPartialMatchEmptyContextNoMatch() throws Exception { + final String rule = + "${3<4 && 2<1 && proctor:versionInRange(version, '1.2.0.0', '2.4.0.0')}"; + final Map values = emptyMap(); + assertFalse(ruleEvaluator.evaluateBooleanRulePartial(rule, values)); + } + + @Test + public void testPartialMatchComplexRule() throws Exception { + final String rule = + "${country == 'US' && adFormat == 'web' && hasAccount && (indeed:contains(US_REMOTE_Q, fn:toLowerCase(fn:trim(query))) || indeed:contains(US_REMOTE_L, searchLocation))}"; + assertTrue(ruleEvaluator.evaluateBooleanRulePartial(rule, ImmutableMap.of())); + assertTrue( + ruleEvaluator.evaluateBooleanRulePartial( + rule, ImmutableMap.of("country", "US", "adFormat", "web"))); + assertFalse( + ruleEvaluator.evaluateBooleanRulePartial( + rule, ImmutableMap.of("country", "GB", "adFormat", "web"))); + assertFalse( + ruleEvaluator.evaluateBooleanRulePartial( + rule, ImmutableMap.of("country", "US", "adFormat", "mob"))); + assertFalse( + ruleEvaluator.evaluateBooleanRulePartial( + rule, + ImmutableMap.of("query", "software engineer", "searchLocation", "Austin"))); + assertTrue( + ruleEvaluator.evaluateBooleanRulePartial( + rule, ImmutableMap.of("query", "remote", "searchLocation", "Austin"))); + assertTrue( + ruleEvaluator.evaluateBooleanRulePartial( + rule, + ImmutableMap.of("query", "software engineer", "searchLocation", "remote"))); + assertTrue( + ruleEvaluator.evaluateBooleanRulePartial( + rule, ImmutableMap.of("query", "software engineer"))); + assertTrue( + ruleEvaluator.evaluateBooleanRulePartial( + rule, ImmutableMap.of("searchLocation", "Austin"))); + assertFalse( + ruleEvaluator.evaluateBooleanRulePartial( + rule, ImmutableMap.of("hasAccount", false, "searchLocation", "Austin"))); + } + + @Test + public void testPartialFalseSometimesHelps() throws Exception { + final String rule = "${!(country == 'US' && adFormat == 'web')}"; + assertFalse( + ruleEvaluator.evaluateBooleanRulePartial( + rule, ImmutableMap.of("country", "US", "adFormat", "web"))); + assertTrue( + ruleEvaluator.evaluateBooleanRulePartial(rule, ImmutableMap.of("country", "US"))); + assertTrue( + ruleEvaluator.evaluateBooleanRulePartial(rule, ImmutableMap.of("adFormat", "web"))); + assertTrue( + ruleEvaluator.evaluateBooleanRulePartial(rule, ImmutableMap.of("country", "GB"))); + assertTrue( + ruleEvaluator.evaluateBooleanRulePartial(rule, ImmutableMap.of("country", "mob"))); + assertTrue( + ruleEvaluator.evaluateBooleanRulePartial( + rule, ImmutableMap.of("country", "US", "adFormat", "mob"))); + } }