diff --git a/lib/Controller/ChatController.php b/lib/Controller/ChatController.php index ea756443985..611c41fe1cf 100644 --- a/lib/Controller/ChatController.php +++ b/lib/Controller/ChatController.php @@ -40,6 +40,7 @@ use OCA\Talk\Service\AttachmentService; use OCA\Talk\Service\ParticipantService; use OCA\Talk\Service\SessionService; +use OCA\Talk\Share\RoomShareProvider; use OCP\App\IAppManager; use OCP\AppFramework\Http; use OCP\AppFramework\Http\DataResponse; @@ -74,6 +75,7 @@ class ChatController extends AEnvironmentAwareController { /** @var string[] */ protected array $guestNames; private MessageParser $messageParser; + protected RoomShareProvider $shareProvider; private IManager $autoCompleteManager; private IUserStatusManager $statusManager; protected MatterbridgeManager $matterbridgeManager; @@ -97,6 +99,7 @@ public function __construct(string $appName, AttachmentService $attachmentService, GuestManager $guestManager, MessageParser $messageParser, + RoomShareProvider $shareProvider, IManager $autoCompleteManager, IUserStatusManager $statusManager, MatterbridgeManager $matterbridgeManager, @@ -119,6 +122,7 @@ public function __construct(string $appName, $this->attachmentService = $attachmentService; $this->guestManager = $guestManager; $this->messageParser = $messageParser; + $this->shareProvider = $shareProvider; $this->autoCompleteManager = $autoCompleteManager; $this->statusManager = $statusManager; $this->matterbridgeManager = $matterbridgeManager; @@ -305,6 +309,33 @@ public function shareObjectToChat(string $objectType, string $objectId, string $ return $this->parseCommentToResponse($comment); } + /* + * Gather share IDs from the comments and preload share definitions + * to avoid separate database query for each individual share. + * + * @param IComment[] $comments + */ + protected function preloadShares(array $comments): void { + // Scan messages for share IDs + $shareIds = []; + foreach ($comments as $comment) { + $verb = $comment->getVerb(); + if ($verb === 'object_shared') { + $message = $comment->getMessage(); + $data = json_decode($message, true); + if (isset($data['parameters']['share'])) { + $shareIds[] = $data['parameters']['share']; + } + } + } + if (!empty($shareIds)) { + // Ignore the result for now. Retrieved Share objects will be cached by + // the RoomShareProvider and returned from the cache to + // the Parser\SystemMessage without additional database queries. + $this->shareProvider->getSharesByIds($shareIds); + } + } + /** * @PublicPage * @RequireParticipant @@ -432,6 +463,8 @@ public function receiveMessages(int $lookIntoFuture, return $response; } + $this->preloadShares($comments); + $i = 0; $messages = $commentIdToIndex = $parentIds = []; foreach ($comments as $comment) { diff --git a/lib/Share/RoomShareProvider.php b/lib/Share/RoomShareProvider.php index 0494fc00c80..54b3550951c 100644 --- a/lib/Share/RoomShareProvider.php +++ b/lib/Share/RoomShareProvider.php @@ -38,6 +38,7 @@ use OCA\Talk\Room; use OCA\Talk\Service\ParticipantService; use OCP\AppFramework\Utility\ITimeFactory; +use OCP\Cache\CappedMemoryCache; use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\EventDispatcher\IEventDispatcher; use OCP\Files\Folder; @@ -83,6 +84,8 @@ class RoomShareProvider implements IShareProvider { private IL10N $l; private IMimeTypeLoader $mimeTypeLoader; + private CappedMemoryCache $sharesByIdCache; + public function __construct( IDBConnection $connection, ISecureRandom $secureRandom, @@ -103,6 +106,14 @@ public function __construct( $this->timeFactory = $timeFactory; $this->l = $l; $this->mimeTypeLoader = $mimeTypeLoader; + $this->sharesByIdCache = new CappedMemoryCache(); + } + + /* + * Clean sharesByIdCache + */ + private function cleanSharesByIdCache(): void { + $this->sharesByIdCache = new CappedMemoryCache(); } public static function register(IEventDispatcher $dispatcher): void { @@ -314,6 +325,8 @@ private function createShareObject(array $data): IShare { * @return IShare The share object */ public function update(IShare $share): IShare { + $this->cleanSharesByIdCache(); + $update = $this->dbConnection->getQueryBuilder(); $update->update('share') ->where($update->expr()->eq('id', $update->createNamedParameter($share->getId()))) @@ -357,6 +370,8 @@ public function update(IShare $share): IShare { * @param IShare $share */ public function delete(IShare $share): void { + $this->cleanSharesByIdCache(); + $delete = $this->dbConnection->getQueryBuilder(); $delete->delete('share') ->where($delete->expr()->eq('id', $delete->createNamedParameter($share->getId()))); @@ -376,6 +391,8 @@ public function delete(IShare $share): void { * @param string $recipient UserId of the recipient */ public function deleteFromSelf(IShare $share, $recipient): void { + $this->cleanSharesByIdCache(); + // Check if there is a userroom share $qb = $this->dbConnection->getQueryBuilder(); $stmt = $qb->select(['id', 'permissions']) @@ -428,6 +445,8 @@ public function deleteFromSelf(IShare $share, $recipient): void { * @throws GenericShareException In case the share could not be restored */ public function restore(IShare $share, string $recipient): IShare { + $this->cleanSharesByIdCache(); + $qb = $this->dbConnection->getQueryBuilder(); $qb->select('permissions') ->from('share') @@ -468,6 +487,8 @@ public function restore(IShare $share, string $recipient): IShare { * @return IShare */ public function move(IShare $share, $recipient): IShare { + $this->cleanSharesByIdCache(); + // Check if there is a userroom share $qb = $this->dbConnection->getQueryBuilder(); $stmt = $qb->select('id') @@ -631,6 +652,34 @@ public function getSharesBy($userId, $shareType, $node, $reshares, $limit, $offs * @throws ShareNotFound */ public function getShareById($id, $recipientId = null): IShare { + if (($recipientId === null) && isset($this->sharesByIdCache[$id])) { + $share = $this->sharesByIdCache[$id]; + } else { + $shares = $this->getSharesByIds([$id], $recipientId); + if (empty($shares)) { + throw new ShareNotFound(); + } + $share = $shares[0]; + } + + // Shares referring to deleted files are stored as 'false' in the cache. + if ($share === false) { + throw new ShareNotFound(); + } + + return $share; + } + + /** + * Get shares by ids + * + * Not part of IShareProvider API, but needed by OCA\Talk\Controller\ChatController. + * + * @param int[] $ids + * @param string|null $recipientId + * @return IShare[] + */ + public function getSharesByIds(array $ids, ?string $recipientId = null): array { $qb = $this->dbConnection->getQueryBuilder(); $qb->select('s.*', 'f.fileid', 'f.path', 'f.permissions AS f_permissions', 'f.storage', 'f.path_hash', @@ -641,28 +690,40 @@ public function getShareById($id, $recipientId = null): IShare { ->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')) - ->where($qb->expr()->eq('s.id', $qb->createNamedParameter($id))) + ->where($qb->expr()->in('s.id', $qb->createNamedParameter($ids, IQueryBuilder::PARAM_INT_ARRAY))) ->andWhere($qb->expr()->eq('s.share_type', $qb->createNamedParameter(IShare::TYPE_ROOM))); $cursor = $qb->executeQuery(); - $data = $cursor->fetch(); - $cursor->closeCursor(); - if ($data === false) { - throw new ShareNotFound(); - } - - if (!$this->isAccessibleResult($data)) { - throw new ShareNotFound(); + /* + * Keep retrieved shares in sharesByIdCache. + * + * Fill the cache only when $recipientId === null. + * + * For inaccessible shares use 'false' instead of the IShare object. + * (This is required to avoid additional queries in getShareById when + * the share refers to a deleted file.) + */ + $shares = []; + while ($data = $cursor->fetch()) { + $id = $data['id']; + if ($this->isAccessibleResult($data)) { + $share = $this->createShareObject($data); + $shares[] = $share; + } else { + $share = false; + } + if ($recipientId === null && !isset($this->sharesByIdCache[$id])) { + $this->sharesByIdCache[$id] = $share; + } } - - $share = $this->createShareObject($data); + $cursor->closeCursor(); if ($recipientId !== null) { - $share = $this->resolveSharesForRecipient([$share], $recipientId)[0]; + return $this->resolveSharesForRecipient($shares, $recipientId); + } else { + return $shares; } - - return $share; } /** @@ -1056,6 +1117,8 @@ public function getChildren(IShare $parent): array { * @param string|null $user */ public function deleteInRoom(string $roomToken, string $user = null): void { + $this->cleanSharesByIdCache(); + //First delete all custom room shares for the original shares to be removed $qb = $this->dbConnection->getQueryBuilder(); $qb->select('id') diff --git a/tests/php/Controller/ChatControllerTest.php b/tests/php/Controller/ChatControllerTest.php index 9a79d091612..b1d5ec4dfbc 100644 --- a/tests/php/Controller/ChatControllerTest.php +++ b/tests/php/Controller/ChatControllerTest.php @@ -37,6 +37,7 @@ use OCA\Talk\Service\AttachmentService; use OCA\Talk\Service\ParticipantService; use OCA\Talk\Service\SessionService; +use OCA\Talk\Share\RoomShareProvider; use OCP\App\IAppManager; use OCP\AppFramework\Http; use OCP\AppFramework\Http\DataResponse; @@ -76,6 +77,8 @@ class ChatControllerTest extends TestCase { protected $guestManager; /** @var MessageParser|MockObject */ protected $messageParser; + /** @var RoomShareProvider|MockObject */ + protected $roomShareProvider; /** @var IManager|MockObject */ protected $autoCompleteManager; /** @var IUserStatusManager|MockObject */ @@ -118,6 +121,7 @@ public function setUp(): void { $this->attachmentService = $this->createMock(AttachmentService::class); $this->guestManager = $this->createMock(GuestManager::class); $this->messageParser = $this->createMock(MessageParser::class); + $this->roomShareProvider = $this->createMock(RoomShareProvider::class); $this->autoCompleteManager = $this->createMock(IManager::class); $this->statusManager = $this->createMock(IUserStatusManager::class); $this->matterbridgeManager = $this->createMock(MatterbridgeManager::class); @@ -155,6 +159,7 @@ private function recreateChatController() { $this->attachmentService, $this->guestManager, $this->messageParser, + $this->roomShareProvider, $this->autoCompleteManager, $this->statusManager, $this->matterbridgeManager,