From 5876b4d5a46d13e5d02c06c6225275c21354c40c Mon Sep 17 00:00:00 2001 From: Matthias Pigulla Date: Fri, 24 Nov 2023 19:31:15 +0100 Subject: [PATCH] Improve topological sort result order --- lib/Doctrine/ORM/Internal/TopologicalSort.php | 16 ++-- lib/Doctrine/ORM/UnitOfWork.php | 14 ++-- .../ORM/Internal/TopologicalSortTest.php | 76 ++++++++++++++++--- 3 files changed, 77 insertions(+), 29 deletions(-) diff --git a/lib/Doctrine/ORM/Internal/TopologicalSort.php b/lib/Doctrine/ORM/Internal/TopologicalSort.php index 2bf1624ced8..aed71f3b206 100644 --- a/lib/Doctrine/ORM/Internal/TopologicalSort.php +++ b/lib/Doctrine/ORM/Internal/TopologicalSort.php @@ -7,8 +7,6 @@ use Doctrine\ORM\Internal\TopologicalSort\CycleDetectedException; use function array_keys; -use function array_reverse; -use function array_unshift; use function spl_object_id; /** @@ -93,18 +91,14 @@ public function addEdge($from, $to, bool $optional): void /** * Returns a topological sort of all nodes. When we have an edge A->B between two nodes - * A and B, then A will be listed before B in the result. + * A and B, then B will be listed before A in the result. Visually speaking, when ordering + * the nodes in the result order from left to right, all edges point to the left. * * @return list */ public function sort(): array { - /* - * When possible, keep objects in the result in the same order in which they were added as nodes. - * Since nodes are unshifted into $this->>sortResult (see the visit() method), that means we - * need to work them in array_reverse order here. - */ - foreach (array_reverse(array_keys($this->nodes)) as $oid) { + foreach (array_keys($this->nodes) as $oid) { if ($this->states[$oid] === self::NOT_VISITED) { $this->visit($oid); } @@ -147,7 +141,7 @@ private function visit(int $oid): void } // We have found a cycle and cannot break it at $edge. Best we can do - // is to retreat from the current vertex, hoping that somewhere up the + // is to backtrack from the current vertex, hoping that somewhere up the // stack this can be salvaged. $this->states[$oid] = self::NOT_VISITED; $exception->addToCycle($this->nodes[$oid]); @@ -160,6 +154,6 @@ private function visit(int $oid): void // So we're done with this vertex as well. $this->states[$oid] = self::VISITED; - array_unshift($this->sortResult, $this->nodes[$oid]); + $this->sortResult[] = $this->nodes[$oid]; } } diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index b1fef6d2012..b3285a8845c 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -1378,9 +1378,10 @@ private function computeInsertExecutionOrder(): array $joinColumns = reset($assoc['joinColumns']); $isNullable = ! isset($joinColumns['nullable']) || $joinColumns['nullable']; - // Add dependency. The dependency direction implies that "$targetEntity has to go before $entity", - // so we can work through the topo sort result from left to right (with all edges pointing right). - $sort->addEdge($targetEntity, $entity, $isNullable); + // Add dependency. The dependency direction implies that "$entity depends on $targetEntity". The + // topological sort result will output the depended-upon nodes first, which means we can insert + // entities in that order. + $sort->addEdge($entity, $targetEntity, $isNullable); } } @@ -1432,9 +1433,10 @@ private function computeDeleteExecutionOrder(): array continue; } - // Add dependency. The dependency direction implies that "$entity has to be removed before $targetEntity", - // so we can work through the topo sort result from left to right (with all edges pointing right). - $sort->addEdge($entity, $targetEntity, false); + // Add dependency. The dependency direction implies that "$targetEntity depends on $entity + // 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); } } diff --git a/tests/Doctrine/Tests/ORM/Internal/TopologicalSortTest.php b/tests/Doctrine/Tests/ORM/Internal/TopologicalSortTest.php index bba00d2d44e..d0dd9178c15 100644 --- a/tests/Doctrine/Tests/ORM/Internal/TopologicalSortTest.php +++ b/tests/Doctrine/Tests/ORM/Internal/TopologicalSortTest.php @@ -34,7 +34,7 @@ public function testSimpleOrdering(): void $this->addEdge('E', 'A'); // There is only 1 valid ordering for this constellation - self::assertSame(['E', 'A', 'B', 'C'], $this->computeResult()); + self::assertSame(['C', 'B', 'A', 'E'], $this->computeResult()); } public function testSkipOptionalEdgeToBreakCycle(): void @@ -44,7 +44,7 @@ public function testSkipOptionalEdgeToBreakCycle(): void $this->addEdge('A', 'B', true); $this->addEdge('B', 'A', false); - self::assertSame(['B', 'A'], $this->computeResult()); + self::assertSame(['A', 'B'], $this->computeResult()); } public function testBreakCycleByBacktracking(): void @@ -57,7 +57,7 @@ public function testBreakCycleByBacktracking(): void $this->addEdge('D', 'A'); // closes the cycle // We can only break B -> C, so the result must be C -> D -> A -> B - self::assertSame(['C', 'D', 'A', 'B'], $this->computeResult()); + self::assertSame(['B', 'A', 'D', 'C'], $this->computeResult()); } public function testCycleRemovedByEliminatingLastOptionalEdge(): void @@ -75,7 +75,7 @@ public function testCycleRemovedByEliminatingLastOptionalEdge(): void $this->addEdge('B', 'D', true); $this->addEdge('D', 'A'); - self::assertSame(['C', 'D', 'A', 'B'], $this->computeResult()); + self::assertSame(['B', 'A', 'C', 'D'], $this->computeResult()); } public function testGH7180Example(): void @@ -89,7 +89,7 @@ public function testGH7180Example(): void $this->addEdge('F', 'E'); $this->addEdge('E', 'D'); - self::assertSame(['F', 'E', 'D', 'G'], $this->computeResult()); + self::assertSame(['G', 'D', 'E', 'F'], $this->computeResult()); } public function testCommitOrderingFromGH7259Test(): void @@ -106,9 +106,9 @@ public function testCommitOrderingFromGH7259Test(): void // the D -> A -> B ordering is important to break the cycle // on the nullable link. $correctOrders = [ - ['D', 'A', 'B', 'C'], - ['D', 'A', 'C', 'B'], - ['D', 'C', 'A', 'B'], + ['C', 'B', 'A', 'D'], + ['B', 'C', 'A', 'D'], + ['B', 'A', 'C', 'D'], ]; self::assertContains($this->computeResult(), $correctOrders); @@ -124,12 +124,12 @@ public function testCommitOrderingFromGH8349Case1Test(): void $this->addEdge('B', 'C', true); $this->addEdge('C', 'D', true); - // Many orderings are possible here, but the bottom line is D must be before A (it's the only hard requirement). + // Many orderings are possible here, but the bottom line is A must be before D (it's the only hard requirement). $result = $this->computeResult(); $indexA = array_search('A', $result, true); $indexD = array_search('D', $result, true); - self::assertTrue($indexD < $indexA); + self::assertTrue($indexD > $indexA); } public function testCommitOrderingFromGH8349Case2Test(): void @@ -141,7 +141,7 @@ public function testCommitOrderingFromGH8349Case2Test(): void $this->addEdge('A', 'B', true); // The B -> A requirement determines the result here - self::assertSame(['B', 'A'], $this->computeResult()); + self::assertSame(['A', 'B'], $this->computeResult()); } public function testNodesMaintainOrderWhenNoDepencency(): void @@ -153,6 +153,58 @@ public function testNodesMaintainOrderWhenNoDepencency(): void self::assertSame(['A', 'B', 'C'], $this->computeResult()); } + public function testNodesReturnedInDepthFirstOrder(): void + { + $this->addNodes('A', 'B', 'C'); + $this->addEdge('A', 'B'); + $this->addEdge('A', 'C'); + + // We start on A and find that it has two dependencies on B and C, + // added (as dependencies) in that order. + // So, first we continue the DFS on B, because that edge was added first. + // This gives the result order B, C, A. + self::assertSame(['B', 'C', 'A'], $this->computeResult()); + } + + public function testNodesReturnedInDepthFirstOrderWithEdgesInDifferentOrderThanNodes(): void + { + $this->addNodes('A', 'B', 'C'); + $this->addEdge('A', 'C'); + $this->addEdge('A', 'B'); + + // This is like testNodesReturnedInDepthFirstOrder, but it shows that for the two + // nodes B and C that A depends upon, the result will follow the order in which + // the edges were added. + self::assertSame(['C', 'B', 'A'], $this->computeResult()); + } + + public function testNodesReturnedInDepthFirstOrderWithDependingNodeLast(): void + { + $this->addNodes('B', 'C', 'A'); + $this->addEdge('A', 'B'); + $this->addEdge('A', 'C'); + + // This again is like testNodesReturnedInDepthFirstOrder, but this + // time the node A that depends on B and C is added as the last node. + // That means processing can go over B and C in the order they were given. + // The order in which edges are added is not relevant (!), since at the time + // the edges are evaluated, the nodes they point to have already been finished. + self::assertSame(['B', 'C', 'A'], $this->computeResult()); + } + + public function testNodesReturnedInDepthFirstOrderWithDependingNodeLastAndEdgeOrderInversed(): void + { + $this->addNodes('B', 'C', 'A'); + $this->addEdge('A', 'C'); + $this->addEdge('A', 'B'); + + // This again is like testNodesReturnedInDepthFirstOrderWithDependingNodeLast, but adds + // the edges in the opposing order. Still, the result order is the same (!). + // This may be surprising when comparing with testNodesReturnedInDepthFirstOrderWithEdgesInDifferentOrderThanNodes, + // where the result order depends upon the _edge_ order. + self::assertSame(['B', 'C', 'A'], $this->computeResult()); + } + public function testDetectSmallCycle(): void { $this->addNodes('A', 'B'); @@ -205,7 +257,7 @@ public function testDetectLargerCycleNotIncludingStartNode(): void $this->computeResult(); } catch (CycleDetectedException $exception) { self::assertEquals( - [$this->nodes['D'], $this->nodes['B'], $this->nodes['C'], $this->nodes['D']], + [$this->nodes['B'], $this->nodes['C'], $this->nodes['D'], $this->nodes['B']], $exception->getCycle() ); }