Skip to content

Commit 9e22367

Browse files
mejo-mejo-
committed
Sort app scripts topologically by its dependencies
Implement a proper topological sorting algorithm. Based on the implementation by https://github.com/marcj/topsort.php Throws a CircularDependencyException if a circular dependency is detected. Fixes: #30278 Signed-off-by: Jonas Meurer <jonas@freesources.org>
1 parent 29dffd7 commit 9e22367

3 files changed

Lines changed: 131 additions & 41 deletions

File tree

‎lib/public/CircularDependencyException.php‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
<?php
2+
/**
3+
* @copyright Copyright (c) 2021, Jonas Meurer <jonas@freesources.org>
4+
*
5+
* @author Jonas Meurer <jonas@freesources.org>
6+
*
7+
* @license AGPL-3.0
8+
*
9+
* This code is free software: you can redistribute it and/or modify
10+
* it under the terms of the GNU Affero General Public License, version 3,
11+
* as published by the Free Software Foundation.
12+
*
13+
* This program is distributed in the hope that it will be useful,
14+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
15+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
16+
* GNU Affero General Public License for more details.
17+
*
18+
* You should have received a copy of the GNU Affero General Public License, version 3,
19+
* along with this program. If not, see <http://www.gnu.org/licenses/>
20+
*
21+
*/
22+
namespace OCP;
23+
24+
/**
25+
* Class CircularDependencyException
26+
*
27+
* Exception for circular dependencies (e.g. in the app scripts)
28+
*
29+
* @package OCP
30+
* @since 24.0.0
31+
*/
32+
class CircularDependencyException extends \Exception {
33+
}

‎lib/public/Util.php‎

Lines changed: 71 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,9 @@ class Util {
8181
/** @var array */
8282
private static $scriptDeps = [];
8383

84+
/** @var array */
85+
private static $sortedScriptDeps = [];
86+
8487
/**
8588
* get the current installed version of Nextcloud
8689
* @return array
@@ -177,12 +180,13 @@ public static function addStyle($application, $file = null) {
177180

178181
/**
179182
* add a javascript file
183+
*
180184
* @param string $application
181-
* @param string $file
185+
* @param string|null $file
182186
* @param string $afterAppId
183187
* @since 4.0.0
184188
*/
185-
public static function addScript($application, $file = null, $afterAppId = null) {
189+
public static function addScript(string $application, string $file = null, string $afterAppId = 'core'): void {
186190
if (!empty($application)) {
187191
$path = "$application/js/$file";
188192
} else {
@@ -198,46 +202,87 @@ public static function addScript($application, $file = null, $afterAppId = null)
198202
self::addTranslations($application);
199203
}
200204

201-
// store dependency if defined
202-
if (!empty($afterAppId)) {
203-
self::$scriptDeps[$application] = $afterAppId;
205+
// store app in dependency list
206+
if (!array_key_exists($application, self::$scriptDeps)) {
207+
self::$scriptDeps[$application] = (object)[
208+
'id' => $application,
209+
'deps' => [$afterAppId],
210+
'visited' => false,
211+
];
212+
} elseif (!in_array($afterAppId, self::$scriptDeps[$application]->deps, true)) {
213+
self::$scriptDeps[$application]->deps[] = $afterAppId;
204214
}
205215

206216
self::$scripts[$application][] = $path;
207217
}
208218

219+
/**
220+
* Recursive topological sorting
221+
*
222+
* @param object $app
223+
* @param array|null $parents
224+
* @throws CircularDependencyException
225+
*/
226+
private static function topSortVisit(object $app, array &$parents = null): void {
227+
// Detect circular dependencies
228+
if (isset($parents[$app->id])) {
229+
throw new CircularDependencyException('Circular app script dependency at app ' . $app->id);
230+
}
231+
232+
// If app has not been visited
233+
if (!$app->visited) {
234+
$parents[$app->id] = true;
235+
$app->visited = true;
236+
237+
foreach ($app->deps as $dep) {
238+
if ($app->id === $dep) {
239+
// Ignore dependency on itself
240+
continue;
241+
}
242+
243+
if (isset(self::$scriptDeps[$dep])) {
244+
$newParents = $parents;
245+
self::topSortVisit(self::$scriptDeps[$dep], $newParents);
246+
}
247+
}
248+
249+
self::$sortedScriptDeps[] = $app->id;
250+
}
251+
}
252+
209253
/**
210254
* Return the list of scripts injected to the page
255+
*
211256
* @return array
257+
* @throws CircularDependencyException
212258
* @since 24.0.0
213259
*/
214260
public static function getScripts(): array {
215-
// Sort by dependency if any
216-
$sortByDeps = static function (string $app1, string $app2): int {
217-
// Always sort core first
218-
if ($app1 === 'core') {
219-
return -1;
220-
}
221-
if ($app2 === 'core') {
222-
return 1;
261+
// Sort scripts topologically by their dependencies
262+
// Implementation based on https://github.com/marcj/topsort.php
263+
264+
// Reset if scriptDeps had been sorted into sortedScriptDeps before
265+
if (!empty(self::$sortedScriptDeps)) {
266+
self::$sortedScriptDeps = [];
267+
foreach (self::$scriptDeps as $app) {
268+
$app->visited = false;
223269
}
270+
}
224271

225-
// If app1 has a dependency
226-
if (array_key_exists($app1, self::$scriptDeps)) {
227-
$apps = array_keys(self::$scripts);
228-
// Move app1 backwards if dep comes afterwards
229-
if (array_search($app1, $apps, true) <
230-
array_search(self::$scriptDeps[$app1], $apps, true)) {
231-
return 1;
232-
}
233-
}
272+
// Sort scriptDeps into sortedScriptDeps
273+
foreach (self::$scriptDeps as $app) {
274+
$parents = [];
275+
self::topSortVisit($app, $parents);
276+
}
234277

235-
return 0;
236-
};
237-
uksort(self::$scripts, $sortByDeps);
278+
// Sort scripts into sortedScripts based on sortedScriptDeps order
279+
$sortedScripts = [];
280+
foreach (self::$sortedScriptDeps as $app) {
281+
$sortedScripts[$app] = self::$scripts[$app] ?? [];
282+
}
238283

239284
// Flatten array and remove duplicates
240-
return self::$scripts ? array_unique(array_merge(...array_values(self::$scripts))) : [];
285+
return $sortedScripts ? array_unique(array_merge(...array_values(($sortedScripts)))) : [];
241286
}
242287

243288
/**

‎tests/lib/UtilTest.php‎

Lines changed: 27 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
namespace Test;
1010

1111
use OC_Util;
12+
use OCP\CircularDependencyException;
1213

1314
/**
1415
* Class UtilTest
@@ -236,6 +237,7 @@ protected function tearDown(): void {
236237
}
237238

238239
public function testAddScript() {
240+
\OCP\Util::addScript('first', 'myFirstJSFile');
239241
\OCP\Util::addScript('core', 'myFancyJSFile1');
240242
\OCP\Util::addScript('files', 'myFancyJSFile2', 'core');
241243
\OCP\Util::addScript('myApp5', 'myApp5JSFile', 'myApp2');
@@ -250,42 +252,44 @@ public function testAddScript() {
250252
\OCP\Util::addScript('myApp3', 'myApp3JSFile', 'myApp2');
251253
\OCP\Util::addScript('myApp2', 'myApp2JSFile', 'myApp');
252254

255+
$scripts = \OCP\Util::getScripts();
256+
253257
// Core should appear first
254258
$this->assertEquals(
255259
0,
256-
array_search('core/js/myFancyJSFile1', \OCP\Util::getScripts(), true)
260+
array_search('core/js/myFancyJSFile1', $scripts, true)
257261
);
258262
$this->assertEquals(
259263
1,
260-
array_search('core/js/myFancyJSFile4', \OCP\Util::getScripts(), true)
264+
array_search('core/js/myFancyJSFile4', $scripts, true)
261265
);
262266

263267
// Dependencies should appear before their children
264268
$this->assertLessThan(
265-
array_search('files/js/myFancyJSFile2', \OCP\Util::getScripts(), true),
266-
array_search('core/js/myFancyJSFile3', \OCP\Util::getScripts(), true)
269+
array_search('files/js/myFancyJSFile2', $scripts, true),
270+
array_search('core/js/myFancyJSFile3', $scripts, true)
267271
);
268272
$this->assertLessThan(
269-
array_search('myApp2/js/myApp2JSFile', \OCP\Util::getScripts(), true),
270-
array_search('myApp/js/myFancyJSFile3', \OCP\Util::getScripts(), true)
273+
array_search('myApp2/js/myApp2JSFile', $scripts, true),
274+
array_search('myApp/js/myFancyJSFile3', $scripts, true)
271275
);
272276
$this->assertLessThan(
273-
array_search('myApp3/js/myApp3JSFile', \OCP\Util::getScripts(), true),
274-
array_search('myApp2/js/myApp2JSFile', \OCP\Util::getScripts(), true)
277+
array_search('myApp3/js/myApp3JSFile', $scripts, true),
278+
array_search('myApp2/js/myApp2JSFile', $scripts, true)
275279
);
276280
$this->assertLessThan(
277-
array_search('myApp4/js/myApp4JSFile', \OCP\Util::getScripts(), true),
278-
array_search('myApp3/js/myApp3JSFile', \OCP\Util::getScripts(), true)
281+
array_search('myApp4/js/myApp4JSFile', $scripts, true),
282+
array_search('myApp3/js/myApp3JSFile', $scripts, true)
279283
);
280284
$this->assertLessThan(
281-
array_search('myApp5/js/myApp5JSFile', \OCP\Util::getScripts(), true),
282-
array_search('myApp2/js/myApp2JSFile', \OCP\Util::getScripts(), true)
285+
array_search('myApp5/js/myApp5JSFile', $scripts, true),
286+
array_search('myApp2/js/myApp2JSFile', $scripts, true)
283287
);
284288

285289
// No duplicates
286290
$this->assertEquals(
287-
\OCP\Util::getScripts(),
288-
array_unique(\OCP\Util::getScripts())
291+
$scripts,
292+
array_unique($scripts)
289293
);
290294

291295
// All scripts still there
@@ -300,10 +304,18 @@ public function testAddScript() {
300304
'myApp4/js/myApp4JSFile',
301305
];
302306
foreach ($scripts as $script) {
303-
$this->assertContains($script, \OCP\Util::getScripts());
307+
$this->assertContains($script, $scripts);
304308
}
305309
}
306310

311+
public function testAddScriptCircularDependency() {
312+
\OCP\Util::addScript('circular', 'file1', 'dependency');
313+
\OCP\Util::addScript('dependency', 'file2', 'circular');
314+
315+
$this->expectException(CircularDependencyException::class);
316+
\OCP\Util::getScripts();
317+
}
318+
307319
public function testAddVendorScript() {
308320
\OC_Util::addVendorScript('core', 'myFancyJSFile1');
309321
\OC_Util::addVendorScript('myApp', 'myFancyJSFile2');

0 commit comments

Comments
 (0)