-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Serializer] Add ability to collect denormalization errors #38968
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.
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
Conversation
src/Symfony/Component/Serializer/Exception/AggregableExceptionInterface.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Serializer/Exception/AggregatedException.php
Outdated
Show resolved
Hide resolved
@@ -633,6 +638,56 @@ public function testDeserializeAndUnwrap() | |||
$serializer->deserialize($jsonData, __NAMESPACE__.'\Model', 'json', [UnwrappingDenormalizer::UNWRAP_PATH => '[baz][inner]']) | |||
); | |||
} | |||
|
|||
public function testCollectDenormalizationErrors() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use a data provider to able to add new cases like for variadic argument, UnwrappingDenormalizer
, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The data provider is suitable for cases where the assertion code is always the same for different data. Perhaps it would be better to write a test for each case here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either way, I just think this one is getting a bit hard to read.
Maybe add a new test for the UnwrappingDenormlizer
because I think you don't handle it already and the key should be added to error to match the input paylod.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you provide a test case for UnwrappingDenormlizer
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is already a few tests that uses it.
The idea is to wrap the data into container: {"container:" {"foo": "string", ...}}
And check that the property path for each errors starts with container.
98b5fd3
to
8264a3b
Compare
8264a3b
to
79024bf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great!
src/Symfony/Component/Serializer/Exception/AggregatedException.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Serializer/Exception/AggregatedException.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Serializer/Exception/VariadicConstructorArgumentsException.php
Outdated
Show resolved
Hide resolved
try { | ||
$this->setAttributeValue($object, $attribute, $value, $format, $context); | ||
} catch (InvalidArgumentException $e) { | ||
throw new NotNormalizableValueException(sprintf('Failed to denormalize attribute "%s" value for class "%s": '.$e->getMessage(), $attribute, $type), $e->getCode(), $e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we try to avoid the extra try / catch. I'm nitpicking but catching and throwing is very costly in PHP so let's avoid it when not strictly necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The inner try / catch was here for some reason. It was wrapped around to handle all calls that throw exceptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think he suggested to remove the inner try/catch, add another catch clause to the outer one when you deal with this exception by throwing or aggregating it depending on the context
src/Symfony/Component/Serializer/Exception/AggregatedException.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Serializer/Normalizer/DenormalizerInterface.php
Outdated
Show resolved
Hide resolved
bca07c6
to
f6dee97
Compare
Added exception handling from I have some thoughts about adding |
I don't think it should be added to all of them, |
src/Symfony/Component/Serializer/Exception/AggregatedException.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Serializer/Exception/AggregatedException.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Serializer/Exception/AggregatedException.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Serializer/Exception/AggregatedException.php
Outdated
Show resolved
Hide resolved
@@ -16,6 +16,6 @@ | |||
* | |||
* @author Johannes M. Schmitt <schmittjoh@gmail.com> | |||
*/ | |||
class InvalidArgumentException extends \InvalidArgumentException implements ExceptionInterface | |||
class InvalidArgumentException extends \InvalidArgumentException implements ExceptionInterface, AggregableExceptionInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After a quick look I think there is two spots that should not be handled:
throw new InvalidArgumentException(sprintf('Cannot denormalize to a "%s" without the HttpFoundation component installed. Try running "composer require symfony/http-foundation".', File::class)); |
throw new InvalidArgumentException('Unsupported class: '.$type); |
Those cases are because of an invalid type or missing class, I think they should stop the denormalization process as usual.
try { | ||
$this->setAttributeValue($object, $attribute, $value, $format, $context); | ||
} catch (InvalidArgumentException $e) { | ||
throw new NotNormalizableValueException(sprintf('Failed to denormalize attribute "%s" value for class "%s": '.$e->getMessage(), $attribute, $type), $e->getCode(), $e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think he suggested to remove the inner try/catch, add another catch clause to the outer one when you deal with this exception by throwing or aggregating it depending on the context
src/Symfony/Component/Serializer/Exception/AggregatedException.php
Outdated
Show resolved
Hide resolved
@@ -633,6 +638,56 @@ public function testDeserializeAndUnwrap() | |||
$serializer->deserialize($jsonData, __NAMESPACE__.'\Model', 'json', [UnwrappingDenormalizer::UNWRAP_PATH => '[baz][inner]']) | |||
); | |||
} | |||
|
|||
public function testCollectDenormalizationErrors() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either way, I just think this one is getting a bit hard to read.
Maybe add a new test for the UnwrappingDenormlizer
because I think you don't handle it already and the key should be added to error to match the input paylod.
see see #42502 |
…ror during denormalization (lyrixx) This PR was merged into the 5.4 branch. Discussion ---------- [Serializer] Add support for collecting type error during denormalization | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | Fix symfony#27824, Fix symfony#42236, Fix symfony#38472, Fix symfony#37419 Fix symfony#38968 | License | MIT | Doc PR | --- There is something that I don't like about the (de)Serializer. It's about the way it deals with typed properties. As soon as you add a type to a property, the API can return 500. Let's consider the following code: ```php class MyDto { public string $string; public int $int; public float $float; public bool $bool; public \DateTime $dateTime; public \DateTimeImmutable $dateTimeImmutable; public \DateTimeZone $dateTimeZone; public \SplFileInfo $splFileInfo; public Uuid $uuid; public array $array; /** `@var` MyDto[] */ public array $collection; } ``` and the following JSON: ```json { "string": null, "int": null, "float": null, "bool": null, "dateTime": null, "dateTimeImmutable": null, "dateTimeZone": null, "splFileInfo": null, "uuid": null, "array": null, "collection": [ { "string": "string" }, { "string": null } ] } ``` **By default**, I got a 500:  It's the same with the prod environment. This is far from perfect when you try to make a public API :/ ATM, the only solution, is to remove all typehints and add assertions (validator component). With that, the public API is nice, but the internal PHP is not so good (PHP 7.4+ FTW!) In APIP, they have support for transforming to [something](https://github.com/api-platform/core/blob/53837eee3ebdea861ffc1c9c7f052eecca114757/src/Core/Serializer/AbstractItemNormalizer.php#L233-L237) they can handle gracefully. But the deserialization stop on the first error (so the end user must fix the error, try again, fix the second error, try again etc.). And the raw exception message is leaked to the end user. So the API can return something like `The type of the "string" attribute for class "App\Dto\MyDto" must be one of "string" ("null" given).`. Really not cool :/ So ATM, building a nice public API is not cool. That's why I propose this PR that address all issues reported * be able to collect all error * with their property path associated * don't leak anymore internal In order to not break the BC, I had to use some fancy code to make it work 🐒 With the following code, I'm able to collect all errors, transform them in `ConstraintViolationList` and render them properly, as expected.  ```php #[Route('/api', methods:['POST'])] public function apiPost(SerializerInterface $serializer, Request $request): Response { $context = ['not_normalizable_value_exceptions' => []]; $exceptions = &$context['not_normalizable_value_exceptions']; $dto = $serializer->deserialize($request->getContent(), MyDto::class, 'json', $context); if ($exceptions) { $violations = new ConstraintViolationList(); /** `@var` NotNormalizableValueException */ foreach ($exceptions as $exception) { $message = sprintf('The type must be one of "%s" ("%s" given).', implode(', ', $exception->getExpectedTypes()), $exception->getCurrentType()); $parameters = []; if ($exception->canUseMessageForUser()) { $parameters['hint'] = $exception->getMessage(); } $violations->add(new ConstraintViolation($message, '', $parameters, null, $exception->getPath(), null)); }; return $this->json($violations, 400); } return $this->json($dto); } ``` If this PR got accepted, the above code could be transferred to APIP to handle correctly the deserialization Commits ------- ebe6551 [Serializer] Add support for collecting type error during denormalization
This PR is a similar with #38472 (for unknown reason that is a draft) but there is a slightly different approach.
This implementation is more to show how the exceptions that are thrown during denormalization can be collected.
It has two part.
In the first part it is collected
NotNormalizableValueException
s inAbstractObjectNormalizer
.The second part is a collect exceptions from
AbstractNormalizer::instantiateObject