From 125bf7e44fe4145127c9e19c25d8c0c6dc1a6c3e Mon Sep 17 00:00:00 2001 From: addshore Date: Mon, 29 Jan 2018 15:54:02 +0000 Subject: [PATCH] [MCR] RevisionStore, enable insertions for new schema Enable inserts to the new MCR db schema in single slot mode only. TODO: - RELEASE NOTES Notes: - When in MIGRATION_WRITE_NEW or greater, deleting and then restoring a page will result in different data in the revision table. For example, if you delete a page that has text_ids present in the revision table and restore it, the text_ids will be blank after. - When in MIGRATION_WRITE_BOTH or greater the archive table will start to ar_content_model entries where previously it would have been given NULL. This is due to the old content schema having NULL in the db when the default content model is used, but the new schema will always have a value, taken from the content_models table Note: If259b1e1c49ce was squashed into this change. Bug: T183488 Bug: T174024 Change-Id: Ic2221da30c8f6ac2ba42720fcd568f2d0ed70534 --- includes/DefaultSettings.php | 11 + includes/ServiceWiring.php | 3 + includes/Storage/RevisionStore.php | 322 ++++++++++++++---- includes/Storage/SlotRecord.php | 44 ++- includes/page/WikiPage.php | 25 +- tests/common/TestsAutoLoader.php | 1 + tests/phpunit/MediaWikiTestCase.php | 6 +- tests/phpunit/includes/RevisionDbTestBase.php | 5 +- .../includes/RevisionMcrWriteBothDbTest.php | 23 ++ tests/phpunit/includes/RevisionTest.php | 80 ++++- .../McrWriteBothRevisionStoreDbTest.php | 114 +++++++ .../Storage/McrWriteBothSchemaOverride.php | 58 ++++ .../Storage/RevisionStoreDbTestBase.php | 6 + .../includes/Storage/RevisionStoreTest.php | 261 +++++--------- .../includes/Storage/SlotRecordTest.php | 12 +- .../page/WikiPageMcrWriteBothDbTest.php | 23 ++ 16 files changed, 745 insertions(+), 249 deletions(-) create mode 100644 tests/phpunit/includes/RevisionMcrWriteBothDbTest.php create mode 100644 tests/phpunit/includes/Storage/McrWriteBothRevisionStoreDbTest.php create mode 100644 tests/phpunit/includes/Storage/McrWriteBothSchemaOverride.php create mode 100644 tests/phpunit/includes/page/WikiPageMcrWriteBothDbTest.php diff --git a/includes/DefaultSettings.php b/includes/DefaultSettings.php index 56fb534754..02cbc2fb04 100644 --- a/includes/DefaultSettings.php +++ b/includes/DefaultSettings.php @@ -8893,6 +8893,17 @@ $wgInterwikiPrefixDisplayTypes = []; */ $wgCommentTableSchemaMigrationStage = MIGRATION_OLD; +/** + * RevisionStore table schema migration stage (content, slots, content_models & slot_roles tables) + * + * @see Task: https://phabricator.wikimedia.org/T174028 + * @see Commit: https://gerrit.wikimedia.org/r/#/c/378724/ + * + * @since 1.32 + * @var int One of the MIGRATION_* constants + */ +$wgMultiContentRevisionSchemaMigrationStage = MIGRATION_OLD; + /** * Actor table schema migration stage. * @since 1.31 diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php index 3f8ba185d5..379424cf14 100644 --- a/includes/ServiceWiring.php +++ b/includes/ServiceWiring.php @@ -471,6 +471,9 @@ return [ $blobStore, $services->getMainWANObjectCache(), $services->getCommentStore(), + $services->getContentModelStore(), + $services->getSlotRoleStore(), + $services->getMainConfig()->get( 'MultiContentRevisionSchemaMigrationStage' ), $services->getActorMigration() ); diff --git a/includes/Storage/RevisionStore.php b/includes/Storage/RevisionStore.php index ce09a6e768..3f852b08b1 100644 --- a/includes/Storage/RevisionStore.php +++ b/includes/Storage/RevisionStore.php @@ -69,6 +69,8 @@ use Wikimedia\Rdbms\LoadBalancer; class RevisionStore implements IDBAccessObject, RevisionFactory, RevisionLookup, LoggerAwareInterface { + const ROW_CACHE_KEY = 'revision-row-1.29'; + /** * @var SqlBlobStore */ @@ -109,6 +111,19 @@ class RevisionStore */ private $logger; + /** + * @var NameTableStore + */ + private $contentModelStore; + + /** + * @var NameTableStore + */ + private $slotRoleStore; + + /** @var int One of the MIGRATION_* constants */ + private $mcrMigrationStage; + /** * @todo $blobStore should be allowed to be any BlobStore! * @@ -116,6 +131,9 @@ class RevisionStore * @param SqlBlobStore $blobStore * @param WANObjectCache $cache * @param CommentStore $commentStore + * @param NameTableStore $contentModelStore + * @param NameTableStore $slotRoleStore + * @param int $migrationStage * @param ActorMigration $actorMigration * @param bool|string $wikiId */ @@ -124,15 +142,26 @@ class RevisionStore SqlBlobStore $blobStore, WANObjectCache $cache, CommentStore $commentStore, + NameTableStore $contentModelStore, + NameTableStore $slotRoleStore, + $migrationStage, ActorMigration $actorMigration, $wikiId = false ) { Assert::parameterType( 'string|boolean', $wikiId, '$wikiId' ); + Assert::parameterType( 'integer', $migrationStage, '$migrationStage' ); + + if ( $migrationStage > MIGRATION_WRITE_BOTH ) { + throw new InvalidArgumentException( 'New schema is not fully supported yet' ); + } $this->loadBalancer = $loadBalancer; $this->blobStore = $blobStore; $this->cache = $cache; $this->commentStore = $commentStore; + $this->contentModelStore = $contentModelStore; + $this->slotRoleStore = $slotRoleStore; + $this->mcrMigrationStage = $migrationStage; $this->actorMigration = $actorMigration; $this->wikiId = $wikiId; $this->logger = new NullLogger(); @@ -158,8 +187,14 @@ class RevisionStore /** * @param bool $contentHandlerUseDB + * @throws MWException */ public function setContentHandlerUseDB( $contentHandlerUseDB ) { + if ( !$contentHandlerUseDB && $this->mcrMigrationStage > MIGRATION_OLD ) { + throw new MWException( + 'Content model must be stored in the database for multi content revision migration.' + ); + } $this->contentHandlerUseDB = $contentHandlerUseDB; } @@ -334,6 +369,7 @@ class RevisionStore throw new InvalidArgumentException( 'At least one slot needs to be defined!' ); } + // RevisionStore currently only supports writing a single slot if ( $rev->getSlotRoles() !== [ 'main' ] ) { throw new InvalidArgumentException( 'Only the main slot is supported for now!' ); } @@ -347,13 +383,15 @@ class RevisionStore : $rev->getParentId(); // Record the text (or external storage URL) to the blob store - $slot = $rev->getSlot( 'main', RevisionRecord::RAW ); + $mainSlot = $rev->getSlot( 'main', RevisionRecord::RAW ); $size = $this->failOnNull( $rev->getSize(), 'size field' ); $sha1 = $this->failOnEmpty( $rev->getSha1(), 'sha1 field' ); - if ( !$slot->hasAddress() ) { - $content = $slot->getContent(); + $dbw->startAtomic( __METHOD__ ); + + if ( !$mainSlot->hasAddress() ) { + $content = $mainSlot->getContent(); $format = $content->getDefaultFormat(); $model = $content->getModel(); @@ -368,18 +406,18 @@ class RevisionStore $blobHints = [ BlobStore::DESIGNATION_HINT => 'page-content', // BlobStore may be used for other things too. BlobStore::PAGE_HINT => $pageId, - BlobStore::ROLE_HINT => $slot->getRole(), + BlobStore::ROLE_HINT => $mainSlot->getRole(), BlobStore::PARENT_HINT => $parentId, - BlobStore::SHA1_HINT => $slot->getSha1(), + BlobStore::SHA1_HINT => $mainSlot->getSha1(), BlobStore::MODEL_HINT => $model, BlobStore::FORMAT_HINT => $format, ]; $blobAddress = $this->blobStore->storeBlob( $data, $blobHints ); } else { - $blobAddress = $slot->getAddress(); - $model = $slot->getModel(); - $format = $slot->getFormat(); + $blobAddress = $mainSlot->getAddress(); + $model = $mainSlot->getModel(); + $format = $mainSlot->getFormat(); } $textId = $this->blobStore->getTextIdFromAddress( $blobAddress ); @@ -402,11 +440,10 @@ class RevisionStore $this->failOnNull( $user->getId(), 'user field' ); $this->failOnEmpty( $user->getName(), 'user_text field' ); - # Record the edit in revisions - $row = [ + // Record the edit in revisions + $revisionRow = [ 'rev_page' => $pageId, 'rev_parent_id' => $parentId, - 'rev_text_id' => $textId, 'rev_minor_edit' => $rev->isMinor() ? 1 : 0, 'rev_timestamp' => $dbw->timestamp( $timestamp ), 'rev_deleted' => $rev->getVisibility(), @@ -416,54 +453,106 @@ class RevisionStore if ( $rev->getId() !== null ) { // Needed to restore revisions with their original ID - $row['rev_id'] = $rev->getId(); + $revisionRow['rev_id'] = $rev->getId(); } list( $commentFields, $commentCallback ) = $this->commentStore->insertWithTempTable( $dbw, 'rev_comment', $comment ); - $row += $commentFields; + $revisionRow += $commentFields; list( $actorFields, $actorCallback ) = $this->actorMigration->getInsertValuesWithTempTable( $dbw, 'rev_user', $user ); - $row += $actorFields; + $revisionRow += $actorFields; + + if ( $this->mcrMigrationStage <= MIGRATION_WRITE_BOTH ) { + $revisionRow['rev_text_id'] = $textId; - if ( $this->contentHandlerUseDB ) { // MCR migration note: rev_content_model and rev_content_format will go away + if ( $this->contentHandlerUseDB ) { + $defaultModel = ContentHandler::getDefaultModelFor( $title ); + $defaultFormat = ContentHandler::getForModelID( $defaultModel )->getDefaultFormat(); - $defaultModel = ContentHandler::getDefaultModelFor( $title ); - $defaultFormat = ContentHandler::getForModelID( $defaultModel )->getDefaultFormat(); + $revisionRow['rev_content_model'] = ( $model === $defaultModel ) ? null : $model; + $revisionRow['rev_content_format'] = ( $format === $defaultFormat ) ? null : $format; + } + } else { + /** + * rev_text_id has NOT NULL and no DEFAULT, so set to 0 when we are not writing to it. + * WARNING: This should NOT be removed after migration until a schema change has been + * made in WMF production giving rev_text_id a DEFAULT value of 0 (otherwise inserts + * will fail) + * Task: https://phabricator.wikimedia.org/T190148#4064625 + */ + $revisionRow['rev_text_id'] = 0; + } + + $dbw->insert( 'revision', $revisionRow, __METHOD__ ); + + $hasSlots = false; + $contentId = false; + + if ( isset( $revisionRow['rev_id'] ) ) { + // Restoring a revision, slots should already exist, + // unless the archive row wasn't migrated yet. + if ( $this->mcrMigrationStage === MIGRATION_NEW ) { + $hasSlots = true; + } else { + $contentId = $this->findSlotContentId( $dbw, $revisionRow['rev_id'], 'main' ); + $hasSlots = (bool)$contentId; + } + } else { + // not restoring a revision, use auto-increment value + $revisionRow['rev_id'] = intval( $dbw->insertId() ); + } - $row['rev_content_model'] = ( $model === $defaultModel ) ? null : $model; - $row['rev_content_format'] = ( $format === $defaultFormat ) ? null : $format; + if ( $this->mcrMigrationStage > MIGRATION_OLD && $mainSlot->hasContentId() ) { + // re-use content row of inherited slot! + $contentId = $mainSlot->getContentId(); } - $dbw->insert( 'revision', $row, __METHOD__ ); + $revisionId = $revisionRow['rev_id']; - if ( !isset( $row['rev_id'] ) ) { - // only if auto-increment was used - $row['rev_id'] = intval( $dbw->insertId() ); - } - $commentCallback( $row['rev_id'] ); - $actorCallback( $row['rev_id'], $row ); + $commentCallback( $revisionId ); + $actorCallback( $revisionId, $revisionRow ); // Insert IP revision into ip_changes for use when querying for a range. if ( $user->getId() === 0 && IP::isValid( $user->getName() ) ) { $ipcRow = [ - 'ipc_rev_id' => $row['rev_id'], - 'ipc_rev_timestamp' => $row['rev_timestamp'], + 'ipc_rev_id' => $revisionId, + 'ipc_rev_timestamp' => $revisionRow['rev_timestamp'], 'ipc_hex' => IP::toHex( $user->getName() ), ]; $dbw->insert( 'ip_changes', $ipcRow, __METHOD__ ); } - $newSlot = SlotRecord::newSaved( $row['rev_id'], $textId, $blobAddress, $slot ); + if ( $this->mcrMigrationStage >= MIGRATION_WRITE_BOTH ) { + + // Only insert slot rows for new revisions (not restored revisions). + // Also, never insert content rows if not inserting slot rows. + if ( !$hasSlots ) { + + // Only insert content rows for new content (not inherited content) + if ( !$contentId ) { + Assert::invariant( !$hasSlots, 'Re-using slots, but not content ID is known' ); + $contentId = $this->insertContentRowOn( $mainSlot, $dbw, $blobAddress ); + } + + $this->insertSlotRowOn( $mainSlot, $dbw, $revisionId, $contentId ); + } + } else { + $contentId = null; + } + + $dbw->endAtomic( __METHOD__ ); + + $newSlot = SlotRecord::newSaved( $revisionId, $contentId, $blobAddress, $mainSlot ); $slots = new RevisionSlots( [ 'main' => $newSlot ] ); $rev = new RevisionStoreRecord( $title, $user, $comment, - (object)$row, + (object)$revisionRow, $slots, $this->wikiId ); @@ -485,7 +574,7 @@ class RevisionStore Assert::postcondition( $newSlot !== null, 'revision must have a main slot' ); Assert::postcondition( $newSlot->getAddress() !== null, - 'main slot must have an addess' + 'main slot must have an address' ); Hooks::run( 'RevisionRecordInserted', [ $rev ] ); @@ -497,6 +586,41 @@ class RevisionStore return $rev; } + /** + * @param SlotRecord $slot + * @param IDatabase $dbw + * @param int $revisionId + * @param int $contentId + */ + private function insertSlotRowOn( SlotRecord $slot, IDatabase $dbw, $revisionId, $contentId ) { + $slotRow = [ + 'slot_revision_id' => $revisionId, + 'slot_role_id' => $this->slotRoleStore->acquireId( $slot->getRole() ), + 'slot_content_id' => $contentId, + // If the slot has a specific origin use that ID, otherwise use the ID of the revision + // that we just inserted. + 'slot_origin' => $slot->hasOrigin() ? $slot->getOrigin() : $revisionId, + ]; + $dbw->insert( 'slots', $slotRow, __METHOD__ ); + } + + /** + * @param SlotRecord $slot + * @param IDatabase $dbw + * @param string $blobAddress + * @return int content row ID + */ + private function insertContentRowOn( SlotRecord $slot, IDatabase $dbw, $blobAddress ) { + $contentRow = [ + 'content_size' => $slot->getSize(), + 'content_sha1' => $slot->getSha1(), + 'content_model' => $this->contentModelStore->acquireId( $slot->getModel() ), + 'content_address' => $blobAddress, + ]; + $dbw->insert( 'content', $contentRow, __METHOD__ ); + return intval( $dbw->insertId() ); + } + /** * MCR migration note: this corresponds to Revision::checkContentModel * @@ -581,20 +705,25 @@ class RevisionStore $fields = [ 'page_latest', 'page_namespace', 'page_title', 'rev_id', 'rev_text_id', 'rev_len', 'rev_sha1' ]; - if ( $this->contentHandlerUseDB ) { - $fields[] = 'rev_content_model'; - $fields[] = 'rev_content_format'; + if ( $this->mcrMigrationStage < MIGRATION_NEW ) { + $fields[] = 'rev_text_id'; + if ( $this->contentHandlerUseDB ) { + $fields[] = 'rev_content_model'; + $fields[] = 'rev_content_format'; + } } + // Don't use getQueryInfo() top avoid locking extra tables, compare T191892. + // XXX: perhaps getQueryInfo() could support 'no-comment' and 'no-actor' as options. $current = $dbw->selectRow( [ 'page', 'revision' ], $fields, [ 'page_id' => $title->getArticleID(), - 'page_latest=rev_id', ], __METHOD__, - [ 'FOR UPDATE' ] // T51581 + [ 'FOR UPDATE' ], // T51581 + [ 'page' => [ 'JOIN', 'page_latest=rev_id' ] ] ); if ( $current ) { @@ -605,19 +734,20 @@ class RevisionStore 'actor' => $user->getActorId(), 'comment' => $comment, 'minor_edit' => $minor, - 'text_id' => $current->rev_text_id, 'parent_id' => $current->page_latest, 'slot_origin' => $current->page_latest, 'len' => $current->rev_len, 'sha1' => $current->rev_sha1 ]; - if ( $this->contentHandlerUseDB ) { - $fields['content_model'] = $current->rev_content_model; - $fields['content_format'] = $current->rev_content_format; - } + if ( $this->mcrMigrationStage < MIGRATION_NEW ) { + $fields['text_id'] = $current->rev_text_id; - $fields['title'] = Title::makeTitle( $current->page_namespace, $current->page_title ); + if ( $this->contentHandlerUseDB ) { + $fields['content_model'] = $current->rev_content_model; + $fields['content_format'] = $current->rev_content_format; + } + } $mainSlot = $this->emulateMainSlot_1_29( $fields, self::READ_LATEST, $title ); $revision = new MutableRevisionRecord( $title, $this->wikiId ); @@ -755,7 +885,6 @@ class RevisionStore $mainSlotRow->model_name = null; $mainSlotRow->slot_revision_id = null; $mainSlotRow->content_address = null; - $mainSlotRow->slot_content_id = null; $content = null; $blobData = null; @@ -768,7 +897,6 @@ class RevisionStore } if ( isset( $row->rev_text_id ) && $row->rev_text_id > 0 ) { - $mainSlotRow->slot_content_id = $row->rev_text_id; $mainSlotRow->content_address = SqlBlobStore::makeAddressFromTextId( $row->rev_text_id ); @@ -803,9 +931,6 @@ class RevisionStore } elseif ( is_array( $row ) ) { $mainSlotRow->slot_revision_id = isset( $row['id'] ) ? intval( $row['id'] ) : null; - $mainSlotRow->slot_content_id = isset( $row['text_id'] ) - ? intval( $row['text_id'] ) - : null; $mainSlotRow->slot_origin = isset( $row['slot_origin'] ) ? intval( $row['slot_origin'] ) : null; @@ -874,7 +999,18 @@ class RevisionStore }; } - $mainSlotRow->slot_id = $mainSlotRow->slot_revision_id; + // NOTE: this callback will be looped through RevisionSlot::newInherited(), allowing + // the inherited slot to have the same content_id as the original slot. In that case, + // $slot will be the inherited slot, while $mainSlotRow still refers to the original slot. + $mainSlotRow->slot_content_id = + function ( SlotRecord $slot ) use ( $queryFlags, $mainSlotRow ) { + list( $dbMode, ) = DBAccessObjectUtils::getDBOptions( $queryFlags ); + $db = $this->getDBConnectionRef( $dbMode ); + return $this->findSlotContentId( $db, $mainSlotRow->slot_revision_id, 'main' ); + }; + + // use negative IDs for fake slot records. + $mainSlotRow->slot_id = -( $mainSlotRow->slot_revision_id ); return new SlotRecord( $mainSlotRow, $content ); } @@ -1580,7 +1716,7 @@ class RevisionStore private function fetchRevisionRowFromConds( IDatabase $db, $conditions, $flags = 0 ) { $this->checkDatabaseWikiId( $db ); - $revQuery = self::getQueryInfo( [ 'page', 'user' ] ); + $revQuery = $this->getQueryInfo( [ 'page', 'user' ] ); $options = []; if ( ( $flags & self::READ_LOCKING ) == self::READ_LOCKING ) { $options[] = 'FOR UPDATE'; @@ -1595,12 +1731,50 @@ class RevisionStore ); } + /** + * Finds the ID of a content row for a given revision and slot role. + * This can be used to re-use content rows even while the content ID + * is still missing from SlotRecords, in MIGRATION_WRITE_BOTH mode. + * + * @todo remove after MCR schema migration is complete. + * + * @param IDatabase $db + * @param int $revId + * @param string $role + * + * @return int|null + */ + private function findSlotContentId( IDatabase $db, $revId, $role ) { + if ( $this->mcrMigrationStage < MIGRATION_WRITE_BOTH ) { + return null; + } + + try { + $roleId = $this->slotRoleStore->getId( $role ); + $conditions = [ + 'slot_revision_id' => $revId, + 'slot_role_id' => $roleId, + ]; + + $contentId = $db->selectField( 'slots', 'slot_content_id', $conditions, __METHOD__ ); + + return $contentId ?: null; + } catch ( NameTableAccessException $ex ) { + // If the role is missing from the slot_roles table, + // the corresponding row in slots cannot exist. + return null; + } + } + /** * Return the tables, fields, and join conditions to be selected to create * a new revision object. * * MCR migration note: this replaces Revision::getQueryInfo * + * If the format of fields returned changes in any way then the cache key provided by + * self::getRevisionRowCacheKey should be updated. + * * @since 1.31 * * @param array $options Any combination of the following strings @@ -1624,7 +1798,6 @@ class RevisionStore $ret['fields'] = array_merge( $ret['fields'], [ 'rev_id', 'rev_page', - 'rev_text_id', 'rev_timestamp', 'rev_minor_edit', 'rev_deleted', @@ -1643,9 +1816,13 @@ class RevisionStore $ret['fields'] = array_merge( $ret['fields'], $actorQuery['fields'] ); $ret['joins'] = array_merge( $ret['joins'], $actorQuery['joins'] ); - if ( $this->contentHandlerUseDB ) { - $ret['fields'][] = 'rev_content_format'; - $ret['fields'][] = 'rev_content_model'; + if ( $this->mcrMigrationStage < MIGRATION_NEW ) { + $ret['fields'][] = 'rev_text_id'; + + if ( $this->contentHandlerUseDB ) { + $ret['fields'][] = 'rev_content_format'; + $ret['fields'][] = 'rev_content_model'; + } } if ( in_array( 'page', $options, true ) ) { @@ -1671,6 +1848,10 @@ class RevisionStore } if ( in_array( 'text', $options, true ) ) { + if ( $this->mcrMigrationStage === MIGRATION_NEW ) { + throw new InvalidArgumentException( 'text table can no longer be joined directly' ); + } + $ret['tables'][] = 'text'; $ret['fields'] = array_merge( $ret['fields'], [ 'old_text', @@ -1706,7 +1887,6 @@ class RevisionStore 'ar_namespace', 'ar_title', 'ar_rev_id', - 'ar_text_id', 'ar_timestamp', 'ar_minor_edit', 'ar_deleted', @@ -1717,9 +1897,13 @@ class RevisionStore 'joins' => $commentQuery['joins'] + $actorQuery['joins'], ]; - if ( $this->contentHandlerUseDB ) { - $ret['fields'][] = 'ar_content_format'; - $ret['fields'][] = 'ar_content_model'; + if ( $this->mcrMigrationStage < MIGRATION_NEW ) { + $ret['fields'][] = 'ar_text_id'; + + if ( $this->contentHandlerUseDB ) { + $ret['fields'][] = 'ar_content_format'; + $ret['fields'][] = 'ar_content_model'; + } } return $ret; @@ -1937,7 +2121,7 @@ class RevisionStore return false; } - $revQuery = self::getQueryInfo(); + $revQuery = $this->getQueryInfo(); $res = $db->select( $revQuery['tables'], [ @@ -1995,7 +2179,7 @@ class RevisionStore $row = $this->cache->getWithSetCallback( // Page/rev IDs passed in from DB to reflect history merges - $this->cache->makeGlobalKey( 'revision-row-1.29', $db->getDomainID(), $pageId, $revId ), + $this->getRevisionRowCacheKey( $db, $pageId, $revId ), WANObjectCache::TTL_WEEK, function ( $curValue, &$ttl, array &$setOpts ) use ( $db, $pageId, $revId ) { $setOpts += Database::getCacheSetOptions( $db ); @@ -2019,6 +2203,26 @@ class RevisionStore } } + /** + * Get a cache key for use with a row as selected with getQueryInfo( [ 'page', 'user' ] ) + * Caching rows without 'page' or 'user' could lead to issues. + * If the format of the rows returned by the query provided by getQueryInfo changes the + * cache key should be updated to avoid conflicts. + * + * @param IDatabase $db + * @param int $pageId + * @param int $revId + * @return string + */ + private function getRevisionRowCacheKey( IDatabase $db, $pageId, $revId ) { + return $this->cache->makeGlobalKey( + self::ROW_CACHE_KEY, + $db->getDomainID(), + $pageId, + $revId + ); + } + // TODO: move relevant methods from Title here, e.g. getFirstRevision, isBigDeletion, etc. } diff --git a/includes/Storage/SlotRecord.php b/includes/Storage/SlotRecord.php index 9462518ffe..e63dd3c355 100644 --- a/includes/Storage/SlotRecord.php +++ b/includes/Storage/SlotRecord.php @@ -38,7 +38,9 @@ use Wikimedia\Assert\Assert; class SlotRecord { /** - * @var object database result row, as a raw object + * @var object database result row, as a raw object. Callbacks are supported for field values, + * to enable on-demand emulation of these values. This is primarily intended for use + * during schema migration. */ private $row; @@ -142,11 +144,11 @@ class SlotRecord { /** * Constructs a complete SlotRecord for a newly saved revision, based on the incomplete * proto-slot. This adds information that has only become available during saving, - * particularly the revision ID and content address. + * particularly the revision ID, content ID and content address. * * @param int $revisionId the revision the slot is to be associated with (field slot_revision_id). * If $protoSlot already has a revision, it must be the same. - * @param int $contentId the ID of the row in the content table describing the content + * @param int|null $contentId the ID of the row in the content table describing the content * referenced by $contentAddress (field slot_content_id). * If $protoSlot already has a content ID, it must be the same. * @param string $contentAddress the slot's content address (field content_address). @@ -163,7 +165,8 @@ class SlotRecord { SlotRecord $protoSlot ) { Assert::parameterType( 'integer', $revisionId, '$revisionId' ); - Assert::parameterType( 'integer', $contentId, '$contentId' ); + // TODO once migration is over $contentId must be an integer + Assert::parameterType( 'integer|null', $contentId, '$contentId' ); Assert::parameterType( 'string', $contentAddress, '$contentAddress' ); if ( $protoSlot->hasRevision() && $protoSlot->getRevision() !== $revisionId ) { @@ -181,7 +184,7 @@ class SlotRecord { ); } - if ( $protoSlot->hasAddress() && $protoSlot->getContentId() !== $contentId ) { + if ( $protoSlot->hasContentId() && $protoSlot->getContentId() !== $contentId ) { throw new LogicException( "Mismatching content ID $contentId: " . "The slot already has content row {$protoSlot->getContentId()} associated." @@ -379,6 +382,13 @@ class SlotRecord { * @return bool whether this record contains the given field */ private function hasField( $name ) { + if ( isset( $this->row->$name ) ) { + // if the field is a callback, resolve first, then re-check + if ( !is_string( $this->row->$name ) && is_callable( $this->row->$name ) ) { + $this->getField( $name ); + } + } + return isset( $this->row->$name ); } @@ -430,6 +440,30 @@ class SlotRecord { return $this->hasField( 'content_address' ); } + /** + * Whether this slot has an origin (revision ID that originated the slot's content. + * + * @since 1.32 + * + * @return bool + */ + public function hasOrigin() { + return $this->hasField( 'slot_origin' ); + } + + /** + * Whether this slot has a content ID. Slots will have a content ID if their + * content has been stored in the content table. While building a new revision, + * SlotRecords will not have an ID associated. + * + * @since 1.32 + * + * @return bool + */ + public function hasContentId() { + return $this->hasField( 'slot_content_id' ); + } + /** * Whether this slot has revision ID associated. Slots will have a revision ID associated * only if they were loaded as part of an existing revision. While building a new revision, diff --git a/includes/page/WikiPage.php b/includes/page/WikiPage.php index 7aa1aadfd5..65b3428df6 100644 --- a/includes/page/WikiPage.php +++ b/includes/page/WikiPage.php @@ -2410,7 +2410,7 @@ class WikiPage implements Page, IDBAccessObject { $tags = [], $logsubtype = 'delete' ) { global $wgUser, $wgContentHandlerUseDB, $wgCommentTableSchemaMigrationStage, - $wgActorTableSchemaMigrationStage; + $wgActorTableSchemaMigrationStage, $wgMultiContentRevisionSchemaMigrationStage; wfDebug( __METHOD__ . "\n" ); @@ -2497,9 +2497,17 @@ class WikiPage implements Page, IDBAccessObject { // Note array_intersect() preserves keys from the first arg, and we're // assuming $revQuery has `revision` primary and isn't using subtables // for anything we care about. + $tablesFlat = []; + array_walk_recursive( + $revQuery['tables'], + function ( $a ) use ( &$tablesFlat ) { + $tablesFlat[] = $a; + } + ); + $res = $dbw->select( array_intersect( - $revQuery['tables'], + $tablesFlat, [ 'revision', 'revision_comment_temp', 'revision_actor_temp' ] ), '1', @@ -2539,6 +2547,14 @@ class WikiPage implements Page, IDBAccessObject { 'ar_minor_edit' => $row->rev_minor_edit, 'ar_rev_id' => $row->rev_id, 'ar_parent_id' => $row->rev_parent_id, + /** + * ar_text_id should probably not be written to when the multi content schema has + * been migrated to (wgMultiContentRevisionSchemaMigrationStage) however there is no + * default for the field in WMF production currently so we must keep writing + * writing until a default of 0 is set. + * Task: https://phabricator.wikimedia.org/T190148 + * Copying the value from the revision table should not lead to any issues for now. + */ 'ar_text_id' => $row->rev_text_id, 'ar_len' => $row->rev_len, 'ar_page_id' => $id, @@ -2546,7 +2562,10 @@ class WikiPage implements Page, IDBAccessObject { 'ar_sha1' => $row->rev_sha1, ] + $commentStore->insert( $dbw, 'ar_comment', $comment ) + $actorMigration->getInsertValues( $dbw, 'ar_user', $user ); - if ( $wgContentHandlerUseDB ) { + if ( + $wgContentHandlerUseDB && + $wgMultiContentRevisionSchemaMigrationStage <= MIGRATION_WRITE_BOTH + ) { $rowInsert['ar_content_model'] = $row->rev_content_model; $rowInsert['ar_content_format'] = $row->rev_content_format; } diff --git a/tests/common/TestsAutoLoader.php b/tests/common/TestsAutoLoader.php index 2cc56419e6..f457a0657a 100644 --- a/tests/common/TestsAutoLoader.php +++ b/tests/common/TestsAutoLoader.php @@ -150,6 +150,7 @@ $wgAutoloadClasses += [ # tests/phpunit/includes/Storage 'MediaWiki\Tests\Storage\McrSchemaDetection' => "$testDir/phpunit/includes/Storage/McrSchemaDetection.php", + 'MediaWiki\Tests\Storage\McrWriteBothSchemaOverride' => "$testDir/phpunit/includes/Storage/McrWriteBothSchemaOverride.php", 'MediaWiki\Tests\Storage\RevisionSlotsTest' => "$testDir/phpunit/includes/Storage/RevisionSlotsTest.php", 'MediaWiki\Tests\Storage\RevisionRecordTests' => "$testDir/phpunit/includes/Storage/RevisionRecordTests.php", 'MediaWiki\Tests\Storage\RevisionStoreDbTestBase' => "$testDir/phpunit/includes/Storage/RevisionStoreDbTestBase.php", diff --git a/tests/phpunit/MediaWikiTestCase.php b/tests/phpunit/MediaWikiTestCase.php index de1dbfd202..b6c569cf6b 100644 --- a/tests/phpunit/MediaWikiTestCase.php +++ b/tests/phpunit/MediaWikiTestCase.php @@ -1602,8 +1602,10 @@ abstract class MediaWikiTestCase extends PHPUnit\Framework\TestCase { private function resetDB( $db, $tablesUsed ) { if ( $db ) { $userTables = [ 'user', 'user_groups', 'user_properties', 'actor' ]; - $pageTables = [ 'page', 'revision', 'ip_changes', 'revision_comment_temp', - 'revision_actor_temp', 'comment', 'archive' ]; + $pageTables = [ + 'page', 'revision', 'ip_changes', 'revision_comment_temp', 'comment', 'archive', + 'revision_actor_temp', 'slots', 'content', 'content_models', 'slot_roles', + ]; $coreDBDataTables = array_merge( $userTables, $pageTables ); // If any of the user or page tables were marked as used, we should clear all of them. diff --git a/tests/phpunit/includes/RevisionDbTestBase.php b/tests/phpunit/includes/RevisionDbTestBase.php index f8c6715290..1ab78f41f5 100644 --- a/tests/phpunit/includes/RevisionDbTestBase.php +++ b/tests/phpunit/includes/RevisionDbTestBase.php @@ -414,6 +414,9 @@ abstract class RevisionDbTestBase extends MediaWikiTestCase { $services->getService( '_SqlBlobStore' ), $services->getMainWANObjectCache(), $services->getCommentStore(), + $services->getContentModelStore(), + $services->getSlotRoleStore(), + $this->getMcrMigrationStage(), $services->getActorMigration() ); @@ -1378,7 +1381,7 @@ abstract class RevisionDbTestBase extends MediaWikiTestCase { $rev = $this->testPage->getRevision(); // Clear any previous cache for the revision during creation - $key = $cache->makeGlobalKey( 'revision-row-1.29', + $key = $cache->makeGlobalKey( RevisionStore::ROW_CACHE_KEY, $db->getDomainID(), $rev->getPage(), $rev->getId() diff --git a/tests/phpunit/includes/RevisionMcrWriteBothDbTest.php b/tests/phpunit/includes/RevisionMcrWriteBothDbTest.php new file mode 100644 index 0000000000..436b37995d --- /dev/null +++ b/tests/phpunit/includes/RevisionMcrWriteBothDbTest.php @@ -0,0 +1,23 @@ +setMwGlobals( 'wgMultiContentRevisionSchemaMigrationStage', MIGRATION_OLD ); + } + public function provideConstructFromArray() { yield 'with text' => [ [ @@ -488,6 +493,9 @@ class RevisionTest extends MediaWikiTestCase { $this->getBlobStore(), $cache, MediaWikiServices::getInstance()->getCommentStore(), + MediaWikiServices::getInstance()->getContentModelStore(), + MediaWikiServices::getInstance()->getSlotRoleStore(), + MIGRATION_OLD, MediaWikiServices::getInstance()->getActorMigration() ); return $blobStore; @@ -1156,10 +1164,60 @@ class RevisionTest extends MediaWikiTestCase { $revisionStore = $this->getRevisionStore(); $revisionStore->setContentHandlerUseDB( $globals['wgContentHandlerUseDB'] ); $this->setService( 'RevisionStore', $revisionStore ); - $this->assertEquals( - $expected, - Revision::getArchiveQueryInfo() + + $queryInfo = Revision::getArchiveQueryInfo(); + + $this->assertArrayEqualsIgnoringIntKeyOrder( + $expected['tables'], + $queryInfo['tables'] ); + $this->assertArrayEqualsIgnoringIntKeyOrder( + $expected['fields'], + $queryInfo['fields'] + ); + $this->assertArrayEqualsIgnoringIntKeyOrder( + $expected['joins'], + $queryInfo['joins'] + ); + } + + /** + * Assert that the two arrays passed are equal, ignoring the order of the values that integer + * keys. + * + * Note: Failures of this assertion can be slightly confusing as the arrays are actually + * split into a string key array and an int key array before assertions occur. + * + * @param array $expected + * @param array $actual + */ + private function assertArrayEqualsIgnoringIntKeyOrder( array $expected, array $actual ) { + $this->objectAssociativeSort( $expected ); + $this->objectAssociativeSort( $actual ); + + // Separate the int key values from the string key values so that assertion failures are + // easier to understand. + $expectedIntKeyValues = []; + $actualIntKeyValues = []; + + // Remove all int keys and re add them at the end after sorting by value + // This will result in all int keys being in the same order with same ints at the end of + // the array + foreach ( $expected as $key => $value ) { + if ( is_int( $key ) ) { + unset( $expected[$key] ); + $expectedIntKeyValues[] = $value; + } + } + foreach ( $actual as $key => $value ) { + if ( is_int( $key ) ) { + unset( $actual[$key] ); + $actualIntKeyValues[] = $value; + } + } + + $this->assertArrayEquals( $expected, $actual, false, true ); + $this->assertArrayEquals( $expectedIntKeyValues, $actualIntKeyValues, false, true ); } public function provideGetQueryInfo() { @@ -1391,9 +1449,19 @@ class RevisionTest extends MediaWikiTestCase { $revisionStore->setContentHandlerUseDB( $globals['wgContentHandlerUseDB'] ); $this->setService( 'RevisionStore', $revisionStore ); - $this->assertEquals( - $expected, - Revision::getQueryInfo( $options ) + $queryInfo = Revision::getQueryInfo( $options ); + + $this->assertArrayEqualsIgnoringIntKeyOrder( + $expected['tables'], + $queryInfo['tables'] + ); + $this->assertArrayEqualsIgnoringIntKeyOrder( + $expected['fields'], + $queryInfo['fields'] + ); + $this->assertArrayEqualsIgnoringIntKeyOrder( + $expected['joins'], + $queryInfo['joins'] ); } diff --git a/tests/phpunit/includes/Storage/McrWriteBothRevisionStoreDbTest.php b/tests/phpunit/includes/Storage/McrWriteBothRevisionStoreDbTest.php new file mode 100644 index 0000000000..81c98c04eb --- /dev/null +++ b/tests/phpunit/includes/Storage/McrWriteBothRevisionStoreDbTest.php @@ -0,0 +1,114 @@ +assertSelect( + 'slots', [ 'count(*)' ], [ 'slot_revision_id' => $rev->getId() ], [ [ '1' ] ] + ); + $this->assertSelect( + 'content', + [ 'count(*)' ], + [ 'content_address' => $rev->getSlot( 'main' )->getAddress() ], + [ [ '1' ] ] + ); + } + + /** + * @param SlotRecord $a + * @param SlotRecord $b + */ + protected function assertSameSlotContent( SlotRecord $a, SlotRecord $b ) { + parent::assertSameSlotContent( $a, $b ); + + // Assert that the same content ID has been used + if ( $a->hasContentId() && $b->hasContentId() ) { + $this->assertSame( $a->getContentId(), $b->getContentId() ); + } + } + + public function provideGetArchiveQueryInfo() { + yield [ + [ + 'tables' => [ 'archive' ], + 'fields' => array_merge( + $this->getDefaultArchiveFields(), + [ + 'ar_comment_text' => 'ar_comment', + 'ar_comment_data' => 'NULL', + 'ar_comment_cid' => 'NULL', + 'ar_user_text' => 'ar_user_text', + 'ar_user' => 'ar_user', + 'ar_actor' => 'NULL', + 'ar_content_format', + 'ar_content_model', + ] + ), + 'joins' => [], + ] + ]; + } + + public function provideGetQueryInfo() { + yield [ + [], + [ + 'tables' => [ 'revision' ], + 'fields' => array_merge( + $this->getDefaultQueryFields(), + $this->getCommentQueryFields(), + $this->getActorQueryFields(), + $this->getContentHandlerQueryFields() + ), + 'joins' => [], + ] + ]; + yield [ + [ 'page', 'user', 'text' ], + [ + 'tables' => [ 'revision', 'page', 'user', 'text' ], + 'fields' => array_merge( + $this->getDefaultQueryFields(), + $this->getCommentQueryFields(), + $this->getActorQueryFields(), + $this->getContentHandlerQueryFields(), + [ + 'page_namespace', + 'page_title', + 'page_id', + 'page_latest', + 'page_is_redirect', + 'page_len', + 'user_name', + 'old_text', + 'old_flags', + ] + ), + 'joins' => [ + 'page' => [ 'INNER JOIN', [ 'page_id = rev_page' ] ], + 'user' => [ 'LEFT JOIN', [ 'rev_user != 0', 'user_id = rev_user' ] ], + 'text' => [ 'INNER JOIN', [ 'rev_text_id=old_id' ] ], + ], + ] + ]; + } + +} diff --git a/tests/phpunit/includes/Storage/McrWriteBothSchemaOverride.php b/tests/phpunit/includes/Storage/McrWriteBothSchemaOverride.php new file mode 100644 index 0000000000..2a54dbe43c --- /dev/null +++ b/tests/phpunit/includes/Storage/McrWriteBothSchemaOverride.php @@ -0,0 +1,58 @@ + [], + 'drop' => [], + 'create' => [], + 'alter' => [], + ]; + + if ( !$this->hasMcrTables( $db ) ) { + $overrides['create'] = [ 'slots', 'content', 'slot_roles', 'content_models', ]; + $overrides['scripts'][] = $this->getSqlPatchPath( $db, 'patch-slot_roles' ); + $overrides['scripts'][] = $this->getSqlPatchPath( $db, 'patch-content_models' ); + $overrides['scripts'][] = $this->getSqlPatchPath( $db, 'patch-content' ); + $overrides['scripts'][] = $this->getSqlPatchPath( $db, 'patch-slots' ); + } + + if ( !$this->hasPreMcrFields( $db ) ) { + $overrides['alter'][] = 'revision'; + $overrides['scripts'][] = $this->getSqlPatchPath( $db, 'create-pre-mcr-fields', __DIR__ ); + } + + return $overrides; + } + +} diff --git a/tests/phpunit/includes/Storage/RevisionStoreDbTestBase.php b/tests/phpunit/includes/Storage/RevisionStoreDbTestBase.php index 9c113b532a..5927cfc9fb 100644 --- a/tests/phpunit/includes/Storage/RevisionStoreDbTestBase.php +++ b/tests/phpunit/includes/Storage/RevisionStoreDbTestBase.php @@ -179,6 +179,9 @@ abstract class RevisionStoreDbTestBase extends MediaWikiTestCase { $blobStore, new WANObjectCache( [ 'cache' => new HashBagOStuff() ] ), MediaWikiServices::getInstance()->getCommentStore(), + MediaWikiServices::getInstance()->getContentModelStore(), + MediaWikiServices::getInstance()->getSlotRoleStore(), + $this->getMcrMigrationStage(), MediaWikiServices::getInstance()->getActorMigration(), $wikiId ); @@ -316,6 +319,8 @@ abstract class RevisionStoreDbTestBase extends MediaWikiTestCase { /** * @dataProvider provideInsertRevisionOn_successes * @covers \MediaWiki\Storage\RevisionStore::insertRevisionOn + * @covers \MediaWiki\Storage\RevisionStore::insertSlotRowOn + * @covers \MediaWiki\Storage\RevisionStore::insertContentRowOn */ public function testInsertRevisionOn_successes( Title $title, @@ -480,6 +485,7 @@ abstract class RevisionStoreDbTestBase extends MediaWikiTestCase { /** * @dataProvider provideNewNullRevision * @covers \MediaWiki\Storage\RevisionStore::newNullRevision + * @covers \MediaWiki\Storage\RevisionStore::findSlotContentId */ public function testNewNullRevision( Title $title, $comment, $minor ) { $this->overrideMwServices(); diff --git a/tests/phpunit/includes/Storage/RevisionStoreTest.php b/tests/phpunit/includes/Storage/RevisionStoreTest.php index 61d0512542..a877f875c3 100644 --- a/tests/phpunit/includes/Storage/RevisionStoreTest.php +++ b/tests/phpunit/includes/Storage/RevisionStoreTest.php @@ -2,17 +2,21 @@ namespace MediaWiki\Tests\Storage; +use CommentStore; use HashBagOStuff; +use InvalidArgumentException; use Language; use MediaWiki\MediaWikiServices; use MediaWiki\Storage\RevisionAccessException; use MediaWiki\Storage\RevisionStore; use MediaWiki\Storage\SqlBlobStore; use MediaWikiTestCase; +use MWException; use Title; use WANObjectCache; use Wikimedia\Rdbms\Database; use Wikimedia\Rdbms\LoadBalancer; +use Wikimedia\TestingAccessWrapper; class RevisionStoreTest extends MediaWikiTestCase { @@ -28,11 +32,18 @@ class RevisionStoreTest extends MediaWikiTestCase { $blobStore = null, $WANObjectCache = null ) { + global $wgMultiContentRevisionSchemaMigrationStage; + // the migration stage should be irrelevant, since all the tests that interact with + // the database are in RevisionStoreDbTest, not here. + return new RevisionStore( $loadBalancer ?: $this->getMockLoadBalancer(), $blobStore ?: $this->getMockSqlBlobStore(), $WANObjectCache ?: $this->getHashWANObjectCache(), MediaWikiServices::getInstance()->getCommentStore(), + MediaWikiServices::getInstance()->getContentModelStore(), + MediaWikiServices::getInstance()->getSlotRoleStore(), + $wgMultiContentRevisionSchemaMigrationStage, MediaWikiServices::getInstance()->getActorMigration() ); } @@ -61,190 +72,53 @@ class RevisionStoreTest extends MediaWikiTestCase { ->disableOriginalConstructor()->getMock(); } - private function getHashWANObjectCache() { - return new WANObjectCache( [ 'cache' => new \HashBagOStuff() ] ); - } - /** - * @covers \MediaWiki\Storage\RevisionStore::getContentHandlerUseDB - * @covers \MediaWiki\Storage\RevisionStore::setContentHandlerUseDB + * @return \PHPUnit_Framework_MockObject_MockObject|CommentStore */ - public function testGetSetContentHandlerDb() { - $store = $this->getRevisionStore(); - $this->assertTrue( $store->getContentHandlerUseDB() ); - $store->setContentHandlerUseDB( false ); - $this->assertFalse( $store->getContentHandlerUseDB() ); - $store->setContentHandlerUseDB( true ); - $this->assertTrue( $store->getContentHandlerUseDB() ); - } - - private function getDefaultQueryFields() { - return [ - 'rev_id', - 'rev_page', - 'rev_text_id', - 'rev_timestamp', - 'rev_minor_edit', - 'rev_deleted', - 'rev_len', - 'rev_parent_id', - 'rev_sha1', - ]; - } - - private function getCommentQueryFields() { - return [ - 'rev_comment_text' => 'rev_comment', - 'rev_comment_data' => 'NULL', - 'rev_comment_cid' => 'NULL', - ]; + private function getMockCommentStore() { + return $this->getMockBuilder( CommentStore::class ) + ->disableOriginalConstructor()->getMock(); } - private function getActorQueryFields() { - return [ - 'rev_user' => 'rev_user', - 'rev_user_text' => 'rev_user_text', - 'rev_actor' => 'NULL', - ]; + private function getHashWANObjectCache() { + return new WANObjectCache( [ 'cache' => new \HashBagOStuff() ] ); } - private function getContentHandlerQueryFields() { + public function provideSetContentHandlerUseDB() { return [ - 'rev_content_format', - 'rev_content_model', - ]; - } - - public function provideGetQueryInfo() { - yield [ - true, - [], - [ - 'tables' => [ 'revision' ], - 'fields' => array_merge( - $this->getDefaultQueryFields(), - $this->getCommentQueryFields(), - $this->getActorQueryFields(), - $this->getContentHandlerQueryFields() - ), - 'joins' => [], - ] - ]; - yield [ - false, - [], - [ - 'tables' => [ 'revision' ], - 'fields' => array_merge( - $this->getDefaultQueryFields(), - $this->getCommentQueryFields(), - $this->getActorQueryFields() - ), - 'joins' => [], - ] - ]; - yield [ - false, - [ 'page' ], - [ - 'tables' => [ 'revision', 'page' ], - 'fields' => array_merge( - $this->getDefaultQueryFields(), - $this->getCommentQueryFields(), - $this->getActorQueryFields(), - [ - 'page_namespace', - 'page_title', - 'page_id', - 'page_latest', - 'page_is_redirect', - 'page_len', - ] - ), - 'joins' => [ - 'page' => [ 'INNER JOIN', [ 'page_id = rev_page' ] ], - ], - ] - ]; - yield [ - false, - [ 'user' ], - [ - 'tables' => [ 'revision', 'user' ], - 'fields' => array_merge( - $this->getDefaultQueryFields(), - $this->getCommentQueryFields(), - $this->getActorQueryFields(), - [ - 'user_name', - ] - ), - 'joins' => [ - 'user' => [ 'LEFT JOIN', [ 'rev_user != 0', 'user_id = rev_user' ] ], - ], - ] - ]; - yield [ - false, - [ 'text' ], - [ - 'tables' => [ 'revision', 'text' ], - 'fields' => array_merge( - $this->getDefaultQueryFields(), - $this->getCommentQueryFields(), - $this->getActorQueryFields(), - [ - 'old_text', - 'old_flags', - ] - ), - 'joins' => [ - 'text' => [ 'INNER JOIN', [ 'rev_text_id=old_id' ] ], - ], - ] - ]; - yield [ - true, - [ 'page', 'user', 'text' ], - [ - 'tables' => [ 'revision', 'page', 'user', 'text' ], - 'fields' => array_merge( - $this->getDefaultQueryFields(), - $this->getCommentQueryFields(), - $this->getActorQueryFields(), - $this->getContentHandlerQueryFields(), - [ - 'page_namespace', - 'page_title', - 'page_id', - 'page_latest', - 'page_is_redirect', - 'page_len', - 'user_name', - 'old_text', - 'old_flags', - ] - ), - 'joins' => [ - 'page' => [ 'INNER JOIN', [ 'page_id = rev_page' ] ], - 'user' => [ 'LEFT JOIN', [ 'rev_user != 0', 'user_id = rev_user' ] ], - 'text' => [ 'INNER JOIN', [ 'rev_text_id=old_id' ] ], - ], - ] + // ContentHandlerUseDB can be true of false pre migration + [ false, MIGRATION_OLD, false ], + [ true, MIGRATION_OLD, false ], + // During migration it can not be false + [ false, MIGRATION_WRITE_BOTH, true ], + // But it can be true + [ true, MIGRATION_WRITE_BOTH, false ], ]; } /** - * @dataProvider provideGetQueryInfo - * @covers \MediaWiki\Storage\RevisionStore::getQueryInfo + * @dataProvider provideSetContentHandlerUseDB + * @covers \MediaWiki\Storage\RevisionStore::getContentHandlerUseDB + * @covers \MediaWiki\Storage\RevisionStore::setContentHandlerUseDB */ - public function testGetQueryInfo( $contentHandlerUseDb, $options, $expected ) { - $this->setMwGlobals( 'wgCommentTableSchemaMigrationStage', MIGRATION_OLD ); - $this->setMwGlobals( 'wgActorTableSchemaMigrationStage', MIGRATION_OLD ); - $this->overrideMwServices(); - $store = $this->getRevisionStore(); - $store->setContentHandlerUseDB( $contentHandlerUseDb ); - $this->assertEquals( $expected, $store->getQueryInfo( $options ) ); + public function testSetContentHandlerUseDB( $contentHandlerDb, $migrationMode, $expectedFail ) { + if ( $expectedFail ) { + $this->setExpectedException( MWException::class ); + } + + $store = new RevisionStore( + $this->getMockLoadBalancer(), + $this->getMockSqlBlobStore(), + $this->getHashWANObjectCache(), + $this->getMockCommentStore(), + MediaWikiServices::getInstance()->getContentModelStore(), + MediaWikiServices::getInstance()->getSlotRoleStore(), + $migrationMode, + MediaWikiServices::getInstance()->getActorMigration() + ); + + $store->setContentHandlerUseDB( $contentHandlerDb ); + $this->assertSame( $contentHandlerDb, $store->getContentHandlerUseDB() ); } public function testGetTitle_successFromPageId() { @@ -608,4 +482,47 @@ class RevisionStoreTest extends MediaWikiTestCase { return (object)$row; } + public function provideMigrationConstruction() { + return [ + [ MIGRATION_OLD, false ], + [ MIGRATION_WRITE_BOTH, false ], + ]; + } + + /** + * @covers \MediaWiki\Storage\RevisionStore::__construct + * @dataProvider provideMigrationConstruction + */ + public function testMigrationConstruction( $migration, $expectException ) { + if ( $expectException ) { + $this->setExpectedException( InvalidArgumentException::class ); + } + $loadBalancer = $this->getMockLoadBalancer(); + $blobStore = $this->getMockSqlBlobStore(); + $cache = $this->getHashWANObjectCache(); + $commentStore = $this->getMockCommentStore(); + $contentModelStore = MediaWikiServices::getInstance()->getContentModelStore(); + $slotRoleStore = MediaWikiServices::getInstance()->getSlotRoleStore(); + $store = new RevisionStore( + $loadBalancer, + $blobStore, + $cache, + $commentStore, + MediaWikiServices::getInstance()->getContentModelStore(), + MediaWikiServices::getInstance()->getSlotRoleStore(), + $migration, + MediaWikiServices::getInstance()->getActorMigration() + ); + if ( !$expectException ) { + $store = TestingAccessWrapper::newFromObject( $store ); + $this->assertSame( $loadBalancer, $store->loadBalancer ); + $this->assertSame( $blobStore, $store->blobStore ); + $this->assertSame( $cache, $store->cache ); + $this->assertSame( $commentStore, $store->commentStore ); + $this->assertSame( $contentModelStore, $store->contentModelStore ); + $this->assertSame( $slotRoleStore, $store->slotRoleStore ); + $this->assertSame( $migration, $store->mcrMigrationStage ); + } + } + } diff --git a/tests/phpunit/includes/Storage/SlotRecordTest.php b/tests/phpunit/includes/Storage/SlotRecordTest.php index feeb538440..1aae16d4cd 100644 --- a/tests/phpunit/includes/Storage/SlotRecordTest.php +++ b/tests/phpunit/includes/Storage/SlotRecordTest.php @@ -36,6 +36,7 @@ class SlotRecordTest extends MediaWikiTestCase { $record = new SlotRecord( $row, new WikitextContent( 'A' ) ); $this->assertTrue( $record->hasAddress() ); + $this->assertTrue( $record->hasContentId() ); $this->assertTrue( $record->hasRevision() ); $this->assertTrue( $record->isInherited() ); $this->assertSame( 'A', $record->getContent()->getNativeData() ); @@ -59,6 +60,9 @@ class SlotRecordTest extends MediaWikiTestCase { }, 'slot_revision_id' => '2', 'slot_origin' => '2', + 'slot_content_id' => function () { + return null; + }, ] ); $content = function () { @@ -69,6 +73,7 @@ class SlotRecordTest extends MediaWikiTestCase { $this->assertTrue( $record->hasAddress() ); $this->assertTrue( $record->hasRevision() ); + $this->assertFalse( $record->hasContentId() ); $this->assertFalse( $record->isInherited() ); $this->assertSame( 'A', $record->getContent()->getNativeData() ); $this->assertSame( 1, $record->getSize() ); @@ -77,7 +82,6 @@ class SlotRecordTest extends MediaWikiTestCase { $this->assertSame( 2, $record->getRevision() ); $this->assertSame( 2, $record->getRevision() ); $this->assertSame( 'tt:456', $record->getAddress() ); - $this->assertSame( 33, $record->getContentId() ); $this->assertSame( CONTENT_FORMAT_WIKITEXT, $record->getFormat() ); $this->assertSame( 'myRole', $record->getRole() ); } @@ -86,8 +90,10 @@ class SlotRecordTest extends MediaWikiTestCase { $record = SlotRecord::newUnsaved( 'myRole', new WikitextContent( 'A' ) ); $this->assertFalse( $record->hasAddress() ); + $this->assertFalse( $record->hasContentId() ); $this->assertFalse( $record->hasRevision() ); $this->assertFalse( $record->isInherited() ); + $this->assertFalse( $record->hasOrigin() ); $this->assertSame( 'A', $record->getContent()->getNativeData() ); $this->assertSame( 1, $record->getSize() ); $this->assertNotNull( $record->getSha1() ); @@ -190,6 +196,7 @@ class SlotRecordTest extends MediaWikiTestCase { $this->assertSame( $parent->getAddress(), $inherited->getAddress() ); $this->assertSame( $parent->getContent(), $inherited->getContent() ); $this->assertTrue( $inherited->isInherited() ); + $this->assertTrue( $inherited->hasOrigin() ); $this->assertFalse( $inherited->hasRevision() ); // make sure we didn't mess with the internal state of $parent @@ -224,8 +231,10 @@ class SlotRecordTest extends MediaWikiTestCase { // and content meta-data. $saved = SlotRecord::newSaved( 10, 20, 'theNewAddress', $unsaved ); $this->assertFalse( $saved->isInherited() ); + $this->assertTrue( $saved->hasOrigin() ); $this->assertTrue( $saved->hasRevision() ); $this->assertTrue( $saved->hasAddress() ); + $this->assertTrue( $saved->hasContentId() ); $this->assertSame( 'theNewAddress', $saved->getAddress() ); $this->assertSame( 20, $saved->getContentId() ); $this->assertSame( 'A', $saved->getContent()->getNativeData() ); @@ -234,6 +243,7 @@ class SlotRecordTest extends MediaWikiTestCase { // make sure we didn't mess with the internal state of $unsaved $this->assertFalse( $unsaved->hasAddress() ); + $this->assertFalse( $unsaved->hasContentId() ); $this->assertFalse( $unsaved->hasRevision() ); } diff --git a/tests/phpunit/includes/page/WikiPageMcrWriteBothDbTest.php b/tests/phpunit/includes/page/WikiPageMcrWriteBothDbTest.php new file mode 100644 index 0000000000..78bbfa7cb9 --- /dev/null +++ b/tests/phpunit/includes/page/WikiPageMcrWriteBothDbTest.php @@ -0,0 +1,23 @@ +