temporalio / sdk-java

Temporal Java SDK
https://temporal.io
Apache License 2.0
225 stars 147 forks source link

Workflow.newExternalWorkflowStub #2180

Open mfateev opened 3 months ago

mfateev commented 3 months ago

Expected Behavior

The following works when using Workflow Client:

public interface Retryable {
    @SignalMethod
    void retryNow();
}

@WorkflowInterface
public interface FileProcessingWorkflow extends Retryable {

    @WorkflowMethod
    String processFile(Arguments args);

    @QueryMethod(name="history")
    List<String> getHistory();

    @QueryMethod
    String getStatus();

    @SignalMethod
    void abandon();
}

@WorkflowInterface
public interface MediaProcessingWorkflow extends Retryable {

    @WorkflowMethod
    String processBlob(Arguments args);
}

Retryable r1 = client.newWorkflowStub(Retryable.class, firstWorkflowId);
Retryable r2 = client.newWorkflowStub(Retryable.class, secondWorkflowId);
r1.retryNow();
r2.retryNow();

The same should work when signaling workflow from an external workflow:

Workflow.newExternalWorkflowStub(Retryable.class, firstWorkflowId);

Retryable r1 = Workflow.newExternalWorkflowStub(Retryable.class, firstWorkflowId);
Retryable r2 = Workflow.newExternalWorkflowStub(Retryable.class, secondWorkflowId);
r1.retryNow();
r2.retryNow();

Actual Behavior

The newExternalWorkflowStub fails with:

Caused by: java.lang.IllegalArgumentException: Missing required @WorkflowInterface annotation: interface ...Retryable
    at io.temporal.common.metadata.POJOWorkflowInterfaceMetadata.newInstanceInternal(POJOWorkflowInterfaceMetadata.java:179)
    at io.temporal.common.metadata.POJOWorkflowInterfaceMetadata.newInstance(POJOWorkflowInterfaceMetadata.java:108)
    at io.temporal.internal.sync.ExternalWorkflowInvocationHandler.<init>(ExternalWorkflowInvocationHandler.java:43)
    at io.temporal.internal.sync.WorkflowInternal.newExternalWorkflowStub(WorkflowInternal.java:412)
    at io.temporal.workflow.Workflow.newExternalWorkflowStub(Workflow.java:204)
cretz commented 3 months ago

Arguably it shouldn't even work on the client. It'd make one wonder why a @WorkflowInterface is even needed ever if we can just assume it's there. In Python and .NET at least, we require the client class passed in to have this (even if it's a base class, to signify user intent). But I can understand if Java we don't want that.

mfateev commented 3 months ago

@WorkflowInterface defines a workflow type. There is no "Retryable" workflow type to register.

cretz commented 3 months ago

I was under the impression the annotation is also meant to be required for caller use. I assume with this issue, we should accept basically any interface in Java including standard library ones (and that the client side already does)? I assume that it's at method call time on the proxy where we will fail for any non-signal/update/query-annotated method? It could be argued (and how we did it in .NET/Python) that if you're willing to define methods w/ Temporal annotations in an interface, you should mark that interface as one capable of being used as a workflow stub via the @Workflow annotation.

But I understand if in Java we expect methods at any level to have annotations but not classes. And of course there's value in consistency with client side. Can disregard my concern.

mfateev commented 3 months ago

The WorkflowClient.newWorkflowStub call validates that only methods that are annotated with signal/update/query are allowed at the interface.