rdbms: make Database query error handling more strict
authorAaron Schulz <aschulz@wikimedia.org>
Fri, 23 Mar 2018 09:57:21 +0000 (02:57 -0700)
committerAaron Schulz <aschulz@wikimedia.org>
Thu, 5 Apr 2018 04:26:11 +0000 (21:26 -0700)
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
includes/libs/rdbms/database/Database.php
includes/libs/rdbms/database/DatabaseMssql.php
includes/libs/rdbms/database/DatabaseMysqlBase.php
includes/libs/rdbms/database/DatabasePostgres.php
includes/libs/rdbms/database/DatabaseSqlite.php
includes/libs/rdbms/exception/DBError.php
includes/libs/rdbms/exception/DBExpectedError.php
includes/libs/rdbms/exception/DBTransactionStateError.php [new file with mode: 0644]
tests/phpunit/includes/db/DatabaseTestHelper.php
tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php

index eed1c95..b4596c4 100644 (file)
@@ -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',
index dde26c7..5bffcea 100644 (file)
@@ -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(
index 773e548..4c187f2 100644 (file)
@@ -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
         */
index b079eb0..6beea17 100644 (file)
@@ -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
index e3dd3d0..9ffcc8b 100644 (file)
@@ -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__
        ) {
index d5a7489..601a62f 100644 (file)
@@ -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
         */
index 5023800..aad219d 100644 (file)
@@ -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 );
        }
 }
 
index 406d82c..7e46420 100644 (file)
@@ -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 (file)
index 0000000..3e21848
--- /dev/null
@@ -0,0 +1,28 @@
+<?php
+/**
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+ * http://www.gnu.org/copyleft/gpl.html
+ *
+ * @file
+ * @ingroup Database
+ */
+
+namespace Wikimedia\Rdbms;
+
+/**
+ * @ingroup Database
+ */
+class DBTransactionStateError extends DBTransactionError {
+}
index 8950152..854479d 100644 (file)
@@ -49,6 +49,7 @@ class DatabaseTestHelper extends Database {
                        wfWarn( get_class( $e ) . ": {$e->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 ) {
index 981c407..5a06cc7 100644 (file)
@@ -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() );
+       }
 }