Do not send the stacktrace along with the exception when attach_stack…

…trace = true (#960)
getsentry · Jan 17, 2020 · f28c0e1 · f28c0e1
1 parent b148cf1
commit f28c0e1
Show file tree

Hide file tree

Showing 3 changed files with 58 additions and 20 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,6 +4,7 @@
 
 - Fix client creation with proxy (#951)
 - Allow unsetting the stack trace on an `Event` by calling `Event::setStacktrace(null)` (#961)
+- Fix sending of both `event.stacktrace` and `event.exceptions` when `attach_stacktrace = true` (#960)
 
 ## 2.3.0 (2020-01-08)
 

diff --git a/src/Client.php b/src/Client.php
@@ -85,13 +85,7 @@ public function captureMessage(string $message, ?Severity $level = null, ?Scope
             'level' => $level,
         ];
 
-        $event = $this->prepareEvent($payload, $scope, $this->options->shouldAttachStacktrace());
-
-        if (null === $event) {
-            return null;
-        }
-
-        return $this->transport->send($event);
+        return $this->captureEvent($payload, $scope);
     }
 
     /**
@@ -111,7 +105,7 @@ public function captureException(\Throwable $exception, ?Scope $scope = null): ?
      */
     public function captureEvent(array $payload, ?Scope $scope = null): ?string
     {
-        $event = $this->prepareEvent($payload, $scope, $this->options->shouldAttachStacktrace());
+        $event = $this->prepareEvent($payload, $scope);
 
         if (null === $event) {
             return null;
@@ -162,21 +156,20 @@ public function flush(?int $timeout = null): PromiseInterface
     /**
      * Assembles an event and prepares it to be sent of to Sentry.
      *
-     * @param array      $payload        the payload that will be converted to an Event
-     * @param Scope|null $scope          optional scope which enriches the Event
-     * @param bool       $withStacktrace True if the event should have and attached stacktrace
+     * @param array      $payload the payload that will be converted to an Event
+     * @param Scope|null $scope   optional scope which enriches the Event
      *
      * @return Event|null returns ready to send Event, however depending on options it can be discarded
      */
-    private function prepareEvent(array $payload, ?Scope $scope = null, bool $withStacktrace = false): ?Event
+    private function prepareEvent(array $payload, ?Scope $scope = null): ?Event
     {
         $sampleRate = $this->getOptions()->getSampleRate();
 
         if ($sampleRate < 1 && mt_rand(1, 100) / 100.0 > $sampleRate) {
             return null;
         }
 
-        if ($withStacktrace) {
+        if ($this->getOptions()->shouldAttachStacktrace() && !isset($payload['exception']) && !isset($payload['stacktrace'])) {
             $event = $this->eventFactory->createWithStacktrace($payload);
         } else {
             $event = $this->eventFactory->create($payload);

diff --git a/tests/ClientTest.php b/tests/ClientTest.php
@@ -140,7 +140,7 @@ public function testCaptureEvent(): void
     /**
      * @dataProvider captureEventAttachesStacktraceAccordingToAttachStacktraceOptionDataProvider
      */
-    public function testCaptureEventAttachesStacktraceAccordingToAttachStacktraceOption(bool $shouldAttachStacktrace): void
+    public function testCaptureEventAttachesStacktraceAccordingToAttachStacktraceOption(bool $attachStacktraceOption, array $payload, bool $shouldAttachStacktrace): void
     {
         /** @var TransportInterface&MockObject $transport */
         $transport = $this->createMock(TransportInterface::class);
@@ -159,19 +159,63 @@ public function testCaptureEventAttachesStacktraceAccordingToAttachStacktraceOpt
             }))
             ->willReturn('500a339f3ab2450b96dee542adf36ba7');
 
-        $client = ClientBuilder::create(['attach_stacktrace' => $shouldAttachStacktrace])
+        $client = ClientBuilder::create(['attach_stacktrace' => $attachStacktraceOption])
             ->setTransportFactory($this->createTransportFactory($transport))
             ->getClient();
 
-        $this->assertEquals('500a339f3ab2450b96dee542adf36ba7', $client->captureEvent([]));
+        $this->assertEquals('500a339f3ab2450b96dee542adf36ba7', $client->captureEvent($payload));
     }
 
-    public function captureEventAttachesStacktraceAccordingToAttachStacktraceOptionDataProvider(): array
+    public function captureEventAttachesStacktraceAccordingToAttachStacktraceOptionDataProvider(): \Generator
     {
-        return [
-            [true],
-            [false],
+        yield 'Stacktrace attached when attach_stacktrace = true and both payload.exception and payload.stacktrace are unset' => [
+            true,
+            [],
+            true,
+        ];
+
+        yield 'No stacktrace attached when attach_stacktrace = false' => [
+            false,
+            [],
+            false,
+        ];
+
+        yield 'No stacktrace attached when attach_stacktrace = true and payload.exception is set' => [
+            true,
+            [
+                'exception' => new \Exception(),
+            ],
+            false,
         ];
+
+        yield 'No stacktrace attached when attach_stacktrace = false and payload.exception is set' => [
+            true,
+            [
+                'exception' => new \Exception(),
+            ],
+            false,
+        ];
+    }
+
+    public function testCaptureEventPrefersExplicitStacktrace(): void
+    {
+        $explicitStacktrace = $this->createMock(Stacktrace::class);
+        $payload = ['stacktrace' => $explicitStacktrace];
+
+        /** @var TransportInterface&MockObject $transport */
+        $transport = $this->createMock(TransportInterface::class);
+        $transport->expects($this->once())
+            ->method('send')
+            ->with($this->callback(static function (Event $event) use ($explicitStacktrace): bool {
+                return $explicitStacktrace === $event->getStacktrace();
+            }))
+            ->willReturn('500a339f3ab2450b96dee542adf36ba7');
+
+        $client = ClientBuilder::create(['attach_stacktrace' => true])
+            ->setTransportFactory($this->createTransportFactory($transport))
+            ->getClient();
+
+        $this->assertEquals('500a339f3ab2450b96dee542adf36ba7', $client->captureEvent($payload));
     }
 
     public function testCaptureLastError(): void