This is an automated email from the ASF dual-hosted git repository. shuber pushed a commit to branch UNOMI-492-fix-rule-null-actions in repository https://gitbox.apache.org/repos/asf/unomi.git
commit 23017eca2f19808bab6ebd4ec7669dc6b82fad9c Author: Serge Huber <[email protected]> AuthorDate: Thu Jun 24 18:39:24 2021 +0200 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 --- .../org/apache/unomi/itests/RuleServiceIT.java | 85 ++++++++++++++++++++++ .../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, 135 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..2dfc21b --- /dev/null +++ b/itests/src/test/java/org/apache/unomi/itests/RuleServiceIT.java @@ -0,0 +1,85 @@ +/* + * 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 java.util.Set; + +import static org.junit.Assert.assertEquals; + +/** + * 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 { + Set<Metadata> ruleMetadatas = rulesService.getRuleMetadatas(); + int initialRuleCount = ruleMetadatas.size(); + 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); + LOGGER.info("Waiting for rules to refresh from persistence..."); + int loopCount = 0; + int lastRuleCount = initialRuleCount; + while (loopCount < 20 && lastRuleCount == initialRuleCount) { + Thread.sleep(1000); + ruleMetadatas = rulesService.getRuleMetadatas(); + lastRuleCount = ruleMetadatas.size(); + loopCount++; + } + assertEquals("Rule not properly saved", initialRuleCount + 1, lastRuleCount); + } +} 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()) {
