From 2af94410f51417c68fa72de77cfef5d9d2dc5951 Mon Sep 17 00:00:00 2001 From: Luka Trovic Date: Thu, 31 Mar 2022 22:44:50 +0200 Subject: [PATCH 1/4] fix: increase file count after sharing Signed-off-by: Luka Trovic --- src/components/card/AttachmentList.vue | 1 + 1 file changed, 1 insertion(+) diff --git a/src/components/card/AttachmentList.vue b/src/components/card/AttachmentList.vue index 3c64b4fef..f28b05ddb 100644 --- a/src/components/card/AttachmentList.vue +++ b/src/components/card/AttachmentList.vue @@ -234,6 +234,7 @@ export default { shareWith: '' + this.cardId, }).then(() => { this.$store.dispatch('fetchAttachments', this.cardId) + this.$store.commit('cardIncreaseAttachmentCount', this.cardId) }) }) }, From 0b6990f8284b53a2380986b43dddf46e81b3ff4e Mon Sep 17 00:00:00 2001 From: Luka Trovic Date: Wed, 27 Apr 2022 09:40:27 +0200 Subject: [PATCH 2/4] fix: update attachments count when sharing Signed-off-by: Luka Trovic --- lib/Service/AttachmentService.php | 5 +++-- lib/Sharing/DeckShareProvider.php | 18 +++++++++++++++++- src/components/card/AttachmentList.vue | 10 ++++++---- 3 files changed, 26 insertions(+), 7 deletions(-) diff --git a/lib/Service/AttachmentService.php b/lib/Service/AttachmentService.php index dc9cbd5f8..46668b8b5 100644 --- a/lib/Service/AttachmentService.php +++ b/lib/Service/AttachmentService.php @@ -137,16 +137,17 @@ class AttachmentService { /** * @param $cardId + * @param bool $update | Force the update of the cached values * @return int|mixed * @throws BadRequestException */ - public function count($cardId) { + public function count($cardId, $update = false) { if (is_numeric($cardId) === false) { throw new BadRequestException('card id must be a number'); } $count = $this->cache->get('card-' . $cardId); - if (!$count) { + if ($update OR !$count) { $count = count($this->attachmentMapper->findAll($cardId)); foreach (array_keys($this->services) as $attachmentType) { diff --git a/lib/Sharing/DeckShareProvider.php b/lib/Sharing/DeckShareProvider.php index 6ac48ea8c..bac0ea6da 100644 --- a/lib/Sharing/DeckShareProvider.php +++ b/lib/Sharing/DeckShareProvider.php @@ -34,6 +34,7 @@ use OCA\Deck\Db\CardMapper; use OCA\Deck\Db\User; use OCA\Deck\NoPermissionException; use OCA\Deck\Service\PermissionService; +use OCA\Deck\Service\AttachmentService; use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Db\MultipleObjectsReturnedException; use OCP\AppFramework\Utility\ITimeFactory; @@ -75,16 +76,29 @@ class DeckShareProvider implements \OCP\Share\IShareProvider { private $cardMapper; /** @var PermissionService */ private $permissionService; + /** @var AttachmentService */ + private $attachmentService; /** @var ITimeFactory */ private $timeFactory; private $l; - public function __construct(IDBConnection $connection, IManager $shareManager, ISecureRandom $secureRandom, BoardMapper $boardMapper, CardMapper $cardMapper, PermissionService $permissionService, IL10N $l) { + public function __construct( + IDBConnection $connection, + IManager $shareManager, + ISecureRandom $secureRandom, + BoardMapper $boardMapper, + CardMapper $cardMapper, + AttachmentService $attachmentService, + PermissionService $permissionService, + IL10N $l + ) { $this->dbConnection = $connection; $this->shareManager = $shareManager; $this->boardMapper = $boardMapper; $this->cardMapper = $cardMapper; + $this->attachmentService = $attachmentService; $this->permissionService = $permissionService; + $this->l = $l; $this->timeFactory = \OC::$server->get(ITimeFactory::class); } @@ -152,6 +166,8 @@ class DeckShareProvider implements \OCP\Share\IShareProvider { ); $data = $this->getRawShare($shareId); + $this->attachmentService->count($cardId, true); + return $this->createShareObject($data); } diff --git a/src/components/card/AttachmentList.vue b/src/components/card/AttachmentList.vue index f28b05ddb..34d46d35c 100644 --- a/src/components/card/AttachmentList.vue +++ b/src/components/card/AttachmentList.vue @@ -109,7 +109,7 @@ import relativeDate from '../../mixins/relativeDate' import { formatFileSize } from '@nextcloud/files' import { getCurrentUser } from '@nextcloud/auth' import { generateUrl, generateOcsUrl, generateRemoteUrl } from '@nextcloud/router' -import { mapState } from 'vuex' +import { mapState, mapActions } from 'vuex' import { loadState } from '@nextcloud/initial-state' import attachmentUpload from '../../mixins/attachmentUpload' import { getFilePickerBuilder } from '@nextcloud/dialogs' @@ -205,11 +205,14 @@ export default { cardId: { immediate: true, handler() { - this.$store.dispatch('fetchAttachments', this.cardId) + this.fetchAttachments(this.cardId) }, }, }, methods: { + ...mapActions([ + "fetchAttachments", + ]), handleUploadFile(event) { const files = event.target.files ?? [] for (const file of files) { @@ -233,8 +236,7 @@ export default { shareType: 12, shareWith: '' + this.cardId, }).then(() => { - this.$store.dispatch('fetchAttachments', this.cardId) - this.$store.commit('cardIncreaseAttachmentCount', this.cardId) + this.fetchAttachments(this.cardId) }) }) }, From ed3be361b53b1727c1af130db2b1f3ec55a91330 Mon Sep 17 00:00:00 2001 From: Luka Trovic Date: Fri, 29 Apr 2022 14:21:22 +0200 Subject: [PATCH 3/4] fix: move shares count cache logic to the DeckShareProvider Signed-off-by: Luka Trovic fix: conflicts Signed-off-by: Luka Trovic fix: conflicts and test issues Signed-off-by: Luka Trovic --- lib/Service/AttachmentService.php | 4 +- lib/Sharing/DeckShareFileCountHelper.php | 145 +++++++++++++++++++++++ lib/Sharing/DeckShareProvider.php | 63 +++++++--- src/components/card/AttachmentList.vue | 2 +- 4 files changed, 197 insertions(+), 17 deletions(-) create mode 100644 lib/Sharing/DeckShareFileCountHelper.php diff --git a/lib/Service/AttachmentService.php b/lib/Service/AttachmentService.php index 46668b8b5..89614864b 100644 --- a/lib/Service/AttachmentService.php +++ b/lib/Service/AttachmentService.php @@ -137,7 +137,7 @@ class AttachmentService { /** * @param $cardId - * @param bool $update | Force the update of the cached values + * @param bool $update | Force the update of the cached values * @return int|mixed * @throws BadRequestException */ @@ -147,7 +147,7 @@ class AttachmentService { } $count = $this->cache->get('card-' . $cardId); - if ($update OR !$count) { + if ($update || !$count) { $count = count($this->attachmentMapper->findAll($cardId)); foreach (array_keys($this->services) as $attachmentType) { diff --git a/lib/Sharing/DeckShareFileCountHelper.php b/lib/Sharing/DeckShareFileCountHelper.php new file mode 100644 index 000000000..1f728c78c --- /dev/null +++ b/lib/Sharing/DeckShareFileCountHelper.php @@ -0,0 +1,145 @@ + + * + * @author Julius Härtl + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +declare(strict_types=1); + + +namespace OCA\Deck\Sharing; + +use OCA\Deck\Db\AttachmentMapper; +use OCA\Deck\BadRequestException; +use OCP\Share\IShare; +use OCP\IDBConnection; + +class DeckShareFileCountHelper { + + /** + * @var array + */ + private $processors = []; + private AttachmentMapper $attachmentMapper; + + public function __construct(AttachmentMapper $attachmentMapper) { + $this->attachmentMapper = $attachmentMapper; + $this->registerProcessors(); + } + + /** + * @param $cardId + * @return int + * @throws BadRequestException + */ + public function getAttachmentCount($cardId) { + $count = $this->countDeckFile($cardId); + foreach (array_keys($this->processors) as $type) { + if ($processor = $this->processor($type)) { + $count += $this->{$processor}((int)$cardId); + } + } + + return $count; + } + + /** + * @param string $type + * @return string|null + * @throws BadRequestException + */ + private function processor(string $type) { + if (!array_key_exists($type, $this->processors)) { + throw new BadRequestException('This type of file is not found'); + } + + return $this->processors[$type]; + } + + private function registerProcessors() { + return $this->processors = [ + 'deck_file' => null, + 'file' => 'countFile' + ]; + } + + /** + * Count deck files + * + * @param $cardId + * @return int + */ + private function countDeckFile($cardId) { + return count($this->attachmentMapper->findAll($cardId)); + } + + /** + * Count files other than the deck ones + * + * @param int $cardId + * @return int + */ + private function countFile(int $cardId) { + /** @var IDBConnection $qb */ + $db = \OC::$server->getDatabaseConnection(); + $qb = $db->getQueryBuilder(); + $qb->select('s.id', 'f.fileid', 'f.path') + ->selectAlias('st.id', 'storage_string_id') + ->from('share', 's') + ->leftJoin('s', 'filecache', 'f', $qb->expr()->eq('s.file_source', 'f.fileid')) + ->leftJoin('f', 'storages', 'st', $qb->expr()->eq('f.storage', 'st.numeric_id')) + ->andWhere($qb->expr()->eq('s.share_type', $qb->createNamedParameter(IShare::TYPE_DECK))) + ->andWhere($qb->expr()->eq('s.share_with', $qb->createNamedParameter($cardId))) + ->andWhere($qb->expr()->isNull('s.parent')) + ->andWhere($qb->expr()->orX( + $qb->expr()->eq('s.item_type', $qb->createNamedParameter('file')), + $qb->expr()->eq('s.item_type', $qb->createNamedParameter('folder')) + )); + + $count = 0; + $cursor = $qb->execute(); + while ($data = $cursor->fetch()) { + if ($this->isAccessibleResult($data)) { + $count++; + } + } + $cursor->closeCursor(); + return $count; + } + + private function isAccessibleResult(array $data): bool { + // exclude shares leading to deleted file entries + if ($data['fileid'] === null || $data['path'] === null) { + return false; + } + + // exclude shares leading to trashbin on home storages + $pathSections = explode('/', $data['path'], 2); + // FIXME: would not detect rare md5'd home storage case properly + if ( + $pathSections[0] !== 'files' + && in_array(explode(':', $data['storage_string_id'], 2)[0], ['home', 'object']) + ) { + return false; + } + return true; + } +} diff --git a/lib/Sharing/DeckShareProvider.php b/lib/Sharing/DeckShareProvider.php index bac0ea6da..5ac0fbbcb 100644 --- a/lib/Sharing/DeckShareProvider.php +++ b/lib/Sharing/DeckShareProvider.php @@ -32,9 +32,11 @@ use OCA\Deck\Db\Board; use OCA\Deck\Db\BoardMapper; use OCA\Deck\Db\CardMapper; use OCA\Deck\Db\User; +use OCA\Deck\BadRequestException; use OCA\Deck\NoPermissionException; +use OCA\Deck\InvalidAttachmentType; use OCA\Deck\Service\PermissionService; -use OCA\Deck\Service\AttachmentService; +use OCA\Deck\Service\IAttachmentService; use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Db\MultipleObjectsReturnedException; use OCP\AppFramework\Utility\ITimeFactory; @@ -44,9 +46,10 @@ use OCP\EventDispatcher\IEventDispatcher; use OCP\Files\Folder; use OCP\Files\IMimeTypeLoader; use OCP\Files\Node; +use OCP\ICache; +use OCP\ICacheFactory; use OCP\IDBConnection; use OCP\IL10N; -use OCP\Security\ISecureRandom; use OCP\Share\Exceptions\GenericShareException; use OCP\Share\Exceptions\ShareNotFound; use OCP\Share\IManager; @@ -66,41 +69,46 @@ class DeckShareProvider implements \OCP\Share\IShareProvider { public const SHARE_TYPE_DECK_USER = IShare::TYPE_DECK_USER; + /** @var IAttachmentService[] */ + private $services = []; /** @var IDBConnection */ private $dbConnection; /** @var IManager */ private $shareManager; + /** @var DeckShareFileCountHelper */ + private $deckShareFileCountHelper; /** @var BoardMapper */ private $boardMapper; /** @var CardMapper */ private $cardMapper; /** @var PermissionService */ private $permissionService; - /** @var AttachmentService */ - private $attachmentService; /** @var ITimeFactory */ + /** @var ICache */ + private $cache; private $timeFactory; private $l; public function __construct( - IDBConnection $connection, - IManager $shareManager, - ISecureRandom $secureRandom, - BoardMapper $boardMapper, - CardMapper $cardMapper, - AttachmentService $attachmentService, - PermissionService $permissionService, + IDBConnection $connection, + IManager $shareManager, + BoardMapper $boardMapper, + CardMapper $cardMapper, + PermissionService $permissionService, + ICacheFactory $cacheFactory, + DeckShareFileCountHelper $deckShareFileCountHelper, IL10N $l ) { $this->dbConnection = $connection; $this->shareManager = $shareManager; $this->boardMapper = $boardMapper; $this->cardMapper = $cardMapper; - $this->attachmentService = $attachmentService; + $this->deckShareFileCountHelper = $deckShareFileCountHelper; $this->permissionService = $permissionService; $this->l = $l; $this->timeFactory = \OC::$server->get(ITimeFactory::class); + $this->cache = $cacheFactory->createDistributed('deck-card-attachments-'); } public static function register(IEventDispatcher $dispatcher): void { @@ -165,12 +173,39 @@ class DeckShareProvider implements \OCP\Share\IShareProvider { $share->getExpirationDate() ); $data = $this->getRawShare($shareId); - - $this->attachmentService->count($cardId, true); + $this->updateCardAttachmentsCountCache($cardId); return $this->createShareObject($data); } + /** + * @param $cardId + * @return int|mixed + * @throws BadRequestException + */ + private function updateCardAttachmentsCountCache($cardId) { + if (is_numeric($cardId) === false) { + throw new BadRequestException('card id must be a number'); + } + + $count = $this->deckShareFileCountHelper->getAttachmentCount($cardId); + $this->cache->set('card-' . $cardId, $count); + + return $count; + } + + /** + * @param string $type + * @return IAttachmentService + * @throws InvalidAttachmentType + */ + public function getService($type) { + if (isset($this->services[$type])) { + return $this->services[$type]; + } + throw new InvalidAttachmentType($type); + } + /** * Add share to the database and return the ID * diff --git a/src/components/card/AttachmentList.vue b/src/components/card/AttachmentList.vue index 34d46d35c..f6cbd7053 100644 --- a/src/components/card/AttachmentList.vue +++ b/src/components/card/AttachmentList.vue @@ -211,7 +211,7 @@ export default { }, methods: { ...mapActions([ - "fetchAttachments", + 'fetchAttachments', ]), handleUploadFile(event) { const files = event.target.files ?? [] From 03f400620e2ae6301636050b20a8144c343f46be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Fri, 6 May 2022 11:32:59 +0200 Subject: [PATCH 4/4] Move all caching to helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- lib/Cache/AttachmentCacheHelper.php | 52 +++++++ lib/Service/AttachmentService.php | 31 ++-- lib/Sharing/DeckShareFileCountHelper.php | 145 ------------------- lib/Sharing/DeckShareProvider.php | 54 ++----- src/store/attachment.js | 1 + src/store/card.js | 6 + tests/unit/Service/AttachmentServiceTest.php | 45 +++--- 7 files changed, 111 insertions(+), 223 deletions(-) create mode 100644 lib/Cache/AttachmentCacheHelper.php delete mode 100644 lib/Sharing/DeckShareFileCountHelper.php diff --git a/lib/Cache/AttachmentCacheHelper.php b/lib/Cache/AttachmentCacheHelper.php new file mode 100644 index 000000000..95eb1aa5a --- /dev/null +++ b/lib/Cache/AttachmentCacheHelper.php @@ -0,0 +1,52 @@ + + * + * @author Julius Härtl + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +declare(strict_types=1); + + +namespace OCA\Deck\Cache; + +use OCP\ICache; +use OCP\ICacheFactory; + +class AttachmentCacheHelper { + /** @var ICache */ + private $cache; + + public function __construct(ICacheFactory $cacheFactory) { + $this->cache = $cacheFactory->createDistributed('deck-attachments'); + } + + public function getAttachmentCount(int $cardId): ?int { + return $this->cache->get('count-' . $cardId); + } + + public function setAttachmentCount(int $cardId, int $count): void { + $this->cache->set('count-' . $cardId, $count); + } + + public function clearAttachmentCount(int $cardId): void { + $this->cache->remove('count-' . $cardId); + } +} diff --git a/lib/Service/AttachmentService.php b/lib/Service/AttachmentService.php index 89614864b..f8b6afc33 100644 --- a/lib/Service/AttachmentService.php +++ b/lib/Service/AttachmentService.php @@ -34,11 +34,10 @@ use OCA\Deck\Db\ChangeHelper; use OCA\Deck\InvalidAttachmentType; use OCA\Deck\NoPermissionException; use OCA\Deck\NotFoundException; +use OCA\Deck\Cache\AttachmentCacheHelper; use OCA\Deck\StatusException; use OCP\AppFramework\Db\IMapperException; use OCP\AppFramework\Http\Response; -use OCP\ICache; -use OCP\ICacheFactory; use OCP\IL10N; class AttachmentService { @@ -49,9 +48,10 @@ class AttachmentService { /** @var IAttachmentService[] */ private $services = []; + /** @var Application */ private $application; - /** @var ICache */ - private $cache; + /** @var AttachmentCacheHelper */ + private $attachmentCacheHelper; /** @var IL10N */ private $l10n; /** @var ActivityManager */ @@ -59,13 +59,13 @@ class AttachmentService { /** @var ChangeHelper */ private $changeHelper; - public function __construct(AttachmentMapper $attachmentMapper, CardMapper $cardMapper, ChangeHelper $changeHelper, PermissionService $permissionService, Application $application, ICacheFactory $cacheFactory, $userId, IL10N $l10n, ActivityManager $activityManager) { + public function __construct(AttachmentMapper $attachmentMapper, CardMapper $cardMapper, ChangeHelper $changeHelper, PermissionService $permissionService, Application $application, AttachmentCacheHelper $attachmentCacheHelper, $userId, IL10N $l10n, ActivityManager $activityManager) { $this->attachmentMapper = $attachmentMapper; $this->cardMapper = $cardMapper; $this->permissionService = $permissionService; $this->userId = $userId; $this->application = $application; - $this->cache = $cacheFactory->createDistributed('deck-card-attachments-'); + $this->attachmentCacheHelper = $attachmentCacheHelper; $this->l10n = $l10n; $this->activityManager = $activityManager; $this->changeHelper = $changeHelper; @@ -137,17 +137,18 @@ class AttachmentService { /** * @param $cardId - * @param bool $update | Force the update of the cached values * @return int|mixed * @throws BadRequestException + * @throws InvalidAttachmentType + * @throws \OCP\DB\Exception */ - public function count($cardId, $update = false) { + public function count($cardId) { if (is_numeric($cardId) === false) { throw new BadRequestException('card id must be a number'); } - $count = $this->cache->get('card-' . $cardId); - if ($update || !$count) { + $count = $this->attachmentCacheHelper->getAttachmentCount((int)$cardId); + if ($count === null) { $count = count($this->attachmentMapper->findAll($cardId)); foreach (array_keys($this->services) as $attachmentType) { @@ -157,7 +158,7 @@ class AttachmentService { } } - $this->cache->set('card-' . $cardId, $count); + $this->attachmentCacheHelper->setAttachmentCount((int)$cardId, $count); } return $count; @@ -187,7 +188,7 @@ class AttachmentService { $this->permissionService->checkPermission($this->cardMapper, $cardId, Acl::PERMISSION_EDIT); - $this->cache->clear('card-' . $cardId); + $this->attachmentCacheHelper->clearAttachmentCount((int)$cardId); $attachment = new Attachment(); $attachment->setCardId($cardId); $attachment->setType($type); @@ -299,7 +300,7 @@ class AttachmentService { } $this->permissionService->checkPermission($this->cardMapper, $attachment->getCardId(), Acl::PERMISSION_EDIT); - $this->cache->clear('card-' . $attachment->getCardId()); + $this->attachmentCacheHelper->clearAttachmentCount($cardId); $attachment->setData($data); try { @@ -357,7 +358,7 @@ class AttachmentService { } } - $this->cache->clear('card-' . $attachment->getCardId()); + $this->attachmentCacheHelper->clearAttachmentCount($cardId); $this->changeHelper->cardChanged($attachment->getCardId()); $this->activityManager->triggerEvent(ActivityManager::DECK_OBJECT_CARD, $attachment, ActivityManager::SUBJECT_ATTACHMENT_DELETE); return $attachment; @@ -371,7 +372,7 @@ class AttachmentService { } $this->permissionService->checkPermission($this->cardMapper, $attachment->getCardId(), Acl::PERMISSION_EDIT); - $this->cache->clear('card-' . $attachment->getCardId()); + $this->attachmentCacheHelper->clearAttachmentCount($cardId); try { $service = $this->getService($attachment->getType()); diff --git a/lib/Sharing/DeckShareFileCountHelper.php b/lib/Sharing/DeckShareFileCountHelper.php deleted file mode 100644 index 1f728c78c..000000000 --- a/lib/Sharing/DeckShareFileCountHelper.php +++ /dev/null @@ -1,145 +0,0 @@ - - * - * @author Julius Härtl - * - * @license GNU AGPL version 3 or any later version - * - * This program is free software: you can redistribute it and/or modify - * it under the terms of the GNU Affero General Public License as - * published by the Free Software Foundation, either version 3 of the - * License, or (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU Affero General Public License for more details. - * - * You should have received a copy of the GNU Affero General Public License - * along with this program. If not, see . - * - */ - -declare(strict_types=1); - - -namespace OCA\Deck\Sharing; - -use OCA\Deck\Db\AttachmentMapper; -use OCA\Deck\BadRequestException; -use OCP\Share\IShare; -use OCP\IDBConnection; - -class DeckShareFileCountHelper { - - /** - * @var array - */ - private $processors = []; - private AttachmentMapper $attachmentMapper; - - public function __construct(AttachmentMapper $attachmentMapper) { - $this->attachmentMapper = $attachmentMapper; - $this->registerProcessors(); - } - - /** - * @param $cardId - * @return int - * @throws BadRequestException - */ - public function getAttachmentCount($cardId) { - $count = $this->countDeckFile($cardId); - foreach (array_keys($this->processors) as $type) { - if ($processor = $this->processor($type)) { - $count += $this->{$processor}((int)$cardId); - } - } - - return $count; - } - - /** - * @param string $type - * @return string|null - * @throws BadRequestException - */ - private function processor(string $type) { - if (!array_key_exists($type, $this->processors)) { - throw new BadRequestException('This type of file is not found'); - } - - return $this->processors[$type]; - } - - private function registerProcessors() { - return $this->processors = [ - 'deck_file' => null, - 'file' => 'countFile' - ]; - } - - /** - * Count deck files - * - * @param $cardId - * @return int - */ - private function countDeckFile($cardId) { - return count($this->attachmentMapper->findAll($cardId)); - } - - /** - * Count files other than the deck ones - * - * @param int $cardId - * @return int - */ - private function countFile(int $cardId) { - /** @var IDBConnection $qb */ - $db = \OC::$server->getDatabaseConnection(); - $qb = $db->getQueryBuilder(); - $qb->select('s.id', 'f.fileid', 'f.path') - ->selectAlias('st.id', 'storage_string_id') - ->from('share', 's') - ->leftJoin('s', 'filecache', 'f', $qb->expr()->eq('s.file_source', 'f.fileid')) - ->leftJoin('f', 'storages', 'st', $qb->expr()->eq('f.storage', 'st.numeric_id')) - ->andWhere($qb->expr()->eq('s.share_type', $qb->createNamedParameter(IShare::TYPE_DECK))) - ->andWhere($qb->expr()->eq('s.share_with', $qb->createNamedParameter($cardId))) - ->andWhere($qb->expr()->isNull('s.parent')) - ->andWhere($qb->expr()->orX( - $qb->expr()->eq('s.item_type', $qb->createNamedParameter('file')), - $qb->expr()->eq('s.item_type', $qb->createNamedParameter('folder')) - )); - - $count = 0; - $cursor = $qb->execute(); - while ($data = $cursor->fetch()) { - if ($this->isAccessibleResult($data)) { - $count++; - } - } - $cursor->closeCursor(); - return $count; - } - - private function isAccessibleResult(array $data): bool { - // exclude shares leading to deleted file entries - if ($data['fileid'] === null || $data['path'] === null) { - return false; - } - - // exclude shares leading to trashbin on home storages - $pathSections = explode('/', $data['path'], 2); - // FIXME: would not detect rare md5'd home storage case properly - if ( - $pathSections[0] !== 'files' - && in_array(explode(':', $data['storage_string_id'], 2)[0], ['home', 'object']) - ) { - return false; - } - return true; - } -} diff --git a/lib/Sharing/DeckShareProvider.php b/lib/Sharing/DeckShareProvider.php index 5ac0fbbcb..cff74fab0 100644 --- a/lib/Sharing/DeckShareProvider.php +++ b/lib/Sharing/DeckShareProvider.php @@ -27,16 +27,14 @@ declare(strict_types=1); namespace OCA\Deck\Sharing; use OC\Files\Cache\Cache; +use OCA\Deck\Cache\AttachmentCacheHelper; use OCA\Deck\Db\Acl; use OCA\Deck\Db\Board; use OCA\Deck\Db\BoardMapper; use OCA\Deck\Db\CardMapper; use OCA\Deck\Db\User; -use OCA\Deck\BadRequestException; use OCA\Deck\NoPermissionException; -use OCA\Deck\InvalidAttachmentType; use OCA\Deck\Service\PermissionService; -use OCA\Deck\Service\IAttachmentService; use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Db\MultipleObjectsReturnedException; use OCP\AppFramework\Utility\ITimeFactory; @@ -46,8 +44,6 @@ use OCP\EventDispatcher\IEventDispatcher; use OCP\Files\Folder; use OCP\Files\IMimeTypeLoader; use OCP\Files\Node; -use OCP\ICache; -use OCP\ICacheFactory; use OCP\IDBConnection; use OCP\IL10N; use OCP\Share\Exceptions\GenericShareException; @@ -69,14 +65,12 @@ class DeckShareProvider implements \OCP\Share\IShareProvider { public const SHARE_TYPE_DECK_USER = IShare::TYPE_DECK_USER; - /** @var IAttachmentService[] */ - private $services = []; /** @var IDBConnection */ private $dbConnection; /** @var IManager */ private $shareManager; - /** @var DeckShareFileCountHelper */ - private $deckShareFileCountHelper; + /** @var AttachmentCacheHelper */ + private $attachmentCacheHelper; /** @var BoardMapper */ private $boardMapper; /** @var CardMapper */ @@ -84,9 +78,8 @@ class DeckShareProvider implements \OCP\Share\IShareProvider { /** @var PermissionService */ private $permissionService; /** @var ITimeFactory */ - /** @var ICache */ - private $cache; private $timeFactory; + /** @var IL10N */ private $l; public function __construct( @@ -95,20 +88,18 @@ class DeckShareProvider implements \OCP\Share\IShareProvider { BoardMapper $boardMapper, CardMapper $cardMapper, PermissionService $permissionService, - ICacheFactory $cacheFactory, - DeckShareFileCountHelper $deckShareFileCountHelper, + AttachmentCacheHelper $attachmentCacheHelper, IL10N $l ) { $this->dbConnection = $connection; $this->shareManager = $shareManager; $this->boardMapper = $boardMapper; $this->cardMapper = $cardMapper; - $this->deckShareFileCountHelper = $deckShareFileCountHelper; + $this->attachmentCacheHelper = $attachmentCacheHelper; $this->permissionService = $permissionService; $this->l = $l; $this->timeFactory = \OC::$server->get(ITimeFactory::class); - $this->cache = $cacheFactory->createDistributed('deck-card-attachments-'); } public static function register(IEventDispatcher $dispatcher): void { @@ -173,39 +164,12 @@ class DeckShareProvider implements \OCP\Share\IShareProvider { $share->getExpirationDate() ); $data = $this->getRawShare($shareId); - $this->updateCardAttachmentsCountCache($cardId); + + $this->attachmentCacheHelper->clearAttachmentCount((int)$cardId); return $this->createShareObject($data); } - /** - * @param $cardId - * @return int|mixed - * @throws BadRequestException - */ - private function updateCardAttachmentsCountCache($cardId) { - if (is_numeric($cardId) === false) { - throw new BadRequestException('card id must be a number'); - } - - $count = $this->deckShareFileCountHelper->getAttachmentCount($cardId); - $this->cache->set('card-' . $cardId, $count); - - return $count; - } - - /** - * @param string $type - * @return IAttachmentService - * @throws InvalidAttachmentType - */ - public function getService($type) { - if (isset($this->services[$type])) { - return $this->services[$type]; - } - throw new InvalidAttachmentType($type); - } - /** * Add share to the database and return the ID * @@ -390,6 +354,8 @@ class DeckShareProvider implements \OCP\Share\IShareProvider { $qb->orWhere($qb->expr()->eq('parent', $qb->createNamedParameter($share->getId()))); $qb->execute(); + + $this->attachmentCacheHelper->clearAttachmentCount((int)$share->getSharedWith()); } /** diff --git a/src/store/attachment.js b/src/store/attachment.js index 2a3892f04..1115b3405 100644 --- a/src/store/attachment.js +++ b/src/store/attachment.js @@ -84,6 +84,7 @@ export default { async fetchAttachments({ commit }, cardId) { const attachments = await apiClient.fetchAttachments(cardId) commit('createAttachments', { cardId, attachments }) + commit('cardSetAttachmentCount', { cardId, count: attachments.length }) }, async createAttachment({ commit }, { cardId, formData, onUploadProgress }) { diff --git a/src/store/card.js b/src/store/card.js index 06f6835fa..50e873f0a 100644 --- a/src/store/card.js +++ b/src/store/card.js @@ -252,6 +252,12 @@ export default { } Vue.set(state.cards[existingIndex], 'lastModified', Date.now() / 1000) }, + cardSetAttachmentCount(state, { cardId, count }) { + const existingIndex = state.cards.findIndex(_card => _card.id === cardId) + if (existingIndex !== -1) { + Vue.set(state.cards[existingIndex], 'attachmentCount', count) + } + }, cardIncreaseAttachmentCount(state, cardId) { const existingIndex = state.cards.findIndex(_card => _card.id === cardId) if (existingIndex !== -1) { diff --git a/tests/unit/Service/AttachmentServiceTest.php b/tests/unit/Service/AttachmentServiceTest.php index 3638cf7ea..4cd318a71 100644 --- a/tests/unit/Service/AttachmentServiceTest.php +++ b/tests/unit/Service/AttachmentServiceTest.php @@ -25,6 +25,7 @@ namespace OCA\Deck\Service; use OCA\Deck\Activity\ActivityManager; use OCA\Deck\AppInfo\Application; +use OCA\Deck\Cache\AttachmentCacheHelper; use OCA\Deck\Db\Acl; use OCA\Deck\Db\Attachment; use OCA\Deck\Db\AttachmentMapper; @@ -67,10 +68,11 @@ class AttachmentServiceTest extends TestCase { private $cardMapper; /** @var PermissionService|MockObject */ private $permissionService; + /** @var string */ private $userId = 'admin'; /** @var Application|MockObject */ private $application; - private $cacheFactory; + private $attachmentCacheHelper; /** @var AttachmentService */ private $attachmentService; /** @var MockObject */ @@ -78,8 +80,6 @@ class AttachmentServiceTest extends TestCase { /** @var ActivityManager */ private $activityManager; private $appContainer; - /** ICache */ - private $cache; /** @var IL10N */ private $l10n; /** @var ChangeHelper */ @@ -104,12 +104,9 @@ class AttachmentServiceTest extends TestCase { $this->cardMapper = $this->createMock(CardMapper::class); $this->permissionService = $this->createMock(PermissionService::class); $this->application = $this->createMock(Application::class); - $this->cacheFactory = $this->createMock(ICacheFactory::class); + $this->attachmentCacheHelper = $this->createMock(AttachmentCacheHelper::class); $this->activityManager = $this->createMock(ActivityManager::class); - $this->cache = $this->createMock(ICache::class); - $this->cacheFactory->expects($this->any())->method('createDistributed')->willReturn($this->cache); - $this->appContainer->expects($this->exactly(2)) ->method('query') ->withConsecutive( @@ -128,7 +125,17 @@ class AttachmentServiceTest extends TestCase { $this->l10n = $this->createMock(IL10N::class); $this->changeHelper = $this->createMock(ChangeHelper::class); - $this->attachmentService = new AttachmentService($this->attachmentMapper, $this->cardMapper, $this->changeHelper, $this->permissionService, $this->application, $this->cacheFactory, $this->userId, $this->l10n, $this->activityManager); + $this->attachmentService = new AttachmentService( + $this->attachmentMapper, + $this->cardMapper, + $this->changeHelper, + $this->permissionService, + $this->application, + $this->attachmentCacheHelper, + $this->userId, + $this->l10n, + $this->activityManager + ); } public function testRegisterAttachmentService() { @@ -153,7 +160,7 @@ class AttachmentServiceTest extends TestCase { $application->expects($this->any()) ->method('getContainer') ->willReturn($appContainer); - $attachmentService = new AttachmentService($this->attachmentMapper, $this->cardMapper, $this->changeHelper, $this->permissionService, $application, $this->cacheFactory, $this->userId, $this->l10n, $this->activityManager); + $attachmentService = new AttachmentService($this->attachmentMapper, $this->cardMapper, $this->changeHelper, $this->permissionService, $application, $this->attachmentCacheHelper, $this->userId, $this->l10n, $this->activityManager); $attachmentService->registerAttachmentService('custom', MyAttachmentService::class); $this->assertEquals($fileServiceMock, $attachmentService->getService('deck_file')); $this->assertEquals(MyAttachmentService::class, get_class($attachmentService->getService('custom'))); @@ -183,7 +190,7 @@ class AttachmentServiceTest extends TestCase { ->method('getContainer') ->willReturn($appContainer); - $attachmentService = new AttachmentService($this->attachmentMapper, $this->cardMapper, $this->changeHelper, $this->permissionService, $application, $this->cacheFactory, $this->userId, $this->l10n, $this->activityManager); + $attachmentService = new AttachmentService($this->attachmentMapper, $this->cardMapper, $this->changeHelper, $this->permissionService, $application, $this->attachmentCacheHelper, $this->userId, $this->l10n, $this->activityManager); $attachmentService->registerAttachmentService('custom', MyAttachmentService::class); $attachmentService->getService('deck_file_invalid'); } @@ -262,14 +269,14 @@ class AttachmentServiceTest extends TestCase { } public function testCount() { - $this->cache->expects($this->once())->method('get')->with('card-123')->willReturn(null); + $this->attachmentCacheHelper->expects($this->once())->method('getAttachmentCount')->with(123)->willReturn(null); $this->attachmentMapper->expects($this->once())->method('findAll')->willReturn([1,2,3,4]); - $this->cache->expects($this->once())->method('set')->with('card-123', 4)->willReturn(null); + $this->attachmentCacheHelper->expects($this->once())->method('setAttachmentCount')->with(123, 4); $this->assertEquals(4, $this->attachmentService->count(123)); } public function testCountCacheHit() { - $this->cache->expects($this->once())->method('get')->with('card-123')->willReturn(4); + $this->attachmentCacheHelper->expects($this->once())->method('getAttachmentCount')->with(123)->willReturn(4); $this->assertEquals(4, $this->attachmentService->count(123)); } @@ -277,7 +284,7 @@ class AttachmentServiceTest extends TestCase { $attachment = $this->createAttachment('deck_file', 'file_name.jpg'); $expected = $this->createAttachment('deck_file', 'file_name.jpg'); $this->mockPermission(Acl::PERMISSION_EDIT); - $this->cache->expects($this->once())->method('clear')->with('card-123'); + $this->attachmentCacheHelper->expects($this->once())->method('clearAttachmentCount')->with(123); $this->attachmentServiceImpl->expects($this->once()) ->method('create'); $this->attachmentMapper->expects($this->once()) @@ -330,7 +337,7 @@ class AttachmentServiceTest extends TestCase { $attachment = $this->createAttachment('deck_file', 'file_name.jpg'); $expected = $this->createAttachment('deck_file', 'file_name.jpg'); $this->mockPermission(Acl::PERMISSION_EDIT); - $this->cache->expects($this->once())->method('clear')->with('card-123'); + $this->attachmentCacheHelper->expects($this->once())->method('clearAttachmentCount')->with(1); $this->attachmentMapper->expects($this->once()) ->method('find') ->with(1) @@ -357,7 +364,7 @@ class AttachmentServiceTest extends TestCase { $attachment = $this->createAttachment('deck_file', 'file_name.jpg'); $expected = $this->createAttachment('deck_file', 'file_name.jpg'); $this->mockPermission(Acl::PERMISSION_EDIT); - $this->cache->expects($this->once())->method('clear')->with('card-123'); + $this->attachmentCacheHelper->expects($this->once())->method('clearAttachmentCount')->with(1); $this->attachmentMapper->expects($this->once()) ->method('find') ->with(1) @@ -378,7 +385,7 @@ class AttachmentServiceTest extends TestCase { $attachment = $this->createAttachment('deck_file', 'file_name.jpg'); $expected = $this->createAttachment('deck_file', 'file_name.jpg'); $this->mockPermission(Acl::PERMISSION_EDIT); - $this->cache->expects($this->once())->method('clear')->with('card-123'); + $this->attachmentCacheHelper->expects($this->once())->method('clearAttachmentCount')->with(1); $this->attachmentMapper->expects($this->once()) ->method('find') ->with(1) @@ -403,7 +410,7 @@ class AttachmentServiceTest extends TestCase { $attachment = $this->createAttachment('deck_file', 'file_name.jpg'); $expected = $this->createAttachment('deck_file', 'file_name.jpg'); $this->mockPermission(Acl::PERMISSION_EDIT); - $this->cache->expects($this->once())->method('clear')->with('card-123'); + $this->attachmentCacheHelper->expects($this->once())->method('clearAttachmentCount')->with(1); $this->attachmentMapper->expects($this->once()) ->method('find') ->with(1) @@ -424,7 +431,7 @@ class AttachmentServiceTest extends TestCase { $attachment = $this->createAttachment('deck_file', 'file_name.jpg'); $expected = $this->createAttachment('deck_file', 'file_name.jpg'); $this->mockPermission(Acl::PERMISSION_EDIT); - $this->cache->expects($this->once())->method('clear')->with('card-123'); + $this->attachmentCacheHelper->expects($this->once())->method('clearAttachmentCount')->with(1); $this->attachmentMapper->expects($this->once()) ->method('find') ->with(1)