Sfoglia il codice sorgente

Infer URL params more accurately when there's a field binding

shalvah 3 anni fa
parent
commit
ce6be7ca68

+ 40 - 11
camel/Extraction/ExtractedEndpointData.php

@@ -139,25 +139,44 @@ class ExtractedEndpointData extends BaseDTO
         $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;
     }
 
@@ -174,4 +193,14 @@ class ExtractedEndpointData extends BaseDTO
             '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;
+    }
 }

+ 2 - 6
src/Extracting/Strategies/UrlParameters/GetFromLaravelAPI.php

@@ -28,12 +28,8 @@ class GetFromLaravelAPI extends Strategy
             $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),

+ 33 - 7
tests/GenerateDocumentationTest.php

@@ -174,7 +174,7 @@ class GenerateDocumentationTest extends BaseLaravelTest
     {
         RouteFacade::resource('/api/users', TestPartialResourceController::class);
 
-        config(['scribe.routes.0.prefixes' => ['api/*']]);
+        config(['scribe.routes.0.match.prefixes' => ['api/*']]);
 
         $output = $this->artisan('scribe:generate');
 
@@ -273,7 +273,7 @@ class GenerateDocumentationTest extends BaseLaravelTest
     {
         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');
@@ -288,7 +288,7 @@ class GenerateDocumentationTest extends BaseLaravelTest
         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');
@@ -304,7 +304,7 @@ class GenerateDocumentationTest extends BaseLaravelTest
     {
         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');
 
@@ -318,7 +318,7 @@ class GenerateDocumentationTest extends BaseLaravelTest
     {
         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');
 
@@ -357,10 +357,32 @@ class GenerateDocumentationTest extends BaseLaravelTest
         $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]);
@@ -383,7 +405,9 @@ class GenerateDocumentationTest extends BaseLaravelTest
     {
         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');
@@ -412,7 +436,9 @@ class GenerateDocumentationTest extends BaseLaravelTest
         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');