Reduce number of queries required to read shares

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,31 @@ 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.
+	 */
+	protected function preloadShares(array $comments) {
+		// 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 +461,8 @@ public function receiveMessages(int $lookIntoFuture,
 			return $response;
 		}
 
+		$this->preloadShares($comments);
+
 		$i = 0;
 		$messages = $commentIdToIndex = $parentIds = [];
 		foreach ($comments as $comment) {

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() {
+		$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,35 @@ 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];
+		}
+
+		if ($share === false) {
+			// Shares referring to deleted files are stored as 'false',
+			// both in the cache and in the array returned from getSharesByIds.
+			throw new ShareNotFound();
+		}
+
+		return $share;
+	}
+
+	/**
+	 * Get shares by ids
+	 *
+	 * Not part of IShareProvider API, but needed by OCA\Talk\Controller\ChatController.
+	 *
+	 * @param int[] $id
+	 * @param string|null $recipientId
+	 * @return (IShare|false)[]
+	 */
+	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 +691,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 +1118,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')

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,