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 dc9cbd5f8..f8b6afc33 100644 --- a/lib/Service/AttachmentService.php +++ b/lib/Service/AttachmentService.php @@ -34,11 +34,10 @@ 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; @@ -139,14 +139,16 @@ public function findAll($cardId, $withDeleted = false) { * @param $cardId * @return int|mixed * @throws BadRequestException + * @throws InvalidAttachmentType + * @throws \OCP\DB\Exception */ 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 (!$count) { + $count = $this->attachmentCacheHelper->getAttachmentCount((int)$cardId); + if ($count === null) { $count = count($this->attachmentMapper->findAll($cardId)); foreach (array_keys($this->services) as $attachmentType) { @@ -156,7 +158,7 @@ public function count($cardId) { } } - $this->cache->set('card-' . $cardId, $count); + $this->attachmentCacheHelper->setAttachmentCount((int)$cardId, $count); } return $count; @@ -186,7 +188,7 @@ public function create($cardId, $type, $data) { $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); @@ -298,7 +300,7 @@ public function update($cardId, $attachmentId, $data, $type = 'deck_file') { } $this->permissionService->checkPermission($this->cardMapper, $attachment->getCardId(), Acl::PERMISSION_EDIT); - $this->cache->clear('card-' . $attachment->getCardId()); + $this->attachmentCacheHelper->clearAttachmentCount($cardId); $attachment->setData($data); try { @@ -356,7 +358,7 @@ public function delete(int $cardId, int $attachmentId, string $type = 'deck_file } } - $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; @@ -370,7 +372,7 @@ public function restore(int $cardId, int $attachmentId, string $type = 'deck_fil } $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/DeckShareProvider.php b/lib/Sharing/DeckShareProvider.php index 503fa50a3..48a3fda6e 100644 --- a/lib/Sharing/DeckShareProvider.php +++ b/lib/Sharing/DeckShareProvider.php @@ -28,6 +28,7 @@ use Doctrine\DBAL\Platforms\MySQLPlatform; 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; @@ -46,7 +47,6 @@ use OCP\Files\Node; use OCP\IDBConnection; use OCP\IL10N; -use OCP\Security\ISecureRandom; use OCP\Share\Exceptions\GenericShareException; use OCP\Share\Exceptions\ShareNotFound; use OCP\Share\IManager; @@ -70,6 +70,8 @@ class DeckShareProvider implements \OCP\Share\IShareProvider { private $dbConnection; /** @var IManager */ private $shareManager; + /** @var AttachmentCacheHelper */ + private $attachmentCacheHelper; /** @var BoardMapper */ private $boardMapper; /** @var CardMapper */ @@ -78,14 +80,25 @@ class DeckShareProvider implements \OCP\Share\IShareProvider { private $permissionService; /** @var ITimeFactory */ private $timeFactory; + /** @var IL10N */ 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, + BoardMapper $boardMapper, + CardMapper $cardMapper, + PermissionService $permissionService, + AttachmentCacheHelper $attachmentCacheHelper, + IL10N $l + ) { $this->dbConnection = $connection; $this->shareManager = $shareManager; $this->boardMapper = $boardMapper; $this->cardMapper = $cardMapper; + $this->attachmentCacheHelper = $attachmentCacheHelper; $this->permissionService = $permissionService; + $this->l = $l; $this->timeFactory = \OC::$server->get(ITimeFactory::class); } @@ -153,6 +166,8 @@ public function create(IShare $share) { ); $data = $this->getRawShare($shareId); + $this->attachmentCacheHelper->clearAttachmentCount((int)$cardId); + return $this->createShareObject($data); } @@ -340,6 +355,8 @@ public function delete(IShare $share) { $qb->orWhere($qb->expr()->eq('parent', $qb->createNamedParameter($share->getId()))); $qb->execute(); + + $this->attachmentCacheHelper->clearAttachmentCount((int)$share->getSharedWith()); } /** diff --git a/src/components/card/AttachmentList.vue b/src/components/card/AttachmentList.vue index 626df017e..fd35954f7 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,7 +236,7 @@ export default { shareType: 12, shareWith: '' + this.cardId, }).then(() => { - this.$store.dispatch('fetchAttachments', this.cardId) + this.fetchAttachments(this.cardId) }) }) }, 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 6dbea6112..099715c9a 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 e90692b8a..54778ba65 100644 --- a/tests/unit/Service/AttachmentServiceTest.php +++ b/tests/unit/Service/AttachmentServiceTest.php @@ -25,6 +25,7 @@ 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; @@ -35,8 +36,6 @@ use OCA\Deck\NotFoundException; use OCP\AppFramework\Http\Response; use OCP\AppFramework\IAppContainer; -use OCP\ICache; -use OCP\ICacheFactory; use OCP\IL10N; use PHPUnit\Framework\MockObject\MockObject; use Test\TestCase; @@ -67,10 +66,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 +78,6 @@ class AttachmentServiceTest extends TestCase { /** @var ActivityManager */ private $activityManager; private $appContainer; - /** ICache */ - private $cache; /** @var IL10N */ private $l10n; /** @var ChangeHelper */ @@ -104,12 +102,9 @@ public function setUp(): void { $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 +123,17 @@ public function setUp(): void { $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 +158,7 @@ public function testRegisterAttachmentService() { $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 +188,7 @@ public function testRegisterAttachmentServiceNotExisting() { ->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 +267,14 @@ public function testFindAllWithDeleted() { } 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 +282,7 @@ public function testCreate() { $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 +335,7 @@ public function testUpdate() { $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 +362,7 @@ public function testDelete() { $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 +383,7 @@ public function testDeleteWithUndo() { $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 +408,7 @@ public function testRestore() { $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 +429,7 @@ public function testRestoreNotAllowed() { $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)