[Serializer] added the ability to gather all deserialization errors. #27136

omissis · 2018-05-03T11:00:19Z

I'd like to propose a new "collect_all_errors" (boolean) setting in the deserialization context, that will have the normalizer loop through all the attributes and collect all the errors, as opposed to breaking at the first one.

If this proposal is acceptable, I'll gladly add the Doc PR and Changelog :)

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	none
License	MIT
Doc PR	tbd

This patch proposes an update to the Symfony\Component\Serializer\Normalizer\AbstractObjectNormalizer that allows clients to receive all deserialization errors instead of the first one only.

Additionally:

Introduces NotDenormalizableValueException for better separation of concerns between normalization and denormalization.

dunglas

Great future, it will definitely be useful for web APIs. Can you take a look to https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Serializer/Normalizer/ConstraintViolationListNormalizer.php, it would be nice to be able to serialize the resulting exception (the one that collects all violations) in the same RFC7807 format.

I strongly encourage to use a similar structure than the Validator's ConstraintViolationList, or even use it directly (we may move this class in an internal shared component for instance). It will allow to factorize some code, and all existing normalizers for this class (ConstraintViolationListNormalizer mentioned above, but also the Hydra one shipped with API Platform) will then be able to serialize the thrown exception.

Also, I'm for enabling this feature by default.

dunglas · 2018-05-29T21:36:41Z

src/Symfony/Component/Serializer/Exception/NotDenormalizableValueException.php

+
+namespace Symfony\Component\Serializer\Exception;
+
+use Throwable;


Usually we don't add use statements for classes in the global namespace, I don't know why fabbot doesn't report a problem.

dunglas · 2018-05-29T21:38:29Z

src/Symfony/Component/Serializer/Exception/UnexpectedValuesException.php

+namespace Symfony\Component\Serializer\Exception;
+
+/**
+ * UnexpectedValuesException.


This line can be removed (it brings nothing)

ok, I added it as I see other exceptions in the namespace have it.

dunglas · 2018-05-29T21:39:14Z

src/Symfony/Component/Serializer/Exception/UnexpectedValuesException.php

+class UnexpectedValuesException extends \RuntimeException implements ExceptionInterface
+{
+    /**
+     * @var string[]


This type information should be moved on the constructor instead.

Right. I usually specify that in both places (property's docblock and constructor's argument): would that be ok, too?

dunglas · 2018-05-29T21:40:39Z

src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php

-     * {@inheritdoc}
-     */
-    public function denormalize($data, $class, $format = null, array $context = array())
+    public function denormalize($data, $class, $format = null, array $context = array(), array &$accumulatingContext = array())


Adding this parameter is a BC break and it doesn't respect the NormalizerInterface.
But are you sure you can't reuse the standard context, using a similar trick than in: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php#L375, or store an \ArrayObject.

You mean passing $context by reference? I would have to try that, but if memory doesn't fail me, I did try and it didn't work: I'll try again and see :)
The thing is, wouldn't that qualify as BC break too? I'd argue that such approach could lead to harder to find bugs as the method signatures would differ only by a &, where the interface would declare a behaviour and the implementation would allow changes to $context to ripple out of the method's scope, so I'd probably like an explicit BC break here instead. Perhaps introducing a solution akin to ContextAwareDenormalizerInterface could do(it does specify an extra argument for supportsDenormalization() compared to DenormalizerInterface)?

dunglas · 2018-05-29T21:45:35Z

src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php

+                $value = $this->validateAndDenormalize($class, $attribute, $value, $format, $context, $accumulatingContext);
+            } catch (NotDenormalizableValueException $exception) {
+                if (isset($context[self::COLLECT_ALL_ERRORS]) && $context[self::COLLECT_ALL_ERRORS]) {
+                    $accumulatingContext[$attribute][] = $exception->getMessage();


I suggest store references to the exceptions instead of just the message. It costs nothing and improve the evolvability.

Same everywhere.

dunglas · 2018-05-29T21:47:03Z

src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php

+                continue;
+            }
+
+            if (null === $value) {


null can be an expected value. The setter must be called even if the value is null (to update an existing object for instance).

dunglas · 2018-05-29T21:47:44Z

src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php

+            try {
+                $value = $this->validateAndDenormalize($class, $attribute, $value, $format, $context, $accumulatingContext);
+            } catch (NotDenormalizableValueException $exception) {
+                if (isset($context[self::COLLECT_ALL_ERRORS]) && $context[self::COLLECT_ALL_ERRORS]) {


Can be simplified in $context[self::COLLECT_ALL_ERRORS] ?? false. Same everywhere.

Ok, I wasn't sure I could use php7 features

dunglas · 2018-05-29T22:01:38Z

src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php

+                    continue;
+                }
+
+                throw $exception;
            }
        }

        if (!empty($extraAttributes)) {
            throw new ExtraAttributesException($extraAttributes);


Shouldn't this exception be collected too? It will be useful when creating web APIs to be able to return all errors in one time to the client, including this one.

dunglas · 2018-05-29T22:03:42Z

src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php

+                        return $this->serializer->denormalize($data, $class, $format, $childContext, $accumulatingContext);
+                    } catch (NotDenormalizableValueException | InvalidArgumentException $e) {
+                        if (isset($context[self::COLLECT_ALL_ERRORS]) && $context[self::COLLECT_ALL_ERRORS]) {
+                            $accumulatingContext[$attribute][] = $e->getMessage();


You should probably merge caught exception with the existing one. It's why collecting messages not enough, it would be better to collect exceptions themselves.

The resulting document (to send to the client) should be something like:

attribute1: error message attribute2.innerAttribute1: foo attribute2.innerAttribute2: bar

At the moment errors live in a nested array like the following one(using your example):

$errors = [ 'attribute1' => [ 0 => 'error message', ], 'attribute2' => [ 'innerAttribute1' => [ 0 => 'foo', ], 'innerAttribute2' => [ 0 => 'bar', ] ], ];

Which should be fairly easy to serialize in the form you suggest.. do you think it'd be better to make it flat from the beginning? i.e.:

$errors = [ 'attribute1' => [ 0 => 'error message', ], 'attribute2.innerAttribute1' => [ 0 => 'foo', ], 'attribute2.innerAttribute2' => [ 0 => 'bar', ], ];

dunglas · 2018-09-04T19:22:46Z

@omissis any news regarding this PR? Do you need some help?

omissis · 2018-09-04T19:45:58Z

Hi! I have been working on it a bit during summer vacations, and I was about to ping you on Slack, but I want to investigate a bit the build failures locally first, and bother you as little as possible. I will ping you soon, thx for asking :)

Nek- · 2018-09-05T16:56:50Z

@omissis I'm really interested in this PR too. Feel free to ask for help if needed. I'm available on slack.

(I missed your PR and created this ticket #28358)

By passing setting "collect_all_errors" to true in the deserialization context, the normalizer will now loop through all the attributes and collect all the errors, as opposed to breaking at the first one.

* move from collecting error messages strings to error objects * add ExtraAttributeException to capture errors about single, unallowed attributes * make extra attributes errors part of the set of collected errors when 'collect_all_errors' is enabled * fix some coding style issues

omissis · 2018-09-09T09:15:47Z

Hey @Nek-, thanks for mentioning the card. Do you think this branch does what you need or it needs further improvemnts?

* remove unused param in UnexpectedValuesException::assertIsError * add return typehint to UnexpectedValuesExceptionTest::wrongErrorsProvider * change reference to RuntimeException to \RuntimeException

Nek- · 2018-09-10T12:24:11Z

src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php

    {
+        if (null === $accumulatingContext) {


I guess this have sense only if $shouldCollectAllErrors is true?

Nek- · 2018-09-10T12:41:59Z

src/Symfony/Component/Serializer/Normalizer/DenormalizerInterface.php

+     * @throws ExtraAttributesException        Occurs when the item doesn't have attribute to receive given data
+     * @throws NotDenormalizableValueException Occurs when the item cannot be denormalized
+     * @throws LogicException                  Occurs when the normalizer is not supposed to denormalize
+     * @throws RuntimeException                Occurs if the class cannot be instantiated


You should add here the new parameter. And also add it as parameter of the method (but commented) so it can be add in Symfony5.

Nek- · 2018-09-10T13:14:52Z

src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php

            }
        }

        if (!empty($extraAttributes)) {
-            throw new ExtraAttributesException($extraAttributes);
+            if (!$shouldCollectAllErrors) {
+                throw new ExtraAttributesException($extraAttributes);


Throwing an exception looks not really good to me because it forces everybody to catch the exception and re-throw on any normalizer that decorate the AbstractObjectNormalizer.

Here is an example of normalizer we could imagine:

class PhoneNumberNormalizer implements DenormalizerInterface { public function __construct(DenormalizerInterface $objectNormalizerDecorated) { $this->decorated = $objectNormalizerDecorated; } public function denormalize($data, $class, $format = null, array $context = array(), AccumulatingContext $accumulatingContext = null) { $object = $this->decorated->denormalize($data); // Here the exception may be throw while we accumulate errors // We should catch and add a new throw (and for each normalizer) // I do some stuff with $object // Well, maybe we should re-throw here in case something's wrong? // Ok, I will re-throw (exceptions should be nested, which is another problem) // How am I suppose at this point know what attribute I want to add? // nevermind, the library is not up to date to support this feature and will generate weird behavior } }

The exception is (IMHO) not designed for this kind of usage. I suggest the usage of a new concept: the context of serialization needs to be an object. And it should also contain an error object (something like your accumulatingContext). This way the user will be able to configure properly (and without only using constants) the serialization/deserialization and also will be able to get back information about what happened in the process.

This may be a lot of work but probably the real best solution. (what do you think @dunglas ?)

Note that another solution I see would be to give the AccumulationContext as user instead of just a boolean. I posted this suggestion as example here: #28358

omissis · 2019-04-09T08:01:28Z

@dunglas I'd like to close this PR so that it doesn't pollute the project's board, would it be ok?

omissis · 2019-05-09T15:37:44Z

closing to avoid polluting the board with a stale pr.

carsonbot added Status: Needs Review Serializer Feature Deprecation labels May 3, 2018

omissis force-pushed the SERIALIZER-object-normalizer-denormalize-validation-errors-list branch from 5959479 to 573de83 Compare May 3, 2018 11:05

nicolas-grekas added this to the next milestone May 3, 2018

dunglas requested changes May 29, 2018

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels May 29, 2018

omissis force-pushed the SERIALIZER-object-normalizer-denormalize-validation-errors-list branch from 573de83 to 779baf8 Compare June 7, 2018 06:31

dunglas mentioned this pull request Sep 4, 2018

[Serializer][RFC] New context option to avoid throw UnexpectedValueException #28358

Closed

omissis added 3 commits September 8, 2018 15:45

[Serializer] added the ability to gather all deserialization errors.

e2107b2

By passing setting "collect_all_errors" to true in the deserialization context, the normalizer will now loop through all the attributes and collect all the errors, as opposed to breaking at the first one.

Make accumulating context an object to produce a flattened error array.

3df123e

omissis force-pushed the SERIALIZER-object-normalizer-denormalize-validation-errors-list branch from 779baf8 to 3df123e Compare September 8, 2018 18:50

omissis added 2 commits September 8, 2018 20:53

fix coding style issues as per signaled by fabbot.

ac2aef6

complete an half-written UnexpectedValuesException test.

132b8c5

omissis force-pushed the SERIALIZER-object-normalizer-denormalize-validation-errors-list branch from 0ff0ff8 to 132b8c5 Compare September 8, 2018 19:36

Fix coding style issues.

6ff28d4

* remove unused param in UnexpectedValuesException::assertIsError * add return typehint to UnexpectedValuesExceptionTest::wrongErrorsProvider * change reference to RuntimeException to \RuntimeException

Nek- reviewed Sep 10, 2018

View reviewed changes

dunglas mentioned this pull request Jan 29, 2019

Request must be validated, but not Object after deserialization api-platform/api-platform#788

Open

omissis closed this May 9, 2019

omissis deleted the SERIALIZER-object-normalizer-denormalize-validation-errors-list branch May 9, 2019 15:38

dunglas mentioned this pull request May 12, 2019

[Serializer] Deprecate type safety in favor of autovalidation #31487

Closed

dunglas mentioned this pull request Jul 23, 2019

[Serializer] Adding ignore_invalid_datetimes flag to Serialization Context to avoid exceptions #32642

Closed

nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019

weaverryan mentioned this pull request Aug 27, 2020

[RFC][Serializer] Ability to collect denormalization failures #37419

Closed

julienfalque mentioned this pull request Sep 12, 2020

[Serializer] Add ability to collect denormalization errors #38165

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Serializer] added the ability to gather all deserialization errors. #27136

[Serializer] added the ability to gather all deserialization errors. #27136

omissis commented May 3, 2018

dunglas left a comment •

edited

Loading

dunglas May 29, 2018

dunglas May 29, 2018

omissis Jun 7, 2018

dunglas May 29, 2018

omissis Jun 7, 2018

dunglas May 29, 2018

omissis Jun 7, 2018

dunglas May 29, 2018

dunglas May 29, 2018

dunglas May 29, 2018

omissis Jun 7, 2018

dunglas May 29, 2018

dunglas May 29, 2018

omissis Jun 7, 2018

dunglas commented Sep 4, 2018

omissis commented Sep 4, 2018

Nek- commented Sep 5, 2018 •

edited

Loading

omissis commented Sep 9, 2018

Nek- Sep 10, 2018

omissis Sep 10, 2018

Nek- Sep 10, 2018

Nek- Sep 10, 2018

omissis commented Apr 9, 2019

omissis commented May 9, 2019


		namespace Symfony\Component\Serializer\Exception;

		use Throwable;

[Serializer] added the ability to gather all deserialization errors. #27136

[Serializer] added the ability to gather all deserialization errors. #27136

Conversation

omissis commented May 3, 2018

dunglas left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunglas commented Sep 4, 2018

omissis commented Sep 4, 2018

Nek- commented Sep 5, 2018 • edited Loading

omissis commented Sep 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omissis commented Apr 9, 2019

omissis commented May 9, 2019

dunglas left a comment •

edited

Loading

Nek- commented Sep 5, 2018 •

edited

Loading