bug #52459 [Cache][HttpFoundation][Lock] Fix PDO store not creating t…

…able + add tests (HypeMC) This PR was merged into the 5.4 branch. Discussion ---------- [Cache][HttpFoundation][Lock] Fix PDO store not creating table + add tests | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | - | License | MIT Unlike e.g. SQLite, PostgreSQL doesn't throw an exception on `$conn->prepare()` if the table is missing, it instead throws it on `$stmt->execute()`, so the table never gets created. The table used to get created, but the behavior was broken with #43332. Commits ------- cb5d832 [Cache][Lock] Fix PDO store not creating table + add tests
symfony · Nov 20, 2023 · 48be4b3 · 48be4b3
2 parents b5376c7 + cb5d832
commit 48be4b3
Show file tree

Hide file tree

Showing 9 changed files with 177 additions and 42 deletions.
diff --git a/src/Symfony/Component/Cache/Adapter/DoctrineDbalAdapter.php b/src/Symfony/Component/Cache/Adapter/DoctrineDbalAdapter.php
@@ -420,7 +420,8 @@ private function getServerVersion(): string
             return $this->serverVersion;
         }
 
-        $conn = $this->conn->getWrappedConnection();
+        // The condition should be removed once support for DBAL <3.3 is dropped
+        $conn = method_exists($this->conn, 'getNativeConnection') ? $this->conn->getNativeConnection() : $this->conn->getWrappedConnection();
         if ($conn instanceof ServerInfoAwareConnection) {
             return $this->serverVersion = $conn->getServerVersion();
         }

diff --git a/src/Symfony/Component/Cache/Adapter/PdoAdapter.php b/src/Symfony/Component/Cache/Adapter/PdoAdapter.php
@@ -507,7 +507,7 @@ protected function doSave(array $values, int $lifetime)
         try {
             $stmt = $conn->prepare($sql);
         } catch (\PDOException $e) {
-            if (!$conn->inTransaction() || \in_array($this->driver, ['pgsql', 'sqlite', 'sqlsrv'], true)) {
+            if ($this->isTableMissing($e) && (!$conn->inTransaction() || \in_array($this->driver, ['pgsql', 'sqlite', 'sqlsrv'], true))) {
                 $this->createTable();
             }
             $stmt = $conn->prepare($sql);
@@ -542,7 +542,7 @@ protected function doSave(array $values, int $lifetime)
             try {
                 $stmt->execute();
             } catch (\PDOException $e) {
-                if (!$conn->inTransaction() || \in_array($this->driver, ['pgsql', 'sqlite', 'sqlsrv'], true)) {
+                if ($this->isTableMissing($e) && (!$conn->inTransaction() || \in_array($this->driver, ['pgsql', 'sqlite', 'sqlsrv'], true))) {
                     $this->createTable();
                 }
                 $stmt->execute();
@@ -596,4 +596,21 @@ private function getServerVersion(): string
 
         return $this->serverVersion;
     }
+
+    private function isTableMissing(\PDOException $exception): bool
+    {
+        $driver = $this->driver;
+        $code = $exception->getCode();
+
+        switch (true) {
+            case 'pgsql' === $driver && '42P01' === $code:
+            case 'sqlite' === $driver && str_contains($exception->getMessage(), 'no such table:'):
+            case 'oci' === $driver && 942 === $code:
+            case 'sqlsrv' === $driver && 208 === $code:
+            case 'mysql' === $driver && 1146 === $code:
+                return true;
+            default:
+                return false;
+        }
+    }
 }
diff --git a/src/Symfony/Component/Cache/Tests/Adapter/DoctrineDbalAdapterTest.php b/src/Symfony/Component/Cache/Tests/Adapter/DoctrineDbalAdapterTest.php
@@ -18,12 +18,13 @@
 use Doctrine\DBAL\DriverManager;
 use Doctrine\DBAL\Schema\DefaultSchemaManagerFactory;
 use Doctrine\DBAL\Schema\Schema;
-use PHPUnit\Framework\SkippedTestSuiteError;
 use Psr\Cache\CacheItemPoolInterface;
 use Symfony\Component\Cache\Adapter\DoctrineDbalAdapter;
 use Symfony\Component\Cache\Tests\Fixtures\DriverWrapper;
 
 /**
+ * @requires extension pdo_sqlite
+ *
  * @group time-sensitive
  */
 class DoctrineDbalAdapterTest extends AdapterTestCase
@@ -32,10 +33,6 @@ class DoctrineDbalAdapterTest extends AdapterTestCase
 
     public static function setUpBeforeClass(): void
     {
-        if (!\extension_loaded('pdo_sqlite')) {
-            throw new SkippedTestSuiteError('Extension pdo_sqlite required.');
-        }
-
         self::$dbFile = tempnam(sys_get_temp_dir(), 'sf_sqlite_cache');
     }
 
@@ -107,13 +104,12 @@ public function testConfigureSchemaTableExists()
     }
 
     /**
-     * @dataProvider provideDsn
+     * @dataProvider provideDsnWithSQLite
      */
-    public function testDsn(string $dsn, string $file = null)
+    public function testDsnWithSQLite(string $dsn, string $file = null)
     {
         try {
             $pool = new DoctrineDbalAdapter($dsn);
-            $pool->createTable();
 
             $item = $pool->getItem('key');
             $item->set('value');
@@ -125,12 +121,35 @@ public function testDsn(string $dsn, string $file = null)
         }
     }
 
-    public static function provideDsn()
+    public static function provideDsnWithSQLite()
     {
         $dbFile = tempnam(sys_get_temp_dir(), 'sf_sqlite_cache');
-        yield ['sqlite://localhost/'.$dbFile.'1', $dbFile.'1'];
-        yield ['sqlite3:///'.$dbFile.'3', $dbFile.'3'];
-        yield ['sqlite://localhost/:memory:'];
+        yield 'SQLite file' => ['sqlite://localhost/'.$dbFile.'1', $dbFile.'1'];
+        yield 'SQLite3 file' => ['sqlite3:///'.$dbFile.'3', $dbFile.'3'];
+        yield 'SQLite in memory' => ['sqlite://localhost/:memory:'];
+    }
+
+    /**
+     * @requires extension pdo_pgsql
+     *
+     * @group integration
+     */
+    public function testDsnWithPostgreSQL()
+    {
+        if (!$host = getenv('POSTGRES_HOST')) {
+            $this->markTestSkipped('Missing POSTGRES_HOST env variable');
+        }
+
+        try {
+            $pool = new DoctrineDbalAdapter('pgsql://postgres:password@'.$host);
+
+            $item = $pool->getItem('key');
+            $item->set('value');
+            $this->assertTrue($pool->save($item));
+        } finally {
+            $pdo = new \PDO('pgsql:host='.$host.';user=postgres;password=password');
+            $pdo->exec('DROP TABLE IF EXISTS cache_items');
+        }
     }
 
     protected function isPruned(DoctrineDbalAdapter $cache, string $name): bool

diff --git a/src/Symfony/Component/Cache/Tests/Adapter/PdoAdapterTest.php b/src/Symfony/Component/Cache/Tests/Adapter/PdoAdapterTest.php
@@ -11,11 +11,12 @@
 
 namespace Symfony\Component\Cache\Tests\Adapter;
 
-use PHPUnit\Framework\SkippedTestSuiteError;
 use Psr\Cache\CacheItemPoolInterface;
 use Symfony\Component\Cache\Adapter\PdoAdapter;
 
 /**
+ * @requires extension pdo_sqlite
+ *
  * @group time-sensitive
  */
 class PdoAdapterTest extends AdapterTestCase
@@ -24,10 +25,6 @@ class PdoAdapterTest extends AdapterTestCase
 
     public static function setUpBeforeClass(): void
     {
-        if (!\extension_loaded('pdo_sqlite')) {
-            throw new SkippedTestSuiteError('Extension pdo_sqlite required.');
-        }
-
         self::$dbFile = tempnam(sys_get_temp_dir(), 'sf_sqlite_cache');
 
         $pool = new PdoAdapter('sqlite:'.self::$dbFile);
@@ -71,13 +68,12 @@ public function testCleanupExpiredItems()
     }
 
     /**
-     * @dataProvider provideDsn
+     * @dataProvider provideDsnSQLite
      */
-    public function testDsn(string $dsn, string $file = null)
+    public function testDsnWithSQLite(string $dsn, string $file = null)
     {
         try {
             $pool = new PdoAdapter($dsn);
-            $pool->createTable();
 
             $item = $pool->getItem('key');
             $item->set('value');
@@ -89,11 +85,36 @@ public function testDsn(string $dsn, string $file = null)
         }
     }
 
-    public static function provideDsn()
+    public static function provideDsnSQLite()
     {
         $dbFile = tempnam(sys_get_temp_dir(), 'sf_sqlite_cache');
-        yield ['sqlite:'.$dbFile.'2', $dbFile.'2'];
-        yield ['sqlite::memory:'];
+        yield 'SQLite file' => ['sqlite:'.$dbFile.'2', $dbFile.'2'];
+        yield 'SQLite in memory' => ['sqlite::memory:'];
+    }
+
+    /**
+     * @requires extension pdo_pgsql
+     *
+     * @group integration
+     */
+    public function testDsnWithPostgreSQL()
+    {
+        if (!$host = getenv('POSTGRES_HOST')) {
+            $this->markTestSkipped('Missing POSTGRES_HOST env variable');
+        }
+
+        $dsn = 'pgsql:host='.$host.';user=postgres;password=password';
+
+        try {
+            $pool = new PdoAdapter($dsn);
+
+            $item = $pool->getItem('key');
+            $item->set('value');
+            $this->assertTrue($pool->save($item));
+        } finally {
+            $pdo = new \PDO($dsn);
+            $pdo->exec('DROP TABLE IF EXISTS cache_items');
+        }
     }
 
     protected function isPruned(PdoAdapter $cache, string $name): bool

diff --git a/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/SessionHandlerFactory.php b/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/SessionHandlerFactory.php
@@ -82,6 +82,7 @@ public static function createHandler($connection): AbstractSessionHandler
                 }
 
                 $connection = DriverManager::getConnection($params, $config);
+                // The condition should be removed once support for DBAL <3.3 is dropped
                 $connection = method_exists($connection, 'getNativeConnection') ? $connection->getNativeConnection() : $connection->getWrappedConnection();
                 // no break;
 

diff --git a/src/Symfony/Component/Lock/Store/PdoStore.php b/src/Symfony/Component/Lock/Store/PdoStore.php
@@ -115,7 +115,7 @@ public function save(Key $key)
         try {
             $stmt = $conn->prepare($sql);
         } catch (\PDOException $e) {
-            if (!$conn->inTransaction() || \in_array($this->driver, ['pgsql', 'sqlite', 'sqlsrv'], true)) {
+            if ($this->isTableMissing($e) && (!$conn->inTransaction() || \in_array($this->driver, ['pgsql', 'sqlite', 'sqlsrv'], true))) {
                 $this->createTable();
             }
             $stmt = $conn->prepare($sql);
@@ -127,8 +127,18 @@ public function save(Key $key)
         try {
             $stmt->execute();
         } catch (\PDOException $e) {
-            // the lock is already acquired. It could be us. Let's try to put off.
-            $this->putOffExpiration($key, $this->initialTtl);
+            if ($this->isTableMissing($e) && (!$conn->inTransaction() || \in_array($this->driver, ['pgsql', 'sqlite', 'sqlsrv'], true))) {
+                $this->createTable();
+
+                try {
+                    $stmt->execute();
+                } catch (\PDOException $e) {
+                    $this->putOffExpiration($key, $this->initialTtl);
+                }
+            } else {
+                // the lock is already acquired. It could be us. Let's try to put off.
+                $this->putOffExpiration($key, $this->initialTtl);
+            }
         }
 
         $this->randomlyPrune();
@@ -316,4 +326,21 @@ private function getCurrentTimestampStatement(): string
                 return (string) time();
         }
     }
+
+    private function isTableMissing(\PDOException $exception): bool
+    {
+        $driver = $this->getDriver();
+        $code = $exception->getCode();
+
+        switch (true) {
+            case 'pgsql' === $driver && '42P01' === $code:
+            case 'sqlite' === $driver && str_contains($exception->getMessage(), 'no such table:'):
+            case 'oci' === $driver && 942 === $code:
+            case 'sqlsrv' === $driver && 208 === $code:
+            case 'mysql' === $driver && 1146 === $code:
+                return true;
+            default:
+                return false;
+        }
+    }
 }
diff --git a/src/Symfony/Component/Lock/Tests/Store/DoctrineDbalStoreTest.php b/src/Symfony/Component/Lock/Tests/Store/DoctrineDbalStoreTest.php
@@ -79,9 +79,9 @@ public function testAbortAfterExpiration()
     }
 
     /**
-     * @dataProvider provideDsn
+     * @dataProvider provideDsnWithSQLite
      */
-    public function testDsn(string $dsn, string $file = null)
+    public function testDsnWithSQLite(string $dsn, string $file = null)
     {
         $key = new Key(uniqid(__METHOD__, true));
 
@@ -97,12 +97,36 @@ public function testDsn(string $dsn, string $file = null)
         }
     }
 
-    public static function provideDsn()
+    public static function provideDsnWithSQLite()
     {
         $dbFile = tempnam(sys_get_temp_dir(), 'sf_sqlite_cache');
-        yield ['sqlite://localhost/'.$dbFile.'1', $dbFile.'1'];
-        yield ['sqlite3:///'.$dbFile.'3', $dbFile.'3'];
-        yield ['sqlite://localhost/:memory:'];
+        yield 'SQLite file' => ['sqlite://localhost/'.$dbFile.'1', $dbFile.'1'];
+        yield 'SQLite3 file' => ['sqlite3:///'.$dbFile.'3', $dbFile.'3'];
+        yield 'SQLite in memory' => ['sqlite://localhost/:memory:'];
+    }
+
+    /**
+     * @requires extension pdo_pgsql
+     *
+     * @group integration
+     */
+    public function testDsnWithPostgreSQL()
+    {
+        if (!$host = getenv('POSTGRES_HOST')) {
+            $this->markTestSkipped('Missing POSTGRES_HOST env variable');
+        }
+
+        $key = new Key(uniqid(__METHOD__, true));
+
+        try {
+            $store = new DoctrineDbalStore('pgsql://postgres:password@'.$host);
+
+            $store->save($key);
+            $this->assertTrue($store->exists($key));
+        } finally {
+            $pdo = new \PDO('pgsql:host='.$host.';user=postgres;password=password');
+            $pdo->exec('DROP TABLE IF EXISTS lock_keys');
+        }
     }
 
     /**

diff --git a/src/Symfony/Component/Lock/Tests/Store/PdoStoreTest.php b/src/Symfony/Component/Lock/Tests/Store/PdoStoreTest.php
@@ -20,8 +20,6 @@
  * @author Jérémy Derussé <jeremy@derusse.com>
  *
  * @requires extension pdo_sqlite
- *
- * @group integration
  */
 class PdoStoreTest extends AbstractStoreTestCase
 {
@@ -78,9 +76,9 @@ public function testInvalidTtlConstruct()
     }
 
     /**
-     * @dataProvider provideDsn
+     * @dataProvider provideDsnWithSQLite
      */
-    public function testDsn(string $dsn, string $file = null)
+    public function testDsnWithSQLite(string $dsn, string $file = null)
     {
         $key = new Key(uniqid(__METHOD__, true));
 
@@ -96,10 +94,36 @@ public function testDsn(string $dsn, string $file = null)
         }
     }
 
-    public static function provideDsn()
+    public static function provideDsnWithSQLite()
     {
         $dbFile = tempnam(sys_get_temp_dir(), 'sf_sqlite_cache');
-        yield ['sqlite:'.$dbFile.'2', $dbFile.'2'];
-        yield ['sqlite::memory:'];
+        yield 'SQLite file' => ['sqlite:'.$dbFile.'2', $dbFile.'2'];
+        yield 'SQLite in memory' => ['sqlite::memory:'];
+    }
+
+    /**
+     * @requires extension pdo_pgsql
+     *
+     * @group integration
+     */
+    public function testDsnWithPostgreSQL()
+    {
+        if (!$host = getenv('POSTGRES_HOST')) {
+            $this->markTestSkipped('Missing POSTGRES_HOST env variable');
+        }
+
+        $key = new Key(uniqid(__METHOD__, true));
+
+        $dsn = 'pgsql:host='.$host.';user=postgres;password=password';
+
+        try {
+            $store = new PdoStore($dsn);
+
+            $store->save($key);
+            $this->assertTrue($store->exists($key));
+        } finally {
+            $pdo = new \PDO($dsn);
+            $pdo->exec('DROP TABLE IF EXISTS lock_keys');
+        }
     }
 }
diff --git a/src/Symfony/Component/Messenger/Bridge/Doctrine/Transport/PostgreSqlConnection.php b/src/Symfony/Component/Messenger/Bridge/Doctrine/Transport/PostgreSqlConnection.php
@@ -64,6 +64,7 @@ public function get(): ?array
         // https://www.postgresql.org/docs/current/sql-syntax-lexical.html#SQL-SYNTAX-IDENTIFIERS
         $this->executeStatement(sprintf('LISTEN "%s"', $this->configuration['table_name']));
 
+        // The condition should be removed once support for DBAL <3.3 is dropped
         if (method_exists($this->driverConnection, 'getNativeConnection')) {
             $wrappedConnection = $this->driverConnection->getNativeConnection();
         } else {