From d3d8dc99652cc2e2863916558e12cd725cf1af15 Mon Sep 17 00:00:00 2001 From: Tim Starling Date: Mon, 3 May 2021 15:13:41 +1000 Subject: [PATCH] Schema migration for revison_actor_temp table removal Introduce a new schema migration stage in which rev_actor is used directly and the revision_actor_temp table is no longer needed. This becomes the new "new" stage whereas the previous situation is now termed SCHEMA_COMPAT_TEMP. Introduce migrateRevisionActorTemp which copies data from revision_actor_temp to rev_actor. The code is similar to migrateImageCommentTemp.php except that it doesn't delete from the old table. Partial revert of c29909e59fd89d9fc6. That change removed direct references to $wgActorTableSchemaMigrationStage and made queries involving revision_actor_temp be unconditional. Such changes need to be reverted to make the use of revision_actor_temp be conditional again. In ActorMigrationTest, I compacted provideGetJoin() and provideGetWhere(), removing most of the duplication between expected values. I gave all the stages a short name, and mostly used the name in providers. Bug: T275246 Change-Id: I7498107dd6433ab7de5bf2e7b3fe2aa5e10e345d --- autoload.php | 1 + includes/ActorMigration.php | 4 + includes/ActorMigrationBase.php | 129 ++- includes/DefaultSettings.php | 22 + includes/Defines.php | 21 +- includes/MergeHistory.php | 24 +- .../Rest/Handler/PageHistoryCountHandler.php | 59 +- includes/Rest/Handler/PageHistoryHandler.php | 6 +- includes/Rest/coreRoutes.json | 3 +- includes/ServiceWiring.php | 2 +- includes/actions/InfoAction.php | 17 +- includes/api/ApiQueryAllRevisions.php | 6 +- includes/api/ApiQueryContributors.php | 14 +- includes/api/ApiQueryRevisions.php | 6 +- includes/api/ApiQueryUserContribs.php | 51 +- includes/page/WikiPage.php | 6 +- .../revisiondelete/RevisionDeleteUser.php | 25 +- includes/specials/pagers/ContribsPager.php | 2 - includes/user/UserEditTracker.php | 2 +- maintenance/findMissingActors.php | 62 +- maintenance/migrateRevisionActorTemp.php | 92 +++ maintenance/reassignEdits.php | 23 +- tests/phpunit/includes/ActorMigrationTest.php | 772 +++++++++--------- tests/phpunit/includes/ActorMigrationTest.sql | 3 +- .../Revision/RevisionQueryInfoTest.php | 99 ++- 25 files changed, 893 insertions(+), 558 deletions(-) create mode 100644 maintenance/migrateRevisionActorTemp.php diff --git a/autoload.php b/autoload.php index 72673bea64b..1c11d0c8db6 100644 --- a/autoload.php +++ b/autoload.php @@ -1140,6 +1140,7 @@ $wgAutoloadLocalClasses = [ 'MigrateComments' => __DIR__ . '/maintenance/migrateComments.php', 'MigrateFileRepoLayout' => __DIR__ . '/maintenance/migrateFileRepoLayout.php', 'MigrateImageCommentTemp' => __DIR__ . '/maintenance/migrateImageCommentTemp.php', + 'MigrateRevisionActorTemp' => __DIR__ . '/maintenance/migrateRevisionActorTemp.php', 'MigrateUserGroup' => __DIR__ . '/maintenance/migrateUserGroup.php', 'MimeAnalyzer' => __DIR__ . '/includes/libs/mime/MimeAnalyzer.php', 'MostimagesPage' => __DIR__ . '/includes/specials/SpecialMostimages.php', diff --git a/includes/ActorMigration.php b/includes/ActorMigration.php index 5fda2ea6b06..1b953f369f8 100644 --- a/includes/ActorMigration.php +++ b/includes/ActorMigration.php @@ -87,6 +87,10 @@ class ActorMigration extends ActorMigrationBase { $stage, ActorStoreFactory $actorStoreFactory ) { + if ( $stage & SCHEMA_COMPAT_OLD ) { + throw new InvalidArgumentException( + 'The old actor table schema is no longer supported' ); + } parent::__construct( self::FIELD_INFOS, $stage, diff --git a/includes/ActorMigrationBase.php b/includes/ActorMigrationBase.php index a2466c4cdf1..3448ae62dbc 100644 --- a/includes/ActorMigrationBase.php +++ b/includes/ActorMigrationBase.php @@ -36,8 +36,11 @@ class ActorMigrationBase { /** @var array[] Cache for `self::getJoin()` */ private $joinCache = []; - /** @var int Combination of SCHEMA_COMPAT_* constants */ - private $stage; + /** @var int One of the SCHEMA_COMPAT_READ_* values */ + private $readStage; + + /** @var int A combination of the SCHEMA_COMPAT_WRITE_* flags */ + private $writeStage; /** @var ActorStoreFactory */ private $actorStoreFactory; @@ -75,7 +78,18 @@ class ActorMigrationBase { * @stable to override * @stable to call * - * @param int $stage The migration stage + * @param int $stage The migration stage. This is a combination of + * SCHEMA_COMPAT_* flags: + * - SCHEMA_COMPAT_READ_OLD, SCHEMA_COMPAT_WRITE_OLD: Use the old schema, + * with *_user and *_user_text fields. + * - SCHEMA_COMPAT_READ_TEMP, SCHEMA_COMPAT_WRITE_TEMP: Use the new schema, + * with an actor table. Normal tables are joined via a *_actor field, + * whereas temp tables are joined to the actor table via an + * intermediate table. + * - SCHEMA_COMPAT_READ_NEW, SCHEMA_COMPAT_WRITE_NEW: Use the new + * schema. Former temp tables are no longer used, and all relevant + * tables join directly to the actor table. + * * @param ActorStoreFactory $actorStoreFactory * @param array $options Array of other options. May contain: * - allowUnknown: Allow fields not present in $fieldInfos. True by default. @@ -89,22 +103,30 @@ class ActorMigrationBase { $this->fieldInfos = $fieldInfos; $this->allowUnknown = $options['allowUnknown'] ?? true; - if ( ( $stage & SCHEMA_COMPAT_WRITE_BOTH ) === 0 ) { + $writeStage = $stage & SCHEMA_COMPAT_WRITE_MASK; + $readStage = $stage & SCHEMA_COMPAT_READ_MASK; + if ( $writeStage === 0 ) { throw new InvalidArgumentException( '$stage must include a write mode' ); } - if ( ( $stage & SCHEMA_COMPAT_READ_BOTH ) === 0 ) { + if ( $readStage === 0 ) { throw new InvalidArgumentException( '$stage must include a read mode' ); } - if ( ( $stage & SCHEMA_COMPAT_READ_BOTH ) === SCHEMA_COMPAT_READ_BOTH ) { - throw new InvalidArgumentException( 'Cannot read both schemas' ); + if ( !in_array( $readStage, + [ SCHEMA_COMPAT_READ_OLD, SCHEMA_COMPAT_READ_TEMP, SCHEMA_COMPAT_READ_NEW ] ) + ) { + throw new InvalidArgumentException( 'Cannot read multiple schemas' ); } - if ( ( $stage & SCHEMA_COMPAT_READ_OLD ) && !( $stage & SCHEMA_COMPAT_WRITE_OLD ) ) { + if ( $readStage === SCHEMA_COMPAT_READ_OLD && !( $writeStage & SCHEMA_COMPAT_WRITE_OLD ) ) { throw new InvalidArgumentException( 'Cannot read the old schema without also writing it' ); } - if ( ( $stage & SCHEMA_COMPAT_READ_NEW ) && !( $stage & SCHEMA_COMPAT_WRITE_NEW ) ) { + if ( $readStage === SCHEMA_COMPAT_READ_TEMP && !( $writeStage & SCHEMA_COMPAT_WRITE_TEMP ) ) { + throw new InvalidArgumentException( 'Cannot read the temp schema without also writing it' ); + } + if ( $readStage === SCHEMA_COMPAT_READ_NEW && !( $writeStage & SCHEMA_COMPAT_WRITE_NEW ) ) { throw new InvalidArgumentException( 'Cannot read the new schema without also writing it' ); } - $this->stage = $stage; + $this->readStage = $readStage; + $this->writeStage = $writeStage; $this->actorStoreFactory = $actorStoreFactory; } @@ -173,7 +195,7 @@ class ActorMigrationBase { * @return string */ public function isAnon( $field ) { - return ( $this->stage & SCHEMA_COMPAT_READ_NEW ) ? "$field IS NULL" : "$field = 0"; + return ( $this->readStage >= SCHEMA_COMPAT_READ_TEMP ) ? "$field IS NULL" : "$field = 0"; } /** @@ -182,7 +204,7 @@ class ActorMigrationBase { * @return string */ public function isNotAnon( $field ) { - return ( $this->stage & SCHEMA_COMPAT_READ_NEW ) ? "$field IS NOT NULL" : "$field != 0"; + return ( $this->readStage >= SCHEMA_COMPAT_READ_TEMP ) ? "$field IS NOT NULL" : "$field != 0"; } /** @@ -230,11 +252,11 @@ class ActorMigrationBase { list( $text, $actor ) = $this->getFieldNames( $key ); - if ( $this->stage & SCHEMA_COMPAT_READ_OLD ) { + if ( $this->readStage === SCHEMA_COMPAT_READ_OLD ) { $fields[$key] = $key; $fields[$text] = $text; $fields[$actor] = 'NULL'; - } else { + } elseif ( $this->readStage === SCHEMA_COMPAT_READ_TEMP ) { $tempTableInfo = $this->getTempTableInfo( $key ); if ( $tempTableInfo ) { $alias = "temp_$key"; @@ -253,6 +275,14 @@ class ActorMigrationBase { $fields[$key] = "{$alias}.actor_user"; $fields[$text] = "{$alias}.actor_name"; $fields[$actor] = $joinField; + } else /* SCHEMA_COMPAT_READ_NEW */ { + $alias = "actor_$key"; + $tables[$alias] = 'actor'; + $joins[$alias] = [ 'JOIN', "{$alias}.actor_id = {$actor}" ]; + + $fields[$key] = "{$alias}.actor_user"; + $fields[$text] = "{$alias}.actor_name"; + $fields[$actor] = $actor; } $this->joinCache[$key] = [ @@ -283,11 +313,13 @@ class ActorMigrationBase { list( $text, $actor ) = $this->getFieldNames( $key ); $ret = []; - if ( $this->stage & SCHEMA_COMPAT_WRITE_OLD ) { + if ( $this->writeStage & SCHEMA_COMPAT_WRITE_OLD ) { $ret[$key] = $user->getId(); $ret[$text] = $user->getName(); } - if ( $this->stage & SCHEMA_COMPAT_WRITE_NEW ) { + if ( $this->writeStage & SCHEMA_COMPAT_WRITE_TEMP + || $this->writeStage & SCHEMA_COMPAT_WRITE_NEW + ) { $ret[$actor] = $this->actorStoreFactory ->getActorNormalization( $dbw->getDomainID() ) ->acquireActorId( $user, $dbw ); @@ -323,50 +355,59 @@ class ActorMigrationBase { list( $text, $actor ) = $this->getFieldNames( $key ); $ret = []; $callback = null; - if ( $this->stage & SCHEMA_COMPAT_WRITE_OLD ) { + + if ( $this->writeStage & SCHEMA_COMPAT_WRITE_OLD ) { $ret[$key] = $user->getId(); $ret[$text] = $user->getName(); } - if ( $this->stage & SCHEMA_COMPAT_WRITE_NEW ) { + if ( $this->writeStage & ( SCHEMA_COMPAT_WRITE_TEMP | SCHEMA_COMPAT_WRITE_NEW ) ) { $id = $this->actorStoreFactory ->getActorNormalization( $dbw->getDomainID() ) ->acquireActorId( $user, $dbw ); + if ( $tempTableInfo ) { + if ( $this->writeStage & SCHEMA_COMPAT_WRITE_TEMP ) { + $func = __METHOD__; + $callback = static function ( $pk, array $extra ) use ( $tempTableInfo, $dbw, $id, $func ) { + $set = [ $tempTableInfo['field'] => $id ]; + foreach ( $tempTableInfo['extra'] as $to => $from ) { + if ( !array_key_exists( $from, $extra ) ) { + throw new InvalidArgumentException( "$func callback: \$extra[$from] is not provided" ); + } + $set[$to] = $extra[$from]; + } + $dbw->upsert( + $tempTableInfo['table'], + [ $tempTableInfo['pk'] => $pk ] + $set, + [ [ $tempTableInfo['pk'] ] ], + $set, + $func + ); + }; + } + if ( $this->writeStage & SCHEMA_COMPAT_WRITE_NEW ) { + $ret[$actor] = $id; + } + } else { + $ret[$actor] = $id; + } + } + + if ( $callback === null ) { + // Make a validation-only callback if there was temp table info if ( $tempTableInfo ) { $func = __METHOD__; - $callback = static function ( $pk, array $extra ) use ( $tempTableInfo, $dbw, $id, $func ) { - $set = [ $tempTableInfo['field'] => $id ]; + $callback = static function ( $pk, array $extra ) use ( $tempTableInfo, $func ) { foreach ( $tempTableInfo['extra'] as $to => $from ) { if ( !array_key_exists( $from, $extra ) ) { throw new InvalidArgumentException( "$func callback: \$extra[$from] is not provided" ); } - $set[$to] = $extra[$from]; } - $dbw->upsert( - $tempTableInfo['table'], - [ $tempTableInfo['pk'] => $pk ] + $set, - [ [ $tempTableInfo['pk'] ] ], - $set, - $func - ); }; } else { - $ret[$actor] = $id; $callback = static function ( $pk, array $extra ) { }; } - } elseif ( $tempTableInfo ) { - $func = __METHOD__; - $callback = static function ( $pk, array $extra ) use ( $tempTableInfo, $func ) { - foreach ( $tempTableInfo['extra'] as $to => $from ) { - if ( !array_key_exists( $from, $extra ) ) { - throw new InvalidArgumentException( "$func callback: \$extra[$from] is not provided" ); - } - } - }; - } else { - $callback = static function ( $pk, array $extra ) { - }; } return [ $ret, $callback ]; } @@ -436,7 +477,11 @@ class ActorMigrationBase { list( $text, $actor ) = $this->getFieldNames( $key ); // Combine data into conditions to be ORed together - if ( $this->stage & SCHEMA_COMPAT_READ_NEW ) { + if ( $this->readStage === SCHEMA_COMPAT_READ_NEW ) { + if ( $actors ) { + $conds['newactor'] = $db->makeList( [ $actor => $actors ], IDatabase::LIST_AND ); + } + } elseif ( $this->readStage === SCHEMA_COMPAT_READ_TEMP ) { if ( $actors ) { $tempTableInfo = $this->getTempTableInfo( $key ); if ( $tempTableInfo ) { diff --git a/includes/DefaultSettings.php b/includes/DefaultSettings.php index c20a0e8563b..82f34533300 100644 --- a/includes/DefaultSettings.php +++ b/includes/DefaultSettings.php @@ -2390,6 +2390,28 @@ $wgDatabaseReplicaLagCritical = 30; */ $wgMultiContentRevisionSchemaMigrationStage = SCHEMA_COMPAT_NEW; +/** + * Actor table schema migration stage, for migration from the temporary table + * revision_actor_temp to the revision.rev_actor field. + * + * Use the SCHEMA_COMPAT_XXX flags. Supported values: + * + * - SCHEMA_COMPAT_TEMP + * - SCHEMA_COMPAT_WRITE_TEMP_AND_NEW | SCHEMA_COMPAT_READ_TEMP + * - SCHEMA_COMPAT_WRITE_TEMP_AND_NEW | SCHEMA_COMPAT_READ_NEW + * - SCHEMA_COMPAT_NEW + * + * History: + * - 1.31: Added + * - 1.32: Now uses SCHEMA_COMPAT_XXX flags + * - 1.34: Removed, implicitly SCHEMA_COMPAT_NEW always + * - 1.37: Re-added with SCHEMA_COMPAT_NEW renamed to SCHEMA_COMPAT_TEMP for + * a new migration which removes temporary tables. + * + * @var int An appropriate combination of SCHEMA_COMPAT_XXX flags. + */ +$wgActorTableSchemaMigrationStage = SCHEMA_COMPAT_TEMP; + // endregion -- End of DB settings /***************************************************************************/ diff --git a/includes/Defines.php b/includes/Defines.php index e8b285469f6..afaac43ac3e 100644 --- a/includes/Defines.php +++ b/includes/Defines.php @@ -252,16 +252,29 @@ define( 'SHELL_MAX_ARG_STRLEN', '100000' ); * * - SCHEMA_COMPAT_WRITE_OLD: Whether information is written to the old schema. * - SCHEMA_COMPAT_READ_OLD: Whether information stored in the old schema is read. - * - SCHEMA_COMPAT_WRITE_NEW: Whether information is written to the new schema. - * - SCHEMA_COMPAT_READ_NEW: Whether information stored in the new schema is read. + * - SCHEMA_COMPAT_WRITE_TEMP: Whether information is written to a temporary + * intermediate schema. + * - SCHEMA_COMPAT_READ_TEMP: Whether information is read from the temporary + * intermediate schema. + * - SCHEMA_COMPAT_WRITE_NEW: Whether information is written to the new schema + * - SCHEMA_COMPAT_READ_NEW: Whether information is read from the new schema */ define( 'SCHEMA_COMPAT_WRITE_OLD', 0x01 ); define( 'SCHEMA_COMPAT_READ_OLD', 0x02 ); -define( 'SCHEMA_COMPAT_WRITE_NEW', 0x10 ); -define( 'SCHEMA_COMPAT_READ_NEW', 0x20 ); +define( 'SCHEMA_COMPAT_WRITE_TEMP', 0x10 ); +define( 'SCHEMA_COMPAT_READ_TEMP', 0x20 ); +define( 'SCHEMA_COMPAT_WRITE_NEW', 0x100 ); +define( 'SCHEMA_COMPAT_READ_NEW', 0x200 ); +define( 'SCHEMA_COMPAT_WRITE_MASK', + SCHEMA_COMPAT_WRITE_OLD | SCHEMA_COMPAT_WRITE_TEMP | SCHEMA_COMPAT_WRITE_NEW ); +define( 'SCHEMA_COMPAT_READ_MASK', + SCHEMA_COMPAT_READ_OLD | SCHEMA_COMPAT_READ_TEMP | SCHEMA_COMPAT_READ_NEW ); define( 'SCHEMA_COMPAT_WRITE_BOTH', SCHEMA_COMPAT_WRITE_OLD | SCHEMA_COMPAT_WRITE_NEW ); +define( 'SCHEMA_COMPAT_WRITE_OLD_AND_TEMP', SCHEMA_COMPAT_WRITE_OLD | SCHEMA_COMPAT_WRITE_TEMP ); +define( 'SCHEMA_COMPAT_WRITE_TEMP_AND_NEW', SCHEMA_COMPAT_WRITE_TEMP | SCHEMA_COMPAT_WRITE_NEW ); define( 'SCHEMA_COMPAT_READ_BOTH', SCHEMA_COMPAT_READ_OLD | SCHEMA_COMPAT_READ_NEW ); define( 'SCHEMA_COMPAT_OLD', SCHEMA_COMPAT_WRITE_OLD | SCHEMA_COMPAT_READ_OLD ); +define( 'SCHEMA_COMPAT_TEMP', SCHEMA_COMPAT_WRITE_TEMP | SCHEMA_COMPAT_READ_TEMP ); define( 'SCHEMA_COMPAT_NEW', SCHEMA_COMPAT_WRITE_NEW | SCHEMA_COMPAT_READ_NEW ); /** @} */ diff --git a/includes/MergeHistory.php b/includes/MergeHistory.php index 541a2704558..f708cd38aad 100644 --- a/includes/MergeHistory.php +++ b/includes/MergeHistory.php @@ -302,6 +302,8 @@ class MergeHistory { * @return Status status of the history merge */ public function merge( Authority $performer, $reason = '' ) { + global $wgActorTableSchemaMigrationStage; + $status = new Status(); // Check validity and permissions required for merge @@ -333,16 +335,18 @@ class MergeHistory { } // Update denormalized revactor_page too - $this->dbw->update( - 'revision_actor_temp', - [ 'revactor_page' => $this->dest->getId() ], - [ - 'revactor_page' => $this->source->getId(), - // Slightly hacky, but should work given the values assigned in this class - str_replace( 'rev_timestamp', 'revactor_timestamp', $this->getTimeWhere() ) - ], - __METHOD__ - ); + if ( $wgActorTableSchemaMigrationStage & SCHEMA_COMPAT_WRITE_TEMP ) { + $this->dbw->update( + 'revision_actor_temp', + [ 'revactor_page' => $this->dest->getId() ], + [ + 'revactor_page' => $this->source->getId(), + // Slightly hacky, but should work given the values assigned in this class + str_replace( 'rev_timestamp', 'revactor_timestamp', $this->getTimeWhere() ) + ], + __METHOD__ + ); + } $haveRevisions = $this->dbw->lockForUpdate( 'revision', diff --git a/includes/Rest/Handler/PageHistoryCountHandler.php b/includes/Rest/Handler/PageHistoryCountHandler.php index 78fdd742147..3f384d6ca23 100644 --- a/includes/Rest/Handler/PageHistoryCountHandler.php +++ b/includes/Rest/Handler/PageHistoryCountHandler.php @@ -2,6 +2,7 @@ namespace MediaWiki\Rest\Handler; +use ActorMigration; use ChangeTags; use MediaWiki\Page\ExistingPageRecord; use MediaWiki\Page\PageLookup; @@ -61,6 +62,9 @@ class PageHistoryCountHandler extends SimpleHandler { /** @var WANObjectCache */ private $cache; + /** @var ActorMigration */ + private $actorMigration; + /** @var RevisionRecord|false|null */ private $revision = false; @@ -77,6 +81,7 @@ class PageHistoryCountHandler extends SimpleHandler { * @param ILoadBalancer $loadBalancer * @param WANObjectCache $cache * @param PageLookup $pageLookup + * @param ActorMigration $actorMigration */ public function __construct( RevisionStore $revisionStore, @@ -84,7 +89,8 @@ class PageHistoryCountHandler extends SimpleHandler { GroupPermissionsLookup $groupPermissionsLookup, ILoadBalancer $loadBalancer, WANObjectCache $cache, - PageLookup $pageLookup + PageLookup $pageLookup, + ActorMigration $actorMigration ) { $this->revisionStore = $revisionStore; $this->changeTagDefStore = $nameTableStoreFactory->getChangeTagDef(); @@ -92,6 +98,7 @@ class PageHistoryCountHandler extends SimpleHandler { $this->loadBalancer = $loadBalancer; $this->cache = $cache; $this->pageLookup = $pageLookup; + $this->actorMigration = $actorMigration; } private function normalizeType( $type ) { @@ -416,6 +423,8 @@ class PageHistoryCountHandler extends SimpleHandler { protected function getAnonCount( $pageId, RevisionRecord $fromRev = null ) { $dbr = $this->loadBalancer->getConnectionRef( DB_REPLICA ); + $revQuery = $this->actorMigration->getJoin( 'rev_user' ); + $cond = [ 'rev_page' => $pageId, 'actor_user IS NULL', @@ -429,26 +438,22 @@ class PageHistoryCountHandler extends SimpleHandler { "OR rev_timestamp > {$oldTs}"; } + // This redundant join condition tells MySQL that rev_page and revactor_page are the + // same, so it can propagate the condition + if ( isset( $revQuery['tables']['temp_rev_user'] ) /* SCHEMA_COMPAT_READ_TEMP */ ) { + $revQuery['joins']['temp_rev_user'][1] = + "temp_rev_user.revactor_rev = rev_id AND revactor_page = rev_page"; + } + $edits = $dbr->selectRowCount( [ - 'revision_actor_temp', 'revision', - 'actor' - ], + ] + $revQuery['tables'], '1', $cond, __METHOD__, [ 'LIMIT' => self::COUNT_LIMITS['anonymous'] + 1 ], // extra to detect truncation - [ - 'revision' => [ - 'JOIN', - 'revactor_rev = rev_id AND revactor_page = rev_page' - ], - 'actor' => [ - 'JOIN', - 'revactor_actor = actor_id' - ] - ] + $revQuery['joins'] ); return $edits; } @@ -461,6 +466,15 @@ class PageHistoryCountHandler extends SimpleHandler { protected function getBotCount( $pageId, RevisionRecord $fromRev = null ) { $dbr = $this->loadBalancer->getConnectionRef( DB_REPLICA ); + $revQuery = $this->actorMigration->getJoin( 'rev_user' ); + + // This redundant join condition tells MySQL that rev_page and revactor_page are the + // same, so it can propagate the condition + if ( isset( $revQuery['tables']['temp_rev_user'] ) /* SCHEMA_COMPAT_READ_TEMP */ ) { + $revQuery['joins']['temp_rev_user'][1] = + "temp_rev_user.revactor_rev = rev_id AND revactor_page = rev_page"; + } + $cond = [ 'rev_page=' . intval( $pageId ), $dbr->bitAnd( 'rev_deleted', @@ -470,7 +484,7 @@ class PageHistoryCountHandler extends SimpleHandler { 'user_groups', '1', [ - 'actor.actor_user = ug_user', + $revQuery['fields']['rev_user'] . ' = ug_user', 'ug_group' => $this->groupPermissionsLookup->getGroupsWithPermission( 'bot' ), 'ug_expiry IS NULL OR ug_expiry >= ' . $dbr->addQuotes( $dbr->timestamp() ) ], @@ -486,24 +500,13 @@ class PageHistoryCountHandler extends SimpleHandler { $edits = $dbr->selectRowCount( [ - 'revision_actor_temp', 'revision', - 'actor', - ], + ] + $revQuery['tables'], '1', $cond, __METHOD__, [ 'LIMIT' => self::COUNT_LIMITS['bot'] + 1 ], // extra to detect truncation - [ - 'revision' => [ - 'JOIN', - 'revactor_rev = rev_id AND revactor_page = rev_page' - ], - 'actor' => [ - 'JOIN', - 'revactor_actor = actor_id' - ], - ] + $revQuery['joins'] ); return $edits; } diff --git a/includes/Rest/Handler/PageHistoryHandler.php b/includes/Rest/Handler/PageHistoryHandler.php index 9dd03f1c8de..6bad3e6e86f 100644 --- a/includes/Rest/Handler/PageHistoryHandler.php +++ b/includes/Rest/Handler/PageHistoryHandler.php @@ -193,8 +193,10 @@ class PageHistoryHandler extends SimpleHandler { if ( $params['filter'] ) { // This redundant join condition tells MySQL that rev_page and revactor_page are the // same, so it can propagate the condition - $revQuery['joins']['temp_rev_user'][1] = - "temp_rev_user.revactor_rev = rev_id AND revactor_page = rev_page"; + if ( isset( $revQuery['tables']['temp_rev_user'] ) /* SCHEMA_COMPAT_READ_TEMP */ ) { + $revQuery['joins']['temp_rev_user'][1] = + "temp_rev_user.revactor_rev = rev_id AND revactor_page = rev_page"; + } // The validator ensures this value, if present, is one of the expected values switch ( $params['filter'] ) { diff --git a/includes/Rest/coreRoutes.json b/includes/Rest/coreRoutes.json index e8677dd4daa..3cd9c6e4037 100644 --- a/includes/Rest/coreRoutes.json +++ b/includes/Rest/coreRoutes.json @@ -20,7 +20,8 @@ "GroupPermissionsLookup", "DBLoadBalancer", "MainWANObjectCache", - "PageStore" + "PageStore", + "ActorMigration" ] }, { diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php index da9b8a62977..5b1deddef66 100644 --- a/includes/ServiceWiring.php +++ b/includes/ServiceWiring.php @@ -151,7 +151,7 @@ use Wikimedia\UUID\GlobalIdGenerator; return [ 'ActorMigration' => static function ( MediaWikiServices $services ) : ActorMigration { return new ActorMigration( - SCHEMA_COMPAT_NEW, + $services->getMainConfig()->get( 'ActorTableSchemaMigrationStage' ), $services->getActorStoreFactory() ); }, diff --git a/includes/actions/InfoAction.php b/includes/actions/InfoAction.php index ff904d3e9a3..9b19d8cee05 100644 --- a/includes/actions/InfoAction.php +++ b/includes/actions/InfoAction.php @@ -827,6 +827,8 @@ class InfoAction extends FormlessAction { self::getCacheKey( $cache, $page->getTitle(), $page->getLatest() ), WANObjectCache::TTL_WEEK, static function ( $oldValue, &$ttl, &$setOpts ) use ( $page, $config, $fname, $services ) { + global $wgActorTableSchemaMigrationStage; + $title = $page->getTitle(); $id = $title->getArticleID(); @@ -834,10 +836,17 @@ class InfoAction extends FormlessAction { $dbrWatchlist = wfGetDB( DB_REPLICA, 'watchlist' ); $setOpts += Database::getCacheSetOptions( $dbr, $dbrWatchlist ); - $tables = [ 'revision_actor_temp' ]; - $field = 'revactor_actor'; - $pageField = 'revactor_page'; - $tsField = 'revactor_timestamp'; + if ( $wgActorTableSchemaMigrationStage & SCHEMA_COMPAT_READ_NEW ) { + $tables = [ 'revision' ]; + $field = 'rev_actor'; + $pageField = 'rev_page'; + $tsField = 'rev_timestamp'; + } else /* SCHEMA_COMPAT_READ_TEMP */ { + $tables = [ 'revision_actor_temp' ]; + $field = 'revactor_actor'; + $pageField = 'revactor_page'; + $tsField = 'revactor_timestamp'; + } $joins = []; $watchedItemStore = $services->getWatchedItemStore(); diff --git a/includes/api/ApiQueryAllRevisions.php b/includes/api/ApiQueryAllRevisions.php index 86dc36752fc..f24097f77ac 100644 --- a/includes/api/ApiQueryAllRevisions.php +++ b/includes/api/ApiQueryAllRevisions.php @@ -82,6 +82,8 @@ class ApiQueryAllRevisions extends ApiQueryRevisionsBase { * @return void */ protected function run( ApiPageSet $resultPageSet = null ) { + global $wgActorTableSchemaMigrationStage; + $db = $this->getDB(); $params = $this->extractRequestParams( false ); @@ -92,7 +94,9 @@ class ApiQueryAllRevisions extends ApiQueryRevisionsBase { $tsField = 'rev_timestamp'; $idField = 'rev_id'; $pageField = 'rev_page'; - if ( $params['user'] !== null ) { + if ( $params['user'] !== null && + ( $wgActorTableSchemaMigrationStage & SCHEMA_COMPAT_READ_TEMP ) + ) { // The query is probably best done using the actor_timestamp index on // revision_actor_temp. Use the denormalized fields from that table. $tsField = 'revactor_timestamp'; diff --git a/includes/api/ApiQueryContributors.php b/includes/api/ApiQueryContributors.php index e46cd8e1621..51c0c20bc3e 100644 --- a/includes/api/ApiQueryContributors.php +++ b/includes/api/ApiQueryContributors.php @@ -79,6 +79,8 @@ class ApiQueryContributors extends ApiQueryBase { } public function execute() { + global $wgActorTableSchemaMigrationStage; + $db = $this->getDB(); $params = $this->extractRequestParams(); $this->requireMaxOneParameter( $params, 'group', 'excludegroup', 'rights', 'excluderights' ); @@ -111,10 +113,14 @@ class ApiQueryContributors extends ApiQueryBase { $result = $this->getResult(); $revQuery = $this->revisionStore->getQueryInfo(); - // Target indexes on the revision_actor_temp table. - $pageField = 'revactor_page'; - $idField = 'revactor_actor'; - $countField = 'revactor_actor'; + // For SCHEMA_COMPAT_READ_TEMP, target indexes on the + // revision_actor_temp table, otherwise on the revision table. + $pageField = ( $wgActorTableSchemaMigrationStage & SCHEMA_COMPAT_READ_TEMP ) + ? 'revactor_page' : 'rev_page'; + $idField = ( $wgActorTableSchemaMigrationStage & SCHEMA_COMPAT_READ_TEMP ) + ? 'revactor_actor' : 'rev_actor'; + $countField = ( $wgActorTableSchemaMigrationStage & SCHEMA_COMPAT_READ_TEMP ) + ? 'revactor_actor' : 'rev_actor'; // First, count anons $this->addTables( $revQuery['tables'] ); diff --git a/includes/api/ApiQueryRevisions.php b/includes/api/ApiQueryRevisions.php index ef9b4faa777..c5a02748c6c 100644 --- a/includes/api/ApiQueryRevisions.php +++ b/includes/api/ApiQueryRevisions.php @@ -125,6 +125,8 @@ class ApiQueryRevisions extends ApiQueryRevisionsBase { } protected function run( ApiPageSet $resultPageSet = null ) { + global $wgActorTableSchemaMigrationStage; + $params = $this->extractRequestParams( false ); // If any of those parameters are used, work in 'enumeration' mode. @@ -183,7 +185,9 @@ class ApiQueryRevisions extends ApiQueryRevisionsBase { ]; $useIndex = []; - if ( $params['user'] !== null ) { + if ( $params['user'] !== null && + ( $wgActorTableSchemaMigrationStage & SCHEMA_COMPAT_READ_TEMP ) + ) { // We're going to want to use the page_actor_timestamp index (on revision_actor_temp) // so use that table's denormalized fields. $idField = 'revactor_rev'; diff --git a/includes/api/ApiQueryUserContribs.php b/includes/api/ApiQueryUserContribs.php index 349b94d43b0..75ddd3df0fd 100644 --- a/includes/api/ApiQueryUserContribs.php +++ b/includes/api/ApiQueryUserContribs.php @@ -309,33 +309,42 @@ class ApiQueryUserContribs extends ApiQueryBase { * @param int $limit */ private function prepareQuery( array $users, $limit ) { + global $wgActorTableSchemaMigrationStage; + $this->resetQueryParams(); $db = $this->getDB(); $revQuery = $this->revisionStore->getQueryInfo( [ 'page' ] ); - $revWhere = $this->actorMigration->getWhere( $db, 'rev_user', $users ); - $orderUserField = 'rev_actor'; - $userField = $this->orderBy === 'actor' ? 'revactor_actor' : 'actor_name'; - $tsField = 'revactor_timestamp'; - $idField = 'revactor_rev'; - // T221511: MySQL/MariaDB (10.1.37) can sometimes irrationally decide that querying `actor` - // before `revision_actor_temp` and filesorting is somehow better than querying $limit+1 rows - // from `revision_actor_temp`. Tell it not to reorder the query (and also reorder it ourselves - // because as generated by RevisionStore it'll have `revision` first rather than - // `revision_actor_temp`). But not when uctag is used, as it seems as likely to be harmed as - // helped in that case, and not when there's only one User because in that case it fetches - // the one `actor` row as a constant and doesn't filesort. - if ( count( $users ) > 1 && !isset( $this->params['tag'] ) ) { - $revQuery['joins']['revision'] = $revQuery['joins']['temp_rev_user']; - unset( $revQuery['joins']['temp_rev_user'] ); - $this->addOption( 'STRAIGHT_JOIN' ); - // It isn't actually necesssary to reorder $revQuery['tables'] as Database does the right thing - // when join conditions are given for all joins, but Gergő is wary of relying on that so pull - // `revision_actor_temp` to the start. - $revQuery['tables'] = - [ 'temp_rev_user' => $revQuery['tables']['temp_rev_user'] ] + $revQuery['tables']; + if ( $wgActorTableSchemaMigrationStage & SCHEMA_COMPAT_READ_TEMP ) { + $orderUserField = 'rev_actor'; + $userField = $this->orderBy === 'actor' ? 'revactor_actor' : 'actor_name'; + $tsField = 'revactor_timestamp'; + $idField = 'revactor_rev'; + + // T221511: MySQL/MariaDB (10.1.37) can sometimes irrationally decide that querying `actor` + // before `revision_actor_temp` and filesorting is somehow better than querying $limit+1 rows + // from `revision_actor_temp`. Tell it not to reorder the query (and also reorder it ourselves + // because as generated by RevisionStore it'll have `revision` first rather than + // `revision_actor_temp`). But not when uctag is used, as it seems as likely to be harmed as + // helped in that case, and not when there's only one User because in that case it fetches + // the one `actor` row as a constant and doesn't filesort. + if ( count( $users ) > 1 && !isset( $this->params['tag'] ) ) { + $revQuery['joins']['revision'] = $revQuery['joins']['temp_rev_user']; + unset( $revQuery['joins']['temp_rev_user'] ); + $this->addOption( 'STRAIGHT_JOIN' ); + // It isn't actually necesssary to reorder $revQuery['tables'] as Database does the right thing + // when join conditions are given for all joins, but Gergő is wary of relying on that so pull + // `revision_actor_temp` to the start. + $revQuery['tables'] = + [ 'temp_rev_user' => $revQuery['tables']['temp_rev_user'] ] + $revQuery['tables']; + } + } else /* SCHEMA_COMPAT_READ_NEW */ { + $orderUserField = 'rev_actor'; + $userField = $this->orderBy === 'actor' ? 'rev_actor' : 'actor_name'; + $tsField = 'rev_timestamp'; + $idField = 'rev_id'; } $this->addTables( $revQuery['tables'] ); diff --git a/includes/page/WikiPage.php b/includes/page/WikiPage.php index 464d50f81c0..7b08cde1a61 100644 --- a/includes/page/WikiPage.php +++ b/includes/page/WikiPage.php @@ -2991,7 +2991,7 @@ class WikiPage implements Page, IDBAccessObject, PageRecord { * @return bool */ protected function archiveRevisions( $dbw, $id, $suppress ) { - global $wgDeleteRevisionsBatchSize; + global $wgDeleteRevisionsBatchSize, $wgActorTableSchemaMigrationStage; // Given the lock above, we can be confident in the title and page ID values $namespace = $this->getTitle()->getNamespace(); @@ -3087,7 +3087,9 @@ class WikiPage implements Page, IDBAccessObject, PageRecord { $dbw->delete( 'revision', [ 'rev_id' => $revids ], __METHOD__ ); $dbw->delete( 'revision_comment_temp', [ 'revcomment_rev' => $revids ], __METHOD__ ); - $dbw->delete( 'revision_actor_temp', [ 'revactor_rev' => $revids ], __METHOD__ ); + if ( $wgActorTableSchemaMigrationStage & SCHEMA_COMPAT_WRITE_TEMP ) { + $dbw->delete( 'revision_actor_temp', [ 'revactor_rev' => $revids ], __METHOD__ ); + } // Also delete records from ip_changes as applicable. if ( count( $ipRevIds ) > 0 ) { diff --git a/includes/revisiondelete/RevisionDeleteUser.php b/includes/revisiondelete/RevisionDeleteUser.php index 74aa13a1ff4..7e1ef745c0e 100644 --- a/includes/revisiondelete/RevisionDeleteUser.php +++ b/includes/revisiondelete/RevisionDeleteUser.php @@ -44,6 +44,8 @@ class RevisionDeleteUser { * @return bool True on success, false on failure (e.g. invalid user ID) */ private static function setUsernameBitfields( $name, $userId, $op, IDatabase $dbw = null ) { + global $wgActorTableSchemaMigrationStage; + if ( !$userId || ( $op !== '|' && $op !== '&' ) ) { return false; // sanity check } @@ -70,14 +72,27 @@ class RevisionDeleteUser { $actorId = $dbw->selectField( 'actor', 'actor_id', [ 'actor_name' => $name ], __METHOD__ ); if ( $actorId ) { # Hide name from live edits - $ids = $dbw->selectFieldValues( - 'revision_actor_temp', 'revactor_rev', [ 'revactor_actor' => $actorId ], __METHOD__ - ); - if ( $ids ) { + # This query depends on the actor migration read stage, not the + # write stage, because the stage determines how we find the rows to + # delete. The write stage determines whether or not to write to + # rev_actor and revision_actor_temp which is not relevant here. + if ( $wgActorTableSchemaMigrationStage & SCHEMA_COMPAT_READ_TEMP ) { + $ids = $dbw->selectFieldValues( + 'revision_actor_temp', 'revactor_rev', [ 'revactor_actor' => $actorId ], __METHOD__ + ); + if ( $ids ) { + $dbw->update( + 'revision', + [ self::buildSetBitDeletedField( 'rev_deleted', $op, $delUser, $dbw ) ], + [ 'rev_id' => $ids ], + __METHOD__ + ); + } + } else /* SCHEMA_COMPAT_READ_NEW */ { $dbw->update( 'revision', [ self::buildSetBitDeletedField( 'rev_deleted', $op, $delUser, $dbw ) ], - [ 'rev_id' => $ids ], + [ 'rev_actor' => $actorId ], __METHOD__ ); } diff --git a/includes/specials/pagers/ContribsPager.php b/includes/specials/pagers/ContribsPager.php index 495c7cc9239..aaeb25e4f70 100644 --- a/includes/specials/pagers/ContribsPager.php +++ b/includes/specials/pagers/ContribsPager.php @@ -310,7 +310,6 @@ class ContribsPager extends RangeChronologicalPager { } else { $conds = $this->actorMigration->getWhere( $dbr, 'rev_user', $user ); if ( isset( $conds['orconds']['actor'] ) ) { - // @todo: This will need changing when revision_actor_temp goes away return 'revision_actor_temp'; } } @@ -345,7 +344,6 @@ class ContribsPager extends RangeChronologicalPager { $queryInfo['conds'][] = $conds['conds']; // Force the appropriate index to avoid bad query plans (T189026) if ( isset( $conds['orconds']['actor'] ) ) { - // @todo: This will need changing when revision_actor_temp goes away $queryInfo['options']['USE INDEX']['temp_rev_user'] = 'actor_timestamp'; } } diff --git a/includes/user/UserEditTracker.php b/includes/user/UserEditTracker.php index 3fc9252a50a..328f1645274 100644 --- a/includes/user/UserEditTracker.php +++ b/includes/user/UserEditTracker.php @@ -172,7 +172,7 @@ class UserEditTracker { $dbr = $this->loadBalancer->getConnectionRef( DB_REPLICA ); $actorWhere = $this->actorMigration->getWhere( $dbr, 'rev_user', $user ); - $tsField = isset( $actorWhere['tables']['temp_rev_user'] ) + $tsField = isset( $actorWhere['tables']['temp_rev_user'] ) // SCHEMA_COMPAT_READ_TEMP ? 'revactor_timestamp' : 'rev_timestamp'; $sortOrder = ( $type === self::FIRST_EDIT ) ? 'ASC' : 'DESC'; diff --git a/maintenance/findMissingActors.php b/maintenance/findMissingActors.php index eb08fb48bcc..b6d66c6d2eb 100644 --- a/maintenance/findMissingActors.php +++ b/maintenance/findMissingActors.php @@ -60,24 +60,14 @@ class FindMissingActors extends Maintenance { */ private $actorNormalization; - private const TABLES = [ - // 'rev_actor' => [ 'revision', 'rev_actor', 'rev_id' ], // not yet used in 1.35 - 'revactor_actor' => [ 'revision_actor_temp', 'revactor_actor', 'revactor_rev' ], - 'ar_actor' => [ 'archive', 'ar_actor', 'ar_id' ], - 'ipb_by_actor' => [ 'ipblocks', 'ipb_by_actor', 'ipb_id' ], // no index on ipb_by_actor! - 'img_actor' => [ 'image', 'img_actor', 'img_name' ], - 'oi_actor' => [ 'oldimage', 'oi_actor', 'oi_archive_name' ], // no index on oi_archive_name! - 'fa_actor' => [ 'filearchive', 'fa_actor', 'fa_id' ], - 'rc_actor' => [ 'recentchanges', 'rc_actor', 'rc_id' ], - 'log_actor' => [ 'logging', 'log_actor', 'log_id' ], - ]; + /** @var array|null */ + private $tables; public function __construct() { parent::__construct(); $this->addDescription( 'Find and fix invalid actor IDs.' ); - $this->addOption( 'field', 'The name of a database field to process. ' - . 'Possible values: ' . implode( ', ', array_keys( self::TABLES ) ), + $this->addOption( 'field', 'The name of a database field to process', true, true ); $this->addOption( 'skip', 'A comma-separated list of actor IDs to skip.', false, true ); @@ -108,6 +98,43 @@ class FindMissingActors extends Maintenance { $services->getActorNormalization(); } + /** + * @return array + */ + private function getTables() { + global $wgActorTableSchemaMigrationStage; + + if ( !$this->tables ) { + $tables = [ + 'ar_actor' => [ 'archive', 'ar_actor', 'ar_id' ], + 'ipb_by_actor' => [ 'ipblocks', 'ipb_by_actor', 'ipb_id' ], // no index on ipb_by_actor! + 'img_actor' => [ 'image', 'img_actor', 'img_name' ], + 'oi_actor' => [ 'oldimage', 'oi_actor', 'oi_archive_name' ], // no index on oi_archive_name! + 'fa_actor' => [ 'filearchive', 'fa_actor', 'fa_id' ], + 'rc_actor' => [ 'recentchanges', 'rc_actor', 'rc_id' ], + 'log_actor' => [ 'logging', 'log_actor', 'log_id' ], + ]; + + if ( $wgActorTableSchemaMigrationStage & SCHEMA_COMPAT_WRITE_TEMP ) { + $tables['revactor_actor'] = [ 'revision_actor_temp', 'revactor_actor', 'revactor_rev' ]; + } + if ( $wgActorTableSchemaMigrationStage & SCHEMA_COMPAT_WRITE_NEW ) { + $tables['rev_actor'] = [ 'revision', 'rev_actor', 'rev_id' ]; + } + $this->tables = $tables; + } + return $this->tables; + } + + /** + * @param string $field + * @return array|null + */ + private function getTableInfo( $field ) { + $tables = $this->getTables(); + return $tables[$field] ?? null; + } + /** * Returns the actor ID of the user specified with the --overwrite-with option, * or null if --overwrite-with is not set. @@ -153,14 +180,11 @@ class FindMissingActors extends Maintenance { return $actorId; } - /** - * @inheritDoc - */ public function execute() { $this->initializeServices(); $field = $this->getOption( 'field' ); - if ( !isset( self::TABLES[$field] ) ) { + if ( !$this->getTableInfo( $field ) ) { $this->fatalError( "Unknown field: $field.\n" ); } @@ -195,7 +219,7 @@ class FindMissingActors extends Maintenance { * @return array a list of row IDs, identifying rows in which the actor ID needs to be replaced. */ private function findBadActors( $field, $skip ) { - [ $table, $actorField, $idField ] = self::TABLES[$field]; + [ $table, $actorField, $idField ] = $this->getTableInfo( $field ); $this->output( "Finding invalid actor IDs in $table.$actorField...\n" ); $dbr = $this->loadBalancer->getConnectionRef( @@ -269,7 +293,7 @@ class FindMissingActors extends Maintenance { * @return int */ private function overwriteActorIDs( $field, array $ids, int $overwrite ) { - [ $table, $actorField, $idField ] = self::TABLES[$field]; + [ $table, $actorField, $idField ] = $this->getTableInfo( $field ); $count = count( $ids ); $this->output( "OVERWRITING $count actor IDs in $table.$actorField with $overwrite...\n" ); diff --git a/maintenance/migrateRevisionActorTemp.php b/maintenance/migrateRevisionActorTemp.php new file mode 100644 index 00000000000..93175584b98 --- /dev/null +++ b/maintenance/migrateRevisionActorTemp.php @@ -0,0 +1,92 @@ +addDescription( + 'Copy the data from the revision_actor_temp into the revision table' + ); + } + + protected function getUpdateKey() { + return __CLASS__; + } + + protected function doDBUpdates() { + $batchSize = $this->getBatchSize(); + + $dbw = $this->getDB( DB_PRIMARY ); + if ( !$dbw->fieldExists( 'revision', 'rev_actor', __METHOD__ ) ) { + $this->output( "Run update.php to create rev_actor.\n" ); + return false; + } + if ( !$dbw->tableExists( 'revision_actor_temp', __METHOD__ ) ) { + $this->output( "revision_actor_temp does not exist, so nothing to do.\n" ); + return true; + } + + $this->output( "Merging the revision_actor_temp table into the revision table...\n" ); + $conds = []; + $updated = 0; + while ( true ) { + $res = $dbw->newSelectQueryBuilder() + ->select( [ 'rev_id', 'rev_actor', 'revactor_actor' ] ) + ->from( 'revision' ) + ->join( 'revision_actor_temp', null, 'rev_id=revactor_rev' ) + ->where( $conds ) + ->limit( $batchSize ) + ->orderBy( 'rev_id' ) + ->caller( __METHOD__ ) + ->fetchResultSet(); + + $numRows = $res->numRows(); + + $last = null; + foreach ( $res as $row ) { + $last = $row->rev_id; + if ( !$row->rev_actor ) { + $dbw->update( + 'revision', + [ 'rev_actor' => $row->revactor_actor ], + [ 'rev_id' => $row->rev_id ], + __METHOD__ + ); + $updated += $dbw->affectedRows(); + } elseif ( $row->rev_actor !== $row->revactor_actor ) { + $this->error( + "Revision ID $row->rev_id has rev_actor = $row->rev_actor and " + . "revactor_actor = $row->revactor_actor. Ignoring the latter." + ); + } + } + + if ( $numRows < $batchSize ) { + // We must have reached the end + break; + } + + $this->output( "... rev_id=$last, updated $updated\n" ); + $conds = [ 'rev_id > ' . $dbw->addQuotes( $last ) ]; + } + + $this->output( + "Completed merge of revision_actor into the revision table, " + . "$updated rows updated.\n" + ); + + return true; + } + +} + +$maintClass = MigrateRevisionActorTemp::class; +require_once RUN_MAINTENANCE_IF_MAIN; diff --git a/maintenance/reassignEdits.php b/maintenance/reassignEdits.php index 70f3091f97a..21a5cba0686 100644 --- a/maintenance/reassignEdits.php +++ b/maintenance/reassignEdits.php @@ -76,6 +76,7 @@ class ReassignEdits extends Maintenance { * @return int Number of entries changed, or that would be changed */ private function doReassignEdits( &$from, &$to, $rc = false, $report = false ) { + $actorTableSchemaMigrationStage = $this->getConfig()->get( 'ActorTableSchemaMigrationStage' ); $dbw = $this->getDB( DB_PRIMARY ); $this->beginTransaction( $dbw, __METHOD__ ); $actorNormalization = MediaWikiServices::getInstance()->getActorNormalization(); @@ -131,12 +132,22 @@ class ReassignEdits extends Maintenance { if ( $total ) { # Reassign edits $this->output( "\nReassigning current edits..." ); - $dbw->update( - 'revision_actor_temp', - [ 'revactor_actor' => $toActorId ], - [ 'revactor_actor' => $fromActorId ], - __METHOD__ - ); + if ( $actorTableSchemaMigrationStage & SCHEMA_COMPAT_WRITE_TEMP ) { + $dbw->update( + 'revision_actor_temp', + [ 'revactor_actor' => $toActorId ], + [ 'revactor_actor' => $fromActorId ], + __METHOD__ + ); + } + if ( $actorTableSchemaMigrationStage & SCHEMA_COMPAT_WRITE_NEW ) { + $dbw->update( + 'revision', + [ 'rev_actor' => $toActorId ], + [ 'rev_actor' => $fromActorId ], + __METHOD__ + ); + } $this->output( "done.\nReassigning deleted edits..." ); $dbw->update( 'archive', [ 'ar_actor' => $toActorId ], diff --git a/tests/phpunit/includes/ActorMigrationTest.php b/tests/phpunit/includes/ActorMigrationTest.php index 8dcf032eed8..a5f4bcad71c 100644 --- a/tests/phpunit/includes/ActorMigrationTest.php +++ b/tests/phpunit/includes/ActorMigrationTest.php @@ -20,6 +20,16 @@ class ActorMigrationTest extends MediaWikiLangTestCase { 'actor', ]; + private const STAGES_BY_NAME = [ + 'old' => SCHEMA_COMPAT_WRITE_OLD | SCHEMA_COMPAT_READ_OLD, + 'read-old' => SCHEMA_COMPAT_WRITE_OLD_AND_TEMP | SCHEMA_COMPAT_READ_OLD, + 'read-temp' => SCHEMA_COMPAT_WRITE_OLD_AND_TEMP | SCHEMA_COMPAT_READ_TEMP, + 'temp' => SCHEMA_COMPAT_WRITE_TEMP | SCHEMA_COMPAT_READ_TEMP, + 'read-temp2' => SCHEMA_COMPAT_WRITE_TEMP_AND_NEW | SCHEMA_COMPAT_READ_TEMP, + 'read-new' => SCHEMA_COMPAT_WRITE_TEMP_AND_NEW | SCHEMA_COMPAT_READ_NEW, + 'new' => SCHEMA_COMPAT_WRITE_NEW | SCHEMA_COMPAT_READ_NEW + ]; + protected function getSchemaOverrides( IMaintainableDatabase $db ) { return [ 'scripts' => [ @@ -55,6 +65,21 @@ class ActorMigrationTest extends MediaWikiLangTestCase { ); } + private static function makeActorCases( $inputs, $expected ) { + foreach ( $expected as $inputName => $expectedCases ) { + foreach ( $expectedCases as list( $stages, $expected ) ) { + foreach ( $stages as $stage ) { + $cases[$inputName . ', ' . $stage] = array_merge( + [ $stage ], + $inputs[$inputName], + [ $expected ] + ); + } + } + } + return $cases; + } + /** * @dataProvider provideConstructor * @param int $stage @@ -76,201 +101,161 @@ class ActorMigrationTest extends MediaWikiLangTestCase { return [ [ 0, '$stage must include a write mode' ], [ SCHEMA_COMPAT_READ_OLD, '$stage must include a write mode' ], - [ SCHEMA_COMPAT_READ_NEW, '$stage must include a write mode' ], + [ SCHEMA_COMPAT_READ_TEMP, '$stage must include a write mode' ], [ SCHEMA_COMPAT_READ_BOTH, '$stage must include a write mode' ], [ SCHEMA_COMPAT_WRITE_OLD, '$stage must include a read mode' ], [ SCHEMA_COMPAT_WRITE_OLD | SCHEMA_COMPAT_READ_OLD, null ], [ - SCHEMA_COMPAT_WRITE_OLD | SCHEMA_COMPAT_READ_NEW, - 'Cannot read the new schema without also writing it' + SCHEMA_COMPAT_WRITE_OLD | SCHEMA_COMPAT_READ_TEMP, + 'Cannot read the temp schema without also writing it' ], - [ SCHEMA_COMPAT_WRITE_OLD | SCHEMA_COMPAT_READ_BOTH, 'Cannot read both schemas' ], + [ SCHEMA_COMPAT_WRITE_OLD | SCHEMA_COMPAT_READ_BOTH, 'Cannot read multiple schemas' ], - [ SCHEMA_COMPAT_WRITE_NEW, '$stage must include a read mode' ], + [ SCHEMA_COMPAT_WRITE_TEMP, '$stage must include a read mode' ], [ - SCHEMA_COMPAT_WRITE_NEW | SCHEMA_COMPAT_READ_OLD, + SCHEMA_COMPAT_WRITE_TEMP | SCHEMA_COMPAT_READ_OLD, 'Cannot read the old schema without also writing it' ], - [ SCHEMA_COMPAT_WRITE_NEW | SCHEMA_COMPAT_READ_NEW, null ], - [ SCHEMA_COMPAT_WRITE_NEW | SCHEMA_COMPAT_READ_BOTH, 'Cannot read both schemas' ], + [ SCHEMA_COMPAT_WRITE_TEMP | SCHEMA_COMPAT_READ_TEMP, null ], + [ SCHEMA_COMPAT_WRITE_TEMP | SCHEMA_COMPAT_READ_BOTH, 'Cannot read multiple schemas' ], - [ SCHEMA_COMPAT_WRITE_BOTH, '$stage must include a read mode' ], - [ SCHEMA_COMPAT_WRITE_BOTH | SCHEMA_COMPAT_READ_OLD, null ], - [ SCHEMA_COMPAT_WRITE_BOTH | SCHEMA_COMPAT_READ_NEW, null ], - [ SCHEMA_COMPAT_WRITE_BOTH | SCHEMA_COMPAT_READ_BOTH, 'Cannot read both schemas' ], + [ SCHEMA_COMPAT_WRITE_OLD_AND_TEMP, '$stage must include a read mode' ], + [ SCHEMA_COMPAT_WRITE_OLD_AND_TEMP | SCHEMA_COMPAT_READ_OLD, null ], + [ SCHEMA_COMPAT_WRITE_OLD_AND_TEMP | SCHEMA_COMPAT_READ_TEMP, null ], + [ SCHEMA_COMPAT_WRITE_OLD_AND_TEMP | SCHEMA_COMPAT_READ_BOTH, 'Cannot read multiple schemas' ], ]; } /** * @dataProvider provideGetJoin - * @param int $stage + * @param string $stageName * @param string $key * @param array $expect */ - public function testGetJoin( $stage, $key, $expect ) { + public function testGetJoin( $stageName, $key, $expect ) { + $stage = self::STAGES_BY_NAME[$stageName]; $m = $this->getMigration( $stage ); $result = $m->getJoin( $key ); $this->assertEquals( $expect, $result ); } public static function provideGetJoin() { - return [ - 'Simple table, old' => [ - SCHEMA_COMPAT_OLD, 'am1_user', [ - 'tables' => [], - 'fields' => [ - 'am1_user' => 'am1_user', - 'am1_user_text' => 'am1_user_text', - 'am1_actor' => 'NULL', - ], - 'joins' => [], + $inputs = [ + 'Simple table' => [ 'am1_user' ], + 'Special name' => [ 'am3_xxx' ], + 'Temp table' => [ 'am2_user' ], + ]; + $expected = [ + 'Simple table' => [ + [ + [ 'old', 'read-old' ], + [ + 'tables' => [], + 'fields' => [ + 'am1_user' => 'am1_user', + 'am1_user_text' => 'am1_user_text', + 'am1_actor' => 'NULL', + ], + 'joins' => [], + ] ], - ], - 'Simple table, read-old' => [ - SCHEMA_COMPAT_WRITE_BOTH | SCHEMA_COMPAT_READ_OLD, 'am1_user', [ - 'tables' => [], - 'fields' => [ - 'am1_user' => 'am1_user', - 'am1_user_text' => 'am1_user_text', - 'am1_actor' => 'NULL', - ], - 'joins' => [], - ], - ], - 'Simple table, read-new' => [ - SCHEMA_COMPAT_WRITE_BOTH | SCHEMA_COMPAT_READ_NEW, 'am1_user', [ - 'tables' => [ 'actor_am1_user' => 'actor' ], - 'fields' => [ - 'am1_user' => 'actor_am1_user.actor_user', - 'am1_user_text' => 'actor_am1_user.actor_name', - 'am1_actor' => 'am1_actor', - ], - 'joins' => [ - 'actor_am1_user' => [ 'JOIN', 'actor_am1_user.actor_id = am1_actor' ], - ], - ], - ], - 'Simple table, new' => [ - SCHEMA_COMPAT_NEW, 'am1_user', [ - 'tables' => [ 'actor_am1_user' => 'actor' ], - 'fields' => [ - 'am1_user' => 'actor_am1_user.actor_user', - 'am1_user_text' => 'actor_am1_user.actor_name', - 'am1_actor' => 'am1_actor', - ], - 'joins' => [ - 'actor_am1_user' => [ 'JOIN', 'actor_am1_user.actor_id = am1_actor' ], + [ + [ 'read-temp', 'temp', 'read-temp2', 'read-new', 'new' ], + [ + 'tables' => [ 'actor_am1_user' => 'actor' ], + 'fields' => [ + 'am1_user' => 'actor_am1_user.actor_user', + 'am1_user_text' => 'actor_am1_user.actor_name', + 'am1_actor' => 'am1_actor', + ], + 'joins' => [ + 'actor_am1_user' => [ 'JOIN', 'actor_am1_user.actor_id = am1_actor' ], + ], ], ], ], - 'Special name, old' => [ - SCHEMA_COMPAT_OLD, 'am3_xxx', [ - 'tables' => [], - 'fields' => [ - 'am3_xxx' => 'am3_xxx', - 'am3_xxx_text' => 'am3_xxx_text', - 'am3_xxx_actor' => 'NULL', - ], - 'joins' => [], - ], - ], - 'Special name, read-old' => [ - SCHEMA_COMPAT_WRITE_BOTH | SCHEMA_COMPAT_READ_OLD, 'am3_xxx', [ - 'tables' => [], - 'fields' => [ - 'am3_xxx' => 'am3_xxx', - 'am3_xxx_text' => 'am3_xxx_text', - 'am3_xxx_actor' => 'NULL', - ], - 'joins' => [], - ], - ], - 'Special name, read-new' => [ - SCHEMA_COMPAT_WRITE_BOTH | SCHEMA_COMPAT_READ_NEW, 'am3_xxx', [ - 'tables' => [ 'actor_am3_xxx' => 'actor' ], - 'fields' => [ - 'am3_xxx' => 'actor_am3_xxx.actor_user', - 'am3_xxx_text' => 'actor_am3_xxx.actor_name', - 'am3_xxx_actor' => 'am3_xxx_actor', - ], - 'joins' => [ - 'actor_am3_xxx' => [ 'JOIN', 'actor_am3_xxx.actor_id = am3_xxx_actor' ], + 'Special name' => [ + [ + [ 'old', 'read-old' ], + [ + 'tables' => [], + 'fields' => [ + 'am3_xxx' => 'am3_xxx', + 'am3_xxx_text' => 'am3_xxx_text', + 'am3_xxx_actor' => 'NULL', + ], + 'joins' => [], ], ], - ], - 'Special name, new' => [ - SCHEMA_COMPAT_NEW, 'am3_xxx', [ - 'tables' => [ 'actor_am3_xxx' => 'actor' ], - 'fields' => [ - 'am3_xxx' => 'actor_am3_xxx.actor_user', - 'am3_xxx_text' => 'actor_am3_xxx.actor_name', - 'am3_xxx_actor' => 'am3_xxx_actor', - ], - 'joins' => [ - 'actor_am3_xxx' => [ 'JOIN', 'actor_am3_xxx.actor_id = am3_xxx_actor' ], + [ + [ 'read-temp', 'temp', 'read-temp2', 'read-new', 'new' ], + [ + 'tables' => [ 'actor_am3_xxx' => 'actor' ], + 'fields' => [ + 'am3_xxx' => 'actor_am3_xxx.actor_user', + 'am3_xxx_text' => 'actor_am3_xxx.actor_name', + 'am3_xxx_actor' => 'am3_xxx_actor', + ], + 'joins' => [ + 'actor_am3_xxx' => [ 'JOIN', 'actor_am3_xxx.actor_id = am3_xxx_actor' ], + ], ], ], ], - 'Temp table, old' => [ - SCHEMA_COMPAT_OLD, 'am2_user', [ - 'tables' => [], - 'fields' => [ - 'am2_user' => 'am2_user', - 'am2_user_text' => 'am2_user_text', - 'am2_actor' => 'NULL', - ], - 'joins' => [], + 'Temp table' => [ + [ + [ 'old', 'read-old' ], + [ + 'tables' => [], + 'fields' => [ + 'am2_user' => 'am2_user', + 'am2_user_text' => 'am2_user_text', + 'am2_actor' => 'NULL', + ], + 'joins' => [], + ] ], - ], - 'Temp table, read-old' => [ - SCHEMA_COMPAT_WRITE_BOTH | SCHEMA_COMPAT_READ_OLD, 'am2_user', [ - 'tables' => [], - 'fields' => [ - 'am2_user' => 'am2_user', - 'am2_user_text' => 'am2_user_text', - 'am2_actor' => 'NULL', - ], - 'joins' => [], + [ + [ 'read-temp', 'temp', 'read-temp2' ], + [ + 'tables' => [ + 'temp_am2_user' => 'actormigration2_temp', + 'actor_am2_user' => 'actor', + ], + 'fields' => [ + 'am2_user' => 'actor_am2_user.actor_user', + 'am2_user_text' => 'actor_am2_user.actor_name', + 'am2_actor' => 'temp_am2_user.am2t_actor', + ], + 'joins' => [ + 'temp_am2_user' => [ 'JOIN', 'temp_am2_user.am2t_id = am2_id' ], + 'actor_am2_user' => [ 'JOIN', 'actor_am2_user.actor_id = temp_am2_user.am2t_actor' ], + ], + ] ], - ], - 'Temp table, read-new' => [ - SCHEMA_COMPAT_WRITE_BOTH | SCHEMA_COMPAT_READ_NEW, 'am2_user', [ - 'tables' => [ - 'temp_am2_user' => 'actormigration2_temp', - 'actor_am2_user' => 'actor', - ], - 'fields' => [ - 'am2_user' => 'actor_am2_user.actor_user', - 'am2_user_text' => 'actor_am2_user.actor_name', - 'am2_actor' => 'temp_am2_user.am2t_actor', - ], - 'joins' => [ - 'temp_am2_user' => [ 'JOIN', 'temp_am2_user.am2t_id = am2_id' ], - 'actor_am2_user' => [ 'JOIN', 'actor_am2_user.actor_id = temp_am2_user.am2t_actor' ], - ], - ], - ], - 'Temp table, new' => [ - SCHEMA_COMPAT_NEW, 'am2_user', [ - 'tables' => [ - 'temp_am2_user' => 'actormigration2_temp', - 'actor_am2_user' => 'actor', - ], - 'fields' => [ - 'am2_user' => 'actor_am2_user.actor_user', - 'am2_user_text' => 'actor_am2_user.actor_name', - 'am2_actor' => 'temp_am2_user.am2t_actor', - ], - 'joins' => [ - 'temp_am2_user' => [ 'JOIN', 'temp_am2_user.am2t_id = am2_id' ], - 'actor_am2_user' => [ 'JOIN', 'actor_am2_user.actor_id = temp_am2_user.am2t_actor' ], - ], + [ + [ 'read-new', 'new' ], + [ + 'tables' => [ + 'actor_am2_user' => 'actor', + ], + 'fields' => [ + 'am2_user' => 'actor_am2_user.actor_user', + 'am2_user_text' => 'actor_am2_user.actor_name', + 'am2_actor' => 'am2_actor', + ], + 'joins' => [ + 'actor_am2_user' => [ 'JOIN', 'actor_am2_user.actor_id = am2_actor' ], + ], + ] ], ], ]; + + return self::makeActorCases( $inputs, $expected ); } private const ACTORS = [ @@ -322,13 +307,14 @@ class ActorMigrationTest extends MediaWikiLangTestCase { /** * @dataProvider provideGetWhere - * @param int $stage + * @param string $stageName * @param string $key * @param UserIdentity|UserIdentity[]|null|false $users * @param bool $useId * @param array $expect */ - public function testGetWhere( $stage, $key, $users, $useId, $expect ) { + public function testGetWhere( $stageName, $key, $users, $useId, $expect ) { + $stage = self::STAGES_BY_NAME[$stageName]; if ( !isset( $expect['conds'] ) ) { $expect['conds'] = '(' . implode( ') OR (', $expect['orconds'] ) . ')'; } @@ -350,199 +336,186 @@ class ActorMigrationTest extends MediaWikiLangTestCase { new UserIdentityValue( 0, '2600:1004:b14a:5ddd:3ebe:bba4:bfba:f37e' ), ]; - return [ - 'Simple table, old' => [ - SCHEMA_COMPAT_OLD, 'am1_user', $genericUser, true, [ - 'tables' => [], - 'orconds' => [ 'userid' => "am1_user = 1" ], - 'joins' => [], - ], - ], - 'Simple table, read-old' => [ - SCHEMA_COMPAT_WRITE_BOTH | SCHEMA_COMPAT_READ_OLD, 'am1_user', $genericUser, true, [ - 'tables' => [], - 'orconds' => [ 'userid' => "am1_user = 1" ], - 'joins' => [], - ], - ], - 'Simple table, read-new' => [ - SCHEMA_COMPAT_WRITE_BOTH | SCHEMA_COMPAT_READ_NEW, 'am1_user', $genericUser, true, [ - 'tables' => [], - 'orconds' => [ 'actor' => "am1_actor = 11" ], - 'joins' => [], - ], - ], - 'Simple table, new' => [ - SCHEMA_COMPAT_NEW, 'am1_user', $genericUser, true, [ - 'tables' => [], - 'orconds' => [ 'actor' => "am1_actor = 11" ], - 'joins' => [], - ], - ], - - 'Special name, old' => [ - SCHEMA_COMPAT_OLD, 'am3_xxx', $genericUser, true, [ - 'tables' => [], - 'orconds' => [ 'userid' => "am3_xxx = 1" ], - 'joins' => [], - ], - ], - 'Special name, read-old' => [ - SCHEMA_COMPAT_WRITE_BOTH | SCHEMA_COMPAT_READ_OLD, 'am3_xxx', $genericUser, true, [ - 'tables' => [], - 'orconds' => [ 'userid' => "am3_xxx = 1" ], - 'joins' => [], - ], - ], - 'Special name, read-new' => [ - SCHEMA_COMPAT_WRITE_BOTH | SCHEMA_COMPAT_READ_NEW, 'am3_xxx', $genericUser, true, [ - 'tables' => [], - 'orconds' => [ 'actor' => "am3_xxx_actor = 11" ], - 'joins' => [], - ], - ], - 'Special name, new' => [ - SCHEMA_COMPAT_NEW, 'am3_xxx', $genericUser, true, [ - 'tables' => [], - 'orconds' => [ 'actor' => "am3_xxx_actor = 11" ], - 'joins' => [], - ], - ], - - 'Temp table, old' => [ - SCHEMA_COMPAT_OLD, 'am2_user', $genericUser, true, [ - 'tables' => [], - 'orconds' => [ 'userid' => "am2_user = 1" ], - 'joins' => [], - ], - ], - 'Temp table, read-old' => [ - SCHEMA_COMPAT_WRITE_BOTH | SCHEMA_COMPAT_READ_OLD, 'am2_user', $genericUser, true, [ - 'tables' => [], - 'orconds' => [ 'userid' => "am2_user = 1" ], - 'joins' => [], - ], - ], - 'Temp table, read-new' => [ - SCHEMA_COMPAT_WRITE_BOTH | SCHEMA_COMPAT_READ_NEW, 'am2_user', $genericUser, true, [ - 'tables' => [ - 'temp_am2_user' => 'actormigration2_temp', - ], - 'orconds' => [ 'actor' => "temp_am2_user.am2t_actor = 11" ], - 'joins' => [ - 'temp_am2_user' => [ 'JOIN', 'temp_am2_user.am2t_id = am2_id' ], - ], - ], - ], - 'Temp table, new' => [ - SCHEMA_COMPAT_NEW, 'am2_user', $genericUser, true, [ - 'tables' => [ - 'temp_am2_user' => 'actormigration2_temp', - ], - 'orconds' => [ 'actor' => "temp_am2_user.am2t_actor = 11" ], - 'joins' => [ - 'temp_am2_user' => [ 'JOIN', 'temp_am2_user.am2t_id = am2_id' ], - ], - ], - ], - - 'Multiple users, old' => [ - SCHEMA_COMPAT_OLD, 'am1_user', $complicatedUsers, true, [ - 'tables' => [], - 'orconds' => [ - 'userid' => "am1_user IN (1,2,3) ", - 'username' => "am1_user_text IN ('192.168.12.34','192.168.12.35'," - . "'2600:1004:B14A:5DDD:3EBE:BBA4:BFBA:F37E') " - ], - 'joins' => [], - ], - ], - 'Multiple users, read-old' => [ - SCHEMA_COMPAT_WRITE_BOTH | SCHEMA_COMPAT_READ_OLD, 'am1_user', $complicatedUsers, true, [ - 'tables' => [], - 'orconds' => [ - 'userid' => "am1_user IN (1,2,3) ", - 'username' => "am1_user_text IN ('192.168.12.34','192.168.12.35'," - . "'2600:1004:B14A:5DDD:3EBE:BBA4:BFBA:F37E') " - ], - 'joins' => [], - ], - ], - 'Multiple users, read-new' => [ - SCHEMA_COMPAT_WRITE_BOTH | SCHEMA_COMPAT_READ_NEW, 'am1_user', $complicatedUsers, true, [ - 'tables' => [], - 'orconds' => [ 'actor' => "am1_actor IN (11,12,34) " ], - 'joins' => [], - ], - ], - 'Multiple users, new' => [ - SCHEMA_COMPAT_NEW, 'am1_user', $complicatedUsers, true, [ - 'tables' => [], - 'orconds' => [ 'actor' => "am1_actor IN (11,12,34) " ], - 'joins' => [], - ], - ], - - 'Multiple users, no use ID, old' => [ - SCHEMA_COMPAT_OLD, 'am1_user', $complicatedUsers, false, [ - 'tables' => [], - 'orconds' => [ - 'username' => "am1_user_text IN ('User1','User2','User3','192.168.12.34'," - . "'192.168.12.35','2600:1004:B14A:5DDD:3EBE:BBA4:BFBA:F37E') " - ], - 'joins' => [], - ], - ], - 'Multiple users, no use ID, read-old' => [ - SCHEMA_COMPAT_WRITE_BOTH | SCHEMA_COMPAT_READ_OLD, 'am1_user', $complicatedUsers, false, [ - 'tables' => [], - 'orconds' => [ - 'username' => "am1_user_text IN ('User1','User2','User3','192.168.12.34'," - . "'192.168.12.35','2600:1004:B14A:5DDD:3EBE:BBA4:BFBA:F37E') " - ], - 'joins' => [], - ], - ], - 'Multiple users, no use ID, read-new' => [ - SCHEMA_COMPAT_WRITE_BOTH | SCHEMA_COMPAT_READ_NEW, 'am1_user', $complicatedUsers, false, [ - 'tables' => [], - 'orconds' => [ 'actor' => "am1_actor IN (11,12,34) " ], - 'joins' => [], - ], - ], - 'Multiple users, no use ID, new' => [ - SCHEMA_COMPAT_NEW, 'am1_user', $complicatedUsers, false, [ - 'tables' => [], - 'orconds' => [ 'actor' => "am1_actor IN (11,12,34) " ], - 'joins' => [], - ], - ], - - 'Empty $users' => [ - SCHEMA_COMPAT_NEW, 'am1_user', [], true, [ - 'tables' => [], - 'conds' => '1=0', - 'orconds' => [], - 'joins' => [], - ], - ], - 'Null $users' => [ - SCHEMA_COMPAT_NEW, 'am1_user', null, true, [ - 'tables' => [], - 'conds' => '1=0', - 'orconds' => [], - 'joins' => [], - ], - ], - 'False $users' => [ - SCHEMA_COMPAT_NEW, 'am1_user', false, true, [ - 'tables' => [], - 'conds' => '1=0', - 'orconds' => [], - 'joins' => [], - ], - ], + $inputs = [ + 'Simple table' => [ 'am1_user', $genericUser, true ], + 'Special name' => [ 'am3_xxx', $genericUser, true ], + 'Temp table' => [ 'am2_user', $genericUser, true ], + 'Multiple users' => [ 'am1_user', $complicatedUsers, true ], + 'Multiple users, no use ID' => [ 'am1_user', $complicatedUsers, false ], + 'Empty $users' => [ 'am1_user', [], true ], + 'Null $users' => [ 'am1_user', null, true ], + 'False $users' => [ 'am1_user', false, true ], ]; + + $expected = [ + 'Simple table' => [ + [ + [ 'old', 'read-old' ], + [ + 'tables' => [], + 'orconds' => [ 'userid' => "am1_user = 1" ], + 'joins' => [], + ] + ], [ + [ 'read-temp', 'temp', 'read-temp2' ], + [ + 'tables' => [], + 'orconds' => [ 'actor' => "am1_actor = 11" ], + 'joins' => [], + ], + ], [ + [ 'read-new', 'new' ], + [ + 'tables' => [], + 'orconds' => [ 'newactor' => "am1_actor = 11" ], + 'joins' => [], + ], + ], + ], + + 'Special name' => [ + [ + [ 'old', 'read-old' ], + [ + 'tables' => [], + 'orconds' => [ 'userid' => "am3_xxx = 1" ], + 'joins' => [], + ], + ], [ + [ 'read-temp', 'temp', 'read-temp2' ], + [ + 'tables' => [], + 'orconds' => [ 'actor' => "am3_xxx_actor = 11" ], + 'joins' => [], + ], + ], [ + [ 'read-new', 'new' ], + [ + 'tables' => [], + 'orconds' => [ 'newactor' => "am3_xxx_actor = 11" ], + 'joins' => [], + ], + ], + ], + + 'Temp table' => [ + [ + [ 'old', 'read-old' ], + [ + 'tables' => [], + 'orconds' => [ 'userid' => "am2_user = 1" ], + 'joins' => [], + ], + ], [ + [ 'read-temp', 'temp', 'read-temp2' ], + [ + 'tables' => [ + 'temp_am2_user' => 'actormigration2_temp', + ], + 'orconds' => [ 'actor' => "temp_am2_user.am2t_actor = 11" ], + 'joins' => [ + 'temp_am2_user' => [ 'JOIN', 'temp_am2_user.am2t_id = am2_id' ], + ], + ], + ], [ + [ 'read-new', 'new' ], + [ + 'tables' => [], + 'orconds' => [ 'newactor' => "am2_actor = 11" ], + 'joins' => [], + ] + ] + ], + + 'Multiple users' => [ + [ + [ 'old', 'read-old' ], + [ + 'tables' => [], + 'orconds' => [ + 'userid' => "am1_user IN (1,2,3) ", + 'username' => "am1_user_text IN ('192.168.12.34','192.168.12.35'," + . "'2600:1004:B14A:5DDD:3EBE:BBA4:BFBA:F37E') " + ], + 'joins' => [], + ] + ], [ + [ 'read-temp', 'temp', 'read-temp2' ], + [ + 'tables' => [], + 'orconds' => [ 'actor' => "am1_actor IN (11,12,34) " ], + 'joins' => [], + ] + ], [ + [ 'read-new', 'new' ], + [ + 'tables' => [], + 'orconds' => [ 'newactor' => "am1_actor IN (11,12,34) " ], + 'joins' => [], + ] + ] + ], + + 'Multiple users, no use ID' => [ + [ + [ 'old', 'read-old' ], + [ + 'tables' => [], + 'orconds' => [ + 'username' => "am1_user_text IN ('User1','User2','User3','192.168.12.34'," + . "'192.168.12.35','2600:1004:B14A:5DDD:3EBE:BBA4:BFBA:F37E') " + ], + 'joins' => [], + ], + ], [ + [ 'read-temp', 'temp', 'read-temp2' ], + [ + 'tables' => [], + 'orconds' => [ 'actor' => "am1_actor IN (11,12,34) " ], + 'joins' => [], + ] + ], [ + [ 'read-new', 'new' ], + [ + 'tables' => [], + 'orconds' => [ 'newactor' => "am1_actor IN (11,12,34) " ], + 'joins' => [], + ] + ] + ], + + 'Empty $users' => [ [ + [ 'old', 'read-old', 'read-temp', 'temp', 'read-temp2', 'read-new', 'new' ], + [ + 'tables' => [], + 'conds' => '1=0', + 'orconds' => [], + 'joins' => [], + ], + ] ], + + 'Null $users' => [ [ + [ 'old', 'read-old', 'read-temp', 'temp', 'read-temp2', 'read-new', 'new' ], + [ + 'tables' => [], + 'conds' => '1=0', + 'orconds' => [], + 'joins' => [], + ], + ] ], + + 'False $users' => [ [ + [ 'old', 'read-old', 'read-temp', 'temp', 'read-temp2', 'read-new', 'new' ], + [ + 'tables' => [], + 'conds' => '1=0', + 'orconds' => [], + 'joins' => [], + ], + ] ], + ]; + + return self::makeActorCases( $inputs, $expected ); } /** @@ -569,40 +542,54 @@ class ActorMigrationTest extends MediaWikiLangTestCase { $u = $this->getTestUser()->getUser(); $user = new UserIdentityValue( $u->getId(), $u->getName() ); - $stageNames = [ - SCHEMA_COMPAT_OLD => 'old', - SCHEMA_COMPAT_WRITE_BOTH | SCHEMA_COMPAT_READ_OLD => 'write-both-read-old', - SCHEMA_COMPAT_WRITE_BOTH | SCHEMA_COMPAT_READ_NEW => 'write-both-read-new', - SCHEMA_COMPAT_NEW => 'new', - ]; + $stageNames = array_flip( self::STAGES_BY_NAME ); $stages = [ - SCHEMA_COMPAT_OLD => [ - SCHEMA_COMPAT_OLD, - SCHEMA_COMPAT_WRITE_BOTH | SCHEMA_COMPAT_READ_OLD, + 'old' => [ + 'old', + 'read-old', ], - SCHEMA_COMPAT_WRITE_BOTH | SCHEMA_COMPAT_READ_OLD => [ - SCHEMA_COMPAT_OLD, - SCHEMA_COMPAT_WRITE_BOTH | SCHEMA_COMPAT_READ_OLD, - SCHEMA_COMPAT_WRITE_BOTH | SCHEMA_COMPAT_READ_NEW, - SCHEMA_COMPAT_NEW + 'read-old' => [ + 'old', + 'read-old', + 'read-temp', + 'temp' ], - SCHEMA_COMPAT_WRITE_BOTH | SCHEMA_COMPAT_READ_NEW => [ - SCHEMA_COMPAT_OLD, - SCHEMA_COMPAT_WRITE_BOTH | SCHEMA_COMPAT_READ_OLD, - SCHEMA_COMPAT_WRITE_BOTH | SCHEMA_COMPAT_READ_NEW, - SCHEMA_COMPAT_NEW + 'read-temp' => [ + 'old', + 'read-old', + 'read-temp', + 'temp' ], - SCHEMA_COMPAT_NEW => [ - SCHEMA_COMPAT_WRITE_BOTH | SCHEMA_COMPAT_READ_NEW, - SCHEMA_COMPAT_NEW + 'temp' => [ + 'read-temp', + 'temp' + ], + 'read-temp2' => [ + 'read-temp', + 'temp', + 'read-temp2', + 'read-new', + 'new' + ], + 'read-new' => [ + 'read-temp', + 'temp', + 'read-temp2', + 'read-new', + 'new' + ], + 'new' => [ + 'read-new', + 'new' ], ]; $nameKey = $key . '_text'; $actorKey = ( $key === 'am3_xxx' ? $key : substr( $key, 0, -5 ) ) . '_actor'; - foreach ( $stages as $writeStage => $possibleReadStages ) { + foreach ( $stages as $writeStageName => $possibleReadStages ) { + $writeStage = self::STAGES_BY_NAME[$writeStageName]; $w = $this->getMigration( $writeStage ); if ( $usesTemp ) { @@ -620,7 +607,9 @@ class ActorMigrationTest extends MediaWikiLangTestCase { $this->assertArrayNotHasKey( $key, $fields, "old field, stage={$stageNames[$writeStage]}" ); $this->assertArrayNotHasKey( $nameKey, $fields, "old field, stage={$stageNames[$writeStage]}" ); } - if ( ( $writeStage & SCHEMA_COMPAT_WRITE_NEW ) && !$usesTemp ) { + if ( ( ( $writeStage & SCHEMA_COMPAT_WRITE_TEMP ) && !$usesTemp ) + || ( $writeStage & SCHEMA_COMPAT_WRITE_NEW ) + ) { $this->assertArrayHasKey( $actorKey, $fields, "new field, stage={$stageNames[$writeStage]}" ); } else { @@ -634,7 +623,8 @@ class ActorMigrationTest extends MediaWikiLangTestCase { $callback( $id, [] ); } - foreach ( $possibleReadStages as $readStage ) { + foreach ( $possibleReadStages as $readStageName ) { + $readStage = self::STAGES_BY_NAME[$readStageName]; $r = $this->getMigration( $readStage ); $queryInfo = $r->getJoin( $key ); @@ -664,12 +654,11 @@ class ActorMigrationTest extends MediaWikiLangTestCase { } public static function provideStages() { - return [ - 'old' => [ SCHEMA_COMPAT_OLD ], - 'read-old' => [ SCHEMA_COMPAT_WRITE_BOTH | SCHEMA_COMPAT_READ_OLD ], - 'read-new' => [ SCHEMA_COMPAT_WRITE_BOTH | SCHEMA_COMPAT_READ_NEW ], - 'new' => [ SCHEMA_COMPAT_NEW ], - ]; + $cases = []; + foreach ( self::STAGES_BY_NAME as $name => $stage ) { + $cases[$name] = [ $stage ]; + } + return $cases; } /** @@ -768,7 +757,8 @@ class ActorMigrationTest extends MediaWikiLangTestCase { $this->assertSame( $user->getId(), (int)$row->am2_user ); $this->assertSame( $user->getName(), $row->am2_user_text ); $this->assertSame( - ( $stage & SCHEMA_COMPAT_READ_NEW ) ? $user->getActorId() : 0, + ( ( $stage & SCHEMA_COMPAT_READ_MASK ) >= SCHEMA_COMPAT_READ_TEMP ) + ? $user->getActorId() : 0, (int)$row->am2_actor ); @@ -781,7 +771,7 @@ class ActorMigrationTest extends MediaWikiLangTestCase { $this->assertArrayNotHasKey( 'dummy_user', $fields ); $this->assertArrayNotHasKey( 'dummy_user_text', $fields ); } - if ( $stage & SCHEMA_COMPAT_WRITE_NEW ) { + if ( $stage & SCHEMA_COMPAT_WRITE_TEMP || $stage & SCHEMA_COMPAT_WRITE_NEW ) { $this->assertSame( $user->getActorId(), $fields['dummy_actor'] ); } else { $this->assertArrayNotHasKey( 'dummy_actor', $fields ); @@ -796,24 +786,26 @@ class ActorMigrationTest extends MediaWikiLangTestCase { /** * @dataProvider provideIsAnon - * @param int $stage + * @param string $stage * @param string $isAnon * @param string $isNotAnon */ public function testIsAnon( $stage, $isAnon, $isNotAnon ) { - $m = $this->getMigration( $stage ); + $numericStage = self::STAGES_BY_NAME[$stage]; + $m = $this->getMigration( $numericStage ); $this->assertSame( $isAnon, $m->isAnon( 'foo' ) ); $this->assertSame( $isNotAnon, $m->isNotAnon( 'foo' ) ); } public static function provideIsAnon() { return [ - 'old' => [ SCHEMA_COMPAT_OLD, 'foo = 0', 'foo != 0' ], - 'read-old' => [ SCHEMA_COMPAT_WRITE_BOTH | SCHEMA_COMPAT_READ_OLD, 'foo = 0', 'foo != 0' ], - 'read-new' => [ - SCHEMA_COMPAT_WRITE_BOTH | SCHEMA_COMPAT_READ_NEW, 'foo IS NULL', 'foo IS NOT NULL' - ], - 'new' => [ SCHEMA_COMPAT_NEW, 'foo IS NULL', 'foo IS NOT NULL' ], + 'old' => [ 'old', 'foo = 0', 'foo != 0' ], + 'read-old' => [ 'read-old', 'foo = 0', 'foo != 0' ], + 'read-temp' => [ 'read-temp', 'foo IS NULL', 'foo IS NOT NULL' ], + 'temp' => [ 'temp', 'foo IS NULL', 'foo IS NOT NULL' ], + 'read-temp2' => [ 'temp', 'foo IS NULL', 'foo IS NOT NULL' ], + 'read-new' => [ 'temp', 'foo IS NULL', 'foo IS NOT NULL' ], + 'new' => [ 'temp', 'foo IS NULL', 'foo IS NOT NULL' ], ]; } @@ -830,7 +822,7 @@ class ActorMigrationTest extends MediaWikiLangTestCase { 'removedVersion' => '1.34', ], ], - SCHEMA_COMPAT_NEW, + SCHEMA_COMPAT_TEMP, MediaWikiServices::getInstance()->getActorStoreFactory() ) extends ActorMigrationBase { public function checkDeprecationForTest( $key ) { diff --git a/tests/phpunit/includes/ActorMigrationTest.sql b/tests/phpunit/includes/ActorMigrationTest.sql index f387dac0690..3b891bd1e14 100644 --- a/tests/phpunit/includes/ActorMigrationTest.sql +++ b/tests/phpunit/includes/ActorMigrationTest.sql @@ -10,7 +10,8 @@ CREATE TABLE /*_*/actormigration1 ( CREATE TABLE /*_*/actormigration2 ( am2_id integer not null, am2_user integer, - am2_user_text varchar(200) + am2_user_text varchar(200), + am2_actor integer ); CREATE TABLE /*_*/actormigration2_temp ( diff --git a/tests/phpunit/includes/Revision/RevisionQueryInfoTest.php b/tests/phpunit/includes/Revision/RevisionQueryInfoTest.php index 1567534887e..517ba510912 100644 --- a/tests/phpunit/includes/Revision/RevisionQueryInfoTest.php +++ b/tests/phpunit/includes/Revision/RevisionQueryInfoTest.php @@ -51,7 +51,7 @@ class RevisionQueryInfoTest extends MediaWikiIntegrationTestCase { return $fields; } - protected function getNewCommentQueryFields( $prefix ) { + protected function getCommentQueryFields( $prefix ) { return [ "{$prefix}_comment_text" => "comment_{$prefix}_comment.comment_text", "{$prefix}_comment_data" => "comment_{$prefix}_comment.comment_data", @@ -59,19 +59,25 @@ class RevisionQueryInfoTest extends MediaWikiIntegrationTestCase { ]; } - protected function getNewActorQueryFields( $prefix, $tmp = false ) { + protected function getActorQueryFields( $prefix, $tmp = false ) { if ( $tmp ) { return [ "{$prefix}_user" => "actor_{$prefix}_user.actor_user", "{$prefix}_user_text" => "actor_{$prefix}_user.actor_name", "{$prefix}_actor" => "temp_{$prefix}_user.{$prefix}actor_actor", ]; - } else { + } elseif ( $prefix === 'ar' ) { return [ "{$prefix}_actor", "{$prefix}_user" => 'archive_actor.actor_user', "{$prefix}_user_text" => 'archive_actor.actor_name', ]; + } else { + return [ + "{$prefix}_actor" => "{$prefix}_actor", + "{$prefix}_user" => "actor_{$prefix}_user.actor_user", + "{$prefix}_user_text" => "actor_{$prefix}_user.actor_name", + ]; } } @@ -117,8 +123,8 @@ class RevisionQueryInfoTest extends MediaWikiIntegrationTestCase { ], 'fields' => array_merge( $this->getArchiveQueryFields( false ), - $this->getNewActorQueryFields( 'ar' ), - $this->getNewCommentQueryFields( 'ar' ) + $this->getActorQueryFields( 'ar' ), + $this->getCommentQueryFields( 'ar' ) ), 'joins' => [ 'comment_ar_comment' @@ -131,8 +137,10 @@ class RevisionQueryInfoTest extends MediaWikiIntegrationTestCase { public function provideQueryInfo() { // TODO: more option variations - yield 'page and user option' => [ - [], + yield 'page and user option, actor-temp' => [ + [ + 'wgActorTableSchemaMigrationStage' => SCHEMA_COMPAT_TEMP, + ], [ 'page', 'user' ], [ 'tables' => [ @@ -148,8 +156,8 @@ class RevisionQueryInfoTest extends MediaWikiIntegrationTestCase { $this->getRevisionQueryFields( false ), $this->getPageQueryFields(), $this->getUserQueryFields(), - $this->getNewActorQueryFields( 'rev', 'temp_rev_user.revactor_actor' ), - $this->getNewCommentQueryFields( 'rev' ) + $this->getActorQueryFields( 'rev', 'temp_rev_user.revactor_actor' ), + $this->getCommentQueryFields( 'rev' ) ), 'joins' => [ 'page' => [ 'JOIN', [ 'page_id = rev_page' ] ], @@ -167,8 +175,46 @@ class RevisionQueryInfoTest extends MediaWikiIntegrationTestCase { ], ] ]; - yield 'no options' => [ - [], + yield 'page and user option, actor-new' => [ + [ + 'wgActorTableSchemaMigrationStage' => SCHEMA_COMPAT_NEW, + ], + [ 'page', 'user' ], + [ + 'tables' => [ + 'revision', + 'page', + 'user', + 'temp_rev_comment' => 'revision_comment_temp', + 'actor_rev_user' => 'actor', + 'comment_rev_comment' => 'comment', + ], + 'fields' => array_merge( + $this->getRevisionQueryFields( false ), + $this->getPageQueryFields(), + $this->getUserQueryFields(), + $this->getActorQueryFields( 'rev' ), + $this->getCommentQueryFields( 'rev' ) + ), + 'joins' => [ + 'page' => [ 'JOIN', [ 'page_id = rev_page' ] ], + 'user' => [ + 'LEFT JOIN', + [ 'actor_rev_user.actor_user != 0', 'user_id = actor_rev_user.actor_user' ], + ], + 'comment_rev_comment' => [ + 'JOIN', + 'comment_rev_comment.comment_id = temp_rev_comment.revcomment_comment_id', + ], + 'temp_rev_comment' => [ 'JOIN', 'temp_rev_comment.revcomment_rev = rev_id' ], + 'actor_rev_user' => [ 'JOIN', 'actor_rev_user.actor_id = rev_actor' ] + ], + ] + ]; + yield 'no options, actor-temp' => [ + [ + 'wgActorTableSchemaMigrationStage' => SCHEMA_COMPAT_TEMP, + ], [], [ 'tables' => [ @@ -180,8 +226,8 @@ class RevisionQueryInfoTest extends MediaWikiIntegrationTestCase { ], 'fields' => array_merge( $this->getRevisionQueryFields( false ), - $this->getNewActorQueryFields( 'rev', 'temp_rev_user.revactor_actor' ), - $this->getNewCommentQueryFields( 'rev' ) + $this->getActorQueryFields( 'rev', 'temp_rev_user.revactor_actor' ), + $this->getCommentQueryFields( 'rev' ) ), 'joins' => [ 'comment_rev_comment' => [ @@ -194,6 +240,33 @@ class RevisionQueryInfoTest extends MediaWikiIntegrationTestCase { ] ] ]; + yield 'no options, actor-new' => [ + [ + 'wgActorTableSchemaMigrationStage' => SCHEMA_COMPAT_NEW, + ], + [], + [ + 'tables' => [ + 'revision', + 'temp_rev_comment' => 'revision_comment_temp', + 'actor_rev_user' => 'actor', + 'comment_rev_comment' => 'comment', + ], + 'fields' => array_merge( + $this->getRevisionQueryFields( false ), + $this->getActorQueryFields( 'rev' ), + $this->getCommentQueryFields( 'rev' ) + ), + 'joins' => [ + 'comment_rev_comment' => [ + 'JOIN', + 'comment_rev_comment.comment_id = temp_rev_comment.revcomment_comment_id', + ], + 'temp_rev_comment' => [ 'JOIN', 'temp_rev_comment.revcomment_rev = rev_id' ], + 'actor_rev_user' => [ 'JOIN', 'actor_rev_user.actor_id = rev_actor' ], + ] + ] + ]; } public function provideSlotsQueryInfo() {