From a88be858eeaa01c0797f3a29b5c85d0c573c15f0 Mon Sep 17 00:00:00 2001 From: Thiemo Kreuz Date: Mon, 17 Dec 2018 15:03:44 +0100 Subject: [PATCH] Fix order on Special:Contributions when timestamps are identical Please see T200259#4827781 for a longer explanation why this is needed. Bug: T200259 Change-Id: I1678f7ec994328a96eed208483421be2f82be0ce --- includes/pager/IndexPager.php | 4 +- includes/specials/pagers/ContribsPager.php | 40 ++++++++++----- .../includes/specials/ContribsPagerTest.php | 50 ++++++++++++++++++- 3 files changed, 78 insertions(+), 16 deletions(-) diff --git a/includes/pager/IndexPager.php b/includes/pager/IndexPager.php index 05af4fdb84..e9cadf34bc 100644 --- a/includes/pager/IndexPager.php +++ b/includes/pager/IndexPager.php @@ -694,7 +694,7 @@ abstract class IndexPager extends ContextSource implements Pager { * Needless to say, it's really not a good idea to use a non-unique index * for this! That won't page right. * - * @return string|array + * @return string|string[] */ abstract function getIndexField(); @@ -712,7 +712,7 @@ abstract class IndexPager extends ContextSource implements Pager { * page_len,page_id avoids temp tables (given a page_len index). This would * also work if page_id was non-unique but we had a page_len,page_id index. * - * @return array + * @return string[]|array[] */ protected function getExtraSortFields() { return []; diff --git a/includes/specials/pagers/ContribsPager.php b/includes/specials/pagers/ContribsPager.php index 18da235fde..90a57ff655 100644 --- a/includes/specials/pagers/ContribsPager.php +++ b/includes/specials/pagers/ContribsPager.php @@ -87,10 +87,6 @@ class ContribsPager extends RangeChronologicalPager { } $this->getDateRangeCond( $startTimestamp, $endTimestamp ); - // This property on IndexPager is set by $this->getIndexField() in parent::__construct(). - // We need to reassign it here so that it is used when the actual query is ran. - $this->mIndexField = $this->getIndexField(); - // Most of this code will use the 'contributions' group DB, which can map to replica DBs // with extra user based indexes or partioning by user. The additional metadata // queries should use a regular replica DB since the lookup pattern is not all by user. @@ -212,6 +208,22 @@ class ContribsPager extends RangeChronologicalPager { $ipRangeConds = $user->isAnon() ? $this->getIpRangeConds( $this->mDb, $this->target ) : null; if ( $ipRangeConds ) { $queryInfo['tables'][] = 'ip_changes'; + /** + * These aliases make `ORDER BY rev_timestamp, rev_id` from {@see getIndexField} and + * {@see getExtraSortFields} use the replicated `ipc_rev_timestamp` and `ipc_rev_id` + * columns from the `ip_changes` table, for more efficient queries. + * @see https://phabricator.wikimedia.org/T200259#4832318 + */ + $queryInfo['fields'] = array_merge( + [ + 'rev_timestamp' => 'ipc_rev_timestamp', + 'rev_id' => 'ipc_rev_id', + ], + array_diff( $queryInfo['fields'], [ + 'rev_timestamp', + 'rev_id', + ] ) + ); $queryInfo['join_conds']['ip_changes'] = [ 'LEFT JOIN', [ 'ipc_rev_id = rev_id' ] ]; @@ -350,17 +362,19 @@ class ContribsPager extends RangeChronologicalPager { } /** - * Override of getIndexField() in IndexPager. - * For IP ranges, it's faster to use the replicated ipc_rev_timestamp - * on the `ip_changes` table than the rev_timestamp on the `revision` table. - * @return string Name of field + * @return string */ public function getIndexField() { - if ( $this->isQueryableRange( $this->target ) ) { - return 'ipc_rev_timestamp'; - } else { - return 'rev_timestamp'; - } + // Note this is run via parent::__construct() *before* $this->target is set! + return 'rev_timestamp'; + } + + /** + * @return string[] + */ + protected function getExtraSortFields() { + // Note this is run via parent::__construct() *before* $this->target is set! + return [ 'rev_id' ]; } function doBatchLookups() { diff --git a/tests/phpunit/includes/specials/ContribsPagerTest.php b/tests/phpunit/includes/specials/ContribsPagerTest.php index 1147805c01..dc02922408 100644 --- a/tests/phpunit/includes/specials/ContribsPagerTest.php +++ b/tests/phpunit/includes/specials/ContribsPagerTest.php @@ -1,5 +1,7 @@ assertArraySubset( $expectedOpts, ContribsPager::processDateFilter( $inputOpts ) ); } @@ -115,4 +117,50 @@ class ContribsPagerTest extends MediaWikiTestCase { [ '2001:db8::/9999' ], ]; } + + /** + * @covers \ContribsPager::getExtraSortFields + * @covers \ContribsPager::getIndexField + * @covers \ContribsPager::getQueryInfo + */ + public function testUniqueSortOrderWithoutIpChanges() { + $pager = new ContribsPager( new RequestContext(), [ + 'start' => '', + 'end' => '', + ] ); + + /** @var ContribsPager $pager */ + $pager = TestingAccessWrapper::newFromObject( $pager ); + $queryInfo = $pager->buildQueryInfo( '', 1, false ); + + $this->assertNotContains( 'ip_changes', $queryInfo[0] ); + $this->assertArrayNotHasKey( 'ip_changes', $queryInfo[5] ); + $this->assertContains( 'rev_timestamp', $queryInfo[1] ); + $this->assertContains( 'rev_id', $queryInfo[1] ); + $this->assertSame( [ 'rev_timestamp DESC', 'rev_id DESC' ], $queryInfo[4]['ORDER BY'] ); + } + + /** + * @covers \ContribsPager::getExtraSortFields + * @covers \ContribsPager::getIndexField + * @covers \ContribsPager::getQueryInfo + */ + public function testUniqueSortOrderOnIpChanges() { + $pager = new ContribsPager( new RequestContext(), [ + 'target' => '116.17.184.5/32', + 'start' => '', + 'end' => '', + ] ); + + /** @var ContribsPager $pager */ + $pager = TestingAccessWrapper::newFromObject( $pager ); + $queryInfo = $pager->buildQueryInfo( '', 1, false ); + + $this->assertContains( 'ip_changes', $queryInfo[0] ); + $this->assertArrayHasKey( 'ip_changes', $queryInfo[5] ); + $this->assertSame( 'ipc_rev_timestamp', $queryInfo[1]['rev_timestamp'] ); + $this->assertSame( 'ipc_rev_id', $queryInfo[1]['rev_id'] ); + $this->assertSame( [ 'rev_timestamp DESC', 'rev_id DESC' ], $queryInfo[4]['ORDER BY'] ); + } + } -- 2.20.1