Persisting changes ordering of elements in embedded collection in some cases

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

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?

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.

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.

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?

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.

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") */

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

Reopening because of other related issue

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

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.

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

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

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

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?