From 5da2c4197d3cf176f8b6ebae9dd25eee39655708 Mon Sep 17 00:00:00 2001 From: Erik Bernhardson Date: Thu, 2 Jun 2016 13:32:59 -0700 Subject: [PATCH] Push common search api parameters into SearchApi class We have a number of parameters that are pretty much the same between these different search api's. Lets make them actually the same by sharing the definitions, and then letting individual classes tweak them as needed by removing the offset, or adjusting the max limits as necessary. Change-Id: I6f987db8ecb63dc943b4d2518bfe3703c677448e --- includes/api/ApiOpenSearch.php | 35 ++---- includes/api/ApiQueryPrefixSearch.php | 40 ++---- includes/api/ApiQuerySearch.php | 67 ++-------- includes/api/SearchApi.php | 119 ++++++++++++++---- .../includes/api/ApiOpenSearchTest.php | 66 ++++++++++ 5 files changed, 190 insertions(+), 137 deletions(-) create mode 100644 tests/phpunit/includes/api/ApiOpenSearchTest.php diff --git a/includes/api/ApiOpenSearch.php b/includes/api/ApiOpenSearch.php index 066aaa3bca..b13ba58653 100644 --- a/includes/api/ApiOpenSearch.php +++ b/includes/api/ApiOpenSearch.php @@ -272,20 +272,7 @@ class ApiOpenSearch extends ApiBase { if ( $this->allowedParams !== null ) { return $this->allowedParams; } - $this->allowedParams = [ - 'search' => null, - 'limit' => [ - ApiBase::PARAM_DFLT => $this->getConfig()->get( 'OpenSearchDefaultLimit' ), - ApiBase::PARAM_TYPE => 'limit', - ApiBase::PARAM_MIN => 1, - ApiBase::PARAM_MAX => 100, - ApiBase::PARAM_MAX2 => 100 - ], - 'namespace' => [ - ApiBase::PARAM_DFLT => NS_MAIN, - ApiBase::PARAM_TYPE => 'namespace', - ApiBase::PARAM_ISMULTI => true - ], + $this->allowedParams = $this->buildCommonApiParams( false ) + [ 'suggest' => false, 'redirects' => [ ApiBase::PARAM_TYPE => [ 'return', 'resolve' ], @@ -297,19 +284,21 @@ class ApiOpenSearch extends ApiBase { 'warningsaserror' => false, ]; - $profileParam = $this->buildProfileApiParam( SearchEngine::COMPLETION_PROFILE_TYPE, - 'apihelp-query+prefixsearch-param-profile' ); - if ( $profileParam ) { - $this->allowedParams['profile'] = $profileParam; - } + // Use open search specific default limit + $this->allowedParams['limit'][ApiBase::PARAM_DFLT] = $this->getConfig()->get( + 'OpenSearchDefaultLimit' + ); + return $this->allowedParams; } public function getSearchProfileParams() { - if ( isset( $this->getAllowedParams()['profile'] ) ) { - return [ SearchEngine::COMPLETION_PROFILE_TYPE => 'profile' ]; - } - return []; + return [ + 'qiprofile' => [ + 'profile-type' => SearchEngine::COMPLETION_PROFILE_TYPE, + 'help-message' => 'apihelp-query+prefixsearch-param-profile' + ], + ]; } protected function getExamplesMessages() { diff --git a/includes/api/ApiQueryPrefixSearch.php b/includes/api/ApiQueryPrefixSearch.php index 46538e0eb1..5bd451e3bd 100644 --- a/includes/api/ApiQueryPrefixSearch.php +++ b/includes/api/ApiQueryPrefixSearch.php @@ -106,42 +106,18 @@ class ApiQueryPrefixSearch extends ApiQueryGeneratorBase { if ( $this->allowedParams !== null ) { return $this->allowedParams; } - $this->allowedParams = [ - 'search' => [ - ApiBase::PARAM_TYPE => 'string', - ApiBase::PARAM_REQUIRED => true, - ], - 'namespace' => [ - ApiBase::PARAM_DFLT => NS_MAIN, - ApiBase::PARAM_TYPE => 'namespace', - ApiBase::PARAM_ISMULTI => true, - ], - 'limit' => [ - ApiBase::PARAM_DFLT => 10, - ApiBase::PARAM_TYPE => 'limit', - ApiBase::PARAM_MIN => 1, - // Non-standard value for compatibility with action=opensearch - ApiBase::PARAM_MAX => 100, - ApiBase::PARAM_MAX2 => 200, - ], - 'offset' => [ - ApiBase::PARAM_DFLT => 0, - ApiBase::PARAM_TYPE => 'integer', - ], - ]; - $profileParam = $this->buildProfileApiParam( SearchEngine::COMPLETION_PROFILE_TYPE, - 'apihelp-query+prefixsearch-param-profile' ); - if ( $profileParam ) { - $this->allowedParams['profile'] = $profileParam; - } + $this->allowedParams = $this->buildCommonApiParams(); + return $this->allowedParams; } public function getSearchProfileParams() { - if ( isset( $this->getAllowedParams()['profile'] ) ) { - return [ SearchEngine::COMPLETION_PROFILE_TYPE => 'profile' ]; - } - return []; + return [ + 'profile' => [ + 'profile-type' => SearchEngine::COMPLETION_PROFILE_TYPE, + 'help-message' => 'apihelp-query+prefixsearch-param-profile', + ], + ]; } protected function getExamplesMessages() { diff --git a/includes/api/ApiQuerySearch.php b/includes/api/ApiQuerySearch.php index 80798a10cd..a05f6be61b 100644 --- a/includes/api/ApiQuerySearch.php +++ b/includes/api/ApiQuerySearch.php @@ -37,14 +37,6 @@ class ApiQuerySearch extends ApiQueryGeneratorBase { /** @var array list of api allowed params */ private $allowedParams; - /** - * When $wgSearchType is null, $wgSearchAlternatives[0] is null. Null isn't - * a valid option for an array for PARAM_TYPE, so we'll use a fake name - * that can't possibly be a class name and describes what the null behavior - * does - */ - const BACKEND_NULL_PARAM = 'database-backed'; - public function __construct( ApiQuery $query, $moduleName ) { parent::__construct( $query, $moduleName, 'sr' ); } @@ -65,10 +57,6 @@ class ApiQuerySearch extends ApiQueryGeneratorBase { global $wgContLang; $params = $this->extractRequestParams(); - if ( isset( $params['backend'] ) && $params['backend'] == self::BACKEND_NULL_PARAM ) { - unset( $params['backend'] ); - } - // Extract parameters $query = $params['search']; $what = $params['what']; @@ -309,16 +297,7 @@ class ApiQuerySearch extends ApiQueryGeneratorBase { return $this->allowedParams; } - $this->allowedParams = [ - 'search' => [ - ApiBase::PARAM_TYPE => 'string', - ApiBase::PARAM_REQUIRED => true - ], - 'namespace' => [ - ApiBase::PARAM_DFLT => NS_MAIN, - ApiBase::PARAM_TYPE => 'namespace', - ApiBase::PARAM_ISMULTI => true, - ], + $this->allowedParams = $this->buildCommonApiParams() + [ 'what' => [ ApiBase::PARAM_TYPE => [ 'title', @@ -355,52 +334,20 @@ class ApiQuerySearch extends ApiQueryGeneratorBase { ApiBase::PARAM_ISMULTI => true, ApiBase::PARAM_HELP_MSG_PER_VALUE => [], ], - 'offset' => [ - ApiBase::PARAM_DFLT => 0, - ApiBase::PARAM_HELP_MSG => 'api-help-param-continue', - ], - 'limit' => [ - ApiBase::PARAM_DFLT => 10, - ApiBase::PARAM_TYPE => 'limit', - ApiBase::PARAM_MIN => 1, - ApiBase::PARAM_MAX => ApiBase::LIMIT_BIG1, - ApiBase::PARAM_MAX2 => ApiBase::LIMIT_BIG2 - ], 'interwiki' => false, 'enablerewrites' => false, ]; - $searchConfig = MediaWikiServices::getInstance()->getSearchEngineConfig(); - $alternatives = $searchConfig->getSearchTypes(); - if ( count( $alternatives ) > 1 ) { - if ( $alternatives[0] === null ) { - $alternatives[0] = self::BACKEND_NULL_PARAM; - } - $this->allowedParams['backend'] = [ - ApiBase::PARAM_DFLT => $searchConfig->getSearchType(), - ApiBase::PARAM_TYPE => $alternatives, - ]; - // @todo: support profile selection when multiple - // backends are available. The solution could be to - // merge all possible profiles and let ApiBase - // subclasses do the check. Making ApiHelp and ApiSandbox - // comprehensive might be more difficult. - } else { - $profileParam = $this->buildProfileApiParam( SearchEngine::FT_QUERY_INDEP_PROFILE_TYPE, - 'apihelp-query+search-param-qiprofile' ); - if ( $profileParam ) { - $this->allowedParams['qiprofile'] = $profileParam; - } - } - return $this->allowedParams; } public function getSearchProfileParams() { - if ( isset( $this->getAllowedParams()['qiprofile'] ) ) { - return [ SearchEngine::FT_QUERY_INDEP_PROFILE_TYPE => 'qiprofile' ]; - } - return []; + return [ + 'qiprofile' => [ + 'profile-type' => SearchEngine::FT_QUERY_INDEP_PROFILE_TYPE, + 'help-message' => 'apihelp-query+search-param-qiprofile', + ], + ]; } protected function getExamplesMessages() { diff --git a/includes/api/SearchApi.php b/includes/api/SearchApi.php index 139793d12b..8ae1192837 100644 --- a/includes/api/SearchApi.php +++ b/includes/api/SearchApi.php @@ -26,26 +26,89 @@ use MediaWiki\MediaWikiServices; * @ingroup API */ trait SearchApi { + + /** + * When $wgSearchType is null, $wgSearchAlternatives[0] is null. Null isn't + * a valid option for an array for PARAM_TYPE, so we'll use a fake name + * that can't possibly be a class name and describes what the null behavior + * does + */ + private static $BACKEND_NULL_PARAM = 'database-backed'; + /** - * Build the profile api param definitions. + * The set of api parameters that are shared between api calls that + * call the SearchEngine. Primarily this defines parameters that + * are utilized by self::buildSearchEngine(). * - * @param string $profileType type of profile to customize - * @param string $helpMsg i18n message - * @param string|null $backendType SearchEngine backend type or null for default engine - * @return array|null the api param definition or null if profiles are - * not supported by the searchEngine implementation. + * @param bool $isScrollable True if the api offers scrolling + * @return array */ - public function buildProfileApiParam( $profileType, $helpMsg, $backendType = null ) { - $searchEngine = null; - if ( $backendType !== null ) { - $searchEngine = MediaWikiServices::getInstance() - ->getSearchEngineFactory()->create( $backendType ); + public function buildCommonApiParams( $isScrollable = true ) { + $params = [ + 'search' => [ + ApiBase::PARAM_TYPE => 'string', + ApiBase::PARAM_REQUIRED => true, + ], + 'namespace' => [ + ApiBase::PARAM_DFLT => NS_MAIN, + ApiBase::PARAM_TYPE => 'namespace', + ApiBase::PARAM_ISMULTI => true, + ], + 'limit' => [ + ApiBase::PARAM_DFLT => 10, + ApiBase::PARAM_TYPE => 'limit', + ApiBase::PARAM_MIN => 1, + ApiBase::PARAM_MAX => ApiBase::LIMIT_BIG1, + ApiBase::PARAM_MAX2 => ApiBase::LIMIT_BIG2, + ], + ]; + if ( $isScrollable ) { + $params['offset'] = [ + ApiBase::PARAM_DFLT => 0, + ApiBase::PARAM_TYPE => 'integer', + ApiBase::PARAM_HELP_MSG => 'api-help-param-continue', + ]; + } + + $searchConfig = MediaWikiServices::getInstance()->getSearchEngineConfig(); + $alternatives = $searchConfig->getSearchTypes(); + if ( count( $alternatives ) > 1 ) { + if ( $alternatives[0] === null ) { + $alternatives[0] = self::$BACKEND_NULL_PARAM; + } + $this->allowedParams['backend'] = [ + ApiBase::PARAM_DFLT => $searchConfig->getSearchType(), + ApiBase::PARAM_TYPE => $alternatives, + ]; + // @todo: support profile selection when multiple + // backends are available. The solution could be to + // merge all possible profiles and let ApiBase + // subclasses do the check. Making ApiHelp and ApiSandbox + // comprehensive might be more difficult. } else { - $searchEngine = MediaWikiServices::getInstance()->newSearchEngine(); + $params += $this->buildProfileApiParam(); } - $profiles = $searchEngine->getProfiles( $profileType ); - if ( $profiles ) { + return $params; + } + + /** + * Build the profile api param definitions. Makes bold assumption only one search + * engine is available, ensure that is true before calling. + * + * @return array array containing available additional api param definitions. + * Empty if profiles are not supported by the searchEngine implementation. + */ + private function buildProfileApiParam() { + $configs = $this->getSearchProfileParams(); + $searchEngine = MediaWikiServices::getInstance()->newSearchEngine(); + $params = []; + foreach ( $configs as $paramName => $paramConfig ) { + $profiles = $searchEngine->getProfiles( $paramConfig['profile-type'] ); + if ( !$profiles ) { + continue; + } + $types = []; $helpMessages = []; $defaultProfile = null; @@ -58,20 +121,23 @@ trait SearchApi { $defaultProfile = $profile['name']; } } - return [ + + $params[$paramName] = [ ApiBase::PARAM_TYPE => $types, - ApiBase::PARAM_HELP_MSG => $helpMsg, + ApiBase::PARAM_HELP_MSG => $paramConfig['help-message'], ApiBase::PARAM_HELP_MSG_PER_VALUE => $helpMessages, ApiBase::PARAM_DFLT => $defaultProfile, ]; } - return null; + + return $params; } /** * Build the search engine to use. * If $params is provided then the following searchEngine options * 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 ) @@ -84,6 +150,9 @@ trait SearchApi { public function buildSearchEngine( array $params = null ) { if ( $params != null ) { $type = isset( $params['backend'] ) ? $params['backend'] : null; + if ( $type === self::$BACKEND_NULL_PARAM ) { + $type = null; + } $searchEngine = MediaWikiServices::getInstance()->getSearchEngineFactory()->create( $type ); $limit = $params['limit']; $searchEngine->setNamespaces( $params['namespace'] ); @@ -97,9 +166,15 @@ trait SearchApi { $limit += 1; } $searchEngine->setLimitOffset( $limit, $offset ); - foreach ( $this->getSearchProfileParams() as $type => $param ) { - if ( isset( $params[$param] ) ) { - $searchEngine->setFeatureData( $type, $params[$param] ); + + // Initialize requested search profiles. + $configs = $this->getSearchProfileParams(); + foreach ( $configs as $paramName => $paramConfig ) { + if ( isset( $params[$paramName] ) ) { + $searchEngine->setFeatureData( + $paramConfig['profile-type'], + $params[$paramName] + ); } } } else { @@ -109,8 +184,8 @@ trait SearchApi { } /** - * @return string[] the list of supported search profile types. Key is - * the profile type and its associated value is the request param. + * @return array[] array of arrays mapping from parameter name to a two value map + * containing 'help-message' and 'profile-type' keys. */ abstract public function getSearchProfileParams(); } diff --git a/tests/phpunit/includes/api/ApiOpenSearchTest.php b/tests/phpunit/includes/api/ApiOpenSearchTest.php new file mode 100644 index 0000000000..fd5b3a9998 --- /dev/null +++ b/tests/phpunit/includes/api/ApiOpenSearchTest.php @@ -0,0 +1,66 @@ +replaceSearchEngineConfig(); + $config->expects( $this->any() ) + ->method( 'getSearchTypes' ) + ->will( $this->returnValue( [ 'the one ring' ] ) ); + + $engine = $this->replaceSearchEngine(); + $engine->expects( $this->any() ) + ->method( 'getProfiles' ) + ->will( $this->returnValueMap( [ + [ SearchEngine::COMPLETION_PROFILE_TYPE, [ + [ + 'name' => 'normal', + 'desc-message' => 'normal-message', + 'default' => true, + ], + [ + 'name' => 'strict', + 'desc-message' => 'strict-message', + ], + ] ], + ] ) ); + + $api = $this->createApi(); + $params = $api->getAllowedParams(); + + $this->assertArrayNotHasKey( 'offset', $params ); + $this->assertArrayHasKey( 'qiprofile', $params, print_r( $params, true ) ); + $this->assertEquals( 'normal', $params['qiprofile'][ApiBase::PARAM_DFLT] ); + } + + private function replaceSearchEngineConfig() { + $config = $this->getMockBuilder( 'SearchEngineConfig' ) + ->disableOriginalConstructor() + ->getMock(); + $this->setService( 'SearchEngineConfig', $config ); + + return $config; + } + + private function replaceSearchEngine() { + $engine = $this->getMockBuilder( 'SearchEngine' ) + ->disableOriginalConstructor() + ->getMock(); + $engineFactory = $this->getMockBuilder( 'SearchEngineFactory' ) + ->disableOriginalConstructor() + ->getMock(); + $engineFactory->expects( $this->any() ) + ->method( 'create' ) + ->will( $this->returnValue( $engine ) ); + $this->setService( 'SearchEngineFactory', $engineFactory ); + + return $engine; + } + + private function createApi() { + $ctx = new RequestContext(); + $apiMain = new ApiMain( $ctx ); + return new ApiOpenSearch( $apiMain, 'opensearch', '' ); + } +} -- 2.20.1