From 44e4aedc03408617392cbaf84c0a228ddcb984a9 Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Thu, 23 Jan 2025 11:41:58 +0800 Subject: [PATCH 1/2] MDL-84300 router: Path params should be aware of unlimited captures Where a path contains an unlimited parameter, designated by `{name:.*}` or `{name:.*?}` it may be empty and therefore cannot be required. --- .../schema/parameters/path_parameter.php | 25 +++++++- lib/classes/router/schema/specification.php | 12 +++- lib/tests/router/request_validator_test.php | 28 --------- .../schema/parameters/path_parameter_test.php | 9 +++ .../router/schema/specification_test.php | 59 +++++++++++++++++-- 5 files changed, 94 insertions(+), 39 deletions(-) diff --git a/lib/classes/router/schema/parameters/path_parameter.php b/lib/classes/router/schema/parameters/path_parameter.php index 5cf7e2c9039..80d5f936bc7 100644 --- a/lib/classes/router/schema/parameters/path_parameter.php +++ b/lib/classes/router/schema/parameters/path_parameter.php @@ -103,11 +103,30 @@ class path_parameter extends parameter { public function is_required(route $route): bool { $path = $route->get_path(); - // Find the position of the parameter in the path. - $paramposition = strpos($path, '{' . $this->name . '}'); + // Find the parameter in the path. + // Search for `{value` with an optional : followed by anything except a closing `}`, and then a closing `}`. + // ~^(?.*\{{$this->name}(?:\:[^}]*)?\})~ + // ~ ~ => Delimiters + // ^(? ) => Named capture group + // .*\{ => Any character, any number of times, followed by { + // {$this->name} => The parameter name + // (?: )? => Optional non-capturing group + // \:[^}]* => : followed by anything except } + // \} => Closing } + // If the parameter is not found in the path, then it is not required. + $matchesfound = preg_match( + "~^(?.*\{{$this->name}(?:\:[^}]*)?\})~", + $path, + $matches, + ); + + if ($matchesfound === 0) { + // Parameter not found in the path. + return false; + } // If _any_ part of the path before the parameter contains a '[' character, then this _must_ be optional. // A required parameter cannot follow an optional parameter. - return !str_contains(substr($path, 0, $paramposition), '['); + return str_contains($matches['match'], '[') === false; } } diff --git a/lib/classes/router/schema/specification.php b/lib/classes/router/schema/specification.php index 0bd3488d31e..6d60f47aa8d 100644 --- a/lib/classes/router/schema/specification.php +++ b/lib/classes/router/schema/specification.php @@ -280,9 +280,17 @@ class specification implements // Note: We use this helper because OpenAPI does not support optional parameters. // Therefore we must handle that in Moodle, adding path variants with and without each optional parameter. $addpath = function (string $path) use ($route, $component) { - // Remove the optional parameters delimiters from the path. $path = str_replace( - ['[', ']'], + [ + // Remove the optional parameters delimiters from the path. + '[', + ']', + + // Remove the greedy and non-greedy unlimited delimters from the path too. + // These are a FastRoute feature not compatible with OpenAPI. + ':.*?', + ':.*', + ], '', $path, ); diff --git a/lib/tests/router/request_validator_test.php b/lib/tests/router/request_validator_test.php index 65cece60658..c03ff3a7546 100644 --- a/lib/tests/router/request_validator_test.php +++ b/lib/tests/router/request_validator_test.php @@ -90,34 +90,6 @@ final class request_validator_test extends route_testcase { ); } - /** - * When a defined pathtype is missing from the path. - */ - public function test_validate_request_missing_path_component(): void { - // A route with a parameter defined in the path, but no pathtype for it. - $route = new route( - path: '/example/123', - pathtypes: [ - new path_parameter( - name: 'required', - type: param::INT, - ), - ], - ); - - $request = $this->get_request_for_routed_route($route, '/example/123'); - - $validator = \core\di::get(request_validator::class); - $this->expectException(\coding_exception::class); - $this->expectExceptionMessageMatches('/Route.*has 0 arguments.* 1 pathtypes./'); - $result = $validator->validate_request($request); - - $this->assertInstanceOf( - ServerRequestInterface::class, - $result, - ); - } - /** * When a pathtype fails to validate, it will result in an HttpNotFoundException. */ diff --git a/lib/tests/router/schema/parameters/path_parameter_test.php b/lib/tests/router/schema/parameters/path_parameter_test.php index 9af500c7cee..76e1fe70ad7 100644 --- a/lib/tests/router/schema/parameters/path_parameter_test.php +++ b/lib/tests/router/schema/parameters/path_parameter_test.php @@ -64,9 +64,18 @@ final class path_parameter_test extends route_testcase { */ public static function is_required_provider(): array { return [ + ['/is/not/found', false], + ['/is/not/found/{values}', false], + ['/is/not/found/{values:.*}', false], + ['/is/not/found/{values:.*?}', false], ['/is/required/{value}', true], + ['/is/required/{value:.*}', true], + ['/is/required/{value:.*?}/example', true], ['/is/optional/[{value}]', false], ['/is/[optional/[{value}]]', false], + ['/is/[optional/[{value:.*}]]', false], + ['/is/[optional/[{value:.*?}/example]]', false], + ['/is/required/{value}[/example]', true], ]; } diff --git a/lib/tests/router/schema/specification_test.php b/lib/tests/router/schema/specification_test.php index f14477b5088..06592071dd4 100644 --- a/lib/tests/router/schema/specification_test.php +++ b/lib/tests/router/schema/specification_test.php @@ -139,13 +139,25 @@ final class specification_test extends route_testcase { $this->assertObjectHasProperty('/core/example/path/with/{option}', $schema->paths); } - public function test_add_path_with_options(): void { + /** + * Test add_path with optional parameters. + * + * @param string $component + * @param string $path + * @param array $expectedpaths + * @dataProvider add_path_with_options_provider + */ + public function test_add_path_with_options( + string $component, + string $path, + array $expectedpaths, + ): void { $spec = new specification(); $spec->add_path( - 'core', + $component, new route( - path: '/example/path/with[/{optional}][/{extras}]', + path: $path, pathtypes: [ new path_parameter(name: 'optional', type: param::INT), new path_parameter(name: 'extras', type: param::INT), @@ -154,9 +166,44 @@ final class specification_test extends route_testcase { ); $schema = $spec->get_schema(); - $this->assertObjectHasProperty('/core/example/path/with', $schema->paths); - $this->assertObjectHasProperty('/core/example/path/with/{optional}', $schema->paths); - $this->assertObjectHasProperty('/core/example/path/with/{optional}/{extras}', $schema->paths); + foreach ($expectedpaths as $expectedpath) { + $this->assertObjectHasProperty($expectedpath, $schema->paths); + } + } + + /** + * Data provider for add_path with optional parameters. + * + * @return \Iterator + */ + public static function add_path_with_options_provider(): \Iterator { + yield 'With options' => [ + 'component' => 'core', + 'path' => '/example/path/with[/{optional}][/{extras}]', + 'expectedpaths' => [ + '/core/example/path/with', + '/core/example/path/with/{optional}', + '/core/example/path/with/{optional}/{extras}', + ], + ]; + yield 'With greedy unlimited options' => [ + 'component' => 'core', + 'path' => '/example/path/with[/{optional}][/{extras:.*}]', + 'expectedpaths' => [ + '/core/example/path/with', + '/core/example/path/with/{optional}', + '/core/example/path/with/{optional}/{extras}', + ], + ]; + yield 'With non-greedy unlimited options' => [ + 'component' => 'core', + 'path' => '/example/path/with[/{optional}][/{extras:.*?}]', + 'expectedpaths' => [ + '/core/example/path/with', + '/core/example/path/with/{optional}', + '/core/example/path/with/{optional}/{extras}', + ], + ]; } public function test_add_parameter(): void { From 49b0ef223fa6a54046cd89ef94937a64165cd25d Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Fri, 24 Jan 2025 09:48:21 +0800 Subject: [PATCH 2/2] MDL-84300 core: Support nested templates in routed API --- lib/classes/route/api/templates.php | 2 +- lib/tests/route/api/templates_test.php | 12 +++++++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/lib/classes/route/api/templates.php b/lib/classes/route/api/templates.php index 867c145e026..171baa96b20 100644 --- a/lib/classes/route/api/templates.php +++ b/lib/classes/route/api/templates.php @@ -44,7 +44,7 @@ class templates { * @return payload_response */ #[route( - path: '/templates/{themename}/{component}/{identifier}', + path: '/templates/{themename}/{component}/{identifier:.*}', method: ['GET'], title: 'Fetch a single template', description: 'Fetch a single template for a component in a theme', diff --git a/lib/tests/route/api/templates_test.php b/lib/tests/route/api/templates_test.php index 4e63e7f14ec..eefbcdddbe2 100644 --- a/lib/tests/route/api/templates_test.php +++ b/lib/tests/route/api/templates_test.php @@ -64,7 +64,17 @@ final class templates_test extends route_testcase { ['core/modal'], [], ], - 'foo' => [ + 'fetch nested template' => [ + 'boost/core/local/modal/alert', + [ + 'core/local/modal/alert', + 'core/modal', + ], + [ + 'moodle/ok', + ], + ], + 'multiple templates' => [ 'boost/core/notification', [ 'core/notification',