From 6e2d6a0b44463559428c11f9beffa3e0ca4b40ca Mon Sep 17 00:00:00 2001 From: addshore Date: Tue, 26 Jan 2016 21:12:37 +0100 Subject: [PATCH] Move counting of watchers to WatchedItemStore Also adds tests Bug: T129479 Bug: T129482 Change-Id: I5a465773599cce9f8c9e94847cede6d12282c827 --- includes/WatchedItemStore.php | 71 +++++++- includes/actions/InfoAction.php | 13 +- includes/api/ApiQueryInfo.php | 23 +-- .../WatchedItemStoreIntegrationTest.php | 21 +++ .../includes/WatchedItemStoreUnitTest.php | 154 ++++++++++++++++++ 5 files changed, 252 insertions(+), 30 deletions(-) diff --git a/includes/WatchedItemStore.php b/includes/WatchedItemStore.php index fc3af5f9ac..86e4925176 100644 --- a/includes/WatchedItemStore.php +++ b/includes/WatchedItemStore.php @@ -45,7 +45,14 @@ class WatchedItemStore { */ private static $instance; - public function __construct( LoadBalancer $loadBalancer, HashBagOStuff $cache ) { + /** + * @param LoadBalancer $loadBalancer + * @param HashBagOStuff $cache + */ + public function __construct( + LoadBalancer $loadBalancer, + HashBagOStuff $cache + ) { $this->loadBalancer = $loadBalancer; $this->cache = $cache; $this->deferredUpdatesAddCallableUpdateCallback = [ 'DeferredUpdates', 'addCallableUpdate' ]; @@ -177,6 +184,68 @@ class WatchedItemStore { ]; } + /** + * @param LinkTarget $target + * + * @return int + */ + public function countWatchers( LinkTarget $target ) { + $dbr = $this->loadBalancer->getConnection( DB_SLAVE, [ 'watchlist' ] ); + $return = (int)$dbr->selectField( + 'watchlist', + 'COUNT(*)', + [ + 'wl_namespace' => $target->getNamespace(), + 'wl_title' => $target->getDBkey(), + ], + __METHOD__ + ); + $this->loadBalancer->reuseConnection( $dbr ); + + return $return; + } + + /** + * @param LinkTarget[] $targets + * @param array $options Allowed keys: + * 'minimumWatchers' => int + * + * @return array multi dimensional like $return[$namespaceId][$titleString] = int $watchers + * All targets will be present in the result. 0 either means no watchers or the number + * of watchers was below the minimumWatchers option if passed. + */ + public function countWatchersMultiple( array $targets, array $options = [] ) { + $dbOptions = [ 'GROUP BY' => [ 'wl_namespace', 'wl_title' ] ]; + + $dbr = $this->loadBalancer->getConnection( DB_SLAVE, [ 'watchlist' ] ); + + if ( array_key_exists( 'minimumWatchers', $options ) ) { + $dbOptions['HAVING'] = 'COUNT(*) >= ' . (int)$options['minimumWatchers']; + } + + $lb = new LinkBatch( $targets ); + $res = $dbr->select( + 'watchlist', + [ 'wl_title', 'wl_namespace', 'watchers' => 'COUNT(*)' ], + [ $lb->constructSet( 'wl', $dbr ) ], + __METHOD__, + $dbOptions + ); + + $this->loadBalancer->reuseConnection( $dbr ); + + $watchCounts = []; + foreach ( $targets as $linkTarget ) { + $watchCounts[$linkTarget->getNamespace()][$linkTarget->getDBkey()] = 0; + } + + foreach ( $res as $row ) { + $watchCounts[$row->wl_namespace][$row->wl_title] = (int)$row->watchers; + } + + return $watchCounts; + } + /** * Get an item (may be cached) * diff --git a/includes/actions/InfoAction.php b/includes/actions/InfoAction.php index 87d269aec6..60829b1db2 100644 --- a/includes/actions/InfoAction.php +++ b/includes/actions/InfoAction.php @@ -678,18 +678,7 @@ class InfoAction extends FormlessAction { $setOpts += Database::getCacheSetOptions( $dbr, $dbrWatchlist ); $result = []; - - // Number of page watchers - $watchers = (int)$dbrWatchlist->selectField( - 'watchlist', - 'COUNT(*)', - [ - 'wl_namespace' => $title->getNamespace(), - 'wl_title' => $title->getDBkey(), - ], - $fname - ); - $result['watchers'] = $watchers; + $result['watchers'] = WatchedItemStore::getDefaultInstance()->countWatchers( $title ); if ( $config->get( 'ShowUpdatedMarker' ) ) { // Threshold: last visited about 26 weeks before latest edit diff --git a/includes/api/ApiQueryInfo.php b/includes/api/ApiQueryInfo.php index ea7818c3b8..a3c98ed5e2 100644 --- a/includes/api/ApiQueryInfo.php +++ b/includes/api/ApiQueryInfo.php @@ -799,28 +799,17 @@ class ApiQueryInfo extends ApiQueryBase { return; } - $this->watchers = []; $this->showZeroWatchers = $canUnwatchedpages; - $db = $this->getDB(); - - $lb = new LinkBatch( $this->everything ); - $this->resetQueryParams(); - $this->addTables( [ 'watchlist' ] ); - $this->addFields( [ 'wl_title', 'wl_namespace', 'count' => 'COUNT(*)' ] ); - $this->addWhere( [ - $lb->constructSet( 'wl', $db ) - ] ); - $this->addOption( 'GROUP BY', [ 'wl_namespace', 'wl_title' ] ); + $countOptions = []; if ( !$canUnwatchedpages ) { - $this->addOption( 'HAVING', "COUNT(*) >= $unwatchedPageThreshold" ); + $countOptions['minimumWatchers'] = $unwatchedPageThreshold; } - $res = $this->select( __METHOD__ ); - - foreach ( $res as $row ) { - $this->watchers[$row->wl_namespace][$row->wl_title] = (int)$row->count; - } + $this->watchers = WatchedItemStore::getDefaultInstance()->countWatchersMultiple( + $this->everything, + $countOptions + ); } /** diff --git a/tests/phpunit/includes/WatchedItemStoreIntegrationTest.php b/tests/phpunit/includes/WatchedItemStoreIntegrationTest.php index e2ab512b29..3e5f261934 100644 --- a/tests/phpunit/includes/WatchedItemStoreIntegrationTest.php +++ b/tests/phpunit/includes/WatchedItemStoreIntegrationTest.php @@ -25,21 +25,42 @@ class WatchedItemStoreIntegrationTest extends MediaWikiTestCase { $store = WatchedItemStore::getDefaultInstance(); // Cleanup after previous tests $store->removeWatch( $user, $title ); + $initialWatchers = $store->countWatchers( $title ); $this->assertFalse( $store->isWatched( $user, $title ), 'Page should not initially be watched' ); + $store->addWatch( $user, $title ); $this->assertTrue( $store->isWatched( $user, $title ), 'Page should be watched' ); + $this->assertEquals( $initialWatchers + 1, $store->countWatchers( $title ) ); + $this->assertEquals( + $initialWatchers + 1, + $store->countWatchersMultiple( [ $title ] )[$title->getNamespace()][$title->getDBkey()] + ); + $this->assertEquals( + [ 0 => [ 'WatchedItemStoreIntegrationTestPage' => $initialWatchers + 1 ] ], + $store->countWatchersMultiple( [ $title ], [ 'minimumWatchers' => $initialWatchers + 1 ] ) + ); + $this->assertEquals( + [ 0 => [ 'WatchedItemStoreIntegrationTestPage' => 0 ] ], + $store->countWatchersMultiple( [ $title ], [ 'minimumWatchers' => $initialWatchers + 2 ] ) + ); + $store->removeWatch( $user, $title ); $this->assertFalse( $store->isWatched( $user, $title ), 'Page should be unwatched' ); + $this->assertEquals( $initialWatchers, $store->countWatchers( $title ) ); + $this->assertEquals( + $initialWatchers, + $store->countWatchersMultiple( [ $title ] )[$title->getNamespace()][$title->getDBkey()] + ); } public function testUpdateAndResetNotificationTimestamp() { diff --git a/tests/phpunit/includes/WatchedItemStoreUnitTest.php b/tests/phpunit/includes/WatchedItemStoreUnitTest.php index 1b1a319438..c8621fc3b4 100644 --- a/tests/phpunit/includes/WatchedItemStoreUnitTest.php +++ b/tests/phpunit/includes/WatchedItemStoreUnitTest.php @@ -81,6 +81,160 @@ class WatchedItemStoreUnitTest extends PHPUnit_Framework_TestCase { $this->assertSame( $instanceOne, $instanceTwo ); } + public function testCountWatchers() { + $titleValue = new TitleValue( 0, 'SomeDbKey' ); + + $mockDb = $this->getMockDb(); + $mockDb->expects( $this->exactly( 1 ) ) + ->method( 'selectField' ) + ->with( + 'watchlist', + 'COUNT(*)', + [ + 'wl_namespace' => $titleValue->getNamespace(), + 'wl_title' => $titleValue->getDBkey(), + ], + $this->isType( 'string' ) + ) + ->will( $this->returnValue( 7 ) ); + + $mockCache = $this->getMockCache(); + $mockCache->expects( $this->never() )->method( 'get' ); + $mockCache->expects( $this->never() )->method( 'set' ); + $mockCache->expects( $this->never() )->method( 'delete' ); + + $store = new WatchedItemStore( + $this->getMockLoadBalancer( $mockDb ), + $mockCache + ); + + $this->assertEquals( 7, $store->countWatchers( $titleValue ) ); + } + + public function testCountWatchersMultiple() { + $titleValues = [ + new TitleValue( 0, 'SomeDbKey' ), + new TitleValue( 0, 'OtherDbKey' ), + new TitleValue( 1, 'AnotherDbKey' ), + ]; + + $mockDb = $this->getMockDb(); + + $dbResult = [ + $this->getFakeRow( [ 'wl_title' => 'SomeDbKey', 'wl_namespace' => 0, 'watchers' => 100 ] ), + $this->getFakeRow( [ 'wl_title' => 'OtherDbKey', 'wl_namespace' => 0, 'watchers' => 300 ] ), + $this->getFakeRow( [ 'wl_title' => 'AnotherDbKey', 'wl_namespace' => 1, 'watchers' => 500 ] + ), + ]; + $mockDb->expects( $this->once() ) + ->method( 'makeWhereFrom2d' ) + ->with( + [ [ 'SomeDbKey' => 1, 'OtherDbKey' => 1 ], [ 'AnotherDbKey' => 1 ] ], + $this->isType( 'string' ), + $this->isType( 'string' ) + ) + ->will( $this->returnValue( 'makeWhereFrom2d return value' ) ); + $mockDb->expects( $this->once() ) + ->method( 'select' ) + ->with( + 'watchlist', + [ 'wl_title', 'wl_namespace', 'watchers' => 'COUNT(*)' ], + [ 'makeWhereFrom2d return value' ], + $this->isType( 'string' ), + [ + 'GROUP BY' => [ 'wl_namespace', 'wl_title' ], + ] + ) + ->will( + $this->returnValue( $dbResult ) + ); + + $mockCache = $this->getMockCache(); + $mockCache->expects( $this->never() )->method( 'get' ); + $mockCache->expects( $this->never() )->method( 'set' ); + $mockCache->expects( $this->never() )->method( 'delete' ); + + $store = new WatchedItemStore( + $this->getMockLoadBalancer( $mockDb ), + $mockCache + ); + + $expected = [ + 0 => [ 'SomeDbKey' => 100, 'OtherDbKey' => 300 ], + 1 => [ 'AnotherDbKey' => 500 ], + ]; + $this->assertEquals( $expected, $store->countWatchersMultiple( $titleValues ) ); + } + + public function provideMinimumWatchers() { + return [ + [ 50 ], + [ "50; DROP TABLE watchlist;\n--" ], + ]; + } + + /** + * @dataProvider provideMinimumWatchers + */ + public function testCountWatchersMultiple_withMinimumWatchers( $minWatchers ) { + $titleValues = [ + new TitleValue( 0, 'SomeDbKey' ), + new TitleValue( 0, 'OtherDbKey' ), + new TitleValue( 1, 'AnotherDbKey' ), + ]; + + $mockDb = $this->getMockDb(); + + $dbResult = [ + $this->getFakeRow( [ 'wl_title' => 'SomeDbKey', 'wl_namespace' => 0, 'watchers' => 100 ] ), + $this->getFakeRow( [ 'wl_title' => 'OtherDbKey', 'wl_namespace' => 0, 'watchers' => 300 ] ), + $this->getFakeRow( [ 'wl_title' => 'AnotherDbKey', 'wl_namespace' => 1, 'watchers' => 500 ] + ), + ]; + $mockDb->expects( $this->once() ) + ->method( 'makeWhereFrom2d' ) + ->with( + [ [ 'SomeDbKey' => 1, 'OtherDbKey' => 1 ], [ 'AnotherDbKey' => 1 ] ], + $this->isType( 'string' ), + $this->isType( 'string' ) + ) + ->will( $this->returnValue( 'makeWhereFrom2d return value' ) ); + $mockDb->expects( $this->once() ) + ->method( 'select' ) + ->with( + 'watchlist', + [ 'wl_title', 'wl_namespace', 'watchers' => 'COUNT(*)' ], + [ 'makeWhereFrom2d return value' ], + $this->isType( 'string' ), + [ + 'GROUP BY' => [ 'wl_namespace', 'wl_title' ], + 'HAVING' => 'COUNT(*) >= 50', + ] + ) + ->will( + $this->returnValue( $dbResult ) + ); + + $mockCache = $this->getMockCache(); + $mockCache->expects( $this->never() )->method( 'get' ); + $mockCache->expects( $this->never() )->method( 'set' ); + $mockCache->expects( $this->never() )->method( 'delete' ); + + $store = new WatchedItemStore( + $this->getMockLoadBalancer( $mockDb ), + $mockCache + ); + + $expected = [ + 0 => [ 'SomeDbKey' => 100, 'OtherDbKey' => 300 ], + 1 => [ 'AnotherDbKey' => 500 ], + ]; + $this->assertEquals( + $expected, + $store->countWatchersMultiple( $titleValues, [ 'minimumWatchers' => $minWatchers ] ) + ); + } + public function testDuplicateEntry_nothingToDuplicate() { $mockDb = $this->getMockDb(); $mockDb->expects( $this->once() ) -- 2.20.1