OrderResolver: fixed wrong order when files had identical logical nam…

…es [closes #18]
nextras · Jul 17, 2015 · c184cf1 · c184cf1
1 parent d3f6e4a
commit c184cf1
Show file tree

Hide file tree

Showing 2 changed files with 112 additions and 5 deletions.
diff --git a/src/Engine/OrderResolver.php b/src/Engine/OrderResolver.php
@@ -32,7 +32,7 @@ public function resolve(array $migrations, array $groups, array $files, $mode)
 		$this->validateGroups($groups);
 
 		if ($mode === Runner::MODE_RESET) {
-			return $this->sortFiles($files);
+			return $this->sortFiles($files, $groups);
 		} elseif ($mode !== Runner::MODE_CONTINUE) {
 			throw new LogicException('Unsupported mode.');
 		}
@@ -82,7 +82,7 @@ public function resolve(array $migrations, array $groups, array $files, $mode)
 		}
 
 		$files = $this->getFlatFiles($files);
-		$files = $this->sortFiles($files);
+		$files = $this->sortFiles($files, $groups);
 		if ($files && $lastMigration) {
 			$firstFile = reset($files);
 			if (strcmp($firstFile->name, $lastMigration->filename) < 0) {
@@ -99,18 +99,64 @@ public function resolve(array $migrations, array $groups, array $files, $mode)
 
 	/**
 	 * @param  File[] $files
+	 * @param  array  $groups (name => Group)
 	 * @return File[] sorted
 	 */
-	protected function sortFiles(array $files)
+	protected function sortFiles(array $files, array $groups)
 	{
-		usort($files, function (File $a, File $b) {
-			return strcmp($a->name, $b->name);
+		usort($files, function (File $a, File $b) use ($groups) {
+			$cmp = strcmp($a->name, $b->name);
+			if ($cmp === 0 && $a !== $b) {
+				$cmpA = $this->isGroupDependentOn($groups, $a->group, $b->group);
+				$cmpB = $this->isGroupDependentOn($groups, $b->group, $a->group);
+				if ($cmpA xor $cmpB) {
+					$cmp = ($cmpA ? -1 : 1);
+
+				} else {
+					throw new LogicException(sprintf(
+						'Unable to determine order for migrations "%s/%s" and "%s/%s".',
+						$a->group->name, $a->name, $b->group->name, $b->name
+					));
+				}
+			}
+
+			return $cmp;
 		});
 
 		return $files;
 	}
 
 
+	/**
+	 * Returns TRUE if groupA depends on groupB.
+	 *
+	 * @param  array  $groups (name => Group)
+	 * @param  Group $groupA
+	 * @param  Group $groupB
+	 * @return bool
+	 */
+	protected function isGroupDependentOn(array $groups, Group $groupA, Group $groupB)
+	{
+		$visited = [];
+		$queue = $groupB->dependencies;
+		while ($node = array_shift($queue)) {
+			if (isset($visited[$node])) {
+				continue;
+			}
+
+			if ($groupA->name === $node) {
+				return TRUE;
+			}
+
+			$visited[$node] = TRUE;
+			foreach ($groups[$node]->dependencies as $dep) {
+				$queue[] = $dep;
+			}
+		}
+		return FALSE;
+	}
+
+
 	protected function getAssocMigrations(array $migrations)
 	{
 		$assoc = array();

diff --git a/tests/cases/unit/OrderResolverTest.phpt b/tests/cases/unit/OrderResolverTest.phpt
@@ -180,6 +180,25 @@ class OrderResolverTest extends Tester\TestCase
 	}
 
 
+	public function testTopologicalOrder()
+	{
+		$resolver = new OrderResolver();
+
+		$groupA = $this->createGroup('structures');
+		$groupB = $this->createGroup('data', TRUE, ['structures']);
+
+		$fileA = $this->createFile('foo', $groupA);
+		$fileB = $this->createFile('foo', $groupB);
+
+		Assert::same([$fileA, $fileB], $resolver->resolve(
+			[],
+			[$groupA, $groupB],
+			[$fileA, $fileB],
+			Runner::MODE_CONTINUE
+		));
+	}
+
+
 	public function testErrorRemovedFile()
 	{
 		$resolver = new OrderResolver;
@@ -317,6 +336,48 @@ class OrderResolverTest extends Tester\TestCase
 	}
 
 
+	public function testErrorAmbiguousLogicalName()
+	{
+		$resolver = new OrderResolver();
+
+		$groupA = $this->createGroup('structures');
+		$groupB = $this->createGroup('data');
+
+		$fileA = $this->createFile('foo', $groupA);
+		$fileB = $this->createFile('foo', $groupB);
+
+		Assert::exception(function () use ($resolver, $groupA, $groupB, $fileA, $fileB) {
+			$resolver->resolve(
+				[],
+				[$groupA, $groupB],
+				[$fileA, $fileB],
+				Runner::MODE_CONTINUE
+			);
+		}, 'Nextras\Migrations\LogicException', 'Unable to determine order for migrations "data/foo" and "structures/foo".');
+	}
+
+
+	public function testErrorAmbiguousLogicalNameCyclic()
+	{
+		$resolver = new OrderResolver();
+
+		$groupA = $this->createGroup('structures', TRUE, ['data']);
+		$groupB = $this->createGroup('data', TRUE, ['structures']);
+
+		$fileA = $this->createFile('foo', $groupA);
+		$fileB = $this->createFile('foo', $groupB);
+
+		Assert::exception(function () use ($resolver, $groupA, $groupB, $fileA, $fileB) {
+			$resolver->resolve(
+				[],
+				[$groupA, $groupB],
+				[$fileA, $fileB],
+				Runner::MODE_CONTINUE
+			);
+		}, 'Nextras\Migrations\LogicException', 'Unable to determine order for migrations "data/foo" and "structures/foo".');
+	}
+
+
 	private function createMigration($groupName, $fileName, $checksum = NULL, $completed = TRUE)
 	{
 		$migration = new Migration;