[MCR] RevisionStore::getTitle final logged fallback to master

There have been many issues with RevisionStore and titles due
to code paths that already know the title for a Revision not
passing the title into Revision in various ways or not passing
in the correct queryFlags.
The getTitle method now has a further fallback using Title::newFromID
and Title::GAID_FOR_UPDATE if not already attempted.

Bug: T183548
Bug: T183716
Bug: T183717
Bug: T183550
Bug: T183505
Bug: T184559
Bug: T184595
Change-Id: I6cf13e6baba354b08533a6151bbbc88a317be9d6

diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php
index a89619f..a7f0c9b 100644 (file)
--- a/includes/ServiceWiring.php
+++ b/includes/ServiceWiring.php
@@ -476,6 +476,8 @@ return [
                        $services->getMainWANObjectCache()
                );
 
+               $store->setLogger( LoggerFactory::getInstance( 'RevisionStore' ) );
+
                $config = $services->getMainConfig();
                $store->setContentHandlerUseDB( $config->get( 'ContentHandlerUseDB' ) );
 

diff --git a/includes/Storage/RevisionStore.php b/includes/Storage/RevisionStore.php
index 79ecec6..42e7933 100644 (file)
--- a/includes/Storage/RevisionStore.php
+++ b/includes/Storage/RevisionStore.php
@@ -42,6 +42,9 @@ use MediaWiki\User\UserIdentityValue;
 use Message;
 use MWException;
 use MWUnknownContentModelException;
+use Psr\Log\LoggerAwareInterface;
+use Psr\Log\LoggerInterface;
+use Psr\Log\NullLogger;
 use RecentChange;
 use stdClass;
 use Title;
@@ -61,7 +64,8 @@ use Wikimedia\Rdbms\LoadBalancer;
  * @note This was written to act as a drop-in replacement for the corresponding
  *       static methods in Revision.
  */
-class RevisionStore implements IDBAccessObject, RevisionFactory, RevisionLookup {
+class RevisionStore
+       implements IDBAccessObject, RevisionFactory, RevisionLookup, LoggerAwareInterface {
 
        /**
         * @var SqlBlobStore
@@ -88,6 +92,11 @@ class RevisionStore implements IDBAccessObject, RevisionFactory, RevisionLookup
         */
        private $cache;
 
+       /**
+        * @var LoggerInterface
+        */
+       private $logger;
+
        /**
         * @todo $blobStore should be allowed to be any BlobStore!
         *
@@ -108,6 +117,11 @@ class RevisionStore implements IDBAccessObject, RevisionFactory, RevisionLookup
                $this->blobStore = $blobStore;
                $this->cache = $cache;
                $this->wikiId = $wikiId;
+               $this->logger = new NullLogger();
+       }
+
+       public function setLogger( LoggerInterface $logger ) {
+               $this->logger = $logger;
        }
 
        /**
@@ -173,23 +187,34 @@ class RevisionStore implements IDBAccessObject, RevisionFactory, RevisionLookup
         * @return Title
         * @throws RevisionAccessException
         */
-       public function getTitle( $pageId, $revId, $queryFlags = 0 ) {
+       public function getTitle( $pageId, $revId, $queryFlags = self::READ_NORMAL ) {
                if ( !$pageId && !$revId ) {
                        throw new InvalidArgumentException( '$pageId and $revId cannot both be 0 or null' );
                }
 
-               list( $dbMode, $dbOptions, , ) = DBAccessObjectUtils::getDBOptions( $queryFlags );
-               $titleFlags = $dbMode == DB_MASTER ? Title::GAID_FOR_UPDATE : 0;
-               $title = null;
+               // This method recalls itself with READ_LATEST if READ_NORMAL doesn't get us a Title
+               // So ignore READ_LATEST_IMMUTABLE flags and handle the fallback logic in this method
+               if ( DBAccessObjectUtils::hasFlags( $queryFlags, self::READ_LATEST_IMMUTABLE ) ) {
+                       $queryFlags = self::READ_NORMAL;
+               }
+
+               $canUseTitleNewFromId = ( $pageId !== null && $pageId > 0 && $this->wikiId === false );
+               list( $dbMode, $dbOptions ) = DBAccessObjectUtils::getDBOptions( $queryFlags );
+               $titleFlags = ( $dbMode == DB_MASTER ? Title::GAID_FOR_UPDATE : 0 );
 
                // Loading by ID is best, but Title::newFromID does not support that for foreign IDs.
-               if ( $pageId !== null && $pageId > 0 && $this->wikiId === false ) {
+               if ( $canUseTitleNewFromId ) {
                        // TODO: better foreign title handling (introduce TitleFactory)
                        $title = Title::newFromID( $pageId, $titleFlags );
+                       if ( $title ) {
+                               return $title;
+                       }
                }
 
                // rev_id is defined as NOT NULL, but this revision may not yet have been inserted.
-               if ( !$title && $revId !== null && $revId > 0 ) {
+               $canUseRevId = ( $revId !== null && $revId > 0 );
+
+               if ( $canUseRevId ) {
                        $dbr = $this->getDBConnectionRef( $dbMode );
                        // @todo: Title::getSelectFields(), or Title::getQueryInfo(), or something like that
                        $row = $dbr->selectRow(
@@ -209,17 +234,25 @@ class RevisionStore implements IDBAccessObject, RevisionFactory, RevisionLookup
                        );
                        if ( $row ) {
                                // TODO: better foreign title handling (introduce TitleFactory)
-                               $title = Title::newFromRow( $row );
+                               return Title::newFromRow( $row );
                        }
                }
 
-               if ( !$title ) {
-                       throw new RevisionAccessException(
-                               "Could not determine title for page ID $pageId and revision ID $revId"
-                       );
+               // If we still don't have a title, fallback to master if that wasn't already happening.
+               if ( $dbMode !== DB_MASTER ) {
+                       $title = $this->getTitle( $pageId, $revId, self::READ_LATEST );
+                       if ( $title ) {
+                               $this->logger->info(
+                                       __METHOD__ . ' fell back to READ_LATEST and got a Title.',
+                                       [ 'trace' => wfDebugBacktrace() ]
+                               );
+                               return $title;
+                       }
                }
 
-               return $title;
+               throw new RevisionAccessException(
+                       "Could not determine title for page ID $pageId and revision ID $revId"
+               );
        }
 
        /**

diff --git a/tests/phpunit/includes/Storage/RevisionStoreTest.php b/tests/phpunit/includes/Storage/RevisionStoreTest.php
index c9e9978..d9e9b06 100644 (file)
--- a/tests/phpunit/includes/Storage/RevisionStoreTest.php
+++ b/tests/phpunit/includes/Storage/RevisionStoreTest.php
@@ -335,6 +335,62 @@ class RevisionStoreTest extends MediaWikiTestCase {
                $this->assertSame( 'Food', $title->getDBkey() );
        }
 
+       public function testGetTitle_successFromPageIdOnFallback() {
+               $mockLoadBalancer = $this->getMockLoadBalancer();
+               // Title calls wfGetDB() so we have to set the main service
+               $this->setService( 'DBLoadBalancer', $mockLoadBalancer );
+
+               $db = $this->getMockDatabase();
+               // Title calls wfGetDB() which uses a regular Connection
+               // Assert that the first call uses a REPLICA and the second falls back to master
+               $mockLoadBalancer->expects( $this->exactly( 2 ) )
+                       ->method( 'getConnection' )
+                       ->willReturn( $db );
+               // RevisionStore getTitle uses a ConnectionRef
+               $mockLoadBalancer->expects( $this->atLeastOnce() )
+                       ->method( 'getConnectionRef' )
+                       ->willReturn( $db );
+
+               // First call to Title::newFromID, faking no result (db lag?)
+               $db->expects( $this->at( 0 ) )
+                       ->method( 'selectRow' )
+                       ->with(
+                               'page',
+                               $this->anything(),
+                               [ 'page_id' => 1 ]
+                       )
+                       ->willReturn( false );
+
+               // First select using rev_id, faking no result (db lag?)
+               $db->expects( $this->at( 1 ) )
+                       ->method( 'selectRow' )
+                       ->with(
+                               [ 'revision', 'page' ],
+                               $this->anything(),
+                               [ 'rev_id' => 2 ]
+                       )
+                       ->willReturn( false );
+
+               // Second call to Title::newFromID, no result
+               $db->expects( $this->at( 2 ) )
+                       ->method( 'selectRow' )
+                       ->with(
+                               'page',
+                               $this->anything(),
+                               [ 'page_id' => 1 ]
+                       )
+                       ->willReturn( (object)[
+                               'page_namespace' => '2',
+                               'page_title' => 'Foodey',
+                       ] );
+
+               $store = $this->getRevisionStore( $mockLoadBalancer );
+               $title = $store->getTitle( 1, 2, RevisionStore::READ_NORMAL );
+
+               $this->assertSame( 2, $title->getNamespace() );
+               $this->assertSame( 'Foodey', $title->getDBkey() );
+       }
+
        public function testGetTitle_successFromRevId() {
                $mockLoadBalancer = $this->getMockLoadBalancer();
                // Title calls wfGetDB() so we have to set the main service
@@ -380,17 +436,15 @@ class RevisionStoreTest extends MediaWikiTestCase {
                $this->assertSame( 'Food2', $title->getDBkey() );
        }
 
-       /**
-        * @covers \MediaWiki\Storage\RevisionStore::getTitle
-        */
-       public function testGetTitle_throwsExceptionAfterFallbacks() {
+       public function testGetTitle_successFromRevIdOnFallback() {
                $mockLoadBalancer = $this->getMockLoadBalancer();
                // Title calls wfGetDB() so we have to set the main service
                $this->setService( 'DBLoadBalancer', $mockLoadBalancer );
 
                $db = $this->getMockDatabase();
                // Title calls wfGetDB() which uses a regular Connection
-               $mockLoadBalancer->expects( $this->atLeastOnce() )
+               // Assert that the first call uses a REPLICA and the second falls back to master
+               $mockLoadBalancer->expects( $this->exactly( 2 ) )
                        ->method( 'getConnection' )
                        ->willReturn( $db );
                // RevisionStore getTitle uses a ConnectionRef
@@ -418,6 +472,88 @@ class RevisionStoreTest extends MediaWikiTestCase {
                        )
                        ->willReturn( false );
 
+               // Second call to Title::newFromID, no result
+               $db->expects( $this->at( 2 ) )
+                       ->method( 'selectRow' )
+                       ->with(
+                               'page',
+                               $this->anything(),
+                               [ 'page_id' => 1 ]
+                       )
+                       ->willReturn( false );
+
+               // Second select using rev_id, result
+               $db->expects( $this->at( 3 ) )
+                       ->method( 'selectRow' )
+                       ->with(
+                               [ 'revision', 'page' ],
+                               $this->anything(),
+                               [ 'rev_id' => 2 ]
+                       )
+                       ->willReturn( (object)[
+                               'page_namespace' => '2',
+                               'page_title' => 'Foodey',
+                       ] );
+
+               $store = $this->getRevisionStore( $mockLoadBalancer );
+               $title = $store->getTitle( 1, 2, RevisionStore::READ_NORMAL );
+
+               $this->assertSame( 2, $title->getNamespace() );
+               $this->assertSame( 'Foodey', $title->getDBkey() );
+       }
+
+       /**
+        * @covers \MediaWiki\Storage\RevisionStore::getTitle
+        */
+       public function testGetTitle_correctFallbackAndthrowsExceptionAfterFallbacks() {
+               $mockLoadBalancer = $this->getMockLoadBalancer();
+               // Title calls wfGetDB() so we have to set the main service
+               $this->setService( 'DBLoadBalancer', $mockLoadBalancer );
+
+               $db = $this->getMockDatabase();
+               // Title calls wfGetDB() which uses a regular Connection
+               // Assert that the first call uses a REPLICA and the second falls back to master
+
+               // RevisionStore getTitle uses getConnectionRef
+               // Title::newFromID uses getConnection
+               foreach ( [ 'getConnection', 'getConnectionRef' ] as $method ) {
+                       $mockLoadBalancer->expects( $this->exactly( 2 ) )
+                               ->method( $method )
+                               ->willReturnCallback( function ( $masterOrReplica ) use ( $db ) {
+                                       static $callCounter = 0;
+                                       $callCounter++;
+                                       // The first call should be to a REPLICA, and the second a MASTER.
+                                       if ( $callCounter === 1 ) {
+                                               $this->assertSame( DB_REPLICA, $masterOrReplica );
+                                       } elseif ( $callCounter === 2 ) {
+                                               $this->assertSame( DB_MASTER, $masterOrReplica );
+                                       }
+                                       return $db;
+                               } );
+               }
+               // First and third call to Title::newFromID, faking no result
+               foreach ( [ 0, 2 ] as $counter ) {
+                       $db->expects( $this->at( $counter ) )
+                               ->method( 'selectRow' )
+                               ->with(
+                                       'page',
+                                       $this->anything(),
+                                       [ 'page_id' => 1 ]
+                               )
+                               ->willReturn( false );
+               }
+
+               foreach ( [ 1, 3 ] as $counter ) {
+                       $db->expects( $this->at( $counter ) )
+                               ->method( 'selectRow' )
+                               ->with(
+                                       [ 'revision', 'page' ],
+                                       $this->anything(),
+                                       [ 'rev_id' => 2 ]
+                               )
+                               ->willReturn( false );
+               }
+
                $store = $this->getRevisionStore( $mockLoadBalancer );
 
                $this->setExpectedException( RevisionAccessException::class );