From 16729b7f02e1c374cf0c9d2cf0981ca08ac9085e Mon Sep 17 00:00:00 2001 From: Petr Skoda Date: Wed, 10 May 2023 10:27:31 +0200 Subject: [PATCH] MDL-78192 external: allow nullable non-scalar params and results --- lib/external/classes/external_api.php | 6 ++ lib/external/classes/external_description.php | 7 +- .../classes/external_function_parameters.php | 2 +- .../classes/external_multiple_structure.php | 6 +- .../classes/external_single_structure.php | 6 +- lib/external/classes/external_value.php | 6 +- lib/external/tests/external_api_test.php | 96 ++++++++++++++++++- 7 files changed, 117 insertions(+), 12 deletions(-) diff --git a/lib/external/classes/external_api.php b/lib/external/classes/external_api.php index 5bd51bd93c7..c770a80ab71 100644 --- a/lib/external/classes/external_api.php +++ b/lib/external/classes/external_api.php @@ -312,6 +312,9 @@ class external_api { * @since Moodle 2.0 */ public static function validate_parameters(external_description $description, $params) { + if ($params === null && $description->allownull == NULL_ALLOWED) { + return null; + } if ($description instanceof external_value) { if (is_array($params) || is_object($params)) { throw new invalid_parameter_exception('Scalar type expected, array or object received.'); @@ -398,6 +401,9 @@ class external_api { * @since Moodle 2.0 */ public static function clean_returnvalue(external_description $description, $response) { + if ($response === null && $description->allownull == NULL_ALLOWED) { + return null; + } if ($description instanceof external_value) { if (is_array($response) || is_object($response)) { throw new invalid_response_exception('Scalar type expected, array or object received.'); diff --git a/lib/external/classes/external_description.php b/lib/external/classes/external_description.php index ef44a87acf7..6deea883fb5 100644 --- a/lib/external/classes/external_description.php +++ b/lib/external/classes/external_description.php @@ -33,14 +33,18 @@ abstract class external_description { /** @var mixed Default value */ public $default; + /** @var bool Allow null values */ + public $allownull; + /** * Contructor. * * @param string $desc Description of element * @param int $required Whether the element value is required. Valid values are VALUE_DEFAULT, VALUE_REQUIRED, VALUE_OPTIONAL. * @param mixed $default The default value + * @param bool $allownull Allow null value */ - public function __construct($desc, $required, $default) { + public function __construct($desc, $required, $default, $allownull = NULL_NOTALLOWED) { if (!in_array($required, [VALUE_DEFAULT, VALUE_REQUIRED, VALUE_OPTIONAL], true)) { $requiredstr = $required; if (is_array($required)) { @@ -52,5 +56,6 @@ abstract class external_description { $this->desc = $desc; $this->required = $required; $this->default = $default; + $this->allownull = (bool)$allownull; } } diff --git a/lib/external/classes/external_function_parameters.php b/lib/external/classes/external_function_parameters.php index 73ec95d969f..085e91d3a8d 100644 --- a/lib/external/classes/external_function_parameters.php +++ b/lib/external/classes/external_function_parameters.php @@ -50,6 +50,6 @@ class external_function_parameters extends external_single_structure { } } } - parent::__construct($keys, $desc, $required, $default); + parent::__construct($keys, $desc, $required, $default, NULL_NOT_ALLOWED); } } diff --git a/lib/external/classes/external_multiple_structure.php b/lib/external/classes/external_multiple_structure.php index 3a16c8a32b0..8362b818e0a 100644 --- a/lib/external/classes/external_multiple_structure.php +++ b/lib/external/classes/external_multiple_structure.php @@ -35,14 +35,16 @@ class external_multiple_structure extends external_description { * @param string $desc * @param int $required * @param array $default + * @param bool $allownull */ public function __construct( external_description $content, $desc = '', $required = VALUE_REQUIRED, - $default = null + $default = null, + $allownull = NULL_NOT_ALLOWED ) { - parent::__construct($desc, $required, $default); + parent::__construct($desc, $required, $default, $allownull); $this->content = $content; } } diff --git a/lib/external/classes/external_single_structure.php b/lib/external/classes/external_single_structure.php index 52231aaff4d..7aa7bad0a89 100644 --- a/lib/external/classes/external_single_structure.php +++ b/lib/external/classes/external_single_structure.php @@ -35,14 +35,16 @@ class external_single_structure extends external_description { * @param string $desc * @param int $required * @param array $default + * @param bool $allownull */ public function __construct( array $keys, $desc = '', $required = VALUE_REQUIRED, - $default = null + $default = null, + $allownull = NULL_NOT_ALLOWED ) { - parent::__construct($desc, $required, $default); + parent::__construct($desc, $required, $default, $allownull); $this->keys = $keys; } } diff --git a/lib/external/classes/external_value.php b/lib/external/classes/external_value.php index 45aef862b03..037441e29dd 100644 --- a/lib/external/classes/external_value.php +++ b/lib/external/classes/external_value.php @@ -28,9 +28,6 @@ class external_value extends external_description { /** @var mixed Value type PARAM_XX */ public $type; - /** @var bool Allow null values */ - public $allownull; - /** * Constructor for the external_value class. * @@ -47,8 +44,7 @@ class external_value extends external_description { $default = null, $allownull = NULL_ALLOWED ) { - parent::__construct($desc, $required, $default); + parent::__construct($desc, $required, $default, $allownull); $this->type = $type; - $this->allownull = $allownull; } } diff --git a/lib/external/tests/external_api_test.php b/lib/external/tests/external_api_test.php index 915831892ef..cc359cae229 100644 --- a/lib/external/tests/external_api_test.php +++ b/lib/external/tests/external_api_test.php @@ -82,6 +82,52 @@ class external_api_test extends \advanced_testcase { $this->assertSame('someid', key($result)); $this->assertSame(6, $result['someid']); $this->assertSame('aaa', $result['text']); + + // Missing required value (an exception is thrown). + $testdata = []; + try { + external_api::clean_returnvalue($description, $testdata); + $this->fail('Exception expected'); + } catch (\moodle_exception $ex) { + $this->assertInstanceOf(\invalid_response_exception::class, $ex); + $this->assertSame('Invalid response value detected (Error in response - ' + . 'Missing following required key in a single structure: text)', $ex->getMessage()); + } + + // Test nullable external_value may optionally return data. + $description = new external_function_parameters([ + 'value' => new external_value(PARAM_INT, '', VALUE_REQUIRED, null, NULL_ALLOWED) + ]); + $testdata = ['value' => null]; + $cleanedvalue = external_api::clean_returnvalue($description, $testdata); + $this->assertSame($testdata, $cleanedvalue); + $testdata = ['value' => 1]; + $cleanedvalue = external_api::clean_returnvalue($description, $testdata); + $this->assertSame($testdata, $cleanedvalue); + + // Test nullable external_single_structure may optionally return data. + $description = new external_function_parameters([ + 'value' => new external_single_structure(['value2' => new external_value(PARAM_INT)], + '', VALUE_REQUIRED, null, NULL_ALLOWED) + ]); + $testdata = ['value' => null]; + $cleanedvalue = external_api::clean_returnvalue($description, $testdata); + $this->assertSame($testdata, $cleanedvalue); + $testdata = ['value' => ['value2' => 1]]; + $cleanedvalue = external_api::clean_returnvalue($description, $testdata); + $this->assertSame($testdata, $cleanedvalue); + + // Test nullable external_multiple_structure may optionally return data. + $description = new external_function_parameters([ + 'value' => new external_multiple_structure( + new external_value(PARAM_INT), '', VALUE_REQUIRED, null, NULL_ALLOWED) + ]); + $testdata = ['value' => null]; + $cleanedvalue = external_api::clean_returnvalue($description, $testdata); + $this->assertSame($testdata, $cleanedvalue); + $testdata = ['value' => [1]]; + $cleanedvalue = external_api::clean_returnvalue($description, $testdata); + $this->assertSame($testdata, $cleanedvalue); } /** @@ -162,8 +208,56 @@ class external_api_test extends \advanced_testcase { $singlestructure['object'] = $object; $singlestructure['value2'] = 'Some text'; $testdata = [$singlestructure]; - $this->expectException('invalid_response_exception'); + try { + external_api::clean_returnvalue($returndesc, $testdata); + $this->fail('Exception expected'); + } catch (\moodle_exception $ex) { + $this->assertInstanceOf(\invalid_response_exception::class, $ex); + $this->assertSame('Invalid response value detected (object => Invalid response value detected ' + . '(Error in response - Missing following required key in a single structure: value1): Error in response - ' + . 'Missing following required key in a single structure: value1)', $ex->getMessage()); + } + + // Fail if no data provided when value required. + $testdata = null; + try { + external_api::clean_returnvalue($returndesc, $testdata); + $this->fail('Exception expected'); + } catch (\moodle_exception $ex) { + $this->assertInstanceOf(\invalid_response_exception::class, $ex); + $this->assertSame('Invalid response value detected (Only arrays accepted. The bad value is: \'\')', + $ex->getMessage()); + } + + // Test nullable external_multiple_structure may optionally return data. + $returndesc = new external_multiple_structure( + new external_value(PARAM_INT), + '', VALUE_REQUIRED, null, NULL_ALLOWED); + $testdata = null; $cleanedvalue = external_api::clean_returnvalue($returndesc, $testdata); + $this->assertSame($testdata, $cleanedvalue); + $testdata = [1]; + $cleanedvalue = external_api::clean_returnvalue($returndesc, $testdata); + $this->assertSame($testdata, $cleanedvalue); + + // Test nullable external_single_structure may optionally return data. + $returndesc = new external_single_structure(['value' => new external_value(PARAM_INT)], + '', VALUE_REQUIRED, null, NULL_ALLOWED); + $testdata = null; + $cleanedvalue = external_api::clean_returnvalue($returndesc, $testdata); + $this->assertSame($testdata, $cleanedvalue); + $testdata = ['value' => 1]; + $cleanedvalue = external_api::clean_returnvalue($returndesc, $testdata); + $this->assertSame($testdata, $cleanedvalue); + + // Test nullable external_value may optionally return data. + $returndesc = new external_value(PARAM_INT, '', VALUE_REQUIRED, null, NULL_ALLOWED); + $testdata = null; + $cleanedvalue = external_api::clean_returnvalue($returndesc, $testdata); + $this->assertSame($testdata, $cleanedvalue); + $testdata = 1; + $cleanedvalue = external_api::clean_returnvalue($returndesc, $testdata); + $this->assertSame($testdata, $cleanedvalue); } /**