Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 6 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e2107b2
[Serializer] added the ability to gather all deserialization errors.
omissis Jan 12, 2018
1a075b3
[Serializer] Fix some bugs and implement CR feedback.
omissis Sep 8, 2018
3df123e
Make accumulating context an object to produce a flattened error array.
omissis Sep 8, 2018
ac2aef6
fix coding style issues as per signaled by fabbot.
omissis Sep 8, 2018
132b8c5
complete an half-written UnexpectedValuesException test.
omissis Sep 8, 2018
6ff28d4
Fix coding style issues.
omissis Sep 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions src/Symfony/Component/Serializer/Context/AccumulatingContext.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

declare(strict_types=1);

namespace Symfony\Component\Serializer\Context;

class AccumulatingContext extends \ArrayObject
{
public function isEmpty(): bool
{
return 0 === $this->count();
}

public function flatten(): array
{
$flatContext = array();

foreach ($this as $key => $value) {
if (\is_array($value)) {
$this->doFlatten($value, $key, $flatContext);

continue;
}

$flatContext[$key] = $value;
}

return $flatContext;
}

private function doFlatten(array $value, $key, array &$flatContext)
{
foreach ($value as $innerKey => $innerValue) {
if (\is_string($key) && \is_array($innerValue)) {
$this->doFlatten($innerValue, $key.'.'.$innerKey, $flatContext);

continue;
}

$flatContext[$key][] = $innerValue;
}
}
}
40 changes: 40 additions & 0 deletions src/Symfony/Component/Serializer/Exception/ExtraAttributeException.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\Serializer\Exception;

/**
* @author Claudio Beatrice <claudi0.beatric3@gmail.com>
*/
class ExtraAttributeException extends \RuntimeException
{
/**
* @var string
*/
private $extraAttribute;

public function __construct(string $extraAttribute, \Exception $previous = null)
{
$msg = sprintf('Extra attribute "%s" is not allowed.', $extraAttribute);

$this->extraAttribute = $extraAttribute;

parent::__construct($msg, 0, $previous);
}

/**
* Get the extra attribute that is not allowed.
*/
public function getExtraAttribute(): string
{
return $this->extraAttribute;
}
}
37 changes: 37 additions & 0 deletions src/Symfony/Component/Serializer/Exception/NotDenormalizableValueException.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\Serializer\Exception;

/**
* @author Claudio Beatrice <claudi0.beatric3@gmail.com>
*/
class NotDenormalizableValueException extends NotNormalizableValueException
{
/**
* @var null|string
*/
private $field;

public function __construct(string $message = '', ?string $field = null, \Throwable $previous = null)
{
@trigger_error(sprintf('The %s class will stop extending %s in Symfony 5.0, where it will extend %s instead.', self::class, NotNormalizableValueException::class, UnexpectedValueException::class), E_USER_NOTICE);

parent::__construct($message, $previous ? $previous->getCode() : 0, $previous);

$this->field = $field;
}

public function getField(): ?string
{
return $this->field;
}
}
107 changes: 107 additions & 0 deletions src/Symfony/Component/Serializer/Exception/UnexpectedValuesException.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\Serializer\Exception;

/**
* @author Claudio Beatrice <claudi0.beatric3@gmail.com>
*/
class UnexpectedValuesException extends \RuntimeException implements ExceptionInterface
{
/**
* @var UnexpectedValueException[]
*/
private $unexpectedValueErrors;

/**
* @param array<string, UnexpectedValueException[]> $unexpectedValueErrors
*/
public function __construct(array $unexpectedValueErrors)
{
$this->validateErrors($unexpectedValueErrors);

parent::__construct();

$this->unexpectedValueErrors = $unexpectedValueErrors;
}

/**
* @return array<string, UnexpectedValueException[]>
*/
public function getUnexpectedValueErrors(): array
{
return $this->unexpectedValueErrors;
}

/**
* @param array<string, UnexpectedValueException[]> $unexpectedValueErrors
*
* @throws InvalidArgumentException
*/
private function validateErrors(array $unexpectedValueErrors): void
{
$this->assertNotEmpty($unexpectedValueErrors, 'No errors were given, at least one is expected.');

foreach ($unexpectedValueErrors as $field => $fieldUnexpectedValueErrors) {
$this->assertIsString($field, sprintf('All keys must be strings, %s given.', $this->getType($field)));
$this->assertNotEmpty($fieldUnexpectedValueErrors, sprintf('No errors were given for key "%s", at least one is expected.', $field));

foreach ((array) $fieldUnexpectedValueErrors as $fieldUnexpectedValueError) {
$this->assertIsError($fieldUnexpectedValueError, sprintf(
'All errors must be instances of %s, %s given for key "%s".',
UnexpectedValueException::class,
$this->getType($fieldUnexpectedValueError),
$field
), $unexpectedValueErrors);
}
}
}

/**
* @param mixed $unexpectedValueError
*/
private function getType($unexpectedValueError): string
{
return \is_object($unexpectedValueError)
? \get_class($unexpectedValueError)
: \gettype($unexpectedValueError);
}

/**
* @param array $values
*/
private function assertNotEmpty(array $values, string $message): void
{
if (empty($values)) {
throw new InvalidArgumentException($message);
}
}

/**
* @param mixed $value
*/
private function assertIsString($value, string $message): void
{
if ('string' !== $this->getType($value)) {
throw new InvalidArgumentException($message);
}
}

/**
* @param mixed $value
*/
private function assertIsError($value, string $message): void
{
if (!$value instanceof UnexpectedValueException) {
throw new InvalidArgumentException($message);
}
}
}
73 changes: 63 additions & 10 deletions src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,15 @@
use Symfony\Component\PropertyAccess\Exception\InvalidArgumentException;
use Symfony\Component\PropertyInfo\PropertyTypeExtractorInterface;
use Symfony\Component\PropertyInfo\Type;
use Symfony\Component\Serializer\Context\AccumulatingContext;
use Symfony\Component\Serializer\Encoder\JsonEncoder;
use Symfony\Component\Serializer\Exception\ExtraAttributeException;
use Symfony\Component\Serializer\Exception\ExtraAttributesException;
use Symfony\Component\Serializer\Exception\LogicException;
use Symfony\Component\Serializer\Exception\NotDenormalizableValueException;
use Symfony\Component\Serializer\Exception\NotNormalizableValueException;
use Symfony\Component\Serializer\Exception\RuntimeException;
use Symfony\Component\Serializer\Exception\UnexpectedValuesException;
use Symfony\Component\Serializer\Mapping\AttributeMetadataInterface;
use Symfony\Component\Serializer\Mapping\ClassDiscriminatorFromClassMetadata;
use Symfony\Component\Serializer\Mapping\ClassDiscriminatorResolverInterface;
Expand All @@ -35,6 +39,7 @@ abstract class AbstractObjectNormalizer extends AbstractNormalizer
const ENABLE_MAX_DEPTH = 'enable_max_depth';
const DEPTH_KEY_PATTERN = 'depth_%s::%s';
const DISABLE_TYPE_ENFORCEMENT = 'disable_type_enforcement';
const COLLECT_ALL_ERRORS = 'collect_all_errors';

private $propertyTypeExtractor;
private $typesCache = array();
Expand Down Expand Up @@ -228,11 +233,12 @@ public function supportsDenormalization($data, $type, $format = null)
return \class_exists($type) || (\interface_exists($type, false) && $this->classDiscriminatorResolver && null !== $this->classDiscriminatorResolver->getMappingForClass($type));
}

/**
* {@inheritdoc}
*/
public function denormalize($data, $class, $format = null, array $context = array())
public function denormalize($data, $class, $format = null, array $context = array(), AccumulatingContext $accumulatingContext = null)
{
if (null === $accumulatingContext) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

$accumulatingContext = new AccumulatingContext();
}

if (!isset($context['cache_key'])) {
$context['cache_key'] = $this->getCacheKey($format, $context);
}
Expand All @@ -244,6 +250,8 @@ public function denormalize($data, $class, $format = null, array $context = arra
$reflectionClass = new \ReflectionClass($class);
$object = $this->instantiateObject($normalizedData, $class, $context, $reflectionClass, $allowedAttributes, $format);

$shouldCollectAllErrors = $context[self::COLLECT_ALL_ERRORS] ?? false;

foreach ($normalizedData as $attribute => $value) {
if ($this->nameConverter) {
$attribute = $this->nameConverter->denormalize($attribute, $class, $format, $context);
Expand All @@ -257,16 +265,49 @@ public function denormalize($data, $class, $format = null, array $context = arra
continue;
}

$value = $this->validateAndDenormalize($class, $attribute, $value, $format, $context);
try {
$value = $this->validateAndDenormalize($class, $attribute, $value, $format, $context, $accumulatingContext);
} catch (NotDenormalizableValueException $exception) {
if ($shouldCollectAllErrors) {
$accumulatingContext[$attribute][] = $exception;

continue;
}

throw $exception;
} catch (UnexpectedValuesException $exception) {
$accumulatingContext[$attribute] = $exception->getUnexpectedValueErrors();

continue;
}

try {
$this->setAttributeValue($object, $attribute, $value, $format, $context);
} catch (InvalidArgumentException $e) {
throw new NotNormalizableValueException($e->getMessage(), $e->getCode(), $e);
$exception = new NotDenormalizableValueException($e->getMessage(), $attribute, $e);

if ($shouldCollectAllErrors) {
$accumulatingContext[$attribute][] = $exception;

continue;
}

throw $exception;
}
}

if (!empty($extraAttributes)) {
throw new ExtraAttributesException($extraAttributes);
if (!$shouldCollectAllErrors) {
throw new ExtraAttributesException($extraAttributes);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

foreach ($extraAttributes as $extraAttribute) {
$accumulatingContext[$extraAttribute][] = new ExtraAttributeException($extraAttribute);
}
}

if (!$accumulatingContext->isEmpty()) {
throw new UnexpectedValuesException($accumulatingContext->flatten());
}

return $object;
Expand All @@ -293,12 +334,14 @@ abstract protected function setAttributeValue($object, $attribute, $value, $form
* @throws NotNormalizableValueException
* @throws LogicException
*/
private function validateAndDenormalize(string $currentClass, string $attribute, $data, ?string $format, array $context)
private function validateAndDenormalize(string $currentClass, string $attribute, $data, ?string $format, array $context, AccumulatingContext $accumulatingContext)
{
if (null === $types = $this->getTypes($currentClass, $attribute)) {
return $data;
}

$shouldCollectAllErrors = $context[self::COLLECT_ALL_ERRORS] ?? false;

$expectedTypes = array();
foreach ($types as $type) {
if (null === $data && $type->isNullable()) {
Expand Down Expand Up @@ -332,7 +375,17 @@ private function validateAndDenormalize(string $currentClass, string $attribute,

$childContext = $this->createChildContext($context, $attribute);
if ($this->serializer->supportsDenormalization($data, $class, $format, $childContext)) {
return $this->serializer->denormalize($data, $class, $format, $childContext);
try {
return $this->serializer->denormalize($data, $class, $format, $childContext, $accumulatingContext);
} catch (NotDenormalizableValueException | InvalidArgumentException $e) {
if ($shouldCollectAllErrors) {
$accumulatingContext[$attribute][] = $e;

continue;
}

throw $e;
}
}
}

Expand All @@ -355,7 +408,7 @@ private function validateAndDenormalize(string $currentClass, string $attribute,
return $data;
}

throw new NotNormalizableValueException(sprintf('The type of the "%s" attribute for class "%s" must be one of "%s" ("%s" given).', $attribute, $currentClass, implode('", "', array_keys($expectedTypes)), \gettype($data)));
throw new NotDenormalizableValueException(sprintf('The type of the "%s" attribute for class "%s" must be one of "%s" ("%s" given).', $attribute, $currentClass, implode('", "', array_keys($expectedTypes)), \gettype($data)), $attribute);
}

/**
Expand Down
Loading