Skip to content

[Serializer] Improve nested payload validation for #[MapRequestPayload] using a new serialization context #53250

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

Draft
wants to merge 1 commit into
base: 7.4
Choose a base branch
from

Conversation

mmouih
Copy link

@mmouih mmouih commented Dec 28, 2023

Q A
Branch? 7.1
Bug fix? no
New feature? yes
Deprecations? no
License MIT

Hello everyone,

When validating a nested object using MapRequestPayload, and given payload schema doesn't respect the DTO provided, Symfony serializer throws PartialDenormalizationException.

Unfortunately the MapRequestPayloadResolver doesn't have enough information about the Dto structure to be able to provide a more detailed error, Instead symfony throws the errors "This value should be of type unknown."

image

This unknown type is originated from the AbstractNormalizer in the serializer component, which injects this value to NotNormalizableValueException class.
I propose to add new Serialization context AbstractNormalizer::USE_CLASS_AS_DEFAULT_EXPECTED_TYPE,
which allows us to retrieve the class as the expected type for Our DTO if needed.

Example when using the context AbstractNormalizer::USE_CLASS_AS_DEFAULT_EXPECTED_TYPE:
image

I honestly do not know the reason behind using unkown instead of the FQCN. I find adding a new context more safe, and certainly Backward compatible to support both usages.

Usage example in a controller:

https://github.com/mmouih/apps-demo/blob/19f9edfa4361567a4b592a32041ead327fb3b898/src/Controller/HomeController.php#L22,L28

I included tests for the new feature:

https://github.com/mmouih/symfony/blob/05ef92028cb75c556ebbf2857617bdf40b8f1643/src/Symfony/Component/HttpKernel/Tests/Controller/ArgumentResolver/RequestPayloadValueResolverTest.php#L288C21-L288C94

Best regards.

@carsonbot
Copy link

Hey!

Thanks for your PR. You are targeting branch "7.1" but it seems your PR description refers to branch "7.1 for features 6.3, 6.4, 7.0".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot

@carsonbot carsonbot changed the title #[MapRequestPayload][Serializer] improve nested payload validation for MapRequestPayload using a new serialization context [Serializer] #[MapRequestPayload] improve nested payload validation for MapRequestPayload using a new serialization context Dec 28, 2023
@mmouih mmouih force-pushed the improve_payload_mapper_error_reporting branch 2 times, most recently from a25d919 to 62d7c88 Compare December 28, 2023 03:08
@OskarStark OskarStark changed the title [Serializer] #[MapRequestPayload] improve nested payload validation for MapRequestPayload using a new serialization context [Serializer] Improve nested payload validation for #[MapRequestPayload] using a new serialization context Dec 28, 2023
@mmouih mmouih force-pushed the improve_payload_mapper_error_reporting branch from 62d7c88 to 55880a5 Compare December 29, 2023 00:15
…r MapRequestPayload using a new serialization context
@mmouih mmouih force-pushed the improve_payload_mapper_error_reporting branch from 55880a5 to 988304e Compare December 29, 2023 00:18
$arguments = $resolver->resolve($request, $argument);
$event = new ControllerArgumentsEvent($kernel, function () {}, $arguments, $request, HttpKernelInterface::MAIN_REQUEST);

try {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can leverage expectException and expectExceptionMessage instead

@mmouih mmouih marked this pull request as draft January 6, 2024 01:18
@xabbuh xabbuh modified the milestones: 7.1, 7.2 May 15, 2024
@fabpot fabpot modified the milestones: 7.2, 7.3 Nov 20, 2024
@fabpot fabpot modified the milestones: 7.3, 7.4 May 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants