From 3975e04cf4d14d92bdb95d144f7258205df4efb5 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Fri, 23 Mar 2018 02:57:21 -0700 Subject: [PATCH] rdbms: make Database query error handling more strict Handle all errors in query() that might have caused rollback by putting the Database handle into an error state that can only be resolved by cancelAtomic() or rollback(). Other queries will be rejected until then. This results in more immediate exceptions in some cases where atomic section mismatch errors would have been thrown, such as a an error bubbling up from a child atomic section. Most cases were a try/catch block assumes that only the statement was rolled back now result in an error and rollback. Callers using try/catch to handle key conflicts should instead use SELECT FOR UPDATE to find conflicts beforehand, or use IGNORE, or the upsert()/replace() methods. The try/catch pattern is unsafe and no longer allowed, except for some common errors known to just rollback the statement. Even then, such statements can come from child atomic sections, so committing would be unsafe. Luckily, in such cases, there will be a mismatch detected on endAtomic() or a dangling section detected in close(), resulting in rollback. Remove caching from DatabaseMyslBase::getServerVariableSettings in case some SET query changes the values. Bug: T189999 Change-Id: I532bc5201681a915d0c8aa7a3b1c143b040b142e --- autoload.php | 1 + includes/libs/rdbms/database/Database.php | 211 +++++++++++++----- .../libs/rdbms/database/DatabaseMssql.php | 22 ++ .../libs/rdbms/database/DatabaseMysqlBase.php | 20 ++ .../libs/rdbms/database/DatabasePostgres.php | 23 +- .../libs/rdbms/database/DatabaseSqlite.php | 8 + includes/libs/rdbms/exception/DBError.php | 5 +- .../libs/rdbms/exception/DBExpectedError.php | 12 +- .../exception/DBTransactionStateError.php | 28 +++ .../includes/db/DatabaseTestHelper.php | 13 +- .../libs/rdbms/database/DatabaseSQLTest.php | 101 +++++++++ 11 files changed, 365 insertions(+), 79 deletions(-) create mode 100644 includes/libs/rdbms/exception/DBTransactionStateError.php diff --git a/autoload.php b/autoload.php index eed1c95c1f..b4596c40a8 100644 --- a/autoload.php +++ b/autoload.php @@ -1690,6 +1690,7 @@ $wgAutoloadLocalClasses = [ 'Wikimedia\\Rdbms\\DBReplicationWaitError' => __DIR__ . '/includes/libs/rdbms/exception/DBReplicationWaitError.php', 'Wikimedia\\Rdbms\\DBTransactionError' => __DIR__ . '/includes/libs/rdbms/exception/DBTransactionError.php', 'Wikimedia\\Rdbms\\DBTransactionSizeError' => __DIR__ . '/includes/libs/rdbms/exception/DBTransactionSizeError.php', + 'Wikimedia\\Rdbms\\DBTransactionStateError' => __DIR__ . '/includes/libs/rdbms/exception/DBTransactionStateError.php', 'Wikimedia\\Rdbms\\DBUnexpectedError' => __DIR__ . '/includes/libs/rdbms/exception/DBUnexpectedError.php', 'Wikimedia\\Rdbms\\Database' => __DIR__ . '/includes/libs/rdbms/database/Database.php', 'Wikimedia\\Rdbms\\DatabaseDomain' => __DIR__ . '/includes/libs/rdbms/database/DatabaseDomain.php', diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php index dde26c78ac..5bffceafed 100644 --- a/includes/libs/rdbms/database/Database.php +++ b/includes/libs/rdbms/database/Database.php @@ -141,6 +141,14 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware /** @var integer|null Rows affected by the last query to query() or its CRUD wrappers */ protected $affectedRowCount; + /** + * @var int Transaction status + */ + protected $trxStatus = self::STATUS_TRX_NONE; + /** + * @var Exception|null The last error that caused the status to become STATUS_TRX_ERROR + */ + protected $trxStatusCause; /** * Either 1 if a transaction is active or 0 otherwise. * The other Trx fields may not be meaningfull if this is 0. @@ -259,6 +267,13 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware /** @var int */ protected $nonNativeInsertSelectBatchSize = 10000; + /** @var int Transaction is in a error state requiring a full or savepoint rollback */ + const STATUS_TRX_ERROR = 1; + /** @var int Transaction is active and in a normal state */ + const STATUS_TRX_OK = 2; + /** @var int No transaction is active */ + const STATUS_TRX_NONE = 3; + /** * @note: exceptions for missing libraries/drivers should be thrown in initConnection() * @param array $params Parameters passed from Database::factory() @@ -548,6 +563,14 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware return $this->trxLevel ? $this->trxTimestamp : null; } + /** + * @return int One of the STATUS_TRX_* class constants + * @since 1.31 + */ + public function trxStatus() { + return $this->trxStatus; + } + public function tablePrefix( $prefix = null ) { $old = $this->tablePrefix; if ( $prefix !== null ) { @@ -690,6 +713,15 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware return $fnames; } + /** + * @return string + */ + private function flatAtomicSectionList() { + return array_reduce( $this->trxAtomicLevels, function ( $accum, $v ) { + return $accum === null ? $v[0] : "$accum, " . $v[0]; + } ); + } + public function isOpen() { return $this->opened; } @@ -832,42 +864,64 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware ); } - public function close() { + final public function close() { + $exception = null; // error to throw after disconnecting + if ( $this->conn ) { // Resolve any dangling transaction first - if ( $this->trxLevel() ) { + if ( $this->trxLevel ) { // Meaningful transactions should ideally have been resolved by now if ( $this->writesOrCallbacksPending() ) { $this->queryLogger->warning( __METHOD__ . ": writes or callbacks still pending.", [ 'trace' => ( new RuntimeException() )->getTraceAsString() ] ); + // Cannot let incomplete atomic sections be committed + if ( $this->trxAtomicLevels ) { + $levels = $this->flatAtomicSectionList(); + $exception = new DBUnexpectedError( + $this, + __METHOD__ . ": atomic sections $levels are still open." + ); + // Check if it is possible to properly commit and trigger callbacks + } elseif ( $this->trxEndCallbacksSuppressed ) { + $exception = new DBUnexpectedError( + $this, + __METHOD__ . ': callbacks are suppressed; cannot properly commit.' + ); + } } - // Check if it is possible to properly commit and trigger callbacks - if ( $this->trxEndCallbacksSuppressed ) { - throw new DBUnexpectedError( - $this, - __METHOD__ . ': callbacks are suppressed; cannot properly commit.' - ); + // Commit or rollback the changes and run any callbacks as needed + if ( $this->trxStatus === self::STATUS_TRX_OK && !$exception ) { + $this->commit( __METHOD__, self::TRANSACTION_INTERNAL ); + } else { + $this->rollback( __METHOD__, self::TRANSACTION_INTERNAL ); } - // Commit the changes and run any callbacks as needed - $this->commit( __METHOD__, self::FLUSHING_INTERNAL ); } // Close the actual connection in the binding handle $closed = $this->closeConnection(); $this->conn = false; - // Sanity check that no callbacks are dangling - if ( - $this->trxIdleCallbacks || $this->trxPreCommitCallbacks || $this->trxEndCallbacks - ) { - throw new RuntimeException( "Transaction callbacks still pending." ); - } } else { $closed = true; // already closed; nothing to do } $this->opened = false; + // Throw any unexpected errors after having disconnected + if ( $exception instanceof Exception ) { + throw $exception; + } + + // Sanity check that no callbacks are dangling + if ( + $this->trxIdleCallbacks || $this->trxPreCommitCallbacks || $this->trxEndCallbacks + ) { + throw new RuntimeException( + "Transaction callbacks are still pending:\n" . + implode( ', ', $this->pendingWriteAndCallbackCallers() ) + ); + } + return $closed; } @@ -991,6 +1045,8 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware } public function query( $sql, $fname = __METHOD__, $tempIgnore = false ) { + $this->assertTransactionStatus( $sql, $fname ); + $priorWritesPending = $this->writesOrCallbacksPending(); $this->lastQuery = $sql; @@ -1069,20 +1125,25 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware } if ( $ret === false ) { - # Deadlocks cause the entire transaction to abort, not just the statement. - # https://dev.mysql.com/doc/refman/5.7/en/innodb-error-handling.html - # https://www.postgresql.org/docs/9.1/static/explicit-locking.html - if ( $this->wasDeadlock() ) { + if ( $this->trxLevel && !$this->wasKnownStatementRollbackError() ) { + # Either the query was aborted or all queries after BEGIN where aborted. if ( $this->explicitTrxActive() || $priorWritesPending ) { - $tempIgnore = false; // not recoverable + # In the first case, the only options going forward are (a) ROLLBACK, or + # (b) ROLLBACK TO SAVEPOINT (if one was set). If the later case, the only + # option is ROLLBACK, since the snapshots would have been released. + if ( is_object( $tempIgnore ) ) { + // Ugly hack to know that savepoints are in use for postgres + // FIXME: remove this and make DatabasePostgres use ATOMIC_CANCELABLE + } else { + $this->trxStatus = self::STATUS_TRX_ERROR; + $this->trxStatusCause = + $this->makeQueryException( $lastError, $lastErrno, $sql, $fname ); + $tempIgnore = false; // cannot recover + } + } else { + # Nothing prior was there to lose from the transaction + $this->trxStatus = self::STATUS_TRX_OK; } - # Usually the transaction is rolled back to BEGIN, leaving an empty transaction. - # Destroy any such transaction so the rollback callbacks run in AUTO-COMMIT mode - # as normal. Also, if DBO_TRX is set and an explicit transaction rolled back here, - # further queries should be back in AUTO-COMMIT mode, not stuck in a transaction. - $this->doRollback( __METHOD__ ); - # Update state tracking to reflect transaction loss - $this->handleTransactionLoss(); } $this->reportQueryError( $lastError, $lastErrno, $sql, $fname, $tempIgnore ); @@ -1186,6 +1247,25 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware } } + /** + * @param string $sql + * @param string $fname + * @throws DBTransactionStateError + */ + private function assertTransactionStatus( $sql, $fname ) { + if ( + $this->trxStatus < self::STATUS_TRX_OK && + $this->getQueryVerb( $sql ) !== 'ROLLBACK' // transaction/savepoint + ) { + throw new DBTransactionStateError( + $this, + "Cannot execute query from $fname while transaction status is ERROR. ", + [], + $this->trxStatusCause + ); + } + } + /** * Determine whether or not it is safe to retry queries after a database * connection is lost @@ -1210,7 +1290,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware } elseif ( $sql === 'ROLLBACK' ) { return true; // transaction lost...which is also what was requested :) } elseif ( $this->explicitTrxActive() ) { - return false; // don't drop atomocity + return false; // don't drop atomocity and explicit snapshots } elseif ( $priorWritesPending ) { return false; // prior writes lost from implicit transaction } @@ -1285,27 +1365,42 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware if ( $tempIgnore ) { $this->queryLogger->debug( "SQL ERROR (ignored): $error\n" ); } else { - $sql1line = mb_substr( str_replace( "\n", "\\n", $sql ), 0, 5 * 1024 ); - $this->queryLogger->error( - "{fname}\t{db_server}\t{errno}\t{error}\t{sql1line}", - $this->getLogContext( [ - 'method' => __METHOD__, - 'errno' => $errno, - 'error' => $error, - 'sql1line' => $sql1line, - 'fname' => $fname, - ] ) - ); - $this->queryLogger->debug( "SQL ERROR: " . $error . "\n" ); - $wasQueryTimeout = $this->wasQueryTimeout( $error, $errno ); - if ( $wasQueryTimeout ) { - throw new DBQueryTimeoutError( $this, $error, $errno, $sql, $fname ); - } else { - throw new DBQueryError( $this, $error, $errno, $sql, $fname ); - } + $exception = $this->makeQueryException( $error, $errno, $sql, $fname ); + + throw $exception; } } + /** + * @param string $error + * @param string|int $errno + * @param string $sql + * @param string $fname + * @return DBError + */ + private function makeQueryException( $error, $errno, $sql, $fname ) { + $sql1line = mb_substr( str_replace( "\n", "\\n", $sql ), 0, 5 * 1024 ); + $this->queryLogger->error( + "{fname}\t{db_server}\t{errno}\t{error}\t{sql1line}", + $this->getLogContext( [ + 'method' => __METHOD__, + 'errno' => $errno, + 'error' => $error, + 'sql1line' => $sql1line, + 'fname' => $fname, + ] ) + ); + $this->queryLogger->debug( "SQL ERROR: " . $error . "\n" ); + $wasQueryTimeout = $this->wasQueryTimeout( $error, $errno ); + if ( $wasQueryTimeout ) { + $e = new DBQueryTimeoutError( $this, $error, $errno, $sql, $fname ); + } else { + $e = new DBQueryError( $this, $error, $errno, $sql, $fname ); + } + + return $e; + } + public function freeResult( $res ) { } @@ -3012,6 +3107,16 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware return false; } + /** + * @return bool Whether it is safe to assume the given error only caused statement rollback + * @note This is for backwards compatibility for callers catching DBError exceptions in + * order to ignore problems like duplicate key errors or foriegn key violations + * @since 1.31 + */ + protected function wasKnownStatementRollbackError() { + return false; // don't know; it could have caused a transaction rollback + } + public function deadlockLoop() { $args = func_get_args(); $function = array_shift( $args ); @@ -3337,6 +3442,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware $this->rollback( $fname, self::FLUSHING_INTERNAL ); } elseif ( $savepointId !== 'n/a' ) { $this->doRollbackToSavepoint( $savepointId, $fname ); + $this->trxStatus = self::STATUS_TRX_OK; // no exception; recovered } $this->affectedRowCount = 0; // for the sake of consistency @@ -3359,9 +3465,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware // Protect against mismatched atomic section, transaction nesting, and snapshot loss if ( $this->trxLevel ) { if ( $this->trxAtomicLevels ) { - $levels = array_reduce( $this->trxAtomicLevels, function ( $accum, $v ) { - return $accum === null ? $v[0] : "$accum, " . $v[0]; - } ); + $levels = $this->flatAtomicSectionList(); $msg = "$fname: Got explicit BEGIN while atomic section(s) $levels are open."; throw new DBUnexpectedError( $this, $msg ); } elseif ( !$this->trxAutomatic ) { @@ -3380,6 +3484,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware $this->assertOpen(); $this->doBegin( $fname ); + $this->trxStatus = self::STATUS_TRX_OK; $this->trxAtomicCounter = 0; $this->trxTimestamp = microtime( true ); $this->trxFname = $fname; @@ -3416,9 +3521,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware final public function commit( $fname = __METHOD__, $flush = '' ) { if ( $this->trxLevel && $this->trxAtomicLevels ) { // There are still atomic sections open. This cannot be ignored - $levels = array_reduce( $this->trxAtomicLevels, function ( $accum, $v ) { - return $accum === null ? $v[0] : "$accum, " . $v[0]; - } ); + $levels = $this->flatAtomicSectionList(); throw new DBUnexpectedError( $this, "$fname: Got COMMIT while atomic sections $levels are still open." @@ -3453,6 +3556,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware $this->runOnTransactionPreCommitCallbacks(); $writeTime = $this->pendingWriteQueryDuration( self::ESTIMATE_DB_APPLY ); $this->doCommit( $fname ); + $this->trxStatus = self::STATUS_TRX_NONE; if ( $this->trxDoneWrites ) { $this->lastWriteTime = microtime( true ); $this->trxProfiler->transactionWritingOut( @@ -3498,6 +3602,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware $this->assertOpen(); $this->doRollback( $fname ); + $this->trxStatus = self::STATUS_TRX_NONE; $this->trxAtomicLevels = []; if ( $this->trxDoneWrites ) { $this->trxProfiler->transactionWritingOut( diff --git a/includes/libs/rdbms/database/DatabaseMssql.php b/includes/libs/rdbms/database/DatabaseMssql.php index 773e548d87..4c187f2357 100644 --- a/includes/libs/rdbms/database/DatabaseMssql.php +++ b/includes/libs/rdbms/database/DatabaseMssql.php @@ -359,6 +359,28 @@ class DatabaseMssql extends Database { } } + protected function wasKnownStatementRollbackError() { + $errors = sqlsrv_errors( SQLSRV_ERR_ALL ); + if ( !$errors ) { + return false; + } + // The transaction vs statement rollback behavior depends on XACT_ABORT, so make sure + // that the "statement has been terminated" error (3621) is specifically present. + // https://docs.microsoft.com/en-us/sql/t-sql/statements/set-xact-abort-transact-sql + $statementOnly = false; + $codeWhitelist = [ '2601', '2627', '547' ]; + foreach ( $errors as $error ) { + if ( $error['code'] == '3621' ) { + $statementOnly = true; + } elseif ( !in_array( $error['code'], $codeWhitelist ) ) { + $statementOnly = false; + break; + } + } + + return $statementOnly; + } + /** * @return int */ diff --git a/includes/libs/rdbms/database/DatabaseMysqlBase.php b/includes/libs/rdbms/database/DatabaseMysqlBase.php index b079eb0e0d..6beea17d20 100644 --- a/includes/libs/rdbms/database/DatabaseMysqlBase.php +++ b/includes/libs/rdbms/database/DatabaseMysqlBase.php @@ -1331,6 +1331,26 @@ abstract class DatabaseMysqlBase extends Database { return $errno == 2013 || $errno == 2006; } + protected function wasKnownStatementRollbackError() { + $errno = $this->lastErrno(); + + if ( $errno === 1205 ) { // lock wait timeout + // Note that this is uncached to avoid stale values of SET is used + $row = $this->selectRow( + false, + [ 'innodb_rollback_on_timeout' => '@@innodb_rollback_on_timeout' ], + [], + __METHOD__ + ); + // https://dev.mysql.com/doc/refman/5.7/en/innodb-error-handling.html + // https://dev.mysql.com/doc/refman/5.5/en/innodb-parameters.html + return $row->innodb_rollback_on_timeout ? false : true; + } + + // See https://dev.mysql.com/doc/refman/5.5/en/error-messages-server.html + return in_array( $errno, [ 1022, 1216, 1217, 1137 ], true ); + } + /** * @param string $oldName * @param string $newName diff --git a/includes/libs/rdbms/database/DatabasePostgres.php b/includes/libs/rdbms/database/DatabasePostgres.php index e3dd3d00b3..9ffcc8b01e 100644 --- a/includes/libs/rdbms/database/DatabasePostgres.php +++ b/includes/libs/rdbms/database/DatabasePostgres.php @@ -248,25 +248,6 @@ class DatabasePostgres extends Database { } } - public function reportQueryError( $error, $errno, $sql, $fname, $tempIgnore = false ) { - if ( $tempIgnore ) { - /* Check for constraint violation */ - if ( $errno === '23505' ) { - parent::reportQueryError( $error, $errno, $sql, $fname, $tempIgnore ); - - return; - } - } - /* Transaction stays in the ERROR state until rolled back */ - if ( $this->trxLevel ) { - // Throw away the transaction state, then raise the error as normal. - // Note that if this connection is managed by LBFactory, it's already expected - // that the other transactions LBFactory manages will be rolled back. - $this->rollback( __METHOD__, self::FLUSHING_INTERNAL ); - } - parent::reportQueryError( $error, $errno, $sql, $fname, false ); - } - public function freeResult( $res ) { if ( $res instanceof ResultWrapper ) { $res = $res->result; @@ -821,6 +802,10 @@ __INDEXATTR__; return in_array( $errno, $codes, true ); } + protected function wasKnownStatementRollbackError() { + return false; // transaction has to be rolled-back from error state + } + public function duplicateTableStructure( $oldName, $newName, $temporary = false, $fname = __METHOD__ ) { diff --git a/includes/libs/rdbms/database/DatabaseSqlite.php b/includes/libs/rdbms/database/DatabaseSqlite.php index d5a74891be..601a62fef7 100644 --- a/includes/libs/rdbms/database/DatabaseSqlite.php +++ b/includes/libs/rdbms/database/DatabaseSqlite.php @@ -727,6 +727,14 @@ class DatabaseSqlite extends Database { return $errno == 17; // SQLITE_SCHEMA; } + protected function wasKnownStatementRollbackError() { + // ON CONFLICT ROLLBACK clauses make it so that SQLITE_CONSTRAINT error is + // ambiguous with regard to whether it implies a ROLLBACK or an ABORT happened. + // https://sqlite.org/lang_createtable.html#uniqueconst + // https://sqlite.org/lang_conflict.html + return false; + } + /** * @return string Wikitext of a link to the server software's web site */ diff --git a/includes/libs/rdbms/exception/DBError.php b/includes/libs/rdbms/exception/DBError.php index 50238003a2..aad219dd29 100644 --- a/includes/libs/rdbms/exception/DBError.php +++ b/includes/libs/rdbms/exception/DBError.php @@ -35,10 +35,11 @@ class DBError extends RuntimeException { * Construct a database error * @param IDatabase $db Object which threw the error * @param string $error A simple error message to be used for debugging + * @param \Exception|\Throwable|null $prev Previous exception */ - public function __construct( IDatabase $db = null, $error ) { + public function __construct( IDatabase $db = null, $error, $prev = null ) { + parent::__construct( $error, 0, $prev ); $this->db = $db; - parent::__construct( $error ); } } diff --git a/includes/libs/rdbms/exception/DBExpectedError.php b/includes/libs/rdbms/exception/DBExpectedError.php index 406d82c139..7e46420daa 100644 --- a/includes/libs/rdbms/exception/DBExpectedError.php +++ b/includes/libs/rdbms/exception/DBExpectedError.php @@ -33,8 +33,16 @@ class DBExpectedError extends DBError implements MessageSpecifier { /** @var string[] Message parameters */ protected $params; - public function __construct( IDatabase $db = null, $error, array $params = [] ) { - parent::__construct( $db, $error ); + /** + * @param IDatabase|null $db + * @param string $error + * @param array $params + * @param \Exception|\Throwable|null $prev + */ + public function __construct( + IDatabase $db = null, $error, array $params = [], $prev = null + ) { + parent::__construct( $db, $error, $prev ); $this->params = $params; } diff --git a/includes/libs/rdbms/exception/DBTransactionStateError.php b/includes/libs/rdbms/exception/DBTransactionStateError.php new file mode 100644 index 0000000000..3e21848236 --- /dev/null +++ b/includes/libs/rdbms/exception/DBTransactionStateError.php @@ -0,0 +1,28 @@ +getMessage()}" ); }; $this->currentDomain = DatabaseDomain::newUnspecified(); + $this->open( 'localhost', 'testuser', 'password', 'testdb' ); } /** @@ -83,6 +84,10 @@ class DatabaseTestHelper extends Database { } protected function checkFunctionName( $fname ) { + if ( $fname === 'Wikimedia\\Rdbms\\Database::close' ) { + return; // no $fname parameter + } + if ( substr( $fname, 0, strlen( $this->testName ) ) !== $this->testName ) { throw new MWException( 'function name does not start with test class. ' . $fname . ' vs. ' . $this->testName . '. ' . @@ -128,7 +133,9 @@ class DatabaseTestHelper extends Database { } function open( $server, $user, $password, $dbName ) { - return false; + $this->conn = (object)[ 'test' ]; + + return true; } function fetchObject( $res ) { @@ -192,7 +199,7 @@ class DatabaseTestHelper extends Database { } function isOpen() { - return true; + return $this->conn ? true : false; } function ping( &$rtt = null ) { @@ -201,7 +208,7 @@ class DatabaseTestHelper extends Database { } protected function closeConnection() { - return false; + return true; } protected function doQuery( $sql ) { diff --git a/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php b/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php index 981c4075fe..5a06cc769f 100644 --- a/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php +++ b/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php @@ -3,6 +3,8 @@ use Wikimedia\Rdbms\IDatabase; use Wikimedia\Rdbms\LikeMatch; use Wikimedia\Rdbms\Database; +use Wikimedia\TestingAccessWrapper; +use Wikimedia\Rdbms\DBUnexpectedError; /** * Test the parts of the Database abstract class that deal @@ -1481,4 +1483,103 @@ class DatabaseSQLTest extends PHPUnit\Framework\TestCase { } } + /** + * @expectedException \Wikimedia\Rdbms\DBTransactionStateError + */ + public function testTransactionErrorState1() { + $wrapper = TestingAccessWrapper::newFromObject( $this->database ); + + $this->database->begin( __METHOD__ ); + $wrapper->trxStatus = Database::STATUS_TRX_ERROR; + $this->database->delete( 'x', [ 'field' => 3 ], __METHOD__ ); + $this->database->commit( __METHOD__ ); + } + + /** + * @covers \Wikimedia\Rdbms\Database::query + */ + public function testTransactionErrorState2() { + $wrapper = TestingAccessWrapper::newFromObject( $this->database ); + + $this->database->startAtomic( __METHOD__ ); + $wrapper->trxStatus = Database::STATUS_TRX_ERROR; + $this->database->rollback( __METHOD__ ); + $this->assertEquals( 0, $this->database->trxLevel() ); + $this->assertEquals( Database::STATUS_TRX_NONE, $wrapper->trxStatus() ); + $this->assertLastSql( 'BEGIN; ROLLBACK' ); + + $this->database->startAtomic( __METHOD__ ); + $this->assertEquals( Database::STATUS_TRX_OK, $wrapper->trxStatus() ); + $this->database->delete( 'x', [ 'field' => 1 ], __METHOD__ ); + $this->database->endAtomic( __METHOD__ ); + $this->assertEquals( Database::STATUS_TRX_NONE, $wrapper->trxStatus() ); + $this->assertLastSql( 'BEGIN; DELETE FROM x WHERE field = \'1\'; COMMIT' ); + $this->assertEquals( 0, $this->database->trxLevel(), 'Use after rollback()' ); + + $this->database->begin( __METHOD__ ); + $this->database->startAtomic( __METHOD__, Database::ATOMIC_CANCELABLE ); + $this->database->update( 'y', [ 'a' => 1 ], [ 'field' => 1 ], __METHOD__ ); + $wrapper->trxStatus = Database::STATUS_TRX_ERROR; + $this->database->cancelAtomic( __METHOD__ ); + $this->assertEquals( Database::STATUS_TRX_OK, $wrapper->trxStatus() ); + $this->database->startAtomic( __METHOD__ ); + $this->database->delete( 'y', [ 'field' => 1 ], __METHOD__ ); + $this->database->endAtomic( __METHOD__ ); + $this->database->commit( __METHOD__ ); + // phpcs:ignore Generic.Files.LineLength + $this->assertLastSql( 'BEGIN; SAVEPOINT wikimedia_rdbms_atomic1; UPDATE y SET a = \'1\' WHERE field = \'1\'; ROLLBACK TO SAVEPOINT wikimedia_rdbms_atomic1; DELETE FROM y WHERE field = \'1\'; COMMIT' ); + $this->assertEquals( 0, $this->database->trxLevel(), 'Use after rollback()' ); + + // Next transaction + $this->database->startAtomic( __METHOD__ ); + $this->assertEquals( Database::STATUS_TRX_OK, $wrapper->trxStatus() ); + $this->database->delete( 'x', [ 'field' => 3 ], __METHOD__ ); + $this->database->endAtomic( __METHOD__ ); + $this->assertEquals( Database::STATUS_TRX_NONE, $wrapper->trxStatus() ); + $this->assertLastSql( 'BEGIN; DELETE FROM x WHERE field = \'3\'; COMMIT' ); + $this->assertEquals( 0, $this->database->trxLevel() ); + } + + /** + * @covers \Wikimedia\Rdbms\Database::close + */ + public function testPrematureClose1() { + $fname = __METHOD__; + $this->database->begin( __METHOD__ ); + $this->database->onTransactionIdle( function () use ( $fname ) { + $this->database->query( 'SELECT 1', $fname ); + } ); + $this->database->delete( 'x', [ 'field' => 3 ], __METHOD__ ); + $this->database->close(); + + $this->assertFalse( $this->database->isOpen() ); + $this->assertLastSql( 'BEGIN; DELETE FROM x WHERE field = \'3\'; COMMIT; SELECT 1' ); + $this->assertEquals( 0, $this->database->trxLevel() ); + } + + /** + * @covers \Wikimedia\Rdbms\Database::close + */ + public function testPrematureClose2() { + try { + $fname = __METHOD__; + $this->database->startAtomic( __METHOD__ ); + $this->database->onTransactionIdle( function () use ( $fname ) { + $this->database->query( 'SELECT 1', $fname ); + } ); + $this->database->delete( 'x', [ 'field' => 3 ], __METHOD__ ); + $this->database->close(); + $this->fail( 'Expected exception not thrown' ); + } catch ( DBUnexpectedError $ex ) { + $this->assertSame( + 'Wikimedia\Rdbms\Database::close: atomic sections ' . + 'DatabaseSQLTest::testPrematureClose2 are still open.', + $ex->getMessage() + ); + } + + $this->assertFalse( $this->database->isOpen() ); + $this->assertLastSql( 'BEGIN; DELETE FROM x WHERE field = \'3\'; ROLLBACK' ); + $this->assertEquals( 0, $this->database->trxLevel() ); + } } -- 2.20.1