From 978975ef01d7b9d230291676527aa1140a7e552f Mon Sep 17 00:00:00 2001 From: Vincent Amstoutz Date: Sat, 7 Dec 2024 12:04:27 +0100 Subject: [PATCH 1/8] fix(jsonschema): hashmaps produces invalid openapi schema (#6830) * fix(jsonschema): hashmaps produces invalid openapi schema * fix --------- Co-authored-by: soyuka --- .../Factory/SchemaPropertyMetadataFactory.php | 14 +++-- src/JsonSchema/SchemaFactory.php | 8 ++- .../TestBundle/ApiResource/Issue6800/Foo.php | 20 +++++++ .../TestApiDocHashmapArrayObjectIssue.php | 29 ++++++++++ .../Command/JsonSchemaGenerateCommandTest.php | 55 +++++++++++++++++++ 5 files changed, 118 insertions(+), 8 deletions(-) create mode 100644 tests/Fixtures/TestBundle/ApiResource/Issue6800/Foo.php create mode 100644 tests/Fixtures/TestBundle/ApiResource/Issue6800/TestApiDocHashmapArrayObjectIssue.php diff --git a/src/JsonSchema/Metadata/Property/Factory/SchemaPropertyMetadataFactory.php b/src/JsonSchema/Metadata/Property/Factory/SchemaPropertyMetadataFactory.php index 2be0459f5a3..b9f484571e2 100644 --- a/src/JsonSchema/Metadata/Property/Factory/SchemaPropertyMetadataFactory.php +++ b/src/JsonSchema/Metadata/Property/Factory/SchemaPropertyMetadataFactory.php @@ -34,8 +34,10 @@ final class SchemaPropertyMetadataFactory implements PropertyMetadataFactoryInte public const JSON_SCHEMA_USER_DEFINED = 'user_defined_schema'; - public function __construct(ResourceClassResolverInterface $resourceClassResolver, private readonly ?PropertyMetadataFactoryInterface $decorated = null) - { + public function __construct( + ResourceClassResolverInterface $resourceClassResolver, + private readonly ?PropertyMetadataFactoryInterface $decorated = null, + ) { $this->resourceClassResolver = $resourceClassResolver; } @@ -198,6 +200,8 @@ private function typeToArray(Type $type, ?bool $readableLink = null): array * Gets the JSON Schema document which specifies the data type corresponding to the given PHP class, and recursively adds needed new schema to the current schema if provided. * * Note: if the class is not part of exceptions listed above, any class is considered as a resource. + * + * @throws PropertyNotFoundException */ private function getClassType(?string $className, bool $nullable, ?bool $readableLink): array { @@ -240,7 +244,8 @@ private function getClassType(?string $className, bool $nullable, ?bool $readabl ]; } - if (!$this->isResourceClass($className) && is_a($className, \BackedEnum::class, true)) { + $isResourceClass = $this->isResourceClass($className); + if (!$isResourceClass && is_a($className, \BackedEnum::class, true)) { $enumCases = array_map(static fn (\BackedEnum $enum): string|int => $enum->value, $className::cases()); $type = \is_string($enumCases[0] ?? '') ? 'string' : 'integer'; @@ -255,7 +260,7 @@ private function getClassType(?string $className, bool $nullable, ?bool $readabl ]; } - if (true !== $readableLink && $this->isResourceClass($className)) { + if (true !== $readableLink && $isResourceClass) { return [ 'type' => 'string', 'format' => 'iri-reference', @@ -263,7 +268,6 @@ private function getClassType(?string $className, bool $nullable, ?bool $readabl ]; } - // TODO: add propertyNameCollectionFactory and recurse to find the underlying schema? Right now SchemaFactory does the job so we don't compute anything here. return ['type' => Schema::UNKNOWN_TYPE]; } diff --git a/src/JsonSchema/SchemaFactory.php b/src/JsonSchema/SchemaFactory.php index 9c6c6a38a35..21103240894 100644 --- a/src/JsonSchema/SchemaFactory.php +++ b/src/JsonSchema/SchemaFactory.php @@ -183,7 +183,8 @@ private function buildPropertySchema(Schema $schema, string $definitionName, str $propertySchemaType = $propertySchema['type'] ?? false; $isUnknown = Schema::UNKNOWN_TYPE === $propertySchemaType - || ('array' === $propertySchemaType && Schema::UNKNOWN_TYPE === ($propertySchema['items']['type'] ?? null)); + || ('array' === $propertySchemaType && Schema::UNKNOWN_TYPE === ($propertySchema['items']['type'] ?? null)) + || ('object' === $propertySchemaType && Schema::UNKNOWN_TYPE === ($propertySchema['additionalProperties']['type'] ?? null)); if ( !$isUnknown && ( @@ -241,8 +242,9 @@ private function buildPropertySchema(Schema $schema, string $definitionName, str } if ($isCollection) { - $propertySchema['items']['$ref'] = $subSchema['$ref']; - unset($propertySchema['items']['type']); + $key = ($propertySchema['type'] ?? null) === 'object' ? 'additionalProperties' : 'items'; + $propertySchema[$key]['$ref'] = $subSchema['$ref']; + unset($propertySchema[$key]['type']); break; } diff --git a/tests/Fixtures/TestBundle/ApiResource/Issue6800/Foo.php b/tests/Fixtures/TestBundle/ApiResource/Issue6800/Foo.php new file mode 100644 index 00000000000..20fd0fd1461 --- /dev/null +++ b/tests/Fixtures/TestBundle/ApiResource/Issue6800/Foo.php @@ -0,0 +1,20 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +declare(strict_types=1); + +namespace ApiPlatform\Tests\Fixtures\TestBundle\ApiResource\Issue6800; + +class Foo +{ + public string $bar; + public int $baz; +} diff --git a/tests/Fixtures/TestBundle/ApiResource/Issue6800/TestApiDocHashmapArrayObjectIssue.php b/tests/Fixtures/TestBundle/ApiResource/Issue6800/TestApiDocHashmapArrayObjectIssue.php new file mode 100644 index 00000000000..08fe6108bb1 --- /dev/null +++ b/tests/Fixtures/TestBundle/ApiResource/Issue6800/TestApiDocHashmapArrayObjectIssue.php @@ -0,0 +1,29 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +declare(strict_types=1); + +namespace ApiPlatform\Tests\Fixtures\TestBundle\ApiResource\Issue6800; + +use ApiPlatform\Metadata\Get; + +#[Get] +class TestApiDocHashmapArrayObjectIssue +{ + /** @var array */ + public array $foos; + + /** @var Foo[] */ + public array $fooOtherSyntax; + + /** @var array */ + public array $fooHashmaps; +} diff --git a/tests/JsonSchema/Command/JsonSchemaGenerateCommandTest.php b/tests/JsonSchema/Command/JsonSchemaGenerateCommandTest.php index 3f64481dc6a..5cc8f168e9a 100644 --- a/tests/JsonSchema/Command/JsonSchemaGenerateCommandTest.php +++ b/tests/JsonSchema/Command/JsonSchemaGenerateCommandTest.php @@ -13,6 +13,7 @@ namespace ApiPlatform\Tests\JsonSchema\Command; +use ApiPlatform\Tests\Fixtures\TestBundle\ApiResource\Issue6800\TestApiDocHashmapArrayObjectIssue; use ApiPlatform\Tests\Fixtures\TestBundle\Document\Dummy as DocumentDummy; use ApiPlatform\Tests\Fixtures\TestBundle\Entity\Dummy; use Symfony\Bundle\FrameworkBundle\Console\Application; @@ -345,4 +346,58 @@ public function testGenId(): void $json = json_decode($result, associative: true); $this->assertArrayNotHasKey('@id', $json['definitions']['DisableIdGenerationItem.jsonld']['properties']); } + + /** + * @dataProvider arrayPropertyTypeSyntaxProvider + */ + public function testOpenApiSchemaGenerationForArrayProperty(string $propertyName, array $expectedProperties): void + { + $this->tester->run([ + 'command' => 'api:json-schema:generate', + 'resource' => TestApiDocHashmapArrayObjectIssue::class, + '--operation' => '_api_/test_api_doc_hashmap_array_object_issues{._format}_get', + '--type' => 'output', + '--format' => 'jsonld', + ]); + + $result = $this->tester->getDisplay(); + $json = json_decode($result, true); + $definitions = $json['definitions']; + $ressourceDefinitions = $definitions['TestApiDocHashmapArrayObjectIssue.jsonld']; + + $this->assertArrayHasKey('TestApiDocHashmapArrayObjectIssue.jsonld', $definitions); + $this->assertEquals('object', $ressourceDefinitions['type']); + $this->assertEquals($expectedProperties, $ressourceDefinitions['properties'][$propertyName]); + } + + public static function arrayPropertyTypeSyntaxProvider(): \Generator + { + yield 'Array of Foo objects using array syntax' => [ + 'foos', + [ + 'type' => 'array', + 'items' => [ + '$ref' => '#/definitions/Foo.jsonld', + ], + ], + ]; + yield 'Array of Foo objects using Foo[] syntax' => [ + 'fooOtherSyntax', + [ + 'type' => 'array', + 'items' => [ + '$ref' => '#/definitions/Foo.jsonld', + ], + ], + ]; + yield 'Hashmap of Foo objects using array syntax' => [ + 'fooHashmaps', + [ + 'type' => 'object', + 'additionalProperties' => [ + '$ref' => '#/definitions/Foo.jsonld', + ], + ], + ]; + } } From a209dd440957176099247acf35b82611073352b1 Mon Sep 17 00:00:00 2001 From: mladencosa <39300405+mladencosa@users.noreply.github.com> Date: Mon, 9 Dec 2024 11:23:06 +0100 Subject: [PATCH 2/8] fix: add missing error normalizer trait and remove deprecated interface (#6853) --- src/JsonApi/Serializer/ErrorNormalizer.php | 1 - .../Serializer/ErrorNormalizerTrait.php | 53 +++++++++++++++++++ 2 files changed, 53 insertions(+), 1 deletion(-) create mode 100644 src/JsonApi/Serializer/ErrorNormalizerTrait.php diff --git a/src/JsonApi/Serializer/ErrorNormalizer.php b/src/JsonApi/Serializer/ErrorNormalizer.php index 19764afa3bf..6249a07231c 100644 --- a/src/JsonApi/Serializer/ErrorNormalizer.php +++ b/src/JsonApi/Serializer/ErrorNormalizer.php @@ -13,7 +13,6 @@ namespace ApiPlatform\JsonApi\Serializer; -use ApiPlatform\Problem\Serializer\ErrorNormalizerTrait; use ApiPlatform\Serializer\CacheableSupportsMethodInterface; use ApiPlatform\Symfony\Validator\Exception\ConstraintViolationListAwareExceptionInterface as LegacyConstraintViolationListAwareExceptionInterface; use ApiPlatform\Validator\Exception\ConstraintViolationListAwareExceptionInterface; diff --git a/src/JsonApi/Serializer/ErrorNormalizerTrait.php b/src/JsonApi/Serializer/ErrorNormalizerTrait.php new file mode 100644 index 00000000000..dfe5606c192 --- /dev/null +++ b/src/JsonApi/Serializer/ErrorNormalizerTrait.php @@ -0,0 +1,53 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +declare(strict_types=1); + +namespace ApiPlatform\JsonApi\Serializer; + +use Symfony\Component\ErrorHandler\Exception\FlattenException; +use Symfony\Component\HttpFoundation\Response; + +trait ErrorNormalizerTrait +{ + private function getErrorMessage($object, array $context, bool $debug = false): string + { + $message = $object->getMessage(); + + if ($debug) { + return $message; + } + + if ($object instanceof FlattenException) { + $statusCode = $context['statusCode'] ?? $object->getStatusCode(); + if ($statusCode >= 500 && $statusCode < 600) { + $message = Response::$statusTexts[$statusCode] ?? Response::$statusTexts[Response::HTTP_INTERNAL_SERVER_ERROR]; + } + } + + return $message; + } + + private function getErrorCode(object $object): ?string + { + if ($object instanceof FlattenException) { + return (string)$object->getStatusCode(); + } + + if ($object instanceof \Exception) { + $code = $object->getCode(); + return $code !== 0 ? (string)$code : null; + } + + return null; + } +} + From abbc031eece83b54781502cd6373b47a09e109f4 Mon Sep 17 00:00:00 2001 From: Antoine Bluchet Date: Mon, 9 Dec 2024 11:30:13 +0100 Subject: [PATCH 3/8] fix: test empty parameter against null (#6852) fixes #6844 --- src/Doctrine/Odm/Extension/ParameterExtension.php | 2 +- src/Doctrine/Orm/Extension/ParameterExtension.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Doctrine/Odm/Extension/ParameterExtension.php b/src/Doctrine/Odm/Extension/ParameterExtension.php index 0191871c07f..c5aaf08d169 100644 --- a/src/Doctrine/Odm/Extension/ParameterExtension.php +++ b/src/Doctrine/Odm/Extension/ParameterExtension.php @@ -36,7 +36,7 @@ public function __construct(private readonly ContainerInterface $filterLocator) private function applyFilter(Builder $aggregationBuilder, ?string $resourceClass = null, ?Operation $operation = null, array &$context = []): void { foreach ($operation->getParameters() ?? [] as $parameter) { - if (!($v = $parameter->getValue()) || $v instanceof ParameterNotFound) { + if (null === ($v = $parameter->getValue()) || $v instanceof ParameterNotFound) { continue; } diff --git a/src/Doctrine/Orm/Extension/ParameterExtension.php b/src/Doctrine/Orm/Extension/ParameterExtension.php index 6ae3a00100b..acd6aace9ec 100644 --- a/src/Doctrine/Orm/Extension/ParameterExtension.php +++ b/src/Doctrine/Orm/Extension/ParameterExtension.php @@ -41,7 +41,7 @@ public function __construct(private readonly ContainerInterface $filterLocator) private function applyFilter(QueryBuilder $queryBuilder, QueryNameGeneratorInterface $queryNameGenerator, string $resourceClass, ?Operation $operation = null, array $context = []): void { foreach ($operation?->getParameters() ?? [] as $parameter) { - if (!($v = $parameter->getValue()) || $v instanceof ParameterNotFound) { + if (null === ($v = $parameter->getValue()) || $v instanceof ParameterNotFound) { continue; } From a580a4e6615fe5a57c3b18e7ee4498b05f0be777 Mon Sep 17 00:00:00 2001 From: Michiel Kalle Date: Fri, 13 Dec 2024 10:07:55 +0100 Subject: [PATCH 4/8] Fix deprecation symfony/dependency-injection --- src/Symfony/Bundle/Resources/config/elasticsearch.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Symfony/Bundle/Resources/config/elasticsearch.xml b/src/Symfony/Bundle/Resources/config/elasticsearch.xml index d5fc1d0730f..2ea8d2cfd57 100644 --- a/src/Symfony/Bundle/Resources/config/elasticsearch.xml +++ b/src/Symfony/Bundle/Resources/config/elasticsearch.xml @@ -88,7 +88,7 @@ - + From 0ad0b4bc32256fb90b058047a7ab1de9398f7f34 Mon Sep 17 00:00:00 2001 From: soyuka Date: Fri, 13 Dec 2024 11:13:54 +0100 Subject: [PATCH 5/8] cs: run php-cs-fixer --- src/JsonApi/Serializer/ErrorNormalizerTrait.php | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/JsonApi/Serializer/ErrorNormalizerTrait.php b/src/JsonApi/Serializer/ErrorNormalizerTrait.php index dfe5606c192..8c5bef8fb70 100644 --- a/src/JsonApi/Serializer/ErrorNormalizerTrait.php +++ b/src/JsonApi/Serializer/ErrorNormalizerTrait.php @@ -13,6 +13,7 @@ namespace ApiPlatform\JsonApi\Serializer; +use ApiPlatform\Exception\ErrorCodeSerializableInterface; use Symfony\Component\ErrorHandler\Exception\FlattenException; use Symfony\Component\HttpFoundation\Response; @@ -39,15 +40,15 @@ private function getErrorMessage($object, array $context, bool $debug = false): private function getErrorCode(object $object): ?string { if ($object instanceof FlattenException) { - return (string)$object->getStatusCode(); + $exceptionClass = $object->getClass(); + } else { + $exceptionClass = $object::class; } - if ($object instanceof \Exception) { - $code = $object->getCode(); - return $code !== 0 ? (string)$code : null; + if (is_a($exceptionClass, ErrorCodeSerializableInterface::class, true)) { + return $exceptionClass::getErrorCode(); } return null; } } - From 5ce7596c8a1b8a066d5ea5d5f60a399045dd20e2 Mon Sep 17 00:00:00 2001 From: soyuka Date: Fri, 13 Dec 2024 11:33:37 +0100 Subject: [PATCH 6/8] ci: use problem error normalizer trait --- src/JsonApi/Serializer/ErrorNormalizerTrait.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/JsonApi/Serializer/ErrorNormalizerTrait.php b/src/JsonApi/Serializer/ErrorNormalizerTrait.php index 8c5bef8fb70..8af5cc524d2 100644 --- a/src/JsonApi/Serializer/ErrorNormalizerTrait.php +++ b/src/JsonApi/Serializer/ErrorNormalizerTrait.php @@ -17,6 +17,9 @@ use Symfony\Component\ErrorHandler\Exception\FlattenException; use Symfony\Component\HttpFoundation\Response; +/** + * @deprecated + */ trait ErrorNormalizerTrait { private function getErrorMessage($object, array $context, bool $debug = false): string From 4db72f55fa9dcd48518dc62b5bf472895b6a966b Mon Sep 17 00:00:00 2001 From: Antoine Bluchet Date: Fri, 13 Dec 2024 12:23:29 +0100 Subject: [PATCH 7/8] fix: filter may not use FilterInterface (#6858) * fix: filter may not use FilterInterface * add tests --- ...meterResourceMetadataCollectionFactory.php | 10 ++++++- ...ResourceMetadataCollectionFactoryTest.php} | 26 +++++++++++++++++-- src/OpenApi/Factory/OpenApiFactory.php | 15 ++++++----- 3 files changed, 42 insertions(+), 9 deletions(-) rename src/Metadata/Tests/Resource/Factory/{ParameterResourceMetadataCollectionFactoryTests.php => ParameterResourceMetadataCollectionFactoryTest.php} (64%) diff --git a/src/Metadata/Resource/Factory/ParameterResourceMetadataCollectionFactory.php b/src/Metadata/Resource/Factory/ParameterResourceMetadataCollectionFactory.php index f0eda744432..22ea107bc18 100644 --- a/src/Metadata/Resource/Factory/ParameterResourceMetadataCollectionFactory.php +++ b/src/Metadata/Resource/Factory/ParameterResourceMetadataCollectionFactory.php @@ -149,6 +149,10 @@ private function addFilterMetadata(Parameter $parameter): Parameter return $parameter; } + if (!\is_object($filterId) && !$this->filterLocator->has($filterId)) { + return $parameter; + } + $filter = \is_object($filterId) ? $filterId : $this->filterLocator->get($filterId); if (!$filter) { @@ -159,7 +163,7 @@ private function addFilterMetadata(Parameter $parameter): Parameter $parameter = $parameter->withSchema($schema); } - if (null === $parameter->getOpenApi() && $filter instanceof OpenApiParameterFilterInterface && ($openApiParameter = $filter->getOpenApiParameters($parameter)) && $openApiParameter instanceof OpenApiParameter) { + if (null === $parameter->getOpenApi() && $filter instanceof OpenApiParameterFilterInterface && ($openApiParameter = $filter->getOpenApiParameters($parameter))) { $parameter = $parameter->withOpenApi($openApiParameter); } @@ -190,6 +194,10 @@ private function setDefaults(string $key, Parameter $parameter, string $resource $parameter = $parameter->withSchema($schema); } + if (($openapi = $description[$key]['openapi'] ?? null) && null === $parameter->getOpenApi() && $openapi instanceof OpenApiParameter) { + $parameter = $parameter->withOpenApi($openapi); + } + $currentKey = $key; if (null === $parameter->getProperty() && isset($properties[$key])) { $parameter = $parameter->withProperty($key); diff --git a/src/Metadata/Tests/Resource/Factory/ParameterResourceMetadataCollectionFactoryTests.php b/src/Metadata/Tests/Resource/Factory/ParameterResourceMetadataCollectionFactoryTest.php similarity index 64% rename from src/Metadata/Tests/Resource/Factory/ParameterResourceMetadataCollectionFactoryTests.php rename to src/Metadata/Tests/Resource/Factory/ParameterResourceMetadataCollectionFactoryTest.php index d845833e69c..75f478df725 100644 --- a/src/Metadata/Tests/Resource/Factory/ParameterResourceMetadataCollectionFactoryTests.php +++ b/src/Metadata/Tests/Resource/Factory/ParameterResourceMetadataCollectionFactoryTest.php @@ -13,10 +13,12 @@ namespace ApiPlatform\Metadata\Tests\Resource\Factory; +use ApiPlatform\Metadata\ApiProperty; use ApiPlatform\Metadata\FilterInterface; use ApiPlatform\Metadata\Parameters; use ApiPlatform\Metadata\Property\Factory\PropertyMetadataFactoryInterface; use ApiPlatform\Metadata\Property\Factory\PropertyNameCollectionFactoryInterface; +use ApiPlatform\Metadata\Property\PropertyNameCollection; use ApiPlatform\Metadata\QueryParameter; use ApiPlatform\Metadata\Resource\Factory\AttributesResourceMetadataCollectionFactory; use ApiPlatform\Metadata\Resource\Factory\ParameterResourceMetadataCollectionFactory; @@ -25,12 +27,14 @@ use PHPUnit\Framework\TestCase; use Psr\Container\ContainerInterface; -class ParameterResourceMetadataCollectionFactoryTests extends TestCase +class ParameterResourceMetadataCollectionFactoryTest extends TestCase { public function testParameterFactory(): void { $nameCollection = $this->createStub(PropertyNameCollectionFactoryInterface::class); + $nameCollection->method('create')->willReturn(new PropertyNameCollection(['id', 'hydra', 'everywhere'])); $propertyMetadata = $this->createStub(PropertyMetadataFactoryInterface::class); + $propertyMetadata->method('create')->willReturnOnConsecutiveCalls(new ApiProperty(identifier: true), new ApiProperty(readable: true), new ApiProperty(readable: true)); $filterLocator = $this->createStub(ContainerInterface::class); $filterLocator->method('has')->willReturn(true); $filterLocator->method('get')->willReturn(new class implements FilterInterface { @@ -64,6 +68,24 @@ public function getDescription(string $resourceClass): array $this->assertEquals(['type' => 'foo'], $hydraParameter->getSchema()); $this->assertEquals(new Parameter('test', 'query'), $hydraParameter->getOpenApi()); $everywhere = $parameters->get('everywhere', QueryParameter::class); - $this->assertEquals(new Parameter('everywhere', 'query', allowEmptyValue: true), $everywhere->getOpenApi()); + $this->assertNull($everywhere->getOpenApi()); + } + + public function testParameterFactoryNoFilter(): void + { + $nameCollection = $this->createStub(PropertyNameCollectionFactoryInterface::class); + $nameCollection->method('create')->willReturn(new PropertyNameCollection(['id', 'hydra', 'everywhere'])); + $propertyMetadata = $this->createStub(PropertyMetadataFactoryInterface::class); + $propertyMetadata->method('create')->willReturnOnConsecutiveCalls(new ApiProperty(identifier: true), new ApiProperty(readable: true), new ApiProperty(readable: true)); + $filterLocator = $this->createStub(ContainerInterface::class); + $filterLocator->method('has')->willReturn(false); + $parameter = new ParameterResourceMetadataCollectionFactory( + $nameCollection, + $propertyMetadata, + new AttributesResourceMetadataCollectionFactory(), + $filterLocator + ); + $operation = $parameter->create(WithParameter::class)->getOperation('collection'); + $this->assertInstanceOf(Parameters::class, $parameters = $operation->getParameters()); } } diff --git a/src/OpenApi/Factory/OpenApiFactory.php b/src/OpenApi/Factory/OpenApiFactory.php index 3a07e9a088c..435cd4f133a 100644 --- a/src/OpenApi/Factory/OpenApiFactory.php +++ b/src/OpenApi/Factory/OpenApiFactory.php @@ -269,15 +269,18 @@ private function collectPaths(ApiResource $resource, ResourceMetadataCollection if (($f = $p->getFilter()) && \is_string($f) && $this->filterLocator && $this->filterLocator->has($f)) { $filter = $this->filterLocator->get($f); - foreach ($filter->getDescription($entityClass) as $name => $description) { - if ($prop = $p->getProperty()) { - $name = str_replace($prop, $key, $name); + + if ($d = $filter->getDescription($entityClass)) { + foreach ($d as $name => $description) { + if ($prop = $p->getProperty()) { + $name = str_replace($prop, $key, $name); + } + + $openapiParameters[] = $this->getFilterParameter($name, $description, $operation->getShortName(), $f); } - $openapiParameters[] = $this->getFilterParameter($name, $description, $operation->getShortName(), $f); + continue; } - - continue; } $in = $p instanceof HeaderParameterInterface ? 'header' : 'query'; From 22cbd0147ef6f817093533d62dc8279add67a647 Mon Sep 17 00:00:00 2001 From: Antoine Bluchet Date: Fri, 13 Dec 2024 14:06:17 +0100 Subject: [PATCH 8/8] fix(metadata): various parameter improvements (#6867) - `Parameter::getValue()` now takes a default value as argument `getValue(mixed $default = new ParameterNotFound()): mixed` - `Parametes::get(string $key, string $parameterClass = QueryParameter::class)` (but also `has` and `remove`) now has a default value as second argument to `QueryParameter::class` - Constraint violation had the wrong message when using `property`, fixed by using the `key` instead: --- features/filter/filter_validation.feature | 8 +++--- src/Metadata/Parameter.php | 4 +-- src/Metadata/Parameters.php | 6 ++-- src/Metadata/Tests/ParameterTest.php | 28 +++++++++++++++++++ src/Metadata/Tests/ParametersTest.php | 28 +++++++++++++++++++ .../State/ParameterValidatorProvider.php | 27 +++++++++++++++--- .../TestBundle/ApiResource/WithParameter.php | 22 +++++++++++++++ .../Functional/Parameters/ValidationTest.php | 10 ++++++- 8 files changed, 119 insertions(+), 14 deletions(-) create mode 100644 src/Metadata/Tests/ParameterTest.php create mode 100644 src/Metadata/Tests/ParametersTest.php diff --git a/features/filter/filter_validation.feature b/features/filter/filter_validation.feature index 1cdfab519e0..326c23982f8 100644 --- a/features/filter/filter_validation.feature +++ b/features/filter/filter_validation.feature @@ -25,15 +25,15 @@ Feature: Validate filters based upon filter description Scenario: Required filter should throw an error if not set When I am on "/array_filter_validators" Then the response status code should be 422 - And the JSON node "detail" should be equal to 'arrayRequired: This value should not be blank.\nindexedArrayRequired: This value should not be blank.' + And the JSON node "detail" should be equal to 'arrayRequired[]: This value should not be blank.\nindexedArrayRequired[foo]: This value should not be blank.' When I am on "/array_filter_validators?arrayRequired[foo]=foo" Then the response status code should be 422 - And the JSON node "detail" should be equal to 'indexedArrayRequired: This value should not be blank.' + And the JSON node "detail" should be equal to 'indexedArrayRequired[foo]: This value should not be blank.' When I am on "/array_filter_validators?arrayRequired[]=foo" Then the response status code should be 422 - And the JSON node "detail" should be equal to 'indexedArrayRequired: This value should not be blank.' + And the JSON node "detail" should be equal to 'indexedArrayRequired[foo]: This value should not be blank.' Scenario: Test filter bounds: maximum When I am on "/filter_validators?required=foo&required-allow-empty&maximum=10" @@ -49,7 +49,7 @@ Feature: Validate filters based upon filter description When I am on "/filter_validators?required=foo&required-allow-empty&exclusiveMaximum=10" Then the response status code should be 422 - And the JSON node "detail" should be equal to 'maximum: This value should be less than 10.' + And the JSON node "detail" should be equal to 'exclusiveMaximum: This value should be less than 10.' Scenario: Test filter bounds: minimum When I am on "/filter_validators?required=foo&required-allow-empty&minimum=5" diff --git a/src/Metadata/Parameter.php b/src/Metadata/Parameter.php index 135899b03fe..263a41e5dbf 100644 --- a/src/Metadata/Parameter.php +++ b/src/Metadata/Parameter.php @@ -124,9 +124,9 @@ public function getSecurityMessage(): ?string * * @readonly */ - public function getValue(): mixed + public function getValue(mixed $default = new ParameterNotFound()): mixed { - return $this->extraProperties['_api_values'] ?? new ParameterNotFound(); + return $this->extraProperties['_api_values'] ?? $default; } /** diff --git a/src/Metadata/Parameters.php b/src/Metadata/Parameters.php index bb8a1ec650e..8a13edf6f5d 100644 --- a/src/Metadata/Parameters.php +++ b/src/Metadata/Parameters.php @@ -70,7 +70,7 @@ public function add(string $key, Parameter $value): self /** * @param class-string $parameterClass */ - public function remove(string $key, string $parameterClass): self + public function remove(string $key, string $parameterClass = QueryParameter::class): self { foreach ($this->parameters as $i => [$parameterName, $parameter]) { if ($parameterName === $key && $parameterClass === $parameter::class) { @@ -86,7 +86,7 @@ public function remove(string $key, string $parameterClass): self /** * @param class-string $parameterClass */ - public function get(string $key, string $parameterClass): ?Parameter + public function get(string $key, string $parameterClass = QueryParameter::class): ?Parameter { foreach ($this->parameters as [$parameterName, $parameter]) { if ($parameterName === $key && $parameterClass === $parameter::class) { @@ -100,7 +100,7 @@ public function get(string $key, string $parameterClass): ?Parameter /** * @param class-string $parameterClass */ - public function has(string $key, string $parameterClass): bool + public function has(string $key, string $parameterClass = QueryParameter::class): bool { foreach ($this->parameters as [$parameterName, $parameter]) { if ($parameterName === $key && $parameterClass === $parameter::class) { diff --git a/src/Metadata/Tests/ParameterTest.php b/src/Metadata/Tests/ParameterTest.php new file mode 100644 index 00000000000..3e38798c16b --- /dev/null +++ b/src/Metadata/Tests/ParameterTest.php @@ -0,0 +1,28 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +declare(strict_types=1); + +namespace ApiPlatform\Metadata\Tests; + +use ApiPlatform\Metadata\QueryParameter; +use ApiPlatform\State\ParameterNotFound; +use PHPUnit\Framework\TestCase; + +class ParameterTest extends TestCase +{ + public function testDefaultValue(): void + { + $parameter = new QueryParameter(); + $this->assertSame('test', $parameter->getValue('test')); + $this->assertInstanceOf(ParameterNotFound::class, $parameter->getValue()); + } +} diff --git a/src/Metadata/Tests/ParametersTest.php b/src/Metadata/Tests/ParametersTest.php new file mode 100644 index 00000000000..6b7b5583390 --- /dev/null +++ b/src/Metadata/Tests/ParametersTest.php @@ -0,0 +1,28 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +declare(strict_types=1); + +namespace ApiPlatform\Metadata\Tests; + +use ApiPlatform\Metadata\Parameters; +use ApiPlatform\Metadata\QueryParameter; +use PHPUnit\Framework\TestCase; + +class ParametersTest extends TestCase +{ + public function testDefaultValue(): void + { + $r = new QueryParameter(); + $parameters = new Parameters(['a' => $r]); + $this->assertSame($r, $parameters->get('a')); + } +} diff --git a/src/Symfony/Validator/State/ParameterValidatorProvider.php b/src/Symfony/Validator/State/ParameterValidatorProvider.php index b88d3c217c9..950e5449729 100644 --- a/src/Symfony/Validator/State/ParameterValidatorProvider.php +++ b/src/Symfony/Validator/State/ParameterValidatorProvider.php @@ -14,12 +14,14 @@ namespace ApiPlatform\Symfony\Validator\State; use ApiPlatform\Metadata\Operation; +use ApiPlatform\Metadata\Parameter; use ApiPlatform\State\ParameterNotFound; use ApiPlatform\State\ProviderInterface; use ApiPlatform\State\Util\ParameterParserTrait; use ApiPlatform\Validator\Exception\ValidationException; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\Validator\ConstraintViolation; +use Symfony\Component\Validator\ConstraintViolationInterface; use Symfony\Component\Validator\ConstraintViolationList; use Symfony\Component\Validator\Validator\ValidatorInterface; @@ -55,7 +57,6 @@ public function provide(Operation $operation, array $uriVariables = [], array $c continue; } - $key = $parameter->getKey(); $value = $parameter->getValue(); if ($value instanceof ParameterNotFound) { $value = null; @@ -68,9 +69,7 @@ public function provide(Operation $operation, array $uriVariables = [], array $c $violation->getMessageTemplate(), $violation->getParameters(), $violation->getRoot(), - $parameter->getProperty() ?? ( - str_contains($key, ':property') ? str_replace('[:property]', $violation->getPropertyPath(), $key) : $key.$violation->getPropertyPath() - ), + $this->getProperty($parameter, $violation), $violation->getInvalidValue(), $violation->getPlural(), $violation->getCode(), @@ -86,4 +85,24 @@ public function provide(Operation $operation, array $uriVariables = [], array $c return $this->decorated->provide($operation, $uriVariables, $context); } + + // There's a `property` inside Parameter but it's used for hydra:search only as here we want the parameter name instead + private function getProperty(Parameter $parameter, ConstraintViolationInterface $violation): string + { + $key = $parameter->getKey(); + + if (str_contains($key, '[:property]')) { + return str_replace('[:property]', $violation->getPropertyPath(), $key); + } + + if (str_contains($key, ':property')) { + return str_replace(':property', $violation->getPropertyPath(), $key); + } + + if ($p = $violation->getPropertyPath()) { + return $p; + } + + return $key; + } } diff --git a/tests/Fixtures/TestBundle/ApiResource/WithParameter.php b/tests/Fixtures/TestBundle/ApiResource/WithParameter.php index 9b828885a17..27d2ae082b5 100644 --- a/tests/Fixtures/TestBundle/ApiResource/WithParameter.php +++ b/tests/Fixtures/TestBundle/ApiResource/WithParameter.php @@ -27,6 +27,7 @@ use Symfony\Component\HttpFoundation\JsonResponse; use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException; use Symfony\Component\Serializer\Attribute\Groups; +use Symfony\Component\Validator\Constraints as Assert; #[Get( uriTemplate: 'with_parameters/{id}{._format}', @@ -63,6 +64,7 @@ 'array' => new QueryParameter(schema: ['minItems' => 2, 'maxItems' => 3]), 'multipleOf' => new QueryParameter(schema: ['multipleOf' => 2]), 'pattern' => new QueryParameter(schema: ['pattern' => '/\d/']), + 'int' => new QueryParameter(property: 'a', constraints: [new Assert\Type('integer')], provider: [self::class, 'toInt']), ], provider: [self::class, 'collectionProvider'] )] @@ -132,4 +134,24 @@ public static function headerAndQueryProvider(Operation $operation, array $uriVa return new JsonResponse($values); } + + public static function toInt(Parameter $parameter, array $parameters = [], array $context = []): ?Operation + { + if (null === ($operation = $context['operation'] ?? null)) { + return null; + } + + $value = $parameter->getValue(); + + if (is_numeric($value)) { + $value = (int) $value; + } + + $parameters = $operation->getParameters(); + $parameters->add($parameter->getKey(), $parameter = $parameter->withExtraProperties( + $parameter->getExtraProperties() + ['_api_values' => $value] + )); + + return $operation->withParameters($parameters); + } } diff --git a/tests/Functional/Parameters/ValidationTest.php b/tests/Functional/Parameters/ValidationTest.php index 26edc53846e..2ccedcc572a 100644 --- a/tests/Functional/Parameters/ValidationTest.php +++ b/tests/Functional/Parameters/ValidationTest.php @@ -20,7 +20,7 @@ final class ValidationTest extends ApiTestCase public function testWithGroupFilter(): void { $response = self::createClient()->request('GET', 'with_parameters_collection'); - $this->assertArraySubset(['violations' => [['propertyPath' => 'a', 'message' => 'The parameter "hydra" is required.']]], $response->toArray(false)); + $this->assertArraySubset(['violations' => [['message' => 'The parameter "hydra" is required.']]], $response->toArray(false)); $response = self::createClient()->request('GET', 'with_parameters_collection?hydra'); $this->assertResponseIsSuccessful(); } @@ -134,4 +134,12 @@ public function testValidatePropertyPlaceholder(): void ], ], $response->toArray(false)); } + + public function testValidateMessage(): void + { + $response = self::createClient()->request('GET', 'validate_parameters?int=test'); + $this->assertArraySubset([ + 'detail' => 'int: This value should be of type integer.', + ], $response->toArray(false)); + } }