Add ChildWorkflowOptions support to WorkflowImplementationOptions (#2)#2887
Add ChildWorkflowOptions support to WorkflowImplementationOptions (#2)#2887porunov wants to merge 1 commit into
Conversation
|
|
|
|
porunov
force-pushed
the
feature/child-workflow-options
branch
from
bd8e2d9 to
3295d53
Compare
maciejdudko
left a comment
maciejdudko
left a comment
There was a problem hiding this comment.
Hi @porunov, thank you for your contribution. The code looks good, but the tests need to be improved before we can merge this - see comment.
| @Test | ||
| public void testDefaultChildWorkflowOptionsApplied() { |
There was a problem hiding this comment.
This test does not actually assert that the right options were used. Please rewrite it so that the child workflow only succeeds if the default options were correctly applied. I think checking its memo would be most straight forward.
Please also add a test that verifies default, not per-type child workflow options work too.
There was a problem hiding this comment.
Thank you @maciejdudko for pointing at it. There was a bug when the default options would overwrite per type options, but it's now fixed. Also added the tests as you requested. All tests are passing now. Let me know please if there are any other concerns with this PR.
…plementationOptions Fixes temporalio#2790 Allow predefining ChildWorkflowOptions on WorkflowImplementationOptions, mirroring the existing ActivityOptions, LocalActivityOptions and NexusServiceOptions support: - Add setChildWorkflowOptions(Map) and setDefaultChildWorkflowOptions() builder methods, matching getters, and equals/hashCode/toString/toBuilder support on WorkflowImplementationOptions - Store and expose the options on SyncWorkflowContext - Add ChildWorkflowOptions.Builder#mergeChildWorkflowOptions() - Resolve the options in WorkflowInternal.newChildWorkflowStub() with the following precedence, highest to lowest, merged field by field: options passed to newChildWorkflowStub > per-type options (setChildWorkflowOptions) > default options (setDefaultChildWorkflowOptions) Fix child workflow options precedence: the predefined options were merged in the wrong order, so when no options were passed to newChildWorkflowStub the default options overrode the per-type options. The default options have the lowest precedence and must never override per-type options. The merge order is now correct, and the contradictory javadocs on setChildWorkflowOptions and setDefaultChildWorkflowOptions now describe the actual precedence. Tests: - Rewrite DefaultChildWorkflowOptionsSetOnWorkflowTest to verify the applied options through the child's memo (covering default, per-type, explicit and field-level merge precedence), so a test only passes if the expected options actually took effect - Add an exhaustive unit test for ChildWorkflowOptions#mergeChildWorkflowOptions that exercises every field Signed-off-by: Oleksandr Porunov <alexandr.porunov@gmail.com>
voplica-git
force-pushed
the
feature/child-workflow-options
branch
from
3295d53 to
948ecc8
Compare
3 participants
Fixes #2790
Tests are added to:
ChildWorkflowOptionsInWorkflowImplementationOptionsTest.javaPotential documentation place: https://docs.temporal.io/develop/java/workflows/child-workflows (it could be something similar as activity options documentation).
Disclaimer: Code was Generated by Opus 4.6 in Copilot. Reviewed manually.