From: Petr Pchelko <ppchelko@wikimedia.org>
Date: Wed, 18 Sep 2019 00:25:43 +0000 (-0700)
Subject: Use BlobStore::getBlobBatch for RevisionStore::newRevisionsFromBatch
X-Git-Tag: 1.34.0-rc.0~142^2
X-Git-Url: https://git.heureux-cyclage.org/?p=lhc%2Fweb%2Fwiklou.git;a=commitdiff_plain;h=d5a5cf502db2813b1e9c61ff38c4565e9da5482d;hp=-c

Use BlobStore::getBlobBatch for RevisionStore::newRevisionsFromBatch

Bug: T233173
Change-Id: I3ffcfabc3524ce137593e0dba32c83f5eb7e5763
---

d5a5cf502db2813b1e9c61ff38c4565e9da5482d
diff --git a/includes/Revision/RevisionStore.php b/includes/Revision/RevisionStore.php
index 735a2124d4..3ecef76fa0 100644
--- a/includes/Revision/RevisionStore.php
+++ b/includes/Revision/RevisionStore.php
@@ -1623,10 +1623,18 @@ class RevisionStore
 	 * @param object[]|IResultWrapper $slotRows
 	 * @param int $queryFlags
 	 * @param Title $title
+	 * @param array|null $slotContents a map from blobAddress to slot
+	 * 	content blob or Content object.
 	 *
 	 * @return SlotRecord[]
 	 */
-	private function constructSlotRecords( $revId, $slotRows, $queryFlags, Title $title ) {
+	private function constructSlotRecords(
+		$revId,
+		$slotRows,
+		$queryFlags,
+		Title $title,
+		$slotContents = null
+	) {
 		$slots = [];
 
 		foreach ( $slotRows as $row ) {
@@ -1651,8 +1659,15 @@ class RevisionStore
 					= $this->emulateContentId( intval( $row->rev_text_id ) );
 			}
 
-			$contentCallback = function ( SlotRecord $slot ) use ( $queryFlags ) {
-				return $this->loadSlotContent( $slot, null, null, null, $queryFlags );
+			$contentCallback = function ( SlotRecord $slot ) use ( $slotContents, $queryFlags ) {
+				$blob = null;
+				if ( isset( $slotContents[$slot->getAddress()] ) ) {
+					$blob = $slotContents[$slot->getAddress()];
+					if ( $blob instanceof Content ) {
+						return $blob;
+					}
+				}
+				return $this->loadSlotContent( $slot, $blob, null, null, $queryFlags );
 			};
 
 			$slots[$row->role_name] = new SlotRecord( $row, $contentCallback );
@@ -1804,8 +1819,10 @@ class RevisionStore
 
 	/**
 	 * @param object $row A database row generated from a query based on getQueryInfo()
-	 * @param null|object[] $slotRows Database rows generated from a query based on
-	 *        getSlotsQueryInfo with the 'content' flag set.
+	 * @param null|object[]|RevisionSlots $slots
+	 * 	- Database rows generated from a query based on getSlotsQueryInfo
+	 * 	  with the 'content' flag set. Or
+	 *  - RevisionSlots instance
 	 * @param int $queryFlags
 	 * @param Title|null $title
 	 * @param bool $fromCache if true, the returned RevisionRecord will ensure that no stale
@@ -1816,11 +1833,10 @@ class RevisionStore
 	 * @see RevisionFactory::newRevisionFromRow
 	 *
 	 * MCR migration note: this replaces Revision::newFromRow
-	 *
 	 */
 	public function newRevisionFromRowAndSlots(
 		$row,
-		$slotRows,
+		$slots,
 		$queryFlags = 0,
 		Title $title = null,
 		$fromCache = false
@@ -1857,7 +1873,9 @@ class RevisionStore
 		// Legacy because $row may have come from self::selectFields()
 		$comment = $this->commentStore->getCommentLegacy( $db, 'rev_comment', $row, true );
 
-		$slots = $this->newRevisionSlots( $row->rev_id, $row, $slotRows, $queryFlags, $title );
+		if ( !( $slots instanceof RevisionSlots ) ) {
+			$slots = $this->newRevisionSlots( $row->rev_id, $row, $slots, $queryFlags, $title );
+		}
 
 		// If this is a cached row, instantiate a cache-aware revision class to avoid stale data.
 		if ( $fromCache ) {
@@ -1887,7 +1905,7 @@ class RevisionStore
 	 *               loaded immediately. Supports falsy or truthy value as well
 	 *               as an explicit list of slot role names.
 	 *               'content'- whether the actual content of the slots should be
-	 *               preloaded. TODO: no supported yet.
+	 *               preloaded.
 	 * @param int $queryFlags
 	 * @param Title|null $title
 	 * @return StatusValue a status with a RevisionRecord[] of successfully fetched revisions
@@ -1957,24 +1975,40 @@ class RevisionStore
 			}, $options['slots'] );
 		}
 
-		// TODO: Support optional fetching of the content
-		$queryInfo = self::getSlotsQueryInfo( [ 'content' ] );
+		// We need to set the `content` flag because newRevisionFromRowAndSlots requires content
+		// metadata to be loaded.
+		$slotQueryInfo = self::getSlotsQueryInfo( [ 'content' ] );
 		$db = $this->getDBConnectionRefForQueryFlags( $queryFlags );
 		$slotRows = $db->select(
-			$queryInfo['tables'],
-			$queryInfo['fields'],
+			$slotQueryInfo['tables'],
+			$slotQueryInfo['fields'],
 			$slotQueryConds,
 			__METHOD__,
 			[],
-			$queryInfo['joins']
+			$slotQueryInfo['joins']
 		);
 
 		$slotRowsByRevId = [];
 		foreach ( $slotRows as $slotRow ) {
 			$slotRowsByRevId[$slotRow->slot_revision_id][] = $slotRow;
 		}
+
+		$slotContents = null;
+		if ( $options['content'] ?? false ) {
+			$blobAddresses = [];
+			foreach ( $slotRows as $slotRow ) {
+				$blobAddresses[] = $slotRow->content_address;
+			}
+			$slotContentFetchStatus = $this->blobStore
+				->getBlobBatch( $blobAddresses, $queryFlags );
+			foreach ( $slotContentFetchStatus->getErrors() as $error ) {
+				$result->warning( $error['message'], ...$error['params'] );
+			}
+			$slotContents = $slotContentFetchStatus->getValue();
+		}
+
 		$result->setResult( true, array_map( function ( $row ) use
-			( $slotRowsByRevId, $queryFlags, $titlesByPageId, $result ) {
+			( $slotRowsByRevId, $queryFlags, $titlesByPageId, $slotContents, $result ) {
 				if ( !isset( $slotRowsByRevId[$row->rev_id] ) ) {
 					$result->warning(
 						'internalerror',
@@ -1985,7 +2019,15 @@ class RevisionStore
 				try {
 					return $this->newRevisionFromRowAndSlots(
 						$row,
-						$slotRowsByRevId[$row->rev_id],
+						new RevisionSlots(
+							$this->constructSlotRecords(
+								$row->rev_id,
+								$slotRowsByRevId[$row->rev_id],
+								$queryFlags,
+								$titlesByPageId[$row->rev_page],
+								$slotContents
+							)
+						),
 						$queryFlags,
 						$titlesByPageId[$row->rev_page]
 					);
diff --git a/tests/phpunit/includes/Revision/McrRevisionStoreDbTest.php b/tests/phpunit/includes/Revision/McrRevisionStoreDbTest.php
index c0ff44b0b8..e4d6b54105 100644
--- a/tests/phpunit/includes/Revision/McrRevisionStoreDbTest.php
+++ b/tests/phpunit/includes/Revision/McrRevisionStoreDbTest.php
@@ -3,12 +3,17 @@
 namespace MediaWiki\Tests\Revision;
 
 use CommentStoreComment;
+use ContentHandler;
 use MediaWiki\MediaWikiServices;
 use MediaWiki\Revision\MutableRevisionRecord;
 use MediaWiki\Revision\RevisionRecord;
 use MediaWiki\Revision\SlotRecord;
+use MediaWiki\Storage\SqlBlobStore;
+use Revision;
+use StatusValue;
 use TextContent;
 use Title;
+use Wikimedia\TestingAccessWrapper;
 use WikitextContent;
 
 /**
@@ -239,4 +244,49 @@ class McrRevisionStoreDbTest extends RevisionStoreDbTestBase {
 			]
 		] ], $result->getErrors() );
 	}
+
+	/**
+	 * @covers \MediaWiki\Revision\RevisionStore::newRevisionsFromBatch
+	 */
+	public function testNewRevisionFromBatchUsesGetBlobBatch() {
+		$page1 = $this->getTestPage();
+		$text = __METHOD__ . 'b-ä';
+		$editStatus = $this->editPage( $page1->getTitle()->getPrefixedDBkey(), $text . '1' );
+		$this->assertTrue( $editStatus->isGood(), 'Sanity: must create revision 1' );
+		/** @var Revision $rev1 */
+		$rev1 = $editStatus->getValue()['revision'];
+
+		$contentAddress = $rev1->getRevisionRecord()->getSlot( SlotRecord::MAIN )->getAddress();
+		$mockBlobStore = $this->getMockBuilder( SqlBlobStore::class )
+			->disableOriginalConstructor()
+			->getMock();
+		$mockBlobStore
+			->expects( $this->once() )
+			->method( 'getBlobBatch' )
+			->with( [ $contentAddress ], $this->anything() )
+			->willReturn( StatusValue::newGood( [
+				$contentAddress => 'Content_From_Mock'
+			] ) );
+		$mockBlobStore
+			->expects( $this->never() )
+			->method( 'getBlob' );
+
+		$revStore = MediaWikiServices::getInstance()
+			->getRevisionStoreFactory()
+			->getRevisionStore();
+		$wrappedRevStore = TestingAccessWrapper::newFromObject( $revStore );
+		$wrappedRevStore->blobStore = $mockBlobStore;
+
+		$result = $revStore->newRevisionsFromBatch(
+			[ $this->revisionToRow( $rev1 ) ],
+			[
+				'slots' => [ SlotRecord::MAIN ],
+				'content' => true
+			]
+		);
+		$this->assertTrue( $result->isGood() );
+		$this->assertSame( 'Content_From_Mock',
+			ContentHandler::getContentText( $result->getValue()[$rev1->getId()]
+				->getContent( SlotRecord::MAIN ) ) );
+	}
 }
diff --git a/tests/phpunit/includes/Revision/RevisionStoreDbTestBase.php b/tests/phpunit/includes/Revision/RevisionStoreDbTestBase.php
index 76190e0a9b..4040ffc76e 100644
--- a/tests/phpunit/includes/Revision/RevisionStoreDbTestBase.php
+++ b/tests/phpunit/includes/Revision/RevisionStoreDbTestBase.php
@@ -4,8 +4,10 @@ namespace MediaWiki\Tests\Revision;
 
 use CommentStoreComment;
 use Content;
+use ContentHandler;
 use Exception;
 use HashBagOStuff;
+use IDBAccessObject;
 use InvalidArgumentException;
 use Language;
 use MediaWiki\Linker\LinkTarget;
@@ -105,7 +107,7 @@ abstract class RevisionStoreDbTestBase extends MediaWikiTestCase {
 	 * @return WikiPage
 	 */
 	protected function getTestPage( $pageTitle = null ) {
-		if ( !is_null( $pageTitle ) && $this->testPage ) {
+		if ( is_null( $pageTitle ) && $this->testPage ) {
 			return $this->testPage;
 		}
 
@@ -1992,23 +1994,17 @@ abstract class RevisionStoreDbTestBase extends MediaWikiTestCase {
 	) {
 		$page1 = $this->getTestPage();
 		$text = __METHOD__ . 'b-ä';
+		$editStatus = $this->editPage( $page1->getTitle()->getPrefixedDBkey(), $text . '1' );
+		$this->assertTrue( $editStatus->isGood(), 'Sanity: must create revision 1' );
 		/** @var Revision $rev1 */
-		$rev1 = $page1->doEditContent(
-			new WikitextContent( $text . '1' ),
-			__METHOD__ . 'b',
-			0,
-			false,
-			$this->getTestUser()->getUser()
-		)->value['revision'];
+		$rev1 = $editStatus->getValue()['revision'];
+
 		$page2 = $this->getTestPage( $otherPageTitle );
+		$editStatus = $this->editPage( $page2->getTitle()->getPrefixedDBkey(), $text . '2' );
+		$this->assertTrue( $editStatus->isGood(), 'Sanity: must create revision 2' );
 		/** @var Revision $rev2 */
-		$rev2 = $page2->doEditContent(
-			new WikitextContent( $text . '2' ),
-			__METHOD__ . 'b',
-			0,
-			false,
-			$this->getTestUser()->getUser()
-		)->value['revision'];
+		$rev2 = $editStatus->getValue()['revision'];
+
 		$store = MediaWikiServices::getInstance()->getRevisionStore();
 		$result = $store->newRevisionsFromBatch(
 			[ $this->revisionToRow( $rev1 ), $this->revisionToRow( $rev2 ) ],
@@ -2016,14 +2012,15 @@ abstract class RevisionStoreDbTestBase extends MediaWikiTestCase {
 		);
 		$this->assertTrue( $result->isGood() );
 		$this->assertEmpty( $result->getErrors() );
+		/** @var RevisionRecord[] $records */
 		$records = $result->getValue();
 		$this->assertRevisionRecordMatchesRevision( $rev1, $records[$rev1->getId()] );
 		$this->assertRevisionRecordMatchesRevision( $rev2, $records[$rev2->getId()] );
 
 		$this->assertSame( $text . '1',
-			$records[$rev1->getId()]->getContent( SlotRecord::MAIN )->serialize() );
+			ContentHandler::getContentText( $records[$rev1->getId()]->getContent( SlotRecord::MAIN ) ) );
 		$this->assertSame( $text . '2',
-			$records[$rev2->getId()]->getContent( SlotRecord::MAIN )->serialize() );
+			ContentHandler::getContentText( $records[$rev2->getId()]->getContent( SlotRecord::MAIN ) ) );
 		$this->assertEquals( $page1->getTitle()->getDBkey(),
 			$records[$rev1->getId()]->getPageAsLinkTarget()->getDBkey() );
 		$this->assertEquals( $page2->getTitle()->getDBkey(),
@@ -2046,4 +2043,41 @@ abstract class RevisionStoreDbTestBase extends MediaWikiTestCase {
 		$this->assertEmpty( $result->getValue() );
 		$this->assertEmpty( $result->getErrors() );
 	}
+
+	/**
+	 * @covers \MediaWiki\Revision\RevisionStore::newRevisionsFromBatch
+	 */
+	public function testNewRevisionsFromBatch_wrongTitle() {
+		$page1 = $this->getTestPage();
+		$text = __METHOD__ . 'b-ä';
+		$editStatus = $this->editPage( $page1->getTitle()->getPrefixedDBkey(), $text . '1' );
+		$this->assertTrue( $editStatus->isGood(), 'Sanity: must create revision 1' );
+		/** @var Revision $rev1 */
+		$rev1 = $editStatus->getValue()['revision'];
+
+		$this->setExpectedException( InvalidArgumentException::class );
+		MediaWikiServices::getInstance()->getRevisionStore()
+			->newRevisionsFromBatch(
+				[ $this->revisionToRow( $rev1 ) ],
+				[],
+				IDBAccessObject::READ_NORMAL,
+				$this->getTestPage( 'Title_Other_Then_The_One_Revision_Belongs_To' )->getTitle()
+			);
+	}
+
+	/**
+	 * @covers \MediaWiki\Revision\RevisionStore::newRevisionsFromBatch
+	 */
+	public function testNewRevisionsFromBatch_DuplicateRows() {
+		$page1 = $this->getTestPage();
+		$text = __METHOD__ . 'b-ä';
+		$editStatus = $this->editPage( $page1->getTitle()->getPrefixedDBkey(), $text . '1' );
+		$this->assertTrue( $editStatus->isGood(), 'Sanity: must create revision 1' );
+		/** @var Revision $rev1 */
+		$rev1 = $editStatus->getValue()['revision'];
+
+		$this->setExpectedException( InvalidArgumentException::class );
+		MediaWikiServices::getInstance()->getRevisionStore()
+			->newRevisionsFromBatch( [ $this->revisionToRow( $rev1 ), $this->revisionToRow( $rev1 ) ] );
+	}
 }