Merge pull request #3449 from nextcloud/backport/3444/stable23

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 = [];
+	/** @var Label[]|null */
-	protected $acl = [];
+	protected $labels = null;
+	/** @var Acl[]|null */
+	protected $acl = null;
 	protected $permissions = [];
 	protected $users = [];
 	protected $shared;
@@ -61,6 +63,10 @@ class Board extends RelationalEntity {
 		if ($this->shared === -1) {
 			unset($json['shared']);
 		}
+		// FIXME: Ideally the API responses should follow the internal data structure and return null if the labels/acls have not been fetched from the db
+		// however this would be a breaking change for consumers of the API
+		$json['acl'] = $this->acl ?? [];
+		$json['labels'] = $this->labels ?? [];
 		return $json;
 	}
 
@@ -85,4 +91,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();
+		if (!isset($this->boardCache[$id])) {
-		$qb->select('*')
+			$qb = $this->db->getQueryBuilder();
-			->from('deck_boards')
+			$qb->select('*')
-			->where($qb->expr()->eq('id', $qb->createNamedParameter($id, IQueryBuilder::PARAM_INT)))
+				->from('deck_boards')
-			->orderBy('id');
+				->where($qb->expr()->eq('id', $qb->createNamedParameter($id, IQueryBuilder::PARAM_INT)))
-		$board = $this->findEntity($qb);
+				->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/Db/CardMapper.php

@@ -30,6 +30,8 @@ use OCA\Deck\Search\Query\SearchQuery;
 use OCP\AppFramework\Db\Entity;
 use OCP\AppFramework\Db\QBMapper;
 use OCP\DB\QueryBuilder\IQueryBuilder;
+use OCP\ICache;
+use OCP\ICacheFactory;
 use OCP\IDBConnection;
 use OCP\IGroupManager;
 use OCP\IUser;
@@ -46,6 +48,8 @@ class CardMapper extends QBMapper implements IPermissionMapper {
 	private $groupManager;
 	/** @var IManager */
 	private $notificationManager;
+	/** @var ICache */
+	private $cache;
 	private $databaseType;
 	private $database4ByteSupport;
 
@@ -55,6 +59,7 @@ class CardMapper extends QBMapper implements IPermissionMapper {
 		IUserManager $userManager,
 		IGroupManager $groupManager,
 		IManager $notificationManager,
+		ICacheFactory $cacheFactory,
 		$databaseType = 'sqlite3',
 		$database4ByteSupport = true
 	) {
@@ -63,6 +68,7 @@ class CardMapper extends QBMapper implements IPermissionMapper {
 		$this->userManager = $userManager;
 		$this->groupManager = $groupManager;
 		$this->notificationManager = $notificationManager;
+		$this->cache = $cacheFactory->createDistributed('deck-cardMapper');
 		$this->databaseType = $databaseType;
 		$this->database4ByteSupport = $database4ByteSupport;
 	}
@@ -75,7 +81,9 @@ class CardMapper extends QBMapper implements IPermissionMapper {
 			$description = preg_replace('/[\x{10000}-\x{10FFFF}]/u', "\xEF\xBF\xBD", $entity->getDescription());
 			$entity->setDescription($description);
 		}
-		return parent::insert($entity);
+		$entity = parent::insert($entity);
+		$this->cache->remove('findBoardId:' . $entity->getId());
+		return $entity;
 	}
 
 	public function update(Entity $entity, $updateModified = true): Entity {
@@ -107,6 +115,10 @@ class CardMapper extends QBMapper implements IPermissionMapper {
 			} catch (Exception $e) {
 			}
 		}
+		// Invalidate cache when the card may be moved to a different board
+		if (isset($updatedFields['stackId'])) {
+			$this->cache->remove('findBoardId:' . $entity->getId());
+		}
 		return parent::update($entity);
 	}
 
@@ -506,9 +518,8 @@ class CardMapper extends QBMapper implements IPermissionMapper {
 	}
 
 	public function delete(Entity $entity): Entity {
-		// delete assigned labels
 		$this->labelMapper->deleteLabelAssignmentsForCard($entity->getId());
-		// delete card
+		$this->cache->remove('findBoardId:' . $entity->getId());
 		return parent::delete($entity);
 	}
 
@@ -547,11 +558,22 @@ class CardMapper extends QBMapper implements IPermissionMapper {
 	}
 
 	public function findBoardId($id): ?int {
-		$sql = 'SELECT id FROM `*PREFIX*deck_boards` WHERE `id` IN (SELECT board_id FROM `*PREFIX*deck_stacks` WHERE id IN (SELECT stack_id FROM `*PREFIX*deck_cards` WHERE id = ?))';
+		$result = $this->cache->get('findBoardId:' . $id);
-		$stmt = $this->db->prepare($sql);
+		if ($result === null) {
-		$stmt->bindParam(1, $id, \PDO::PARAM_INT);
+			try {
-		$stmt->execute();
+				$qb = $this->db->getQueryBuilder();
-		return $stmt->fetchColumn() ?? null;
+				$qb->select('board_id')
+					->from('deck_stacks', 's')
+					->innerJoin('s', 'deck_cards', 'c', 'c.stack_id = s.id')
+					->where($qb->expr()->eq('c.id', $qb->createNamedParameter($id)));
+				$queryResult = $qb->executeQuery();
+				$result = $queryResult->fetchOne();
+			} catch (\Exception $e) {
+				$result = false;
+			}
+			$this->cache->set('findBoardId:' . $id, $result);
+		}
+		return $result !== false ? $result : null;
 	}
 
 	public function mapOwner(Card &$card) {

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 ($board->getAcl() !== null) {
-			if ($acl !== null) {
+			foreach ($board->getAcl() as $acl) {
-				$this->boardMapper->mapAcl($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

@@ -36,6 +36,29 @@ class BoardTest extends TestCase {
 		], $board->jsonSerialize());
 	}
 
+	public function testUnfetchedValues() {
+		$board = $this->createBoard();
+		$board->setUsers(['user1', 'user2']);
+		self::assertNull($board->getAcl());
+		self::assertNull($board->getLabels());
+		$this->assertEquals([
+			'id' => 1,
+			'title' => "My Board",
+			'owner' => "admin",
+			'color' => "000000",
+			'labels' => [],
+			'permissions' => [],
+			'stacks' => [],
+			'deletedAt' => 0,
+			'lastModified' => 0,
+			'acl' => [],
+			'archived' => false,
+			'users' => ['user1', 'user2'],
+			'settings' => [],
+			'ETag' => $board->getETag(),
+		], $board->jsonSerialize());
+	}
+
 	public function testSetLabels() {
 		$board = $this->createBoard();
 		$board->setLabels(["foo", "bar"]);