You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@unomi.apache.org by GitBox <gi...@apache.org> on 2021/03/22 14:59:43 UTC
[GitHub] [unomi] sergehuber commented on a change in pull request #265: UNOMI-446 improve increment action and make it generic
sergehuber commented on a change in pull request #265:
URL: https://github.com/apache/unomi/pull/265#discussion_r598728046
##########
File path: plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/IncrementPropertyAction.java
##########
@@ -16,56 +16,88 @@
*/
package org.apache.unomi.plugins.baseplugin.actions;
+import java.lang.reflect.InvocationTargetException;
import java.util.Date;
import java.util.HashMap;
import java.util.Map;
+import org.apache.commons.beanutils.PropertyUtils;
import org.apache.unomi.api.Event;
import org.apache.unomi.api.Profile;
import org.apache.unomi.api.actions.Action;
import org.apache.unomi.api.actions.ActionExecutor;
import org.apache.unomi.api.services.EventService;
import org.apache.unomi.api.services.TopicService;
+import org.apache.unomi.persistence.spi.PropertyHelper;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-public class IncrementInterestAction implements ActionExecutor {
+public class IncrementPropertyAction implements ActionExecutor {
- private static final Logger LOG = LoggerFactory.getLogger(IncrementInterestAction.class.getName());
+ private static final Logger LOG = LoggerFactory.getLogger(IncrementPropertyAction.class.getName());
private static final String EVENT_INTERESTS_PROPERTY = "interests";
-
private static final String ACTION_INTERESTS_PROPERTY = "eventInterestProperty";
private TopicService topicService;
-
private EventService eventService;
-
private Double interestsMinValue;
-
private Double interestsMaxValue;
-
private Double interestsDividerValue;
@Override
@SuppressWarnings("unchecked")
public int execute(final Action action, final Event event) {
- Map<String, Double> interestsAsMap = (Map<String, Double>) action.getParameterValues().get( ACTION_INTERESTS_PROPERTY );
+ final Profile profile = event.getProfile();
- if ( interestsAsMap == null ) {
- interestsAsMap = (Map<String, Double>) event.getProperty( EVENT_INTERESTS_PROPERTY );
+ Object propertyValue;
+ String propertyName = (String) action.getParameterValues().get("propertyName");
+ String parentPropertyName = propertyName.split("\\.")[0];
Review comment:
What happens if no parent is provided. Isn't the parentPropertyName and the propertyName the same in that case?P
##########
File path: plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/IncrementPropertyAction.java
##########
@@ -16,56 +16,88 @@
*/
package org.apache.unomi.plugins.baseplugin.actions;
+import java.lang.reflect.InvocationTargetException;
import java.util.Date;
import java.util.HashMap;
import java.util.Map;
+import org.apache.commons.beanutils.PropertyUtils;
import org.apache.unomi.api.Event;
import org.apache.unomi.api.Profile;
import org.apache.unomi.api.actions.Action;
import org.apache.unomi.api.actions.ActionExecutor;
import org.apache.unomi.api.services.EventService;
import org.apache.unomi.api.services.TopicService;
+import org.apache.unomi.persistence.spi.PropertyHelper;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-public class IncrementInterestAction implements ActionExecutor {
+public class IncrementPropertyAction implements ActionExecutor {
- private static final Logger LOG = LoggerFactory.getLogger(IncrementInterestAction.class.getName());
+ private static final Logger LOG = LoggerFactory.getLogger(IncrementPropertyAction.class.getName());
private static final String EVENT_INTERESTS_PROPERTY = "interests";
-
private static final String ACTION_INTERESTS_PROPERTY = "eventInterestProperty";
private TopicService topicService;
-
private EventService eventService;
-
private Double interestsMinValue;
-
private Double interestsMaxValue;
-
private Double interestsDividerValue;
@Override
@SuppressWarnings("unchecked")
public int execute(final Action action, final Event event) {
- Map<String, Double> interestsAsMap = (Map<String, Double>) action.getParameterValues().get( ACTION_INTERESTS_PROPERTY );
+ final Profile profile = event.getProfile();
- if ( interestsAsMap == null ) {
- interestsAsMap = (Map<String, Double>) event.getProperty( EVENT_INTERESTS_PROPERTY );
+ Object propertyValue;
+ String propertyName = (String) action.getParameterValues().get("propertyName");
+ String parentPropertyName = propertyName.split("\\.")[0];
- if (interestsAsMap == null) {
- return EventService.NO_CHANGE;
+ try {
+ if (EVENT_INTERESTS_PROPERTY.equals(propertyName)) {
+ propertyValue = incrementInterests(action, event, profile);
Review comment:
After reflection I wonder if we should split this action into 2 separate actions: IncrementInterests and IncrementProperty, because their logic is quite different.
##########
File path: services/src/main/java/org/apache/unomi/services/actions/ActionExecutorDispatcher.java
##########
@@ -124,13 +126,21 @@ public Action getContextualAction(Action action, Event event) {
if (value instanceof String) {
String s = (String) value;
try {
- // check if we have special values
- if (s.contains(VALUE_NAME_SEPARATOR)) {
- final String valueType = StringUtils.substringBefore(s, VALUE_NAME_SEPARATOR);
- final String valueAsString = StringUtils.substringAfter(s, VALUE_NAME_SEPARATOR);
- final ValueExtractor extractor = valueExtractors.get(valueType);
- if (extractor != null) {
- value = extractor.extract(valueAsString, event);
+ if (s.contains(PLACEHOLDER_PREFIX)) {
+ while (s.contains(PLACEHOLDER_PREFIX)) {
+ String substring = s.substring(s.indexOf(PLACEHOLDER_PREFIX) + 2, s.indexOf(PLACEHOLDER_SUFFIX));
Review comment:
What happens if the PLACEHOLDER_SUFFIX is not found, does it raise an exception because indexOf is < 0 ?
##########
File path: plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/IncrementPropertyAction.java
##########
@@ -16,56 +16,88 @@
*/
package org.apache.unomi.plugins.baseplugin.actions;
+import java.lang.reflect.InvocationTargetException;
import java.util.Date;
import java.util.HashMap;
import java.util.Map;
+import org.apache.commons.beanutils.PropertyUtils;
import org.apache.unomi.api.Event;
import org.apache.unomi.api.Profile;
import org.apache.unomi.api.actions.Action;
import org.apache.unomi.api.actions.ActionExecutor;
import org.apache.unomi.api.services.EventService;
import org.apache.unomi.api.services.TopicService;
+import org.apache.unomi.persistence.spi.PropertyHelper;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-public class IncrementInterestAction implements ActionExecutor {
+public class IncrementPropertyAction implements ActionExecutor {
- private static final Logger LOG = LoggerFactory.getLogger(IncrementInterestAction.class.getName());
+ private static final Logger LOG = LoggerFactory.getLogger(IncrementPropertyAction.class.getName());
private static final String EVENT_INTERESTS_PROPERTY = "interests";
-
private static final String ACTION_INTERESTS_PROPERTY = "eventInterestProperty";
private TopicService topicService;
-
private EventService eventService;
-
private Double interestsMinValue;
-
private Double interestsMaxValue;
-
private Double interestsDividerValue;
@Override
@SuppressWarnings("unchecked")
public int execute(final Action action, final Event event) {
- Map<String, Double> interestsAsMap = (Map<String, Double>) action.getParameterValues().get( ACTION_INTERESTS_PROPERTY );
+ final Profile profile = event.getProfile();
- if ( interestsAsMap == null ) {
- interestsAsMap = (Map<String, Double>) event.getProperty( EVENT_INTERESTS_PROPERTY );
+ Object propertyValue;
+ String propertyName = (String) action.getParameterValues().get("propertyName");
+ String parentPropertyName = propertyName.split("\\.")[0];
Review comment:
Maybe it's that's normal, maybe we could rename it to rootPropertyName?
##########
File path: plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/IncrementPropertyAction.java
##########
@@ -16,56 +16,88 @@
*/
package org.apache.unomi.plugins.baseplugin.actions;
+import java.lang.reflect.InvocationTargetException;
import java.util.Date;
import java.util.HashMap;
import java.util.Map;
+import org.apache.commons.beanutils.PropertyUtils;
import org.apache.unomi.api.Event;
import org.apache.unomi.api.Profile;
import org.apache.unomi.api.actions.Action;
import org.apache.unomi.api.actions.ActionExecutor;
import org.apache.unomi.api.services.EventService;
import org.apache.unomi.api.services.TopicService;
+import org.apache.unomi.persistence.spi.PropertyHelper;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-public class IncrementInterestAction implements ActionExecutor {
+public class IncrementPropertyAction implements ActionExecutor {
- private static final Logger LOG = LoggerFactory.getLogger(IncrementInterestAction.class.getName());
+ private static final Logger LOG = LoggerFactory.getLogger(IncrementPropertyAction.class.getName());
private static final String EVENT_INTERESTS_PROPERTY = "interests";
-
private static final String ACTION_INTERESTS_PROPERTY = "eventInterestProperty";
private TopicService topicService;
-
private EventService eventService;
-
private Double interestsMinValue;
-
private Double interestsMaxValue;
-
private Double interestsDividerValue;
@Override
@SuppressWarnings("unchecked")
public int execute(final Action action, final Event event) {
- Map<String, Double> interestsAsMap = (Map<String, Double>) action.getParameterValues().get( ACTION_INTERESTS_PROPERTY );
+ final Profile profile = event.getProfile();
- if ( interestsAsMap == null ) {
- interestsAsMap = (Map<String, Double>) event.getProperty( EVENT_INTERESTS_PROPERTY );
+ Object propertyValue;
+ String propertyName = (String) action.getParameterValues().get("propertyName");
+ String parentPropertyName = propertyName.split("\\.")[0];
- if (interestsAsMap == null) {
- return EventService.NO_CHANGE;
+ try {
+ if (EVENT_INTERESTS_PROPERTY.equals(propertyName)) {
+ propertyValue = incrementInterests(action, event, profile);
Review comment:
I think we should rename this method to processIncrementInterestEvent
##########
File path: itests/src/test/java/org/apache/unomi/itests/IncrementPropertyIT.java
##########
@@ -0,0 +1,305 @@
+/*
+ * 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 java.util.*;
+
+import javax.inject.Inject;
+
+import org.apache.unomi.api.CustomItem;
+import org.apache.unomi.api.Event;
+import org.apache.unomi.api.Metadata;
+import org.apache.unomi.api.Profile;
+import org.apache.unomi.api.Topic;
+import org.apache.unomi.api.actions.Action;
+import org.apache.unomi.api.conditions.Condition;
+import org.apache.unomi.api.rules.Rule;
+import org.apache.unomi.api.services.DefinitionsService;
+import org.apache.unomi.api.services.EventService;
+import org.apache.unomi.api.services.ProfileService;
+import org.apache.unomi.api.services.RulesService;
+import org.apache.unomi.api.services.TopicService;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.ops4j.pax.exam.junit.PaxExam;
+import org.ops4j.pax.exam.spi.reactors.ExamReactorStrategy;
+import org.ops4j.pax.exam.spi.reactors.PerSuite;
+import org.ops4j.pax.exam.util.Filter;
+
+import static org.apache.unomi.itests.BasicIT.ITEM_TYPE_PAGE;
+
+@RunWith(PaxExam.class)
+@ExamReactorStrategy(PerSuite.class)
+public class IncrementPropertyIT extends BaseIT {
+ private Profile profile;
+ private Topic topic;
+ private Rule rule;
+
+ @Inject
+ @Filter(timeout = 600000)
+ protected ProfileService profileService;
+
+ @Inject
+ @Filter(timeout = 600000)
+ protected EventService eventService;
+
+ @Inject
+ @Filter(timeout = 600000)
+ protected TopicService topicService;
+
+ @Inject
+ @Filter(timeout = 600000)
+ protected RulesService rulesService;
+
+ @Inject
+ @Filter(timeout = 600000)
+ protected DefinitionsService definitionsService;
+
+ @Before
+ public void setup() throws Exception {
+ profile = createProfile();
+ topic = createTopic("topicId");
+ rule = new Rule();
+ }
+
+ @After
+ public void tearDown() {
+ rulesService.removeRule(rule.getItemId());
+ topicService.delete(topic.getItemId());
+ profileService.delete(profile.getItemId(), false);
+ }
+
+ @Test
+ public void testIncrementInterests() throws InterruptedException {
+ final Map<String, Double> interestsAsMap = new HashMap<>();
+ interestsAsMap.put(topic.getTopicId(), 50.0);
+ interestsAsMap.put("unknown", 10.0);
+
+ final Event event = createEvent(profile, interestsAsMap);
+
+ int eventCode = eventService.send(event);
+
+ if (eventCode == EventService.PROFILE_UPDATED) {
+ Profile updatedProfile = profileService.save(event.getProfile());
+
+ refreshPersistence();
+
+ Map<String, Double> interests = (Map<String, Double>) updatedProfile.getProperty("interests");
+
+ Assert.assertEquals(0.5, interests.get(topic.getTopicId()), 0.0);
+ Assert.assertFalse(interests.containsKey("unknown"));
+ } else {
+ throw new IllegalStateException("Profile was not updated");
+ }
+ }
+
+ @Test
+ public void testIncrementInterestsByAction() throws InterruptedException {
+ Action incrementPropertyAction = new Action(definitionsService.getActionType("incrementPropertyAction"));
+ incrementPropertyAction.setParameter("propertyName", "interests");
+ incrementPropertyAction.setParameter("eventInterestProperty", "eventProperty::target.properties.interests");
+
+ Condition condition = createCondition();
+ Metadata metadata = createMetadata();
+
+ List<Action> actions = new ArrayList<>();
+ actions.add(incrementPropertyAction);
+
+ rule.setCondition(condition);
+ rule.setActions(actions);
+ rule.setMetadata(metadata);
+
+ rulesService.setRule(rule);
+ refreshPersistence();
+
+ keepTrying("Failed waiting for the creation of the rule for the IncrementPropertyIT test",
+ () -> rulesService.getRule(rule.getItemId()), Objects::nonNull, 1000, 100);
+
+ Map<String, Double> interestsAsMap = new HashMap<>();
+ interestsAsMap.put(topic.getTopicId(), 50.0);
+ interestsAsMap.put("unknown", 10.0);
+
+ Map<String, Object> properties = new HashMap<>();
+
+ properties.put("interests", interestsAsMap);
+
+ CustomItem item = new CustomItem("page", ITEM_TYPE_PAGE);
+ item.setProperties(properties);
+
+ Event event = new Event("view", null, profile, null, null, item, new Date());
+ event.setPersistent(false);
+
+ int eventCode = eventService.send(event);
+ refreshPersistence();
+
+ if (eventCode == EventService.PROFILE_UPDATED) {
+ Profile updatedProfile = profileService.save(event.getProfile());
+
+ refreshPersistence();
+
+ Map<String, Double> interests = (Map<String, Double>) updatedProfile.getProperty("interests");
+
+ Assert.assertEquals(0.5, interests.get(topic.getTopicId()), 0.0);
+ Assert.assertFalse(interests.containsKey("unknown"));
+ } else {
+ throw new IllegalStateException("Profile was not updated");
+ }
+ }
+
+ @Test
+ public void testIncrementPropertyByAction() throws InterruptedException {
+ Action incrementPropertyAction = new Action(definitionsService.getActionType("incrementPropertyAction"));
+ incrementPropertyAction.setParameter("propertyName", "pageView.acme");
+
+ Condition condition = createCondition();
+ Metadata metadata = createMetadata();
+
+ List<Action> actions = new ArrayList<>();
+ actions.add(incrementPropertyAction);
+
+ rule.setCondition(condition);
+ rule.setActions(actions);
+ rule.setMetadata(metadata);
+ rulesService.setRule(rule);
+ refreshPersistence();
+
+ keepTrying("Failed waiting for the creation of the rule for the IncrementPropertyIT test",
+ () -> rulesService.getRule(rule.getItemId()), Objects::nonNull, 1000, 100);
+
+
+ Event event = new Event("view", null, profile, null, null, profile, new Date());
+ event.setPersistent(false);
+
+ int eventCode = eventService.send(event);
+ refreshPersistence();
+
+ if (eventCode == EventService.PROFILE_UPDATED) {
+ Profile updatedProfile = profileService.save(event.getProfile());
+
+ refreshPersistence();
+
+ int value = ((Map<String, Integer>) updatedProfile.getProperty("pageView")).get("acme");
+ Assert.assertEquals(1, value, 0.0);
+ } else {
+ throw new IllegalStateException("Profile was not updated");
+ }
+ }
+
+ @Test
+ public void testIncrementExistingPropertyByAction() throws InterruptedException {
+ Action incrementPropertyAction = new Action(definitionsService.getActionType("incrementPropertyAction"));
Review comment:
Could you please add a test for the case of the ${} marker?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org