Fix incorrect handling of transactions using deferred constraints

- 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 <[email protected]>

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.
  *

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

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

src/Driver/PDO/Exception.php

@@ -7,15 +7,22 @@
 use Doctrine\DBAL\Driver\AbstractException;
 use PDOException;
 
+use function explode;
+use function str_replace;
+
 /**
  * @internal
  *
  * @psalm-immutable
  */
 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);
  }
 }

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;

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(),
+ );
+ }
 }

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