Merge pull request #406: Fix FailureConverter when an exception CallS…

…tack contains an incomplete set of keys
temporalio · Mar 11, 2024 · d230c85 · d230c85
2 parents 41c9c3b + ec4c8fd
commit d230c85
Show file tree

Hide file tree

Showing 3 changed files with 91 additions and 29 deletions.
diff --git a/psalm-baseline.xml b/psalm-baseline.xml
@@ -395,9 +395,6 @@
     <InvalidOperand>
       <code><![CDATA[count($arg)]]></code>
     </InvalidOperand>
-    <MissingClosureReturnType>
-      <code><![CDATA[static fn() => \sprintf(]]></code>
-    </MissingClosureReturnType>
     <NullableReturnStatement>
       <code><![CDATA[$e->getFailure()]]></code>
     </NullableReturnStatement>
@@ -429,13 +426,6 @@
       <code><![CDATA[hasLastHeartbeatDetails]]></code>
       <code><![CDATA[setStackTrace]]></code>
     </PossiblyNullReference>
-    <RedundantCastGivenDocblockType>
-      <code><![CDATA[(string)$frame['class']]]></code>
-    </RedundantCastGivenDocblockType>
-    <RiskyTruthyFalsyComparison>
-      <code><![CDATA[empty($args)]]></code>
-      <code><![CDATA[empty($frame['line'])]]></code>
-    </RiskyTruthyFalsyComparison>
   </file>
   <file src="src/Exception/Failure/TemporalFailure.php">
     <MissingClosureParamType>

diff --git a/src/Exception/Failure/FailureConverter.php b/src/Exception/Failure/FailureConverter.php
@@ -286,13 +286,13 @@ private static function createFailureException(Failure $failure, DataConverterIn
     private static function generateStackTraceString(\Throwable $e, bool $skipInternal = true): string
     {
         /** @var list<array{
-         *     function: string,
-         *     line: int<0, max>|null,
-         *     file: non-empty-string|null,
-         *     class: class-string,
-         *     object?: object,
-         *     type: string,
-         *     args: array|null
+         *     function?: non-empty-string|null,
+         *     line?: int<0, max>|null,
+         *     file?: non-empty-string|null,
+         *     class?: class-string|null,
+         *     object?: object|null,
+         *     type?: string|null,
+         *     args?: array|null
          * }> $frames
          */
         $frames = $e->getTrace();
@@ -308,20 +308,19 @@ private static function generateStackTraceString(\Throwable $e, bool $skipIntern
                 continue;
             }
 
-            $renderer = static fn() => \sprintf(
+            $renderer = static fn(): string => \sprintf(
                 "%s%s%s\n%s%s%s%s(%s)",
                 \str_pad("#$i", $numPad, ' '),
                 $frame['file'] ?? '[internal function]',
-                empty($frame['line']) ? '' : ":{$frame['line']}",
+                isset($frame['line']) ? ":{$frame['line']}" : '',
                 \str_repeat(' ', $numPad),
-                $frame['class'],
-                $frame['type'],
-                $frame['function'],
-                self::renderTraceAttributes($frame['args']),
+                $frame['class'] ?? '',
+                $frame['type'] ?? '',
+                $frame['function'] ?? '',
+                self::renderTraceAttributes($frame['args'] ?? []),
             );
 
-
-            if ($skipInternal && \str_starts_with((string)$frame['class'], 'Temporal\\')) {
+            if ($skipInternal && \str_starts_with($frame['class'] ?? '', 'Temporal\\')) {
                 if (!$isFirst) {
                     $internals[] = $renderer;
                     $isFirst = false;
@@ -337,12 +336,11 @@ private static function generateStackTraceString(\Throwable $e, bool $skipIntern
                     '[%d hidden internal calls]',
                     \count($internals),
                 );
-                $internals = [];
             } else {
                 $result = [...$result, ...\array_map(static fn(callable $renderer) => $renderer(), $internals)];
-                $internals = [];
             }
 
+            $internals = [];
             $result[] = $renderer();
         }
 
@@ -353,9 +351,9 @@ private static function generateStackTraceString(\Throwable $e, bool $skipIntern
         return \implode("\n", $result);
     }
 
-    private static function renderTraceAttributes(?array $args): string
+    private static function renderTraceAttributes(array $args): string
     {
-        if (empty($args)) {
+        if ($args === []) {
             return '';
         }
 

diff --git a/tests/Unit/Exception/FailureConverterTestCase.php b/tests/Unit/Exception/FailureConverterTestCase.php
@@ -4,6 +4,7 @@
 
 namespace Temporal\Tests\Unit\Exception;
 
+use Exception;
 use Temporal\DataConverter\DataConverter;
 use Temporal\DataConverter\EncodedValues;
 use Temporal\Exception\Failure\ApplicationFailure;
@@ -30,4 +31,77 @@ public function testApplicationFailureCanTransferData(): void
         $this->assertSame('abc', $restoredDetails->getValue(0));
         $this->assertSame(123, $restoredDetails->getValue(1));
     }
+
+    public function testStackTraceStringForAdditionalContext(): void
+    {
+        $trace = FailureConverter::mapExceptionToFailure(
+            new Exception(),
+            DataConverter::createDefault(),
+        )->getStackTrace();
+
+        self::assertStringContainsString(
+            'Temporal\Tests\Unit\Exception\FailureConverterTestCase->testStackTraceStringForAdditionalContext()',
+            $trace,
+        );
+
+        self::assertStringContainsString(
+            'PHPUnit\Framework\TestCase->runTest()',
+            $trace,
+        );
+    }
+
+    public function testStackTraceStringForAdditionalContextEvenWhenClassIsNotPresented(): void
+    {
+        $previous = \ini_get('zend.exception_ignore_args');
+        \ini_set('zend.exception_ignore_args', 'Off');
+
+        try {
+            $trace = FailureConverter::mapExceptionToFailure(
+                call_user_func(fn () => new Exception()),
+                DataConverter::createDefault(),
+            )->getStackTrace();
+        } finally {
+            \ini_set('zend.exception_ignore_args', $previous);
+        }
+
+        self::assertStringContainsString(
+            '[internal function]',
+            $trace,
+        );
+
+        self::assertStringContainsString(
+            'Temporal\Tests\Unit\Exception\FailureConverterTestCase->Temporal\Tests\Unit\Exception\{closure}()',
+            $trace,
+        );
+
+        self::assertStringContainsString(
+            'call_user_func(Closure)',
+            $trace,
+        );
+
+        self::assertStringContainsString(
+            'PHPUnit\Framework\TestCase->runTest()',
+            $trace,
+        );
+    }
+
+    public function testStackTraceStringWithoutExceptionArgs(): void
+    {
+        $previous = \ini_get('zend.exception_ignore_args');
+        \ini_set('zend.exception_ignore_args', 'On');
+
+        try {
+            $trace = FailureConverter::mapExceptionToFailure(
+                call_user_func(static fn() => new Exception()),
+                DataConverter::createDefault(),
+            )->getStackTrace();
+        } finally {
+            \ini_set('zend.exception_ignore_args', $previous);
+        }
+
+        self::assertStringContainsString(
+            'call_user_func()',
+            $trace,
+        );
+    }
 }