Refactor workflow engine code #13

sadilchamishka · 2025-07-04T16:52:33Z

Purpose

$subject

Related Issues

Improve workflow engine implementation wso2/product-is#24580

Depends on

Add workflow service methods wso2/carbon-identity-framework#6927

...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java

Malith-19 · 2025-07-05T04:47:33Z

...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java

+        String tenantDomain = CarbonContext.getThreadLocalCarbonContext().getTenantDomain();
+        List<String> roleIds = getAssignedRoleIds(userId, tenantDomain);
+        List<String> roleRequestsList;
+        List<String> lst = new ArrayList<>();


Suggested change

List<String> lst = new ArrayList<>();

List<String> combinedList = new ArrayList<>();

WDYT?

Can we remove this method and use the method getAllAssignedTasks It looks both methods do the same logic. Need to have close look. We may able to re use the same method passing a specific status.

...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java

sadilchamishka · 2025-07-05T05:21:15Z

...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java

+    public List<WorkflowAssociation> getAssociations(WorkflowRequest workflowRequest) {
+
+        List<WorkflowAssociation> associations = null;
+        WorkflowRequestAssociationDAO requestAssociationDAO = new WorkflowRequestAssociationDAO();
+        try {
+            associations = requestAssociationDAO.getWorkflowAssociationsForRequest(
+                    workflowRequest.getEventType(), workflowRequest.getTenantId());
+        } catch (InternalWorkflowException e) {
+        }
+        return associations;
+    }


This method need to check whether it's logic is correct. For a give workflow request it should return one workflow association.

When we have multiple workflows with the same association, eg: ADD_USER in workflow1 and workflow2 this will return multiple associations and therefore it is possible to return multiple associations.

sadilchamishka · 2025-07-05T05:22:25Z

...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java

+    public String getWorkflowId(WorkflowRequest request) {
+
+        WorkflowManagementService workflowManagementService = new WorkflowManagementServiceImpl();
+        List<WorkflowAssociation> associations = getAssociations(request);


Related the comment. https://github.com/wso2-extensions/identity-workflow/pull/13/files#r2186705368

Below for loop should not be necessary.

In here we are iterating through the all the associations we have for the same operation like ADD_USER. Only chance to have the multiple associations here is 2 workflows have same operation configured.

Still with the current logic this only the last workflow id will be picked up. That means all other workflows are neglected. IMO, we should not support to have same operation in 2 workflows as it creates conflicts like this.
cc: @sadilchamishka

CLAassistant · 2025-07-05T05:24:41Z

All committers have signed the CLA.

Copilot

Pull Request Overview

Refactors the workflow engine by removing legacy approval event code and migrating to a new approval task service API, cleaning up dependencies, and streamlining SQL constants.

Removes old test classes and Powermock setup in favor of a shared test utility.
Updates project POMs to drop internal WSO2 plugin repositories and obsolete dependencies.
Introduces ApprovalTaskDAO/WorkflowRequestDAO and ApprovalTaskServiceImpl; updates SQL constants.

Reviewed Changes

Copilot reviewed 21 out of 22 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
pom.xml	Removed internal plugin/repository blocks; updated framework & testutil versions
components/.../pom.xml	Swapped out Powermock for shared `org.wso2.carbon.identity.testutil` and cleaned test deps
components/.../util/WorkflowEngineConstants.java	Refactored SQL templates (e.g., `ADD_CURRENT_STEP_FOR_EVENT`) and renamed error messages
components/.../internal/dao/ApprovalTaskDAO.java & DAOImpl.java	New DAO interface/implementation for approval tasks; dropped deprecated methods
components/.../ApprovalTaskServiceImpl.java	Core service overhaul to list, retrieve, and update approval tasks

Comments suppressed due to low confidence (3)

components/org.wso2.carbon.identity.workflow.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java:1

The new ApprovalTaskServiceImpl and its DAO lack accompanying unit or integration tests; consider adding tests to cover core methods like listing tasks, updating statuses, and step progression.

/*

components/org.wso2.carbon.identity.workflow.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/util/WorkflowEngineConstants.java:57

Hardcoding the step value to '1' in the INSERT will ignore any dynamic currentStep; switch back to a placeholder (?,?,?) so the bound parameter is used.

                "(EVENT_ID,WORKFLOW_ID, CURRENT_STEP) VALUES (?,?,1)";

components/org.wso2.carbon.identity.workflow.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java:247

The method 'updateStateOfRequest(...)' isn't declared in ApprovalTaskDAO; add this signature to the DAO interface and implement it, or call the correct method, to avoid compilation errors.

            approvalTaskDAO.updateStateOfRequest(workflowRequestId, workflowId, currentStep);

...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java

sadilchamishka · 2025-07-07T05:03:32Z

...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java

+
+        String userId = CarbonContext.getThreadLocalCarbonContext().getUserId();
+
+        List<ApprovalTaskSummaryDTO> approvalTaskSummaryDTOS = getAllAssignedTasks(status, userId);


Try to improve getAllAssignedTasks to remove duplicated workflow requests, preserve reserved tasks etc..

sadilchamishka · 2025-07-07T05:06:10Z

...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java

+            approvalTaskSummaryDTO.setApprovalStatus(approvalTaskSummaryDTO.getApprovalStatus());
+        }
+
+        if (limit == null || limit < 0) {


Pagination improvements. Here or at the getAllAssignedTasks method level.

...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java

sadilchamishka · 2025-07-07T05:07:53Z

...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java

+        taskId = taskArray[0];
+        String requestId = approvalTaskDAO.getWorkflowRequestIdByApprovalTaskId(taskId);
+        WorkflowRequest request = getWorkflowRequest(requestId);
+        TaskDetails taskDetails = getTaskDetails(request);


We can comment at this moment.

sadilchamishka · 2025-07-07T05:10:10Z

...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java

+        TaskDetails taskDetails = getTaskDetails(request);
+        String initiator = workflowRequestDAO.getInitiatedUser(requestId);
+        List<String> approvers = approvalTaskDAO.listApprovers(taskId);
+        Map<String, String> assigneeMap = null;


We may not need this.

if approval task is blocked, share who has got assigned.

...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java

sadilchamishka · 2025-07-07T05:13:54Z

...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java

+        String statusValue = approvalTaskDAO.getApprovalTaskStatus(taskId);
+        approvalTaskDTO.setApprovalStatus(statusValue);
+        approvalTaskDTO.setInitiator(WorkflowEngineConstants.ParameterName.INITIATED_BY + initiator);
+        approvalTaskDTO.setPriority(WorkflowEngineConstants.ParameterName.PRIORITY);


We can remove this if no use case.

sadilchamishka · 2025-07-07T05:17:50Z

...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java

+                isAssignedApprovalTask = true;
+            }
+        } else {
+            List<String> roleIds = getAssignedRoleIds(userId, tenantDomain);


rename the method
userAssignedRoleIds

update get the method name of approverDTO. ex: getApproverId()

sadilchamishka · 2025-07-07T05:18:43Z

...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java

+        }
+    }
+
+    private void validateApprovers(String taskId) throws WorkflowEngineException {


Suggest good method name.

sadilchamishka · 2025-07-07T05:21:09Z

...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java

+                                maxStep = step;
+                            }
+                        } catch (NumberFormatException e) {
+                            throw new RuntimeException(e);


Don't return runtime exceptions.

Improve the algorithm.

sadilchamishka · 2025-07-07T05:22:23Z

...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java

+
+    private void handleApproval(String taskId) throws WorkflowEngineServerException {
+
+        String requestID = approvalTaskDAO.getWorkflowRequestIdByApprovalTaskId(taskId);


Check whether we can pass the request id.

sadilchamishka · 2025-07-07T05:23:29Z

...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java

+    private void handleApproval(String taskId) throws WorkflowEngineServerException {
+
+        String requestID = approvalTaskDAO.getWorkflowRequestIdByApprovalTaskId(taskId);
+        List<String> taskIds = approvalTaskDAO.getApprovalTasksByWorkflowRequestId(requestID);


Improve update the status by one database query.

If the completing step is the final step, clean the records in this table also.

sadilchamishka · 2025-07-07T05:25:49Z

...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java

+        }
+    }
+
+    private void handleRelease(String taskId) throws WorkflowEngineServerException {


Same apply for this method. https://github.com/wso2-extensions/identity-workflow/pull/13/files#r2188997886

sadilchamishka · 2025-07-07T05:27:09Z

...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java

+        String reservedStatus = WorkflowEngineConstants.TaskStatus.RESERVED.toString();
+        String blockedStatus = WorkflowEngineConstants.TaskStatus.BLOCKED.toString();
+
+        if (blockedStatus.equals(approvalTaskDAO.getApprovalTaskStatus(updatedApprovalTaskId))) {


As we validate Approvers at the begining, we may not need this again.

sadilchamishka · 2025-07-07T05:28:13Z

...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java

+    private void handleClaim(String updatedApprovalTaskId) throws WorkflowEngineException {
+
+        String userId = CarbonContext.getThreadLocalCarbonContext().getUserId();
+        String reservedStatus = WorkflowEngineConstants.TaskStatus.RESERVED.toString();


use getValue, getName and no need to assign to a variable.

sadilchamishka · 2025-07-07T05:31:33Z

...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java

+                    String newTaskId = UUID.randomUUID().toString();
+                    approvalTaskDAO.addApproversOfRequest(newTaskId, workflowRequestID, workflowId,
+                            ENTITY_TYPE_CLAIMED_USERS, userId, reservedStatus);
+                    approvalTaskDAO.updateApprovalTaskStatus(newTaskId, reservedStatus);


Check whether this is needed. As the add operation should do the same.

sadilchamishka · 2025-07-07T05:31:49Z

...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java

+            if (existingApprovalTaskId.equals(updatedApprovalTaskId)) {
+                if (WorkflowEngineConstants.APPROVER_TYPE_USERS.equals(approverType)) {
+                    approvalTaskDAO.updateApprovalTaskStatus(updatedApprovalTaskId, reservedStatus);
+                } else if (WorkflowEngineConstants.APPROVER_TYPE_ROLES.equals(approverType)) {


Add a comment explaining the business logic.

sadilchamishka · 2025-07-07T05:34:35Z

...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java

+                    approvalTaskDAO.addApproversOfRequest(newTaskId, workflowRequestID, workflowId,
+                            ENTITY_TYPE_CLAIMED_USERS, userId, reservedStatus);
+                    approvalTaskDAO.updateApprovalTaskStatus(newTaskId, reservedStatus);
+                    approvalTaskDAO.updateApprovalTaskStatus(updatedApprovalTaskId, blockedStatus);


Check whether we need to improve the logic to improve the performance.

sadilchamishka · 2025-07-07T05:35:33Z

...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java

+        wsWorkflowCallBackService.onCallback(wsWorkflowResponse);
+    }
+
+    private TaskDetails getTaskDetails(WorkflowRequest workflowRequest) throws WorkflowEngineException {


We don't have a use case for TASK_SUBJECT, TASK_DESCRIPTION at the moment.

sadilchamishka · 2025-07-07T05:36:26Z

...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java

+        return taskDetails;
+    }
+
+    private List<Parameter> getParameterList(WorkflowRequest request) throws WorkflowEngineException {


Need to check this logic. There should be bug.

sadilchamishka · 2025-07-07T05:39:00Z

...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java

+        List<RequestParameter> requestParameter;
+        for (int i = 0; i < request.getRequestParameters().size(); i++) {
+            requestParameter = request.getRequestParameters();
+            if (requestParameter.get(i).getName().equals(WorkflowEngineConstants.ParameterName.REQUEST_ID)) {


Use constant first.

sadilchamishka · 2025-07-07T05:40:15Z

...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java

+
+    public String getWorkflowId(WorkflowRequest request) throws WorkflowEngineException {
+
+        WorkflowManagementService workflowManagementService = new WorkflowManagementServiceImpl();


Get the service from the dataholder.

sadilchamishka · 2025-07-07T05:43:19Z

...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java

+        for (WorkflowAssociation association : associations) {
+            try {
+                Workflow workflow = workflowManagementService.getWorkflow(association.getWorkflowId());
+                workflowId = workflow.getWorkflowId();
+            } catch (WorkflowClientException e) {
+                throw new WorkflowEngineClientException(
+                        WorkflowEngineConstants.ErrorMessages.WORKFLOW_ID_NOT_FOUND.getCode(),
+                        WorkflowEngineConstants.ErrorMessages.WORKFLOW_ID_NOT_FOUND.getDescription());
+            } catch (WorkflowException e) {
+                throw new WorkflowEngineServerException(e.getErrorCode(), e.getMessage());
+            }
+        }
+        return workflowId;


Check the logic.

sadilchamishka · 2025-07-07T05:43:52Z

...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java

+        } catch (InternalWorkflowException e) {
+        }


Handle exception.

sadilchamishka · 2025-07-07T05:45:49Z

...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java

+            long cal = calendar.getTimeInMillis();
+            setCreatedTime(cal);
+            approvalTaskSummaryDTO.setId(approvalTaskSummaryDTO.getId());
+            approvalTaskSummaryDTO.setName(WorkflowEngineConstants.ParameterName.APPROVAL_TASK);


Remove this line.

sadilchamishka · 2025-07-07T05:46:33Z

...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java

+            String eventType = request.getEventType();
+
+            String workflowID = approvalTaskDAO.getWorkflowID(approvalTaskSummaryDTO.getId());
+            String workflowAssociationName = findAssociationNameByWorkflowAndEvent(workflowID, eventType);


This may not need now.

jenkins-is-staging · 2025-07-07T09:18:35Z

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/16112970873

jenkins-is-staging · 2025-07-07T09:58:42Z

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/16112970873
Status: failure

.../java/org/wso2/carbon/identity/workflow/engine/internal/dao/impl/WorkflowRequestDAOImpl.java

jenkins-is-staging · 2025-07-08T05:46:46Z

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/16135035038

jenkins-is-staging · 2025-07-08T06:24:35Z

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/16135035038
Status: failure

sadilchamishka · 2025-07-08T15:28:08Z

The comments will be addressed by a different PR.

The integration test was failed due to dependency with

Improve workflow approval code wso2/identity-api-user#268

The integration test will be run with both PRs merged. As we haven't onboard integration tests for the workflow approval, the code refactoring should not affect the integration tests.

sadilchamishka marked this pull request as draft July 4, 2025 16:54

Malith-19 reviewed Jul 5, 2025

View reviewed changes

...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java Outdated Show resolved Hide resolved

Malith-19 requested changes Jul 5, 2025

View reviewed changes

sadilchamishka commented Jul 5, 2025

View reviewed changes

sadilchamishka force-pushed the Refactor-workflow-engine branch from 905287e to a28f6ba Compare July 5, 2025 05:24

sadilchamishka force-pushed the Refactor-workflow-engine branch 2 times, most recently from 16dff73 to 8fdb7a3 Compare July 5, 2025 05:28

Refactor workflow engine code

d740cd1

sadilchamishka force-pushed the Refactor-workflow-engine branch from 8fdb7a3 to d740cd1 Compare July 5, 2025 05:29

Malith-19 requested a review from Copilot July 5, 2025 11:26

Copilot AI reviewed Jul 5, 2025

View reviewed changes

Refactor workflow engine code

d70a581

sadilchamishka mentioned this pull request Jul 7, 2025

Improve workflow approval code wso2/identity-api-user#268

Merged

Bump framework version

7050cb2

sadilchamishka marked this pull request as ready for review July 7, 2025 04:29

sadilchamishka commented Jul 7, 2025

View reviewed changes

...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java Show resolved Hide resolved

sadilchamishka commented Jul 7, 2025

View reviewed changes

...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java Show resolved Hide resolved

sadilchamishka commented Jul 7, 2025

View reviewed changes

...w.engine/src/main/java/org/wso2/carbon/identity/workflow/engine/ApprovalTaskServiceImpl.java Show resolved Hide resolved

sadilchamishka commented Jul 7, 2025

View reviewed changes

Malith-19 reviewed Jul 8, 2025

View reviewed changes

.../java/org/wso2/carbon/identity/workflow/engine/internal/dao/impl/WorkflowRequestDAOImpl.java Show resolved Hide resolved

Add the missing license header.

f7b8776

Malith-19 approved these changes Jul 8, 2025

View reviewed changes

sadilchamishka merged commit 3c23732 into wso2-extensions:main Jul 8, 2025
2 checks passed

Malith-19 mentioned this pull request Jul 14, 2025

Refactor the implementation - phase 2 #14

Merged

	List<String> lst = new ArrayList<>();
	List<String> combinedList = new ArrayList<>();


		String userId = CarbonContext.getThreadLocalCarbonContext().getUserId();

		List<ApprovalTaskSummaryDTO> approvalTaskSummaryDTOS = getAllAssignedTasks(status, userId);


		private void handleApproval(String taskId) throws WorkflowEngineServerException {

		String requestID = approvalTaskDAO.getWorkflowRequestIdByApprovalTaskId(taskId);


		public String getWorkflowId(WorkflowRequest request) throws WorkflowEngineException {

		WorkflowManagementService workflowManagementService = new WorkflowManagementServiceImpl();

Refactor workflow engine code #13

Refactor workflow engine code #13

Uh oh!

Conversation

sadilchamishka commented Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Related Issues

Depends on

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sadilchamishka Jul 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CLAassistant commented Jul 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sadilchamishka commented Jul 4, 2025 •

edited

Loading

sadilchamishka Jul 5, 2025 •

edited

Loading

CLAassistant commented Jul 5, 2025 •

edited

Loading

sadilchamishka commented Jul 8, 2025 •

edited

Loading