dunglas / doctrine-json-odm

An object document mapper for Doctrine ORM using JSON types of modern RDBMS.
https://dunglas.fr
MIT License
597 stars 67 forks source link

Redundant denormalize call in SerializerTrait #127

Open Aweptimum opened 1 year ago

Aweptimum commented 1 year ago

I've been trying to debug the deserialization from a database of a vector object. While stepping through a unit test, I noticed that the property of another entity typed as a json_document column in the test was being treated as an array of objects. The requested type inside the Serializer is of App\MyDTO[], but the #type key is set to App\MyDTO in the database. Still, it is somehow being deserialized properly as a single object, while each element in my vector object's array is being treated as a vector object and failing.

I tracked down the appended [] to line 82 in the SerializerTrait

https://github.com/dunglas/doctrine-json-odm/blob/89b01483c4f6e7a5e25fc2ea18b2fe46574617ae/src/SerializerTrait.php#L70C5-L94C6

(not sure why the embedded snippet won't render)

The extra call to $this->denormalize on 82 supplies $data back to itself, but the #type key has been unset. So in that recursive denormalize call, it skips down to the is_iterable check, which evaluates to true because $data is an array. It then appends [] to the $type and delegates to the Symfony Serializer. It, in turn, delegates to the ArrayDenormalizer because [] has been appended.

I'm not sure why the data at times comes back fine as a single object and others do not.

I think removing line 82 altogether would solve the issue - it seems like parent::denormalize would be all that is needed to correctly return the denormalized data. As a test, I commented out 82 in my project, and the deserialization process actually works now for my vector type while also making less calls to other normalizers.

Aweptimum commented 1 year ago

I forked it, made the change, and ran the test suite. It seems removing it breaks the ::nestedObjects tests as well as enums:


There were 3 failures:

1) Dunglas\DoctrineJsonOdm\Tests\FunctionalTest::testNestedObjects
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
         'key' => 'parent'
         'value' => Array (
             0 => Array (
-                0 => Dunglas\DoctrineJsonOdm\Tests\Fixtures\TestBundle\Document\Attribute Object (...)
+                0 => Array (...)
             )
         )
     )
 )

doctrine-json-odm-redundant\tests\FunctionalTest.php:117

2) Dunglas\DoctrineJsonOdm\Tests\FunctionalTest::testStoreAndRetrieveEnum
Failed asserting that Array &0 (
    '#type' => 'Dunglas\DoctrineJsonOdm\Tests\Fixtures\TestBundle\Enum\InputMode'
    '#scalar' => 'email'
) is identical to an object of class "Dunglas\DoctrineJsonOdm\Tests\Fixtures\TestBundle\Enum\InputMode".

doctrine-json-odm-redundant\tests\FunctionalTest.php:219

3) Dunglas\DoctrineJsonOdm\Tests\SerializerTest::testNestedObjects
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
         'key' => 'parent'
         'value' => Array (
             0 => Array (
-                0 => Dunglas\DoctrineJsonOdm\Tests\Fixtures\TestBundle\Document\Attribute Object (...)
+                0 => Array (...)
             )
         )
     )
 )
doctrine-json-odm-redundant\tests\SerializerTest.php:88
Aweptimum commented 1 year ago

I had a moment of realization - the class I'm trying to serialize implements the \Iterator interface and I think this makes it pass the is_iterable check in SerializerTrait->denormalize when it shouldn't. I added a test case in #128 that demonstrates the issue

Arkemlar commented 3 months ago

Well, this thing brokes my wishes to use that library, so sad.

Aweptimum commented 3 months ago

@Arkemlar you can create a custom normalizer for your iterable type In my Symfony 6.2 project it looked roughly like this:

class VectorNormalizer implements NormalizerInterface
{
    function supportsNormalization($object)
    {
        return $object instance of Vector;
    }

    function normalize($object)
    {
        return $object->getArray();
    }
}

You don't need a custom denormalizer so long as normalize returns data in the format the constructor expects (an array in my case)