Doctrine\ODM\MongoDB\Persisters\CollectionPersister::deleteCollections don't remove all necessary elements

[TvitiDev]

Bug Report

Q	A
BC Break	no
Version	1.3.7

Summary

In upgrading version up to 1.3.* in DocumentPersister::handleCollections() method you made optimisation. Now you update the list collections in method CollectionPersister::updateAll() and method CollectionPersister::deleteCollections() remove all elements as one "$unset". But you lost the some paths for remove.

Current behavior and how to reproduce

For example, I have next document:

{
  "_id" : ObjectId("5ed602e89bfe8563bd61c302"),
  "property" : [
    {
      "value" : 100,
      "items" : [
        {
          "value" : 1,
          "is_flag" : false
        },
        {
          "value" : 2,
          "is_flag" : false
        },
        {
          "value" : 3,
          "is_flag" : false
        }
      ]
    },
    {
      "value" : 101,
      "items" : [
        {
          "value" : 1,
          "is_flag" : false
        },
        {
          "value" : 2,
          "is_flag" : false
        },
        {
          "value" : 3,
          "is_flag" : false
        }
      ]
    }
  ],
  "property_old" : [
    {
      "value" : 200,
      "items" : [
        {
          "value" : 1,
          "is_flag" : false
        },
        {
          "value" : 2,
          "is_flag" : false
        },
        {
          "value" : 3,
          "is_flag" : false
        }
      ]
    }
  ]
}

me need remove:

'property.1',
'property.0.items.2',
'property_old.0'

I expect, what your code remove this element, but your code remove only 'property.1', because variable $paths (line 368 of file CollectionPersister.php) contains next elements:
before execute method excludeSubPaths():

'property',
'property.0.items',
'property_old'

after execute method excludeSubPaths():

'property'

This is not correct. You lost 2 path for remove and will remove only 'property.1'

Expected behavior

I see 2 errors:

1. Incorrect list paths before execute method excludeSubPaths(), he should contains:

'property.1',
'property.0.items.2',
'property_old.0'

but contains:

'property',
'property.0.items',
'property_old'

2. If me need totally remove property "property" and remove "property_old.0" - method excludeSubPaths() return only path "property", because strpos('property_old.0', 'property') === 0 is true, but "property_old.0" not sub-path 'property'. Maybe need compare strpos($paths[$i], end($uniquePaths) . '.') === 0, because sub-path should have point?

[malarzm]

Please provide a failing test case as that's too much info in the issue to digest :) You can mimic how ticket related tests are written: https://github.com/doctrine/mongodb-odm/tree/master/tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket

In upgrading version up to 1.3.*

From which version were you updating?

[TvitiDev]

We updated up to version 1.3.7 from version 1.2.1, but this code also is available in version 2.*
Ok, i'm will add test

[malarzm]

Yes but the question in which version it worked differently :)

[TvitiDev]

In this pull #1948 was changed logic remove elements. Earlier items are removed step by step, now items removed at one step. This is good, but they made a mistake during optimization

[TvitiDev]

This my mini test. In branch 1.2.x successfully working. In branch 1.3.x don't working.

<?php

namespace Doctrine\ODM\MongoDB\Tests\Functional\Ticket;

use Doctrine\Common\Collections\ArrayCollection;
use Doctrine\ODM\MongoDB\Mapping\Annotations as ODM;
use Doctrine\ODM\MongoDB\Tests\BaseTest;

class GH2195Test extends BaseTest
{
    public $id;

    public function setUp()
    {
        parent::setUp();

        $this->id = (string) new \MongoId();

        $document = new GH2195MainDocument();
        $document->id = $this->id;

        $document->property[] = new GH2195Level1(100, [[1, true], [2, true], [3, true]]);
        $document->property[] = new GH2195Level1(101, [[1, true], [2, true], [3, true]]);
        $document->property[] = new GH2195Level1(102, [[1, true], [2, true], [3, true]]);

        $document->property_old[] = new GH2195Level1(100, [[1, true], [2, true], [3, true]]);
        $document->property_old[] = new GH2195Level1(101, [[1, true], [2, true], [3, true]]);
        $document->property_old[] = new GH2195Level1(102, [[1, true], [2, true], [3, true]]);

        $this->dm->persist($document);
        $this->dm->flush();
        $this->dm->clear();
    }

    /**
     * This demonstration what if name variable have a common part - your code remove second path, because it considers it as a sub-path
     */
    public function testFirstSuccessTest()
    {
        /** @var GH2195MainDocument $document */
        $document = $this->dm->find(GH2195MainDocument::class, $this->id);

        $this->assertNotNull($document);

        $document->property->remove(1);
        $document->property_old->remove(2);

        $this->dm->flush();
        $this->dm->refresh($document);

        $this->assertEquals(2, $document->property->count(), 'Should was deleted 2th element');
        $this->assertEquals(2, $document->property_old->count(), 'Should was deleted 3th element');
    }

    /**
     * This demonstration what sub-path basically not correctly reduced to unique
     */
    public function testSecondSuccessTest()
    {
        /** @var GH2195MainDocument $document */
        $document = $this->dm->find(GH2195MainDocument::class, $this->id);

        $this->assertNotNull($document);

        $document->property_old->offsetGet(1)->items->remove(0);
        $document->property_old->remove(2);

        $this->dm->flush();
        $this->dm->refresh($document);

        $this->assertEquals(2, $document->property_old->offsetGet(1)->items->count(), 'Should was deleted 1th element');
        $this->assertEquals(2, $document->property_old->count(), 'Should was deleted 3th element');
    }
}

/** @ODM\Document */
class GH2195MainDocument
{
    /**
     * @ODM\Id
     * @var string
     */
    public $id;

    /**
     * @ODM\EmbedMany(targetDocument=GH2195Level1::class)
     * @var ArrayCollection
     */
    public $property;

    /**
     * @ODM\EmbedMany(targetDocument=GH2195Level1::class)
     * @var ArrayCollection
     */
    public $property_old;
}

/** @ODM\EmbeddedDocument */
class GH2195Level1
{
    /** @ODM\Field(type="int") */
    public $value;

    /** @ODM\EmbedMany(targetDocument=GH2195Level2::class) */
    public $items;

    /**
     * Level1 constructor.
     * @param int $value
     * @param array $items
     */
    public function __construct($value, $items)
    {
        $this->value   = $value;
        foreach ($items as list($v, $f))
        {
            $this->items[] = new GH2195Level2($v, $f);
        }
    }
}

/** @ODM\EmbeddedDocument */
class GH2195Level2
{
    /** @ODM\Field(type="int") */
    public $value;

    /** @ODM\Field(type="boolean") */
    public $is_flag;

    /**
     * Level2 constructor.
     * @param int $value
     * @param bool $is_flag
     */
    public function __construct($value, $is_flag)
    {
        $this->value   = $value;
        $this->is_flag = $is_flag;
    }
}

[malarzm]

@TvitiDev thanks for the test case! While this won't fix the issue itself, is there any reason you're not using setArray strategy for your collections? The test you provided passes then and maybe could continue the update.