From 8ea62b95b8e4a8b3de000a59f018ec250b41d714 Mon Sep 17 00:00:00 2001 From: Guilherme Blanco Date: Wed, 25 Nov 2015 04:20:35 +0000 Subject: [PATCH] Tests around reported cases over DDC-2524 --- .../ORM/Internal/CommitOrderCalculator.php | 227 ++++++++++-------- lib/Doctrine/ORM/UnitOfWork.php | 39 +-- .../Tests/ORM/CommitOrderCalculatorTest.php | 43 +++- .../ORM/Functional/CascadeRemoveOrderTest.php | 155 ++++++++++++ .../Functional/OneToManyOrphanRemovalTest.php | 21 ++ 5 files changed, 348 insertions(+), 137 deletions(-) create mode 100644 tests/Doctrine/Tests/ORM/Functional/CascadeRemoveOrderTest.php diff --git a/lib/Doctrine/ORM/Internal/CommitOrderCalculator.php b/lib/Doctrine/ORM/Internal/CommitOrderCalculator.php index 9e069b0e3..11fd0a779 100644 --- a/lib/Doctrine/ORM/Internal/CommitOrderCalculator.php +++ b/lib/Doctrine/ORM/Internal/CommitOrderCalculator.php @@ -1,4 +1,5 @@ - * @author Guilherme Blanco + * @since 2.0 + * @author Guilherme Blanco + * @author Roman Borschel */ class CommitOrderCalculator { - const NOT_VISITED = 1; - const IN_PROGRESS = 2; - const VISITED = 3; + const NOT_VISITED = 0; + const IN_PROGRESS = 1; + const VISITED = 2; /** - * @var array + * Matrix of nodes (aka. vertex). + * Keys are provided hashes and values are the node definition objects. + * + * The node state definition contains the following properties: + * + * - state (integer) + * Whether the node is NOT_VISITED or IN_PROGRESS + * + * - value (object) + * Actual node value + * + * - dependencyList (array) + * Map of node dependencies defined as hashes. + * + * @var array */ - private $_nodeStates = array(); + private $nodeList = array(); /** - * The nodes to sort. + * Volatile variable holding calculated nodes during sorting process. * * @var array */ - private $_classes = array(); + private $sortedNodeList = array(); /** - * @var array - */ - private $_relatedClasses = array(); - - /** - * @var array - */ - private $_sorted = array(); - - /** - * Clears the current graph. + * Checks for node (vertex) existence in graph. * - * @return void + * @param string $hash + * + * @return boolean */ - public function clear() + public function hasNode($hash) { - $this->_classes = array(); - $this->_relatedClasses = array(); + return isset($this->nodeList[$hash]); } /** - * Gets a valid commit order for all current nodes. + * Adds a new node (vertex) to the graph, assigning its hash and value. * - * Uses a depth-first search (DFS) to traverse the graph. - * The desired topological sorting is the reverse postorder of these searches. + * @param string $hash + * @param object $node * - * @return array The list of ordered classes. + * @return void */ - public function getCommitOrder() + public function addNode($hash, $node) { - // Check whether we need to do anything. 0 or 1 node is easy. - $nodeCount = count($this->_classes); + $vertex = new \stdClass(); - if ($nodeCount <= 1) { - return ($nodeCount == 1) ? array_values($this->_classes) : array(); + $vertex->hash = $hash; + $vertex->state = self::NOT_VISITED; + $vertex->value = $node; + $vertex->dependencyList = array(); + + $this->nodeList[$hash] = $vertex; + } + + /** + * Adds a new dependency (edge) to the graph using their hashes. + * + * @param string $fromHash + * @param string $toHash + * @param integer $weight + * + * @return void + */ + public function addDependency($fromHash, $toHash, $weight) + { + $vertex = $this->nodeList[$fromHash]; + $edge = new \stdClass(); + + $edge->from = $fromHash; + $edge->to = $toHash; + $edge->weight = $weight; + + $vertex->dependencyList[$toHash] = $edge; + } + + /** + * Return a valid order list of all current nodes. + * The desired topological sorting is the reverse post order of these searches. + * + * {@internal Highly performance-sensitive method.} + * + * @return array + */ + public function sort() + { + foreach ($this->nodeList as $vertex) { + if ($vertex->state !== self::NOT_VISITED) { + continue; + } + + $this->visit($vertex); } - // Init - foreach ($this->_classes as $node) { - $this->_nodeStates[$node->name] = self::NOT_VISITED; - } + $sortedList = $this->sortedNodeList; - // Go - foreach ($this->_classes as $node) { - if ($this->_nodeStates[$node->name] == self::NOT_VISITED) { - $this->_visitNode($node); + $this->nodeList = array(); + $this->sortedNodeList = array(); + + return array_reverse($sortedList); + } + + /** + * Visit a given node definition for reordering. + * + * {@internal Highly performance-sensitive method.} + * + * @param \stdClass $vertex + */ + private function visit($vertex) + { + $vertex->state = self::IN_PROGRESS; + + foreach ($vertex->dependencyList as $edge) { + $adjacentVertex = $this->nodeList[$edge->to]; + + switch ($adjacentVertex->state) { + case self::VISITED: + // Do nothing, since node was already visited + break; + + case self::IN_PROGRESS: + if ($adjacentVertex->dependencyList[$vertex->hash]->weight < $edge->weight) { + $adjacentVertex->state = self::VISITED; + + $this->sortedNodeList[] = $adjacentVertex->value; + } + break; + + case self::NOT_VISITED: + $this->visit($adjacentVertex); } } - $sorted = array_reverse($this->_sorted); + if ($vertex->state !== self::VISITED) { + $vertex->state = self::VISITED; - $this->_sorted = $this->_nodeStates = array(); - - return $sorted; - } - - /** - * @param \Doctrine\ORM\Mapping\ClassMetadata $node - * - * @return void - */ - private function _visitNode($node) - { - $this->_nodeStates[$node->name] = self::IN_PROGRESS; - - if (isset($this->_relatedClasses[$node->name])) { - foreach ($this->_relatedClasses[$node->name] as $relatedNode) { - if ($this->_nodeStates[$relatedNode->name] == self::NOT_VISITED) { - $this->_visitNode($relatedNode); - } - } + $this->sortedNodeList[] = $vertex->value; } - - $this->_nodeStates[$node->name] = self::VISITED; - $this->_sorted[] = $node; } - - /** - * @param \Doctrine\ORM\Mapping\ClassMetadata $fromClass - * @param \Doctrine\ORM\Mapping\ClassMetadata $toClass - * - * @return void - */ - public function addDependency($fromClass, $toClass) - { - $this->_relatedClasses[$fromClass->name][] = $toClass; - } - - /** - * @param string $className - * - * @return bool - */ - public function hasClass($className) - { - return isset($this->_classes[$className]); - } - - /** - * @param \Doctrine\ORM\Mapping\ClassMetadata $class - * - * @return void - */ - public function addClass($class) - { - $this->_classes[$class->name] = $class; - } -} +} \ No newline at end of file diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index 3ba860b89..37a123d26 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -212,14 +212,6 @@ class UnitOfWork implements PropertyChangedListener */ private $em; - /** - * The calculator used to calculate the order in which changes to - * entities need to be written to the database. - * - * @var \Doctrine\ORM\Internal\CommitOrderCalculator - */ - private $commitOrderCalculator; - /** * The entity persister instances used to persist entity instances. * @@ -1138,11 +1130,11 @@ class UnitOfWork implements PropertyChangedListener foreach ($entityChangeSet as $entity) { $class = $this->em->getClassMetadata(get_class($entity)); - if ($calc->hasClass($class->name)) { + if ($calc->hasNode($class->name)) { continue; } - $calc->addClass($class); + $calc->addNode($class->name, $class); $newNodes[] = $class; } @@ -1156,13 +1148,16 @@ class UnitOfWork implements PropertyChangedListener $targetClass = $this->em->getClassMetadata($assoc['targetEntity']); - if ( ! $calc->hasClass($targetClass->name)) { - $calc->addClass($targetClass); + if ( ! $calc->hasNode($targetClass->name)) { + $calc->addNode($targetClass->name, $targetClass); $newNodes[] = $targetClass; } - $calc->addDependency($targetClass, $class); + $joinColumns = reset($assoc['joinColumns']); + $isNullable = isset($joinColumns['nullable']) ? $joinColumns['nullable'] : false; + + $calc->addDependency($targetClass->name, $class->name, $isNullable ? 0 : 1); // If the target class has mapped subclasses, these share the same dependency. if ( ! $targetClass->subClasses) { @@ -1172,18 +1167,18 @@ class UnitOfWork implements PropertyChangedListener foreach ($targetClass->subClasses as $subClassName) { $targetSubClass = $this->em->getClassMetadata($subClassName); - if ( ! $calc->hasClass($subClassName)) { - $calc->addClass($targetSubClass); + if ( ! $calc->hasNode($subClassName)) { + $calc->addNode($targetSubClass->name, $targetSubClass); $newNodes[] = $targetSubClass; } - $calc->addDependency($targetSubClass, $class); + $calc->addDependency($targetSubClass->name, $class->name, 1); } } } - return $calc->getCommitOrder(); + return $calc->sort(); } /** @@ -2354,11 +2349,7 @@ class UnitOfWork implements PropertyChangedListener */ public function getCommitOrderCalculator() { - if ($this->commitOrderCalculator === null) { - $this->commitOrderCalculator = new Internal\CommitOrderCalculator; - } - - return $this->commitOrderCalculator; + return new Internal\CommitOrderCalculator(); } /** @@ -2386,10 +2377,6 @@ class UnitOfWork implements PropertyChangedListener $this->readOnlyObjects = $this->visitedCollections = $this->orphanRemovals = array(); - - if ($this->commitOrderCalculator !== null) { - $this->commitOrderCalculator->clear(); - } } else { $visited = array(); diff --git a/tests/Doctrine/Tests/ORM/CommitOrderCalculatorTest.php b/tests/Doctrine/Tests/ORM/CommitOrderCalculatorTest.php index d4b1899fc..e9bf04459 100644 --- a/tests/Doctrine/Tests/ORM/CommitOrderCalculatorTest.php +++ b/tests/Doctrine/Tests/ORM/CommitOrderCalculatorTest.php @@ -2,6 +2,7 @@ namespace Doctrine\Tests\ORM; +use Doctrine\ORM\Internal\CommitOrderCalculator; use Doctrine\ORM\Mapping\ClassMetadata; /** @@ -17,7 +18,7 @@ class CommitOrderCalculatorTest extends \Doctrine\Tests\OrmTestCase protected function setUp() { - $this->_calc = new \Doctrine\ORM\Internal\CommitOrderCalculator(); + $this->_calc = new CommitOrderCalculator(); } public function testCommitOrdering1() @@ -28,21 +29,41 @@ class CommitOrderCalculatorTest extends \Doctrine\Tests\OrmTestCase $class4 = new ClassMetadata(__NAMESPACE__ . '\NodeClass4'); $class5 = new ClassMetadata(__NAMESPACE__ . '\NodeClass5'); - $this->_calc->addClass($class1); - $this->_calc->addClass($class2); - $this->_calc->addClass($class3); - $this->_calc->addClass($class4); - $this->_calc->addClass($class5); + $this->_calc->addNode($class1->name, $class1); + $this->_calc->addNode($class2->name, $class2); + $this->_calc->addNode($class3->name, $class3); + $this->_calc->addNode($class4->name, $class4); + $this->_calc->addNode($class5->name, $class5); - $this->_calc->addDependency($class1, $class2); - $this->_calc->addDependency($class2, $class3); - $this->_calc->addDependency($class3, $class4); - $this->_calc->addDependency($class5, $class1); + $this->_calc->addDependency($class1->name, $class2->name, 1); + $this->_calc->addDependency($class2->name, $class3->name, 1); + $this->_calc->addDependency($class3->name, $class4->name, 1); + $this->_calc->addDependency($class5->name, $class1->name, 1); - $sorted = $this->_calc->getCommitOrder(); + $sorted = $this->_calc->sort(); // There is only 1 valid ordering for this constellation $correctOrder = array($class5, $class1, $class2, $class3, $class4); + + $this->assertSame($correctOrder, $sorted); + } + + public function testCommitOrdering2() + { + $class1 = new ClassMetadata(__NAMESPACE__ . '\NodeClass1'); + $class2 = new ClassMetadata(__NAMESPACE__ . '\NodeClass2'); + + $this->_calc->addNode($class1->name, $class1); + $this->_calc->addNode($class2->name, $class2); + + $this->_calc->addDependency($class1->name, $class2->name, 0); + $this->_calc->addDependency($class2->name, $class1->name, 1); + + $sorted = $this->_calc->sort(); + + // There is only 1 valid ordering for this constellation + $correctOrder = array($class2, $class1); + $this->assertSame($correctOrder, $sorted); } } diff --git a/tests/Doctrine/Tests/ORM/Functional/CascadeRemoveOrderTest.php b/tests/Doctrine/Tests/ORM/Functional/CascadeRemoveOrderTest.php new file mode 100644 index 000000000..9fd1648e0 --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Functional/CascadeRemoveOrderTest.php @@ -0,0 +1,155 @@ +_schemaTool->createSchema(array( + $this->_em->getClassMetadata(__NAMESPACE__ . '\CascadeRemoveOrderEntityO'), + $this->_em->getClassMetadata(__NAMESPACE__ . '\CascadeRemoveOrderEntityG'), + )); + } + + protected function tearDown() + { + parent::tearDown(); + + $this->_schemaTool->dropSchema(array( + $this->_em->getClassMetadata(__NAMESPACE__ . '\CascadeRemoveOrderEntityO'), + $this->_em->getClassMetadata(__NAMESPACE__ . '\CascadeRemoveOrderEntityG'), + )); + } + + public function testSingle() + { + $eO = new CascadeRemoveOrderEntityO(); + $eG = new CascadeRemoveOrderEntityG($eO); + + $this->_em->persist($eO); + $this->_em->flush(); + $this->_em->clear(); + + $eOloaded = $this->_em->find('Doctrine\Tests\ORM\Functional\CascadeRemoveOrderEntityO', $eO->getId()); + + $this->_em->remove($eOloaded); + $this->_em->flush(); + } + + public function testMany() + { + $eO = new CascadeRemoveOrderEntityO(); + $eG1 = new CascadeRemoveOrderEntityG($eO); + $eG2 = new CascadeRemoveOrderEntityG($eO); + $eG3 = new CascadeRemoveOrderEntityG($eO); + + $eO->setOneToOneG($eG2); + + $this->_em->persist($eO); + $this->_em->flush(); + $this->_em->clear(); + + $eOloaded = $this->_em->find('Doctrine\Tests\ORM\Functional\CascadeRemoveOrderEntityO', $eO->getId()); + + $this->_em->remove($eOloaded); + $this->_em->flush(); + } +} + +/** + * @Entity + */ +class CascadeRemoveOrderEntityO +{ + /** + * @Id @Column(type="integer") + * @GeneratedValue + */ + private $id; + + /** + * @OneToOne(targetEntity="Doctrine\Tests\ORM\Functional\CascadeRemoveOrderEntityG") + * @JoinColumn(nullable=true, onDelete="SET NULL") + */ + private $oneToOneG; + + /** + * @OneToMany( + * targetEntity="Doctrine\Tests\ORM\Functional\CascadeRemoveOrderEntityG", + * mappedBy="ownerO", + * cascade={"persist", "remove"} + * ) + */ + private $oneToManyG; + + + public function __construct() + { + $this->oneToManyG = new ArrayCollection(); + } + + public function getId() + { + return $this->id; + } + + public function setOneToOneG(CascadeRemoveOrderEntityG $eG) + { + $this->oneToOneG = $eG; + } + + public function getOneToOneG() + { + return $this->oneToOneG; + } + + public function addOneToManyG(CascadeRemoveOrderEntityG $eG) + { + $this->oneToManyG->add($eG); + } + + public function getOneToManyGs() + { + return $this->oneToManyG->toArray(); + } +} + +/** + * @Entity + */ +class CascadeRemoveOrderEntityG +{ + /** + * @Id @Column(type="integer") + * @GeneratedValue + */ + private $id; + + /** + * @ManyToOne( + * targetEntity="Doctrine\Tests\ORM\Functional\CascadeRemoveOrderEntityO", + * inversedBy="oneToMany" + * ) + */ + private $ownerO; + + public function __construct(CascadeRemoveOrderEntityO $eO, $position=1) + { + $this->position = $position; + $this->ownerO= $eO; + $this->ownerO->addOneToManyG($this); + } + + public function getId() + { + return $this->id; + } +} \ No newline at end of file diff --git a/tests/Doctrine/Tests/ORM/Functional/OneToManyOrphanRemovalTest.php b/tests/Doctrine/Tests/ORM/Functional/OneToManyOrphanRemovalTest.php index 640f078d9..e10c1f923 100644 --- a/tests/Doctrine/Tests/ORM/Functional/OneToManyOrphanRemovalTest.php +++ b/tests/Doctrine/Tests/ORM/Functional/OneToManyOrphanRemovalTest.php @@ -97,6 +97,27 @@ class OneToManyOrphanRemovalTest extends \Doctrine\Tests\OrmFunctionalTestCase $this->assertEquals(1, count($result), 'CmsPhonenumber should be removed by orphanRemoval'); } + /** + * @group DDC-2524 + */ + public function testOrphanRemovalClearCollectionAndAddNew() + { + $user = $this->_em->find('Doctrine\Tests\Models\CMS\CmsUser', $this->userId); + $newPhone = new CmsPhonenumber(); + + $newPhone->phonenumber = '654321'; + + $user->getPhonenumbers()->clear(); + $user->addPhonenumber($newPhone); + + $this->_em->flush(); + + $query = $this->_em->createQuery('SELECT p FROM Doctrine\Tests\Models\CMS\CmsPhonenumber p'); + $result = $query->getResult(); + + $this->assertEquals(1, count($result), 'Old CmsPhonenumbers should be removed by orphanRemoval and new one added'); + } + /** * @group DDC-1496 */