From 93cbb2450c54935fe63df7946b076faeaf399c6b Mon Sep 17 00:00:00 2001 From: Stephane Bisson Date: Mon, 10 Apr 2017 13:23:45 -0400 Subject: [PATCH] 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 --- includes/changes/ChangesListBooleanFilter.php | 9 ++ includes/changes/ChangesListFilter.php | 105 ++++++++++++++++++ includes/changes/ChangesListFilterGroup.php | 47 ++++++++ .../ChangesListStringOptionsFilter.php | 11 ++ .../specialpage/ChangesListSpecialPage.php | 44 ++++++++ includes/specials/SpecialRecentchanges.php | 4 + .../ChangesListSpecialPageTest.php | 81 ++++++++++++++ tests/phpunit/mocks/MockChangesListFilter.php | 4 + 8 files changed, 305 insertions(+) diff --git a/includes/changes/ChangesListBooleanFilter.php b/includes/changes/ChangesListBooleanFilter.php index 851d173b60..1c116ab50a 100644 --- 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 b3a16a81b3..9af9adc010 100644 --- 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 4ff55201cc..0cdc24a41f 100644 --- 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 1c977b9d4e..6754d679f9 100644 --- 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 8e797035be..3aafc948e5 100644 --- 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 f88f09c60e..6c1103244a 100644 --- 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 e10a97f222..1ddb98d07d 100644 --- 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 aeb9f0f22c..79232ad148 100644 --- 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; + } } -- 2.20.1