Doctrine 2 - ORM
  1. Doctrine 2 - ORM
  2. DDC-758

When merging many to many entites back into the repository changes to the associations are not respected

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.0-RC1
    • Component/s: ORM
    • Security Level: All
    • Labels:
      None

      Description

      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.

      1. DDC758Test.php
        6 kB
        Dave Keen
      2. 0147-DDC-758.patch
        2 kB
        Dave Keen

        Activity

        Hide
        Dave Keen added a comment -

        Here is a failing test case for this issue.

        Show
        Dave Keen added a comment - Here is a failing test case for this issue.
        Hide
        Dave Keen added a comment -

        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.

        Show
        Dave Keen added a comment - 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.
        Hide
        Dave Keen added a comment -

        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.

        Show
        Dave Keen added a comment - 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.
        Hide
        Benjamin Eberlei added a comment -

        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.

        Show
        Benjamin Eberlei added a comment - 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.
        Hide
        Dave Keen added a comment -

        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?

        Show
        Dave Keen added a comment - 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?
        Hide
        Dave Keen added a comment -

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

        Show
        Dave Keen added a comment - This is an attempt to patch the issue. After each cascade merge this fills in $snapshot from the database.
        Hide
        Dave Keen added a comment -

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

        Show
        Dave Keen added a comment - Slightly changed the patch - this uses setDirty() instead of changed() - without this a Flextrine test fails.
        Hide
        Benjamin Eberlei added a comment -

        Fixed, can you verify Dave?

        Show
        Benjamin Eberlei added a comment - Fixed, can you verify Dave?

          People

          • Assignee:
            Benjamin Eberlei
            Reporter:
            Dave Keen
          • Votes:
            1 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: