From 73229446693c2bdd1a52d5e0bce43b834ace05bf Mon Sep 17 00:00:00 2001 From: Simon Podlipsky Date: Mon, 2 Nov 2020 13:00:56 +0100 Subject: [PATCH 1/2] Remove redundant phpstan param from DriverManager::getConnection() This effectively prevented phpstan from inferring type of `T` template. > Unable to resolve the template type T in call to method static method Doctrine\DBAL\DriverManager::getConnection() --- lib/Doctrine/DBAL/DriverManager.php | 1 - tests/Doctrine/Tests/DBAL/ConnectionTest.php | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/Doctrine/DBAL/DriverManager.php b/lib/Doctrine/DBAL/DriverManager.php index 17ba8584da2..6a11b489f4a 100644 --- a/lib/Doctrine/DBAL/DriverManager.php +++ b/lib/Doctrine/DBAL/DriverManager.php @@ -119,7 +119,6 @@ private function __construct() * * @throws Exception * - * @phpstan-param mixed[] $params * @psalm-return ($params is array{wrapperClass:mixed} ? T : Connection) * @template T of Connection */ diff --git a/tests/Doctrine/Tests/DBAL/ConnectionTest.php b/tests/Doctrine/Tests/DBAL/ConnectionTest.php index 675881ac207..32129bf6c91 100644 --- a/tests/Doctrine/Tests/DBAL/ConnectionTest.php +++ b/tests/Doctrine/Tests/DBAL/ConnectionTest.php @@ -36,7 +36,7 @@ class ConnectionTest extends DbalTestCase /** @var Connection */ private $connection; - /** @var string[] */ + /** @var array{wrapperClass?: class-string} */ protected $params = [ 'driver' => 'pdo_mysql', 'host' => 'localhost', From 3ed11aa8a9e988b398a368ff4a45964693eb59be Mon Sep 17 00:00:00 2001 From: Benjamin Morel Date: Sun, 1 Nov 2020 23:22:31 +0100 Subject: [PATCH 2/2] LockMode::NONE should not set WITH (NOLOCK) This fixes the issue detailed in #4391, with SQL Server and SQL Anywhere setting WITH (NOLOCK) for LockMode::NONE, which effectively means using a READ UNCOMMITTED isolation level at table level, which is not the contract of LockMode::NONE. --- .../DBAL/Platforms/SQLAnywherePlatform.php | 2 +- .../DBAL/Platforms/SQLServerPlatform.php | 2 +- .../DBAL/Functional/LockMode/NoneTest.php | 101 ++++++++++++++++++ .../AbstractSQLServerPlatformTestCase.php | 4 +- .../Platforms/SQLAnywherePlatformTest.php | 2 +- .../Platforms/SQLServer2012PlatformTest.php | 4 +- .../DBAL/Platforms/SQLServerPlatformTest.php | 2 +- 7 files changed, 109 insertions(+), 8 deletions(-) create mode 100644 tests/Doctrine/Tests/DBAL/Functional/LockMode/NoneTest.php diff --git a/lib/Doctrine/DBAL/Platforms/SQLAnywherePlatform.php b/lib/Doctrine/DBAL/Platforms/SQLAnywherePlatform.php index f7921cbde48..fd7eafdc71b 100644 --- a/lib/Doctrine/DBAL/Platforms/SQLAnywherePlatform.php +++ b/lib/Doctrine/DBAL/Platforms/SQLAnywherePlatform.php @@ -52,7 +52,7 @@ public function appendLockHint($fromClause, $lockMode) { switch (true) { case $lockMode === LockMode::NONE: - return $fromClause . ' WITH (NOLOCK)'; + return $fromClause; case $lockMode === LockMode::PESSIMISTIC_READ: return $fromClause . ' WITH (UPDLOCK)'; diff --git a/lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php b/lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php index b41f33bef46..693517609c4 100644 --- a/lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php +++ b/lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php @@ -1574,7 +1574,7 @@ public function appendLockHint($fromClause, $lockMode) { switch (true) { case $lockMode === LockMode::NONE: - return $fromClause . ' WITH (NOLOCK)'; + return $fromClause; case $lockMode === LockMode::PESSIMISTIC_READ: return $fromClause . ' WITH (HOLDLOCK, ROWLOCK)'; diff --git a/tests/Doctrine/Tests/DBAL/Functional/LockMode/NoneTest.php b/tests/Doctrine/Tests/DBAL/Functional/LockMode/NoneTest.php new file mode 100644 index 00000000000..e91dd2a824b --- /dev/null +++ b/tests/Doctrine/Tests/DBAL/Functional/LockMode/NoneTest.php @@ -0,0 +1,101 @@ +connection->getDriver() instanceof OCI8\Driver) { + // https://github.com/doctrine/dbal/issues/4417 + self::markTestSkipped('This test fails on OCI8 for a currently unknown reason'); + } + + if ($this->connection->getDatabasePlatform() instanceof SQLServerPlatform) { + // Use row versioning instead of locking on SQL Server (if we don't, the second connection will block when + // attempting to read the row created by the first connection, instead of reading the previous version); + // for some reason we cannot set READ_COMMITTED_SNAPSHOT ON when not running this test in isolation, + // there may be another connection active at this point; temporarily forcing to SINGLE_USER does the trick. + $db = $this->connection->getDatabase(); + $this->connection->executeStatement('ALTER DATABASE ' . $db . ' SET SINGLE_USER WITH ROLLBACK IMMEDIATE'); + $this->connection->executeStatement('ALTER DATABASE ' . $db . ' SET READ_COMMITTED_SNAPSHOT ON'); + $this->connection->executeStatement('ALTER DATABASE ' . $db . ' SET MULTI_USER'); + } + + $table = new Table('users'); + $table->addColumn('id', 'integer'); + $table->setPrimaryKey(['id']); + + $this->connection->getSchemaManager()->dropAndCreateTable($table); + + $this->connection2 = TestUtil::getConnection(); + + if ($this->connection2->getSchemaManager()->tablesExist('users')) { + return; + } + + if ($this->connection2->getDatabasePlatform() instanceof SqlitePlatform) { + self::markTestSkipped('This test cannot run on SQLite using an in-memory database'); + } + + self::fail('Separate connections do not seem to talk to the same database'); + } + + public function tearDown(): void + { + parent::tearDown(); + + if ($this->connection2->isTransactionActive()) { + $this->connection2->rollBack(); + } + + $this->connection2->close(); + + $this->connection->getSchemaManager()->dropTable('users'); + + if (! $this->connection->getDatabasePlatform() instanceof SQLServerPlatform) { + return; + } + + $db = $this->connection->getDatabase(); + $this->connection->executeStatement('ALTER DATABASE ' . $db . ' SET READ_COMMITTED_SNAPSHOT OFF'); + } + + public function testLockModeNoneDoesNotBreakTransactionIsolation(): void + { + try { + $this->connection->setTransactionIsolation(TransactionIsolationLevel::READ_COMMITTED); + $this->connection2->setTransactionIsolation(TransactionIsolationLevel::READ_COMMITTED); + } catch (Exception $e) { + self::markTestSkipped('This test must be able to set a transaction isolation level'); + } + + $this->connection->beginTransaction(); + $this->connection2->beginTransaction(); + + $this->connection->insert('users', ['id' => 1]); + + $query = 'SELECT id FROM users'; + $query = $this->connection2->getDatabasePlatform()->appendLockHint($query, LockMode::NONE); + + self::assertFalse($this->connection2->fetchOne($query)); + } +} diff --git a/tests/Doctrine/Tests/DBAL/Platforms/AbstractSQLServerPlatformTestCase.php b/tests/Doctrine/Tests/DBAL/Platforms/AbstractSQLServerPlatformTestCase.php index 7a63f3ffccf..273e109beb9 100644 --- a/tests/Doctrine/Tests/DBAL/Platforms/AbstractSQLServerPlatformTestCase.php +++ b/tests/Doctrine/Tests/DBAL/Platforms/AbstractSQLServerPlatformTestCase.php @@ -374,14 +374,14 @@ public function testModifyLimitQueryWithOrderByClause(): void } $sql = 'SELECT m0_.NOMBRE AS NOMBRE0, m0_.FECHAINICIO AS FECHAINICIO1, m0_.FECHAFIN AS FECHAFIN2' - . ' FROM MEDICION m0_ WITH (NOLOCK)' + . ' FROM MEDICION m0_' . ' INNER JOIN ESTUDIO e1_ ON m0_.ESTUDIO_ID = e1_.ID' . ' INNER JOIN CLIENTE c2_ ON e1_.CLIENTE_ID = c2_.ID' . ' INNER JOIN USUARIO u3_ ON c2_.ID = u3_.CLIENTE_ID' . ' WHERE u3_.ID = ? ORDER BY m0_.FECHAINICIO DESC'; $alteredSql = 'SELECT TOP 15 m0_.NOMBRE AS NOMBRE0, m0_.FECHAINICIO AS FECHAINICIO1, m0_.FECHAFIN AS FECHAFIN2' - . ' FROM MEDICION m0_ WITH (NOLOCK)' + . ' FROM MEDICION m0_' . ' INNER JOIN ESTUDIO e1_ ON m0_.ESTUDIO_ID = e1_.ID' . ' INNER JOIN CLIENTE c2_ ON e1_.CLIENTE_ID = c2_.ID' . ' INNER JOIN USUARIO u3_ ON c2_.ID = u3_.CLIENTE_ID' diff --git a/tests/Doctrine/Tests/DBAL/Platforms/SQLAnywherePlatformTest.php b/tests/Doctrine/Tests/DBAL/Platforms/SQLAnywherePlatformTest.php index 5f3bd50e2ac..119b87bc3f9 100644 --- a/tests/Doctrine/Tests/DBAL/Platforms/SQLAnywherePlatformTest.php +++ b/tests/Doctrine/Tests/DBAL/Platforms/SQLAnywherePlatformTest.php @@ -266,7 +266,7 @@ public static function getLockHints(): iterable [null, ''], [false, ''], [true, ''], - [LockMode::NONE, ' WITH (NOLOCK)'], + [LockMode::NONE, ''], [LockMode::OPTIMISTIC, ''], [LockMode::PESSIMISTIC_READ, ' WITH (UPDLOCK)'], [LockMode::PESSIMISTIC_WRITE, ' WITH (XLOCK)'], diff --git a/tests/Doctrine/Tests/DBAL/Platforms/SQLServer2012PlatformTest.php b/tests/Doctrine/Tests/DBAL/Platforms/SQLServer2012PlatformTest.php index 1c21098b678..d4f8220e9df 100644 --- a/tests/Doctrine/Tests/DBAL/Platforms/SQLServer2012PlatformTest.php +++ b/tests/Doctrine/Tests/DBAL/Platforms/SQLServer2012PlatformTest.php @@ -224,14 +224,14 @@ public function testModifyLimitQueryWithExtraLongQuery(): void public function testModifyLimitQueryWithOrderByClause(): void { $sql = 'SELECT m0_.NOMBRE AS NOMBRE0, m0_.FECHAINICIO AS FECHAINICIO1, m0_.FECHAFIN AS FECHAFIN2' - . ' FROM MEDICION m0_ WITH (NOLOCK)' + . ' FROM MEDICION m0_' . ' INNER JOIN ESTUDIO e1_ ON m0_.ESTUDIO_ID = e1_.ID' . ' INNER JOIN CLIENTE c2_ ON e1_.CLIENTE_ID = c2_.ID' . ' INNER JOIN USUARIO u3_ ON c2_.ID = u3_.CLIENTE_ID' . ' WHERE u3_.ID = ? ORDER BY m0_.FECHAINICIO DESC'; $expected = 'SELECT m0_.NOMBRE AS NOMBRE0, m0_.FECHAINICIO AS FECHAINICIO1, m0_.FECHAFIN AS FECHAFIN2' - . ' FROM MEDICION m0_ WITH (NOLOCK)' + . ' FROM MEDICION m0_' . ' INNER JOIN ESTUDIO e1_ ON m0_.ESTUDIO_ID = e1_.ID' . ' INNER JOIN CLIENTE c2_ ON e1_.CLIENTE_ID = c2_.ID' . ' INNER JOIN USUARIO u3_ ON c2_.ID = u3_.CLIENTE_ID' diff --git a/tests/Doctrine/Tests/DBAL/Platforms/SQLServerPlatformTest.php b/tests/Doctrine/Tests/DBAL/Platforms/SQLServerPlatformTest.php index 2357c1a796b..08f2e4a5530 100644 --- a/tests/Doctrine/Tests/DBAL/Platforms/SQLServerPlatformTest.php +++ b/tests/Doctrine/Tests/DBAL/Platforms/SQLServerPlatformTest.php @@ -39,7 +39,7 @@ public static function getLockHints(): iterable { return [ [null, ''], - [LockMode::NONE, ' WITH (NOLOCK)'], + [LockMode::NONE, ''], [LockMode::OPTIMISTIC, ''], [LockMode::PESSIMISTIC_READ, ' WITH (HOLDLOCK, ROWLOCK)'], [LockMode::PESSIMISTIC_WRITE, ' WITH (UPDLOCK, ROWLOCK)'],