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 6afb018..e68676a 100644 (file)
--- 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 b9e4645..fdc9c1b 100644 (file)
--- 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' );
+       }
 }