Doctrine 2 - ORM
  1. Doctrine 2 - ORM
  2. DDC-763

Cascade merge on associated entities can insert too many rows through "Persistence by Reachability"

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: 2.x
    • Component/s: ORM
    • Security Level: All
    • Labels:
      None

      Description

      I think that the UnitOfWork needs to maintain a map of spl_object_hash($newEntity)->$managedEntity for entities that were persisted via reachability during a merge. doMerge should then only call persistNew if the original entity has not already been persisted (if it has already been persisted it should merge the managed entity from the map). The map should be maintained until a flush() or until the UnitOfWork is cleared. The reasoning is as follows.

      Imagine we have a simple doctor object with no associations:

      $doctor = new Doctor();
      $em->persist($doctor);
      $em->persist($doctor);
      $em->flush();
      

      After the first persist() $doctor is MANAGED so the second persist has no effect and this results in a single Doctor row.

      If we do the same thing using merge and persistence by reachability:

      $doctor = new Doctor();
      $em->merge($doctor);
      $em->merge($doctor);
      $em->flush();
      

      we get 2 Doctor rows being added.

      Obviously in this particular case we should use the return value from the first merge() as the parameter of the second merge which would give correct behaviour.

      However, now imagine one Doctor has many Patients and many Patients have one Doctor, all the associations have cascade merge enabled, and further assume that $d1 (Doctor id=1) is already in the database. We now attempt to create two patients and assign them to the existing doctor:

      $d1= new Doctor(); $d1->id = 1; // This is a DETACHED entity
      
      $p1 = new Patient();
      $p2 = new Patient();
      
      $d1->patients->add($p1); $p1->doctor = $d1;
      $d1->patients->add($p2); $p2->doctor = $d1;
      
      $em->merge($p1);
      $em->merge($p2);
      
      $em->flush();
      

      This actually results in 4 rows being added to the 'patients' table instead of 2, I think because $p1 and $p2 are getting persisted both as the root objects and then again from the patient->doctor->patients array. Since the cascade merging happens internally we can't replace the array contents with the managed return values without walking through the object graph (in which case there is no point in using cascade merge in the first place). Maintaining a map in UnitOfWork will allow doMerge to ensure it doesn't persist the same entities twice.

      I'm not sure, but this might be relevant for cascade persist too.

      P.S. Another bug report on this can be found at http://code.google.com/p/flextrine2/issues/detail?id=32 (it basically says the same thing with different entities).

      1. 0149-DDC-763.patch
        16 kB
        Dave Keen
      2. DDC763Test.php
        7 kB
        Dave Keen
      3. multipleaddmerge.diff
        1 kB
        Dave Keen

        Activity

        Dave Keen created issue -
        Dave Keen made changes -
        Field Original Value New Value
        Description I think that the UnitOfWork needs to maintain a map of spl_object_hash($newEntity)->$managedEntity for entities that were persisted via reachability during a merge. doMerge should then only call persistNew is the original entity has not already been persisted (if so it merged the managed entity from the map). The map should be maintained until a flush() or until the UnitOfWork is cleared. The reasoning is as follows.

        Imagine we have a simple doctor object with no associations:

        $doctor = new Doctor();
        $em->persist($doctor);
        $em->persist($doctor);
        $em->flush();

        After the first persist() $doctor is MANAGED so the second persist has no effect and this results in a single Doctor row.

        If we do the same thing using merge and persistence by reachability:

        $doctor = new Doctor();
        $em->merge($doctor);
        $em->merge($doctor);
        $em->flush();

        we get 2 Doctor rows being added.

        Obviously in this particular case we should use the return value from the first merge() as the parameter of the second merge which would give correct behaviour.

        However, now imagine one Doctor has many Patients and many Patients have one Doctor, all the associations have cascade merge enabled, and further assume that $d1 (Doctor id=1) is already in the database. We now attempt to create two patients and assign them to the existing doctor:

        $d1= new Doctor(); $d1->id = 1; // This is a DETACHED entity

        $p1 = new Patient();
        $p2 = new Patient();

        $d1->patients->add($p1); $p1->doctor = $d1;
        $d1->patients->add($p2); $p2->doctor = $d1;

        $em->merge($p1);
        $em->merge($p2);

        $em->flush();

        This actually results in 4 rows being added to the 'patients' table instead of 2, I think because $p1 and $p2 are getting persisted both as the root objects and then again from the patient->doctor->patients array. Since the cascade merging happens internally we can't replace the array contents with the managed return values without walking through the object graph (in which case there is no point in using cascade merge in the first place). Maintaining a map in UnitOfWork will allow doMerge to ensure it doesn't persist the same entities twice.

        I'm not sure, but this might be relevant for cascade persist too.

        P.S. Another bug report on this can be found at http://code.google.com/p/flextrine2/issues/detail?id=32 (it basically says the same thing with different entities).
        I think that the UnitOfWork needs to maintain a map of spl_object_hash($newEntity)->$managedEntity for entities that were persisted via reachability during a merge. doMerge should then only call persistNew is the original entity has not already been persisted (if so it merged the managed entity from the map). The map should be maintained until a flush() or until the UnitOfWork is cleared. The reasoning is as follows.

        Imagine we have a simple doctor object with no associations:

        {code}
        $doctor = new Doctor();
        $em->persist($doctor);
        $em->persist($doctor);
        $em->flush();
        {code}

        After the first persist() $doctor is MANAGED so the second persist has no effect and this results in a single Doctor row.

        If we do the same thing using merge and persistence by reachability:

        {code}
        $doctor = new Doctor();
        $em->merge($doctor);
        $em->merge($doctor);
        $em->flush();
        {code}

        we get 2 Doctor rows being added.

        Obviously in this particular case we should use the return value from the first merge() as the parameter of the second merge which would give correct behaviour.

        However, now imagine one Doctor has many Patients and many Patients have one Doctor, all the associations have cascade merge enabled, and further assume that $d1 (Doctor id=1) is already in the database. We now attempt to create two patients and assign them to the existing doctor:

        {code}
        $d1= new Doctor(); $d1->id = 1; // This is a DETACHED entity

        $p1 = new Patient();
        $p2 = new Patient();

        $d1->patients->add($p1); $p1->doctor = $d1;
        $d1->patients->add($p2); $p2->doctor = $d1;

        $em->merge($p1);
        $em->merge($p2);

        $em->flush();
        {code}

        This actually results in 4 rows being added to the 'patients' table instead of 2, I think because $p1 and $p2 are getting persisted both as the root objects and then again from the patient->doctor->patients array. Since the cascade merging happens internally we can't replace the array contents with the managed return values without walking through the object graph (in which case there is no point in using cascade merge in the first place). Maintaining a map in UnitOfWork will allow doMerge to ensure it doesn't persist the same entities twice.

        I'm not sure, but this might be relevant for cascade persist too.

        P.S. Another bug report on this can be found at http://code.google.com/p/flextrine2/issues/detail?id=32 (it basically says the same thing with different entities).
        Dave Keen made changes -
        Description I think that the UnitOfWork needs to maintain a map of spl_object_hash($newEntity)->$managedEntity for entities that were persisted via reachability during a merge. doMerge should then only call persistNew is the original entity has not already been persisted (if so it merged the managed entity from the map). The map should be maintained until a flush() or until the UnitOfWork is cleared. The reasoning is as follows.

        Imagine we have a simple doctor object with no associations:

        {code}
        $doctor = new Doctor();
        $em->persist($doctor);
        $em->persist($doctor);
        $em->flush();
        {code}

        After the first persist() $doctor is MANAGED so the second persist has no effect and this results in a single Doctor row.

        If we do the same thing using merge and persistence by reachability:

        {code}
        $doctor = new Doctor();
        $em->merge($doctor);
        $em->merge($doctor);
        $em->flush();
        {code}

        we get 2 Doctor rows being added.

        Obviously in this particular case we should use the return value from the first merge() as the parameter of the second merge which would give correct behaviour.

        However, now imagine one Doctor has many Patients and many Patients have one Doctor, all the associations have cascade merge enabled, and further assume that $d1 (Doctor id=1) is already in the database. We now attempt to create two patients and assign them to the existing doctor:

        {code}
        $d1= new Doctor(); $d1->id = 1; // This is a DETACHED entity

        $p1 = new Patient();
        $p2 = new Patient();

        $d1->patients->add($p1); $p1->doctor = $d1;
        $d1->patients->add($p2); $p2->doctor = $d1;

        $em->merge($p1);
        $em->merge($p2);

        $em->flush();
        {code}

        This actually results in 4 rows being added to the 'patients' table instead of 2, I think because $p1 and $p2 are getting persisted both as the root objects and then again from the patient->doctor->patients array. Since the cascade merging happens internally we can't replace the array contents with the managed return values without walking through the object graph (in which case there is no point in using cascade merge in the first place). Maintaining a map in UnitOfWork will allow doMerge to ensure it doesn't persist the same entities twice.

        I'm not sure, but this might be relevant for cascade persist too.

        P.S. Another bug report on this can be found at http://code.google.com/p/flextrine2/issues/detail?id=32 (it basically says the same thing with different entities).
        I think that the UnitOfWork needs to maintain a map of spl_object_hash($newEntity)->$managedEntity for entities that were persisted via reachability during a merge. doMerge should then only call persistNew if the original entity has not already been persisted (if it has already been persisted it should merge the managed entity from the map). The map should be maintained until a flush() or until the UnitOfWork is cleared. The reasoning is as follows.

        Imagine we have a simple doctor object with no associations:

        {code}
        $doctor = new Doctor();
        $em->persist($doctor);
        $em->persist($doctor);
        $em->flush();
        {code}

        After the first persist() $doctor is MANAGED so the second persist has no effect and this results in a single Doctor row.

        If we do the same thing using merge and persistence by reachability:

        {code}
        $doctor = new Doctor();
        $em->merge($doctor);
        $em->merge($doctor);
        $em->flush();
        {code}

        we get 2 Doctor rows being added.

        Obviously in this particular case we should use the return value from the first merge() as the parameter of the second merge which would give correct behaviour.

        However, now imagine one Doctor has many Patients and many Patients have one Doctor, all the associations have cascade merge enabled, and further assume that $d1 (Doctor id=1) is already in the database. We now attempt to create two patients and assign them to the existing doctor:

        {code}
        $d1= new Doctor(); $d1->id = 1; // This is a DETACHED entity

        $p1 = new Patient();
        $p2 = new Patient();

        $d1->patients->add($p1); $p1->doctor = $d1;
        $d1->patients->add($p2); $p2->doctor = $d1;

        $em->merge($p1);
        $em->merge($p2);

        $em->flush();
        {code}

        This actually results in 4 rows being added to the 'patients' table instead of 2, I think because $p1 and $p2 are getting persisted both as the root objects and then again from the patient->doctor->patients array. Since the cascade merging happens internally we can't replace the array contents with the managed return values without walking through the object graph (in which case there is no point in using cascade merge in the first place). Maintaining a map in UnitOfWork will allow doMerge to ensure it doesn't persist the same entities twice.

        I'm not sure, but this might be relevant for cascade persist too.

        P.S. Another bug report on this can be found at http://code.google.com/p/flextrine2/issues/detail?id=32 (it basically says the same thing with different entities).
        Roman S. Borschel made changes -
        Fix Version/s 2.0-BETA4 [ 10072 ]
        Hide
        Benjamin Eberlei added a comment -

        @Roman A possible fix for this in my opinion is another map in UnitOfWork $mergedEntities = array(); and a patch like this:

        diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php
        index 242d84b..1d0d8b3 100644
        --- a/lib/Doctrine/ORM/UnitOfWork.php
        +++ b/lib/Doctrine/ORM/UnitOfWork.php
        @@ -1340,6 +1340,10 @@ class UnitOfWork implements PropertyChangedListener
                     return; // Prevent infinite recursion
                 }
         
        +        if (isset($this->mergedEntities[$oid])) {
        +            return $this->mergedEntities[$oid];
        +        }
        +
                 $visited[$oid] = $entity; // mark visited
         
                 $class = $this->em->getClassMetadata(get_class($entity));
        @@ -1468,6 +1472,8 @@ class UnitOfWork implements PropertyChangedListener
         
                 $this->cascadeMerge($entity, $managedCopy, $visited);
         
        +        $this->mergedEntities[$oid] = $managedCopy;
        +
                 return $managedCopy;
             }
        
        Show
        Benjamin Eberlei added a comment - @Roman A possible fix for this in my opinion is another map in UnitOfWork $mergedEntities = array(); and a patch like this: diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index 242d84b..1d0d8b3 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -1340,6 +1340,10 @@ class UnitOfWork implements PropertyChangedListener return ; // Prevent infinite recursion } + if (isset($ this ->mergedEntities[$oid])) { + return $ this ->mergedEntities[$oid]; + } + $visited[$oid] = $entity; // mark visited $class = $ this ->em->getClassMetadata(get_class($entity)); @@ -1468,6 +1472,8 @@ class UnitOfWork implements PropertyChangedListener $ this ->cascadeMerge($entity, $managedCopy, $visited); + $ this ->mergedEntities[$oid] = $managedCopy; + return $managedCopy; }
        Hide
        Dave Keen added a comment -

        I have tested this patch with my application and it fixes the problem in all my relevant test cases apart from one. The test case that's failing is one that persists a bi-directional many to many relationship, so the associations interweave with each other (if you know what I mean).

        I wonder if perhaps doMerge need to continue cascading even if it finds an item in $this->mergedEntities

        This is the Flextrine code that fails - it results in no entries in movie_artist. This might also be related to DDC-758?

        m1 = new Movie();
        m1.title = "Movie 1";

        m2 = new Movie();
        m2.title = "Movie 2";

        a1 = new Artist();
        a1.name = "Artist 1";

        a2 = new Artist();
        a2.name = "Artist 2";

        m1.artists.addItem(a1); a1.movies.addItem(m1);
        m1.artists.addItem(a2); a2.movies.addItem(m1);

        m2.artists.addItem(a1); a1.movies.addItem(m2);
        m2.artists.addItem(a2); a2.movies.addItem(m2);

        // These translate to cascade merges on the server
        em.persist(m1);
        em.persist(m2);
        em.persist(a1);
        em.persist(a2);

        // Now flush
        em.flush();

        Show
        Dave Keen added a comment - I have tested this patch with my application and it fixes the problem in all my relevant test cases apart from one. The test case that's failing is one that persists a bi-directional many to many relationship, so the associations interweave with each other (if you know what I mean). I wonder if perhaps doMerge need to continue cascading even if it finds an item in $this->mergedEntities This is the Flextrine code that fails - it results in no entries in movie_artist. This might also be related to DDC-758 ? m1 = new Movie(); m1.title = "Movie 1"; m2 = new Movie(); m2.title = "Movie 2"; a1 = new Artist(); a1.name = "Artist 1"; a2 = new Artist(); a2.name = "Artist 2"; m1.artists.addItem(a1); a1.movies.addItem(m1); m1.artists.addItem(a2); a2.movies.addItem(m1); m2.artists.addItem(a1); a1.movies.addItem(m2); m2.artists.addItem(a2); a2.movies.addItem(m2); // These translate to cascade merges on the server em.persist(m1); em.persist(m2); em.persist(a1); em.persist(a2); // Now flush em.flush();
        Hide
        Dave Keen added a comment -

        P.S. This test passes if I translate em.persist() to $em->persist() (not cascading) on the server instead of translating it to a cascade merge; not sure if that helps

        Show
        Dave Keen added a comment - P.S. This test passes if I translate em.persist() to $em->persist() (not cascading) on the server instead of translating it to a cascade merge; not sure if that helps
        Hide
        Roman S. Borschel added a comment -

        I'd really like to avoid introducing an additional instance variable just to solve this issue but I did not find the time yet to really look into it.

        Does someone have a unit test for this already and can attach it to the issue?

        Show
        Roman S. Borschel added a comment - I'd really like to avoid introducing an additional instance variable just to solve this issue but I did not find the time yet to really look into it. Does someone have a unit test for this already and can attach it to the issue?
        Hide
        Roman S. Borschel added a comment -

        Rescheduling for RC1.

        Show
        Roman S. Borschel added a comment - Rescheduling for RC1.
        Roman S. Borschel made changes -
        Fix Version/s 2.0-RC1 [ 10091 ]
        Fix Version/s 2.0-BETA4 [ 10072 ]
        Dave Keen made changes -
        Attachment PersistByReachabilityMergeTest.php [ 10785 ]
        Dave Keen made changes -
        Attachment PersistByReachabilityMergeTest.php [ 10785 ]
        Dave Keen made changes -
        Attachment DDC763Test.php [ 10793 ]
        Dave Keen made changes -
        Attachment DDC763Test.php [ 10793 ]
        Dave Keen made changes -
        Comment [ Also to confirm, Benjamin's fix seems to work fine, and passes the test case. The many-many issue I mentioned above is definitely a different bug (DDC-758). ]
        Dave Keen made changes -
        Comment [ Here is a functional unit test case for this issue (both the simple and complex cases). ]
        Hide
        Dave Keen added a comment -

        Here is a functional test case containing three tests:

        testMultiMerge tests basic merging of two new entities, checking that only a single entity ends up in the database. This passes with Benjamin's patch.

        testMultiCascadeMerge tests the more complex case of merging a OneToMany association. This also passes with Benjamin's patch.

        testManyToManyPersistByReachability tests the ManyToMany case described above and this fails with Benjamin's patch, probably because doMerge doesn't cascade down entities that it has already merged and some ManyToMany associations are being ignored. Its a bit hard to be certain what is causing this as even without Benjamin's patch this test would fail due to DDC-758.

        Show
        Dave Keen added a comment - Here is a functional test case containing three tests: testMultiMerge tests basic merging of two new entities, checking that only a single entity ends up in the database. This passes with Benjamin's patch. testMultiCascadeMerge tests the more complex case of merging a OneToMany association. This also passes with Benjamin's patch. testManyToManyPersistByReachability tests the ManyToMany case described above and this fails with Benjamin's patch, probably because doMerge doesn't cascade down entities that it has already merged and some ManyToMany associations are being ignored. Its a bit hard to be certain what is causing this as even without Benjamin's patch this test would fail due to DDC-758 .
        Dave Keen made changes -
        Attachment DDC763Test.php [ 10795 ]
        Hide
        Benjamin Eberlei added a comment - - edited

        @Roman i thought about this issue, its not possible without that additional map of merged entities. There is no way we can get that information from other sources.

        Problem is rather that the use-case probably only applies in mass-merging scenarios and client-server serialization.

        Show
        Benjamin Eberlei added a comment - - edited @Roman i thought about this issue, its not possible without that additional map of merged entities. There is no way we can get that information from other sources. Problem is rather that the use-case probably only applies in mass-merging scenarios and client-server serialization.
        Dave Keen made changes -
        Attachment DDC763Test.php [ 10795 ]
        Hide
        Dave Keen added a comment -

        Added another failing test case - adding the same entity from different ends of a many to many bi-directional association to check that there isn't an integrity constraint violation caused by Doctrine trying to add the same row twice.

        Show
        Dave Keen added a comment - Added another failing test case - adding the same entity from different ends of a many to many bi-directional association to check that there isn't an integrity constraint violation caused by Doctrine trying to add the same row twice.
        Dave Keen made changes -
        Attachment DDC763Test.php [ 10811 ]
        Hide
        Dave Keen added a comment -

        Attached a patch for this issue.

        Show
        Dave Keen added a comment - Attached a patch for this issue.
        Dave Keen made changes -
        Attachment 0149-DDC-763.patch [ 10812 ]
        Hide
        Benjamin Eberlei added a comment -

        can you comment why all the additionall stuff is necessary compared to my patch?

        Show
        Benjamin Eberlei added a comment - can you comment why all the additionall stuff is necessary compared to my patch?
        Hide
        Dave Keen added a comment - - edited

        It fixes the two additional test cases - testManyToManyPersistByReachability and testManyToManyDuplicatePersistByReachability.

        testManyToManyPersistByReachability was failing with your original patch because there are ManyToMany cases where an entity may have already been merged, but its still necessary to add it to an association and continue to cascade. Running the following with the original patch will miss out some of the associations.

        $m1 = new Movie();
        $m1->title = "Movie 1";
        
        $m2 = new Movie();
        $m2->title = "Movie 2";
        
        $a1 = new Artist();
        $a1->name = "Artist 1";
        
        $a2 = new Artist();
        $a2->name = "Artist 2";
        
        $m1->artists->add($a1); $a1->movies->add($m1);
        $m1->artists->add($a2); $a2->movies->add($m1);
        $m2->artists->add($a1); $a1->movies->add($m2);
        $m2->artists->add($a2); $a2->movies->add($m2);
        
        $em->merge($a1);
        $em->merge($a2);
        $em->flush();
        

        The other change in my patch is to protect against this case. It ensures that the following code doesn't add the same entity twice to a collection.

        $em->merge($m1);
        $em->merge($m2);
        $em->merge($a2);
        $em->merge($a2);
        $em->flush();
        
        Show
        Dave Keen added a comment - - edited It fixes the two additional test cases - testManyToManyPersistByReachability and testManyToManyDuplicatePersistByReachability. testManyToManyPersistByReachability was failing with your original patch because there are ManyToMany cases where an entity may have already been merged, but its still necessary to add it to an association and continue to cascade. Running the following with the original patch will miss out some of the associations. $m1 = new Movie(); $m1->title = "Movie 1" ; $m2 = new Movie(); $m2->title = "Movie 2" ; $a1 = new Artist(); $a1->name = "Artist 1" ; $a2 = new Artist(); $a2->name = "Artist 2" ; $m1->artists->add($a1); $a1->movies->add($m1); $m1->artists->add($a2); $a2->movies->add($m1); $m2->artists->add($a1); $a1->movies->add($m2); $m2->artists->add($a2); $a2->movies->add($m2); $em->merge($a1); $em->merge($a2); $em->flush(); The other change in my patch is to protect against this case. It ensures that the following code doesn't add the same entity twice to a collection. $em->merge($m1); $em->merge($m2); $em->merge($a2); $em->merge($a2); $em->flush();
        Hide
        Benjamin Eberlei added a comment -

        I am not sure if the issue here is rather multiple calls to merge that contain different parts of the same object-graph.

        There should be a very simple fix for this, call ->clear() after each merge.

        I am not sure if this patch drags us into a blackhole of issues with merging.

        Show
        Benjamin Eberlei added a comment - I am not sure if the issue here is rather multiple calls to merge that contain different parts of the same object-graph. There should be a very simple fix for this, call ->clear() after each merge. I am not sure if this patch drags us into a blackhole of issues with merging.
        Hide
        Dave Keen added a comment -

        Calling ->clear() and ->flush() after each merge is a workaround for the simple case, but unless I am misunderstanding I don't think its a solution for cases where the merging is happening automatically in cascadeMerge. I've actually encountered this issue in another project and scenario to do with creating REST APIs and merging JSON objects into entities, and applying the patch fixed it so a) I think this issue might be a more common that we first thought and b) the patch basically seems to work (plus it doesn't introduce any failing cases in the existing test suite). I can actually still find one edge case to do with cascading merging interlinked many to many associations that this doesn't fix, but I was planning to open that as a new ticket after this My feeling is that the current merge already has issues and this definitely improves it.

        Show
        Dave Keen added a comment - Calling ->clear() and ->flush() after each merge is a workaround for the simple case, but unless I am misunderstanding I don't think its a solution for cases where the merging is happening automatically in cascadeMerge. I've actually encountered this issue in another project and scenario to do with creating REST APIs and merging JSON objects into entities, and applying the patch fixed it so a) I think this issue might be a more common that we first thought and b) the patch basically seems to work (plus it doesn't introduce any failing cases in the existing test suite). I can actually still find one edge case to do with cascading merging interlinked many to many associations that this doesn't fix, but I was planning to open that as a new ticket after this My feeling is that the current merge already has issues and this definitely improves it.
        Hide
        Benjamin Eberlei added a comment -

        It cannot happen inside a single merge, single merges use the $visited to avoid infinite recursions, each entity can only be merged once inside a single merge operation.

        Show
        Benjamin Eberlei added a comment - It cannot happen inside a single merge, single merges use the $visited to avoid infinite recursions, each entity can only be merged once inside a single merge operation.
        Hide
        Benjamin Eberlei added a comment -

        Added a note into the documentation about using EntityManager#clear between merging of entities which share subgraphs and cascade merge.

        Handling this issue in UnitOfwork will be declared an improvement, not a bug anymore and be scheduled for later releases. The required changes to the core are to dangerous and big.

        Show
        Benjamin Eberlei added a comment - Added a note into the documentation about using EntityManager#clear between merging of entities which share subgraphs and cascade merge. Handling this issue in UnitOfwork will be declared an improvement, not a bug anymore and be scheduled for later releases. The required changes to the core are to dangerous and big.
        Benjamin Eberlei made changes -
        Issue Type Bug [ 1 ] Improvement [ 4 ]
        Assignee Roman S. Borschel [ romanb ] Benjamin Eberlei [ beberlei ]
        Fix Version/s 2.1 [ 10022 ]
        Fix Version/s 2.0-RC1 [ 10091 ]
        Priority Critical [ 2 ] Major [ 3 ]
        Hide
        Dave Keen added a comment -

        Where in the docs is that?

        Just to summarize, the equivalent operation to having multiple merges and a single flush is to call merge followed by flush each time, with the whole thing surrounded by a transaction? Does this have a big impact on performance?

        Show
        Dave Keen added a comment - Where in the docs is that? Just to summarize, the equivalent operation to having multiple merges and a single flush is to call merge followed by flush each time, with the whole thing surrounded by a transaction? Does this have a big impact on performance?
        Hide
        Dave Keen added a comment -

        Ben - even given the decision not to implement this (and I do understand your thinking, as it is a major change), is there any reason not to implement the bit that ensures that the same entity isn't added to a collection twice during a merge? I can't think of a situation where this should be allowed, and I have a use case where I get 'DUPLICATE KEY' errors if this isn't there.

        Please see attached patch.

        Show
        Dave Keen added a comment - Ben - even given the decision not to implement this (and I do understand your thinking, as it is a major change), is there any reason not to implement the bit that ensures that the same entity isn't added to a collection twice during a merge? I can't think of a situation where this should be allowed, and I have a use case where I get 'DUPLICATE KEY' errors if this isn't there. Please see attached patch.
        Dave Keen made changes -
        Attachment multipleaddmerge.diff [ 10858 ]
        Hide
        Benjamin Eberlei added a comment -

        What bit of that huge patch is that? Can you extract it into another ticket if thats possible?

        Show
        Benjamin Eberlei added a comment - What bit of that huge patch is that? Can you extract it into another ticket if thats possible?
        Hide
        Benjamin Eberlei added a comment -

        I added it to "Working with Objects" and the descripton of Merge. Its not yet live on the site.

        Using this current workaround has a performance impact, since more SELECT statements have to be issued against the database.

        Show
        Benjamin Eberlei added a comment - I added it to "Working with Objects" and the descripton of Merge. Its not yet live on the site. Using this current workaround has a performance impact, since more SELECT statements have to be issued against the database.
        Hide
        Dave Keen added a comment -

        Apologies for not being clear - only the 3rd patch (multipleaddmerge.diff) is relevant to the 'DUPLICATE KEY' error I am now talking about, but I'll put it in a nother ticket if you prefer.

        Show
        Dave Keen added a comment - Apologies for not being clear - only the 3rd patch (multipleaddmerge.diff) is relevant to the 'DUPLICATE KEY' error I am now talking about, but I'll put it in a nother ticket if you prefer.
        Hide
        Benjamin Eberlei added a comment -

        please add a new ticket, patch looks good.

        Show
        Benjamin Eberlei added a comment - please add a new ticket, patch looks good.
        Hide
        Dave Keen added a comment -

        Created as DDC-875

        Show
        Dave Keen added a comment - Created as DDC-875
        Benjamin Eberlei made changes -
        Fix Version/s 2.x [ 10090 ]
        Fix Version/s 2.1 [ 10022 ]
        Benjamin Eberlei made changes -
        Workflow jira [ 11812 ] jira-feedback [ 13871 ]
        Benjamin Eberlei made changes -
        Workflow jira-feedback [ 13871 ] jira-feedback2 [ 15735 ]
        Benjamin Eberlei made changes -
        Workflow jira-feedback2 [ 15735 ] jira-feedback3 [ 17992 ]

        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-763, 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:
            2 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated: