Copilot commented on code in PR #7166: URL: https://github.com/apache/incubator-kie/pull/7166#discussion_r4262000182
########## kogito-jbpm/process-workitems/src/main/java/org/jbpm/process/workitem/builtin/ProcessWorkItemHandlerExceptionHandler.java: ########## @@ -0,0 +1,82 @@ +/* + * 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.jbpm.process.workitem.builtin; + +import org.kie.api.runtime.process.ProcessWorkItemHandlerException; +import org.kie.kogito.internal.process.workitem.KogitoWorkItem; +import org.kie.kogito.internal.process.workitem.KogitoWorkItemHandler; +import org.kie.kogito.internal.process.workitem.KogitoWorkItemManager; +import org.kie.kogito.internal.process.workitem.WorkItemTransition; + +public class ProcessWorkItemHandlerExceptionHandler extends AbstractExceptionHandlingTaskHandler { + + public static final String ERROR_HANDLER_PROCESS_ID = "ErrorHandlerProcessId"; + public static final String ERROR_HANDLING_STRATEGY = "ErrorHandlingStrategy"; + public static final String ERROR_HANDLING_RETRIES = "ErrorHandlingRetries"; + + public ProcessWorkItemHandlerExceptionHandler(KogitoWorkItemHandler originalTaskHandler) { + super(originalTaskHandler); + } + + public ProcessWorkItemHandlerExceptionHandler(Class<? extends KogitoWorkItemHandler> originalTaskHandlerClass) { + super(originalTaskHandlerClass); + } + + @Override + public void handleException(KogitoWorkItemManager manager, KogitoWorkItemHandler originalTaskHandler, KogitoWorkItem workItem, WorkItemTransition transition, Throwable cause) { + if (cause instanceof ProcessWorkItemHandlerException) { + throw (ProcessWorkItemHandlerException) cause; + } + + String processId = (String) workItem.getParameter(ERROR_HANDLER_PROCESS_ID); + String strategyStr = (String) workItem.getParameter(ERROR_HANDLING_STRATEGY); + Object retriesObj = workItem.getParameter(ERROR_HANDLING_RETRIES); + + if (processId != null && !processId.trim().isEmpty() && strategyStr != null && !strategyStr.trim().isEmpty()) { + ProcessWorkItemHandlerException.HandlingStrategy strategy; + try { + strategy = ProcessWorkItemHandlerException.HandlingStrategy.valueOf(strategyStr.trim().toUpperCase()); + } catch (IllegalArgumentException e) { + throw new IllegalArgumentException("Invalid " + ERROR_HANDLING_STRATEGY + " value: '" + strategyStr + "'. Expected RETRY, COMPLETE, ABORT, or RETHROW.", e); + } + + int retries = 0; + if (retriesObj instanceof Number) { + retries = ((Number) retriesObj).intValue(); + } else if (retriesObj instanceof String && !((String) retriesObj).trim().isEmpty()) { + try { + retries = Integer.parseInt(((String) retriesObj).trim()); + } catch (NumberFormatException nfe) { + throw new IllegalArgumentException("Invalid " + ERROR_HANDLING_RETRIES + " value: '" + retriesObj + "'. Expected an integer.", nfe); + } + } Review Comment: Using `Number.intValue()` can silently truncate/overflow (e.g., Long values > Integer.MAX_VALUE) and can also accept negative retries without any validation. Prefer converting with range checking (e.g., `Math.toIntExact(...)` for Long/Integer types) and validate retries >= 0, throwing an IllegalArgumentException when out of range/invalid to avoid hard-to-debug runtime behavior. ########## kogito-jbpm/process-workitems/src/main/java/org/kie/kogito/process/workitems/impl/KogitoDefaultWorkItemManager.java: ########## @@ -99,7 +101,14 @@ public KogitoWorkItemHandler getKogitoWorkItemHandler(String name) { @Override public void registerWorkItemHandler(String workItemName, KogitoWorkItemHandler handler) { - this.workItemHandlers.put(workItemName, handler); + this.workItemHandlers.put(workItemName, wrapIfNeeded(handler)); + } + + private static KogitoWorkItemHandler wrapIfNeeded(KogitoWorkItemHandler handler) { + if (handler == null || handler instanceof AbstractExceptionHandlingTaskHandler) { + return handler; + } + return new ProcessWorkItemHandlerExceptionHandler(handler); } Review Comment: `wrapIfNeeded` is duplicated in both `KogitoDefaultWorkItemManager` and `LightWorkItemManager` (including slightly different documentation). To avoid divergence, consider extracting this into a shared utility (or a common base class) used by both managers. ########## kogito-jbpm/process-workitems/src/main/java/org/jbpm/process/workitem/builtin/ProcessWorkItemHandlerExceptionHandler.java: ########## @@ -0,0 +1,82 @@ +/* + * 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.jbpm.process.workitem.builtin; + +import org.kie.api.runtime.process.ProcessWorkItemHandlerException; +import org.kie.kogito.internal.process.workitem.KogitoWorkItem; +import org.kie.kogito.internal.process.workitem.KogitoWorkItemHandler; +import org.kie.kogito.internal.process.workitem.KogitoWorkItemManager; +import org.kie.kogito.internal.process.workitem.WorkItemTransition; + +public class ProcessWorkItemHandlerExceptionHandler extends AbstractExceptionHandlingTaskHandler { + + public static final String ERROR_HANDLER_PROCESS_ID = "ErrorHandlerProcessId"; + public static final String ERROR_HANDLING_STRATEGY = "ErrorHandlingStrategy"; + public static final String ERROR_HANDLING_RETRIES = "ErrorHandlingRetries"; + + public ProcessWorkItemHandlerExceptionHandler(KogitoWorkItemHandler originalTaskHandler) { + super(originalTaskHandler); + } + + public ProcessWorkItemHandlerExceptionHandler(Class<? extends KogitoWorkItemHandler> originalTaskHandlerClass) { + super(originalTaskHandlerClass); + } + + @Override + public void handleException(KogitoWorkItemManager manager, KogitoWorkItemHandler originalTaskHandler, KogitoWorkItem workItem, WorkItemTransition transition, Throwable cause) { + if (cause instanceof ProcessWorkItemHandlerException) { + throw (ProcessWorkItemHandlerException) cause; + } + + String processId = (String) workItem.getParameter(ERROR_HANDLER_PROCESS_ID); + String strategyStr = (String) workItem.getParameter(ERROR_HANDLING_STRATEGY); + Object retriesObj = workItem.getParameter(ERROR_HANDLING_RETRIES); Review Comment: Directly casting work item parameters to String can throw ClassCastException at runtime if the parameter is provided as a non-String (e.g., via programmatic APIs or typed BPMN inputs). Consider reading these as Object and converting safely (e.g., `instanceof String` with `.trim()` or `String.valueOf(obj)` guarded for null) and, when the type is invalid, throw a clear IllegalArgumentException instead of failing with a ClassCastException. ########## kogito-jbpm/process-workitems/src/main/java/org/jbpm/process/workitem/builtin/ProcessWorkItemHandlerExceptionHandler.java: ########## @@ -0,0 +1,82 @@ +/* + * 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.jbpm.process.workitem.builtin; + +import org.kie.api.runtime.process.ProcessWorkItemHandlerException; +import org.kie.kogito.internal.process.workitem.KogitoWorkItem; +import org.kie.kogito.internal.process.workitem.KogitoWorkItemHandler; +import org.kie.kogito.internal.process.workitem.KogitoWorkItemManager; +import org.kie.kogito.internal.process.workitem.WorkItemTransition; + +public class ProcessWorkItemHandlerExceptionHandler extends AbstractExceptionHandlingTaskHandler { + + public static final String ERROR_HANDLER_PROCESS_ID = "ErrorHandlerProcessId"; + public static final String ERROR_HANDLING_STRATEGY = "ErrorHandlingStrategy"; + public static final String ERROR_HANDLING_RETRIES = "ErrorHandlingRetries"; + + public ProcessWorkItemHandlerExceptionHandler(KogitoWorkItemHandler originalTaskHandler) { + super(originalTaskHandler); + } + + public ProcessWorkItemHandlerExceptionHandler(Class<? extends KogitoWorkItemHandler> originalTaskHandlerClass) { + super(originalTaskHandlerClass); + } + + @Override + public void handleException(KogitoWorkItemManager manager, KogitoWorkItemHandler originalTaskHandler, KogitoWorkItem workItem, WorkItemTransition transition, Throwable cause) { + if (cause instanceof ProcessWorkItemHandlerException) { + throw (ProcessWorkItemHandlerException) cause; + } + + String processId = (String) workItem.getParameter(ERROR_HANDLER_PROCESS_ID); + String strategyStr = (String) workItem.getParameter(ERROR_HANDLING_STRATEGY); + Object retriesObj = workItem.getParameter(ERROR_HANDLING_RETRIES); + + if (processId != null && !processId.trim().isEmpty() && strategyStr != null && !strategyStr.trim().isEmpty()) { + ProcessWorkItemHandlerException.HandlingStrategy strategy; + try { + strategy = ProcessWorkItemHandlerException.HandlingStrategy.valueOf(strategyStr.trim().toUpperCase()); + } catch (IllegalArgumentException e) { + throw new IllegalArgumentException("Invalid " + ERROR_HANDLING_STRATEGY + " value: '" + strategyStr + "'. Expected RETRY, COMPLETE, ABORT, or RETHROW.", e); + } Review Comment: The list of expected strategy values is hard-coded in the error message, so it can become outdated if the enum changes. Consider generating the allowed values from `ProcessWorkItemHandlerException.HandlingStrategy.values()` to keep the message accurate automatically. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
