Use already cached parents when fetching ACL

When fetching ACL rules, don't refetch the rules from already cached
parents. This fixes performance issues with large folders.

Removed sibling prefetching becasue this information is already retrieved
by getRules() earlier as it's being called with all the parent paths.

This significantly improves performance.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>

2 files changed, 21 insertions, 26 deletions

diff --git a/lib/ACL/ACLManager.php b/lib/ACL/ACLManager.php
index 4ee8cac6..f2fcb2c3 100644
--- a/lib/ACL/ACLManager.php
+++ b/lib/ACL/ACLManager.php
@@ -52,40 +52,31 @@ class ACLManager {
 		return $this->rootStorageId;
 	}
 
-	private function pathsAreCached(array $paths): bool {
-		foreach ($paths as $path) {
-			if (!$this->ruleCache->hasKey($path)) {
-				return false;
-			}
-		}
-		return true;
-	}
-
 	/**
 	 * @param int $folderId
 	 * @param array $paths
 	 * @return (Rule[])[]
 	 */
 	private function getRules(array $paths): array {
-		if ($this->pathsAreCached($paths)) {
-			$rules = array_combine($paths, array_map(function (string $path) {
-				return $this->ruleCache->get($path);
-			}, $paths));
-		} else {
-			$rules = $this->ruleManager->getRulesForFilesByPath($this->user, $this->getRootStorageId(), $paths);
-			foreach ($rules as $path => $rulesForPath) {
+		// beware: adding new rules to the cache besides the cap
+		// might discard former cached entries, so we can't assume they'll stay
+		// cached, so we read everything out initially to be able to return it
+		$rules = array_combine($paths, array_map(function (string $path) {
+			return $this->ruleCache->get($path);
+		}, $paths));
+
+		$nonCachedPaths = array_filter($paths, function (string $path) use ($rules) {
+			return !isset($rules[$path]);
+		});
+
+		if (!empty($nonCachedPaths)) {
+			$newRules = $this->ruleManager->getRulesForFilesByPath($this->user, $this->getRootStorageId(), $nonCachedPaths);
+			foreach ($newRules as $path => $rulesForPath) {
 				$this->ruleCache->set($path, $rulesForPath);
-			}
-
-			if (count($paths) > 2) {
-				// also cache the direct sibling since it's likely that we'll be needing those later
-				$directParent = $paths[1];
-				$siblingRules = $this->ruleManager->getRulesForFilesByParent($this->user, $this->getRootStorageId(), $directParent);
-				foreach ($siblingRules as $path => $rulesForPath) {
-					$this->ruleCache->set($path, $rulesForPath);
-				}
+				$rules[$path] = $rulesForPath;
 			}
 		}
+
 		ksort($rules);
 
 		return $rules;
diff --git a/tests/ACL/ACLManagerTest.php b/tests/ACL/ACLManagerTest.php
index 8aa3ed38..a743b931 100644
--- a/tests/ACL/ACLManagerTest.php
+++ b/tests/ACL/ACLManagerTest.php
@@ -61,9 +61,13 @@ class ACLManagerTest extends TestCase {
 
 		$this->ruleManager->method('getRulesForFilesByPath')
 			->willReturnCallback(function (IUser $user, int $storageId, array $paths) {
-				return array_filter($this->rules, function (string $path) use ($paths) {
+				// fill with empty in case no rule was found
+				$rules = array_fill_keys($paths, []);
+				$actualRules = array_filter($this->rules, function (string $path) use ($paths) {
 					return array_search($path, $paths) !== false;
 				}, ARRAY_FILTER_USE_KEY);
+
+				return array_merge($rules, $actualRules);
 			});
 	}