Skip to content

Commit 592fe57

Browse files
mpdudesylfabre
mpdude
and
sylfabre
committed
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 * 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 (doctrine#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 doctrine#7877, closes doctrine#7882. Co-authored-by: Sylvain Fabre <sylvain.fabre@assoconnect.com>
1 parent da0998c commit 592fe57

File tree

2 files changed

+168
-7
lines changed

2 files changed

+168
-7
lines changed

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

+33-7
Original file line numberDiff line numberDiff line change
@@ -266,7 +266,7 @@ public function executeInserts()
266266
$stmt = $this->conn->prepare($this->getInsertSQL());
267267
$tableName = $this->class->getTableName();
268268

269-
foreach ($this->queuedInserts as $entity) {
269+
foreach ($this->queuedInserts as $key => $entity) {
270270
$insertData = $this->prepareInsertData($entity);
271271

272272
if (isset($insertData[$tableName])) {
@@ -297,9 +297,16 @@ public function executeInserts()
297297
if ($this->class->requiresFetchAfterChange) {
298298
$this->assignDefaultVersionAndUpsertableValues($entity, $id);
299299
}
300-
}
301300

302-
$this->queuedInserts = [];
301+
// Unset this queued insert, so that the prepareUpdateData() method knows right away
302+
// (for the next entity already) that the current entity has been written to the database
303+
// and no extra updates need to be scheduled to refer to it.
304+
//
305+
// In \Doctrine\ORM\UnitOfWork::executeInserts(), the UoW already removed entities
306+
// from its own list (\Doctrine\ORM\UnitOfWork::$entityInsertions) right after they
307+
// were given to our addInsert() method.
308+
unset($this->queuedInserts[$key]);
309+
}
303310

304311
return $postInsertIds;
305312
}
@@ -683,10 +690,29 @@ protected function prepareUpdateData($entity, bool $isInsert = false)
683690
if ($newVal !== null) {
684691
$oid = spl_object_id($newVal);
685692

686-
if (isset($this->queuedInserts[$oid]) || $uow->isScheduledForInsert($newVal)) {
687-
// The associated entity $newVal is not yet persisted, so we must
688-
// set $newVal = null, in order to insert a null value and schedule an
689-
// extra update on the UnitOfWork.
693+
// If the associated entity $newVal is not yet persisted, we must
694+
// set $newVal = null, in order to insert a null value and schedule an
695+
// extra update on the UnitOfWork.
696+
//
697+
// This gives us extra time to a) possibly obtain a database-generated identifier
698+
// value for $newVal, and b) insert $newVal into the database before the foreign
699+
// key reference is being made.
700+
//
701+
// When looking at $this->queuedInserts and $uow->isScheduledForInsert, be aware
702+
// of the implementation details that our own executeInserts() method will remove
703+
// entities from the former as soon as the insert statement has been executed, and
704+
// that the UnitOfWork has already removed entities from its own list at the time
705+
// they were passed to our addInsert() method.
706+
//
707+
// Then, there is one extra exception we can make: An entity that references back to itself
708+
// _and_ uses an application-provided ID (the "NONE" generator strategy) also does not
709+
// need the extra update, although it is still in the list of insertions itself.
710+
// This looks like a minor optimization at first, but is the capstone for being able to
711+
// use non-NULLable, self-referencing associations in applications that provide IDs (like UUIDs).
712+
if (
713+
(isset($this->queuedInserts[$oid]) || $uow->isScheduledForInsert($newVal))
714+
&& ! ($newVal === $entity && $this->class->isIdentifierNatural())
715+
) {
690716
$uow->scheduleExtraUpdate($entity, [$field => [null, $newVal]]);
691717

692718
$newVal = null;

‎tests/Doctrine/Tests/ORM/Functional/GH7877Test.php

+135
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,135 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Doctrine\Tests\ORM\Functional;
6+
7+
use Doctrine\ORM\Mapping as ORM;
8+
use Doctrine\Tests\OrmFunctionalTestCase;
9+
10+
use function uniqid;
11+
12+
/**
13+
* @group GH7877
14+
*/
15+
class GH7877Test extends OrmFunctionalTestCase
16+
{
17+
protected function setUp(): void
18+
{
19+
parent::setUp();
20+
21+
$this->createSchemaForModels(
22+
GH7877ApplicationGeneratedIdEntity::class,
23+
GH7877EntityWithNullableAssociation::class
24+
);
25+
}
26+
27+
public function testSelfReferenceWithApplicationGeneratedIdMayBeNotNullable(): void
28+
{
29+
$entity = new GH7877ApplicationGeneratedIdEntity();
30+
$entity->parent = $entity;
31+
32+
$this->expectNotToPerformAssertions();
33+
34+
$this->_em->persist($entity);
35+
$this->_em->flush();
36+
}
37+
38+
public function testCrossReferenceWithApplicationGeneratedIdMayBeNotNullable(): void
39+
{
40+
$entity1 = new GH7877ApplicationGeneratedIdEntity();
41+
$entity1->parent = $entity1;
42+
$entity2 = new GH7877ApplicationGeneratedIdEntity();
43+
$entity2->parent = $entity1;
44+
45+
$this->expectNotToPerformAssertions();
46+
47+
// As long as we do not have entity-level commit order computation
48+
// (see https://github.com/doctrine/orm/pull/10547),
49+
// this only works when the UoW processes $entity1 before $entity2,
50+
// so that the foreign key constraint E2 -> E1 can be satisfied.
51+
52+
$this->_em->persist($entity1);
53+
$this->_em->persist($entity2);
54+
$this->_em->flush();
55+
}
56+
57+
public function testNullableForeignKeysMakeInsertOrderLessRelevant(): void
58+
{
59+
$entity1 = new GH7877EntityWithNullableAssociation();
60+
$entity1->parent = $entity1;
61+
$entity2 = new GH7877EntityWithNullableAssociation();
62+
$entity2->parent = $entity1;
63+
64+
$this->expectNotToPerformAssertions();
65+
66+
// In contrast to the previous test, this case demonstrates that with NULLable
67+
// associations, even without entity-level commit order computation
68+
// (see https://github.com/doctrine/orm/pull/10547), we can get away with an
69+
// insertion order of E2 before E1. That is because the UoW will schedule an extra
70+
// update that saves the day - the foreign key reference will established only after
71+
// all insertions have been performed.
72+
73+
$this->_em->persist($entity2);
74+
$this->_em->persist($entity1);
75+
$this->_em->flush();
76+
}
77+
}
78+
79+
/**
80+
* @ORM\Entity
81+
*/
82+
class GH7877ApplicationGeneratedIdEntity
83+
{
84+
/**
85+
* @ORM\Id
86+
* @ORM\Column(type="string")
87+
* @ORM\GeneratedValue(strategy="NONE")
88+
*
89+
* @var string
90+
*/
91+
public $id;
92+
93+
/**
94+
* (!) Note this uses "nullable=false"
95+
*
96+
* @ORM\ManyToOne(targetEntity="GH7877ApplicationGeneratedIdEntity")
97+
* @ORM\JoinColumn(name="parent_id", referencedColumnName="id", nullable=false)
98+
*
99+
* @var self
100+
*/
101+
public $parent;
102+
103+
public function __construct()
104+
{
105+
$this->id = uniqid();
106+
}
107+
}
108+
109+
/**
110+
* @ORM\Entity
111+
*/
112+
class GH7877EntityWithNullableAssociation
113+
{
114+
/**
115+
* @ORM\Id
116+
* @ORM\Column(type="string")
117+
* @ORM\GeneratedValue(strategy="NONE")
118+
*
119+
* @var string
120+
*/
121+
public $id;
122+
123+
/**
124+
* @ORM\ManyToOne(targetEntity="GH7877EntityWithNullableAssociation")
125+
* @ORM\JoinColumn(name="parent_id", referencedColumnName="id", nullable=true)
126+
*
127+
* @var self
128+
*/
129+
public $parent;
130+
131+
public function __construct()
132+
{
133+
$this->id = uniqid();
134+
}
135+
}

0 commit comments

Comments
 (0)