MapQueryString & MapRequestPayload empty when type missmatch

[Grafikart]

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

1. 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
    )
    {
    }

}

2. 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)
    }

}

3. 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();
            }

[BafS]

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]

+1

[saxap]

same issue

[rlandgrebe]

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

	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]

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]

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]

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]

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]

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]

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.