Fix incorrect handling of transactions when using deferred constraints (v4)

src/Connection.php

@@ -12,6 +12,7 @@
 use Doctrine\DBAL\Connection\StaticServerVersionProvider;
 use Doctrine\DBAL\Driver\API\ExceptionConverter;
 use Doctrine\DBAL\Driver\Connection as DriverConnection;
+use Doctrine\DBAL\Driver\Exception as TheDriverException;
 use Doctrine\DBAL\Driver\Statement as DriverStatement;
 use Doctrine\DBAL\Exception\CommitFailedRollbackOnly;
 use Doctrine\DBAL\Exception\ConnectionLost;
@@ -931,16 +932,30 @@
 
         try {
             $res = $func($this);
-            $this->commit();
 
             $successful = true;
-
-            return $res;
         } finally {
             if (! $successful) {
                 $this->rollBack();
             }
         }
+
+        $shouldRollback = true;
+        try {
+            $this->commit();
+
+            $shouldRollback = false;
+        } catch (TheDriverException $t) {
+            $shouldRollback = false;
+
+            throw $t;
+        } finally {
+            if ($shouldRollback) {
+                $this->rollBack();
+            }
+        }
+
+        return $res;
     }
 
     /**
@@ -1019,17 +1034,26 @@
 
         $connection = $this->connect();
 
-        if ($this->transactionNestingLevel === 1) {
-            try {
-                $connection->commit();
-            } catch (Driver\Exception $e) {
-                throw $this->convertException($e);
+        try {
+            if ($this->transactionNestingLevel === 1) {
+                try {
+                    $connection->commit();
+                } catch (Driver\Exception $e) {
+                    throw $this->convertException($e);
+                }
+            } else {
+                $this->releaseSavepoint($this->_getNestedTransactionSavePointName());
             }
-        } else {
-            $this->releaseSavepoint($this->_getNestedTransactionSavePointName());
+        } finally {
+            $this->updateTransactionStateAfterCommit();
         }
+    }
 
-        --$this->transactionNestingLevel;
+    private function updateTransactionStateAfterCommit(): void
+    {
+        if ($this->transactionNestingLevel !== 0) {
+            --$this->transactionNestingLevel;
+        }
 
         if ($this->autoCommit !== false || $this->transactionNestingLevel !== 0) {
             return;

src/Driver/OCI8/Connection.php

@@ -95,7 +95,7 @@
 
     public function commit(): void
     {
-        if (! oci_commit($this->connection)) {
+        if (! @oci_commit($this->connection)) {
             throw Error::new($this->connection);
         }
 

tests/ConnectionTest.php

@@ -644,4 +644,26 @@ public function rollBack(): void
             self::assertSame('Original exception', $e->getPrevious()->getMessage());
         }
     }
+
+    /**
+     * We are not sure if this can happen in real life scenario
+     */
+    public function testItFailsDuringCommitBeforeTouchingDb(): void
+    {
+        $connection = new class (['memory' => true], new Driver\SQLite3\Driver()) extends Connection {
+            public function commit(): void
+            {
+                throw new \Exception('Fail before touching the db');
+            }
+
+            public function rollBack(): void
+            {
+                throw new \Exception('Rollback got triggered');
+            }
+        };
+
+        $this->expectExceptionMessage('Rollback got triggered');
+        $connection->transactional(static function (): void {
+        });
+    }
 }

tests/Functional/UniqueConstraintViolationsTest.php

@@ -0,0 +1,208 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Doctrine\DBAL\Tests\Functional;
+
+use Doctrine\DBAL\Connection;
+use Doctrine\DBAL\Driver\AbstractPostgreSQLDriver;
+use Doctrine\DBAL\Driver\PDO\Exception as PDOException;
+use Doctrine\DBAL\Driver\PDO\PgSQL\Driver as PDOPgSQLDriver;
+use Doctrine\DBAL\Driver\PgSQL\Driver as PgSQLDriver;
+use Doctrine\DBAL\Driver\PgSQL\Exception as PgSQLException;
+use Doctrine\DBAL\Exception\DriverException;
+use Doctrine\DBAL\Exception\UniqueConstraintViolationException;
+use Doctrine\DBAL\Platforms\DB2Platform;
+use Doctrine\DBAL\Platforms\OraclePlatform;
+use Doctrine\DBAL\Platforms\PostgreSQLPlatform;
+use Doctrine\DBAL\Platforms\SQLitePlatform;
+use Doctrine\DBAL\Platforms\SQLServerPlatform;
+use Doctrine\DBAL\Schema\Table;
+use Doctrine\DBAL\Schema\UniqueConstraint;
+use Doctrine\DBAL\Tests\FunctionalTestCase;
+use PHPUnit\Framework\Assert;
+use Throwable;
+
+use function sprintf;
+
+final class UniqueConstraintViolationsTest extends FunctionalTestCase
+{
+    private string $constraintName = '';
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        $platform = $this->connection->getDatabasePlatform();
+
+        if ($platform instanceof OraclePlatform) {
+            $constraintName = 'C1_UNIQUE';
+        } elseif ($platform instanceof PostgreSQLPlatform) {
+            $constraintName = 'c1_unique';
+        } else {
+            $constraintName = 'c1_unique';
+        }
+
+        $this->constraintName = $constraintName;
+
+        $schemaManager = $this->connection->createSchemaManager();
+
+        $table = new Table('unique_constraint_violations');
+        $table->addColumn('unique_field', 'integer', ['notnull' => true]);
+        $schemaManager->createTable($table);
+
+        if ($platform instanceof OraclePlatform) {
+            $createConstraint = sprintf(
+                'ALTER TABLE unique_constraint_violations ' .
+                'ADD CONSTRAINT %s UNIQUE (unique_field) DEFERRABLE INITIALLY IMMEDIATE',
+                $constraintName,
+            );
+        } elseif ($platform instanceof PostgreSQLPlatform) {
+            $createConstraint = sprintf(
+                'ALTER TABLE unique_constraint_violations ' .
+                'ADD CONSTRAINT %s UNIQUE (unique_field) DEFERRABLE INITIALLY IMMEDIATE',
+                $constraintName,
+            );
+        } elseif ($platform instanceof SQLitePlatform) {
+            $createConstraint = sprintf(
+                'CREATE UNIQUE INDEX %s ON unique_constraint_violations(unique_field)',
+                $constraintName,
+            );
+        } else {
+            $createConstraint = new UniqueConstraint($constraintName, ['unique_field']);
+        }
+
+        if ($createConstraint instanceof UniqueConstraint) {
+            $schemaManager->createUniqueConstraint($createConstraint, 'unique_constraint_violations');
+        } else {
+            $this->connection->executeStatement($createConstraint);
+        }
+
+        $this->connection->executeStatement('INSERT INTO unique_constraint_violations VALUES (1)');
+    }
+
+    public function testTransactionalViolatesDeferredConstraint(): void
+    {
+        $this->skipIfDeferrableIsNotSupported();
+
+        $this->connection->transactional(function (Connection $connection): void {
+            $connection->executeStatement(sprintf('SET CONSTRAINTS "%s" DEFERRED', $this->constraintName));
+
+            $connection->executeStatement('INSERT INTO unique_constraint_violations VALUES (1)');
+
+            $this->expectUniqueConstraintViolation(true);
+        });
+    }
+
+    public function testTransactionalViolatesConstraint(): void
+    {
+        $this->connection->transactional(function (Connection $connection): void {
+            $this->expectUniqueConstraintViolation(false);
+            $connection->executeStatement('INSERT INTO unique_constraint_violations VALUES (1)');
+        });
+    }
+
+    public function testCommitViolatesDeferredConstraint(): void
+    {
+        $this->skipIfDeferrableIsNotSupported();
+
+        $this->connection->beginTransaction();
+        $this->connection->executeStatement(sprintf('SET CONSTRAINTS "%s" DEFERRED', $this->constraintName));
+        $this->connection->executeStatement('INSERT INTO unique_constraint_violations VALUES (1)');
+
+        $this->expectUniqueConstraintViolation(true);
+        $this->connection->commit();
+    }
+
+    public function testInsertViolatesConstraint(): void
+    {
+        $this->connection->beginTransaction();
+
+        try {
+            $this->connection->executeStatement('INSERT INTO unique_constraint_violations VALUES (1)');
+        } catch (Throwable $t) {
+            $this->connection->rollBack();
+
+            $this->expectUniqueConstraintViolation(false);
+
+            throw $t;
+        }
+    }
+
+    private function supportsDeferrableConstraints(): bool
+    {
+        $platform = $this->connection->getDatabasePlatform();
+
+        return $platform instanceof OraclePlatform || $platform instanceof PostgreSQLPlatform;
+    }
+
+    private function skipIfDeferrableIsNotSupported(): void
+    {
+        if ($this->supportsDeferrableConstraints()) {
+            return;
+        }
+
+        self::markTestSkipped('Only databases supporting deferrable constraints are eligible for this test.');
+    }
+
+    private function expectUniqueConstraintViolation(bool $deferred): void
+    {
+        if ($this->connection->getDatabasePlatform() instanceof SQLServerPlatform) {
+            $this->expectExceptionMessage(sprintf("Violation of UNIQUE KEY constraint '%s'", $this->constraintName));
+
+            return;
+        }
+
+        if ($this->connection->getDatabasePlatform() instanceof DB2Platform) {
+            // No concrete message is provided
+            $this->expectException(DriverException::class);
+
+            return;
+        }
+
+        if ($deferred) {
+            if ($this->connection->getDatabasePlatform() instanceof OraclePlatform) {
+                $this->expectExceptionMessageMatches(
+                    sprintf('~unique constraint \(.+\.%s\) violated~', $this->constraintName),
+                );
+
+                return;
+            }
+
+            $driver = $this->connection->getDriver();
+            if ($driver instanceof AbstractPostgreSQLDriver) {
+                $this->expectExceptionMessageMatches(
+                    sprintf('~duplicate key value violates unique constraint "%s"~', $this->constraintName),
+                );
+
+                if ($driver instanceof PDOPgSQLDriver) {
+                    $this->expectException(PDOException::class);
+
+                    return;
+                }
+
+                if ($driver instanceof PgSQLDriver) {
+                    $this->expectException(PgSQLException::class);
+
+                    return;
+                }
+
+                Assert::fail('Unsupported PG driver');
+            }
+
+            Assert::fail('Unsupported platform');
+        } else {
+            $this->expectException(UniqueConstraintViolationException::class);
+        }
+    }
+
+    protected function tearDown(): void
+    {
+        $schemaManager = $this->connection->createSchemaManager();
+        $schemaManager->dropTable('unique_constraint_violations');
+
+        $this->markConnectionNotReusable();
+
+        parent::tearDown();
+    }
+}