8000 [Serializer] Handling array properties during denormalization · Issue #51261 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content
8000

[Serializer] Handling array properties during denormalization #51261

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.

Sign up for GitHub

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

Jump to bottom
Closed
X-Coder264 opened this issue Aug 4, 2023 · 3 comments
Closed

[Serializer] Handling array properties during denormalization #51261

X-Coder264 opened this issue Aug 4, 2023 · 3 comments
Labels
Bug Serializer Status: Needs Review

Comments

@X-Coder264
Copy link
Contributor
X-Coder264 commented Aug 4, 2023 •
edited
Loading

Symfony version(s) affected

6.3

Description

Updating from Serializer 5.4 to 6.3 breaks our testsuite as the denormalizer which is supposed to denormalize an array property does not get executed anymore as it does implement the getSupportedTypes method and in the serializer code path that handles the getSupportedTypes logic it no longer calls the supportsDenormalization method on that denormalizer (the concrete example can be seen under the steps to reproduce). I'm not entirely familiar with the Serializer component so maybe I'm just doing something wrong.

How to reproduce

The interfaces/classes needed to reproduce this:

interface FooBaseDummy
{
}
class FooDummy implements FooBaseDummy
{
    /**
     * @var string
     */
    public $name;
}
class ArrayPropertyDummy
{
    /**
     * @var FooDummy[]
     */
    public $foo;

    public function getFoo(): array
    {
        return $this->foo;
    }
}
final class FooBaseDummyDenormalizer implements DenormalizerInterface
{
    public function denormalize(mixed $data, string $type, string $format = null, array $context = [])
    {
        $result = [];
        foreach ($data as $foo) {
            $fooDummy = new FooDummy();
            $fooDummy->name = $foo['name'];
            $result[] = $fooDummy;
        }

        return $result;
    }

    public function supportsDenormalization(mixed $data, string $type, string $format = null)
    {
        if (mb_substr($type, -2) === '[]') {
            $className = mb_substr($type, 0, -2);
            $classImplements = class_implements($className);
            \assert(\is_array($classImplements));

            return class_exists($className) && \in_array(FooBaseDummy::class, $classImplements, true);
        }

        return false;
    }

    /**
     * @return array<string, bool>
     */
    public function getSupportedTypes(?string $format): array
    {
        return [FooBaseDummy::class.'[]' => false];
    }
}

The test that I expect to pass but currently does not with Serializer 6.3:

    public function testDeserializeOnObjectWithArrayProperty()
    {
        $serializer = new Serializer([new FooBaseDummyDenormalizer(), new ObjectNormalizer(null, null, null, new PhpDocExtractor())], [new JsonEncoder()]);

        $obj = $serializer->deserialize('{"foo":[{"name":"bar"}]}', ArrayPropertyDummy::class, 'json');
        $this->assertInstanceOf(ArrayPropertyDummy::class, $obj);
        $this->assertInstanceOf(FooDummy::class, $obj->getFoo()[0]);
    }

The test passes when using Serializer 5.4

Removing the getSupportedTypes method from the FooBaseDummyDenormalizer when using Serializer 6.3 makes the test pass again (but with a deprecation). The FooBaseDummyDenormalizer is a simplified equivalent of a class from a bundle we use in our app.
When we use Serializer 5.4 the serializer calls $normalizer->supportsDenormalization which returns true and everything works fine ->

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

Line 315 in b39fcef

} elseif ($normalizer->supportsDenormalization(null, $class, $format, $context)) {

When we use Serializer 6.3 it skips that code path as the denormalizer has the getSupportedTypes method so it enters this if statement which just does a continue ->

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

Lines 363 to 367 in bbb5f5c

if (\in_array($supportedType, ['*', 'object'], true)
|| $class !== $supportedType && ('object' !== $genericType || !is_subclass_of($class, $supportedType))
) {
continue;
}

$supportedType here is Symfony\Component\Serializer\Tests\Fixtures\FooBaseDummy[], $class is Symfony\Component\Serializer\Tests\Fixtures\FooDummy[] and $genericType is *

Possible Solution

No response

Additional Context

No response
@X-Coder264 X-Coder264 added the Bug label Aug 4, 2023
@X-Coder264
Copy link
Contributor Author

cc-ing @tucksaun and @nicolas-grekas as I've seen that you've worked on the getSupportedTypes stuff so any input from you here would be greatly appreciated, thanks.

@andersmateusz
Copy link
Contributor

@X-Coder264
The problem is that serializer recognizes your variable foo as FooDummy[], because you have annotated that:

/**
* @var FooDummy[]
*/
public $foo;

So it makes strict comparision FooBaseDummy[] === FooDummy[], which evaluates to false.
Currently It is not able to look for subclasses when iterable is provided. Serializer should consider subclasses, because it was possible before getSupportedTypes was introduced.
I have encountered similar problem:

<?php

declare(strict_types=1);

namespace App\Serializer\Normalizer;

use App\Entity\Prop\CornerOfTheWorld;
use App\Serializer\Type\ImportableEnum;
use Symfony\Component\Serializer\Exception\NotNormalizableValueException;
use Symfony\Component\Serializer\Normalizer\DenormalizerInterface;
use Symfony\Component\Serializer\Normalizer\NormalizerInterface;

final readonly class ImportableEnumNormalizer implements NormalizerInterface, DenormalizerInterface
{
    public function getSupportedTypes(?string $format): array
    {
        if ('csv' === $format) {
            return [
                ImportableEnum::class => true,
                ImportableEnum::class . '[]' => true,
                CornerOfTheWorld::class . '[]' => true,
                'native-array' => true,
            ];
        } else {
            return [];
        }
    }

    /**
     * @param class-string<ImportableEnum> $type
     * @param int|string $data
     */
    public function denormalize(mixed $data, string $type, string $format = null, array $context = []): null|ImportableEnum|array
    {
        $className = \rtrim($type, '[]');
        if (!\is_subclass_of($className, ImportableEnum::class)) {
            throw new \InvalidArgumentException(\sprintf('Expected type "%s". "%s" given.', ImportableEnum::class . '|' . ImportableEnum::class . '[]', $type));
        }
        if ($isCollectionType = \strpos($type, '[]')) {
            $data = \explode(',', $data);
        }

        if (empty($data)) {
            return $isCollectionType ? [] : null;
        }

        $enumFrom = static function(string $value) use ($className): ImportableEnum {
            $value = \trim($value);
            try {
                $enumValue = \array_flip($className::getExcelMap())[$value] ?? throw new NotNormalizableValueException(\sprintf('"%s" is not valid backing value for "%s"', $value, $className));
                return $className::from($enumValue);
            } catch (\TypeError $e) {
                throw new NotNormalizableValueException(\sprintf('Failed converting %s "%s" to "%s"', \get_debug_type($value), $value, $className), previous: $e);
            }
        };

       
8000
 if (!$isCollectionType) {
            return $enumFrom($data);
        }

        return  \array_map($enumFrom, $data);
    }

    public function supportsDenormalization(mixed $data, string $type, string $format = null): bool
    {
        return \is_subclass_of($type, ImportableEnum::class) || \is_subclass_of(\rtrim($type, '[]'), ImportableEnum::class) && 'csv' === $format;
    }

    public function normalize(mixed $object, string $format = null, array $context = []): string
    {
        $isCollectionType = \is_array($object);
        if (!\is_object($object) && false === $isCollectionType) {
            throw new \InvalidArgumentException(\sprintf('Expected object or array of objects. "%s" given.', \get_debug_type($object)));
        }

        $checkType = static function($value) {
            if (!$value instanceof ImportableEnum) {
                throw new \InvalidArgumentException(\sprintf('Object must implement "%s".', ImportableEnum::class));
            }
        };

        $map = static function (ImportableEnum $value): string {
            return $value->getExcelMap()[$value->value];
        };

        $isCollectionType ? \array_walk($object, $checkType) : $checkType($object);

        return $isCollectionType ? \implode(',', \array_map($map, $object)) : $map($object);
    }

    public function supportsNormalization(mixed $data, string $format = null): bool
    {
        return ($data instanceof ImportableEnum || (\is_array($data) && \count(\array_filter($data, static fn ($el) => $el instanceof ImportableEnum)))) && 'csv' === $format;
    }

}

Apart from the fact that CornerOfTheWorld implements ImportableEnum I must have declared CornerOfTheWordl[] implicitly in getSupportedTypes method. Otherwise serializer skips my supportsDenormalization method. Also interesting though that when denormalizing data I must have added CornerOfTheWorld[] to supported types and when normalizing I must have added 'native-array'. I would expect that data can be transformed both ways using the same declared type.

@X-Coder264 X-Coder264 mentioned this issue Aug 12, 2023
Merged
@X-Coder264
Copy link
Contributor Author

@andersmateusz I've had a bit of time so I've sent a PR with the fix for denormalization -> #51369

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Serializer Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@X-Coder264 @carsonbot @andersmateusz
0