From 7d4e225cb90ac275c6077e830d8b63678c3ea9dc Mon Sep 17 00:00:00 2001 From: Leszek Manicki Date: Tue, 15 Mar 2016 11:39:22 +0100 Subject: [PATCH] Add WatchedItemStore::countVisitingWatchersMultiple This is for batch counting of visiting watchers, following the change made in I2868c31fc09121de381d822e8f49194e3022bb42. Query/logic has been extracted from ApiQueryInfo. Bug: T129482 Change-Id: Ia9a534f5edb7af3cb7bf86be358dddb5d8c259cf --- includes/WatchedItemStore.php | 94 +++++++ includes/api/ApiQueryInfo.php | 76 ++---- .../WatchedItemStoreIntegrationTest.php | 24 ++ .../includes/WatchedItemStoreUnitTest.php | 241 ++++++++++++++++++ 4 files changed, 385 insertions(+), 50 deletions(-) diff --git a/includes/WatchedItemStore.php b/includes/WatchedItemStore.php index b3a06e2737..4e2dfa5d99 100644 --- a/includes/WatchedItemStore.php +++ b/includes/WatchedItemStore.php @@ -339,6 +339,100 @@ class WatchedItemStore { return $watchCounts; } + /** + * Number of watchers of each page who have visited recent edits to that page + * + * @param array $targetsWithVisitThresholds array of pairs (LinkTarget $target, mixed $threshold), + * $threshold is: + * - a timestamp of the recent edit if $target exists (format accepted by wfTimestamp) + * - null if $target doesn't exist + * @param int|null $minimumWatchers + * @return array multi-dimensional like $return[$namespaceId][$titleString] = $watchers, + * where $watchers is an int: + * - if the page exists, number of users watching who have visited the page recently + * - if the page doesn't exist, number of users that have the page on their watchlist + * - 0 means there are no visiting watchers or their number is below the minimumWatchers + * option (if passed). + */ + public function countVisitingWatchersMultiple( + array $targetsWithVisitThresholds, + $minimumWatchers = null + ) { + $dbr = $this->getConnection( DB_SLAVE ); + + $conds = $this->getVisitingWatchersCondition( $dbr, $targetsWithVisitThresholds ); + + $dbOptions = [ 'GROUP BY' => [ 'wl_namespace', 'wl_title' ] ]; + if ( $minimumWatchers !== null ) { + $dbOptions['HAVING'] = 'COUNT(*) >= ' . (int)$minimumWatchers; + } + $res = $dbr->select( + 'watchlist', + [ 'wl_namespace', 'wl_title', 'watchers' => 'COUNT(*)' ], + $conds, + __METHOD__, + $dbOptions + ); + + $this->reuseConnection( $dbr ); + + $watcherCounts = []; + foreach ( $targetsWithVisitThresholds as list( $target ) ) { + /* @var LinkTarget $target */ + $watcherCounts[$target->getNamespace()][$target->getDBkey()] = 0; + } + + foreach ( $res as $row ) { + $watcherCounts[$row->wl_namespace][$row->wl_title] = (int)$row->watchers; + } + + return $watcherCounts; + } + + /** + * Generates condition for the query used in a batch count visiting watchers. + * + * @param IDatabase $db + * @param array $targetsWithVisitThresholds array of pairs (LinkTarget, last visit threshold) + * @return string + */ + private function getVisitingWatchersCondition( + IDatabase $db, + array $targetsWithVisitThresholds + ) { + $missingTargets = []; + $namespaceConds = []; + foreach ( $targetsWithVisitThresholds as list( $target, $threshold ) ) { + if ( $threshold === null ) { + $missingTargets[] = $target; + continue; + } + /* @var LinkTarget $target */ + $namespaceConds[$target->getNamespace()][] = $db->makeList( [ + 'wl_title = ' . $db->addQuotes( $target->getDBkey() ), + $db->makeList( [ + 'wl_notificationtimestamp >= ' . $db->addQuotes( $db->timestamp( $threshold ) ), + 'wl_notificationtimestamp IS NULL' + ], LIST_OR ) + ], LIST_AND ); + } + + $conds = []; + foreach ( $namespaceConds as $namespace => $pageConds ) { + $conds[] = $db->makeList( [ + 'wl_namespace = ' . $namespace, + '(' . $db->makeList( $pageConds, LIST_OR ) . ')' + ], LIST_AND ); + } + + if ( $missingTargets ) { + $lb = new LinkBatch( $missingTargets ); + $conds[] = $lb->constructSet( 'wl', $db ); + } + + return $db->makeList( $conds, LIST_OR ); + } + /** * Get an item (may be cached) * diff --git a/includes/api/ApiQueryInfo.php b/includes/api/ApiQueryInfo.php index f33da1e486..2d382dd54e 100644 --- a/includes/api/ApiQueryInfo.php +++ b/includes/api/ApiQueryInfo.php @@ -461,7 +461,7 @@ class ApiQueryInfo extends ApiQueryBase { } if ( $this->fld_visitingwatchers ) { - if ( isset( $this->visitingwatchers[$ns][$dbkey] ) ) { + if ( $this->visitingwatchers !== null && $this->visitingwatchers[$ns][$dbkey] !== 0 ) { $pageInfo['visitingwatchers'] = $this->visitingwatchers[$ns][$dbkey]; } elseif ( $this->showZeroWatchers ) { $pageInfo['visitingwatchers'] = 0; @@ -831,14 +831,7 @@ class ApiQueryInfo extends ApiQueryBase { $this->showZeroWatchers = $canUnwatchedpages; - // Assemble a WHERE condition to find: - // * if the page exists, number of users watching who have - // visited the page recently - // * if the page doesn't exist, number of users that have - // the page on their watchlist - $whereStrings = []; - - // For pages that exist + $titlesWithThresholds = []; if ( $this->titles ) { $lb = new LinkBatch( $this->titles ); @@ -853,55 +846,38 @@ class ApiQueryInfo extends ApiQueryBase { $this->addOption( 'GROUP BY', [ 'page_namespace', 'page_title' ] ); $timestampRes = $this->select( __METHOD__ ); - // Assemble SQL WHERE condition to find number of page watchers who also - // visited a "recent" edit (last visited about 26 weeks before latest edit) $age = $config->get( 'WatchersMaxAge' ); $timestamps = []; foreach ( $timestampRes as $row ) { $revTimestamp = wfTimestamp( TS_UNIX, (int)$row->rev_timestamp ); - $threshold = $db->timestamp( $revTimestamp - $age ); - $timestamps[$row->page_namespace][$row->page_title] = $threshold; - } - - foreach ( $timestamps as $ns_key => $namespace ) { - $pageStrings = []; - foreach ( $namespace as $pg_key => $threshold ) { - $pageStrings[] = "wl_title = '$pg_key' AND" . - ' (wl_notificationtimestamp >= ' . - $db->addQuotes( $threshold ) . - ' OR wl_notificationtimestamp IS NULL)'; - } - $whereStrings[] = "wl_namespace = '$ns_key' AND (" . - $db->makeList( $pageStrings, LIST_OR ) . ')'; + $timestamps[$row->page_namespace][$row->page_title] = $revTimestamp - $age; } + $titlesWithThresholds = array_map( + function( LinkTarget $target ) use ( $timestamps ) { + return [ + $target, $timestamps[$target->getNamespace()][$target->getDBkey()] + ]; + }, + $this->titles + ); } - // For nonexistant pages if ( $this->missing ) { - $lb = new LinkBatch( $this->missing ); - $whereStrings[] = $lb->constructSet( 'wl', $db ); - } - - // Make the actual string and do the query - $whereString = $db->makeList( $whereStrings, LIST_OR ); - - $this->resetQueryParams(); - $this->addTables( [ 'watchlist' ] ); - $this->addFields( [ - 'wl_namespace', - 'wl_title', - 'count' => 'COUNT(*)' - ] ); - $this->addWhere( [ $whereString ] ); - $this->addOption( 'GROUP BY', [ 'wl_namespace', 'wl_title' ] ); - if ( !$canUnwatchedpages ) { - $this->addOption( 'HAVING', "COUNT(*) >= $unwatchedPageThreshold" ); - } - - $res = $this->select( __METHOD__ ); - foreach ( $res as $row ) { - $this->visitingwatchers[$row->wl_namespace][$row->wl_title] = (int)$row->count; - } + $titlesWithThresholds = array_merge( + $titlesWithThresholds, + array_map( + function( LinkTarget $target ) { + return [ $target, null ]; + }, + $this->missing + ) + ); + } + + $this->visitingwatchers = WatchedItemStore::getDefaultInstance()->countVisitingWatchersMultiple( + $titlesWithThresholds, + !$canUnwatchedpages ? $unwatchedPageThreshold : null + ); } public function getCacheMode( $params ) { diff --git a/tests/phpunit/includes/WatchedItemStoreIntegrationTest.php b/tests/phpunit/includes/WatchedItemStoreIntegrationTest.php index 91dd1aae24..0dea461974 100644 --- a/tests/phpunit/includes/WatchedItemStoreIntegrationTest.php +++ b/tests/phpunit/includes/WatchedItemStoreIntegrationTest.php @@ -85,6 +85,12 @@ class WatchedItemStoreIntegrationTest extends MediaWikiTestCase { $initialVisitingWatchers - 1, $store->countVisitingWatchers( $title, '20150202020202' ) ); + $this->assertEquals( + $initialVisitingWatchers - 1, + $store->countVisitingWatchersMultiple( + [ [ $title, '20150202020202' ] ] + )[$title->getNamespace()][$title->getDBkey()] + ); $this->assertEquals( $initialUnreadNotifications + 1, $store->countUnreadNotifications( $user ) @@ -100,6 +106,24 @@ class WatchedItemStoreIntegrationTest extends MediaWikiTestCase { $initialVisitingWatchers, $store->countVisitingWatchers( $title, '20150202020202' ) ); + $this->assertEquals( + $initialVisitingWatchers, + $store->countVisitingWatchersMultiple( + [ [ $title, '20150202020202' ] ] + )[$title->getNamespace()][$title->getDBkey()] + ); + $this->assertEquals( + [ 0 => [ 'WatchedItemStoreIntegrationTestPage' => $initialVisitingWatchers ] ], + $store->countVisitingWatchersMultiple( + [ [ $title, '20150202020202' ] ], $initialVisitingWatchers + ) + ); + $this->assertEquals( + [ 0 => [ 'WatchedItemStoreIntegrationTestPage' => 0 ] ], + $store->countVisitingWatchersMultiple( + [ [ $title, '20150202020202' ] ], $initialVisitingWatchers + 1 + ) + ); } public function testDuplicateAllAssociatedEntries() { diff --git a/tests/phpunit/includes/WatchedItemStoreUnitTest.php b/tests/phpunit/includes/WatchedItemStoreUnitTest.php index 983a5fe4dd..869b0d2db0 100644 --- a/tests/phpunit/includes/WatchedItemStoreUnitTest.php +++ b/tests/phpunit/includes/WatchedItemStoreUnitTest.php @@ -316,6 +316,247 @@ class WatchedItemStoreUnitTest extends PHPUnit_Framework_TestCase { $this->assertEquals( 7, $store->countVisitingWatchers( $titleValue, '111' ) ); } + public function testCountVisitingWatchersMultiple() { + $titleValuesWithThresholds = [ + [ new TitleValue( 0, 'SomeDbKey' ), '111' ], + [ new TitleValue( 0, 'OtherDbKey' ), '111' ], + [ new TitleValue( 1, 'AnotherDbKey' ), '123' ], + ]; + + $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 = $this->getMockDb(); + $mockDb->expects( $this->exactly( 2 * 3 ) ) + ->method( 'addQuotes' ) + ->will( $this->returnCallback( function( $value ) { + return "'$value'"; + } ) ); + $mockDb->expects( $this->exactly( 3 ) ) + ->method( 'timestamp' ) + ->will( $this->returnCallback( function( $value ) { + return 'TS' . $value . 'TS'; + } ) ); + $mockDb->expects( $this->any() ) + ->method( 'makeList' ) + ->with( + $this->isType( 'array' ), + $this->isType( 'int' ) + ) + ->will( $this->returnCallback( function( $a, $conj ) { + $sqlConj = $conj === LIST_AND ? ' AND ' : ' OR '; + return join( $sqlConj, array_map( function( $s ) { + return '(' . $s . ')'; + }, $a + ) ); + } ) ); + $mockDb->expects( $this->never() ) + ->method( 'makeWhereFrom2d' ); + + $expectedCond = + '((wl_namespace = 0) AND (' . + "(((wl_title = 'SomeDbKey') AND (" . + "(wl_notificationtimestamp >= 'TS111TS') OR (wl_notificationtimestamp IS NULL)" . + ')) OR (' . + "(wl_title = 'OtherDbKey') AND (" . + "(wl_notificationtimestamp >= 'TS111TS') OR (wl_notificationtimestamp IS NULL)" . + '))))' . + ') OR ((wl_namespace = 1) AND (' . + "(((wl_title = 'AnotherDbKey') AND (". + "(wl_notificationtimestamp >= 'TS123TS') OR (wl_notificationtimestamp IS NULL)" . + ')))))'; + $mockDb->expects( $this->once() ) + ->method( 'select' ) + ->with( + 'watchlist', + [ 'wl_namespace', 'wl_title', 'watchers' => 'COUNT(*)' ], + $expectedCond, + $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->countVisitingWatchersMultiple( $titleValuesWithThresholds ) + ); + } + + public function testCountVisitingWatchersMultiple_withMissingTargets() { + $titleValuesWithThresholds = [ + [ new TitleValue( 0, 'SomeDbKey' ), '111' ], + [ new TitleValue( 0, 'OtherDbKey' ), '111' ], + [ new TitleValue( 1, 'AnotherDbKey' ), '123' ], + [ new TitleValue( 0, 'SomeNotExisitingDbKey' ), null ], + [ new TitleValue( 0, 'OtherNotExisitingDbKey' ), null ], + ]; + + $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 ] ), + $this->getFakeRow( + [ 'wl_title' => 'SomeNotExisitingDbKey', 'wl_namespace' => 0, 'watchers' => 100 ] + ), + $this->getFakeRow( + [ 'wl_title' => 'OtherNotExisitingDbKey', 'wl_namespace' => 0, 'watchers' => 200 ] + ), + ]; + $mockDb = $this->getMockDb(); + $mockDb->expects( $this->exactly( 2 * 3 ) ) + ->method( 'addQuotes' ) + ->will( $this->returnCallback( function( $value ) { + return "'$value'"; + } ) ); + $mockDb->expects( $this->exactly( 3 ) ) + ->method( 'timestamp' ) + ->will( $this->returnCallback( function( $value ) { + return 'TS' . $value . 'TS'; + } ) ); + $mockDb->expects( $this->any() ) + ->method( 'makeList' ) + ->with( + $this->isType( 'array' ), + $this->isType( 'int' ) + ) + ->will( $this->returnCallback( function( $a, $conj ) { + $sqlConj = $conj === LIST_AND ? ' AND ' : ' OR '; + return join( $sqlConj, array_map( function( $s ) { + return '(' . $s . ')'; + }, $a + ) ); + } ) ); + $mockDb->expects( $this->once() ) + ->method( 'makeWhereFrom2d' ) + ->with( + [ [ 'SomeNotExisitingDbKey' => 1, 'OtherNotExisitingDbKey' => 1 ] ], + $this->isType( 'string' ), + $this->isType( 'string' ) + ) + ->will( $this->returnValue( 'makeWhereFrom2d return value' ) ); + + $expectedCond = + '((wl_namespace = 0) AND (' . + "(((wl_title = 'SomeDbKey') AND (" . + "(wl_notificationtimestamp >= 'TS111TS') OR (wl_notificationtimestamp IS NULL)" . + ')) OR (' . + "(wl_title = 'OtherDbKey') AND (" . + "(wl_notificationtimestamp >= 'TS111TS') OR (wl_notificationtimestamp IS NULL)" . + '))))' . + ') OR ((wl_namespace = 1) AND (' . + "(((wl_title = 'AnotherDbKey') AND (". + "(wl_notificationtimestamp >= 'TS123TS') OR (wl_notificationtimestamp IS NULL)" . + '))))' . + ') OR ' . + '(makeWhereFrom2d return value)'; + $mockDb->expects( $this->once() ) + ->method( 'select' ) + ->with( + 'watchlist', + [ 'wl_namespace', 'wl_title', 'watchers' => 'COUNT(*)' ], + $expectedCond, + $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, + 'SomeNotExisitingDbKey' => 100, 'OtherNotExisitingDbKey' => 200 + ], + 1 => [ 'AnotherDbKey' => 500 ], + ]; + $this->assertEquals( + $expected, + $store->countVisitingWatchersMultiple( $titleValuesWithThresholds ) + ); + } + + /** + * @dataProvider provideIntWithDbUnsafeVersion + */ + public function testCountVisitingWatchersMultiple_withMinimumWatchers( $minWatchers ) { + $titleValuesWithThresholds = [ + [ new TitleValue( 0, 'SomeDbKey' ), '111' ], + [ new TitleValue( 0, 'OtherDbKey' ), '111' ], + [ new TitleValue( 1, 'AnotherDbKey' ), '123' ], + ]; + + $mockDb = $this->getMockDb(); + $mockDb->expects( $this->any() ) + ->method( 'makeList' ) + ->will( $this->returnValue( 'makeList return value' ) ); + $mockDb->expects( $this->once() ) + ->method( 'select' ) + ->with( + 'watchlist', + [ 'wl_namespace', 'wl_title', 'watchers' => 'COUNT(*)' ], + 'makeList return value', + $this->isType( 'string' ), + [ + 'GROUP BY' => [ 'wl_namespace', 'wl_title' ], + 'HAVING' => 'COUNT(*) >= 50', + ] + ) + ->will( + $this->returnValue( [] ) + ); + + $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' => 0, 'OtherDbKey' => 0 ], + 1 => [ 'AnotherDbKey' => 0 ], + ]; + $this->assertEquals( + $expected, + $store->countVisitingWatchersMultiple( $titleValuesWithThresholds, $minWatchers ) + ); + } + public function testCountUnreadNotifications() { $user = $this->getMockNonAnonUserWithId( 1 ); -- 2.20.1