Repository: knuckleswtf/scribe (php)

Issue:
@urlParam as slug which is not primary key
- [x] **I confirm that I have read and attempted the tips in the [Troubleshooting Guide](https://scribe.knuckles.wtf/laravel/troubleshooting).**

**What happened?**

1. I have a model `Author` with a primary key as `int` `id` but I bind my routes with `string` `slug` to find details of model.

In `routes/api.php`, the URL parameter is called `author`.

```php
<?php

// ...
Route::get('/authors', [AuthorController::class, 'index'])->name('api.authors.index'); // authors list
Route::get('/authors/{author}', [AuthorController::class, 'show'])->name('api.authors.show'); // author show from author param as slug
```

In `app/Http/Controllers/Api:AuthorController.php`, I have to override with `@urlParam` for `id` instead of `author`, because scribe find `id` as primary key I suppose. It's not really a problem, except that seeing a parameter indicated as `id` can lead to errors but I don't know how to override the parameter name. I give an example for request to have correct response.

```php
<?php

/**
* Author details
*
* Details for one author, find by slug.
*
* @urlParam id string required The slug of author. Example: lovecraft-howard-phillips
*
*/
public function show(string $slug)
{
    try {
        $author = Author::whereSlug($slug)->with('media')->withCount('books')->firstOrFail();
        $author = AuthorResource::make($author);

        return $author;
    } catch (\Throwable $th) {
        return response()->json(['failed' => 'No result for '.$slug], 404);
    }
}
```

2. Then I ran `php artisan scribe:generate`, I've this result.

![laravel-scribe-param-slug](https://user-images.githubusercontent.com/48261459/125634813-21e4a201-4c2f-43e9-af1c-9dc576751e12.jpg)

3. But I saw, at right, I have correct `Example request` with `"http://localhost:8000/api/authors/lovecraft-howard-phillips"` but for the response, scribe try to find result with `id` `1` as parameter: `"failed": "No result for 1"`.


**My environment:**
 - PHP version: 8.0.7
 - Framework: Laravel v8.5.0
 - Scribe version: v3.6

<details>
  <summary>My Scribe config</summary>
  
```php
<?php

use Knuckles\Scribe\Extracting\Strategies;

return [
    'theme' => 'default',

    /*
     * The HTML <title> for the generated documentation. If this is empty, Scribe will infer it from config('app.name').
     */
    'title' => config('app.name').' API Documentation',

    /*
     * A short description of your API. Will be included in the docs webpage, Postman collection and OpenAPI spec.
     */
    'description' => 'The API documentation for '.config('app.name').' to use endpoints with another app.',

    /*
     * The base URL displayed in the docs. If this is empty, Scribe will use the value of config('app.url').
     */
    'base_url' => config('app.url'),

    /*
     * Tell Scribe what routes to generate documentation for.
     * Each group contains rules defining which routes should be included ('match', 'include' and 'exclude' sections)
     * and settings which should be applied to them ('apply' section).
     */
    'routes' => [
        [
            /*
             * Specify conditions to determine what routes will be a part of this group.
             * A route must fulfill ALL conditions to be included.
             */
            'match' => [
                /*
                 * Match only routes whose paths match this pattern (use * as a wildcard to match any characters). Example: 'users/*'.
                 */
                'prefixes' => ['api/*'],

                /*
                 * Match only routes whose domains match this pattern (use * as a wildcard to match any characters). Example: 'api.*'.
                 */
                'domains' => ['*'],

                /*
                 * [Dingo router only] Match only routes registered under this version. Wildcards are not supported.
                 */
                'versions' => ['v1'],
            ],

            /*
             * Include these routes even if they did not match the rules above.
             * The route can be referenced by name or path here. Wildcards are supported.
             */
            'include' => [
                // 'users.index', 'healthcheck*'
            ],

            /*
             * Exclude these routes even if they matched the rules above.
             * The route can be referenced by name or path here. Wildcards are supported.
             */
            'exclude' => [
                // '/health', 'admin.*'
            ],

            /*
             * Settings to be applied to all the matched routes in this group when generating documentation
             */
            'apply' => [
                /*
                 * Additional headers to be added to the example requests
                 */
                'headers' => [
                    'Content-Type' => 'application/json',
                    'Accept'       => 'application/json',
                ],

                /*
                 * If no @response or @transformer declarations are found for the route,
                 * Scribe will try to get a sample response by attempting an API call.
                 * Configure the settings for the API call here.
                 */
                'response_calls' => [
                    /*
                     * API calls will be made only for routes in this group matching these HTTP methods (GET, POST, etc).
                     * List the methods here or use '*' to mean all methods. Leave empty to disable API calls.
                     */
                    'methods' => ['GET'],

                    /*
                     * Laravel config variables which should be set for the API call.
                     * This is a good place to ensure that notifications, emails and other external services
                     * are not triggered during the documentation API calls.
                     * You can also create a `.env.docs` file and run the generate command with `--env docs`.
                     */
                    'config' => [
                        'app.env' => 'documentation',
                        // 'app.debug' => false,
                    ],

                    /*
                     * Query parameters which should be sent with the API call.
                     */
                    'queryParams' => [
                        // 'key' => 'value',
                    ],

                    /*
                     * Body parameters which should be sent with the API call.
                     */
                    'bodyParams' => [
                        // 'key' => 'value',
                    ],

                    /*
                     * Files which should be sent with the API call.
                     * Each value should be a valid path (absolute or relative to your project directory) to a file on this machine (but not in the project root).
                     */
                    'fileParams' => [
                        // 'key' => 'storage/app/image.png',
                    ],

                    /*
                     * Cookies which should be sent with the API call.
                     */
                    'cookies' => [
                        // 'name' => 'value'
                    ],
                ],
            ],
        ],
    ],

    /*
     * The type of documentation output to generate.
     * - "static" will generate a static HTMl page in the /public/docs folder,
     * - "laravel" will generate the documentation as a Blade view, so you can add routing and authentication.
     */
    'type' => 'static',

    /*
     * Settings for `static` type output.
     */
    'static' => [
        /*
         * HTML documentation, assets and Postman collection will be generated to this folder.
         * Source Markdown will still be in resources/docs.
         */
        'output_path' => 'public/docs',
    ],

    /*
     * Settings for `laravel` type output.
     */
    'laravel' => [
        /*
         * Whether to automatically create a docs endpoint for you to view your generated docs.
         * If this is false, you can still set up routing manually.
         */
        'add_routes' => true,

        /*
         * URL path to use for the docs endpoint (if `add_routes` is true).
         * By default, `/docs` opens the HTML page, `/docs.postman` opens the Postman collection, and `/docs.openapi` the OpenAPI spec.
         */
        'docs_url' => '/docs',

        /*
         * Middleware to attach to the docs endpoint (if `add_routes` is true).
         */
        'middleware' => [],
    ],

    'try_it_out' => [
        /*
         * Add a Try It Out button to your endpoints so consumers can test endpoints right from their browser.
         * Don't forget to enable CORS headers for your endpoints.
         */
        'enabled' => true,

        /*
         * The base URL for the API tester to use (for example, you can set this to your staging URL).
         * Leave as null to use the current app URL (config(app.url)).
         */
        'base_url' => null,
    ],

    /*
     * How is your API authenticated? This information will be used in the displayed docs, generated examples and response calls.
     */
    'auth' => [
        /*
         * Set this to true if any endpoints in your API use authentication.
         */
        'enabled' => false,

        /*
         * Set this to true if your API should be authenticated by default. If so, you must also set `enabled` (above) to true.
         * You can then use @unauthenticated or @authenticated on individual endpoints to change their status from the default.
         */
        'default' => false,

        /*
         * Where is the auth value meant to be sent in a request?
         * Options: query, body, basic, bearer, header (for custom header)
         */
        'in' => 'bearer',

        /*
         * The name of the auth parameter (eg token, key, apiKey) or header (eg Authorization, Api-Key).
         */
        'name' => 'key',

        /*
         * The value of the parameter to be used by Scribe to authenticate response calls.
         * This will NOT be included in the generated documentation.
         * If this value is empty, Scribe will use a random value.
         */
        'use_value' => env('SCRIBE_AUTH_KEY'),

        /*
         * Placeholder your users will see for the auth parameter in the example requests.
         * Set this to null if you want Scribe to use a random value as placeholder instead.
         */
        'placeholder' => '{YOUR_AUTH_KEY}',

        /*
         * Any extra authentication-related info for your users. For instance, you can describe how to find or generate their auth credentials.
         * Markdown and HTML are supported.
         */
        'extra_info' => 'You can retrieve your token by visiting your dashboard and clicking <b>Generate API token</b>.',
    ],

    /*
     * Text to place in the "Introduction" section, right after the `description`. Markdown and HTML are supported.
     */
    'intro_text' => <<<INTRO
This documentation aims to provide all the information you need to work with our API.

<aside>As you scroll, you'll see code examples for working with the API in different programming languages in the dark area to the right (or as part of the content on mobile).
You can switch the language used with the tabs at the top right (or from the nav menu at the top left on mobile).</aside>
INTRO
    ,

    /*
     * Example requests for each endpoint will be shown in each of these languages.
     * Supported options are: bash, javascript, php, python
     * To add a language of your own, see https://scribe.knuckles.wtf/laravel/advanced/example-requests
     *
     */
    'example_languages' => [
        'bash',
        // 'javascript',
    ],

    /*
     * Generate a Postman collection (v2.1.0) in addition to HTML docs.
     * For 'static' docs, the collection will be generated to public/docs/collection.json.
     * For 'laravel' docs, it will be generated to storage/app/scribe/collection.json.
     * Setting `laravel.add_routes` to true (above) will also add a route for the collection.
     */
    'postman' => [
        'enabled' => true,

        /*
         * Manually override some generated content in the spec. Dot notation is supported.
         */
        'overrides' => [
            // 'info.version' => '2.0.0',
        ],
    ],

    /*
     * Generate an OpenAPI spec (v3.0.1) in addition to docs webpage.
     * For 'static' docs, the collection will be generated to public/docs/openapi.yaml.
     * For 'laravel' docs, it will be generated to storage/app/scribe/openapi.yaml.
     * Setting `laravel.add_routes` to true (above) will also add a route for the spec.
     */
    'openapi' => [
        'enabled' => true,

        /*
         * Manually override some generated content in the spec. Dot notation is supported.
         */
        'overrides' => [
            // 'info.version' => '2.0.0',
        ],
    ],

    /*
     * Endpoints which don't have a @group will be placed in this default group.
     */
    'default_group' => 'Misc',

    /*
     * Custom logo path. This will be used as the value of the src attribute for the <img> tag,
     * so make sure it points to an accessible URL or path. Set to false to not use a logo.
     *
     * For example, if your logo is in public/img:
     * - 'logo' => '../img/logo.png' // for `static` type (output folder is public/docs)
     * - 'logo' => 'img/logo.png' // for `laravel` type
     *
     */
    'logo' => false,

    /*
     * If you would like the package to generate the same example values for parameters on each run,
     * set this to any number (eg. 1234)
     */
    'faker_seed' => null,

    /*
     * The strategies Scribe will use to extract information about your routes at each stage.
     * If you create or install a custom strategy, add it here.
     */
    'strategies' => [
        'metadata' => [
            Strategies\Metadata\GetFromDocBlocks::class,
        ],
        'urlParameters' => [
            Strategies\UrlParameters\GetFromLaravelAPI::class,
            Strategies\UrlParameters\GetFromLumenAPI::class,
            Strategies\UrlParameters\GetFromUrlParamTag::class,
        ],
        'queryParameters' => [
            Strategies\QueryParameters\GetFromFormRequest::class,
            Strategies\QueryParameters\GetFromInlineValidator::class,
            Strategies\QueryParameters\GetFromQueryParamTag::class,
        ],
        'headers' => [
            Strategies\Headers\GetFromRouteRules::class,
            Strategies\Headers\GetFromHeaderTag::class,
        ],
        'bodyParameters' => [
            Strategies\BodyParameters\GetFromFormRequest::class,
            Strategies\BodyParameters\GetFromInlineValidator::class,
            Strategies\BodyParameters\GetFromBodyParamTag::class,
        ],
        'responses' => [
            Strategies\Responses\UseTransformerTags::class,
            Strategies\Responses\UseResponseTag::class,
            Strategies\Responses\UseResponseFileTag::class,
            Strategies\Responses\UseApiResourceTags::class,
            Strategies\Responses\ResponseCalls::class,
        ],
        'responseFields' => [
            Strategies\ResponseFields\GetFromResponseFieldTag::class,
        ],
    ],

    'fractal' => [
        /* If you are using a custom serializer with league/fractal, you can specify it here.
         * Leave as null to use no serializer or return simple JSON.
         */
        'serializer' => null,
    ],

    /*
     * [Advanced] Custom implementation of RouteMatcherInterface to customise how routes are matched
     *
     */
    'routeMatcher' => \Knuckles\Scribe\Matching\RouteMatcher::class,

    /*
     * For response calls, API resource responses and transformer responses,
     * Scribe will try to start database transactions, so no changes are persisted to your database.
     * Tell Scribe which connections should be transacted here.
     * If you only use one db connection, you can leave this as is.
     */
    'database_connections_to_transact' => [config('database.default')],
];
```
  
</details>


Pull request description:
Fixed a bug in normalizing URL parameters
This pull request fixes #275. 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 another column by overriding the `getRouteKeyName()` method in the model.

## Behavior
#### Old behavior with API resource routes
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.
#### New behavior for API resource routes
1. If there is an inline binding in the route, use it.
2. Else, If there is a type-hinted variable in the controller action, with the same name as the route segment name, use it to call `getRouteKeyName()`.
3. Else, if it is a resource route parameter, use the `id`.
4. Otherwise, keep the same parameter name.

Examples of the differences can be found in a [previous discussion](https://github.com/knuckleswtf/scribe/pull/489#discussion_r914571572).

## Code
#### Before
`Camel\Extraction\ExtractedEndpointData@normalizeResourceParamName` is called in the constructor before calling the parent constructor and initializing the object, so you cannot access the `method` property in the normalization process, as it has not been initialized yet.
#### After
`Camel\Extraction\ExtractedEndpointData@normalizeResourceParamName` is called after calling the parent constructor.
If there was no inline binding in the route, `Camel\Extraction\ExtractedEndpointData@getFieldBindingForUrlParam` will search for a type-hinted variable whose name matches the route segment name, and will use it to call `getRouteKeyName()` on the model.

## Testing
Created two feature tests in `Knuckles\Scribe\Tests\GenerateDocumentationTest` to test generating URL params from resource and non-resource routes with model binding. Created some fixtures to achieve that: A `TestPost` model that overrides `getRouteKeyName()` to return `slug`, A partial resource controller with an injected `TestPost` argument, and another partial resource controller for a nested resource `(posts.users)` with injected `TestPost` and `TestUser` models.

## Possible Refactor
Maybe we can create a new class `Src\Extracting\UrlParamsNormalizer` and move the normalization logic into it. We can use that class inside the constructor of `Camel\Extraction\ExtractedEndpointData`.

Patch:
diff --git a/camel/Extraction/ExtractedEndpointData.php b/camel/Extraction/ExtractedEndpointData.php
index 4938fd85..cc5fce16 100644
--- a/camel/Extraction/ExtractedEndpointData.php
+++ b/camel/Extraction/ExtractedEndpointData.php
@@ -2,6 +2,7 @@
 
 namespace Knuckles\Camel\Extraction;
 
+use Illuminate\Database\Eloquent\Model;
 use Illuminate\Routing\Route;
 use Illuminate\Support\Str;
 use Knuckles\Camel\BaseDTO;
@@ -81,11 +82,12 @@ class ExtractedEndpointData extends BaseDTO
 
     public function __construct(array $parameters = [])
     {
-        $parameters['uri'] = $this->normalizeResourceParamName($parameters['uri'], $parameters['route']);
         $parameters['metadata'] = $parameters['metadata'] ?? new Metadata([]);
         $parameters['responses'] = $parameters['responses'] ?? new ResponseCollection([]);
 
         parent::__construct($parameters);
+
+        $this->uri = $this->normalizeResourceParamName($this->uri, $this->route, $this->getTypeHintedArguments());
     }
 
     public static function fromRoute(Route $route, array $extras = []): self
@@ -131,7 +133,7 @@ class ExtractedEndpointData extends BaseDTO
         return $this->httpMethods[0] . str_replace(['/', '?', '{', '}', ':', '\\', '+', '|'], '-', $this->uri);
     }
 
-    public function normalizeResourceParamName(string $uri, Route $route): string
+    public function normalizeResourceParamName(string $uri, Route $route, array $typeHintedArguments): string
     {
         $params = [];
         preg_match_all('#\{(\w+?)}#', $uri, $params);
@@ -157,9 +159,11 @@ class ExtractedEndpointData extends BaseDTO
                     "{$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 there is an inline binding in the route, like /users/{user:uuid}, use that key,
+                // Else, search for a type-hinted variable in the action, whose name matches the route segment name,
+                // If there is such variable (like User $user), call getRouteKeyName() on the model,
+                // Otherwise, use the id
+                $binding = static::getFieldBindingForUrlParam($route, $singularResource, $typeHintedArguments, 'id');
 
                 if (!$foundResourceParam) {
                     // Only the last resource param should be {id}
@@ -179,8 +183,8 @@ class ExtractedEndpointData extends BaseDTO
         }
 
         foreach ($params[1] as $param) {
-            // For non-resource parameters, if there's a field binding, replace that too:
-            if ($binding = static::getFieldBindingForUrlParam($route, $param)) {
+            // For non-resource parameters, if there's a field binding/type-hinted variable, replace that too:
+            if ($binding = static::getFieldBindingForUrlParam($route, $param, $typeHintedArguments)) {
                 $search = ["{{$param}}", "{{$param}?}"];
                 $replace = ["{{$param}_{$binding}}", "{{$param}_{$binding}?}"];
                 $uri = str_replace($search, $replace, $uri);
@@ -206,7 +210,8 @@ class ExtractedEndpointData extends BaseDTO
         return $copy;
     }
 
-    public static function getFieldBindingForUrlParam(Route $route, string $paramName, string $default = null): ?string
+    public static function getFieldBindingForUrlParam(Route $route, string $paramName, array $typeHintedArguments = [],
+                                                      string $default = null): ?string
     {
         $binding = null;
         // Was added in Laravel 7.x
@@ -214,6 +219,48 @@ class ExtractedEndpointData extends BaseDTO
             $binding = $route->bindingFieldFor($paramName);
         }
 
+        // Search for a type-hinted variable whose name matches the route segment name
+        if (is_null($binding) && array_key_exists($paramName, $typeHintedArguments)) {
+            $argumentType = $typeHintedArguments[$paramName]->getType();
+            $argumentClassName = $argumentType->getName();
+            $argumentInstance = new $argumentClassName;
+            $binding = $argumentInstance->getRouteKeyName();
+        }
+
         return $binding ?: $default;
     }
+
+    /**
+     * Return the type-hinted method arguments in the action that have a Model type,
+     * The arguments will be returned as an array of the form: $arguments[<variable_name>] = $argument
+     */
+    protected function getTypeHintedArguments(): array
+    {
+        $arguments = [];
+        if ($this->method) {
+            foreach ($this->method->getParameters() as $argument) {
+                if ($this->argumentHasModelType($argument)) {
+                    $arguments[$argument->getName()] = $argument;
+                }
+            }
+        }
+
+        return $arguments;
+    }
+
+    /**
+     * Determine whether the argument has a Model type
+     */
+    protected function argumentHasModelType(\ReflectionParameter $argument): bool
+    {
+        $argumentType = $argument->getType();
+        if (!$argumentType) {
+            // The argument does not have a type-hint
+            return false;
+        } else {
+            $argumentClassName = $argumentType->getName();
+            $argumentInstance = new $argumentClassName;
+            return ($argumentInstance instanceof Model);
+        }
+    }
 }


Test patch:
diff --git a/tests/Fixtures/TestPost.php b/tests/Fixtures/TestPost.php
new file mode 100644
index 00000000..5d75c3ed
--- /dev/null
+++ b/tests/Fixtures/TestPost.php
@@ -0,0 +1,13 @@
+<?php
+
+namespace Knuckles\Scribe\Tests\Fixtures;
+
+use Illuminate\Database\Eloquent\Model;
+
+class TestPost extends Model
+{
+    public function getRouteKeyName()
+    {
+        return 'slug';
+    }
+}
diff --git a/tests/Fixtures/TestPostController.php b/tests/Fixtures/TestPostController.php
new file mode 100644
index 00000000..5efb7843
--- /dev/null
+++ b/tests/Fixtures/TestPostController.php
@@ -0,0 +1,10 @@
+<?php
+
+namespace Knuckles\Scribe\Tests\Fixtures;
+
+class TestPostController
+{
+    public function update(TestPost $post)
+    {
+    }
+}
diff --git a/tests/Fixtures/TestPostUserController.php b/tests/Fixtures/TestPostUserController.php
new file mode 100644
index 00000000..a7aff2c0
--- /dev/null
+++ b/tests/Fixtures/TestPostUserController.php
@@ -0,0 +1,10 @@
+<?php
+
+namespace Knuckles\Scribe\Tests\Fixtures;
+
+class TestPostUserController
+{
+    public function update(TestPost $post, TestUser $user)
+    {
+    }
+}
diff --git a/tests/GenerateDocumentationTest.php b/tests/GenerateDocumentationTest.php
index 0335f924..179be3bf 100644
--- a/tests/GenerateDocumentationTest.php
+++ b/tests/GenerateDocumentationTest.php
@@ -8,6 +8,9 @@ use Knuckles\Scribe\Tests\Fixtures\TestController;
 use Knuckles\Scribe\Tests\Fixtures\TestGroupController;
 use Knuckles\Scribe\Tests\Fixtures\TestIgnoreThisController;
 use Knuckles\Scribe\Tests\Fixtures\TestPartialResourceController;
+use Knuckles\Scribe\Tests\Fixtures\TestPost;
+use Knuckles\Scribe\Tests\Fixtures\TestPostController;
+use Knuckles\Scribe\Tests\Fixtures\TestPostUserController;
 use Knuckles\Scribe\Tests\Fixtures\TestResourceController;
 use Knuckles\Scribe\Tests\Fixtures\TestUser;
 use Knuckles\Scribe\Tools\Utils;
@@ -414,6 +417,36 @@ class GenerateDocumentationTest extends BaseLaravelTest
         $this->assertEquals('providers/{provider_slug}/users/{user_id}/addresses/{uuid}', $groupB['endpoints'][0]['uri']);
     }
 
+    /** @test */
+    public function generates_correct_url_params_from_resource_routes_and_model_binding()
+    {
+        RouteFacade::resource('posts', TestPostController::class)->only('update');
+        RouteFacade::resource('posts.users', TestPostUserController::class)->only('update');
+
+        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('posts/{slug}', $group['endpoints'][0]['uri']);
+        $this->assertEquals('posts/{post_slug}/users/{id}', $group['endpoints'][1]['uri']);
+    }
+
+    /** @test */
+    public function generates_correct_url_params_from_non_resource_routes_and_model_binding()
+    {
+        RouteFacade::get('posts/{post}/users', function(TestPost $post) {});
+
+        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('posts/{post_slug}/users', $group['endpoints'][0]['uri']);
+    }
+
     /** @test */
     public function will_generate_without_extracting_if_noExtraction_flag_is_set()
     {