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>

lib/composer/composer/autoload_classmap.php

@@ -661,6 +661,8 @@ return array(
     'OC\\AppFramework\\Utility\\ControllerMethodReflector' => $baseDir . '/lib/private/AppFramework/Utility/ControllerMethodReflector.php',
     'OC\\AppFramework\\Utility\\SimpleContainer' => $baseDir . '/lib/private/AppFramework/Utility/SimpleContainer.php',
     'OC\\AppFramework\\Utility\\TimeFactory' => $baseDir . '/lib/private/AppFramework/Utility/TimeFactory.php',
+    'OC\\AppScriptDependency' => $baseDir . '/lib/private/AppScriptDependency.php',
+    'OC\\AppScriptSort' => $baseDir . '/lib/private/AppScriptSort.php',
     'OC\\App\\AppManager' => $baseDir . '/lib/private/App/AppManager.php',
     'OC\\App\\AppStore\\Bundles\\Bundle' => $baseDir . '/lib/private/App/AppStore/Bundles/Bundle.php',
     'OC\\App\\AppStore\\Bundles\\BundleFetcher' => $baseDir . '/lib/private/App/AppStore/Bundles/BundleFetcher.php',

lib/composer/composer/autoload_static.php

@@ -690,6 +690,8 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c
         'OC\\AppFramework\\Utility\\ControllerMethodReflector' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Utility/ControllerMethodReflector.php',
         'OC\\AppFramework\\Utility\\SimpleContainer' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Utility/SimpleContainer.php',
         'OC\\AppFramework\\Utility\\TimeFactory' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Utility/TimeFactory.php',
+        'OC\\AppScriptDependency' => __DIR__ . '/../../..' . '/lib/private/AppScriptDependency.php',
+        'OC\\AppScriptSort' => __DIR__ . '/../../..' . '/lib/private/AppScriptSort.php',
         'OC\\App\\AppManager' => __DIR__ . '/../../..' . '/lib/private/App/AppManager.php',
         'OC\\App\\AppStore\\Bundles\\Bundle' => __DIR__ . '/../../..' . '/lib/private/App/AppStore/Bundles/Bundle.php',
         'OC\\App\\AppStore\\Bundles\\BundleFetcher' => __DIR__ . '/../../..' . '/lib/private/App/AppStore/Bundles/BundleFetcher.php',

lib/private/AppScriptDependency.php

@@ -0,0 +1,97 @@
+<?php
+/**
+ * @copyright Copyright (c) 2021, Jonas Meurer <jonas@freesources.org>
+ *
+ * @author Jonas Meurer <jonas@freesources.org>
+ *
+ * @license GNU AGPL version 3 or any later version
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+namespace OC;
+
+class AppScriptDependency {
+	/** @var string */
+	private $id;
+
+	/** @var array */
+	private $deps;
+
+	/** @var bool */
+	private $visited;
+
+	/**
+	 * @param string $id
+	 * @param array $deps
+	 * @param bool $visited
+	 */
+	public function __construct(string $id, array $deps = [], bool $visited = false) {
+		$this->setId($id);
+		$this->setDeps($deps);
+		$this->setVisited($visited);
+	}
+
+	/**
+	 * @return string
+	 */
+	public function getId(): string {
+		return $this->id;
+	}
+
+	/**
+	 * @param string $id
+	 */
+	public function setId(string $id): void {
+		$this->id = $id;
+	}
+
+	/**
+	 * @return array
+	 */
+	public function getDeps(): array {
+		return $this->deps;
+	}
+
+	/**
+	 * @param array $deps
+	 */
+	public function setDeps(array $deps): void {
+		$this->deps = $deps;
+	}
+
+	/**
+	 * @param string $dep
+	 */
+	public function addDep(string $dep): void {
+		if (!in_array($dep, $this->deps, true)) {
+			$this->deps[] = $dep;
+		}
+	}
+
+	/**
+	 * @return bool
+	 */
+	public function isVisited(): bool {
+		return $this->visited;
+	}
+
+	/**
+	 * @param bool $visited
+	 */
+	public function setVisited(bool $visited): void {
+		$this->visited = $visited;
+	}
+}

lib/private/AppScriptSort.php

@@ -0,0 +1,105 @@
+<?php
+/**
+ * @copyright Copyright (c) 2021, Jonas Meurer <jonas@freesources.org>
+ *
+ * @author Jonas Meurer <jonas@freesources.org>
+ *
+ * @license GNU AGPL version 3 or any later version
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+namespace OC;
+
+use Psr\Log\LoggerInterface;
+
+/**
+ * Sort scripts topologically by their dependencies
+ * Implementation based on https://github.com/marcj/topsort.php
+ */
+class AppScriptSort {
+	/** @var LoggerInterface */
+	private $logger;
+
+	public function __construct(LoggerInterface $logger) {
+		$this->logger = $logger;
+	}
+
+	/**
+	 * Recursive topological sorting
+	 *
+	 * @param AppScriptDependency $app
+	 * @param array $parents
+	 * @param array $scriptDeps
+	 * @param array $sortedScriptDeps
+	 */
+	private function topSortVisit(
+			AppScriptDependency $app,
+			array &$parents,
+			array &$scriptDeps,
+			array &$sortedScriptDeps): void {
+		// Detect and log circular dependencies
+		if (isset($parents[$app->getId()])) {
+			$this->logger->error('Circular dependency in app scripts at app ' . $app->getId());
+		}
+
+		// If app has not been visited
+		if (!$app->isVisited()) {
+			$parents[$app->getId()] = true;
+			$app->setVisited(true);
+
+			foreach ($app->getDeps() as $dep) {
+				if ($app->getId() === $dep) {
+					// Ignore dependency on itself
+					continue;
+				}
+
+				if (isset($scriptDeps[$dep])) {
+					$newParents = $parents;
+					$this->topSortVisit($scriptDeps[$dep], $newParents, $scriptDeps, $sortedScriptDeps);
+				}
+			}
+
+			$sortedScriptDeps[] = $app->getId();
+		}
+	}
+
+	/**
+	 * @return array scripts sorted by dependencies
+	 */
+	public function sort(array $scripts, array $scriptDeps): array {
+		// Sort scriptDeps into sortedScriptDeps
+		$sortedScriptDeps = [];
+		foreach ($scriptDeps as $app) {
+			$parents = [];
+			$this->topSortVisit($app, $parents, $scriptDeps, $sortedScriptDeps);
+		}
+
+		// Sort scripts into sortedScripts based on sortedScriptDeps order
+		$sortedScripts = [];
+		foreach ($sortedScriptDeps as $app) {
+			$sortedScripts[$app] = $scripts[$app] ?? [];
+		}
+
+		// Add remaining scripts
+		foreach (array_keys($scripts) as $app) {
+			if (!isset($sortedScripts[$app])) {
+				$sortedScripts[$app] = $scripts[$app];
+			}
+		}
+
+		return $sortedScripts;
+	}
+}

lib/public/Util.php

@@ -12,6 +12,7 @@
  * @author J0WI <J0WI@users.noreply.github.com>
  * @author Jens-Christian Fischer <jens-christian.fischer@switch.ch>
  * @author Joas Schilling <coding@schilljs.com>
+ * @author Jonas Meurer <jonas@freesources.org>
  * @author Julius Härtl <jus@bitgrid.net>
  * @author Lukas Reschke <lukas@statuscode.ch>
  * @author Michael Gapczynski <GapczynskiM@gmail.com>
@@ -45,6 +46,9 @@
 
 namespace OCP;
 
+use OC\AppScriptDependency;
+use OC\AppScriptSort;
+
 /**
  * This class provides different helper functions to make the life of a developer easier
  *
@@ -81,6 +85,9 @@ class Util {
 	/** @var array */
 	private static $scriptDeps = [];
 
+	/** @var array */
+	private static $sortedScriptDeps = [];
+
 	/**
 	 * get the current installed version of Nextcloud
 	 * @return array
@@ -177,12 +184,13 @@ class Util {
 
 	/**
 	 * add a javascript file
+	 *
 	 * @param string $application
-	 * @param string $file
+	 * @param string|null $file
 	 * @param string $afterAppId
 	 * @since 4.0.0
 	 */
-	public static function addScript($application, $file = null, $afterAppId = null) {
+	public static function addScript(string $application, string $file = null, string $afterAppId = 'core'): void {
 		if (!empty($application)) {
 			$path = "$application/js/$file";
 		} else {
@@ -198,9 +206,11 @@ class Util {
 			self::addTranslations($application);
 		}
 
-		// store dependency if defined
-		if (!empty($afterAppId)) {
-			self::$scriptDeps[$application] = $afterAppId;
+		// store app in dependency list
+		if (!array_key_exists($application, self::$scriptDeps)) {
+			self::$scriptDeps[$application] = new AppScriptDependency($application, [$afterAppId]);
+		} else {
+			self::$scriptDeps[$application]->addDep($afterAppId);
 		}
 
 		self::$scripts[$application][] = $path;
@@ -208,36 +218,17 @@ class Util {
 
 	/**
 	 * Return the list of scripts injected to the page
+	 *
 	 * @return array
 	 * @since 24.0.0
 	 */
 	public static function getScripts(): array {
-		// Sort by dependency if any
-		$sortByDeps = static function (string $app1, string $app2): int {
-			// Always sort core first
-			if ($app1 === 'core') {
-				return -1;
-			}
-			if ($app2 === 'core') {
-				return 1;
-			}
-
-			// If app1 has a dependency
-			if (array_key_exists($app1, self::$scriptDeps)) {
-				$apps = array_keys(self::$scripts);
-				// Move app1 backwards if dep comes afterwards
-				if (array_search($app1, $apps, true) <
-					array_search(self::$scriptDeps[$app1], $apps, true)) {
-					return 1;
-				}
-			}
-
-			return 0;
-		};
-		uksort(self::$scripts, $sortByDeps);
+		// Sort scriptDeps into sortedScriptDeps
+		$scriptSort = \OC::$server->get(AppScriptSort::class);
+		$sortedScripts = $scriptSort->sort(self::$scripts, self::$scriptDeps);
 
 		// Flatten array and remove duplicates
-		return self::$scripts ? array_unique(array_merge(...array_values(self::$scripts))) : [];
+		return $sortedScripts ? array_unique(array_merge(...array_values(($sortedScripts)))) : [];
 	}
 
 	/**

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);
+		}
+	}
+}

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');