fix: managed workflow result is available even when exception is thrown #2552
Conversation
Fixes #2551 Signed-off-by: Chris Laprun <[email protected]>
| @@ -102,6 +105,8 @@ public WorkflowCleanupResult cleanup(P primary, Context<P> context) { | |||
| WorkflowCleanupExecutor<P> workflowCleanupExecutor = | |||
| new WorkflowCleanupExecutor<>(this, primary, context); | |||
| var result = workflowCleanupExecutor.cleanup(); | |||
| context.managedDependentResourceContext() | |||
| .put(DefaultManagedDependentResourceContext.CLEANUP_RESULT_KEY, result); | |||
| if (throwExceptionAutomatically) { | |||
There was a problem hiding this comment.
Shouldn't rather only this throw removed?
It seems to be redundant. If I remember correctly I was solving this for v5, there it seems to have different structure now.
There was a problem hiding this comment.
It's slightly different in v5 but the problem is still present in a somewhat different form, though.
| @Override | ||
| public Optional<WorkflowReconcileResult> getWorkflowReconcileResult() { | ||
| return Optional.ofNullable(workflowReconcileResult); | ||
| return get(RECONCILE_RESULT_KEY, WorkflowReconcileResult.class); |
There was a problem hiding this comment.
I don't think we should solve this this way (with the context map), only if there is no other way around this.
There was a problem hiding this comment.
I agree that it's not super nice but the current way is not great either (casting to the implementation, which is not a given, especially during tests since it breaks with mocks). I actually had a different implementation initially but I would like to remove the reliance on a specific implementation and a cast to it because I don't think it's a nice design.
Fixes #2551
Signed-off-by: Chris Laprun [email protected]