Skip to content

Commit

Permalink
Infer URL params more accurately when there's a field binding
Browse files Browse the repository at this point in the history
  • Loading branch information
@shalvah
shalvah committed Jul 5, 2021
1 parent 77c27be commit ce6be7c
Show file tree
Hide file tree
Showing 3 changed files with 75 additions and 24 deletions.
51 changes: 40 additions & 11 deletions camel/Extraction/ExtractedEndpointData.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,25 +139,44 @@ public function normalizeResourceParamName(string $uri, Route $route): string
$params = [];
preg_match_all('#\{(\w+?)}#', $uri, $params);

$foundResourceParam = false;
foreach ($params[1] as $param) {
$pluralParam = Str::plural($param);
$resourceRouteNames = ["$pluralParam.show", "$pluralParam.update", "$pluralParam.destroy"];

if (Str::contains($route->action['as'] ?? '', $resourceRouteNames)) {
$search = sprintf("%s/{%s}", $pluralParam, $param);
$resourceRouteNames = [
".index", ".show", ".update", ".destroy",
];

if (Str::endsWith($route->action['as'] ?? '', $resourceRouteNames)) {
// Note that resource routes can be nested eg users.posts.show
$pluralResources = explode('.', $route->action['as']);
array_pop($pluralResources);

$foundResourceParam = false;
foreach (array_reverse($pluralResources) as $pluralResource) {
$singularResource = Str::singular($pluralResource);
$search = ["{$pluralResource}/{{$singularResource}}", "{$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 first resource param should be {id}
$replace = "$pluralParam/{id}";
// Only the last resource param should be {id}
$replace = ["$pluralResource/{{$binding}}", "$pluralResource/{{$binding}?}"];
$foundResourceParam = true;
} else {
// Subsequent ones should be {<param>_id}
$replace = sprintf("%s/{%s}", $pluralParam, $param.'_id');
// Earlier ones should be {<param>_id}
$replace = ["{$pluralResource}/{{$singularResource}_{$binding}}", "{$pluralResource}/{{$singularResource}_{$binding}?}"];
}
$uri = str_replace($search, $replace, $uri);
}
}

foreach ($params[1] as $param) {
// For non-resource parameters, if there's a field binding, replace that too:
if ($binding = static::getFieldBindingForUrlParam($route, $param)) {
$search = ["{{$param}}", "{{$param}?}"];
$replace = ["{{$param}_{$binding}}", "{{$param}_{$binding}?}"];
$uri = str_replace($search, $replace, $uri);
}
}

return $uri;
}

Expand All @@ -174,4 +193,14 @@ public function forSerialisation()
'route', 'controller', 'method', 'auth',
);
}

public static function getFieldBindingForUrlParam(Route $route, string $paramName, string $default = null): ?string
{
$binding = null;
// Was added in Laravel 7.x
if (method_exists($route, 'bindingFieldFor')) {
$binding = $route->bindingFieldFor($paramName);
}
return $binding ?: $default;
}
}
8 changes: 2 additions & 6 deletions src/Extracting/Strategies/UrlParameters/GetFromLaravelAPI.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,8 @@ public function __invoke(ExtractedEndpointData $endpointData, array $routeRules)
$optional = Str::endsWith($match, '?');
$name = rtrim($match, '?');

// In case of /users/{user:id}, make the param {user}, but
$binding = null;
// Was added in Laravel 7.x
if (method_exists($endpointData->route, 'bindingFieldFor')) {
$binding = $endpointData->route->bindingFieldFor($name);
}
// In case of /users/{user:id}, make the param {user_id}
$binding = ExtractedEndpointData::getFieldBindingForUrlParam($endpointData->route, $name);
$parameters[$name] = [
'name' => $name,
'description' => $this->inferUrlParamDescription($endpointData->uri, $binding ?: $name, $binding ? $name : null),
Expand Down
40 changes: 33 additions & 7 deletions tests/GenerateDocumentationTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ public function supports_partial_resource_controller()
{
RouteFacade::resource('/api/users', TestPartialResourceController::class);

config(['scribe.routes.0.prefixes' => ['api/*']]);
config(['scribe.routes.0.match.prefixes' => ['api/*']]);

$output = $this->artisan('scribe:generate');

Expand Down Expand Up @@ -273,7 +273,7 @@ public function can_parse_utf8_response()
{
RouteFacade::get('/api/utf8', [TestController::class, 'withUtf8ResponseTag']);

config(['scribe.routes.0.prefixes' => ['api/*']]);
config(['scribe.routes.0.match.prefixes' => ['api/*']]);
$this->artisan('scribe:generate');

$generatedHtml = file_get_contents('public/docs/index.html');
Expand All @@ -288,7 +288,7 @@ public function sorts_group_naturally()
RouteFacade::get('/api/action2', TestGroupController::class . '@action2');
RouteFacade::get('/api/action10', TestGroupController::class . '@action10');

config(['scribe.routes.0.prefixes' => ['api/*']]);
config(['scribe.routes.0.match.prefixes' => ['api/*']]);
$this->artisan('scribe:generate');

$this->assertFileExists(__DIR__ . '/../.scribe/endpoints/0.yaml');
Expand All @@ -304,7 +304,7 @@ public function can_customise_static_output_path()
{
RouteFacade::get('/api/action1', TestGroupController::class . '@action1');

config(['scribe.routes.0.prefixes' => ['*']]);
config(['scribe.routes.0.match.prefixes' => ['*']]);
config(['scribe.static.output_path' => 'static/docs']);
$this->artisan('scribe:generate');

Expand All @@ -318,7 +318,7 @@ public function will_not_overwrite_manually_modified_content_unless_force_flag_i
{
RouteFacade::get('/api/action1', [TestGroupController::class, 'action1']);
RouteFacade::get('/api/action1b', [TestGroupController::class, 'action1b']);
config(['scribe.routes.0.prefixes' => ['api/*']]);
config(['scribe.routes.0.match.prefixes' => ['api/*']]);

$this->artisan('scribe:generate');

Expand Down Expand Up @@ -357,10 +357,32 @@ public function will_not_overwrite_manually_modified_content_unless_force_flag_i
$this->assertStringNotContainsString('Some other useful stuff.', file_get_contents($authFilePath));
}

/** @test */
public function generates_correct_url_params_from_resource_routes_and_field_bindings()
{
RouteFacade::prefix('providers/{provider:slug}')->group(function () {
RouteFacade::resource('users.addresses', TestPartialResourceController::class)->parameters([
'addresses' => 'address:uuid',
]);
});
config(['scribe.routes.0.match.prefixes' => ['*']]);
config(['scribe.openapi.enabled' => false]);
config(['scribe.postman.enabled' => false]);

$this->artisan('scribe:generate');

$groupA = Yaml::parseFile('.scribe/endpoints/0.yaml');
$this->assertEquals('providers/{provider_slug}/users/{user_id}/addresses', $groupA['endpoints'][0]['uri']);
$groupB = Yaml::parseFile('.scribe/endpoints/1.yaml');
$this->assertEquals('providers/{provider_slug}/users/{user_id}/addresses/{uuid}', $groupB['endpoints'][0]['uri']);
}

/** @test */
public function will_not_extract_if_noExtraction_flag_is_set()
{
config(['scribe.routes.0.exclude' => ['*']]);
config(['scribe.openapi.enabled' => false]);
config(['scribe.postman.enabled' => false]);
Utils::copyDirectory(__DIR__.'/Fixtures/.scribe', '.scribe');

$output = $this->artisan('scribe:generate', ['--no-extraction' => true]);
Expand All @@ -383,7 +405,9 @@ public function merges_user_defined_endpoints()
{
RouteFacade::get('/api/action1', [TestGroupController::class, 'action1']);
RouteFacade::get('/api/action2', [TestGroupController::class, 'action2']);
config(['scribe.routes.0.prefixes' => ['api/*']]);
config(['scribe.routes.0.match.prefixes' => ['api/*']]);
config(['scribe.openapi.enabled' => false]);
config(['scribe.postman.enabled' => false]);
if (!is_dir('.scribe/endpoints'))
mkdir('.scribe/endpoints', 0777, true);
copy(__DIR__ . '/Fixtures/custom.0.yaml', '.scribe/endpoints/custom.0.yaml');
Expand Down Expand Up @@ -412,7 +436,9 @@ public function respects_endpoints_and_group_sort_order()
RouteFacade::get('/api/action1', [TestGroupController::class, 'action1']);
RouteFacade::get('/api/action1b', [TestGroupController::class, 'action1b']);
RouteFacade::get('/api/action2', [TestGroupController::class, 'action2']);
config(['scribe.routes.0.prefixes' => ['api/*']]);
config(['scribe.routes.0.match.prefixes' => ['api/*']]);
config(['scribe.openapi.enabled' => false]);
config(['scribe.postman.enabled' => false]);

$this->artisan('scribe:generate');

Expand Down

0 comments on commit ce6be7c

Please sign in to comment.