Skip to content

Commit ae60cf0

Browse files
mpdudempdude
authored
Commit order for removals has to consider SET NULL, not nullable (#10566)
When computing the commit order for entity removals, we have to look out for `@ORM\JoinColumn(onDelete="SET NULL")` to find places where cyclic associations can be broken. #### Background The UoW computes a "commit order" to find the sequence in which tables shall be processed when inserting entities into the database or performing delete operations. For the insert case, the ORM is able to schedule _extra updates_ that will be performed after all entities have been inserted. Associations which are configured as `@ORM\JoinColumn(nullable=true, ...)` can be left as `NULL` in the database when performing the initial `INSERT` statements, and will be updated once all new entities have been written to the database. This can be used to break cyclic associations between entity instances. For removals, the ORM does not currently implement up-front `UPDATE` statements to `NULL` out associations before `DELETE` statements are executed. That means when associations form a cycle, users have to configure `@ORM\JoinColumn(onDelete="SET NULL", ...)` on one of the associations involved. This transfers responsibility to the DBMS to break the cycle at that place. _But_, we still have to perform the delete statements in an order that makes this happen early enough. This may be a _different_ order than the one required for the insert case. We can find it _only_ by looking at the `onDelete` behaviour. We must ignore the `nullable` property, which is irrelevant, since we do not even try to `NULL` anything. #### Example Assume three entity classes `A`, `B`, `C`. There are unidirectional one-to-one associations `A -> B`, `B -> C`, `C -> A`. All those associations are `nullable= true`. Three entities `$a`, `$b`, `$c` are created from these respective classes and associations are set up. All operations `cascade` at the ORM level. So we can test what happens when we start the operations at the three individual entities, but in the end, they will always involve all three of them. _Any_ insert order will work, so the improvements necessary to solve #10531 or #10532 are not needed here. Since all associations are between different tables, the current table-level computation is good enough. For the removal case, only the `A -> B` association has `onDelete="SET NULL"`. So, the only possible execution order is `$b`, `$c`, `$a`. We have to find that regardless of where we start the cascade operation. The DBMS will set the `A -> B` association on `$a` to `NULL` when we remove `$b`. We can then remove `$c` since it is no longer being referred to, then `$a`. #### Related cases These cases ask for the ORM to perform the extra update before the delete by itself, without DBMS-level support: * #5665 * #10548
1 parent 9ac063d commit ae60cf0

File tree

2 files changed

+230
-26
lines changed

2 files changed

+230
-26
lines changed

‎lib/Doctrine/ORM/UnitOfWork.php

+53-26
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
use Doctrine\ORM\Id\AssignedGenerator;
2929
use Doctrine\ORM\Internal\CommitOrderCalculator;
3030
use Doctrine\ORM\Internal\HydrationCompleteHandler;
31+
use Doctrine\ORM\Internal\TopologicalSort;
3132
use Doctrine\ORM\Mapping\ClassMetadata;
3233
use Doctrine\ORM\Mapping\MappingException;
3334
use Doctrine\ORM\Mapping\Reflection\ReflectionPropertiesGetter;
@@ -57,7 +58,6 @@
5758
use function array_map;
5859
use function array_merge;
5960
use function array_pop;
60-
use function array_reverse;
6161
use function array_sum;
6262
use function array_values;
6363
use function assert;
@@ -74,6 +74,7 @@
7474
use function reset;
7575
use function spl_object_id;
7676
use function sprintf;
77+
use function strtolower;
7778

7879
/**
7980
* The UnitOfWork is responsible for tracking changes to objects during an
@@ -408,9 +409,6 @@ public function commit($entity = null)
408409

409410
$this->dispatchOnFlushEvent();
410411

411-
// Now we need a commit order to maintain referential integrity
412-
$commitOrder = $this->getCommitOrder();
413-
414412
$conn = $this->em->getConnection();
415413
$conn->beginTransaction();
416414

@@ -431,7 +429,7 @@ public function commit($entity = null)
431429
// into account (new entities referring to other new entities), since all other types (entities
432430
// with updates or scheduled deletions) are currently not a problem, since they are already
433431
// in the database.
434-
$this->executeInserts($this->computeInsertExecutionOrder($commitOrder));
432+
$this->executeInserts($this->computeInsertExecutionOrder());
435433
}
436434

437435
if ($this->entityUpdates) {
@@ -456,7 +454,7 @@ public function commit($entity = null)
456454
// Entity deletions come last. Their order only needs to take care of other deletions
457455
// (first delete entities depending upon others, before deleting depended-upon entities).
458456
if ($this->entityDeletions) {
459-
$this->executeDeletions($this->computeDeleteExecutionOrder($commitOrder));
457+
$this->executeDeletions($this->computeDeleteExecutionOrder());
460458
}
461459

462460
// Commit failed silently
@@ -1265,14 +1263,11 @@ private function executeDeletions(array $entities): void
12651263
}
12661264
}
12671265

1268-
/**
1269-
* @param list<ClassMetadata> $commitOrder
1270-
*
1271-
* @return list<object>
1272-
*/
1273-
private function computeInsertExecutionOrder(array $commitOrder): array
1266+
/** @return list<object> */
1267+
private function computeInsertExecutionOrder(): array
12741268
{
1275-
$result = [];
1269+
$commitOrder = $this->getCommitOrder();
1270+
$result = [];
12761271
foreach ($commitOrder as $class) {
12771272
$className = $class->name;
12781273
foreach ($this->entityInsertions as $entity) {
@@ -1287,26 +1282,58 @@ private function computeInsertExecutionOrder(array $commitOrder): array
12871282
return $result;
12881283
}
12891284

1290-
/**
1291-
* @param list<ClassMetadata> $commitOrder
1292-
*
1293-
* @return list<object>
1294-
*/
1295-
private function computeDeleteExecutionOrder(array $commitOrder): array
1285+
/** @return list<object> */
1286+
private function computeDeleteExecutionOrder(): array
12961287
{
1297-
$result = [];
1298-
foreach (array_reverse($commitOrder) as $class) {
1299-
$className = $class->name;
1300-
foreach ($this->entityDeletions as $entity) {
1301-
if ($this->em->getClassMetadata(get_class($entity))->name !== $className) {
1288+
$sort = new TopologicalSort();
1289+
1290+
// First make sure we have all the nodes
1291+
foreach ($this->entityDeletions as $entity) {
1292+
$sort->addNode($entity);
1293+
}
1294+
1295+
// Now add edges
1296+
foreach ($this->entityDeletions as $entity) {
1297+
$class = $this->em->getClassMetadata(get_class($entity));
1298+
1299+
foreach ($class->associationMappings as $assoc) {
1300+
// We only need to consider the owning sides of to-one associations,
1301+
// since many-to-many associations can always be (and have already been)
1302+
// deleted in a preceding step.
1303+
if (! ($assoc['isOwningSide'] && $assoc['type'] & ClassMetadata::TO_ONE)) {
13021304
continue;
13031305
}
13041306

1305-
$result[] = $entity;
1307+
// For associations that implement a database-level cascade/set null operation,
1308+
// we do not have to follow a particular order: If the referred-to entity is
1309+
// deleted first, the DBMS will either delete the current $entity right away
1310+
// (CASCADE) or temporarily set the foreign key to NULL (SET NULL).
1311+
// Either way, we can skip it in the computation.
1312+
assert(isset($assoc['joinColumns']));
1313+
$joinColumns = reset($assoc['joinColumns']);
1314+
if (isset($joinColumns['onDelete'])) {
1315+
$onDeleteOption = strtolower($joinColumns['onDelete']);
1316+
if ($onDeleteOption === 'cascade' || $onDeleteOption === 'set null') {
1317+
continue;
1318+
}
1319+
}
1320+
1321+
$targetEntity = $class->getFieldValue($entity, $assoc['fieldName']);
1322+
1323+
// If the association does not refer to another entity or that entity
1324+
// is not to be deleted, there is no ordering problem and we can
1325+
// skip this particular association.
1326+
if ($targetEntity === null || ! $sort->hasNode($targetEntity)) {
1327+
continue;
1328+
}
1329+
1330+
// Add dependency. The dependency direction implies that "$entity has to be removed before $targetEntity",
1331+
// so we can work through the topo sort result from left to right (with all edges pointing right).
1332+
$sort->addEdge($entity, $targetEntity, false);
13061333
}
13071334
}
13081335

1309-
return $result;
1336+
return $sort->sort();
13101337
}
13111338

13121339
/**

‎tests/Doctrine/Tests/ORM/Functional/Ticket/GH10566Test.php

+177
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,177 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Doctrine\Tests\ORM\Functional\Ticket;
6+
7+
use Doctrine\ORM\Mapping as ORM;
8+
use Doctrine\Tests\OrmFunctionalTestCase;
9+
use Generator;
10+
11+
use function is_a;
12+
13+
class GH10566Test extends OrmFunctionalTestCase
14+
{
15+
protected function setUp(): void
16+
{
17+
parent::setUp();
18+
19+
$this->createSchemaForModels(
20+
GH10566A::class,
21+
GH10566B::class,
22+
GH10566C::class
23+
);
24+
}
25+
26+
/**
27+
* @dataProvider provideEntityClasses
28+
*/
29+
public function testInsertion(string $startEntityClass): void
30+
{
31+
$a = new GH10566A();
32+
$b = new GH10566B();
33+
$c = new GH10566C();
34+
35+
$a->other = $b;
36+
$b->other = $c;
37+
$c->other = $a;
38+
39+
foreach ([$a, $b, $c] as $candidate) {
40+
if (is_a($candidate, $startEntityClass)) {
41+
$this->_em->persist($candidate);
42+
}
43+
}
44+
45+
// Since all associations are nullable, the ORM has no problem finding an insert order,
46+
// it can always schedule "deferred updates" to fill missing foreign key values.
47+
$this->_em->flush();
48+
49+
self::assertNotNull($a->id);
50+
self::assertNotNull($b->id);
51+
self::assertNotNull($c->id);
52+
}
53+
54+
/**
55+
* @dataProvider provideEntityClasses
56+
*/
57+
public function testRemoval(string $startEntityClass): void
58+
{
59+
$a = new GH10566A();
60+
$b = new GH10566B();
61+
$c = new GH10566C();
62+
63+
$a->other = $b;
64+
$b->other = $c;
65+
$c->other = $a;
66+
67+
$this->_em->persist($a);
68+
$this->_em->flush();
69+
70+
$aId = $a->id;
71+
$bId = $b->id;
72+
$cId = $c->id;
73+
74+
// In the removal case, the ORM currently does not schedule "extra updates"
75+
// to break association cycles before entities are removed. So, we must not
76+
// look at "nullable" for associations to find a delete commit order.
77+
//
78+
// To make it work, the user needs to have a database-level "ON DELETE SET NULL"
79+
// on an association. That's where the cycle can be broken. Commit order computation
80+
// for the removal case needs to look at this property.
81+
//
82+
// In this example, only A -> B can be used to break the cycle. So, regardless which
83+
// entity we start with, the ORM-level cascade will always remove all three entities,
84+
// and the order of database deletes always has to be (can only be) from B, then C, then A.
85+
86+
foreach ([$a, $b, $c] as $candidate) {
87+
if (is_a($candidate, $startEntityClass)) {
88+
$this->_em->remove($candidate);
89+
}
90+
}
91+
92+
$this->_em->flush();
93+
94+
self::assertFalse($this->_em->getConnection()->fetchOne('SELECT id FROM gh10566_a WHERE id = ?', [$aId]));
95+
self::assertFalse($this->_em->getConnection()->fetchOne('SELECT id FROM gh10566_b WHERE id = ?', [$bId]));
96+
self::assertFalse($this->_em->getConnection()->fetchOne('SELECT id FROM gh10566_c WHERE id = ?', [$cId]));
97+
}
98+
99+
public function provideEntityClasses(): Generator
100+
{
101+
yield [GH10566A::class];
102+
yield [GH10566B::class];
103+
yield [GH10566C::class];
104+
}
105+
}
106+
107+
/**
108+
* @ORM\Entity
109+
* @ORM\Table(name="gh10566_a")
110+
*/
111+
class GH10566A
112+
{
113+
/**
114+
* @ORM\Id
115+
* @ORM\Column(type="integer")
116+
* @ORM\GeneratedValue()
117+
*
118+
* @var int
119+
*/
120+
public $id;
121+
122+
/**
123+
* @ORM\OneToOne(targetEntity="GH10566B", cascade={"all"})
124+
* @ORM\JoinColumn(nullable=true, onDelete="SET NULL")
125+
*
126+
* @var GH10566B
127+
*/
128+
public $other;
129+
}
130+
131+
/**
132+
* @ORM\Entity
133+
* @ORM\Table(name="gh10566_b")
134+
*/
135+
class GH10566B
136+
{
137+
/**
138+
* @ORM\Id
139+
* @ORM\Column(type="integer")
140+
* @ORM\GeneratedValue()
141+
*
142+
* @var int
143+
*/
144+
public $id;
145+
146+
/**
147+
* @ORM\OneToOne(targetEntity="GH10566C", cascade={"all"})
148+
* @ORM\JoinColumn(nullable=true)
149+
*
150+
* @var GH10566C
151+
*/
152+
public $other;
153+
}
154+
155+
/**
156+
* @ORM\Entity
157+
* @ORM\Table(name="gh10566_c")
158+
*/
159+
class GH10566C
160+
{
161+
/**
162+
* @ORM\Id
163+
* @ORM\Column(type="integer")
164+
* @ORM\GeneratedValue()
165+
*
166+
* @var int
167+
*/
168+
public $id;
169+
170+
/**
171+
* @ORM\OneToOne(targetEntity="GH10566A", cascade={"all"})
172+
* @ORM\JoinColumn(nullable=true)
173+
*
174+
* @var GH10566A
175+
*/
176+
public $other;
177+
}

0 commit comments

Comments
 (0)