Merge pull request #1552 from cyberemissary/duplicate-param-cleanup

Added describer that removes duplicate parameters when using $ref.

Resources/config/fos_rest.xml

@@ -7,7 +7,7 @@
         <service id="nelmio_api_doc.route_describers.fos_rest" class="Nelmio\ApiDocBundle\RouteDescriber\FosRestDescriber" public="false">
             <argument type="service" id="annotation_reader" />
 
-            <tag name="nelmio_api_doc.route_describer" priority="-300" />
+            <tag name="nelmio_api_doc.route_describer" priority="-250" />
         </service>
     </services>
 

Resources/config/php_doc.xml

@@ -5,7 +5,7 @@
 
     <services>
         <service id="nelmio_api_doc.route_describers.php_doc" class="Nelmio\ApiDocBundle\RouteDescriber\PhpDocDescriber" public="false">
-            <tag name="nelmio_api_doc.route_describer" priority="-1000" />
+            <tag name="nelmio_api_doc.route_describer" priority="-275" />
         </service>
     </services>
 

RouteDescriber/RouteMetadataDescriber.php

@@ -11,9 +11,15 @@
 
 namespace Nelmio\ApiDocBundle\RouteDescriber;
 
+use EXSyst\Component\Swagger\Operation;
+use EXSyst\Component\Swagger\Parameter;
 use EXSyst\Component\Swagger\Swagger;
+use LogicException;
 use Symfony\Component\Routing\Route;
 
+/**
+ * Should be last route describer executed to make sure all params are set.
+ */
 final class RouteMetadataDescriber implements RouteDescriberInterface
 {
     use RouteDescriberTrait;
@@ -25,6 +31,7 @@ final class RouteMetadataDescriber implements RouteDescriberInterface
 
             $requirements = $route->getRequirements();
             $compiledRoute = $route->compile();
+            $existingParams = $this->getRefParams($api, $operation);
 
             // Don't include host requirements
             foreach ($compiledRoute->getPathVariables() as $pathVariable) {
@@ -32,6 +39,16 @@ final class RouteMetadataDescriber implements RouteDescriberInterface
                     continue;
                 }
 
+                $paramId = $pathVariable.'/path';
+                $parameter = $existingParams[$paramId] ?? null;
+                if (null !== $parameter) {
+                    if (!$parameter->getRequired()) {
+                        throw new LogicException(\sprintf('Global parameter "%s" is used as part of route "%s" and must be set as "required"', $pathVariable, $route->getPath()));
+                    }
+
+                    continue;
+                }
+
                 $parameter = $operation->getParameters()->get($pathVariable, 'path');
                 $parameter->setRequired(true);
 
@@ -39,10 +56,43 @@ final class RouteMetadataDescriber implements RouteDescriberInterface
                     $parameter->setType('string');
                 }
 
-                if (isset($requirements[$pathVariable])) {
+                if (isset($requirements[$pathVariable]) && null === $parameter->getPattern()) {
                     $parameter->setPattern($requirements[$pathVariable]);
                 }
             }
         }
     }
+
+    /**
+     * The '$ref' parameters need special handling, since their objects are missing 'name' and 'in'.
+     *
+     * @return Parameter[] existing $ref parameters
+     */
+    private function getRefParams(Swagger $api, Operation $operation): array
+    {
+        /** @var Parameter[] $globalParams */
+        $globalParams = $api->getParameters();
+        $existingParams = [];
+
+        foreach ($operation->getParameters() as $id => $parameter) {
+            $ref = $parameter->getRef();
+            if (null === $ref) {
+                // we only concern ourselves with '$ref' parameters, so continue the loop
+                continue;
+            }
+
+            $ref = \mb_substr($ref, 13); // trim the '#/parameters/' part of ref
+            if (!isset($globalParams[$ref])) {
+                // this shouldn't happen during proper configs, but in case of bad config, just ignore it here
+                continue;
+            }
+
+            $refParameter = $globalParams[$ref];
+
+            // param ids are in form {name}/{in}
+            $existingParams[\sprintf('%s/%s', $refParameter->getName(), $refParameter->getIn())] = $refParameter;
+        }
+
+        return $existingParams;
+    }
 }

Tests/Describer/AbstractDescriberTest.php

@@ -12,10 +12,12 @@
 namespace Nelmio\ApiDocBundle\Tests\Describer;
 
 use EXSyst\Component\Swagger\Swagger;
+use Nelmio\ApiDocBundle\Describer\DescriberInterface;
 use PHPUnit\Framework\TestCase;
 
 abstract class AbstractDescriberTest extends TestCase
 {
+    /** @var DescriberInterface */
     protected $describer;
 
     protected function getSwaggerDoc(): Swagger

Tests/Functional/Controller/TestController.php

@@ -29,4 +29,16 @@ class TestController
     public function testAction()
     {
     }
+
+    /**
+     * @SWG\Parameter(ref="#/parameters/test"),
+     * @SWG\Response(
+     *     response="200",
+     *     description="Test Ref"
+     * )
+     * @Route("/test/{id}", methods={"GET"})
+     */
+    public function testRefAction()
+    {
+    }
 }

Tests/Functional/FunctionalTest.php

@@ -402,4 +402,9 @@ class FunctionalTest extends WebTestCase
         $this->assertSame('This is post', $postOperation->getDescription());
         $this->assertSame('Worked well!', $postOperation->getResponses()->get(200)->getDescription());
     }
+
+    public function testNoDuplicatedParameters()
+    {
+        $this->assertFalse($this->getOperation('/api/article/{id}', 'get')->getParameters()->has('id', 'path'));
+    }
 }

Tests/Functional/SwaggerUiTest.php

@@ -59,6 +59,10 @@ class SwaggerUiTest extends WebTestCase
             '/test/test/' => ['get' => [
                 'responses' => ['200' => ['description' => 'Test']],
             ]],
+            '/test/test/{id}' => ['get' => [
+                'responses' => ['200' => ['description' => 'Test Ref']],
+                'parameters' => [['$ref' => '#/parameters/test']],
+            ]],
         ];
         $expected['definitions'] = [
             'Dummy' => $expected['definitions']['Dummy'],