Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed a bug in binding the URL parameters for API resource routes #489

Closed
wants to merge 3 commits into from
Closed

Fixed a bug in binding the URL parameters for API resource routes #489

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f9bff06
Fixed a bug with the default URL parameters
Khaled-Farhat Jun 26, 2022
0082a78
Updated the tests to check the laravel support for field binding syntax
Khaled-Farhat Jun 28, 2022
e186b1d
Created a test case to test generating url parameters using bound models
Khaled-Farhat Jun 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 37 additions & 26 deletions camel/Extraction/ExtractedEndpointData.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,44 +137,29 @@ 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)) {
// Note that resource routes can be nested eg users.posts.show
$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 {<param>_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;
}
}

Expand Down Expand Up @@ -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);
}
}
45 changes: 41 additions & 4 deletions src/Extracting/Strategies/UrlParameters/GetFromLaravelAPI.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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;
}
Expand Down Expand Up @@ -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.";
Expand Down
17 changes: 15 additions & 2 deletions tests/GenerateDocumentationTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,9 +409,22 @@ 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']);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that we don't want this. This sued to be the case before, but a number of people pointed out that it was unintuitive. It makes sense from the Laravel perspective, since we know about route model binding, but the reader of the API docs wants to know the value that goes in there (the user ID), not just the model.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that using only the model name is not intuitive, but that is not the case when using type-hinted variables in the controller action.
In the test case you pointed out, there are no type-hinted variables in the controller action, and that is the reason why the URL parameter was user.
But when using type-hinted variables in the controller action, the URL parameter will be user_id (or user_slug if the developer customized the route key name to slug). I think this behavior satisfies both the API docs reader and the developer since he can customize the route key name.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I don't agree there.

since he can customize the route key name

Yes, but will he? It would be annoying to ask a developer to go through their routes and add :id for every parameter they use. Especially as Laravel defaults to using the id as the route key, it makes no sense that we can't.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From your original comment:

The bug is that Scribe uses the id as the default URL parameter when no inline binding is applied, but the user might be using other column by overriding the getRouteKeyName() method in the model.

I think we should focus on this. See if the model has getRouteKeyName() defined or a custom key in the route, and use that. Otherwise, we stick to id. We may just need to move the order of operations around to achieve this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you want to stick with the id even for varaibles that are not type-hinted? as the id will be used by default for the type-hinted variables because getRouteKeyName() is defined in Illuminate\Database\Eloquent\Model to return the id.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. The current behavior:

  1. If there is an inline binding in the route, use it.
  2. Else, if it is a resource route parameter, use the id.
  3. Otherwise, keep the same parameter name.

What I will do in my new pull request, is to add a new step before the second step to check if there is a type-hinted variable in the controller action with the same name as the route segment name. This is the same behavior as the laravel implicit binding:

Laravel automatically resolves Eloquent models defined in routes or controller actions whose type-hinted variable names match a route segment name.

Case 1: If there is an inline binding in the route.

Route::apiResource('posts', PostController::class)->only('show')->parameters(['posts' => 'post:slug']);
Route::get('posts/{post:slug}/comments', function() {});

Current behavior: use inline binding. Current URIs: posts/{slug} and posts/{post_slug}/comments.
The new behavior is the same.

Case 2: No inline binding exists, but model binding exists.

Assume that Post@getRouteKeyName returns slug, and PostController@show uses model binding (Post $post).

Route::apiResource('posts', PostController::class)->only('show').
Route::get('posts/{post}/comments', function(Post $post) {});

Current behavior: use the id for resource parameters, and keep the same name for non-resource parameters.
Current URIs: posts/{id} and posts/{post}/comments.
New behavior: call getRouteKeyName().
New URIs: posts/{slug} and posts/{post_slug}/comments.

Case 3: Resource route parameter. No inline binding exists. No model binding exists.

Assume that PostController@show does not use model binding.

Route::apiResource('posts', PostController::class)->only('show');

Current behavior: use the id. Current URI: posts/{id}.
The new behavior is the same.

Case 4: Non-Resource route. No inline binding exists. No model binding exists.

Route::get('posts/{post}/commets', function($postId) {});

Current behavior: keep the same parameter name. Current URI: posts/{post}.
The new behavior is the same.

Copy link
Contributor Author

@Khaled-Farhat Khaled-Farhat Jul 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have another question about the testing, Is there a problem if I added 3 fixtures to test the new behavior?

The fixtures I want to add:

  • TestPost model that overrides getRouteKeyName() to return slug.
  • TestPostController with one method that has a type-hinted variable (TestPost $post). I will use it to test generating the URI posts/{slug}.
  • TestPostUserController with one method that uses two type-hinted variables (TestPost $post, TestUser $user). I will use it to test generating the URI posts/{post_slug}/users/{id} from a nested resource route.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that sounds good. We probably need to clean up the code to make it less confusing, perhaps with some inline documentation of the examples. Also, I think the responsibility is half-shared; IIRC, there's somewhere else in the code where the URI is "fixed" slightly, which may change the results from here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you can add whatever you need.

Copy link
Contributor

@shalvah shalvah Jul 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I just noticed your PR edits both locations (GetFromLaravelAPI and ExtractedEndpointData), which is good. I wish there was a way for that logic to remain in one place (ideally the GetFromLaravelAPI strategy), but the URL is one of the basic things about the endpoint, so it needs to be fixed as early as possible. Plus, even though they can, I'd prefer to avoid modifying the endpoint object directly inside a strategy.

$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 */
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 */
Expand Down
4 changes: 2 additions & 2 deletions tests/Strategies/UrlParameters/GetFromLaravelAPITest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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']);
}
}
28 changes: 25 additions & 3 deletions tests/Unit/ExtractedEndpointDataTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,17 @@ 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');
->only('show')
->parameters([
'things' => 'thing:id',
]);
$routeRules[0]['match'] = ['prefixes' => '*', 'domains' => '*'];

$matcher = new RouteMatcher();
Expand All @@ -32,7 +41,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);
Expand All @@ -51,8 +64,17 @@ 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');
->only('show')
->parameters([
'audio-things' => 'audio_thing:id',
]);
$routeRules[0]['match'] = ['prefixes' => '*', 'domains' => '*'];

$matcher = new RouteMatcher();
Expand Down