[Fix-17938][Master&ConditionLogicTask]throw an exception when the referenced dependent task does not exist #17939
Conversation
njnu-seafish
requested review from
SbloodyS,
caishunfeng and
ruanwenjun
as code owners
| log.info("The depend item: {} has not completed yet", DependResult.FAILED); | ||
| log.info("The dependent result will be {}", DependResult.FAILED); | ||
| return DependResult.FAILED; | ||
| // Dependent task not found – likely misconfigured task code |
There was a problem hiding this comment.
I don't understand why this should be changed. The current implementation is to put the task in a failed state.
There was a problem hiding this comment.
I don't understand why this should be changed. The current implementation is to put the task in a failed state.
Most critically, it will keep failing repeatedly—this is fundamentally different from a legitimate failure of a preceding task—and doing so would obscure the underlying configuration error.
| // Dependent task not found – likely misconfigured task code | ||
| log.error("Dependent task definition missing, depTaskCode: {}", item.getDepTaskCode()); | ||
| throw new RuntimeException(String.format( | ||
| "Dependency validation failed: task code %d not found in current workflow. Check dependency config: %s", | ||
| item.getDepTaskCode(), item)); |
There was a problem hiding this comment.
You should perform validation when the workflow insert/update or ConditionLogicTask start method.
The validation logic should be check if the task is exist at IWorkflowGraph.
In some normal cases, there may be no related task instances here, such as when execution starts from a specific node.
There was a problem hiding this comment.
In a ConditionLogicTask, dependencies should only reference task codes from preceding tasks in the workflow. Therefore, taskInstance == null should not occur under normal circumstances. If it does, it indicates that a non-existent task code has been configured, and the user should be given a clear error message about this misconfiguration.
There was a problem hiding this comment.
There was a problem hiding this comment.
instances here, such as when execution starts from a specific node.
If task pre-sh is skipped and execution proceeds directly to the condition task, should the condition task succeed or fail? I still think throwing an exception is more reasonable in this case.
There was a problem hiding this comment.
The condition task might contains multiple predecessors, you cannot simple throw exception if one task instance is null here.
| .isInstanceOf(RuntimeException.class) | ||
| .hasMessageContaining("Dependency validation failed") | ||
| .hasMessageContaining("task code 999 not found in current workflow"); | ||
| .hasMessageContaining("task 999 not found or skipped in current workflow"); |
There was a problem hiding this comment.
Have you run the test? The test shouldn't work.
3 participants
Purpose of the pull request
close #17938
Brief change log
throw an exception when the referenced dependent task does not exist
Verify this pull request
This pull request is code cleanup without any test coverage.
(or)
This pull request is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(or)
Pull Request Notice
Pull Request Notice
If your pull request contains incompatible change, you should also add it to
docs/docs/en/guide/upgrade/incompatible.md