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. 0147-DDC-758.patch
        2 kB
        Dave Keen
      2. DDC758Test.php
        6 kB
        Dave Keen

        Activity

        Dave Keen created issue -
        Dave Keen made changes -
        Field Original Value New Value
        Description When merging a DETACHED entity into the repository with a 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:

        {code}
        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();
        }

        }
        {code}

        {code}
        <?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();
        }

        }
        {code}

        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:

        {code}
        $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); $a1->movies->add($m1);

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

        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:

        {code}
        $em->flush();
        {code}

        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.
        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:

        {code}
        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();
        }

        }
        {code}

        {code}
        <?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();
        }

        }
        {code}

        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:

        {code}
        $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);
        {code}

        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:

        {code}
        $em->flush();
        {code}

        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.
        Roman S. Borschel made changes -
        Fix Version/s 2.0-RC1 [ 10091 ]
        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.
        Dave Keen made changes -
        Attachment DDC758Test.php [ 10792 ]
        Dave Keen made changes -
        Attachment DDC758Test.php [ 10792 ]
        Dave Keen made changes -
        Attachment DDC758Test.php [ 10794 ]
        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.
        Dave Keen made changes -
        Attachment DDC758Test.php [ 10794 ]
        Dave Keen made changes -
        Attachment DDC758Test.php [ 10807 ]
        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?
        Dave Keen made changes -
        Attachment 0147-DDC-758.patch [ 10808 ]
        Dave Keen made changes -
        Attachment 0147-DDC-758.patch [ 10808 ]
        Dave Keen made changes -
        Comment [ This is an attempted patch to the issue. After each cascade merges this fills in $snapshot from the database. ]
        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.
        Dave Keen made changes -
        Attachment 0147-DDC-758.patch [ 10809 ]
        Dave Keen made changes -
        Attachment 0147-DDC-758.patch [ 10809 ]
        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.
        Dave Keen made changes -
        Attachment 0147-DDC-758.patch [ 10810 ]
        Hide
        Benjamin Eberlei added a comment -

        Fixed, can you verify Dave?

        Show
        Benjamin Eberlei added a comment - Fixed, can you verify Dave?
        Benjamin Eberlei made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Assignee Roman S. Borschel [ romanb ] Benjamin Eberlei [ beberlei ]
        Resolution Fixed [ 1 ]
        Benjamin Eberlei made changes -
        Workflow jira [ 11802 ] jira-feedback [ 14544 ]
        Benjamin Eberlei made changes -
        Workflow jira-feedback [ 14544 ] jira-feedback2 [ 16408 ]
        Benjamin Eberlei made changes -
        Workflow jira-feedback2 [ 16408 ] jira-feedback3 [ 18661 ]

        This list may be incomplete, as errors occurred whilst retrieving source from linked applications:

        • Request to http://www.doctrine-project.org/fisheye/ failed: Error in remote call to 'FishEye 0 (http://www.doctrine-project.org/fisheye/)' (http://www.doctrine-project.org/fisheye) [AbstractRestCommand{path='/rest-service-fe/search-v1/crossRepositoryQuery', params={query=DDC-758, expand=changesets[0:20].revisions[0:29],reviews}, methodType=GET}] : Received status code 503 (Service Temporarily Unavailable)

          People

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

            Dates

            • Created:
              Updated:
              Resolved: