Convert SearchResultSet to typical iteration

The funky iteration here was at best annoying. Switch
it over to an iterator based approach with appropriate
BC code to simulate the old iteration style.

Depends-On: I19a8d6621a130811871dec9335038797627d9448
Change-Id: I9fccda15dd58a0dc35771d3b5cd7a6e8b02514a0
This commit is contained in:
Erik Bernhardson 2018-05-10 15:03:55 -07:00
parent b80bcdeae8
commit c2a308075f
18 changed files with 434 additions and 76 deletions

View file

@ -188,6 +188,13 @@ because of Phabricator reports.
* The ApiQueryContributions class has been renamed to ApiQueryUserContribs.
* The XMPInfo, XMPReader, and XMPValidate classes have been deprecated in favor
of the namespaced classes provided by the wikimedia/xmp-reader library.
* SearchResultSet::{next,rewind} are deprecated. Calling code should
use foreach on the SearchResultSet, or the extractResults method. Extending
code should override extractResults.
* Instantiating SearchResultSet directly is deprecated. SearchEngine
implementations must subclass SearchResultSet for their purposes.
* SearchResult::setExtensionData argument has been changed from accepting an
array to accepting a Closure that returns the array when called.
* Class CryptRand, everything in MWCryptRand except generateHex() and function
MediaWikiServices::getCryptRand() are deprecated, use random_bytes() to
generate cryptographically secure random byte sequences.

View file

@ -142,10 +142,9 @@ class ApiQuerySearch extends ApiQueryGeneratorBase {
$terms = $wgContLang->convertForSearchResult( $matches->termMatches() );
$titles = [];
$count = 0;
$result = $matches->next();
$limit = $params['limit'];
while ( $result ) {
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...
@ -155,7 +154,6 @@ class ApiQuerySearch extends ApiQueryGeneratorBase {
// Silently skip broken and missing titles
if ( $result->isBrokenTitle() || $result->isMissingRevision() ) {
$result = $matches->next();
continue;
}
@ -172,8 +170,6 @@ class ApiQuerySearch extends ApiQueryGeneratorBase {
} else {
$titles[] = $result->getTitle();
}
$result = $matches->next();
}
// Here we assume interwiki results do not count with
@ -301,8 +297,7 @@ class ApiQuerySearch extends ApiQueryGeneratorBase {
// Include number of results if requested
$totalhits += $interwikiMatches->getTotalHits();
$result = $interwikiMatches->next();
while ( $result ) {
foreach ( $interwikiMatches as $result ) {
$title = $result->getTitle();
$vals = $this->getSearchResultData( $result, $prop, $terms );
@ -322,8 +317,6 @@ class ApiQuerySearch extends ApiQueryGeneratorBase {
// pagination info so just bail out
break;
}
$result = $interwikiMatches->next();
}
}
if ( $totalhits !== null ) {

View file

@ -3,28 +3,18 @@
* A SearchResultSet wrapper for SearchNearMatcher
*/
class SearchNearMatchResultSet extends SearchResultSet {
private $fetched = false;
/**
* @param Title|null $match Title if matched, else null
*/
public function __construct( $match ) {
$this->result = $match;
if ( $match === null ) {
$this->results = [];
} else {
$this->results = [ SearchResult::newFromTitle( $match, $this ) ];
}
}
public function numRows() {
return $this->result ? 1 : 0;
}
public function next() {
if ( $this->fetched || !$this->result ) {
return false;
}
$this->fetched = true;
return SearchResult::newFromTitle( $this->result, $this );
}
public function rewind() {
$this->fetched = false;
return $this->results ? 1 : 0;
}
}

View file

@ -57,8 +57,8 @@ class SearchResult {
protected $searchEngine;
/**
* A set of extension data.
* @var array[]
* A function returning a set of extension data.
* @var Closure|null
*/
protected $extensionData;
@ -267,17 +267,34 @@ class SearchResult {
* @return array[]
*/
public function getExtensionData() {
return $this->extensionData;
if ( $this->extensionData ) {
return call_user_func( $this->extensionData );
} else {
return [];
}
}
/**
* Set extension data for this result.
* The data is:
* augmentor name => data
* @param array[] $extensionData
* @param Closure|array $extensionData Takes no arguments, returns
* either array of extension data or null.
*/
public function setExtensionData( array $extensionData ) {
$this->extensionData = $extensionData;
public function setExtensionData( $extensionData ) {
if ( $extensionData instanceof Closure ) {
$this->extensionData = $extensionData;
} elseif ( is_array( $extensionData ) ) {
wfDeprecated( __METHOD__ . ' with array argument', 1.32 );
$this->extensionData = function () use ( $extensionData ) {
return $extensionData;
};
} else {
$type = is_object( $extensionData )
? get_class( $extensionData )
: gettype( $extensionData );
throw new \InvalidArgumentException(
__METHOD__ . " must be called with Closure|array, but received $type" );
}
}
}

View file

@ -24,7 +24,7 @@
/**
* @ingroup Search
*/
class SearchResultSet {
class SearchResultSet implements IteratorAggregate {
/**
* Types of interwiki results
@ -54,7 +54,7 @@ class SearchResultSet {
* as an array.
* @var SearchResult[]
*/
private $results;
protected $results;
/**
* Set of result's extra data, indexed per result id
@ -65,7 +65,16 @@ class SearchResultSet {
*/
protected $extraData = [];
/** @var ArrayIterator|null Iterator supporting BC iteration methods */
private $bcIterator;
public function __construct( $containedSyntax = false ) {
if ( static::class === __CLASS__ ) {
// This class will eventually be abstract. SearchEngine implementations
// already have to extend this class anyways to provide the actual
// search results.
wfDeprecated( __METHOD__, 1.32 );
}
$this->containedSyntax = $containedSyntax;
}
@ -171,20 +180,39 @@ class SearchResultSet {
/**
* Fetches next search result, or false.
* STUB
* FIXME: refactor as iterator, so we could use nicer interfaces.
* @deprecated since 1.32; Use self::extractResults()
* @deprecated since 1.32; Use self::extractResults() or foreach
* @return SearchResult|false
*/
function next() {
return false;
public function next() {
wfDeprecated( __METHOD__, '1.32' );
$it = $this->bcIterator();
$searchResult = $it->current();
$it->next();
return $searchResult === null ? false : $searchResult;
}
/**
* Rewind result set back to beginning
* @deprecated since 1.32; Use self::extractResults()
* @deprecated since 1.32; Use self::extractResults() or foreach
*/
function rewind() {
public function rewind() {
wfDeprecated( __METHOD__, '1.32' );
$this->bcIterator()->rewind();
}
private function bcIterator() {
if ( $this->bcIterator === null ) {
$this->bcIterator = 'RECURSION';
$this->bcIterator = $this->getIterator();
} elseif ( $this->bcIterator === 'RECURSION' ) {
// Either next/rewind or extractResults must be implemented. This
// class was potentially instantiated directly. It should be
// abstract with abstract methods to enforce this but that's a
// breaking change...
wfDeprecated( static::class . ' without implementing extractResults', '1.32' );
$this->bcIterator = new ArrayIterator( [] );
}
return $this->bcIterator;
}
/**
@ -258,15 +286,19 @@ class SearchResultSet {
/**
* Returns extra data for specific result and store it in SearchResult object.
* @param SearchResult $result
* @return array|null List of data as name => value or null if none present.
*/
public function augmentResult( SearchResult $result ) {
$id = $result->getTitle()->getArticleID();
if ( !$id || !isset( $this->extraData[$id] ) ) {
return null;
if ( $id === -1 ) {
return;
}
$result->setExtensionData( $this->extraData[$id] );
return $this->extraData[$id];
$result->setExtensionData( function () use ( $id ) {
if ( isset( $this->extraData[$id] ) ) {
return $this->extraData[$id];
} else {
return [];
}
} );
}
/**
@ -278,4 +310,8 @@ class SearchResultSet {
public function getOffset() {
return null;
}
final public function getIterator() {
return new ArrayIterator( $this->extractResults() );
}
}

View file

@ -7,8 +7,11 @@ use Wikimedia\Rdbms\ResultWrapper;
* @ingroup Search
*/
class SqlSearchResultSet extends SearchResultSet {
/** @var ResultWrapper Result object from database */
protected $resultSet;
/** @var string Requested search query */
protected $terms;
/** @var int|null Total number of hits for $terms */
protected $totalHits;
function __construct( ResultWrapper $resultSet, $terms, $total = null ) {
@ -29,25 +32,21 @@ class SqlSearchResultSet extends SearchResultSet {
return $this->resultSet->numRows();
}
function next() {
public function extractResults() {
if ( $this->resultSet === false ) {
return false;
return [];
}
$row = $this->resultSet->fetchObject();
if ( $row === false ) {
return false;
}
return SearchResult::newFromTitle(
Title::makeTitle( $row->page_namespace, $row->page_title ), $this
);
}
function rewind() {
if ( $this->resultSet ) {
if ( $this->results === null ) {
$this->results = [];
$this->resultSet->rewind();
while ( ( $row = $this->resultSet->fetchObject() ) !== false ) {
$this->results[] = SearchResult::newFromTitle(
Title::makeTitle( $row->page_namespace, $row->page_title ), $this
);
}
}
return $this->results;
}
function free() {

View file

@ -123,10 +123,8 @@ class BasicSearchResultSetWidget {
$terms = $wgContLang->convertForSearchResult( $resultSet->termMatches() );
$hits = [];
$result = $resultSet->next();
while ( $result ) {
$hits[] .= $this->resultWidget->render( $result, $terms, $offset++ );
$result = $resultSet->next();
foreach ( $resultSet as $result ) {
$hits[] = $this->resultWidget->render( $result, $terms, $offset++ );
}
return "<ul class='mw-search-results'>" . implode( '', $hits ) . "</ul>";

View file

@ -56,12 +56,10 @@ class SimpleSearchResultSetWidget implements SearchResultSetWidget {
$iwResults = [];
foreach ( $resultSets as $resultSet ) {
$result = $resultSet->next();
while ( $result ) {
foreach ( $resultSet as $result ) {
if ( !$result->isBrokenTitle() ) {
$iwResults[$result->getTitle()->getInterwiki()][] = $result;
}
$result = $resultSet->next();
}
}

View file

@ -184,6 +184,9 @@ $wgAutoloadClasses += [
=> "$testDir/phpunit/mocks/session/DummySessionBackend.php",
'DummySessionProvider' => "$testDir/phpunit/mocks/session/DummySessionProvider.php",
'MockMessageLocalizer' => "$testDir/phpunit/mocks/MockMessageLocalizer.php",
'MockSearchEngine' => "$testDir/phpunit/mocks/search/MockSearchEngine.php",
'MockSearchResultSet' => "$testDir/phpunit/mocks/search/MockSearchResultSet.php",
'MockSearchResult' => "$testDir/phpunit/mocks/search/MockSearchResult.php",
# tests/suites
'ParserTestFileSuite' => "$testDir/phpunit/suites/ParserTestFileSuite.php",

View file

@ -0,0 +1,109 @@
<?php
/**
* @group medium
* @covers ApiQuerySearch
*/
class ApiQuerySearchTest extends ApiTestCase {
public function provideSearchResults() {
return [
'empty search result' => [ [], [] ],
'has search results' => [
[ 'Zomg' ],
[ $this->mockResult( 'Zomg' ) ],
],
'filters broken search results' => [
[ 'A', 'B' ],
[
$this->mockResult( 'a' ),
$this->mockResult( 'Zomg' )->setBrokenTitle( true ),
$this->mockResult( 'b' ),
],
],
'filters results with missing revision' => [
[ 'B', 'A' ],
[
$this->mockResult( 'Zomg' )->setMissingRevision( true ),
$this->mockResult( 'b' ),
$this->mockResult( 'a' ),
],
],
];
}
/**
* @dataProvider provideSearchResults
*/
public function testSearchResults( $expect, $hits, array $params = [] ) {
MockSearchEngine::addMockResults( 'my query', $hits );
list( $response, $request ) = $this->doApiRequest( $params + [
'action' => 'query',
'list' => 'search',
'srsearch' => 'my query',
] );
$titles = [];
foreach ( $response['query']['search'] as $result ) {
$titles[] = $result['title'];
}
$this->assertEquals( $expect, $titles );
}
public function provideInterwikiResults() {
return [
'empty' => [ [], [] ],
'one wiki response' => [
[ 'utwiki' => [ 'Qwerty' ] ],
[
SearchResultSet::SECONDARY_RESULTS => [
'utwiki' => new MockSearchResultSet( [
$this->mockResult( 'Qwerty' )->setInterwikiPrefix( 'utwiki' ),
] ),
],
]
],
];
}
/**
* @dataProvider provideInterwikiResults
*/
public function testInterwikiResults( $expect, $hits, array $params = [] ) {
MockSearchEngine::setMockInterwikiResults( $hits );
list( $response, $request ) = $this->doApiRequest( $params + [
'action' => 'query',
'list' => 'search',
'srsearch' => 'my query',
'srinterwiki' => true,
] );
if ( !$expect ) {
$this->assertArrayNotHasKey( 'interwikisearch', $response['query'] );
return;
}
$results = [];
$this->assertArrayHasKey( 'interwikisearchinfo', $response['query'] );
foreach ( $response['query']['interwikisearch'] as $wiki => $wikiResults ) {
$results[$wiki] = [];
foreach ( $wikiResults as $wikiResult ) {
$results[$wiki][] = $wikiResult['title'];
}
}
$this->assertEquals( $expect, $results );
}
public function setUp() {
parent::setUp();
MockSearchEngine::clearMockResults();
$this->registerMockSearchEngine();
}
private function registerMockSearchEngine() {
$this->setMwGlobals( [
'wgSearchType' => MockSearchEngine::class,
] );
}
private function mockResult( $title ) {
return MockSearchResult::newFromtitle( Title::newFromText( $title ) );
}
}

View file

@ -84,10 +84,8 @@ class SearchEngineTest extends MediaWikiLangTestCase {
$this->assertTrue( is_object( $results ) );
$matches = [];
$row = $results->next();
while ( $row ) {
foreach ( $results as $row ) {
$matches[] = $row->getTitle()->getPrefixedText();
$row = $results->next();
}
$results->free();
# Search is not guaranteed to return results in a certain order;
@ -173,7 +171,7 @@ class SearchEngineTest extends MediaWikiLangTestCase {
public function testPhraseSearchHighlight() {
$phrase = "smithee is one who smiths";
$res = $this->search->searchText( "\"$phrase\"" );
$match = $res->next();
$match = $res->getIterator()->current();
$snippet = "A <span class='searchmatch'>" . $phrase . "</span>";
$this->assertStringStartsWith( $snippet,
$match->getTextSnippet( $res->termMatches() ),
@ -277,7 +275,7 @@ class SearchEngineTest extends MediaWikiLangTestCase {
$this->mergeMwGlobalArrayValue( 'wgHooks',
[ 'SearchResultsAugment' => [ [ $this, 'addAugmentors' ] ] ] );
$this->search->augmentSearchResults( $resultSet );
for ( $result = $resultSet->next(); $result; $result = $resultSet->next() ) {
foreach ( $resultSet as $result ) {
$id = $result->getTitle()->getArticleID();
$augmentData = "Result:$id:" . $result->getTitle()->getText();
$augmentData2 = "Result2:$id:" . $result->getTitle()->getText();
@ -292,11 +290,10 @@ class SearchEngineTest extends MediaWikiLangTestCase {
->method( 'augmentAll' )
->willReturnCallback( function ( SearchResultSet $resultSet ) {
$data = [];
for ( $result = $resultSet->next(); $result; $result = $resultSet->next() ) {
foreach ( $resultSet as $result ) {
$id = $result->getTitle()->getArticleID();
$data[$id] = "Result:$id:" . $result->getTitle()->getText();
}
$resultSet->rewind();
return $data;
} );
$setAugmentors['testSet'] = $setAugmentor;

View file

@ -0,0 +1,15 @@
<?php
class SearchNearMatchResultSetTest extends PHPUnit\Framework\TestCase {
/**
* @covers SearchNearMatchResultSet::__construct
* @covers SearchNearMatchResultSet::numRows
*/
public function testNumRows() {
$resultSet = new SearchNearMatchResultSet( null );
$this->assertEquals( 0, $resultSet->numRows() );
$resultSet = new SearchNearMatchResultSet( Title::newMainPage() );
$this->assertEquals( 1, $resultSet->numRows() );
}
}

View file

@ -0,0 +1,45 @@
<?php
class SearchResultSetTest extends MediaWikiTestCase {
/**
* @covers SearchResultSet::getIterator
* @covers SearchResultSet::next
* @covers SearchResultSet::rewind
*/
public function testIterate() {
$result = SearchResult::newFromTitle( Title::newMainPage() );
$resultSet = new MockSearchResultSet( [ $result ] );
$this->assertEquals( 1, $resultSet->numRows() );
$count = 0;
foreach ( $resultSet as $iterResult ) {
$this->assertEquals( $result, $iterResult );
$count++;
}
$this->assertEquals( 1, $count );
$this->hideDeprecated( 'SearchResultSet::rewind' );
$this->hideDeprecated( 'SearchResultSet::next' );
$resultSet->rewind();
$count = 0;
while ( false !== ( $iterResult = $resultSet->next() ) ) {
$this->assertEquals( $result, $iterResult );
$count++;
}
$this->assertEquals( 1, $count );
}
/**
* @covers SearchResultSet::augmentResult
* @covers SearchResultSet::setAugmentedData
*/
public function testDelayedResultAugment() {
$result = SearchResult::newFromTitle( Title::newMainPage() );
$resultSet = new MockSearchResultSet( [ $result ] );
$resultSet->augmentResult( $result );
$this->assertEquals( [], $result->getExtensionData() );
$resultSet->setAugmentedData( 'foo', [
$result->getTitle()->getArticleID() => 'bar'
] );
$this->assertEquals( [ 'foo' => 'bar' ], $result->getExtensionData() );
}
}

View file

@ -0,0 +1,38 @@
<?php
class SearchResultTest extends MediawikiTestCase {
/**
* @covers SearchResult::getExtensionData
* @covers SearchResult::setExtensionData
*/
public function testExtensionData() {
$result = SearchResult::newFromTitle( Title::newMainPage() );
$this->assertEquals( [], $result->getExtensionData(), 'starts empty' );
$data = [ 'hello' => 'world' ];
$result->setExtensionData( function () use ( &$data ) {
return $data;
} );
$this->assertEquals( $data, $result->getExtensionData(), 'can set extension data' );
$data['this'] = 'that';
$this->assertEquals( $data, $result->getExtensionData(), 'refetches from callback' );
}
/**
* @covers SearchResult::getExtensionData
* @covers SearchResult::setExtensionData
*/
public function testExtensionDataArrayBC() {
$result = SearchResult::newFromTitle( Title::newMainPage() );
$data = [ 'hello' => 'world' ];
$this->hideDeprecated( 'SearchResult::setExtensionData with array argument' );
$this->assertEquals( [], $result->getExtensionData(), 'starts empty' );
$result->setExtensionData( $data );
$this->assertEquals( $data, $result->getExtensionData(), 'can set extension data' );
$data['this'] = 'that';
$this->assertNotEquals( $data, $result->getExtensionData(), 'shouldnt hold any reference' );
$result->setExtensionData( $data );
$this->assertEquals( $data, $result->getExtensionData(), 'can replace extension data' );
}
}

View file

@ -262,8 +262,8 @@ class SpecialSearchTestMockResultSet extends SearchResultSet {
$this->containedSyntax = $containedSyntax;
}
public function numRows() {
return count( $this->results );
public function expandResults() {
return $this->results;
}
public function getTotalHits() {

View file

@ -0,0 +1,45 @@
<?php
use MediaWiki\MediaWikiServices;
class MockSearchEngine extends SearchEngine {
/** @var SearchResult[][] */
private static $results = [];
/** @var SearchResultSet[][] */
private static $interwikiResults = [];
public static function clearMockResults() {
self::$results = [];
self::$interwikiResults = [];
}
/**
* @param string $query The query searched for *after* initial
* transformations have been applied.
* @param SearchResult[] $results The results to return for $query
*/
public static function addMockResults( $query, array $results ) {
self::$results[$query] = $results;
$lc = MediaWikiServices::getInstance()->getLinkCache();
foreach ( $results as $result ) {
// TODO: better page ids?
$lc->addGoodLinkObj( mt_rand(), $result->getTitle() );
}
}
/**
* @param SearchResultSet[][] $interwikiResults
*/
public static function setMockInterwikiResults( array $interwikiResults ) {
self::$interwikiResults = $interwikiResults;
}
protected function doSearchText( $term ) {
if ( isset( self::$results[ $term ] ) ) {
$results = array_slice( self::$results[ $term ], $this->offset, $this->limit );
} else {
$results = [];
}
return new MockSearchResultSet( $results, self::$interwikiResults );
}
}

View file

@ -0,0 +1,32 @@
<?php
class MockSearchResult extends SearchResult {
private $isMissingRevision = false;
private $isBrokenTitle = false;
public function isMissingRevision() {
return $this->isMissingRevision;
}
public function setMissingRevision( $isMissingRevision ) {
$this->isMissingRevision = $isMissingRevision;
return $this;
}
public function isBrokenTitle() {
return $this->isBrokenTitle;
}
public function setBrokenTitle( $isBrokenTitle ) {
$this->isBrokenTitle = $isBrokenTitle;
return $this;
}
public function getInterwikiPrefix() {
return $this->interwikiPrefix;
}
public function setInterwikiPrefix( $interwikiPrefix ) {
$this->interwikiPrefix = $interwikiPrefix;
return $this;
}
}

View file

@ -0,0 +1,36 @@
<?php
class MockSearchResultSet extends SearchResultSet {
/*
* @var SearchResultSet[][] Map from result type to list of results for
* that type.
*/
private $interwikiResults;
/**
* @param SearchResult[] $results
* @param SearchResultSet[][] $interwikiResults Map from result type
* to list of results for that type.
*/
public function __construct( array $results, array $interwikiResults = [] ) {
$this->results = $results;
$this->interwikiResults = $interwikiResults;
}
public function numRows() {
return count( $this->results );
}
public function hasInterwikiResults( $type = self::SECONDARY_RESULTS ) {
return isset( $this->interwikiResults[$type] ) &&
count( $this->interwikiResults[$type] ) > 0;
}
public function getInterwikiResults( $type = self::SECONDARY_RESULTS ) {
if ( $this->hasInterwikiResults( $type ) ) {
return $this->interwikiResults[$type];
} else {
return null;
}
}
}