MapQueryString & MapRequestPayload empty when type missmatch #50904

Grafikart · 2023-07-06T14:27:06Z

Symfony version(s) affected

6.3.1

Description

When there is a type missmatch on an objet with the attribute MapQueryString or MapRequestPayload an empty instance is created.

For instance if I use MapQueryString expecting a name (string) and age (int) but the user send a "John" and "foo", I end up with an empty object.

How to reproduce

Create a simple DTO (I deliberately omitted NotBlank)

final readonly class PayloadDTO
{

    public function __construct(
        #[Assert\Length(min: 3)]
        public string $name,

        #[Assert\Positive]
        public int    $age
    )
    {
    }

}

Map this DTO in a controller

class HomeController extends AbstractController
{
    #[Route('/', name: 'home')]
    public function index(
        #[MapQueryString] PayloadDTO $payload,
    ): Response
    {
        dd($payload); // I expect the dto to be valid (with an age / name property)
    }

}

Try to access the page using an "age" that is not a number. http://localhost:8000/?name=John&age=foo, you'll end up with an empty PayloadDTO (which will cause bugs further down the application).

Possible Solution

I tracked down the issue to the AbstractNormalizer that ends up instantiating the object without using the constructor (using the reflectionClass) since there is a not_normalizable_value_exceptions in the context. https://github.com/symfony/symfony/blob/6.4/src/Symfony/Component/Serializer/Normalizer/AbstractNormalizer.php#L424-L428

A solution would be to check the context for "collect_denormalization_errors" before skipping the exception or to collect the exception when it happens.

            try {
                return $reflectionClass->newInstanceArgs($params);
            } catch (\TypeError $e) {
                if (!isset($context['not_normalizable_value_exceptions'])) {
                    throw $e;
                }

                $context['not_normalizable_value_exceptions'][] = NotNormalizableValueException::createForUnexpectedDataType(/* Find which parameters to give */);

                return $reflectionClass->newInstanceWithoutConstructor();
            }

The text was updated successfully, but these errors were encountered:

BafS · 2023-07-19T09:51:33Z

Same issue for us. I asked the question on slack without success.

The workaround I did is to not use the constructor:

final class PayloadDTO
{
    #[Assert\Length(min: 3)]
    public ?string $name = null;

    #[Assert\Positive]
    public ?int $age = null;
}

Like that you can never have an empty object.

lukasojd · 2023-09-03T22:35:42Z

+1

saxap · 2023-09-07T08:02:02Z

same issue

rlandgrebe · 2023-09-30T12:53:25Z

I have the same issue when using the MapRequestPayload annotation. The problem seems to happen in this check:

symfony/src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php

Lines 593 to 597 in 0b78897

    
8000
  
           if ($context[self::DISABLE_TYPE_ENFORCEMENT] ?? $this->defaultContext[self::DISABLE_TYPE_ENFORCEMENT] ?? false) { 
        
               return $data; 
        
           } 
        
           throw NotNormalizableValueException::createForUnexpectedDataType(sprintf('The type of the "%s" attribute for class "%s" must be one of "%s" ("%s" given).', $attribute, $currentClass, implode('", "', array_keys($expectedTypes)), get_debug_type($data)), $data, array_keys($expectedTypes), $context['deserialization_path'] ?? $attribute);

where $context[self::DISABLE_TYPE_ENFORCEMENT] evaluates to true and therefore the data is returned. If this would have evaluated to false a NotNormalizableValueException would have been thrown.

Checking where this context variable is set to true I found the context settings where type enforcement is disabled:

symfony/src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/RequestPayloadValueResolver.php

Lines 46 to 49 in 0b78897

    
           private const CONTEXT_DENORMALIZE = [ 
        
               'disable_type_enforcement' => true, 
        
               'collect_denormalization_errors' => true, 
        
           ];

The fix would therefore be easy, but I'm not sure if this is the intended behaviour because enabling type enforcement would also mean that an integer given as a string (e.g. "123") is not automatically converted to an int. It's a question of strictness.

rlandgrebe · 2023-10-09T16:50:08Z

New findings from my side: This only happens when the following code is executed:

symfony/src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/RequestPayloadValueResolver.php

Lines 177 to 179 in 0b78897

    
           if ($data = $request->request->all()) { 
        
               return $this->serializer->denormalize($data, $type, null, $attribute->serializationContext + self::CONTEXT_DENORMALIZE); 
        
           }

Otherwise

symfony/src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/RequestPayloadValueResolver.php

Line 190 in 0b78897

    
           return $this->serializer->deserialize($data, $type, $format, self::CONTEXT_DESERIALIZE + $attribute->serializationContext);

is called and everything runs fine.

So I found another component in our code that automatically converted the request data to an array. In this case, only denormalization takes place and an empty instance is created. The conversion snippet is this one here: https://medium.com/@peter.lafferty/converting-a-json-post-in-symfony-13a24c98fc0e

jurchiks · 2023-12-08T05:16:40Z

I have just now stumbled upon a very similar, if not the same bug. I was trying to implement generic interfaces for some of my requests:

interface GetListRequest
{
    public function getFilters(): ?ListFilters;
    // I have more getters, pruned for brevity.
}

interface ListFilters {}

readonly class SpecificListFilters
{
  public function __construct(
    // any fields, doesn't matter for this example
    public ?string $foo = null
  ) {
  }
}

readonly class GetSpecificListRequest implements GetListRequest
{
    public function __construct(
        #[Assert\Valid]
        public ?SpecificListFilters $filters,
    ) {
    }

    public function getFilters(): ?ListFilters <- GetSpecificListRequest is empty
    public function getFilters(): ?SpecificListFilters <- GetSpecificListRequest is populated
    {
        return $this->filters;
    }
}

public function index(#[MapQueryString] GetSpecificListRequest $request) ...

This makes it annoying to use generic interfaces for these mapped DTOs, I cannot DRY my DTOs by using a generic trait for the getters:

trait GetListRequestImpl
{
    public function getFilters(): ?ListFilters
    {
        return $this->filters;
    }
}

Which forces me to copy-paste the same identical getters into every DTO and change the return types from interface to implementation, even though interface absolutely should work.

Wolg · 2024-01-08T09:30:15Z

Same issue here! Any chance this PR can be prioritized for the next release? Otherwise MapQueryString/MapRequestPayload are not very usable.
When validating promoted property and passing the wrong type serializer ends up with an empty DTO. For the same scenario, but when the property is defined with traditional syntax, the type context misbehaves and results in This value should be of type unknown.

KevsRepos · 2024-03-08T10:29:16Z

I have the same issue but its "fixed" when I remove the readonly flag from all properties. Symfony then responds with 422 but somehow cant resolve the type anymore, as the error message is "This value should be of type unknown.".

rlandgrebe · 2024-05-15T09:34:51Z

I'm again debugging this problem and decided to go a little bit deeper. It's seems to be a combination of two factors:

First: Disabling the type enforcement in the RequestPayloadValueResolver:

symfony/src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/RequestPayloadValueResolver.php

Line 53 in ad68f73

'disable_type_enforcement' => true,

This is normally good behavior, because if you have a "123" (string) this will be automatically converted to 123 (int). This is useful for query strings where each parameter is given as a string.

Second: Insufficient validation of data in the AbstractObjectNormalizer:

symfony/src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php

Line 625 in ad68f73

    
           private function validateAndDenormalize(Type $type, string $currentClass, string $attribute, mixed $data, ?string $format, array $context): mixed

This method validates and denormalizes the data, but if I try to map a string "hello" to an integer type, no check in this method will catch the error. In the end it will reach this part:

symfony/src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php

Lines 610 to 612 in ad68f73

    
           if ($context[self::DISABLE_TYPE_ENFORCEMENT] ?? $this->defaultContext[self::DISABLE_TYPE_ENFORCEMENT] ?? false) { 
        
               return $data; 
        
           }

And that is the main problem, because RequestPayloadValueResolver disables type enforcement. This is good and needed. But in my opinion it has no sense to disable type enforcement completely, because there's no way to convert "hello" to an integer. I think the best way is to check if the type is coercible, e.g. "123" (string) -> 123 (integer) is okay, but "hello" is not okay to convert to an integer. Therefore it has to throw a NotNormalizableValueException, even if type enforcement is disabled. Type enforcement, though, is still useful for being strict about those "123" -> 123 coercions.

Funny thing is that types are checked this way for the XML and CSV format, e.g.:

symfony/src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php

Lines 683 to 689 in ad68f73

    
           case TypeIdentifier::INT: 
        
               if (ctype_digit(isset($data[0]) && '-' === $data[0] ? substr($data, 1) : $data)) { 
        
                   $data = (int) $data; 
        
               } else { 
        
                   throw NotNormalizableValueException::createForUnexpectedDataType(sprintf('The type of the "%s" attribute for class "%s" must be int ("%s" given).', $attribute, $currentClass, $data), $data, [Type::int()], $context['deserialization_path'] ?? null); 
        
               } 
        
               break;

dwgebler · 2024-05-29T14:08:09Z

You can write #[MapRequestPayload(serializationContext: ['disable_type_enforcement' => false])] to override the context for denormalization. I had this issue because I have an early request listener which automatically overwrites $request->request with decoded JSON payload for JSON requests.

ovidiuenache · 2024-12-07T22:00:07Z

This has been reported again in #59104. There's a PR attached to it with the fix and a more detailed description of the issue. The fix will make it work as expected without changing your DTOs or providing a custom context to the attribute.

… when using "#[MapQueryString]" or "#[MapRequestPayload]" (except for content data) (ovidiuenache) This PR was squashed before being merged into the 6.4 branch. Discussion ---------- [HttpKernel] Denormalize request data using the csv format when using "#[MapQueryString]" or "#[MapRequestPayload]" (except for content data) | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | #59104 #50904 | License | MIT This fixes the scenario mentioned above where using `#[MapQueryString]` or `#[MapRequestPayload]` (except request content) with at least one readonly property with a type mismatch error leads to getting an object with uninitialized properties. The only properties that are set are the ones that are assigned a value via a request parameter and are NOT readonly. Moreover, if the corresponding query parameter is not provided for non-readonly property A and there is at least one other readonly property B that has a type mismatch then A will still be returned as uninitialized (even if it has a default value). Shortly put, the instantiation fails and the values of the properties cannot be set later on. Examples ``` class FilterDto { public function __construct( public readonly ?int $id = null, public readonly ?string $name = null, public ?string $description = null, ) { } } GET https://127.0.0.1:8000/test?id=x&name=test id: ? ?int name: ? ?string description: ? ?string GET https://127.0.0.1:8000/test?id=x&name=test&description=desc id: ? ?int name: ? ?string description: "desc" ``` The complete list of steps to reproduce this is provided in #59104. The reason why this happens is because we are disabling the type enforcement of the denormalizer in the `Symfony\Component\HttpKernel\Controller\ArgumentResolver\RequestPayloadValueResolver` class and when we eventually end up in the `validateAndDenormalize` method of the `Symfony\Component\Serializer\Normalizer\AbstractObjectNormalizer` class we ignore the type mismatch because of: ``` if ($context[self::DISABLE_TYPE_ENFORCEMENT] ?? $this->defaultContext[self::DISABLE_TYPE_ENFORCEMENT] ?? false) { return $data; } ``` Thus, we get a type error when trying to create the object and we fall back to `$reflectionClass->newInstanceWithoutConstructor();` Then, based on the provided request data, we attempt to set the values of the properties but this process fails for the readonly properties so they stay uninitialized. As discussed with `@nicolas`-grekas during [the hackathon at SymfonyCon Vienna 2024](https://live.symfony.com/2024-vienna-con/) the solution here is to stop disabling the type enforcement of the denormalizer. However, this alone is not enough because then we won't be able to use anything but string since this is the type that comes in the request so we also need to set the denormalization format to either `csv` or `xml`. This comment from `the Symfony\Component\Serializer\Normalizer\AbstractObjectNormalizer` sheds some light on why: ``` // In XML and CSV all basic datatypes are represented as strings, it is e.g. not possible to determine, // if a value is meant to be a string, float, int or a boolean value from the serialized representation. // That's why we have to transform the values, if one of these non-string basic datatypes is expected. ``` We avoid `xml` due to some special formatting that occurs so the proposed solution uses `csv`. Basically, we start using type enforcement + csv format where non-string values are transformed. Commits ------- e0957a0 [HttpKernel] Denormalize request data using the csv format when using "#[MapQueryString]" or "#[MapRequestPayload]" (except for content data)

Grafikart added the Bug label Jul 6, 2023

carsonbot added the Status: Needs Review label Jul 6, 2023

xabbuh added the HttpKernel label Jul 6, 2023

kylekatarnls mentioned this issue Jul 18, 2023

[Serializer] Handle type error constructing input #51003

Open

ovidiuenache mentioned this issue Dec 7, 2024

[HttpKernel] Denormalize request data using the csv format when using "#[MapQueryString]" or "#[MapRequestPayload]" (except for content data) #59134

Merged

fabpot closed this as completed Dec 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MapQueryString & MapRequestPayload empty when type missmatch #50904

MapQueryString & MapRequestPayload empty when type missmatch #50904

MapQueryString & MapRequestPayload empty when type missmatch #50904

MapQueryString & MapRequestPayload empty when type missmatch #50904

Comments

Symfony version(s) affected

Description

How to reproduce

Possible Solution