From 179daaa1efc55c836f73737b4b6783c7a2dc55c8 Mon Sep 17 00:00:00 2001 From: Simon Podlipsky Date: Fri, 4 Jan 2019 14:20:32 +0100 Subject: [PATCH] Fix incorrect handling of transactions using deferred constraints MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Let root Errors bubble up - Catch concrete Exception in TransactionTest as we can do it now - Expose underlying errors on Oracle platform Co-authored-by: Jakub Chábek --- src/Connection.php | 26 +- src/Driver/OCI8/Connection.php | 2 +- src/Driver/OCI8/Exception/Error.php | 18 +- src/Driver/PDO/Connection.php | 6 +- src/Driver/PDO/Exception.php | 18 +- tests/Driver/PDO/ExceptionTest.php | 21 ++ .../Functional/DeferrableConstraintsTest.php | 258 ++++++++++++++++++ tests/Functional/TransactionTest.php | 4 +- 8 files changed, 340 insertions(+), 13 deletions(-) create mode 100644 tests/Functional/DeferrableConstraintsTest.php diff --git a/src/Connection.php b/src/Connection.php index 12dcf5bd2a6..35d09d4ff14 100644 --- a/src/Connection.php +++ b/src/Connection.php @@ -1202,14 +1202,15 @@ public function transactional(Closure $func) $this->beginTransaction(); try { $res = $func($this); - $this->commit(); - - return $res; } catch (Throwable $e) { $this->rollBack(); throw $e; } + + $this->commit(); + + return $res; } /** @@ -1320,7 +1321,13 @@ public function commit() $logger->startQuery('"COMMIT"'); } - $result = $connection->commit(); + try { + $result = $connection->commit(); + } catch (Driver\Exception $e) { + $this->updateTransactionStateAfterCommit(); + + throw $this->convertExceptionDuringQuery($e, 'COMMIT'); + } if ($logger !== null) { $logger->stopQuery(); @@ -1336,17 +1343,22 @@ public function commit() } } + $this->updateTransactionStateAfterCommit(); + + return $result; + } + + private function updateTransactionStateAfterCommit(): void + { --$this->transactionNestingLevel; $this->getEventManager()->dispatchEvent(Events::onTransactionCommit, new TransactionCommitEventArgs($this)); if ($this->autoCommit !== false || $this->transactionNestingLevel !== 0) { - return $result; + return; } $this->beginTransaction(); - - return $result; } /** diff --git a/src/Driver/OCI8/Connection.php b/src/Driver/OCI8/Connection.php index 12149e58129..e6101983d06 100644 --- a/src/Driver/OCI8/Connection.php +++ b/src/Driver/OCI8/Connection.php @@ -156,7 +156,7 @@ public function beginTransaction() */ public function commit() { - if (! oci_commit($this->dbh)) { + if (! @oci_commit($this->dbh)) { throw Error::new($this->dbh); } diff --git a/src/Driver/OCI8/Exception/Error.php b/src/Driver/OCI8/Exception/Error.php index 7a27141fe5e..30ed1f8d079 100644 --- a/src/Driver/OCI8/Exception/Error.php +++ b/src/Driver/OCI8/Exception/Error.php @@ -7,7 +7,9 @@ use Doctrine\DBAL\Driver\AbstractException; use function assert; +use function explode; use function oci_error; +use function str_replace; /** * @internal @@ -16,6 +18,8 @@ */ final class Error extends AbstractException { + private const CODE_TRANSACTION_ROLLED_BACK = 2091; + /** * @param resource $resource */ @@ -24,6 +28,18 @@ public static function new($resource): self $error = oci_error($resource); assert($error !== false); - return new self($error['message'], null, $error['code']); + $code = $error['code']; + $message = $error['message']; + if ($code === self::CODE_TRANSACTION_ROLLED_BACK) { + // There's no way this can be unit-tested as it's impossible to mock $resource + //ORA-02091: transaction rolled back + //ORA-00001: unique constraint (DOCTRINE.GH3423_UNIQUE) violated + [$firstMessage, $secondMessage] = explode("\n", $message, 2); + + [$code, $message] = explode(': ', $secondMessage, 2); + $code = (int) str_replace('ORA-', '', $code); + } + + return new self($message, null, $code); } } diff --git a/src/Driver/PDO/Connection.php b/src/Driver/PDO/Connection.php index 4d7b90b6e31..0a72be3f372 100644 --- a/src/Driver/PDO/Connection.php +++ b/src/Driver/PDO/Connection.php @@ -140,7 +140,11 @@ public function beginTransaction() */ public function commit() { - return $this->connection->commit(); + try { + return $this->connection->commit(); + } catch (PDOException $exception) { + throw Exception::new($exception); + } } /** diff --git a/src/Driver/PDO/Exception.php b/src/Driver/PDO/Exception.php index 49f55951d82..a9208f0b6e9 100644 --- a/src/Driver/PDO/Exception.php +++ b/src/Driver/PDO/Exception.php @@ -7,6 +7,9 @@ use Doctrine\DBAL\Driver\AbstractException; use PDOException; +use function explode; +use function str_replace; + /** * @internal * @@ -14,8 +17,12 @@ */ final class Exception extends AbstractException { + private const CODE_TRANSACTION_ROLLED_BACK = 2091; + public static function new(PDOException $exception): self { + $message = $exception->getMessage(); + if ($exception->errorInfo !== null) { [$sqlState, $code] = $exception->errorInfo; } else { @@ -23,6 +30,15 @@ public static function new(PDOException $exception): self $sqlState = null; } - return new self($exception->getMessage(), $sqlState, $code, $exception); + if ($code === self::CODE_TRANSACTION_ROLLED_BACK) { + //ORA-02091: transaction rolled back + //ORA-00001: unique constraint (DOCTRINE.GH3423_UNIQUE) violated + [$firstMessage, $secondMessage] = explode("\n", $message, 2); + + [$code, $message] = explode(': ', $secondMessage, 2); + $code = (int) str_replace('ORA-', '', $code); + } + + return new self($message, $sqlState, $code, $exception); } } diff --git a/tests/Driver/PDO/ExceptionTest.php b/tests/Driver/PDO/ExceptionTest.php index b00c5da4c80..9863f1e9c4d 100644 --- a/tests/Driver/PDO/ExceptionTest.php +++ b/tests/Driver/PDO/ExceptionTest.php @@ -59,4 +59,25 @@ public function testOriginalExceptionIsInChain(): void { self::assertSame($this->wrappedException, $this->exception->getPrevious()); } + + public function testExposesUnderlyingErrorOnOracle(): void + { + $pdoException = new PDOException(<<errorInfo = [self::SQLSTATE, 2091, + + ]; + + $exception = Exception::new($pdoException); + + self::assertSame(1, $exception->getCode()); + self::assertStringContainsString( + 'unique constraint (DOCTRINE.C1_UNIQUE) violated', + $exception->getMessage() + ); + } } diff --git a/tests/Functional/DeferrableConstraintsTest.php b/tests/Functional/DeferrableConstraintsTest.php new file mode 100644 index 00000000000..17b876efbfa --- /dev/null +++ b/tests/Functional/DeferrableConstraintsTest.php @@ -0,0 +1,258 @@ +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('deferrable_constraints'); + $table->addColumn('unique_field', 'integer', ['notnull' => true]); + $schemaManager->createTable($table); + + if ($platform instanceof OraclePlatform) { + $createConstraint = sprintf( + 'ALTER TABLE deferrable_constraints ' . + 'ADD CONSTRAINT %s UNIQUE (unique_field) DEFERRABLE INITIALLY IMMEDIATE', + $constraintName + ); + } elseif ($platform instanceof PostgreSQLPlatform) { + $createConstraint = sprintf( + 'ALTER TABLE deferrable_constraints ' . + 'ADD CONSTRAINT %s UNIQUE (unique_field) DEFERRABLE INITIALLY IMMEDIATE', + $constraintName + ); + } elseif ($platform instanceof SqlitePlatform) { + $createConstraint = sprintf( + 'CREATE UNIQUE INDEX %s ON deferrable_constraints(unique_field)', + $constraintName + ); + } else { + $createConstraint = new UniqueConstraint($constraintName, ['unique_field']); + } + + if ($createConstraint instanceof UniqueConstraint) { + $schemaManager->createUniqueConstraint($createConstraint, 'deferrable_constraints'); + } else { + $this->connection->executeStatement($createConstraint); + } + + $this->connection->executeStatement('INSERT INTO deferrable_constraints VALUES (1)'); + } + + public function testTransactionalWithDeferredConstraint(): void + { + $this->skipIfDeferrableIsNotSupported(); + + $this->connection->transactional(function (Connection $connection): void { + $connection->executeStatement(sprintf('SET CONSTRAINTS "%s" DEFERRED', $this->constraintName)); + + $connection->executeStatement('INSERT INTO deferrable_constraints VALUES (1)'); + + $this->expectUniqueConstraintViolation(); + }); + } + + public function testTransactionalWithNonDeferredConstraint(): void + { + $this->connection->transactional(function (Connection $connection): void { + $this->expectUniqueConstraintViolation(); + $connection->executeStatement('INSERT INTO deferrable_constraints VALUES (1)'); + }); + } + + public function testTransactionalWithDeferredConstraintAndTransactionNesting(): void + { + if (! $this->connection->getDatabasePlatform()->supportsSavepoints()) { + self::markTestSkipped('This test requires the platform to support savepoints.'); + } + + $this->skipIfDeferrableIsNotSupported(); + + $this->connection->setNestTransactionsWithSavepoints(true); + + $this->connection->transactional(function (Connection $connection): void { + $connection->executeStatement(sprintf('SET CONSTRAINTS "%s" DEFERRED', $this->constraintName)); + $connection->beginTransaction(); + $connection->executeStatement('INSERT INTO deferrable_constraints VALUES (1)'); + $connection->commit(); + + $this->expectUniqueConstraintViolation(); + }); + } + + public function testTransactionalWithNonDeferredConstraintAndTransactionNesting(): void + { + if (! $this->connection->getDatabasePlatform()->supportsSavepoints()) { + self::markTestSkipped('This test requires the platform to support savepoints.'); + } + + $this->connection->setNestTransactionsWithSavepoints(true); + + $this->connection->transactional(function (Connection $connection): void { + $connection->beginTransaction(); + + try { + $this->connection->executeStatement('INSERT INTO deferrable_constraints VALUES (1)'); + } catch (Throwable $t) { + $this->connection->rollBack(); + + $this->expectUniqueConstraintViolation(); + + throw $t; + } + }); + } + + public function testCommitWithDeferredConstraint(): void + { + $this->skipIfDeferrableIsNotSupported(); + + $this->connection->beginTransaction(); + $this->connection->executeStatement(sprintf('SET CONSTRAINTS "%s" DEFERRED', $this->constraintName)); + $this->connection->executeStatement('INSERT INTO deferrable_constraints VALUES (1)'); + + $this->expectUniqueConstraintViolation(); + $this->connection->commit(); + } + + public function testInsertWithNonDeferredConstraint(): void + { + $this->connection->beginTransaction(); + + try { + $this->connection->executeStatement('INSERT INTO deferrable_constraints VALUES (1)'); + } catch (Throwable $t) { + $this->connection->rollBack(); + + $this->expectUniqueConstraintViolation(); + + throw $t; + } + } + + public function testCommitWithDeferredConstraintAndTransactionNesting(): void + { + if (! $this->connection->getDatabasePlatform()->supportsSavepoints()) { + self::markTestSkipped('This test requires the platform to support savepoints.'); + } + + $this->skipIfDeferrableIsNotSupported(); + + $this->connection->setNestTransactionsWithSavepoints(true); + + $this->connection->beginTransaction(); + $this->connection->executeStatement(sprintf('SET CONSTRAINTS "%s" DEFERRED', $this->constraintName)); + $this->connection->beginTransaction(); + $this->connection->executeStatement('INSERT INTO deferrable_constraints VALUES (1)'); + $this->connection->commit(); + + $this->expectUniqueConstraintViolation(); + + $this->connection->commit(); + } + + public function testCommitWithNonDeferredConstraintAndTransactionNesting(): void + { + if (! $this->connection->getDatabasePlatform()->supportsSavepoints()) { + self::markTestSkipped('This test requires the platform to support savepoints.'); + } + + $this->skipIfDeferrableIsNotSupported(); + + $this->connection->setNestTransactionsWithSavepoints(true); + + $this->connection->beginTransaction(); + $this->connection->beginTransaction(); + + try { + $this->connection->executeStatement('INSERT INTO deferrable_constraints VALUES (1)'); + } catch (Throwable $t) { + $this->connection->rollBack(); + + $this->expectUniqueConstraintViolation(); + + 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(): 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; + } + + $this->expectException(UniqueConstraintViolationException::class); + } + + protected function tearDown(): void + { + $schemaManager = $this->connection->createSchemaManager(); + $schemaManager->dropTable('deferrable_constraints'); + + $this->markConnectionNotReusable(); + + parent::tearDown(); + } +} diff --git a/tests/Functional/TransactionTest.php b/tests/Functional/TransactionTest.php index 6c8f86ca5b0..14fe5cb28c5 100644 --- a/tests/Functional/TransactionTest.php +++ b/tests/Functional/TransactionTest.php @@ -2,9 +2,9 @@ namespace Doctrine\DBAL\Tests\Functional; +use Doctrine\DBAL\Exception\ConnectionLost; use Doctrine\DBAL\Platforms\MySQLPlatform; use Doctrine\DBAL\Tests\FunctionalTestCase; -use PDOException; use function sleep; @@ -29,7 +29,7 @@ public function testCommitFalse(): void try { self::assertFalse(@$this->connection->commit()); // we will ignore `MySQL server has gone away` warnings - } catch (PDOException $e) { + } catch (ConnectionLost $e) { /* For PDO, we are using ERRMODE EXCEPTION, so this catch should be * necessary as the equivalent of the error control operator above. * This seems to be the case only since PHP 8 */