From 395462b7d5e1384f8c8c8df4c1ef7ec6e9fdc573 Mon Sep 17 00:00:00 2001 From: Brad Jorsch Date: Thu, 5 Apr 2018 14:16:03 -0400 Subject: [PATCH] rdbms: Roll back empty implicit transaction on error If we're not going to set trxStatus to an error state in this case, we need to issue a rollback to be sure the database (i.e. PostgreSQL) isn't still in an error state too. Bug: T189999 Change-Id: Id6e203b216fff937b6a97d779b36c278e3366409 --- includes/libs/rdbms/database/Database.php | 4 +- .../includes/db/DatabaseTestHelper.php | 44 +++++++++++++++++-- .../libs/rdbms/database/DatabaseSQLTest.php | 39 ++++++++++++++++ 3 files changed, 82 insertions(+), 5 deletions(-) diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php index b3957117dc..53810da4a8 100644 --- a/includes/libs/rdbms/database/Database.php +++ b/includes/libs/rdbms/database/Database.php @@ -1155,7 +1155,9 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware $tempIgnore = false; // cannot recover } } else { - # Nothing prior was there to lose from the transaction + # Nothing prior was there to lose from the transaction, + # so just roll it back. + $this->doRollback( __METHOD__ . " ($fname)" ); $this->trxStatus = self::STATUS_TRX_OK; } } diff --git a/tests/phpunit/includes/db/DatabaseTestHelper.php b/tests/phpunit/includes/db/DatabaseTestHelper.php index 854479dd32..36254f7d9c 100644 --- a/tests/phpunit/includes/db/DatabaseTestHelper.php +++ b/tests/phpunit/includes/db/DatabaseTestHelper.php @@ -26,6 +26,11 @@ class DatabaseTestHelper extends Database { /** @var array List of row arrays */ protected $nextResult = []; + /** @var array|null */ + protected $nextError = null; + /** @var array|null */ + protected $lastError = null; + /** * Array of tables to be considered as existing by tableExist() * Use setExistingTables() to alter. @@ -75,6 +80,16 @@ class DatabaseTestHelper extends Database { $this->nextResult = $res; } + /** + * @param int $errno Error number + * @param string $error Error text + * @param array $options + * - wasKnownStatementRollbackError: Return value for wasKnownStatementRollbackError() + */ + public function forceNextQueryError( $errno, $error, $options = [] ) { + $this->nextError = [ 'errno' => $errno, 'error' => $error ] + $options; + } + protected function addSql( $sql ) { // clean up spaces before and after some words and the whole string $this->lastSqls[] = trim( preg_replace( @@ -88,7 +103,13 @@ class DatabaseTestHelper extends Database { return; // no $fname parameter } - if ( substr( $fname, 0, strlen( $this->testName ) ) !== $this->testName ) { + // Handle some internal calls from the Database class + $check = $fname; + if ( preg_match( '/^Wikimedia\\\\Rdbms\\\\Database::query \((.+)\)$/', $fname, $m ) ) { + $check = $m[1]; + } + + if ( substr( $check, 0, strlen( $this->testName ) ) !== $this->testName ) { throw new MWException( 'function name does not start with test class. ' . $fname . ' vs. ' . $this->testName . '. ' . 'Please provide __METHOD__ to database methods.' ); @@ -107,7 +128,6 @@ class DatabaseTestHelper extends Database { public function query( $sql, $fname = '', $tempIgnore = false ) { $this->checkFunctionName( $fname ); - $this->addSql( $sql ); return parent::query( $sql, $fname, $tempIgnore ); } @@ -167,11 +187,17 @@ class DatabaseTestHelper extends Database { } function lastErrno() { - return -1; + return $this->lastError ? $this->lastError['errno'] : -1; } function lastError() { - return 'test'; + return $this->lastError ? $this->lastError['error'] : 'test'; + } + + protected function wasKnownStatementRollbackError() { + return isset( $this->lastError['wasKnownStatementRollbackError'] ) + ? $this->lastError['wasKnownStatementRollbackError'] + : false; } function fieldInfo( $table, $field ) { @@ -212,8 +238,18 @@ class DatabaseTestHelper extends Database { } protected function doQuery( $sql ) { + $sql = preg_replace( '< /\* .+? \*/>', '', $sql ); + $this->addSql( $sql ); + + if ( $this->nextError ) { + $this->lastError = $this->nextError; + $this->nextError = null; + return false; + } + $res = $this->nextResult; $this->nextResult = []; + $this->lastError = null; return new FakeResultWrapper( $res ); } diff --git a/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php b/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php index 5a06cc769f..40e07d8f3c 100644 --- a/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php +++ b/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php @@ -4,6 +4,7 @@ use Wikimedia\Rdbms\IDatabase; use Wikimedia\Rdbms\LikeMatch; use Wikimedia\Rdbms\Database; use Wikimedia\TestingAccessWrapper; +use Wikimedia\Rdbms\DBTransactionStateError; use Wikimedia\Rdbms\DBUnexpectedError; /** @@ -1540,6 +1541,44 @@ class DatabaseSQLTest extends PHPUnit\Framework\TestCase { $this->assertEquals( 0, $this->database->trxLevel() ); } + /** + * @covers \Wikimedia\Rdbms\Database::query + */ + public function testImplicitTransactionRollback() { + $doError = function ( $wasKnown = true ) { + $this->database->forceNextQueryError( 666, 'Evilness' ); + try { + $this->database->delete( 'error', '1', __CLASS__ . '::SomeCaller' ); + $this->fail( 'Expected exception not thrown' ); + } catch ( DBError $e ) { + $this->assertSame( 666, $e->errno ); + } + }; + + $this->database->setFlag( Database::DBO_TRX ); + + // Implicit transaction gets silently rolled back + $this->database->begin( __METHOD__, Database::TRANSACTION_INTERNAL ); + call_user_func( $doError, false ); + $this->database->delete( 'x', [ 'field' => 1 ], __METHOD__ ); + $this->database->commit( __METHOD__, Database::FLUSHING_INTERNAL ); + // phpcs:ignore + $this->assertLastSql( 'BEGIN; DELETE FROM error WHERE 1; ROLLBACK; BEGIN; DELETE FROM x WHERE field = \'1\'; COMMIT' ); + + // ... unless there were prior writes + $this->database->begin( __METHOD__, Database::TRANSACTION_INTERNAL ); + $this->database->delete( 'x', [ 'field' => 1 ], __METHOD__ ); + call_user_func( $doError, false ); + try { + $this->database->delete( 'x', [ 'field' => 1 ], __METHOD__ ); + $this->fail( 'Expected exception not thrown' ); + } catch ( DBTransactionStateError $e ) { + } + $this->database->rollback( __METHOD__, Database::FLUSHING_INTERNAL ); + // phpcs:ignore + $this->assertLastSql( 'BEGIN; DELETE FROM x WHERE field = \'1\'; DELETE FROM error WHERE 1; ROLLBACK' ); + } + /** * @covers \Wikimedia\Rdbms\Database::close */ -- 2.20.1