10000 bug #52459 [Cache][HttpFoundation][Lock] Fix PDO store not creating t… · symfony/symfony@48be4b3 · GitHub
[go: up one dir, main page]
Skip to content

Commit 48be4b3

Browse files
nicolas-grekasnicolas-grekas
committed
bug #52459 [Cache][HttpFoundation][Lock] Fix PDO store not creating table + 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
2 parents b5376c7 + cb5d832 commit 48be4b3

File tree

9 files changed

+177
-42
lines changed

9 files changed

+177
-42
lines changed

‎src/Symfony/Component/Cache/Adapter/DoctrineDbalAdapter.php

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -420,7 +420,8 @@ private function getServerVersion(): string
420420
return $this->serverVersion;
421421
}
422422

423-
$conn = $this->conn->getWrappedConnection();
423+
// The condition should be removed once support for DBAL <3.3 is dropped
424+
$conn = method_exists($this->conn, 'getNativeConnection') ? $this->conn->getNativeConnection() : $this->conn->getWrappedConnection();
424425
if ($conn instanceof ServerInfoAwareConnection) {
425426
return $this->serverVersion = $conn->getServerVersion();
426427
}

‎src/Symfony/Component/Cache/Adapter/PdoAdapter.php

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -507,7 +507,7 @@ protected function doSave(array $values, int $lifetime)
507507
try {
508508
$stmt = $conn->prepare($sql);
509509
} catch (\PDOException $e) {
510-
if (!$conn->inTransaction() || \in_array($this->driver, ['pgsql', 'sqlite', 'sqlsrv'], true)) {
510+
if ($this->isTableMissing($e) && (!$conn->inTransaction() || \in_array($this->driver, ['pgsql', 'sqlite', 'sqlsrv'], true))) {
511511
$this->createTable();
512512
}
513513
$stmt = $conn->prepare($sql);
@@ -542,7 +542,7 @@ protected function doSave(array $values, int $lifetime)
542542
try {
543543
$stmt->execute();
544544
} catch (\PDOException $e) {
545-
if (!$conn->inTransaction() || \in_array($this->driver, ['pgsql', 'sqlite', 'sqlsrv'], true)) {
545+
if ($this->isTableMissing($e) && (!$conn->inTransaction() || \in_array($this->driver, ['pgsql', 'sqlite', 'sqlsrv'], true))) {
546546
$this->createTable();
547547
}
548548
$stmt->execute();
@@ -596,4 +596,21 @@ private function getServerVersion(): string
596596

597597
return $this->serverVersion;
598598
}
599+
600+
private function isTableMissing(\PDOException $exception): bool
601+
{
602+
$driver = $this->driver;
603+
$code = $exception->getCode();
604+
605+
switch (true) {
606+
case 'pgsql' === $driver && '42P01' === $code:
607+
case 'sqlite' === $driver && str_contains($exception->getMessage(), 'no such table:'):
608+
case 'oci' === $driver && 942 === $code:
609+
case 'sqlsrv' === $driver && 208 === $code:
610+
case 'mysql' === $driver && 1146 === $code:
611+
return true;
612+
default:
613+
return false;
614+
}
615+
}
599616
}

‎src/Symfony/Component/Cache/Tests/Adapter/DoctrineDbalAdapterTest.php

Lines changed: 31 additions & 12 deletion 10000 s
Original file line numberDiff line numberDiff line change
@@ -18,12 +18,13 @@
1818
use Doctrine\DBAL\DriverManager;
1919
use Doctrine\DBAL\Schema\DefaultSchemaManagerFactory;
2020
use Doctrine\DBAL\Schema\Schema;
21-
use PHPUnit\Framework\SkippedTestSuiteError;
2221
use Psr\Cache\CacheItemPoolInterface;
2322
use Symfony\Component\Cache\Adapter\DoctrineDbalAdapter;
2423
use Symfony\Component\Cache\Tests\Fixtures\DriverWrapper;
2524

2625
/**
26+
* @requires extension pdo_sqlite
27+
*
2728
* @group time-sensitive
2829
*/
2930
class DoctrineDbalAdapterTest extends AdapterTestCase
@@ -32,10 +33,6 @@ class DoctrineDbalAdapterTest extends AdapterTestCase
3233

3334
public static function setUpBeforeClass(): void
3435
{
35-
if (!\extension_loaded('pdo_sqlite')) {
36-
throw new SkippedTestSuiteError('Extension pdo_sqlite required.');
37-
}
38-
3936
self::$dbFile = tempnam(sys_get_temp_dir(), 'sf_sqlite_cache');
4037
}
4138

@@ -107,13 +104,12 @@ public function testConfigureSchemaTableExists()
107104
}
108105

109106
/**
110-
* @dataProvider provideDsn
107+
* @dataProvider provideDsnWithSQLite
111108
*/
112-
public function testDsn(string $dsn, string $file = null)
109+
public function testDsnWithSQLite(string $dsn, string $file = null)
113110
{
114111
try {
115112
$pool = new DoctrineDbalAdapter($dsn);
116-
$pool->createTable();
117113

118114
$item = $pool->getItem('key');
119115
$item->set('value');
@@ -125,12 +121,35 @@ public function testDsn(string $dsn, string $file = null)
125121
}
126122
}
127123

128-
public static function provideDsn()
124+
public static function provideDsnWithSQLite()
129125
{
130126
$dbFile = tempnam(sys_get_temp_dir(), 'sf_sqlite_cache');
131-
yield ['sqlite://localhost/'.$dbFile.'1', $dbFile.'1'];
132-
yield ['sqlite3:///'.$dbFile.'3', $dbFile.'3'];
133-
yield ['sqlite://localhost/:memory:'];
127+
yield 'SQLite file' => ['sqlite://localhost/'.$dbFile.'1', $dbFile.'1'];
128+
yield 'SQLite3 file' => ['sqlite3:///'.$dbFile.'3', $dbFile.'3'];
129+
yield 'SQLite in memory' => ['sqlite://localhost/:memory:'];
130+
}
131+
132+
/**
133+
* @requires extension pdo_pgsql
134+
*
135+
* @group integration
136+
*/
137+
public function testDsnWithPostgreSQL()
138+
{
139+
if (!$host = getenv('POSTGRES_HOST')) {
140+
$this->markTestSkipped('Missing POSTGRES_HOST env variable');
141+
}
142+
143+
try {
144+
$pool = new DoctrineDbalAdapter('pgsql://postgres:password@'.$host);
145+
146+
$item = $pool->getItem('key');
147+
$item->set('value');
148+
$this->assertTrue($pool->save($item));
149+
} finally {
150+
$pdo = new \PDO('pgsql:host='.$host.';user=postgres;password=password');
151+
$pdo->exec('DROP TABLE IF EXISTS cache_items');
152+
}
134153
}
135154

136155
protected function isPruned(DoctrineDbalAdapter $cache, string $name): bool

‎src/Symfony/Component/Cache/Tests/Adapter/PdoAdapterTest.php

Lines changed: 32 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -11,11 +11,12 @@
1111

1212
namespace Symfony\Component\Cache\Tests\Adapter;
1313

14-
use PHPUnit\Framework\SkippedTestSuiteError;
1514
use Psr\Cache\CacheItemPoolInterface;
1615
use Symfony\Component\Cache\Adapter\PdoAdapter;
1716

1817
/**
18+
* @requires extension pdo_sqlite
19+
*
1920
* @group time-sensitive
2021
*/
2122
class PdoAdapterTest extends AdapterTestCase
@@ -24,10 +25,6 @@ class PdoAdapterTest extends AdapterTestCase
2425

2526
public static function setUpBeforeClass(): void
2627
{
27-
if (!\extension_loaded('pdo_sqlite')) {
28-
throw new SkippedTestSuiteError('Extension pdo_sqlite required.');
29-
}
30-
3128
self::$dbFile = tempnam(sys_get_temp_dir(), 'sf_sqlite_cache');
3229

3330
$pool = new PdoAdapter('sqlite:'.self::$dbFile);
@@ -71,13 +68,12 @@ public function testCleanupExpiredItems()
7168
}
7269

7370
/**
74-
* @dataProvider provideDsn
71+
* @dataProvider provideDsnSQLite
7572
*/
76-
public function testDsn(string $dsn, string $file = null)
73+
public function testDsnWithSQLite(string $dsn, string $file = null)
7774
{
7875
try {
7976
$pool = new PdoAdapter($dsn);
80-
$pool->createTable();
8177

8278
$item = $pool->getItem('key');
8379
$item->set('value');
@@ -89,11 +85,36 @@ public function testDsn(string $dsn, string $file = null)
8985
}
9086
}
9187

92-
public static function provideDsn()
88+
public static function provideDsnSQLite()
9389
{
9490
$dbFile = tempnam(sys_get_temp_dir(), 'sf_sqlite_cache');
95-
yield ['sqlite:'.$dbFile.'2', $dbFile.'2'];
96-
yield ['sqlite::memory:'];
91+
yield 'SQLite file' => ['sqlite:'.$dbFile.'2', $dbFile.'2'];
92+
yield 'SQLite in memory' => ['sqlite::memory:'];
93+
}
94+
95+
/**
96+
* @requires extension pdo_pgsql
97+
*
98+
* @group integration
99+
*/
100+
public function testDsnWithPostgreSQL()
101+
{
102+
if (!$host = getenv('POSTGRES_HOST')) {
103+
$this->markTestSkipped('Missing POSTGRES_HOST env variable');
104+
}
105+
106+
$dsn = 'pgsql:host='.$host.';user=postgres;password=password';
107+
108+
try {
109+
$pool = new PdoAdapter($dsn);
110+
111+
$item = $pool->getItem('key');
112+
$item->set('value');
113+
$this->assertTrue($pool->save($item));
114+
} finally {
115+
$pdo = new \PDO($dsn);
116+
$pdo->exec('DROP TABLE IF EXISTS cache_items');
117+
}
97118
}
98119

99120
protected function isPruned(PdoAdapter $cache, string $name): bool

‎src/Symfony/Component/HttpFoundation/Session/Storage/Handler/SessionHandlerFactory.php

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,7 @@ public static function createHandler($connection): AbstractSessionHandler
8282
}
8383

8484
$connection = DriverManager::getConnection($params, $config);
85+
// The condition should be removed once support for DBAL <3.3 is dropped
8586
$connection = method_exists($connection, 'getNativeConnection') ? $connection->getNativeConnection() : $connection->getWrappedConnection();
8687
// no break;
8788

‎src/Symfony/Component/Lock/Store/PdoStore.php

Lines changed: 30 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ public function save(Key $key)
115115
try {
116116
$stmt = $conn->prepare($sql);
117117
} catch (\PDOException $e) {
118-
if (!$conn->inTransaction() || \in_array($this->driver, ['pgsql', 'sqlite', 'sqlsrv'], true)) {
118+
if ($this->isTableMissing($e) && (!$conn->inTransaction() || \in_array($this->driver, ['pgsql', 'sqlite', 'sqlsrv'], true))) {
119119
$this->createTable();
120120
}
121121
$stmt = $conn->prepare($sql);
@@ -127,8 +127,18 @@ public function save(Key $key)
127127
try {
128128
$stmt->execute();
129129
} catch (\PDOException $e) {
130-
// the lock is already acquired. It could be us. Let's try to put off.
131-
$this->putOffE B41A xpiration($key, $this->initialTtl);
130+
if ($this->isTableMissing($e) && (!$conn->inTransaction() || \in_array($this->driver, ['pgsql', 'sqlite', 'sqlsrv'], true))) {
131+
$this->createTable();
132+
133+
try {
134+
$stmt->execute();
135+
} catch (\PDOException $e) {
136+
$this->putOffExpiration($key, $this->initialTtl);
137+
}
138+
} else {
139+
// the lock is already acquired. It could be us. Let's try to put off.
140+
$this->putOffExpiration($key, $this->initialTtl);
141+
}
132142
}
133143

134144
$this->randomlyPrune();
@@ -316,4 +326,21 @@ private function getCurrentTimestampStatement(): string
316326
return (string) time();
317327
}
318328
}
329+
330+
private function isTableMissing(\PDOException $exception): bool
331+
{
332+
$driver = $this->getDriver();
333+
$code = $exception->getCode();
334+
335+
switch (true) {
336+
case 'pgsql' === $driver && '42P01' === $code:
337+
case 'sqlite' === $driver && str_contains($exception->getMessage(), 'no such table:'):
338+
case 'oci' === $driver && 942 === $code:
339+
case 'sqlsrv' === $driver && 208 === $code:
340+
case 'mysql' === $driver && 1146 === $code:
341+
return true;
342+
default:
343+
return false;
344+
}
345+
}
319346
}

‎src/Symfony/Component/Lock/Tests/Store/DoctrineDbalStoreTest.php

Lines changed: 30 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -79,9 +79,9 @@ public function testAbortAfterExpiration()
7979
}
8080

8181
/**
82-
* @dataProvider provideDsn
82+
* @dataProvider provideDsnWithSQLite
8383
*/
84-
public function testDsn(string $dsn, string $file = null)
84+
public function testDsnWithSQLite(string $dsn, string $file = null)
8585
{
8686
$key = new Key(uniqid(__METHOD__, true));
8787

@@ -97,12 +97,36 @@ public function testDsn(string $dsn, string $file = null)
9797
}
9898
}
9999

100-
public static function provideDsn()
100+
public static function provideDsnWithSQLite()
101101
{
102102
$dbFile = tempnam(sys_get_temp_dir(), 'sf_sqlite_cache');
103-
yield ['sqlite://localhost/'.$dbFile.'1', $dbFile.'1'];
104-
yield ['sqlite3:///'.$dbFile.'3', $dbFile.'3'];
105-
yield ['sqlite://localhost/:memory:'];
103+
yield 'SQLite file' => ['sqlite://localhost/'.$dbFile.'1', $dbFile.'1'];
104+
yield 'SQLite3 file' => ['sqlite3:///'.$dbFile.'3', $dbFile.'3'];
105+
yield 'SQLite in memory' => ['sqlite://localhost/:memory:'];
106+
}
107+
108+
/**
109+
* @requires extension pdo_pgsql
110+
*
111+
* @group integration
112+
*/
113+
public function testDsnWithPostgreSQL()
114+
{
115+
if (!$host = getenv('POSTGRES_HOST')) {
116+
$this->markTestSkipped('Missing POSTGRES_HOST env variable');
117+
}
118+
119+
$key = new Key(uniqid(__METHOD__, true));
120+
121+
try {
122+
$store = new DoctrineDbalStore('pgsql://postgres:password@'.$host);
123+
124+
$store->save($key);
125+
$this->assertTrue($store->exists($key));
126+
} finally {
127+
$pdo = new \PDO('pgsql:host='.$host.';user=postgres;password=password');
128+
$pdo->exec('DROP TABLE IF EXISTS lock_keys');
129+
}
106130
}
107131

108132
/**

‎src/Symfony/Component/Lock/Tests/Store/PdoStoreTest.php

Lines changed: 31 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,6 @@
2020
* @author Jérémy Derussé <jeremy@derusse.com>
2121
*
2222
* @requires extension pdo_sqlite
23-
*
24-
* @group integration
2523
*/
2624
class PdoStoreTest extends AbstractStoreTestCase
2725
{
@@ -78,9 +76,9 @@ public function testInvalidTtlConstruct()
7876
}
7977

8078
/**
81-
* @dataProvider provideDsn
79+
* @dataProvider provideDsnWithSQLite
8280
*/
83-
public function testDsn(string $dsn, string $file = null)
81+
public function testDsnWithSQLite(string $dsn, string $file = null)
8482
{
8583
$key = new Key(uniqid(__METHOD__, true));
8684

@@ -96,10 +94,36 @@ public function testDsn(string $dsn, string $file = null)
9694
}
9795
}
9896

99-
public static function provideDsn()
97+
public static function provideDsnWithSQLite()
10098
{
10199
$dbFile = tempnam(sys_get_temp_dir(), 'sf_sqlite_cache');
102-
yield ['sqlite:'.$dbFile.'2', $dbFile.'2'];
103-
yield ['sqlite::memory:'];
100+
yield 'SQLite file' => ['sqlite:'.$dbFile.'2', $dbFile.'2'];
101+
yield 'SQLite in memory' => ['sqlite::memory:'];
102+
}
103+
104+
/**
105+
* @requires extension pdo_pgsql
106+
*
107+
* @group integration
108+
*/
109+
public function testDsnWithPostgreSQL()
110+
{
111+
if (!$host = getenv('POSTGRES_HOST')) {
112+
$this->markTestSkipped('Missing POSTGRES_HOST env variable');
113+
}
114+
115+
$key = new Key(uniqid(__METHOD__, true));
116+
117+
$dsn = 'pgsql:host='.$host.';user=postgres;password=password';
118+
119+
try {
120+
$store = new PdoStore($dsn);
121+
122+
$store->save($key);
123+
$this->assertTrue($store->exists($key));
124+
} finally {
125+
$pdo = new \PDO($dsn);
126+
$pdo->exec('DROP TABLE IF EXISTS lock_keys');
127+
}
104128
}
105129
}

‎src/Symfony/Component/Messenger/Bridge/Doctrine/Transport/PostgreSqlConnection.php

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,7 @@ public function get(): ?array
6464
// https://www.postgresql.org/docs/current/sql-syntax-lexical.html#SQL-SYNTAX-IDENTIFIERS
6565
$this->executeStatement(sprintf('LISTEN "%s"', $this->configuration['table_name']));
6666

67+
// The condition should be removed once support for DBAL <3.3 is dropped
6768
if (method_exists($this->driverConnection, 'getNativeConnection')) {
6869
$wrappedConnection = $this->driverConnection->getNativeConnection();
6970
} else {

0 commit comments

Comments
 (0)
0