From: addshore Date: Wed, 10 Jan 2018 16:05:46 +0000 (+0000) Subject: [MCR] RevisionStore::getTitle final logged fallback to master X-Git-Tag: 1.31.0-rc.0~691^2 X-Git-Url: http://git.cyclocoop.org/data/Luca_Pacioli_%28Gemaelde%29.jpeg?a=commitdiff_plain;h=9a509a1792f1ac5c8aa3e16878c616b3be00110e;p=lhc%2Fweb%2Fwiklou.git [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 a89619f273..a7f0c9beea 100644 --- 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 79ecec691f..42e7933959 100644 --- 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 c9e99780e1..d9e9b068fe 100644 --- 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 );