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.
voplica-git
force-pushed
the
feature/child-workflow-options
branch
from
3295d53 to
948ecc8
Compare
…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 - Expose the options through SyncWorkflowContext, served directly from the immutable WorkflowImplementationOptions - Add ChildWorkflowOptions.Builder#mergeChildWorkflowOptions(): non-null override fields win, except contextPropagators lists are concatenated (matching ActivityOptions.Builder#mergeActivityOptions), the mutually exclusive searchAttributes/typedSearchAttributes are merged as one logical field so the merged options never carry both flavors, and VERSIONING_INTENT_UNSPECIFIED is treated as unset - Resolve the predefined options for both typed and untyped child stubs (WorkflowInternal.newChildWorkflowStub and newUntypedChildWorkflowStub, so DynamicWorkflow parents are covered too) with the following precedence, highest to lowest, merged field by field: options passed to the stub creation method > per-type options (setChildWorkflowOptions) > default options (setDefaultChildWorkflowOptions) - Keep child stub creation cost unchanged: the interface metadata is computed once and passed into ChildWorkflowInvocationHandler instead of being reflectively re-derived - Keep the previous public WorkflowImplementationOptions constructor as a backward-compatible delegating overload - Document the new behavior on the Workflow child stub factory methods and warn against predefining workflowId, which would be applied to every child of the matching scope 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 javadocs on setChildWorkflowOptions and setDefaultChildWorkflowOptions describe the actual precedence. Tests: - Verify the applied options through the child's memo in DefaultChildWorkflowOptionsSetOnWorkflowTest (covering default, per-type, explicit and field-level merge precedence for both typed and untyped stubs), so a test only passes if the expected options actually took effect - Add an exhaustive unit test for ChildWorkflowOptions#mergeChildWorkflowOptions that exercises every field, plus dedicated tests for the search attribute flavors, context propagator concatenation and unspecified versioning intent Signed-off-by: Oleksandr Porunov <alexandr.porunov@gmail.com>
porunov
force-pushed
the
feature/child-workflow-options
branch
from
948ecc8 to
101bb90
Compare
|
@maciejdudko I've rebased in from |
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.