Embedded documents with targetDocument and discriminatorMap property do not save discriminatorField

[Steveb-p]

Q	A
Version	2.1.0

Support Question

tl;dr When using targetDocument and discriminatorMap annotation property for EmbedMany (I believe EmbedOne as well) embedded documents do not save discriminator property.

I believe it should be mentioned in the docs that those two should not be mixed.
If possible I would either allow targetDocumentto coexist with discriminatorMap as a "sanity check" when document passed is expected to be a specific class / interface.
Otherwise, I would throw an exception if they are used together.

Context: I was extending an existing document to include a new type of embedded document with new properties. I had a document looking like this:

use Doctrine\ODM\MongoDB\Mapping\Annotations as ODM;

/**
 * @ODM\Document
 */
class Order 
{
    /**
     * @ODM\EmbedMany(targetDocument=LegacyClass::class)
     */
    private $embedded;
}

then I wanted to introduce another possible target document:

use Doctrine\ODM\MongoDB\Mapping\Annotations as ODM;

/**
 * @ODM\Document
 */
class Order 
{
    /**
     * @ODM\EmbedMany(
     *     targetDocument=AbstractClass::class),
     *     strategy="atomicSetArray",
     *     discriminatorField="type",
     *     discriminatorMap={
     *         "legacy"=LegacyClass::class,
     *         "statistic_aggregate"=NewClass::class,
     *     },
     *     defaultDiscriminatorValue="legacy",
     */
    private $embedded;
}

However, this resulted in type property to never be set.

While documents inserted into this embedded collection were NewClass instances and have it's property, when trying to read ODM was not finding type property and tried to instantiate LegacyClass.

Therefore I think that docs should explicitly warn about mixing those two annotations (targetDocument & discriminatorMap).

EDIT: A test that shown this misbehaving in my application:

use Symfony\Bundle\FrameworkBundle\Test\KernelTestCase;

class OrderTest extends KernelTestCase
{
    public function testEmbeddedDocumentsWorkCorrectly(): void
    {
        self::bootKernel();
        $dm = self::$container->get('doctrine_mongodb.odm.default_document_manager');

        $order = new Order();
        $order->addClass(new NewClass());
        $order->addClass(new LegacyClass());
        $dm->persist($order);
        $dm->flush();
        $id = $order->getId();
        $dm->clear();

        /** @var Order $order */
        $order = $dm->find(Order::class, $id);
        [ $reference, $legacyReference ] = $order->getClasses()->toArray();
        $this->assertInstanceOf(NewClass::class, $reference);
        $this->assertInstanceOf(LegacyClass::class, $legacyReference);
    }
}

[malarzm]

If possible I would either allow targetDocumentto coexist with discriminatorMap as a "sanity check" when document passed is expected to be a specific class / interface.

IIRC we've made discriminator map strict if it's present (i.e. ODM will throw an exception if class is not listed in the map), so a sanity check is already there. My vote goes for an exception given it's not working anyway, a note in the docs will be also a nice to have.

[Steveb-p]

@malarzm so basically an additional check should be added around here:

https://github.com/doctrine/mongodb-odm/blob/011c4652542fc82b095b5d620a5b225e6aad799a/lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadata.php#L1925-L1936

Simply looking if targetDocument and discriminatorMap are set? And then throwing a new exception class?

If yes then I think I should be able to handle it :laughing:

[malarzm]

Exactly :)

[stale[bot]]

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in a week if no further activity occurs. Thank you for your contributions.

[alcaeus]

I'll take another look at this - this should definitely work as you intend it to work.

[Steveb-p]

@alcaeus initial PR for it that I've made seems to be a little to aggresive (isset checks are to broad and cause exception to be thrown even then it shouldn't). Feel free to take it over and use those commits (or discard them and only use code).

[Steveb-p]

@alcaeus what's the prefered approach for this? From your comment I assume - maybe wrong - that those two annotation properties should be able to simultanously exist?