From 1da4c1a629db200e8b7f22b66ee1dcb053914ddb Mon Sep 17 00:00:00 2001 From: soyuka Date: Mon, 15 Mar 2021 12:07:35 +0100 Subject: [PATCH 1/5] OpenAPI: Make sure we do not override defined parameters #4137 --- src/OpenApi/Factory/OpenApiFactory.php | 45 ++++++++++++++++++-- tests/OpenApi/Factory/OpenApiFactoryTest.php | 13 ++++-- 2 files changed, 50 insertions(+), 8 deletions(-) diff --git a/src/OpenApi/Factory/OpenApiFactory.php b/src/OpenApi/Factory/OpenApiFactory.php index 8dbb40492f2..f39fa531359 100644 --- a/src/OpenApi/Factory/OpenApiFactory.php +++ b/src/OpenApi/Factory/OpenApiFactory.php @@ -175,19 +175,42 @@ private function collectPaths(ResourceMetadata $resourceMetadata, string $resour // Set up parameters if (OperationType::ITEM === $operationType) { foreach ($identifiers as $parameterName => $identifier) { - $parameters[] = new Model\Parameter(\is_string($parameterName) ? $parameterName : $identifier, 'path', 'Resource identifier', true, false, false, ['type' => 'string']); + $parameterName = \is_string($parameterName) ? $parameterName : $identifier; + $parameter = new Model\Parameter($parameterName, 'path', 'Resource identifier', true, false, false, ['type' => 'string']); + if ($this->hasParameter($parameter, $parameters)) { + continue; + } + + $parameters[] = $parameter; } $links[$operationId] = $this->getLink($resourceClass, $operationId, $path); } elseif (OperationType::COLLECTION === $operationType && 'GET' === $method) { - $parameters = array_merge($parameters, $this->getPaginationParameters($resourceMetadata, $operationName), $this->getFiltersParameters($resourceMetadata, $operationName, $resourceClass)); + foreach (array_merge($this->getPaginationParameters($resourceMetadata, $operationName), $this->getFiltersParameters($resourceMetadata, $operationName, $resourceClass)) as $parameter) { + if ($this->hasParameter($parameter, $parameters)) { + continue; + } + + $parameters[] = $parameter; + } } elseif (OperationType::SUBRESOURCE === $operationType) { foreach ($operation['identifiers'] as $parameterName => [$class, $property]) { - $parameters[] = new Model\Parameter($parameterName, 'path', $this->resourceMetadataFactory->create($class)->getShortName().' identifier', true, false, false, ['type' => 'string']); + $parameter = new Model\Parameter($parameterName, 'path', $this->resourceMetadataFactory->create($class)->getShortName().' identifier', true, false, false, ['type' => 'string']); + if ($this->hasParameter($parameter, $parameters)) { + continue; + } + + $parameters[] = $parameter; } if ($operation['collection']) { $subresourceMetadata = $this->resourceMetadataFactory->create($resourceClass); - $parameters = array_merge($parameters, $this->getPaginationParameters($resourceMetadata, $operationName), $this->getFiltersParameters($subresourceMetadata, $operationName, $resourceClass)); + foreach (array_merge($this->getPaginationParameters($resourceMetadata, $operationName), $this->getFiltersParameters($subresourceMetadata, $operationName, $resourceClass)) as $parameter) { + if ($this->hasParameter($parameter, $parameters)) { + continue; + } + + $parameters[] = $parameter; + } } } @@ -486,4 +509,18 @@ private function getSecuritySchemes(): array return $securitySchemes; } + + /** + * @var Model\Parameter[] + */ + private function hasParameter(Model\Parameter $parameter, array $parameters): bool + { + foreach ($parameters as $existingParameter) { + if ($existingParameter->getName() === $parameter->getName() && $existingParameter->getIn() === $parameter->getIn()) { + return true; + } + } + + return false; + } } diff --git a/tests/OpenApi/Factory/OpenApiFactoryTest.php b/tests/OpenApi/Factory/OpenApiFactoryTest.php index d8b398d60fe..ebd4258725a 100644 --- a/tests/OpenApi/Factory/OpenApiFactoryTest.php +++ b/tests/OpenApi/Factory/OpenApiFactoryTest.php @@ -77,7 +77,8 @@ public function testInvoke(): void 'custom' => ['method' => 'HEAD', 'path' => '/foo/{id}', 'openapi_context' => [ 'description' => 'Custom description', 'parameters' => [ - ['description' => 'Test parameter', 'name' => 'param', 'in' => 'path', 'type' => 'string', 'required' => true, 'default' => 'BOTH'], + ['description' => 'Test parameter', 'name' => 'param', 'in' => 'path', 'required' => true], + ['description' => 'Replace parameter', 'name' => 'id', 'in' => 'path', 'required' => true, 'schema' => ['type' => 'string', 'format' => 'uuid']], ], 'tags' => ['Dummy', 'Profile'], 'responses' => [ @@ -117,7 +118,11 @@ public function testInvoke(): void 'formats' => ['method' => 'PUT', 'path' => '/formatted/{id}', 'output_formats' => ['json' => ['application/json'], 'csv' => ['text/csv']], 'input_formats' => ['json' => ['application/json'], 'csv' => ['text/csv']]], ], [ - 'get' => ['method' => 'GET'] + self::OPERATION_FORMATS, + 'get' => ['method' => 'GET', 'openapi_context' => [ + 'parameters' => [ + ['description' => 'Test modified collection page number', 'name' => 'page', 'in' => 'query', 'required' => false, 'schema' => ['type' => 'integer', 'default' => 1], 'allowEmptyValue' => true], + ], + ]] + self::OPERATION_FORMATS, 'post' => ['method' => 'POST'] + self::OPERATION_FORMATS, 'filtered' => ['method' => 'GET', 'filters' => ['f1', 'f2', 'f3', 'f4', 'f5'], 'path' => '/filtered'] + self::OPERATION_FORMATS, 'paginated' => ['method' => 'GET', 'pagination_client_enabled' => true, 'pagination_client_items_per_page' => true, 'pagination_items_per_page' => 20, 'pagination_maximum_items_per_page' => 80, 'path' => '/paginated'] + self::OPERATION_FORMATS, @@ -306,7 +311,7 @@ public function testInvoke(): void 'Retrieves the collection of Dummy resources.', null, [ - new Model\Parameter('page', 'query', 'The collection page number', false, false, true, [ + new Model\Parameter('page', 'query', 'Test modified collection page number', false, false, true, [ 'type' => 'integer', 'default' => 1, ]), @@ -434,7 +439,7 @@ public function testInvoke(): void 'Dummy', 'Custom description', null, - [new Model\Parameter('param', 'path', 'Test parameter', true), new Model\Parameter('id', 'path', 'Resource identifier', true, false, false, ['type' => 'string'])], + [new Model\Parameter('param', 'path', 'Test parameter', true), new Model\Parameter('id', 'path', 'Replace parameter', true, false, false, ['type' => 'string', 'format' => 'uuid'])], new Model\RequestBody('Custom request body', new \ArrayObject([ 'multipart/form-data' => [ 'schema' => [ From afdbdd194753b0655e8fdd341a348c0ff201b3ab Mon Sep 17 00:00:00 2001 From: soyuka Date: Mon, 15 Mar 2021 14:31:34 +0100 Subject: [PATCH 2/5] Fix #4124 --- src/OpenApi/Factory/OpenApiFactory.php | 2 +- src/OpenApi/Model/Encoding.php | 10 ++++++++++ src/OpenApi/Model/Parameter.php | 12 +++++++++++- tests/OpenApi/Factory/OpenApiFactoryTest.php | 4 ++-- 4 files changed, 24 insertions(+), 4 deletions(-) diff --git a/src/OpenApi/Factory/OpenApiFactory.php b/src/OpenApi/Factory/OpenApiFactory.php index f39fa531359..9f0446e4acb 100644 --- a/src/OpenApi/Factory/OpenApiFactory.php +++ b/src/OpenApi/Factory/OpenApiFactory.php @@ -419,7 +419,7 @@ private function getFiltersParameters(ResourceMetadata $resourceMetadata, string $schema, 'array' === $schema['type'] && \in_array($data['type'], [Type::BUILTIN_TYPE_ARRAY, Type::BUILTIN_TYPE_OBJECT], true) ? 'deepObject' : 'form', - 'array' === $schema['type'], + $data['openapi']['explode'] ?? ('array' === $schema['type']), $data['openapi']['allowReserved'] ?? false, $data['openapi']['example'] ?? null, isset($data['openapi']['examples'] diff --git a/src/OpenApi/Model/Encoding.php b/src/OpenApi/Model/Encoding.php index 12b8d91a1d2..38366265fa5 100644 --- a/src/OpenApi/Model/Encoding.php +++ b/src/OpenApi/Model/Encoding.php @@ -52,11 +52,21 @@ public function canExplode(): bool return $this->explode; } + public function getExplode(): bool + { + return $this->explode; + } + public function canAllowReserved(): bool { return $this->allowReserved; } + public function getAllowReserved(): bool + { + return $this->allowReserved; + } + public function withContentType(string $contentType): self { $clone = clone $this; diff --git a/src/OpenApi/Model/Parameter.php b/src/OpenApi/Model/Parameter.php index a44ab7ee8d1..c8fad4f8089 100644 --- a/src/OpenApi/Model/Parameter.php +++ b/src/OpenApi/Model/Parameter.php @@ -83,7 +83,7 @@ public function getDeprecated(): bool return $this->deprecated; } - public function canAllowEmptyValue(): bool + public function getAllowEmptyValue(): bool { return $this->allowEmptyValue; } @@ -103,11 +103,21 @@ public function canExplode(): bool return $this->explode; } + public function getExplode(): bool + { + return $this->explode; + } + public function canAllowReserved(): bool { return $this->allowReserved; } + public function getAllowReserved(): bool + { + return $this->allowReserved; + } + public function getExample() { return $this->example; diff --git a/tests/OpenApi/Factory/OpenApiFactoryTest.php b/tests/OpenApi/Factory/OpenApiFactoryTest.php index ebd4258725a..67b5a11756c 100644 --- a/tests/OpenApi/Factory/OpenApiFactoryTest.php +++ b/tests/OpenApi/Factory/OpenApiFactoryTest.php @@ -158,7 +158,7 @@ public function testInvoke(): void 'type' => 'string', 'required' => true, 'strategy' => 'exact', - 'openapi' => ['example' => 'bar', 'deprecated' => true, 'allowEmptyValue' => true, 'allowReserved' => true], + 'openapi' => ['example' => 'bar', 'deprecated' => true, 'allowEmptyValue' => true, 'allowReserved' => true, 'explode' => true], ]]), 'f2' => new DummyFilter(['ha' => [ 'property' => 'foo', @@ -517,7 +517,7 @@ public function testInvoke(): void ]), new Model\Parameter('name', 'query', '', true, true, true, [ 'type' => 'string', - ], 'form', false, true, 'bar'), + ], 'form', true, true, 'bar'), new Model\Parameter('ha', 'query', '', false, false, true, [ 'type' => 'integer', ]), From e5cf715f8662509e0263cb859e27cb0070215863 Mon Sep 17 00:00:00 2001 From: soyuka Date: Tue, 16 Mar 2021 09:54:04 +0100 Subject: [PATCH 3/5] Improve openapi performances --- src/JsonSchema/SchemaFactory.php | 6 ++---- src/JsonSchema/TypeFactory.php | 2 +- src/OpenApi/Factory/OpenApiFactory.php | 27 +++++++++++++++++--------- tests/JsonSchema/TypeFactoryTest.php | 2 +- 4 files changed, 22 insertions(+), 15 deletions(-) diff --git a/src/JsonSchema/SchemaFactory.php b/src/JsonSchema/SchemaFactory.php index a1ab3610e0b..b518c308c1b 100644 --- a/src/JsonSchema/SchemaFactory.php +++ b/src/JsonSchema/SchemaFactory.php @@ -80,7 +80,7 @@ public function buildSchema(string $className, string $format = 'json', string $ } $version = $schema->getVersion(); - $definitionName = $this->buildDefinitionName($className, $format, $type, $operationType, $operationName, $serializerContext); + $definitionName = $this->buildDefinitionName($className, $format, $inputOrOutputClass, $resourceMetadata, $serializerContext); if (null === $operationType || null === $operationName) { $method = Schema::TYPE_INPUT === $type ? 'POST' : 'GET'; @@ -239,10 +239,8 @@ private function buildPropertySchema(Schema $schema, string $definitionName, str $schema->getDefinitions()[$definitionName]['properties'][$normalizedPropertyName] = $propertySchema; } - private function buildDefinitionName(string $className, string $format = 'json', string $type = Schema::TYPE_OUTPUT, ?string $operationType = null, ?string $operationName = null, ?array $serializerContext = null): string + private function buildDefinitionName(string $className, string $format = 'json', ?string $inputOrOutputClass = null, ?ResourceMetadata $resourceMetadata = null, ?array $serializerContext = null): string { - [$resourceMetadata, $serializerContext,, $inputOrOutputClass] = $this->getMetadata($className, $type, $operationType, $operationName, $serializerContext); - $prefix = $resourceMetadata ? $resourceMetadata->getShortName() : (new \ReflectionClass($className))->getShortName(); if (null !== $inputOrOutputClass && $className !== $inputOrOutputClass) { $parts = explode('\\', $inputOrOutputClass); diff --git a/src/JsonSchema/TypeFactory.php b/src/JsonSchema/TypeFactory.php index 04e71d4e60f..8979534395c 100644 --- a/src/JsonSchema/TypeFactory.php +++ b/src/JsonSchema/TypeFactory.php @@ -142,7 +142,7 @@ private function getClassType(?string $className, string $format, ?bool $readabl throw new \LogicException('The schema factory must be injected by calling the "setSchemaFactory" method.'); } - $subSchema = $this->schemaFactory->buildSchema($className, $format, Schema::TYPE_OUTPUT, null, null, $subSchema, $serializerContext); + $subSchema = $this->schemaFactory->buildSchema($className, $format, Schema::TYPE_OUTPUT, null, null, $subSchema, $serializerContext, false); return ['$ref' => $subSchema['$ref']]; } diff --git a/src/OpenApi/Factory/OpenApiFactory.php b/src/OpenApi/Factory/OpenApiFactory.php index 9f0446e4acb..db18623e182 100644 --- a/src/OpenApi/Factory/OpenApiFactory.php +++ b/src/OpenApi/Factory/OpenApiFactory.php @@ -86,7 +86,7 @@ public function __invoke(array $context = []): OpenApi $servers = '/' === $baseUrl || '' === $baseUrl ? [new Model\Server('/')] : [new Model\Server($baseUrl)]; $paths = new Model\Paths(); $links = []; - $schemas = []; + $schemas = new \ArrayObject(); foreach ($this->resourceNameCollectionFactory->create() as $resourceClass) { $resourceMetadata = $this->resourceMetadataFactory->create($resourceClass); @@ -94,11 +94,11 @@ public function __invoke(array $context = []): OpenApi // Items needs to be parsed first to be able to reference the lines from the collection operation [$itemOperationLinks, $itemOperationSchemas] = $this->collectPaths($resourceMetadata, $resourceClass, OperationType::ITEM, $context, $paths, $links, $schemas); - $schemas += $itemOperationSchemas; + $this->appendSchemaDefinitions($schemas, $itemOperationSchemas); [$collectionOperationLinks, $collectionOperationSchemas] = $this->collectPaths($resourceMetadata, $resourceClass, OperationType::COLLECTION, $context, $paths, $links, $schemas); [$subresourceOperationLinks, $subresourceOperationSchemas] = $this->collectPaths($resourceMetadata, $resourceClass, OperationType::SUBRESOURCE, $context, $paths, $links, $schemas); - $schemas += $collectionOperationSchemas; + $this->appendSchemaDefinitions($schemas, $collectionOperationSchemas); } $securitySchemes = $this->getSecuritySchemes(); @@ -113,7 +113,7 @@ public function __invoke(array $context = []): OpenApi $servers, $paths, new Model\Components( - new \ArrayObject($schemas), + $schemas, new \ArrayObject(), new \ArrayObject(), new \ArrayObject(), @@ -128,7 +128,7 @@ public function __invoke(array $context = []): OpenApi /** * @return array | array */ - private function collectPaths(ResourceMetadata $resourceMetadata, string $resourceClass, string $operationType, array $context, Model\Paths $paths, array &$links, array $schemas = []): array + private function collectPaths(ResourceMetadata $resourceMetadata, string $resourceClass, string $operationType, array $context, Model\Paths $paths, array &$links, \ArrayObject $schemas): array { $resourceShortName = $resourceMetadata->getShortName(); $operations = OperationType::COLLECTION === $operationType ? $resourceMetadata->getCollectionOperations() : (OperationType::ITEM === $operationType ? $resourceMetadata->getItemOperations() : $this->subresourceOperationFactory->create($resourceClass)); @@ -155,12 +155,14 @@ private function collectPaths(ResourceMetadata $resourceMetadata, string $resour $linkedOperationId = 'get'.ucfirst($resourceShortName).ucfirst(OperationType::ITEM); $pathItem = $paths->getPath($path) ?: new Model\PathItem(); $forceSchemaCollection = OperationType::SUBRESOURCE === $operationType ? ($operation['collection'] ?? false) : false; + $schema = new Schema('openapi'); + $schema->setDefinitions($schemas); $operationOutputSchemas = []; foreach ($responseMimeTypes as $operationFormat) { - $operationOutputSchema = $this->jsonSchemaFactory->buildSchema($resourceClass, $operationFormat, Schema::TYPE_OUTPUT, $operationType, $operationName, new Schema('openapi'), null, $forceSchemaCollection); - $schemas += $operationOutputSchema->getDefinitions()->getArrayCopy(); + $operationOutputSchema = $this->jsonSchemaFactory->buildSchema($resourceClass, $operationFormat, Schema::TYPE_OUTPUT, $operationType, $operationName, $schema, null, $forceSchemaCollection); $operationOutputSchemas[$operationFormat] = $operationOutputSchema; + $this->appendSchemaDefinitions($schemas, $operationOutputSchema->getDefinitions()); } $parameters = []; @@ -264,9 +266,9 @@ private function collectPaths(ResourceMetadata $resourceMetadata, string $resour } elseif ('PUT' === $method || 'POST' === $method || 'PATCH' === $method) { $operationInputSchemas = []; foreach ($requestMimeTypes as $operationFormat) { - $operationInputSchema = $this->jsonSchemaFactory->buildSchema($resourceClass, $operationFormat, Schema::TYPE_INPUT, $operationType, $operationName, new Schema('openapi'), null, $forceSchemaCollection); - $schemas += $operationInputSchema->getDefinitions()->getArrayCopy(); + $operationInputSchema = $this->jsonSchemaFactory->buildSchema($resourceClass, $operationFormat, Schema::TYPE_INPUT, $operationType, $operationName, $schema, null, $forceSchemaCollection); $operationInputSchemas[$operationFormat] = $operationInputSchema; + $this->appendSchemaDefinitions($schemas, $operationInputSchema->getDefinitions()); } $requestBody = new Model\RequestBody(sprintf('The %s %s resource', 'POST' === $method ? 'new' : 'updated', $resourceShortName), $this->buildContent($requestMimeTypes, $operationInputSchemas), true); @@ -510,6 +512,13 @@ private function getSecuritySchemes(): array return $securitySchemes; } + private function appendSchemaDefinitions(\ArrayObject &$schemas, \ArrayObject $definitions): void + { + foreach ($definitions as $key => $value) { + $schemas[$key] = $value; + } + } + /** * @var Model\Parameter[] */ diff --git a/tests/JsonSchema/TypeFactoryTest.php b/tests/JsonSchema/TypeFactoryTest.php index 03ea3409d31..0a953ec860c 100644 --- a/tests/JsonSchema/TypeFactoryTest.php +++ b/tests/JsonSchema/TypeFactoryTest.php @@ -372,7 +372,7 @@ public function testGetClassType(): void { $schemaFactoryProphecy = $this->prophesize(SchemaFactoryInterface::class); - $schemaFactoryProphecy->buildSchema(Dummy::class, 'jsonld', Schema::TYPE_OUTPUT, null, null, Argument::type(Schema::class), ['foo' => 'bar'])->will(function (array $args) { + $schemaFactoryProphecy->buildSchema(Dummy::class, 'jsonld', Schema::TYPE_OUTPUT, null, null, Argument::type(Schema::class), ['foo' => 'bar'], false)->will(function (array $args) { $args[5]['$ref'] = 'ref'; return $args[5]; From d57626f93b5326eed897eb711c5db60939dd03a2 Mon Sep 17 00:00:00 2001 From: Alan Poulain Date: Tue, 16 Mar 2021 14:27:59 +0100 Subject: [PATCH 4/5] fix PHPDoc --- src/OpenApi/Factory/OpenApiFactory.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenApi/Factory/OpenApiFactory.php b/src/OpenApi/Factory/OpenApiFactory.php index db18623e182..bb5c1bede70 100644 --- a/src/OpenApi/Factory/OpenApiFactory.php +++ b/src/OpenApi/Factory/OpenApiFactory.php @@ -520,7 +520,7 @@ private function appendSchemaDefinitions(\ArrayObject &$schemas, \ArrayObject $d } /** - * @var Model\Parameter[] + * @param Model\Parameter[] $parameters */ private function hasParameter(Model\Parameter $parameter, array $parameters): bool { From 441ffa631c5b239b1e67cc13a8fe865a582f969a Mon Sep 17 00:00:00 2001 From: soyuka Date: Tue, 16 Mar 2021 17:17:19 +0100 Subject: [PATCH 5/5] remove ref --- src/OpenApi/Factory/OpenApiFactory.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenApi/Factory/OpenApiFactory.php b/src/OpenApi/Factory/OpenApiFactory.php index bb5c1bede70..863eddccbb4 100644 --- a/src/OpenApi/Factory/OpenApiFactory.php +++ b/src/OpenApi/Factory/OpenApiFactory.php @@ -512,7 +512,7 @@ private function getSecuritySchemes(): array return $securitySchemes; } - private function appendSchemaDefinitions(\ArrayObject &$schemas, \ArrayObject $definitions): void + private function appendSchemaDefinitions(\ArrayObject $schemas, \ArrayObject $definitions): void { foreach ($definitions as $key => $value) { $schemas[$key] = $value;