Add schedule task enable/disable/trigger functionality #4531
Conversation
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
|
|
||
| private void handlePost(MessageContext msgCtx, org.apache.axis2.context.MessageContext axisMsgCtx) { | ||
|
|
There was a problem hiding this comment.
Log Improvement Suggestion No: 1
| private void handlePost(MessageContext msgCtx, org.apache.axis2.context.MessageContext axisMsgCtx) { | |
| private void handlePost(MessageContext msgCtx, org.apache.axis2.context.MessageContext axisMsgCtx) { | |
| log.info("Processing POST request for task management"); |
| org.apache.axis2.context.MessageContext axis2MessageContext = | ||
| ((Axis2MessageContext) messageContext).getAxis2MessageContext(); | ||
| JSONObject jsonResponse = new JSONObject(); | ||
| StartUpController controllerTask = (StartUpController)task; |
There was a problem hiding this comment.
Log Improvement Suggestion No: 2
| StartUpController controllerTask = (StartUpController)task; | |
| StartUpController controllerTask = (StartUpController)task; | |
| if (log.isDebugEnabled()) { | |
| log.debug("Processing status update for task: " + name + " to status: " + status); | |
| } |
There was a problem hiding this comment.
AI Agent Log Improvement Checklist
- The log-related comments and suggestions in this review were generated by an AI tool to assist with identifying potential improvements. Purpose of reviewing the code for log improvements is to improve the troubleshooting capabilities of our products.
- Please make sure to manually review and validate all suggestions before applying any changes. Not every code suggestion would make sense or add value to our purpose. Therefore, you have the freedom to decide which of the suggestions are helpful.
✅ Before merging this pull request:
- Review all AI-generated comments for accuracy and relevance.
- Complete and verify the table below. We need your feedback to measure the accuracy of these suggestions and the value they add. If you are rejecting a certain code suggestion, please mention the reason briefly in the suggestion for us to capture it.
| Comment | Accepted (Y/N) | Reason |
|---|---|---|
| #### Log Improvement Suggestion No: 1 | ||
| #### Log Improvement Suggestion No: 2 |
|
Warning Rate limit exceeded@chanikag has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 54 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughAdds POST support to TaskResource to change task status (activate, deactivate, trigger) with permission checks, audit logging, and logging; adds two new constants ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TaskResource
participant SecurityUtils
participant StartUpController
participant AuditLogger
Client->>TaskResource: POST /tasks/{name} + JSON {status, ...}
activate TaskResource
TaskResource->>TaskResource: parse & validate payload
alt task exists
TaskResource->>SecurityUtils: check edit permission for user
SecurityUtils-->>TaskResource: allowed/denied
alt allowed
TaskResource->>StartUpController: perform status change (activate/deactivate/trigger)
StartUpController-->>TaskResource: result (success/failure)
alt success
TaskResource->>AuditLogger: emit audit event (type: task)
TaskResource-->>Client: 200 OK + updated task JSON
else failure
TaskResource-->>Client: 4xx/5xx with error
end
else denied
TaskResource-->>Client: 403 Forbidden
end
else not found
TaskResource-->>Client: 404 Not Found
end
deactivate TaskResource
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
components/org.wso2.micro.integrator.extensions/org.wso2.micro.integrator.management.apis/src/main/java/org/wso2/micro/integrator/management/apis/TaskResource.java (1)
100-114: Redundant Axis2 message context retrieval.
axis2MessageContextis already obtained at lines 86-87. TheaxisMsgCtxat lines 102-103 is redundant.} else { String userName = (String) messageContext.getProperty(USERNAME_PROPERTY); - org.apache.axis2.context.MessageContext axisMsgCtx = - ((Axis2MessageContext) messageContext).getAxis2MessageContext(); try { if (SecurityUtils.canUserEdit(userName)) { - handlePost(messageContext, axisMsgCtx); + handlePost(messageContext, axis2MessageContext); } else { - Utils.sendForbiddenFaultResponse(axisMsgCtx); + Utils.sendForbiddenFaultResponse(axis2MessageContext); } } catch (UserStoreException e) { log.error("Error occurred while retrieving the user data", e); - Utils.setJsonPayLoad(axisMsgCtx, Utils.createJsonErrorObject("Error occurred while retrieving the user data")); + Utils.setJsonPayLoad(axis2MessageContext, Utils.createJsonErrorObject("Error occurred while retrieving the user data")); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/org.wso2.micro.integrator.extensions/org.wso2.micro.integrator.management.apis/src/main/java/org/wso2/micro/integrator/management/apis/Constants.java(1 hunks)components/org.wso2.micro.integrator.extensions/org.wso2.micro.integrator.management.apis/src/main/java/org/wso2/micro/integrator/management/apis/InboundEndpointResource.java(1 hunks)components/org.wso2.micro.integrator.extensions/org.wso2.micro.integrator.management.apis/src/main/java/org/wso2/micro/integrator/management/apis/TaskResource.java(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
components/org.wso2.micro.integrator.extensions/org.wso2.micro.integrator.management.apis/src/main/java/org/wso2/micro/integrator/management/apis/TaskResource.java (3)
components/org.wso2.micro.integrator.core/src/main/java/org/wso2/micro/core/util/AuditLogger.java (1)
AuditLogger(28-41)components/org.wso2.micro.integrator.extensions/org.wso2.micro.integrator.management.apis/src/main/java/org/wso2/micro/integrator/management/apis/security/handler/SecurityUtils.java (1)
SecurityUtils(37-150)components/org.wso2.micro.integrator.extensions/org.wso2.micro.integrator.management.apis/src/main/java/org/wso2/micro/integrator/management/apis/Constants.java (1)
Constants(25-247)
🔇 Additional comments (6)
components/org.wso2.micro.integrator.extensions/org.wso2.micro.integrator.management.apis/src/main/java/org/wso2/micro/integrator/management/apis/InboundEndpointResource.java (1)
29-29: Import path update looks correct.The import relocation for
DynamicControlOperationResultfromorg.apache.synapse.inboundtoorg.apache.synapse.utilis consistent with the same import inTaskResource.java.components/org.wso2.micro.integrator.extensions/org.wso2.micro.integrator.management.apis/src/main/java/org/wso2/micro/integrator/management/apis/Constants.java (1)
119-122: New constant appropriately placed.The
TRIGGER_STATUSconstant is logically grouped with the other Synapse service status constants and follows the established naming convention.components/org.wso2.micro.integrator.extensions/org.wso2.micro.integrator.management.apis/src/main/java/org/wso2/micro/integrator/management/apis/TaskResource.java (4)
22-67: Imports and logger setup look appropriate.The new imports support the POST handling, security checks, and audit logging functionality. The logger declaration follows the standard pattern used in similar resource classes.
73-79: HTTP POST method registration is correct.Adding
HTTP_POSTenables the new task enable/disable/trigger functionality and is consistent with the pattern inInboundEndpointResource.
171-176: Good use of constant for JSON key.Using the
NAMEconstant instead of a hardcoded string improves maintainability.
275-296: Consistent use of NAME constant.The change at line 279 aligns with the pattern used elsewhere in this file.
...or.management.apis/src/main/java/org/wso2/micro/integrator/management/apis/TaskResource.java
Show resolved
Hide resolved
...or.management.apis/src/main/java/org/wso2/micro/integrator/management/apis/TaskResource.java
Show resolved
Hide resolved
...or.management.apis/src/main/java/org/wso2/micro/integrator/management/apis/TaskResource.java
Outdated
Show resolved
Hide resolved
...or.management.apis/src/main/java/org/wso2/micro/integrator/management/apis/TaskResource.java
Show resolved
Hide resolved
...or.management.apis/src/main/java/org/wso2/micro/integrator/management/apis/TaskResource.java
Outdated
Show resolved
Hide resolved
...or.management.apis/src/main/java/org/wso2/micro/integrator/management/apis/TaskResource.java
Outdated
Show resolved
Hide resolved
| if (task instanceof StartUpController) { | ||
| controllerTask = (StartUpController)task; | ||
| } | ||
| if (controllerTask == null) { |
There was a problem hiding this comment.
We can have this for the else block of if condition at L320
...or.management.apis/src/main/java/org/wso2/micro/integrator/management/apis/TaskResource.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
components/org.wso2.micro.integrator.extensions/org.wso2.micro.integrator.management.apis/src/main/java/org/wso2/micro/integrator/management/apis/TaskResource.java (1)
136-138: Add else branch for missing STATUS field.When the payload contains
namebut lacksstatus,responseremainsnulland is set as the payload at line 146. This can result in a null or undefined response being sent to the client.Apply this diff to handle the missing
statuscase:if (payload.has(STATUS)) { response = handleStatusUpdate(task, performedBy, info, msgCtx, payload); + } else { + response = Utils.createJsonError("Missing 'status' field in payload", axisMsgCtx, Constants.BAD_REQUEST); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/org.wso2.micro.integrator.extensions/org.wso2.micro.integrator.management.apis/src/main/java/org/wso2/micro/integrator/management/apis/TaskResource.java(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
components/org.wso2.micro.integrator.extensions/org.wso2.micro.integrator.management.apis/src/main/java/org/wso2/micro/integrator/management/apis/TaskResource.java (3)
components/org.wso2.micro.integrator.core/src/main/java/org/wso2/micro/core/util/AuditLogger.java (1)
AuditLogger(28-41)components/org.wso2.micro.integrator.extensions/org.wso2.micro.integrator.management.apis/src/main/java/org/wso2/micro/integrator/management/apis/security/handler/SecurityUtils.java (1)
SecurityUtils(37-150)components/org.wso2.micro.integrator.extensions/org.wso2.micro.integrator.management.apis/src/main/java/org/wso2/micro/integrator/management/apis/Constants.java (1)
Constants(25-248)
🔇 Additional comments (7)
components/org.wso2.micro.integrator.extensions/org.wso2.micro.integrator.management.apis/src/main/java/org/wso2/micro/integrator/management/apis/TaskResource.java (7)
22-22: LGTM! Imports are appropriate for the new POST functionality.All new imports support the enable/disable/trigger feature: JSON payload parsing, logging, dynamic control operations, audit logging, and security checks.
Also applies to: 26-27, 34-35, 40-42, 44-44, 56-62
67-67: LGTM! Logger properly declared.The static logger is correctly initialized and used for error reporting in the new POST handling logic.
77-77: LGTM! POST support correctly added.The method properly extends the API resource to handle POST requests for task management operations.
92-114: LGTM! Proper security checks and error handling.The POST request flow correctly:
- Retrieves the authenticated username
- Validates edit permissions via
SecurityUtils.canUserEdit()- Returns 403 Forbidden for unauthorized users
- Handles
UserStoreExceptionwith appropriate logging
173-173: LGTM! Good use of constant for JSON key.Replacing hardcoded "name" strings with the
NAMEconstant improves maintainability and ensures consistency across the codebase.Also applies to: 215-215, 279-279
318-325: LGTM! Proper type checking and error handling.The null check for
controllerTaskcorrectly handles the case where a task exists but is not an instance ofStartUpController. This is not dead code—it's a defensive guard against tasks that cannot be controlled dynamically. The error message is also accurate.
327-344: LGTM! Correct audit log types for activate and deactivate.Both the deactivate (line 331) and activate (line 340) operations correctly use
Constants.AUDIT_LOG_TYPE_TASKfor audit logging, ensuring proper categorization of task-related operations.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
components/org.wso2.micro.integrator.extensions/org.wso2.micro.integrator.management.apis/src/main/java/org/wso2/micro/integrator/management/apis/TaskResource.java (1)
120-151: Add error handling when STATUS is missing from payload.If the payload contains
namebut notstatus, theresponsevariable remainsnullat line 136-138 and is set as the payload at line 146, which will cause issues. This was flagged in a previous review but remains unaddressed.Apply this diff to handle the missing STATUS case:
if (payload.has(STATUS)) { response = handleStatusUpdate(task, performedBy, info, msgCtx, payload); + } else { + response = Utils.createJsonError("Missing 'status' in payload", axisMsgCtx, Constants.BAD_REQUEST); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/org.wso2.micro.integrator.extensions/org.wso2.micro.integrator.management.apis/src/main/java/org/wso2/micro/integrator/management/apis/TaskResource.java(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
components/org.wso2.micro.integrator.extensions/org.wso2.micro.integrator.management.apis/src/main/java/org/wso2/micro/integrator/management/apis/TaskResource.java (3)
components/org.wso2.micro.integrator.core/src/main/java/org/wso2/micro/core/util/AuditLogger.java (1)
AuditLogger(28-41)components/org.wso2.micro.integrator.extensions/org.wso2.micro.integrator.management.apis/src/main/java/org/wso2/micro/integrator/management/apis/security/handler/SecurityUtils.java (1)
SecurityUtils(37-150)components/org.wso2.micro.integrator.extensions/org.wso2.micro.integrator.management.apis/src/main/java/org/wso2/micro/integrator/management/apis/Constants.java (1)
Constants(25-248)
🔇 Additional comments (6)
components/org.wso2.micro.integrator.extensions/org.wso2.micro.integrator.management.apis/src/main/java/org/wso2/micro/integrator/management/apis/TaskResource.java (6)
22-67: LGTM: Imports and logger setup are appropriate.The new imports and logger instance are correctly added to support POST operations, security checks, audit logging, and error handling.
74-79: LGTM: POST method correctly added.The resource now properly declares support for POST operations in addition to GET.
92-114: LGTM: Security checks properly implemented.POST requests are correctly guarded with
SecurityUtils.canUserEdit()checks, and forbidden responses are sent for unauthorized users. Error handling for user store exceptions is appropriate.
173-173: LGTM: Consistent use of NAME constant.Good refactoring to use the
NAMEconstant instead of hardcoded strings.
318-324: LGTM: Task type validation is correct.The
instanceofcheck and error handling are properly implemented. The previous issue with the error message has been addressed.
326-343: LGTM: Activate and deactivate audit logging is correct.The activate and deactivate branches now correctly use
AUDIT_LOG_TYPE_TASKwith appropriate action constants. Previous audit log type issues have been addressed.
...or.management.apis/src/main/java/org/wso2/micro/integrator/management/apis/TaskResource.java
Show resolved
Hide resolved
Add schedule task enble/disable/trigger This is to fix wso2#3567
2 participants
Add schedule task enble/disable/trigger
This is to fix #3567
Purpose
Goals
Approach
User stories
Release note
Documentation
Training
Certification
Marketing
Automation tests
Security checks
Samples
Related PRs
Migrations (if applicable)
Test environment
Learning
Summary by CodeRabbit
New Features
Security & Auditing
Chores
✏️ Tip: You can customize this high-level summary in your review settings.