RC Filters: Detect filters conflicts to by-pass db query

Filters are in conflict when their combination is guaranteed
to return no results. For instance: minor and log entries
is a conflict because major/minor does not apply to
log entries and the field is set to major by default.

Letting conflicts go through result in some very slow
database queries.

Bug: T160220
Change-Id: Ia6b0125c675c4a3cc4e4be4f83d1bd10d23059ba

diff --git a/includes/changes/ChangesListBooleanFilter.php b/includes/changes/ChangesListBooleanFilter.php
index 851d173..1c116ab 100644 (file)
--- a/includes/changes/ChangesListBooleanFilter.php
+++ b/includes/changes/ChangesListBooleanFilter.php
@@ -223,4 +223,13 @@ class ChangesListBooleanFilter extends ChangesListFilter {
                return $output;
        }
 
+       /**
+        * @inheritdoc
+        */
+       public function isSelected( FormOptions $opts ) {
+               return !$opts[ $this->getName() ] &&
+                       array_filter( $this->getSiblings(), function ( $sibling ) use ( $opts ) {
+                               return $opts[ $sibling->getName() ];
+                       } );
+       }
 }

diff --git a/includes/changes/ChangesListFilter.php b/includes/changes/ChangesListFilter.php
index b3a16a8..9af9adc 100644 (file)
--- a/includes/changes/ChangesListFilter.php
+++ b/includes/changes/ChangesListFilter.php
@@ -227,6 +227,7 @@ abstract class ChangesListFilter {
                if ( $other instanceof ChangesListFilterGroup ) {
                        $this->conflictingGroups[] = [
                                'group' => $other->getName(),
+                               'groupObject' => $other,
                                'globalDescription' => $globalDescription,
                                'contextDescription' => $contextDescription,
                        ];
@@ -234,6 +235,7 @@ abstract class ChangesListFilter {
                        $this->conflictingFilters[] = [
                                'group' => $other->getGroup()->getName(),
                                'filter' => $other->getName(),
+                               'filterObject' => $other,
                                'globalDescription' => $globalDescription,
                                'contextDescription' => $contextDescription,
                        ];
@@ -385,6 +387,8 @@ abstract class ChangesListFilter {
                );
 
                foreach ( $conflicts as $conflictInfo ) {
+                       unset( $conflictInfo['filterObject'] );
+                       unset( $conflictInfo['groupObject'] );
                        $output['conflicts'][] = $conflictInfo;
                        array_push(
                                $output['messageKeys'],
@@ -395,4 +399,105 @@ abstract class ChangesListFilter {
 
                return $output;
        }
+
+       /**
+        * Checks whether this filter is selected in the provided options
+        *
+        * @param FormOptions $opts
+        * @return bool
+        */
+       abstract public function isSelected( FormOptions $opts );
+
+       /**
+        * Get groups conflicting with this filter
+        *
+        * @return ChangesListFilterGroup[]
+        */
+       public function getConflictingGroups() {
+               return array_map(
+                       function ( $conflictDesc ) {
+                               return $conflictDesc[ 'groupObject' ];
+                       },
+                       $this->conflictingGroups
+               );
+       }
+
+       /**
+        * Get filters conflicting with this filter
+        *
+        * @return ChangesListFilter[]
+        */
+       public function getConflictingFilters() {
+               return array_map(
+                       function ( $conflictDesc ) {
+                               return $conflictDesc[ 'filterObject' ];
+                       },
+                       $this->conflictingFilters
+               );
+       }
+
+       /**
+        * Check if the conflict with a group is currently "active"
+        *
+        * @param ChangesListFilterGroup $group
+        * @param FormOptions $opts
+        * @return bool
+        */
+       public function activelyInConflictWithGroup( ChangesListFilterGroup $group, FormOptions $opts ) {
+               if ( $group->anySelected( $opts ) && $this->isSelected( $opts ) ) {
+                       /** @var ChangesListFilter $siblingFilter */
+                       foreach ( $this->getSiblings() as $siblingFilter ) {
+                               if ( $siblingFilter->isSelected( $opts ) && !$siblingFilter->hasConflictWithGroup( $group ) ) {
+                                       return false;
+                               }
+                       }
+                       return true;
+               }
+               return false;
+       }
+
+       private function hasConflictWithGroup( ChangesListFilterGroup $group ) {
+               return in_array( $group, $this->getConflictingGroups() );
+       }
+
+       /**
+        * Check if the conflict with a filter is currently "active"
+        *
+        * @param ChangesListFilter $filter
+        * @param FormOptions $opts
+        * @return bool
+        */
+       public function activelyInConflictWithFilter( ChangeslistFilter $filter, FormOptions $opts ) {
+               if ( $this->isSelected( $opts ) && $filter->isSelected( $opts ) ) {
+                       /** @var ChangesListFilter $siblingFilter */
+                       foreach ( $this->getSiblings() as $siblingFilter ) {
+                               if (
+                                       $siblingFilter->isSelected( $opts ) &&
+                                       !$siblingFilter->hasConflictWithFilter( $filter )
+                               ) {
+                                       return false;
+                               }
+                       }
+                       return true;
+               }
+               return false;
+       }
+
+       private function hasConflictWithFilter( ChangeslistFilter $filter ) {
+               return in_array( $filter, $this->getConflictingFilters() );
+       }
+
+       /**
+        * Get filters in the same group
+        *
+        * @return ChangesListFilter[]
+        */
+       protected function getSiblings() {
+               return array_filter(
+                       $this->getGroup()->getFilters(),
+                       function ( $filter ) {
+                               return $filter !== $this;
+                       }
+               );
+       }
 }

diff --git a/includes/changes/ChangesListFilterGroup.php b/includes/changes/ChangesListFilterGroup.php
index 4ff5520..0cdc24a 100644 (file)
--- a/includes/changes/ChangesListFilterGroup.php
+++ b/includes/changes/ChangesListFilterGroup.php
@@ -261,6 +261,7 @@ abstract class ChangesListFilterGroup {
                if ( $other instanceof ChangesListFilterGroup ) {
                        $this->conflictingGroups[] = [
                                'group' => $other->getName(),
+                               'groupObject' => $other,
                                'globalDescription' => $globalDescription,
                                'contextDescription' => $contextDescription,
                        ];
@@ -268,6 +269,7 @@ abstract class ChangesListFilterGroup {
                        $this->conflictingFilters[] = [
                                'group' => $other->getGroup()->getName(),
                                'filter' => $other->getName(),
+                               'filterObject' => $other,
                                'globalDescription' => $globalDescription,
                                'contextDescription' => $contextDescription,
                        ];
@@ -390,6 +392,8 @@ abstract class ChangesListFilterGroup {
 
                foreach ( $conflicts as $conflictInfo ) {
                        $output['conflicts'][] = $conflictInfo;
+                       unset( $conflictInfo['filterObject'] );
+                       unset( $conflictInfo['groupObject'] );
                        array_push(
                                $output['messageKeys'],
                                $conflictInfo['globalDescription'],
@@ -399,4 +403,47 @@ abstract class ChangesListFilterGroup {
 
                return $output;
        }
+
+       /**
+        * Get groups conflicting with this filter group
+        *
+        * @return ChangesListFilterGroup[]
+        */
+       public function getConflictingGroups() {
+               return array_map(
+                       function ( $conflictDesc ) {
+                               return $conflictDesc[ 'groupObject' ];
+                       },
+                       $this->conflictingGroups
+               );
+       }
+
+       /**
+        * Get filters conflicting with this filter group
+        *
+        * @return ChangesListFilter[]
+        */
+       public function getConflictingFilters() {
+               return array_map(
+                       function ( $conflictDesc ) {
+                               return $conflictDesc[ 'filterObject' ];
+                       },
+                       $this->conflictingFilters
+               );
+       }
+
+       /**
+        * Check if any filter in this group is selected
+        *
+        * @param FormOptions $opts
+        * @return bool
+        */
+       public function anySelected( FormOptions $opts ) {
+               return !!count( array_filter(
+                       $this->getFilters(),
+                       function ( ChangesListFilter $filter ) use ( $opts ) {
+                               return $filter->isSelected( $opts );
+                       }
+               ) );
+       }
 }

diff --git a/includes/changes/ChangesListStringOptionsFilter.php b/includes/changes/ChangesListStringOptionsFilter.php
index 1c977b9..6754d67 100644 (file)
--- a/includes/changes/ChangesListStringOptionsFilter.php
+++ b/includes/changes/ChangesListStringOptionsFilter.php
@@ -14,4 +14,15 @@ class ChangesListStringOptionsFilter extends ChangesListFilter {
        public function displaysOnUnstructuredUi() {
                return false;
        }
+
+       /**
+        * @inheritdoc
+        */
+       public function isSelected( FormOptions $opts ) {
+               $values = explode(
+                       ChangesListStringOptionsFilterGroup::SEPARATOR,
+                       $opts[ $this->getGroup()->getName() ]
+               );
+               return in_array( $this->getName(), $values );
+       }
 }

diff --git a/includes/specialpage/ChangesListSpecialPage.php b/includes/specialpage/ChangesListSpecialPage.php
index 8e79703..3aafc94 100644 (file)
--- a/includes/specialpage/ChangesListSpecialPage.php
+++ b/includes/specialpage/ChangesListSpecialPage.php
@@ -422,6 +422,50 @@ abstract class ChangesListSpecialPage extends SpecialPage {
                ];
        }
 
+       /**
+        * Check if filters are in conflict and guaranteed to return no results.
+        *
+        * @return bool
+        */
+       protected function areFiltersInConflict() {
+               $opts = $this->getOptions();
+               /** @var ChangesListFilterGroup $group */
+               foreach ( $this->getFilterGroups() as $group ) {
+
+                       if ( $group->getConflictingGroups() ) {
+                               wfLogWarning(
+                                       $group->getName() .
+                                       " specifies conflicts with other groups but these are not supported yet."
+                               );
+                       }
+
+                       /** @var ChangesListFilter $conflictingFilter */
+                       foreach ( $group->getConflictingFilters() as $conflictingFilter ) {
+                               if ( $conflictingFilter->activelyInConflictWithGroup( $group, $opts ) ) {
+                                       return true;
+                               }
+                       }
+
+                       /** @var ChangesListFilter $filter */
+                       foreach ( $group->getFilters() as $filter ) {
+
+                               /** @var ChangesListFilter $conflictingFilter */
+                               foreach ( $filter->getConflictingFilters() as $conflictingFilter ) {
+                                       if (
+                                               $conflictingFilter->activelyInConflictWithFilter( $filter, $opts ) &&
+                                               $filter->activelyInConflictWithFilter( $conflictingFilter, $opts )
+                                       ) {
+                                               return true;
+                                       }
+                               }
+
+                       }
+
+               }
+
+               return false;
+       }
+
        /**
         * Main execution point
         *

diff --git a/includes/specials/SpecialRecentchanges.php b/includes/specials/SpecialRecentchanges.php
index f88f09c..6c11032 100644 (file)
--- a/includes/specials/SpecialRecentchanges.php
+++ b/includes/specials/SpecialRecentchanges.php
@@ -268,6 +268,10 @@ class SpecialRecentChanges extends ChangesListSpecialPage {
                        return false;
                }
 
+               if ( $this->areFiltersInConflict() ) {
+                       return false;
+               }
+
                // array_merge() is used intentionally here so that hooks can, should
                // they so desire, override the ORDER BY / LIMIT condition(s); prior to
                // MediaWiki 1.26 this used to use the plus operator instead, which meant

diff --git a/tests/phpunit/includes/specialpage/ChangesListSpecialPageTest.php b/tests/phpunit/includes/specialpage/ChangesListSpecialPageTest.php
index e10a97f..1ddb98d 100644 (file)
--- a/tests/phpunit/includes/specialpage/ChangesListSpecialPageTest.php
+++ b/tests/phpunit/includes/specialpage/ChangesListSpecialPageTest.php
@@ -801,4 +801,85 @@ class ChangesListSpecialPageTest extends AbstractChangesListSpecialPageTestCase
                        ],
                ];
        }
+
+       public function provideGetFilterConflicts() {
+               return [
+                       [
+                               "parameters" => [],
+                               "expectedConflicts" => false,
+                       ],
+                       [
+                               "parameters" => [
+                                       "hideliu" => true,
+                                       "userExpLevel" => "newcomer",
+                               ],
+                               "expectedConflicts" => true,
+                       ],
+                       [
+                               "parameters" => [
+                                       "hideanons" => true,
+                                       "userExpLevel" => "learner",
+                               ],
+                               "expectedConflicts" => false,
+                       ],
+                       [
+                               "parameters" => [
+                                       "hidemajor" => true,
+                                       "hidenewpages" => true,
+                                       "hidepageedits" => true,
+                                       "hidecategorization" => false,
+                                       "hidelog" => true,
+                                       "hideWikidata" => true,
+                               ],
+                               "expectedConflicts" => true,
+                       ],
+                       [
+                               "parameters" => [
+                                       "hidemajor" => true,
+                                       "hidenewpages" => false,
+                                       "hidepageedits" => true,
+                                       "hidecategorization" => false,
+                                       "hidelog" => false,
+                                       "hideWikidata" => true,
+                               ],
+                               "expectedConflicts" => true,
+                       ],
+                       [
+                               "parameters" => [
+                                       "hidemajor" => true,
+                                       "hidenewpages" => false,
+                                       "hidepageedits" => false,
+                                       "hidecategorization" => true,
+                                       "hidelog" => true,
+                                       "hideWikidata" => true,
+                               ],
+                               "expectedConflicts" => false,
+                       ],
+                       [
+                               "parameters" => [
+                                       "hideminor" => true,
+                                       "hidenewpages" => true,
+                                       "hidepageedits" => true,
+                                       "hidecategorization" => false,
+                                       "hidelog" => true,
+                                       "hideWikidata" => true,
+                               ],
+                               "expectedConflicts" => false,
+                       ],
+               ];
+       }
+
+       /**
+        * @dataProvider provideGetFilterConflicts
+        */
+       public function testGetFilterConflicts( $parameters, $expectedConflicts ) {
+               $context = new RequestContext;
+               $context->setRequest( new FauxRequest( $parameters ) );
+               $this->changesListSpecialPage->setContext( $context );
+
+               $this->assertEquals(
+                       $expectedConflicts,
+                       $this->changesListSpecialPage->areFiltersInConflict()
+               );
+       }
 }

diff --git a/tests/phpunit/mocks/MockChangesListFilter.php b/tests/phpunit/mocks/MockChangesListFilter.php
index aeb9f0f..79232ad 100644 (file)
--- a/tests/phpunit/mocks/MockChangesListFilter.php
+++ b/tests/phpunit/mocks/MockChangesListFilter.php
@@ -8,4 +8,8 @@ class MockChangesListFilter extends ChangesListFilter {
                        'instead of testing the abstract class'
                );
        }
+
+       public function isSelected( FormOptions $opts ) {
+               return false;
+       }
 }