Copilot commented on code in PR #755: URL: https://github.com/apache/unomi/pull/755#discussion_r3239459886
########## tools/shell-dev-commands/src/main/java/org/apache/unomi/shell/dev/commands/UndeployDefinition.java: ########## @@ -0,0 +1,97 @@ +/* + * 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.shell.dev.commands; + +import org.apache.karaf.shell.api.action.Command; +import org.apache.karaf.shell.api.action.lifecycle.Service; +import org.apache.unomi.api.Patch; +import org.apache.unomi.api.PersonaWithSessions; +import org.apache.unomi.api.PropertyType; +import org.apache.unomi.api.actions.ActionType; +import org.apache.unomi.api.campaigns.Campaign; +import org.apache.unomi.api.conditions.ConditionType; +import org.apache.unomi.api.goals.Goal; +import org.apache.unomi.api.rules.Rule; +import org.apache.unomi.api.segments.Scoring; +import org.apache.unomi.api.segments.Segment; + +import java.io.IOException; +import java.io.PrintStream; +import java.net.URL; + +@Command(scope = "unomi", name = "undeploy-definition", description = "This will undeploy definitions contained in bundles") +@Service +public class UndeployDefinition extends DeploymentCommandSupport { + + public void processDefinition(String definitionType, URL definitionURL) { + try { + processDefinitionInternal(definitionType, definitionURL, getConsole(), "Predefined definition unregistered"); + } catch (IOException e) { + handleDefinitionError(definitionURL, "removing", e); + } + } + + @Override + protected boolean processDefinitionByType(String definitionType, URL definitionURL, PrintStream console) throws IOException { + switch (definitionType) { + case CONDITION_DEFINITION_TYPE: + ConditionType conditionType = readDefinition(definitionURL, ConditionType.class); + definitionsService.removeActionType(conditionType.getItemId()); + return true; + case ACTION_DEFINITION_TYPE: Review Comment: In the CONDITION_DEFINITION_TYPE branch you’re removing a condition type via definitionsService.removeActionType(...). This looks incorrect (it removes an action type, not the condition type) and will leave the condition type still registered. Use definitionsService.removeConditionType(conditionType.getItemId()) here. ########## tools/shell-dev-commands/src/main/java/org/apache/unomi/shell/dev/commands/rules/RuleCrudCommand.java: ########## @@ -0,0 +1,209 @@ +/* + * 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.shell.dev.commands.rules; + +import com.fasterxml.jackson.databind.ObjectMapper; +import org.apache.commons.lang3.StringUtils; +import org.apache.unomi.api.Metadata; +import org.apache.unomi.api.PartialList; +import org.apache.unomi.api.query.Query; +import org.apache.unomi.api.rules.Rule; +import org.apache.unomi.api.rules.RuleStatistics; +import org.apache.unomi.api.services.RulesService; +import org.apache.unomi.persistence.spi.CustomObjectMapper; +import org.apache.unomi.shell.dev.services.BaseCrudCommand; +import org.apache.unomi.shell.dev.services.CrudCommand; +import org.apache.unomi.api.conditions.Condition; +import org.osgi.service.component.annotations.Component; +import org.osgi.service.component.annotations.Reference; + +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.stream.Collectors; + +@Component(service = CrudCommand.class, immediate = true) +public class RuleCrudCommand extends BaseCrudCommand { + + private static final ObjectMapper OBJECT_MAPPER = new CustomObjectMapper(); + private static final List<String> PROPERTY_NAMES = List.of( + "itemId", "name", "description", "priority", "condition", "actions", "metadata" + ); + private static final List<String> CONDITION_TYPES = List.of( + "booleanCondition", "profilePropertyCondition", "sessionPropertyCondition", "eventPropertyCondition", + "pastEventCondition", "matchAllCondition", "notCondition", "orCondition", "andCondition" + ); + + @Reference + private RulesService rulesService; + + @Override + public String getObjectType() { + return "rule"; + } + + @Override + protected String[] getHeadersWithoutTenant() { + return new String[] { + "Activated", + "Hidden", + "Read-only", + "Identifier", + "Scope", + "Name", + "Tags", + "System tags", + "Executions", + "Conditions [ms]", + "Actions [ms]" + }; + } + + @Override + protected PartialList<?> getItems(Query query) { + return rulesService.getRuleDetails(query); + } + + @Override + protected Comparable[] buildRow(Object item) { + Rule rule = (Rule) item; + String ruleId = rule.getItemId(); + Map<String,RuleStatistics> allRuleStatistics = rulesService.getAllRuleStatistics(); + + ArrayList<Comparable> rowData = new ArrayList<>(); Review Comment: getAllRuleStatistics() is called inside buildRow(), which is executed once per row. If the implementation does any I/O or synchronization, this becomes an avoidable per-row cost. Fetch the statistics map once per list execution (e.g., in getItems/buildDataTable/buildRows) and reuse it when building rows. ########## tools/shell-dev-commands/src/main/java/org/apache/unomi/shell/dev/commands/personas/PersonaCrudCommand.java: ########## @@ -0,0 +1,150 @@ +/* + * 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.shell.dev.commands.personas; + +import com.fasterxml.jackson.databind.ObjectMapper; +import org.apache.unomi.api.PartialList; +import org.apache.unomi.api.Persona; +import org.apache.unomi.api.query.Query; +import org.apache.unomi.api.services.ProfileService; +import org.apache.unomi.persistence.spi.CustomObjectMapper; +import org.apache.unomi.shell.dev.services.BaseCrudCommand; +import org.apache.unomi.shell.dev.services.CrudCommand; +import org.osgi.service.component.annotations.Component; +import org.osgi.service.component.annotations.Reference; + +import java.util.Arrays; +import java.util.List; +import java.util.Map; +import java.util.stream.Collectors; + +@Component(service = CrudCommand.class, immediate = true) +public class PersonaCrudCommand extends BaseCrudCommand { + + private static final ObjectMapper OBJECT_MAPPER = new CustomObjectMapper(); + private static final List<String> PROPERTY_NAMES = Arrays.asList( + "firstName", + "lastName", + "email", + "description", + "properties" + ); + + @Reference + private ProfileService profileService; + + @Override + public String getObjectType() { + return Persona.ITEM_TYPE; + } + + @Override + protected String[] getHeadersWithoutTenant() { + return new String[] { + "Identifier", + "First Name", + "Last Name", + "Email", + "Description", + "Last Updated" + }; + } + + @Override + protected PartialList<?> getItems(Query query) { + return profileService.search(query, Persona.class); + } + + @Override + protected String[] buildRow(Object item) { + Persona persona = (Persona) item; + return new String[] { + persona.getItemId(), + (String) persona.getProperty("firstName"), + (String) persona.getProperty("lastName"), + (String) persona.getProperty("email"), + (String) persona.getProperty("description"), + persona.getSystemProperties().get("lastUpdated").toString() + }; Review Comment: persona.getSystemProperties().get("lastUpdated") may be null; calling toString() on it will throw a NullPointerException. Prefer null-safe formatting (e.g., check for null and output an empty string or "-"), similar to ProfileCrudCommand’s handling. ########## tools/shell-dev-commands/src/main/java/org/apache/unomi/shell/dev/commands/consents/ConsentCrudCommand.java: ########## @@ -0,0 +1,244 @@ +/* + * 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.shell.dev.commands.consents; + +import com.fasterxml.jackson.databind.ObjectMapper; +import org.apache.unomi.api.Consent; +import org.apache.unomi.api.PartialList; +import org.apache.unomi.api.Profile; +import org.apache.unomi.api.query.Query; +import org.apache.unomi.api.services.ProfileService; +import org.apache.unomi.persistence.spi.CustomObjectMapper; +import org.apache.unomi.shell.dev.services.BaseCrudCommand; +import org.apache.unomi.shell.dev.services.CrudCommand; +import org.osgi.service.component.annotations.Component; +import org.osgi.service.component.annotations.Reference; + +import java.text.SimpleDateFormat; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.stream.Collectors; + +/** + * A command to perform CRUD operations on consents + */ +@Component(service = CrudCommand.class, immediate = true) +public class ConsentCrudCommand extends BaseCrudCommand { + + private static final ObjectMapper OBJECT_MAPPER = new CustomObjectMapper(); + private static final SimpleDateFormat DATE_FORMAT = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss.SSSZ"); + private static final List<String> PROPERTY_NAMES = List.of( + "profileId", "scope", "typeIdentifier", "status", "statusDate", "revokeDate" + ); Review Comment: DATE_FORMAT is a static SimpleDateFormat, which is not thread-safe. Shell commands can be executed concurrently (multiple sessions/threads), so this can cause corrupted formatting/parsing. Consider using a thread-safe formatter (e.g., DateTimeFormatter) or ThreadLocal like CommandUtils does. ########## tools/shell-dev-commands/src/main/java/org/apache/unomi/shell/dev/commands/rules/RuleStatisticsCrudCommand.java: ########## @@ -0,0 +1,135 @@ +/* + * 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.shell.dev.commands.rules; + +import com.fasterxml.jackson.databind.ObjectMapper; +import org.apache.unomi.api.PartialList; +import org.apache.unomi.api.query.Query; +import org.apache.unomi.api.rules.RuleStatistics; +import org.apache.unomi.api.services.RulesService; +import org.apache.unomi.persistence.spi.CustomObjectMapper; +import org.apache.unomi.shell.dev.services.BaseCrudCommand; +import org.apache.unomi.shell.dev.services.CrudCommand; +import org.osgi.service.component.annotations.Component; +import org.osgi.service.component.annotations.Reference; + +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.stream.Collectors; + +/** + * A command to perform CRUD operations on rule statistics + */ +@Component(service = CrudCommand.class, immediate = true) +public class RuleStatisticsCrudCommand extends BaseCrudCommand { + + private static final ObjectMapper OBJECT_MAPPER = new CustomObjectMapper(); + private static final List<String> PROPERTY_NAMES = List.of( + "itemId", "executionCount", "localExecutionCount", "conditionsTime", "localConditionsTime", "actionsTime", "localActionsTime", "lastSyncDate" + ); + + @Reference + private RulesService rulesService; + + @Override + public String getObjectType() { + return "rulestats"; + } + + @Override + protected String[] getHeadersWithoutTenant() { + return new String[]{"ID", "Executions", "Local Executions", "Conditions Time", "Local Conditions Time", "Actions Time", "Local Actions Time", "Last Sync"}; + } + + @Override + protected PartialList<?> getItems(Query query) { + // Get all rules and their statistics + Map<String, RuleStatistics> statisticsMap = rulesService.getAllRuleStatistics(); + List<RuleStatistics> statistics = new ArrayList<>(statisticsMap.values()); + return new PartialList<>(statistics, 0, statistics.size(), statistics.size(), PartialList.Relation.EQUAL); + } + + @Override + protected Comparable[] buildRow(Object item) { + RuleStatistics stats = (RuleStatistics) item; + return new Comparable[]{ + stats.getItemId(), + stats.getExecutionCount(), + stats.getLocalExecutionCount(), + stats.getConditionsTime(), + stats.getLocalConditionsTime(), + stats.getActionsTime(), + stats.getLocalActionsTime(), + stats.getLastSyncDate() != null ? stats.getLastSyncDate().toString() : "" + }; + } + + @Override + public Map<String, Object> read(String id) { + RuleStatistics stats = rulesService.getRuleStatistics(id); + if (stats == null) { + return null; + } + return OBJECT_MAPPER.convertValue(stats, Map.class); + } + + @Override + public String create(Map<String, Object> properties) { + // Note: RulesService doesn't provide a direct way to create rule statistics + // They are automatically managed by the rules engine + return null; + } Review Comment: create() returns null, which makes the generic CRUD handler report a generic failure. If this type is intentionally read-only, it would be clearer to throw UnsupportedOperationException with a helpful message (and/or document that create/update are not supported). ########## tools/shell-dev-commands/src/main/java/org/apache/unomi/shell/dev/commands/rules/RuleStatisticsCrudCommand.java: ########## @@ -0,0 +1,135 @@ +/* + * 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.shell.dev.commands.rules; + +import com.fasterxml.jackson.databind.ObjectMapper; +import org.apache.unomi.api.PartialList; +import org.apache.unomi.api.query.Query; +import org.apache.unomi.api.rules.RuleStatistics; +import org.apache.unomi.api.services.RulesService; +import org.apache.unomi.persistence.spi.CustomObjectMapper; +import org.apache.unomi.shell.dev.services.BaseCrudCommand; +import org.apache.unomi.shell.dev.services.CrudCommand; +import org.osgi.service.component.annotations.Component; +import org.osgi.service.component.annotations.Reference; + +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.stream.Collectors; + +/** + * A command to perform CRUD operations on rule statistics + */ +@Component(service = CrudCommand.class, immediate = true) +public class RuleStatisticsCrudCommand extends BaseCrudCommand { + + private static final ObjectMapper OBJECT_MAPPER = new CustomObjectMapper(); + private static final List<String> PROPERTY_NAMES = List.of( + "itemId", "executionCount", "localExecutionCount", "conditionsTime", "localConditionsTime", "actionsTime", "localActionsTime", "lastSyncDate" + ); + + @Reference + private RulesService rulesService; + + @Override + public String getObjectType() { + return "rulestats"; + } + + @Override + protected String[] getHeadersWithoutTenant() { + return new String[]{"ID", "Executions", "Local Executions", "Conditions Time", "Local Conditions Time", "Actions Time", "Local Actions Time", "Last Sync"}; + } + + @Override + protected PartialList<?> getItems(Query query) { + // Get all rules and their statistics + Map<String, RuleStatistics> statisticsMap = rulesService.getAllRuleStatistics(); + List<RuleStatistics> statistics = new ArrayList<>(statisticsMap.values()); + return new PartialList<>(statistics, 0, statistics.size(), statistics.size(), PartialList.Relation.EQUAL); + } + + @Override + protected Comparable[] buildRow(Object item) { + RuleStatistics stats = (RuleStatistics) item; + return new Comparable[]{ + stats.getItemId(), + stats.getExecutionCount(), + stats.getLocalExecutionCount(), + stats.getConditionsTime(), + stats.getLocalConditionsTime(), + stats.getActionsTime(), + stats.getLocalActionsTime(), + stats.getLastSyncDate() != null ? stats.getLastSyncDate().toString() : "" + }; + } + + @Override + public Map<String, Object> read(String id) { + RuleStatistics stats = rulesService.getRuleStatistics(id); + if (stats == null) { + return null; + } + return OBJECT_MAPPER.convertValue(stats, Map.class); + } + + @Override + public String create(Map<String, Object> properties) { + // Note: RulesService doesn't provide a direct way to create rule statistics + // They are automatically managed by the rules engine + return null; + } + + @Override + public void update(String id, Map<String, Object> properties) { + // Note: RulesService doesn't provide a direct way to update rule statistics + // They are automatically managed by the rules engine + } + + @Override + public void delete(String id) { + // Note: RulesService doesn't provide a direct way to delete individual rule statistics + // They are automatically managed by the rules engine + // You can use resetAllRuleStatistics() to reset all statistics to zero + rulesService.resetAllRuleStatistics(); + } Review Comment: delete(String id) ignores the provided id and resets *all* rule statistics. With the unified CRUD interface this is surprising and potentially dangerous (a user expects delete to affect only the requested resource). Consider either rejecting delete with UnsupportedOperationException, or renaming/exposing this as an explicit "reset-all" operation. ########## tools/shell-dev-commands/src/main/java/org/apache/unomi/shell/dev/commands/rules/RuleStatisticsCrudCommand.java: ########## @@ -0,0 +1,135 @@ +/* + * 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.shell.dev.commands.rules; + +import com.fasterxml.jackson.databind.ObjectMapper; +import org.apache.unomi.api.PartialList; +import org.apache.unomi.api.query.Query; +import org.apache.unomi.api.rules.RuleStatistics; +import org.apache.unomi.api.services.RulesService; +import org.apache.unomi.persistence.spi.CustomObjectMapper; +import org.apache.unomi.shell.dev.services.BaseCrudCommand; +import org.apache.unomi.shell.dev.services.CrudCommand; +import org.osgi.service.component.annotations.Component; +import org.osgi.service.component.annotations.Reference; + +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.stream.Collectors; + +/** + * A command to perform CRUD operations on rule statistics + */ +@Component(service = CrudCommand.class, immediate = true) +public class RuleStatisticsCrudCommand extends BaseCrudCommand { + + private static final ObjectMapper OBJECT_MAPPER = new CustomObjectMapper(); + private static final List<String> PROPERTY_NAMES = List.of( + "itemId", "executionCount", "localExecutionCount", "conditionsTime", "localConditionsTime", "actionsTime", "localActionsTime", "lastSyncDate" + ); + + @Reference + private RulesService rulesService; + + @Override + public String getObjectType() { + return "rulestats"; + } + + @Override + protected String[] getHeadersWithoutTenant() { + return new String[]{"ID", "Executions", "Local Executions", "Conditions Time", "Local Conditions Time", "Actions Time", "Local Actions Time", "Last Sync"}; + } + + @Override + protected PartialList<?> getItems(Query query) { + // Get all rules and their statistics + Map<String, RuleStatistics> statisticsMap = rulesService.getAllRuleStatistics(); + List<RuleStatistics> statistics = new ArrayList<>(statisticsMap.values()); + return new PartialList<>(statistics, 0, statistics.size(), statistics.size(), PartialList.Relation.EQUAL); + } + + @Override + protected Comparable[] buildRow(Object item) { + RuleStatistics stats = (RuleStatistics) item; + return new Comparable[]{ + stats.getItemId(), + stats.getExecutionCount(), + stats.getLocalExecutionCount(), + stats.getConditionsTime(), + stats.getLocalConditionsTime(), + stats.getActionsTime(), + stats.getLocalActionsTime(), + stats.getLastSyncDate() != null ? stats.getLastSyncDate().toString() : "" + }; + } + + @Override + public Map<String, Object> read(String id) { + RuleStatistics stats = rulesService.getRuleStatistics(id); + if (stats == null) { + return null; + } + return OBJECT_MAPPER.convertValue(stats, Map.class); + } + + @Override + public String create(Map<String, Object> properties) { + // Note: RulesService doesn't provide a direct way to create rule statistics + // They are automatically managed by the rules engine + return null; + } + + @Override + public void update(String id, Map<String, Object> properties) { + // Note: RulesService doesn't provide a direct way to update rule statistics + // They are automatically managed by the rules engine + } + + @Override + public void delete(String id) { + // Note: RulesService doesn't provide a direct way to delete individual rule statistics + // They are automatically managed by the rules engine + // You can use resetAllRuleStatistics() to reset all statistics to zero + rulesService.resetAllRuleStatistics(); + } + + @Override + public List<String> completePropertyNames(String prefix) { + return filterPropertyNames(PROPERTY_NAMES, prefix); + } + + @Override + public String getPropertiesHelp() { + return String.join("\n", + "Rule statistics are automatically managed by the rules engine and cannot be directly modified.", + "You can view statistics using the following properties:", + "", + "- itemId: The unique identifier of the rule statistics (matches the rule ID)", + "- executionCount: Total number of rule executions in the cluster", + "- localExecutionCount: Number of rule executions on this node since last sync", + "- conditionsTime: Total time spent evaluating conditions in the cluster (ms)", + "- localConditionsTime: Time spent evaluating conditions on this node since last sync (ms)", + "- actionsTime: Total time spent executing actions in the cluster (ms)", + "- localActionsTime: Time spent executing actions on this node since last sync (ms)", + "- lastSyncDate: Date of the last synchronization with the cluster", + "", + "Note: Use 'unomi:crud rulestats reset' to reset all rule statistics to zero." + ); Review Comment: The help text references `unomi:crud rulestats reset`, but the command only supports operations {create, read, update, delete, list, help}. This makes the guidance unusable/misleading; update the text to the actual supported operation (or add the missing operation). ########## tools/shell-dev-commands/src/main/java/org/apache/unomi/shell/dev/commands/segments/SegmentCrudCommand.java: ########## @@ -0,0 +1,195 @@ +/* + * 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.shell.dev.commands.segments; + +import com.fasterxml.jackson.databind.ObjectMapper; +import org.apache.commons.lang3.StringUtils; +import org.apache.unomi.api.Metadata; +import org.apache.unomi.api.PartialList; +import org.apache.unomi.api.query.Query; +import org.apache.unomi.api.segments.Segment; +import org.apache.unomi.api.services.SegmentService; +import org.apache.unomi.persistence.spi.CustomObjectMapper; +import org.apache.unomi.shell.dev.services.BaseCrudCommand; +import org.apache.unomi.shell.dev.services.CrudCommand; +import org.osgi.service.component.annotations.Component; +import org.osgi.service.component.annotations.Reference; +import org.apache.unomi.api.conditions.Condition; + +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.stream.Collectors; + +@Component(service = CrudCommand.class, immediate = true) +public class SegmentCrudCommand extends BaseCrudCommand { + + private static final ObjectMapper OBJECT_MAPPER = new CustomObjectMapper(); + private static final List<String> PROPERTY_NAMES = List.of( + "itemId", "name", "description", "scope", "condition", "metadata" + ); + private static final List<String> CONDITION_TYPES = List.of( + "booleanCondition", "profilePropertyCondition", "sessionPropertyCondition", "eventPropertyCondition", + "pastEventCondition", "matchAllCondition", "notCondition", "orCondition", "andCondition", + "profileSegmentCondition", "scoringCondition" + ); + + @Reference + private SegmentService segmentService; + + @Override + public String getObjectType() { + return "segment"; + } + + @Override + protected String[] getHeadersWithoutTenant() { + return new String[] { + "Activated", + "Hidden", + "Read-only", + "Identifier", + "Scope", + "Name", + "Tags", + "System tags" + }; + } + + @Override + protected PartialList<?> getItems(Query query) { + return segmentService.getSegmentMetadatas(query); + } + + @Override + protected Comparable[] buildRow(Object item) { + Metadata segmentMetadata = (Metadata) item; + ArrayList<Comparable> rowData = new ArrayList<>(); + rowData.add(segmentMetadata.isEnabled() ? "x" : ""); + rowData.add(segmentMetadata.isHidden() ? "x" : ""); + rowData.add(segmentMetadata.isReadOnly() ? "x" : ""); + rowData.add(segmentMetadata.getId()); + rowData.add(segmentMetadata.getScope()); + rowData.add(segmentMetadata.getName()); + rowData.add(StringUtils.join(segmentMetadata.getTags(), ",")); + rowData.add(StringUtils.join(segmentMetadata.getSystemTags(), ",")); + return rowData.toArray(new Comparable[0]); + } + + @Override + public String create(Map<String, Object> properties) { + Segment segment = OBJECT_MAPPER.convertValue(properties, Segment.class); + segmentService.setSegmentDefinition(segment); + return segment.getItemId(); + } + + @Override + public Map<String, Object> read(String id) { + Segment segment = segmentService.getSegmentDefinition(id); + if (segment == null) { + return null; + } + return OBJECT_MAPPER.convertValue(segment, Map.class); + } + + @Override + public void update(String id, Map<String, Object> properties) { + properties.put("itemId", id); + Segment segment = OBJECT_MAPPER.convertValue(properties, Segment.class); + segmentService.setSegmentDefinition(segment); + } + + @Override + public void delete(String id) { + segmentService.removeSegmentDefinition(id, false); + } + + @Override + public String getPropertiesHelp() { + return String.join("\n", + "Required properties:", + "- itemId: Segment ID (string)", + "- name: Segment name", + "- condition: Segment condition object", + "", + "Optional properties:", + "- description: Segment description", + "- scope: Segment scope", + "- metadata: Segment metadata", + "", + "Condition types:", + "- booleanCondition: Simple true/false condition", + "- profilePropertyCondition: Match profile property", + "- sessionPropertyCondition: Match session property", + "- eventPropertyCondition: Match event property", + "- pastEventCondition: Match past events", + "- matchAllCondition: Match all sub-conditions", + "- notCondition: Negate sub-condition", + "- orCondition: Match any sub-condition", + "- andCondition: Match all sub-conditions", + "- profileSegmentCondition: Match profile segment", + "- scoringCondition: Match scoring value" + ); + } + + @Override + public List<String> completePropertyNames(String prefix) { + return filterPropertyNames(PROPERTY_NAMES, prefix); + } + + @Override + public List<String> completePropertyValue(String propertyName, String prefix) { + if ("condition.type".equals(propertyName)) { + return CONDITION_TYPES.stream() + .filter(type -> type.startsWith(prefix)) + .collect(Collectors.toList()); + } else if ("scope".equals(propertyName)) { + return List.of(); + } + return List.of(); + } + + @Override + public List<String> completeId(String prefix) { + // Create a query to find segments that match the prefix + Query query = new Query(); + query.setLimit(20); // Reasonable limit for auto-completion + + try { + // If prefix is not empty, use it to filter results + if (!prefix.isEmpty()) { + Condition condition = new Condition(definitionsService.getConditionType("booleanCondition")); + condition.setParameter("operator", "startsWith"); + condition.setParameter("propertyName", "itemId"); + condition.setParameter("propertyValue", prefix); + query.setCondition(condition); Review Comment: This builds a booleanCondition but sets operator="startsWith" and propertyName/propertyValue parameters. booleanCondition only supports logical operators like "and"/"or" with subConditions, so this condition won’t work as intended and ID completion will likely always fail. Use a propertyCondition (e.g., sessionPropertyCondition) with comparisonOperator="startsWith" instead. ########## bom/pom.xml: ########## @@ -270,6 +270,12 @@ <artifactId>httpclient-osgi</artifactId> <version>${httpclient-osgi.version}</version> </dependency> + <dependency> + <groupId>org.apache.httpcomponents</groupId> + <artifactId>httpclient-osgi</artifactId> + <version>${httpclient-osgi.version}</version> + <type>bundle</type> + </dependency> <dependency> Review Comment: This dependency declaration duplicates the existing managed dependency for org.apache.httpcomponents:httpclient-osgi (same G/A/V) but changes only <type>. Duplicating dependencyManagement entries like this is confusing and can lead to unpredictable resolution; prefer a single managed entry and set <type> only where required. ########## tools/shell-dev-commands/src/main/java/org/apache/unomi/shell/dev/commands/consents/ConsentCrudCommand.java: ########## @@ -0,0 +1,244 @@ +/* + * 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.shell.dev.commands.consents; + +import com.fasterxml.jackson.databind.ObjectMapper; +import org.apache.unomi.api.Consent; +import org.apache.unomi.api.PartialList; +import org.apache.unomi.api.Profile; +import org.apache.unomi.api.query.Query; +import org.apache.unomi.api.services.ProfileService; +import org.apache.unomi.persistence.spi.CustomObjectMapper; +import org.apache.unomi.shell.dev.services.BaseCrudCommand; +import org.apache.unomi.shell.dev.services.CrudCommand; +import org.osgi.service.component.annotations.Component; +import org.osgi.service.component.annotations.Reference; + +import java.text.SimpleDateFormat; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.stream.Collectors; + +/** + * A command to perform CRUD operations on consents + */ +@Component(service = CrudCommand.class, immediate = true) +public class ConsentCrudCommand extends BaseCrudCommand { + + private static final ObjectMapper OBJECT_MAPPER = new CustomObjectMapper(); + private static final SimpleDateFormat DATE_FORMAT = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss.SSSZ"); + private static final List<String> PROPERTY_NAMES = List.of( + "profileId", "scope", "typeIdentifier", "status", "statusDate", "revokeDate" + ); + + @Reference + private ProfileService profileService; + + @Override + public String getObjectType() { + return "consent"; + } + + @Override + protected String[] getHeadersWithoutTenant() { + return new String[]{"Profile ID", "Scope", "Type", "Status", "Status Date", "Revoke Date"}; + } + + @Override + protected PartialList<?> getItems(Query query) { + // Since consents are stored within profiles, we need to get all profiles and extract their consents + PartialList<Profile> profiles = profileService.search(query, Profile.class); + List<Map<String, Object>> consents = new ArrayList<>(); + + for (Profile profile : profiles.getList()) { + Map<String, Object> profileProperties = profile.getProperties(); + if (profileProperties.containsKey("consents")) { + @SuppressWarnings("unchecked") + Map<String, Consent> profileConsents = (Map<String, Consent>) profileProperties.get("consents"); + for (Map.Entry<String, Consent> entry : profileConsents.entrySet()) { + Map<String, Object> consentMap = entry.getValue().toMap(DATE_FORMAT); + consentMap.put("profileId", profile.getItemId()); + consents.add(consentMap); + } + } + } + + return new PartialList<Map<String, Object>>(consents, profiles.getOffset(), profiles.getPageSize(), profiles.getTotalSize(), PartialList.Relation.EQUAL); + } Review Comment: The returned PartialList metadata (offset/pageSize/totalSize) is taken from the profile query, but the list content is consents. This can exceed the requested limit and makes pagination/warnings inaccurate. Consider applying maxEntries to consent rows (or at least set pageSize/totalSize to reflect the consent list you return). ########## tools/shell-dev-commands/src/main/java/org/apache/unomi/shell/dev/commands/goals/GoalCrudCommand.java: ########## @@ -0,0 +1,141 @@ +/* + * 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.shell.dev.commands.goals; + +import com.fasterxml.jackson.databind.ObjectMapper; +import org.apache.unomi.api.Metadata; +import org.apache.unomi.api.PartialList; +import org.apache.unomi.api.goals.Goal; +import org.apache.unomi.api.query.Query; +import org.apache.unomi.api.services.GoalsService; +import org.apache.unomi.persistence.spi.CustomObjectMapper; +import org.apache.unomi.shell.dev.services.BaseCrudCommand; +import org.apache.unomi.shell.dev.services.CrudCommand; +import org.osgi.service.component.annotations.Component; +import org.osgi.service.component.annotations.Reference; + +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; + +/** + * A command to perform CRUD operations on goals + */ +@Component(service = CrudCommand.class, immediate = true) +public class GoalCrudCommand extends BaseCrudCommand { + + private static final ObjectMapper OBJECT_MAPPER = new CustomObjectMapper(); + private static final List<String> PROPERTY_NAMES = List.of( + "itemId", "name", "description", "scope", "campaignId", "enabled" + ); + + @Reference + private GoalsService goalsService; + + @Override + public String getObjectType() { + return "goal"; + } + + @Override + protected String[] getHeadersWithoutTenant() { + return new String[]{"ID", "Name", "Description", "Scope", "Campaign ID", "Enabled"}; + } + + @Override + protected PartialList<?> getItems(Query query) { + // Convert Set<Metadata> to PartialList for consistency + Set<Metadata> metadatas = goalsService.getGoalMetadatas(query); + List<Goal> goals = metadatas.stream() + .map(metadata -> goalsService.getGoal(metadata.getId())) + .filter(goal -> goal != null) + .collect(Collectors.toList()); + return new PartialList<>(goals, goals.size(), 0, goals.size(), null); + } Review Comment: The PartialList constructor arguments are incorrect here (offset is set to goals.size(), pageSize is 0, and totalSizeRelation is null). This will break pagination metadata and can trigger incorrect warnings. Use query.getOffset() for offset, goals.size() for pageSize, and a non-null Relation (typically EQUAL). -- 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]
