From 7ac4705669dc498c8fe74bd230fd4564869b66ca Mon Sep 17 00:00:00 2001 From: Simon Podlipsky Date: Wed, 21 Sep 2022 10:19:03 +0200 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 | 70 +++-- src/Driver/OCI8/Connection.php | 2 +- src/Driver/OCI8/Exception/Error.php | 18 +- src/Driver/PDO/Exception.php | 18 +- src/Driver/PDO/PDOException.php | 25 +- tests/Driver/PDO/ExceptionTest.php | 21 ++ tests/Functional/TransactionTest.php | 5 +- .../UniqueConstraintViolationsTest.php | 257 ++++++++++++++++++ 8 files changed, 379 insertions(+), 37 deletions(-) create mode 100644 tests/Functional/UniqueConstraintViolationsTest.php diff --git a/src/Connection.php b/src/Connection.php index 1fb8cdbff45..50f54505dde 100644 --- a/src/Connection.php +++ b/src/Connection.php @@ -1275,14 +1275,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; } /** @@ -1418,33 +1419,16 @@ public function commit() $connection = $this->getWrappedConnection(); - if ($this->transactionNestingLevel === 1) { - $result = $this->doCommit($connection); - } elseif ($this->nestTransactionsWithSavepoints) { - $this->releaseSavepoint($this->_getNestedTransactionSavePointName()); - } - - --$this->transactionNestingLevel; - - $eventManager = $this->getEventManager(); - - if ($eventManager->hasListeners(Events::onTransactionCommit)) { - Deprecation::trigger( - 'doctrine/dbal', - 'https://github.com/doctrine/dbal/issues/5784', - 'Subscribing to %s events is deprecated.', - Events::onTransactionCommit, - ); - - $eventManager->dispatchEvent(Events::onTransactionCommit, new TransactionCommitEventArgs($this)); - } - - if ($this->autoCommit !== false || $this->transactionNestingLevel !== 0) { - return $result; + try { + if ($this->transactionNestingLevel === 1) { + $result = $this->doCommit($connection); + } elseif ($this->nestTransactionsWithSavepoints) { + $this->releaseSavepoint($this->_getNestedTransactionSavePointName()); + } + } finally { + $this->updateTransactionStateAfterCommit(); } - $this->beginTransaction(); - return $result; } @@ -1461,7 +1445,11 @@ private function doCommit(DriverConnection $connection) $logger->startQuery('"COMMIT"'); } - $result = $connection->commit(); + try { + $result = $connection->commit(); + } catch (Driver\Exception $e) { + throw $this->convertExceptionDuringQuery($e, 'COMMIT'); + } if ($logger !== null) { $logger->stopQuery(); @@ -1470,6 +1458,30 @@ private function doCommit(DriverConnection $connection) return $result; } + private function updateTransactionStateAfterCommit(): void + { + --$this->transactionNestingLevel; + + $eventManager = $this->getEventManager(); + + if ($eventManager->hasListeners(Events::onTransactionCommit)) { + Deprecation::trigger( + 'doctrine/dbal', + 'https://github.com/doctrine/dbal/issues/5784', + 'Subscribing to %s events is deprecated.', + Events::onTransactionCommit, + ); + + $eventManager->dispatchEvent(Events::onTransactionCommit, new TransactionCommitEventArgs($this)); + } + + if ($this->autoCommit !== false || $this->transactionNestingLevel !== 0) { + return; + } + + $this->beginTransaction(); + } + /** * Commits all current nesting transactions. * diff --git a/src/Driver/OCI8/Connection.php b/src/Driver/OCI8/Connection.php index 72353fa31c1..155b2d1d3f8 100644 --- a/src/Driver/OCI8/Connection.php +++ b/src/Driver/OCI8/Connection.php @@ -142,7 +142,7 @@ public function beginTransaction(): bool public function commit(): bool { - if (! oci_commit($this->connection)) { + if (! @oci_commit($this->connection)) { throw Error::new($this->connection); } diff --git a/src/Driver/OCI8/Exception/Error.php b/src/Driver/OCI8/Exception/Error.php index 6abdf233c11..29ab677e02b 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,12 +18,26 @@ */ final class Error extends AbstractException { + private const CODE_TRANSACTION_ROLLED_BACK = 2091; + /** @param resource $resource */ 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/Exception.php b/src/Driver/PDO/Exception.php index fbb81253bf2..faac1a1176d 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; @@ -25,6 +32,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/src/Driver/PDO/PDOException.php b/src/Driver/PDO/PDOException.php index 6eefda40ad6..3343b562ecc 100644 --- a/src/Driver/PDO/PDOException.php +++ b/src/Driver/PDO/PDOException.php @@ -6,6 +6,11 @@ use Doctrine\DBAL\Driver\Exception as DriverException; +use function explode; +use function is_numeric; +use function str_replace; +use function strpos; + /** * @internal * @@ -17,10 +22,26 @@ final class PDOException extends \PDOException implements DriverException public static function new(\PDOException $previous): self { - $exception = new self($previous->message, 0, $previous); + if (isset($previous->errorInfo[2]) && strpos($previous->errorInfo[2], 'OCITransCommit: ORA-02091') === 0) { + // With pdo_oci driver, the root-cause error is in the second line + //ORA-02091: transaction rolled back + //ORA-00001: unique constraint (DOCTRINE.GH3423_UNIQUE) violated + [$firstMessage, $secondMessage] = explode("\n", $previous->message, 2); + + [$code, $message] = explode(': ', $secondMessage, 2); + $code = (int) str_replace('ORA-', '', $code); + } else { + $message = $previous->message; + if (is_numeric($previous->code)) { + $code = (int) $previous->code; + } else { + $code = $previous->errorInfo[1] ?? 0; + } + } + + $exception = new self($message, $code, $previous); $exception->errorInfo = $previous->errorInfo; - $exception->code = $previous->code; $exception->sqlState = $previous->errorInfo[0] ?? null; return $exception; diff --git a/tests/Driver/PDO/ExceptionTest.php b/tests/Driver/PDO/ExceptionTest.php index 13b025c5650..f1190215c64 100644 --- a/tests/Driver/PDO/ExceptionTest.php +++ b/tests/Driver/PDO/ExceptionTest.php @@ -53,4 +53,25 @@ public function testOriginalExceptionIsInChain(): void { self::assertSame($this->wrappedException, $this->exception->getPrevious()); } + + public function testExposesUnderlyingErrorOnOracle(): void + { + $pdoException = new PDOException(<<<'TEXT' +OCITransCommit: ORA-02091: transaction rolled back +ORA-00001: unique constraint (DOCTRINE.C1_UNIQUE) violated + (/private/tmp/php-20211003-35441-1sggrmq/php-8.0.11/ext/pdo_oci/oci_driver.c:410) +TEXT); + + $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/TransactionTest.php b/tests/Functional/TransactionTest.php index 1421bd10a8a..1dab0bda950 100644 --- a/tests/Functional/TransactionTest.php +++ b/tests/Functional/TransactionTest.php @@ -5,7 +5,6 @@ use Doctrine\DBAL\Driver\Exception as DriverException; use Doctrine\DBAL\Platforms\AbstractMySQLPlatform; use Doctrine\DBAL\Tests\FunctionalTestCase; -use PDOException; use function sleep; @@ -29,8 +28,8 @@ public function testCommitFalse(): void sleep(2); // during the sleep mysql will close the connection try { - self::assertFalse(@$this->connection->commit()); // we will ignore `MySQL server has gone away` warnings - } catch (PDOException $e) { + @$this->connection->commit(); // we will ignore `Packets out of order.` error + } catch (\Doctrine\DBAL\Exception\DriverException $e) { self::assertInstanceOf(DriverException::class, $e); /* For PDO, we are using ERRMODE EXCEPTION, so this catch should be diff --git a/tests/Functional/UniqueConstraintViolationsTest.php b/tests/Functional/UniqueConstraintViolationsTest.php new file mode 100644 index 00000000000..8cbba6d3683 --- /dev/null +++ b/tests/Functional/UniqueConstraintViolationsTest.php @@ -0,0 +1,257 @@ +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(); + }); + } + + public function testTransactionalViolatesConstraint(): void + { + $this->connection->transactional(function (Connection $connection): void { + $this->expectUniqueConstraintViolation(); + $connection->executeStatement('INSERT INTO unique_constraint_violations VALUES (1)'); + }); + } + + public function testTransactionalViolatesDeferredConstraintWhileUsingTransactionNesting(): 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 unique_constraint_violations VALUES (1)'); + $connection->commit(); + + $this->expectUniqueConstraintViolation(); + }); + } + + public function testTransactionalViolatesConstraintWhileUsingTransactionNesting(): 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 unique_constraint_violations VALUES (1)'); + } catch (Throwable $t) { + $this->connection->rollBack(); + + $this->expectUniqueConstraintViolation(); + + throw $t; + } + }); + } + + 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(); + $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(); + + throw $t; + } + } + + public function testCommitViolatesDeferredConstraintWhileUsingTransactionNesting(): 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 unique_constraint_violations VALUES (1)'); + $this->connection->commit(); + + $this->expectUniqueConstraintViolation(); + + $this->connection->commit(); + } + + public function testCommitViolatesConstraintWhileUsingTransactionNesting(): 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 unique_constraint_violations 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('unique_constraint_violations'); + + $this->markConnectionNotReusable(); + + parent::tearDown(); + } +}