corrupt custom collection on persist is created

[g13013]

Q	A
Version	2.0.5
Support Question	yes
Bug Report	for me yes

Support Question

I have implemented a custom collection for a field that extends from ArrayCollection

class MyCollection extends ArrayCollection
{
    public function customFunction()
}

class MyDoc {
    /**
     * @ReferenceMany(
     *     collectionClass=MyCollection::class,
     *     targetDocument=Document::class,
     *     orphanRemoval=true,
     *     strategy="atomicSetArray",
     *     cascade={"persist"}
     * )
     */
    private $collection = null;
}

The save operation works without problem, however, when trying to load the saved entity, the collection field loads with ArrayCollection instead of MyCollection, this makes calls to MyCollection::customFunction fail, is this the correct behavior ?, I would expect the collectionClass to be instantiated on save as well as on data fetch

also the generated wrapper persistent class make use of \Doctrine\ODM\MongoDB\PersistentCollection\PersistentCollectionInterface which generates a notice

1x: The "Doctrine\ODM\MongoDB\PersistentCollection\PersistentCollectionInterface" interface is considered internal. It may change without further notice. You should not use it fromPersistentCollections\MyCollectionPersistent`

[malarzm]

That sounds weird, we have tests covering that you get a correct instance of a collection: https://github.com/doctrine/mongodb-odm/blob/b852d476d521d60860530ec7811d92d71333a116/tests/Doctrine/ODM/MongoDB/Tests/Functional/CustomCollectionsTest.php#L83 I'm afraid that without a failing test case we won't be able to help you.

also the generated wrapper persistent class make use of \Doctrine\ODM\MongoDB\PersistentCollection\PersistentCollectionInterface which generates a notice

PersistentCollectionInterface is indeed marked as @internal to warn people to not implement it themselves. You may need to turn off the notice for code generated by Doctrine. Out of curiosity, what tool is reporting usage of @internal stuff?

[alcaeus]

Out of curiosity, what tool is reporting usage of @internal stuff?

This looks like it's coming from the DebugClassLoader in Symfony, which checks the namespace on internal classes. This causes the warning. This can be ignored for the time being, there's nothing we can do to prevent this message.

[g13013]

@alcaeus indeed

[g13013]

It seems that the problem occurs on persistence as long as it involves the same instance of the documentManager. if I instantiate a fresh instance of DocumentManager the problem disappears.

In the test below $dn->persist($parent) fills $children with a proxied (Persistent) version of ChildrenCollection since $this->assertInstanceOf(ChildrenCollection::class, $parent->getChildren()); passes, but under the hood the proxied class (Persistent) reference an ArrayCollection instead of ChildrenCollection

If I use the same instance of DocumentManager to fetch the same document, since it returns data from the cache, the same problem occurs.

I would expect the same behavior here.

here is a failing test case illustrating the failing scenario

<?php

// ParentClass.php

declare(strict_types=1);

namespace App\Document;

use Doctrine\ODM\MongoDB\Mapping\Annotations\HasLifecycleCallbacks;
use Doctrine\ODM\MongoDB\Mapping\Annotations\Document;
use Doctrine\ODM\MongoDB\Mapping\Annotations\Id;
use Doctrine\ODM\MongoDB\Mapping\Annotations\ReferenceMany;

/**
 * @Document(collection="parents")
 * @HasLifecycleCallbacks
 */
Class ParentClass
{

    /**
     * @Id(strategy="NONE")
     *
     * @var string
     */
    private string $id = "__ID__";

    /**
     * @ReferenceMany(
     *     collectionClass=ChildrenCollection::class,
     *     targetDocument=Child::class,
     *     orphanRemoval=true,
     *     strategy="atomicSetArray",
     *     cascade={"persist"}
     * )
     */
    private $children = null;

    /**
     * @return string
     */
    public function getId(): string
    {
        return $this->id;
    }

    /**
     * @param null $children
     */
    public function setChildren($children): void
    {
        $this->children = $children;
    }

    /**
     * @return null
     */
    public function getChildren()
    {
        return $this->children;
    }
}

<?php
// Child.php
declare(strict_types=1);

namespace App\Document;

use Doctrine\ODM\MongoDB\Mapping\Annotations\Document;
use Doctrine\ODM\MongoDB\Mapping\Annotations\Field;
use Doctrine\ODM\MongoDB\Mapping\Annotations\HasLifecycleCallbacks;
use Doctrine\ODM\MongoDB\Mapping\Annotations\Id;

/**
 * @Document(collection="children")
 * @HasLifecycleCallbacks
 * */
Class Child
{
    /**
     * @Id(strategy="NONE")
     *
     * @var string
     */
    private string $id;

    public function __construct($id)
    {
        $this->id = $id;
    }

    /**
     * @Field()
     *
     * @return string
     */
    public function getId(): string
    {
        return $this->id;
    }

    /**
     * @return string
     */
    public function getName(): string
    {
        return "child $this->id";
    }
}

<?php
// ChildrenCollection.php

declare(strict_types=1);

namespace App\Document;

use Doctrine\Common\Collections\ArrayCollection;
use function array_map;

class ChildrenCollection extends ArrayCollection
{
    public function getNames() : array
    {
        return array_map(fn($item) => $item->getName(), $this->getValues());
    }
}

<?php
// Collection_Test.php

declare(strict_types=1);

namespace App\Document;

use Doctrine\ODM\MongoDB\DocumentManager;
use Symfony\Bundle\FrameworkBundle\Test\KernelTestCase;
use Throwable;

class Collection_Test extends KernelTestCase
{
    public function testInsertParent()
    {

        $parent = new ParentClass();
        $parent->setChildren([new Child("one"), new Child("two")]);

        $dm = $this->getDocumentManager();
        $dm->persist($parent);
        $dm->flush();
        $fail = false;
        $this->assertInstanceOf(ChildrenCollection::class, $parent->getChildren());
        try {
            $fields = $parent->getChildren()->getNames();
            $this->assertEquals(["child one", "child two"], $fields);
        } catch (Throwable $e) {
            //postpone the failure to test on fetch with the same instance of DocumentManager
            $fail = true;
        }

        $parent = $dm->find(ParentClass::class, ["id" => '__ID__']);
        $this->assertInstanceOf(ChildrenCollection::class, $parent->getChildren());
        try {
            $fields = $parent->getChildren()->getNames();
            $this->assertEquals(["child one", "child two"], $fields);
        } catch (Throwable $e) {
            $this->fail($e->getMessage());
        }

        if ($fail) {
            $this->fail($e->getMessage());
        }
    }

    public function testReadParent()
    {
        $dm = $this->getDocumentManager();
        $parent = $dm->find(ParentClass::class, '__ID__');
        $this->assertInstanceOf(ChildrenCollection::class, $parent->getChildren());
        try {
            $fields = $parent->getChildren()->getNames();
            $this->assertEquals(["child one", "child two"], $fields);
        } catch (Throwable $e) {
            $this->fail($e->getMessage());
        }
    }

    public function getDocumentManager() : DocumentManager {
        self::bootKernel([]);
        return static::$container->get('doctrine_mongodb')->getManager();
    }
}

[malarzm]

Please provide a failing test we can run ourselves, i.e. without Symfony. You can take a look at our ticket-specific tests since they're easiest to write: https://github.com/doctrine/mongodb-odm/tree/master/tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket. A PR with a failing test case is the best course of action.

Besides:

$this->assertInstanceOf(ChildrenCollection::class, $parent->getChildren()); passes, but under the hood the proxied class (Persistent) reference an ArrayCollection instead of ChildrenCollection

This makes no sense to me, if the test passes then the collection is of correct type and you can call your methods on the collection. Also I'm not sure what references what, everything under the hood shouldn't be consuming code's concern.

$parent->setChildren([new Child("one"), new Child("two")]);

Why are you using plain array when you're expecting a custom collection in the document?

[alcaeus]

To elaborate on the last paragraph in the previous comment, you should always use collections internally. Allowing a setChildren call to replace the entire collection is bound to cause problems, e.g. an inconsistent type (as evidenced by your test), as well as inefficient writes as this will always be treated as a complete clear followed by a complete insert.

Your ParentClass should probably look like this (unrelated functionality omitted)

class ParentClass
{
    private ChildrenCollection $children;

    public function __construct()
    {
        $this->children = new ChildrenCollection();
    }

    public function getChildren(): ChildrenCollection
    {
        return $this->children;
    }
}

Not exposing a setter will prevent users from completely overriding the collection, which shouldn't happen anyways. You can expose a method to add elements, but this normally isn't necessary as you can call $parent->getChildren()->add(). The only way to lock this down is by not exposing the $children collection at all.

[g13013]

$parent->setChildren([new Child("one"), new Child("two")]); Why are you using plain array when you're expecting a custom collection in the document?

it seems that replacing with the following solves the problem

$parent->setChildren(new ChildrenCollection([new Child("one"), new Child("two")]));

but still, the persist function seems to incapsulate the array of Child with an instance of AppDocumentChildrenCollectionPersistent class with ArrayCollection instead of ChildrenCollection.

I think that persist should either fail if children is not set to the expected ChildrenCollection or support arrays but implement the correct behavior by instantiating the inner coll property with ChildrenCollection instead of ArrayCollection

[g13013]

This makes no sense to me, if the test passes then the collection is of correct type and you can call your methods on the collection. Also I'm not sure what references what, everything under the hood shouldn't be consuming code's concern.

it doesn't make sens to me neither but

$this->assertInstanceOf(ChildrenCollection::class, $parent->getChildren()); // passes
$parent->getChildren()->getNames(); // fails with error Call to undefined method Doctrine\Common\Collections\ArrayCollection::getNames()

[malarzm]

Most probably this is caused due to this line: https://github.com/doctrine/mongodb-odm/blob/9f19a49c39f7fdd64d9534690d9d2e58b36c8427/lib/Doctrine/ODM/MongoDB/UnitOfWork.php#L565 as it's the only place in ODM (beside collection factory) that instantiates an ArrayCollection. I believe that disallowing arrays in this place at all would be the best course of action but this is way too strict for a minor release.

[g13013]

Good that we've found the source of the problem

@malarzm @alcaeus thank you for your support

[stale[bot]]

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in a week if no further activity occurs. Thank you for your contributions.

[g13013]

should not be closed

[alcaeus]

I'm scheduling this for a fix in the next patch release.

On a different note, I believe it may be worth deprecating this automatic type conversion. For a clean API, you should always initialise your relationships to the correct collection class instead of having a mix of array and collection instances.