From: Aaron Schulz Date: Fri, 26 Oct 2018 20:17:34 +0000 (-0700) Subject: rdbms: clean up return values of IDatabase write methods X-Git-Tag: 1.34.0-rc.0~3613^2 X-Git-Url: https://git.cyclocoop.org/%28%28?a=commitdiff_plain;h=633eb437a3b808518469c6eaf4e86a436941d837;p=lhc%2Fweb%2Fwiklou.git rdbms: clean up return values of IDatabase write methods Also improved the atomicity and affected row count logic for insert/replace with sqlite. Also remove unused "fileHandle" code from insert(). Change-Id: If7b9148fd44f3a958899885753c7c86ba66bf193 --- diff --git a/includes/db/DatabaseOracle.php b/includes/db/DatabaseOracle.php index 6d921b9724..696e81f50c 100644 --- a/includes/db/DatabaseOracle.php +++ b/includes/db/DatabaseOracle.php @@ -390,13 +390,12 @@ class DatabaseOracle extends Database { foreach ( $a as &$row ) { $this->insertOneRow( $table, $row, $fname ); } - $retVal = true; if ( in_array( 'IGNORE', $options ) ) { $this->ignoreDupValOnIndex = false; } - return $retVal; + return true; } private function fieldBindStatement( $table, $col, &$val, $includeCol = false ) { @@ -580,13 +579,11 @@ class DatabaseOracle extends Database { $this->ignoreDupValOnIndex = true; } - $retval = $this->query( $sql, $fname ); + $this->query( $sql, $fname ); if ( in_array( 'IGNORE', $insertOptions ) ) { $this->ignoreDupValOnIndex = false; } - - return $retval; } public function upsert( $table, array $rows, array $uniqueIndexes, array $set, diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php index 83b9660fd3..2c40c72bc1 100644 --- a/includes/libs/rdbms/database/Database.php +++ b/includes/libs/rdbms/database/Database.php @@ -2038,7 +2038,6 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware $options = [ $options ]; } - $fh = $options['fileHandle'] ?? null; $options = $this->makeInsertOptions( $options ); if ( isset( $a[0] ) && is_array( $a[0] ) ) { @@ -2066,13 +2065,9 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware $sql .= '(' . $this->makeList( $a ) . ')'; } - if ( $fh !== null && false === fwrite( $fh, $sql ) ) { - return false; - } elseif ( $fh !== null ) { - return true; - } + $this->query( $sql, $fname ); - return (bool)$this->query( $sql, $fname ); + return true; } /** @@ -2116,7 +2111,9 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware $sql .= " WHERE " . $this->makeList( $conds, self::LIST_AND ); } - return (bool)$this->query( $sql, $fname ); + $this->query( $sql, $fname ); + + return true; } public function makeList( $a, $mode = self::LIST_COMMA ) { @@ -2830,8 +2827,6 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware * @param string $table Table name * @param array|string $rows Row(s) to insert * @param string $fname Caller function name - * - * @return ResultWrapper */ protected function nativeReplace( $table, $rows, $fname ) { $table = $this->tableName( $table ); @@ -2854,7 +2849,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware $sql .= '(' . $this->makeList( $row ) . ')'; } - return $this->query( $sql, $fname ); + $this->query( $sql, $fname ); } public function upsert( $table, array $rows, array $uniqueIndexes, array $set, @@ -2890,13 +2885,11 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware $this->startAtomic( $fname, self::ATOMIC_CANCELABLE ); # Update any existing conflicting row(s) if ( $where !== false ) { - $ok = $this->update( $table, $set, $where, $fname ); + $this->update( $table, $set, $where, $fname ); $affectedRowCount += $this->affectedRows(); - } else { - $ok = true; } # Now insert any non-conflicting row(s) - $ok = $this->insert( $table, $rows, $fname, [ 'IGNORE' ] ) && $ok; + $this->insert( $table, $rows, $fname, [ 'IGNORE' ] ); $affectedRowCount += $this->affectedRows(); $this->endAtomic( $fname ); $this->affectedRowCount = $affectedRowCount; @@ -2905,7 +2898,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware throw $e; } - return $ok; + return true; } public function deleteJoin( $delTable, $joinTable, $delVar, $joinVar, $conds, @@ -2958,7 +2951,9 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware $sql .= ' WHERE ' . $conds; } - return $this->query( $sql, $fname ); + $this->query( $sql, $fname ); + + return true; } final public function insertSelect( @@ -2973,7 +2968,18 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware if ( $this->cliMode && $this->isInsertSelectSafe( $insertOptions, $selectOptions ) ) { // For massive migrations with downtime, we don't want to select everything // into memory and OOM, so do all this native on the server side if possible. - return $this->nativeInsertSelect( + $this->nativeInsertSelect( + $destTable, + $srcTable, + $varMap, + $conds, + $fname, + array_diff( $insertOptions, $hints ), + $selectOptions, + $selectJoinConds + ); + } else { + $this->nonNativeInsertSelect( $destTable, $srcTable, $varMap, @@ -2985,16 +2991,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware ); } - return $this->nonNativeInsertSelect( - $destTable, - $srcTable, - $varMap, - $conds, - $fname, - array_diff( $insertOptions, $hints ), - $selectOptions, - $selectJoinConds - ); + return true; } /** @@ -3020,7 +3017,6 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware * @param array $insertOptions * @param array $selectOptions * @param array $selectJoinConds - * @return bool */ protected function nonNativeInsertSelect( $destTable, $srcTable, $varMap, $conds, $fname = __METHOD__, @@ -3038,7 +3034,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware $srcTable, implode( ',', $fields ), $conds, $fname, $selectOptions, $selectJoinConds ); if ( !$res ) { - return false; + return; } try { @@ -3071,7 +3067,6 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware } else { $this->cancelAtomic( $fname ); } - return $ok; } catch ( Exception $e ) { $this->cancelAtomic( $fname ); throw $e; @@ -3091,7 +3086,6 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware * @param array $insertOptions * @param array $selectOptions * @param array $selectJoinConds - * @return bool */ protected function nativeInsertSelect( $destTable, $srcTable, $varMap, $conds, $fname = __METHOD__, @@ -3118,7 +3112,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware " INTO $destTable (" . implode( ',', array_keys( $varMap ) ) . ') ' . $selectSql; - return $this->query( $sql, $fname ); + $this->query( $sql, $fname ); } /** diff --git a/includes/libs/rdbms/database/DatabaseMssql.php b/includes/libs/rdbms/database/DatabaseMssql.php index de102a74ba..93bb5d3bfd 100644 --- a/includes/libs/rdbms/database/DatabaseMssql.php +++ b/includes/libs/rdbms/database/DatabaseMssql.php @@ -513,6 +513,8 @@ class DatabaseMssql extends Database { throw $e; } $this->scrollableCursor = true; + + return true; } /** @@ -741,7 +743,7 @@ class DatabaseMssql extends Database { $this->ignoreDupKeyErrors = false; - return $ret; + return true; } /** @@ -757,15 +759,14 @@ class DatabaseMssql extends Database { * @param array $insertOptions * @param array $selectOptions * @param array $selectJoinConds - * @return bool * @throws Exception */ - public function nativeInsertSelect( $destTable, $srcTable, $varMap, $conds, $fname = __METHOD__, + protected function nativeInsertSelect( $destTable, $srcTable, $varMap, $conds, $fname = __METHOD__, $insertOptions = [], $selectOptions = [], $selectJoinConds = [] ) { $this->scrollableCursor = false; try { - $ret = parent::nativeInsertSelect( + parent::nativeInsertSelect( $destTable, $srcTable, $varMap, @@ -780,8 +781,6 @@ class DatabaseMssql extends Database { throw $e; } $this->scrollableCursor = true; - - return $ret; } /** diff --git a/includes/libs/rdbms/database/DatabaseMysqlBase.php b/includes/libs/rdbms/database/DatabaseMysqlBase.php index cc9e98ffa3..eeedff7d07 100644 --- a/includes/libs/rdbms/database/DatabaseMysqlBase.php +++ b/includes/libs/rdbms/database/DatabaseMysqlBase.php @@ -493,10 +493,9 @@ abstract class DatabaseMysqlBase extends Database { * @param array $uniqueIndexes * @param array $rows * @param string $fname - * @return ResultWrapper */ public function replace( $table, $uniqueIndexes, $rows, $fname = __METHOD__ ) { - return $this->nativeReplace( $table, $rows, $fname ); + $this->nativeReplace( $table, $rows, $fname ); } protected function isInsertSelectSafe( array $insertOptions, array $selectOptions ) { @@ -1303,7 +1302,6 @@ abstract class DatabaseMysqlBase extends Database { * @param array|string $conds * @param bool|string $fname * @throws DBUnexpectedError - * @return bool|ResultWrapper */ public function deleteJoin( $delTable, $joinTable, $delVar, $joinVar, $conds, $fname = __METHOD__ @@ -1320,7 +1318,7 @@ abstract class DatabaseMysqlBase extends Database { $sql .= ' AND ' . $this->makeList( $conds, self::LIST_AND ); } - return $this->query( $sql, $fname ); + $this->query( $sql, $fname ); } /** @@ -1353,7 +1351,9 @@ abstract class DatabaseMysqlBase extends Database { $sql .= implode( ',', $rowTuples ); $sql .= " ON DUPLICATE KEY UPDATE " . $this->makeList( $set, self::LIST_SET ); - return (bool)$this->query( $sql, $fname ); + $this->query( $sql, $fname ); + + return true; } /** diff --git a/includes/libs/rdbms/database/DatabasePostgres.php b/includes/libs/rdbms/database/DatabasePostgres.php index e1cd764d9a..adcd899902 100644 --- a/includes/libs/rdbms/database/DatabasePostgres.php +++ b/includes/libs/rdbms/database/DatabasePostgres.php @@ -670,9 +670,8 @@ __INDEXATTR__; * @param array $insertOptions * @param array $selectOptions * @param array $selectJoinConds - * @return bool */ - public function nativeInsertSelect( + protected function nativeInsertSelect( $destTable, $srcTable, $varMap, $conds, $fname = __METHOD__, $insertOptions = [], $selectOptions = [], $selectJoinConds = [] ) { @@ -697,18 +696,18 @@ __INDEXATTR__; $sql = "INSERT INTO $destTable (" . implode( ',', array_keys( $varMap ) ) . ') ' . $selectSql . ' ON CONFLICT DO NOTHING'; - return $this->query( $sql, $fname ); + $this->query( $sql, $fname ); } else { // IGNORE and we don't have ON CONFLICT DO NOTHING, so just use the non-native version - return $this->nonNativeInsertSelect( + $this->nonNativeInsertSelect( $destTable, $srcTable, $varMap, $conds, $fname, $insertOptions, $selectOptions, $selectJoinConds ); } + } else { + parent::nativeInsertSelect( $destTable, $srcTable, $varMap, $conds, $fname, + $insertOptions, $selectOptions, $selectJoinConds ); } - - return parent::nativeInsertSelect( $destTable, $srcTable, $varMap, $conds, $fname, - $insertOptions, $selectOptions, $selectJoinConds ); } public function tableName( $name, $format = 'quoted' ) { diff --git a/includes/libs/rdbms/database/DatabaseSqlite.php b/includes/libs/rdbms/database/DatabaseSqlite.php index 487e1221be..43776b5b37 100644 --- a/includes/libs/rdbms/database/DatabaseSqlite.php +++ b/includes/libs/rdbms/database/DatabaseSqlite.php @@ -25,6 +25,7 @@ namespace Wikimedia\Rdbms; use PDO; use PDOException; +use Exception; use LockManager; use FSLockManager; use InvalidArgumentException; @@ -648,17 +649,23 @@ class DatabaseSqlite extends Database { # SQLite can't handle multi-row inserts, so divide up into multiple single-row inserts if ( isset( $a[0] ) && is_array( $a[0] ) ) { - $ret = true; - foreach ( $a as $v ) { - if ( !parent::insert( $table, $v, "$fname/multi-row", $options ) ) { - $ret = false; + $affectedRowCount = 0; + try { + $this->startAtomic( $fname, self::ATOMIC_CANCELABLE ); + foreach ( $a as $v ) { + parent::insert( $table, $v, "$fname/multi-row", $options ); } + $this->endAtomic( $fname ); + } catch ( Exception $e ) { + $this->cancelAtomic( $fname ); + throw $e; } + $this->affectedRowCount = $affectedRowCount; } else { - $ret = parent::insert( $table, $a, "$fname/single-row", $options ); + parent::insert( $table, $a, "$fname/single-row", $options ); } - return $ret; + return true; } /** @@ -666,26 +673,30 @@ class DatabaseSqlite extends Database { * @param array $uniqueIndexes Unused * @param string|array $rows * @param string $fname - * @return bool|ResultWrapper */ function replace( $table, $uniqueIndexes, $rows, $fname = __METHOD__ ) { if ( !count( $rows ) ) { - return true; + return; } # SQLite can't handle multi-row replaces, so divide up into multiple single-row queries if ( isset( $rows[0] ) && is_array( $rows[0] ) ) { - $ret = true; - foreach ( $rows as $v ) { - if ( !$this->nativeReplace( $table, $v, "$fname/multi-row" ) ) { - $ret = false; + $affectedRowCount = 0; + try { + $this->startAtomic( $fname, self::ATOMIC_CANCELABLE ); + foreach ( $rows as $v ) { + $this->nativeReplace( $table, $v, "$fname/multi-row" ); + $affectedRowCount += $this->affectedRows(); } + $this->endAtomic( $fname ); + } catch ( Exception $e ) { + $this->cancelAtomic( $fname ); + throw $e; } + $this->affectedRowCount = $affectedRowCount; } else { - $ret = $this->nativeReplace( $table, $rows, "$fname/single-row" ); + $this->nativeReplace( $table, $rows, "$fname/single-row" ); } - - return $ret; } /** diff --git a/includes/libs/rdbms/database/IDatabase.php b/includes/libs/rdbms/database/IDatabase.php index 3d3f855f7d..532d818771 100644 --- a/includes/libs/rdbms/database/IDatabase.php +++ b/includes/libs/rdbms/database/IDatabase.php @@ -924,8 +924,7 @@ interface IDatabase { * @param array $a Array of rows to insert * @param string $fname Calling function name (use __METHOD__) for logs/profiling * @param array $options Array of options - * - * @return bool + * @return bool Return true if no exception was thrown (deprecated since 1.33) * @throws DBError */ public function insert( $table, $a, $fname = __METHOD__, $options = [] ); @@ -948,7 +947,7 @@ interface IDatabase { * @param array $options An array of UPDATE options, can be: * - IGNORE: Ignore unique key conflicts * - LOW_PRIORITY: MySQL-specific, see MySQL manual. - * @return bool + * @return bool Return true if no exception was thrown (deprecated since 1.33) * @throws DBError */ public function update( $table, $values, $conds, $fname = __METHOD__, $options = [] ); @@ -1262,7 +1261,7 @@ interface IDatabase { * things like "field = field + 1" or similar computed values. * @param string $fname Calling function name (use __METHOD__) for logs/profiling * @throws DBError - * @return bool + * @return bool Return true if no exception was thrown (deprecated since 1.33) */ public function upsert( $table, array $rows, array $uniqueIndexes, array $set, $fname = __METHOD__ @@ -1300,7 +1299,7 @@ interface IDatabase { * for the format. Use $conds == "*" to delete all rows * @param string $fname Name of the calling function * @throws DBUnexpectedError - * @return bool|IResultWrapper + * @return bool Return true if no exception was thrown (deprecated since 1.33) * @throws DBError */ public function delete( $table, $conds, $fname = __METHOD__ ); @@ -1338,7 +1337,7 @@ interface IDatabase { * @param array $selectJoinConds Join conditions for the SELECT part of the query, see * IDatabase::select() for details. * - * @return bool + * @return bool Return true if no exception was thrown (deprecated since 1.33) * @throws DBError */ public function insertSelect( $destTable, $srcTable, $varMap, $conds, diff --git a/tests/phpunit/includes/db/DatabaseTestHelper.php b/tests/phpunit/includes/db/DatabaseTestHelper.php index 936bee092e..65b82abf0f 100644 --- a/tests/phpunit/includes/db/DatabaseTestHelper.php +++ b/tests/phpunit/includes/db/DatabaseTestHelper.php @@ -148,7 +148,7 @@ class DatabaseTestHelper extends Database { // Redeclare parent method to make it public public function nativeReplace( $table, $rows, $fname ) { - return parent::nativeReplace( $table, $rows, $fname ); + parent::nativeReplace( $table, $rows, $fname ); } function getType() {