Fix SessionData counters across multiple errors

The SessionData middleware is intended to keep counters for the
number of handled and unhandled errors that occured in the current
request. However it was not actually incrementing the counts, so
the would always be reported as '1' or '0'

This change ensures that we tell the SessionTracker about the
updated data, so the counters are correct

There is a small BC break here in that the constructor arguments
for SessionData have changed from Client to SessionTracker, but this
is constructed internally so shouldn't have a major impact

src/Client.php

@@ -133,7 +133,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

@@ -96,7 +96,7 @@ class SessionTracker
      *
      * @var array
      */
-    protected $currentSession;
+    protected $currentSession = [];
 
     /**
      * @param Configuration $config
@@ -135,7 +135,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']);
+        }
+    }
 }