Doctrine MongoDB ODM
  1. Doctrine MongoDB ODM
  2. MODM-29

Persisting changes ordering of elements in embedded collection in some cases

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.0.0ALPHA2
    • Fix Version/s: 1.0.0BETA2
    • Component/s: Persister
    • Labels:
      None

      Description

      After changing ordering of elements in collection, it is persisted incorrectly (with different ordering than expected).

      Here is a test case:

      /** @Document(collection="tests", db="tests") */
      class doc
      {
          /** @Id */
          protected $id;
      
          /** @EmbedMany(targetDocument="emb") */
          protected $collection;
      
          function __construct($c) {$this->set($c);}
      
          function set($c) {$this->collection = $c;}
          function get() {return $this->collection;}
      }
      
      /** @EmbeddedDocument */
      class emb
      {
          /** @String */
          protected $val;
      
          function __construct($val) {$this->val = $val;}
          function get() {return $this->val;}
      }
      
      $collection = new ArrayCollection(array(
          new emb('0'),
          new emb('1'),
          new emb('2')
      ));
      
      // TEST CASE:
      $doc = new doc($collection);
      
      $dm->persist($doc);
      $dm->flush();
      
      // place element '0' after '1'
      $collection = new ArrayCollection(array(
          $collection[1],
          $collection[0],
          $collection[2]
      ));
      
      $doc->set($collection);
      
      $dm->persist($doc);
      $dm->flush();
      
      $dm->refresh($doc);
      
      foreach($doc->get() as $value) {
          echo $value->get() . "\n";
      }
      
      // expecting: 
      // 1
      // 0
      // 2
      
      // getting: 
      // 2
      // 1
      // 0
      

      Is it an issue? Or is it supposed that ordering should be resolved in application code?

        Activity

        Vladimir Razuvaev created issue -
        Hide
        Jonathan H. Wage added a comment -

        Hi, I think we should try and make it maintain the order. I will investigate.

        Show
        Jonathan H. Wage added a comment - Hi, I think we should try and make it maintain the order. I will investigate.
        Hide
        Jonathan H. Wage added a comment -

        I am not sure if we can control the order. Mongo doesn't give us anyway to do this. We use $pullAll and $pushAll operators currently.

        Can you log that test and paste what queries are executed on mongo when persisting?

        Show
        Jonathan H. Wage added a comment - I am not sure if we can control the order. Mongo doesn't give us anyway to do this. We use $pullAll and $pushAll operators currently. Can you log that test and paste what queries are executed on mongo when persisting?
        Hide
        Bulat Shakirzyanov added a comment - - edited

        I think that you should be getting 2,1,0 after the first flush as well, since we would not send an update after the second flush at all. We check whether the actual value in the collection changed or not, if you want the keys to be taken into account, you should use @Hash instead.

        Show
        Bulat Shakirzyanov added a comment - - edited I think that you should be getting 2,1,0 after the first flush as well, since we would not send an update after the second flush at all. We check whether the actual value in the collection changed or not, if you want the keys to be taken into account, you should use @Hash instead.
        Hide
        Vladimir Razuvaev added a comment -

        I have just checked. After first flush I get expected result: 0, 1, 2. And for second flush Doctrine Persister generates following update query:

        Array
        (
            [$pullAll] => Array
                (
                    [collection] => Array
                        (
                            [0] => Array
                                (
                                    [val] => 0
                                )
        
                            [1] => Array
                                (
                                    [val] => 1
                                )
        
                        )
        
                )
        
            [$pushAll] => Array
                (
                    [collection] => Array
                        (
                            [0] => Array
                                (
                                    [val] => 1
                                )
        
                            [1] => Array
                                (
                                    [val] => 0
                                )
        
                        )
        
                )
        
        )
        

        Then it sequentally executes two updates: 1 for $pullAll and then another for $pushAll (due to http://jira.mongodb.org/browse/SERVER-1050). As a result 1, 0 are pushed after 2 and we get 2,1,0.

        I think in many cases using $set would work as a workaround for this issue. But as far as I understand there is no way to hint Doctrine to use $set currently. Or is it possible somehow?

        Hash won't work in this case, because it doesn't preserve classes of embedded objects.

        Show
        Vladimir Razuvaev added a comment - I have just checked. After first flush I get expected result: 0, 1, 2. And for second flush Doctrine Persister generates following update query: Array ( [$pullAll] => Array ( [collection] => Array ( [0] => Array ( [val] => 0 ) [1] => Array ( [val] => 1 ) ) ) [$pushAll] => Array ( [collection] => Array ( [0] => Array ( [val] => 1 ) [1] => Array ( [val] => 0 ) ) ) ) Then it sequentally executes two updates: 1 for $pullAll and then another for $pushAll (due to http://jira.mongodb.org/browse/SERVER-1050 ). As a result 1, 0 are pushed after 2 and we get 2,1,0. I think in many cases using $set would work as a workaround for this issue. But as far as I understand there is no way to hint Doctrine to use $set currently. Or is it possible somehow? Hash won't work in this case, because it doesn't preserve classes of embedded objects.
        Hide
        Jonathan H. Wage added a comment -

        I think we can maybe allow you to force a collection to always use $set if you want to maintain ordering. The downside is it is much slower and not atomic. I don't know if it is a good idea, what do you think?

        Show
        Jonathan H. Wage added a comment - I think we can maybe allow you to force a collection to always use $set if you want to maintain ordering. The downside is it is much slower and not atomic. I don't know if it is a good idea, what do you think?
        Jonathan H. Wage made changes -
        Field Original Value New Value
        Fix Version/s 1.0.0BETA1 [ 10080 ]
        Affects Version/s 1.0.0ALPHA2 [ 10065 ]
        Affects Version/s 1.0.0ALPHA1 [ 10064 ]
        Hide
        Vladimir Razuvaev added a comment -

        Well, in my use case $set will work well, because embedded objects are small, there will be only 1-40 of them and saving operation is rare. I think this use-case is pretty usual, so $set will be definately useful.

        The alternative is to complicate application-code to resolve ordering on the client-side, which doesn't worth it in many cases. And people will use different workarounds as I did: currently I update all embedded objects with unique random token and Doctrine pulls/pushes them all back with valid ordering.

        Show
        Vladimir Razuvaev added a comment - Well, in my use case $set will work well, because embedded objects are small, there will be only 1-40 of them and saving operation is rare. I think this use-case is pretty usual, so $set will be definately useful. The alternative is to complicate application-code to resolve ordering on the client-side, which doesn't worth it in many cases. And people will use different workarounds as I did: currently I update all embedded objects with unique random token and Doctrine pulls/pushes them all back with valid ordering.
        Hide
        Jonathan H. Wage added a comment -

        In this commit http://github.com/doctrine/mongodb-odm/commit/7a9e0088153e1f3f7f9a0650f5f7461bbc41ec64 I made it so that you can control the strategy for persisting. The default value is pushPull but you can also specify set so the entire value is re $set each time:

        /** @Collection(strategy="set") */
        /** @ReferenceMany(strategy="set") */
        /** @EmbedMany(strategy="set") */
        
        Show
        Jonathan H. Wage added a comment - In this commit http://github.com/doctrine/mongodb-odm/commit/7a9e0088153e1f3f7f9a0650f5f7461bbc41ec64 I made it so that you can control the strategy for persisting. The default value is pushPull but you can also specify set so the entire value is re $set each time: /** @Collection(strategy= "set" ) */ /** @ReferenceMany(strategy= "set" ) */ /** @EmbedMany(strategy= "set" ) */
        Jonathan H. Wage made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        Vladimir Razuvaev added a comment -

        Discovered another related issue. There are situations with strategy "set", when ODM generates following update query:

        array (
          '$set' => array (
            'collection.1.val' => 'tmp',
            'collection' => array (
              0 => array (
                'val' => '1',
              ),
              1 => array (
                'val' => 'tmp',
              ),
              2 => array (
                'val' => '2',
              ),
            ),
          ),
        )
        

        E.g. when changing ordering of elements AND field value of any embedded document.
        This update will only persist new field value, but not elements reordering.

        Full Test Case:

        /** @Document(collection="tests", db="tests") */
        class doc
        {
            /** @Id */
            protected $id;
        
            /** @EmbedMany(targetDocument="emb", strategy="set", cascade="all") */
            protected $collection;
        
            /** @String */
            protected $tmp = ''; // force save
        
            function __construct($c) {$this->set($c);}
        
            function getId() {return $this->id;}
            function set($c) {$this->collection = $c;}
            function get() {return $this->collection;}
        }
        
        /** @EmbeddedDocument */
        class emb
        {
            /** @String */
            protected $val;
        
            function __construct($val) {$this->set($val);}
            function get() {return $this->val;}
            function set($val) {$this->val = $val;}
        }
        
        $collection = new ArrayCollection(array(
            new emb('0'),
            new emb('1'),
            new emb('2')
        ));
        
        // TEST CASE:
        $doc = new doc($collection);
        
        $dm->persist($doc);
        $dm->flush();
        
        // place element '0' after '1'
        $collection = new ArrayCollection(array(
            $collection[1],
            $collection[0],
            $collection[2]
        ));
        
        $doc->set($collection);
        
        // changing value together with reordering causes issue when saving:
        $collection[1]->set('tmp');
        
        $dm->persist($doc);
        $dm->flush();
        
        $dm->refresh($doc);
        
        foreach($doc->get() as $value) {
            echo $value->get() . "\n";
        }
        
        // expecting: 
        // 1
        // 0
        // 2
        
        // getting: 
        // 0
        // 1
        // 2
        
        Show
        Vladimir Razuvaev added a comment - Discovered another related issue. There are situations with strategy "set", when ODM generates following update query: array ( '$set' => array ( 'collection.1.val' => 'tmp', 'collection' => array ( 0 => array ( 'val' => '1', ), 1 => array ( 'val' => 'tmp', ), 2 => array ( 'val' => '2', ), ), ), ) E.g. when changing ordering of elements AND field value of any embedded document. This update will only persist new field value, but not elements reordering. Full Test Case: /** @Document(collection= "tests" , db= "tests" ) */ class doc { /** @Id */ protected $id; /** @EmbedMany(targetDocument= "emb" , strategy= "set" , cascade= "all" ) */ protected $collection; /** @ String */ protected $tmp = ''; // force save function __construct($c) {$ this ->set($c);} function getId() { return $ this ->id;} function set($c) {$ this ->collection = $c;} function get() { return $ this ->collection;} } /** @EmbeddedDocument */ class emb { /** @ String */ protected $val; function __construct($val) {$ this ->set($val);} function get() { return $ this ->val;} function set($val) {$ this ->val = $val;} } $collection = new ArrayCollection(array( new emb('0'), new emb('1'), new emb('2') )); // TEST CASE: $doc = new doc($collection); $dm->persist($doc); $dm->flush(); // place element '0' after '1' $collection = new ArrayCollection(array( $collection[1], $collection[0], $collection[2] )); $doc->set($collection); // changing value together with reordering causes issue when saving: $collection[1]->set('tmp'); $dm->persist($doc); $dm->flush(); $dm->refresh($doc); foreach($doc->get() as $value) { echo $value->get() . "\n" ; } // expecting: // 1 // 0 // 2 // getting: // 0 // 1 // 2
        Hide
        Vladimir Razuvaev added a comment -

        Reopening because of other related issue

        Show
        Vladimir Razuvaev added a comment - Reopening because of other related issue
        Vladimir Razuvaev made changes -
        Resolution Fixed [ 1 ]
        Status Resolved [ 5 ] Reopened [ 4 ]
        Hide
        Jonathan H. Wage added a comment -

        So the "'collection.1.val' => 'tmp'," part succeeds and the rest is ignored? Obviously the 'collection.1.val' => 'tmp', part should not be there.

        Show
        Jonathan H. Wage added a comment - So the "'collection.1.val' => 'tmp'," part succeeds and the rest is ignored? Obviously the 'collection.1.val' => 'tmp', part should not be there.
        Hide
        Jonathan H. Wage added a comment -

        Hi, to make things easier would you mind making the test cases in the Doctrine test suite? Every test case you provide is great and it works but I have to spend a few minutes modifying the code and putting it into a test case in phpunit.

        Show
        Jonathan H. Wage added a comment - Hi, to make things easier would you mind making the test cases in the Doctrine test suite? Every test case you provide is great and it works but I have to spend a few minutes modifying the code and putting it into a test case in phpunit.
        Hide
        Jonathan H. Wage added a comment -

        Why are you expecting 1, 0, 2? Wouldn't the change to 'tmp' need to be persisted as well?

        Show
        Jonathan H. Wage added a comment - Why are you expecting 1, 0, 2? Wouldn't the change to 'tmp' need to be persisted as well?
        Hide
        Jonathan H. Wage added a comment -

        Fixed here http://github.com/doctrine/mongodb-odm/commit/485fbdfb7f85ecb290530ea0fc9c55a6dca91e19

        I think the expected values and order would be "1, tmp, 2"

        Show
        Jonathan H. Wage added a comment - Fixed here http://github.com/doctrine/mongodb-odm/commit/485fbdfb7f85ecb290530ea0fc9c55a6dca91e19 I think the expected values and order would be "1, tmp, 2"
        Jonathan H. Wage made changes -
        Status Reopened [ 4 ] Resolved [ 5 ]
        Fix Version/s 1.0.0BETA2 [ 10092 ]
        Fix Version/s 1.0.0BETA1 [ 10080 ]
        Resolution Fixed [ 1 ]
        Hide
        Vladimir Razuvaev added a comment -

        Yeah, sorry, expecting "1, tmp, 2" of course.

        As for tests - I'll setup Doctrine tests on my side. How should I deliver test cases then? Via github? Or just copy-paste phpunit tests here?

        Show
        Vladimir Razuvaev added a comment - Yeah, sorry, expecting "1, tmp, 2" of course. As for tests - I'll setup Doctrine tests on my side. How should I deliver test cases then? Via github? Or just copy-paste phpunit tests here?

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

          People

          • Assignee:
            Jonathan H. Wage
            Reporter:
            Vladimir Razuvaev
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: