Refactor workflow init validation (#2316)

Refactor workflow init validation

Author: Quinn-With-Two-Ns

temporal-sdk/src/main/java/io/temporal/common/metadata/POJOWorkflowImplMetadata.java

@@ -175,14 +175,27 @@ private POJOWorkflowImplMetadata(
     this.queryMethods = ImmutableList.copyOf(queryMethods.values());
     this.updateMethods = ImmutableList.copyOf(updateMethods.values());
     this.updateValidatorMethods = ImmutableList.copyOf(updateValidatorMethods.values());
-    if (!listener && validateConstructor) {
+    if (!listener) {
       this.workflowInit =
-          ReflectionUtils.getConstructor(
+          ReflectionUtils.getWorkflowInitConstructor(
                   implClass,
                   this.workflowMethods.stream()
                       .map(POJOWorkflowMethodMetadata::getWorkflowMethod)
                       .collect(Collectors.toList()))
               .orElse(null);
+      if (validateConstructor) {
+        Constructor<?> defaultConstructor =
+            ReflectionUtils.getPublicDefaultConstructor(implClass).orElse(null);
+        if (defaultConstructor == null && this.workflowInit == null) {
+          throw new IllegalArgumentException(
+              "No default constructor or constructor annotated with @WorkflowInit found: "
+                  + implClass.getName());
+        } else if (defaultConstructor != null && this.workflowInit != null) {
+          throw new IllegalArgumentException(
+              "Found both a default constructor and constructor annotated with @WorkflowInit: "
+                  + implClass.getName());
+        }
+      }
     } else {
       this.workflowInit = null;
     }

temporal-sdk/src/main/java/io/temporal/internal/common/env/ReflectionUtils.java

@@ -32,37 +32,26 @@
 public final class ReflectionUtils {
   private ReflectionUtils() {}
 
-  public static Optional<Constructor<?>> getConstructor(
+  public static Optional<Constructor<?>> getWorkflowInitConstructor(
       Class<?> clazz, List<Method> workflowMethod) {
     // We iterate through all constructors to find the one annotated with @WorkflowInit
     // and check if it has the same parameters as the workflow method.
     // We check all declared constructors to find any constructors that are annotated with
-    // @WorkflowInit, but not public,
-    // to give a more informative error message.
+    // @WorkflowInit, but are not public, to give a more informative error message.
     Optional<Constructor<?>> workflowInit = Optional.empty();
-    Constructor<?> defaultConstructors = null;
     for (Constructor<?> ctor : clazz.getDeclaredConstructors()) {
       WorkflowInit wfInit = ctor.getAnnotation(WorkflowInit.class);
       if (wfInit == null) {
-        if (ctor.getParameterCount() == 0 && Modifier.isPublic(ctor.getModifiers())) {
-          if (workflowInit.isPresent() || defaultConstructors != null) {
-            throw new IllegalArgumentException(
-                "Multiple constructors annotated with @WorkflowInit or a default constructor found: "
-                    + clazz.getName());
-          }
-          defaultConstructors = ctor;
-          continue;
-        }
         continue;
       }
       if (workflowMethod.size() != 1) {
         throw new IllegalArgumentException(
             "Multiple interfaces implemented while using @WorkflowInit annotation. Only one is allowed: "
                 + clazz.getName());
       }
-      if (workflowInit.isPresent() || defaultConstructors != null) {
+      if (workflowInit.isPresent()) {
         throw new IllegalArgumentException(
-            "Multiple constructors annotated with @WorkflowInit or a default constructor found: "
+            "Multiple constructors annotated with @WorkflowInit found. Only one is allowed: "
                 + clazz.getName());
       }
       if (!Modifier.isPublic(ctor.getModifiers())) {
@@ -76,14 +65,25 @@ public static Optional<Constructor<?>> getConstructor(
       }
       workflowInit = Optional.of(ctor);
     }
-    if (!workflowInit.isPresent() && defaultConstructors == null) {
-      throw new IllegalArgumentException(
-          "No default constructor or constructor annotated with @WorkflowInit found: "
-              + clazz.getName());
-    }
     return workflowInit;
   }
 
+  public static Optional<Constructor<?>> getPublicDefaultConstructor(Class<?> clazz) {
+    Constructor<?> defaultConstructors = null;
+    for (Constructor<?> ctor : clazz.getDeclaredConstructors()) {
+      if (ctor.getParameterCount() != 0) {
+        continue;
+      }
+      if (!Modifier.isPublic(ctor.getModifiers())) {
+        throw new IllegalArgumentException(
+            "Default constructor must be public: " + clazz.getName());
+      }
+      defaultConstructors = ctor;
+      break;
+    }
+    return Optional.ofNullable(defaultConstructors);
+  }
+
   public static String getMethodNameForStackTraceCutoff(
       Class<?> clazz, String methodName, Class<?>... parameterTypes) throws RuntimeException {
     try {

temporal-sdk/src/main/java/io/temporal/internal/sync/POJOWorkflowImplementationFactory.java

@@ -191,7 +191,7 @@ private <T> void registerWorkflowImplementationType(
         Method executeMethod =
             workflowImplementationClass.getMethod("execute", EncodedValues.class);
         Optional<Constructor<?>> ctor =
-            ReflectionUtils.getConstructor(
+            ReflectionUtils.getWorkflowInitConstructor(
                 workflowImplementationClass, Collections.singletonList(executeMethod));
         dynamicWorkflowImplementationFactory =
             (encodedValues) -> {

temporal-sdk/src/test/java/io/temporal/common/metadata/POJOWorkflowImplMetadataTest.java

@@ -196,7 +196,7 @@ public void testWorkflowWithMultipleInit() {
       assertTrue(
           e.getMessage()
               .contains(
-                  "Multiple constructors annotated with @WorkflowInit or a default constructor found:"));
+                  "Found both a default constructor and constructor annotated with @WorkflowInit"));
     }
   }