dunglas / doctrine-json-odm

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

Serialization issues w/ nested DTOs & Symfony 7(.1) #139

Open j-schumann opened 1 month ago

j-schumann commented 1 month ago

Hello @dunglas,

we have been using doctrine-json-odm for a while now, without issues. (Project with ApiPlatform 3.3.11, doctrine-json-odm version: 1.4.1)

Our setup is something like this:

class Foo
{
    /**
     * @var BarInterface[]|array
     */
    #[ORM\Column(type: 'json_document', options: ['jsonb' => true, 'default' => '[]'])]
    public array $list = [];
}

interface BarInterface
{
}

class Bar implements BarInterface
{
    /**
     * @var BazInterface[]|array
     */
    public array $nested = [];
}

interface BazInterface
{
}

class Baz implements BazInterface
{
    public string $anyProp = '';
}

Using Symfony 7.0 (symfony/serializer version 7.0.10) this works: a) Persisting to the database always works b) Loading from the database requires "|array" to be in the phpdoc, else the Symfony serializer eventually receives a [{Baz object}] and will not set it on the $nested property: Even though he correctly detects that BazInterface[] is a collectionType (https://github.com/symfony/serializer/blob/7.0/Normalizer/AbstractObjectNormalizer.php#L494) he will then fail with: The type of the "nested" attribute for class "Bar" must be one of "BazInterface[]" ("array" given). c) It will also work without the "|array", but only if we use @var Baz[] (the concrete class, not the interface) d) We cannot simply skip the phpdoc as then the serializer would not know how to deserialize the array he receives from API requests into object, he would simply leave them as plain arrays

But switching to Symfony 7.1. (serializer 7.1.3 + the new symfony/type-info 7.1.1) this no longer works: a) again, removing "|array" from our previously working phpdoc @var BarInterface[]|array would cause the deserialization from the DB to fail (see above) b) but keeping "|array" now results in the Symfony serializer to not denormalize the nested data into objects, he simply sets the plain arrays, which causes errors later on

I could not find out, what exactly is the reason, that the json_document denormalization requires the "|array" addition, as without it it would theoretically work in 7.0 & 7.1. So I opened the issue here instead of the symfony/serializer repository. I also was not yet able to create a simple reproducer, still working on that. But maybe you already have an idea what could be the issue? What puzzles me, is that @var Baz[] works without the "array", but @var BazInterface[] doesn't, so maybe it's a Serializer issue after all.

Thanks in advance, JS

Aweptimum commented 2 weeks ago

Does BarInterface have a DiscriminatorMap? If not, it should Symfony Docs: Serializing Interfaces and Abstract Classes

If you don't have a discriminator map, I would hazard the reason you're seeing it work at all with |array is that you're telling the serializer "you will either get back a collection of an interface or an array of anything".

If your interface doesn't have a discriminator, then it will fall back to array which is really any[]. In that case, the serializer does not care at all about what it needs to deserialize into. This should (I think) always result in collection where each entry is an associative array that has a shape that matches your object (or each entry an instance of stdClass if json_decode_associative is set to False). Which would match the error you're getting.

I think the Serializer should throw a better exception when deserializing into an interface that doesn't have a DiscriminatorMap

I'm also surprised you were seeing any success at all on Symfony 7.0 🤔

j-schumann commented 1 week ago

Thanks Sam, sorry for the late response, hectic days before vacation...

No, BarInterface does not have a DiscriminatorMap. I guess, you are wondering how deserialization of the API JSON Input works then: We have custom Denormalizers for each DTO interface, that map an "@type" field given by the API client to the correct DTO class and does some other stuff. I wonder if a DiscriminatorMap would even be compatible with json-odm, haven't tried it yet, does json-odm then still auto-determines the type value to use in the database JSON? Without having to specify those discrimnators and letting json-odm do it's thing is quite appealing, in the denormalizer we only have to specify the currently supported values and they do not have to match those used in the database.

But as I said in my first post, I don't think that the deserialization of the API input is the problem, the error happens when deserializing the values from the database. Especially "annoying" is that the serializer in 7.0 and 7.1 actually recieves the correct list of nested objects: [{Baz object}] but fails so set it on the parent object if |array is missing in the phpdoc. I also want to stress that this only happens for nested DTOs, properties on the entity class can have DtoInterface[] in the phpdoc, while other DTOs within the DtoInterface require OtherDtoInterface[]|array.

Aweptimum commented 4 days ago

DiscriminatorMap's are compatible. json-odm consists of a doctrine type and a serializer that just extends from the Symfony/Serializer and patches it. It still has all the features of the base serializer.

It worked really well for storing geometry data in my previous project. I had a base abstract class -> a few abstract subtypes -> several concrete implementations per subtype. I only needed one DiscriminatorMap on each abstract parent and it worked flawlessly. It did result in a little data duplication (type field for the map and a #type field for json-odm) and it was a bit of a pain to have to remember to add a new entry in a Map when adding a new concrete class, but I think I preferred it to having to manage 11 denormalizers.

Now I could see how a custom denormalizer would sort-of-work.

IIRC, when the serializer sees an interface as a type-hint on a property, it assumes there's a DiscriminatorMap and so goes through a different code path (although I swear it would throw an error in 6.x instead of returning an array). So if you have BarInterface[]|array, the BarInterface[] type is never going to succeed because there is no DiscriminatorMap. The latter array hint probably succeeds in 7.0 because it's only looking at how to denormalize an associative array, which your custom denormalizer explicitly supports. I don't have a theory for why it breaks in 7.1 besides possibly a built-in normalizer's priority changed.

As for why it would work on non-nested properties, I noticed a similar inconsistency when troubleshooting #127, but I could not comprehend what I was seeing. The conditional I pointed out in that issue is necessary, but for iterable data it appeared to result in a lot of hijinx.

TL;DR my advice is to use DiscriminatorMap's - it's the Serializer's intended solution to your problem.