Merge pull request #3168 from nextcloud/backport/3161/stable22

[stable22] Reduce duplicate queries when fetching user boards an permissions

lib/Db/BoardMapper.php

@@ -23,6 +23,7 @@
 
 namespace OCA\Deck\Db;
 
+use OC\Cache\CappedMemoryCache;
 use OCP\AppFramework\Db\DoesNotExistException;
 use OCP\IDBConnection;
 use OCP\IUserManager;
@@ -39,6 +40,8 @@ class BoardMapper extends DeckMapper implements IPermissionMapper {
 
 	private $circlesEnabled;
 
+	private $userBoardCache;
+
 	public function __construct(
 		IDBConnection $db,
 		LabelMapper $labelMapper,
@@ -56,6 +59,9 @@ class BoardMapper extends DeckMapper implements IPermissionMapper {
 		$this->groupManager = $groupManager;
 		$this->logger = $logger;
 
+		$this->userBoardCache = new CappedMemoryCache();
+
+
 		$this->circlesEnabled = \OC::$server->getAppManager()->isEnabledForUser('circles');
 	}
 
@@ -89,13 +95,21 @@ class BoardMapper extends DeckMapper implements IPermissionMapper {
 	}
 
 	public function findAllForUser(string $userId, int $since = -1, $includeArchived = true): array {
-		$groups = $this->groupManager->getUserGroupIds(
+		$useCache = ($since === -1 && $includeArchived === true);
-			$this->userManager->get($userId)
+		if (!isset($this->userBoardCache[$userId]) || !$useCache) {
-		);
+			$groups = $this->groupManager->getUserGroupIds(
-		$userBoards = $this->findAllByUser($userId, null, null, $since, $includeArchived);
+				$this->userManager->get($userId)
-		$groupBoards = $this->findAllByGroups($userId, $groups,null, null, $since, $includeArchived);
+			);
-		$circleBoards = $this->findAllByCircles($userId, null, null,  $since, $includeArchived);
+			$userBoards = $this->findAllByUser($userId, null, null, $since, $includeArchived);
-		return array_unique(array_merge($userBoards, $groupBoards, $circleBoards));
+			$groupBoards = $this->findAllByGroups($userId, $groups, null, null, $since, $includeArchived);
+			$circleBoards = $this->findAllByCircles($userId, null, null, $since, $includeArchived);
+			$allBoards = array_unique(array_merge($userBoards, $groupBoards, $circleBoards));
+			if ($useCache) {
+				$this->userBoardCache[$userId] = $allBoards;
+			}
+			return $allBoards;
+		}
+		return $this->userBoardCache[$userId];
 	}
 
 	/**

lib/Service/PermissionService.php

@@ -23,6 +23,7 @@
 
 namespace OCA\Deck\Service;
 
+use OC\Cache\CappedMemoryCache;
 use OCA\Deck\Db\Acl;
 use OCA\Deck\Db\AclMapper;
 use OCA\Deck\Db\Board;
@@ -61,6 +62,7 @@ class PermissionService {
 	private $users = [];
 
 	private $circlesEnabled = false;
+	private $boardCache;
 
 	public function __construct(
 		ILogger $logger,
@@ -81,6 +83,8 @@ class PermissionService {
 		$this->config = $config;
 		$this->userId = $userId;
 
+		$this->boardCache = new CappedMemoryCache();
+
 		$this->circlesEnabled = \OC::$server->getAppManager()->isEnabledForUser('circles') &&
 			(version_compare(\OC::$server->getAppManager()->getAppVersion('circles'), '0.17.1') >= 0);
 	}
@@ -149,10 +153,13 @@ class PermissionService {
 			return true;
 		}
 
-		$acls = $this->aclMapper->findAll($boardId);
+		try {
-		$result = $this->userCan($acls, $permission, $userId);
+			$acls = $this->getBoard($boardId)->getAcl();
-		if ($result) {
+			$result = $this->userCan($acls, $permission, $userId);
-			return true;
+			if ($result) {
+				return true;
+			}
+		} catch (DoesNotExistException | MultipleObjectsReturnedException $e) {
 		}
 
 		// Throw NoPermission to not leak information about existing entries
@@ -168,13 +175,24 @@ class PermissionService {
 			$userId = $this->userId;
 		}
 		try {
-			$board = $this->boardMapper->find($boardId);
+			$board = $this->getBoard($boardId);
-			return $board && $userId === $board->getOwner();
+			return $userId === $board->getOwner();
 		} catch (DoesNotExistException | MultipleObjectsReturnedException $e) {
 		}
 		return false;
 	}
 
+	/**
+	 * @throws MultipleObjectsReturnedException
+	 * @throws DoesNotExistException
+	 */
+	private function getBoard($boardId): Board {
+		if (!isset($this->boardCache[$boardId])) {
+			$this->boardCache[$boardId] = $this->boardMapper->find($boardId, false, true);
+		}
+		return $this->boardCache[$boardId];
+	}
+
 	/**
 	 * Check if permission matches the acl rules for current user and groups
 	 *

tests/unit/Service/PermissionServiceTest.php

@@ -146,7 +146,7 @@ class PermissionServiceTest extends \Test\TestCase {
 	}
 
 	public function testUserIsBoardOwnerNull() {
-		$this->boardMapper->expects($this->once())->method('find')->willReturn(null);
+		$this->boardMapper->expects($this->once())->method('find')->willThrowException(new DoesNotExistException('board does not exist'));
 		$this->assertEquals(false, $this->service->userIsBoardOwner(123));
 	}
 
@@ -225,12 +225,9 @@ class PermissionServiceTest extends \Test\TestCase {
 		$board = new Board();
 		$board->setId($boardId);
 		$board->setOwner($owner);
+		$board->setAcl($this->getAcls($boardId));
 		$this->boardMapper->expects($this->any())->method('find')->willReturn($board);
 
-		// acl check
-		$acls = $this->getAcls($boardId);
-		$this->aclMapper->expects($this->any())->method('findAll')->willReturn($acls);
-
 		$this->shareManager->expects($this->any())
 			->method('sharingDisabledForUser')
 			->willReturn(false);
@@ -250,14 +247,12 @@ class PermissionServiceTest extends \Test\TestCase {
 		$board = new Board();
 		$board->setId($boardId);
 		$board->setOwner($owner);
+		$board->setAcl($this->getAcls($boardId));
 		if ($boardId === null) {
 			$this->boardMapper->expects($this->any())->method('find')->willThrowException(new DoesNotExistException('not found'));
 		} else {
 			$this->boardMapper->expects($this->any())->method('find')->willReturn($board);
 		}
-		$acls = $this->getAcls($boardId);
-		$this->aclMapper->expects($this->any())->method('findAll')->willReturn($acls);
-
 
 		if ($result) {
 			$actual = $this->service->checkPermission($mapper, 1234, $permission);