EBernhardson has uploaded a new change for review. (
https://gerrit.wikimedia.org/r/399312 )
Change subject: Push pagination decision for prefix search into SearchEngine
......................................................................
Push pagination decision for prefix 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. Full-text search unfortunately does the same
thing, but fixing it is delegated to some future patch.
Change-Id: Ica094428700637dfdedb723b03f6aeadfe12b9f4
---
M includes/api/ApiQueryPrefixSearch.php
M includes/api/SearchApi.php
M includes/search/SearchEngine.php
M includes/search/SearchSuggestionSet.php
M tests/phpunit/includes/search/SearchEnginePrefixTest.php
5 files changed, 116 insertions(+), 47 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/core
refs/changes/12/399312/1
diff --git a/includes/api/ApiQueryPrefixSearch.php
b/includes/api/ApiQueryPrefixSearch.php
index 2fbc518..aaf81b0 100644
--- a/includes/api/ApiQueryPrefixSearch.php
+++ b/includes/api/ApiQueryPrefixSearch.php
@@ -51,7 +51,12 @@
$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 @@
}
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 @@
$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(),
diff --git a/includes/api/SearchApi.php b/includes/api/SearchApi.php
index f7c6471..fb6b635 100644
--- a/includes/api/SearchApi.php
+++ b/includes/api/SearchApi.php
@@ -157,15 +157,7 @@
$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/SearchEngine.php b/includes/search/SearchEngine.php
index 3c8fe60..6876099 100644
--- a/includes/search/SearchEngine.php
+++ b/includes/search/SearchEngine.php
@@ -517,7 +517,15 @@
return SearchSuggestionSet::emptySuggestionSet(); //
Return empty result
}
$search = $this->normalizeNamespaces( $search );
- return $this->processCompletionResults( $search,
$this->completionSearchBackend( $search ) );
+ // Over-fetch results so we can determine if pagination is
possible in
+ // processCompletionResults
+ $this->limit++;
+ try {
+ $suggestions = $this->completionSearchBackend( $search
);
+ } finally {
+ $this->limit--;
+ }
+ return $this->processCompletionResults( $search, $suggestions );
}
/**
@@ -531,23 +539,29 @@
}
$search = $this->normalizeNamespaces( $search );
- $results = $this->completionSearchBackend( $search );
- $fallbackLimit = $this->limit - $results->getSize();
- if ( $fallbackLimit > 0 ) {
- global $wgContLang;
+ // Over-fetch results so we can determine if more results exist
(for pagination).
+ $this->limit++;
+ try {
+ $results = $this->completionSearchBackend( $search );
+ $fallbackLimit = $this->limit - $results->getSize();
+ if ( $fallbackLimit > 0 ) {
+ global $wgContLang;
- $fallbackSearches =
$wgContLang->autoConvertToAllVariants( $search );
- $fallbackSearches = array_diff( array_unique(
$fallbackSearches ), [ $search ] );
+ $fallbackSearches =
$wgContLang->autoConvertToAllVariants( $search );
+ $fallbackSearches = array_diff( array_unique(
$fallbackSearches ), [ $search ] );
- foreach ( $fallbackSearches as $fbs ) {
- $this->setLimitOffset( $fallbackLimit );
- $fallbackSearchResult =
$this->completionSearch( $fbs );
- $results->appendAll( $fallbackSearchResult );
- $fallbackLimit -= count( $fallbackSearchResult
);
- if ( $fallbackLimit <= 0 ) {
- break;
+ foreach ( $fallbackSearches as $fbs ) {
+ $this->setLimitOffset( $fallbackLimit );
+ $fallbackSearchResult =
$this->completionSearch( $fbs );
+ $results->appendAll(
$fallbackSearchResult );
+ $fallbackLimit -= count(
$fallbackSearchResult );
+ if ( $fallbackLimit <= 0 ) {
+ break;
+ }
}
}
+ } finally {
+ $this->limit--;
}
return $this->processCompletionResults( $search, $results );
}
@@ -571,6 +585,10 @@
* @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
) {
diff --git a/includes/search/SearchSuggestionSet.php
b/includes/search/SearchSuggestionSet.php
index aced5e1..ab38420 100644
--- a/includes/search/SearchSuggestionSet.php
+++ b/includes/search/SearchSuggestionSet.php
@@ -36,6 +36,11 @@
private $pageMap = [];
/**
+ * @var bool Are more results available?
+ */
+ private $hasMoreResults;
+
+ /**
* Builds a new set of suggestions.
*
* NOTE: the array should be sorted by score (higher is better),
@@ -45,8 +50,10 @@
* 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] ) ) {
@@ -54,6 +61,13 @@
}
$this->suggestions[] = $suggestion;
}
+ }
+
+ /**
+ * @return bool Are more results available?
+ */
+ public function hasMoreResults() {
+ return $this->hasMoreResults;
}
/**
@@ -167,6 +181,7 @@
public function shrink( $limit ) {
if ( count( $this->suggestions ) > $limit ) {
$this->suggestions = array_slice( $this->suggestions,
0, $limit );
+ $this->hasMoreResults = true;
}
}
@@ -177,14 +192,15 @@
* 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 @@
* 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/phpunit/includes/search/SearchEnginePrefixTest.php
b/tests/phpunit/includes/search/SearchEnginePrefixTest.php
index 4c5bab3..edc789c 100644
--- a/tests/phpunit/includes/search/SearchEnginePrefixTest.php
+++ b/tests/phpunit/includes/search/SearchEnginePrefixTest.php
@@ -329,6 +329,21 @@
'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 @@
* @covers PrefixSearch::searchBackend
*/
public function testSearchBackend( array $case ) {
- $search = $stub = $this->getMockBuilder( 'SearchEngine' )
- ->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 @@
$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 = $this->getMockBuilder( 'SearchEngine' )
+ ->setMethods( [ 'completionSearchBackend' ]
)->getMock();
+
+ $return = SearchSuggestionSet::fromStrings( $titleStrings );
+
+ $search->expects( $this->any() )
+ ->method( 'completionSearchBackend' )
+ ->will( $this->returnValue( $return ) );
+
+ $search->setLimitOffset( $limit );
+ return $search;
+ }
}
--
To view, visit https://gerrit.wikimedia.org/r/399312
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ica094428700637dfdedb723b03f6aeadfe12b9f4
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: EBernhardson <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
