Fix decoding Payloads in case when an external Temporal SDK returns e…

…mpty payload that doesn't contain even NULL value. (#442) Co-authored-by: Anton Titov <[email protected]>
temporalio · May 27, 2024 · e6d9588 · e6d9588
1 parent cae430c
commit e6d9588
Show file tree

Hide file tree

Showing 8 changed files with 146 additions and 51 deletions.
diff --git a/psalm-baseline.xml b/psalm-baseline.xml
@@ -244,9 +244,6 @@
     <MissingClosureParamType>
       <code><![CDATA[$value]]></code>
     </MissingClosureParamType>
-    <MoreSpecificImplementedParamType>
-      <code><![CDATA[$type]]></code>
-    </MoreSpecificImplementedParamType>
     <NullableReturnStatement>
       <code><![CDATA[$result]]></code>
       <code><![CDATA[$result]]></code>
@@ -1200,9 +1197,6 @@
       <code><![CDATA[$name]]></code>
       <code><![CDATA[$name]]></code>
     </ArgumentTypeCoercion>
-    <ImplicitToStringCast>
-      <code><![CDATA[$returnType]]></code>
-    </ImplicitToStringCast>
     <LessSpecificReturnStatement>
       <code><![CDATA[EncodedValues::decodePromise($this->request($request), $returnType)]]></code>
     </LessSpecificReturnStatement>
@@ -1319,9 +1313,6 @@
       <code><![CDATA[$type]]></code>
       <code><![CDATA[$type]]></code>
     </ArgumentTypeCoercion>
-    <ImplicitToStringCast>
-      <code><![CDATA[$returnType]]></code>
-    </ImplicitToStringCast>
     <InvalidArgument>
       <code><![CDATA[$options]]></code>
     </InvalidArgument>
@@ -1348,7 +1339,6 @@
       <code><![CDATA[$result]]></code>
     </MissingClosureParamType>
     <MissingClosureReturnType>
-      <code><![CDATA[fn() => EncodedValues::decodePromise(]]></code>
       <code><![CDATA[function ($result) use ($conditionGroupId) {]]></code>
     </MissingClosureReturnType>
     <MissingImmutableAnnotation>

diff --git a/src/DataConverter/EncodedValues.php b/src/DataConverter/EncodedValues.php
@@ -69,9 +69,6 @@ public static function fromPayloads(Payloads $payloads, DataConverterInterface $
         return static::fromPayloadCollection($payloads->getPayloads(), $dataConverter);
     }
 
-    /**
-     * @return Payloads
-     */
     public function toPayloads(): Payloads
     {
         return new Payloads(['payloads' => $this->toProtoCollection()]);
@@ -102,7 +99,7 @@ public static function sliceValues(
      * Decode promise response upon returning it to the domain layer.
      *
      * @param PromiseInterface $promise
-     * @param Type|string|null $type
+     * @param string|\ReflectionClass|\ReflectionType|Type|null $type
      *
      * @return PromiseInterface
      */
@@ -119,17 +116,18 @@ function ($value) use ($type) {
         );
     }
 
-    /**
-     * @param Type|string|null $type
-     *
-     * @return mixed
-     */
     public function getValue(int|string $index, $type = null): mixed
     {
         if (\is_array($this->values) && \array_key_exists($index, $this->values)) {
             return $this->values[$index];
         }
 
+        // External SDKs might return an empty array with metadata, alias to null
+        // Most likely this is a void type
+        if ($index === 0 && $this->count() === 0 && $this->isVoidType($type)) {
+            return null;
+        }
+
         if ($this->converter === null) {
             throw new \LogicException('DataConverter is not set');
         }
@@ -211,6 +209,16 @@ public function isEmpty(): bool
         return $this->count() === 0;
     }
 
+    private function isVoidType(mixed $type = null): bool
+    {
+        return match (true) {
+            $type === null => true,
+            $type instanceof Type => \in_array($type->getName(), [Type::TYPE_VOID, Type::TYPE_NULL], true),
+            $type instanceof \ReflectionNamedType => $type->getName() === Type::TYPE_VOID,
+            default => false,
+        };
+    }
+
     /**
      * Returns collection of {@see Payloads}.
      *

diff --git a/src/DataConverter/ValuesInterface.php b/src/DataConverter/ValuesInterface.php
@@ -35,8 +35,11 @@ public function setDataConverter(DataConverterInterface $converter);
     /**
      * Get value by it's index.
      *
+     * Returns {@see null} if there are no values and $type has null value
+     * like {@see null}, {@see Type::TYPE_VOID} or {@see Type::TYPE_NULL}.
+     *
      * @param int $index
-     * @param Type|TypeEnum|mixed $type
+     * @param string|\ReflectionClass|\ReflectionType|Type|null $type
      * @return mixed
      */
     public function getValue(int $index, $type);

diff --git a/src/Internal/Workflow/WorkflowContext.php b/src/Internal/Workflow/WorkflowContext.php
@@ -228,7 +228,7 @@ public function getVersion(string $changeId, int $minSupported, int $maxSupporte
     public function sideEffect(callable $context): PromiseInterface
     {
         $value = null;
-        $closure = \Closure::fromCallable($context);
+        $closure = $context(...);
 
         try {
             if (!$this->isReplaying()) {
@@ -249,7 +249,7 @@ public function sideEffect(callable $context): PromiseInterface
         } catch (\Throwable) {
         }
 
-        $last = fn() => EncodedValues::decodePromise(
+        $last = fn(): PromiseInterface => EncodedValues::decodePromise(
             $this->request(new SideEffect(EncodedValues::fromValues([$value]))),
             $returnType,
         );

diff --git a/tests/Fixtures/WorkerMock.php b/tests/Fixtures/WorkerMock.php
@@ -11,6 +11,9 @@
 
 namespace Temporal\Tests\Fixtures;
 
+use Temporal\Api\Failure\V1\Failure;
+use Temporal\DataConverter\DataConverter;
+use Temporal\Exception\Failure\FailureConverter;
 use Temporal\Tests\TestCase;
 use Temporal\Worker\Transport\HostConnectionInterface;
 use Temporal\Worker\WorkerFactoryInterface;
@@ -122,6 +125,24 @@ public function send(string $frame): void
             dump($frame);
         }
 
+        if ($pair[0] !== $frame) {
+            // Parse error if exists
+            $json = \json_decode($frame, true);
+            if (\is_array($json)) {
+                foreach ($json as $part) {
+                    if (isset($part['failure'])) {
+                        $failure = new Failure();
+                        try {
+                            $failure->mergeFromString(\base64_decode($part['failure']));
+                        } catch (\Throwable) {
+                            continue;
+                        }
+                        throw FailureConverter::mapFailureToException($failure, DataConverter::createDefault());
+                    }
+                }
+            }
+        }
+
         $this->testCase->assertEquals($pair[0], $frame);
 
         $this->indexIn++;

diff --git a/tests/Fixtures/src/Activity/SimpleActivity.php b/tests/Fixtures/src/Activity/SimpleActivity.php
@@ -11,6 +11,7 @@
 
 namespace Temporal\Tests\Activity;
 
+use React\Promise\PromiseInterface;
 use Temporal\Activity;
 use Temporal\Activity\ActivityInterface;
 use Temporal\Activity\ActivityMethod;
@@ -149,4 +150,12 @@ public function namedArguments(
             'optionalNullableString' => $optionalNullableString,
         ];
     }
+
+    /**
+     * @return PromiseInterface<null>
+     */
+    #[ActivityMethod]
+    public function empty(): void
+    {
+    }
 }
diff --git a/tests/Fixtures/src/Workflow/VoidActivityStubWorkflow.php b/tests/Fixtures/src/Workflow/VoidActivityStubWorkflow.php
@@ -0,0 +1,33 @@
+<?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;
+
+use Temporal\Activity\ActivityOptions;
+use Temporal\Tests\Activity\SimpleActivity;
+use Temporal\Workflow;
+use Temporal\Workflow\WorkflowMethod;
+
+#[Workflow\WorkflowInterface]
+class VoidActivityStubWorkflow
+{
+    #[WorkflowMethod(name: 'VoidActivityStubWorkflow')]
+    public function handler()
+    {
+        // typed stub
+        $simple = Workflow::newActivityStub(
+            SimpleActivity::class,
+            ActivityOptions::new()->withStartToCloseTimeout(5)
+        );
+
+        return yield $simple->empty();
+    }
+}