Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:5.4fromlyrixx:seria-typ-val
Sep 10, 2021

Conversation

lyrixx
Copy link
Member

@lyrixxlyrixx commentedAug 12, 2021
edited
Loading

QA
Branch?5.4
Bug fix?no
New feature?yes
Deprecations?no
TicketsFix#27824,Fix#42236,Fix#38472,Fix#37419Fix#38968
LicenseMIT
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:

class MyDto{publicstring$string;publicint$int;publicfloat$float;publicbool$bool;public\DateTime$dateTime;public\DateTimeImmutable$dateTimeImmutable;public\DateTimeZone$dateTimeZone;public\SplFileInfo$splFileInfo;publicUuid$uuid;publicarray$array;/** @var MyDto[] */publicarray$collection;}

and the following 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:

image

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 tosomething 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 likeThe 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 inConstraintViolationList and render them properly, as expected.

image

    #[Route('/api', methods:['POST'])]publicfunctionapiPost(SerializerInterface$serializer,Request$request):Response    {try {$dto =$serializer->deserialize($request->getContent(), MyDto::class,'json', [                DenormalizerInterface::COLLECT_DENORMALIZATION_ERRORS =>true,            ]);        }catch (PartialDenormalizationException$e) {$violations =newConstraintViolationList();/** @var NotNormalizableValueException */foreach ($e->getErrors()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(newConstraintViolation($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

root-aza, BafS, Warxcell, MyZik, jdreesen, RuSPanzer, DrummerKH, julienfalque, falkenhawk, SerhiiPlotnikov, and 18 more reacted with thumbs up emojiBafS, Warxcell, MyZik, falkenhawk, juuuuuu, root-aza, Koc, and maks-rafalko reacted with hooray emojiWarxcell, MyZik, jdreesen, falkenhawk, rvanlaak, juuuuuu, root-aza, jeremyFreeAgent, kubk, Cryde, and 4 more reacted with heart emojiroot-aza, Warxcell, MyZik, falkenhawk, rvanlaak, juuuuuu, mtarld, yceruto, kubk, Cryde, and maks-rafalko reacted with rocket emoji
@carsonbot
Copy link

Hey!

I think@VincentLanglet has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

Copy link
Member

@jderussejderusse left a 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..

@lyrixx
Copy link
MemberAuthor

I'm mix feeling with this PR that brings validation without to much changes. But also silents deserialization failures..

I understand this is a bit weird. But this process has an advantage overs others PR: you can get the hydrated objectand the errors.
So you can merge deserialization errors with validations errors and return everything to the end user.

@jderusse
Copy link
Member

I'm mix feeling with this PR that brings validation without to much changes. But also silents deserialization failures..

I understand this is a bit weird. But this process has an advantage overs others PR: you can get the hydrated objectand the errors.
So you can merge deserialization errors with validations errors and return everything to the end user.

Yeah, that's what I like in your PR!.

What's about

try {   $o = $serializer->unserialize(...);} catch (PartialDenormalizationException $e) {  $o = $e->getPartialObject();  $exceptions = $e->getExceptions();}
ro0NL reacted with thumbs up emoji

@lyrixx
Copy link
MemberAuthor

Oh, this is a good idea too! I'll update the PR

@ro0NL
Copy link
Contributor

about partial objects, im not sure they should ever leak.

you can get the hydrated object and the errors

dont see the usecase yet 🤔

@lyrixx
Copy link
MemberAuthor

about partial objects, im not sure they should ever leak.

I'm not sure I follow you :/

dont see the usecase yet

See the PR description => you can get the whole (type + validator) validation at once

@ro0NL
Copy link
Contributor

} catch (PartialDenormalizationException $e) {  $o = $e->getPartialObject();

would be leaking something invalid isnt it?

IIUC in your PR example code you dont actually need a partial$dto within theif ($exceptions) { block. Or why would you?

@lyrixx
Copy link
MemberAuthor

would be leaking something invalid isnt it?

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)

IIUC in your PR example code you dont actually need a partial$dto within theif ($exceptions) { block. Or why would you?

In my example, it don't trigger the validation, but it would be even better if it's the case ❤️

@ro0NL
Copy link
Contributor

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.

@ro0NL
Copy link
Contributor

In my example, it don't trigger the validation, but it would be even better if it's the case

well i think we should serialize the NotNormalizableValueException to a constration violation list yes

@ro0NL
Copy link
Contributor

ohh =) you mean$validator->validate($dto), after deserialize 🤦 i see we have 2 point of views.

im really not sure one should continue from an invalid state, assuming validator does some job maybe.

@nicolas-grekasnicolas-grekas added this to the5.4 milestoneAug 18, 2021
@lyrixx
Copy link
MemberAuthor

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?

class Test{    public int $int; // required, but a string is passed    public bool $bool; // required, nothing is passed}
  • the "int" value must be a int, string passed
  • the "bool" value must not be blank

=> this is nice for the end user to report all errors. But again, it's up to the developer to choose

@ro0NL
Copy link
Contributor

ro0NL commentedAug 18, 2021
edited
Loading

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 :)

@lyrixx
Copy link
MemberAuthor

I pushed another implementation where I introduced aPartialDenormalizationException as asked by@jderusse

I also addressed CS comments.

I'm not sure about the@ro0NL comment to add other exception classes. What do other people think ?

@fabpot
Copy link
Member

Thank you@lyrixx.

@julienfalque
Copy link
Contributor

Thanks@lyrixx!

lyrixx reacted with heart emoji

@lyrixxlyrixx deleted the seria-typ-val branchSeptember 10, 2021 08:50
hultberg pushed a commit to hultberg/symfony that referenced this pull requestSep 17, 2021
…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       |Fixsymfony#27824,Fixsymfony#42236,Fixsymfony#38472,Fixsymfony#37419Fixsymfony#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:```phpclass 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:![image](https://user-images.githubusercontent.com/408368/129211588-0ce9064e-171d-42f2-89ac-b126fc3f9eab.png)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 internalIn 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.![image](https://user-images.githubusercontent.com/408368/129215560-b0254a4e-fec7-4422-bee0-95cf9f9eda6c.png)```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 deserializationCommits-------ebe6551 [Serializer] Add support for collecting type error during denormalization
fabpot added a commit that referenced this pull requestSep 21, 2021
…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#42502Commits-------903a94a [Serializer] Save missing arguments in MissingConstructorArgumentsException
This was referencedNov 5, 2021
@roman-eonx
Copy link

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?

itorgov, akl-95, and sheepwall reacted with thumbs up emoji

fabpot added a commit that referenced this pull requestDec 13, 2023
…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       | MITThis 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
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@BafSBafSBafS left review comments

@ro0NLro0NLro0NL approved these changes

@KocalKocalKocal left review comments

@fabpotfabpotfabpot approved these changes

@dunglasdunglasdunglas approved these changes

@jderussejderussejderusse approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.

[RFC][Serializer] Ability to collect denormalization failures [Serializer] Do not throw exception in the DateTimeNormalizer if it's not a date
11 participants
@lyrixx@carsonbot@jderusse@ro0NL@fabpot@julienfalque@roman-eonx@dunglas@BafS@Kocal@nicolas-grekas

[8]ページ先頭

©2009-2025 Movatter.jp