Sort app scripts topologically by its dependencies

Implement a proper topological sorting algorithm. Based on the
implementation by https://github.com/marcj/topsort.php

Logs an error in case a circular dependency is detected.

Fixes: #30278

Signed-off-by: Jonas Meurer <jonas@freesources.org>

2 files changed, 151 insertions, 15 deletions

diff --git a/tests/lib/AppScriptSortTest.php b/tests/lib/AppScriptSortTest.php
new file mode 100644
index 00000000000..2db3e2282d9
--- /dev/null
+++ b/tests/lib/AppScriptSortTest.php
@@ -0,0 +1,124 @@
+<?php
+/**
+ * Copyright (c) 2012 Lukas Reschke <lukas@statuscode.ch>
+ * This file is licensed under the Affero General Public License version 3 or
+ * later.
+ * See the COPYING-README file.
+ */
+
+namespace Test;
+
+use OC\AppScriptDependency;
+use OC\AppScriptSort;
+use Psr\Log\LoggerInterface;
+
+/**
+ * Class AppScriptSortTest
+ *
+ * @package Test
+ * @group DB
+ */
+class AppScriptSortTest extends \Test\TestCase {
+	private $logger;
+
+	protected function setUp(): void {
+		$this->logger = $this->getMockBuilder(LoggerInterface::class)
+			->disableOriginalConstructor()
+			->getMock();
+
+		parent::setUp();
+	}
+
+	public function testSort(): void {
+		$scripts = [
+			'first' => ['myFirstJSFile'],
+			'core' => [
+				'core/js/myFancyJSFile1',
+				'core/js/myFancyJSFile4',
+				'core/js/myFancyJSFile5',
+				'core/js/myFancyJSFile1',
+			],
+			'files' => ['files/js/myFancyJSFile2'],
+			'myApp5' => ['myApp5/js/myApp5JSFile'],
+			'myApp' => ['myApp/js/myFancyJSFile3'],
+			'myApp4' => ['myApp4/js/myApp4JSFile'],
+			'myApp3' => ['myApp3/js/myApp3JSFile'],
+			'myApp2' => ['myApp2/js/myApp2JSFile'],
+		];
+		$scriptDeps = [
+			'first' => new AppScriptDependency('first', ['core']),
+			'core' => new AppScriptDependency('core', ['core']),
+			'files' => new AppScriptDependency('files', ['core']),
+			'myApp5' => new AppScriptDependency('myApp5', ['myApp2']),
+			'myApp' => new AppScriptDependency('myApp', ['core']),
+			'myApp4' => new AppScriptDependency('myApp4', ['myApp3']),
+			'myApp3' => new AppScriptDependency('myApp3', ['myApp2']),
+			'myApp2' => new AppScriptDependency('myApp2', ['myApp']),
+		];
+
+		// No circular dependency is detected and logged as an error
+		$this->logger->expects(self::never())->method('error');
+
+		$scriptSort = new AppScriptSort($this->logger);
+		$sortedScripts = $scriptSort->sort($scripts, $scriptDeps);
+
+		$sortedScriptKeys = array_keys($sortedScripts);
+
+		// Core should appear first
+		$this->assertEquals(
+			0,
+			array_search('core', $sortedScriptKeys, true)
+		);
+
+		// Dependencies should appear before their children
+		$this->assertLessThan(
+			array_search('files', $sortedScriptKeys, true),
+			array_search('core', $sortedScriptKeys, true)
+		);
+		$this->assertLessThan(
+			array_search('myApp2', $sortedScriptKeys, true),
+			array_search('myApp', $sortedScriptKeys, true)
+		);
+		$this->assertLessThan(
+			array_search('myApp3', $sortedScriptKeys, true),
+			array_search('myApp2', $sortedScriptKeys, true)
+		);
+		$this->assertLessThan(
+			array_search('myApp4', $sortedScriptKeys, true),
+			array_search('myApp3', $sortedScriptKeys, true)
+		);
+		$this->assertLessThan(
+			array_search('myApp5', $sortedScriptKeys, true),
+			array_search('myApp2', $sortedScriptKeys, true)
+		);
+
+		// All apps still there
+		foreach ($scripts as $app => $_) {
+			$this->assertContains($app, $sortedScriptKeys);
+		}
+	}
+
+	public function testSortCircularDependency(): void {
+		$scripts = [
+			'circular' => ['circular/js/file1'],
+			'dependency' => ['dependency/js/file2'],
+		];
+		$scriptDeps = [
+			'circular' => new AppScriptDependency('circular', ['dependency']),
+			'dependency' => new AppScriptDependency('dependency', ['circular']),
+		];
+
+		// A circular dependency is detected and logged as an error
+		$this->logger->expects(self::once())->method('error');
+
+		$scriptSort = new AppScriptSort($this->logger);
+		$sortedScripts = $scriptSort->sort($scripts, $scriptDeps);
+
+		$sortedScriptKeys = array_keys($sortedScripts);
+
+		// All apps still there
+		foreach ($scripts as $app => $_) {
+			$this->assertContains($app, $sortedScriptKeys);
+		}
+	}
+}
diff --git a/tests/lib/UtilTest.php b/tests/lib/UtilTest.php
index 1b244d29dd5..c394792a72f 100644
--- a/tests/lib/UtilTest.php
+++ b/tests/lib/UtilTest.php
@@ -236,6 +236,7 @@ class UtilTest extends \Test\TestCase {
 	}
 
 	public function testAddScript() {
+		\OCP\Util::addScript('first', 'myFirstJSFile');
 		\OCP\Util::addScript('core', 'myFancyJSFile1');
 		\OCP\Util::addScript('files', 'myFancyJSFile2', 'core');
 		\OCP\Util::addScript('myApp5', 'myApp5JSFile', 'myApp2');
@@ -250,42 +251,44 @@ class UtilTest extends \Test\TestCase {
 		\OCP\Util::addScript('myApp3', 'myApp3JSFile', 'myApp2');
 		\OCP\Util::addScript('myApp2', 'myApp2JSFile', 'myApp');
 
+		$scripts = \OCP\Util::getScripts();
+
 		// Core should appear first
 		$this->assertEquals(
 			0,
-			array_search('core/js/myFancyJSFile1', \OCP\Util::getScripts(), true)
+			array_search('core/js/myFancyJSFile1', $scripts, true)
 		);
 		$this->assertEquals(
 			1,
-			array_search('core/js/myFancyJSFile4', \OCP\Util::getScripts(), true)
+			array_search('core/js/myFancyJSFile4', $scripts, true)
 		);
 
 		// Dependencies should appear before their children
 		$this->assertLessThan(
-			array_search('files/js/myFancyJSFile2', \OCP\Util::getScripts(), true),
-			array_search('core/js/myFancyJSFile3', \OCP\Util::getScripts(), true)
+			array_search('files/js/myFancyJSFile2', $scripts, true),
+			array_search('core/js/myFancyJSFile3', $scripts, true)
 		);
 		$this->assertLessThan(
-			array_search('myApp2/js/myApp2JSFile', \OCP\Util::getScripts(), true),
-			array_search('myApp/js/myFancyJSFile3', \OCP\Util::getScripts(), true)
+			array_search('myApp2/js/myApp2JSFile', $scripts, true),
+			array_search('myApp/js/myFancyJSFile3', $scripts, true)
 		);
 		$this->assertLessThan(
-			array_search('myApp3/js/myApp3JSFile', \OCP\Util::getScripts(), true),
-			array_search('myApp2/js/myApp2JSFile', \OCP\Util::getScripts(), true)
+			array_search('myApp3/js/myApp3JSFile', $scripts, true),
+			array_search('myApp2/js/myApp2JSFile', $scripts, true)
 		);
 		$this->assertLessThan(
-			array_search('myApp4/js/myApp4JSFile', \OCP\Util::getScripts(), true),
-			array_search('myApp3/js/myApp3JSFile', \OCP\Util::getScripts(), true)
+			array_search('myApp4/js/myApp4JSFile', $scripts, true),
+			array_search('myApp3/js/myApp3JSFile', $scripts, true)
 		);
 		$this->assertLessThan(
-			array_search('myApp5/js/myApp5JSFile', \OCP\Util::getScripts(), true),
-			array_search('myApp2/js/myApp2JSFile', \OCP\Util::getScripts(), true)
+			array_search('myApp5/js/myApp5JSFile', $scripts, true),
+			array_search('myApp2/js/myApp2JSFile', $scripts, true)
 		);
 
 		// No duplicates
 		$this->assertEquals(
-			\OCP\Util::getScripts(),
-			array_unique(\OCP\Util::getScripts())
+			$scripts,
+			array_unique($scripts)
 		);
 
 		// All scripts still there
@@ -300,10 +303,19 @@ class UtilTest extends \Test\TestCase {
 			'myApp4/js/myApp4JSFile',
 		];
 		foreach ($scripts as $script) {
-			$this->assertContains($script, \OCP\Util::getScripts());
+			$this->assertContains($script, $scripts);
 		}
 	}
 
+	public function testAddScriptCircularDependency() {
+		\OCP\Util::addScript('circular', 'file1', 'dependency');
+		\OCP\Util::addScript('dependency', 'file2', 'circular');
+
+		$scripts = \OCP\Util::getScripts();
+		$this->assertContains('circular/js/file1', $scripts);
+		$this->assertContains('dependency/js/file2', $scripts);
+	}
+
 	public function testAddVendorScript() {
 		\OC_Util::addVendorScript('core', 'myFancyJSFile1');
 		\OC_Util::addVendorScript('myApp', 'myFancyJSFile2');