Make it possible to have non-NULLable self-referencing associations w…

…hen 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
* 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.

I am considering this a bugfix since the ORM previously executed additional queries that were not strictly necessary, and that required users to work with NULLable columns where conceptually a non-NULLable column would be valid and more expressive.

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 <[email protected]>

lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php

@@ -266,7 +266,7 @@ public function executeInserts()
         $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])) {
@@ -297,9 +297,16 @@ public function executeInserts()
             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]);
+        }
 
         return $postInsertIds;
     }
@@ -683,10 +690,37 @@ protected function prepareUpdateData($entity, bool $isInsert = false)
             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.
+                //
+                // Depending on the ID generator strategy used, which entity persister is responsible
+                // for inserting the entity and whether the UoW already had an opportunity to write
+                // database-generated IDs ("post insert id") back into the entities, combinations of
+                // these factors may occur. For example: An entity may have an application-provided (natural)
+                // ID, but has not yet been inserted into the database. Or, we used the "AUTO" generator
+                // strategy and have already inserted the entity into the database, but the ID is not yet
+                // available in the entity's fields (it's set by the UoW after the EntityPersister finished).
+                //
+                // 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
+                // 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) || !$uow->isInIdentityMap($newVal))
+                    && ! ($newVal === $entity && $this->class->isIdentifierNatural())
+                ) {
                     $uow->scheduleExtraUpdate($entity, [$field => [null, $newVal]]);
 
                     $newVal = null;

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();
+    }
+}