jkevan commented on a change in pull request #268: URL: https://github.com/apache/unomi/pull/268#discussion_r601479704
########## File path: plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/CopyPropertiesAction.java ########## @@ -0,0 +1,116 @@ +/* + * 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.plugins.baseplugin.actions; + +import org.apache.commons.beanutils.BeanUtilsBean; +import org.apache.commons.lang3.StringUtils; +import org.apache.unomi.api.Event; +import org.apache.unomi.api.PropertyType; +import org.apache.unomi.api.actions.Action; +import org.apache.unomi.api.actions.ActionExecutor; +import org.apache.unomi.api.services.EventService; +import org.apache.unomi.api.services.ProfileService; +import org.apache.unomi.persistence.spi.PropertyHelper; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +public class CopyPropertiesAction implements ActionExecutor { + + private static final Logger logger = LoggerFactory.getLogger(CopyPropertiesAction.class); + private ProfileService profileService; + + public void setProfileService(ProfileService profileService) { + this.profileService = profileService; + } + + @SuppressWarnings({ "unchecked", "rawtypes" }) + public int execute(Action action, Event event) { + boolean changed = false; + List<String> mandatoryPropTypeSystemTags = (List<String>) action.getParameterValues().get("mandatoryPropTypeSystemTag"); + String singleValueStrategy = (String) action.getParameterValues().get("singleValueStrategy"); + for (Map.Entry<String, Object> entry : getEventPropsToCopy(action, event).entrySet()) { + // propType Check + PropertyType propertyType = profileService.getPropertyType(entry.getKey()); + Object previousValue = event.getProfile().getProperty(entry.getKey()); + if (mandatoryPropTypeSystemTags != null && mandatoryPropTypeSystemTags.size() > 0) { + if (propertyType == null || propertyType.getMetadata() == null || propertyType.getMetadata().getSystemTags() == null + || !propertyType.getMetadata().getSystemTags().containsAll(mandatoryPropTypeSystemTags)) { + continue; + } + } + String propertyName = "properties." + entry.getKey(); + + if (previousValue == null && propertyType == null) { + changed = changed || PropertyHelper.setProperty(event.getProfile(), propertyName, entry.getValue(), "alwaysSet"); + } else if (previousValue != null) { + if (previousValue instanceof List) { + changed = changed || PropertyHelper.setProperty(event.getProfile(), propertyName, entry.getValue(), "addValues"); + } else if (entry.getValue() instanceof List) { + logger.error("A single property named {} is already set on the profile. Impossible to replace with a list", + entry.getKey()); + } else { + changed = + changed || PropertyHelper.setProperty(event.getProfile(), propertyName, entry.getValue(), singleValueStrategy); + } + } else { + if (propertyType.isMultivalued()) { + changed = changed || PropertyHelper.setProperty(event.getProfile(), propertyName, entry.getValue(), "addValues"); + } else if (entry.getValue() instanceof List) { + logger.error("The property {} should contains a single value as declared in the property types", entry.getKey()); + } else { + changed = + changed || PropertyHelper.setProperty(event.getProfile(), propertyName, entry.getValue(), singleValueStrategy); + } + } + } Review comment: can be simplified: (also we should avoid logging info from the event to avoid creating log injection backdoor) ``` else { boolean multipleIsExpected = previousValue != null ? previousValue instanceof List : propertyType.isMultivalued(); if (multipleIsExpected) { changed = changed || PropertyHelper.setProperty(event.getProfile(), propertyName, entry.getValue(), "addValues"); } else if (entry.getValue() instanceof List) { logger.error("Impossible to copy the property of type List to the profile, either a single value already exist on the profile or the property type is declared as a single value property. Enable debug log level for more information"); if (logger.isDebugEnabled()) { logger.debug("cannot copy property {}, because it's a List", entry.getKey()) } } else { changed = changed || PropertyHelper.setProperty(event.getProfile(), propertyName, entry.getValue(), singleValueStrategy); } } ``` ########## File path: plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/CopyPropertiesAction.java ########## @@ -0,0 +1,109 @@ +/* + * 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.plugins.baseplugin.actions; + +import org.apache.commons.beanutils.BeanUtilsBean; +import org.apache.commons.lang3.StringUtils; +import org.apache.unomi.api.Event; +import org.apache.unomi.api.PropertyType; +import org.apache.unomi.api.actions.Action; +import org.apache.unomi.api.actions.ActionExecutor; +import org.apache.unomi.api.services.EventService; +import org.apache.unomi.api.services.ProfileService; +import org.apache.unomi.persistence.spi.PropertyHelper; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +public class CopyPropertiesAction implements ActionExecutor { + + private static final Logger logger = LoggerFactory.getLogger(CopyPropertiesAction.class); + private ProfileService profileService; + + public void setProfileService(ProfileService profileService) { + this.profileService = profileService; + } + + @SuppressWarnings({ "unchecked", "rawtypes" }) + public int execute(Action action, Event event) { + boolean changed = false; + + for (Map.Entry<String, Object> entry : getEventPropsToCopy(action, event).entrySet()) { + // propType Check + PropertyType propertyType = profileService.getPropertyType(entry.getKey()); + String propertyName = "properties." + entry.getKey(); + if (propertyType != null && propertyType.isMultivalued()) { + changed = changed || PropertyHelper.setProperty(event.getProfile(), propertyName, entry.getValue(), "addValue"); + } else { + Object profileProperty = event.getProfile().getProperty(entry.getKey()); + if (profileProperty == null) { + if (entry.getValue() instanceof List) { + changed = changed || PropertyHelper.setProperty(event.getProfile(), propertyName, entry.getValue(), "addValues"); + } else { + changed = changed || PropertyHelper.setProperty(event.getProfile(), propertyName, entry.getValue(), "alwaysSet"); + } + } else if (profileProperty instanceof List) { + if (entry.getValue() instanceof List) { + changed = changed || PropertyHelper.setProperty(event.getProfile(), propertyName, entry.getValue(), "addValues"); + } else { + changed = changed || PropertyHelper.setProperty(event.getProfile(), propertyName, entry.getValue(), "addValue"); + } + } else { + if (entry.getValue() instanceof List) { + logger.warn("A single property named {} is already set on the profile.", entry.getKey()); + } else { + changed = changed || PropertyHelper.setProperty(event.getProfile(), propertyName, entry.getValue(), "alwaysSet"); + } + } + } + } + return changed ? EventService.PROFILE_UPDATED : EventService.NO_CHANGE; + } + + private Map<String, Object> getEventPropsToCopy(Action action, Event event) { + Map<String, Object> propsToCopy = new HashMap<String, Object>(); + + String rootProperty = (String) action.getParameterValues().get("rootProperty"); + boolean copyEventProps = false; + + if (StringUtils.isEmpty(rootProperty)) { + copyEventProps = true; + rootProperty = "target.properties"; + } + + // copy props from the event.properties + if (copyEventProps && event.getProperties() != null) { + propsToCopy.putAll(event.getProperties()); + } + + // copy props from the specified level (default is: target.properties) + try { + Object targetProperties = BeanUtilsBean.getInstance().getPropertyUtils().getProperty(event, rootProperty); + if (targetProperties instanceof Map) { + propsToCopy.putAll((Map) targetProperties); + } + } catch (Exception e) { + // Ignore Review comment: Still required ########## File path: itests/src/test/java/org/apache/unomi/itests/CopyPropertiesActionIT.java ########## @@ -0,0 +1,312 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License + */ + +package org.apache.unomi.itests; + +import org.apache.unomi.api.Event; +import org.apache.unomi.api.Metadata; +import org.apache.unomi.api.Profile; +import org.apache.unomi.api.PropertyType; +import org.apache.unomi.api.rules.Rule; +import org.apache.unomi.api.services.EventService; +import org.apache.unomi.api.services.ProfileService; +import org.apache.unomi.api.services.RulesService; +import org.apache.unomi.persistence.spi.CustomObjectMapper; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.ops4j.pax.exam.junit.PaxExam; +import org.ops4j.pax.exam.spi.reactors.ExamReactorStrategy; +import org.ops4j.pax.exam.spi.reactors.PerSuite; +import org.ops4j.pax.exam.util.Filter; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import javax.inject.Inject; +import java.io.File; +import java.io.IOException; +import java.util.Arrays; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; +import java.util.Date; + +/** + * Created by amidani on 12/10/2017. + */ + +@RunWith(PaxExam.class) +@ExamReactorStrategy(PerSuite.class) +public class CopyPropertiesActionIT extends BaseIT { + private final static Logger LOGGER = LoggerFactory.getLogger(CopyPropertiesActionIT.class); + + private final static String PROFILE_TARGET_TEST_ID = "profile-target-event"; + private final static String PROFILE_TEST_ID = "profile-to-update-by-event"; + + @Inject + @Filter(timeout = 600000) + protected RulesService rulesService; + @Inject + @Filter(timeout = 600000) + protected ProfileService profileService; + @Inject + @Filter(timeout = 600000) + protected EventService eventService; + + @Before + public void setUp() throws IOException, InterruptedException { + Profile profile = new Profile(); + profile.setItemId(PROFILE_TEST_ID); + 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 + profileService.save(profile); + LOGGER.info("Profile saved with ID [{}].", profile.getItemId()); + + Profile profileTarget = new Profile(); + profileTarget.setItemId(PROFILE_TARGET_TEST_ID); + profileService.save(profileTarget); + LOGGER.info("Profile saved with ID [{}].", profileTarget.getItemId()); + + refreshPersistence(); + } + + @Test + public void testCopyProperties_copyMultipleValueWithExistingPropertyType() throws IOException, InterruptedException { + Rule rule = CustomObjectMapper.getObjectMapper() + .readValue(new File("data/tmp/testCopyProperties.json").toURI().toURL(), Rule.class); + rulesService.setRule(rule); + Thread.sleep(2000); + + Profile profile = profileService.load(PROFILE_TARGET_TEST_ID); + Assert.assertNull(profile.getProperty("firstName")); + + Event event = new Event("copyProperties", null, profile, null, null, profile, new Date()); + event.setPersistent(false); + + Map<String, Object> properties = new HashMap<>(); + properties.put("param1", Arrays.asList("value")); + properties.put("param2", Arrays.asList("valueA", "valueB")); + + event.setProperty("urlParameters", properties); + + Metadata metadata = new Metadata(); + metadata.setSystemTags(new HashSet<>(Arrays.asList("urlParameters"))); + metadata.setId("param1"); + metadata.setName("Url parameters"); + + PropertyType propertyType1 = new PropertyType(); + propertyType1.setItemId("param1"); + propertyType1.setMetadata(metadata); + propertyType1.setTarget("profiles"); + propertyType1.setValueTypeId("string"); + propertyType1.setMultivalued(true); + + profileService.setPropertyType(propertyType1); + + Metadata metadata2 = new Metadata(); + metadata2.setSystemTags(new HashSet<>(Arrays.asList("urlParameters"))); + metadata2.setId("param2"); + metadata2.setName("Url parameters"); + + PropertyType propertyType2 = new PropertyType(); + propertyType2.setItemId("param2"); + propertyType2.setMetadata(metadata2); + propertyType2.setTarget("profiles"); + propertyType2.setValueTypeId("string"); + propertyType2.setMultivalued(true); + + profileService.setPropertyType(propertyType2); + + int changes = eventService.send(event); + + LOGGER.info("Changes of the event : {}", changes); + + Assert.assertTrue(changes > 0); + } + + @Test + public void testCopyProperties_copyMultipleValueWithoutExistingPropertyType() throws IOException, InterruptedException { + Rule rule = CustomObjectMapper.getObjectMapper() + .readValue(new File("data/tmp/testCopyProperties.json").toURI().toURL(), Rule.class); + rulesService.setRule(rule); + Thread.sleep(2000); + + Profile profile = profileService.load(PROFILE_TARGET_TEST_ID); + Assert.assertNull(profile.getProperty("firstName")); + + Event event = new Event("copyProperties", null, profile, null, null, profile, new Date()); + event.setPersistent(false); + + Map<String, Object> properties = new HashMap<>(); + properties.put("param1WithoutPropertyType", Arrays.asList("value")); + properties.put("param2WithoutPropertyType", Arrays.asList("valueA", "valueB")); + + event.setProperty("urlParameters", properties); + + int changes = eventService.send(event); + + LOGGER.info("Changes of the event : {}", changes); + + Assert.assertTrue(changes > 0); + } + + @Test + public void testCopyProperties_copySingleValueWithoutPropertyType() throws IOException, InterruptedException { + Rule rule = CustomObjectMapper.getObjectMapper() + .readValue(new File("data/tmp/testCopyProperties.json").toURI().toURL(), Rule.class); + rulesService.setRule(rule); + Thread.sleep(2000); + + Profile profile = profileService.load(PROFILE_TARGET_TEST_ID); + Assert.assertNull(profile.getProperty("firstName")); + + Event event = new Event("copyProperties", null, profile, null, null, profile, new Date()); + event.setPersistent(false); + + Map<String, Object> properties = new HashMap<>(); + properties.put("param1SingleValue", "SingleValue"); + + event.setProperty("urlParameters", properties); + + int changes = eventService.send(event); + + LOGGER.info("Changes of the event : {}", changes); + + Assert.assertTrue(changes > 0); + } + + @Test + public void testCopyProperties_copySingleValueWithExistingPropertyType() throws IOException, InterruptedException { + Rule rule = CustomObjectMapper.getObjectMapper() + .readValue(new File("data/tmp/testCopyProperties.json").toURI().toURL(), Rule.class); + rulesService.setRule(rule); + Thread.sleep(2000); + + Profile profile = profileService.load(PROFILE_TARGET_TEST_ID); + Assert.assertNull(profile.getProperty("firstName")); + + Event event = new Event("copyProperties", null, profile, null, null, profile, new Date()); + event.setPersistent(false); + + Map<String, Object> properties = new HashMap<>(); + properties.put("param1SingleWithPropertyType", "SingleValue"); + + event.setProperty("urlParameters", properties); + + Metadata metadata = new Metadata(); + metadata.setSystemTags(new HashSet<>(Arrays.asList("urlParameters"))); + metadata.setId("param1SingleWithPropertyType"); + metadata.setName("Url parameters"); + + PropertyType propertyType1 = new PropertyType(); + propertyType1.setItemId("param1SingleWithPropertyType"); + propertyType1.setMetadata(metadata); + propertyType1.setTarget("profiles"); + propertyType1.setValueTypeId("string"); + propertyType1.setMultivalued(false); + + profileService.setPropertyType(propertyType1); + + int changes = eventService.send(event); + + LOGGER.info("Changes of the event : {}", changes); + + Assert.assertTrue(changes > 0); + } + + + @Test + public void testCopyProperties_copySingleValueWithExistingPropertyOnProfile() throws IOException, InterruptedException { + Rule rule = CustomObjectMapper.getObjectMapper() + .readValue(new File("data/tmp/testCopyProperties.json").toURI().toURL(), Rule.class); + rulesService.setRule(rule); + Thread.sleep(2000); + + Profile profile = profileService.load(PROFILE_TARGET_TEST_ID); + profile.setProperty("param1Existing", "existing"); + profile.setProperty("param2Existing", Arrays.asList("existingArray")); + Assert.assertNull(profile.getProperty("firstName")); + + Event event = new Event("copyProperties", null, profile, null, null, profile, new Date()); + event.setPersistent(false); + + Map<String, Object> properties = new HashMap<>(); + properties.put("param1Existing", "SingleValue"); + + event.setProperty("urlParameters", properties); + + int changes = eventService.send(event); + + LOGGER.info("Changes of the event : {}", changes); + + Assert.assertTrue(changes > 0); + } + + @Test + public void testCopyProperties_copySingleValueWithExistingArray() throws IOException, InterruptedException { + Rule rule = CustomObjectMapper.getObjectMapper() + .readValue(new File("data/tmp/testCopyProperties.json").toURI().toURL(), Rule.class); + rulesService.setRule(rule); + Thread.sleep(2000); + + Profile profile = profileService.load(PROFILE_TARGET_TEST_ID); + profile.setProperty("paramExistingArray", Arrays.asList("existingArray")); + Assert.assertNull(profile.getProperty("firstName")); + + Event event = new Event("copyProperties", null, profile, null, null, profile, new Date()); + event.setPersistent(false); + + Map<String, Object> properties = new HashMap<>(); + properties.put("paramExistingArray", "SingleValue"); + + event.setProperty("urlParameters", properties); + + int changes = eventService.send(event); + + LOGGER.info("Changes of the event : {}", changes); + + Assert.assertTrue(changes > 0); + } + + @Test + public void testCopyProperties_copyArrayOnSingleValueShouldNotCopy() throws IOException, InterruptedException { + Rule rule = CustomObjectMapper.getObjectMapper() + .readValue(new File("data/tmp/testCopyProperties.json").toURI().toURL(), Rule.class); + rulesService.setRule(rule); + Thread.sleep(2000); + + Profile profile = profileService.load(PROFILE_TARGET_TEST_ID); + profile.setProperty("paramToNotReplace", "existingSingleValue"); + Assert.assertNull(profile.getProperty("firstName")); + + Event event = new Event("copyProperties", null, profile, null, null, profile, new Date()); + event.setPersistent(false); + + Map<String, Object> properties = new HashMap<>(); + properties.put("paramToNotReplace", Arrays.asList("newArray")); + + event.setProperty("urlParameters", properties); + + int changes = eventService.send(event); + + LOGGER.info("Changes of the event : {}", changes); + + Assert.assertTrue(changes == 0); + } +} Review comment: Still not done -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected]
