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

diff --git a/includes/pager/IndexPager.php b/includes/pager/IndexPager.php
index 05af4fd..e9cadf3 100644 (file)
--- 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 18da235..90a57ff 100644 (file)
--- 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 1147805..dc02922 100644 (file)
--- a/tests/phpunit/includes/specials/ContribsPagerTest.php
+++ b/tests/phpunit/includes/specials/ContribsPagerTest.php
@@ -1,5 +1,7 @@
 <?php
 
+use Wikimedia\TestingAccessWrapper;
+
 /**
  * @group Database
  */
@@ -23,7 +25,7 @@ class ContribsPagerTest extends MediaWikiTestCase {
         * @param array $inputOpts Input options
         * @param array $expectedOpts Expected options
         */
-       public function testDateFilterOptionProcessing( $inputOpts, $expectedOpts ) {
+       public function testDateFilterOptionProcessing( array $inputOpts, array $expectedOpts ) {
                $this->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'] );
+       }
+
 }