From 5b0b85aa2ae60dc8dd14242aa97e31d0cbba1331 Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Sun, 23 Apr 2023 11:21:54 +0200 Subject: [PATCH] [HttpKernel] Fix handling of `MapRequest*` attributes --- .../FrameworkExtension.php | 3 +- .../FrameworkBundle/Resources/config/web.php | 2 + .../HttpKernel/Attribute/MapQueryString.php | 3 + .../Attribute/MapRequestPayload.php | 3 + .../RequestPayloadValueResolver.php | 63 ++++++++++---- .../RequestPayloadValueResolverTest.php | 82 +++++++++++++++---- 6 files changed, 123 insertions(+), 33 deletions(-) diff --git a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php index 7acee5c278e07..668f2be322411 100644 --- a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php +++ b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php @@ -381,7 +381,8 @@ public function load(array $configs, ContainerBuilder $container) ); $container->getDefinition('argument_resolver.request_payload') - ->replaceArgument(0, new Reference('.argument_resolver.request_payload.no_serializer', ContainerInterface::RUNTIME_EXCEPTION_ON_INVALID_REFERENCE)); + ->replaceArgument(0, new Reference('.argument_resolver.request_payload.no_serializer', ContainerInterface::RUNTIME_EXCEPTION_ON_INVALID_REFERENCE)) + ->clearTag('kernel.event_subscriber'); $container->removeDefinition('console.command.serializer_debug'); } diff --git a/src/Symfony/Bundle/FrameworkBundle/Resources/config/web.php b/src/Symfony/Bundle/FrameworkBundle/Resources/config/web.php index 1044ded974721..a3a6ef7735612 100644 --- a/src/Symfony/Bundle/FrameworkBundle/Resources/config/web.php +++ b/src/Symfony/Bundle/FrameworkBundle/Resources/config/web.php @@ -70,6 +70,8 @@ service('translator')->nullOnInvalid(), ]) ->tag('controller.targeted_value_resolver', ['name' => RequestPayloadValueResolver::class]) + ->tag('kernel.event_subscriber') + ->lazy() ->set('argument_resolver.request_attribute', RequestAttributeValueResolver::class) ->tag('controller.argument_value_resolver', ['priority' => 100, 'name' => RequestAttributeValueResolver::class]) diff --git a/src/Symfony/Component/HttpKernel/Attribute/MapQueryString.php b/src/Symfony/Component/HttpKernel/Attribute/MapQueryString.php index f3717b31d0628..6ba85e1bddd9e 100644 --- a/src/Symfony/Component/HttpKernel/Attribute/MapQueryString.php +++ b/src/Symfony/Component/HttpKernel/Attribute/MapQueryString.php @@ -12,6 +12,7 @@ namespace Symfony\Component\HttpKernel\Attribute; use Symfony\Component\HttpKernel\Controller\ArgumentResolver\RequestPayloadValueResolver; +use Symfony\Component\HttpKernel\ControllerMetadata\ArgumentMetadata; use Symfony\Component\Validator\Constraints\GroupSequence; /** @@ -22,6 +23,8 @@ #[\Attribute(\Attribute::TARGET_PARAMETER)] class MapQueryString extends ValueResolver { + public ArgumentMetadata $metadata; + public function __construct( public readonly array $serializationContext = [], public readonly string|GroupSequence|array|null $validationGroups = null, diff --git a/src/Symfony/Component/HttpKernel/Attribute/MapRequestPayload.php b/src/Symfony/Component/HttpKernel/Attribute/MapRequestPayload.php index f02414343f0d1..02c01fa40bf39 100644 --- a/src/Symfony/Component/HttpKernel/Attribute/MapRequestPayload.php +++ b/src/Symfony/Component/HttpKernel/Attribute/MapRequestPayload.php @@ -12,6 +12,7 @@ namespace Symfony\Component\HttpKernel\Attribute; use Symfony\Component\HttpKernel\Controller\ArgumentResolver\RequestPayloadValueResolver; +use Symfony\Component\HttpKernel\ControllerMetadata\ArgumentMetadata; use Symfony\Component\Validator\Constraints\GroupSequence; /** @@ -22,6 +23,8 @@ #[\Attribute(\Attribute::TARGET_PARAMETER)] class MapRequestPayload extends ValueResolver { + public ArgumentMetadata $metadata; + public function __construct( public readonly array|string|null $acceptFormat = null, public readonly array $serializationContext = [], diff --git a/src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/RequestPayloadValueResolver.php b/src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/RequestPayloadValueResolver.php index 89a07c19f5ca9..9ae65c7dfdbf0 100644 --- a/src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/RequestPayloadValueResolver.php +++ b/src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/RequestPayloadValueResolver.php @@ -11,13 +11,16 @@ namespace Symfony\Component\HttpKernel\Controller\ArgumentResolver; +use Symfony\Component\EventDispatcher\EventSubscriberInterface; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; use Symfony\Component\HttpKernel\Attribute\MapQueryString; use Symfony\Component\HttpKernel\Attribute\MapRequestPayload; use Symfony\Component\HttpKernel\Controller\ValueResolverInterface; use Symfony\Component\HttpKernel\ControllerMetadata\ArgumentMetadata; +use Symfony\Component\HttpKernel\Event\ControllerArgumentsEvent; use Symfony\Component\HttpKernel\Exception\HttpException; +use Symfony\Component\HttpKernel\KernelEvents; use Symfony\Component\Serializer\Exception\NotEncodableValueException; use Symfony\Component\Serializer\Exception\PartialDenormalizationException; use Symfony\Component\Serializer\Exception\UnsupportedFormatException; @@ -31,8 +34,10 @@ /** * @author Konstantin Myakshin + * + * @final */ -final class RequestPayloadValueResolver implements ValueResolverInterface +class RequestPayloadValueResolver implements ValueResolverInterface, EventSubscriberInterface { /** * @see \Symfony\Component\Serializer\Normalizer\AbstractObjectNormalizer::DISABLE_TYPE_ENFORCEMENT @@ -59,24 +64,43 @@ public function __construct( public function resolve(Request $request, ArgumentMetadata $argument): iterable { - $payloadMappers = [ - MapQueryString::class => ['mapQueryString', Response::HTTP_NOT_FOUND], - MapRequestPayload::class => ['mapRequestPayload', Response::HTTP_UNPROCESSABLE_ENTITY], - ]; + $attribute = $argument->getAttributesOfType(MapQueryString::class, ArgumentMetadata::IS_INSTANCEOF)[0] + ?? $argument->getAttributesOfType(MapRequestPayload::class, ArgumentMetadata::IS_INSTANCEOF)[0] + ?? null; + + if (!$attribute) { + return []; + } + + $attribute->metadata = $argument; + + return [$attribute]; + } - foreach ($payloadMappers as $mappingAttribute => [$payloadMapper, $validationFailedCode]) { - if (!$attributes = $argument->getAttributesOfType($mappingAttribute, ArgumentMetadata::IS_INSTANCEOF)) { + public function onKernelControllerArguments(ControllerArgumentsEvent $event): void + { + $arguments = $event->getArguments(); + + foreach ($arguments as $i => $argument) { + if ($argument instanceof MapQueryString) { + $payloadMapper = 'mapQueryString'; + $validationFailedCode = Response::HTTP_NOT_FOUND; + } elseif ($argument instanceof MapRequestPayload) { + $payloadMapper = 'mapRequestPayload'; + $validationFailedCode = Response::HTTP_UNPROCESSABLE_ENTITY; + } else { continue; } + $request = $event->getRequest(); - if (!$type = $argument->getType()) { - throw new \LogicException(sprintf('Could not resolve the "$%s" controller argument: argument should be typed.', $argument->getName())); + if (!$type = $argument->metadata->getType()) { + throw new \LogicException(sprintf('Could not resolve the "$%s" controller argument: argument should be typed.', $argument->metadata->getName())); } if ($this->validator) { $violations = new ConstraintViolationList(); try { - $payload = $this->$payloadMapper($request, $type, $attributes[0]); + $payload = $this->$payloadMapper($request, $type, $argument); } catch (PartialDenormalizationException $e) { $trans = $this->translator ? $this->translator->trans(...) : fn ($m, $p) => strtr($m, $p); foreach ($e->getErrors() as $error) { @@ -92,7 +116,7 @@ public function resolve(Request $request, ArgumentMetadata $argument): iterable } if (null !== $payload) { - $violations->addAll($this->validator->validate($payload, null, $attributes[0]->validationGroups ?? null)); + $violations->addAll($this->validator->validate($payload, null, $argument->validationGroups ?? null)); } if (\count($violations)) { @@ -100,18 +124,27 @@ public function resolve(Request $request, ArgumentMetadata $argument): iterable } } else { try { - $payload = $this->$payloadMapper($request, $type, $attributes[0]); + $payload = $this->$payloadMapper($request, $type, $argument); } catch (PartialDenormalizationException $e) { throw new HttpException($validationFailedCode, implode("\n", array_map(static fn ($e) => $e->getMessage(), $e->getErrors())), $e); } } - if (null !== $payload || $argument->isNullable()) { - return [$payload]; + if (null === $payload && !$argument->metadata->isNullable()) { + throw new HttpException($validationFailedCode); } + + $arguments[$i] = $payload; } - return []; + $event->setArguments($arguments); + } + + public static function getSubscribedEvents(): array + { + return [ + KernelEvents::CONTROLLER_ARGUMENTS => 'onKernelControllerArguments', + ]; } private function mapQueryString(Request $request, string $type, MapQueryString $attribute): ?object diff --git a/src/Symfony/Component/HttpKernel/Tests/Controller/ArgumentResolver/RequestPayloadValueResolverTest.php b/src/Symfony/Component/HttpKernel/Tests/Controller/ArgumentResolver/RequestPayloadValueResolverTest.php index ecfaa3e0f621d..0f6fd60b75a74 100644 --- a/src/Symfony/Component/HttpKernel/Tests/Controller/ArgumentResolver/RequestPayloadValueResolverTest.php +++ b/src/Symfony/Component/HttpKernel/Tests/Controller/ArgumentResolver/RequestPayloadValueResolverTest.php @@ -18,7 +18,9 @@ use Symfony\Component\HttpKernel\Attribute\ValueResolver; use Symfony\Component\HttpKernel\Controller\ArgumentResolver\RequestPayloadValueResolver; use Symfony\Component\HttpKernel\ControllerMetadata\ArgumentMetadata; +use Symfony\Component\HttpKernel\Event\ControllerArgumentsEvent; use Symfony\Component\HttpKernel\Exception\HttpException; +use Symfony\Component\HttpKernel\HttpKernelInterface; use Symfony\Component\Serializer\Encoder\JsonEncoder; use Symfony\Component\Serializer\Encoder\XmlEncoder; use Symfony\Component\Serializer\Exception\PartialDenormalizationException; @@ -45,10 +47,14 @@ public function testNotTypedArgument() ]); $request = Request::create('/', 'POST', server: ['HTTP_CONTENT_TYPE' => 'application/json']); + $kernel = $this->createMock(HttpKernelInterface::class); + $arguments = $resolver->resolve($request, $argument); + $event = new ControllerArgumentsEvent($kernel, function () {}, $arguments, $request, HttpKernelInterface::MAIN_REQUEST); + $this->expectException(\LogicException::class); $this->expectExceptionMessage('Could not resolve the "$notTyped" controller argument: argument should be typed.'); - $resolver->resolve($request, $argument); + $resolver->onKernelControllerArguments($event); } public function testWithoutValidatorAndCouldNotDenormalize() @@ -63,8 +69,12 @@ public function testWithoutValidatorAndCouldNotDenormalize() ]); $request = Request::create('/', 'POST', server: ['CONTENT_TYPE' => 'application/json'], content: $content); + $kernel = $this->createMock(HttpKernelInterface::class); + $arguments = $resolver->resolve($request, $argument); + $event = new ControllerArgumentsEvent($kernel, function () {}, $arguments, $request, HttpKernelInterface::MAIN_REQUEST); + try { - $resolver->resolve($request, $argument); + $resolver->onKernelControllerArguments($event); $this->fail(sprintf('Expected "%s" to be thrown.', HttpException::class)); } catch (HttpException $e) { $this->assertInstanceOf(PartialDenormalizationException::class, $e->getPrevious()); @@ -90,8 +100,12 @@ public function testValidationNotPassed() ]); $request = Request::create('/', 'POST', server: ['CONTENT_TYPE' => 'application/json'], content: $content); + $kernel = $this->createMock(HttpKernelInterface::class); + $arguments = $resolver->resolve($request, $argument); + $event = new ControllerArgumentsEvent($kernel, function () {}, $arguments, $request, HttpKernelInterface::MAIN_REQUEST); + try { - $resolver->resolve($request, $argument); + $resolver->onKernelControllerArguments($event); $this->fail(sprintf('Expected "%s" to be thrown.', HttpException::class)); } catch (HttpException $e) { $validationFailedException = $e->getPrevious(); @@ -112,9 +126,12 @@ public function testUnsupportedMedia() ]); $request = Request::create('/', 'POST', server: ['CONTENT_TYPE' => 'foo/bar'], content: 'foo-bar'); - try { - $resolver->resolve($request, $argument); + $kernel = $this->createMock(HttpKernelInterface::class); + $arguments = $resolver->resolve($request, $argument); + $event = new ControllerArgumentsEvent($kernel, function () {}, $arguments, $request, HttpKernelInterface::MAIN_REQUEST); + try { + $resolver->onKernelControllerArguments($event); $this->fail(sprintf('Expected "%s" to be thrown.', HttpException::class)); } catch (HttpException $e) { $this->assertSame(415, $e->getStatusCode()); @@ -139,7 +156,13 @@ public function testRequestContentValidationPassed() ]); $request = Request::create('/', 'POST', server: ['CONTENT_TYPE' => 'application/json'], content: $content); - $this->assertEquals($payload, $resolver->resolve($request, $argument)[0]); + $kernel = $this->createMock(HttpKernelInterface::class); + $arguments = $resolver->resolve($request, $argument); + $event = new ControllerArgumentsEvent($kernel, function () {}, $arguments, $request, HttpKernelInterface::MAIN_REQUEST); + + $resolver->onKernelControllerArguments($event); + + $this->assertEquals([$payload], $event->getArguments()); } public function testQueryStringValidationPassed() @@ -161,7 +184,13 @@ public function testQueryStringValidationPassed() ]); $request = Request::create('/', 'GET', $query); - $this->assertEquals($payload, $resolver->resolve($request, $argument)[0]); + $kernel = $this->createMock(HttpKernelInterface::class); + $arguments = $resolver->resolve($request, $argument); + $event = new ControllerArgumentsEvent($kernel, function () {}, $arguments, $request, HttpKernelInterface::MAIN_REQUEST); + + $resolver->onKernelControllerArguments($event); + + $this->assertEquals([$payload], $event->getArguments()); } public function testRequestInputValidationPassed() @@ -183,7 +212,13 @@ public function testRequestInputValidationPassed() ]); $request = Request::create('/', 'POST', $input); - $this->assertEquals($payload, $resolver->resolve($request, $argument)[0]); + $kernel = $this->createMock(HttpKernelInterface::class); + $arguments = $resolver->resolve($request, $argument); + $event = new ControllerArgumentsEvent($kernel, function () {}, $arguments, $request, HttpKernelInterface::MAIN_REQUEST); + + $resolver->onKernelControllerArguments($event); + + $this->assertEquals([$payload], $event->getArguments()); } /** @@ -202,10 +237,13 @@ public function testAcceptFormatPassed(mixed $acceptFormat, string $contentType, MapRequestPayload::class => new MapRequestPayload(acceptFormat: $acceptFormat), ]); - $resolved = $resolver->resolve($request, $argument); + $kernel = $this->createMock(HttpKernelInterface::class); + $arguments = $resolver->resolve($request, $argument); + $event = new ControllerArgumentsEvent($kernel, function () {}, $arguments, $request, HttpKernelInterface::MAIN_REQUEST); + + $resolver->onKernelControllerArguments($event); - $this->assertCount(1, $resolved); - $this->assertEquals(new RequestPayload(50), $resolved[0]); + $this->assertEquals([new RequestPayload(50)], $event->getArguments()); } public static function provideMatchedFormatContext(): iterable @@ -262,9 +300,12 @@ public function testAcceptFormatNotPassed(mixed $acceptFormat, string $contentTy MapRequestPayload::class => new MapRequestPayload(acceptFormat: $acceptFormat), ]); - try { - $resolver->resolve($request, $argument); + $kernel = $this->createMock(HttpKernelInterface::class); + $arguments = $resolver->resolve($request, $argument); + $event = new ControllerArgumentsEvent($kernel, function () {}, $arguments, $request, HttpKernelInterface::MAIN_REQUEST); + try { + $resolver->onKernelControllerArguments($event); $this->fail(sprintf('Expected "%s" to be thrown.', HttpException::class)); } catch (HttpException $e) { $this->assertSame(415, $e->getStatusCode()); @@ -330,10 +371,13 @@ public function testValidationGroupsPassed(string $method, ValueResolver $attrib $attribute::class => $attribute, ]); - $resolved = $resolver->resolve($request, $argument); + $kernel = $this->createMock(HttpKernelInterface::class); + $arguments = $resolver->resolve($request, $argument); + $event = new ControllerArgumentsEvent($kernel, function () {}, $arguments, $request, HttpKernelInterface::MAIN_REQUEST); - $this->assertCount(1, $resolved); - $this->assertEquals($payload, $resolved[0]); + $resolver->onKernelControllerArguments($event); + + $this->assertEquals([$payload], $event->getArguments()); } /** @@ -352,8 +396,12 @@ public function testValidationGroupsNotPassed(string $method, ValueResolver $att ]); $request = Request::create('/', $method, $input); + $kernel = $this->createMock(HttpKernelInterface::class); + $arguments = $resolver->resolve($request, $argument); + $event = new ControllerArgumentsEvent($kernel, function () {}, $arguments, $request, HttpKernelInterface::MAIN_REQUEST); + try { - $resolver->resolve($request, $argument); + $resolver->onKernelControllerArguments($event); $this->fail(sprintf('Expected "%s" to be thrown.', HttpException::class)); } catch (HttpException $e) { $validationFailedException = $e->getPrevious();