Merge pull request #11086 from mpdude/11058-revisited

Avoid an inconsistency in topological sort result order

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<object>
      */
     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];
     }
 }

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

tests/Doctrine/Tests/ORM/Functional/Ticket/GH11058Test.php

@@ -0,0 +1,146 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Doctrine\Tests\ORM\Functional\Ticket;
+
+use Doctrine\Common\Collections\ArrayCollection;
+use Doctrine\Common\Collections\Collection;
+use Doctrine\ORM\Mapping as ORM;
+use Doctrine\Tests\OrmFunctionalTestCase;
+
+class GH11058Test extends OrmFunctionalTestCase
+{
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        $this->setUpEntitySchema([
+            GH11058Parent::class,
+            GH11058Child::class,
+        ]);
+    }
+
+    public function testChildrenInsertedInOrderOfPersistCalls1WhenParentPersistedLast(): void
+    {
+        [$parent, $child1, $child2] = $this->createParentWithTwoChildEntities();
+
+        $this->_em->persist($child1);
+        $this->_em->persist($child2);
+        $this->_em->persist($parent);
+        $this->_em->flush();
+
+        self::assertTrue($child1->id < $child2->id);
+    }
+
+    public function testChildrenInsertedInOrderOfPersistCalls2WhenParentPersistedLast(): void
+    {
+        [$parent, $child1, $child2] = $this->createParentWithTwoChildEntities();
+
+        $this->_em->persist($child2);
+        $this->_em->persist($child1);
+        $this->_em->persist($parent);
+        $this->_em->flush();
+
+        self::assertTrue($child2->id < $child1->id);
+    }
+
+    public function testChildrenInsertedInOrderOfPersistCalls1WhenParentPersistedFirst(): void
+    {
+        [$parent, $child1, $child2] = $this->createParentWithTwoChildEntities();
+
+        $this->_em->persist($parent);
+        $this->_em->persist($child1);
+        $this->_em->persist($child2);
+        $this->_em->flush();
+
+        self::assertTrue($child1->id < $child2->id);
+    }
+
+    public function testChildrenInsertedInOrderOfPersistCalls2WhenParentPersistedFirst(): void
+    {
+        [$parent, $child1, $child2] = $this->createParentWithTwoChildEntities();
+
+        $this->_em->persist($parent);
+        $this->_em->persist($child2);
+        $this->_em->persist($child1);
+        $this->_em->flush();
+
+        self::assertTrue($child2->id < $child1->id);
+    }
+
+    private function createParentWithTwoChildEntities(): array
+    {
+        $parent = new GH11058Parent();
+        $child1 = new GH11058Child();
+        $child2 = new GH11058Child();
+
+        $parent->addChild($child1);
+        $parent->addChild($child2);
+
+        return [$parent, $child1, $child2];
+    }
+}
+
+/**
+ * @ORM\Entity()
+ */
+class GH11058Parent
+{
+    /**
+     * @ORM\Id
+     * @ORM\Column(type="integer")
+     * @ORM\GeneratedValue
+     *
+     * @var int
+     */
+    public $id;
+
+    /**
+     * @ORM\OneToMany(targetEntity="GH11058Child", mappedBy="parent")
+     *
+     * @var Collection<int, GH11058Child>
+     */
+    public $children;
+
+    public function __construct()
+    {
+        $this->children = new ArrayCollection();
+    }
+
+    public function addChild(GH11058Child $child): void
+    {
+        if (! $this->children->contains($child)) {
+            $this->children->add($child);
+            $child->setParent($this);
+        }
+    }
+}
+
+/**
+ * @ORM\Entity()
+ */
+class GH11058Child
+{
+    /**
+     * @ORM\Id
+     * @ORM\Column(type="integer")
+     * @ORM\GeneratedValue
+     *
+     * @var int
+     */
+    public $id;
+
+    /**
+     * @ORM\ManyToOne(targetEntity="GH11058Parent", inversedBy="children")
+     *
+     * @var GH11058Parent
+     */
+    public $parent;
+
+    public function setParent(GH11058Parent $parent): void
+    {
+        $this->parent = $parent;
+        $parent->addChild($this);
+    }
+}

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