From 11423f08f83906acd80609c04c3ca9ddcc5e7903 Mon Sep 17 00:00:00 2001 From: "Nek (Maxime Veber)" Date: Tue, 18 Aug 2020 20:28:47 +0200 Subject: [PATCH] feat(form): Api request handler In Symfony Forms the request is managed by a request handler. This commit adds a custom request handler for melodiia. This avoids to make too much code while handling a request and also fixes #3 ! --- CHANGELOG.md | 6 ++ composer.json | 2 +- src/Bridge/Symfony/Form/Type/ApiType.php | 9 ++ src/Crud/Controller/BaseCrudController.php | 25 +---- src/Form/ApiRequestHandler.php | 45 +++++++++ tests/Melodiia/Crud/Controller/CreateTest.php | 31 +------ tests/Melodiia/Crud/Controller/UpdateTest.php | 91 +------------------ tests/Melodiia/Form/ApiRequestHandlerTest.php | 82 +++++++++++++++++ 8 files changed, 151 insertions(+), 140 deletions(-) create mode 100644 src/Form/ApiRequestHandler.php create mode 100644 tests/Melodiia/Form/ApiRequestHandlerTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 20ce5d2..5b26095 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - BC Break: on CRUD classes. It's big changes time. So we made the security optional for crud controllers, this has a consequence on their constructor - BC Break: `CrudableModelInterface` is now `MelodiiaModel` +- The ApiType now uses a custom request handler. It makes handling api request easier and removes some code + in controllers. You should now use `handleRequest($request)` on your form. Here is a list of other changes: + - An api form cannot be "not submitted" anymore after a call to `handleRequest` + - In case the input data is invalid, a form error is add to the form + - There is a new optional option on the ApiType: `clear_missing` but its value is automatically guessed by default + to follow HTTP verbs. ## [0.6.0] 2020-06-01 ### Added diff --git a/composer.json b/composer.json index 17d82a7..da38173 100644 --- a/composer.json +++ b/composer.json @@ -8,7 +8,7 @@ "post-install-cmd": "make hooks.install" }, "require": { - "php": ">=7.2", + "php": ">=7.3", "zircote/swagger-php": "^3.0.3", "nekland/tools": "^2.5.1", "zendframework/zend-json": "^3.0", diff --git a/src/Bridge/Symfony/Form/Type/ApiType.php b/src/Bridge/Symfony/Form/Type/ApiType.php index 488e23c..41b4d4f 100644 --- a/src/Bridge/Symfony/Form/Type/ApiType.php +++ b/src/Bridge/Symfony/Form/Type/ApiType.php @@ -4,6 +4,7 @@ use SwagIndustries\Melodiia\Bridge\Symfony\Form\DomainObjectDataMapperInterface; use SwagIndustries\Melodiia\Bridge\Symfony\Form\DomainObjectsDataMapper; +use SwagIndustries\Melodiia\Form\ApiRequestHandler; use Symfony\Component\Form\AbstractType; use Symfony\Component\Form\DataMapperInterface; use Symfony\Component\Form\FormBuilderInterface; @@ -12,6 +13,8 @@ class ApiType extends AbstractType { + public const CLEAR_MISSING_OPTION = 'clear_missing'; + /** @var DataMapperInterface */ private $dataMapper; @@ -31,6 +34,8 @@ public function buildForm(FormBuilderInterface $builder, array $options) if ($options['value_object']) { $builder->setDataMapper($dataMapper); } + + $builder->setRequestHandler(new ApiRequestHandler()); } public function configureOptions(OptionsResolver $resolver) @@ -40,6 +45,10 @@ public function configureOptions(OptionsResolver $resolver) 'value_object' => false, 'melodiiaDataMapper' => null, + // If set to false, all fields will be required, in the request, otherwise some may be removed. + // By default is null, but it will be automatically guess by the request handler. + self::CLEAR_MISSING_OPTION => null, + /* * Creates data object just like standard form would do * but used constructor with given data. diff --git a/src/Crud/Controller/BaseCrudController.php b/src/Crud/Controller/BaseCrudController.php index 8ba914d..0f5731d 100644 --- a/src/Crud/Controller/BaseCrudController.php +++ b/src/Crud/Controller/BaseCrudController.php @@ -5,13 +5,10 @@ use SwagIndustries\Melodiia\Bridge\Symfony\Response\FormErrorResponse; use SwagIndustries\Melodiia\Crud\CrudControllerInterface; use SwagIndustries\Melodiia\Response\ApiResponse; -use SwagIndustries\Melodiia\Response\WrongDataInput; use Symfony\Component\EventDispatcher\EventDispatcherInterface; use Symfony\Component\Form\FormFactoryInterface; use Symfony\Component\Form\FormInterface; use Symfony\Component\HttpFoundation\Request; -use Zend\Json\Exception\RuntimeException; -use Zend\Json\Json; abstract class BaseCrudController implements CrudControllerInterface { @@ -28,23 +25,11 @@ public function __construct(EventDispatcherInterface $eventDispatcher) */ protected function decodeInputData(FormFactoryInterface $formFactory, string $form, Request $request, bool $clearMissing = null, object $data = null) { - if (null === $clearMissing) { - $clearMissing = !in_array($request->getMethod(), ['POST', 'PUT']); - } - try { - $form = $formFactory->createNamed('', $form, $data); - $inputData = Json::decode($request->getContent(), Json::TYPE_ARRAY); - $form->submit($inputData, $clearMissing); - - if (!$form->isSubmitted()) { - return new WrongDataInput(); - } - - if (!$form->isValid()) { - return new FormErrorResponse($form); - } - } catch (RuntimeException $e) { - return new WrongDataInput(); + $form = $formFactory->createNamed('', $form, $data, ['clear_missing' => $clearMissing]); + $form->handleRequest($request); + + if (!$form->isValid()) { + return new FormErrorResponse($form); } return $form; diff --git a/src/Form/ApiRequestHandler.php b/src/Form/ApiRequestHandler.php new file mode 100644 index 0000000..40cf87d --- /dev/null +++ b/src/Form/ApiRequestHandler.php @@ -0,0 +1,45 @@ +getConfig()->getOption(ApiType::CLEAR_MISSING_OPTION); + if (null === $clearMissing) { + $clearMissing = !in_array($request->getMethod(), ['POST', 'PUT']); + } + + try { + $payload = \json_decode($request->getContent(), true, 512, JSON_THROW_ON_ERROR); + + $form->submit($payload, $clearMissing); + } catch (\JsonException $e) { + $form->addError(new FormError( + 'Invalid request content', + )); + $form->submit(null, false); + } + } + + /** + * Notice: this impacts allow_file_upload field. + * {@inheritdoc} + */ + public function isFileUpload($data) + { + return false; + } +} diff --git a/tests/Melodiia/Crud/Controller/CreateTest.php b/tests/Melodiia/Crud/Controller/CreateTest.php index ee4509a..4647d6f 100644 --- a/tests/Melodiia/Crud/Controller/CreateTest.php +++ b/tests/Melodiia/Crud/Controller/CreateTest.php @@ -66,8 +66,7 @@ public function setUp() $this->attributes->get(CrudControllerInterface::SECURITY_CHECK, null)->willReturn(null); $this->attributes->getBoolean(CrudControllerInterface::FORM_CLEAR_MISSING, true)->willReturn(true); $this->request->attributes = $this->attributes->reveal(); - $this->request->getContent()->willReturn('{"awesome":"json"}'); - $this->form->submit(['awesome' => 'json'], true)->willReturn(); + $this->form->handleRequest($this->request)->willReturn(); $this->formFactory->createNamed('', Argument::cetera())->willReturn($this->form); $this->controller = new Create( @@ -78,17 +77,6 @@ public function setUp() ); } - public function testItReturn400OnNotSubmittedForm() - { - $this->form->isSubmitted()->willReturn(false); - - /** @var ApiResponse $res */ - $res = ($this->controller)($this->request->reveal()); - - $this->assertInstanceOf(ApiResponse::class, $res); - $this->assertEquals(400, $res->httpStatus()); - } - public function testItReturn400OnInvalidForm() { $this->form->isSubmitted()->willReturn(true); @@ -101,22 +89,8 @@ public function testItReturn400OnInvalidForm() $this->assertEquals(400, $res->httpStatus()); } - /** - * Issue #28. - */ - public function testItReturnProperlyOnWrongInput() - { - $this->request->getContent()->willReturn('{"awesome":json"}'); // Wrong JSON - - /** @var ApiResponse $res */ - $res = ($this->controller)($this->request->reveal(), 'id'); - $this->assertInstanceOf(ApiResponse::class, $res); - $this->assertEquals(400, $res->httpStatus()); - } - public function testItCreateMelodiiaObject() { - $this->form->isSubmitted()->willReturn(true); $this->form->isValid()->willReturn(true); $this->form->getData()->willReturn(new FakeMelodiiaModel()); @@ -133,7 +107,6 @@ public function testItCreateMelodiiaObject() public function testICanChangeTheClearSubmitParam() { - $this->form->isSubmitted()->willReturn(true); $this->form->isValid()->willReturn(true); $this->form->getData()->willReturn(new FakeMelodiiaModel()); @@ -141,7 +114,7 @@ public function testICanChangeTheClearSubmitParam() $this->mockDispatch($this->dispatcher, Argument::type(CustomResponseEvent::class), Create::EVENT_POST_CREATE)->shouldBeCalled(); $this->dataStore->save(Argument::type(FakeMelodiiaModel::class))->shouldBeCalled(); $this->attributes->getBoolean(CrudControllerInterface::FORM_CLEAR_MISSING, true)->willReturn(false); - $this->form->submit(['awesome' => 'json'], false)->willReturn()->shouldBeCalled(); + $this->form->handleRequest($this->request)->willReturn()->shouldBeCalled(); /** @var ApiResponse $res */ $res = ($this->controller)($this->request->reveal()); diff --git a/tests/Melodiia/Crud/Controller/UpdateTest.php b/tests/Melodiia/Crud/Controller/UpdateTest.php index 57f024c..d92fa1d 100644 --- a/tests/Melodiia/Crud/Controller/UpdateTest.php +++ b/tests/Melodiia/Crud/Controller/UpdateTest.php @@ -68,9 +68,7 @@ public function setUp() $this->attributes->has(CrudControllerInterface::FORM_CLEAR_MISSING)->willReturn(false); $this->attributes->getBoolean(CrudControllerInterface::FORM_CLEAR_MISSING, false)->willReturn(false); $this->request->attributes = $this->attributes->reveal(); - $this->request->getMethod()->willReturn('POST'); - $this->request->getContent()->willReturn('{"awesome":"json"}'); - $this->form->submit(['awesome' => 'json'], false)->willReturn(); + $this->form->handleRequest($this->request)->willReturn(); $this->formFactory->createNamed('', Argument::cetera())->willReturn($this->form); $this->dataStore->find(FakeMelodiiaModel::class, 'id')->willReturn(new \stdClass()); @@ -87,74 +85,6 @@ public function setUp() ); } - public function testItReturn400OnNotSubmittedForm() - { - $this->form->isSubmitted()->willReturn(false); - - /** @var ApiResponse $res */ - $res = ($this->controller)($this->request->reveal()); - - $this->assertInstanceOf(ApiResponse::class, $res); - $this->assertEquals(400, $res->httpStatus()); - } - - /** - * Issue #54. - */ - public function testItClearMissingWhileNullGivenAndMethodPatch() - { - // Mocking - $this->form->isSubmitted()->willReturn(true); - $this->form->isValid()->willReturn(true); - $this->form->getData()->willReturn(new FakeMelodiiaModel()); - $this->dataStore->save(Argument::type(FakeMelodiiaModel::class))->shouldBeCalled(); - $this->mockDispatch($this->dispatcher, Argument::type(CrudEvent::class), Argument::type('string')); - - // Most important check - $this->form->submit(['awesome' => 'json'], true)->willReturn()->shouldBeCalled(); - - $this->request->getMethod()->willReturn('PATCH'); - - /** @var ApiResponse $res */ - $res = ($this->controller)($this->request->reveal()); - - $this->assertInstanceOf(ApiResponse::class, $res); - $this->assertEquals(200, $res->httpStatus()); - } - - public function testItClearMissingWhenEnforcedToTrue() - { - // Mocking - $this->form->isSubmitted()->willReturn(true); - $this->form->isValid()->willReturn(true); - $this->form->getData()->willReturn(new FakeMelodiiaModel()); - $this->attributes->has(CrudControllerInterface::FORM_CLEAR_MISSING)->willReturn(true); - $this->attributes->getBoolean(CrudControllerInterface::FORM_CLEAR_MISSING)->willReturn(true); - $this->dataStore->save(Argument::type(FakeMelodiiaModel::class))->shouldBeCalled(); - $this->mockDispatch($this->dispatcher, Argument::type(CrudEvent::class), Argument::type('string')); - - // Most important check - $this->form->submit(['awesome' => 'json'], true)->willReturn()->shouldBeCalled(); - - /** @var ApiResponse $res */ - $res = ($this->controller)($this->request->reveal()); - $this->assertInstanceOf(ApiResponse::class, $res); - $this->assertEquals(200, $res->httpStatus()); - } - - /** - * Issue #28. - */ - public function testItReturnProperlyOnWrongInput() - { - $this->request->getContent()->willReturn('{"awesome":json"}'); // Wrong JSON - - /** @var ApiResponse $res */ - $res = ($this->controller)($this->request->reveal()); - $this->assertInstanceOf(ApiResponse::class, $res); - $this->assertEquals(400, $res->httpStatus()); - } - public function testItReturn400OnInvalidForm() { $this->form->isSubmitted()->willReturn(true); @@ -183,25 +113,6 @@ public function testItUpdateMelodiiaObject() $this->assertInstanceOf(OkContent::class, $res); } - public function testICanChangeClearMissingOption() - { - $this->form->isSubmitted()->willReturn(true); - $this->form->isValid()->willReturn(true); - - $this->form->getData()->willReturn(new FakeMelodiiaModel()); - $this->attributes->has(CrudControllerInterface::FORM_CLEAR_MISSING)->willReturn(true); - $this->attributes->getBoolean(CrudControllerInterface::FORM_CLEAR_MISSING)->willReturn(true); - $this->form->submit(['awesome' => 'json'], true)->willReturn()->shouldBeCalled(); - $this->mockDispatch($this->dispatcher, Argument::type(CrudEvent::class), Update::EVENT_PRE_UPDATE)->shouldBeCalled(); - $this->mockDispatch($this->dispatcher, Argument::type(CustomResponseEvent::class), Update::EVENT_POST_UPDATE)->shouldBeCalled(); - $this->dataStore->save(Argument::type(FakeMelodiiaModel::class))->shouldBeCalled(); - - /** @var ApiResponse $res */ - $res = ($this->controller)($this->request->reveal()); - - $this->assertInstanceOf(OkContent::class, $res); - } - /** * @expectedException \Symfony\Component\Security\Core\Exception\AccessDeniedException */ diff --git a/tests/Melodiia/Form/ApiRequestHandlerTest.php b/tests/Melodiia/Form/ApiRequestHandlerTest.php new file mode 100644 index 0000000..63e3501 --- /dev/null +++ b/tests/Melodiia/Form/ApiRequestHandlerTest.php @@ -0,0 +1,82 @@ +subject = new ApiRequestHandler(); + + $this->request = $this->prophesize(Request::class); + $this->form = $this->prophesize(FormInterface::class); + $this->formConfig = $this->prophesize(FormConfigInterface::class); + + $this->form->getConfig()->willReturn($this->formConfig->reveal()); + $this->request->getMethod()->willReturn('POST'); + } + + public function tearDown() + { + $this->subject = null; + $this->form = null; + $this->formConfig = null; + $this->request = null; + } + + public function testItSubmitJsonData() + { + $this->form->submit(['hello' => 'foo'], false)->shouldBeCalled(); + $this->request->getContent()->willReturn('{"hello":"foo"}'); + + $this->subject->handleRequest($this->form->reveal(), $this->request->reveal()); + } + + public function testItAddsAFormErrorInCaseOfWrongInput() + { + $this->form->submit(null, false)->shouldBeCalled(); + $this->form->addError(Argument::type(FormError::class))->shouldBeCalled(); + $this->request->getContent()->willReturn('{"hello":"foo}'); + + $this->subject->handleRequest($this->form->reveal(), $this->request->reveal()); + } + + public function testItClearsMissingOnPatchRequest() + { + $this->form->submit(['hello' => 'foo'], true)->shouldBeCalled(); + $this->request->getContent()->willReturn('{"hello":"foo"}'); + $this->request->getMethod()->willReturn('PATCH'); + + $this->subject->handleRequest($this->form->reveal(), $this->request->reveal()); + } + + public function testICanChangeClearMissingOption() + { + $this->form->submit(['hello' => 'foo'], true)->shouldBeCalled(); + $this->formConfig->getOption(ApiType::CLEAR_MISSING_OPTION)->willReturn(true); + $this->request->getContent()->willReturn('{"hello":"foo"}'); + + $this->subject->handleRequest($this->form->reveal(), $this->request->reveal()); + } +}