You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@unomi.apache.org by sh...@apache.org on 2021/06/28 11:05:34 UTC
[unomi] branch master updated: UNOMI-492 Make rules engine more
robust when some rules are invalid (null actions) (#314)
This is an automated email from the ASF dual-hosted git repository.
shuber pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/unomi.git
The following commit(s) were added to refs/heads/master by this push:
new 0fa7cc1 UNOMI-492 Make rules engine more robust when some rules are invalid (null actions) (#314)
0fa7cc1 is described below
commit 0fa7cc10e8452611025bbca4e3b858a4ed4d67f9
Author: Serge Huber <sh...@jahia.com>
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
---
.../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 04a7674..8cb4e8f 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
@@ -276,7 +276,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;
@@ -295,7 +295,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);
@@ -315,7 +315,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;
}
@@ -515,7 +515,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 a163d32..ced1351 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
@@ -312,7 +312,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 4a802ba..fcf3afc 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
@@ -215,8 +215,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;
}
@@ -230,8 +230,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) {
@@ -398,7 +402,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;
}
@@ -412,7 +416,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");
@@ -457,7 +461,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 38356cf..90c0c86 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
@@ -796,7 +796,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");
@@ -821,7 +821,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 8ccc767..4c601d2 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
@@ -243,8 +243,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;
}
@@ -334,12 +334,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;
}
@@ -351,7 +347,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");
}
}
@@ -368,7 +364,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 385ab08..fba8e16 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
@@ -243,7 +243,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;
}
@@ -251,13 +251,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();
}
@@ -533,7 +533,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;
@@ -543,7 +543,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;
@@ -551,7 +551,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()) {