This is an automated email from the ASF dual-hosted git repository.
shuber pushed a commit to branch unomi-1.5.x
in repository https://gitbox.apache.org/repos/asf/unomi.git
The following commit(s) were added to refs/heads/unomi-1.5.x by this push:
new ec2ea6c UNOMI-492 Make rules engine more robust when some rules are
invalid (null actions) (#314)
ec2ea6c is described below
commit ec2ea6c6ae2ff59d909ec3eeca4478de6b7f1d04
Author: Serge Huber <[email protected]>
AuthorDate: Mon Jun 28 13:04:47 2021 +0200
UNOMI-492 Make rules engine more robust when some rules are invalid (null
actions) (#314)
* UNOMI-492 Make rules engine more robust when some rules are invalid (null
actions)
- Make sure we handle null actions gracefully instead of generating a NPE
and blocking loading of further rules
- Add an integration test to test the case of null actions in rule
- Improve logging when rules have null or empty conditions and actions
* UNOMI-492 Simplify and improve integration test
(cherry picked from commit 0fa7cc10e8452611025bbca4e3b858a4ed4d67f9)
---
.../org/apache/unomi/itests/RuleServiceIT.java | 78 ++++++++++++++++++++++
.../apache/unomi/services/impl/ParserHelper.java | 21 ++++--
.../impl/definitions/DefinitionsServiceImpl.java | 8 +--
.../services/impl/events/EventServiceImpl.java | 2 +-
.../services/impl/goals/GoalsServiceImpl.java | 18 +++--
.../services/impl/profiles/ProfileServiceImpl.java | 4 +-
.../services/impl/queries/QueryServiceImpl.java | 8 +--
.../services/impl/rules/RulesServiceImpl.java | 16 ++---
.../services/impl/segments/SegmentServiceImpl.java | 12 ++--
9 files changed, 128 insertions(+), 39 deletions(-)
diff --git a/itests/src/test/java/org/apache/unomi/itests/RuleServiceIT.java
b/itests/src/test/java/org/apache/unomi/itests/RuleServiceIT.java
new file mode 100644
index 0000000..ee25a09
--- /dev/null
+++ b/itests/src/test/java/org/apache/unomi/itests/RuleServiceIT.java
@@ -0,0 +1,78 @@
+/*
+ * 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 org.apache.unomi.itests;
+
+import org.apache.unomi.api.Metadata;
+import org.apache.unomi.api.rules.Rule;
+import org.apache.unomi.api.services.DefinitionsService;
+import org.apache.unomi.api.services.RulesService;
+import org.apache.unomi.persistence.spi.PersistenceService;
+import org.junit.Before;
+import org.junit.Test;
+import org.ops4j.pax.exam.util.Filter;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.inject.Inject;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
+
+/**
+ * Integration tests for the Unomi rule service.
+ */
+public class RuleServiceIT extends BaseIT {
+
+ private final static Logger LOGGER =
LoggerFactory.getLogger(RuleServiceIT.class);
+
+ private final static String TEST_RULE_ID = "test-rule-id";
+ public static final String TEST_SCOPE = "test-scope";
+
+ @Inject
+ @Filter(timeout = 600000)
+ protected RulesService rulesService;
+
+ @Inject
+ @Filter(timeout = 600000)
+ protected PersistenceService persistenceService;
+
+ @Inject
+ @Filter(timeout = 600000)
+ protected DefinitionsService definitionsService;
+
+ @Before
+ public void setUp() {
+ TestUtils.removeAllProfiles(definitionsService, persistenceService);
+ }
+
+ @Test
+ public void testRuleWithNullActions() throws InterruptedException {
+ Metadata metadata = new Metadata(TEST_RULE_ID);
+ metadata.setName(TEST_RULE_ID + "_name");
+ metadata.setDescription(TEST_RULE_ID + "_description");
+ metadata.setScope(TEST_SCOPE);
+ Rule nullRule = new Rule(metadata);
+ nullRule.setCondition(null);
+ nullRule.setActions(null);
+ rulesService.setRule(nullRule);
+ refreshPersistence();
+ nullRule = rulesService.getRule(TEST_RULE_ID);
+ assertNull("Expected rule actions to be null", nullRule.getActions());
+ assertNull("Expected rule condition to be null",
nullRule.getCondition());
+ assertEquals("Invalid rule name", TEST_RULE_ID + "_name",
nullRule.getMetadata().getName());
+ }
+}
diff --git
a/services/src/main/java/org/apache/unomi/services/impl/ParserHelper.java
b/services/src/main/java/org/apache/unomi/services/impl/ParserHelper.java
index a861f6c..ef9b9de 100644
--- a/services/src/main/java/org/apache/unomi/services/impl/ParserHelper.java
+++ b/services/src/main/java/org/apache/unomi/services/impl/ParserHelper.java
@@ -23,6 +23,7 @@ import org.apache.unomi.api.actions.Action;
import org.apache.unomi.api.actions.ActionType;
import org.apache.unomi.api.conditions.Condition;
import org.apache.unomi.api.conditions.ConditionType;
+import org.apache.unomi.api.rules.Rule;
import org.apache.unomi.api.services.DefinitionsService;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -39,8 +40,9 @@ public class ParserHelper {
private static final Set<String> unresolvedActionTypes = new HashSet<>();
private static final Set<String> unresolvedConditionTypes = new
HashSet<>();
- public static boolean resolveConditionType(final DefinitionsService
definitionsService, Condition rootCondition) {
+ public static boolean resolveConditionType(final DefinitionsService
definitionsService, Condition rootCondition, String contextObjectName) {
if (rootCondition == null) {
+ logger.warn("Couldn't resolve null condition for {}",
contextObjectName);
return false;
}
final List<String> result = new ArrayList<String>();
@@ -56,7 +58,7 @@ public class ParserHelper {
result.add(condition.getConditionTypeId());
if
(!unresolvedConditionTypes.contains(condition.getConditionTypeId())) {
unresolvedConditionTypes.add(condition.getConditionTypeId());
- logger.warn("Couldn't resolve condition type: " +
condition.getConditionTypeId());
+ logger.warn("Couldn't resolve condition type: {}
for {}", condition.getConditionTypeId(), contextObjectName);
}
}
}
@@ -96,15 +98,26 @@ public class ParserHelper {
}
}
- public static boolean resolveActionTypes(DefinitionsService
definitionsService, List<Action> actions) {
+ public static boolean resolveActionTypes(DefinitionsService
definitionsService, Rule rule) {
boolean result = true;
- for (Action action : actions) {
+ if (rule.getActions() == null) {
+ logger.warn("Rule {}:{} has null actions", rule.getItemId(),
rule.getMetadata().getName());
+ return false;
+ }
+ if (rule.getActions().isEmpty()) {
+ logger.warn("Rule {}:{} has empty actions", rule.getItemId(),
rule.getMetadata().getName());
+ return result;
+ }
+ for (Action action : rule.getActions()) {
result &= ParserHelper.resolveActionType(definitionsService,
action);
}
return result;
}
public static boolean resolveActionType(DefinitionsService
definitionsService, Action action) {
+ if (definitionsService == null) {
+ return false;
+ }
if (action.getActionType() == null) {
ActionType actionType =
definitionsService.getActionType(action.getActionTypeId());
if (actionType != null) {
diff --git
a/services/src/main/java/org/apache/unomi/services/impl/definitions/DefinitionsServiceImpl.java
b/services/src/main/java/org/apache/unomi/services/impl/definitions/DefinitionsServiceImpl.java
index 259fdb7..159084e 100644
---
a/services/src/main/java/org/apache/unomi/services/impl/definitions/DefinitionsServiceImpl.java
+++
b/services/src/main/java/org/apache/unomi/services/impl/definitions/DefinitionsServiceImpl.java
@@ -277,7 +277,7 @@ public class DefinitionsServiceImpl implements
DefinitionsService, SynchronousBu
Collection<ConditionType> all =
persistenceService.getAllItems(ConditionType.class);
for (ConditionType type : all) {
if (type != null && type.getParentCondition() != null) {
- ParserHelper.resolveConditionType(this,
type.getParentCondition());
+ ParserHelper.resolveConditionType(this,
type.getParentCondition(), "condition type " + type.getItemId());
}
}
return all;
@@ -296,7 +296,7 @@ public class DefinitionsServiceImpl implements
DefinitionsService, SynchronousBu
List<ConditionType> directConditionTypes =
persistenceService.query(fieldName, fieldValue,null, ConditionType.class);
for (ConditionType type : directConditionTypes) {
if (type.getParentCondition() != null) {
- ParserHelper.resolveConditionType(this,
type.getParentCondition());
+ ParserHelper.resolveConditionType(this,
type.getParentCondition(), "condition type " + type.getItemId());
}
}
conditionTypes.addAll(directConditionTypes);
@@ -316,7 +316,7 @@ public class DefinitionsServiceImpl implements
DefinitionsService, SynchronousBu
}
}
if (type != null && type.getParentCondition() != null) {
- ParserHelper.resolveConditionType(this, type.getParentCondition());
+ ParserHelper.resolveConditionType(this, type.getParentCondition(),
"condition type " + type.getItemId());
}
return type;
}
@@ -516,7 +516,7 @@ public class DefinitionsServiceImpl implements
DefinitionsService, SynchronousBu
@Override
public boolean resolveConditionType(Condition rootCondition) {
- return ParserHelper.resolveConditionType(this, rootCondition);
+ return ParserHelper.resolveConditionType(this, rootCondition,
(rootCondition != null ? "condition type " + rootCondition.getConditionTypeId()
: "unknown"));
}
@Override
diff --git
a/services/src/main/java/org/apache/unomi/services/impl/events/EventServiceImpl.java
b/services/src/main/java/org/apache/unomi/services/impl/events/EventServiceImpl.java
index 25db0b5..3465bfd 100644
---
a/services/src/main/java/org/apache/unomi/services/impl/events/EventServiceImpl.java
+++
b/services/src/main/java/org/apache/unomi/services/impl/events/EventServiceImpl.java
@@ -220,7 +220,7 @@ public class EventServiceImpl implements EventService {
@Override
public PartialList<Event> searchEvents(Condition condition, int offset,
int size) {
- ParserHelper.resolveConditionType(definitionsService, condition);
+ ParserHelper.resolveConditionType(definitionsService, condition,
"event search");
return persistenceService.query(condition, "timeStamp", Event.class,
offset, size);
}
diff --git
a/services/src/main/java/org/apache/unomi/services/impl/goals/GoalsServiceImpl.java
b/services/src/main/java/org/apache/unomi/services/impl/goals/GoalsServiceImpl.java
index cf2788e..de4f4da 100644
---
a/services/src/main/java/org/apache/unomi/services/impl/goals/GoalsServiceImpl.java
+++
b/services/src/main/java/org/apache/unomi/services/impl/goals/GoalsServiceImpl.java
@@ -219,8 +219,8 @@ public class GoalsServiceImpl implements GoalsService,
SynchronousBundleListener
public Goal getGoal(String goalId) {
Goal goal = persistenceService.load(goalId, Goal.class);
if (goal != null) {
- ParserHelper.resolveConditionType(definitionsService,
goal.getStartEvent());
- ParserHelper.resolveConditionType(definitionsService,
goal.getTargetEvent());
+ ParserHelper.resolveConditionType(definitionsService,
goal.getStartEvent(), "goal "+goalId+" start event");
+ ParserHelper.resolveConditionType(definitionsService,
goal.getTargetEvent(), "goal "+goalId+" target event");
}
return goal;
}
@@ -234,8 +234,12 @@ public class GoalsServiceImpl implements GoalsService,
SynchronousBundleListener
@Override
public void setGoal(Goal goal) {
- ParserHelper.resolveConditionType(definitionsService,
goal.getStartEvent());
- ParserHelper.resolveConditionType(definitionsService,
goal.getTargetEvent());
+ if (goal == null) {
+ logger.warn("Trying to save null goal, aborting...");
+ return;
+ }
+ ParserHelper.resolveConditionType(definitionsService,
goal.getStartEvent(), "goal "+goal.getItemId()+" start event");
+ ParserHelper.resolveConditionType(definitionsService,
goal.getTargetEvent(), "goal "+goal.getItemId()+" start event");
if (goal.getMetadata().isEnabled()) {
if (goal.getStartEvent() != null) {
@@ -407,7 +411,7 @@ public class GoalsServiceImpl implements GoalsService,
SynchronousBundleListener
public Campaign getCampaign(String id) {
Campaign campaign = persistenceService.load(id, Campaign.class);
if (campaign != null) {
- ParserHelper.resolveConditionType(definitionsService,
campaign.getEntryCondition());
+ ParserHelper.resolveConditionType(definitionsService,
campaign.getEntryCondition(), "campaign " + id);
}
return campaign;
}
@@ -421,7 +425,7 @@ public class GoalsServiceImpl implements GoalsService,
SynchronousBundleListener
}
public void setCampaign(Campaign campaign) {
- ParserHelper.resolveConditionType(definitionsService,
campaign.getEntryCondition());
+ ParserHelper.resolveConditionType(definitionsService,
campaign.getEntryCondition(), "campaign " + campaign.getItemId());
if(rulesService.getRule(campaign.getMetadata().getId() + "EntryEvent")
!= null) {
rulesService.removeRule(campaign.getMetadata().getId() +
"EntryEvent");
@@ -466,7 +470,7 @@ public class GoalsServiceImpl implements GoalsService,
SynchronousBundleListener
}
if (query != null && query.getCondition() != null) {
- ParserHelper.resolveConditionType(definitionsService,
query.getCondition());
+ ParserHelper.resolveConditionType(definitionsService,
query.getCondition(), "goal " + goalId + " report");
list.add(query.getCondition());
}
diff --git
a/services/src/main/java/org/apache/unomi/services/impl/profiles/ProfileServiceImpl.java
b/services/src/main/java/org/apache/unomi/services/impl/profiles/ProfileServiceImpl.java
index eb63346..20136f4 100644
---
a/services/src/main/java/org/apache/unomi/services/impl/profiles/ProfileServiceImpl.java
+++
b/services/src/main/java/org/apache/unomi/services/impl/profiles/ProfileServiceImpl.java
@@ -763,7 +763,7 @@ public class ProfileServiceImpl implements ProfileService,
SynchronousBundleList
@Override
public boolean matchCondition(Condition condition, Profile profile,
Session session) {
- ParserHelper.resolveConditionType(definitionsService, condition);
+ ParserHelper.resolveConditionType(definitionsService, condition,
"profile " + profile.getItemId() + " matching");
if (condition.getConditionTypeId().equals("booleanCondition")) {
List<Condition> subConditions = (List<Condition>)
condition.getParameter("subConditions");
@@ -788,7 +788,7 @@ public class ProfileServiceImpl implements ProfileService,
SynchronousBundleList
}
public void batchProfilesUpdate(BatchUpdate update) {
- ParserHelper.resolveConditionType(definitionsService,
update.getCondition());
+ ParserHelper.resolveConditionType(definitionsService,
update.getCondition(), "batch update on property " + update.getPropertyName());
List<Profile> profiles =
persistenceService.query(update.getCondition(), null, Profile.class);
for (Profile profile : profiles) {
diff --git
a/services/src/main/java/org/apache/unomi/services/impl/queries/QueryServiceImpl.java
b/services/src/main/java/org/apache/unomi/services/impl/queries/QueryServiceImpl.java
index ca278eb..c9317ee 100644
---
a/services/src/main/java/org/apache/unomi/services/impl/queries/QueryServiceImpl.java
+++
b/services/src/main/java/org/apache/unomi/services/impl/queries/QueryServiceImpl.java
@@ -74,7 +74,7 @@ public class QueryServiceImpl implements QueryService {
@Override
public Map<String, Double> getMetric(String type, String property, String
slashConcatenatedMetrics, Condition condition) {
if (condition.getConditionType() == null) {
- ParserHelper.resolveConditionType(definitionsService, condition);
+ ParserHelper.resolveConditionType(definitionsService, condition,
"metric " + type + " on property " + property);
}
return persistenceService.getSingleValuesMetrics(condition,
slashConcatenatedMetrics.split("/"), property, type);
}
@@ -82,7 +82,7 @@ public class QueryServiceImpl implements QueryService {
@Override
public long getQueryCount(String itemType, Condition condition) {
if (condition.getConditionType() == null) {
- ParserHelper.resolveConditionType(definitionsService, condition);
+ ParserHelper.resolveConditionType(definitionsService, condition,
"query count on " +itemType);
}
return persistenceService.queryCount(condition, itemType);
}
@@ -90,9 +90,7 @@ public class QueryServiceImpl implements QueryService {
private Map<String, Long> getAggregate(String itemType, String property,
AggregateQuery query, boolean optimizedQuery) {
if (query != null) {
// resolve condition
- if (query.getCondition() != null) {
- ParserHelper.resolveConditionType(definitionsService,
query.getCondition());
- }
+ ParserHelper.resolveConditionType(definitionsService,
query.getCondition(), "aggregate on property " + property + " for type " +
itemType);
// resolve aggregate
BaseAggregate baseAggregate = null;
diff --git
a/services/src/main/java/org/apache/unomi/services/impl/rules/RulesServiceImpl.java
b/services/src/main/java/org/apache/unomi/services/impl/rules/RulesServiceImpl.java
index 734d856..42e0210 100644
---
a/services/src/main/java/org/apache/unomi/services/impl/rules/RulesServiceImpl.java
+++
b/services/src/main/java/org/apache/unomi/services/impl/rules/RulesServiceImpl.java
@@ -248,8 +248,8 @@ public class RulesServiceImpl implements RulesService,
EventListenerService, Syn
private List<Rule> getAllRules() {
List<Rule> allItems = persistenceService.getAllItems(Rule.class, 0,
-1, "priority").getList();
for (Rule rule : allItems) {
- ParserHelper.resolveConditionType(definitionsService,
rule.getCondition());
- ParserHelper.resolveActionTypes(definitionsService,
rule.getActions());
+ ParserHelper.resolveConditionType(definitionsService,
rule.getCondition(), "rule " + rule.getItemId());
+ ParserHelper.resolveActionTypes(definitionsService, rule);
}
return allItems;
}
@@ -339,12 +339,8 @@ public class RulesServiceImpl implements RulesService,
EventListenerService, Syn
public Rule getRule(String ruleId) {
Rule rule = persistenceService.load(ruleId, Rule.class);
if (rule != null) {
- if (rule.getCondition() != null) {
- ParserHelper.resolveConditionType(definitionsService,
rule.getCondition());
- }
- if (rule.getActions() != null) {
- ParserHelper.resolveActionTypes(definitionsService,
rule.getActions());
- }
+ ParserHelper.resolveConditionType(definitionsService,
rule.getCondition(), "rule " + rule.getItemId());
+ ParserHelper.resolveActionTypes(definitionsService, rule);
}
return rule;
}
@@ -356,7 +352,7 @@ public class RulesServiceImpl implements RulesService,
EventListenerService, Syn
Condition condition = rule.getCondition();
if (condition != null) {
if (rule.getMetadata().isEnabled() &&
!rule.getMetadata().isMissingPlugins()) {
- ParserHelper.resolveConditionType(definitionsService,
condition);
+ ParserHelper.resolveConditionType(definitionsService,
condition, "rule " + rule.getItemId());
definitionsService.extractConditionBySystemTag(condition,
"eventCondition");
}
}
@@ -373,7 +369,7 @@ public class RulesServiceImpl implements RulesService,
EventListenerService, Syn
if(trackedCondition != null){
Condition sourceEventPropertyCondition =
definitionsService.extractConditionBySystemTag(r.getCondition(),
"sourceEventCondition");
if(source != null && sourceEventPropertyCondition != null) {
- ParserHelper.resolveConditionType(definitionsService,
sourceEventPropertyCondition);
+ ParserHelper.resolveConditionType(definitionsService,
sourceEventPropertyCondition, "rule " + r.getItemId() + " source event
condition");
if(persistenceService.testMatch(sourceEventPropertyCondition, source)){
trackedConditions.add(trackedCondition);
}
diff --git
a/services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java
b/services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java
index 886ad8f..7c03503 100644
---
a/services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java
+++
b/services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java
@@ -253,7 +253,7 @@ public class SegmentServiceImpl extends AbstractServiceImpl
implements SegmentSe
private List<Segment> getAllSegmentDefinitions() {
List<Segment> allItems = persistenceService.getAllItems(Segment.class);
for (Segment segment : allItems) {
- ParserHelper.resolveConditionType(definitionsService,
segment.getCondition());
+ ParserHelper.resolveConditionType(definitionsService,
segment.getCondition(), "segment " + segment.getItemId());
}
return allItems;
}
@@ -261,13 +261,13 @@ public class SegmentServiceImpl extends
AbstractServiceImpl implements SegmentSe
public Segment getSegmentDefinition(String segmentId) {
Segment definition = persistenceService.load(segmentId, Segment.class);
if (definition != null) {
- ParserHelper.resolveConditionType(definitionsService,
definition.getCondition());
+ ParserHelper.resolveConditionType(definitionsService,
definition.getCondition(), "segment " + segmentId);
}
return definition;
}
public void setSegmentDefinition(Segment segment) {
- ParserHelper.resolveConditionType(definitionsService,
segment.getCondition());
+ ParserHelper.resolveConditionType(definitionsService,
segment.getCondition(), "segment " + segment.getItemId());
if (!persistenceService.isValidCondition(segment.getCondition(), new
Profile())) {
throw new BadSegmentConditionException();
}
@@ -543,7 +543,7 @@ public class SegmentServiceImpl extends AbstractServiceImpl
implements SegmentSe
List<Scoring> allItems = persistenceService.getAllItems(Scoring.class);
for (Scoring scoring : allItems) {
for (ScoringElement element : scoring.getElements()) {
- ParserHelper.resolveConditionType(definitionsService,
element.getCondition());
+ ParserHelper.resolveConditionType(definitionsService,
element.getCondition(), "scoring " + scoring.getItemId());
}
}
return allItems;
@@ -553,7 +553,7 @@ public class SegmentServiceImpl extends AbstractServiceImpl
implements SegmentSe
Scoring definition = persistenceService.load(scoringId, Scoring.class);
if (definition != null) {
for (ScoringElement element : definition.getElements()) {
- ParserHelper.resolveConditionType(definitionsService,
element.getCondition());
+ ParserHelper.resolveConditionType(definitionsService,
element.getCondition(), "scoring " + scoringId);
}
}
return definition;
@@ -561,7 +561,7 @@ public class SegmentServiceImpl extends AbstractServiceImpl
implements SegmentSe
public void setScoringDefinition(Scoring scoring) {
for (ScoringElement element : scoring.getElements()) {
- ParserHelper.resolveConditionType(definitionsService,
element.getCondition());
+ ParserHelper.resolveConditionType(definitionsService,
element.getCondition(), "scoring " + scoring.getItemId() + " element ");
}
for (ScoringElement element : scoring.getElements()) {
if (scoring.getMetadata().isEnabled() &&
!scoring.getMetadata().isMissingPlugins()) {
