Activity options merging logic is not correct

[mfateev]

Expected Behavior

There are six potential sources for activity options:

1. WorkflowImplementationOptions#defaultActivityOptions
2. WorkflowImplementationOptions#activityOptions map
3. Workflow.setDefaultActivityOptions
4. Workflow.applyActivityOptions map
5. options argument passed to WorkflowInternal#newActivityStub
6. activityMethodOptions map argument passed to WorkflowInternal#newActivityStub

When all these options are specified for the given activity type, the logical behavior is to merge them in the same order as specified above.

Actual Behavior

The current logic is split between:

https://github.com/temporalio/sdk-java/blob/8af4a2647dacab2cb0d4a3a613482e00bc297a39/temporal-sdk/src/main/java/io/temporal/internal/sync/WorkflowInternal.java#L303

and

https://github.com/temporalio/sdk-java/blob/16755a1bb7ebba29fb820b86a001c079cf4ffb62/temporal-sdk/src/main/java/io/temporal/internal/sync/ActivityInvocationHandler.java#L70

The short description is:

1. options = options argument passed to [WorkflowInternal#newActivityStub] ? WorkflowImplementationOptions#defaultActivityOptions // not that they are not merged if the argument is present.

2. WorkflowImplementationOptions#activityOptions map is overridden by options found in activityMethodOptions map argument passed to WorkflowInternal#newActivityStub

3. options (from step 1) are overridden by a value (with the key equal to the activity type) from the map generated by step (2)

I believe that we should fix the merging logic to match the intuitive behavior explained in the "Expected Behavior" section.

[Quinn-With-Two-Ns]

Ignoring backwards compatibility for a moment

, the logical behavior is to merge them in the same order as specified above.

I would disagree that is intuitive or logical. Before reading the docs I would not expect all the options to be merged in order. Specifically I would not expect the default activity options to be merged with the specific activity options. If I pass a specific option it is because I don't want the default, I would not expect the default to still get merged if I passed a more specific option. Looking at the docs for setActivityOptions supports this.

[mfateev]

I see two separate questions here:

1. Do we merge options at all, or are they always replaced completely?
2. What is the precedence of merging/replacement?

I personally think we want merging as it allows overriding either specific values as well as all values.

The precedence outlined above is the only one that makes sense to me.

I see a specific exception for testing where the options sometimes need to be forcefully replaced through WorkflowTestEnvironment.

Looking at the docs for setActivityOptions supports this.

The doc explicitly says about merging:

 * Set individual activity options per activityType. Will be merged with the map from {@link
 * io.temporal.workflow.Workflow#newActivityStub(Class, ActivityOptions, Map)} which has the
 * highest precedence.

[Quinn-With-Two-Ns]

The precedence outlined above is the only one that makes sense to me.

I disagree, I find the outlined approach unintuitive for the reasons I state above , specifically merging defaultActivityOptions and activityOptions, and it disagrees with our documentation.

[mfateev]

Would you specify the currently implemented rules for the 6 sources of options and what is the logic behind them?

[Quinn-With-Two-Ns]

I am specifically talking about:

WorkflowImplementationOptions#defaultActivityOptions WorkflowImplementationOptions#activityOptions map

If I set a default value for something and a specific value I would not expect them to be merged, the documentation does not say they will be merged.

The documentation for setDefaultActivityOptions explicitly say it will be overwritten. Ignoring the subjective concept of what is intuitive, I think from a backwards compatibility perspective I don't think we should change how the default options interact.

     * These activity options have the lowest precedence across all activity options. Will be
     * overwritten entirely by {@link io.temporal.workflow.Workflow#newActivityStub(Class,
     * ActivityOptions)} and then by the individual activity options if any are set through {@link
     * #setActivityOptions(Map)}
     *
     * @param defaultActivityOptions ActivityOptions for all activities in the workflow.
     */

[Quinn-With-Two-Ns]

To clarify my opinion is default options should not be used if a more specific option for an activity type is specified. A default is a preselect option if no other is present, not a base to build more specific options form . This is how we document setDefaultActivityOptions

  /**
   * Sets the default activity options that will be used for activity stubs that have no {@link
   * ActivityOptions} specified.<br>
   * This overrides a value provided by {@link
   * WorkflowImplementationOptions#getDefaultActivityOptions}.<br>
   * A more specific per-activity-type option specified in {@link
   * WorkflowImplementationOptions#getActivityOptions} or {@link #applyActivityOptions(Map)} takes
   * precedence over this setting.
   *
   * @param defaultActivityOptions {@link ActivityOptions} to be used as a default
   */
  public static void setDefaultActivityOptions(ActivityOptions defaultActivityOptions) {
    WorkflowInternal.setDefaultActivityOptions(defaultActivityOptions);
  }

The only time the default options should be considered is if no options are provided so , ignoring null options, I believe that is only if this method is used to construct a stub.

Workflow.newActivityStub(Class<T> activityInterface)

Otherwise, If activity options are provides we should use the activity options, again ignoring null options, with this method

 /**
   * Creates client stub to activities that implement given interface.
   *
   * @param activityInterface interface type implemented by activities
   * @param options options that together with the properties of {@link
   *     io.temporal.activity.ActivityMethod} specify the activity invocation parameters
   * @param activityMethodOptions activity method-specific invocation parameters
   */
  public static <T> T newActivityStub(
      Class<T> activityInterface,
      ActivityOptions options,
      Map<String, ActivityOptions> activityMethodOptions) 

• WorkflowImplementationOptions#activityOptions
• Workflow.applyActivityOptions
• options
• activityMethodOptions

Edit: I believe this is the documented and intended behaviour, but the documentation and code is spread out enough that it is not trivial obvious which we should fix.