Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

[Cache][HttpFoundation][Lock] Fix PDO store not creating table + add tests #52459

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 20, 2023
Merged

[Cache][HttpFoundation][Lock] Fix PDO store not creating table + add tests #52459

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion 3 src/Symfony/Component/Cache/Adapter/DoctrineDbalAdapter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
GromNaN marked this conversation as resolved.
Show resolved Hide resolved
if ($conn instanceof ServerInfoAwareConnection) {
return $this->serverVersion = $conn->getServerVersion();
}
Expand Down
21 changes: 19 additions & 2 deletions 21 src/Symfony/Component/Cache/Adapter/PdoAdapter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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;
}
}
}
43 changes: 31 additions & 12 deletions 43 src/Symfony/Component/Cache/Tests/Adapter/DoctrineDbalAdapterTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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');
}

Expand Down Expand Up @@ -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');
Expand All @@ -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
Expand Down
43 changes: 32 additions & 11 deletions 43 src/Symfony/Component/Cache/Tests/Adapter/PdoAdapterTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);
Expand Down Expand Up @@ -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');
Expand All @@ -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
Expand Down
1 change: 1 addition & 0 deletions 1 src/Symfony/Component/HttpFoundation/Session/Storage/Handler/SessionHandlerFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
33 changes: 30 additions & 3 deletions 33 src/Symfony/Component/Lock/Store/PdoStore.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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))) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed that the PdoAdapter has the same logic but without the $this->isTableMissing() part so I'm wondering, should I add it there as well, or remove it form here to keep it consistent?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes indeed, also in DoctrineDbalAdapter, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The DBAL adapter catches TableNotFoundException so it is already fine (and the DBAL adapter uses the DBAL API that does the preparation and the execution internally so the try/catch wraps both).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the try/catch is around the call to prepare, not to execute, that's why I'm wondering

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added the logic to PdoAdapter, the DoctrineDbalAdapter already works as expected. I've added some tests to the adapters/stores to make super everything works as expected.

$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();
Expand Down Expand Up @@ -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:
Comment on lines +335 to +340
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taken from Doctrine.

return true;
default:
return false;
}
}
}
36 changes: 30 additions & 6 deletions 36 src/Symfony/Component/Lock/Tests/Store/DoctrineDbalStoreTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));

Expand All @@ -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');
}
}

/**
Expand Down
38 changes: 31 additions & 7 deletions 38 src/Symfony/Component/Lock/Tests/Store/PdoStoreTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@
* @author Jérémy Derussé <jeremy@derusse.com>
*
* @requires extension pdo_sqlite
*
* @group integration
*/
class PdoStoreTest extends AbstractStoreTestCase
{
Expand Down Expand Up @@ -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));

Expand All @@ -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');
}
}
}
1 change: 1 addition & 0 deletions 1 src/Symfony/Component/Messenger/Bridge/Doctrine/Transport/PostgreSqlConnection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Morty Proxy This is a proxified and sanitized view of the page, visit original site.