Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Tests around reported cases over DDC-2524 #1570

Merged
merged 1 commit into from
Dec 1, 2015
Merged

[RFC] Tests around reported cases over DDC-2524 #1570

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
197 changes: 112 additions & 85 deletions lib/Doctrine/ORM/Internal/CommitOrderCalculator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
<?php

/*
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
* "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
Expand All @@ -20,137 +21,163 @@
namespace Doctrine\ORM\Internal;

/**
* The CommitOrderCalculator is used by the UnitOfWork to sort out the
* correct order in which changes to entities need to be persisted.
* CommitOrderCalculator implements topological sorting, which is an ordering
* algorithm for directed graphs (DG) and/or directed acyclic graphs (DAG) by
* using a depth-first searching (DFS) to traverse the graph built in memory.
* This algorithm have a linear running time based on nodes (V) and dependency
* between the nodes (E), resulting in a computational complexity of O(V + E).
*
* @since 2.0
* @author Roman Borschel <roman@code-factory.org>
* @author Guilherme Blanco <guilhermeblanco@hotmail.com>
* @since 2.0
* @author Guilherme Blanco <guilhermeblanco@hotmail.com>
* @author Roman Borschel <roman@code-factory.org>
*/
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
*/
private $_nodeStates = array();

/**
* The nodes to sort.
* Matrix of nodes (aka. vertex).
* Keys are provided hashes and values are the node definition objects.
*
* @var array
*/
private $_classes = array();

/**
* @var array
* The node state definition contains the following properties:
*
* - <b>state</b> (integer)
* Whether the node is NOT_VISITED or IN_PROGRESS
*
* - <b>value</b> (object)
* Actual node value
*
* - <b>dependencyList</b> (array<string>)
* Map of node dependencies defined as hashes.
*
* @var array<stdClass>
*/
private $_relatedClasses = array();
private $nodeList = array();

/**
* Volatile variable holding calculated nodes during sorting process.
*
* @var array
*/
private $_sorted = array();
private $sortedNodeList = 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();
}

// Init
foreach ($this->_classes as $node) {
$this->_nodeStates[$node->name] = self::NOT_VISITED;
}
$vertex->hash = $hash;
$vertex->state = self::NOT_VISITED;
$vertex->value = $node;
$vertex->dependencyList = array();

// Go
foreach ($this->_classes as $node) {
if ($this->_nodeStates[$node->name] == self::NOT_VISITED) {
$this->_visitNode($node);
}
}

$sorted = array_reverse($this->_sorted);

$this->_sorted = $this->_nodeStates = array();

return $sorted;
$this->nodeList[$hash] = $vertex;
}

/**
* @param \Doctrine\ORM\Mapping\ClassMetadata $node
* Adds a new dependency (edge) to the graph using their hashes.
*
* @param string $fromHash
* @param string $toHash
* @param integer $weight
*
* @return void
*/
private function _visitNode($node)
public function addDependency($fromHash, $toHash, $weight)
{
$this->_nodeStates[$node->name] = self::IN_PROGRESS;
$vertex = $this->nodeList[$fromHash];
$edge = new \stdClass();

if (isset($this->_relatedClasses[$node->name])) {
foreach ($this->_relatedClasses[$node->name] as $relatedNode) {
if ($this->_nodeStates[$relatedNode->name] == self::NOT_VISITED) {
$this->_visitNode($relatedNode);
}
}
}
$edge->from = $fromHash;
$edge->to = $toHash;
$edge->weight = $weight;

$this->_nodeStates[$node->name] = self::VISITED;
$this->_sorted[] = $node;
$vertex->dependencyList[$toHash] = $edge;
}

/**
* @param \Doctrine\ORM\Mapping\ClassMetadata $fromClass
* @param \Doctrine\ORM\Mapping\ClassMetadata $toClass
* Return a valid order list of all current nodes.
* The desired topological sorting is the reverse post order of these searches.
*
* @return void
*/
public function addDependency($fromClass, $toClass)
{
$this->_relatedClasses[$fromClass->name][] = $toClass;
}

/**
* @param string $className
* {@internal Highly performance-sensitive method.}
*
* @return bool
* @return array
*/
public function hasClass($className)
public function sort()
{
return isset($this->_classes[$className]);
foreach ($this->nodeList as $vertex) {
if ($vertex->state !== self::NOT_VISITED) {
continue;
}

$this->visit($vertex);
}

$sortedList = $this->sortedNodeList;

$this->nodeList = array();
$this->sortedNodeList = array();

return array_reverse($sortedList);
}

/**
* @param \Doctrine\ORM\Mapping\ClassMetadata $class
* Visit a given node definition for reordering.
*
* @return void
* {@internal Highly performance-sensitive method.}
*
* @param \stdClass $vertex
*/
public function addClass($class)
private function visit($vertex)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removing public properties is a BC break, which must not be done in minor releases. You broke doctrine/data-fixtures. Please add the methods back

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree here - the merge has to be reverted and delayed to 3.x :-(

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class was never meant to be used by anyone else, not even other Doctrine projects.
data-fixtures should copy the new class and use on its own

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a more detailed reason why I think this should not be reverted and why data-fixtures usage of the old class here is broken: doctrine/data-fixtures#212 (comment)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A solution as been decided for this BC break ? We temporarily fix the Doctrine ORM version to 2.5.2 on our project.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fsevestre yes, we'll not revert this commit. Explanation is available at data-fixtures comment I left in previous message.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@guilhermeblanco Does that mean this fix will land in the next 2.5.x release?

I've got a different issue with commit ordering that this PR solves, so I really need it soon.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upon further review, it looks like my issue is likely related to cyclic dependencies, which explains why this fix would solve my issue.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'll only land in 2.6, as this introduces a BC break for data-fixtures project.

{
$this->_classes[$class->name] = $class;
$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);
}
}

if ($vertex->state !== self::VISITED) {
$vertex->state = self::VISITED;

$this->sortedNodeList[] = $vertex->value;
}
}
}
}
39 changes: 13 additions & 26 deletions lib/Doctrine/ORM/UnitOfWork.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand Down Expand Up @@ -1138,11 +1130,11 @@ private function getCommitOrder(array $entityChangeSet = null)
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;
}
Expand All @@ -1156,13 +1148,16 @@ private function getCommitOrder(array $entityChangeSet = null)

$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) {
Expand All @@ -1172,18 +1167,18 @@ private function getCommitOrder(array $entityChangeSet = null)
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();
}

/**
Expand Down Expand Up @@ -2354,11 +2349,7 @@ public function lock($entity, $lockMode, $lockVersion = null)
*/
public function getCommitOrderCalculator()
{
if ($this->commitOrderCalculator === null) {
$this->commitOrderCalculator = new Internal\CommitOrderCalculator;
}

return $this->commitOrderCalculator;
return new Internal\CommitOrderCalculator();
}

/**
Expand Down Expand Up @@ -2386,10 +2377,6 @@ public function clear($entityName = null)
$this->readOnlyObjects =
$this->visitedCollections =
$this->orphanRemovals = array();

if ($this->commitOrderCalculator !== null) {
$this->commitOrderCalculator->clear();
}
} else {
$visited = array();

Expand Down
Loading