symfony: #[MapRequestPayload] Returns full ViolationsList if object cannot be created due to a single missing value

Symfony version(s) affected

6.3.0

Description

If you use #[MapRequestPayload] and the payload is missing a single (or multiple) fields, the ViolationList created by the resolver does not contain the single missing field, but all asserts on every field.

How to reproduce

Controller:

...
public function testPayloadAction(
        #[MapRequestPayload] User $user
    ): JsonResponse {
        return new JsonResponse(data: $user->getEmail());
    }
...

DTO:

final readonly class User
{
    public function __construct(
        #[Assert\NotNull,
            Assert\NotBlank,
            Assert\Email]
        private string $email,
        #[Assert\NotNull,
            Assert\NotBlank]
        private string $password,
    ) {
    }

    public function getEmail(): string
    {
        return $this->email;
    }

    public function getPassword(): string
    {
        return $this->password;
    }
}

Payload to route:

{
  "email": "test@mail.com"
}

Excpected: I would expect the error message that PartialDenormalizationException holds in RequestPayloadValueResolver.php to be returned. In this case:

Failed to create object because the class misses the “password” property.

Actual:

:
    This value should be of type unknown.
Object(...\DTO\User).email:
    This value should not be null. (code ad32d13f-c3d4-423b-909a-857b961eb720)
Object(...\DTO\User).email:
    This value should not be blank. (code c1051bb4-d103-4f74-8988-acbcafc7fdc3)
Object(...\DTO\User).password:
    This value should not be null. (code ad32d13f-c3d4-423b-909a-857b961eb720)
Object(...\DTO\User).password:
    This value should not be blank. (code c1051bb4-d103-4f74-8988-acbcafc7fdc3)

Possible Solution

Do not run the validation on an empty object in RequestPayloadValueResolver.php

Additional Context

No response

About this issue

Original URL
State: closed
Created a year ago
Reactions: 11
Comments: 25 (11 by maintainers)

Commits related to this issue

bug #52578 [Serializer] Fix denormalize constructor arguments (mtarld) This PR was merged into the 5.4 branch. Discussion ---------- [Serializer] Fix denormalize constructor arguments | Q ... — committed to symfony/symfony by nicolas-grekas 7 months ago

Most upvoted comments

FYI: https://github.com/Havrin/symfony-payload-bug has been updated to the latest version of symfony 6.3 and the underlying setup of https://symfony.com/doc/current/setup/docker.html has been updated as well. The error is the same as in 6.3.0 Thanks for the help 😃

Edit: In symfony/serializer they did change something but it did not really fix this issue here. But it seems like the handling did change a little https://github.com/symfony/symfony/issues/51907 Edit2: seems the fix in serializer https://github.com/symfony/symfony/issues/51907 rose another issue https://github.com/symfony/symfony/issues/52422. With another DTO setup (for example with 3 values, one of them as optional) the error looks different. I think the serializer has some flaws that is showing especially in MapRequestPayload

Havrin on Nov 2, 2023

Hey! Unfortunately, this is not fixed in the latest 6.4 Update. Updated the Project https://github.com/Havrin/symfony-payload-bug to 6.4 and tested again against http://localhost/api/payload with payload:

{
  "password": "abc"
}

Result (first object is a var_dump inisde the exception subscriber):

object(App\DTO\User)#191 (0) {
    [
        "email":"App\DTO\User":private
    ]=>
  uninitialized(string)
  [
        "password":"App\DTO\User":private
    ]=>
  uninitialized(string)
}
{
    "type": "https://symfony.com/errors/validation",
    "title": "Validation Failed",
    "status": 422,
    "detail": "This value should be of type unknown.\nemail: This value should not be null.\nemail: This value should not be blank.\npassword: This value should not be null.\npassword: This value should not be blank.",
    "violations": [
        {
            "propertyPath": "",
            "title": "This value should be of type unknown.",
            "template": "This value should be of type {{ type }}.",
            "parameters": {
                "{{ type }}": "unknown",
                "hint": "Failed to create object because the class misses the \"email\" property."
            }
        },
        {
            "propertyPath": "email",
            "title": "This value should not be null.",
            "template": "This value should not be null.",
            "parameters": {
                "{{ value }}": "null"
            },
            "type": "urn:uuid:ad32d13f-c3d4-423b-909a-857b961eb720"
        },
        {
            "propertyPath": "email",
            "title": "This value should not be blank.",
            "template": "This value should not be blank.",
            "parameters": {
                "{{ value }}": "null"
            },
            "type": "urn:uuid:c1051bb4-d103-4f74-8988-acbcafc7fdc3"
        },
        {
            "propertyPath": "password",
            "title": "This value should not be null.",
            "template": "This value should not be null.",
            "parameters": {
                "{{ value }}": "null"
            },
            "type": "urn:uuid:ad32d13f-c3d4-423b-909a-857b961eb720"
        },
        {
            "propertyPath": "password",
            "title": "This value should not be blank.",
            "template": "This value should not be blank.",
            "parameters": {
                "{{ value }}": "null"
            },
            "type": "urn:uuid:c1051bb4-d103-4f74-8988-acbcafc7fdc3"
        }
    ],
    "class": "Symfony\\Component\\HttpKernel\\Exception\\HttpException",
...

The first Violation of Type unknown holds the correct message: "Failed to create object because the class misses the \"email\" property." which should be returned instead of every assert/validation.

Havrin on Dec 5, 2023

@Havrin No, it’s me. Sorry! I’ll try to have a look this week. Thanks

lyrixx on Oct 30, 2023

@zim32 It’s basically the 4th option from my list. I already have noted that

You have to replace public properties with query methods with return $this->required ?? throw new ShouldNotHappen(). Which is a lot of boilerplate (room for a mistake) I’d prefer to avoid.

Otherwise, there is a big chance you will return null in a method that should return only string. And phpstan cannot be happy about that possiblity.

luxemate on Aug 27, 2023

Hello! Do we have any updates on this issue? Im also receive this error

milshteinroman on Jul 11, 2023

@lyrixx yeah that’s true, notBlank is already checking null on default, but this is just a demo. Removing NotNull does not change a lot. The response is

<!-- This value should be of type unknown.
This value should not be blank.
This value should not be blank. (422 Unprocessable Content) -->

which again, is wrong, because email was sent and is not blank.

@luxemate interesting. If this is a wanted behaviour from the serializer, I would still suggest, that the PayloadResolver is not doing a validation on that empty object after catching the PartialDenormalizationException. It makes no sense to do that.

Havrin on Jul 4, 2023

Hello

I fail to see why you are using not blank and not null a the same time. Not blank already cover not null so remove the last one and everything will be better ?

lyrixx on Jul 3, 2023

@Havrin @ro0NL It basically happens because of the else statement here on line 409: https://github.com/symfony/symfony/blob/3e172b31c9178935c5a8ba352e6dab95cb59afcd/src/Symfony/Component/Serializer/Normalizer/AbstractNormalizer.php#L351-L411

Moreover, in the similar setup I get one more error about the missing field (actually, property path is null) This value should be of type unknown.. It also comes from that else statement.

I’ve tried different approaches to deal with this issue last weekend, but they all come with a bunch of trade-offs.

No constructor + non-nullable properties.

Pros:
- Required parameters are clearly visible.
- All correctly provided values are filled in, other properties are left uninitialized.
Cons:
- Triggers static analysis errors like phpstan: Class SomeRequest has an uninitialized property $required. Give it default value or assign it in the constructor..
- If not validated properly, it will blow up the downstream code with access to uninitialized properties.
- Parameters with wrong types are reported as blank.
Constructor + non-nullable parameters.

Pros:
- Required parameters are clearly visible.
Cons:
- If any parameter is missing or has an invalid type, then an empty object is constructed.
- Validation messages are incorrect.
Constructor + non-nullable paramters + zero initialization.

Pros:
- Object cannot be constructed with invalid types.
Cons:
- Required parameters are not separable from optional.
- Required parameters that were missing are silently substituted with a default which might not qualify as a blank value in some cases (int field that must not have a default, and which allows values from -90 to +90).
Constructor + nullable parameters.

Pros:
- Object cannot be constructed with invalid types apart from null.
- Required parameters are somewhat visible (they have no default value).
- All correctly provided values are filled in, other properties are set to null.
Cons:
- You have to replace public properties with query methods with return $this->required ?? throw new ShouldNotHappen(). Which is a lot of boilerplate (room for a mistake) I’d prefer to avoid.
- Or otherwise in the downstream code you’ll have to check or cast values, that must not be missing by design.
- Or otherwise static analysis won’t be happy.
- Parameters with wrong types are reported as blank.

Maybe I forgot a thing or two, but should be fairly close. None of the options are ideal, but IMO 1 and 4 are a bit better. Since PHPStan error can’t be turned off for child classes of some other class, I would go with 4 and query methods.

To summarize, the ideal solution should have all those traits:

Object cannot be constructed with invalid types.
Required parameters are clearly visible.
Missing or empty parameters are reported as blank.
Parameters with the wrong types are reported as wrong types.
Downstream code receives only allowed types.
Static analysis is happy.

I guess that would require deserializing a JSON into stdClass, and validating that stdClass using annotations from the DTO class. Then if validation went through, deserialize a JSON into DTO. But validator allows only to validate a parameter using rules from annotations in a class, but not validating an object using rules from annotations in another class.

That would be the ultimate solution, but… the complexity of the validator just melts my brain. Plus, I don’t know how to deal with collections, especially with abstract collections, where each implementation could have it’s own validation rules. Seems, it’s impossible to deal with them using this approach.

Maybe someone has other workarounds or thoughts on the topic?

luxemate on Jul 3, 2023

Related to the error message, I think it would be helpful to have the parameter names in the exception, like "password" should not be blank instead of This value should not be blank.

BafS on Jun 29, 2023

How to map snake_case request variable to camelCase?

final readonly class CreateCompanyRequest
{
    public function __construct(
        #[Assert\Length(min: 3, max: 255)]
        public string $bank_address,
    ) {
    }
}

public function __invoke(#[MapRequestPayload] CreateCompanyRequest $request)
{
   $bankAddress = $request->bank_address;  
 // $request->bankAddress ==> here I want to have bankAddress variable in my request
}

dykyi-roman on Aug 30, 2023

same issue

sridharkalaibala on Aug 16, 2023

Hello, I also have the same problem.

Couss on Jul 25, 2023

@BafS it’s unrelated and is existing translation infrastructure. Usually the messages are put on the field, and clients can leverage the property path also. One is free to use custom translations for asserts ofcourse.

@Havrin please debug 😄 im really curious what happens to your payload. Perhaps better create a reproducer app.

ro0NL on Jun 29, 2023

That’s acutally the complete output, I just copied it while debugging through my ExceptionEventSubscriber. Here a little bit more from the profiler, maybe that helps 😃 profiler-validator

Havrin on Jun 26, 2023