From: Umherirrender <umherirrender_de.wp@web.de>
Date: Sat, 22 Sep 2018 09:31:02 +0000 (+0200)
Subject: Fix use of GenderCache in ApiPageSet::processTitlesArray
X-Git-Tag: 1.34.0-rc.0~299^2
X-Git-Url: https://git.heureux-cyclage.org/?p=lhc%2Fweb%2Fwiklou.git;a=commitdiff_plain;h=91fd689add2a4067d3e306fd0a5a2d8a6ffaff1b

Fix use of GenderCache in ApiPageSet::processTitlesArray

Title::getPrefixedText was called before the GenderCache was set up,
which lazy loads the cache for each title,
resulting in one query per user title

Splitted the foreach to fill the cache at a better location

Added a test for ApiPageSet to test that the gender cache is filled

Bug: T200238
Change-Id: I7972dd1bf3731a92328caab20e70d7b9b82c1f7c
---

diff --git a/includes/api/ApiPageSet.php b/includes/api/ApiPageSet.php
index 6afb018e2f..e68676a94e 100644
--- a/includes/api/ApiPageSet.php
+++ b/includes/api/ApiPageSet.php
@@ -1166,7 +1166,8 @@ class ApiPageSet extends ApiBase {
 		$services = MediaWikiServices::getInstance();
 		$contLang = $services->getContentLanguage();
 
-		foreach ( $titles as $title ) {
+		$titleObjects = [];
+		foreach ( $titles as $index => $title ) {
 			if ( is_string( $title ) ) {
 				try {
 					$titleObj = Title::newFromTextThrow( $title, $this->mDefaultNamespace );
@@ -1185,6 +1186,16 @@ class ApiPageSet extends ApiBase {
 			} else {
 				$titleObj = $title;
 			}
+
+			$titleObjects[$index] = $titleObj;
+		}
+
+		// Get gender information
+		$genderCache = $services->getGenderCache();
+		$genderCache->doTitlesArray( $titleObjects, __METHOD__ );
+
+		foreach ( $titleObjects as $index => $titleObj ) {
+			$title = is_string( $titles[$index] ) ? $titles[$index] : false;
 			$unconvertedTitle = $titleObj->getPrefixedText();
 			$titleWasConverted = false;
 			if ( $titleObj->isExternal() ) {
@@ -1197,7 +1208,7 @@ class ApiPageSet extends ApiBase {
 				) {
 					// Language::findVariantLink will modify titleText and titleObj into
 					// the canonical variant if possible
-					$titleText = is_string( $title ) ? $title : $titleObj->getPrefixedText();
+					$titleText = $title !== false ? $title : $titleObj->getPrefixedText();
 					$contLang->findVariantLink( $titleText, $titleObj );
 					$titleWasConverted = $unconvertedTitle !== $titleObj->getPrefixedText();
 				}
@@ -1245,24 +1256,13 @@ class ApiPageSet extends ApiBase {
 			if ( $titleWasConverted ) {
 				$this->mConvertedTitles[$unconvertedTitle] = $titleObj->getPrefixedText();
 				// In this case the page can't be Special.
-				if ( is_string( $title ) && $title !== $unconvertedTitle ) {
+				if ( $title !== false && $title !== $unconvertedTitle ) {
 					$this->mNormalizedTitles[$title] = $unconvertedTitle;
 				}
-			} elseif ( is_string( $title ) && $title !== $titleObj->getPrefixedText() ) {
+			} elseif ( $title !== false && $title !== $titleObj->getPrefixedText() ) {
 				$this->mNormalizedTitles[$title] = $titleObj->getPrefixedText();
 			}
-
-			// Need gender information
-			if (
-				$services->getNamespaceInfo()->
-					hasGenderDistinction( $titleObj->getNamespace() )
-			) {
-				$usernames[] = $titleObj->getText();
-			}
 		}
-		// Get gender information
-		$genderCache = $services->getGenderCache();
-		$genderCache->doQuery( $usernames, __METHOD__ );
 
 		return $linkBatch;
 	}
diff --git a/tests/phpunit/includes/api/ApiPageSetTest.php b/tests/phpunit/includes/api/ApiPageSetTest.php
index b9e4645d52..fdc9c1b2b3 100644
--- a/tests/phpunit/includes/api/ApiPageSetTest.php
+++ b/tests/phpunit/includes/api/ApiPageSetTest.php
@@ -1,5 +1,8 @@
 <?php
 
+use MediaWiki\MediaWikiServices;
+use Wikimedia\TestingAccessWrapper;
+
 /**
  * @group API
  * @group medium
@@ -176,4 +179,44 @@ class ApiPageSetTest extends ApiTestCase {
 			3 => [ "$userDbkey/subpage" => -3 ],
 		], $pageSet->getAllTitlesByNamespace() );
 	}
+
+	/**
+	 * Test that ApiPageSet is calling GenderCache for provided user names to prefill the
+	 * GenderCache and avoid a performance issue when loading each users' gender on it's own.
+	 * The test is setting the "missLimit" to 0 on the GenderCache to trigger misses logic.
+	 * When the "misses" property is no longer 0 at the end of the test,
+	 * something was requested which is not part of the cache. Than the test is failing.
+	 */
+	public function testGenderCaching() {
+		// Set up the user namespace to have gender aliases to trigger the gender cache
+		$this->setMwGlobals( [
+			'wgExtraGenderNamespaces' => [ NS_USER => [ 'male' => 'Male', 'female' => 'Female' ] ]
+		] );
+		$this->overrideMwServices();
+
+		// User names to test with - it is not needed that the user exists in the database
+		// to trigger gender cache
+		$userNames = [
+			'Female',
+			'Unknown',
+			'Male',
+		];
+
+		// Prepare the gender cache for testing - this is a fresh instance due to service override
+		$genderCache = TestingAccessWrapper::newFromObject(
+			MediaWikiServices::getInstance()->getGenderCache()
+		);
+		$genderCache->missLimit = 0;
+
+		// Do an api request to trigger ApiPageSet code
+		$this->doApiRequest( [
+			'action' => 'query',
+			'titles' => 'User:' . implode( '|User:', $userNames ),
+		] );
+
+		$this->assertEquals( 0, $genderCache->misses,
+			'ApiPageSet does not prefill the gender cache correctly' );
+		$this->assertEquals( $userNames, array_keys( $genderCache->cache ),
+			'ApiPageSet does not prefill all users into the gender cache' );
+	}
 }