From 5e7640338736c63faf5178b8f4d11b29a47da304 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Gerg=C5=91=20Tisza?= Date: Fri, 28 Jul 2017 17:41:13 +0000 Subject: [PATCH] Make API multivalue limits configurable Adds two new parameter settings, ApiBase::PARAM_ISMULTI_LIMIT1 and PARAM_ISMULTI_LIMIT2 for configuring the maximum number of values that can be contained in a multivalue field (for unprivileged and apihighlimits users, respectively). When present, these replace the default 50/500. Change-Id: Ic1b1bcc7ff556b7762c8d2375d910cc4fcb43087 --- includes/api/ApiBase.php | 37 +++++++++-- includes/api/ApiHelp.php | 24 +++++-- includes/api/ApiParamInfo.php | 14 ++-- includes/api/i18n/en.json | 1 + includes/api/i18n/qqq.json | 3 +- ...mentationTest.php => ApiStructureTest.php} | 65 +++++++++++++++++-- 6 files changed, 121 insertions(+), 23 deletions(-) rename tests/phpunit/structure/{ApiDocumentationTest.php => ApiStructureTest.php} (69%) diff --git a/includes/api/ApiBase.php b/includes/api/ApiBase.php index 2012e7d8cf..80aeff5478 100644 --- a/includes/api/ApiBase.php +++ b/includes/api/ApiBase.php @@ -204,6 +204,19 @@ abstract class ApiBase extends ContextSource { */ const PARAM_DEPRECATED_VALUES = 20; + /** + * (integer) Maximum number of values, for normal users. Must be used with PARAM_ISMULTI. + * @since 1.30 + */ + const PARAM_ISMULTI_LIMIT1 = 21; + + /** + * (integer) Maximum number of values, for users with the apihighimits right. + * Must be used with PARAM_ISMULTI. + * @since 1.30 + */ + const PARAM_ISMULTI_LIMIT2 = 22; + /**@}*/ const ALL_DEFAULT_STRING = '*'; @@ -1024,6 +1037,12 @@ abstract class ApiBase extends ContextSource { $multi = isset( $paramSettings[self::PARAM_ISMULTI] ) ? $paramSettings[self::PARAM_ISMULTI] : false; + $multiLimit1 = isset( $paramSettings[self::PARAM_ISMULTI_LIMIT1] ) + ? $paramSettings[self::PARAM_ISMULTI_LIMIT1] + : null; + $multiLimit2 = isset( $paramSettings[self::PARAM_ISMULTI_LIMIT2] ) + ? $paramSettings[self::PARAM_ISMULTI_LIMIT2] + : null; $type = isset( $paramSettings[self::PARAM_TYPE] ) ? $paramSettings[self::PARAM_TYPE] : null; @@ -1148,7 +1167,9 @@ abstract class ApiBase extends ContextSource { $value, $multi, is_array( $type ) ? $type : null, - $allowAll ? $allSpecifier : null + $allowAll ? $allSpecifier : null, + $multiLimit1, + $multiLimit2 ); } @@ -1350,21 +1371,25 @@ abstract class ApiBase extends ContextSource { * null, all values are accepted. * @param string|null $allSpecifier String to use to specify all allowed values, or null * if this behavior should not be allowed + * @param int|null $limit1 Maximum number of values, for normal users. + * @param int|null $limit2 Maximum number of values, for users with the apihighlimits right. * @return string|string[] (allowMultiple ? an_array_of_values : a_single_value) */ protected function parseMultiValue( $valueName, $value, $allowMultiple, $allowedValues, - $allSpecifier = null + $allSpecifier = null, $limit1 = null, $limit2 = null ) { if ( ( trim( $value ) === '' || trim( $value ) === "\x1f" ) && $allowMultiple ) { return []; } + $limit1 = $limit1 ?: self::LIMIT_SML1; + $limit2 = $limit2 ?: self::LIMIT_SML2; // This is a bit awkward, but we want to avoid calling canApiHighLimits() // because it unstubs $wgUser - $valuesList = $this->explodeMultiValue( $value, self::LIMIT_SML2 + 1 ); - $sizeLimit = count( $valuesList ) > self::LIMIT_SML1 && $this->mMainModule->canApiHighLimits() - ? self::LIMIT_SML2 - : self::LIMIT_SML1; + $valuesList = $this->explodeMultiValue( $value, $limit2 + 1 ); + $sizeLimit = count( $valuesList ) > $limit1 && $this->mMainModule->canApiHighLimits() + ? $limit2 + : $limit1; if ( $allowMultiple && is_array( $allowedValues ) && $allSpecifier && count( $valuesList ) === 1 && $valuesList[0] === $allSpecifier diff --git a/includes/api/ApiHelp.php b/includes/api/ApiHelp.php index 12e778bfb6..318555a93b 100644 --- a/includes/api/ApiHelp.php +++ b/includes/api/ApiHelp.php @@ -485,7 +485,9 @@ class ApiHelp extends ApiBase { $type = $settings[ApiBase::PARAM_TYPE]; $multi = !empty( $settings[ApiBase::PARAM_ISMULTI] ); $hintPipeSeparated = true; - $count = ApiBase::LIMIT_SML2 + 1; + $count = !empty( $settings[ApiBase::PARAM_ISMULTI_LIMIT2] ) + ? $settings[ApiBase::PARAM_ISMULTI_LIMIT2] + 1 + : ApiBase::LIMIT_SML2 + 1; if ( is_array( $type ) ) { $count = count( $type ); @@ -669,13 +671,25 @@ class ApiHelp extends ApiBase { if ( $multi ) { $extra = []; + $lowcount = !empty( $settings[ApiBase::PARAM_ISMULTI_LIMIT1] ) + ? $settings[ApiBase::PARAM_ISMULTI_LIMIT1] + : ApiBase::LIMIT_SML1; + $highcount = !empty( $settings[ApiBase::PARAM_ISMULTI_LIMIT2] ) + ? $settings[ApiBase::PARAM_ISMULTI_LIMIT2] + : ApiBase::LIMIT_SML2; + if ( $hintPipeSeparated ) { $extra[] = $context->msg( 'api-help-param-multi-separate' )->parse(); } - if ( $count > ApiBase::LIMIT_SML1 ) { - $extra[] = $context->msg( 'api-help-param-multi-max' ) - ->numParams( ApiBase::LIMIT_SML1, ApiBase::LIMIT_SML2 ) - ->parse(); + if ( $count > $lowcount ) { + if ( $lowcount === $highcount ) { + $msg = $context->msg( 'api-help-param-multi-max-simple' ) + ->numParams( $lowcount ); + } else { + $msg = $context->msg( 'api-help-param-multi-max' ) + ->numParams( $lowcount, $highcount ); + } + $extra[] = $msg->parse(); } if ( $extra ) { $info[] = implode( ' ', $extra ); diff --git a/includes/api/ApiParamInfo.php b/includes/api/ApiParamInfo.php index 480575ca26..2fa20a96ea 100644 --- a/includes/api/ApiParamInfo.php +++ b/includes/api/ApiParamInfo.php @@ -371,11 +371,15 @@ class ApiParamInfo extends ApiBase { $item['multi'] = !empty( $settings[ApiBase::PARAM_ISMULTI] ); if ( $item['multi'] ) { - $item['limit'] = $this->getMain()->canApiHighLimits() ? - ApiBase::LIMIT_SML2 : - ApiBase::LIMIT_SML1; - $item['lowlimit'] = ApiBase::LIMIT_SML1; - $item['highlimit'] = ApiBase::LIMIT_SML2; + $item['lowlimit'] = !empty( $settings[ApiBase::PARAM_ISMULTI_LIMIT1] ) + ? $settings[ApiBase::PARAM_ISMULTI_LIMIT1] + : ApiBase::LIMIT_SML1; + $item['highlimit'] = !empty( $settings[ApiBase::PARAM_ISMULTI_LIMIT2] ) + ? $settings[ApiBase::PARAM_ISMULTI_LIMIT2] + : ApiBase::LIMIT_SML2; + $item['limit'] = $this->getMain()->canApiHighLimits() + ? $item['highlimit'] + : $item['lowlimit']; } if ( !empty( $settings[ApiBase::PARAM_ALLOW_DUPLICATES] ) ) { diff --git a/includes/api/i18n/en.json b/includes/api/i18n/en.json index 3d4a100419..9fbc0127fe 100644 --- a/includes/api/i18n/en.json +++ b/includes/api/i18n/en.json @@ -1593,6 +1593,7 @@ "api-help-param-upload": "Must be posted as a file upload using multipart/form-data.", "api-help-param-multi-separate": "Separate values with | or [[Special:ApiHelp/main#main/datatypes|alternative]].", "api-help-param-multi-max": "Maximum number of values is {{PLURAL:$1|$1}} ({{PLURAL:$2|$2}} for bots).", + "api-help-param-multi-max-simple": "Maximum number of values is {{PLURAL:$1|$1}}.", "api-help-param-multi-all": "To specify all values, use $1.", "api-help-param-default": "Default: $1", "api-help-param-default-empty": "Default: (empty)", diff --git a/includes/api/i18n/qqq.json b/includes/api/i18n/qqq.json index 4336c29349..c878a539e6 100644 --- a/includes/api/i18n/qqq.json +++ b/includes/api/i18n/qqq.json @@ -1483,7 +1483,8 @@ "api-help-param-integer-minmax": "Used to display an integer parameter with a maximum and minimum values\n\nParameters:\n* $1 - 1 if the parameter takes one value, 2 if the parameter takes any number of values\n* $2 - Minimum value\n* $3 - Maximum value\n\nSee also:\n* {{msg-mw|api-help-param-integer-min}}\n* {{msg-mw|api-help-param-integer-max}}", "api-help-param-upload": "{{technical}} Used to indicate that an 'upload'-type parameter must be posted as a file upload using multipart/form-data", "api-help-param-multi-separate": "Used to indicate how to separate multiple values. Not used with {{msg-mw|api-help-param-list}}.", - "api-help-param-multi-max": "Used to indicate the maximum number of values accepted for a multi-valued parameter.\n\nParameters:\n* $1 - Maximum value without the apihighlimits right\n* $2 - Maximum value with the apihighlimits right", + "api-help-param-multi-max": "Used to indicate the maximum number of values accepted for a multi-valued parameter when that value is influenced by the user having apihighlimits right (otherwise {{msg-mw|api-help-param-multi-max-simple}} is used).\n\nParameters:\n* $1 - Maximum value without the apihighlimits right\n* $2 - Maximum value with the apihighlimits right", + "api-help-param-multi-max-simple": "Used to indicate the maximum number of values accepted for a multi-valued parameter when that value is not influenced by the user having apihighlimits right (otherwise {{msg-mw|api-help-param-multi-max}} is used).\n\nParameters:\n* $1 - Maximum value", "api-help-param-multi-all": "Used to indicate what string can be used to specify all possible values of a multi-valued parameter. \n\nParameters:\n* $1 - String to specify all possible values of the parameter", "api-help-param-default": "Used to display the default value for an API parameter\n\nParameters:\n* $1 - Default value\n\nSee also:\n* {{msg-mw|api-help-param-default-empty}}\n{{Identical|Default}}", "api-help-param-default-empty": "Used to display the default value for an API parameter when that default is an empty value\n\nSee also:\n* {{msg-mw|api-help-param-default}}", diff --git a/tests/phpunit/structure/ApiDocumentationTest.php b/tests/phpunit/structure/ApiStructureTest.php similarity index 69% rename from tests/phpunit/structure/ApiDocumentationTest.php rename to tests/phpunit/structure/ApiStructureTest.php index 83585af52e..7912f97902 100644 --- a/tests/phpunit/structure/ApiDocumentationTest.php +++ b/tests/phpunit/structure/ApiStructureTest.php @@ -3,14 +3,15 @@ use Wikimedia\TestingAccessWrapper; /** - * Checks that all API modules, core and extensions, have documentation i18n messages - * - * It won't catch everything since i18n messages can vary based on the wiki - * configuration, but it should catch many cases for forgotten i18n. + * Checks that all API modules, core and extensions, conform to the conventions: + * - have documentation i18n messages (the test won't catch everything since + * i18n messages can vary based on the wiki configuration, but it should + * catch many cases for forgotten i18n) + * - do not have inconsistencies in the parameter definitions * * @group API */ -class ApiDocumentationTest extends MediaWikiTestCase { +class ApiStructureTest extends MediaWikiTestCase { /** @var ApiMain */ private static $main; @@ -36,7 +37,7 @@ class ApiDocumentationTest extends MediaWikiTestCase { self::$main = new ApiMain( RequestContext::getMain() ); self::$main->getContext()->setLanguage( 'en' ); self::$main->getContext()->setTitle( - Title::makeTitle( NS_SPECIAL, 'Badtitle/dummy title for ApiDocumentationTest' ) + Title::makeTitle( NS_SPECIAL, 'Badtitle/dummy title for ApiStructureTest' ) ); } return self::$main; @@ -165,6 +166,58 @@ class ApiDocumentationTest extends MediaWikiTestCase { return $ret; } + /** + * @dataProvider provideParameterConsistency + * @param string $path + */ + public function testParameterConsistency( $path ) { + $main = self::getMain(); + $module = TestingAccessWrapper::newFromObject( $main->getModuleFromPath( $path ) ); + + $paramsPlain = $module->getFinalParams(); + $paramsForHelp = $module->getFinalParams( ApiBase::GET_VALUES_FOR_HELP ); + + // avoid warnings about empty tests when no parameter needs to be checked + $this->assertTrue( true ); + + foreach ( [ $paramsPlain, $paramsForHelp ] as $params ) { + foreach ( $params as $param => $config ) { + if ( + isset( $config[ApiBase::PARAM_ISMULTI_LIMIT1] ) + || isset( $config[ApiBase::PARAM_ISMULTI_LIMIT2] ) + ) { + $this->assertTrue( !empty( $config[ApiBase::PARAM_ISMULTI] ), $param + . ': PARAM_ISMULTI_LIMIT* only makes sense when PARAM_ISMULTI is true' ); + $this->assertTrue( isset( $config[ApiBase::PARAM_ISMULTI_LIMIT1] ) + && isset( $config[ApiBase::PARAM_ISMULTI_LIMIT2] ), $param + . ': PARAM_ISMULTI_LIMIT1 and PARAM_ISMULTI_LIMIT2 must be used together' ); + $this->assertType( 'int', $config[ApiBase::PARAM_ISMULTI_LIMIT1], $param + . 'PARAM_ISMULTI_LIMIT1 must be an integer' ); + $this->assertType( 'int', $config[ApiBase::PARAM_ISMULTI_LIMIT2], $param + . 'PARAM_ISMULTI_LIMIT2 must be an integer' ); + $this->assertGreaterThanOrEqual( $config[ApiBase::PARAM_ISMULTI_LIMIT1], + $config[ApiBase::PARAM_ISMULTI_LIMIT2], $param + . 'PARAM_ISMULTI limit cannot be smaller for users with apihighlimits rights' ); + } + } + } + } + + /** + * @return array List of API module paths to test + */ + public static function provideParameterConsistency() { + $main = self::getMain(); + $paths = self::getSubModulePaths( $main->getModuleManager() ); + array_unshift( $paths, $main->getModulePath() ); + + $ret = []; + foreach ( $paths as $path ) { + $ret[] = [ $path ]; + } + return $ret; + } + /** * Return paths of all submodules in an ApiModuleManager, recursively * @param ApiModuleManager $manager -- 2.20.1