Adding a new parameter type List to a workflow input causes the data converter to fail

[antmendoza]

Expected Behavior

Adding new parameters to running/closed workflows should be safe for the data converter and converted to null during replay

Actual Behavior

Adding a new parameter of type List to a workflow input causes the data converter to fail.

From:

        @WorkflowMethod
        String execute(String name);

to :

        @WorkflowMethod
        String execute(String name, List<String> names);

throws during replay

Caused by: java.lang.ClassCastException: class sun.reflect.generics.reflectiveObjects.ParameterizedTypeImpl cannot be cast to class java.lang.Class (sun.reflect.generics.reflectiveObjects.ParameterizedTypeImpl and java.lang.Class are in module java.base of loader 'bootstrap')
	at io.temporal.common.converter.DataConverter.fromPayloads(DataConverter.java:162)
	at io.temporal.internal.sync.POJOWorkflowImplementationFactory$POJOWorkflowImplementation.execute(POJOWorkflowImplementationFactory.java:349)
	at io.temporal.internal.sync.WorkflowExecutionHandler.runWorkflowMethod(WorkflowExecutionHandler.java:71)

Adding a new parameter of type String or Custom object works as expected.

From:

        @WorkflowMethod
        String execute(String name);

to :

        @WorkflowMethod
        String execute(String name, String names);

        @WorkflowMethod
        String execute(String name, MyObject names);

Steps to Reproduce the Problem

I have attached a project that reproduces the issue

Archive.zip

Specifications

• Version: SDK 1.27.1
• Platform:

[cretz]

Adding new parameters to running/closed workflows should be safe for the data converter and converted to null during replay

Hrmm, I am not sure this is true across all SDKs. We discard extra arguments if too many come in, but I am not sure we make empty/zero values in all SDKs if the value is not there. In Go we might default to zero value because zero values are clear, but this is not necessarily the case with all SDKs. In .NET for instance we even have a specific error saying not enough payloads are present. If I had strict Java null annotation checking on in my analyzer and I only use the type-safe client calls, and my analyzer expects this parameter to be non-null, it would be surprising/bad if we just made it null. I believe adding a parameter is a backwards incompatible operation and is why we encourage using objects where fields can be added instead.

@Quinn-With-Two-Ns may need to double-check my assertions here.

EDIT: Ignore this, this issue is about Java's inconsistency with itself on extra parameters (generic vs non), not new functionality.

[antmendoza]

I believe adding a parameter is a backwards incompatible operation and is why we encourage using objects where fields can be added instead.

Agreed Chad.
I can update the request to have the data converter behaving consistently, regardless of the parameter type (meaning it should either always fail or never fail).

[cretz]

Oh, I see now, we do allow extra non-generic parameters, we just don't allow generic parameters due to a bug. Disregard my comments, we should fix it to be consistent I agree.

[antmendoza]

thanks @cretz