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:22 UTC
[unomi] branch UNOMI-535-setPropertyAction-refacto created (now 9dfcaed)
This is an automated email from the ASF dual-hosted git repository.
jkevan pushed a change to branch UNOMI-535-setPropertyAction-refacto
in repository https://gitbox.apache.org/repos/asf/unomi.git.
at 9dfcaed UNOMI-535: cleanup, refacto, stabilization, fixes and new tests for SetPropertyAction, PropertyHelper and IncrementPropertyAction
This branch includes the following new commits:
new 9dfcaed UNOMI-535: cleanup, refacto, stabilization, fixes and new tests for SetPropertyAction, PropertyHelper and IncrementPropertyAction
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
[unomi] 01/01: UNOMI-535: cleanup, refacto, stabilization, fixes and new tests for SetPropertyAction, PropertyHelper and IncrementPropertyAction
Posted by jk...@apache.org.
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;
}
}