Skip to content
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

#[MapRequestPayload] Returns full ViolationsList if object cannot be created due to a single missing value (re-open) #53075

Closed
Havrin opened this issue Dec 14, 2023 · 4 comments

Comments

@Havrin
Copy link

Havrin commented Dec 14, 2023

Symfony version(s) affected

6.4

Description

Hey! I re create #50759 since a comment did not re-open it. Unfortunately, this is not fixed in the latest 6.4 Update.

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

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.

Possible Solution

No response

Additional Context

Apologize if re creating a new ticket was not the right move. Feel free to close this and re open the other one.

@HypeMC
Copy link
Contributor

HypeMC commented Dec 17, 2023

@Havrin Could you see if #53107 fixes your issue.

@Havrin
Copy link
Author

Havrin commented Dec 18, 2023

@HypeMC The result looks like this:

{
    "type": "https://symfony.com/errors/validation",
    "title": "Validation Failed",
    "status": 422,
    "detail": "This value should be of type unknown.",
    "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."
            }
        }
    ],
...

so yeah, it only validates the correct one, but the message is still wrong in my opinion. The hint should be displayed as the message. But it is better than before, thanks a lot :)

@HypeMC
Copy link
Contributor

HypeMC commented Dec 18, 2023

@Havrin The message being incorrect is a hole different issue, see #52879. A similar thing should be done for

$exception = NotNormalizableValueException::createForUnexpectedDataType(
sprintf('Failed to create object because the class misses the "%s" property.', $constructorParameter->name),
$data,
['unknown'],
$context['deserialization_path'] ?? null,
true
);

@Havrin
Copy link
Author

Havrin commented Dec 18, 2023

perfect, than I would say your fix is enough and in combination with the other issue, the user experience will be way better. Thanks!

nicolas-grekas added a commit that referenced this issue Dec 18, 2023
…(HypeMC)

This PR was merged into the 6.3 branch.

Discussion
----------

[HttpKernel] Don't validate partially denormalized object

| Q             | A
| ------------- | ---
| Branch?       | 6.3
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | Fix #53075
| License       | MIT

When one or more required constructor arguments are missing, the object is instantiated with `newInstanceWithoutConstructor()`:
https://github.com/symfony/symfony/blob/0af28a02dbfabef351de8a739dee65830ed0271a/src/Symfony/Component/Serializer/Normalizer/AbstractNormalizer.php#L415-L421

Since this means that all properties are empty, validating the entire object is unnecessary. This PR suggests avoiding the call to the validator when violations have already been identified through the `PartialDenormalizationException`.

Commits
-------

f9a0126 [HttpKernel] Don't validate partially denormalized object
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants