From 6237fd11b63bfeaa803acd8b9186316e7a543436 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Sun, 28 Jan 2018 06:10:39 -0800 Subject: [PATCH] rdbms: make affectedRows() work more consistently * Update replace()/upsert() to combine the affected row count for the non-native case * Also make replace() atomic in the non-native case, similar to how upsert() already works Change-Id: I6c9bcba54eca6bcf4a93a9b230aaedf7f36aa877 --- includes/db/DatabaseOracle.php | 2 +- includes/libs/rdbms/database/Database.php | 28 +++++++++++++++++-- .../libs/rdbms/database/DatabaseMssql.php | 2 +- .../libs/rdbms/database/DatabaseMysqli.php | 2 +- .../libs/rdbms/database/DatabasePostgres.php | 2 +- .../libs/rdbms/database/DatabaseSqlite.php | 2 +- .../includes/db/DatabaseTestHelper.php | 2 +- .../rdbms/database/DatabaseMysqlBaseTest.php | 3 ++ .../libs/rdbms/database/DatabaseTest.php | 2 +- 9 files changed, 35 insertions(+), 10 deletions(-) diff --git a/includes/db/DatabaseOracle.php b/includes/db/DatabaseOracle.php index e2feb1fa7c..90fabaf5c4 100644 --- a/includes/db/DatabaseOracle.php +++ b/includes/db/DatabaseOracle.php @@ -354,7 +354,7 @@ class DatabaseOracle extends Database { return $e['code']; } - function affectedRows() { + protected function fetchAffectedRowCount() { return $this->mAffectedRows; } diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php index 6d1cff7d6a..f3877fbe1e 100644 --- a/includes/libs/rdbms/database/Database.php +++ b/includes/libs/rdbms/database/Database.php @@ -126,6 +126,8 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware protected $delimiter = ';'; /** @var DatabaseDomain */ protected $currentDomain; + /** @var integer|null Rows affected by the last query to query() or its CRUD wrappers */ + protected $affectedRowCount; /** * Either 1 if a transaction is active or 0 otherwise. @@ -1002,8 +1004,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware } /** - * Helper method for query() that handles profiling and logging and sends - * the query to doQuery() + * Wrapper for query() that also handles profiling, logging, and affected row count updates * * @param string $sql Original SQL query * @param string $commentedSql SQL query with debugging/trace comment @@ -1029,7 +1030,9 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware if ( $this->profiler ) { call_user_func( [ $this->profiler, 'profileIn' ], $queryProf ); } + $this->affectedRowCount = null; $ret = $this->doQuery( $commentedSql ); + $this->affectedRowCount = $this->affectedRows(); if ( $this->profiler ) { call_user_func( [ $this->profiler, 'profileOut' ], $queryProf ); } @@ -2247,7 +2250,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware $rows = [ $rows ]; } - // @FIXME: this is not atomic, but a trx would break affectedRows() + $affectedRowCount = 0; foreach ( $rows as $row ) { // Delete rows which collide with this one $indexWhereClauses = []; @@ -2272,11 +2275,15 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware if ( $indexWhereClauses ) { $this->delete( $table, $this->makeList( $indexWhereClauses, LIST_OR ), $fname ); + $affectedRowCount += $this->affectedRows(); } // Now insert the row $this->insert( $table, $row, $fname ); + $affectedRowCount += $this->affectedRows(); } + + $this->affectedRowCount = $affectedRowCount; } /** @@ -2341,6 +2348,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware $where = false; } + $affectedRowCount = 0; $useTrx = !$this->mTrxLevel; if ( $useTrx ) { $this->begin( $fname, self::TRANSACTION_INTERNAL ); @@ -2349,11 +2357,13 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware # Update any existing conflicting row(s) if ( $where !== false ) { $ok = $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; + $affectedRowCount += $this->affectedRows(); } catch ( Exception $e ) { if ( $useTrx ) { $this->rollback( $fname, self::FLUSHING_INTERNAL ); @@ -2363,6 +2373,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware if ( $useTrx ) { $this->commit( $fname, self::FLUSHING_INTERNAL ); } + $this->affectedRowCount = $affectedRowCount; return $ok; } @@ -3181,6 +3192,17 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware } } + public function affectedRows() { + return ( $this->affectedRowCount === null ) + ? $this->fetchAffectedRowCount() // default to driver value + : $this->affectedRowCount; + } + + /** + * @return int Number of retrieved rows according to the driver + */ + abstract protected function fetchAffectedRowCount(); + /** * Take the result from a query, and wrap it in a ResultWrapper if * necessary. Boolean values are passed through as is, to indicate success diff --git a/includes/libs/rdbms/database/DatabaseMssql.php b/includes/libs/rdbms/database/DatabaseMssql.php index 53beb65f9a..832ed9e11e 100644 --- a/includes/libs/rdbms/database/DatabaseMssql.php +++ b/includes/libs/rdbms/database/DatabaseMssql.php @@ -353,7 +353,7 @@ class DatabaseMssql extends Database { /** * @return int */ - public function affectedRows() { + protected function fetchAffectedRowCount() { return $this->mAffectedRows; } diff --git a/includes/libs/rdbms/database/DatabaseMysqli.php b/includes/libs/rdbms/database/DatabaseMysqli.php index c1a56988d5..09ea66cac8 100644 --- a/includes/libs/rdbms/database/DatabaseMysqli.php +++ b/includes/libs/rdbms/database/DatabaseMysqli.php @@ -172,7 +172,7 @@ class DatabaseMysqli extends DatabaseMysqlBase { /** * @return int */ - function affectedRows() { + protected function fetchAffectedRowCount() { $conn = $this->getBindingHandle(); return $conn->affected_rows; diff --git a/includes/libs/rdbms/database/DatabasePostgres.php b/includes/libs/rdbms/database/DatabasePostgres.php index 8c21d726b0..5bf845b1af 100644 --- a/includes/libs/rdbms/database/DatabasePostgres.php +++ b/includes/libs/rdbms/database/DatabasePostgres.php @@ -384,7 +384,7 @@ class DatabasePostgres extends Database { } } - public function affectedRows() { + protected function fetchAffectedRowCount() { if ( !is_null( $this->mAffectedRows ) ) { // Forced result for simulated queries return $this->mAffectedRows; diff --git a/includes/libs/rdbms/database/DatabaseSqlite.php b/includes/libs/rdbms/database/DatabaseSqlite.php index 3bdcd65671..01772cf809 100644 --- a/includes/libs/rdbms/database/DatabaseSqlite.php +++ b/includes/libs/rdbms/database/DatabaseSqlite.php @@ -496,7 +496,7 @@ class DatabaseSqlite extends Database { /** * @return int */ - function affectedRows() { + protected function fetchAffectedRowCount() { return $this->mAffectedRows; } diff --git a/tests/phpunit/includes/db/DatabaseTestHelper.php b/tests/phpunit/includes/db/DatabaseTestHelper.php index d19d998146..fa9898d1cd 100644 --- a/tests/phpunit/includes/db/DatabaseTestHelper.php +++ b/tests/phpunit/includes/db/DatabaseTestHelper.php @@ -174,7 +174,7 @@ class DatabaseTestHelper extends Database { return false; } - function affectedRows() { + function fetchAffectedRowCount() { return -1; } diff --git a/tests/phpunit/includes/libs/rdbms/database/DatabaseMysqlBaseTest.php b/tests/phpunit/includes/libs/rdbms/database/DatabaseMysqlBaseTest.php index a23a2a0096..caf1281e65 100644 --- a/tests/phpunit/includes/libs/rdbms/database/DatabaseMysqlBaseTest.php +++ b/tests/phpunit/includes/libs/rdbms/database/DatabaseMysqlBaseTest.php @@ -53,6 +53,9 @@ class FakeDatabaseMysqlBase extends DatabaseMysqlBase { protected function doQuery( $sql ) { } + protected function fetchAffectedRowCount() { + } + // From DatabaseMysqli protected function mysqlConnect( $realServer ) { } diff --git a/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php b/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php index 4a0a7e71df..e131506729 100644 --- a/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php +++ b/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php @@ -322,7 +322,7 @@ class DatabaseTest extends PHPUnit_Framework_TestCase { */ private function getMockDB( $methods = [] ) { static $abstractMethods = [ - 'affectedRows', + 'fetchAffectedRowCount', 'closeConnection', 'dataSeek', 'doQuery', -- 2.20.1