[MODM-29] Persisting changes ordering of elements in embedded collection in some cases Created: 27/Jul/10  Updated: 19/Aug/10  Resolved: 18/Aug/10

Status: Resolved
Project: Doctrine MongoDB ODM
Component/s: Persister
Affects Version/s: 1.0.0ALPHA2
Fix Version/s: 1.0.0BETA2

Type: Bug Priority: Major
Reporter: Vladimir Razuvaev Assignee: Jonathan H. Wage
Resolution: Fixed Votes: 0
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?



 Comments   
Comment by Jonathan H. Wage [ 27/Jul/10 ]

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

Comment by Jonathan H. Wage [ 27/Jul/10 ]

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?

Comment by Bulat Shakirzyanov [ 27/Jul/10 ]

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.

Comment by Vladimir Razuvaev [ 28/Jul/10 ]

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.

Comment by Jonathan H. Wage [ 30/Jul/10 ]

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?

Comment by Vladimir Razuvaev [ 31/Jul/10 ]

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.

Comment by Jonathan H. Wage [ 07/Aug/10 ]

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") */
Comment by Vladimir Razuvaev [ 17/Aug/10 ]

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
Comment by Vladimir Razuvaev [ 18/Aug/10 ]

Reopening because of other related issue

Comment by Jonathan H. Wage [ 18/Aug/10 ]

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

Comment by Jonathan H. Wage [ 18/Aug/10 ]

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.

Comment by Jonathan H. Wage [ 18/Aug/10 ]

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

Comment by Jonathan H. Wage [ 18/Aug/10 ]

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

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

Comment by Vladimir Razuvaev [ 19/Aug/10 ]

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?

Generated at Sat Sep 20 22:05:09 UTC 2014 using JIRA 6.2.3#6260-sha1:63ef1d6dac3f4f4d7db4c1effd405ba38ccdc558.