Merge pull request #421 from bowphp/refactor/code-base

fix(session): fix database adaptor

Author: papac

src/Session/Adapters/DatabaseAdapter.php

@@ -20,13 +20,6 @@ class DatabaseAdapter implements SessionHandlerInterface
      */
     private string $table;
 
-    /**
-     * The current session session_id
-     *
-     * @var string
-     */
-    private string $session_id;
-
     /**
      * The current user ip
      *
@@ -91,11 +84,11 @@ private function sessions(): QueryBuilder
      */
     public function gc(int $max_lifetime): int|false
     {
-        $this->sessions()
-            ->where('time', '<', $this->createTimestamp())
+        // The `time` column stores each session's expiry timestamp, so a
+        // session is collectable once that expiry is in the past.
+        return $this->sessions()
+            ->where('time', '<', date('Y-m-d H:i:s'))
             ->delete();
-
-        return 1;
     }
 
     /**
@@ -117,10 +110,14 @@ public function open(string $path, string $name): bool
      * @return string
      * @throws QueryBuilderException
      */
-    public function read(string $id): string
+    public function read(string $session_id): string
     {
+        // Only return live sessions: an expired row (expiry in the past) must
+        // be treated as absent, otherwise stale sessions stay usable until gc.
         $session = $this->sessions()
-            ->where('id', $id)->first();
+            ->where('id', $session_id)
+            ->where('time', '>=', date('Y-m-d H:i:s'))
+            ->first();
 
         if (is_null($session)) {
             return '';
@@ -147,11 +144,12 @@ public function write(string $id, string $data): bool
             return (bool)$insert;
         }
 
-        // Update the session information
+        // Update the session payload and slide the expiry forward so an active
+        // session does not expire a fixed window after its first request.
         $update = $this->sessions()->where('id', $id)->update(
             [
             'data' => $data,
-            'id' => $id
+            'time' => $this->createTimestamp()
             ]
         );
 

tests/Session/DatabaseAdapterTest.php

@@ -0,0 +1,106 @@
+<?php
+
+namespace Bow\Tests\Session;
+
+use Bow\Database\Database;
+use Bow\Session\Adapters\DatabaseAdapter;
+use Bow\Tests\Config\TestingConfiguration;
+use PHPUnit\Framework\TestCase;
+
+class DatabaseAdapterTest extends TestCase
+{
+    private DatabaseAdapter $adapter;
+
+    public static function setUpBeforeClass(): void
+    {
+        $config = TestingConfiguration::getConfig();
+
+        Database::configure($config["database"]);
+        Database::connection('sqlite');
+        Database::statement("DROP TABLE IF EXISTS sessions;");
+        Database::statement("
+            CREATE TABLE sessions (
+                id varchar(200) not null primary key,
+                time datetime null,
+                data text null,
+                ip varchar(255) null
+            )");
+    }
+
+    protected function setUp(): void
+    {
+        Database::connection('sqlite');
+        Database::statement("DELETE FROM sessions;");
+
+        $this->adapter = new DatabaseAdapter(['table' => 'sessions'], '127.0.0.1');
+    }
+
+    /**
+     * Seed a row directly with an explicit expiry offset so each test controls
+     * exactly whether a session is expired or still active.
+     */
+    private function seed(string $id, string $data, int $expiresInSeconds): void
+    {
+        Database::table('sessions')->insert([
+            'id' => $id,
+            'time' => date('Y-m-d H:i:s', time() + $expiresInSeconds),
+            'data' => $data,
+            'ip' => '127.0.0.1',
+        ]);
+    }
+
+    private function exists(string $id): bool
+    {
+        return Database::table('sessions')->where('id', $id)->exists();
+    }
+
+    /** Bug #1 — gc() must drop expired rows but keep still-valid ones. */
+    public function test_gc_removes_only_expired_sessions(): void
+    {
+        $this->seed('expired-session', 'old', -60);   // expired a minute ago
+        $this->seed('active-session', 'fresh', 3600); // valid for another hour
+
+        $this->adapter->gc(0);
+
+        $this->assertFalse($this->exists('expired-session'), 'Expired session should be collected');
+        $this->assertTrue($this->exists('active-session'), 'Active session must survive gc');
+    }
+
+    /** Bug #3 — read() must not return data for an expired session. */
+    public function test_read_returns_empty_for_expired_session(): void
+    {
+        $this->seed('stale', 'secret-payload', -60);
+
+        $this->assertSame('', $this->adapter->read('stale'));
+    }
+
+    /** read() still returns the payload for a live session. */
+    public function test_read_returns_data_for_active_session(): void
+    {
+        $this->seed('live', 'hello world', 3600);
+
+        $this->assertSame('hello world', $this->adapter->read('live'));
+    }
+
+    /** Bug #2 — write() on an existing session must refresh its expiry (sliding window). */
+    public function test_write_refreshes_expiry_on_update(): void
+    {
+        $this->adapter->write('rolling', 'v1');
+
+        // Simulate time passing: push the stored expiry into the past.
+        Database::table('sessions')
+            ->where('id', 'rolling')
+            ->update(['time' => date('Y-m-d H:i:s', time() - 60)]);
+
+        $this->adapter->write('rolling', 'v2');
+
+        $row = Database::table('sessions')->where('id', 'rolling')->first();
+
+        $this->assertSame('v2', $row->data, 'Payload should be updated');
+        $this->assertGreaterThan(
+            date('Y-m-d H:i:s'),
+            $row->time,
+            'Expiry must be pushed back into the future on each write'
+        );
+    }
+}