From f9bff064887cfb382a9ab27fd47fb8c58c8555ed Mon Sep 17 00:00:00 2001 From: Khaled Farhat Date: Sun, 26 Jun 2022 23:20:12 +0300 Subject: [PATCH 1/3] Fixed a bug with the default URL parameters --- camel/Extraction/ExtractedEndpointData.php | 63 +++++++++++-------- .../UrlParameters/GetFromLaravelAPI.php | 45 +++++++++++-- tests/GenerateDocumentationTest.php | 4 +- .../UrlParameters/GetFromLaravelAPITest.php | 4 +- tests/Unit/ExtractedEndpointDataTest.php | 16 ++++- 5 files changed, 95 insertions(+), 37 deletions(-) diff --git a/camel/Extraction/ExtractedEndpointData.php b/camel/Extraction/ExtractedEndpointData.php index 4938fd85..adf6287f 100644 --- a/camel/Extraction/ExtractedEndpointData.php +++ b/camel/Extraction/ExtractedEndpointData.php @@ -137,7 +137,7 @@ public function normalizeResourceParamName(string $uri, Route $route): string preg_match_all('#\{(\w+?)}#', $uri, $params); $resourceRouteNames = [ - ".index", ".show", ".update", ".destroy", + ".index", ".store", ".show", ".update", ".destroy", ]; if (Str::endsWith($route->action['as'] ?? '', $resourceRouteNames)) { @@ -145,36 +145,21 @@ public function normalizeResourceParamName(string $uri, Route $route): string $pluralResources = explode('.', $route->action['as']); array_pop($pluralResources); - $foundResourceParam = false; + $isLastResource = true; foreach (array_reverse($pluralResources) as $pluralResource) { $singularResource = Str::singular($pluralResource); $singularResourceParam = str_replace('-', '_', $singularResource); - $search = [ - "{$pluralResource}/{{$singularResourceParam}}", - "{$pluralResource}/{{$singularResource}}", - "{$pluralResource}/{{$singularResourceParam}?}", - "{$pluralResource}/{{$singularResource}?}" - ]; - - // We'll replace with {id} by default, but if the user is using a different key, - // like /users/{user:uuid}, use that instead - $binding = static::getFieldBindingForUrlParam($route, $singularResource, 'id'); - - if (!$foundResourceParam) { - // Only the last resource param should be {id} - $replace = ["$pluralResource/{{$binding}}", "$pluralResource/{{$binding}?}"]; - $foundResourceParam = true; - } else { - // Earlier ones should be {_id} - $replace = [ - "{$pluralResource}/{{$singularResource}_{$binding}}", - "{$pluralResource}/{{$singularResourceParam}_{$binding}}", - "{$pluralResource}/{{$singularResource}_{$binding}?}", - "{$pluralResource}/{{$singularResourceParam}_{$binding}?}" - ]; + $binding = static::getFieldBindingForUrlParam($route, $singularResourceParam); + + // If there is a field binding, like /users/{user:uuid} + if (!is_null($binding)) { + // If the resource was the last, replace {singularResourceParam} with {binding} + // otherwise, replace {singularResourceParam} with {singularResourceParam_binding} + $uri = self::bindUrlParam($singularResourceParam, $binding, $isLastResource, $uri); } - $uri = str_replace($search, $replace, $uri); + + $isLastResource = false; } } @@ -216,4 +201,30 @@ public static function getFieldBindingForUrlParam(Route $route, string $paramNam return $binding ?: $default; } + + public static function bindUrlParam(string $singularResourceParam, string $binding, bool $isLastResource, string $uri): string + { + $singularResource = str_replace('_', '-', $singularResourceParam); + $pluralResource = Str::plural($singularResource); + + $search = [ + "{$pluralResource}/{{$singularResourceParam}}", + "{$pluralResource}/{{$singularResource}}", + "{$pluralResource}/{{$singularResourceParam}?}", + "{$pluralResource}/{{$singularResource}?}" + ]; + + if ($isLastResource === true) { + $replace = ["$pluralResource/{{$binding}}", "$pluralResource/{{$binding}?}"]; + } else { + $replace = [ + "{$pluralResource}/{{$singularResource}_{$binding}}", + "{$pluralResource}/{{$singularResourceParam}_{$binding}}", + "{$pluralResource}/{{$singularResource}_{$binding}?}", + "{$pluralResource}/{{$singularResourceParam}_{$binding}?}" + ]; + } + + return str_replace($search, $replace, $uri); + } } diff --git a/src/Extracting/Strategies/UrlParameters/GetFromLaravelAPI.php b/src/Extracting/Strategies/UrlParameters/GetFromLaravelAPI.php index ba379564..5a4785da 100644 --- a/src/Extracting/Strategies/UrlParameters/GetFromLaravelAPI.php +++ b/src/Extracting/Strategies/UrlParameters/GetFromLaravelAPI.php @@ -43,8 +43,16 @@ public function __invoke(ExtractedEndpointData $endpointData, array $routeRules) // and (User $user) model is typehinted on method // If User model has an int primary key, {user} param should be an int + // Also, bind url parameters for any bound models + // Eg Suppose route is /posts/{post}, + // and (Post $post) model is typehinted on method + // If Post model has a slug routeKeyName, use {post_slug} parameter instead of {post} + $methodArguments = $endpointData->method->getParameters(); - foreach ($methodArguments as $argument) { + $currentArgumentPosition = 0; // to check if we are processing the last argument + foreach (array_reverse($methodArguments) as $argument) { + ++$currentArgumentPosition; + $argumentType = $argument->getType(); // If there's no typehint, continue if (!$argumentType) { @@ -54,10 +62,31 @@ public function __invoke(ExtractedEndpointData $endpointData, array $routeRules) $argumentClassName = $argumentType->getName(); $argumentInstance = new $argumentClassName; if ($argumentInstance instanceof Model) { + $routeKeyName = $argumentInstance->getRouteKeyName(); + if (isset($parameters[$argument->getName()])) { - $paramName = $argument->getName(); - } else if (isset($parameters['id'])) { - $paramName = 'id'; + $oldParamName = $argument->getName(); + + $isLastResource = ($currentArgumentPosition === 1); + $endpointData->uri = $endpointData->bindUrlParam($oldParamName, $routeKeyName, $isLastResource, $endpointData->uri); + + // Find the new paramName + if ($isLastResource === true) { + // We are processing the last argument, use {routeKeyName} as field binding + $paramName = $routeKeyName; + } else { + // We are processing an earlier argument, use {argumentName_routeKeyName} as field binding + $paramName = $argument->getName() . '_' . $routeKeyName; + } + + // Rename the parameter from oldParamName to paramName + $parameters[$paramName] = $parameters[$oldParamName]; + $parameters[$paramName]['name'] = $paramName; + unset($parameters[$oldParamName]); + + $parameters[$paramName]['description'] = $this->inferUrlParamDescription($endpointData->uri, $paramName, $oldParamName); + } else if (isset($parameters[$routeKeyName])) { + $paramName = $routeKeyName; } else { continue; } @@ -131,6 +160,14 @@ protected function inferUrlParamDescription(string $url, string $paramName, stri // If $url is sth like /something/{user_id}, return "The ID of the user." $parts = explode("_", $paramName); return "The ID of the $parts[0]."; + } else if (Str::contains($paramName, '_')) { + $parts = explode("_", $paramName); + + $field = $parts; + unset($field[0]); + $field = implode('_', $field); + + return "The $field of the $parts[0]."; } else if ($paramName && $originalBindingName) { // A case like /posts/{post:slug} -> The slug of the post return "The $paramName of the $originalBindingName."; diff --git a/tests/GenerateDocumentationTest.php b/tests/GenerateDocumentationTest.php index 0335f924..2247029a 100644 --- a/tests/GenerateDocumentationTest.php +++ b/tests/GenerateDocumentationTest.php @@ -409,9 +409,9 @@ public function generates_correct_url_params_from_resource_routes_and_field_bind $this->artisan('scribe:generate'); $groupA = Yaml::parseFile('.scribe/endpoints/00.yaml'); - $this->assertEquals('providers/{provider_slug}/users/{user_id}/addresses', $groupA['endpoints'][0]['uri']); + $this->assertEquals('providers/{provider_slug}/users/{user}/addresses', $groupA['endpoints'][0]['uri']); $groupB = Yaml::parseFile('.scribe/endpoints/01.yaml'); - $this->assertEquals('providers/{provider_slug}/users/{user_id}/addresses/{uuid}', $groupB['endpoints'][0]['uri']); + $this->assertEquals('providers/{provider_slug}/users/{user}/addresses/{uuid}', $groupB['endpoints'][0]['uri']); } /** @test */ diff --git a/tests/Strategies/UrlParameters/GetFromLaravelAPITest.php b/tests/Strategies/UrlParameters/GetFromLaravelAPITest.php index 88062afd..2bdfc614 100644 --- a/tests/Strategies/UrlParameters/GetFromLaravelAPITest.php +++ b/tests/Strategies/UrlParameters/GetFromLaravelAPITest.php @@ -145,10 +145,10 @@ public function __construct(array $parameters = []) $results = $strategy($endpoint, []); $this->assertArraySubset([ - "name" => "user", + "name" => "id", "description" => "The ID of the user.", "required" => true, "type" => "integer", - ], $results['user']); + ], $results['id']); } } diff --git a/tests/Unit/ExtractedEndpointDataTest.php b/tests/Unit/ExtractedEndpointDataTest.php index 31829959..ef753c03 100644 --- a/tests/Unit/ExtractedEndpointDataTest.php +++ b/tests/Unit/ExtractedEndpointDataTest.php @@ -14,7 +14,10 @@ class ExtractedEndpointDataTest extends BaseLaravelTest public function will_normalize_resource_url_params() { Route::apiResource('things', TestController::class) - ->only('show'); + ->only('show') + ->parameters([ + 'things' => 'thing:id', + ]); $routeRules[0]['match'] = ['prefixes' => '*', 'domains' => '*']; $matcher = new RouteMatcher(); @@ -32,7 +35,11 @@ public function will_normalize_resource_url_params() } Route::apiResource('things.otherthings', TestController::class) - ->only( 'destroy'); + ->only( 'destroy') + ->parameters([ + 'things' => 'thing:id', + 'otherthings' => 'otherthing:id', + ]); $routeRules[0]['match'] = ['prefixes' => '*/otherthings/*', 'domains' => '*']; $matchedRoutes = $matcher->getRoutes($routeRules); @@ -52,7 +59,10 @@ public function will_normalize_resource_url_params() public function will_normalize_resource_url_params_with_hyphens() { Route::apiResource('audio-things', TestController::class) - ->only('show'); + ->only('show') + ->parameters([ + 'audio-things' => 'audio_thing:id', + ]); $routeRules[0]['match'] = ['prefixes' => '*', 'domains' => '*']; $matcher = new RouteMatcher(); From 0082a78eb57cd20c8839d3a69ce058883345d922 Mon Sep 17 00:00:00 2001 From: Khaled Farhat Date: Tue, 28 Jun 2022 13:34:35 +0300 Subject: [PATCH 2/3] Updated the tests to check the laravel support for field binding syntax --- tests/Unit/ExtractedEndpointDataTest.php | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/Unit/ExtractedEndpointDataTest.php b/tests/Unit/ExtractedEndpointDataTest.php index ef753c03..c473c81d 100644 --- a/tests/Unit/ExtractedEndpointDataTest.php +++ b/tests/Unit/ExtractedEndpointDataTest.php @@ -13,6 +13,12 @@ class ExtractedEndpointDataTest extends BaseLaravelTest /** @test */ public function will_normalize_resource_url_params() { + if (version_compare($this->app->version(), '7.0.0', '<')) { + $this->markTestSkipped("Laravel < 7.x doesn't support field binding syntax."); + + return; + } + Route::apiResource('things', TestController::class) ->only('show') ->parameters([ @@ -58,6 +64,12 @@ public function will_normalize_resource_url_params() /** @test */ public function will_normalize_resource_url_params_with_hyphens() { + if (version_compare($this->app->version(), '7.0.0', '<')) { + $this->markTestSkipped("Laravel < 7.x doesn't support field binding syntax."); + + return; + } + Route::apiResource('audio-things', TestController::class) ->only('show') ->parameters([ From e186b1ddc68a7b0f8131cb13a65169d6923a0f7d Mon Sep 17 00:00:00 2001 From: Khaled Farhat Date: Thu, 30 Jun 2022 19:42:15 +0300 Subject: [PATCH 3/3] Created a test case to test generating url parameters using bound models --- tests/GenerateDocumentationTest.php | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/GenerateDocumentationTest.php b/tests/GenerateDocumentationTest.php index 2247029a..ddb6dbd2 100644 --- a/tests/GenerateDocumentationTest.php +++ b/tests/GenerateDocumentationTest.php @@ -414,6 +414,19 @@ public function generates_correct_url_params_from_resource_routes_and_field_bind $this->assertEquals('providers/{provider_slug}/users/{user}/addresses/{uuid}', $groupB['endpoints'][0]['uri']); } + /** @test */ + public function generates_correct_url_params_using_bound_models() + { + RouteFacade::get('users/{user}', [TestController::class, 'withInjectedModel']); + config(['scribe.routes.0.match.prefixes' => ['*']]); + config(['scribe.routes.0.apply.response_calls.methods' => []]); + + $this->artisan('scribe:generate'); + + $group = Yaml::parseFile('.scribe/endpoints/00.yaml'); + $this->assertEquals('users/{id}', $group['endpoints'][0]['uri']); + } + /** @test */ public function will_generate_without_extracting_if_noExtraction_flag_is_set() {