jkevan commented on code in PR #659: URL: https://github.com/apache/unomi/pull/659#discussion_r1599636155
########## api/src/main/java/org/apache/unomi/api/utils/ConditionHelper.java: ########## @@ -0,0 +1,113 @@ +/* + * 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.api.utils; + +import org.apache.unomi.api.conditions.Condition; +import org.apache.unomi.api.services.DefinitionsService; + +import java.util.List; +import java.util.Set; + +/** + * Utility class for creating various types of {@link Condition} objects. + * This class provides methods to easily construct conditions used for querying data based on specific criteria. + */ +public class ConditionHelper { Review Comment: I see we have in itests: ConditionBuilder https://github.com/apache/unomi/blob/master/itests/src/test/java/org/apache/unomi/itests/ConditionBuilder.java it say: ``` /** * Utility class for building conditions */ ``` It seem's similar to yours (both try to provide easy JAVA API to build various type of condition). To me it's not good to have 2 places that try to do the same things. We should consider capitalize on only one place. ########## plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/SetEventOccurenceCountAction.java: ########## @@ -123,14 +121,38 @@ 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) { + List<Map<String, Object>> existingPastEvents = (List<Map<String, Object>>) event.getProfile().getSystemProperties().get("pastEvents"); + if (existingPastEvents == null) { + existingPastEvents = new ArrayList<>(); + event.getProfile().getSystemProperties().put("pastEvents", existingPastEvents); + } + + for (Map<String, Object> pastEvent : existingPastEvents) { + if (generatedPropertyKey.equals(pastEvent.get("key"))) { + pastEvent.put("count", count); + return true; + } + } + + return addNewPastEvent(existingPastEvents, generatedPropertyKey, count); + } + + private boolean addNewPastEvent(List<Map<String, Object>> existingPastEvents, String key, long count) { + Map<String, Object> newPastEvent = new HashMap<>(); + newPastEvent.put("key", key); + newPastEvent.put("count", count); + existingPastEvents.add(newPastEvent); + return true; // New event added + } Review Comment: Unecessary granularity of function, move this function code to the caller directly for better readability. (the name is a bit not clear: `addNewPastEvent`, this function is not adding a new past event, it's adding the event occurence count on a profile for past event key. ) ########## services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java: ########## @@ -158,6 +158,7 @@ public void postConstruct() throws IOException { } bundleContext.addBundleListener(this); initializeTimer(); + conditionHelper = new ConditionHelper(definitionsService); Review Comment: It's a bit weird to have an instance of ConditionHelper in every services that need it. either: - it should be a dedicated service. - it should be provided by the DefinitionService it self `getConditionBuilder()` - it should be static. ########## plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/actions/SetEventOccurenceCountAction.java: ########## @@ -123,14 +121,38 @@ 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) { Review Comment: It seem's this function always return true, so I would suggest to remove the boolean type, and consider it's always true from the caller to avoid testing the result of the call. ########## plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/conditions/PastEventConditionESQueryBuilder.java: ########## @@ -122,44 +128,42 @@ protected static boolean getStrategyFromOperator(String operator) { return operator == null || operator.equals("eventsOccurred"); } - private Condition getProfileIdsCondition(Set<String> ids, boolean shouldMatch) { - Condition idsCondition = new Condition(); - idsCondition.setConditionType(definitionsService.getConditionType("idsCondition")); - idsCondition.setParameter("ids", ids); - idsCondition.setParameter("match", shouldMatch); - return idsCondition; - } - private Condition getProfileConditionForCounter(String generatedPropertyKey, Integer minimumEventCount, Integer maximumEventCount, boolean eventsOccurred) { - String generatedPropertyName = "systemProperties.pastEvents." + 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; + return createEventOccurredCondition(generatedPropertyKey, minimumEventCount, maximumEventCount); } 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); - counterZero.setParameter("comparisonOperator", "equals"); - counterZero.setParameter("propertyValueInteger", 0); - - Condition counterCondition = new Condition(); - counterCondition.setConditionType(definitionsService.getConditionType("booleanCondition")); - counterCondition.setParameter("operator", "or"); - counterCondition.setParameter("subConditions", Arrays.asList(counterMissing, counterZero)); - return counterCondition; + return createEventNotOccurredCondition(generatedPropertyKey); } } + private Condition createEventOccurredCondition(String generatedPropertyKey, Integer minimumEventCount, Integer maximumEventCount) { + Condition subConditionCount = conditionHelper.createProfilePropertyCondition("systemProperties.pastEvents.count", "between", Arrays.asList(minimumEventCount, maximumEventCount), "propertyValuesInteger"); + Condition subConditionKey = conditionHelper.createProfilePropertyCondition("systemProperties.pastEvents.key", "equals", generatedPropertyKey, "propertyValue"); + + Condition booleanCondition = conditionHelper.createBooleanCondition("and", Arrays.asList(subConditionCount, subConditionKey)); + return conditionHelper.createNestedCondition("systemProperties.pastEvents", booleanCondition); + } + + private Condition createEventNotOccurredCondition(String generatedPropertyKey) { + Condition pastEventsNotExist = createPastEventMustNotExistCondition(generatedPropertyKey); Review Comment: Would rename to `counterMissing` ########## services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java: ########## @@ -986,31 +986,35 @@ public void recalculatePastEventConditions() { * * @param eventCountByProfile the events count per profileId map * @param propertyKey the generate property key for this past event condition, to keep track of the count in the profile - * @return the list of profiles for witch the count of event occurrences have been updated. + * @return the set of profiles for witch the count of event occurrences have been updated. */ private Set<String> updatePastEventOccurrencesOnProfiles(Map<String, Long> eventCountByProfile, String propertyKey) { Set<String> profilesUpdated = new HashSet<>(); - Map<Item, Map> batch = new HashMap<>(); + Map<String, Map[]> batch = new HashMap<>(); Iterator<Map.Entry<String, Long>> entryIterator = eventCountByProfile.entrySet().iterator(); + while (entryIterator.hasNext()) { 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)); + Map<String, Object> scriptParams = new HashMap<>(); + scriptParams.put("pastEventKey", propertyKey); + scriptParams.put("valueToAdd", entry.getValue()); + Map<String, Object>[] params = new Map[]{scriptParams}; + batch.put(profileId, params); profilesUpdated.add(profileId); } - if (batch.size() == segmentUpdateBatchSize || (!entryIterator.hasNext() && batch.size() > 0)) { + + + + if (batch.size() == segmentUpdateBatchSize || (!entryIterator.hasNext() && !batch.isEmpty())) { try { - persistenceService.update(batch, Profile.class); + batch.forEach((id, params) -> { + Condition profileIdCondition = conditionHelper.createProfilePropertyCondition("itemId", "equals", id, "propertyValue"); + Condition[] conditions = new Condition[]{profileIdCondition}; + persistenceService.updateWithQueryAndStoredScript(Profile.class, new String[]{"updatePastEventOccurences"}, params, conditions); + }); Review Comment: Doesn't seem's correct, what is the goal of having a batch list if it's for updating each profile one by one ? Consider one update query for the entire batch. use a OR boolean query that will contains all the profileID. And pass the entire eventCountByProfile directly as param, so that each update will pick the profile it's currently updating. ########## plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/conditions/PastEventConditionESQueryBuilder.java: ########## @@ -122,44 +128,42 @@ protected static boolean getStrategyFromOperator(String operator) { return operator == null || operator.equals("eventsOccurred"); } - private Condition getProfileIdsCondition(Set<String> ids, boolean shouldMatch) { - Condition idsCondition = new Condition(); - idsCondition.setConditionType(definitionsService.getConditionType("idsCondition")); - idsCondition.setParameter("ids", ids); - idsCondition.setParameter("match", shouldMatch); - return idsCondition; - } - private Condition getProfileConditionForCounter(String generatedPropertyKey, Integer minimumEventCount, Integer maximumEventCount, boolean eventsOccurred) { - String generatedPropertyName = "systemProperties.pastEvents." + 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; + return createEventOccurredCondition(generatedPropertyKey, minimumEventCount, maximumEventCount); } 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); - counterZero.setParameter("comparisonOperator", "equals"); - counterZero.setParameter("propertyValueInteger", 0); - - Condition counterCondition = new Condition(); - counterCondition.setConditionType(definitionsService.getConditionType("booleanCondition")); - counterCondition.setParameter("operator", "or"); - counterCondition.setParameter("subConditions", Arrays.asList(counterMissing, counterZero)); - return counterCondition; + return createEventNotOccurredCondition(generatedPropertyKey); } } + private Condition createEventOccurredCondition(String generatedPropertyKey, Integer minimumEventCount, Integer maximumEventCount) { + Condition subConditionCount = conditionHelper.createProfilePropertyCondition("systemProperties.pastEvents.count", "between", Arrays.asList(minimumEventCount, maximumEventCount), "propertyValuesInteger"); + Condition subConditionKey = conditionHelper.createProfilePropertyCondition("systemProperties.pastEvents.key", "equals", generatedPropertyKey, "propertyValue"); + + Condition booleanCondition = conditionHelper.createBooleanCondition("and", Arrays.asList(subConditionCount, subConditionKey)); + return conditionHelper.createNestedCondition("systemProperties.pastEvents", booleanCondition); + } + + private Condition createEventNotOccurredCondition(String generatedPropertyKey) { + Condition pastEventsNotExist = createPastEventMustNotExistCondition(generatedPropertyKey); + Condition counterZero = conditionHelper.createProfilePropertyCondition("systemProperties.pastEvents.count", "equals", 0, "propertyValueInteger"); + Condition keyEquals = conditionHelper.createProfilePropertyCondition("systemProperties.pastEvents.key", "equals", generatedPropertyKey, "propertyValue"); + Condition keyExistsAndCounterZero = conditionHelper.createBooleanCondition("and", Arrays.asList(keyEquals, counterZero)); + Condition nestedKeyExistsAndCounterZero = conditionHelper.createNestedCondition("systemProperties.pastEvents", keyExistsAndCounterZero); + + return conditionHelper.createBooleanCondition("or", Arrays.asList(pastEventsNotExist, nestedKeyExistsAndCounterZero)); Review Comment: It seem's both subConditions are nested. instead of having a boolean OR: nested OR nested I think it would be better to have a nested condition wrapping the OR condition. this way only one nested clause would be used. It's seem's possible -- 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: dev-unsubscr...@unomi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
