-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Serializer] Add support for collecting type error during denormalization #42502
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
ab7348b
to
6365c07
Compare
Hey! I think @VincentLanglet has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
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.
replaces #27824 ?
I'm mix feeling with this PR that brings validation without to much changes. But also silents deserialization failures..
src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Serializer/Exception/NotNormalizableValueException.php
Outdated
Show resolved
Hide resolved
I understand this is a bit weird. But this process has an advantage overs others PR: you can get the hydrated object and the errors. |
Yeah, that's what I like in your PR!. What's about
|
Oh, this is a good idea too! I'll update the PR |
src/Symfony/Component/Serializer/Exception/NotNormalizableValueException.php
Show resolved
Hide resolved
about partial objects, im not sure they should ever leak.
dont see the usecase yet 🤔 |
I'm not sure I follow you :/
See the PR description => you can get the whole (type + validator) validation at once |
would be leaking something invalid isnt it? IIUC in your PR example code you dont actually need a partial |
anyway, it's always the case, that's why we validate the data after. Same with form (but the form component trigger the validation itself)
In my example, it don't trigger the validation, but it would be even better if it's the case ❤️ |
currently no invalid object leaks isnt it? you get a NotNormalizableValueException then i agree with @jderusse , as i have mixed feelings about silencing those now. |
well i think we should serialize the NotNormalizableValueException to a constration violation list yes |
ohh =) you mean im really not sure one should continue from an invalid state, assuming validator does some job maybe. |
It's up to the final developper, but why not?
=> this is nice for the end user to report all errors. But again, it's up to the developer to choose |
got ya. Reporting deserialization AND validation in one go is also nice yes 👍 (overread #42502 (comment)) I agree with #42502 (comment) to optin 👍 sorry if i was noisy :) |
Thank you @lyrixx. |
Thanks @lyrixx! |
…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
…torArgumentsException (BafS) This PR was squashed before being merged into the 5.4 branch. Discussion ---------- [Serializer] Save missing arguments in MissingConstructorArgumentsException | Q | A | ------------- | --- | Branch? | 6.0 (but it could maybe be 5.4?) | Bug fix? | no | New feature? | not really | Deprecations? | no | Tickets | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead --> | License | MIT | Doc PR | symfony/symfony-docs#... <!-- required for new features --> It's currently not possible to get the information about which constructor argument is missing (except by doing regex in the exception message string). Having this information is useful when we want to have an input transformer without the need to set every fields to nullable. For example with this flow: `[request payload] -> [normalizer/unserializer] -> [dto] -> [validation]`, each field must be nullable to not have exception during the normalization and then we still need to validate, instead we could catch `MissingConstructorArgumentsException`, handle it and make a nice http exception. Edit: related to #42502 Commits ------- 903a94a [Serializer] Save missing arguments in MissingConstructorArgumentsException
Thanks, @lyrixx for this fix we were waiting that long! Are you planning to make a PR into APIP to start using this new feature? |
…hat exception it throws (Nyholm) This PR was merged into the 5.4 branch. Discussion ---------- [Serializer] Make sure Serializer::denormalize() shows what exception it throws | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Issues | no | License | MIT This was missing from #42502. We have no contract that explains that this exceptions should be thrown. With this addition, we technically make this a "sometimes" feature of the Serializer class. Commits ------- bca846b Make sure Serializer::denormalize have show what exception it throws
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:
and the following JSON:
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 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
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.If this PR got accepted, the above code could be transferred to APIP to handle correctly the deserialization