Merge pull request #4400 from BenMorel/with-nolock

LockMode::NONE should not set WITH (NOLOCK)

Author: morozov

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)';

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)';

tests/Doctrine/Tests/DBAL/Functional/LockMode/NoneTest.php

@@ -0,0 +1,101 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Doctrine\Tests\DBAL\Functional\LockMode;
+
+use Doctrine\DBAL\Connection;
+use Doctrine\DBAL\Driver\OCI8;
+use Doctrine\DBAL\Exception;
+use Doctrine\DBAL\LockMode;
+use Doctrine\DBAL\Platforms\SqlitePlatform;
+use Doctrine\DBAL\Platforms\SQLServerPlatform;
+use Doctrine\DBAL\Schema\Table;
+use Doctrine\DBAL\TransactionIsolationLevel;
+use Doctrine\Tests\DbalFunctionalTestCase;
+use Doctrine\Tests\TestUtil;
+
+class NoneTest extends DbalFunctionalTestCase
+{
+    /** @var Connection */
+    private $connection2;
+
+    public function setUp(): void
+    {
+        parent::setUp();
+
+        if ($this->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));
+    }
+}

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'

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)'],

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'

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)'],