Repository: doctrine/orm (php)

Issue:
Extra update for self-referencing Many-To-One with NONE generator strategy during persist
### Bug Report

<!-- Fill in the relevant information below to help triage your issue. -->

|    Q        |   A
|------------ | ------
| BC Break    | no
| Version     | 2.6.4

#### Summary

Using a self-referencing entity with a Many-To-One relationship set during the `__construct()` call with a `NONE` strategy for id generation leads to two queries:
- first an INSERT
- then an UPDATE for the relationship

#### Current behavior

- this requires the relationship related column to accept `NULL` for the first `INSERT`

`[2019-10-23 16:04:21] doctrine.DEBUG: INSERT INTO organization (id, legal_parent_id) VALUES (?, ?, ?, ?, ?, ?, ?) {"1":"[object] (Ramsey\\Uuid\\Uuid: \"c68555b0-f5ae-11e9-914e-38baf82a0624\")","2":null} []`

- this leads to a useless `UPDATE` query

`[2019-10-23 16:04:21] doctrine.DEBUG: UPDATE organization SET legal_parent_id = ? WHERE id = ? ["[object] (Ramsey\\Uuid\\Uuid: \"c68555b0-f5ae-11e9-914e-38baf82a0624\")","[object] (Ramsey\\Uuid\\Uuid: \"c68555b0-f5ae-11e9-914e-38baf82a0624\")"] []`

#### How to reproduce

```sql

CREATE TABLE `organization` (
  `id` binary(16) NOT NULL COMMENT '(DC2Type:uuid_binary_ordered_time)',
  `legal_parent_id` BINARY(16) NOT NULL COMMENT '(DC2Type:uuid_binary_ordered_time)',
  PRIMARY KEY (`id`),
  CONSTRAINT FK_C1EE637C9EB3F521 FOREIGN KEY (legal_parent_id) REFERENCES organization (id)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4;

```

```php
<?php

declare(strict_types=1);

namespace App\Entity;

use Doctrine\Common\Collections\ArrayCollection;
use Doctrine\Common\Collections\Collection;
use Doctrine\ORM\Mapping as ORM;

/**
 * An Organization
 * @ORM\Entity(repositoryClass="App\Repository\OrganizationRepository")
 */
class Organization
{
    public function __construct()
    {
        $factory = clone \Ramsey\Uuid\Uuid::getFactory();
        $codec = new \Ramsey\Uuid\Codec\OrderedTimeCodec(
            $factory->getUuidBuilder()
        );
        $factory->setCodec($codec);
		$this->id = $factory->uuid1();
        $this->legalParent = $this;
        parent::__construct();
    }

    /**
     * Unique ID of the entity
     *
     * @ORM\Id()
     * @ORM\GeneratedValue(strategy="NONE")
     * @ORM\Column(type="uuid_binary_ordered_time", unique=true)
     */
    private $id;

    public function getId(): ?string
    {
        return $this->id->__toString();
    }

    /**
     * @var self
     * @ORM\ManyToOne(targetEntity="App\Entity\Organization")
     * @ORM\JoinColumn(fieldName="legal_parent_id", referencedColumnName="id", nullable=false)
     */
    protected $legalParent;

    public function getLegalParent(): self
    {
        return $this->legalParent;
    }

    public function setLegalParent(self $legalParent): self
    {
        $this->legalParent = $legalParent;

        return $this;
    }
}

```

#### Expected behavior

- Only the `INSERT` query with `legal_parent_id` not `NULL`

`[2019-10-23 16:04:21] doctrine.DEBUG: INSERT INTO organization (id, legal_parent_id) VALUES (?, ?, ?, ?, ?, ?, ?) {"1":"[object] (Ramsey\\Uuid\\Uuid: \"c68555b0-f5ae-11e9-914e-38baf82a0624\")","2":[object] (Ramsey\\Uuid\\Uuid: \"c68555b0-f5ae-11e9-914e-38baf82a0624\")} []`

#### Possible solution

This comes from `vendor/doctrine/orm/lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php`

![image](https://user-images.githubusercontent.com/3177556/67414724-f3400e00-f5c3-11e9-9d4b-abb4bc298bf0.png)

We could check if `spl_object_hash($newVal) === spl_object_hash($entity)` and if `ClassMetadataInfo::GENERATOR_TYPE_NONE === $this->class->generatorType`. If yes, then do not set `$newVal = null;` and do not schedule extra update.

I can provide a PR if you want

Pull request description:
Make it possible to have non-NULLable self-referencing associations when using application-provided IDs
This change improves scheduling of extra updates in the `BasicEntityPersister`.

Extra updates can be avoided when
* the referred-to entity has already been inserted during the current insert batch/transaction and the post-insert (database) ID is already available in the entity
* we have a self-referencing entity with application-provided ID values (the `NONE` generator strategy).

As a corollary, with this change applications that provide their own IDs can define self-referencing associations as not NULLable.

One caveat, though: In the absence of entity-level commit ordering (#10547), it is not guaranteed that entities with self-references (at the class level) will be inserted in a suitable order. The order depends on the sequence in which the entities were added with `persist()`. 

Fixes #7877, closes #7882.

Co-authored-by: Sylvain Fabre <sylvain.fabre@assoconnect.com>

Patch:
diff --git a/lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php b/lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php
index b84fcb9a6..0fbde8bac 100644
--- a/lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php
+++ b/lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php
@@ -266,7 +266,7 @@ class BasicEntityPersister implements EntityPersister
         $stmt      = $this->conn->prepare($this->getInsertSQL());
         $tableName = $this->class->getTableName();
 
-        foreach ($this->queuedInserts as $entity) {
+        foreach ($this->queuedInserts as $key => $entity) {
             $insertData = $this->prepareInsertData($entity);
 
             if (isset($insertData[$tableName])) {
@@ -291,9 +291,16 @@ class BasicEntityPersister implements EntityPersister
             if ($this->class->requiresFetchAfterChange) {
                 $this->assignDefaultVersionAndUpsertableValues($entity, $id);
             }
-        }
 
-        $this->queuedInserts = [];
+            // Unset this queued insert, so that the prepareUpdateData() method knows right away
+            // (for the next entity already) that the current entity has been written to the database
+            // and no extra updates need to be scheduled to refer to it.
+            //
+            // In \Doctrine\ORM\UnitOfWork::executeInserts(), the UoW already removed entities
+            // from its own list (\Doctrine\ORM\UnitOfWork::$entityInsertions) right after they
+            // were given to our addInsert() method.
+            unset($this->queuedInserts[$key]);
+        }
     }
 
     /**
@@ -671,10 +678,30 @@ class BasicEntityPersister implements EntityPersister
             if ($newVal !== null) {
                 $oid = spl_object_id($newVal);
 
-                if (isset($this->queuedInserts[$oid]) || $uow->isScheduledForInsert($newVal)) {
-                    // The associated entity $newVal is not yet persisted, so we must
-                    // set $newVal = null, in order to insert a null value and schedule an
-                    // extra update on the UnitOfWork.
+                // If the associated entity $newVal is not yet persisted and/or does not yet have
+                // an ID assigned, we must set $newVal = null. This will insert a null value and
+                // schedule an extra update on the UnitOfWork.
+                //
+                // This gives us extra time to a) possibly obtain a database-generated identifier
+                // value for $newVal, and b) insert $newVal into the database before the foreign
+                // key reference is being made.
+                //
+                // When looking at $this->queuedInserts and $uow->isScheduledForInsert, be aware
+                // of the implementation details that our own executeInserts() method will remove
+                // entities from the former as soon as the insert statement has been executed and
+                // a post-insert ID has been assigned (if necessary), and that the UnitOfWork has
+                // already removed entities from its own list at the time they were passed to our
+                // addInsert() method.
+                //
+                // Then, there is one extra exception we can make: An entity that references back to itself
+                // _and_ uses an application-provided ID (the "NONE" generator strategy) also does not
+                // need the extra update, although it is still in the list of insertions itself.
+                // This looks like a minor optimization at first, but is the capstone for being able to
+                // use non-NULLable, self-referencing associations in applications that provide IDs (like UUIDs).
+                if (
+                    (isset($this->queuedInserts[$oid]) || $uow->isScheduledForInsert($newVal))
+                    && ! ($newVal === $entity && $this->class->isIdentifierNatural())
+                ) {
                     $uow->scheduleExtraUpdate($entity, [$field => [null, $newVal]]);
 
                     $newVal = null;


Test patch:
diff --git a/tests/Doctrine/Tests/ORM/Functional/GH7877Test.php b/tests/Doctrine/Tests/ORM/Functional/GH7877Test.php
new file mode 100644
index 000000000..6461c6865
--- /dev/null
+++ b/tests/Doctrine/Tests/ORM/Functional/GH7877Test.php
@@ -0,0 +1,135 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Doctrine\Tests\ORM\Functional;
+
+use Doctrine\ORM\Mapping as ORM;
+use Doctrine\Tests\OrmFunctionalTestCase;
+
+use function uniqid;
+
+/**
+ * @group GH7877
+ */
+class GH7877Test extends OrmFunctionalTestCase
+{
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        $this->createSchemaForModels(
+            GH7877ApplicationGeneratedIdEntity::class,
+            GH7877EntityWithNullableAssociation::class
+        );
+    }
+
+    public function testSelfReferenceWithApplicationGeneratedIdMayBeNotNullable(): void
+    {
+        $entity         = new GH7877ApplicationGeneratedIdEntity();
+        $entity->parent = $entity;
+
+        $this->expectNotToPerformAssertions();
+
+        $this->_em->persist($entity);
+        $this->_em->flush();
+    }
+
+    public function testCrossReferenceWithApplicationGeneratedIdMayBeNotNullable(): void
+    {
+        $entity1         = new GH7877ApplicationGeneratedIdEntity();
+        $entity1->parent = $entity1;
+        $entity2         = new GH7877ApplicationGeneratedIdEntity();
+        $entity2->parent = $entity1;
+
+        $this->expectNotToPerformAssertions();
+
+        // As long as we do not have entity-level commit order computation
+        // (see https://github.com/doctrine/orm/pull/10547),
+        // this only works when the UoW processes $entity1 before $entity2,
+        // so that the foreign key constraint E2 -> E1 can be satisfied.
+
+        $this->_em->persist($entity1);
+        $this->_em->persist($entity2);
+        $this->_em->flush();
+    }
+
+    public function testNullableForeignKeysMakeInsertOrderLessRelevant(): void
+    {
+        $entity1         = new GH7877EntityWithNullableAssociation();
+        $entity1->parent = $entity1;
+        $entity2         = new GH7877EntityWithNullableAssociation();
+        $entity2->parent = $entity1;
+
+        $this->expectNotToPerformAssertions();
+
+        // In contrast to the previous test, this case demonstrates that with NULLable
+        // associations, even without entity-level commit order computation
+        // (see https://github.com/doctrine/orm/pull/10547), we can get away with an
+        // insertion order of E2 before E1. That is because the UoW will schedule an extra
+        // update that saves the day - the foreign key reference will established only after
+        // all insertions have been performed.
+
+        $this->_em->persist($entity2);
+        $this->_em->persist($entity1);
+        $this->_em->flush();
+    }
+}
+
+/**
+ * @ORM\Entity
+ */
+class GH7877ApplicationGeneratedIdEntity
+{
+    /**
+     * @ORM\Id
+     * @ORM\Column(type="string")
+     * @ORM\GeneratedValue(strategy="NONE")
+     *
+     * @var string
+     */
+    public $id;
+
+    /**
+     * (!) Note this uses "nullable=false"
+     *
+     * @ORM\ManyToOne(targetEntity="GH7877ApplicationGeneratedIdEntity")
+     * @ORM\JoinColumn(name="parent_id", referencedColumnName="id", nullable=false)
+     *
+     * @var self
+     */
+    public $parent;
+
+    public function __construct()
+    {
+        $this->id = uniqid();
+    }
+}
+
+/**
+ * @ORM\Entity
+ */
+class GH7877EntityWithNullableAssociation
+{
+    /**
+     * @ORM\Id
+     * @ORM\Column(type="string")
+     * @ORM\GeneratedValue(strategy="NONE")
+     *
+     * @var string
+     */
+    public $id;
+
+    /**
+     * @ORM\ManyToOne(targetEntity="GH7877EntityWithNullableAssociation")
+     * @ORM\JoinColumn(name="parent_id", referencedColumnName="id", nullable=true)
+     *
+     * @var self
+     */
+    public $parent;
+
+    public function __construct()
+    {
+        $this->id = uniqid();
+    }
+}