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 <[email protected]> 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; } }
