Skip to content

Loading…

DDC-758: When merging many to many entites back into the repository changes to the associations are not respected #5271

Closed
doctrinebot opened this Issue · 10 comments

2 participants

@doctrinebot

Jira issue originally created by user ccapndave:

When merging a DETACHED entity into the repository with ManyToMany associations that have changed since the last flush, the join table is not updated with the new associations.

Many Movies have many Artists with cascade merge:

class Movie {

    /*** @Id @Column(type="integer") @GeneratedValue(strategy="IDENTITY") **/
    public $id;

    /*** @Column(length=50, type="string") **/
    public $title;

    /**** 
     * @ManyToMany(targetEntity="Artist", cascade={"merge"})
     */
    public $artists;

    public function **construct() {
        $this->artists = new ArrayCollection();
    }   

}
<?php
class Artist {

    /*** @Id @Column(type="integer") @GeneratedValue(strategy="IDENTITY") **/
    public $id;

    /*** @Column(length=50, type="string") **/
    public $name;

    /*** @ManyToMany(targetEntity="Movie", mappedBy="artists", cascade={"merge"}) **/
    public $movies;

    public function **construct() {
        $this->movies = new ArrayCollection();
    }

}

Assume that the database contains:
Movie: id=1, title="Movie 1"
Artist: id=1, name="Artist 1"
Artist: id=2, name="Artist 2"

and that there is a entry (1, 1) in movie_artist so that there is a many-many relationship between Movie 1 and Artist 1.

I now want to merge a detched version of a movie into the repository, with an extra relationship between Movie 1 and Artist 2:

$m1 = new \vo\Movie();
$m1->id = 1;
$m1->title = "Movie 1";

$a1 = new \vo\Artist();
$a1->id = 1;
$a1->name = "Artist 1";

$a2 = new \vo\Artist();
$a2->id = 2;
$a2->name = "Artist 2";

$m1->artists->add($a1); $a1->movies->add($m1);

// Add the new association
$m1->artists->add($a2); $a2->movies->add($m1);

// This is a cascade merge so it will merge the entities down the tree
$m1 = $em->merge($m1);

At this point $m1 should contains merged entities reflecting the same as what is in the database, plus the extra association between Movie 1 and Artist 2, so after a flush() movie_artist should contain (1,1),(1,2)

I now run:

$em->flush();

but movie_artist remains unchanged (still just (1,1)) and $em->getUnitOfWork()->computeChangeSets() is empty before the flush().

The same behaviour is exhibited when merging $m1 after deleting entities from its many-to-many associations; movie_artist does not change.

@doctrinebot

Comment created by ccapndave:

Here is a failing test case for this issue.

@doctrinebot

Comment created by ccapndave:

The problem area seems to be in the first lines of UnitOfWork::computeAssociationChanges:

if ($value instanceof PersistentCollection && $value->isDirty()) {
            if ($assoc['isOwningSide']) {
                $this->collectionUpdates[] = $value;
            }
            $this->visitedCollections[] = $value;
        }

If I comment out the check for $value->isDirty() then the test case passes so it looks like this is failing because the ManyToMany PersistentCollection isn't getting marked as dirty during computeChangeSets. I've actually fiddled around and found various ways to fix it, but computeChangeSets is pretty complicated and I'm worried that any changes I make might affect performance without me realizing. However, if you can give me a few pointers I'm happy to have another go at patching the issue.

@doctrinebot

Comment created by ccapndave:

As it turns out commenting out $value->isDirty() only half fixes the problem; it fixes it in the case of adding* many to many associations (as in the example about), but it doesn't fix the case of *removing many to many associations.

I have updated the attached test to include a test case which creates some many to many associations, then merges in equivalent detached entities but which have empty association attributes. On a cascade merge and flush I would expect this to empty out the associations table; in fact it has no effect.

@doctrinebot

Comment created by @beberlei:

removing $value->isDirty() is no option as it is really necessary at that position. The question is, why does merge not mark the collection as dirty.

@doctrinebot

Comment created by ccapndave:

Agreed. I am hard at work on this right now, and have discovered something that may be another clue. It actually seems the problem is twofold. As well as the collection not getting marked dirty, the reason that the ManyToManyPersister is not removing rows is that PersistentCollection::getDeleteDiff() isn't returning any differences, and the reason for that is that $snapshot isn't get set to anything during the merge process. When merging a collection I think we need to set $snapshot to whatever is currently in the database, and set $coll to whatever is in the collection being merged. I'm not quite sure, but perhaps this could also be the reason why the collection isn't getting set as dirty?

@doctrinebot

Comment created by ccapndave:

This is an attempt to patch the issue. After each cascade merge this fills in $snapshot from the database.

@doctrinebot

Comment created by ccapndave:

Slightly changed the patch - this uses setDirty() instead of changed() - without this a Flextrine test fails.

@doctrinebot

Comment created by @beberlei:

Fixed, can you verify Dave?

@doctrinebot

Issue was closed with resolution "Fixed"

@beberlei beberlei was assigned by doctrinebot
@doctrinebot doctrinebot added this to the 2.0-RC1 milestone
@doctrinebot doctrinebot closed this
@doctrinebot doctrinebot added the Bug label
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.