Skip to content
This repository was archived by the owner on Jul 10, 2024. It is now read-only.

Commit ed45c4d

Browse files
author
Joseph Barratt
committed
PROC-1334: Move rule filtering to its own class
1 parent ed925ab commit ed45c4d

7 files changed

Lines changed: 215 additions & 103 deletions

File tree

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

Lines changed: 0 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22

33
import com.indeed.proctor.common.el.LibraryFunctionMapperBuilder;
44
import com.indeed.proctor.common.el.MulticontextReadOnlyVariableMapper;
5-
import com.indeed.proctor.common.el.PartialExpressionBuilder;
65
import org.apache.commons.lang3.ClassUtils;
76
import org.apache.commons.lang3.StringUtils;
87
import org.apache.el.ExpressionFactoryImpl;
@@ -22,10 +21,7 @@
2221
import javax.el.MapELResolver;
2322
import javax.el.ValueExpression;
2423
import javax.el.VariableMapper;
25-
26-
import java.util.HashSet;
2724
import java.util.Map;
28-
import java.util.Set;
2925

3026
/**
3127
* A nice tidy packaging of javax.el stuff.
@@ -157,50 +153,6 @@ public boolean evaluateBooleanRuleWithValueExpr(final String rule, @Nonnull fina
157153
+ " from rule " + rule);
158154
}
159155

160-
/**
161-
* @deprecated Use evaluateBooleanRulePartialWithValueExpr(String, Map) instead, it's more efficient
162-
*/
163-
@Deprecated
164-
public boolean evaluateBooleanRulePartial(final String rule, @Nonnull final Map<String, Object> values) throws IllegalArgumentException {
165-
final Map<String, ValueExpression> localContext = ProctorUtils.convertToValueExpressionMap(expressionFactory, values);
166-
return evaluateBooleanRulePartialWithValueExpr(rule, localContext);
167-
}
168-
169-
public boolean evaluateBooleanRulePartialWithValueExpr(final String rule, @Nonnull final Map<String, ValueExpression> values) throws IllegalArgumentException {
170-
if (StringUtils.isBlank(rule)) {
171-
return true;
172-
}
173-
if (!rule.startsWith("${") || !rule.endsWith("}")) {
174-
LOGGER.error("Invalid rule '" + rule + "'"); // TODO: should this be an exception?
175-
return false;
176-
}
177-
final String bareRule = ProctorUtils.removeElExpressionBraces(rule);
178-
if (StringUtils.isBlank(bareRule) || "true".equalsIgnoreCase(bareRule)) {
179-
return true; // always passes
180-
}
181-
if ("false".equalsIgnoreCase(bareRule)) {
182-
return false;
183-
}
184-
185-
final ELContext elContext = createElContext(values);
186-
final Set<String> variablesDefined = new HashSet<>();
187-
variablesDefined.addAll(values.keySet());
188-
variablesDefined.addAll(testConstants.keySet());
189-
final PartialExpressionBuilder builder = new PartialExpressionBuilder(rule, elContext, variablesDefined);
190-
final ValueExpression ve = builder.createValueExpression(boolean.class);
191-
checkRuleIsBooleanType(rule, elContext, ve);
192-
193-
final Object result = ve.getValue(elContext);
194-
195-
if (result instanceof Boolean) {
196-
return ((Boolean) result);
197-
}
198-
// this should never happen, evaluateRule throws ELException when it cannot coerce to Boolean
199-
throw new IllegalArgumentException("Received non-boolean return value: "
200-
+ (result == null ? "null" : result.getClass().getCanonicalName())
201-
+ " from rule " + rule);
202-
}
203-
204156
/**
205157
* @throws IllegalArgumentException if type of expression is not boolean
206158
*/

‎proctor-common/src/main/java/com/indeed/proctor/common/RuleFilter.java‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
package com.indeed.proctor.common;
2+
3+
import com.indeed.proctor.common.el.filter.PartialExpressionFactory;
4+
5+
import javax.annotation.Nonnull;
6+
import javax.annotation.concurrent.NotThreadSafe;
7+
import javax.el.FunctionMapper;
8+
import javax.el.ValueExpression;
9+
10+
import java.util.Map;
11+
12+
/**
13+
* Leverages RuleEvaluator to determine whether a given rule *could* match the
14+
* given context; useful for tools that search allocations.
15+
**/
16+
@NotThreadSafe
17+
public class RuleFilter {
18+
@Nonnull
19+
private final PartialExpressionFactory expressionFactory;
20+
private final RuleEvaluator ruleEvaluator;
21+
22+
RuleFilter(
23+
final FunctionMapper functionMapper,
24+
@Nonnull final Map<String, Object> testConstantsMap
25+
) {
26+
this.expressionFactory = new PartialExpressionFactory(testConstantsMap.keySet());
27+
this.ruleEvaluator = new RuleEvaluator(
28+
expressionFactory,
29+
functionMapper,
30+
testConstantsMap);
31+
}
32+
33+
public static RuleFilter createDefaultRuleFilter(final Map<String, Object> testConstantsMap) {
34+
return new RuleFilter(RuleEvaluator.FUNCTION_MAPPER, testConstantsMap);
35+
}
36+
37+
public boolean ruleCanMatch(final String rule, final Map<String, Object> values) {
38+
expressionFactory.setContextVariables(values.keySet());
39+
final Map<String, ValueExpression> localContext = ProctorUtils
40+
.convertToValueExpressionMap(expressionFactory, values);
41+
return ruleEvaluator.evaluateBooleanRuleWithValueExpr(rule, localContext);
42+
}
43+
}

proctor-common/src/main/java/com/indeed/proctor/common/el/NodeHunter.java renamed to proctor-common/src/main/java/com/indeed/proctor/common/el/filter/NodeHunter.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
package com.indeed.proctor.common.el;
1+
package com.indeed.proctor.common.el.filter;
22

33
import com.google.common.collect.ImmutableList;
44
import com.indeed.proctor.common.ProctorRuleFunctions.MaybeBool;
@@ -24,7 +24,7 @@
2424
import java.util.Stack;
2525
import java.util.stream.Collectors;
2626

27-
public class NodeHunter implements NodeVisitor {
27+
class NodeHunter implements NodeVisitor {
2828
private static final List<String> NODE_TYPES = ImmutableList.copyOf(ELParserTreeConstants.jjtNodeName)
2929
.stream()
3030
.map(nodeName -> "Ast" + nodeName)

proctor-common/src/main/java/com/indeed/proctor/common/el/PartialExpressionBuilder.java renamed to proctor-common/src/main/java/com/indeed/proctor/common/el/filter/PartialExpressionBuilder.java

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
* limitations under the License.
1616
*/
1717

18-
package com.indeed.proctor.common.el;
18+
package com.indeed.proctor.common.el.filter;
1919

2020
import org.apache.el.MethodExpressionImpl;
2121
import org.apache.el.MethodExpressionLiteral;
@@ -48,28 +48,23 @@
4848
/**
4949
* Like ExpressionBuilder except removes nodes referring to missing variables
5050
*/
51-
public final class PartialExpressionBuilder implements NodeVisitor {
51+
class PartialExpressionBuilder implements NodeVisitor {
5252

5353
private FunctionMapper fnMapper;
5454

5555
private VariableMapper varMapper;
5656

57-
private ELContext ctx;
58-
5957
private String expression;
6058

6159
private final Set<String> variablesDefined;
62-
/**
63-
*
64-
*/
60+
6561
public PartialExpressionBuilder(String expression, ELContext ctx, Set<String> variablesDefined)
6662
throws ELException {
6763
this.expression = expression;
6864

6965

7066
FunctionMapper ctxFn = ctx.getFunctionMapper();
7167
VariableMapper ctxVar = ctx.getVariableMapper();
72-
this.ctx = ctx;
7368

7469
if (ctxFn != null) {
7570
this.fnMapper = new FunctionMapperFactory(ctxFn);

‎proctor-common/src/main/java/com/indeed/proctor/common/el/filter/PartialExpressionFactory.java‎

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,95 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one or more
3+
* contributor license agreements. See the NOTICE file distributed with
4+
* this work for additional information regarding copyright ownership.
5+
* The ASF licenses this file to You under the Apache License, Version 2.0
6+
* (the "License"); you may not use this file except in compliance with
7+
* the License. You may obtain a copy of the License at
8+
*
9+
* http://www.apache.org/licenses/LICENSE-2.0
10+
*
11+
* Unless required by applicable law or agreed to in writing, software
12+
* distributed under the License is distributed on an "AS IS" BASIS,
13+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
14+
* See the License for the specific language governing permissions and
15+
* limitations under the License.
16+
*/
17+
18+
package com.indeed.proctor.common.el.filter;
19+
20+
import com.google.common.collect.ImmutableSet;
21+
import org.apache.el.ValueExpressionLiteral;
22+
import org.apache.el.lang.ELSupport;
23+
import org.apache.el.util.MessageFactory;
24+
25+
import javax.annotation.concurrent.NotThreadSafe;
26+
import javax.el.ELContext;
27+
import javax.el.ExpressionFactory;
28+
import javax.el.MethodExpression;
29+
import javax.el.ValueExpression;
30+
31+
import java.util.HashSet;
32+
import java.util.Set;
33+
34+
/**
35+
* Like ExpressionFactoryImpl except removes nodes referring to missing variables
36+
*/
37+
@NotThreadSafe
38+
public class PartialExpressionFactory extends ExpressionFactory {
39+
40+
private final Set<String> testConstants;
41+
private final Set<String> contextVariables;
42+
43+
public PartialExpressionFactory(final Set<String> testConstants) {
44+
super();
45+
this.testConstants = testConstants;
46+
this.contextVariables = new HashSet<>();
47+
}
48+
49+
@Override
50+
public Object coerceToType(Object obj, Class<?> type) {
51+
return ELSupport.coerceToType(obj, type);
52+
}
53+
54+
@Override
55+
public MethodExpression createMethodExpression(ELContext context,
56+
String expression, Class<?> expectedReturnType,
57+
Class<?>[] expectedParamTypes) {
58+
PartialExpressionBuilder builder = new PartialExpressionBuilder(expression, context, getAllDefinedVariables());
59+
return builder.createMethodExpression(expectedReturnType,
60+
expectedParamTypes);
61+
}
62+
63+
@Override
64+
public ValueExpression createValueExpression(ELContext context,
65+
String expression, Class<?> expectedType) {
66+
if (expectedType == null) {
67+
throw new NullPointerException(MessageFactory
68+
.get("error.value.expectedType"));
69+
}
70+
PartialExpressionBuilder builder = new PartialExpressionBuilder(expression, context, getAllDefinedVariables());
71+
return builder.createValueExpression(expectedType);
72+
}
73+
74+
@Override
75+
public ValueExpression createValueExpression(Object instance,
76+
Class<?> expectedType) {
77+
if (expectedType == null) {
78+
throw new NullPointerException(MessageFactory
79+
.get("error.value.expectedType"));
80+
}
81+
return new ValueExpressionLiteral(instance, expectedType);
82+
}
83+
84+
public void setContextVariables(final Set<String> contextVariables) {
85+
this.contextVariables.clear();
86+
this.contextVariables.addAll(contextVariables);
87+
}
88+
89+
private Set<String> getAllDefinedVariables() {
90+
return ImmutableSet.<String>builder()
91+
.addAll(testConstants)
92+
.addAll(contextVariables)
93+
.build();
94+
}
95+
}

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

Lines changed: 1 addition & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88
import org.junit.Test;
99

1010
import javax.el.ELException;
11-
1211
import java.util.Arrays;
1312
import java.util.Map;
1413

@@ -28,10 +27,7 @@ public class TestRuleEvaluator {
2827

2928
@Before
3029
public void setUp() throws Exception {
31-
final Map<String, Object> testConstants = ImmutableMap.of(
32-
"LANGUAGES_ENABLED", Lists.newArrayList("en", "fr", "de"),
33-
"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"),
34-
"US_REMOTE_L", Lists.newArrayList("work at home", "at home", "online", "work from home", "remote", "remote", "us", "remote,us", ""));
30+
final Map<String, Object> testConstants = singletonMap("LANGUAGES_ENABLED", Lists.newArrayList("en", "fr", "de"));
3531
ruleEvaluator = new RuleEvaluator(RuleEvaluator.EXPRESSION_FACTORY, RuleEvaluator.FUNCTION_MAPPER, testConstants);
3632
}
3733

@@ -461,44 +457,4 @@ public void testNonBooleanRule() {
461457
assertThat(ruleEvaluator.evaluateRule("${4}", emptyMap(), String.class)).isEqualTo("4");
462458
assertThat(ruleEvaluator.evaluateRule("${true}", emptyMap(), String.class)).isEqualTo("true");
463459
}
464-
465-
@Test
466-
public void testPartialMatchEmptyContextMatch() throws Exception {
467-
final String rule = "${3<4 && (2 > 1 || proctor:versionInRange(version, '1.2.0.0', '2.4.0.0'))}";
468-
final Map<String, Object> values = emptyMap();
469-
assertTrue(ruleEvaluator.evaluateBooleanRulePartial(rule, values));
470-
}
471-
472-
@Test
473-
public void testPartialMatchEmptyContextNoMatch() throws Exception {
474-
final String rule = "${3<4 && 2<1 && proctor:versionInRange(version, '1.2.0.0', '2.4.0.0')}";
475-
final Map<String, Object> values = emptyMap();
476-
assertFalse(ruleEvaluator.evaluateBooleanRulePartial(rule, values));
477-
}
478-
479-
@Test
480-
public void testPartialMatchComplexRule() throws Exception {
481-
final String rule = "${country == 'US' && adFormat == 'web' && hasAccount && (indeed:contains(US_REMOTE_Q, fn:toLowerCase(fn:trim(query))) || indeed:contains(US_REMOTE_L, searchLocation))}";
482-
assertTrue(ruleEvaluator.evaluateBooleanRulePartial(rule, ImmutableMap.of()));
483-
assertTrue(ruleEvaluator.evaluateBooleanRulePartial(rule, ImmutableMap.of("country", "US", "adFormat", "web")));
484-
assertFalse(ruleEvaluator.evaluateBooleanRulePartial(rule, ImmutableMap.of("country", "GB", "adFormat", "web")));
485-
assertFalse(ruleEvaluator.evaluateBooleanRulePartial(rule, ImmutableMap.of("country", "US", "adFormat", "mob")));
486-
assertFalse(ruleEvaluator.evaluateBooleanRulePartial(rule, ImmutableMap.of("query", "software engineer", "searchLocation", "Austin")));
487-
assertTrue(ruleEvaluator.evaluateBooleanRulePartial(rule, ImmutableMap.of("query", "remote", "searchLocation", "Austin")));
488-
assertTrue(ruleEvaluator.evaluateBooleanRulePartial(rule, ImmutableMap.of("query", "software engineer", "searchLocation", "remote")));
489-
assertTrue(ruleEvaluator.evaluateBooleanRulePartial(rule, ImmutableMap.of("query", "software engineer")));
490-
assertTrue(ruleEvaluator.evaluateBooleanRulePartial(rule, ImmutableMap.of("searchLocation", "Austin")));
491-
assertFalse(ruleEvaluator.evaluateBooleanRulePartial(rule, ImmutableMap.of("hasAccount", false, "searchLocation", "Austin")));
492-
}
493-
494-
@Test
495-
public void testPartialFalseSometimesHelps() throws Exception {
496-
final String rule = "${!(country == 'US' && adFormat == 'web')}";
497-
assertFalse(ruleEvaluator.evaluateBooleanRulePartial(rule, ImmutableMap.of("country", "US", "adFormat", "web")));
498-
assertTrue(ruleEvaluator.evaluateBooleanRulePartial(rule, ImmutableMap.of("country", "US")));
499-
assertTrue(ruleEvaluator.evaluateBooleanRulePartial(rule, ImmutableMap.of("adFormat", "web")));
500-
assertTrue(ruleEvaluator.evaluateBooleanRulePartial(rule, ImmutableMap.of("country", "GB")));
501-
assertTrue(ruleEvaluator.evaluateBooleanRulePartial(rule, ImmutableMap.of("country", "mob")));
502-
assertTrue(ruleEvaluator.evaluateBooleanRulePartial(rule, ImmutableMap.of("country", "US", "adFormat", "mob")));
503-
}
504460
}

0 commit comments

Comments
 (0)