[Improvement-17855] A dependent node should't depend on the workflow it is in#17887
[Improvement-17855] A dependent node should't depend on the workflow it is in#17887Mrhs121 wants to merge 1 commit intoapache:devfrom
Conversation
|
| if (dependentItem.getDefinitionCode() == workflowDefinitionCode) { | ||
| throw new ServiceException(Status.WORKFLOW_NODE_HAS_CYCLE); | ||
| } |
There was a problem hiding this comment.
NIT: Maybe only check the dependeny type is DEPENDENT_ON_WORKFLOW? I am not clear if we shouldn't support dependent a task is in the same workflow.
And it's better to provide a method to check if the workflow exist cycle, since there are some extra case may stiil cause cycle, e.g. dependent a subworkflow node, the subworkflow contains a subworkflow task point to the parent workflow.
| if (dependentItem.getDefinitionCode() == workflowDefinitionCode) { | |
| throw new ServiceException(Status.WORKFLOW_NODE_HAS_CYCLE); | |
| } | |
| if (dependentItem.getDependentType() == DependentType.DEPENDENT_ON_WORKFLOW && dependentItem.getDefinitionCode() == workflowDefinitionCode) { | |
| throw new ServiceException(Status.WORKFLOW_NODE_HAS_CYCLE); | |
| } |
There was a problem hiding this comment.
Thank you for your review—your suggestion makes sense.
However, I’m concerned that this change could break the existing design, since the current documentation explicitly permits this configuration of the dependent node. I’m not yet sure which behavior should be considered correct; please see my comment for details: #17873 (comment)
There was a problem hiding this comment.
@Mrhs121 So we should not do any restrictions here, as there may be scenarios where the current workflow depends on the previous day's operations? It might be better to let users control all of this themselves.
So this PR only fix the lineage cycle?
|
I haven't figured out how to fix this behavior for now. turn the pr into a draft for now |
3 participants
Purpose of the pull request
close #17873
Brief change log
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