Merge pull request #513: Fix hierarchical Workflow classes processing

temporalio · Oct 7, 2024 · 221afce · 221afce
2 parents b714bab + 057e261
commit 221afce
Show file tree

Hide file tree

Showing 15 changed files with 206 additions and 38 deletions.
diff --git a/src/Internal/Declaration/Graph/ClassNode.php b/src/Internal/Declaration/Graph/ClassNode.php
@@ -11,13 +11,10 @@
 
 namespace Temporal\Internal\Declaration\Graph;
 
+use Temporal\Tests\Workflow\Inheritance\ExtendingWorkflow;
+
 final class ClassNode implements NodeInterface
 {
-    /**
-     * @var \ReflectionClass
-     */
-    private \ReflectionClass $class;
-
     /**
      * @var array|null
      */
@@ -26,17 +23,35 @@ final class ClassNode implements NodeInterface
     /**
      * @param \ReflectionClass $class
      */
-    public function __construct(\ReflectionClass $class)
+    public function __construct(
+        private \ReflectionClass $class,
+    ) {}
+
+    public function getReflection(): \ReflectionClass
     {
-        $this->class = $class;
+        return $this->class;
     }
 
     /**
-     * @return \ReflectionClass
+     * Get all methods from the class and its parents without duplicates.
+     *
+     * @return array<non-empty-string, \ReflectionMethod>
+     *
+     * @throws \ReflectionException
      */
-    public function getReflection(): \ReflectionClass
-    {
-        return $this->class;
+    public function getAllMethods(): array {
+        /** @var array<non-empty-string, \ReflectionMethod> $result */
+        $result = [];
+
+        foreach ($this->getInheritance() as $classes) {
+            foreach ($classes as $class) {
+                foreach ($class->getReflection()->getMethods() as $method) {
+                    $result[$method->getName()] ??= $method;
+                }
+            }
+        }
+
+        return $result;
     }
 
     /**
@@ -50,8 +65,9 @@ public function count(): int
     }
 
     /**
-     * @param string $name
-     * @param bool $reverse
+     * Get a method with all the declared classes.
+     *
+     * @param non-empty-string $name
      * @return \Traversable<ClassNode, \ReflectionMethod>
      * @throws \ReflectionException
      */

diff --git a/src/Internal/Declaration/Instantiator/WorkflowInstantiator.php b/src/Internal/Declaration/Instantiator/WorkflowInstantiator.php
@@ -48,11 +48,11 @@ public function instantiate(PrototypeInterface $prototype): WorkflowInstance
      */
     protected function getInstance(PrototypeInterface $prototype): object
     {
-        $handler = $prototype->getHandler() ?? throw new InstantiationException(\sprintf(
+        $prototype->getHandler() ?? throw new InstantiationException(\sprintf(
             'Unable to instantiate workflow "%s" without handler method',
             $prototype->getID(),
         ));
 
-        return $handler->getDeclaringClass()->newInstanceWithoutConstructor();
+        return $prototype->getClass()->newInstanceWithoutConstructor();
     }
 }
diff --git a/src/Internal/Declaration/Reader/WorkflowReader.php b/src/Internal/Declaration/Reader/WorkflowReader.php
@@ -107,9 +107,7 @@ public function fromObject(object $object): WorkflowPrototype
      */
     protected function getWorkflowPrototypes(ClassNode $graph): \Traversable
     {
-        $class = $graph->getReflection();
-
-        foreach ($class->getMethods() as $reflection) {
+        foreach ($graph->getAllMethods() as $reflection) {
             if (!$this->isValidMethod($reflection)) {
                 continue;
             }
@@ -305,7 +303,7 @@ private function getPrototype(ClassNode $graph, \ReflectionMethod $handler): ?Wo
             //
             $contextualRetry = $previousRetry;
 
-            foreach ($group as $ctx => $method) {
+            foreach ($group as $method) {
                 /** @var MethodRetry $retry */
                 $retry = $this->reader->firstFunctionMetadata($method, MethodRetry::class);
 
@@ -335,15 +333,29 @@ private function getPrototype(ClassNode $graph, \ReflectionMethod $handler): ?Wo
                 //
                 //  - #[WorkflowInterface]
                 //
-                $interface = $this->reader->firstClassMetadata($ctx->getReflection(), WorkflowInterface::class);
+                /** @var \ReflectionClass|null $context */
+                $interface = $context = null;
+                foreach ($graph->getIterator() as $edges) {
+                    foreach ($edges as $node) {
+                        $interface = $this->reader->firstClassMetadata(
+                            $context = $node->getReflection(),
+                            WorkflowInterface::class,
+                        );
+
+                        if ($interface !== null) {
+                            break 2;
+                        }
+                    }
+                }
 
                 // In case
                 if ($interface === null) {
                     continue;
                 }
 
+                \assert($context !== null);
                 if ($prototype === null) {
-                    $prototype = $this->findProto($handler, $method);
+                    $prototype = $this->findProto($handler, $method, $context, $graph->getReflection());
                 }
 
                 if ($prototype !== null && $retry !== null) {
@@ -371,15 +383,18 @@ private function getDefaultPrototype(ClassNode $graph): WorkflowPrototype
     }
 
     /**
-     * @param \ReflectionMethod $handler
-     * @param \ReflectionMethod $ctx
+     * @param \ReflectionMethod $handler First method in the inheritance chain
+     * @param \ReflectionMethod $ctx Current method in the inheritance chain
+     * @param \ReflectionClass $interface Class or Interface with #[WorkflowInterface] attribute
+     * @param \ReflectionClass $class Target class
      * @return WorkflowPrototype|null
      */
-    private function findProto(\ReflectionMethod $handler, \ReflectionMethod $ctx): ?WorkflowPrototype
-    {
-        $reflection = $ctx->getDeclaringClass();
-
-        //
+    private function findProto(
+        \ReflectionMethod $handler,
+        \ReflectionMethod $ctx,
+        \ReflectionClass $interface,
+        \ReflectionClass $class,
+    ): ?WorkflowPrototype {
         // The name of the workflow handler must be generated based
         // method's name which can be redefined using #[WorkflowMethod]
         // attribute.
@@ -418,8 +433,8 @@ private function findProto(\ReflectionMethod $handler, \ReflectionMethod $ctx):
             );
         }
 
-        $name = $info->name ?? $reflection->getShortName();
+        $name = $info->name ?? $interface->getShortName();
 
-        return new WorkflowPrototype($name, $handler, $reflection);
+        return new WorkflowPrototype($name, $handler, $class);
     }
 }
diff --git a/src/Internal/Workflow/Process/Process.php b/src/Internal/Workflow/Process/Process.php
@@ -174,7 +174,7 @@ function (\Throwable $e): void {
     /**
      * @param \Closure(ValuesInterface): mixed $handler
      */
-    public function start(\Closure $handler, ValuesInterface $values = null, bool $deferred): void
+    public function start(\Closure $handler, ?ValuesInterface $values, bool $deferred): void
     {
         try {
             $this->makeCurrent();

diff --git a/src/Internal/Workflow/Process/Scope.php b/src/Internal/Workflow/Process/Scope.php
@@ -136,7 +136,7 @@ public function getContext(): WorkflowContext
     /**
      * @param \Closure(ValuesInterface): mixed $handler
      */
-    public function start(\Closure $handler, ValuesInterface $values = null, bool $deferred): void
+    public function start(\Closure $handler, ?ValuesInterface $values, bool $deferred): void
     {
         // Create a coroutine generator
         $this->coroutine = $this->call($handler, $values ?? EncodedValues::empty());

diff --git a/tests/Fixtures/src/Workflow/Inheritance/BaseWorkflowWithHandler.php b/tests/Fixtures/src/Workflow/Inheritance/BaseWorkflowWithHandler.php
@@ -0,0 +1,23 @@
+<?php
+
+/**
+ * This file is part of Temporal package.
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+declare(strict_types=1);
+
+namespace Temporal\Tests\Workflow\Inheritance;
+
+use Temporal\Workflow\WorkflowMethod;
+
+abstract class BaseWorkflowWithHandler
+{
+    /** @WorkflowMethod */
+    #[WorkflowMethod]
+    public function handler(): void
+    {
+    }
+}
diff --git a/tests/Fixtures/src/Workflow/Inheritance/ExtendingWorkflow.php b/tests/Fixtures/src/Workflow/Inheritance/ExtendingWorkflow.php
@@ -0,0 +1,20 @@
+<?php
+
+/**
+ * This file is part of Temporal package.
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+declare(strict_types=1);
+
+namespace Temporal\Tests\Workflow\Inheritance;
+
+use Temporal\Workflow\WorkflowInterface;
+
+/** @WorkflowInterface */
+#[WorkflowInterface]
+class ExtendingWorkflow extends BaseWorkflowWithHandler
+{
+}
diff --git a/tests/Functional/Client/AwaitTestCase.php b/tests/Functional/Client/AwaitTestCase.php
@@ -63,7 +63,7 @@ public function testAggregated(): void
                 'test3',
                 'test4'
             ],
-            $run->getResult(Type::TYPE_ARRAY)
+            $run->getResult(Type::TYPE_ARRAY, 3)
         );
     }
 

diff --git a/tests/Functional/Client/UntypedWorkflowStubTestCase.php b/tests/Functional/Client/UntypedWorkflowStubTestCase.php
@@ -18,7 +18,6 @@
 use Temporal\Exception\Failure\TerminatedFailure;
 use Temporal\Exception\IllegalStateException;
 use Temporal\Exception\InvalidArgumentException;
-use Temporal\Tests\Unit\Declaration\Fixture\WorkflowWithoutHandler;
 use Temporal\Workflow\WorkflowExecutionStatus;
 
 /**
@@ -264,7 +263,7 @@ public function testSignalRunningWorkflowWithInheritedSignal()
         $signaller = $client->newUntypedRunningWorkflowStub($workflowId, $workflowRunId);
         $signaller->signal('addValue', 'test1');
 
-        $result = $workflowRun->getResult();
+        $result = $workflowRun->getResult(timeout: 10);
         $this->assertEquals(['test1'], $result);
     }
 }
diff --git a/tests/Functional/SimpleWorkflowTestCase.php b/tests/Functional/SimpleWorkflowTestCase.php
@@ -10,6 +10,7 @@
 use Temporal\Client\WorkflowOptions;
 use Temporal\Testing\ActivityMocker;
 use Temporal\Tests\TestCase;
+use Temporal\Tests\Workflow\Inheritance\ExtendingWorkflow;
 use Temporal\Tests\Workflow\SimpleWorkflow;
 use Temporal\Tests\Workflow\YieldGeneratorWorkflow;
 use Temporal\Tests\Workflow\YieldScalarsWorkflow;
@@ -119,6 +120,13 @@ public function testYieldGenerator(): void
         $this->assertSame('bar', $run->getResult());
     }
 
+    public function testWorkflowMethodInAbstractParent(): void
+    {
+        $workflow = $this->workflowClient->newWorkflowStub(ExtendingWorkflow::class);
+        $run = $this->workflowClient->start($workflow);
+        $this->assertNull($run->getResult(timeout: 5));
+    }
+
     private function assertContainsEvent(WorkflowExecution $execution, int $event): void
     {
         $history = $this->workflowClient->getWorkflowHistory(

diff --git a/tests/Functional/worker.php b/tests/Functional/worker.php
@@ -56,7 +56,12 @@
 
 // register all workflows
 foreach ($getClasses(__DIR__ . '/../Fixtures/src/Workflow', 'Temporal\\Tests\\Workflow\\') as $class) {
-    if (class_exists($class) && !\interface_exists($class)) {
+    if (\class_exists($class) && !\interface_exists($class)) {
+        $wfRef = new \ReflectionClass($class);
+        if ($wfRef->isAbstract()) {
+            continue;
+        }
+
         \array_walk(
             $workers,
             static fn (WorkerInterface $worker) => $worker->registerWorkflowTypes($class),

diff --git a/tests/Unit/Declaration/Fixture/Inheritance/BaseWorkflowWithHandler.php b/tests/Unit/Declaration/Fixture/Inheritance/BaseWorkflowWithHandler.php
@@ -0,0 +1,23 @@
+<?php
+
+/**
+ * This file is part of Temporal package.
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+declare(strict_types=1);
+
+namespace Temporal\Tests\Unit\Declaration\Fixture\Inheritance;
+
+use Temporal\Workflow\WorkflowMethod;
+
+abstract class BaseWorkflowWithHandler
+{
+    /** @WorkflowMethod */
+    #[WorkflowMethod]
+    public function handler(): void
+    {
+    }
+}
diff --git a/tests/Unit/Declaration/Fixture/Inheritance/ExtendingWorkflow.php b/tests/Unit/Declaration/Fixture/Inheritance/ExtendingWorkflow.php
@@ -0,0 +1,20 @@
+<?php
+
+/**
+ * This file is part of Temporal package.
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+declare(strict_types=1);
+
+namespace Temporal\Tests\Unit\Declaration\Fixture\Inheritance;
+
+use Temporal\Workflow\WorkflowInterface;
+
+/** @WorkflowInterface */
+#[WorkflowInterface]
+class ExtendingWorkflow extends BaseWorkflowWithHandler
+{
+}