jkevan commented on code in PR #659: URL: https://github.com/apache/unomi/pull/659#discussion_r1579151121
########## tools/shell-commands/src/main/resources/requestBody/2.5.0/update_pastEvents_profile.painless: ########## @@ -0,0 +1,28 @@ +/* + * 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. + */ + +if (ctx._source.systemProperties != null && ctx._source.systemProperties.pastEvents != null && ctx._source.systemProperties.pastEvents instanceof Map) { + Map updatedPastEvents = new HashMap(); Review Comment: `updatedPastEvents` seems useless ? ########## plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/conditions/PastEventConditionESQueryBuilder.java: ########## @@ -131,35 +131,77 @@ private Condition getProfileIdsCondition(Set<String> ids, boolean shouldMatch) { } private Condition getProfileConditionForCounter(String generatedPropertyKey, Integer minimumEventCount, Integer maximumEventCount, boolean eventsOccurred) { - String generatedPropertyName = "systemProperties.pastEvents." + generatedPropertyKey; + Condition countCondition = new Condition(); + + countCondition.setConditionType(definitionsService.getConditionType("nestedCondition")); + countCondition.setParameter("path", "systemProperties.pastEvents"); + + Condition subConditionCount = new Condition(definitionsService.getConditionType("profilePropertyCondition")); + + Condition subConditionKey = getKeyEqualsCondition(generatedPropertyKey); + ConditionType profilePropertyConditionType = definitionsService.getConditionType("profilePropertyCondition"); if (eventsOccurred) { - Condition counterIsBetweenBoundaries = new Condition(); - counterIsBetweenBoundaries.setConditionType(profilePropertyConditionType); - counterIsBetweenBoundaries.setParameter("propertyName", generatedPropertyName); - counterIsBetweenBoundaries.setParameter("comparisonOperator", "between"); - counterIsBetweenBoundaries.setParameter("propertyValuesInteger", Arrays.asList(minimumEventCount, maximumEventCount)); - return counterIsBetweenBoundaries; + subConditionCount.setParameter("propertyName", "systemProperties.pastEvents.count"); + subConditionCount.setParameter("comparisonOperator", "between"); + subConditionCount.setParameter("propertyValuesInteger", Arrays.asList(minimumEventCount, maximumEventCount)); + + Condition booleanCondition = new Condition(definitionsService.getConditionType("booleanCondition")); + booleanCondition.setParameter("operator", "and"); + booleanCondition.setParameter("subConditions", Arrays.asList(subConditionCount, subConditionKey)); + + countCondition.setParameter("subCondition", booleanCondition); + return countCondition; + } else { - Condition counterMissing = new Condition(); - counterMissing.setConditionType(profilePropertyConditionType); - counterMissing.setParameter("propertyName", generatedPropertyName); - counterMissing.setParameter("comparisonOperator", "missing"); - - Condition counterZero = new Condition(); - counterZero.setConditionType(profilePropertyConditionType); - counterZero.setParameter("propertyName", generatedPropertyName); + + // 1. Key not present in profile + Condition keyNestedCondition = new Condition(); + keyNestedCondition.setConditionType(definitionsService.getConditionType("nestedCondition")); + keyNestedCondition.setParameter("path", "systemProperties.pastEvents"); + + Condition keyEquals = new Condition(profilePropertyConditionType); + keyEquals.setParameter("propertyName", "systemProperties.pastEvents.key"); + keyEquals.setParameter("comparisonOperator", "equals"); + keyEquals.setParameter("propertyValue", generatedPropertyKey); Review Comment: What is the difference with the keyNestedCondition ? ########## plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/conditions/PastEventConditionESQueryBuilder.java: ########## @@ -131,35 +131,77 @@ private Condition getProfileIdsCondition(Set<String> ids, boolean shouldMatch) { } private Condition getProfileConditionForCounter(String generatedPropertyKey, Integer minimumEventCount, Integer maximumEventCount, boolean eventsOccurred) { - String generatedPropertyName = "systemProperties.pastEvents." + generatedPropertyKey; + Condition countCondition = new Condition(); + + countCondition.setConditionType(definitionsService.getConditionType("nestedCondition")); + countCondition.setParameter("path", "systemProperties.pastEvents"); + + Condition subConditionCount = new Condition(definitionsService.getConditionType("profilePropertyCondition")); Review Comment: Why not using `profilePropertyConditionType` declared few lines after ? Also it's seems this condition is only used in first if clause, could you move it ? Actually all the code block seem;s only related to the first if clause ? ########## plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/SetEventOccurenceCountAction.java: ########## @@ -123,14 +121,27 @@ public int execute(Action action, Event event) { count++; } - String generatedPropertyKey = (String) pastEventCondition.getParameter("generatedPropertyKey"); - if (PropertyHelper.setProperty(event.getProfile(), "systemProperties.pastEvents." + generatedPropertyKey, count, "alwaysSet")) { + if (updatePastEvents(event, (String) pastEventCondition.getParameter("generatedPropertyKey"), count)) { return EventService.PROFILE_UPDATED; } return EventService.NO_CHANGE; } + private boolean updatePastEvents(Event event, String generatedPropertyKey, long count) { + ArrayList<Map<String, Object>> pastEvents = new ArrayList<>(); + ArrayList<Map<String, Object>> existingPastEvents = (ArrayList<Map<String, Object>>) event.getProfile().getSystemProperties().get("pastEvents"); Review Comment: A solution with a single list seems possible to me. - just iterate on existingPastEvents with a classic for/each. - stop at corresponding key (if found) -> update the count - if not found add entry to the list - save the existingPastEvents ########## plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/conditions/PastEventConditionESQueryBuilder.java: ########## @@ -131,35 +131,77 @@ private Condition getProfileIdsCondition(Set<String> ids, boolean shouldMatch) { } private Condition getProfileConditionForCounter(String generatedPropertyKey, Integer minimumEventCount, Integer maximumEventCount, boolean eventsOccurred) { - String generatedPropertyName = "systemProperties.pastEvents." + generatedPropertyKey; + Condition countCondition = new Condition(); + + countCondition.setConditionType(definitionsService.getConditionType("nestedCondition")); + countCondition.setParameter("path", "systemProperties.pastEvents"); + + Condition subConditionCount = new Condition(definitionsService.getConditionType("profilePropertyCondition")); + + Condition subConditionKey = getKeyEqualsCondition(generatedPropertyKey); + ConditionType profilePropertyConditionType = definitionsService.getConditionType("profilePropertyCondition"); if (eventsOccurred) { - Condition counterIsBetweenBoundaries = new Condition(); - counterIsBetweenBoundaries.setConditionType(profilePropertyConditionType); - counterIsBetweenBoundaries.setParameter("propertyName", generatedPropertyName); - counterIsBetweenBoundaries.setParameter("comparisonOperator", "between"); - counterIsBetweenBoundaries.setParameter("propertyValuesInteger", Arrays.asList(minimumEventCount, maximumEventCount)); - return counterIsBetweenBoundaries; + subConditionCount.setParameter("propertyName", "systemProperties.pastEvents.count"); + subConditionCount.setParameter("comparisonOperator", "between"); + subConditionCount.setParameter("propertyValuesInteger", Arrays.asList(minimumEventCount, maximumEventCount)); + + Condition booleanCondition = new Condition(definitionsService.getConditionType("booleanCondition")); + booleanCondition.setParameter("operator", "and"); + booleanCondition.setParameter("subConditions", Arrays.asList(subConditionCount, subConditionKey)); + + countCondition.setParameter("subCondition", booleanCondition); + return countCondition; + } else { - Condition counterMissing = new Condition(); - counterMissing.setConditionType(profilePropertyConditionType); - counterMissing.setParameter("propertyName", generatedPropertyName); - counterMissing.setParameter("comparisonOperator", "missing"); - - Condition counterZero = new Condition(); - counterZero.setConditionType(profilePropertyConditionType); - counterZero.setParameter("propertyName", generatedPropertyName); + + // 1. Key not present in profile + Condition keyNestedCondition = new Condition(); + keyNestedCondition.setConditionType(definitionsService.getConditionType("nestedCondition")); + keyNestedCondition.setParameter("path", "systemProperties.pastEvents"); + + Condition keyEquals = new Condition(profilePropertyConditionType); + keyEquals.setParameter("propertyName", "systemProperties.pastEvents.key"); + keyEquals.setParameter("comparisonOperator", "equals"); + keyEquals.setParameter("propertyValue", generatedPropertyKey); + + keyNestedCondition.setParameter("subCondition", keyEquals); + + Condition mustNotExist = new Condition(definitionsService.getConditionType("notCondition")); + mustNotExist.setParameter("subCondition", keyNestedCondition); + + // 2. Key present in profile but value equals to 0 + Condition counterZero = new Condition(profilePropertyConditionType); + counterZero.setParameter("propertyName", "systemProperties.pastEvents.count"); counterZero.setParameter("comparisonOperator", "equals"); counterZero.setParameter("propertyValueInteger", 0); + Condition keyExistsAndCounterZero = new Condition(definitionsService.getConditionType("booleanCondition")); + keyExistsAndCounterZero.setParameter("operator", "and"); + keyExistsAndCounterZero.setParameter("subConditions", Arrays.asList(subConditionKey, counterZero)); + + Condition nestedKeyExistsAndCounterZero = new Condition(); + nestedKeyExistsAndCounterZero.setConditionType(definitionsService.getConditionType("nestedCondition")); + nestedKeyExistsAndCounterZero.setParameter("path", "systemProperties.pastEvents"); + nestedKeyExistsAndCounterZero.setParameter("subCondition", keyExistsAndCounterZero); + Condition counterCondition = new Condition(); counterCondition.setConditionType(definitionsService.getConditionType("booleanCondition")); counterCondition.setParameter("operator", "or"); - counterCondition.setParameter("subConditions", Arrays.asList(counterMissing, counterZero)); + counterCondition.setParameter("subConditions", Arrays.asList(mustNotExist, nestedKeyExistsAndCounterZero)); + return counterCondition; } } + private Condition getKeyEqualsCondition(String generatedPropertyKey) { + Condition subConditionKey = new Condition(definitionsService.getConditionType("profilePropertyCondition")); + subConditionKey.setParameter("propertyName", "systemProperties.pastEvents.key"); + subConditionKey.setParameter("comparisonOperator", "equals"); + subConditionKey.setParameter("propertyValue", generatedPropertyKey); + return subConditionKey; + } + Review Comment: Overall this code is really complex to read and looks like there is a lot of stuff to refactor to make it more clear. Please consider cleaning the condition building here to make it easy to read and understand. Also fix the redundant conditions, and code structure regarding the various cases it should support. ########## plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/SetEventOccurenceCountAction.java: ########## @@ -123,14 +121,27 @@ public int execute(Action action, Event event) { count++; } - String generatedPropertyKey = (String) pastEventCondition.getParameter("generatedPropertyKey"); - if (PropertyHelper.setProperty(event.getProfile(), "systemProperties.pastEvents." + generatedPropertyKey, count, "alwaysSet")) { + if (updatePastEvents(event, (String) pastEventCondition.getParameter("generatedPropertyKey"), count)) { return EventService.PROFILE_UPDATED; } return EventService.NO_CHANGE; } + private boolean updatePastEvents(Event event, String generatedPropertyKey, long count) { + ArrayList<Map<String, Object>> pastEvents = new ArrayList<>(); + ArrayList<Map<String, Object>> existingPastEvents = (ArrayList<Map<String, Object>>) event.getProfile().getSystemProperties().get("pastEvents"); + if (existingPastEvents != null) { + pastEvents.addAll(existingPastEvents.stream().filter(pastEvent -> !pastEvent.get("key").equals(generatedPropertyKey)).collect(Collectors.toList())); + } + + Map<String, Object> pastEvent = new HashMap<>(); + pastEvent.put("key", generatedPropertyKey); + pastEvent.put("count", count); + pastEvents.add(pastEvent); + return PropertyHelper.setProperty(event.getProfile(), "systemProperties.pastEvents", pastEvents, "alwaysSet"); Review Comment: Don't know how the PropertyHelper is behaving on List<Map> objects. I know it's doing internal compare of values, but I doubt that in our case it's really something we want. I think it would be safer to not use the PropertyHelper and directly manipulate the Profile object here to set modify acccordingly the `pastEvents` system props. ########## services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java: ########## @@ -996,19 +1011,26 @@ private Set<String> updatePastEventOccurrencesOnProfiles(Map<String, Long> event Map.Entry<String, Long> entry = entryIterator.next(); String profileId = entry.getKey(); if (!profileId.startsWith("_")) { - Map<String, Long> pastEventCounts = new HashMap<>(); - pastEventCounts.put(propertyKey, entry.getValue()); - Map<String, Object> systemProperties = new HashMap<>(); - systemProperties.put("pastEvents", pastEventCounts); - systemProperties.put("lastUpdated", new Date()); - - Profile profile = new Profile(); - profile.setItemId(profileId); - batch.put(profile, Collections.singletonMap("systemProperties", systemProperties)); - profilesUpdated.add(profileId); + Profile storedProfile = persistenceService.load(profileId, Profile.class); Review Comment: Dangerous in term of perf to load each profiles here. But I understand why it's necessary. Should we consider to perform an update by painless script here to avoid loading all profiles here ? I am a bit worried about perf here, this process was quiet heavy already and loading each profiles will have perf impacts. I know we update the scorings with painless scripts and some other places for other reasons, may be it could be wise to investigate similare update here. ########## plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/conditions/PastEventConditionEvaluator.java: ########## @@ -55,10 +57,15 @@ public boolean eval(Condition condition, Item item, Map<String, Object> context, if (parameters.containsKey("generatedPropertyKey")) { String key = (String) parameters.get("generatedPropertyKey"); Profile profile = (Profile) item; - Map<String,Object> pastEvents = (Map<String, Object>) profile.getSystemProperties().get("pastEvents"); - if (pastEvents != null) { - Number l = (Number) pastEvents.get(key); - count = l != null ? l.longValue() : 0L; + Object rawPastEvents = profile.getSystemProperties().get("pastEvents"); + if (rawPastEvents != null) { + List<Map<String, Object>> pastEvents = (ArrayList<Map<String, Object>>) rawPastEvents; Review Comment: I think cast support null. so it could be done in one call here. ``` List<Map<String, Object>> pastEvents = profile.getSystemProperties().get("pastEvents"); if (pastEvents != null) { ``` ########## services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java: ########## @@ -881,10 +881,25 @@ private void recalculatePastEventOccurrencesOnProfiles(Condition eventCondition, */ private Set<String> getExistingProfilesWithPastEventOccurrenceCount(String generatedPropertyKey) { Condition countExistsCondition = new Condition(); - countExistsCondition.setConditionType(definitionsService.getConditionType("profilePropertyCondition")); - countExistsCondition.setParameter("propertyName", "systemProperties.pastEvents." + generatedPropertyKey); - countExistsCondition.setParameter("comparisonOperator", "greaterThan"); - countExistsCondition.setParameter("propertyValueInteger", 0); + + countExistsCondition.setConditionType(definitionsService.getConditionType("nestedCondition")); + countExistsCondition.setParameter("path", "systemProperties.pastEvents"); + + Condition subConditionCount = new Condition(definitionsService.getConditionType("profilePropertyCondition")); + subConditionCount.setParameter("propertyName", "systemProperties.pastEvents.count"); + subConditionCount.setParameter("comparisonOperator", "greaterThan"); + subConditionCount.setParameter("propertyValueInteger", 0); + + Condition subConditionKey = new Condition(definitionsService.getConditionType("profilePropertyCondition")); + subConditionKey.setParameter("propertyName", "systemProperties.pastEvents.key"); + subConditionKey.setParameter("comparisonOperator", "equals"); + subConditionKey.setParameter("propertyValue", generatedPropertyKey); + + Condition booleanCondition = new Condition(definitionsService.getConditionType("booleanCondition")); + booleanCondition.setParameter("operator", "and"); + booleanCondition.setParameter("subConditions", Arrays.asList(subConditionCount, subConditionKey)); + + countExistsCondition.setParameter("subCondition", booleanCondition); Review Comment: Could be interesting to externalize this condition building into a reusable function. Seems plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/conditions/PastEventConditionESQueryBuilder.java is already building the same condition. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
