diff --git a/lib/Doctrine/ORM/Internal/StronglyConnectedComponents.php b/lib/Doctrine/ORM/Internal/StronglyConnectedComponents.php new file mode 100644 index 00000000000..0e184c30fc6 --- /dev/null +++ b/lib/Doctrine/ORM/Internal/StronglyConnectedComponents.php @@ -0,0 +1,170 @@ + + */ + private $nodes = []; + + /** + * DFS state for the different nodes, indexed by node object id and using one of + * this class' constants as value. + * + * @var array + */ + private $states = []; + + /** + * Edges between the nodes. The first-level key is the object id of the outgoing + * node; the second array maps the destination node by object id as key. + * + * @var array> + */ + private $edges = []; + + /** + * DFS numbers, by object ID + * + * @var array + */ + private $dfs = []; + + /** + * lowlink numbers, by object ID + * + * @var array + */ + private $lowlink = []; + + /** @var int */ + private $maxdfs = 0; + + /** + * Nodes representing the SCC another node is in, indexed by lookup-node object ID + * + * @var array + */ + private $representingNodes = []; + + /** + * Stack with OIDs of nodes visited in the current state of the DFS + * + * @var list + */ + private $stack = []; + + /** @param object $node */ + public function addNode($node): void + { + $id = spl_object_id($node); + $this->nodes[$id] = $node; + $this->states[$id] = self::NOT_VISITED; + $this->edges[$id] = []; + } + + /** @param object $node */ + public function hasNode($node): bool + { + return isset($this->nodes[spl_object_id($node)]); + } + + /** + * Adds a new edge between two nodes to the graph + * + * @param object $from + * @param object $to + */ + public function addEdge($from, $to): void + { + $fromId = spl_object_id($from); + $toId = spl_object_id($to); + + $this->edges[$fromId][$toId] = true; + } + + public function findStronglyConnectedComponents(): void + { + foreach (array_keys($this->nodes) as $oid) { + if ($this->states[$oid] === self::NOT_VISITED) { + $this->tarjan($oid); + } + } + } + + private function tarjan(int $oid): void + { + $this->dfs[$oid] = $this->lowlink[$oid] = $this->maxdfs++; + $this->states[$oid] = self::IN_PROGRESS; + array_push($this->stack, $oid); + + foreach ($this->edges[$oid] as $adjacentId => $ignored) { + if ($this->states[$adjacentId] === self::NOT_VISITED) { + $this->tarjan($adjacentId); + $this->lowlink[$oid] = min($this->lowlink[$oid], $this->lowlink[$adjacentId]); + } elseif ($this->states[$adjacentId] === self::IN_PROGRESS) { + $this->lowlink[$oid] = min($this->lowlink[$oid], $this->dfs[$adjacentId]); + } + } + + $lowlink = $this->lowlink[$oid]; + if ($lowlink === $this->dfs[$oid]) { + $representingNode = null; + do { + $unwindOid = array_pop($this->stack); + + if (! $representingNode) { + $representingNode = $this->nodes[$unwindOid]; + } + + $this->representingNodes[$unwindOid] = $representingNode; + $this->states[$unwindOid] = self::VISITED; + } while ($unwindOid !== $oid); + } + } + + /** + * @param object $node + * + * @return object + */ + public function getNodeRepresentingStronglyConnectedComponent($node) + { + $oid = spl_object_id($node); + + if (! isset($this->representingNodes[$oid])) { + throw new InvalidArgumentException('unknown node'); + } + + return $this->representingNodes[$oid]; + } +} diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index b3285a8845c..bd2d0ba5910 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -28,6 +28,7 @@ use Doctrine\ORM\Exception\UnexpectedAssociationValue; use Doctrine\ORM\Id\AssignedGenerator; use Doctrine\ORM\Internal\HydrationCompleteHandler; +use Doctrine\ORM\Internal\StronglyConnectedComponents; use Doctrine\ORM\Internal\TopologicalSort; use Doctrine\ORM\Mapping\ClassMetadata; use Doctrine\ORM\Mapping\MappingException; @@ -1391,14 +1392,19 @@ private function computeInsertExecutionOrder(): array /** @return list */ private function computeDeleteExecutionOrder(): array { - $sort = new TopologicalSort(); + $stronglyConnectedComponents = new StronglyConnectedComponents(); + $sort = new TopologicalSort(); - // First make sure we have all the nodes foreach ($this->entityDeletions as $entity) { + $stronglyConnectedComponents->addNode($entity); $sort->addNode($entity); } - // Now add edges + // First, consider only "on delete cascade" associations between entities + // and find strongly connected groups. Once we delete any one of the entities + // in such a group, _all_ of the other entities will be removed as well. So, + // we need to treat those groups like a single entity when performing delete + // order topological sorting. foreach ($this->entityDeletions as $entity) { $class = $this->em->getClassMetadata(get_class($entity)); @@ -1410,16 +1416,65 @@ private function computeDeleteExecutionOrder(): array continue; } - // For associations that implement a database-level cascade/set null operation, + assert(isset($assoc['joinColumns'])); + $joinColumns = reset($assoc['joinColumns']); + if (! isset($joinColumns['onDelete'])) { + continue; + } + + $onDeleteOption = strtolower($joinColumns['onDelete']); + if ($onDeleteOption !== 'cascade') { + continue; + } + + $targetEntity = $class->getFieldValue($entity, $assoc['fieldName']); + + // If the association does not refer to another entity or that entity + // is not to be deleted, there is no ordering problem and we can + // skip this particular association. + if ($targetEntity === null || ! $stronglyConnectedComponents->hasNode($targetEntity)) { + continue; + } + + $stronglyConnectedComponents->addEdge($entity, $targetEntity); + } + } + + $stronglyConnectedComponents->findStronglyConnectedComponents(); + + // Now do the actual topological sorting to find the delete order. + foreach ($this->entityDeletions as $entity) { + $class = $this->em->getClassMetadata(get_class($entity)); + + // Get the entities representing the SCC + $entityComponent = $stronglyConnectedComponents->getNodeRepresentingStronglyConnectedComponent($entity); + + // When $entity is part of a non-trivial strongly connected component group + // (a group containing not only those entities alone), make sure we process it _after_ the + // entity representing the group. + // The dependency direction implies that "$entity depends on $entityComponent + // being deleted first". The topological sort will output the depended-upon nodes first. + if ($entityComponent !== $entity) { + $sort->addEdge($entity, $entityComponent, false); + } + + foreach ($class->associationMappings as $assoc) { + // We only need to consider the owning sides of to-one associations, + // since many-to-many associations can always be (and have already been) + // deleted in a preceding step. + if (! ($assoc['isOwningSide'] && $assoc['type'] & ClassMetadata::TO_ONE)) { + continue; + } + + // For associations that implement a database-level set null operation, // we do not have to follow a particular order: If the referred-to entity is - // deleted first, the DBMS will either delete the current $entity right away - // (CASCADE) or temporarily set the foreign key to NULL (SET NULL). - // Either way, we can skip it in the computation. + // deleted first, the DBMS will temporarily set the foreign key to NULL (SET NULL). + // So, we can skip it in the computation. assert(isset($assoc['joinColumns'])); $joinColumns = reset($assoc['joinColumns']); if (isset($joinColumns['onDelete'])) { $onDeleteOption = strtolower($joinColumns['onDelete']); - if ($onDeleteOption === 'cascade' || $onDeleteOption === 'set null') { + if ($onDeleteOption === 'set null') { continue; } } @@ -1433,10 +1488,17 @@ private function computeDeleteExecutionOrder(): array continue; } - // Add dependency. The dependency direction implies that "$targetEntity depends on $entity + // Get the entities representing the SCC + $targetEntityComponent = $stronglyConnectedComponents->getNodeRepresentingStronglyConnectedComponent($targetEntity); + + // When we have a dependency between two different groups of strongly connected nodes, + // add it to the computation. + // The dependency direction implies that "$targetEntityComponent depends on $entityComponent // being deleted first". The topological sort will output the depended-upon nodes first, // so we can work through the result in the returned order. - $sort->addEdge($targetEntity, $entity, false); + if ($targetEntityComponent !== $entityComponent) { + $sort->addEdge($targetEntityComponent, $entityComponent, false); + } } } diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/GH10912Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH10912Test.php new file mode 100644 index 00000000000..b81f23f51ee --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH10912Test.php @@ -0,0 +1,165 @@ +setUpEntitySchema([ + GH10912User::class, + GH10912Profile::class, + GH10912Room::class, + ]); + } + + public function testIssue(): void + { + $user = new GH10912User(); + $profile = new GH10912Profile(); + $room = new GH10912Room(); + + $user->rooms->add($room); + $user->profile = $profile; + $profile->user = $user; + $room->user = $user; + + $this->_em->persist($room); + $this->_em->persist($user); + $this->_em->persist($profile); + $this->_em->flush(); + + /* + * This issue is about finding a special deletion order: + * $user and $profile cross-reference each other with ON DELETE CASCADE. + * So, whichever one gets deleted first, the DBMS will immediately dispose + * of the other one as well. + * + * $user -> $room is the unproblematic (irrelevant) inverse side of + * a OneToMany association. + * + * $room -> $user is a not-nullable, no DBMS-level-cascade, owning side + * of ManyToOne. We *must* remove the $room _before_ the $user can be + * deleted. And remember, $user deletion happens either when we DELETE the + * user (direct deletion), or when we delete the $profile (ON DELETE CASCADE + * propagates to the user). + * + * In the original bug report, the ordering of fields in the entities was + * relevant, in combination with a cascade=persist configuration. + * + * But, for the sake of clarity, let's put these features away and create + * the problematic sequence in UnitOfWork::$entityDeletions directly: + */ + $this->_em->remove($profile); + $this->_em->remove($user); + $this->_em->remove($room); + + $queryLog = $this->getQueryLog(); + $queryLog->reset()->enable(); + + $this->_em->flush(); + + $queries = array_values(array_filter($queryLog->queries, static function (array $entry): bool { + return strpos($entry['sql'], 'DELETE') === 0; + })); + + self::assertCount(3, $queries); + + // we do not care about the order of $user vs. $profile, so do not check them. + self::assertSame('DELETE FROM GH10912Room WHERE id = ?', $queries[0]['sql'], '$room deletion is the first query'); + + // The EntityManager is aware that all three entities have been deleted (sanity check) + $im = $this->_em->getUnitOfWork()->getIdentityMap(); + self::assertEmpty($im[GH10912Profile::class]); + self::assertEmpty($im[GH10912User::class]); + self::assertEmpty($im[GH10912Room::class]); + } +} + +/** @ORM\Entity */ +class GH10912User +{ + /** + * @ORM\Id + * @ORM\Column(type="integer") + * @ORM\GeneratedValue + * + * @var int + */ + public $id; + + /** + * @ORM\OneToMany(targetEntity=GH10912Room::class, mappedBy="user") + * + * @var Collection + */ + public $rooms; + + /** + * @ORM\OneToOne(targetEntity=GH10912Profile::class) + * @ORM\JoinColumn(onDelete="cascade") + * + * @var GH10912Profile + */ + public $profile; + + public function __construct() + { + $this->rooms = new ArrayCollection(); + } +} + +/** @ORM\Entity */ +class GH10912Profile +{ + /** + * @ORM\Id + * @ORM\Column(type="integer") + * @ORM\GeneratedValue + * + * @var int + */ + public $id; + + /** + * @ORM\OneToOne(targetEntity=GH10912User::class) + * @ORM\JoinColumn(onDelete="cascade") + * + * @var GH10912User + */ + public $user; +} + +/** @ORM\Entity */ +class GH10912Room +{ + /** + * @ORM\Id + * @ORM\Column(type="integer") + * @ORM\GeneratedValue + * + * @var int + */ + public $id; + + /** + * @ORM\ManyToOne(targetEntity=GH10912User::class, inversedBy="rooms") + * @ORM\JoinColumn(nullable=false) + * + * @var GH10912User + */ + public $user; +} diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/GH10913Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH10913Test.php new file mode 100644 index 00000000000..c75f7d9da7a --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH10913Test.php @@ -0,0 +1,167 @@ +setUpEntitySchema([ + GH10913Entity::class, + ]); + } + + public function testExample1(): void + { + [$a, $b, $c] = $this->createEntities(3); + + $c->ref = $b; + $b->odc = $a; + + $this->_em->persist($a); + $this->_em->persist($b); + $this->_em->persist($c); + $this->_em->flush(); + + $this->_em->remove($a); + $this->_em->remove($b); + $this->_em->remove($c); + + $this->flushAndAssertNumberOfDeleteQueries(3); + } + + public function testExample2(): void + { + [$a, $b, $c] = $this->createEntities(3); + + $a->odc = $b; + $b->odc = $a; + $c->ref = $b; + + $this->_em->persist($a); + $this->_em->persist($b); + $this->_em->persist($c); + $this->_em->flush(); + + $this->_em->remove($a); + $this->_em->remove($b); + $this->_em->remove($c); + + $this->flushAndAssertNumberOfDeleteQueries(3); + } + + public function testExample3(): void + { + [$a, $b, $c] = $this->createEntities(3); + + $a->odc = $b; + $a->ref = $c; + $c->ref = $b; + $b->odc = $a; + + $this->_em->persist($a); + $this->_em->persist($b); + $this->_em->persist($c); + $this->_em->flush(); + + $this->_em->remove($a); + $this->_em->remove($b); + $this->_em->remove($c); + + self::expectException(CycleDetectedException::class); + + $this->_em->flush(); + } + + public function testExample4(): void + { + [$a, $b, $c, $d] = $this->createEntities(4); + + $a->ref = $b; + $b->odc = $c; + $c->odc = $b; + $d->ref = $c; + + $this->_em->persist($a); + $this->_em->persist($b); + $this->_em->persist($c); + $this->_em->persist($d); + $this->_em->flush(); + + $this->_em->remove($b); + $this->_em->remove($c); + $this->_em->remove($d); + $this->_em->remove($a); + + $this->flushAndAssertNumberOfDeleteQueries(4); + } + + private function flushAndAssertNumberOfDeleteQueries(int $expectedCount): void + { + $queryLog = $this->getQueryLog(); + $queryLog->reset()->enable(); + + $this->_em->flush(); + + $queries = array_values(array_filter($queryLog->queries, static function (array $entry): bool { + return strpos($entry['sql'], 'DELETE') === 0; + })); + + self::assertCount($expectedCount, $queries); + } + + /** + * @return list + */ + private function createEntities(int $count = 1): array + { + $result = []; + + for ($i = 0; $i < $count; $i++) { + $result[] = new GH10913Entity(); + } + + return $result; + } +} + +/** @ORM\Entity */ +class GH10913Entity +{ + /** + * @ORM\Id + * @ORM\Column(type="integer") + * @ORM\GeneratedValue + * + * @var int + */ + public $id; + + /** + * @ORM\ManyToOne(targetEntity=GH10913Entity::class) + * @ORM\JoinColumn(nullable=true, onDelete="CASCADE") + * + * @var GH10913Entity + */ + public $odc; + + /** + * @ORM\ManyToOne(targetEntity=GH10913Entity::class) + * @ORM\JoinColumn(nullable=true) + * + * @var GH10913Entity + */ + public $ref; +} diff --git a/tests/Doctrine/Tests/ORM/Internal/StronglyConnectedComponentsTest.php b/tests/Doctrine/Tests/ORM/Internal/StronglyConnectedComponentsTest.php new file mode 100644 index 00000000000..a2adee9dc22 --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Internal/StronglyConnectedComponentsTest.php @@ -0,0 +1,113 @@ + */ + private $nodes = []; + + /** @var StronglyConnectedComponents */ + private $stronglyConnectedComponents; + + protected function setUp(): void + { + $this->stronglyConnectedComponents = new StronglyConnectedComponents(); + } + + public function testFindStronglyConnectedComponents(): void + { + // A -> B <-> C -> D <-> E + $this->addNodes('A', 'B', 'C', 'D', 'E'); + + $this->addEdge('A', 'B'); + $this->addEdge('B', 'C'); + $this->addEdge('C', 'B'); + $this->addEdge('C', 'D'); + $this->addEdge('D', 'E'); + $this->addEdge('E', 'D'); + + $this->stronglyConnectedComponents->findStronglyConnectedComponents(); + + $this->assertNodesAreInSameComponent('B', 'C'); + $this->assertNodesAreInSameComponent('D', 'E'); + $this->assertNodesAreNotInSameComponent('A', 'B'); + $this->assertNodesAreNotInSameComponent('A', 'D'); + } + + public function testFindStronglyConnectedComponents2(): void + { + // A -> B -> C -> D -> B + $this->addNodes('A', 'B', 'C', 'D'); + + $this->addEdge('A', 'B'); + $this->addEdge('B', 'C'); + $this->addEdge('C', 'D'); + $this->addEdge('D', 'B'); + + $this->stronglyConnectedComponents->findStronglyConnectedComponents(); + + $this->assertNodesAreInSameComponent('B', 'C'); + $this->assertNodesAreInSameComponent('C', 'D'); + $this->assertNodesAreNotInSameComponent('A', 'B'); + } + + public function testFindStronglyConnectedComponents3(): void + { + // v---------. + // A -> B -> C -> D -> E + // ^--------ยด + + $this->addNodes('A', 'B', 'C', 'D', 'E'); + + $this->addEdge('A', 'B'); + $this->addEdge('B', 'C'); + $this->addEdge('C', 'D'); + $this->addEdge('D', 'E'); + $this->addEdge('E', 'C'); + $this->addEdge('D', 'B'); + + $this->stronglyConnectedComponents->findStronglyConnectedComponents(); + + $this->assertNodesAreInSameComponent('B', 'C'); + $this->assertNodesAreInSameComponent('C', 'D'); + $this->assertNodesAreInSameComponent('D', 'E'); + $this->assertNodesAreInSameComponent('E', 'B'); + $this->assertNodesAreNotInSameComponent('A', 'B'); + } + + private function addNodes(string ...$names): void + { + foreach ($names as $name) { + $node = new Node($name); + $this->nodes[$name] = $node; + $this->stronglyConnectedComponents->addNode($node); + } + } + + private function addEdge(string $from, string $to, bool $optional = false): void + { + $this->stronglyConnectedComponents->addEdge($this->nodes[$from], $this->nodes[$to], $optional); + } + + private function assertNodesAreInSameComponent(string $first, string $second): void + { + self::assertSame( + $this->stronglyConnectedComponents->getNodeRepresentingStronglyConnectedComponent($this->nodes[$first]), + $this->stronglyConnectedComponents->getNodeRepresentingStronglyConnectedComponent($this->nodes[$second]) + ); + } + + private function assertNodesAreNotInSameComponent(string $first, string $second): void + { + self::assertNotSame( + $this->stronglyConnectedComponents->getNodeRepresentingStronglyConnectedComponent($this->nodes[$first]), + $this->stronglyConnectedComponents->getNodeRepresentingStronglyConnectedComponent($this->nodes[$second]) + ); + } +}