From 80388d1a884ad214d5db357be06972dd9788d5af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Tue, 23 Nov 2021 23:34:08 +0100 Subject: [PATCH 1/3] Cache card to board id relation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- lib/Db/CardMapper.php | 42 ++++++++++++++++++++++++++++++++---------- 1 file changed, 32 insertions(+), 10 deletions(-) diff --git a/lib/Db/CardMapper.php b/lib/Db/CardMapper.php index e416d1fb9..529808f55 100644 --- a/lib/Db/CardMapper.php +++ b/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); } @@ -479,8 +491,8 @@ class CardMapper extends QBMapper implements IPermissionMapper { } return $qb->createNamedParameter($dateTime, IQueryBuilder::PARAM_DATE); } - - + + public function searchRaw($boardIds, $term, $limit = null, $offset = null) { $qb = $this->queryCardsByBoards($boardIds) @@ -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 = ?))'; - $stmt = $this->db->prepare($sql); - $stmt->bindParam(1, $id, \PDO::PARAM_INT); - $stmt->execute(); - return $stmt->fetchColumn() ?? null; + $result = $this->cache->get('findBoardId:' . $id); + if ($result === null) { + try { + $qb = $this->db->getQueryBuilder(); + $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) { From 901b8f25060157835157b882859f117200165e8b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Tue, 23 Nov 2021 23:34:54 +0100 Subject: [PATCH 2/3] Avoid fetching board details multiple times MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- .../Resources/ResourceProvider.php | 3 ++ .../Resources/ResourceProviderCard.php | 3 ++ lib/Db/Board.php | 16 ++++++++-- lib/Db/BoardMapper.php | 32 ++++++++++++------- lib/Service/BoardService.php | 8 +++-- lib/Service/PermissionService.php | 4 +-- tests/unit/Db/BoardTest.php | 10 +++--- 7 files changed, 52 insertions(+), 24 deletions(-) diff --git a/lib/Collaboration/Resources/ResourceProvider.php b/lib/Collaboration/Resources/ResourceProvider.php index deffb475d..b4677148d 100644 --- a/lib/Collaboration/Resources/ResourceProvider.php +++ b/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()); } diff --git a/lib/Collaboration/Resources/ResourceProviderCard.php b/lib/Collaboration/Resources/ResourceProviderCard.php index 4d21fd16b..e436ba684 100644 --- a/lib/Collaboration/Resources/ResourceProviderCard.php +++ b/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()); } diff --git a/lib/Db/Board.php b/lib/Db/Board.php index 31caa5b27..5026380d7 100644 --- a/lib/Db/Board.php +++ b/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; + } } diff --git a/lib/Db/BoardMapper.php b/lib/Db/BoardMapper.php index 75796a6fc..2a204c50f 100644 --- a/lib/Db/BoardMapper.php +++ b/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; } diff --git a/lib/Service/BoardService.php b/lib/Service/BoardService.php index 71cf6172a..1ee0a0cf2 100644 --- a/lib/Service/BoardService.php +++ b/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); diff --git a/lib/Service/PermissionService.php b/lib/Service/PermissionService.php index 347863ece..8691b930b 100644 --- a/lib/Service/PermissionService.php +++ b/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; diff --git a/tests/unit/Db/BoardTest.php b/tests/unit/Db/BoardTest.php index e7bef3328..aa742ea8b 100644 --- a/tests/unit/Db/BoardTest.php +++ b/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' => [], From 4ec57d337b49eead0473fac770493ab449884cd8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Wed, 24 Nov 2021 10:08:41 +0100 Subject: [PATCH 3/3] Keep API results the same as before MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- lib/Db/Board.php | 4 ++++ tests/unit/Db/BoardTest.php | 33 ++++++++++++++++++++++++++++----- 2 files changed, 32 insertions(+), 5 deletions(-) diff --git a/lib/Db/Board.php b/lib/Db/Board.php index 5026380d7..9125c2ca7 100644 --- a/lib/Db/Board.php +++ b/lib/Db/Board.php @@ -63,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; } diff --git a/tests/unit/Db/BoardTest.php b/tests/unit/Db/BoardTest.php index aa742ea8b..0c1814705 100644 --- a/tests/unit/Db/BoardTest.php +++ b/tests/unit/Db/BoardTest.php @@ -23,12 +23,35 @@ class BoardTest extends TestCase { 'title' => "My Board", 'owner' => "admin", 'color' => "000000", - 'labels' => null, + 'labels' => [], 'permissions' => [], 'stacks' => [], 'deletedAt' => 0, 'lastModified' => 0, - 'acl' => null, + 'acl' => [], + 'archived' => false, + 'users' => ['user1', 'user2'], + 'settings' => [], + 'ETag' => $board->getETag(), + ], $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' => [], @@ -49,7 +72,7 @@ class BoardTest extends TestCase { 'stacks' => [], 'deletedAt' => 0, 'lastModified' => 0, - 'acl' => null, + 'acl' => [], 'archived' => false, 'users' => [], 'settings' => [], @@ -72,12 +95,12 @@ class BoardTest extends TestCase { 'title' => "My Board", 'owner' => "admin", 'color' => "000000", - 'labels' => null, + 'labels' => [], 'permissions' => [], 'stacks' => [], 'deletedAt' => 0, 'lastModified' => 0, - 'acl' => null, + 'acl' => [], 'archived' => false, 'shared' => 1, 'users' => [],