From: Brad Jorsch <bjorsch@wikimedia.org>
Date: Mon, 28 Apr 2014 16:03:02 +0000 (-0400)
Subject: API: Fix queries for list=allusers&auactiveusers
X-Git-Tag: 1.31.0-rc.0~15136^2
X-Git-Url: http://git.cyclocoop.org/%22.%24info%5B?a=commitdiff_plain;h=eb4eb3c79888870455819ff10c0e7a3ff4f4b49e;p=lhc%2Fweb%2Fwiklou.git

API: Fix queries for list=allusers&auactiveusers

The query introduced to support the auactiveusers is itself broken (it
counts every edit multiple times when combined with the group filters or
auprop=groups or auprop=rights, or for users with multiple rows in
ipblocks) and it breaks auprop=groups and auprop=rights.

Instead, let's filter using the same cached data used by
Special:ActiveUsers and do the actual counting of recent "edits" in a
subquery. And for parity with Special:ActiveUsers, let's skip
RC_EXTERNAL when doing the count.

Also, it turns out the "recenteditcount" property in the result is
really more like "recentactions" since it counts any action that shows
up in recentchanges; the discrepancy between that and "editcount" can be
confusing if someone is doing a lot of logged actions that don't create
dummy revisions. Let's rename that, but we'll have to keep the old
property around for a while for BC.

Bug: 64505
Bug: 64507
Bug: 67301
Change-Id: I461e92819188c311cbb3853bc6bfad45962c8d7b
---

diff --git a/includes/api/ApiQueryAllUsers.php b/includes/api/ApiQueryAllUsers.php
index e32104f87d..d0980e6b14 100644
--- a/includes/api/ApiQueryAllUsers.php
+++ b/includes/api/ApiQueryAllUsers.php
@@ -45,9 +45,15 @@ class ApiQueryAllUsers extends ApiQueryBase {
 	}
 
 	public function execute() {
-		$db = $this->getDB();
 		$params = $this->extractRequestParams();
 
+		if ( $params['activeusers'] ) {
+			// Update active user cache
+			SpecialActiveUsers::mergeActiveUsers( 600 );
+		}
+
+		$db = $this->getDB();
+
 		$prop = $params['prop'];
 		if ( !is_null( $prop ) ) {
 			$prop = array_flip( $prop );
@@ -71,9 +77,9 @@ class ApiQueryAllUsers extends ApiQueryBase {
 		$from = is_null( $params['from'] ) ? null : $this->getCanonicalUserName( $params['from'] );
 		$to = is_null( $params['to'] ) ? null : $this->getCanonicalUserName( $params['to'] );
 
-		# MySQL doesn't seem to use 'equality propagation' here, so like the
-		# ActiveUsers special page, we have to use rc_user_text for some cases.
-		$userFieldToSort = $params['activeusers'] ? 'rc_user_text' : 'user_name';
+		# MySQL can't figure out that 'user_name' and 'qcc_title' are the same
+		# despite the JOIN condition, so manually sort on the correct one.
+		$userFieldToSort = $params['activeusers'] ? 'qcc_title' : 'user_name';
 
 		$this->addWhereRange( $userFieldToSort, $dir, $from, $to );
 
@@ -155,19 +161,32 @@ class ApiQueryAllUsers extends ApiQueryBase {
 		}
 
 		if ( $params['activeusers'] ) {
-			$this->addTables( 'recentchanges' );
-
-			$this->addJoinConds( array( 'recentchanges' => array(
-				'INNER JOIN', 'rc_user_text=user_name'
+			$activeUserSeconds = $this->getConfig()->get( 'ActiveUserDays' ) * 86400;
+
+			// Filter query to only include users in the active users cache
+			$this->addTables( 'querycachetwo' );
+			$this->addJoinConds( array( 'querycachetwo' => array(
+				'INNER JOIN', array(
+					'qcc_type' => 'activeusers',
+					'qcc_namespace' => NS_USER,
+					'qcc_title=user_name',
+				),
 			) ) );
 
-			$this->addFields( array( 'recentedits' => 'COUNT(*)' ) );
-
-			$this->addWhere( 'rc_log_type IS NULL OR rc_log_type != ' . $db->addQuotes( 'newusers' ) );
-			$timestamp = $db->timestamp( wfTimestamp( TS_UNIX ) - $this->getConfig()->get( 'ActiveUserDays' ) * 24 * 3600 );
-			$this->addWhere( 'rc_timestamp >= ' . $db->addQuotes( $timestamp ) );
-
-			$this->addOption( 'GROUP BY', $userFieldToSort );
+			// Actually count the actions using a subquery (bug 64505 and bug 64507)
+			$timestamp = $db->timestamp( wfTimestamp( TS_UNIX ) - $activeUserSeconds );
+			$this->addFields( array(
+				'recentactions' => '(' . $db->selectSQLText(
+					'recentchanges',
+					'COUNT(*)',
+					array(
+						'rc_user_text = user_name',
+						'rc_type != ' . $db->addQuotes( RC_EXTERNAL ), // no wikidata
+						'rc_log_type IS NULL OR rc_log_type != ' . $db->addQuotes( 'newusers' ),
+						'rc_timestamp >= ' . $db->addQuotes( $timestamp ),
+					)
+				) . ')'
+			) );
 		}
 
 		$this->addOption( 'LIMIT', $sqlLimit );
@@ -203,8 +222,13 @@ class ApiQueryAllUsers extends ApiQueryBase {
 			if ( $lastUser !== $row->user_name ) {
 				// Save the last pass's user data
 				if ( is_array( $lastUserData ) ) {
-					$fit = $result->addValue( array( 'query', $this->getModuleName() ),
-						null, $lastUserData );
+					if ( $params['activeusers'] && $lastUserData['recentactions'] === 0 ) {
+						// activeusers cache was out of date
+						$fit = true;
+					} else {
+						$fit = $result->addValue( array( 'query', $this->getModuleName() ),
+							null, $lastUserData );
+					}
 
 					$lastUserData = null;
 
@@ -241,7 +265,9 @@ class ApiQueryAllUsers extends ApiQueryBase {
 					$lastUserData['editcount'] = intval( $row->user_editcount );
 				}
 				if ( $params['activeusers'] ) {
-					$lastUserData['recenteditcount'] = intval( $row->recentedits );
+					$lastUserData['recentactions'] = intval( $row->recentactions );
+					// @todo 'recenteditcount' is set for BC, remove in 1.25
+					$lastUserData['recenteditcount'] = $lastUserData['recentactions'];
 				}
 				if ( $fld_registration ) {
 					$lastUserData['registration'] = $row->user_registration ?
@@ -302,7 +328,9 @@ class ApiQueryAllUsers extends ApiQueryBase {
 			}
 		}
 
-		if ( is_array( $lastUserData ) ) {
+		if ( is_array( $lastUserData ) &&
+			!( $params['activeusers'] && $lastUserData['recentactions'] === 0 )
+		) {
 			$fit = $result->addValue( array( 'query', $this->getModuleName() ),
 				null, $lastUserData );
 			if ( !$fit ) {
@@ -397,7 +425,7 @@ class ApiQueryAllUsers extends ApiQueryBase {
 			'' => array(
 				'userid' => 'integer',
 				'name' => 'string',
-				'recenteditcount' => array(
+				'recentactions' => array(
 					ApiBase::PROP_TYPE => 'integer',
 					ApiBase::PROP_NULLABLE => true
 				)