Avoid fetching board details multiple times

Signed-off-by: Julius Härtl <jus@bitgrid.net>

lib/Collaboration/Resources/ResourceProvider.php

@@ -100,6 +100,9 @@ class ResourceProvider implements IProvider {
 		if ($board->getOwner() === $user->getUID()) {
 			return true;
 		}
+		if ($board->getAcl() === null) {
+			return false;
+		}
 		return $this->permissionService->userCan($board->getAcl(), Acl::PERMISSION_READ, $user->getUID());
 	}
 

lib/Collaboration/Resources/ResourceProviderCard.php

@@ -127,6 +127,9 @@ class ResourceProviderCard implements IProvider {
 		if ($board->getOwner() === $user->getUID()) {
 			return true;
 		}
+		if ($board->getAcl() === null) {
+			return false;
+		}
 		return $this->permissionService->userCan($board->getAcl(), Acl::PERMISSION_READ, $user->getUID());
 	}
 

lib/Db/Board.php

@@ -28,8 +28,10 @@ class Board extends RelationalEntity {
 	protected $owner;
 	protected $color;
 	protected $archived = false;
-	protected $labels = [];
-	protected $acl = [];
+	/** @var Label[]|null */
+	protected $labels = null;
+	/** @var Acl[]|null */
+	protected $acl = null;
 	protected $permissions = [];
 	protected $users = [];
 	protected $shared;
@@ -85,4 +87,14 @@ class Board extends RelationalEntity {
 	public function getETag() {
 		return md5((string)$this->getLastModified());
 	}
+
+	/** @returns Acl[]|null */
+	public function getAcl(): ?array {
+		return $this->acl;
+	}
+
+	/** @returns Label[]|null */
+	public function getLabels(): ?array {
+		return $this->labels;
+	}
 }

lib/Db/BoardMapper.php

@@ -42,7 +42,10 @@ class BoardMapper extends QBMapper implements IPermissionMapper {
 
 	private $circlesEnabled;
 
+	/** @var CappedMemoryCache */
 	private $userBoardCache;
+	/** @var CappedMemoryCache */
+	private $boardCache;
 
 	public function __construct(
 		IDBConnection $db,
@@ -62,7 +65,7 @@ class BoardMapper extends QBMapper implements IPermissionMapper {
 		$this->logger = $logger;
 
 		$this->userBoardCache = new CappedMemoryCache();
-
+		$this->boardCache = new CappedMemoryCache();
 
 		$this->circlesEnabled = \OC::$server->getAppManager()->isEnabledForUser('circles');
 	}
@@ -77,29 +80,31 @@ class BoardMapper extends QBMapper implements IPermissionMapper {
 	 * @throws DoesNotExistException
 	 */
 	public function find($id, $withLabels = false, $withAcl = false): Board {
-		$qb = $this->db->getQueryBuilder();
-		$qb->select('*')
-			->from('deck_boards')
-			->where($qb->expr()->eq('id', $qb->createNamedParameter($id, IQueryBuilder::PARAM_INT)))
-			->orderBy('id');
-		$board = $this->findEntity($qb);
+		if (!isset($this->boardCache[$id])) {
+			$qb = $this->db->getQueryBuilder();
+			$qb->select('*')
+				->from('deck_boards')
+				->where($qb->expr()->eq('id', $qb->createNamedParameter($id, IQueryBuilder::PARAM_INT)))
+				->orderBy('id');
+			$this->boardCache[$id] = $this->findEntity($qb);
+		}
 
 		// FIXME is this necessary? it was NOT done with the old mapper
 		// $this->mapOwner($board);
 
 		// Add labels
-		if ($withLabels) {
+		if ($withLabels && $this->boardCache[$id]->getLabels() === null) {
 			$labels = $this->labelMapper->findAll($id);
-			$board->setLabels($labels);
+			$this->boardCache[$id]->setLabels($labels);
 		}
 
 		// Add acl
-		if ($withAcl) {
+		if ($withAcl && $this->boardCache[$id]->getAcl() === null) {
 			$acl = $this->aclMapper->findAll($id);
-			$board->setAcl($acl);
+			$this->boardCache[$id]->setAcl($acl);
 		}
 
-		return $board;
+		return $this->boardCache[$id];
 	}
 
 	public function findAllForUser(string $userId, ?int $since = null, bool $includeArchived = true, ?int $before = null,
@@ -113,6 +118,9 @@ class BoardMapper extends QBMapper implements IPermissionMapper {
 			$groupBoards = $this->findAllByGroups($userId, $groups, null, null, $since, $includeArchived, $before, $term);
 			$circleBoards = $this->findAllByCircles($userId, null, null, $since, $includeArchived, $before, $term);
 			$allBoards = array_unique(array_merge($userBoards, $groupBoards, $circleBoards));
+			foreach ($allBoards as $board) {
+				$this->boardCache[$board->getId()] = $board;
+			}
 			if ($useCache) {
 				$this->userBoardCache[$userId] = $allBoards;
 			}

lib/Service/BoardService.php

@@ -179,9 +179,11 @@ class BoardService {
 		/** @var Board $board */
 		$board = $this->boardMapper->find($boardId, true, true);
 		$this->boardMapper->mapOwner($board);
-		foreach ($board->getAcl() as &$acl) {
-			if ($acl !== null) {
-				$this->boardMapper->mapAcl($acl);
+		if ($board->getAcl() !== null) {
+			foreach ($board->getAcl() as $acl) {
+				if ($acl !== null) {
+					$this->boardMapper->mapAcl($acl);
+				}
 			}
 		}
 		$permissions = $this->permissionService->matchPermissions($board);

lib/Service/PermissionService.php

@@ -117,7 +117,7 @@ class PermissionService {
 	 */
 	public function matchPermissions(Board $board) {
 		$owner = $this->userIsBoardOwner($board->getId());
-		$acls = $board->getAcl();
+		$acls = $board->getAcl() ?? [];
 		return [
 			Acl::PERMISSION_READ => $owner || $this->userCan($acls, Acl::PERMISSION_READ),
 			Acl::PERMISSION_EDIT => $owner || $this->userCan($acls, Acl::PERMISSION_EDIT),
@@ -155,7 +155,7 @@ class PermissionService {
 		}
 
 		try {
-			$acls = $this->getBoard($boardId)->getAcl();
+			$acls = $this->getBoard($boardId)->getAcl() ?? [];
 			$result = $this->userCan($acls, $permission, $userId);
 			if ($result) {
 				return true;

tests/unit/Db/BoardTest.php

@@ -23,12 +23,12 @@ class BoardTest extends TestCase {
 			'title' => "My Board",
 			'owner' => "admin",
 			'color' => "000000",
-			'labels' => [],
+			'labels' => null,
 			'permissions' => [],
 			'stacks' => [],
 			'deletedAt' => 0,
 			'lastModified' => 0,
-			'acl' => [],
+			'acl' => null,
 			'archived' => false,
 			'users' => ['user1', 'user2'],
 			'settings' => [],
@@ -49,7 +49,7 @@ class BoardTest extends TestCase {
 			'stacks' => [],
 			'deletedAt' => 0,
 			'lastModified' => 0,
-			'acl' => [],
+			'acl' => null,
 			'archived' => false,
 			'users' => [],
 			'settings' => [],
@@ -72,12 +72,12 @@ class BoardTest extends TestCase {
 			'title' => "My Board",
 			'owner' => "admin",
 			'color' => "000000",
-			'labels' => [],
+			'labels' => null,
 			'permissions' => [],
 			'stacks' => [],
 			'deletedAt' => 0,
 			'lastModified' => 0,
-			'acl' => [],
+			'acl' => null,
 			'archived' => false,
 			'shared' => 1,
 			'users' => [],