The "unique" option does not play well together with relations

[kgilden]

Hi, thanks for all the work on this awesome package!

Similar to https://github.com/nelmio/alice/issues/722 and https://github.com/nelmio/alice/issues/888, I'm getting exceptions about new entities being found via relationships.

Suppose the following set-up.

Foo:
  dummy1:
    foo: bar
    sibling (unique): '@dummy*'
  dummy2:
    foo: baz

I get the following exception from Doctrine.

A new entity was found through the relationship 'Foo#sibling' that was not configured to cascade persist operations for entity: Foo. To solve this issue: Either explicitly call EntityManager#persist() on this unknown entity or configure cascade persist this association in the mapping for example @ManyToOne(..,cascade={"persist"}).                                

Exception trace:
 () at /usr/share/nginx/html/vendor/doctrine/orm/lib/Doctrine/ORM/ORMInvalidArgumentException.php:105
 Doctrine\ORM\ORMInvalidArgumentException::newEntitiesFoundThroughRelationships() at /usr/share/nginx/html/vendor/doctrine/orm/lib/Doctrine/ORM/UnitOfWork.php:3443
 Doctrine\ORM\UnitOfWork->assertThatThereAreNoUnintentionallyNonPersistedAssociations() at /usr/share/nginx/html/vendor/doctrine/orm/lib/Doctrine/ORM/UnitOfWork.php:362
 Doctrine\ORM\UnitOfWork->commit() at /usr/share/nginx/html/vendor/doctrine/orm/lib/Doctrine/ORM/EntityManager.php:359
 Doctrine\ORM\EntityManager->flush() at /usr/share/nginx/html/var/cache/dev/ContainerMnyzq5c/EntityManager_9a5be93.php:125
 EntityManager_9a5be93->flush() at /usr/share/nginx/html/vendor/theofidry/alice-data-fixtures/src/Bridge/Doctrine/Persister/ObjectManagerPersister.php:103
 Fidry\AliceDataFixtures\Bridge\Doctrine\Persister\ObjectManagerPersister->flush() at /usr/share/nginx/html/vendor/theofidry/alice-data-fixtures/src/Loader/PersisterLoader.php:91
 Fidry\AliceDataFixtures\Loader\PersisterLoader->load() at /usr/share/nginx/html/vendor/theofidry/alice-data-fixtures/src/Loader/PurgerLoader.php:119
 Fidry\AliceDataFixtures\Loader\PurgerLoader->load() at /usr/share/nginx/html/vendor/theofidry/alice-data-fixtures/src/Loader/FileResolverLoader.php:73
 Fidry\AliceDataFixtures\Loader\FileResolverLoader->load() at /usr/share/nginx/html/vendor/hautelook/alice-bundle/src/Loader/DoctrineOrmLoader.php:155
 Hautelook\AliceBundle\Loader\DoctrineOrmLoader->loadFixtures() at /usr/share/nginx/html/vendor/hautelook/alice-bundle/src/Loader/DoctrineOrmLoader.php:125
 Hautelook\AliceBundle\Loader\DoctrineOrmLoader->load() at /usr/share/nginx/html/vendor/hautelook/alice-bundle/src/Console/Command/Doctrine/DoctrineOrmLoadDataFixturesCommand.php:137
 Hautelook\AliceBundle\Console\Command\Doctrine\DoctrineOrmLoadDataFixturesCommand->execute() at /usr/share/nginx/html/vendor/symfony/symfony/src/Symfony/Component/Console/Command/Command.php:255
 Symfony\Component\Console\Command\Command->run() at /usr/share/nginx/html/vendor/symfony/symfony/src/Symfony/Component/Console/Application.php:971
 Symfony\Component\Console\Application->doRunCommand() at /usr/share/nginx/html/vendor/symfony/symfony/src/Symfony/Bundle/FrameworkBundle/Console/Application.php:86
 Symfony\Bundle\FrameworkBundle\Console\Application->doRunCommand() at /usr/share/nginx/html/vendor/symfony/symfony/src/Symfony/Component/Console/Application.php:248
 Symfony\Component\Console\Application->doRun() at /usr/share/nginx/html/vendor/symfony/symfony/src/Symfony/Bundle/FrameworkBundle/Console/Application.php:74
 Symfony\Bundle\FrameworkBundle\Console\Application->doRun() at /usr/share/nginx/html/vendor/symfony/symfony/src/Symfony/Component/Console/Application.php:148
 Symfony\Component\Console\Application->run() at /usr/share/nginx/html/bin/console:28

I believe the issue is caused by UniqueValue cloning its value. A copy is made of an existing entity and now Doctrine sees that there is a new entity to be persisted rather than using the existing one, which was the intended purpose.

I can reproduce this with the following failing test case added to LoaderIntegrationTest.

    public function testUniqueKeepsSameInstances()
    {
        $set = $this->loader->loadData([
            stdClass::class => [
                'dummy1' => [
                    'foo' => 'bar',
                    'sibling (unique)' => '@dummy*',
                ],
                'dummy2' => [
                    'foo' => 'baz',
                ]
            ]
        ]);

        $this->assertCount(2, $set->getObjects());
        list('dummy1' => $dummy1, 'dummy2' => $dummy2) = $set->getObjects();

        $this->assertNotSame($dummy1, $dummy2);
        $this->assertSame($dummy2, $dummy1->sibling);
    }

May I ask why is deep copying of values required (added in this PR)? Could we get by with not deep copying? I can confirm that removing these two deep_clone uses from UniqueValue eliminates the problem.

My project is using the latest relevant dependencies.

fzaninotto/faker              v1.8.0
hautelook/alice-bundle        v2.3.0
myclabs/deep-copy             1.8.1
nelmio/alice                  v3.5.3
theofidry/alice-data-fixtures v1.1.1

Thanks, Kristen

[theofidry]

Hm I see, thanks for the detailed report and managing to point the exact issue.

The original idea behind the deep cloning was to achieve real immutability. There was various ideas of using the FixtureSet relying on it being an immutable "tree". It was also a wish of mine to push for a lot of small classes and heavily relying on testing, immutability and final classes. alice 2.x was design-wise much simpler, but very flawed and impossible to debug. 3.x is certainly way more complex in terms of design/architecture (for a similar feature-set), but at least it more testable, extendable and deterministic.

There's however been a lot of changes and I've not managed to free as much time neither as much need to really take advantage of the whole design. So I think regarding the immutability of the values there is some stuff to reconsider. I wouldn't go as far as removing the deep cloning everywhere as I'm certain it's being used in some places, but at the very least I don't think non definition values, such as injected & resolved objects should be deep cloned. And this should I think solve your issue

[kgilden]

Right, thanks!

So if I understood correctly, a potential fix would be checking whether the passed value is some instance of the alice package or a foreign object? Basically, the __clone method of each alice class that can be passed as value should recursively clone their wrapped alice class, but not a foreign object?

I did, however manage to find a workaround by going with a custom provider and enforcing uniqueness there by keeping track of which elements have been used already. Adding it here for reference, if someone else runs into the same problem.

use Faker\Generator;

final class UniqueElementProvider
{
    private $generator;
    private $usedElements = [];

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

    public function uniqueElements(array $elements, int $count = 1): array
    {
        $this->usedElements = [];

        $unusedElements = array_udiff($elements, $this->usedElements, function ($a, $b) {
            return spl_object_hash($a) <=> spl_object_hash($b);
        });

        $chosenElements = $this->generator->randomElements($unusedElements, $count);
        $this->usedElements = array_merge($this->usedElements, $chosenElements);

        return $chosenElements;
    }
}

[theofidry]

So if I understood correctly, a potential fix would be checking whether the passed value is some instance of the alice package or a foreign object?

Yes

Basically, the __clone method of each alice class that can be passed as value should recursively clone their wrapped alice class, but not a foreign object?

I would be a bit simpler and just look up the ValueInterface children classes and only do the deep clone if not an instance of ValueInterface

[jzecca]

I can confirm the issue is still there.

fzaninotto/faker              v1.9.1
hautelook/alice-bundle        v2.7.2
myclabs/deep-copy             1.9.5
nelmio/alice                  v3.6.0
theofidry/alice-data-fixtures v1.1.2

I can set up a reproduction project if anyone needs one, feel free to ask :+1:

[theofidry]

@jzecca we have a reproducer here, but what we are missing right now is implementing this fix :)

[ferdynator]

I just ran into the same issue. Fix by @kgilden worked for me with slight modification.

What I wanted to archive:

Out of 500 fixtures of doctrine entities, pick 5 random Elements as a relation. Because the relationship is unique, no single one of the 500 fixtures may be picked again.

I tried this:

App\Entity\A:
  a{1..100}:
    b (unique): '5x @b*'

What worked for me:

Not clearing the usedElements in @kgilden example was the solution. Removing this line:

$this->usedElements = [];

The result is:

App\Entity\A:
  a{1..100}:
    b: '<uniqueElements(@b{1..500}, 5)>'

I would be interested in creating a PR for this but obviously the use-case might differ. Is there an update on deep_clone? Should we go with an extra parameter to switch between locally and globally unique?

[florianajir]

Ran into a similar issue a scratched my head for a while before figuring what caused this behaviour. 3 years later have you figured how to manage this bug for a next release ?

[theofidry]

@florianajir there is several possible solutions, one proposed above via a custom provider can be implemented on your own as a workaround.

A more definite and in-core solution is also possible, but no one did a PR for it yet.