Doctrine 2 - ORM
  1. Doctrine 2 - ORM
  2. DDC-1283

Possible issue with PersistentCollection#getDelete/InsertDiff()

    Details

    • Type: Improvement Improvement
    • Status: Reopened
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 2.1
    • Fix Version/s: 2.5
    • Component/s: None
    • Security Level: All
    • Labels:
      None

      Description

      Using the following code, when you go from (1, 2) to (1), (2) is deleted as expected. However, if you go from (1, 2) to (2), (1) and (2) are deleted and (2) is then inserted. Is this the desired behaviour? (i.e. 2 extra queries)

      $bracket->getTournamentLocations()->takeSnapshot();
      
      $col = $bracket->getTournamentLocations()->unwrap();
      
      $col->clear();
      
      foreach ($form->getValue('tournamentLocations') as $id) {
          $col->add($em->getReference('Tournaments_Model_TournamentLocation', $id));
      }
      
      $bracket->getTournamentLocations()->setDirty(true);

        Activity

        Glen Ainscow created issue -
        Glen Ainscow made changes -
        Field Original Value New Value
        Description Using the following code, when you go from (1, 2) to (1), (2) is deleted as expected. However, if you go from (1, 2) to (2), (1) and (2) are deleted and (2) is then inserted. Is this the desired behaviour? (i.e. 2 extra queries)

        {code}$bracket->getTournamentLocations()->takeSnapshot();

                    $col = $bracket->getTournamentLocations()->unwrap();

                    $col->clear();

                    foreach ($form->getValue('tournamentLocations') as $id) {
                        $col->add($em->getReference('Tournaments_Model_TournamentLocation', $id));
                    }

                    $bracket->getTournamentLocations()->setDirty(true);{code}
        Using the following code, when you go from (1, 2) to (1), (2) is deleted as expected. However, if you go from (1, 2) to (2), (1) and (2) are deleted and (2) is then inserted. Is this the desired behaviour? (i.e. 2 extra queries)

        {code}$bracket->getTournamentLocations()->takeSnapshot();

        $col = $bracket->getTournamentLocations()->unwrap();

        $col->clear();

        foreach ($form->getValue('tournamentLocations') as $id) {
            $col->add($em->getReference('Tournaments_Model_TournamentLocation', $id));
        }

        $bracket->getTournamentLocations()->setDirty(true);{code}
        Hide
        Benjamin Eberlei added a comment -

        First, you are using internal API therefore you are on your own anyways.

        This is marked as improvment now, the functionality works, it may just be inefficient.

        Show
        Benjamin Eberlei added a comment - First, you are using internal API therefore you are on your own anyways. This is marked as improvment now, the functionality works, it may just be inefficient.
        Benjamin Eberlei made changes -
        Issue Type Bug [ 1 ] Improvement [ 4 ]
        Fix Version/s 2.2-DEV [ 10157 ]
        Hide
        Guilherme Blanco added a comment -

        Hi,

        I'm marking issue as invalid because you're conceptually wrong.
        What you're trying to do is telling that a collection of new entities is holded by a collection of Persistent entities.
        The reference internally of PersistentCollection to ArrayCollection means a lot here.

        Correct code would be you to regenerate the collection (a new ArrayCollection) and just assign it to setTournamentLocations($newCollection);

        Does this explanation is enough for you?

        Cheers,

        Show
        Guilherme Blanco added a comment - Hi, I'm marking issue as invalid because you're conceptually wrong. What you're trying to do is telling that a collection of new entities is holded by a collection of Persistent entities. The reference internally of PersistentCollection to ArrayCollection means a lot here. Correct code would be you to regenerate the collection (a new ArrayCollection) and just assign it to setTournamentLocations($newCollection); Does this explanation is enough for you? Cheers,
        Guilherme Blanco made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Assignee Benjamin Eberlei [ beberlei ] Guilherme Blanco [ guilhermeblanco ]
        Resolution Fixed [ 1 ]
        Hide
        Glen Ainscow added a comment -

        Hi Guilherme,

        If I do this:

        $locations = new ArrayCollection();
        
        foreach ($form->getValue('tournamentLocations') as $id) {
            $locations->add($em->getReference('Tournaments_Model_TournamentLocation', $id));
        }
        
        $bracket->setTournamentLocations($locations);
        

        ... then all the records are deleted, before adding the new records. This is inefficient and causes extra, unnecessary write operations.

        Can't Doctrine perform diffs when persisting the collection, so that only the necessary deletes and inserts are executed?

        Show
        Glen Ainscow added a comment - Hi Guilherme, If I do this: $locations = new ArrayCollection(); foreach ($form->getValue('tournamentLocations') as $id) { $locations->add($em->getReference('Tournaments_Model_TournamentLocation', $id)); } $bracket->setTournamentLocations($locations); ... then all the records are deleted, before adding the new records. This is inefficient and causes extra, unnecessary write operations. Can't Doctrine perform diffs when persisting the collection, so that only the necessary deletes and inserts are executed?
        Glen Ainscow made changes -
        Resolution Fixed [ 1 ]
        Status Resolved [ 5 ] Reopened [ 4 ]
        Hide
        Guilherme Blanco added a comment -

        We could add it, but I don't think it worth the effort.
        Main problem with this one is that we use C-level binary comparison to get the diff. That's what you entities/hash pointers are different.
        We would have to write our own diff-comparator for both collections, which would probably slowdown the entire Doctrine.

        I'd rather consider that it's not possible to be done at the moment, but I need much more investigation for that. This will be something that I'll probably only do when I look at this issue again with a lot of time (which is really hard to happen).

        If you have some spare time, feel free to make some attempts.
        Just don't forget to enable performance tests in Doctrine Unit Test suite.

        Show
        Guilherme Blanco added a comment - We could add it, but I don't think it worth the effort. Main problem with this one is that we use C-level binary comparison to get the diff. That's what you entities/hash pointers are different. We would have to write our own diff-comparator for both collections, which would probably slowdown the entire Doctrine. I'd rather consider that it's not possible to be done at the moment, but I need much more investigation for that. This will be something that I'll probably only do when I look at this issue again with a lot of time (which is really hard to happen). If you have some spare time, feel free to make some attempts. Just don't forget to enable performance tests in Doctrine Unit Test suite.
        Benjamin Eberlei made changes -
        Fix Version/s 2.3 [ 10185 ]
        Fix Version/s 2.2 [ 10157 ]
        Benjamin Eberlei made changes -
        Workflow jira [ 12840 ] jira-feedback [ 14103 ]
        Benjamin Eberlei made changes -
        Workflow jira-feedback [ 14103 ] jira-feedback2 [ 15967 ]
        Benjamin Eberlei made changes -
        Workflow jira-feedback2 [ 15967 ] jira-feedback3 [ 18221 ]
        Benjamin Eberlei made changes -
        Fix Version/s 2.4 [ 10321 ]
        Fix Version/s 2.3 [ 10185 ]
        Benjamin Eberlei made changes -
        Fix Version/s 2.5 [ 10522 ]
        Fix Version/s 2.4 [ 10321 ]

        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-1283, expand=changesets[0:20].revisions[0:29],reviews}, methodType=GET}] : Received status code 503 (Service Temporarily Unavailable)

          People

          • Assignee:
            Guilherme Blanco
            Reporter:
            Glen Ainscow
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated: