Thijs/wiki.techinc.nl
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Stop using legacy ActorMigration field "ar_user"

Browse source 
Bug: T278917
Change-Id: Ib6bdb727d58a1732448a2034732269f2f125be7e
This commit is contained in:
Tim Starling 2021-04-19 10:53:24 +10:00
parent 768cb1874e
commit 419dde00f5
11 changed files with 88 additions and 105 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

6
includes/Revision/RevisionFactory.php
View file

@ -79,7 +79,11 @@ interface RevisionFactory extends IDBAccessObject {
/**
* Make a fake revision object from an archive table row. This is queried
* for permissions or even inserted (as in Special:Undelete)
* for permissions or even inserted (as in Special:Undelete).
*
* The user ID and user name may optionally be supplied using the aliases
* ar_user and ar_user_text (the names of fields which existed before
* MW 1.34).
*
* MCR migration note: this replaces Revision::newFromArchiveRow
*

25
includes/Revision/RevisionStore.php
View file

@ -1526,6 +1526,10 @@ class RevisionStore
* Make a fake revision object from an archive table row. This is queried
* for permissions or even inserted (as in Special:Undelete)
*
* The user ID and user name may optionally be supplied using the aliases
* ar_user and ar_user_text (the names of fields which existed before
* MW 1.34).
*
* MCR migration note: this replaces Revision::newFromArchiveRow
*
* @param \stdClass $row
@ -2809,6 +2813,10 @@ class RevisionStore
* Return the tables, fields, and join conditions to be selected to create
* a new RevisionArchiveRecord object.
*
* Since 1.34, ar_user and ar_user_text have not been present in the
* database, but they continue to be available in query results as
* aliases.
*
* MCR migration note: this replaces Revision::getArchiveQueryInfo
*
* @since 1.31
@ -2820,9 +2828,11 @@ class RevisionStore
*/
public function getArchiveQueryInfo() {
$commentQuery = $this->commentStore->getJoin( 'ar_comment' );
$actorQuery = $this->actorMigration->getJoin( 'ar_user' );
$ret = [
'tables' => [ 'archive' ] + $commentQuery['tables'] + $actorQuery['tables'],
'tables' => [
'archive',
'archive_actor' => 'actor'
] + $commentQuery['tables'],
'fields' => [
'ar_id',
'ar_page_id',
@ -2834,9 +2844,14 @@ class RevisionStore
'ar_deleted',
'ar_len',
'ar_parent_id',
'ar_sha1',
] + $commentQuery['fields'] + $actorQuery['fields'],
'joins' => $commentQuery['joins'] + $actorQuery['joins'],
'ar_sha1',
'ar_actor',
'ar_user' => 'archive_actor.actor_user',
'ar_user_text' => 'archive_actor.actor_name',
] + $commentQuery['fields'],
'joins' => [
'archive_actor' => [ 'JOIN', 'actor_id=ar_actor' ]
] + $commentQuery['joins'],
];
return $ret;

24
includes/api/ApiQueryAllDeletedRevisions.php
View file

@ -125,6 +125,10 @@ class ApiQueryAllDeletedRevisions extends ApiQueryRevisionsBase {
} else {
$this->addFields( [ 'ar_timestamp', 'ar_rev_id', 'ar_id' ] );
}
if ( $params['user'] !== null || $params['excludeuser'] !== null ) {
$this->addTables( 'actor' );
$this->addJoinConds( [ 'actor' => 'actor_id=ar_actor' ] );
}
}
if ( $this->fld_tags ) {
@ -227,19 +231,13 @@ class ApiQueryAllDeletedRevisions extends ApiQueryRevisionsBase {
}
if ( $params['user'] !== null ) {
// Don't query by user ID here, it might be able to use the ar_usertext_timestamp index.
$actorQuery = ActorMigration::newMigration()
->getWhere( $db, 'ar_user', $params['user'], false );
$this->addTables( $actorQuery['tables'] );
$this->addJoinConds( $actorQuery['joins'] );
$this->addWhere( $actorQuery['conds'] );
// We could get the actor ID from the ActorStore, but it's probably
// uncached at this point, and the non-generator case needs an actor
// join anyway so adding this join here is normally free. This should
// use the ar_actor_timestamp index.
$this->addWhereFld( 'actor_name', $params['user'] );
} elseif ( $params['excludeuser'] !== null ) {
// Here there's no chance of using ar_usertext_timestamp.
$actorQuery = ActorMigration::newMigration()
->getWhere( $db, 'ar_user', $params['excludeuser'] );
$this->addTables( $actorQuery['tables'] );
$this->addJoinConds( $actorQuery['joins'] );
$this->addWhere( 'NOT(' . $actorQuery['conds'] . ')' );
$this->addWhere( 'actor_name<>' . $db->addQuotes( $params['excludeuser'] ) );
}
if ( $params['user'] !== null || $params['excludeuser'] !== null ) {
@ -407,7 +405,6 @@ class ApiQueryAllDeletedRevisions extends ApiQueryRevisionsBase {
'user' => [
ApiBase::PARAM_TYPE => 'user',
UserDef::PARAM_ALLOWED_USER_TYPES => [ 'name', 'ip', 'id', 'interwiki' ],
UserDef::PARAM_RETURN_OBJECT => true,
],
'namespace' => [
ApiBase::PARAM_ISMULTI => true,
@ -441,7 +438,6 @@ class ApiQueryAllDeletedRevisions extends ApiQueryRevisionsBase {
'excludeuser' => [
ApiBase::PARAM_TYPE => 'user',
UserDef::PARAM_ALLOWED_USER_TYPES => [ 'name', 'ip', 'id', 'interwiki' ],
UserDef::PARAM_RETURN_OBJECT => true,
ApiBase::PARAM_HELP_MSG_INFO => [ [ 'nonuseronly' ] ],
],
'tag' => null,

27
includes/api/ApiQueryDeletedRevisions.php
View file

@ -121,20 +121,17 @@ class ApiQueryDeletedRevisions extends ApiQueryRevisionsBase {
$this->addWhere( $where );
}
if ( $params['user'] !== null ) {
// Don't query by user ID here, it might be able to use the ar_usertext_timestamp index.
$actorQuery = ActorMigration::newMigration()
->getWhere( $db, 'ar_user', $params['user'], false );
$this->addTables( $actorQuery['tables'] );
$this->addJoinConds( $actorQuery['joins'] );
$this->addWhere( $actorQuery['conds'] );
} elseif ( $params['excludeuser'] !== null ) {
// Here there's no chance of using ar_usertext_timestamp.
$actorQuery = ActorMigration::newMigration()
->getWhere( $db, 'ar_user', $params['excludeuser'] );
$this->addTables( $actorQuery['tables'] );
$this->addJoinConds( $actorQuery['joins'] );
$this->addWhere( 'NOT(' . $actorQuery['conds'] . ')' );
if ( $params['user'] !== null || $params['excludeuser'] !== null ) {
// In the non-generator case, the actor join will already be present.
if ( $resultPageSet !== null ) {
$this->addTables( 'actor' );
$this->addJoinConds( [ 'actor' => [ 'JOIN', 'actor_id=ar_actor' ] ] );
}
if ( $params['user'] !== null ) {
$this->addWhereFld( 'actor_name', $params['user'] );
} elseif ( $params['excludeuser'] !== null ) {
$this->addWhere( 'actor_name<>' . $db->addQuotes( $params['excludeuser'] ) );
}
}
if ( $params['user'] !== null || $params['excludeuser'] !== null ) {
@ -281,12 +278,10 @@ class ApiQueryDeletedRevisions extends ApiQueryRevisionsBase {
'user' => [
ApiBase::PARAM_TYPE => 'user',
UserDef::PARAM_ALLOWED_USER_TYPES => [ 'name', 'ip', 'id', 'interwiki' ],
UserDef::PARAM_RETURN_OBJECT => true,
],
'excludeuser' => [
ApiBase::PARAM_TYPE => 'user',
UserDef::PARAM_ALLOWED_USER_TYPES => [ 'name', 'ip', 'id', 'interwiki' ],
UserDef::PARAM_RETURN_OBJECT => true,
],
'continue' => [
ApiBase::PARAM_HELP_MSG => 'api-help-param-continue',

17
includes/api/ApiQueryDeletedrevs.php
View file

@ -191,19 +191,10 @@ class ApiQueryDeletedrevs extends ApiQueryBase {
}
if ( $params['user'] !== null ) {
// Don't query by user ID here, it might be able to use the ar_usertext_timestamp index.
$actorQuery = ActorMigration::newMigration()
->getWhere( $db, 'ar_user', $params['user'], false );
$this->addTables( $actorQuery['tables'] );
$this->addJoinConds( $actorQuery['joins'] );
$this->addWhere( $actorQuery['conds'] );
// We already join on actor due to getArchiveQueryInfo()
$this->addWhereFld( 'actor_name', $params['user'] );
} elseif ( $params['excludeuser'] !== null ) {
// Here there's no chance of using ar_usertext_timestamp.
$actorQuery = ActorMigration::newMigration()
->getWhere( $db, 'ar_user', $params['excludeuser'] );
$this->addTables( $actorQuery['tables'] );
$this->addJoinConds( $actorQuery['joins'] );
$this->addWhere( 'NOT(' . $actorQuery['conds'] . ')' );
$this->addWhere( 'actor_name<>' . $db->addQuotes( $params['excludeuser'] ) );
}
if ( $params['user'] !== null || $params['excludeuser'] !== null ) {
@ -470,12 +461,10 @@ class ApiQueryDeletedrevs extends ApiQueryBase {
'user' => [
ApiBase::PARAM_TYPE => 'user',
UserDef::PARAM_ALLOWED_USER_TYPES => [ 'name', 'ip', 'id', 'interwiki' ],
UserDef::PARAM_RETURN_OBJECT => true,
],
'excludeuser' => [
ApiBase::PARAM_TYPE => 'user',
UserDef::PARAM_ALLOWED_USER_TYPES => [ 'name', 'ip', 'id', 'interwiki' ],
UserDef::PARAM_RETURN_OBJECT => true,
],
'prop' => [
ApiBase::PARAM_DFLT => 'user|comment',

6
includes/page/WikiPage.php
View file

@ -3153,7 +3153,6 @@ class WikiPage implements Page, IDBAccessObject, PageRecord {
$dbKey = $this->getTitle()->getDBkey();
$commentStore = CommentStore::getStore();
$actorMigration = ActorMigration::newMigration();
$revQuery = $this->getRevisionStore()->getQueryInfo();
$bitfield = false;
@ -3212,10 +3211,10 @@ class WikiPage implements Page, IDBAccessObject, PageRecord {
}
$comment = $commentStore->getComment( 'rev_comment', $row );
$user = User::newFromAnyId( $row->rev_user, $row->rev_user_text, $row->rev_actor );
$rowInsert = [
'ar_namespace' => $namespace,
'ar_title' => $dbKey,
'ar_actor' => $row->rev_actor,
'ar_timestamp' => $row->rev_timestamp,
'ar_minor_edit' => $row->rev_minor_edit,
'ar_rev_id' => $row->rev_id,
@ -3224,8 +3223,7 @@ class WikiPage implements Page, IDBAccessObject, PageRecord {
'ar_page_id' => $id,
'ar_deleted' => $suppress ? $bitfield : $row->rev_deleted,
'ar_sha1' => $row->rev_sha1,
] + $commentStore->insert( $dbw, 'ar_comment', $comment )
+ $actorMigration->getInsertValues( $dbw, 'ar_user', $user );
] + $commentStore->insert( $dbw, 'ar_comment', $comment );
$rowsInsert[] = $rowInsert;
$revids[] = $row->rev_id;

1
includes/specialpage/SpecialPageFactory.php
View file

@ -414,7 +414,6 @@ class SpecialPageFactory {
'PermissionManager',
'DBLoadBalancer',
'CommentStore',
'ActorMigration',
'RevisionFactory',
'NamespaceInfo',
'UserNameUtils',

7
includes/specials/SpecialDeletedContributions.php
View file

@ -46,9 +46,6 @@ class SpecialDeletedContributions extends SpecialPage {
/** @var CommentStore */
private $commentStore;
/** @var ActorMigration */
private $actorMigration;
/** @var RevisionFactory */
private $revisionFactory;
@ -65,7 +62,6 @@ class SpecialDeletedContributions extends SpecialPage {
* @param PermissionManager $permissionManager
* @param ILoadBalancer $loadBalancer
* @param CommentStore $commentStore
* @param ActorMigration $actorMigration
* @param RevisionFactory $revisionFactory
* @param NamespaceInfo $namespaceInfo
* @param UserNameUtils $userNameUtils
@ -75,7 +71,6 @@ class SpecialDeletedContributions extends SpecialPage {
PermissionManager $permissionManager,
ILoadBalancer $loadBalancer,
CommentStore $commentStore,
ActorMigration $actorMigration,
RevisionFactory $revisionFactory,
NamespaceInfo $namespaceInfo,
UserNameUtils $userNameUtils,
@ -85,7 +80,6 @@ class SpecialDeletedContributions extends SpecialPage {
$this->permissionManager = $permissionManager;
$this->loadBalancer = $loadBalancer;
$this->commentStore = $commentStore;
$this->actorMigration = $actorMigration;
$this->revisionFactory = $revisionFactory;
$this->namespaceInfo = $namespaceInfo;
$this->userNameUtils = $userNameUtils;
@ -160,7 +154,6 @@ class SpecialDeletedContributions extends SpecialPage {
$this->getHookContainer(),
$this->loadBalancer,
$this->commentStore,
$this->actorMigration,
$this->revisionFactory
);
if ( !$pager->getNumRows() ) {

35
includes/specials/pagers/DeletedContribsPager.php
View file

@ -64,9 +64,6 @@ class DeletedContribsPager extends IndexPager {
/** @var CommentStore */
private $commentStore;
/** @var ActorMigration */
private $actorMigration;
/** @var RevisionFactory */
private $revisionFactory;
@ -78,7 +75,6 @@ class DeletedContribsPager extends IndexPager {
* @param HookContainer $hookContainer
* @param ILoadBalancer $loadBalancer
* @param CommentStore $commentStore
* @param ActorMigration $actorMigration
* @param RevisionFactory $revisionFactory
*/
public function __construct(
@ -89,7 +85,6 @@ class DeletedContribsPager extends IndexPager {
HookContainer $hookContainer,
ILoadBalancer $loadBalancer,
CommentStore $commentStore,
ActorMigration $actorMigration,
RevisionFactory $revisionFactory
) {
// Set database before parent constructor to avoid setting it there with wfGetDB
@ -103,7 +98,6 @@ class DeletedContribsPager extends IndexPager {
$this->namespace = $namespace;
$this->hookRunner = new HookRunner( $hookContainer );
$this->commentStore = $commentStore;
$this->actorMigration = $actorMigration;
$this->revisionFactory = $revisionFactory;
}
@ -116,14 +110,8 @@ class DeletedContribsPager extends IndexPager {
public function getQueryInfo() {
$dbr = $this->getDatabase();
$userCond = [
// ->getJoin() below takes care of any joins needed
$this->actorMigration->getWhere(
$dbr, 'ar_user', User::newFromName( $this->target, false ), false
)['conds']
];
$userCond = [ 'actor_name' => $this->target ];
$conds = array_merge( $userCond, $this->getNamespaceCond() );
$user = $this->getUser();
// Paranoia: avoid brute force searches (T19792)
if ( !$this->getAuthority()->isAllowed( 'deletedhistory' ) ) {
$conds[] = $dbr->bitAnd( 'ar_deleted', RevisionRecord::DELETED_USER ) . ' = 0';
@ -133,17 +121,26 @@ class DeletedContribsPager extends IndexPager {
}
$commentQuery = $this->commentStore->getJoin( 'ar_comment' );
$actorQuery = $this->actorMigration->getJoin( 'ar_user' );
return [
'tables' => [ 'archive' ] + $commentQuery['tables'] + $actorQuery['tables'],
'tables' => [ 'archive', 'actor' ] + $commentQuery['tables'],
'fields' => [
'ar_rev_id', 'ar_id', 'ar_namespace', 'ar_title', 'ar_timestamp',
'ar_minor_edit', 'ar_deleted'
] + $commentQuery['fields'] + $actorQuery['fields'],
'ar_rev_id',
'ar_id',
'ar_namespace',
'ar_title',
'ar_actor',
'ar_timestamp',
'ar_minor_edit',
'ar_deleted',
'ar_user' => 'actor_user',
'ar_user_text' => 'actor_name',
] + $commentQuery['fields'],
'conds' => $conds,
'options' => [],
'join_conds' => $commentQuery['joins'] + $actorQuery['joins'],
'join_conds' => [
'actor' => [ 'JOIN', 'actor_id=ar_actor' ]
] + $commentQuery['joins'],
];
}

23
maintenance/deduplicateArchiveRevId.php
View file

@ -13,12 +13,6 @@ require_once __DIR__ . '/Maintenance.php';
*/
class DeduplicateArchiveRevId extends LoggedUpdateMaintenance {
/**
* @var array[]|null
* @phan-var array{tables:string[],fields:string[],joins:array}|null
*/
private $arActorQuery = null;
private $deleted = 0;
private $reassigned = 0;
@ -49,7 +43,6 @@ class DeduplicateArchiveRevId extends LoggedUpdateMaintenance {
$maxId = $dbw->selectField( 'archive', 'MAX(ar_rev_id)', [], __METHOD__ );
$batchSize = $this->getBatchSize();
$this->arActorQuery = ActorMigration::newMigration()->getJoin( 'ar_user' );
$revActorQuery = ActorMigration::newMigration()->getJoin( 'rev_user' );
for ( $id = $minId; $id <= $maxId; $id += $batchSize ) {
@ -121,13 +114,11 @@ class DeduplicateArchiveRevId extends LoggedUpdateMaintenance {
private function processArRevIds( IDatabase $dbw, array $arRevIds, array $revRows ) {
// Select all the data we need for deduplication
$res = $dbw->select(
[ 'archive' ] + $this->arActorQuery['tables'],
[ 'ar_id', 'ar_rev_id', 'ar_namespace', 'ar_title', 'ar_timestamp', 'ar_sha1' ]
+ $this->arActorQuery['fields'],
[ 'archive' ],
[ 'ar_id', 'ar_rev_id', 'ar_namespace', 'ar_title', 'ar_actor',
'ar_timestamp', 'ar_sha1' ],
[ 'ar_rev_id' => $arRevIds ],
__METHOD__,
[],
$this->arActorQuery['joins']
__METHOD__
);
// Determine which rows we need to delete or reassign
@ -150,8 +141,7 @@ class DeduplicateArchiveRevId extends LoggedUpdateMaintenance {
// of page, because moves can change IDs and titles.
if ( $row->ar_timestamp === $revRow->rev_timestamp &&
$row->ar_sha1 === $revRow->rev_sha1 &&
$row->ar_user === $revRow->rev_user &&
$row->ar_user_text === $revRow->rev_user_text
$row->ar_actor === $revRow->rev_actor
) {
$this->output(
"Row $row->ar_id duplicates revision row for rev_id $revRow->rev_id, deleting\n"
@ -207,8 +197,7 @@ class DeduplicateArchiveRevId extends LoggedUpdateMaintenance {
$row->ar_title,
$row->ar_timestamp,
$row->ar_sha1,
$row->ar_user,
$row->ar_user_text,
$row->ar_actor,
] );
}

22
tests/phpunit/includes/Revision/RevisionQueryInfoTest.php
View file

@ -61,11 +61,19 @@ class RevisionQueryInfoTest extends MediaWikiIntegrationTestCase {
}
protected function getNewActorQueryFields( $prefix, $tmp = false ) {
return [
"{$prefix}_user" => "actor_{$prefix}_user.actor_user",
"{$prefix}_user_text" => "actor_{$prefix}_user.actor_name",
"{$prefix}_actor" => $tmp ? "temp_{$prefix}_user.{$prefix}actor_actor" : "{$prefix}_actor",
];
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 {
return [
"{$prefix}_actor",
"{$prefix}_user" => 'archive_actor.actor_user',
"{$prefix}_user_text" => 'archive_actor.actor_name',
];
}
}
protected function getTextQueryFields() {
@ -105,7 +113,7 @@ class RevisionQueryInfoTest extends MediaWikiIntegrationTestCase {
[
'tables' => [
'archive',
'actor_ar_user' => 'actor',
'archive_actor' => 'actor',
'comment_ar_comment' => 'comment',
],
'fields' => array_merge(
@ -116,7 +124,7 @@ class RevisionQueryInfoTest extends MediaWikiIntegrationTestCase {
'joins' => [
'comment_ar_comment'
=> [ 'JOIN', 'comment_ar_comment.comment_id = ar_comment_id' ],
'actor_ar_user' => [ 'JOIN', 'actor_ar_user.actor_id = ar_actor' ],
'archive_actor' => [ 'JOIN', 'actor_id=ar_actor' ],
],
]
];