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

Jump to bottom

Fix order of sorting for cyclic dependencies #481

Open
wants to merge 5 commits into
base: 1.7.x
Choose a base branch
from
Open

Fix order of sorting for cyclic dependencies #481

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
914064c
Fix order of sorting for cyclic dependencies
esserj Jun 20, 2024
a6d1281
fix missing cylic dependency and add permutation covarage
esserj Jun 21, 2024
3300921
make test more readable
esserj Jun 21, 2024
e928373
cleanup
esserj Jun 21, 2024
b710339
make internal method private
esserj Jul 30, 2024
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
41 changes: 31 additions & 10 deletions src/Sorter/TopologicalSorter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,9 @@
use Doctrine\ORM\Mapping\ClassMetadata;
use RuntimeException;

use function get_class;
use function count;
use function sprintf;
use function uasort;

/**
* TopologicalSorter is an ordering algorithm for directed graphs (DG) and/or
Expand Down Expand Up @@ -101,6 +102,9 @@
*/
public function sort()
{
uasort($this->nodeList, static function (Vertex $a, Vertex $b) {
return count($a->dependencyList) > count($b->dependencyList) ? 1 : -1;
});
foreach ($this->nodeList as $definition) {
if ($definition->state !== Vertex::NOT_VISITED) {
continue;
Expand Down Expand Up @@ -132,15 +136,7 @@
$definition->state = Vertex::IN_PROGRESS;

foreach ($definition->dependencyList as $dependency) {
if (! isset($this->nodeList[$dependency])) {
throw new RuntimeException(sprintf(
'Fixture "%s" has a dependency of fixture "%s", but it not listed to be loaded.',
get_class($definition->value),
$dependency,
));
}

$childDefinition = $this->nodeList[$dependency];
$childDefinition = $this->getDefinition($dependency, $definition);

// allow self referencing classes
if ($definition === $childDefinition) {
Expand All @@ -167,6 +163,18 @@
);
}

// first do the rest of the unvisited children of the "in_progress" child before we continue
// with the current one, to be sure that there are no other dependencies that need
// to be cleared before this ($definition) node
foreach ($childDefinition->dependencyList as $childDependency) {
$nestedChildDefinition = $this->getDefinition($childDependency, $childDefinition);
if ($nestedChildDefinition->state !== Vertex::NOT_VISITED) {
continue;
}

$this->visit($nestedChildDefinition);

Check warning on line 175 in src/Sorter/TopologicalSorter.php

View check run for this annotation

Codecov / codecov/patch

src/Sorter/TopologicalSorter.php#L175 

Added line #L175 was not covered by tests
}

break;
case Vertex::NOT_VISITED:
$this->visit($childDefinition);
Expand All @@ -177,4 +185,17 @@

$this->sortedNodeList[] = $definition->value;
}

private function getDefinition(string $dependency, Vertex $definition): Vertex
{
if (! isset($this->nodeList[$dependency])) {
throw new RuntimeException(sprintf(
'Fixture "%s" has a dependency of fixture "%s", but it\'s not listed to be loaded.',
$definition->value->getName(),
$dependency,
));
}

return $this->nodeList[$dependency];
}
}
88 changes: 73 additions & 15 deletions tests/Common/DataFixtures/Sorter/TopologicalSorterTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,13 @@
use Doctrine\Tests\Common\DataFixtures\BaseTestCase;
use RuntimeException;

use function array_map;
use function array_search;
use function array_splice;
use function array_unshift;
use function count;
use function implode;

/**
* TopologicalSorter tests.
*
Expand Down Expand Up @@ -76,28 +83,79 @@ public function testSuccessSortMultiDependency(): void
self::assertSame($correctList, $sortedList);
}

public function testSortCyclicDependency(): void
public static function cyclicNodePermutationsDataProvider(): iterable
{
$sorter = new TopologicalSorter();
$list[] = new ClassMetadata('1');
$list[] = new ClassMetadata('2');
$list[] = new ClassMetadata('3');
$list[] = new ClassMetadata('4');
$list[] = new ClassMetadata('5');

$node1 = new ClassMetadata('1');
$node2 = new ClassMetadata('2');
$node3 = new ClassMetadata('3');
foreach (self::permute($list) as $list) {
$label = 'Node order: ' . implode(',', self::nodeNames($list));

$sorter->addNode('1', $node1);
$sorter->addNode('2', $node2);
$sorter->addNode('3', $node3);
yield $label => [$list];
}
}

$sorter->addDependency('1', '2');
$sorter->addDependency('2', '3');
$sorter->addDependency('3', '1');
/** @param ClassMetadata[] $list */
private static function nodeNames(array $list): array
{
return array_map(static function (ClassMetadata $node) {
return $node->getName();
}, $list);
}

$sortedList = $sorter->sort();
$correctList = [$node3, $node2, $node1];
private static function permute(array $items, array $permutation = [], array &$result = []): array
{
if (empty($items)) {
$result[] = $permutation;
} else {
for ($i = count($items) - 1; $i >= 0; --$i) {
$newItems = $items;
$newPermutation = $permutation;
[$item] = array_splice($newItems, $i, 1);
array_unshift($newPermutation, $item);
self::permute($newItems, $newPermutation, $result);
}
}

return $result;
}

self::assertSame($correctList, $sortedList);
/** @dataProvider cyclicNodePermutationsDataProvider */
public function testSortCyclicDependency(array $nodes): void
{
$sorter = new TopologicalSorter();

$sorter->sort();
foreach ($nodes as $node) {
$sorter->addNode($node->getName(), $node);
}

$sorter->addDependency('1', '2');
$sorter->addDependency('2', '3');
$sorter->addDependency('3', '4');
$sorter->addDependency('4', '2');
$sorter->addDependency('2', '5');

$sortedList = self::nodeNames($sorter->sort());
$node1Position = array_search('1', $sortedList);
$node2Position = array_search('2', $sortedList);
$node3Position = array_search('3', $sortedList);
$node4Position = array_search('4', $sortedList);
$node5Position = array_search('5', $sortedList);

self::assertTrue($node5Position < $node2Position, '5 should come before 2');
self::assertTrue($node5Position < $node3Position, '5 should come before 3');
self::assertTrue($node5Position < $node4Position, '5 should come before 4');
self::assertTrue($node1Position > $node5Position, '1 should come after 2');
self::assertTrue($node1Position > $node3Position, '1 should come after 3');
self::assertTrue($node1Position > $node4Position, '1 should come after 4');

// these a cyclic and sort order is based on the order of the nodes
self::assertContains('2', $sortedList);
self::assertContains('3', $sortedList);
self::assertContains('4', $sortedList);
}

public function testFailureSortCyclicDependency(): void
Expand Down