From 2a43939ffb0cfb0808c2b56430be4f5e88d863ee Mon Sep 17 00:00:00 2001 From: Erik Bernhardson Date: Tue, 19 Dec 2017 14:19:49 -0800 Subject: [PATCH] Push pagination decision for search into SearchEngine Various code using the search engine shouldn't need to implement it's own methods, such as over-fetching, to determine if there are more results available. This should be knowledge internal to search that is exposed by a boolean. Change-Id: Ica094428700637dfdedb723b03f6aeadfe12b9f4 --- autoload.php | 1 + includes/api/ApiQueryPrefixSearch.php | 18 +++-- includes/api/ApiQuerySearch.php | 12 ++-- includes/api/SearchApi.php | 15 +--- includes/search/PaginatingSearchEngine.php | 12 ++++ includes/search/SearchEngine.php | 70 +++++++++++++++++-- includes/search/SearchResultSet.php | 55 +++++++++++++-- includes/search/SearchSuggestionSet.php | 27 +++++-- tests/common/TestsAutoLoader.php | 1 + .../includes/api/ApiQueryPrefixSearchTest.php | 48 +++++++++++++ .../search/SearchEnginePrefixTest.php | 65 ++++++++++++++--- .../includes/search/SearchResultSetTest.php | 16 +++++ .../search/MockCompletionSearchEngine.php | 27 +++++++ .../mocks/search/MockSearchResultSet.php | 1 + 14 files changed, 313 insertions(+), 55 deletions(-) create mode 100644 includes/search/PaginatingSearchEngine.php create mode 100644 tests/phpunit/includes/api/ApiQueryPrefixSearchTest.php create mode 100644 tests/phpunit/mocks/search/MockCompletionSearchEngine.php diff --git a/autoload.php b/autoload.php index 1adc5e4ea6..ac1ac18189 100644 --- a/autoload.php +++ b/autoload.php @@ -1070,6 +1070,7 @@ $wgAutoloadLocalClasses = [ 'PageProps' => __DIR__ . '/includes/PageProps.php', 'PageQueryPage' => __DIR__ . '/includes/specialpage/PageQueryPage.php', 'Pager' => __DIR__ . '/includes/pager/Pager.php', + 'PaginatingSearchEngine' => __DIR__ . '/includes/search/PaginatingSearchEngine.php', 'ParameterizedPassword' => __DIR__ . '/includes/password/ParameterizedPassword.php', 'Parser' => __DIR__ . '/includes/parser/Parser.php', 'ParserCache' => __DIR__ . '/includes/parser/ParserCache.php', diff --git a/includes/api/ApiQueryPrefixSearch.php b/includes/api/ApiQueryPrefixSearch.php index 2fbc518b1e..04d3f2c9fc 100644 --- a/includes/api/ApiQueryPrefixSearch.php +++ b/includes/api/ApiQueryPrefixSearch.php @@ -51,7 +51,12 @@ class ApiQueryPrefixSearch extends ApiQueryGeneratorBase { $offset = $params['offset']; $searchEngine = $this->buildSearchEngine( $params ); - $titles = $searchEngine->extractTitles( $searchEngine->completionSearchWithVariants( $search ) ); + $suggestions = $searchEngine->completionSearchWithVariants( $search ); + $titles = $searchEngine->extractTitles( $suggestions ); + + if ( $suggestions->hasMoreResults() ) { + $this->setContinueEnumParameter( 'offset', $offset + $limit ); + } if ( $resultPageSet ) { $resultPageSet->setRedirectMergePolicy( function ( array $current, array $new ) { @@ -60,10 +65,6 @@ class ApiQueryPrefixSearch extends ApiQueryGeneratorBase { } return $current; } ); - if ( count( $titles ) > $limit ) { - $this->setContinueEnumParameter( 'offset', $offset + $limit ); - array_pop( $titles ); - } $resultPageSet->populateFromTitles( $titles ); foreach ( $titles as $index => $title ) { $resultPageSet->setGeneratorData( $title, [ 'index' => $index + $offset + 1 ] ); @@ -72,10 +73,6 @@ class ApiQueryPrefixSearch extends ApiQueryGeneratorBase { $result = $this->getResult(); $count = 0; foreach ( $titles as $title ) { - if ( ++$count > $limit ) { - $this->setContinueEnumParameter( 'offset', $offset + $limit ); - break; - } $vals = [ 'ns' => intval( $title->getNamespace() ), 'title' => $title->getPrefixedText(), @@ -86,8 +83,9 @@ class ApiQueryPrefixSearch extends ApiQueryGeneratorBase { $vals['pageid'] = intval( $title->getArticleID() ); } $fit = $result->addValue( [ 'query', $this->getModuleName() ], null, $vals ); + ++$count; if ( !$fit ) { - $this->setContinueEnumParameter( 'offset', $offset + $count - 1 ); + $this->setContinueEnumParameter( 'offset', $offset + $count ); break; } } diff --git a/includes/api/ApiQuerySearch.php b/includes/api/ApiQuerySearch.php index 87913e682f..708c944eb9 100644 --- a/includes/api/ApiQuerySearch.php +++ b/includes/api/ApiQuerySearch.php @@ -144,14 +144,12 @@ class ApiQuerySearch extends ApiQueryGeneratorBase { $count = 0; $limit = $params['limit']; - foreach ( $matches as $result ) { - if ( ++$count > $limit ) { - // We've reached the one extra which shows that there are - // additional items to be had. Stop here... - $this->setContinueEnumParameter( 'offset', $params['offset'] + $params['limit'] ); - break; - } + if ( $matches->hasMoreResults() ) { + $this->setContinueEnumParameter( 'offset', $params['offset'] + $params['limit'] ); + } + foreach ( $matches as $result ) { + $count++; // Silently skip broken and missing titles if ( $result->isBrokenTitle() || $result->isMissingRevision() ) { continue; diff --git a/includes/api/SearchApi.php b/includes/api/SearchApi.php index 40d477880f..fc6eddf800 100644 --- a/includes/api/SearchApi.php +++ b/includes/api/SearchApi.php @@ -76,7 +76,7 @@ trait SearchApi { if ( $alternatives[0] === null ) { $alternatives[0] = self::$BACKEND_NULL_PARAM; } - $this->allowedParams['backend'] = [ + $params['backend'] = [ ApiBase::PARAM_DFLT => $searchConfig->getSearchType(), ApiBase::PARAM_TYPE => $alternatives, ]; @@ -140,8 +140,7 @@ trait SearchApi { * will be set: * - backend: which search backend to use * - limit: mandatory - * - offset: optional, if set limit will be incremented by - * one ( to support the continue parameter ) + * - offset: optional * - namespace: mandatory * - search engine profiles defined by SearchApi::getSearchProfileParams() * @param string[]|null $params API request params (must be sanitized by @@ -157,15 +156,7 @@ trait SearchApi { $searchEngine = MediaWikiServices::getInstance()->getSearchEngineFactory()->create( $type ); $limit = $params['limit']; $searchEngine->setNamespaces( $params['namespace'] ); - $offset = null; - if ( isset( $params['offset'] ) ) { - // If the API supports offset then it probably - // wants to fetch limit+1 so it can check if - // more results are available to properly set - // the continue param - $offset = $params['offset']; - $limit += 1; - } + $offset = isset( $params['offset'] ) ? $params['offset'] : null; $searchEngine->setLimitOffset( $limit, $offset ); // Initialize requested search profiles. diff --git a/includes/search/PaginatingSearchEngine.php b/includes/search/PaginatingSearchEngine.php new file mode 100644 index 0000000000..97ef2d57c5 --- /dev/null +++ b/includes/search/PaginatingSearchEngine.php @@ -0,0 +1,12 @@ +doSearchText( $term ); + return $this->maybePaginate( function () use ( $term ) { + return $this->doSearchText( $term ); + } ); } /** @@ -132,7 +134,9 @@ abstract class SearchEngine { * @return SearchResultSet|null */ public function searchTitle( $term ) { - return $this->doSearchTitle( $term ); + return $this->maybePaginate( function () use ( $term ) { + return $this->doSearchTitle( $term ); + } ); } /** @@ -146,6 +150,40 @@ abstract class SearchEngine { return null; } + /** + * Performs an overfetch and shrink operation to determine if + * the next page is available for search engines that do not + * explicitly implement their own pagination. + * + * @param Closure $fn Takes no arguments + * @return SearchResultSet|Status|null Result of calling $fn + */ + private function maybePaginate( Closure $fn ) { + if ( $this instanceof PaginatingSearchEngine ) { + return $fn(); + } + $this->limit++; + try { + $resultSetOrStatus = $fn(); + } finally { + $this->limit--; + } + + $resultSet = null; + if ( $resultSetOrStatus instanceof SearchResultSet ) { + $resultSet = $resultSetOrStatus; + } elseif ( $resultSetOrStatus instanceof Status && + $resultSetOrStatus->getValue() instanceof SearchResultSet + ) { + $resultSet = $resultSetOrStatus->getValue(); + } + if ( $resultSet ) { + $resultSet->shrink( $this->limit ); + } + + return $resultSetOrStatus; + } + /** * @since 1.18 * @param string $feature @@ -523,6 +561,22 @@ abstract class SearchEngine { return $search; } + /** + * Perform an overfetch of completion search results. This allows + * determining if another page of results is available. + * + * @param string $search + * @return SearchSuggestionSet + */ + protected function completionSearchBackendOverfetch( $search ) { + $this->limit++; + try { + return $this->completionSearchBackend( $search ); + } finally { + $this->limit--; + } + } + /** * Perform a completion search. * Does not resolve namespaces and does not check variants. @@ -560,7 +614,8 @@ abstract class SearchEngine { return SearchSuggestionSet::emptySuggestionSet(); // Return empty result } $search = $this->normalizeNamespaces( $search ); - return $this->processCompletionResults( $search, $this->completionSearchBackend( $search ) ); + $suggestions = $this->completionSearchBackendOverfetch( $search ); + return $this->processCompletionResults( $search, $suggestions ); } /** @@ -574,8 +629,8 @@ abstract class SearchEngine { } $search = $this->normalizeNamespaces( $search ); - $results = $this->completionSearchBackend( $search ); - $fallbackLimit = $this->limit - $results->getSize(); + $results = $this->completionSearchBackendOverfetch( $search ); + $fallbackLimit = 1 + $this->limit - $results->getSize(); if ( $fallbackLimit > 0 ) { global $wgContLang; @@ -614,6 +669,10 @@ abstract class SearchEngine { * @return SearchSuggestionSet */ protected function processCompletionResults( $search, SearchSuggestionSet $suggestions ) { + // We over-fetched to determine pagination. Shrink back down if we have extra results + // and mark if pagination is possible + $suggestions->shrink( $this->limit ); + $search = trim( $search ); // preload the titles with LinkBatch $titles = $suggestions->map( function ( SearchSuggestion $sugg ) { @@ -830,7 +889,6 @@ abstract class SearchEngine { $setAugmentors = []; $rowAugmentors = []; Hooks::run( "SearchResultsAugment", [ &$setAugmentors, &$rowAugmentors ] ); - if ( !$setAugmentors && !$rowAugmentors ) { // We're done here return; diff --git a/includes/search/SearchResultSet.php b/includes/search/SearchResultSet.php index eb57559958..5728a52304 100644 --- a/includes/search/SearchResultSet.php +++ b/includes/search/SearchResultSet.php @@ -24,8 +24,7 @@ /** * @ingroup Search */ -class SearchResultSet implements IteratorAggregate { - +class SearchResultSet implements Countable, IteratorAggregate { /** * Types of interwiki results */ @@ -65,10 +64,23 @@ class SearchResultSet implements IteratorAggregate { */ protected $extraData = []; - /** @var ArrayIterator|null Iterator supporting BC iteration methods */ + /** + * @var boolean True when there are more pages of search results available. + */ + private $hasMoreResults; + + /** + * @var ArrayIterator|null Iterator supporting BC iteration methods + */ private $bcIterator; - public function __construct( $containedSyntax = false ) { + /** + * @param bool $containedSyntax True when query is not requesting a simple + * term match + * @param bool $hasMoreResults True when there are more pages of search + * results available. + */ + public function __construct( $containedSyntax = false, $hasMoreResults = false ) { if ( static::class === __CLASS__ ) { // This class will eventually be abstract. SearchEngine implementations // already have to extend this class anyways to provide the actual @@ -76,6 +88,7 @@ class SearchResultSet implements IteratorAggregate { wfDeprecated( __METHOD__, 1.32 ); } $this->containedSyntax = $containedSyntax; + $this->hasMoreResults = $hasMoreResults; } /** @@ -90,7 +103,11 @@ class SearchResultSet implements IteratorAggregate { } function numRows() { - return 0; + return $this->count(); + } + + final public function count() { + return count( $this->extractResults() ); } /** @@ -232,6 +249,34 @@ class SearchResultSet implements IteratorAggregate { return $this->containedSyntax; } + /** + * @return bool True when there are more pages of search results available. + */ + public function hasMoreResults() { + return $this->hasMoreResults; + } + + /** + * @param int $limit Shrink result set to $limit and flag + * if more results are available. + */ + public function shrink( $limit ) { + if ( $this->count() > $limit ) { + $this->hasMoreResults = true; + // shrinking result set for implementations that + // have not implemented extractResults and use + // the default cache location. Other implementations + // must override this as well. + if ( is_array( $this->results ) ) { + $this->results = array_slice( $this->results, 0, $limit ); + } else { + throw new \UnexpectedValueException( + "When overriding result store extending classes must " + . " also override " . __METHOD__ ); + } + } + } + /** * Extract all the results in the result set as array. * @return SearchResult[] diff --git a/includes/search/SearchSuggestionSet.php b/includes/search/SearchSuggestionSet.php index aced5e18af..ab384206d3 100644 --- a/includes/search/SearchSuggestionSet.php +++ b/includes/search/SearchSuggestionSet.php @@ -35,6 +35,11 @@ class SearchSuggestionSet { */ private $pageMap = []; + /** + * @var bool Are more results available? + */ + private $hasMoreResults; + /** * Builds a new set of suggestions. * @@ -45,8 +50,10 @@ class SearchSuggestionSet { * unexpected behaviors. * * @param SearchSuggestion[] $suggestions (must be sorted by score) + * @param bool $hasMoreResults Are more results available? */ - public function __construct( array $suggestions ) { + public function __construct( array $suggestions, $hasMoreResults = false ) { + $this->hasMoreResults = $hasMoreResults; foreach ( $suggestions as $suggestion ) { $pageID = $suggestion->getSuggestedTitleID(); if ( $pageID && empty( $this->pageMap[$pageID] ) ) { @@ -56,6 +63,13 @@ class SearchSuggestionSet { } } + /** + * @return bool Are more results available? + */ + public function hasMoreResults() { + return $this->hasMoreResults; + } + /** * Get the list of suggestions. * @return SearchSuggestion[] @@ -167,6 +181,7 @@ class SearchSuggestionSet { public function shrink( $limit ) { if ( count( $this->suggestions ) > $limit ) { $this->suggestions = array_slice( $this->suggestions, 0, $limit ); + $this->hasMoreResults = true; } } @@ -177,14 +192,15 @@ class SearchSuggestionSet { * NOTE: Suggestion scores will be generated. * * @param Title[] $titles + * @param bool $hasMoreResults Are more results available? * @return SearchSuggestionSet */ - public static function fromTitles( array $titles ) { + public static function fromTitles( array $titles, $hasMoreResults = false ) { $score = count( $titles ); $suggestions = array_map( function ( $title ) use ( &$score ) { return SearchSuggestion::fromTitle( $score--, $title ); }, $titles ); - return new SearchSuggestionSet( $suggestions ); + return new SearchSuggestionSet( $suggestions, $hasMoreResults ); } /** @@ -193,14 +209,15 @@ class SearchSuggestionSet { * NOTE: Suggestion scores will be generated. * * @param string[] $titles + * @param bool $hasMoreResults Are more results available? * @return SearchSuggestionSet */ - public static function fromStrings( array $titles ) { + public static function fromStrings( array $titles, $hasMoreResults = false ) { $score = count( $titles ); $suggestions = array_map( function ( $title ) use ( &$score ) { return SearchSuggestion::fromText( $score--, $title ); }, $titles ); - return new SearchSuggestionSet( $suggestions ); + return new SearchSuggestionSet( $suggestions, $hasMoreResults ); } /** diff --git a/tests/common/TestsAutoLoader.php b/tests/common/TestsAutoLoader.php index 4ecd383c76..2cc56419e6 100644 --- a/tests/common/TestsAutoLoader.php +++ b/tests/common/TestsAutoLoader.php @@ -184,6 +184,7 @@ $wgAutoloadClasses += [ => "$testDir/phpunit/mocks/session/DummySessionBackend.php", 'DummySessionProvider' => "$testDir/phpunit/mocks/session/DummySessionProvider.php", 'MockMessageLocalizer' => "$testDir/phpunit/mocks/MockMessageLocalizer.php", + 'MockCompletionSearchEngine' => "$testDir/phpunit/mocks/search/MockCompletionSearchEngine.php", 'MockSearchEngine' => "$testDir/phpunit/mocks/search/MockSearchEngine.php", 'MockSearchResultSet' => "$testDir/phpunit/mocks/search/MockSearchResultSet.php", 'MockSearchResult' => "$testDir/phpunit/mocks/search/MockSearchResult.php", diff --git a/tests/phpunit/includes/api/ApiQueryPrefixSearchTest.php b/tests/phpunit/includes/api/ApiQueryPrefixSearchTest.php new file mode 100644 index 0000000000..ae5924d800 --- /dev/null +++ b/tests/phpunit/includes/api/ApiQueryPrefixSearchTest.php @@ -0,0 +1,48 @@ + [ 2, 2, 0, 2 ], + 'with offset' => [ 7, 2, 5, 2 ], + 'past end, no offset' => [ null, 11, 0, 20 ], + 'past end, with offset' => [ null, 5, 6, 10 ], + ]; + } + + /** + * @dataProvider offsetContinueProvider + */ + public function testOffsetContinue( $expectedOffset, $expectedResults, $offset, $limit ) { + $this->registerMockSearchEngine(); + $response = $this->doApiRequest( [ + 'action' => 'query', + 'list' => 'prefixsearch', + 'pssearch' => 'example query terms', + 'psoffset' => $offset, + 'pslimit' => $limit, + ] ); + $result = $response[0]; + $this->assertArrayNotHasKey( 'warnings', $result ); + $suggestions = $result['query']['prefixsearch']; + $this->assertCount( $expectedResults, $suggestions ); + if ( $expectedOffset == null ) { + $this->assertArrayNotHasKey( 'continue', $result ); + } else { + $this->assertArrayHasKey( 'continue', $result ); + $this->assertEquals( $expectedOffset, $result['continue']['psoffset'] ); + } + } + + private function registerMockSearchEngine() { + $this->setMwGlobals( [ + 'wgSearchType' => MockCompletionSearchEngine::class, + ] ); + } +} diff --git a/tests/phpunit/includes/search/SearchEnginePrefixTest.php b/tests/phpunit/includes/search/SearchEnginePrefixTest.php index 3f59295ab6..dd12590554 100644 --- a/tests/phpunit/includes/search/SearchEnginePrefixTest.php +++ b/tests/phpunit/includes/search/SearchEnginePrefixTest.php @@ -329,6 +329,21 @@ class SearchEnginePrefixTest extends MediaWikiLangTestCase { 'Redirect test', ], ] ], + [ [ + "Extra results must not be returned", + 'provision' => [ + 'Example', + 'Example Bar', + 'Example Foo', + 'Example Foo/Bar' + ], + 'query' => 'foo', + 'results' => [ + 'Example', + 'Example Bar', + 'Example Foo', + ], + ] ], ]; } @@ -337,16 +352,7 @@ class SearchEnginePrefixTest extends MediaWikiLangTestCase { * @covers PrefixSearch::searchBackend */ public function testSearchBackend( array $case ) { - $search = $stub = $this->getMockBuilder( SearchEngine::class ) - ->setMethods( [ 'completionSearchBackend' ] )->getMock(); - - $return = SearchSuggestionSet::fromStrings( $case['provision'] ); - - $search->expects( $this->any() ) - ->method( 'completionSearchBackend' ) - ->will( $this->returnValue( $return ) ); - - $search->setLimitOffset( 3 ); + $search = $this->mockSearchWithResults( $case['provision'] ); $results = $search->completionSearch( $case['query'] ); $results = $results->map( function ( SearchSuggestion $s ) { @@ -359,4 +365,43 @@ class SearchEnginePrefixTest extends MediaWikiLangTestCase { $case[0] ); } + + public function paginationProvider() { + $res = [ 'Example', 'Example Bar', 'Example Foo', 'Example Foo/Bar' ]; + return [ + 'With less than requested results no pagination' => [ + false, array_slice( $res, 0, 2 ), + ], + 'With same as requested results no pagination' => [ + false, array_slice( $res, 0, 3 ), + ], + 'With extra result returned offer pagination' => [ + true, $res, + ], + ]; + } + + /** + * @dataProvider paginationProvider + */ + public function testPagination( $hasMoreResults, $provision ) { + $search = $this->mockSearchWithResults( $provision ); + $results = $search->completionSearch( 'irrelevant' ); + + $this->assertEquals( $hasMoreResults, $results->hasMoreResults() ); + } + + private function mockSearchWithResults( $titleStrings, $limit = 3 ) { + $search = $stub = $this->getMockBuilder( SearchEngine::class ) + ->setMethods( [ 'completionSearchBackend' ] )->getMock(); + + $return = SearchSuggestionSet::fromStrings( $titleStrings ); + + $search->expects( $this->any() ) + ->method( 'completionSearchBackend' ) + ->will( $this->returnValue( $return ) ); + + $search->setLimitOffset( $limit ); + return $search; + } } diff --git a/tests/phpunit/includes/search/SearchResultSetTest.php b/tests/phpunit/includes/search/SearchResultSetTest.php index 9eeee63d21..26a0672922 100644 --- a/tests/phpunit/includes/search/SearchResultSetTest.php +++ b/tests/phpunit/includes/search/SearchResultSetTest.php @@ -42,4 +42,20 @@ class SearchResultSetTest extends MediaWikiTestCase { ] ); $this->assertEquals( [ 'foo' => 'bar' ], $result->getExtensionData() ); } + + /** + * @covers SearchResultSet::shrink + * @covers SearchResultSet::count + * @covers SearchResultSet::hasMoreResults + */ + public function testHasMoreResults() { + $result = SearchResult::newFromTitle( Title::newMainPage() ); + $resultSet = new MockSearchResultSet( array_fill( 0, 3, $result ) ); + $this->assertEquals( 3, count( $resultSet ) ); + $this->assertFalse( $resultSet->hasMoreResults() ); + $resultSet->shrink( 3 ); + $this->assertFalse( $resultSet->hasMoreResults() ); + $resultSet->shrink( 2 ); + $this->assertTrue( $resultSet->hasMoreResults() ); + } } diff --git a/tests/phpunit/mocks/search/MockCompletionSearchEngine.php b/tests/phpunit/mocks/search/MockCompletionSearchEngine.php new file mode 100644 index 0000000000..ae3d3eccd5 --- /dev/null +++ b/tests/phpunit/mocks/search/MockCompletionSearchEngine.php @@ -0,0 +1,27 @@ +getLinkCache(); + foreach ( range( 0, 10 ) as $i ) { + $dbkey = "Search_Result_$i"; + $lc->addGoodLinkObj( 6543 + $i, new TitleValue( NS_MAIN, $dbkey ) ); + self::$completionSearchResult[] = "Search Result $i"; + } + } + $results = array_slice( self::$completionSearchResult, $this->offset, $this->limit ); + + return SearchSuggestionSet::fromStrings( $results ); + } + +} diff --git a/tests/phpunit/mocks/search/MockSearchResultSet.php b/tests/phpunit/mocks/search/MockSearchResultSet.php index 99f093fced..20e2a9fb68 100644 --- a/tests/phpunit/mocks/search/MockSearchResultSet.php +++ b/tests/phpunit/mocks/search/MockSearchResultSet.php @@ -13,6 +13,7 @@ class MockSearchResultSet extends SearchResultSet { * to list of results for that type. */ public function __construct( array $results, array $interwikiResults = [] ) { + parent::__construct( false, false ); $this->results = $results; $this->interwikiResults = $interwikiResults; } -- 2.20.1