Skip to content

Symfony Serializer is inconsistent in deserialization #57546

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

Closed
idbentley opened this issue Jun 26, 2024 · 3 comments
Closed

Symfony Serializer is inconsistent in deserialization #57546

idbentley opened this issue Jun 26, 2024 · 3 comments

Comments

@idbentley
Copy link

idbentley commented Jun 26, 2024

Symfony version(s) affected

7.2 and earlier

Description

The Symfony Serializer is inconsistent in it's handling of types depending on if they are in an object, or not.

This ticket is a followup to: #57432

There are major differences in deserialization behaviour between values passed to deserialize directly versus when handling properties of Objects.

In 57432 we see that the type specification syntax is different when calling deserialize, versus properties on an object.

However, this inconsistency goes even further. In the reproduction seen hereafter, we see that union types are unsupported when passed to deserialize directly, versus when a property on an object contains a union.

Example 1 - demonstrating Union support:

class ObjectOne
{
    /**
     * $foo
     *
     * @var int
     */
    public int $foo;

    public function __construct(int $foo)
    {
        $this->foo = $foo;
    }
}

class ObjectTwo
{
    /**
     * $bar
     *
     * @var int
     */
    public int $bar;

    public function __construct(int $bar)
    {
        $this->bar = $bar;
    }
}

class UnionWrapper
{
    /**
     * $foo
     *
     * @var ObjectOne|ObjectTwo
     */
    public ObjectOne|ObjectTwo $obj;

    public function __construct(ObjectOne|ObjectTwo $obj)
    {
        $this->obj = $obj;
    }

}

$bodyTopLevel = '{
    "foo": 1
  }';

$bodyObject = '{
    "obj": {
        "foo": 1
    }
  }';

$phpDocExtractor = new PhpDocExtractor();
$propertyTypeExtractor = new PropertyInfoExtractor(
    typeExtractors: [$phpDocExtractor],
);
$classMetadataFactory = new ClassMetadataFactory(new AttributeLoader());
$encoders = [new JsonEncoder()];
$normalizers = [
    new ArrayDenormalizer(),
    new ObjectNormalizer(propertyTypeExtractor: $propertyTypeExtractor, classMetadataFactory: $classMetadataFactory),
];

$serializer = new Serializer($normalizers, $encoders);

$responseObject = $serializer->deserialize($bodyObject, 'UnionWrapper', 'json', [AbstractNormalizer::REQUIRE_ALL_PROPERTIES => true]);
var_dump($responseObject);
/*
object(UnionWrapper)#163 (1) {
  ["obj"]=>
  object(ObjectOne)#194 (1) {
    ["foo"]=>
    int(1)
  }
}
*/

$responseTopLevel = $serializer->deserialize($bodyTopLevel, 'ObjectOne|ObjectTwo', 'json', [AbstractNormalizer::REQUIRE_ALL_PROPERTIES => true]);
// Uncaught Symfony\Component\Serializer\Exception\NotNormalizableValueException: Could not denormalize object of type "ObjectOne|ObjectTwo", no supporting normalizer found

A user of the API can support Union's passed directly to the deserializer by providing their own UnionDenormalizer

final class UnionDenormalizer implements DenormalizerAwareInterface, DenormalizerInterface
{
    use DenormalizerAwareTrait;

    public function setDenormalizer(DenormalizerInterface $denormalizer): void
    {
        $this->denormalizer = $denormalizer;
    }

    public function getSupportedTypes(?string $format): array
    {
        return ['*' => true];
    }

    public function supportsDenormalization(mixed $data, string $type, ?string $format = null, array $context = []): bool
    {
        if ($this->denormalizer === null) {
            throw new \BadMethodCallException(sprintf('The nested denormalizer needs to be set to allow "%s()" to be used.', __METHOD__));
        }
        if (str_contains($type, '|')) {
            $possibleTypes = explode('|', $type);
            $support = true;

            // all possible types must be supported
            foreach ($possibleTypes as $possibleType) {
                $typeSupport = $this->denormalizer->supportsDenormalization($data, $possibleType, $format, $context);
                $support = $support && $typeSupport;
            }
            return $support;
        }

        return false;
    }

    /** @phpstan-ignore-next-line */
    public function denormalize(mixed $data, string $type, ?string $format = null, array $context = []): mixed
    {
    // ...snip...
    }
}

But this just brings up another deserialization inconsistency, because any Unions that are properties of objects will use the implementation that's buried in the AbstractObjectNormalizer, while Unions passed directly to deserialize may use the UnionDenormalizer class. Similarly (this is related to 57432, but not captured in that ticket) array handling when calling the deserialize function will be handled by the ArrayDenormalizer (if it's provided to the serializer), while arrays that are embedded in an object, will use the implementation in AbstractObjectNormalizer - those implementations may be identical, but it's certainly not clear.

How to reproduce

See above

Possible Solution

My proposed solution here is to provide an alternative operation mode for the ObjectNormalizer that bypasses the implementations in AbstractObjectNormalizer for special types, and instead uses ObjectNormalizer as a recursive DenormalizerAwareInterface. Each property on the object is denormalized by invoking the appropriate denormalizer as passed to the Serializer constructor. I've prepared a Pull Request that shows a proof of concept of this implementation. I would like to get feedback from your team before I invest more time in pursuing this approach.

Additional Context

No response

@carsonbot
Copy link

Hey, thanks for your report!
There has not been a lot of activity here for a while. Is this bug still relevant? Have you managed to find a workaround?

@carsonbot
Copy link

Just a quick reminder to make a comment on this. If I don't hear anything I'll close this.

@carsonbot
Copy link

Hey,

I didn't hear anything so I'm going to close it. Feel free to comment if this is still relevant, I can always reopen!

@derrabus derrabus closed this as not planned Won't fix, can't repro, duplicate, stale Jan 24, 2025
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

4 participants