Merge pull request #583 from bugsnag/session-data-counter-fix

Fix SessionData counters across multiple errors

CHANGELOG.md

@@ -5,7 +5,7 @@ Changelog
 
 ### Enhancements
 
-- The Client and SessionTracker now share a single Guzzle instance (#582)
+- The `Client` and `SessionTracker` now share a single Guzzle instance (#582)
 
 ### BC breaks
 
@@ -36,6 +36,8 @@ Changelog
 - `Client::makeGuzzle` has been made private (#582)
   Use the `$guzzle` parameter of `Bugsnag\Client::__construct` instead
 
+- The `SessionData` class now takes `SessionTracker` instead of `Client` in its constructor
+
 ## 3.21.0 (2020-04-29)
 
 ### Enhancements

src/Client.php

@@ -128,7 +128,7 @@ public function __construct(
 
         $this->registerMiddleware(new NotificationSkipper($config));
         $this->registerMiddleware(new BreadcrumbData($this->recorder));
-        $this->registerMiddleware(new SessionData($this));
+        $this->registerMiddleware(new SessionData($this->sessionTracker));
 
         // Shutdown strategy is used to trigger flush() calls when batch sending is enabled
         $shutdownStrategy = $shutdownStrategy ?: new PhpShutdownStrategy();

src/Middleware/SessionData.php

@@ -2,48 +2,50 @@
 
 namespace Bugsnag\Middleware;
 
-use Bugsnag\Client;
 use Bugsnag\Report;
+use Bugsnag\SessionTracker;
 
 class SessionData
 {
     /**
-     * The client instance.
-     *
-     * @var \Bugsnag\Client
+     * @var SessionTracker
      */
-    protected $client;
+    protected $sessionTracker;
 
     /**
-     * Create a new session data middleware instance.
-     *
-     * @param \Bugsnag\Client $client the client instance.
-     *
-     * @return void
+     * @param SessionTracker $sessionTracker
      */
-    public function __construct(Client $client)
+    public function __construct(SessionTracker $sessionTracker)
     {
-        $this->client = $client;
+        $this->sessionTracker = $sessionTracker;
     }
 
     /**
-     * Execute the session data middleware.
+     * Attaches session information to the Report, if the SessionTracker has a
+     * current session. Note that this is not the same as the PHP session, but
+     * refers to the current request.
      *
-     * @param \Bugsnag\Report $report the bugsnag report instance
-     * @param callable        $next   the next stage callback
+     * If the SessionTracker does not have a current session, the report will
+     * not be changed.
+     *
+     * @param Report   $report
+     * @param callable $next
      *
      * @return void
      */
     public function __invoke(Report $report, callable $next)
     {
-        $session = $this->client->getSessionTracker()->getCurrentSession();
-        if (!is_null($session) && isset($session['events'])) {
+        $session = $this->sessionTracker->getCurrentSession();
+
+        if (isset($session['events'])) {
             if ($report->getUnhandled()) {
                 $session['events']['unhandled'] += 1;
             } else {
                 $session['events']['handled'] += 1;
             }
+
             $report->setSessionData($session);
+            $this->sessionTracker->setCurrentSession($session);
         }
 
         $next($report);

src/SessionTracker.php

@@ -91,7 +91,7 @@ class SessionTracker
      *
      * @var array
      */
-    protected $currentSession;
+    protected $currentSession = [];
 
     /**
      * @param Configuration $config
@@ -130,7 +130,7 @@ public function startSession()
      *
      * @return void
      */
-    protected function setCurrentSession(array $session)
+    public function setCurrentSession(array $session)
     {
         if (is_callable($this->sessionFunction)) {
             call_user_func($this->sessionFunction, $session);

tests/Middleware/SessionDataTest.php

@@ -2,84 +2,172 @@
 
 namespace Bugsnag\Tests\Middleware;
 
-use Bugsnag\Client;
+use Bugsnag\Configuration;
+use Bugsnag\HttpClient;
 use Bugsnag\Middleware\SessionData;
 use Bugsnag\Report;
 use Bugsnag\SessionTracker;
 use Bugsnag\Tests\TestCase;
-use Mockery;
+use Exception;
 
 class SessionDataTest extends TestCase
 {
-    public function testUnhandledError()
+    /**
+     * @var SessionTracker
+     */
+    private $sessionTracker;
+
+    /**
+     * @var Report
+     */
+    private $report;
+
+    protected function setUp()
+    {
+        $config = new Configuration('api-key');
+        $httpClient = $this->getMockBuilder(HttpClient::class)
+            ->disableOriginalConstructor()
+            ->getMock();
+
+        $this->sessionTracker = new SessionTracker($config, $httpClient);
+        $this->report = Report::fromPHPThrowable($config, new Exception('no'));
+    }
+
+    public function testItSetsTheUnhandledCountWhenAnUnhandledErrorOccurs()
     {
-        $sessionTracker = Mockery::mock(SessionTracker::class);
-        $sessionTracker->shouldReceive('getCurrentSession')->andReturn([
-            'events' => [
-                'unhandled' => 0,
-                'handled' => 0,
-            ],
-        ]);
-
-        $client = Mockery::mock(Client::class);
-        $client->shouldReceive('getSessionTracker')->andReturn($sessionTracker);
-
-        $report = Mockery::mock(Report::class);
-        $report->shouldReceive('getUnhandled')->andReturn(true);
-        $report->shouldReceive('setSessionData')->with([
-            'events' => [
-                'unhandled' => 1,
-                'handled' => 0,
-            ],
-        ]);
-
-        $middleware = new SessionData($client);
-        $middleware($report, function ($var) use ($report) {
-            $this->assertSame($var, $report);
+        $this->sessionTracker->startSession();
+        $this->report->setUnhandled(true);
+
+        $middleware = new SessionData($this->sessionTracker);
+
+        $middleware($this->report, function (Report $report) {
+            $this->assertReportHasUnhandledErrors($report, 1);
         });
     }
 
-    public function testHandledError()
+    public function testItIncrementsTheUnhandledCountWhenMultipleUnhandledErrorsOccur()
     {
-        $sessionTracker = Mockery::mock(SessionTracker::class);
-        $sessionTracker->shouldReceive('getCurrentSession')->andReturn([
-            'events' => [
-                'unhandled' => 0,
-                'handled' => 0,
-            ],
-        ]);
-
-        $client = Mockery::mock(Client::class);
-        $client->shouldReceive('getSessionTracker')->andReturn($sessionTracker);
-
-        $report = Mockery::mock(Report::class);
-        $report->shouldReceive('getUnhandled')->andReturn(false);
-        $report->shouldReceive('setSessionData')->with([
-            'events' => [
-                'unhandled' => 0,
-                'handled' => 1,
-            ],
-        ]);
-
-        $middleware = new SessionData($client);
-        $middleware($report, function ($var) use ($report) {
-            $this->assertSame($var, $report);
+        $this->sessionTracker->startSession();
+        $this->report->setUnhandled(true);
+
+        $middleware = new SessionData($this->sessionTracker);
+
+        foreach (range(1, 5) as $errorCount) {
+            $middleware($this->report, function (Report $report) use ($errorCount) {
+                $this->assertReportHasUnhandledErrors($report, $errorCount);
+            });
+        }
+    }
+
+    public function testItSetsTheHandledCountWhenAHandledErrorOccurs()
+    {
+        $this->sessionTracker->startSession();
+        $this->report->setUnhandled(false);
+
+        $middleware = new SessionData($this->sessionTracker);
+
+        $middleware($this->report, function (Report $report) {
+            $this->assertReportHasHandledErrors($report, 1);
         });
     }
 
-    public function testNullSession()
+    public function testItIncrementsTheHandledCountWhenMultipleHandledErrorsOccur()
     {
-        $sessionTracker = Mockery::mock(SessionTracker::class);
-        $sessionTracker->shouldReceive('getCurrentSession')->andReturn(null);
+        $this->sessionTracker->startSession();
+        $this->report->setUnhandled(false);
 
-        $client = Mockery::mock(Client::class);
-        $client->shouldReceive('getSessionTracker')->andReturn($sessionTracker);
+        $middleware = new SessionData($this->sessionTracker);
+
+        foreach (range(1, 5) as $errorCount) {
+            $middleware($this->report, function (Report $report) use ($errorCount) {
+                $this->assertReportHasHandledErrors($report, $errorCount);
+            });
+        }
+    }
+
+    public function testItDoesNothingWhenForAnUnhandledErrorWhenThereIsNoSessionData()
+    {
+        // We don't call 'startSession' here so there is no session data to
+        // capture in the middleware
+        $this->report->setUnhandled(true);
 
-        $report = Mockery::mock(Report::class);
+        $middleware = new SessionData($this->sessionTracker);
 
-        $middleware = new SessionData($client);
-        $middleware($report, function ($var) use ($report) {
-            $this->assertSame($var, $report);
+        $middleware($this->report, function (Report $report) {
+            $reportArray = $report->toArray();
+            $this->assertArrayNotHasKey('session', $reportArray);
         });
     }
+
+    public function testItDoesNothingWhenForAHandledErrorWhenThereIsNoSessionData()
+    {
+        // We don't call 'startSession' here so there is no session data to
+        // capture in the middleware
+        $this->report->setUnhandled(false);
+
+        $middleware = new SessionData($this->sessionTracker);
+
+        $middleware($this->report, function (Report $report) {
+            $reportArray = $report->toArray();
+            $this->assertArrayNotHasKey('session', $reportArray);
+        });
+    }
+
+    /**
+     * Assert the given report has the expected number of unhandled errors.
+     *
+     * @param Report $report
+     * @param int    $expectedCount
+     *
+     * @return void
+     */
+    private function assertReportHasUnhandledErrors(Report $report, $expectedCount)
+    {
+        $this->assertReportHasErrors($report, 'unhandled', $expectedCount);
+    }
+
+    /**
+     * Assert the given report has the expected number of handled errors.
+     *
+     * @param Report $report
+     * @param int    $expectedCount
+     *
+     * @return void
+     */
+    private function assertReportHasHandledErrors(Report $report, $expectedCount)
+    {
+        $this->assertReportHasErrors($report, 'handled', $expectedCount);
+    }
+
+    /**
+     * Assert the given report has the expected number of errors of type '$type'.
+     *
+     * @psalm-param 'handled'|'unhandled' $type
+     *
+     * @param Report $report
+     * @param string $type          one of 'handled' or 'unhandled'
+     * @param int    $expectedCount
+     *
+     * @return void
+     */
+    private function assertReportHasErrors(Report $report, $type, $expectedCount)
+    {
+        $reportArray = $report->toArray();
+        $this->assertArrayHasKey('session', $reportArray);
+
+        $session = $reportArray['session'];
+        $this->assertArrayHasKey('events', $session);
+
+        $events = $session['events'];
+        $this->assertArrayHasKey('handled', $events);
+        $this->assertArrayHasKey('unhandled', $events);
+
+        if ($type === 'handled') {
+            $this->assertSame($expectedCount, $events['handled']);
+            $this->assertSame(0, $events['unhandled']);
+        } else {
+            $this->assertSame(0, $events['handled']);
+            $this->assertSame($expectedCount, $events['unhandled']);
+        }
+    }
 }