You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@unomi.apache.org by jk...@apache.org on 2021/12/03 21:38:23 UTC
[unomi] 01/01: UNOMI-535: cleanup, refacto, stabilization, fixes and new tests for SetPropertyAction, PropertyHelper and IncrementPropertyAction
This is an automated email from the ASF dual-hosted git repository.
jkevan pushed a commit to branch UNOMI-535-setPropertyAction-refacto
in repository https://gitbox.apache.org/repos/asf/unomi.git
commit 9dfcaedbf90ae5c666a8ee8a623b449f9e669448
Author: Kevan <ke...@jahia.com>
AuthorDate: Fri Dec 3 22:38:02 2021 +0100
UNOMI-535: cleanup, refacto, stabilization, fixes and new tests for SetPropertyAction, PropertyHelper and IncrementPropertyAction
---
.../test/java/org/apache/unomi/itests/BaseIT.java | 16 +++
.../test/java/org/apache/unomi/itests/BasicIT.java | 4 +-
.../unomi/itests/CopyPropertiesActionIT.java | 3 +-
.../unomi/itests/GroovyActionsServiceIT.java | 3 +-
.../apache/unomi/itests/IncrementInterestsIT.java | 3 +-
.../apache/unomi/itests/IncrementPropertyIT.java | 3 +-
.../org/apache/unomi/itests/ProfileMergeIT.java | 9 +-
.../unomi/itests/PropertiesUpdateActionIT.java | 4 +-
.../org/apache/unomi/itests/RuleServiceIT.java | 17 +--
persistence-spi/pom.xml | 13 ++
.../unomi/persistence/spi/PropertyHelper.java | 56 ++++-----
.../unomi/persistence/PropertyHelperTest.java | 131 +++++++++++++++++++++
.../actions/IncrementPropertyAction.java | 36 ++++--
.../baseplugin/actions/SetPropertyAction.java | 70 +++++------
14 files changed, 263 insertions(+), 105 deletions(-)
diff --git a/itests/src/test/java/org/apache/unomi/itests/BaseIT.java b/itests/src/test/java/org/apache/unomi/itests/BaseIT.java
index 81d28da..427b309 100644
--- a/itests/src/test/java/org/apache/unomi/itests/BaseIT.java
+++ b/itests/src/test/java/org/apache/unomi/itests/BaseIT.java
@@ -21,7 +21,9 @@ import com.fasterxml.jackson.databind.ObjectMapper;
import org.apache.commons.io.IOUtils;
import org.apache.unomi.api.Item;
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.RulesService;
import org.apache.unomi.lifecycle.BundleWatcher;
import org.apache.unomi.persistence.spi.CustomObjectMapper;
import org.apache.unomi.persistence.spi.PersistenceService;
@@ -81,6 +83,10 @@ public abstract class BaseIT {
@Inject
@Filter(timeout = 600000)
+ protected RulesService rulesService;
+
+ @Inject
+ @Filter(timeout = 600000)
protected DefinitionsService definitionsService;
@Inject
@@ -339,4 +345,14 @@ public abstract class BaseIT {
return bundleContext.getService(serviceReference);
}
+ public void createAndWaitForRule(Rule rule) throws InterruptedException {
+ rulesService.setRule(rule);
+ refreshPersistence();
+ keepTrying("Failed waiting for rule to be saved",
+ () -> rulesService.getRule(rule.getMetadata().getId()),
+ Objects::nonNull,
+ 3000,
+ 100);
+ rulesService.refreshRules();
+ }
}
diff --git a/itests/src/test/java/org/apache/unomi/itests/BasicIT.java b/itests/src/test/java/org/apache/unomi/itests/BasicIT.java
index dd95038..e726ce4 100644
--- a/itests/src/test/java/org/apache/unomi/itests/BasicIT.java
+++ b/itests/src/test/java/org/apache/unomi/itests/BasicIT.java
@@ -158,9 +158,7 @@ public class BasicIT extends BaseIT {
// Add login rule
Rule rule = CustomObjectMapper.getObjectMapper().readValue(new File("data/tmp/testLogin.json").toURI().toURL(),
Rule.class);
- rulesService.setRule(rule);
- Thread.sleep(2000);
- refreshPersistence();
+ createAndWaitForRule(rule);
CustomItem sourceSite = new CustomItem(ITEM_ID_SITE, ITEM_TYPE_SITE);
sourceSite.setScope(TEST_SCOPE);
diff --git a/itests/src/test/java/org/apache/unomi/itests/CopyPropertiesActionIT.java b/itests/src/test/java/org/apache/unomi/itests/CopyPropertiesActionIT.java
index 88daecc..c514550 100644
--- a/itests/src/test/java/org/apache/unomi/itests/CopyPropertiesActionIT.java
+++ b/itests/src/test/java/org/apache/unomi/itests/CopyPropertiesActionIT.java
@@ -165,8 +165,7 @@ public class CopyPropertiesActionIT extends BaseIT {
private void createRule(String filename) throws IOException, InterruptedException {
Rule rule = CustomObjectMapper.getObjectMapper().readValue(new File(filename).toURI().toURL(), Rule.class);
- rulesService.setRule(rule);
- Thread.sleep(2000);
+ createAndWaitForRule(rule);
}
private Event sendCopyPropertyEvent(Map<String, Object> properties, String profileType) {
diff --git a/itests/src/test/java/org/apache/unomi/itests/GroovyActionsServiceIT.java b/itests/src/test/java/org/apache/unomi/itests/GroovyActionsServiceIT.java
index 6fb794c..d34734a 100644
--- a/itests/src/test/java/org/apache/unomi/itests/GroovyActionsServiceIT.java
+++ b/itests/src/test/java/org/apache/unomi/itests/GroovyActionsServiceIT.java
@@ -100,8 +100,7 @@ public class GroovyActionsServiceIT extends BaseIT {
private void createRule(String filename) throws IOException, InterruptedException {
Rule rule = CustomObjectMapper.getObjectMapper().readValue(new File(filename).toURI().toURL(), Rule.class);
- rulesService.setRule(rule);
- Thread.sleep(2000);
+ createAndWaitForRule(rule);
}
private Event sendGroovyActionEvent() {
diff --git a/itests/src/test/java/org/apache/unomi/itests/IncrementInterestsIT.java b/itests/src/test/java/org/apache/unomi/itests/IncrementInterestsIT.java
index a0c3e92..93af871 100644
--- a/itests/src/test/java/org/apache/unomi/itests/IncrementInterestsIT.java
+++ b/itests/src/test/java/org/apache/unomi/itests/IncrementInterestsIT.java
@@ -131,8 +131,7 @@ public class IncrementInterestsIT extends BaseIT {
rule.setActions(actions);
rule.setMetadata(metadata);
- rulesService.setRule(rule);
- refreshPersistence();
+ createAndWaitForRule(rule);
Map<String, Double> interestsAsMap = new HashMap<>();
interestsAsMap.put(topic.getTopicId(), 50.0);
diff --git a/itests/src/test/java/org/apache/unomi/itests/IncrementPropertyIT.java b/itests/src/test/java/org/apache/unomi/itests/IncrementPropertyIT.java
index 35999a0..3b75ce7 100644
--- a/itests/src/test/java/org/apache/unomi/itests/IncrementPropertyIT.java
+++ b/itests/src/test/java/org/apache/unomi/itests/IncrementPropertyIT.java
@@ -370,8 +370,7 @@ public class IncrementPropertyIT extends BaseIT {
rule.setCondition(condition);
rule.setActions(actions);
rule.setMetadata(metadata);
- rulesService.setRule(rule);
- refreshPersistence();
+ createAndWaitForRule(rule);
}
private int buildActionAndSendEvent(String propertyName, String propertyTargetName, Map<String, Object> properties, Map<String, Object> targetProperties) throws InterruptedException {
diff --git a/itests/src/test/java/org/apache/unomi/itests/ProfileMergeIT.java b/itests/src/test/java/org/apache/unomi/itests/ProfileMergeIT.java
index 9e451d6..4408510 100644
--- a/itests/src/test/java/org/apache/unomi/itests/ProfileMergeIT.java
+++ b/itests/src/test/java/org/apache/unomi/itests/ProfileMergeIT.java
@@ -73,8 +73,7 @@ public class ProfileMergeIT extends BaseIT {
@Test
public void testProfileMergeOnPropertyAction_dont_forceEventProfileAsMaster() throws InterruptedException {
- rulesService.setRule(createMergeOnPropertyRule(false));
- Thread.sleep(2000); // sleep for rule to be loaded by the Task
+ createAndWaitForRule(createMergeOnPropertyRule(false));
// A new profile should be created.
Assert.assertNotEquals(sendEvent().getProfile().getItemId(), TEST_PROFILE_ID);
@@ -82,8 +81,7 @@ public class ProfileMergeIT extends BaseIT {
@Test
public void testProfileMergeOnPropertyAction_forceEventProfileAsMaster() throws InterruptedException {
- rulesService.setRule(createMergeOnPropertyRule(true));
- Thread.sleep(2000); // sleep for rule to be loaded by the Task
+ createAndWaitForRule(createMergeOnPropertyRule(true));
// No new profile should be created, instead the profile of the event should be used.
Assert.assertEquals(sendEvent().getProfile().getItemId(), TEST_PROFILE_ID);
@@ -105,8 +103,7 @@ public class ProfileMergeIT extends BaseIT {
rule.setCondition(condition);
rule.setActions(Collections.singletonList(action));
- rulesService.setRule(rule);
- refreshPersistence();
+ createAndWaitForRule(rule);
// create master profile
Profile masterProfile = new Profile();
diff --git a/itests/src/test/java/org/apache/unomi/itests/PropertiesUpdateActionIT.java b/itests/src/test/java/org/apache/unomi/itests/PropertiesUpdateActionIT.java
index 1220e5f..f671273 100644
--- a/itests/src/test/java/org/apache/unomi/itests/PropertiesUpdateActionIT.java
+++ b/itests/src/test/java/org/apache/unomi/itests/PropertiesUpdateActionIT.java
@@ -71,6 +71,7 @@ public class PropertiesUpdateActionIT extends BaseIT {
profile.setProperties(new HashMap<>());
profile.setProperty("lastName", "Jose"); // property that have a propertyType registered in the system
profile.setProperty("prop4", "New property 4"); // property that do not have a propertyType registered in the system
+ profile.setProperty("prop4", "New property 4"); // property that do not have a propertyType registered in the system
profileService.save(profile);
LOGGER.info("Profile saved with ID [{}].", profile.getItemId());
@@ -322,8 +323,7 @@ public class PropertiesUpdateActionIT extends BaseIT {
// register test rule
Rule rule = CustomObjectMapper.getObjectMapper().readValue(getValidatedBundleJSON("testSetPropertyActionRule.json", new HashMap<>()), Rule.class);
- rulesService.setRule(rule);
- Thread.sleep(2000);
+ createAndWaitForRule(rule);
try {
Profile profile = profileService.load(PROFILE_TEST_ID);
diff --git a/itests/src/test/java/org/apache/unomi/itests/RuleServiceIT.java b/itests/src/test/java/org/apache/unomi/itests/RuleServiceIT.java
index 67d30e5..71eb9fd 100644
--- a/itests/src/test/java/org/apache/unomi/itests/RuleServiceIT.java
+++ b/itests/src/test/java/org/apache/unomi/itests/RuleServiceIT.java
@@ -77,9 +77,7 @@ public class RuleServiceIT extends BaseIT {
Rule nullRule = new Rule(metadata);
nullRule.setCondition(null);
nullRule.setActions(null);
- rulesService.setRule(nullRule);
- refreshPersistence();
- nullRule = rulesService.getRule(TEST_RULE_ID);
+ createAndWaitForRule(nullRule);
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());
@@ -94,7 +92,7 @@ public class RuleServiceIT extends BaseIT {
ConditionBuilder builder = new ConditionBuilder(definitionsService);
Rule simpleEventTypeRule = new Rule(new Metadata(TEST_SCOPE, "simple-event-type-rule", "Simple event type rule", "A rule with a simple condition to match an event type"));
simpleEventTypeRule.setCondition(builder.condition("eventTypeCondition").parameter("eventTypeId", "view").build());
- rulesService.setRule(simpleEventTypeRule);
+ createAndWaitForRule(simpleEventTypeRule);
Rule complexEventTypeRule = new Rule(new Metadata(TEST_SCOPE, "complex-event-type-rule", "Complex event type rule", "A rule with a complex condition to match multiple event types with negations"));
complexEventTypeRule.setCondition(
builder.not(
@@ -104,10 +102,7 @@ public class RuleServiceIT extends BaseIT {
)
).build()
);
- rulesService.setRule(complexEventTypeRule);
-
- refreshPersistence();
- rulesService.refreshRules();
+ createAndWaitForRule(complexEventTypeRule);
Profile profile = new Profile(UUID.randomUUID().toString());
Session session = new Session(UUID.randomUUID().toString(), profile, new Date(), TEST_SCOPE);
@@ -200,7 +195,7 @@ public class RuleServiceIT extends BaseIT {
trackedCondition.setParameter("referrer", "https://unomi.apache.org");
trackedCondition.getConditionType().getMetadata().getSystemTags().add("trackedCondition");
trackParameterRule.setCondition(trackedCondition);
- rulesService.setRule(trackParameterRule);
+ createAndWaitForRule(trackParameterRule);
// Add rule that has a trackParameter condition that does not match
Rule unTrackParameterRule = new Rule(new Metadata(TEST_SCOPE, "not-tracked-parameter-rule", "Not Tracked parameter rule", "A rule that has a parameter not tracked"));
Condition unTrackedCondition = builder.condition("clickEventCondition").build();
@@ -208,9 +203,7 @@ public class RuleServiceIT extends BaseIT {
unTrackedCondition.setParameter("referrer", "https://localhost");
unTrackedCondition.getConditionType().getMetadata().getSystemTags().add("trackedCondition");
unTrackParameterRule.setCondition(unTrackedCondition);
- rulesService.setRule(unTrackParameterRule);
- refreshPersistence();
- rulesService.refreshRules();
+ createAndWaitForRule(unTrackParameterRule);
// Check that the given event return the tracked condition
Profile profile = new Profile(UUID.randomUUID().toString());
Session session = new Session(UUID.randomUUID().toString(), profile, new Date(), TEST_SCOPE);
diff --git a/persistence-spi/pom.xml b/persistence-spi/pom.xml
index 12bcf7d..17477cc 100644
--- a/persistence-spi/pom.xml
+++ b/persistence-spi/pom.xml
@@ -61,6 +61,19 @@
<artifactId>commons-beanutils</artifactId>
</dependency>
+ <!-- Unit tests -->
+ <dependency>
+ <groupId>junit</groupId>
+ <artifactId>junit</artifactId>
+ <version>4.11</version>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.slf4j</groupId>
+ <artifactId>slf4j-simple</artifactId>
+ <version>1.6.6</version>
+ <scope>test</scope>
+ </dependency>
</dependencies>
</project>
diff --git a/persistence-spi/src/main/java/org/apache/unomi/persistence/spi/PropertyHelper.java b/persistence-spi/src/main/java/org/apache/unomi/persistence/spi/PropertyHelper.java
index 6315025..33735ac 100644
--- a/persistence-spi/src/main/java/org/apache/unomi/persistence/spi/PropertyHelper.java
+++ b/persistence-spi/src/main/java/org/apache/unomi/persistence/spi/PropertyHelper.java
@@ -39,6 +39,7 @@ public class PropertyHelper {
public static boolean setProperty(Object target, String propertyName, Object propertyValue, String setPropertyStrategy) {
try {
+ // Handle remove
String parentPropertyName;
if (setPropertyStrategy != null && setPropertyStrategy.equals("remove")) {
if (resolver.hasNested(propertyName)) {
@@ -61,6 +62,13 @@ public class PropertyHelper {
}
return false;
}
+
+ // Leave now, next strategies require a propertyValue, if no propertyValue, nothing to update.
+ if (propertyValue == null) {
+ return false;
+ }
+
+ // Resolve propertyName
while (resolver.hasNested(propertyName)) {
Object v = PropertyUtils.getProperty(target, resolver.next(propertyName));
if (v == null) {
@@ -71,40 +79,32 @@ public class PropertyHelper {
target = v;
}
- if (setPropertyStrategy != null && setPropertyStrategy.equals("addValue")) {
- Object previousValue = PropertyUtils.getProperty(target, propertyName);
- List<Object> values = new ArrayList<>();
- if (previousValue != null && previousValue instanceof List) {
- values.addAll((List) previousValue);
- } else if (previousValue != null) {
- values.add(previousValue);
- }
- if (!values.contains(propertyValue)) {
- values.add(propertyValue);
- BeanUtils.setProperty(target, propertyName, values);
+ // Get previous value
+ Object previousValue = PropertyUtils.getProperty(target, propertyName);
+
+ // Handle strategies
+ if (setPropertyStrategy == null ||
+ setPropertyStrategy.equals("alwaysSet") ||
+ (setPropertyStrategy.equals("setIfMissing") && previousValue == null)) {
+ if (!compareValues(propertyValue, previousValue)) {
+ BeanUtils.setProperty(target, propertyName, propertyValue);
return true;
}
- }
- if (setPropertyStrategy != null && setPropertyStrategy.equals("addValues")) {
- Object newValues = propertyValue;
- List<Object> newValuesList = convertToList(newValues);
-
- Object previousValue = PropertyUtils.getProperty(target, propertyName);
+ } else if (setPropertyStrategy.equals("addValue") || setPropertyStrategy.equals("addValues")) {
+ List<Object> newValuesList = convertToList(propertyValue);
List<Object> previousValueList = convertToList(previousValue);
newValuesList.addAll(previousValueList);
- Set<Object> propertiesSet = new HashSet<>(newValuesList);
- List<Object> propertiesList = Arrays.asList(propertiesSet.toArray());
-
- BeanUtils.setProperty(target, propertyName, propertiesList);
- return true;
+ Set<Object> newValuesSet = new HashSet<>(newValuesList);
+ if (newValuesSet.size() != previousValueList.size()) {
+ BeanUtils.setProperty(target, propertyName, Arrays.asList(newValuesSet.toArray()));
+ return true;
+ }
+ } else if (setPropertyStrategy.equals("removeValue") || setPropertyStrategy.equals("removeValues")) {
+ List<Object> previousValueList = convertToList(previousValue);
- }
- else if (propertyValue != null && !compareValues(propertyValue, BeanUtils.getProperty(target, propertyName))) {
- if (setPropertyStrategy == null ||
- setPropertyStrategy.equals("alwaysSet") ||
- (setPropertyStrategy.equals("setIfMissing") && BeanUtils.getProperty(target, propertyName) == null)) {
- BeanUtils.setProperty(target, propertyName, propertyValue);
+ if (previousValueList.removeAll(convertToList(propertyValue))) {
+ BeanUtils.setProperty(target, propertyName, previousValueList);
return true;
}
}
diff --git a/persistence-spi/src/test/java/org/apache/unomi/persistence/PropertyHelperTest.java b/persistence-spi/src/test/java/org/apache/unomi/persistence/PropertyHelperTest.java
new file mode 100644
index 0000000..3ee01b3
--- /dev/null
+++ b/persistence-spi/src/test/java/org/apache/unomi/persistence/PropertyHelperTest.java
@@ -0,0 +1,131 @@
+/*
+ * 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.persistence;
+
+import org.apache.unomi.api.Profile;
+import org.apache.unomi.persistence.spi.PropertyHelper;
+import org.junit.Test;
+
+import java.util.Arrays;
+import java.util.List;
+
+import static org.junit.Assert.*;
+
+public class PropertyHelperTest {
+ @Test
+ public void testStrategy_Remove() {
+ Profile profile = new Profile();
+ profile.setProperty("test", "test");
+ boolean updated = PropertyHelper.setProperty(profile, "properties.test", null, "remove");
+ assertNull("Profile property should be removed", profile.getProperty("test"));
+ assertTrue("Should return updated", updated);
+
+ // Removing non existing prop should do nothing
+ updated = PropertyHelper.setProperty(profile, "properties.test", null, "remove");
+ assertNull("Profile property should not exist", profile.getProperty("test"));
+ assertFalse("Should return not updated", updated);
+ }
+
+ @Test
+ public void testStrategy_Null_AlwaysSet_SetIfMissing() {
+ Profile profile = new Profile();
+ profile.setProperty("test", "test");
+ boolean updated = PropertyHelper.setProperty(profile, "properties.test", "test updated", null);
+ assertEquals("Profile property should be updated", "test updated", profile.getProperty("test"));
+ assertTrue("Should return updated", updated);
+
+ updated = PropertyHelper.setProperty(profile, "properties.test", "test updated 2", "alwaysSet");
+ assertEquals("Profile property should be updated", "test updated 2", profile.getProperty("test"));
+ assertTrue("Should return updated", updated);
+
+ updated = PropertyHelper.setProperty(profile, "properties.test", "test updated 3", "setIfMissing");
+ assertEquals("Profile property should not be updated", "test updated 2", profile.getProperty("test"));
+ assertFalse("Should return not updated", updated);
+
+ updated = PropertyHelper.setProperty(profile, "properties.testMissing", "test missing", "setIfMissing");
+ assertEquals("Profile property should be updated", "test missing", profile.getProperty("testMissing"));
+ assertTrue("Should return updated", updated);
+ }
+
+ @Test
+ public void testStrategy_AddValue() {
+ Profile profile = new Profile();
+ profile.setProperty("test", Arrays.asList("value 1"));
+
+ // test add 1 element
+ boolean updated = PropertyHelper.setProperty(profile, "properties.test", "value 2", "addValue");
+ assertList(profile, "test", Arrays.asList("value 1", "value 2"));
+ assertTrue("Should return updated", updated);
+
+ // test element are not added twice
+ updated = PropertyHelper.setProperty(profile, "properties.test", "value 2", "addValue");
+ assertList(profile, "test", Arrays.asList("value 1", "value 2"));
+ assertFalse("Should return not be updated", updated);
+
+ // test add multiple elements
+ updated = PropertyHelper.setProperty(profile, "properties.test", Arrays.asList("value 2", "value 3", "value 4"), "addValues");
+ assertList(profile, "test", Arrays.asList("value 1", "value 2", "value 3", "value 4"));
+ assertTrue("Should return updated", updated);
+
+ // test element are not added twice
+ updated = PropertyHelper.setProperty(profile, "properties.test", Arrays.asList("value 2", "value 3", "value 4"), "addValues");
+ assertList(profile, "test", Arrays.asList("value 1", "value 2", "value 3", "value 4"));
+ assertFalse("Should return not be updated", updated);
+
+ // test add values to non existing prop
+ updated = PropertyHelper.setProperty(profile, "properties.newProp", "value 1", "addValue");
+ assertList(profile, "newProp", Arrays.asList("value 1"));
+ assertTrue("Should return updated", updated);
+ }
+
+ @Test
+ public void testStrategy_RemoveValue() {
+ Profile profile = new Profile();
+ profile.setProperty("test", Arrays.asList("value 1", "value 2", "value 3", "value 4", "value 5"));
+
+ // test remove 1 element
+ boolean updated = PropertyHelper.setProperty(profile, "properties.test", "value 5", "removeValue");
+ assertList(profile, "test", Arrays.asList("value 1", "value 2", "value 3", "value 4"));
+ assertTrue("Should return updated", updated);
+
+ // test remove 1 element that doesnt exist in the list
+ updated = PropertyHelper.setProperty(profile, "properties.test", "value 5", "removeValue");
+ assertList(profile, "test", Arrays.asList("value 1", "value 2", "value 3", "value 4"));
+ assertFalse("Should return not be updated", updated);
+
+ // test remove multiple elements
+ updated = PropertyHelper.setProperty(profile, "properties.test", Arrays.asList("value 3", "value 4"), "removeValues");
+ assertList(profile, "test", Arrays.asList("value 1", "value 2"));
+ assertTrue("Should return updated", updated);
+
+ // test remove multiple element that doesnt exist
+ updated = PropertyHelper.setProperty(profile, "properties.test", Arrays.asList("value 3", "value 4"), "removeValues");
+ assertList(profile, "test", Arrays.asList("value 1", "value 2"));
+ assertFalse("Should return not be updated", updated);
+
+ // test remove values to non existing prop
+ updated = PropertyHelper.setProperty(profile, "properties.newProp", "value 1", "removeValue");
+ assertNull("Profile property should not exist", profile.getProperty("newProp"));
+ assertFalse("Should return not updated", updated);
+ }
+
+ private void assertList(Profile profile, String propertyName, List<String> expectedList) {
+ List<String> currentValue = (List<String>) profile.getProperty(propertyName);
+ assertTrue("The list is not containing the expected elements", currentValue.containsAll(expectedList));
+ assertEquals("The list size does not match the expected list size", expectedList.size(), currentValue.size());
+ }
+}
diff --git a/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/IncrementPropertyAction.java b/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/IncrementPropertyAction.java
index 3651ece..c4b1dfe 100644
--- a/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/IncrementPropertyAction.java
+++ b/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/IncrementPropertyAction.java
@@ -27,6 +27,7 @@ import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import java.lang.reflect.InvocationTargetException;
+import java.util.HashMap;
import java.util.Map;
public class IncrementPropertyAction implements ActionExecutor {
@@ -96,15 +97,25 @@ public class IncrementPropertyAction implements ActionExecutor {
if (nestedPropertyValue == null) {
propertyValue = propertyTargetValue;
} else if (nestedPropertyValue instanceof Map) {
+ // Create a new map to avoid modifying the original Object
+ Map<String, Object> newPropertyValue = new HashMap<>();
Map<String, Object> nestedProperty = (Map<String, Object>) nestedPropertyValue;
+ // increment with target
((Map<String, Object>) propertyTargetValue).forEach((key, targetValue) -> {
if ((targetValue instanceof Integer && (nestedProperty.containsKey(key) && nestedProperty.get(key) instanceof Integer)) ||
(targetValue instanceof Integer && !nestedProperty.containsKey(key))) {
- nestedProperty.put(key, nestedProperty.containsKey(key) ? (int) nestedProperty.get(key) + (int) targetValue : targetValue);
+ newPropertyValue.put(key, nestedProperty.containsKey(key) ? (int) nestedProperty.get(key) + (int) targetValue : targetValue);
}
});
- propertyValue = nestedProperty;
+
+ // add original props that was not incremented
+ nestedProperty.forEach((key, nestedValue) -> {
+ if (!newPropertyValue.containsKey(key)) {
+ newPropertyValue.put(key, nestedValue);
+ }
+ });
+ propertyValue = newPropertyValue;
} else {
throw new IllegalStateException("Property: " + propertyName + " already exist, can not increment the properties from the map because the exiting property is not map");
}
@@ -114,18 +125,17 @@ public class IncrementPropertyAction implements ActionExecutor {
}
} else {
if (properties.containsKey(rootPropertyName)) {
- Object nestedProperty = PropertyUtils.getNestedProperty(properties, propertyName);
- if (nestedProperty == null) {
+ Object nestedPropertyValue = PropertyUtils.getNestedProperty(properties, propertyName);
+ if (nestedPropertyValue == null) {
propertyValue = 1;
- } else if (nestedProperty instanceof Integer) {
- propertyValue = (int) nestedProperty + 1;
- } else if (nestedProperty instanceof Map) {
- ((Map<String, Object>) nestedProperty).forEach((key, propValue) -> {
- if (propValue instanceof Integer) {
- ((Map<String, Integer>) nestedProperty).merge(key, 1, Integer::sum);
- }
- });
- propertyValue = nestedProperty;
+ } else if (nestedPropertyValue instanceof Integer) {
+ propertyValue = (int) nestedPropertyValue + 1;
+ } else if (nestedPropertyValue instanceof Map) {
+ // Create a new map to avoid modifying the original object
+ Map<String, Object> newPropertyValue = new HashMap<>();
+ Map<String, Object> nestedProperty = (Map<String, Object>) nestedPropertyValue;
+ nestedProperty.forEach((key, propValue) -> newPropertyValue.put(key, propValue instanceof Integer ? (int) propValue + 1 : propValue));
+ propertyValue = newPropertyValue;
} else {
throw new IllegalStateException("Property: " + propertyName + " already exist, can not increment the property because the exiting property is not integer or map");
}
diff --git a/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/SetPropertyAction.java b/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/SetPropertyAction.java
index f518dba..23d2364 100644
--- a/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/SetPropertyAction.java
+++ b/plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/SetPropertyAction.java
@@ -25,7 +25,6 @@ import org.apache.unomi.persistence.spi.PropertyHelper;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-import java.text.ParseException;
import java.text.SimpleDateFormat;
import java.util.Date;
import java.util.HashMap;
@@ -50,7 +49,43 @@ public class SetPropertyAction implements ActionExecutor {
}
String propertyName = (String) action.getParameterValues().get("setPropertyName");
+ Object propertyValue = getPropertyValue(action, event);
+ if (storeInSession) {
+ // in the case of session storage we directly update the session
+ if (PropertyHelper.setProperty(event.getSession(), propertyName, propertyValue, (String) action.getParameterValues().get("setPropertyStrategy"))) {
+ return EventService.SESSION_UPDATED;
+ }
+ } else {
+ if (useEventToUpdateProfile) {
+ // in the case of profile storage we use the update profile properties event instead.
+ Map<String, Object> propertyToUpdate = new HashMap<>();
+ propertyToUpdate.put(propertyName, propertyValue);
+
+ Event updateProperties = new Event("updateProperties", event.getSession(), event.getProfile(), event.getSourceId(), null, event.getProfile(), new Date());
+ updateProperties.setPersistent(false);
+
+ updateProperties.setProperty(UpdatePropertiesAction.PROPS_TO_UPDATE, propertyToUpdate);
+ int changes = eventService.send(updateProperties);
+
+ if ((changes & EventService.PROFILE_UPDATED) == EventService.PROFILE_UPDATED) {
+ return EventService.PROFILE_UPDATED;
+ }
+ } else {
+ if (PropertyHelper.setProperty(event.getProfile(), propertyName, propertyValue, (String) action.getParameterValues().get("setPropertyStrategy"))) {
+ return EventService.PROFILE_UPDATED;
+ }
+ }
+ }
+
+ return EventService.NO_CHANGE;
+ }
+
+ public void setEventService(EventService eventService) {
+ this.eventService = eventService;
+ }
+
+ private Object getPropertyValue(Action action, Event event) {
Object propertyValue = action.getParameterValues().get("setPropertyValue");
if (propertyValue == null) {
propertyValue = action.getParameterValues().get("setPropertyValueMultiple");
@@ -88,38 +123,7 @@ public class SetPropertyAction implements ActionExecutor {
propertyValue = format.format(event.getTimeStamp());
}
- if (storeInSession) {
- // in the case of session storage we directly update the session
- if (PropertyHelper.setProperty(event.getSession(), propertyName, propertyValue, (String) action.getParameterValues().get("setPropertyStrategy"))) {
- return EventService.SESSION_UPDATED;
- }
- } else {
- if (useEventToUpdateProfile) {
- // in the case of profile storage we use the update profile properties event instead.
- Map<String, Object> propertyToUpdate = new HashMap<>();
- propertyToUpdate.put(propertyName, propertyValue);
-
- Event updateProperties = new Event("updateProperties", event.getSession(), event.getProfile(), event.getSourceId(), null, event.getProfile(), new Date());
- updateProperties.setPersistent(false);
-
- updateProperties.setProperty(UpdatePropertiesAction.PROPS_TO_UPDATE, propertyToUpdate);
- int changes = eventService.send(updateProperties);
-
- if ((changes & EventService.PROFILE_UPDATED) == EventService.PROFILE_UPDATED) {
- return EventService.PROFILE_UPDATED;
- }
- } else {
- if (PropertyHelper.setProperty(event.getProfile(), propertyName, propertyValue, (String) action.getParameterValues().get("setPropertyStrategy"))) {
- return EventService.PROFILE_UPDATED;
- }
- }
- }
-
- return EventService.NO_CHANGE;
- }
-
- public void setEventService(EventService eventService) {
- this.eventService = eventService;
+ return propertyValue;
}
}