From d3484b0f1bf06e518c83cd15e67ed10e9a75fe03 Mon Sep 17 00:00:00 2001 From: Antoine Bluchet Date: Fri, 29 Dec 2023 19:05:17 +0100 Subject: [PATCH 1/6] fix(serializer): integrate root_resource_class to cache key (#6073) * fix: operation cache key without operation name on related resources (de)normalization * test: cache key --------- Co-authored-by: Marius J --- src/Serializer/AbstractItemNormalizer.php | 9 ++- .../Serializer/AbstractItemNormalizerTest.php | 78 +++++++++++++++++++ 2 files changed, 83 insertions(+), 4 deletions(-) diff --git a/src/Serializer/AbstractItemNormalizer.php b/src/Serializer/AbstractItemNormalizer.php index c24fc563a4a..d5669e49eeb 100644 --- a/src/Serializer/AbstractItemNormalizer.php +++ b/src/Serializer/AbstractItemNormalizer.php @@ -580,9 +580,10 @@ protected function getFactoryOptions(array $context): array $options['serializer_groups'] = (array) $context[self::GROUPS]; } - $operationCacheKey = ($context['resource_class'] ?? '').($context['operation_name'] ?? '').($context['api_normalize'] ?? ''); - if ($operationCacheKey && isset($this->localFactoryOptionsCache[$operationCacheKey])) { - return $options + $this->localFactoryOptionsCache[$operationCacheKey]; + $operationCacheKey = ($context['resource_class'] ?? '').($context['operation_name'] ?? '').($context['root_operation_name'] ?? ''); + $suffix = ($context['api_normalize'] ?? '') ? 'n' : ''; + if ($operationCacheKey && isset($this->localFactoryOptionsCache[$operationCacheKey.$suffix])) { + return $options + $this->localFactoryOptionsCache[$operationCacheKey.$suffix]; } // This is a hot spot @@ -595,7 +596,7 @@ protected function getFactoryOptions(array $context): array } } - return $options + $this->localFactoryOptionsCache[$operationCacheKey] = $options; + return $options + $this->localFactoryOptionsCache[$operationCacheKey.$suffix] = $options; } /** diff --git a/tests/Serializer/AbstractItemNormalizerTest.php b/tests/Serializer/AbstractItemNormalizerTest.php index 66f074fdfec..4c82141317e 100644 --- a/tests/Serializer/AbstractItemNormalizerTest.php +++ b/tests/Serializer/AbstractItemNormalizerTest.php @@ -1399,6 +1399,84 @@ public function testDenormalizeObjectWithNullDisabledTypeEnforcement(): void $this->assertInstanceOf(DtoWithNullValue::class, $actual); $this->assertEquals(new DtoWithNullValue(), $actual); } + + public function testCacheKey(): void + { + $relatedDummy = new RelatedDummy(); + + $dummy = new Dummy(); + $dummy->setName('foo'); + $dummy->setAlias('ignored'); + $dummy->setRelatedDummy($relatedDummy); + $dummy->relatedDummies->add(new RelatedDummy()); + + $relatedDummies = new ArrayCollection([$relatedDummy]); + + $propertyNameCollectionFactoryProphecy = $this->prophesize(PropertyNameCollectionFactoryInterface::class); + $propertyNameCollectionFactoryProphecy->create(Dummy::class, Argument::type('array'))->willReturn(new PropertyNameCollection(['name', 'alias', 'relatedDummy', 'relatedDummies'])); + + $relatedDummyType = new Type(Type::BUILTIN_TYPE_OBJECT, false, RelatedDummy::class); + $relatedDummiesType = new Type(Type::BUILTIN_TYPE_OBJECT, false, ArrayCollection::class, true, new Type(Type::BUILTIN_TYPE_INT), $relatedDummyType); + + $propertyMetadataFactoryProphecy = $this->prophesize(PropertyMetadataFactoryInterface::class); + $propertyMetadataFactoryProphecy->create(Dummy::class, 'name', Argument::type('array'))->willReturn((new ApiProperty())->withBuiltinTypes([new Type(Type::BUILTIN_TYPE_STRING)])->withDescription('')->withReadable(true)); + $propertyMetadataFactoryProphecy->create(Dummy::class, 'alias', Argument::type('array'))->willReturn((new ApiProperty())->withBuiltinTypes([new Type(Type::BUILTIN_TYPE_STRING)])->withDescription('')->withReadable(true)); + $propertyMetadataFactoryProphecy->create(Dummy::class, 'relatedDummy', Argument::type('array'))->willReturn((new ApiProperty())->withBuiltinTypes([$relatedDummyType])->withDescription('')->withReadable(true)->withWritable(false)->withReadableLink(false)); + $propertyMetadataFactoryProphecy->create(Dummy::class, 'relatedDummies', Argument::type('array'))->willReturn((new ApiProperty())->withBuiltinTypes([$relatedDummiesType])->withReadable(true)->withWritable(false)->withReadableLink(false)); + + $iriConverterProphecy = $this->prophesize(IriConverterInterface::class); + $iriConverterProphecy->getIriFromResource($dummy, Argument::cetera())->willReturn('/dummies/1'); + $iriConverterProphecy->getIriFromResource($relatedDummy, Argument::cetera())->willReturn('/dummies/2'); + + $propertyAccessorProphecy = $this->prophesize(PropertyAccessorInterface::class); + $propertyAccessorProphecy->getValue($dummy, 'name')->willReturn('foo'); + $propertyAccessorProphecy->getValue($dummy, 'relatedDummy')->willReturn($relatedDummy); + $propertyAccessorProphecy->getValue($dummy, 'relatedDummies')->willReturn($relatedDummies); + + $resourceClassResolverProphecy = $this->prophesize(ResourceClassResolverInterface::class); + $resourceClassResolverProphecy->getResourceClass(null, Dummy::class)->willReturn(Dummy::class); + $resourceClassResolverProphecy->getResourceClass($dummy, null)->willReturn(Dummy::class); + $resourceClassResolverProphecy->getResourceClass($relatedDummy, RelatedDummy::class)->willReturn(RelatedDummy::class); + $resourceClassResolverProphecy->getResourceClass($relatedDummies, RelatedDummy::class)->willReturn(RelatedDummy::class); + $resourceClassResolverProphecy->isResourceClass(Dummy::class)->willReturn(true); + $resourceClassResolverProphecy->isResourceClass(RelatedDummy::class)->willReturn(true); + + $serializerProphecy = $this->prophesize(SerializerInterface::class); + $serializerProphecy->willImplement(NormalizerInterface::class); + $serializerProphecy->normalize('foo', null, Argument::type('array'))->willReturn('foo'); + $serializerProphecy->normalize(['/dummies/2'], null, Argument::type('array'))->willReturn(['/dummies/2']); + + $normalizer = $this->getMockForAbstractClass(AbstractItemNormalizer::class, [ + $propertyNameCollectionFactoryProphecy->reveal(), + $propertyMetadataFactoryProphecy->reveal(), + $iriConverterProphecy->reveal(), + $resourceClassResolverProphecy->reveal(), + $propertyAccessorProphecy->reveal(), + null, + null, + [], + null, + null, + ]); + $normalizer->setSerializer($serializerProphecy->reveal()); + + $expected = [ + 'name' => 'foo', + 'relatedDummy' => '/dummies/2', + 'relatedDummies' => ['/dummies/2'], + ]; + $this->assertSame($expected, $normalizer->normalize($dummy, null, [ + 'resources' => [], + 'groups' => ['group'], + 'ignored_attributes' => ['alias'], + 'operation_name' => 'operation_name', + 'root_operation_name' => 'root_operation_name', + ])); + + $operationCacheKey = (new \ReflectionClass($normalizer))->getProperty('localFactoryOptionsCache')->getValue($normalizer); + $this->assertEquals(array_keys($operationCacheKey), [sprintf('%s%s%s%s', Dummy::class, 'operation_name', 'root_operation_name', 'n')]); + $this->assertEquals(current($operationCacheKey), ['serializer_groups' => ['group']]); + } } class ObjectWithBasicProperties From cd01e043a17f4092bf302a415bba777fab3a9cfc Mon Sep 17 00:00:00 2001 From: Priyadi Iman Nurcahyo Date: Fri, 5 Jan 2024 16:04:46 +0700 Subject: [PATCH 2/6] fix(symfony): handle empty content-type as set by Symfony (#6078) * fix: requests with an empty content type In PHP, requests without content-type will result in a content-type header containing an empty string, not null * test: add tests for content type containing an empty string * test: added a test for DeserializeListener --- .../Provider/ContentNegotiationProvider.php | 3 +- src/State/Provider/DeserializeProvider.php | 2 +- .../EventListener/DeserializeListener.php | 2 +- .../ContentNegotiationProviderTest.php | 63 +++++++++++++++++++ .../Provider/DeserializeProviderTest.php | 63 +++++++++++++++++++ .../EventListener/DeserializeListenerTest.php | 53 ++++++++++++++++ 6 files changed, 183 insertions(+), 3 deletions(-) create mode 100644 tests/State/Provider/ContentNegotiationProviderTest.php create mode 100644 tests/State/Provider/DeserializeProviderTest.php diff --git a/src/State/Provider/ContentNegotiationProvider.php b/src/State/Provider/ContentNegotiationProvider.php index 8b3cb533ff8..05da2a02b65 100644 --- a/src/State/Provider/ContentNegotiationProvider.php +++ b/src/State/Provider/ContentNegotiationProvider.php @@ -97,7 +97,8 @@ private function flattenMimeTypes(array $formats): array */ private function getInputFormat(HttpOperation $operation, Request $request): ?string { - if (null === ($contentType = $request->headers->get('CONTENT_TYPE'))) { + $contentType = $request->headers->get('CONTENT_TYPE'); + if (null === $contentType || '' === $contentType) { return null; } diff --git a/src/State/Provider/DeserializeProvider.php b/src/State/Provider/DeserializeProvider.php index b2217275051..09a3f7b7f51 100644 --- a/src/State/Provider/DeserializeProvider.php +++ b/src/State/Provider/DeserializeProvider.php @@ -56,7 +56,7 @@ public function provide(Operation $operation, array $uriVariables = [], array $c } $contentType = $request->headers->get('CONTENT_TYPE'); - if (null === $contentType) { + if (null === $contentType || '' === $contentType) { throw new UnsupportedMediaTypeHttpException('The "Content-Type" header must exist.'); } diff --git a/src/Symfony/EventListener/DeserializeListener.php b/src/Symfony/EventListener/DeserializeListener.php index a246ad9fef3..00b53ec42eb 100644 --- a/src/Symfony/EventListener/DeserializeListener.php +++ b/src/Symfony/EventListener/DeserializeListener.php @@ -132,7 +132,7 @@ private function getFormat(Request $request, array $formats): string { /** @var ?string $contentType */ $contentType = $request->headers->get('CONTENT_TYPE'); - if (null === $contentType) { + if (null === $contentType || '' === $contentType) { throw new UnsupportedMediaTypeHttpException('The "Content-Type" header must exist.'); } diff --git a/tests/State/Provider/ContentNegotiationProviderTest.php b/tests/State/Provider/ContentNegotiationProviderTest.php new file mode 100644 index 00000000000..e4eb99ba904 --- /dev/null +++ b/tests/State/Provider/ContentNegotiationProviderTest.php @@ -0,0 +1,63 @@ + + * + * 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\State\Provider; + +use ApiPlatform\Metadata\Post; +use ApiPlatform\State\Provider\ContentNegotiationProvider; +use ApiPlatform\State\ProviderInterface; +use Negotiation\Negotiator; +use PHPUnit\Framework\TestCase; +use Prophecy\Argument; +use Prophecy\PhpUnit\ProphecyTrait; +use Symfony\Component\HttpFoundation\Request; + +class ContentNegotiationProviderTest extends TestCase +{ + use ProphecyTrait; + + public function testRequestWithEmptyContentType(): void + { + $expectedResult = new \stdClass(); + + $decorated = $this->prophesize(ProviderInterface::class); + $decorated->provide(Argument::cetera())->willReturn($expectedResult); + + $negotiator = new Negotiator(); + $formats = ['jsonld' => ['application/ld+json']]; + $errorFormats = ['jsonld' => ['application/ld+json']]; + + $provider = new ContentNegotiationProvider($decorated->reveal(), $negotiator, $formats, $errorFormats); + + // in Symfony (at least up to 7.0.2, 6.4.2, 6.3.11, 5.4.34), a request + // without a content-type and content-length header will result in the + // variables set to an empty string, not null + + $request = new Request( + server: [ + 'REQUEST_METHOD' => 'POST', + 'REQUEST_URI' => '/', + 'CONTENT_TYPE' => '', + 'CONTENT_LENGTH' => '', + ], + content: '' + ); + + $operation = new Post(); + $context = ['request' => $request]; + + $result = $provider->provide($operation, [], $context); + + $this->assertSame($expectedResult, $result); + } +} diff --git a/tests/State/Provider/DeserializeProviderTest.php b/tests/State/Provider/DeserializeProviderTest.php new file mode 100644 index 00000000000..b94c7473ca9 --- /dev/null +++ b/tests/State/Provider/DeserializeProviderTest.php @@ -0,0 +1,63 @@ + + * + * 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\State\Provider; + +use ApiPlatform\Metadata\Post; +use ApiPlatform\Serializer\SerializerContextBuilderInterface; +use ApiPlatform\State\Provider\DeserializeProvider; +use ApiPlatform\State\ProviderInterface; +use PHPUnit\Framework\TestCase; +use Prophecy\Argument; +use Prophecy\PhpUnit\ProphecyTrait; +use Symfony\Component\HttpFoundation\Request; +use Symfony\Component\HttpKernel\Exception\UnsupportedMediaTypeHttpException; +use Symfony\Component\Serializer\SerializerInterface; + +class DeserializeProviderTest extends TestCase +{ + use ProphecyTrait; + + public function testRequestWithEmptyContentType(): void + { + $expectedResult = new \stdClass(); + + $decorated = $this->prophesize(ProviderInterface::class); + $decorated->provide(Argument::cetera())->willReturn($expectedResult); + + $serializer = $this->prophesize(SerializerInterface::class); + $serializerContextBuilder = $this->prophesize(SerializerContextBuilderInterface::class); + + $provider = new DeserializeProvider($decorated->reveal(), $serializer->reveal(), $serializerContextBuilder->reveal()); + + // in Symfony (at least up to 7.0.2, 6.4.2, 6.3.11, 5.4.34), a request + // without a content-type and content-length header will result in the + // variables set to an empty string, not null + + $request = new Request( + server: [ + 'REQUEST_METHOD' => 'POST', + 'REQUEST_URI' => '/', + 'CONTENT_TYPE' => '', + 'CONTENT_LENGTH' => '', + ], + content: '' + ); + + $operation = new Post(deserialize: true); + $context = ['request' => $request]; + + $this->expectException(UnsupportedMediaTypeHttpException::class); + $result = $provider->provide($operation, [], $context); + } +} diff --git a/tests/Symfony/EventListener/DeserializeListenerTest.php b/tests/Symfony/EventListener/DeserializeListenerTest.php index 8290d47b801..f12489bb6e0 100644 --- a/tests/Symfony/EventListener/DeserializeListenerTest.php +++ b/tests/Symfony/EventListener/DeserializeListenerTest.php @@ -28,6 +28,7 @@ use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpKernel\Event\RequestEvent; use Symfony\Component\HttpKernel\Exception\UnsupportedMediaTypeHttpException; +use Symfony\Component\HttpKernel\HttpKernelInterface; use Symfony\Component\Serializer\Exception\NotNormalizableValueException; use Symfony\Component\Serializer\Exception\PartialDenormalizationException; use Symfony\Component\Serializer\Normalizer\AbstractNormalizer; @@ -369,4 +370,56 @@ public function testTurnPartialDenormalizationExceptionIntoValidationException() $this->assertSame($violation->getCode(), 'ba785a8c-82cb-4283-967c-3cf342181b40'); } } + + public function testRequestWithEmptyContentType(): void + { + $serializerProphecy = $this->prophesize(SerializerInterface::class); + $serializerProphecy->deserialize(Argument::cetera())->shouldNotBeCalled(); + + $serializerContextBuilderProphecy = $this->prophesize(SerializerContextBuilderInterface::class); + $serializerContextBuilderProphecy->createFromRequest(Argument::cetera())->willReturn([]); + + $resourceMetadataFactoryProphecy = $this->prophesize(ResourceMetadataCollectionFactoryInterface::class); + $resourceMetadataFactoryProphecy->create(Argument::cetera())->willReturn(new ResourceMetadataCollection(Dummy::class, [ + new ApiResource(operations: [ + 'post' => new Post(inputFormats: self::FORMATS), + ]), + ]))->shouldBeCalled(); + + $listener = new DeserializeListener( + $serializerProphecy->reveal(), + $serializerContextBuilderProphecy->reveal(), + $resourceMetadataFactoryProphecy->reveal() + ); + + // in Symfony (at least up to 7.0.2, 6.4.2, 6.3.11, 5.4.34), a request + // without a content-type and content-length header will result in the + // variables set to an empty string, not null + + $request = new Request( + server: [ + 'REQUEST_METHOD' => 'POST', + 'REQUEST_URI' => '/', + 'CONTENT_TYPE' => '', + 'CONTENT_LENGTH' => '', + ], + attributes: [ + '_api_resource_class' => Dummy::class, + '_api_operation_name' => 'post', + '_api_receive' => true, + ], + content: '' + ); + + $event = new RequestEvent( + $this->prophesize(HttpKernelInterface::class)->reveal(), + $request, + \defined(HttpKernelInterface::class.'::MAIN_REQUEST') ? HttpKernelInterface::MAIN_REQUEST : HttpKernelInterface::MASTER_REQUEST, + ); + + $this->expectException(UnsupportedMediaTypeHttpException::class); + $this->expectExceptionMessage('The "Content-Type" header must exist.'); + + $listener->onKernelRequest($event); + } } From c6867aba079d317955f67a1701960bdbd717eb99 Mon Sep 17 00:00:00 2001 From: soyuka Date: Fri, 5 Jan 2024 10:10:58 +0100 Subject: [PATCH 3/6] cs: various fixes --- docs/public/index.php | 3 ++- src/deprecation.php | 4 ++-- tests/State/RespondProcessorTest.php | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/docs/public/index.php b/docs/public/index.php index 84aa91a9a02..379ffa30365 100644 --- a/docs/public/index.php +++ b/docs/public/index.php @@ -17,9 +17,10 @@ require __DIR__.'/../vendor/autoload.php'; use ApiPlatform\Playground\Kernel; +use RuntimeException; if (!($guide = $_SERVER['APP_GUIDE'] ?? $_ENV['APP_GUIDE'] ?? null)) { - throw new \RuntimeException('No guide.'); + throw new RuntimeException('No guide.'); } $app = function (array $context) use ($guide) { diff --git a/src/deprecation.php b/src/deprecation.php index 2b6daa5a99c..8a17557598b 100644 --- a/src/deprecation.php +++ b/src/deprecation.php @@ -12,8 +12,8 @@ declare(strict_types=1); $deprecatedClassesWithAliases = [ - \ApiPlatform\HttpCache\EventListener\AddHeadersListener::class => \ApiPlatform\Symfony\EventListener\AddHeadersListener::class, - \ApiPlatform\HttpCache\EventListener\AddTagsListener::class => \ApiPlatform\Symfony\EventListener\AddTagsListener::class, + ApiPlatform\HttpCache\EventListener\AddHeadersListener::class => \ApiPlatform\Symfony\EventListener\AddHeadersListener::class, + ApiPlatform\HttpCache\EventListener\AddTagsListener::class => \ApiPlatform\Symfony\EventListener\AddTagsListener::class, ]; spl_autoload_register(function ($className) use ($deprecatedClassesWithAliases): void { diff --git a/tests/State/RespondProcessorTest.php b/tests/State/RespondProcessorTest.php index 935d4109640..0550b6fee45 100644 --- a/tests/State/RespondProcessorTest.php +++ b/tests/State/RespondProcessorTest.php @@ -67,7 +67,7 @@ public function testRedirectToOperation(): void $iriConverter = $this->prophesize(IriConverterInterface::class); $iriConverter ->getIriFromResource(Argument::cetera()) - ->will(static function (array $args): ?string { + ->will(static function (array $args): string { return ($args[2] ?? null)?->getUriTemplate() ?? '/default'; }); From a98ef1aef0e2f9a0e00a164befdd80470130c52f Mon Sep 17 00:00:00 2001 From: soyuka Date: Fri, 5 Jan 2024 10:18:34 +0100 Subject: [PATCH 4/6] cs: various fixes --- src/deprecation.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/deprecation.php b/src/deprecation.php index 8a17557598b..5b83c239df5 100644 --- a/src/deprecation.php +++ b/src/deprecation.php @@ -12,8 +12,8 @@ declare(strict_types=1); $deprecatedClassesWithAliases = [ - ApiPlatform\HttpCache\EventListener\AddHeadersListener::class => \ApiPlatform\Symfony\EventListener\AddHeadersListener::class, - ApiPlatform\HttpCache\EventListener\AddTagsListener::class => \ApiPlatform\Symfony\EventListener\AddTagsListener::class, + ApiPlatform\HttpCache\EventListener\AddHeadersListener::class => ApiPlatform\Symfony\EventListener\AddHeadersListener::class, + ApiPlatform\HttpCache\EventListener\AddTagsListener::class => ApiPlatform\Symfony\EventListener\AddTagsListener::class, ]; spl_autoload_register(function ($className) use ($deprecatedClassesWithAliases): void { From 3754bdaf876d97b2c347e8a48a180bf894492caa Mon Sep 17 00:00:00 2001 From: soyuka Date: Mon, 8 Jan 2024 15:32:04 +0100 Subject: [PATCH 5/6] chore: php-parser interface breaks --- phpunit.xml.dist | 1 + 1 file changed, 1 insertion(+) diff --git a/phpunit.xml.dist b/phpunit.xml.dist index 42d3a440cc9..d2ec73ce596 100644 --- a/phpunit.xml.dist +++ b/phpunit.xml.dist @@ -11,6 +11,7 @@ + From 2aed3c1fb2e49b860943555aaef3ba74cbfe6ea7 Mon Sep 17 00:00:00 2001 From: soyuka Date: Mon, 8 Jan 2024 16:25:25 +0100 Subject: [PATCH 6/6] style: cs --- docs/public/index.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/public/index.php b/docs/public/index.php index 84aa91a9a02..34f36a9d45f 100644 --- a/docs/public/index.php +++ b/docs/public/index.php @@ -19,7 +19,7 @@ use ApiPlatform\Playground\Kernel; if (!($guide = $_SERVER['APP_GUIDE'] ?? $_ENV['APP_GUIDE'] ?? null)) { - throw new \RuntimeException('No guide.'); + throw new RuntimeException('No guide.'); } $app = function (array $context) use ($guide) {