fix validation on 2 or more DTOs in request

pfilsx · Dec 5, 2022 · 84201ed · 84201ed
1 parent e0fbd2c
commit 84201ed
Show file tree

Hide file tree

Showing 7 changed files with 156 additions and 29 deletions.
diff --git a/src/Collector/ValidationCollector.php b/src/Collector/ValidationCollector.php
@@ -0,0 +1,43 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Pfilsx\DtoParamConverter\Collector;
+
+use Symfony\Component\Validator\ConstraintViolationInterface;
+use Symfony\Component\Validator\ConstraintViolationList;
+use Symfony\Component\Validator\ConstraintViolationListInterface;
+
+final class ValidationCollector
+{
+    private ConstraintViolationList $violations;
+
+    public function __construct()
+    {
+        $this->violations = new ConstraintViolationList();
+    }
+
+    public function addViolation(ConstraintViolationInterface $violation): self
+    {
+        $this->violations->add($violation);
+
+        return $this;
+    }
+
+    public function addAllViolations(ConstraintViolationListInterface $violations): self
+    {
+        $this->violations->addAll($violations);
+
+        return $this;
+    }
+
+    public function hasViolations(): bool
+    {
+        return $this->violations->count() > 0;
+    }
+
+    public function getViolations(): ConstraintViolationList
+    {
+        return $this->violations;
+    }
+}
diff --git a/src/EventSubscriber/ControllerEventSubscriber.php b/src/EventSubscriber/ControllerEventSubscriber.php
@@ -4,27 +4,53 @@
 
 namespace Pfilsx\DtoParamConverter\EventSubscriber;
 
+use Pfilsx\DtoParamConverter\Collector\ValidationCollector;
+use Pfilsx\DtoParamConverter\Configuration\Configuration;
 use Pfilsx\DtoParamConverter\Provider\RouteMetadataProvider;
 use Symfony\Component\EventDispatcher\EventSubscriberInterface;
+use Symfony\Component\HttpKernel\Event\ControllerArgumentsEvent;
 use Symfony\Component\HttpKernel\Event\KernelEvent;
 use Symfony\Component\HttpKernel\KernelEvents;
 
 final class ControllerEventSubscriber implements EventSubscriberInterface
 {
+    private Configuration $configuration;
+
     private RouteMetadataProvider $routeMetadataProvider;
 
-    public function __construct(RouteMetadataProvider $routeMetadataProvider)
-    {
+    private ValidationCollector $validationCollector;
+
+    public function __construct(
+        Configuration $configuration,
+        RouteMetadataProvider $routeMetadataProvider,
+        ValidationCollector $validationCollector
+    ) {
+        $this->configuration = $configuration;
         $this->routeMetadataProvider = $routeMetadataProvider;
+        $this->validationCollector = $validationCollector;
     }
 
     public static function getSubscribedEvents(): array
     {
         return [
             KernelEvents::CONTROLLER => 'onKernelController',
+            KernelEvents::CONTROLLER_ARGUMENTS => 'onControllerArguments',
         ];
     }
 
+    public function onControllerArguments(ControllerArgumentsEvent $event): void
+    {
+        if (!$this->validationCollector->hasViolations()) {
+            return;
+        }
+
+        $exceptionClass = $this->configuration->getValidationConfiguration()->getExceptionClass();
+        $exception = new $exceptionClass();
+        $exception->setViolations($this->validationCollector->getViolations());
+
+        throw $exception;
+    }
+
     public function onKernelController(KernelEvent $event): void
     {
         $controller = $event->getController();

diff --git a/src/Request/ArgumentResolver/DtoArgumentResolver.php b/src/Request/ArgumentResolver/DtoArgumentResolver.php
@@ -10,8 +10,8 @@
 use Doctrine\Persistence\ObjectManager;
 use LogicException;
 use Pfilsx\DtoParamConverter\Annotation\Dto;
+use Pfilsx\DtoParamConverter\Collector\ValidationCollector;
 use Pfilsx\DtoParamConverter\Configuration\Configuration;
-use Pfilsx\DtoParamConverter\Contract\ValidationExceptionInterface;
 use Pfilsx\DtoParamConverter\Factory\DtoMapperFactory;
 use Pfilsx\DtoParamConverter\Provider\DtoMetadataProvider;
 use Pfilsx\DtoParamConverter\Provider\RouteMetadataProvider;
@@ -63,6 +63,8 @@ final class DtoArgumentResolver implements ArgumentValueResolverInterface
 
     private DtoMapperFactory $mapperFactory;
 
+    private ValidationCollector $validationCollector;
+
     private ?ValidatorInterface $validator;
 
     private ?ManagerRegistry $registry;
@@ -79,6 +81,7 @@ public function __construct(
         DtoMetadataProvider $dtoMetadataProvider,
         RouteMetadataProvider $routeMetadataProvider,
         DtoMapperFactory $mapperFactory,
+        ValidationCollector $validationCollector,
         ?ValidatorInterface $validator = null,
         ?ManagerRegistry $registry = null,
         ?ExpressionLanguage $expressionLanguage = null,
@@ -88,6 +91,7 @@ public function __construct(
         $this->serializer = $serializer;
         $this->dtoMetadataProvider = $dtoMetadataProvider;
         $this->mapperFactory = $mapperFactory;
+        $this->validationCollector = $validationCollector;
         $this->validator = $validator;
         $this->registry = $registry;
         $this->language = $expressionLanguage;
@@ -148,22 +152,23 @@ public function resolve(Request $request, ArgumentMetadata $argument): iterable
                 $violations->add(new ConstraintViolation($message, '', $parameters, null, $exception->getPath(), null));
             }
 
-            throw $this->generateValidationException($violations);
+            $this->validationCollector->addAllViolations($violations);
+            $object = null;
         } catch (NotNormalizableValueException $exception) {
             $exceptionClass = $this->configuration->getSerializerConfiguration()->getNormalizerExceptionClass();
 
             throw new $exceptionClass($exception->getMessage(), 400, $exception);
         }
 
-        if ($this->isValidationRequired($className, $request)) {
+        if ($object !== null && $this->isValidationRequired($className, $request)) {
             $violations = $this->validator->validate(
                 $object,
                 null,
                 $this->getOption(self::OPTION_VALIDATOR_GROUPS, ['Default', $request->attributes->get('_route')])
             );
 
             if ($violations->count() !== 0) {
-                throw $this->generateValidationException($violations);
+                $this->validationCollector->addAllViolations($violations);
             }
         }
 
@@ -443,15 +448,6 @@ private function isValidationRequired(string $className, Request $request): bool
         return $validationConfiguration->isEnabled() && !\in_array($request->getMethod(), $validationConfiguration->getExcludedMethods(), true);
     }
 
-    private function generateValidationException(ConstraintViolationList $violations): ValidationExceptionInterface
-    {
-        $exceptionClass = $this->configuration->getValidationConfiguration()->getExceptionClass();
-        $exception = new $exceptionClass();
-        $exception->setViolations($violations);
-
-        throw $exception;
-    }
-
     /**
      * @param string $key
      * @param mixed  $default

diff --git a/src/Resources/config/services.xml b/src/Resources/config/services.xml
@@ -30,9 +30,13 @@
             <argument type="service" id="annotation_reader"/>
         </service>
 
+        <service id="pfilsx.dto_converter.validation_collector" class="Pfilsx\DtoParamConverter\Collector\ValidationCollector"/>
+
         <service id="pfilsx.dto_converter.controller.subscriber" class="Pfilsx\DtoParamConverter\EventSubscriber\ControllerEventSubscriber">
             <tag name="kernel.event_subscriber"/>
+            <argument type="service" id="pfilsx.dto_converter.configuration"/>
             <argument type="service" id="pfilsx.dto_converter.controller.route_metadata_provider"/>
+            <argument type="service" id="pfilsx.dto_converter.validation_collector"/>
         </service>
 
         <service id="pfilsx.dto_converter.expression_language.default" class="Symfony\Component\ExpressionLanguage\ExpressionLanguage" />
@@ -45,6 +49,7 @@
             <argument type="service" id="pfilsx.dto_converter.dto_metadata_provider"/>
             <argument type="service" id="pfilsx.dto_converter.controller.route_metadata_provider"/>
             <argument type="service" id="pfilsx.dto_converter.mapper_factory"/>
+            <argument type="service" id="pfilsx.dto_converter.validation_collector"/>
             <argument type="service" id="validator" on-invalid="null"/>
             <argument type="service" id="doctrine" on-invalid="null"/>
             <argument type="service" id="pfilsx.dto_converter.expression_language"

diff --git a/tests/Fixtures/Controller/SimpleController.php b/tests/Fixtures/Controller/SimpleController.php
@@ -120,10 +120,6 @@ public function postActionWithValidationDisabledInDto(TestAllDisabledDto $dto):
      *     DtoArgumentResolver::OPTION_ENTITY_EXPR: "repository.find(1)"
      * })
      *
-     * @DtoResolver("dto2", {
-     *     DtoArgumentResolver::OPTION_VALIDATE: false
-     * })
-     *
      * @param TestDto  $dto
      * @param Test2Dto $dto2
      *

diff --git a/tests/Functional/SimpleControllerTest.php b/tests/Functional/SimpleControllerTest.php
@@ -8,6 +8,7 @@
 use Pfilsx\DtoParamConverter\Tests\Fixtures\Controller\SimpleController;
 use Symfony\Bundle\FrameworkBundle\Test\WebTestCase;
 use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpFoundation\Response;
 
 final class SimpleControllerTest extends WebTestCase
 {
@@ -129,7 +130,7 @@ public function testPostActionWithValidationDisabledInDto(): void
     public function testPostActionWithMultipleDto(): void
     {
         $client = self::createClient();
-        $client->jsonRequest(Request::METHOD_POST, '/test/multiple', ['url' => 'test']);
+        $client->jsonRequest(Request::METHOD_POST, '/test/multiple', ['url' => 'http://test.test']);
 
         $this->assertResponseIsSuccessful();
         self::assertEquals([
@@ -138,11 +139,34 @@ public function testPostActionWithMultipleDto(): void
                 'value' => 10,
             ],
             'dto2' => [
-                'url' => 'test',
+                'url' => 'http://test.test',
             ],
         ], json_decode($client->getResponse()->getContent(), true));
     }
 
+    /**
+     * @see SimpleController::postActionWithMultipleDto()
+     */
+    public function testPostActionWithMultipleDtoOnInvalidData(): void
+    {
+        try {
+            $client = self::createClient();
+            $client->catchExceptions(false);
+            $client->jsonRequest(Request::METHOD_POST, '/test/multiple', ['title' => '', 'url' => '']);
+            self::assertResponseStatusCodeSame(Response::HTTP_BAD_REQUEST);
+        } catch (ConverterValidationException $exception) {
+            $result = [];
+            foreach ($exception->getViolations() as $violation) {
+                $result[$violation->getPropertyPath()][] = $violation->getMessage();
+            }
+
+            self::assertEquals([
+                'title' => ['This value should not be blank.'],
+                'url' => ['This value should not be blank.'],
+            ], $result);
+        }
+    }
+
     /**
      * @see SimpleController::patchAction()
      */

diff --git a/tests/Unit/Request/ArgumentResolver/DtoArgumentResolverTest.php b/tests/Unit/Request/ArgumentResolver/DtoArgumentResolverTest.php
@@ -10,6 +10,7 @@
 use Doctrine\Persistence\ManagerRegistry;
 use Doctrine\Persistence\Mapping\ClassMetadata;
 use Doctrine\Persistence\ObjectManager;
+use Pfilsx\DtoParamConverter\Collector\ValidationCollector;
 use Pfilsx\DtoParamConverter\Configuration\Configuration;
 use Pfilsx\DtoParamConverter\Exception\ConverterValidationException;
 use Pfilsx\DtoParamConverter\Factory\DtoMapperFactory;
@@ -56,6 +57,11 @@ final class DtoArgumentResolverTest extends TestCase
      */
     private $tokenStorage;
 
+    /**
+     * @var ValidationCollector
+     */
+    private $validationCollector;
+
     private DtoArgumentResolver $resolver;
 
     protected function setUp(): void
@@ -64,6 +70,7 @@ protected function setUp(): void
         $this->language = $this->getMockBuilder(ExpressionLanguage::class)->getMock();
         $this->serviceLocator = $this->getMockBuilder(ServiceLocator::class)->disableOriginalConstructor()->getMock();
         $this->tokenStorage = $this->getMockBuilder(TokenStorageInterface::class)->getMock();
+        $this->validationCollector = new ValidationCollector();
     }
 
     /**
@@ -241,14 +248,23 @@ public function providerTestApplyOnExpressionFailure(): array
 
     public function testApplyOnGetWithForcedValidation(): void
     {
-        self::expectException(ConverterValidationException::class);
-
         $request = $this->createRequest(Request::METHOD_GET, ['title' => 'Test', 'value' => 5]);
 
         $argumentMetadata = $this->createArgumentMetadataMock(TestDto::class);
         $this->initializeResolver(null, [DtoArgumentResolver::OPTION_PRELOAD_ENTITY => false, DtoArgumentResolver::OPTION_VALIDATE => true]);
 
         $this->resolver->resolve($request, $argumentMetadata)->current();
+
+        self::assertTrue($this->validationCollector->hasViolations());
+
+        $result = [];
+        foreach ($this->validationCollector->getViolations() as $violation) {
+            $result[$violation->getPropertyPath()][] = $violation->getMessage();
+        }
+
+        self::assertEquals([
+            'value' => ['This value should be greater than or equal to 10.'],
+        ], $result);
     }
 
     public function testApplyOnPost(): void
@@ -283,25 +299,45 @@ public function testApplyOnPostWithDisabledValidation(): void
      * @dataProvider providerTestApplyOnPostWithInvalidData
      *
      * @param array $requestData
+     * @param array $violations
      */
-    public function testApplyOnPostWithInvalidData(array $requestData): void
+    public function testApplyOnPostWithInvalidData(array $requestData, array $violations): void
     {
-        self::expectException(ConverterValidationException::class);
-
         $request = $this->createRequest(Request::METHOD_POST, $requestData);
 
         $argumentMetadata = $this->createArgumentMetadataMock(TestDto::class);
         $this->initializeResolver();
 
         $this->resolver->resolve($request, $argumentMetadata)->current();
+
+        self::assertTrue($this->validationCollector->hasViolations());
+
+        $result = [];
+        foreach ($this->validationCollector->getViolations() as $violation) {
+            $result[$violation->getPropertyPath()][] = $violation->getMessage();
+        }
+
+        self::assertEquals($violations, $result);
     }
 
     public function providerTestApplyOnPostWithInvalidData(): array
     {
         return [
-            'wrong type' => [['title' => 'Test', 'value' => 'test']],
-            'similar type' => [['title' => 'Test', 'value' => '20']],
-            'invalid value' => [['title' => '', 'value' => 5]],
+            'wrong type' => [
+                ['title' => 'Test', 'value' => 'test'],
+                ['value' => ['The type must be one of "int" ("string" given).']],
+            ],
+            'similar type' => [
+                ['title' => 'Test', 'value' => '20'],
+                ['value' => ['The type must be one of "int" ("string" given).']],
+            ],
+            'invalid value' => [
+                ['title' => '', 'value' => 5],
+                [
+                    'title' => ['This value should not be blank.'],
+                    'value' => ['This value should be greater than or equal to 10.'],
+                ],
+            ],
         ];
     }
 
@@ -388,6 +424,7 @@ private function initializeResolver(?Configuration $configuration = null, array
             new DtoMetadataProvider($reader),
             $this->initializeRouteMetadataProviderMock($routeOptions),
             new DtoMapperFactory($this->serviceLocator),
+            $this->validationCollector,
             Validation::createValidatorBuilder()->addLoader(new AnnotationLoader($reader))->getValidator(),
             $this->registry,
             $this->language,