From 9b9dbe69ddb6a3548e5a4155d293f641b2f1b0ff Mon Sep 17 00:00:00 2001 From: cyberemissary <25044236+cyberemissary@users.noreply.github.com> Date: Tue, 5 Nov 2019 08:41:32 -0500 Subject: [PATCH 1/3] Added describer that removes duplicate parameters when using $ref. --- Describer/ParameterRefMergeDescriber.php | 79 +++++++++++++++++++ Resources/config/services.xml | 4 + Tests/Describer/AbstractDescriberTest.php | 2 + .../ParameterRefMergeDescriberTest.php | 64 +++++++++++++++ 4 files changed, 149 insertions(+) create mode 100644 Describer/ParameterRefMergeDescriber.php create mode 100644 Tests/Describer/ParameterRefMergeDescriberTest.php diff --git a/Describer/ParameterRefMergeDescriber.php b/Describer/ParameterRefMergeDescriber.php new file mode 100644 index 0000000..bebf8d3 --- /dev/null +++ b/Describer/ParameterRefMergeDescriber.php @@ -0,0 +1,79 @@ +getPaths() as $path) { + /** @var Operation $operation */ + foreach ($path->getOperations() as $operation) { + $this->checkOperation($api, $operation); + } + } + } + + /** + * This method removes parameters that also have a ref version as they will be duplicated otherwise. + */ + private function checkOperation(Swagger $api, Operation $operation) + { + $parametersToRemove = []; + + /** @var Parameter[] $globalParams */ + $globalParams = $api->getParameters(); + /** @var Parameters|Parameter[] $currentParams */ + $currentParams = $operation->getParameters(); + + foreach ($currentParams as $parameter) { + $ref = $parameter->getRef(); + + if (null === $ref) { + // we only concern ourselves with '$ref' parameters + continue; + } + + $ref = \mb_substr($ref, 13); // trim the '#/parameters/' part of ref + if (!isset($globalParams[$ref])) { + // this really shouldn't happen, if it does there will be other failures elsewhere, so just ignore here + continue; + } + + $refParameter = $globalParams[$ref]; + // param ids are in form {name}/{in} + $refParameterId = \sprintf('%s/%s', $refParameter->getName(), $refParameter->getIn()); + if ($currentParams->has($refParameterId)) { + // if we got here it means a duplicate parameter is directly defined, schedule it for removal + $parametersToRemove[] = $currentParams->get($refParameterId); + } + } + + foreach ($parametersToRemove as $parameterToRemove) { + $currentParams->remove($parameterToRemove); + } + } +} diff --git a/Resources/config/services.xml b/Resources/config/services.xml index 978930e..c6ad025 100644 --- a/Resources/config/services.xml +++ b/Resources/config/services.xml @@ -32,6 +32,10 @@ + + + + diff --git a/Tests/Describer/AbstractDescriberTest.php b/Tests/Describer/AbstractDescriberTest.php index b1cca2d..f73dcb3 100644 --- a/Tests/Describer/AbstractDescriberTest.php +++ b/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 diff --git a/Tests/Describer/ParameterRefMergeDescriberTest.php b/Tests/Describer/ParameterRefMergeDescriberTest.php new file mode 100644 index 0000000..bd2b666 --- /dev/null +++ b/Tests/Describer/ParameterRefMergeDescriberTest.php @@ -0,0 +1,64 @@ + '2.0', + 'info' => ['title' => 'Ref Test'], + 'paths' => [ + '/api/{version}/product' => [ + 'get' => [ + 'parameters' => [ + [ + '$ref' => '#/parameters/versionParam', + ], + [ + 'name' => 'version', + 'in' => 'path', + 'required' => true, + 'type' => 'string', + 'pattern' => 'v\\d+', + ], + ], + ], + ], + ], + 'parameters' => [ + 'versionParam' => [ + 'name' => 'version', + 'in' => 'path', + 'required' => true, + 'type' => 'string', + ], + ], + ]; + $api = new Swagger($apiDef); + $this->describer->describe($api); + + $describedData = $api->toArray(); + // only one parameter should remain as they were duplicates + $this->assertCount(1, $describedData['paths']['/api/{version}/product']['get']['parameters']); + } + + protected function setUp() + { + $this->describer = new ParameterRefMergeDescriber(); + } +} From 3710e95d267ac62c4cabbe9a71efafb55c46b137 Mon Sep 17 00:00:00 2001 From: Guilhem Niot Date: Wed, 10 Apr 2019 20:52:45 +0200 Subject: [PATCH 2/3] Rework PR --- Describer/ParameterRefMergeDescriber.php | 79 ------------------- Resources/config/fos_rest.xml | 2 +- Resources/config/php_doc.xml | 2 +- Resources/config/services.xml | 4 - RouteDescriber/RouteMetadataDescriber.php | 32 +++++++- .../ParameterRefMergeDescriberTest.php | 64 --------------- Tests/Functional/FunctionalTest.php | 5 ++ 7 files changed, 38 insertions(+), 150 deletions(-) delete mode 100644 Describer/ParameterRefMergeDescriber.php delete mode 100644 Tests/Describer/ParameterRefMergeDescriberTest.php diff --git a/Describer/ParameterRefMergeDescriber.php b/Describer/ParameterRefMergeDescriber.php deleted file mode 100644 index bebf8d3..0000000 --- a/Describer/ParameterRefMergeDescriber.php +++ /dev/null @@ -1,79 +0,0 @@ -getPaths() as $path) { - /** @var Operation $operation */ - foreach ($path->getOperations() as $operation) { - $this->checkOperation($api, $operation); - } - } - } - - /** - * This method removes parameters that also have a ref version as they will be duplicated otherwise. - */ - private function checkOperation(Swagger $api, Operation $operation) - { - $parametersToRemove = []; - - /** @var Parameter[] $globalParams */ - $globalParams = $api->getParameters(); - /** @var Parameters|Parameter[] $currentParams */ - $currentParams = $operation->getParameters(); - - foreach ($currentParams as $parameter) { - $ref = $parameter->getRef(); - - if (null === $ref) { - // we only concern ourselves with '$ref' parameters - continue; - } - - $ref = \mb_substr($ref, 13); // trim the '#/parameters/' part of ref - if (!isset($globalParams[$ref])) { - // this really shouldn't happen, if it does there will be other failures elsewhere, so just ignore here - continue; - } - - $refParameter = $globalParams[$ref]; - // param ids are in form {name}/{in} - $refParameterId = \sprintf('%s/%s', $refParameter->getName(), $refParameter->getIn()); - if ($currentParams->has($refParameterId)) { - // if we got here it means a duplicate parameter is directly defined, schedule it for removal - $parametersToRemove[] = $currentParams->get($refParameterId); - } - } - - foreach ($parametersToRemove as $parameterToRemove) { - $currentParams->remove($parameterToRemove); - } - } -} diff --git a/Resources/config/fos_rest.xml b/Resources/config/fos_rest.xml index ede8766..dbf8f2f 100644 --- a/Resources/config/fos_rest.xml +++ b/Resources/config/fos_rest.xml @@ -7,7 +7,7 @@ - + diff --git a/Resources/config/php_doc.xml b/Resources/config/php_doc.xml index 47b2624..2adc859 100644 --- a/Resources/config/php_doc.xml +++ b/Resources/config/php_doc.xml @@ -5,7 +5,7 @@ - + diff --git a/Resources/config/services.xml b/Resources/config/services.xml index c6ad025..978930e 100644 --- a/Resources/config/services.xml +++ b/Resources/config/services.xml @@ -32,10 +32,6 @@ - - - - diff --git a/RouteDescriber/RouteMetadataDescriber.php b/RouteDescriber/RouteMetadataDescriber.php index ed67dd2..180237c 100644 --- a/RouteDescriber/RouteMetadataDescriber.php +++ b/RouteDescriber/RouteMetadataDescriber.php @@ -14,6 +14,9 @@ namespace Nelmio\ApiDocBundle\RouteDescriber; use EXSyst\Component\Swagger\Swagger; 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; @@ -26,12 +29,39 @@ final class RouteMetadataDescriber implements RouteDescriberInterface $requirements = $route->getRequirements(); $compiledRoute = $route->compile(); + $globalParams = $api->getParameters(); + $existingParams = []; + foreach ($operation->getParameters() as $id => $parameter) { + $ref = $parameter->getRef(); + if (null === $ref) { + $existingParams[$id] = true; + + // we only concern ourselves with '$ref' parameters + continue; + } + + $ref = \mb_substr($ref, 13); // trim the '#/parameters/' part of ref + if (!isset($globalParams[$ref])) { + // this shouldn't happen, so just ignore here + continue; + } + + $refParameter = $globalParams[$ref]; + + // param ids are in form {name}/{in} + $existingParams[\sprintf('%s/%s', $refParameter->getName(), $refParameter->getIn())] = true; + } + // Don't include host requirements foreach ($compiledRoute->getPathVariables() as $pathVariable) { if ('_format' === $pathVariable) { continue; } + if (isset($existingParams[$pathVariable.'/path'])) { + continue; // ignore this param, it is already defined + } + $parameter = $operation->getParameters()->get($pathVariable, 'path'); $parameter->setRequired(true); @@ -39,7 +69,7 @@ final class RouteMetadataDescriber implements RouteDescriberInterface $parameter->setType('string'); } - if (isset($requirements[$pathVariable])) { + if (isset($requirements[$pathVariable]) && null === $parameter->getPattern()) { $parameter->setPattern($requirements[$pathVariable]); } } diff --git a/Tests/Describer/ParameterRefMergeDescriberTest.php b/Tests/Describer/ParameterRefMergeDescriberTest.php deleted file mode 100644 index bd2b666..0000000 --- a/Tests/Describer/ParameterRefMergeDescriberTest.php +++ /dev/null @@ -1,64 +0,0 @@ - '2.0', - 'info' => ['title' => 'Ref Test'], - 'paths' => [ - '/api/{version}/product' => [ - 'get' => [ - 'parameters' => [ - [ - '$ref' => '#/parameters/versionParam', - ], - [ - 'name' => 'version', - 'in' => 'path', - 'required' => true, - 'type' => 'string', - 'pattern' => 'v\\d+', - ], - ], - ], - ], - ], - 'parameters' => [ - 'versionParam' => [ - 'name' => 'version', - 'in' => 'path', - 'required' => true, - 'type' => 'string', - ], - ], - ]; - $api = new Swagger($apiDef); - $this->describer->describe($api); - - $describedData = $api->toArray(); - // only one parameter should remain as they were duplicates - $this->assertCount(1, $describedData['paths']['/api/{version}/product']['get']['parameters']); - } - - protected function setUp() - { - $this->describer = new ParameterRefMergeDescriber(); - } -} diff --git a/Tests/Functional/FunctionalTest.php b/Tests/Functional/FunctionalTest.php index c3ea09a..1f2536a 100644 --- a/Tests/Functional/FunctionalTest.php +++ b/Tests/Functional/FunctionalTest.php @@ -433,4 +433,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')); + } } From 94f7715f68b00bde5a8a48f6e8b3f273475e2a28 Mon Sep 17 00:00:00 2001 From: cyberemissary <25044236+cyberemissary@users.noreply.github.com> Date: Fri, 10 Jan 2020 11:53:34 -0500 Subject: [PATCH 3/3] Refactor Existing Ref parsing in RouteMetadataDescriber --- RouteDescriber/RouteMetadataDescriber.php | 70 ++++++++++++------- .../Functional/Controller/TestController.php | 12 ++++ Tests/Functional/SwaggerUiTest.php | 4 ++ 3 files changed, 61 insertions(+), 25 deletions(-) diff --git a/RouteDescriber/RouteMetadataDescriber.php b/RouteDescriber/RouteMetadataDescriber.php index 180237c..a226a32 100644 --- a/RouteDescriber/RouteMetadataDescriber.php +++ b/RouteDescriber/RouteMetadataDescriber.php @@ -11,7 +11,10 @@ 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; /** @@ -28,29 +31,7 @@ final class RouteMetadataDescriber implements RouteDescriberInterface $requirements = $route->getRequirements(); $compiledRoute = $route->compile(); - - $globalParams = $api->getParameters(); - $existingParams = []; - foreach ($operation->getParameters() as $id => $parameter) { - $ref = $parameter->getRef(); - if (null === $ref) { - $existingParams[$id] = true; - - // we only concern ourselves with '$ref' parameters - continue; - } - - $ref = \mb_substr($ref, 13); // trim the '#/parameters/' part of ref - if (!isset($globalParams[$ref])) { - // this shouldn't happen, so just ignore here - continue; - } - - $refParameter = $globalParams[$ref]; - - // param ids are in form {name}/{in} - $existingParams[\sprintf('%s/%s', $refParameter->getName(), $refParameter->getIn())] = true; - } + $existingParams = $this->getRefParams($api, $operation); // Don't include host requirements foreach ($compiledRoute->getPathVariables() as $pathVariable) { @@ -58,8 +39,14 @@ final class RouteMetadataDescriber implements RouteDescriberInterface continue; } - if (isset($existingParams[$pathVariable.'/path'])) { - continue; // ignore this param, it is already defined + $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'); @@ -75,4 +62,37 @@ final class RouteMetadataDescriber implements RouteDescriberInterface } } } + + /** + * 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; + } } diff --git a/Tests/Functional/Controller/TestController.php b/Tests/Functional/Controller/TestController.php index f7cf30d..8b2aaba 100644 --- a/Tests/Functional/Controller/TestController.php +++ b/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() + { + } } diff --git a/Tests/Functional/SwaggerUiTest.php b/Tests/Functional/SwaggerUiTest.php index cd29e7a..0ed8ce5 100644 --- a/Tests/Functional/SwaggerUiTest.php +++ b/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'],