Mongodb-odm: Embedded document isn't added in update

[johanliefers]

Sometimes an embedded document is not added to the update data in the PersistenceBuilder.

This happens when I have 2 documents and add the same embedded document to both documents. Then I perform a flush (in a loop so the documents are flushed seperately).
The first time the unit of work sees the embedded document as new and $this->uow->isScheduledForInsert($new) returns true and the embedded document is added to the $updateData.

The second time I perform a flush the unit of work sees the document as already added, so $this->uow->isScheduledForInsert($new) returns false. Then $this->prepareUpdateData($new); will be called, which will return nothing because the documentchangeset for the embedded document is empty.

My temporary workaround is to clear the documentmanager after each flush, but thats not really a desirable solution.

• the code I mentioned is in the PersistenceBuilder from line 161 to 178 (function prepareUpdateData)

[Steveb-p]

...add the same embedded document...

Embedded documents should not share the same instance. Once they get into mongo they become two separate entities and ODM follows this. Clone the instance if you need to, but I doubt it will be changed since from what I know it's working as intended.

[alcaeus]

As @Steveb-p has mentioned, using the same embedded document in different documents will not work. However, there is code in UnitOfWork to prevent that and to clone the document automagically so developers don't have to do that themselves.

@johanliefers do you think you could provide a failing test case? That would speed up looking into this issue. Thanks!

[malarzm]

@johanliefers

Then I perform a flush (in a loop so the documents are flushed seperately).

Flushing only certain things has funny side effects one may not expect, for instance all inserts will happen even if document is not listed.

[johanliefers]

I think this issue happened since I updated from 1.0.5 to 1.1.1. Before the update everything worked fine.
Just as @alcaeus mentioned something in the UnitOfWork might be working differently and embedded documents are not cloned automatically anymore.

I will try to post a testcase this friday, I don't have time for it right now.

[johanliefers]

@alcaeus I created a unit test for it:

<?php
use Doctrine\ODM\MongoDB\Mapping\Annotations as ODM;

class EmbeddedTest extends BaseTest
{
    public function testEmbedClone()
    {
        $embedded = new Embedded();
        $embedded->name = 'embedded';

        for ($i = 0; $i < 2; ++$i) {
            $parent = new Document();
            $parent->name = 'test' . $i;
            $parent->embedded = $embedded;

            $this->dm->flush();
        }

        $this->dm->clear();

        for ($i = 0; $i < 2; ++$i) {
            $test = $this->dm->getRepository(Document::class)->findOneBy(array('name' => 'test' . $i));

            $this->assertInstanceOf(Document::class, $test);
            $this->assertInstanceOf(Embedded::class, $test->embedded);
            $this->assertSame($test->embedded->name, $embedded->name);
        }
    }
}

/** @ODM\Document(collection="document_test") */
class Document
{
    /** @ODM\Id */
    public $id;
    /** @ODM\Field(type="string") */
    public $name;
    /** @ODM\EmbedOne(targetDocument="Embedded") */
    public $embedded;
}


/** @ODM\EmbeddedDocument */
class Embedded
{
    /** @ODM\Field(type="string") */
    public $name;
}

this will fail the second time it passes $this->assertInstanceOf(Embedded::class, $test>embedded); as $test->embedded is then null

[alcaeus]

@johanliefers You're missing a $this->dm->persist($parent); just before the flush. Adding that fixes the test for me.

[johanliefers]

@alcaeus you're right, the code I had earlier was:

public function testEmbedClone()
    {
        $embedded = new Embedded();
        $embedded->name = 'embedded';

        for ($i = 0; $i < 2; ++$i) {
            $parent = new Document();
            $parent->name = 'test' . $i;

            $this->dm->persist($parent);
            $this->dm->flush();

            $parent->embedded = $embedded;

            $this->dm->flush();
        }

        $this->dm->clear();

        for ($i = 0; $i < 2; ++$i) {
            $test = $this->dm->getRepository(Document::class)->findOneBy(array('name' => 'test' . $i));

            $this->assertInstanceOf(Document::class, $test);
            $this->assertInstanceOf(Embedded::class, $test->embedded);
            $this->assertSame($test->embedded->name, $embedded->name);
        }
    }

And with this code the test does fail the second time. I thought it would also happen if I remove the first flush, but I accidentally also removed the persist with it. So you're right that it does work with one flush, but with 2 flushes like in my test it fails.

[alcaeus]

@johanliefers This works on my machine: https://github.com/alcaeus/mongodb-odm/blob/gh-1525-test/tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH1525Test.php

[johanliefers]

@alcaeus I know that also works on my machine, however if you add a flush before you add the embedded document and then flush again after the embedded document is added the second time will fail.

[alcaeus]

Ah, ok, then I just misunderstood you. I'll test it later.

[johanliefers]

@alcaeus When do you think you have time to test it?

[alcaeus]

I apologize for the delay. I was debugging it last night, but couldn't figure out why it goes wrong. The embedded object is recognized as being owned by a different parent document and is properly cloned. The DocumentPersister however still tries to write the other embedded document to the database. I'll have to set some more time aside to debug this issue.

In the meantime I can only suggest what @malarzm and I have been saying for a while now: don't reuse embedded document instances, always clone them.

[johanliefers]

@alcaeus no problem, thanks for checking, I'll clone the embedded documents before adding, that works fine indeed.

[alcaeus]

@johanliefers Again, sorry for the delay. I finally managed to figure out the cause for the issue and created #1542 to fix it. Thanks for the report!

[malarzm]

Forgot to post here that #1542 was merged few days ago, @johanliefers could you try out latest 1.1.x branch and let us know if something is still off? Closing this one for now but please do post any findings here (we'll reopen issue then).

[johanliefers]

@malarzm I already fixed it by cloning the embedded document manually. Unfortunantly I can't just update because our client will then have to retest the enitre application. If the unit test now passes then it should work fine. @alcaeus Thanks for fixing!

[malarzm]

@johanliefers understood, thanks!

[redthor]

Hi there, I'm having this issue as well. I didn't realise we needed to clone() shared embedded objects.

I have tried the code in the PR and I still get the issue. What I find is that just above @alcaeus changes there is this check:

https://github.com/doctrine/mongodb-odm/commit/62b8ab436e2845d7c0c20e21421843fd0053b01f#diff-ae2c9f4b58c612dce08a0f60ff683aecR731

if ($orgValue === $actualValue) {

which equates to true. I do not know how $orgValue is not null because it is not in the database at this stage. I would need to work out why the embedded doc shows up in $this->originalDocumentData but basically it doesn't reach the code that clone()s $actualValue for us.

Doing the clone() ourselves appears to fix the issue.

[alcaeus]

Could you provide a failing test case? I'm not sure why it's not working for you.

I do not know how $orgValue is not null [...]

I'd have to see a specific test case to check out what's going on. Since you're dealing with at least 3 documents here (two parents and one shared embedded document), computeOrRecomputeChangeSet is called at least three times, more often depending on how you flush your changes. It's hard to know which call this is originating from. If you could provide a failing test case, I'd be more than happy to debug this.

[redthor]

It was a bit hard to try and pinpoint which of the differences between the test case and our actual configuration is causing the issue. However I was able to get an error by using ID strategy "NONE" on the parent, which is what we use for all our models.

This gist contains the test I wrote. The first test method passes, the second one fails in my env.

https://gist.github.com/redthor/ce6ef962439775afb9afb3897ffa4a61

1) Doctrine\ODM\MongoDB\Tests\Functional\Ticket\GH1525Test::testEmbedCloneWithIdStrategyNoneOnParent
Failed asserting that null is an instance of class "Doctrine\ODM\MongoDB\Tests\Functional\Ticket\GH1525Embedded".

/home/d/other/doc-mongodb-odm/tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH1525Test.php:54

[alcaeus]

Thank you, I'll take a look at it tonight!

[malarzm]

1550 was merged and confirmed to work, let's try to close this issue one more time :)