When Copying Many Embed Subdocuments From One Document To Another the source Documents loses the subdocuments

    public function testWhenCopyingManyEmbedSubDocumentsFromOneDocumentToAnotherWillNotAffectTheSourceDocument()
    {

        $test1 = new ChangeEmbeddedIdTest();

        $embedded = new EmbeddedDocumentWithId();
        $embedded->id = (string) new \MongoId();;
        $test1->embedMany = array($embedded);

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

        $test2 = new ChangeEmbeddedIdTest();
        $test2->embedMany = $test1->embedMany; //using clone will work
        $this->dm->persist($test2);
        $this->dm->flush();

        //do some operations on test1
        $this->dm->persist($test1);
        $this->dm->flush();

        $this->dm->clear(); //get clean results from mongo
        $test1 = $this->dm->find(get_class($test1), $test1->id);

        $this->assertEquals(1, count($test1->embedMany));
    }

This bug was not present on 1.0.0-BETA11 but appears after 1.0.0.

[alcaeus]

Without having gone in and bisecting it I would suppose this could have been introduced by #782, it also is related to #648.
I'll have to dive in and check what we can do, but it seems that we should throw an exception when trying to re-use an embedded document that belongs to a different owner.

Should I consider cloning a safe solution even for complex subdocuments?

[alcaeus]

The cookbook has a section on safely implementing clone. You also need to make sure to deep-clone any other embedded documents or collections.

For the moment I choose the safer route to just create new objects and copy each property. Should be 100% safe.

[malarzm]

that we should throw an exception when trying to re-use an embedded document that belongs to a different owner.

Sounds good :)

Also to throw my 2 cents in "clone all the things" is my personal approach when embedded document needs to be copied

[malarzm]

that we should throw an exception when trying to re-use an embedded document that belongs to a different owner.

Actually copied PersitentCollection is fixed during persisting, maybe instead of throwing exception we should investigate cloning (or copying somehow) embedded document that is detected to belong to another document?

[JardaHorvat]

Hello, I have tried to solve this issue by calling DocumentManager::detach( $embedded ) which removed the emb. documents assiment to its parent and managed state, thus removing it correctly from existing collection, but it caused another issue with discriminator field not being saved with the emb. document. Can anybody confirm this?

[malarzm]

@JardaHorvat I think the best will be failing test case :)

[JardaHorvat]

Your test case is ready : ). Please download the archive from url bellow, and follow my installation instructions. Start by checking www/index.php or running it. (walkthru inside)

http://s000.tinyupload.com/?file_id=96707162348476446526
(reuploaded with fewer typos)

[malarzm]

@JardaHorvat thanks, I'll check it prob tomorrow as I just got back to office after vacations and have load of things to take care of :)

[alcaeus]

@JardaHorvat Since I couldn't get your test case to run I extracted what you described to a doctrine test case:

• Create two parent objects
• Add a bunch of children to the first parent as embedded docs without targetDocument (thus using the _class discriminatorField)
• Move all children from parent 1 to parent 2

Here's the doctrine test case I created: https://gist.github.com/alcaeus/f1f1ea4b6b69030b0ab8
The test works without using detach, which I expected: as long as an embedded document is not copied to another parent, only moved there there are no problems whatsoever. (Retracted, see update below)

I'll go back to your test case and see if I can reproduce the issue later today.

[JardaHorvat]

Hello @alcaeus, I used some third party tools in my test case and if they caused you trouble, I apologize. Therefor I moved the bare test case code into my repository and downgraded it to run on php 5.3, you can find it here https://github.com/JardaHorvat/ODM1229Issue

As to your test, you understand the basic idea but there are still differences. Yes there are two parents ( of class Folder ) and multiple children types in one collection ( classes GenericFile and ImageFile ), using field _class as discriminator. First difference i see, is that you are missing file ordering. When i add file to its parent, the file receives an order value. Same when removing file, remaining file orders are corrected to follow basic range of zero to the file count. Next, I move only single file ( with order equal 1 ), not whole collection at once. Last thing might not be an issue, but you never know : ), I use concrete class of GenericFile and ImageFile extending it, not same ancestor for both.

Please try to get my case running now, and follow my steps.

First test:

• TestCase::createTestDataA()
• TestCase::methodA()

Ends throwing MongoWriteConcernException ( The field 'files' must be an array but is of type Object in document {_id: "b")

Second test:

• TestCase::createTestDataB()
• TestCase::methodA()
• TestCase::getTestData()
• TestCase::methodA()
• TestCase::getTestData()

When calling Folder::getFiles() throws MappingException ( Class '' does not exist )

Third test:

• TestCase::createTestDataB()
• TestCase::methodB()
• TestCase::getTestData()

When calling Folder::getFiles() throws MappingException ( Class '' does not exist )

[alcaeus]

After taking another look at what happens when editing and moving documents at the same time, I retract my statement from above:

as long as an embedded document is not copied to another parent, only moved there there are no problems whatsoever.

This statement is only true in a few amount of cases, one of which is "as long as you take care of all your association changes in one go, flush only when you're done changing stuff and then clear the document manager". That being said, it is just not safe to copy embedded documents. I can only repeat what @malarzm has said multiple times: deep-clone all embedded documents whenever you're moving or copying them from one parent to another.

In this case, when removing an embedded document from a PersistentCollection, UnitOfWork will mark the document in question as "new". This happens after all changes have been written to the database, so these problems arise on the next call to flush() unless you clear() the DocumentManager first (which is why it didn't arise) in my earlier testcase. Nevertheless, all of this can be avoided by simply using clone. Instead of doing this:

$firstParent->removeChild($child);
$secondParent->addChild($child);

You should add a clone when re-adding an embedded document that belongs to a different parent:

$firstParent->removeChild($child);
$secondParent->addChild(clone $child);

I've adjusted the testcase in the gist: https://gist.github.com/alcaeus/f1f1ea4b6b69030b0ab8 to do pretty much what you're doing (except that I'm using generic terms and I've prepared this as a PHPUnit testcase in case we want to add it to our testsuite).

Looking at your code, there's another issue I want to point out: When you clear the document manager, no further changes are persisted. You can try this by taking my testcase and moving the call to clear() within the loop. You'll notice that the second child we're removing from the parent is never actually removed in the database. This is because after calling clear your parent is no longer managed and thus won't be persisted to the database.